core/state/snapshot: fix various iteration issues due to destruct set

core/state/snapshot/difflayer.go

@@ -493,12 +493,14 @@ func (dl *diffLayer) AccountList() []common.Hash {
 	defer dl.lock.Unlock()
 
 	dl.accountList = make([]common.Hash, 0, len(dl.destructSet)+len(dl.accountData))
-	for hash := range dl.destructSet {
-		dl.accountList = append(dl.accountList, hash)
-	}
 	for hash := range dl.accountData {
 		dl.accountList = append(dl.accountList, hash)
 	}
+	for hash := range dl.destructSet {
+		if _, ok := dl.accountData[hash]; !ok {
+			dl.accountList = append(dl.accountList, hash)
+		}
+	}
 	sort.Sort(hashes(dl.accountList))
 	return dl.accountList
 }

core/state/snapshot/difflayer_test.go

@@ -27,6 +27,33 @@ import (
 	"github.com/ethereum/go-ethereum/ethdb/memorydb"
 )
 
+func copyDestructs(destructs map[common.Hash]struct{}) map[common.Hash]struct{} {
+	copy := make(map[common.Hash]struct{})
+	for hash := range destructs {
+		copy[hash] = struct{}{}
+	}
+	return copy
+}
+
+func copyAccounts(accounts map[common.Hash][]byte) map[common.Hash][]byte {
+	copy := make(map[common.Hash][]byte)
+	for hash, blob := range accounts {
+		copy[hash] = blob
+	}
+	return copy
+}
+
+func copyStorage(storage map[common.Hash]map[common.Hash][]byte) map[common.Hash]map[common.Hash][]byte {
+	copy := make(map[common.Hash]map[common.Hash][]byte)
+	for accHash, slots := range storage {
+		copy[accHash] = make(map[common.Hash][]byte)
+		for slotHash, blob := range slots {
+			copy[accHash][slotHash] = blob
+		}
+	}
+	return copy
+}
+
 // TestMergeBasics tests some simple merges
 func TestMergeBasics(t *testing.T) {
 	var (
@@ -52,41 +79,41 @@ func TestMergeBasics(t *testing.T) {
 		}
 	}
 	// Add some (identical) layers on top
-	parent := newDiffLayer(emptyLayer(), common.Hash{}, destructs, accounts, storage)
+	parent := newDiffLayer(emptyLayer(), common.Hash{}, copyDestructs(destructs), copyAccounts(accounts), copyStorage(storage))
-	child := newDiffLayer(parent, common.Hash{}, destructs, accounts, storage)
+	child := newDiffLayer(parent, common.Hash{}, copyDestructs(destructs), copyAccounts(accounts), copyStorage(storage))
-	child = newDiffLayer(child, common.Hash{}, destructs, accounts, storage)
+	child = newDiffLayer(child, common.Hash{}, copyDestructs(destructs), copyAccounts(accounts), copyStorage(storage))
-	child = newDiffLayer(child, common.Hash{}, destructs, accounts, storage)
+	child = newDiffLayer(child, common.Hash{}, copyDestructs(destructs), copyAccounts(accounts), copyStorage(storage))
-	child = newDiffLayer(child, common.Hash{}, destructs, accounts, storage)
+	child = newDiffLayer(child, common.Hash{}, copyDestructs(destructs), copyAccounts(accounts), copyStorage(storage))
 	// And flatten
 	merged := (child.flatten()).(*diffLayer)
 
 	{ // Check account lists
-		if got, exp := len(merged.accountList), 0; got != exp {
+		if have, want := len(merged.accountList), 0; have != want {
-			t.Errorf("accountList wrong, got %v exp %v", got, exp)
+			t.Errorf("accountList wrong: have %v, want %v", have, want)
 		}
-		if got, exp := len(merged.AccountList()), len(accounts); got != exp {
+		if have, want := len(merged.AccountList()), len(accounts); have != want {
-			t.Errorf("AccountList() wrong, got %v exp %v", got, exp)
+			t.Errorf("AccountList() wrong: have %v, want %v", have, want)
 		}
-		if got, exp := len(merged.accountList), len(accounts); got != exp {
+		if have, want := len(merged.accountList), len(accounts); have != want {
-			t.Errorf("accountList [2] wrong, got %v exp %v", got, exp)
+			t.Errorf("accountList [2] wrong: have %v, want %v", have, want)
 		}
 	}
 	{ // Check account drops
-		if got, exp := len(merged.destructSet), len(destructs); got != exp {
+		if have, want := len(merged.destructSet), len(destructs); have != want {
-			t.Errorf("accountDrop wrong, got %v exp %v", got, exp)
+			t.Errorf("accountDrop wrong: have %v, want %v", have, want)
 		}
 	}
 	{ // Check storage lists
 		i := 0
 		for aHash, sMap := range storage {
-			if got, exp := len(merged.storageList), i; got != exp {
+			if have, want := len(merged.storageList), i; have != want {
-				t.Errorf("[1] storageList wrong, got %v exp %v", got, exp)
+				t.Errorf("[1] storageList wrong: have %v, want %v", have, want)
 			}
-			if got, exp := len(merged.StorageList(aHash)), len(sMap); got != exp {
+			if have, want := len(merged.StorageList(aHash)), len(sMap); have != want {
-				t.Errorf("[2] StorageList() wrong, got %v exp %v", got, exp)
+				t.Errorf("[2] StorageList() wrong: have %v, want %v", have, want)
 			}
-			if got, exp := len(merged.storageList[aHash]), len(sMap); got != exp {
+			if have, want := len(merged.storageList[aHash]), len(sMap); have != want {
-				t.Errorf("storageList wrong, got %v exp %v", got, exp)
+				t.Errorf("storageList wrong: have %v, want %v", have, want)
 			}
 			i++
 		}
@@ -160,8 +187,8 @@ func TestMergeDelete(t *testing.T) {
 	}
 	// If we add more granular metering of memory, we can enable this again,
 	// but it's not implemented for now
-	//if got, exp := merged.memory, child.memory; got != exp {
+	//if have, want := merged.memory, child.memory; have != want {
-	//	t.Errorf("mem wrong, got %d, exp %d", got, exp)
+	//	t.Errorf("mem wrong: have %d, want %d", have, want)
 	//}
 }
 
@@ -197,9 +224,9 @@ func TestInsertAndMerge(t *testing.T) {
 	// And flatten
 	merged := (child.flatten()).(*diffLayer)
 	{ // Check that slot value is present
-		got, _ := merged.Storage(acc, slot)
+		have, _ := merged.Storage(acc, slot)
-		if exp := []byte{0x01}; !bytes.Equal(got, exp) {
+		if want := []byte{0x01}; !bytes.Equal(have, want) {
-			t.Errorf("merged slot value wrong, got %x, exp %x", got, exp)
+			t.Errorf("merged slot value wrong: have %x, want %x", have, want)
 		}
 	}
 }

core/state/snapshot/iterator.go

@@ -60,12 +60,6 @@ type diffAccountIterator struct {
 	// hash as long as the iterator is not touched any more.
 	curHash common.Hash
 
-	// curAccount is the current value the iterator is positioned on. The field
-	// is explicitly tracked since the referenced diff layer might go stale after
-	// the iterator was positioned and we don't want to fail accessing the old
-	// value as long as the iterator is not touched any more.
-	//curAccount []byte
-
 	layer *diffLayer    // Live layer to retrieve values from
 	keys  []common.Hash // Keys left in the layer to iterate
 	fail  error         // Any failures encountered (stale)
@@ -130,6 +124,9 @@ func (it *diffAccountIterator) Account() []byte {
 	it.layer.lock.RLock()
 	blob, ok := it.layer.accountData[it.curHash]
 	if !ok {
+		if _, ok := it.layer.destructSet[it.curHash]; ok {
+			return nil
+		}
 		panic(fmt.Sprintf("iterator referenced non-existent account: %x", it.curHash))
 	}
 	it.layer.lock.RUnlock()

core/state/snapshot/iterator_test.go

@@ -53,7 +53,7 @@ func TestAccountIteratorBasics(t *testing.T) {
 		}
 	}
 	// Add some (identical) layers on top
-	parent := newDiffLayer(emptyLayer(), common.Hash{}, destructs, accounts, storage)
+	parent := newDiffLayer(emptyLayer(), common.Hash{}, copyDestructs(destructs), copyAccounts(accounts), copyStorage(storage))
 	it := parent.AccountIterator(common.Hash{})
 	verifyIterator(t, 100, it)
 }
@@ -398,15 +398,17 @@ func TestIteratorDeletions(t *testing.T) {
 	}
 	// Stack three diff layers on top with various overlaps
 	snaps.Update(common.HexToHash("0x02"), common.HexToHash("0x01"),
-		randomAccountSet("0x11", "0x22", "0x33"), nil)
+		nil, randomAccountSet("0x11", "0x22", "0x33"), nil)
 
-	set := randomAccountSet("0x11", "0x22", "0x33")
 	deleted := common.HexToHash("0x22")
-	set[deleted] = nil
+	destructed := map[common.Hash]struct{}{
-	snaps.Update(common.HexToHash("0x03"), common.HexToHash("0x02"), set, nil)
+		deleted: struct{}{},
+	}
+	snaps.Update(common.HexToHash("0x03"), common.HexToHash("0x02"),
+		destructed, randomAccountSet("0x11", "0x33"), nil)
 
 	snaps.Update(common.HexToHash("0x04"), common.HexToHash("0x03"),
-		randomAccountSet("0x33", "0x44", "0x55"), nil)
+		nil, randomAccountSet("0x33", "0x44", "0x55"), nil)
 
 	// The output should be 11,33,44,55
 	it, _ := snaps.AccountIterator(common.HexToHash("0x04"), common.Hash{})