diff --git a/core/state/snapshot/difflayer.go b/core/state/snapshot/difflayer.go index 0915fb6bc6..86ca5c8ba0 100644 --- a/core/state/snapshot/difflayer.go +++ b/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 } diff --git a/core/state/snapshot/difflayer_test.go b/core/state/snapshot/difflayer_test.go index 61d2ed9c0b..329e0eb8e2 100644 --- a/core/state/snapshot/difflayer_test.go +++ b/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) - child := newDiffLayer(parent, common.Hash{}, destructs, accounts, storage) - child = newDiffLayer(child, common.Hash{}, destructs, accounts, storage) - child = newDiffLayer(child, common.Hash{}, destructs, accounts, storage) - child = newDiffLayer(child, common.Hash{}, destructs, accounts, storage) + parent := newDiffLayer(emptyLayer(), common.Hash{}, copyDestructs(destructs), copyAccounts(accounts), copyStorage(storage)) + child := newDiffLayer(parent, common.Hash{}, copyDestructs(destructs), copyAccounts(accounts), copyStorage(storage)) + child = newDiffLayer(child, common.Hash{}, copyDestructs(destructs), copyAccounts(accounts), copyStorage(storage)) + child = newDiffLayer(child, common.Hash{}, copyDestructs(destructs), copyAccounts(accounts), copyStorage(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 { - t.Errorf("accountList wrong, got %v exp %v", got, exp) + if have, want := len(merged.accountList), 0; have != want { + t.Errorf("accountList wrong: have %v, want %v", have, want) } - if got, exp := len(merged.AccountList()), len(accounts); got != exp { - t.Errorf("AccountList() wrong, got %v exp %v", got, exp) + if have, want := len(merged.AccountList()), len(accounts); have != want { + t.Errorf("AccountList() wrong: have %v, want %v", have, want) } - if got, exp := len(merged.accountList), len(accounts); got != exp { - t.Errorf("accountList [2] wrong, got %v exp %v", got, exp) + if have, want := len(merged.accountList), len(accounts); have != want { + t.Errorf("accountList [2] wrong: have %v, want %v", have, want) } } { // Check account drops - if got, exp := len(merged.destructSet), len(destructs); got != exp { - t.Errorf("accountDrop wrong, got %v exp %v", got, exp) + if have, want := len(merged.destructSet), len(destructs); have != want { + 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 { - t.Errorf("[1] storageList wrong, got %v exp %v", got, exp) + if have, want := len(merged.storageList), i; have != want { + t.Errorf("[1] storageList wrong: have %v, want %v", have, want) } - if got, exp := len(merged.StorageList(aHash)), len(sMap); got != exp { - t.Errorf("[2] StorageList() wrong, got %v exp %v", got, exp) + if have, want := len(merged.StorageList(aHash)), len(sMap); have != want { + t.Errorf("[2] StorageList() wrong: have %v, want %v", have, want) } - if got, exp := len(merged.storageList[aHash]), len(sMap); got != exp { - t.Errorf("storageList wrong, got %v exp %v", got, exp) + if have, want := len(merged.storageList[aHash]), len(sMap); have != want { + 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 { - // t.Errorf("mem wrong, got %d, exp %d", got, exp) + //if have, want := merged.memory, child.memory; have != want { + // 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) - if exp := []byte{0x01}; !bytes.Equal(got, exp) { - t.Errorf("merged slot value wrong, got %x, exp %x", got, exp) + have, _ := merged.Storage(acc, slot) + if want := []byte{0x01}; !bytes.Equal(have, want) { + t.Errorf("merged slot value wrong: have %x, want %x", have, want) } } } diff --git a/core/state/snapshot/iterator.go b/core/state/snapshot/iterator.go index 774e9f5542..f0b1eafa99 100644 --- a/core/state/snapshot/iterator.go +++ b/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() diff --git a/core/state/snapshot/iterator_test.go b/core/state/snapshot/iterator_test.go index 935fafc2f5..5468a9a589 100644 --- a/core/state/snapshot/iterator_test.go +++ b/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 - snaps.Update(common.HexToHash("0x03"), common.HexToHash("0x02"), set, nil) + destructed := map[common.Hash]struct{}{ + 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{})