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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Aggregate subsets #3493

Closed
wants to merge 16 commits into from
Closed

[Merged by Bors] - Aggregate subsets #3493

wants to merge 16 commits into from

Conversation

pawanjay176
Copy link
Member

Issue Addressed

Resolves #3238

Proposed Changes

Please list or describe the changes introduced by this PR.

Additional Info

Please provide any additional information. For example, future considerations
or information useful for reviewers.

@paulhauner paulhauner added the work-in-progress PR is a work-in-progress label Sep 29, 2022
@pawanjay176
Copy link
Member Author

Screenshot 2023-05-23 at 4 03 19 PM

Initial results are looking good. We are rejecting almost double the number of attestation aggregates as subsets compared to before which is resulting in a corresponding decrease in outbound bandwidth on the topic. Sync contributions have also reduced but not as much since there are fewer aggregators and only 4 aggregate bits.

Gonna clean this up a bit to get it ready for review.

@pawanjay176 pawanjay176 added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels May 25, 2023
@pawanjay176 pawanjay176 marked this pull request as ready for review May 25, 2023 22:43
@pawanjay176
Copy link
Member Author

Running this on prater now, will run on mainnet next if results are good.

@pawanjay176
Copy link
Member Author

This is looking good on prater.
@michaelsproul Is there any metric I can look at to check that the aggregates produced are of similar quality compared to before in terms of rewards? Basically, I want to make sure that dropping the subset aggregates isn't leading to to reduced rewards for some reason. Or is it not necessary?

@pawanjay176
Copy link
Member Author

Okay confirmed with @michaelsproul that rewards are same after this PR.
Ready for review now.

AgeManning
AgeManning approved these changes Jun 7, 2023
View reviewed changes
Copy link
Member

@AgeManning AgeManning left a comment

Choose a reason for hiding this comment

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

Yeah nice! It looks like a subset is inclusive if the bitfields are identical, which is what we want to reduce duplicates. So lgtm

michaelsproul
michaelsproul requested changes Jun 8, 2023
View reviewed changes
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

A few tiny fixes and one substantive suggestion. Keen to hear your thoughts.

The bandwidth improvements from this look awesome, I'm keen to get it released!

beacon_node/beacon_chain/src/observed_aggregates.rs Outdated Show resolved Hide resolved
beacon_node/beacon_chain/src/observed_aggregates.rs Outdated Show resolved Hide resolved
beacon_node/beacon_chain/src/sync_committee_verification.rs Outdated Show resolved Hide resolved
@michaelsproul michaelsproul added waiting-on-author The reviewer has suggested changes and awaits thier implementation. v4.3.0 Estimated Q2 2023 and removed ready-for-review The code is ready for review labels Jun 8, 2023
@michaelsproul michaelsproul mentioned this pull request Jun 13, 2023
Open
@pawanjay176
Copy link
Member Author

Deployed this with michael's suggestion on prater to check if we see similar behaviour.
Gonna let this run for the weekend.

@pawanjay176 pawanjay176 added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jun 24, 2023
@pawanjay176
Copy link
Member Author

Seeing similar results as before 🎉 (Note: the spike is when I deployed a buggy version before fixing it)

Screenshot 2023-06-26 at 11 35 00 AM

michaelsproul
michaelsproul approved these changes Jun 27, 2023
View reviewed changes
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

LGTM, let's goooo 🚀

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Jun 27, 2023
@michaelsproul
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Jun 27, 2023
@pawanjay176
Aggregate subsets (#3493)
448d3ec 
## Issue Addressed

Resolves #3238 

## Proposed Changes

Please list or describe the changes introduced by this PR.

## Additional Info

Please provide any additional information. For example, future considerations
or information useful for reviewers.
@bors
Copy link

bors bot commented Jun 27, 2023

Pull request successfully merged into unstable.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot changed the title Aggregate subsets [Merged by Bors] - Aggregate subsets Jun 27, 2023
@bors bors bot closed this Jun 27, 2023
@pawanjay176 pawanjay176 mentioned this pull request Jun 29, 2023
Closed
ghost pushed a commit to oone-world/lighthouse that referenced this pull request Jul 13, 2023
@pawanjay176
Aggregate subsets (sigp#3493)
3ad2d07 
## Issue Addressed

Resolves sigp#3238 

## Proposed Changes

Please list or describe the changes introduced by this PR.

## Additional Info

Please provide any additional information. For example, future considerations
or information useful for reviewers.
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
@pawanjay176 @Woodpile37
Aggregate subsets (sigp#3493)
32fbf9f 
Resolves sigp#3238

Please list or describe the changes introduced by this PR.

Please provide any additional information. For example, future considerations
or information useful for reviewers.
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
@pawanjay176 @Woodpile37
Aggregate subsets (sigp#3493)
ad52b6c 
Resolves sigp#3238

Please list or describe the changes introduced by this PR.

Please provide any additional information. For example, future considerations
or information useful for reviewers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AgeManning AgeManning AgeManning approved these changes

@michaelsproul michaelsproul michaelsproul approved these changes

Assignees
No one assigned
Labels
ready-for-merge This PR is ready to merge. v4.3.0 Estimated Q2 2023
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pawanjay176 @michaelsproul @AgeManning @paulhauner