Speed up account ledger a little (#2279)

`persist` is a hotspot when processing blocks because it is run at least
once per transaction and loops over the entire account cache every time.

Here, we introduce an extra `dirty` map that keeps track of all accounts
that need checking during `persist` which fixes the immediate
inefficiency, though probably this could benefit from a more thorough
review - we also get rid of the unused clearCache flag - we start with
a fresh cache on every fresh vmState.

* avoid unnecessary code hash comparisons
* avoid unnecessary copies when iterating
* use EMPTY_CODE_HASH throughout for code hash comparison

hive_integration/nodocker/engine/node.nim

@@ -74,7 +74,7 @@ proc processBlock(
 
   vmState.mutateStateDB:
     let clearEmptyAccount = vmState.determineFork >= FkSpurious
-    db.persist(clearEmptyAccount, ClearCache in vmState.flags)
+    db.persist(clearEmptyAccount)
 
   # `applyDeletes = false`
   # If the trie pruning activated, each of the block will have its own state

nimbus/core/executor/process_block.nim

@@ -117,8 +117,8 @@ proc procBlkEpilogue(vmState: BaseVMState;
   vmState.mutateStateDB:
     if vmState.generateWitness:
       db.collectWitnessData()
-    let clearEmptyAccount = vmState.determineFork >= FkSpurious
-    db.persist(clearEmptyAccount, ClearCache in vmState.flags)
+
+    db.persist(clearEmptyAccount = vmState.determineFork >= FkSpurious)
 
   let stateDb = vmState.stateDB
   if header.stateRoot != stateDb.rootHash:

nimbus/core/executor/process_transaction.nim

@@ -118,9 +118,7 @@ proc processTransactionImpl(
 
   if vmState.generateWitness:
     vmState.stateDB.collectWitnessData()
-  vmState.stateDB.persist(
-    clearEmptyAccount = fork >= FkSpurious,
-    clearCache = false)
+  vmState.stateDB.persist(clearEmptyAccount = fork >= FkSpurious)
 
   return res
 
@@ -157,7 +155,7 @@ proc processBeaconBlockRoot*(vmState: BaseVMState, beaconRoot: Hash256):
   if res.isError:
     return err("processBeaconBlockRoot: " & res.error)
 
-  statedb.persist(clearEmptyAccount = true, clearCache = false)
+  statedb.persist(clearEmptyAccount = true)
   ok()
 
 proc processTransaction*(

nimbus/core/tx_pool/tx_tasks/tx_packer.nim

@@ -71,9 +71,7 @@ proc persist(pst: TxPackerStateRef)
   ## Smart wrapper
   if not pst.cleanState:
     let fork = pst.xp.chain.nextFork
-    pst.xp.chain.vmState.stateDB.persist(
-      clearEmptyAccount = fork >= FkSpurious,
-      clearCache = false)
+    pst.xp.chain.vmState.stateDB.persist(clearEmptyAccount = fork >= FkSpurious)
     pst.cleanState = true
 
 # ------------------------------------------------------------------------------
@@ -227,9 +225,7 @@ proc vmExecGrabItem(pst: TxPackerStateRef; item: TxItemRef): Result[bool,void]
   # Commit account state DB
   vmState.stateDB.commit(accTx)
 
-  vmState.stateDB.persist(
-    clearEmptyAccount = xp.chain.nextFork >= FkSpurious,
-    clearCache = false)
+  vmState.stateDB.persist(clearEmptyAccount = xp.chain.nextFork >= FkSpurious)
   # let midRoot = vmState.stateDB.rootHash -- notused
 
   # Finish book-keeping and move item to `packed` bucket
@@ -254,9 +250,7 @@ proc vmExecCommit(pst: TxPackerStateRef)
     if vmState.generateWitness:
       db.collectWitnessData()
     # Finish up, then vmState.stateDB.rootHash may be accessed
-    db.persist(
-      clearEmptyAccount = xp.chain.nextFork >= FkSpurious,
-      clearCache = ClearCache in vmState.flags)
+    db.persist(clearEmptyAccount = xp.chain.nextFork >= FkSpurious)
 
   # Update flexi-array, set proper length
   let nItems = xp.txDB.byStatus.eq(txItemPacked).nItems

nimbus/core/validate.nim

@@ -354,7 +354,7 @@ proc validateTransaction*(
     # `eth_call` and `eth_estimateGas`
     # EOA = Externally Owned Account
     let codeHash = roDB.getCodeHash(sender)
-    if codeHash != EMPTY_SHA3:
+    if codeHash != EMPTY_CODE_HASH:
       return err("invalid tx: sender is not an EOA. sender=$1, codeHash=$2" % [
         sender.toHex, codeHash.data.toHex])
 

nimbus/db/ledger/accounts_ledger.nim

@@ -80,6 +80,7 @@ type
   LedgerSavePoint* = ref object
     parentSavepoint: LedgerSavePoint
     cache: Table[EthAddress, AccountRef]
+    dirty: Table[EthAddress, AccountRef]
     selfDestruct: HashSet[EthAddress]
     logEntries: seq[Log]
     accessList: ac_access_list.AccessList
@@ -129,6 +130,12 @@ func newCoreDbAccount(address: EthAddress): CoreDbAccount =
     codeHash: emptyEthAccount.codeHash,
     storage:  CoreDbColRef(nil))
 
+func resetCoreDbAccount(v: var CoreDbAccount) =
+  v.nonce = emptyEthAccount.nonce
+  v.balance = emptyEthAccount.balance
+  v.codeHash = emptyEthAccount.codeHash
+  v.storage = nil
+
 template noRlpException(info: static[string]; code: untyped) =
   try:
     code
@@ -196,6 +203,9 @@ proc commit*(ac: AccountsLedgerRef, sp: LedgerSavePoint) =
   for k, v in sp.cache:
     sp.parentSavepoint.cache[k] = v
 
+  for k, v in sp.dirty:
+    sp.parentSavepoint.dirty[k] = v
+
   ac.savePoint.transientStorage.merge(sp.transientStorage)
   ac.savePoint.accessList.merge(sp.accessList)
   ac.savePoint.selfDestruct.incl sp.selfDestruct
@@ -242,7 +252,7 @@ proc getAccount(
 
   # cache the account
   ac.savePoint.cache[address] = result
-
+  ac.savePoint.dirty[address] = result
 
 proc clone(acc: AccountRef, cloneStorage: bool): AccountRef =
   result = AccountRef(
@@ -256,9 +266,9 @@ proc clone(acc: AccountRef, cloneStorage: bool): AccountRef =
     result.overlayStorage = acc.overlayStorage
 
 proc isEmpty(acc: AccountRef): bool =
-  result = acc.statement.codeHash == EMPTY_SHA3 and
+  acc.statement.nonce == 0 and
     acc.statement.balance.isZero and
-    acc.statement.nonce == 0
+    acc.statement.codeHash == EMPTY_CODE_HASH
 
 template exists(acc: AccountRef): bool =
   Alive in acc.flags
@@ -294,12 +304,12 @@ proc storageValue(
   do:
     result = acc.originalStorageValue(slot, ac)
 
-proc kill(acc: AccountRef, address: EthAddress) =
+proc kill(acc: AccountRef) =
   acc.flags.excl Alive
   acc.overlayStorage.clear()
   acc.originalStorage = nil
-  acc.statement = address.newCoreDbAccount()
-  acc.code = default(seq[byte])
+  acc.statement.resetCoreDbAccount()
+  acc.code.reset()
 
 type
   PersistMode = enum
@@ -324,13 +334,13 @@ proc persistCode(acc: AccountRef, ac: AccountsLedgerRef) =
       warn logTxt "persistCode()",
        codeHash=acc.statement.codeHash, error=($$rc.error)
 
-proc persistStorage(acc: AccountRef, ac: AccountsLedgerRef, clearCache: bool) =
+proc persistStorage(acc: AccountRef, ac: AccountsLedgerRef) =
   if acc.overlayStorage.len == 0:
     # TODO: remove the storage too if we figure out
     # how to create 'virtual' storage room for each account
     return
 
-  if not clearCache and acc.originalStorage.isNil:
+  if acc.originalStorage.isNil:
     acc.originalStorage = newTable[UInt256, UInt256]()
 
   # Make sure that there is an account column on the database. This is needed
@@ -352,15 +362,13 @@ proc persistStorage(acc: AccountRef, ac: AccountsLedgerRef, clearCache: bool) =
     if rc.isErr:
       warn logTxt "persistStorage()", slot, error=($$rc.error)
 
-  if not clearCache:
-    # if we preserve cache, move the overlayStorage
-    # to originalStorage, related to EIP2200, EIP1283
-    for slot, value in acc.overlayStorage:
-      if value > 0:
-        acc.originalStorage[slot] = value
-      else:
-        acc.originalStorage.del(slot)
-    acc.overlayStorage.clear()
+  # move the overlayStorage to originalStorage, related to EIP2200, EIP1283
+  for slot, value in acc.overlayStorage:
+    if value > 0:
+      acc.originalStorage[slot] = value
+    else:
+      acc.originalStorage.del(slot)
+  acc.overlayStorage.clear()
 
   # Changing the storage trie might also change the `storage` descriptor when
   # the trie changes from empty to exixting or v.v.
@@ -378,12 +386,14 @@ proc makeDirty(ac: AccountsLedgerRef, address: EthAddress, cloneStorage = true):
   if address in ac.savePoint.cache:
     # it's already in latest savepoint
     result.flags.incl Dirty
+    ac.savePoint.dirty[address] = result
     return
 
   # put a copy into latest savepoint
   result = result.clone(cloneStorage)
   result.flags.incl Dirty
   ac.savePoint.cache[address] = result
+  ac.savePoint.dirty[address] = result
 
 proc getCodeHash*(ac: AccountsLedgerRef, address: EthAddress): Hash256 =
   let acc = ac.getAccount(address, false)
@@ -400,24 +410,33 @@ proc getNonce*(ac: AccountsLedgerRef, address: EthAddress): AccountNonce =
   if acc.isNil: emptyEthAccount.nonce
   else: acc.statement.nonce
 
+proc getCode(acc: AccountRef, kvt: CoreDxKvtRef): lent seq[byte] =
+  if CodeLoaded notin acc.flags and CodeChanged notin acc.flags:
+    if acc.statement.codeHash != EMPTY_CODE_HASH:
+      var rc = kvt.get(contractHashKey(acc.statement.codeHash).toOpenArray)
+      if rc.isErr:
+        warn logTxt "getCode()", codeHash=acc.statement.codeHash, error=($$rc.error)
+      else:
+        acc.code = move(rc.value)
+        acc.flags.incl CodeLoaded
+  else:
+    acc.flags.incl CodeLoaded # avoid hash comparisons
+
+  acc.code
+
 proc getCode*(ac: AccountsLedgerRef, address: EthAddress): seq[byte] =
   let acc = ac.getAccount(address, false)
   if acc.isNil:
     return
 
-  if CodeLoaded in acc.flags or CodeChanged in acc.flags:
-    result = acc.code
-  elif acc.statement.codeHash != EMPTY_CODE_HASH:
-    let rc = ac.kvt.get(contractHashKey(acc.statement.codeHash).toOpenArray)
-    if rc.isErr:
-      warn logTxt "getCode()", codeHash=acc.statement.codeHash, error=($$rc.error)
-    else:
-      acc.code = rc.value
-      acc.flags.incl CodeLoaded
-      result = acc.code
+  acc.getCode(ac.kvt)
 
 proc getCodeSize*(ac: AccountsLedgerRef, address: EthAddress): int =
-  ac.getCode(address).len
+  let acc = ac.getAccount(address, false)
+  if acc.isNil:
+    return
+
+  acc.getCode(ac.kvt).len
 
 proc getCommittedStorage*(ac: AccountsLedgerRef, address: EthAddress, slot: UInt256): UInt256 =
   let acc = ac.getAccount(address, false)
@@ -436,7 +455,7 @@ proc contractCollision*(ac: AccountsLedgerRef, address: EthAddress): bool =
   if acc.isNil:
     return
   acc.statement.nonce != 0 or
-    acc.statement.codeHash != EMPTY_SHA3 or
+    acc.statement.codeHash != EMPTY_CODE_HASH or
      acc.statement.storage.stateOrVoid != EMPTY_ROOT_HASH
 
 proc accountExists*(ac: AccountsLedgerRef, address: EthAddress): bool =
@@ -534,7 +553,8 @@ proc deleteAccount*(ac: AccountsLedgerRef, address: EthAddress) =
   # make sure all savepoints already committed
   doAssert(ac.savePoint.parentSavepoint.isNil)
   let acc = ac.getAccount(address)
-  acc.kill(address)
+  ac.savePoint.dirty[address] = acc
+  acc.kill()
 
 proc selfDestruct*(ac: AccountsLedgerRef, address: EthAddress) =
   ac.setBalance(address, 0.u256)
@@ -572,60 +592,54 @@ proc deleteEmptyAccount(ac: AccountsLedgerRef, address: EthAddress) =
     return
   if not acc.exists:
     return
-  acc.kill(address)
+
+  ac.savePoint.dirty[address] = acc
+  acc.kill()
 
 proc clearEmptyAccounts(ac: AccountsLedgerRef) =
-  for address, acc in ac.savePoint.cache:
+  # https://github.com/ethereum/EIPs/blob/master/EIPS/eip-161.md
+  for acc in ac.savePoint.dirty.values():
     if Touched in acc.flags and
         acc.isEmpty and acc.exists:
-      acc.kill(address)
+      acc.kill()
 
   # https://github.com/ethereum/EIPs/issues/716
   if ac.ripemdSpecial:
     ac.deleteEmptyAccount(RIPEMD_ADDR)
     ac.ripemdSpecial = false
 
 proc persist*(ac: AccountsLedgerRef,
-              clearEmptyAccount: bool = false,
-              clearCache: bool = true) =
+              clearEmptyAccount: bool = false) =
   # make sure all savepoint already committed
   doAssert(ac.savePoint.parentSavepoint.isNil)
-  var cleanAccounts = initHashSet[EthAddress]()
 
   if clearEmptyAccount:
     ac.clearEmptyAccounts()
 
   for address in ac.savePoint.selfDestruct:
     ac.deleteAccount(address)
 
-  for address, acc in ac.savePoint.cache:
-    assert address == acc.statement.address # debugging only
+  for acc in ac.savePoint.dirty.values(): # This is a hotspot in block processing
     case acc.persistMode()
     of Update:
       if CodeChanged in acc.flags:
         acc.persistCode(ac)
       if StorageChanged in acc.flags:
         # storageRoot must be updated first
         # before persisting account into merkle trie
-        acc.persistStorage(ac, clearCache)
+        acc.persistStorage(ac)
       ac.ledger.merge(acc.statement)
     of Remove:
-      ac.ledger.delete address
-      if not clearCache:
-        cleanAccounts.incl address
+      ac.ledger.delete acc.statement.address
+      ac.savePoint.cache.del acc.statement.address
     of DoNothing:
       # dead man tell no tales
       # remove touched dead account from cache
-      if not clearCache and Alive notin acc.flags:
-        cleanAccounts.incl address
+      if Alive notin acc.flags:
+        ac.savePoint.cache.del acc.statement.address
 
     acc.flags = acc.flags - resetFlags
-
-  if clearCache:
-    ac.savePoint.cache.clear()
-  else:
-    for x in cleanAccounts:
-      ac.savePoint.cache.del x
+  ac.savePoint.dirty.clear()
 
   ac.savePoint.selfDestruct.clear()
 

nimbus/db/ledger/base.nim

@@ -268,10 +268,10 @@ proc makeMultiKeys*(ldg: LedgerRef): MultiKeysRef =
   result = ldg.ac.makeMultiKeys()
   ldg.ifTrackApi: debug apiTxt, api, elapsed
 
-proc persist*(ldg: LedgerRef, clearEmptyAccount = false, clearCache = true) =
+proc persist*(ldg: LedgerRef, clearEmptyAccount = false) =
   ldg.beginTrackApi LdgPersistFn
-  ldg.ac.persist(clearEmptyAccount, clearCache)
-  ldg.ifTrackApi: debug apiTxt, api, elapsed, clearEmptyAccount, clearCache
+  ldg.ac.persist(clearEmptyAccount)
+  ldg.ifTrackApi: debug apiTxt, api, elapsed, clearEmptyAccount
 
 proc ripemdSpecial*(ldg: LedgerRef) =
   ldg.beginTrackApi LdgRipemdSpecialFn

nimbus/db/state_db/base.nim

@@ -223,7 +223,7 @@ proc getCode*(db: AccountStateDB, address: EthAddress): seq[byte] =
 
 proc contractCollision*(db: AccountStateDB, address: EthAddress): bool {.inline.} =
   db.getNonce(address) != 0 or
-    db.getCodeHash(address) != EMPTY_SHA3 or
+    db.getCodeHash(address) != EMPTY_CODE_HASH or
       db.getStorageRoot(address) != EMPTY_ROOT_HASH
 
 proc dumpAccount*(db: AccountStateDB, addressS: string): string =
@@ -238,19 +238,19 @@ proc isEmptyAccount*(db: AccountStateDB, address: EthAddress): bool =
   assert(recordFound.len > 0)
 
   let account = rlp.decode(recordFound, Account)
-  result = account.codeHash == EMPTY_SHA3 and
+  account.nonce == 0 and
     account.balance.isZero and
-    account.nonce == 0
+    account.codeHash == EMPTY_CODE_HASH
 
 proc isDeadAccount*(db: AccountStateDB, address: EthAddress): bool =
   let recordFound = db.trie.getAccountBytes(address)
   if recordFound.len > 0:
     let account = rlp.decode(recordFound, Account)
-    result = account.codeHash == EMPTY_SHA3 and
+    account.nonce == 0 and
       account.balance.isZero and
-      account.nonce == 0
+      account.codeHash == EMPTY_CODE_HASH
   else:
-    result = true
+    true
 
 proc removeEmptyRlpNode(branch: var seq[MptNodeRlpBytes]) =
   if branch.len() == 1 and branch[0] == emptyRlp:

nimbus/evm/types.nim

@@ -39,7 +39,6 @@ type
   VMFlag* = enum
     ExecutionOK
     GenerateWitness
-    ClearCache
 
   BlockContext* = object
     timestamp*        : EthTime