don't send fcUs every block if in lc-opt regime and block putatively finalized #5248
Conversation
| finalizedBlockHash = newHead.get.finalizedExecutionPayloadHash, | ||
| payloadAttributes = none attributes) | ||
| if NewPayloadStatus.noResponse != payloadStatus and | ||
| not self.consensusManager[].optimisticExecutionPayloadHash.isZero: |
There was a problem hiding this comment.
this looks weird - we use the finalized EPH from the block but the light client EPH for head? I thought the light client gives a finalized head too?
There was a problem hiding this comment.
Noticed this as well.
My read is that the LC isn't trusted for FC purposes here for finalization, which in theory might trigger the EL to act in irrevocable, or irrevocable without disproportionate computation/storage cost, ways.
Nimbus still effectively treats the LC as extra-consensus, purely on top of it, an aid to reaching consensus, rather than consensus itself, and it's okay for FC head to bounce around in case that goes wrong in a ways it's not for finalized/safe to.
There was a problem hiding this comment.
yes, critically though it might happen that the FC consensus is behind (for example if LC connectivity is broken due to lack of peers)
There was a problem hiding this comment.
Exactly, the LC is used to optimistically sync the EL. It is not used to provide safety. Once DAG catches up, its head is used instead of the LC head. Safe/Finalized is always reported based on the fork choice store (highest finality observed on any branch by DAG).
There was a problem hiding this comment.
well, this feels kind of sketchy - seems like we should be looking at the max of the two syncs potentially ensuring that they share ancestry or something like that... otherwise, this code looks to me like finality from the block can end up being from a slot more recent than the block from LC?
There was a problem hiding this comment.
All of this is guarded by if self.consensusManager[].shouldSyncOptimistically(wallSlot):
func shouldSyncOptimistically*(
optimisticSlot, dagSlot, wallSlot: Slot): bool =
## Determine whether an optimistic execution block hash should be reported
## to the EL client instead of the current head as determined by fork choice.
# Check whether optimistic head is sufficiently ahead of DAG
const minProgress = 8 * SLOTS_PER_EPOCH # Set arbitrarily
if optimisticSlot < dagSlot or optimisticSlot - dagSlot < minProgress:
return false
# Check whether optimistic head has synced sufficiently close to wall slot
const maxAge = 2 * SLOTS_PER_EPOCH # Set arbitrarily
if optimisticSlot < max(wallSlot, maxAge.Slot) - maxAge:
return false
true
func shouldSyncOptimistically*(self: ConsensusManager, wallSlot: Slot): bool =
if self.optimisticHead.execution_block_hash.isZero:
return false
shouldSyncOptimistically(
optimisticSlot = self.optimisticHead.bid.slot,
dagSlot = getStateField(self.dag.headState, slot),
wallSlot = wallSlot)so, LC head is only used if DAG head is far behind and LC head is recent.
That is, render it consistent with the non-lc-opt regime, which already has this check for fcU.
Also remove a small optimization for now-irrelevant pre-Bellatrix which makes code a little less clear and more redundant (two different places it returns
ZERO_HASHfor the same reason).