Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Problem: Restore don't work with snapshot revert usage #245

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions store/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* [#241](https://github.com/crypto-org-chain/cosmos-sdk/pull/241) Refactor the cache store to be btree backed, prepare to support copy-on-write atomic branching.
* [#242](https://github.com/crypto-org-chain/cosmos-sdk/pull/242) Init cache on cache lazily, save memory allocations.
* [#243](https://github.com/crypto-org-chain/cosmos-sdk/pull/243) Support `RunAtomic` API to use new CoW cache store.
* [#244](https://github.com/crypto-org-chain/cosmos-sdk/pull/244) Add `Discard` method to CacheWrap to discard the write buffer.

## v1.1.0 (March 20, 2024)

Expand Down
4 changes: 4 additions & 0 deletions store/cachekv/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,10 @@ func (store *GStore[V]) Write() {
store.writeSet.Clear()
}

func (store *GStore[V]) Discard() {
store.writeSet.Clear()
}

// CacheWrap implements CacheWrapper.
func (store *GStore[V]) CacheWrap() types.CacheWrap {
return NewGStore(store, store.isZero, store.valueLen)
Expand Down
47 changes: 33 additions & 14 deletions store/cachemulti/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
}

for key, store := range stores {
cms.stores[key] = cms.initStore(key, store)
cms.initStore(key, store)
}

return cms
Expand Down Expand Up @@ -80,7 +80,9 @@
store = tracekv.NewStore(kvstore, cms.traceWriter, tctx)
}
}
return store.CacheWrap()
cache := store.CacheWrap()
cms.stores[key] = cache
return cache
}

// SetTracer sets the tracer for the MultiStore that the underlying
Expand Down Expand Up @@ -126,6 +128,12 @@
}
}

func (cms Store) Discard() {
for _, store := range cms.stores {
store.Discard()
}
Comment on lines +132 to +134

Check warning

Code scanning / CodeQL

Iteration over map Warning

Iteration over map may be a possible source of non-determinism
}

