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

Module migration fails when code upload access is set to OnlyAddress #1725

Closed
pwnfoo opened this issue Nov 16, 2023 · 3 comments · Fixed by #1729
Closed

Module migration fails when code upload access is set to OnlyAddress #1725

pwnfoo opened this issue Nov 16, 2023 · 3 comments · Fixed by #1729
Milestone

Comments

@pwnfoo
Copy link

pwnfoo commented Nov 16, 2023

Steps to reproduce

  1. Compile a binary from a 0.3x version and start chain ( tested with tag v.0.33.0)
  2. Set code upload access with OnlyAddress
  3. Propose upgrade to 0.4x, compile a 0.4x binary and upgrade (tested with v0.45.0)
  4. Upgrade panics with below message :
INF migrating module upgrade from version 1 to version 2 module=server
INF migrating module wasm from version 2 to version 3 module=server
panic: upload access: type: empty

goroutine 7 [running]:
github.com/cosmos/cosmos-sdk/x/upgrade/keeper.Keeper.ApplyUpgrade({{_, _}, _, {_, _}, {_, _}, _, {_, _}, ...}, ...)
	github.com/cosmos/cosmos-sdk@v0.47.5/x/upgrade/keeper/keeper.go:361 +0x398
github.com/cosmos/cosmos-sdk/x/upgrade.BeginBlocker(_, {{0x10531c9d0, 0x14000368080}, {0x105330010, 0x14001400e80}, {{0xb, 0x0}, {0x14001248a08, 0x4}, 0x9b, ...}, ...}, ...)

This seems to be an error originating from the ValidateBasic method

@alpe
Copy link
Contributor

alpe commented Nov 16, 2023

Thanks for reporting the issue!
When we migrate the params from the legacy param keeper to the module a ValidateBasic is called. As OnlyAddress was deprecated and removed later, this validation fails now.

The quick solution would be to not use OnlyAddress but AnyOfAddresses instead. Does this work for you?

@pwnfoo
Copy link
Author

pwnfoo commented Nov 16, 2023

I did notice that it was deprecated and the solution works well. However, when looking over at the migration code for v3->v4, specifically here, I thought it was expected for the module to handle this migration automatically.

Seemed a little counter-intuitive, so decided to open this issue.

@alpe
Copy link
Contributor

alpe commented Nov 16, 2023

This is clearly a 🐛 on our side. Removing old code seems to be a bigger challenge. I appreciate the issue and will work on a fix. But this can take some time to be released

@alpe alpe added this to the v0.46.0 milestone Nov 16, 2023
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 a pull request may close this issue.

2 participants