Skip to content
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: proposer boost reorg #6298

Closed
wants to merge 45 commits into from

Conversation

ensi321
Copy link
Contributor

@ensi321 ensi321 commented Jan 15, 2024

Motivation

There is an optional piece of the consensus spec that allows a CL client to re-org out a late head block by proposing new block on the parent of the canonical head. This discourages other block proposers on the network to maximize MEV by intentionally publishing a block late and thus, improving the overall health of the network.

Description

  • Implement get_proposer_head() and should_override_forkchoice_update()
  • Rename computeProposerBoostScore to computeCommitteeFraction to align with the naming in the spec
  • Add proposerBoostReorgEnabled cli flag to enable/disable the proposer boost reorg feature
  • Add timeliness field to ProtoBlock

Closes #5125

Copy link

codecov bot commented Jan 16, 2024

Codecov Report

Merging #6298 (39b1113) into unstable (d10ed38) will decrease coverage by 0.16%.
Report is 36 commits behind head on unstable.
The diff coverage is 72.90%.

❗ Current head 39b1113 differs from pull request most recent head 07a11d5. Consider uploading reports for the commit 07a11d5 to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #6298      +/-   ##
============================================
- Coverage     61.69%   61.53%   -0.16%     
============================================
  Files           555      556       +1     
  Lines         58255    59090     +835     
  Branches       1844     1874      +30     
============================================
+ Hits          35939    36363     +424     
- Misses        22277    22686     +409     
- Partials         39       41       +2     

@ensi321 ensi321 marked this pull request as ready for review January 16, 2024 14:00
@ensi321 ensi321 requested a review from a team as a code owner January 16, 2024 14:00
@@ -352,7 +352,7 @@ export function getValidatorApi({
// forkChoice.updateTime() might have already been called by the onSlot clock
// handler, in which case this should just return.
chain.forkChoice.updateTime(slot);
chain.recomputeForkChoiceHead();
chain.recomputeForkChoiceHead(UpdateHeadOpt.GetProposerHead, slot);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to get the result (proposer head) from this function, put it in chain.produceBlindedBlock() in order to build block with expected parent_root
same to the other 2 chain.recomputeForkChoiceHead () calls below

@@ -646,12 +647,12 @@ export class BeaconChain implements IBeaconChain {
};
}

recomputeForkChoiceHead(): ProtoBlock {
recomputeForkChoiceHead(mode: UpdateHeadOpt = UpdateHeadOpt.GetCanonicialHead, slot?: Slot): ProtoBlock {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it takes me a while to figure out the use of different params here. I prefer to keep this function as is and add:

  • predictProposerHead: to be used in prepareNextSlot.ts
  • getProposerHead when we produce a block

that'd be cleaner to me, each function has its own purpose. Would like to know opinions from @wemeetagain @g11tech too

@@ -476,7 +484,7 @@ export class BeaconChain implements IBeaconChain {

async produceCommonBlockBody(blockAttributes: BlockAttributes): Promise<CommonBlockBody> {
const {slot} = blockAttributes;
const head = this.forkChoice.getHead();
const head = blockAttributes.proposerHead ?? this.forkChoice.getHead();
Copy link
Contributor

@twoeths twoeths Mar 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to make change in chain.produceBlockWrapper() to use use proposerHead inside BlockAttributes to build block

@twoeths
Copy link
Contributor

twoeths commented Mar 22, 2024

this is too big to review and get merged imo. @ensi321 I suggest breaking this into at least 2 PRs:

  • Forkchoice change, constants and related tests. This is quite straightforward and does not affect the current implementation so it should be fast to go through review process
  • Produce block flow + cli: need to review carefully as this is an important flow

once we have e2e test, I'm comfortable merging this work with disable flag, test it for a while on different networks and fix bugs if any, then finally turn in on by default in the future

@ensi321
Copy link
Contributor Author

ensi321 commented Mar 22, 2024

Splitting this PR into smaller chunks for better reviewability.

@ensi321 ensi321 closed this Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposer boost reorg
2 participants