Remove vertex ID recycle function

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)`.
status-im · Aug 12, 2024 · ec6b095 · ec6b095
1 parent 8723a79
commit ec6b095
Show file tree

Hide file tree

Showing 4 changed files with 16 additions and 11 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
 # ------------------------------------------------------------------------------