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

Add builder boost factor part1 #7927

Merged
merged 15 commits into from
Jan 29, 2024

Conversation

tbenr
Copy link
Contributor

@tbenr tbenr commented Jan 26, 2024

  • Adds requestedBuilderBoostFactor eventually coming from BlockV3
  • The requestedBuilderBoostFactor will always override the eventual comparison factor configured on BN side.
  • Uniforms our internal builderComparisonFactor representation to the new param (from Optional<Integer> to Uint64) so the handling is simplified

In our CLI we could support PREFER_BUILDER as an alias to BUILDER_ALWAYS and start advertising the new maintaining backward compatibility (separate PR)

related to #7851

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

Copy link
Contributor

@zilm13 zilm13 left a comment

Choose a reason for hiding this comment

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

LGTM, but it would be great to see some more tests for it. They could be probably done in separate PR if not forgotten
Tests could include:

  1. builderBoost input is delivered to ExecutionBuilderModule.builderGetHeader() from any source (we have very branchy createBlock flow, all looks impossible, but at least any)
  2. more like builderGetHeaderGetPayload_shouldGivePriorityToRequestedBuilderBoostFactor, maybe unit tests for methods from ExecutionBuilderModule, even in API description it's highlighted to be careful with overflow and there could be bunch of others cases (I see by eyes that it shouldn't overflow, but who knows!)
  3. Maybe considering verbosity and size of current integration tests there is a sense to do just a unit tests for isLocalPayloadValueWinning and maybe logLocalPayloadWin

@tbenr
Copy link
Contributor Author

tbenr commented Jan 28, 2024

My plan is to move value comparison tests on a separate PR

Copy link
Contributor

@zilm13 zilm13 left a comment

Choose a reason for hiding this comment

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

LGTM

@tbenr tbenr enabled auto-merge (squash) January 29, 2024 17:17
@tbenr tbenr merged commit 281b63a into Consensys:master Jan 29, 2024
15 checks passed
@tbenr tbenr deleted the add-builder-boost-factor-part1 branch January 29, 2024 17:37
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.

2 participants