Simplify block quarantine blobless (#4824)

* Simplify block quarantine blobless

The quarantine blobless table was initially keyed off of (Eth2Digest,
ValidatorSig). This was modelled off the orphan table. The presence of
the signature in the key is necessary for orphans, because we can't
verify the signature for an orphan. That is not the case for a
blobless block, where the signature can be verified.

So this PR changes the blobless block table to be keyed off a
Eth2Digest only. This simplifies the retrieval and handling of
blobless blocks.

* review feedback

beacon_chain/consensus_object_pools/block_quarantine.nim

@@ -8,7 +8,7 @@
 {.push raises: [].}
 
 import
-  std/[tables],
+  std/[options, tables],
   stew/bitops2,
   ../spec/forks
 
@@ -47,9 +47,9 @@ type
       ## invalid signature won't cause a block with a valid signature
       ## to be dropped. An orphan block may also be "blobless" (see
       ## below) - if so, upon resolving the parent, it should be
-      ## stored in the blobless table.
+      ## added to the blobless table, after verifying its signature.
 
-    blobless*: Table[(Eth2Digest, ValidatorSig), deneb.SignedBeaconBlock]
+    blobless*: Table[Eth2Digest, deneb.SignedBeaconBlock]
       ## Blocks that we don't have blobs for. When we have received
       ## all blobs for this block, we can proceed to resolving the
       ## block as well. A blobless block inserted into this table must
@@ -128,7 +128,7 @@ func removeOrphan*(
 
 func removeBlobless*(
   quarantine: var Quarantine, signedBlock: ForkySignedBeaconBlock) =
-  quarantine.blobless.del((signedBlock.root, signedBlock.signature))
+  quarantine.blobless.del(signedBlock.root)
 
 func isViable(
     finalizedSlot: Slot, slot: Slot): bool =
@@ -144,10 +144,11 @@ func cleanupUnviable(quarantine: var Quarantine) =
       break # Cannot modify while for-looping
     quarantine.unviable.del(toDel)
 
-func removeUnviableTree[T](quarantine: var Quarantine,
+func removeUnviableOrphanTree(quarantine: var Quarantine,
                         toCheck: var seq[Eth2Digest],
                         tbl: var Table[(Eth2Digest, ValidatorSig),
-                                       T]): seq[Eth2Digest] =
+                                       ForkedSignedBeaconBlock]):
+                                         seq[Eth2Digest] =
   # Remove the tree of orphans whose ancestor is unviable - they are now also
   # unviable! This helps avoiding junk in the quarantine, because we don't keep
   # unviable parents in the DAG and there's no way to tell an orphan from an
@@ -160,13 +161,7 @@ func removeUnviableTree[T](quarantine: var Quarantine,
     if root notin checked:
       checked.add(root)
     for k, v in tbl.mpairs():
-      when T is ForkedSignedBeaconBlock:
       let blockRoot = getForkedBlockField(v, parent_root)
-      elif T is ForkySignedBeaconBlock:
-        let blockRoot = v.message.parent_root
-      else:
-        static: doAssert false
-
       if blockRoot == root:
         toCheck.add(k[0])
         toRemove.add(k)
@@ -181,14 +176,36 @@ func removeUnviableTree[T](quarantine: var Quarantine,
 
   checked
 
+func removeUnviableBloblessTree(quarantine: var Quarantine,
+                                toCheck: var seq[Eth2Digest],
+                                tbl: var Table[Eth2Digest,
+                                               deneb.SignedBeaconBlock]) =
+  var
+    toRemove: seq[Eth2Digest] # Can't modify while iterating
+  while toCheck.len > 0:
+    let root = toCheck.pop()
+    for k, v in tbl.mpairs():
+      let blockRoot = v.message.parent_root
+      if blockRoot == root:
+        toCheck.add(k)
+        toRemove.add(k)
+      elif k == root:
+        toRemove.add(k)
+
+    for k in toRemove:
+      tbl.del k
+      quarantine.unviable[k] = ()
+
+    toRemove.setLen(0)
+
 func addUnviable*(quarantine: var Quarantine, root: Eth2Digest) =
   if root in quarantine.unviable:
     return
 
   quarantine.cleanupUnviable()
   var toCheck = @[root]
-  var checked = quarantine.removeUnviableTree(toCheck, quarantine.orphans)
-  discard quarantine.removeUnviableTree(checked, quarantine.blobless)
+  var checked = quarantine.removeUnviableOrphanTree(toCheck, quarantine.orphans)
+  quarantine.removeUnviableBloblessTree(checked, quarantine.blobless)
 
   quarantine.unviable[root] = ()
 
@@ -204,14 +221,14 @@ func cleanupOrphans(quarantine: var Quarantine, finalizedSlot: Slot) =
     quarantine.orphans.del k
 
 func cleanupBlobless(quarantine: var Quarantine, finalizedSlot: Slot) =
-  var toDel: seq[(Eth2Digest, ValidatorSig)]
+  var toDel: seq[Eth2Digest]
 
   for k, v in quarantine.blobless:
     if not isViable(finalizedSlot, v.message.slot):
       toDel.add k
 
   for k in toDel:
-    quarantine.addUnviable k[0]
+    quarantine.addUnviable k
     quarantine.blobless.del k
 
 func clearAfterReorg*(quarantine: var Quarantine) =
@@ -286,12 +303,14 @@ proc addBlobless*(
   if quarantine.blobless.lenu64 >= MaxBlobless:
     return false
 
-  quarantine.blobless[(signedBlock.root, signedBlock.signature)] = signedBlock
+  quarantine.blobless[signedBlock.root] = signedBlock
   quarantine.missing.del(signedBlock.root)
   true
 
-iterator peekBlobless*(quarantine: var Quarantine, root: Eth2Digest):
-         deneb.SignedBeaconBlock =
-  for k, v in quarantine.blobless.mpairs():
-    if k[0] == root:
-      yield v
+func peekBlobless*(quarantine: var Quarantine, root: Eth2Digest):
+         Option[deneb.SignedBeaconBlock] =
+  var blck: deneb.SignedBeaconBlock
+  if quarantine.blobless.pop(root, blck):
+    return some(blck)
+  else:
+    return none(deneb.SignedBeaconBlock)

beacon_chain/gossip_processing/block_processor.nim

@@ -10,13 +10,15 @@
 import
   stew/results,
   chronicles, chronos, metrics,
-  ../spec/signatures_batch,
+  ../spec/[signatures, signatures_batch],
   ../sszdump
 
 from ../consensus_object_pools/consensus_manager import
   ConsensusManager, checkNextProposer, optimisticExecutionPayloadHash,
   runProposalForkchoiceUpdated, shouldSyncOptimistically, updateHead,
   updateHeadWithExecution
+from ../consensus_object_pools/blockchain_dag import
+  getBlockRef, getProposer, forkAtEpoch, validatorKey
 from ../beacon_clock import GetBeaconTimeFn, toFloatSeconds
 from ../consensus_object_pools/block_dag import BlockRef, root, shortLog, slot
 from ../consensus_object_pools/block_pools_types import
@@ -334,6 +336,26 @@ proc getExecutionValidity(
     error "getExecutionValidity: newPayload failed", err = err.msg
     return NewPayloadStatus.noResponse
 
+proc checkBloblessSignature(self: BlockProcessor,
+                            signed_beacon_block: deneb.SignedBeaconBlock):
+                              Result[void, cstring] =
+  let dag = self.consensusManager.dag
+  let parent = dag.getBlockRef(signed_beacon_block.message.parent_root).valueOr:
+    return err("checkBloblessSignature called with orphan block")
+  let proposer = getProposer(
+        dag, parent, signed_beacon_block.message.slot).valueOr:
+    return err("checkBloblessSignature: Cannot compute proposer")
+  if uint64(proposer) != signed_beacon_block.message.proposer_index:
+    return err("checkBloblessSignature: 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("checkBloblessSignature: Invalid proposer signature")
+
 proc storeBlock*(
     self: ref BlockProcessor, src: MsgSource, wallTime: BeaconTime,
     signedBlock: ForkySignedBeaconBlock,
@@ -575,6 +597,11 @@ proc storeBlock*(
         if len(blck.message.body.blob_kzg_commitments) == 0:
           self[].addBlock(MsgSource.gossip, quarantined, BlobSidecars @[])
         else:
+          if (let res = checkBloblessSignature(self[], blck); res.isErr):
+            warn "Failed to verify signature of unorphaned blobless block",
+             blck = shortLog(blck),
+             error = res.error()
+            continue
           if self.blobQuarantine[].hasBlobs(blck):
             let blobs = self.blobQuarantine[].popBlobs(blck.root)
             self[].addBlock(MsgSource.gossip, quarantined, blobs)

beacon_chain/gossip_processing/eth2_processor.nim

@@ -305,26 +305,17 @@ proc processSignedBlobSidecar*(
 
   var skippedBlocks = false
 
-  for blobless in self.quarantine[].peekBlobless(
-    signedBlobSidecar.message.block_root):
-    if self.blobQuarantine[].hasBlobs(blobless):
-      toAdd.add(blobless)
-    else:
-      skippedBlocks = true
+  if (let o = self.quarantine[].peekBlobless(
+    signedBlobSidecar.message.block_root); o.isSome):
+    let blobless = o.unsafeGet()
 
-  if len(toAdd) > 0:
-    let blobs = self.blobQuarantine[].peekBlobs(
-      signedBlobSidecar.message.block_root)
-    for blobless in toAdd:
+    if self.blobQuarantine[].hasBlobs(blobless):
       self.blockProcessor[].addBlock(
         MsgSource.gossip,
-        ForkedSignedBeaconBlock.init(blobless), blobs)
-      self.quarantine[].removeBlobless(blobless)
-
-    if not skippedBlocks:
-      # no blobless blocks remain in quarantine that need these blobs,
-      # so we can remove them.
-      self.blobQuarantine[].removeBlobs(toAdd[0].root)
+        ForkedSignedBeaconBlock.init(blobless),
+        self.blobQuarantine[].popBlobs(
+          signedBlobSidecar.message.block_root)
+      )
 
   blob_sidecars_received.inc()
   blob_sidecar_delay.observe(delay.toFloatSeconds())

tests/test_block_quarantine.nim

@@ -56,16 +56,16 @@ suite "Block quarantine":
       quarantine.addBlobless(Slot 0, b6)
 
       (b4.root, ValidatorSig()) in quarantine.orphans
-      (b5.root, ValidatorSig()) in quarantine.blobless
-      (b6.root, ValidatorSig()) in quarantine.blobless
+      b5.root in quarantine.blobless
+      b6.root in quarantine.blobless
 
     quarantine.addUnviable(b4.root)
 
     check:
       (b4.root, ValidatorSig()) notin quarantine.orphans
 
-      (b5.root, ValidatorSig()) in quarantine.blobless
-      (b6.root, ValidatorSig()) notin quarantine.blobless
+      b5.root in quarantine.blobless
+      b6.root notin quarantine.blobless
 
     quarantine.addUnviable(b1.root)
 
@@ -74,5 +74,5 @@ suite "Block quarantine":
       (b2.root, ValidatorSig()) notin quarantine.orphans
       (b3.root, ValidatorSig()) notin quarantine.orphans
 
-      (b5.root, ValidatorSig()) notin quarantine.blobless
-      (b6.root, ValidatorSig()) notin quarantine.blobless
+      b5.root notin quarantine.blobless
+      b6.root notin quarantine.blobless