feat!: connect app version with consensus params in end block #16244

cmwaters · 2023-05-22T08:26:21Z

Description

This PR wires up the upgrade module to the app version delivered through the ConsensusParams in EndBlock (thus removing the previous appVersion field in BaseApp). x/upgrade no longer keeps its own copy of the app version but uses the baseapp's canonical app version

NOTE: this touches both baseapp and x/upgrade. As they are different go modules, I can also split them into separate PRs

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)

tac0turtle

there will need to be a store migration to migrate the existing version to consensus params

cmwaters · 2023-05-22T11:50:43Z

there will need to be a store migration to migrate the existing version to consensus params

This is a migration of the upgrade module right? So it would read in the old key and then set the app version in baseapp

tac0turtle · 2023-05-22T11:57:08Z

there will need to be a store migration to migrate the existing version to consensus params

This is a migration of the upgrade module right? So it would read in the old key and then set the app version in baseapp

yes

cmwaters · 2023-05-22T12:47:37Z

Does that mean I need to bump the consensus version from 2 to 3?

tac0turtle · 2023-05-22T13:14:39Z

Does that mean I need to bump the consensus version from 2 to 3?

yea, since its consensus breaking

alexanderbez · 2023-05-22T13:50:29Z

baseapp/baseapp.go

@@ -199,8 +196,19 @@ func (app *BaseApp) Name() string {
 }

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


Can we have this return an error? In the ABCI++ branch, all the ABCI methods return an error so I would prefer this not to panic.

For now, we can panic in Info which we'll adjust once we update the ABCI++ branch.

alexanderbez · 2023-05-22T13:52:18Z

x/upgrade/types/keys.go

+	// LegacyProtocolVersionByte was the prefix to look up Protocol Version (AppVersion)
+	LegacyProtocolVersionByte = 0x3



I would delete this and then define it in the migration file.

cmwaters · 2023-05-23T12:48:07Z

