Skip to content

refactor(levm): improve and simplify some db functions#2651

Merged
JereSalo merged 16 commits intomainfrom
levm/improve_some_db_functions
May 6, 2025
Merged

refactor(levm): improve and simplify some db functions#2651
JereSalo merged 16 commits intomainfrom
levm/improve_some_db_functions

Conversation

@JereSalo
Copy link
Copy Markdown
Contributor

@JereSalo JereSalo commented Apr 30, 2025 •
edited
Loading

Motivation

  • Try to remove account_exists if possible because it adds complexity and unnecessary checks to the DB.
  • Try to finally remove get_account_no_push_cache, which is related to the previous thing too.

Description

  • We now ignore a specific test because EIP-7702 spec has changed and we no longer need to check if the account exists in the trie.
  • Remove Option from specific_tests
  • Remove get_account_no_push_cache and the usage of account_exists in LEVM. This method is not deleted from the Database because it's used in get_state_transitions, and even here it could be removed but I think it is better to keep it in this PR and maybe decide later what to do with this function. (If we remove it it wouldn't make a difference to the state though).
  • We were able to remove a SpuriousDragon check because we don't support pre-merge forks now

Note: account_exists hasn't been completely removed from Database because we use it in get_state_transitions but that is going to change soon and we'll be able to remove it.

Closes #issue_number

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 30, 2025 •
edited
Loading

Lines of code report

Total lines added: 10
Total lines removed: 32
Total lines changed: 42

Detailed view 
+-----------------------------------------------------+-------+------+
| File                                                | Lines | Diff |
+-----------------------------------------------------+-------+------+
| ethrex/cmd/ef_tests/state/parser.rs                 | 160   | +3   |
+-----------------------------------------------------+-------+------+
| ethrex/cmd/ef_tests/state/runner/mod.rs             | 238   | +7   |
+-----------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/db/gen_db.rs              | 182   | -6   |
+-----------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/gas_cost.rs               | 1128  | -3   |
+-----------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/opcode_handlers/system.rs | 831   | -11  |
+-----------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/utils.rs                  | 414   | -12  |
+-----------------------------------------------------+-------+------+

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 30, 2025 •
edited
Loading

Benchmark Results Comparison

PR Results

Benchmark Results: Factorial

Command Mean [ms] Min [ms] Max [ms] Relative
revm_Factorial 240.3 ± 1.5 239.0 244.0 1.00
levm_Factorial 891.7 ± 40.5 870.1 1005.4 3.71 ± 0.17

Benchmark Results: Factorial - Recursive

Command Mean [s] Min [s] Max [s] Relative
revm_FactorialRecursive 1.528 ± 0.064 1.446 1.662 1.00
levm_FactorialRecursive 13.348 ± 0.282 12.963 13.981 8.73 ± 0.41

Benchmark Results: Fibonacci

Command Mean [ms] Min [ms] Max [ms] Relative
revm_Fibonacci 210.5 ± 1.0 208.4 211.8 1.00
levm_Fibonacci 880.4 ± 20.2 865.2 935.5 4.18 ± 0.10

Benchmark Results: ManyHashes

Command Mean [ms] Min [ms] Max [ms] Relative
revm_ManyHashes 8.7 ± 0.1 8.6 8.8 1.00
levm_ManyHashes 17.5 ± 0.1 17.3 17.7 2.01 ± 0.02

Benchmark Results: BubbleSort

Command Mean [s] Min [s] Max [s] Relative
revm_BubbleSort 3.251 ± 0.023 3.233 3.306 1.00
levm_BubbleSort 5.760 ± 0.064 5.680 5.898 1.77 ± 0.02

Benchmark Results: ERC20 - Transfer

Command Mean [ms] Min [ms] Max [ms] Relative
revm_ERC20Transfer 247.0 ± 2.2 245.3 251.3 1.00
levm_ERC20Transfer 506.6 ± 3.9 501.8 511.6 2.05 ± 0.02

Benchmark Results: ERC20 - Mint

Command Mean [ms] Min [ms] Max [ms] Relative
revm_ERC20Mint 140.7 ± 0.7 139.9 142.0 1.00
levm_ERC20Mint 320.9 ± 2.7 317.0 325.4 2.28 ± 0.02

Benchmark Results: ERC20 - Approval

Command Mean [s] Min [s] Max [s] Relative
revm_ERC20Approval 1.032 ± 0.010 1.024 1.055 1.00
levm_ERC20Approval 1.927 ± 0.031 1.900 2.010 1.87 ± 0.03

Main Results

Benchmark Results: Factorial

Command Mean [ms] Min [ms] Max [ms] Relative
revm_Factorial 245.5 ± 30.3 234.9 331.7 1.00
levm_Factorial 872.7 ± 3.6 868.2 878.6 3.55 ± 0.44

Benchmark Results: Factorial - Recursive

Command Mean [s] Min [s] Max [s] Relative
revm_FactorialRecursive 1.499 ± 0.077 1.397 1.644 1.00
levm_FactorialRecursive 13.298 ± 0.251 13.021 13.806 8.87 ± 0.49

Benchmark Results: Fibonacci

Command Mean [ms] Min [ms] Max [ms] Relative
revm_Fibonacci 208.5 ± 3.7 198.2 210.7 1.00
levm_Fibonacci 876.4 ± 16.8 864.1 919.3 4.20 ± 0.11

Benchmark Results: ManyHashes

Command Mean [ms] Min [ms] Max [ms] Relative
revm_ManyHashes 8.5 ± 0.0 8.5 8.5 1.00
levm_ManyHashes 17.9 ± 0.2 17.8 18.4 2.11 ± 0.03

Benchmark Results: BubbleSort

Command Mean [s] Min [s] Max [s] Relative
revm_BubbleSort 3.252 ± 0.013 3.232 3.272 1.00
levm_BubbleSort 5.992 ± 0.137 5.917 6.371 1.84 ± 0.04

Benchmark Results: ERC20 - Transfer

Command Mean [ms] Min [ms] Max [ms] Relative
revm_ERC20Transfer 249.5 ± 1.7 247.2 253.3 1.00
levm_ERC20Transfer 507.1 ± 3.0 502.2 511.7 2.03 ± 0.02

Benchmark Results: ERC20 - Mint

Command Mean [ms] Min [ms] Max [ms] Relative
revm_ERC20Mint 142.8 ± 2.8 141.3 150.6 1.00
levm_ERC20Mint 327.1 ± 8.5 319.3 346.8 2.29 ± 0.07

Benchmark Results: ERC20 - Approval

Command Mean [s] Min [s] Max [s] Relative
revm_ERC20Approval 1.040 ± 0.008 1.033 1.060 1.00
levm_ERC20Approval 1.928 ± 0.016 1.910 1.965 1.85 ± 0.02

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 30, 2025 •
edited
Loading

EF Tests Comparison

Test Name MAIN PR DIFF
Summary: 18910/18910 (100.00%) 18909/18909 (100.00%) ➖️ -1
Prague: 5202/5202 (100.00%) 5201/5201 (100.00%) ➖️ -1
Cancun: 7608/7608 (100.00%) 7608/7608 (100.00%) ➖️
Shanghai: 3214/3214 (100.00%) 3214/3214 (100.00%) ➖️
Paris: 2886/2886 (100.00%) 2886/2886 (100.00%) ➖️
London: 0/0 (NaN%) 0/0 (NaN%) ➖️
Berlin: 0/0 (NaN%) 0/0 (NaN%) ➖️
Istanbul: 0/0 (NaN%) 0/0 (NaN%) ➖️
Petersburg: 0/0 (NaN%) 0/0 (NaN%) ➖️
Constantinople: 0/0 (NaN%) 0/0 (NaN%) ➖️
Byzantium: 0/0 (NaN%) 0/0 (NaN%) ➖️
SpuriousDragon: 0/0 (NaN%) 0/0 (NaN%) ➖️
Tangerine: 0/0 (NaN%) 0/0 (NaN%) ➖️
Homestead: 0/0 (NaN%) 0/0 (NaN%) ➖️
Frontier: 0/0 (NaN%) 0/0 (NaN%) ➖️

