-
-
Notifications
You must be signed in to change notification settings - Fork 410
feat: score attestations for block by profitability #8016
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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## unstable #8016 +/- ##
=========================================
Coverage 55.68% 55.68%
=========================================
Files 835 835
Lines 59476 59491 +15
Branches 4585 4586 +1
=========================================
+ Hits 33119 33130 +11
- Misses 26290 26294 +4
Partials 67 67 🚀 New features to boost your workflow:
|
Performance Report✔️ no performance regression detected Full benchmark results
|
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.
Clean 👍
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.
while I see how straightforward it is to bring processAttestation reward computation to this flow, we need to also consider how this affects blockprint because we change our algorithm and how unique we are compared to other client (which the old algorithm is)
with our total consolidations are < 8 most of the time, this will not bring better block profit compared to our current sort algorithm
I'd keep the old sorting algorithm and put this new sorting algorithm as a comment or refer to it somewhere so that it give us more insights when we have to revisit this later, unless metrics show we have better profit with this
|
As noted by Michael, keeping the current sorting / scoring is good for blockprint as it’s slightly different from other clients and should help differentiate Lodestar. I also tend towards just closing this for now and revisit in the future / in different network conditions as it could make a difference during non-finality when there are a lot of different votes (less consolidation) + missed slots which require us to fill up the block most of the time. |
|
created an issue #8019 to keep track of this |
|
based on data from running this on it makes sense to re-test this branch in a non-finality network or once we have implemented #8028 |
no reason, I think we can merge this. I'm not sure when can I continue with SingleAttestation inclusion for block production anyway. |
|
/gemini review |
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.
Code Review
This pull request updates the scoring mechanism for attestations to maximize block profitability. The new logic correctly uses participation flags (timely source, target, and head) and their associated weights to score attestations, which is a more accurate model of the actual proposer rewards compared to the previous heuristic. The implementation looks solid. I've added one suggestion to improve the readability of the weight calculation logic.
|
🎉 This PR is included in v1.34.0 🎉 |
Updates scoring / sorting of consolidated attestations during block packing to follow
processAttestationreward computation to maximize profitability of block.lodestar/packages/state-transition/src/block/processAttestationsAltair.ts
Line 33 in 2fdd3fa
Closes #8019