Skip to content
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.

Sign up for GitHub

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

Keymanager API: fix fee recipient API and add persistence #11540

Merged
merged 4 commits into from
Oct 4, 2022
Merged

Keymanager API: fix fee recipient API and add persistence #11540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
00198a4
fixing bug with fee recipient api
james-prysm Oct 4, 2022
1080efb
fixing unit tests
james-prysm Oct 4, 2022
4c296fb
Merge branch 'develop' into persist-fee-recipient
james-prysm Oct 4, 2022
0d502fe
clarifying logs
james-prysm Oct 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions validator/accounts/testing/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ go_library(
"//validator:__subpackages__",
],
deps = [
"//config/validator/service:go_default_library",
"//consensus-types/primitives:go_default_library",
"//proto/prysm/v1alpha1:go_default_library",
"//validator/accounts/iface:go_default_library",
Expand Down
14 changes: 13 additions & 1 deletion validator/accounts/testing/mock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"sync"
"time"

validatorserviceconfig "github.com/prysmaticlabs/prysm/v3/config/validator/service"
types "github.com/prysmaticlabs/prysm/v3/consensus-types/primitives"
ethpb "github.com/prysmaticlabs/prysm/v3/proto/prysm/v1alpha1"
"github.com/prysmaticlabs/prysm/v3/validator/accounts/iface"
Expand Down Expand Up @@ -81,7 +82,8 @@ func (_ *Wallet) InitializeKeymanager(_ context.Context, _ iface.InitKeymanagerC
}

type MockValidator struct {
Km keymanager.IKeymanager
Km keymanager.IKeymanager
proposerSettings *validatorserviceconfig.ProposerSettings
}

func (_ MockValidator) LogSyncCommitteeMessagesSubmitted() {}
Expand Down Expand Up @@ -197,3 +199,13 @@ func (_ MockValidator) SetPubKeyToValidatorIndexMap(_ context.Context, _ keymana
func (_ MockValidator) SignValidatorRegistrationRequest(_ context.Context, _ iface2.SigningFunc, _ *ethpb.ValidatorRegistrationV1) (*ethpb.SignedValidatorRegistrationV1, error) {
panic("implement me")
}

// ProposerSettings for mocking
func (m *MockValidator) ProposerSettings() *validatorserviceconfig.ProposerSettings {
return m.proposerSettings
}

// SetProposerSettings for mocking
func (m *MockValidator) SetProposerSettings(settings *validatorserviceconfig.ProposerSettings) {
m.proposerSettings = settings
}
1 change: 1 addition & 0 deletions validator/client/iface/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ go_library(
visibility = ["//validator:__subpackages__"],
deps = [
"//config/fieldparams:go_default_library",
"//config/validator/service:go_default_library",
"//consensus-types/primitives:go_default_library",
"//crypto/bls:go_default_library",
"//proto/prysm/v1alpha1:go_default_library",
Expand Down
4 changes: 3 additions & 1 deletion validator/client/iface/validator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"time"

fieldparams "github.com/prysmaticlabs/prysm/v3/config/fieldparams"
validatorserviceconfig "github.com/prysmaticlabs/prysm/v3/config/validator/service"
types "github.com/prysmaticlabs/prysm/v3/consensus-types/primitives"
"github.com/prysmaticlabs/prysm/v3/crypto/bls"
ethpb "github.com/prysmaticlabs/prysm/v3/proto/prysm/v1alpha1"
Expand Down Expand Up @@ -60,9 +61,10 @@ type Validator interface {
ReceiveBlocks(ctx context.Context, connectionErrorChannel chan<- error)
HandleKeyReload(ctx context.Context, newKeys [][fieldparams.BLSPubkeyLength]byte) (bool, error)
CheckDoppelGanger(ctx context.Context) error
HasProposerSettings() bool
PushProposerSettings(ctx context.Context, km keymanager.IKeymanager) error
SignValidatorRegistrationRequest(ctx context.Context, signer SigningFunc, newValidatorRegistration *ethpb.ValidatorRegistrationV1) (*ethpb.SignedValidatorRegistrationV1, error)
ProposerSettings() *validatorserviceconfig.ProposerSettings
SetProposerSettings(*validatorserviceconfig.ProposerSettings)
}

// SigningFunc interface defines a type for the a function that signs a message
Expand Down
10 changes: 5 additions & 5 deletions validator/client/runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,9 @@ func run(ctx context.Context, v iface.Validator) {
}
sub := km.SubscribeAccountChanges(accountsChangedChan)
// Set properties on the beacon node like the fee recipient for validators that are being used & active.
if v.HasProposerSettings() {
log.Infof("Provided proposer settings will periodically update settings such as fee recipient"+
" in the beacon node and custom builder ( if --%s)", flags.EnableBuilderFlag.Name)
if v.ProposerSettings() != nil {
log.Infof("Validator client started with provided proposer settings that sets options such as fee recipient"+
" and will periodically update the beacon node and custom builder ( if --%s)", flags.EnableBuilderFlag.Name)
if err := v.PushProposerSettings(ctx, km); err != nil {
if errors.Is(err, ErrBuilderValidatorRegistration) {
log.WithError(err).Warn("Push proposer settings error")
Expand All @@ -67,7 +67,7 @@ func run(ctx context.Context, v iface.Validator) {
}
}
} else {
log.Info("Proposer settings such as fee recipient are not defined in the validator client" +
log.Warnln("Validator client started without proposer settings such as fee recipient" +
" and will continue to use settings provided in the beacon node.")
}

Expand Down Expand Up @@ -127,7 +127,7 @@ func run(ctx context.Context, v iface.Validator) {
continue
}

if slots.IsEpochStart(slot) && v.HasProposerSettings() {
if slots.IsEpochStart(slot) && v.ProposerSettings() != nil {
go func() {
//deadline set for next epoch rounded up
if err := v.PushProposerSettings(ctx, km); err != nil {
Expand Down
12 changes: 12 additions & 0 deletions validator/client/runner_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,12 @@ import (
"testing"
"time"

"github.com/ethereum/go-ethereum/common"
"github.com/pkg/errors"
"github.com/prysmaticlabs/prysm/v3/async/event"
fieldparams "github.com/prysmaticlabs/prysm/v3/config/fieldparams"
"github.com/prysmaticlabs/prysm/v3/config/params"
validatorserviceconfig "github.com/prysmaticlabs/prysm/v3/config/validator/service"
types "github.com/prysmaticlabs/prysm/v3/consensus-types/primitives"
"github.com/prysmaticlabs/prysm/v3/testing/assert"
"github.com/prysmaticlabs/prysm/v3/testing/require"
Expand Down Expand Up @@ -249,6 +251,11 @@ func TestKeyReload_RemoteKeymanager(t *testing.T) {

func TestUpdateProposerSettingsAt_EpochStart(t *testing.T) {
v := &testutil.FakeValidator{Km: &mockKeymanager{accountsChangedFeed: &event.Feed{}}}
v.SetProposerSettings(&validatorserviceconfig.ProposerSettings{
DefaultConfig: &validatorserviceconfig.ProposerOption{
FeeRecipient: common.HexToAddress("0x046Fb65722E7b2455012BFEBf6177F1D2e9738D9"),
},
})
ctx, cancel := context.WithCancel(context.Background())
hook := logTest.NewGlobal()
slot := params.BeaconConfig().SlotsPerEpoch
Expand All @@ -270,6 +277,11 @@ func TestUpdateProposerSettings_ContinuesAfterValidatorRegistrationFails(t *test
ProposerSettingsErr: errors.Wrap(ErrBuilderValidatorRegistration, errSomeotherError.Error()),
Km: &mockKeymanager{accountsChangedFeed: &event.Feed{}},
}
v.SetProposerSettings(&validatorserviceconfig.ProposerSettings{
DefaultConfig: &validatorserviceconfig.ProposerOption{
FeeRecipient: common.HexToAddress("0x046Fb65722E7b2455012BFEBf6177F1D2e9738D9"),
},
})
ctx, cancel := context.WithCancel(context.Background())
hook := logTest.NewGlobal()
slot := params.BeaconConfig().SlotsPerEpoch
Expand Down
15 changes: 12 additions & 3 deletions validator/client/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ type ValidatorService struct {
grpcHeaders []string
graffiti []byte
Web3SignerConfig *remoteweb3signer.SetupConfig
ProposerSettings *validatorserviceconfig.ProposerSettings
proposerSettings *validatorserviceconfig.ProposerSettings
}

// Config for the validator service.
Expand Down Expand Up @@ -120,7 +120,7 @@ func NewValidatorService(ctx context.Context, cfg *Config) (*ValidatorService, e
interopKeysConfig: cfg.InteropKeysConfig,
graffitiStruct: cfg.GraffitiStruct,
Web3SignerConfig: cfg.Web3SignerConfig,
ProposerSettings: cfg.ProposerSettings,
proposerSettings: cfg.ProposerSettings,
}

dialOpts := ConstructDialOptions(
Expand Down Expand Up @@ -204,7 +204,7 @@ func (v *ValidatorService) Start() {
graffitiOrderedIndex: graffitiOrderedIndex,
eipImportBlacklistedPublicKeys: slashablePublicKeys,
Web3SignerConfig: v.Web3SignerConfig,
ProposerSettings: v.ProposerSettings,
proposerSettings: v.proposerSettings,
walletInitializedChannel: make(chan *wallet.Wallet, 1),
}
// To resolve a race condition at startup due to the interface
Expand Down Expand Up @@ -248,6 +248,15 @@ func (v *ValidatorService) Keymanager() (keymanager.IKeymanager, error) {
return v.validator.Keymanager()
}

func (v *ValidatorService) ProposerSettings() *validatorserviceconfig.ProposerSettings {
return v.validator.ProposerSettings()
}

func (v *ValidatorService) SetProposerSettings(settings *validatorserviceconfig.ProposerSettings) {
v.proposerSettings = settings
v.validator.SetProposerSettings(settings)
Comment on lines +256 to +257
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have proposer settings in two places when we can read them from v.validator? This can cause bugs when we update in one place but not the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is what caused the original problem, unfortunately...

the runner.go has access to the validator directly but other parts such as the API only has access to the validator service and not the validator inside it... the node service at startup passes the data via config to the validator service into the validator as well ( the validator is only created with the values on validator service start). To try to remediate some of this I made the validator service's proposersettings private and calling the update this way will update both. open to other suggestions on this.

}

// ConstructDialOptions constructs a list of grpc dial options
func ConstructDialOptions(
maxCallRecvMsgSize int,
Expand Down
1 change: 1 addition & 0 deletions validator/client/testutil/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ go_library(
visibility = ["//validator:__subpackages__"],
deps = [
"//config/fieldparams:go_default_library",
"//config/validator/service:go_default_library",
"//consensus-types/primitives:go_default_library",
"//encoding/bytesutil:go_default_library",
"//proto/prysm/v1alpha1:go_default_library",
Expand Down
12 changes: 12 additions & 0 deletions validator/client/testutil/mock_validator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"time"

fieldparams "github.com/prysmaticlabs/prysm/v3/config/fieldparams"
validatorserviceconfig "github.com/prysmaticlabs/prysm/v3/config/validator/service"
types "github.com/prysmaticlabs/prysm/v3/consensus-types/primitives"
ethpb "github.com/prysmaticlabs/prysm/v3/proto/prysm/v1alpha1"
prysmTime "github.com/prysmaticlabs/prysm/v3/time"
Expand Down Expand Up @@ -51,6 +52,7 @@ type FakeValidator struct {
IndexToPubkeyMap map[uint64][fieldparams.BLSPubkeyLength]byte
PubkeyToIndexMap map[[fieldparams.BLSPubkeyLength]byte]uint64
PubkeysToStatusesMap map[[fieldparams.BLSPubkeyLength]byte]ethpb.ValidatorStatus
proposerSettings *validatorserviceconfig.ProposerSettings
Km keymanager.IKeymanager
}

Expand Down Expand Up @@ -270,3 +272,13 @@ func (_ *FakeValidator) SetPubKeyToValidatorIndexMap(_ context.Context, _ keyman
func (_ *FakeValidator) SignValidatorRegistrationRequest(_ context.Context, _ iface.SigningFunc, _ *ethpb.ValidatorRegistrationV1) (*ethpb.SignedValidatorRegistrationV1, error) {
return nil, nil
}

// ProposerSettings for mocking
func (f *FakeValidator) ProposerSettings() *validatorserviceconfig.ProposerSettings {
return f.proposerSettings
}

// SetProposerSettings for mocking
func (f *FakeValidator) SetProposerSettings(settings *validatorserviceconfig.ProposerSettings) {
f.proposerSettings = settings
}
28 changes: 16 additions & 12 deletions validator/client/validator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ type validator struct {
voteStats voteStats
syncCommitteeStats syncCommitteeStats
Web3SignerConfig *remoteweb3signer.SetupConfig
ProposerSettings *validatorserviceconfig.ProposerSettings
proposerSettings *validatorserviceconfig.ProposerSettings
walletInitializedChannel chan *wallet.Wallet
}

Expand Down Expand Up @@ -964,8 +964,12 @@ func (v *validator) logDuties(slot types.Slot, duties []*ethpb.DutiesResponse_Du
}
}

func (v *validator) HasProposerSettings() bool {
return v.ProposerSettings != nil
func (v *validator) ProposerSettings() *validatorserviceconfig.ProposerSettings {
return v.proposerSettings
}

func (v *validator) SetProposerSettings(settings *validatorserviceconfig.ProposerSettings) {
v.proposerSettings = settings
}

// PushProposerSettings calls the prepareBeaconProposer RPC to set the fee recipient and also the register validator API if using a custom builder.
Expand Down Expand Up @@ -1035,11 +1039,11 @@ func (v *validator) buildPrepProposerReqs(ctx context.Context, pubkeys [][fieldp
v.pubkeyToValidatorIndex[k] = i
}
feeRecipient := common.HexToAddress(params.BeaconConfig().EthBurnAddressHex)
if v.ProposerSettings.DefaultConfig != nil {
feeRecipient = v.ProposerSettings.DefaultConfig.FeeRecipient // Use cli config for fee recipient.
if v.ProposerSettings().DefaultConfig != nil {
feeRecipient = v.ProposerSettings().DefaultConfig.FeeRecipient // Use cli config for fee recipient.
}
if v.ProposerSettings.ProposeConfig != nil {
config, ok := v.ProposerSettings.ProposeConfig[k]
if v.ProposerSettings().ProposeConfig != nil {
config, ok := v.ProposerSettings().ProposeConfig[k]
if ok && config != nil {
feeRecipient = config.FeeRecipient // Use file config for fee recipient.
}
Expand All @@ -1065,16 +1069,16 @@ func (v *validator) buildSignedRegReqs(ctx context.Context, pubkeys [][fieldpara
feeRecipient := common.HexToAddress(params.BeaconConfig().EthBurnAddressHex)
gasLimit := params.BeaconConfig().DefaultBuilderGasLimit
enabled := false
if v.ProposerSettings.DefaultConfig != nil {
feeRecipient = v.ProposerSettings.DefaultConfig.FeeRecipient // Use cli config for fee recipient.
config := v.ProposerSettings.DefaultConfig.BuilderConfig
if v.ProposerSettings().DefaultConfig != nil {
feeRecipient = v.ProposerSettings().DefaultConfig.FeeRecipient // Use cli config for fee recipient.
config := v.ProposerSettings().DefaultConfig.BuilderConfig
if config != nil && config.Enabled {
gasLimit = uint64(config.GasLimit) // Use cli config for gas limit.
enabled = true
}
}
if v.ProposerSettings.ProposeConfig != nil {
config, ok := v.ProposerSettings.ProposeConfig[k]
if v.ProposerSettings().ProposeConfig != nil {
config, ok := v.ProposerSettings().ProposeConfig[k]
if ok && config != nil {
feeRecipient = config.FeeRecipient // Use file config for fee recipient.
builderConfig := config.BuilderConfig
Expand Down
Loading