From 21c60dd483d95ab0e618e0429179d04215976379 Mon Sep 17 00:00:00 2001 From: Matt Kocubinski Date: Tue, 16 Aug 2022 17:24:30 -0500 Subject: [PATCH 1/6] fix: reproduce nil end iterator bug in tests https://github.com/cosmos/cosmos-sdk/issues/12661 is incomplete, this is only an issue with dirty items in the cache iterator. These (failing) tests cases demonstrate this. --- store/cachekv/store_test.go | 62 +++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/store/cachekv/store_test.go b/store/cachekv/store_test.go index d589932d30fc..4fe94efce25c 100644 --- a/store/cachekv/store_test.go +++ b/store/cachekv/store_test.go @@ -306,6 +306,68 @@ func TestCacheKVMergeIteratorRandom(t *testing.T) { } } +func TestNilEndIterator(t *testing.T) { + tests := []struct { + name string + write bool + end []byte + }{ + {name: "write=false, end=nil", write: false, end: nil}, + {name: "write=true, end=nil", write: true, end: nil}, + {name: "write=false, end=non-nil", write: false, end: keyFmt(3000)}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + st := newCacheKVStore() + + for i := 0; i < 3000; i++ { + kstr := keyFmt(i) + st.Set(kstr, valFmt(i)) + } + + if tt.write { + st.Write() + } + + itr := st.Iterator(keyFmt(1000), tt.end) + i := 1000 + j := 0 + for itr.Valid() { + require.Equal(t, keyFmt(i), itr.Key()) + require.Equal(t, valFmt(i), itr.Value()) + itr.Next() + i++ + j++ + } + + require.Equal(t, 2000, j) + }) + } +} + +func TestNilEndIterator2(t *testing.T) { + st := newCacheKVStore() + st.Set([]byte("AB"), []byte{1}) + + // difference between success and fail is this line: + //st.Write() + + // may also be side-effected by a successful iteration + + iter1 := st.Iterator([]byte("A"), []byte("B")) + require.True(t, iter1.Valid()) + require.NoError(t, iter1.Close()) + + iter2 := st.Iterator([]byte("AA"), nil) + require.True(t, iter2.Valid()) + require.NoError(t, iter2.Close()) + + iter3 := st.Iterator([]byte("A"), nil) + require.True(t, iter3.Valid()) + require.NoError(t, iter3.Close()) +} + //------------------------------------------------------------------------------------------- // do some random ops From 9eaa49c8a1f1af7152be0b918404206d822c2d39 Mon Sep 17 00:00:00 2001 From: Matt Kocubinski Date: Wed, 17 Aug 2022 09:08:10 -0500 Subject: [PATCH 2/6] Handle nil end iterator case in cachekv dirtyItems --- store/cachekv/store.go | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/store/cachekv/store.go b/store/cachekv/store.go index 1e7b74ff93a7..29e297f4cf67 100644 --- a/store/cachekv/store.go +++ b/store/cachekv/store.go @@ -281,7 +281,7 @@ const ( // Constructs a slice of dirty items, to use w/ memIterator. func (store *Store) dirtyItems(start, end []byte) { startStr, endStr := conv.UnsafeBytesToStr(start), conv.UnsafeBytesToStr(end) - if startStr > endStr { + if end != nil && startStr > endStr { // Nothing to do here. return } @@ -296,6 +296,7 @@ func (store *Store) dirtyItems(start, end []byte) { // than just not having the cache. if n < 1024 { for key := range store.unsortedCache { + // dbm.IsKeyInDomain is nil safe and returns true iff key is greater than start if dbm.IsKeyInDomain(conv.UnsafeStrToBytes(key), start, end) { cacheValue := store.cache[key] unsorted = append(unsorted, &kv.Pair{Key: []byte(key), Value: cacheValue.value}) @@ -316,13 +317,18 @@ func (store *Store) dirtyItems(start, end []byte) { // Now find the values within the domain // [start, end) startIndex := findStartIndex(strL, startStr) - endIndex := findEndIndex(strL, endStr) + if startIndex < 0 { + startIndex = 0 + } - if endIndex < 0 { + var endIndex int + if end == nil { endIndex = len(strL) - 1 + } else { + endIndex = findEndIndex(strL, endStr) } - if startIndex < 0 { - startIndex = 0 + if endIndex < 0 { + endIndex = len(strL) - 1 } kvL := make([]*kv.Pair, 0) From 68f3f39bbb1025312f1c9dd5502cf75a7685e9e5 Mon Sep 17 00:00:00 2001 From: Matt Kocubinski Date: Wed, 17 Aug 2022 09:13:16 -0500 Subject: [PATCH 3/6] remove redundant test --- store/cachekv/store_test.go | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/store/cachekv/store_test.go b/store/cachekv/store_test.go index 4fe94efce25c..e51b96994e92 100644 --- a/store/cachekv/store_test.go +++ b/store/cachekv/store_test.go @@ -346,28 +346,6 @@ func TestNilEndIterator(t *testing.T) { } } -func TestNilEndIterator2(t *testing.T) { - st := newCacheKVStore() - st.Set([]byte("AB"), []byte{1}) - - // difference between success and fail is this line: - //st.Write() - - // may also be side-effected by a successful iteration - - iter1 := st.Iterator([]byte("A"), []byte("B")) - require.True(t, iter1.Valid()) - require.NoError(t, iter1.Close()) - - iter2 := st.Iterator([]byte("AA"), nil) - require.True(t, iter2.Valid()) - require.NoError(t, iter2.Close()) - - iter3 := st.Iterator([]byte("A"), nil) - require.True(t, iter3.Valid()) - require.NoError(t, iter3.Close()) -} - //------------------------------------------------------------------------------------------- // do some random ops From e73f5ed3448c6e8c1151dab7d2bf3481b99635b9 Mon Sep 17 00:00:00 2001 From: Matt Kocubinski Date: Wed, 17 Aug 2022 09:20:13 -0500 Subject: [PATCH 4/6] add test case for full key scan in dirtyItems --- store/cachekv/store_test.go | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/store/cachekv/store_test.go b/store/cachekv/store_test.go index e51b96994e92..aeb4194b34a9 100644 --- a/store/cachekv/store_test.go +++ b/store/cachekv/store_test.go @@ -307,21 +307,25 @@ func TestCacheKVMergeIteratorRandom(t *testing.T) { } func TestNilEndIterator(t *testing.T) { + const SIZE = 3000 + tests := []struct { - name string - write bool - end []byte + name string + write bool + startIndex int + end []byte }{ - {name: "write=false, end=nil", write: false, end: nil}, - {name: "write=true, end=nil", write: true, end: nil}, - {name: "write=false, end=non-nil", write: false, end: keyFmt(3000)}, + {name: "write=false, end=nil", write: false, end: nil, startIndex: 1000}, + {name: "write=false, end=nil; full key scan", write: false, end: nil, startIndex: 2000}, + {name: "write=true, end=nil", write: true, end: nil, startIndex: 1000}, + {name: "write=false, end=non-nil", write: false, end: keyFmt(3000), startIndex: 1000}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { st := newCacheKVStore() - for i := 0; i < 3000; i++ { + for i := 0; i < SIZE; i++ { kstr := keyFmt(i) st.Set(kstr, valFmt(i)) } @@ -330,8 +334,8 @@ func TestNilEndIterator(t *testing.T) { st.Write() } - itr := st.Iterator(keyFmt(1000), tt.end) - i := 1000 + itr := st.Iterator(keyFmt(tt.startIndex), tt.end) + i := tt.startIndex j := 0 for itr.Valid() { require.Equal(t, keyFmt(i), itr.Key()) @@ -341,7 +345,7 @@ func TestNilEndIterator(t *testing.T) { j++ } - require.Equal(t, 2000, j) + require.Equal(t, SIZE-tt.startIndex, j) }) } } From ef66e91c9d331adf8565b885eb460baff6c82762 Mon Sep 17 00:00:00 2001 From: Matt Kocubinski Date: Wed, 17 Aug 2022 10:57:08 -0500 Subject: [PATCH 5/6] Add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b18fa3384b9a..0516c401f075 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,6 +61,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * [#12187](https://github.com/cosmos/cosmos-sdk/pull/12187) Add batch operation for x/nft module. * [#12693](https://github.com/cosmos/cosmos-sdk/pull/12693) Make sure the order of each node is consistent when emitting proto events. * [#12455](https://github.com/cosmos/cosmos-sdk/pull/12455) Show attempts count in error for signing. +* [#12945](https://github.com/cosmos/cosmos-sdk/pull/12945) Handle nil end semantics in store/cachekv/iterator ### State Machine Breaking From e8838577b1797627c7c2643da4bef5bc2fae57a4 Mon Sep 17 00:00:00 2001 From: Matt Kocubinski Date: Wed, 17 Aug 2022 11:01:08 -0500 Subject: [PATCH 6/6] Fix changelog entry --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0516c401f075..43d16411c856 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,7 +61,6 @@ Ref: https://keepachangelog.com/en/1.0.0/ * [#12187](https://github.com/cosmos/cosmos-sdk/pull/12187) Add batch operation for x/nft module. * [#12693](https://github.com/cosmos/cosmos-sdk/pull/12693) Make sure the order of each node is consistent when emitting proto events. * [#12455](https://github.com/cosmos/cosmos-sdk/pull/12455) Show attempts count in error for signing. -* [#12945](https://github.com/cosmos/cosmos-sdk/pull/12945) Handle nil end semantics in store/cachekv/iterator ### State Machine Breaking @@ -123,6 +122,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (x/group) [#12888](https://github.com/cosmos/cosmos-sdk/pull/12888) Fix event propagation to the current context of `x/group` message execution `[]sdk.Result`. * (sdk/dec_coins) [#12903](https://github.com/cosmos/cosmos-sdk/pull/12903) Fix nil `DecCoin` creation when converting `Coins` to `DecCoins` * (x/upgrade) [#12906](https://github.com/cosmos/cosmos-sdk/pull/12906) Fix upgrade failure by moving downgrade verification logic after store migration. +* (store) [#12945](https://github.com/cosmos/cosmos-sdk/pull/12945) Fix nil end semantics in store/cachekv/iterator when iterating a dirty cache. ### Deprecated