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.

Sign up for GitHub

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

fix: add app version to params #11182

Closed
wants to merge 19 commits into from
Closed

fix: add app version to params #11182

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
cdec534
add app version
tac0turtle Feb 14, 2022
d59a055
set app version
tac0turtle Feb 14, 2022
7f87350
protocol version is consensusparam
tac0turtle Feb 14, 2022
1fead59
revert set app version
tac0turtle Feb 14, 2022
69b3599
reuse upgrade module param
tac0turtle Feb 15, 2022
e1c1a55
revert comment
tac0turtle Feb 15, 2022
f892ab6
revert to baseapp params
tac0turtle Feb 15, 2022
d56f5d6
remove app vesion get
tac0turtle Feb 15, 2022
ef4446f
fix comment
tac0turtle Feb 15, 2022
1a40fb1
revert public and init
tac0turtle Feb 15, 2022
cf53846
add store migration
tac0turtle Feb 15, 2022
93f981c
add doc
tac0turtle Feb 15, 2022
840d964
Merge branch 'master' into 10791_app_version
tac0turtle Feb 15, 2022
0533538
Update x/params/types/consensus_params.go
tac0turtle Feb 15, 2022
cc80298
Update baseapp/options.go
tac0turtle Feb 16, 2022
a46b0f1
Update baseapp/options.go
tac0turtle Feb 16, 2022
52b435e
Merge branch 'master' into 10791_app_version
tac0turtle Mar 9, 2022
7501871
Merge branch 'master' into 10791_app_version
tac0turtle Mar 27, 2022
e704983
Merge branch 'master' into 10791_app_version
tac0turtle Apr 8, 2022
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 baseapp/abci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -753,6 +753,7 @@ func handleQueryApp(app *BaseApp, path []string, req abci.RequestQuery) abci.Res
}

case "version":

return abci.ResponseQuery{
Codespace: sdkerrors.RootCodespace,
Height: req.Height,
Expand Down
5 changes: 4 additions & 1 deletion baseapp/baseapp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ type BaseApp struct { // nolint: maligned
// ResponseCommit.RetainHeight.
minRetainBlocks uint64

// application's version string
// version represents the application software semantic version.
version string
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a separate field? IMO we only need the AppVersion field

Copy link
Member Author

Choose a reason for hiding this comment

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

version is used to represent the applications semantic software version, tendermint asks for both. In most cases I think its empty because it set via a flag.


// application's protocol version that increments on every upgrade
Expand Down Expand Up @@ -395,6 +395,9 @@ func (app *BaseApp) GetConsensusParams(ctx sdk.Context) *tmproto.ConsensusParams
cp.Validator = &vp
}

// set app version from upgrade module in consensus params
cp.Version = &tmproto.VersionParams{AppVersion: app.appVersion}

return cp
}

Expand Down
4 changes: 3 additions & 1 deletion baseapp/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,16 +101,18 @@ func (app *BaseApp) SetParamStore(ps ParamStore) {
app.paramStore = ps
}

// SetVersion sets the application's version string.
// SetVersion sets the application's version string. TODO
func (app *BaseApp) SetVersion(v string) {
if app.sealed {
panic("SetVersion() on sealed BaseApp")
}

app.version = v
}

// SetProtocolVersion sets the application's protocol version
func (app *BaseApp) SetProtocolVersion(v uint64) {

app.appVersion = v
}

Expand Down
16 changes: 8 additions & 8 deletions x/upgrade/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ type Keeper struct {
storeKey storetypes.StoreKey // key to access x/upgrade store
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
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.
}

Expand All @@ -51,7 +51,7 @@ func NewKeeper(skipUpgradeHeights map[int64]bool, storeKey storetypes.StoreKey,
storeKey: storeKey,
cdc: cdc,
upgradeHandlers: map[string]types.UpgradeHandler{},
versionSetter: vs,
VersionSetter: vs,
}
}

Expand All @@ -70,8 +70,8 @@ func (k Keeper) setProtocolVersion(ctx sdk.Context, v uint64) {
store.Set([]byte{types.ProtocolVersionByte}, versionBytes)
}

// getProtocolVersion gets the protocol version from state
func (k Keeper) getProtocolVersion(ctx sdk.Context) uint64 {
// GetProtocolVersion gets the protocol version from state
Copy link

Choose a reason for hiding this comment

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

Suggested change
// GetProtocolVersion gets the protocol version from state
// getProtocolVersion gets the protocol version from state

func (k Keeper) GetProtocolVersion(ctx sdk.Context) uint64 {
store := ctx.KVStore(k.storeKey)
ok := store.Has([]byte{types.ProtocolVersionByte})
if ok {
Expand Down Expand Up @@ -325,12 +325,12 @@ func (k Keeper) ApplyUpgrade(ctx sdk.Context, plan types.Plan) {

k.SetModuleVersionMap(ctx, updatedVM)

// incremement the protocol version and set it in state and baseapp
nextProtocolVersion := k.getProtocolVersion(ctx) + 1
// increment the protocol version and set it in state and baseapp
nextProtocolVersion := k.GetProtocolVersion(ctx) + 1
k.setProtocolVersion(ctx, nextProtocolVersion)
if k.versionSetter != nil {
if k.VersionSetter != nil {
// set protocol version on BaseApp
k.versionSetter.SetProtocolVersion(nextProtocolVersion)
k.VersionSetter.SetProtocolVersion(nextProtocolVersion)
}

// Must clear IBC state after upgrade is applied as it is stored separately from the upgrade plan.
Expand Down
4 changes: 3 additions & 1 deletion x/upgrade/module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,9 @@ func (am AppModule) RegisterServices(cfg module.Configurator) {
}

// InitGenesis is ignored, no sense in serializing future upgrades
func (am AppModule) InitGenesis(_ sdk.Context, _ codec.JSONCodec, _ json.RawMessage) []abci.ValidatorUpdate {
func (am AppModule) InitGenesis(ctx sdk.Context, _ codec.JSONCodec, _ json.RawMessage) []abci.ValidatorUpdate {
version := am.keeper.GetProtocolVersion(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently x/upgrade doesn't have a genesis state. I think this line will always return 0. We should probably add a genesis.proto and return a DefaultGenesis for x/upgrade

Copy link
Member Author

Choose a reason for hiding this comment

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

so across genesis upgrades we don't retain app version?

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm this may be wrong then, im not sure where to set this data for snapshot restoring. After we restore the snapshot we have to update the variable in BaseApp.

Copy link
Member Author

@tac0turtle tac0turtle Feb 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

this doesn't work actually. I think the only way this works if its part of BaseApp, not upgrade module. It would be a circular dependency otherwise

am.keeper.VersionSetter.SetProtocolVersion(version)
return []abci.ValidatorUpdate{}
}

Expand Down