I've come across something of a deadlock here. We don't set state and the consensus params until InitChain which makes sense as CometBFT is providing the consensus params (which contain the app version). However Info is called before InitChain and requires the app version (which isn't set, i.e. Info has no access to state).

Best solution I can think of is to deprecate the app version in the Info call. CometBFT can pull it from their latest state. SDK can panic if InitChain returns an app version that the application doesn't support

into cal/app-version

baseapp/abci.go

x/upgrade/keeper/migrations.go

x/upgrade/keeper/keeper.go

baseapp/options.go

x/upgrade/keeper/migrations.go

into cal/app-version

julienrbrt · 2023-06-23T15:12:58Z

CHANGELOG.md

can we get those changelog entries under unreleased?

gentle ping @cmwaters

Sorry, I left this PR. Have tried to update it and move the changelog entries under unreleased

baseapp/baseapp.go

baseapp/options.go

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

baseapp/abci.go

julienrbrt · 2023-08-31T07:23:33Z

x/upgrade/keeper/keeper.go

@@ -441,6 +402,7 @@ func (k Keeper) GetUpgradePlan(ctx context.Context) (plan types.Plan, err error)
 func (k Keeper) setDone(ctx context.Context, name string) error {
 	store := k.storeService.OpenKVStore(ctx)
 	sdkCtx := sdk.UnwrapSDKContext(ctx)
+	fmt.Println("setting done", "height", sdkCtx.HeaderInfo().Height, "name", name)


This should have been using the logger

I can remove in a follow up

feat!: connect app version with consensus params in end block

00cee72

cmwaters requested a review from a team as a code owner May 22, 2023 08:26

github-actions bot added the C:x/upgrade label May 22, 2023

tac0turtle reviewed May 22, 2023

View reviewed changes

add migration and bump to version 3

bfec2f8

alexanderbez reviewed May 22, 2023

View reviewed changes

cmwaters added 3 commits May 23, 2023 10:48

fix some of the broken tests

6dedbc0

update changelog

e7f45a0

Merge branch 'main' into cal/app-version

3df29df

cmwaters added 4 commits May 24, 2023 16:30

fix info call

b0bdf3b

Merge branch 'cal/app-version' of https://github.com/cmwaters/cosmos-sdk

f46d3c2

into cal/app-version

Merge branch 'main' into cal/app-version

31c5d9b

fix imports

d2f714d

github-advanced-security bot found potential problems May 24, 2023

View reviewed changes

baseapp/abci.go Fixed Show fixed Hide fixed

tac0turtle reviewed May 26, 2023

View reviewed changes

baseapp/abci.go Outdated Show resolved Hide resolved

tac0turtle assigned alexanderbez and atheeshp Jun 3, 2023

alexanderbez reviewed Jun 5, 2023

View reviewed changes

baseapp/abci.go Outdated Show resolved Hide resolved

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

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

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

atheeshp reviewed Jun 6, 2023

View reviewed changes

baseapp/options.go Show resolved Hide resolved

cmwaters added 3 commits June 8, 2023 16:37

Merge branch 'main' into cal/app-version

bd9c4f6

incorporate suggestions

eb97ef5

add a comment for applying upgrades

a7a90ea

github-advanced-security bot found potential problems Jun 8, 2023

View reviewed changes

x/upgrade/keeper/migrations.go Fixed Show fixed Hide fixed

add nil check

0142d23

cmwaters added 3 commits June 16, 2023 15:43

Merge branch 'cal/app-version' of https://github.com/cmwaters/cosmos-sdk

209cd3d

into cal/app-version

Merge branch 'main' into cal/app-version

af8e556

Merge branch 'main' into cal/app-version

a2d9880

julienrbrt reviewed Jun 23, 2023

View reviewed changes

cmwaters added 2 commits August 10, 2023 18:33

Merge remote-tracking branch 'upstream/main' into cal/app-version

e123895

lint

1991f40

tac0turtle requested review from atheeshp and alexanderbez August 17, 2023 12:41

alexanderbez reviewed Aug 17, 2023

View reviewed changes

baseapp/baseapp.go Outdated Show resolved Hide resolved

baseapp/options.go Outdated Show resolved Hide resolved

tac0turtle and others added 5 commits August 18, 2023 16:36

Update baseapp/baseapp.go

37b73a5

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

Update baseapp/options.go

f12fb7c

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

Merge branch 'main' into cal/app-version

8075b5f

lint-fix, go mod replace

dbe58bb

fix upgrade tests

d523bb9

tac0turtle requested review from alexanderbez and julienrbrt August 28, 2023 12:50

Merge branch 'main' into cal/app-version

29f38cb

alexanderbez approved these changes Aug 28, 2023

View reviewed changes

baseapp/abci.go Outdated Show resolved Hide resolved

tac0turtle force-pushed the cal/app-version branch from 5ca9c50 to 7a52769 Compare August 29, 2023 07:05

fix lint

60f3164

tac0turtle force-pushed the cal/app-version branch from 7a52769 to 60f3164 Compare August 29, 2023 07:05

tac0turtle enabled auto-merge August 29, 2023 07:06

tac0turtle added 2 commits August 29, 2023 09:23

lint

61f3fc0

Merge branch 'main' into cal/app-version

0ea7c57

tac0turtle added this pull request to the merge queue Aug 29, 2023

Merged via the queue into cosmos:main with commit 79cc75b Aug 29, 2023
44 of 45 checks passed

julienrbrt reviewed Aug 31, 2023

View reviewed changes

julienrbrt mentioned this pull request May 13, 2024

refactor: move baseapp/AppVersionModifer -> core/app #20361

Merged

12 tasks

lucaslopezf mentioned this pull request Aug 19, 2024

[Feature]: pass protocol version to cometbft #16455

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: connect app version with consensus params in end block #16244

feat!: connect app version with consensus params in end block #16244

cmwaters commented May 22, 2023 •

edited

Loading

tac0turtle left a comment

cmwaters commented May 22, 2023

tac0turtle commented May 22, 2023

cmwaters commented May 22, 2023

tac0turtle commented May 22, 2023

alexanderbez May 22, 2023 •

edited

Loading

alexanderbez May 22, 2023

cmwaters commented May 23, 2023

julienrbrt Jun 23, 2023

tac0turtle Jul 18, 2023

cmwaters Aug 10, 2023

julienrbrt Aug 31, 2023

tac0turtle Aug 31, 2023

		// LegacyProtocolVersionByte was the prefix to look up Protocol Version (AppVersion)
		LegacyProtocolVersionByte = 0x3

feat!: connect app version with consensus params in end block #16244

feat!: connect app version with consensus params in end block #16244

Conversation

cmwaters commented May 22, 2023 • edited Loading

Description

Author Checklist

Reviewers Checklist

tac0turtle left a comment

Choose a reason for hiding this comment

cmwaters commented May 22, 2023

tac0turtle commented May 22, 2023

cmwaters commented May 22, 2023

tac0turtle commented May 22, 2023

alexanderbez May 22, 2023 • edited Loading

Choose a reason for hiding this comment

alexanderbez May 22, 2023

Choose a reason for hiding this comment

cmwaters commented May 23, 2023

julienrbrt Jun 23, 2023

Choose a reason for hiding this comment

tac0turtle Jul 18, 2023

Choose a reason for hiding this comment

cmwaters Aug 10, 2023

Choose a reason for hiding this comment

julienrbrt Aug 31, 2023

Choose a reason for hiding this comment

tac0turtle Aug 31, 2023

Choose a reason for hiding this comment

cmwaters commented May 22, 2023 •

edited

Loading

alexanderbez May 22, 2023 •

edited

Loading