@JereSalo JereSalo marked this pull request as ready for review April 30, 2025 21:26
@JereSalo JereSalo requested a review from a team as a code owner April 30, 2025 21:26
rodrigo-o
rodrigo-o approved these changes May 6, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@rodrigo-o rodrigo-o left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just a brief question, the _no_push_cache version was just needed because of the account_exists? that's what I infer from the desc but wanted to be sure.

Comment thread crates/vm/levm/src/gas_cost.rs
Comment on lines -861 to -864
// https://eips.ethereum.org/EIPS/eip-161 change the definition of empty address
let value_to_empty_account = if (!address_exists && fork < Fork::SpuriousDragon)
|| address_is_empty && !value_to_transfer.is_zero() && fork >= Fork::SpuriousDragon
{
Copy link
Copy Markdown
Contributor

@rodrigo-o rodrigo-o May 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Probably worth mentioning in the description that we were able to remove this due to stop supporting pre-merge forks.

Copy link
Copy Markdown
Contributor Author

@JereSalo JereSalo May 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. I should've mentioned that

@JereSalo
Copy link
Copy Markdown
Contributor Author

JereSalo commented May 6, 2025 •
edited
Loading

LGTM. Just a brief question, the _no_push_cache version was just needed because of the account_exists? that's what I infer from the desc but wanted to be sure.

Yes, we were actually doing some weird things with get_account_no_push_cache and account_exists that actually are hard to follow. Getting an account and not pushing it to cache shouldn't ever happen but if we just replaced it for a simple get_account that pushes to the cache then the behavior of that function changed because account_exists relied on the cache not having that account.
IMO it was all held together with duct tape and error-prone but it made single transactions tests pass. I'm sure we would've ran into issues with the previous implementation

@JereSalo JereSalo added this pull request to the merge queue May 6, 2025
Merged via the queue into main with commit e08e959 May 6, 2025
37 checks passed
@JereSalo JereSalo deleted the levm/improve_some_db_functions branch May 6, 2025 15:30
pedrobergamini pushed a commit to pedrobergamini/ethrex that referenced this pull request Aug 24, 2025
@JereSalo
refactor(levm): improve and simplify some db functions (lambdaclass#2651
6564f13 
)

**Motivation**

<!-- Why does this pull request exist? What are its goals? -->
- Try to remove `account_exists` if possible because it adds complexity
and unnecessary checks to the DB.
- Try to finally remove `get_account_no_push_cache`, which is related to
the previous thing too.

**Description**

- We now ignore a specific test because [EIP-7702 spec has
changed](ethereum/EIPs#9710) and we no longer
need to check if the account exists in the trie.
- Remove `Option` from `specific_tests`
- Remove `get_account_no_push_cache` and the usage of `account_exists`
in LEVM. This method is not deleted from the Database because it's used
in `get_state_transitions`, and even here it could be removed but I
think it is better to keep it in this PR and maybe decide later what to
do with this function. (If we remove it it wouldn't make a difference to
the state though).
- We were able to remove a SpuriousDragon check because we don't support
pre-merge forks now


Note: `account_exists` hasn't been completely removed from `Database`
because we use it in `get_state_transitions` but that is going to change
soon and we'll be able to remove it.
<!-- Link to issues: Resolves lambdaclass#111, Resolves lambdaclass#222 -->

Closes #issue_number
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rodrigo-o rodrigo-o rodrigo-o approved these changes

+1 more reviewer

@fmoletta fmoletta fmoletta approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@JereSalo @rodrigo-o @fmoletta