feat(l1): fix EIP-8025 compliance#6516
Conversation
|
|
||
| // ── Spec limits (Electra) ────────────────────────────────────────── | ||
|
|
||
| /// MAX_TRANSACTIONS_PER_PAYLOAD (Electra). |
There was a problem hiding this comment.
Some renamings to conform to the consensus-spec names, but also some fixes that I'll comment in particular.
| /// MAX_CONSOLIDATION_REQUESTS_PER_PAYLOAD (Electra). | ||
| const MAX_CONSOLIDATION_REQUESTS: usize = 1; |
There was a problem hiding this comment.
Note that the previous 1 value wasn't correct, this was fixed below L30 with value 2 as defined in the specs.
| pub deposit_requests: SszList<DepositRequest, MAX_DEPOSIT_REQUESTS>, | ||
| pub withdrawal_requests: SszList<WithdrawalRequest, MAX_WITHDRAWAL_REQUESTS>, | ||
| pub consolidation_requests: SszList<ConsolidationRequest, MAX_CONSOLIDATION_REQUESTS>, |
There was a problem hiding this comment.
In the specs, ExecutionPayload does not have these three fields. They are actually part of the NewPayloadRequest.
| /// SSZ `ExecutionRequests` container (Electra) — the typed EIP-7685 bundle | ||
| /// that the CL commits to alongside `ExecutionPayload`. | ||
| #[derive(Debug, Clone, PartialEq, Eq, SszEncode, SszDecode, HashTreeRoot)] | ||
| pub struct ExecutionPayloadHeader { |
There was a problem hiding this comment.
This isn't strictly required so we can remove.
| pub deposit_requests_root: [u8; 32], | ||
| pub withdrawal_requests_root: [u8; 32], | ||
| pub consolidation_requests_root: [u8; 32], | ||
| pub struct ExecutionRequests { |
There was a problem hiding this comment.
See my next comment to understand this.
| pub execution_requests: | ||
| SszList<SszList<u8, MAX_EXECUTION_REQUEST_BYTES>, MAX_EXECUTION_REQUESTS>, | ||
| pub execution_requests: ExecutionRequests, |
There was a problem hiding this comment.
The consensus-specs don't define execution_requests as a list of bytes, but an ExecutionRequests deserialized struct.
Fixing it here.
| /// 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 | ||
| /// [`decode_eip8025`](super::decode_eip8025). | ||
| #[cfg(feature = "eip-8025")] | ||
| pub fn execution_program_eip8025_bytes( | ||
| bytes: &[u8], | ||
| crypto: Arc<dyn Crypto>, | ||
| ) -> Result<ProgramOutput, ExecutionError> { |
There was a problem hiding this comment.
This is a new entrypoint which is aligned with the execution-specs.
In ere-guests now we use it to send raw bytes directly, removing a lot of "wrapper logic" we had in ere-guest guest program before calling Ethrex entry point.
jsign
changed the title
There was a problem hiding this comment.
Pull request overview
This PR completes the EIP-8025 guest-program execution path and aligns the SSZ NewPayloadRequest model with the Electra/CL spec, including deriving the EL requests_hash from a typed ExecutionRequests container.
Changes:
- Added a raw-bytes EIP-8025 execution entrypoint (
[ssz_len][ssz_bytes][rkyv_bytes]) and re-exported it for external consumers. - Refactored EIP-8025 validation/execution to share common logic for
NewPayloadRequest -> Blockconversion, hash checks, and stateless execution. - Updated SSZ types to match Electra and introduced
ExecutionRequests::to_encoded_requests()for EIP-7685requests_hashcomputation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/guest-program/src/lib.rs | Re-exports the new EIP-8025 bytes entrypoint under the L1 execution module. |
| crates/guest-program/src/l1/program.rs | Adds the bytes entrypoint, centralizes EIP-8025 validation/execution, and adjusts request-hash reconstruction. |
| crates/guest-program/src/l1/mod.rs | Re-exports execution_program_eip8025_bytes behind the eip-8025 feature. |
| crates/common/types/eip8025_ssz.rs | Aligns SSZ containers with Electra and adds typed ExecutionRequests plus EIP-7685 encoding conversion. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| impl ExecutionRequests { | ||
| /// Produce the EIP-7685 encoded form: three `EncodedRequests` entries, | ||
| /// one per request type, each `[type_byte] ++ concat(ssz_encode(item))`. | ||
| /// | ||
| /// The three request types are all fixed-size SSZ containers, so their | ||
| /// SSZ encoding is byte-for-byte the EL wire concatenation that | ||
| /// `compute_requests_hash` expects. | ||
| pub fn to_encoded_requests(&self) -> Vec<EncodedRequests> { | ||
| fn encode<T: SszEncode>( | ||
| type_byte: u8, | ||
| items: impl IntoIterator<Item = T>, | ||
| ) -> EncodedRequests { | ||
| let mut buf = Vec::new(); | ||
| buf.push(type_byte); | ||
| for item in items { | ||
| item.ssz_append(&mut buf); | ||
| } | ||
| EncodedRequests(Bytes::from(buf)) | ||
| } | ||
|
|
||
| vec![ | ||
| encode(DEPOSIT_REQUEST_TYPE, self.deposits.iter().cloned()), | ||
| encode(WITHDRAWAL_REQUEST_TYPE, self.withdrawals.iter().cloned()), | ||
| encode( | ||
| CONSOLIDATION_REQUEST_TYPE, | ||
| self.consolidations.iter().cloned(), | ||
| ), | ||
| ] | ||
| } |
There was a problem hiding this comment.
ExecutionRequests::to_encoded_requests() currently always returns 3 EncodedRequests entries, even when a request list is empty (yielding a 1-byte [type] entry). Elsewhere in the repo the EIP-7685 rules are enforced such that entries with empty request_data must be excluded (see validate_execution_requests, which rejects len() < 2). Consider emitting only non-empty request types (and/or filtering out EncodedRequests where is_empty() is true) so the helper is spec-compliant if reused beyond compute_requests_hash.
| let (new_payload_request, execution_witness) = super::decode_eip8025(bytes).map_err(|err| { | ||
| ExecutionError::Internal(format!("failed to decode EIP-8025 input: {err}")) | ||
| })?; | ||
|
|
||
| let request_root = new_payload_request.hash_tree_root(&Sha2Hasher); | ||
| let valid = validate_eip8025_execution(&new_payload_request, execution_witness, crypto).is_ok(); | ||
|
|
||
| Ok(ProgramOutput { | ||
| new_payload_request_root: request_root, | ||
| valid: true, | ||
| valid, | ||
| }) |
There was a problem hiding this comment.
The byte entrypoint sets valid based on validate_eip8025_execution(...).is_ok() but there’s no test covering the post-decode failure path (i.e., decoding succeeds but validation fails and the function must still return Ok(ProgramOutput { valid: false, ... })). Adding a test that builds a minimally valid wire payload with an intentionally-invalid block_hash (or versioned-hash mismatch) would lock in the intended behavior and prevent regressions back to returning an error.
Greptile SummaryThis PR aligns the EIP-8025 SSZ types with the Electra consensus spec by moving execution requests out of Confidence Score: 5/5Safe to merge; all remaining findings are style/diagnostic P2 items with no correctness impact. The SSZ type changes correctly mirror the Electra spec. compute_requests_hash's > 1 guard correctly handles the always-3-entries output of to_encoded_requests(). Request sizes verified in tests match EIP-6110/7002/7251 wire formats. MAX_CONSOLIDATION_REQUESTS_PER_PAYLOAD=2 is the correct Electra value. The only remaining findings are a stale comment and the intentional silent error conversion in the bytes entrypoint. crates/guest-program/src/l1/program.rs — the is_ok() error swallowing in execution_program_eip8025_bytes may complicate future debugging.
|
| Filename | Overview |
|---|---|
| crates/common/types/eip8025_ssz.rs | Removes deposit/withdrawal/consolidation from ExecutionPayload and moves them into a typed ExecutionRequests container; adds to_encoded_requests() that always emits 3 entries (handled correctly downstream by compute_requests_hash's > 1 guard); corrects MAX_CONSOLIDATION_REQUESTS_PER_PAYLOAD to 2 (Electra value); renames spec constants for clarity; removes now-unused ExecutionPayloadHeader and NewPayloadRequestHeader. |
| crates/guest-program/src/l1/program.rs | Extracts shared validation logic into validate_eip8025_execution; adds execution_program_eip8025_bytes for the raw wire-format entrypoint; the bytes entrypoint silently converts all validation/execution errors to valid:false (P2: internal errors become indistinguishable from protocol invalidity). |
| crates/guest-program/src/l1/mod.rs | Re-exports the new execution_program_eip8025_bytes function, gated correctly behind #[cfg(feature = "eip-8025")]. |
| crates/guest-program/src/lib.rs | Adds a conditional re-export of execution_program_eip8025_bytes in the public execution module, consistently double-gated on not(l2) and eip-8025. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Wire bytes\nssz_len ++ ssz_bytes ++ rkyv_bytes] --> B[decode_eip8025]
B --> C[NewPayloadRequest\nExecutionRequests\nExecutionWitness]
C --> D[hash_tree_root\n→ request_root]
C --> E[validate_eip8025_execution]
E --> F[new_payload_request_to_block]
F --> G[execution_requests\n.to_encoded_requests]
G --> H[compute_requests_hash\nskips len==1 entries]
H --> I[Block]
I --> J{block_hash match?}
J -- No --> K[Err block_hash mismatch]
J -- Yes --> L[validate_versioned_hashes]
L --> M{hashes match?}
M -- No --> N[Err versioned hashes mismatch]
M -- Yes --> O[execute_blocks]
O --> P{ok?}
P -- No / execution_program --> Q[Err propagated]
P -- No / eip8025_bytes --> R[ProgramOutput\nvalid=false]
P -- Yes --> S[ProgramOutput\nvalid=true]
Prompt To Fix All With AI
This is a comment left during a code review.
Path: crates/guest-program/src/l1/program.rs
Line: 100
Comment:
**Silent error swallowing on validation failure**
`validate_eip8025_execution(…).is_ok()` converts all errors — including internal panics turned into `Err`, OOM, or logic bugs — to `valid: false`. A caller of `execution_program_eip8025_bytes` can never distinguish "this payload was legitimately invalid per the protocol" from "an unexpected internal error occurred during execution." At minimum, consider logging or propagating the error variant so that operator-level diagnostics are possible when this runs outside a strict zkVM context.
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: 72
Comment:
**Misleading "consuming" comment**
The comment says "before consuming the payload", but `hash_tree_root` takes `&self` — it borrows, not consumes, `new_payload_request`. The comment was meaningful when the old code moved the value into `new_payload_request_to_block`; it is now misleading.
```suggestion
// Compute the hash_tree_root of the payload before validation.
let request_root = new_payload_request.hash_tree_root(&Sha2Hasher);
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "refactor(l1): streamline EIP-8025 execut..." | 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.
validate_eip8025_execution(…).is_ok() converts all errors — including internal panics turned into Err, OOM, or logic bugs — to valid: false. A caller of execution_program_eip8025_bytes can never distinguish "this payload was legitimately invalid per the protocol" from "an unexpected internal error occurred during execution." At minimum, consider logging or propagating the error variant so that operator-level diagnostics are possible when this runs outside a strict zkVM context.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/guest-program/src/l1/program.rs
Line: 100
Comment:
**Silent error swallowing on validation failure**
`validate_eip8025_execution(…).is_ok()` converts all errors — including internal panics turned into `Err`, OOM, or logic bugs — to `valid: false`. A caller of `execution_program_eip8025_bytes` can never distinguish "this payload was legitimately invalid per the protocol" from "an unexpected internal error occurred during execution." At minimum, consider logging or propagating the error variant so that operator-level diagnostics are possible when this runs outside a strict zkVM context.
How can I resolve this? If you propose a fix, please make it concise.| @@ -72,36 +72,36 @@ pub fn execution_program( | |||
| // Compute the hash_tree_root before consuming the payload. | |||
There was a problem hiding this comment.
The comment says "before consuming the payload", but hash_tree_root takes &self — it borrows, not consumes, new_payload_request. The comment was meaningful when the old code moved the value into new_payload_request_to_block; it is now misleading.
| // Compute the hash_tree_root before consuming the payload. | |
| // Compute the hash_tree_root of the payload before validation. | |
| let request_root = new_payload_request.hash_tree_root(&Sha2Hasher); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/guest-program/src/l1/program.rs
Line: 72
Comment:
**Misleading "consuming" comment**
The comment says "before consuming the payload", but `hash_tree_root` takes `&self` — it borrows, not consumes, `new_payload_request`. The comment was meaningful when the old code moved the value into `new_payload_request_to_block`; it is now misleading.
```suggestion
// Compute the hash_tree_root of the payload before validation.
let request_root = new_payload_request.hash_tree_root(&Sha2Hasher);
```
How can I resolve this? If you propose a fix, please make it concise.| /// The wire format is `[ssz_len: u32 LE][ssz_bytes][rkyv_bytes]`, matching | ||
| /// [`decode_eip8025`](super::decode_eip8025). | ||
| #[cfg(feature = "eip-8025")] | ||
| pub fn execution_program_eip8025_bytes( |
There was a problem hiding this comment.
Thanks for the PR! Looks good to me.
One thing: since execution_program is already under the eip-8025 feature flag, I'd keep only the bytes version and drop the typed one. I think there's no reason to maintain the typed entrypoint.
WDYT @ilitteri?
There was a problem hiding this comment.
Yep, I was thinking about the same when making the PR but prefered to be conservative in removing existing APIs.
Leaving that decision to you guys!
There was a problem hiding this comment.
I agree, let's keep the bytes version only.
There was a problem hiding this comment.
Sounds good. I'll adjust and reping here when ready.
There was a problem hiding this comment.
@avilagaston9 @ilitteri, all done see 0058d27
…nical execution_program Signed-off-by: jsign <jsign.uy@gmail.com>
|
@ilitteri, is this mergeable? :) |
We need one more approval. I'd get it asap. |
Brings in main commits since the prior merge: #6516 EIP-8025 compliance (Electra-aligned ExecutionRequests typed container in NewPayloadRequest, MAX_CONSOLIDATION_REQUESTS_PER_PAYLOAD corrected from 1 to 2, to_encoded_requests() helper for EIP-7685 bytes, removal of ExecutionPayloadHeader/NewPayloadRequestHeader, new byte-oriented execution_program entrypoint that decodes the wire format internally and returns valid: false instead of erroring on post-decode failures), #6463 BAL withdrawal reverse check (DB->BAL direction so a malicious builder can't omit a withdrawal recipient from the BAL), #6505 Kademlia k-bucket revert (PeerTableServer::spawn no longer takes a node_id), plus snap-sync observability + dashboards (#6470), pivot-update crash fix (#6475), weighted peer selection (#6428), txpool_contentFrom/txpool_inspect RPC (#6446), block-by-block exec fallback (#6464), Amsterdam EELS branch pin (#6495), and rollup store SQLite v9->v10 migration (#6514). Conflict resolutions: - crates/common/types/stateless_ssz.rs: this branch had already moved the EIP-8025 SSZ types out of crates/common/types/eip8025_ssz.rs into stateless_ssz.rs and tucked the native-rollup containers below them. Kept that layout, applied #6516's content updates to the EIP-8025 section (renamed spec-limit constants, ExecutionRequests typed container with to_encoded_requests, dropped header types and their tests), pulled in the EncodedRequests import, and kept both the new test_execution_requests_to_encoded_bytes and the branch's stateless round-trip tests. - crates/guest-program/src/l1/program.rs: adopted #6516's new execution_program(bytes: &[u8], crypto) API with the internal decode_eip8025 call, the validate_eip8025_execution helper, and the decode-failure test. Rewrote all `eip-8025` feature gates as `experimental-devnet` and all `eip8025_ssz::` paths as `stateless_ssz::` to match this branch's renames. - crates/guest-program/bin/{sp1,risc0,zisk,openvm}/src/main.rs: applied #6516's simplification (drop decode_eip8025 import, pass &input straight to execution_program) under the experimental-devnet feature gate. Also flipped the rkyv::rancor::Error import gate from the old `eip-8025` name to `experimental-devnet` so the non-devnet build still has the import it needs. - crates/prover/src/backend/exec.rs: kept #6516's updated comment ("raw input bytes" instead of "(NewPayloadRequest, ExecutionWitness)") under the experimental-devnet feature gate. Auto-merged regions checked: crates/vm/backends/levm/mod.rs picked up all of #6463's Part B (DB->BAL) reverse check intact, and cmd/ethrex/l2/initializers.rs picked up #6505's PeerTableServer::spawn signature change. Verified cargo fmt --all clean, cargo check --workspace clean, cargo check --workspace --tests clean, and cargo check -p ethrex-guest-program --features experimental-devnet --tests clean.
crates/guest-program/src/l1/program.rs::execution_program ("Decode and
execute the L1 stateless validation program from EIP-8025 wire bytes.")
— the merge resolution kept only the wire-format paragraph and dropped
that opening sentence by accident, so the function lost the high-level
"what does this do" line and the doc started straight into a format
description.
Also trim the "Stateless validation limits" preamble in
crates/common/types/stateless_ssz.rs down to just the section divider.
The block underneath was editorializing about how stateless.py uses
untyped Python lists and how only MAX_WITNESS_HEADERS has spec backing
— useful context the day the constants were chosen, but it doesn't
help anyone reading the file now and the per-constant docstrings below
are sufficient on their own.
….py at the actual NativeRollup.sol/MPTProof.sol location so the Blockscout verification script can find its sources to flatten. The script lives at tooling/l2/dev/, so a single ".." resolves to tooling/l2/, and the previous "../crates/vm/levm/contracts" path was doubly wrong: it had only one parent step (giving tooling/l2/crates/vm/levm/contracts, which doesn't exist) and pointed at the pre-PR-#6516 location for the L1 native-rollup contracts. After the contracts were moved to crates/l2/contracts/src/nativeRollup/l1/ during the experimental-devnet unification, the script raised FileNotFoundError on flatten_source(), so Step 6 of the native-rollup deployment demo (verifying the deployed NativeRollup against a local Blockscout) couldn't run. Update the path to "../../../crates/l2/contracts/src/nativeRollup/l1" so Step 6 succeeds.
Summary
This branch fixes the EIP-8025 guest-program execution flow and aligns the SSZ-side
NewPayloadRequestmodel with the Electra/CL spec.At a high level, it adds a byte-oriented EIP-8025 execution entrypoint, moves execution requests into a typed SSZ container, and uses that typed representation to reconstruct the EL
requests_hashcorrectly when converting aNewPayloadRequestinto a block.What changed
execution_program_eip8025_bytes, which decodes and executes the EIP-8025 wire format directly:[ssz_len: u32 LE][ssz_bytes][rkyv_bytes]NewPayloadRequest -> Blockconversionexecute_blocksethrex-commonto better match the Electra consensus spec:ExecutionPayloadno longer carries deposit/withdrawal/consolidation request listsNewPayloadRequest.execution_requestsis now a typedExecutionRequestscontainerExecutionRequests::to_encoded_requests()to convert the SSZ typed request container into the EIP-7685 encoded request list expected bycompute_requests_hashethrex-guest-programBehavior notes
new_payload_request_rootProgramOutput { valid: false, ... }instead of failing outrightWhy this matters
This makes the EIP-8025 guest path closer to the consensus-side structure we actually need to prove against, while also giving us a direct raw-bytes execution entrypoint for the EIP-8025 wire format.
It also removes the mismatch between the SSZ request representation and the EL request-hash computation path by deriving
requests_hashfrom a properly typedExecutionRequestscontainer.External usage
I'm already using this PR branch into an ere-guests PR here: eth-act/ere-guests#39
This allowed
ere-gueststo basically remove a lot of wrapper code before calling the guest program regarding execution-payload->Block, output calculation, etc -- making now Ethrex own all that logic as expected.