Core db+aristo update storage trie handling (#2023)

* CoreDb: Test module with additional sample selector cmd line options

* Aristo: Do not automatically remove a storage trie with the account

why:
  This is an unnecessary side effect. Rather than using an automatism, a
  a storage root must be deleted manually.

* Aristo: Can handle stale storage root vertex IDs as empty IDs.

why:
  This is currently needed for the ledger API supporting both, a legacy
  and the `Aristo` database backend.

  This feature can be disabled at compile time by re-setting the
  `LOOSE_STORAGE_TRIE_COUPLING` flag in the `aristo_constants` module.

* CoreDb+Aristo: Flush/delete storage trie when deleting account

why:
  On either backend, a deleted account leave a dangling storage trie on
  the database.

  For consistency nn the legacy backend, storage tries must not be
  deleted as they might be shared by several accounts whereas on `Aristo`
  they are always unique.

nimbus/db/aristo/aristo_constants.nim

@@ -59,6 +59,17 @@ const
  ## functions with fixed assignments of the type of a state root (e.g. for
  ## a receipt or a transaction root.)
 
+ LOOSE_STORAGE_TRIE_COUPLING* = true
+ ## Enabling the `LOOSE_STORAGE_TRIE_COUPLING` flag a sub-trie is considered
+ ## empty if the root vertex ID is zero or at least `LEAST_FREE_VID` and
+ ## there is no vertex available. If the vertex ID is not zero and should
+ ## be considered as such will affect calculating the Merkel hash node key
+ ## for an accou.t leaf of payload type `AccountData`.
+ ##
+ ## Setting this flag `true` might be helpful for running an API supporting
+ ## both, a legacy and# the `Aristo` database backend.
+ ##
+
 static:
  doAssert 1 < LEAST_FREE_VID # must stay away from `VertexID(1)`
 

nimbus/db/aristo/aristo_delete.nim

@@ -298,10 +298,16 @@ proc collapseLeaf(
 proc delSubTree(
  db: AristoDbRef; # Database, top layer
  root: VertexID; # Root vertex
+ accPath: PathID; # Needed for real storage tries
  ): Result[void,(VertexID,AristoError)] =
  ## Implementation of *delete* sub-trie.
  if not root.isValid:
  return err((root,DelSubTreeVoidRoot))
+
+ if LEAST_FREE_VID <= root.distinctBase:
+ db.registerAccount(root, accPath).isOkOr:
+ return err((root,error))
+
  var
  dispose = @[root]
  rootVtx = db.getVtxRc(root).valueOr:
@@ -333,7 +339,7 @@ proc deleteImpl(
  hike: Hike; # Fully expanded path
  lty: LeafTie; # `Patricia Trie` path root-to-leaf
  accPath: PathID; # Needed for accounts payload
- ): Result[void,(VertexID,AristoError)] =
+ ): Result[bool,(VertexID,AristoError)] =
  ## Implementation of *delete* functionality.
 
  if LEAST_FREE_VID <= lty.root.distinctBase:
@@ -347,15 +353,13 @@ proc deleteImpl(
  if lf.vid in db.pPrf:
  return err((lf.vid, DelLeafLocked))
 
- # Will be needed at the end. Just detect an error early enouhh
- let leafVidBe = block:
- let rc = db.getVtxBE lf.vid
- if rc.isErr:
- if rc.error != GetVtxNotFound:
- return err((lf.vid, rc.error))
- VertexRef(nil)
- else:
- rc.value
+ # Verify thet there is no dangling storage trie
+ block:
+ let data = lf.vtx.lData
+ if data.pType == AccountData:
+ let vid = data.account.storageID
+ if vid.isValid and db.getVtx(vid).isValid:
+ return err((vid,DelDanglingStoTrie))
 
  db.disposeOfVtx lf.vid
 
@@ -407,14 +411,7 @@ proc deleteImpl(
  # at a later state.
  db.top.final.lTab[lty] = VertexID(0)
 
- # Delete dependent leaf node storage tree if there is any
- let data = lf.vtx.lData
- if data.pType == AccountData:
- let vid = data.account.storageID
- if vid.isValid:
- return db.delSubTree vid
-
- ok()
+ ok(not db.getVtx(hike.root).isValid)
 
 # ------------------------------------------------------------------------------
 # Public functions
@@ -423,22 +420,25 @@ proc deleteImpl(
 proc delete*(
  db: AristoDbRef; # Database, top layer
  root: VertexID; # Root vertex
+ accPath: PathID; # Needed for real storage tries
  ): Result[void,(VertexID,AristoError)] =
  ## Delete sub-trie below `root`. The maximum supported sub-tree size is
  ## `SUB_TREE_DISPOSAL_MAX`. Larger tries must be disposed by walk-deleting
  ## leaf nodes using `left()` or `right()` traversal functions.
  ##
- ## Caveat:
- ## There is no way to quickly verify that the `root` argument is isolated.
- ## Deleting random sub-trees might lead to an inconsistent database.
+ ## For a `root` argument greater than `LEAST_FREE_VID`, the sub-tree spanned
+ ## by `root` is considered a storage trie linked to an account leaf referred
+ ## to by a valid `accPath` (i.e. different from `VOID_PATH_ID`.) In that
+ ## case, an account must exists. If there is payload of type `AccountData`,
+ ## its `storageID` field must be unset or equal to the `hike.root` vertex ID.
  ##
- db.delSubTree root
+ db.delSubTree(root, accPath)
 
 proc delete*(
  db: AristoDbRef; # Database, top layer
  hike: Hike; # Fully expanded chain of vertices
  accPath: PathID; # Needed for accounts payload
- ): Result[void,(VertexID,AristoError)] =
+ ): Result[bool,(VertexID,AristoError)] =
  ## Delete argument `hike` chain of vertices from the database.
  ##
  ## For a `hike.root` with `VertexID` greater than `LEAST_FREE_VID`, the
@@ -448,9 +448,7 @@ proc delete*(
  ## of type `AccountData`, its `storageID` field must be unset or equal to the
  ## `hike.root` vertex ID.
  ##
- ## Note:
- ## If the leaf node has an account payload referring to a storage sub-trie,
- ## this one will be deleted as well.
+ ## The return code is `true` iff the trie has become empty.
  ##
  # Need path in order to remove it from `lTab[]`
  let lty = LeafTie(
@@ -462,7 +460,7 @@ proc delete*(
  db: AristoDbRef; # Database, top layer
  lty: LeafTie; # `Patricia Trie` path root-to-leaf
  accPath: PathID; # Needed for accounts payload
- ): Result[void,(VertexID,AristoError)] =
+ ): Result[bool,(VertexID,AristoError)] =
  ## Variant of `delete()`
  ##
  db.deleteImpl(? lty.hikeUp(db).mapErr toVae, lty, accPath)
@@ -472,7 +470,7 @@ proc delete*(
  root: VertexID;
  path: openArray[byte];
  accPath: PathID; # Needed for accounts payload
- ): Result[void,(VertexID,AristoError)] =
+ ): Result[bool,(VertexID,AristoError)] =
  ## Variant of `delete()`
  ##
  let rc = path.initNibbleRange.hikeUp(root, db)

nimbus/db/aristo/aristo_desc/desc_error.nim

@@ -186,18 +186,19 @@ type
  NearbyVidInvalid
 
  # Deletion of vertices, `delete()`
- DelPathTagError
- DelLeafExpexted
- DelLeafLocked
- DelLeafUnexpected
  DelBranchExpexted
  DelBranchLocked
  DelBranchWithoutRefs
+ DelDanglingStoTrie
  DelExtLocked
- DelVidStaleVtx
+ DelLeafExpexted
+ DelLeafLocked
+ DelLeafUnexpected
+ DelPathNotFound
+ DelPathTagError
  DelSubTreeTooBig
  DelSubTreeVoidRoot
- DelPathNotFound
+ DelVidStaleVtx
 
  # Functions from `aristo_filter.nim`
  FilBackendMissing

nimbus/db/aristo/aristo_hashify.nim

@@ -366,9 +366,7 @@ proc hashify*(
  if not vtx.isValid:
  # This might happen when proof nodes (see `snap` protocol) are on
  # an incomplete trie where this `vid` has a key but no vertex yet.
- # Also, the key (as part of the proof data) must be on the backend
- # by the way `leafToRootCrawler()` works. So it is enough to verify
- # the key there.
+ # Also, the key (as part of the proof data) must be on the backend.
  discard db.getKeyBE(vid).valueOr:
  return err((vid,HashifyNodeUnresolved))
  else:

nimbus/db/aristo/aristo_utils.nim

@@ -14,10 +14,10 @@
 {.push raises: [].}
 
 import
- std/[sequtils, tables],
+ std/[sequtils, tables, typetraits],
  eth/common,
  results,
- "."/[aristo_desc, aristo_get, aristo_hike, aristo_layers]
+ "."/[aristo_constants, aristo_desc, aristo_get, aristo_hike, aristo_layers]
 
 # ------------------------------------------------------------------------------
 # Public functions, converters
@@ -128,10 +128,16 @@ proc toNode*(
  let vid = vtx.lData.account.storageID
  if vid.isValid:
  let key = db.getKey vid
- if key.isValid:
- node.key[0] = key
- else:
- return err(@[vid])
+ if not key.isValid:
+ block looseCoupling:
+ when LOOSE_STORAGE_TRIE_COUPLING:
+ # Stale storage trie?
+ if LEAST_FREE_VID <= vid.distinctBase and
+ not db.getVtx(vid).isValid:
+ node.lData.account.storageID = VertexID(0)
+ break looseCoupling
+ # Otherwise this is a stale storage trie.
+ return err(@[vid])
  node.key[0] = key
  return ok node
 

nimbus/db/core_db/backend/aristo_db/handlers_aristo.nim

@@ -447,6 +447,11 @@ proc mptMethods(cMpt: AristoChildDbRef): CoreDbMptFns =
  if rc.error[1] == DelPathNotFound:
  return err(rc.error.toError(db, info, MptNotFound))
  return err(rc.error.toError(db, info))
+
+ if rc.value:
+ # Trie has become empty
+ cMpt.root = VoidTrieID
+
  ok()
 
  proc mptHasPath(
@@ -575,6 +580,27 @@ proc accMethods(cAcc: AristoChildDbRef): CoreDbAccFns =
  return err(rc.error.toError(db, info))
  ok()
 
+ proc accStoFlush(
+ cAcc: AristoChildDbRef;
+ address: EthAddress;
+ info: static[string];
+ ): CoreDbRc[void] =
+ let
+ db = cAcc.base.parent
+ mpt = cAcc.mpt
+ key = address.keccakHash.data
+ pyl = mpt.fetchPayload(cAcc.root, key).valueOr:
+ return ok()
+
+ # Use storage ID from account and delete that sub-trie
+ if pyl.pType == AccountData:
+ let stoID = pyl.account.storageID
+ if stoID.isValid:
+ let rc = mpt.delete(stoID, address.to(PathID))
+ if rc.isErr:
+ return err(rc.error.toError(db, info))
+ ok()
+
  proc accHasPath(
  cAcc: AristoChildDbRef;
  address: EthAddress;
@@ -602,6 +628,9 @@ proc accMethods(cAcc: AristoChildDbRef): CoreDbAccFns =
  deleteFn: proc(address: EthAddress): CoreDbRc[void] =
  cAcc.accDelete(address, "deleteFn()"),
 
+ stoFlushFn: proc(address: EthAddress): CoreDbRc[void] =
+ cAcc.accStoFlush(address, "stoFlushFn()"),
+
  mergeFn: proc(acc: CoreDbAccount): CoreDbRc[void] =
  cAcc.accMerge(acc, "mergeFn()"),
 
@@ -869,7 +898,11 @@ proc newMptHandler*(
  if rc.isErr:
  return err(rc.error[1].toError(db, info, AccNotFound))
  if trie.reset:
- let rc = trie.ctx.mpt.delete(trie.root)
+ # Note that `reset` only applies to non-dynamic trie roots with vertex ID
+ # beween `VertexID(2) ..< LEAST_FREE_VID`. At the moment, this applies to
+ # `GenericTrie` type sub-tries somehow emulating the behaviour of a new
+ # empty MPT on the legacy database (handle with care, though.)
+ let rc = trie.ctx.mpt.delete(trie.root, VOID_PATH_ID)
  if rc.isErr:
  return err(rc.error.toError(db, info, AutoFlushFailed))
  trie.reset = false

nimbus/db/core_db/backend/legacy_db.nim

@@ -327,6 +327,9 @@ proc accMethods(mpt: HexaryChildDbRef; db: LegacyDbRef): CoreDbAccFns =
  mpt.trie.del(k.keccakHash.data)
  ok(),
 
+ stoFlushFn: proc(k: EthAddress): CoreDbRc[void] =
+ ok(),
+
  mergeFn: proc(v: CoreDbAccount): CoreDbRc[void] =
  db.mapRlpException("mergeFn()"):
  mpt.trie.put(v.address.keccakHash.data, rlp.encode v.toAccount)

nimbus/db/core_db/base.nim

@@ -911,6 +911,21 @@ proc delete*(acc: CoreDxAccRef; address: EthAddress): CoreDbRc[void] =
  result = acc.methods.deleteFn address
  acc.ifTrackNewApi: debug newApiTxt, ctx, elapsed, address, result
 
+proc stoFlush*(acc: CoreDxAccRef; address: EthAddress): CoreDbRc[void] =
+ ## Recursively delete all data elements from the storage trie associated to
+ ## the account identified by the argument `address`. After successful run,
+ ## the storage trie will be empty.
+ ##
+ ## caveat:
+ ## This function has currently no effect on the legacy backend so it must
+ ## not be relied upon in general. On the legacy backend, storage tries
+ ## might be shared by several accounts whereas they are unique on the
+ ## `Aristo` backend.
+ ##
+ acc.setTrackNewApi AccStoFlushFn
+ result = acc.methods.stoFlushFn address
+ acc.ifTrackNewApi: debug newApiTxt, ctx, elapsed, address, result
+
 proc merge*(
  acc: CoreDxAccRef;
  account: CoreDbAccount;

nimbus/db/core_db/base/api_tracking.nim

@@ -34,6 +34,7 @@ type
  AccMergeFn = "acc/merge"
  AccNewMptFn = "acc/newMpt"
  AccPersistentFn = "acc/persistent"
+ AccStoFlushFn = "acc/stoFlush"
  AccToMptFn = "acc/toMpt"
 
  AnyBackendFn = "any/backend"

nimbus/db/core_db/base/base_desc.nim

@@ -188,14 +188,14 @@ type
  persistentFn*: CoreDbMptPersistentFn
  forgetFn*: CoreDbMptForgetFn
 
-
  # ----------------------------------------------------
  # Sub-descriptor: Mpt/hexary trie methods for accounts
  # ------------------------------------------------------
  CoreDbAccBackendFn* = proc(): CoreDbAccBackendRef {.noRaise.}
  CoreDbAccNewMptFn* = proc(): CoreDbRc[CoreDxMptRef] {.noRaise.}
  CoreDbAccFetchFn* = proc(k: EthAddress): CoreDbRc[CoreDbAccount] {.noRaise.}
  CoreDbAccDeleteFn* = proc(k: EthAddress): CoreDbRc[void] {.noRaise.}
+ CoreDbAccStoFlushFn* = proc(k: EthAddress): CoreDbRc[void] {.noRaise.}
  CoreDbAccMergeFn* = proc(v: CoreDbAccount): CoreDbRc[void] {.noRaise.}
  CoreDbAccHasPathFn* = proc(k: EthAddress): CoreDbRc[bool] {.noRaise.}
  CoreDbAccGetTrieFn* = proc(): CoreDbTrieRef {.noRaise.}
@@ -209,6 +209,7 @@ type
  newMptFn*: CoreDbAccNewMptFn
  fetchFn*: CoreDbAccFetchFn
  deleteFn*: CoreDbAccDeleteFn
+ stoFlushFn*: CoreDbAccStoFlushFn
  mergeFn*: CoreDbAccMergeFn
  hasPathFn*: CoreDbAccHasPathFn
  getTrieFn*: CoreDbAccGetTrieFn

nimbus/db/core_db/base/validate.nim

@@ -72,6 +72,7 @@ proc validateMethodsDesc(fns: CoreDbAccFns) =
  doAssert not fns.newMptFn.isNil
  doAssert not fns.fetchFn.isNil
  doAssert not fns.deleteFn.isNil
+ doAssert not fns.stoFlushFn.isNil
  doAssert not fns.mergeFn.isNil
  doAssert not fns.hasPathFn.isNil
  doAssert not fns.getTrieFn.isNil

nimbus/db/ledger/distinct_ledgers.nim

@@ -114,6 +114,10 @@ proc merge*(al: AccountLedger; account: CoreDbAccount) =
 
 proc delete*(al: AccountLedger, eAddr: EthAddress) =
  const info = "AccountLedger/delete()"
+ # Flush associated storage trie
+ al.distinctBase.stoFlush(eAddr).isOkOr:
+ raiseAssert info & $$error
+ # Clear account
  al.distinctBase.delete(eAddr).isOkOr:
  if error.error == MptNotFound:
  return

tests/test_aristo/test_tx.nim

@@ -476,7 +476,7 @@ proc testTxMergeAndDeleteSubTree*(
  ""
  # Delete sub-tree
  block:
- let rc = db.delete VertexID(1)
+ let rc = db.delete(VertexID(1), VOID_PATH_ID)
  xCheckRc rc.error == (0,0):
  noisy.say "***", "del(2)",
  " n=", n, "/", list.len,