From 73757243c2d43c7f4541a587f987cb30e42b625a Mon Sep 17 00:00:00 2001 From: colin axner <25233464+colin-axner@users.noreply.github.com> Date: Fri, 3 Jul 2020 11:28:02 +0200 Subject: [PATCH] Update connection handshake version negotiation (#6534) * update version negotiation and add basic testing * add integration tests * update spec * apply @fedekunze suggestions * update to enforce connection validation checks * add test * update godoc and spec * small doc fix * update versioning to feature set * update version code and tests to specified version tuple * update docs/spec * merge and fix bug * Update x/ibc/03-connection/types/version.go Co-authored-by: Aditya * Apply suggestions from code review Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com> * add verify proposed feature set test * fix formatting bug * add safety check * merge tests into existing handshake tests Co-authored-by: Aditya Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com> --- x/ibc/03-connection/keeper/handshake.go | 22 +- x/ibc/03-connection/keeper/handshake_test.go | 34 ++- x/ibc/03-connection/types/connection.go | 6 +- x/ibc/03-connection/types/connection_test.go | 10 +- x/ibc/03-connection/types/errors.go | 2 + x/ibc/03-connection/types/genesis_test.go | 8 +- x/ibc/03-connection/types/msgs.go | 10 +- x/ibc/03-connection/types/msgs_test.go | 36 +-- x/ibc/03-connection/types/version.go | 244 ++++++++++++------- x/ibc/03-connection/types/version_test.go | 114 +++++++++ x/ibc/24-host/errors.go | 7 +- x/ibc/24-host/validate.go | 27 ++ x/ibc/24-host/validate_test.go | 28 +++ x/ibc/genesis_test.go | 2 +- x/ibc/spec/01_concepts.md | 39 +++ x/ibc/testing/chain.go | 11 +- x/ibc/testing/keys.go | 5 - 17 files changed, 462 insertions(+), 143 deletions(-) create mode 100644 x/ibc/03-connection/types/version_test.go delete mode 100644 x/ibc/testing/keys.go diff --git a/x/ibc/03-connection/keeper/handshake.go b/x/ibc/03-connection/keeper/handshake.go index 48aefed3f3..48658f65cc 100644 --- a/x/ibc/03-connection/keeper/handshake.go +++ b/x/ibc/03-connection/keeper/handshake.go @@ -71,7 +71,10 @@ func (k Keeper) ConnOpenTry( // chain B picks a version from Chain A's available versions that is compatible // with the supported IBC versions - version := types.PickVersion(counterpartyVersions, types.GetCompatibleVersions()) + version, err := types.PickVersion(counterpartyVersions) + if err != nil { + return err + } // connection defines chain B's ConnectionEnd connection := types.NewConnectionEnd(types.UNINITIALIZED, connectionID, clientID, counterparty, []string{version}) @@ -147,11 +150,20 @@ func (k Keeper) ConnOpenAck( ) } - // Check that ChainB's proposed version is one of chainA's accepted versions - if types.LatestVersion(connection.Versions) != version { + // Check that ChainB's proposed version number is supported by chainA + supportedVersion, found := types.FindSupportedVersion(version, types.GetCompatibleVersions()) + if !found { return sdkerrors.Wrapf( - sdkerrors.ErrInvalidVersion, - "connection version does't match provided one (%s ≠ %s)", types.LatestVersion(connection.Versions), version, + types.ErrVersionNegotiationFailed, + "connection version provided (%s) is not supported (%s)", version, types.GetCompatibleVersions(), + ) + } + + // Check that ChainB's proposed feature set is supported by chainA + if !types.VerifyProposedFeatureSet(version, supportedVersion) { + return sdkerrors.Wrapf( + types.ErrVersionNegotiationFailed, + "connection version feature set provided (%s) is not supported (%s)", version, types.GetCompatibleVersions(), ) } diff --git a/x/ibc/03-connection/keeper/handshake_test.go b/x/ibc/03-connection/keeper/handshake_test.go index 3e83c95807..724577f276 100644 --- a/x/ibc/03-connection/keeper/handshake_test.go +++ b/x/ibc/03-connection/keeper/handshake_test.go @@ -62,6 +62,7 @@ func (suite *KeeperTestSuite) TestConnOpenTry() { var ( clientA string clientB string + versions []string consensusHeight uint64 ) @@ -89,6 +90,20 @@ func (suite *KeeperTestSuite) TestConnOpenTry() { consensusHeight = 1 }, false}, + {"counterparty versions is empty", func() { + clientA, clientB = suite.coordinator.SetupClients(suite.chainA, suite.chainB, clientexported.Tendermint) + _, _, err := suite.coordinator.ConnOpenInit(suite.chainA, suite.chainB, clientA, clientB) + suite.Require().NoError(err) + + versions = nil + }, false}, + {"counterparty versions don't have a match", func() { + clientA, clientB = suite.coordinator.SetupClients(suite.chainA, suite.chainB, clientexported.Tendermint) + _, _, err := suite.coordinator.ConnOpenInit(suite.chainA, suite.chainB, clientA, clientB) + suite.Require().NoError(err) + + versions = []string{"(version won't match,[])"} + }, false}, {"connection state verification failed", func() { clientA, clientB = suite.coordinator.SetupClients(suite.chainA, suite.chainB, clientexported.Tendermint) // chainA connection not created @@ -125,8 +140,9 @@ func (suite *KeeperTestSuite) TestConnOpenTry() { tc := tc suite.Run(tc.msg, func() { - suite.SetupTest() // reset - consensusHeight = 0 // must be explicity changed in malleate + suite.SetupTest() // reset + consensusHeight = 0 // must be explicitly changed in malleate + versions = types.GetCompatibleVersions() // must be explicitly changed in malleate tc.malleate() @@ -149,7 +165,7 @@ func (suite *KeeperTestSuite) TestConnOpenTry() { err := suite.chainB.App.IBCKeeper.ConnectionKeeper.ConnOpenTry( suite.chainB.GetContext(), connB.ID, counterparty, clientB, - types.GetCompatibleVersions(), proofInit, proofConsensus, + versions, proofInit, proofConsensus, proofHeight, consensusHeight, ) @@ -238,7 +254,17 @@ func (suite *KeeperTestSuite) TestConnOpenAck() { suite.Require().NoError(err) // set version to a non-compatible version - version = "2.0" + version = "(2.0,[])" + }, false}, + {"empty version", func() { + clientA, clientB = suite.coordinator.SetupClients(suite.chainA, suite.chainB, clientexported.Tendermint) + connA, connB, err := suite.coordinator.ConnOpenInit(suite.chainA, suite.chainB, clientA, clientB) + suite.Require().NoError(err) + + err = suite.coordinator.ConnOpenTry(suite.chainB, suite.chainA, connB, connA) + suite.Require().NoError(err) + + version = "" }, false}, {"self consensus state not found", func() { clientA, clientB = suite.coordinator.SetupClients(suite.chainA, suite.chainB, clientexported.Tendermint) diff --git a/x/ibc/03-connection/types/connection.go b/x/ibc/03-connection/types/connection.go index 5406b60e3e..1abb34d687 100644 --- a/x/ibc/03-connection/types/connection.go +++ b/x/ibc/03-connection/types/connection.go @@ -1,8 +1,6 @@ package types import ( - "strings" - sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" "github.com/cosmos/cosmos-sdk/x/ibc/03-connection/exported" commitmentexported "github.com/cosmos/cosmos-sdk/x/ibc/23-commitment/exported" @@ -62,8 +60,8 @@ func (c ConnectionEnd) ValidateBasic() error { return sdkerrors.Wrap(sdkerrors.ErrInvalidVersion, "missing connection versions") } for _, version := range c.Versions { - if strings.TrimSpace(version) == "" { - return sdkerrors.Wrap(sdkerrors.ErrInvalidVersion, "version can't be blank") + if err := host.ConnectionVersionValidator(version); err != nil { + return err } } return c.Counterparty.ValidateBasic() diff --git a/x/ibc/03-connection/types/connection_test.go b/x/ibc/03-connection/types/connection_test.go index 0a6e63206c..a5d40f8e17 100644 --- a/x/ibc/03-connection/types/connection_test.go +++ b/x/ibc/03-connection/types/connection_test.go @@ -24,17 +24,17 @@ func TestConnectionValidateBasic(t *testing.T) { }{ { "valid connection", - types.ConnectionEnd{connectionID, clientID, []string{"1.0.0"}, types.INIT, types.Counterparty{clientID2, connectionID2, commitmenttypes.NewMerklePrefix([]byte("prefix"))}}, + types.ConnectionEnd{connectionID, clientID, []string{types.DefaultIBCVersion}, types.INIT, types.Counterparty{clientID2, connectionID2, commitmenttypes.NewMerklePrefix([]byte("prefix"))}}, true, }, { "invalid connection id", - types.ConnectionEnd{"(connectionIDONE)", clientID, []string{"1.0.0"}, types.INIT, types.Counterparty{clientID2, connectionID2, commitmenttypes.NewMerklePrefix([]byte("prefix"))}}, + types.ConnectionEnd{"(connectionIDONE)", clientID, []string{types.DefaultIBCVersion}, types.INIT, types.Counterparty{clientID2, connectionID2, commitmenttypes.NewMerklePrefix([]byte("prefix"))}}, false, }, { "invalid client id", - types.ConnectionEnd{connectionID, "(clientID1)", []string{"1.0.0"}, types.INIT, types.Counterparty{clientID2, connectionID2, commitmenttypes.NewMerklePrefix([]byte("prefix"))}}, + types.ConnectionEnd{connectionID, "(clientID1)", []string{types.DefaultIBCVersion}, types.INIT, types.Counterparty{clientID2, connectionID2, commitmenttypes.NewMerklePrefix([]byte("prefix"))}}, false, }, { @@ -44,12 +44,12 @@ func TestConnectionValidateBasic(t *testing.T) { }, { "invalid version", - types.ConnectionEnd{connectionID, clientID, []string{""}, types.INIT, types.Counterparty{clientID2, connectionID2, commitmenttypes.NewMerklePrefix([]byte("prefix"))}}, + types.ConnectionEnd{connectionID, clientID, []string{"1.0.0"}, types.INIT, types.Counterparty{clientID2, connectionID2, commitmenttypes.NewMerklePrefix([]byte("prefix"))}}, false, }, { "invalid counterparty", - types.ConnectionEnd{connectionID, clientID, []string{"1.0.0"}, types.INIT, types.Counterparty{clientID2, connectionID2, emptyPrefix}}, + types.ConnectionEnd{connectionID, clientID, []string{types.DefaultIBCVersion}, types.INIT, types.Counterparty{clientID2, connectionID2, emptyPrefix}}, false, }, } diff --git a/x/ibc/03-connection/types/errors.go b/x/ibc/03-connection/types/errors.go index facea9f003..41ff557637 100644 --- a/x/ibc/03-connection/types/errors.go +++ b/x/ibc/03-connection/types/errors.go @@ -13,4 +13,6 @@ var ( ErrInvalidConnectionState = sdkerrors.Register(SubModuleName, 6, "invalid connection state") ErrInvalidCounterparty = sdkerrors.Register(SubModuleName, 7, "invalid counterparty connection") ErrInvalidConnection = sdkerrors.Register(SubModuleName, 8, "invalid connection") + ErrInvalidVersion = sdkerrors.Register(SubModuleName, 9, "invalid connection version") + ErrVersionNegotiationFailed = sdkerrors.Register(SubModuleName, 10, "connection version negotiation failed") ) diff --git a/x/ibc/03-connection/types/genesis_test.go b/x/ibc/03-connection/types/genesis_test.go index a617dcd1d5..6c2c7ff7ab 100644 --- a/x/ibc/03-connection/types/genesis_test.go +++ b/x/ibc/03-connection/types/genesis_test.go @@ -26,7 +26,7 @@ func TestValidateGenesis(t *testing.T) { name: "valid genesis", genState: types.NewGenesisState( []types.ConnectionEnd{ - types.NewConnectionEnd(types.INIT, connectionID, clientID, types.Counterparty{clientID2, connectionID2, commitmenttypes.NewMerklePrefix([]byte("prefix"))}, []string{"1.0.0"}), + types.NewConnectionEnd(types.INIT, connectionID, clientID, types.Counterparty{clientID2, connectionID2, commitmenttypes.NewMerklePrefix([]byte("prefix"))}, []string{types.DefaultIBCVersion}), }, []types.ConnectionPaths{ {clientID, []string{host.ConnectionPath(connectionID)}}, @@ -38,7 +38,7 @@ func TestValidateGenesis(t *testing.T) { name: "invalid connection", genState: types.NewGenesisState( []types.ConnectionEnd{ - types.NewConnectionEnd(types.INIT, connectionID, "(CLIENTIDONE)", types.Counterparty{clientID, connectionID, commitmenttypes.NewMerklePrefix([]byte("prefix"))}, []string{"1.0.0"}), + types.NewConnectionEnd(types.INIT, connectionID, "(CLIENTIDONE)", types.Counterparty{clientID, connectionID, commitmenttypes.NewMerklePrefix([]byte("prefix"))}, []string{types.DefaultIBCVersion}), }, []types.ConnectionPaths{ {clientID, []string{host.ConnectionPath(connectionID)}}, @@ -50,7 +50,7 @@ func TestValidateGenesis(t *testing.T) { name: "invalid client id", genState: types.NewGenesisState( []types.ConnectionEnd{ - types.NewConnectionEnd(types.INIT, connectionID, clientID, types.Counterparty{clientID2, connectionID2, commitmenttypes.NewMerklePrefix([]byte("prefix"))}, []string{"1.0.0"}), + types.NewConnectionEnd(types.INIT, connectionID, clientID, types.Counterparty{clientID2, connectionID2, commitmenttypes.NewMerklePrefix([]byte("prefix"))}, []string{types.DefaultIBCVersion}), }, []types.ConnectionPaths{ {"(CLIENTIDONE)", []string{host.ConnectionPath(connectionID)}}, @@ -62,7 +62,7 @@ func TestValidateGenesis(t *testing.T) { name: "invalid path", genState: types.NewGenesisState( []types.ConnectionEnd{ - types.NewConnectionEnd(types.INIT, connectionID, clientID, types.Counterparty{clientID2, connectionID2, commitmenttypes.NewMerklePrefix([]byte("prefix"))}, []string{"1.0.0"}), + types.NewConnectionEnd(types.INIT, connectionID, clientID, types.Counterparty{clientID2, connectionID2, commitmenttypes.NewMerklePrefix([]byte("prefix"))}, []string{types.DefaultIBCVersion}), }, []types.ConnectionPaths{ {clientID, []string{connectionID}}, diff --git a/x/ibc/03-connection/types/msgs.go b/x/ibc/03-connection/types/msgs.go index 5f2adc9694..59bc9cdf52 100644 --- a/x/ibc/03-connection/types/msgs.go +++ b/x/ibc/03-connection/types/msgs.go @@ -1,8 +1,6 @@ package types import ( - "strings" - sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" commitmenttypes "github.com/cosmos/cosmos-sdk/x/ibc/23-commitment/types" @@ -105,8 +103,8 @@ func (msg MsgConnectionOpenTry) ValidateBasic() error { return sdkerrors.Wrap(sdkerrors.ErrInvalidVersion, "missing counterparty versions") } for _, version := range msg.CounterpartyVersions { - if strings.TrimSpace(version) == "" { - return sdkerrors.Wrap(sdkerrors.ErrInvalidVersion, "version can't be blank") + if err := host.ConnectionVersionValidator(version); err != nil { + return err } } if len(msg.ProofInit) == 0 { @@ -171,8 +169,8 @@ func (msg MsgConnectionOpenAck) ValidateBasic() error { if err := host.ConnectionIdentifierValidator(msg.ConnectionID); err != nil { return sdkerrors.Wrap(err, "invalid connection ID") } - if strings.TrimSpace(msg.Version) == "" { - return sdkerrors.Wrap(sdkerrors.ErrInvalidVersion, "version can't be blank") + if err := host.ConnectionVersionValidator(msg.Version); err != nil { + return err } if len(msg.ProofTry) == 0 { return sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "cannot submit an empty proof try") diff --git a/x/ibc/03-connection/types/msgs_test.go b/x/ibc/03-connection/types/msgs_test.go index 48ec2214a5..365d874899 100644 --- a/x/ibc/03-connection/types/msgs_test.go +++ b/x/ibc/03-connection/types/msgs_test.go @@ -103,18 +103,18 @@ func (suite *MsgTestSuite) TestNewMsgConnectionOpenTry() { signer, _ := sdk.AccAddressFromBech32("cosmos1ckgw5d7jfj7wwxjzs9fdrdev9vc8dzcw3n2lht") testMsgs := []*types.MsgConnectionOpenTry{ - types.NewMsgConnectionOpenTry("test/conn1", "clienttotesta", "connectiontotest", "clienttotest", prefix, []string{"1.0.0"}, suite.proof, suite.proof, 10, 10, signer), - types.NewMsgConnectionOpenTry("ibcconntest", "test/iris", "connectiontotest", "clienttotest", prefix, []string{"1.0.0"}, suite.proof, suite.proof, 10, 10, signer), - types.NewMsgConnectionOpenTry("ibcconntest", "clienttotesta", "ibc/test", "clienttotest", prefix, []string{"1.0.0"}, suite.proof, suite.proof, 10, 10, signer), - types.NewMsgConnectionOpenTry("ibcconntest", "clienttotesta", "connectiontotest", "test/conn1", prefix, []string{"1.0.0"}, suite.proof, suite.proof, 10, 10, signer), - types.NewMsgConnectionOpenTry("ibcconntest", "clienttotesta", "connectiontotest", "clienttotest", emptyPrefix, []string{"1.0.0"}, suite.proof, suite.proof, 10, 10, signer), + types.NewMsgConnectionOpenTry("test/conn1", "clienttotesta", "connectiontotest", "clienttotest", prefix, []string{types.DefaultIBCVersion}, suite.proof, suite.proof, 10, 10, signer), + types.NewMsgConnectionOpenTry("ibcconntest", "test/iris", "connectiontotest", "clienttotest", prefix, []string{types.DefaultIBCVersion}, suite.proof, suite.proof, 10, 10, signer), + types.NewMsgConnectionOpenTry("ibcconntest", "clienttotesta", "ibc/test", "clienttotest", prefix, []string{types.DefaultIBCVersion}, suite.proof, suite.proof, 10, 10, signer), + types.NewMsgConnectionOpenTry("ibcconntest", "clienttotesta", "connectiontotest", "test/conn1", prefix, []string{types.DefaultIBCVersion}, suite.proof, suite.proof, 10, 10, signer), + types.NewMsgConnectionOpenTry("ibcconntest", "clienttotesta", "connectiontotest", "clienttotest", emptyPrefix, []string{types.DefaultIBCVersion}, suite.proof, suite.proof, 10, 10, signer), types.NewMsgConnectionOpenTry("ibcconntest", "clienttotesta", "connectiontotest", "clienttotest", prefix, []string{}, suite.proof, suite.proof, 10, 10, signer), - types.NewMsgConnectionOpenTry("ibcconntest", "clienttotesta", "connectiontotest", "clienttotest", prefix, []string{"1.0.0"}, emptyProof, suite.proof, 10, 10, signer), - types.NewMsgConnectionOpenTry("ibcconntest", "clienttotesta", "connectiontotest", "clienttotest", prefix, []string{"1.0.0"}, suite.proof, emptyProof, 10, 10, signer), - types.NewMsgConnectionOpenTry("ibcconntest", "clienttotesta", "connectiontotest", "clienttotest", prefix, []string{"1.0.0"}, suite.proof, suite.proof, 0, 10, signer), - types.NewMsgConnectionOpenTry("ibcconntest", "clienttotesta", "connectiontotest", "clienttotest", prefix, []string{"1.0.0"}, suite.proof, suite.proof, 10, 0, signer), - types.NewMsgConnectionOpenTry("ibcconntest", "clienttotesta", "connectiontotest", "clienttotest", prefix, []string{"1.0.0"}, suite.proof, suite.proof, 10, 10, nil), - types.NewMsgConnectionOpenTry("ibcconntest", "clienttotesta", "connectiontotest", "clienttotest", prefix, []string{"1.0.0"}, suite.proof, suite.proof, 10, 10, signer), + types.NewMsgConnectionOpenTry("ibcconntest", "clienttotesta", "connectiontotest", "clienttotest", prefix, []string{types.DefaultIBCVersion}, emptyProof, suite.proof, 10, 10, signer), + types.NewMsgConnectionOpenTry("ibcconntest", "clienttotesta", "connectiontotest", "clienttotest", prefix, []string{types.DefaultIBCVersion}, suite.proof, emptyProof, 10, 10, signer), + types.NewMsgConnectionOpenTry("ibcconntest", "clienttotesta", "connectiontotest", "clienttotest", prefix, []string{types.DefaultIBCVersion}, suite.proof, suite.proof, 0, 10, signer), + types.NewMsgConnectionOpenTry("ibcconntest", "clienttotesta", "connectiontotest", "clienttotest", prefix, []string{types.DefaultIBCVersion}, suite.proof, suite.proof, 10, 0, signer), + types.NewMsgConnectionOpenTry("ibcconntest", "clienttotesta", "connectiontotest", "clienttotest", prefix, []string{types.DefaultIBCVersion}, suite.proof, suite.proof, 10, 10, nil), + types.NewMsgConnectionOpenTry("ibcconntest", "clienttotesta", "connectiontotest", "clienttotest", prefix, []string{types.DefaultIBCVersion}, suite.proof, suite.proof, 10, 10, signer), } var testCases = []struct { @@ -150,14 +150,14 @@ func (suite *MsgTestSuite) TestNewMsgConnectionOpenAck() { signer, _ := sdk.AccAddressFromBech32("cosmos1ckgw5d7jfj7wwxjzs9fdrdev9vc8dzcw3n2lht") testMsgs := []*types.MsgConnectionOpenAck{ - types.NewMsgConnectionOpenAck("test/conn1", suite.proof, suite.proof, 10, 10, "1.0.0", signer), - types.NewMsgConnectionOpenAck("ibcconntest", emptyProof, suite.proof, 10, 10, "1.0.0", signer), - types.NewMsgConnectionOpenAck("ibcconntest", suite.proof, emptyProof, 10, 10, "1.0.0", signer), - types.NewMsgConnectionOpenAck("ibcconntest", suite.proof, suite.proof, 0, 10, "1.0.0", signer), - types.NewMsgConnectionOpenAck("ibcconntest", suite.proof, suite.proof, 10, 0, "1.0.0", signer), + types.NewMsgConnectionOpenAck("test/conn1", suite.proof, suite.proof, 10, 10, types.DefaultIBCVersion, signer), + types.NewMsgConnectionOpenAck("ibcconntest", emptyProof, suite.proof, 10, 10, types.DefaultIBCVersion, signer), + types.NewMsgConnectionOpenAck("ibcconntest", suite.proof, emptyProof, 10, 10, types.DefaultIBCVersion, signer), + types.NewMsgConnectionOpenAck("ibcconntest", suite.proof, suite.proof, 0, 10, types.DefaultIBCVersion, signer), + types.NewMsgConnectionOpenAck("ibcconntest", suite.proof, suite.proof, 10, 0, types.DefaultIBCVersion, signer), types.NewMsgConnectionOpenAck("ibcconntest", suite.proof, suite.proof, 10, 10, "", signer), - types.NewMsgConnectionOpenAck("ibcconntest", suite.proof, suite.proof, 10, 10, "1.0.0", nil), - types.NewMsgConnectionOpenAck("ibcconntest", suite.proof, suite.proof, 10, 10, "1.0.0", signer), + types.NewMsgConnectionOpenAck("ibcconntest", suite.proof, suite.proof, 10, 10, types.DefaultIBCVersion, nil), + types.NewMsgConnectionOpenAck("ibcconntest", suite.proof, suite.proof, 10, 10, types.DefaultIBCVersion, signer), } var testCases = []struct { msg *types.MsgConnectionOpenAck diff --git a/x/ibc/03-connection/types/version.go b/x/ibc/03-connection/types/version.go index bf74a52dae..ee79267457 100644 --- a/x/ibc/03-connection/types/version.go +++ b/x/ibc/03-connection/types/version.go @@ -1,96 +1,174 @@ package types -// GetCompatibleVersions returns an ordered set of compatible IBC versions for the -// caller chain's connection end. +import ( + "fmt" + "strings" + + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" + host "github.com/cosmos/cosmos-sdk/x/ibc/24-host" +) + +var ( + // DefaultIBCVersion represents the latest supported version of IBC. + // The current version supports only ORDERED and UNORDERED channels. + DefaultIBCVersion = CreateVersionString("1", []string{"ORDERED", "UNORDERED"}) +) + +// GetCompatibleVersions returns a descending ordered set of compatible IBC +// versions for the caller chain's connection end. The latest supported +// version should be first element and the set should descend to the oldest +// supported version. func GetCompatibleVersions() []string { - return []string{"1.0.0"} + return []string{DefaultIBCVersion} } -// LatestVersion gets the latest version of a connection protocol +// CreateVersionString constructs a valid connection version given a +// version identifier and feature set. The format is written as: // -// CONTRACT: version array MUST be already sorted. -func LatestVersion(versions []string) string { - if len(versions) == 0 { - return "" - } - return versions[len(versions)-1] +// ([version_identifier],[feature_0,feature_1,feature_2...]) +// +// A connection version is considered invalid if it is not in this format +// or violates one of these rules: +// - the version identifier is empty or contains commas +// - a specified feature contains commas +func CreateVersionString(identifier string, featureSet []string) string { + return fmt.Sprintf("(%s,[%s])", identifier, strings.Join(featureSet, ",")) } -// PickVersion picks the counterparty latest version that is matches the list -// of compatible versions for the connection. -func PickVersion(counterpartyVersions, compatibleVersions []string) string { - - n := len(counterpartyVersions) - m := len(compatibleVersions) - - // aux hash maps to lookup already seen versions - counterpartyVerLookup := make(map[string]bool) - compatibleVerLookup := make(map[string]bool) - - // versions suported - var supportedVersions []string - - switch { - case n == 0 || m == 0: - return "" - case n == m: - for i := n - 1; i >= 0; i-- { - counterpartyVerLookup[counterpartyVersions[i]] = true - compatibleVerLookup[compatibleVersions[i]] = true - - // check if we've seen any of the versions - if _, ok := compatibleVerLookup[counterpartyVersions[i]]; ok { - supportedVersions = append(supportedVersions, counterpartyVersions[i]) - } - - if _, ok := counterpartyVerLookup[compatibleVersions[i]]; ok { - // TODO: check if the version is already in the array - supportedVersions = append(supportedVersions, compatibleVersions[i]) - } - } - case n > m: - for i := n - 1; i >= m; i-- { - counterpartyVerLookup[compatibleVersions[i]] = true - } - - for i := m - 1; i >= 0; i-- { - counterpartyVerLookup[counterpartyVersions[i]] = true - compatibleVerLookup[compatibleVersions[i]] = true - - // check if we've seen any of the versions - if _, ok := compatibleVerLookup[counterpartyVersions[i]]; ok { - supportedVersions = append(supportedVersions, counterpartyVersions[i]) - } - - if _, ok := counterpartyVerLookup[compatibleVersions[i]]; ok { - supportedVersions = append(supportedVersions, compatibleVersions[i]) - } - } - - case n < m: - for i := m - 1; i >= n; i-- { - compatibleVerLookup[compatibleVersions[i]] = true - } - - for i := n - 1; i >= 0; i-- { - counterpartyVerLookup[counterpartyVersions[i]] = true - compatibleVerLookup[compatibleVersions[i]] = true - - // check if we've seen any of the versions - if _, ok := compatibleVerLookup[counterpartyVersions[i]]; ok { - supportedVersions = append(supportedVersions, counterpartyVersions[i]) - } - - if _, ok := counterpartyVerLookup[compatibleVersions[i]]; ok { - supportedVersions = append(supportedVersions, compatibleVersions[i]) - } - } +// UnpackVersion parses a version string and returns the identifier and the +// feature set of a version. An error is returned if the version is not valid. +func UnpackVersion(version string) (string, []string, error) { + // validate version so valid splitting assumptions can be made + if err := host.ConnectionVersionValidator(version); err != nil { + return "", nil, err } - if len(supportedVersions) == 0 { - return "" + // peel off prefix and suffix of the tuple + version = strings.TrimPrefix(version, "(") + version = strings.TrimSuffix(version, ")") + + // split into identifier and feature set + splitVersion := strings.SplitN(version, ",", 2) + if splitVersion[0] == version { + return "", nil, sdkerrors.Wrapf(ErrInvalidVersion, "failed to split version '%s' into identifier and features", version) + } + identifier := splitVersion[0] + + // peel off prefix and suffix of features + featureSet := strings.TrimPrefix(splitVersion[1], "[") + featureSet = strings.TrimSuffix(featureSet, "]") + + // check if features are empty + if len(featureSet) == 0 { + return identifier, []string{}, nil } - // TODO: compare latest version before appending - return supportedVersions[len(supportedVersions)-1] + features := strings.Split(featureSet, ",") + + return identifier, features, nil +} + +// 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. +func FindSupportedVersion(version string, supportedVersions []string) (string, bool) { + identifier, _, err := UnpackVersion(version) + if err != nil { + return "", false + } + + for _, supportedVersion := range supportedVersions { + supportedIdentifier, _, err := UnpackVersion(supportedVersion) + if err != nil { + continue + } + + if identifier == supportedIdentifier { + return supportedVersion, true + } + } + return "", false +} + +// PickVersion iterates over the descending ordered set of compatible IBC +// versions and selects the first version with a version identifier that is +// supported by the counterparty. The returned version contains a feature +// set with the intersection of the features supported by the source and +// counterparty chains. This function is called in the ConnOpenTry handshake +// procedure. +func PickVersion(counterpartyVersions []string) (string, error) { + versions := GetCompatibleVersions() + + for _, ver := range versions { + // check if the source version is supported by the counterparty + if counterpartyVer, found := FindSupportedVersion(ver, counterpartyVersions); found { + sourceIdentifier, sourceFeatures, err := UnpackVersion(ver) + if err != nil { + return "", err + } + + _, counterpartyFeatures, err := UnpackVersion(counterpartyVer) + if err != nil { + return "", err + } + + featureSet := GetFeatureSetIntersection(sourceFeatures, counterpartyFeatures) + + version := CreateVersionString(sourceIdentifier, featureSet) + return version, nil + } + } + + return "", sdkerrors.Wrapf( + ErrVersionNegotiationFailed, + "failed to find a matching counterparty version (%s) from the supported version list (%s)", counterpartyVersions, versions, + ) +} + +// GetFeatureSetIntersection returns the intersections of source feature set +// and the counterparty feature set. This is done by iterating over all the +// features in the source version and seeing if they exist in the feature +// set for the counterparty version. +func GetFeatureSetIntersection(sourceFeatureSet, counterpartyFeatureSet []string) (featureSet []string) { + for _, feature := range sourceFeatureSet { + if contains(feature, counterpartyFeatureSet) { + featureSet = append(featureSet, feature) + } + } + + return featureSet +} + +// VerifyProposedFeatureSet verifies that the entire feature set in the +// proposed version is supported by this chain. +func VerifyProposedFeatureSet(proposedVersion, supportedVersion string) bool { + _, proposedFeatureSet, err := UnpackVersion(proposedVersion) + if err != nil { + return false + } + + _, supportedFeatureSet, err := UnpackVersion(supportedVersion) + if err != nil { + return false + } + + for _, proposedFeature := range proposedFeatureSet { + if !contains(proposedFeature, supportedFeatureSet) { + return false + } + } + + return true +} + +// contains returns true if the provided string element exists within the +// string set. +func contains(elem string, set []string) bool { + for _, element := range set { + if strings.TrimSpace(elem) == strings.TrimSpace(element) { + return true + } + } + + return false } diff --git a/x/ibc/03-connection/types/version_test.go b/x/ibc/03-connection/types/version_test.go new file mode 100644 index 0000000000..611c5c9bab --- /dev/null +++ b/x/ibc/03-connection/types/version_test.go @@ -0,0 +1,114 @@ +package types_test + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/cosmos/cosmos-sdk/x/ibc/03-connection/types" +) + +// testing of invalid version formats exist within 24-host/validate_test.go +func TestUnpackVersion(t *testing.T) { + testCases := []struct { + name string + version string + expIdentifier string + expFeatures []string + expPass bool + }{ + {"valid version", "(1,[ORDERED channel,UNORDERED channel])", "1", []string{"ORDERED channel", "UNORDERED channel"}, true}, + {"valid empty features", "(1,[])", "1", []string{}, true}, + {"empty identifier", "(,[features])", "", []string{}, false}, + {"invalid version", "identifier,[features]", "", []string{}, false}, + {"empty string", " ", "", []string{}, false}, + } + + for _, tc := range testCases { + tc := tc + + t.Run(tc.name, func(t *testing.T) { + identifier, features, err := types.UnpackVersion(tc.version) + + if tc.expPass { + require.NoError(t, err) + require.Equal(t, tc.expIdentifier, identifier) + require.Equal(t, tc.expFeatures, features) + } else { + require.Error(t, err) + } + }) + } +} + +func TestFindSupportedVersion(t *testing.T) { + testCases := []struct { + name string + version string + supportedVersions []string + expVersion string + expFound bool + }{ + {"valid supported version", types.DefaultIBCVersion, types.GetCompatibleVersions(), types.DefaultIBCVersion, true}, + {"empty (invalid) version", "", types.GetCompatibleVersions(), "", false}, + {"empty supported versions", types.DefaultIBCVersion, []string{}, "", false}, + {"desired version is last", types.DefaultIBCVersion, []string{"(validversion,[])", "(2,[feature])", "(3,[])", types.DefaultIBCVersion}, types.DefaultIBCVersion, true}, + {"desired version identifier with different feature set", "(1,[features])", types.GetCompatibleVersions(), types.DefaultIBCVersion, true}, + {"version not supported", "(2,[DAG])", types.GetCompatibleVersions(), "", false}, + } + + for i, tc := range testCases { + version, found := types.FindSupportedVersion(tc.version, tc.supportedVersions) + + require.Equal(t, tc.expVersion, version, "test case %d: %s", i, tc.name) + require.Equal(t, tc.expFound, found, "test case %d: %s", i, tc.name) + } +} + +func TestPickVersion(t *testing.T) { + testCases := []struct { + name string + counterpartyVersions []string + expVer string + expPass bool + }{ + {"valid default ibc version", types.GetCompatibleVersions(), types.DefaultIBCVersion, true}, + {"valid version in counterparty versions", []string{"(version1,[])", "(2.0.0,[DAG,ZK])", types.DefaultIBCVersion}, types.DefaultIBCVersion, true}, + {"valid identifier match but empty feature set", []string{"(1,[DAG,ORDERED-ZK,UNORDERED-zk])"}, "(1,[])", true}, + {"empty counterparty versions", []string{}, "", false}, + {"non-matching counterparty versions", []string{"(2.0.0,[])"}, "", false}, + } + + for i, tc := range testCases { + version, err := types.PickVersion(tc.counterpartyVersions) + + if tc.expPass { + require.NoError(t, err, "valid test case %d failed: %s", i, tc.name) + require.Equal(t, tc.expVer, version, "valid test case %d falied: %s", i, tc.name) + } else { + require.Error(t, err, "invalid test case %d passed: %s", i, tc.name) + require.Equal(t, "", version, "invalid test case %d passed: %s", i, tc.name) + } + } +} + +func TestVerifyProposedFeatureSet(t *testing.T) { + testCases := []struct { + name string + proposedVersion string + supportedVersion string + expPass bool + }{ + {"entire feature set supported", types.DefaultIBCVersion, types.CreateVersionString("1", []string{"ORDERED", "UNORDERED", "DAG"}), true}, + {"empty feature sets", types.CreateVersionString("1", []string{}), types.DefaultIBCVersion, true}, + {"one feature missing", types.DefaultIBCVersion, types.CreateVersionString("1", []string{"UNORDERED", "DAG"}), false}, + {"both features missing", types.DefaultIBCVersion, types.CreateVersionString("1", []string{"DAG"}), false}, + } + + for i, tc := range testCases { + supported := types.VerifyProposedFeatureSet(tc.proposedVersion, tc.supportedVersion) + + require.Equal(t, tc.expPass, supported, "test case %d: %s", i, tc.name) + } + +} diff --git a/x/ibc/24-host/errors.go b/x/ibc/24-host/errors.go index fe8129bde8..3ec901625d 100644 --- a/x/ibc/24-host/errors.go +++ b/x/ibc/24-host/errors.go @@ -9,7 +9,8 @@ const SubModuleName = "host" // IBC client sentinel errors var ( - ErrInvalidID = sdkerrors.Register(SubModuleName, 2, "invalid identifier") - ErrInvalidPath = sdkerrors.Register(SubModuleName, 3, "invalid path") - ErrInvalidPacket = sdkerrors.Register(SubModuleName, 4, "invalid packet") + ErrInvalidID = sdkerrors.Register(SubModuleName, 2, "invalid identifier") + ErrInvalidPath = sdkerrors.Register(SubModuleName, 3, "invalid path") + ErrInvalidPacket = sdkerrors.Register(SubModuleName, 4, "invalid packet") + ErrInvalidVersion = sdkerrors.Register(SubModuleName, 5, "invalid version") ) diff --git a/x/ibc/24-host/validate.go b/x/ibc/24-host/validate.go index 0cf552eb7d..f01b06b279 100644 --- a/x/ibc/24-host/validate.go +++ b/x/ibc/24-host/validate.go @@ -14,6 +14,15 @@ import ( // - `[`, `]`, `<`, `>` var IsValidID = regexp.MustCompile(`^[a-zA-Z0-9\.\_\+\-\#\[\]\<\>]+$`).MatchString +// IsValidConnectionVersion defines the regular expression to check if the +// string is in the form of a tuple consisting of a string identifier and +// a set of features. The entire version tuple must be enclosed in parentheses. +// The version identifier must not contain any commas. The set of features +// must be enclosed in brackets and separated by commas. +// +// valid connection version = ([version_identifier], [feature_0, feature_1, etc]) +var IsValidConnectionVersion = regexp.MustCompile(`^\([^,]+\,\[([^,]+(\,[^,]+)*)?\]\)$`).MatchString + // ICS 024 Identifier and Path Validation Implementation // // This file defines ValidateFn to validate identifier and path strings @@ -74,6 +83,24 @@ func PortIdentifierValidator(id string) error { return defaultIdentifierValidator(id, 2, 20) } +// ConnectionVersionValidator is the default validator function for Connection +// versions. A valid version must be in semantic versioning form and contain +// only non-negative integers. +func ConnectionVersionValidator(version string) error { + if strings.TrimSpace(version) == "" { + return sdkerrors.Wrap(ErrInvalidVersion, "version cannot be blank") + } + + if !IsValidConnectionVersion(version) { + return sdkerrors.Wrapf( + ErrInvalidVersion, + "version '%s' must be in '(version_identifier,[feature_0, feature_1])' with no extra spacing", version, + ) + } + + return nil +} + // NewPathValidator takes in a Identifier Validator function and returns // a Path Validator function which requires path only has valid identifiers // alphanumeric character strings, and "/" separators diff --git a/x/ibc/24-host/validate_test.go b/x/ibc/24-host/validate_test.go index 417447fe03..7215d3a522 100644 --- a/x/ibc/24-host/validate_test.go +++ b/x/ibc/24-host/validate_test.go @@ -111,3 +111,31 @@ func TestCustomPathValidator(t *testing.T) { } } } + +func TestConnectionVersionValidator(t *testing.T) { + testCases := []testCase{ + {"valid connection version", "(my-test-version 1.0,[feature0, feature1])", true}, + {"valid random character version, no commas", "(a!@!#$%^&34,[)(*&^),....,feature_2])", true}, + {"valid: empty features", "(identifier,[])", true}, + {"invalid: empty features with spacing", "(identifier, [ ])", false}, + {"missing identifier", "( , [feature_0])", false}, + {"no features bracket", "(identifier, feature_0, feature_1)", false}, + {"no tuple parentheses", "identifier, [feature$%#]", false}, + {"string with only spaces", " ", false}, + {"empty string", "", false}, + {"no comma", "(idenitifer [features])", false}, + {"invalid comma usage in features", "(identifier, [feature_0,,feature_1])", false}, + {"empty features with comma", "(identifier, [ , ])", false}, + } + + for _, tc := range testCases { + + err := ConnectionVersionValidator(tc.id) + + if tc.expPass { + require.NoError(t, err, tc.msg) + } else { + require.Error(t, err, tc.msg) + } + } +} diff --git a/x/ibc/genesis_test.go b/x/ibc/genesis_test.go index 5f47a68703..40ce6ba206 100644 --- a/x/ibc/genesis_test.go +++ b/x/ibc/genesis_test.go @@ -47,7 +47,7 @@ func (suite *IBCTestSuite) TestValidateGenesis() { ), ConnectionGenesis: connectiontypes.NewGenesisState( []connectiontypes.ConnectionEnd{ - connectiontypes.NewConnectionEnd(connectiontypes.INIT, connectionID, clientID, connectiontypes.NewCounterparty(clientID2, connectionID2, commitmenttypes.NewMerklePrefix([]byte("prefix"))), []string{"1.0.0"}), + connectiontypes.NewConnectionEnd(connectiontypes.INIT, connectionID, clientID, connectiontypes.NewCounterparty(clientID2, connectionID2, commitmenttypes.NewMerklePrefix([]byte("prefix"))), []string{connectiontypes.DefaultIBCVersion}), }, []connectiontypes.ConnectionPaths{ connectiontypes.NewConnectionPaths(clientID, []string{host.ConnectionPath(connectionID)}), diff --git a/x/ibc/spec/01_concepts.md b/x/ibc/spec/01_concepts.md index 907893de27..486075a9b7 100644 --- a/x/ibc/spec/01_concepts.md +++ b/x/ibc/spec/01_concepts.md @@ -6,3 +6,42 @@ order: 1 > NOTE: if you are not familiar with the IBC terminology and concepts, please read this [document](https://github.com/cosmos/ics/blob/master/ibc/1_IBC_TERMINOLOGY.md) as prerequisite reading. + +### Connection Version Negotation + +During the handshake procedure for connections a version string is agreed +upon between the two parties. This occurs during the first 3 steps of the +handshake. + +During `ConnOpenInit`, party A is expected to set all the versions they wish +to support within their connection state. It is expected that this set of +versions is from most preferred to least preferred. This is not a strict +requirement for the SDK implementation of IBC because the party calling +`ConnOpenTry` will greedily select the latest version it supports that the +counterparty supports as well. + +During `ConnOpenTry`, party B will select a version from the counterparty's +supported versions. Priority will be placed on the latest supported version. +If a matching version cannot be found an error is returned. + +During `ConnOpenAck`, party A will verify that they can support the version +party B selected. If they do not support the selected version an error is +returned. After this step, the connection version is considered agreed upon. + +A valid connection version is considered to be in the following format: +`(version-identifier,[feature-0,feature-1])` + +- the version tuple must be enclosed in parentheses +- the feature set must be enclosed in brackets +- there should be no space between the comma separting the identifier and the +feature set +- the version identifier must no contain any commas +- each feature must not contain any commas +- each feature must be separated by commas + +::: warning +A set of versions should not contain two versions with the same +identifier, but differing feature sets. This will result in undefined behavior +with regards to version selection in `ConnOpenTry`. Each version in a set of +versions should have a unique version identifier. +::: diff --git a/x/ibc/testing/chain.go b/x/ibc/testing/chain.go index b352ae5203..1bf19d9107 100644 --- a/x/ibc/testing/chain.go +++ b/x/ibc/testing/chain.go @@ -37,9 +37,8 @@ const ( UnbondingPeriod time.Duration = time.Hour * 24 * 7 * 3 MaxClockDrift time.Duration = time.Second * 10 - ConnectionVersion = "1.0.0" - ChannelVersion = "ics20-1" - InvalidID = "IDisInvalid" + ChannelVersion = "ics20-1" + InvalidID = "IDisInvalid" ConnectionIDPrefix = "connectionid" @@ -49,6 +48,8 @@ const ( var ( DefaultTrustLevel tmmath.Fraction = lite.DefaultTrustLevel TestHash = []byte("TESTING HASH") + + ConnectionVersion = connectiontypes.DefaultIBCVersion ) // TestChain is a testing struct that wraps a simapp with the last TM Header, the current ABCI @@ -388,7 +389,7 @@ func (chain *TestChain) ConnectionOpenTry( require.True(chain.t, found) consensusHeight := consState.GetHeight() - consensusKey := prefixedClientKey(counterpartyConnection.ClientID, host.KeyConsensusState(consensusHeight)) + consensusKey := host.FullKeyClientPath(counterpartyConnection.ClientID, host.KeyConsensusState(consensusHeight)) proofConsensus, _ := counterparty.QueryProof(consensusKey) msg := connectiontypes.NewMsgConnectionOpenTry( @@ -415,7 +416,7 @@ func (chain *TestChain) ConnectionOpenAck( require.True(chain.t, found) consensusHeight := consState.GetHeight() - consensusKey := prefixedClientKey(counterpartyConnection.ClientID, host.KeyConsensusState(consensusHeight)) + consensusKey := host.FullKeyClientPath(counterpartyConnection.ClientID, host.KeyConsensusState(consensusHeight)) proofConsensus, _ := counterparty.QueryProof(consensusKey) msg := connectiontypes.NewMsgConnectionOpenAck( diff --git a/x/ibc/testing/keys.go b/x/ibc/testing/keys.go deleted file mode 100644 index 5d307f6a6a..0000000000 --- a/x/ibc/testing/keys.go +++ /dev/null @@ -1,5 +0,0 @@ -package testing - -func prefixedClientKey(clientID string, key []byte) []byte { - return append([]byte("clients/"+clientID+"/"), key...) -}