fix StopConsumerChain not running in cachedContext (#802)

* fix StopConsumerChain not running in cachedContext

* start hermes in docker tests

* fix some tests

tests/integration/actions.go

@@ -597,7 +597,7 @@ func (tr TestRun) addChainToRelayer(
 	keyName := "query"
 	rpcAddr := "http://" + queryNodeIP + ":26658"
 	grpcAddr := "tcp://" + queryNodeIP + ":9091"
-	wsAddr := "ws://" + queryNodeIP + ":26657/websocket"
+	wsAddr := "ws://" + queryNodeIP + ":26658/websocket"
 
 	chainConfig := fmt.Sprintf(hermesChainConfigTemplate,
 		grpcAddr,
@@ -693,6 +693,28 @@ type addIbcChannelAction struct {
 	order       string
 }
 
+type startHermesAction struct {
+}
+
+func (tr TestRun) startHermes(
+	action startHermesAction,
+	verbose bool,
+) {
+	// hermes start is running in detached mode
+	//#nosec G204 -- Bypass linter warning for spawning subprocess with cmd arguments.
+	cmd := exec.Command("docker", "exec", "-d", tr.containerConfig.instanceName, "hermes",
+		"start",
+	)
+
+	if err := cmd.Start(); err != nil {
+		log.Fatal(err)
+	}
+
+	if verbose {
+		fmt.Println("started Hermes")
+	}
+}
+
 func (tr TestRun) addIbcChannel(
 	action addIbcChannelAction,
 	verbose bool,

tests/integration/main.go

@@ -136,6 +136,8 @@ func (tr *TestRun) runStep(step Step, verbose bool) {
 		tr.assignConsumerPubKey(action, verbose)
 	case slashThrottleDequeue:
 		tr.waitForSlashThrottleDequeue(action, verbose)
+	case startHermesAction:
+		tr.startHermes(action, verbose)
 	default:
 		log.Fatalf("unknown action in testRun %s: %#v", tr.name, action)
 	}

tests/integration/steps.go

@@ -23,7 +23,9 @@ var happyPathSteps = concatSteps(
 	stepsRejectEquivocationProposal("consu", 2),   // prop to tombstone bob is rejected
 	stepsDoubleSignOnProviderAndConsumer("consu"), // carol double signs on provider, bob double signs on consumer
 	stepsSubmitEquivocationProposal("consu", 2),   // now prop to tombstone bob is submitted and accepted
-	stepsStopChain("consu", 3),
+	stepsStartHermes(),
+	stepsConsumerRemovalPropNotPassing("consu", 3), // submit removal prop but vote no on it - chain should stay
+	stepsStopChain("consu", 4),                     // stop chain
 )
 
 var slashThrottleSteps = concatSteps(

tests/integration/steps_stop_chain.go

@@ -2,6 +2,16 @@ package main
 
 import "time"
 
+// start hermes so that all messages are relayed
+func stepsStartHermes() []Step {
+	return []Step{
+		{
+			action: startHermesAction{},
+			state:  State{},
+		},
+	}
+}
+
 // submits a consumer-removal proposal and removes the chain
 func stepsStopChain(consumerName string, propNumber uint) []Step {
 	s := []Step{
@@ -58,3 +68,62 @@ func stepsStopChain(consumerName string, propNumber uint) []Step {
 
 	return s
 }
+
+// submits a consumer-removal proposal and votes no on it
+// the chain should not be removed
+func stepsConsumerRemovalPropNotPassing(consumerName string, propNumber uint) []Step {
+	s := []Step{
+
+		{
+			action: submitConsumerRemovalProposalAction{
+				chain:          chainID("provi"),
+				from:           validatorID("bob"),
+				deposit:        10000001,
+				consumerChain:  chainID(consumerName),
+				stopTimeOffset: 0 * time.Millisecond,
+			},
+			state: State{
+				chainID("provi"): ChainState{
+					ValBalances: &map[validatorID]uint{
+						validatorID("bob"): 9489999999,
+					},
+					Proposals: &map[uint]Proposal{
+						propNumber: ConsumerRemovalProposal{
+							Deposit:  10000001,
+							Chain:    chainID(consumerName),
+							StopTime: 0,
+							Status:   "PROPOSAL_STATUS_VOTING_PERIOD",
+						},
+					},
+					ConsumerChains: &map[chainID]bool{"consu": true}, // consumer chain not removed
+				},
+			},
+		},
+		{
+			action: voteGovProposalAction{
+				chain:      chainID("provi"),
+				from:       []validatorID{validatorID("alice"), validatorID("bob"), validatorID("carol")},
+				vote:       []string{"no", "no", "no"},
+				propNumber: propNumber,
+			},
+			state: State{
+				chainID("provi"): ChainState{
+					Proposals: &map[uint]Proposal{
+						propNumber: ConsumerRemovalProposal{
+							Deposit:  10000001,
+							Chain:    chainID(consumerName),
+							StopTime: 0,
+							Status:   "PROPOSAL_STATUS_REJECTED",
+						},
+					},
+					ValBalances: &map[validatorID]uint{
+						validatorID("bob"): 9500000000,
+					},
+					ConsumerChains: &map[chainID]bool{"consu": true}, // consumer chain not removed
+				},
+			},
+		},
+	}
+
+	return s
+}

x/ccv/provider/keeper/proposal.go

@@ -598,7 +598,7 @@ func (k Keeper) CreateConsumerClientInCachedCtx(ctx sdk.Context, p types.Consume
 // from a given consumer removal proposal in a cached context
 func (k Keeper) StopConsumerChainInCachedCtx(ctx sdk.Context, p types.ConsumerRemovalProposal) (cc sdk.Context, writeCache func(), err error) {
 	cc, writeCache = ctx.CacheContext()
-	err = k.StopConsumerChain(ctx, p.ChainId, true)
+	err = k.StopConsumerChain(cc, p.ChainId, true)
 	return
 }
 

x/ccv/provider/keeper/proposal_test.go

@@ -383,7 +383,7 @@ func TestHandleConsumerRemovalProposal(t *testing.T) {
 
 	type testCase struct {
 		description string
-		malleate    func(ctx sdk.Context, k providerkeeper.Keeper, chainID string)
+		setupMocks  func(ctx sdk.Context, k providerkeeper.Keeper, chainID string)
 
 		// Consumer removal proposal to handle
 		prop *providertypes.ConsumerRemovalProposal
@@ -392,16 +392,21 @@ func TestHandleConsumerRemovalProposal(t *testing.T) {
 		// Whether it's expected that the proposal is successfully verified
 		// and appended to the pending proposals
 		expAppendProp bool
+
+		// chainID of the consumer chain
+		// tests need to check that the CCV channel is not closed prematurely
+		chainId string
 	}
 
 	// Snapshot times asserted in tests
 	now := time.Now().UTC()
-	hourFromNow := now.Add(time.Hour).UTC()
+	hourAfterNow := now.Add(time.Hour).UTC()
+	hourBeforeNow := now.Add(-time.Hour).UTC()
 
 	tests := []testCase{
 		{
 			description: "valid proposal",
-			malleate: func(ctx sdk.Context, k providerkeeper.Keeper, chainID string) {
+			setupMocks: func(ctx sdk.Context, k providerkeeper.Keeper, chainID string) {
 				k.SetConsumerClientId(ctx, chainID, "ClientID")
 			},
 			prop: providertypes.NewConsumerRemovalProposal(
@@ -410,20 +415,52 @@ func TestHandleConsumerRemovalProposal(t *testing.T) {
 				"chainID",
 				now,
 			).(*providertypes.ConsumerRemovalProposal),
-			blockTime:     hourFromNow, // After stop time.
+			blockTime:     hourAfterNow, // After stop time.
 			expAppendProp: true,
+			chainId:       "chainID",
 		},
 		{
-			description: "invalid valid proposal: consumer chain does not exist",
-			malleate:    func(ctx sdk.Context, k providerkeeper.Keeper, chainID string) {},
+			description: "valid proposal - stop_time in the past",
+			setupMocks: func(ctx sdk.Context, k providerkeeper.Keeper, chainID string) {
+				k.SetConsumerClientId(ctx, chainID, "ClientID")
+			},
+			prop: providertypes.NewConsumerRemovalProposal(
+				"title",
+				"description",
+				"chainID",
+				hourBeforeNow,
+			).(*providertypes.ConsumerRemovalProposal),
+			blockTime:     hourAfterNow, // After stop time.
+			expAppendProp: true,
+			chainId:       "chainID",
+		},
+		{
+			description: "valid proposal - before stop_time in the future",
+			setupMocks: func(ctx sdk.Context, k providerkeeper.Keeper, chainID string) {
+				k.SetConsumerClientId(ctx, chainID, "ClientID")
+			},
+			prop: providertypes.NewConsumerRemovalProposal(
+				"title",
+				"description",
+				"chainID",
+				hourAfterNow,
+			).(*providertypes.ConsumerRemovalProposal),
+			blockTime:     now,
+			expAppendProp: true,
+			chainId:       "chainID",
+		},
+		{
+			description: "rejected valid proposal - consumer chain does not exist",
+			setupMocks:  func(ctx sdk.Context, k providerkeeper.Keeper, chainID string) {},
 			prop: providertypes.NewConsumerRemovalProposal(
 				"title",
 				"description",
 				"chainID-2",
-				hourFromNow,
+				hourAfterNow,
 			).(*providertypes.ConsumerRemovalProposal),
-			blockTime:     hourFromNow, // After stop time.
+			blockTime:     hourAfterNow, // After stop time.
 			expAppendProp: false,
+			chainId:       "chainID-2",
 		},
 	}
 
@@ -436,13 +473,13 @@ func TestHandleConsumerRemovalProposal(t *testing.T) {
 		ctx = ctx.WithBlockTime(tc.blockTime)
 
 		// Mock expectations and setup for stopping the consumer chain, if applicable
+		// Note: when expAppendProp is false, no mocks are setup,
+		// meaning no external keeper methods are allowed to be called.
 		if tc.expAppendProp {
 			testkeeper.SetupForStoppingConsumerChain(t, ctx, &providerKeeper, mocks)
 		}
-		// Note: when expAppendProp is false, no mocks are setup,
-		// meaning no external keeper methods are allowed to be called.
 
-		tc.malleate(ctx, providerKeeper, tc.prop.ChainId)
+		tc.setupMocks(ctx, providerKeeper, tc.prop.ChainId)
 
 		err := providerKeeper.HandleConsumerRemovalProposal(ctx, tc.prop)
 
@@ -453,6 +490,9 @@ func TestHandleConsumerRemovalProposal(t *testing.T) {
 			found := providerKeeper.PendingConsumerRemovalPropExists(ctx, tc.prop.ChainId, tc.prop.StopTime)
 			require.True(t, found)
 
+			// confirm that the channel was not closed
+			_, found = providerKeeper.GetChainToChannel(ctx, tc.chainId)
+			require.True(t, found)
 		} else {
 			require.Error(t, err)