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

Add --unsafe-skip-upgrade CLI flag #5

Closed
aaronc opened this issue Oct 24, 2019 · 7 comments · Fixed by cosmos/cosmos-sdk#5367
Closed

Add --unsafe-skip-upgrade CLI flag #5

aaronc opened this issue Oct 24, 2019 · 7 comments · Fixed by cosmos/cosmos-sdk#5367
Labels
Milestone

Comments

@aaronc
Copy link
Member

aaronc commented Oct 24, 2019

Context

There are scenarios in which a planned upgrade handled by the upgrade module needs to be aborted. This could happen when:

  • an error happens in the upgrade handler in the new binary and the upgrade can't complete
  • there is a problem with the upgrade handler that causes a consensus failure (note that solving this also requires --unsafe-roll-back)

Acceptance Criteria

Given that an upgrade needs to be aborted because the new binary simply doesn't work right
When a user invokes the old binary with the --unsafe-skip-upgrade flag (ex. xrnd start --unsafe-skip-upgrade)
Then old binary will continue to operate as before and clear the planned upgrade from state

@aaronc aaronc added this to the Rhine milestone Oct 24, 2019
@aaronc aaronc self-assigned this Oct 24, 2019
@aaronc aaronc removed their assignment Oct 24, 2019
@ethanfrey
Copy link

You need the upgrade Keeper and access to cli flags, not sure where the best place to do so is - app init or in upgrades/BeginBlock.

Please add your reasoning which is the better place for it

@ethanfrey
Copy link

We need to add the flag here: https://github.com/cosmos/cosmos-sdk/blob/9652ec5a504c12019ce62e8b90d7fd8676d4610f/server/start.go#L83-L84

But where do we run the logic from the flag

@ethanfrey
Copy link

Please start with a proposal on where to add the logic (if flag set, clear keeper plan) and why this is the best location

@sahith-narahari
Copy link

I think it should go to upgrade/beginblock,as importing from child to parent isn't a good idea.
And how about just returning from upgrade/beginblock rather than clearing keeper plan

@sahith-narahari
Copy link

sahith-narahari commented Nov 1, 2019

Also, we don't think it's possible to import keeper in init. Instead accessing flag inside BeginBlocker makes sense.

@sahith-narahari
Copy link

@aaronc @ethanfrey, please review this solution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants