Unify block proposer verification (#7675)

* verify proposer index/signature the same way across
blocks/blobs/columns/quarantine
* rename block clearance helpers to differentiate backfill vs light
forward syncing and signature versus proposer checking (the latter
includes checking the proposer index against the state)
* avoid redundant parent lookup when re-queueing quarantine blocks
* delay `Forked...` block init in quarantine until the block is actually
being added

Author: arnetheduck

beacon_chain/consensus_object_pools/block_clearance.nim

@@ -28,6 +28,39 @@ export results, signatures_batch, block_dag, blockchain_dag
 logScope:
   topics = "clearance"
 
+proc verifyBlockProposer*(
+    dag: ChainDAGRef,
+    parent: BlockRef,
+    slot: Slot,
+    proposer_index: uint64,
+    blockRoot: Eth2Digest,
+    signature: ValidatorSig,
+): Result[void, tuple[msg: cstring, invalid: bool]] =
+  ## Verify block proposer and signature, making sure to check that the proposer
+  ## was indeed elected for the given slot and that the signature checks out.
+  ##
+  ## Because the signature only covers the block body (via its root), it's
+  ## possible that the block itself is valid while the signature is not: in
+  ## this case false is returned for "invalid".
+  let proposer = dag.getProposer(parent, slot).valueOr:
+    warn "cannot compute proposer for block", parent, slot, proposer_index, blockRoot
+    return err(("verifyBlockProposer: cannot compute proposer", false)) # internal issue
+
+  # `getProposer` returns a trusted proposer index, while `proposer_index` may
+  # be invalid -> convert the former to the latter
+  if uint64(proposer) != proposer_index:
+    return err(("verifyBlockProposer: unexpected proposer", true))
+
+  let
+    proposerKey = dag.validatorKey(proposer).expect("valid after getProposer")
+    fork = dag.forkAtEpoch(slot.epoch)
+  if not verify_block_signature(
+    fork, dag.genesis_validators_root, slot, blockRoot, proposerKey, signature
+  ):
+    return err(("verifyBlockProposer: invalid signature", false))
+
+  ok()
+
 proc addResolvedHeadBlock(
        dag: ChainDAGRef,
        state: var ForkedHashedBeaconState,
@@ -435,7 +468,7 @@ proc addBackfillBlock*(
 
   ok()
 
-proc verifyBlockProposer*(
+proc verifyBlockSignatures*(
     verifier: var BatchVerifier,
     fork: Fork,
     genesis_validators_root: Eth2Digest,
@@ -452,7 +485,7 @@ proc verifyBlockProposer*(
   else:
     ok()
 
-proc addBackfillBlockData*(
+proc addLightForwardBlock*(
     dag: ChainDAGRef,
     consensusFork: static ConsensusFork,
     bdata: BlockData,

beacon_chain/consensus_object_pools/block_quarantine.nim

@@ -301,17 +301,17 @@ func pruneAfterFinalization*(
 proc addOrphan*(
     quarantine: var Quarantine,
     finalizedSlot: Slot,
-    signedBlock: ForkedSignedBeaconBlock
+    signedBlock: ForkySignedBeaconBlock
 ): Result[void, cstring] =
   ## Adds block to quarantine's `orphans` and `missing` lists.
 
-  if not isViable(finalizedSlot, getForkedBlockField(signedBlock, slot)):
+  if not isViable(finalizedSlot, signedBlock.message.slot):
     quarantine.addUnviable(signedBlock.root) # will remove from missing
     return err("block unviable")
 
   quarantine.cleanupOrphans(finalizedSlot)
 
-  let parent_root = getForkedBlockField(signedBlock, parent_root)
+  let parent_root = signedBlock.message.parent_root
 
   if parent_root in quarantine.unviable:
     quarantine.addUnviable(signedBlock.root)
@@ -335,7 +335,8 @@ proc addOrphan*(
     quarantine.orphans.del oldest_orphan_key
     quarantine.sidecarless.del oldest_orphan_key[0]
 
-  quarantine.orphans[(signedBlock.root, signedBlock.signature)] = signedBlock
+  quarantine.orphans[(signedBlock.root, signedBlock.signature)] =
+    ForkedSignedBeaconBlock.init(signedBlock)
   quarantine.orphansEvent.fire()
 
   ok()

beacon_chain/consensus_object_pools/blockchain_dag.nim

@@ -999,6 +999,9 @@ proc applyBlock(
 
   ok()
 
+proc genesis_validators_root*(dag: ChainDAGRef): Eth2Digest =
+  getStateField(dag.headState, genesis_validators_root)
+
 proc init*(T: type ChainDAGRef, cfg: RuntimeConfig, db: BeaconChainDB,
            validatorMonitor: ref ValidatorMonitor, updateFlags: UpdateFlags,
            eraPath = ".",
@@ -1183,8 +1186,7 @@ proc init*(T: type ChainDAGRef, cfg: RuntimeConfig, db: BeaconChainDB,
     quit 1
 
   # Need to load state to find genesis validators root, before loading era db
-  dag.era = EraDB.new(
-    cfg, eraPath, getStateField(dag.headState, genesis_validators_root))
+  dag.era = EraDB.new(cfg, eraPath, dag.genesis_validators_root)
 
   # We used an interim finalizedHead while loading the head state above - now
   # that we have loaded the dag up to the finalized slot, we can also set
@@ -1255,8 +1257,7 @@ proc init*(T: type ChainDAGRef, cfg: RuntimeConfig, db: BeaconChainDB,
         slot: dag.tail.slot + 1,
         parent_root: dag.tail.root)
 
-  dag.forkDigests = newClone ForkDigests.init(
-    cfg, getStateField(dag.headState, genesis_validators_root))
+  dag.forkDigests = newClone ForkDigests.init(cfg, dag.genesis_validators_root)
 
   withState(dag.headState):
     dag.validatorMonitor[].registerState(forkyState.data)
@@ -1344,9 +1345,6 @@ proc init*(T: type ChainDAGRef, cfg: RuntimeConfig, db: BeaconChainDB,
 
   dag
 
-template genesis_validators_root*(dag: ChainDAGRef): Eth2Digest =
-  getStateField(dag.headState, genesis_validators_root)
-
 proc genesisBlockRoot*(dag: ChainDAGRef): Eth2Digest =
   dag.db.getGenesisBlock().expect("DB must be initialized with genesis block")
 

beacon_chain/consensus_object_pools/blockchain_list.nim

@@ -167,7 +167,7 @@ proc checkBlobs(signedBlock: ForkedSignedBeaconBlock,
             return err(VerifierError.Invalid)
   ok()
 
-proc addBackfillBlockData*(
+proc addLightForwardBlock*(
     clist: ChainListRef, signedBlock: ForkedSignedBeaconBlock,
     blobsOpt: Opt[BlobSidecars]): Result[void, VerifierError] =
   doAssert(not(isNil(clist)))
@@ -243,5 +243,5 @@ proc untrustedBackfillVerifier*(
 ): Future[Result[void, VerifierError]] {.
   async: (raises: [CancelledError], raw: true).} =
   let retFuture = newFuture[Result[void, VerifierError]]()
-  retFuture.complete(clist.addBackfillBlockData(signedBlock, blobs))
+  retFuture.complete(clist.addLightForwardBlock(signedBlock, blobs))
   retFuture

beacon_chain/gossip_processing/block_processor.nim

@@ -157,7 +157,7 @@ proc dumpInvalidBlock*(
 proc dumpBlock(
     self: BlockProcessor,
     signedBlock: ForkySignedBeaconBlock,
-    res: Result[void, VerifierError]) =
+    res: Result[BlockRef, VerifierError]) =
   if self.dumpEnabled and res.isErr:
     case res.error
     of VerifierError.Invalid:
@@ -168,7 +168,7 @@ proc dumpBlock(
       discard
 
 from ../consensus_object_pools/block_clearance import
-  addBackfillBlock, addHeadBlockWithParent, checkHeadBlock
+  addBackfillBlock, addHeadBlockWithParent, checkHeadBlock, verifyBlockProposer
 
 proc verifySidecars(
     signedBlock: ForkySignedBeaconBlock,
@@ -179,10 +179,7 @@ proc verifySidecars(
   when consensusFork == ConsensusFork.Gloas:
     # For Gloas, we still need to store the columns if they're provided
     # but skip validation since we don't have kzg_commitments in the block
-    if sidecarsOpt.isSome:
-      debugGloasComment "potentially validate against payload envelope"
-      let columns = sidecarsOpt.get()
-      discard
+    debugGloasComment "potentially validate against payload envelope"
   elif consensusFork == ConsensusFork.Fulu:
     if sidecarsOpt.isSome:
       let columns = sidecarsOpt.get()
@@ -332,29 +329,6 @@ proc getExecutionValidity(
 
   Opt.some(optimisticStatus)
 
-proc checkBlobOrColumnlessSignature(
-    self: BlockProcessor,
-    signed_beacon_block: deneb.SignedBeaconBlock | electra.SignedBeaconBlock |
-                         fulu.SignedBeaconBlock):
-    Result[void, cstring] =
-  let dag = self.consensusManager.dag
-  let parent = dag.getBlockRef(signed_beacon_block.message.parent_root).valueOr:
-    return err("checkBlobOrColumnlessSignature called with orphan block")
-  let proposer = getProposer(
-        dag, parent, signed_beacon_block.message.slot).valueOr:
-    return err("checkBlobOrColumnlessSignature: Cannot compute proposer")
-  if distinctBase(proposer) != signed_beacon_block.message.proposer_index:
-    return err("checkBlobOrColumnlessSignature: Incorrect proposer")
-  if not verify_block_signature(
-      dag.forkAtEpoch(signed_beacon_block.message.slot.epoch),
-      getStateField(dag.headState, genesis_validators_root),
-      signed_beacon_block.message.slot,
-      signed_beacon_block.root,
-      dag.validatorKey(proposer).get(),
-      signed_beacon_block.signature):
-    return err("checkBlobOrColumnlessSignature: Invalid proposer signature")
-  ok()
-
 proc addBlock*(
   self: ref BlockProcessor,
   src: MsgSource,
@@ -387,55 +361,46 @@ proc enqueueBlock*(
   # `addBlock` should be used where managing backpressure is appropriate.
   discard self.addBlock(src, blck, sidecarsOpt, maybeFinalized, validationDur)
 
-proc enqueueQuarantine(self: ref BlockProcessor, root: Eth2Digest) =
+proc enqueueQuarantine(self: ref BlockProcessor, parent: BlockRef) =
   ## Enqueue blocks whose parent is `root` - ie when `root` has been added to
   ## the blockchain dag, its direct descendants are now candidates for
   ## processing
-  for quarantined in self.consensusManager.quarantine[].pop(root):
+  let
+    dag = self.consensusManager[].dag
+    quarantine = self.consensusManager[].quarantine
+  for quarantined in quarantine[].pop(parent.root):
     # Process the blocks that had the newly accepted block as parent
-    debug "Block from quarantine",
-      blockRoot = shortLog(root), quarantined = shortLog(quarantined.root)
+    debug "Block from quarantine", parent, quarantined = shortLog(quarantined.root)
 
     withBlck(quarantined):
       when consensusFork == ConsensusFork.Gloas:
         debugGloasComment ""
-        self.enqueueBlock(
-          MsgSource.gossip, forkyBlck, Opt.none(gloas.DataColumnSidecars))
+        const sidecarsOpt = noSidecars
       elif consensusFork == ConsensusFork.Fulu:
-        if len(forkyBlck.message.body.blob_kzg_commitments) == 0:
-          self.enqueueBlock(
-            MsgSource.gossip, forkyBlck, Opt.some(fulu.DataColumnSidecars @[])
-          )
-        else:
-          if (let res = checkBlobOrColumnlessSignature(self[], forkyBlck); res.isErr):
-            warn "Failed to verify signature of unorphaned blobless block",
-              blck = shortLog(forkyBlck), error = res.error()
-            continue
-          let cres = self.dataColumnQuarantine[].popSidecars(forkyBlck.root, forkyBlck)
-          if cres.isSome:
-            self.enqueueBlock(MsgSource.gossip, forkyBlck, cres)
-          else:
-            discard self.consensusManager.quarantine[].addSidecarless(
-              self.consensusManager[].dag.finalizedHead.slot, forkyBlck
-            )
+        let sidecarsOpt =
+          self.dataColumnQuarantine[].popSidecars(forkyBlck.root, forkyBlck)
       elif consensusFork in ConsensusFork.Deneb .. ConsensusFork.Electra:
-        if len(forkyBlck.message.body.blob_kzg_commitments) == 0:
-          self.enqueueBlock(MsgSource.gossip, forkyBlck, Opt.some(BlobSidecars @[]))
-        else:
-          if (let res = checkBlobOrColumnlessSignature(self[], forkyBlck); res.isErr):
-            warn "Failed to verify signature of unorphaned columnless block",
-              blck = shortLog(forkyBlck), error = res.error()
-            continue
-          let bres = self.blobQuarantine[].popSidecars(forkyBlck.root, forkyBlck)
-          if bres.isSome():
-            self.enqueueBlock(MsgSource.gossip, forkyBlck, bres)
-          else:
-            self.consensusManager.quarantine[].addSidecarless(forkyBlck)
+        let sidecarsOpt = self.blobQuarantine[].popSidecars(forkyBlck.root, forkyBlck)
       elif consensusFork in ConsensusFork.Phase0 .. ConsensusFork.Capella:
-        self.enqueueBlock(MsgSource.gossip, forkyBlck, noSidecars)
+        const sidecarsOpt = noSidecars
       else:
         {.error: "Unknown consensus fork " & $consensusFork.}
 
+      when consensusFork in ConsensusFork.Deneb .. ConsensusFork.Fulu:
+        if not sidecarsOpt.isSome():
+          dag.verifyBlockProposer(
+            parent, forkyBlck.message.slot, forkyBlck.message.proposer_index,
+            forkyBlck.root, forkyBlck.signature,
+          ).isOkOr:
+            warn "Failed to verify signature of unorphaned blobless block",
+              blck = shortLog(forkyBlck), error = error.msg
+            continue
+
+          discard quarantine[].addSidecarless(dag.finalizedHead.slot, forkyBlck)
+          continue
+
+      self.enqueueBlock(MsgSource.gossip, forkyBlck, sidecarsOpt)
+
 proc onBlockAdded*(
     dag: ChainDAGRef,
     consensusFork: static ConsensusFork,
@@ -571,7 +536,7 @@ proc storeBlock(
     maybeFinalized: bool,
     queueTick: Moment,
     validationDur: Duration,
-): Future[Result[void, VerifierError]] {.async: (raises: [CancelledError]).} =
+): Future[Result[BlockRef, VerifierError]] {.async: (raises: [CancelledError]).} =
   ## storeBlock is the main entry point for unvalidated blocks - all untrusted
   ## blocks, regardless of origin, pass through here. When storing a block,
   ## we will add it to the dag and pass it to all block consumers that need
@@ -586,9 +551,9 @@ proc storeBlock(
     deadlineTime =
       block:
         let slotTime =
-          (wallSlot + 1).start_beacon_time(dag.timeParams) - 1.seconds
+          (wallSlot + 1).start_beacon_time(dag.timeParams) - chronos.seconds(1)
         if slotTime <= wallTime:
-          0.seconds
+          chronos.seconds(0)
         else:
           chronos.nanoseconds((slotTime - wallTime).nanoseconds)
     deadline = sleepAsync(deadlineTime)
@@ -725,7 +690,7 @@ proc storeBlock(
     blck = shortLog(blck),
     validationDur, queueDur, newPayloadDur, addHeadBlockDur, updateHeadDur
 
-  ok()
+  ok(blck)
 
 proc addBlock*(
     self: ref BlockProcessor,
@@ -776,7 +741,7 @@ proc addBlock*(
         # taking up all CPU - we don't want to _completely_ stop processing blocks
         # in this case - doing so also allows us to benefit from more batching /
         # larger network reads when under load.
-        idleTimeout = 10.milliseconds
+        idleTimeout = chronos.milliseconds(10)
 
       discard await idleAsync().withTimeout(idleTimeout)
 
@@ -799,15 +764,13 @@ proc addBlock*(
 
   if res.isOk():
     # Once a block is successfully stored, enqueue the direct descendants
-    self.enqueueQuarantine(blockRoot)
+    self.enqueueQuarantine(res[])
   else:
     case res.error()
     of VerifierError.MissingParent:
       let finalizedSlot = self.consensusManager.dag.finalizedHead.slot
       if (
-        let r = self.consensusManager.quarantine[].addOrphan(
-          finalizedSlot, ForkedSignedBeaconBlock.init(blck)
-        )
+        let r = self.consensusManager.quarantine[].addOrphan(finalizedSlot, blck)
         r.isErr()
       ):
         debug "Could not add orphan",
@@ -838,4 +801,4 @@ proc addBlock*(
     else:
       discard
 
-  res
+  res.mapConvert(void)