feat!: add protection against accidental downgrades #10407

aleem1314 · 2021-10-20T04:48:20Z

Description

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)

x/upgrade/abci.go

…smos/cosmos-sdk into aleem/10318-binary-downgrades

x/upgrade/abci.go

robert-zaremba

When reading the code it's hard to understand the goal of multiple verifyAppliedHandlerPresent calls in the BeginBlocker. I think we can do that check only once at the very beginning. See comments.

x/upgrade/keeper/keeper.go

x/upgrade/abci.go

ValarDragon · 2021-11-19T06:34:49Z

Race test failure looks certainly unrelated. Now that DowngradeVerified is made private, can we merge?

anilcse

lgtm

x/upgrade/keeper/keeper.go

alexanderbez · 2022-01-05T18:04:02Z

x/upgrade/abci.go

@@ -12,6 +12,16 @@ import (
 	"github.com/cosmos/cosmos-sdk/x/upgrade/types"
 )

+// downgradeVerified is a variable that tells if we've already sanity checked that this binary version isn't being used against an old state.
+// If so, we dont run that check every beginblock.
+var downgradeVerified = false


can we have this be part of the keeper instead? Or is that not possible? I just really try to advise against the use of globals when possible.

Aren't keepers supposed to be stateless though?

Kinda sorta, yes. Staking isn't (hooks). This is just a suggestion though.

Putting this variable in keeper is better than using global.
Why keepers should be stateless?

ValarDragon · 2022-01-10T03:54:07Z

Any chance we can get this merged this week? (and have follow-up in issues if needed? Nothing looks blocking, but moreso code hygiene that can probably be done in a small follow-up)

It would be really nice to get this into software versions ASAP, to protect more node operator time. Would like this to not get stuck in open PR purgatory.

Also, can we get this PR backported to v0.44.x and v0.45.x?

robert-zaremba

let's move the downgradeVerified to keeper structure

robert-zaremba · 2022-01-11T09:12:06Z

x/upgrade/abci.go

@@ -12,6 +12,16 @@ import (
 	"github.com/cosmos/cosmos-sdk/x/upgrade/types"
 )

+// downgradeVerified is a variable that tells if we've already sanity checked that this binary version isn't being used against an old state.
+// If so, we dont run that check every beginblock.
+var downgradeVerified = false


Putting this variable in keeper is better than using global.
Why keepers should be stateless?

robert-zaremba · 2022-01-11T09:12:39Z

x/upgrade/abci_test.go

@@ -410,3 +410,70 @@ func TestDumpUpgradeInfoToFile(t *testing.T) {
 	err = os.Remove(upgradeInfoFilePath)
 	require.Nil(err)
 }
+
+// TODO: add testcase to for `no upgrade handler is present for last applied upgrade`.


how about this TODO?

It is hard to add tests for a few cases without exposing/adding few functions. And we cannot move those tests to internal package (getting import cycles).

x/upgrade/keeper/keeper.go

robert-zaremba · 2022-01-11T09:15:45Z

we could backport it to v0.45 if we get it merged today.
cc: @AmauryM

robert-zaremba · 2022-01-19T18:18:08Z

For the record: this changeset is NOT consensus breaking because

We don't modify the state
Happens only once at the first begin blocker call to the upgrade module
It only impacts the local node (it will stop it if it detects a downgrade)

ValarDragon · 2022-01-24T22:36:15Z

Can this be merged?

ValarDragon · 2022-01-25T15:25:10Z

Is this test failure related to the PR? Seems to not be present on master

## Description Closes: #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

…#11026) * feat!: add protection against accidental downgrades (#10407) ## Description Closes: #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 <72057206+aleem1314@users.noreply.github.com> Co-authored-by: aleem1314 <aleem@vitwit.com>

…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 <72057206+aleem1314@users.noreply.github.com> Co-authored-by: aleem1314 <aleem@vitwit.com>

add handler check in BeginBlock

0f65ae5

github-actions bot added the C:x/upgrade label Oct 20, 2021

aleem1314 commented Oct 20, 2021

View reviewed changes

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

aleem1314 and others added 3 commits November 3, 2021 15:25

review changes

7011976

add tests

9d8e964

Merge branch 'master' into aleem/10318-binary-downgrades

6f9780e

anilcse reviewed Nov 3, 2021

View reviewed changes

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

anilcse reviewed Nov 3, 2021

View reviewed changes

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

aleem1314 and others added 7 commits November 3, 2021 19:47

review changes

00aa578

Merge branch 'aleem/10318-binary-downgrades' of https://github.com/co…

bbb6259

…smos/cosmos-sdk into aleem/10318-binary-downgrades

review changes

7f51fac

review changes

b023d1a

review changes

ea8b276

Merge branch 'master' into aleem/10318-binary-downgrades

0bce20c

Merge branch 'master' into aleem/10318-binary-downgrades

cac3107

