-
Notifications
You must be signed in to change notification settings - Fork 314
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
Add builder boost factor part1 #7927
Conversation
...dator/remote/src/main/java/tech/pegasys/teku/validator/remote/RemoteValidatorApiHandler.java
Dismissed
Show dismissed
Hide dismissed
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, 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:
- builderBoost input is delivered to
ExecutionBuilderModule.builderGetHeader()
from any source (we have very branchy createBlock flow, all looks impossible, but at least any) - 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!) - Maybe considering verbosity and size of current integration tests there is a sense to do just a unit tests for
isLocalPayloadValueWinning
and maybelogLocalPayloadWin
...ionlayer/src/main/java/tech/pegasys/teku/ethereum/executionlayer/ExecutionBuilderModule.java
Outdated
Show resolved
Hide resolved
My plan is to move value comparison tests on a separate PR |
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
requestedBuilderBoostFactor
eventually coming from BlockV3requestedBuilderBoostFactor
will always override the eventual comparison factor configured on BN side.builderComparisonFactor
representation to the new param (fromOptional<Integer>
toUint64
) so the handling is simplifiedIn our CLI we could support
PREFER_BUILDER
as an alias toBUILDER_ALWAYS
and start advertising the new maintaining backward compatibility (separate PR)related to #7851
Documentation
doc-change-required
label to this PR if updates are required.Changelog