persistent validator settings in validator client #12354

james-prysm · 2023-05-03T02:44:51Z

What type of PR is this?

Feature

What does this PR do? Why is it needed?

Currently validator settings from proposer-settings is never persisted in the validator client. Instead, fee recipients and validator registrations are persisted on the beacon node.

Downsides to the current approach.

settings are not persisted on restart if using MEV Boost if KeymanagerAPIs are used to populate the settings. This is because the validator registrations will expire after 3 epochs if we stop receiving them.
validator settings updated through keymanager API are not immediately active, instead will be scheduled to update every epoch

With the new approach

validator settings are saved to db in a bucket
bucket will be overridden if restarted with the proposer settings flag, but will update if restarted with default-fee-recipient or enable builder flags.
Keymanager APIs will save to bucket

Side Effects
Our gaslimit API was not working in the way that was intended, as it is only used for MEV-related items it will now return an error if those settings are not active or if no proposer settings exist in the first place.

Flag/DB/API combination effects
ps = proposer settings, vc = validator client

no ps in db + vc started with suggested fee recipient flag (+ enable builder flag)= new ps saved to db
no ps in db + vc started with proposer-settings-file or proposer-settings-url = new ps saved to db
no ps in db + vc started without flags + set fee recipient via keymanager api = new ps saved to db
ps in db + vc started without flags = validator starts with proposer settings from db
ps in db + vc started with suggested fee recipient flag (+ enabled builder flag) = update default settings in ps in db
ps in db + vc started with proposer-settings-file or proposer-settings-url = new ps saved to db
ps in db + vc started (with or without flags, see results above) + set fee recipient via keymanager api = update key's values in existing ps in db

Future things to consider

migrate persistent settings from beacon node to validator client
delete validator registration db in favor of cache ( currently behind a flag)

proto/prysm/v1alpha1/validator-client/keymanager.proto

validator/db/kv/proposer_settings.go

nisdas · 2023-05-10T09:27:37Z

validator/db/kv/proposer_settings.go

+		if err := proto.Unmarshal(b, to); err != nil {
+			return errors.Wrap(err, "failed to unmarshal proposer settings")
+		}
+		settings, err := validatorServiceConfig.ToSettings(to)


why can't we just modify the proto file directly here ? Or is it just much simpler to update settings ?

I can change this, it's just options was passed as the internal struct representation so i need to convert it into a proto type anyways. I can change this to converting the options to a proto type to update instead or just update the values of the proto if it's preferred.

nisdas · 2023-05-10T09:28:09Z

validator/db/kv/proposer_settings.go

