From 8d56bf5ceb74a7ed45c986450848a89e2df61189 Mon Sep 17 00:00:00 2001
From: Felix Lange <fjl@twurst.com>
Date: Mon, 17 Oct 2016 23:01:29 +0200
Subject: trie: ensure dirty flag is unset for embedded child nodes

This was caught by the new invariant check.
---
 trie/hasher.go    | 31 ++++++++++++++-----------------
 trie/trie_test.go | 43 ++++++++++++++++++++++++++++---------------
 2 files changed, 42 insertions(+), 32 deletions(-)

(limited to 'trie')

diff --git a/trie/hasher.go b/trie/hasher.go
index 57e156ebf..b6223bf32 100644
--- a/trie/hasher.go
+++ b/trie/hasher.go
@@ -75,23 +75,20 @@ func (h *hasher) hash(n node, db DatabaseWriter, force bool) (node, node, error)
 	if err != nil {
 		return hashNode{}, n, err
 	}
-	// Cache the hash of the ndoe for later reuse.
-	if hash, ok := hashed.(hashNode); ok && !force {
-		switch cached := cached.(type) {
-		case *shortNode:
-			cached = cached.copy()
-			cached.flags.hash = hash
-			if db != nil {
-				cached.flags.dirty = false
-			}
-			return hashed, cached, nil
-		case *fullNode:
-			cached = cached.copy()
-			cached.flags.hash = hash
-			if db != nil {
-				cached.flags.dirty = false
-			}
-			return hashed, cached, nil
+	// Cache the hash of the ndoe for later reuse and remove
+	// the dirty flag in commit mode. It's fine to assign these values directly
+	// without copying the node first because hashChildren copies it.
+	cachedHash, _ := hashed.(hashNode)
+	switch cn := cached.(type) {
+	case *shortNode:
+		cn.flags.hash = cachedHash
+		if db != nil {
+			cn.flags.dirty = false
+		}
+	case *fullNode:
+		cn.flags.hash = cachedHash
+		if db != nil {
+			cn.flags.dirty = false
 		}
 	}
 	return hashed, cached, nil
diff --git a/trie/trie_test.go b/trie/trie_test.go
index da0d2360b..14ac5a666 100644
--- a/trie/trie_test.go
+++ b/trie/trie_test.go
@@ -462,31 +462,44 @@ func runRandTest(rt randTest) bool {
 				return false
 			}
 		case opCheckCacheInvariant:
-			return checkCacheInvariant(tr.root, tr.cachegen, 0)
+			return checkCacheInvariant(tr.root, nil, tr.cachegen, false, 0)
 		}
 	}
 	return true
 }
 
-func checkCacheInvariant(n node, parentCachegen uint16, depth int) bool {
+func checkCacheInvariant(n, parent node, parentCachegen uint16, parentDirty bool, depth int) bool {
+	var children []node
+	var flag nodeFlag
 	switch n := n.(type) {
 	case *shortNode:
-		if n.flags.gen > parentCachegen {
-			fmt.Printf("cache invariant violation: %d > %d\nat depth %d node %s", n.flags.gen, parentCachegen, depth, spew.Sdump(n))
-			return false
-		}
-		return checkCacheInvariant(n.Val, n.flags.gen, depth+1)
+		flag = n.flags
+		children = []node{n.Val}
 	case *fullNode:
-		if n.flags.gen > parentCachegen {
-			fmt.Printf("cache invariant violation: %d > %d\nat depth %d node %s", n.flags.gen, parentCachegen, depth, spew.Sdump(n))
+		flag = n.flags
+		children = n.Children[:]
+	default:
+		return true
+	}
+
+	showerror := func() {
+		fmt.Printf("at depth %d node %s", depth, spew.Sdump(n))
+		fmt.Printf("parent: %s", spew.Sdump(parent))
+	}
+	if flag.gen > parentCachegen {
+		fmt.Printf("cache invariant violation: %d > %d\n", flag.gen, parentCachegen)
+		showerror()
+		return false
+	}
+	if depth > 0 && !parentDirty && flag.dirty {
+		fmt.Printf("cache invariant violation: child is dirty but parent isn't\n")
+		showerror()
+		return false
+	}
+	for _, child := range children {
+		if !checkCacheInvariant(child, n, flag.gen, flag.dirty, depth+1) {
 			return false
 		}
-		for _, child := range n.Children {
-			if !checkCacheInvariant(child, n.flags.gen, depth+1) {
-				return false
-			}
-		}
-		return true
 	}
 	return true
 }
-- 
cgit