fix: add app version to params #11182

tac0turtle · 2022-02-14T15:27:38Z

Description

add app version to consensus params

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...

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 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)

cmwaters

I think this would work. Would be good to test it with Tendermint just to make sure.

baseapp/abci.go

x/params/types/consensus_params.go

amaury1093 · 2022-02-15T09:51:15Z

x/upgrade/module.go

@@ -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)


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

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

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.

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

x/upgrade/keeper/keeper.go

amaury1093 · 2022-02-15T09:53:03Z

baseapp/baseapp.go

@@ -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


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

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.

tac0turtle · 2022-02-15T12:01:21Z

I still need to test this locally, but generally it should solve the issue.

alexanderbez · 2022-02-15T15:13:34Z

baseapp/abci.go

 	return abci.ResponseInfo{
 		Data:             app.name,
 		Version:          app.version,
-		AppVersion:       app.appVersion,
+		AppVersion:       appVersion,


Suggested change

AppVersion: appVersion,

AppVersion: app.GetProtocolVersion(),

baseapp/options.go

baseapp/params.go

x/params/types/consensus_params.go

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

peterbourgon · 2022-03-16T22:22:45Z

x/upgrade/keeper/keeper.go

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

-// getProtocolVersion gets the protocol version from state
+// GetProtocolVersion gets the protocol version from state


Suggested change

// GetProtocolVersion gets the protocol version from state

// getProtocolVersion gets the protocol version from state

peterbourgon · 2022-03-16T22:23:14Z

x/upgrade/keeper/keeper.go

-	versionSetter      xp.ProtocolVersionSetter        // implements setting the protocol version field on BaseApp
+	baseapp            xp.ProtocolVersionSetter        // implements setting the protocol version field on BaseApp


Baseapp is passed here from app.go, the name version setter I found confusing here. Do you think it should be version setter?

Yes.

Interfaces have the primary purpose of decoupling dependencies from the code which depends on them. The keeper depends on a ProtocolVersionSetter, which means anything that provides the method set of the ProtocolVersionSetter interface can be used to satisfy that dependency and "back" this field. It may be the case that the BaseApp is used to satisfy this dependency in "production" code, but that's incidental. The keeper doesn't (and shouldn't!) know about BaseApps. It just knows it needs to call something that can set a protocol version.

happy revert, the interface is the same, but I see your point its confusing naming it baseapp

Yeah, we should keep this as versionSetter.

08d2 · 2022-03-28T19:27:46Z

baseapp/baseapp.go

-// AppVersion returns the application's protocol version.
-func (app *BaseApp) AppVersion() uint64 {
-	return app.appVersion
-}
-


What happens to code that calls this method?

they would need to get the consensus params and see what's in there

they would need to get the consensus params and see what's in there

So this PR is a breaking change. Is that fact, or your proposed fix, documented anywhere? I couldn't find it in the diff for the PR, but I might not be looking in the right place.

And is it actually true that appVersion should be a part of consensus parameters? Is it not possible that an application updates its version in a way which is consensus-compatible?

This will land with documentation, just want to test it first. I haven't had time to test it yet. appVersion is part of the consensus parameters from tendermint, we were not setting it on restart.

alexanderbez · 2022-04-08T15:13:39Z

baseapp/grpcrouter.go

 	gogogrpc "github.com/gogo/protobuf/grpc"
 	abci "github.com/tendermint/tendermint/abci/types"
 	"google.golang.org/grpc"
-
+


nit: format

alexanderbez · 2022-04-08T15:14:22Z

x/params/keeper/migrations.go

+	}
+}
+
+// Migrate2to3 migrates x/staking state from consensus version 2 to 3.


Suggested change

// Migrate2to3 migrates x/staking state from consensus version 2 to 3.

// Migrate1to2 migrates baseapp from consensus version 1 to 2.

Do you think this belongs here? Maybe we need a separate package for baseapp? This doesn't seem right.

