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

upgrade handler flag #706

Merged
merged 26 commits into from
Aug 3, 2022
Merged

upgrade handler flag #706

merged 26 commits into from
Aug 3, 2022

Conversation

faddat
Copy link
Contributor

@faddat faddat commented Aug 1, 2022

Description

This PR adds an upgrade handler flag to Pylons.

It works like this:

Validators composing >2/3rds of votepower, set a halt height using the --halt-height flag.

Their daemons stop at that height.

the other nodes in the chain can no longer make a block, it's down at that block. 

they then restart the chain as:

pylonsd start --run-upgrade-handlers

This will allow pylons to upgrade, without the governance process. It's anti-pattern for the sdk, but I like the degree of control that it gives, especially during difficult situations. I'll probably upstream it.


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 in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • 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 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)

@MikeSofaer
Copy link
Collaborator

IMO this is the way.

Ideally there would be a more fleshed out executive actions module that could halt the chain, do upgrades, etc.

@faddat
Copy link
Contributor Author

faddat commented Aug 1, 2022

Going to pound on the logic here and call it a night.

There are a bunch of reasons that you're correct @MikeSofaer -- this is better off as part of a set of tools.

@faddat
Copy link
Contributor Author

faddat commented Aug 1, 2022

nice

Copy link

@mustafapylons mustafapylons left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@ThanhNhann
Copy link
Contributor

With the magic, It does not show my info when I push the commit sir lol @faddat

@faddat
Copy link
Contributor Author

faddat commented Aug 2, 2022

What magic?

@faddat
Copy link
Contributor Author

faddat commented Aug 2, 2022

just fixing linting now

@hieuvubk hieuvubk marked this pull request as draft August 2, 2022 08:43
app/app.go Fixed Show fixed Hide fixed
@nghuyenthevinh2000
Copy link
Contributor

Screenshot from 2022-08-02 23-19-35

what I did?

  1. make localnet-build: build a docker image of v0.4.2 pylons
  2. docker container run -it --entrypoint /bin/bash pylons-node
  3. bash test_node_deploy.sh (this runs 0.4.2 pylons binary)
  4. wait for a while
  5. ctrl + c to stop
  6. git clone https://github.com/Pylons-tech/pylons.git
  7. cd pylons & checkout upgrade-handler-flag
  8. go build -o bin/pylonsd -mod=readonly ./cmd/pylonsd (this builds this newest version)
  9. cd bin
  10. ./pylonsd start --run-upgrade-handlers --upgrade-height 100

why docker?

so that environmnents for testing for all machine will be the same.

@hieuvubk hieuvubk marked this pull request as ready for review August 2, 2022 17:45
@hieuvubk
Copy link
Contributor

hieuvubk commented Aug 2, 2022

So I think this pr work on my local test net.
And here are the steps to upgrade chain directly on the node without going through gov:

  • Halt chain.
  • Reinstall binaries
  • Run cmd pylonsd start --run-upgrade-handlers=true --upgrade-height=[next-block-after-halt]
  • If the same message below appears, the upgrade is successful

And I think this is just a temporary solution to reduce the waiting time (we are on testnet). Using gov is still a standard and safe way
Ảnh chụp Màn hình 2022-08-03 lúc 00 51 02
cc @faddat @MikeSofaer @mustafapylons

// Because we upgrade directly on the node without the proposal.
// So create an upgrade plan at the block that needs to be upgraded
if app.upgradeHeight != 0 && app.upgradeHeight == ctx.BlockHeight() {
err := app.UpgradeKeeper.ScheduleUpgrade(ctx, upgradetypes.Plan{Name: upgradev46.UpgradeName, Height: ctx.BlockHeight()})

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
if app.upgradeHeight != 0 && app.upgradeHeight == ctx.BlockHeight() {
err := app.UpgradeKeeper.ScheduleUpgrade(ctx, upgradetypes.Plan{Name: upgradev46.UpgradeName, Height: ctx.BlockHeight()})
if err != nil {
panic(err)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
@nghuyenthevinh2000
Copy link
Contributor

Everything is cool, it is good to go.

Screenshot from 2022-08-03 09-57-30

@faddat
Copy link
Contributor Author

faddat commented Aug 3, 2022

nice reproducibility

@faddat faddat merged commit ec6f8d2 into main Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants