feat!: add protection against accidental downgrades (backport cosmos#…

…10407) (cosmos#11026)

* feat!: add protection against accidental downgrades (cosmos#10407)

## Description

Closes: cosmos#10318

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)

(cherry picked from commit 5622115)

# Conflicts:
#	CHANGELOG.md
#	x/upgrade/keeper/keeper.go

* chore: resolve conflicts

Co-authored-by: MD Aleem <72057206+aleem1314@users.noreply.github.com>
Co-authored-by: aleem1314 <aleem@vitwit.com>

CHANGELOG.md

@@ -41,6 +41,11 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 * (grpc) [\#10985](https://github.com/cosmos/cosmos-sdk/pull/10992) The `/cosmos/tx/v1beta1/txs/{hash}` endpoint returns a 404 when a tx does not exist.
 
+### Improvements
+
+* [\#10407](https://github.com/cosmos/cosmos-sdk/pull/10407) Add validation to `x/upgrade` module's `BeginBlock` to check accidental binary downgrades
+
+
 ## [v0.45.0](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.45.0) - 2022-01-18
 
 ### State Machine Breaking

x/upgrade/abci.go

@@ -22,7 +22,24 @@ import (
 // skipUpgradeHeightArray is a set of block heights for which the upgrade must be skipped
 func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) {
  defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyBeginBlocker)
+
  plan, found := k.GetUpgradePlan(ctx)
+
+ if !k.DowngradeVerified() {
+ k.SetDowngradeVerified(true)
+ lastAppliedPlan, _ := k.GetLastCompletedUpgrade(ctx)
+ // This check will make sure that we are using a valid binary.
+ // It'll panic in these cases if there is no upgrade handler registered for the last applied upgrade.
+ // 1. If there is no scheduled upgrade.
+ // 2. If the plan is not ready.
+ // 3. If the plan is ready and skip upgrade height is set for current height.
+ if !found || !plan.ShouldExecute(ctx) || (plan.ShouldExecute(ctx) && k.IsSkipHeight(ctx.BlockHeight())) {
+ if lastAppliedPlan != "" && !k.HasHandler(lastAppliedPlan) {
+ panic(fmt.Sprintf("Wrong app version %d, upgrade handler is missing for %s upgrade plan", ctx.ConsensusParams().Version.AppVersion, lastAppliedPlan))
+ }
+ }
+ }
+
  if !found {
  return
  }

x/upgrade/abci_test.go

@@ -411,3 +411,70 @@ func TestDumpUpgradeInfoToFile(t *testing.T) {
  err = os.Remove(upgradeInfoFilePath)
  require.Nil(t, err)
 }
+
+// TODO: add testcase to for `no upgrade handler is present for last applied upgrade`.
+func TestBinaryVersion(t *testing.T) {
+ var skipHeight int64 = 15
+ s := setupTest(10, map[int64]bool{skipHeight: true})
+
+ testCases := []struct {
+ name string
+ preRun func() (sdk.Context, abci.RequestBeginBlock)
+ expectPanic bool
+ }{
+ {
+ "test not panic: no scheduled upgrade or applied upgrade is present",
+ func() (sdk.Context, abci.RequestBeginBlock) {
+ req := abci.RequestBeginBlock{Header: s.ctx.BlockHeader()}
+ return s.ctx, req
+ },
+ false,
+ },
+ {
+ "test not panic: upgrade handler is present for last applied upgrade",
+ func() (sdk.Context, abci.RequestBeginBlock) {
+ s.keeper.SetUpgradeHandler("test0", func(_ sdk.Context, _ types.Plan, vm module.VersionMap) (module.VersionMap, error) {
+ return vm, nil
+ })
+
+ err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "Upgrade test", Plan: types.Plan{Name: "test0", Height: s.ctx.BlockHeight() + 2}})
+ require.Nil(t, err)
+
+ newCtx := s.ctx.WithBlockHeight(12)
+ s.keeper.ApplyUpgrade(newCtx, types.Plan{
+ Name: "test0",
+ Height: 12,
+ })
+
+ req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()}
+ return newCtx, req
+ },
+ false,
+ },
+ {
+ "test panic: upgrade needed",
+ func() (sdk.Context, abci.RequestBeginBlock) {
+ err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "Upgrade test", Plan: types.Plan{Name: "test2", Height: 13}})
+ require.Nil(t, err)
+
+ newCtx := s.ctx.WithBlockHeight(13)
+ req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()}
+ return newCtx, req
+ },
+ true,
+ },
+ }
+
+ for _, tc := range testCases {
+ ctx, req := tc.preRun()
+ if tc.expectPanic {
+ require.Panics(t, func() {
+ s.module.BeginBlock(ctx, req)
+ })
+ } else {
+ require.NotPanics(t, func() {
+ s.module.BeginBlock(ctx, req)
+ })
+ }
+ }
+}

