leaf cache fixes (#2637)

* Add missing leaf cache update when a leaf turns to a branch with two
leaves (on merge) and vice versa (on delete) - this could lead to stale
leaves being returned from the cache causing validation failures - it
didn't happen because the leaf caches were not being used efficiently :)
* Replace `seq` with `ArrayBuf` in `Hike` allowing it to become
allocation-free - this PR also works around an inefficiency in nim in
returning large types via a `var` parameter
* Use the leaf cache instead of `getVtxRc` to fetch recent leaves - this
makes the vertex cache more efficient at caching branches because fewer
leaf requests pass through it.

nimbus/db/aristo/aristo_debug.nim

@@ -17,7 +17,7 @@ import
   stew/[byteutils, interval_set],
   ./aristo_desc/desc_backend,
   ./aristo_init/[memory_db, memory_only, rocks_db],
-  "."/[aristo_desc, aristo_get, aristo_hike, aristo_layers,
+  "."/[aristo_desc, aristo_get, aristo_layers,
        aristo_serialise, aristo_utils]
 
 # ------------------------------------------------------------------------------

nimbus/db/aristo/aristo_delete.nim

@@ -19,8 +19,7 @@ import
   eth/common,
   results,
   ./aristo_delete/[delete_helpers, delete_subtree],
-  "."/[aristo_desc, aristo_fetch, aristo_get, aristo_hike, aristo_layers,
-       aristo_utils]
+  "."/[aristo_desc, aristo_fetch, aristo_get, aristo_hike, aristo_layers]
 
 # ------------------------------------------------------------------------------
 # Private heplers
@@ -46,69 +45,80 @@ proc branchStillNeeded(vtx: VertexRef): Result[int,void] =
 proc deleteImpl(
     db: AristoDbRef;                   # Database, top layer
     hike: Hike;                        # Fully expanded path
-      ): Result[void,AristoError] =
-  ## Implementation of *delete* functionality.
+      ): Result[VertexRef,AristoError] =
+  ## Removes the last node in the hike and returns the updated leaf in case
+  ## a branch collapsed
 
   # Remove leaf entry
-  let lf =  hike.legs[^1].wp
+  let lf = hike.legs[^1].wp
   if lf.vtx.vType != Leaf:
     return err(DelLeafExpexted)
 
   db.disposeOfVtx((hike.root, lf.vid))
 
-  if 1 < hike.legs.len:
-    # Get current `Branch` vertex `br`
-    let br = block:
-      var wp = hike.legs[^2].wp
-      wp.vtx = wp.vtx.dup # make sure that layers are not impliciteley modified
-      wp
-    if br.vtx.vType != Branch:
-      return err(DelBranchExpexted)
-
-    # Unlink child vertex from structural table
-    br.vtx.bVid[hike.legs[^2].nibble] = VertexID(0)
-    db.layersPutVtx((hike.root, br.vid), br.vtx)
-
-    # Clear all Merkle hash keys up to the root key
-    for n in 0 .. hike.legs.len - 2:
-      let vid = hike.legs[n].wp.vid
-      db.layersResKey((hike.root, vid))
-
-    let nbl = block:
-      let rc = br.vtx.branchStillNeeded()
-      if rc.isErr:
-        return err(DelBranchWithoutRefs)
-      rc.value
-
-    if 0 <= nbl:
-      # Branch has only one entry - convert it to a leaf or join with parent
-
-      # Get child vertex (there must be one after a `Branch` node)
-      let
-        vid = br.vtx.bVid[nbl]
-        nxt = db.getVtx (hike.root, vid)
-      if not nxt.isValid:
-        return err(DelVidStaleVtx)
-
-      db.disposeOfVtx((hike.root, vid))
-
-      let vtx =
-        case nxt.vType
-        of Leaf:
-          VertexRef(
-            vType: Leaf,
-            pfx:  br.vtx.pfx & NibblesBuf.nibble(nbl.byte) & nxt.pfx,
-            lData: nxt.lData)
-        of Branch:
-          VertexRef(
-            vType: Branch,
-            pfx:  br.vtx.pfx & NibblesBuf.nibble(nbl.byte) & nxt.pfx,
-            bVid: nxt.bVid)
-
-      # Put the new vertex at the id of the obsolete branch
-      db.layersPutVtx((hike.root, br.vid), vtx)
-
-  ok()
+  if hike.legs.len == 1:
+    # This was the last node in the trie, meaning we don't have any branches or
+    # leaves to update
+    return ok(nil)
+
+  # Get current `Branch` vertex `br`
+  let br = block:
+    var wp = hike.legs[^2].wp
+    wp.vtx = wp.vtx.dup # make sure that layers are not impliciteley modified
+    wp
+  if br.vtx.vType != Branch:
+    return err(DelBranchExpexted)
+
+  # Unlink child vertex from structural table
+  br.vtx.bVid[hike.legs[^2].nibble] = VertexID(0)
+  db.layersPutVtx((hike.root, br.vid), br.vtx)
+
+  # Clear all Merkle hash keys up to the root key
+  for n in 0 .. hike.legs.len - 2:
+    let vid = hike.legs[n].wp.vid
+    db.layersResKey((hike.root, vid))
+
+  let nbl = block:
+    let rc = br.vtx.branchStillNeeded()
+    if rc.isErr:
+      return err(DelBranchWithoutRefs)
+    rc.value
+
+  if 0 <= nbl:
+    # Branch has only one entry - convert it to a leaf or join with parent
+
+    # Get child vertex (there must be one after a `Branch` node)
+    let
+      vid = br.vtx.bVid[nbl]
+      nxt = db.getVtx (hike.root, vid)
+    if not nxt.isValid:
+      return err(DelVidStaleVtx)
+
+    db.disposeOfVtx((hike.root, vid))
+
+    let vtx =
+      case nxt.vType
+      of Leaf:
+        VertexRef(
+          vType: Leaf,
+          pfx:  br.vtx.pfx & NibblesBuf.nibble(nbl.byte) & nxt.pfx,
+          lData: nxt.lData)
+
+      of Branch:
+        VertexRef(
+          vType: Branch,
+          pfx:  br.vtx.pfx & NibblesBuf.nibble(nbl.byte) & nxt.pfx,
+          bVid: nxt.bVid)
+
+    # Put the new vertex at the id of the obsolete branch
+    db.layersPutVtx((hike.root, br.vid), vtx)
+
+    if vtx.vType == Leaf:
+      ok(vtx)
+    else:
+      ok(nil)
+  else:
+    ok(nil)
 
 # ------------------------------------------------------------------------------
 # Public functions
@@ -121,23 +131,28 @@ proc deleteAccountRecord*(
   ## Delete the account leaf entry addressed by the argument `path`. If this
   ## leaf entry referres to a storage tree, this one will be deleted as well.
   ##
+  var accHike: Hike
+  db.fetchAccountHike(accPath, accHike).isOkOr:
+    if error == FetchAccInaccessible:
+      return err(DelPathNotFound)
+    return err(error)
   let
-    hike = accPath.hikeUp(VertexID(1), db).valueOr:
-      if error[1] in HikeAcceptableStopsNotFound:
-        return err(DelPathNotFound)
-      return err(error[1])
-    stoID = hike.legs[^1].wp.vtx.lData.stoID
+    stoID = accHike.legs[^1].wp.vtx.lData.stoID
 
   # Delete storage tree if present
   if stoID.isValid:
     ? db.delStoTreeImpl((stoID.vid, stoID.vid), accPath)
 
-  ?db.deleteImpl(hike)
+  let otherLeaf = ?db.deleteImpl(accHike)
 
   db.layersPutAccLeaf(accPath, nil)
 
-  ok()
+  if otherLeaf.isValid:
+    db.layersPutAccLeaf(
+      Hash256(data: getBytes(NibblesBuf.fromBytes(accPath.data).replaceSuffix(otherLeaf.pfx))),
+      otherLeaf)
 
+  ok()
 
 proc deleteGenericData*(
     db: AristoDbRef;
@@ -160,12 +175,13 @@ proc deleteGenericData*(
   elif LEAST_FREE_VID <= root.distinctBase:
     return err(DelStoRootNotAccepted)
 
-  let hike = path.hikeUp(root, db).valueOr:
+  var hike: Hike
+  path.hikeUp(root, db, Opt.none(VertexRef), hike).isOkOr:
     if error[1] in HikeAcceptableStopsNotFound:
       return err(DelPathNotFound)
     return err(error[1])
 
-  ?db.deleteImpl(hike)
+  discard ?db.deleteImpl(hike)
 
   ok(not db.getVtx((root, root)).isValid)
 
@@ -185,7 +201,6 @@ proc deleteGenericTree*(
 
   db.delSubTreeImpl root
 
-
 proc deleteStorageData*(
     db: AristoDbRef;
     accPath: Hash256;          # Implies storage data tree
@@ -198,31 +213,48 @@ proc deleteStorageData*(
   ## not refer to a storage tree anymore. In the latter case only the function
   ## will return `true`.
   ##
+
+  let
+    mixPath = mixUp(accPath, stoPath)
+    stoLeaf = db.cachedStoLeaf(mixPath)
+
+  if stoLeaf == Opt.some(nil):
+    return err(DelPathNotFound)
+
+  var accHike: Hike
+  db.fetchAccountHike(accPath, accHike).isOkOr:
+    if error == FetchAccInaccessible:
+      return err(DelStoAccMissing)
+    return err(error)
+
   let
-    accHike = db.fetchAccountHike(accPath).valueOr:
-      if error == FetchAccInaccessible:
-        return err(DelStoAccMissing)
-      return err(error)
     wpAcc = accHike.legs[^1].wp
     stoID = wpAcc.vtx.lData.stoID
 
   if not stoID.isValid:
     return err(DelStoRootMissing)
 
-  let stoHike = stoPath.hikeUp(stoID.vid, db).valueOr:
+  let stoNibbles = NibblesBuf.fromBytes(stoPath.data)
+  var stoHike: Hike
+  stoNibbles.hikeUp(stoID.vid, db, stoLeaf, stoHike).isOkOr:
     if error[1] in HikeAcceptableStopsNotFound:
       return err(DelPathNotFound)
     return err(error[1])
 
   # Mark account path Merkle keys for update
-  db.updateAccountForHasher accHike
+  db.layersResKeys accHike
 
-  ?db.deleteImpl(stoHike)
+  let otherLeaf = ?db.deleteImpl(stoHike)
+  db.layersPutStoLeaf(mixPath, nil)
 
-  db.layersPutStoLeaf(mixUp(accPath, stoPath), nil)
+  if otherLeaf.isValid:
+    let leafMixPath = mixUp(
+      accPath,
+      Hash256(data: getBytes(stoNibbles.replaceSuffix(otherLeaf.pfx))))
+    db.layersPutStoLeaf(leafMixPath, otherLeaf)
 
-  # Make sure that an account leaf has no dangling sub-trie
-  if db.getVtx((stoID.vid, stoID.vid)).isValid:
+  # If there was only one item (that got deleted), update the account as well
+  if stoHike.legs.len > 1:
     return ok(false)
 
   # De-register the deleted storage tree from the account record
@@ -239,19 +271,21 @@ proc deleteStorageTree*(
   ## Variant of `deleteStorageData()` for purging the whole storage tree
   ## associated to the account argument `accPath`.
   ##
+  var accHike: Hike
+  db.fetchAccountHike(accPath, accHike).isOkOr:
+    if error == FetchAccInaccessible:
+      return err(DelStoAccMissing)
+    return err(error)
+
   let
-    accHike = db.fetchAccountHike(accPath).valueOr:
-      if error == FetchAccInaccessible:
-        return err(DelStoAccMissing)
-      return err(error)
     wpAcc = accHike.legs[^1].wp
     stoID = wpAcc.vtx.lData.stoID
 
   if not stoID.isValid:
     return err(DelStoRootMissing)
 
   # Mark account path Merkle keys for update
-  db.updateAccountForHasher accHike
+  db.layersResKeys accHike
 
   ? db.delStoTreeImpl((stoID.vid, stoID.vid), accPath)
 

nimbus/db/aristo/aristo_desc.nim

@@ -87,6 +87,17 @@ type
     # Debugging data below, might go away in future
     xMap*: Table[HashKey,RootedVertexID] ## For pretty printing/debugging
 
+  Leg* = object
+    ## For constructing a `VertexPath`
+    wp*: VidVtxPair                ## Vertex ID and data ref
+    nibble*: int8                  ## Next vertex selector for `Branch` (if any)
+
+  Hike* = object
+    ## Trie traversal path
+    root*: VertexID                ## Handy for some fringe cases
+    legs*: ArrayBuf[NibblesBuf.high + 1, Leg] ## Chain of vertices and IDs
+    tail*: NibblesBuf              ## Portion of non completed path
+
 # ------------------------------------------------------------------------------
 # Public helpers
 # ------------------------------------------------------------------------------

nimbus/db/aristo/aristo_desc/desc_error.nim

@@ -127,8 +127,7 @@ type
     MergeHikeFailed # Ooops, internal error
     MergeAccRootNotAccepted
     MergeStoRootNotAccepted
-    MergeLeafPathCachedAlready
-    MergeLeafPathOnBackendAlready
+    MergeNoAction
     MergeRootVidMissing
     MergeStoAccMissing
 

nimbus/db/aristo/aristo_desc/desc_nibbles.nim

@@ -77,6 +77,13 @@ func slice*(r: NibblesBuf, ibegin: int, iend = -1): NibblesBuf {.noinit.} =
   doAssert ibegin >= 0 and e <= result.bytes.len * 2
   result.iend = e.int8
 
+func replaceSuffix*(r: NibblesBuf, suffix: NibblesBuf): NibblesBuf =
+  for i in 0..<r.len - suffix.len:
+    result[i] = r[i]
+  for i in 0..<suffix.len:
+    result[i + r.len - suffix.len] = suffix[i]
+  result.iend = min(64, r.len + suffix.len).int8
+
 template writeFirstByte(nibbleCountExpr) {.dirty.} =
   let nibbleCount = nibbleCountExpr
   var oddnessFlag = (nibbleCount and 1) != 0