From 6814140c63e7e2b1824e11e421fe18ed345611d7 Mon Sep 17 00:00:00 2001 From: jangko Date: Mon, 9 Jan 2023 21:39:36 +0700 Subject: [PATCH] unify coinbase state clearing of gst, evmstate, and t8n it is troublesome if we have to fix it in three places in case of a bug, it also reduce code duplication. --- nimbus/common/common.nim | 3 +++ tests/test_blockchain_json.nim | 2 ++ tests/test_generalstate_json.nim | 23 ++-------------- tools/common/state_clearing.nim | 45 ++++++++++++++++++++++++++++++++ tools/evmstate/evmstate.nim | 11 +++----- tools/t8n/transition.nim | 11 +++----- 6 files changed, 58 insertions(+), 37 deletions(-) create mode 100644 tools/common/state_clearing.nim diff --git a/nimbus/common/common.nim b/nimbus/common/common.nim index a09ca8bc8..c861230ed 100644 --- a/nimbus/common/common.nim +++ b/nimbus/common/common.nim @@ -295,6 +295,9 @@ func toEVMFork*(com: CommonRef, number: BlockNumber): EVMFork = let fork = com.toHardFork(number) ToEVMFork[fork] +func toEVMFork*(com: CommonRef): EVMFork = + ToEVMFork[com.currentFork] + func isLondon*(com: CommonRef, number: BlockNumber): bool = # TODO: Fixme, use only London comparator com.toHardFork(number) >= London diff --git a/tests/test_blockchain_json.nim b/tests/test_blockchain_json.nim index 5e0177633..c146953f7 100644 --- a/tests/test_blockchain_json.nim +++ b/tests/test_blockchain_json.nim @@ -390,6 +390,8 @@ proc testFixture(node: JsonNode, testStatusIMPL: var TestStatus, debugMode = fal # state root to test against 'postState' let stateDB = AccountsCache.init(memDB, header.stateRoot, pruneTrie) verifyStateDB(fixture["postState"], ReadOnlyStateDB(stateDB)) + + success = lastBlockHash == tester.lastBlockHash except ValidationError as E: echo fixtureName, " ERROR: ", E.msg success = false diff --git a/tests/test_generalstate_json.nim b/tests/test_generalstate_json.nim index 104f97f8e..94edafdb4 100644 --- a/tests/test_generalstate_json.nim +++ b/tests/test_generalstate_json.nim @@ -16,6 +16,7 @@ import ../nimbus/utils/utils, ../tools/common/helpers as chp, ../tools/evmstate/helpers, + ../tools/common/state_clearing, chronicles, eth/rlp, eth/trie/trie_defs, @@ -124,28 +125,8 @@ proc testFixtureIndexes(tester: Tester, testStatusIMPL: var TestStatus) = if rc.isOk: gasUsed = rc.value - # This is necessary due to the manner in which the state tests are - # generated. State tests are generated from the BlockChainTest tests - # in which these transactions are included in the larger context of a - # block and thus, the mechanisms which would touch/create/clear the - # coinbase account based on the mining reward are present during test - # generation, but not part of the execution, thus we must artificially - # create the account in VMs prior to the state clearing rules, - # as well as conditionally cleaning up the coinbase account when left - # empty in VMs after the state clearing rules came into effect. let miner = tester.header.coinbase - if miner in vmState.selfDestructs: - vmState.mutateStateDB: - db.addBalance(miner, 0.u256) - if fork >= FkSpurious: - if db.isEmptyAccount(miner): - db.deleteAccount(miner) - - # this is an important step when using accounts_cache - # it will affect the account storage's location - # during the next call to `getComittedStorage` - # and the result of rootHash - db.persist() + coinbaseStateClearing(vmState, miner, fork) proc testFixture(fixtures: JsonNode, testStatusIMPL: var TestStatus, trace = false, debugMode = false) = diff --git a/tools/common/state_clearing.nim b/tools/common/state_clearing.nim new file mode 100644 index 000000000..c92f6df50 --- /dev/null +++ b/tools/common/state_clearing.nim @@ -0,0 +1,45 @@ +# Nimbus +# Copyright (c) 2023 Status Research & Development GmbH +# Licensed under either of +# * Apache License, version 2.0, ([LICENSE-APACHE](LICENSE-APACHE) or +# http://www.apache.org/licenses/LICENSE-2.0) +# * MIT license ([LICENSE-MIT](LICENSE-MIT) or +# http://opensource.org/licenses/MIT) +# at your option. This file may not be copied, modified, or distributed except +# according to those terms. + +import + ../../nimbus/common/common, + ../../nimbus/[vm_state, vm_types], + ../../nimbus/db/accounts_cache + +proc coinbaseStateClearing*(vmState: BaseVMState, + miner: EthAddress, + fork: EVMFork, + touched = true) = + # This is necessary due to the manner in which the state tests are + # generated. State tests are generated from the BlockChainTest tests + # in which these transactions are included in the larger context of a + # block and thus, the mechanisms which would touch/create/clear the + # coinbase account based on the mining reward are present during test + # generation, but not part of the execution, thus we must artificially + # create the account in VMs prior to the state clearing rules, + # as well as conditionally cleaning up the coinbase account when left + # empty in VMs after the state clearing rules came into effect. + + vmState.mutateStateDB: + if touched: + db.addBalance(miner, 0.u256) + if fork >= FkSpurious: + # EIP158/161 state clearing + if db.accountExists(miner) and db.isEmptyAccount(miner): + db.deleteAccount(miner) + + # db.persist is an important step when using accounts_cache + # it will affect the account storage's location + # during the next call to `getComittedStorage` + # and the result of rootHash + + # do not clear cache, we need the cache when constructing + # post state + db.persist(clearCache = false) diff --git a/tools/evmstate/evmstate.nim b/tools/evmstate/evmstate.nim index 9368d4a6c..25562f3de 100644 --- a/tools/evmstate/evmstate.nim +++ b/tools/evmstate/evmstate.nim @@ -21,7 +21,8 @@ import ../../nimbus/core/executor, ../../nimbus/common/common, ../common/helpers as chp, - "."/[config, helpers] + "."/[config, helpers], + ../common/state_clearing type StateContext = object @@ -235,13 +236,7 @@ proc runExecution(ctx: var StateContext, conf: StateConf, pre: JsonNode): StateR gasUsed = rc.value let miner = ctx.header.coinbase - if miner in vmState.selfDestructs: - vmState.mutateStateDB: - db.addBalance(miner, 0.u256) - if fork >= FkSpurious: - if db.isEmptyAccount(miner): - db.deleteAccount(miner) - db.persist(clearCache = false) + coinbaseStateClearing(vmState, miner, fork) proc toTracerFlags(conf: Stateconf): set[TracerFlags] = result = { diff --git a/tools/t8n/transition.nim b/tools/t8n/transition.nim index 9c99eb0bf..a068f0cf0 100644 --- a/tools/t8n/transition.nim +++ b/tools/t8n/transition.nim @@ -13,6 +13,7 @@ import eth/[rlp, trie, eip1559], stint, chronicles, stew/results, "."/[config, types, helpers], + ../common/state_clearing, ../../nimbus/[vm_types, vm_state, transaction], ../../nimbus/common/common, ../../nimbus/db/accounts_cache, @@ -222,14 +223,8 @@ proc exec(ctx: var TransContext, db.persist(clearCache = false) let miner = ctx.env.currentCoinbase - if vmState.com.forkGTE(Spurious): - # EIP158/161 state clearing - vmState.mutateStateDB: - if db.accountExists(miner) and db.isEmptyAccount(miner): - db.deleteAccount(miner) - # do not clear cache, we need the cache when constructing - # post state - db.persist(clearCache = false) + let fork = vmState.com.toEVMFork + coinbaseStateClearing(vmState, miner, fork, stateReward.isSome()) let stateDB = vmState.stateDB stateDB.postState(result.alloc)