-
Notifications
You must be signed in to change notification settings - Fork 303
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
[MEV Boost] Allow Blinded block creation flow in BN - Part1 #5249
Conversation
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, just small details and worth adding some tests.
.getPayload(payloadId, blockSlotState.getSlot()) | ||
.join())); | ||
|
||
if (isMevBoostEnabled || blinded) { |
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.
It feels weird to me that you could ask for a non-blinded block but be given one anyway because mev boost is enabled. Nots are it's wrong as such, just feels very unexpected. Maybe it's just that blinded
should be forceBlinded
?
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.
Thinking again this is wrong.
At high-level these are the conditions I want to handle.
if (isMevBoostEnabled && blinded) {
// blinded mevboost flow
// set `bodyBuilder.executionPayloadHeader` via `executionEngineChannel.getPayloadHeader`
} else if (!isMevBoostEnabled && blinded) {
// blinded non-mevboost flow (To Be implemented - issue #5103)
// set `bodyBuilder.executionPayloadHeader` via `executionEngineChannel.getPayload` (extract header from full payload)
// cache the full payload to be used to "unblind" the signed block before publication
} else {
// non-blind standard flow
// set `bodyBuilder.executionPayload` via `executionEngineChannel.getPayload`
}
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.
ah yeah that makes sense to me.
...tor/src/main/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactory.java
Outdated
Show resolved
Hide resolved
@@ -115,7 +119,8 @@ private ExecutionPayload getExecutionPayload(final BeaconBlock block) { | |||
return BeaconBlockBodyBellatrix.required(block.getBody()).getExecutionPayload(); | |||
} | |||
|
|||
private BeaconBlock assertBlockCreated(final int blockSlot, final Spec spec) | |||
private BeaconBlock assertBlockCreated( |
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.
We probably should add a test for the cases where isMevBoostEnabled
and blinded
are true (and the various combinations). Looks like we're always passing false, false
at the moment.
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.
quite not there yet. I'll do a Part-2 of the PR.
...src/test/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactoryTest.java
Outdated
Show resolved
Hide resolved
...src/main/java/tech/pegasys/teku/validator/remote/apiclient/OkHttpValidatorRestApiClient.java
Show resolved
Hide resolved
…r can behave based on builder itself
093a039
to
6b53bc7
Compare
PR Description
BlockFactory
become mev_boost \ blinded block awareBlockOperationSelectorFactory
become mev_boost aware.Instead of haveing a dedicated
BlindedBeaconBlockBodyBuilder
, the flow has been integrated inBeaconBlockBodyBuilderBellatrix
.BeaconBlockBodyBuilder
interface exposesisBlinded
and will be used to expose toBlockOperationSelectorFactory
to generate a builderConsumer that can reason about blinded\non-blinded blocks.Fixed Issue(s)
#5261
Documentation
doc-change-required
label to this PR if updates are required.Changelog