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

x/auth InitGenesis changes account numbers if there's a skipped entry. #13033

Closed
SpicyLemon opened this issue Aug 24, 2022 · 3 comments · Fixed by #13877
Closed

x/auth InitGenesis changes account numbers if there's a skipped entry. #13033

SpicyLemon opened this issue Aug 24, 2022 · 3 comments · Fixed by #13877
Assignees
Labels
C:genesis relating to chain genesis C:x/auth T:Bug

Comments

@SpicyLemon
Copy link
Collaborator

Summary of Bug

In the x/auth InitGenesis function, if the provided GenesisState.Accounts has a max AccountNumber that is greater than the number of Accounts, the account numbers of the created accounts are different from those provided.

For example, if GenesisState.Accounts has account numbers 0, 1, 3, 4, 5; then InitGenesis will change 3, 4, and 5 to 2, 3, and 4 (respectively).

Version

First noticed in 0.46.0, but it looks like it's always been this way.
Still exists in main as of commit 6af89d6ce3f43e8a67ea2ee93c5a49e395ebd09d Wed Aug 24 23:13:11 2022 +0200.

Steps to Reproduce

  1. Create an x/auth GenesisState with a single Accounts entry that has account number 10.
  2. Call InitGenesis.
  3. Look up the account.

Expected result:
The account has account number 10.

Actual result:
The account has account number 0.

The reason I found this, though was in Provenance's import/export sim. Our sim created several accounts, and removed one. Then it exported the current state and imported it again (as a separate app). Then it compared the state of the two apps and found that the states were different because the 2nd app had reduced account numbers.

@SpicyLemon
Copy link
Collaborator Author

I fixed this in our fork with this PR: provenance-io#228
That PR also contains the fix for #13028, but the rest is just for this.

That solution still fixes entries with duplicated account numbers (e.g. if they're all '0'), but only changes them when they're duplicates. It also updates the global account number to the largest one, so new accounts will go up from there.

@ValarDragon ValarDragon added T:Bug C:genesis relating to chain genesis labels Aug 26, 2022
@ValarDragon
Copy link
Contributor

Any chance this PR can be upstreamed to mainline SDK? This feels like a notable problem to me.

cc @julienrbrt

@julienrbrt
Copy link
Member

Thanks for the ping. I'll have a look 👍🏾

@julienrbrt julienrbrt self-assigned this Nov 14, 2022
@julienrbrt julienrbrt moved this to 📝 Todo in Cosmos-SDK Nov 14, 2022
@facundomedica facundomedica self-assigned this Nov 15, 2022
@amaury1093 amaury1093 moved this from 📝 Todo to 👀 Needs Review in Cosmos-SDK Nov 16, 2022
Repository owner moved this from 👀 Needs Review to 👏 Done in Cosmos-SDK Nov 16, 2022
@tac0turtle tac0turtle removed this from Cosmos-SDK Jan 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:genesis relating to chain genesis C:x/auth T:Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants