Engine API: extend semantics of `executePayload` and `forkchoiceUpdated` methods #165

mkalinin · 2022-01-21T12:57:08Z

The following changes to executePayload and forkchoiceUpdated methods are proposed:

Makes execution semantics optional for executePayload, and renames this method to newPayload to make the name sound
Adds an option to execute payload on receiving forkchoiceUpdated before updating the forkchoice state
Allows for EL client to skip updating the forkchoice state if forkchoiceUpdated references a historical block from the canonical chain that has been validating a while ago, i.e. forkchoice state may never go backwards if an update to the same chain is requested
Introduces ACCEPTED status. It may be sent in the response in the following cases:
- If CL sends a payload from a side fork and the parent block is missing, EL client software may just store this payload locally and no sync or payload validation process won't be initiated until this payload (or its descendant) will be designated as the head of the canonical chain
- If CL sends forkchoiceUpdated to make set the head to the payload that does exist and can be validated (parent block and state are available locally), but validation is not yet done and client software implementation doesn't induce validation process on forkchoiceUpdated. Probably, in this case it is better for EL client software to respond with IGNORED to better reflect the semantics of this response
Requires payload execution on the canonical chain to be unaffected by an active sync process if it is happening on a side branch of the block tree, and data required to validate the payload (the parent block and state) is locally available. This requirement disallows EL client software to respond SYNCING if newPayload sends a child of the head of the canonical chain when the software is catching up with the head of a side fork, it prevents node to turn optimistic (as per optimistic sync spec) for no reason
Introduces the blockHash check as the first step of newPayload processing flow; this validation must run disregarding the state of EL client, whether it's syncing or not

Additionally:

Moves Payload validation process to a separate section for the sake of modularity.
Introduces Sync process section to give a notion of the sync process to the spec. It may be removed if deemed superfluous, but could be useful in some sense

A separate EXECUTING/VALIDATING status has been previously considered to denote the state of EL client software in which it executes multiple blocks in a row in order to obtain the parent state to be able to validate a payload. But this operation has been kept under SYNCING status with the reflection in the Sync process description. Distinguishing these two different states is not that useful from the CL client software perspective as both doesn't give an understanding of how much time the operation will take to inform the CL behaviour.

holiman · 2022-01-21T14:22:13Z

src/engine/specification.md


