Skip to content

Commit

Permalink
refactor: remove crossing hellos from 03-connection (cosmos#1672)
Browse files Browse the repository at this point in the history 
* 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]>
  • Loading branch information
@colin-axner @AdityaSripal
colin-axner and AdityaSripal authored Jul 12, 2022
1 parent c449d85 commit 9aab42d
Show file tree
Hide file tree
Showing 11 changed files with 99 additions and 223 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
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/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).
Expand Down
2 changes: 1 addition & 1 deletion docs/ibc/proto-docs.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) | | |
Expand Down
7 changes: 6 additions & 1 deletion docs/migrations/v3-to-v4.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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).
79 changes: 13 additions & 66 deletions modules/core/03-connection/keeper/handshake.go
View file
Original file line number Diff line number Diff line change
@@ -1,12 +1,9 @@
package keeper

import (
"bytes"

"github.com/cosmos/cosmos-sdk/telemetry"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-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"
Expand All @@ -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")
}

Expand Down Expand Up @@ -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
Expand All @@ -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) {
Expand All @@ -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
}
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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,
)
}

Expand Down Expand Up @@ -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")
Expand Down
70 changes: 6 additions & 64 deletions modules/core/03-connection/keeper/handshake_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand All @@ -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},
}

Expand All @@ -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)

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

0 comments on commit 9aab42d

Please sign in to comment.