From eb08ce15a0b388fd90107c3e9981a655ed125a5a Mon Sep 17 00:00:00 2001
From: HuangYi <huang@crypto.com>
Date: Wed, 21 Dec 2022 10:34:30 +0800
Subject: [PATCH 1/8] refactor: make cachekv store thread-safe again

Closes: #14376

Solution:
- copy isolated sortedCache when starting iterating
- cleanup deletion logic
---
 store/cachekv/internal/btree.go       |  38 ++++++----
 store/cachekv/internal/btree_test.go  | 102 ++++++++++----------------
 store/cachekv/internal/memiterator.go |  24 +-----
 store/cachekv/store.go                |  63 +++++-----------
 4 files changed, 84 insertions(+), 143 deletions(-)

diff --git a/store/cachekv/internal/btree.go b/store/cachekv/internal/btree.go
index c09b33fab217..d963c937fe29 100644
--- a/store/cachekv/internal/btree.go
+++ b/store/cachekv/internal/btree.go
@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"errors"
 
+	"github.com/cosmos/cosmos-sdk/store/types"
 	"github.com/tidwall/btree"
 )
 
@@ -21,23 +22,22 @@ var errKeyEmpty = errors.New("key cannot be empty")
 //
 // We choose tidwall/btree over google/btree here because it provides API to implement step iterator directly.
 type BTree struct {
-	tree btree.BTreeG[item]
+	tree *btree.BTreeG[item]
 }
 
 // NewBTree creates a wrapper around `btree.BTreeG`.