// Implements CacheWrapper.
func (cms Store) CacheWrap() types.CacheWrap {
return cms.CacheMultiStore().(types.CacheWrap)
Expand All @@ -138,14 +146,9 @@

func (cms Store) getCacheWrap(key types.StoreKey) types.CacheWrap {
store, ok := cms.stores[key]
if !ok {
if !ok && cms.parentStore != nil {
// load on demand
if cms.branched {
store = cms.parentStore(key).(types.BranchStore).Clone().(types.CacheWrap)
} else if cms.parentStore != nil {
store = cms.initStore(key, cms.parentStore(key))
}
cms.stores[key] = store
store = cms.initStore(key, cms.parentStore(key))
}
if key == nil || store == nil {
panic(fmt.Sprintf("kv store with key %v has not been registered in stores", key))
Expand Down Expand Up @@ -181,12 +184,15 @@
}

func (cms Store) Clone() Store {
stores := make(map[types.StoreKey]types.CacheWrap, len(cms.stores))
for k, v := range cms.stores {
stores[k] = v.(types.BranchStore).Clone().(types.CacheWrap)
}
yihuang marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +188 to +190

Check warning

Code scanning / CodeQL

Iteration over map Warning

Iteration over map may be a possible source of non-determinism
return Store{
stores: make(map[types.StoreKey]types.CacheWrap),

stores: stores,
traceWriter: cms.traceWriter,
traceContext: cms.traceContext,
parentStore: cms.getCacheWrap,
parentStore: cms.parentStore,

branched: true,
}
Expand All @@ -197,9 +203,22 @@
panic("cannot restore from non-branched store")
}

// restore the stores
// discard the non-exists stores
for k, v := range cms.stores {
if _, ok := other.stores[k]; !ok {
// clear the cache store if it's not in the other
v.Discard()
}
}
Comment on lines +207 to +212

Check warning

Code scanning / CodeQL

Iteration over map Warning

Iteration over map may be a possible source of non-determinism

// restore the other stores
for k, v := range other.stores {
cms.stores[k].(types.BranchStore).Restore(v.(types.BranchStore))
store, ok := cms.stores[k]
if !ok {
store = cms.initStore(k, cms.parentStore(k))
}

store.(types.BranchStore).Restore(v.(types.BranchStore))
}
}

Expand Down
39 changes: 32 additions & 7 deletions store/cachemulti/store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,22 +35,22 @@ func TestRunAtomic(t *testing.T) {
func(v any) int { return 1 },
)
keys := map[string]types.StoreKey{
"abc": types.NewKVStoreKey("abc"),
"obj": types.NewObjectStoreKey("obj"),
"lazy": types.NewKVStoreKey("lazy"),
"abc": types.NewKVStoreKey("abc"),
"obj": types.NewObjectStoreKey("obj"),
}
s := Store{stores: map[types.StoreKey]types.CacheWrap{
keys["abc"]: store.CacheWrap(),
keys["obj"]: objStore.CacheWrap(),
keys["lazy"]: nil,
parent := Store{stores: map[types.StoreKey]types.CacheWrap{
keys["abc"]: store.CacheWrap(),
keys["obj"]: objStore.CacheWrap(),
}}

s := Store{stores: map[types.StoreKey]types.CacheWrap{}, parentStore: parent.getCacheWrap}
s.RunAtomic(func(ms types.CacheMultiStore) error {
ms.GetKVStore(keys["abc"]).Set([]byte("key"), []byte("value"))
ms.GetObjKVStore(keys["obj"]).Set([]byte("key"), "value")
return nil
})
require.Equal(t, []byte("value"), s.GetKVStore(keys["abc"]).Get([]byte("key")))
require.Equal(t, []byte(nil), s.GetKVStore(keys["abc"]).Get([]byte("key-non-exist")))
require.Equal(t, "value", s.GetObjKVStore(keys["obj"]).Get([]byte("key")).(string))

require.Error(t, s.RunAtomic(func(ms types.CacheMultiStore) error {
Expand All @@ -61,3 +61,28 @@ func TestRunAtomic(t *testing.T) {
require.Equal(t, []byte("value"), s.GetKVStore(keys["abc"]).Get([]byte("key")))
require.Equal(t, "value", s.GetObjKVStore(keys["obj"]).Get([]byte("key")).(string))
}

func TestBranchStore(t *testing.T) {
store := dbadapter.Store{DB: dbm.NewMemDB()}
objStore := internal.NewBTreeStore(btree.NewBTree[any](),
func(v any) bool { return v == nil },
func(v any) int { return 1 },
)
keys := map[string]types.StoreKey{
"abc": types.NewKVStoreKey("abc"),
"obj": types.NewObjectStoreKey("obj"),
}
parent := Store{stores: map[types.StoreKey]types.CacheWrap{
keys["abc"]: store.CacheWrap(),
keys["obj"]: objStore.CacheWrap(),
}}

s := Store{stores: map[types.StoreKey]types.CacheWrap{}, parentStore: parent.getCacheWrap}
s.GetKVStore(keys["abc"]).Set([]byte("key"), []byte("value"))
snapshot := s.Clone()
s.GetKVStore(keys["abc"]).Set([]byte("key"), []byte("value2"))
s.GetObjKVStore(keys["obj"]).Set([]byte("key"), "value")
s.Restore(snapshot)
require.Equal(t, []byte("value"), s.GetKVStore(keys["abc"]).Get([]byte("key")))
require.Equal(t, nil, s.GetObjKVStore(keys["obj"]).Get([]byte("key")))
}
3 changes: 3 additions & 0 deletions store/types/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,9 @@ type CacheWrap interface {

// Write syncs with the underlying store.
Write()

// Discard the write set
Discard()
}

type CacheWrapper interface {
Expand Down
Loading