-
Notifications
You must be signed in to change notification settings - Fork 965
Check slashability of attestations in batches to avoid sequential bottleneck #8516
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
Conversation
This reverts commit 1fa3b39.
…nto attestation-batch-new
|
Self-review complete. This is ready for review and hopefully merge 🤞 |
jimmygchen
left a comment
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.
Nice, I've done another round of review and this looks solid! Thanks
Merge Queue Status🚫 The pull request has left the queue (rule: This pull request spent 24 minutes 47 seconds in the queue, including 23 minutes 6 seconds running CI. Required conditions to merge
ReasonThe merge conditions cannot be satisfied due to failing checks HintYou may have to fix your CI before adding the pull request to the queue again. |
|
@mergify requeue |
✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically |
Merge Queue Status✅ The pull request has been merged at 7099fda This pull request spent 1 hour 58 minutes 46 seconds in the queue, including 1 hour 57 minutes 26 seconds running CI. Required conditions to merge
|
…tleneck (sigp#8516) Closes: - sigp#1914 Sign attestations prior to checking them against the slashing protection DB. This allows us to avoid the sequential DB checks which are observed in traces here: - sigp#8508 (comment) Co-Authored-By: Jimmy Chen <[email protected]> Co-Authored-By: Michael Sproul <[email protected]> Co-Authored-By: Michael Sproul <[email protected]>
|
Seeing improved attestation task times but worse aggregate task times - i think you noticed and mentioned this last time, just noting it down in case we want to investigate furhter next week @michaelsproul
|
|
hmm I don't remember seeing this previously, might be worth a look |

Issue Addressed
Closes:
Proposed Changes
Sign attestations prior to checking them against the slashing protection DB. This allows us to avoid the sequential DB checks which are observed in traces here:
Additional Info
This PR builds on:
This is a rework of Eitan's PR:
I started by trying to resolve merge conflicts, but there were so many breakages I ended up redoing it. I also left out some of the other changes (like the
AttestationDataService) as we are probably going to introduce a new version of that in the course of implementing the head monitor + consensus service, see: