From 2aed798a50e051958a924c628cf4bd0a48229f05 Mon Sep 17 00:00:00 2001 From: Gary Rong Date: Wed, 28 Feb 2024 11:33:30 +0800 Subject: [PATCH] core, tests: don't create account if deployment destination is existent --- core/state/statedb.go | 32 ++++++++++---------------------- core/vm/evm.go | 14 ++++++++++---- core/vm/interface.go | 1 + tests/block_test.go | 13 +++++++++++++ tests/state_test.go | 12 ++++++++++++ 5 files changed, 46 insertions(+), 26 deletions(-) diff --git a/core/state/statedb.go b/core/state/statedb.go index a4b8cf93e2..8468d670b9 100644 --- a/core/state/statedb.go +++ b/core/state/statedb.go @@ -618,16 +618,16 @@ func (s *StateDB) setStateObject(object *stateObject) { func (s *StateDB) getOrNewStateObject(addr common.Address) *stateObject { stateObject := s.getStateObject(addr) if stateObject == nil { - stateObject, _ = s.createObject(addr) + stateObject = s.createObject(addr) } return stateObject } // createObject creates a new state object. If there is an existing account with // the given address, it is overwritten and returned as the second return value. -func (s *StateDB) createObject(addr common.Address) (newobj, prev *stateObject) { - prev = s.getDeletedStateObject(addr) // Note, prev might have been deleted, we need that! - newobj = newObject(s, addr, nil) +func (s *StateDB) createObject(addr common.Address) *stateObject { + prev := s.getDeletedStateObject(addr) // Note, prev might have been deleted, we need that! + newobj := newObject(s, addr, nil) if prev == nil { s.journal.append(createObjectChange{account: &addr}) } else { @@ -659,27 +659,15 @@ func (s *StateDB) createObject(addr common.Address) (newobj, prev *stateObject) delete(s.storagesOrigin, prev.address) } s.setStateObject(newobj) - if prev != nil && !prev.deleted { - return newobj, prev - } - return newobj, nil + return newobj } -// CreateAccount explicitly creates a state object. If a state object with the address -// already exists the balance is carried over to the new account. -// -// CreateAccount is called during the EVM CREATE operation. The situation might arise that -// a contract does the following: -// -// 1. sends funds to sha(account ++ (nonce + 1)) -// 2. tx_create(sha(account ++ nonce)) (note that this gets the address of 1) -// -// Carrying over the balance ensures that Ether doesn't disappear. +// CreateAccount explicitly creates a new state object, assuming that the +// account did not previously exist in the state. If the account already +// exists, this function will silently overwrite it which might lead to a +// consensus bug eventually. func (s *StateDB) CreateAccount(addr common.Address) { - newObj, prev := s.createObject(addr) - if prev != nil { - newObj.setBalance(prev.data.Balance) - } + s.createObject(addr) } // Copy creates a deep, independent copy of the state. diff --git a/core/vm/evm.go b/core/vm/evm.go index 16cc854908..7edf3e6660 100644 --- a/core/vm/evm.go +++ b/core/vm/evm.go @@ -433,8 +433,9 @@ func (evm *EVM) create(caller ContractRef, codeAndHash *codeAndHash, gas uint64, return nil, common.Address{}, gas, ErrNonceUintOverflow } evm.StateDB.SetNonce(caller.Address(), nonce+1) - // We add this to the access list _before_ taking a snapshot. Even if the creation fails, - // the access-list change should not be rolled back + + // We add this to the access list _before_ taking a snapshot. Even if the + // creation fails, the access-list change should not be rolled back. if evm.chainRules.IsBerlin { evm.StateDB.AddAddressToAccessList(address) } @@ -443,9 +444,14 @@ func (evm *EVM) create(caller ContractRef, codeAndHash *codeAndHash, gas uint64, if evm.StateDB.GetNonce(address) != 0 || (contractHash != (common.Hash{}) && contractHash != types.EmptyCodeHash) { return nil, common.Address{}, 0, ErrContractAddressCollision } - // Create a new account on the state + // Create a new account on the state only if the object was not present. + // It might be possible the contract code is deployed to a pre-existent + // account with non-zero balance and potential non-empty storage. If so, + // inherit the leftover balance and storage instead of clearing it. snapshot := evm.StateDB.Snapshot() - evm.StateDB.CreateAccount(address) + if !evm.StateDB.Exist(address) { + evm.StateDB.CreateAccount(address) + } if evm.chainRules.IsEIP158 { evm.StateDB.SetNonce(address, 1) } diff --git a/core/vm/interface.go b/core/vm/interface.go index 25bfa06720..101f28b900 100644 --- a/core/vm/interface.go +++ b/core/vm/interface.go @@ -60,6 +60,7 @@ type StateDB interface { // Exist reports whether the given account exists in state. // Notably this should also return true for self-destructed accounts. Exist(common.Address) bool + // Empty returns whether the given account is empty. Empty // is defined according to EIP161 (balance = nonce = code = 0). Empty(common.Address) bool diff --git a/tests/block_test.go b/tests/block_test.go index fb355085fd..2db1a23286 100644 --- a/tests/block_test.go +++ b/tests/block_test.go @@ -50,6 +50,11 @@ func TestBlockchain(t *testing.T) { // using 4.6 TGas bt.skipLoad(`.*randomStatetest94.json.*`) + // The tests under Pyspecs are the ones that are published as execution-spect tests. + // We run these tests separately, no need to _also_ run them as part of the + // reference tests. + bt.skipLoad(`^Pyspecs/`) + bt.walk(t, blockTestDir, func(t *testing.T, name string, test *BlockTest) { if runtime.GOARCH == "386" && runtime.GOOS == "windows" && rand.Int63()%2 == 0 { t.Skip("test (randomly) skipped on 32-bit windows") @@ -68,6 +73,14 @@ func TestExecutionSpecBlocktests(t *testing.T) { } bt := new(testMatcher) + // These tests fail as of https://github.com/ethereum/go-ethereum/pull/28666, since we + // no longer delete "leftover storage" when deploying a contract. + bt.skipLoad(`^cancun/eip6780_selfdestruct/selfdestruct/self_destructing_initcode_create_tx.json`) + bt.skipLoad(`^cancun/eip6780_selfdestruct/selfdestruct/self_destructing_initcode.json`) + bt.skipLoad(`^cancun/eip6780_selfdestruct/selfdestruct/recreate_self_destructed_contract_different_txs.json`) + bt.skipLoad(`^cancun/eip6780_selfdestruct/selfdestruct/delegatecall_from_new_contract_to_pre_existing_contract.json`) + bt.skipLoad(`^cancun/eip6780_selfdestruct/selfdestruct/create_selfdestruct_same_tx.json`) + bt.walk(t, executionSpecBlockchainTestDir, func(t *testing.T, name string, test *BlockTest) { execBlockTest(t, bt, test) }) diff --git a/tests/state_test.go b/tests/state_test.go index 1d749d8bcf..a5de9f862d 100644 --- a/tests/state_test.go +++ b/tests/state_test.go @@ -55,6 +55,18 @@ func initMatcher(st *testMatcher) { // Uses 1GB RAM per tested fork st.skipLoad(`^stStaticCall/static_Call1MB`) + // These tests fail as of https://github.com/ethereum/go-ethereum/pull/28666, since we + // no longer delete "leftover storage" when deploying a contract. + st.skipLoad(`^stSStoreTest/InitCollision\.json`) + st.skipLoad(`^stRevertTest/RevertInCreateInInit\.json`) + st.skipLoad(`^stExtCodeHash/dynamicAccountOverwriteEmpty\.json`) + st.skipLoad(`^stCreate2/create2collisionStorage\.json`) + st.skipLoad(`^stCreate2/RevertInCreateInInitCreate2\.json`) + st.skipLoad(`^stRevertTest/RevertInCreateInInit\.json`) + st.skipLoad(`^stExtCodeHash/dynamicAccountOverwriteEmpty\.json`) + st.skipLoad(`^stCreate2/create2collisionStorage\.json`) + st.skipLoad(`^stCreate2/RevertInCreateInInitCreate2\.json`) + // Broken tests: // EOF is not part of cancun st.skipLoad(`^stEOF/`)