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.

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

process_registry_updates without a full copy of the validator set #14130

Merged

Conversation

prestonvanloon
Copy link
Member

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

state.Validators() is a full copy of 1M+ validators. By using a series of validator index slices, we can reduce the worst case copy to ~8M bytes ~= 8Mb of indices to process.
The average case is near zero allocation.

Special shoutout to @terencechain for the idea and the debugging of my implementation.

Soft requirement for #14005.

Which issues(s) does this PR fix?

Other notes for review

I moved sortableIndices to its own file and added unit tests for this specifically. The lack of unit tests on this struct cost us nearly a full day of debugging a silly mistake.

@prestonvanloon prestonvanloon requested a review from a team as a code owner June 21, 2024 14:17
@prestonvanloon prestonvanloon force-pushed the process_registry_updates-readFromEveryValidator branch from 11ee9c3 to 3f55435 Compare June 21, 2024 14:18
@prestonvanloon prestonvanloon force-pushed the process_registry_updates-readFromEveryValidator branch from 3f55435 to 96f8db7 Compare June 21, 2024 14:19
beacon-chain/core/epoch/epoch_processing.go Show resolved Hide resolved
Comment on lines +78 to +88
// To avoid copying the state validator set via st.Validators(), we will perform a read only pass
// over the validator set while collecting validator indices where the validator copy is actually
// necessary, then we will process these operations.
eligibleForActivationQ := make([]primitives.ValidatorIndex, 0)
eligibleForActivation := make([]primitives.ValidatorIndex, 0)
eligibleForEjection := make([]primitives.ValidatorIndex, 0)

if err := st.ReadFromEveryValidator(func(idx int, val state.ReadOnlyValidator) error {
// Collect validators eligible to enter the activation queue.
if helpers.IsEligibleForActivationQueue(val, currentEpoch) {
eligibleForActivationQ = append(eligibleForActivationQ, primitives.ValidatorIndex(idx))
Copy link
Contributor

Choose a reason for hiding this comment

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

This pattern seems useful generically: we can probably split out a helper that takes a predicate and returns the list of validator indices that satisfies the predicate. We could have it take a slice of predicates and then return a slice of slices with the val indices (in this case it would be three predicates). PErhaps a benchmark shows that three loops vs 1 loop with three predicates is more or less the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think that would be a good addition as well. I am thinking something like

func (s BeaconState) FilterValidators(fns []func(ReadOnlyValidator) bool) [][]primitives.ValidatorIndex {
  res := make([]primitives.ValidatorIndex, len(fns))
  for i, v := range s.ReadOnlyValidators {
   for j, fn := range fns {
    if fn(v) {
     res[j] = append(res[j], primitives.ValidatorIndex(i))
    }
   }
  }
 return res
}

Can I add it in a follow up PR?

Copy link
Member

@nisdas nisdas left a comment

Choose a reason for hiding this comment

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

LGTM

@prestonvanloon prestonvanloon added this pull request to the merge queue Jun 27, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jun 27, 2024
@prestonvanloon prestonvanloon added this pull request to the merge queue Jun 27, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jun 27, 2024
@prestonvanloon prestonvanloon added this pull request to the merge queue Jun 27, 2024
Merged via the queue into develop with commit eab9daf Jun 27, 2024
16 of 17 checks passed
@prestonvanloon prestonvanloon deleted the process_registry_updates-readFromEveryValidator branch June 27, 2024 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants