From 0d4c38865e9cda492e71221c4c429d9b1bec8ac5 Mon Sep 17 00:00:00 2001 From: Martin HS Date: Wed, 24 Apr 2024 11:59:06 +0200 Subject: [PATCH 1/4] core/state: remove account reset operation v2 (#29520) * core/state, tests: remove account reset operation * core/state, core/vm: implement createcontract journal event * core/state: make createcontract not emit dirtied account, unskip tests * core/state: add createcontract to journal fuzzing * core/state: fix journal * core/state: address comments * core/state: remove useless code --------- Co-authored-by: Gary Rong --- core/state/journal.go | 151 ++++++++++++++---- core/state/state_object.go | 54 +++---- core/state/state_test.go | 102 +----------- core/state/statedb.go | 270 ++++++++++++++++---------------- core/state/statedb_fuzz_test.go | 4 +- core/state/statedb_test.go | 173 ++++++++++++++------ core/vm/evm.go | 21 ++- core/vm/interface.go | 1 + tests/block_test.go | 8 - 9 files changed, 426 insertions(+), 358 deletions(-) diff --git a/core/state/journal.go b/core/state/journal.go index 6cdc1fc868..cfd3782eb0 100644 --- a/core/state/journal.go +++ b/core/state/journal.go @@ -17,6 +17,8 @@ package state import ( + "maps" + "github.com/ethereum/go-ethereum/common" "github.com/holiman/uint256" ) @@ -29,6 +31,9 @@ type journalEntry interface { // dirtied returns the Ethereum address modified by this journal entry. dirtied() *common.Address + + // copy returns a deep-copied journal entry. + copy() journalEntry } // journal contains the list of state modifications applied since the last state @@ -83,22 +88,31 @@ func (j *journal) length() int { return len(j.entries) } +// copy returns a deep-copied journal. +func (j *journal) copy() *journal { + entries := make([]journalEntry, 0, j.length()) + for i := 0; i < j.length(); i++ { + entries = append(entries, j.entries[i].copy()) + } + return &journal{ + entries: entries, + dirties: maps.Clone(j.dirties), + } +} + type ( // Changes to the account trie. createObjectChange struct { account *common.Address } - resetObjectChange struct { - account *common.Address - prev *stateObject - prevdestruct bool - prevAccount []byte - prevStorage map[common.Hash][]byte - prevAccountOriginExist bool - prevAccountOrigin []byte - prevStorageOrigin map[common.Hash][]byte + // createContractChange represents an account becoming a contract-account. + // This event happens prior to executing initcode. The journal-event simply + // manages the created-flag, in order to allow same-tx destruction. + createContractChange struct { + account common.Address } + selfDestructChange struct { account *common.Address prev bool // whether account had already self-destructed @@ -136,6 +150,7 @@ type ( touchChange struct { account *common.Address } + // Changes to the access list accessListAddAccountChange struct { address *common.Address @@ -145,6 +160,7 @@ type ( slot *common.Hash } + // Changes to transient storage transientStorageChange struct { account *common.Address key, prevalue common.Hash @@ -153,34 +169,30 @@ type ( func (ch createObjectChange) revert(s *StateDB) { delete(s.stateObjects, *ch.account) - delete(s.stateObjectsDirty, *ch.account) } func (ch createObjectChange) dirtied() *common.Address { return ch.account } -func (ch resetObjectChange) revert(s *StateDB) { - s.setStateObject(ch.prev) - if !ch.prevdestruct { - delete(s.stateObjectsDestruct, ch.prev.address) - } - if ch.prevAccount != nil { - s.accounts[ch.prev.addrHash] = ch.prevAccount - } - if ch.prevStorage != nil { - s.storages[ch.prev.addrHash] = ch.prevStorage - } - if ch.prevAccountOriginExist { - s.accountsOrigin[ch.prev.address] = ch.prevAccountOrigin - } - if ch.prevStorageOrigin != nil { - s.storagesOrigin[ch.prev.address] = ch.prevStorageOrigin +func (ch createObjectChange) copy() journalEntry { + return createObjectChange{ + account: ch.account, } } -func (ch resetObjectChange) dirtied() *common.Address { - return ch.account +func (ch createContractChange) revert(s *StateDB) { + s.getStateObject(ch.account).newContract = false +} + +func (ch createContractChange) dirtied() *common.Address { + return nil +} + +func (ch createContractChange) copy() journalEntry { + return createContractChange{ + account: ch.account, + } } func (ch selfDestructChange) revert(s *StateDB) { @@ -195,6 +207,14 @@ func (ch selfDestructChange) dirtied() *common.Address { return ch.account } +func (ch selfDestructChange) copy() journalEntry { + return selfDestructChange{ + account: ch.account, + prev: ch.prev, + prevbalance: new(uint256.Int).Set(ch.prevbalance), + } +} + var ripemd = common.HexToAddress("0000000000000000000000000000000000000003") func (ch touchChange) revert(s *StateDB) { @@ -204,6 +224,12 @@ func (ch touchChange) dirtied() *common.Address { return ch.account } +func (ch touchChange) copy() journalEntry { + return touchChange{ + account: ch.account, + } +} + func (ch balanceChange) revert(s *StateDB) { s.getStateObject(*ch.account).setBalance(ch.prev) } @@ -212,6 +238,13 @@ func (ch balanceChange) dirtied() *common.Address { return ch.account } +func (ch balanceChange) copy() journalEntry { + return balanceChange{ + account: ch.account, + prev: new(uint256.Int).Set(ch.prev), + } +} + func (ch nonceChange) revert(s *StateDB) { s.getStateObject(*ch.account).setNonce(ch.prev) } @@ -220,6 +253,13 @@ func (ch nonceChange) dirtied() *common.Address { return ch.account } +func (ch nonceChange) copy() journalEntry { + return nonceChange{ + account: ch.account, + prev: ch.prev, + } +} + func (ch codeChange) revert(s *StateDB) { s.getStateObject(*ch.account).setCode(common.BytesToHash(ch.prevhash), ch.prevcode) } @@ -228,6 +268,14 @@ func (ch codeChange) dirtied() *common.Address { return ch.account } +func (ch codeChange) copy() journalEntry { + return codeChange{ + account: ch.account, + prevhash: common.CopyBytes(ch.prevhash), + prevcode: common.CopyBytes(ch.prevcode), + } +} + func (ch storageChange) revert(s *StateDB) { s.getStateObject(*ch.account).setState(ch.key, ch.prevalue) } @@ -236,6 +284,14 @@ func (ch storageChange) dirtied() *common.Address { return ch.account } +func (ch storageChange) copy() journalEntry { + return storageChange{ + account: ch.account, + key: ch.key, + prevalue: ch.prevalue, + } +} + func (ch transientStorageChange) revert(s *StateDB) { s.setTransientState(*ch.account, ch.key, ch.prevalue) } @@ -244,6 +300,14 @@ func (ch transientStorageChange) dirtied() *common.Address { return nil } +func (ch transientStorageChange) copy() journalEntry { + return transientStorageChange{ + account: ch.account, + key: ch.key, + prevalue: ch.prevalue, + } +} + func (ch refundChange) revert(s *StateDB) { s.refund = ch.prev } @@ -252,6 +316,12 @@ func (ch refundChange) dirtied() *common.Address { return nil } +func (ch refundChange) copy() journalEntry { + return refundChange{ + prev: ch.prev, + } +} + func (ch addLogChange) revert(s *StateDB) { logs := s.logs[ch.txhash] if len(logs) == 1 { @@ -266,6 +336,12 @@ func (ch addLogChange) dirtied() *common.Address { return nil } +func (ch addLogChange) copy() journalEntry { + return addLogChange{ + txhash: ch.txhash, + } +} + func (ch addPreimageChange) revert(s *StateDB) { delete(s.preimages, ch.hash) } @@ -274,6 +350,12 @@ func (ch addPreimageChange) dirtied() *common.Address { return nil } +func (ch addPreimageChange) copy() journalEntry { + return addPreimageChange{ + hash: ch.hash, + } +} + func (ch accessListAddAccountChange) revert(s *StateDB) { /* One important invariant here, is that whenever a (addr, slot) is added, if the @@ -291,6 +373,12 @@ func (ch accessListAddAccountChange) dirtied() *common.Address { return nil } +func (ch accessListAddAccountChange) copy() journalEntry { + return accessListAddAccountChange{ + address: ch.address, + } +} + func (ch accessListAddSlotChange) revert(s *StateDB) { s.accessList.DeleteSlot(*ch.address, *ch.slot) } @@ -298,3 +386,10 @@ func (ch accessListAddSlotChange) revert(s *StateDB) { func (ch accessListAddSlotChange) dirtied() *common.Address { return nil } + +func (ch accessListAddSlotChange) copy() journalEntry { + return accessListAddSlotChange{ + address: ch.address, + slot: ch.slot, + } +} diff --git a/core/state/state_object.go b/core/state/state_object.go index db3c32f2f2..8978b30542 100644 --- a/core/state/state_object.go +++ b/core/state/state_object.go @@ -32,21 +32,8 @@ import ( "github.com/holiman/uint256" ) -type Code []byte - -func (c Code) String() string { - return string(c) //strings.Join(Disassemble(c), " ") -} - type Storage map[common.Hash]common.Hash -func (s Storage) String() (str string) { - for key, value := range s { - str += fmt.Sprintf("%X : %X\n", key, value) - } - return -} - func (s Storage) Copy() Storage { return maps.Clone(s) } @@ -65,8 +52,8 @@ type stateObject struct { data types.StateAccount // Account data with all mutations applied in the scope of block // Write caches. - trie Trie // storage trie, which becomes non-nil on first access - code Code // contract bytecode, which gets set when code is loaded + trie Trie // storage trie, which becomes non-nil on first access + code []byte // contract bytecode, which gets set when code is loaded originStorage Storage // Storage cache of original entries to dedup rewrites pendingStorage Storage // Storage entries that need to be flushed to disk, at the end of an entire block @@ -75,17 +62,16 @@ type stateObject struct { // Cache flags. dirtyCode bool // true if the code was updated - // Flag whether the account was marked as self-destructed. The self-destructed account - // is still accessible in the scope of same transaction. + // Flag whether the account was marked as self-destructed. The self-destructed + // account is still accessible in the scope of same transaction. selfDestructed bool - // Flag whether the account was marked as deleted. A self-destructed account - // or an account that is considered as empty will be marked as deleted at - // the end of transaction and no longer accessible anymore. - deleted bool - - // Flag whether the object was created in the current transaction - created bool + // This is an EIP-6780 flag indicating whether the object is eligible for + // self-destruct according to EIP-6780. The flag could be set either when + // the contract is just created within the current transaction, or when the + // object was previously existent and is being deployed as a contract within + // the current transaction. + newContract bool } // empty returns whether the account is considered empty. @@ -95,10 +81,7 @@ func (s *stateObject) empty() bool { // newObject creates a state object. func newObject(db *StateDB, address common.Address, acct *types.StateAccount) *stateObject { - var ( - origin = acct - created = acct == nil // true if the account was not existent - ) + origin := acct if acct == nil { acct = types.NewEmptyStateAccount() } @@ -111,7 +94,6 @@ func newObject(db *StateDB, address common.Address, acct *types.StateAccount) *s originStorage: make(Storage), pendingStorage: make(Storage), dirtyStorage: make(Storage), - created: created, } } @@ -264,6 +246,10 @@ func (s *stateObject) finalise(prefetch bool) { if len(s.dirtyStorage) > 0 { s.dirtyStorage = make(Storage) } + // Revoke the flag at the end of the transaction. It finalizes the status + // of the newly-created object as it's no longer eligible for self-destruct + // by EIP-6780. For non-newly-created objects, it's a no-op. + s.newContract = false } // updateTrie is responsible for persisting cached storage changes into the @@ -463,12 +449,12 @@ func (s *stateObject) deepCopy(db *StateDB) *stateObject { obj.trie = db.db.CopyTrie(s.trie) } obj.code = s.code - obj.dirtyStorage = s.dirtyStorage.Copy() obj.originStorage = s.originStorage.Copy() obj.pendingStorage = s.pendingStorage.Copy() - obj.selfDestructed = s.selfDestructed + obj.dirtyStorage = s.dirtyStorage.Copy() obj.dirtyCode = s.dirtyCode - obj.deleted = s.deleted + obj.selfDestructed = s.selfDestructed + obj.newContract = s.newContract return obj } @@ -483,7 +469,7 @@ func (s *stateObject) Address() common.Address { // Code returns the contract code associated with this object, if any. func (s *stateObject) Code() []byte { - if s.code != nil { + if len(s.code) != 0 { return s.code } if bytes.Equal(s.CodeHash(), types.EmptyCodeHash.Bytes()) { @@ -501,7 +487,7 @@ func (s *stateObject) Code() []byte { // or zero if none. This method is an almost mirror of Code, but uses a cache // inside the database to avoid loading codes seen recently. func (s *stateObject) CodeSize() int { - if s.code != nil { + if len(s.code) != 0 { return len(s.code) } if bytes.Equal(s.CodeHash(), types.EmptyCodeHash.Bytes()) { diff --git a/core/state/state_test.go b/core/state/state_test.go index c6e6db906e..9200e4abe9 100644 --- a/core/state/state_test.go +++ b/core/state/state_test.go @@ -194,106 +194,20 @@ func TestSnapshotEmpty(t *testing.T) { s.state.RevertToSnapshot(s.state.Snapshot()) } -func TestSnapshot2(t *testing.T) { +func TestCreateObjectRevert(t *testing.T) { state, _ := New(types.EmptyRootHash, NewDatabase(rawdb.NewMemoryDatabase()), nil) + addr := common.BytesToAddress([]byte("so0")) + snap := state.Snapshot() - stateobjaddr0 := common.BytesToAddress([]byte("so0")) - stateobjaddr1 := common.BytesToAddress([]byte("so1")) - var storageaddr common.Hash - - data0 := common.BytesToHash([]byte{17}) - data1 := common.BytesToHash([]byte{18}) - - state.SetState(stateobjaddr0, storageaddr, data0) - state.SetState(stateobjaddr1, storageaddr, data1) - - // db, trie are already non-empty values - so0 := state.getStateObject(stateobjaddr0) + state.CreateAccount(addr) + so0 := state.getStateObject(addr) so0.SetBalance(uint256.NewInt(42), tracing.BalanceChangeUnspecified) so0.SetNonce(43) so0.SetCode(crypto.Keccak256Hash([]byte{'c', 'a', 'f', 'e'}), []byte{'c', 'a', 'f', 'e'}) - so0.selfDestructed = false - so0.deleted = false state.setStateObject(so0) - root, _ := state.Commit(0, false) - state, _ = New(root, state.db, state.snaps) - - // and one with deleted == true - so1 := state.getStateObject(stateobjaddr1) - so1.SetBalance(uint256.NewInt(52), tracing.BalanceChangeUnspecified) - so1.SetNonce(53) - so1.SetCode(crypto.Keccak256Hash([]byte{'c', 'a', 'f', 'e', '2'}), []byte{'c', 'a', 'f', 'e', '2'}) - so1.selfDestructed = true - so1.deleted = true - state.setStateObject(so1) - - so1 = state.getStateObject(stateobjaddr1) - if so1 != nil { - t.Fatalf("deleted object not nil when getting") - } - - snapshot := state.Snapshot() - state.RevertToSnapshot(snapshot) - - so0Restored := state.getStateObject(stateobjaddr0) - // Update lazily-loaded values before comparing. - so0Restored.GetState(storageaddr) - so0Restored.Code() - // non-deleted is equal (restored) - compareStateObjects(so0Restored, so0, t) - - // deleted should be nil, both before and after restore of state copy - so1Restored := state.getStateObject(stateobjaddr1) - if so1Restored != nil { - t.Fatalf("deleted object not nil after restoring snapshot: %+v", so1Restored) - } -} - -func compareStateObjects(so0, so1 *stateObject, t *testing.T) { - if so0.Address() != so1.Address() { - t.Fatalf("Address mismatch: have %v, want %v", so0.address, so1.address) - } - if so0.Balance().Cmp(so1.Balance()) != 0 { - t.Fatalf("Balance mismatch: have %v, want %v", so0.Balance(), so1.Balance()) - } - if so0.Nonce() != so1.Nonce() { - t.Fatalf("Nonce mismatch: have %v, want %v", so0.Nonce(), so1.Nonce()) - } - if so0.data.Root != so1.data.Root { - t.Errorf("Root mismatch: have %x, want %x", so0.data.Root[:], so1.data.Root[:]) - } - if !bytes.Equal(so0.CodeHash(), so1.CodeHash()) { - t.Fatalf("CodeHash mismatch: have %v, want %v", so0.CodeHash(), so1.CodeHash()) - } - if !bytes.Equal(so0.code, so1.code) { - t.Fatalf("Code mismatch: have %v, want %v", so0.code, so1.code) - } - - if len(so1.dirtyStorage) != len(so0.dirtyStorage) { - t.Errorf("Dirty storage size mismatch: have %d, want %d", len(so1.dirtyStorage), len(so0.dirtyStorage)) - } - for k, v := range so1.dirtyStorage { - if so0.dirtyStorage[k] != v { - t.Errorf("Dirty storage key %x mismatch: have %v, want %v", k, so0.dirtyStorage[k], v) - } - } - for k, v := range so0.dirtyStorage { - if so1.dirtyStorage[k] != v { - t.Errorf("Dirty storage key %x mismatch: have %v, want none.", k, v) - } - } - if len(so1.originStorage) != len(so0.originStorage) { - t.Errorf("Origin storage size mismatch: have %d, want %d", len(so1.originStorage), len(so0.originStorage)) - } - for k, v := range so1.originStorage { - if so0.originStorage[k] != v { - t.Errorf("Origin storage key %x mismatch: have %v, want %v", k, so0.originStorage[k], v) - } - } - for k, v := range so0.originStorage { - if so1.originStorage[k] != v { - t.Errorf("Origin storage key %x mismatch: have %v, want none.", k, v) - } + state.RevertToSnapshot(snap) + if state.Exist(addr) { + t.Error("Unexpected account after revert") } } diff --git a/core/state/statedb.go b/core/state/statedb.go index ab152dd18d..4a934fe82c 100644 --- a/core/state/statedb.go +++ b/core/state/statedb.go @@ -44,6 +44,26 @@ type revision struct { journalIndex int } +type mutationType int + +const ( + update mutationType = iota + deletion +) + +type mutation struct { + typ mutationType + applied bool +} + +func (m *mutation) copy() *mutation { + return &mutation{typ: m.typ, applied: m.applied} +} + +func (m *mutation) isDelete() bool { + return m.typ == deletion +} + // StateDB structs within the ethereum protocol are used to store anything // within the merkle trie. StateDBs take care of caching and storing // nested states. It's the general query interface to retrieve: @@ -75,12 +95,22 @@ type StateDB struct { accountsOrigin map[common.Address][]byte // The original value of mutated accounts in 'slim RLP' encoding storagesOrigin map[common.Address]map[common.Hash][]byte // The original value of mutated slots in prefix-zero trimmed rlp format - // This map holds 'live' objects, which will get modified while processing - // a state transition. - stateObjects map[common.Address]*stateObject - stateObjectsPending map[common.Address]struct{} // State objects finalized but not yet written to the trie - stateObjectsDirty map[common.Address]struct{} // State objects modified in the current execution - stateObjectsDestruct map[common.Address]*types.StateAccount // State objects destructed in the block along with its previous value + // This map holds 'live' objects, which will get modified while + // processing a state transition. + stateObjects map[common.Address]*stateObject + + // This map holds 'deleted' objects. An object with the same address + // might also occur in the 'stateObjects' map due to account + // resurrection. The account value is tracked as the original value + // before the transition. This map is populated at the transaction + // boundaries. + stateObjectsDestruct map[common.Address]*types.StateAccount + + // This map tracks the account mutations that occurred during the + // transition. Uncommitted mutations belonging to the same account + // can be merged into a single one which is equivalent from database's + // perspective. This map is populated at the transaction boundaries. + mutations map[common.Address]*mutation // DB error. // State objects are used by the consensus core and VM which are @@ -154,9 +184,8 @@ func New(root common.Hash, db Database, snaps *snapshot.Tree) (*StateDB, error) accountsOrigin: make(map[common.Address][]byte), storagesOrigin: make(map[common.Address]map[common.Hash][]byte), stateObjects: make(map[common.Address]*stateObject), - stateObjectsPending: make(map[common.Address]struct{}), - stateObjectsDirty: make(map[common.Address]struct{}), stateObjectsDestruct: make(map[common.Address]*types.StateAccount), + mutations: make(map[common.Address]*mutation), logs: make(map[common.Hash][]*types.Log), preimages: make(map[common.Hash][]byte), journal: newJournal(), @@ -472,8 +501,7 @@ func (s *StateDB) Selfdestruct6780(addr common.Address) { if stateObject == nil { return } - - if stateObject.created { + if stateObject.newContract { s.SelfDestruct(addr) } } @@ -552,24 +580,16 @@ func (s *StateDB) deleteStateObject(addr common.Address) { } // getStateObject retrieves a state object given by the address, returning nil if -// the object is not found or was deleted in this execution context. If you need -// to differentiate between non-existent/just-deleted, use getDeletedStateObject. +// the object is not found or was deleted in this execution context. func (s *StateDB) getStateObject(addr common.Address) *stateObject { - if obj := s.getDeletedStateObject(addr); obj != nil && !obj.deleted { - return obj - } - return nil -} - -// getDeletedStateObject is similar to getStateObject, but instead of returning -// nil for a deleted state object, it returns the actual object with the deleted -// flag set. This is needed by the state journal to revert to the correct s- -// destructed object instead of wiping all knowledge about the state object. -func (s *StateDB) getDeletedStateObject(addr common.Address) *stateObject { // Prefer live objects if any is available if obj := s.stateObjects[addr]; obj != nil { return obj } + // Short circuit if the account is already destructed in this block. + if _, ok := s.stateObjectsDestruct[addr]; ok { + return nil + } // If no live objects are available, attempt to use snapshots var data *types.StateAccount if s.snap != nil { @@ -622,69 +642,40 @@ func (s *StateDB) setStateObject(object *stateObject) { // getOrNewStateObject retrieves a state object or create a new state object if nil. func (s *StateDB) getOrNewStateObject(addr common.Address) *stateObject { - stateObject := s.getStateObject(addr) - if stateObject == nil { - stateObject, _ = s.createObject(addr) + obj := s.getStateObject(addr) + if obj == nil { + obj = s.createObject(addr) } - return stateObject + return obj } -// 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) - if prev == nil { - s.journal.append(createObjectChange{account: &addr}) - } else { - // The original account should be marked as destructed and all cached - // account and storage data should be cleared as well. Note, it must - // be done here, otherwise the destruction event of "original account" - // will be lost. - _, prevdestruct := s.stateObjectsDestruct[prev.address] - if !prevdestruct { - s.stateObjectsDestruct[prev.address] = prev.origin - } - // There may be some cached account/storage data already since IntermediateRoot - // will be called for each transaction before byzantium fork which will always - // cache the latest account/storage data. - prevAccount, ok := s.accountsOrigin[prev.address] - s.journal.append(resetObjectChange{ - account: &addr, - prev: prev, - prevdestruct: prevdestruct, - prevAccount: s.accounts[prev.addrHash], - prevStorage: s.storages[prev.addrHash], - prevAccountOriginExist: ok, - prevAccountOrigin: prevAccount, - prevStorageOrigin: s.storagesOrigin[prev.address], - }) - delete(s.accounts, prev.addrHash) - delete(s.storages, prev.addrHash) - delete(s.accountsOrigin, prev.address) - delete(s.storagesOrigin, prev.address) - } - s.setStateObject(newobj) - if prev != nil && !prev.deleted { - return newobj, prev - } - return newobj, nil +// createObject creates a new state object. The assumption is held there is no +// existing account with the given address, otherwise it will be silently overwritten. +func (s *StateDB) createObject(addr common.Address) *stateObject { + obj := newObject(s, addr, nil) + s.journal.append(createObjectChange{account: &addr}) + s.setStateObject(obj) + return obj } -// 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) +} + +// CreateContract is used whenever a contract is created. This may be preceded +// by CreateAccount, but that is not required if it already existed in the +// state due to funds sent beforehand. +// This operation sets the 'newContract'-flag, which is required in order to +// correctly handle EIP-6780 'delete-in-same-transaction' logic. +func (s *StateDB) CreateContract(addr common.Address) { + obj := s.getStateObject(addr) + if !obj.newContract { + obj.newContract = true + s.journal.append(createContractChange{account: addr}) } } @@ -695,21 +686,25 @@ func (s *StateDB) Copy() *StateDB { state := &StateDB{ db: s.db, trie: s.db.CopyTrie(s.trie), + hasher: crypto.NewKeccakState(), originalRoot: s.originalRoot, accounts: copySet(s.accounts), storages: copy2DSet(s.storages), accountsOrigin: copySet(s.accountsOrigin), storagesOrigin: copy2DSet(s.storagesOrigin), - stateObjects: make(map[common.Address]*stateObject, len(s.journal.dirties)), - stateObjectsPending: make(map[common.Address]struct{}, len(s.stateObjectsPending)), - stateObjectsDirty: make(map[common.Address]struct{}, len(s.journal.dirties)), + stateObjects: make(map[common.Address]*stateObject, len(s.stateObjects)), stateObjectsDestruct: maps.Clone(s.stateObjectsDestruct), + mutations: make(map[common.Address]*mutation, len(s.mutations)), + dbErr: s.dbErr, refund: s.refund, + thash: s.thash, + txIndex: s.txIndex, logs: make(map[common.Hash][]*types.Log, len(s.logs)), logSize: s.logSize, preimages: maps.Clone(s.preimages), - journal: newJournal(), - hasher: crypto.NewKeccakState(), + journal: s.journal.copy(), + validRevisions: slices.Clone(s.validRevisions), + nextRevisionId: s.nextRevisionId, // In order for the block producer to be able to use and make additions // to the snapshot tree, we need to copy that as well. Otherwise, any @@ -718,39 +713,14 @@ func (s *StateDB) Copy() *StateDB { snaps: s.snaps, snap: s.snap, } - // Copy the dirty states, logs, and preimages - for addr := range s.journal.dirties { - // As documented [here](https://github.com/ethereum/go-ethereum/pull/16485#issuecomment-380438527), - // and in the Finalise-method, there is a case where an object is in the journal but not - // in the stateObjects: OOG after touch on ripeMD prior to Byzantium. Thus, we need to check for - // nil - if object, exist := s.stateObjects[addr]; exist { - // Even though the original object is dirty, we are not copying the journal, - // so we need to make sure that any side-effect the journal would have caused - // during a commit (or similar op) is already applied to the copy. - state.stateObjects[addr] = object.deepCopy(state) - - state.stateObjectsDirty[addr] = struct{}{} // Mark the copy dirty to force internal (code/state) commits - state.stateObjectsPending[addr] = struct{}{} // Mark the copy pending to force external (account) commits - } + // Deep copy cached state objects. + for addr, obj := range s.stateObjects { + state.stateObjects[addr] = obj.deepCopy(state) } - // Above, we don't copy the actual journal. This means that if the copy - // is copied, the loop above will be a no-op, since the copy's journal - // is empty. Thus, here we iterate over stateObjects, to enable copies - // of copies. - for addr := range s.stateObjectsPending { - if _, exist := state.stateObjects[addr]; !exist { - state.stateObjects[addr] = s.stateObjects[addr].deepCopy(state) - } - state.stateObjectsPending[addr] = struct{}{} + // Deep copy the object state markers. + for addr, op := range s.mutations { + state.mutations[addr] = op.copy() } - for addr := range s.stateObjectsDirty { - if _, exist := state.stateObjects[addr]; !exist { - state.stateObjects[addr] = s.stateObjects[addr].deepCopy(state) - } - state.stateObjectsDirty[addr] = struct{}{} - } - // Deep copy the logs occurred in the scope of block for hash, logs := range s.logs { cpy := make([]*types.Log, len(logs)) @@ -760,7 +730,6 @@ func (s *StateDB) Copy() *StateDB { } state.logs[hash] = cpy } - // Do we need to copy the access list and transient storage? // In practice: No. At the start of a transaction, these two lists are empty. // In practice, we only ever copy state _between_ transactions/blocks, never @@ -825,7 +794,8 @@ func (s *StateDB) Finalise(deleteEmptyObjects bool) { continue } if obj.selfDestructed || (deleteEmptyObjects && obj.empty()) { - obj.deleted = true + delete(s.stateObjects, obj.address) + s.markDelete(addr) // If ether was sent to account post-selfdestruct it is burnt. if bal := obj.Balance(); s.logger != nil && s.logger.OnBalanceChange != nil && obj.selfDestructed && bal.Sign() != 0 { @@ -846,11 +816,8 @@ func (s *StateDB) Finalise(deleteEmptyObjects bool) { delete(s.storagesOrigin, obj.address) // Clear out any previously updated storage data (may be recreated via a resurrect) } else { obj.finalise(true) // Prefetch slots in the background + s.markUpdate(addr) } - obj.created = false - s.stateObjectsPending[addr] = struct{}{} - s.stateObjectsDirty[addr] = struct{}{} - // At this point, also ship the address off to the precacher. The precacher // will start loading tries, and when the change is eventually committed, // the commit-phase will be a lot faster @@ -889,10 +856,14 @@ func (s *StateDB) IntermediateRoot(deleteEmptyObjects bool) common.Hash { // the account prefetcher. Instead, let's process all the storage updates // first, giving the account prefetches just a few more milliseconds of time // to pull useful data from disk. - for addr := range s.stateObjectsPending { - if obj := s.stateObjects[addr]; !obj.deleted { - obj.updateRoot() + for addr, op := range s.mutations { + if op.applied { + continue } + if op.isDelete() { + continue + } + s.stateObjects[addr].updateRoot() } // Now we're about to start to write changes to the trie. The trie is so far // _untouched_. We can check with the prefetcher, if it can give us a trie @@ -902,7 +873,6 @@ func (s *StateDB) IntermediateRoot(deleteEmptyObjects bool) common.Hash { s.trie = trie } } - usedAddrs := make([][]byte, 0, len(s.stateObjectsPending)) // Perform updates before deletions. This prevents resolution of unnecessary trie nodes // in circumstances similar to the following: // @@ -913,13 +883,21 @@ func (s *StateDB) IntermediateRoot(deleteEmptyObjects bool) common.Hash { // If the self-destruct is handled first, then `P` would be left with only one child, thus collapsed // into a shortnode. This requires `B` to be resolved from disk. // Whereas if the created node is handled first, then the collapse is avoided, and `B` is not resolved. - var deletedAddrs []common.Address - for addr := range s.stateObjectsPending { - if obj := s.stateObjects[addr]; !obj.deleted { - s.updateStateObject(obj) - s.AccountUpdated += 1 + var ( + usedAddrs [][]byte + deletedAddrs []common.Address + ) + for addr, op := range s.mutations { + if op.applied { + continue + } + op.applied = true + + if op.isDelete() { + deletedAddrs = append(deletedAddrs, addr) } else { - deletedAddrs = append(deletedAddrs, obj.address) + s.updateStateObject(s.stateObjects[addr]) + s.AccountUpdated += 1 } usedAddrs = append(usedAddrs, common.CopyBytes(addr[:])) // Copy needed for closure } @@ -930,9 +908,6 @@ func (s *StateDB) IntermediateRoot(deleteEmptyObjects bool) common.Hash { if prefetcher != nil { prefetcher.used(common.Hash{}, s.originalRoot, usedAddrs) } - if len(s.stateObjectsPending) > 0 { - s.stateObjectsPending = make(map[common.Address]struct{}) - } // Track the amount of time wasted on hashing the account trie defer func(start time.Time) { s.AccountHashes += time.Since(start) }(time.Now()) @@ -1176,11 +1151,12 @@ func (s *StateDB) Commit(block uint64, deleteEmptyObjects bool) (common.Hash, er return common.Hash{}, err } // Handle all state updates afterwards - for addr := range s.stateObjectsDirty { - obj := s.stateObjects[addr] - if obj.deleted { + for addr, op := range s.mutations { + if op.isDelete() { continue } + obj := s.stateObjects[addr] + // Write any contract code associated with the state object if obj.code != nil && obj.dirtyCode { rawdb.WriteCode(codeWriter, common.BytesToHash(obj.CodeHash()), obj.code) @@ -1280,7 +1256,7 @@ func (s *StateDB) Commit(block uint64, deleteEmptyObjects bool) (common.Hash, er s.storages = make(map[common.Hash]map[common.Hash][]byte) s.accountsOrigin = make(map[common.Address][]byte) s.storagesOrigin = make(map[common.Address]map[common.Hash][]byte) - s.stateObjectsDirty = make(map[common.Address]struct{}) + s.mutations = make(map[common.Address]*mutation) s.stateObjectsDestruct = make(map[common.Address]*types.StateAccount) return root, nil } @@ -1395,3 +1371,19 @@ func copy2DSet[k comparable](set map[k]map[common.Hash][]byte) map[k]map[common. } return copied } + +func (s *StateDB) markDelete(addr common.Address) { + if _, ok := s.mutations[addr]; !ok { + s.mutations[addr] = &mutation{} + } + s.mutations[addr].applied = false + s.mutations[addr].typ = deletion +} + +func (s *StateDB) markUpdate(addr common.Address) { + if _, ok := s.mutations[addr]; !ok { + s.mutations[addr] = &mutation{} + } + s.mutations[addr].applied = false + s.mutations[addr].typ = update +} diff --git a/core/state/statedb_fuzz_test.go b/core/state/statedb_fuzz_test.go index 65cf278108..6317681a7f 100644 --- a/core/state/statedb_fuzz_test.go +++ b/core/state/statedb_fuzz_test.go @@ -96,7 +96,9 @@ func newStateTestAction(addr common.Address, r *rand.Rand, index int) testAction { name: "CreateAccount", fn: func(a testAction, s *StateDB) { - s.CreateAccount(addr) + if !s.Exist(addr) { + s.CreateAccount(addr) + } }, }, { diff --git a/core/state/statedb_test.go b/core/state/statedb_test.go index edde6e8254..1a3eccfe10 100644 --- a/core/state/statedb_test.go +++ b/core/state/statedb_test.go @@ -225,6 +225,78 @@ func TestCopy(t *testing.T) { } } +// TestCopyWithDirtyJournal tests if Copy can correct create a equal copied +// stateDB with dirty journal present. +func TestCopyWithDirtyJournal(t *testing.T) { + db := NewDatabase(rawdb.NewMemoryDatabase()) + orig, _ := New(types.EmptyRootHash, db, nil) + + // Fill up the initial states + for i := byte(0); i < 255; i++ { + obj := orig.getOrNewStateObject(common.BytesToAddress([]byte{i})) + obj.AddBalance(uint256.NewInt(uint64(i)), tracing.BalanceChangeUnspecified) + obj.data.Root = common.HexToHash("0xdeadbeef") + orig.updateStateObject(obj) + } + root, _ := orig.Commit(0, true) + orig, _ = New(root, db, nil) + + // modify all in memory without finalizing + for i := byte(0); i < 255; i++ { + obj := orig.getOrNewStateObject(common.BytesToAddress([]byte{i})) + obj.SubBalance(uint256.NewInt(uint64(i)), tracing.BalanceChangeUnspecified) + orig.updateStateObject(obj) + } + cpy := orig.Copy() + + orig.Finalise(true) + for i := byte(0); i < 255; i++ { + root := orig.GetStorageRoot(common.BytesToAddress([]byte{i})) + if root != (common.Hash{}) { + t.Errorf("Unexpected storage root %x", root) + } + } + cpy.Finalise(true) + for i := byte(0); i < 255; i++ { + root := cpy.GetStorageRoot(common.BytesToAddress([]byte{i})) + if root != (common.Hash{}) { + t.Errorf("Unexpected storage root %x", root) + } + } + if cpy.IntermediateRoot(true) != orig.IntermediateRoot(true) { + t.Error("State is not equal after copy") + } +} + +// TestCopyObjectState creates an original state, S1, and makes a copy S2. +// It then proceeds to make changes to S1. Those changes are _not_ supposed +// to affect S2. This test checks that the copy properly deep-copies the objectstate +func TestCopyObjectState(t *testing.T) { + db := NewDatabase(rawdb.NewMemoryDatabase()) + orig, _ := New(types.EmptyRootHash, db, nil) + + // Fill up the initial states + for i := byte(0); i < 5; i++ { + obj := orig.getOrNewStateObject(common.BytesToAddress([]byte{i})) + obj.AddBalance(uint256.NewInt(uint64(i)), tracing.BalanceChangeUnspecified) + obj.data.Root = common.HexToHash("0xdeadbeef") + orig.updateStateObject(obj) + } + orig.Finalise(true) + cpy := orig.Copy() + for _, op := range cpy.mutations { + if have, want := op.applied, false; have != want { + t.Fatalf("Error in test itself, the 'done' flag should not be set before Commit, have %v want %v", have, want) + } + } + orig.Commit(0, true) + for _, op := range cpy.mutations { + if have, want := op.applied, false; have != want { + t.Fatalf("Error: original state affected copy, have %v want %v", have, want) + } + } +} + func TestSnapshotRandom(t *testing.T) { config := &quick.Config{MaxCount: 1000} err := quick.Check((*snapshotTest).run, config) @@ -308,7 +380,30 @@ func newTestAction(addr common.Address, r *rand.Rand) testAction { { name: "CreateAccount", fn: func(a testAction, s *StateDB) { - s.CreateAccount(addr) + if !s.Exist(addr) { + s.CreateAccount(addr) + } + }, + }, + { + name: "CreateContract", + fn: func(a testAction, s *StateDB) { + if !s.Exist(addr) { + s.CreateAccount(addr) + } + contractHash := s.GetCodeHash(addr) + emptyCode := contractHash == (common.Hash{}) || contractHash == types.EmptyCodeHash + storageRoot := s.GetStorageRoot(addr) + emptyStorage := storageRoot == (common.Hash{}) || storageRoot == types.EmptyRootHash + if s.GetNonce(addr) == 0 && emptyCode && emptyStorage { + s.CreateContract(addr) + // We also set some code here, to prevent the + // CreateContract action from being performed twice in a row, + // which would cause a difference in state when unrolling + // the journal. (CreateContact assumes created was false prior to + // invocation, and the journal rollback sets it to false). + s.SetCode(addr, []byte{1}) + } }, }, { @@ -709,18 +804,19 @@ func TestCopyCopyCommitCopy(t *testing.T) { } } -// TestCommitCopy tests the copy from a committed state is not functional. +// TestCommitCopy tests the copy from a committed state is not fully functional. func TestCommitCopy(t *testing.T) { - state, _ := New(types.EmptyRootHash, NewDatabase(rawdb.NewMemoryDatabase()), nil) + db := NewDatabase(rawdb.NewMemoryDatabase()) + state, _ := New(types.EmptyRootHash, db, nil) // Create an account and check if the retrieved balance is correct addr := common.HexToAddress("0xaffeaffeaffeaffeaffeaffeaffeaffeaffeaffe") - skey := common.HexToHash("aaa") - sval := common.HexToHash("bbb") + skey1, skey2 := common.HexToHash("a1"), common.HexToHash("a2") + sval1, sval2 := common.HexToHash("b1"), common.HexToHash("b2") state.SetBalance(addr, uint256.NewInt(42), tracing.BalanceChangeUnspecified) // Change the account trie state.SetCode(addr, []byte("hello")) // Change an external metadata - state.SetState(addr, skey, sval) // Change the storage trie + state.SetState(addr, skey1, sval1) // Change the storage trie if balance := state.GetBalance(addr); balance.Cmp(uint256.NewInt(42)) != 0 { t.Fatalf("initial balance mismatch: have %v, want %v", balance, 42) @@ -728,25 +824,38 @@ func TestCommitCopy(t *testing.T) { if code := state.GetCode(addr); !bytes.Equal(code, []byte("hello")) { t.Fatalf("initial code mismatch: have %x, want %x", code, []byte("hello")) } - if val := state.GetState(addr, skey); val != sval { - t.Fatalf("initial non-committed storage slot mismatch: have %x, want %x", val, sval) + if val := state.GetState(addr, skey1); val != sval1 { + t.Fatalf("initial non-committed storage slot mismatch: have %x, want %x", val, sval1) } - if val := state.GetCommittedState(addr, skey); val != (common.Hash{}) { + if val := state.GetCommittedState(addr, skey1); val != (common.Hash{}) { t.Fatalf("initial committed storage slot mismatch: have %x, want %x", val, common.Hash{}) } - // Copy the committed state database, the copied one is not functional. - state.Commit(0, true) + root, _ := state.Commit(0, true) + + state, _ = New(root, db, nil) + state.SetState(addr, skey2, sval2) + state.Commit(1, true) + + // Copy the committed state database, the copied one is not fully functional. copied := state.Copy() - if balance := copied.GetBalance(addr); balance.Cmp(uint256.NewInt(0)) != 0 { + if balance := copied.GetBalance(addr); balance.Cmp(uint256.NewInt(42)) != 0 { t.Fatalf("unexpected balance: have %v", balance) } - if code := copied.GetCode(addr); code != nil { + if code := copied.GetCode(addr); !bytes.Equal(code, []byte("hello")) { t.Fatalf("unexpected code: have %x", code) } - if val := copied.GetState(addr, skey); val != (common.Hash{}) { + // Miss slots because of non-functional trie after commit + if val := copied.GetState(addr, skey1); val != (common.Hash{}) { + t.Fatalf("unexpected storage slot: have %x", sval1) + } + if val := copied.GetCommittedState(addr, skey1); val != (common.Hash{}) { t.Fatalf("unexpected storage slot: have %x", val) } - if val := copied.GetCommittedState(addr, skey); val != (common.Hash{}) { + // Slots cached in the stateDB, available after commit + if val := copied.GetState(addr, skey2); val != sval2 { + t.Fatalf("unexpected storage slot: have %x", sval1) + } + if val := copied.GetCommittedState(addr, skey2); val != sval2 { t.Fatalf("unexpected storage slot: have %x", val) } if !errors.Is(copied.Error(), trie.ErrCommitted) { @@ -1103,40 +1212,6 @@ func TestStateDBTransientStorage(t *testing.T) { } } -func TestResetObject(t *testing.T) { - var ( - disk = rawdb.NewMemoryDatabase() - tdb = triedb.NewDatabase(disk, nil) - db = NewDatabaseWithNodeDB(disk, tdb) - snaps, _ = snapshot.New(snapshot.Config{CacheSize: 10}, disk, tdb, types.EmptyRootHash) - state, _ = New(types.EmptyRootHash, db, snaps) - addr = common.HexToAddress("0x1") - slotA = common.HexToHash("0x1") - slotB = common.HexToHash("0x2") - ) - // Initialize account with balance and storage in first transaction. - state.SetBalance(addr, uint256.NewInt(1), tracing.BalanceChangeUnspecified) - state.SetState(addr, slotA, common.BytesToHash([]byte{0x1})) - state.IntermediateRoot(true) - - // Reset account and mutate balance and storages - state.CreateAccount(addr) - state.SetBalance(addr, uint256.NewInt(2), tracing.BalanceChangeUnspecified) - state.SetState(addr, slotB, common.BytesToHash([]byte{0x2})) - root, _ := state.Commit(0, true) - - // Ensure the original account is wiped properly - snap := snaps.Snapshot(root) - slot, _ := snap.Storage(crypto.Keccak256Hash(addr.Bytes()), crypto.Keccak256Hash(slotA.Bytes())) - if len(slot) != 0 { - t.Fatalf("Unexpected storage slot") - } - slot, _ = snap.Storage(crypto.Keccak256Hash(addr.Bytes()), crypto.Keccak256Hash(slotB.Bytes())) - if !bytes.Equal(slot, []byte{0x2}) { - t.Fatalf("Unexpected storage slot value %v", slot) - } -} - func TestDeleteStorage(t *testing.T) { var ( disk = rawdb.NewMemoryDatabase() diff --git a/core/vm/evm.go b/core/vm/evm.go index 045506a1fd..c18353a973 100644 --- a/core/vm/evm.go +++ b/core/vm/evm.go @@ -436,14 +436,15 @@ 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) } // Ensure there's no existing contract already at the designated address. // Account is regarded as existent if any of these three conditions is met: - // - the nonce is nonzero + // - the nonce is non-zero // - the code is non-empty // - the storage is non-empty contractHash := evm.StateDB.GetCodeHash(address) @@ -456,9 +457,19 @@ func (evm *EVM) create(caller ContractRef, codeAndHash *codeAndHash, gas uint64, } 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. snapshot := evm.StateDB.Snapshot() - evm.StateDB.CreateAccount(address) + if !evm.StateDB.Exist(address) { + evm.StateDB.CreateAccount(address) + } + // CreateContract means that regardless of whether the account previously existed + // in the state trie or not, it _now_ becomes created as a _contract_ account. + // This is performed _prior_ to executing the initcode, since the initcode + // acts inside that account. + evm.StateDB.CreateContract(address) + if evm.chainRules.IsEIP158 { evm.StateDB.SetNonce(address, 1) } diff --git a/core/vm/interface.go b/core/vm/interface.go index 30742e96de..774360a08e 100644 --- a/core/vm/interface.go +++ b/core/vm/interface.go @@ -29,6 +29,7 @@ import ( // StateDB is an EVM database for full state querying. type StateDB interface { CreateAccount(common.Address) + CreateContract(common.Address) SubBalance(common.Address, *uint256.Int, tracing.BalanceChangeReason) AddBalance(common.Address, *uint256.Int, tracing.BalanceChangeReason) diff --git a/tests/block_test.go b/tests/block_test.go index 43e3d99b3e..1ba84f5f24 100644 --- a/tests/block_test.go +++ b/tests/block_test.go @@ -64,14 +64,6 @@ 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) }) From ac21f9bfb5154854e241928840b3978d801eac8c Mon Sep 17 00:00:00 2001 From: qcrao Date: Wed, 24 Apr 2024 20:04:20 +0800 Subject: [PATCH 2/4] trie: preallocate capacity for fields slice (#29614) trie: Preallocate capacity for fields slice --- trie/trienode/node.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/trie/trienode/node.go b/trie/trienode/node.go index 95315c2e9a..055db8822e 100644 --- a/trie/trienode/node.go +++ b/trie/trienode/node.go @@ -78,7 +78,7 @@ func NewNodeSet(owner common.Hash) *NodeSet { // ForEachWithOrder iterates the nodes with the order from bottom to top, // right to left, nodes with the longest path will be iterated first. func (set *NodeSet) ForEachWithOrder(callback func(path string, n *Node)) { - var paths []string + paths := make([]string, 0, len(set.Nodes)) for path := range set.Nodes { paths = append(paths, path) } @@ -133,7 +133,7 @@ func (set *NodeSet) Size() (int, int) { // Hashes returns the hashes of all updated nodes. TODO(rjl493456442) how can // we get rid of it? func (set *NodeSet) Hashes() []common.Hash { - var ret []common.Hash + ret := make([]common.Hash, 0, len(set.Nodes)) for _, node := range set.Nodes { ret = append(ret, node.Hash) } From 7362691479df9eb7d76c8b968ed8f440372fab6e Mon Sep 17 00:00:00 2001 From: Aaron Chen Date: Wed, 24 Apr 2024 20:27:58 +0800 Subject: [PATCH 3/4] trie, consensus/clique: use maps.Clone (#29616) --- consensus/clique/snapshot.go | 23 ++++++----------------- trie/tracer.go | 18 +++++------------- 2 files changed, 11 insertions(+), 30 deletions(-) diff --git a/consensus/clique/snapshot.go b/consensus/clique/snapshot.go index 8ff0b3a70f..d0b15e9489 100644 --- a/consensus/clique/snapshot.go +++ b/consensus/clique/snapshot.go @@ -19,6 +19,7 @@ package clique import ( "bytes" "encoding/json" + "maps" "slices" "time" @@ -108,28 +109,16 @@ func (s *Snapshot) store(db ethdb.Database) error { // copy creates a deep copy of the snapshot, though not the individual votes. func (s *Snapshot) copy() *Snapshot { - cpy := &Snapshot{ + return &Snapshot{ config: s.config, sigcache: s.sigcache, Number: s.Number, Hash: s.Hash, - Signers: make(map[common.Address]struct{}), - Recents: make(map[uint64]common.Address), - Votes: make([]*Vote, len(s.Votes)), - Tally: make(map[common.Address]Tally), + Signers: maps.Clone(s.Signers), + Recents: maps.Clone(s.Recents), + Votes: slices.Clone(s.Votes), + Tally: maps.Clone(s.Tally), } - for signer := range s.Signers { - cpy.Signers[signer] = struct{}{} - } - for block, signer := range s.Recents { - cpy.Recents[block] = signer - } - for address, tally := range s.Tally { - cpy.Tally[address] = tally - } - copy(cpy.Votes, s.Votes) - - return cpy } // validVote returns whether it makes sense to cast the specified vote in the diff --git a/trie/tracer.go b/trie/tracer.go index 5786af4d3e..90b9666f0b 100644 --- a/trie/tracer.go +++ b/trie/tracer.go @@ -17,6 +17,8 @@ package trie import ( + "maps" + "github.com/ethereum/go-ethereum/common" ) @@ -92,23 +94,13 @@ func (t *tracer) reset() { // copy returns a deep copied tracer instance. func (t *tracer) copy() *tracer { - var ( - inserts = make(map[string]struct{}) - deletes = make(map[string]struct{}) - accessList = make(map[string][]byte) - ) - for path := range t.inserts { - inserts[path] = struct{}{} - } - for path := range t.deletes { - deletes[path] = struct{}{} - } + accessList := make(map[string][]byte, len(t.accessList)) for path, blob := range t.accessList { accessList[path] = common.CopyBytes(blob) } return &tracer{ - inserts: inserts, - deletes: deletes, + inserts: maps.Clone(t.inserts), + deletes: maps.Clone(t.deletes), accessList: accessList, } } From 4f4f9d88d3a59e972c9fe3a5e6e6a93dad2e7db4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A9ter=20Szil=C3=A1gyi?= Date: Wed, 24 Apr 2024 18:45:24 +0300 Subject: [PATCH 4/4] core/state: storage journal entry should revert dirtyness too (#29641) Currently our state journal tracks each storage update to a contract, having the ability to revert those changes to the previously set value. For the very first modification however, it behaves a bit wonky. Reverting the update doesn't actually remove the dirty-ness of the slot, rather leaves it as "change this slot to it's original value". This can cause issues down the line with for example write witnesses needing to gather an unneeded proof. This PR modifies the storageChange journal entry to not only track the previous value of a slot, but also whether there was any previous value at all set in the current execution context. In essence, the PR changes the semantic of storageChange so it does not simply track storage changes, rather it tracks dirty storage changes, an important distinction for being able to cleanly revert the journal item. --- core/state/journal.go | 13 +++++++------ core/state/state_object.go | 40 ++++++++++++++++++++++++++++---------- 2 files changed, 37 insertions(+), 16 deletions(-) diff --git a/core/state/journal.go b/core/state/journal.go index cfd3782eb0..c0f5615c98 100644 --- a/core/state/journal.go +++ b/core/state/journal.go @@ -129,8 +129,9 @@ type ( prev uint64 } storageChange struct { - account *common.Address - key, prevalue common.Hash + account *common.Address + key common.Hash + prevvalue *common.Hash } codeChange struct { account *common.Address @@ -277,7 +278,7 @@ func (ch codeChange) copy() journalEntry { } func (ch storageChange) revert(s *StateDB) { - s.getStateObject(*ch.account).setState(ch.key, ch.prevalue) + s.getStateObject(*ch.account).setState(ch.key, ch.prevvalue) } func (ch storageChange) dirtied() *common.Address { @@ -286,9 +287,9 @@ func (ch storageChange) dirtied() *common.Address { func (ch storageChange) copy() journalEntry { return storageChange{ - account: ch.account, - key: ch.key, - prevalue: ch.prevalue, + account: ch.account, + key: ch.key, + prevvalue: ch.prevvalue, } } diff --git a/core/state/state_object.go b/core/state/state_object.go index 8978b30542..aa748f08ac 100644 --- a/core/state/state_object.go +++ b/core/state/state_object.go @@ -140,13 +140,20 @@ func (s *stateObject) getTrie() (Trie, error) { // GetState retrieves a value from the account storage trie. func (s *stateObject) GetState(key common.Hash) common.Hash { + value, _ := s.getState(key) + return value +} + +// getState retrieves a value from the account storage trie and also returns if +// the slot is already dirty or not. +func (s *stateObject) getState(key common.Hash) (common.Hash, bool) { // If we have a dirty value for this state entry, return it value, dirty := s.dirtyStorage[key] if dirty { - return value + return value, true } // Otherwise return the entry's original value - return s.GetCommittedState(key) + return s.GetCommittedState(key), false } // GetCommittedState retrieves a value from the committed account storage trie. @@ -209,25 +216,38 @@ func (s *stateObject) GetCommittedState(key common.Hash) common.Hash { // SetState updates a value in account storage. func (s *stateObject) SetState(key, value common.Hash) { - // If the new value is the same as old, don't set - prev := s.GetState(key) + // If the new value is the same as old, don't set. Otherwise, track only the + // dirty changes, supporting reverting all of it back to no change. + prev, dirty := s.getState(key) if prev == value { return } + var prevvalue *common.Hash + if dirty { + prevvalue = &prev + } // New value is different, update and journal the change s.db.journal.append(storageChange{ - account: &s.address, - key: key, - prevalue: prev, + account: &s.address, + key: key, + prevvalue: prevvalue, }) if s.db.logger != nil && s.db.logger.OnStorageChange != nil { s.db.logger.OnStorageChange(s.address, key, prev, value) } - s.setState(key, value) + s.setState(key, &value) } -func (s *stateObject) setState(key, value common.Hash) { - s.dirtyStorage[key] = value +// setState updates a value in account dirty storage. If the value being set is +// nil (assuming journal revert), the dirtyness is removed. +func (s *stateObject) setState(key common.Hash, value *common.Hash) { + // If the first set is being reverted, undo the dirty marker + if value == nil { + delete(s.dirtyStorage, key) + return + } + // Otherwise restore the previous value + s.dirtyStorage[key] = *value } // finalise moves all dirty storage slots into the pending area to be hashed or