Improve quarantine performance on forked chains (#7006)

* Improve quarantine performance on forked chains

A bit silly, but if we downloaded an orphan, we'd keep re-downloading it
if the missing list was full.

Also, we'd keep redownloading unviables :/

* lint

Author: arnetheduck

AllTests-mainnet.md

@@ -124,6 +124,8 @@ AllTests-mainnet
 ```
 ## Block quarantine
 ```diff
++ Don't re-download unviable blocks                                                          OK
++ Keep downloading parent chain even if we hit missing limit                                 OK
 + Recursive missing parent                                                                   OK
 + Unviable smoke test                                                                        OK
 ```

beacon_chain/consensus_object_pools/block_quarantine.nim

@@ -1,5 +1,5 @@
 # beacon_chain
-# Copyright (c) 2018-2024 Status Research & Development GmbH
+# Copyright (c) 2018-2025 Status Research & Development GmbH
 # Licensed and distributed under either of
 #   * MIT license (license terms in the root directory or at https://opensource.org/licenses/MIT).
 #   * Apache v2 license (license terms in the root directory or at https://www.apache.org/licenses/LICENSE-2.0).
@@ -18,7 +18,7 @@ export tables, forks
 const
   MaxRetriesPerMissingItem = 7
     ## Exponential backoff, double interval between each attempt
-  MaxMissingItems = 1024
+  MaxMissingItems* = 1024
     ## Arbitrary
   MaxOrphans = SLOTS_PER_EPOCH * 3
     ## Enough for finalization in an alternative fork
@@ -215,6 +215,9 @@ func removeUnviableBloblessTree(
     toRemove.setLen(0)
 
 func addUnviable*(quarantine: var Quarantine, root: Eth2Digest) =
+  # Unviable - don't try to download again!
+  quarantine.missing.del(root)
+
   if root in quarantine.unviable:
     return
 
@@ -281,18 +284,24 @@ func addOrphan*(
     quarantine: var Quarantine, finalizedSlot: Slot,
     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)
+    quarantine.addUnviable(signedBlock.root) # will remove from missing
     return err("block unviable")
 
   quarantine.cleanupOrphans(finalizedSlot)
 
   let parent_root = getForkedBlockField(signedBlock, parent_root)
 
   if parent_root in quarantine.unviable:
-    quarantine.unviable[signedBlock.root] = ()
+    quarantine.addUnviable(signedBlock.root)
     return err("block parent unviable")
 
+  # It's no longer missing if we downloaded it - remove before adding to make
+  # sure parent chains get downloaded even if missing list is full (works as
+  # long as the orphan was in the missing list, which is likely)
+  quarantine.missing.del(signedBlock.root)
+
   # 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)
@@ -307,7 +316,6 @@ func addOrphan*(
     quarantine.blobless.del oldest_orphan_key[0]
 
   quarantine.orphans[(signedBlock.root, signedBlock.signature)] = signedBlock
-  quarantine.missing.del(signedBlock.root)
 
   ok()
 

tests/test_block_quarantine.nim

@@ -1,5 +1,5 @@
 # beacon_chain
-# Copyright (c) 2022-2024 Status Research & Development GmbH
+# Copyright (c) 2022-2025 Status Research & Development GmbH
 # Licensed and distributed under either of
 #   * MIT license (license terms in the root directory or at https://opensource.org/licenses/MIT).
 #   * Apache v2 license (license terms in the root directory or at https://www.apache.org/licenses/LICENSE-2.0).
@@ -10,6 +10,7 @@
 
 import
   unittest2,
+  chronicles,
   ../beacon_chain/spec/forks,
   ../beacon_chain/spec/datatypes/[phase0, deneb],
   ../beacon_chain/consensus_object_pools/block_quarantine
@@ -118,3 +119,39 @@ suite "Block quarantine":
       b0.root in quarantine.missing
       b1.root notin quarantine.missing
       b2.root notin quarantine.missing
+
+  test "Keep downloading parent chain even if we hit missing limit":
+    var quarantine: Quarantine
+    var blocks = @[makeBlock(Slot 0, ZERO_HASH)]
+    for i in 0..<MaxMissingItems:
+      blocks.add makeBlock(blocks[^1].slot + 1, blocks[^1].root)
+
+    # Fill missing list with junk
+    for i in 0..<MaxMissingItems:
+      quarantine.addMissing(blocks[^(i + 1)].root)
+
+    check:
+      blocks[0].root notin quarantine.missing
+      quarantine.addOrphan(Slot 0, blocks[1]) == Result[void, cstring].ok()
+      blocks[0].root in quarantine.missing
+
+  test "Don't re-download unviable blocks":
+    var quarantine: Quarantine
+    let
+      b0 = makeBlock(Slot 0, ZERO_HASH)
+      b1 = makeBlock(Slot 1, b0.root)
+      b2 = makeBlock(Slot 2, b1.root)
+
+    quarantine.addMissing(b1.root)
+    quarantine.addMissing(b2.root)
+
+    check:
+      b2.root in quarantine.missing
+
+    quarantine.addUnviable(b1.root)
+    check:
+      b1.root notin quarantine.missing
+
+    check:
+      quarantine.addOrphan(Slot 0, b2).isErr()
+      b2.root notin quarantine.missing