Skip to content

docs(levm): added Rust docs and modified handle_return function to make it more d…#2703

Closed
SDartayet wants to merge 1 commit intomainfrom
add-docs-and-comments
Closed

docs(levm): added Rust docs and modified handle_return function to make it more d…#2703
SDartayet wants to merge 1 commit intomainfrom
add-docs-and-comments

Conversation

@SDartayet
Copy link
Copy Markdown
Contributor

Motivation

Making the code easier to understand.

Description

Some comments were added to the main VM code. Some Rust docs were added to various data structures too to make them easier to understand.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 8, 2025

Lines of code report

Total lines added: 13
Total lines removed: 10
Total lines changed: 23

Detailed view
+-----------------------------------------------------+-------+------+
| File                                                | Lines | Diff |
+-----------------------------------------------------+-------+------+
| ethrex/crates/common/trie/node_hash.rs              | 129   | +2   |
+-----------------------------------------------------+-------+------+
| ethrex/crates/common/types/blobs_bundle.rs          | 324   | -3   |
+-----------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/discv4/server.rs       | 682   | -3   |
+-----------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/engine/payload.rs      | 729   | +5   |
+-----------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/opcode_handlers/system.rs | 827   | -4   |
+-----------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/vm.rs                     | 392   | +6   |
+-----------------------------------------------------+-------+------+

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 8, 2025

Benchmark Results Comparison

PR Results

Benchmark Results: Factorial

Command Mean [ms] Min [ms] Max [ms] Relative
revm_Factorial 241.3 ± 1.5 239.9 245.0 1.00
levm_Factorial 876.8 ± 13.9 868.3 913.8 3.63 ± 0.06

Benchmark Results: Factorial - Recursive

Command Mean [s] Min [s] Max [s] Relative
revm_FactorialRecursive 1.502 ± 0.093 1.405 1.651 1.00
levm_FactorialRecursive 13.356 ± 0.242 13.069 13.598 8.89 ± 0.58

Benchmark Results: Fibonacci

Command Mean [ms] Min [ms] Max [ms] Relative
revm_Fibonacci 212.6 ± 0.6 211.9 214.0 1.00
levm_Fibonacci 872.0 ± 8.4 863.6 886.1 4.10 ± 0.04

Benchmark Results: ManyHashes

Command Mean [ms] Min [ms] Max [ms] Relative
revm_ManyHashes 8.7 ± 0.1 8.5 8.9 1.00
levm_ManyHashes 17.4 ± 0.4 17.2 18.4 2.00 ± 0.05

Benchmark Results: BubbleSort

Command Mean [s] Min [s] Max [s] Relative
revm_BubbleSort 3.236 ± 0.023 3.211 3.287 1.00
levm_BubbleSort 5.793 ± 0.048 5.731 5.882 1.79 ± 0.02

Benchmark Results: ERC20 - Transfer

Command Mean [ms] Min [ms] Max [ms] Relative
revm_ERC20Transfer 249.5 ± 6.4 245.6 266.5 1.00
levm_ERC20Transfer 505.1 ± 3.1 501.1 509.6 2.02 ± 0.05

Benchmark Results: ERC20 - Mint

Command Mean [ms] Min [ms] Max [ms] Relative
revm_ERC20Mint 142.1 ± 0.5 141.6 143.4 1.00
levm_ERC20Mint 324.0 ± 12.4 316.3 358.5 2.28 ± 0.09

Benchmark Results: ERC20 - Approval

Command Mean [s] Min [s] Max [s] Relative
revm_ERC20Approval 1.030 ± 0.023 1.017 1.093 1.00
levm_ERC20Approval 1.934 ± 0.014 1.912 1.953 1.88 ± 0.04

Main Results

Benchmark Results: Factorial

Command Mean [ms] Min [ms] Max [ms] Relative
revm_Factorial 239.2 ± 1.4 237.9 241.7 1.00
levm_Factorial 874.8 ± 5.1 867.4 887.0 3.66 ± 0.03

Benchmark Results: Factorial - Recursive

Command Mean [s] Min [s] Max [s] Relative
revm_FactorialRecursive 1.509 ± 0.089 1.409 1.659 1.00
levm_FactorialRecursive 13.150 ± 0.220 12.951 13.486 8.72 ± 0.54

Benchmark Results: Fibonacci