-func NewBTree() *BTree {
-	return &BTree{tree: *btree.NewBTreeGOptions(byKeys, btree.Options{
-		Degree: bTreeDegree,
-		// Contract: cachekv store must not be called concurrently
-		NoLocks: true,
+func NewBTree() BTree {
+	return BTree{tree: btree.NewBTreeGOptions(byKeys, btree.Options{
+		Degree:  bTreeDegree,
+		NoLocks: false,
 	})}
 }
 
-func (bt *BTree) Set(key, value []byte) {
+func (bt BTree) Set(key, value []byte) {
 	bt.tree.Set(newItem(key, value))
 }
 
-func (bt *BTree) Get(key []byte) []byte {
+func (bt BTree) Get(key []byte) []byte {
 	i, found := bt.tree.Get(newItem(key, nil))
 	if !found {
 		return nil
@@ -45,22 +45,30 @@ func (bt *BTree) Get(key []byte) []byte {
 	return i.value
 }
 
-func (bt *BTree) Delete(key []byte) {
+func (bt BTree) Delete(key []byte) {
 	bt.tree.Delete(newItem(key, nil))
 }
 
-func (bt *BTree) Iterator(start, end []byte) (*memIterator, error) { //nolint:revive
+func (bt BTree) Iterator(start, end []byte) types.Iterator { //nolint:revive
 	if (start != nil && len(start) == 0) || (end != nil && len(end) == 0) {
-		return nil, errKeyEmpty
+		panic(errKeyEmpty)
 	}
-	return NewMemIterator(start, end, bt, make(map[string]struct{}), true), nil
+	return NewMemIterator(start, end, bt, true)
 }
 
-func (bt *BTree) ReverseIterator(start, end []byte) (*memIterator, error) { //nolint:revive
+func (bt BTree) ReverseIterator(start, end []byte) types.Iterator { //nolint:revive
 	if (start != nil && len(start) == 0) || (end != nil && len(end) == 0) {
-		return nil, errKeyEmpty
+		panic(errKeyEmpty)
+	}
+	return NewMemIterator(start, end, bt, false)
+}
+
+// Copy the tree. This is a copy-on-write operation and is very fast because
+// it only performs a shadowed copy.
+func (bt BTree) Copy() BTree {
+	return BTree{
+		tree: bt.tree.Copy(),
 	}
-	return NewMemIterator(start, end, bt, make(map[string]struct{}), false), nil
 }
 
 // item is a btree item with byte slices as keys and values
diff --git a/store/cachekv/internal/btree_test.go b/store/cachekv/internal/btree_test.go
index f85a8bbaf109..4783abf9f4db 100644
--- a/store/cachekv/internal/btree_test.go
+++ b/store/cachekv/internal/btree_test.go
@@ -3,6 +3,7 @@ package internal
 import (
 	"testing"
 
+	"github.com/cosmos/cosmos-sdk/store/types"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/stretchr/testify/require"
 )
@@ -48,140 +49,113 @@ func TestDBIterator(t *testing.T) {
 	}
 
 	// Blank iterator keys should error
-	_, err := db.ReverseIterator([]byte{}, nil)
-	require.Equal(t, errKeyEmpty, err)
-	_, err = db.ReverseIterator(nil, []byte{})
-	require.Equal(t, errKeyEmpty, err)
-
-	itr, err := db.Iterator(nil, nil)
-	require.NoError(t, err)
+	require.PanicsWithError(t, errKeyEmpty.Error(), func() {
+		db.ReverseIterator([]byte{}, nil)
+	})
+	require.PanicsWithError(t, errKeyEmpty.Error(), func() {
+		db.ReverseIterator(nil, []byte{})
+	})
+
+	itr := db.Iterator(nil, nil)
 	verifyIterator(t, itr, []int64{0, 1, 2, 3, 4, 5, 7, 8, 9}, "forward iterator")
 
-	ritr, err := db.ReverseIterator(nil, nil)
-	require.NoError(t, err)
+	ritr := db.ReverseIterator(nil, nil)
 	verifyIterator(t, ritr, []int64{9, 8, 7, 5, 4, 3, 2, 1, 0}, "reverse iterator")
 
-	itr, err = db.Iterator(nil, int642Bytes(0))
-	require.NoError(t, err)
+	itr = db.Iterator(nil, int642Bytes(0))
 	verifyIterator(t, itr, []int64(nil), "forward iterator to 0")
 
-	ritr, err = db.ReverseIterator(int642Bytes(10), nil)
-	require.NoError(t, err)
+	ritr = db.ReverseIterator(int642Bytes(10), nil)
 	verifyIterator(t, ritr, []int64(nil), "reverse iterator from 10 (ex)")
 
-	itr, err = db.Iterator(int642Bytes(0), nil)
-	require.NoError(t, err)
+	itr = db.Iterator(int642Bytes(0), nil)
 	verifyIterator(t, itr, []int64{0, 1, 2, 3, 4, 5, 7, 8, 9}, "forward iterator from 0")
 
-	itr, err = db.Iterator(int642Bytes(1), nil)
-	require.NoError(t, err)
+	itr = db.Iterator(int642Bytes(1), nil)
 	verifyIterator(t, itr, []int64{1, 2, 3, 4, 5, 7, 8, 9}, "forward iterator from 1")
 
-	ritr, err = db.ReverseIterator(nil, int642Bytes(10))
-	require.NoError(t, err)
+	ritr = db.ReverseIterator(nil, int642Bytes(10))
 	verifyIterator(t, ritr,
 		[]int64{9, 8, 7, 5, 4, 3, 2, 1, 0}, "reverse iterator from 10 (ex)")
 
-	ritr, err = db.ReverseIterator(nil, int642Bytes(9))
-	require.NoError(t, err)
+	ritr = db.ReverseIterator(nil, int642Bytes(9))
 	verifyIterator(t, ritr,
 		[]int64{8, 7, 5, 4, 3, 2, 1, 0}, "reverse iterator from 9 (ex)")
 
-	ritr, err = db.ReverseIterator(nil, int642Bytes(8))
-	require.NoError(t, err)
+	ritr = db.ReverseIterator(nil, int642Bytes(8))
 	verifyIterator(t, ritr,
 		[]int64{7, 5, 4, 3, 2, 1, 0}, "reverse iterator from 8 (ex)")
 
-	itr, err = db.Iterator(int642Bytes(5), int642Bytes(6))
-	require.NoError(t, err)
+	itr = db.Iterator(int642Bytes(5), int642Bytes(6))
 	verifyIterator(t, itr, []int64{5}, "forward iterator from 5 to 6")
 
-	itr, err = db.Iterator(int642Bytes(5), int642Bytes(7))
-	require.NoError(t, err)
+	itr = db.Iterator(int642Bytes(5), int642Bytes(7))
 	verifyIterator(t, itr, []int64{5}, "forward iterator from 5 to 7")
 
-	itr, err = db.Iterator(int642Bytes(5), int642Bytes(8))
-	require.NoError(t, err)
+	itr = db.Iterator(int642Bytes(5), int642Bytes(8))
 	verifyIterator(t, itr, []int64{5, 7}, "forward iterator from 5 to 8")
 
-	itr, err = db.Iterator(int642Bytes(6), int642Bytes(7))
-	require.NoError(t, err)
+	itr = db.Iterator(int642Bytes(6), int642Bytes(7))
 	verifyIterator(t, itr, []int64(nil), "forward iterator from 6 to 7")
 
-	itr, err = db.Iterator(int642Bytes(6), int642Bytes(8))
-	require.NoError(t, err)
+	itr = db.Iterator(int642Bytes(6), int642Bytes(8))
 	verifyIterator(t, itr, []int64{7}, "forward iterator from 6 to 8")
 
-	itr, err = db.Iterator(int642Bytes(7), int642Bytes(8))
-	require.NoError(t, err)
+	itr = db.Iterator(int642Bytes(7), int642Bytes(8))
 	verifyIterator(t, itr, []int64{7}, "forward iterator from 7 to 8")
 
-	ritr, err = db.ReverseIterator(int642Bytes(4), int642Bytes(5))
-	require.NoError(t, err)
+	ritr = db.ReverseIterator(int642Bytes(4), int642Bytes(5))
 	verifyIterator(t, ritr, []int64{4}, "reverse iterator from 5 (ex) to 4")
 
-	ritr, err = db.ReverseIterator(int642Bytes(4), int642Bytes(6))
-	require.NoError(t, err)
+	ritr = db.ReverseIterator(int642Bytes(4), int642Bytes(6))
 	verifyIterator(t, ritr,
 		[]int64{5, 4}, "reverse iterator from 6 (ex) to 4")
 
-	ritr, err = db.ReverseIterator(int642Bytes(4), int642Bytes(7))
-	require.NoError(t, err)
+	ritr = db.ReverseIterator(int642Bytes(4), int642Bytes(7))
 	verifyIterator(t, ritr,
 		[]int64{5, 4}, "reverse iterator from 7 (ex) to 4")
 
-	ritr, err = db.ReverseIterator(int642Bytes(5), int642Bytes(6))
-	require.NoError(t, err)
+	ritr = db.ReverseIterator(int642Bytes(5), int642Bytes(6))
 	verifyIterator(t, ritr, []int64{5}, "reverse iterator from 6 (ex) to 5")
 
-	ritr, err = db.ReverseIterator(int642Bytes(5), int642Bytes(7))
-	require.NoError(t, err)
+	ritr = db.ReverseIterator(int642Bytes(5), int642Bytes(7))
 	verifyIterator(t, ritr, []int64{5}, "reverse iterator from 7 (ex) to 5")
 
-	ritr, err = db.ReverseIterator(int642Bytes(6), int642Bytes(7))
-	require.NoError(t, err)
+	ritr = db.ReverseIterator(int642Bytes(6), int642Bytes(7))
 	verifyIterator(t, ritr,
 		[]int64(nil), "reverse iterator from 7 (ex) to 6")
 
-	ritr, err = db.ReverseIterator(int642Bytes(10), nil)
-	require.NoError(t, err)
+	ritr = db.ReverseIterator(int642Bytes(10), nil)
 	verifyIterator(t, ritr, []int64(nil), "reverse iterator to 10")
 
-	ritr, err = db.ReverseIterator(int642Bytes(6), nil)
-	require.NoError(t, err)
+	ritr = db.ReverseIterator(int642Bytes(6), nil)
 	verifyIterator(t, ritr, []int64{9, 8, 7}, "reverse iterator to 6")
 
-	ritr, err = db.ReverseIterator(int642Bytes(5), nil)
-	require.NoError(t, err)
+	ritr = db.ReverseIterator(int642Bytes(5), nil)
 	verifyIterator(t, ritr, []int64{9, 8, 7, 5}, "reverse iterator to 5")
 
-	ritr, err = db.ReverseIterator(int642Bytes(8), int642Bytes(9))
-	require.NoError(t, err)
+	ritr = db.ReverseIterator(int642Bytes(8), int642Bytes(9))
 	verifyIterator(t, ritr, []int64{8}, "reverse iterator from 9 (ex) to 8")
 
-	ritr, err = db.ReverseIterator(int642Bytes(2), int642Bytes(4))
-	require.NoError(t, err)
+	ritr = db.ReverseIterator(int642Bytes(2), int642Bytes(4))
 	verifyIterator(t, ritr,
 		[]int64{3, 2}, "reverse iterator from 4 (ex) to 2")
 
-	ritr, err = db.ReverseIterator(int642Bytes(4), int642Bytes(2))
-	require.NoError(t, err)
+	ritr = db.ReverseIterator(int642Bytes(4), int642Bytes(2))
 	verifyIterator(t, ritr,
 		[]int64(nil), "reverse iterator from 2 (ex) to 4")
 
 	// Ensure that the iterators don't panic with an empty database.
 	db2 := NewBTree()
 
-	itr, err = db2.Iterator(nil, nil)
-	require.NoError(t, err)
+	itr = db2.Iterator(nil, nil)
 	verifyIterator(t, itr, nil, "forward iterator with empty db")
 
-	ritr, err = db2.ReverseIterator(nil, nil)
-	require.NoError(t, err)
+	ritr = db2.ReverseIterator(nil, nil)
 	verifyIterator(t, ritr, nil, "reverse iterator with empty db")
 }
 
-func verifyIterator(t *testing.T, itr *memIterator, expected []int64, msg string) {
+func verifyIterator(t *testing.T, itr types.Iterator, expected []int64, msg string) {
 	i := 0
 	for itr.Valid() {
 		key := itr.Key()
diff --git a/store/cachekv/internal/memiterator.go b/store/cachekv/internal/memiterator.go
index 34c3796c0677..1a34a29b54b5 100644
--- a/store/cachekv/internal/memiterator.go
+++ b/store/cachekv/internal/memiterator.go
@@ -11,7 +11,7 @@ import (
 var _ types.Iterator = (*memIterator)(nil)
 
 // memIterator iterates over iterKVCache items.
-// if key is nil, means it was deleted.
+// if value is nil, means it was deleted.
 // Implements Iterator.
 type memIterator struct {
 	iter btree.IterG[item]
@@ -19,12 +19,10 @@ type memIterator struct {
 	start     []byte
 	end       []byte
 	ascending bool
-	lastKey   []byte
-	deleted   map[string]struct{}
 	valid     bool
 }
 
-func NewMemIterator(start, end []byte, items *BTree, deleted map[string]struct{}, ascending bool) *memIterator { //nolint:revive
+func NewMemIterator(start, end []byte, items BTree, ascending bool) *memIterator { //nolint:revive
 	iter := items.tree.Iter()
 	var valid bool
 	if ascending {
@@ -52,8 +50,6 @@ func NewMemIterator(start, end []byte, items *BTree, deleted map[string]struct{}
 		start:     start,
 		end:       end,
 		ascending: ascending,
-		lastKey:   nil,
-		deleted:   deleted,
 		valid:     valid,
 	}
 
@@ -113,21 +109,7 @@ func (mi *memIterator) Key() []byte {
 }
 
 func (mi *memIterator) Value() []byte {
-	item := mi.iter.Item()
-	key := item.key
-	// We need to handle the case where deleted is modified and includes our current key
-	// We handle this by maintaining a lastKey object in the iterator.
-	// If the current key is the same as the last key (and last key is not nil / the start)
-	// then we are calling value on the same thing as last time.
-	// Therefore we don't check the mi.deleted to see if this key is included in there.
-	if _, ok := mi.deleted[string(key)]; ok {
-		if mi.lastKey == nil || !bytes.Equal(key, mi.lastKey) {
-			// not re-calling on old last key
-			return nil
-		}
-	}
-	mi.lastKey = key
-	return item.value
+	return mi.iter.Item().value
 }
 
 func (mi *memIterator) assertValid() {
diff --git a/store/cachekv/store.go b/store/cachekv/store.go
index e9f9d7195f4c..2d7339f87739 100644
--- a/store/cachekv/store.go
+++ b/store/cachekv/store.go
@@ -24,14 +24,11 @@ type cValue struct {
 }
 
 // Store wraps an in-memory cache around an underlying types.KVStore.
-// If a cached value is nil but deleted is defined for the corresponding key,
-// it means the parent doesn't have the key. (No need to delete upon Write())
 type Store struct {
 	mtx           sync.Mutex
 	cache         map[string]*cValue
-	deleted       map[string]struct{}
 	unsortedCache map[string]struct{}
-	sortedCache   *internal.BTree // always ascending sorted
+	sortedCache   internal.BTree // always ascending sorted
 	parent        types.KVStore
 }
 
@@ -41,7 +38,6 @@ var _ types.CacheKVStore = (*Store)(nil)
 func NewStore(parent types.KVStore) *Store {
 	return &Store{
 		cache:         make(map[string]*cValue),
-		deleted:       make(map[string]struct{}),
 		unsortedCache: make(map[string]struct{}),
 		sortedCache:   internal.NewBTree(),
 		parent:        parent,
@@ -63,7 +59,7 @@ func (store *Store) Get(key []byte) (value []byte) {
 	cacheValue, ok := store.cache[conv.UnsafeBytesToStr(key)]
 	if !ok {
 		value = store.parent.Get(key)
-		store.setCacheValue(key, value, false, false)
+		store.setCacheValue(key, value, false)
 	} else {
 		value = cacheValue.value
 	}
@@ -79,7 +75,7 @@ func (store *Store) Set(key []byte, value []byte) {
 	types.AssertValidKey(key)
 	types.AssertValidValue(value)
 
-	store.setCacheValue(key, value, false, true)
+	store.setCacheValue(key, value, true)
 }
 
 // Has implements types.KVStore.
@@ -94,7 +90,7 @@ func (store *Store) Delete(key []byte) {
 	defer store.mtx.Unlock()
 
 	types.AssertValidKey(key)
-	store.setCacheValue(key, nil, true, true)
+	store.setCacheValue(key, nil, true)
 }
 
 // Implements Cachetypes.KVStore.
@@ -102,7 +98,7 @@ func (store *Store) Write() {
 	store.mtx.Lock()
 	defer store.mtx.Unlock()
 
-	if len(store.cache) == 0 && len(store.deleted) == 0 && len(store.unsortedCache) == 0 {
+	if len(store.cache) == 0 && len(store.unsortedCache) == 0 {
 		store.sortedCache = internal.NewBTree()
 		return
 	}
@@ -122,19 +118,16 @@ func (store *Store) Write() {
 	// TODO: Consider allowing usage of Batch, which would allow the write to
 	// at least happen atomically.
 	for _, key := range keys {
-		if store.isDeleted(key) {
-			// We use []byte(key) instead of conv.UnsafeStrToBytes because we cannot
-			// be sure if the underlying store might do a save with the byteslice or
-			// not. Once we get confirmation that .Delete is guaranteed not to
-			// save the byteslice, then we can assume only a read-only copy is sufficient.
-			store.parent.Delete([]byte(key))
-			continue
-		}
-
+		// We use []byte(key) instead of conv.UnsafeStrToBytes because we cannot
+		// be sure if the underlying store might do a save with the byteslice or
+		// not. Once we get confirmation that .Delete is guaranteed not to
+		// save the byteslice, then we can assume only a read-only copy is sufficient.
 		cacheValue := store.cache[key]
 		if cacheValue.value != nil {
-			// It already exists in the parent, hence delete it.
+			// It already exists in the parent, hence update it.
 			store.parent.Set([]byte(key), cacheValue.value)
+		} else {
+			store.parent.Delete([]byte(key))
 		}
 	}
 
@@ -144,9 +137,6 @@ func (store *Store) Write() {
 	for key := range store.cache {
 		delete(store.cache, key)
 	}
-	for key := range store.deleted {
-		delete(store.deleted, key)
-	}
 	for key := range store.unsortedCache {
 		delete(store.unsortedCache, key)
 	}
@@ -180,17 +170,19 @@ func (store *Store) iterator(start, end []byte, ascending bool) types.Iterator {
 	store.mtx.Lock()
 	defer store.mtx.Unlock()
 
+	store.dirtyItems(start, end)
+	isoSortedCache := store.sortedCache.Copy()
+
 	var parent, cache types.Iterator
 
 	if ascending {
 		parent = store.parent.Iterator(start, end)
+		cache = isoSortedCache.Iterator(start, end)
 	} else {
 		parent = store.parent.ReverseIterator(start, end)
+		cache = isoSortedCache.ReverseIterator(start, end)
 	}
 
-	store.dirtyItems(start, end)
-	cache = internal.NewMemIterator(start, end, store.sortedCache, store.deleted, ascending)
-
 	return internal.NewCacheMergeIterator(parent, cache, ascending)
 }
 
@@ -370,13 +362,7 @@ func (store *Store) clearUnsortedCacheSubset(unsorted []*kv.Pair, sortState sort
 	}
 
 	for _, item := range unsorted {
-		if item.Value == nil {
-			// deleted element, tracked by store.deleted
-			// setting arbitrary value
-			store.sortedCache.Set(item.Key, []byte{})
-			continue
-		}
-
+		// sortedCache is able to store `nil` value to represent deleted items.
 		store.sortedCache.Set(item.Key, item.Value)
 	}
 }
@@ -385,23 +371,14 @@ func (store *Store) clearUnsortedCacheSubset(unsorted []*kv.Pair, sortState sort
 // etc
 
 // Only entrypoint to mutate store.cache.
-func (store *Store) setCacheValue(key, value []byte, deleted bool, dirty bool) {
+// A `nil` value means a deletion.
+func (store *Store) setCacheValue(key, value []byte, dirty bool) {
 	keyStr := conv.UnsafeBytesToStr(key)
 	store.cache[keyStr] = &cValue{
 		value: value,
 		dirty: dirty,
 	}
-	if deleted {
-		store.deleted[keyStr] = struct{}{}
-	} else {
-		delete(store.deleted, keyStr)
-	}
 	if dirty {
 		store.unsortedCache[keyStr] = struct{}{}
 	}
 }
-
-func (store *Store) isDeleted(key string) bool {
-	_, ok := store.deleted[key]
-	return ok
-}

From 1757999b2af0886d21a67b62c8ccb826e8357b25 Mon Sep 17 00:00:00 2001
From: HuangYi <huang@crypto.com>
Date: Wed, 21 Dec 2022 19:29:09 +0800
Subject: [PATCH 2/8] channgelog

---
 CHANGELOG.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 0d8c6e31f426..5bda90d48854 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -128,6 +128,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (x/auth/vesting) [#13502](https://github.com/cosmos/cosmos-sdk/pull/13502) Add Amino Msg registration for `MsgCreatePeriodicVestingAccount`.
 * (x/auth)[#13780](https://github.com/cosmos/cosmos-sdk/pull/13780) `id` (type of int64) in `AccountAddressByID` grpc query is now deprecated, update to account-id(type of uint64) to use `AccountAddressByID`.
 * (x/group) [#13876](https://github.com/cosmos/cosmos-sdk/pull/13876) Fix group MinExecutionPeriod that is checked on execution now, instead of voting period end.
+* (store) [#](https://github.com/cosmos/cosmos-sdk/pull/14378) make cachekv store thread-safe again, cleanup iteration and deletion logic, iteration is on strictly isolated view now, which is breaking from previous behavior.
 
 ### API Breaking Changes
 

From b8e66ffd81ba7468c93a2a846fe00c4b16d0f12a Mon Sep 17 00:00:00 2001
From: HuangYi <huang@crypto.com>
Date: Wed, 21 Dec 2022 22:14:14 +0800
Subject: [PATCH 3/8] fix lint

---
 store/cachekv/internal/btree.go | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/store/cachekv/internal/btree.go b/store/cachekv/internal/btree.go
index d963c937fe29..9e48ba7c35cd 100644
--- a/store/cachekv/internal/btree.go
+++ b/store/cachekv/internal/btree.go
@@ -49,14 +49,14 @@ func (bt BTree) Delete(key []byte) {
 	bt.tree.Delete(newItem(key, nil))
 }
 
-func (bt BTree) Iterator(start, end []byte) types.Iterator { //nolint:revive
+func (bt BTree) Iterator(start, end []byte) types.Iterator {
 	if (start != nil && len(start) == 0) || (end != nil && len(end) == 0) {
 		panic(errKeyEmpty)
 	}
 	return NewMemIterator(start, end, bt, true)
 }
 
-func (bt BTree) ReverseIterator(start, end []byte) types.Iterator { //nolint:revive
+func (bt BTree) ReverseIterator(start, end []byte) types.Iterator {
 	if (start != nil && len(start) == 0) || (end != nil && len(end) == 0) {
 		panic(errKeyEmpty)
 	}

From 3d5a52fa88f6e3a878a833633083a6aad0edbe8e Mon Sep 17 00:00:00 2001
From: yihuang <huang@crypto.com>
Date: Thu, 22 Dec 2022 01:46:13 +0800
Subject: [PATCH 4/8] Update CHANGELOG.md

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
---
 CHANGELOG.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5bda90d48854..1b30e097b786 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -128,7 +128,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (x/auth/vesting) [#13502](https://github.com/cosmos/cosmos-sdk/pull/13502) Add Amino Msg registration for `MsgCreatePeriodicVestingAccount`.
 * (x/auth)[#13780](https://github.com/cosmos/cosmos-sdk/pull/13780) `id` (type of int64) in `AccountAddressByID` grpc query is now deprecated, update to account-id(type of uint64) to use `AccountAddressByID`.
 * (x/group) [#13876](https://github.com/cosmos/cosmos-sdk/pull/13876) Fix group MinExecutionPeriod that is checked on execution now, instead of voting period end.
-* (store) [#](https://github.com/cosmos/cosmos-sdk/pull/14378) make cachekv store thread-safe again, cleanup iteration and deletion logic, iteration is on strictly isolated view now, which is breaking from previous behavior.
+* (store) [#14378](https://github.com/cosmos/cosmos-sdk/pull/14378) The `CacheKV` store is thread-safe again, which includes improved iteration and deletion logic. Iteration is on a strictly isolated view now, which is breaking from previous behavior.
 
 ### API Breaking Changes
 

From e36ff1282f9f92fd1bc23cbe9a174d5974862e56 Mon Sep 17 00:00:00 2001
From: yihuang <huang@crypto.com>
Date: Thu, 22 Dec 2022 01:49:47 +0800
Subject: [PATCH 5/8] Update store/cachekv/internal/btree.go

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
---
 store/cachekv/internal/btree.go | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/store/cachekv/internal/btree.go b/store/cachekv/internal/btree.go
index 9e48ba7c35cd..c20524ea3b24 100644
--- a/store/cachekv/internal/btree.go
+++ b/store/cachekv/internal/btree.go
@@ -27,10 +27,12 @@ type BTree struct {
 
 // NewBTree creates a wrapper around `btree.BTreeG`.
 func NewBTree() BTree {
-	return BTree{tree: btree.NewBTreeGOptions(byKeys, btree.Options{
-		Degree:  bTreeDegree,
-		NoLocks: false,
-	})}
+	return BTree{
+		tree: btree.NewBTreeGOptions(byKeys, btree.Options{
+			Degree:  bTreeDegree,
+			NoLocks: false,
+		}),
+	}
 }
 
 func (bt BTree) Set(key, value []byte) {

From a321063b43becb7f6e7e142d7d82f21a358c2fe9 Mon Sep 17 00:00:00 2001
From: HuangYi <huang@crypto.com>
Date: Thu, 22 Dec 2022 01:59:09 +0800
Subject: [PATCH 6/8] better interface

---
 store/cachekv/internal/btree.go      | 12 ++--
 store/cachekv/internal/btree_test.go | 99 ++++++++++++++++++----------
 store/cachekv/store.go               | 12 +++-
 3 files changed, 78 insertions(+), 45 deletions(-)

diff --git a/store/cachekv/internal/btree.go b/store/cachekv/internal/btree.go
index c20524ea3b24..98bc54d656f3 100644
--- a/store/cachekv/internal/btree.go
+++ b/store/cachekv/internal/btree.go
@@ -51,18 +51,18 @@ func (bt BTree) Delete(key []byte) {
 	bt.tree.Delete(newItem(key, nil))
 }
 
-func (bt BTree) Iterator(start, end []byte) types.Iterator {
+func (bt BTree) Iterator(start, end []byte) (types.Iterator, error) {
 	if (start != nil && len(start) == 0) || (end != nil && len(end) == 0) {
-		panic(errKeyEmpty)
+		return nil, errKeyEmpty
 	}
-	return NewMemIterator(start, end, bt, true)
+	return NewMemIterator(start, end, bt, true), nil
 }
 
-func (bt BTree) ReverseIterator(start, end []byte) types.Iterator {
+func (bt BTree) ReverseIterator(start, end []byte) (types.Iterator, error) {
 	if (start != nil && len(start) == 0) || (end != nil && len(end) == 0) {
-		panic(errKeyEmpty)
+		return nil, errKeyEmpty
 	}
-	return NewMemIterator(start, end, bt, false)
+	return NewMemIterator(start, end, bt, false), nil
 }
 
 // Copy the tree. This is a copy-on-write operation and is very fast because
diff --git a/store/cachekv/internal/btree_test.go b/store/cachekv/internal/btree_test.go
index 4783abf9f4db..b6aa22db8e0f 100644
--- a/store/cachekv/internal/btree_test.go
+++ b/store/cachekv/internal/btree_test.go
@@ -49,109 +49,136 @@ func TestDBIterator(t *testing.T) {
 	}
 
 	// Blank iterator keys should error
-	require.PanicsWithError(t, errKeyEmpty.Error(), func() {
-		db.ReverseIterator([]byte{}, nil)
-	})
-	require.PanicsWithError(t, errKeyEmpty.Error(), func() {
-		db.ReverseIterator(nil, []byte{})
-	})
-
-	itr := db.Iterator(nil, nil)
+	_, err := db.ReverseIterator([]byte{}, nil)
+	require.Equal(t, errKeyEmpty, err)
+	_, err = db.ReverseIterator(nil, []byte{})
+	require.Equal(t, errKeyEmpty, err)
+
+	itr, err := db.Iterator(nil, nil)
+	require.NoError(t, err)
 	verifyIterator(t, itr, []int64{0, 1, 2, 3, 4, 5, 7, 8, 9}, "forward iterator")
 
-	ritr := db.ReverseIterator(nil, nil)
+	ritr, err := db.ReverseIterator(nil, nil)
+	require.NoError(t, err)
 	verifyIterator(t, ritr, []int64{9, 8, 7, 5, 4, 3, 2, 1, 0}, "reverse iterator")
 
-	itr = db.Iterator(nil, int642Bytes(0))
+	itr, err = db.Iterator(nil, int642Bytes(0))
+	require.NoError(t, err)
 	verifyIterator(t, itr, []int64(nil), "forward iterator to 0")
 
-	ritr = db.ReverseIterator(int642Bytes(10), nil)
+	ritr, err = db.ReverseIterator(int642Bytes(10), nil)
+	require.NoError(t, err)
 	verifyIterator(t, ritr, []int64(nil), "reverse iterator from 10 (ex)")
 
-	itr = db.Iterator(int642Bytes(0), nil)
+	itr, err = db.Iterator(int642Bytes(0), nil)
+	require.NoError(t, err)
 	verifyIterator(t, itr, []int64{0, 1, 2, 3, 4, 5, 7, 8, 9}, "forward iterator from 0")
 
-	itr = db.Iterator(int642Bytes(1), nil)
+	itr, err = db.Iterator(int642Bytes(1), nil)
+	require.NoError(t, err)
 	verifyIterator(t, itr, []int64{1, 2, 3, 4, 5, 7, 8, 9}, "forward iterator from 1")
 
-	ritr = db.ReverseIterator(nil, int642Bytes(10))
+	ritr, err = db.ReverseIterator(nil, int642Bytes(10))
+	require.NoError(t, err)
 	verifyIterator(t, ritr,
 		[]int64{9, 8, 7, 5, 4, 3, 2, 1, 0}, "reverse iterator from 10 (ex)")
 
-	ritr = db.ReverseIterator(nil, int642Bytes(9))
+	ritr, err = db.ReverseIterator(nil, int642Bytes(9))
+	require.NoError(t, err)
 	verifyIterator(t, ritr,
 		[]int64{8, 7, 5, 4, 3, 2, 1, 0}, "reverse iterator from 9 (ex)")
 
-	ritr = db.ReverseIterator(nil, int642Bytes(8))
+	ritr, err = db.ReverseIterator(nil, int642Bytes(8))
+	require.NoError(t, err)
 	verifyIterator(t, ritr,
 		[]int64{7, 5, 4, 3, 2, 1, 0}, "reverse iterator from 8 (ex)")
 
-	itr = db.Iterator(int642Bytes(5), int642Bytes(6))
+	itr, err = db.Iterator(int642Bytes(5), int642Bytes(6))
+	require.NoError(t, err)
 	verifyIterator(t, itr, []int64{5}, "forward iterator from 5 to 6")
 
-	itr = db.Iterator(int642Bytes(5), int642Bytes(7))
+	itr, err = db.Iterator(int642Bytes(5), int642Bytes(7))
+	require.NoError(t, err)
 	verifyIterator(t, itr, []int64{5}, "forward iterator from 5 to 7")
 
-	itr = db.Iterator(int642Bytes(5), int642Bytes(8))
+	itr, err = db.Iterator(int642Bytes(5), int642Bytes(8))
+	require.NoError(t, err)
 	verifyIterator(t, itr, []int64{5, 7}, "forward iterator from 5 to 8")
 
-	itr = db.Iterator(int642Bytes(6), int642Bytes(7))
+	itr, err = db.Iterator(int642Bytes(6), int642Bytes(7))
+	require.NoError(t, err)
 	verifyIterator(t, itr, []int64(nil), "forward iterator from 6 to 7")
 
-	itr = db.Iterator(int642Bytes(6), int642Bytes(8))
+	itr, err = db.Iterator(int642Bytes(6), int642Bytes(8))
+	require.NoError(t, err)
 	verifyIterator(t, itr, []int64{7}, "forward iterator from 6 to 8")
 
-	itr = db.Iterator(int642Bytes(7), int642Bytes(8))
+	itr, err = db.Iterator(int642Bytes(7), int642Bytes(8))
+	require.NoError(t, err)
 	verifyIterator(t, itr, []int64{7}, "forward iterator from 7 to 8")
 
-	ritr = db.ReverseIterator(int642Bytes(4), int642Bytes(5))
+	ritr, err = db.ReverseIterator(int642Bytes(4), int642Bytes(5))
+	require.NoError(t, err)
 	verifyIterator(t, ritr, []int64{4}, "reverse iterator from 5 (ex) to 4")
 
-	ritr = db.ReverseIterator(int642Bytes(4), int642Bytes(6))
+	ritr, err = db.ReverseIterator(int642Bytes(4), int642Bytes(6))
+	require.NoError(t, err)
 	verifyIterator(t, ritr,
 		[]int64{5, 4}, "reverse iterator from 6 (ex) to 4")
 
-	ritr = db.ReverseIterator(int642Bytes(4), int642Bytes(7))
+	ritr, err = db.ReverseIterator(int642Bytes(4), int642Bytes(7))
+	require.NoError(t, err)
 	verifyIterator(t, ritr,
 		[]int64{5, 4}, "reverse iterator from 7 (ex) to 4")
 
-	ritr = db.ReverseIterator(int642Bytes(5), int642Bytes(6))
+	ritr, err = db.ReverseIterator(int642Bytes(5), int642Bytes(6))
+	require.NoError(t, err)
 	verifyIterator(t, ritr, []int64{5}, "reverse iterator from 6 (ex) to 5")
 
-	ritr = db.ReverseIterator(int642Bytes(5), int642Bytes(7))
+	ritr, err = db.ReverseIterator(int642Bytes(5), int642Bytes(7))
+	require.NoError(t, err)
 	verifyIterator(t, ritr, []int64{5}, "reverse iterator from 7 (ex) to 5")
 
-	ritr = db.ReverseIterator(int642Bytes(6), int642Bytes(7))
+	ritr, err = db.ReverseIterator(int642Bytes(6), int642Bytes(7))
+	require.NoError(t, err)
 	verifyIterator(t, ritr,
 		[]int64(nil), "reverse iterator from 7 (ex) to 6")
 
-	ritr = db.ReverseIterator(int642Bytes(10), nil)
+	ritr, err = db.ReverseIterator(int642Bytes(10), nil)
+	require.NoError(t, err)
 	verifyIterator(t, ritr, []int64(nil), "reverse iterator to 10")
 
-	ritr = db.ReverseIterator(int642Bytes(6), nil)
+	ritr, err = db.ReverseIterator(int642Bytes(6), nil)
+	require.NoError(t, err)
 	verifyIterator(t, ritr, []int64{9, 8, 7}, "reverse iterator to 6")
 
-	ritr = db.ReverseIterator(int642Bytes(5), nil)
+	ritr, err = db.ReverseIterator(int642Bytes(5), nil)
+	require.NoError(t, err)
 	verifyIterator(t, ritr, []int64{9, 8, 7, 5}, "reverse iterator to 5")
 
-	ritr = db.ReverseIterator(int642Bytes(8), int642Bytes(9))
+	ritr, err = db.ReverseIterator(int642Bytes(8), int642Bytes(9))
+	require.NoError(t, err)
 	verifyIterator(t, ritr, []int64{8}, "reverse iterator from 9 (ex) to 8")
 
-	ritr = db.ReverseIterator(int642Bytes(2), int642Bytes(4))
+	ritr, err = db.ReverseIterator(int642Bytes(2), int642Bytes(4))
+	require.NoError(t, err)
 	verifyIterator(t, ritr,
 		[]int64{3, 2}, "reverse iterator from 4 (ex) to 2")
 
-	ritr = db.ReverseIterator(int642Bytes(4), int642Bytes(2))
+	ritr, err = db.ReverseIterator(int642Bytes(4), int642Bytes(2))
+	require.NoError(t, err)
 	verifyIterator(t, ritr,
 		[]int64(nil), "reverse iterator from 2 (ex) to 4")
 
 	// Ensure that the iterators don't panic with an empty database.
 	db2 := NewBTree()
 
-	itr = db2.Iterator(nil, nil)
+	itr, err = db2.Iterator(nil, nil)
+	require.NoError(t, err)
 	verifyIterator(t, itr, nil, "forward iterator with empty db")
 
-	ritr = db2.ReverseIterator(nil, nil)
+	ritr, err = db2.ReverseIterator(nil, nil)
+	require.NoError(t, err)
 	verifyIterator(t, ritr, nil, "reverse iterator with empty db")
 }
 
diff --git a/store/cachekv/store.go b/store/cachekv/store.go
index 2d7339f87739..666a71257354 100644
--- a/store/cachekv/store.go
+++ b/store/cachekv/store.go
@@ -173,14 +173,20 @@ func (store *Store) iterator(start, end []byte, ascending bool) types.Iterator {
 	store.dirtyItems(start, end)
 	isoSortedCache := store.sortedCache.Copy()
 
-	var parent, cache types.Iterator
+	var (
+		err           error
+		parent, cache types.Iterator
+	)
 
 	if ascending {
 		parent = store.parent.Iterator(start, end)
-		cache = isoSortedCache.Iterator(start, end)
+		cache, err = isoSortedCache.Iterator(start, end)
 	} else {
 		parent = store.parent.ReverseIterator(start, end)
-		cache = isoSortedCache.ReverseIterator(start, end)
+		cache, err = isoSortedCache.ReverseIterator(start, end)
+	}
+	if err != nil {
+		panic(err)
 	}
 
 	return internal.NewCacheMergeIterator(parent, cache, ascending)

From 354b208a4061b3cc1c66154f1d7b55c804ecc3e1 Mon Sep 17 00:00:00 2001
From: HuangYi <huang@crypto.com>
Date: Thu, 22 Dec 2022 02:03:10 +0800
Subject: [PATCH 7/8] make newMemIterator private again

---
 store/cachekv/internal/btree.go       | 4 ++--
 store/cachekv/internal/memiterator.go | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/store/cachekv/internal/btree.go b/store/cachekv/internal/btree.go
index 98bc54d656f3..d0340022f2c5 100644
--- a/store/cachekv/internal/btree.go
+++ b/store/cachekv/internal/btree.go
@@ -55,14 +55,14 @@ func (bt BTree) Iterator(start, end []byte) (types.Iterator, error) {
 	if (start != nil && len(start) == 0) || (end != nil && len(end) == 0) {
 		return nil, errKeyEmpty
 	}
-	return NewMemIterator(start, end, bt, true), nil
+	return newMemIterator(start, end, bt, true), nil
 }
 
 func (bt BTree) ReverseIterator(start, end []byte) (types.Iterator, error) {
 	if (start != nil && len(start) == 0) || (end != nil && len(end) == 0) {
 		return nil, errKeyEmpty
 	}
-	return NewMemIterator(start, end, bt, false), nil
+	return newMemIterator(start, end, bt, false), nil
 }
 
 // Copy the tree. This is a copy-on-write operation and is very fast because
diff --git a/store/cachekv/internal/memiterator.go b/store/cachekv/internal/memiterator.go
index 1a34a29b54b5..71d5185d68b2 100644
--- a/store/cachekv/internal/memiterator.go
+++ b/store/cachekv/internal/memiterator.go
@@ -22,7 +22,7 @@ type memIterator struct {
 	valid     bool
 }
 
-func NewMemIterator(start, end []byte, items BTree, ascending bool) *memIterator { //nolint:revive
+func newMemIterator(start, end []byte, items BTree, ascending bool) *memIterator { //nolint:revive
 	iter := items.tree.Iter()
 	var valid bool
 	if ascending {

From 90f831d24380f6a2e1a51b30f3bd4cc15a699926 Mon Sep 17 00:00:00 2001
From: HuangYi <huang@crypto.com>
Date: Thu, 22 Dec 2022 02:42:19 +0800
Subject: [PATCH 8/8] fix lint

---
 store/cachekv/internal/memiterator.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/store/cachekv/internal/memiterator.go b/store/cachekv/internal/memiterator.go
index 71d5185d68b2..b2fa93d3f71c 100644
--- a/store/cachekv/internal/memiterator.go
+++ b/store/cachekv/internal/memiterator.go
@@ -22,7 +22,7 @@ type memIterator struct {
 	valid     bool
 }
 
-func newMemIterator(start, end []byte, items BTree, ascending bool) *memIterator { //nolint:revive
+func newMemIterator(start, end []byte, items BTree, ascending bool) *memIterator {
 	iter := items.tree.Iter()
 	var valid bool
 	if ascending {