feat(l1): add ssz encoding for witness in eip-8025#6524
Conversation
Greptile SummaryThis PR replaces
Confidence Score: 3/5Not safe to merge as-is — two P1 issues could cause panics on untrusted witness data and silent error masking in the execution program entry point. Two P1 findings: (1) .unwrap() panics on user-supplied trie node data exceeding SSZ list bounds in to_ssz_bytes, and (2) .is_ok() in execution_program_eip8025_bytes silently drops all internal errors as valid:false. Both are on the primary changed code paths. crates/common/types/block_execution_witness.rs (unwrap panics), crates/guest-program/src/l1/program.rs (silent error swallowing)
|
| Filename | Overview |
|---|---|
| crates/common/types/block_execution_witness.rs | Adds SSZ encoding/decoding for ExecutionWitness under feature eip-8025; contains multiple .unwrap() calls on user-controlled trie node data that will panic if SSZ bounds are exceeded |
| crates/guest-program/src/l1/program.rs | Adds execution_program_eip8025_bytes helper; silently swallows all validation errors (including internal failures) via .is_ok(), and has a stale rkyv doc comment |
| crates/common/types/eip8025_ssz.rs | Replaces flat execution_requests byte-list with typed ExecutionRequests struct; renames constants with full suffix; removes ExecutionPayloadHeader and header conversion helpers; adds to_encoded_requests() and tests |
| crates/common/types/genesis.rs | Adds SSZ encode/decode for ChainConfig via SszChainConfig mirror struct; optional fields represented as SSZ union enums; straightforward mechanical mapping |
| crates/guest-program/src/l1/input.rs | Replaces rkyv serialization of ExecutionWitness with SSZ in encode_eip8025/decode_eip8025; adds crypto parameter to decode_eip8025; error enums updated correctly |
| crates/guest-program/src/l1/mod.rs | Exports new execution_program_eip8025_bytes under eip-8025 feature flag |
| crates/guest-program/src/lib.rs | Re-exports execution_program_eip8025_bytes in the public execution module under the eip-8025 feature |
Sequence Diagram
sequenceDiagram
participant Caller
participant input.rs as encode/decode_eip8025
participant block_execution_witness.rs as ExecutionWitness
participant program.rs as execution_program_eip8025_bytes
participant eip8025_ssz.rs as NewPayloadRequest/ExecutionRequests
Caller->>input.rs: encode_eip8025(new_payload_request, execution_witness)
input.rs->>eip8025_ssz.rs: new_payload_request.to_ssz()
input.rs->>block_execution_witness.rs: execution_witness.to_ssz_bytes()
block_execution_witness.rs-->>input.rs: [ssz_len][ssz_bytes][witness_ssz_bytes]
input.rs-->>Caller: wire bytes
Caller->>program.rs: execution_program_eip8025_bytes(bytes, crypto)
program.rs->>input.rs: decode_eip8025(bytes, crypto)
input.rs->>eip8025_ssz.rs: NewPayloadRequest::from_ssz_bytes()
input.rs->>block_execution_witness.rs: ExecutionWitness::from_ssz_bytes()
input.rs-->>program.rs: (NewPayloadRequest, ExecutionWitness)
program.rs->>eip8025_ssz.rs: hash_tree_root()
program.rs->>program.rs: validate_eip8025_execution()
note over program.rs: .is_ok() swallows internal errors as valid:false
program.rs-->>Caller: ProgramOutput { request_root, valid }
Prompt To Fix All With AI
This is a comment left during a code review.
Path: crates/guest-program/src/l1/program.rs
Line: 101
Comment:
**Silent error swallowing via `.is_ok()`**
`validate_eip8025_execution(...).is_ok()` converts every error — including internal infrastructure failures (OOM, crypto errors, panics caught upstream) — into `valid: false`, making them indistinguishable from legitimately invalid blocks. The caller receives `Ok(ProgramOutput { valid: false })` where it should receive an `Err`. By contrast, `execution_program` correctly propagates errors with `?`. This means real failures in `execution_program_eip8025_bytes` are silently misreported as invalid-but-successfully-processed blocks.
If returning `Ok` with `valid: false` for invalid blocks is intentional (e.g., for a proving circuit that needs to prove invalidity), then at minimum the `Internal` error variant should still be propagated, and only domain-level block-validity errors should become `valid: false`.
```suggestion
let valid = validate_eip8025_execution(&new_payload_request, execution_witness, crypto)
.map(|_| true)
.map_err(|e| ExecutionError::Internal(format!("execution validation failed: {e}")))?;
```
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: 213-234
Comment:
**Unwrap panics on trie node data that may exceed SSZ bounds**
Several `.unwrap()` calls inside `add_node` will panic if the underlying trie node data exceeds the hard-coded SSZ list bounds. The bounds are:
- `SszBranchNode.value`: max 128 bytes — a trie branch value can in theory be longer
- `SszExtensionNode.prefix` / `SszLeafNode.partial`: max 64 bytes
- `SszLeafNode.value`: max 128 bytes
Because `ExecutionWitness` is decoded from an untrusted wire input (the EIP-8025 byte stream), a crafted witness with oversized node values would cause a panic instead of a controlled `Err`. The outer helper `to_ssz_bytes` already returns a `Result` — these `unwrap()` calls should be replaced with `?` after mapping through `ExecutionWitnessSszError::InvalidSszType`, consistent with how other fields in the same function are handled.
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/guest-program/src/l1/program.rs
Line: 86
Comment:
**Stale `[rkyv_bytes]` in doc comment**
The wire-format description still mentions `[rkyv_bytes]` even though the encoding was changed to SSZ in this PR. The corresponding `encode_eip8025` and `decode_eip8025` docs in `input.rs` were updated correctly.
```suggestion
/// The wire format is `[ssz_len: u32 LE][ssz_bytes][witness_ssz_bytes]`, matching
```
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: 98
Comment:
**Unused constant `MAX_BYTES_PER_WITNESS_NODE`**
This constant is defined but never referenced anywhere in the module. It appears it should be used as the element-size bound when building the flattened `state_nodes` list, but the current implementation stores `SszNode` structs rather than raw bytes, so the constant is orphaned. If it's reserved for a future change, a `#[allow(dead_code)]` annotation or a comment explaining its intent would clarify the purpose. Otherwise it can be removed.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(l1): add ssz encoding for witness i..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| })?; | ||
|
|
||
| let request_root = new_payload_request.hash_tree_root(&Sha2Hasher); | ||
| let valid = validate_eip8025_execution(&new_payload_request, execution_witness, crypto).is_ok(); |
There was a problem hiding this comment.
.is_ok()
validate_eip8025_execution(...).is_ok() converts every error — including internal infrastructure failures (OOM, crypto errors, panics caught upstream) — into valid: false, making them indistinguishable from legitimately invalid blocks. The caller receives Ok(ProgramOutput { valid: false }) where it should receive an Err. By contrast, execution_program correctly propagates errors with ?. This means real failures in execution_program_eip8025_bytes are silently misreported as invalid-but-successfully-processed blocks.
If returning Ok with valid: false for invalid blocks is intentional (e.g., for a proving circuit that needs to prove invalidity), then at minimum the Internal error variant should still be propagated, and only domain-level block-validity errors should become valid: false.
| let valid = validate_eip8025_execution(&new_payload_request, execution_witness, crypto).is_ok(); | |
| let valid = validate_eip8025_execution(&new_payload_request, execution_witness, crypto) | |
| .map(|_| true) | |
| .map_err(|e| ExecutionError::Internal(format!("execution validation failed: {e}")))?; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/guest-program/src/l1/program.rs
Line: 101
Comment:
**Silent error swallowing via `.is_ok()`**
`validate_eip8025_execution(...).is_ok()` converts every error — including internal infrastructure failures (OOM, crypto errors, panics caught upstream) — into `valid: false`, making them indistinguishable from legitimately invalid blocks. The caller receives `Ok(ProgramOutput { valid: false })` where it should receive an `Err`. By contrast, `execution_program` correctly propagates errors with `?`. This means real failures in `execution_program_eip8025_bytes` are silently misreported as invalid-but-successfully-processed blocks.
If returning `Ok` with `valid: false` for invalid blocks is intentional (e.g., for a proving circuit that needs to prove invalidity), then at minimum the `Internal` error variant should still be propagated, and only domain-level block-validity errors should become `valid: false`.
```suggestion
let valid = validate_eip8025_execution(&new_payload_request, execution_witness, crypto)
.map(|_| true)
.map_err(|e| ExecutionError::Internal(format!("execution validation failed: {e}")))?;
```
How can I resolve this? If you propose a fix, please make it concise.| choices: SszList::try_from(choices).unwrap(), | ||
| value: SszList::try_from(branch.value.clone()).unwrap(), | ||
| }) | ||
| } | ||
| Node::Extension(ext) => { | ||
| let ssz_child = match &ext.child { | ||
| NodeRef::Node(n, _) => add_node(n, state_nodes)?, | ||
| NodeRef::Hash(hash) => SszNodeRef::Hash( | ||
| SszList::try_from(hash.as_ref().to_vec()).map_err(|e| { | ||
| ExecutionWitnessSszError::InvalidSszType(e.to_string()) | ||
| })?, | ||
| ), | ||
| }; | ||
| SszNode::Extension(SszExtensionNode { | ||
| prefix: SszList::try_from(ext.prefix.encode_compact()).unwrap(), | ||
| child: ssz_child, | ||
| }) | ||
| } | ||
| Node::Leaf(leaf) => SszNode::Leaf(SszLeafNode { | ||
| partial: SszList::try_from(leaf.partial.encode_compact()).unwrap(), | ||
| value: SszList::try_from(leaf.value.clone()).unwrap(), | ||
| }), |
There was a problem hiding this comment.
Several .unwrap() calls inside add_node will panic if the underlying trie node data exceeds the hard-coded SSZ list bounds. The bounds are:
SszBranchNode.value: max 128 bytes — a trie branch value can in theory be longerSszExtensionNode.prefix/SszLeafNode.partial: max 64 bytesSszLeafNode.value: max 128 bytes
Because ExecutionWitness is decoded from an untrusted wire input (the EIP-8025 byte stream), a crafted witness with oversized node values would cause a panic instead of a controlled Err. The outer helper to_ssz_bytes already returns a Result — these unwrap() calls should be replaced with ? after mapping through ExecutionWitnessSszError::InvalidSszType, consistent with how other fields in the same function are handled.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/common/types/block_execution_witness.rs
Line: 213-234
Comment:
**Unwrap panics on trie node data that may exceed SSZ bounds**
Several `.unwrap()` calls inside `add_node` will panic if the underlying trie node data exceeds the hard-coded SSZ list bounds. The bounds are:
- `SszBranchNode.value`: max 128 bytes — a trie branch value can in theory be longer
- `SszExtensionNode.prefix` / `SszLeafNode.partial`: max 64 bytes
- `SszLeafNode.value`: max 128 bytes
Because `ExecutionWitness` is decoded from an untrusted wire input (the EIP-8025 byte stream), a crafted witness with oversized node values would cause a panic instead of a controlled `Err`. The outer helper `to_ssz_bytes` already returns a `Result` — these `unwrap()` calls should be replaced with `?` after mapping through `ExecutionWitnessSszError::InvalidSszType`, consistent with how other fields in the same function are handled.
How can I resolve this? If you propose a fix, please make it concise.| /// Decode and execute the L1 stateless validation program from EIP-8025 wire | ||
| /// bytes. | ||
| /// | ||
| /// The wire format is `[ssz_len: u32 LE][ssz_bytes][rkyv_bytes]`, matching |
There was a problem hiding this comment.
[rkyv_bytes] in doc comment
The wire-format description still mentions [rkyv_bytes] even though the encoding was changed to SSZ in this PR. The corresponding encode_eip8025 and decode_eip8025 docs in input.rs were updated correctly.
| /// The wire format is `[ssz_len: u32 LE][ssz_bytes][rkyv_bytes]`, matching | |
| /// The wire format is `[ssz_len: u32 LE][ssz_bytes][witness_ssz_bytes]`, matching |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/guest-program/src/l1/program.rs
Line: 86
Comment:
**Stale `[rkyv_bytes]` in doc comment**
The wire-format description still mentions `[rkyv_bytes]` even though the encoding was changed to SSZ in this PR. The corresponding `encode_eip8025` and `decode_eip8025` docs in `input.rs` were updated correctly.
```suggestion
/// The wire format is `[ssz_len: u32 LE][ssz_bytes][witness_ssz_bytes]`, matching
```
How can I resolve this? If you propose a fix, please make it concise.| const MAX_WITNESS_NODES: usize = 1 << 20; | ||
| const MAX_WITNESS_CODES: usize = 1 << 16; | ||
| const MAX_WITNESS_HEADERS: usize = 256; | ||
| const MAX_BYTES_PER_WITNESS_NODE: usize = 1 << 20; |
There was a problem hiding this comment.
MAX_BYTES_PER_WITNESS_NODE
This constant is defined but never referenced anywhere in the module. It appears it should be used as the element-size bound when building the flattened state_nodes list, but the current implementation stores SszNode structs rather than raw bytes, so the constant is orphaned. If it's reserved for a future change, a #[allow(dead_code)] annotation or a comment explaining its intent would clarify the purpose. Otherwise it can be removed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/common/types/block_execution_witness.rs
Line: 98
Comment:
**Unused constant `MAX_BYTES_PER_WITNESS_NODE`**
This constant is defined but never referenced anywhere in the module. It appears it should be used as the element-size bound when building the flattened `state_nodes` list, but the current implementation stores `SszNode` structs rather than raw bytes, so the constant is orphaned. If it's reserved for a future change, a `#[allow(dead_code)]` annotation or a comment explaining its intent would clarify the purpose. Otherwise it can be removed.
How can I resolve this? If you propose a fix, please make it concise.
| })?; | ||
|
|
||
| let request_root = new_payload_request.hash_tree_root(&Sha2Hasher); | ||
| let valid = validate_eip8025_execution(&new_payload_request, execution_witness, crypto).is_ok(); |
There was a problem hiding this comment.
P1: this collapses every error path — payload conversion, blob hash mismatch, witness conversion bugs, EVM internal errors, OOM-like infra failures — into a normal valid: false outcome. Wrong shape for a stateless verifier: it makes infra bugs look like consensus-relevant rejections. The earlier execution_program propagates Err(...) for those cases, which is correct. Either match it (only valid: false for the categories EIP-8025 §Tolerance permits) or document explicitly which errors map to valid: false and assert with a typed enum split. (Greptile flagged.)
| choices.push(ssz_child); | ||
| } | ||
| SszNode::Branch(SszBranchNode { | ||
| choices: SszList::try_from(choices).unwrap(), |
There was a problem hiding this comment.
P1: SszList::try_from(...).unwrap() here (and lines 141, 154, 159, 160, 178, 193, 194) panics on overflow. Bounds:
SszBranchNode.value: 128— branch values for state trie can hit ~110 bytes (RLP-encodedAccountState), so 128 is on the edge.SszLeafNode.value: 128— same.SszExtensionNode.prefix: 64/SszLeafNode.partial: 64—encode_compact()outputs of nibble paths up to 64 nibbles, worst case 33 bytes. Comfortable but un-asserted.
Replace each unwrap() with ?-propagating try_from and let the existing ExecutionWitnessSszError::InvalidSszType carry the error up. Encoder side, where input is locally trusted, but still a robustness-vs-spec-evolution bug. (Greptile flagged.)
| } | ||
| } | ||
|
|
||
| fn rebuild_node_by_index( |
There was a problem hiding this comment.
Decoder DoS via cyclic references: this recurses into SszNodeRef::Index(i) without checking that i < current_index (or any forward-only invariant). The encoder is post-order so indices are forward-only by construction, but a malicious witness can craft nodes[0] referencing nodes[1] referencing nodes[0] → infinite recursion → stack overflow. For a verifier accepting bytes from untrusted sources (CL relaying execution proofs), that's a DoS vector. Add an explicit i as usize > current_node_index check, or a visited-set, or convert to iterative reconstruction with topological-order validation.
| const MAX_WITNESS_CODES: usize = 1 << 16; | ||
| const MAX_WITNESS_HEADERS: usize = 256; | ||
| const MAX_BYTES_PER_WITNESS_NODE: usize = 1 << 20; | ||
| const MAX_BYTES_PER_CODE: usize = 1 << 24; |
There was a problem hiding this comment.
MAX_BYTES_PER_CODE = 1<<24 (16 MB) is much larger than EIP-170's 24576-byte cap and EIP-3860's 49152 init-code cap. Tighter bound = tighter spec contract = fewer witness shapes a verifier must accept. Tighten to 49_152 (covers init code) or document why the loose bound is intentional.
| const MAX_BYTES_PER_HEADER: usize = 1 << 10; | ||
|
|
||
| // TODO: think about these constants | ||
| const MAX_CHAIN_CONFIG_BYTES: usize = 1 << 10; |
There was a problem hiding this comment.
TODO: think about these constants — worth resolving in this PR rather than carrying it forward. 1024 is fine for the current 30-ish-field config, but the TODO invites bit-rot.
| const MAX_WITNESS_NODES: usize = 1 << 20; | ||
| const MAX_WITNESS_CODES: usize = 1 << 16; | ||
| const MAX_WITNESS_HEADERS: usize = 256; | ||
| const MAX_BYTES_PER_WITNESS_NODE: usize = 1 << 20; |
There was a problem hiding this comment.
(Greptile P2) MAX_BYTES_PER_WITNESS_NODE declared but never referenced. Current encoding stores SszNode structs directly in SszList<SszNode, MAX_WITNESS_NODES>, so per-node byte bounds aren't enforceable at the SSZ-list level. Either remove the constant, or restructure to encode each node as SszList<u8, MAX_BYTES_PER_WITNESS_NODE> and parse on decode (latter buys a uniform bound but adds an indirection).
| /// Decode and execute the L1 stateless validation program from EIP-8025 wire | ||
| /// bytes. | ||
| /// | ||
| /// The wire format is `[ssz_len: u32 LE][ssz_bytes][rkyv_bytes]`, matching |
There was a problem hiding this comment.
(Greptile P2) Stale doc comment: still says [rkyv_bytes] even though the encoding flipped to SSZ in this PR. Wire format is now SSZ end-to-end.
|
|
||
| pub fn from_ssz_bytes( | ||
| bytes: &[u8], | ||
| _crypto: &dyn Crypto, |
There was a problem hiding this comment.
_crypto: &dyn Crypto is unused. Either wire it through future signature recovery / hash verification or drop the parameter — keeping unused parameters in a feature-gated path tends to bit-rot.
…6549) **Motivation** Earlier work lambdaclass#6365 added `&dyn Crypto`, so zkVM can route hashing to precompiles. However, it is not applied everywhere in the eip-8025 execution path. **Description** The [execution_program](https://github.com/lambdaclass/ethrex/blob/main/crates/guest-program/src/l1/program.rs#L73) is calling `hash_tree_root` with `libssz_merkle::Sha2Hasher`. This fix implements `libssz_merkle::Sha256Hasher` trait for `&dyn Crypto` such that hash precompiles can be used in computing the hash of the tree root. This fix **significantly reduces the AIR-costs** inside ZisK v0.16.1, and it is the main reason for the previously observed inefficiency in lambdaclass#6524. Profiling on mainnet blocks: 24949787 - 24949836: - Average cost (main `1e85b1c36`): 57.6B - Average cost (this change): 45.2B --------- Co-authored-by: ElFantasma <estebandh@gmail.com> Co-authored-by: Ivan Litteri <67517699+ilitteri@users.noreply.github.com>
Update: Let's wait until the changes are finalized in #6550
Motivation
Replace
rkyvencoding ofExecutionWitnesswith SSZ. The structure ofExecutionWitnessmight change further, so this PR is a good middle step towards making EIP-8025 more spec-compliant.Related to issue #6502, but this PR does not yet make decoding faster.
Description
Changes add SSZ encoding by using
libsszSszExecutionWitnessas the SSZ representation forExecutionWitness.ExecutionWitness.ChainConfigCan be (almost) directly plugged into ere-guests PR39