Command Mean [ms] Min [ms] Max [ms] Relative
revm_Fibonacci 229.3 ± 26.1 217.8 303.0 1.00
levm_Fibonacci 887.5 ± 19.1 871.2 933.0 3.87 ± 0.45

Benchmark Results: ManyHashes

Command Mean [ms] Min [ms] Max [ms] Relative
revm_ManyHashes 8.8 ± 0.1 8.7 8.9 1.00
levm_ManyHashes 17.8 ± 0.2 17.5 18.2 2.02 ± 0.03

Benchmark Results: BubbleSort

Command Mean [s] Min [s] Max [s] Relative
revm_BubbleSort 3.200 ± 0.011 3.185 3.219 1.00
levm_BubbleSort 5.745 ± 0.038 5.684 5.808 1.80 ± 0.01

Benchmark Results: ERC20 - Transfer

Command Mean [ms] Min [ms] Max [ms] Relative
revm_ERC20Transfer 246.8 ± 2.9 245.0 254.6 1.00
levm_ERC20Transfer 502.2 ± 2.8 498.4 507.4 2.04 ± 0.03

Benchmark Results: ERC20 - Mint

Command Mean [ms] Min [ms] Max [ms] Relative
revm_ERC20Mint 141.1 ± 0.5 140.5 142.1 1.00
levm_ERC20Mint 320.0 ± 3.2 314.6 325.0 2.27 ± 0.02

Benchmark Results: ERC20 - Approval

Command Mean [s] Min [s] Max [s] Relative
revm_ERC20Approval 1.033 ± 0.010 1.024 1.056 1.00
levm_ERC20Approval 1.912 ± 0.014 1.888 1.929 1.85 ± 0.02

@JereSalo JereSalo closed this May 17, 2025
edg-l added a commit that referenced this pull request Apr 23, 2026
Brings ethrex up to bal-devnet-4 fixture spec. Rolls up EIP-7928,
EIP-8037, EIP-7976, EIP-7981, EIP-7708 and misc BAL validation fixes
into one change set.

BAL (EIP-7928)
- Widen BlockAccessIndex and related recorder/index fields to u32.
- Shadow BAL recorder on per-tx tx_dbs in the parallel validator:
  diff touched_addresses / storage_reads against header BAL to catch
  missing pure-access entries and missing storage_reads.
- Fall back to pre-state code_hash in validate_tx_execution PART B
  when the BAL has no code_changes entry (missing_code_change).
- Stop whitelisting SYSTEM_ADDRESS from unaccessed_pure_accounts via
  system_seed / current_accounts_state scrubs; user-tx touches still
  remove it via the per-tx tracked_accounts path.

