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

feat: custom upgrade module #82

Merged
merged 18 commits into from
Jan 5, 2023
Merged

feat: custom upgrade module #82

merged 18 commits into from
Jan 5, 2023

Conversation

j75689
Copy link
Collaborator

@j75689 j75689 commented Dec 29, 2022

Description

Customize the upgrade module for BNB Inscription, and delete useless APIs from the original module.

Rationale

The original upgrade module needs to be carried out through the gov module, and the upgrade process cannot be smoothed, which is inconsistent with our needs.

Therefore, modifying this module supports soft upgrades.

Example

  1. upgrade config in app.toml

image

  1. default upgrade plan by chain id

image

  1. register specific actions when the upgrade happen
// Register the upgrade keeper
upgradeHandler := map[string]upgradetypes.UpgradeHandler{
	// Add specific actions when the upgrade happen
	// ex.
	// "BEP111": func(ctx sdk.Context, plan upgradetypes.Plan, fromVM module.VersionMap) (module.VersionMap, error) {
	// 	app.Logger().Info("upgrade to ", plan.Name)
	// 	return fromVM, nil
	// },
}

var err error
ms := app.CommitMultiStore()
ctx := sdk.NewContext(ms, tmproto.Header{ChainID: app.ChainID(), Height: app.LastBlockHeight()}, true, app.Logger())
upgradeKeeperOpts := []upgradekeeper.KeeperOption{
	upgradekeeper.RegisterUpgradePlan(ctx, bApp.AppConfig().Upgrade),
	upgradekeeper.RegisterUpgradeHandler(upgradeHandler),
}
app.UpgradeKeeper, err = upgradekeeper.NewKeeper(skipUpgradeHeights, keys[upgradetypes.StoreKey], appCodec, homePath, upgradeKeeperOpts...)
if err != nil {
	panic(err)
}

Changes

Notable changes:

  • x/upgrade

@unclezoro
Copy link
Collaborator

LGTM

@j75689 j75689 force-pushed the upgrade_module branch 4 times, most recently from 08c05fc to 93370fb Compare January 3, 2023 08:48
@github-actions github-actions bot removed the C:Store label Jan 3, 2023
@j75689 j75689 removed the C:x/gov label Jan 3, 2023
@j75689 j75689 force-pushed the upgrade_module branch 2 times, most recently from 1457f2c to 6a71dd6 Compare January 3, 2023 10:07
@j75689 j75689 added r4r and removed C:x/gov labels Jan 3, 2023
@j75689 j75689 marked this pull request as ready for review January 3, 2023 10:11
x/upgrade/abci.go Outdated Show resolved Hide resolved
simapp/app.go Outdated
// set the governance module account as the authority for conducting upgrades
app.UpgradeKeeper = upgradekeeper.NewKeeper(skipUpgradeHeights, keys[upgradetypes.StoreKey], appCodec, homePath, app.BaseApp, authtypes.NewModuleAddress(govtypes.ModuleName).String())
// Register the upgrade keeper
upgradeHandler := map[string]upgradetypes.UpgradeHandler{
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is not proper to write the upgrade handler when new an app.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added a new handler for restarting the app. if the app is restarted, will execute handlers for upgraded.

@unclezoro
Copy link
Collaborator

If the tx handler want to know whether the current height is after some upgrade, how can them implement? It looks like the context does not have such info yet.

x/upgrade/client/client.go Show resolved Hide resolved
x/upgrade/keeper/keeper.go Outdated Show resolved Hide resolved
x/upgrade/keeper/keeper.go Outdated Show resolved Hide resolved
baseapp/baseapp.go Outdated Show resolved Hide resolved
baseapp/baseapp.go Outdated Show resolved Hide resolved
upgradeName, height := parseDoneKey(iter.Key())
if height < ctx.BlockHeight() {
f := k.upgradeInitializer[upgradeName]
if f == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is possible that f is nil, let us tolerate with it.

@unclezoro unclezoro merged commit 4c7b018 into develop Jan 5, 2023
@unclezoro unclezoro deleted the upgrade_module branch April 18, 2023 09:17
@luchenqun
Copy link

luchenqun commented Sep 9, 2024

@j75689 @unclezoro @keefel

In the original Cosmos SDK, the gov module proposal is used for coordinated upgrades. When the specified upgrade height is reached, if users haven't used the new client, they will remain at the specified height waiting for users to stop the chain and replace the old client with a new one. If you want to automate the upgrade, you can use Cosmovisor.

Now you have modified the code to directly write the upgrade logic, which executes the upgrade at the specified height. So I have some questions: Since you're not using the gov module for coordinated upgrades, how do the old clients know about the upcoming upgrade? Because the old clients don't know that an upgrade is needed at a certain height, they will run according to the old logic, which will inevitably lead to consensus errors. How do you solve this?

If I understand correctly, you would coordinate offline. All nodes would be required to replace the old client with the latest client before the upgrade, then update the app.toml file and wait for the upgrade. However, if this process is not well-coordinated, for example, if some users haven't used the latest client or have used the latest client but haven't updated the upgrade configuration in app.toml, it would lead to consensus error issues.

If an extreme situation occurs, for example, if the upgrade was originally planned for height 100, but when block 100 is reached, nodes with more than 2/3+ of the voting weight are still running the old client. Would the upgrade fail, but due to having more than 2/3+ of the voting weight, would the chain still be able to reach consensus and produce blocks?

Additionally, in the example above, it mentions "upgrade config in app.toml". This shouldn't be updating app.toml, right? It's updating the Go code, and the two screenshots are the same.

Thank you!

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

Successfully merging this pull request may close these issues.

4 participants