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

Param name change during on-chain upgrade breaks the start command #9637

Closed
4 tasks
RiccardoM opened this issue Jul 6, 2021 · 6 comments
Closed
4 tasks
Labels
C:x/params S:needs more info This bug can't be addressed until more information is provided by the reporter.

Comments

@RiccardoM
Copy link
Contributor

RiccardoM commented Jul 6, 2021

Summary of Bug

If an on-chain upgrade changes the names of some module parameters, it will break new node from syncing in the future.

Version

v0.42.6

Steps to Reproduce

  1. Start a chain with the defaults module installed.
  2. Perform an on-chain upgrade that changes the name of some module parameter from A to B.
  3. After the upgrade, stop and restart the node.

This should return the following error:

panic: unknown field "B" in types.Params
goroutine 1 [running]:
github.com/cosmos/cosmos-sdk/codec.(*ProtoCodec).MustUnmarshalJSON(0xc0011d6310, 0xc00010e800, 0xff, 0x100, 0x2155b70, 0xc000024200)
         github.com/cosmos/cosmos-sdk@v0.42.4/codec/proto_codec.go:160 +0x98

From what I understand, the problem is that even after the upgrade the software tries reading the genesis state using the old parameter name as it is supposed to do. However, since the parameter name is changed, the node can never start again as the genesis reading will always fail when unmarshaling the JSON file.

This also happens to node that are synced using state sync, not only to nodes that are synced using fast sync.

Tracked also inside desmos-labs/desmos#531


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@alexanderbez
Copy link
Contributor

Interesting. I didn't even know we supported parameter name changes. Why not just deprecate the old and use a new param? Or can this potentially be handled via the migration process?

@RiccardoM
Copy link
Contributor Author

@alexanderbez This was handled via an on-chain upgrade by simply unmarshalling the params using the old types, and then re-marshalling them with the new types, so this can happen to other chains as well

@alexanderbez
Copy link
Contributor

The application does not read genesis state apart from InitGenesis which is only called on the first height. Could it be that other modules are still referencing the old names?

@tac0turtle
Copy link
Member

Are you reusing the same proto field number?

@clevinson clevinson added S:needs more info This bug can't be addressed until more information is provided by the reporter. C:x/params labels Jul 9, 2021
@amaury1093
Copy link
Contributor

amaury1093 commented Jul 9, 2021

@RiccardoM For some context, which module are you using? Is it a custom module or a SDK module?

If you're using genesis JSON migration, do you have a Param proto message in the module (an equivalent of this message)? If yes, the param field should be renamed too, but renaming fields should be considered proto-breaking changes and the proto pkg version should be bumped (cf #9613).

@RiccardoM
Copy link
Contributor Author

@AmauryM It was a custom module that used on-chain migration doing the following:

  1. Read the params as the old JSON definition
  2. Create the new params object
  3. Store the params as with new object JSON definition

Anyway, it appear that this was an error caused by a wrong configuration from the node operator. It was solved by using state sync and no error showed up later on. Thanks everyone, I'm closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/params S:needs more info This bug can't be addressed until more information is provided by the reporter.
Projects
None yet
Development

No branches or pull requests

5 participants