adding the duties stores for validator duties processing - #16479
Conversation
|
tested on kurtosis |
| needsCurrDependentRootUpdate := v.duties == nil || !bytes.Equal(currDepedentRoot, v.duties.CurrDependentRoot) | ||
| v.dutiesLock.RUnlock() | ||
| if !needsCurrDependentRootUpdate { | ||
| needsCurrUpdate := storedCurr == nil || !bytes.Equal(currDependentRoot, storedCurr) |
There was a problem hiding this comment.
should this be read again within a lock? could it possibly be outdated? previously this was read within a lock.
There was a problem hiding this comment.
ok yeah good point will fix this one
| func (ds *dutyStore) CurrentEpochDuties() map[pubkey]*ethpb.ValidatorDuty { | ||
| if !ds.IsInitialized() { | ||
| return nil | ||
| } | ||
| return ds.currentDuties | ||
| } |
There was a problem hiding this comment.
is it okay to return the actual map by reference in these getters? or should they return a copy? same for the other methods.
There was a problem hiding this comment.
maybe I should wrap it so it doesn't mutate? nothing mutates the map right now
There was a problem hiding this comment.
i think it might be overkill, before it returned a slice which callers could mutate too. i'm less worried about this one unless you think there's some other concern
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| // SetLegacy stores a legacy combined duties response by decomposing it into | ||
| // duty maps, proposer slots, and sync committee maps. | ||
| func (ds *dutyStore) SetLegacy(container *ethpb.ValidatorDutiesContainer) { |
There was a problem hiding this comment.
I understand your naming for this, but i don't agree with it. The naming of this method is not very idiomatic. Can you give it a proper name? If it's deprecated, then make it // DEPRECATED and provide a comment to offer the new API that folks should use.
There was a problem hiding this comment.
i.e. something like this:
| // SetLegacy stores a legacy combined duties response by decomposing it into | |
| // duty maps, proposer slots, and sync committee maps. | |
| func (ds *dutyStore) SetLegacy(container *ethpb.ValidatorDutiesContainer) { | |
| // SetFromCombinedDutiesResponse stores a legacy combined duties response by decomposing it into | |
| // duty maps, proposer slots, and sync committee maps. | |
| // DEPRECATED: Use the ?? endpoint from gloas onwards. | |
| func (ds *dutyStore) SetFromCombinedDutiesResponse(container *ethpb.ValidatorDutiesContainer) { |
There was a problem hiding this comment.
I added with the endpoint that is deprecating and what to use instead ( which isn't merged in yet)
There was a problem hiding this comment.
this is the related pr that introduces the new endpoints #16416
Co-authored-by: Preston Van Loon <pvanloon@offchainlabs.com>
…s#16479) **What type of PR is this?** Other **What does this PR do? Why is it needed?** This PR refactors the way we store different validator duties into a duties store for easier splitting of tasks and in a future pr processing duties for split endpoints. This PR will reduce the number of changes when we start calling the different endpoints introduced in OffchainLabs#16416 pr is part of OffchainLabs#16421 **Which issues(s) does this PR fix?** Fixes # **Other notes for review** **Acknowledgements** - [x] I have read [CONTRIBUTING.md](https://github.com/prysmaticlabs/prysm/blob/develop/CONTRIBUTING.md). - [x] I have included a uniquely named [changelog fragment file](https://github.com/prysmaticlabs/prysm/blob/develop/CONTRIBUTING.md#maintaining-changelogmd). - [x] I have added a description with sufficient context for reviewers to understand this PR. - [x] I have tested that my changes work as expected and I added a testing plan to the PR description (if applicable). --------- Co-authored-by: Preston Van Loon <pvanloon@offchainlabs.com>
3 participants
What type of PR is this?
Other
What does this PR do? Why is it needed?
This PR refactors the way we store different validator duties into a duties store for easier splitting of tasks and in a future pr processing duties for split endpoints. This PR will reduce the number of changes when we start calling the different endpoints introduced in #16416
pr is part of #16421
Which issues(s) does this PR fix?
Fixes #
Other notes for review
Acknowledgements