fix(l1): align to latest EIP-8025 spec and unskip stateless validation tests#6740
fix(l1): align to latest EIP-8025 spec and unskip stateless validation tests#6740iovoid wants to merge 19 commits into
Conversation
|
Lines of code reportTotal lines added: Detailed view |
🤖 Claude Code ReviewNow I have enough context for a thorough review. PR #6740 Review — fix(l1): align to latest EIP-8025 spec and unskip stateless validation testsOverall this is a well-executed spec-alignment PR. The structural changes are clean, the security posture is improved, and removing 38 test skips is strong evidence the fixes are correct. A few things warrant attention. Critical / Correctness1. CALL gas ordering — In the new vm.current_call_frame.check_gas(static_cost)?; // line ~934
vm.substate.add_accessed_address(callee);
let address_is_empty = vm.db.get_account(callee)?.is_empty();
let (is_delegation_7702, eip7702_gas_consumed, code_address, bytecode) =
eip7702_get_code(...)?;
let create_cost = if address_is_empty && !value.is_zero() {
gas_cost::CALL_TO_EMPTY_ACCOUNT
} else { 0 };
if is_delegation_7702 {
vm.current_call_frame.check_gas(
static_cost.checked_add(create_cost)? .checked_add(eip7702_gas_consumed)?,
)?;
}When 2. Silent node drop changes error semantics for all callers
.collect::<Result<_, RLPDecodeError>>()?to: let node = Node::decode(&b).ok()?;
Some((keccak(&b), node))Malformed trie nodes are now silently dropped instead of failing. This follows EELS tolerance behaviour for extra nodes, as the test Security3. // `fork` and `activation` are not compared: EELS and ethrex number forks differently,
// and the spec stores activation values for canonical-root determinism rather than
// verifier cross-checking. The blob-schedule check below is a partial proxy…The comment is transparent about the gap, but it means a prover claiming a different fork (e.g. sending Prague-era data tagged as Cancun) would pass validation as long as the blob-schedule fields match. A block at the boundary where two forks happen to share a blob schedule would be accepted under either fork label. This is a spec-fidelity gap that should at minimum be a documented 4. Public key zero-index access is safe but add a length assert let pk_bytes: &[u8] = public_key;
if pk_bytes[0] != 0x04 {
Rust Correctness / Idiomatic5. For vm.current_call_frame.check_gas(static_cost)?; // check 1
// ... state reads ...
if is_delegation_7702 {
vm.current_call_frame.check_gas(static_cost + create + eip7702)?; // check 2
}The second check covers a strictly larger value, so the first check is redundant on that path. This is harmless but creates a mental mismatch when reading the code. The first call's stated purpose ("before any state read") is the real invariant — it could be annotated to make clear it's an early-exit guard, not the authoritative gas deduction check. 6. Unnecessary final hash in prev_hash = Some(header.compute_block_hash(crypto));This is computed for every header including the last one, whose hash is never used. The last iteration's 7. ProgramInput::Direct { blocks, execution_witness } => {
// ...
Ok(ProgramOutput {
new_payload_request_root: [0u8; 32], // sentinel
valid: true,
chain_id,
})
}This is documented in the function-level comment, but Minor / Nits
What's Good
Automated review by Claude (Anthropic) · sonnet · custom prompt |
🤖 Codex Code Review
Otherwise, the stricter missing-bytecode handling and the CALL-family gas-ordering changes look directionally correct. I couldn’t run Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Greptile SummaryThis PR aligns ethrex's EIP-8025 stateless validation with the latest Amsterdam spec (bal-devnet-7 /
Confidence Score: 4/5Safe to merge; the execution path is correct and the unskipped test suite provides substantial coverage of the new canonical wire format. The CALL handler reordering is the riskiest change: it correctly moves the EIP-7702 code fetch after the static gas check, but the crates/vm/levm/src/opcode_handlers/system.rs — specifically the
|
| Filename | Overview |
|---|---|
| crates/vm/levm/src/opcode_handlers/system.rs | Reorders gas checks before state reads for all four CALL variants to align with EELS; introduces check_gas helper. Non-delegation CALL to empty accounts still defers the create_cost check to increase_consumed_gas rather than doing an explicit pre-read check. |
| crates/common/types/block_execution_witness.rs | Extracts header decoding/validation helpers, adds validate_witness_headers_chain (input-order contiguity check), tightens get_account_code/get_code_metadata from silent-default to hard MissingBytecode error, and silently drops malformed trie nodes (.ok()?) instead of propagating RLPDecodeError — intentional EELS tolerance semantics but errors for needed malformed nodes now surface as the less-descriptive RootNotFound. |
| crates/guest-program/src/l1/input.rs | Splits ProgramInput into Direct/Wire variants (feature-gated), adds canonical SSZ structs (CanonicalForkConfig, CanonicalBlobSchedule, CanonicalForkActivation, fixed-size PublicKeysList), and adds decode_canonical_stateless_input_bytes with schema-ID validation. |
| crates/guest-program/src/l1/program.rs | Adds execute_decoded dispatcher, extracts execute_canonical_stateless_input_decoded (returning ProgramOutput with errors absorbed as valid=false), adds blob-schedule cross-check in validate_canonical_chain_config, and validates per-transaction public keys against recovered senders. |
| tooling/ef_tests/blockchain/test_runner.rs | Routes fixtures with statelessInputBytes through the new canonical decode path; adds explicit header decode step before into_execution_witness; gates two-pass parallel test under not(stateless) to avoid missing merkleizer dependency. |
| crates/prover/src/lib.rs | Adds a compile_error! guard preventing eip-8025 from being combined with ZK backends that require rkyv-serialisable ProgramInput. |
| crates/prover/src/backend/exec.rs | Delegates to execute_decoded and surfaces valid=false as Err so test callers treat it as execution failure, matching legacy path semantics. |
| tooling/ef_tests/blockchain/tests/all.rs | Removes all previously-skipped EIP-8025 tests now that fixtures are regenerated against bal-devnet-7 and validation gaps are closed; EXTRA_SKIPS for stateless is now empty. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Input bytes / ProgramInput] --> B{Feature: eip-8025?}
B -- No --> C[ProgramInput struct\nblocks + ExecutionWitness]
C --> D[execute_blocks]
D --> E[ProgramOutput]
B -- Yes --> F{ProgramInput variant}
F -- Direct --> G[execute_blocks direct\nsentinel ProgramOutput]
F -- Wire --> H{DecodedEip8025 variant}
H -- Legacy --> I[decode_eip8025_legacy\nnew_payload_request + witness]
H -- Canonical --> J[decode_canonical_stateless_input_bytes\nschema_id=0x0001 + SSZ]
I --> K[validate_eip8025_execution\nhash_tree_root check]
J --> L[validate_canonical_chain_config\nchain_id + blob_schedule]
L --> M[decode_witness_headers]
M --> N[validate_witness_headers_chain\ninput-order contiguity]
N --> O[into_execution_witness\nsilent-drop malformed nodes]
O --> P[validate_eip8025_amsterdam_execution\npublic_keys sender match]
P --> Q[execute_blocks]
Q --> R{valid?}
R -- Yes --> S[ProgramOutput valid=true]
R -- No --> T[ProgramOutput valid=false\nExecBackend → Err]
K --> S
G --> S
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
crates/vm/levm/src/opcode_handlers/system.rs:98-106
**Missing `check_gas(create_cost)` for non-EIP-7702 CALL to empty account**
The explicit gas check for `create_cost` (`CALL_TO_EMPTY_ACCOUNT`, 25,000 gas) only runs inside the `is_delegation_7702` branch. For a regular (non-delegation) CALL with a non-zero `value` to an empty account, `create_cost` is never `check_gas`-ed before proceeding — it is only deducted later at `increase_consumed_gas`. EELS has an explicit `check_gas(GAS_NEW_ACCOUNT)` step after the account read that is independent of the delegation path; this ordering is what allows validators to reject witnesses that don't include the callee account when there is insufficient gas. While this does not affect execution correctness (OOG is still caught at `increase_consumed_gas`), it diverges from the stated EELS alignment goal and is inconsistent with how the delegation case is handled.
Reviews (1): Last reviewed commit: "docs(l1): tighten verbose comments" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if is_delegation_7702 { | ||
| vm.current_call_frame.check_gas( | ||
| static_cost | ||
| .checked_add(create_cost) | ||
| .ok_or(ExceptionalHalt::OutOfGas)? | ||
| .checked_add(eip7702_gas_consumed) | ||
| .ok_or(ExceptionalHalt::OutOfGas)?, | ||
| )?; | ||
| } |
There was a problem hiding this comment.
check_gas(create_cost) for non-EIP-7702 CALL to empty account
The explicit gas check for create_cost (CALL_TO_EMPTY_ACCOUNT, 25,000 gas) only runs inside the is_delegation_7702 branch. For a regular (non-delegation) CALL with a non-zero value to an empty account, create_cost is never check_gas-ed before proceeding — it is only deducted later at increase_consumed_gas. EELS has an explicit check_gas(GAS_NEW_ACCOUNT) step after the account read that is independent of the delegation path; this ordering is what allows validators to reject witnesses that don't include the callee account when there is insufficient gas. While this does not affect execution correctness (OOG is still caught at increase_consumed_gas), it diverges from the stated EELS alignment goal and is inconsistent with how the delegation case is handled.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/vm/levm/src/opcode_handlers/system.rs
Line: 98-106
Comment:
**Missing `check_gas(create_cost)` for non-EIP-7702 CALL to empty account**
The explicit gas check for `create_cost` (`CALL_TO_EMPTY_ACCOUNT`, 25,000 gas) only runs inside the `is_delegation_7702` branch. For a regular (non-delegation) CALL with a non-zero `value` to an empty account, `create_cost` is never `check_gas`-ed before proceeding — it is only deducted later at `increase_consumed_gas`. EELS has an explicit `check_gas(GAS_NEW_ACCOUNT)` step after the account read that is independent of the delegation path; this ordering is what allows validators to reject witnesses that don't include the callee account when there is insufficient gas. While this does not affect execution correctness (OOG is still caught at `increase_consumed_gas`), it diverges from the stated EELS alignment goal and is inconsistent with how the delegation case is handled.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
False for several reasons:
- To know the account is empty (and thus needs creation) you need to read it and therefore include a proof of non-existence in the witness
- The code in fact only uses the creation cost when is_delegation is True. See https://github.com/ethereum/execution-specs/blob/forks/amsterdam/src/ethereum/forks/amsterdam/vm/instructions/system.py#L407-L432
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
|
| ); | ||
|
|
||
| if is_delegation_7702 { | ||
| vm.current_call_frame.check_gas( |
There was a problem hiding this comment.
EELS Amsterdam call() L454 does check_gas(extra_gas + extend_memory.cost) where extra_gas = access + transfer + delegation_access_cost; create_cost is not included. It is charged later from the separate state-gas pool via charge_state_gas (L465-468), not regular gas.
Including create_cost in the regular-gas preview here can OOG on regular gas in the delegation + empty-account + value-transfer case where EELS would proceed and pay create_cost from the state-gas reservoir. CI is green so no v0.4.1 fixture triggers it, but the asymmetry vs the non-delegation branch (no second check_gas) is worth a comment or dropping create_cost from the preview. Intentional?
| } else { | ||
| gas_cost::WARM_ADDRESS_ACCESS_COST | ||
| }; | ||
| let static_cost = memory_expansion_cost |
There was a problem hiding this comment.
Static-gas block (new_memory_size + address_was_cold + static_cost + check_gas(static_cost)) is duplicated across CALL/CALLCODE/DELEGATECALL/STATICCALL (also at lines 238, 356, 474); only value_cost varies. Extract a helper returning (new_memory_size, address_was_cold, static_cost) after check_gas so the static-then-state ordering invariant lives in one place. Can be a follow-up.
There was a problem hiding this comment.
The change was short, so done in bf87942
| // same final state is reached. | ||
| // Skipped under `stateless`: `eip-8025` on `ethrex-blockchain` drops the merkleizer | ||
| // Sender the parallel path needs; the non-stateless runs still cover this. | ||
| #[cfg(not(feature = "stateless"))] |
There was a problem hiding this comment.
The current wording suggests this is a temporary skip waiting on a fix, but I think it's actually a permanent design boundary: the stateless harness doesn't drive the blockchain.add_block pipeline that run_two_pass_parallel exercises, and parallel BAL-driven merkleization gives no benefit in zkVM guest builds (single-threaded proof targets). The parallel path is already covered by the non-stateless runs against the same fixtures.
Could you reword along the lines of "two-pass parallel exercises the blockchain pipeline's BAL-driven merkleization, which the stateless harness doesn't use; non-stateless runs already cover it on the same fixtures" so future readers don't treat this as a regression to chase?
| self.codes_hashed | ||
| .get(&code_hash) | ||
| .cloned() | ||
| .ok_or(GuestProgramStateError::MissingBytecode(code_hash)) |
There was a problem hiding this comment.
get_account_code now hard-errors with MissingBytecode instead of silently returning empty. Looks correct for stateless validation, but can you confirm no non-stateless caller relied on the prior silent-empty fallback? A quick rg get_account_code over the workspace would lock it down. Test matrix is green but worth eyeballing.
There was a problem hiding this comment.
Confirmed get_account_code isn't used elsewhere. Other methods (RPC, etc) use the Store and not the VM to access bytecode.
There was a problem hiding this comment.
Reviewed against EELS tests-zkevm@v0.4.1 (forks/amsterdam/vm/instructions/system.py). Gas-charge ordering rewrite matches spec; raising a few items inline.
Verified separately:
.fixtures_url_amsterdam(bal@v7.2.0) untouched.vectors_zkevm/stays a separate extraction root fromvectors/. This isolation is correct and worth keeping as a design invariant: zkevm and bal fixture bundles version on independent cadences, so when the zkevm bundle drifts out of date relative to the current bal spec (as happened with zkevm@v0.3.3 vs bal-devnet-7 on main) we need the two trees to stay disjoint. Without that separation, extracting one would clobber the other and the suite the maintainers haven't bumped yet would silently break.Test - {ubuntu-22.04, ubuntu-22.04-arm, macos-15}(which runsmake -C tooling/ef_tests/blockchain test) is green on all three runners; both bal@v7.2.0 (test-levm) and the re-broadened zkevm@v0.4.1 (test-stateless) are exercised.- Pre-Amsterdam forks unchanged:
gas_cost::callstill bakesCALL_TO_EMPTY_ACCOUNTintocall_gas_costswhenfork < Amsterdam(gas_cost.rs:748).
Approving once the delegation-branch check_gas question and the two-pass-parallel comment are addressed.
|
You are right about the gas, I was looking at the forks/amsterdam branch which I assumed was more up-to-date, but actually doesn't include EIP-8037. |
Lines of code reportTotal lines added: Detailed view |
ElFantasma
left a comment
ElFantasma
left a comment
There was a problem hiding this comment.
LGTM, but left some non-blocking comments. The get_account(callee) may be the more important one
| )?; | ||
|
|
||
| vm.substate.add_accessed_address(callee); | ||
| let address_is_empty = vm.db.get_account(callee)?.is_empty(); |
There was a problem hiding this comment.
get_account(callee)? unconditionally reads the callee account. But address_is_empty is only consumed at lines 69 (create_cost, gated on !value.is_zero()) and 111 (needs_state_gas, same gate). So when value.is_zero(), the read result is computed and immediately thrown away.
That's the same class of issue this PR fixes for eip7702_get_code: a state read that EELS doesn't perform, which forces witnesses to include records that EELS-conformant witnesses can legitimately omit. EELS' spec only checks target.balance + value == 0 (the empty-via-create case) when value > 0; for value = 0 calls, the account is not loaded for this purpose.
Low-effort fix:
let address_is_empty = if value.is_zero() {
false // unused in either gated path
} else {
vm.db.get_account(callee)?.is_empty()
};Worth verifying against the unskipped EIP-8025 tests — if none of them currently catch this, it's because no witness fixture is sparse enough yet; that'll be a future witness regression.
Non-blocking but same family as the bug this PR fixes.
| self.codes_hashed | ||
| .get(&code_hash) | ||
| .cloned() | ||
| .ok_or(GuestProgramStateError::MissingBytecode(code_hash)) |
There was a problem hiding this comment.
Removing the silent "default to empty Code" fallback is the right call — it was masking exactly the kind of bug the system.rs CALL-reorder is fixing (witness misses a code entry; we silently default to empty; downstream execution diverges from EELS).
Worth a sanity check that every code-hash path the VM walks during execution is now preceded by an addressed-cold check or the witness invariant guarantees inclusion. If get_account_code can be reached for a code hash that wasn't already established as touched, this change converts the previous silent-but-wrong behavior into a hard MissingBytecode failure — possibly during stateless validation of an otherwise-valid block.
Based on the PR description ("unskipping tests"), the unskip is the regression net here. If those tests cover the once-fallback cases, we're good. Non-blocking.
| ) -> Result<(), GuestProgramStateError> { | ||
| for pair in headers.windows(2) { | ||
| let (prev, next) = (&pair[0], &pair[1]); | ||
| if next.number != prev.number.saturating_add(1) |
There was a problem hiding this comment.
nit: prev.number.saturating_add(1) silently lets a contiguous chain pass at u64::MAX → u64::MAX. Astronomically irrelevant, but the safer expression is prev.number.checked_add(1) which returns None and lets the comparison fail loudly — matches the spirit of the function ("validate chain linkage, surface violations").
| /// `Direct` carries in-memory blocks + witness (test path). `Wire` carries an | ||
| /// already-decoded EIP-8025 stateless input from spec wire bytes. | ||
| #[cfg(feature = "eip-8025")] | ||
| pub enum ProgramInput { |
There was a problem hiding this comment.
The same identifier ProgramInput is a struct (line 15) without the eip-8025 feature and an enum (here) with it. Every consumer of ProgramInput::new(blocks, witness) works under both because both have a matching new method — but ProgramInput::default() returns different shapes (struct-default vs Direct { ..Default::default() }), and any code that pattern-matches on ProgramInput::Wire(...) or ProgramInput::Direct { .. } only compiles under one cfg.
The shape is consistent enough that this is probably fine, but a one-line module-level doc note ("this type's kind is feature-gated; cross-feature destructuring will not compile in the disabled branch") would save the next contributor a confused 10 minutes. Non-blocking.
4 participants
Motivation
The stateless execution zkEVM fixtures have been updated to reflect bal-devnet-7 spec (which we currently implement) and some other minor changes to the wire format.
Description
Unskips all of the EIP-8025 tests, implements the latest changes to the ProgramInput, and closes validation gaps.
Since we were using the old witness format, a small refactor is needed to keep support.
In the process we found a divergence with the spec in how CALLs order gas checks and state reads. Fixing this was necessary since we were requiring a code to be in the witness when that wasn't needed.