-
Notifications
You must be signed in to change notification settings - Fork 201
Standardized Epbs Beacon Api #552
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
f630964 to
2953725
Compare
1f530e8 to
8faafdf
Compare
apis/validator/block.v3.yaml
Outdated
| oneOf: | ||
| - title: "ProduceBlockV3Response (Gloas)" | ||
| type: object | ||
| required: [version, data, consensus_block_value] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the execution_payload_blinded and execution_payload_value from being required in the response post-gloas since there's no longer a concept of blinded blocks, and the block's "value" is now the bid, BeaconBlockBody::SignedExecutionPayloadBid::value
but lmk if these fields should be resurrected
not really sure if we need consensus_block_value at this point. it doesn't seem used in lighthouse, but I kept it as required since perhaps other clients use it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
introducing a produceBlockV4 makes much more sense imo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The consensus_block_value is used by multi-node validator clients like Vero/Vouch, let's please keep it unless there's a good reason to remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the feedback. we agreed that produceBlockV4 makes more sense on the last epbs breakout room call, so I added the endpoint and left produceBlockV3 untouched. produceBlockV4 has consensus_block_value as required per @eth2353's description as to why it's useful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
produceBlockV4 definitely makes more sense
This is not the design for dual deadlines though: there is a single object to sign at the second deadline, but the beacon will return payload present |
apis/validator/attestation_data.yaml
Outdated
| description: | | ||
| Requests that the beacon node produce an AttestationData. For `slot`s in | ||
| Electra and later, this AttestationData must have a `committee_index` of 0. | ||
| Electra and Fulu, this AttestationData must have a `committee_index` of 0. In Gloas, this `committee_index` field is repurposed to signal payload status: 0 if the execution payload is not present in the canonical chain (EMPTY), or 1 if the payload is present (FULL). For current slot attestations, always use 0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a more precise statement to the meaning of "current slot attestation"? I've seen people confused by this term.
At any rate it refers to the fact that the head block root is the block proposed in the same slot as the AttestationData.slot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Def can be a bit confusing. Updated the copy to "For current slot attestations, which means the head block root is a block proposed in the same slot as the AttestationData.slot, always use 0."
| - name: builder_index | ||
| in: path | ||
| required: true | ||
| description: "Index of the builder from which the execution payload envelope is requested." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need the builder here? I assume this is to be signed, but there can only be different builder indices in case of an equivocation or a bad fork. Shouldn't the beacon just return the one for it's head view?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@potuz, great question. I saw that prysm was passing builder_index in the GetExecutionPayloadEnvelope here
My first thought was that it's a nice early return to prevent expensive state root calculation supposing the passed in builder_index doesn't match the one in the envelope
Possibly credible scenarios I considered:
a. a proposer (self-building) and an in-protocol builder could call on the same beacon node to create bids for a slot. That BN would want to cache both resulting envelopes containing their respective builder_index. Then, after the block is released, either the in-protocol builder or the self-building proposer's bid would be included, so they'd want to fetch their cached envelope from the BN, and would use their builder_index to grab the correct one
b. rare edge cases like chain re-org causing proposer duties to change for a slot, and the VC doesn't remove the previously assigned proposer quickly enough, and you end up with two proposers for the same slot. if both proposers are connected to same BN, you'd want to be able to fetch the right envelope based off their builder_index
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in case the block was build locally do we just pass the validator index here?
apis/beacon/blocks/blocks.v2.yaml
Outdated
| before doing so, so as to aid timely delivery of the block. Should the block fail full | ||
| validation, a separate success response code (202) is used to indicate that the block was | ||
| successfully broadcast but failed integration. After Deneb, this additionally instructs | ||
| successfully broadcast but failed integration. After Deneb and before Gloas, this additionally instructs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| successfully broadcast but failed integration. After Deneb and before Gloas, this additionally instructs | |
| successfully broadcast but failed integration. For Deneb/Electra/Fulu, this additionally instructs |
nitpick: It is a bit confusing that after is inclusive but before is exclusive
| operationId: publishExecutionPayloadBid | ||
| summary: Publish signed execution payload bid | ||
| description: | | ||
| Instructs the beacon node to broadcast a signed execution payload bid to the beacon network, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Instructs the beacon node to broadcast a signed execution payload bid to the beacon network, | |
| Instructs the beacon node to broadcast a signed execution payload bid to the network, |
| operationId: publishExecutionPayloadEnvelope | ||
| summary: Publish signed execution payload envelope | ||
| description: | | ||
| Instructs the beacon node to broadcast a signed execution payload envelope to the beacon network, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Instructs the beacon node to broadcast a signed execution payload envelope to the beacon network, | |
| Instructs the beacon node to broadcast a signed execution payload envelope to the network, |
| summary: Publish signed execution payload bid | ||
| description: | | ||
| Instructs the beacon node to broadcast a signed execution payload bid to the beacon network, | ||
| to be gossiped for potential inclusion in block building. A success response indicates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| to be gossiped for potential inclusion in block building. A success response indicates | |
| to be gossiped for potential inclusion in block building. A success response (20x) indicates |
For uniformity with publishExecutionPayloadEnvelope
beacon-node-oapi.yaml
Outdated
| Execution payload value in Wei. Required in response so client can determine relative value | ||
| of execution payloads. | ||
| required: true | ||
| DEPRECATED: Not applicable for gloas fork. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| DEPRECATED: Not applicable for gloas fork. | |
| DEPRECATED: Not applicable post-gloas. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're making a ProduceBlockV4 instead of modifying ProduceBlockV3, we no longer need to touch these Eth-Consensus-Block-Value and Eth-Execution-Payload-Blinded headers, so I reverted them to as they were before this PR
validator-flow.md
Outdated
| with bit on position `validator_committee_index` (see AttesterDuty) set to true | ||
| 5. If aggregator: | ||
| - Wait for `SECONDS_PER_SLOT * 2 / 3` seconds into the assigned slot | ||
| - Wait for `SECONDS_PER_SLOT / 2` seconds into the assigned slot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see #552 (comment)
| Monitor chain block reorganization events (TBD) as they could change attesters and aggregators. | ||
| If reorg is detected, ask for new attester duties and proceed from 1.. | ||
|
|
||
| ### PTC Attesting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add a note that this is post-Gloas
| Monitor chain block reorganization events (TBD) as they could change PTC assignments. | ||
| If reorg is detected, ask for new PTC duties and proceed from 1.. | ||
|
|
||
| ### Builder (Optional) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add a note that this is post-Gloas
validator-flow.md
Outdated
| set `is_aggregator` to `True`, | ||
| 2. Wait for new BeaconBlock for the assigned slot (either stream updates or poll) | ||
| - Max wait: `SECONDS_PER_SLOT / 3` seconds into the assigned slot | ||
| - Max wait: `SECONDS_PER_SLOT / 4` seconds into the assigned slot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we simply overwrite the previous description, the validator flow will no longer explain how things work before Gloas. This is problematic for two reasons:
- After the Gloas hard fork happens, pre-Gloas workflow will not be explained
- If this change merges before the Gloas hard fork, then this flow will describe a future fork and not how things work at present
| Metadata in the response indicates the type of block produced, and the supported types of block | ||
| will be added to as forks progress. | ||
| are for all pre-Gloas forks. This endpoint is not forwards compatible after the Fulu fork. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to deprecate produceBlockV3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't need it anymore after the fork transition, right? not sure if it's customary to deprecate now or in separate PR after the fork is live on mainnet. I welcome more thoughts on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, yeah, produceBlockV3 is stated in this PR to not work into Glamsterdam. So if produceBlockV4 will work in pre-Glamsterdam forks, it can and will completely replace produceBlockV3.
Regarding the deprecation, the overall goal per existing custom would be to have deprecated for the duration of Glamsterdam, i.e. there's one hardfork during which people can adjust tooling for their other use cases where in theory produceBlockV3 might still suffice, then by H-fork it can be removed from the API.
I don't know if it's critical exactly when the deprecation would happen to achieve this timeline; there's an argument that if one waits until just after the mainnet fork, well, it's not been deprecated for all of mainnet Glamsterdam, has it, etc. So it should cleanly line up somehow with that fork, or be treated as such for the deprecation-then-one-hardfork-later-remove logistics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, so produceBlockV4 won't work in pre-glamsterdam forks because it won't support response headers like execution_payload_value or execution_payload_blinded. Additionally, it doesn't support sending back BlockContents containing block, kzg_proofs, and blobs, just block going forward.
originally, I thought backwards compatibility might be preferable, so I had just extended produceBlockV3 to support gloas, but the consensus is to make a new produceBlockV4 as discussed here, so I made the simplified produceBlockV4
is there utility in supporting produceBlockV3 post-gloas? I had looking through lighthouse codebase, and it seems like the produceBlockV3 logic is only called by the VC during block production, so my assumption was we'd need produceBlockV3 and produceBlockV4 to handle fork transition. then, later, we could rip out produceBlockV3. but maybe other clients or otherwise will have some utility of being able to use produceBlockV3 to be able to create pre-gloas blocks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, if produceBlockV4 won't be able to create pre-Glamsterdam blocks, probably not ideal to deprecate produceBlockV3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually the new api which deprecates the previous one is backward compatible since we wanna keep supporting previous forks, but when it comes to block production, many clients already can't produce pre-electra blocks due to removing eth1 deposit code. I think we can keep v3 for now without deprecating it as doing so would suggest we would remove it in H* which is not clear we wanna do that, also depends on how long we need to support pre-gloas for other networks.
types/gloas/block.yaml
Outdated
| BeaconBlockBodyCommon: | ||
| # An abstract object to collect the common fields between the BeaconBlockBody and the BlindedBeaconBlockBody objects | ||
| type: object | ||
| description: "The [`BeaconBlockBody`](https://github.com/ethereum/consensus-specs/blob/master/specs/gloas/beacon-chain.md#beaconblockbody) object from the CL Gloas spec." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these https://github.com/ethereum/consensus-specs/blob/master/ URLs are potentially unstable, because the consensus specs can and have in the past been reorganized. Some approaches are commit hashes and tags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great point! updated all the gloas types to reference URL's with latest commit hash
| description: "Slot for which the execution payload bid is requested. Must be current slot or next slot." | ||
| schema: | ||
| $ref: "../../beacon-node-oapi.yaml#/components/schemas/Uint64" | ||
| - name: builder_index |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apologies in advance, i'm asking this without understanding the full scope of changes in gloas, but the way this API reads sounds like it's requires a beacon node to be configured for builder setup via flag. If that's the case we should specify in the description for getExecutionPayloadBid that some error code should be returned if the bn only allows for self building.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is correct that this GET execution_payload_bid endpoint will only need to be called by validator's who register as builders in order to be able to construct a bid
The BN that this builder queries could create a bid in at least a couple of ways:
a. call get_payload and return payload retrieved from it's local EL, basically same flow as self-building
b. use some modified get_payload that retrieves a payload using more advanced sequencing, as discussed a bit on discord
I suppose there would be some configuration flag in the BN to decide whether to retrieve payload via approach a or b
but since the BN could return a payload either way, perhaps we don't need any additional error codes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps an error could be if the builder index is not a 0x03 validator, there's no need to get a bid, even locally, if it's not for an actual builder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that makes sense. added here
beacon-APIs/apis/validator/execution_payload_bid.yaml
Lines 67 to 75 in b30273c
| "422": | |
| description: "The builder_index does not correspond to a validator with builder withdrawal credentials (0x03 prefix)" | |
| content: | |
| application/json: | |
| schema: | |
| $ref: "../../beacon-node-oapi.yaml#/components/schemas/ErrorMessage" | |
| example: | |
| code: 422 | |
| message: "Builder index does not correspond to a registered builder validator" |
validator-flow.md
Outdated
| 2. Wait for new BeaconBlock for the assigned slot (either stream updates or poll) | ||
| - Max wait: `SECONDS_PER_SLOT / 3` seconds into the assigned slot | ||
| - Pre-Gloas forks: Max wait `SECONDS_PER_SLOT / 3` seconds into the assigned slot | ||
| - Post-Gloas forks: Max wait `SECONDS_PER_SLOT / 4` seconds into the assigned slot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since SECONDS_PER_SLOT will be deprecated in the fork, we should use ATTESTATION_DUE_BPS_GLOAS here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sg to me, updated here 2d1438f
validator-flow.md
Outdated
| 4. If bid is selected by proposer in their block: | ||
| - [Fetch ExecutionPayloadEnvelope](#/Validator/getExecutionPayloadEnvelope) from beacon node | ||
| - Sign envelope and [submit SignedExecutionPayloadEnvelope](#/Beacon/publishExecutionPayloadEnvelope) | ||
| - Must submit early enough for PTC attestation by `3/4` of slot duration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should use PAYLOAD_ATTESTATION_DUE_BPS which reference to consensus-specs rather than fixed, because it is configurable parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see reply above
ensi321
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we also need to deprecate/remove all apis related to blinded block. Could be done in another PR
validator-flow.md
Outdated
| This requires registering with builder-specific withdrawal credentials (`BUILDER_WITHDRAWAL_PREFIX`). | ||
|
|
||
| Building: | ||
| 1. [Fetch ExecutionPayloadBid](#/Validator/getExecutionPayloadBid) from beacon node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when would we actually ask the beacon node for a bid? currently the bidding for slot N already starts in slot N-1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nflaig, I see in the builder section of spec that it says, "Builders can broadcast a payload bid for the current or the next slot's proposer to include."
so I updated this section to:
1. [Fetch ExecutionPayloadBid](#/Validator/getExecutionPayloadBid) from beacon node for the current or next slot's proposer to include.
but that doesn't really answer your question of when the builder technically has to ask the beacon node for a bid as to be included in the block. maybe it makes sense to add something like, "if submitting a bid for the current slot, the bid should be published at the start of the slot". or should be just not be opinionated on when to submit the bid?
cc @potuz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or should be just not be opinionated on when to submit the bid?
maybe that's the way to go, even though the spec states the following
Validators are still expected to propose
SignedBeaconBlockat the beginning of
any slot
but realistically you are not gonna pick the best bid at t=0 of the slot but rather use the available time you have to collect a better bid, so very likely competitive bids will not even be published at the start of the slot but close to the ATTESTATION_DUE_BPS_GLOAS deadline (currently 3 seconds into the slot)
| - name: builder_index | ||
| in: path | ||
| required: true | ||
| description: "Index of the builder from which the execution payload envelope is requested." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in case the block was build locally do we just pass the validator index here?
it's the same as with |
aa8a636 to
3f5ecc5
Compare
|
@nflaig updated description to make it more clear: |
| 4. Post-Gloas, if self-building (proposer's own bid included in block): | ||
| - [Fetch ExecutionPayloadEnvelope](#/Validator/getExecutionPayloadEnvelope) from beacon node | ||
| - Sign envelope and [submit SignedExecutionPayloadEnvelope](#/Beacon/publishExecutionPayloadEnvelope) | ||
| - Must submit early enough for PTC attestation by [PAYLOAD_ATTESTATION_DUE_BPS](https://github.com/ethereum/consensus-specs/blob/00d531949b1f30516979b60ddd2a411e7f388299/specs/gloas/validator.md#time-parameters) of slot duration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is more of a question but is there any incentive to publish the execution payload later? Since you already commit to a specific block hash when you publish the SignedBeaconBlock I would assume the payload roughly at the same time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ha good question, I did ask this some time ago on an epbs breakout room call. The consensus was that releasing payload right after gossiping the block makes the most sense during self-building. Additionally, you can trigger blob release in the BN when you call the publishExecutionPayloadEnvelope since no reason to delay that either
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The consensus was that releasing payload right after gossiping the block makes the most sense during self-building
This definitely makes sense, I guess builders (that include MEV transactions) have to be more careful and make sure the SignedBeaconBlock has been seen timely by the network before publishing their payload to avoid unbundling attacks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exactly!
validator-flow.md
Outdated
| This requires registering with builder-specific withdrawal credentials (`BUILDER_WITHDRAWAL_PREFIX`). | ||
|
|
||
| Building: | ||
| 1. [Fetch ExecutionPayloadBid](#/Validator/getExecutionPayloadBid) from beacon node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or should be just not be opinionated on when to submit the bid?
maybe that's the way to go, even though the spec states the following
Validators are still expected to propose
SignedBeaconBlockat the beginning of
any slot
but realistically you are not gonna pick the best bid at t=0 of the slot but rather use the available time you have to collect a better bid, so very likely competitive bids will not even be published at the start of the slot but close to the ATTESTATION_DUE_BPS_GLOAS deadline (currently 3 seconds into the slot)
77d808b to
92769ae
Compare
92769ae to
76165b8
Compare
| value: | | ||
| event: data_column_sidecar | ||
| data: {"block_root": "0xcf8e0d4e9587369b2301d0790347320302cc0943d5a1884560367e8208d920f2", "index": "1", "slot": "1", "kzg_commitments": ["0x1b66ac1fb663c9bc59509846d6ec05345bd908eda73e670af888da41af171505cc411d61252fb6cb3fa0017b679f8bb2305b26a285fa2737f175668d0dff91cc1b66ac1fb663c9bc59509846d6ec05345bd908eda73e670af888da41af171505"]} | ||
| execution_payload_available: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need one event stream for execution payload, bid and payload attestation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the epbs consensus specs are in good shape, we can formalize the beacon api endpoints.
Changes
Updated:
POST /eth/v2/beacon/blocksBeaconBlockto the networkNew:
BeaconBlockGET /eth/v4/validator/blocks/{slot}BeaconBlockso it can be signedblobs/kzg_commitmentsanymoreexecution_payload_valueandexecution_payload_blindedin the responseExecutionPayloadBid
GET /eth/v1/validator/execution_payload_bid/{slot}/{builder_index}POST /eth/v1/beacon/execution_payload_bidExecutionPayloadEnvelope
GET /eth/v1/validator/execution_payload_envelope/{slot}/{builder_index}response
POST /eth/v1/beacon/execution_payload_envelopePTC
POST /eth/v1/validator/duties/ptc/{epoch}GET /eth/v1/validator/payload_attestation_data/{slot}POST /eth/v1/beacon/pool/payload_attestationsGET /eth/v1/beacon/pool/payload_attestationsTBD
DataColumnSidecarby whoever's bid is included in the consensus block or perhaps this can be triggered after the call to release aSignedExecutionPayloadEnvelopemay need to later separate theGET /eth/v1/validator/payload_attestation_data/{slot}andPOST /eth/v1/beacon/pool/payload_attestationsinto two endpoints if we end up doing dual-ptc deadlines, which would result in separate payload attestation objects, one forpayload_presentand another forblob_data_available