ethereum / execution-apis

Collection of APIs provided by Ethereum execution layer clients
Creative Commons Zero v1.0 Universal
940 stars 373 forks source link

Clarify and limit `engine_forkchoiceUpdated` validation semantics for `safeBlockHash` #548

Open gnattishness opened 4 months ago

gnattishness commented 4 months ago

Concerns

Consider the following excerpt of the engine_forkchoiceUpdated specification:

https://github.com/ethereum/execution-apis/blob/03911ffc053b8b806123f1fc237184b0092a485a/src/engine/paris.md?plain=1#L215

  1. 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.

  1. 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

One solution could look like

  1. 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 commented 4 months ago

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.