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.

Sign up for GitHub

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

Jump to bottom

Bank and Supply are not pruning zero coins causing invariants to break #9159

Closed
4 tasks
colin-axner opened this issue Apr 21, 2021 · 13 comments · Fixed by #9229
Closed
4 tasks

Bank and Supply are not pruning zero coins causing invariants to break #9159

colin-axner opened this issue Apr 21, 2021 · 13 comments · Fixed by #9229
Assignees
@robert-zaremba @atheeshp
Labels
C:x/bank T:Bug
Milestone
v0.43

Comments

@colin-axner
Copy link
Contributor

colin-axner commented Apr 21, 2021 •
edited
Loading

While updating to the latest SDK commit 045c45f5507315834415c45b6eae5b1755cf002a on a branch in ibc-go, one of our ICS20 transfer tests broke:

--- FAIL: TestTransferTestSuite (1.92s)
    --- FAIL: TestTransferTestSuite/TestHandleMsgTransfer (0.41s)
        suite.go:63: test panicked: invariant broken: bank: total supply invariant
            	sum of accounts coins: 100000279066286stake
            	supply.Total:          0ibc/F48A74D4F2A8B3D0CA0572F682A334C4F9595827BFE030BFE112863A2BC928C0,100000279066286stake
            
            
            	CRITICAL please submit the following transaction:
            		 tx crisis invariant-broken bank total-supply
            goroutine 99 [running]:
            runtime/debug.Stack(0xc000535b70, 0x17be5e0, 0xc001c20b20)
            	/usr/lib/go/src/runtime/debug/stack.go:24 +0x9f
            github.com/stretchr/testify/suite.failOnPanic(0xc000ed4300)
            	/home/bartleby/work/go/pkg/mod/github.com/stretchr/testify@v1.7.0/suite/suite.go:63 +0x5b
            panic(0x17be5e0, 0xc001c20b20)
            	/usr/lib/go/src/runtime/panic.go:965 +0x1b9
            github.com/cosmos/cosmos-sdk/x/crisis/keeper.Keeper.AssertInvariants(0xc000f83900, 0xb, 0x10, 0x1e1c8e8, 0xc001362130, 0xc0011ceb78, 0x1df1aa0, 0xc001363460, 0x1df1af0, 0xc0013634e0, ...)
            	/home/bartleby/work/go/pkg/mod/github.com/cosmos/cosmos-sdk@v0.42.0-alpha1.0.20210421132932-045c45f55073/x/crisis/keeper/keeper.go:83 +0x7b7
            github.com/cosmos/cosmos-sdk/x/crisis.EndBlocker(0x1e0a3a8, 0xc000040150, 0x1e1eae0, 0xc001709800, 0x0, 0x0, 0xc0013330f0, 0xa, 0x1e, 0x0, ...)
            	/home/bartleby/work/go/pkg/mod/github.com/cosmos/cosmos-sdk@v0.42.0-alpha1.0.20210421132932-045c45f55073/x/crisis/abci.go:20 +0x27b
            github.com/cosmos/cosmos-sdk/x/crisis.AppModule.EndBlock(...)
            	/home/bartleby/work/go/pkg/mod/github.com/cosmos/cosmos-sdk@v0.42.0-alpha1.0.20210421132932-045c45f55073/x/crisis/module.go:168
            github.com/cosmos/cosmos-sdk/types/module.(*Manager).EndBlock(0xc001372540, 0x1e0a3a8, 0xc000040150, 0x1e1eae0, 0xc001709800, 0x0, 0x0, 0xc0013330f0, 0xa, 0x1e, ...)
            	/home/bartleby/work/go/pkg/mod/github.com/cosmos/cosmos-sdk@v0.42.0-alpha1.0.20210421132932-045c45f55073/types/module/module.go:454 +0x1e5
            github.com/cosmos/ibc-go/testing/simapp.(*SimApp).EndBlocker(...)
            	/home/bartleby/work/go/src/github.com/cosmos/ibc-go/testing/simapp/app.go:486
            github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).EndBlock(0xc00131f860, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0)
            	/home/bartleby/work/go/pkg/mod/github.com/cosmos/cosmos-sdk@v0.42.0-alpha1.0.20210421132932-045c45f55073/baseapp/abci.go:198 +0x2c6
            github.com/cosmos/ibc-go/testing/simapp.SignCheckDeliver(0xc000ed4300, 0x1e19858, 0xc001064080, 0xc00131f860, 0x0, 0x0, 0xc0013330f0, 0xa, 0x1e, 0x0, ...)
            	/home/bartleby/work/go/src/github.com/cosmos/ibc-go/testing/simapp/test_helpers.go:365 +0x589
            github.com/cosmos/ibc-go/testing.(*TestChain).SendMsgs(0xc001041440, 0xc001056730, 0x1, 0x1, 0x4103b8, 0x10, 0x177f280)
            	/home/bartleby/work/go/src/github.com/cosmos/ibc-go/testing/chain.go:283 +0x3a5
            github.com/cosmos/ibc-go/testing.(*TestChain).sendMsgs(...)
            	/home/bartleby/work/go/src/github.com/cosmos/ibc-go/testing/chain.go:275
            github.com/cosmos/ibc-go/testing.(*Coordinator).SendMsgs(0xc0005ae290, 0xc001041440, 0xc001322900, 0xc001dac5f0, 0xf, 0xc001056730, 0x1, 0x1, 0xa, 0x3b)
            	/home/bartleby/work/go/src/github.com/cosmos/ibc-go/testing/coordinator.go:346 +0x56
            github.com/cosmos/ibc-go/testing.(*Coordinator).SendMsg(...)
            	/home/bartleby/work/go/src/github.com/cosmos/ibc-go/testing/coordinator.go:340
            github.com/cosmos/ibc-go/modules/apps/transfer_test.(*TransferTestSuite).TestHandleMsgTransfer(0xc0000eed40)
            	/home/bartleby/work/go/src/github.com/cosmos/ibc-go/modules/apps/transfer/handler_test.go:96 +0x2417
            reflect.Value.call(0xc000f22240, 0xc000023080, 0x13, 0x19d8613, 0x4, 0xc000f6fe30, 0x1, 0x1, 0xc000f6fcf8, 0x40db8a, ...)
            	/usr/lib/go/src/reflect/value.go:476 +0x8e7
            reflect.Value.Call(0xc000f22240, 0xc000023080, 0x13, 0xc000f6fe30, 0x1, 0x1, 0x21535f9, 0x22, 0x64cb00000439)
            	/usr/lib/go/src/reflect/value.go:337 +0xb9
            github.com/stretchr/testify/suite.Run.func1(0xc000ed4300)
            	/home/bartleby/work/go/pkg/mod/github.com/stretchr/testify@v1.7.0/suite/suite.go:158 +0x379
            testing.tRunner(0xc000ed4300, 0xc000eae2d0)
            	/usr/lib/go/src/testing/testing.go:1194 +0xef
            created by testing.(*T).Run
            	/usr/lib/go/src/testing/testing.go:1239 +0x2b3
