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

chore: Add check for uneven stores' height #14410

Merged
merged 38 commits into from
Jan 31, 2023
Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
49e770c
add store height check in loadVersion(); add test for that
catShaark Dec 23, 2022
a16b35f
minor
catShaark Dec 23, 2022
248a816
Merge branch 'main' into uneven-height
faddat Dec 24, 2022
cfea0f8
Merge branch 'main' into uneven-height
catShaark Dec 25, 2022
c2a1777
Merge branch 'main' into uneven-height
faddat Dec 25, 2022
da1456c
Merge branch 'main' into uneven-height
catShaark Dec 26, 2022
5c2095e
Merge branch 'main' into uneven-height
tac0turtle Dec 27, 2022
6c9ec1e
Merge branch 'main' into uneven-height
faddat Dec 28, 2022
62942cf
Merge branch 'main' into uneven-height
catShaark Dec 28, 2022
8cba224
update
catShaark Dec 30, 2022
57033e9
NewStoreParams
catShaark Dec 30, 2022
59d2d35
use NewStoreParams
catShaark Dec 30, 2022
343e7b5
Merge branch 'main' into uneven-height
tac0turtle Jan 2, 2023
ebc8f08
Merge branch 'main' into uneven-height
catShaark Jan 10, 2023
a821cbd
Update store/rootmulti/store.go
catShaark Jan 11, 2023
7f23d14
Update store/rootmulti/store.go
catShaark Jan 11, 2023
d0fa186
Merge branch 'main' into uneven-height
tac0turtle Jan 16, 2023
cf71697
update loadVersion
catShaark Jan 18, 2023
0352c08
fix tests in store_test.go
catShaark Jan 18, 2023
2929387
Merge branch 'main' into uneven-height
catShaark Jan 18, 2023
a55ba07
changes to commitStores()
catShaark Jan 18, 2023
8308d59
Merge branch 'uneven-height' of https://github.com/notional-labs/cosm…
catShaark Jan 18, 2023
c65287d
Merge branch 'main' into uneven-height
catShaark Jan 21, 2023
54fddbe
revert commitStores changes
catShaark Jan 21, 2023
c11e0da
update TestSetLoader
catShaark Jan 21, 2023
db95ddd
add nolint for NewStoreParams
catShaark Jan 21, 2023
4d12c11
Merge branch 'main' into uneven-height
catShaark Jan 25, 2023
f40a22f
add go doc to TestUnevenStoresHeightCheck
catShaark Jan 25, 2023
7fe88ab
Merge branch 'main' into uneven-height
tac0turtle Jan 25, 2023
ea288e3
Merge branch 'main' into uneven-height
catShaark Jan 25, 2023
527583d
NewStoreParams -> newStoreParams
catShaark Jan 27, 2023
a322fd2
Merge branch 'main' into uneven-height
catShaark Jan 27, 2023
0f9e7a8
update CL
catShaark Jan 27, 2023
712d795
Update store/rootmulti/store.go
catShaark Jan 27, 2023
56fdd42
Merge branch 'main' into uneven-height
catShaark Jan 28, 2023
d948f31
Merge branch 'main' into uneven-height
catShaark Jan 28, 2023
3eaf5c5
Merge branch 'main' into uneven-height
catShaark Jan 30, 2023
22c0d95
Merge branch 'main' into uneven-height
catShaark Jan 31, 2023
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
22 changes: 14 additions & 8 deletions store/rootmulti/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -156,11 +156,7 @@ func (rs *Store) MountStoreWithDB(key types.StoreKey, typ types.StoreType, db db
if _, ok := rs.keysByName[key.Name()]; ok {
panic(fmt.Sprintf("store duplicate store key name %v", key))
}
rs.storesParams[key] = storeParams{
key: key,
typ: typ,
db: db,
}
rs.storesParams[key] = NewStoreParams(key, db, typ, 0)
rs.keysByName[key.Name()] = key
}

