Remove crossings hello (#1317) (#1692)

* remove crossing hellos from ChanOpenTry

* remove crossing hellos testcase

* revert single connectionHops check

* add comment in MsgChannelOpenTry tx proto about deprecate field

* Update proto/ibc/core/channel/v1/tx.proto

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

* minor fixes && UPDATE CHANGELOG.md

* Update proto/ibc/core/channel/v1/tx.proto

Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

* Update proto/ibc/core/channel/v1/tx.proto

Co-authored-by: Damian Nolan <damiannolan@gmail.com>

* apply remaining changes for crossing hello removal

Deprecate previous channel id with proto tag
Remove unnecessary test cases from 04-channel
Remove crossing hello check in transfer application and the associated test case

* remove previous channel check in WriteChannelOpenTry

* regenerate proto files

* update documentation

* add migration documentation

* remove unnecessary doc

* remove crossing hello notion from ChanOpenAck

* apply review suggestions

Co-authored-by: Aditya <adityasripal@gmail.com>
Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
Co-authored-by: Damian Nolan <damiannolan@gmail.com>
Co-authored-by: Jacob Gadikian <jacobgadikian@gmail.com>
Co-authored-by: Colin Axnér <25233464+colin-axner@users.noreply.github.com>
(cherry picked from commit 7370a8b)

Co-authored-by: vuong <56973102+vuong177@users.noreply.github.com>

CHANGELOG.md

@@ -43,6 +43,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### API Breaking
 
+* (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).  
 * (modules/29-fee)[\#1338](https://github.com/cosmos/ibc-go/pull/1338) Renaming `Result` field in `IncentivizedAcknowledgement` to `AppAcknowledgement`.
@@ -62,6 +63,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (modules/core/04-channel) [\#1232](https://github.com/cosmos/ibc-go/pull/1232) Updating params on `NewPacketId` and moving to bottom of file.
 * (app/29-fee) [\#1305](https://github.com/cosmos/ibc-go/pull/1305) Change version string for fee module to `ics29-1`
 * (app/29-fee) [\#1341](https://github.com/cosmos/ibc-go/pull/1341) Check if the fee module is locked and if the fee module is enabled before refunding all fees
+* (transfer) [\#1414](https://github.com/cosmos/ibc-go/pull/1414) Emitting Sender address from `fungible_token_packet` events in `OnRecvPacket` and `OnAcknowledgementPacket`.
 * (testing/simapp) [\#1397](https://github.com/cosmos/ibc-go/pull/1397) Adding mock module to maccperms and adding check to ensure mock module is not a blocked account address. 
 * (core/02-client) [\#1570](https://github.com/cosmos/ibc-go/pull/1570) Emitting an event when handling an upgrade client proposal. 
 

docs/ibc/apps.md

@@ -73,15 +73,9 @@ OnChanOpenTry(
     counterparty channeltypes.Counterparty,
     counterpartyVersion string,
 ) (string, error) {
-    // Module may have already claimed capability in OnChanOpenInit in the case of crossing hellos
-    // (ie chainA and chainB both call ChanOpenInit before one of them calls ChanOpenTry)
-    // If the module can already authenticate the capability then the module already owns it so we don't need to claim
-    // Otherwise, module does not have channel capability and we must claim it from IBC
-    if !k.AuthenticateCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)) {
-        // Only claim channel capability passed back by IBC module if we do not already own it
-        if err := k.scopedKeeper.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil {
-            return err
-        }
+    // OpenTry must claim the channelCapability that IBC passes into the callback
+    if err := k.scopedKeeper.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil {
+        return err
     }
 
     // ... do custom initialization logic

docs/ibc/proto-docs.md

@@ -2949,7 +2949,7 @@ value will be ignored by core IBC.
 | Field | Type | Label | Description |
 | ----- | ---- | ----- | ----------- |
 | `port_id` | [string](#string) |  |  |
-| `previous_channel_id` | [string](#string) |  | in the case of crossing hello's, when both chains call OpenInit, we need the channel identifier of the previous channel in state INIT |
+| `previous_channel_id` | [string](#string) |  | **Deprecated.** Deprecated: this field is unused. Crossing hello's are no longer supported in core IBC. |
 | `channel` | [Channel](#ibc.core.channel.v1.Channel) |  | NOTE: the version field within the channel has been deprecated. Its value will be ignored by core IBC. |
 | `counterparty_version` | [string](#string) |  |  |
 | `proof_init` | [bytes](#bytes) |  |  |

docs/migrations/v3-to-v4.md

@@ -18,6 +18,10 @@ No genesis or in-place migrations required when upgrading from v1 or v2 of ibc-g
 
 ## Chains
 
+- No relevant changes were made in this release.
+
+## IBC Apps
+
 ### ICS04 - Channel 
 
 The `WriteAcknowledgement` API now takes the `exported.Acknowledgement` type instead of passing in the acknowledgement byte array directly. 
@@ -30,6 +34,10 @@ The `NewErrorAcknowledgement` method signature has changed.
 It now accepts an `error` rather than a `string`. This was done in order to prevent accidental state changes.
 All error acknowledgements now contain a deterministic ABCI code and error message. It is the responsibility of the application developer to emit error details in events.
 
+Crossing hellos have been removed from 04-channel handshake negotiation. 
+IBC Applications no longer need to account from already claimed capabilities in the `OnChanOpenTry` callback. The capability provided by core IBC must be able to be claimed with error. 
+`PreviousChannelId` in `MsgChannelOpenTry` has been deprecated and is no longer used by core IBC.
+
 ### ICS27 - Interchain Accounts
 
 The `RegisterInterchainAccount` API has been modified to include an additional `version` argument. This change has been made in order to support ICS29 fee middleware, for relayer incentivization of ICS27 packets.
@@ -91,3 +99,5 @@ if err := k.icaControllerKeeper.RegisterInterchainAccount(ctx, msg.ConnectionId,
 ## Relayers
 
 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).

modules/apps/29-fee/ibc_middleware_test.go

@@ -141,38 +141,27 @@ func (suite *FeeTestSuite) TestOnChanOpenTry() {
 	testCases := []struct {
 		name      string
 		cpVersion string
-		crossing  bool
 		expPass   bool
 	}{
 		{
 			"success - valid fee middleware version",
 			string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: ibcmock.Version})),
-			false,
 			true,
 		},
 		{
 			"success - valid mock version",
 			ibcmock.Version,
-			false,
-			true,
-		},
-		{
-			"success - crossing hellos: valid fee middleware",
-			string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: ibcmock.Version})),
-			true,
 			true,
 		},
 		{
 			"invalid fee middleware version",
 			string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: "invalid-ics29-1", AppVersion: ibcmock.Version})),
 			false,
-			false,
 		},
 		{
 			"invalid mock version",
 			string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: "invalid-mock-version"})),
 			false,
-			false,
 		},
 	}
 
@@ -201,14 +190,9 @@ func (suite *FeeTestSuite) TestOnChanOpenTry() {
 				ok      bool
 				err     error
 			)
-			if tc.crossing {
-				suite.path.EndpointA.ChanOpenInit()
-				chanCap, ok = suite.chainA.GetSimApp().ScopedFeeMockKeeper.GetCapability(suite.chainA.GetContext(), host.ChannelCapabilityPath(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID))
-				suite.Require().True(ok)
-			} else {
-				chanCap, err = suite.chainA.App.GetScopedIBCKeeper().NewCapability(suite.chainA.GetContext(), host.ChannelCapabilityPath(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID))
-				suite.Require().NoError(err)
-			}
+
+			chanCap, err = suite.chainA.App.GetScopedIBCKeeper().NewCapability(suite.chainA.GetContext(), host.ChannelCapabilityPath(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID))
+			suite.Require().NoError(err)
 
 			suite.path.EndpointA.ChannelID = ibctesting.FirstChannelID
 

modules/apps/transfer/ibc_module.go

@@ -111,15 +111,9 @@ func (im IBCModule) OnChanOpenTry(
 		return "", sdkerrors.Wrapf(types.ErrInvalidVersion, "invalid counterparty version: got: %s, expected %s", counterpartyVersion, types.Version)
 	}
 
-	// Module may have already claimed capability in OnChanOpenInit in the case of crossing hellos
-	// (ie chainA and chainB both call ChanOpenInit before one of them calls ChanOpenTry)
-	// If module can already authenticate the capability then module already owns it so we don't need to claim
-	// Otherwise, module does not have channel capability and we must claim it from IBC
-	if !im.keeper.AuthenticateCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)) {
-		// Only claim channel capability passed back by IBC module if we do not already own it
-		if err := im.keeper.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil {
-			return "", err
-		}
+	// OpenTry must claim the channelCapability that IBC passes into the callback
+	if err := im.keeper.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil {
+		return "", err
 	}
 
 	return types.Version, nil

modules/apps/transfer/ibc_module_test.go

@@ -124,10 +124,10 @@ func (suite *TransferTestSuite) TestOnChanOpenTry() {
 			}, false,
 		},
 		{
-			"capability already claimed in INIT should pass", func() {
+			"capability already claimed", func() {
 				err := suite.chainA.GetSimApp().ScopedTransferKeeper.ClaimCapability(suite.chainA.GetContext(), chanCap, host.ChannelCapabilityPath(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID))
 				suite.Require().NoError(err)
-			}, true,
+			}, false,
 		},
 		{
 			"invalid order - ORDERED", func() {

modules/core/04-channel/keeper/handshake.go

@@ -98,51 +98,20 @@ func (k Keeper) ChanOpenTry(
 	ctx sdk.Context,
 	order types.Order,
 	connectionHops []string,
-	portID,
-	previousChannelID string,
+	portID string,
 	portCap *capabilitytypes.Capability,
 	counterparty types.Counterparty,
 	counterpartyVersion string,
 	proofInit []byte,
 	proofHeight exported.Height,
 ) (string, *capabilitytypes.Capability, error) {
-	var (
-		previousChannel      types.Channel
-		previousChannelFound bool
-	)
-
-	channelID := previousChannelID
-
 	// connection hops only supports a single connection
 	if len(connectionHops) != 1 {
 		return "", nil, sdkerrors.Wrapf(types.ErrTooManyConnectionHops, "expected 1, got %d", len(connectionHops))
 	}
 
-	// empty channel identifier indicates continuing a previous channel handshake
-	if previousChannelID != "" {
-		// channel identifier and connection hop length checked on msg.ValidateBasic()
-		// ensure that the previous channel exists
-		previousChannel, previousChannelFound = k.GetChannel(ctx, portID, previousChannelID)
-		if !previousChannelFound {
-			return "", nil, sdkerrors.Wrapf(types.ErrInvalidChannel, "previous channel does not exist for supplied previous channelID %s", previousChannelID)
-		}
-		// previous channel must use the same fields
-		if !(previousChannel.Ordering == order &&
-			previousChannel.Counterparty.PortId == counterparty.PortId &&
-			previousChannel.Counterparty.ChannelId == "" &&
-			previousChannel.ConnectionHops[0] == connectionHops[0] && // ChanOpenInit will only set a single connection hop
-			previousChannel.Version == counterpartyVersion) {
-			return "", nil, sdkerrors.Wrap(types.ErrInvalidChannel, "channel fields mismatch previous channel fields")
-		}
-
-		if previousChannel.State != types.INIT {
-			return "", nil, sdkerrors.Wrapf(types.ErrInvalidChannelState, "previous channel state is in %s, expected INIT", previousChannel.State)
-		}
-
-	} else {
-		// generate a new channel
-		channelID = k.GenerateChannelIdentifier(ctx)
-	}
+	// generate a new channel
+	channelID := k.GenerateChannelIdentifier(ctx)
 
 	if !k.portKeeper.Authenticate(ctx, portCap, portID) {
 		return "", nil, sdkerrors.Wrapf(porttypes.ErrInvalidPort, "caller does not own port capability for port ID %s", portID)
@@ -199,20 +168,9 @@ func (k Keeper) ChanOpenTry(
 		err    error
 	)
 
-	if !previousChannelFound {
-		capKey, err = k.scopedKeeper.NewCapability(ctx, host.ChannelCapabilityPath(portID, channelID))
-		if err != nil {
-			return "", nil, sdkerrors.Wrapf(err, "could not create channel capability for port ID %s and channel ID %s", portID, channelID)
-		}
-
-	} else {
-		// capability initialized in ChanOpenInit
-		capKey, found = k.scopedKeeper.GetCapability(ctx, host.ChannelCapabilityPath(portID, channelID))
-		if !found {
-			return "", nil, sdkerrors.Wrapf(types.ErrChannelCapabilityNotFound,
-				"capability not found for existing channel, portID (%s) channelID (%s)", portID, channelID,
-			)
-		}
+	capKey, err = k.scopedKeeper.NewCapability(ctx, host.ChannelCapabilityPath(portID, channelID))
+	if err != nil {
+		return "", nil, sdkerrors.Wrapf(err, "could not create channel capability for port ID %s and channel ID %s", portID, channelID)
 	}
 
 	return channelID, capKey, nil
@@ -230,18 +188,15 @@ func (k Keeper) WriteOpenTryChannel(
 	counterparty types.Counterparty,
 	version string,
 ) {
-	previousChannel, previousChannelFound := k.GetChannel(ctx, portID, channelID)
-	if !previousChannelFound {
-		k.SetNextSequenceSend(ctx, portID, channelID, 1)
-		k.SetNextSequenceRecv(ctx, portID, channelID, 1)
-		k.SetNextSequenceAck(ctx, portID, channelID, 1)
-	}
+	k.SetNextSequenceSend(ctx, portID, channelID, 1)
+	k.SetNextSequenceRecv(ctx, portID, channelID, 1)
+	k.SetNextSequenceAck(ctx, portID, channelID, 1)
 
 	channel := types.NewChannel(types.TRYOPEN, order, counterparty, connectionHops, version)
 
 	k.SetChannel(ctx, portID, channelID, channel)
 
-	k.Logger(ctx).Info("channel state updated", "port-id", portID, "channel-id", channelID, "previous-state", previousChannel.State.String(), "new-state", "TRYOPEN")
+	k.Logger(ctx).Info("channel state updated", "port-id", portID, "channel-id", channelID, "previous-state", "NONE", "new-state", "TRYOPEN")
 
 	defer func() {
 		telemetry.IncrCounter(1, "ibc", "channel", "open-try")
@@ -267,11 +222,8 @@ func (k Keeper) ChanOpenAck(
 		return sdkerrors.Wrapf(types.ErrChannelNotFound, "port ID (%s) channel ID (%s)", portID, channelID)
 	}
 
-	if !(channel.State == types.INIT || channel.State == types.TRYOPEN) {
-		return sdkerrors.Wrapf(
-			types.ErrInvalidChannelState,
-			"channel state should be INIT or TRYOPEN (got %s)", channel.State.String(),
-		)
+	if channel.State != types.INIT {
+		return sdkerrors.Wrapf(types.ErrInvalidChannelState, "channel state should be INIT (got %s)", channel.State.String())
 	}
 
 	if !k.scopedKeeper.AuthenticateCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)) {

modules/core/04-channel/keeper/handshake_test.go

@@ -143,10 +143,9 @@ func (suite *KeeperTestSuite) TestChanOpenInit() {
 // ChanOpenTry can succeed.
 func (suite *KeeperTestSuite) TestChanOpenTry() {
 	var (
-		path              *ibctesting.Path
-		previousChannelID string
-		portCap           *capabilitytypes.Capability
-		heightDiff        uint64
+		path       *ibctesting.Path
+		portCap    *capabilitytypes.Capability
+		heightDiff uint64
 	)
 
 	testCases := []testCase{
@@ -158,34 +157,6 @@ func (suite *KeeperTestSuite) TestChanOpenTry() {
 			suite.chainB.CreatePortCapability(suite.chainB.GetSimApp().ScopedIBCMockKeeper, ibctesting.MockPort)
 			portCap = suite.chainB.GetPortCapability(ibctesting.MockPort)
 		}, true},
-		{"success with crossing hello", func() {
-			suite.coordinator.SetupConnections(path)
-			path.SetChannelOrdered()
-			err := suite.coordinator.ChanOpenInitOnBothChains(path)
-			suite.Require().NoError(err)
-
-			previousChannelID = path.EndpointB.ChannelID
-			portCap = suite.chainB.GetPortCapability(ibctesting.MockPort)
-		}, true},
-		{"previous channel with invalid version, crossing hello", func() {
-			suite.coordinator.SetupConnections(path)
-			path.SetChannelOrdered()
-
-			// modify channel version
-			path.EndpointA.ChannelConfig.Version = "invalid version"
-
-			err := suite.coordinator.ChanOpenInitOnBothChains(path)
-			suite.Require().NoError(err)
-
-			previousChannelID = path.EndpointB.ChannelID
-			portCap = suite.chainB.GetPortCapability(ibctesting.MockPort)
-		}, false},
-		{"previous channel with invalid state", func() {
-			suite.coordinator.SetupConnections(path)
-
-			// make previous channel have wrong ordering
-			path.EndpointA.ChanOpenInit()
-		}, false},
 		{"connection doesn't exist", func() {
 			path.EndpointA.ConnectionID = ibctesting.FirstConnectionID
 			path.EndpointB.ConnectionID = ibctesting.FirstConnectionID
@@ -268,7 +239,6 @@ func (suite *KeeperTestSuite) TestChanOpenTry() {
 		suite.Run(fmt.Sprintf("Case %s", tc.msg), func() {
 			suite.SetupTest() // reset
 			heightDiff = 0    // must be explicitly changed in malleate
-			previousChannelID = ""
 			path = ibctesting.NewPath(suite.chainA, suite.chainB)
 
 			tc.malleate()
@@ -286,7 +256,7 @@ func (suite *KeeperTestSuite) TestChanOpenTry() {
 
 			channelID, cap, err := suite.chainB.App.GetIBCKeeper().ChannelKeeper.ChanOpenTry(
 				suite.chainB.GetContext(), types.ORDERED, []string{path.EndpointB.ConnectionID},
-				path.EndpointB.ChannelConfig.PortID, previousChannelID, portCap, counterparty, path.EndpointA.ChannelConfig.Version,
+				path.EndpointB.ChannelConfig.PortID, portCap, counterparty, path.EndpointA.ChannelConfig.Version,
 				proof, malleateHeight(proofHeight, heightDiff),
 			)
 
@@ -352,7 +322,7 @@ func (suite *KeeperTestSuite) TestChanOpenAck() {
 			channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
 		}, true},
 		{"channel doesn't exist", func() {}, false},
-		{"channel state is not INIT or TRYOPEN", func() {
+		{"channel state is not INIT", func() {
 			// create fully open channels on both chains
 			suite.coordinator.Setup(path)
 			channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)