fix: remove stores from renamed/deleted store upgrades

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{

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) {

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)
 		})
 	}
 }