From f757c90f61eb7d2646fafd4cc3df7d4f556037b5 Mon Sep 17 00:00:00 2001 From: Tomas Tauber <2410580+tomtau@users.noreply.github.com> Date: Tue, 28 Sep 2021 23:20:43 +0800 Subject: [PATCH] fix: removed potential sources of non-determinism in upgrades (#10189) forced deterministic iteration order in upgrade migrations, x/upgrade and store during upgrades Co-authored-by: Robert Zaremba --- CHANGELOG.md | 3 ++- store/rootmulti/store.go | 17 ++++++++++++++++- types/module/module.go | 14 +++++++++++++- x/upgrade/keeper/keeper.go | 14 +++++++++++++- 4 files changed, 44 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ccefe533f662..4b05cc98fbe4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -139,7 +139,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (server) [#10016](https://github.com/cosmos/cosmos-sdk/issues/10016) Fix marshaling of index-events into server config file. * (x/feegrant) [\#10049](https://github.com/cosmos/cosmos-sdk/issues/10049) Fixed the error message when `period` or `period-limit` flag is not set on a feegrant grant transaction. * [\#10184](https://github.com/cosmos/cosmos-sdk/pull/10184) Fixed CLI tx commands to no longer explicitly require the chain-id flag as this value can come from a user config. - +* (x/upgrade) [\#10189](https://github.com/cosmos/cosmos-sdk/issues/10189) Removed potential sources of non-determinism in upgrades ### State Machine Breaking @@ -148,6 +148,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ token holders of a specific denomination. `DenomOwners` is updated to use the new reverse index. * (x/bank) [\#9832] (https://github.com/cosmos/cosmos-sdk/pull/9832) Account balance is stored as `sdk.Int` rather than `sdk.Coin`. * (x/bank) [\#9890] (https://github.com/cosmos/cosmos-sdk/pull/9890) Remove duplicate denom from denom metadata key. +* (x/upgrade) [\#10189](https://github.com/cosmos/cosmos-sdk/issues/10189) Removed potential sources of non-determinism in upgrades ### Deprecated diff --git a/store/rootmulti/store.go b/store/rootmulti/store.go index daf3c22ace34..d00302e7aa6f 100644 --- a/store/rootmulti/store.go +++ b/store/rootmulti/store.go @@ -192,7 +192,22 @@ func (rs *Store) loadVersion(ver int64, upgrades *types.StoreUpgrades) error { // load each Store (note this doesn't panic on unmounted keys now) var newStores = make(map[types.StoreKey]types.CommitKVStore) - for key, storeParams := range rs.storesParams { + storesKeys := make([]types.StoreKey, 0, len(rs.storesParams)) + + for key := range rs.storesParams { + storesKeys = append(storesKeys, key) + } + if upgrades != nil { + // deterministic iteration order for upgrades + // (as the underlying store may change and + // upgrades make store changes where the execution order may matter) + sort.Slice(storesKeys, func(i, j int) bool { + return storesKeys[i].Name() < storesKeys[j].Name() + }) + } + + for _, key := range storesKeys { + storeParams := rs.storesParams[key] commitID := rs.getCommitID(infos, key.Name()) // If it has been added, set the initial version diff --git a/types/module/module.go b/types/module/module.go index 9b8dde31455c..7341edf8e816 100644 --- a/types/module/module.go +++ b/types/module/module.go @@ -30,6 +30,7 @@ package module import ( "encoding/json" + "sort" "github.com/gorilla/mux" "github.com/grpc-ecosystem/grpc-gateway/runtime" @@ -391,7 +392,18 @@ func (m Manager) RunMigrations(ctx sdk.Context, cfg Configurator, fromVM Version } updatedVM := make(VersionMap) - for moduleName, module := range m.Modules { + // for deterministic iteration order + // (as some migrations depend on other modules + // and the order of executing migrations matters) + // TODO: make the order user-configurable? + sortedModNames := make([]string, 0, len(m.Modules)) + for key := range m.Modules { + sortedModNames = append(sortedModNames, key) + } + sort.Strings(sortedModNames) + + for _, moduleName := range sortedModNames { + module := m.Modules[moduleName] fromVersion, exists := fromVM[moduleName] toVersion := module.ConsensusVersion() diff --git a/x/upgrade/keeper/keeper.go b/x/upgrade/keeper/keeper.go index 918a7dc7e4cb..a6a5ee9552e2 100644 --- a/x/upgrade/keeper/keeper.go +++ b/x/upgrade/keeper/keeper.go @@ -7,6 +7,7 @@ import ( "os" "path" "path/filepath" + "sort" "github.com/tendermint/tendermint/libs/log" tmos "github.com/tendermint/tendermint/libs/os" @@ -84,7 +85,18 @@ func (k Keeper) SetModuleVersionMap(ctx sdk.Context, vm module.VersionMap) { if len(vm) > 0 { store := ctx.KVStore(k.storeKey) versionStore := prefix.NewStore(store, []byte{types.VersionMapByte}) - for modName, ver := range vm { + // Even though the underlying store (cachekv) store is sorted, we still + // prefer a deterministic iteration order of the map, to avoid undesired + // surprises if we ever change stores. + sortedModNames := make([]string, 0, len(vm)) + + for key := range vm { + sortedModNames = append(sortedModNames, key) + } + sort.Strings(sortedModNames) + + for _, modName := range sortedModNames { + ver := vm[modName] nameBytes := []byte(modName) verBytes := make([]byte, 8) binary.BigEndian.PutUint64(verBytes, ver)