add per-key builder configuration endpoints - #88
Conversation
Adds GET/POST/DELETE on /eth/v1/validator/{pubkey}/builders, letting an
operator configure which external builders a validator key sources bids
from, and with what per-builder limits.
An alternative to ethereum#87, which managed one atomic per-key document at
/eth/v1/validator/config and deprecated the fee recipient, gas limit and
graffiti endpoints. This branch is cut from master, so those three are
untouched, and each concern keeps its own endpoint.
POST replaces the key's list and requires at least one entry; DELETE
removes the configuration so the key follows the validator client's own,
exactly as a key that was never configured does. Entries are identified
by their (url, auth_data) pair, unique per key.
The entry describes what each value means to the operator. The bid
selection rule itself lives in beacon-APIs, since only the beacon node
can implement it.
| builder_pubkey: | ||
| description: | | ||
| The builder's BLS public key, distinct from the validator `pubkey` in the request path. When | ||
| set, it binds this builder's trusted execution payments to its on-chain key. _48-bytes, hex |
There was a problem hiding this comment.
it binds this builder's trusted execution payments to its on-chain key
I honestly have no idea what this "binding" means, what it is for, or when a Keymanager API client would want to submit this. What does the validator client do with this value? Can you make that clearer or point me somewhere where this is explained? Thanks!
There was a problem hiding this comment.
This came from Terrence's implementation. The idea was the signed auth lets a proposer authenticate to a builder. But a MITM (or even a relay) could return arbitrary bids. The builder_pubkey was another optional layer for the BN to validate that a bid came from a specific builder (and reject otherwise). Note this really only makes sense in the context of receiving a trusted bid.
I'm personally not too sure how much this would be used / strongly opinionated on keeping it, but the functionality makes sense to me.
There was a problem hiding this comment.
I took a look at the linked implementation, I think I understand now. So this is to protect against e.g. a relay returning a bid with an execution payment from a builder that the validator doesn't trust to perform that payment.
That only seems needed when using a combination of relay/proxy/sidecar + execution payments. (other forms of MITM don't seem relevant with https?).
It makes sense to me even though I share your concern of it not being widely used. The alternative to this would be to only allow execution payments for direct builder connections but I don't know how you'd enforce that in practice.
| The keymanager treats this value as opaque bytes: it only ever compares `auth_data` values | ||
| for equality, and never parses or inspects them. When no value has been agreed with the | ||
| builder out of band, the caller SHOULD set `auth_data` to the UTF-8 bytes of the builder's | ||
| own advertised URL, exactly as advertised, hex encoded: it is deterministic, distinct per | ||
| builder, and requires no coordination between the signers of a distributed validator. That | ||
| is purely a convention about what tooling writes into the field. |
There was a problem hiding this comment.
Wouldn't it be simpler/better to just treat this auth_data field as optional? I assume custom auth data will not be set in the vast majority of cases.
There was a problem hiding this comment.
The thinking was if the key manager API required fully resolved fields, it simplified everything downstream since the request auth could be required by beacon api -> builder api and remove the need to reason about the cases where it's absent. If everyone used the default auth data they could just supply auth_data = builder_url as part of the keymanager call.
some motivation was talking with @james-prysm here where it was hard to reason about how to resolve configs when VCs have different defaults + keymanager fields were all optional
There was a problem hiding this comment.
why does auth_data need to be required in the config though? we could still make it required in the call to the beacon node and default it to url if it's not explicitly set
There was a problem hiding this comment.
If it isn't in the config, how could a node operator set their auth_data if they aren't using the default? I assumed it would have to be by manually inserting into the config which is what this PR helps make programmatic
There was a problem hiding this comment.
operators can still set the auth_data if they wanna override the default (based on the url), but why does it need to be a required field to be configured by the user?
There was a problem hiding this comment.
It definitely doesn't need to be required. I'm happy to make it and all of the fields that can as optional. It was just simpler if the key manager API supplied the VC with fully resolved values derived from the caller's config, so we don't have to reason about how missing fields in the key manager request body have to be resolved differently across all the different VC implementations
There was a problem hiding this comment.
I do see your point but most users are just fine with the default that validator clients provide and each client will have their own defaults, eg. we currently don't force users to provide a gas limit in their config, and most don't and just go with whatever the client sets as default. so the vc can easily fill missing fields with default and supply all fields to the beacon node via api. but when it comes to configuring the validator client from a user perspective, I would prefer if users only have to configure the values they are interested in instead of having to provide the full set of options
There was a problem hiding this comment.
From your comment here, if we use builder_pubkey + the absence of a url as the way to tell the BN "use these config values when evaluating p2p bids from this builder" then I think it makes sense to switch all of these keymanager fields to optional, since you need to show the absence of something.
I'm pretty in favor of this. Not too sure who will want to set per-key, per-builder p2p bid settings but it is elegant that we can support both p2p bids / builder api bids symmetrically. (will build towards this if it sounds good)
There was a problem hiding this comment.
if we use builder_pubkey + the absence of a url as the way to tell the BN "use these config values when evaluating p2p bids from this builder"
if we want the mutually exclusive it could even be a config field like builder_id, which could be a url, or a pubkey, or even a builder index. but it's probably better to have separate fields, also when transmitting this to the beacon node, it would be good if we can support ssz encoding, and union or optional isn't widely supported and not part of standard spec, but for missing pubkey we can just set zero bytes for example
There was a problem hiding this comment.
Agreed separate fields over builder_id since url and pubkey are complementary rather than strictly alternatives. In Terence's PoC the url is where you send and the pubkey is what you check came back, and a union can only carry one of those.
What I'm thinking is optional fields in keymanager and VC has to fully resolve before sending to BN. The one thing is we'll still need per-key min_bid and builder_boost_factor that apply to p2p bids from builders with no entry. From Luca's comment:
All per-key configurability in Vero is handled through the Keymanager API which is standardized across all CL/VC clients. This is the only way Vero allows node operators to specify certain things, like different fee recipient per validator key. I hate the idea of adding support for some kind of YAML file (that hasn't even been standardized) instead of doing this in a more standard way using the Keymanager API. We should seriously stop with these CL-client-specific YAML files, we have the standard Keymanager API for a reason.
So thinking to also add a way for the keymanager api to express the per-key p2p defaults.
| so hex case does not distinguish entries. Several entries MAY share a `url`, each with different | ||
| `auth_data`, and one request is made per entry. |
There was a problem hiding this comment.
Several entries MAY share a
url, each with differentauth_data, and one request is made per entry.
Why is that? Why is a builder not identified only by their URL? What is the use case for fetching bids from the same builder URL multiple times with differing auth data?
There was a problem hiding this comment.
In the case where you connect to a relay/proxy/sidecar/etc fronting multiple builders, the URL is fixed but the auth_data may differ. This came up after removing the original PR's proxy field
Co-authored-by: Luca | Serenita <70237279+eth2353@users.noreply.github.com>
| minItems: 1 | ||
| maxItems: 64 # MAX_BUILDER_ENTRIES | ||
| items: | ||
| $ref: "../keymanager-oapi.yaml#/components/schemas/BuilderEntry" |
There was a problem hiding this comment.
so this allows to configure per-builder, but how would I configure a min_bid or builder_boost_factor that applies to all builders (even from p2p) for this validator? this relates to the comment here ethereum/beacon-APIs#630 (comment), I feel like we add this really complex per-builder config, but at the same time we lose the simple config
| selects a bid; see [produceBlockV4](https://ethereum.github.io/beacon-APIs/#/Validator/produceBlockV4) for the selection rule. | ||
| required: [url, auth_data, max_execution_payment, min_bid, builder_boost_factor] | ||
| properties: | ||
| url: |
There was a problem hiding this comment.
this configuration seems to be fully tailored towards builders connected via builder-api, based on the original intend from ethereum/beacon-APIs#620, this doesn't seem to match up, I left a related comment here ethereum/beacon-APIs#625 (comment)
we also lose functionality we currently have, eg. the builder_boost_factor query param as defined in the beacon-api (on master branch) allows to specify how local payload vs. bids (from api or p2p) should be selected, but with this change, we have 0 ways to configure any bid selection strategy from p2p
maybe @potuz, @terence, or @james-prysm can clarify this, I thought this follows the prysm implementation but it doesn't at all match what potuz documented in ethereum/beacon-APIs#620 which I do prefer since it at least supported the full spectrum of builders, and not just the builder-api
There was a problem hiding this comment.
I don't believe we lose current functionality - we still can prefer local vs bids (from api or p2p) bids using builder_boost_factor right? The same way we do today.
It's just that the api builders as currently proposed enjoy per-builder configurability too, whereas for p2p values (min_bid, builder_boost_factor) apply globally.
There was a problem hiding this comment.
yes this comment is quite outdated, we've added a global builder_boost_factor and min_bid now which can be used to control p2p bids
it's possible to prefer local payload by configuring { "builders": [], "builder_boost_factor": "0" } and you can even configure local only (ie. not accept any bid even if local fails or is delayed) by setting the following { "builders": [], "min_bid": "18446744073709551615" }
regardless of VC's global config
Defaults: default_min_bid and default_builder_boost_factor on BuilderConfig. An entry that omits min_bid or builder_boost_factor takes the key's default, so one value covers every builder without being repeated per entry, and the same values apply to bids from builders no entry identifies. Resolution order is stated as entry value, then the key's default, then the validator client's own configuration. Without the middle step an operator who set a default and omitted the field on entries would silently get the validator client's value instead. GET returns the effective configuration with omitted values resolved, matching getGasLimit and getGraffiti
| Each entry MUST contain at least one of `url` and `builder_pubkey`. No two entries may share the | ||
| same `url`, `auth_data` and `builder_pubkey`, and no `builder_pubkey` may appear twice. The |
There was a problem hiding this comment.
no
builder_pubkeymay appear twice
What if a builder submits bids to 2+ different relays?
There was a problem hiding this comment.
good catch, the rule would need to be:
# a request is identified by where it goes, not by who we expect back
if entry.url:
assert (entry.url, entry.auth_data) not in seen_requests
# a p2p policy is identified by the builder it names
if not entry.url:
assert entry.builder_pubkey not in seen_p2p_policiesSo, you can support 2+ different relays bc if the url is supplied, the only check is unique url/auth pairs and you can reuse the builder_pubkey.
If url is missing, the entry is explicitly for p2p config and that's the case you need to block duplicates
There was a problem hiding this comment.
This is getting so complex, it feels like we should write up some reference test cases for implementers 😅
There was a problem hiding this comment.
I'll share a hack md implementation guide to try to cover all these confusing edge cases tomorrow
There was a problem hiding this comment.
Opted for git for easier tracking over time: JasonVranek/epbs-config-flow#1
| builder_pubkey: | ||
| description: | | ||
| The builder's BLS public key, distinct from the validator `pubkey` in the request path. When | ||
| set, a builder-API bid not signed by it MUST NOT be accepted, and this entry also applies to | ||
| that builder's p2p bids. _48-bytes, hex encoded with 0x prefix, case insensitive._ |
There was a problem hiding this comment.
from #88 (comment)
In Terence's PoC the url is where you send and the pubkey is what you check came back, and a union can only carry one of those.
why is that, the relay or builder will return a SignedExecutionPayloadBid so you can always verify it's signature, the CL should apply the same validations as on gossip on those bids with a few exceptions.
is there another scenario where a separate builder_pubkey should be used rather than the builder_index inside the bid itself?
There was a problem hiding this comment.
It's not about checking the bid is self-consistent but verifying that this bid came from the exact builder the user expected.
# check bid is correctly signed at all
assert validate_signature(signed_bid)
# check bid was signed by expected
assert signed_bid.pubkey == builder_pubkeyAnd I think the main use case is a url fronting multiple builders and you accept a trusted payment. You might not equally trust all the builder indices behind the url. So this is one extra check to filter out bids from unexpected builders.
As to using pubkey vs index I'm not opinionated but I think the reasoning was pubkey is more static
There was a problem hiding this comment.
ok but for that can the pubkey just be part of the URL as it is today? might be easier than having to set it explicitly, I really just wanna have users set --builder.urls in the simple setup case which will be sufficient for most users
There was a problem hiding this comment.
think this makes it difficult for the p2p bid config case since you would omit url
There was a problem hiding this comment.
but for p2p builders you set the pubkey explicitly, but if operators/relays want this to be connected to the url then why does it need to be a separate parameter instead of using the current format which has the pubkey in the url?
There was a problem hiding this comment.
Been thinking about this and think the change makes configs a little simpler but the wire more complicated.
first, for the users you're talking about who just want to set --builder.urls, I'd argue that either builder_pubkey as a field or embedded in userinfo mev-boost style isn't something this category of users actually want to set. adding the builder_pubkey is a provenance check only needed when you are receiving trusted bids over builder api which I assume the set and forget users won't be optimizing for. what follows is if you restrict yourself to something like https://B@relay.host you deny yourself trustless bids from other pubkeys behind that host / must update your config if B ever rotates keys. so if the user doesn't care about trusted payments, they can just set --builder.urls https://relay.host and not be losing anything / be able to set and forget.
now regarding the change. the advantage is definitely that the builder_pubkey field explicitly becomes the toggle for p2p bid preferences and the "provenance check" happens only if you supply the target builder's pubkey in your url's userinfo. you can reason about what's happening right away by looking at the config entry.
the con is that the VC will still have to send an optional builder_pubkey (zero'd out in SSZ) and now an optional pubkey in the userinfo. that's two places a builder key can live, and the userinfo copy is just bytes in the url, so it sidesteps the schema validation the field already gives us + BN has to validate by hand
Mirror of the #630 beacon-APIs change. `builder_pubkey` filters the response on an entry with a `url` and names the p2p builder on one without, so the POST 400 rules and the identity paragraph split in two: entries with a url are unique by (url, auth_data), entries without one by builder_pubkey. This unblocks a builder bidding into several relays, which the old "builder_pubkey may not appear twice" rule rejected.
Reference RequestAuth.data (not RequestAuthV1.data) in the auth_data description
Move min_bid and builder_boost_factor above the builders list, keeping enabled first.
Every entry is a bid request: url is required, and an entry no longer supplies p2p policy for a builder's gossiped bids. The key-level min_bid and builder_boost_factor defaults apply to p2p bids
builder_pubkeys replaces builder_pubkey; omitted resolves to the empty list, which accepts any builder. Also rename validator_builder.yaml -> builders.yaml
Removes the last absence ambiguity: an empty auth_data had no defined meaning, since omission already means derive from the builder's URL. A url must be non-empty so the derivation cannot produce empty bytes.
| data: | ||
| min_bid: "10000000" | ||
| builder_boost_factor: "100" | ||
| builders: |
There was a problem hiding this comment.
left that #88 (comment) before, worth noting that current keymanager apis return the pubkey in the response of any GET request
eg. curl -X GET http://localhost/eth/v1/validator/0x93247f2209abcacf57b75a51dafae777f9dd38bc7053d1af526f220a7489a6d3a2753e5f3e8b1cfe39b56f43611df74a/feerecipient
would return
{
"data": {
"pubkey": "0x93247f2209abcacf57b75a51dafae777f9dd38bc7053d1af526f220a7489a6d3a2753e5f3e8b1cfe39b56f43611df74a",
"ethaddress": "0xAbcF8e0d4e9587369b2301D0790347320302cc09"
}
}do we need to echo the pubkey just to be consistent? curious if @rolfyone or @james-prysm have an opinion on this, or maybe @yorickdowne is that any useful for eth-docker?
There was a problem hiding this comment.
I'm ok either way, without it is also fine, no strong opinions here
There was a problem hiding this comment.
also realized the pubkey field is optional and not required, so it's not even something tooling today can rely on for the existing apis
I did a quick AI analysis of how existing tooling uses the pubkey in the response, and seems like nobody does
-
eth-docker (https://github.com/eth-educators/eth-docker/blob/4fea304aa9dd44235e2c033041c216d32a22c884/vc-utils/keymanager.sh#L232-L238) reads only .data.ethaddress; it uses the request’s
__pubkey for display. -
DAppNode StakingBrain models the field in its response type
(https://github.com/dappnode/StakingBrain/blob/4b7306969557d12546d807378a4df6e6d6f85997/packages/brain/src/modules/apiClients/validator/types.ts#L3-L8), but its actual consumer
(https://github.com/dappnode/StakingBrain/blob/4b7306969557d12546d807378a4df6e6d6f85997/packages/brain/src/modules/db/index.ts#L240-L245) reads only response.data.ethaddress. -
Siren (https://github.com/sigp/siren/blob/ad9fc6eb09816eb7017b69a241c49c17da85026b/backend/src/validator/validator.service.ts#L95-L131) correlates parallel responses by request order and
reads only ethaddress. -
Stereum (https://github.com/stereum-dev/ethereum-node/blob/7cd5a68ff068a30aa98033d1a09f855480a9cc71/launcher/src/components/UI/staking-page/components/list/rows/KeyRow.vue#L453-L455)
reads only ethaddress. -
Rocket Pool Smartnode (https://github.com/rocket-pool/smartnode/blob/fdf0011abb2389ff696516751aa64f7d8e0e48ba/shared/services/rocketpool/fee-recipient.go#L125-L138) deliberately defines
a response structure containing only ethaddress.
so based on this, let's keep it out of the response for new apis going forward, it seems rather useless to echo back the pubkey that caller explicitly has to supply already
Co-authored-by: Nico Flaig <nflaig@protonmail.com>
Co-authored-by: Nico Flaig <nflaig@protonmail.com>
Co-authored-by: Nico Flaig <nflaig@protonmail.com>
| $ref: "../keymanager-oapi.yaml#/components/responses/Unauthorized" | ||
| "403": | ||
| $ref: "../keymanager-oapi.yaml#/components/responses/Forbidden" | ||
| "404": |
There was a problem hiding this comment.
how do you guys feel about this error code? we shouldn't return it if the validator key doesn't exist
There was a problem hiding this comment.
isn't that what we do for all others apis today? eg. https://ethereum.github.io/keymanager-APIs/?urls.primaryName=dev#/Fee%20Recipient/setFeeRecipient
at least in lodestar we explicitly check this and return 404 if validator pubkey is not known
There was a problem hiding this comment.
client responses for an unknown pubkey
james-prysm
left a comment
james-prysm
left a comment
There was a problem hiding this comment.
looks good to me, one note is around 404 on the endpoints it seems like other ones work that way too so i guess we can just leave it. 404 shouldn't be returned if the key isn't found fyi
…ger endpoints (OffchainLabs#17261) **What type of PR is this?** Feature **What does this PR do? Why is it needed?** Adds the per-key builder configuration endpoints from [[keymanager-APIs #88](https://github.com/ethereum/keymanager-APIs/pull/88)](https://github.com/ethereum/keymanager-APIs/pull/88) and the v2 proposer-settings schema that backs them. This is how an operator tells the validator client which external builders each key may source gloas blocks from, and under what per-builder limits. **New endpoints** — `GET`/`POST`/`DELETE /eth/v1/validator/{pubkey}/builders`: - `POST` replaces the key's builder configuration in full. `enabled` is required; each entry requires a `url`; no two entries may share `(url, auth_data)` (an omitted `auth_data` compares as its derived value, the UTF-8 bytes of the url). Violations are rejected with 400 rather than partially stored. - `GET` returns the configuration fully resolved: omitted entry fields are filled with the values that will be used (key defaults, then validator client config, then runtime fallbacks — no floor, neutral boost, trustless-only payment ceiling), so re-submitting a `GET` response pins those values. - `DELETE` removes the key's configuration so it follows the validator client again (204, also when nothing was set). This differs from `enabled: false`, which is a stored statement that the key sources no builder bids at all. - The `builders` list is tri-state: omitted = use the validator client's builders, `[]` = use none (p2p bids only), a list = use exactly these. The use-none marker survives persistence via a proto presence field. **v2 proposer settings** (`"version": 2`): gas limit moves to the option level (a validator property, not a builder property), builder configs gain the `builders[]` entry list plus `min_bid`/`builder_boost_factor` defaults, and per-key fields resolve with field-level inheritance from `default_config`. v1 settings keep their exact object-level semantics until migrated. The unused `relays` field is removed. **Timeline across the fork** - *Before gloas*: v1 settings behave byte-for-byte as on develop (registration, fee recipients, gas limits). The new endpoints are live; `POST /builders` migrates the node's settings to v2 in place (builder lists are v2 content, so writing them is the opt-in). v1 settings that contain builder content log a deprecation warning at startup; fee-recipient/graffiti-only settings never do. - *One epoch before gloas*: for keys with v2 builder configs, the VC starts pre-signing `RequestAuth`s and submitting ahead-of-time builder preferences for upcoming proposal slots, so preferences are in place at the beacon node for the fork's first slot. v1 settings have nothing legitimate to warm and stay inert. - *At and after gloas*: any remaining v1 settings migrate automatically on the first push cycle of the fork epoch — idempotent and value-preserving (gas limits hoisted to the option level and resolving to the same values; an explicit `max_execution_payment: 0` survives). mev-boost registration naturally ends. Keys whose migrated config has no `builders` entries solicit no builder bids and produce blocks locally: safe, but outside the builder market until the operator opts in. **Design decisions and tradeoffs** 1. *Version follows content.* `POST /builders` upgrades to v2 immediately (explicit, authenticated opt-in); the fork is the automatic backstop for everyone else; fee recipient and graffiti are schema-neutral and never touch the version. Tradeoff: one pre-fork `POST` switches the whole node to v2 semantics — accepted as an explicit admin action; a store-without-upgrade variant was tried and rejected as a worse contract (content sitting in version limbo until the fork). 2. *Migration carries nothing into the builder market.* `relays`/`--enable-builder` do not translate into builder entries; participation post-gloas requires an explicitly configured list. Tradeoff: do-nothing operators lose builder revenue but never miss a proposal — the right default when the fork changes who is trusted with payments. 3. *`--enable-builder` keeps its v1 meaning*: it forces the default builder toggle on, even over an explicit false in the default config. Per-key `enabled: false` still opts a key out (the spec's escape hatch). Tradeoff: flag-over-config at the default level, chosen for continuity — a v1 file plus the flag produces the same result before and after migration. 4. *File/URL settings stay authoritative wholesale*: restarting with a file resets per-key state, exactly as today, because there is no other reliable way to clear the DB. Tradeoff: API writes are ephemeral for file users (matching the existing fee-recipient endpoints); the reset path stays predictable. 5. *Current preferences wire is builder-URL-keyed*, so same-url entries collapse to the lowest (safest) payment ceiling, and per-entry `min_bid`/`builder_boost_factor`/`builder_pubkey`/custom `auth_data` are stored and returned by `GET` but only take effect with the beacon-APIs OffchainLabs#630 inline produce wire (marked with `TODO(gloas)` at both the producer and consumer). **Testing plan (kurtosis)** Config (`gloas-km88.yml`), gloas at epoch 3 to leave pre-fork time for API calls: ```yaml participants: - el_type: geth el_image: ethpandaops/geth:bal-devnet-6 cl_type: prysm cl_image: gcr.io/offchainlabs/prysm/beacon-chain:latest vc_image: gcr.io/offchainlabs/prysm/validator:latest supernode: true count: 2 cl_extra_params: - --verbosity=debug vc_extra_params: - --verbosity=debug - --http-host=0.0.0.0 - --http-port=7500 network_params: fulu_fork_epoch: 0 gloas_fork_epoch: 3 seconds_per_slot: 6 genesis_delay: 40 additional_services: - dora ``` Setup: `kurtosis run --enclave km88 github.com/ethpandaops/ethereum-package --args-file gloas-km88.yml`, then from inside the first VC container grab a validating pubkey (`PK`) and the keymanager token (`TOKEN`, from the wallet dir's `auth-token` file). All calls below run inside that container against `http://127.0.0.1:7500`. *A — API CRUD and resolution (pre-fork, epoch 0–2):* 1. `GET /eth/v1/validator/$PK/builders` → 200 with `enabled: false`, `builders: []`, resolved fallbacks (`min_bid "0"`, `builder_boost_factor "100"`). 2. `POST` `{"enabled":true,"min_bid":"1000","builders":[{"url":"https://builder-a.example","max_execution_payment":"250000000"}]}` → 202. 3. `GET` again → entry returned with `auth_data` resolved to the hex of the url bytes, entry `min_bid "1000"` inherited from the key default. 4. `POST` a duplicate `(url, auth_data)` pair and an entry without `url` → both 400, config unchanged. 5. `DELETE` → 204; `GET` shows defaults again; second `DELETE` → 204. **Which issue(s) does this PR fix?** addresse ethereum/keymanager-APIs#88 **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. - [ ] I have tested that my changes work as expected and I added a testing plan to the PR description (if applicable). --------- Co-authored-by: Jun Song <87601811+syjn99@users.noreply.github.com>
| @@ -0,0 +1,158 @@ | |||
| get: | |||
| operationId: getBuilders | |||
There was a problem hiding this comment.
Given the changes since I last took a look, I do think naming these builder_config would fit better than builders. The endpoint descriptions literally say:
Get the builder configuration [...]
(not "Get builders")
Therefore getBuilderConfig / setBuilderConfig / deleteBuilderConfig make more sense to me. The builder config consists of a couple options (min_bid, builder_boost_factor) + builders => it's not only about builders, it's about the surrounding builder config.
There was a problem hiding this comment.
the summary should definitely be adapted, and probably also the operationId, this was renamed earlier when we didn't have the other top-level configs and it was literally just an array of builders
I find it kinda silly though to have _config in an api route in a specification that is literally designed to configure validators, I would prefer to keep the route name as /eth/v1/validator/{pubkey}/builders, where builders is the domain/resource being configured
so my suggestion would be
- (1) keep
/eth/v1/validator/{pubkey}/builders - (2) operationIds:
getBuilderConfigsetBuilderConfigdeleteBuilderConfig
- (3) summaries:
Get Builder ConfigurationSet Builder ConfigurationDelete Builder Configuration
There was a problem hiding this comment.
to me get builders is fine because in the context of all these endpoints it's about updating configurations. Explicitly saying config in my mind doesnt get additional clarification knowing that keymanager apis are about updating configurations. But i don't have strong feelings towards anything just giving my 2 cents. I will change prysm to match what is decided.
There was a problem hiding this comment.
actually, I don't even care, we can even go with /eth/v1/validator/{pubkey}/builder_config, can @JasonVranek make the call? 😂
I will implement whatever you guys prefer here
There was a problem hiding this comment.
while the spec is still malleable went and changed to hopefully avoid tripping people up in the future, now:
/eth/v1/validator/{pubkey}/builder_configgetBuilderConfig/setBuilderConfig/deleteBuilderConfig
| selects a bid; see [produceBlockV4](https://ethereum.github.io/beacon-APIs/#/Validator/produceBlockV4) for the selection rule. | ||
| required: [url, auth_data, max_execution_payment, min_bid, builder_boost_factor] | ||
| properties: | ||
| url: |
There was a problem hiding this comment.
I don't believe we lose current functionality - we still can prefer local vs bids (from api or p2p) bids using builder_boost_factor right? The same way we do today.
It's just that the api builders as currently proposed enjoy per-builder configurability too, whereas for p2p values (min_bid, builder_boost_factor) apply globally.
setBuilderConfig, deleteBuilderConfig
nflaig
changed the title
Co-authored-by: Nico Flaig <nflaig@protonmail.com>
nflaig
left a comment
nflaig
left a comment
There was a problem hiding this comment.
LGTM, thanks @JasonVranek for working through all the details
…ffchainLabs#17379) **What type of PR is this?** Other **What does this PR do? Why is it needed?** renames to GET /eth/v1/validator/{pubkey}/builder_config POST /eth/v1/validator/{pubkey}/builder_config DELETE /eth/v1/validator/{pubkey}/builder_config **Which issue(s) does this PR fix?** last minute follow up to ethereum/keymanager-APIs#88 **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. - [ ] I have tested that my changes work as expected and I added a testing plan to the PR description (if applicable).
…rwarding (#630) The Beacon API side of the Gloas builder flow, aligned with [ethereum/builder-specs#165](ethereum/builder-specs#165) and [ethereum/keymanager-APIs#88](ethereum/keymanager-APIs#88) so the three specs describe one proposer flow. Supersedes #625. - `produceBlockV4` takes a required `BuilderConfig` body: a `builders` list with one `BuilderEntry` per builder-API bid request, plus a top-level `min_bid` and `builder_boost_factor` that apply to p2p bids. A bid is valued at `value + min(execution_payment, max_execution_payment)`, surviving bids are weighted by their `builder_boost_factor` and compared without division, and the local build's value is weighted by `100`. - `submitBuilderPreferences` (`POST /eth/v1/validator/builder_preferences`) lets the validator client push `BuilderPreferencesEntry` objects, batched across its proposers, so the beacon node can submit each builder's `max_execution_payment` cap ahead of the slot. - Block forwarding: `produceBlockV4` returns `Eth-Builder-Url` when a builder-API bid wins; `publishBlockV2` echoes it so the beacon node forwards the signed block to that builder via `submitSignedBeaconBlock`, letting it learn the result without waiting on gossip. - Both request bodies support JSON and SSZ and require `Eth-Consensus-Version`; builder failures are handled per entry so one unusable builder does not fail the block-production request or prevent other preference submissions. Reference implementation: [ChainSafe/lodestar#9832](ChainSafe/lodestar#9832)
6 participants
Adds GET/POST/DELETE on /eth/v1/validator/{pubkey}/builder_config, letting an operator configure which external builders a validator key sources bids from, and with what per-builder limits.
An alternative to #87, which managed one atomic per-key document at /eth/v1/validator/config and deprecated the fee recipient, gas limit and graffiti endpoints. This branch is cut from master, so those three are untouched, and each concern keeps its own endpoint.