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/bank setSupply is inefficient #8931

Closed
4 tasks
aaronc opened this issue Mar 18, 2021 · 5 comments · Fixed by #8950
Closed
4 tasks

x/bank setSupply is inefficient #8931

aaronc opened this issue Mar 18, 2021 · 5 comments · Fixed by #8950
Assignees
Labels
C:x/bank T:Bug T: Performance Performance improvements
Milestone

Comments

@aaronc
Copy link
Member

aaronc commented Mar 18, 2021

Summary of Bug

In #7092, we removed the global supply blog but setSupply, BurnCoins and MintCoins are still iterating over the whole supply and re-writing the whole supply store for every operation.

// update supply for coins which have non zero amount
for _, coin := range storeSupply {
if supply.AmountOf(coin.Denom).IsZero() {
zeroCoin := &sdk.Coin{
Denom: coin.Denom,
Amount: sdk.NewInt(0),
}
bz := k.cdc.MustMarshalBinaryBare(zeroCoin)
supplyStore.Set([]byte(coin.Denom), bz)
}
}
newSupply = append(newSupply, supply...)
for i := range newSupply {
bz := k.cdc.MustMarshalBinaryBare(&supply[i])
supplyStore.Set([]byte(supply[i].Denom), bz)
}

supply := k.GetTotalSupply(ctx)
supply = supply.Sub(amt)
k.setSupply(ctx, supply)

Proposed Fix

Set the supply one denom at a time rather than reading and writing the whole supply for each operation. A related issue may be #4076 as the sdk.Coins array is inherently inefficient when there are multiple denoms.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@aaronc aaronc added this to the v0.43 milestone Mar 19, 2021
@aaronc
Copy link
Member Author

aaronc commented Mar 19, 2021

I think this should go in v0.43 to complete #7092.

@aaronc
Copy link
Member Author

aaronc commented Mar 19, 2021

I can open a small PR if it's helpful.

@jgimeno
Copy link
Contributor

jgimeno commented Mar 22, 2021

Remember that when we set some supply which is zero, in the past as we rewrote the full struct we did not have this problem, but as soon as the value is zero it needs to rewrite the zero value.

@jgimeno
Copy link
Contributor

jgimeno commented Mar 22, 2021

@aaronc if it is ok I am gonna take this one

@aaronc
Copy link
Member Author

aaronc commented Mar 22, 2021

@aaronc if it is ok I am gonna take this one

go for it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/bank T:Bug T: Performance Performance improvements
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants