-
Notifications
You must be signed in to change notification settings - Fork 995
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
Implement Stream Duties in Validator Client #5814
Conversation
@@ -37,7 +38,8 @@ func (vs *Server) StreamDuties(req *ethpb.DutiesRequest, stream ethpb.BeaconNode | |||
} | |||
|
|||
// We compute duties the very first time the endpoint is called. | |||
res, err := vs.duties(stream.Context(), req) | |||
currentEpoch := slotutil.EpochsSinceGenesis(vs.GenesisTimeFetcher.GenesisTime()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given duties are now a server-side stream, the beacon node needs to use an epoch ticker using roughtime to determine the duties instead of expecting an epoch argument in the grpc request
@@ -66,6 +68,7 @@ func (vs *Server) StreamDuties(req *ethpb.DutiesRequest, stream ethpb.BeaconNode | |||
case ev := <-stateChannel: | |||
// If a reorg occurred, we recompute duties for the connected validator clients | |||
// and send another response over the server stream right away. | |||
currentEpoch = slotutil.EpochsSinceGenesis(vs.GenesisTimeFetcher.GenesisTime()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a reorg occurs, we still need to use the clock time to determine duties
@@ -149,7 +152,7 @@ func (vs *Server) duties(ctx context.Context, req *ethpb.DutiesRequest) (*ethpb. | |||
} | |||
|
|||
return ðpb.DutiesResponse{ | |||
Duties: validatorAssignments, | |||
CurrentEpochDuties: validatorAssignments, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duties
is now deprecated. Instead, we send out current epoch duties and next epoch duties
@@ -300,108 +300,6 @@ func (v *validator) SlotDeadline(slot uint64) time.Time { | |||
return time.Unix(int64(v.genesisTime), 0 /*ns*/).Add(time.Duration(secs) * time.Second) | |||
} | |||
|
|||
// UpdateDuties checks the slot number to determine if the validator's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this to its own file for clarity and isolation
@@ -447,8 +345,8 @@ func (v *validator) RolesAt(ctx context.Context, slot uint64) (map[[48]byte][]va | |||
// UpdateProtections goes through the duties of the given slot and fetches the required validator history, | |||
// assigning it in validator. | |||
func (v *validator) UpdateProtections(ctx context.Context, slot uint64) error { | |||
attestingPubKeys := make([][48]byte, 0, len(v.duties.Duties)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The .Duties
type is deprecated
res, err := stream.Recv() | ||
// If the stream is closed, we stop the loop. | ||
if err == io.EOF { | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should we do here? Safe to break? Panic?
Closing in favor of #5867 |
What type of PR is this?
What does this PR do? Why is it needed?
This PR is the follow-up to #5685, which now uses the server-side stream to update the validator client and call out to this function. This PR also does a few amends to the server-side implementation, where there were a few mismatches in requirements.
Other notes for review