Register sync subnet when fetching sync committee duties through Beacon API #12972
Conversation
| @@ -978,15 +978,16 @@ func (s *Server) GetSyncCommitteeDuties(w http.ResponseWriter, r *http.Request) | |||
| return | |||
| } | |||
| nextSyncCommitteeFirstEpoch := currentSyncCommitteeFirstEpoch + params.BeaconConfig().EpochsPerSyncCommitteePeriod | |||
| currentCommitteeRequested := requestedEpoch < nextSyncCommitteeFirstEpoch | |||
There was a problem hiding this comment.
suggested rename to indicate a boolean isCurrentCommitteeRequested
| @@ -997,11 +998,38 @@ func (s *Server) GetSyncCommitteeDuties(w http.ResponseWriter, r *http.Request) | |||
| pubkey48 := bytesutil.ToBytes48(pubkey) | |||
| committeePubkeys[pubkey48] = append(committeePubkeys[pubkey48], strconv.FormatUint(uint64(j), 10)) | |||
| } | |||
| duties, err := syncCommitteeDuties(requestedValIndices, st, committeePubkeys) | |||
| duties, vals, err := syncCommitteeDutiesAndVals(requestedValIndices, st, committeePubkeys) | |||
There was a problem hiding this comment.
it might more readable if this function was duplicated instead of having the if statements duplicated
or you can set a function inside to alias the function like
var operation (s beaconState.BeaconState, epoch primitives.Epoch, pubKey []byte, status ethpb.ValidatorStatus) error
operation = core.RegisterSyncSubnetCurrentPeriod or core.RegisterSyncSubnetNextPeriod
and use
operation(st, requestedEpoch, pk[:], valStatus)
this way you can skip copying and pasting most of the loop or even put it in its own function.
| @@ -1169,29 +1197,35 @@ func syncCommitteeDutiesLastValidEpoch(currentEpoch primitives.Epoch) primitives | |||
| return (currentSyncPeriodIndex+2)*params.BeaconConfig().EpochsPerSyncCommitteePeriod - 1 | |||
| } | |||
|
|
|||
| func syncCommitteeDuties( | |||
| func syncCommitteeDutiesAndVals( | |||
There was a problem hiding this comment.
this function seems to be doing a lot and taking in a bunch of different arguments to do its tasks, i wonder if we can do something about that..
maybe we can get rid of some of this and break some of this up by moving this logic into where request indices is processed
requestedValIndices := make([]primitives.ValidatorIndex, len(indices))
for i, ix := range indices {
valIx, valid := shared.ValidateUint(w, fmt.Sprintf("ValidatorIndices[%d]", i), ix)
if !valid {
return
}
requestedValIndices[i] = primitives.ValidatorIndex(valIx)
}
There was a problem hiding this comment.
It's difficult to split this function without doing some of the work twice. I reordered params and added a comment, hope that's ok.
beacon-chain/rpc/core/validator.go
Outdated
| // registerSyncSubnet checks the status and pubkey of a particular validator | ||
| // to discern whether persistent subnets need to be registered for them. | ||
| func registerSyncSubnetInternal( |
There was a problem hiding this comment.
This comment seems to belong to the function on line 532
| @@ -483,3 +484,122 @@ func (s *Service) SubmitSyncMessage(ctx context.Context, msg *ethpb.SyncCommitte | |||
| // Wait for p2p broadcast to complete and return the first error (if any) | |||
| return errs.Wait() | |||
| } | |||
|
|
|||
| // RegisterSyncSubnetCurrentPeriod registers a persistent subnet for the current sync committee period. | |||
| func RegisterSyncSubnetCurrentPeriod(s beaconState.BeaconState, epoch primitives.Epoch, pubKey []byte, status validator.ValidatorStatus) error { | |||
There was a problem hiding this comment.
Most of these added functions don't appear to have any unit tests.
There was a problem hiding this comment.
The tests in validator_test.go already existed in v1alpha1. We don't add any new tests to the core service because the code is tested indirectly through handlers and v1alpha1 endpoints.
What type of PR is this?
Other
What does this PR do? Why is it needed?
When fetching validator duties through v1alpha1, we register sync subnets. This was never present in the Beacon API.
Registration logic is moved from
beacon-chain/rpc/prysm/v1alpha1/validator/assignments.gotobeacon-chain/rpc/core/validator.go. There are two pairs of functions, with and withoutProtosuffix, to deal with two different types representing the validator's status.Which issues(s) does this PR fix?
Part of #12958