feat(l1): bump zkEVM fixtures to v0.3.3 and tighten witness validation

[ilitteri]

Summary

• Bump .fixtures_url_zkevm from zkevm@v0.3.0 to zkevm@v0.3.3. v0.3.3 introduces new witness_validation_* tests that exercise invalid execution witnesses (missing code, reordered headers, extra unused trie nodes, etc.).
• 16 of the new tests initially failed, exposing real gaps in ethrex's stateless validation. Fixed all of them:
  • get_account_code / get_code_metadata now error on missing bytecode instead of silently falling back to empty code. A missing code hash means the witness is invalid; the guest program must not conclude anything about the state.
  • RpcExecutionWitness::into_execution_witness tolerates extra unused state entries that don't decode as trie nodes (matches other clients, passes validation_state_extra_unused_trie_node).
  • GuestProgramState::from_witness rejects non-contiguous or reordered block_headers_bytes. Headers must form a contiguous ascending chain.
  • execute_blocks now propagates errors from get_first_invalid_block_hash instead of swallowing NoncontiguousBlockHeaders.
• Test runner: run_stateless_from_fixture reads the fixture's statelessOutputBytes valid flag (byte 32) and expects execution to fail when the fixture marks the block invalid (valid=0x00).

Final: 93/93 zkevm tests pass under --features stateless.

Test plan

• cd tooling/ef_tests/blockchain && make test-stateless-zkevm passes
• Full EF blockchain suite (make test) is no worse than main

[github-actions]

🤖 Kimi Code Review

The PR introduces important security hardening for witness validation and fixes error-handling bugs. Overall quality is high with good documentation.

Critical Security Improvements

crates/common/types/block_execution_witness.rs (lines 603–636)

The change from permissive to strict code lookup prevents a critical witness withholding attack. Previously, missing bytecode silently defaulted to empty, allowing a malicious prover to omit contract code and alter execution results.

// Before (insecure):
None => {
    println!("Missing bytecode... Defaulting to empty code.");
    Ok(Code::default())  // Silent failure!
}

// After (secure):
.ok_or_else(|| {
    GuestProgramStateError::Custom(format!(
        "missing bytecode for hash {} in execution witness", ...
    ))
})

Same applies to get_code_metadata (lines 631–644). These changes ensure the guest program halts with an error rather than executing with corrupted state when the witness is incomplete.

Bug Fix: Error Propagation

crates/guest-program/src/common/execution.rs (lines 70–79)

Previously, database errors from get_first_invalid_block_hash were silently discarded as Ok(()). The new explicit match arm correctly propagates failures:

Err(e) => Err(ExecutionError::GuestProgramState(e)),  // Was: Ok(())

Witness Structure Validation

crates/common/types/block_execution_witness.rs (lines 328–337)

The contiguous header check prevents manipulation where reordered or gapped ancestor headers could trick the stateless client into accepting an invalid chain. Using windows(2) is idiomatic, though note this permits single-header witnesses (correct for genesis/boundary cases).

Tolerance for Extra Witness Data

crates/common/types/block_execution_witness.rs (lines 180–183)

Switching from collect::<Result<_, _>> to .ok().map(...).collect() tolerates non-trie-node entries. This is safe because:

