From 068c8574295e7cd13244bce952f5b54447b37a3b Mon Sep 17 00:00:00 2001 From: HuangYi Date: Thu, 6 Oct 2022 14:47:59 +0800 Subject: [PATCH 1/4] fix: state listener could observe uncommitted writes Closes: #13457 don't pass listeners to nested cached store, only the most inner layer's cache writes should be observed. --- CHANGELOG.md | 1 + store/cachemulti/store.go | 6 +++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c6446bc9cd7b..6ad003fe51c6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -166,6 +166,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (x/gov) [#13051](https://github.com/cosmos/cosmos-sdk/pull/13051) In SubmitPropsal, when a legacy msg fails it's handler call, wrap the error as ErrInvalidProposalContent (instead of ErrNoProposalHandlerExists). * (x/gov) [#13045](https://github.com/cosmos/cosmos-sdk/pull/13045) Fix gov migrations for v3(0.46). * (snapshot) [#13400](https://github.com/cosmos/cosmos-sdk/pull/13400) Fix snapshot checksum issue in golang 1.19. +* (store) [#]() Don't let state listener observe the uncommitted writes. ### Deprecated diff --git a/store/cachemulti/store.go b/store/cachemulti/store.go index 2e39554e17ce..deb1d46272dd 100644 --- a/store/cachemulti/store.go +++ b/store/cachemulti/store.go @@ -45,6 +45,9 @@ func NewFromKVStore( keys map[string]types.StoreKey, traceWriter io.Writer, traceContext types.TraceContext, listeners map[types.StoreKey][]types.WriteListener, ) Store { + if listeners == nil { + listeners = make(map[types.StoreKey][]types.WriteListener) + } cms := Store{ db: cachekv.NewStore(store), stores: make(map[types.StoreKey]types.CacheWrap, len(stores)), @@ -86,7 +89,8 @@ func newCacheMultiStoreFromCMS(cms Store) Store { stores[k] = v } - return NewFromKVStore(cms.db, stores, nil, cms.traceWriter, cms.traceContext, cms.listeners) + // don't pass listeners to nested cache store. + return NewFromKVStore(cms.db, stores, nil, cms.traceWriter, cms.traceContext, nil) } // SetTracer sets the tracer for the MultiStore that the underlying From abd2cbf4e300db864ec515813c3bcf137ac221c3 Mon Sep 17 00:00:00 2001 From: yihuang Date: Thu, 6 Oct 2022 14:53:44 +0800 Subject: [PATCH 2/4] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6ad003fe51c6..85e354245bbf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -166,7 +166,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (x/gov) [#13051](https://github.com/cosmos/cosmos-sdk/pull/13051) In SubmitPropsal, when a legacy msg fails it's handler call, wrap the error as ErrInvalidProposalContent (instead of ErrNoProposalHandlerExists). * (x/gov) [#13045](https://github.com/cosmos/cosmos-sdk/pull/13045) Fix gov migrations for v3(0.46). * (snapshot) [#13400](https://github.com/cosmos/cosmos-sdk/pull/13400) Fix snapshot checksum issue in golang 1.19. -* (store) [#]() Don't let state listener observe the uncommitted writes. +* (store) [#13459](https://github.com/cosmos/cosmos-sdk/pull/13459) Don't let state listener observe the uncommitted writes. ### Deprecated From 4cb3c01eda13dcd3c9cd5b5c6230a47b60c10158 Mon Sep 17 00:00:00 2001 From: HuangYi Date: Thu, 6 Oct 2022 15:44:49 +0800 Subject: [PATCH 3/4] add unit test --- store/rootmulti/store_test.go | 49 +++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/store/rootmulti/store_test.go b/store/rootmulti/store_test.go index af829105e7f1..e4806dbb9ce0 100644 --- a/store/rootmulti/store_test.go +++ b/store/rootmulti/store_test.go @@ -912,3 +912,52 @@ func hashStores(stores map[types.StoreKey]types.CommitKVStore) []byte { } return sdkmaps.HashFromMap(m) } + +type TestListener struct { + stateCache []types.StoreKVPair +} + +func (tl *TestListener) OnWrite(storeKey types.StoreKey, key []byte, value []byte, delete bool) error { + tl.stateCache = append(tl.stateCache, types.StoreKVPair{ + StoreKey: storeKey.Name(), + Key: key, + Value: value, + Delete: delete, + }) + return nil +} + +func TestStateListeners(t *testing.T) { + var db dbm.DB = dbm.NewMemDB() + ms := newMultiStoreWithMounts(db, pruningtypes.NewPruningOptions(pruningtypes.PruningNothing)) + + listener := &TestListener{} + ms.AddListeners(testStoreKey1, []types.WriteListener{listener}) + + require.NoError(t, ms.LoadLatestVersion()) + cacheMulti := ms.CacheMultiStore() + + store1 := cacheMulti.GetKVStore(testStoreKey1) + store1.Set([]byte{1}, []byte{1}) + require.Empty(t, listener.stateCache) + + // writes are observed when cache store commit. + cacheMulti.Write() + require.Equal(t, 1, len(listener.stateCache)) + + // test nested cache store + listener.stateCache = []types.StoreKVPair{} + nested := cacheMulti.CacheMultiStore() + + store1 = nested.GetKVStore(testStoreKey1) + store1.Set([]byte{1}, []byte{1}) + require.Empty(t, listener.stateCache) + + // writes are not observed when nested cache store commit + nested.Write() + require.Empty(t, listener.stateCache) + + // writes are observed when inner cache store commit + cacheMulti.Write() + require.Equal(t, 1, len(listener.stateCache)) +} From 6903cd319c3b96fd229663c4fe03459b42adcaef Mon Sep 17 00:00:00 2001 From: HuangYi Date: Thu, 6 Oct 2022 15:46:29 +0800 Subject: [PATCH 4/4] rename --- store/rootmulti/store_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/store/rootmulti/store_test.go b/store/rootmulti/store_test.go index e4806dbb9ce0..a219593fbb86 100644 --- a/store/rootmulti/store_test.go +++ b/store/rootmulti/store_test.go @@ -913,11 +913,11 @@ func hashStores(stores map[types.StoreKey]types.CommitKVStore) []byte { return sdkmaps.HashFromMap(m) } -type TestListener struct { +type MockListener struct { stateCache []types.StoreKVPair } -func (tl *TestListener) OnWrite(storeKey types.StoreKey, key []byte, value []byte, delete bool) error { +func (tl *MockListener) OnWrite(storeKey types.StoreKey, key []byte, value []byte, delete bool) error { tl.stateCache = append(tl.stateCache, types.StoreKVPair{ StoreKey: storeKey.Name(), Key: key, @@ -931,7 +931,7 @@ func TestStateListeners(t *testing.T) { var db dbm.DB = dbm.NewMemDB() ms := newMultiStoreWithMounts(db, pruningtypes.NewPruningOptions(pruningtypes.PruningNothing)) - listener := &TestListener{} + listener := &MockListener{} ms.AddListeners(testStoreKey1, []types.WriteListener{listener}) require.NoError(t, ms.LoadLatestVersion())