Closes: #XXX ## What is the purpose of the change Backporting and expanding on: cosmos#11182 This is an attempt to fix app version bug related to state-sync and tendermit. More context is in the linked-above PR. Upon backporting all relevant commits from the linked-above PR, it was discovered that some integration and simulation tests were failing. With the original design, it was difficult to find the source of the problem due to storing the protocol version in 2 database locations. Therefore, the original work was significantly redesigned to only keep the protocol version in param store. Additionally, the protocol version was assumed to have already been set. Since we are creating a new entry in the database, it is now possible that the protocol version is non-existent at the start. As a result, mechanisms for detecting a missing protocol version were added: - `baseapp.init(...)` * This is called on every restart. Here, we check if the protocol version is already in db. If not, set it to 0 - `baseapp.InitChain(...)` * This is called once at genesis. Here, we always set protocol version to 0 Then, on every upgrade the version gets incremented by 1 ## Brief Changelog - cherry-picked all relevant commits from cosmos#11182 - fixed some issues - addressed some style comments from code review in cosmos#11182 - redesigned to only store protocol version in params store and removed logic for storing it in the upgrade keeper - added logic for detecting missing protocol version and, if missing, setting it to 0 - unit and integration tested - fixed all tests ## Testing and Verifying This change is a trivial rework / code cleanup without any test coverage. This change is already covered by existing tests, such as *(please describe tests)*. TODO: This change needs to be manually tested ## Documentation and Release Note - Does this pull request introduce a new feature or user-facing behavior changes? yes - Is a relevant changelog entry added to the `Unreleased` section in `CHANGELOG.md`? no - How is the feature or change documented? not applicable

Closes: #XXX ## What is the purpose of the change Backporting and expanding on: cosmos#11182 This is an attempt to fix app version bug related to state-sync and tendermit. More context is in the linked-above PR. Upon backporting all relevant commits from the linked-above PR, it was discovered that some integration and simulation tests were failing. With the original design, it was difficult to find the source of the problem due to storing the protocol version in 2 database locations. Therefore, the original work was significantly redesigned to only keep the protocol version in param store. Additionally, the protocol version was assumed to have already been set. Since we are creating a new entry in the database, it is now possible that the protocol version is non-existent at the start. As a result, mechanisms for detecting a missing protocol version were added: - `baseapp.init(...)` * This is called on every restart. Here, we check if the protocol version is already in db. If not, set it to 0 - `baseapp.InitChain(...)` * This is called once at genesis. Here, we always set protocol version to 0 Then, on every upgrade the version gets incremented by 1 ## Brief Changelog - cherry-picked all relevant commits from cosmos#11182 - fixed some issues - addressed some style comments from code review in cosmos#11182 - redesigned to only store protocol version in params store and removed logic for storing it in the upgrade keeper - added logic for detecting missing protocol version and, if missing, setting it to 0 - unit and integration tested - fixed all tests ## Testing and Verifying This change is a trivial rework / code cleanup without any test coverage. This change is already covered by existing tests, such as *(please describe tests)*. TODO: This change needs to be manually tested ## Documentation and Release Note - Does this pull request introduce a new feature or user-facing behavior changes? yes - Is a relevant changelog entry added to the `Unreleased` section in `CHANGELOG.md`? no - How is the feature or change documented? not applicable (cherry picked from commit a68d28e)

Closes: #XXX ## What is the purpose of the change Backporting and expanding on: cosmos#11182 This is an attempt to fix app version bug related to state-sync and tendermit. More context is in the linked-above PR. Upon backporting all relevant commits from the linked-above PR, it was discovered that some integration and simulation tests were failing. With the original design, it was difficult to find the source of the problem due to storing the protocol version in 2 database locations. Therefore, the original work was significantly redesigned to only keep the protocol version in param store. Additionally, the protocol version was assumed to have already been set. Since we are creating a new entry in the database, it is now possible that the protocol version is non-existent at the start. As a result, mechanisms for detecting a missing protocol version were added: - `baseapp.init(...)` * This is called on every restart. Here, we check if the protocol version is already in db. If not, set it to 0 - `baseapp.InitChain(...)` * This is called once at genesis. Here, we always set protocol version to 0 Then, on every upgrade the version gets incremented by 1 ## Brief Changelog - cherry-picked all relevant commits from cosmos#11182 - fixed some issues - addressed some style comments from code review in cosmos#11182 - redesigned to only store protocol version in params store and removed logic for storing it in the upgrade keeper - added logic for detecting missing protocol version and, if missing, setting it to 0 - unit and integration tested - fixed all tests ## Testing and Verifying This change is a trivial rework / code cleanup without any test coverage. This change is already covered by existing tests, such as *(please describe tests)*. TODO: This change needs to be manually tested ## Documentation and Release Note - Does this pull request introduce a new feature or user-facing behavior changes? yes - Is a relevant changelog entry added to the `Unreleased` section in `CHANGELOG.md`? no - How is the feature or change documented? not applicable (cherry picked from commit a68d28e) Co-authored-by: Roman <roman@osmosis.team>

alexanderbez · 2022-05-25T18:58:52Z

