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 12 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
There was a problem hiding this comment.
I do not feel strongly about removing it, also I think when I commented there was in addition a Eth-Proxy-Url header which is now gone, so this single header, if useful for DVT teams, might be worth keeping
| 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
| 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
selection Reference the Builder API containers at a pinned commit with anchors instead of restating them (will need to bump pin later); BuilderEntryV1 has no Builder API definition, so it stays defined here and now says why. Bid selection: all candidates are compared in Gwei and the highest boosted value wins. Behaviour: the beacon node MAY consider a p2p bid rather than always doing so; an entry the beacon node cannot use never fails the request; each entry's auth.message.slot is the slot the request is for. Also: revert the accidental dictionary.dic edit, add the new endpoint and the Eth-Builder-Url header to CHANGES.md, and document the builder-bid path in validator-flow.md.
| A per-builder input the validator client supplies on a block-production request, one entry per | ||
| builder-API bid request the beacon node should make. `auth` is the | ||
| [`SignedRequestAuthV1`](https://github.com/ethereum/builder-specs/blob/78a5546d9d8253beabf7db8baf988a58abdec87f/specs/gloas/validator.md#signedrequestauthv1) object from the Builder API Gloas spec. |
There was a problem hiding this comment.
the previous builder_boost_factor query param also applied to p2p bids, how does this work using the BuilderEntry mechanism?
we should definitely have a way to apply builder_boost_factor (and min_bid) globally and at the same time it would be good if BuilderEntry can also be applied to p2p builders either by index or pubkey instead of url
semantics New Eth-Execution-Payload-Source response header (local, builder, p2p). A self-built block with include_payload=false and a p2p bid win were indistinguishable: both return a bare BeaconBlock with no Eth-Builder-Url, but the validator publishes the envelope itself on the first and must not on the second. Defaults: default_min_bid and default_builder_boost_factor query parameters cover bids that match no BuilderEntry. No default value is asserted. Bid selection is one rule over both channels: reject on min_bid or max_execution_payment, boost every survivor, the highest boosted bid competes with the local build, and the local build wins a tie. Entries: url and builder_pubkey act independently, so the three legal combinations are enumerated rather than left to inference.
| `builder` when a builder-API bid won, and `p2p` when a p2p bid won. Required in response so | ||
| client can determine whether it must publish the execution payload envelope itself. |
There was a problem hiding this comment.
Required in response so client can determine whether it must publish the execution payload envelope itself.
this shouldn't be required, you can check bid.builder_index === BUILDER_INDEX_SELF_BUILD to figure out if it's self-build and whether or not you need to publish the execution payload envelope yourself
is there another egde case I am missing? generally for the validator client it shouldn't matter from where the builder bid is sourced
aside that, I kinda like the idea of this header, it's useful metadata, so I am not really opposed to adding it, could be a separate pr though, I don't really see how it's related to this one
`builder_pubkey` does two different jobs depending on whether the entry has a `url`: on a url entry it filters the returned bid (a builder-API bid not signed by it is rejected), on a url-less entry it names the builder whose p2p bids the entry applies to. The old rule gave an entry one combined identity and forbade a `builder_pubkey` appearing twice, which was wrong both ways: it rejected a builder bidding into two relays (two entries naming it), and accepted two entries sharing a url and auth but expecting different pubkeys, which produce identical requests. Split into two scoped rules. An entry with a url is a bid request, unique by (url, auth.data); several may share a url with different data. An entry with no url supplies p2p policy, unique by builder_pubkey. p2p policy now comes only from url-less entries, so p2p matching is scoped to them and the requestBody note follows suit.
default_builder_boost_factor values, so drop the two query parameters.
The opening paragraph still called the request body a list of BuilderEntry objects; the schema and the rest of the description already describe a BuilderConfig. Update the intro to match.
| max_execution_payment: | ||
| allOf: | ||
| - $ref: "../primitive.yaml#/Gwei" | ||
| - description: "Maximum trusted execution-layer payment, in Gwei, accepted from this builder. A bid promising more MUST be rejected." |
There was a problem hiding this comment.
Another option for valuing a bid is simply:
total value = bid.value + min(bid.execution_payment, builder_entry.max_execution_payment)
So we wouldn't have to reject bids that offer more. This is entirely possible if the builder simply didn't receive our BuilderPreferences.
There was a problem hiding this comment.
this should be documented in the builder-specs though? I personally don't think the beacon-api should be aware of how to use max_execution_payment at all other than it being a field that exists and that it is passing around, and sure, the description is also fine to explain the field, but the rest should be part of builder-specs
we can keep in-protocol bid selection values, like min-bid or builder boost factor documented here, but max_execution_payment seems entirely off-protocol to me
There was a problem hiding this comment.
this should be documented in the builder-specs though?
it's also described there but the BN needs to ultimately decide the winning bid. That means enforcing the max_execution_payment constraint on builder api bids. Otherwise it would need to be enforced at something like a sidecar level or the constraint itself is moot.
There was a problem hiding this comment.
it's also described there but the BN needs to ultimately decide the winning bid
beacon nodes implement the builders-specs since The Merge, or I guess most of it, all the apis are implemented at least otherwise you can't even talk with mev-boost or other sidecars today. the point is that how we select off-protocol bids doesn't need to be defined in the beacon-APIs spec, that doesn't have any implication though on whether or not the beacon node does something, if what you are suggesting is true, we would have to move everything from the builder-specs that is not related to builders into this repo or the consensus-specs, but it's kept separately and this is by intend, the builder-specs are optional, while beacon-apis is not from a client implementers perspective
There was a problem hiding this comment.
I'll trim the max_execution_payment description to just what the field is and leave its selection semantics to the builder-specs
| MAX_BUILDER_URL_SIZE = 2048 | ||
|
|
||
| class BuilderEntryV1(Container): | ||
| url: ByteList[MAX_BUILDER_URL_SIZE] # UTF-8 bytes of `url`, exactly as in JSON |
There was a problem hiding this comment.
so in json the url would be hex-encoded? I guess this doesn't need to be human-readable so should be fine, I do think we should make sure to support ssz for everything that is transmitted over the wire from the validator --> beacon and from beacon --> builder
| min_bid: | ||
| allOf: | ||
| - $ref: "../primitive.yaml#/Gwei" | ||
| - description: "Minimum total payment, in Gwei, accepted from a bid that matches no entry, which in practice is a p2p bid. A bid whose `value` plus `execution_payment` is below this MUST be rejected." | ||
| example: "10000000" | ||
| builder_boost_factor: | ||
| allOf: | ||
| - $ref: "../primitive.yaml#/Uint64" | ||
| - description: "Percentage multiplier applied to a bid that matches no entry, exactly as a `BuilderEntry`'s `builder_boost_factor` is applied to a bid from that entry." | ||
| example: "100" |
There was a problem hiding this comment.
so it should only be min_bid and builder_boost_factor that are applicable as global parameters. what I am wondering if it's in that case best to keep them here, or add them as query parameters, technically both works, and it might be nice to keep everything related to bid selection in one place, so we keep the query params for operational stuff only
4 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