From 053ed9cc847647a9b3ef707d0efe7104c4ab2a4c Mon Sep 17 00:00:00 2001 From: gary rong Date: Wed, 30 Sep 2020 19:45:56 +0800 Subject: [PATCH] trie: polishes to trie committer (#21351) * trie: update tests to check commit integrity * trie: polish committer * trie: fix typo * trie: remove hasvalue notion According to the benchmarks, type assertion between the pointer and interface is extremely fast. BenchmarkIntmethod-12 1000000000 1.91 ns/op BenchmarkInterface-12 1000000000 2.13 ns/op BenchmarkTypeSwitch-12 1000000000 1.81 ns/op BenchmarkTypeAssertion-12 2000000000 1.78 ns/op So the overhead for asserting whether the shortnode has "valuenode" child is super tiny. No necessary to have another field. * trie: linter nitpicks Co-authored-by: Martin Holst Swende --- trie/committer.go | 118 +++++++++++++++++++--------------------- trie/hasher.go | 4 +- trie/trie.go | 5 +- trie/trie_test.go | 134 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 196 insertions(+), 65 deletions(-) diff --git a/trie/committer.go b/trie/committer.go index fc8b7ceda..20c95bed0 100644 --- a/trie/committer.go +++ b/trie/committer.go @@ -23,7 +23,6 @@ import ( "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/crypto" - "github.com/ethereum/go-ethereum/rlp" "golang.org/x/crypto/sha3" ) @@ -33,10 +32,9 @@ const leafChanSize = 200 // leaf represents a trie leaf value type leaf struct { - size int // size of the rlp data (estimate) - hash common.Hash // hash of rlp data - node node // the node to commit - vnodes bool // set to true if the node (possibly) contains a valueNode + size int // size of the rlp data (estimate) + hash common.Hash // hash of rlp data + node node // the node to commit } // committer is a type used for the trie Commit operation. A committer has some @@ -74,18 +72,12 @@ func returnCommitterToPool(h *committer) { committerPool.Put(h) } -// commitNeeded returns 'false' if the given node is already in sync with db -func (c *committer) commitNeeded(n node) bool { - hash, dirty := n.cache() - return hash == nil || dirty -} - // commit collapses a node down into a hash node and inserts it into the database func (c *committer) Commit(n node, db *Database) (hashNode, error) { if db == nil { return nil, errors.New("no db provided") } - h, err := c.commit(n, db, true) + h, err := c.commit(n, db) if err != nil { return nil, err } @@ -93,7 +85,7 @@ func (c *committer) Commit(n node, db *Database) (hashNode, error) { } // commit collapses a node down into a hash node and inserts it into the database -func (c *committer) commit(n node, db *Database, force bool) (node, error) { +func (c *committer) commit(n node, db *Database) (node, error) { // if this path is clean, use available cached data hash, dirty := n.cache() if hash != nil && !dirty { @@ -104,8 +96,11 @@ func (c *committer) commit(n node, db *Database, force bool) (node, error) { case *shortNode: // Commit child collapsed := cn.copy() - if _, ok := cn.Val.(valueNode); !ok { - childV, err := c.commit(cn.Val, db, false) + + // If the child is fullnode, recursively commit. + // Otherwise it can only be hashNode or valueNode. + if _, ok := cn.Val.(*fullNode); ok { + childV, err := c.commit(cn.Val, db) if err != nil { return nil, err } @@ -113,78 +108,78 @@ func (c *committer) commit(n node, db *Database, force bool) (node, error) { } // The key needs to be copied, since we're delivering it to database collapsed.Key = hexToCompact(cn.Key) - hashedNode := c.store(collapsed, db, force, true) + hashedNode := c.store(collapsed, db) if hn, ok := hashedNode.(hashNode); ok { return hn, nil } return collapsed, nil case *fullNode: - hashedKids, hasVnodes, err := c.commitChildren(cn, db, force) + hashedKids, err := c.commitChildren(cn, db) if err != nil { return nil, err } collapsed := cn.copy() collapsed.Children = hashedKids - hashedNode := c.store(collapsed, db, force, hasVnodes) + hashedNode := c.store(collapsed, db) if hn, ok := hashedNode.(hashNode); ok { return hn, nil } return collapsed, nil - case valueNode: - return c.store(cn, db, force, false), nil - // hashnodes aren't stored case hashNode: return cn, nil + default: + // nil, valuenode shouldn't be committed + panic(fmt.Sprintf("%T: invalid node: %v", n, n)) } - return hash, nil } // commitChildren commits the children of the given fullnode -func (c *committer) commitChildren(n *fullNode, db *Database, force bool) ([17]node, bool, error) { +func (c *committer) commitChildren(n *fullNode, db *Database) ([17]node, error) { var children [17]node - var hasValueNodeChildren = false - for i, child := range n.Children { + for i := 0; i < 16; i++ { + child := n.Children[i] if child == nil { continue } - hnode, err := c.commit(child, db, false) + // If it's the hashed child, save the hash value directly. + // Note: it's impossible that the child in range [0, 15] + // is a valuenode. + if hn, ok := child.(hashNode); ok { + children[i] = hn + continue + } + // Commit the child recursively and store the "hashed" value. + // Note the returned node can be some embedded nodes, so it's + // possible the type is not hashnode. + hashed, err := c.commit(child, db) if err != nil { - return children, false, err - } - children[i] = hnode - if _, ok := hnode.(valueNode); ok { - hasValueNodeChildren = true + return children, err } + children[i] = hashed } - return children, hasValueNodeChildren, nil + // For the 17th child, it's possible the type is valuenode. + if n.Children[16] != nil { + children[16] = n.Children[16] + } + return children, nil } // store hashes the node n and if we have a storage layer specified, it writes // the key/value pair to it and tracks any node->child references as well as any // node->external trie references. -func (c *committer) store(n node, db *Database, force bool, hasVnodeChildren bool) node { +func (c *committer) store(n node, db *Database) node { // Larger nodes are replaced by their hash and stored in the database. var ( hash, _ = n.cache() size int ) if hash == nil { - if vn, ok := n.(valueNode); ok { - c.tmp.Reset() - if err := rlp.Encode(&c.tmp, vn); err != nil { - panic("encode error: " + err.Error()) - } - size = len(c.tmp) - if size < 32 && !force { - return n // Nodes smaller than 32 bytes are stored inside their parent - } - hash = c.makeHashNode(c.tmp) - } else { - // This was not generated - must be a small node stored in the parent - // No need to do anything here - return n - } + // This was not generated - must be a small node stored in the parent. + // In theory we should apply the leafCall here if it's not nil(embedded + // node usually contains value). But small value(less than 32bytes) is + // not our target. + return n } else { // We have the hash already, estimate the RLP encoding-size of the node. // The size is used for mem tracking, does not need to be exact @@ -194,10 +189,9 @@ func (c *committer) store(n node, db *Database, force bool, hasVnodeChildren boo // The leaf channel will be active only when there an active leaf-callback if c.leafCh != nil { c.leafCh <- &leaf{ - size: size, - hash: common.BytesToHash(hash), - node: n, - vnodes: hasVnodeChildren, + size: size, + hash: common.BytesToHash(hash), + node: n, } } else if db != nil { // No leaf-callback used, but there's still a database. Do serial @@ -209,30 +203,30 @@ func (c *committer) store(n node, db *Database, force bool, hasVnodeChildren boo return hash } -// commitLoop does the actual insert + leaf callback for nodes +// commitLoop does the actual insert + leaf callback for nodes. func (c *committer) commitLoop(db *Database) { for item := range c.leafCh { var ( - hash = item.hash - size = item.size - n = item.node - hasVnodes = item.vnodes + hash = item.hash + size = item.size + n = item.node ) // We are pooling the trie nodes into an intermediate memory cache db.lock.Lock() db.insert(hash, size, n) db.lock.Unlock() - if c.onleaf != nil && hasVnodes { + + if c.onleaf != nil { switch n := n.(type) { case *shortNode: if child, ok := n.Val.(valueNode); ok { c.onleaf(nil, child, hash) } case *fullNode: - for i := 0; i < 16; i++ { - if child, ok := n.Children[i].(valueNode); ok { - c.onleaf(nil, child, hash) - } + // For children in range [0, 15], it's impossible + // to contain valuenode. Only check the 17th child. + if n.Children[16] != nil { + c.onleaf(nil, n.Children[16].(valueNode), hash) } } } diff --git a/trie/hasher.go b/trie/hasher.go index 57cd3e1f3..3a62a2f11 100644 --- a/trie/hasher.go +++ b/trie/hasher.go @@ -66,11 +66,11 @@ func returnHasherToPool(h *hasher) { // hash collapses a node down into a hash node, also returning a copy of the // original node initialized with the computed hash to replace the original one. func (h *hasher) hash(n node, force bool) (hashed node, cached node) { - // We're not storing the node, just hashing, use available cached data + // Return the cached hash if it's available if hash, _ := n.cache(); hash != nil { return hash, n } - // Trie not processed yet or needs storage, walk the children + // Trie not processed yet, walk the children switch n := n.(type) { case *shortNode: collapsed, cached := h.hashShortNodeChildren(n) diff --git a/trie/trie.go b/trie/trie.go index 1e1749a4f..6ddbbd78d 100644 --- a/trie/trie.go +++ b/trie/trie.go @@ -505,13 +505,16 @@ func (t *Trie) Commit(onleaf LeafCallback) (root common.Hash, err error) { if t.root == nil { return emptyRoot, nil } + // Derive the hash for all dirty nodes first. We hold the assumption + // in the following procedure that all nodes are hashed. rootHash := t.Hash() h := newCommitter() defer returnCommitterToPool(h) + // Do a quick check if we really need to commit, before we spin // up goroutines. This can happen e.g. if we load a trie for reading storage // values, but don't write to it. - if !h.commitNeeded(t.root) { + if _, dirty := t.root.cache(); !dirty { return rootHash, nil } var wg sync.WaitGroup diff --git a/trie/trie_test.go b/trie/trie_test.go index 2356b7a74..6a32f2436 100644 --- a/trie/trie_test.go +++ b/trie/trie_test.go @@ -19,7 +19,9 @@ package trie import ( "bytes" "encoding/binary" + "errors" "fmt" + "hash" "io/ioutil" "math/big" "math/rand" @@ -31,9 +33,11 @@ import ( "github.com/davecgh/go-spew/spew" "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/crypto" + "github.com/ethereum/go-ethereum/ethdb" "github.com/ethereum/go-ethereum/ethdb/leveldb" "github.com/ethereum/go-ethereum/ethdb/memorydb" "github.com/ethereum/go-ethereum/rlp" + "golang.org/x/crypto/sha3" ) func init() { @@ -659,6 +663,136 @@ func makeAccounts(size int) (addresses [][20]byte, accounts [][]byte) { return addresses, accounts } +// spongeDb is a dummy db backend which accumulates writes in a sponge +type spongeDb struct { + sponge hash.Hash +} + +func (s *spongeDb) Has(key []byte) (bool, error) { panic("implement me") } +func (s *spongeDb) Get(key []byte) ([]byte, error) { return nil, errors.New("no such elem") } +func (s *spongeDb) Delete(key []byte) error { panic("implement me") } +func (s *spongeDb) NewBatch() ethdb.Batch { return &spongeBatch{s} } +func (s *spongeDb) Stat(property string) (string, error) { panic("implement me") } +func (s *spongeDb) Compact(start []byte, limit []byte) error { panic("implement me") } +func (s *spongeDb) Close() error { return nil } +func (s *spongeDb) Put(key []byte, value []byte) error { + s.sponge.Write(key) + s.sponge.Write(value) + return nil +} +func (s *spongeDb) NewIterator(prefix []byte, start []byte) ethdb.Iterator { panic("implement me") } + +// spongeBatch is a dummy batch which immediately writes to the underlying spongedb +type spongeBatch struct { + db *spongeDb +} + +func (b *spongeBatch) Put(key, value []byte) error { + b.db.Put(key, value) + return nil +} +func (b *spongeBatch) Delete(key []byte) error { panic("implement me") } +func (b *spongeBatch) ValueSize() int { return 100 } +func (b *spongeBatch) Write() error { return nil } +func (b *spongeBatch) Reset() {} +func (b *spongeBatch) Replay(w ethdb.KeyValueWriter) error { return nil } + +// TestCommitSequence tests that the trie.Commit operation writes the elements of the trie +// in the expected order, and calls the callbacks in the expected order. +// The test data was based on the 'master' code, and is basically random. It can be used +// to check whether changes to the trie modifies the write order or data in any way. +func TestCommitSequence(t *testing.T) { + for i, tc := range []struct { + count int + expWriteSeqHash []byte + expCallbackSeqHash []byte + }{ + {20, common.FromHex("68c495e45209e243eb7e4f4e8ca8f9f7be71003bd9cafb8061b4534373740193"), + common.FromHex("01783213033d6b7781a641ab499e680d959336d025ac16f44d02f4f0c021bbf5")}, + {200, common.FromHex("3b20d16c13c4bc3eb3b8d0ad7a169fef3b1600e056c0665895d03d3d2b2ff236"), + common.FromHex("fb8db0ec82e8f02729f11228940885b181c3047ab0d654ed0110291ca57111a8")}, + {2000, common.FromHex("34eff3d1048bebdf77e9ae8bd939f2e7c742edc3dcd1173cff1aad9dbd20451a"), + common.FromHex("1c981604b1a9f8ffa40e0ae66b14830a87f5a4ed8345146a3912e6b2dcb05e63")}, + } { + addresses, accounts := makeAccounts(tc.count) + // This spongeDb is used to check the sequence of disk-db-writes + s := &spongeDb{sponge: sha3.NewLegacyKeccak256()} + db := NewDatabase(s) + trie, _ := New(common.Hash{}, db) + // Another sponge is used to check the callback-sequence + callbackSponge := sha3.NewLegacyKeccak256() + // Fill the trie with elements + for i := 0; i < tc.count; i++ { + trie.Update(crypto.Keccak256(addresses[i][:]), accounts[i]) + } + // Flush trie -> database + root, _ := trie.Commit(nil) + // Flush memdb -> disk (sponge) + db.Commit(root, false, func(c common.Hash) { + // And spongify the callback-order + callbackSponge.Write(c[:]) + }) + if got, exp := s.sponge.Sum(nil), tc.expWriteSeqHash; !bytes.Equal(got, exp) { + t.Fatalf("test %d, disk write sequence wrong:\ngot %x exp %x\n", i, got, exp) + } + if got, exp := callbackSponge.Sum(nil), tc.expCallbackSeqHash; !bytes.Equal(got, exp) { + t.Fatalf("test %d, call back sequence wrong:\ngot: %x exp %x\n", i, got, exp) + } + } +} + +// TestCommitSequenceRandomBlobs is identical to TestCommitSequence +// but uses random blobs instead of 'accounts' +func TestCommitSequenceRandomBlobs(t *testing.T) { + for i, tc := range []struct { + count int + expWriteSeqHash []byte + expCallbackSeqHash []byte + }{ + {20, common.FromHex("8e4a01548551d139fa9e833ebc4e66fc1ba40a4b9b7259d80db32cff7b64ebbc"), + common.FromHex("450238d73bc36dc6cc6f926987e5428535e64be403877c4560e238a52749ba24")}, + {200, common.FromHex("6869b4e7b95f3097a19ddb30ff735f922b915314047e041614df06958fc50554"), + common.FromHex("0ace0b03d6cb8c0b82f6289ef5b1a1838306b455a62dafc63cada8e2924f2550")}, + {2000, common.FromHex("444200e6f4e2df49f77752f629a96ccf7445d4698c164f962bbd85a0526ef424"), + common.FromHex("117d30dafaa62a1eed498c3dfd70982b377ba2b46dd3e725ed6120c80829e518")}, + } { + prng := rand.New(rand.NewSource(int64(i))) + // This spongeDb is used to check the sequence of disk-db-writes + s := &spongeDb{sponge: sha3.NewLegacyKeccak256()} + db := NewDatabase(s) + trie, _ := New(common.Hash{}, db) + // Another sponge is used to check the callback-sequence + callbackSponge := sha3.NewLegacyKeccak256() + // Fill the trie with elements + for i := 0; i < tc.count; i++ { + key := make([]byte, 32) + var val []byte + // 50% short elements, 50% large elements + if prng.Intn(2) == 0 { + val = make([]byte, 1+prng.Intn(32)) + } else { + val = make([]byte, 1+prng.Intn(4096)) + } + prng.Read(key) + prng.Read(val) + trie.Update(key, val) + } + // Flush trie -> database + root, _ := trie.Commit(nil) + // Flush memdb -> disk (sponge) + db.Commit(root, false, func(c common.Hash) { + // And spongify the callback-order + callbackSponge.Write(c[:]) + }) + if got, exp := s.sponge.Sum(nil), tc.expWriteSeqHash; !bytes.Equal(got, exp) { + t.Fatalf("test %d, disk write sequence wrong:\ngot %x exp %x\n", i, got, exp) + } + if got, exp := callbackSponge.Sum(nil), tc.expCallbackSeqHash; !bytes.Equal(got, exp) { + t.Fatalf("test %d, call back sequence wrong:\ngot: %x exp %x\n", i, 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