@marbar3778 going to close this. @p0mvn did an excellent job of pushing this to the Osmosis' fork -- we should just upstream that PR.

osmosis-labs#241

add app version

cdec534

github-actions bot added the C:x/params label Feb 14, 2022

tac0turtle added 3 commits February 14, 2022 16:33

set app version

d59a055

protocol version is consensusparam

7f87350

revert set app version

1fead59

cmwaters reviewed Feb 15, 2022

View reviewed changes

baseapp/abci.go Outdated Show resolved Hide resolved

x/params/types/consensus_params.go Show resolved Hide resolved

reuse upgrade module param

69b3599

github-actions bot added C:x/upgrade and removed C:x/params labels Feb 15, 2022

revert comment

e1c1a55

amaury1093 reviewed Feb 15, 2022

View reviewed changes

x/upgrade/keeper/keeper.go Outdated Show resolved Hide resolved

amaury1093 reviewed Feb 15, 2022

View reviewed changes

revert to baseapp params

f892ab6

github-actions bot added the C:x/params label Feb 15, 2022

tac0turtle added 6 commits February 15, 2022 11:43

remove app vesion get

d56f5d6

fix comment

ef4446f

revert public and init

1a40fb1

add store migration

cf53846

add doc

93f981c

Merge branch 'master' into 10791_app_version

840d964

tac0turtle marked this pull request as ready for review February 15, 2022 12:00

tac0turtle requested review from aaronc and alexanderbez as code owners February 15, 2022 12:00

tac0turtle requested review from atheeshp, amaury1093 and cmwaters February 15, 2022 12:00

tac0turtle requested a review from ValarDragon February 15, 2022 12:15

alexanderbez reviewed Feb 15, 2022

View reviewed changes

tac0turtle and others added 3 commits February 15, 2022 16:29

Update x/params/types/consensus_params.go

0533538

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

Update baseapp/options.go

cc80298

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

Update baseapp/options.go

a46b0f1

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

blushi assigned atheeshp Feb 24, 2022

Merge branch 'master' into 10791_app_version

52b435e

peterbourgon reviewed Mar 16, 2022

View reviewed changes

Merge branch 'master' into 10791_app_version

7501871

tac0turtle mentioned this pull request Mar 27, 2022

Wrong Block.Header.Version is got in State Sync on many chains. tendermint/tendermint#8198

Closed

08d2 reviewed Mar 28, 2022

View reviewed changes

Merge branch 'master' into 10791_app_version

e704983

alexanderbez reviewed Apr 8, 2022

View reviewed changes

p0mvn mentioned this pull request May 20, 2022

fix: state-sync - app version stored in params osmosis-labs/cosmos-sdk#241

Merged

alexanderbez closed this May 25, 2022

alexanderbez deleted the 10791_app_version branch May 25, 2022 18:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add app version to params #11182

fix: add app version to params #11182

tac0turtle commented Feb 14, 2022

cmwaters left a comment

amaury1093 Feb 15, 2022

tac0turtle Feb 15, 2022

tac0turtle Feb 15, 2022

tac0turtle Feb 15, 2022 •

edited

Loading

amaury1093 Feb 15, 2022

tac0turtle Feb 15, 2022

tac0turtle commented Feb 15, 2022

alexanderbez Feb 15, 2022

peterbourgon Mar 16, 2022

peterbourgon Mar 16, 2022

tac0turtle Mar 17, 2022

peterbourgon Apr 10, 2022

tac0turtle Apr 10, 2022

alexanderbez Apr 11, 2022

08d2 Mar 28, 2022

tac0turtle Apr 8, 2022

08d2 Apr 10, 2022

08d2 Apr 10, 2022

tac0turtle Apr 11, 2022

alexanderbez Apr 8, 2022

alexanderbez Apr 8, 2022

alexanderbez commented May 25, 2022

	AppVersion: appVersion,
	AppVersion: app.GetProtocolVersion(),

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

		versionSetter xp.ProtocolVersionSetter // implements setting the protocol version field on BaseApp
		baseapp xp.ProtocolVersionSetter // implements setting the protocol version field on BaseApp

	// Migrate2to3 migrates x/staking state from consensus version 2 to 3.
	// Migrate1to2 migrates baseapp from consensus version 1 to 2.

fix: add app version to params #11182

fix: add app version to params #11182

Conversation

tac0turtle commented Feb 14, 2022

Description

Author Checklist

Reviewers Checklist

cmwaters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tac0turtle Feb 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tac0turtle commented Feb 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez commented May 25, 2022

tac0turtle Feb 15, 2022 •

edited

Loading