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

Invoking a "unbond begin" twice causes CLI to panic #1831

Closed
4 tasks
alexanderbez opened this issue Jul 26, 2018 · 6 comments · Fixed by #1997
Closed
4 tasks

Invoking a "unbond begin" twice causes CLI to panic #1831

alexanderbez opened this issue Jul 26, 2018 · 6 comments · Fixed by #1997
Assignees
@alessio
Labels
C:CLI T:Bug

Comments

@alexanderbez
Copy link
Contributor

Summary of Bug

Executing $ gaiacli stake unbond begin ... twice in a row causes the second execution to panic (assuming first was successful).

Running on 0.23.0-e0e31c5

Steps to Reproduce

  1. Execute begin unbond
$ gaiacli stake unbond begin --from=REDACTED \
  --address-delegator=REDACTED \
  --address-validator=REDACTED \
  --chain-id=REDACTED \
  --shares-percent=1

commits to block...
2. Execute again, panic occurs in CLI:

panic: UnmarshalBinary cannot decode empty bytes

goroutine 1 [running]:
github.com/cosmos/cosmos-sdk/x/stake/types.MustUnmarshalDelegation(0xc4207b6e70, 0xc420869b80, 0x29, 0x40, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/home/bez/go/src/github.com/cosmos/cosmos-sdk/x/stake/types/delegation.go:40 +0x170
github.com/cosmos/cosmos-sdk/x/stake/client/cli.getShares(0xdcaae3, 0x5, 0xc4207b6e70, 0x0, 0x0, 0x7ffc078fe76c, 0x1, 0xc4208ea9a0, 0x14, 0x20, ...)
	/home/bez/go/src/github.com/cosmos/cosmos-sdk/x/stake/client/cli/tx.go:257 +0x415
github.com/cosmos/cosmos-sdk/x/stake/client/cli.GetCmdBeginUnbonding.func1(0xc420896480, 0xc4200af810, 0x0, 0x5, 0x0, 0x0)
	/home/bez/go/src/github.com/cosmos/cosmos-sdk/x/stake/client/cli/tx.go:334 +0x21f
github.com/cosmos/cosmos-sdk/vendor/github.com/spf13/cobra.(*Command).execute(0xc420896480, 0xc4200af7c0, 0x5, 0x5, 0xc420896480, 0xc4200af7c0)
	/home/bez/go/src/github.com/cosmos/cosmos-sdk/vendor/github.com/spf13/cobra/command.go:698 +0x46d
github.com/cosmos/cosmos-sdk/vendor/github.com/spf13/cobra.(*Command).ExecuteC(0x1655ce0, 0xde9171, 0x24, 0xc420839678)
	/home/bez/go/src/github.com/cosmos/cosmos-sdk/vendor/github.com/spf13/cobra/command.go:783 +0x2e4
github.com/cosmos/cosmos-sdk/vendor/github.com/spf13/cobra.(*Command).Execute(0x1655ce0, 0xc42086cee0, 0x1655dc0)
	/home/bez/go/src/github.com/cosmos/cosmos-sdk/vendor/github.com/spf13/cobra/command.go:736 +0x2b
github.com/cosmos/cosmos-sdk/vendor/github.com/tendermint/tendermint/libs/cli.Executor.Execute(0x1655ce0, 0xf32b08, 0x2, 0xc4208417a0)
	/home/bez/go/src/github.com/cosmos/cosmos-sdk/vendor/github.com/tendermint/tendermint/libs/cli/setup.go:89 +0x4e
main.main()
	/home/bez/go/src/github.com/cosmos/cosmos-sdk/cmd/gaia/cmd/gaiacli/main.go:146 +0xb88

For Admin Use

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

alexanderbez commented Jul 26, 2018 •
edited
Loading

I know it's calling MustUnmarshalDelegation which panics, but I don't think this is desired output from a CLI utility.

@AdityaSripal
Copy link
Member

Able to reproduce. Not limited to calling twice, calling gaiacli stake unbond begin on a nonexistent validator panics.

@alexanderbez
Copy link
Contributor Author

alexanderbez commented Jul 26, 2018 •
edited
Loading

Yup. I've noticed pretty much any stake command where it cannot find/decode an account, it panics :-/

@AdityaSripal
Copy link
Member

Should be solved with #870

Adding a recover in gaiacli/main should fix this. We can just print the error message, but if the panic happens deep in the state machine, it's associated error is unlikely to be helpful to the user.
Better than a giant stacktrace tho.

@ValarDragon
Copy link
Contributor

ValarDragon commented Jul 26, 2018 •
edited
Loading

Should be solved with #870

I don't think thats the correct framing of this. We should avoid these panics before getting to the relevant code in attempting to execute the tx in the CLI. The panics existence is a different matter for if this did happen on-chain.

Yup. I've noticed pretty much any stake command where it cannot find/decode an account, it panics :-/

Iirc I added a method to ensure sign build broadcast to make sure the account exists. We can just copy paste that method over to the staking commands / within some command the relevant staking commands all run.

@alexanderbez
Copy link
Contributor Author

Iirc I added a method to ensure sign build broadcast to make sure the account exists. We can just copy paste that method over to the staking commands / within some command the relevant staking commands all run.

Yeah, this should suffice I think. We have to just make sure there is no other branch possible for other panics to creep up 👍

@gin gin mentioned this issue Aug 2, 2018
Closed
4 tasks
@alessio alessio mentioned this issue Aug 13, 2018
Merged
rigelrozanski pushed a commit that referenced this issue Aug 15, 2018
@alessio @rigelrozanski
Merge pull request #1997: Handle unmarshalling failures gracefully in…
4fbaee2 
… x/stake commands

* Handle panic gracefully when unbond begin fails

See #1831

* Handle failure to query delegation gracefully.

Closes #1907

* Update PENDING.md

* Reuse stake's error functions

* New ErrBadValidatorAddr error

UnmarshalValidator() checks the address length first;
it does not make sense to attempt unmarshalling if the
address is wrong.

* New ErrBadDelegationAddr error

* Introduce ErrBad{Redelegation,UnbondingDelegation}Addr custom errors to replace errors.New() calls

* Replace ErrBadUnbondingDelegationAddr with ErrBadDelegationAddr to avoid duplication

Thanks: @melekes for pointing this out

* Use sdk.AddrLen instead of hardcoded address length

* s/triple/tuple/ ## mention PR id in PENDING.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@alessio alessio

Labels
C:CLI T:Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Handle unmarshalling failures gracefully in x/stake commands
4 participants
@alessio @alexanderbez @ValarDragon @AdityaSripal