fix(l1): amsterdam EIP fixes

[edg-l]

• Fix EIP-7928 (BAL) recorder to correctly handle SYSTEM_ADDRESS changes from regular (non-system) transactions, selfdestruct cleanup, and storage write-to-read reversion
• Fix EIP-7928 BAL address touch recording to match the reference gas-check gating for CALL/CALLCODE/DELEGATECALL/STATICCALL and CREATE/CREATE2
• Fix EIP-7708 (ETH Transfer Logs) emission for selfdestruct-to-self and CALLCODE self-transfers
• Remove legacy Amsterdam skip list from blockchain tests and enable all Amsterdam selfdestruct EF tests
• Add run-ef-tests.py script for running EF state tests across forks
• Add Amsterdam to default forks in the state test runner

Test Infrastructure

• Remove 150+ line Amsterdam skip list from blockchain/tests/all.rs

• Add run-ef-tests.py for running EF state tests with fork filtering

• Add Amsterdam to DEFAULT_FORKS in the state test runner

• Amsterdam state tests: 250/250

• Prague, Cancun, Shanghai, Paris state tests: 51,728/51,728

Added a python script which helps iterate over the ef tests, keeping it for future use in future forks

[github-actions]

🤖 Kimi Code Review

Review Summary

This PR implements EIP-7928 (Block-Level Access Lists) and related Amsterdam fork features. The changes are well-structured and follow Ethereum specifications closely. Here are my findings:

✅ Good Practices

• EIP Compliance: Correctly implements EIP-7928 filtering for SYSTEM_ADDRESS during system calls
• State Management: Proper handling of self-destruct account cleanup per EIP-7928/EIP-6780
• Gas Accounting: EIP-7778 implementation appears correct with proper floor calculations
• Testing: Comprehensive test coverage including edge cases for system address filtering

⚠️ Issues Found

1. Potential Integer Overflow (Critical)

File: crates/vm/levm/src/opcode_handlers/system.rs:123-130

