Allow zero safeBlockHash and finalizedBlockHash after finalization#760
Allow zero safeBlockHash and finalizedBlockHash after finalization#760dapplion wants to merge 1 commit intoethereum:mainfrom
Conversation
c943ecb to
7736063
Compare
| - `finalizedBlockHash`: `DATA`, 32 Bytes - block hash of the most recent finalized block | ||
|
|
||
| *Note:* `safeBlockHash` and `finalizedBlockHash` fields are allowed to have `0x0000000000000000000000000000000000000000000000000000000000000000` value unless transition block is finalized. | ||
| *Note:* `safeBlockHash` and `finalizedBlockHash` fields are allowed to have `0x0000000000000000000000000000000000000000000000000000000000000000` value. When set to zero, the client **MUST** default to the latest known value for the corresponding field, or the genesis block hash if no value is known. |
There was a problem hiding this comment.
The safeBlockHash with FCR implementation may point to an already unsafe block on the EL side. In this case EL should rather revert it to finalizedBlockHash
There was a problem hiding this comment.
How would you phrase that?
There was a problem hiding this comment.
| *Note:* `safeBlockHash` and `finalizedBlockHash` fields are allowed to have `0x0000000000000000000000000000000000000000000000000000000000000000` value. When set to zero, the client **MUST** default to the latest known value for the corresponding field, or the genesis block hash if no value is known. | |
| *Note:* `safeBlockHash` and `finalizedBlockHash` fields are allowed to have `0x0000000000000000000000000000000000000000000000000000000000000000` value. When set to zero, the client **MUST** default both values to the latest known `finalizedBlockHash` value, or the genesis block hash if no `finalizedBlockHash` value is known. |
Will the above work?
There was a problem hiding this comment.
I think we should make this an explicit statement in the fcU spec below to aid visibility.
| - `finalizedBlockHash`: `DATA`, 32 Bytes - block hash of the most recent finalized block | ||
|
|
||
| *Note:* `safeBlockHash` and `finalizedBlockHash` fields are allowed to have `0x0000000000000000000000000000000000000000000000000000000000000000` value unless transition block is finalized. | ||
| *Note:* `safeBlockHash` and `finalizedBlockHash` fields are allowed to have `0x0000000000000000000000000000000000000000000000000000000000000000` value. When set to zero, the client **MUST** default to the latest known value for the corresponding field, or the genesis block hash if no value is known. |
There was a problem hiding this comment.
I think the zero case is especially important for the transition period, changing this now is possible since we have transitioned, but it does not feel very "clean" to "change" the behavior at the transition. I also realize that not being backwards compatible should not be a big problem (assume chains have already transitioned or new chains start in PoS mode) so not a super strong opinion here, but one suggestion:
How about adding this rule to forkchoiceUpdatedV2 and all FcUs after, instead of forkchoiceUpdatedV1? This would keep the "old" behavior and ship new behavior to a fork after the transition.
There was a problem hiding this comment.
I think that zero hash would work for both situations:
- When transition block isn’t yet finalized, there is yet no known finalized block on EL side and it’s fine to mark genesis block as final
- When there is already a finalized block, EL will stick with it unless a non-zero hash comes from the CL side
Summary
safeBlockHashandfinalizedBlockHashmust be non-zero once the transition block is finalized-38002invalid forkchoice state check now only applies to non-zero hashesMotivation
During extended non-finality periods, a CL client checkpoint-syncing from a non-finalized state may not yet know the network's finalized or safe block hash. The current spec forces the CL to provide non-zero values post-merge, which is not always possible. This change lets the CL signal "unknown" by sending zeroes, with the EL falling back to its last known state.
The other options are:
See also: sigp/lighthouse#8382