Skip to content

Commit 6907302

Browse files
mergify[bot]aleem1314
authored andcommitted
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 <[email protected]> Co-authored-by: aleem1314 <[email protected]>
1 parent 9e03b97 commit 6907302

File tree

3 files changed

+76
-6
lines changed

3 files changed

+76
-6
lines changed

CHANGELOG.md

+5
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,11 @@ Ref: https://keepachangelog.com/en/1.0.0/
4242

4343
* (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.
4444

45+
### Improvements
46+
47+
* [\#10407](https://github.com/cosmos/cosmos-sdk/pull/10407) Add validation to `x/upgrade` module's `BeginBlock` to check accidental binary downgrades
48+
49+
4550
## [v0.45.0](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.45.0) - 2022-01-18
4651

4752
### State Machine Breaking

x/upgrade/keeper/keeper.go

+32-6
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,8 @@ import (
44
"context"
55
"encoding/binary"
66
"encoding/json"
7-
"errors"
87
"fmt"
8+
"io/ioutil"
99
"os"
1010
"path/filepath"
1111
"sort"
@@ -33,12 +33,8 @@ type Keeper struct {
3333
skipUpgradeHeights map[int64]bool // map of heights to skip for an upgrade
3434
cdc codec.BinaryCodec // App-wide binary codec
3535
upgradeHandlers map[string]types.UpgradeHandler // map of plan name to upgrade handler
36-
versionModifier server.VersionModifier // implements setting the protocol version field on BaseApp
36+
versionSetter xp.ProtocolVersionSetter // implements setting the protocol version field on BaseApp
3737
downgradeVerified bool // tells if we've already sanity checked that this binary version isn't being used against an old state.
38-
authority string // the address capable of executing and canceling an upgrade. Usually the gov module account
39-
initVersionMap appmodule.VersionMap // the module version map at init genesis
40-
41-
consensusKeeper types.ConsensusKeeper
4238
}
4339

4440
// NewKeeper constructs an upgrade Keeper which requires the following arguments:
@@ -324,6 +320,26 @@ func encodeDoneKey(name string, height int64) []byte {
324320
return key
325321
}
326322

323+
// GetLastCompletedUpgrade returns the last applied upgrade name and height.
324+
func (k Keeper) GetLastCompletedUpgrade(ctx sdk.Context) (string, int64) {
325+
iter := sdk.KVStoreReversePrefixIterator(ctx.KVStore(k.storeKey), []byte{types.DoneByte})
326+
defer iter.Close()
327+
if iter.Valid() {
328+
return parseDoneKey(iter.Key()), int64(binary.BigEndian.Uint64(iter.Value()))
329+
}
330+
331+
return "", 0
332+
}
333+
334+
// parseDoneKey - split upgrade name from the done key
335+
func parseDoneKey(key []byte) string {
336+
if len(key) < 2 {
337+
panic(fmt.Sprintf("expected key of length at least %d, got %d", 2, len(key)))
338+
}
339+
340+
return string(key[1:])
341+
}
342+
327343
// GetDoneHeight returns the height at which the given upgrade was executed
328344
func (k Keeper) GetDoneHeight(ctx context.Context, name string) (int64, error) {
329345
store := k.KVStoreService.OpenKVStore(ctx)
@@ -555,3 +571,13 @@ type upgradeInfo struct {
555571
// Height has types.Plan.Height value
556572
Info string `json:"info,omitempty"`
557573
}
574+
575+
// SetDowngradeVerified updates downgradeVerified.
576+
func (k *Keeper) SetDowngradeVerified(v bool) {
577+
k.downgradeVerified = v
578+
}
579+
580+
// DowngradeVerified returns downgradeVerified.
581+
func (k Keeper) DowngradeVerified() bool {
582+
return k.downgradeVerified
583+
}

x/upgrade/keeper/keeper_test.go

+39
Original file line numberDiff line numberDiff line change
@@ -417,6 +417,45 @@ func (s *KeeperTestSuite) TestLastCompletedUpgradeOrdering() {
417417
require.NoError(err)
418418
}
419419

420+
func (s *KeeperTestSuite) TestLastCompletedUpgrade() {
421+
keeper := s.app.UpgradeKeeper
422+
require := s.Require()
423+
424+
s.T().Log("verify empty name if applied upgrades are empty")
425+
name, height := keeper.GetLastCompletedUpgrade(s.ctx)
426+
require.Equal("", name)
427+
require.Equal(int64(0), height)
428+
429+
keeper.SetUpgradeHandler("test0", func(_ sdk.Context, _ types.Plan, vm module.VersionMap) (module.VersionMap, error) {
430+
return vm, nil
431+
})
432+
433+
keeper.ApplyUpgrade(s.ctx, types.Plan{
434+
Name: "test0",
435+
Height: 10,
436+
})
437+
438+
s.T().Log("verify valid upgrade name and height")
439+
name, height = keeper.GetLastCompletedUpgrade(s.ctx)
440+
require.Equal("test0", name)
441+
require.Equal(int64(10), height)
442+
443+
keeper.SetUpgradeHandler("test1", func(_ sdk.Context, _ types.Plan, vm module.VersionMap) (module.VersionMap, error) {
444+
return vm, nil
445+
})
446+
447+
newCtx := s.ctx.WithBlockHeight(15)
448+
keeper.ApplyUpgrade(newCtx, types.Plan{
449+
Name: "test1",
450+
Height: 15,
451+
})
452+
453+
s.T().Log("verify valid upgrade name and height with multiple upgrades")
454+
name, height = keeper.GetLastCompletedUpgrade(newCtx)
455+
require.Equal("test1", name)
456+
require.Equal(int64(15), height)
457+
}
458+
420459
func TestKeeperTestSuite(t *testing.T) {
421460
suite.Run(t, new(KeeperTestSuite))
422461
}

0 commit comments

Comments
 (0)