diff --git a/changelog/syjn99_proposer-settings-loud-logs.md b/changelog/syjn99_proposer-settings-loud-logs.md new file mode 100644 index 000000000000..e76abe7ebe17 --- /dev/null +++ b/changelog/syjn99_proposer-settings-loud-logs.md @@ -0,0 +1,3 @@ +### Changed + +- The validator client now warns at startup when per-key proposer settings saved in the validator DB (including changes made through the keymanager API) are replaced by the configured `--proposer-settings-file`/`--proposer-settings-url`, listing the dropped and overridden keys. diff --git a/config/proposer/loader/BUILD.bazel b/config/proposer/loader/BUILD.bazel index 401a0fbcb33c..2681de0d44c0 100644 --- a/config/proposer/loader/BUILD.bazel +++ b/config/proposer/loader/BUILD.bazel @@ -45,5 +45,6 @@ go_library( "@com_github_pkg_errors//:go_default_library", "@com_github_sirupsen_logrus//:go_default_library", "@com_github_urfave_cli_v2//:go_default_library", + "@org_golang_google_protobuf//proto:go_default_library", ], ) diff --git a/config/proposer/loader/loader.go b/config/proposer/loader/loader.go index 1ecaa71dfe84..dbb4e7e48afe 100644 --- a/config/proposer/loader/loader.go +++ b/config/proposer/loader/loader.go @@ -2,7 +2,9 @@ package loader import ( "fmt" + "sort" "strconv" + "strings" "github.com/OffchainLabs/prysm/v7/cmd/validator/flags" "github.com/OffchainLabs/prysm/v7/config" @@ -14,8 +16,12 @@ import ( "github.com/ethereum/go-ethereum/common" "github.com/pkg/errors" "github.com/urfave/cli/v2" + "google.golang.org/protobuf/proto" ) +// maxLoggedKeys caps the key lists in the DB-replacement warning. +const maxLoggedKeys = 10 + type settingsType int const ( @@ -27,10 +33,11 @@ const ( ) type SettingsLoader struct { - loadMethods []settingsType - existsInDB bool - db iface.ValidatorDB - options *flagOptions + loadMethods []settingsType + existsInDB bool + replacesDBKeys bool + db iface.ValidatorDB + options *flagOptions } type flagOptions struct { @@ -123,13 +130,15 @@ func determineLoadMethods(cliCtx *cli.Context, loadedFromDB bool) []settingsType // Load saves the proposer settings to the database func (psl *SettingsLoader) Load(cliCtx *cli.Context) (*proposer.Settings, error) { var loadedSettings, dbSettings *validatorpb.ProposerSettingsPayload + var dbps *proposer.Settings // override settings based on other options psl.applyOverrides() // check if database has settings already if psl.existsInDB { - dbps, err := psl.db.ProposerSettings(cliCtx.Context) + var err error + dbps, err = psl.db.ProposerSettings(cliCtx.Context) if err != nil { return nil, err } @@ -179,12 +188,52 @@ func (psl *SettingsLoader) Load(cliCtx *cli.Context) (*proposer.Settings, error) } ps.WarnDeprecatedSchema() ps.WarnUnsetMaxExecutionPayment() + if psl.replacesDBKeys { + warnReplacedDBKeys(dbps, ps) + } if err := psl.db.SaveProposerSettings(cliCtx.Context, ps); err != nil { return nil, err } return ps, nil } +// warnReplacedDBKeys lists the DB per-key entries the settings file/URL replaced. +// Comparing normalized settings keeps an unchanged restart quiet. +func warnReplacedDBKeys(db, merged *proposer.Settings) { + if db == nil || len(db.ProposeConfig) == 0 { + return + } + var dropped, overridden []string + for key, dbOpt := range db.ProposeConfig { + opt, ok := merged.ProposeConfig[key] + switch { + case !ok: + dropped = append(dropped, fmt.Sprintf("%#x", key)) + case !proto.Equal(opt.ToConsensus(), dbOpt.ToConsensus()): + overridden = append(overridden, fmt.Sprintf("%#x", key)) + } + } + if len(dropped) == 0 && len(overridden) == 0 { + return + } + log.WithField("droppedKeys", capKeys(dropped)). + WithField("droppedCount", len(dropped)). + WithField("overriddenKeys", capKeys(overridden)). + WithField("overriddenCount", len(overridden)). + Warn("Per-key proposer settings saved in the validator DB by a previous run differ from the configured settings file/URL; " + + "the settings source is authoritative and the DB entries are replaced. " + + "Changes made through the keymanager API do not survive a restart while a settings file or URL is configured") +} + +// capKeys renders a sorted key list, truncated to maxLoggedKeys with a "+N more" tail. +func capKeys(keys []string) string { + sort.Strings(keys) + if len(keys) > maxLoggedKeys { + return fmt.Sprintf("%s +%d more", strings.Join(keys[:maxLoggedKeys], ","), len(keys)-maxLoggedKeys) + } + return strings.Join(keys, ",") +} + func (psl *SettingsLoader) applyOverrides() { if psl.options.builderConfig != nil && psl.options.gasLimit != nil { psl.options.builderConfig.GasLimit = *psl.options.gasLimit @@ -220,6 +269,7 @@ func (psl *SettingsLoader) loadFromFile(cliCtx *cli.Context, dbSettings *validat } markExplicitEmptyBuilders(settingFromFile) inferSchemaVersion(settingFromFile) + psl.replacesDBKeys = len(settingFromFile.ProposerConfig) > 0 log.WithField(flags.ProposerSettingsFlag.Name, cliCtx.String(flags.ProposerSettingsFlag.Name)).Info("Proposer settings loaded from file") return psl.processProposerSettings(settingFromFile, dbSettings), nil } @@ -234,6 +284,7 @@ func (psl *SettingsLoader) loadFromURL(cliCtx *cli.Context, dbSettings *validato } markExplicitEmptyBuilders(settingFromURL) inferSchemaVersion(settingFromURL) + psl.replacesDBKeys = len(settingFromURL.ProposerConfig) > 0 log.WithField(flags.ProposerSettingsURLFlag.Name, cliCtx.String(flags.ProposerSettingsURLFlag.Name)).Infof("Proposer settings loaded from URL") return psl.processProposerSettings(settingFromURL, dbSettings), nil } @@ -328,6 +379,7 @@ func inferSchemaVersion(p *validatorpb.ProposerSettingsPayload) { // 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. +// Load reports what that replaced through warnReplacedDBKeys. func selectProposerConfig(db, loaded *validatorpb.ProposerSettingsPayload) map[string]*validatorpb.ProposerOptionPayload { if loaded != nil && len(loaded.ProposerConfig) > 0 { return loaded.ProposerConfig diff --git a/config/proposer/loader/loader_test.go b/config/proposer/loader/loader_test.go index 7f1a55a4a069..e09fc5e79b48 100644 --- a/config/proposer/loader/loader_test.go +++ b/config/proposer/loader/loader_test.go @@ -28,6 +28,10 @@ import ( func TestProposerSettingsLoader(t *testing.T) { hook := logtest.NewGlobal() + // Keys used by the per-key-replaces-db*.json testdata. + keyA := [fieldparams.BLSPubkeyLength]byte{0xaa} + keyB := [fieldparams.BLSPubkeyLength]byte{0xbb} + keyC := [fieldparams.BLSPubkeyLength]byte{0xcc} type proposerSettingsFlag struct { dir string url string @@ -45,7 +49,8 @@ func TestProposerSettingsLoader(t *testing.T) { urlResponse string wantInitErr string wantErr string - wantLog string + wantLogs []string + wantNoLogs []string withdb func(db iface.ValidatorDB) error validatorRegistrationEnabled bool skipDBSavedCheck bool @@ -235,7 +240,7 @@ func TestProposerSettingsLoader(t *testing.T) { want: func() *proposer.Settings { return nil }, - wantLog: "No proposer settings were provided", + wantLogs: []string{"No proposer settings were provided"}, skipDBSavedCheck: true, }, { @@ -288,8 +293,8 @@ func TestProposerSettingsLoader(t *testing.T) { }, } }, - wantErr: "", - wantLog: "is not a checksum Ethereum address", + wantErr: "", + wantLogs: []string{"is not a checksum Ethereum address"}, }, { name: "Happy Path Config file File multiple fee recipients", @@ -969,10 +974,112 @@ func TestProposerSettingsLoader(t *testing.T) { }, wantErr: "failed to unmarshal yaml file", }, + { + name: "file per-key entries replace db entries and warn with dropped and overridden keys", + args: args{ + proposerSettingsFlagValues: &proposerSettingsFlag{ + dir: "./testdata/per-key-replaces-db.json", + }, + }, + want: func() *proposer.Settings { + return &proposer.Settings{ + ProposeConfig: map[[fieldparams.BLSPubkeyLength]byte]*proposer.Option{ + keyA: &proposer.Option{FeeRecipientConfig: &proposer.FeeRecipientConfig{FeeRecipient: common.HexToAddress("0x3333333333333333333333333333333333333333")}}, + keyC: &proposer.Option{FeeRecipientConfig: &proposer.FeeRecipientConfig{FeeRecipient: common.HexToAddress("0x4444444444444444444444444444444444444444")}}, + }, + } + }, + withdb: func(db iface.ValidatorDB) error { + return db.SaveProposerSettings(t.Context(), &proposer.Settings{ + ProposeConfig: map[[fieldparams.BLSPubkeyLength]byte]*proposer.Option{ + keyA: &proposer.Option{FeeRecipientConfig: &proposer.FeeRecipientConfig{FeeRecipient: common.HexToAddress("0x1111111111111111111111111111111111111111")}}, + keyB: &proposer.Option{FeeRecipientConfig: &proposer.FeeRecipientConfig{FeeRecipient: common.HexToAddress("0x2222222222222222222222222222222222222222")}}, + }, + }) + }, + wantLogs: []string{ + "differ from the configured settings file/URL", + fmt.Sprintf("overriddenKeys=%#x", keyA), + fmt.Sprintf("droppedKeys=%#x", keyB), + "overriddenCount=1", + "droppedCount=1", + }, + }, + { + // Lowercase in the file, checksummed in the DB: the compare runs after normalization. + name: "file per-key entries identical to db entries do not warn", + args: args{ + proposerSettingsFlagValues: &proposerSettingsFlag{ + dir: "./testdata/per-key-identical-to-db.json", + }, + }, + want: func() *proposer.Settings { + return &proposer.Settings{ + ProposeConfig: map[[fieldparams.BLSPubkeyLength]byte]*proposer.Option{ + keyA: &proposer.Option{FeeRecipientConfig: &proposer.FeeRecipientConfig{FeeRecipient: common.HexToAddress("0xabcdefabcdefabcdefabcdefabcdefabcdefabcd")}}, + }, + } + }, + withdb: func(db iface.ValidatorDB) error { + return db.SaveProposerSettings(t.Context(), &proposer.Settings{ + ProposeConfig: map[[fieldparams.BLSPubkeyLength]byte]*proposer.Option{ + keyA: &proposer.Option{FeeRecipientConfig: &proposer.FeeRecipientConfig{FeeRecipient: common.HexToAddress("0xabcdefabcdefabcdefabcdefabcdefabcdefabcd")}}, + }, + }) + }, + wantNoLogs: []string{"differ from the configured settings file/URL"}, + }, + { + name: "file per-key entries with an empty db do not warn", + args: args{ + proposerSettingsFlagValues: &proposerSettingsFlag{ + dir: "./testdata/per-key-identical-to-db.json", + }, + }, + want: func() *proposer.Settings { + return &proposer.Settings{ + ProposeConfig: map[[fieldparams.BLSPubkeyLength]byte]*proposer.Option{ + keyA: &proposer.Option{FeeRecipientConfig: &proposer.FeeRecipientConfig{FeeRecipient: common.HexToAddress("0xabcdefabcdefabcdefabcdefabcdefabcdefabcd")}}, + }, + } + }, + wantNoLogs: []string{"differ from the configured settings file/URL"}, + }, + { + name: "replaced db key list is capped", + args: args{ + proposerSettingsFlagValues: &proposerSettingsFlag{ + dir: "./testdata/per-key-replaces-db-capped.json", + }, + }, + want: func() *proposer.Settings { + return &proposer.Settings{ + ProposeConfig: map[[fieldparams.BLSPubkeyLength]byte]*proposer.Option{ + {0xff}: &proposer.Option{FeeRecipientConfig: &proposer.FeeRecipientConfig{FeeRecipient: common.HexToAddress("0x1111111111111111111111111111111111111111")}}, + }, + } + }, + withdb: func(db iface.ValidatorDB) error { + settings := &proposer.Settings{ProposeConfig: map[[fieldparams.BLSPubkeyLength]byte]*proposer.Option{}} + for i := 0; i < 15; i++ { + settings.ProposeConfig[[fieldparams.BLSPubkeyLength]byte{byte(i)}] = &proposer.Option{FeeRecipientConfig: &proposer.FeeRecipientConfig{FeeRecipient: common.HexToAddress("0x1111111111111111111111111111111111111111")}} + } + return db.SaveProposerSettings(t.Context(), settings) + }, + wantLogs: []string{ + "differ from the configured settings file/URL", + "droppedCount=15", + "+5 more", + // Sorted hex: the 10th key (first byte 0x09) is listed, the 11th (0x0a) is behind the cap. + fmt.Sprintf("%#x", [fieldparams.BLSPubkeyLength]byte{0x09}), + }, + wantNoLogs: []string{fmt.Sprintf("%#x", [fieldparams.BLSPubkeyLength]byte{0x0a})}, + }, } for _, tt := range tests { for _, isSlashingProtectionMinimal := range [...]bool{false, true} { t.Run(fmt.Sprintf("%v-minimal:%v", tt.name, isSlashingProtectionMinimal), func(t *testing.T) { + hook.Reset() app := cli.App{} set := flag.NewFlagSet("test", 0) if tt.args.proposerSettingsFlagValues.dir != "" { @@ -1029,10 +1136,11 @@ func TestProposerSettingsLoader(t *testing.T) { } else { require.NoError(t, err) } - if tt.wantLog != "" { - assert.LogsContain(t, hook, - tt.wantLog, - ) + for _, want := range tt.wantLogs { + assert.LogsContain(t, hook, want) + } + for _, notWant := range tt.wantNoLogs { + assert.LogsDoNotContain(t, hook, notWant) } w := tt.want() require.DeepEqual(t, w, got) diff --git a/config/proposer/loader/testdata/per-key-identical-to-db.json b/config/proposer/loader/testdata/per-key-identical-to-db.json new file mode 100644 index 000000000000..ba0a5da4205e --- /dev/null +++ b/config/proposer/loader/testdata/per-key-identical-to-db.json @@ -0,0 +1,7 @@ +{ + "proposer_config": { + "0xaa0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000": { + "fee_recipient": "0xabcdefabcdefabcdefabcdefabcdefabcdefabcd" + } + } +} diff --git a/config/proposer/loader/testdata/per-key-replaces-db-capped.json b/config/proposer/loader/testdata/per-key-replaces-db-capped.json new file mode 100644 index 000000000000..aedbcfc2eba6 --- /dev/null +++ b/config/proposer/loader/testdata/per-key-replaces-db-capped.json @@ -0,0 +1,7 @@ +{ + "proposer_config": { + "0xff0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000": { + "fee_recipient": "0x1111111111111111111111111111111111111111" + } + } +} diff --git a/config/proposer/loader/testdata/per-key-replaces-db.json b/config/proposer/loader/testdata/per-key-replaces-db.json new file mode 100644 index 000000000000..ec5fb4ab8c1c --- /dev/null +++ b/config/proposer/loader/testdata/per-key-replaces-db.json @@ -0,0 +1,10 @@ +{ + "proposer_config": { + "0xaa0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000": { + "fee_recipient": "0x3333333333333333333333333333333333333333" + }, + "0xcc0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000": { + "fee_recipient": "0x4444444444444444444444444444444444444444" + } + } +}