Refactor EIP-8025 specs#4828
Conversation
|
Ah, also a minor request, please run |
| Provers subscribe to the `signed_execution_payload_envelope` gossip topic | ||
| (defined in [Gloas](../../gloas/p2p-interface.md)) to receive execution payloads | ||
| for which they can generate execution proofs. |
There was a problem hiding this comment.
What was the rationale for registering provers into the network? Before there was no separation between a builder and a prover (ie builders would create proofs)
There was a problem hiding this comment.
Before I was using a Union[BuilderIndex, BLSPubkey] type to abstract the entity responsible for proof gen. By splitting this out and having a separate ProverSignedExecutionProof means that we can easily deprecate the type and prover role when we transition to enforced proofs using builders. I have placed the honest builder spec in the gloas branch here - https://github.com/frisitano/consensus-specs/blob/feat/eip-8025-gloas/specs/_features/eip8025_gloas/builder.md
There was a problem hiding this comment.
Makes sense -- I just checked and BuilderSignedExecutionProof has been removed since I last looked
There was a problem hiding this comment.
Yep I reverted BuilderSignedExecutionProof back to SignedExecutionProof. The SignedExecutionProof is the variant that contains the BuilderIndex (only valid in gloas).
class SignedExecutionProof(Container):
message: ExecutionProof
builder_index: BuilderIndex
signature: BLSSignatureOnce we migrate from optional proofs to mandatory proofs, we will deprecate the ProverSignedExecutionProof and keep SigneExecutionProof only.
|
|
||
| | Name | SSZ equivalent | Description | | ||
| | ------------ | -------------- | ---------------------------------------- | | ||
| | `ProofGenId` | `Bytes8` | Identifier for tracking proof generation | |
There was a problem hiding this comment.
I'm wondering if this is needed -- it seems that all uses of it go along the lines of:
proof_gen_id = request_proofs()
proofs = get_proofs(proof_gen_id)
Why not have a single method that requests the proofs and returns them?
There was a problem hiding this comment.
Ah, I had the same question when reading this. I think any benefit of this async-like design could be created in a real implementation by using threads, so the blocking nature of requests_proofs (if we remove get_proofs) doesn't sounds limiting.
There was a problem hiding this comment.
I'm still uncertain about how best to structure this. Maybe we should discuss the current state. Is it the case that the Proof Engine will submit the proofs back to the beacon node via http rpc once the proofs have been generated?
There was a problem hiding this comment.
yep exactly -- so all zkCL nodes are the same and when a proof becomes available, someone will send the proof to a particular zkCL who will then propagate it to other zkCLs
There was a problem hiding this comment.
There was an iteration where some zkCL nodes were also proof generating, but it introduced a few edge cases and I instead opted to have a homogeneous network of one zkCL node type
There was a problem hiding this comment.
In that case, it sounds like we should remove the get_proofs method and change the return type of request_proofs to be None. Is the CL node http rpc for submitting the proofs to the node documented anywhere? Maybe we should add that to the spec so that the reader can get a holistic view of the flow.
There was a problem hiding this comment.
We have it in the reference implementation here: https://github.com/eth-act/lighthouse/blob/93766c6e29905c20577610c9885a36e76fd29d9f/beacon_node/http_api/src/beacon/pool.rs#L534 on the optional-proofs branch
There was a problem hiding this comment.
In that case, I propose we remove the get_proofs method and introduce the definition of the newly introduced CL endpoint. This will explain the proof flow for the reader.
kevaundray
left a comment
There was a problem hiding this comment.
Left some general questions
|
|
||
| ## Introduction | ||
|
|
||
| This document represents the prover guide accompanying EIP-8025. Provers are |
There was a problem hiding this comment.
What's the benefit of whitelisted provers versus permissionless ones? Is it primarily to prevent DoS attacks from fake proofs?
If that's the only reason, why not simply rely on peer scoring, where a peer gossiping fake proofs gets downscored and eventually disconnected?
There was a problem hiding this comment.
Yeah I feel like a peer scoring approach would be better. It's unclear to me how provers would be added to the whitelist in the first place; who decides this? Eventually, I could see us having in-protocol provers (like builders in ePBS) but I suppose that's a bit too much at this stage (optional proofs).
If an attacker were to spam the execution_proof topic with 1000+ (incorrect) proofs, it would probably cause all of the lightweight verifiers to get out of sync; so I see the purpose in wanting a whitelist of some kind. In-protocol provers would fix this, as you'd be able to slash them if they provided an incorrect proof. Is this an option in EIP-8025? cc @kevaundray
There was a problem hiding this comment.
This was mentioned here for reference: #4828 (comment)
There was a problem hiding this comment.
in-protocol provers would be possible but it would mean that we'd need a hardfork for 8025 because everyone on the network would need to slash the bad provers, not just the ones running the lightweight verifiers
There was a problem hiding this comment.
in-protocol provers would be possible but it would mean that we'd need a hardfork for 8025
As it is now, 8025 will require a hardfork because it modified BeaconState.
There was a problem hiding this comment.
You can refer to BPO for a whitlelist prover config.
However, I would suggest to consider adding prover entity similar to builder. It allows a new prover to join permissionlessly and we would have more control to DoS prevention.
A prover config requires consensus which ties any changes to the prover's landscape to fork cadence. It also seems a bit overly complicated that we need to drive a social consensus on prover list.
There was a problem hiding this comment.
You can refer to BPO for a whitlelist prover config.
However, I would suggest to consider adding prover entity similar to builder. It allows a new prover to join permissionlessly and we would have more control to DoS prevention.
A prover config requires consensus which ties any changes to the prover's landscape to fork cadence. It also seems a bit overly complicated that we need to drive a social consensus on prover list.
A few questions... Would that require a hard fork? Can the Provers be slashed? If not, what value is added by adding the Prover entity?
The high-level constraint was that optional proofs should not require a hard fork.
There was a problem hiding this comment.
Yeah that was the hope but it now changes the beacon state. Also all nodes (that are zkAttesters) should have the same prover whitelist. Otherwise, there could be a divergence.
There was a problem hiding this comment.
Sorry, that's my lack of experience with spec writing and the implications of changes. The intention is to have it as a node config. I did have it as a global variable, but that also wasn't suitable - #4828 (comment). @jtraglia offered to refactor this, so I think he'll push a commit in due course.
There was a problem hiding this comment.
Sorry, I wasn't up-to-date. PPO (Prover Parameter Only) forks looks reasonable to me.
|
|
||
| #### Modified `process_execution_payload` | ||
|
|
||
| *Note*: `process_execution_payload` is modified in EIP-8025 to require both |
There was a problem hiding this comment.
Wasn't the whole purpose of execution proofs to let lightweight nodes run without an execution client (or at least with a stateless one)?
If so, why require both an ExecutionEngine and a ProofEngine for a lightweight client?
Conversely, EIP-8025 is titled "Optional Execution Proofs."
Yet with this PR, all nodes, including stateful, classic ones, will need execution proofs to import a block.
The EIP states:
Optional execution proofs allow beacon nodes to verify the validity of the execution payload within a beacon block without running an execution layer client.
This PR seems to make an EL mandatory for all node types.
There was a problem hiding this comment.
I believe these specs represent what full nodes & validators would implement. There could be another node type for lightweight verifiers which are not connected to an EL client.
Edit: Yeah, see the "Proof Verification Flow" diagram in the PR's description.
There was a problem hiding this comment.
I wonder if we should add back a stateless validation flag like on master here to either verify using the execution engine or veify a proof using the proof engine @frisitano ?
There was a problem hiding this comment.
The high-level idea was that a node can be spun up with a ProofEngine, an ExecutionEngine, or both. If we only have a ProofEngine, then the ExecutionEngine invocation would be a no-op. Similarly, if we only have an ExecutionEngine, then the ProofEngine call would be a no-op.
I agree that this spec doesn't capture this construct well and isn't clear. In regard to options as I see them:
- Introduce the stateless validation flag as @kevaundray suggested (I think this exposes implementation details to the spec, which isn't ideal)
- Make this clear in the specs via comments
- Revert
process_execution_payloadback to default, and instead add a comment toExecutionEngine::verify_and_notify_new_payloadthat it should invoke theProofEngine(if configured) as well as an external re-execution node (if configured). - Instead of introducing the
ProofEngine, we extend theExecutionEnginewith methods for proofs and leave the method implementations as an implementation detail.
I tend to be in favour of 3 or 4 but keen to hear others' opinions.
There was a problem hiding this comment.
I think replacing ExecutionEngine with ProofEngine would be the ideal especially in terms of forward-compatibility. process_execution_payload can receive proof engine instead of execution engine, and replace the call to execution engine with proof engine.
What do people feel about this?
There was a problem hiding this comment.
Yes, that sounds like a good approach. I agree.
There was a problem hiding this comment.
(Bridging my comment from Discord)
On the actual implementation, clients can expose a flag to switch to EIP-8025. But on the spec side, we can remove payload execution (and maybe other execution engine stuff, if it makes sense)
|
@frisitano from our meeting with Mikhail + Ignacio, noting down the points that came up:
Let me know if this sounds off! |
Sounds correct, and I align with your proposal to follow the blob flows. |
|
|
||
| ## Introduction | ||
|
|
||
| This document represents the prover guide accompanying EIP-8025. Provers are |
There was a problem hiding this comment.
Yeah that was the hope but it now changes the beacon state. Also all nodes (that are zkAttesters) should have the same prover whitelist. Otherwise, there could be a divergence.
| assert proof_engine.verify_new_payload_request_header(new_payload_request_header) | ||
|
|
||
| # Cache execution payload header | ||
| state.latest_execution_payload_header = ExecutionPayloadHeader( |
There was a problem hiding this comment.
We can reuse ExecutionPayloadHeader that we constructed when calling verify_new_payload_request_header.
| #### New `process_prover_signed_execution_proof` | ||
|
|
||
| ```python | ||
| def process_prover_signed_execution_proof( |
There was a problem hiding this comment.
Where is this function be called?
There was a problem hiding this comment.
This would be called upon receipt of an execution proof from the execution_proof gossip topic.
There was a problem hiding this comment.
Please specify that requirement.
| - `execution_requests = body.execution_requests` | ||
| 2. Create `ProofAttributes` with desired proof types. | ||
| 3. Call | ||
| `proof_gen_id = proof_engine.request_proofs(new_payload_request, proof_attributes)` |
There was a problem hiding this comment.
Could you give me a pointer to a discussion proof_gen_id vs proof_id?
There was a problem hiding this comment.
proof_id currently isn't defined in these specs. We also haven't defined how proof_gen_id would be generated.
There was a problem hiding this comment.
Oh, I was asking naming decision. I think I saw a comment that proof_id could be misleading for some reason but not sure if I remember it correctly nor where I saw it.
There was a problem hiding this comment.
I wasn't able to find the discussion you are referring to, but I do vaguely remember it being discussed somewhere at some point.
I was thinking that proof_id could be reserved as an identifier for a specific proof instance, whereas proof_gen_id would be the proof generation identifier.
There was a problem hiding this comment.
Thanks for explanation. What is proof instance? The proof (bytes) itself?
| `proof_gen_id = proof_engine.request_proofs(new_payload_request, proof_attributes)` | ||
| to initiate proof generation. | ||
| 4. The proof engine generates proofs asynchronously and delivers them to the | ||
| prover via `POST /eth/v1/prover/execution_proofs`. Each proof is delivered |
There was a problem hiding this comment.
This part smells bad to me. To my understanding, provers will be builders eventually. Then it doesn't seem right to specify the proof generation part here.
I also think we should adds proof deadline for prover such that they can do something when the requested proof is not delivered in time.
There was a problem hiding this comment.
To my understanding, provers will be builders eventually.
Yes, that's correct.
they can do something when the requested proof is not delivered in time.
What do you think they should do? The idea is that the contract between the CL and the proof engine is that proofs must be delivered by the proof engine upon request, and it's the responsibility of the ProofEngine to fulfil this, including any retries, etc.
There was a problem hiding this comment.
But you can't guarantee that you will have your proofs always. If ProofEngine fails to deliver any proof in time, you would have no choice but to miss support from zkAttesters. It's part of the design IMO.
There was a problem hiding this comment.
So yeah, there is not so much they can do and hence probably nothing to specify their behavior when they haven't received any proof in time. A short note that what they will do/get would work, such as broadcast the full payload.
There was a problem hiding this comment.
Yes, that's correct. If no one generates proofs in time, then zk attesters won't be able to attest, but I'm unsure what modifications/behaviour the prover or attestors could implement to address this.
There was a problem hiding this comment.
That makes sense, lets add a note.
There was a problem hiding this comment.
Agreed that nothing to specify when proofs are missing.
Going back to my original comment, ExecutionEngine is designed to be unidirectional. Whether ProofEngine should follow that convention is another topic. It was designed way before I joined this space so I would like to hear other's opinions on if the communication proposed in this PR looks good.
Until then, I would propose to stick to request_proofs & get_proof combination.
There was a problem hiding this comment.
I initially followed this convention, but the team provided feedback on the approach, discussed here: #4828 (comment). I would invite @kevaundray to this conversation.
There was a problem hiding this comment.
Where would we define how zk attesters receive proofs?
Co-authored-by: JihoonSong <jihoonsong@users.noreply.github.com>
| parent_beacon_block_root=state.latest_block_header.parent_root, | ||
| execution_requests=body.execution_requests, | ||
| ) | ||
| assert proof_engine.verify_new_payload_request_header(new_payload_request_header) |
There was a problem hiding this comment.
If I'm not wrong, the right modifications here at the high-level are:
- pass proofs received so far (i.e., change
process_execution_payload's signature) - extract
NewPayloadRequestHeader assert hash_tree_root(new_payload_request_header) == proof.new_payload_request_header_root- call proof engine's
verify_execution_proof.
This basically merges process_prover_signed_execution_proof into process_execution_payload. But the tricky part is that you should have enough proofs before calling this. So we would need some assertion like this assert len(proofs) >= N (I'm thinking out loud here).
One easy solution comes to me is, we introduce a deadline for proofs, which is set before the block's deadline. e.g., proofs deadline is 7s while block deadline is 8s on top of Fulu.
There was a problem hiding this comment.
I was thinking this could be done async and left as an implementation detail (left unspecified). If proofs do not arrive before proof_engine.verify_new_payload_request_header is invoked, then it should return SYNCING, and the block should be added to the forkchoice store optimistically. Then, upon proof arrival when len(proofs) >= N is satisfied, the proof engine will return something like Result { valid: Valid, valid_root: Some(root) } and there is a call on the fork choice store to update the payload status associated with the valid_root. This allows async processing of blocks and proofs. I believe this somewhat mirrors the pattern in ePBS.
There was a problem hiding this comment.
Could you explain what does verify_new_payload_request_header do and why do we need it?
There was a problem hiding this comment.
It must be specified, especially if it touches fork choice.
There was a problem hiding this comment.
I believe this somewhat mirrors the pattern in ePBS.
I don't follow this at all. ePBS is not underspecified with regard to block process and fork choice from my understanding.
There was a problem hiding this comment.
verify_new_payload_request_header just checks len(execution_proofs) > threshold in the ProofEngine.
There was a problem hiding this comment.
By the way I've written up some considerations on consensus for fulu and gloas here - https://hackmd.io/@frisitano/BJ9P2IpQ-e#Consensus-Considerations
There was a problem hiding this comment.
Ah, is it because proofs are stored in ProofEngine? Then, the storing function should also be specified. But to me, I'd prefer to store proofs in the CL, rather than making ProofEngine stateful. You can store it in an implementation dependent cache and still avoid hard fork. You can refer to InclusionListStore.
There was a problem hiding this comment.
This makes sense. I will introduce this.
There was a problem hiding this comment.
Sorry, multiple factors came up to my mind. Please wait a bit. Let me think more about this.
| #### New `process_prover_signed_execution_proof` | ||
|
|
||
| ```python | ||
| def process_execution_proof( |
There was a problem hiding this comment.
I also think k-out-of-n policy should be described somewhere in the spec.
jtraglia
left a comment
There was a problem hiding this comment.
@frisitano & others, great work on this PR! I think the refactor looks really good. It's not quite perfect yet, but it's in a good enough state to get merged 🙂 I would like to merge this now & continue working on it in smaller, more iterative PRs. This one takes like 20 seconds to load on my poor old laptop now due to all of the review comments and flow diagrams 😅
Thanks everyone for helping to get this over the line 🙂 |
|
Thank you everyone who worked on this PR! On top of that, I must say @frisitano is doing great job in this PR and other related work. Merging this now and iterating from here sounds very good as this PR now has too long threads to follow (... and yes I contributed to that mess 😅.) Kudos for @kevaundray for creating an issue that lists up ongoing discussions! |
EIP-8025
This is a refactor of EIP-8025 to introduce the
ProofEngineas an implementation-specific protocol for proof verification. We migrate to using theNewPayloadRequestas the private input andNewPayloadRequest.hash_tree_root()as the public input. Containers and functions related to the internals of the proof engine have been removed.EL API: ethereum/execution-apis#735
BN API: ethereum/beacon-APIs#569
EIP-8025 Flow Diagrams
1. Proof Generation & Submission Flow
sequenceDiagram participant Prover as Prover/Relay participant BN as Beacon Node participant PE as Proof Engine (EL) BN->>Prover: Observe new BeaconBlock Prover->>Prover: Extract NewPayloadRequest Prover->>PE: engine_requestProofsV1(payload, proofAttributes) PE-->>Prover: ProofGenId Note over PE: Proof generation (async) PE->>Prover: POST /eth/v1/prover/execution_proofs<br/>(unsigned ExecutionProof) Prover->>Prover: Sign with prover key Prover->>BN: Broadcast ProverSignedExecutionProof<br/>on execution_proof topic2. Signed Proof Reception Flow
sequenceDiagram participant Gossip as P2P Gossip participant BN as Beacon Node participant PE as Proof Engine (EL) Gossip->>BN: ProverSignedExecutionProof<br/>on execution_proof topic BN->>BN: Verify prover_pubkey in whitelist BN->>BN: Verify BLS signature BN->>PE: engine_verifyExecutionProofV1(proof) PE-->>BN: Valid PE->>PE: Store proof in proof engine3. Proof Verification Flow (Block Processing)
sequenceDiagram participant BN as Beacon Node participant PE as Proof Engine (EL) Note left of BN: BeaconBlock received BN->>BN: process_block() BN->>BN: Construct NewPayloadRequestHeader BN->>PE: engine_verifyNewPayloadRequestHeaderV1() Note over PE: Check sufficient proofs exist<br/>for newPayloadRequestRoot PE-->>BN: Valid (proofs satisfied) BN->>BN: Update state4. System Architecture
flowchart TB subgraph Consensus Layer BN[Beacon Node] Gossip[P2P Gossip<br/>execution_proof topic] Prover[Prover/Relay] end subgraph Execution Layer PE[Proof Engine] end BN-->|1. new BeaconBlock|Prover Prover -->|2. engine_requestProofsV1| PE PE -->|3. POST /eth/v1/prover/execution_proofs| Prover Prover -->|4. Sign & Broadcast| Gossip Gossip -->|5. Receive proofs| BN BN -->|6. engine_verifyExecutionProofV1| PE BN -->|7. engine_verifyNewPayloadRequestHeaderV1| PE5. Data Type Relationships
classDiagram class ExecutionProof { +ByteList proof_data +uint8 proof_type +PublicInput public_input } class PublicInput { +Root new_payload_request_root } class ProverSignedExecutionProof { +ExecutionProof message +BLSPubkey prover_pubkey +BLSSignature signature } class NewPayloadRequestHeader { +ExecutionPayloadHeader execution_payload_header +Sequence~VersionedHash~ versioned_hashes +Root parent_beacon_block_root +ExecutionRequests execution_requests } ExecutionProof --> PublicInput ProverSignedExecutionProof --> ExecutionProof