-
Notifications
You must be signed in to change notification settings - Fork 199
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
95e9511
to
ab847f5
Compare
ab847f5
to
d0ba3ba
Compare
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
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
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
Since the epbs consensus specs are in good shape, we can formalize the beacon api endpoints.
Changes
Updated:
GET /eth/v3/validator/blocks/{slot}
BeaconBlock
so it can be signedblobs/kzg_commitments
anymoreexecution_payload_value
andexecution_payload_blinded
from gloas variant in the responsePOST /eth/v2/beacon/blocks
BeaconBlock
to the networkNew:
ExecutionPayloadBid
GET /eth/v1/validator/execution_payload_bid/{slot}/{builder_index}
POST /eth/v1/beacon/execution_payload_bid
ExecutionPayloadEnvelope
GET /eth/v1/validator/execution_payload_envelope/{slot}/{builder_index}
response
POST /eth/v1/beacon/execution_payload_envelope
PTC
POST /eth/v1/validator/duties/ptc/{epoch}
GET /eth/v1/validator/payload_attestation_data/{slot}
POST /eth/v1/beacon/pool/payload_attestations
GET /eth/v1/beacon/pool/payload_attestations
TBD
DataColumnSidecar
by whoever's bid is included in the consensus block or perhaps this can be triggered after the call to release aSignedExecutionPayloadEnvelope
may need to later separate theGET /eth/v1/validator/payload_attestation_data/{slot}
andPOST /eth/v1/beacon/pool/payload_attestations
into two endpoints if we end up doing dual-ptc deadlines, which would result in separate payload attestation objects, one forpayload_present
and another forblob_data_available