core/state: handle no-op account trie updates better

core/state/state_object.go

@@ -293,28 +293,22 @@ func (s *stateObject) finalise() {
 
 // updateTrie writes cached storage modifications into the object's storage trie.
 // It will return nil if the trie has not been loaded and no changes have been made
+// Note: It may return non-nil if the trie is already loaded due to previous changes
+// in the same block
 func (s *stateObject) updateTrie(db Database) Trie {
 	// Make sure all dirty slots are finalized into the pending storage area
 	s.finalise()
 	if len(s.pendingStorage) == 0 {
 		return s.trie
 	}
+	// Insert all the pending updates into the trie
+	tr := s.getTrie(db)
 	// 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())
 	}
 	// Retrieve the snapshot storage map for the object
 	var storage map[common.Hash][]byte
-	if s.db.snap != nil {
-		// Retrieve the old storage map, if available, create a new one otherwise
-		storage = s.db.snapStorage[s.addrHash]
-		if storage == nil {
-			storage = make(map[common.Hash][]byte)
-			s.db.snapStorage[s.addrHash] = storage
-		}
-	}
-	// Insert all the pending updates into the trie
-	tr := s.getTrie(db)
 	for key, value := range s.pendingStorage {
 		// Skip noop changes, persist actual changes
 		if value == s.originStorage[key] {
@@ -331,7 +325,23 @@ func (s *stateObject) updateTrie(db Database) Trie {
 			s.setError(tr.TryUpdate(key[:], v))
 		}
 		// If state snapshotting is active, cache the data til commit
-		if storage != nil {
+		if s.db.snap != nil {
+			// lazy load storage
+			if storage == nil {
+				// Retrieve the old storage map, if available
+				s.db.snapLock.RLock()
+				storage = s.db.snapStorage[s.addrHash]
+				s.db.snapLock.RUnlock()
+
+				// If no old storage map was available, create a new one
+				if storage == nil {
+					storage = make(map[common.Hash][]byte)
+
+					s.db.snapLock.Lock()
+					s.db.snapStorage[s.addrHash] = storage
+					s.db.snapLock.Unlock()
+				}
+			}
 			storage[crypto.Keccak256Hash(key[:])] = v // v will be nil if value is 0x00
 		}
 	}
@@ -341,7 +351,7 @@ func (s *stateObject) updateTrie(db Database) Trie {
 	return tr
 }
 
-// UpdateRoot sets the trie root to the current root hash of
+// UpdateRoot sets the trie root to the current root hash of the storage
 func (s *stateObject) updateRoot(db Database) {
 	// If nothing changed, don't bother with hashing anything
 	if s.updateTrie(db) == nil {
@@ -352,6 +362,7 @@ func (s *stateObject) updateRoot(db Database) {
 		defer func(start time.Time) { s.db.StorageHashes += time.Since(start) }(time.Now())
 	}
 	s.data.Root = s.trie.Hash()
+	return
 }
 
 // CommitTrie the storage trie of the object to db.

core/state/statedb.go

@@ -359,6 +359,7 @@ func (s *StateDB) StorageTrie(addr common.Address) Trie {
 	}
 	cpy := stateObject.deepCopy(s)
 	cpy.updateTrie(s.db)
+	// Explicitly load the trie
 	return cpy.getTrie(s.db)
 }
 
@@ -806,7 +807,6 @@ func (s *StateDB) clearJournalAndRefund() {
 func (s *StateDB) Commit(deleteEmptyObjects bool) (common.Hash, error) {
 	// Finalize any pending changes and merge everything into the tries
 	s.IntermediateRoot(deleteEmptyObjects)
-
 	// Commit objects to the trie, measuring the elapsed time
 	for addr := range s.stateObjectsDirty {
 		if obj := s.stateObjects[addr]; !obj.deleted {

core/state/statedb_test.go

@@ -680,3 +680,58 @@ func TestDeleteCreateRevert(t *testing.T) {
 		t.Fatalf("self-destructed contract came alive")
 	}
 }
+
+func TestPlainTransferCommit(t *testing.T) {
+	// Create an initial state with a single contract and a single sender
+	state, _ := New(common.Hash{}, NewDatabase(rawdb.NewMemoryDatabase()), nil)
+
+	// Create three contracts with non-empty root hash
+	// contract1 -- only balance change
+	// contract2 -- change to a slot
+	// contract3 -- temporary change to slot, which is changed back again
+	// EOA - plain account without storage
+
+	addrC1 := toAddr([]byte("contract1"))
+	state.SetBalance(addrC1, big.NewInt(0))
+	state.SetState(addrC1, common.Hash{0x1}, common.Hash{0x1})
+
+	addrC2 := toAddr([]byte("contract2"))
+	state.SetBalance(addrC2, big.NewInt(0))
+	state.SetState(addrC2, common.Hash{0x1}, common.Hash{0x2})
+
+	addrC3 := toAddr([]byte("contract3"))
+	state.SetBalance(addrC3, big.NewInt(0))
+	state.SetState(addrC3, common.Hash{0x1}, common.Hash{0x3})
+
+	// Create EOA
+	addrEOA := toAddr([]byte("plain"))
+	state.SetBalance(addrEOA, big.NewInt(1))
+	// Make sure none of them are empty
+	state.SetNonce(addrC1, 1)
+	state.SetNonce(addrC2, 1)
+	state.SetNonce(addrC3, 1)
+
+	root, _ := state.Commit(false)
+	state.Reset(root)
+	// Simulate sending from user to contract
+	state.SetNonce(addrEOA, 1)
+	state.SetBalance(addrEOA, new(big.Int))
+	// contract1 gets some ether (no slot changed)
+	state.SetBalance(addrC1, big.NewInt(1))
+	// contract2 has a slot changed, updating the storage root
+	state.SetState(addrC2, common.Hash{0x1}, common.Hash{0xFF})
+
+	// contract3 has a slot changed, and changed back again
+	state.SetState(addrC3, common.Hash{0x1}, common.Hash{0x4})
+	state.SetState(addrC3, common.Hash{0x1}, common.Hash{0x3})
+
+	// Tx done, now Finalise
+	state.Finalise(true)
+	// Block done, Commit
+	root, _ = state.Commit(true)
+	// adf3478f2e81a39f5e24413e13673502a75ab8c4675fd2691ca1f442877e2a25
+	exp := common.HexToHash("adf3478f2e81a39f5e24413e13673502a75ab8c4675fd2691ca1f442877e2a25")
+	if exp != root {
+		t.Errorf("root wrong, exp: %x, got %x", exp, root)
+	}
+}