Clarify addOrphan error/logging (#4981)

* Clarify addOrphan error/logging

addOrphan returned a bool to indicate success. Change this to a Result
so that different errors can be distinguished.

* Update beacon_chain/consensus_object_pools/block_quarantine.nim

Co-authored-by: tersec <tersec@users.noreply.github.com>

* Update beacon_chain/gossip_processing/gossip_validation.nim

---------

Co-authored-by: tersec <tersec@users.noreply.github.com>

beacon_chain/consensus_object_pools/block_quarantine.nim

@@ -251,11 +251,11 @@ func clearAfterReorg*(quarantine: var Quarantine) =
 # likely imminent arrival.
 func addOrphan*(
     quarantine: var Quarantine, finalizedSlot: Slot,
-    signedBlock: ForkedSignedBeaconBlock): bool =
+    signedBlock: ForkedSignedBeaconBlock): Result[void, cstring] =
   ## Adds block to quarantine's `orphans` and `missing` lists.
   if not isViable(finalizedSlot, getForkedBlockField(signedBlock, slot)):
     quarantine.addUnviable(signedBlock.root)
-    return false
+    return err("block unviable")
 
   quarantine.cleanupOrphans(finalizedSlot)
 
@@ -263,19 +263,19 @@ func addOrphan*(
 
   if parent_root in quarantine.unviable:
     quarantine.unviable[signedBlock.root] = ()
-    return true
+    return ok()
 
   # Even if the quarantine is full, we need to schedule its parent for
   # downloading or we'll never get to the bottom of things
   quarantine.addMissing(parent_root)
 
   if quarantine.orphans.lenu64 >= MaxOrphans:
-    return false
+    return err("block quarantine full")
 
   quarantine.orphans[(signedBlock.root, signedBlock.signature)] = signedBlock
   quarantine.missing.del(signedBlock.root)
 
-  true
+  ok()
 
 iterator pop*(quarantine: var Quarantine, root: Eth2Digest):
          ForkedSignedBeaconBlock =

beacon_chain/gossip_processing/block_processor.nim

@@ -520,12 +520,14 @@ proc storeBlock*(
 
         return err((VerifierError.UnviableFork, ProcessingStatus.completed))
 
-      if not self.consensusManager.quarantine[].addOrphan(
-          dag.finalizedHead.slot, ForkedSignedBeaconBlock.init(signedBlock)):
-        debug "Block quarantine full",
+      if (let r = self.consensusManager.quarantine[].addOrphan(
+          dag.finalizedHead.slot, ForkedSignedBeaconBlock.init(signedBlock));
+              r.isErr()):
+        debug "storeBlock: could not add orphan",
           blockRoot = shortLog(signedBlock.root),
           blck = shortLog(signedBlock.message),
-          signature = shortLog(signedBlock.signature)
+          signature = shortLog(signedBlock.signature),
+          err = r.error()
     of VerifierError.UnviableFork:
       # Track unviables so that descendants can be discarded properly
       self.consensusManager.quarantine[].addUnviable(signedBlock.root)

beacon_chain/gossip_processing/gossip_validation.nim

@@ -457,10 +457,13 @@ proc validateBeaconBlock*(
 
     # When the parent is missing, we can't validate the block - we'll queue it
     # in the quarantine for later processing
-    if not quarantine[].addOrphan(
+    if (let r = quarantine[].addOrphan(
         dag.finalizedHead.slot,
-        ForkedSignedBeaconBlock.init(signed_beacon_block)):
-      debug "Block quarantine full"
+        ForkedSignedBeaconBlock.init(signed_beacon_block)); r.isErr):
+      debug "validateBeaconBlock: could not add orphan",
+       blockRoot = shortLog(signed_beacon_block.root),
+       blck = shortLog(signed_beacon_block.message),
+       err = r.error()
 
     return errIgnore("BeaconBlock: Parent not found")
 

tests/test_block_quarantine.nim

@@ -44,13 +44,13 @@ suite "Block quarantine":
     check:
       FetchRecord(root: b1.root) in quarantine.checkMissing()
 
-      quarantine.addOrphan(Slot 0, b1)
+      quarantine.addOrphan(Slot 0, b1).isOk
 
       FetchRecord(root: b1.root) notin quarantine.checkMissing()
 
-      quarantine.addOrphan(Slot 0, b2)
-      quarantine.addOrphan(Slot 0, b3)
-      quarantine.addOrphan(Slot 0, b4)
+      quarantine.addOrphan(Slot 0, b2).isOk
+      quarantine.addOrphan(Slot 0, b3).isOk
+      quarantine.addOrphan(Slot 0, b4).isOk
 
       quarantine.addBlobless(Slot 0, b5)
       quarantine.addBlobless(Slot 0, b6)