trie, core/state: fix review concerns

core/state/state_object.go

@@ -273,8 +273,6 @@ func (s *stateObject) finalise() {
 
 // updateTrie writes cached storage modifications into the object's storage trie.
 // It will return nil if the trie has not been loaded and no changes have been made
-// Note: It may return non-nil if the trie is already loaded due to previous changes
-// in the same block
 func (s *stateObject) updateTrie(db Database) Trie {
 	// Make sure all dirty slots are finalized into the pending storage area
 	s.finalise()
@@ -310,8 +308,8 @@ func (s *stateObject) updateTrie(db Database) Trie {
 
 // UpdateRoot sets the trie root to the current root hash of
 func (s *stateObject) updateRoot(db Database) {
+	// If nothing changed, don't bother with hashing anything
 	if s.updateTrie(db) == nil {
-		// No changes, storage trie is not even loaded
 		return
 	}
 	// Track the amount of time wasted on hashing the storge trie
@@ -324,8 +322,8 @@ func (s *stateObject) updateRoot(db Database) {
 // CommitTrie the storage trie of the object to db.
 // This updates the trie root.
 func (s *stateObject) CommitTrie(db Database) error {
+	// If nothing changed, don't bother with hashing anything
 	if s.updateTrie(db) == nil {
-		// No changes, storage trie is not even loaded
 		return nil
 	}
 	if s.dbErr != nil {

trie/committer.go

@@ -26,9 +26,9 @@ import (
 	"golang.org/x/crypto/sha3"
 )
 
-// LeafChanSize is the size of the leafCh. It's a pretty arbitrary number, to allow
+// leafChanSize is the size of the leafCh. It's a pretty arbitrary number, to allow
 // some paralellism but not incur too much memory overhead.
-const LeafChanSize = 200
+const leafChanSize = 200
 
 // Leaf represents a trie leaf value
 type Leaf struct {
@@ -52,7 +52,7 @@ type committer struct {
 	leafCh chan *Leaf
 }
 
-// committers live in a global db.
+// committers live in a global sync.Pool
 var committerPool = sync.Pool{
 	New: func() interface{} {
 		return &committer{
@@ -62,19 +62,9 @@ var committerPool = sync.Pool{
 	},
 }
 
-// newCommitter creates a new committer or picks one from the pool, and
+// newCommitter creates a new committer or picks one from the pool.
-// initializes the leafCh, if needed.
+func newCommitter() *committer {
-// In case no onleaf-callback is provided, the committer does not
+	return committerPool.Get().(*committer)
-// use a channel-based commit, but inlined.
-// Typically, the account trie is committed with a channel-based leaf-commit,
-// whereas storage tries are committed 'inline'.
-func newCommitter(onleaf LeafCallback) *committer {
-	h := committerPool.Get().(*committer)
-	h.onleaf = onleaf
-	if onleaf != nil {
-		h.leafCh = make(chan *Leaf, LeafChanSize)
-	}
-	return h
 }
 
 func returnCommitterToPool(h *committer) {
@@ -84,14 +74,13 @@ func returnCommitterToPool(h *committer) {
 }
 
 // commitNeeded returns 'false' if the given node is already in sync with db
-func (h *committer) commitNeeded(n node) bool {
+func (c *committer) commitNeeded(n node) bool {
 	hash, dirty := n.cache()
 	return hash == nil || dirty
 }
 
-// hash collapses a node down into a hash node, also returning a copy of the
+// commit collapses a node down into a hash node and inserts it into the database
-// original node initialized with the computed hash to replace the original one.
+func (c *committer) commit(n node, db *Database, force bool) (node, error) {
-func (h *committer) commit(n node, db *Database, force bool) (node, error) {
 	// If we're not storing the node, just hashing, use available cached data
 	hash, dirty := n.cache()
 	if hash != nil && !dirty {
@@ -100,14 +89,13 @@ func (h *committer) commit(n node, db *Database, force bool) (node, error) {
 	if db == nil {
 		return nil, errors.New("no db provided")
 	}
-	// Commit children. then parent
+	// Commit children, then parent, and remove remove the dirty flag.
-	// Remove the dirty flag.
 	switch cn := n.(type) {
 	case *shortNode:
 		// Commit child
 		collapsed := cn.copy()
 		if _, ok := cn.Val.(valueNode); !ok {
-			if childV, err := h.commit(cn.Val, db, false); err != nil {
+			if childV, err := c.commit(cn.Val, db, false); err != nil {
 				return nil, err
 			} else {
 				collapsed.Val = childV
@@ -115,7 +103,7 @@ func (h *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 := h.store(collapsed, db, force, true)
+		hashedNode := c.store(collapsed, db, force, true)
 		if hn, ok := hashedNode.(hashNode); ok {
 			cn.flags.dirty = false
 			return hn, nil
@@ -123,14 +111,14 @@ func (h *committer) commit(n node, db *Database, force bool) (node, error) {
 			return collapsed, nil
 		}
 	case *fullNode:
-		hashedKids, hasVnodes, err := h.commitChildren(cn, db, force)
+		hashedKids, hasVnodes, err := c.commitChildren(cn, db, force)
 		if err != nil {
 			return nil, err
 		}
 		collapsed := cn.copy()
 		collapsed.Children = hashedKids
 
-		hashedNode := h.store(collapsed, db, force, hasVnodes)
+		hashedNode := c.store(collapsed, db, force, hasVnodes)
 		if hn, ok := hashedNode.(hashNode); ok {
 			cn.flags.dirty = false
 			return hn, nil
@@ -138,7 +126,7 @@ func (h *committer) commit(n node, db *Database, force bool) (node, error) {
 			return collapsed, nil
 		}
 	case valueNode:
-		return h.store(cn, db, force, false), nil
+		return c.store(cn, db, force, false), nil
 	// hashnodes aren't stored
 	case hashNode:
 		return cn, nil
@@ -147,14 +135,14 @@ func (h *committer) commit(n node, db *Database, force bool) (node, error) {
 }
 
 // commitChildren commits the children of the given fullnode
-func (h *committer) commitChildren(n *fullNode, db *Database, force bool) ([17]node, bool, error) {
+func (c *committer) commitChildren(n *fullNode, db *Database, force bool) ([17]node, bool, error) {
 	var children [17]node
 	var hasValueNodeChildren = false
 	for i, child := range n.Children {
 		if child == nil {
 			continue
 		}
-		hnode, err := h.commit(child, db, false)
+		hnode, err := c.commit(child, db, false)
 		if err != nil {
 			return children, false, err
 		}
@@ -169,7 +157,7 @@ func (h *committer) commitChildren(n *fullNode, db *Database, force bool) ([17]n
 // 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 (h *committer) store(n node, db *Database, force bool, hasVnodeChildren bool) node {
+func (c *committer) store(n node, db *Database, force bool, hasVnodeChildren bool) node {
 	// Larger nodes are replaced by their hash and stored in the database.
 	var (
 		hash, _ = n.cache()
@@ -177,15 +165,15 @@ func (h *committer) store(n node, db *Database, force bool, hasVnodeChildren boo
 	)
 	if hash == nil {
 		if vn, ok := n.(valueNode); ok {
-			h.tmp.Reset()
+			c.tmp.Reset()
-			if err := rlp.Encode(&h.tmp, vn); err != nil {
+			if err := rlp.Encode(&c.tmp, vn); err != nil {
 				panic("encode error: " + err.Error())
 			}
-			size = len(h.tmp)
+			size = len(c.tmp)
 			if size < 32 && !force {
 				return n // Nodes smaller than 32 bytes are stored inside their parent
 			}
-			hash = h.makeHashNode(h.tmp)
+			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
@@ -198,8 +186,8 @@ func (h *committer) store(n node, db *Database, force bool, hasVnodeChildren boo
 	}
 	// If we're using channel-based leaf-reporting, send to channel.
 	// The leaf channel will be active only when there an active leaf-callback
-	if h.leafCh != nil {
+	if c.leafCh != nil {
-		h.leafCh <- &Leaf{
+		c.leafCh <- &Leaf{
 			size:   size,
 			hash:   common.BytesToHash(hash),
 			node:   n,
@@ -216,8 +204,8 @@ func (h *committer) store(n node, db *Database, force bool, hasVnodeChildren boo
 }
 
 // commitLoop does the actual insert + leaf callback for nodes
-func (h *committer) commitLoop(db *Database) {
+func (c *committer) commitLoop(db *Database) {
-	for item := range h.leafCh {
+	for item := range c.leafCh {
 		var (
 			hash      = item.hash
 			size      = item.size
@@ -228,16 +216,16 @@ func (h *committer) commitLoop(db *Database) {
 		db.lock.Lock()
 		db.insert(hash, size, n)
 		db.lock.Unlock()
-		if h.onleaf != nil && hasVnodes {
+		if c.onleaf != nil && hasVnodes {
 			switch n := n.(type) {
 			case *shortNode:
 				if child, ok := n.Val.(valueNode); ok {
-					h.onleaf(child, hash)
+					c.onleaf(child, hash)
 				}
 			case *fullNode:
 				for i := 0; i < 16; i++ {
 					if child, ok := n.Children[i].(valueNode); ok {
-						h.onleaf(child, hash)
+						c.onleaf(child, hash)
 					}
 				}
 			}
@@ -245,11 +233,11 @@ func (h *committer) commitLoop(db *Database) {
 	}
 }
 
-func (h *committer) makeHashNode(data []byte) hashNode {
+func (c *committer) makeHashNode(data []byte) hashNode {
-	n := make(hashNode, h.sha.Size())
+	n := make(hashNode, c.sha.Size())
-	h.sha.Reset()
+	c.sha.Reset()
-	h.sha.Write(data)
+	c.sha.Write(data)
-	h.sha.Read(n)
+	c.sha.Read(n)
 	return n
 }
 

trie/hasher.go

@@ -47,9 +47,7 @@ func (b *sliceBuffer) Reset() {
 // internal preallocated temp space
 type hasher struct {
 	sha keccakState
-
 	tmp sliceBuffer
-	tmpKey []byte
 }
 
 // hasherPool holds pureHashers
@@ -57,7 +55,6 @@ var hasherPool = sync.Pool{
 	New: func() interface{} {
 		return &hasher{
 			tmp: make(sliceBuffer, 0, 550), // cap is as large as a full fullNode.
-			tmpKey: make([]byte, 64),          // space for an packed key
 			sha: sha3.NewLegacyKeccak256().(keccakState),
 		}
 	},

trie/trie.go

@@ -420,7 +420,7 @@ func (t *Trie) Commit(onleaf LeafCallback) (root common.Hash, err error) {
 		return emptyRoot, nil
 	}
 	rootHash := t.Hash()
-	h := newCommitter(onleaf)
+	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
@@ -430,6 +430,8 @@ func (t *Trie) Commit(onleaf LeafCallback) (root common.Hash, err error) {
 	}
 	var wg sync.WaitGroup
 	if onleaf != nil {
+		h.onleaf = onleaf
+		h.leafCh = make(chan *Leaf, leafChanSize)
 		wg.Add(1)
 		go func() {
 			defer wg.Done()