Mev/Builder api integration #3969
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## unstable #3969 +/- ##
================================
================================
|
dapplion
reviewed
May 10, 2022
dapplion
reviewed
May 10, 2022
| }), | ||
|
|
||
| // At the same time fetch any remaining unknown validator indices, then poll duties for those newIndices only | ||
| this.indicesService | ||
| .pollValidatorIndices() | ||
| .then((newIndices) => this.api.validator.prepareBeaconProposer(this.getProposerData(newIndices))) | ||
| .then((newIndices) => this.getProposerData(slot, newIndices)) | ||
| .then((proposerData) => this.api.validator.prepareBeaconProposer(proposerData)) |
There was a problem hiding this comment.
At this point just use an async function instead of a then chain, for consistency with the rest of the codebase
There was a problem hiding this comment.
modeled changes on the rebased function
dapplion
reviewed
May 10, 2022
dapplion
reviewed
May 10, 2022
dapplion
reviewed
May 10, 2022
dapplion
reviewed
May 10, 2022
dapplion
reviewed
May 10, 2022
dapplion
reviewed
May 10, 2022
|
thanks @dapplion for looking through ❤️, yes need to refactor as the bn <> boost interaction is now CL api style. will incorporate the feedback once am able to successfully implement the flow 🙂 |
g11tech
force-pushed
the
g11tech/engine-prepare-fcus
branch
2 times, most recently
from
5752b9f
to
ef57bec
Compare
|
Builder spec was moved to https://github.com/ethereum/builder-specs |
g11tech
force-pushed
the
g11tech/mev-integration
branch
2 times, most recently
from
67b6c5d
to
cbefe3d
Compare
Performance Report✔️ no performance regression detected Full benchmark results
|
g11tech
force-pushed
the
g11tech/mev-integration
branch
3 times, most recently
from
2e78e09
to
3608f31
Compare
|
Update successful lodestar <> kiln builder |
|
update: successful lodestar <> ropsten builder |
dapplion
reviewed
Jun 14, 2022
g11tech
force-pushed
the
g11tech/mev-integration
branch
2 times, most recently
from
2b2ebfd
to
9e57a7f
Compare
dapplion
reviewed
Jun 16, 2022
| import {toHexString, byteArrayEquals} from "@chainsafe/ssz"; | ||
| import {CachedBeaconStateBellatrix} from "../types.js"; | ||
| import {getRandaoMix} from "../util/index.js"; | ||
| import {ExecutionEngine} from "../util/executionEngine.js"; | ||
| import {isMergeTransitionComplete} from "../util/bellatrix.js"; | ||
|
|
||
| export function processExecutionPayload( | ||
| export function processExecutionPayload<T extends allForks.BlockType>( | ||
| type: T, |
There was a problem hiding this comment.
If would be nice if we can avoid having to pass this type everywhere
There was a problem hiding this comment.
changed, now conditioned on the body's execution payload/header inspection of the block 👍
dapplion
reviewed
Jun 16, 2022
packages/beacon-state-transition/src/block/processExecutionPayload.ts
Outdated
Show resolved
Hide resolved
twoeths
reviewed
Jun 17, 2022
twoeths
reviewed
Jun 17, 2022
|
|
||
| private registerValidator = async (epoch: Epoch): Promise<void> => { | ||
| if (epoch < this.config.BELLATRIX_FORK_EPOCH - 1) return; | ||
| this.getValidatorRegistrations(epoch, this.indicesService.getAllLocalIndices()) |
There was a problem hiding this comment.
why do we have to register all validators for every epoch?
There was a problem hiding this comment.
because a validator registration might only be valid for 2 epochs on the builder, its just simpler to blindly register validator with them every epoch. 🤔
twoeths
reviewed
Jun 17, 2022
twoeths
reviewed
Jun 17, 2022
g11tech
force-pushed
the
g11tech/mev-integration
branch
3 times, most recently
from
6936a04
to
7bf6a82
Compare
wemeetagain
reviewed
Jun 27, 2022
Comment on lines
35
to
36
| const fullOrBlindedBody = ((block as bellatrix.BlindedBeaconBlock).body ?? | ||
| (block as bellatrix.BeaconBlock).body) as allForks.FullOrBlindedBellatrixBeaconBlockBody; |
There was a problem hiding this comment.
Is this intended? It looks like without type assertions, this reads like block.body ?? block.body
There was a problem hiding this comment.
good eye @wemeetagain! thanks 🙂
packages/beacon-state-transition/src/block/processBlockHeader.ts
Outdated
Show resolved
Hide resolved
| ): ISignatureSet { | ||
| const {config, epochCtx} = state; | ||
| const domain = state.config.getDomain(DOMAIN_BEACON_PROPOSER, signedBlock.message.slot); | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/strict-boolean-expressions | ||
| const blockType = (signedBlock.message as bellatrix.BlindedBeaconBlock).body.executionPayloadHeader | ||
| ? ssz.bellatrix.BlindedBeaconBlock |
There was a problem hiding this comment.
We should add the blinded variants to getForkTypes somehow.
There was a problem hiding this comment.
can I pick this in the followup ? Will add this to a followup issue tracker among few others i think.
6 tasks
g11tech
force-pushed
the
g11tech/mev-integration
branch
from
cddba5f
to
e23a3dd
Compare
* extract builder in its own folder space * fix engine path * rebase fixes * refactor the builder with latest beacon state transtion processing changes * fix the registration signature * make typescript happy * remove the early return * rebase fixes * gas limit option * builder enabling on validator, gaslimit and other refac * fixes * Rebase fixes with the api refac * some cleanup fixes * comment explaining the immediate registration call * use already defined batchItems * serialize the validator registration chunks * validate transactions root on payload response * removing the BlockType from global types and sandboxing it just inside block factory * make builder optional in dev validator setup * option builder with dev val init with keystores * add builder api test routes * fix validator tests * fix block assembly scripts * handle the mergemock customization * fix body extraction * remove linter superession for boolean expressions
g11tech
force-pushed
the
g11tech/mev-integration
branch
from
e23a3dd
to
d5a128f
Compare
dapplion
reviewed
Jun 30, 2022
| @@ -46,10 +46,18 @@ export function processExecutionPayload( | |||
| // if executionEngine is null, executionEngine.onPayload MUST be called after running processBlock to get the | |||
| // correct randao mix. Since executionEngine will be an async call in most cases it is called afterwards to keep | |||
| // the state transition sync | |||
| if (executionEngine && !executionEngine.notifyNewPayload(payload)) { | |||
| if ( | |||
| (payload as bellatrix.ExecutionPayload).transactions !== undefined && | |||
There was a problem hiding this comment.
Unless it looks really annoying this should be a type guard. Like
function isExecutionPayload(
payload: bellatrix.ExecutionPayload | bellatrix.ExecutionPayloadBlinded
): payload is bellatrix.ExecutionPayload {
return (payload as bellatrix.ExecutionPayload).transactions !== undefined
}
if (isExecutionPayload(payload) && !executionEngine.notifyNewPayload(payload as bellatrix.ExecutionPayload)) {
throw Error("Invalid execution payload");
}
dapplion
reviewed
Jun 30, 2022
| throw Error("Invalid execution payload"); | ||
| } | ||
|
|
||
| const transactionsRoot = | ||
| (payload as bellatrix.ExecutionPayloadHeader).transactionsRoot ?? | ||
| ssz.bellatrix.Transactions.hashTreeRoot((payload as bellatrix.ExecutionPayload).transactions); |
There was a problem hiding this comment.
Same here, use type guards
dapplion
reviewed
Jun 30, 2022
| ): ISignatureSet { | ||
| const {config, epochCtx} = state; | ||
| const domain = state.config.getDomain(DOMAIN_BEACON_PROPOSER, signedBlock.message.slot); | ||
|
|
||
| const blockType = | ||
| (signedBlock.message as bellatrix.BlindedBeaconBlock).body.executionPayloadHeader !== undefined |
dapplion
reviewed
Jun 30, 2022
| options?: {verifyStateRoot?: boolean; verifyProposer?: boolean; verifySignatures?: boolean}, | ||
| metrics?: IBeaconStateTransitionMetrics | null | ||
| ): CachedBeaconStateAllForks { | ||
| const {verifyStateRoot = true, verifyProposer = true, verifySignatures = true} = options || {}; | ||
|
|
||
| const block = signedBlock.message; | ||
| const block = signedBlock.message as allForks.FullOrBlindedBeaconBlock; |
There was a problem hiding this comment.
Is this casting necessary? If so why?
dapplion
reviewed
Jun 30, 2022
| return ( | ||
| isMergeTransitionComplete(state) || | ||
| !ssz.bellatrix.ExecutionPayload.equals(body.executionPayload, ssz.bellatrix.ExecutionPayload.defaultValue()) | ||
| ((body as bellatrix.BlindedBeaconBlockBody).executionPayloadHeader !== undefined |
|
I think we can have two directories named |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Reference: https://github.com/ethereum/execution-apis/pull/209/files
Dev TODOs
- [ ] Choice logic to be includedmoving to a followup issue Followup tracker for MeV related pending issues/cleanup #4208Dev Cleanup
builder.enabledoptionTests with mergemock
Tests with the full mev builder
Mergemock/local testnet relay integration with merge-interop
- [ ] path needs to be figured out yet#4208Old work
This PR integrates the builder api and consists broadly of 3 parts:
Json rpc setupIt was decided to implement the api CL styleValidatorRegistrationV1in beacon proposer api which BN can propagatebuilder_registerValidatorV1builder_getHeaderV1in parallel with fetching payload from engine while assembling body to get theExecutionPayloadHeaderV1and signedBuilderBidV1BlindedSignedBeaconBlockand sendbuilder_getPayloadV1and getExecutionPayloadV1which can be used be combined with the signedBlindedBeaconBlockto form SignedBeaconBlock and propogatedCloses #3953
UPDATE: integration/test SUCCESS with
mergemock!!! 🥳 locally with some caveatsCaveats