Skip to content

Engine API: return error if forkchoice state is inconsistent #213

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
May 10, 2022
Merged

Engine API: return error if forkchoice state is inconsistent #213

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f5e780e
Engine API: return error if forkchoice state is inconsistent
mkalinin Apr 29, 2022
e9053b6
Adjust error code with JSON-RPC spec
mkalinin May 3, 2022
cfc8374
Merge branch 'main' into invalid-forkchoice-state
mkalinin May 6, 2022
6438091
Harden forkchoice state validity condition
mkalinin May 6, 2022
02f55b2
Fix steps in forkchoiceUpdated method spec
mkalinin May 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Engine API: return error if forkchoice state is inconsistent
  • Loading branch information
@mkalinin
mkalinin committed Apr 29, 2022
commit f5e780ec6be9d5c3f476c94744cd69098f338181
12 changes: 8 additions & 4 deletions src/engine/specification.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ The list of error codes introduced by this specification can be found below.
| -32603 | Internal error | Internal JSON-RPC error. |
| -32000 | Server error | Generic client error while processing request. |
| -32001 | Unknown payload | Payload does not exist / is not available. |
| -32003 | Invalid forkchoice state | Forkchoice state is invalid / inconsistent. |

Each error returns a `null` `data` value, except `-32000` which returns the `data` object with a `err` member that explains the error encountered.

Expand Down Expand Up @@ -301,16 +302,19 @@ The payload build process is specified as follows:
* The values `(forkchoiceState.headBlockHash, forkchoiceState.finalizedBlockHash)` of this method call map on the `POS_FORKCHOICE_UPDATED` event of [EIP-3675](https://eips.ethereum.org/EIPS/eip-3675#block-validity) and **MUST** be processed according to the specification defined in the EIP
* All updates to the forkchoice state resulting from this call **MUST** be made atomically.

6. Client software **MUST** begin a payload build process building on top of `forkchoiceState.headBlockHash` and identified via `buildProcessId` value if `payloadAttributes` is not `null` and the forkchoice state has been updated successfully. The build process is specified in the [Payload building](#payload-building) section.
6. Client software **MUST** return `-32003: Invalid forkchoice state` error if the payload referenced by `forkchoiceState.headBlockHash` is `VALID` and a payload referenced by either `forkchoiceState.finalizedBlockHash` or `forkchoiceState.safeBlockHash` does not exist.
Copy link
Contributor

@djrtwo djrtwo Apr 29, 2022

Choose a reason for hiding this comment

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

Does not exist? or is not in the chain defined by headBlockHash -- aka is inconsistent (which covers DNE as well)

Copy link
Contributor Author

@mkalinin mkalinin May 1, 2022

Choose a reason for hiding this comment

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

EL will have to traverse the blockchain to check if safeBlock and finalizedBlock do belong to the same chain as a headBlock does. This kind of check is expensive to be run per each fcU call. I agree that proving existence doesn't necessarily prove the consistency (I would call it a weak consistency check), but it looks like a reasonable trade-off in this case.

Copy link
Contributor

@djrtwo djrtwo May 3, 2022

Choose a reason for hiding this comment

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

It is actually expensive?
The existence check is cheap because they just maintain some sort of hash map for all blocks? But not for a given chain?

Copy link
Contributor Author

@mkalinin mkalinin May 6, 2022

Choose a reason for hiding this comment

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

Apparently, I have overestimated EL implementation complexity. Most of EL clients are fine with full consistency check. I've made the corresponding adjustment to the spec. Also, I have swapped codes for payload attributes and forkchoice state inconsistency errors to make codes respect the order they appear in the spec.


7. Client software **MUST** respond to this method call in the following way:
7. Client software **MUST** begin a payload build process building on top of `forkchoiceState.headBlockHash` and identified via `buildProcessId` value if `payloadAttributes` is not `null` and the forkchoice state has been updated successfully. The build process is specified in the [Payload building](#payload-building) section.

8. Client software **MUST** respond to this method call in the following way:
* `{payloadStatus: {status: SYNCING, latestValidHash: null, validationError: null}, payloadId: null}` if `forkchoiceState.headBlockHash` references an unknown payload or a payload that can't be validated because requisite data for the validation is missing
* `{payloadStatus: {status: INVALID, latestValidHash: validHash, validationError: errorMessage | null}, payloadId: null}` obtained from the [Payload validation](#payload-validation) process if the payload is deemed `INVALID`
* `{payloadStatus: {status: INVALID_TERMINAL_BLOCK, latestValidHash: null, validationError: errorMessage | null}, payloadId: null}` obtained either from the [Payload validation](#payload-validation) process or as a result of validating a PoW block referenced by `forkchoiceState.headBlockHash`
* `{payloadStatus: {status: VALID, latestValidHash: forkchoiceState.headBlockHash, validationError: null}, payloadId: null}` if the payload is deemed `VALID` and a build process hasn't been started
* `{payloadStatus: {status: VALID, latestValidHash: forkchoiceState.headBlockHash, validationError: null}, payloadId: buildProcessId}` if the payload is deemed `VALID` and the build process has begun.
* `{payloadStatus: {status: VALID, latestValidHash: forkchoiceState.headBlockHash, validationError: null}, payloadId: buildProcessId}` if the payload is deemed `VALID` and the build process has begun
* `{error: {code: -32003, message: "Invalid forkchoice state"}}` if `forkchoiceState` is either invalid or inconsistent.

8. If any of the above fails due to errors unrelated to the normal processing flow of the method, client software **MUST** respond with an error object.
9. If any of the above fails due to errors unrelated to the normal processing flow of the method, client software **MUST** respond with an error object.

### engine_getPayloadV1

Expand Down