From 8727307ef4e4608f169f4aa2bfec6152a473151b Mon Sep 17 00:00:00 2001 From: Jordan Hrycaj Date: Tue, 18 Jun 2024 19:30:01 +0000 Subject: [PATCH] Aristo uses pre classified tree types cont1 (#2389) * Provide dedicated functions for deleteing accounts and storage trees why: Storage trees are always linked to an account, so there is no need for an application to fiddle about (e.g. re-cycling, unlinking) storage tree vertex IDs. * Remove `delete()` and other cruft from API, `aristo_delete`, etc. * clean up delete functions details: The delete implementations `deleteImpl()` and `delTreeImpl()` do not need to be super generic anymore as all the edge cases are covered by the specialised `deleteAccountPayload()`, `deleteGenericData()`, etc. * Avoid unnecessary re-calculations of account keys why: The function `registerAccountForUpdate()` did extract the storage ID (if any) and automatically marked the Merkle keys along the account path for re-hashing. This would also apply if there was later detected that the account or the storage tree did not need to be updated. So the `registerAccountForUpdate()` function was split into a part which retrieved the storage ID, and another one which marked the Merkle keys for re-calculation to be applied only when needed. --- nimbus/db/aristo/aristo_api.nim | 247 +++++++++------- nimbus/db/aristo/aristo_delete.nim | 269 ++++++++++-------- nimbus/db/aristo/aristo_desc/desc_error.nim | 5 + nimbus/db/aristo/aristo_merge.nim | 17 +- nimbus/db/aristo/aristo_utils.nim | 85 ++---- .../backend/aristo_db/handlers_aristo.nim | 48 ++-- nimbus/db/ledger/distinct_ledgers.nim | 5 +- tests/test_aristo/test_helpers.nim | 39 +-- tests/test_aristo/test_tx.nim | 8 +- 9 files changed, 358 insertions(+), 365 deletions(-) diff --git a/nimbus/db/aristo/aristo_api.nim b/nimbus/db/aristo/aristo_api.nim index fbda1acc7..b25155df0 100644 --- a/nimbus/db/aristo/aristo_api.nim +++ b/nimbus/db/aristo/aristo_api.nim @@ -49,41 +49,56 @@ type ## previous layer. The previous transaction is returned if there ## was any. - AristoApiDeleteFn* = + AristoApiDeleteAccountPayloadFn* = + proc(db: AristoDbRef; + path: openArray[byte]; + ): Result[void,AristoError] + {.noRaise.} + ## 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. + + AristoApiDeleteGenericDataFn* = proc(db: AristoDbRef; root: VertexID; path: openArray[byte]; - accPath: PathID; - ): Result[bool,(VertexID,AristoError)] + ): Result[bool,AristoError] {.noRaise.} - ## Delete a leaf with path `path` starting at root `root`. + ## Delete the leaf data entry addressed by the argument `path`. The MPT + ## sub-tree the leaf data entry is subsumed under is passed as argument + ## `root` which must be greater than `VertexID(1)` and smaller than + ## `LEAST_FREE_VID`. ## - ## For a `root` with `VertexID` greater than `LEAST_FREE_VID`, the - ## sub-tree generated by `payload.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 `root` vertex ID. - ## - ## The return code is `true` iff the trie has become empty. + ## The return value is `true` if the argument `path` deleted was the last + ## one and the tree does not exist anymore. - AristoApiDelTreeFn* = + AristoApiDeleteGenericTreeFn* = proc(db: AristoDbRef; root: VertexID; - accPath: PathID; - ): Result[void,(VertexID,AristoError)] + ): Result[void,AristoError] {.noRaise.} - ## 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. - ## - ## 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. + ## Variant of `deleteGenericData()` for purging the whole MPT sub-tree. + + AristoApiDeleteStorageDataFn* = + proc(db: AristoDbRef; + path: openArray[byte]; + accPath: PathID; + ): Result[bool,AristoError] + {.noRaise.} + ## For a given account argument `accPath`, this function deletes the + ## argument `path` from the associated storage tree (if any, at all.) If + ## the if the argument `path` deleted was the last one on the storage + ## tree, account leaf referred to by `accPath` will be updated so that + ## it will not refer to a storage tree anymore. In the latter case only + ## the function will return `true`. + + AristoApiDeleteStorageTreeFn* = + proc(db: AristoDbRef; + accPath: PathID; + ): Result[void,AristoError] + {.noRaise.} + ## Variant of `deleteStorageData()` for purging the whole storage tree + ## associated to the account argument `accPath`. AristoApiFetchLastSavedStateFn* = proc(db: AristoDbRef @@ -363,8 +378,11 @@ type AristoApiObj* = object of RootObj ## Useful set of `Aristo` fuctions that can be filtered, stacked etc. commit*: AristoApiCommitFn - delete*: AristoApiDeleteFn - delTree*: AristoApiDelTreeFn + deleteAccountPayload*: AristoApiDeleteAccountPayloadFn + deleteGenericData*: AristoApiDeleteGenericDataFn + deleteGenericTree*: AristoApiDeleteGenericTreeFn + deleteStorageData*: AristoApiDeleteStorageDataFn + deleteStorageTree*: AristoApiDeleteStorageTreeFn fetchLastSavedState*: AristoApiFetchLastSavedStateFn fetchPayload*: AristoApiFetchPayloadFn findTx*: AristoApiFindTxFn @@ -394,42 +412,45 @@ type ## Index/name mapping for profile slots AristoApiProfTotal = "total" - AristoApiProfCommitFn = "commit" - AristoApiProfDeleteFn = "delete" - AristoApiProfDelTreeFn = "delTree" - AristoApiProfFetchLastSavedStateFn = "fetchPayload" - AristoApiProfFetchPayloadFn = "fetchPayload" - AristoApiProfFindTxFn = "findTx" - AristoApiProfFinishFn = "finish" - AristoApiProfForgetFn = "forget" - AristoApiProfForkTxFn = "forkTx" - AristoApiProfGetKeyRcFn = "getKeyRc" - AristoApiProfHashifyFn = "hashify" - AristoApiProfHasPathFn = "hasPath" - AristoApiProfHikeUpFn = "hikeUp" - AristoApiProfIsTopFn = "isTop" - AristoApiProfLevelFn = "level" - AristoApiProfNForkedFn = "nForked" - AristoApiProfMergeAccountPayloadFn = "mergeAccountPayload" - AristoApiProfMergeGenericDataFn = "mergeGenericData" - AristoApiProfMergeStorageDataFn = "mergeStorageData" - AristoApiProfPathAsBlobFn = "pathAsBlob" - AristoApiProfPersistFn = "persist" - AristoApiProfReCentreFn = "reCentre" - AristoApiProfRollbackFn = "rollback" - AristoApiProfSerialiseFn = "serialise" - AristoApiProfTxBeginFn = "txBegin" - AristoApiProfTxTopFn = "txTop" + AristoApiProfCommitFn = "commit" + AristoApiProfDeleteAccountPayloadFn = "deleteAccountPayload" + AristoApiProfDeleteGenericDataFn = "deleteGnericData" + AristoApiProfDeleteGenericTreeFn = "deleteGnericTree" + AristoApiProfDeleteStorageDataFn = "deleteStorageData" + AristoApiProfDeleteStorageTreeFn = "deleteStorageTree" + AristoApiProfFetchLastSavedStateFn = "fetchPayload" + AristoApiProfFetchPayloadFn = "fetchPayload" + AristoApiProfFindTxFn = "findTx" + AristoApiProfFinishFn = "finish" + AristoApiProfForgetFn = "forget" + AristoApiProfForkTxFn = "forkTx" + AristoApiProfGetKeyRcFn = "getKeyRc" + AristoApiProfHashifyFn = "hashify" + AristoApiProfHasPathFn = "hasPath" + AristoApiProfHikeUpFn = "hikeUp" + AristoApiProfIsTopFn = "isTop" + AristoApiProfLevelFn = "level" + AristoApiProfNForkedFn = "nForked" + AristoApiProfMergeAccountPayloadFn = "mergeAccountPayload" + AristoApiProfMergeGenericDataFn = "mergeGenericData" + AristoApiProfMergeStorageDataFn = "mergeStorageData" + AristoApiProfPathAsBlobFn = "pathAsBlob" + AristoApiProfPersistFn = "persist" + AristoApiProfReCentreFn = "reCentre" + AristoApiProfRollbackFn = "rollback" + AristoApiProfSerialiseFn = "serialise" + AristoApiProfTxBeginFn = "txBegin" + AristoApiProfTxTopFn = "txTop" - AristoApiProfBeGetVtxFn = "be/getVtx" - AristoApiProfBeGetKeyFn = "be/getKey" - AristoApiProfBeGetTuvFn = "be/getTuv" - AristoApiProfBeGetLstFn = "be/getLst" - AristoApiProfBePutVtxFn = "be/putVtx" - AristoApiProfBePutKeyFn = "be/putKey" - AristoApiProfBePutTuvFn = "be/putTuv" - AristoApiProfBePutLstFn = "be/putLst" - AristoApiProfBePutEndFn = "be/putEnd" + AristoApiProfBeGetVtxFn = "be/getVtx" + AristoApiProfBeGetKeyFn = "be/getKey" + AristoApiProfBeGetTuvFn = "be/getTuv" + AristoApiProfBeGetLstFn = "be/getLst" + AristoApiProfBePutVtxFn = "be/putVtx" + AristoApiProfBePutKeyFn = "be/putKey" + AristoApiProfBePutTuvFn = "be/putTuv" + AristoApiProfBePutLstFn = "be/putLst" + AristoApiProfBePutEndFn = "be/putEnd" AristoApiProfRef* = ref object of AristoApiRef ## Profiling API extension of `AristoApiObj` @@ -443,8 +464,11 @@ type when AutoValidateApiHooks: proc validate(api: AristoApiObj|AristoApiRef) = doAssert not api.commit.isNil - doAssert not api.delete.isNil - doAssert not api.delTree.isNil + doAssert not api.deleteAccountPayload.isNil + doAssert not api.deleteGenericData.isNil + doAssert not api.deleteGenericTree.isNil + doAssert not api.deleteStorageData.isNil + doAssert not api.deleteStorageTree.isNil doAssert not api.fetchLastSavedState.isNil doAssert not api.fetchPayload.isNil doAssert not api.findTx.isNil @@ -495,8 +519,11 @@ func init*(api: var AristoApiObj) = when AutoValidateApiHooks: api.reset api.commit = commit - api.delete = delete - api.delTree = delTree + api.deleteAccountPayload = deleteAccountPayload + api.deleteGenericData = deleteGenericData + api.deleteGenericTree = deleteGenericTree + api.deleteStorageData = deleteStorageData + api.deleteStorageTree = deleteStorageTree api.fetchLastSavedState = fetchLastSavedState api.fetchPayload = fetchPayload api.findTx = findTx @@ -529,32 +556,35 @@ func init*(T: type AristoApiRef): T = func dup*(api: AristoApiRef): AristoApiRef = result = AristoApiRef( - commit: api.commit, - delete: api.delete, - delTree: api.delTree, - fetchLastSavedState: api.fetchLastSavedState, - fetchPayload: api.fetchPayload, - findTx: api.findTx, - finish: api.finish, - forget: api.forget, - forkTx: api.forkTx, - getKeyRc: api.getKeyRc, - hashify: api.hashify, - hasPath: api.hasPath, - hikeUp: api.hikeUp, - isTop: api.isTop, - level: api.level, - nForked: api.nForked, - mergeAccountPayload: api.mergeAccountPayload, - mergeGenericData: api.mergeGenericData, - mergeStorageData: api.mergeStorageData, - pathAsBlob: api.pathAsBlob, - persist: api.persist, - reCentre: api.reCentre, - rollback: api.rollback, - serialise: api.serialise, - txBegin: api.txBegin, - txTop: api.txTop) + commit: api.commit, + deleteAccountPayload: api.deleteAccountPayload, + deleteGenericData: api.deleteGenericData, + deleteGenericTree: api.deleteGenericTree, + deleteStorageData: api.deleteStorageData, + deleteStorageTree: api.deleteStorageTree, + fetchLastSavedState: api.fetchLastSavedState, + fetchPayload: api.fetchPayload, + findTx: api.findTx, + finish: api.finish, + forget: api.forget, + forkTx: api.forkTx, + getKeyRc: api.getKeyRc, + hashify: api.hashify, + hasPath: api.hasPath, + hikeUp: api.hikeUp, + isTop: api.isTop, + level: api.level, + nForked: api.nForked, + mergeAccountPayload: api.mergeAccountPayload, + mergeGenericData: api.mergeGenericData, + mergeStorageData: api.mergeStorageData, + pathAsBlob: api.pathAsBlob, + persist: api.persist, + reCentre: api.reCentre, + rollback: api.rollback, + serialise: api.serialise, + txBegin: api.txBegin, + txTop: api.txTop) when AutoValidateApiHooks: api.validate @@ -590,15 +620,30 @@ func init*( AristoApiProfCommitFn.profileRunner: result = api.commit(a) - profApi.delete = - proc(a: AristoDbRef; b: VertexID; c: openArray[byte]; d: PathID): auto = - AristoApiProfDeleteFn.profileRunner: - result = api.delete(a, b, c, d) + profApi.deleteAccountPayload = + proc(a: AristoDbRef; b: openArray[byte]): auto = + AristoApiProfDeleteAccountPayloadFn.profileRunner: + result = api.deleteAccountPayload(a, b) - profApi.delTree = - proc(a: AristoDbRef; b: VertexID; c: PathID): auto = - AristoApiProfDelTreeFn.profileRunner: - result = api.delTree(a, b, c) + profApi.deleteGenericData = + proc(a: AristoDbRef; b: VertexID; c: openArray[byte]): auto = + AristoApiProfDeleteGenericDataFn.profileRunner: + result = api.deleteGenericData(a, b, c) + + profApi.deleteGenericTree = + proc(a: AristoDbRef; b: VertexID): auto = + AristoApiProfDeleteGenericTreeFn.profileRunner: + result = api.deleteGenericTree(a, b) + + profApi.deleteStorageData = + proc(a: AristoDbRef; b: openArray[byte]; c: PathID): auto = + AristoApiProfDeleteStorageDataFn.profileRunner: + result = api.deleteStorageData(a, b, c) + + profApi.deleteStorageTree = + proc(a: AristoDbRef; b: PathID): auto = + AristoApiProfDeleteStorageTreeFn.profileRunner: + result = api.deleteStorageTree(a, b) profApi.fetchLastSavedState = proc(a: AristoDbRef): auto = diff --git a/nimbus/db/aristo/aristo_delete.nim b/nimbus/db/aristo/aristo_delete.nim index 651bf6a2a..6a629ea6c 100644 --- a/nimbus/db/aristo/aristo_delete.nim +++ b/nimbus/db/aristo/aristo_delete.nim @@ -253,28 +253,14 @@ proc collapseLeaf( proc delSubTreeImpl( db: AristoDbRef; # Database, top layer root: VertexID; # Root vertex - accPath: PathID; # Needed for real storage tries - ): Result[void,(VertexID,AristoError)] = + ): Result[void,AristoError] = ## Implementation of *delete* sub-trie. - let wp = block: - if root.distinctBase < LEAST_FREE_VID: - if not root.isValid: - return err((root,DelSubTreeVoidRoot)) - if root == VertexID(1): - return err((root,DelSubTreeAccRoot)) - VidVtxPair() - else: - let rc = db.registerAccount(root, accPath) - if rc.isErr: - return err((root,rc.error)) - else: - rc.value var dispose = @[root] rootVtx = db.getVtxRc(root).valueOr: if error == GetVtxNotFound: return ok() - return err((root,error)) + return err(error) follow = @[rootVtx] # Collect list of nodes to delete @@ -283,58 +269,31 @@ proc delSubTreeImpl( for vtx in follow: for vid in vtx.subVids: # Exiting here leaves the tree as-is - let vtx = ? db.getVtxRc(vid).mapErr toVae(vid) + let vtx = ? db.getVtxRc(vid) redo.add vtx dispose.add vid redo.swap follow - # Mark nodes deleted + # Mark collected vertices to be deleted for vid in dispose: db.disposeOfVtx(root, vid) - # Make sure that an account leaf has no dangling sub-trie - if wp.vid.isValid: - let leaf = wp.vtx.dup # Dup on modify - leaf.lData.account.storageID = VertexID(0) - db.layersPutVtx(VertexID(1), wp.vid, leaf) - db.layersResKey(VertexID(1), wp.vid) - ok() proc deleteImpl( db: AristoDbRef; # Database, top layer hike: Hike; # Fully expanded path - lty: LeafTie; # `Patricia Trie` path root-to-leaf - accPath: PathID; # Needed for accounts payload - ): Result[bool,(VertexID,AristoError)] = + ): Result[void,(VertexID,AristoError)] = ## Implementation of *delete* functionality. - let wp = block: - if lty.root.distinctBase < LEAST_FREE_VID: - VidVtxPair() - else: - let rc = db.registerAccount(lty.root, accPath) - if rc.isErr: - return err((lty.root,rc.error)) - else: - rc.value - - # Remove leaf entry on the top + # Remove leaf entry let lf = hike.legs[^1].wp if lf.vtx.vType != Leaf: return err((lf.vid,DelLeafExpexted)) if lf.vid in db.pPrf: return err((lf.vid, DelLeafLocked)) - # Verify that 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(hike.root, lf.vid) if 1 < hike.legs.len: @@ -350,7 +309,7 @@ proc deleteImpl( br.vtx.bVid[hike.legs[^2].nibble] = VertexID(0) db.layersPutVtx(hike.root, br.vid, br.vtx) - # Clear all keys up to the root key + # 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 if vid in db.top.final.pPrf: @@ -381,97 +340,155 @@ proc deleteImpl( of Leaf: ? db.collapseLeaf(hike, nibble.byte, nxt.vtx) - let emptySubTreeOk = not db.getVtx(hike.root).isValid - - # Make sure that an account leaf has no dangling sub-trie - if emptySubTreeOk and wp.vid.isValid: - let leaf = wp.vtx.dup # Dup on modify - leaf.lData.account.storageID = VertexID(0) - db.layersPutVtx(VertexID(1), wp.vid, leaf) - db.layersResKey(VertexID(1), wp.vid) - - ok(emptySubTreeOk) + ok() # ------------------------------------------------------------------------------ # Public functions # ------------------------------------------------------------------------------ -proc delTree*( - 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`. +proc deleteAccountPayload*( + db: AristoDbRef; + path: openArray[byte]; + ): Result[void,AristoError] = + ## 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. ## - ## Note that the accounts trie hinging on `VertexID(1)` cannot be deleted. - ## - ## If the `root` argument belongs to a well known sub trie (i.e. it does - ## not exceed `LEAST_FREE_VID`) the `accPath` argument is ignored and the - ## sub-trie will just be deleted. - ## - ## Otherwise, a valid `accPath` (i.e. different from `VOID_PATH_ID`.) is - ## required relating to an account leaf entry (starting at `VertexID(`)`). - ## If the payload of that leaf entry is not of type `AccountData` it is - ## ignored. Otherwise its `storageID` field must be equal to the `hike.root` - ## vertex ID. This leaf entry `storageID` field will be reset to - ## `VertexID(0)` after having deleted the sub-trie. - ## - db.delSubTreeImpl(root, accPath) + let + hike = path.initNibbleRange.hikeUp(VertexID(1), db).valueOr: + if error[1] in HikeAcceptableStopsNotFound: + return err(DelPathNotFound) + return err(error[1]) + stoID = hike.legs[^1].wp.vtx.lData.account.storageID -proc delete*( - db: AristoDbRef; # Database, top layer - hike: Hike; # Fully expanded chain of vertices - accPath: PathID; # Needed for accounts payload - ): Result[bool,(VertexID,AristoError)] = - ## Delete argument `hike` chain of vertices from the database. The return - ## code will be `true` iff the sub-trie starting at `hike.root` will have - ## become empty. - ## - ## If the `hike` argument referes to aa account entrie (i.e. `hike.root` - ## equals `VertexID(1)`) and the leaf entry has an `AccountData` payload, - ## its `storageID` field must have been reset to `VertexID(0)`. the - ## `accPath` argument will be ignored. - ## - ## Otherwise, if the `root` argument belongs to a well known sub trie (i.e. - ## it does not exceed `LEAST_FREE_VID`) the `accPath` argument is ignored - ## and the entry will just be deleted. - ## - ## Otherwise, a valid `accPath` (i.e. different from `VOID_PATH_ID`.) is - ## required relating to an account leaf entry (starting at `VertexID(`)`). - ## If the payload of that leaf entry is not of type `AccountData` it is - ## ignored. Otherwise its `storageID` field must be equal to the `hike.root` - ## vertex ID. This leaf entry `storageID` field will be reset to - ## `VertexID(0)` in case the entry to be deleted will render the sub-trie - ## empty. - ## - let lty = LeafTie( - root: hike.root, - path: ? hike.to(NibblesSeq).pathToTag().mapErr toVae) - db.deleteImpl(hike, lty, accPath) + # Delete storage tree if present + if stoID.isValid: + ? db.delSubTreeImpl stoID -proc delete*( - db: AristoDbRef; # Database, top layer - lty: LeafTie; # `Patricia Trie` path root-to-leaf - accPath: PathID; # Needed for accounts payload - ): Result[bool,(VertexID,AristoError)] = - ## Variant of `delete()` - ## - db.deleteImpl(? lty.hikeUp(db).mapErr toVae, lty, accPath) + db.deleteImpl(hike).isOkOr: + return err(error[1]) -proc delete*( + ok() + + +proc deleteGenericData*( db: AristoDbRef; root: VertexID; path: openArray[byte]; - accPath: PathID; # Needed for accounts payload - ): Result[bool,(VertexID,AristoError)] = - ## Variant of `delete()` + ): Result[bool,AristoError] = + ## Delete the leaf data entry addressed by the argument `path`. The MPT + ## sub-tree the leaf data entry is subsumed under is passed as argument + ## `root` which must be greater than `VertexID(1)` and smaller than + ## `LEAST_FREE_VID`. ## - let rc = path.initNibbleRange.hikeUp(root, db) - if rc.isOk: - return db.delete(rc.value, accPath) - if rc.error[1] in HikeAcceptableStopsNotFound: - return err((rc.error[0], DelPathNotFound)) - err((rc.error[0],rc.error[1])) + ## The return value is `true` if the argument `path` deleted was the last + ## one and the tree does not exist anymore. + ## + # Verify that `root` is neither an accounts tree nor a strorage tree. + if not root.isValid: + return err(DelRootVidMissing) + elif root == VertexID(1): + return err(DelAccRootNotAccepted) + elif LEAST_FREE_VID <= root.distinctBase: + return err(DelStoRootNotAccepted) + + let hike = path.initNibbleRange.hikeUp(root, db).valueOr: + if error[1] in HikeAcceptableStopsNotFound: + return err(DelPathNotFound) + return err(error[1]) + + db.deleteImpl(hike).isOkOr: + return err(error[1]) + + ok(not db.getVtx(root).isValid) + +proc deleteGenericTree*( + db: AristoDbRef; # Database, top layer + root: VertexID; # Root vertex + ): Result[void,AristoError] = + ## Variant of `deleteGenericData()` for purging the whole MPT sub-tree. + ## + # Verify that `root` is neither an accounts tree nor a strorage tree. + if not root.isValid: + return err(DelRootVidMissing) + elif root == VertexID(1): + return err(DelAccRootNotAccepted) + elif LEAST_FREE_VID <= root.distinctBase: + return err(DelStoRootNotAccepted) + + db.delSubTreeImpl root + + +proc deleteStorageData*( + db: AristoDbRef; + path: openArray[byte]; + accPath: PathID; # Needed for accounts payload + ): Result[bool,AristoError] = + ## For a given account argument `accPath`, this function deletes the + ## argument `path` from the associated storage tree (if any, at all.) If + ## the if the argument `path` deleted was the last one on the storage tree, + ## account leaf referred to by `accPath` will be updated so that it will + ## not refer to a storage tree anymore. In the latter case only the function + ## will return `true`. + ## + let + accHike = ? db.retrieveStoAccHike accPath + wpAcc = accHike.legs[^1].wp + stoID = wpAcc.vtx.lData.account.storageID + + if not stoID.isValid: + return err(DelStoRootMissing) + + let stoHike = path.initNibbleRange.hikeUp(stoID, db).valueOr: + if error[1] in HikeAcceptableStopsNotFound: + return err(DelPathNotFound) + return err(error[1]) + + # Mark account path for update for `hashify()` + db.updateAccountForHasher accHike + + db.deleteImpl(stoHike).isOkOr: + return err(error[1]) + + # Make sure that an account leaf has no dangling sub-trie + if db.getVtx(stoID).isValid: + return ok(false) + + # De-register the deleted storage tree from the account record + let leaf = wpAcc.vtx.dup # Dup on modify + leaf.lData.account.storageID = VertexID(0) + db.layersPutVtx(VertexID(1), wpAcc.vid, leaf) + db.layersResKey(VertexID(1), wpAcc.vid) + ok(true) + +proc deleteStorageTree*( + db: AristoDbRef; # Database, top layer + accPath: PathID; # Needed for accounts payload + ): Result[void,AristoError] = + ## Variant of `deleteStorageData()` for purging the whole storage tree + ## associated to the account argument `accPath`. + ## + let + accHike = db.retrieveStoAccHike(accPath).valueOr: + if error == UtilsAccInaccessible: + return err(DelStoAccMissing) + return err(error) + wpAcc = accHike.legs[^1].wp + stoID = wpAcc.vtx.lData.account.storageID + + if not stoID.isValid: + return err(DelStoRootMissing) + + # Mark account path for update for `hashify()` + db.updateAccountForHasher accHike + + ? db.delSubTreeImpl stoID + + # De-register the deleted storage tree from the accounts record + let leaf = wpAcc.vtx.dup # Dup on modify + leaf.lData.account.storageID = VertexID(0) + db.layersPutVtx(VertexID(1), wpAcc.vid, leaf) + db.layersResKey(VertexID(1), wpAcc.vid) + ok() # ------------------------------------------------------------------------------ # End diff --git a/nimbus/db/aristo/aristo_desc/desc_error.nim b/nimbus/db/aristo/aristo_desc/desc_error.nim index 4512d5295..ddefce590 100644 --- a/nimbus/db/aristo/aristo_desc/desc_error.nim +++ b/nimbus/db/aristo/aristo_desc/desc_error.nim @@ -190,6 +190,7 @@ type NearbyVidInvalid # Deletion of vertices, `delete()` + DelAccRootNotAccepted DelBranchExpexted DelBranchLocked DelBranchWithoutRefs @@ -200,6 +201,10 @@ type DelLeafUnexpected DelPathNotFound DelPathTagError + DelRootVidMissing + DelStoAccMissing + DelStoRootMissing + DelStoRootNotAccepted DelSubTreeAccRoot DelSubTreeVoidRoot DelVidStaleVtx diff --git a/nimbus/db/aristo/aristo_merge.nim b/nimbus/db/aristo/aristo_merge.nim index 5f72456ac..6d08726c4 100644 --- a/nimbus/db/aristo/aristo_merge.nim +++ b/nimbus/db/aristo/aristo_merge.nim @@ -53,6 +53,10 @@ proc mergeAccountPayload*( ## unset/invalid or referring to a existing vertex which will be assumed ## to be a storage tree. ## + ## On success, the function returns `true` if the `accPayload` argument was + ## merged into the database ot updated, and `false` if it was on the database + ## already. + ## let pyl = PayloadRef(pType: AccountData, account: accPayload) rc = db.mergePayloadImpl(VertexID(1), accKey, pyl, VidVtxPair()) @@ -73,6 +77,10 @@ proc mergeGenericData*( ## Variant of `mergeXXX()` for generic sub-trees, i.e. for arguments ## `root` greater than `VertexID(1)` and smaller than `LEAST_FREE_VID`. ## + ## On success, the function returns `true` if the `data` argument was merged + ## into the database ot updated, and `false` if it was on the database + ## already. + ## # Verify that `root` is neither an accounts tree nor a strorage tree. if not root.isValid: return err(MergeRootVidMissing) @@ -112,7 +120,8 @@ proc mergeStorageData*( ## otherwise `VertexID(0)`. ## let - wpAcc = ? db.registerAccountForUpdate accPath + accHike = ?db.retrieveStoAccHike accPath + wpAcc = accHike.legs[^1].wp stoID = wpAcc.vtx.lData.account.storageID # Provide new storage ID when needed @@ -123,6 +132,9 @@ proc mergeStorageData*( rc = db.mergePayloadImpl(useID, stoKey, pyl, wpAcc) if rc.isOk: + # Mark account path for update for `hashify()` + db.updateAccountForHasher accHike + if stoID.isValid: return ok VertexID(0) @@ -138,7 +150,8 @@ proc mergeStorageData*( assert stoID.isValid # debugging only return ok VertexID(0) - # else + # Error: mark account path for update for `hashify()` + db.updateAccountForHasher accHike err(rc.error) # ------------------------------------------------------------------------------ diff --git a/nimbus/db/aristo/aristo_utils.nim b/nimbus/db/aristo/aristo_utils.nim index 193da5ca9..db78edfe2 100644 --- a/nimbus/db/aristo/aristo_utils.nim +++ b/nimbus/db/aristo/aristo_utils.nim @@ -172,69 +172,16 @@ proc subVids*(vtx: VertexRef): seq[VertexID] = # --------------------- -proc registerAccount*( - db: AristoDbRef; # Database, top layer - stoRoot: VertexID; # Storage root ID - accPath: PathID; # Needed for accounts payload - ): Result[VidVtxPair,AristoError] = - ## Verify that the `stoRoot` argument is properly referred to by the - ## account data (if any) implied to by the `accPath` argument. +proc retrieveStoAccHike*( + db: AristoDbRef; # Database + accPath: PathID; # Implies a storage ID (if any) + ): Result[Hike,AristoError] = + ## Verify that the `accPath` argument properly referres to a storage root + ## vertex ID. The function will reset the keys along the `accPath` for + ## being modified. ## - ## The function will return an account leaf node if there was any, or an empty - ## `VidVtxPair()` object. - ## - # Verify storage root and account path - if not stoRoot.isValid: - return err(UtilsStoRootMissing) - if not accPath.isValid: - return err(UtilsAccPathMissing) - - # Get account leaf with account data - let hike = LeafTie(root: VertexID(1), path: accPath).hikeUp(db).valueOr: - return err(UtilsAccInaccessible) - - # Check account payload - let wp = hike.legs[^1].wp - if wp.vtx.vType != Leaf: - return err(UtilsAccPathWithoutLeaf) - if wp.vtx.lData.pType != AccountData: - return err(UtilsAccLeafPayloadExpected) - - # Check whether the `stoRoot` exists on the databse - let stoVtx = block: - let rc = db.getVtxRc stoRoot - if rc.isOk: - rc.value - elif rc.error == GetVtxNotFound: - VertexRef(nil) - else: - return err(rc.error) - - # Verify `stoVtx` against storage root - let stoID = wp.vtx.lData.account.storageID - if stoVtx.isValid: - if stoID != stoRoot: - return err(UtilsAccWrongStorageRoot) - else: - if stoID.isValid: - return err(UtilsAccWrongStorageRoot) - - # Clear Merkle keys so that `hasify()` can calculate the re-hash forest/tree - for w in hike.legs.mapIt(it.wp.vid): - db.layersResKey(hike.root, w) - - # Signal to `hashify()` where to start rebuilding Merkel hashes - db.top.final.dirty.incl hike.root - db.top.final.dirty.incl wp.vid - - ok(wp) - - -proc registerAccountForUpdate*( - db: AristoDbRef; # Database, top layer - accPath: PathID; # Needed for accounts payload - ): Result[VidVtxPair,AristoError] = - ## ... + ## On success, the function will return an account leaf pair with the leaf + ## vertex and the vertex ID. ## # Expand vertex path to account leaf let hike = (@accPath).initNibbleRange.hikeUp(VertexID(1), db).valueOr: @@ -253,15 +200,23 @@ proc registerAccountForUpdate*( discard db.getVtxRc(acc.storageID).valueOr: return err(UtilsStoRootInaccessible) + ok(hike) + +proc updateAccountForHasher*( + db: AristoDbRef; # Database + hike: Hike; # Return value from `retrieveStorageID()` + ) = + ## For a successful run of `retrieveStoAccHike()`, the argument `hike` is + ## used to mark/reset the keys along the `accPath` for being re-calculated + ## by `hashify()`. + ## # Clear Merkle keys so that `hasify()` can calculate the re-hash forest/tree for w in hike.legs.mapIt(it.wp.vid): db.layersResKey(hike.root, w) # Signal to `hashify()` where to start rebuilding Merkel hashes db.top.final.dirty.incl hike.root - db.top.final.dirty.incl wp.vid - - ok(wp) + db.top.final.dirty.incl hike.legs[^1].wp.vid # ------------------------------------------------------------------------------ # End diff --git a/nimbus/db/core_db/backend/aristo_db/handlers_aristo.nim b/nimbus/db/core_db/backend/aristo_db/handlers_aristo.nim index f9cd9bf93..08389d888 100644 --- a/nimbus/db/core_db/backend/aristo_db/handlers_aristo.nim +++ b/nimbus/db/core_db/backend/aristo_db/handlers_aristo.nim @@ -256,19 +256,25 @@ proc mptMethods(cMpt: AristoCoreDxMptRef): CoreDbMptFns = proc mptDelete(key: openArray[byte]): CoreDbRc[void] = const info = "deleteFn()" - if not cMpt.mptRoot.isValid and cMpt.accPath.isValid: - # This is insane but legit. A storage column was announced for an account - # but no data have been added, yet. - return ok() - let rc = api.delete(mpt, cMpt.mptRoot, key, cMpt.accPath) + let rc = block: + if cMpt.accPath.isValid: + api.deleteStorageData(mpt, key, cMpt.accPath) + else: + api.deleteGenericData(mpt, cMpt.mptRoot, key) + if rc.isErr: - if rc.error[1] == DelPathNotFound: + if rc.error == DelPathNotFound: return err(rc.error.toError(base, info, MptNotFound)) + if rc.error == DelStoRootMissing: + # This is insane but legit. A storage column was announced for an + # account but no data have been added, yet. + return ok() return err(rc.error.toError(base, info)) if rc.value: # Column has become empty cMpt.mptRoot = VoidVID + ok() proc mptHasPath(key: openArray[byte]): CoreDbRc[bool] = @@ -357,30 +363,21 @@ proc accMethods(cAcc: AristoCoreDxAccRef): CoreDbAccFns = proc accDelete(address: EthAddress): CoreDbRc[void] = const info = "acc/deleteFn()" - let - key = address.keccakHash.data - rc = api.delete(mpt, AccountsVID, key, VOID_PATH_ID) - if rc.isErr: - if rc.error[1] == DelPathNotFound: - return err(rc.error.toError(base, info, AccNotFound)) - return err(rc.error.toError(base, info)) + let key = address.keccakHash.data + api.deleteAccountPayload(mpt, key).isOkOr: + if error == DelPathNotFound: + return err(error.toError(base, info, AccNotFound)) + return err(error.toError(base, info)) + ok() proc accStoDelete(address: EthAddress): CoreDbRc[void] = const info = "stoDeleteFn()" - let - key = address.keccakHash.data - pyl = api.fetchPayload(mpt, AccountsVID, key).valueOr: - return ok() + let rc = api.deleteStorageTree(mpt, address.to(PathID)) + if rc.isErr and rc.error notin {DelStoRootMissing,DelStoAccMissing}: + return err(rc.error.toError(base, info)) - # Use storage ID from account and delete that column - if pyl.pType == AccountData: - let stoID = pyl.account.storageID - if stoID.isValid: - let rc = api.delTree(mpt, stoID, address.to(PathID)) - if rc.isErr: - return err(rc.error.toError(base, info)) ok() proc accHasPath(address: EthAddress): CoreDbRc[bool] = @@ -500,8 +497,9 @@ proc ctxMethods(cCtx: AristoCoreDbCtxRef): CoreDbCtxFns = # Reset column. This a emulates the behaviour of a new empty MPT on the # legacy database. if reset: - let rc = api.delTree(mpt, newMpt.mptRoot, VOID_PATH_ID) + let rc = api.deleteGenericTree(mpt, newMpt.mptRoot) if rc.isErr: + raiseAssert "find me" return err(rc.error.toError(base, info, AutoFlushFailed)) col.reset = false diff --git a/nimbus/db/ledger/distinct_ledgers.nim b/nimbus/db/ledger/distinct_ledgers.nim index 02098003d..4294f7d23 100644 --- a/nimbus/db/ledger/distinct_ledgers.nim +++ b/nimbus/db/ledger/distinct_ledgers.nim @@ -157,10 +157,7 @@ proc freeStorage*(al: AccountLedger, eAddr: EthAddress) = proc delete*(al: AccountLedger, eAddr: EthAddress) = const info = "AccountLedger/delete()" - # Flush associated storage trie - al.distinctBase.stoDelete(eAddr).isOkOr: - raiseAssert info & $$error - # Clear account + # Delete account and associated storage tree (if any) al.distinctBase.delete(eAddr).isOkOr: if error.error == MptNotFound: return diff --git a/tests/test_aristo/test_helpers.nim b/tests/test_aristo/test_helpers.nim index 9292e31e7..6fa457664 100644 --- a/tests/test_aristo/test_helpers.nim +++ b/tests/test_aristo/test_helpers.nim @@ -13,8 +13,7 @@ import eth/common, stew/endians2, ../../nimbus/db/aristo/[ - aristo_debug, aristo_desc, aristo_delete, - aristo_hashify, aristo_hike, aristo_merge], + aristo_debug, aristo_desc, aristo_hashify, aristo_hike, aristo_merge], ../../nimbus/db/kvstore_rocksdb, ../../nimbus/sync/protocol/snap/snap_types, ../replay/[pp, undump_accounts, undump_storages], @@ -211,42 +210,6 @@ proc hashify*( else: aristo_hashify.hashify(db) - -proc delete*( - db: AristoDbRef; - root: VertexID; - path: openArray[byte]; - accPath: PathID; - noisy: bool; - ): Result[bool,(VertexID,AristoError)] = - when declared(aristo_delete.noisy): - aristo_delete.exec(noisy, aristo_delete.delete(db, root, path, accPath)) - else: - aristo_delete.delete(db, root, path, accPath) - -proc delete*( - db: AristoDbRef; - lty: LeafTie; - accPath: PathID; - noisy: bool; - ): Result[bool,(VertexID,AristoError)] = - when declared(aristo_delete.noisy): - aristo_delete.exec(noisy, aristo_delete.delete(db, lty, accPath)) - else: - aristo_delete.delete(db, lty, accPath) - -proc delTree*( - db: AristoDbRef; - root: VertexID; - accPath: PathID; - noisy: bool; - ): Result[void,(VertexID,AristoError)] = - when declared(aristo_delete.noisy): - aristo_delete.exec(noisy, aristo_delete.delTree(db, root, accPath)) - else: - aristo_delete.delTree(db, root, accPath) - - proc mergeGenericData*( db: AristoDbRef; # Database, top layer leaf: LeafTiePayload; # Leaf item to add to the database diff --git a/tests/test_aristo/test_tx.nim b/tests/test_aristo/test_tx.nim index bb7e2cceb..883c7ab9f 100644 --- a/tests/test_aristo/test_tx.nim +++ b/tests/test_aristo/test_tx.nim @@ -394,8 +394,8 @@ proc testTxMergeAndDeleteOneByOne*( # Delete leaf block: - let rc = db.delete(leaf, VOID_PATH_ID) - xCheckRc rc.error == (0,0) + let rc = db.deleteGenericData(leaf.root, @(leaf.path)) + xCheckRc rc.error == 0 # Update list of remaininf leafs leafsLeft.excl leaf @@ -492,8 +492,8 @@ proc testTxMergeAndDeleteSubTree*( "" # Delete sub-tree block: - let rc = db.delTree(testRootVid, VOID_PATH_ID) - xCheckRc rc.error == (0,0): + let rc = db.deleteGenericTree testRootVid + xCheckRc rc.error == 0: noisy.say "***", "del(2)", " n=", n, "/", list.len, "\n db\n ", db.pp(backendOk=true),