core/state/snapshot: fix storage issue

If an account is deleted in the tx_1 but recreated in the tx_2,
the it can happen that in this diff layer, both destructedSet
and storageData records this account. In this case, the storage
iterator should be able to iterate the slots belong to new account
but disable further iteration in deeper layers(belong to old account)

core/state/snapshot/difflayer.go

@@ -105,6 +105,13 @@ type diffLayer struct {
 	root  common.Hash // Root hash to which this snapshot diff belongs to
 	stale uint32      // Signals that the layer became stale (state progressed)
 
+	// destructSet is a very special helper marker. If an account is marked as
+	// deleted, then it's recorded in this set. However it's allowed that an account
+	// is included here but still available in other sets(e.g. storageData). The
+	// reason is the diff layer includes all the changes in a *block*. It can
+	// happen that in the tx_1, account A is self-destructed while in the tx_2
+	// it's recreated. But we still need this marker to indicate the "old" A is
+	// deleted, all data in other set belongs to the "new" A.
 	destructSet map[common.Hash]struct{}               // Keyed markers for deleted (and potentially) recreated accounts
 	accountList []common.Hash                          // List of account for iteration. If it exists, it's sorted, otherwise it's nil
 	accountData map[common.Hash][]byte                 // Keyed accounts for direct retrival (nil means deleted)
@@ -510,18 +517,18 @@ func (dl *diffLayer) AccountList() []common.Hash {
 // for the given account. If the whole storage is destructed in this layer, then
 // an additional flag *destructed = true* will be returned, otherwise the flag is
 // false. Besides, the returned list will include the hash of deleted storage slot.
+// Note a special case is an account is deleted in a prior tx but is recreated in
+// the following tx with some storage slots set. In this case the returned list is
+// not empty but the flag is true.
 //
 // Note, the returned slice is not a copy, so do not modify it.
 func (dl *diffLayer) StorageList(accountHash common.Hash) ([]common.Hash, bool) {
 	// If an old list already exists, return it
 	dl.lock.RLock()
-	if _, exist := dl.destructSet[accountHash]; exist {
+	_, destructed := dl.destructSet[accountHash]
-		dl.lock.RUnlock()
-		return nil, true
-	}
 	if list, exist := dl.storageList[accountHash]; exist {
 		dl.lock.RUnlock()
-		return list, false // all cached lists are still alive, even if they are empty.
+		return list, destructed // The list might be nil
 	}
 	dl.lock.RUnlock()
 
@@ -529,9 +536,6 @@ func (dl *diffLayer) StorageList(accountHash common.Hash) ([]common.Hash, bool)
 	dl.lock.Lock()
 	defer dl.lock.Unlock()
 
-	// Otherwise allocate the sorted storage and return. Note even there is zero
-	// storage change included in this layer, the returned slice is not **nil**.
-	// Nil slice represents the whole storage is removed.
 	storageMap := dl.storageData[accountHash]
 	storageList := make([]common.Hash, 0, len(storageMap))
 	for k := range storageMap {
@@ -539,6 +543,6 @@ func (dl *diffLayer) StorageList(accountHash common.Hash) ([]common.Hash, bool)
 	}
 	sort.Sort(hashes(storageList))
 	dl.storageList[accountHash] = storageList
-	dl.memory += uint64(len(dl.storageList) * common.HashLength)
+	dl.memory += uint64(len(dl.storageList)*common.HashLength + common.HashLength)
-	return storageList, false
+	return storageList, destructed
 }

core/state/snapshot/iterator.go

@@ -243,15 +243,13 @@ type diffStorageIterator struct {
 // StorageIterator creates a storage iterator over a single diff layer.
 // Execept the storage iterator is returned, there is an additional flag
 // "destructed" returned. If it's true then it means the whole storage is
-// destructed.
+// destructed in this layer(maybe recreated too), don't bother deeper layer
+// for storage retrieval.
 func (dl *diffLayer) StorageIterator(account common.Hash, seek common.Hash) (StorageIterator, bool) {
-	// If the storage is destructed, return nil iterator.
+	// Create the storage for this account even it's marked
+	// as destructed. The iterator is for the new one which
+	// just has the same adddress as the deleted one.
 	hashes, destructed := dl.StorageList(account)
-	if destructed {
-		return nil, true
-	}
-	// Otherwise, create the storage iterator even there is
-	// zero storage change included(the exhausted iterator).
 	index := sort.Search(len(hashes), func(i int) bool {
 		return bytes.Compare(seek[:], hashes[i][:]) <= 0
 	})
@@ -260,7 +258,7 @@ func (dl *diffLayer) StorageIterator(account common.Hash, seek common.Hash) (Sto
 		layer:   dl,
 		account: account,
 		keys:    hashes[index:],
-	}, false
+	}, destructed
 }
 
 // Next steps the iterator forward one element, returning false if exhausted.

core/state/snapshot/iterator_binary.go

@@ -67,20 +67,9 @@ func (dl *diffLayer) initBinaryAccountIterator() Iterator {
 func (dl *diffLayer) initBinaryStorageIterator(account common.Hash) Iterator {
 	parent, ok := dl.parent.(*diffLayer)
 	if !ok {
-		// If the storage in this layer is already destructed, discard
+		// If the storage in this layer is already destructed, discard all
-		// all deeper layers and return an exhausted iterator.
+		// deeper layers but still return an valid single-branch iterator.
 		a, destructed := dl.StorageIterator(account, common.Hash{})
-		if destructed {
-			return &binaryIterator{
-				aDone:   true,
-				bDone:   true,
-				account: account,
-			}
-		}
-		// If the storage in the parent layer is destructed,
-		// return a single-branch iterator with another branch
-		// set as exhausted.
-		b, destructed := dl.Parent().StorageIterator(account, common.Hash{})
 		if destructed {
 			l := &binaryIterator{
 				a:       a,
@@ -90,7 +79,9 @@ func (dl *diffLayer) initBinaryStorageIterator(account common.Hash) Iterator {
 			l.bDone = true
 			return l
 		}
-		// Both branches are still available, return the binary iterator.
+		// Even if the storage in the parent layer is destructed,
+		// still return the normal iterator with both-branch enabled.
+		b, _ := dl.Parent().StorageIterator(account, common.Hash{})
 		l := &binaryIterator{
 			a:       a,
 			b:       b,
@@ -100,15 +91,17 @@ func (dl *diffLayer) initBinaryStorageIterator(account common.Hash) Iterator {
 		l.bDone = !l.b.Next()
 		return l
 	}
-	// If the storage in this layer is already destructed, discard
+	// If the storage in this layer is already destructed, discard all
-	// all deeper layers and return an exhausted iterator.
+	// deeper layers but still return an valid single-branch iterator.
 	a, destructed := dl.StorageIterator(account, common.Hash{})
 	if destructed {
-		return &binaryIterator{
+		l := &binaryIterator{
-			aDone:   true,
+			a:       a,
-			bDone:   true,
 			account: account,
 		}
+		l.aDone = !l.a.Next()
+		l.bDone = true
+		return l
 	}
 	l := &binaryIterator{
 		a:       a,

core/state/snapshot/iterator_fast.go

@@ -97,15 +97,17 @@ func newFastIterator(tree *Tree, root common.Hash, account common.Hash, seek com
 			})
 		} else {
 			// If the whole storage is destructed in this layer, don't
-			// bother deeper layer anymore.
+			// bother deeper layer anymore. But we should still keep
+			// the iterator for this layer, since the iterator can contain
+			// some valid slots which belongs to the re-created account.
 			it, destructed := current.StorageIterator(account, seek)
-			if destructed {
-				break
-			}
 			fi.iterators = append(fi.iterators, &weightedIterator{
 				it:       it,
 				priority: depth,
 			})
+			if destructed {
+				break
+			}
 		}
 		current = current.Parent()
 	}

core/state/snapshot/iterator_test.go

@@ -806,6 +806,14 @@ func TestStorageIteratorDeletions(t *testing.T) {
 	it, _ = snaps.StorageIterator(common.HexToHash("0x05"), common.HexToHash("0xaa"), common.Hash{})
 	verifyIterator(t, 3, it, verifyStorage)
 	it.Release()
+
+	// Destruct the whole storage but re-create the account in the same layer
+	snaps.Update(common.HexToHash("0x06"), common.HexToHash("0x05"), destructed, randomAccountSet("0xaa"), randomStorageSet([]string{"0xaa"}, [][]string{{"0x11", "0x12"}}, nil))
+	it, _ = snaps.StorageIterator(common.HexToHash("0x06"), common.HexToHash("0xaa"), common.Hash{})
+	verifyIterator(t, 2, it, verifyStorage) // The output should be 11,12
+	it.Release()
+
+	verifyIterator(t, 2, snaps.Snapshot(common.HexToHash("0x06")).(*diffLayer).newBinaryStorageIterator(common.HexToHash("0xaa")), verifyStorage)
 }
 
 // BenchmarkAccountIteratorTraversal is a bit a bit notorious -- all layers contain the