Revert "core, trie, eth: refactor stacktrie constructor (#28350)"

This reverts commit eed2347f95.

core/state/snapshot/conversion.go

@@ -362,15 +362,21 @@ func generateTrieRoot(db ethdb.KeyValueWriter, scheme string, it Iterator, accou
 }
 
 func stackTrieGenerate(db ethdb.KeyValueWriter, scheme string, owner common.Hash, in chan trieKV, out chan common.Hash) {
-	options := trie.NewStackTrieOptions()
+	var nodeWriter trie.NodeWriteFunc
 	if db != nil {
-		options = options.WithWriter(func(path []byte, hash common.Hash, blob []byte) {
+		nodeWriter = func(path []byte, hash common.Hash, blob []byte) {
 			rawdb.WriteTrieNode(db, owner, path, hash, blob, scheme)
-		})
 		}
-	t := trie.NewStackTrie(options)
+	}
+	t := trie.NewStackTrie(nodeWriter)
 	for leaf := range in {
 		t.Update(leaf.key[:], leaf.value)
 	}
-	out <- t.Commit()
+	var root common.Hash
+	if db == nil {
+		root = t.Hash()
+	} else {
+		root, _ = t.Commit()
+	}
+	out <- root
 }

core/state/statedb.go

@@ -964,12 +964,10 @@ func (s *StateDB) fastDeleteStorage(addrHash common.Hash, root common.Hash) (boo
 		nodes = trienode.NewNodeSet(addrHash)
 		slots = make(map[common.Hash][]byte)
 	)
-	options := trie.NewStackTrieOptions()
+	stack := trie.NewStackTrie(func(path []byte, hash common.Hash, blob []byte) {
-	options = options.WithWriter(func(path []byte, hash common.Hash, blob []byte) {
 		nodes.AddNode(path, trienode.NewDeleted())
 		size += common.StorageSize(len(path))
 	})
-	stack := trie.NewStackTrie(options)
 	for iter.Next() {
 		if size > storageDeleteLimit {
 			return true, size, nil, nil, nil

eth/protocols/snap/sync.go

@@ -738,11 +738,9 @@ func (s *Syncer) loadSyncStatus() {
 						s.accountBytes += common.StorageSize(len(key) + len(value))
 					},
 				}
-				options := trie.NewStackTrieOptions()
+				task.genTrie = trie.NewStackTrie(func(path []byte, hash common.Hash, val []byte) {
-				options = options.WithWriter(func(path []byte, hash common.Hash, blob []byte) {
+					rawdb.WriteTrieNode(task.genBatch, common.Hash{}, path, hash, val, s.scheme)
-					rawdb.WriteTrieNode(task.genBatch, common.Hash{}, path, hash, blob, s.scheme)
 				})
-				task.genTrie = trie.NewStackTrie(options)
 				for accountHash, subtasks := range task.SubTasks {
 					for _, subtask := range subtasks {
 						subtask := subtask // closure for subtask.genBatch in the stacktrie writer callback
@@ -754,11 +752,9 @@ func (s *Syncer) loadSyncStatus() {
 							},
 						}
 						owner := accountHash // local assignment for stacktrie writer closure
-						options := trie.NewStackTrieOptions()
+						subtask.genTrie = trie.NewStackTrie(func(path []byte, hash common.Hash, val []byte) {
-						options = options.WithWriter(func(path []byte, hash common.Hash, blob []byte) {
+							rawdb.WriteTrieNode(subtask.genBatch, owner, path, hash, val, s.scheme)
-							rawdb.WriteTrieNode(subtask.genBatch, owner, path, hash, blob, s.scheme)
 						})
-						subtask.genTrie = trie.NewStackTrie(options)
 					}
 				}
 			}
@@ -810,16 +806,14 @@ func (s *Syncer) loadSyncStatus() {
 				s.accountBytes += common.StorageSize(len(key) + len(value))
 			},
 		}
-		options := trie.NewStackTrieOptions()
-		options = options.WithWriter(func(path []byte, hash common.Hash, blob []byte) {
-			rawdb.WriteTrieNode(batch, common.Hash{}, path, hash, blob, s.scheme)
-		})
 		s.tasks = append(s.tasks, &accountTask{
 			Next:     next,
 			Last:     last,
 			SubTasks: make(map[common.Hash][]*storageTask),
 			genBatch: batch,
-			genTrie:  trie.NewStackTrie(options),
+			genTrie: trie.NewStackTrie(func(path []byte, hash common.Hash, val []byte) {
+				rawdb.WriteTrieNode(batch, common.Hash{}, path, hash, val, s.scheme)
+			}),
 		})
 		log.Debug("Created account sync task", "from", next, "last", last)
 		next = common.BigToHash(new(big.Int).Add(last.Big(), common.Big1))
@@ -2012,16 +2006,14 @@ func (s *Syncer) processStorageResponse(res *storageResponse) {
 						},
 					}
 					owner := account // local assignment for stacktrie writer closure
-					options := trie.NewStackTrieOptions()
-					options = options.WithWriter(func(path []byte, hash common.Hash, blob []byte) {
-						rawdb.WriteTrieNode(batch, owner, path, hash, blob, s.scheme)
-					})
 					tasks = append(tasks, &storageTask{
 						Next:     common.Hash{},
 						Last:     r.End(),
 						root:     acc.Root,
 						genBatch: batch,
-						genTrie:  trie.NewStackTrie(options),
+						genTrie: trie.NewStackTrie(func(path []byte, hash common.Hash, val []byte) {
+							rawdb.WriteTrieNode(batch, owner, path, hash, val, s.scheme)
+						}),
 					})
 					for r.Next() {
 						batch := ethdb.HookedBatch{
@@ -2030,16 +2022,14 @@ func (s *Syncer) processStorageResponse(res *storageResponse) {
 								s.storageBytes += common.StorageSize(len(key) + len(value))
 							},
 						}
-						options := trie.NewStackTrieOptions()
-						options = options.WithWriter(func(path []byte, hash common.Hash, blob []byte) {
-							rawdb.WriteTrieNode(batch, owner, path, hash, blob, s.scheme)
-						})
 						tasks = append(tasks, &storageTask{
 							Next:     r.Start(),
 							Last:     r.End(),
 							root:     acc.Root,
 							genBatch: batch,
-							genTrie:  trie.NewStackTrie(options),
+							genTrie: trie.NewStackTrie(func(path []byte, hash common.Hash, val []byte) {
+								rawdb.WriteTrieNode(batch, owner, path, hash, val, s.scheme)
+							}),
 						})
 					}
 					for _, task := range tasks {
@@ -2085,11 +2075,9 @@ func (s *Syncer) processStorageResponse(res *storageResponse) {
 
 		if i < len(res.hashes)-1 || res.subTask == nil {
 			// no need to make local reassignment of account: this closure does not outlive the loop
-			options := trie.NewStackTrieOptions()
+			tr := trie.NewStackTrie(func(path []byte, hash common.Hash, val []byte) {
-			options = options.WithWriter(func(path []byte, hash common.Hash, blob []byte) {
+				rawdb.WriteTrieNode(batch, account, path, hash, val, s.scheme)
-				rawdb.WriteTrieNode(batch, account, path, hash, blob, s.scheme)
 			})
-			tr := trie.NewStackTrie(options)
 			for j := 0; j < len(res.hashes[i]); j++ {
 				tr.Update(res.hashes[i][j][:], res.slots[i][j])
 			}
@@ -2111,8 +2099,9 @@ func (s *Syncer) processStorageResponse(res *storageResponse) {
 	// Large contracts could have generated new trie nodes, flush them to disk
 	if res.subTask != nil {
 		if res.subTask.done {
-			root := res.subTask.genTrie.Commit()
+			if root, err := res.subTask.genTrie.Commit(); err != nil {
-			if root == res.subTask.root {
+				log.Error("Failed to commit stack slots", "err", err)
+			} else if root == res.subTask.root {
 				// If the chunk's root is an overflown but full delivery, clear the heal request
 				for i, account := range res.mainTask.res.hashes {
 					if account == res.accounts[len(res.accounts)-1] {
@@ -2328,7 +2317,9 @@ func (s *Syncer) forwardAccountTask(task *accountTask) {
 	// flush after finalizing task.done. It's fine even if we crash and lose this
 	// write as it will only cause more data to be downloaded during heal.
 	if task.done {
-		task.genTrie.Commit()
+		if _, err := task.genTrie.Commit(); err != nil {
+			log.Error("Failed to commit stack account", "err", err)
+		}
 	}
 	if task.genBatch.ValueSize() > ethdb.IdealBatchSize || task.done {
 		if err := task.genBatch.Write(); err != nil {

tests/fuzzers/stacktrie/trie_fuzzer.go

@@ -140,11 +140,9 @@ func (f *fuzzer) fuzz() int {
 		trieA   = trie.NewEmpty(dbA)
 		spongeB = &spongeDb{sponge: sha3.NewLegacyKeccak256()}
 		dbB     = trie.NewDatabase(rawdb.NewDatabase(spongeB), nil)
-
+		trieB   = trie.NewStackTrie(func(path []byte, hash common.Hash, blob []byte) {
-		options = trie.NewStackTrieOptions().WithWriter(func(path []byte, hash common.Hash, blob []byte) {
 			rawdb.WriteTrieNode(spongeB, common.Hash{}, path, hash, blob, dbB.Scheme())
 		})
-		trieB       = trie.NewStackTrie(options)
 		vals        []kv
 		useful      bool
 		maxElements = 10000
@@ -207,19 +205,18 @@ func (f *fuzzer) fuzz() int {
 	// Ensure all the nodes are persisted correctly
 	var (
 		nodeset = make(map[string][]byte) // path -> blob
-		optionsC = trie.NewStackTrieOptions().WithWriter(func(path []byte, hash common.Hash, blob []byte) {
+		trieC   = trie.NewStackTrie(func(path []byte, hash common.Hash, blob []byte) {
 			if crypto.Keccak256Hash(blob) != hash {
 				panic("invalid node blob")
 			}
 			nodeset[string(path)] = common.CopyBytes(blob)
 		})
-		trieC   = trie.NewStackTrie(optionsC)
 		checked int
 	)
 	for _, kv := range vals {
 		trieC.MustUpdate(kv.k, kv.v)
 	}
-	rootC := trieC.Commit()
+	rootC, _ := trieC.Commit()
 	if rootA != rootC {
 		panic(fmt.Sprintf("roots differ: (trie) %x != %x (stacktrie)", rootA, rootC))
 	}

trie/stacktrie.go

@@ -17,6 +17,7 @@
 package trie
 
 import (
+	"errors"
 	"sync"
 
 	"github.com/ethereum/go-ethereum/common"
@@ -25,40 +26,28 @@ import (
 )
 
 var (
+	ErrCommitDisabled = errors.New("no database for committing")
 	stPool            = sync.Pool{New: func() any { return new(stNode) }}
 	_                 = types.TrieHasher((*StackTrie)(nil))
 )
 
-// StackTrieOptions contains the configured options for manipulating the stackTrie.
+// NodeWriteFunc is used to provide all information of a dirty node for committing
-type StackTrieOptions struct {
+// so that callers can flush nodes into database with desired scheme.
-	Writer func(path []byte, hash common.Hash, blob []byte) // The function to commit the dirty nodes
+type NodeWriteFunc = func(path []byte, hash common.Hash, blob []byte)
-}
-
-// NewStackTrieOptions initializes an empty options for stackTrie.
-func NewStackTrieOptions() *StackTrieOptions { return &StackTrieOptions{} }
-
-// WithWriter configures trie node writer within the options.
-func (o *StackTrieOptions) WithWriter(writer func(path []byte, hash common.Hash, blob []byte)) *StackTrieOptions {
-	o.Writer = writer
-	return o
-}
 
 // StackTrie is a trie implementation that expects keys to be inserted
 // in order. Once it determines that a subtree will no longer be inserted
 // into, it will hash it and free up the memory it uses.
 type StackTrie struct {
-	options *StackTrieOptions
+	writeFn NodeWriteFunc // function for committing nodes, can be nil
 	root    *stNode
 	h       *hasher
 }
 
 // NewStackTrie allocates and initializes an empty trie.
-func NewStackTrie(options *StackTrieOptions) *StackTrie {
+func NewStackTrie(writeFn NodeWriteFunc) *StackTrie {
-	if options == nil {
-		options = NewStackTrieOptions()
-	}
 	return &StackTrie{
-		options: options,
+		writeFn: writeFn,
 		root:    stPool.Get().(*stNode),
 		h:       newHasher(false),
 	}
@@ -70,9 +59,7 @@ func (t *StackTrie) Update(key, value []byte) error {
 	if len(value) == 0 {
 		panic("deletion not supported")
 	}
-	k = k[:len(k)-1] // chop the termination flag
+	t.insert(t.root, k[:len(k)-1], value, nil)
-
-	t.insert(t.root, k, value, nil)
 	return nil
 }
 
@@ -84,9 +71,8 @@ func (t *StackTrie) MustUpdate(key, value []byte) {
 	}
 }
 
-// Reset resets the stack trie object to empty state.
 func (t *StackTrie) Reset() {
-	t.options = NewStackTrieOptions()
+	t.writeFn = nil
 	t.root = stPool.Get().(*stNode)
 }
 
@@ -152,7 +138,7 @@ func (n *stNode) getDiffIndex(key []byte) int {
 
 // Helper function to that inserts a (key, value) pair into
 // the trie.
-func (t *StackTrie) insert(st *stNode, key, value []byte, path []byte) {
+func (t *StackTrie) insert(st *stNode, key, value []byte, prefix []byte) {
 	switch st.typ {
 	case branchNode: /* Branch */
 		idx := int(key[0])
@@ -161,7 +147,7 @@ func (t *StackTrie) insert(st *stNode, key, value []byte, path []byte) {
 		for i := idx - 1; i >= 0; i-- {
 			if st.children[i] != nil {
 				if st.children[i].typ != hashedNode {
-					t.hash(st.children[i], append(path, byte(i)))
+					t.hash(st.children[i], append(prefix, byte(i)))
 				}
 				break
 			}
@@ -171,7 +157,7 @@ func (t *StackTrie) insert(st *stNode, key, value []byte, path []byte) {
 		if st.children[idx] == nil {
 			st.children[idx] = newLeaf(key[1:], value)
 		} else {
-			t.insert(st.children[idx], key[1:], value, append(path, key[0]))
+			t.insert(st.children[idx], key[1:], value, append(prefix, key[0]))
 		}
 
 	case extNode: /* Ext */
@@ -186,7 +172,7 @@ func (t *StackTrie) insert(st *stNode, key, value []byte, path []byte) {
 		if diffidx == len(st.key) {
 			// Ext key and key segment are identical, recurse into
 			// the child node.
-			t.insert(st.children[0], key[diffidx:], value, append(path, key[:diffidx]...))
+			t.insert(st.children[0], key[diffidx:], value, append(prefix, key[:diffidx]...))
 			return
 		}
 		// Save the original part. Depending if the break is
@@ -199,14 +185,14 @@ func (t *StackTrie) insert(st *stNode, key, value []byte, path []byte) {
 			// extension. The path prefix of the newly-inserted
 			// extension should also contain the different byte.
 			n = newExt(st.key[diffidx+1:], st.children[0])
-			t.hash(n, append(path, st.key[:diffidx+1]...))
+			t.hash(n, append(prefix, st.key[:diffidx+1]...))
 		} else {
 			// Break on the last byte, no need to insert
 			// an extension node: reuse the current node.
 			// The path prefix of the original part should
 			// still be same.
 			n = st.children[0]
-			t.hash(n, append(path, st.key...))
+			t.hash(n, append(prefix, st.key...))
 		}
 		var p *stNode
 		if diffidx == 0 {
@@ -271,7 +257,7 @@ func (t *StackTrie) insert(st *stNode, key, value []byte, path []byte) {
 		// is hashed directly in order to free up some memory.
 		origIdx := st.key[diffidx]
 		p.children[origIdx] = newLeaf(st.key[diffidx+1:], st.val)
-		t.hash(p.children[origIdx], append(path, st.key[:diffidx+1]...))
+		t.hash(p.children[origIdx], append(prefix, st.key[:diffidx+1]...))
 
 		newIdx := key[diffidx]
 		p.children[newIdx] = newLeaf(key[diffidx+1:], value)
@@ -306,7 +292,8 @@ func (t *StackTrie) insert(st *stNode, key, value []byte, path []byte) {
 //
 // This method also sets 'st.type' to hashedNode, and clears 'st.key'.
 func (t *StackTrie) hash(st *stNode, path []byte) {
-	var blob []byte // RLP-encoded node blob
+	// The switch below sets this to the RLP-encoding of this node.
+	var encodedNode []byte
 
 	switch st.typ {
 	case hashedNode:
@@ -336,13 +323,11 @@ func (t *StackTrie) hash(st *stNode, path []byte) {
 			stPool.Put(child.reset()) // Release child back to pool.
 		}
 		nodes.encode(t.h.encbuf)
-		blob = t.h.encodedBytes()
+		encodedNode = t.h.encodedBytes()
 
 	case extNode:
-		// recursively hash and commit child as the first step
 		t.hash(st.children[0], append(path, st.key...))
 
-		// encode the extension node
 		n := shortNode{Key: hexToCompactInPlace(st.key)}
 		if len(st.children[0].val) < 32 {
 			n.Val = rawNode(st.children[0].val)
@@ -350,7 +335,7 @@ func (t *StackTrie) hash(st *stNode, path []byte) {
 			n.Val = hashNode(st.children[0].val)
 		}
 		n.encode(t.h.encbuf)
-		blob = t.h.encodedBytes()
+		encodedNode = t.h.encodedBytes()
 
 		stPool.Put(st.children[0].reset()) // Release child back to pool.
 		st.children[0] = nil
@@ -360,7 +345,7 @@ func (t *StackTrie) hash(st *stNode, path []byte) {
 		n := shortNode{Key: hexToCompactInPlace(st.key), Val: valueNode(st.val)}
 
 		n.encode(t.h.encbuf)
-		blob = t.h.encodedBytes()
+		encodedNode = t.h.encodedBytes()
 
 	default:
 		panic("invalid node type")
@@ -368,38 +353,60 @@ func (t *StackTrie) hash(st *stNode, path []byte) {
 
 	st.typ = hashedNode
 	st.key = st.key[:0]
-
+	if len(encodedNode) < 32 {
-	// Skip committing the non-root node if the size is smaller than 32 bytes.
+		st.val = common.CopyBytes(encodedNode)
-	if len(blob) < 32 && len(path) > 0 {
-		st.val = common.CopyBytes(blob)
 		return
 	}
-	// Write the hash to the 'val'. We allocate a new val here to not mutate
-	// input values.
-	st.val = t.h.hashData(blob)
 
-	// Commit the trie node if the writer is configured.
+	// Write the hash to the 'val'. We allocate a new val here to not mutate
-	if t.options.Writer != nil {
+	// input values
-		t.options.Writer(path, common.BytesToHash(st.val), blob)
+	st.val = t.h.hashData(encodedNode)
+	if t.writeFn != nil {
+		t.writeFn(path, common.BytesToHash(st.val), encodedNode)
 	}
 }
 
-// Hash will firstly hash the entire trie if it's still not hashed and then commit
+// Hash returns the hash of the current node.
-// all nodes to the associated database. Actually most of the trie nodes have been
+func (t *StackTrie) Hash() (h common.Hash) {
-// committed already. The main purpose here is to commit the nodes on right boundary.
+	st := t.root
-//
+	t.hash(st, nil)
-// For stack trie, Hash and Commit are functionally identical.
+	if len(st.val) == 32 {
-func (t *StackTrie) Hash() common.Hash {
+		copy(h[:], st.val)
-	n := t.root
+		return h
-	t.hash(n, nil)
+	}
-	return common.BytesToHash(n.val)
+	// If the node's RLP isn't 32 bytes long, the node will not
+	// be hashed, and instead contain the  rlp-encoding of the
+	// node. For the top level node, we need to force the hashing.
+	t.h.sha.Reset()
+	t.h.sha.Write(st.val)
+	t.h.sha.Read(h[:])
+	return h
 }
 
-// Commit will firstly hash the entire trie if it's still not hashed and then commit
+// Commit will firstly hash the entire trie if it's still not hashed
-// all nodes to the associated database. Actually most of the trie nodes have been
+// and then commit all nodes to the associated database. Actually most
-// committed already. The main purpose here is to commit the nodes on right boundary.
+// of the trie nodes MAY have been committed already. The main purpose
+// here is to commit the root node.
 //
-// For stack trie, Hash and Commit are functionally identical.
+// The associated database is expected, otherwise the whole commit
-func (t *StackTrie) Commit() common.Hash {
+// functionality should be disabled.
-	return t.Hash()
+func (t *StackTrie) Commit() (h common.Hash, err error) {
+	if t.writeFn == nil {
+		return common.Hash{}, ErrCommitDisabled
+	}
+	st := t.root
+	t.hash(st, nil)
+	if len(st.val) == 32 {
+		copy(h[:], st.val)
+		return h, nil
+	}
+	// If the node's RLP isn't 32 bytes long, the node will not
+	// be hashed (and committed), and instead contain the rlp-encoding of the
+	// node. For the top level node, we need to force the hashing+commit.
+	t.h.sha.Reset()
+	t.h.sha.Write(st.val)
+	t.h.sha.Read(h[:])
+
+	t.writeFn(nil, h, st.val)
+	return h, nil
 }

trie/trie_test.go

@@ -912,12 +912,9 @@ func TestCommitSequenceStackTrie(t *testing.T) {
 		trie := NewEmpty(db)
 		// Another sponge is used for the stacktrie commits
 		stackTrieSponge := &spongeDb{sponge: sha3.NewLegacyKeccak256(), id: "b"}
-
+		stTrie := NewStackTrie(func(path []byte, hash common.Hash, blob []byte) {
-		options := NewStackTrieOptions()
-		options = options.WithWriter(func(path []byte, hash common.Hash, blob []byte) {
 			rawdb.WriteTrieNode(stackTrieSponge, common.Hash{}, path, hash, blob, db.Scheme())
 		})
-		stTrie := NewStackTrie(options)
 		// Fill the trie with elements
 		for i := 0; i < count; i++ {
 			// For the stack trie, we need to do inserts in proper order
@@ -940,7 +937,10 @@ func TestCommitSequenceStackTrie(t *testing.T) {
 		db.Update(root, types.EmptyRootHash, 0, trienode.NewWithNodeSet(nodes), nil)
 		db.Commit(root, false)
 		// And flush stacktrie -> disk
-		stRoot := stTrie.Commit()
+		stRoot, err := stTrie.Commit()
+		if err != nil {
+			t.Fatalf("Failed to commit stack trie %v", err)
+		}
 		if stRoot != root {
 			t.Fatalf("root wrong, got %x exp %x", stRoot, root)
 		}
@@ -971,12 +971,9 @@ func TestCommitSequenceSmallRoot(t *testing.T) {
 	trie := NewEmpty(db)
 	// Another sponge is used for the stacktrie commits
 	stackTrieSponge := &spongeDb{sponge: sha3.NewLegacyKeccak256(), id: "b"}
-
+	stTrie := NewStackTrie(func(path []byte, hash common.Hash, blob []byte) {
-	options := NewStackTrieOptions()
-	options = options.WithWriter(func(path []byte, hash common.Hash, blob []byte) {
 		rawdb.WriteTrieNode(stackTrieSponge, common.Hash{}, path, hash, blob, db.Scheme())
 	})
-	stTrie := NewStackTrie(options)
 	// Add a single small-element to the trie(s)
 	key := make([]byte, 5)
 	key[0] = 1
@@ -988,7 +985,10 @@ func TestCommitSequenceSmallRoot(t *testing.T) {
 	db.Update(root, types.EmptyRootHash, 0, trienode.NewWithNodeSet(nodes), nil)
 	db.Commit(root, false)
 	// And flush stacktrie -> disk
-	stRoot := stTrie.Commit()
+	stRoot, err := stTrie.Commit()
+	if err != nil {
+		t.Fatalf("Failed to commit stack trie %v", err)
+	}
 	if stRoot != root {
 		t.Fatalf("root wrong, got %x exp %x", stRoot, root)
 	}