⚠️ Known Issues — intentionally skipped tests

Source: docs/known_issues.md

Known Issues

Tests intentionally excluded from CI. Source of truth for the Known
Issues section the L1 workflow appends to each ef-tests job summary
and posts as a sticky PR comment.

EF Tests — Stateless coverage narrowed to EIP-8025 optional-proofs

make -C tooling/ef_tests/blockchain test calls test-stateless-zkevm
instead of test-stateless. The zkevm@v0.3.3 fixtures are filled against
bal@v5.6.1, out of sync with current bal spec; the broad target trips ~549
fixtures. Re-broaden once the zkevm bundle is regenerated.

Lines of code report

Total lines added: 55
Total lines removed: 89
Total lines changed: 144

+-----------------------------------------------------+-------+------+
| File                                                | Lines | Diff |
+-----------------------------------------------------+-------+------+
| ethrex/crates/common/types/block.rs                 | 978   | +17  |
+-----------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/engine/payload.rs      | 1033  | +38  |
+-----------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/call_frame.rs             | 374   | -8   |
+-----------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/hooks/default_hook.rs     | 490   | -4   |
+-----------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/opcode_handlers/system.rs | 949   | -36  |
+-----------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/utils.rs                  | 628   | -3   |
+-----------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/vm.rs                     | 702   | -38  |
+-----------------------------------------------------+-------+------+

Benchmark Results Comparison

No significant difference was registered for any benchmark run.

🤖 Kimi Code Review

