From d4d25f5e18e77e3040df49f0f37006fb50ffd501 Mon Sep 17 00:00:00 2001 From: Tyler <48813565+technicallyty@users.noreply.github.com> Date: Mon, 12 Jul 2021 09:54:07 -0700 Subject: [PATCH] fix: remove stores from renamed/deleted store upgrades (#9409) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description - stores that were renamed are now properly deleted - deleted/renamed and renamed stores are no longer added to `CommitInfo` - deleted/renamed stores are now properly removed from rootmulti store's memory ref: #7991 closes: N/A --- Before we can merge this PR, please make sure that all the following items have been checked off. If any of the checklist items are not applicable, please leave them but write a little note why. - [x] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [x] Linked to Github issue with discussion and accepted design OR link to spec that describes this work. - [x] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md). - [ ] Wrote unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] Updated relevant documentation (`docs/`) or specification (`x//spec/`) - [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code). - [ ] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md` - [x] Re-reviewed `Files changed` in the Github PR explorer - [x] Review `Codecov Report` in the comment section below once CI passes --- store/rootmulti/store.go | 35 +++++++++++++++++++++++------ store/rootmulti/store_test.go | 4 ++-- x/upgrade/types/storeloader_test.go | 7 +++++- 3 files changed, 36 insertions(+), 10 deletions(-) diff --git a/store/rootmulti/store.go b/store/rootmulti/store.go index da095d6807fa..daf3c22ace34 100644 --- a/store/rootmulti/store.go +++ b/store/rootmulti/store.go @@ -54,6 +54,7 @@ type Store struct { lazyLoading bool pruneHeights []int64 initialVersion int64 + removalMap map[types.StoreKey]bool traceWriter io.Writer traceContext types.TraceContext @@ -81,6 +82,7 @@ func NewStore(db dbm.DB) *Store { keysByName: make(map[string]types.StoreKey), pruneHeights: make([]int64, 0), listeners: make(map[types.StoreKey][]types.WriteListener), + removalMap: make(map[types.StoreKey]bool), } } @@ -210,9 +212,10 @@ func (rs *Store) loadVersion(ver int64, upgrades *types.StoreUpgrades) error { if err := deleteKVStore(store.(types.KVStore)); err != nil { return errors.Wrapf(err, "failed to delete store %s", key.Name()) } + rs.removalMap[key] = true } else if oldName := upgrades.RenamedFrom(key.Name()); oldName != "" { // handle renames specially - // make an unregistered key to satify loadCommitStore params + // make an unregistered key to satisfy loadCommitStore params oldKey := types.NewKVStoreKey(oldName) oldParams := storeParams oldParams.key = oldKey @@ -227,6 +230,11 @@ func (rs *Store) loadVersion(ver int64, upgrades *types.StoreUpgrades) error { if err := moveKVStoreData(oldStore.(types.KVStore), store.(types.KVStore)); err != nil { return errors.Wrapf(err, "failed to move store %s -> %s", oldName, key.Name()) } + + // add the old key so its deletion is committed + newStores[oldKey] = oldStore + // this will ensure it's not perpetually stored in commitInfo + rs.removalMap[oldKey] = true } } @@ -361,8 +369,19 @@ func (rs *Store) Commit() types.CommitID { previousHeight = rs.lastCommitInfo.GetVersion() version = previousHeight + 1 } + rs.lastCommitInfo = commitStores(version, rs.stores, rs.removalMap) - rs.lastCommitInfo = commitStores(version, rs.stores) + // remove remnants of removed stores + for sk := range rs.removalMap { + if _, ok := rs.stores[sk]; ok { + delete(rs.stores, sk) + delete(rs.storesParams, sk) + delete(rs.keysByName, sk.Name()) + } + } + + // reset the removalMap + rs.removalMap = make(map[types.StoreKey]bool) // Determine if pruneHeight height needs to be added to the list of heights to // be pruned, where pruneHeight = (commitHeight - 1) - KeepRecent. @@ -944,7 +963,7 @@ func getLatestVersion(db dbm.DB) int64 { } // Commits each store and returns a new commitInfo. -func commitStores(version int64, storeMap map[types.StoreKey]types.CommitKVStore) *types.CommitInfo { +func commitStores(version int64, storeMap map[types.StoreKey]types.CommitKVStore, removalMap map[types.StoreKey]bool) *types.CommitInfo { storeInfos := make([]types.StoreInfo, 0, len(storeMap)) for key, store := range storeMap { @@ -954,10 +973,12 @@ func commitStores(version int64, storeMap map[types.StoreKey]types.CommitKVStore continue } - si := types.StoreInfo{} - si.Name = key.Name() - si.CommitId = commitID - storeInfos = append(storeInfos, si) + if !removalMap[key] { + si := types.StoreInfo{} + si.Name = key.Name() + si.CommitId = commitID + storeInfos = append(storeInfos, si) + } } return &types.CommitInfo{ diff --git a/store/rootmulti/store_test.go b/store/rootmulti/store_test.go index cf7653b076cb..05817d6a8411 100644 --- a/store/rootmulti/store_test.go +++ b/store/rootmulti/store_test.go @@ -300,8 +300,8 @@ func TestMultistoreLoadWithUpgrade(t *testing.T) { ci, err = getCommitInfo(db, 2) require.NoError(t, err) require.Equal(t, int64(2), ci.Version) - require.Equal(t, 4, len(ci.StoreInfos), ci.StoreInfos) - checkContains(t, ci.StoreInfos, []string{"store1", "restore2", "store3", "store4"}) + require.Equal(t, 3, len(ci.StoreInfos), ci.StoreInfos) + checkContains(t, ci.StoreInfos, []string{"store1", "restore2", "store4"}) } func TestParsePath(t *testing.T) { diff --git a/x/upgrade/types/storeloader_test.go b/x/upgrade/types/storeloader_test.go index ec2bfa824d07..680e4d356881 100644 --- a/x/upgrade/types/storeloader_test.go +++ b/x/upgrade/types/storeloader_test.go @@ -90,6 +90,7 @@ func TestSetLoader(t *testing.T) { loadStoreKey string }{ "don't set loader": { + setLoader: nil, origStoreKey: "foo", loadStoreKey: "foo", }, @@ -145,9 +146,13 @@ 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) - checkStore(t, db, upgradeHeight, tc.loadStoreKey, []byte("foo"), nil) }) } }