1. Nodes are referenced by hash during execution, not index
2. Extra junk cannot be referenced without the corresponding hash preimage
3. Malicious insertion of valid-but-unrelated nodes is harmless (state root verification fails if unreferenced nodes don't match)

Test Infrastructure

tooling/ef_tests/blockchain/test_runner.rs (lines 556–626)

The negative test handling is well-structured. The expected_stateless_valid parser correctly handles the fixture format (32-byte root || 1-byte flag). One minor suggestion: the hex slice bounds check at line 620 uses s.get(64..66) which safely handles short strings by returning None and defaulting to true—consider documenting this fallback behavior explicitly.

Minor Observations

1. Line 333: Ensure GuestProgramStateError::NoncontiguousBlockHeaders variant exists in the enum definition (not shown in diff).
2. Line 621: The strip_prefix("0x") handles both prefixed and raw hex gracefully.
3. Line 622: u8::from_str_radix(b, 16) correctly parses the valid flag.

The upgrade to zkevm fixtures v0.3.3 (.fixtures_url_zkevm) is appropriately bundled with these validation changes.

---

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

[github-actions]

Lines of code report

Total lines added: 1
Total lines removed: 0
Total lines changed: 1

Detailed view

+-------------------------------------------------------+-------+------+
| File                                                  | Lines | Diff |
+-------------------------------------------------------+-------+------+
| ethrex/crates/common/types/block_execution_witness.rs | 571   | +1   |
+-------------------------------------------------------+-------+------+

[github-actions]

🤖 Codex Code Review

1. GuestProgramState::from_witness() now rejects any header list that is not already in ascending order, but ethrex’s own witness builders still emit those headers in descending order. The new check at crates/common/types/block_execution_witness.rs:340 compares adjacent decoded headers as n, n+1, while both generators push headers while walking backward from the tip at crates/blockchain/blockchain.rs:1415 and crates/blockchain/blockchain.rs:1654. In practice that produces N-1, N-2, ..., so valid locally-generated witnesses will now fail with NoncontiguousBlockHeaders. This needs either reversing before validation, validating after sorting, or changing generation to emit oldest-first.

2. The stateless fixture runner now treats any witness-conversion failure as acceptable whenever the fixture says valid=0 at tooling/ef_tests/blockchain/test_runner.rs:571. That weakens the test: a regression in RpcExecutionWitness parsing/adaptation can be masked simply because the fixture is marked invalid. If that shortcut is intentional, it should at least constrain the accepted error class to witness-validation failures rather than silently accepting every conversion error.

Aside from those points, the other behavior changes look directionally correct: surfacing get_first_invalid_block_hash() errors instead of swallowing them, and failing hard on missing non-empty bytecode hashes, both improve witness soundness.

---

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

[github-actions]

🤖 Claude Code Review

Here is my review:

---

Review: feat(l1): bump zkEVM fixtures to v0.3.3 and tighten witness validation

Overview

This PR bumps the zkEVM fixture suite from v0.3.0 to v0.3.3, fixes 16 newly-exposed stateless validation bugs, and brings the test count to 93/93. The fixes span four distinct areas: trie node tolerance, block header ordering enforcement, missing-bytecode error propagation, and the test runner's invalid-fixture handling.

---

Correctness

RpcExecutionWitness::into_execution_witness — tolerating extra trie nodes (block_execution_witness.rs:184)

The change from a hard error on undecoded entries to a silent skip is well-reasoned. Node lookups happen by keccak hash, so an entry that cannot be decoded cannot be referenced during state access. The keccak is computed before the decode attempt, and only valid decode results are kept. The approach matches other clients and the comment adequately explains the invariant. No issue here.

GuestProgramState::from_witness — contiguity check (block_execution_witness.rs:343-346)

The early ordering check is correct and important. One minor concern:

if window[1].number != window[0].number + 1 {

window[0].number is u64, so + 1 wraps on overflow if window[0].number == u64::MAX. Practically impossible for current Ethereum, but arithmetic overflow should be avoided in consensus-adjacent code. Consider:

if window[1].number.checked_sub(1) != Some(window[0].number) {

Note also that a redundant contiguity check already exists at block_execution_witness.rs:513 (inside get_first_invalid_block_hash). The duplication is harmless and the earlier check is beneficial for fast failure, but it is worth a brief comment noting the intentional redundancy.

get_account_code / get_code_metadata — error on missing bytecode (block_execution_witness.rs:609-641)

This is a meaningful correctness improvement. The previous silent fallback to empty code was actively dangerous: a malformed witness missing bytecode for a contract would produce incorrect execution results that could still pass state-root validation (or worse, pass prover verification). The removal of the println! is also appropriate — it did not work in the SP1 zkVM environment anyway, and the old comment acknowledged as much.

The two functions now produce identical error messages. Consider using a shared helper or at minimum consistent formatting, to avoid them drifting apart.

execute_blocks — error propagation (execution.rs:72-80)

This was a real bug. NoncontiguousBlockHeaders from get_first_invalid_block_hash was silently swallowed by the original if let Ok(Some(...)) pattern. The new match arm correctly propagates it as ExecutionError::GuestProgramState(e). Straightforward and correct.

---

Test Runner

expected_stateless_valid (test_runner.rs:618-629)

The byte-offset parsing is correct. The comment on line 624 is helpful. A few observations:

• If statelessOutputBytes is present but shorter than 66 hex chars, s.get(64..66) returns None and the function defaults to true. This silently treats a truncated/malformed field as "valid", which could mask test setup bugs. A warning log or assertion on minimum length might be worth adding.
• The pattern Some(0) => false, _ => true is a candidate for !matches!(..., Some(0)), but the current form is readable.
• This function has non-trivial parsing logic (specific byte offset, conditional hex stripping) that would benefit from a small unit test, e.g.:

#[test]
fn test_expected_stateless_valid() {
    // 32 zero bytes (payload root) + 0x00 (invalid)
    let invalid = format!("0x{}{}", "00".repeat(32), "00");
    assert!(!expected_stateless_valid(Some(&invalid)));
    // 32 zero bytes + 0x01 (valid)
    let valid = format!("0x{}{}", "00".repeat(32), "01");
    assert!(expected_stateless_valid(Some(&valid)));
    assert!(expected_stateless_valid(None));
}

run_stateless_from_fixture — invalid-witness handling (test_runner.rs:572-608)

The two-stage handling (conversion failure vs. execution failure, each conditioned on expect_stateless_valid) is logically correct. The Err(e) branch with !expect_stateless_valid falls through silently — this is correct Rust, but a brief comment like // expected failure — proceed to next block would make the intent explicit to a future reader.

---

Minor / Style

• Line 571 (let witness_conversion_result = rpc_witness.into_execution_witness(...)) is notably long. Wrapping or inlining would improve readability.
• The two GuestProgramStateError::Custom(format!("missing bytecode for hash {} ...", ...)) messages in get_account_code and get_code_metadata are identical. Consider a shared private helper or a dedicated MissingBytecode(H256) error variant — it would also improve error handling at call sites.

---

Security

The changes are net positive from a security standpoint:

• Promoting silent empty-code fallback to a hard error is a meaningful improvement. In a ZK context, silent state fabrication is a critical failure mode.
• Block header ordering enforcement closes a gap that could allow a crafted witness to confuse the parent-hash validation chain.
• The trie node tolerance is the only relaxation. As argued above, the security argument holds because unreferenced garbage cannot affect execution results.

---

Summary

The core logic changes are correct and fix genuine validation gaps. The main actionable items are:

1. Guard window[0].number + 1 against u64 overflow (low practical risk, but should be consistent with safe-arithmetic practices elsewhere in consensus code).
2. Add a unit test for expected_stateless_valid to lock in the byte-offset parsing.
3. Consider a MissingBytecode(H256) error variant to deduplicate the two identical Custom messages and enable typed matching at call sites.

Everything else is clean. Good work fixing the silent fallback — that was the highest-risk item.

---

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

[greptile-apps]

Greptile Summary

This PR bumps the zkEVM fixture suite from v0.3.0 to v0.3.3, then fixes the 16 stateless-validation gaps the new witness_validation_* tests exposed. The core changes harden the witness validator: missing bytecode now errors instead of silently producing empty code, non-contiguous block-header sequences are rejected before BTreeMap normalization can mask them, extra undecodable trie nodes are tolerated (matching other clients), and errors from get_first_invalid_block_hash are propagated rather than swallowed. The test runner is extended to parse the statelessOutputBytes validity flag so that intentionally-invalid fixtures are tested for expected failure rather than expected success.

Confidence Score: 5/5

Safe to merge — all changes correctly harden witness validation with no P0/P1 issues found.

All four changes are logically sound and well-motivated by the new fixture test failures they fix. The error-propagation fix in execute_blocks closes a real silent-failure path. The contiguity check, missing-bytecode errors, and extra-node tolerance all match the expected semantics. No security regressions or data integrity issues identified.

No files require special attention.

Important Files Changed

Filename	Overview
crates/common/types/block_execution_witness.rs	Four targeted hardening changes: tolerate undecodable extra trie nodes (filter_map instead of early-return), validate header contiguity before BTreeMap insertion, and convert silent empty-code fallbacks in get_account_code/get_code_metadata into proper errors. All changes are logically sound.
crates/guest-program/src/common/execution.rs	Fixes error swallowing: the previous if let Ok(Some(...)) pattern silently ignored Err returns from get_first_invalid_block_hash (e.g. NoncontiguousBlockHeaders). The match now properly propagates those errors.
tooling/ef_tests/blockchain/test_runner.rs	Adds expected_stateless_valid() to parse the validity flag from statelessOutputBytes, and updates run_stateless_from_fixture to correctly handle witness-conversion failures and execution failures for intentionally-invalid fixtures.
tooling/ef_tests/blockchain/.fixtures_url_zkevm	Single-line bump from zkevm@v0.3.0 to zkevm@v0.3.3.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[run_stateless_from_fixture] --> B{block has\nexecutionWitness?}
    B -- No --> SKIP1[skip]
    B -- Yes --> C[parse statelessOutputBytes\nexpected_stateless_valid flag]
    C --> D[RpcExecutionWitness::into_execution_witness]
    D -- Error --> E{expect_stateless_valid?}
    E -- false --> SKIP2[continue — invalid witness\nis expected failure]
    E -- true --> FAIL1[return Err: conversion failed]
    D -- Ok --> F[from_witness: validate\nheader contiguity]
    F -- NoncontiguousBlockHeaders --> G[propagate via\nexecute_blocks match arm]
    F -- Ok --> H[get_account_code /\nget_code_metadata]
    H -- missing hash --> ERR[return Err: missing bytecode\nin execution witness]
    H -- Ok --> I[execute_blocks]
    I --> J{execute_result?}
    J -- Ok --> K{expect_stateless_valid?}
    K -- true --> SUCCESS[test passes]
    K -- false --> FAIL2[return Err: expected failure\nbut succeeded]
    J -- Err --> L{expect_stateless_valid?}
    L -- false --> SUCCESS2[test passes — expected failure]
    L -- true --> FAIL3[return Err: stateless\nexecution failed]

Loading

_{Reviews (1): Last reviewed commit: "Add a dedicated CI job for the stateless..." | Re-trigger Greptile}

[Copilot]

Pull request overview

Updates the zkevm stateless fixture set and tightens stateless witness validation so ethrex correctly rejects intentionally-invalid witnesses introduced in the newer fixtures.

Changes:

• Bump zkevm fixtures download to v0.3.3.
• Make stateless fixture runner interpret statelessOutputBytes validity flag and invert expectations for invalid-witness cases.
• Tighten witness validation: propagate block-header validation errors, reject non-contiguous/reordered headers, error on missing bytecode, and tolerate non-decodable extra unused trie-node entries during witness conversion.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

File	Description
tooling/ef_tests/blockchain/.fixtures_url_zkevm	Updates zkevm fixtures URL to v0.3.3.
tooling/ef_tests/blockchain/test_runner.rs	Reads fixture validity flag and expects failure for invalid-witness fixtures; handles witness-conversion failures accordingly.
crates/guest-program/src/common/execution.rs	Propagates get_first_invalid_block_hash errors instead of swallowing them.
crates/common/types/block_execution_witness.rs	Loosens decoding of extra unused trie entries, enforces contiguous ascending header chain, and errors on missing bytecode/code-metadata in the witness.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.