EIP-7732 current fork spectests #3854
Conversation
|
|
||
| *Note:* This specification is built upon [Electra](../../electra/beacon-chain.md) and is under active development. | ||
|
|
||
| This feature adds new staked consensus participants called *Builders* and new honest validators duties called *payload timeliness attestations*. The slot is divided in **four** intervals. Honest validators gather *signed bids* (a `SignedExecutionPayloadHeader`) from builders and submit their consensus blocks (a `SignedBeaconBlock`) including these bids at the beginning of the slot. At the start of the second interval, honest validators submit attestations just as they do previous to this feature). At the start of the third interval, aggregators aggregate these attestations and the builder broadcasts either a full payload or a message indicating that they are withholding the payload (a `SignedExecutionPayloadEnvelope`). At the start of the fourth interval, some validators selected to be members of the new **Payload Timeliness Committee** (PTC) attest to the presence and timeliness of the builder's payload. |
There was a problem hiding this comment.
I think (Signed|)ExecutionPayloadHeader should be renamed to (Signed|)ExecutionPayload(Bid|Commitment|Info|Metadata), as it serves a new purpose.
.+Header so far is used for SSZ summaries of the corresponding base type, which is no longer the case as the full ExecutionPayload still contains the extra fields such as transactions_root and so on.
Also, the ExecutionPayloadHeader is used by light clients as part of LightClientHeader. They still need the previous semantics of it being the SSZ summary of the ExecutionPayload. Therefore, recommending to drop ExecutionPayloadHeader from the beacon-chain.md spec and creating a new type of different name.
There was a problem hiding this comment.
Yeah @ethDreamer had the same complaint, I don't mind any name change eventually for this object. It's not a trivial change to implement in the spec, so probably will wait until this is agreed to be shipped.
| ```python | ||
| class ExecutionPayloadEnvelope(Container): | ||
| payload: ExecutionPayload | ||
| builder_index: ValidatorIndex | ||
| beacon_block_root: Root | ||
| blob_kzg_commitments: List[KZGCommitment, MAX_BLOB_COMMITMENTS_PER_BLOCK] | ||
| payload_withheld: boolean | ||
| state_root: Root | ||
| ``` |
There was a problem hiding this comment.
Side note that this one would benefit from EIP-7495 SSZ StableContainer as payload_withheld could be dropped and payload become Optional[ExecutionPayload]
There was a problem hiding this comment.
Indeed, I'm waiting for that to be adopted in Electra and I'll rebase on top of it
specs/_features/eip7732/builder.md
Outdated
|
|
||
| ### Honest payload withheld messages | ||
|
|
||
| An honest builder that has seen a `SignedBeaconBlock` referencing his signed bid, but that block was not timely and thus it is not the head of the builder's chain, may choose to withhold their execution payload. For this the builder should simply act as if it were building an empty payload, without any transactions, withdrawals, etc. The `payload.block_hash` may not be equal to `header.block_hash`. The builder may then sets `payload_withheld` to `True`. If the PTC sees this message and votes for it, validators will attribute a *withholding boost* to the builder, which would increase the forkchoice weight of the parent block, favoring it and preventing the builder from being charged for the bid by not revealing. |
There was a problem hiding this comment.
Today, empty payload is actually not allowed in Capella, at the very least the withdrawals are required. Would this allow the situation where someone can intentionally produce a block with a payload that does not contain withdrawals?
There was a problem hiding this comment.
A payload without withdrawals would be irrelevant here, it could even be an invalid payload. If the withheld wins then the CL block is reorged, if it loses then the payload is invalid and the builder pays the bid. Either way, having the payload to be optional is a cleaner solution, but I will wait until it's merged in dev.
| builder_index: ValidatorIndex | ||
| slot: Slot | ||
| value: Gwei | ||
| blob_kzg_commitments_root: Root |
There was a problem hiding this comment.
Couldn't blob_kzg_commitments_root also be delayed until after process_execution_payload?
Otherwise, that's non-deleteable jank that remains even if the payload ultimately gets withheld.
There was a problem hiding this comment.
I think in principle it could, the bid may not even commit a payload has even in principle. But I went with the minimal modification to the current consensus algo. Can switch to that if required. I don't see why this causes yunk though, it's just in the header that it's last committed. It doesn't appear anywhere else.
| | `EIP7732_FORK_VERSION` | `eip7732.BlobSidecar` | | ||
|
|
||
|
|
||
| ##### ExecutionPayloadEnvelopeByRoot v1 |
There was a problem hiding this comment.
A ByRange request could be useful for historical syncing (with same retention period as SignedBeaconBlock).
It should still be possible to sync an EL that is not connected to the Internet, from solely:
- the archived EL history up to the merge
- and then from archived CL history (era files +
SignedExecutionPayloadEnvelope)
There was a problem hiding this comment.
| | `EIP7732_FORK_VERSION` | `eip7732.SignedBeaconBlock` | | ||
|
|
||
|
|
||
| ##### BlobSidecarsByRoot v2 |
There was a problem hiding this comment.
These don't need a new (v2) version, the existing one is already versioned by ForkDigest-context. New version on the request is only needed if the request parameters or semantics change.
| voluntary_exits: List[SignedVoluntaryExit, MAX_VOLUNTARY_EXITS] | ||
| sync_aggregate: SyncAggregate | ||
| # Execution | ||
| # Removed execution_payload [Removed in EIP-7732] |
There was a problem hiding this comment.
In Nimbus, we actually use execution_payload.prev_randao to skip RANDAO mix computation from history when computing shufflings for data from alternate branches 🙃
There was a problem hiding this comment.
Actually, not an issue, the prev_randao is still available by loading the SignedExecutionPayloadEnvelope.
There was a problem hiding this comment.
Thanks for the proposal. This is a big upgrade over the current builder API.
One aspect that needs more consideration is the sync committee.
As is, the sync committee keeps signing the BeaconBlock from prior slot. However, the BeaconBlock no longer contains the full EL block header. body.signed_execution_payload_header.message.block_hash is unreliable because it may turn out that the payload was withheld and the block itself is a noop.
The latest reliable EL block hash is available in state_root->state.latest_block_hash, which is most likely the one from 2 slots ago (latest), or earlier (in case of missed blocks / withheld payloads). This is probably as good as it can get here, but is still notable as it means that any LC based syncs are now another 12 seconds behind (not prohibitively slow, still).
With state_root->state.latest_block_hash, implementations need additional BeaconState data to obtain the latest valid EL block hash. Given that historical BeaconState access is quite expensive, it could be worthwhile to continue tracking the latest_block_hash inside BeaconBlockBody (or in BeaconBlockHeader so it automatically gets exposed to BeaconState.latest_block_header as well).
Furthermore, the EL block header itself has not been switched to SSZ. As proposed, while the EL block hash continues to be available, all SSZ commitments to the ExecutionPayload are removed: Neither BeaconBlock nor BeaconState contain an SSZ commitment to it. This is a step backwards for CL light clients because it removes efficient SSZ access to EL block header fields and requires MPT/RLP when fetching the data in a separate roundtrip from an EL. Adding the latest_execution_payload_root to BeaconBlockBody (and, therefore, also toBeaconState) would be very appreciated to retain the functionality until the EL block header is switched to SSZ.
specs/_features/eip7732/builder.md
Outdated
|
|
||
| [Modified in EIP-7732] | ||
|
|
||
| The `BlobSidecar` container is modified indirectly because the constant `KZG_COMMITMENT_INCLUSION_PROOF_DEPTH` is modified. The function `get_blob_sidecars` is modified because the KZG commitments are no longer included in the beacon block but rather in the `ExecutionPayloadEnvelope`, the builder has to send the commitments as parameters to this function. |
There was a problem hiding this comment.
If EIP-7688 is adopted, BeaconBlockBody gindices and depth constants become stable, and BeaconBlockBody changes no longer affect random other containers such as BlobSidecar
Furthermore, PeerDAS removes BlobSidecar; if the activation epoch is synchronized with EIP-7688, it means that there is no expected breakage here.
| At any given slot, the status of the blockchain's head may be either | ||
| - A block from a previous slot (e.g. the current slot's proposer did not submit its block). | ||
| - An *empty* block from the current slot (e.g. the proposer submitted a timely block, but the builder did not reveal the payload on time). | ||
| - A full block for the current slot (both the proposer and the builder revealed on time). |
There was a problem hiding this comment.
Maybe present/absent would be better names than full/empty.
And, using execution block explicitly in the spec to avoid ambiguity with the beacon block.
This reverts commit aac95e2.
- Disables blocks with blobs in deneb tests - Correctly starts forkchoice on `get_forkchoice_store`
This PR enables and implements the modification to the pyspec tests so that the currently existing tests are executed with the EIP-7732 fork enabled. It is based on top of #3828