if gas_remaining >= i64::try_from(basic_cost).unwrap_or(i64::MAX) {

• Issue: i64::try_from(basic_cost) can fail on 32-bit systems when basic_cost > i64::MAX
• Fix: Use saturating conversion or handle the error case properly:

let basic_cost_i64 = i64::try_from(basic_cost).unwrap_or(i64::MAX);
if gas_remaining >= basic_cost_i64 {

2. Race Condition in System Call Mode (Medium)

File: crates/vm/backends/levm/mod.rs:754-769

• Issue: If VM execution panics, exit_system_call() won't be called, leaving BAL recorder in inconsistent state
• Fix: Use RAII pattern with a guard struct to ensure exit_system_call() is always called

3. Inefficient BTreeMap Operations (Performance)

File: crates/common/types/block_access_list.rs:597-605

if let Some(promoted) = self.reads_promoted_to_writes.get_mut(&addr) {
    promoted.retain(|s| *s != slot);
    if promoted.is_empty() {
        self.reads_promoted_to_writes.remove(&addr);
    }
}

• Issue: Multiple lookups for the same key
• Fix: Use entry API to avoid double lookup:

if let btree_map::Entry::Occupied(mut entry) = self.reads_promoted_to_writes.entry(addr) {
    entry.get_mut().retain(|s| *s != slot);
    if entry.get().is_empty() {
        entry.remove();
    }
}

4. Missing Error Handling (Medium)

File: tooling/ef_tests/run-ef-tests.py:842

• Issue: Python script doesn't validate exit codes from subprocess calls
• Fix: Check proc.returncode and handle non-zero exits appropriately

🔍 Minor Suggestions

1. Documentation: Add doc comments explaining the in_system_call flag behavior in BlockAccessListRecorder
2. Naming: Consider renaming record_bal_call_touch to record_bal_addresses_if_gas_sufficient for clarity
3. Constants: Define magic numbers like i64::MAX as named constants for better readability

🎯 Security Considerations

• The SYSTEM_ADDRESS filtering logic correctly prevents state pollution during system calls
• Gas accounting changes follow EIP-7778 specification accurately
• No apparent vulnerabilities in state transition handling

📊 Overall Assessment

The implementation is solid and follows Ethereum specifications correctly. The main concerns are around edge case handling (overflows, panics) rather than consensus correctness. The PR is ready to merge after addressing the overflow issue and adding proper cleanup guarantees for system call mode.

---

Automated review by Kimi (Moonshot AI) · custom prompt

[github-actions]

🤖 Codex Code Review

Found a few correctness/consensus-risk issues and a couple of design nits. Line refs are approximate from the diff.

High risk / correctness

• generic_system_contract_levm leaves in_system_call set on early error paths. enter_system_call() is called before VM::new and execute(), but exit_system_call() is only called after a successful execute(). Any error return will leave the recorder in “system call mode” for subsequent calls, silently dropping SYSTEM_ADDRESS touches/balance/nonce changes. Consider a RAII guard or scopeguard to ensure exit on all paths. (crates/vm/backends/levm/mod.rs:751-770)
• record_bal_call_touch moved before gas checks for CALL/CALLCODE/DELEGATECALL/STATICCALL, which can record BAL touches even if the call OOGs before execution. EIP-7928 semantics typically only include accesses if the call passes gas checks. Previously the touch was after gas_cost::call* and OOG checks. This looks like a behavior regression. (crates/vm/levm/src/opcode_handlers/system.rs:80-140, 200-260, 330-380, 440-490)

Consensus / EIP compliance

• The new system-call “mode” is a bool, not a depth counter. If system calls can nest (e.g., system contract calls another system contract), exit_system_call() will flip the flag false too early and leak SYSTEM_ADDRESS events from the outer call. Use a counter or stack depth. (crates/common/types/block_access_list.rs:592-610, crates/vm/backends/levm/mod.rs:751-770)
• In refund_sender, the comment says “User pays post-refund gas (with floor)” but gas_spent isn’t floored; only gas_used is clamped via max(pre_refund, floor). If gas_spent can be < floor, this mismatches EIP-7778 intent. If it’s already floored elsewhere, add a note or make it explicit. (crates/vm/levm/src/hooks/default_hook.rs:182-200)

Maintainability / robustness

• track_selfdestruct is keyed on current_index and called from delete_self_destruct_accounts. If current_index advances after selfdestruct within the same tx (e.g., nested contexts), double-check it still matches the intended “current tx index.” Consider threading the tx index explicitly to avoid hidden coupling. (crates/common/types/block_access_list.rs:1123-1185, crates/vm/levm/src/hooks/default_hook.rs:300-310)

If you want, I can suggest a small RAII guard for the system-call mode and a minimal fix to restore BAL touch ordering.

Possible next steps:

1. Add a scoped guard for enter_system_call/exit_system_call and make in_system_call a counter.
2. Move record_bal_call_touch back after gas checks (or confirm via spec/tests that pre-check touches are correct).
3. Add/adjust tests for OOG-before-call and nested system call BAL behavior.

---

Automated review by OpenAI Codex · custom prompt

[github-actions]

🤖 Claude Code Review

Perfect! Now I have a comprehensive view of the changes. Let me compile my review:

PR #6149 Code Review: Amsterdam EIP Fixes

Summary

This PR implements critical fixes for Amsterdam fork EIP-7928 (Block Access Lists), EIP-7708 (ETH Transfer Logs), and EIP-7778 (Gas Accounting). It removes 158 lines of Amsterdam test skips and adds test infrastructure. The changes enable 250 Amsterdam state tests to pass.

✅ Strengths

1. Comprehensive EIP-7928 BAL fixes - Correctly handles system address filtering with in_system_call flag
2. Gas gating for BAL touches - Properly implements intermediate gas checks matching EELS reference
3. Selfdestruct cleanup - track_selfdestruct() correctly converts writes to reads and removes transient changes
4. Test infrastructure - 842-line Python script for EF test analysis is well-structured
5. Removed skip list - Enables 250 Amsterdam tests, demonstrating real progress

🔴 Critical Issues

1. Potential integer overflow in BAL gas check (crates/vm/levm/src/opcode_handlers/system.rs:527)

if gas_remaining >= i64::try_from(basic_cost).unwrap_or(i64::MAX) {

Issue: Using i64::MAX as fallback makes the gas check always pass when basic_cost overflows u64→i64 conversion. This could allow BAL touches when they should be rejected.

Recommendation: Should fail conservatively:

if let Ok(cost_i64) = i64::try_from(basic_cost) {
    if gas_remaining >= cost_i64 {
        recorder.record_touched_address(target);
        // ...
    }
}

Severity: Medium - Non-consensus BAL recording path, but incorrect behavior nonetheless.

2. Missing BAL cleanup on revert (crates/vm/levm/src/opcode_handlers/system.rs:638-667)

The selfdestruct operation calls track_selfdestruct() in the hook (default_hook.rs:241), but this happens AFTER the operation completes. If the transaction reverts, the BAL cleanup will have already occurred.

Issue: BAL state changes should be checkpoint/restore aware. The current implementation doesn't integrate with the checkpoint mechanism properly.

Recommendation: Verify that track_selfdestruct is called at the correct point in the execution flow, after the account is marked for destruction but coordinated with checkpoint/restore logic.

Severity: High - Could cause incorrect BAL in reverted transactions.

3. EIP-7778 gas floor calculation (crates/vm/levm/src/hooks/default_hook.rs:187)

let floor = vm.get_min_gas_used()?;
ctx_result.gas_used = gas_used_pre_refund.max(floor);

Question: Is get_min_gas_used() correctly implementing the EIP-7778 calldata floor? The comment says "calldata floor" but we don't see the implementation.

Recommendation: Add a comment explaining what get_min_gas_used() returns (intrinsic gas + calldata costs?) or verify it matches the spec.

Severity: Medium - Critical for EIP-7778 correctness.

⚠️ Important Issues

4. Race condition in read-to-write promotion cleanup (crates/common/types/block_access_list.rs:595-602, 1183-1188)

The same cleanup logic appears in two places:

• filter_net_zero_storage() at line 595
• track_selfdestruct() at line 1183

Issue: This duplication suggests the logic is fragile. If track_selfdestruct is called before filter_net_zero_storage, the promotion state could be inconsistent.

Recommendation: Consider extracting this into a helper method like unpromote_slot(address, slot) to ensure consistent behavior.

Severity: Low-Medium - Maintenance risk and potential edge case bugs.

5. EIP-7708 selfdestruct-to-self logic (crates/vm/levm/src/opcode_handlers/system.rs:662-667)

} else if self.substate.is_account_created(&to) {
    // Selfdestruct-to-self: only emit log when created in same tx (burns ETH)
    // Pre-existing contracts selfdestructing to self emit NO log
    let log = create_selfdestruct_log(to, balance);
    self.substate.add_log(log);
}

Question: For Cancun+ with EIP-6780, selfdestruct only burns ETH if the contract was created in the same tx. The log emission looks correct, but is balance already transferred to self before this check?

Looking at line 642: self.transfer(to, beneficiary, balance)? - when to == beneficiary, this is a no-op. But then line 647 sets balance to zero. The log should show the original balance being burned.

Recommendation: Verify that balance variable contains the correct value (pre-burn balance) when emitting the selfdestruct log.

Severity: Low - Logging correctness issue.

6. CALLCODE self-transfer logging (crates/vm/levm/src/opcode_handlers/system.rs:559, 570)

// EIP-7708: Emit transfer log for nonzero-value CALL/CALLCODE
// Self-transfers (msg_sender == to) do NOT emit a log (includes CALLCODE)
if self.env.config.fork >= Fork::Amsterdam && !value.is_zero() && msg_sender != to {

Issue: For CALLCODE, msg_sender and to are ALWAYS the same (line 361: "Sender and recipient are the same"). The check msg_sender != to will always be false, so CALLCODE never emits logs.

Question: Is this the intended behavior per EIP-7708? The comment says "includes CALLCODE" which suggests CALLCODE self-transfers shouldn't log, but is this because ALL CALLCODE transfers are self-transfers?

Recommendation: Clarify in comment: "CALLCODE transfers are always self-transfers (to == msg_sender), so never emit logs per EIP-7708".

Severity: Low - Either correct with unclear comment, or incorrect interpretation of EIP-7708.

7. CREATE BAL tracking moved after early failure checks (crates/vm/levm/src/opcode_handlers/system.rs:471-485)

The PR moves record_touched_address(new_address) to AFTER the early failure checks (depth, balance, nonce). The comment says "per reference" but this changes when addresses enter the BAL.

Question: Are there any edge cases where a CREATE fails early but other code expects the address to be in the BAL?

Recommendation: Verify this matches the EELS reference behavior exactly. The change looks correct per the comment, but it's a behavior change worth noting.

Severity: Low - Appears correct but is a subtle behavior change.

🟡 Code Quality Issues

8. Missing initialization of in_system_call (crates/common/types/block_access_list.rs:520)

The new field in_system_call: bool is added but the struct uses #[derive(Default)]. Bools default to false which is correct, but it's implicit.

Recommendation: For clarity, explicitly initialize in the new() method or document that false is the correct default.

Severity: Very Low - Pedantic, but good for maintainability.

9. Python script included in Rust codebase (tooling/ef_tests/run-ef-tests.py)

842 lines of Python in a Rust project. While the script is well-written, it adds a Python dependency to the test infrastructure.

Recommendation: Consider if this should be:

• Rewritten in Rust as a cargo test utility
• Documented in CI/CD as a Python requirement
• Moved to a separate tools/ directory

Severity: Very Low - Organizational/style preference.

🔵 Minor Observations

10. slot_number parsing added (tooling/ef_tests/state/types.rs:1640) - Good catch, enables EIP-7843 SLOTNUM tests.

11. Amsterdam added to DEFAULT_FORKS (tooling/ef_tests/state_v2/src/modules/types.rs:1666) - Correct for enabling Amsterdam state tests.

12. Test updates (test/tests/levm/eip7928_tests.rs:582, 590) - Existing tests updated to call enter_system_call(). Good consistency.

🎯 Recommendations

1. Fix the integer overflow handling in record_bal_call_touch (Issue docs: add milestones #1) - This is the most important fix.

2. Verify selfdestruct BAL cleanup timing (Issue chore: create project structure #2) - Ensure it works correctly with reverts.

3. Document or verify get_min_gas_used() for EIP-7778 (Issue build: add Github actions boilerplate #3) - Critical for gas accounting correctness.

4. Add integration test for the system call filtering - Verify that system contract calls correctly filter SYSTEM_ADDRESS from BAL.

5. Add test for selfdestruct BAL cleanup - Verify storage writes become reads and nonce/code changes are removed.

Overall Assessment

Strong work on a complex set of EIP interactions. The core logic is sound and the test results (250/250 Amsterdam tests passing) demonstrate correctness. The main concerns are:

• Integer overflow handling in gas checks
• Selfdestruct cleanup timing with reverts
• Gas floor calculation verification

These should be addressed before merging to ensure consensus-critical correctness.

Recommendation: Request changes for issues #1, #2, and #3. The rest are minor and can be addressed in follow-ups.

---

Automated review by Claude (Anthropic) · custom prompt

[greptile-apps]

Greptile Overview

Greptile Summary

This PR tightens Amsterdam fork behavior across LEVM and the EF test tooling:

• EIP-7928 (Block Access List): improves address touch recording to match reference gas-check gating for CALL/CALLCODE/DELEGATECALL/STATICCALL and adjusts CREATE/CREATE2 touch timing; adds recorder support for excluding SYSTEM_ADDRESS only during system-contract calls; adds selfdestruct cleanup logic to drop transient nonce/code changes and convert same-tx storage writes into reads.
• EIP-7708 / EIP-7778: adjusts ETH transfer/selfdestruct log emission edge-cases and applies the calldata floor to Amsterdam+ block gas accounting.
• EF tests: removes the Amsterdam skip list so Amsterdam selfdestruct tests run, adds a new tooling/ef_tests/run-ef-tests.py helper for iterating on EF failures, and plumbs slot_number through state test runners while adding Amsterdam to the v2 default forks.

Main merge-blocker: generic_system_contract_levm sets the BAL recorder “system call mode” but doesn’t guarantee it is cleared on error returns, which can corrupt subsequent BAL recording in the same block.

Confidence Score: 3/5

• Not safe to merge until the BAL system-call flag is guaranteed to be reset on all error paths.
• A single clearly identifiable logic issue can leave the BAL recorder in system-call mode if VM construction/execution fails during a system contract call, causing incorrect EIP-7928 recording for subsequent non-system execution. Other changes look consistent with the PR goals and test tooling updates.
• crates/vm/backends/levm/mod.rs

Important Files Changed

Filename	Overview
.gitignore	Adds pycache/ to gitignore; no functional impact.
crates/common/types/block_access_list.rs	Adds system-call filtering for SYSTEM_ADDRESS and selfdestruct BAL cleanup (track_selfdestruct), plus fixes undoing read→write promotion when net-zero writes revert to reads.
crates/vm/backends/levm/mod.rs	Wraps system contract calls with BAL recorder enter/exit system-call mode; currently leaks flag on error paths (enter without guaranteed exit).
crates/vm/levm/src/hooks/default_hook.rs	Adjusts Amsterdam+ block gas accounting to apply calldata floor and calls BAL selfdestruct cleanup during account deletion.
crates/vm/levm/src/opcode_handlers/system.rs	Moves BAL touch recording to match reference gas-check gating for CALL-family, adjusts CREATE touch timing, and tweaks EIP-7708 selfdestruct/self-transfer log comments/logic.
test/tests/levm/eip7928_tests.rs	Updates BAL tests to explicitly enter system-call mode when asserting SYSTEM_ADDRESS filtering behavior.
tooling/ef_tests/blockchain/tests/all.rs	Removes Amsterdam-specific skip list from blockchain EF tests to enable full Amsterdam coverage.
tooling/ef_tests/run-ef-tests.py	Adds a standalone EF test runner/analyzer script for blockchain + state tests with failure categorization and JSON lookup.
tooling/ef_tests/state/runner/levm_runner.rs	Plumbs EF env slot_number into VM env instead of hardcoding to zero.
tooling/ef_tests/state/types.rs	Adds optional slot_number field to EF state test env parsing.
tooling/ef_tests/state_v2/src/modules/runner.rs	Plumbs slot_number from v2 EF env into VM env instead of hardcoding zero.
tooling/ef_tests/state_v2/src/modules/types.rs	Adds Amsterdam to DEFAULT_FORKS and adds optional slot_number to v2 Env deserialization.

Sequence Diagram

sequenceDiagram
    participant LEVM as LEVM::prepare_block
    participant SysCall as generic_system_contract_levm
    participant DB as GeneralizedDatabase
    participant BAL as BlockAccessListRecorder
    participant VM as ethrex_levm::VM
    participant Hook as DefaultHook::finalize_execution

    LEVM->>SysCall: call system contract
    SysCall->>DB: backup SYSTEM_ADDRESS + coinbase
    SysCall->>BAL: enter_system_call()
    SysCall->>VM: VM::new(env, db, tx)
    VM->>VM: execute()
    VM-->>SysCall: report or Err

    alt success
        SysCall->>BAL: exit_system_call()
        SysCall->>DB: restore backed-up accounts
        SysCall-->>LEVM: ExecutionReport
    else error (current PR)
        note over SysCall,BAL: exit_system_call() not run on Err
        SysCall-->>LEVM: Err
    end

    note over VM,Hook: During tx finalization
    Hook->>BAL: track_selfdestruct(address)

Loading

[greptile-apps]

_{12 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}