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

[Merge] Move execution payload validation out of state transition #4648

Merged
merged 19 commits into from
Nov 17, 2021

Conversation

ajsutton
Copy link
Contributor

@ajsutton ajsutton commented Nov 16, 2021

PR Description

Moves execution payload validation out of state transition to be done asynchronously, enabling optimistic sync.

There are still quite a few loose ends here but want to see what mess this makes of tests.

Fixed Issue(s)

#4599
fixes #4348

Documentation

  • I thought about documentation and added the documentation label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

@ajsutton ajsutton marked this pull request as ready for review November 17, 2021 00:02
@tbenr tbenr changed the title Move execution payload validation out of state transition [Merge] Move execution payload validation out of state transition Nov 17, 2021
private final EventThread forkChoiceExecutor;
private final SignedBeaconBlock block;
private final ExecutionEngineChannel executionEngine;
private SafeFuture<ExecutePayloadResult> result;
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason why you don't use Optional here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly because Optional<SafeFuture<ExecutePayloadResult>> is kind of awful and it's entirely internal. The class did get a fair bit bigger since I made that decision though so it probably is worth changing to Optional.

Copy link
Contributor

@tbenr tbenr left a comment

Choose a reason for hiding this comment

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

this is a tricky one, but LGTM.

@ajsutton ajsutton enabled auto-merge (squash) November 17, 2021 20:54
@ajsutton ajsutton merged commit 57bcd49 into Consensys:master Nov 17, 2021
@ajsutton ajsutton deleted the validate-payload branch November 17, 2021 21:02
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.

[Merge] Support Async payload execution
2 participants