-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add GET/POST/DELETE /eth/v1/validator/{pubkey}/builders keymanager endpoints
#17261
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
james-prysm
wants to merge
33
commits into
develop
Choose a base branch
from
km88-proposer-settings
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+3,084
−674
Open
Changes from 21 commits
Commits
Show all changes
33 commits
Select commit
Hold shift + click to select a range
4c09570
initial commit implementing https://github.com/ethereum/keymanager-AP…
james-prysm 27698ca
gaz
james-prysm f48d53b
Merge branch 'develop' into km88-proposer-settings
james-prysm 602d824
Merge branch 'develop' into km88-proposer-settings
james-prysm f4c3354
updating based on removal of p2p boost config
james-prysm df8afee
self review
james-prysm cc92577
Merge branch 'develop' into km88-proposer-settings
james-prysm ea814aa
make changelog more concise
james-prysm 4f25f60
rolling back some flag overrides
james-prysm 40486bd
reverting change
james-prysm aa6247b
clarifying changelog
james-prysm 586904e
self review on comments
james-prysm ba99fc3
moving functions around after self review
james-prysm d9b6989
more bug fixes and missing items after self review
james-prysm 0d863aa
Merge branch 'develop' into km88-proposer-settings
james-prysm 3429854
Update validator/rpc/structs.go
james-prysm 29ea6a0
Update validator/rpc/handlers_validator_config.go
james-prysm 2a365bc
remove proxy that is no longer used
james-prysm e68f2fe
remove enabled tag and use builders tag instead
james-prysm 4ea3e1a
gaz
james-prysm 9698cf1
self review points, don't migrate gas limit in v1 and take in in v2 i…
james-prysm bf4fd8c
fixing test order
james-prysm 3cd047b
adding gloas gates for endpoints, fixing bugs after self review, make…
james-prysm da52e2e
gaz
james-prysm 1526ec2
Merge branch 'develop' into km88-proposer-settings
james-prysm ae648dc
jun's comments
james-prysm 3234435
adding test
james-prysm 57a9174
terence's comments
james-prysm 934cc8d
Merge branch 'develop' into km88-proposer-settings
james-prysm 1ebba34
Merge branch 'develop' into km88-proposer-settings
james-prysm 25d6063
adding test
james-prysm 7a95158
Merge branch 'develop' into km88-proposer-settings
james-prysm 3d761b1
self review, making sure fork transitions are smoother
james-prysm File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| ### Added | ||
|
|
||
| - Add `GET`/`POST`/`DELETE /eth/v1/validator/{pubkey}/builders` keymanager endpoints for per-key builder configuration (keymanager-APIs #88). | ||
|
|
||
| ### Changed | ||
|
|
||
| - Add v2 proposer settings (`"version": 2`): builder fields a key does not set inherit from `default_config`. v2 has no `enabled` field; a key participates in builder registration when its resolved `builders` list names at least one builder, and an explicit empty `builders` list opts the key out. | ||
| - v1 builder settings are not migrated to v2: at the gloas fork fee recipients and graffiti carry over, while v1 builder content — including its gas limits — is replaced with defaults, with a warning. Gas limits apply only when explicitly set on v2 settings, so validators follow future chain-default gas limit increases unless they opt out. `--enable-builder` has no effect with v2 settings. | ||
| - Setting a fee recipient, gas limit, or graffiti no longer snapshots `default_config`'s builder settings onto that key; the key keeps following the default as it changes. | ||
|
|
||
| ### Deprecated | ||
|
|
||
| - `--with-builder` generates legacy (pre-gloas) mev-boost builder settings, which are discontinued at the gloas fork; the command now warns when the flag is used. | ||
|
|
||
| ### Fixed | ||
|
|
||
| - Fix the minimal slashing protection database decoding unset builder settings fields as explicit zero values; both validator DB backends now read the same stored settings identically. | ||
|
|
||
| ### Removed | ||
|
|
||
| - Remove the unused `relays` field from the builder config; a settings file that still contains it is unaffected. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| package proposer | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/OffchainLabs/prysm/v7/consensus-types/validator" | ||
| "github.com/OffchainLabs/prysm/v7/testing/require" | ||
| ) | ||
|
|
||
| // These cases mirror the test vectors proposed upstream on keymanager-APIs #87 | ||
| // for builder-config inheritance granularity. | ||
| func TestEffectiveBuilderConfig(t *testing.T) { | ||
| entryA := &BuilderEntry{URL: "https://a"} | ||
| entryB := &BuilderEntry{URL: "https://b"} | ||
| entryC := &BuilderEntry{URL: "https://c"} | ||
|
|
||
| t.Run("nil per-key returns default", func(t *testing.T) { | ||
| def := &BuilderConfig{Enabled: true} | ||
| require.Equal(t, def, effectiveBuilderConfig(nil, def)) | ||
| }) | ||
| t.Run("nil default returns per-key", func(t *testing.T) { | ||
| perKey := &BuilderConfig{Enabled: true} | ||
| require.Equal(t, perKey, effectiveBuilderConfig(perKey, nil)) | ||
| }) | ||
| t.Run("min_bid inherits when per-key omits it", func(t *testing.T) { | ||
| def := &BuilderConfig{Enabled: true, Builders: []*BuilderEntry{entryA, entryB}, MinBid: uint64ValPtr(5000000)} | ||
| perKey := &BuilderConfig{Enabled: true, Builders: []*BuilderEntry{entryC}} | ||
| eff := effectiveBuilderConfig(perKey, def) | ||
| require.NotNil(t, eff.MinBid) | ||
| require.Equal(t, validator.Uint64(5000000), *eff.MinBid) | ||
| require.Equal(t, 1, len(eff.Builders)) | ||
| require.Equal(t, "https://c", eff.Builders[0].URL) | ||
| }) | ||
| t.Run("explicit zero max payment is preserved, not inherited over", func(t *testing.T) { | ||
| def := &BuilderConfig{MaxExecutionPayment: uint64ValPtr(1000000000)} | ||
| perKey := &BuilderConfig{Enabled: true, MaxExecutionPayment: uint64ValPtr(0)} | ||
| eff := effectiveBuilderConfig(perKey, def) | ||
| require.NotNil(t, eff.MaxExecutionPayment) | ||
| require.Equal(t, validator.Uint64(0), *eff.MaxExecutionPayment) | ||
| }) | ||
| t.Run("unset max payment inherits default", func(t *testing.T) { | ||
| def := &BuilderConfig{MaxExecutionPayment: uint64ValPtr(1000000000)} | ||
| perKey := &BuilderConfig{Enabled: true} | ||
| eff := effectiveBuilderConfig(perKey, def) | ||
| require.NotNil(t, eff.MaxExecutionPayment) | ||
| require.Equal(t, validator.Uint64(1000000000), *eff.MaxExecutionPayment) | ||
| }) | ||
| t.Run("explicit per-key disable wins over enabled default", func(t *testing.T) { | ||
| def := &BuilderConfig{Enabled: true} | ||
| perKey := &BuilderConfig{Enabled: false, MinBid: uint64ValPtr(1)} | ||
| require.Equal(t, false, effectiveBuilderConfig(perKey, def).IsEnabled()) | ||
| }) | ||
| t.Run("present per-key builder config is authoritative on enabled", func(t *testing.T) { | ||
| // A per-key config with enabled false does not inherit an enabled default; | ||
| // whole-config inheritance happens only when the per-key builder config is nil. | ||
| def := &BuilderConfig{Enabled: true} | ||
| perKey := &BuilderConfig{MinBid: uint64ValPtr(1)} | ||
| require.Equal(t, false, effectiveBuilderConfig(perKey, def).IsEnabled()) | ||
| require.Equal(t, true, effectiveBuilderConfig(nil, def).IsEnabled()) | ||
| }) | ||
| t.Run("present builders list replaces, never unions", func(t *testing.T) { | ||
| def := &BuilderConfig{Builders: []*BuilderEntry{entryA, entryB}} | ||
| perKey := &BuilderConfig{Builders: []*BuilderEntry{entryC}} | ||
| eff := effectiveBuilderConfig(perKey, def) | ||
| require.Equal(t, 1, len(eff.Builders)) | ||
| require.Equal(t, "https://c", eff.Builders[0].URL) | ||
| }) | ||
| t.Run("absent builders list inherits default list", func(t *testing.T) { | ||
| def := &BuilderConfig{Builders: []*BuilderEntry{entryA, entryB}} | ||
| perKey := &BuilderConfig{Enabled: true} | ||
| require.Equal(t, 2, len(effectiveBuilderConfig(perKey, def).Builders)) | ||
| }) | ||
| t.Run("zero gas limit inherits default gas limit", func(t *testing.T) { | ||
| def := &BuilderConfig{GasLimit: validator.Uint64(30000000)} | ||
| perKey := &BuilderConfig{Enabled: true} | ||
| require.Equal(t, validator.Uint64(30000000), effectiveBuilderConfig(perKey, def).GasLimit) | ||
| }) | ||
| t.Run("boost factor inherits per field", func(t *testing.T) { | ||
| def := &BuilderConfig{MinBid: uint64ValPtr(5000000), BuilderBoostFactor: uint64ValPtr(90)} | ||
| perKey := &BuilderConfig{Enabled: true, BuilderBoostFactor: uint64ValPtr(120)} | ||
| eff := effectiveBuilderConfig(perKey, def) | ||
| require.Equal(t, validator.Uint64(5000000), *eff.MinBid) | ||
| require.Equal(t, validator.Uint64(120), *eff.BuilderBoostFactor) | ||
| }) | ||
| t.Run("both-set min_bid: per-key wins", func(t *testing.T) { | ||
| def := &BuilderConfig{MinBid: uint64ValPtr(5000000)} | ||
| perKey := &BuilderConfig{MinBid: uint64ValPtr(7000000)} | ||
| require.Equal(t, validator.Uint64(7000000), *effectiveBuilderConfig(perKey, def).MinBid) | ||
| }) | ||
| t.Run("nonzero per-key gas limit wins over default", func(t *testing.T) { | ||
| def := &BuilderConfig{GasLimit: validator.Uint64(30000000)} | ||
| perKey := &BuilderConfig{GasLimit: validator.Uint64(45000000)} | ||
| require.Equal(t, validator.Uint64(45000000), effectiveBuilderConfig(perKey, def).GasLimit) | ||
| }) | ||
| t.Run("nil nil is nil", func(t *testing.T) { | ||
| require.Equal(t, (*BuilderConfig)(nil), effectiveBuilderConfig(nil, nil)) | ||
| }) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,6 @@ | ||
| package loader | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "fmt" | ||
| "strconv" | ||
|
|
||
|
|
@@ -132,13 +131,9 @@ func (psl *SettingsLoader) Load(cliCtx *cli.Context) (*proposer.Settings, error) | |
| return nil, err | ||
| } | ||
| dbSettings = dbps.ToConsensus() | ||
| log.Debugf("DB loaded proposer settings: %s", func() string { | ||
| b, err := json.Marshal(dbSettings) | ||
| if err != nil { | ||
| return err.Error() | ||
| } | ||
| return string(b) | ||
| }()) | ||
| log.WithField("version", dbSettings.Version). | ||
| WithField("proposerKeys", len(dbSettings.ProposerConfig)). | ||
| Debug("Loaded proposer settings from DB") | ||
| } | ||
|
|
||
| // start to process based on load method | ||
|
|
@@ -186,18 +181,6 @@ func (psl *SettingsLoader) Load(cliCtx *cli.Context) (*proposer.Settings, error) | |
| return ps, nil | ||
| } | ||
|
|
||
| func hasBuilderShape(p *validatorpb.ProposerSettingsPayload) bool { | ||
| if p.DefaultConfig != nil && p.DefaultConfig.Builder != nil { | ||
| return true | ||
| } | ||
| for _, o := range p.ProposerConfig { | ||
| if o != nil && o.Builder != nil { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| func (psl *SettingsLoader) applyOverrides() { | ||
| if psl.options.builderConfig != nil && psl.options.gasLimit != nil { | ||
| psl.options.builderConfig.GasLimit = *psl.options.gasLimit | ||
|
|
@@ -231,6 +214,7 @@ func (psl *SettingsLoader) loadFromFile(cliCtx *cli.Context, dbSettings *validat | |
| if settingFromFile == nil { | ||
| return nil, errors.Errorf("proposer settings is empty after unmarshalling from file specified by %s flag", flags.ProposerSettingsFlag.Name) | ||
| } | ||
| markExplicitEmptyBuilders(settingFromFile) | ||
| log.WithField(flags.ProposerSettingsFlag.Name, cliCtx.String(flags.ProposerSettingsFlag.Name)).Info("Proposer settings loaded from file") | ||
| return psl.processProposerSettings(settingFromFile, dbSettings), nil | ||
| } | ||
|
|
@@ -243,6 +227,7 @@ func (psl *SettingsLoader) loadFromURL(cliCtx *cli.Context, dbSettings *validato | |
| if settingFromURL == nil { | ||
| return nil, errors.Errorf("proposer settings is empty after unmarshalling from url specified by %s flag", flags.ProposerSettingsURLFlag.Name) | ||
| } | ||
| markExplicitEmptyBuilders(settingFromURL) | ||
| log.WithField(flags.ProposerSettingsURLFlag.Name, cliCtx.String(flags.ProposerSettingsURLFlag.Name)).Infof("Proposer settings loaded from URL") | ||
| return psl.processProposerSettings(settingFromURL, dbSettings), nil | ||
| } | ||
|
|
@@ -271,10 +256,17 @@ func mergeProposerSettings(loaded, db *validatorpb.ProposerSettingsPayload, opti | |
| if db != nil { | ||
| merged.Version = db.Version | ||
| } | ||
| // v1-shaped source content must not inherit DB's v2, else the runtime upgrade is skipped. | ||
| if loaded != nil && (loaded.Version != 0 || hasBuilderShape(loaded)) { | ||
| if loaded != nil && loaded.Version > merged.Version { | ||
| merged.Version = loaded.Version | ||
| } | ||
| if merged.Version == proposer.SchemaV2 { | ||
| if db != nil && db.Version < proposer.SchemaV2 { | ||
| promotePayloadToV2(db) | ||
| } | ||
| if loaded != nil && loaded.Version < proposer.SchemaV2 { | ||
| promotePayloadToV2(loaded) | ||
| } | ||
| } | ||
|
|
||
| var builderConfig *validatorpb.BuilderConfig | ||
| var gasLimitOnly *validator.Uint64 | ||
|
|
@@ -286,11 +278,60 @@ func mergeProposerSettings(loaded, db *validatorpb.ProposerSettingsPayload, opti | |
| } | ||
|
|
||
| if merged.Version == proposer.SchemaV2 { | ||
| return mergeProposerSettingsV2(merged, loaded, db, gasLimitOnly) | ||
| return mergeProposerSettingsV2(merged, loaded, db, builderConfig, gasLimitOnly) | ||
| } | ||
| return mergeProposerSettingsV1(merged, loaded, db, builderConfig, gasLimitOnly) | ||
| } | ||
|
|
||
| // markExplicitEmptyBuilders stamps the persistence marker for a user source's | ||
| // explicit "builders": [] (opt-out), which yaml keeps distinct from absent. | ||
| func markExplicitEmptyBuilders(p *validatorpb.ProposerSettingsPayload) { | ||
|
syjn99 marked this conversation as resolved.
|
||
| mark := func(opt *validatorpb.ProposerOptionPayload) { | ||
| if opt == nil || opt.Builder == nil { | ||
| return | ||
| } | ||
| if opt.Builder.Builders != nil { | ||
| opt.Builder.BuildersSet = true | ||
| } | ||
| } | ||
| mark(p.DefaultConfig) | ||
| for _, opt := range p.ProposerConfig { | ||
| mark(opt) | ||
| } | ||
| } | ||
|
|
||
| // promotePayloadToV2 mirrors Settings.UpgradeToV2: v1 builder content, including | ||
| // its gas limits, does not apply to v2 and is dropped. | ||
| func promotePayloadToV2(p *validatorpb.ProposerSettingsPayload) { | ||
| dropped := false | ||
| promote := func(opt *validatorpb.ProposerOptionPayload) { | ||
| if opt == nil || opt.Builder == nil { | ||
| return | ||
| } | ||
| opt.Builder = nil | ||
| dropped = true | ||
| } | ||
| promote(p.DefaultConfig) | ||
| for _, opt := range p.ProposerConfig { | ||
| promote(opt) | ||
| } | ||
| if dropped { | ||
| log.Warn("v1 builder settings, including gas limits, do not apply to the v2 schema and were replaced with defaults; provide v2 proposer settings to configure builders") | ||
| } | ||
| } | ||
|
|
||
| // selectProposerConfig keeps the pre-v2 source precedence: a loaded per-key | ||
| // section replaces the DB's entirely, so restarting with a file resets the DB. | ||
| func selectProposerConfig(db, loaded *validatorpb.ProposerSettingsPayload) map[string]*validatorpb.ProposerOptionPayload { | ||
| if loaded != nil && len(loaded.ProposerConfig) > 0 { | ||
| return loaded.ProposerConfig | ||
| } | ||
| if db != nil && len(db.ProposerConfig) > 0 { | ||
| return db.ProposerConfig | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func mergeProposerSettingsV1(merged, loaded, db *validatorpb.ProposerSettingsPayload, builderConfig *validatorpb.BuilderConfig, gasLimitOnly *validator.Uint64) *validatorpb.ProposerSettingsPayload { | ||
| stripDBBuilder := builderConfig == nil | ||
|
|
||
|
|
@@ -304,17 +345,12 @@ func mergeProposerSettingsV1(merged, loaded, db *validatorpb.ProposerSettingsPay | |
| merged.DefaultConfig = loaded.DefaultConfig | ||
| } | ||
|
|
||
| if db != nil && len(db.ProposerConfig) > 0 { | ||
| merged.ProposerConfig = db.ProposerConfig | ||
| if stripDBBuilder { | ||
| for _, option := range db.ProposerConfig { | ||
| option.Builder = nil | ||
| } | ||
| if db != nil && stripDBBuilder { | ||
| for _, option := range db.ProposerConfig { | ||
| option.Builder = nil | ||
| } | ||
| } | ||
| if loaded != nil && len(loaded.ProposerConfig) > 0 { | ||
| merged.ProposerConfig = loaded.ProposerConfig | ||
| } | ||
| merged.ProposerConfig = selectProposerConfig(db, loaded) | ||
|
|
||
| if merged.DefaultConfig != nil { | ||
| merged.DefaultConfig.Builder = processBuilderConfig(merged.DefaultConfig.Builder, builderConfig, gasLimitOnly) | ||
|
|
@@ -331,25 +367,26 @@ func mergeProposerSettingsV1(merged, loaded, db *validatorpb.ProposerSettingsPay | |
| merged.DefaultConfig = &validatorpb.ProposerOptionPayload{Builder: builderConfig} | ||
| case gasLimitOnly != nil: | ||
| merged.DefaultConfig = &validatorpb.ProposerOptionPayload{ | ||
| Builder: &validatorpb.BuilderConfig{Enabled: false, GasLimit: *gasLimitOnly}, | ||
| Builder: &validatorpb.BuilderConfig{GasLimit: *gasLimitOnly}, | ||
| } | ||
| } | ||
| } | ||
| return merged | ||
| } | ||
|
|
||
| func mergeProposerSettingsV2(merged, loaded, db *validatorpb.ProposerSettingsPayload, gasLimitOnly *validator.Uint64) *validatorpb.ProposerSettingsPayload { | ||
| func mergeProposerSettingsV2(merged, loaded, db *validatorpb.ProposerSettingsPayload, builderConfig *validatorpb.BuilderConfig, gasLimitOnly *validator.Uint64) *validatorpb.ProposerSettingsPayload { | ||
| if db != nil && db.DefaultConfig != nil { | ||
| merged.DefaultConfig = db.DefaultConfig | ||
| } | ||
| if loaded != nil && loaded.DefaultConfig != nil { | ||
| merged.DefaultConfig = loaded.DefaultConfig | ||
| } | ||
| if db != nil && len(db.ProposerConfig) > 0 { | ||
| merged.ProposerConfig = db.ProposerConfig | ||
| } | ||
| if loaded != nil && len(loaded.ProposerConfig) > 0 { | ||
| merged.ProposerConfig = loaded.ProposerConfig | ||
| merged.ProposerConfig = selectProposerConfig(db, loaded) | ||
|
|
||
| // v2 has no enabled toggle: participation follows the configured builders | ||
| // list, so --enable-builder has nothing to force on. | ||
| if builderConfig != nil { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can change this in a subsequent pr ( or if there's something temporary we can do it here too) |
||
| log.Warnf("--%s has no effect with v2 proposer settings; configure builders via the settings source or keymanager API", flags.EnableBuilderFlag.Name) | ||
| } | ||
|
|
||
| if gasLimitOnly == nil { | ||
|
|
@@ -383,7 +420,7 @@ func processBuilderConfig(current *validatorpb.BuilderConfig, override *validato | |
| return override | ||
| } | ||
| if gasLimitOnly != nil { | ||
| return &validatorpb.BuilderConfig{Enabled: false, GasLimit: *gasLimitOnly} | ||
| return &validatorpb.BuilderConfig{GasLimit: *gasLimitOnly} | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.