From d341d82bb7ef44bf45c64751066806bace054add Mon Sep 17 00:00:00 2001 From: Matija Salopek Date: Fri, 24 Mar 2023 10:27:02 +0100 Subject: [PATCH 1/3] fix StopConsumerChain not running in cachedContext --- x/ccv/provider/keeper/proposal.go | 2 +- x/ccv/provider/keeper/proposal_test.go | 62 +++++++++++++++++++++----- 2 files changed, 52 insertions(+), 12 deletions(-) diff --git a/x/ccv/provider/keeper/proposal.go b/x/ccv/provider/keeper/proposal.go index c60051e372..16829710d6 100644 --- a/x/ccv/provider/keeper/proposal.go +++ b/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 } diff --git a/x/ccv/provider/keeper/proposal_test.go b/x/ccv/provider/keeper/proposal_test.go index 3c9dc27863..ed8f821bef 100644 --- a/x/ccv/provider/keeper/proposal_test.go +++ b/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) From fa4df87c23a459f88562a0b2a7244fe7594fff58 Mon Sep 17 00:00:00 2001 From: Matija Salopek Date: Fri, 24 Mar 2023 12:52:02 +0100 Subject: [PATCH 2/3] start hermes in docker tests --- tests/integration/actions.go | 24 +++++++++- tests/integration/main.go | 2 + tests/integration/steps.go | 9 ++-- tests/integration/steps_stop_chain.go | 64 +++++++++++++++++++++++++++ 4 files changed, 94 insertions(+), 5 deletions(-) diff --git a/tests/integration/actions.go b/tests/integration/actions.go index beec0b1f08..379488ab34 100644 --- a/tests/integration/actions.go +++ b/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, diff --git a/tests/integration/main.go b/tests/integration/main.go index b2bb34c535..d2b4266ddd 100644 --- a/tests/integration/main.go +++ b/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) } diff --git a/tests/integration/steps.go b/tests/integration/steps.go index de91a744f2..e49a290367 100644 --- a/tests/integration/steps.go +++ b/tests/integration/steps.go @@ -20,10 +20,11 @@ var happyPathSteps = concatSteps( stepsUnbond("consu"), stepsRedelegate("consu"), stepsDowntime("consu"), - 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), + 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 + stepsConsumerRemovalPropNotPassing("consu", 3), // submit removal prop but vote no on it - chain should stay + stepsStopChain("consu", 4), // stop chain ) var slashThrottleSteps = concatSteps( diff --git a/tests/integration/steps_stop_chain.go b/tests/integration/steps_stop_chain.go index e231592d76..0239a58222 100644 --- a/tests/integration/steps_stop_chain.go +++ b/tests/integration/steps_stop_chain.go @@ -5,6 +5,11 @@ import "time" // submits a consumer-removal proposal and removes the chain func stepsStopChain(consumerName string, propNumber uint) []Step { s := []Step{ + { + // start hermes so that all messages are relayed + action: startHermesAction{}, + state: State{}, + }, { action: submitConsumerRemovalProposalAction{ chain: chainID("provi"), @@ -58,3 +63,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 +} From e0aa05f5fe29effdd2bea21e567780ba99983106 Mon Sep 17 00:00:00 2001 From: Matija Salopek Date: Fri, 24 Mar 2023 14:34:31 +0100 Subject: [PATCH 3/3] fix some tests --- tests/integration/steps.go | 7 ++++--- tests/integration/steps_stop_chain.go | 13 +++++++++---- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/tests/integration/steps.go b/tests/integration/steps.go index e49a290367..448b6d3bce 100644 --- a/tests/integration/steps.go +++ b/tests/integration/steps.go @@ -20,9 +20,10 @@ var happyPathSteps = concatSteps( stepsUnbond("consu"), stepsRedelegate("consu"), stepsDowntime("consu"), - 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 + 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 + stepsStartHermes(), stepsConsumerRemovalPropNotPassing("consu", 3), // submit removal prop but vote no on it - chain should stay stepsStopChain("consu", 4), // stop chain ) diff --git a/tests/integration/steps_stop_chain.go b/tests/integration/steps_stop_chain.go index 0239a58222..5884afe9f8 100644 --- a/tests/integration/steps_stop_chain.go +++ b/tests/integration/steps_stop_chain.go @@ -2,14 +2,19 @@ package main import "time" -// submits a consumer-removal proposal and removes the chain -func stepsStopChain(consumerName string, propNumber uint) []Step { - s := []Step{ +// start hermes so that all messages are relayed +func stepsStartHermes() []Step { + return []Step{ { - // start hermes so that all messages are relayed action: startHermesAction{}, state: State{}, }, + } +} + +// submits a consumer-removal proposal and removes the chain +func stepsStopChain(consumerName string, propNumber uint) []Step { + s := []Step{ { action: submitConsumerRemovalProposalAction{ chain: chainID("provi"),