Define recursive EIP-8025 execution proof guest - #5534
Conversation
| from the arguments. A typed implementation invokes | ||
| `verify_stateless_new_payload` directly. A serialized zkVM implementation MAY | ||
| invoke `run_stateless_guest`, decodes its output, and MUST check that the | ||
| returned `new_payload_request_root` equals `compute_new_payload_request_root` | ||
| for the constructed input before exposing the result. In both cases the logical |
There was a problem hiding this comment.
Other than the mismatched tense here, something about this sentence is a bit awkward to read. I would suggest splitting it up into two+ sentences as it's pretty long.
| from the arguments. A typed implementation invokes | |
| `verify_stateless_new_payload` directly. A serialized zkVM implementation MAY | |
| invoke `run_stateless_guest`, decodes its output, and MUST check that the | |
| returned `new_payload_request_root` equals `compute_new_payload_request_root` | |
| for the constructed input before exposing the result. In both cases the logical | |
| from the arguments. A typed implementation invokes | |
| `verify_stateless_new_payload` directly. A serialized zkVM implementation MAY | |
| invoke `run_stateless_guest`, decode its output, and MUST check that the | |
| returned `new_payload_request_root` equals `compute_new_payload_request_root` | |
| for the constructed input before exposing the result. In both cases the logical |
There was a problem hiding this comment.
Yep, agreed, I expect much of the prose will change in this PR. I'll mark the PR as ready for review when I have things in a better state.
There was a problem hiding this comment.
Yep, just did a quick skim & wanted to mention this 😅 I'll do an in-depth review when its marked as ready.
There was a problem hiding this comment.
That's appreciated. I just don't want you wasting your time reviewing trash 🤣
| - _[IGNORE]_ The proof has not already been processed -- i.e. | ||
| `hash_tree_root(proof)` has not been seen before. | ||
| - _[IGNORE]_ The proof's corresponding new payload request (identified by | ||
| `proof.public_input.new_payload_request_root`) has been seen (via gossip or | ||
| non-gossip sources) (a client MAY queue proofs for processing once the new | ||
| payload request is retrieved). | ||
| - _[IGNORE]_ The beacon block identified by | ||
| `proof.public_input.head.beacon_block_root` and its accepted | ||
| `SignedExecutionPayloadEnvelope` have been seen (via gossip or non-gossip | ||
| sources). A client MAY queue the proof until both are available. |
There was a problem hiding this comment.
Since Gloas gossip validation specs are now executable functions, it might be worth converting these to executable functions now too. Not sure if you'd want to handle this here or in a separate PR. Since it's just a single new gossip topic, I think it would be alright to do it in this PR. And no need to worry about tests.
There was a problem hiding this comment.
Very nice addition to the specs! Yep, let's get it done in this PR
…nsensus-specs-pr-5534-recursive-execution-proof # Conflicts: # pysetup/spec_builders/eip8025.py # tests/core/pyspec/eth_consensus_specs/test/helpers/gossip.py
This PR simplifies the EIP-8025 consensus specifications by defining a more constrained baseline covering execution-proof types, gossip, verification, storage, and optional prover behaviour. Most notably, we remove the network req/resp protocol and introduce an `ExecutionProofEnvelope` type that uses the `beacon_block_root` to associate an execution proof with a block. Furthermore, we migrate to the updated gossip validation defintion pattern for execution proofs and introduce proof verification into the gossip validation pipeline to address concerns raised in #5502. This PR assumes that execution proofs are recursive - i.e. a proof for block n also attests to the validity of the chain prefix [weak-subjectivity..n) - the recursive guest design is drafted in #5534.
jihoonsong
left a comment
There was a problem hiding this comment.
I skimmed the p2p change. I can review other parts later when I have fresh brain.
Additionally, following this issue, I think we would need to detach tests from this PR to get it merged. We'd still need to find out what would be the best way going forward, but the tests could live in a forked repo, for instance.
|
|
||
| # [REJECT] The proof data is non-empty and within the size limit | ||
| if len(proof.proof_data) == 0: | ||
| raise GossipReject("execution proof data is empty") |
There was a problem hiding this comment.
Can we have each of these have comment? See this as a reference.
| raise GossipIgnore("verified proof already known for this head and proof type") | ||
|
|
||
| # [IGNORE] This is the prover's first valid or invalid proof for this key | ||
| prover_key = (head_root, proof.proof_type, validator_index) |
There was a problem hiding this comment.
Can we put Seen check at the beginning?
| raise GossipReject("execution proof's head block failed validation") | ||
|
|
||
| # [IGNORE] No verified proof is known for this head and proof type | ||
| if proof.proof_type in store.execution_proofs.get(head_root, {}): |
There was a problem hiding this comment.
I left this in another PR but leaving it also here to put it with other comments; execution_proofs could be DefaultDict.
Summary
originand advancingheadcheckpointseproofpeering discoveryDesign notes
Validation
make lintmake test fork=eip8025(1366 passed, 6432 skipped)