fix(l1): bump BAL fixtures to v7.3.2 and fix self-destruct-in-initcode + pre-fork newPayload BAL handling - #6842
Conversation
|
Lines of code reportTotal lines added: Detailed view |
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
|
🤖 Kimi Code ReviewAutomated review by Kimi (Moonshot AI) |
🤖 Codex Code ReviewNo findings. The LEVM change in Residual risk: I did not find a focused local regression test for the new Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Greptile SummaryBumps the Amsterdam BAL fixture bundle to
Confidence Score: 4/5Safe to merge; both fixes are well-motivated by EELS behaviour, all 8753 amsterdam tests pass, and the two stateless skips are documented with a clear removal plan. The CREATE-in-static gas accounting and the self-destruct BAL collapse are both small, targeted fixes with thorough in-code documentation and a passing test run. The only rough edge is that track_selfdestruct correctness silently depends on record_balance_change being called before add_selfdestruct (currently always true, but not enforced by the function itself), and the stateless skip list is slightly over-broad for create2check_fields_in_initcode. crates/common/types/block_access_list.rs — the track_selfdestruct guard relies on an implicit ordering invariant from the call site; worth hardening if the function is ever called from a new context. tooling/ef_tests/blockchain/tests/all.rs — the create2check_fields_in_initcode skip is broader than the documented d3/d7 variants.
|
| Filename | Overview |
|---|---|
| crates/vm/levm/src/opcode_handlers/system.rs | Moves the CREATE/CREATE2 static-context check to after the create_message_gas reservation in generic_create; pre-Amsterdam check retained before reservation |
| crates/common/types/block_access_list.rs | track_selfdestruct now collapses intermediate balance entries to a single (idx, 0) for pre-existing addresses; guard silently no-ops if balance_changes has no entry for the address (benign today due to call-site ordering) |
| crates/vm/levm/src/vm.rs | Adds create_static_regular_spill VM-level field initialized to 0 for EIP-7778 regular-gas dimension exclusion |
| crates/vm/levm/src/hooks/default_hook.rs | refund_sender subtracts create_static_regular_spill from the regular-gas dimension, keeping receipt gas intact |
| tooling/ef_tests/blockchain/tests/all.rs | Adds three skip entries for the stateless suite; create2check_fields_in_initcode matches all d-variants, not only the documented d3 and d7 |
| docs/known_issues.md | New file documenting the stateless-suite skip rationale and the action item to remove skips once a newer zkevm bundle is released |
| .github/config/hive/amsterdam.yaml | Fixture pin and EELS commit bumped from tests-bal@v7.2.0 to v7.3.0 |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[CREATE / CREATE2 opcode] --> B[Pop stack args\nCharge static gas]
B --> C{fork >= Amsterdam\nAND is_static?}
C -- Pre-Amsterdam --> D[Check is_static\nbefore gas reservation\nHalt immediately]
C -- No --> E[Reserve create_message_gas\nmax_message_call_gas]
C -- Yes --> F[Add gas_limit to\ncreate_static_regular_spill]
F --> G[Halt: OpcodeNotAllowedInStaticContext\nfull frame burned to user]
E --> H[Execute initcode / deployment]
H --> I{SELFDESTRUCT\nin initcode?}
I -- Yes --> J[record_balance_change addr 0\nadd_selfdestruct addr]
I -- No --> K[Normal CREATE completion]
J --> L[destroy_accounts:\ntrack_selfdestruct addr]
L --> M{pre_balance == 0?}
M -- Yes --> N[Remove all idx entries\nno BAL change recorded]
M -- No --> O[Remove all idx entries\nPush idx 0 as final balance]
G --> P[refund_sender:\nsubtract create_static_regular_spill\nfrom regular-gas dimension]
Comments Outside Diff (1)
-
crates/common/types/block_access_list.rs, line 1585-1595 (link)Guard may silently skip the (idx, 0) push for pre-existing contracts
If
balance_changeshas no entry foraddressat all butpre_balance != 0, the outerif let Some(changes)returnsNoneand the(idx, 0)change is never pushed. EELS would still record a pre-tx → 0 diff for such an address. In practice this can't arise on the Amsterdam code path becauserecord_balance_change(to, U256::zero())is always called immediately beforeadd_selfdestruct(to)(guaranteeing at least one entry exists), but the guard makes the invariant implicit. A fallback that inserts the entry when the map has no slot for the address would make the function self-sufficient against future callers who don't uphold that ordering.Prompt To Fix With AI
This is a comment left during a code review. Path: crates/common/types/block_access_list.rs Line: 1585-1595 Comment: **Guard may silently skip the (idx, 0) push for pre-existing contracts** If `balance_changes` has no entry for `address` at all but `pre_balance != 0`, the outer `if let Some(changes)` returns `None` and the `(idx, 0)` change is never pushed. EELS would still record a pre-tx → 0 diff for such an address. In practice this can't arise on the Amsterdam code path because `record_balance_change(to, U256::zero())` is always called immediately before `add_selfdestruct(to)` (guaranteeing at least one entry exists), but the guard makes the invariant implicit. A fallback that inserts the entry when the map has no slot for the address would make the function self-sufficient against future callers who don't uphold that ordering. 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!
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
crates/common/types/block_access_list.rs:1585-1595
**Guard may silently skip the (idx, 0) push for pre-existing contracts**
If `balance_changes` has no entry for `address` at all but `pre_balance != 0`, the outer `if let Some(changes)` returns `None` and the `(idx, 0)` change is never pushed. EELS would still record a pre-tx → 0 diff for such an address. In practice this can't arise on the Amsterdam code path because `record_balance_change(to, U256::zero())` is always called immediately before `add_selfdestruct(to)` (guaranteeing at least one entry exists), but the guard makes the invariant implicit. A fallback that inserts the entry when the map has no slot for the address would make the function self-sufficient against future callers who don't uphold that ordering.
### Issue 2 of 2
tooling/ef_tests/blockchain/tests/all.rs:45-49
**`create2check_fields_in_initcode` skip matches all d-variants, not only d3 and d7**
`known_issues.md` documents only `d3` and `d7` as the affected vectors, but the skip string `"create2check_fields_in_initcode"` is a substring match and will suppress every variant (d0–d9) in the stateless run. This is temporarily over-broad and could mask regressions in the unaffected variants (d0, d1, d2, d4, d5, d6, d8, d9) until the zkevm bundle is bumped.
Reviews (1): Last reviewed commit: "test(l1): skip CREATE-in-static stateles..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const EXTRA_SKIPS: &[&str] = &[ | ||
| "bal_create_in_static_context", | ||
| "create2check_fields_in_initcode", | ||
| "test_static_call_create.py", | ||
| ]; |
There was a problem hiding this comment.
create2check_fields_in_initcode skip matches all d-variants, not only d3 and d7
known_issues.md documents only d3 and d7 as the affected vectors, but the skip string "create2check_fields_in_initcode" is a substring match and will suppress every variant (d0–d9) in the stateless run. This is temporarily over-broad and could mask regressions in the unaffected variants (d0, d1, d2, d4, d5, d6, d8, d9) until the zkevm bundle is bumped.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tooling/ef_tests/blockchain/tests/all.rs
Line: 45-49
Comment:
**`create2check_fields_in_initcode` skip matches all d-variants, not only d3 and d7**
`known_issues.md` documents only `d3` and `d7` as the affected vectors, but the skip string `"create2check_fields_in_initcode"` is a substring match and will suppress every variant (d0–d9) in the stateless run. This is temporarily over-broad and could mask regressions in the unaffected variants (d0, d1, d2, d4, d5, d6, d8, d9) until the zkevm bundle is bumped.
How can I resolve this? If you propose a fix, please make it concise.
🤖 Claude Code ReviewNow I have enough context for a thorough review. PR #6842 Review: fix(l1): bump BAL fixtures to v7.3.0 and fix CREATE-in-static gas + self-destruct BALSummaryTwo correctness fixes for Amsterdam BAL accounting, both triggered by new v7.3.0 fixtures. The implementation is well-reasoned, well-commented, and verifiably matches EELS behavior. One minor defensive-coding gap and one implicit assumption worth noting. Fix 1 —
|
| // Drop all intermediate balance changes recorded for this tx. | ||
| changes.retain(|(i, _)| *i != idx); | ||
| // Record the final post-tx balance of 0 unless it equals the pre-tx balance. | ||
| if !pre_balance.is_zero() { |
There was a problem hiding this comment.
Under Amsterdam+ EIP-7928 BAL + EIP-6780 SELFDESTRUCT semantics, this if !pre_balance.is_zero() branch is unreachable in practice: track_selfdestruct is only called for contracts created and destroyed in the same tx (per the function-level comment + the is_account_created gate in system.rs:680), so by definition initial_balances[address] was either unset (→ unwrap_or_default = 0) or set during the in-tx CREATE with the creation balance (typically 0 — CREATE-with-value sets initial_balance only if balance > 0, but that path is the CREATE side, not the SELFDESTRUCT side that reaches here).
The branch is sound defensive code matching the EELS post-state diff exactly, but the doc comment above (lines 1568-1581) reads as though the non-zero pre-balance case is a real scenario. Worth tightening the comment to: "In practice pre_balance is always 0 under EIP-6780 (same-tx-created); the non-zero branch mirrors EELS's general pre/post diff and guards against future spec changes or test fixtures that exercise alternate paths." Otherwise a future reader will hunt for the scenario.
edg-l
changed the title
edg-l
changed the title
…in-static (EIP-7778)
…ruct in initcode)
v7.3.1 moves the is_static check upfront in CREATE/CREATE2 (eels a8b705f), so create_message_gas is never reserved in a static context. This undoes the v7.3.0-era workaround (7defce9) that charged it and carved it out of the regular-gas dimension, and removes the stateless zkevm@v0.4.1 skips (839074d) that only existed to mask that divergence.
b7bd710 to
059d42f
Compare
d75c1e9 to
2a757ce
Compare
…BAL hash field
Deserialize empty blockAccessList ('0x') as None and drop the V4
fork-boundary detector so a pre-Amsterdam header carrying
block_access_list_hash fails the normal block-hash check (INVALID_BLOCK_HASH)
instead of returning -32602. Matches tests-bal@v7.3.2 fixture
test_invalid_pre_fork_block_with_bal_hash_field.
…e BAL (#6842) Squash-merge of PR #6842 (ci/bump-bal-fixtures-v7.3.0): - bump BAL fixtures to tests-bal@v7.3.0/v7.3.1/v7.3.2 - record destroyed account's final 0 balance in BAL (self-destruct in initcode) - exclude create_message_gas from regular dimension on CREATE-in-static (EIP-7778) - return PayloadStatus.INVALID for pre-fork newPayloadV4 with BAL hash field
ElFantasma
left a comment
ElFantasma
left a comment
There was a problem hiding this comment.
One of the previous comments is still relevant, but it is non-blocking. LGTM
edg-l
changed the title
4 participants
Bumps the Amsterdam BAL fixture bundle to
tests-bal@v7.3.2(from v7.2.0) and folds in the correctness fixes that the newer fixtures surfaced.What actually changes vs
mainci: bump fixture pins totests-bal@v7.3.2(eels_commitd28f7977069d1b71984ff88ed8040b671f6e6a77) acrossamsterdam.yamland the blockchain/engine/state.fixtures_url_amsterdamfiles.fix(block_access_list.rs): self-destruct in initcode was recording an intermediate balance in the BAL instead of the final post-tx balance of 0.track_selfdestructnow collapses intermediate per-tx balance entries to the final(idx, 0).fix(serde_utils.rs): an empty hex string ("0x") encodes the absence of a BAL, not an empty list. Deserialize it asNoneso pre-AmsterdamnewPayloadcalls (which send"0x") parse instead of failing RLP decode.fix(payload.rs): drop the bespoke V4 fork-boundary detector. A pre-Amsterdam header carryingblock_access_list_hashproduces ablock_hashthat won't match the one ethrex reconstructs (the field is omitted from the V4 header schema), so the mismatch now surfaces asPayloadStatus.INVALIDvia the normal block-hash check — matching the EELS fixturetest_invalid_pre_fork_block_with_bal_hash_field(INVALID_BLOCK_HASH, no engine API error code).Why the CREATE-in-static workaround was reverted
In v7.3.0, EELS
generic_createreservedcreate_message_gas(the forwarded frame) before checkingis_static, so a CREATE/CREATE2 in a static context charged that gas and then halted. To match, ethrex moved itsis_staticcheck after the reservation, burned the forwarded frame, and addedcreate_static_regular_spillto carve that reserved-but-unused gas out of EIP-7778's regular-gas dimension. The stalezkevm@v0.4.1stateless bundle (filled against ~v7.2.0) didn't have this behavior, hence the temporary skips +known_issues.md.v7.3.1 (eels
59b466e→a8b705f) moved theis_staticcheck back upfront:raise WriteInStaticContextnow runs before any gas is charged, socreate_message_gasis never reserved in a static context. That made the workaround unnecessary — the spill field andrefund_sendersubtraction were removed, the check returned to its original upfront position (which already matched ethrex's pre-existing behavior), and the stateless skips were dropped since the divergence they masked no longer exists.