Expand Down Expand Up @@ -254,8 +250,10 @@ func (rs *Store) loadVersion(ver int64, upgrades *types.StoreUpgrades) error {
commitID := rs.getCommitID(infos, key.Name())

// If it has been added, set the initial version
if upgrades.IsAdded(key.Name()) {
if upgrades.IsAdded(key.Name()) || upgrades.RenamedFrom(key.Name()) != "" {
storeParams.initialVersion = uint64(ver) + 1
} else if commitID.Version != ver && storeParams.typ == types.StoreTypeIAVL {
return fmt.Errorf("version of store %s mismatch root store's version; expect %d got %d", key.Name(), ver, commitID.Version)
catShaark marked this conversation as resolved.
Show resolved Hide resolved
}

store, err := rs.loadCommitStoreFromParams(key, commitID, storeParams)
Expand All @@ -275,8 +273,7 @@ func (rs *Store) loadVersion(ver int64, upgrades *types.StoreUpgrades) error {
// handle renames specially
// make an unregistered key to satisfy loadCommitStore params
oldKey := types.NewKVStoreKey(oldName)
oldParams := storeParams
oldParams.key = oldKey
oldParams := NewStoreParams(oldKey, storeParams.db, storeParams.typ, 0)
Copy link
Member

@julienrbrt julienrbrt Jan 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it is a stupid question, but why is it 0 and not storeParams.initialVersion-1 for the initial version?
It has been bumped at L254.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great Q @julienrbrt. @catShaark is this because the store already exists and is just being renamed? Let's be sure to have a test that tests this edge case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because we're loading the old store so that we can move data from that old store to the new store. If we want to load an already exist store, we must set initial version to 0 or else we'll have initial version set to %v, but found earlier version %v error

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's already a test for store rename which is TestSetLoader in x/upgrades/types

Copy link
Member

@julienrbrt julienrbrt Jan 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's already a test for store rename which is TestSetLoader in x/upgrades/types

Right, but haven't you removed the case for the store rename? https://github.com/cosmos/cosmos-sdk/pull/14410/files#diff-d7f6a3ea9905545b1e8afd31dcd8f9fccf72124c43349d647c3bae894d4d6114L151.
I may just confused here don't mind my question if it is not relevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So there're 2 checks, the first one checks if we can load the root store with the old store mounted, the second checks if we can load the root store with the new store mounted. I only remove the first one and also we only need the second one to confirm that new store is of correct height.


// load from the old name
oldStore, err := rs.loadCommitStoreFromParams(oldKey, rs.getCommitID(infos, oldName), oldParams)
Expand Down Expand Up @@ -1039,6 +1036,15 @@ type storeParams struct {
initialVersion uint64
}

func NewStoreParams(key types.StoreKey, db dbm.DB, typ types.StoreType, initialVersion uint64) storeParams { // nolint
catShaark marked this conversation as resolved.
Show resolved Hide resolved
return storeParams{
key: key,
db: db,
typ: typ,
initialVersion: initialVersion,
}
}

func GetLatestVersion(db dbm.DB) int64 {
bz, err := db.Get([]byte(latestVersionKey))
if err != nil {
Expand Down
39 changes: 39 additions & 0 deletions store/rootmulti/store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,12 @@ func TestMultistoreLoadWithUpgrade(t *testing.T) {
require.Equal(t, migratedID.Version, int64(2))

reload, _ := newMultiStoreWithModifiedMounts(db, pruningtypes.NewPruningOptions(pruningtypes.PruningNothing))
// unmount store3 since store3 was deleted
unmountStore(reload, "store3")

rs3, _ := reload.GetStoreByName("store3").(types.KVStore)
require.Nil(t, rs3)

err = reload.LoadLatestVersion()
require.Nil(t, err)
require.Equal(t, migratedID, reload.LastCommitID())
Expand Down Expand Up @@ -606,6 +612,32 @@ func TestMultiStore_PruningRestart(t *testing.T) {
}
}

// TestUnevenStoresHeightCheck tests if loading root store correctly errors when
// there's any module store with the wrong height
func TestUnevenStoresHeightCheck(t *testing.T) {
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved
var db dbm.DB = dbm.NewMemDB()
store := newMultiStoreWithMounts(db, pruningtypes.NewPruningOptions(pruningtypes.PruningNothing))
err := store.LoadLatestVersion()
require.Nil(t, err)

// commit to increment store's height
store.Commit()

// mount store4 to root store
store.MountStoreWithDB(types.NewKVStoreKey("store4"), types.StoreTypeIAVL, nil)

// load the stores without upgrades
err = store.LoadLatestVersion()
require.Error(t, err)
catShaark marked this conversation as resolved.
Show resolved Hide resolved

// now, let's load with upgrades...
upgrades := &types.StoreUpgrades{
Added: []string{"store4"},
}
err = store.LoadLatestVersionAndUpgrade(upgrades)
require.Nil(t, err)
}

func TestSetInitialVersion(t *testing.T) {
db := dbm.NewMemDB()
multi := newMultiStoreWithMounts(db, pruningtypes.NewPruningOptions(pruningtypes.PruningNothing))
Expand Down Expand Up @@ -866,6 +898,13 @@ func newMultiStoreWithModifiedMounts(db dbm.DB, pruningOpts pruningtypes.Pruning
return store, upgrades
}

func unmountStore(rootStore *Store, storeKeyName string) {
sk := rootStore.keysByName[storeKeyName]
delete(rootStore.stores, sk)
delete(rootStore.storesParams, sk)
delete(rootStore.keysByName, storeKeyName)
}

func checkStore(t *testing.T, store *Store, expect, got types.CommitID) {
require.Equal(t, expect, got)
require.Equal(t, expect, store.LastCommitID())
Expand Down
5 changes: 0 additions & 5 deletions x/upgrade/types/storeloader_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -146,11 +146,6 @@ func TestSetLoader(t *testing.T) {
res := app.Commit()
require.NotNil(t, res.Data)

// checking the case of the store being renamed
if tc.setLoader != nil {
checkStore(t, db, upgradeHeight, tc.origStoreKey, k, nil)
}

// check db is properly updated
checkStore(t, db, upgradeHeight, tc.loadStoreKey, k, v)
})
Expand Down