From dc13e14647e54df3ed699a2076c31b5026c806da Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Thu, 14 Nov 2024 14:13:24 +0100 Subject: [PATCH 1/5] core/state: tests on the binary iterator --- core/state/snapshot/iterator_test.go | 129 ++++++++++++++++++++++----- 1 file changed, 105 insertions(+), 24 deletions(-) diff --git a/core/state/snapshot/iterator_test.go b/core/state/snapshot/iterator_test.go index daa8cdcc543a..9fdc56d033f8 100644 --- a/core/state/snapshot/iterator_test.go +++ b/core/state/snapshot/iterator_test.go @@ -58,6 +58,9 @@ func TestAccountIteratorBasics(t *testing.T) { it := diffLayer.AccountIterator(common.Hash{}) verifyIterator(t, 100, it, verifyNothing) // Nil is allowed for single layer iterator + it = diffLayer.newBinaryAccountIterator() + verifyIterator(t, 100, it, verifyNothing) // Nil is allowed for single layer iterator + diskLayer := diffToDisk(diffLayer) it = diskLayer.AccountIterator(common.Hash{}) verifyIterator(t, 100, it, verifyNothing) // Nil is allowed for single layer iterator @@ -96,6 +99,9 @@ func TestStorageIteratorBasics(t *testing.T) { for account := range accounts { it, _ := diffLayer.StorageIterator(account, common.Hash{}) verifyIterator(t, 100, it, verifyNothing) // Nil is allowed for single layer iterator + + it = diffLayer.newBinaryStorageIterator(account) + verifyIterator(t, 100, it, verifyNothing) // Nil is allowed for single layer iterator } diskLayer := diffToDisk(diffLayer) @@ -555,6 +561,21 @@ func TestAccountIteratorLargeTraversal(t *testing.T) { // - flattens C2 all the way into CN // - continues iterating func TestAccountIteratorFlattening(t *testing.T) { + t.Run("fast", func(t *testing.T) { + testAccountIteratorFlattening(t, func(snaps *Tree, root, seek common.Hash) AccountIterator { + it, _ := snaps.AccountIterator(root, seek) + return it + }) + }) + t.Run("binary", func(t *testing.T) { + testAccountIteratorFlattening(t, func(snaps *Tree, root, seek common.Hash) AccountIterator { + return snaps.layers[root].(*diffLayer).newBinaryAccountIterator() + + }) + }) +} + +func testAccountIteratorFlattening(t *testing.T, newIterator func(snaps *Tree, root, seek common.Hash) AccountIterator) { // Create an empty base layer and a snapshot tree out of it base := &diskLayer{ diskdb: rawdb.NewMemoryDatabase(), @@ -577,7 +598,7 @@ func TestAccountIteratorFlattening(t *testing.T) { randomAccountSet("0xcc", "0xf0", "0xff"), nil) // Create an iterator and flatten the data from underneath it - it, _ := snaps.AccountIterator(common.HexToHash("0x04"), common.Hash{}) + it := newIterator(snaps, common.HexToHash("0x04"), common.Hash{}) defer it.Release() if err := snaps.Cap(common.HexToHash("0x04"), 1); err != nil { @@ -587,6 +608,21 @@ func TestAccountIteratorFlattening(t *testing.T) { } func TestAccountIteratorSeek(t *testing.T) { + t.Run("fast", func(t *testing.T) { + testAccountIteratorSeek(t, func(snaps *Tree, root, seek common.Hash) AccountIterator { + it, _ := snaps.AccountIterator(root, seek) + return it + }) + }) + t.Run("binary", func(t *testing.T) { + testAccountIteratorSeek(t, func(snaps *Tree, root, seek common.Hash) AccountIterator { + return snaps.layers[root].(*diffLayer).newBinaryAccountIterator() + + }) + }) +} + +func testAccountIteratorSeek(t *testing.T, newIterator func(snaps *Tree, root, seek common.Hash) AccountIterator) { // Create a snapshot stack with some initial data base := &diskLayer{ diskdb: rawdb.NewMemoryDatabase(), @@ -612,44 +648,58 @@ func TestAccountIteratorSeek(t *testing.T) { // 03: aa, bb, dd, ee, f0 (, f0), ff // 04: aa, bb, cc, dd, ee, f0 (, f0), ff (, ff) // Construct various iterators and ensure their traversal is correct - it, _ := snaps.AccountIterator(common.HexToHash("0x02"), common.HexToHash("0xdd")) + it := newIterator(snaps, common.HexToHash("0x02"), common.HexToHash("0xdd")) defer it.Release() verifyIterator(t, 3, it, verifyAccount) // expected: ee, f0, ff - it, _ = snaps.AccountIterator(common.HexToHash("0x02"), common.HexToHash("0xaa")) + it = newIterator(snaps, common.HexToHash("0x02"), common.HexToHash("0xaa")) defer it.Release() verifyIterator(t, 4, it, verifyAccount) // expected: aa, ee, f0, ff - it, _ = snaps.AccountIterator(common.HexToHash("0x02"), common.HexToHash("0xff")) + it = newIterator(snaps, common.HexToHash("0x02"), common.HexToHash("0xff")) defer it.Release() verifyIterator(t, 1, it, verifyAccount) // expected: ff - it, _ = snaps.AccountIterator(common.HexToHash("0x02"), common.HexToHash("0xff1")) + it = newIterator(snaps, common.HexToHash("0x02"), common.HexToHash("0xff1")) defer it.Release() verifyIterator(t, 0, it, verifyAccount) // expected: nothing - it, _ = snaps.AccountIterator(common.HexToHash("0x04"), common.HexToHash("0xbb")) + it = newIterator(snaps, common.HexToHash("0x04"), common.HexToHash("0xbb")) defer it.Release() verifyIterator(t, 6, it, verifyAccount) // expected: bb, cc, dd, ee, f0, ff - it, _ = snaps.AccountIterator(common.HexToHash("0x04"), common.HexToHash("0xef")) + it = newIterator(snaps, common.HexToHash("0x04"), common.HexToHash("0xef")) defer it.Release() verifyIterator(t, 2, it, verifyAccount) // expected: f0, ff - it, _ = snaps.AccountIterator(common.HexToHash("0x04"), common.HexToHash("0xf0")) + it = newIterator(snaps, common.HexToHash("0x04"), common.HexToHash("0xf0")) defer it.Release() verifyIterator(t, 2, it, verifyAccount) // expected: f0, ff - it, _ = snaps.AccountIterator(common.HexToHash("0x04"), common.HexToHash("0xff")) + it = newIterator(snaps, common.HexToHash("0x04"), common.HexToHash("0xff")) defer it.Release() verifyIterator(t, 1, it, verifyAccount) // expected: ff - it, _ = snaps.AccountIterator(common.HexToHash("0x04"), common.HexToHash("0xff1")) + it = newIterator(snaps, common.HexToHash("0x04"), common.HexToHash("0xff1")) defer it.Release() verifyIterator(t, 0, it, verifyAccount) // expected: nothing } func TestStorageIteratorSeek(t *testing.T) { + t.Run("fast", func(t *testing.T) { + testStorageIteratorSeek(t, func(snaps *Tree, root, account, seek common.Hash) StorageIterator { + it, _ := snaps.StorageIterator(root, account, seek) + return it + }) + }) + t.Run("binary", func(t *testing.T) { + testStorageIteratorSeek(t, func(snaps *Tree, root, account, seek common.Hash) StorageIterator { + return snaps.layers[root].(*diffLayer).newBinaryStorageIterator(account) + }) + }) +} + +func testStorageIteratorSeek(t *testing.T, newIterator func(snaps *Tree, root, account, seek common.Hash) StorageIterator) { // Create a snapshot stack with some initial data base := &diskLayer{ diskdb: rawdb.NewMemoryDatabase(), @@ -676,35 +726,35 @@ func TestStorageIteratorSeek(t *testing.T) { // 03: 01, 02, 03, 05 (, 05), 06 // 04: 01(, 01), 02, 03, 05(, 05, 05), 06, 08 // Construct various iterators and ensure their traversal is correct - it, _ := snaps.StorageIterator(common.HexToHash("0x02"), common.HexToHash("0xaa"), common.HexToHash("0x01")) + it := newIterator(snaps, common.HexToHash("0x02"), common.HexToHash("0xaa"), common.HexToHash("0x01")) defer it.Release() verifyIterator(t, 3, it, verifyStorage) // expected: 01, 03, 05 - it, _ = snaps.StorageIterator(common.HexToHash("0x02"), common.HexToHash("0xaa"), common.HexToHash("0x02")) + it = newIterator(snaps, common.HexToHash("0x02"), common.HexToHash("0xaa"), common.HexToHash("0x02")) defer it.Release() verifyIterator(t, 2, it, verifyStorage) // expected: 03, 05 - it, _ = snaps.StorageIterator(common.HexToHash("0x02"), common.HexToHash("0xaa"), common.HexToHash("0x5")) + it = newIterator(snaps, common.HexToHash("0x02"), common.HexToHash("0xaa"), common.HexToHash("0x5")) defer it.Release() verifyIterator(t, 1, it, verifyStorage) // expected: 05 - it, _ = snaps.StorageIterator(common.HexToHash("0x02"), common.HexToHash("0xaa"), common.HexToHash("0x6")) + it = newIterator(snaps, common.HexToHash("0x02"), common.HexToHash("0xaa"), common.HexToHash("0x6")) defer it.Release() verifyIterator(t, 0, it, verifyStorage) // expected: nothing - it, _ = snaps.StorageIterator(common.HexToHash("0x04"), common.HexToHash("0xaa"), common.HexToHash("0x01")) + it = newIterator(snaps, common.HexToHash("0x04"), common.HexToHash("0xaa"), common.HexToHash("0x01")) defer it.Release() verifyIterator(t, 6, it, verifyStorage) // expected: 01, 02, 03, 05, 06, 08 - it, _ = snaps.StorageIterator(common.HexToHash("0x04"), common.HexToHash("0xaa"), common.HexToHash("0x05")) + it = newIterator(snaps, common.HexToHash("0x04"), common.HexToHash("0xaa"), common.HexToHash("0x05")) defer it.Release() verifyIterator(t, 3, it, verifyStorage) // expected: 05, 06, 08 - it, _ = snaps.StorageIterator(common.HexToHash("0x04"), common.HexToHash("0xaa"), common.HexToHash("0x08")) + it = newIterator(snaps, common.HexToHash("0x04"), common.HexToHash("0xaa"), common.HexToHash("0x08")) defer it.Release() verifyIterator(t, 1, it, verifyStorage) // expected: 08 - it, _ = snaps.StorageIterator(common.HexToHash("0x04"), common.HexToHash("0xaa"), common.HexToHash("0x09")) + it = newIterator(snaps, common.HexToHash("0x04"), common.HexToHash("0xaa"), common.HexToHash("0x09")) defer it.Release() verifyIterator(t, 0, it, verifyStorage) // expected: nothing } @@ -713,6 +763,21 @@ func TestStorageIteratorSeek(t *testing.T) { // deleted accounts (where the Account() value is nil). The iterator // should not output any accounts or nil-values for those cases. func TestAccountIteratorDeletions(t *testing.T) { + t.Run("fast", func(t *testing.T) { + testAccountIteratorDeletions(t, func(snaps *Tree, root, seek common.Hash) AccountIterator { + it, _ := snaps.AccountIterator(root, seek) + return it + }) + }) + t.Run("binary", func(t *testing.T) { + testAccountIteratorDeletions(t, func(snaps *Tree, root, seek common.Hash) AccountIterator { + return snaps.layers[root].(*diffLayer).newBinaryAccountIterator() + + }) + }) +} + +func testAccountIteratorDeletions(t *testing.T, newIterator func(snaps *Tree, root, seek common.Hash) AccountIterator) { // Create an empty base layer and a snapshot tree out of it base := &diskLayer{ diskdb: rawdb.NewMemoryDatabase(), @@ -739,7 +804,8 @@ func TestAccountIteratorDeletions(t *testing.T) { nil, randomAccountSet("0x33", "0x44", "0x55"), nil) // The output should be 11,33,44,55 - it, _ := snaps.AccountIterator(common.HexToHash("0x04"), common.Hash{}) + it := newIterator(snaps, common.HexToHash("0x04"), (common.Hash{})) + // Do a quick check verifyIterator(t, 4, it, verifyAccount) it.Release() @@ -759,6 +825,20 @@ func TestAccountIteratorDeletions(t *testing.T) { } func TestStorageIteratorDeletions(t *testing.T) { + t.Run("fast", func(t *testing.T) { + testStorageIteratorDeletions(t, func(snaps *Tree, root, account, seek common.Hash) StorageIterator { + it, _ := snaps.StorageIterator(root, account, seek) + return it + }) + }) + t.Run("binary", func(t *testing.T) { + testStorageIteratorDeletions(t, func(snaps *Tree, root, account, seek common.Hash) StorageIterator { + return snaps.layers[root].(*diffLayer).newBinaryStorageIterator(account) + }) + }) +} + +func testStorageIteratorDeletions(t *testing.T, newIterator func(snaps *Tree, root, account, seek common.Hash) StorageIterator) { // Create an empty base layer and a snapshot tree out of it base := &diskLayer{ diskdb: rawdb.NewMemoryDatabase(), @@ -778,12 +858,12 @@ func TestStorageIteratorDeletions(t *testing.T) { randomAccountSet("0xaa"), randomStorageSet([]string{"0xaa"}, [][]string{{"0x02", "0x04", "0x06"}}, [][]string{{"0x01", "0x03"}})) // The output should be 02,04,05,06 - it, _ := snaps.StorageIterator(common.HexToHash("0x03"), common.HexToHash("0xaa"), common.Hash{}) + it := newIterator(snaps, common.HexToHash("0x03"), common.HexToHash("0xaa"), common.Hash{}) verifyIterator(t, 4, it, verifyStorage) it.Release() // The output should be 04,05,06 - it, _ = snaps.StorageIterator(common.HexToHash("0x03"), common.HexToHash("0xaa"), common.HexToHash("0x03")) + it = newIterator(snaps, common.HexToHash("0x03"), common.HexToHash("0xaa"), common.HexToHash("0x03")) verifyIterator(t, 3, it, verifyStorage) it.Release() @@ -793,7 +873,7 @@ func TestStorageIteratorDeletions(t *testing.T) { } snaps.Update(common.HexToHash("0x04"), common.HexToHash("0x03"), destructed, nil, nil) - it, _ = snaps.StorageIterator(common.HexToHash("0x04"), common.HexToHash("0xaa"), common.Hash{}) + it = newIterator(snaps, common.HexToHash("0x04"), common.HexToHash("0xaa"), common.Hash{}) verifyIterator(t, 0, it, verifyStorage) it.Release() @@ -802,13 +882,14 @@ func TestStorageIteratorDeletions(t *testing.T) { randomAccountSet("0xaa"), randomStorageSet([]string{"0xaa"}, [][]string{{"0x07", "0x08", "0x09"}}, nil)) // The output should be 07,08,09 - it, _ = snaps.StorageIterator(common.HexToHash("0x05"), common.HexToHash("0xaa"), common.Hash{}) + + it = newIterator(snaps, 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{}) + it = newIterator(snaps, common.HexToHash("0x06"), common.HexToHash("0xaa"), common.Hash{}) verifyIterator(t, 2, it, verifyStorage) // The output should be 11,12 it.Release() From e9fc202fa12499c2d942395984f92063f75970b7 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Thu, 14 Nov 2024 15:07:32 +0100 Subject: [PATCH 2/5] core/state/snapshot: fix binary iterator seeking --- core/state/snapshot/iterator_binary.go | 32 ++++++++++-------- core/state/snapshot/iterator_test.go | 47 ++++++++++++++------------ 2 files changed, 42 insertions(+), 37 deletions(-) diff --git a/core/state/snapshot/iterator_binary.go b/core/state/snapshot/iterator_binary.go index edf471213f4a..d6f84f2e34bf 100644 --- a/core/state/snapshot/iterator_binary.go +++ b/core/state/snapshot/iterator_binary.go @@ -39,12 +39,12 @@ type binaryIterator struct { // initBinaryAccountIterator creates a simplistic iterator to step over all the // accounts in a slow, but easily verifiable way. Note this function is used for // initialization, use `newBinaryAccountIterator` as the API. -func (dl *diffLayer) initBinaryAccountIterator() Iterator { +func (dl *diffLayer) initBinaryAccountIterator(seek common.Hash) Iterator { parent, ok := dl.parent.(*diffLayer) if !ok { l := &binaryIterator{ - a: dl.AccountIterator(common.Hash{}), - b: dl.Parent().AccountIterator(common.Hash{}), + a: dl.AccountIterator(seek), + b: dl.Parent().AccountIterator(seek), accountIterator: true, } l.aDone = !l.a.Next() @@ -52,8 +52,8 @@ func (dl *diffLayer) initBinaryAccountIterator() Iterator { return l } l := &binaryIterator{ - a: dl.AccountIterator(common.Hash{}), - b: parent.initBinaryAccountIterator(), + a: dl.AccountIterator(seek), + b: parent.initBinaryAccountIterator(seek), accountIterator: true, } l.aDone = !l.a.Next() @@ -64,12 +64,12 @@ func (dl *diffLayer) initBinaryAccountIterator() Iterator { // initBinaryStorageIterator creates a simplistic iterator to step over all the // storage slots in a slow, but easily verifiable way. Note this function is used // for initialization, use `newBinaryStorageIterator` as the API. -func (dl *diffLayer) initBinaryStorageIterator(account common.Hash) Iterator { +func (dl *diffLayer) initBinaryStorageIterator(account, seek common.Hash) Iterator { parent, ok := dl.parent.(*diffLayer) if !ok { // If the storage in this layer is already destructed, discard all // deeper layers but still return a valid single-branch iterator. - a, destructed := dl.StorageIterator(account, common.Hash{}) + a, destructed := dl.StorageIterator(account, seek) if destructed { l := &binaryIterator{ a: a, @@ -81,7 +81,7 @@ func (dl *diffLayer) initBinaryStorageIterator(account common.Hash) Iterator { } // The parent is disk layer, don't need to take care "destructed" // anymore. - b, _ := dl.Parent().StorageIterator(account, common.Hash{}) + b, _ := dl.Parent().StorageIterator(account, seek) l := &binaryIterator{ a: a, b: b, @@ -93,7 +93,7 @@ func (dl *diffLayer) initBinaryStorageIterator(account common.Hash) Iterator { } // If the storage in this layer is already destructed, discard all // deeper layers but still return a valid single-branch iterator. - a, destructed := dl.StorageIterator(account, common.Hash{}) + a, destructed := dl.StorageIterator(account, seek) if destructed { l := &binaryIterator{ a: a, @@ -105,7 +105,7 @@ func (dl *diffLayer) initBinaryStorageIterator(account common.Hash) Iterator { } l := &binaryIterator{ a: a, - b: parent.initBinaryStorageIterator(account), + b: parent.initBinaryStorageIterator(account, seek), account: account, } l.aDone = !l.a.Next() @@ -195,19 +195,21 @@ func (it *binaryIterator) Slot() []byte { // Release recursively releases all the iterators in the stack. func (it *binaryIterator) Release() { it.a.Release() - it.b.Release() + if it.b != nil { + it.b.Release() + } } // newBinaryAccountIterator creates a simplistic account iterator to step over // all the accounts in a slow, but easily verifiable way. -func (dl *diffLayer) newBinaryAccountIterator() AccountIterator { - iter := dl.initBinaryAccountIterator() +func (dl *diffLayer) newBinaryAccountIterator(seek common.Hash) AccountIterator { + iter := dl.initBinaryAccountIterator(seek) return iter.(AccountIterator) } // newBinaryStorageIterator creates a simplistic account iterator to step over // all the storage slots in a slow, but easily verifiable way. -func (dl *diffLayer) newBinaryStorageIterator(account common.Hash) StorageIterator { - iter := dl.initBinaryStorageIterator(account) +func (dl *diffLayer) newBinaryStorageIterator(account, seek common.Hash) StorageIterator { + iter := dl.initBinaryStorageIterator(account, seek) return iter.(StorageIterator) } diff --git a/core/state/snapshot/iterator_test.go b/core/state/snapshot/iterator_test.go index 9fdc56d033f8..752c78216c35 100644 --- a/core/state/snapshot/iterator_test.go +++ b/core/state/snapshot/iterator_test.go @@ -58,7 +58,7 @@ func TestAccountIteratorBasics(t *testing.T) { it := diffLayer.AccountIterator(common.Hash{}) verifyIterator(t, 100, it, verifyNothing) // Nil is allowed for single layer iterator - it = diffLayer.newBinaryAccountIterator() + it = diffLayer.newBinaryAccountIterator(common.Hash{}) verifyIterator(t, 100, it, verifyNothing) // Nil is allowed for single layer iterator diskLayer := diffToDisk(diffLayer) @@ -100,7 +100,7 @@ func TestStorageIteratorBasics(t *testing.T) { it, _ := diffLayer.StorageIterator(account, common.Hash{}) verifyIterator(t, 100, it, verifyNothing) // Nil is allowed for single layer iterator - it = diffLayer.newBinaryStorageIterator(account) + it = diffLayer.newBinaryStorageIterator(account, common.Hash{}) verifyIterator(t, 100, it, verifyNothing) // Nil is allowed for single layer iterator } @@ -241,7 +241,7 @@ func TestAccountIteratorTraversal(t *testing.T) { head := snaps.Snapshot(common.HexToHash("0x04")) verifyIterator(t, 3, head.(snapshot).AccountIterator(common.Hash{}), verifyNothing) - verifyIterator(t, 7, head.(*diffLayer).newBinaryAccountIterator(), verifyAccount) + verifyIterator(t, 7, head.(*diffLayer).newBinaryAccountIterator(common.Hash{}), verifyAccount) it, _ := snaps.AccountIterator(common.HexToHash("0x04"), common.Hash{}) verifyIterator(t, 7, it, verifyAccount) @@ -255,7 +255,7 @@ func TestAccountIteratorTraversal(t *testing.T) { }() aggregatorMemoryLimit = 0 // Force pushing the bottom-most layer into disk snaps.Cap(common.HexToHash("0x04"), 2) - verifyIterator(t, 7, head.(*diffLayer).newBinaryAccountIterator(), verifyAccount) + verifyIterator(t, 7, head.(*diffLayer).newBinaryAccountIterator(common.Hash{}), verifyAccount) it, _ = snaps.AccountIterator(common.HexToHash("0x04"), common.Hash{}) verifyIterator(t, 7, it, verifyAccount) @@ -289,7 +289,7 @@ func TestStorageIteratorTraversal(t *testing.T) { diffIter, _ := head.(snapshot).StorageIterator(common.HexToHash("0xaa"), common.Hash{}) verifyIterator(t, 3, diffIter, verifyNothing) - verifyIterator(t, 6, head.(*diffLayer).newBinaryStorageIterator(common.HexToHash("0xaa")), verifyStorage) + verifyIterator(t, 6, head.(*diffLayer).newBinaryStorageIterator(common.HexToHash("0xaa"), common.Hash{}), verifyStorage) it, _ := snaps.StorageIterator(common.HexToHash("0x04"), common.HexToHash("0xaa"), common.Hash{}) verifyIterator(t, 6, it, verifyStorage) @@ -303,7 +303,7 @@ func TestStorageIteratorTraversal(t *testing.T) { }() aggregatorMemoryLimit = 0 // Force pushing the bottom-most layer into disk snaps.Cap(common.HexToHash("0x04"), 2) - verifyIterator(t, 6, head.(*diffLayer).newBinaryStorageIterator(common.HexToHash("0xaa")), verifyStorage) + verifyIterator(t, 6, head.(*diffLayer).newBinaryStorageIterator(common.HexToHash("0xaa"), common.Hash{}), verifyStorage) it, _ = snaps.StorageIterator(common.HexToHash("0x04"), common.HexToHash("0xaa"), common.Hash{}) verifyIterator(t, 6, it, verifyStorage) @@ -534,7 +534,7 @@ func TestAccountIteratorLargeTraversal(t *testing.T) { // Iterate the entire stack and ensure everything is hit only once head := snaps.Snapshot(common.HexToHash("0x80")) verifyIterator(t, 200, head.(snapshot).AccountIterator(common.Hash{}), verifyNothing) - verifyIterator(t, 200, head.(*diffLayer).newBinaryAccountIterator(), verifyAccount) + verifyIterator(t, 200, head.(*diffLayer).newBinaryAccountIterator(common.Hash{}), verifyAccount) it, _ := snaps.AccountIterator(common.HexToHash("0x80"), common.Hash{}) verifyIterator(t, 200, it, verifyAccount) @@ -549,7 +549,7 @@ func TestAccountIteratorLargeTraversal(t *testing.T) { aggregatorMemoryLimit = 0 // Force pushing the bottom-most layer into disk snaps.Cap(common.HexToHash("0x80"), 2) - verifyIterator(t, 200, head.(*diffLayer).newBinaryAccountIterator(), verifyAccount) + verifyIterator(t, 200, head.(*diffLayer).newBinaryAccountIterator(common.Hash{}), verifyAccount) it, _ = snaps.AccountIterator(common.HexToHash("0x80"), common.Hash{}) verifyIterator(t, 200, it, verifyAccount) @@ -569,7 +569,7 @@ func TestAccountIteratorFlattening(t *testing.T) { }) t.Run("binary", func(t *testing.T) { testAccountIteratorFlattening(t, func(snaps *Tree, root, seek common.Hash) AccountIterator { - return snaps.layers[root].(*diffLayer).newBinaryAccountIterator() + return snaps.layers[root].(*diffLayer).newBinaryAccountIterator(common.Hash{}) }) }) @@ -616,8 +616,8 @@ func TestAccountIteratorSeek(t *testing.T) { }) t.Run("binary", func(t *testing.T) { testAccountIteratorSeek(t, func(snaps *Tree, root, seek common.Hash) AccountIterator { - return snaps.layers[root].(*diffLayer).newBinaryAccountIterator() - + it := snaps.layers[root].(*diffLayer).newBinaryAccountIterator(seek) + return it }) }) } @@ -694,7 +694,7 @@ func TestStorageIteratorSeek(t *testing.T) { }) t.Run("binary", func(t *testing.T) { testStorageIteratorSeek(t, func(snaps *Tree, root, account, seek common.Hash) StorageIterator { - return snaps.layers[root].(*diffLayer).newBinaryStorageIterator(account) + return snaps.layers[root].(*diffLayer).newBinaryStorageIterator(account, seek) }) }) } @@ -771,7 +771,7 @@ func TestAccountIteratorDeletions(t *testing.T) { }) t.Run("binary", func(t *testing.T) { testAccountIteratorDeletions(t, func(snaps *Tree, root, seek common.Hash) AccountIterator { - return snaps.layers[root].(*diffLayer).newBinaryAccountIterator() + return snaps.layers[root].(*diffLayer).newBinaryAccountIterator(common.Hash{}) }) }) @@ -833,7 +833,7 @@ func TestStorageIteratorDeletions(t *testing.T) { }) t.Run("binary", func(t *testing.T) { testStorageIteratorDeletions(t, func(snaps *Tree, root, account, seek common.Hash) StorageIterator { - return snaps.layers[root].(*diffLayer).newBinaryStorageIterator(account) + return snaps.layers[root].(*diffLayer).newBinaryStorageIterator(account, common.Hash{}) }) }) } @@ -893,7 +893,10 @@ func testStorageIteratorDeletions(t *testing.T, newIterator func(snaps *Tree, ro 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) + verifyIterator(t, 2, snaps.Snapshot( + common.HexToHash("0x06")).(*diffLayer). + newBinaryStorageIterator(common.HexToHash("0xaa"), common.Hash{}), + verifyStorage) } // BenchmarkAccountIteratorTraversal is a bit notorious -- all layers contain the @@ -935,12 +938,12 @@ func BenchmarkAccountIteratorTraversal(b *testing.B) { // We call this once before the benchmark, so the creation of // sorted accountlists are not included in the results. head := snaps.Snapshot(common.HexToHash("0x65")) - head.(*diffLayer).newBinaryAccountIterator() + head.(*diffLayer).newBinaryAccountIterator(common.Hash{}) b.Run("binary iterator keys", func(b *testing.B) { for i := 0; i < b.N; i++ { got := 0 - it := head.(*diffLayer).newBinaryAccountIterator() + it := head.(*diffLayer).newBinaryAccountIterator(common.Hash{}) for it.Next() { got++ } @@ -952,7 +955,7 @@ func BenchmarkAccountIteratorTraversal(b *testing.B) { b.Run("binary iterator values", func(b *testing.B) { for i := 0; i < b.N; i++ { got := 0 - it := head.(*diffLayer).newBinaryAccountIterator() + it := head.(*diffLayer).newBinaryAccountIterator(common.Hash{}) for it.Next() { got++ head.(*diffLayer).accountRLP(it.Hash(), 0) @@ -1032,12 +1035,12 @@ func BenchmarkAccountIteratorLargeBaselayer(b *testing.B) { // We call this once before the benchmark, so the creation of // sorted accountlists are not included in the results. head := snaps.Snapshot(common.HexToHash("0x65")) - head.(*diffLayer).newBinaryAccountIterator() + head.(*diffLayer).newBinaryAccountIterator(common.Hash{}) b.Run("binary iterator (keys)", func(b *testing.B) { for i := 0; i < b.N; i++ { got := 0 - it := head.(*diffLayer).newBinaryAccountIterator() + it := head.(*diffLayer).newBinaryAccountIterator(common.Hash{}) for it.Next() { got++ } @@ -1049,7 +1052,7 @@ func BenchmarkAccountIteratorLargeBaselayer(b *testing.B) { b.Run("binary iterator (values)", func(b *testing.B) { for i := 0; i < b.N; i++ { got := 0 - it := head.(*diffLayer).newBinaryAccountIterator() + it := head.(*diffLayer).newBinaryAccountIterator(common.Hash{}) for it.Next() { got++ v := it.Hash() @@ -1094,7 +1097,7 @@ func BenchmarkAccountIteratorLargeBaselayer(b *testing.B) { /* func BenchmarkBinaryAccountIteration(b *testing.B) { benchmarkAccountIteration(b, func(snap snapshot) AccountIterator { - return snap.(*diffLayer).newBinaryAccountIterator() + return snap.(*diffLayer).newBinaryAccountIterator(common.Hash{}) }) } From 5fcf093668e033675dd1ce06d898b8176efe1215 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Thu, 14 Nov 2024 15:39:06 +0100 Subject: [PATCH 3/5] core/state/snapshot: fix binary iterator --- core/state/snapshot/iterator_binary.go | 56 ++++++++++++++++---------- core/state/snapshot/iterator_test.go | 9 ++--- 2 files changed, 38 insertions(+), 27 deletions(-) diff --git a/core/state/snapshot/iterator_binary.go b/core/state/snapshot/iterator_binary.go index d6f84f2e34bf..6d17d3775933 100644 --- a/core/state/snapshot/iterator_binary.go +++ b/core/state/snapshot/iterator_binary.go @@ -117,33 +117,47 @@ func (dl *diffLayer) initBinaryStorageIterator(account, seek common.Hash) Iterat // or an error if iteration failed for some reason (e.g. root being iterated // becomes stale and garbage collected). func (it *binaryIterator) Next() bool { + for { + ok := it.next() + if !ok { + return ok + } + if len(it.Account()) == 0 && len(it.Slot()) == 0 { + continue + } + return ok + } +} + +func (it *binaryIterator) next() bool { if it.aDone && it.bDone { return false } -first: - if it.aDone { - it.k = it.b.Hash() + for { + if it.aDone { + it.k = it.b.Hash() + it.bDone = !it.b.Next() + return true + } + if it.bDone { + it.k = it.a.Hash() + it.aDone = !it.a.Next() + return true + } + nextA, nextB := it.a.Hash(), it.b.Hash() + if diff := bytes.Compare(nextA[:], nextB[:]); diff < 0 { + it.aDone = !it.a.Next() + it.k = nextA + return true + } else if diff == 0 { + // Now we need to advance one of them + it.aDone = !it.a.Next() + continue + } it.bDone = !it.b.Next() + it.k = nextB return true } - if it.bDone { - it.k = it.a.Hash() - it.aDone = !it.a.Next() - return true - } - nextA, nextB := it.a.Hash(), it.b.Hash() - if diff := bytes.Compare(nextA[:], nextB[:]); diff < 0 { - it.aDone = !it.a.Next() - it.k = nextA - return true - } else if diff == 0 { - // Now we need to advance one of them - it.aDone = !it.a.Next() - goto first - } - it.bDone = !it.b.Next() - it.k = nextB - return true } // Error returns any failure that occurred during iteration, which might have diff --git a/core/state/snapshot/iterator_test.go b/core/state/snapshot/iterator_test.go index 752c78216c35..c6c6d4928d0b 100644 --- a/core/state/snapshot/iterator_test.go +++ b/core/state/snapshot/iterator_test.go @@ -99,9 +99,6 @@ func TestStorageIteratorBasics(t *testing.T) { for account := range accounts { it, _ := diffLayer.StorageIterator(account, common.Hash{}) verifyIterator(t, 100, it, verifyNothing) // Nil is allowed for single layer iterator - - it = diffLayer.newBinaryStorageIterator(account, common.Hash{}) - verifyIterator(t, 100, it, verifyNothing) // Nil is allowed for single layer iterator } diskLayer := diffToDisk(diffLayer) @@ -569,7 +566,7 @@ func TestAccountIteratorFlattening(t *testing.T) { }) t.Run("binary", func(t *testing.T) { testAccountIteratorFlattening(t, func(snaps *Tree, root, seek common.Hash) AccountIterator { - return snaps.layers[root].(*diffLayer).newBinaryAccountIterator(common.Hash{}) + return snaps.layers[root].(*diffLayer).newBinaryAccountIterator(seek) }) }) @@ -771,7 +768,7 @@ func TestAccountIteratorDeletions(t *testing.T) { }) t.Run("binary", func(t *testing.T) { testAccountIteratorDeletions(t, func(snaps *Tree, root, seek common.Hash) AccountIterator { - return snaps.layers[root].(*diffLayer).newBinaryAccountIterator(common.Hash{}) + return snaps.layers[root].(*diffLayer).newBinaryAccountIterator(seek) }) }) @@ -833,7 +830,7 @@ func TestStorageIteratorDeletions(t *testing.T) { }) t.Run("binary", func(t *testing.T) { testStorageIteratorDeletions(t, func(snaps *Tree, root, account, seek common.Hash) StorageIterator { - return snaps.layers[root].(*diffLayer).newBinaryStorageIterator(account, common.Hash{}) + return snaps.layers[root].(*diffLayer).newBinaryStorageIterator(account, seek) }) }) } From 1b0705e2f5c7640a7cfe015fc355a9eb634b37fb Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Thu, 14 Nov 2024 15:42:51 +0100 Subject: [PATCH 4/5] core/state/snapshot: polishes --- core/state/snapshot/iterator_binary.go | 7 +++---- core/state/snapshot/iterator_test.go | 2 -- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/core/state/snapshot/iterator_binary.go b/core/state/snapshot/iterator_binary.go index 6d17d3775933..41d96be4290e 100644 --- a/core/state/snapshot/iterator_binary.go +++ b/core/state/snapshot/iterator_binary.go @@ -118,14 +118,13 @@ func (dl *diffLayer) initBinaryStorageIterator(account, seek common.Hash) Iterat // becomes stale and garbage collected). func (it *binaryIterator) Next() bool { for { - ok := it.next() - if !ok { - return ok + if !it.next() { + return false } if len(it.Account()) == 0 && len(it.Slot()) == 0 { continue } - return ok + return true } } diff --git a/core/state/snapshot/iterator_test.go b/core/state/snapshot/iterator_test.go index c6c6d4928d0b..f2f4d19f833c 100644 --- a/core/state/snapshot/iterator_test.go +++ b/core/state/snapshot/iterator_test.go @@ -567,7 +567,6 @@ func TestAccountIteratorFlattening(t *testing.T) { t.Run("binary", func(t *testing.T) { testAccountIteratorFlattening(t, func(snaps *Tree, root, seek common.Hash) AccountIterator { return snaps.layers[root].(*diffLayer).newBinaryAccountIterator(seek) - }) }) } @@ -769,7 +768,6 @@ func TestAccountIteratorDeletions(t *testing.T) { t.Run("binary", func(t *testing.T) { testAccountIteratorDeletions(t, func(snaps *Tree, root, seek common.Hash) AccountIterator { return snaps.layers[root].(*diffLayer).newBinaryAccountIterator(seek) - }) }) } From cebc3312f34e6f11bfb250fefe3a2fd514593e96 Mon Sep 17 00:00:00 2001 From: Gary Rong Date: Fri, 15 Nov 2024 09:44:31 +0800 Subject: [PATCH 5/5] core/state/snapshot: check it.fail error --- core/state/snapshot/iterator_binary.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/core/state/snapshot/iterator_binary.go b/core/state/snapshot/iterator_binary.go index 41d96be4290e..523c7b9946a9 100644 --- a/core/state/snapshot/iterator_binary.go +++ b/core/state/snapshot/iterator_binary.go @@ -121,10 +121,14 @@ func (it *binaryIterator) Next() bool { if !it.next() { return false } - if len(it.Account()) == 0 && len(it.Slot()) == 0 { - continue + if len(it.Account()) != 0 || len(it.Slot()) != 0 { + return true + } + // it.fail might be set if error occurs by calling + // it.Account() or it.Slot(), stop iteration if so. + if it.fail != nil { + return false } - return true } }