Fix aggregated attestation pool grows large in size #4932
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
terencechain
added
Bug
Codecov Report
@@ Coverage Diff @@
## master #4932 +/- ##
==========================================
+ Coverage 45.45% 45.48% +0.02%
==========================================
Files 209 209
Lines 15960 15963 +3
==========================================
+ Hits 7254 7260 +6
+ Misses 7469 7466 -3
Partials 1237 1237 |
|
Don't merge yet, I'm testing proposer is passing |
terencechain
added
In Progress
and removed
Ready For Review
terencechain
added
Ready For Review
| @@ -24,8 +24,7 @@ func (s *Service) aggregateRoutine() { | |||
| case <-s.ctx.Done(): | |||
| return | |||
| case <-ticker.C: | |||
| attsToBeAggregated := append(s.pool.UnaggregatedAttestations(), s.pool.AggregatedAttestations()...) | |||
| if err := s.aggregateAttestations(ctx, attsToBeAggregated); err != nil { | |||
| if err := s.aggregateAttestations(ctx, s.pool.UnaggregatedAttestations()); err != nil { | |||
There was a problem hiding this comment.
Why wouldn't we use the aggregated ones too?
There was a problem hiding this comment.
Agreed this a problem. Reverted
rauljordan
approved these changes
Feb 24, 2020
cryptomental
pushed a commit
to cryptomental/prysm
that referenced
this pull request
Feb 24, 2020
* Add metrics * Use it * Use it * Fixed exp time and tests * Update on save too * Expose getters * One epoch purge time * Fixed a timing issue * Clean up * Gazelle * Interface * Prune every epoch * Aggregate twice per slot * Revert attsToBeAggregated * Delete expired atts Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>
cryptomental
pushed a commit
to cryptomental/prysm
that referenced
this pull request
Feb 28, 2020
* Add metrics * Use it * Use it * Fixed exp time and tests * Update on save too * Expose getters * One epoch purge time * Fixed a timing issue * Clean up * Gazelle * Interface * Prune every epoch * Aggregate twice per slot * Revert attsToBeAggregated * Delete expired atts Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes #4929
Please review and merge #4930 first
Attestation pool were growing large in size because we reset an item's expiration time upon new update. Say a new attestation with
dataAand bits 0x01 comes in, the expires time of the attestation is set totime.Now + N.One slot later another attestation with
dataAbut different bits came in. Here we should not touch the expire time, but instead the node reset the expire time totime.Now + Nagain. That's why some attestations would never expire because we keep on re aggregating itself here:https://github.com/prysmaticlabs/prysm/compare/fix-exp-time?expand=1#diff-45a8f7c8f90ae232acabd3f59fdea76cL27
Change list:
Live testing update:
[2020-02-23 14:05:36 PST]: Ran it for 10+ epochs, the attestations pool count is steadily below 500 versus 2000.. before