-3. Client software **MUST** return `{status: SYNCING, latestValidHash: null}` if the sync process is already in progress or if requisite data for payload validation is missing. In the event that requisite data to validate the payload is missing (e.g. does not have payload identified by `parentHash`), the client software **SHOULD** initiate the sync process.
+1. Client software **SHOULD** validate the payload if requisite data for payload validation is locally available. The validation process is specified in the [Payload validation process](#payload-validation-process) section. Additionally:


From acd today: this should be made stricter, to not make it implementation-dependent so that clients all lazily choose not to validate any newPayloads at all

Yeah, I think it's more like the following:

MUST validate the payload if extending the canonical head, and MAY validate if on a non-canonical branch

djrtwo

Awesome! I have a few questions/clarifications in there. Let's get to those first.

Beyond that, I have some ideas about how to clean up or further modularize.
If any speak to you, please go ahead and do it.

If my suggestions aren't clear, I might take a stab at a cleanup PR after we address my core questions/clarifications about the core of the content

src/engine/specification.md

djrtwo · 2022-01-22T19:33:05Z

src/engine/specification.md


-3. Client software **MUST** return `{status: SYNCING, latestValidHash: null}` if the sync process is already in progress or if requisite data for payload validation is missing. In the event that requisite data to validate the payload is missing (e.g. does not have payload identified by `parentHash`), the client software **SHOULD** initiate the sync process.
+1. Client software **SHOULD** validate the payload if requisite data for payload validation is locally available. The validation process is specified in the [Payload validation process](#payload-validation-process) section. Additionally:


Yeah, I think it's more like the following:

MUST validate the payload if extending the canonical head, and MAY validate if on a non-canonical branch

src/engine/specification.md

djrtwo · 2022-01-22T20:16:31Z

src/engine/specification.md


-2. All updates to the forkchoice state resulting from this call **MUST** be made atomically.
+1. Client software **MAY** skip an update of the forkchoice state and **MAY NOT** begin a payload build process if the payload identified by `forkchoiceState.headBlockHash` hasn't been validated yet. Additionally:


If you follow the above suggestion to insert SYNCING logic into payloadValidation, then you need to update this statement to say:

if the payload identified by forkchoiceState.headBlockHash known but has not been validated yet

src/engine/specification.md

djrtwo

this is awesome! it's really cleaning up nicely :)

djrtwo · 2022-01-25T16:55:40Z

src/engine/specification.md


-1. Client software **MAY** skip an update of the forkchoice state and **MAY NOT** begin a payload build process if the payload identified by `forkchoiceState.headBlockHash` hasn't been validated yet. Additionally:
-    * Client software **MUST** return `{status: ACCEPTED, latestValidHash: null, validationError: null, payloadId: null}` in this case.
+1. Client software **MAY** skip an update of the forkchoice state and **MUST NOT** begin a payload build process if `forkchoiceState.headBlockHash` references an ancestor of the head of the canonical chain.


Should this be any non-leaf value? -- That is you MAY skip it if it isn't a leaf of the block tree

Otherwise you can abuse the rewind (turn this MAY into a MUST) by fcu to a non-canonical branch and then fcu to a non-leaf block in what was previously the canonical chain

MariusVanDerWijden · 2022-01-25T18:30:43Z

src/engine/specification.md

-* Client software **SHOULD** build the initial version of the payload which has an empty transaction set.
-* Client software **SHOULD** start the process of updating the payload. The strategy of this process is implementation dependent. The default strategy is to keep the transaction set up-to-date with the state of local mempool.
-* Client software **SHOULD** stop the updating process when either a call to `engine_getPayload` with the build process's `payloadId` is made or [`SECONDS_PER_SLOT`](https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/beacon-chain.md#time-parameters-1) (12s in the Mainnet configuration) have passed since the point in time identified by the `timestamp` parameter.
+1. If any of the above fails due to errors unrelated to the normal processing flow of the method, the client software **MUST** return an error.


So errors in the payload assembling process are returned as JSON_RPC errors directly?

Not sure I understand your question correctly. But if an exception occurred during initiating a payload build process then the call of this method must be responded with error.

Generally, this point states that in the case when e.g. a payload is INVALID according to EE rules an INVALID status must be in the response, but if exception happened during the validation then the call must be responded with error, not an INVALID status.

What should EL return if we receive the wrong timestamp in payload attributes? Because I think INVALID is connected to headBlockHash, not payload attributes. By the wrong timestamp, I mean headBlockHash.Timestamp >= payloadAttributes.Timestamp

It would mean that CL is faulty, EL may respond with error in this case. There is -32602: Invalid params error that fits well into this case

djrtwo

very minor copy suggestions. i'll fix them

src/engine/specification.md

walido0z

nice jov

Engine API: extend executePayload and forkchoiceUpdated semantics

9b5ff91

This was referenced Jan 21, 2022

Engine API: Make SYNCING response and according actions fork specific #152

Closed

Ethereum Core Devs Meeting 130 Agenda ethereum/pm#449

Closed

holiman reviewed Jan 21, 2022

View reviewed changes

timbeiko mentioned this pull request Jan 21, 2022

Consensus-layer Call 80 ethereum/pm#458

Closed

djrtwo reviewed Jan 22, 2022

View reviewed changes

This was referenced Jan 23, 2022

Update Merge Engine API (Kiln v1) erigontech/interfaces#91

Merged

Update Merge Engine API (Kiln v1) ledgerwatch/erigon-lib#268

Merged

New Engine API semantics (Kiln v1) erigontech/erigon#3340

Merged

terencechain mentioned this pull request Jan 24, 2022

Optimistic Sync ethereum/consensus-specs#2770

Merged

Address Danny's feedback

0324389

djrtwo reviewed Jan 25, 2022

View reviewed changes

MariusVanDerWijden reviewed Jan 25, 2022

View reviewed changes

djrtwo mentioned this pull request Jan 25, 2022

Release updated merge specs ethereum/consensus-specs#2815

Merged

3 tasks

mkalinin added 3 commits January 26, 2022 19:16

Engine API: refine semantics changes

e269920

Fix spellchecker

c5a7fb3

Fix spellchecker, take 2

56efc7b

djrtwo approved these changes Jan 26, 2022

View reviewed changes

minor copy edits from code review

3edcc1e

djrtwo reviewed Jan 26, 2022

View reviewed changes

src/engine/specification.md Outdated Show resolved Hide resolved

Update src/engine/specification.md

24996ff

djrtwo merged commit f040e6d into ethereum:main Jan 26, 2022

mkalinin mentioned this pull request Jan 27, 2022

Bellatrix: Rename execute_payload to notify_new_payload ethereum/consensus-specs#2817

Merged

daniellehrner mentioned this pull request Feb 2, 2022

MERGE: extend semantics of executePayload / newPayload method hyperledger/besu#3365

Closed

ryanio mentioned this pull request Feb 9, 2022

Client: Kiln🔥🧱 Tracking Issue ethereumjs/ethereumjs-monorepo#1703

Closed

11 tasks

walido0z reviewed Jun 14, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Engine API: extend semantics of `executePayload` and `forkchoiceUpdated` methods #165

Engine API: extend semantics of `executePayload` and `forkchoiceUpdated` methods #165

mkalinin commented Jan 21, 2022 •

edited

Loading

holiman Jan 21, 2022

djrtwo Jan 22, 2022

djrtwo left a comment

djrtwo Jan 22, 2022

djrtwo Jan 22, 2022

djrtwo left a comment

djrtwo Jan 25, 2022

MariusVanDerWijden Jan 25, 2022

mkalinin Jan 26, 2022

MarekM25 Jan 28, 2022

mkalinin Jan 29, 2022

djrtwo left a comment

walido0z left a comment


		3. Client software MUST return `{status: SYNCING, latestValidHash: null}` if the sync process is already in progress or if requisite data for payload validation is missing. In the event that requisite data to validate the payload is missing (e.g. does not have payload identified by `parentHash`), the client software SHOULD initiate the sync process.
		1. Client software SHOULD validate the payload if requisite data for payload validation is locally available. The validation process is specified in the [Payload validation process](#payload-validation-process) section. Additionally:


		2. All updates to the forkchoice state resulting from this call MUST be made atomically.
		1. Client software MAY skip an update of the forkchoice state and MAY NOT begin a payload build process if the payload identified by `forkchoiceState.headBlockHash` hasn't been validated yet. Additionally:

Engine API: extend semantics of executePayload and forkchoiceUpdated methods #165

Engine API: extend semantics of executePayload and forkchoiceUpdated methods #165

Conversation

mkalinin commented Jan 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djrtwo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djrtwo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djrtwo left a comment

Choose a reason for hiding this comment

walido0z left a comment

Choose a reason for hiding this comment

Engine API: extend semantics of `executePayload` and `forkchoiceUpdated` methods #165

Engine API: extend semantics of `executePayload` and `forkchoiceUpdated` methods #165

mkalinin commented Jan 21, 2022 •

edited

Loading