Update Beacon API for parity with Builder API and KeyManager API#630
Update Beacon API for parity with Builder API and KeyManager API#630JasonVranek wants to merge 5 commits into
Conversation
| description: | | ||
| An optional proxy URL. When set, the beacon node sends the request to this URL instead of | ||
| to `url`, and sets the `Eth-Builder-Url` header to `url` so the proxy can forward the | ||
| request to the intended builder. |
There was a problem hiding this comment.
can you explain what's the use case of this, what's the purpose of the proxy in this setup and why can't the CL call the builder directly?
There was a problem hiding this comment.
updated the language in f8d2943
An optional proxy URL for reaching this builder. Some deployments place a sidecar between
the beacon node and its builders; whenproxyis set the beacon node sends the request to
this URL and sets theEth-Builder-Urlheader tourlso the sidecar can forward it to the
intended builder. Whenproxyis unset the beacon node contacts the builder directly at
url.
There was a problem hiding this comment.
that doesn't really answer my question, what I wanna know is what does the proxy do in addition to the CL client in gloas when it comes to the PBS pipeline and why should be enshrine the concept of such a proxy into the protocol?
There was a problem hiding this comment.
the plan post-gloas is for the BN to do muxing and bid selection. If we assume those are the only roles the builder-facing layer will ever need, the proxy is unnecessary, but I don't think that assumption holds. PBS itself came from out-of-protocol experimentation, not bc the protocol predicted it.
The BN exposes a fixed bid selection policy using min_bid, max_execution_payment, builder_boost_factor (at least in this PR). Out-of-protocol services may have different policy reqs beyond that set, e.g. (acknowledging my bias) commit-boost lets a proposer make commitments, so the winning bid must also be commitment-honoring, which the BN knobs can't express but a proxy in the request path can.
These out-of-protocol experiments have a chicken and egg problem. To eventually become enshrined they have to run in the wild first, and if the only way to do that is forking a CL, it either kills the experimentation or incentivize forked clients. I'm advocating to keep the experimentation zone open (and ofc optional)
There was a problem hiding this comment.
PBS itself came from out-of-protocol experimentation, not bc the protocol predicted it.
well I don't think mev-boost should even exist as a sidecar, CL clients are already 90% there in terms of functionality since the merge, all we needed to add is multiplexing and bid selection
commit-boost lets a proposer make commitments
doesn't commit-boost need access to the bls keys of the proposer in that case anyways? so it could sign the auth request itself
but anyways, my concerns are addressed since we got rid of the concept of a proxy, while allowing such a "proxy" (whether that means relay or sidecar) to exist and work if operators wanna use it
| names a concrete builder `url`, and no two entries may share the same `url`. For each entry the | ||
| beacon node contacts the builder at its `proxy` if set, otherwise at its `url`; forwards the | ||
| entry's `auth` unchanged; and applies the entry's `max_execution_payment`, `min_bid`, and | ||
| `builder_boost_factor` when comparing its bid. Malformed entries are ignored rather than failing |
There was a problem hiding this comment.
this says ignore malformed entries but submit builder preferences has a hard 400 error, maybe those should be ignored too instead?
There was a problem hiding this comment.
submitBuilderPreferences is now explicitly best-effort/non-atomic to match produceBlockV4
| $ref: '../../beacon-node-oapi.yaml#/components/headers/Eth-Execution-Payload-Included' | ||
| Eth-Builder-Url: | ||
| description: | | ||
| For a block built on an external builder's bid, the `url` of the winning builder. The |
There was a problem hiding this comment.
should this be clarified that it's won via the builder api bid channel and not that an external builder won ?
or not that it's present only when builder api is used
| - name: Eth-Builder-Url | ||
| in: header | ||
| required: false | ||
| description: | |
There was a problem hiding this comment.
it's required false but maybe we should specify that we must echo this and eth-builder-proxy when available if returned by produceBlockv4 just to be explicit
| Maximum trusted execution-layer payment, in Gwei, accepted from this builder. Bids | ||
| promising a trusted payment above this value MUST be rejected. | ||
| example: "1000000000" | ||
| min_bid: |
There was a problem hiding this comment.
min_bid and builder_boost_factor are defined per entry, but they feed a single global decision (which bid wins the slot). How have you interpreted for this
Per-builder reading: each entry's values judge only that builder's bid.
Proposer-level reading: they express one overall policy — but then entries can disagree and a collapse rule is needed (first? max?).
hese pick different blocks from identical configs.
┌───────────────────────────────────────────────┬───────────────────────────┬───────────────────────────┬────────────────────┐
│ Interpretation │ A's test │ B's test │ Winner │
├───────────────────────────────────────────────┼───────────────────────────┼───────────────────────────┼────────────────────┤
│ Per-builder │ 90 × 100% = 90 < 100, out │ 60 × 200% = 120 > 100, in │ B (a 60-value bid) │
├───────────────────────────────────────────────┼───────────────────────────┼───────────────────────────┼────────────────────┤
│ Proposer-level, collapse = first entry (100%) │ 90 < 100, out │ 60 < 100, out │ local │
├───────────────────────────────────────────────┼───────────────────────────┼───────────────────────────┼────────────────────┤
│ Proposer-level, collapse = max (200%) │ 180 > 100, in │ 120 > 100, in │ A (best raw bid) │
└───────────────────────────────────────────────┴───────────────────────────┴───────────────────────────┴────────────────────┘
There was a problem hiding this comment.
my understanding was the per-builder reading. the keymanager API describes the normative resolution order from BuilderEntry → BuilderConfig → default_config → client default per field so by the time we're at the bid selection process, the BN should have received all of the per-builder resolved preferences. So there shouldn't be a notion of a "global" min_bid at that point, but earlier upstream the VC could have effectively resolved a global min_bid from its default_config. still the comparisons should all be per-builder. in other words the 'collapse rule' isn't needed bc it should have collapsed upstream at the VC upon keymanager config
if that makes sense I can make that more explicit in the spec
There was a problem hiding this comment.
sounds good interested what other cls think
| "200": | ||
| description: "The beacon node submitted the preferences to every builder." | ||
| "400": | ||
| description: "Errors with one or more preference submissions; the failing entries are identified by index. Well-formed entries are still submitted." |
There was a problem hiding this comment.
we should also say the duplicate thing here right? or two entries sharing the same url
| When the body is omitted or empty, or every entry is ignored, the beacon node contacts no | ||
| external builders and chooses between its local build and any p2p bid. | ||
| content: | ||
| application/json: |
There was a problem hiding this comment.
wait we should also allow ssz
proxy routing - BuilderEntry mirrors the keymanager config: url, auth, max_execution_payment, min_bid, and builder_boost_factor required, pubkey the sole optional verification pin; entries arrive fully resolved and the beacon node never substitutes defaults - no uniqueness rule here: multiple entries may share a url (the keymanager enforces the pair rule at write time), one getExecutionPayloadBid call per entry, malformed entries ignored - produceBlockV4 and submitBuilderPreferences accept SSZ request bodies: BuilderEntryV1 / BuilderPreferenceEntryV1 containers with MAX_BUILDER_ENTRIES (64) and MAX_BUILDER_URL_SIZE (2048) defined on the BuilderEntry type, all-zero pubkey means unset in either encoding, maxItems mirrors the SSZ cap, 415 for unsupported media types - bid evaluation made precise: min_bid floors the bid's total payment (value plus execution_payment), max_execution_payment caps the trusted component, and builder_boost_factor applies to the total after both checks - proxy fields and Eth-Builder-Proxy headers removed; the Eth-Builder-Url winner echo stays, with the stateless-setup rationale stated - request_auth data matches the builder-specs twin (UTF-8 URL default, whole-byte hex pattern); builder-API path parameter renamed to proposer_pubkey
| The body MAY be encoded as JSON (`Content-Type: application/json`) or SSZ | ||
| (`Content-Type: application/octet-stream`); the `BuilderEntry` list is not fork-versioned, so | ||
| no `Eth-Consensus-Version` header is required. |
There was a problem hiding this comment.
so we never plan to change BuilderEntry in the future?
also please make sure this somewhat follows how we document stuff in the beacon-api
eg. "MAY be encoded as JSON (Content-Type: application/json) or SSZ (Content-Type: application/octet-stream)", that seems completely unnecessary to point out, the spec already documents that and the content: section is enough
| validator publishes the signed block via `publishBlockV2`, echoing the `Eth-Builder-Url` header, | ||
| and the winning builder releases the execution payload envelope. For a self-built block, the | ||
| validator publishes the signed block via `publishBlockV2` and the execution payload envelope | ||
| via `publishExecutionPayloadEnvelope`. |
There was a problem hiding this comment.
this is quite verbose, the flow should be somewhat intuitive from a spec perspective, without having to overexplain it, really need to try and simplify the large text sections in this PR
| When using an external builder bid, only the `BeaconBlock` is returned as the beacon node | ||
| does not have access to the builder's execution payload. | ||
| Post-Gloas, proposers submit execution payload bids rather than full execution payloads, so | ||
| there is no longer a blinded or unblinded distinction; builders release the payload later. This |
There was a problem hiding this comment.
why does this mention anything about blinded or unblinded? sure it doesn't exist anymore but produce block v4 is post-gloas only
what's the intend behind pointing this out explicitly here?
There was a problem hiding this comment.
ah realized we had this note before, hmm I don't really like it, but out of scope for this PR in that case
| required: true | ||
| name: Eth-Consensus-Version | ||
| description: "The active consensus version to which the block being submitted belongs." | ||
| - name: Eth-Builder-Url |
There was a problem hiding this comment.
do we really need this header? if I understand correctly the sole purpose of this is so that the beacon node knows where to submit it via submitSignedBeaconBlock but the beacon node should know this anyways, since it got the bid from that builder via api
the only case where the beacon node doesn't know this is if it's a different node (eg. in a multi-node setup) if the primary node that sourced the bid from the builder via api went offline during the proposal flow but this seems like an edge case and even then, publishing only via p2p is a fine fallback behavior that is sufficient anyways most of the time
that said, we can keep the header if people wanna have it, just questionable to me if it's useful enough
There was a problem hiding this comment.
this was solely for the multi-node support, not opinionated on it but it seemed like DVT teams wanted. Will leave for now and address most of the fat trimming comments
| see. The proposer's private bid-filtering strategy (`min_bid` and `builder_boost_factor`) is | ||
| never sent to a builder; the beacon node applies those locally when selecting a bid. | ||
|
|
||
| SSZ request encoding. The `application/octet-stream` request body of the beacon-node |
There was a problem hiding this comment.
this is standard in the beacon-api, repeating this over and over is not necessary, please try to follow existing conventions, this will also help trim these large text sections a bit and make it easier to review
| the highest-value candidate across those, the local build, and any p2p bid wins; there is no | ||
| cross-entry policy to reconcile. Malformed entries are ignored rather than failing the | ||
| request, so a single bad entry never blocks block production. When the body is omitted or | ||
| empty, or every entry is ignored, the beacon node contacts no external builders and chooses |
There was a problem hiding this comment.
there is no cross-entry policy to reconcile
what does this mean?
Malformed entries are ignored rather than failing the request
what does malformed mean in that case, I am guessing the format should still be per schema, especially in case of ssz that must be the case otherwise it fails deserialization
There was a problem hiding this comment.
did another quick pass, I feel like mostly this needs to be trimmed a lot, if details regarding bid selection are already part of the builder-specs there is no need to repeat that here, I don't really think the beacon-api should care about this at all, the builder-specs is not part of the required specs, so we should avoid leaking so much off protocol details into the spec here, especially when it comes to configs like max_execution_payment, min bid and boost factor we can consider keeping documented, similar to how we had it for builder_boost_factor query param
| so there is no longer a concept of blinded or unblinded blocks. Builders release the | ||
| payload later. This endpoint is specific to the post-Gloas forks and is not backwards compatible | ||
| with previous forks. | ||
| The beacon node always builds a local payload and considers any p2p bid. The validator |
There was a problem hiding this comment.
The beacon node always builds a local payload and considers any p2p bid.
hmm I don't think this is the default behavior I want in lodestar, we have a option called executiononly on the validator client and I intend to respect that
I don't think the spec should be opinionated about this, there are valid reasons to not consider p2p bids, eg. a user that just prefers to build local, or for censorship resistance reasons
| `getExecutionPayloadBid` call to make; the beacon node adds those bids to the comparison and | ||
| returns the highest-value candidate. It biases each builder's bid by the entry's | ||
| `builder_boost_factor` and rejects a bid that falls outside the entry's `min_bid` (floor) or | ||
| `max_execution_payment` (ceiling on the trusted execution payment). `builder_boost_factor` leans |
There was a problem hiding this comment.
isn't that documented in the builder-specs already? max_execution_payment shouldn't be documented here
| When using an external builder bid, only the `BeaconBlock` is returned as the beacon node | ||
| does not have access to the builder's execution payload. | ||
| Post-Gloas, proposers submit execution payload bids rather than full execution payloads, so | ||
| there is no longer a blinded or unblinded distinction; builders release the payload later. This |
There was a problem hiding this comment.
ah realized we had this note before, hmm I don't really like it, but out of scope for this PR in that case
| Gloas: | ||
| RequestAuthV1: | ||
| type: object | ||
| description: "The `RequestAuthV1` object from the [Builder API](https://ethereum.github.io/builder-specs/) Gloas spec." |
There was a problem hiding this comment.
usually there is a proper reference to the object itself, at least when it comes to the consensus specs, I guess we kinda need a release of builder specs, could use master branch for now
| Instructs the beacon node to broadcast a newly signed beacon block to the beacon network, | ||
| to be included in the beacon chain. A success response (20x) indicates that the block | ||
| passed gossip validation and was successfully broadcast onto the network. | ||
| The beacon node is also expected to integrate the block into the state, but may broadcast it | ||
| 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. For Deneb/Electra/Fulu, this additionally instructs | ||
| the beacon node to broadcast all given blobs. For Gloas and later, blobs are broadcast as part | ||
| of the `ExecutionPayloadEnvelope` and are not submitted with the block. The broadcast behaviour | ||
| may be adjusted via the `broadcast_validation` query parameter. | ||
| Instructs the beacon node to broadcast a newly signed beacon block to the beacon network, to be | ||
| included in the beacon chain. A success response (20x) indicates that the block passed gossip | ||
| validation and was successfully broadcast onto the network. The beacon node is also expected to | ||
| integrate the block into the state, but may broadcast it 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. For | ||
| Deneb/Electra/Fulu, this additionally instructs the beacon node to broadcast all given blobs. | ||
| For Gloas and later, blobs are broadcast as part of the `ExecutionPayloadEnvelope` and are not | ||
| submitted with the block. The broadcast behaviour may be adjusted via the `broadcast_validation` | ||
| query parameter. |
There was a problem hiding this comment.
what's the diff here, it seems like just reformatting?
| class BuilderPreferenceEntryV1(Container): | ||
| url: ByteList[MAX_BUILDER_URL_SIZE] # UTF-8 bytes of `url`, exactly as in JSON | ||
| auth: SignedRequestAuthV1 | ||
| max_execution_payment: Gwei |
There was a problem hiding this comment.
there is no need to re-define containers here, ideally we wanna add a reference to the spec container
| description: | | ||
| An array of `BuilderPreferenceEntry` objects, one per preference submission the beacon node | ||
| should make. Each entry names the builder `url` to contact; multiple entries MAY share a | ||
| `url`, and the beacon node makes one submission per entry. Inclusion is best-effort: the |
There was a problem hiding this comment.
if the beacon node anyways makes one submission per entry, why do we need to submit on request per pubkey to the beacon node? we could add the validator pubkey to each entry and more closely align this api with POST /eth/v1/validator/proposer_preferences, so max items would be 64 * SLOTS_PER_EPOCH * 2
might be missing something, would need to implement this to know what's easier, but seems simpler on paper if the validator client only has to send a single request
There was a problem hiding this comment.
even submitBuilderPreferences should probably be reconsidered to allow an array and add pubkey to request body per entry. I know it's gonna be unlikely that this happens often on mainnet but implementation wise this seems simpler
3 participants
Builds on @potuz's #625. Since #625 predates #624, this rebases onto master to incorporate it and aligns the naming and conventions from ethereum/keymanager-APIs/pull/87 and ethereum/builder-specs/pull/165, so the three describe one proposer flow.
The beacon API hands the beacon node the parameters to make builder-API calls on the validator's behalf:
produceBlockV4takes an optionalBuilderEntryarray (one per external builder), renaming Add a POST produceBlockV4 #625'sBuilderPreferences/max_trusted_bidto the sharedBuilderEntry/max_execution_payment.submitBuilderPreferences(POST /eth/v1/validator/builder_preferences/{pubkey}) mirrors the builder-API endpoint so the validator pushesmax_execution_paymentahead of the slot; itsBuilderPreferenceEntryomits the privatemin_bid/builder_boost_factor.produceBlockV4returnsEth-Builder-Url/Eth-Builder-Proxy;publishBlockV2takes them back, so the beacon node forwards the signed block to the winner viasubmitSignedBeaconBlock.Note OffchainLabs/prysm#17124 has been a reference implementation