track block/blob matching/quarantines using both indices and commitments (#5621)

beacon_chain/consensus_object_pools/blob_quarantine.nim

@@ -16,7 +16,8 @@ const
 
 type
   BlobQuarantine* = object
-    blobs*: OrderedTable[(Eth2Digest, BlobIndex), ref BlobSidecar]
+    blobs*:
+      OrderedTable[(Eth2Digest, BlobIndex, KzgCommitment), ref BlobSidecar]
   BlobFetchRecord* = object
     block_root*: Eth2Digest
     indices*: seq[BlobIndex]
@@ -32,22 +33,19 @@ func put*(quarantine: var BlobQuarantine, blobSidecar: ref BlobSidecar) =
     # FIFO if full. For example, sync manager and request manager can race to
     # put blobs in at the same time, so one gets blob insert -> block resolve
     # -> blob insert sequence, which leaves garbage blobs.
-    var oldest_blob_key: (Eth2Digest, BlobIndex)
+    #
+    # This also therefore automatically garbage-collects otherwise valid garbage
+    # blobs which are correctly signed, point to either correct block roots or a
+    # block root which isn't ever seen, and then are for any reason simply never
+    # used.
+    var oldest_blob_key: (Eth2Digest, BlobIndex, KzgCommitment)
     for k in quarantine.blobs.keys:
       oldest_blob_key = k
       break
     quarantine.blobs.del oldest_blob_key
   let block_root = hash_tree_root(blobSidecar.signed_block_header.message)
   discard quarantine.blobs.hasKeyOrPut(
-    (block_root, blobSidecar.index), blobSidecar)
-
-func blobIndices*(quarantine: BlobQuarantine, digest: Eth2Digest):
-     seq[BlobIndex] =
-  var r: seq[BlobIndex] = @[]
-  for i in 0..<MAX_BLOBS_PER_BLOCK:
-    if quarantine.blobs.hasKey((digest, i)):
-      r.add(i)
-  r
+    (block_root, blobSidecar.index, blobSidecar.kzg_commitment), blobSidecar)
 
 func hasBlob*(
     quarantine: BlobQuarantine,
@@ -62,22 +60,20 @@ func hasBlob*(
       return true
   false
 
-func popBlobs*(quarantine: var BlobQuarantine, digest: Eth2Digest):
-     seq[ref BlobSidecar] =
+func popBlobs*(
+    quarantine: var BlobQuarantine, digest: Eth2Digest,
+    blck: deneb.SignedBeaconBlock): seq[ref BlobSidecar] =
   var r: seq[ref BlobSidecar] = @[]
-  for i in 0..<MAX_BLOBS_PER_BLOCK:
+  for idx, kzg_commitment in blck.message.body.blob_kzg_commitments:
     var b: ref BlobSidecar
-    if quarantine.blobs.pop((digest, i), b):
+    if quarantine.blobs.pop((digest, BlobIndex idx, kzg_commitment), b):
       r.add(b)
   r
 
 func hasBlobs*(quarantine: BlobQuarantine, blck: deneb.SignedBeaconBlock):
      bool =
-  let idxs = quarantine.blobIndices(blck.root)
-  if len(blck.message.body.blob_kzg_commitments) != len(idxs):
-    return false
-  for i in 0..<len(idxs):
-    if idxs[i] != uint64(i):
+  for idx, kzg_commitment in blck.message.body.blob_kzg_commitments:
+    if (blck.root, BlobIndex idx, kzg_commitment) notin quarantine.blobs:
       return false
   true
 
@@ -86,6 +82,7 @@ func blobFetchRecord*(quarantine: BlobQuarantine, blck: deneb.SignedBeaconBlock)
   var indices: seq[BlobIndex]
   for i in 0..<len(blck.message.body.blob_kzg_commitments):
     let idx = BlobIndex(i)
-    if not quarantine.blobs.hasKey((blck.root, idx)):
+    if not quarantine.blobs.hasKey(
+        (blck.root, idx, blck.message.body.blob_kzg_commitments[i])):
       indices.add(idx)
   BlobFetchRecord(block_root: blck.root, indices: indices)

beacon_chain/gossip_processing/block_processor.nim

@@ -746,7 +746,8 @@ proc storeBlock(
              error = res.error()
             continue
           if self.blobQuarantine[].hasBlobs(forkyBlck):
-            let blobs = self.blobQuarantine[].popBlobs(forkyBlck.root)
+            let blobs = self.blobQuarantine[].popBlobs(
+              forkyBlck.root, forkyBlck)
             self[].enqueueBlock(MsgSource.gossip, quarantined, Opt.some(blobs))
           else:
             if not self.consensusManager.quarantine[].addBlobless(

beacon_chain/gossip_processing/eth2_processor.nim

@@ -242,7 +242,7 @@ proc processSignedBeaconBlock*(
     let blobs =
       when typeof(signedBlock).kind >= ConsensusFork.Deneb:
         if self.blobQuarantine[].hasBlobs(signedBlock):
-          Opt.some(self.blobQuarantine[].popBlobs(signedBlock.root))
+          Opt.some(self.blobQuarantine[].popBlobs(signedBlock.root, signedBlock))
         else:
           if not self.quarantine[].addBlobless(self.dag.finalizedHead.slot,
                                                signedBlock):
@@ -310,7 +310,7 @@ proc processBlobSidecar*(
       self.blockProcessor[].enqueueBlock(
         MsgSource.gossip,
         ForkedSignedBeaconBlock.init(blobless),
-        Opt.some(self.blobQuarantine[].popBlobs(block_root)))
+        Opt.some(self.blobQuarantine[].popBlobs(block_root, blobless)))
     else:
       discard self.quarantine[].addBlobless(self.dag.finalizedHead.slot,
                                             blobless)

beacon_chain/nimbus_beacon_node.nim

@@ -408,7 +408,7 @@ proc initFullNode(
                 Result[void, VerifierError].err(VerifierError.MissingParent),
                 "rmanBlockVerifier")
           else:
-            let blobs = blobQuarantine[].popBlobs(forkyBlck.root)
+            let blobs = blobQuarantine[].popBlobs(forkyBlck.root, forkyBlck)
             blockProcessor[].addBlock(MsgSource.gossip, signedBlock,
                                       Opt.some(blobs),
                                       maybeFinalized = maybeFinalized)

beacon_chain/spec/state_transition.nim

@@ -504,8 +504,6 @@ proc makeBeaconBlock*(
   else:
     {.error: "Unsupported fork".}
 
-# workaround for https://github.com/nim-lang/Nim/issues/20900 rather than have
-# these be default arguments
 proc makeBeaconBlock*(
     cfg: RuntimeConfig, state: var ForkedHashedBeaconState,
     proposer_index: ValidatorIndex, randao_reveal: ValidatorSig,

beacon_chain/sync/request_manager.nim

@@ -293,7 +293,6 @@ proc getMissingBlobs(rman: RequestManager): seq[BlobIdentifier] =
       if len(missing.indices) == 0:
         warn "quarantine missing blobs, but missing indices is empty",
          blk=blobless.root,
-         indices=rman.blobQuarantine[].blobIndices(blobless.root),
          commitments=len(blobless.message.body.blob_kzg_commitments)
       for idx in missing.indices:
         let id = BlobIdentifier(block_root: blobless.root, index: idx)
@@ -303,7 +302,6 @@ proc getMissingBlobs(rman: RequestManager): seq[BlobIdentifier] =
       # this is a programming error should it occur.
       warn "missing blob handler found blobless block with all blobs",
          blk=blobless.root,
-         indices=rman.blobQuarantine[].blobIndices(blobless.root),
          commitments=len(blobless.message.body.blob_kzg_commitments)
       discard rman.blockVerifier(ForkedSignedBeaconBlock.init(blobless),
                                  false)

beacon_chain/validators/beacon_validators.nim

@@ -560,8 +560,6 @@ proc makeBeaconBlockForHeadAndSlot*(
   else:
     err(blck.error)
 
-# workaround for https://github.com/nim-lang/Nim/issues/20900 to avoid default
-# parameters
 proc makeBeaconBlockForHeadAndSlot*(
     PayloadType: type ForkyExecutionPayloadForSigning, node: BeaconNode, randao_reveal: ValidatorSig,
     validator_index: ValidatorIndex, graffiti: GraffitiBytes, head: BlockRef,