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

Bank genesis allows the supply to be incorrectly set #8919

Closed
4 tasks
aaronc opened this issue Mar 17, 2021 · 4 comments · Fixed by #8930
Closed
4 tasks

Bank genesis allows the supply to be incorrectly set #8919

aaronc opened this issue Mar 17, 2021 · 4 comments · Fixed by #8930

Comments

@aaronc
Copy link
Member

aaronc commented Mar 17, 2021

Summary of Bug

Supply is just a sum of balances. Allowing it to be set separately in genesis is incorrect and should be considered a bug. This code allows the supply in genesis to override what is calculated by summing up the balances (totalSupply):

if genState.Supply.Empty() {
genState.Supply = totalSupply
}
k.setSupply(ctx, genState.Supply)
.

Proposed Fix

Use the bank GenesisState.supply field to check that the supply is correct in InitGenesis and ValidateGenesis rather than allowing it to override the sum of the balances.

Version

Master

Steps to Reproduce

Just set supply in genesis to an incorrect supply.


For Admin Use

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

aaronc commented Mar 17, 2021

I suggest we try to get this into 0.43 because IMHO it should go together with the proper balancing tracking work that's already been done.

@aaronc
Copy link
Member Author

aaronc commented Mar 17, 2021

An alternative to removing is using it correctly as an invariant check in InitGenesis and ValidateGenesis unlike the current code.

@alexanderbez
Copy link
Contributor

There is no proposal in the issue body @aaronc :)

@aaronc aaronc changed the title Remove supply from bank genesis Supply from bank genesis allows the supply to be incorrectly set Mar 18, 2021
@aaronc aaronc changed the title Supply from bank genesis allows the supply to be incorrectly set Bank genesis allows the supply to be incorrectly set Mar 18, 2021
@mergify mergify bot closed this as completed in #8930 Mar 22, 2021
@aaronc aaronc removed the in-review label Mar 22, 2021
@colin-axner
Copy link
Contributor

colin-axner commented May 20, 2021

An alternative to removing is using it correctly as an invariant check in InitGenesis and ValidateGenesis unlike the current code.

I'm in favor of this solution. I find the current solution to be inconsistent. See gaia issue. Some of the gaiad commands will update the supply while others don't causing supply validation issues if used together. It seems to me the supply set in the genesis is unnecessary and will only lead to problems. I guess it is useful in being able to visually see the total supply before starting a chain, but maybe this could be some cli command instead?

If sticking with the current solution and not removing supply, I think it'd be best to add a comment into the genesis file if possible indicating the new behaviour. It'd also be great to remove all supply updates from the associated genutil commands

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

Successfully merging a pull request may close this issue.

4 participants