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] Support Async payload execution #4348

Closed
ajsutton opened this issue Sep 13, 2021 · 2 comments · Fixed by #4648
Closed

[Merge] Support Async payload execution #4348

ajsutton opened this issue Sep 13, 2021 · 2 comments · Fixed by #4648
Assignees

Comments

@ajsutton
Copy link
Contributor

Description

Currently, synchronous calls are made to the execution engine during block processing which results in blocking the fork choice thread while the execution engine processes the payload. The fork choice thread however is not designed to be blocked.

We either need to move block processing off the fork choice thread entirely (without introducing race conditions with fork choice vote updating or deadlocks while accessing the protoarray) or be able to process the execution payload before moving onto the fork choice thread (similar to how the beacon state is retrieved prior to moving onto the fork choice thread).

@ajsutton ajsutton mentioned this issue Sep 13, 2021
20 tasks
@ajsutton
Copy link
Contributor Author

I suspect the key to this is to split block verification from processing. We ultimately need to move the executionPayloadUtil.verifyExecutionStateTransition call which is currently in BlockProcessorMerge.processExecutionPayload to happen off the fork choice thread - thus it would be a pre-verification step and the block can be rejected if the execution state is invalid. Since verifying the state transition is expensive we likely also need to do some of the other verifications before this step though - at minimum checking the block signature. Likely we could and should move quite a lot of the verifications forward and thus be able to perform them off the fork choice thread as well which is useful.

The potential risk here is that doing this verification off the fork choice thread makes it more difficult to guarantee the order of messages sent to the execution engine. We'd guarantee that newBlock is called before setHead for that block but there might be multiple blocks from different forks being processed in parallel resulting in newBlock(blockA) newBlock(blockB) setHead(blockA) type sequences (in old RPC method name terms). In the absence of forks, we'd still have to import blocks one at a time anyway but the pending block pool automatically does that sequencing for us. @mkalinin might be worth considering this as part of discussions on the API.

@mkalinin
Copy link

The potential risk here is that doing this verification off the fork choice thread makes it more difficult to guarantee the order of messages sent to the execution engine

This is very good point. There is a the Message ordering section in the design doc. But for the Merge F2F event I am going to suggest consensus clients to make synchronous calls. This is straightforward and doesn't affect aims that we have for the interop. It also allows for using HTTP as the communication protocol. Making these calls async is a must for production implementations and I think can be done later on considering the message ordering strategy and restrictions on the communication protocol that it will bring. This is what I'd like to raise during the ACD this week, I think we will get a rough agreement for synchronous approach.

So, I'd wait for the ACD before starting this portion of work. It will likely be deferred for after the interop.

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 a pull request may close this issue.

2 participants