From 83f2c75f3d07776ee033e9ae70e3af576cb2bbfd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?colin=20axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Tue, 22 Sep 2020 15:47:43 +0200 Subject: [PATCH] connection handshake updates for versioning (#7328) * update handshake to match spec * add test for version * add test for open try * add more tests Add a test for supplied versions that do not function with previous connection states in INIT and TRYOPEN * update godoc * update version checks to switch Update the version checks in ConnOpenAck to use a switch to increase readability of the code as well as provide custom error messages for each possible case that may occur. Slighly modified version to review suggestion by @fedekunze * add intersection comments to handshake and pick version * remove old code --- x/ibc/03-connection/keeper/handshake.go | 45 +++++++------ x/ibc/03-connection/keeper/handshake_test.go | 70 +++++++++++++++++++- x/ibc/03-connection/types/version.go | 24 +++++++ x/ibc/03-connection/types/version_test.go | 34 ++++++++++ 4 files changed, 149 insertions(+), 24 deletions(-) diff --git a/x/ibc/03-connection/keeper/handshake.go b/x/ibc/03-connection/keeper/handshake.go index a4acc5801d..12b874d36a 100644 --- a/x/ibc/03-connection/keeper/handshake.go +++ b/x/ibc/03-connection/keeper/handshake.go @@ -3,6 +3,7 @@ package keeper import ( "bytes" "fmt" + "reflect" sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" @@ -82,7 +83,8 @@ func (k Keeper) ConnOpenTry( expectedConnection := types.NewConnectionEnd(types.INIT, counterparty.ClientId, expectedCounterparty, counterpartyVersions) // chain B picks a version from Chain A's available versions that is compatible - // with the supported IBC versions + // with Chain B's supported IBC versions. PickVersion will select the intersection + // of the supported versions and the counterparty versions. version, err := types.PickVersion(counterpartyVersions) if err != nil { return err @@ -111,16 +113,17 @@ func (k Keeper) ConnOpenTry( return err } - // If connection already exists for connectionID, ensure that the existing connection's counterparty - // is chainA and connection is on INIT stage - // Check that existing connection version is on desired version of current handshake + // If connection already exists for connectionID, 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. previousConnection, found := k.GetConnection(ctx, connectionID) if found && !(previousConnection.State == types.INIT && previousConnection.Counterparty.ConnectionId == counterparty.ConnectionId && bytes.Equal(previousConnection.Counterparty.Prefix.Bytes(), counterparty.Prefix.Bytes()) && previousConnection.ClientId == clientID && previousConnection.Counterparty.ClientId == counterparty.ClientId && - previousConnection.Versions[0] == version) { + reflect.DeepEqual(previousConnection.Versions, types.GetCompatibleEncodedVersions())) { return sdkerrors.Wrap(types.ErrInvalidConnection, "cannot relay connection attempt") } @@ -164,31 +167,29 @@ func (k Keeper) ConnOpenAck( return sdkerrors.Wrap(types.ErrConnectionNotFound, connectionID) } - // Check connection on ChainA is on correct state: INIT or TRYOPEN - if connection.State != types.INIT && connection.State != types.TRYOPEN { + // 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(), ) - } - version, err := types.DecodeVersion(encodedVersion) - if err != nil { - return sdkerrors.Wrap(err, "version negotiation failed") - } - - // Check that ChainB's proposed version identifier is supported by chainA - supportedVersion, found := types.FindSupportedVersion(version, types.GetCompatibleVersions()) - if !found { + // if the connection is INIT then the provided version must be supproted + case connection.State == types.INIT && !types.IsSupportedVersion(encodedVersion): return sdkerrors.Wrapf( - types.ErrVersionNegotiationFailed, - "connection version provided (%s) is not supported (%s)", version, types.GetCompatibleVersions(), + types.ErrInvalidConnectionState, + "connection state is in INIT but the provided encoded version is not supported %s", encodedVersion, ) - } - // Check that ChainB's proposed feature set is supported by chainA - if err := supportedVersion.VerifyProposedVersion(version); err != nil { - return err + // if the connection is in TRYOPEN then the encoded version must be the only set version in the + // retreived connection state. + case connection.State == types.TRYOPEN && (len(connection.Versions) != 1 || connection.Versions[0] != encodedVersion): + return sdkerrors.Wrapf( + types.ErrInvalidConnectionState, + "connection state is in TRYOPEN but the provided encoded version (%s) is not set in the previous connection %s", encodedVersion, connection, + ) } // validate client parameters of a chainA client stored on chainB diff --git a/x/ibc/03-connection/keeper/handshake_test.go b/x/ibc/03-connection/keeper/handshake_test.go index 1cd5947421..955db4d722 100644 --- a/x/ibc/03-connection/keeper/handshake_test.go +++ b/x/ibc/03-connection/keeper/handshake_test.go @@ -180,9 +180,24 @@ func (suite *KeeperTestSuite) TestConnOpenTry() { {"invalid previous connection is in TRYOPEN", func() { clientA, clientB = suite.coordinator.SetupClients(suite.chainA, suite.chainB, exported.Tendermint) + // open init chainA + connA, connB, err := suite.coordinator.ConnOpenInit(suite.chainA, suite.chainB, clientA, clientB) + suite.Require().NoError(err) + + // open try chainB + err = suite.coordinator.ConnOpenTry(suite.chainB, suite.chainA, connB, connA) + suite.Require().NoError(err) + + err = suite.coordinator.UpdateClient(suite.chainB, suite.chainA, clientB, exported.Tendermint) + suite.Require().NoError(err) + // retrieve client state of chainA to pass as counterpartyClient counterpartyClient = suite.chainA.GetClientState(clientA) + }, false}, + {"invalid previous connection has invalid versions", func() { + clientA, clientB = suite.coordinator.SetupClients(suite.chainA, suite.chainB, exported.Tendermint) + // open init chainA connA, connB, err := suite.coordinator.ConnOpenInit(suite.chainA, suite.chainB, clientA, clientB) suite.Require().NoError(err) @@ -190,6 +205,21 @@ func (suite *KeeperTestSuite) TestConnOpenTry() { // open try chainB err = suite.coordinator.ConnOpenTry(suite.chainB, suite.chainA, connB, connA) suite.Require().NoError(err) + + // modify connB to be in INIT with incorrect versions + connection, found := suite.chainB.App.IBCKeeper.ConnectionKeeper.GetConnection(suite.chainB.GetContext(), connB.ID) + suite.Require().True(found) + + connection.State = types.INIT + connection.Versions = []string{"invalid version"} + + suite.chainB.App.IBCKeeper.ConnectionKeeper.SetConnection(suite.chainB.GetContext(), connB.ID, connection) + + err = suite.coordinator.UpdateClient(suite.chainB, suite.chainA, clientB, exported.Tendermint) + suite.Require().NoError(err) + + // retrieve client state of chainA to pass as counterpartyClient + counterpartyClient = suite.chainA.GetClientState(clientA) }, false}, } @@ -212,8 +242,7 @@ func (suite *KeeperTestSuite) TestConnOpenTry() { if consensusHeight.IsZero() { // retrieve consensus state height to provide proof for - clientState := suite.chainA.GetClientState(clientA) - consensusHeight = clientState.GetLatestHeight() + consensusHeight = counterpartyClient.GetLatestHeight() } consensusKey := host.FullKeyClientPath(clientA, host.KeyConsensusState(consensusHeight)) proofConsensus, _ := suite.chainA.QueryProof(consensusKey) @@ -338,6 +367,43 @@ func (suite *KeeperTestSuite) TestConnOpenAck() { err = suite.coordinator.ConnOpenAck(suite.chainA, suite.chainB, connA, connB) suite.Require().NoError(err) }, false}, + {"connection is in INIT but the proposed version is invalid", func() { + // chainA is in INIT, chainB is in TRYOPEN + clientA, clientB = suite.coordinator.SetupClients(suite.chainA, suite.chainB, exported.Tendermint) + connA, connB, err := suite.coordinator.ConnOpenInit(suite.chainA, suite.chainB, clientA, clientB) + suite.Require().NoError(err) + + // retrieve client state of chainB to pass as counterpartyClient + counterpartyClient = suite.chainB.GetClientState(clientB) + + err = suite.coordinator.ConnOpenTry(suite.chainB, suite.chainA, connB, connA) + suite.Require().NoError(err) + + version = "2.0" + }, false}, + {"connection is in TRYOPEN but the set version in the connection is invalid", func() { + // chainA is in TRYOPEN, chainB is in TRYOPEN + clientA, clientB = suite.coordinator.SetupClients(suite.chainA, suite.chainB, exported.Tendermint) + connB, connA, err := suite.coordinator.ConnOpenInit(suite.chainB, suite.chainA, clientB, clientA) + suite.Require().NoError(err) + + err = suite.coordinator.ConnOpenTry(suite.chainA, suite.chainB, connA, connB) + suite.Require().NoError(err) + + // set chainB to TRYOPEN + connection := suite.chainB.GetConnection(connB) + connection.State = types.TRYOPEN + suite.chainB.App.IBCKeeper.ConnectionKeeper.SetConnection(suite.chainB.GetContext(), connB.ID, connection) + + // update clientB so state change is committed + suite.coordinator.UpdateClient(suite.chainB, suite.chainA, clientB, exported.Tendermint) + suite.coordinator.UpdateClient(suite.chainA, suite.chainB, clientA, exported.Tendermint) + + // retrieve client state of chainB to pass as counterpartyClient + counterpartyClient = suite.chainB.GetClientState(clientB) + + version = "2.0" + }, false}, {"incompatible IBC versions", func() { clientA, clientB = suite.coordinator.SetupClients(suite.chainA, suite.chainB, exported.Tendermint) connA, connB, err := suite.coordinator.ConnOpenInit(suite.chainA, suite.chainB, clientA, clientB) diff --git a/x/ibc/03-connection/types/version.go b/x/ibc/03-connection/types/version.go index 6d6097c832..e6c13cec01 100644 --- a/x/ibc/03-connection/types/version.go +++ b/x/ibc/03-connection/types/version.go @@ -136,6 +136,27 @@ func GetCompatibleEncodedVersions() []string { return versions } +// IsSupportedVersion returns true if the proposed version has a matching version +// identifier and its entire feature set is supported or the version identifier +// supports an empty feature set. +func IsSupportedVersion(encodedProposedVersion string) bool { + proposedVersion, err := DecodeVersion(encodedProposedVersion) + if err != nil { + return false + } + + supportedVersion, found := FindSupportedVersion(proposedVersion, GetCompatibleVersions()) + if !found { + return false + } + + if err := supportedVersion.VerifyProposedVersion(proposedVersion); err != nil { + return false + } + + return true +} + // FindSupportedVersion returns the version with a matching version identifier // if it exists. The returned boolean is true if the version is found and // false otherwise. @@ -156,6 +177,9 @@ func FindSupportedVersion(version Version, supportedVersions []Version) (Version // not allowed for the chosen version identifier then the search for a // compatible version continues. This function is called in the ConnOpenTry // handshake procedure. +// +// CONTRACT: PickVersion must only provide a version that is in the +// intersection of the supported versions and the counterparty versions. func PickVersion(encodedCounterpartyVersions []string) (string, error) { counterpartyVersions, err := DecodeVersions(encodedCounterpartyVersions) if err != nil { diff --git a/x/ibc/03-connection/types/version_test.go b/x/ibc/03-connection/types/version_test.go index 7bbb55e143..a8e5aeeed3 100644 --- a/x/ibc/03-connection/types/version_test.go +++ b/x/ibc/03-connection/types/version_test.go @@ -59,6 +59,40 @@ func TestDecodeVersion(t *testing.T) { } } +func TestIsSupportedVersion(t *testing.T) { + testCases := []struct { + name string + version types.Version + expPass bool + }{ + { + "version is supported", + types.GetCompatibleVersions()[0], + true, + }, + { + "version is not supported", + types.Version{}, + false, + }, + { + "version feature is not supported", + types.NewVersion(types.DefaultIBCVersionIdentifier, []string{"ORDER_DAG"}), + false, + }, + } + + // test that a version that cannot be decoded does not pass + require.False(t, types.IsSupportedVersion("1.0")) + + for _, tc := range testCases { + encodedVersion, err := tc.version.Encode() + require.NoError(t, err) + + require.Equal(t, tc.expPass, types.IsSupportedVersion(encodedVersion)) + } +} + func TestFindSupportedVersion(t *testing.T) { testCases := []struct { name string