Make ExecutionHash not option in ForkChoiceParams#8901
Conversation
| /// | ||
| /// This `bool` only exists to satisfy our SSZ implementation which requires all variants | ||
| /// to have a value. It can be set to anything. | ||
| Irrelevant(bool), |
There was a problem hiding this comment.
@michaelsproul we know that no blocks in fork-choice have this status. If we remove this variant knowing that it is un-used, we are fine right?
There was a problem hiding this comment.
I think we should update the v29 schema migration to do Irrelevant -> Valid(0x00). That would require keeping this variant around a little longer too.
|
Let's hold up to have a design of fork-choice Gloas |
michaelsproul
added
fork-choice
and removed
ready-for-review
dapplion
force-pushed
the
execution-hash-not-option
branch
3 times, most recently
from
6fde121 to
353a886
Compare
- Remove ExecutionStatus::Irrelevant and make execution hashes non-optional - Handle pre-merge blocks with default payloads as valid - Remove cached head state from fork choice - Replace unwrap_or_default with proper error handling for block lookups - Dedup forkchoice_update_parameters into helper function - Handle pre-merge anchor blocks without execution payloads - Handle pre-merge blocks without execution payloads - Document forkchoice_update_parameters failure conditions
dapplion
force-pushed
the
execution-hash-not-option
branch
from
353a886 to
06c94b1
Compare
| /// The `execution_payload.block_hash` of the block at the head of the chain. | ||
| head_hash: ExecutionBlockHash, | ||
| /// The `execution_payload.block_hash` of the justified block. | ||
| justified_hash: ExecutionBlockHash, | ||
| /// The `execution_payload.block_hash` of the finalized block. | ||
| finalized_hash: ExecutionBlockHash, |
There was a problem hiding this comment.
This is the key change: when the head changes we MUST know the execution hashes of its head / just / fin blocks
| .get_block_execution_block_hash(&fork_choice.finalized_checkpoint().root) | ||
| .ok_or(Error::BlockMissingFromForkChoice( | ||
| fork_choice.finalized_checkpoint().root, | ||
| ))?, |
There was a problem hiding this comment.
We expect fork-choice to contain nodes for the head + justified block + finalized block. This invariant is always true and pruning removes nodes prior to finalized and justified is always a descendant of finalized. When we initialize fork-choice there's always a node for the justified and finalized checkpoint
| // Gloas blocks don't contain an execution payload. | ||
| Some(PayloadVerificationStatus::Irrelevant) | ||
| } else if is_execution_enabled(state, block.message().body()) { | ||
| Some(PayloadVerificationStatus::Verified) |
There was a problem hiding this comment.
We coerce Irrelevant status to Verified to not have to deal with Option return types. Semantically it's the same.
| /// Attestations that arrived at the current slot and must be queued for later processing. | ||
| queued_attestations: Vec<QueuedAttestation>, | ||
| /// Stores a cache of the values required to be sent to the execution layer. | ||
| forkchoice_update_parameters: ForkchoiceUpdateParameters, |
There was a problem hiding this comment.
Caching this here is unnecessary. The values are already cached in canonical head. Removing them from here simplifies reasoning about them and moves the error condition (justified / finalized nodes not present) outside of the fork-choice code
| .ok_or(Error::MissingPersistedForkChoice)?; | ||
| let current_slot_for_head = fork_choice.fc_store().get_current_slot(); | ||
| let (_, head_payload_status) = fork_choice.get_head(current_slot_for_head, spec)?; | ||
| let fork_choice_view = fork_choice.cached_fork_choice_view(); |
There was a problem hiding this comment.
No cached view, we recompute the head on init
| ExecutionStatus::Valid(ExecutionBlockHash::zero()), | ||
| None, | ||
| None, | ||
| ) |
There was a problem hiding this comment.
Fork-choice does not process pre-merge blocks, and this change is cosmetic. If we accept having Valid(0) for this case, the simplicity of not having to think about Option in downstream code is worth it.
| // V29 (Gloas) nodes don't have execution_status. Use | ||
| // execution_payload_block_hash which is always present on V29 | ||
| // (the zero fallback is cosmetic and unreachable). | ||
| execution_status: block.execution_status().unwrap_or_else(|_| { |
There was a problem hiding this comment.
Could maybe use None for Gloas instead, to prevent misuse like:
- get_head -> block root B
- get_block(B)
- look at block_B.execution_status
- assume block_B.execution_status is valid (e.g. send to EL in fcU), even though we may not have seen this block
Or another option could be to check payload_received and set Valid if it is true, or Optimistic otherwise. Although this is also a conflation.
Replace the `Valid(0)` coercion at the canonical-head / fork-choice-update
boundary with an explicit `FcuHash { PreMerge, Hash }` enum.
`ForkchoiceUpdateParameters` and `CachedHead`'s head/justified/finalized
hash fields move from `ExecutionBlockHash` to `FcuHash`. The Engine API
boundary still uses zero as the documented sentinel for "no terminal
block known"; convert with `FcuHash::to_engine_hash` only there.
Pre-merge detection switches from `== ExecutionBlockHash::zero()` to
`is_pre_merge()` in `is_healthy` and the startup fcU guard.
Internally proto-array still stores `ExecutionStatus::Valid(zero)` for
pre-merge blocks; pushing `PreMerge` deeper would touch SSZ on-disk
schema and is left as a follow-up.
dapplion
force-pushed
the
execution-hash-not-option
branch
from
7d3c80a to
f90ecdf
Compare
2 participants
Issue Addressed
Builds on #8761. Now that merge transition code is being removed, execution block hashes no longer need to be Option types throughout the fork choice and beacon chain.
Two key changes: