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 20 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
There was a problem hiding this comment.
There's an important nuance here..
by the time we're at the bid selection process, the BN should have received all of the per-builder resolved preferences … the comparisons should all be per-builder
That holds for bids the BN pulls directly via getExecutionPayloadBid — each arrives together with its entry, so the BN can rank by the per-builder boosted value. But I don't think it holds for p2p (gossip) bids, and it's the table's headline example that breaks.
Gossip bids are pre-filtered by the consensus-specs gossip rule
[IGNORE] this bid is the highest value bid seen for the tuple
(bid.slot, bid.parent_block_hash, bid.parent_block_root)
So a node retains only the single highest-value bid per tuple; lower bids are IGNOREd at validation and never cached. Now re-read the per-builder example assuming A and B are gossip bids for the same tuple:
| A | B | |
|---|---|---|
| raw value | 90 | 60 |
| boost | 100% | 200% |
| boosted | 90 | 120 → wins |
The table says B (the 60-value bid) wins. But under the gossip rule, B was IGNOREd the moment A (value 90) was seen for that tuple — it's never in the cache, so per-builder boost (which is only known later at proposal time) can't resurrect it. In other words, builder_boost_factor and min_bid can't reorder among gossip bids, because the gossip layer has already collapsed them to one before selection runs.
There was a problem hiding this comment.
Yes the boost is kinda irrelevant for p2p bids since they are filtered by nodes that don't know the proposer's value.
| "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.
| 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
| parameters: | ||
| - name: pubkey | ||
| in: path | ||
| required: true | ||
| description: | | ||
| The BLS public key of the proposing validator whose preferences these are. The beacon node | ||
| passes it as the `proposer_pubkey` of each builder-API preference submission, and each | ||
| entry's `auth` is signed by this key. |
There was a problem hiding this comment.
I left this comment before but can we please fix this api, I don't see a good reason why we would do a single call per pubkey. I don't believe there is a single beacon api that works like this, we generally do batching cc @rolfyone @james-prysm @tersec or anyone else that could have an opinion on this too
@JasonVranek can you please do the same on the builder-api, or check why we went with that api design, and also double check with builders
There was a problem hiding this comment.
since gloas gossips proposer preferences instead of flooding relays with validator registrations, and builder preferences are only sent for the proposers in the lookahead, this is realistically only an optimization for operators large enough to have >1 proposer in the lookahead, and even then it just saves a couple of http calls outside the hot path.
I'm happy to update the beacon API to batch and match existing conventions (will start on that now). I'd push back on changing the builder spec though. The BN already has to fan out per-builder to request bids, so batching preferences there doesn't save it any mechanism it isn't already building. also it makes error handing more confusing at VC since there'd be two levels of partial failures (some entries rejected at the BN, then some at each builder)
There was a problem hiding this comment.
this is realistically only an optimization for operators large enough to have >1 proposer in the lookahead
this is not about a performance optimization, it's about having a good api design, yes it's unlikely on mainnet that this is gonna be >1 but the validator client still needs to support this scenario, but this opens up several implementation details, do you send the request in sequence or parallel? if in sequence, if there is a http error, should you stop sending or keep going?
The BN already has to fan out per-builder to request bids
yes this is already bad enough, why do we want to make it per-builder per-pubkey?
also it makes error handing more confusing at VC since there'd be two levels of partial failures (some entries rejected at the BN, then some at each builder)
imo the error handling is strictly better, we already have an established pattern for this, see IndexedErrorMessage in the beacon-api spec, we use this for consensus critical apis like submitPoolAttestationsV2 and it works perfectly there
some entries rejected at the BN
I need to read up on this still, you might have documented that already, but what are rejections/error that can happen on the beacon node side? I would expect the beacon node to act as more of a proxy here, and then report back the failures/errors it gets from the builder requests, and only then send the response 200/400 back to the validator client, othwerise you have 0 visiblity from the vc side on what's going on
but generally, I don't really see what kind of error can happen here, the individual fields are defined in the schema and there shouldn't be many ways to produce a invalid request?
the failure cause I can see is a 401 if the authentication fails, but would you rather have one 401 from the builder or a 401 per-pubkey? I can see the argument here if pubkeys sign over different auth_data? maybe that is an argument to keep one request per pubkey on builder-specs?
but considering we already send batches to builders via registerValidator and it's working since ~4 years I don't see why that is a problem
I would probably just go with what builders/relays prefer here, they have to implement the server-side of this, I wouldn't care to much to add a for-loop on the beacon node side if they much prefer per-pubkey
There was a problem hiding this comment.
the failure cause I can see is a 401 if the authentication fails, but would you rather have one 401 from the builder or a 401 per-pubkey? I can see the argument here if pubkeys sign over different auth_data? maybe that is an argument to keep one request per pubkey on builder-specs?
answering my own question, I think on the beacon node --> builder request side it makes sense to send per-pubkey due to 401 http errors, added more details here ethereum/builder-specs#165
There was a problem hiding this comment.
sounds good, in the process of drafting:
- VC -> BN batching for preferences
- dropping *_V1 across the PRs in favor of fork-versioned types
note a byproduct of VC->BN batching is moving pubkey from the VC->BN path to the request body, so to simplify/share types I'm mirroring the change on the builder spec. So a BuilderPreferencesRequest gets a pubkey field and the submitBuilderPreferences builder api endpoint drops the proposer_pubkey path param. This is also a step towards enabling BN->builder batching but intentionally holding off on that change for now
There was a problem hiding this comment.
this works. I was considering reusing the BuilderEntry struct supplied in produceBlockV4 in the preferences endpoint to avoid defining a new intermediate type like BuilderPreferenceEntry. Idea was from the BuilderEntry you could create the BuilderPreferencesRequest the builder api needs. Also the builder api doesn't allow you to send things like min_bid so we structurally don't have to worry about leaking the private proposer config values. But bc of the ssz not allowing optional fields it actually turned out a bit messier/uglier than just defining the intermediate type.
why does how we submit it from vc --> bn have any impact on bn --> builder
yea not going to end up doing this. Will keep BuilderPreferencesRequest as is (except for the fork versioning change)
There was a problem hiding this comment.
I think it's fine to keep containers separate, maybe even desired. also the screenshots above, we probably wanna rename pubkey to validator_pubkey to avoid potential confusing with builder_pubkey
There was a problem hiding this comment.
we probably wanna rename pubkey to validator_pubkey to avoid potential confusing with builder_pubkey
going with proposer_pubkey to match the builder api's path params
There was a problem hiding this comment.
going with proposer_pubkey to match the builder api's path params
I would need to double check, it's sounds bit more unfamiliar to me than validator_pubkey but for the purpose of differentiating it works fine, so no strong preference
There was a problem hiding this comment.
The changes have been pushed across the 3 PRs:
- bb1e790 adds batched submitBuilderPreferences beacon api calls
- ethereum/builder-specs@36cefe4 adopts fork-versioned types
- ethereum/keymanager-APIs@655d731 is a minor diff to reference the fork-versioned type
- this is the version of the implementer guide pinned to these commits
| The `BuilderPreferenceEntry` objects to submit. Inclusion is best-effort: the beacon node | ||
| submits each entry independently and skips any entry it cannot submit rather than failing the | ||
| whole request. Entries that were not submitted are reported by index in a 400 response. | ||
| content: |
There was a problem hiding this comment.
this api is missing fork versioning, wouldn't it be good to keep the door open to easily update BuilderPreferenceEntryV1 in heze without having to bump the api? it's pretty painful to introduce a new api version outside of a hard fork, so might as well version via fork, especially a api like this
how to deal with this on the builder-api side can be different, but I would just add a fork version to leave the door open to add a new container in heze, there isn't much harm to add this metadata
There was a problem hiding this comment.
dropping *_V1 across the PRs in favor of fork-versioned types
@JasonVranek let's discussion the versioning here, so from my experience, using a V1 suffix isn't useful, it implies you need a new api version to change it? or how else does this work?
in my opinion and this has worked quite nicely so far is to use the Eth-Consensus-Version header, this allows pretty seamless updates to the containers across forks to meet new requirements, and notably, this doesn't prevent you from doing a v2 mid-fork, although, like I said, it's pretty hard to roll this out mid-fork, rolling out just ssz which is supposed to be backward compatible was a huge pain... so really even if you do a v2 api, you can mostly just rely on it in the next fork once all parties are forced to update their software
There was a problem hiding this comment.
in my opinion and this has worked quite nicely so far is to use the Eth-Consensus-Version header
yes I believe I'm doing this exactly, just to verify:
BuilderPreferencesRequestV1->BuilderPreferencesRequestEth-Consensus-Versionoptional -> required
There was a problem hiding this comment.
yes sounds good to me, having Eth-Consensus-Version optional anywhere should be considered a bug
The validator client submits a flat array of BuilderPreferencesEntry, one entry per builder per proposer, each with proposer_pubkey, url, auth and max_execution_payment. The beacon node returns 200, or 400 with an IndexedErrorMessage naming each failed entry, and fans out one builder-API submission per entry.
| 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: |
There was a problem hiding this comment.
same nit here, I would prefer to have global fields at the top of the container
| Controls whether the execution payload envelope and blobs are included in the response | ||
| when self-building (using local execution payload). | ||
| Whether a self-built block's response carries the execution payload inline, or omits it for | ||
| the beacon node to cache and the validator client to retrieve separately. | ||
|
|
||
| When `true`, the response includes the full block contents: beacon block, | ||
| execution payload envelope, blobs, and KZG proofs. This enables stateless operation | ||
| where the validator client can use multiple beacon nodes (multi-BN setups, distributed validators, failover). | ||
| When `true`, a self-built response includes the full block contents (beacon block, execution | ||
| payload envelope, blobs, and KZG proofs) and reports `Eth-Execution-Payload-Included: true`. | ||
| The validator client holds the whole payload, so it can sign and publish the envelope through | ||
| any beacon node (stateless operation: multi-BN setups, distributed validators, failover). | ||
|
|
||
| When `false`, only the beacon block is returned and the beacon node caches the execution | ||
| payload envelope and blobs internally. The validator client must then fetch them separately | ||
| via `GET /eth/v1/validator/execution_payload_envelopes/{slot}/{beacon_block_root}`. This saves | ||
| bandwidth but requires the validator client to publish via the same beacon node that | ||
| produced the block (stateful operation). | ||
| When `false`, a self-built response returns only the beacon block, reporting | ||
| `Eth-Execution-Payload-Included: false`; the beacon node caches the execution payload | ||
| envelope and blobs. The validator client retrieves the envelope via | ||
| `GET /eth/v1/validator/execution_payload_envelopes/{slot}/{beacon_block_root}`, signs it, and | ||
| publishes it via `POST /eth/v1/beacon/execution_payload_envelopes`, and the producing beacon | ||
| node attaches its cached blobs and KZG proofs before broadcasting. This sends fewer bytes but | ||
| ties publishing to that one beacon node (stateful operation). | ||
|
|
||
| This parameter only affects self-building scenarios. When using an external builder's bid, | ||
| only the beacon block is returned regardless of this parameter (the beacon node does not | ||
| have access to the builder's execution payload). | ||
| This parameter affects the self-built case only. When a bid wins, the beacon node does not | ||
| hold that payload, so it returns only the beacon block regardless of this parameter. |
There was a problem hiding this comment.
what exactly changed here?
This parameter affects the self-built case only. When a bid wins, the beacon node does not
hold that payload, so it returns only the beacon block regardless of this parameter.
this note seems useful
but also previously there was
included in the response when self-building
so if block includes a builder bid, this flag is ignored anyways
| - description: | | ||
| Percentage multiplier applied to this builder's bid once it has passed the | ||
| `min_bid` check, giving | ||
| `builder_boost_factor * (builder_payload_value // 100)`, where |
There was a problem hiding this comment.
hmm can you please revisit this, I did some cleanup here https://github.com/ethereum/beacon-APIs/pull/624/changes#diff-0bfdd4898edd7ab2e4cdefa66264cd4d0c4467ab5ff6cff00a3a8296b4274cbc, now in the pr we revert that, builder_payload_value is just the wrong term imo, can you please make sure this is simiarl to what we have on master branch, of course with the caveat that it's per-builder here so wording might be different
Order the global min_bid and builder_boost_factor before the builders list in BuilderConfig. Rename builder_payload_value to builder_bid_value to match the term cleaned up in ethereum#624. Revert the include_payload description to the master wording, keeping only the note that the flag is ignored when a bid wins.
Every BuilderEntry is now a bid request: url and auth are required, and an entry no longer supplies p2p policy for a builder's gossiped bids. The top-level min_bid and builder_boost_factor apply to p2p bids
| 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 MAY consider a p2p bid, so a block is returned | ||
| even when no bid is available. The validator client optionally supplies a `BuilderConfig` in the |
There was a problem hiding this comment.
nit: there are several places where we mention bid (i.e. here "no bid is available"). in the local build case a bid is still constructed. It might be useful instead to specify builder bid, for example
| even when no bid is available. The validator client optionally supplies a `BuilderConfig` in the | |
| even when no builder bid is available. The validator client optionally supplies a `BuilderConfig` in the |
there are multiple places in the PR where "bid" is assumed to be a builder bid. it might make things a bit clearer if we just explicitly say builder bid in those cases
There was a problem hiding this comment.
not disagreeing with adding "builder" here as suggested but referring to local payload as "local bid" or similar is really confusing, during block proposal you either use a bid from a builder or you commit to a local payload, the fact that the ExecutionPayloadBid is overloaded and used for both is mostly spec limitation, ideally this should be something like Union[ExecutionPayloadBid, ExecutionPayloadCommitment] where the commitment type used when self-building drops anything that is unnecessary for local payloads like builder_index, fee_recipient and value
There was a problem hiding this comment.
yeah i totally agree with not using the term local bid
| requestBody: | ||
| required: false |
There was a problem hiding this comment.
while this request body is not required, we should make the Eth-Consensus-Version header required, it's completely missing from the request right now
arguably it's only needed if BuilderConfig is passed in the request body, but I would make the header always required
also worth considering making passing BuilderConfig since it also applies to p2p bids, I don't see a good reason to omit this?
| builder_boost_factor: uint64 | ||
|
|
||
| SSZ cannot express absence, so an unset `builder_pubkey` is all-zero, which is not a valid | ||
| BLS public key. A zero-length `url` is equivalent to omitting it, so a body containing one is |
There was a problem hiding this comment.
url is required now? so the comment here seems outdated
| builder_pubkey: | ||
| description: "The builder's BLS public key. When present it filters the response: a builder-API bid not signed by it MUST NOT be accepted." |
There was a problem hiding this comment.
this should be required in json too so that ssz/json has same shape
builder_pubkey becomes builder_pubkeys, a List[BLSPubkey, MAX_BUILDER_PUBKEYS]: an empty list accepts any builder, and a bid MUST be signed by one of a non-empty list. Every BuilderEntry field is now required and the SSZ absence sentinels are gone. Also says builder bid where a builder bid is meant, per review.
Removes the last SSZ absence ambiguity: with the builder_pubkey sentinel gone, an empty data ByteList was the one remaining wire value that could read as "unset". It had no defined meaning either way, since omission already expresses derive from the builder's URL and auth is required on every request. Also state the zero-length url rule on BuilderPreferencesEntry.
|
Batch update across the three PRs, summarizing the recent review threads. Beacon-API changes are in eb752a1 through 71c7315, ethereum/keymanager-APIs#88 (d5b54fc through be0f46e), ethereum/builder-specs#165 (9a58e07 through a5552d6).
|
6 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