From c032e9d58fc4476d859d500679418b4d88017975 Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Tue, 18 Apr 2023 14:56:43 +0300 Subject: [PATCH 1/3] imp: add a check for the client status in CreateClient --- modules/core/02-client/keeper/client.go | 4 ++ modules/core/02-client/keeper/client_test.go | 68 ++++++++++++++------ 2 files changed, 52 insertions(+), 20 deletions(-) diff --git a/modules/core/02-client/keeper/client.go b/modules/core/02-client/keeper/client.go index 4935cc13299..1be50fdb820 100644 --- a/modules/core/02-client/keeper/client.go +++ b/modules/core/02-client/keeper/client.go @@ -35,6 +35,10 @@ func (k Keeper) CreateClient( return "", err } + if status := k.GetClientStatus(ctx, clientState, clientID); status != exported.Active { + return "", errorsmod.Wrapf(types.ErrClientNotActive, "cannot create client (%s) with status %s", clientID, status) + } + k.Logger(ctx).Info("client created at height", "client-id", clientID, "height", clientState.GetLatestHeight().String()) defer telemetry.IncrCounterWithLabels( diff --git a/modules/core/02-client/keeper/client_test.go b/modules/core/02-client/keeper/client_test.go index ce338d25853..f23bf2ca57d 100644 --- a/modules/core/02-client/keeper/client_test.go +++ b/modules/core/02-client/keeper/client_test.go @@ -18,43 +18,71 @@ import ( ) func (suite *KeeperTestSuite) TestCreateClient() { - cases := []struct { - msg string + var ( clientState exported.ClientState consensusState exported.ConsensusState - expPass bool + ) + + testCases := []struct { + msg string + malleate func() + expPass bool }{ { "success: 07-tendermint client type supported", - ibctm.NewClientState(testChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath), - suite.consensusState, + func() { + clientState = ibctm.NewClientState(testChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath) + consensusState = suite.consensusState + }, true, }, + { + "failure: 07-tendermint client status is not active", + func() { + clientState = ibctm.NewClientState(testChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath) + tmcs, ok := clientState.(*ibctm.ClientState) + suite.Require().True(ok) + tmcs.FrozenHeight = clienttypes.NewHeight(0, 1) + consensusState = suite.consensusState + }, + false, + }, { "success: 06-solomachine client type supported", - solomachine.NewClientState(0, &solomachine.ConsensusState{PublicKey: suite.solomachine.ConsensusState().PublicKey, Diversifier: suite.solomachine.Diversifier, Timestamp: suite.solomachine.Time}), - &solomachine.ConsensusState{PublicKey: suite.solomachine.ConsensusState().PublicKey, Diversifier: suite.solomachine.Diversifier, Timestamp: suite.solomachine.Time}, + func() { + clientState = solomachine.NewClientState(0, &solomachine.ConsensusState{PublicKey: suite.solomachine.ConsensusState().PublicKey, Diversifier: suite.solomachine.Diversifier, Timestamp: suite.solomachine.Time}) + consensusState = &solomachine.ConsensusState{PublicKey: suite.solomachine.ConsensusState().PublicKey, Diversifier: suite.solomachine.Diversifier, Timestamp: suite.solomachine.Time} + }, true, }, { "failure: 09-localhost client type not supported", - localhost.NewClientState(clienttypes.GetSelfHeight(suite.ctx)), - nil, + func() { + clientState = localhost.NewClientState(clienttypes.GetSelfHeight(suite.ctx)) + consensusState = nil + }, false, }, } - for i, tc := range cases { - clientID, err := suite.keeper.CreateClient(suite.ctx, tc.clientState, tc.consensusState) - if tc.expPass { - suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.msg) - suite.Require().NotNil(clientID, "valid test case %d failed: %s", i, tc.msg) - suite.Require().True(suite.keeper.ClientStore(suite.ctx, clientID).Has(host.ClientStateKey())) - } else { - suite.Require().Error(err, "invalid test case %d passed: %s", i, tc.msg) - suite.Require().Equal("", clientID, "invalid test case %d passed: %s", i, tc.msg) - suite.Require().False(suite.keeper.ClientStore(suite.ctx, clientID).Has(host.ClientStateKey())) - } + for i, tc := range testCases { + tc := tc + + suite.Run(fmt.Sprintf("Case %s", tc.msg), func() { + suite.SetupTest() // reset + tc.malleate() + + clientID, err := suite.keeper.CreateClient(suite.ctx, clientState, consensusState) + if tc.expPass { + suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.msg) + suite.Require().NotNil(clientID, "valid test case %d failed: %s", i, tc.msg) + suite.Require().True(suite.keeper.ClientStore(suite.ctx, clientID).Has(host.ClientStateKey())) + } else { + suite.Require().Error(err, "invalid test case %d passed: %s", i, tc.msg) + suite.Require().Equal("", clientID, "invalid test case %d passed: %s", i, tc.msg) + suite.Require().False(suite.keeper.ClientStore(suite.ctx, clientID).Has(host.ClientStateKey())) + } + }) } } From 84ca3ffc70bab9e9ec0c9bdd0fedbac9573cbb4e Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Wed, 26 Apr 2023 23:01:31 +0300 Subject: [PATCH 2/3] bump bad trusting period to ten seconds, address review comments. --- e2e/tests/core/client_test.go | 2 +- modules/core/02-client/keeper/client_test.go | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/e2e/tests/core/client_test.go b/e2e/tests/core/client_test.go index 1f9cbb1cf4d..4f8992b8ce0 100644 --- a/e2e/tests/core/client_test.go +++ b/e2e/tests/core/client_test.go @@ -67,7 +67,7 @@ func (s *ClientTestSuite) TestClientUpdateProposal_Succeeds() { relayer ibc.Relayer subjectClientID string substituteClientID string - badTrustingPeriod = time.Duration(time.Second) + badTrustingPeriod = time.Duration(time.Second * 10) ) t.Run("create substitute client with correct trusting period", func(t *testing.T) { diff --git a/modules/core/02-client/keeper/client_test.go b/modules/core/02-client/keeper/client_test.go index f23bf2ca57d..012efa62091 100644 --- a/modules/core/02-client/keeper/client_test.go +++ b/modules/core/02-client/keeper/client_test.go @@ -42,7 +42,7 @@ func (suite *KeeperTestSuite) TestCreateClient() { clientState = ibctm.NewClientState(testChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath) tmcs, ok := clientState.(*ibctm.ClientState) suite.Require().True(ok) - tmcs.FrozenHeight = clienttypes.NewHeight(0, 1) + tmcs.FrozenHeight = ibctm.FrozenHeight consensusState = suite.consensusState }, false, @@ -65,7 +65,7 @@ func (suite *KeeperTestSuite) TestCreateClient() { }, } - for i, tc := range testCases { + for _, tc := range testCases { tc := tc suite.Run(fmt.Sprintf("Case %s", tc.msg), func() { @@ -74,12 +74,12 @@ func (suite *KeeperTestSuite) TestCreateClient() { clientID, err := suite.keeper.CreateClient(suite.ctx, clientState, consensusState) if tc.expPass { - suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.msg) - suite.Require().NotNil(clientID, "valid test case %d failed: %s", i, tc.msg) + suite.Require().NoError(err) + suite.Require().NotNil(clientID) suite.Require().True(suite.keeper.ClientStore(suite.ctx, clientID).Has(host.ClientStateKey())) } else { - suite.Require().Error(err, "invalid test case %d passed: %s", i, tc.msg) - suite.Require().Equal("", clientID, "invalid test case %d passed: %s", i, tc.msg) + suite.Require().Error(err) + suite.Require().Equal("", clientID) suite.Require().False(suite.keeper.ClientStore(suite.ctx, clientID).Has(host.ClientStateKey())) } }) From 7af692f26b85a07a6f74dc14cea1d5957349300d Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Thu, 27 Apr 2023 11:54:09 +0300 Subject: [PATCH 3/3] Final review touches. --- e2e/tests/core/client_test.go | 3 ++- modules/core/02-client/keeper/client_test.go | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/e2e/tests/core/client_test.go b/e2e/tests/core/client_test.go index 4f8992b8ce0..71f2c41727e 100644 --- a/e2e/tests/core/client_test.go +++ b/e2e/tests/core/client_test.go @@ -67,7 +67,8 @@ func (s *ClientTestSuite) TestClientUpdateProposal_Succeeds() { relayer ibc.Relayer subjectClientID string substituteClientID string - badTrustingPeriod = time.Duration(time.Second * 10) + // set the trusting period to a value which will still be valid upon client creation, but invalid before the first update + badTrustingPeriod = time.Duration(time.Second * 10) ) t.Run("create substitute client with correct trusting period", func(t *testing.T) { diff --git a/modules/core/02-client/keeper/client_test.go b/modules/core/02-client/keeper/client_test.go index 012efa62091..913d079def5 100644 --- a/modules/core/02-client/keeper/client_test.go +++ b/modules/core/02-client/keeper/client_test.go @@ -75,11 +75,11 @@ func (suite *KeeperTestSuite) TestCreateClient() { clientID, err := suite.keeper.CreateClient(suite.ctx, clientState, consensusState) if tc.expPass { suite.Require().NoError(err) - suite.Require().NotNil(clientID) + suite.Require().NotEmpty(clientID) suite.Require().True(suite.keeper.ClientStore(suite.ctx, clientID).Has(host.ClientStateKey())) } else { suite.Require().Error(err) - suite.Require().Equal("", clientID) + suite.Require().Empty(clientID) suite.Require().False(suite.keeper.ClientStore(suite.ctx, clientID).Has(host.ClientStateKey())) } })