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.

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

validator startup deadline bug #12049

Merged
merged 20 commits into from
Mar 14, 2023
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
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
2 changes: 1 addition & 1 deletion validator/accounts/testing/mock.go
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ func (MockValidator) HasProposerSettings() bool {
}

// PushProposerSettings for mocking
func (_ MockValidator) PushProposerSettings(_ context.Context, _ keymanager.IKeymanager) error {
func (_ MockValidator) PushProposerSettings(_ context.Context, _ keymanager.IKeymanager, _ ...time.Duration) error {
panic("implement me")
}

Expand Down
2 changes: 1 addition & 1 deletion validator/client/iface/validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ type Validator interface {
ReceiveBlocks(ctx context.Context, connectionErrorChannel chan<- error)
HandleKeyReload(ctx context.Context, currentKeys [][fieldparams.BLSPubkeyLength]byte) (bool, error)
CheckDoppelGanger(ctx context.Context) error
PushProposerSettings(ctx context.Context, km keymanager.IKeymanager) error
PushProposerSettings(ctx context.Context, km keymanager.IKeymanager, deadline ...time.Duration) error
SignValidatorRegistrationRequest(ctx context.Context, signer SigningFunc, newValidatorRegistration *ethpb.ValidatorRegistrationV1) (*ethpb.SignedValidatorRegistrationV1, error)
ProposerSettings() *validatorserviceconfig.ProposerSettings
SetProposerSettings(*validatorserviceconfig.ProposerSettings)
Expand Down
3 changes: 2 additions & 1 deletion validator/client/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,8 @@ func run(ctx context.Context, v iface.Validator) {
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 {
deadline := time.Duration(params.BeaconConfig().SecondsPerSlot) * time.Second
if err := v.PushProposerSettings(ctx, km, deadline); err != nil {
if errors.Is(err, ErrBuilderValidatorRegistration) {
log.WithError(err).Warn("Push proposer settings error")
} else {
Expand Down
48 changes: 48 additions & 0 deletions validator/client/runner_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,54 @@ func TestUpdateProposerSettingsAt_EpochStart(t *testing.T) {
assert.LogsContain(t, hook, "updated proposer settings")
}

func TestUpdateProposerSettingsAt_EpochEndExceeded(t *testing.T) {
v := &testutil.FakeValidator{Km: &mockKeymanager{accountsChangedFeed: &event.Feed{}}, ProposerSettingWait: time.Duration(params.BeaconConfig().SecondsPerSlot+1) * time.Second}
v.SetProposerSettings(&validatorserviceconfig.ProposerSettings{
DefaultConfig: &validatorserviceconfig.ProposerOption{
FeeRecipientConfig: &validatorserviceconfig.FeeRecipientConfig{
FeeRecipient: common.HexToAddress("0x046Fb65722E7b2455012BFEBf6177F1D2e9738D9"),
},
},
})
ctx, cancel := context.WithCancel(context.Background())
hook := logTest.NewGlobal()
slot := params.BeaconConfig().SlotsPerEpoch - 1 //have it set close to the end of epoch
ticker := make(chan primitives.Slot)
v.NextSlotRet = ticker
go func() {
ticker <- slot
cancel()
}()

run(ctx, v)
// can't test "Failed to update proposer settings" because of log.fatal
assert.LogsContain(t, hook, "deadline exceeded")
}

func TestUpdateProposerSettingsAt_EpochEndOk(t *testing.T) {
v := &testutil.FakeValidator{Km: &mockKeymanager{accountsChangedFeed: &event.Feed{}}, ProposerSettingWait: time.Duration(params.BeaconConfig().SecondsPerSlot-1) * time.Second}
v.SetProposerSettings(&validatorserviceconfig.ProposerSettings{
DefaultConfig: &validatorserviceconfig.ProposerOption{
FeeRecipientConfig: &validatorserviceconfig.FeeRecipientConfig{
FeeRecipient: common.HexToAddress("0x046Fb65722E7b2455012BFEBf6177F1D2e9738D9"),
},
},
})
ctx, cancel := context.WithCancel(context.Background())
hook := logTest.NewGlobal()
slot := params.BeaconConfig().SlotsPerEpoch - 1 //have it set close to the end of epoch
ticker := make(chan primitives.Slot)
v.NextSlotRet = ticker
go func() {
ticker <- slot
cancel()
}()

run(ctx, v)
// can't test "Failed to update proposer settings" because of log.fatal
assert.LogsContain(t, hook, "Mock updated proposer settings")
}

func TestUpdateProposerSettings_ContinuesAfterValidatorRegistrationFails(t *testing.T) {
errSomeotherError := errors.New("some internal error")
v := &testutil.FakeValidator{
Expand Down
16 changes: 15 additions & 1 deletion validator/client/testutil/mock_validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ type FakeValidator struct {
PubkeyToIndexMap map[[fieldparams.BLSPubkeyLength]byte]uint64
PubkeysToStatusesMap map[[fieldparams.BLSPubkeyLength]byte]ethpb.ValidatorStatus
proposerSettings *validatorserviceconfig.ProposerSettings
ProposerSettingWait time.Duration
Km keymanager.IKeymanager
}

Expand Down Expand Up @@ -258,10 +259,23 @@ func (*FakeValidator) HasProposerSettings() bool {
}

// PushProposerSettings for mocking
func (fv *FakeValidator) PushProposerSettings(_ context.Context, _ keymanager.IKeymanager) error {
func (fv *FakeValidator) PushProposerSettings(ctx context.Context, _ keymanager.IKeymanager, deadline ...time.Duration) error {
if len(deadline) > 0 && deadline[0] > 0 {
nctx, cancel := context.WithTimeout(ctx, deadline[0])
ctx = nctx
defer cancel()
time.Sleep(fv.ProposerSettingWait)
if ctx.Err() == context.DeadlineExceeded {
log.Error("deadline exceeded")
// can't return error or it will trigger a log.fatal
return nil
}
}

if fv.ProposerSettingsErr != nil {
return fv.ProposerSettingsErr
}

log.Infoln("Mock updated proposer settings")
return nil
}
Expand Down
18 changes: 14 additions & 4 deletions validator/client/validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -985,13 +985,23 @@ func (v *validator) SetProposerSettings(settings *validatorserviceconfig.Propose
}

// PushProposerSettings calls the prepareBeaconProposer RPC to set the fee recipient and also the register validator API if using a custom builder.
func (v *validator) PushProposerSettings(ctx context.Context, km keymanager.IKeymanager) error {
func (v *validator) PushProposerSettings(ctx context.Context, km keymanager.IKeymanager, overrideDeadline ...time.Duration) error {
if km == nil {
return errors.New("keymanager is nil when calling PrepareBeaconProposer")
}

deadline := v.SlotDeadline(slots.RoundUpToNearestEpoch(slots.CurrentSlot(v.genesisTime)))
ctx, cancel := context.WithDeadline(ctx, deadline)
var deadline time.Time
if len(overrideDeadline) > 0 && overrideDeadline[0] > 0*time.Second {
Copy link
Member

Choose a reason for hiding this comment

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

why not just provide the deadline into the main loop ? rather than doing it this way where you have it as an optional parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated based on slack discussion on this feedback.

deadline = time.Now().Add(overrideDeadline[0])
} else {
d, deadlineSet := ctx.Deadline()
if deadlineSet {
deadline = d
} else {
deadline = v.SlotDeadline(slots.RoundUpToNearestEpoch(slots.CurrentSlot(v.genesisTime)))
}
}
nctx, cancel := context.WithDeadline(ctx, deadline)
ctx = nctx
defer cancel()

pubkeys, err := km.FetchValidatingPublicKeys(ctx)
Expand Down