Allow fetching sync committee duties for current and next period's epochs #9728
Conversation
| @@ -398,6 +434,19 @@ func TestGetSyncCommitteeDuties_SyncNotReady(t *testing.T) { | |||
| assert.ErrorContains(t, "Syncing to latest head, not ready to respond", err) | |||
| } | |||
|
|
|||
| func TestSyncCommitteeDutiesLastValidEpoch(t *testing.T) { | |||
There was a problem hiding this comment.
I don't particularly like this test because it duplicates a part of the implementation. But because I fetch a config value, which changes between regular and minimal configs, I cannot simply hardcode a number here.
| @@ -175,45 +186,55 @@ func (vs *Server) GetSyncCommitteeDuties(ctx context.Context, req *ethpbv2.SyncC | |||
| return nil, status.Error(codes.Unavailable, "Syncing to latest head, not ready to respond") | |||
| } | |||
|
|
|||
| slot, err := slots.EpochStart(req.Epoch) | |||
| currentSlot := slots.CurrentSlot(uint64(vs.TimeFetcher.GenesisTime().Unix())) | |||
There was a problem hiding this comment.
| currentSlot := slots.CurrentSlot(uint64(vs.TimeFetcher.GenesisTime().Unix())) | |
| currentSlot := vs.TimeFetcher.CurrentSlot() |
|
|
||
| var stateEpoch types.Epoch | ||
| // Integer division and multiplication do not cancel out. | ||
| currentSyncCommitteeFirstEpoch := (currentEpoch / params.BeaconConfig().EpochsPerSyncCommitteePeriod) * params.BeaconConfig().EpochsPerSyncCommitteePeriod |
There was a problem hiding this comment.
please use slots.SyncCommitteePeriodStartEpoch. It also uses SafeMul
| if req.Epoch >= currentSyncCommitteeFirstEpoch { | ||
| stateEpoch = currentSyncCommitteeFirstEpoch | ||
| } else { | ||
| stateEpoch = req.Epoch |
There was a problem hiding this comment.
this is not correct, stateEpoch should be slots.SyncCommitteePeriodStartEpoch(req.epoch)
There was a problem hiding this comment.
Does it make a difference? Won't duties be the same throughout the whole period because of the same committees returned from the state? I will change this to what you recommend either way.
There was a problem hiding this comment.
it wont make a difference here as i read through the function, luckily we are just getting the committee from the state, we are not sampling the committee. To sample the committee, we should always use the epoch boundary state as described here ethereum/consensus-specs#2394
|
|
||
| var stateEpoch types.Epoch | ||
| currentSyncCommitteeFirstEpoch, err := slots.SyncCommitteePeriodStartEpoch(currentEpoch / params.BeaconConfig().EpochsPerSyncCommitteePeriod) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.InvalidArgument, "Could not get sync committee period start epoch: %v.", err) | ||
| } | ||
| if req.Epoch >= currentSyncCommitteeFirstEpoch { | ||
| stateEpoch = currentSyncCommitteeFirstEpoch | ||
| } else { | ||
| stateEpoch, err = slots.SyncCommitteePeriodStartEpoch(req.Epoch) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.InvalidArgument, "Could not get sync committee period start epoch: %v.", err) | ||
| } | ||
| } | ||
| slot, err := slots.EpochStart(stateEpoch) |
There was a problem hiding this comment.
Final feedback. No need for the fancier if else condition. Since we already verified req.Epoch > lastValidEpoch, the following should be work
| var stateEpoch types.Epoch | |
| currentSyncCommitteeFirstEpoch, err := slots.SyncCommitteePeriodStartEpoch(currentEpoch / params.BeaconConfig().EpochsPerSyncCommitteePeriod) | |
| if err != nil { | |
| return nil, status.Errorf(codes.InvalidArgument, "Could not get sync committee period start epoch: %v.", err) | |
| } | |
| if req.Epoch >= currentSyncCommitteeFirstEpoch { | |
| stateEpoch = currentSyncCommitteeFirstEpoch | |
| } else { | |
| stateEpoch, err = slots.SyncCommitteePeriodStartEpoch(req.Epoch) | |
| if err != nil { | |
| return nil, status.Errorf(codes.InvalidArgument, "Could not get sync committee period start epoch: %v.", err) | |
| } | |
| } | |
| slot, err := slots.EpochStart(stateEpoch) | |
| requestedEpoch := req.Epoch | |
| if requestedEpoch > currentEpoch { | |
| requestedEpoch = currentEpoch | |
| } | |
| slot, err := slots.EpochStart(requestedEpoch) |
What type of PR is this?
Bug fix
What does this PR do? Why is it needed?
https://ethereum.github.io/beacon-APIs/?urls.primaryName=dev#/Validator/getSyncCommitteeDuties describes the
epochparameter asThis means that any epoch from the current or next sync committee period is valid.
Which issues(s) does this PR fix?
Fixes #9720