fix(l1): close EIP-8025 stateless witness validation gaps (re-enable 9 zkevm tests)

[avilagaston9]

Motivation

PR #6527 widened stateless coverage to all for_amsterdam fixtures and skipped 9
validation_* tests that exposed gaps in ethrex's witness handling. This PR
closes those gaps so the 9 tests pass.

Description

Four spec-aligned changes (refs to EELS execution-specs@projects/zkevm and
geth core/stateless/database.go are inline in the code):

• Tolerance — into_execution_witness and from_witness drop entries that
  fail RLP-decode instead of failing the whole conversion. Bad/extra entries
  can't be looked up by hash; if execution requires one, the trie or BLOCKHASH
  walk errors there.
• Header chain contiguity — from_witness rejects when
  headers[i].parent_hash != keccak(headers[i-1]). Mirrors EELS
  validate_headers.
• Codes completeness — get_account_code / get_code_metadata error on
  missing bytecode instead of silently returning empty code.
• Generator alignment — generate_witness_for_blocks now emits ancestors in
  ascending order (was descending). Required so ethrex-generated witnesses
  satisfy the contiguity check above.

How to test

make -C tooling/ef_tests/blockchain test-stateless

Expected: 8720 passed / 0 failed / 0 ignored, ~155 s. test-levm unaffected.

Checklist

• Updated STORE_SCHEMA_VERSION — N/A

[github-actions]

Lines of code report

Total lines added: 2
Total lines removed: 4
Total lines changed: 6

Detailed view

+-------------------------------------------------------+-------+------+
| File                                                  | Lines | Diff |
+-------------------------------------------------------+-------+------+
| ethrex/crates/blockchain/blockchain.rs                | 2489  | +2   |
+-------------------------------------------------------+-------+------+
| ethrex/crates/common/types/block_execution_witness.rs | 566   | -4   |
+-------------------------------------------------------+-------+------+

[greptile-apps]

Greptile Summary

This PR closes four EIP-8025 stateless-witness validation gaps, re-enabling 9 previously-skipped validation_* ef-tests: (1) malformed trie nodes/headers are now silently dropped instead of aborting conversion; (2) from_witness validates that the header list forms a contiguous chain in list order; (3) get_account_code / get_code_metadata now hard-error on missing bytecode; and (4) the witness generator emits ancestor headers in ascending order to satisfy the new contiguity check.

Confidence Score: 4/5

Safe to merge; only P2 style issues found, core logic is correct and all targeted tests pass.

All findings are P2 (a misleading comment and using Custom instead of the typed NoncontiguousBlockHeaders variant). The spec-aligned logic changes are well-reasoned and backed by EELS references.

crates/common/types/block_execution_witness.rs — comment accuracy and error variant usage in from_witness.

Important Files Changed

Filename	Overview
crates/common/types/block_execution_witness.rs	Four EIP-8025 changes: tolerance for malformed trie nodes/headers, contiguous-chain validation in from_witness, and hard errors on missing bytecode. Minor: a misleading comment on the malformed-entry path and a Custom error where the typed NoncontiguousBlockHeaders variant already exists.
crates/blockchain/blockchain.rs	Adds block_headers_bytes.reverse() in both generate_witness_for_blocks code paths so the emitted ancestor list is ascending, satisfying the new contiguity check in from_witness.
tooling/ef_tests/blockchain/tests/all.rs	Removes the stateless-specific skip list (9 tests) and simplifies to a single #[cfg(not(feature = "sp1"))] empty slice, re-enabling all formerly-skipped validation tests.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["RpcExecutionWitness\n(from RPC / fixture)"] -->|"into_execution_witness()"| B["ExecutionWitness\n(block_headers_bytes in ascending order)"]
    GEN["generate_witness_for_blocks\n(walks chain backward)"] -->|"reverse() → ascending"| B

    B -->|"from_witness()"| C{For each header bytes}
    C -->|"decode fails"| D["prev_hash = None\nskip (tolerance)"]
    D --> C
    C -->|"decode ok\nprev_hash != None AND\nheader.parent_hash != prev_hash"| E["❌ NoncontiguousBlockHeaders\nerror"]
    C -->|"decode ok\nchain link valid"| F["insert into block_headers\nprev_hash = keccak(bytes)"]
    F --> C
    C -->|"done"| G["GuestProgramState"]

    G --> H["get_account_code(hash)"]
    H -->|"hash missing in codes_hashed"| I["❌ Database error\n(was: silent empty code)"]
    H -->|"hash found"| J["✅ Return Code"]

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: crates/common/types/block_execution_witness.rs
Line: 330-334

Comment:
**Misleading inline comment — check is skipped, not failed**

The comment says "the next parent_hash check fails," but when `prev_hash` is set to `None`, the subsequent `if let Some(expected_parent) = prev_hash` guard in the next iteration simply does not fire — the check is bypassed, not triggered-and-failed. A validator reading this could mistake the skipping for active rejection.