FAIL

I have just run into this issue and have not spent time looking into it. Based on the error message, it appears the bank balance is not being pruned after it hits 0 causing the invariant to trigger. This error occurs when we send from A -> B and then B -> A causing the tokens to be fully sent out of the account

cc @AdityaSripal

For Admin Use

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

It is possible I didn't configure something correctly when updating, but the error message seems to clearly indicate there exists a 0 coin

The branch I'm using can be found here

@colin-axner
Copy link
Contributor Author

colin-axner commented Apr 21, 2021 •
edited
Loading

I figured it out. Burning coins is currently on master subtracting from the supply coin by coin. Previous behaviour used Coins to subtract from the total supply.

Coin and Coins do not perform the same operations

Coin will end up here clearly capable of returning a 0coin

Coins appears to remove zero coins after subtraction. I can also see this in the code

Thus the old behaviour pruned zero coins from the total supply.

The issue is two fold

  • supply isn't pruning zero coins (new bug)
  • bank is continuing to store zero coins (existing bug - have not verified, may be wrong)

@colin-axner colin-axner added this to the v0.43 milestone Apr 21, 2021
@colin-axner
Copy link
Contributor Author

cc @AmauryM @clevinson

I added to v0.43 milestone as it needs to be fixed for the next release

@colin-axner colin-axner changed the title Bank is not pruning zero balance accounts causing invariants to break Bank and Supply are not pruning zero coins causing invariants to break Apr 21, 2021
@aaronc
Copy link
Member

