backport-v8.5.x: self client/consensus state removal from connection …

…handshake (#7129) * refactor: remove client/consensus state validation from connection handshake * chore: generate proto deprecation notice * add changelog * refactor: remove unpacking from msg server
cosmos · Aug 13, 2024 · 27630bb · 27630bb
1 parent 8479edf
commit 27630bb
Show file tree

Hide file tree

Showing 10 changed files with 107 additions and 407 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -34,6 +34,12 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 # Changelog
 
+## [v8.5.0]()
+
+### State Machine Breaking
+
+* (core/03-connection)[\#7129](https://github.com/cosmos/ibc-go/pull/7129) Remove verification of self client and consensus state from connection handshake. 
+
 ## [v8.4.0](https://github.com/cosmos/ibc-go/releases/tag/v8.4.0) - 2024-07-29
 
 ### Improvements

diff --git a/go.mod b/go.mod
@@ -3,12 +3,12 @@ go 1.21
 module github.com/cosmos/ibc-go/v8
 
 retract (
-		// contain bug in channel upgradability that can result 
-		// in a channel upgrade succeeding but with channel ends
-		// in incompatible state
-    [v8.2.0, v8.3.2]
-		// contain ASA-2024-007 vulnerability
-    [v8.0.0, v8.1.1]
+	// contain bug in channel upgradability that can result
+	// in a channel upgrade succeeding but with channel ends
+	// in incompatible state
+	[v8.2.0, v8.3.2]
+	// contain ASA-2024-007 vulnerability
+	[v8.0.0, v8.1.1]
 )
 
 require (

diff --git a/modules/core/03-connection/keeper/handshake.go b/modules/core/03-connection/keeper/handshake.go
@@ -9,7 +9,6 @@ import (
 	clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
 	"github.com/cosmos/ibc-go/v8/modules/core/03-connection/types"
 	commitmenttypes "github.com/cosmos/ibc-go/v8/modules/core/23-commitment/types"
-	ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors"
 	"github.com/cosmos/ibc-go/v8/modules/core/exported"
 )
 
@@ -72,37 +71,17 @@ func (k Keeper) ConnOpenTry(
 	counterparty types.Counterparty, // counterpartyConnectionIdentifier, counterpartyPrefix and counterpartyClientIdentifier
 	delayPeriod uint64,
 	clientID string, // clientID of chainA
-	clientState exported.ClientState, // clientState that chainA has for chainB
+	_ exported.ClientState, // clientState that chainA has for chainB
 	counterpartyVersions []*types.Version, // supported versions of chain A
 	initProof []byte, // proof that chainA stored connectionEnd in state (on ConnOpenInit)
-	clientProof []byte, // proof that chainA stored a light client of chainB
-	consensusProof []byte, // proof that chainA stored chainB's consensus state at consensus height
+	_ []byte, // proof that chainA stored a light client of chainB
+	_ []byte, // proof that chainA stored chainB's consensus state at consensus height
 	proofHeight exported.Height, // height at which relayer constructs proof of A storing connectionEnd in state
-	consensusHeight exported.Height, // latest height of chain B which chain A has stored in its chain B client
+	_ exported.Height, // latest height of chain B which chain A has stored in its chain B client
 ) (string, error) {
 	// generate a new connection
 	connectionID := k.GenerateConnectionIdentifier(ctx)
 
-	// check that the consensus height the counterparty chain is using to store a representation
-	// of this chain's consensus state is at a height in the past
-	selfHeight := clienttypes.GetSelfHeight(ctx)
-	if consensusHeight.GTE(selfHeight) {
-		return "", errorsmod.Wrapf(
-			ibcerrors.ErrInvalidHeight,
-			"consensus height is greater than or equal to the current block height (%s >= %s)", consensusHeight, selfHeight,
-		)
-	}
-
-	// validate client parameters of a chainB client stored on chainA
-	if err := k.consensusHost.ValidateSelfClient(ctx, clientState); err != nil {
-		return "", err
-	}
-
-	expectedConsensusState, err := k.consensusHost.GetSelfConsensusState(ctx, consensusHeight)
-	if err != nil {
-		return "", errorsmod.Wrapf(err, "self consensus state not found for height %s", consensusHeight.String())
-	}
-
 	// expectedConnection defines Chain A's ConnectionEnd
 	// NOTE: chain A's counterparty is chain B (i.e where this code is executed)
 	// NOTE: chainA and chainB must have the same delay period
@@ -129,18 +108,6 @@ func (k Keeper) ConnOpenTry(
 		return "", err
 	}
 
-	// Check that ChainA stored the clientState provided in the msg
-	if err := k.VerifyClientState(ctx, connection, proofHeight, clientProof, clientState); err != nil {
-		return "", err
-	}
-
-	// Check that ChainA stored the correct ConsensusState of chainB at the given consensusHeight
-	if err := k.VerifyClientConsensusState(
-		ctx, connection, proofHeight, consensusHeight, consensusProof, expectedConsensusState,
-	); err != nil {
-		return "", err
-	}
-
 	// store connection in chainB state
 	if err := k.addConnectionToClient(ctx, clientID, connectionID); err != nil {
 		return "", errorsmod.Wrapf(err, "failed to add connection with ID %s to client with ID %s", connectionID, clientID)
@@ -163,25 +130,15 @@ func (k Keeper) ConnOpenTry(
 func (k Keeper) ConnOpenAck(
 	ctx sdk.Context,
 	connectionID string,
-	clientState exported.ClientState, // client state for chainA on chainB
+	_ exported.ClientState, // client state for chainA on chainB
 	version *types.Version, // version that ChainB chose in ConnOpenTry
 	counterpartyConnectionID string,
 	tryProof []byte, // proof that connectionEnd was added to ChainB state in ConnOpenTry
-	clientProof []byte, // proof of client state on chainB for chainA
-	consensusProof []byte, // proof that chainB has stored ConsensusState of chainA on its client
+	_ []byte, // proof of client state on chainB for chainA
+	_ []byte, // proof that chainB has stored ConsensusState of chainA on its client
 	proofHeight exported.Height, // height that relayer constructed proofTry
-	consensusHeight exported.Height, // latest height of chainA that chainB has stored on its chainA client
+	_ exported.Height, // latest height of chainA that chainB has stored on its chainA client
 ) error {
-	// check that the consensus height the counterparty chain is using to store a representation
-	// of this chain's consensus state is at a height in the past
-	selfHeight := clienttypes.GetSelfHeight(ctx)
-	if consensusHeight.GTE(selfHeight) {
-		return errorsmod.Wrapf(
-			ibcerrors.ErrInvalidHeight,
-			"consensus height is greater than or equal to the current block height (%s >= %s)", consensusHeight, selfHeight,
-		)
-	}
-
 	// Retrieve connection
 	connection, found := k.GetConnection(ctx, connectionID)
 	if !found {
@@ -204,17 +161,6 @@ func (k Keeper) ConnOpenAck(
 		)
 	}
 
-	// validate client parameters of a chainA client stored on chainB
-	if err := k.consensusHost.ValidateSelfClient(ctx, clientState); err != nil {
-		return err
-	}
-
-	// Retrieve chainA's consensus state at consensusheight
-	expectedConsensusState, err := k.consensusHost.GetSelfConsensusState(ctx, consensusHeight)
-	if err != nil {
-		return errorsmod.Wrapf(err, "self consensus state not found for height %s", consensusHeight.String())
-	}
-
 	prefix := k.GetCommitmentPrefix()
 	expectedCounterparty := types.NewCounterparty(connection.ClientId, connectionID, commitmenttypes.NewMerklePrefix(prefix.Bytes()))
 	expectedConnection := types.NewConnectionEnd(types.TRYOPEN, connection.Counterparty.ClientId, expectedCounterparty, []*types.Version{version}, connection.DelayPeriod)
@@ -227,18 +173,6 @@ func (k Keeper) ConnOpenAck(
 		return err
 	}
 
-	// Check that ChainB stored the clientState provided in the msg
-	if err := k.VerifyClientState(ctx, connection, proofHeight, clientProof, clientState); err != nil {
-		return err
-	}
-
-	// Ensure that ChainB has stored the correct ConsensusState for chainA at the consensusHeight
-	if err := k.VerifyClientConsensusState(
-		ctx, connection, proofHeight, consensusHeight, consensusProof, expectedConsensusState,
-	); err != nil {
-		return err
-	}
-
 	k.Logger(ctx).Info("connection state updated", "connection-id", connectionID, "previous-state", types.INIT.String(), "new-state", types.OPEN.String())
 
 	defer telemetry.IncrCounter(1, "ibc", "connection", "open-ack")

diff --git a/modules/core/03-connection/keeper/handshake_test.go b/modules/core/03-connection/keeper/handshake_test.go
@@ -3,15 +3,11 @@ package keeper_test
 import (
 	"time"
 
-	sdk "github.com/cosmos/cosmos-sdk/types"
-
 	clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
 	"github.com/cosmos/ibc-go/v8/modules/core/03-connection/types"
 	host "github.com/cosmos/ibc-go/v8/modules/core/24-host"
 	"github.com/cosmos/ibc-go/v8/modules/core/exported"
-	ibctm "github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint"
 	ibctesting "github.com/cosmos/ibc-go/v8/testing"
-	"github.com/cosmos/ibc-go/v8/testing/mock"
 )
 
 // TestConnOpenInit - chainA initializes (INIT state) a connection with
@@ -135,38 +131,6 @@ func (suite *KeeperTestSuite) TestConnOpenTry() {
 			// retrieve client state of chainA to pass as counterpartyClient
 			counterpartyClient = suite.chainA.GetClientState(path.EndpointA.ClientID)
 		}, true},
-		{"invalid counterparty client", func() {
-			err := path.EndpointA.ConnOpenInit()
-			suite.Require().NoError(err)
-
-			// retrieve client state of chainB to pass as counterpartyClient
-			counterpartyClient = suite.chainA.GetClientState(path.EndpointA.ClientID)
-
-			// Set an invalid client of chainA on chainB
-			tmClient, ok := counterpartyClient.(*ibctm.ClientState)
-			suite.Require().True(ok)
-			tmClient.ChainId = "wrongchainid"
-
-			suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainA.GetContext(), path.EndpointA.ClientID, tmClient)
-		}, false},
-		{"consensus height >= latest height", func() {
-			err := path.EndpointA.ConnOpenInit()
-			suite.Require().NoError(err)
-
-			// retrieve client state of chainA to pass as counterpartyClient
-			counterpartyClient = suite.chainA.GetClientState(path.EndpointA.ClientID)
-
-			consensusHeight = clienttypes.GetSelfHeight(suite.chainB.GetContext())
-		}, false},
-		{"self consensus state not found", func() {
-			err := path.EndpointA.ConnOpenInit()
-			suite.Require().NoError(err)
-
-			// retrieve client state of chainA to pass as counterpartyClient
-			counterpartyClient = suite.chainA.GetClientState(path.EndpointA.ClientID)
-
-			consensusHeight = clienttypes.NewHeight(0, 1)
-		}, false},
 		{"counterparty versions is empty", func() {
 			err := path.EndpointA.ConnOpenInit()
 			suite.Require().NoError(err)
@@ -192,50 +156,6 @@ func (suite *KeeperTestSuite) TestConnOpenTry() {
 			// retrieve client state of chainA to pass as counterpartyClient
 			counterpartyClient = suite.chainA.GetClientState(path.EndpointA.ClientID)
 		}, false},
-		{"client state verification failed", func() {
-			err := path.EndpointA.ConnOpenInit()
-			suite.Require().NoError(err)
-
-			// retrieve client state of chainA to pass as counterpartyClient
-			counterpartyClient = suite.chainA.GetClientState(path.EndpointA.ClientID)
-
-			// modify counterparty client without setting in store so it still passes validate but fails proof verification
-			tmClient, ok := counterpartyClient.(*ibctm.ClientState)
-			suite.Require().True(ok)
-			tmClient.LatestHeight = tmClient.LatestHeight.Increment().(clienttypes.Height)
-		}, false},
-		{"consensus state verification failed", func() {
-			// retrieve client state of chainA to pass as counterpartyClient
-			counterpartyClient = suite.chainA.GetClientState(path.EndpointA.ClientID)
-
-			// give chainA wrong consensus state for chainB
-			consState, found := suite.chainA.App.GetIBCKeeper().ClientKeeper.GetLatestClientConsensusState(suite.chainA.GetContext(), path.EndpointA.ClientID)
-			suite.Require().True(found)
-
-			tmConsState, ok := consState.(*ibctm.ConsensusState)
-			suite.Require().True(ok)
-
-			tmConsState.Timestamp = time.Now()
-			suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientConsensusState(suite.chainA.GetContext(), path.EndpointA.ClientID, counterpartyClient.GetLatestHeight(), tmConsState)
-
-			err := path.EndpointA.ConnOpenInit()
-			suite.Require().NoError(err)
-		}, false},
-		{"override self consensus host", func() {
-			err := path.EndpointA.ConnOpenInit()
-			suite.Require().NoError(err)
-
-			// retrieve client state of chainA to pass as counterpartyClient
-			counterpartyClient = suite.chainA.GetClientState(path.EndpointA.ClientID)
-
-			mockValidator := mock.ConsensusHost{
-				ValidateSelfClientFn: func(ctx sdk.Context, clientState exported.ClientState) error {
-					return mock.MockApplicationCallbackError
-				},
-			}
-
-			suite.chainB.App.GetIBCKeeper().SetConsensusHost(&mockValidator)
-		}, false},
 	}
 
 	for _, tc := range testCases {
@@ -313,35 +233,6 @@ func (suite *KeeperTestSuite) TestConnOpenAck() {
 			// retrieve client state of chainB to pass as counterpartyClient
 			counterpartyClient = suite.chainB.GetClientState(path.EndpointB.ClientID)
 		}, true},
-		{"invalid counterparty client", func() {
-			err := path.EndpointA.ConnOpenInit()
-			suite.Require().NoError(err)
-
-			err = path.EndpointB.ConnOpenTry()
-			suite.Require().NoError(err)
-
-			// retrieve client state of chainB to pass as counterpartyClient
-			counterpartyClient = suite.chainB.GetClientState(path.EndpointB.ClientID)
-
-			// Set an invalid client of chainA on chainB
-			tmClient, ok := counterpartyClient.(*ibctm.ClientState)
-			suite.Require().True(ok)
-			tmClient.ChainId = "wrongchainid"
-
-			suite.chainB.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainB.GetContext(), path.EndpointB.ClientID, tmClient)
-		}, false},
-		{"consensus height >= latest height", func() {
-			err := path.EndpointA.ConnOpenInit()
-			suite.Require().NoError(err)
-
-			// retrieve client state of chainB to pass as counterpartyClient
-			counterpartyClient = suite.chainB.GetClientState(path.EndpointB.ClientID)
-
-			err = path.EndpointB.ConnOpenTry()
-			suite.Require().NoError(err)
-
-			consensusHeight = clienttypes.GetSelfHeight(suite.chainA.GetContext())
-		}, false},
 		{"connection not found", func() {
 			// connections are never created
 
@@ -363,6 +254,7 @@ func (suite *KeeperTestSuite) TestConnOpenAck() {
 			suite.Require().True(found)
 
 			connection.Counterparty.ConnectionId = "badconnectionid"
+			path.EndpointB.ConnectionID = "badconnectionid"
 
 			suite.chainA.App.GetIBCKeeper().ConnectionKeeper.SetConnection(suite.chainA.GetContext(), path.EndpointA.ConnectionID, connection)
 
@@ -436,18 +328,6 @@ func (suite *KeeperTestSuite) TestConnOpenAck() {
 
 			version = types.NewVersion(types.DefaultIBCVersionIdentifier, []string{"ORDER_ORDERED", "ORDER_UNORDERED", "ORDER_DAG"})
 		}, false},
-		{"self consensus state not found", func() {
-			err := path.EndpointA.ConnOpenInit()
-			suite.Require().NoError(err)
-
-			// retrieve client state of chainB to pass as counterpartyClient
-			counterpartyClient = suite.chainB.GetClientState(path.EndpointB.ClientID)
-
-			err = path.EndpointB.ConnOpenTry()
-			suite.Require().NoError(err)
-
-			consensusHeight = clienttypes.NewHeight(0, 1)
-		}, false},
 		{"connection state verification failed", func() {
 			// chainB connection is not in INIT
 			err := path.EndpointA.ConnOpenInit()
@@ -456,41 +336,6 @@ func (suite *KeeperTestSuite) TestConnOpenAck() {
 			// retrieve client state of chainB to pass as counterpartyClient
 			counterpartyClient = suite.chainB.GetClientState(path.EndpointB.ClientID)
 		}, false},
-		{"client state verification failed", func() {
-			err := path.EndpointA.ConnOpenInit()
-			suite.Require().NoError(err)
-
-			// retrieve client state of chainB to pass as counterpartyClient
-			counterpartyClient = suite.chainB.GetClientState(path.EndpointB.ClientID)
-
-			// modify counterparty client without setting in store so it still passes validate but fails proof verification
-			tmClient, ok := counterpartyClient.(*ibctm.ClientState)
-			suite.Require().True(ok)
-			tmClient.LatestHeight = tmClient.LatestHeight.Increment().(clienttypes.Height)
-
-			err = path.EndpointB.ConnOpenTry()
-			suite.Require().NoError(err)
-		}, false},
-		{"consensus state verification failed", func() {
-			err := path.EndpointA.ConnOpenInit()
-			suite.Require().NoError(err)
-
-			// retrieve client state of chainB to pass as counterpartyClient
-			counterpartyClient = suite.chainB.GetClientState(path.EndpointB.ClientID)
-
-			// give chainB wrong consensus state for chainA
-			consState, found := suite.chainB.App.GetIBCKeeper().ClientKeeper.GetLatestClientConsensusState(suite.chainB.GetContext(), path.EndpointB.ClientID)
-			suite.Require().True(found)
-
-			tmConsState, ok := consState.(*ibctm.ConsensusState)
-			suite.Require().True(ok)
-
-			tmConsState.Timestamp = tmConsState.Timestamp.Add(time.Second)
-			suite.chainB.App.GetIBCKeeper().ClientKeeper.SetClientConsensusState(suite.chainB.GetContext(), path.EndpointB.ClientID, counterpartyClient.GetLatestHeight(), tmConsState)
-
-			err = path.EndpointB.ConnOpenTry()
-			suite.Require().NoError(err)
-		}, false},
 	}
 
 	for _, tc := range testCases {