x/upgrade/keeper/keeper.go

@@ -3,6 +3,7 @@ package keeper
 import (
  "encoding/binary"
  "encoding/json"
+ "fmt"
  "io/ioutil"
  "os"
  "path"
@@ -32,6 +33,7 @@ type Keeper struct {
  cdc codec.BinaryCodec // App-wide binary codec
  upgradeHandlers map[string]types.UpgradeHandler // map of plan name to upgrade handler
  versionSetter xp.ProtocolVersionSetter // implements setting the protocol version field on BaseApp
+ downgradeVerified bool // tells if we've already sanity checked that this binary version isn't being used against an old state.
 }
 
 // NewKeeper constructs an upgrade Keeper which requires the following arguments:
@@ -228,6 +230,26 @@ func (k Keeper) GetUpgradedConsensusState(ctx sdk.Context, lastHeight int64) ([]
  return bz, true
 }
 
+// GetLastCompletedUpgrade returns the last applied upgrade name and height.
+func (k Keeper) GetLastCompletedUpgrade(ctx sdk.Context) (string, int64) {
+ iter := sdk.KVStoreReversePrefixIterator(ctx.KVStore(k.storeKey), []byte{types.DoneByte})
+ defer iter.Close()
+ if iter.Valid() {
+ return parseDoneKey(iter.Key()), int64(binary.BigEndian.Uint64(iter.Value()))
+ }
+
+ return "", 0
+}
+
+// parseDoneKey - split upgrade name from the done key
+func parseDoneKey(key []byte) string {
+ if len(key) < 2 {
+ panic(fmt.Sprintf("expected key of length at least %d, got %d", 2, len(key)))
+ }
+
+ return string(key[1:])
+}
+
 // GetDoneHeight returns the height at which the given upgrade was executed
 func (k Keeper) GetDoneHeight(ctx sdk.Context, name string) int64 {
  store := prefix.NewStore(ctx.KVStore(k.storeKey), []byte{types.DoneByte})
@@ -410,3 +432,13 @@ type upgradeInfo struct {
  // Height has types.Plan.Height value
  Info string `json:"info,omitempty"`
 }
+
+// SetDowngradeVerified updates downgradeVerified.
+func (k *Keeper) SetDowngradeVerified(v bool) {
+ k.downgradeVerified = v
+}
+
+// DowngradeVerified returns downgradeVerified.
+func (k Keeper) DowngradeVerified() bool {
+ return k.downgradeVerified
+}

x/upgrade/keeper/keeper_test.go

@@ -232,6 +232,45 @@ func (s *KeeperTestSuite) TestMigrations() {
  s.Require().Equal(vmBefore["bank"]+1, vm["bank"])
 }
 
+func (s *KeeperTestSuite) TestLastCompletedUpgrade() {
+ keeper := s.app.UpgradeKeeper
+ require := s.Require()
+
+ s.T().Log("verify empty name if applied upgrades are empty")
+ name, height := keeper.GetLastCompletedUpgrade(s.ctx)
+ require.Equal("", name)
+ require.Equal(int64(0), height)
+
+ keeper.SetUpgradeHandler("test0", func(_ sdk.Context, _ types.Plan, vm module.VersionMap) (module.VersionMap, error) {
+ return vm, nil
+ })
+
+ keeper.ApplyUpgrade(s.ctx, types.Plan{
+ Name: "test0",
+ Height: 10,
+ })
+
+ s.T().Log("verify valid upgrade name and height")
+ name, height = keeper.GetLastCompletedUpgrade(s.ctx)
+ require.Equal("test0", name)
+ require.Equal(int64(10), height)
+
+ keeper.SetUpgradeHandler("test1", func(_ sdk.Context, _ types.Plan, vm module.VersionMap) (module.VersionMap, error) {
+ return vm, nil
+ })
+
+ newCtx := s.ctx.WithBlockHeight(15)
+ keeper.ApplyUpgrade(newCtx, types.Plan{
+ Name: "test1",
+ Height: 15,
+ })
+
+ s.T().Log("verify valid upgrade name and height with multiple upgrades")
+ name, height = keeper.GetLastCompletedUpgrade(newCtx)
+ require.Equal("test1", name)
+ require.Equal(int64(15), height)
+}
+
 func TestKeeperTestSuite(t *testing.T) {
  suite.Run(t, new(KeeperTestSuite))
 }