Conversation
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).
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).
Regression guards for builder/validator drift on Amsterdam blocks. Both paths share the VM core but diverge in plumbing (mempool admission, shadow BAL recorder, 2D inclusion check, BAL checkpoint/restore, coinbase and SYSTEM_ADDRESS filters), and a disagreement means a missed slot on devnet that a green ef-tests run cannot catch — ef-tests only consume blocks, never produce them. Two groups of tests: Positive parity (9). Builder produces a legitimate block, validator pipeline (parallel, BAL-seeded) must accept. Covers: empty block with only pre-exec system calls; plain transfer; CREATE tx (Amsterdam intrinsic split); SSTORE state gas; BALANCE of untouched account (pure-access BAL entry); calldata floor (EIP-7976); access-list floor (EIP-7981); user-tx touch of SYSTEM_ADDRESS; multi-tx multi-sender aggregation. Negative parity (5). Builder produces a legitimate block, the test corrupts the BAL (drops / mutates / appends entries), re-hashes the header, and the validator must reject. Each scenario mirrors one of the Hive test_bal_invalid_* cases fixed in session 3: parity_reject_missing_pure_access_account parity_reject_surplus_system_address parity_reject_missing_storage_read parity_reject_missing_storage_change parity_reject_missing_code_change If one of the negative tests ever flips to "accept" the corresponding BAL-validation check has regressed; treat as P0.
Follow-up to PR #6518 addressing the test-gap list documented in the session-3 review. Covers every remaining item in TODO.md except the upstream zkevm@v0.4.x fixture re-enable (tracked externally). Tests (10 new): - `test_cpsb_clamp_to_one_for_tiny_gas_limit`, `test_cpsb_30m_bin_boundary` — cpsb quantization boundaries. Guards against an off-by-one in the `if quantized > CPSB_OFFSET` branch and against bin boundary regressions in the 5M-30M range. - `test_change_variants_rlp_roundtrip_index_above_u16_max` — RLP round-trip for all 4 BAL change variants at index 70_000, guarding against an accidental revert to the pre-devnet-4 `u16` type that would silently truncate high indices. - `amsterdam_create_intrinsic_matches_vm_dimensions` — mempool admission for Amsterdam CREATE txs must match the VM's `(regular, state)` split (TX_BASE + REGULAR_GAS_CREATE + STATE_BYTES_PER_NEW_ACCOUNT * cpsb), not the legacy 53000. - `test_intrinsic_parity_plain_transfer` / `test_intrinsic_parity_create_tx` / `test_intrinsic_parity_with_calldata_and_access_list` / `test_intrinsic_parity_eip7702_auth_list` — parity between the standalone `intrinsic_gas_dimensions` helper (used by mempool and payload builder) and `VM::get_intrinsic_gas` (used during execution). Run across Prague / Osaka / Amsterdam at 30M and 120M block gas limits. - `test_call_to_empty_account_with_value_retains_parent_state_gas` — EIP-8037 CALL-to-empty-with-value charges new-account state gas in the caller's frame, retained across successful parent continuation. Pairs with the existing `test_child_charge_then_revert_returns_state_gas_to_parent` for the revert direction. Code polish: - Clarifying comment on the `frame_outstanding_delta` invariant in `credit_state_gas_refund` (`crates/vm/levm/src/vm.rs`). The subtraction is fragile — documenting why it must read `state_gas_spill_outstanding` and not `state_gas_spill`. - `debug_assert!` guards on tx count vs `u32::MAX` at each block-exec entry (`execute_block`, `execute_block_pipeline`), keeping the EIP-7928 `BlockAccessIndex` invariant explicit rather than implicit in the ~10 downstream `u32::try_from(...).unwrap_or(u32::MAX)` sites. Docs: - `docs/roadmaps/forks-roadmap.md` — EIP-7976 / EIP-7981 flipped 🔴→✅, EIP-8037 status line expanded (dynamic cpsb, clamp-and-spill, 2D inclusion, same-tx SELFDESTRUCT refund), priority note updated for bal-devnet-4 + PR #6518. All 478 tests pass. No behavior changes — these are regression guards and documentation for the bal-devnet-4 work landed in PR #6518.
edg-l
requested review from
a team,
ManuelBilbao,
avilagaston9 and
ilitteri
as code owners
Lines of code reportTotal lines added: Detailed view |
🤖 Codex Code Review
Aside from those L2 builder regressions, the L1-side Amsterdam changes look coherent: mempool intrinsic gas now matches the VM split, the builder/validator both enforce the 2D rule, and the BAL index widening to Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Greptile SummaryThis PR brings ethrex to Confidence Score: 5/5Safe to merge — all remaining findings are P2 style suggestions; extensive test coverage (8781 ef_tests + 1342 hive + 24 new unit tests) validates correctness. No P0 or P1 issues found. The complex EIP-8037 spill/drain accounting is well-documented, internally consistent, and validated by the full test suite. The three P2 comments are defensive hardening suggestions and do not affect correctness. crates/vm/backends/levm/mod.rs (withdrawal_idx overflow pattern), crates/vm/levm/src/utils.rs (intrinsic_gas_dimensions divergence risk)
|
| Filename | Overview |
|---|---|
| crates/vm/backends/levm/mod.rs | Shadow BAL recorder on per-tx parallel executor, SYSTEM_ADDRESS whitelist, EIP-8037 2D inclusion check, u16→u32 BAL index migration — minor u32::try_from().map( |
| crates/vm/levm/src/vm.rs | New EIP-8037 state-gas counters (spill/drain split, clamped reservoir math), credit_state_gas_refund, top-level failure wipe — complex but well-commented and test-covered |
| crates/vm/levm/src/utils.rs | New standalone intrinsic_gas_dimensions mirrors VM::get_intrinsic_gas; EIP-7976/7981 floor token updates; duplication is guarded by parity tests but is a maintenance risk |
| crates/vm/levm/src/gas_cost.rs | Dynamic cost_per_state_byte formula (sanity-checked at 120M gas → 1174), EIP-7976/7981 floor helpers; usize→u64 cast in access_list_bytes is benign on all real targets |
| crates/blockchain/payload.rs | 2D inclusion check before BAL touches, u16→u32 BAL index, saturating_sub rollback guard — parity with validator path now explicit |
| crates/blockchain/mempool.rs | Routes Amsterdam CREATE intrinsic through intrinsic_gas_dimensions so mempool admission matches VM charge — correct fix for EIP-8037/7976/7981 parity |
| crates/common/types/block_access_list.rs | u16→u32 widening of BlockAccessIndex throughout; take_touched_addresses/take_storage_reads helpers for shadow recorder extraction; no logic changes to existing BAL recording semantics |
| crates/vm/levm/src/hooks/default_hook.rs | Revised refund_sender block-accounting formula (raw_consumed - intrinsic_state - reservoir_initial - spill), same-tx SELFDESTRUCT state refund, EIP-7976/7981 floor updates |
| crates/l2/sequencer/block_producer/payload_builder.rs | BAL checkpoint/restore added to both rejection paths in L2 fill_transactions so failed txs leave no trace in BAL — parity with L1 builder |
| test/tests/blockchain/builder_validator_parity_tests.rs | 14 new positive/negative parity tests: 9 builder→validator round-trips + 5 BAL corruption rejection guards mirroring Hive test_bal_invalid_* cases |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
TX[Transaction] --> MEM{Mempool\nAmsterdam?}
MEM -->|yes| IGD[intrinsic_gas_dimensions\nEIP-7976/7981/8037]
MEM -->|no| LEGACYG[legacy intrinsic gas]
IGD --> ADMIT{regular+state\n≤ gas_limit?}
ADMIT -->|no| REJECT[Reject tx]
ADMIT -->|yes| POOL[Mempool]
POOL --> BUILD[Payload Builder]
BUILD --> CHECK2D{check_2d_gas_allowance\nEIP-8037 PR2703}
CHECK2D -->|fail| SKIP[Skip tx]
CHECK2D -->|pass| BALCHK[BAL checkpoint\nset_bal_index u32]
BALCHK --> EXECB[Execute tx]
EXECB -->|error| RESTORE[tx_restore checkpoint]
EXECB -->|ok| COMMIT[Commit BAL + gas]
COMMIT --> HEADER[Block Header BAL hash]
HEADER --> VAL{Validator}
VAL --> PARAL[execute_block_parallel]
PARAL --> SEED[seed_db_from_bal per-tx CacheDB]
SEED --> SHADOW[Shadow BAL recorder\ntouched_addresses / storage_reads]
SHADOW --> EXECV[Execute tx\nEIP-8037 reservoir spill/drain]
EXECV --> CHKGAS[check_2d_gas_allowance\nper running totals]
EXECV --> CHKBAL[BAL shadow validation\nmissing account/slot?]
CHKBAL -->|mismatch| INVALID[Block INVALID]
CHKBAL -->|ok| VALID[Block VALID]
Prompt To Fix All With AI
This is a comment left during a code review.
Path: crates/vm/backends/levm/mod.rs
Line: 444-446
Comment:
**Potential u32 arithmetic overflow on withdrawal index**
`u32::try_from(...).map(|n| n + 1)` performs the `+ 1` inside `u32`. If `transactions.len()` happens to equal `u32::MAX` (which the `debug_assert` guards against in debug builds but not release), `n + 1` wraps to `0` in release mode instead of `u32::MAX`. The other call sites in this PR consistently use `u32::try_from(n + 1).unwrap_or(u32::MAX)` (usize addition, then fallible conversion), which is more defensive.
```suggestion
let withdrawal_idx =
u32::try_from(block.body.transactions.len() + 1).unwrap_or(u32::MAX);
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: crates/vm/levm/src/gas_cost.rs
Line: 660-661
Comment:
**`usize → u64` cast in `access_list_bytes`**
`keys.len() as u64` is an unchecked `usize → u64` cast. On any 64-bit platform this is lossless, but it suppresses the lint that `saturating_mul` already guards the product. For consistency with the rest of the codebase (which uses `try_into` or `saturating_mul` chains) consider using `u64::try_from(keys.len()).unwrap_or(u64::MAX)` instead.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: crates/vm/levm/src/utils.rs
Line: 639-644
Comment:
**Duplicated intrinsic gas logic risks silent divergence**
`intrinsic_gas_dimensions` mirrors `VM::get_intrinsic_gas` almost line-for-line. The PR adds `test_intrinsic_parity_*` guards, which is the right mitigation, but any future change to one function that misses the other will silently break mempool admission or the 2D inclusion check. A doc comment cross-referencing the sister function (e.g. `/// KEEP IN SYNC WITH [`VM::get_intrinsic_gas`]`) would make the coupling explicit for future maintainers.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "test(l1): bal-devnet-4 follow-up regress..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| // Validate BAL entries at the withdrawal index against actual | ||
| // post-withdrawal/request state. |
There was a problem hiding this comment.
u32::try_from(...).map(|n| n + 1) performs the + 1 inside u32. If transactions.len() happens to equal u32::MAX (which the debug_assert guards against in debug builds but not release), n + 1 wraps to 0 in release mode instead of u32::MAX. The other call sites in this PR consistently use u32::try_from(n + 1).unwrap_or(u32::MAX) (usize addition, then fallible conversion), which is more defensive.
| // Validate BAL entries at the withdrawal index against actual | |
| // post-withdrawal/request state. | |
| let withdrawal_idx = | |
| u32::try_from(block.body.transactions.len() + 1).unwrap_or(u32::MAX); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/vm/backends/levm/mod.rs
Line: 444-446
Comment:
**Potential u32 arithmetic overflow on withdrawal index**
`u32::try_from(...).map(|n| n + 1)` performs the `+ 1` inside `u32`. If `transactions.len()` happens to equal `u32::MAX` (which the `debug_assert` guards against in debug builds but not release), `n + 1` wraps to `0` in release mode instead of `u32::MAX`. The other call sites in this PR consistently use `u32::try_from(n + 1).unwrap_or(u32::MAX)` (usize addition, then fallible conversion), which is more defensive.
```suggestion
let withdrawal_idx =
u32::try_from(block.body.transactions.len() + 1).unwrap_or(u32::MAX);
```
How can I resolve this? If you propose a fix, please make it concise.| /// 20 bytes per address entry + 32 bytes per storage key. | ||
| pub fn access_list_bytes(access_list: &AccessList) -> u64 { |
There was a problem hiding this comment.
usize → u64 cast in access_list_bytes
keys.len() as u64 is an unchecked usize → u64 cast. On any 64-bit platform this is lossless, but it suppresses the lint that saturating_mul already guards the product. For consistency with the rest of the codebase (which uses try_into or saturating_mul chains) consider using u64::try_from(keys.len()).unwrap_or(u64::MAX) instead.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/vm/levm/src/gas_cost.rs
Line: 660-661
Comment:
**`usize → u64` cast in `access_list_bytes`**
`keys.len() as u64` is an unchecked `usize → u64` cast. On any 64-bit platform this is lossless, but it suppresses the lint that `saturating_mul` already guards the product. For consistency with the rest of the codebase (which uses `try_into` or `saturating_mul` chains) consider using `u64::try_from(keys.len()).unwrap_or(u64::MAX)` instead.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| /// Compute `(regular, state)` intrinsic gas for a transaction without needing | ||
| /// a full VM instance. Mirrors `VM::get_intrinsic_gas` but operates on the raw | ||
| /// transaction, fork, and block gas limit (for cpsb derivation). Pre-Amsterdam | ||
| /// returns `(regular, 0)`. | ||
| /// | ||
| /// Used by the block executor to perform the EIP-8037 (PR #2703) per-tx 2D |
There was a problem hiding this comment.
intrinsic_gas_dimensions mirrors VM::get_intrinsic_gas almost line-for-line. The PR adds test_intrinsic_parity_* guards, which is the right mitigation, but any future change to one function that misses the other will silently break mempool admission or the 2D inclusion check. A doc comment cross-referencing the sister function (e.g. /// KEEP IN SYNC WITH [VM::get_intrinsic_gas]) would make the coupling explicit for future maintainers.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/vm/levm/src/utils.rs
Line: 639-644
Comment:
**Duplicated intrinsic gas logic risks silent divergence**
`intrinsic_gas_dimensions` mirrors `VM::get_intrinsic_gas` almost line-for-line. The PR adds `test_intrinsic_parity_*` guards, which is the right mitigation, but any future change to one function that misses the other will silently break mempool admission or the 2D inclusion check. A doc comment cross-referencing the sister function (e.g. `/// KEEP IN SYNC WITH [`VM::get_intrinsic_gas`]`) would make the coupling explicit for future maintainers.
How can I resolve this? If you propose a fix, please make it concise.
🤖 Claude Code ReviewNow I have enough to write a comprehensive review. PR #6521 Review —
|
Benchmark Results ComparisonNo significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: FibonacciRecursive
Benchmark Results: ManyHashes
Benchmark Results: MstoreBench
Benchmark Results: Push
Benchmark Results: SstoreBench_no_opt
|
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.
…erals `Environment` gained an `is_system_call: bool` field earlier in the bal-devnet-4 rollup, but two tooling-side struct literals still constructed it without the new field. CI Lint + EF Tests Check fail with E0063 (no default fallback because they're full literals, not `..Default::default()` spreads). - tooling/ef_tests/state/runner/levm_runner.rs:205 - tooling/ef_tests/state_v2/src/modules/runner.rs:126 Both transaction runners for the ef_tests harnesses are not running system calls, so `is_system_call: false` is the correct value.
bal-devnet-4 testing requires the same fixed cost_per_state_byte value that bal-devnet-3 used (1174). Replace the dynamic formula body with a constant return; the formula constants (BLOCKS_PER_YEAR, TARGET_STATE_GROWTH_PER_YEAR, CPSB_SIGNIFICANT_BITS, CPSB_OFFSET) and all VM field plumbing are kept intact so this commit can be reverted with a single \`git revert\` to restore the dynamic formula. Mark formula-specific unit tests and one calibration-sensitive deposit OOG test as #[ignore] with a re-enable note.
Updates the fixtures URL (and matching docs/labels) from bal@v5.7.0 to sn%C3%B8bal-devnet-4%40v1.0.0/fixtures_snobal-devnet-4.tar.gz across the .fixtures_url_amsterdam file, Makefile, hive amsterdam.yaml, and hive.md.
…esis (#6534) Not having this was making our genesis management different from clients like nethermind.
| /// | ||
| /// Used by the block executor to perform the EIP-8037 (PR #2703) per-tx 2D | ||
| /// inclusion check before the tx runs. | ||
| pub fn intrinsic_gas_dimensions( |
There was a problem hiding this comment.
(Greptile P2) intrinsic_gas_dimensions mirrors VM::get_intrinsic_gas line-for-line. test_intrinsic_parity_* is the right immediate guard, but the duplication will eventually drift — a future Amsterdam-tweak adds a charge to one and forgets the other. The good outcome: parity test catches it. The bad outcome: someone disables/relaxes the parity test under deadline pressure. Worth a follow-up that extracts shared logic into compute_intrinsic_gas_internal(tx, fork, block_gas_limit, mode: { Vm | Standalone }) and calls it from both. Not blocking this PR.
| /// TARGET_STATE_GROWTH_PER_YEAR / CPSB_SIGNIFICANT_BITS / CPSB_OFFSET) is preserved | ||
| /// in the consts above so this commit can be reverted with a single `git revert` to | ||
| /// restore the formula body. See execution-specs#2687. | ||
| pub fn cost_per_state_byte(_block_gas_limit: u64) -> u64 { |
There was a problem hiding this comment.
Description / implementation mismatch: PR description says "dynamic cpsb (previously a static 1174)" as a delta against bal@v5.6.1, but this is still pinned to 1174 with the same TEMPORARY for bal-devnet-4 doc that #6546 has been working to clean up. Either (a) the dynamic formula isn't actually being used yet on this branch and the description's delta refers to the spec change, or (b) the dynamic body is in a follow-up. Worth clarifying the description, since reading it suggests dynamic CPSB lands here. Also intersects with #6546 — the EIP-11573 sync that nukes the dynamic formula entirely. Whichever lands second should be aware of the ordering.
| // gas; our devnet-4 (bal@v5.7.0) impl correctly keeps intrinsic_state_gas | ||
| // immutable and routes the refund to the reservoir only. Re-enable once the | ||
| // zkevm@v0.4.x release ships fixtures regenerated against devnet-4. | ||
| "witness_codes_redelegation_old_marker_included_new_marker_excluded", |
There was a problem hiding this comment.
Action items for these skipped fixtures:
- Add a TODO comment naming the upstream version that lifts the skip (zkevm@v0.4.x), so a future updater knows when to remove it without re-deriving the reasoning.
- File a tracking issue (or link an existing one) so the skip doesn't outlive its trigger silently.
3 participants
Brings ethrex up to
bal@v5.7.0(bal-devnet-4). Replaces closed PR #6518 — that one was accidentally opened on the long-livedbal-devnet-4branch that ethpanda tracks; this one uses the dedicatedbal-devnet-4-prbranch so the PR lifecycle doesn't disturb the rolling progress branch.What bal-devnet-4 requires
The spec update rolls up five EIPs that interlock:
BlockAccessIndexwidenedu16 → u32.cost_per_state_byte, per-frame reservoir, clamp-and-spill refunds, per-tx 2D inclusion check, immutable intrinsic state gas, same-tx SELFDESTRUCT refund, and top-level reservoir reset on tx failure.Against bal@v5.6.1 the notable deltas are the u32 index widening, dynamic cpsb (previously a static 1174), immutable intrinsic state gas (PR #2711), and the per-tx 2D inclusion check (PR #2703). Fixtures were regenerated accordingly.
Results
eels/consume-engineamsterdam: 1342 pass / 0 fail (from 10 failing at the start).witness_codes_*zkevm@v0.3.x fixtures skipped — filled against bal@v5.6.1 pre-chore(l1): fix precompile tests from EIP-7702 #2711, explicit skip intooling/ef_tests/blockchain/tests/all.rs, re-enable when zkevm@v0.4.x ships).Builder/validator parity
The bigger-than-expected part of this PR is making sure ethrex-as-builder and ethrex-as-validator agree on every Amsterdam output. An audit turned up three miss-slot risks on the builder side (Amsterdam CREATE intrinsic gas in the mempool, missing 2D inclusion check in
fill_transactions, missing BAL checkpoint/restore in the L2 builder). All three are fixed.To keep this from regressing silently, there's a new test file,
test/tests/blockchain/builder_validator_parity_tests.rs, that builds a block via the payload builder and then runs the built block+BAL through the validator pipeline. The module doc explains the rationale and what a failure means (P0: would cause a missed slot on devnet). Nine positive scenarios exercise the shared path; five negative scenarios corrupt the BAL deterministically and assert the validator rejects — each mirrors one of the Hivetest_bal_invalid_*cases we fixed in this PR.Where to focus review
crates/vm/backends/levm/mod.rs— biggest diff. Parallel validator with the shadow BAL recorder, PART B validation interactions, SYSTEM_ADDRESS handling.crates/vm/levm/src/vm.rs— new state-gas counters + CallFrame snapshot fields for the EIP-8037 reservoir model.crates/vm/levm/src/gas_cost.rs::cost_per_state_byte— formula implementation, matched against EELS. Sanity:cpsb(120_000_000) == 1174.crates/vm/levm/src/utils.rs—get_intrinsic_gas(VM) andintrinsic_gas_dimensions(standalone, used by mempool + builder) must stay in sync. Parity is checked bytest_intrinsic_parity_*.crates/blockchain/payload.rs::fill_transactions— 2D inclusion check + BAL checkpoint semantics.test/tests/blockchain/builder_validator_parity_tests.rs— the primary regression guard for everything else.Relationship to PR #6463
#6463 (withdrawal-phase BAL reverse check) merged into main before this branch was rebased. This PR builds on its
validate_bal_withdrawal_indexPart B; the u32 widening + #6463's new&BalAddressIndexparameter are merged into a combined signature. Not re-introducing #6463's changes.Test plan
make -C tooling/ef_tests/blockchain testmake run-hive-eels-amsterdamcargo test -p ethrex-test --test ethrex_testscargo fmt --all+cargo check --workspace --libPins
524b44617e410ab21b5122f0be5113b62a0e76eeon branchdevnets/bal/4