handle provider and consumer client expiration (#448)

* handle expired client when sending packets * add e2e test * add upgradeExpiredClient to e2e tests * improve incrementTime... functions * fix golangci-lint error * add client expired check * replace incrementTimeBy w/ incrementTime * replace AppendPendingVSC w/ AppendPendingVSCs * simplify logic of sendValidatorUpdates * separate PrepareIBCPacketSend from SendIBCPacket * error handling on SendPacket * export pending VSC packets * improve comments * use k.GetCCVTimeoutPeriod * remove GetUpgradeKeeper * AppendPendingVSCs: use variadic function * remove unnecessary if * refactor pending VSC CRUD methods * refactor sending valset updates to chains * add tests for VSC queueing * refactor after reviews * refactor after reviews * Merge marius/435-client-expired-consumer into marius/435-client-expired Squashed commit of the following: commit 3d82d19304a49938bfef573c99d2a77182167645 Author: mpoke <[email protected]> Date: Fri Nov 11 10:37:06 2022 +0100 fix typo commit 1efa9909162acb22a0e6d70e8da540ba437a3a59 Author: mpoke <[email protected]> Date: Wed Nov 9 19:07:30 2022 +0100 avoid trying to send on expired client commit a0cb6452776cdf68bf1f35ec5c069bdd76086991 Author: mpoke <[email protected]> Date: Wed Nov 9 15:56:59 2022 +0100 error handling on SendPacket commit 7c9c7629966a7d0b894fde3be9ad83f36afac97f Author: mpoke <[email protected]> Date: Wed Nov 9 13:55:05 2022 +0100 use PrepareIBCPacketSend pattern on consumer commit e7ff9d96fd325f851c1c1eb7dc1ed87c65274878 Author: mpoke <[email protected]> Date: Wed Nov 9 10:17:18 2022 +0100 update QA plan commit d7fafe8e9987f3b449e3cff07733f8316c484027 Author: mpoke <[email protected]> Date: Wed Nov 9 10:09:41 2022 +0100 add e2e test TestConsumerPacketSendExpiredClient commit 1722f1319edb44e3dd867329c6c6875436d9457e Author: mpoke <[email protected]> Date: Tue Nov 8 20:20:13 2022 +0100 remove SlashRequest from proto commit 241e13b2d9d47b641a9054973f4d109c78e68ec6 Author: mpoke <[email protected]> Date: Tue Nov 8 20:17:52 2022 +0100 remove pending slash requests from genesis commit 073f10160dd9a1d4cd857043e4dcff494230e489 Author: mpoke <[email protected]> Date: Tue Nov 8 19:44:46 2022 +0100 replace pending SlashRequests w/ peding DataPackets commit a2d1069459f99de0c3d2ce8d4794e51cff5cd8ed Author: mpoke <[email protected]> Date: Tue Nov 8 19:08:33 2022 +0100 code for pending data packets commit acc3454279052237054abab26bf20c7f5a42c386 Author: mpoke <[email protected]> Date: Mon Nov 7 21:03:03 2022 +0100 add e2e test commit 6170fa879745bb8f1e12f82b47deee13462f3c0e Author: mpoke <[email protected]> Date: Mon Nov 7 11:37:57 2022 +0100 handle expired client when sending packets * and packet queueing to consumer keeper * refactor e2e tests * refactor after review session * additional refactor after reviews Co-authored-by: Matija Salopek <[email protected]> Co-authored-by: Jehan <[email protected]>
cosmos · Nov 18, 2022 · f47bef7 · f47bef7
1 parent e706893
commit f47bef7
Show file tree

Hide file tree

Showing 36 changed files with 1,002 additions and 658 deletions.
diff --git a/app/consumer-democracy/app.go b/app/consumer-democracy/app.go
@@ -785,6 +785,11 @@ func (app *App) GetE2eEvidenceKeeper() e2e.E2eEvidenceKeeper {
 	return app.EvidenceKeeper
 }
 
+// GetUpgradeKeeper implements the ConsumerApp interface.
+func (app *App) GetUpgradeKeeper() upgradekeeper.Keeper {
+	return app.UpgradeKeeper
+}
+
 // GetE2eStakingKeeper implements the ConsumerApp interface.
 func (app *App) GetE2eStakingKeeper() e2e.E2eStakingKeeper {
 	return app.StakingKeeper

diff --git a/app/consumer/app.go b/app/consumer/app.go
@@ -679,6 +679,11 @@ func (app *App) GetE2eEvidenceKeeper() e2e.E2eEvidenceKeeper {
 	return app.EvidenceKeeper
 }
 
+// GetUpgradeKeeper implements the ConsumerApp interface.
+func (app *App) GetUpgradeKeeper() upgradekeeper.Keeper {
+	return app.UpgradeKeeper
+}
+
 // TestingApp functions
 
 // GetBaseApp implements the TestingApp interface.

diff --git a/docs/quality_assurance.md b/docs/quality_assurance.md
@@ -49,10 +49,10 @@ IBC packets:
 | -- | ------- | ----------- | ------------ | ----------- | ------------- | ------- |
 | 2.01 | Create IBC clients | `Scheduled` (ibc-go) | `Done` [TestCreateConsumerClient](../x/ccv/provider/keeper/proposal_test.go#117), [TestInitGenesis](../x/ccv/consumer/keeper/genesis_test.go#26) | `Done` [SetupTest](../tests/e2e/setup_test.go#39), [TestConsumerGenesis](../tests/e2e/channel_init_test.go#21) | `Future work` | `Scheduled` |
 | 2.02 | Create CCV channel (handshake) | `Scheduled` (ibc-go) | `Done` [provider/ibc_module_test.go](../x/ccv/provider/ibc_module_test.go), [consumer/ibc_module_test.go](../x/ccv/consumer/ibc_module_test.go) | `Done` [SetupCCVChannel](../tests/e2e/setup_test.go#125) | `Future work` | `Scheduled` |
-| 2.03 | Sending IBC packets <br> [SendIBCPacket](../x/ccv/utils/utils.go#40) | `Scheduled` (ibc-go) | `NA` | `Done` [TestSendVSCMaturedPackets](../tests/e2e/valset_update_test.go#39), [TestSendSlashPacket](../tests/e2e/slashing_test.go#648) | `Done` | `Scheduled` |
+| 2.03 | Sending IBC packets | `Scheduled` (ibc-go) | `NA` | `Done` [TestSendVSCMaturedPackets](../tests/e2e/valset_update_test.go#39), [TestSendSlashPacket](../tests/e2e/slashing_test.go#648) | `Done` | `Scheduled` |
 | 2.04 | Handling acknowledgments | `Scheduled` (ibc-go) | [Scheduled](https://github.com/cosmos/interchain-security/issues/362) | `Partial coverage` [TestOnAcknowledgementPacket](../x/ccv/consumer/keeper/relay_test.go#152), [TestSlashPacketAcknowldgement](../tests/e2e/slashing_test.go#258) | `Done` | `Scheduled` |
 | 2.05 | Handling timeouts | `Scheduled` (ibc-go) | [Scheduled](https://github.com/cosmos/interchain-security/issues/362) |`NA` | `Future work` | `Scheduled` |
-| 2.06 | Handling IBC client expiration <br> - high priority| `Scheduled` (ibc-go)  | `NA` | `NA` | `Future work` | `Scheduled` |
+| 2.06 | Handling IBC client expiration | `Scheduled` (ibc-go)  | `NA` | `Done` [expired_client.go](../tests/e2e/expired_client.go) | `Future work` | `Scheduled` |
 | 2.07 | ICS-20 channel creation | `Scheduled` (ibc-go) | `NA` | `Done` [SetupTransferChannel](../tests/e2e/setup_test.go#152) |`Future work` | `Scheduled` |
 | 2.08 | ICS-20 transfer | `Scheduled` (ibc-go) | `NA` | `Done` [TestRewardsDistribution](../tests/e2e/distribution_test.go#17) | `NA` | `Scheduled` |
 | 2.09 | Changes in IBC-GO testing suite | `Scheduled` (ibc-go) | `NA` | `NA` | `Partial coverage` | `NA` |
@@ -63,7 +63,7 @@ IBC packets:
 
 | ID | Concern | Code Review | Unit Testing | E2E Testing | Diff. Testing | Testnet |
 | -- | ------- | ----------- | ------------ | ----------- | ------------- | ------- |
-| 3.01 | Changes to staking module | `Done` | `Done` (Cosmos-SDK side) | `Partial coverage` <br> [unbonding_test.go](../tests/e2e/unbonding_test.go) <br> redelegation could be expanded, validator unbonding missing | `Partial coverage` | `Scheduled` |
+| 3.01 | Changes to staking module | `Done` | `Done` [unbonding_test.go](https://github.com/cosmos/cosmos-sdk/blob/interchain-security-rebase.0.45.6/x/staking/keeper/unbonding_test.go) | `Partial coverage` <br> [unbonding_test.go](../tests/e2e/unbonding_test.go) <br> redelegation could be expanded, validator unbonding missing | `Partial coverage` | `Scheduled` |
 | 3.02 | Changes to slashing module | `Done` | `NA` | `Done` <br> [TestValidatorDowntime](../tests/e2e/slashing_test.go#L502) <br>  | `Partial coverage` | `Scheduled` |
 | 3.03 | Changes to evidence module | `Done` | `NA` | `Done` <br> [TestValidatorDoubleSigning](../tests/e2e/slashing_test.go#L584) <br>  | `NA` | `Scheduled` |
 

diff --git a/proto/interchain_security/ccv/consumer/v1/consumer.proto b/proto/interchain_security/ccv/consumer/v1/consumer.proto
@@ -6,7 +6,6 @@ import "interchain_security/ccv/v1/ccv.proto";
 option go_package = "github.com/cosmos/interchain-security/x/ccv/consumer/types";
 
 import "google/protobuf/any.proto";
-import "cosmos/staking/v1beta1/staking.proto";
 import "gogoproto/gogo.proto";
 import "cosmos_proto/cosmos.proto";
 import "google/protobuf/duration.proto";
@@ -69,14 +68,26 @@ message CrossChainValidator {
   ];
 }
 
-// SlashRequest defines a slashing request for CCV consumer module
-message SlashRequest {
-    interchain_security.ccv.v1.SlashPacketData packet = 1;
-    cosmos.staking.v1beta1.InfractionType infraction = 2;
+// ConsumerPacketType indicates interchain security specific packet types.
+enum ConsumerPacketType {
+  option (gogoproto.goproto_enum_prefix) = false;
+
+  // UNSPECIFIED packet type
+  CONSUMER_PACKET_TYPE_UNSPECIFIED = 0 [(gogoproto.enumvalue_customname) = "UnspecifiedPacket"];
+  // Slash packet
+  CONSUMER_PACKET_TYPE_SLASH = 1 [(gogoproto.enumvalue_customname) = "SlashPacket"];
+  // VSCMatured packet
+  CONSUMER_PACKET_TYPE_VSCM = 2 [(gogoproto.enumvalue_customname) = "VscMaturedPacket"];
+}
+
+// ConsumerPacket contains raw packet bytes and packet type.
+message ConsumerPacket {
+  ConsumerPacketType type = 1; 
+  bytes data = 2;
 }
 
-// SlashRequests is a list of slash requests for CCV consumer module
-message SlashRequests {
-  repeated SlashRequest requests = 1
+// ConsumerPackets is a list of data packets.
+message ConsumerPackets {
+  repeated ConsumerPacket list = 1
   [ (gogoproto.nullable) = false ];
 }
diff --git a/proto/interchain_security/ccv/consumer/v1/genesis.proto b/proto/interchain_security/ccv/consumer/v1/genesis.proto
@@ -32,9 +32,6 @@ message GenesisState {
   // OutstandingDowntimes nil on new chain, filled on restart.
   repeated OutstandingDowntime outstanding_downtime_slashing = 10
       [ (gogoproto.nullable) = false ];
-  // PendingSlashRequests filled in on new chain, nil on restart.
- interchain_security.ccv.consumer.v1.SlashRequests pending_slash_requests = 11
- [ (gogoproto.nullable) = false ];
 }
 
 // MaturingVSCPacket defines the genesis information for the

diff --git a/tests/difference/core/driver/core_test.go b/tests/difference/core/driver/core_test.go
@@ -381,8 +381,8 @@ func (s *CoreSuite) TestAssumptions() {
 		s.T().Fatal(FAIL_MSG)
 	}
 
-	// Consumer has no slash requests
-	s.Require().Empty(s.consumerKeeper().GetPendingSlashRequests(s.ctx(C)))
+	// Consumer has no pending data packets
+	s.Require().Empty(s.consumerKeeper().GetPendingPackets(s.ctx(C)))
 
 	// Consumer has no maturities
 	s.consumerKeeper().IteratePacketMaturityTime(s.ctx(C),

diff --git a/tests/difference/core/driver/setup.go b/tests/difference/core/driver/setup.go
@@ -481,7 +481,7 @@ func (b *Builder) createConsumerGenesis(tmConfig *ibctesting.TendermintConfig) *
 		consumertypes.DefaultHistoricalEntries,
 		consumertypes.DefaultConsumerUnbondingPeriod,
 	)
-	return consumertypes.NewInitialGenesisState(providerClient, providerConsState, valUpdates, consumertypes.SlashRequests{}, params)
+	return consumertypes.NewInitialGenesisState(providerClient, providerConsState, valUpdates, params)
 }
 
 func (b *Builder) createLink() {

diff --git a/tests/e2e/README.md b/tests/e2e/README.md
@@ -12,6 +12,7 @@ E2e tests are categorized into files as follows:
 - `distribution.go` - e2e tests for the _Reward Distribution_ sub-protocol
 - `stop_consumer.go` - e2e tests for the _Consumer Chain Removal_ sub-protocol
 - `normal_operations.go` - e2e tests for _normal operations_ of ICS enabled chains
+- `expired_client.go` - e2e tests for testing expired clients
 - `instance_test.go` - ties the e2e test structure into golang's standard test mechanism, with appropriate definitions for concrete app types and setup callback
 
 To run the e2e tests defined in this repo on any arbitrary consumer and provider implementation, copy the pattern exemplified in `instance_test.go` and `specific_setup.go`
diff --git a/tests/e2e/channel_init.go b/tests/e2e/channel_init.go
@@ -166,7 +166,7 @@ func (suite *CCVTestSuite) TestInitTimeout() {
 		suite.providerChain.NextBlock()
 
 		// increment time
-		incrementTimeBy(suite, initTimeout)
+		incrementTime(suite, initTimeout)
 
 		// check whether the chain was removed
 		_, found = providerKeeper.GetConsumerClientId(suite.providerCtx(), chainID)

diff --git a/tests/e2e/common.go b/tests/e2e/common.go
@@ -7,7 +7,6 @@ import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
 	"github.com/cosmos/interchain-security/testutil/e2e"
-	consumertypes "github.com/cosmos/interchain-security/x/ccv/consumer/types"
 	providertypes "github.com/cosmos/interchain-security/x/ccv/provider/types"
 	ccv "github.com/cosmos/interchain-security/x/ccv/types"
 	"github.com/stretchr/testify/require"
@@ -17,6 +16,7 @@ import (
 	channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types"
 	commitmenttypes "github.com/cosmos/ibc-go/v3/modules/core/23-commitment/types"
 	"github.com/cosmos/ibc-go/v3/modules/core/exported"
+	ibctm "github.com/cosmos/ibc-go/v3/modules/light-clients/07-tendermint/types"
 	ibctmtypes "github.com/cosmos/ibc-go/v3/modules/light-clients/07-tendermint/types"
 	ibctesting "github.com/cosmos/ibc-go/v3/testing"
 )
@@ -212,29 +212,16 @@ func relayAllCommittedPackets(
 // Note that it is expected for the provider unbonding period
 // to be one day larger than the consumer unbonding period.
 func incrementTimeByUnbondingPeriod(s *CCVTestSuite, chainType ChainType) {
-	// Get unboding period from staking keeper
+	// Get unboding periods
 	providerUnbondingPeriod := s.providerApp.GetStakingKeeper().UnbondingTime(s.providerCtx())
 	consumerUnbondingPeriod := s.consumerApp.GetConsumerKeeper().GetUnbondingPeriod(s.consumerCtx())
-	// Note: the assertions below are not strictly necessary, and rely on default values
-	s.Require().Equal(consumertypes.DefaultConsumerUnbondingPeriod+24*time.Hour, providerUnbondingPeriod, "unexpected provider unbonding period")
-	s.Require().Equal(consumertypes.DefaultConsumerUnbondingPeriod, consumerUnbondingPeriod, "unexpected consumer unbonding period")
 	var jumpPeriod time.Duration
 	if chainType == Provider {
 		jumpPeriod = providerUnbondingPeriod
 	} else {
 		jumpPeriod = consumerUnbondingPeriod
 	}
-	// Make sure the clients do not expire
-	jumpPeriod = jumpPeriod/4 + time.Hour
-	for i := 0; i < 4; i++ {
-		s.coordinator.IncrementTimeBy(jumpPeriod)
-		// Update the provider client on the consumer
-		err := s.path.EndpointA.UpdateClient()
-		s.Require().NoError(err)
-		// Update the consumer client on the provider
-		err = s.path.EndpointB.UpdateClient()
-		s.Require().NoError(err)
-	}
+	incrementTime(s, jumpPeriod)
 }
 
 func checkStakingUnbondingOps(s *CCVTestSuite, id uint64, found bool, onHold bool, msgAndArgs ...interface{}) {
@@ -350,25 +337,74 @@ func (suite *CCVTestSuite) commitSlashPacket(ctx sdk.Context, packetData ccv.Sla
 	return channeltypes.CommitPacket(suite.consumerChain.App.AppCodec(), packet)
 }
 
-// incrementTimeBy increments the overall time by jumpPeriod
-func incrementTimeBy(s *CCVTestSuite, jumpPeriod time.Duration) {
-	// Get unboding period from staking keeper
-	consumerUnbondingPeriod := s.consumerApp.GetConsumerKeeper().GetUnbondingPeriod(s.consumerChain.GetContext())
-	split := 1
-	trustingPeriodFraction := s.providerApp.GetProviderKeeper().GetTrustingPeriodFraction(s.providerCtx())
-	if jumpPeriod > consumerUnbondingPeriod/time.Duration(trustingPeriodFraction) {
-		// Make sure the clients do not expire
-		split = 4
-		jumpPeriod = jumpPeriod / 4
+// incrementTime increments the overall time by jumpPeriod
+// while updating to not expire the clients
+func incrementTime(s *CCVTestSuite, jumpPeriod time.Duration) {
+	// get trusting period of client on provider endpoint
+	cs, ok := s.providerApp.GetIBCKeeper().ClientKeeper.GetClientState(s.providerCtx(), s.path.EndpointB.ClientID)
+	s.Require().True(ok)
+	providerEndpointTP := cs.(*ibctm.ClientState).TrustingPeriod
+	// get trusting period of client on consumer endpoint
+	cs, ok = s.consumerApp.GetIBCKeeper().ClientKeeper.GetClientState(s.consumerCtx(), s.path.EndpointA.ClientID)
+	s.Require().True(ok)
+	consumerEndpointTP := cs.(*ibctm.ClientState).TrustingPeriod
+	// find the minimum trusting period
+	var minTP time.Duration
+	if providerEndpointTP < consumerEndpointTP {
+		minTP = providerEndpointTP
+	} else {
+		minTP = consumerEndpointTP
 	}
-	for i := 0; i < split; i++ {
-		s.coordinator.IncrementTimeBy(jumpPeriod)
-		// Update the provider client on the consumer
+	// jumpStep is the maximum interval at which both clients are updated
+	jumpStep := minTP / 2
+	for jumpPeriod > 0 {
+		var step time.Duration
+		if jumpPeriod < jumpStep {
+			step = jumpPeriod
+		} else {
+			step = jumpStep
+		}
+		s.coordinator.IncrementTimeBy(step)
+		// update the provider client on the consumer
 		err := s.path.EndpointA.UpdateClient()
 		s.Require().NoError(err)
-		// Update the consumer client on the provider
+		// update the consumer client on the provider
 		err = s.path.EndpointB.UpdateClient()
 		s.Require().NoError(err)
+		jumpPeriod -= step
+	}
+}
+
+// incrementTimeWithoutUpdate increments the overall time by jumpPeriod
+// without updating the client to the `noUpdate` chain
+func incrementTimeWithoutUpdate(s *CCVTestSuite, jumpPeriod time.Duration, noUpdate ChainType) {
+	var trustingPeriod time.Duration
+	var endpointToUpdate *ibctesting.Endpoint
+	if noUpdate == Consumer {
+		cs, ok := s.consumerApp.GetIBCKeeper().ClientKeeper.GetClientState(s.consumerCtx(), s.path.EndpointA.ClientID)
+		s.Require().True(ok)
+		trustingPeriod = cs.(*ibctm.ClientState).TrustingPeriod
+		endpointToUpdate = s.path.EndpointA
+	} else {
+		cs, ok := s.providerApp.GetIBCKeeper().ClientKeeper.GetClientState(s.providerCtx(), s.path.EndpointB.ClientID)
+		s.Require().True(ok)
+		trustingPeriod = cs.(*ibctm.ClientState).TrustingPeriod
+		endpointToUpdate = s.path.EndpointB
+	}
+	// jumpStep is the maximum interval at which the client on endpointToUpdate is updated
+	jumpStep := trustingPeriod / 2
+	for jumpPeriod > 0 {
+		var step time.Duration
+		if jumpPeriod < jumpStep {
+			step = jumpPeriod
+		} else {
+			step = jumpStep
+		}
+		s.coordinator.IncrementTimeBy(step)
+		// update the client
+		err := endpointToUpdate.UpdateClient()
+		s.Require().NoError(err)
+		jumpPeriod -= step
 	}
 }