Skip to content

Commit

Permalink
Remove crossings hello (cosmos#1317) (cosmos#1692)
Browse files Browse the repository at this point in the history
* 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 <[email protected]>

* minor fixes && UPDATE CHANGELOG.md

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

Co-authored-by: Carlos Rodriguez <[email protected]>

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

Co-authored-by: Damian Nolan <[email protected]>

* 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 <[email protected]>
Co-authored-by: Carlos Rodriguez <[email protected]>
Co-authored-by: Damian Nolan <[email protected]>
Co-authored-by: Jacob Gadikian <[email protected]>
Co-authored-by: Colin Axnér <[email protected]>
(cherry picked from commit 7370a8b)

Co-authored-by: vuong <[email protected]>
  • Loading branch information
2 people authored and ulbqb committed Jul 31, 2023
1 parent db27ba4 commit c301e38
Show file tree
Hide file tree
Showing 13 changed files with 128 additions and 245 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand All @@ -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.

Expand Down
12 changes: 3 additions & 9 deletions docs/ibc/apps.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion docs/ibc/proto-docs.md
Original file line number Diff line number Diff line change
Expand Up @@ -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) | | |
Expand Down
10 changes: 10 additions & 0 deletions docs/migrations/v3-to-v4.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
Expand Down Expand Up @@ -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).
22 changes: 3 additions & 19 deletions modules/apps/29-fee/ibc_middleware_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
}

Expand Down Expand Up @@ -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

Expand Down
12 changes: 3 additions & 9 deletions modules/apps/transfer/ibc_module.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions modules/apps/transfer/ibc_module_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
72 changes: 12 additions & 60 deletions modules/core/04-channel/keeper/handshake.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand All @@ -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")
Expand All @@ -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)) {
Expand Down
40 changes: 5 additions & 35 deletions modules/core/04-channel/keeper/handshake_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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
Expand Down Expand Up @@ -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()
Expand All @@ -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),
)

Expand Down Expand Up @@ -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)
Expand Down
Loading

0 comments on commit c301e38

Please sign in to comment.