Handle unmarshalling failures gracefully in x/stake commands #1997

alessio · 2018-08-13T08:49:06Z

Introduce new Err{NoDelegation,Bad{Redelegation,UnbondingDelegation,Validator}Addr} functions in stake's types.
stake/types.Unmarshal{Validator,UBD,RED}():
- Return consistent errors on address validation failures.
stake/types.UnmarshalValidator():
- Perform address validation first to avoid attempting unmarshalling at all if the latter fails.
Replace MustUnmarshalDelegation() with UnmarshalDelegation() calls and handle errors gracefully rather than panicking to fix Invoking a "unbond begin" twice causes CLI to panic #1831 and Querying delegation with non-existent validator address causes CLI to panic #1907.

Closes #1907

fedekunze · 2018-08-13T09:17:00Z

x/stake/client/cli/query.go

-			delegation := types.MustUnmarshalDelegation(cdc, key, res)
+			delegation, err := types.UnmarshalDelegation(cdc, key, res)
+			if err != nil {
+				return errors.Errorf("cannot unmarshal delegation: %v", err)


Wouldn't it be better to add a new error function in stake/types/errors.go and use that instead ? you can use the CodeInternal as the CodeType

fedekunze · 2018-08-13T09:17:12Z

x/stake/client/cli/tx.go

-		delegation := types.MustUnmarshalDelegation(cdc, key, resQuery)
+		delegation, err := types.UnmarshalDelegation(cdc, key, resQuery)
+		if err != nil {
+			return sdk.ZeroRat(), errors.Errorf("cannot unmarshal delegation: %v", err)


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

alessio · 2018-08-13T11:54:37Z

@fedekunze addressed your comments, please review

alexanderbez · 2018-08-13T14:06:55Z

Nice work @alessio. Did you notice any other places this kind of thing could happen as well?

fedekunze · 2018-08-13T14:16:20Z

@alexanderbez @alessio I believe there are some in the client/rest/ folder as well

alessio · 2018-08-13T14:21:55Z

@alexanderbez @fedekunze Chances are that there are some more. I'll have a look and amend the PR accordingly.

alexanderbez

LGTM 👍

…to replace errors.New() calls

alessio · 2018-08-13T15:11:50Z

@alexanderbez @fedekunze removed all errors.New() calls. Please review.

alexanderbez · 2018-08-13T16:48:46Z

Will test these changes in the internal testnet 👍

melekes · 2018-08-14T07:02:44Z

x/stake/types/errors.go

@@ -117,6 +126,10 @@ func ErrNotMature(codespace sdk.CodespaceType, operation, descriptor string, got
 	return sdk.NewError(codespace, CodeUnauthorized, msg)
 }

+func ErrBadUnbondingDelegationAddr(codespace sdk.CodespaceType) sdk.Error {


I see no difference between this and ErrBadDelegationAddr err messages.

True that, will amend it.

Addressed - @melekes please review

melekes · 2018-08-14T07:03:24Z

x/stake/types/validator.go

@@ -115,17 +114,16 @@ func MustUnmarshalValidator(cdc *wire.Codec, ownerAddr, value []byte) Validator

 // unmarshal a redelegation from a store key and value
 func UnmarshalValidator(cdc *wire.Codec, ownerAddr, value []byte) (validator Validator, err error) {
+	if len(ownerAddr) != 20 {


magic number

Addressed, please review

@melekes

…oid duplication Thanks: @melekes for pointing this out

alexanderbez

@alessio changes LGTM -- just left minor remarks 👍

alexanderbez · 2018-08-15T12:31:27Z

x/stake/types/errors.go

@@ -125,6 +134,10 @@ func ErrExistingUnbondingDelegation(codespace sdk.CodespaceType) sdk.Error {
 	return sdk.NewError(codespace, CodeInvalidDelegation, "existing unbonding delegation found")
 }

+func ErrBadRedelegationAddr(codespace sdk.CodespaceType) sdk.Error {
+	return sdk.NewError(codespace, CodeInvalidInput, "unexpected address length for this (address, srcValidator, dstValidator) triple")


tuple 🤷‍♂️ ?

alexanderbez · 2018-08-15T12:31:50Z

PENDING.md

@@ -95,3 +95,4 @@ BUG FIXES
 *  \#1787 Fixed bug where Tally fails due to revoked/unbonding validator
 *  \#1787 Fixed bug where Tally fails due to revoked/unbonding validator
 * [basecoin] Fixes coin transaction failure and account query [discussion](https://forum.cosmos.network/t/unmarshalbinarybare-expected-to-read-prefix-bytes-75fbfab8-since-it-is-registered-concrete-but-got-0a141dfa/664/6)
+* [cli] Handle panics gracefully when `gaiacli stake {delegation,unbond}` fail to unmarshal delegation.


Let's reference the PR here 👍

alessio · 2018-08-15T13:13:09Z

Thanks @alexanderbez, amended. Please review

alexanderbez

utACK 👍

rigelrozanski

awesome code looks good!

rigelrozanski · 2018-08-15T19:50:22Z

@alessio - thanks for the PR - in the future it would be great to leave the default template PR checkboxes/bullets in the PR - it's a good reminder for reviewers as to what needs to be done

alessio · 2018-08-16T02:20:30Z

Will do, thanks!

…

On Wed, 15 Aug 2018, 21:50 Rigel, ***@***.***> wrote: @alessio <https://github.com/alessio> - thanks for the PR - in the future it would be great to leave the default template PR checkboxes/bullets in the PR - it's a good reminder for reviewers as to what needs to be done — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1997 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAN_7Jn8vmbsVcc6Be8XifoUhePYJ-j8ks5uRHuDgaJpZM4V6HOk> .

alessio requested review from cwgoes, ebuchman and rigelrozanski as code owners August 13, 2018 08:49

Alessio Treglia added 3 commits August 13, 2018 09:51

Handle panic gracefully when unbond begin fails

e67df7a

See #1831

Handle failure to query delegation gracefully.

2358caf

Closes #1907

Update PENDING.md

e220dcf

fedekunze reviewed Aug 13, 2018

View reviewed changes

Alessio Treglia added 3 commits August 13, 2018 11:07

Reuse stake's error functions

14d7c20

New ErrBadValidatorAddr error

a96cf15

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

New ErrBadDelegationAddr error

b8d4888

alexanderbez approved these changes Aug 13, 2018

View reviewed changes

Introduce ErrBad{Redelegation,UnbondingDelegation}Addr custom errors …

858f6ef

…to replace errors.New() calls

melekes reviewed Aug 14, 2018

View reviewed changes

Alessio Treglia added 2 commits August 14, 2018 13:43

Replace ErrBadUnbondingDelegationAddr with ErrBadDelegationAddr to av…

84917c9

…oid duplication Thanks: @melekes for pointing this out

Use sdk.AddrLen instead of hardcoded address length

e020976

alessio mentioned this pull request Aug 15, 2018

Rename stake Validator.Owner -> .Operator #1950

Merged

alexanderbez suggested changes Aug 15, 2018

View reviewed changes

s/triple/tuple/ ## mention PR id in PENDING.md

306b854

alexanderbez approved these changes Aug 15, 2018

View reviewed changes

rigelrozanski approved these changes Aug 15, 2018

View reviewed changes

rigelrozanski merged commit 4fbaee2 into cosmos:develop Aug 15, 2018

alessio deleted the alessio/1831-avoid-panic-on-unbond-begin branch September 6, 2018 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle unmarshalling failures gracefully in x/stake commands #1997

Handle unmarshalling failures gracefully in x/stake commands #1997

alessio commented Aug 13, 2018 •

edited

Loading

fedekunze Aug 13, 2018 •

edited

Loading

fedekunze Aug 13, 2018

alessio commented Aug 13, 2018

alexanderbez commented Aug 13, 2018

fedekunze commented Aug 13, 2018

alessio commented Aug 13, 2018

alexanderbez left a comment

alessio commented Aug 13, 2018

alexanderbez commented Aug 13, 2018

melekes Aug 14, 2018

alexanderbez Aug 14, 2018

alessio Aug 14, 2018

alessio Aug 14, 2018

melekes Aug 14, 2018

alessio Aug 15, 2018

alexanderbez left a comment

alexanderbez Aug 15, 2018

alexanderbez Aug 15, 2018

alessio commented Aug 15, 2018

alexanderbez left a comment

rigelrozanski left a comment

rigelrozanski commented Aug 15, 2018

alessio commented Aug 16, 2018 via email

Handle unmarshalling failures gracefully in x/stake commands #1997

Handle unmarshalling failures gracefully in x/stake commands #1997

Conversation

alessio commented Aug 13, 2018 • edited Loading

fedekunze Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alessio commented Aug 13, 2018

alexanderbez commented Aug 13, 2018

fedekunze commented Aug 13, 2018

alessio commented Aug 13, 2018

alexanderbez left a comment

Choose a reason for hiding this comment

alessio commented Aug 13, 2018

alexanderbez commented Aug 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alessio commented Aug 15, 2018

alexanderbez left a comment

Choose a reason for hiding this comment

rigelrozanski left a comment

Choose a reason for hiding this comment

rigelrozanski commented Aug 15, 2018

alessio commented Aug 16, 2018 via email

alessio commented Aug 13, 2018 •

edited

Loading

fedekunze Aug 13, 2018 •

edited

Loading