EIP-8037 (state gas 2D accounting)
- Dynamic cost_per_state_byte(block_gas_limit), Amsterdam only.
- Two-counter reservoir: state_gas_spill_outstanding +
  state_gas_credit_against_drain for correct revert math across
  nested sub-calls (PR #2733 clamp-and-spill).
- Per-tx 2D inclusion check (PR #2703) in sequential + parallel
  paths: reject with GAS_ALLOWANCE_EXCEEDED when tx.gas worst-case
  exceeds remaining block regular/state budget.
- intrinsic_state_gas immutable across the tx (PR #2711) and
  subtracted separately when deriving block-dimensional regular gas.
- CREATE collision/early/child failure refunds account state gas.
- Same-tx SELFDESTRUCT refunds state gas clamped against
  execution-only state gas (PR #2707), not total state_gas_used.
- Revert-path reservoir refill uses the PR #2733 X - Z formula.
- Top-level reservoir reset on tx failure (PR #2689).
- Zero gas_remaining on precompile exceptional halt so block
  accounting sees the full intrinsic.

Calldata / access-list floors
- TOTAL_COST_FLOOR_PER_TOKEN 10 -> 16 under Amsterdam (EIP-7976).
- Access-list data bytes fold into floor-token count (EIP-7981).

EIP-7708
- Lex-ordered burn logs, no coinbase priority-fee log, SELFDESTRUCT-
  destination coalescing.

Tests
- New levm tests for EIP-7976/7981, EIP-8037 refund/code-deposit/
  top-level-failure paths.
- Skip 6 zkevm@v0.3.0 EIP-8025 fixtures filled against bal@v5.6.1
  (re-enable once zkevm@v0.4.x ships).

Hive consume-engine amsterdam: 1339 pass, 3 remaining (withdrawal
missing-entry cases addressed by PR #6463, cherry-pick pending).
edg-l added a commit that referenced this pull request Apr 23, 2026
Brings ethrex up to bal-devnet-4 fixture spec. Rolls up EIP-7928,
EIP-8037, EIP-7976, EIP-7981, EIP-7708 and misc BAL validation fixes
into one change set.

BAL (EIP-7928)
- Widen BlockAccessIndex and related recorder/index fields to u32.
- Shadow BAL recorder on per-tx tx_dbs in the parallel validator:
  diff touched_addresses / storage_reads against header BAL to catch
  missing pure-access entries and missing storage_reads.
- Fall back to pre-state code_hash in validate_tx_execution PART B
  when the BAL has no code_changes entry (missing_code_change).
- Stop whitelisting SYSTEM_ADDRESS from unaccessed_pure_accounts via
  system_seed / current_accounts_state scrubs; user-tx touches still
  remove it via the per-tx tracked_accounts path.

EIP-8037 (state gas 2D accounting)
- Dynamic cost_per_state_byte(block_gas_limit), Amsterdam only.
- Two-counter reservoir: state_gas_spill_outstanding +
  state_gas_credit_against_drain for correct revert math across
  nested sub-calls (PR #2733 clamp-and-spill).
- Per-tx 2D inclusion check (PR #2703) in sequential + parallel
  paths: reject with GAS_ALLOWANCE_EXCEEDED when tx.gas worst-case
  exceeds remaining block regular/state budget.
- intrinsic_state_gas immutable across the tx (PR #2711) and
  subtracted separately when deriving block-dimensional regular gas.
- CREATE collision/early/child failure refunds account state gas.
- Same-tx SELFDESTRUCT refunds state gas clamped against
  execution-only state gas (PR #2707), not total state_gas_used.
- Revert-path reservoir refill uses the PR #2733 X - Z formula.
- Top-level reservoir reset on tx failure (PR #2689).
- Zero gas_remaining on precompile exceptional halt so block
  accounting sees the full intrinsic.

Calldata / access-list floors
- TOTAL_COST_FLOOR_PER_TOKEN 10 -> 16 under Amsterdam (EIP-7976).
- Access-list data bytes fold into floor-token count (EIP-7981).

EIP-7708
- Lex-ordered burn logs, no coinbase priority-fee log, SELFDESTRUCT-
  destination coalescing.

Tests
- New levm tests for EIP-7976/7981, EIP-8037 refund/code-deposit/
  top-level-failure paths.
- Skip 6 zkevm@v0.3.0 EIP-8025 fixtures filled against bal@v5.6.1
  (re-enable once zkevm@v0.4.x ships).

Hive consume-engine amsterdam: 1339 pass, 3 remaining (withdrawal
missing-entry cases addressed by PR #6463, cherry-pick pending).
edg-l added a commit that referenced this pull request Apr 23, 2026
Addresses miss-slot risks found in the builder/validator parity audit of
the bal-devnet-4 rollup. Three builder-side paths could produce blocks
the validator rejects, plus minor hardening.

- Mempool intrinsic gas was using `TX_CREATE_GAS_COST = 53000`
  unconditionally for CREATE. Under Amsterdam the VM charges the
  `(regular, state)` split derived from `intrinsic_gas_dimensions`
  (`REGULAR_GAS_CREATE + STATE_BYTES_PER_NEW_ACCOUNT * cpsb`). Route
  through the shared helper for Amsterdam+ so admission matches VM charge.

- Payload builder (`fill_transactions`) had no EIP-8037 PR #2703 per-tx
  2D inclusion check. A tx passing execution in the builder could still
  fail the check in the validator's aggregation loop and invalidate the
  block. Expose `check_2d_gas_allowance` as pub and call it before any
  BAL touches so rejected txs contribute nothing.

- L2 payload builder recorded sender/recipient BAL touches before
  executing, with no checkpoint/restore for the `undo_last_tx` path
  (invalid L2 out-message) or apply-tx error. Mirror the L1 builder:
  take a `bal_checkpoint` after `set_bal_index`, restore on both
  rejection paths.

- `execute_block_parallel` now computes `is_amsterdam` locally and
  explicitly gates the 2D inclusion loop, keeping the Amsterdam-only
  invariant checkable rather than implicit in the caller.

- `check_2d_gas_allowance` doc now explains why our
  `block_regular_gas_used` aggregates `max(raw_regular, floor)` at
  tx-report time (matching EELS `block_output.block_gas_used`).

- Post-exec 2D overflow rollback in `apply_plain_transaction` replaces
  the unchecked `-=` on `cumulative_gas_spent` with `saturating_sub` +
  `debug_assert`.

- Missing-code-change per-tx BAL validation: when a BAL account has no
  `code_changes` entry (`seeded_pos == 0`), fall back to the
  `system_seed` / store pre-state code_hash. Fixes the remaining
  `missing_code_change` Hive test.

Hive consume-engine amsterdam: 1340 pass, 2 remaining (withdrawal
missing-entry cases addressed by PR #6463).
edg-l added a commit that referenced this pull request Apr 23, 2026
Brings ethrex up to bal-devnet-4 fixture spec. Rolls up EIP-7928,
EIP-8037, EIP-7976, EIP-7981, EIP-7708 and misc BAL validation fixes
into one change set.

BAL (EIP-7928)
- Widen BlockAccessIndex and related recorder/index fields to u32.
- Shadow BAL recorder on per-tx tx_dbs in the parallel validator:
  diff touched_addresses / storage_reads against header BAL to catch
  missing pure-access entries and missing storage_reads.
- Fall back to pre-state code_hash in validate_tx_execution PART B
  when the BAL has no code_changes entry (missing_code_change).
- Stop whitelisting SYSTEM_ADDRESS from unaccessed_pure_accounts via
  system_seed / current_accounts_state scrubs; user-tx touches still
  remove it via the per-tx tracked_accounts path.

EIP-8037 (state gas 2D accounting)
- Dynamic cost_per_state_byte(block_gas_limit), Amsterdam only.
- Two-counter reservoir: state_gas_spill_outstanding +
  state_gas_credit_against_drain for correct revert math across
  nested sub-calls (PR #2733 clamp-and-spill).
- Per-tx 2D inclusion check (PR #2703) in sequential + parallel
  paths: reject with GAS_ALLOWANCE_EXCEEDED when tx.gas worst-case
  exceeds remaining block regular/state budget.
- intrinsic_state_gas immutable across the tx (PR #2711) and
  subtracted separately when deriving block-dimensional regular gas.
- CREATE collision/early/child failure refunds account state gas.
- Same-tx SELFDESTRUCT refunds state gas clamped against
  execution-only state gas (PR #2707), not total state_gas_used.
- Revert-path reservoir refill uses the PR #2733 X - Z formula.
- Top-level reservoir reset on tx failure (PR #2689).
- Zero gas_remaining on precompile exceptional halt so block
  accounting sees the full intrinsic.

Calldata / access-list floors
- TOTAL_COST_FLOOR_PER_TOKEN 10 -> 16 under Amsterdam (EIP-7976).
- Access-list data bytes fold into floor-token count (EIP-7981).

EIP-7708
- Lex-ordered burn logs, no coinbase priority-fee log, SELFDESTRUCT-
  destination coalescing.

Tests
- New levm tests for EIP-7976/7981, EIP-8037 refund/code-deposit/
  top-level-failure paths.
- Skip 6 zkevm@v0.3.0 EIP-8025 fixtures filled against bal@v5.6.1
  (re-enable once zkevm@v0.4.x ships).

Hive consume-engine amsterdam: 1339 pass, 3 remaining (withdrawal
missing-entry cases addressed by PR #6463, cherry-pick pending).
edg-l added a commit that referenced this pull request Apr 23, 2026
Addresses miss-slot risks found in the builder/validator parity audit of
the bal-devnet-4 rollup. Three builder-side paths could produce blocks
the validator rejects, plus minor hardening.

- Mempool intrinsic gas was using `TX_CREATE_GAS_COST = 53000`
  unconditionally for CREATE. Under Amsterdam the VM charges the
  `(regular, state)` split derived from `intrinsic_gas_dimensions`
  (`REGULAR_GAS_CREATE + STATE_BYTES_PER_NEW_ACCOUNT * cpsb`). Route
  through the shared helper for Amsterdam+ so admission matches VM charge.

- Payload builder (`fill_transactions`) had no EIP-8037 PR #2703 per-tx
  2D inclusion check. A tx passing execution in the builder could still
  fail the check in the validator's aggregation loop and invalidate the
  block. Expose `check_2d_gas_allowance` as pub and call it before any
  BAL touches so rejected txs contribute nothing.

- L2 payload builder recorded sender/recipient BAL touches before
  executing, with no checkpoint/restore for the `undo_last_tx` path
  (invalid L2 out-message) or apply-tx error. Mirror the L1 builder:
  take a `bal_checkpoint` after `set_bal_index`, restore on both
  rejection paths.

- `execute_block_parallel` now computes `is_amsterdam` locally and
  explicitly gates the 2D inclusion loop, keeping the Amsterdam-only
  invariant checkable rather than implicit in the caller.

- `check_2d_gas_allowance` doc now explains why our
  `block_regular_gas_used` aggregates `max(raw_regular, floor)` at
  tx-report time (matching EELS `block_output.block_gas_used`).

- Post-exec 2D overflow rollback in `apply_plain_transaction` replaces
  the unchecked `-=` on `cumulative_gas_spent` with `saturating_sub` +
  `debug_assert`.

- Missing-code-change per-tx BAL validation: when a BAL account has no
  `code_changes` entry (`seeded_pos == 0`), fall back to the
  `system_seed` / store pre-state code_hash. Fixes the remaining
  `missing_code_change` Hive test.

Hive consume-engine amsterdam: 1340 pass, 2 remaining (withdrawal
missing-entry cases addressed by PR #6463).
edg-l added a commit that referenced this pull request Apr 23, 2026
Two bot reviews (Codex, Claude) + one code reviewer (Greptile) flagged 7
issues. All 7 verified real and fixed in-PR per user request.

Critical (L2 consensus/liveness):

- `crates/l2/sequencer/block_producer/payload_builder.rs` now enforces
  the EIP-8037 PR #2703 per-tx 2D inclusion check against the L2's
  `configured_block_gas_limit` before executing each tx, matching the
  L1 builder. Without this, the L2 builder could reject valid txs or
  accept txs that violate one dimension of the block cap.

- `fill_transactions` now snapshots and restores
  `block_regular_gas_used` / `block_state_gas_used` around
  `apply_plain_transaction` on the `undo_last_tx` rollback path
  (invalid L2 out-message). Previously those two counters stayed
  inflated after a rejected tx, polluting `gas_used()` and the final
  header `gas_used`.

High (mempool DoS avenue):

- `crates/blockchain/mempool.rs::transaction_intrinsic_gas` now
  enforces `max(intrinsic_regular + intrinsic_state, floor)` for
  Amsterdam+, matching the VM's `validate_min_gas_limit` check.
  Previously a tx with mostly zero calldata could pass mempool
  admission at the weighted EIP-2028 cost (400 gas for 100 zero
  bytes) but fail the VM's 6400-gas unweighted floor at block
  inclusion, polluting the pool.

  New standalone helper `intrinsic_gas_floor(tx, fork)` added in
  `crates/vm/levm/src/utils.rs` mirroring `VM::get_min_gas_used` so
  the mempool / payload builder can compute the floor without a VM
  instance. Re-exported from `ethrex-vm`.

Medium:

- `crates/vm/backends/levm/mod.rs` withdrawal index computation
  switched from `.map(|n| n + 1)` to `.map(|n| n.saturating_add(1))`.
  The prior form wraps to 0 in release builds when `n == u32::MAX`
  (the `debug_assert` only fires in debug).

- `crates/vm/levm/src/opcode_handlers/system.rs` adds `debug_assert!`
  at the two reservoir-revert sites verifying
  `outstanding_delta >= credit_against_drain_delta`. If that invariant
  is ever violated, the `saturating_sub` silently mischarges the
  block's regular dimension; a loud debug panic is preferable.

Style:

- `crates/vm/levm/src/gas_cost.rs::access_list_bytes` — replace
  `keys.len() as u64` with `u64::try_from(...).unwrap_or(u64::MAX)`
  for consistency with the rest of the codebase.

- `crates/vm/levm/src/hooks/default_hook.rs::refund_sender` — rename
  the currently-unused `gas_used_pre_refund` parameter to
  `_gas_used_pre_refund` at the signature and drop the interior
  `let _ =` that was silencing it. Expanded doc explains it's kept
  in the signature for future reintroduction.

All 478 tests pass; no behavior changes except the three
intentional ones (mempool floor, L2 2D check, L2 counter rollback)
plus the already-exercised saturation edge.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants