Produce block v4 with payload - #580
Conversation
| - Validator | ||
| - ValidatorRequiredApi | ||
| parameters: | ||
| - name: slot |
There was a problem hiding this comment.
while implementing this, I noticed the slot alone is not definitive enough (eg. in case of forks), we should consider also passing the beacon_block_root here
There was a problem hiding this comment.
hm, what do you think about making it optional to pass beacon_block_root like in 0c45859? pretty sure the VC already computes the block's tree hash root during signing, so passing it here should be straightforward
There was a problem hiding this comment.
Yes, we do already compute the block root on the vc side. I am not a big fan of optional parameters, so I'd rather have it as required or remove it if it's not needed, so far it makes our implementation simpler, maybe get more feedback from others who already implemented the apis
There was a problem hiding this comment.
I feel you, perhaps @eserilev @tersec @StefanBratanov or others from your teams have some thoughts here
There was a problem hiding this comment.
Yes, prefer required to optional fields.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| description: "Slot for which the execution payload envelope is requested." | ||
| schema: | ||
| $ref: "../../beacon-node-oapi.yaml#/components/schemas/Uint64" | ||
| - name: builder_index |
There was a problem hiding this comment.
Will this endpoint be used for all builders outside of self building? I think the pipeline might endup being very different which makes me question if we would ever be caching more than just the current slot's payload envelope. terence asked this internally and I didn't have a good answer.
There was a problem hiding this comment.
this endpoint is only meant for self-building if you are the proposer or the "act as a builder" mode where you build the payload for another proposer
builders that produce payloads outside of the beacon node would get their own endpoint which would be under the /builder namespace (at least that's how I am thinking about it right now)
it should be sufficient to only cache the current slot as you consume the cache within the same slot, maybe if that's the case my comment here #580 (comment) is also not needed but I feel like the block root is useful to specify, not just the slot
There was a problem hiding this comment.
After thinking about it more, maybe we keep the current apis under /validator to be only used by proposers, which means we can remove builder_index here as it's only used for self-building
There was a problem hiding this comment.
makes sense, removed builder_index in 0c45859
There was a problem hiding this comment.
so looks like we might wanna use this for builders too if the validator "acts as a builder"
worth revisiting if we wanna re-add the builder_index here
Edit:
we might be fine, I think it's worth to wait until someone actually implement the "act as a builder" flow e2e, I rather tend towards not mixing up apis required by validators and builders too much
see discord for conversation about this
Notable changes
- implement epbs stateful block production (self-build only)
- `GET /eth/v4/validator/blocks/{slot}` added
- `GET
/eth/v1/validator/execution_payload_envelope/{slot}/{beacon_block_root}`
added
- `POST /eth/v1/beacon/execution_payload_envelope` added
- `POST /eth/v2/beacon/blocks` updated
- implement envelope state root computation
- update block production cache for gloas (includes state root)
- add validator block production flow for gloas
- add envelope signing to validator store / remote signer
- data column sidecar changes required to wire up new gloas type
- update beacon-api spec to `v5.0.0-alpha.0`
see ethereum/beacon-APIs#580 for reference
| * if `exec_node_payload_value >= builder_boost_factor * (builder_payload_value // 100)`, | ||
| then return a full (unblinded) block containing the execution node payload. | ||
| * otherwise, return a blinded block containing the builder payload header. |
There was a problem hiding this comment.
I don't think this applies with gloas anymore. Regardless of whether we self-build or fetch a bid from a builder. We will get back a SignedExecutionPayloadBid in the beacon block. In the case of the builder, the envelope won't be available with the validator.
There was a problem hiding this comment.
this needs to be rephrased for gloas, I think I brought that up in the other PR as well
There was a problem hiding this comment.
made minimal changes to the description to remove the blinded/unblinded references in 8bf9080
agreed builder_boost_factor needs to be rethought for gloas. we discussed briefly in the old PR here and briefly in the discord
I also found this builder-specs PR defining how BN can set preferences between external bids/p2p bis/VC bids
The question is whether we want to be able to override these BN preferences via the VC. I'm sure the answer is yes, and the question becomes "how"?
Would we want to modify the validator registrations endpoint to signal a VC's per builder bid boost preferences similar to as proposed in the builder spec PR? Pretty sure this POST fires every epoch or when one of the values change
This could replace builder_boost_factor for example
Open to other ideas as well
There was a problem hiding this comment.
Would we want to modify the validator registrations endpoint to signal a VC's per builder bid boost preferences similar to as proposed in the builder spec PR? Pretty sure this POST fires every epoch or when one of the values change
We want to get rid of this endpoint, by default, there won't be any registrations or builder preferences submitted to the beacon node, we only submit proposer preferences.
I left a comment related to this here #580 (comment). In my opinion, the beacon-api spec should have no notion of per builder configuration. The builder_boost_factor is still useful for the validator client to signal to the beacon node how it should decide on whether to self-build or pick a bid from the p2p pool, and the bid from the p2p pool is simply the one with the highest value.
There was a problem hiding this comment.
agree on keeping builder_boost_factor and not introducing per-builder config in the beacon-api. but i'm not sure the pre-Gloas formula carries over cleanly
pre-Gloas, the BN got exec_node_payload_value from its paired EL and compared it against builder_payload_value
in Gloas the bid's value field is the builder→proposer payment, and the spec mandates that self-builds set bid.value to zero. so if we keep the formula local_bid_value >= builder_boost_factor * (external_bid_value // 100) using bid value fields:
factor=0→ returns local (0 ≥ 0)factor=100→ returns external (0 ≥ external_value is false)factor=2**64-1→ returns external
100 and 2**64-1 seem to collapse to the same meaning
do we still expect the BN to somehow make factor=100 (profit max) meaningful? if yes, the formula stays but we should say so explicitly. if no, maybe we switch builder_boost_factor to a boolean like prefer_local=true|false
There was a problem hiding this comment.
just to clarify this, we wanna compare the local payload value vs. the builder bid (the highest bid we have, could even include bids from api), the changes to builder_boost_factor description should be minimal, it isn't really different from what we do today, let me know @shane-moore if that clears things up
There was a problem hiding this comment.
thanks, that clears it up. Done in 4114350. Preconditions now read "has at least one viable builder ExecutionPayloadBid available" as to cover p2p and builder api derived bids. Open to any other tweaks!
|
|
||
| Servers should use saturating arithmetic or another technique to ensure that large values of | ||
| the `builder_boost_factor` do not trigger overflows or errors. If this parameter is | ||
| provided and the beacon node is not configured with a builder then the beacon node MUST |
There was a problem hiding this comment.
I guess with ePBS, the beacon node is always configured with a builder as it listens to the builder bids on the gossip topic anyways.
There was a problem hiding this comment.
not necessarily, we currently have an option to only produce local blocks which I intend to keep, but the default behavior will likely change with gloas to either use bid from p2p or build local, there are still valid reasons to force the node to build a local block, eg. builders censoring, which especially until FOCIL is shipped requires local blocks or a really sophisticated logic in clients to filter out bids from censoring builders
| description: | | ||
| Instructs the beacon node to broadcast a signed execution payload envelope to the network, | ||
| to be gossiped for payload validation. A success response (20x) indicates | ||
| that the envelope passed gossip validation and was successfully broadcast onto the network. |
There was a problem hiding this comment.
In publishBlockV2 we return 202 if block passed gossip validation but failed integration.
Are we doing something similar for publishExecutionPayloadEnvelope?
There was a problem hiding this comment.
I guess from another angle to think about this is we broadcast the payload regardless the result of the payload integration, or we integrate first, if it goes well, we then gossip?
There was a problem hiding this comment.
We probably want to propagate the envelope as quickly as possible, so ideally:
- Apply gossip validation
- Broadcast
- Run
process_execution_payloadand import to DB
If (1) fails, we 400. If (3) fails we should probably 202 just like for blocks.
I was hoping we could avoid the nastiness of broadcast_validation for envelopes, but I think there might actually be an unbundling attack (background) for envelopes now:
I think there is an unbundling attack something like:
- Malicious proposer publishes block
Aat slotNcommitting tobidAandpayloadA, wherebidAcomes from an external builder (could be P2P or otherwise). - Malicious proposer publishes block
B(slashable) at slotNcommitting tobidBand payloadpayloadB. They will be slashed at the next slot. - Builder for
bidAstarts to publishpayloadA. Even on beacon nodes that have seen the equivocating block, the payload will PASS gossip validation as long as block A was imported. - Proposer publishes
payloadBfor blockB. Envelope gossip validation passes forpayloadBbecause it is signed by a different builder frompayloadA. - Result: either A or B could become head, depending on timing. If B wins, the builder of A has revealed potentially valuable information, which could be worth the proposer slashing themselves for (they could use it for a self-build in the next slot).
Proposed fixes:
- We could put the burden on the builder to check for block equivocations prior to revealing their payload (no API/spec changes).
- We could add a gossip condition to reject envelopes for slashable blocks.
- We could add a
broadcast_validation=equivocationflag (like we have for pre-Gloas blocks) to check that the envelope's block is not an equivocation. This flag would likely be used by all builders when interacting with their BNs. - We could make the block equivocation check a default part of the API (same as
broadcast_validation=equivocationfrom fix 3, but no flag required and no ability to opt-out).
There was a problem hiding this comment.
There was a problem hiding this comment.
I'm just catching up on the discussions here, but I think the broadcast_validation flag and its values make sense to me.
If im understanding correctly, the broadcast_validation =consensus_and_equivocation value on the publish_block endpoint will no longer be useful for unbundling protection? Probably no harm to keep it though.
There was a problem hiding this comment.
If im understanding correctly, the broadcast_validation =consensus_and_equivocation value on the publish_block endpoint will no longer be useful for unbundling protection?
no, it's not needed for this anymore, we could discuss if we wanna deprecate broadcast_validation on publishBlockV2, but might still be useful to have basic checks before publishing the block
Notable changes
- implement epbs stateful block production (self-build only)
- `GET /eth/v4/validator/blocks/{slot}` added
- `GET
/eth/v1/validator/execution_payload_envelope/{slot}/{beacon_block_root}`
added
- `POST /eth/v1/beacon/execution_payload_envelope` added
- `POST /eth/v2/beacon/blocks` updated
- implement envelope state root computation
- update block production cache for gloas (includes state root)
- add validator block production flow for gloas
- add envelope signing to validator store / remote signer
- data column sidecar changes required to wire up new gloas type
- update beacon-api spec to `v5.0.0-alpha.0`
see ethereum/beacon-APIs#580 for reference
**What type of PR is this?**
Feature
**What does this PR do? Why is it needed?**
implements the new GET /eth/v4/validator/blocks/{slot} endpoint, we
don't hook up the validator client to use it yet for post gloas in this
pr.
**Which issues(s) does this PR fix?**
Fixes ethereum/beacon-APIs#580
**Other notes for review**
**Acknowledgements**
- [x] I have read
[CONTRIBUTING.md](https://github.com/prysmaticlabs/prysm/blob/develop/CONTRIBUTING.md).
- [x] I have included a uniquely named [changelog fragment
file](https://github.com/prysmaticlabs/prysm/blob/develop/CONTRIBUTING.md#maintaining-changelogmd).
- [x] I have added a description with sufficient context for reviewers
to understand this PR.
- [x] I have tested that my changes work as expected and I added a
testing plan to the PR description (if applicable).
The GET description said the blinded form replaces transactions/withdrawals/ block_access_list with their HTRs, but BlindedExecutionPayloadEnvelope replaces the whole payload field with a single payload_root. Align the prose with the type. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
folded in shane-moore#10, which adopts a |
|
@nflaig intentional to close this? there isnt a v4/blocks in the dev instance... |
|
I don't think this was intentional lol i will reopen |
james-prysm
left a comment
There was a problem hiding this comment.
I'm voting ship it, i think it now covers everything, please don't merge until at least another person reviews
ok this is crazy, yesterday I rebased my prysm fork and pushed the new commits which include James' commit that has the |
|
I left some comments about |
Hey folks, I'm OoO til July 4th getting married 🤵👰 Glanced at #620, and I had similar concerns about builder_boost_factor. I leave it to y'all to decide whether to merge this PR as is and handle #620 as a follow-up or reach consensus on #620 approach first and then someone pushes an impl to this PR in my absence. Have no strong preference either way |
|
I'd rather merge this quickly (and also try to reach consensus on 620 quickly). Congratulations on the wedding! |
|
Ok let's just merge this and address |
nflaig
left a comment
There was a problem hiding this comment.
LGTM, great work @shane-moore and congrats on the wedding!
…ainLabs#16960) **What type of PR is this?** Feature **What does this PR do? Why is it needed?** resolves one of our todos which is default to gossip validations on rest apis for envelope: spec found here ethereum/beacon-APIs#580 This approach takes the shortest path which is just reapplying the steps but a greater refactor is needed to share the actual gossip validation function as something shared. ``` participants: # Stateless VC: requests block + envelope inline (include_payload=true) and # publishes SignedExecutionPayloadEnvelopeContents (blobs + KZG proofs). - el_type: ethrex el_image: ethpandaops/ethrex:glamsterdam-devnet-4 el_extra_params: - --http.api=eth,net,web3,admin cl_type: prysm cl_image: gcr.io/offchainlabs/prysm/beacon-chain:latest vc_image: gcr.io/offchainlabs/prysm/validator:latest supernode: true count: 1 validator_count: 32 cl_extra_params: - --subscribe-all-subnets - --verbosity=debug vc_extra_params: - --enable-beacon-rest-api - --stateless - --verbosity=debug # Stateful VC: BN caches the envelope (include_payload=false) and the VC # publishes the spec-wire SignedBlindedExecutionPayloadEnvelope. - el_type: ethrex el_image: ethpandaops/ethrex:glamsterdam-devnet-4 el_extra_params: - --http.api=eth,net,web3,admin cl_type: prysm cl_image: gcr.io/offchainlabs/prysm/beacon-chain:latest vc_image: gcr.io/offchainlabs/prysm/validator:latest supernode: true count: 1 validator_count: 32 cl_extra_params: - --subscribe-all-subnets - --verbosity=debug vc_extra_params: - --enable-beacon-rest-api - --verbosity=debug network_params: fulu_fork_epoch: 0 gloas_fork_epoch: 2 seconds_per_slot: 6 genesis_delay: 40 additional_services: - dora - spamoor # spamoor generates load so payloads are non-empty: eoatx for normal EL txs, # blobs to exercise the blob/KZG path through the stateless envelope Contents flow. spamoor_params: spammers: - scenario: eoatx config: throughput: 10 - scenario: blobs config: throughput: 2 global_log_level: debug ``` **Which issues(s) does this PR fix?** Fixes # **Other notes for review** **Acknowledgements** - [x] I have read [CONTRIBUTING.md](https://github.com/prysmaticlabs/prysm/blob/develop/CONTRIBUTING.md). - [x] I have included a uniquely named [changelog fragment file](https://github.com/prysmaticlabs/prysm/blob/develop/CONTRIBUTING.md#maintaining-changelogmd). - [x] I have added a description with sufficient context for reviewers to understand this PR. - [x] I have tested that my changes work as expected and I added a testing plan to the PR description (if applicable).
#8828 Align the gloas execution payload envelope `GET` with beacon-APIs [PR 580](ethereum/beacon-APIs#580): key it by `{slot}/{beacon_block_root}`. The endpoint returns the cached envelope only when its root matches the request, and the VC fetches using the root of the block it produced. Co-Authored-By: shane-moore <skm1790@gmail.com>
| blobs: | ||
| $ref: "../deneb/block_contents.yaml#/Deneb/Blobs" | ||
|
|
||
| BlindedExecutionPayloadEnvelope: |
There was a problem hiding this comment.
- will be removed in remove BlindedExecutionPayloadEnvelope #624 in case anyone is looking at the diff here to implement this
While looking at the implementation of #580 I realized using a `BlindedExecutionPayloadEnvelope` adds quite some complexity and I don't think it's warranted to add it to the spec - (1) requires a second signing route for the same signature, validator clients, remote signers and DVT middleware would all have to support this - (2) might be implementation specifc but at least for us having a object without a slot adds some complexity, initially when we discussed adding `BlindedExecutionPayloadEnvelope`, the slot was still a top-level field but we moved it to `payload.slot_number` so it's not part of the blinded object anymore - (3) on top of that, it requires to maintain a new type and adds more complexity to caching and reconstruction on the beacon node side during publishing, which adds more failure modes to a hot path, not ideal There is probably more, I stopped the implementation when I realized using a blinded object was a bad idea, as it also just optimizes not having to send `transactions` and `block_access_list` between VC<>BN, everything else in the envelope is a few hundred bytes. While in theory, this seems like the ideal way to do it from a performance perspective (you could even just send the root), I don't think it makes sense as it's rather a micro optimization compared to not sending around the blobs and proofs, which this PR still retains, and previous concerns from #519 are still being addressed in gloas. **Note:** This PR includes several smaller follow-ups to #580 that are opinionated, please take a look at my comments below with further rationale, I also just polished a few descriptions here and there, shouldn't be too controversial
Part of #8828 for the stateful path. Aligns `produceBlockV4` and `POST /eth/v1/beacon/execution_payload_envelopes` with beacon-APIs [#580](ethereum/beacon-APIs#580) and [#624](ethereum/beacon-APIs#624). - Request stateful block production with required `include_payload=false` - Submit `SignedExecutionPayloadEnvelope` with required `Eth-Consensus-Version` and `Eth-Blob-Data-Included: false` headers - Support `broadcast_validation` (`gossip`, `consensus`, `consensus_and_equivocation`) - Return `202` when the envelope is broadcast but not fully imported Co-Authored-By: shane-moore <skm1790@gmail.com>
Introduces the block production and execution payload envelope endpoints for post-Gloas (ePBS) forks. These were separated out from #552 to be discussed and reviewed independently. Prior discussion in PR 552 thread and shane-moore/beacon-APIs#2 as well.
New endpoints:
GET /eth/v4/validator/blocks/{slot}— Post-Gloas block production endpoint (produceBlockV4). Supports aninclude_payloadquery parameter that controls whether the execution payload envelope and blobs are returned inline (stateless multi-BN operation) or cached by the beacon node for separate retrieval (stateful single-BN operation).GET /eth/v1/validator/execution_payload_envelopes/{slot}/{beacon_block_root}— Fetch the cached execution payload envelope for the current proposer's self-built payload, returned as aBlindedExecutionPayloadEnvelope(the minimal object the validator client signs over).beacon_block_rootis a required path param so the lookup is re-org resistant. Only available for the current slot; returns 404 for past slots or when no envelope has been cached. Used in stateful mode.POST /eth/v1/beacon/execution_payload_envelopes— Publish a signed execution payload envelope. The request body is selected by theEth-Execution-Payload-Blindedheader:falsecarriesSignedExecutionPayloadEnvelopeContents(envelope + blobs + KZG proofs, stateless flow);truecarries aSignedBlindedExecutionPayloadEnvelopethat the beacon node reconstructs from its cache before broadcasting (stateful flow).Blinded execution payload envelope
Folds in shane-moore#10, resolving the earlier open question on whether a blinded envelope should be used in the stateful flow.
BlindedExecutionPayloadEnvelopereplaces thepayloadfield withpayload_root(the hash tree root of the full execution payload). Its hash tree root matches the fullExecutionPayloadEnvelope, so a validator signature over the blinded form is valid against the full envelope. This keeps the object the VC signs minimal in the stateful flow, where the beacon node already has the full payload and blobs cached from block production. Discussion: r3310000141, r3277279908.