The changes look correct. This PR updates the Amsterdam fork (EIP-8037) implementation to align with execution-specs v7.2.0 (EELS PR #2863), simplifying the state gas accounting from a complex "clamp-and-spill" mechanism to signed arithmetic.

Correctness & Safety:

1. Signed state gas accounting (crates/vm/levm/src/vm.rs:449, crates/vm/levm/src/call_frame.rs:290):
   Changing state_gas_used to i64 and tracking only a single entry snapshot (state_gas_used_at_entry) correctly implements the v7.2.0 spec where inline refunds may locally exceed gross charges. All conversions use try_from with explicit error handling, and arithmetic uses checked_add/checked_sub, preventing overflow/underflow panics.

2. Revert logic (crates/vm/levm/src/opcode_handlers/system.rs:1148-1167, 1220-1232):
   The incorporate_child_on_error math correctly uses signed arithmetic to return the child's net state gas consumption plus remaining reservoir to the parent. The max(0) clamp before converting back to u64 for the reservoir is spec-compliant.

3. Engine API validation (crates/networking/rpc/engine/payload.rs:195-201, 314-320):
   Correctly enforces structural validation per the Engine API spec: V4 rejects BAL presence, V5 requires it, and fork timestamps are validated with -32602 (Invalid params) rather than INVALID payload status.

4. Top-level failure handling (crates/vm/levm/src/vm.rs:894-905):
   The logic correctly splits the lumped state_gas_used into intrinsic (preserved for block accounting) and execution (refunded to reservoir) components on REVERT/Halt/OOG.

Minor observations:

• Field naming consistency: The rename from intrinsic_state_gas_charged to intrinsic_state_gas (crates/vm/levm/src/vm.rs:489) is applied consistently across all usages (hooks, utils, vm).
• Cleanup: Removal of unused std::mem import (crates/vm/levm/src/opcode_handlers/system.rs) and the five deleted snapshot fields from CallFrame reduces memory footprint per frame.

Suggestion (non-blocking):
Consider adding a debug assertion in credit_state_gas_refund (crates/vm/levm/src/vm.rs:649) verifying that amount does not exceed reasonable bounds (e.g., u64::MAX / 2), though the try_from conversions already prevent i64 overflow given Ethereum's gas limit constraints.

Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

🤖 Claude Code Review

Here is my review of PR #6671:

PR #6671 — fix(l1): credit inline state-gas refunds to local reservoir (bal-devnet-7 v7.2.0)

Overview

This PR aligns ethrex with EELS PR #2863, replacing the "clamp-and-spill" state-gas refund model with simpler inline credits. Key changes:

• state_gas_used changes type from u64 → i64 (can go negative when inline refunds exceed gross charges in a frame).
• Five VM-level fields and four CallFrame snapshot fields are removed; replaced by a single state_gas_used_at_entry: i64 per frame.
• credit_state_gas_refund simplifies from ~70 lines to ~10 lines.
• intrinsic_state_gas_charged is renamed to intrinsic_state_gas with corrected semantics (stores raw intrinsic amount, not a snapshot).
• engine_newPayloadV4/V5 now correctly return -32602 (Invalid params) on fork/field mismatch instead of PayloadStatus.INVALID.
• Test fixtures and eels_commit bumped to tests-bal@v7.2.0.

The 332-line net deletion is a healthy signal. The 8726/0 EF test run provides strong correctness coverage.

Issues Found

1. saturating_add on a critical path in finalize_execution (vm.rs ~line 921)

self.state_refund = self.state_refund.saturating_add(new_account_refund);

All similar accumulations in this file use checked_add and propagate InternalError::Overflow. A silent saturation at u64::MAX here would corrupt block-level state_gas_used accounting. Should be:

self.state_refund = self
    .state_refund
    .checked_add(new_account_refund)
    .ok_or(InternalError::Overflow)?;

2. saturating_sub / saturating_add in the top-level error path vs removed debug_assert (vm.rs, finalize_execution)

Old code had:

debug_assert!(
    self.state_gas_used >= self.intrinsic_state_gas_charged,
    "invariant: intrinsic is a floor on state_gas_used ({} >= {})",
    ...
);

The new code uses saturating_sub and saturating_add for the same path, silently masking the case where state_gas_used < intrinsic_signed or reservoir_signed overflows:

let execution_state_gas_used = self.state_gas_used.saturating_sub(intrinsic_signed);
let reservoir_signed = i64::try_from(self.state_gas_reservoir)
    ...
    .saturating_add(execution_state_gas_used);

Given the invariant is removed, a debug_assert!(self.state_gas_used >= intrinsic_signed) would help catch regressions during testing without cost in release builds.

3. Misleading error variant in incorporate_child_on_error (system.rs)

let child_state_gas_used = self
    .state_gas_used
    .checked_sub(state_gas_used_at_entry)
    .ok_or(InternalError::Underflow)?;

For i64::checked_sub, None is returned on arithmetic overflow (result below i64::MIN), not on going negative (which is expected and correct here). The label InternalError::Underflow is misleading — InternalError::Overflow is closer to what's actually being caught. Same pattern appears in handle_return_create.

4. Unreachable error arm (system.rs, both revert paths)

self.state_gas_reservoir =
    u64::try_from(net_return.max(0)).map_err(|_| InternalError::Overflow)?;

After .max(0), net_return is guaranteed non-negative, so u64::try_from can never return Err. The .map_err(...) arm is dead code. A comment or a debug_assert!(net_return >= 0) would make the invariant explicit without the confusing implied fallibility.

5. Missing timing comment for CREATE snapshot (system.rs ~line 818)

The removed code had an explicit guard:

// EIP-8037: Save snapshot AFTER charging CREATE's account state gas
let create_state_gas_used_snapshot = self.state_gas_used;

The new code drops this comment entirely:

new_call_frame.state_gas_used_at_entry = self.state_gas_used;

The ordering invariant (snapshot taken after the CREATE account state gas charge so the child's net on revert excludes the parent's charge) is now invisible. A short comment preserving this reasoning would help future readers verify correctness without re-deriving from the EELS spec.

6. Duplicate incorporate_child_on_error logic (system.rs)

The identical 10-line block appears in both handle_return_call and handle_return_create. This duplication existed before the PR, but the new logic is nontrivial (signed arithmetic, clamping, checked operations). A shared helper (e.g., fn incorporate_child_state_gas_on_revert(&mut self, entry: i64) -> Result<(), VMError>) would reduce drift risk.

Correctness Confirmations

• The i64 semantics for state_gas_used are correct: checked_sub in credit_state_gas_refund only returns None for actual i64 range overflow, not negative values — going negative is the intended behavior.
• The state_gas_used_at_entry: 0 default in CallFrame::new() is safe — the field is always overwritten before it is read (only used in handle_return_call/handle_return_create for child frames).
• The debug_assert_eq!(self.intrinsic_state_gas, 0, "intrinsic_state_gas set twice") guard is good and correctly carried over.
• Engine API checks in payload.rs are correctly ordered: BAL field check before block parsing (fast path), Amsterdam timestamp check after block parsing (requires block.header.timestamp). Semantic correctness of -32602 vs INVALID is properly documented.
• The intrinsic_state_gas rename (from intrinsic_state_gas_charged) is semantically correct: it now stores the raw intrinsic amount (equivalent to the old snapshot since state_gas_used starts at 0).

Summary

The PR is a clean, well-motivated simplification. The primary concern is Item 1 (silent saturation on state_refund in a block-accounting critical path) — the fix is one line. Items 2-5 are robustness/clarity issues rather than active bugs. The 8726/0 test result is reassuring but doesn't cover the saturation edge cases in Item 1.

Automated review by Claude (Anthropic) · sonnet · custom prompt

Greptile Summary

This PR ports ethereum/execution-specs#2863 into ethrex's LEVM, replacing the clamp-and-spill state-gas scaffolding with a simpler inline-credit model: state_gas_used becomes i64, credits decrement it directly (possibly negative), and revert handling restores the parent reservoir via a single state_gas_used_at_entry snapshot. It also fixes the intrinsic/execution split on top-level tx failure and corrects V4/V5 engine-API error codes.

• State-gas model rewrite (vm.rs, call_frame.rs, system.rs): five per-frame snapshot fields and three VM-level counters replaced by one i64 snapshot; credit_state_gas_refund simplified to reservoir += amount; state_gas_used -= amount.
• Intrinsic split on failure (vm.rs, utils.rs): new intrinsic_state_gas field keeps the intrinsic portion in state_gas_used after a top-level failure, preventing EIP-7702 and CREATE-tx paths from under-billing block_state_gas_used.
• Engine-API fix (payload.rs): V4/V5 field and fork-version mismatches now return JSON-RPC -32602 instead of PayloadStatus.INVALID.

Confidence Score: 4/5

The core accounting model is mechanically sound and fully tested (8726/8726 fixtures pass); two spots use silent saturation where the rest of the PR propagates errors explicitly.

The inline-credit simplification is well-reasoned: signed state_gas_used tracks net charges, revert math correctly restores the parent reservoir, and the intrinsic-split fix addresses a real under-billing path on failure. The only gaps are two saturating_* calls in finalize_execution that don't match the PR's own checked_* discipline.

The finalize_execution block in crates/vm/levm/src/vm.rs (lines 894-921) mixes saturating_* with the otherwise consistent checked_* pattern.

Sequence Diagram

sequenceDiagram
    participant TX as Transaction
    participant VM as VM (finalize_execution)
    participant Hook as DefaultHook
    participant RS as refund_sender

    TX->>VM: top-level failure (REVERT/HALT/OOG)
    VM->>VM: "if !collision: reservoir += (state_gas_used - intrinsic).max(0)"
    VM->>VM: "state_gas_used = intrinsic_state_gas"
    VM->>VM: "if CREATE-tx: reservoir += new_account_refund, state_refund += new_account_refund"
    VM->>Hook: finalize_execution(ctx_result)
    alt collision
        Hook->>Hook: "state_gas = (state_gas_used - state_refund).max(0)"
        Hook->>Hook: "regular_gas = gas_limit - reservoir"
        Hook-->>TX: return early
    else non-collision
        Hook->>RS: refund_sender(vm, ctx_result)
        RS->>RS: "state_gas = (state_gas_used - state_refund).max(0)"
        RS->>RS: "regular_gas = raw_consumed - intrinsic_state - reservoir_initial - spill"
        RS-->>TX: gas accounting complete
    end

Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
crates/vm/levm/src/vm.rs:898-901
The execution-portion refund calculation in the error path uses `saturating_sub` and `saturating_add` while the rest of this PR consistently propagates errors via `checked_*`. If `state_gas_used` were ever less than `i64::MIN + intrinsic_signed`, saturation would silently produce `i64::MIN` as `execution_state_gas_used` and then clamp the reservoir to zero — obscuring the accounting error rather than surfacing it.

```suggestion
                let execution_state_gas_used = self
                    .state_gas_used
                    .checked_sub(intrinsic_signed)
                    .ok_or(InternalError::Underflow)?;
                let reservoir_signed = i64::try_from(self.state_gas_reservoir)
                    .map_err(|_| InternalError::Overflow)?
                    .checked_add(execution_state_gas_used)
                    .ok_or(InternalError::Overflow)?
```

### Issue 2 of 2
crates/vm/levm/src/vm.rs:918-920
These two `saturating_add` calls on `u64` silently cap on overflow, inconsistent with the `checked_add` + `ok_or(InternalError::Overflow)` pattern used everywhere else in this PR (including in `charge_state_gas` and `credit_state_gas_refund` just above).

```suggestion
                self.state_gas_reservoir = self
                    .state_gas_reservoir
                    .checked_add(new_account_refund)
                    .ok_or(InternalError::Overflow)?;
                self.state_refund = self
                    .state_refund
                    .checked_add(new_account_refund)
                    .ok_or(InternalError::Overflow)?;
```

_{Reviews (1): Last reviewed commit: "chore(l1): drop devnet/PR refs from EIP-..." | Re-trigger Greptile}

🤖 Codex Code Review

Findings

1. payload.rs:224, payload.rs:350, auth/mod.rs:99, auth/mod.rs:120, auth/mod.rs:160, block_producer.rs:69, block_producer.rs:85
   The server is now strict about engine_newPayloadV4 vs V5, but the in-tree auth client/dev block producer is still pinned to engine_getPayloadV5 + engine_new_payload_v4 and still advertises only the old capability set. Once BAL-bearing Amsterdam/Osaka payloads are produced, that path will fetch a payload and then resubmit it to a method that now rejects it. This needs the companion client-side switch to engine_newPayloadV5 and fork-aware getPayloadV6/capability negotiation.

2. payload.rs:208, payload.rs:224, payload.rs:334, payload.rs:350
   The new method-version gate is after get_block_from_payload(). That means a V4 request with an Amsterdam timestamp, or a V5 request with a pre-Amsterdam timestamp, can still return PayloadStatus::INVALID if tx decoding/block reconstruction fails first, instead of the intended WrongParam (-32602). Since this check only depends on self.payload.timestamp, it should run before block reconstruction so the version error cannot be masked by unrelated payload invalidity.

3. payload.rs:183, payload.rs:198, serde_utils.rs:685
   engine_newPayloadV4 rejects BAL only after deserializing into Option<BlockAccessList>. With the current rlp_str_opt helper, blockAccessList: null and even blockAccessList: "" collapse to None, so an explicitly present BAL field can bypass the new V4 rejection. If the intent is “field must be absent”, you need to inspect raw JSON key presence before deserialization, like the V5 handler already does.

The LEVM signed state-gas refactor looked internally coherent from a static pass; my concrete concerns are on the RPC/versioning edges above. I couldn’t run cargo test in this environment because the sandbox blocks writable ~/.cargo/git dependency checkout, so this review is source-only.

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

Good catch, applied in deced65.

1. V5 + pre-Amsterdam now returns UnsupportedFork (-38005); comment cites amsterdam.md verbatim. Symmetric with abb964f; geth eth/catalyst/api.go:756-757 does the same.
2. Added EELS fixture citation at the V4 alt-hash heuristic (test_invalid_pre_fork_block_with_bal_hash_field[fork_BPO2ToAmsterdamAtTime15k-blockchain_test_engine]) and symmetric one at V5 (test_invalid_post_fork_block_without_bal_hash_field).
3. Not flipping V5 alt-hash to UnsupportedFork: EELS test_invalid_post_fork_block_without_bal_hash_field (tests/amsterdam/eip7928_block_level_access_lists/test_fork_transition.py:148) explicitly asserts EngineAPIError.InvalidParams (-32602), so the flip would break the engine fixture. All EELS engine_api_error_code sites use InvalidParams; none use UnsupportedFork.

Verified via hive + consume engine on v7.2.0 fixtures: 21/21 amsterdam fork-transition tests, 6/6 cancun excess_blob_gas fork-transition tests.