From 261029b29a407bf5cc7fa3049e2d18340819b3ac Mon Sep 17 00:00:00 2001 From: rjl493456442 Date: Sun, 26 Apr 2020 19:29:52 +0800 Subject: [PATCH] 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 | 24 +++++++++++--------- core/state/snapshot/iterator.go | 14 +++++------- core/state/snapshot/iterator_binary.go | 31 ++++++++++---------------- core/state/snapshot/iterator_fast.go | 10 +++++---- core/state/snapshot/iterator_test.go | 8 +++++++ 5 files changed, 46 insertions(+), 41 deletions(-) diff --git a/core/state/snapshot/difflayer.go b/core/state/snapshot/difflayer.go index 4abbdcc46f..00a2f32064 100644 --- a/core/state/snapshot/difflayer.go +++ b/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 { - dl.lock.RUnlock() - return nil, true - } + _, destructed := dl.destructSet[accountHash] 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) - return storageList, false + dl.memory += uint64(len(dl.storageList)*common.HashLength + common.HashLength) + return storageList, destructed } diff --git a/core/state/snapshot/iterator.go b/core/state/snapshot/iterator.go index e123d94752..f180402df8 100644 --- a/core/state/snapshot/iterator.go +++ b/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. diff --git a/core/state/snapshot/iterator_binary.go b/core/state/snapshot/iterator_binary.go index a836c8c8dc..30ccff09dc 100644 --- a/core/state/snapshot/iterator_binary.go +++ b/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 - // all deeper layers and return an exhausted iterator. + // If the storage in this layer is already destructed, discard all + // 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 - // all deeper layers and return an exhausted iterator. + // If the storage in this layer is already destructed, discard all + // 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, + l := &binaryIterator{ + a: a, account: account, } + l.aDone = !l.a.Next() + l.bDone = true + return l } l := &binaryIterator{ a: a, diff --git a/core/state/snapshot/iterator_fast.go b/core/state/snapshot/iterator_fast.go index d686f3e55a..ef3a27ac95 100644 --- a/core/state/snapshot/iterator_fast.go +++ b/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() } diff --git a/core/state/snapshot/iterator_test.go b/core/state/snapshot/iterator_test.go index b067e13f4a..504e8e672b 100644 --- a/core/state/snapshot/iterator_test.go +++ b/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