eth, trie: improve code comment

eth/protocols/snap/gentrie.go

@@ -25,18 +25,14 @@ import (
 	"github.com/ethereum/go-ethereum/trie"
 )
 
-// genTrie interface is used by the trie to generate merkle tree nodes based
+// genTrie interface is used by the snap syncer to generate merkle tree nodes
-// on a received batch of states.
+// based on a received batch of states.
 type genTrie interface {
 	// update inserts the state into generator trie.
 	update(key, value []byte) error
 
-	// commit flushes the leftover nodes produced in the trie into database.
+	// commit flushes the right boundary nodes if complete flag is true. This
-	// The nodes on right boundary won't be committed unless this function
+	// function must be called before flushing the associated database batch.
-	// is called. The flag complete should be set to true if there are more
-	// items on the right side.
-	//
-	// This function must be called before flushing database batch.
 	commit(complete bool) common.Hash
 }
 
@@ -47,11 +43,11 @@ type genTrie interface {
 type pathTrie struct {
 	owner common.Hash     // identifier of trie owner, empty for account trie
 	tr    *trie.StackTrie // underlying raw stack trie
-	first []byte          // the path of first written node
+	first []byte          // the path of first committed node by stackTrie
-	last  []byte          // the path of last written node
+	last  []byte          // the path of last committed node by stackTrie
 
-	// Flag whether the nodes on the left boundary are skipped for committing.
+	// This flag indicates whether nodes on the left boundary are skipped for
-	// If it's set, then nodes on the left boundary are regarded as incomplete
+	// committing. If set, the left boundary nodes are considered incomplete
 	// due to potentially missing left children.
 	skipLeftBoundary bool
 	db               ethdb.KeyValueReader
@@ -70,58 +66,52 @@ func newPathTrie(owner common.Hash, skipLeftBoundary bool, db ethdb.KeyValueRead
 	return tr
 }
 
-// onTrieNode is invoked whenever a new node is produced by the stackTrie.
+// onTrieNode is invoked whenever a new node is committed by the stackTrie.
 //
-// As the produced nodes might be incomplete if they are on the boundaries
+// As the committed nodes might be incomplete if they are on the boundaries
 // (left or right), this function has the ability to detect the incomplete
-// ones and filter them out for committing. Namely, only the nodes belonging
+// ones and filter them out for committing.
-// to completed subtries will be committed.
 //
 // Additionally, the assumption is made that there may exist leftover dangling
-// nodes in the database. This function has the ability to detect all the
+// nodes in the database. This function has the ability to detect the dangling
-// dangling nodes that fall within the committed subtries (on the path covered
+// nodes that fall within the path space of committed nodes (specifically on
-// by internal extension nodes) and remove them from the database. This property
+// the path covered by internal extension nodes) and remove them from the
-// ensures that the entire path space is uniquely occupied by committed subtries.
+// database. This property ensures that the entire path space is uniquely
+// occupied by committed nodes.
 //
-// Furthermore, all leftover dangling nodes along the path from committed tries
+// Furthermore, all leftover dangling nodes along the path from committed nodes
-// to the root node should be removed as well; otherwise, they might potentially
+// to the trie root (left and right boundaries) should be removed as well;
-// disrupt the state healing process, leaving behind an inconsistent state.
+// otherwise, they might potentially disrupt the state healing process.
 func (t *pathTrie) onTrieNode(path []byte, hash common.Hash, blob []byte) {
-	// Filter out the nodes on the left boundary if skipLeftBoundary is configured.
+	// Filter out the nodes on the left boundary if skipLeftBoundary is
-	// Nodes are considered to be on the left boundary if it's the first one
+	// configured. Nodes are considered to be on the left boundary if
-	// produced, or on the path of the first produced one.
+	// it's the first one to be committed, or the parent/ancestor of the
+	// first committed node.
 	if t.skipLeftBoundary && (t.first == nil || bytes.HasPrefix(t.first, path)) {
 		if t.first == nil {
-			// Memorize the path of first produced node, which is regarded
+			// Memorize the path of first committed node, which is regarded
 			// as left boundary. Deep-copy is necessary as the path given
 			// is volatile.
 			t.first = append([]byte{}, path...)
 
-			// The position of first complete sub trie (e.g. N_4) can be determined
+			// The left boundary can be uniquely determined by the first committed node
-			// by the first produced node(e.g. N_1) correctly, with a branch node
+			// from stackTrie (e.g., N_1), as the shared path prefix between the first
-			// (e.g. N_5) as the common parent for shared path prefix. Therefore,
+			// two inserted state items is deterministic (the path of N_3). The path
-			// the nodes along the path from root to N_1 can be regarded as left
+			// from trie root towards the first committed node is considered the left
-			// boundary and the parent of the first complete sub trie. The leftover
+			// boundary. The potential leftover dangling nodes on left boundary should
-			// dangling nodes on left boundary should be cleaned out first before
+			// be cleaned out.
-			// committing any node.
 			//
-			//                           +-------------+
+			//                            +-----+
-			//                           |     N_5     |  parent for shared path prefix
+			//                            | N_3 | shared path prefix of state_1 and state_2
-			//                           +-------------+
+			//                            +-----+
-			//                           /-     |     -\
-			//                          /       |      [ others ]
-			//                     +-----+   +-----+
-			// First produced one  | N_1 |   | N_4 | First completed sub trie
-			//                     +-----+   +-----+
 			//                            /-   -\
-			//                              N-2 ... N-3
+			//                       +-----+   +-----+
+			// First committed node  | N_1 |   | N_2 | latest inserted node (contain state_2)
+			//                       +-----+   +-----+
 			//
-			// Nodes must be cleaned from top to bottom as it's possible the procedure
+			// The node with the path of the first committed one (e.g, N_1) is not
-			// is interrupted in the middle.
+			// removed because it's a sibling of the complete nodes, not the parent
-			//
+			// or ancestor.
-			// The node with the path of the first produced node is not removed, as
-			// it's a sibling of the first complete sub-trie, not the parent. There
-			// is no reason to remove it.
 			for i := 0; i < len(path); i++ {
 				t.delete(path[:i], false)
 			}
@@ -131,21 +121,21 @@ func (t *pathTrie) onTrieNode(path []byte, hash common.Hash, blob []byte) {
 	// If boundary filtering is not configured, or the node is not on the left
 	// boundary, commit it to database.
 	//
-	// Note, the nodes fall within the path between extension node and its
+	// Note: If the current committed node is an extension node, then the nodes
-	// **in disk** child must be cleaned out before committing the extension
+	// falling within the path between itself and its standalone (not embedded
-	// node. This is essential in snap sync to avoid leaving dangling nodes
+	// in parent) child should be cleaned out for exclusively occupy the inner
-	// within this range covered by extension node which could potentially
+	// path.
-	// break the state healing.
 	//
-	// The target node is detected if its path is the prefix of last written
+	// This is essential in snap sync to avoid leaving dangling nodes within
-	// one and path gap is larger than one. The current node could be a full
+	// this range covered by extension node which could potentially break the
-	// node, or a shortNode with single byte key if the path gap is one,
+	// state healing.
-	// unnecessary to sweep the internal path in this case.
 	//
-	// Nodes must be cleaned from top to bottom, including the node with the
+	// The extension node is detected if its path is the prefix of last written
-	// path of the committed extension node itself.
+	// one and path gap is larger than one. If the path gap is only one byte,
+	// the current node could either be a full node, or a extension with single
+	// byte key. In either case, no gaps will be left in the path.
 	if t.last != nil && bytes.HasPrefix(t.last, path) && len(t.last)-len(path) > 1 {
-		for i := len(path); i < len(t.last); i++ {
+		for i := len(path) + 1; i < len(t.last); i++ {
 			t.delete(t.last[:i], true)
 		}
 	}
@@ -168,36 +158,46 @@ func (t *pathTrie) write(path []byte, blob []byte) {
 	}
 }
 
-// delete commits the node deletion to provided database batch in path mode.
+func (t *pathTrie) deleteAccountNode(path []byte, inner bool) {
-func (t *pathTrie) delete(path []byte, inner bool) {
-	if t.owner == (common.Hash{}) {
-		if rawdb.ExistsAccountTrieNode(t.db, path) {
-			rawdb.DeleteAccountTrieNode(t.batch, path)
-			if inner {
-				accountInnerDeleteGauge.Inc(1)
-			} else {
-				accountOuterDeleteGauge.Inc(1)
-			}
-		}
 	if inner {
 		accountInnerLookupGauge.Inc(1)
 	} else {
 		accountOuterLookupGauge.Inc(1)
 	}
+	if !rawdb.ExistsAccountTrieNode(t.db, path) {
+		return
+	}
+	if inner {
+		accountInnerDeleteGauge.Inc(1)
+	} else {
+		accountOuterDeleteGauge.Inc(1)
+	}
+	rawdb.DeleteAccountTrieNode(t.batch, path)
+}
+
+func (t *pathTrie) deleteStorageNode(path []byte, inner bool) {
+	if inner {
+		storageInnerLookupGauge.Inc(1)
+	} else {
+		storageOuterLookupGauge.Inc(1)
+	}
+	if !rawdb.ExistsStorageTrieNode(t.db, t.owner, path) {
 		return
 	}
-	if rawdb.ExistsStorageTrieNode(t.db, t.owner, path) {
-		rawdb.DeleteStorageTrieNode(t.batch, t.owner, path)
 	if inner {
 		storageInnerDeleteGauge.Inc(1)
 	} else {
 		storageOuterDeleteGauge.Inc(1)
 	}
-	}
+	rawdb.DeleteStorageTrieNode(t.batch, t.owner, path)
-	if inner {
+}
-		storageInnerLookupGauge.Inc(1)
+
+// delete commits the node deletion to provided database batch in path mode.
+func (t *pathTrie) delete(path []byte, inner bool) {
+	if t.owner == (common.Hash{}) {
+		t.deleteAccountNode(path, inner)
 	} else {
-		storageOuterLookupGauge.Inc(1)
+		t.deleteStorageNode(path, inner)
 	}
 }
 
@@ -208,8 +208,8 @@ func (t *pathTrie) update(key, value []byte) error {
 }
 
 // commit implements genTrie interface, flushing the right boundary if it's
-// regarded as complete. Otherwise, the nodes on the right boundary are discarded
+// considered as complete. Otherwise, the nodes on the right boundary are
-// and cleaned up.
+// discarded and cleaned up.
 //
 // Note, this function must be called before flushing database batch, otherwise,
 // dangling nodes might be left in database.
@@ -218,51 +218,43 @@ func (t *pathTrie) commit(complete bool) common.Hash {
 	// The nodes on both left and right boundary will still be filtered
 	// out if left boundary filtering is configured.
 	if complete {
-		// The produced hash is meaningless if left side is incomplete
+		// Commit all inserted but not yet committed nodes(on the right
+		// boundary) in the stackTrie.
+		hash := t.tr.Hash()
 		if t.skipLeftBoundary {
-			return common.Hash{}
+			return common.Hash{} // hash is meaningless if left side is incomplete
 		}
-		return t.tr.Hash()
+		return hash
 	}
-	// If the right boundary is claimed as incomplete, the uncommitted
+	// Discard nodes on the right boundary as it's claimed as incomplete. These
-	// nodes should be discarded, as they might be incomplete due to
+	// nodes might be incomplete due to missing children on the right side.
-	// missing children on the right side. Furthermore, previously committed
+	// Furthermore, the potential leftover nodes on right boundary should also
-	// nodes can be the children of the right boundary nodes; therefore,
+	// be cleaned out.
-	// the nodes of the right boundary must be cleaned out!
 	//
-	// The position of the last complete sub-trie (e.g., N_1) can be correctly
+	// The right boundary can be uniquely determined by the last committed node
-	// determined by the last produced node (e.g., N_4), with a branch node
+	// from stackTrie (e.g., N_1), as the shared path prefix between the last
-	// (e.g., N_5) as the common parent for the shared path prefix. Therefore,
+	// two inserted state items is deterministic (the path of N_3). The path
-	// the nodes along the path from the root to N_4 can be regarded as the
+	// from trie root towards the last committed node is considered the right
-	// right boundary and the parent of the last complete subtrie.
+	// boundary (root to N_3).
 	//
 	//                           +-----+
-	//                             | N_5 |  parent for shared path prefix
+	//                           | N_3 | shared path prefix of last two states
 	//                           +-----+
 	//                           /-   -\
 	//                      +-----+   +-----+
-	// Last complete subtrie  | N_1 |   | N_4 | Last produced node
+	// Last committed node  | N_1 |   | N_2 | latest inserted node  (contain last state)
 	//                      +-----+   +-----+
-	//                        /-  -\
-	//                      N-2 .. N-3
 	//
 	// Another interesting scenario occurs when the trie is committed due to
 	// too many items being accumulated in the batch. To flush them out to
-	// the database, the path of the last inserted item is temporarily treated
+	// the database, the path of the last inserted node (N_2) is temporarily
-	// as an incomplete right boundary, and nodes on this path are removed.
+	// treated as an incomplete right boundary, and nodes on this path are
-	//
+	// removed (e.g. from root to N_3).
 	// However, this path will be reclaimed as an internal path by inserting
-	// more items after the batch flush. Newly produced nodes on this path
+	// more items after the batch flush. New nodes on this path can be committed
-	// can be committed with no issues as they are actually complete (also
+	// with no issues as they are actually complete. Also, from a database
-	// from a database perspective, first deleting and then rewriting is
+	// perspective, first deleting and then rewriting is a valid data update.
-	// still a valid data update).
-	//
-	// Nodes must be cleaned from top to bottom as it's possible the procedure
-	// is interrupted in the middle.
 	for i := 0; i < len(t.last); i++ {
-		// The node with the path of the last produced node is not removed, as
-		// it's a sibling of the last complete sub-trie, not the parent. There
-		// is no reason to remove it.
 		t.delete(t.last[:i], false)
 	}
 	return common.Hash{} // the hash is meaningless for incomplete commit

trie/stacktrie.go

@@ -30,13 +30,13 @@ var (
 	_      = types.TrieHasher((*StackTrie)(nil))
 )
 
-// OnTrieNode is a callback method to invoke when a trie node is produced
+// OnTrieNode is a callback method invoked when a trie node is committed
-// by the stack trie.
+// by the stack trie. The node is only committed if it's considered complete.
 //
 // The caller should not modify the contents of the returned path and blob
-// slice, and their contents may change after the call. It is up to the
+// slice, and their contents may be changed after the call. It is up to the
-// `onTrieNode` receiver function to deep-copy the data if it wants to
+// `onTrieNode` receiver function to deep-copy the data if it wants to retain
-// retain it after the call ends.
+// it after the call ends.
 type OnTrieNode func(path []byte, hash common.Hash, blob []byte)
 
 // StackTrie is a trie implementation that expects keys to be inserted
@@ -49,8 +49,8 @@ type StackTrie struct {
 	onTrieNode OnTrieNode
 }
 
-// NewStackTrie allocates and initializes an empty trie. The produced nodes will
+// NewStackTrie allocates and initializes an empty trie. The committed nodes
-// be discarded immediately if no callback is provided.
+// will be discarded immediately if no callback is configured.
 func NewStackTrie(onTrieNode OnTrieNode) *StackTrie {
 	return &StackTrie{
 		root:       stPool.Get().(*stNode),
@@ -374,7 +374,7 @@ func (t *StackTrie) hash(st *stNode, path []byte) {
 
 	// Invoke the callback it's provided. Notably, the path and blob slices are
 	// volatile, please deep-copy the slices in callback if the contents need
-	// to be saved.
+	// to be retained.
 	if t.onTrieNode != nil {
 		t.onTrieNode(path, common.BytesToHash(st.val), blob)
 	}