Revert "core/trie: remove trie tracer (ethereum#26665)" (ethereum#26732)

This reverts commit 7c749c9.

core/state/metrics.go

@@ -19,10 +19,12 @@ package state
 import "github.com/ethereum/go-ethereum/metrics"
 
 var (
-	accountUpdatedMeter    = metrics.NewRegisteredMeter("state/update/account", nil)
-	storageUpdatedMeter    = metrics.NewRegisteredMeter("state/update/storage", nil)
-	accountDeletedMeter    = metrics.NewRegisteredMeter("state/delete/account", nil)
-	storageDeletedMeter    = metrics.NewRegisteredMeter("state/delete/storage", nil)
-	accountTrieNodesMeter  = metrics.NewRegisteredMeter("state/trie/account", nil)
-	storageTriesNodesMeter = metrics.NewRegisteredMeter("state/trie/storage", nil)
+	accountUpdatedMeter      = metrics.NewRegisteredMeter("state/update/account", nil)
+	storageUpdatedMeter      = metrics.NewRegisteredMeter("state/update/storage", nil)
+	accountDeletedMeter      = metrics.NewRegisteredMeter("state/delete/account", nil)
+	storageDeletedMeter      = metrics.NewRegisteredMeter("state/delete/storage", nil)
+	accountTrieUpdatedMeter  = metrics.NewRegisteredMeter("state/update/accountnodes", nil)
+	storageTriesUpdatedMeter = metrics.NewRegisteredMeter("state/update/storagenodes", nil)
+	accountTrieDeletedMeter  = metrics.NewRegisteredMeter("state/delete/accountnodes", nil)
+	storageTriesDeletedMeter = metrics.NewRegisteredMeter("state/delete/storagenodes", nil)
 )

core/state/statedb.go

@@ -970,11 +970,13 @@ func (s *StateDB) Commit(deleteEmptyObjects bool) (common.Hash, error) {
 
 	// Commit objects to the trie, measuring the elapsed time
 	var (
-		accountTrieNodes int
-		storageTrieNodes int
-		nodes            = trie.NewMergedNodeSet()
-		codeWriter       = s.db.DiskDB().NewBatch()
+		accountTrieNodesUpdated int
+		accountTrieNodesDeleted int
+		storageTrieNodesUpdated int
+		storageTrieNodesDeleted int
+		nodes                   = trie.NewMergedNodeSet()
 	)
+	codeWriter := s.db.DiskDB().NewBatch()
 	for addr := range s.stateObjectsDirty {
 		if obj := s.stateObjects[addr]; !obj.deleted {
 			// Write any contract code associated with the state object
@@ -992,9 +994,17 @@ func (s *StateDB) Commit(deleteEmptyObjects bool) (common.Hash, error) {
 				if err := nodes.Merge(set); err != nil {
 					return common.Hash{}, err
 				}
-				storageTrieNodes += set.Size()
+				updates, deleted := set.Size()
+				storageTrieNodesUpdated += updates
+				storageTrieNodesDeleted += deleted
 			}
 		}
+		// If the contract is destructed, the storage is still left in the
+		// database as dangling data. Theoretically it's should be wiped from
+		// database as well, but in hash-based-scheme it's extremely hard to
+		// determine that if the trie nodes are also referenced by other storage,
+		// and in path-based-scheme some technical challenges are still unsolved.
+		// Although it won't affect the correctness but please fix it TODO(rjl493456442).
 	}
 	if len(s.stateObjectsDirty) > 0 {
 		s.stateObjectsDirty = make(map[common.Address]struct{})
@@ -1015,7 +1025,7 @@ func (s *StateDB) Commit(deleteEmptyObjects bool) (common.Hash, error) {
 		if err := nodes.Merge(set); err != nil {
 			return common.Hash{}, err
 		}
-		accountTrieNodes = set.Size()
+		accountTrieNodesUpdated, accountTrieNodesDeleted = set.Size()
 	}
 	if metrics.EnabledExpensive {
 		s.AccountCommits += time.Since(start)
@@ -1024,9 +1034,10 @@ func (s *StateDB) Commit(deleteEmptyObjects bool) (common.Hash, error) {
 		storageUpdatedMeter.Mark(int64(s.StorageUpdated))
 		accountDeletedMeter.Mark(int64(s.AccountDeleted))
 		storageDeletedMeter.Mark(int64(s.StorageDeleted))
-		accountTrieNodesMeter.Mark(int64(accountTrieNodes))
-		storageTriesNodesMeter.Mark(int64(storageTrieNodes))
-
+		accountTrieUpdatedMeter.Mark(int64(accountTrieNodesUpdated))
+		accountTrieDeletedMeter.Mark(int64(accountTrieNodesDeleted))
+		storageTriesUpdatedMeter.Mark(int64(storageTrieNodesUpdated))
+		storageTriesDeletedMeter.Mark(int64(storageTrieNodesDeleted))
 		s.AccountUpdated, s.AccountDeleted = 0, 0
 		s.StorageUpdated, s.StorageDeleted = 0, 0
 	}

trie/committer.go

@@ -33,22 +33,29 @@ type leaf struct {
 // insertion order.
 type committer struct {
 	nodes       *NodeSet
+	tracer      *tracer
 	collectLeaf bool
 }
 
 // newCommitter creates a new committer or picks one from the pool.
-func newCommitter(nodes *NodeSet, collectLeaf bool) *committer {
+func newCommitter(owner common.Hash, tracer *tracer, collectLeaf bool) *committer {
 	return &committer{
-		nodes:       nodes,
+		nodes:       NewNodeSet(owner),
+		tracer:      tracer,
 		collectLeaf: collectLeaf,
 	}
 }
 
 // Commit collapses a node down into a hash node and returns it along with
 // the modified nodeset.
-func (c *committer) Commit(n node) hashNode {
+func (c *committer) Commit(n node) (hashNode, *NodeSet) {
 	h := c.commit(nil, n)
-	return h.(hashNode)
+	// Some nodes can be deleted from trie which can't be captured
+	// by committer itself. Iterate all deleted nodes tracked by
+	// tracer and marked them as deleted only if they are present
+	// in database previously.
+	c.tracer.markDeletions(c.nodes)
+	return h.(hashNode), c.nodes
 }
 
 // commit collapses a node down into a hash node and returns it.
@@ -78,6 +85,12 @@ func (c *committer) commit(path []byte, n node) node {
 		if hn, ok := hashedNode.(hashNode); ok {
 			return hn
 		}
+		// The short node now is embedded in its parent. Mark the node as
+		// deleted if it's present in database previously. It's equivalent
+		// as deletion from database's perspective.
+		if prev := c.tracer.getPrev(path); len(prev) != 0 {
+			c.nodes.markDeleted(path, prev)
+		}
 		return collapsed
 	case *fullNode:
 		hashedKids := c.commitChildren(path, cn)
@@ -88,6 +101,12 @@ func (c *committer) commit(path []byte, n node) node {
 		if hn, ok := hashedNode.(hashNode); ok {
 			return hn
 		}
+		// The full node now is embedded in its parent. Mark the node as
+		// deleted if it's present in database previously. It's equivalent
+		// as deletion from database's perspective.
+		if prev := c.tracer.getPrev(path); len(prev) != 0 {
+			c.nodes.markDeleted(path, prev)
+		}
 		return collapsed
 	case hashNode:
 		return cn
@@ -150,7 +169,7 @@ func (c *committer) store(path []byte, n node) node {
 		}
 	)
 	// Collect the dirty node to nodeset for return.
-	c.nodes.markUpdated(path, mnode)
+	c.nodes.markUpdated(path, mnode, c.tracer.getPrev(path))
 
 	// Collect the corresponding leaf node if it's required. We don't check
 	// full node since it's impossible to store value in fullNode. The key

trie/database.go

@@ -792,12 +792,13 @@ func (db *Database) Update(nodes *MergedNodeSet) error {
 	}
 	for _, owner := range order {
 		subset := nodes.sets[owner]
-		subset.forEachWithOrder(false, func(path string, n *memoryNode) {
-			if n.isDeleted() {
-				return // ignore deletion
+		for _, path := range subset.updates.order {
+			n, ok := subset.updates.nodes[path]
+			if !ok {
+				return fmt.Errorf("missing node %x %v", owner, path)
 			}
 			db.insert(n.hash, int(n.size), n.node)
-		})
+		}
 	}
 	// Link up the account trie and storage trie if the node points
 	// to an account trie leaf.

trie/nodeset.go

@@ -19,7 +19,6 @@ package trie
 import (
 	"fmt"
 	"reflect"
-	"sort"
 	"strings"
 
 	"github.com/ethereum/go-ethereum/common"
@@ -41,8 +40,8 @@ var memoryNodeSize = int(reflect.TypeOf(memoryNode{}).Size())
 
 // memorySize returns the total memory size used by this node.
 // nolint:unused
-func (n *memoryNode) memorySize(pathlen int) int {
-	return int(n.size) + memoryNodeSize + pathlen
+func (n *memoryNode) memorySize(key int) int {
+	return int(n.size) + memoryNodeSize + key
 }
 
 // rlp returns the raw rlp encoded blob of the cached trie node, either directly
@@ -65,107 +64,96 @@ func (n *memoryNode) obj() node {
 	return expandNode(n.hash[:], n.node)
 }
 
-// isDeleted returns the indicator if the node is marked as deleted.
-func (n *memoryNode) isDeleted() bool {
-	return n.hash == (common.Hash{})
-}
-
 // nodeWithPrev wraps the memoryNode with the previous node value.
-// nolint: unused
 type nodeWithPrev struct {
 	*memoryNode
 	prev []byte // RLP-encoded previous value, nil means it's non-existent
 }
 
 // unwrap returns the internal memoryNode object.
-// nolint: unused
+// nolint:unused
 func (n *nodeWithPrev) unwrap() *memoryNode {
 	return n.memoryNode
 }
 
 // memorySize returns the total memory size used by this node. It overloads
 // the function in memoryNode by counting the size of previous value as well.
 // nolint: unused
-func (n *nodeWithPrev) memorySize(pathlen int) int {
-	return n.memoryNode.memorySize(pathlen) + len(n.prev)
+func (n *nodeWithPrev) memorySize(key int) int {
+	return n.memoryNode.memorySize(key) + len(n.prev)
+}
+
+// nodesWithOrder represents a collection of dirty nodes which includes
+// newly-inserted and updated nodes. The modification order of all nodes
+// is represented by order list.
+type nodesWithOrder struct {
+	order []string                 // the path list of dirty nodes, sort by insertion order
+	nodes map[string]*nodeWithPrev // the map of dirty nodes, keyed by node path
 }
 
 // NodeSet contains all dirty nodes collected during the commit operation.
 // Each node is keyed by path. It's not thread-safe to use.
 type NodeSet struct {
-	owner      common.Hash            // the identifier of the trie
-	nodes      map[string]*memoryNode // the set of dirty nodes(inserted, updated, deleted)
-	leaves     []*leaf                // the list of dirty leaves
-	accessList map[string][]byte      // The list of accessed nodes, which records the original node value
+	owner   common.Hash       // the identifier of the trie
+	updates *nodesWithOrder   // the set of updated nodes(newly inserted, updated)
+	deletes map[string][]byte // the map of deleted nodes, keyed by node
+	leaves  []*leaf           // the list of dirty leaves
 }
 
 // NewNodeSet initializes an empty node set to be used for tracking dirty nodes
-// for a specific account or storage trie. The owner is zero for the account trie
-// and the owning account address hash for storage tries. The provided accessList
-// represents the original value of accessed nodes, it can be optional but would
-// be beneficial for speeding up the construction of trie history.
-func NewNodeSet(owner common.Hash, accessList map[string][]byte) *NodeSet {
-	// Don't panic for lazy users
-	if accessList == nil {
-		accessList = make(map[string][]byte)
-	}
+// from a specific account or storage trie. The owner is zero for the account
+// trie and the owning account address hash for storage tries.
+func NewNodeSet(owner common.Hash) *NodeSet {
 	return &NodeSet{
-		owner:      owner,
-		nodes:      make(map[string]*memoryNode),
-		accessList: accessList,
+		owner: owner,
+		updates: &nodesWithOrder{
+			nodes: make(map[string]*nodeWithPrev),
+		},
+		deletes: make(map[string][]byte),
 	}
 }
 
-// forEachWithOrder iterates the dirty nodes with the specified order.
-// If topToBottom is true:
-//
-//	then the order of iteration is top to bottom, left to right.
-//
-// If topToBottom is false:
-//
-//	then the order of iteration is bottom to top, right to left.
-func (set *NodeSet) forEachWithOrder(topToBottom bool, callback func(path string, n *memoryNode)) {
-	var paths sort.StringSlice
-	for path := range set.nodes {
-		paths = append(paths, path)
-	}
-	if topToBottom {
-		paths.Sort()
-	} else {
-		sort.Sort(sort.Reverse(paths))
-	}
-	for _, path := range paths {
-		callback(path, set.nodes[path])
+/*
+// NewNodeSetWithDeletion initializes the nodeset with provided deletion set.
+func NewNodeSetWithDeletion(owner common.Hash, paths [][]byte, prev [][]byte) *NodeSet {
+	set := NewNodeSet(owner)
+	for i, path := range paths {
+		set.markDeleted(path, prev[i])
 	}
+	return set
 }
+*/
 
 // markUpdated marks the node as dirty(newly-inserted or updated) with provided
 // node path, node object along with its previous value.
-func (set *NodeSet) markUpdated(path []byte, node *memoryNode) {
-	set.nodes[string(path)] = node
+func (set *NodeSet) markUpdated(path []byte, node *memoryNode, prev []byte) {
+	set.updates.order = append(set.updates.order, string(path))
+	set.updates.nodes[string(path)] = &nodeWithPrev{
+		memoryNode: node,
+		prev:       prev,
+	}
 }
 
 // markDeleted marks the node as deleted with provided path and previous value.
-// nolint: unused
-func (set *NodeSet) markDeleted(path []byte) {
-	set.nodes[string(path)] = &memoryNode{}
+func (set *NodeSet) markDeleted(path []byte, prev []byte) {
+	set.deletes[string(path)] = prev
 }
 
 // addLeaf collects the provided leaf node into set.
 func (set *NodeSet) addLeaf(node *leaf) {
 	set.leaves = append(set.leaves, node)
 }
 
-// Size returns the number of dirty nodes contained in the set.
-func (set *NodeSet) Size() int {
-	return len(set.nodes)
+// Size returns the number of updated and deleted nodes contained in the set.
+func (set *NodeSet) Size() (int, int) {
+	return len(set.updates.order), len(set.deletes)
 }
 
 // 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
-	for _, node := range set.nodes {
+	for _, node := range set.updates.nodes {
 		ret = append(ret, node.hash)
 	}
 	return ret
@@ -175,17 +163,19 @@ func (set *NodeSet) Hashes() []common.Hash {
 func (set *NodeSet) Summary() string {
 	var out = new(strings.Builder)
 	fmt.Fprintf(out, "nodeset owner: %v\n", set.owner)
-	if set.nodes != nil {
-		for path, n := range set.nodes {
-			// Deletion
-			if n.isDeleted() {
-				fmt.Fprintf(out, "  [-]: %x\n", path)
-				continue
+	if set.updates != nil {
+		for _, key := range set.updates.order {
+			updated := set.updates.nodes[key]
+			if updated.prev != nil {
+				fmt.Fprintf(out, "  [*]: %x -> %v prev: %x\n", key, updated.hash, updated.prev)
+			} else {
+				fmt.Fprintf(out, "  [+]: %x -> %v\n", key, updated.hash)
 			}
-			// Update
-			fmt.Fprintf(out, "  [+]: %x -> %v\n", path, n.hash)
 		}
 	}
+	for k, n := range set.deletes {
+		fmt.Fprintf(out, "  [-]: %x -> %x\n", k, n)
+	}
 	for _, n := range set.leaves {
 		fmt.Fprintf(out, "[leaf]: %v\n", n)
 	}