Introduce option to use local payload instead of builder #6878
Conversation
...r/src/test/java/tech/pegasys/teku/ethereum/executionlayer/ExecutionLayerManagerImplTest.java
Show resolved
Hide resolved
|
I decided to add feature to use builder's bid whenever is possible not comparing with local value to the same flag with keyword "NEVER". If it totally doesn't work for us, I made it with a single commit, so it could be easily reverted |
| paramLabel = "<STRING>", | ||
| showDefaultValue = Visibility.ALWAYS, | ||
| description = | ||
| "Fallback to local payload, when builder bid is available but its value could be beat by local payload. " |
There was a problem hiding this comment.
we can simply say "Fallback to local payload whenever its value beats the builder bid."
| "Fallback to local payload, when builder bid is available but its value could be beat by local payload. " | ||
| + "Value is whole, percent (e.g. 100, default value, means use local payload when it at least matches builder bid, " | ||
| + "80 means use local payload when its value is at least 80% of builder bid).\n" | ||
| + "NEVER: ignore local value, use builder's bid whenever it passed validation", |
| return getResultFromLocalExecutionPayload( | ||
| localExecutionPayload, slot, FallbackReason.LOCAL_BLOCK_VALUE_HIGHER); | ||
| localExecutionPayload, slot, FallbackReason.LOCAL_BLOCK_VALUE_WIN); |
There was a problem hiding this comment.
what about LOCAL_BLOCK_VALUE_WON ? similarly to CIRCUIT_BREAKER_ENGAGED
There was a problem hiding this comment.
it was the first name. don't know why I reverted it
| @@ -164,9 +168,21 @@ public SafeFuture<HeaderWithFallbackData> builderGetHeader( | |||
| .map(ExecutionPayloadWithValue::getValue) | |||
| .orElse(UInt256.ZERO); | |||
| logReceivedBuilderBid(signedBuilderBid.getMessage()); | |||
| if (signedBuilderBid.getMessage().getValue().lessOrEqualThan(localPayloadValue)) { | |||
| // 1 ETH is 10^18 wei, Uint256 max is more than 10^77 | |||
| if (builderBidChallengePercentage.isPresent() | |||
There was a problem hiding this comment.
what about moving this in a isLocalPayloadValueWinning function?
|
@tbenr updated up to your feedback. Let's keep it till native speaker for final proof-reading |
|
I've remembered what was wrong when we posted about this feature last time. It will be enabled only since Capella, so we'd better keep it hidden to avoid misleading. Docs etc should be added with Capella release I think. What do you think? (but we may have a lot of debts for this release) |
OH yes. very good catch. I was about to fall under the same issue. Even if when we start using the new engine api system that Loucas is working on, we will detach from the Capella fork. |
| @@ -50,6 +50,7 @@ | |||
| public class ExecutionBuilderModule { | |||
|
|
|||
| private static final Logger LOG = LogManager.getLogger(); | |||
| private static final int HUNDRED_PERCENTS = 100; | |||
There was a problem hiding this comment.
we shouldnt need to define 100%, but if we do, maybe just HUNDRED_PERCENT, as the s is confusing :)
There was a problem hiding this comment.
Oh, could you explain this, please, I know you are right, but I don't understand this. Is percent always plural?
| "Using local Execution Payload instead of Builder Bid as it's challenged. " | ||
| + "Builder bid value: {}, local block value: {}, builderBidChallengePercentage: {}%", |
There was a problem hiding this comment.
The local execution payload value ({}) was awarded the block over the builder payload ({}), {}% challenge configured.
PR Description
Introduces option to use local payload instead of builder if local matches X% of builder's value. X is configured:
Fixed Issue(s)
Fixes #6327
Documentation
doc-change-requiredlabel to this PR if updates are required.Changelog