Clarify and limit engine_forkchoiceUpdated validation semantics for safeBlockHash

[gnattishness]

Concerns

Consider the following excerpt of the engine_forkchoiceUpdated specification:

execution-apis/src/engine/paris.md

Line 215 in 03911ff

5. Client software **MUST** update its forkchoice state if payloads referenced by `forkchoiceState.headBlockHash` and `forkchoiceState.finalizedBlockHash` are `VALID`. The update is specified as follows:

5. Client software MUST update its forkchoice state if payloads referenced by forkchoiceState.headBlockHash and forkchoiceState.finalizedBlockHash are VALID. The update is specified as follows:

1. Missing safeBlockHash:

This contains no mention of how and if execution layer clients validate safeBlockHash.
As such, it implies that the software MUST update its forkchoice state even if the safeBlockHash references an INVALID block, which may be unexpected for client implementations.

I appreciate that a consensus layer client is non-compliant if they provide an invalid safeBlockHash thanks to this part of its field definition:

This value MUST be either equal to or an ancestor of headBlockHash

However, I believe it's safer to also specify consistent behavior for the EL client, and point 6 allows for the possibility of receiving an out-of-chain safeBlockHash.

2. Missing "belong to the same chain" requirement:

Point 5 only requires that the finalized and head blocks are VALID, not that they are in the same chain.
As such, a fully compliant client would need to update its state even if the final block was in a different chain.

While point 6 includes "belong to the same chain", it only defines how the EL client should respond, not how it should update its state.

6. Client software MUST return -38002: Invalid forkchoice state error ... does not belong to the chain defined by forkchoiceState.headBlockHash.

Current client behavior

Geth

When receiving a forkChoiceState with VALID headBlockHash and finalizedBlockHash, but an INVALID or out-of-chain safeBlockHash, Geth currently handles this by updating the head and finalized blocks in the chain, but leaving the safe block unchanged.

It also requires that the safe and finalized blocks are part of the same chain as

It still returns -38002: Invalid forkchoice state as required by point 6, but the state of the head and finalized blocks has been updated.
It does not roll back these updates.

Safe block validation:
https://github.com/ethereum/go-ethereum/blob/7ed52c949e66ca7874ba39e8d69b451615382edc/eth/catalyst/api.go#L345

Same chain validation:
https://github.com/ethereum/go-ethereum/blob/7ed52c949e66ca7874ba39e8d69b451615382edc/eth/catalyst/api.go#L337-L339

Recommendation

• Clarify point 5 to include whether EL clients are expected to update the forkchoice state when the safeBlockHash is INVALID
• Clarify point 5 to include "same chain" requirement
• Clarify whether partial updates to internal fork choice state are allowed

One solution could look like

5. Client software MUST update its forkchoice state if and only if payloads referenced by forkchoiceState.headBlockHash, forkchoiceState.safeBlockHash and forkchoiceState.finalizedBlockHash are VALID and belong to the same chain. The update is specified as follows:

Note that Geth would not currently abide by this more strict requirement (it doesn't handle and only if because it partially updates the state)
Maybe a relaxed version is preferred to maintain backwards compatibility?
I could also discuss and only if in a separate issue.

[michaelsproul]

I like your proposed solution to validate all three block blocks and ensure they are all on the same chain prior to making any state updates.