add per-key builders 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
| Every field except `builder_pubkey` is required: an entry carries every preference that applies | ||
| to this builder, and none are inherited. The beacon node applies these preferences when it | ||
| 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] |
There was a problem hiding this comment.
why is anything besides url required here?
| $ref: './apis/local_keystores.yaml' | ||
| /eth/v1/remotekeys: | ||
| $ref: './apis/remote_keystores.yaml' | ||
| /eth/v1/validator/{pubkey}/builders: |
There was a problem hiding this comment.
maybe if we do allow a default config per pubkey that applies to all builders we should rename this to builder_config since builders would be a field of the request body @eth2353 suggested that naming earlier here #87 (comment)
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
| 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.
| default_min_bid: "10000000" | ||
| default_builder_boost_factor: "100" |
There was a problem hiding this comment.
why do we need to prefix that with default_? that seems obvious from the hierachy
There was a problem hiding this comment.
agreed with nico we can remove default_
4 participants
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 #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.