Drop duplicate spawn/stop proposals (#254)

* drop duplicate create / stop proposals

* small test fix for #254 (#258)

two different chain ids

* return nil on duplicate proposal

* minor changes to UTs

Co-authored-by: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com>

x/ccv/provider/keeper/proposal.go

@@ -50,6 +50,11 @@ func (k Keeper) StopConsumerChainProposal(ctx sdk.Context, p *types.StopConsumer
 // StopConsumerChain cleans up the states for the given consumer chain ID and, if the given lockUbd is false,
 // it completes the outstanding unbonding operations lock by the consumer chain.
 func (k Keeper) StopConsumerChain(ctx sdk.Context, chainID string, lockUbd, closeChan bool) (err error) {
+	// check that a client for chainID exists
+	if _, found := k.GetConsumerClientId(ctx, chainID); !found {
+		// drop the proposal
+		return nil
+	}
 
 	// clean up states
 	k.DeleteConsumerClientId(ctx, chainID)
@@ -113,6 +118,12 @@ func (k Keeper) StopConsumerChain(ctx sdk.Context, chainID string, lockUbd, clos
 // CreateConsumerClient will create the CCV client for the given consumer chain. The CCV channel must be built
 // on top of the CCV client to ensure connection with the right consumer chain.
 func (k Keeper) CreateConsumerClient(ctx sdk.Context, chainID string, initialHeight clienttypes.Height, lockUbdOnTimeout bool) error {
+	// check that a client for this chain does not exist
+	if _, found := k.GetConsumerClientId(ctx, chainID); found {
+		// drop the proposal
+		return nil
+	}
+
 	// Use the unbonding period on the provider to
 	// compute the unbonding period on the consumer
 	unbondingTime := utils.ComputeConsumerUnbondingPeriod(k.stakingKeeper.UnbondingTime(ctx))

x/ccv/provider/keeper/proposal_test.go

@@ -66,6 +66,7 @@ func (suite *KeeperTestSuite) TestCreateConsumerChainProposal() {
 				suite.Require().NoError(err)
 				proposal, ok = content.(*types.CreateConsumerChainProposal)
 				suite.Require().True(ok)
+				proposal.LockUnbondingOnTimeout = lockUbdOnTimeout
 			}, true, true,
 		},
 		{
@@ -76,6 +77,7 @@ func (suite *KeeperTestSuite) TestCreateConsumerChainProposal() {
 				suite.Require().NoError(err)
 				proposal, ok = content.(*types.CreateConsumerChainProposal)
 				suite.Require().True(ok)
+				proposal.LockUnbondingOnTimeout = lockUbdOnTimeout
 			}, true, false,
 		},
 	}
@@ -103,9 +105,9 @@ func (suite *KeeperTestSuite) TestCreateConsumerChainProposal() {
 					suite.Require().Equal(expectedGenesis, consumerGenesis)
 					suite.Require().NotEqual("", clientId, "consumer client was not created after spawn time reached")
 				} else {
-					gotClient := suite.providerChain.App.(*appProvider.App).ProviderKeeper.GetPendingCreateProposal(ctx, proposal.SpawnTime, chainID)
-					suite.Require().Equal(initialHeight, gotClient.InitialHeight, "pending client not equal to clientstate in proposal")
-					suite.Require().Equal(lockUbdOnTimeout, gotClient.LockUnbondingOnTimeout, "pending client not equal to clientstate in proposal")
+					gotProposal := suite.providerChain.App.(*appProvider.App).ProviderKeeper.GetPendingCreateProposal(ctx, proposal.SpawnTime, chainID)
+					suite.Require().Equal(initialHeight, gotProposal.InitialHeight, "unexpected pending proposal (InitialHeight)")
+					suite.Require().Equal(lockUbdOnTimeout, gotProposal.LockUnbondingOnTimeout, "unexpected pending proposal (LockUnbondingOnTimeout)")
 				}
 			} else {
 				suite.Require().Error(err, "did not return error on invalid case")
@@ -148,18 +150,16 @@ func (suite *KeeperTestSuite) TestIteratePendingStopProposal() {
 
 func (suite *KeeperTestSuite) TestIteratePendingClientInfo() {
 
-	chainID := suite.consumerChain.ChainID
-
 	testCases := []struct {
 		types.CreateConsumerChainProposal
 		ExpDeleted bool
 	}{
 		{
-			CreateConsumerChainProposal: types.CreateConsumerChainProposal{ChainId: chainID, SpawnTime: time.Now().UTC()},
+			CreateConsumerChainProposal: types.CreateConsumerChainProposal{ChainId: "0", SpawnTime: time.Now().UTC()},
 			ExpDeleted:                  true,
 		},
 		{
-			CreateConsumerChainProposal: types.CreateConsumerChainProposal{ChainId: chainID, SpawnTime: time.Now().UTC().Add(time.Hour)},
+			CreateConsumerChainProposal: types.CreateConsumerChainProposal{ChainId: "1", SpawnTime: time.Now().UTC().Add(time.Hour)},
 			ExpDeleted:                  false,
 		},
 	}

x/ccv/provider/proposal_handler_test.go

@@ -13,7 +13,9 @@ import (
 	"github.com/cosmos/interchain-security/x/ccv/provider/types"
 )
 
-func (suite *ProviderTestSuite) TestCreateConsumerChainProposalHandler() {
+// TestConsumerChainProposalHandler tests the handler for consumer chain proposals
+// for both CreateConsumerChainProposal and StopConsumerChainProposal
+func (suite *ProviderTestSuite) TestConsumerChainProposalHandler() {
 	var (
 		ctx     sdk.Context
 		content govtypes.Content

x/ccv/provider/types/errors.go

@@ -6,7 +6,8 @@ import (
 
 // Provider sentinel errors
 var (
-	ErrInvalidProposal          = sdkerrors.Register(ModuleName, 1, "invalid create consumer chain proposal")
-	ErrUnknownConsumerChainId   = sdkerrors.Register(ModuleName, 2, "no consumer chain with this chain id")
-	ErrUnknownConsumerChannelId = sdkerrors.Register(ModuleName, 3, "no consumer chain with this channel id")
+	ErrInvalidCreateProposal    = sdkerrors.Register(ModuleName, 1, "invalid create consumer chain proposal")
+	ErrInvalidStopProposal      = sdkerrors.Register(ModuleName, 2, "invalid stop consumer chain proposal")
+	ErrUnknownConsumerChainId   = sdkerrors.Register(ModuleName, 3, "no consumer chain with this chain id")
+	ErrUnknownConsumerChannelId = sdkerrors.Register(ModuleName, 4, "no consumer chain with this channel id")
 )

x/ccv/provider/types/proposal.go

@@ -57,22 +57,22 @@ func (cccp *CreateConsumerChainProposal) ValidateBasic() error {
 	}
 
 	if strings.TrimSpace(cccp.ChainId) == "" {
-		return sdkerrors.Wrap(ErrInvalidProposal, "consumer chain id must not be blank")
+		return sdkerrors.Wrap(ErrInvalidCreateProposal, "consumer chain id must not be blank")
 	}
 
 	if cccp.InitialHeight.IsZero() {
-		return sdkerrors.Wrap(ErrInvalidProposal, "initial height cannot be zero")
+		return sdkerrors.Wrap(ErrInvalidCreateProposal, "initial height cannot be zero")
 	}
 
 	if len(cccp.GenesisHash) == 0 {
-		return sdkerrors.Wrap(ErrInvalidProposal, "genesis hash cannot be empty")
+		return sdkerrors.Wrap(ErrInvalidCreateProposal, "genesis hash cannot be empty")
 	}
 	if len(cccp.BinaryHash) == 0 {
-		return sdkerrors.Wrap(ErrInvalidProposal, "binary hash cannot be empty")
+		return sdkerrors.Wrap(ErrInvalidCreateProposal, "binary hash cannot be empty")
 	}
 
 	if cccp.SpawnTime.IsZero() {
-		return sdkerrors.Wrap(ErrInvalidProposal, "spawn time cannot be zero")
+		return sdkerrors.Wrap(ErrInvalidCreateProposal, "spawn time cannot be zero")
 	}
 	return nil
 }
@@ -112,11 +112,11 @@ func (sccp *StopConsumerChainProposal) ValidateBasic() error {
 	}
 
 	if strings.TrimSpace(sccp.ChainId) == "" {
-		return sdkerrors.Wrap(ErrInvalidProposal, "consumer chain id must not be blank")
+		return sdkerrors.Wrap(ErrInvalidStopProposal, "consumer chain id must not be blank")
 	}
 
 	if sccp.StopTime.IsZero() {
-		return sdkerrors.Wrap(ErrInvalidProposal, "spawn time cannot be zero")
+		return sdkerrors.Wrap(ErrInvalidStopProposal, "spawn time cannot be zero")
 	}
 	return nil
 }