+	err := s.db.Update(func(tx *bolt.Tx) error {
+		bkt := tx.Bucket(proposerSettingsBucket)
+		b := bkt.Get(proposerSettingsKey)
+		if len(b) == 0 {


will we always have a proposer settings object saved in the db ? is that enforced at startup

validator/db/kv/db.go will start with the bucket, and the check to see if it exists will trigger whether to update or to save a new copy

nisdas · 2023-05-10T09:30:48Z

validator/node/node.go

@@ -60,7 +62,7 @@ import (
 	"github.com/sirupsen/logrus"
 	"github.com/urfave/cli/v2"
 	"google.golang.org/protobuf/encoding/protojson"
-	"gopkg.in/yaml.v2"
+	"k8s.io/apimachinery/pkg/util/yaml"


any reason to change this to a different package ?

the k8s package will unmarshal yamls despsite only having json: in the struct, in our case the proto. since we manually wrote the json object that parses before I added yaml: settings but since i'm using the proto now it doesn't auto generate with it. the k8s library is more flexible in this case

nisdas · 2023-05-10T09:37:08Z

validator/node/node.go

+			pubkeyB := bytesutil.ToBytes48(decodedKey)
+			vpSettings.ProposeConfig[pubkeyB] = o
+		}
+		if psExists {


if no settings are saved don't you want to save them ? trying to understand the purpose of psExists here

right so if the default settings are being updated witth proposer-settings-file,--suggested-fee-recipient,--enable-builder it will update the existing data, whereas if we are restarting with the proposer settings populating specific settings for validators it should just reset the whole thing, otherwise there's no way to clear the set values. We could also introduce a flag to do this if we want...

if there is something that exists we save a new copy.

So proposer settings will only exist in the db, if its a fresh run with the proposer-settings file ?

proposer settings will load from db and not be overwritten if starting without proposer-settings-file. I'll write down all the cases in the description of the PR and transfer it to some docs page.

I updated the description with all the different cases to make this more clear.

rauljordan · 2023-05-10T12:20:26Z

config/validator/service/proposer-settings.go

+					FeeRecipient: common.HexToAddress(optionPayload.FeeRecipient),
+				},
+			}
+			if optionPayload.Builder != nil {


What if there is no fee recipient but there is a builder option. Is it fine to continue?

if there's no fee recipient it skips it , see on line 18

rauljordan · 2023-05-10T12:38:17Z

consensus-types/validator/custom_types.go

+
+// UnmarshalJSON custom unmarshal function for json
+func (u *Uint64) UnmarshalJSON(bs []byte) error {
+	str := string(bs) // Parse plain numbers directly.


Should do some length checks before the lines below

adding more tests.

terencechain · 2023-05-10T22:39:10Z

config/validator/service/proposer-settings.go

@@ -77,3 +123,81 @@ type ProposerOption struct {
 	FeeRecipientConfig *FeeRecipientConfig
 	BuilderConfig      *BuilderConfig
 }
+
+// Clone creates a deep copy of the proposer settings


Are these clone behaviors tested somewhere?

It's tested in the db tests but I'm working on adding very specific unit tests right now for clone.

terencechain · 2023-05-10T22:40:21Z

validator/client/service.go

@@ -188,10 +188,21 @@ func (v *ValidatorService) Start() {
 		return
 	}

+	// checks db if proposer settings exist if none is provided.
+	if v.proposerSettings == nil {
+		settings, err := v.db.ProposerSettings(v.ctx)


what happens if err is not nil?

it's going to start without db settings, will start with the flag settings, and if those are not provided it will fall back to the beacon node's settings

terencechain · 2023-05-10T22:45:41Z

validator/db/kv/proposer_settings.go

+}
+
+// ProposerSettingsExists returns true or false if the settings exist or not
+func (s *Store) ProposerSettingsExists(ctx context.Context) (bool, error) {


mostly covered by the node_test.go will have other tests to cover related.

terencechain · 2023-05-10T22:46:25Z

validator/db/kv/proposer_settings.go

+
+// UpdateProposerSettingsForPubkey updates the existing settings for an internal representation of the proposers settings file at a particular public key
+func (s *Store) UpdateProposerSettingsForPubkey(ctx context.Context, pubkey [fieldparams.BLSPubkeyLength]byte, options *validatorServiceConfig.ProposerOption) error {
+	_, span := trace.StartSpan(ctx, "validator.db.UpdateProposerSettingsDefault")


Wrong span name

rauljordan · 2023-05-16T16:54:50Z

config/validator/service/proposer_settings.go

+			}
+			b, err := hexutil.Decode(key)
+			if err != nil {
+				return nil, err


We should add info about what the bad key was that we were trying to decode here

errorf perhaps and %s

james-prysm added 4 commits April 28, 2023 15:58

WIP

1ed6655

improving proposer settings store

7b73e87

WIP persistent validator settings

53a4b2f

WIP persistent validator settings

2677b98

james-prysm self-assigned this May 3, 2023

Merge branch 'develop' into db-proposer-settings

80cc2b2

james-prysm added API Api related tasks Builder PR or issue that supports builder related work labels May 3, 2023

james-prysm and others added 22 commits May 2, 2023 21:48

changing visibility level

05ceecc

fixing some deepsource issues

02b0856

fixing more deepsource issues

9be0bf6

fixing json marshalling

8327663

Merge branch 'develop' into db-proposer-settings

c76e44a

fix linting

3614ea8

fixing tests

915defb

fixing more tests

7594faa

fixing more tests

60b5724

fixing more tests

b068077

fixing linting

465ea7d

WIP fixing unit tests

fd37664

fixing remaining db tests

3eb463d

converting json to protobuf

d40fdd4

Merge branch 'develop' into db-proposer-settings

bff330a

fixing e2e

9af83d5

k8s yaml library is used directly

aef3998

fixing linting

ca80b84

fixing broken unit test

250fdf2

reverting changes on e2e

75329be

fixing linting

f099fa7

fixing deepsource issue

20b6b69

james-prysm commented May 8, 2023

View reviewed changes

proto/prysm/v1alpha1/validator-client/keymanager.proto Show resolved Hide resolved

james-prysm commented May 8, 2023

View reviewed changes

validator/db/kv/proposer_settings.go Show resolved Hide resolved

resolving some internal comments

44c7aea

james-prysm requested review from prestonvanloon and nisdas and removed request for potuz May 8, 2023 16:27

nisdas reviewed May 10, 2023

View reviewed changes

rauljordan reviewed May 10, 2023

View reviewed changes

Merge branch 'develop' into db-proposer-settings

222926c

terencechain reviewed May 10, 2023

View reviewed changes

james-prysm and others added 8 commits May 10, 2023 20:45

resolving some comments and adding more tests

c5c1cb4

adding more unit tests

659087c

gaz

cb3ac2a

fixing flaking test

5429807

fixing unit test contamination

3e8a773

fixing deepsource issue

3ec92f7

Merge branch 'develop' into db-proposer-settings

a637e07

Merge branch 'develop' into db-proposer-settings

001a63a

james-prysm mentioned this pull request May 15, 2023

Unregister validator - fix behind feature flag #12316

Merged

Merge branch 'develop' into db-proposer-settings

1a072a4

rauljordan reviewed May 16, 2023

View reviewed changes

james-prysm and others added 3 commits May 16, 2023 12:28

resolving review item

51a6a3d

Merge branch 'develop' into db-proposer-settings

1bd9d75

gaz

23f52e1

rauljordan approved these changes May 16, 2023

View reviewed changes

james-prysm merged commit d907cae into develop May 16, 2023

james-prysm deleted the db-proposer-settings branch May 16, 2023 19:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

persistent validator settings in validator client #12354

persistent validator settings in validator client #12354

james-prysm commented May 3, 2023 •

edited

Loading

nisdas May 10, 2023

james-prysm May 10, 2023

nisdas May 10, 2023

james-prysm May 10, 2023

nisdas May 10, 2023

james-prysm May 10, 2023

nisdas May 10, 2023

james-prysm May 10, 2023

nisdas May 11, 2023

james-prysm May 11, 2023

james-prysm May 11, 2023

rauljordan May 10, 2023

james-prysm May 10, 2023

rauljordan May 10, 2023

james-prysm May 11, 2023

terencechain May 10, 2023

james-prysm May 10, 2023

terencechain May 10, 2023

james-prysm May 11, 2023

terencechain May 10, 2023

james-prysm May 11, 2023

terencechain May 10, 2023

rauljordan May 16, 2023

rauljordan May 16, 2023

james-prysm May 16, 2023

persistent validator settings in validator client #12354

persistent validator settings in validator client #12354

Conversation

james-prysm commented May 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-prysm commented May 3, 2023 •

edited

Loading