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

[MEV Boost\Builder] execution builder client part 3 #5429

Conversation

tbenr
Copy link
Contributor

@tbenr tbenr commented May 4, 2022

PR Description

  • Completes the separation of builder client and engine client.
  • Moves throttling from channel to clients
  • Introduces ExecutionPayloadContext which carries the context for a new execution payload has been produced (payloadId, forkChoiceState, and payloadAttributes). Used to retrieve parentHash and (in later PRs) publicKey of the validator, required in builder apis.

related to #5396

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.

ExecutionPayloadHeaderSchema executionPayloadHeaderSchema =
SchemaDefinitionsBellatrix.required(spec.atSlot(slot).getSchemaDefinitions())
.getExecutionPayloadHeaderSchema();
// validate signature
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does the public key to validate the signature come from? If it's a validator key then it would have to come from state and we won't be able to verify the signature over here - we'd have to pass it all back to the beacon node unchecked which is messy...

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like we should make sure we have a ticket to come back and actually validate the signature here.

ExecutionPayloadContext
remove builder api from ExecutionEngineClient
Let ExecutionLayerChannelImpl use builder client
complete execution engine channel renaming
move throttling from channel to clients
execution builder CLI
ExecutionEngine* to ExecutionLayer*
@tbenr tbenr force-pushed the integrate_new_builder_client_in_executionLayerChannel branch from dff6cd2 to dd8c576 Compare May 9, 2022 11:57
@tbenr tbenr marked this pull request as ready for review May 9, 2022 13:28
Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

LGTM.

ExecutionPayloadHeaderSchema executionPayloadHeaderSchema =
SchemaDefinitionsBellatrix.required(spec.atSlot(slot).getSchemaDefinitions())
.getExecutionPayloadHeaderSchema();
// validate signature
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like we should make sure we have a ticket to come back and actually validate the signature here.

@tbenr tbenr enabled auto-merge (squash) May 10, 2022 06:33
@tbenr tbenr merged commit 419aeab into Consensys:master May 10, 2022
@tbenr tbenr deleted the integrate_new_builder_client_in_executionLayerChannel branch May 10, 2022 07:16
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