From 78f8411ad22d8bc792242bf2fc897eaacc97607e Mon Sep 17 00:00:00 2001 From: james-prysm <90280386+james-prysm@users.noreply.github.com> Date: Fri, 11 Jul 2025 14:39:52 -0500 Subject: [PATCH] move validator run slot ticker (#15479) * moving the ticker from chain start to right before the main loop and also after the wait for activation edge case * fixing unit test * adding in a unit test * adding in comment based on potuz's feedback --- changelog/james-prysm_move-ticker.md | 3 +++ testing/validator-mock/validator_mock.go | 12 ++++++++++ time/slots/slottime_test.go | 25 +++++++++++++++++++++ validator/client/iface/validator.go | 1 + validator/client/runner.go | 6 +++-- validator/client/testutil/mock_validator.go | 3 +++ validator/client/validator.go | 19 +++++++++++++--- validator/client/validator_test.go | 2 -- 8 files changed, 64 insertions(+), 7 deletions(-) create mode 100644 changelog/james-prysm_move-ticker.md diff --git a/changelog/james-prysm_move-ticker.md b/changelog/james-prysm_move-ticker.md new file mode 100644 index 0000000000..a5c6982ace --- /dev/null +++ b/changelog/james-prysm_move-ticker.md @@ -0,0 +1,3 @@ +### Fixed + +- Fixes edge case starting validator client with new validator keys starts the slot ticker too early resulting in replayed slots in the main runner loop. Fixes edge case of replayed slots when waiting for account acivations. \ No newline at end of file diff --git a/testing/validator-mock/validator_mock.go b/testing/validator-mock/validator_mock.go index 002f97f7d6..d4a6d63782 100644 --- a/testing/validator-mock/validator_mock.go +++ b/testing/validator-mock/validator_mock.go @@ -362,6 +362,18 @@ func (mr *MockValidatorMockRecorder) SetProposerSettings(arg0, arg1 any) *gomock return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetProposerSettings", reflect.TypeOf((*MockValidator)(nil).SetProposerSettings), arg0, arg1) } +// SetTicker mocks base method. +func (m *MockValidator) SetTicker() { + m.ctrl.T.Helper() + m.ctrl.Call(m, "SetTicker") +} + +// SetTicker indicates an expected call of SetTicker. +func (mr *MockValidatorMockRecorder) SetTicker() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetTicker", reflect.TypeOf((*MockValidator)(nil).SetTicker)) +} + // SignValidatorRegistrationRequest mocks base method. func (m *MockValidator) SignValidatorRegistrationRequest(arg0 context.Context, arg1 iface.SigningFunc, arg2 *eth.ValidatorRegistrationV1) (*eth.SignedValidatorRegistrationV1, bool, error) { m.ctrl.T.Helper() diff --git a/time/slots/slottime_test.go b/time/slots/slottime_test.go index ac4329074e..11225151e4 100644 --- a/time/slots/slottime_test.go +++ b/time/slots/slottime_test.go @@ -1,6 +1,7 @@ package slots import ( + "context" "math" "testing" "time" @@ -701,3 +702,27 @@ func TestToForkVersion(t *testing.T) { require.Equal(t, version.Phase0, result) }) } + +func TestSlotTickerReplayBehaviour(t *testing.T) { + secondsPerslot := uint64(1) + st := NewSlotTicker(time.Unix(time.Now().Unix(), 0), secondsPerslot) // 1-second period + const ticks = 5 + + ctx, cancel := context.WithTimeout(t.Context(), 6*time.Second) // make the timeout very close + defer cancel() + time.Sleep(time.Duration(ticks) * time.Second) // simulate slow consumer by delaying tick consumption + counter := 0 + prevTime := time.Now() + for counter < ticks { + select { + case <-st.C(): // simulate ticks faster than supposed iteration due to replaying old ticks + assert.Equal(t, true, time.Now().Sub(prevTime) < time.Duration(secondsPerslot)*time.Second) + counter++ + prevTime = time.Now() + case <-ctx.Done(): // timed out before enough ticks arrived + t.Fatalf("expected %d ticks, got %d", ticks, counter) + } + } + + require.Equal(t, ticks, counter) +} diff --git a/validator/client/iface/validator.go b/validator/client/iface/validator.go index 0f40d48987..73c814a2d5 100644 --- a/validator/client/iface/validator.go +++ b/validator/client/iface/validator.go @@ -70,6 +70,7 @@ type Validator interface { DeleteGraffiti(ctx context.Context, pubKey [fieldparams.BLSPubkeyLength]byte) error Host() string FindHealthyHost(ctx context.Context) bool + SetTicker() } // SigningFunc interface defines a type for the function that signs a message diff --git a/validator/client/runner.go b/validator/client/runner.go index e3c5aaaab6..f57810c3b7 100644 --- a/validator/client/runner.go +++ b/validator/client/runner.go @@ -67,7 +67,6 @@ func newRunner(ctx context.Context, v iface.Validator, monitor *healthMonitor) ( v.Done() return nil, errors.Wrap(err, "failed to update proposer settings") } - return &runner{ validator: v, healthMonitor: monitor, @@ -86,7 +85,7 @@ func (r *runner) run(ctx context.Context) { v := r.validator cleanup := v.Done defer cleanup() - + v.SetTicker() for { select { case <-ctx.Done(): @@ -169,6 +168,9 @@ func onAccountsChanged(ctx context.Context, v iface.Validator, current [][48]byt err := v.WaitForActivation(ctx) if err != nil { log.WithError(err).Warn("Could not wait for validator activation") + } else { + log.Debug("Resetting slot ticker after waiting for validator activation.") + v.SetTicker() } } } diff --git a/validator/client/testutil/mock_validator.go b/validator/client/testutil/mock_validator.go index 1d02fbea02..1babe30d6a 100644 --- a/validator/client/testutil/mock_validator.go +++ b/validator/client/testutil/mock_validator.go @@ -333,3 +333,6 @@ func (*FakeValidator) Host() string { func (fv *FakeValidator) FindHealthyHost(_ context.Context) bool { return fv.CanChangeHost } + +func (fv *FakeValidator) SetTicker() { +} diff --git a/validator/client/validator.go b/validator/client/validator.go index 71bb0c0c67..fcbb5205e6 100644 --- a/validator/client/validator.go +++ b/validator/client/validator.go @@ -310,7 +310,6 @@ func (v *validator) WaitForChainStart(ctx context.Context) error { return errors.Wrap(err, "could not save genesis validators root") } - v.setTicker() return nil } @@ -328,11 +327,25 @@ func (v *validator) WaitForChainStart(ctx context.Context) error { ) } - v.setTicker() return nil } -func (v *validator) setTicker() { +func (v *validator) SetTicker() { + // If a ticker already exists, stop it before creating a new one + // to prevent resource leaks. + + // note to reader: + // This function chooses to adapt to the existing slot ticker instead of changing how it works + // The slot ticker will currently start from genesis time but tick based on the current time. + // This means that sometimes we need to reset the ticker to avoid replaying old ticks on a slow consumer of the ticks. + // i.e., + // 1. tick starts at 0 + // 2. loop stops consuming on slot 10 due to accounts changed tigger with no active keys + // 3. new active keys are added in slot 20 resolving wait for activation + // 4. new tick starts ticking from slot 20 instead of slot 10 + if v.ticker != nil { + v.ticker.Done() + } // Once the ChainStart log is received, we update the genesis time of the validator client // and begin a slot ticker used to track the current slot the beacon node is in. v.ticker = slots.NewSlotTicker(time.Unix(int64(v.genesisTime), 0), params.BeaconConfig().SecondsPerSlot) diff --git a/validator/client/validator_test.go b/validator/client/validator_test.go index 8f4f04f3d3..39121ebf23 100644 --- a/validator/client/validator_test.go +++ b/validator/client/validator_test.go @@ -192,7 +192,6 @@ func TestWaitForChainStart_SetsGenesisInfo(t *testing.T) { assert.DeepEqual(t, genesisValidatorsRoot[:], savedGenValRoot, "Unexpected saved genesis validators root") assert.Equal(t, genesis, v.genesisTime, "Unexpected chain start time") - assert.NotNil(t, v.ticker, "Expected ticker to be set, received nil") // Make sure there are no errors running if it is the same data. client.EXPECT().WaitForChainStart( @@ -236,7 +235,6 @@ func TestWaitForChainStart_SetsGenesisInfo_IncorrectSecondTry(t *testing.T) { assert.DeepEqual(t, genesisValidatorsRoot[:], savedGenValRoot, "Unexpected saved genesis validators root") assert.Equal(t, genesis, v.genesisTime, "Unexpected chain start time") - assert.NotNil(t, v.ticker, "Expected ticker to be set, received nil") genesisValidatorsRoot = bytesutil.ToBytes32([]byte("badvalidators"))