Engine API: return error if forkchoice state is inconsistent #213
Conversation
src/engine/specification.md
Outdated
| @@ -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. | |||
There was a problem hiding this comment.
Does not exist? or is not in the chain defined by headBlockHash -- aka is inconsistent (which covers DNE as well)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There could be the case (likely if CL is buggy) when
headBlockdoes exist while eitherfinalizedBlockorsafeBlockdoes not.This PR proposes to have an explicit check in
engine_forkchoiceUpdatedprocessing flow and the corresponding error message in response to the call ifforkchoiceStateis found inconsistent.Should be rebased with #211