Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/upgrade: protocol version tracking #8897

Merged
merged 41 commits into from
Apr 2, 2021
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
e900fb1
-added consensus version tracking to x/upgrade
technicallyty Mar 2, 2021
036db4f
-added interface to module manager -added e2e test for migrations usi…
technicallyty Mar 3, 2021
8c7b105
Merge branch 'master' into ty-8514-module_tracking
technicallyty Mar 3, 2021
efe2817
protocol version p1
technicallyty Mar 4, 2021
32316ef
add protocol version to baseapp
technicallyty Mar 10, 2021
1a51e9e
Merge branch 'master' into ty-7487-protocol_version
technicallyty Mar 11, 2021
0fe4051
rebase against master
technicallyty Mar 11, 2021
a6eae2a
add test
technicallyty Mar 11, 2021
fb696bc
added test case
technicallyty Mar 11, 2021
dc8e0d0
Merge branch 'master' into ty-7487-protocol_version
technicallyty Mar 16, 2021
128926d
cleanup
technicallyty Mar 16, 2021
35a424e
docs and change to bigendian
technicallyty Mar 16, 2021
16b92fb
changelog
technicallyty Mar 16, 2021
d9c29c4
Merge branch 'master' into ty-7487-protocol_version
Mar 18, 2021
07ab2cc
Merge branch 'master' into ty-7487-protocol_version
technicallyty Mar 19, 2021
4039b34
cleanup keeper
technicallyty Mar 19, 2021
61576a1
Merge branch 'ty-7487-protocol_version' of https://github.com/cosmos/…
technicallyty Mar 19, 2021
cb4c0f4
swap appVersion and version
technicallyty Mar 20, 2021
ec21cf5
rebase
technicallyty Mar 20, 2021
21af48c
cleanup
technicallyty Mar 20, 2021
f6e16a2
Merge branch 'master' into ty-7487-protocol_version
technicallyty Mar 22, 2021
ad4fceb
change interface id
technicallyty Mar 22, 2021
d7af939
merge master
technicallyty Mar 23, 2021
6e3da1c
update keeper field name to versionSetter
technicallyty Mar 23, 2021
26ac281
reorder keys and docs
technicallyty Mar 23, 2021
e7309fd
Merge branch 'master' into ty-7487-protocol_version
technicallyty Mar 24, 2021
008bd20
Merge branch 'master' into ty-7487-protocol_version2
technicallyty Mar 25, 2021
71a4dfa
-move interface into exported folder
technicallyty Mar 25, 2021
31a1f12
typo
technicallyty Mar 25, 2021
d20bfc1
typo2
technicallyty Mar 25, 2021
97ab692
Merge branch 'master' into ty-7487-protocol_version
technicallyty Mar 27, 2021
4bf3eaa
Merge branch 'master' into ty-7487-protocol_version
technicallyty Mar 29, 2021
1df5a51
docs on keeper fields
technicallyty Mar 29, 2021
b61ab5a
Merge branch 'master' into ty-7487-protocol_version
technicallyty Mar 29, 2021
b0f7765
Merge branch 'master' into ty-7487-protocol_version
technicallyty Mar 29, 2021
31a6a1a
Merge branch 'master' into ty-7487-protocol_version
technicallyty Mar 30, 2021
a43f7e6
Merge branch 'master' into ty-7487-protocol_version
technicallyty Mar 30, 2021
7760ab1
docs for upgrade NewKeeper
technicallyty Mar 30, 2021
bebb4fe
cleanup
technicallyty Mar 30, 2021
b2003dd
Merge branch 'master' into ty-7487-protocol_version
technicallyty Mar 31, 2021
d86db6f
NewKeeper docs
technicallyty Apr 1, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (x/upgrade) [\#8673](https://github.com/cosmos/cosmos-sdk/pull/8673) Remove IBC logic from x/upgrade. Deprecates IBC fields in an Upgrade Plan. IBC upgrade logic moved to 02-client and an IBC UpgradeProposal is added.
* (x/bank) [\#8517](https://github.com/cosmos/cosmos-sdk/pull/8517) `SupplyI` interface and `Supply` are removed and uses `sdk.Coins` for supply tracking
* (x/auth) [\#8129](https://github.com/cosmos/cosmos-sdk/pull/8828) Updated `SigVerifiableTx.GetPubKeys` method signature to return error.
* (x/upgrade) [\7487](https://github.com/cosmos/cosmos-sdk/pull/8897) Upgrade `Keeper` takes new argument `ProtocolManager` which implements setting a protocol version on baseapp.


### State Machine Breaking
Expand Down
2 changes: 2 additions & 0 deletions baseapp/abci.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,8 @@ func (app *BaseApp) Info(req abci.RequestInfo) abci.ResponseInfo {

return abci.ResponseInfo{
Data: app.name,
Version: app.appVersion,
AppVersion: app.protocolVersion,
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
LastBlockHeight: lastCommitID.Version,
LastBlockAppHash: lastCommitID.Hash,
}
Expand Down
7 changes: 7 additions & 0 deletions baseapp/baseapp.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,9 @@ type BaseApp struct { // nolint: maligned
// application's version string
appVersion string

// application's protocol version that increments on every upgrade
protocolVersion uint64

// recovery handler for app.runTx method
runTxRecoveryMiddleware recoveryMiddleware

Expand Down Expand Up @@ -175,6 +178,10 @@ func (app *BaseApp) AppVersion() string {
return app.appVersion
}

func (app *BaseApp) ProtocolVersion() uint64 {
return app.protocolVersion
}

// Logger returns the logger of the BaseApp.
func (app *BaseApp) Logger() log.Logger {
return app.logger
Expand Down
2 changes: 1 addition & 1 deletion baseapp/baseapp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -498,7 +498,7 @@ func TestInfo(t *testing.T) {
assert.Equal(t, t.Name(), res.GetData())
assert.Equal(t, int64(0), res.LastBlockHeight)
require.Equal(t, []uint8(nil), res.LastBlockAppHash)

require.Equal(t, app.ProtocolVersion(), res.AppVersion)
// ----- test a proper response -------
// TODO
}
Expand Down
8 changes: 8 additions & 0 deletions baseapp/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,14 @@ func (app *BaseApp) SetAppVersion(v string) {
app.appVersion = v
}

type ProtocolManager interface {
SetProtocolVersion(uint64)
}

func (app *BaseApp) SetProtocolVersion(v uint64) {
app.protocolVersion = v
}

func (app *BaseApp) SetDB(db dbm.DB) {
if app.sealed {
panic("SetDB() on sealed BaseApp")
Expand Down
2 changes: 1 addition & 1 deletion simapp/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ func NewSimApp(
)

app.FeeGrantKeeper = feegrantkeeper.NewKeeper(appCodec, keys[feegranttypes.StoreKey], app.AccountKeeper)
app.UpgradeKeeper = upgradekeeper.NewKeeper(skipUpgradeHeights, keys[upgradetypes.StoreKey], appCodec, homePath)
app.UpgradeKeeper = upgradekeeper.NewKeeper(skipUpgradeHeights, keys[upgradetypes.StoreKey], appCodec, homePath, app.BaseApp)

// register the staking hooks
// NOTE: stakingKeeper above is passed by reference, so that it will contain these hooks
Expand Down
32 changes: 31 additions & 1 deletion x/upgrade/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/tendermint/tendermint/libs/log"
tmos "github.com/tendermint/tendermint/libs/os"

"github.com/cosmos/cosmos-sdk/baseapp"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of importing baseapp can we duplicate the ProtocolManager interface here following the expected keepers pattern?

"github.com/cosmos/cosmos-sdk/codec"
"github.com/cosmos/cosmos-sdk/store/prefix"
store "github.com/cosmos/cosmos-sdk/store/types"
Expand All @@ -28,16 +29,18 @@ type Keeper struct {
storeKey sdk.StoreKey
cdc codec.BinaryMarshaler
upgradeHandlers map[string]types.UpgradeHandler
protoManager baseapp.ProtocolManager
}

// NewKeeper constructs an upgrade Keeper
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we document all of the parameters here? I'd also like this to explicitly allow ProtocolManager to be nil.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't seem like this has been addressed @technicallyty, although I guess documenting vs ProtocolVersionSetter would be sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does documentation on just the versionSetter suffice @aaronc? I can add comments to all of them, or maybe can do that in a separate PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like comments on all of them. It can be a separate PR if you prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added comments for all the fields @aaronc

Copy link
Member

@aaronc aaronc Mar 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome that's great. We also do want docs for all arguments to NewKeeper. That is what users of the SDK will see in godocs when they're using NewKeeper.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added doc comments. let me know if the format/content needs to change @aaronc

func NewKeeper(skipUpgradeHeights map[int64]bool, storeKey sdk.StoreKey, cdc codec.BinaryMarshaler, homePath string) Keeper {
func NewKeeper(skipUpgradeHeights map[int64]bool, storeKey sdk.StoreKey, cdc codec.BinaryMarshaler, homePath string, pm baseapp.ProtocolManager) Keeper {
return Keeper{
homePath: homePath,
skipUpgradeHeights: skipUpgradeHeights,
storeKey: storeKey,
cdc: cdc,
upgradeHandlers: map[string]types.UpgradeHandler{},
protoManager: pm,
}
}

Expand All @@ -48,6 +51,28 @@ func (k Keeper) SetUpgradeHandler(name string, upgradeHandler types.UpgradeHandl
k.upgradeHandlers[name] = upgradeHandler
}

// setProtocolVersion sets the protocol version to state
func (k Keeper) setProtocolVersion(ctx sdk.Context, v uint64) {
store := ctx.KVStore(k.storeKey)
versionBytes := make([]byte, 8)
binary.BigEndian.PutUint64(versionBytes, v)
store.Set([]byte{types.ProtocolVersionByte}, versionBytes)
}

// GetAppVersion gets the protocol version from state
func (k Keeper) GetProtocolVersion(ctx sdk.Context) uint64 {
technicallyty marked this conversation as resolved.
Show resolved Hide resolved
store := ctx.KVStore(k.storeKey)
ok := store.Has([]byte{types.ProtocolVersionByte})
if ok {
pvBytes := store.Get([]byte{types.ProtocolVersionByte})
protocolVersion := binary.BigEndian.Uint64(pvBytes)

return protocolVersion
}
// default value
return 0
}

// ScheduleUpgrade schedules an upgrade based on the specified plan.
// If there is another Plan already scheduled, it will overwrite it
// (implicitly cancelling the current plan)
Expand Down Expand Up @@ -189,6 +214,11 @@ func (k Keeper) ApplyUpgrade(ctx sdk.Context, plan types.Plan) {

handler(ctx, plan)

// incremement the protocol version and set it in state and baseapp
nextProtoVersion := k.GetProtocolVersion(ctx) + 1
k.setProtocolVersion(ctx, nextProtoVersion)
k.protoManager.SetProtocolVersion(nextProtoVersion)
technicallyty marked this conversation as resolved.
Show resolved Hide resolved

// Must clear IBC state after upgrade is applied as it is stored separately from the upgrade plan.
// This will prevent resubmission of upgrade msg after upgrade is already completed.
k.ClearIBCState(ctx, plan.Height)
Expand Down
16 changes: 15 additions & 1 deletion x/upgrade/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ func (s *KeeperTestSuite) SetupTest() {
app := simapp.Setup(false)
homeDir := filepath.Join(s.T().TempDir(), "x_upgrade_keeper_test")
app.UpgradeKeeper = keeper.NewKeeper( // recreate keeper in order to use a custom home path
make(map[int64]bool), app.GetKey(types.StoreKey), app.AppCodec(), homeDir,
make(map[int64]bool), app.GetKey(types.StoreKey), app.AppCodec(), homeDir, app.BaseApp,
)
s.T().Log("home dir:", homeDir)
s.homeDir = homeDir
Expand Down Expand Up @@ -191,6 +191,20 @@ func (s *KeeperTestSuite) TestSetUpgradedClient() {

}

func (s *KeeperTestSuite) TestIncrementProtocolVersion() {
currentProtocolVersion := s.app.BaseApp.ProtocolVersion()
s.app.UpgradeKeeper.SetUpgradeHandler("dummy", func(_ sdk.Context, _ types.Plan) {})
dummyPlan := types.Plan{
Name: "dummy",
Info: "some text here",
Height: 100,
}
s.app.UpgradeKeeper.ApplyUpgrade(s.ctx, dummyPlan)
upgradedProtocolVersion := s.app.BaseApp.ProtocolVersion()

s.Require().Equal(currentProtocolVersion+1, upgradedProtocolVersion)
}

func TestKeeperTestSuite(t *testing.T) {
suite.Run(t, new(KeeperTestSuite))
}
9 changes: 7 additions & 2 deletions x/upgrade/spec/02_state.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,12 @@ order: 2
# State

The internal state of the `x/upgrade` module is relatively minimal and simple. The
state only contains the currently active upgrade `Plan` (if one exists) by key
`0x0` and if a `Plan` is marked as "done" by key `0x1`.
state contains the currently active upgrade `Plan` (if one exists) by key
`0x0` and if a `Plan` is marked as "done" by key `0x1`. The state maintains a
`Protocol Version` which can be accessed by key `0x3`.

- Plan: `0x0 -> Plan`
- Done: `0x1 | byte(plan name) -> BigEndian(Block Height)`
- ProtocolVersion: `0x3 -> BigEndian(Protocol Version)`

The `x/upgrade` module contains no genesis state.
3 changes: 3 additions & 0 deletions x/upgrade/types/keys.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ const (
// DoneByte is a prefix for to look up completed upgrade plan by name
DoneByte = 0x1

// ProtocolVersionByte is a prefix to look up Protocol Version
ProtocolVersionByte = 0x3
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved

// KeyUpgradedIBCState is the key under which upgraded ibc state is stored in the upgrade store
KeyUpgradedIBCState = "upgradedIBCState"

Expand Down