fix!: avoid panicking on CancelUnbondingDelegation (#977)

* handle CancelUnbondingDelegation message * tests: add cancel-unbond e2e test * fix: appease linter * chore: Hardcode golangci-lint version (#990) * Hardcode golangci-lint version * Hardcode version in CI config --------- Co-authored-by: MSalopek <[email protected]> Co-authored-by: Philip Offtermatt <[email protected]>
cosmos · Jun 12, 2023 · 8cd0ca9 · 8cd0ca9
1 parent 1bb8d39
commit 8cd0ca9
Show file tree

Hide file tree

Showing 9 changed files with 184 additions and 4 deletions.
diff --git a/.github/workflows/golangci-lint.yml b/.github/workflows/golangci-lint.yml
@@ -24,7 +24,7 @@ jobs:
         uses: golangci/golangci-lint-action@v3
         with:
           # Optional: version of golangci-lint to use in form of v1.2 or v1.2.3 or `latest` to use the latest version
-          version: latest
+          version: v1.52.2
 
           # Optional: working directory, useful for monorepos
           # working-directory: somedir

diff --git a/Makefile b/Makefile
@@ -75,7 +75,7 @@ test-no-cache:
 ###                                Linting                                  ###
 ###############################################################################
 
-golangci_version=latest
+golangci_version=v1.52.2
 
 lint:
 	@echo "--> Running linter"

diff --git a/tests/e2e/actions.go b/tests/e2e/actions.go
@@ -14,6 +14,7 @@ import (
 	evidencetypes "github.com/cosmos/cosmos-sdk/x/evidence/types"
 	clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types"
 	consumertypes "github.com/cosmos/interchain-security/x/ccv/consumer/types"
+	"github.com/tidwall/gjson"
 
 	"github.com/cosmos/interchain-security/x/ccv/provider/client"
 	"github.com/cosmos/interchain-security/x/ccv/provider/types"
@@ -985,6 +986,74 @@ func (tr TestRun) unbondTokens(
 	tr.waitBlocks(action.chain, 2, 20*time.Second)
 }
 
+type cancelUnbondTokensAction struct {
+	chain     chainID
+	delegator validatorID
+	validator validatorID
+	amount    uint
+}
+
+func (tr TestRun) cancelUnbondTokens(
+	action cancelUnbondTokensAction,
+	verbose bool,
+) {
+	validator := tr.validatorConfigs[action.validator].valoperAddress
+	if tr.validatorConfigs[action.validator].useConsumerKey {
+		validator = tr.validatorConfigs[action.validator].consumerValoperAddress
+	}
+
+	// get creation-height from state
+	//#nosec G204 -- Bypass linter warning for spawning subprocess with cmd arguments.
+	cmd := exec.Command("docker", "exec", tr.containerConfig.instanceName, tr.chainConfigs[action.chain].binaryName,
+		"q", "staking", "unbonding-delegation",
+		tr.validatorConfigs[action.delegator].delAddress,
+		validator,
+		`--home`, tr.getValidatorHome(action.chain, action.delegator),
+		`--node`, tr.getValidatorNode(action.chain, action.delegator),
+		`-o`, `json`,
+	)
+	if verbose {
+		fmt.Println("get unbonding delegations cmd:", cmd.String())
+	}
+
+	bz, err := cmd.CombinedOutput()
+	if err != nil {
+		log.Fatal(err, "\n", string(bz))
+	}
+	creationHeight := gjson.Get(string(bz), "entries.0.creation_height").Int()
+	if creationHeight == 0 {
+		log.Fatal("invalid creation height")
+	}
+
+	//#nosec G204 -- Bypass linter warning for spawning subprocess with cmd arguments.
+	cmd = exec.Command("docker", "exec", tr.containerConfig.instanceName, tr.chainConfigs[action.chain].binaryName,
+		"tx", "staking", "cancel-unbond",
+		validator,
+		fmt.Sprint(action.amount)+`stake`,
+		fmt.Sprint(creationHeight),
+		`--from`, `validator`+fmt.Sprint(action.delegator),
+		`--chain-id`, string(tr.chainConfigs[action.chain].chainId),
+		`--home`, tr.getValidatorHome(action.chain, action.delegator),
+		`--node`, tr.getValidatorNode(action.chain, action.delegator),
+		`--gas`, "900000",
+		`--keyring-backend`, `test`,
+		`-o`, `json`,
+		`-y`,
+	)
+
+	if verbose {
+		fmt.Println("unbond cmd:", cmd.String())
+	}
+
+	bz, err = cmd.CombinedOutput()
+	if err != nil {
+		log.Fatal(err, "\n", string(bz))
+	}
+
+	// wait for inclusion in a block -> '--broadcast-mode block' is deprecated
+	tr.waitBlocks(action.chain, 2, 20*time.Second)
+}
+
 type redelegateTokensAction struct {
 	chain    chainID
 	src      validatorID

diff --git a/tests/e2e/main.go b/tests/e2e/main.go
@@ -127,6 +127,8 @@ func (tr *TestRun) runStep(step Step, verbose bool) {
 		tr.delegateTokens(action, verbose)
 	case unbondTokensAction:
 		tr.unbondTokens(action, verbose)
+	case cancelUnbondTokensAction:
+		tr.cancelUnbondTokens(action, verbose)
 	case redelegateTokensAction:
 		tr.redelegateTokens(action, verbose)
 	case downtimeSlashAction:

diff --git a/tests/e2e/step_delegation.go b/tests/e2e/step_delegation.go
@@ -127,6 +127,95 @@ func stepsUnbond(consumerName string) []Step {
 	}
 }
 
+// stepsCancelUnbond canceling unbonding operation for delegator and validator combination
+// the steps perform a full unbonding where the unbonding delegation is removed from the unbonding queue
+func stepsCancelUnbond(consumerName string) []Step {
+	return []Step{
+		{
+			action: unbondTokensAction{
+				chain:      chainID("provi"),
+				unbondFrom: validatorID("alice"),
+				sender:     validatorID("alice"),
+				amount:     1000000,
+			},
+			state: State{
+				chainID("provi"): ChainState{
+					ValPowers: &map[validatorID]uint{
+						validatorID("alice"): 509,
+						validatorID("bob"):   500,
+						validatorID("carol"): 500,
+					},
+				},
+				chainID("consu"): ChainState{
+					ValPowers: &map[validatorID]uint{
+						// Voting power on consumer should not be affected yet
+						validatorID("alice"): 510,
+						validatorID("bob"):   500,
+						validatorID("carol"): 500,
+					},
+				},
+			},
+		},
+		{
+			action: relayPacketsAction{
+				chain:   chainID("provi"),
+				port:    "provider",
+				channel: 0,
+			},
+			state: State{
+				chainID("consu"): ChainState{
+					ValPowers: &map[validatorID]uint{
+						validatorID("alice"): 509,
+						validatorID("bob"):   500,
+						validatorID("carol"): 500,
+					},
+				},
+			},
+		},
+		{
+			action: cancelUnbondTokensAction{
+				chain:     chainID("provi"),
+				delegator: validatorID("alice"),
+				validator: validatorID("alice"),
+				amount:    1000000, // cancel unbonding the full amount
+			},
+			state: State{
+				chainID("provi"): ChainState{
+					ValPowers: &map[validatorID]uint{
+						validatorID("alice"): 510, // power restored
+						validatorID("bob"):   500,
+						validatorID("carol"): 500,
+					},
+				},
+				chainID("consu"): ChainState{
+					ValPowers: &map[validatorID]uint{
+						// Voting power on consumer should not be affected yet
+						validatorID("alice"): 509,
+						validatorID("bob"):   500,
+						validatorID("carol"): 500,
+					},
+				},
+			},
+		},
+		{
+			action: relayPacketsAction{
+				chain:   chainID("provi"),
+				port:    "provider",
+				channel: 0,
+			},
+			state: State{
+				chainID("consu"): ChainState{
+					ValPowers: &map[validatorID]uint{
+						validatorID("alice"): 510, // power restored on consumer
+						validatorID("bob"):   500,
+						validatorID("carol"): 500,
+					},
+				},
+			},
+		},
+	}
+}
+
 // stepsRedelegateForOptOut tests redelegation, and sets up voting powers s.t
 // alice will have less than 5% of the total voting power. This is needed to
 // test opt-out functionality.

diff --git a/tests/e2e/steps.go b/tests/e2e/steps.go
@@ -18,6 +18,7 @@ var happyPathSteps = concatSteps(
 	stepsDelegate("consu"),
 	stepsAssignConsumerKeyOnStartedChain("consu", "bob"),
 	stepsUnbond("consu"),
+	stepsCancelUnbond("consu"),
 	stepsRedelegateForOptOut("consu"),
 	stepsDowntimeWithOptOut("consu"),
 	stepsRedelegate("consu"),

diff --git a/testutil/keeper/mocks.go b/testutil/keeper/mocks.go
diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go
@@ -103,8 +103,18 @@ func (k Keeper) completeMaturedUnbondingOps(ctx sdk.Context) {
 		// Attempt to complete unbonding in staking module
 		err := k.stakingKeeper.UnbondingCanComplete(ctx, id)
 		if err != nil {
-			// UnbondingCanComplete fails if the unbonding operation was not found,
-			// which means that the state of the x/staking module of cosmos-sdk is invalid.
+			if stakingtypes.ErrUnbondingNotFound.Is(err) {
+				// The unbonding was not found.
+				unbondingType, found := k.stakingKeeper.GetUnbondingType(ctx, id)
+				if found && unbondingType == stakingtypes.UnbondingType_UnbondingDelegation {
+					// If this is an unbonding delegation, it may have been removed
+					// after through a CancelUnbondingDelegation message
+					k.Logger(ctx).Debug("unbonding delegation was already removed:", "unbondingID", id)
+					continue
+				}
+			}
+			// UnbondingCanComplete failing means that the state of the x/staking module
+			// of cosmos-sdk is invalid. An exception is the case handled above
 			panic(fmt.Sprintf("could not complete unbonding op: %s", err.Error()))
 		}
 		k.Logger(ctx).Debug("unbonding operation matured on all consumers", "opID", id)

diff --git a/x/ccv/types/expected_keepers.go b/x/ccv/types/expected_keepers.go
@@ -46,6 +46,7 @@ type StakingKeeper interface {
 	MaxValidators(ctx sdk.Context) uint32
 	GetLastTotalPower(ctx sdk.Context) math.Int
 	GetLastValidators(ctx sdk.Context) (validators []stakingtypes.Validator)
+	GetUnbondingType(ctx sdk.Context, id uint64) (unbondingType stakingtypes.UnbondingType, found bool)
 }
 
 type EvidenceKeeper interface {