feat: asyncAggregateWithRandomness #7204
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## unstable #7204 +/- ##
===========================================
Coverage ? 49.22%
===========================================
Files ? 598
Lines ? 39789
Branches ? 2103
===========================================
Hits ? 19588
Misses ? 20161
Partials ? 40 |
Performance Report✔️ no performance regression detected Full benchmark results
|
twoeths
reviewed
Oct 29, 2024
twoeths
reviewed
Oct 29, 2024
|
there are big improvement of this PR, missed attestations + target were reduced from ~10% to 3-4% on a 
|
wemeetagain
previously approved these changes
Oct 29, 2024
matthewkeil
added
the
status-do-not-merge
Great news! Can we let the metrics soak a bit more before we merge? Just incase anything wonky shows up |
|
Oky, will wait until later today to get more data. I'm hoping to cut a new RC by my end of day today and want to include this. |
matthewkeil
removed
the
status-do-not-merge
|
Everything across the board looks healthier from attestations to scrape duration. The one thing that did stand out though is mesh peers are not as good. Not sure if that is just a function of the node still being relatively fresh and that will stabilize over the next several hours or not... CPU utilization is up a bit but i suppose its just working more efficiently with the multithreading. Wondering if we can tune that even further by changing the number of libuv threads. Can play with that in another PR though. |
I don't think that is an apples-to-apples comparison. your test is using subscribe-all-subnets and other comparable nodes aren't. |
wemeetagain
approved these changes
Oct 29, 2024
matthewkeil
added a commit
that referenced
this pull request
Nov 4, 2024
This reverts commit e31d535.
|
attestation job time decreased significantly since this PR, this is the unstable mainnet node 
|
|
same to "main to worker" latency 
|
|
"worker to main" latency on unstable mainnet node was significantly improved as well 
|
matthewkeil
added a commit
that referenced
this pull request
Nov 6, 2024
This reverts commit e31d535.
matthewkeil
added a commit
that referenced
this pull request
Nov 6, 2024
This reverts commit e31d535.
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.
Motivation
Resolves #7197
Moves signature checks and aggregation for unaggregated attestations off main thread. Will help clear up thread congestion when subscribing to all subnets.