Conversation
| - name: engine_requestProofsV1 | ||
| summary: Requests proof generation for a new payload request |
There was a problem hiding this comment.
Checking, do we still need this if the proofs are pushed to the CL versus the CL requesting proofs from the proof engine?
There was a problem hiding this comment.
I think it makes sense to keep it. The CL should instruct the proof engine what blocks to generate proofs for. It requires consensus layer context to determine when a proof is required by that nodes builder. I think the flow is pretty clean if you look at the first diagram in this PR (prover replaced by builder in the future) ethereum/consensus-specs#4828. What do you think?
There was a problem hiding this comment.
Ah I see, I think I merged some of the prover related responsibilities into zkboost -- but perhaps it doesn't matter
src/engine/eip8025.md
Outdated
|
|
||
| The fields are encoded as follows: | ||
|
|
||
| - `newPayloadRequestRoot`: `DATA`, 32 Bytes - Root of the `NewPayloadRequest` being proven. |
There was a problem hiding this comment.
Do we need to be more explicit what root means in this context?
I think might be okay as it is if usually "root" means CL and "hash" means EL as Jihoon mentioned some days ago.
There was a problem hiding this comment.
Yes, I think it is ok because a similar pattern and explanation is used for other constructs in the repo, e.g.:
3. `parentBeaconBlockRoot`: `DATA`, 32 Bytes - Root of the parent beacon block.
src/engine/eip8025.md
Outdated
| This structure contains the header fields of an execution payload (without transactions): | ||
|
|
||
| - `parentHash`: `DATA`, 32 Bytes | ||
| - `feeRecipient`: `DATA`, 20 Bytes | ||
| - `stateRoot`: `DATA`, 32 Bytes | ||
| - `receiptsRoot`: `DATA`, 32 Bytes | ||
| - `logsBloom`: `DATA`, 256 Bytes | ||
| - `prevRandao`: `DATA`, 32 Bytes | ||
| - `blockNumber`: `QUANTITY`, 64 Bits | ||
| - `gasLimit`: `QUANTITY`, 64 Bits | ||
| - `gasUsed`: `QUANTITY`, 64 Bits | ||
| - `timestamp`: `QUANTITY`, 64 Bits | ||
| - `extraData`: `DATA`, 0 to 32 Bytes | ||
| - `baseFeePerGas`: `QUANTITY`, 256 Bits | ||
| - `blockHash`: `DATA`, 32 Bytes | ||
| - `transactionsRoot`: `DATA`, 32 Bytes | ||
| - `withdrawalsRoot`: `DATA`, 32 Bytes | ||
| - `blobGasUsed`: `QUANTITY`, 64 Bits | ||
| - `excessBlobGas`: `QUANTITY`, 64 Bits |
There was a problem hiding this comment.
Does this definition has to be aware of forks, thus some fields being optional?
There was a problem hiding this comment.
I think the standard in this repo is that the types should be versioned, and each version contains the appropriate fields for the fork. This means the current V1 struct is aligned with Osaka.
|
|
||
| #### Response | ||
|
|
||
| * result: [`ProofStatusV1`](#proofstatusv1) - The verification result. |
There was a problem hiding this comment.
Maybe a nit, but for this engine_verifyNewPayloadRequestHeaderV1 we should technically name the return type ProofsStatusV1 instead of ProofStatusV1?
Since technically we are checking a proofs policy, no?
Not sure it is worth changing tbh -- just surfacing thought.
There was a problem hiding this comment.
I think it is fine to use the same return type ProofStatusV1 for both engine_verifyNewPayloadRequestHeaderV1 and engine_verifyExecutionProofV1 and have slightly different interpretations. This is analogous to how PayloadStatusV* is used as the return type for both engine_newPayload* and engine_forkchoiceUpdated*.
| 2. `versionedHashes`: `Array of DATA`, 32 Bytes each - Array of blob versioned hashes. | ||
| 3. `parentBeaconBlockRoot`: `DATA`, 32 Bytes - Root of the parent beacon block. | ||
| 4. `proofAttributes`: [`ProofAttributesV1`](#proofAttributesv1) - Attributes specifying which proofs to generate. | ||
| * timeout: 1s |
There was a problem hiding this comment.
Good catch, I added a note - *Note*: The timeouts are not definitive.
src/engine/eip8025.md
Outdated
| 1. `executionPayload`: [`ExecutionPayloadV3`](./cancun.md#executionpayloadv3) - The execution payload to generate proofs for. | ||
| 2. `versionedHashes`: `Array of DATA`, 32 Bytes each - Array of blob versioned hashes. | ||
| 3. `parentBeaconBlockRoot`: `DATA`, 32 Bytes - Root of the parent beacon block. | ||
| 4. `proofAttributes`: [`ProofAttributesV1`](#proofAttributesv1) - Attributes specifying which proofs to generate. |
There was a problem hiding this comment.
Do we need to add execution requests here?
There was a problem hiding this comment.
Yep, could be named request_bytes or similar (use same name as CL specs).
There was a problem hiding this comment.
I propose we remain internally consistent in the execution APi specs? For example, executionRequests is used here:
execution-apis/src/engine/prague.md
Lines 66 to 74 in 11517b4
Introduces the API for the proof engine.
CL PR: ethereum/consensus-specs#4828