Channel initialization timeout (#406)

* add provider-based timeout params

* add InitTimeoutTimestamp to store

* add init timeout logic

* params boilerplate code & making tests pass

* add TestInitTimeout* e2e tests

* improve e2e tests; add test case to TestUndelegationDuringInit

* remove VSC timeout

* remove VSC timeout param

* add testcase to TestValidateParams

* handle StopConsumerChain error & gofmt

* Fix init timeout conflicts (#409)

* Importable e2e tests (#401)

* fixes

* add comment to GetInitTimeoutTimestamp

* Update proto/interchain_security/ccv/provider/v1/provider.proto

Co-authored-by: Aditya <adityasripal@gmail.com>

* fix formatting in proto file

* add comment to SetConsumerChain

* fix typo

* add comment re. EndBlock order

* change name of testcase in TestUndelegationDuringInit

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

app/consumer-democracy/export.go

@@ -49,7 +49,8 @@ func (app *App) ExportAppStateAndValidators(
 
 // prepare for fresh start at zero height
 // NOTE zero height genesis is a temporary feature which will be deprecated
-//      in favour of export at a block height
+//
+//	in favour of export at a block height
 func (app *App) prepForZeroHeightGenesis(ctx sdk.Context, jailAllowedAddrs []string) {
 	// applyAllowedAddrs := false
 

app/provider/export.go

@@ -50,7 +50,8 @@ func (app *App) ExportAppStateAndValidators(
 
 // prepare for fresh start at zero height
 // NOTE zero height genesis is a temporary feature which will be deprecated
-//      in favour of export at a block height
+//
+//	in favour of export at a block height
 func (app *App) prepForZeroHeightGenesis(ctx sdk.Context, jailAllowedAddrs []string) {
 	applyAllowedAddrs := false
 

docs/quality_assurance.md

@@ -80,7 +80,7 @@ The main concern addressed in this section is the correctness of the provider ch
 | 4.01 | Liveness of undelegations <br> - unbonding delegation entries are eventually removed from `UnbondingDelegation` | `Scheduled` | `NA` | `Done` <br> [unbonding_test.go](../tests/e2e/unbonding_test.go) | `Done` | `Scheduled` | `NA` |
 | 4.02 | Liveness of redelegations <br> - redelegations entries are eventually removed from `Redelegations` | `Scheduled` | `NA` | `Scheduled` | `Scheduled` | `Scheduled` | `NA` |
 | 4.03 | Liveness of validator unbondings <br> - unbonding validators with no delegations are eventually removed from `Validators` | `Scheduled` | `NA` | `NA` | `Done` | `Scheduled` | `NA` |
-| 4.04 | Unbonding operations (undelegations, redelegations, validator unbondings) should eventually complete even if the CCV channel is never established (due to error) <br> - expected outcome: the channel initialization sub-protocol eventually times out, which leads to the consumer chain removal <br> - requires https://github.com/cosmos/interchain-security/issues/278 | `Scheduled` | `NA` | `Scheduled` | `Future work` | `Scheduled` | `Done` |
+| 4.04 | Unbonding operations (undelegations, redelegations, validator unbondings) should eventually complete even if the CCV channel is never established (due to error) <br> - expected outcome: the channel initialization sub-protocol eventually times out, which leads to the consumer chain removal | `Scheduled` | `NA` | `Done` [TestUndelegationDuringInit](../tests/e2e/unbonding_test.go#145) | `Future work` | `Scheduled` | `Done` |
 | 4.05 | Unbonding operations (undelegations, redelegations, validator unbondings) should eventually complete even if one of the clients expire <br> - expected outcome: the pending VSC packets eventually timeout, which leads to the consumer chain removal <br> - requires https://github.com/cosmos/interchain-security/issues/283 | `Scheduled` | `NA` | `Scheduled` | `Future work` | `Scheduled` | `NA` |
 | 4.06 | A validator cannot get slashed more than once for double signing, regardless of how many times it double signs on different chains (consumers or provider) | `Scheduled` | `NA` |`Done` <br> [TestHandleSlashPacketErrors](../tests/e2e/slashing_test.go#L317) | `Done` | `Scheduled` | `NA` |
 | 4.07 | A validator cannot get slashed multiple times for downtime on the same consumer chain without requesting to `Unjail` itself on the provider chain in between | `Scheduled` | `NA` | `Partial coverage` <br> [TestSendSlashPacket](../tests/e2e/slashing_test.go#L648) | `Partial coverage` | `Scheduled` | `NA` |

go.mod

@@ -4,7 +4,6 @@ go 1.18
 
 require (
 	github.com/cosmos/cosmos-sdk v0.45.2-0.20220901181011-06d4a64bf808
-	github.com/cosmos/ibc-go v1.2.2
 	github.com/cosmos/ibc-go/v3 v3.0.0-alpha1.0.20220210141024-fb2f0416254b
 	github.com/gogo/protobuf v1.3.3
 	github.com/golang/protobuf v1.5.2

go.sum

@@ -243,7 +243,6 @@ github.com/cosmos/iavl v0.15.3/go.mod h1:OLjQiAQ4fGD2KDZooyJG9yz+p2ao2IAYSbke8mV
 github.com/cosmos/iavl v0.17.1/go.mod h1:7aisPZK8yCpQdy3PMvKeO+bhq1NwDjUwjzxwwROUxFk=
 github.com/cosmos/iavl v0.17.3 h1:s2N819a2olOmiauVa0WAhoIJq9EhSXE9HDBAoR9k+8Y=
 github.com/cosmos/iavl v0.17.3/go.mod h1:prJoErZFABYZGDHka1R6Oay4z9PrNeFFiMKHDAMOi4w=
-github.com/cosmos/ibc-go v1.2.2 h1:bs6TZ8Es1kycIu2AHlRZ9dzJ+mveqlLN/0sjWtRH88o=
 github.com/cosmos/ibc-go v1.2.2/go.mod h1:XmYjsRFOs6Q9Cz+CSsX21icNoH27vQKb3squgnCOCbs=
 github.com/cosmos/keyring v1.1.7-0.20210622111912-ef00f8ac3d76 h1:DdzS1m6o/pCqeZ8VOAit/gyATedRgjvkVI+UCrLpyuU=
 github.com/cosmos/keyring v1.1.7-0.20210622111912-ef00f8ac3d76/go.mod h1:0mkLWIoZuQ7uBoospo5Q9zIpqq6rYCPJDSUdeCJvPM8=

proto/interchain_security/ccv/provider/v1/provider.proto

@@ -58,11 +58,14 @@ message ConsumerAdditionProposal {
 // Params defines the parameters for CCV Provider module
 message Params {
   ibc.lightclients.tendermint.v1.ClientState template_client = 1;
+  // TrustingPeriodFraction is used to compute the consumer and provider IBC client's TrustingPeriod from the chain defined UnbondingPeriod
+  int64 trusting_period_fraction = 2;
   // Sent IBC packets will timeout after this duration
-  google.protobuf.Duration ccv_timeout_period = 2
+  google.protobuf.Duration ccv_timeout_period = 3
       [(gogoproto.nullable) = false, (gogoproto.stdduration) = true];
-  // TrustingPeriodFraction is used to compute the consumer and provider IBC client's TrustingPeriod
-  int64 trusting_period_fraction = 3;
+  // The channel initialization (IBC channel opening handshake) will timeout after this duration
+  google.protobuf.Duration init_timeout_period = 4
+      [(gogoproto.nullable) = false, (gogoproto.stdduration) = true]; 
 }
 
 message HandshakeMetadata {

tests/e2e/channel_init.go

@@ -93,3 +93,100 @@ func (suite *CCVTestSuite) TestProviderClientMatches() {
 	clientState, _ := suite.consumerApp.GetIBCKeeper().ClientKeeper.GetClientState(suite.consumerCtx(), providerClientID)
 	suite.Require().Equal(suite.providerClient, clientState, "stored client state does not match genesis provider client")
 }
+
+// TestInitTimeout tests the init timeout
+func (suite *CCVTestSuite) TestInitTimeout() {
+	testCases := []struct {
+		name      string
+		handshake func()
+		removed   bool
+	}{
+		{
+			"init times out before INIT", func() {}, true,
+		},
+		{
+			"init times out before TRY", func() {
+				// send ChanOpenInit
+				err := suite.path.EndpointA.ChanOpenInit()
+				suite.Require().NoError(err)
+			}, true,
+		},
+		{
+			"init times out before ACK", func() {
+				// send ChanOpenInit
+				err := suite.path.EndpointA.ChanOpenInit()
+				suite.Require().NoError(err)
+				// send ChanOpenTry
+				err = suite.path.EndpointB.ChanOpenTry()
+				suite.Require().NoError(err)
+			}, true,
+		},
+		{
+			"init times out before CONFIRM", func() {
+				// send ChanOpenInit
+				err := suite.path.EndpointA.ChanOpenInit()
+				suite.Require().NoError(err)
+				// send ChanOpenTry
+				err = suite.path.EndpointB.ChanOpenTry()
+				suite.Require().NoError(err)
+				// send ChanOpenAck
+				err = suite.path.EndpointA.ChanOpenAck()
+				suite.Require().NoError(err)
+			}, true,
+		},
+		{
+			"init completes before timeout", func() {
+				// send ChanOpenInit
+				err := suite.path.EndpointA.ChanOpenInit()
+				suite.Require().NoError(err)
+				// send ChanOpenTry
+				err = suite.path.EndpointB.ChanOpenTry()
+				suite.Require().NoError(err)
+				// send ChanOpenAck
+				err = suite.path.EndpointA.ChanOpenAck()
+				suite.Require().NoError(err)
+				// send ChanOpenConfirm
+				err = suite.path.EndpointB.ChanOpenConfirm()
+				suite.Require().NoError(err)
+			}, false,
+		},
+	}
+
+	for i, tc := range testCases {
+		providerKeeper := suite.providerApp.GetProviderKeeper()
+		initTimeout := providerKeeper.GetParams(suite.providerCtx()).InitTimeoutPeriod
+		chainID := suite.consumerChain.ChainID
+
+		// get init timeout timestamp
+		ts, found := providerKeeper.GetInitTimeoutTimestamp(suite.providerCtx(), chainID)
+		suite.Require().True(found, "cannot find init timeout timestamp; test: %s", tc.name)
+		expectedTs := suite.providerCtx().BlockTime().Add(initTimeout)
+		suite.Require().Equal(uint64(expectedTs.UnixNano()), ts, "unexpected init timeout timestamp; test: %s", tc.name)
+
+		// create connection
+		suite.coordinator.CreateConnections(suite.path)
+
+		// channel opening handshake
+		tc.handshake()
+
+		// call NextBlock
+		suite.providerChain.NextBlock()
+
+		// increment time
+		incrementTimeBy(suite, initTimeout)
+
+		// check whether the chain was removed
+		_, found = providerKeeper.GetConsumerClientId(suite.providerCtx(), chainID)
+		suite.Require().Equal(!tc.removed, found, "unexpected outcome; test: %s", tc.name)
+
+		if tc.removed {
+			// check if the chain was properly removed
+			suite.checkConsumerChainIsRemoved(chainID, false, false)
+		}
+
+		if i+1 < len(testCases) {
+			// reset suite to reset provider client
+			suite.SetupTest()
+		}
+	}
+}

tests/e2e/common.go

@@ -1,23 +1,22 @@
 package e2e
 
 import (
-	"strings"
+	"fmt"
 	"time"
 
 	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"
 	abci "github.com/tendermint/tendermint/abci/types"
 
-	"github.com/cosmos/ibc-go/modules/core/exported"
 	clienttypes "github.com/cosmos/ibc-go/v3/modules/core/02-client/types"
 	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"
 	ibctmtypes "github.com/cosmos/ibc-go/v3/modules/light-clients/07-tendermint/types"
 	ibctesting "github.com/cosmos/ibc-go/v3/testing"
 )
@@ -207,8 +206,8 @@ func relayAllCommittedPackets(
 }
 
 // incrementTimeByUnbondingPeriod increments the overall time by
-//  - if chainType == Provider, the unbonding period on the provider.
-//  - otherwise, the unbonding period on the consumer.
+//   - if chainType == Provider, the unbonding period on the provider.
+//   - otherwise, the unbonding period on the consumer.
 //
 // Note that it is expected for the provider unbonding period
 // to be one day larger than the consumer unbonding period.
@@ -238,19 +237,44 @@ func incrementTimeByUnbondingPeriod(s *CCVTestSuite, chainType ChainType) {
 	}
 }
 
-func checkStakingUnbondingOps(s *CCVTestSuite, id uint64, found bool, onHold bool) {
+func checkStakingUnbondingOps(s *CCVTestSuite, id uint64, found bool, onHold bool, msgAndArgs ...interface{}) {
 	stakingUnbondingOp, wasFound := getStakingUnbondingDelegationEntry(s.providerCtx(), s.providerApp.GetE2eStakingKeeper(), id)
-	s.Require().True(found == wasFound)
-	s.Require().True(onHold == (0 < stakingUnbondingOp.UnbondingOnHoldRefCount))
+	s.Require().Equal(
+		found,
+		wasFound,
+		fmt.Sprintf("checkStakingUnbondingOps failed - getStakingUnbondingDelegationEntry; %s", msgAndArgs...),
+	)
+	if wasFound {
+		s.Require().True(
+			onHold == (0 < stakingUnbondingOp.UnbondingOnHoldRefCount),
+			fmt.Sprintf("checkStakingUnbondingOps failed - onHold; %s", msgAndArgs...),
+		)
+	}
 }
 
-func checkCCVUnbondingOp(s *CCVTestSuite, providerCtx sdk.Context, chainID string, valUpdateID uint64, found bool) {
+func checkCCVUnbondingOp(s *CCVTestSuite, providerCtx sdk.Context, chainID string, valUpdateID uint64, found bool, msgAndArgs ...interface{}) {
 	entries, wasFound := s.providerApp.GetProviderKeeper().GetUnbondingOpsFromIndex(providerCtx, chainID, valUpdateID)
-	s.Require().True(found == wasFound)
+	s.Require().Equal(
+		found,
+		wasFound,
+		fmt.Sprintf("checkCCVUnbondingOp failed - GetUnbondingOpsFromIndex; %s", msgAndArgs...),
+	)
 	if found {
-		s.Require().True(len(entries) > 0, "No unbonding ops found")
-		s.Require().True(len(entries[0].UnbondingConsumerChains) > 0, "Unbonding op with no consumer chains")
-		s.Require().True(strings.Compare(entries[0].UnbondingConsumerChains[0], "testchain2") == 0, "Unbonding op with unexpected consumer chain")
+		s.Require().Greater(
+			len(entries),
+			0,
+			fmt.Sprintf("checkCCVUnbondingOp failed - no unbonding ops found; %s", msgAndArgs...),
+		)
+		s.Require().Greater(
+			len(entries[0].UnbondingConsumerChains),
+			0,
+			fmt.Sprintf("checkCCVUnbondingOp failed - unbonding op with no consumer chains; %s", msgAndArgs...),
+		)
+		s.Require().Equal(
+			"testchain2",
+			entries[0].UnbondingConsumerChains[0],
+			fmt.Sprintf("checkCCVUnbondingOp failed - unbonding op with unexpected consumer chain; %s", msgAndArgs...),
+		)
 	}
 }
 

tests/e2e/distribution.go

@@ -10,7 +10,7 @@ import (
 	ccv "github.com/cosmos/interchain-security/x/ccv/types"
 )
 
-//This test is valid for minimal viable consumer chain
+// This test is valid for minimal viable consumer chain
 func (s *CCVTestSuite) TestRewardsDistribution() {
 
 	//set up channel and delegate some tokens in order for validator set update to be sent to the consumer chain

tests/e2e/stop_consumer.go

@@ -93,7 +93,7 @@ func (s *CCVTestSuite) TestStopConsumerChain() {
 	s.Require().NoError(err)
 
 	// check all states are removed and the unbonding operation released
-	s.checkConsumerChainIsRemoved(consumerChainID, false)
+	s.checkConsumerChainIsRemoved(consumerChainID, false, true)
 }
 
 // TODO Simon: implement OnChanCloseConfirm in IBC-GO testing to close the consumer chain's channel end
@@ -127,13 +127,15 @@ func (s *CCVTestSuite) TestStopConsumerOnChannelClosed() {
 	// s.Require().False(found)
 }
 
-func (s *CCVTestSuite) checkConsumerChainIsRemoved(chainID string, lockUbd bool) {
+func (s *CCVTestSuite) checkConsumerChainIsRemoved(chainID string, lockUbd bool, checkChannel bool) {
 	channelID := s.path.EndpointB.ChannelID
 	providerKeeper := s.providerApp.GetProviderKeeper()
 	providerStakingKeeper := s.providerApp.GetE2eStakingKeeper()
 
-	// check channel's state is closed
-	s.Require().Equal(channeltypes.CLOSED, s.path.EndpointB.GetChannel().State)
+	if checkChannel {
+		// check channel's state is closed
+		s.Require().Equal(channeltypes.CLOSED, s.path.EndpointB.GetChannel().State)
+	}
 
 	// check UnbondingOps were deleted and undelegation entries aren't onHold
 	if !lockUbd {