-
-
Notifications
You must be signed in to change notification settings - Fork 405
feat: include single attestations in producing blocks #8076
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
base: unstable
Are you sure you want to change the base?
feat: include single attestations in producing blocks #8076
Conversation
Performance Report✔️ no performance regression detected Full benchmark results
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## unstable #8076 +/- ##
============================================
+ Coverage 55.78% 55.93% +0.14%
============================================
Files 834 836 +2
Lines 59535 60444 +909
Branches 4602 4721 +119
============================================
+ Hits 33214 33808 +594
- Misses 26253 26568 +315
Partials 68 68 🚀 New features to boost your workflow:
|
here are metrics on mainnet node (before and after I subscribe to all subnets)
![]()
![]()
![]()
![]()
![]()
|
will separate this PR into 4 smaller PRs to make it easier to review:
|
It looks like that 150_000 cap is only enforced during prune and not add. So the 50MB is not a hard bound per se as it can go over 50MB during the slot. |
|
||
/** | ||
* Remove any attestations with a slot lower than `current_slot - MAX_SLOTS_RETAINED`. | ||
* Remove more slots until we have less than `MAX_ATTESTATIONS_RETAINED` attestations in the pool or at least `MIN_SLOTS_RETAINED` slots. |
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.
Do we still have MIN_SLOTS_RETAINED
mechanism? It looks like prune()
will prune all attestations except those for current epoch and last epoch, and then prune down to MAX_ATTESTATIONS_RETAINED
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.
I did have MIN_SLOTS_RETAINED
then removed since I switched to only store not-seen attestations
I don't see the use of it anymore. In unfinality time it's more important to keep the node stable than trying to find not-seen attestations to include in block
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.
In unfinality time it's more important to keep the node stable than trying to find not-seen attestations to include in block
For single attestations this strategy seems fine as it's not likely they will represent a lot of weight
Motivation
Description
3263 slots depending on total attestation count capped by100_000150_000 as analyzed in the PR3250 MB of SingleAttestations so that it does not affect performance of nodegetAttestationsForBlock
, for each slot:AggregatedAttestationPool
SingleAttestationPool
Closes Include SingleAttestation when producing block #8028
TODOs
cc @wemeetagain @nflaig