refactor: remove crossing hellos from 03-connection (backport cosmos#1672) (cosmos#1691)

* refactor: remove crossing hellos from 03-connection (cosmos#1672)

* remove crossing hello check from ConnOpenTry and ConnOpenAck

* remove unnecessary test cases and fix build

* fix tests and add migration docs

* fix connection version check in conn open ack

* add changelog entry

* Update modules/core/03-connection/keeper/handshake.go

Co-authored-by: Aditya <[email protected]>

* remove unnecessary testing function

* improve migration documentation as per review suggestion

Co-authored-by: Aditya <[email protected]>
(cherry picked from commit 9aab42d)

* fix merge conflicts

* remove missed merge conflicts

* Update docs/migrations/v3-to-v4.md

Co-authored-by: colin axnér <[email protected]>

Author: 2 people

CHANGELOG.md

@@ -43,6 +43,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### API Breaking
 
+* (modules/core/03-connection) [\#1672](https://github.com/cosmos/ibc-go/pull/1672) Remove crossing hellos from connection handshakes. The `PreviousConnectionId` in `MsgConnectionOpenTry` has been deprecated.
 * (modules/core/04-channel) [\#1317](https://github.com/cosmos/ibc-go/pull/1317) Remove crossing hellos from channel handshakes. The `PreviousChannelId` in `MsgChannelOpenTry` has been deprecated.  
 * (transfer) [\#1250](https://github.com/cosmos/ibc-go/pull/1250) Deprecate `GetTransferAccount` since the `transfer` module account is never used.
 * (channel) [\#1283](https://github.com/cosmos/ibc-go/pull/1283) The `OnChanOpenInit` application callback now returns a version string in line with the latest [spec changes](https://github.com/cosmos/ibc/pull/629).  

docs/ibc/proto-docs.md

@@ -4244,7 +4244,7 @@ connection on Chain B.
 | Field | Type | Label | Description |
 | ----- | ---- | ----- | ----------- |
 | `client_id` | [string](#string) |  |  |
-| `previous_connection_id` | [string](#string) |  | in the case of crossing hello's, when both chains call OpenInit, we need the connection identifier of the previous connection in state INIT |
+| `previous_connection_id` | [string](#string) |  | **Deprecated.** Deprecated: this field is unused. Crossing hellos are no longer supported in core IBC. |
 | `client_state` | [google.protobuf.Any](#google.protobuf.Any) |  |  |
 | `counterparty` | [Counterparty](#ibc.core.connection.v1.Counterparty) |  |  |
 | `delay_period` | [uint64](#uint64) |  |  |

docs/migrations/v3-to-v4.md

@@ -22,6 +22,11 @@ No genesis or in-place migrations required when upgrading from v1 or v2 of ibc-g
 
 ## IBC Apps
 
+### ICS03 - Connection
+
+Crossing hellos have been removed from 03-connection handshake negotiation. 
+`PreviousConnectionId` in `MsgConnectionOpenTry` has been deprecated and is no longer used by core IBC.
+
 ### ICS04 - Channel 
 
 The `WriteAcknowledgement` API now takes the `exported.Acknowledgement` type instead of passing in the acknowledgement byte array directly. 
@@ -100,4 +105,4 @@ if err := k.icaControllerKeeper.RegisterInterchainAccount(ctx, msg.ConnectionId,
 
 When using the `DenomTrace` gRPC, the full IBC denomination with the `ibc/` prefix may now be passed in.
 
-Crossing hellos are no longer supported by core IBC. The handshake should be completed in the logical 4 step process (INIT, TRY, ACK, CONFIRM).
+Crossing hellos are no longer supported by core IBC for 03-connection and 04-channel. The handshake should be completed in the logical 4 step process (INIT, TRY, ACK, CONFIRM).

modules/core/03-connection/keeper/handshake.go

@@ -1,12 +1,9 @@
 package keeper
 
 import (
-	"bytes"
-
 	"github.com/Finschia/finschia-sdk/telemetry"
 	sdk "github.com/Finschia/finschia-sdk/types"
 	sdkerrors "github.com/Finschia/finschia-sdk/types/errors"
-	"github.com/gogo/protobuf/proto"
 
 	clienttypes "github.com/cosmos/ibc-go/v4/modules/core/02-client/types"
 	"github.com/cosmos/ibc-go/v4/modules/core/03-connection/types"
@@ -28,7 +25,7 @@ func (k Keeper) ConnOpenInit(
 ) (string, error) {
 	versions := types.GetCompatibleVersions()
 	if version != nil {
-		if !types.IsSupportedVersion(version) {
+		if !types.IsSupportedVersion(types.GetCompatibleVersions(), version) {
 			return "", sdkerrors.Wrap(types.ErrInvalidVersion, "version is not supported")
 		}
 
@@ -63,7 +60,6 @@ func (k Keeper) ConnOpenInit(
 //   - Identifiers are checked on msg validation
 func (k Keeper) ConnOpenTry(
 	ctx sdk.Context,
-	previousConnectionID string, // previousIdentifier
 	counterparty types.Counterparty, // counterpartyConnectionIdentifier, counterpartyPrefix and counterpartyClientIdentifier
 	delayPeriod uint64,
 	clientID string, // clientID of chainA
@@ -75,44 +71,9 @@ func (k Keeper) ConnOpenTry(
 	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
 ) (string, error) {
-	var (
-		connectionID       string
-		previousConnection types.ConnectionEnd
-		found              bool
-	)
-
-	// empty connection identifier indicates continuing a previous connection handshake
-	if previousConnectionID != "" {
-		// ensure that the previous connection exists
-		previousConnection, found = k.GetConnection(ctx, previousConnectionID)
-		if !found {
-			return "", sdkerrors.Wrapf(types.ErrConnectionNotFound, "previous connection does not exist for supplied previous connectionID %s", previousConnectionID)
-		}
-
-		// ensure that the existing connection's
-		// counterparty is chainA and connection is on INIT stage.
-		// Check that existing connection versions for initialized connection is equal to compatible
-		// versions for this chain.
-		// ensure that existing connection's delay period is the same as desired delay period.
-		if !(previousConnection.Counterparty.ConnectionId == "" &&
-			bytes.Equal(previousConnection.Counterparty.Prefix.Bytes(), counterparty.Prefix.Bytes()) &&
-			previousConnection.ClientId == clientID &&
-			previousConnection.Counterparty.ClientId == counterparty.ClientId &&
-			previousConnection.DelayPeriod == delayPeriod) {
-			return "", sdkerrors.Wrap(types.ErrInvalidConnection, "connection fields mismatch previous connection fields")
-		}
 
-		if !(previousConnection.State == types.INIT) {
-			return "", sdkerrors.Wrapf(types.ErrInvalidConnectionState, "previous connection state is in state %s, expected INIT", previousConnection.State)
-		}
-
-		// continue with previous connection
-		connectionID = previousConnectionID
-
-	} else {
-		// generate a new connection
-		connectionID = k.GenerateConnectionIdentifier(ctx)
-	}
+	// generate a new connection
+	connectionID := k.GenerateConnectionIdentifier(ctx)
 
 	selfHeight := clienttypes.GetSelfHeight(ctx)
 	if consensusHeight.GTE(selfHeight) {
@@ -139,15 +100,10 @@ func (k Keeper) ConnOpenTry(
 	expectedCounterparty := types.NewCounterparty(clientID, "", commitmenttypes.NewMerklePrefix(prefix.Bytes()))
 	expectedConnection := types.NewConnectionEnd(types.INIT, counterparty.ClientId, expectedCounterparty, types.ExportedVersionsToProto(counterpartyVersions), delayPeriod)
 
-	supportedVersions := types.GetCompatibleVersions()
-	if len(previousConnection.Versions) != 0 {
-		supportedVersions = previousConnection.GetVersions()
-	}
-
 	// chain B picks a version from Chain A's available versions that is compatible
 	// with Chain B's supported IBC versions. PickVersion will select the intersection
 	// of the supported versions and the counterparty versions.
-	version, err := types.PickVersion(supportedVersions, counterpartyVersions)
+	version, err := types.PickVersion(types.GetCompatibleVersions(), counterpartyVersions)
 	if err != nil {
 		return "", err
 	}
@@ -181,7 +137,7 @@ func (k Keeper) ConnOpenTry(
 	}
 
 	k.SetConnection(ctx, connectionID, connection)
-	k.Logger(ctx).Info("connection state updated", "connection-id", connectionID, "previous-state", previousConnection.State.String(), "new-state", "TRYOPEN")
+	k.Logger(ctx).Info("connection state updated", "connection-id", connectionID, "previous-state", "NONE", "new-state", "TRYOPEN")
 
 	defer func() {
 		telemetry.IncrCounter(1, "ibc", "connection", "open-try")
@@ -223,28 +179,19 @@ func (k Keeper) ConnOpenAck(
 		return sdkerrors.Wrap(types.ErrConnectionNotFound, connectionID)
 	}
 
-	// Verify the provided version against the previously set connection state
-	switch {
-	// connection on ChainA must be in INIT or TRYOPEN
-	case connection.State != types.INIT && connection.State != types.TRYOPEN:
-		return sdkerrors.Wrapf(
-			types.ErrInvalidConnectionState,
-			"connection state is not INIT or TRYOPEN (got %s)", connection.State.String(),
-		)
-
-	// if the connection is INIT then the provided version must be supproted
-	case connection.State == types.INIT && !types.IsSupportedVersion(version):
+	// verify the previously set connection state
+	if connection.State != types.INIT {
 		return sdkerrors.Wrapf(
 			types.ErrInvalidConnectionState,
-			"connection state is in INIT but the provided version is not supported %s", version,
+			"connection state is not INIT (got %s)", connection.State.String(),
 		)
+	}
 
-	// if the connection is in TRYOPEN then the version must be the only set version in the
-	// retreived connection state.
-	case connection.State == types.TRYOPEN && (len(connection.Versions) != 1 || !proto.Equal(connection.Versions[0], version)):
+	// ensure selected version is supported
+	if !types.IsSupportedVersion(types.ProtoVersionsToExported(connection.Versions), version) {
 		return sdkerrors.Wrapf(
 			types.ErrInvalidConnectionState,
-			"connection state is in TRYOPEN but the provided version (%s) is not set in the previous connection versions %s", version, connection.Versions,
+			"the counterparty selected version %s is not supported by versions selected on INIT", version,
 		)
 	}
 
@@ -283,7 +230,7 @@ func (k Keeper) ConnOpenAck(
 		return err
 	}
 
-	k.Logger(ctx).Info("connection state updated", "connection-id", connectionID, "previous-state", connection.State.String(), "new-state", "OPEN")
+	k.Logger(ctx).Info("connection state updated", "connection-id", connectionID, "previous-state", "INIT", "new-state", "OPEN")
 
 	defer func() {
 		telemetry.IncrCounter(1, "ibc", "connection", "open-ack")

modules/core/03-connection/keeper/handshake_test.go

@@ -80,12 +80,11 @@ func (suite *KeeperTestSuite) TestConnOpenInit() {
 // connection on chainA is INIT
 func (suite *KeeperTestSuite) TestConnOpenTry() {
 	var (
-		path                 *ibctesting.Path
-		delayPeriod          uint64
-		previousConnectionID string
-		versions             []exported.Version
-		consensusHeight      exported.Height
-		counterpartyClient   exported.ClientState
+		path               *ibctesting.Path
+		delayPeriod        uint64
+		versions           []exported.Version
+		consensusHeight    exported.Height
+		counterpartyClient exported.ClientState
 	)
 
 	testCases := []struct {
@@ -100,15 +99,6 @@ func (suite *KeeperTestSuite) TestConnOpenTry() {
 			// retrieve client state of chainA to pass as counterpartyClient
 			counterpartyClient = suite.chainA.GetClientState(path.EndpointA.ClientID)
 		}, true},
-		{"success with crossing hellos", func() {
-			err := suite.coordinator.ConnOpenInitOnBothChains(path)
-			suite.Require().NoError(err)
-
-			// retrieve client state of chainA to pass as counterpartyClient
-			counterpartyClient = suite.chainA.GetClientState(path.EndpointA.ClientID)
-
-			previousConnectionID = path.EndpointB.ConnectionID
-		}, true},
 		{"success with delay period", func() {
 			err := path.EndpointA.ConnOpenInit()
 			suite.Require().NoError(err)
@@ -227,8 +217,6 @@ func (suite *KeeperTestSuite) TestConnOpenTry() {
 
 			// retrieve client state of chainA to pass as counterpartyClient
 			counterpartyClient = suite.chainA.GetClientState(path.EndpointA.ClientID)
-
-			previousConnectionID = path.EndpointB.ConnectionID
 		}, false},
 		{"invalid previous connection has invalid versions", func() {
 			// open init chainA
@@ -253,8 +241,6 @@ func (suite *KeeperTestSuite) TestConnOpenTry() {
 
 			// retrieve client state of chainA to pass as counterpartyClient
 			counterpartyClient = suite.chainA.GetClientState(path.EndpointA.ClientID)
-
-			previousConnectionID = path.EndpointB.ConnectionID
 		}, false},
 	}
 
@@ -265,7 +251,6 @@ func (suite *KeeperTestSuite) TestConnOpenTry() {
 			suite.SetupTest()                          // reset
 			consensusHeight = clienttypes.ZeroHeight() // must be explicitly changed in malleate
 			versions = types.GetCompatibleVersions()   // must be explicitly changed in malleate
-			previousConnectionID = ""
 			path = ibctesting.NewPath(suite.chainA, suite.chainB)
 			suite.coordinator.SetupClients(path)
 
@@ -292,7 +277,7 @@ func (suite *KeeperTestSuite) TestConnOpenTry() {
 			proofClient, _ := suite.chainA.QueryProof(clientKey)
 
 			connectionID, err := suite.chainB.App.GetIBCKeeper().ConnectionKeeper.ConnOpenTry(
-				suite.chainB.GetContext(), previousConnectionID, counterparty, delayPeriod, path.EndpointB.ClientID, counterpartyClient,
+				suite.chainB.GetContext(), counterparty, delayPeriod, path.EndpointB.ClientID, counterpartyClient,
 				versions, proofInit, proofClient, proofConsensus,
 				proofHeight, consensusHeight,
 			)
@@ -333,27 +318,6 @@ func (suite *KeeperTestSuite) TestConnOpenAck() {
 			// retrieve client state of chainB to pass as counterpartyClient
 			counterpartyClient = suite.chainB.GetClientState(path.EndpointB.ClientID)
 		}, true},
-		{"success from tryopen", func() {
-			// chainA is in TRYOPEN, chainB is in TRYOPEN
-			err := path.EndpointB.ConnOpenInit()
-			suite.Require().NoError(err)
-
-			err = path.EndpointA.ConnOpenTry()
-			suite.Require().NoError(err)
-
-			// set chainB to TRYOPEN
-			connection := path.EndpointB.GetConnection()
-			connection.State = types.TRYOPEN
-			connection.Counterparty.ConnectionId = path.EndpointA.ConnectionID
-			suite.chainB.App.GetIBCKeeper().ConnectionKeeper.SetConnection(suite.chainB.GetContext(), path.EndpointB.ConnectionID, connection)
-			// update path.EndpointB.ClientID so state change is committed
-			path.EndpointB.UpdateClient()
-
-			path.EndpointA.UpdateClient()
-
-			// 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)
@@ -440,28 +404,6 @@ func (suite *KeeperTestSuite) TestConnOpenAck() {
 
 			version = types.NewVersion("2.0", nil)
 		}, false},
-		{"connection is in TRYOPEN but the set version in the connection is invalid", func() {
-			// chainA is in TRYOPEN, chainB is in TRYOPEN
-			err := path.EndpointB.ConnOpenInit()
-			suite.Require().NoError(err)
-
-			err = path.EndpointA.ConnOpenTry()
-			suite.Require().NoError(err)
-
-			// set chainB to TRYOPEN
-			connection := path.EndpointB.GetConnection()
-			connection.State = types.TRYOPEN
-			suite.chainB.App.GetIBCKeeper().ConnectionKeeper.SetConnection(suite.chainB.GetContext(), path.EndpointB.ConnectionID, connection)
-
-			// update path.EndpointB.ClientID so state change is committed
-			path.EndpointB.UpdateClient()
-			path.EndpointA.UpdateClient()
-
-			// retrieve client state of chainB to pass as counterpartyClient
-			counterpartyClient = suite.chainB.GetClientState(path.EndpointB.ClientID)
-
-			version = types.NewVersion("2.0", nil)
-		}, false},
 		{"incompatible IBC versions", func() {
 			err := path.EndpointA.ConnOpenInit()
 			suite.Require().NoError(err)