From b1a0cd4d6f534b23ede14321ab1613cb4b6aabc2 Mon Sep 17 00:00:00 2001 From: devopsbo3 <69951731+devopsbo3@users.noreply.github.com> Date: Fri, 10 Nov 2023 12:27:53 -0600 Subject: [PATCH] Revert "core, trie, eth: refactor stacktrie constructor (#28350)" This reverts commit eed2347f950a54d6242bc835d8183747eb1e842c. --- core/state/snapshot/conversion.go | 16 ++- core/state/statedb.go | 4 +- eth/protocols/snap/sync.go | 51 ++++------ tests/fuzzers/stacktrie/trie_fuzzer.go | 11 +- trie/stacktrie.go | 135 +++++++++++++------------ trie/trie_test.go | 20 ++-- 6 files changed, 118 insertions(+), 119 deletions(-) diff --git a/core/state/snapshot/conversion.go b/core/state/snapshot/conversion.go index 681be7ebc0..321bfbc6a2 100644 --- a/core/state/snapshot/conversion.go +++ b/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 } diff --git a/core/state/statedb.go b/core/state/statedb.go index 195e463c28..d28cd29b30 100644 --- a/core/state/statedb.go +++ b/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() - options = options.WithWriter(func(path []byte, hash common.Hash, blob []byte) { + stack := trie.NewStackTrie(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 diff --git a/eth/protocols/snap/sync.go b/eth/protocols/snap/sync.go index 041bf32b99..22638d0473 100644 --- a/eth/protocols/snap/sync.go +++ b/eth/protocols/snap/sync.go @@ -738,11 +738,9 @@ 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(task.genBatch, common.Hash{}, path, hash, blob, s.scheme) + task.genTrie = trie.NewStackTrie(func(path []byte, hash common.Hash, val []byte) { + rawdb.WriteTrieNode(task.genBatch, common.Hash{}, path, hash, val, 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() - options = options.WithWriter(func(path []byte, hash common.Hash, blob []byte) { - rawdb.WriteTrieNode(subtask.genBatch, owner, path, hash, blob, s.scheme) + subtask.genTrie = trie.NewStackTrie(func(path []byte, hash common.Hash, val []byte) { + rawdb.WriteTrieNode(subtask.genBatch, owner, path, hash, val, 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() - options = options.WithWriter(func(path []byte, hash common.Hash, blob []byte) { - rawdb.WriteTrieNode(batch, account, path, hash, blob, s.scheme) + tr := trie.NewStackTrie(func(path []byte, hash common.Hash, val []byte) { + rawdb.WriteTrieNode(batch, account, path, hash, val, 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 == res.subTask.root { + if root, err := res.subTask.genTrie.Commit(); err != nil { + 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 { diff --git a/tests/fuzzers/stacktrie/trie_fuzzer.go b/tests/fuzzers/stacktrie/trie_fuzzer.go index b818f2ec3c..20b8ca24b3 100644 --- a/tests/fuzzers/stacktrie/trie_fuzzer.go +++ b/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) - - options = trie.NewStackTrieOptions().WithWriter(func(path []byte, hash common.Hash, blob []byte) { + trieB = trie.NewStackTrie(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 @@ -206,20 +204,19 @@ 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) { + nodeset = make(map[string][]byte) // path -> blob + 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)) } diff --git a/trie/stacktrie.go b/trie/stacktrie.go index 09c3402663..35208e1cb3 100644 --- a/trie/stacktrie.go +++ b/trie/stacktrie.go @@ -17,6 +17,7 @@ package trie import ( + "errors" "sync" "github.com/ethereum/go-ethereum/common" @@ -25,40 +26,28 @@ import ( ) var ( - stPool = sync.Pool{New: func() any { return new(stNode) }} - _ = types.TrieHasher((*StackTrie)(nil)) + 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. -type StackTrieOptions struct { - Writer func(path []byte, hash common.Hash, blob []byte) // The function to commit the dirty nodes -} - -// 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 -} +// NodeWriteFunc is used to provide all information of a dirty node for committing +// so that callers can flush nodes into database with desired scheme. +type NodeWriteFunc = func(path []byte, hash common.Hash, blob []byte) // 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 { - if options == nil { - options = NewStackTrieOptions() - } +func NewStackTrie(writeFn NodeWriteFunc) *StackTrie { 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, value, nil) + t.insert(t.root, k[:len(k)-1], 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] - - // Skip committing the non-root node if the size is smaller than 32 bytes. - if len(blob) < 32 && len(path) > 0 { - st.val = common.CopyBytes(blob) + if len(encodedNode) < 32 { + st.val = common.CopyBytes(encodedNode) 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. - if t.options.Writer != nil { - t.options.Writer(path, common.BytesToHash(st.val), blob) + // Write the hash to the 'val'. We allocate a new val here to not mutate + // input values + 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 -// all nodes to the associated database. Actually most of the trie nodes have been -// committed already. The main purpose here is to commit the nodes on right boundary. -// -// For stack trie, Hash and Commit are functionally identical. -func (t *StackTrie) Hash() common.Hash { - n := t.root - t.hash(n, nil) - return common.BytesToHash(n.val) +// Hash returns the hash of the current node. +func (t *StackTrie) Hash() (h common.Hash) { + st := t.root + t.hash(st, nil) + if len(st.val) == 32 { + copy(h[:], st.val) + return h + } + // 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 -// all nodes to the associated database. Actually most of the trie nodes have been -// committed already. The main purpose here is to commit the nodes on right boundary. +// Commit will firstly hash the entire trie if it's still not hashed +// and then commit all nodes to the associated database. Actually most +// 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. -func (t *StackTrie) Commit() common.Hash { - return t.Hash() +// The associated database is expected, otherwise the whole commit +// functionality should be disabled. +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 } diff --git a/trie/trie_test.go b/trie/trie_test.go index 4315753548..2dfe81ef81 100644 --- a/trie/trie_test.go +++ b/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"} - - options := NewStackTrieOptions() - options = options.WithWriter(func(path []byte, hash common.Hash, blob []byte) { + stTrie := NewStackTrie(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"} - - options := NewStackTrieOptions() - options = options.WithWriter(func(path []byte, hash common.Hash, blob []byte) { + stTrie := NewStackTrie(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) }