-
-
Notifications
You must be signed in to change notification settings - Fork 290
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: getAttestationsForBlock performance issue #6367
Conversation
Performance Report✔️ no performance regression detected 🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
095e68e
to
1d6887c
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6367 +/- ##
=========================================
Coverage 60.15% 60.15%
=========================================
Files 407 407
Lines 46511 46511
Branches 1550 1550
=========================================
Hits 27980 27980
Misses 18499 18499
Partials 32 32 |
bringing back Even when sim tests is green, I think it's still good to have this e2e test:
|
holesky_produced_block_884610.json.zip produced this block in holesky using it looks good with 128 attestations, most of them are for previous block slot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure how significant the boost will be if from the starting itself we only insert and maintain in a sorted array by score each time we evaluate and add attestation by score (and we can easily get the score cutoff by inspecting MAX_ATTESTIONS's index in the array for each iteration, so probably we can cut off more attestations with lower score in average case
Overall compute time will not change because there is sorting still involved later which would not need to be done as the array is already sorted by score.
However the current approach already optimizes it quite a bit (1sec => 100ms), so may be it would be worth to look at the above approach only if there we are looking to extract further juice.
For now looks all good 👍
🎉 This PR is included in v1.16.0 🎉 |
Motivation
Get aggregated attestations for block production may take up to >1s, this PR improve it
Description
slot & index
so that we can get committee once and we run the participation function way less. This is possible thanks to the attestation validation against the stateseenAttestingIndices
tonotSeenAttestingIndices
which makes the logic simplerbeaconBlockRoot + targetEpoch
Closes #6334