CoreDb: remove PHK tries

why: There is no general use anymore for an MPT with a pre-hashed key. It was used to resemble the `SecureHexaryTrie` logic from the legacy DB. The only pace where this is needed is the `Leger` which uses a a distinct MPT version anyway (see `distinct_ledgers.nim`.)
status-im · Jun 19, 2024 · a841033 · a841033
1 parent e7be0d1
commit a841033
Show file tree

Hide file tree

Showing 7 changed files with 23 additions and 147 deletions.
diff --git a/nimbus/db/core_db/base.nim b/nimbus/db/core_db/base.nim
@@ -53,7 +53,6 @@ export
   CoreDxCaptRef,
   CoreDxKvtRef,
   CoreDxMptRef,
-  CoreDxPhkRef,
   CoreDxTxRef,
   PayloadRef
 
@@ -117,37 +116,6 @@ template ifTrackNewApi*(w: CoreDxApiTrackRef; code: untyped) =
     w.endNewApiIf:
       code
 
-# ---------
-
-func toCoreDxPhkRef(mpt: CoreDxMptRef): CoreDxPhkRef =
-  ## MPT => pre-hashed MPT (aka PHK)
-  result = CoreDxPhkRef(
-    toMpt: mpt,
-    methods: mpt.methods)
-
-  result.methods.fetchFn =
-    proc(k: openArray[byte]): CoreDbRc[Blob] =
-      mpt.methods.fetchFn(k.keccakHash.data)
-
-  result.methods.deleteFn =
-    proc(k: openArray[byte]): CoreDbRc[void] =
-      mpt.methods.deleteFn(k.keccakHash.data)
-
-  result.methods.mergeFn =
-    proc(k:openArray[byte]; v: openArray[byte]): CoreDbRc[void] =
-      mpt.methods.mergeFn(k.keccakHash.data, v)
-
-  result.methods.hasPathFn =
-    proc(k: openArray[byte]): CoreDbRc[bool] =
-      mpt.methods.hasPathFn(k.keccakHash.data)
-
-  when AutoValidateDescriptors:
-    result.validate
-
-
-func parent(phk: CoreDxPhkRef): CoreDbRef =
-  phk.toMpt.parent
-
 # ------------------------------------------------------------------------------
 # Public constructor helper
 # ------------------------------------------------------------------------------
@@ -176,7 +144,7 @@ proc bless*(db: CoreDbRef; kvt: CoreDxKvtRef): CoreDxKvtRef =
   kvt
 
 proc bless*[T: CoreDxKvtRef |
-               CoreDbCtxRef | CoreDxMptRef | CoreDxPhkRef | CoreDxAccRef |
+               CoreDbCtxRef | CoreDxMptRef | CoreDxAccRef |
                CoreDxTxRef  | CoreDxCaptRef |
                CoreDbKvtBackendRef | CoreDbMptBackendRef](
     db: CoreDbRef;
@@ -228,7 +196,7 @@ proc dbType*(db: CoreDbRef): CoreDbType =
 
 proc parent*[T: CoreDxKvtRef |
                 CoreDbColRef |
-                CoreDbCtxRef | CoreDxMptRef | CoreDxPhkRef | CoreDxAccRef |
+                CoreDbCtxRef | CoreDxMptRef | CoreDxAccRef |
                 CoreDxTxRef |
                 CoreDxCaptRef |
                 CoreDbErrorRef](
@@ -549,34 +517,14 @@ proc getAcc*(
   ##     ... # Was not the state root for the accounts column
   ##     return
   ##
-  ## This function works similar to `getMpt()` for handling accounts. Although
-  ## one can emulate this function by means of `getMpt(..).toPhk()`, it is
-  ## recommended using `CoreDxAccRef` methods for accounts.
+  ## This function works similar to `getMpt()` for handling accounts.
   ##
   ctx.setTrackNewApi CtxGetAccFn
   result = ctx.methods.getAccFn col
   ctx.ifTrackNewApi: debug newApiTxt, api, elapsed, col, result
 
-proc toMpt*(phk: CoreDxPhkRef): CoreDxMptRef =
-  ## Replaces the pre-hashed argument column `phk` by the non pre-hashed *MPT*.
-  ##
-  phk.setTrackNewApi PhkToMptFn
-  result = phk.toMpt
-  phk.ifTrackNewApi:
-    let col = result.methods.getColFn()
-    debug newApiTxt, api, elapsed, col
-
-proc toPhk*(mpt: CoreDxMptRef): CoreDxPhkRef =
-  ## Replaces argument `mpt` by a pre-hashed *MPT*.
-  ##
-  mpt.setTrackNewApi MptToPhkFn
-  result = mpt.toCoreDxPhkRef
-  mpt.ifTrackNewApi:
-    let col = result.methods.getColFn()
-    debug newApiTxt, api, elapsed, col
-
 # ------------------------------------------------------------------------------
-# Public common methods for all hexary trie databases (`mpt`, `phk`, or `acc`)
+# Public common methods for all hexary trie databases (`mpt`, or `acc`)
 # ------------------------------------------------------------------------------
 
 proc getColumn*(acc: CoreDxAccRef): CoreDbColRef =
@@ -593,15 +541,8 @@ proc getColumn*(mpt: CoreDxMptRef): CoreDbColRef =
   result = mpt.methods.getColFn()
   mpt.ifTrackNewApi: debug newApiTxt, api, elapsed, result
 
-proc getColumn*(phk: CoreDxPhkRef): CoreDbColRef =
-  ## Variant of `getColumn()`
-  ##
-  phk.setTrackNewApi PhkGetColFn
-  result = phk.methods.getColFn()
-  phk.ifTrackNewApi: debug newApiTxt, api, elapsed, result
-
 # ------------------------------------------------------------------------------
-# Public generic hexary trie database methods (`mpt` or `phk`)
+# Public generic hexary trie database methods
 # ------------------------------------------------------------------------------
 
 proc fetch*(mpt: CoreDxMptRef; key: openArray[byte]): CoreDbRc[Blob] =
@@ -614,15 +555,6 @@ proc fetch*(mpt: CoreDxMptRef; key: openArray[byte]): CoreDbRc[Blob] =
     let col = mpt.methods.getColFn()
     debug newApiTxt, api, elapsed, col, key=key.toStr, result
 
-proc fetch*(phk: CoreDxPhkRef; key: openArray[byte]): CoreDbRc[Blob] =
-  ## Variant of `fetch()"
-  phk.setTrackNewApi PhkFetchFn
-  result = phk.methods.fetchFn key
-  phk.ifTrackNewApi:
-    let col = phk.methods.getColFn()
-    debug newApiTxt, api, elapsed, col, key=key.toStr, result
-
-
 proc fetchOrEmpty*(mpt: CoreDxMptRef; key: openArray[byte]): CoreDbRc[Blob] =
   ## This function returns an empty `Blob` if the argument `key` is not found
   ## on the database.
@@ -635,32 +567,13 @@ proc fetchOrEmpty*(mpt: CoreDxMptRef; key: openArray[byte]): CoreDbRc[Blob] =
     let col = mpt.methods.getColFn()
     debug newApiTxt, api, elapsed, col, key=key.toStr, result
 
-proc fetchOrEmpty*(phk: CoreDxPhkRef; key: openArray[byte]): CoreDbRc[Blob] =
-  ## Variant of `fetchOrEmpty()`
-  phk.setTrackNewApi PhkFetchOrEmptyFn
-  result = phk.methods.fetchFn key
-  if result.isErr and result.error.error == MptNotFound:
-    result = CoreDbRc[Blob].ok(EmptyBlob)
-  phk.ifTrackNewApi:
-    let col = phk.methods.getColFn()
-    debug newApiTxt, api, elapsed, col, key=key.toStr, result
-
-
 proc delete*(mpt: CoreDxMptRef; key: openArray[byte]): CoreDbRc[void] =
   mpt.setTrackNewApi MptDeleteFn
   result = mpt.methods.deleteFn key
   mpt.ifTrackNewApi:
     let col = mpt.methods.getColFn()
     debug newApiTxt, api, elapsed, col, key=key.toStr, result
 
-proc delete*(phk: CoreDxPhkRef; key: openArray[byte]): CoreDbRc[void] =
-  phk.setTrackNewApi PhkDeleteFn
-  result = phk.methods.deleteFn key
-  phk.ifTrackNewApi:
-    let col = phk.methods.getColFn()
-    debug newApiTxt, api, elapsed, col, key=key.toStr, result
-
-
 proc merge*(
     mpt: CoreDxMptRef;
     key: openArray[byte];
@@ -672,18 +585,6 @@ proc merge*(
     let col = mpt.methods.getColFn()
     debug newApiTxt, api, elapsed, col, key=key.toStr, val=val.toLenStr, result
 
-proc merge*(
-    phk: CoreDxPhkRef;
-    key: openArray[byte];
-    val: openArray[byte];
-      ): CoreDbRc[void] =
-  phk.setTrackNewApi PhkMergeFn
-  result = phk.methods.mergeFn(key, val)
-  phk.ifTrackNewApi:
-    let col = phk.methods.getColFn()
-    debug newApiTxt, api, elapsed, col, key=key.toStr, val=val.toLenStr, result
-
-
 proc hasPath*(mpt: CoreDxMptRef; key: openArray[byte]): CoreDbRc[bool] =
   ## This function would be named `contains()` if it returned `bool` rather
   ## than a `Result[]`.
@@ -694,14 +595,6 @@ proc hasPath*(mpt: CoreDxMptRef; key: openArray[byte]): CoreDbRc[bool] =
     let col = mpt.methods.getColFn()
     debug newApiTxt, api, elapsed, col, key=key.toStr, result
 
-proc hasPath*(phk: CoreDxPhkRef; key: openArray[byte]): CoreDbRc[bool] =
-  ## Variant of `hasPath()`
-  phk.setTrackNewApi PhkHasPathFn
-  result = phk.methods.hasPathFn key
-  phk.ifTrackNewApi:
-    let col = phk.methods.getColFn()
-    debug newApiTxt, api, elapsed, col, key=key.toStr, result
-
 # ------------------------------------------------------------------------------
 # Public trie database methods for accounts
 # ------------------------------------------------------------------------------

diff --git a/nimbus/db/core_db/base/api_tracking.nim b/nimbus/db/core_db/base/api_tracking.nim
@@ -21,7 +21,7 @@ import
 type
   CoreDxApiTrackRef* =
     CoreDbRef | CoreDxKvtRef | CoreDbColRef |
-    CoreDbCtxRef | CoreDxMptRef | CoreDxPhkRef | CoreDxAccRef |
+    CoreDbCtxRef | CoreDxMptRef | CoreDxAccRef |
     CoreDxTxRef | CoreDxCaptRef | CoreDbErrorRef
 
   CoreDbFnInx* = enum
@@ -84,16 +84,6 @@ type
     MptMergeFn          = "mpt/merge"
     MptPairsIt          = "mpt/pairs"
     MptReplicateIt      = "mpt/replicate"
-    MptToPhkFn          = "mpt/toPhk"
-
-    PhkDeleteFn         = "phk/delete"
-    PhkFetchFn          = "phk/fetch"
-    PhkFetchOrEmptyFn   = "phk/fetchOrEmpty"
-    PhkForgetFn         = "phk/forget"
-    PhkGetColFn         = "phk/getColumn"
-    PhkHasPathFn        = "phk/hasPath"
-    PhkMergeFn          = "phk/merge"
-    PhkToMptFn          = "phk/toMpt"
 
     TxCommitFn          = "commit"
     TxDisposeFn         = "dispose"

diff --git a/nimbus/db/core_db/base/base_desc.nim b/nimbus/db/core_db/base/base_desc.nim
@@ -312,13 +312,6 @@ type
     parent*: CoreDbRef
     ready*: bool              ## Must be set `true` to enable
 
-  CoreDxPhkRef* = ref object
-    ## Similar to `CoreDbMptRef` but with pre-hashed keys. That is, any
-    ## argument key for `merge()`, `fetch()` etc. will be hashed first
-    ## before being applied.
-    toMpt*: CoreDxMptRef
-    methods*: CoreDbMptFns
-
   CoreDxTxRef* = ref object of RootRef
     ## Transaction descriptor derived from `CoreDbRef`
     parent*: CoreDbRef

diff --git a/nimbus/db/core_db/base/validate.nim b/nimbus/db/core_db/base/validate.nim
@@ -17,7 +17,7 @@ type
 
   MethodsDesc =
     CoreDxKvtRef |
-    CoreDbCtxRef | CoreDxMptRef | CoreDxPhkRef | CoreDxAccRef  |
+    CoreDbCtxRef | CoreDxMptRef | CoreDxAccRef  |
     CoreDxTxRef  |
     CoreDxCaptRef
 
@@ -108,11 +108,6 @@ proc validateMethodsDesc(acc: CoreDxAccRef) =
   doAssert not acc.parent.isNil
   acc.methods.validateMethodsDesc
 
-proc validateMethodsDesc(phk: CoreDxPhkRef) =
-  doAssert not phk.isNil
-  doAssert not phk.toMpt.isNil
-  phk.methods.validateMethodsDesc
-
 when false: # currently disabled
   proc validateMethodsDesc(cpt: CoreDxCaptRef) =
     doAssert not cpt.isNil

diff --git a/nimbus/db/core_db/base_iterators.nim b/nimbus/db/core_db/base_iterators.nim
@@ -47,7 +47,7 @@ iterator pairs*(kvt: CoreDxKvtRef): (Blob, Blob) {.apiRaise.} =
   kvt.ifTrackNewApi: debug newApiTxt, api, elapsed
 
 iterator pairs*(mpt: CoreDxMptRef): (Blob, Blob) =
-  ## Trie traversal, only supported for `CoreDxMptRef` (not `Phk`)
+  ## Trie traversal, only supported for `CoreDxMptRef`
   ##
   mpt.setTrackNewApi MptPairsIt
   case mpt.parent.dbType:
@@ -61,7 +61,7 @@ iterator pairs*(mpt: CoreDxMptRef): (Blob, Blob) =
     debug newApiTxt, api, elapsed, trie
 
 iterator replicate*(mpt: CoreDxMptRef): (Blob, Blob) {.apiRaise.} =
-  ## Low level trie dump, only supported for `CoreDxMptRef` (not `Phk`)
+  ## Low level trie dump, only supported for `CoreDxMptRef`
   ##
   mpt.setTrackNewApi MptReplicateIt
   case mpt.parent.dbType:

diff --git a/nimbus/db/ledger/distinct_ledgers.nim b/nimbus/db/ledger/distinct_ledgers.nim
@@ -33,7 +33,7 @@ import
 
 type
   AccountLedger* = distinct CoreDxAccRef
-  StorageLedger* = distinct CoreDxPhkRef
+  StorageLedger* = distinct CoreDxMptRef
   SomeLedger* = AccountLedger | StorageLedger
 
 const
@@ -56,7 +56,7 @@ proc toSvp*(sl: StorageLedger): seq[(UInt256,UInt256)] =
   let kvt = db.newKvt
   var kvp: Table[UInt256,UInt256]
   try:
-    for (slotHash,val) in sl.distinctBase.toMpt.pairs:
+    for (slotHash,val) in sl.distinctBase.pairs:
       let rc = kvt.get(slotHashToSlotKey(slotHash).toOpenArray)
       if rc.isErr:
         warn "StorageLedger.dump()", slotHash, error=($$rc.error)
@@ -186,22 +186,22 @@ proc init*(
       if rc.isErr:
         raiseAssert info & $$rc.error
       rc.value
-  mpt.toPhk.T
+  mpt.T
 
 proc fetch*(sl: StorageLedger, slot: UInt256): Result[Blob,void] =
-  var rc = sl.distinctBase.fetch(slot.toBytesBE)
+  var rc = sl.distinctBase.fetch(slot.toBytesBE.keccakHash.data)
   if rc.isErr:
     return err()
   ok move(rc.value)
 
 proc merge*(sl: StorageLedger, slot: UInt256, value: openArray[byte]) =
   const info = "StorageLedger/merge(): "
-  sl.distinctBase.merge(slot.toBytesBE, value).isOkOr:
+  sl.distinctBase.merge(slot.toBytesBE.keccakHash.data, value).isOkOr:
     raiseAssert info & $$error
 
 proc delete*(sl: StorageLedger, slot: UInt256) =
   const info = "StorageLedger/delete(): "
-  sl.distinctBase.delete(slot.toBytesBE).isOkOr:
+  sl.distinctBase.delete(slot.toBytesBE.keccakHash.data).isOkOr:
     if error.error == MptNotFound:
       return
     raiseAssert info & $$error

diff --git a/nimbus/db/trie_get_branch.nim b/nimbus/db/trie_get_branch.nim
@@ -8,8 +8,8 @@
 # at your option. This file may not be copied, modified, or distributed except
 # according to those terms.
 
-# This implementation of getBranch on the CoreDbPhkRef type is a temporary solution
-# which can be removed once we get an equivient proc defined on the CoreDbPhkRef type
+# This implementation of getBranch on the CoreDbMptRef type is a temporary solution
+# which can be removed once we get an equivient proc defined on the CoreDbMptRef type
 # in the db layer.
 
 {.push raises: [].}
@@ -88,7 +88,12 @@ proc getBranchAux(
     raise newException(RlpError, "node has an unexpected number of children")
 
 proc getBranch*(
-    self: CoreDxPhkRef;
+    # self: CoreDxPhkRef;
+    # Note that PHK type has been removed. The difference PHK an MPT was that
+    # the keys of the PHK were pre-hased (as in the legacy `SecureHexaryTrie`
+    # object.) Can this code have worked at all? Looking at the `keyHash`
+    # below it would mean the `key` was double hashed? -- j
+    self: CoreDxMptRef;
     key: openArray[byte]): seq[seq[byte]] {.raises: [RlpError].} =
   let
     keyHash = keccakHash(key).data