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

Fix CommitteeAssignments to not return every validator #14039

Merged
merged 2 commits into from
May 24, 2024

Conversation

terencechain
Copy link
Member

@terencechain terencechain commented May 22, 2024

Currently, CommitteeAssignments returns every active validator in the beacon state. With 1 million validators, this easily exceeds 32 MB in the heap. This PR addresses the issue by refactoring CommitteeAssignment into three parts:

  • verifyAssignmentEpoch: Verifies the requested epoch is correct with respect to the input beacon state.
  • ProposerAssignments: Takes state and epoch as inputs, uses verifyAssignmentEpoch, and returns map[primitives.ValidatorIndex][]primitives.Slot. All proposers of the epoch are returned, with only one proposer per slot.
  • CommitteeAssignments: Takes state, epoch, and requested_indices as inputs, uses verifyAssignmentEpoch, and returns map[primitives.ValidatorIndex]*CommitteeAssignment. The output is bounded by requested_indices.

Note to the reviewer: In ProposerAssignments, at the end of the function, I reset the beacon state to its original slot to prevent the function from mutating the slot.

// Reset state back to its original slot.
if err := state.SetSlot(originalStateSlot); err != nil {
    return nil, err
}

The previous behavior was incorrect and caused many inconsistent test false positives. The previous behavior was:

currentProposalEpoch := epoch < nextEpoch
if !currentProposalEpoch {
    if err := state.SetSlot(state.Slot() - params.BeaconConfig().SlotsPerEpoch); err != nil {
        return nil, nil, err

I'll document the false test behavior and the bug in the code diff.

@terencechain terencechain requested a review from a team as a code owner May 22, 2024 16:11
@terencechain terencechain force-pushed the assignment-container1 branch from 6e583b1 to 6a117eb Compare May 22, 2024 23:05
Comment on lines -126 to +142
nextSlotToEpoch := slots.ToEpoch(s.Slot() + 1)
nextEpoch := req.Epoch + 1
Copy link
Member Author

@terencechain terencechain May 22, 2024

Choose a reason for hiding this comment

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

This was wrong, but luckily it was not surfaced because the old CommitteeAssignments would mutate the slot. We should use the request epoch instead of the state slot for the query, and only update the next sync committee if the next epoch crosses the boundary as specified below.

@@ -1738,6 +1738,7 @@ func TestGetProposerDuties(t *testing.T) {
t.Run("next epoch", func(t *testing.T) {
bs, err := transition.GenesisBeaconState(context.Background(), deposits, 0, eth1Data)
require.NoError(t, err, "Could not set up genesis state")
require.NoError(t, bs.SetSlot(params.BeaconConfig().SlotsPerEpoch))
Copy link
Member Author

Choose a reason for hiding this comment

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

This was another test bug because the old CommitteeAssignments would mutate state slot. We should set the state slot here similar to what we did in the first test case

@terencechain terencechain force-pushed the assignment-container1 branch from 6a117eb to f7ec94a Compare May 22, 2024 23:16
@terencechain terencechain added the Ready For Review A pull request ready for code review label May 23, 2024
// we need to reset state slot back to start slot so that we can compute the correct committees.
currentProposalEpoch := epoch < nextEpoch
if !currentProposalEpoch {
if err := state.SetSlot(state.Slot() - params.BeaconConfig().SlotsPerEpoch); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

The function verifyAssignmentEpoch allows to check for a state that has a slot from a past epoch. However, we never perform an epoch transition on the state when checking and simply use SetSlot() which does not update balances. This could potentially give the wrong indices for both proposers and attesters when the state passed is from an epoch before the requested epoch.

vals[v] = struct{}{}
}

// Compute committee assignments for each slot in the epoch.
for i := primitives.Slot(0); i < params.BeaconConfig().SlotsPerEpoch; i++ {
Copy link
Contributor

Choose a reason for hiding this comment

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

This reads simpler as

Suggested change
for i := primitives.Slot(0); i < params.BeaconConfig().SlotsPerEpoch; i++ {
for slot := primitives.Slot(startSlot); slot < startSlot + params.BeaconConfig().SlotsPerEpoch; slot++ {

return nil, err
}

// Retrieve active validator indices for the specified epoch.
activeValidatorIndices, err := ActiveValidatorIndices(ctx, state, epoch)
Copy link
Contributor

Choose a reason for hiding this comment

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

This allocates a very large slice for no reason other than requesting its length in the next line 240. We only need to get the number of committees for the slot. No allocations should be performed here.

@terencechain terencechain force-pushed the assignment-container1 branch from a404d64 to afb414c Compare May 24, 2024 01:59
@terencechain terencechain force-pushed the assignment-container1 branch from afb414c to d220d4d Compare May 24, 2024 03:06
@terencechain terencechain added this pull request to the merge queue May 24, 2024
Merged via the queue into develop with commit c35889d May 24, 2024
16 of 17 checks passed
@terencechain terencechain deleted the assignment-container1 branch May 24, 2024 17:03
prestonvanloon pushed a commit that referenced this pull request May 31, 2024
* Rewrite CommitteeAssignments to not return every validator

* Potuz's feedback

(cherry picked from commit c35889d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants