From f7fd4af08750129850daf30d98acc5ebd4f43ce6 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Mon, 9 Nov 2020 15:08:12 +0100 Subject: [PATCH] trie: stacktrie fixes (#21799) * trie: fix error in stacktrie not committing small roots * trie: improved tests * trie: fix error in stacktrie with small nodes * trie: add (skipped) testcase for stacktrie * trie: fix docs in stacktrie --- trie/stacktrie.go | 39 ++++++++++++++++++++++----------- trie/stacktrie_test.go | 49 ++++++++++++++++++++++++++++++++++++++++++ trie/trie_test.go | 36 +++++++++++++++++++++++++++++++ 3 files changed, 112 insertions(+), 12 deletions(-) diff --git a/trie/stacktrie.go b/trie/stacktrie.go index 95fe5087f1..3e5f1ef2c0 100644 --- a/trie/stacktrie.go +++ b/trie/stacktrie.go @@ -315,19 +315,22 @@ func (st *StackTrie) hash() { panic(err) } case extNode: + st.children[0].hash() h = newHasher(false) defer returnHasherToPool(h) h.tmp.Reset() - st.children[0].hash() - // This is also possible: - //sz := hexToCompactInPlace(st.key) - //n := [][]byte{ - // st.key[:sz], - // st.children[0].val, - //} - n := [][]byte{ - hexToCompact(st.key), - st.children[0].val, + var valuenode Node + if len(st.children[0].val) < 32 { + valuenode = rawNode(st.children[0].val) + } else { + valuenode = HashNode(st.children[0].val) + } + n := struct { + Key []byte + Val Node + }{ + Key: hexToCompact(st.key), + Val: valuenode, } if err := rlp.Encode(&h.tmp, n); err != nil { panic(err) @@ -407,6 +410,18 @@ func (st *StackTrie) Commit() (common.Hash, error) { return common.Hash{}, ErrCommitDisabled } st.hash() - h := common.BytesToHash(st.val) - return h, nil + if len(st.val) != 32 { + // 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. + ret := make([]byte, 32) + h := newHasher(false) + defer returnHasherToPool(h) + h.sha.Reset() + h.sha.Write(st.val) + h.sha.Read(ret) + st.db.Put(ret, st.val) + return common.BytesToHash(ret), nil + } + return common.BytesToHash(st.val), nil } diff --git a/trie/stacktrie_test.go b/trie/stacktrie_test.go index 2ff8866de7..ad2716ec0c 100644 --- a/trie/stacktrie_test.go +++ b/trie/stacktrie_test.go @@ -240,3 +240,52 @@ func TestDerivableList(t *testing.T) { } } } + +// TestUpdateSmallNodes tests a case where the leaves are small (both key and value), +// which causes a lot of node-within-node. This case was found via fuzzing. +func TestUpdateSmallNodes(t *testing.T) { + st := NewStackTrie(nil) + nt, _ := New(common.Hash{}, NewDatabase(memorydb.New())) + kvs := []struct { + K string + V string + }{ + {"63303030", "3041"}, // stacktrie.Update + {"65", "3000"}, // stacktrie.Update + } + for _, kv := range kvs { + nt.TryUpdate(common.FromHex(kv.K), common.FromHex(kv.V)) + st.TryUpdate(common.FromHex(kv.K), common.FromHex(kv.V)) + } + if nt.Hash() != st.Hash() { + t.Fatalf("error %x != %x", st.Hash(), nt.Hash()) + } +} + +// TestUpdateVariableKeys contains a case which stacktrie fails: when keys of different +// sizes are used, and the second one has the same prefix as the first, then the +// stacktrie fails, since it's unable to 'expand' on an already added leaf. +// For all practical purposes, this is fine, since keys are fixed-size length +// in account and storage tries. +// +// The test is marked as 'skipped', and exists just to have the behaviour documented. +// This case was found via fuzzing. +func TestUpdateVariableKeys(t *testing.T) { + t.SkipNow() + st := NewStackTrie(nil) + nt, _ := New(common.Hash{}, NewDatabase(memorydb.New())) + kvs := []struct { + K string + V string + }{ + {"0x33303534636532393561313031676174", "303030"}, + {"0x3330353463653239356131303167617430", "313131"}, + } + for _, kv := range kvs { + nt.TryUpdate(common.FromHex(kv.K), common.FromHex(kv.V)) + st.TryUpdate(common.FromHex(kv.K), common.FromHex(kv.V)) + } + if nt.Hash() != st.Hash() { + t.Fatalf("error %x != %x", st.Hash(), nt.Hash()) + } +} diff --git a/trie/trie_test.go b/trie/trie_test.go index c33d660a89..8dda9d5b8d 100644 --- a/trie/trie_test.go +++ b/trie/trie_test.go @@ -605,6 +605,42 @@ func TestCommitSequenceStackTrie(t *testing.T) { } } +// TestCommitSequenceSmallRoot tests that a trie which is essentially only a +// small (<32 byte) shortnode with an included value is properly committed to a +// database. +// This case might not matter, since in practice, all keys are 32 bytes, which means +// that even a small trie which contains a leaf will have an extension making it +// not fit into 32 bytes, rlp-encoded. However, it's still the correct thing to do. +func TestCommitSequenceSmallRoot(t *testing.T) { + s := &spongeDb{sponge: sha3.NewLegacyKeccak256(), id: "a"} + db := NewDatabase(s) + trie, _ := New(common.Hash{}, db) + // Another sponge is used for the stacktrie commits + stackTrieSponge := &spongeDb{sponge: sha3.NewLegacyKeccak256(), id: "b"} + stTrie := NewStackTrie(stackTrieSponge) + // Add a single small-element to the trie(s) + key := make([]byte, 5) + key[0] = 1 + trie.TryUpdate(key, []byte{0x1}) + stTrie.TryUpdate(key, []byte{0x1}) + // Flush trie -> database + root, _ := trie.Commit(nil) + // Flush memdb -> disk (sponge) + db.Commit(root, false) + // And flush stacktrie -> disk + 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) + } + fmt.Printf("root: %x\n", stRoot) + if got, exp := stackTrieSponge.sponge.Sum(nil), s.sponge.Sum(nil); !bytes.Equal(got, exp) { + t.Fatalf("test, disk write sequence wrong:\ngot %x exp %x\n", got, exp) + } +} + // BenchmarkCommitAfterHashFixedSize benchmarks the Commit (after Hash) of a fixed number of updates to a trie. // This benchmark is meant to capture the difference on efficiency of small versus large changes. Typically, // storage tries are small (a couple of entries), whereas the full post-block account trie update is large (a couple