Add GET/POST/DELETE /eth/v1/validator/{pubkey}/builders keymanager endpoints - #17261
Add GET/POST/DELETE /eth/v1/validator/{pubkey}/builders keymanager endpoints#17261james-prysm wants to merge 29 commits into
GET/POST/DELETE /eth/v1/validator/{pubkey}/builders keymanager endpoints#17261Conversation
GET/POST/DELETE /eth/v1/validator/{pubkey}/builders keymanager endpoints
| ]; | ||
| repeated string relays = 3; | ||
| reserved 3; | ||
| reserved "relays"; |
There was a problem hiding this comment.
for the reviewer, let me know what you think if we really need this value, i think removing it outright is a breaking change possibly but it's never been used before so not sure if i can just remove it
|
pr changes how bid pathing works and affects #17124 |
| return nil, errors.Wrapf(err, "could not unmarshal %s", cleanedConfigFilePath) | ||
| } | ||
|
|
||
| // yaml.Unmarshal converts nil array to empty array. |
There was a problem hiding this comment.
we never used the relay field to begin with so just get rid of it
Co-authored-by: Jun Song <87601811+syjn99@users.noreply.github.com>
Co-authored-by: Jun Song <87601811+syjn99@users.noreply.github.com>
|
for the reviewer, lets leave the enable-builder flag fixes/changes to a separate pr, the ux is akward here but i don't want to clutter this pr with more |
| promote(opt) | ||
| } | ||
| if dropped { | ||
| log.Warn("v1 builder settings do not apply to the v2 schema and were replaced with defaults; provide v2 proposer settings to configure builders") |
There was a problem hiding this comment.
i'm choosing to force promote those who didn't update their configs themselves, what do you guys think?
|
|
||
| // v2 has no enabled toggle: participation follows the configured builders | ||
| // list, so --enable-builder has nothing to force on. | ||
| if builderConfig != nil { |
There was a problem hiding this comment.
we can change this in a subsequent pr ( or if there's something temporary we can do it here too)
| return targets | ||
| } | ||
|
|
||
| func uint64Ptr(v *validatortypes.Uint64) *uint64 { |
There was a problem hiding this comment.
maybe i should move this? not sure if this type should live here
| return be, nil | ||
| } | ||
|
|
||
| func parseUint(s, field string) (validator.Uint64, error) { |
There was a problem hiding this comment.
maybe i should move this somewhere else as a helper?
There was a problem hiding this comment.
I think it's harmless to use strconv.ParseUint(s, 10, 64) every time, and decide the caller to create a new error with field.
… sure we only use gas limits on explicitly set upgraded settings
|
i explicitly gated the new endpoints in this pr until gloas is set, we can decide in a follow up if we want that vs allowing it on release that focuses mroe on the fork transition aspects ( will update pr description) |
| type BuilderEntry struct { | ||
| Url string `json:"url"` | ||
| AuthData *string `json:"auth_data,omitempty"` | ||
| BuilderPubkeys []string `json:"builder_pubkeys"` |
There was a problem hiding this comment.
The builder BLS public keys this entry accepts bids from. Empty or omitted accepts any builder; otherwise a bid not signed by one of them MUST NOT be accepted.
Do we have to make this field required?
There was a problem hiding this comment.
I think post requests accept it omitted and only get is required
| ]; | ||
| repeated string relays = 3; | ||
| reserved 3; | ||
| reserved "relays"; |
syjn99
left a comment
There was a problem hiding this comment.
One other comment is that we should rewrite our documentation that mentions about the version system of proposer settings. Also would be great if we can restructurize the docs - this guide is under fee recipient section.
| walletDir string | ||
| jwtSecret []byte | ||
| grpcHeaders []string | ||
| proposerSettingsLock sync.Mutex |
There was a problem hiding this comment.
If we really need the serialization for read and write, why don't we use atomic.Pointer[proposer.Settings] in validator struct instead of the Server managing the lock? Or we might add a lock in validator struct.
There was a problem hiding this comment.
I don't think atomic.Pointer is enough in this case i think we need s lock anyways. probably something to deal with in a followup pr though
| @@ -129,16 +129,53 @@ message ProposerOptionPayload { | |||
|
|
|||
| // BuilderConfig is a property of ProposerOptionPayload | |||
| message BuilderConfig { | |||
There was a problem hiding this comment.
Maybe marking each fields as deprecated like: [ deprecated = true ]?
There was a problem hiding this comment.
post gloas we can probably just delete those fields, not sure if marking deprecated as true works if still used in v1
| } | ||
| // UpsertProposeOption returns pubkey's option, creating it if absent. A new | ||
| // option keeps BuilderConfig nil so it inherits default_config. | ||
| func (ps *Settings) UpsertProposeOption(pubkey [fieldparams.BLSPubkeyLength]byte) *Option { |
There was a problem hiding this comment.
Nit, to make this method "upsert" on the ps, I'd pass a callback function as an argument and let it mutate the Option: mutate func(*Option) *Option so it'll look like:
settings.UpsertProposeOption(bytesutil.ToBytes48(pubkey), func(opt *proposer.Option) {
opt.FeeRecipientConfig = &proposer.FeeRecipientConfig{FeeRecipient: feeRecipient}
})There was a problem hiding this comment.
hmm looking at it, it doesn't seem to remove any duplication, is there another reason to do this? ( i haven't set this up yet)
| (ethereum.eth.ext.cast_type) = | ||
| "github.com/OffchainLabs/prysm/v7/consensus-types/validator.Uint64" | ||
| ]; | ||
| optional uint64 max_execution_payment = 5 [ |
There was a problem hiding this comment.
Seems like we still has a map from pubkey -> max_execution_payment value. So does this mean per-entry max_execution_payment is not enforced?
yes that's a needed step i think it should be part of a followup pr as even this pr has some tradeoffs we still need to address in a followup |
| // UpgradeToV2 is the v1 cutover: v1 builder configs, including their gas limits, | ||
| // do not apply to gloas and are dropped. Returns true if anything changed. | ||
| func (ps *Settings) UpgradeToV2() bool { | ||
| if ps == nil || ps.isV2() { |
There was a problem hiding this comment.
this means v1 builder content that ends up under a v2 stamp is never cleaned up
There was a problem hiding this comment.
scrubbed in 57a9174#diff-22d828bd0d0814ea79cf0313093e66de9c290b6e89166ae373e49d5da8aff21a what do you think
| if opt.GasLimit == 0 { | ||
| opt.GasLimit = opt.BuilderConfig.GasLimit | ||
| } | ||
| opt.BuilderConfig = nil |
There was a problem hiding this comment.
this also drops an explicit --suggested-gas-limit
There was a problem hiding this comment.
I think we should handle this in a sebsequent pr, i'm afraid of using this to override the schedule numbers and should think more about it, let me see what I can do to make sure it's cut off for now
There was a problem hiding this comment.
officially dropped in 57a9174#diff-22d828bd0d0814ea79cf0313093e66de9c290b6e89166ae373e49d5da8aff21a what do you think?
| } | ||
| // Pointer changed = a keymanager write landed after our snapshot; swapping | ||
| // our stale clone would erase it. This cleanup simply reruns next cycle. | ||
| if v.ProposerSettings() != snapshot { |
There was a problem hiding this comment.
I think there's a race, the keymanager handlers serialize on the server's proposerSettingsLock but this path takes no lock
There was a problem hiding this comment.
crap i think you're right let me see how i can fix this
There was a problem hiding this comment.
57a9174#diff-22d828bd0d0814ea79cf0313093e66de9c290b6e89166ae373e49d5da8aff21a passed in a mutation function i think it fixes it but will double check again
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](ethereum/keymanager-APIs#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:POSTreplaces the key's builder configuration in full.enabledis required; each entry requires aurl; no two entries may share(url, auth_data)(an omittedauth_datacompares as its derived value, the UTF-8 bytes of the url). Violations are rejected with 400 rather than partially stored.GETreturns 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 aGETresponse pins those values.DELETEremoves the key's configuration so it follows the validator client again (204, also when nothing was set). This differs fromenabled: false, which is a stored statement that the key sources no builder bids at all.builderslist 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 thebuilders[]entry list plusmin_bid/builder_boost_factordefaults, and per-key fields resolve with field-level inheritance fromdefault_config. v1 settings keep their exact object-level semantics until migrated. The unusedrelaysfield is removed.Timeline across the fork
POST /buildersmigrates 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.RequestAuths 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.max_execution_payment: 0survives). mev-boost registration naturally ends. Keys whose migrated config has nobuildersentries solicit no builder bids and produce blocks locally: safe, but outside the builder market until the operator opts in.Design decisions and tradeoffs
POST /buildersupgrades 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-forkPOSTswitches 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).relays/--enable-builderdo 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.--enable-builderkeeps its v1 meaning: it forces the default builder toggle on, even over an explicit false in the default config. Per-keyenabled: falsestill 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.min_bid/builder_boost_factor/builder_pubkey/customauth_dataare stored and returned byGETbut only take effect with the beacon-APIs Updates pubkey from bytes32 to bytes #630 inline produce wire (marked withTODO(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: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'sauth-tokenfile). All calls below run inside that container againsthttp://127.0.0.1:7500.A — API CRUD and resolution (pre-fork, epoch 0–2):
GET /eth/v1/validator/$PK/builders→ 200 withenabled: false,builders: [], resolved fallbacks (min_bid "0",builder_boost_factor "100").POST{"enabled":true,"min_bid":"1000","builders":[{"url":"https://builder-a.example","max_execution_payment":"250000000"}]}→ 202.GETagain → entry returned withauth_dataresolved to the hex of the url bytes, entrymin_bid "1000"inherited from the key default.POSTa duplicate(url, auth_data)pair and an entry withouturl→ both 400, config unchanged.DELETE→ 204;GETshows defaults again; secondDELETE→ 204.Which issue(s) does this PR fix?
addresse ethereum/keymanager-APIs#88
Other notes for review
Acknowledgements