```suggestion
            let Ok(header) = BlockHeader::decode(bytes.as_ref()) else {
                // Malformed entry: skip it and reset the chain cursor so the
                // next decodable header is accepted without a parent-hash check.
                prev_hash = None;
                continue;
            };
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: crates/common/types/block_execution_witness.rs
Line: 337-340

Comment:
**Use the existing `NoncontiguousBlockHeaders` error variant**

There is already a typed variant `GuestProgramStateError::NoncontiguousBlockHeaders` (defined a few lines above) with the message "Non-contiguous block headers (there's a gap in the block headers list)". Using `Custom(...)` here bypasses that variant and makes error matching on the call-site harder.

```suggestion
            {
                return Err(GuestProgramStateError::NoncontiguousBlockHeaders);
            }
```

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "Collapse the nested `if let Some(expecte..." | Re-trigger Greptile}

[greptile-apps]

Misleading inline comment — check is skipped, not failed

The comment says "the next parent_hash check fails," but when prev_hash is set to None, the subsequent if let Some(expected_parent) = prev_hash guard in the next iteration simply does not fire — the check is bypassed, not triggered-and-failed. A validator reading this could mistake the skipping for active rejection.

Suggested change

	let Ok(header) = BlockHeader::decode(bytes.as_ref()) else {
	// Malformed entry is a chain break; the next parent_hash check fails.
	prev_hash = None;
	continue;
	};
	let Ok(header) = BlockHeader::decode(bytes.as_ref()) else {
	// Malformed entry: skip it and reset the chain cursor so the
	// next decodable header is accepted without a parent-hash check.
	prev_hash = None;
	continue;
	};

Prompt To Fix With AI

This is a comment left during a code review.
Path: crates/common/types/block_execution_witness.rs
Line: 330-334

Comment:
**Misleading inline comment — check is skipped, not failed**

The comment says "the next parent_hash check fails," but when `prev_hash` is set to `None`, the subsequent `if let Some(expected_parent) = prev_hash` guard in the next iteration simply does not fire — the check is bypassed, not triggered-and-failed. A validator reading this could mistake the skipping for active rejection.

```suggestion
            let Ok(header) = BlockHeader::decode(bytes.as_ref()) else {
                // Malformed entry: skip it and reset the chain cursor so the
                // next decodable header is accepted without a parent-hash check.
                prev_hash = None;
                continue;
            };
```

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

Use the existing NoncontiguousBlockHeaders error variant

There is already a typed variant GuestProgramStateError::NoncontiguousBlockHeaders (defined a few lines above) with the message "Non-contiguous block headers (there's a gap in the block headers list)". Using Custom(...) here bypasses that variant and makes error matching on the call-site harder.

Suggested change

	{
	return Err(GuestProgramStateError::Custom(
	"witness headers are not contiguous".to_string(),
	));
	{
	return Err(GuestProgramStateError::NoncontiguousBlockHeaders);
	}

Prompt To Fix With AI

This is a comment left during a code review.
Path: crates/common/types/block_execution_witness.rs
Line: 337-340

Comment:
**Use the existing `NoncontiguousBlockHeaders` error variant**

There is already a typed variant `GuestProgramStateError::NoncontiguousBlockHeaders` (defined a few lines above) with the message "Non-contiguous block headers (there's a gap in the block headers list)". Using `Custom(...)` here bypasses that variant and makes error matching on the call-site harder.

```suggestion
            {
                return Err(GuestProgramStateError::NoncontiguousBlockHeaders);
            }
```

How can I resolve this? If you propose a fix, please make it concise.

[github-actions]

🤖 Claude Code Review

Now I have a full picture of the PR. Here is my review:

---

PR #6541 Review: fix(l1): close EIP-8025 stateless witness validation gaps

Overview

This PR closes four stateless witness validation gaps that were preventing 9 validation_* EF-test fixtures from passing under the stateless feature flag. The changes are well-motivated, commit messages are thorough, and spec references (EELS + geth) are provided inline. All 9 previously-skipped tests are re-enabled.

---

Correctness Analysis

1. Header contiguity check — comment is inaccurate (minor bug)

block_execution_witness.rs, from_witness:

let Ok(header) = BlockHeader::decode(bytes.as_ref()) else {
    // Malformed entry is a chain break; the next parent_hash check fails.
    prev_hash = None;
    continue;
};
if let Some(expected_parent) = prev_hash
    && header.parent_hash != expected_parent
{
    return Err(...);
}

Setting prev_hash = None means the if let Some(expected_parent) guard on the next valid header evaluates to false, so the check is skipped, not "failed" as the comment claims. The real behavior is: a malformed entry resets the chain anchor, and the subsequent valid header starts a new unverified segment.

The commit message repeats the same inaccuracy: "subsequent headers won't satisfy the parent_hash check." They actually bypass it. The behavior is spec-correct (tolerance) but the comment will mislead future readers. Suggested replacement:

// Drop the malformed entry; reset the chain anchor so the next valid
// header starts a new contiguous segment (EIP-8025 tolerance).
prev_hash = None;

2. Malformed-entry reset creates a potential contiguity bypass window

As a consequence of Point 1: a witness shaped as [valid_A, malformed_blob, valid_B_not_contiguous_with_A] passes the contiguity check because the malformed blob resets prev_hash. Whether the spec intends this is not entirely clear from the EELS link cited. Given that validation_headers_non_contiguous_chain (a pure-valid-header case) is now correctly rejected and validation_headers_malformed_rlp_header is correctly accepted, this is likely intentional — but worth a brief note or a reference to the EELS line that makes the "chain-restart-on-junk" behavior explicit.

3. into_execution_witness — removal of the 0x80 (RLP null) special case

Old code:

if b == Bytes::from_static(&[0x80]) {
    return None;
}
let hash = keccak(&b);
Some(Node::decode(&b).map(|node| (hash, node)))

New code:

let node = Node::decode(&b).ok()?;
Some((keccak(&b), node))

The 0x80 case is now implicitly handled because Node::decode on [0x80] (RLP null) returns an error, so ok()? drops it. This is cleaner, but the old comment explained why some implementations emit 0x80 (other implementations allow a Null node). Dropping that comment removes useful cross-client context. Consider keeping one line like:

// EIP-8025 tolerance: drop entries that don't decode (e.g. RLP-null 0x80 from some clients).

4. get_account_code / get_code_metadata — error variant mismatch

The new missing-bytecode paths use GuestProgramStateError::Database, while the header contiguity failure uses GuestProgramStateError::Custom. If the caller dispatches on error type, this inconsistency could matter. Worth confirming these are intentionally different, or aligning them.

---

Code Quality

5. Duplicated block_headers_bytes.reverse() in blockchain.rs

The identical fix appears at two call sites (~line 1439 and ~line 1676). This is fine for now, but the accompanying comments (and the EELS URL) are copy-pasted verbatim. If a third call site is added later, it's easy to miss. Not blocking, but worth noting.

6. hex::encode(code_hash) on H256

H256 likely already implements Display / LowerHex, so format!("{code_hash:x}") or format!("{code_hash:#x}") may avoid the hex::encode dependency. Minor.

---

Test Coverage

The test simplification in all.rs is correct: the #[cfg(feature = "stateless")] override with 9 skips is removed, and the previously separate #[cfg(not(any(feature = "sp1", feature = "stateless")))] empty slice collapses into #[cfg(not(feature = "sp1"))]. The result is that the stateless path now runs with the same (empty) extra-skip list as the baseline, which is exactly right.

No new unit tests are added for the individual behaviors (contiguity check, decode tolerance, missing-code error). Given that the EF fixture suite covers these paths, this is acceptable — but a targeted unit test for from_witness contiguity rejection would be a cheap safety net for regressions.

---

Summary

Area	Verdict
Spec alignment (EELS / geth)	Correct
EF test re-enablement	Correct
Ascending header order fix	Correct
Contiguity check logic	Correct, comment misleading
Tolerance (drop malformed entries)	Correct
Codes completeness (error on miss)	Correct
Comment clarity	One inaccurate comment (Point 1)
Error variant consistency	Minor inconsistency (Point 4)

The logic is sound and the spec alignment looks right. The main ask before merge is fixing the comment in the malformed-header branch — it currently says the next check "fails" when it is actually skipped.

---

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

[github-actions]

🤖 Codex Code Review

1. GuestProgramState::from_witness still lets some non-contiguous header sets through. At crates/common/types/block_execution_witness.rs:329 the new check only enforces parent_hash == keccak(previous header bytes); it never enforces header.number == prev.number + 1. On top of that, a malformed entry resets prev_hash at crates/common/types/block_execution_witness.rs:330, so the next decoded header is not checked against the previous chain at all. That would still accept witnesses like [98, <garbage>, 100] or forged-number chains like [98, 100(parent=hash98)] unless block 99 is actually queried. The fallback check does exist, but its Err(NoncontiguousBlockHeaders) path is ignored in crates/guest-program/src/common/execution.rs:72. I’d add an explicit number-adjacency check here, or propagate that error instead of discarding it.

2. The header-order change is not backward-compatible with cached witnesses. Newly generated witnesses are reversed into ascending order at crates/blockchain/blockchain.rs:1439 and crates/blockchain/blockchain.rs:1676, but cached RPC witnesses are still served back verbatim from crates/storage/store.rs:2025 via crates/networking/rpc/debug/execution_witness.rs:84. RpcExecutionWitness::into_execution_witness also preserves self.headers unchanged at crates/common/types/block_execution_witness.rs:222. After an upgrade, the same node can therefore return old descending witnesses for cached blocks and new ascending witnesses for fresh ones, and the new stateless path will reject the cached ones as “not contiguous”. This needs cache invalidation/migration or read-side normalization.

Assumption: I’m reading the EIP-8025 intent here as “tolerate unused malformed entries, but require the usable header chain itself to be contiguous”. Under that interpretation, Item 1 is still a real validation gap.

I couldn’t run cargo check here because rustup tried to write under /home/runner/.rustup, which is read-only in this sandbox. Otherwise, the stricter missing-bytecode handling looks directionally correct.

---

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