aaronc commented Apr 21, 2021

This should be pretty straightforward to fix in bank.

It also strikes me as strange that Coins itself doesn't handle zero coins correctly. First, the zero coin should have never gotten added to Coins to begin with and second IsEqual should have properly dealt with zero values.

@colin-axner
Copy link
Contributor Author

It also strikes me as strange that Coins itself doesn't handle zero coins correctly. First, the zero coin should have never gotten added to Coins to begin with and second IsEqual should have properly dealt with zero values.

Yes I completely agree. The mechanics of coin need to be revisited/audited/documented imo. There's a lot of subtle details that can easily lead to misuse. There's other oddities like bank allowing empty coin sets as valid no-ops

@robert-zaremba
Copy link
Collaborator

We are looking into it with @atheeshp

@robert-zaremba
Copy link
Collaborator

BTW, why do we need Coins? We can do things directly without that structure.

@colin-axner
Copy link
Contributor Author

I realize I left some details off. I believe the issue is here

As you can see, GetSupply() returns sdk.Coin. Thus we end up here and here which results in a 0denom coin being set in supply

If coins was used, we would end up here and safeSub calls safeAdd, which I believe removes zero coins.

Partially related, but I didn't find any indication that bank prunes 0 amount balances as well (technically not a bug, for now). I will link code in a second

@colin-axner
Copy link
Contributor Author

If we send our entire balance away, we should expect our entire balance to be deducted (and pruned) in subUnlockedCoins. First the function only checks if the send results in a negative balance (which is correct), then it performs the same .Sub, using sdk.Coin. Following the same logic as above, the zero coin is not pruned, thus we call setBalance with a 0coin. This functionality seems odd/incorrect to me. Maybe it is safest not to delete the 0coin but it should be well documented if that is the decision (as it disagrees with the expected output of supply which prunes 0coins)

@colin-axner
Copy link
Contributor Author

colin-axner commented Apr 28, 2021 •
edited
Loading

I think there are a variety of fixes here and I'm not sure what is the best approach, but one simple approach is to the check if the amount.IsZero() on setSupply and setBalance and to delete the key/value if that is true

@colin-axner
Copy link
Contributor Author

It'd be great if simulations could be updated to randomly burn the entire supply of some coin (that would catch the existing bug)

@atheeshp atheeshp self-assigned this Apr 28, 2021
@robert-zaremba
Copy link
Collaborator

There is one more bug in the Coins.safeAdd function - it assumes that coins are sorted by denom. If we don't have it then the operation doesn't work.

@colin-axner
Copy link
Contributor Author

There is one more bug in the Coins.safeAdd function - it assumes that coins are sorted by denom. If we don't have it then the operation doesn't work.

I think this is safe since coins.Validate checks that the coins are sorted and we should never .Add/.Sub invalid coins

These sort of idiosyncrasies are not well documented. Another good example of why I think the entire design of Coin needs to be audited and documented in an ADR #7046. It'd be a lot easier to reason about the safety of Bank or other modules with coins if it was clearly documented how everything functioned and what assumptions are made at each step

@aaronc aaronc mentioned this issue Apr 29, 2021
Closed
@atheeshp atheeshp mentioned this issue Apr 29, 2021
Merged
9 tasks
@robert-zaremba robert-zaremba mentioned this issue Apr 30, 2021
Merged
9 tasks
@robert-zaremba robert-zaremba self-assigned this Apr 30, 2021
@mergify mergify bot closed this as completed in #9229 Apr 30, 2021
@aaronc aaronc mentioned this issue Jul 7, 2021
Closed
4 tasks
@AlexiaChen AlexiaChen mentioned this issue Dec 5, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@robert-zaremba robert-zaremba

@atheeshp atheeshp

Labels
C:x/bank T:Bug
Projects
None yet
Milestone
v0.43
Development

Successfully merging a pull request may close this issue.

fix zero coins cosmos/cosmos-sdk
5 participants
@aaronc @robert-zaremba @blushi @colin-axner @atheeshp