clean up clearanceState access (#7518)

* clean up `clearanceState` access

There are several assumptions in the code about the clearance state that
lead to fragility - resolve some of these by moving things around a bit

* advance clearance state together with proposal fcU
* pass state in `OnBlockAdded` to emphasize the lifetime of its validity
* remove some code duplication between block_processor / sync_overseer
(there's a lot more to do but it's scheduled for dedicated refactoring
so do just the minimum)
* move blob/column validation to its own proc shared between
backfill/head blocks
* avoid pointless block lookup from dag when processing block
attestations

* push sidecar style selection up the call chain

Author: arnetheduck

beacon_chain/consensus_object_pools/block_clearance.nim

@@ -34,12 +34,13 @@ proc addResolvedHeadBlock(
        trustedBlock: ForkyTrustedSignedBeaconBlock,
        optimisticStatus: OptimisticStatus,
        parent: BlockRef, cache: var StateCache,
-       onBlockAdded: OnForkyBlockAdded,
+       onBlockAdded: OnBlockAdded,
        stateDataDur, sigVerifyDur, stateVerifyDur: Duration
      ): BlockRef =
   doAssert state.matches_block_slot(
     trustedBlock.root, trustedBlock.message.slot),
     "Given state must have the new block applied"
+  const consensusFork = typeof(trustedBlock).kind
 
   let
     blockRoot = trustedBlock.root
@@ -100,12 +101,15 @@ proc addResolvedHeadBlock(
   # Notify others of the new block before processing the quarantine, such that
   # notifications for parents happens before those of the children
   if onBlockAdded != nil:
-    let unrealized = withState(state):
+    let unrealized =
       when consensusFork >= ConsensusFork.Altair:
-        forkyState.data.compute_unrealized_finality()
+        state.forky(consensusFork).data.compute_unrealized_finality()
       else:
-        forkyState.data.compute_unrealized_finality(cache)
-    onBlockAdded(blockRef, trustedBlock, epochRef, unrealized)
+        state.forky(consensusFork).data.compute_unrealized_finality(cache)
+    onBlockAdded(
+      blockRef, trustedBlock, state.forky(consensusFork).data, epochRef, unrealized
+    )
+
   if not(isNil(dag.onBlockAdded)):
     dag.onBlockAdded(ForkedTrustedSignedBeaconBlock.init(trustedBlock))
 
@@ -135,29 +139,6 @@ proc checkStateTransition(
   else:
     ok()
 
-proc advanceClearanceState*(dag: ChainDAGRef, nextSlot: Slot) =
-  # When the chain is synced, the most likely block to be produced is the block
-  # right after head - we can exploit this assumption and advance the state
-  # to that slot before the block arrives, thus allowing us to do the expensive
-  # epoch transition ahead of time.
-  # Notably, we use the clearance state here because that's where the block will
-  # first be seen - later, this state will be copied to the head state!
-  let head = dag.head
-  if dag.clearanceState.matches_block_slot(head.root, nextSlot):
-    return
-
-  let startTick = Moment.now()
-  var cache = StateCache()
-  if dag.updateState(
-    dag.clearanceState,
-    BlockSlotId.init(head.bid, nextSlot),
-    true,
-    cache,
-    dag.updateFlags,
-  ):
-    debug "Prepared clearance state for next block",
-      nextSlot, head, updateStateDur = Moment.now() - startTick
-
 proc checkHeadBlock*(
     dag: ChainDAGRef, signedBlock: ForkySignedBeaconBlock):
     Result[BlockRef, VerifierError] =
@@ -228,7 +209,7 @@ proc checkHeadBlock*(
 proc addHeadBlockWithParent*(
     dag: ChainDAGRef, verifier: var BatchVerifier,
     signedBlock: ForkySignedBeaconBlock, parent: BlockRef,
-    optimisticStatus: OptimisticStatus, onBlockAdded: OnForkyBlockAdded
+    optimisticStatus: OptimisticStatus, onBlockAdded: OnBlockAdded
     ): Result[BlockRef, VerifierError] =
   ## Try adding a block to the chain, verifying first that it passes the state
   ## transition function and contains correct cryptographic signature.
@@ -347,8 +328,7 @@ proc addBackfillBlock*(
           info "Invalid genesis block signature"
           return err(VerifierError.Invalid)
       else:
-        let proposerKey = dag.validatorKey(blck.proposer_index)
-        if proposerKey.isNone():
+        let proposerKey = dag.validatorKey(blck.proposer_index).valueOr:
           # We've verified that the block root matches our expectations by following
           # the chain of parents all the way from checkpoint. If all those blocks
           # were valid, the proposer_index in this block must also be valid, and we
@@ -365,7 +345,7 @@ proc addBackfillBlock*(
             getStateField(dag.headState, genesis_validators_root),
             blck.slot,
             signedBlock.root,
-            proposerKey.get(),
+            proposerKey,
             signedBlock.signature):
           info "Block signature verification failed"
           return err(VerifierError.Invalid)
@@ -455,26 +435,6 @@ proc addBackfillBlock*(
 
   ok()
 
-template BlockAdded(kind: static ConsensusFork): untyped =
-  when kind == ConsensusFork.Gloas:
-    OnGloasBlockAdded
-  elif kind == ConsensusFork.Fulu:
-    OnFuluBlockAdded
-  elif kind == ConsensusFork.Electra:
-    OnElectraBlockAdded
-  elif kind == ConsensusFork.Deneb:
-    OnDenebBlockAdded
-  elif kind == ConsensusFork.Capella:
-    OnCapellaBlockAdded
-  elif kind == ConsensusFork.Bellatrix:
-    OnBellatrixBlockAdded
-  elif kind == ConsensusFork.Altair:
-    OnAltairBlockAdded
-  elif kind == ConsensusFork.Phase0:
-    OnPhase0BlockAdded
-  else:
-    static: raiseAssert "Unreachable"
-
 proc verifyBlockProposer*(
     verifier: var BatchVerifier,
     fork: Fork,
@@ -494,73 +454,57 @@ proc verifyBlockProposer*(
 
 proc addBackfillBlockData*(
     dag: ChainDAGRef,
+    consensusFork: static ConsensusFork,
     bdata: BlockData,
     onStateUpdated: OnStateUpdated,
-    onBlockAdded: OnForkedBlockAdded
+    onBlockAdded: OnBlockAdded,
 ): Result[void, VerifierError] =
   var cache = StateCache()
+  template forkyBlck: untyped = bdata.blck.forky(consensusFork)
+  let
+    parent = checkHeadBlock(dag, forkyBlck).valueOr:
+      if error == VerifierError.Duplicate:
+        return ok()
+      return err(error)
+    startTick = Moment.now()
+    clearanceBlock = BlockSlotId.init(parent.bid, forkyBlck.message.slot)
+    updateFlags1 = dag.updateFlags
+      # TODO (cheatfate): {skipLastStateRootCalculation} flag here could
+      # improve performance by 100%, but this approach needs some
+      # improvements, which is unclear.
+
+  if not updateState(dag, dag.clearanceState, clearanceBlock, true, cache,
+                      updateFlags1):
+    error "Unable to load clearance state for parent block, " &
+          "database corrupt?", clearanceBlock = shortLog(clearanceBlock)
+    return err(VerifierError.MissingParent)
 
-  withBlck(bdata.blck):
-    let
-      parent = checkHeadBlock(dag, forkyBlck).valueOr:
-        if error == VerifierError.Duplicate:
-          return ok()
-        return err(error)
-      startTick = Moment.now()
-      clearanceBlock = BlockSlotId.init(parent.bid, forkyBlck.message.slot)
-      updateFlags1 = dag.updateFlags
-        # TODO (cheatfate): {skipLastStateRootCalculation} flag here could
-        # improve performance by 100%, but this approach needs some
-        # improvements, which is unclear.
-
-    if not updateState(dag, dag.clearanceState, clearanceBlock, true, cache,
-                       updateFlags1):
-      error "Unable to load clearance state for parent block, " &
-            "database corrupt?", clearanceBlock = shortLog(clearanceBlock)
-      return err(VerifierError.MissingParent)
+  let proposerVerifyTick = Moment.now()
 
-    let proposerVerifyTick = Moment.now()
-
-    if not(isNil(onStateUpdated)):
-      ? onStateUpdated(forkyBlck.message.slot)
-
-    let
-      stateDataTick = Moment.now()
-      updateFlags2 =
-        dag.updateFlags + {skipBlsValidation, skipStateRootValidation}
-
-    ? checkStateTransition(dag, forkyBlck.asSigVerified(), cache, updateFlags2)
-
-    let stateVerifyTick = Moment.now()
-
-    if bdata.blob.isSome():
-      for blob in bdata.blob.get():
-        dag.db.putBlobSidecar(blob[])
-
-    type Trusted = typeof forkyBlck.asTrusted()
-
-    proc onBlockAddedHandler(
-        blckRef: BlockRef,
-        trustedBlock: Trusted,
-        epochRef: EpochRef,
-        unrealized: FinalityCheckpoints
-    ) {.gcsafe, raises: [].} =
-      onBlockAdded(
-        blckRef,
-        ForkedTrustedSignedBeaconBlock.init(trustedBlock),
-        epochRef,
-        unrealized)
-
-    let blockHandler: BlockAdded(consensusFork) = onBlockAddedHandler
-
-    discard addResolvedHeadBlock(
-      dag, dag.clearanceState,
-      forkyBlck.asTrusted(),
-      OptimisticStatus.notValidated,
-      parent, cache,
-      blockHandler,
-      proposerVerifyTick - startTick,
-      stateDataTick - proposerVerifyTick,
-      stateVerifyTick - stateDataTick)
+  if not(isNil(onStateUpdated)):
+    ? onStateUpdated(forkyBlck.message.slot)
+
+  let
+    stateDataTick = Moment.now()
+    updateFlags2 =
+      dag.updateFlags + {skipBlsValidation, skipStateRootValidation}
+
+  ? checkStateTransition(dag, forkyBlck.asSigVerified(), cache, updateFlags2)
+
+  let stateVerifyTick = Moment.now()
+
+  if bdata.blob.isSome():
+    for blob in bdata.blob.get():
+      dag.db.putBlobSidecar(blob[])
+
+  discard addResolvedHeadBlock(
+    dag, dag.clearanceState,
+    forkyBlck.asTrusted(),
+    OptimisticStatus.notValidated,
+    parent, cache,
+    onBlockAdded,
+    proposerVerifyTick - startTick,
+    stateDataTick - proposerVerifyTick,
+    stateVerifyTick - stateDataTick)
 
   ok()

beacon_chain/consensus_object_pools/block_pools_types.nim

@@ -299,25 +299,9 @@ type
     blck*: ForkedSignedBeaconBlock
     blob*: Opt[BlobSidecars]
 
-  OnBlockAdded*[T: ForkyTrustedSignedBeaconBlock] = proc(
-    blckRef: BlockRef, blck: T, epochRef: EpochRef,
-    unrealized: FinalityCheckpoints) {.gcsafe, raises: [].}
-  OnPhase0BlockAdded* = OnBlockAdded[phase0.TrustedSignedBeaconBlock]
-  OnAltairBlockAdded* = OnBlockAdded[altair.TrustedSignedBeaconBlock]
-  OnBellatrixBlockAdded* = OnBlockAdded[bellatrix.TrustedSignedBeaconBlock]
-  OnCapellaBlockAdded* = OnBlockAdded[capella.TrustedSignedBeaconBlock]
-  OnDenebBlockAdded* = OnBlockAdded[deneb.TrustedSignedBeaconBlock]
-  OnElectraBlockAdded* = OnBlockAdded[electra.TrustedSignedBeaconBlock]
-  OnFuluBlockAdded* = OnBlockAdded[fulu.TrustedSignedBeaconBlock]
-  OnGloasBlockAdded* = OnBlockAdded[gloas.TrustedSignedBeaconBlock]
-
-  OnForkyBlockAdded* =
-    OnPhase0BlockAdded | OnAltairBlockAdded | OnBellatrixBlockAdded |
-    OnCapellaBlockAdded | OnDenebBlockAdded | OnElectraBlockAdded |
-    OnFuluBlockAdded | OnGloasBlockAdded
-
-  OnForkedBlockAdded* = proc(
-    blckRef: BlockRef, blck: ForkedTrustedSignedBeaconBlock, epochRef: EpochRef,
+  OnBlockAdded*[consensusFork: static ConsensusFork] = proc(
+    blckRef: BlockRef, blck: consensusFork.TrustedSignedBeaconBlock,
+    state: consensusFork.BeaconState, epochRef: EpochRef,
     unrealized: FinalityCheckpoints) {.gcsafe, raises: [].}
 
   OnStateUpdated* = proc(
@@ -356,26 +340,6 @@ type
     slot*: Slot
     block_root* {.serializedFieldName: "block".}: Eth2Digest
 
-template OnBlockAddedCallback*(kind: static ConsensusFork): auto =
-  when kind == ConsensusFork.Gloas:
-    typedesc[OnGloasBlockAdded]
-  elif kind == ConsensusFork.Fulu:
-    typedesc[OnFuluBlockAdded]
-  elif kind == ConsensusFork.Electra:
-    typedesc[OnElectraBlockAdded]
-  elif kind == ConsensusFork.Deneb:
-    typedesc[OnDenebBlockAdded]
-  elif kind == ConsensusFork.Capella:
-    typedesc[OnCapellaBlockAdded]
-  elif kind == ConsensusFork.Bellatrix:
-    typedesc[OnBellatrixBlockAdded]
-  elif kind == ConsensusFork.Altair:
-    typedesc[OnAltairBlockAdded]
-  elif kind == ConsensusFork.Phase0:
-    typedesc[OnPhase0BlockAdded]
-  else:
-    static: raiseAssert "Unreachable"
-
 func proposer_dependent_slot*(epochRef: EpochRef): Slot =
   epochRef.key.epoch.proposer_dependent_slot()
 

beacon_chain/consensus_object_pools/blockchain_dag.nim

@@ -990,53 +990,10 @@ proc applyBlock(
     updateFlags: UpdateFlags): Result[void, cstring] =
   loadStateCache(dag, cache, bid, getStateField(state, slot).epoch)
 
-  discard case dag.cfg.consensusForkAtEpoch(bid.slot.epoch)
-  of ConsensusFork.Phase0:
-    let data = getBlock(dag, bid, phase0.TrustedSignedBeaconBlock).valueOr:
+  withConsensusFork(dag.cfg.consensusForkAtEpoch(bid.slot.epoch)):
+    let data = getBlock(dag, bid, consensusFork.TrustedSignedBeaconBlock).valueOr:
       return err("Block load failed")
-    ? state_transition(
-      dag.cfg, state, data, cache, info,
-      updateFlags + {slotProcessed}, noRollback)
-  of ConsensusFork.Altair:
-    let data = getBlock(dag, bid, altair.TrustedSignedBeaconBlock).valueOr:
-      return err("Block load failed")
-    ? state_transition(
-      dag.cfg, state, data, cache, info,
-      updateFlags + {slotProcessed}, noRollback)
-  of ConsensusFork.Bellatrix:
-    let data = getBlock(dag, bid, bellatrix.TrustedSignedBeaconBlock).valueOr:
-      return err("Block load failed")
-    ? state_transition(
-      dag.cfg, state, data, cache, info,
-      updateFlags + {slotProcessed}, noRollback)
-  of ConsensusFork.Capella:
-    let data = getBlock(dag, bid, capella.TrustedSignedBeaconBlock).valueOr:
-      return err("Block load failed")
-    ? state_transition(
-      dag.cfg, state, data, cache, info,
-      updateFlags + {slotProcessed}, noRollback)
-  of ConsensusFork.Deneb:
-    let data = getBlock(dag, bid, deneb.TrustedSignedBeaconBlock).valueOr:
-      return err("Block load failed")
-    ? state_transition(
-      dag.cfg, state, data, cache, info,
-      updateFlags + {slotProcessed}, noRollback)
-  of ConsensusFork.Electra:
-    let data = getBlock(dag, bid, electra.TrustedSignedBeaconBlock).valueOr:
-      return err("Block load failed")
-    ? state_transition(
-      dag.cfg, state, data, cache, info,
-      updateFlags + {slotProcessed}, noRollback)
-  of ConsensusFork.Fulu:
-    let data = getBlock(dag, bid, fulu.TrustedSignedBeaconBlock).valueOr:
-      return err("Block load failed")
-    ? state_transition(
-      dag.cfg, state, data, cache, info,
-      updateFlags + {slotProcessed}, noRollback)
-  of ConsensusFork.Gloas:
-    let data = getBlock(dag, bid, gloas.TrustedSignedBeaconBlock).valueOr:
-      return err("Block load failed")
-    ? state_transition(
+    discard ? state_transition(
       dag.cfg, state, data, cache, info,
       updateFlags + {slotProcessed}, noRollback)