From c4a5bca629c8ceaaca8b45ad5a6d62cc30971c88 Mon Sep 17 00:00:00 2001
From: Etan Kissling <etan@status.im>
Date: Sun, 24 Mar 2024 06:03:51 +0100
Subject: [PATCH] update block quarantine eviction order to FIFO (#6129)

Use the same eviction policy for blocks as already the case for blobs.
FIFO makes more sense, because it favors keeping ancestors of blocks
which need to be applied to the DAG before their children get eligible.
---
 .../blob_quarantine.nim                       |  2 +-
 .../block_quarantine.nim                      | 36 ++++++++++++-------
 2 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/beacon_chain/consensus_object_pools/blob_quarantine.nim b/beacon_chain/consensus_object_pools/blob_quarantine.nim
index f4e5623bf..346b9d239 100644
--- a/beacon_chain/consensus_object_pools/blob_quarantine.nim
+++ b/beacon_chain/consensus_object_pools/blob_quarantine.nim
@@ -38,7 +38,7 @@ func shortLog*(x: seq[BlobFetchRecord]): string =
   "[" & x.mapIt(shortLog(it.block_root) & shortLog(it.indices)).join(", ") & "]"
 
 func put*(quarantine: var BlobQuarantine, blobSidecar: ref BlobSidecar) =
-  if quarantine.blobs.lenu64 > MaxBlobs:
+  if quarantine.blobs.lenu64 >= MaxBlobs:
     # 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.
diff --git a/beacon_chain/consensus_object_pools/block_quarantine.nim b/beacon_chain/consensus_object_pools/block_quarantine.nim
index 6f04a8307..13c725109 100644
--- a/beacon_chain/consensus_object_pools/block_quarantine.nim
+++ b/beacon_chain/consensus_object_pools/block_quarantine.nim
@@ -43,7 +43,7 @@ type
     ##
     ## Trivially invalid blocks may be dropped before reaching this stage.
 
-    orphans*: Table[(Eth2Digest, ValidatorSig), ForkedSignedBeaconBlock]
+    orphans*: OrderedTable[(Eth2Digest, ValidatorSig), ForkedSignedBeaconBlock]
       ## Blocks that we don't have a parent for - when we resolve the
       ## parent, we can proceed to resolving the block as well - we
       ## index this by root and signature such that a block with
@@ -52,7 +52,7 @@ type
       ## below) - if so, upon resolving the parent, it should be
       ## added to the blobless table, after verifying its signature.
 
-    blobless*: Table[Eth2Digest, deneb.SignedBeaconBlock]
+    blobless*: OrderedTable[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
@@ -146,11 +146,11 @@ func cleanupUnviable(quarantine: var Quarantine) =
       break # Cannot modify while for-looping
     quarantine.unviable.del(toDel)
 
-func removeUnviableOrphanTree(quarantine: var Quarantine,
-                        toCheck: var seq[Eth2Digest],
-                        tbl: var Table[(Eth2Digest, ValidatorSig),
-                                       ForkedSignedBeaconBlock]):
-                                         seq[Eth2Digest] =
+func removeUnviableOrphanTree(
+    quarantine: var Quarantine,
+    toCheck: var seq[Eth2Digest],
+    tbl: var OrderedTable[(Eth2Digest, ValidatorSig), 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
@@ -178,10 +178,10 @@ func removeUnviableOrphanTree(quarantine: var Quarantine,
 
   checked
 
-func removeUnviableBloblessTree(quarantine: var Quarantine,
-                                toCheck: var seq[Eth2Digest],
-                                tbl: var Table[Eth2Digest,
-                                               deneb.SignedBeaconBlock]) =
+func removeUnviableBloblessTree(
+    quarantine: var Quarantine,
+    toCheck: var seq[Eth2Digest],
+    tbl: var OrderedTable[Eth2Digest, deneb.SignedBeaconBlock]) =
   var
     toRemove: seq[Eth2Digest] # Can't modify while iterating
   while toCheck.len > 0:
@@ -271,7 +271,13 @@ func addOrphan*(
   quarantine.addMissing(parent_root)
 
   if quarantine.orphans.lenu64 >= MaxOrphans:
-    return err("block quarantine full")
+    # Evict based on FIFO
+    var oldest_orphan_key: (Eth2Digest, ValidatorSig)
+    for k in quarantine.orphans.keys:
+      oldest_orphan_key = k
+      break
+    quarantine.orphans.del oldest_orphan_key
+    quarantine.blobless.del oldest_orphan_key[0]
 
   quarantine.orphans[(signedBlock.root, signedBlock.signature)] = signedBlock
   quarantine.missing.del(signedBlock.root)
@@ -303,7 +309,11 @@ proc addBlobless*(
   quarantine.cleanupBlobless(finalizedSlot)
 
   if quarantine.blobless.lenu64 >= MaxBlobless:
-    return true
+    var oldest_blobless_key: Eth2Digest
+    for k in quarantine.blobless.keys:
+      oldest_blobless_key = k
+      break
+    quarantine.blobless.del oldest_blobless_key
 
   debug "block quarantine: Adding blobless", blck = shortLog(signedBlock)
   quarantine.blobless[signedBlock.root] = signedBlock