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

Jump to bottom

refactor: use go ticker instead of timer #14134

Merged
merged 5 commits into from
Jun 25, 2024
Merged

refactor: use go ticker instead of timer #14134

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9ee840c
refactor: use go ticker instead of timer
hoanguyenkh Jun 24, 2024
fe1dea5
Merge branch 'develop' into hoa/refactor-ticker
james-prysm Jun 24, 2024
ae663ca
Merge branch 'develop' into hoa/refactor-ticker
saolyn Jun 24, 2024
e7c2007
Merge branch 'develop' into hoa/refactor-ticker
hoanguyenkh Jun 25, 2024
3d09783
Merge branch 'develop' into hoa/refactor-ticker
james-prysm Jun 25, 2024
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
7 changes: 3 additions & 4 deletions beacon-chain/rpc/prysm/v1alpha1/validator/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,14 +100,13 @@ func (vs *Server) WaitForActivation(req *ethpb.ValidatorActivationRequest, strea
}

waitTime := time.Duration(params.BeaconConfig().SecondsPerSlot) * time.Second
timer := time.NewTimer(waitTime)
defer timer.Stop()
ticker := time.NewTicker(waitTime)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to use a ticker here , instead using the New Timer because we only have a 1 time action. Wait for Activation only triggers if there are 0 active keys while accounts are updated. the action no longer needs to run afterwards

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The loop finishes when activeValidatorExists is true, and using NewTicker will remove the need for the manual timer.Reset(waitTime) call

Copy link
Contributor

Choose a reason for hiding this comment

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

ok that's fair, I think you're right on this and I was mistaken

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for this I'll go ahead and approve

defer ticker.Stop()

for {
timer.Reset(waitTime)
select {
// Pinging every slot for activation.
case <-timer.C:
case <-ticker.C:
activeValidatorExists, validatorStatuses, err := vs.activationStatus(stream.Context(), req.PublicKeys)
if err != nil {
return status.Errorf(codes.Internal, "Could not fetch validator status: %v", err)
Expand Down
Loading