aleem1314 marked this pull request as ready for review November 10, 2021 06:10

aleem1314 requested a review from aaronc as a code owner November 10, 2021 06:10

aleem1314 and others added 2 commits November 10, 2021 21:02

chore: cleanup

512a73c

Merge branch 'master' into aleem/10318-binary-downgrades

ad8823b

clevinson assigned anilcse Nov 10, 2021

aleem1314 marked this pull request as draft November 11, 2021 06:22

aleem1314 and others added 3 commits November 11, 2021 11:56

chore: add more tests

23ccc63

Merge branch 'aleem/10318-binary-downgrades' of https://github.com/co…

cc9fa0a

…smos/cosmos-sdk into aleem/10318-binary-downgrades

Merge branch 'master' into aleem/10318-binary-downgrades

cf1251a

aleem1314 marked this pull request as ready for review November 11, 2021 06:44

aleem1314 requested review from robert-zaremba, alexanderbez and ValarDragon November 11, 2021 11:31

alexanderbez reviewed Nov 11, 2021

View reviewed changes

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

robert-zaremba reviewed Nov 11, 2021

View reviewed changes

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

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

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

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

ValarDragon mentioned this pull request Nov 19, 2021

chore: Cosmos SDK 0.44.* series upgrade for Osmosis osmosis-labs/osmosis#538

Merged

13 tasks

Merge branch 'master' into aleem/10318-binary-downgrades

3cb9b82

aleem1314 requested review from anilcse and alexanderbez November 24, 2021 06:31

anilcse approved these changes Jan 5, 2022

View reviewed changes

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

alexanderbez reviewed Jan 5, 2022

View reviewed changes

aleem1314 added 2 commits January 10, 2022 10:36

Merge branch 'master' into aleem/10318-binary-downgrades

42c72a9

chore: review changes

97fada7

robert-zaremba approved these changes Jan 11, 2022

View reviewed changes

aleem1314 and others added 2 commits January 12, 2022 18:22

chore: move downgradeVerified to keeper

6bae3a7

Merge branch 'master' into aleem/10318-binary-downgrades

1032b33

amaury1093 added the backport/0.45.x label Jan 19, 2022

Merge branch 'master' into aleem/10318-binary-downgrades

42c6ac5

Merge branch 'master' into aleem/10318-binary-downgrades

4229034

aleem1314 added the A:automerge Automatically merge PR once all prerequisites pass. label Jan 25, 2022

mergify bot merged commit 5622115 into master Jan 25, 2022

mergify bot deleted the aleem/10318-binary-downgrades branch January 25, 2022 16:23

mergify bot mentioned this pull request Jan 25, 2022

feat!: add protection against accidental downgrades (backport #10407) #11026

Merged

faddat mentioned this pull request Feb 28, 2022

v0.45.x akash-network/cosmos-sdk#176

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: add protection against accidental downgrades #10407

feat!: add protection against accidental downgrades #10407

aleem1314 commented Oct 20, 2021

robert-zaremba left a comment

ValarDragon commented Nov 19, 2021

anilcse left a comment

alexanderbez Jan 5, 2022

ValarDragon Jan 5, 2022

alexanderbez Jan 5, 2022

robert-zaremba Jan 11, 2022

ValarDragon commented Jan 10, 2022 •

edited

Loading

robert-zaremba left a comment

robert-zaremba Jan 11, 2022

robert-zaremba Jan 11, 2022

aleem1314 Jan 11, 2022

robert-zaremba commented Jan 11, 2022

robert-zaremba commented Jan 19, 2022

ValarDragon commented Jan 24, 2022

ValarDragon commented Jan 25, 2022

feat!: add protection against accidental downgrades #10407

feat!: add protection against accidental downgrades #10407

Conversation

aleem1314 commented Oct 20, 2021

Description

Author Checklist

Reviewers Checklist

robert-zaremba left a comment

Choose a reason for hiding this comment

ValarDragon commented Nov 19, 2021

anilcse left a comment

Choose a reason for hiding this comment

alexanderbez Jan 5, 2022

Choose a reason for hiding this comment

ValarDragon Jan 5, 2022

Choose a reason for hiding this comment

alexanderbez Jan 5, 2022

Choose a reason for hiding this comment

robert-zaremba Jan 11, 2022

Choose a reason for hiding this comment

ValarDragon commented Jan 10, 2022 • edited Loading

robert-zaremba left a comment

Choose a reason for hiding this comment

robert-zaremba Jan 11, 2022

Choose a reason for hiding this comment

robert-zaremba Jan 11, 2022

Choose a reason for hiding this comment

aleem1314 Jan 11, 2022

Choose a reason for hiding this comment

robert-zaremba commented Jan 11, 2022

robert-zaremba commented Jan 19, 2022

ValarDragon commented Jan 24, 2022

ValarDragon commented Jan 25, 2022

ValarDragon commented Jan 10, 2022 •

edited

Loading