-
-
Notifications
You must be signed in to change notification settings - Fork 311
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
feat: add proposer boost reorg flag #6652
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #6652 +/- ##
============================================
- Coverage 62.19% 62.19% -0.01%
============================================
Files 571 571
Lines 60021 60099 +78
Branches 1973 1977 +4
============================================
+ Hits 37333 37377 +44
- Misses 22645 22679 +34
Partials 43 43 |
Performance Report✔️ no performance regression detected Full benchmark results
|
packages/fork-choice/test/unit/forkChoice/getProposerHead.test.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: twoeths <tuyen@chainsafe.io>
Co-authored-by: Nico Flaig <nflaig@protonmail.com>
@ensi321 there is a conflicting file |
@@ -131,6 +133,14 @@ Will double processing times. Use only for debugging purposes.", | |||
group: "chain", | |||
}, | |||
|
|||
"chain.proposerBoostReorgEnabled": { |
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.
can you remove Enabled
here?
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.
Will address in the next PR as per #6652 (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.
Why not right now? I'm assuming we're wanting to test this on our test fleet?
if you change this later thats just another opportunity for things (like our testing infra) to get out of sync and break.
Just ctrl-f change
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.
Why not right now? I'm assuming we're wanting to test this on our test fleet? if you change this later thats just another opportunity for things (like our testing infra) to get out of sync and break. Just ctrl-f change
@wemeetagain You got a point. Updated.
@@ -123,11 +125,19 @@ Will double processing times. Use only for debugging purposes.", | |||
group: "chain", | |||
}, | |||
|
|||
"chain.proposerBoostEnabled": { | |||
"chain.proposerBoost": { |
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.
as this is an existing flag, we must add the prev. value as an alias (even though I doubt anyone is using it)
At some point, when we do a v2 release, we can clean those up
Co-authored-by: Nico Flaig <nflaig@protonmail.com>
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.
LGTM lets wait for @twoeths to chime in too
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.
looks good to me, I added a new enum to RegenCaller
to review metrics easier
🎉 This PR is included in v1.20.0 🎉 |
Motivation
Wire the existed proposer boost reorg functionality to block production.
Description
getProposerHead()
in block productionpredictProposerHead()
inprepareForNextSlot()
proposerBoostReorg
flag. Default to be false for now as this is seen as an experimental featureproposerBoostEnabled
flag toproposerBoost
Closes #5125