diff --git a/core/state/state_object.go b/core/state/state_object.go index 8680de021f..e354cdf208 100644 --- a/core/state/state_object.go +++ b/core/state/state_object.go @@ -79,10 +79,9 @@ type stateObject struct { trie Trie // storage trie, which becomes non-nil on first access code Code // contract bytecode, which gets set when code is loaded - originStorage Storage // Storage cache of original entries to dedup rewrites, reset for every transaction - pendingStorage Storage // Storage entries that need to be flushed to disk, at the end of an entire block - dirtyStorage Storage // Storage entries that have been modified in the current transaction execution - fakeStorage Storage // Fake storage which constructed by caller for debugging purpose. + originStorage Storage // Storage cache of original entries to dedup rewrites, reset for every transaction + dirtyStorage Storage // Storage entries that have been modified since the last intermediate root or commit + fakeStorage Storage // Fake storage which constructed by caller for debugging purpose. // Cache flags. // When an object is marked suicided it will be delete from the trie @@ -118,13 +117,12 @@ func newObject(db *StateDB, address common.Address, data Account) *stateObject { data.Root = emptyRoot } return &stateObject{ - db: db, - address: address, - addrHash: crypto.Keccak256Hash(address[:]), - data: data, - originStorage: make(Storage), - pendingStorage: make(Storage), - dirtyStorage: make(Storage), + db: db, + address: address, + addrHash: crypto.Keccak256Hash(address[:]), + data: data, + originStorage: make(Storage), + dirtyStorage: make(Storage), } } @@ -189,9 +187,6 @@ func (s *stateObject) GetCommittedState(db Database, key common.Hash) common.Has return s.fakeStorage[key] } // If we have a pending write or clean cached, return that - if value, pending := s.pendingStorage[key]; pending { - return value - } if value, cached := s.originStorage[key]; cached { return value } @@ -260,29 +255,15 @@ func (s *stateObject) setState(key, value common.Hash) { s.dirtyStorage[key] = value } -// finalise moves all dirty storage slots into the pending area to be hashed or -// committed later. It is invoked at the end of every transaction. -func (s *stateObject) finalise() { - for key, value := range s.dirtyStorage { - s.pendingStorage[key] = value - } - if len(s.dirtyStorage) > 0 { - s.dirtyStorage = make(Storage) - } -} - // updateTrie writes cached storage modifications into the object's storage trie. func (s *stateObject) updateTrie(db Database) Trie { - // Make sure all dirty slots are finalized into the pending storage area - s.finalise() - // Track the amount of time wasted on updating the storge trie if metrics.EnabledExpensive { defer func(start time.Time) { s.db.StorageUpdates += time.Since(start) }(time.Now()) } // Insert all the pending updates into the trie tr := s.getTrie(db) - for key, value := range s.pendingStorage { + for key, value := range s.dirtyStorage { // Skip noop changes, persist actual changes if value == s.originStorage[key] { continue @@ -297,8 +278,8 @@ func (s *stateObject) updateTrie(db Database) Trie { v, _ := rlp.EncodeToBytes(common.TrimLeftZeroes(value[:])) s.setError(tr.TryUpdate(key[:], v)) } - if len(s.pendingStorage) > 0 { - s.pendingStorage = make(Storage) + if len(s.dirtyStorage) > 0 { + s.dirtyStorage = make(Storage) } return tr } @@ -379,7 +360,6 @@ func (s *stateObject) deepCopy(db *StateDB) *stateObject { stateObject.code = s.code stateObject.dirtyStorage = s.dirtyStorage.Copy() stateObject.originStorage = s.originStorage.Copy() - stateObject.pendingStorage = s.pendingStorage.Copy() stateObject.suicided = s.suicided stateObject.dirtyCode = s.dirtyCode stateObject.deleted = s.deleted diff --git a/core/state/statedb.go b/core/state/statedb.go index 9034d55336..ec239ca1ac 100644 --- a/core/state/statedb.go +++ b/core/state/statedb.go @@ -588,8 +588,13 @@ func (self *StateDB) Copy() *StateDB { // in the stateObjects: OOG after touch on ripeMD prior to Byzantium. Thus, we need to check for // nil if object, exist := self.stateObjects[addr]; exist { + // Even though the original object is dirty, we are not copying the journal, + // so we need to make sure that anyside 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{}{} + + 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 } } // Above, we don't copy the actual journal. This means that if the copy is copied, the @@ -667,8 +672,6 @@ func (s *StateDB) Finalise(deleteEmptyObjects bool) { } if obj.suicided || (deleteEmptyObjects && obj.empty()) { obj.deleted = true - } else { - obj.finalise() } s.stateObjectsPending[addr] = struct{}{} s.stateObjectsDirty[addr] = struct{}{} diff --git a/core/state/statedb_test.go b/core/state/statedb_test.go index bb3b87ead2..145efbc893 100644 --- a/core/state/statedb_test.go +++ b/core/state/statedb_test.go @@ -438,18 +438,149 @@ func (s *StateSuite) TestTouchDelete(c *check.C) { // TestCopyOfCopy tests that modified objects are carried over to the copy, and the copy of the copy. // See https://github.com/ethereum/go-ethereum/pull/15225#issuecomment-380191512 func TestCopyOfCopy(t *testing.T) { - sdb, _ := New(common.Hash{}, NewDatabase(rawdb.NewMemoryDatabase())) + state, _ := New(common.Hash{}, NewDatabase(rawdb.NewMemoryDatabase())) addr := common.HexToAddress("aaaa") - sdb.SetBalance(addr, big.NewInt(42)) + state.SetBalance(addr, big.NewInt(42)) - if got := sdb.Copy().GetBalance(addr).Uint64(); got != 42 { + if got := state.Copy().GetBalance(addr).Uint64(); got != 42 { t.Fatalf("1st copy fail, expected 42, got %v", got) } - if got := sdb.Copy().Copy().GetBalance(addr).Uint64(); got != 42 { + if got := state.Copy().Copy().GetBalance(addr).Uint64(); got != 42 { t.Fatalf("2nd copy fail, expected 42, got %v", got) } } +// Tests a regression where committing a copy lost some internal meta information, +// leading to corrupted subsequent copies. +// +// See https://github.com/ethereum/go-ethereum/issues/20106. +func TestCopyCommitCopy(t *testing.T) { + state, _ := New(common.Hash{}, NewDatabase(rawdb.NewMemoryDatabase())) + + // Create an account and check if the retrieved balance is correct + addr := common.HexToAddress("0xaffeaffeaffeaffeaffeaffeaffeaffeaffeaffe") + skey := common.HexToHash("aaa") + sval := common.HexToHash("bbb") + + state.SetBalance(addr, big.NewInt(42)) // Change the account trie + state.SetCode(addr, []byte("hello")) // Change an external metadata + state.SetState(addr, skey, sval) // Change the storage trie + + if balance := state.GetBalance(addr); balance.Cmp(big.NewInt(42)) != 0 { + t.Fatalf("initial balance mismatch: have %v, want %v", balance, 42) + } + 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 storage slot mismatch: have %x, want %x", val, sval) + } + // Copy the non-committed state database and check pre/post commit balance + copyOne := state.Copy() + if balance := copyOne.GetBalance(addr); balance.Cmp(big.NewInt(42)) != 0 { + t.Fatalf("first copy pre-commit balance mismatch: have %v, want %v", balance, 42) + } + if code := copyOne.GetCode(addr); !bytes.Equal(code, []byte("hello")) { + t.Fatalf("first copy pre-commit code mismatch: have %x, want %x", code, []byte("hello")) + } + if val := copyOne.GetState(addr, skey); val != sval { + t.Fatalf("first copy pre-commit storage slot mismatch: have %x, want %x", val, sval) + } + + copyOne.Commit(false) + if balance := copyOne.GetBalance(addr); balance.Cmp(big.NewInt(42)) != 0 { + t.Fatalf("first copy post-commit balance mismatch: have %v, want %v", balance, 42) + } + if code := copyOne.GetCode(addr); !bytes.Equal(code, []byte("hello")) { + t.Fatalf("first copy post-commit code mismatch: have %x, want %x", code, []byte("hello")) + } + if val := copyOne.GetState(addr, skey); val != sval { + t.Fatalf("first copy post-commit storage slot mismatch: have %x, want %x", val, sval) + } + // Copy the copy and check the balance once more + copyTwo := copyOne.Copy() + if balance := copyTwo.GetBalance(addr); balance.Cmp(big.NewInt(42)) != 0 { + t.Fatalf("second copy balance mismatch: have %v, want %v", balance, 42) + } + if code := copyTwo.GetCode(addr); !bytes.Equal(code, []byte("hello")) { + t.Fatalf("second copy code mismatch: have %x, want %x", code, []byte("hello")) + } + if val := copyTwo.GetState(addr, skey); val != sval { + t.Fatalf("second copy storage slot mismatch: have %x, want %x", val, sval) + } +} + +// Tests a regression where committing a copy lost some internal meta information, +// leading to corrupted subsequent copies. +// +// See https://github.com/ethereum/go-ethereum/issues/20106. +func TestCopyCopyCommitCopy(t *testing.T) { + state, _ := New(common.Hash{}, NewDatabase(rawdb.NewMemoryDatabase())) + + // Create an account and check if the retrieved balance is correct + addr := common.HexToAddress("0xaffeaffeaffeaffeaffeaffeaffeaffeaffeaffe") + skey := common.HexToHash("aaa") + sval := common.HexToHash("bbb") + + state.SetBalance(addr, big.NewInt(42)) // Change the account trie + state.SetCode(addr, []byte("hello")) // Change an external metadata + state.SetState(addr, skey, sval) // Change the storage trie + + if balance := state.GetBalance(addr); balance.Cmp(big.NewInt(42)) != 0 { + t.Fatalf("initial balance mismatch: have %v, want %v", balance, 42) + } + 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 storage slot mismatch: have %x, want %x", val, sval) + } + // Copy the non-committed state database and check pre/post commit balance + copyOne := state.Copy() + if balance := copyOne.GetBalance(addr); balance.Cmp(big.NewInt(42)) != 0 { + t.Fatalf("first copy balance mismatch: have %v, want %v", balance, 42) + } + if code := copyOne.GetCode(addr); !bytes.Equal(code, []byte("hello")) { + t.Fatalf("first copy code mismatch: have %x, want %x", code, []byte("hello")) + } + if val := copyOne.GetState(addr, skey); val != sval { + t.Fatalf("first copy storage slot mismatch: have %x, want %x", val, sval) + } + // Copy the copy and check the balance once more + copyTwo := copyOne.Copy() + if balance := copyTwo.GetBalance(addr); balance.Cmp(big.NewInt(42)) != 0 { + t.Fatalf("second copy pre-commit balance mismatch: have %v, want %v", balance, 42) + } + if code := copyTwo.GetCode(addr); !bytes.Equal(code, []byte("hello")) { + t.Fatalf("second copy pre-commit code mismatch: have %x, want %x", code, []byte("hello")) + } + if val := copyTwo.GetState(addr, skey); val != sval { + t.Fatalf("second copy pre-commit storage slot mismatch: have %x, want %x", val, sval) + } + + copyTwo.Commit(false) + if balance := copyTwo.GetBalance(addr); balance.Cmp(big.NewInt(42)) != 0 { + t.Fatalf("second copy post-commit balance mismatch: have %v, want %v", balance, 42) + } + if code := copyTwo.GetCode(addr); !bytes.Equal(code, []byte("hello")) { + t.Fatalf("second copy post-commit code mismatch: have %x, want %x", code, []byte("hello")) + } + if val := copyTwo.GetState(addr, skey); val != sval { + t.Fatalf("second copy post-commit storage slot mismatch: have %x, want %x", val, sval) + } + // Copy the copy-copy and check the balance once more + copyThree := copyTwo.Copy() + if balance := copyThree.GetBalance(addr); balance.Cmp(big.NewInt(42)) != 0 { + t.Fatalf("third copy balance mismatch: have %v, want %v", balance, 42) + } + if code := copyThree.GetCode(addr); !bytes.Equal(code, []byte("hello")) { + t.Fatalf("third copy code mismatch: have %x, want %x", code, []byte("hello")) + } + if val := copyThree.GetState(addr, skey); val != sval { + t.Fatalf("third copy storage slot mismatch: have %x, want %x", val, sval) + } +} + // TestDeleteCreateRevert tests a weird state transition corner case that we hit // while changing the internals of statedb. The workflow is that a contract is // self destructed, then in a followup transaction (but same block) it's created