From 7becf4e3890b4cfb6012e202d76f18a343c73eea Mon Sep 17 00:00:00 2001
From: Jordan Hrycaj <jordan@status.im>
Date: Mon, 12 Aug 2024 20:56:15 +0000
Subject: [PATCH] Remove vertex ID recycle function (#2558)

why:
  It is not safe in general to recycle vertex IDs while the `RocksDb`
  cache has `VertexID` rather than `RootedVertexID` where the former
  type seems preferable.

  In some fringe cases one might remove a vertex with key `(root1,vid)`
  and insert another vertex with key `(root2,vid)` while re-using the
  vertex ID `vid`. Without knowledge of `root1` and `root2`, the LRU
  cache will return the same vertex for `(root2,vid)` also for
  `(root1,vid)`.
---
 nimbus/db/aristo/aristo_delete.nim                 |  3 +--
 nimbus/db/aristo/aristo_init/rocks_db/rdb_desc.nim | 14 ++++++++++++++
 nimbus/db/aristo/aristo_part/part_ctx.nim          |  4 +---
 nimbus/db/aristo/aristo_vid.nim                    |  6 ------
 4 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/nimbus/db/aristo/aristo_delete.nim b/nimbus/db/aristo/aristo_delete.nim
index df04aa865..c2ad2a17c 100644
--- a/nimbus/db/aristo/aristo_delete.nim
+++ b/nimbus/db/aristo/aristo_delete.nim
@@ -20,7 +20,7 @@ import
   eth/common,
   results,
   "."/[aristo_desc, aristo_fetch, aristo_get, aristo_hike, aristo_layers,
-       aristo_utils, aristo_vid]
+       aristo_utils]
 
 # ------------------------------------------------------------------------------
 # Private heplers
@@ -48,7 +48,6 @@ proc disposeOfVtx(
   # Remove entry
   db.layersResVtx(rvid)
   db.layersResKey(rvid)
-  db.vidDispose rvid.vid               # Recycle ID
 
 # ------------------------------------------------------------------------------
 # Private functions
diff --git a/nimbus/db/aristo/aristo_init/rocks_db/rdb_desc.nim b/nimbus/db/aristo/aristo_init/rocks_db/rdb_desc.nim
index fa516e1fa..caf791426 100644
--- a/nimbus/db/aristo/aristo_init/rocks_db/rdb_desc.nim
+++ b/nimbus/db/aristo/aristo_init/rocks_db/rdb_desc.nim
@@ -38,6 +38,20 @@ type
     vtxCol*: ColFamilyReadWrite        ## Vertex column family handler
     keyCol*: ColFamilyReadWrite        ## Hash key column family handler
     session*: WriteBatchRef            ## For batched `put()`
+
+    # Note that the key type `VertexID` for LRU caches requires that there is
+    # strictly no vertex ID re-use.
+    #
+    # Otherwise, in some fringe cases one might remove a vertex with key
+    # `(root1,vid)` and insert another vertex with key `(root2,vid)` while
+    # re-using the vertex ID `vid`. Without knowledge of `root1` and `root2`,
+    # the LRU cache will return the same vertex for `(root2,vid)` also for
+    # `(root1,vid)`.
+    #
+    # The other alternaive would be to use the key type `RootedVertexID` which
+    # is less memory and time efficient (the latter one due to internal LRU
+    # handling of the longer key.)
+    #
     rdKeyLru*: KeyedQueue[VertexID,HashKey] ## Read cache
     rdVtxLru*: KeyedQueue[VertexID,VertexRef] ## Read cache
 
diff --git a/nimbus/db/aristo/aristo_part/part_ctx.nim b/nimbus/db/aristo/aristo_part/part_ctx.nim
index 23142ef10..46f7df6b3 100644
--- a/nimbus/db/aristo/aristo_part/part_ctx.nim
+++ b/nimbus/db/aristo/aristo_part/part_ctx.nim
@@ -14,8 +14,7 @@ import
   std/sets,
   eth/common,
   results,
-  ".."/[aristo_desc, aristo_get, aristo_hike, aristo_layers, aristo_utils,
-        aristo_vid],
+  ".."/[aristo_desc, aristo_get, aristo_hike, aristo_layers, aristo_utils],
   #./part_debug,
   ./part_desc
 
@@ -112,7 +111,6 @@ proc ctxAcceptChange(psc: PartStateCtx): Result[bool,AristoError] =
     let key = ps.move(psc.fromVid, toVid)
     doAssert key.isValid
     ps.changed.incl key
-    db.vidDispose psc.fromVid # typically no action
 
     # Remove parent vertex it it has become complete.
     ps.removeCompletedNodes(psc.location)
diff --git a/nimbus/db/aristo/aristo_vid.nim b/nimbus/db/aristo/aristo_vid.nim
index ec4604260..820c7341d 100644
--- a/nimbus/db/aristo/aristo_vid.nim
+++ b/nimbus/db/aristo/aristo_vid.nim
@@ -14,7 +14,6 @@
 {.push raises: [].}
 
 import
-  std/typetraits,
   ./aristo_desc
 
 # ------------------------------------------------------------------------------
@@ -30,11 +29,6 @@ proc vidFetch*(db: AristoDbRef): VertexID =
     db.top.vTop.inc
   db.top.vTop
 
-proc vidDispose*(db: AristoDbRef; vid: VertexID) =
-  ## Only top vertexIDs are disposed
-  if vid == db.top.vTop and LEAST_FREE_VID < db.top.vTop.distinctBase:
-    db.top.vTop.dec
-
 # ------------------------------------------------------------------------------
 # End
 # ------------------------------------------------------------------------------