From ea76c13f6b43e82ff60b6257d0bed6278bdf1683 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 7 Jun 2023 14:16:30 +0200 Subject: [PATCH 01/18] refactor: add rest of the handler code --- modules/core/04-channel/keeper/upgrade.go | 82 ++++++++++++++++++----- 1 file changed, 65 insertions(+), 17 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 98d01fe10cb..597d56a951b 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -76,7 +76,7 @@ func (k Keeper) ChanUpgradeTry( channelID string, proposedConnectionHops []string, proposedUpgradeTimeout types.Timeout, - counterpartyProposedUpgrade types.Upgrade, + counterpartyUpgrade types.Upgrade, counterpartyUpgradeSequence uint64, proofCounterpartyChannel, proofCounterpartyUpgrade []byte, @@ -92,21 +92,21 @@ func (k Keeper) ChanUpgradeTry( return types.Upgrade{}, errorsmod.Wrapf(types.ErrInvalidChannelState, "expected one of [%s, %s], got %s", types.OPEN, types.INITUPGRADE, channel.State) } - connectionEnd, err := k.GetConnection(ctx, channel.ConnectionHops[0]) + connection, err := k.GetConnection(ctx, channel.ConnectionHops[0]) if err != nil { return types.Upgrade{}, errorsmod.Wrap(err, "failed to retrieve connection using the channel connection hops") } // make sure connection is OPEN - if connectionEnd.GetState() != int32(connectiontypes.OPEN) { + if connection.GetState() != int32(connectiontypes.OPEN) { return types.Upgrade{}, errorsmod.Wrapf( connectiontypes.ErrInvalidConnectionState, - "connection state is not OPEN (got %s)", connectiontypes.State(connectionEnd.GetState()).String(), + "connection state is not OPEN (got %s)", connectiontypes.State(connection.GetState()).String(), ) } // check if packet is timed out on the receiving chain - if hasPassed, err := counterpartyProposedUpgrade.Timeout.HasPassed(ctx); hasPassed { + if hasPassed, err := counterpartyUpgrade.Timeout.HasPassed(ctx); hasPassed { // abort here and let counterparty timeout the upgrade return types.Upgrade{}, errorsmod.Wrap(err, "upgrade timeout has passed") } @@ -114,23 +114,71 @@ func (k Keeper) ChanUpgradeTry( // assert that the proposed connection hops are compatible with the counterparty connection hops // the proposed connections hops must have a counterparty which matches the counterparty connection hops - // construct counterpartyChannel from existing information and provided counterpartyUpgradeSequence - // create upgrade fields from counterparty proposed upgrade and own verified connection hops + proposedUpgradeFields := types.UpgradeFields{ + Ordering: counterpartyUpgrade.Fields.Ordering, + ConnectionHops: proposedConnectionHops, + Version: counterpartyUpgrade.Fields.Version, + } - // if OPEN, then initialize handshake with upgradeFields - // this should validate the upgrade fields, set the upgrade path and set the final correct sequence. + var upgrade types.Upgrade - // otherwise, if the channel state is already in INITUPGRADE (crossing hellos case), - // assert that the upgrade fields are the same as the upgrade already in progress + switch channel.State { + case types.OPEN: + // initialize handshake with upgradeFields + // TODO: missing timeout in upgradeTry provided fields + upgrade, err = k.ChanUpgradeInit(ctx, portID, channelID, proposedUpgradeFields, upgradeTimeout) + if err != nil { + return types.Upgrade{}, errorsmod.Wrap(err, "failed to initialize upgrade") + } - // } else if channel.State == types.INITUPGRADE { + // NOTE: OnChanUpgradeInit will not be executed - // if the counterparty sequence is not equal to our own at this point, either the counterparty chain is out-of-sync or the message is out-of-sync - // we write an error receipt with our own sequence so that the counterparty can update their sequence as well. - // We must then increment our sequence so both sides start the next upgrade with a fresh sequence. - var proposedUpgrade types.Upgrade - return proposedUpgrade, nil + k.WriteUpgradeInitChannel(ctx, portID, channelID, upgrade) + + case types.INITUPGRADE: + // crossing hello's + // assert that the upgrade fields are the same as the upgrade already in progress + upgrade, found = k.GetUpgrade(ctx, portID, channelID) + if !found { + return types.Upgrade{}, errorsmod.Wrapf(types.ErrUpgradeNotFound, "current upgrade not found despite channel state being in %s", types.INITUPGRADE) + } + + if !reflect.DeepEqual(upgrade.Fields, proposedUpgradeFields) { + return types.Upgrade{}, errorsmod.Wrapf( + types.ErrInvalidUpgrade, "upgrade fields are not equal to current upgrade fields in crossing hello's case, expected %s", upgrade.Fields) + } + + default: + panic(fmt.Sprintf("channel state should be asserted to be in OPEN or INITUPGRADE before reaching this check; state is %s", channel.State)) + } + + // construct expected counterparty channel from information in state + // only the counterpartyUpgradeSequence is provided by the relayer + counterpartyConnectionHops := []string{connection.GetCounterparty().GetConnectionID()} + counterpartyChannel := types.Channel{ + State: types.INITUPGRADE, + Ordering: channel.Ordering, + Counterparty: types.NewCounterparty(portID, channelID), + ConnectionHops: counterpartyConnectionHops, + Version: channel.Version, + UpgradeSequence: counterpartyUpgradeSequence, // provided by the relayer + FlushStatus: types.NOTINFLUSH, + } + + if err := k.startFlushUpgradeHandshake( + ctx, + portID, channelID, + proposedUpgradeFields, + counterpartyChannel, + counterpartyUpgrade, + proofCounterpartyChannel, proofCounterpartyUpgrade, + proofHeight, + ); err != nil { + return types.Upgrade{}, err + } + + return upgrade, nil } // WriteUpgradeTryChannel writes a channel which has successfully passed the UpgradeTry handshake step. From a5d997e8b5af0e92cd1ad22070178488c01ba9a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 7 Jun 2023 16:13:20 +0200 Subject: [PATCH 02/18] fix: update upgrade tests --- modules/core/04-channel/keeper/upgrade.go | 2 +- .../core/04-channel/keeper/upgrade_test.go | 170 ++++++++---------- 2 files changed, 77 insertions(+), 95 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 0d60a78b908..4813fb15cf9 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -316,7 +316,7 @@ func (k Keeper) validateUpgradeFields(ctx sdk.Context, proposedUpgrade types.Upg currentFields := extractUpgradeFields(currentChannel) if reflect.DeepEqual(proposedUpgrade, currentFields) { - return errorsmod.Wrap(types.ErrChannelExists, "existing channel end is identical to proposed upgrade channel end") + return errorsmod.Wrapf(types.ErrChannelExists, "existing channel end is identical to proposed upgrade channel end: got %s", proposedUpgrade) } connectionID := proposedUpgrade.ConnectionHops[0] diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index a70c9c2ff1e..0973bf7e641 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -134,8 +134,8 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { var ( path *ibctesting.Path // expSequence uint64 - counterpartyUpgrade types.Upgrade - proposedConnectionHops []string + upgrade types.Upgrade + counterpartyUpgrade types.Upgrade ) testCases := []struct { @@ -148,8 +148,16 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { func() {}, true, }, + { + "success: crossing hellos", + func() { + err := path.EndpointB.ChanUpgradeInit() + suite.Require().NoError(err) + }, + true, + }, // { - // "success: counterparty upgrade sequence", + // "success: upgrade sequence is fast forwarded to counterparty upgrade sequence", // func() { // channel := path.EndpointA.GetChannel() // channel.UpgradeSequence = 5 @@ -175,9 +183,11 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { false, }, { - "timeout has passed", + "connection not found", func() { - counterpartyUpgrade.Timeout = types.NewTimeout(clienttypes.NewHeight(0, 1), 0) + channel := path.EndpointB.GetChannel() + channel.ConnectionHops = []string{"connection-100"} + path.EndpointB.SetChannel(channel) }, false, }, @@ -191,11 +201,62 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { false, }, { - "invalid connection hops", + "timeout has passed", func() { - channel := path.EndpointB.GetChannel() - channel.ConnectionHops = []string{"connection-100"} - path.EndpointB.SetChannel(channel) + counterpartyUpgrade.Timeout = types.NewTimeout(clienttypes.NewHeight(0, 1), 0) + }, + false, + }, + { + "initializing handshake fails, proposed connection hops do not exist", + func() { + upgrade.Fields.ConnectionHops = []string{ibctesting.InvalidID} + }, + false, + }, + { + "current upgrade not found even though channel is in INITUPGRADE", + func() { + suite.chainB.DeleteKey(host.ChannelUpgradeKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)) + }, + false, + }, + { + "proposed upgrade fields must be equal to existing upgrade fields in crossing hellos", + func() { + err := path.EndpointB.ChanUpgradeInit() + suite.Require().NoError(err) + + upgrade.Fields.ConnectionHops = []string{ibctesting.InvalidID} + }, + false, + }, + { + "startFlushUpgradeHandshake fails due to proof verification failure, counterparty upgrade connection hops are tampered with", + func() { + counterpartyUpgrade.Fields.ConnectionHops = []string{ibctesting.InvalidID} + }, + false, + }, + { + "startFlushUpgradeHandshake fails due to incompatible upgrades, chainB proposes a new connection hop that does not match counterparty", + func() { + // reuse existing connection to create a new connection in a non OPEN state + connection := path.EndpointB.GetConnection() + // ensure counterparty connectionID does not match connectionID set in counterparty proposed upgrade + connection.Counterparty.ConnectionId = "connection-50" + + // set proposed connection in state + proposedConnectionID := "connection-100" + suite.chainB.GetSimApp().GetIBCKeeper().ConnectionKeeper.SetConnection(suite.chainB.GetContext(), proposedConnectionID, connection) + upgrade.Fields.ConnectionHops[0] = proposedConnectionID + }, + false, + }, + { + "startFlushUpgradeHandshake fails due to mismatch in upgrade sequences", + func() { + }, false, }, @@ -205,19 +266,19 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { tc := tc suite.Run(tc.name, func() { suite.SetupTest() - path = ibctesting.NewPath(suite.chainA, suite.chainB) suite.coordinator.Setup(path) - path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = fmt.Sprintf("%s-v2", mock.Version) - + path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion err := path.EndpointA.ChanUpgradeInit() suite.Require().NoError(err) path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion - upgrade := path.EndpointB.GetProposedUpgrade() + upgrade = path.EndpointB.GetProposedUpgrade() - counterpartyUpgrade = path.EndpointA.GetProposedUpgrade() + var found bool + counterpartyUpgrade, found = path.EndpointA.Chain.GetSimApp().IBCKeeper.ChannelKeeper.GetUpgrade(path.EndpointA.Chain.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + suite.Require().True(found) // expSequence = 1 tc.malleate() @@ -231,7 +292,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, - proposedConnectionHops, + upgrade.Fields.ConnectionHops, upgrade.Timeout, counterpartyUpgrade, path.EndpointA.GetChannel().UpgradeSequence, @@ -250,85 +311,6 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { } } -func (suite *KeeperTestSuite) TestChanUpgradeTry_CrossingHellos() { - var ( - path *ibctesting.Path - // expSequence uint64 - upgrade types.Upgrade - counterpartyUpgrade types.Upgrade - counterpartyUpgradeSequence uint64 - ) - - testCases := []struct { - name string - malleate func() - expPass bool - }{ - { - "success", - func() {}, - true, - }, - // { - // "success: counterparty sequence > channel.UpgradeSequence", - // func() { - // counterpartyUpgradeSequence = 5 - // expSequence = 5 - // }, - // true, - // }, - // { - // "fail: upgrade fields have changed", - // func() { - // counterpartyUpgrade.Fields.Ordering = types.ORDERED - // counterpartyUpgrade.Fields.Version = fmt.Sprintf("%s-v3", mock.Version) - // }, - // false, - // }, - } - - for _, tc := range testCases { - tc := tc - suite.Run(tc.name, func() { - suite.SetupTest() - - path = ibctesting.NewPath(suite.chainA, suite.chainB) - suite.coordinator.Setup(path) - - path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion - err := path.EndpointA.ChanUpgradeInit() - suite.Require().NoError(err) - - // UpgradeInit on endpointB to simulate crossing hellos situation - path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion - err = path.EndpointB.ChanUpgradeInit() - suite.Require().NoError(err) - - counterpartyUpgrade = path.EndpointA.GetProposedUpgrade() - upgrade = path.EndpointB.GetProposedUpgrade() - - tc.malleate() - - // ensure clients are up to date to receive valid proofs - suite.Require().NoError(path.EndpointB.UpdateClient()) - - proofCounterpartyChannel, proofCounterpartyUpgrade, proofHeight := path.EndpointB.QueryChannelUpgradeProof() - - _, err = suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.ChanUpgradeTry( - suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, upgrade.Fields.ConnectionHops, upgrade.Timeout, - counterpartyUpgrade, counterpartyUpgradeSequence, proofCounterpartyChannel, proofCounterpartyUpgrade, proofHeight, - ) - - if tc.expPass { - suite.Require().NoError(err) - // suite.Require().Equal(expSequence, path.EndpointB.GetChannel().UpgradeSequence) - } else { - suite.Require().Error(err) - } - }) - } -} - // TestStartFlushUpgradeHandshake tests the startFlushUpgradeHandshake. // UpgradeInit will be run on chainA and startFlushUpgradeHandshake // will be called on chainB From d9a899978549023bef815a1f75e834ed188598e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 7 Jun 2023 16:14:38 +0200 Subject: [PATCH 03/18] fix: test fixes for upgradeTry --- testing/endpoint.go | 4 ++-- testing/mock/ibc_module.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/testing/endpoint.go b/testing/endpoint.go index 2f376a0ddb1..ce1249a891f 100644 --- a/testing/endpoint.go +++ b/testing/endpoint.go @@ -573,11 +573,11 @@ func (endpoint *Endpoint) QueryChannelUpgradeProof() ([]byte, []byte, clienttype // query proof for the channel on the counterparty channelKey := host.ChannelKey(counterpartyPortID, counterpartyChannelID) - proofChannel, height := endpoint.Counterparty.Chain.QueryProof(channelKey) + proofChannel, height := endpoint.Counterparty.QueryProof(channelKey) // query proof for the upgrade attempt on the counterparty upgradeKey := host.ChannelUpgradeKey(counterpartyPortID, counterpartyChannelID) - proofUpgrade, _ := endpoint.Counterparty.Chain.QueryProof(upgradeKey) + proofUpgrade, _ := endpoint.Counterparty.QueryProof(upgradeKey) return proofChannel, proofUpgrade, height } diff --git a/testing/mock/ibc_module.go b/testing/mock/ibc_module.go index af1475ad1aa..987f13d8f6f 100644 --- a/testing/mock/ibc_module.go +++ b/testing/mock/ibc_module.go @@ -164,7 +164,7 @@ func (im IBCModule) OnTimeoutPacket(ctx sdk.Context, packet channeltypes.Packet, // OnChanUpgradeInit implements the IBCModule interface func (im IBCModule) OnChanUpgradeInit(ctx sdk.Context, portID, channelID string, order channeltypes.Order, connectionHops []string, sequence uint64, version, previousVersion string) (string, error) { - return Version, nil + return UpgradeVersion, nil } // OnChanUpgradeTry implements the IBCModule interface From fe0095dbff82ab2895ae62eefe966e3b5b53e461 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 7 Jun 2023 16:22:50 +0200 Subject: [PATCH 04/18] fix: upgrade tests --- modules/core/04-channel/keeper/upgrade_test.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index 0973bf7e641..3c643a27b88 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -217,6 +217,10 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { { "current upgrade not found even though channel is in INITUPGRADE", func() { + // crossing hellos + err := path.EndpointB.ChanUpgradeInit() + suite.Require().NoError(err) + suite.chainB.DeleteKey(host.ChannelUpgradeKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)) }, false, @@ -224,6 +228,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { { "proposed upgrade fields must be equal to existing upgrade fields in crossing hellos", func() { + // crossing hellos err := path.EndpointB.ChanUpgradeInit() suite.Require().NoError(err) @@ -256,7 +261,9 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { { "startFlushUpgradeHandshake fails due to mismatch in upgrade sequences", func() { - + channel := path.EndpointB.GetChannel() + channel.UpgradeSequence = 5 + path.EndpointB.SetChannel(channel) }, false, }, From fa6598bb5153473a3874bf3f4ea1fef2b4f1a618 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 7 Jun 2023 16:41:31 +0200 Subject: [PATCH 05/18] imp: add error code checking to tests --- .../core/04-channel/keeper/upgrade_test.go | 64 ++++++++++--------- 1 file changed, 34 insertions(+), 30 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index 3c643a27b88..366cf1d1161 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -132,21 +132,20 @@ func (suite *KeeperTestSuite) TestChanUpgradeInit() { func (suite *KeeperTestSuite) TestChanUpgradeTry() { var ( - path *ibctesting.Path - // expSequence uint64 - upgrade types.Upgrade + path *ibctesting.Path + proposedUpgrade types.Upgrade counterpartyUpgrade types.Upgrade ) testCases := []struct { name string malleate func() - expPass bool + expError error }{ { "success", func() {}, - true, + nil, }, { "success: crossing hellos", @@ -154,7 +153,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { err := path.EndpointB.ChanUpgradeInit() suite.Require().NoError(err) }, - true, + nil, }, // { // "success: upgrade sequence is fast forwarded to counterparty upgrade sequence", @@ -173,14 +172,14 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { func() { path.EndpointB.ChannelID = ibctesting.InvalidID }, - false, + types.ErrChannelNotFound, }, { "channel state is not in OPEN or INITUPGRADE state", func() { suite.Require().NoError(path.EndpointB.SetChannelState(types.CLOSED)) }, - false, + types.ErrInvalidChannelState, }, { "connection not found", @@ -189,7 +188,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { channel.ConnectionHops = []string{"connection-100"} path.EndpointB.SetChannel(channel) }, - false, + connectiontypes.ErrConnectionNotFound, }, { "invalid connection state", @@ -198,21 +197,21 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { connectionEnd.State = connectiontypes.UNINITIALIZED suite.chainB.GetSimApp().GetIBCKeeper().ConnectionKeeper.SetConnection(suite.chainB.GetContext(), path.EndpointB.ConnectionID, connectionEnd) }, - false, + connectiontypes.ErrInvalidConnectionState, }, { "timeout has passed", func() { counterpartyUpgrade.Timeout = types.NewTimeout(clienttypes.NewHeight(0, 1), 0) }, - false, + types.ErrInvalidUpgrade, }, { "initializing handshake fails, proposed connection hops do not exist", func() { - upgrade.Fields.ConnectionHops = []string{ibctesting.InvalidID} + proposedUpgrade.Fields.ConnectionHops = []string{ibctesting.InvalidID} }, - false, + connectiontypes.ErrConnectionNotFound, }, { "current upgrade not found even though channel is in INITUPGRADE", @@ -223,7 +222,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { suite.chainB.DeleteKey(host.ChannelUpgradeKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)) }, - false, + types.ErrUpgradeNotFound, }, { "proposed upgrade fields must be equal to existing upgrade fields in crossing hellos", @@ -232,16 +231,16 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { err := path.EndpointB.ChanUpgradeInit() suite.Require().NoError(err) - upgrade.Fields.ConnectionHops = []string{ibctesting.InvalidID} + proposedUpgrade.Fields.ConnectionHops = []string{ibctesting.InvalidID} }, - false, + types.ErrInvalidUpgrade, }, { "startFlushUpgradeHandshake fails due to proof verification failure, counterparty upgrade connection hops are tampered with", func() { counterpartyUpgrade.Fields.ConnectionHops = []string{ibctesting.InvalidID} }, - false, + commitmenttypes.ErrInvalidProof, }, { "startFlushUpgradeHandshake fails due to incompatible upgrades, chainB proposes a new connection hop that does not match counterparty", @@ -254,9 +253,9 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { // set proposed connection in state proposedConnectionID := "connection-100" suite.chainB.GetSimApp().GetIBCKeeper().ConnectionKeeper.SetConnection(suite.chainB.GetContext(), proposedConnectionID, connection) - upgrade.Fields.ConnectionHops[0] = proposedConnectionID + proposedUpgrade.Fields.ConnectionHops[0] = proposedConnectionID }, - false, + types.NewUpgradeError(1, types.ErrIncompatibleCounterpartyUpgrade), }, { "startFlushUpgradeHandshake fails due to mismatch in upgrade sequences", @@ -265,7 +264,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { channel.UpgradeSequence = 5 path.EndpointB.SetChannel(channel) }, - false, + types.NewUpgradeError(6, types.ErrIncompatibleCounterpartyUpgrade), // max sequence + 1 will be returned }, } @@ -281,12 +280,11 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { suite.Require().NoError(err) path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion - upgrade = path.EndpointB.GetProposedUpgrade() + proposedUpgrade = path.EndpointB.GetProposedUpgrade() var found bool counterpartyUpgrade, found = path.EndpointA.Chain.GetSimApp().IBCKeeper.ChannelKeeper.GetUpgrade(path.EndpointA.Chain.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) suite.Require().True(found) - // expSequence = 1 tc.malleate() @@ -295,12 +293,12 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { proofCounterpartyChannel, proofCounterpartyUpgrade, proofHeight := path.EndpointB.QueryChannelUpgradeProof() - _, err = suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.ChanUpgradeTry( + upgrade, err := suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.ChanUpgradeTry( suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, - upgrade.Fields.ConnectionHops, - upgrade.Timeout, + proposedUpgrade.Fields.ConnectionHops, + proposedUpgrade.Timeout, counterpartyUpgrade, path.EndpointA.GetChannel().UpgradeSequence, proofCounterpartyChannel, @@ -308,11 +306,18 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { proofHeight, ) - if tc.expPass { - suite.Require().NoError(err) - // suite.Require().Equal(expSequence, path.EndpointB.GetChannel().UpgradeSequence) + if tc.expError != nil { + suite.assertUpgradeError(err, tc.expError) + suite.Require().Empty(upgrade) } else { - suite.Require().Error(err) + suite.Require().NoError(err) + suite.Require().NotEmpty(upgrade) + suite.Require().Equal(proposedUpgrade.Fields, upgrade.Fields) + suite.Require().Equal(proposedUpgrade.Timeout, upgrade.Timeout) + + latestSequenceSend, found := path.EndpointB.Chain.GetSimApp().IBCKeeper.ChannelKeeper.GetNextSequenceSend(path.EndpointB.Chain.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + suite.Require().True(found) + suite.Require().Equal(latestSequenceSend-1, upgrade.LatestSequenceSend) } }) } @@ -473,7 +478,6 @@ func (suite *KeeperTestSuite) TestStartFlushUpgradeHandshake() { ) if tc.expError != nil { - suite.Require().Error(err) suite.assertUpgradeError(err, tc.expError) } else { suite.Require().NoError(err) From 75b52f13a9bfac7ea028dcd7ad010c20a8d447d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 7 Jun 2023 16:47:34 +0200 Subject: [PATCH 06/18] chore: add todo --- modules/core/04-channel/keeper/upgrade.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 4813fb15cf9..489c1eb6270 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -126,6 +126,8 @@ func (k Keeper) ChanUpgradeTry( return types.Upgrade{}, errorsmod.Wrap(err, "failed to initialize upgrade") } + // TODO: add fast forward feature + // NOTE: OnChanUpgradeInit will not be executed k.WriteUpgradeInitChannel(ctx, portID, channelID, upgrade) From 89a8ce86434d3fd7d2107d9041b2b8e9d47c72e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?colin=20axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 7 Jun 2023 16:49:19 +0200 Subject: [PATCH 07/18] Update modules/core/04-channel/keeper/upgrade.go --- modules/core/04-channel/keeper/upgrade.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 9cd9e89a6b3..18dd613706e 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -120,7 +120,7 @@ func (k Keeper) ChanUpgradeTry( switch channel.State { case types.OPEN: - // initialize handshake with upgradeFields + // initialize handshake with upgrade fields upgrade, err = k.ChanUpgradeInit(ctx, portID, channelID, proposedUpgradeFields, upgradeTimeout) if err != nil { return types.Upgrade{}, errorsmod.Wrap(err, "failed to initialize upgrade") From e1a7d62dccbf9b8fc37b6299401cdeb48b0607f7 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 7 Jun 2023 17:57:07 +0200 Subject: [PATCH 08/18] adding msg server scaffolding for MsgChannelUpgradeTry --- modules/core/04-channel/keeper/upgrade.go | 4 ++ modules/core/keeper/msg_server.go | 57 ++++++++++++++++++++++- 2 files changed, 60 insertions(+), 1 deletion(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 18dd613706e..4e3a5f49093 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -203,6 +203,10 @@ func (k Keeper) WriteUpgradeTryChannel( emitChannelUpgradeTryEvent(ctx, portID, channelID, channel, proposedUpgrade) } +func (k Keeper) AbortUpgrade(ctx sdk.Context, portID, channelID string, err error) error { + return nil +} + // startFlushUpgradeHandshake will verify the counterparty proposed upgrade and the current channel state. // Once the counterparty information has been verified, it will be validated against the self proposed upgrade. // If any of the proposed upgrade fields are incompatible, an upgrade error will be returned resulting in an diff --git a/modules/core/keeper/msg_server.go b/modules/core/keeper/msg_server.go index e25f753e889..40b40ba0e60 100644 --- a/modules/core/keeper/msg_server.go +++ b/modules/core/keeper/msg_server.go @@ -751,7 +751,62 @@ func (k Keeper) ChannelUpgradeInit(goCtx context.Context, msg *channeltypes.MsgC // ChannelUpgradeTry defines a rpc handler method for MsgChannelUpgradeTry. func (k Keeper) ChannelUpgradeTry(goCtx context.Context, msg *channeltypes.MsgChannelUpgradeTry) (*channeltypes.MsgChannelUpgradeTryResponse, error) { - return nil, nil + ctx := sdk.UnwrapSDKContext(goCtx) + + module, _, err := k.ChannelKeeper.LookupModuleByChannel(ctx, msg.PortId, msg.ChannelId) + if err != nil { + ctx.Logger().Error("channel upgrade init failed", "port-id", msg.PortId, "error", errorsmod.Wrap(err, "could not retrieve module from port-id")) + return nil, errorsmod.Wrap(err, "could not retrieve module from port-id") + } + + cbs, ok := k.Router.GetRoute(module) + if !ok { + ctx.Logger().Error("channel upgrade init failed", "port-id", msg.PortId, "error", errorsmod.Wrapf(porttypes.ErrInvalidRoute, "route not found to module: %s", module)) + return nil, errorsmod.Wrapf(porttypes.ErrInvalidRoute, "route not found to module: %s", module) + } + + upgrade, err := k.ChannelKeeper.ChanUpgradeTry(ctx, msg.PortId, msg.ChannelId, msg.ProposedUpgradeConnectionHops, msg.UpgradeTimeout, msg.CounterpartyProposedUpgrade, msg.CounterpartyUpgradeSequence, msg.ProofChannel, msg.ProofUpgrade, msg.ProofHeight) + if err != nil { + if upgradeErr, ok := err.(*channeltypes.UpgradeError); ok { + if err := k.ChannelKeeper.AbortUpgrade(ctx, msg.PortId, msg.ChannelId, upgradeErr); err != nil { + return nil, err + } + + // NOTE: a failure Result is indicated to the client and an error receipt is written to state + return &channeltypes.MsgChannelUpgradeTryResponse{ + Result: channeltypes.FAILURE, + }, nil + } + + // TODO: add meaningful comment about type of error and rejecting full tx + revert state + return nil, err + } + + cacheCtx, writeFn := ctx.CacheContext() + version, err := cbs.OnChanUpgradeTry(cacheCtx, msg.PortId, msg.ChannelId, upgrade.Fields.Ordering, upgrade.Fields.ConnectionHops, upgrade.Fields.Version) + if err != nil { + if err := k.ChannelKeeper.AbortUpgrade(ctx, msg.PortId, msg.ChannelId, err); err != nil { + return nil, err + } + + return &channeltypes.MsgChannelUpgradeTryResponse{ + Result: channeltypes.FAILURE, + }, nil + } + + writeFn() + + upgrade.Fields.Version = version + + // TODO: determine where flushStatus is computed, pendingInflightPackets could be checked within WriteUpgradeTryChannel(?) + // the spec currently defines this in startFlushUpgradeHandshake(), if done here it would need to be returned and propagated to this call + k.ChannelKeeper.WriteUpgradeTryChannel(ctx, msg.PortId, msg.ChannelId, upgrade, channeltypes.FLUSHING) + + return &channeltypes.MsgChannelUpgradeTryResponse{ + ChannelId: msg.ChannelId, + Version: "", // todo: return upgrade and upgrade sequence similarly to INIT + Result: channeltypes.SUCCESS, + }, nil } // ChannelUpgradeAck defines a rpc handler method for MsgChannelUpgradeAck. From a3a2ac51519382c27bbd305265740c52b64d281a Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 7 Jun 2023 18:02:03 +0200 Subject: [PATCH 09/18] updating logging and comments --- modules/core/keeper/msg_server.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/modules/core/keeper/msg_server.go b/modules/core/keeper/msg_server.go index 40b40ba0e60..99d0eacc4ea 100644 --- a/modules/core/keeper/msg_server.go +++ b/modules/core/keeper/msg_server.go @@ -755,13 +755,13 @@ func (k Keeper) ChannelUpgradeTry(goCtx context.Context, msg *channeltypes.MsgCh module, _, err := k.ChannelKeeper.LookupModuleByChannel(ctx, msg.PortId, msg.ChannelId) if err != nil { - ctx.Logger().Error("channel upgrade init failed", "port-id", msg.PortId, "error", errorsmod.Wrap(err, "could not retrieve module from port-id")) + ctx.Logger().Error("channel upgrade try failed", "port-id", msg.PortId, "error", errorsmod.Wrap(err, "could not retrieve module from port-id")) return nil, errorsmod.Wrap(err, "could not retrieve module from port-id") } cbs, ok := k.Router.GetRoute(module) if !ok { - ctx.Logger().Error("channel upgrade init failed", "port-id", msg.PortId, "error", errorsmod.Wrapf(porttypes.ErrInvalidRoute, "route not found to module: %s", module)) + ctx.Logger().Error("channel upgrade try failed", "port-id", msg.PortId, "error", errorsmod.Wrapf(porttypes.ErrInvalidRoute, "route not found to module: %s", module)) return nil, errorsmod.Wrapf(porttypes.ErrInvalidRoute, "route not found to module: %s", module) } @@ -804,7 +804,7 @@ func (k Keeper) ChannelUpgradeTry(goCtx context.Context, msg *channeltypes.MsgCh return &channeltypes.MsgChannelUpgradeTryResponse{ ChannelId: msg.ChannelId, - Version: "", // todo: return upgrade and upgrade sequence similarly to INIT + Version: "", // todo: return upgrade and upgrade sequence aligning with return args of INIT Result: channeltypes.SUCCESS, }, nil } From b732ce32dfb4219940284b193cfdd9cc6010224e Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Thu, 8 Jun 2023 17:42:11 +0200 Subject: [PATCH 10/18] removing flushStatus arg from WriteUpgradeTryChannel --- modules/core/04-channel/keeper/upgrade.go | 4 ++-- modules/core/keeper/msg_server.go | 10 ++++------ 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 7bf9db30c36..0a1e5ed498c 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -184,7 +184,6 @@ func (k Keeper) WriteUpgradeTryChannel( ctx sdk.Context, portID, channelID string, proposedUpgrade types.Upgrade, - flushStatus types.FlushStatus, ) { defer telemetry.IncrCounter(1, "ibc", "channel", "upgrade-try") @@ -195,7 +194,8 @@ func (k Keeper) WriteUpgradeTryChannel( previousState := channel.State channel.State = types.TRYUPGRADE - channel.FlushStatus = flushStatus + // TODO: determine flush status + // channel.FlushStatus = flushStatus k.SetChannel(ctx, portID, channelID, channel) k.SetUpgrade(ctx, portID, channelID, proposedUpgrade) diff --git a/modules/core/keeper/msg_server.go b/modules/core/keeper/msg_server.go index 99d0eacc4ea..7307075c79f 100644 --- a/modules/core/keeper/msg_server.go +++ b/modules/core/keeper/msg_server.go @@ -794,17 +794,15 @@ func (k Keeper) ChannelUpgradeTry(goCtx context.Context, msg *channeltypes.MsgCh }, nil } - writeFn() - upgrade.Fields.Version = version + writeFn() - // TODO: determine where flushStatus is computed, pendingInflightPackets could be checked within WriteUpgradeTryChannel(?) - // the spec currently defines this in startFlushUpgradeHandshake(), if done here it would need to be returned and propagated to this call - k.ChannelKeeper.WriteUpgradeTryChannel(ctx, msg.PortId, msg.ChannelId, upgrade, channeltypes.FLUSHING) + k.ChannelKeeper.WriteUpgradeTryChannel(ctx, msg.PortId, msg.ChannelId, upgrade) + // TODO: add upgrade sequence to response return &channeltypes.MsgChannelUpgradeTryResponse{ ChannelId: msg.ChannelId, - Version: "", // todo: return upgrade and upgrade sequence aligning with return args of INIT + Upgrade: upgrade, Result: channeltypes.SUCCESS, }, nil } From 4462bd7e5cf2241eddba0696690a442eabb26da6 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Thu, 8 Jun 2023 17:53:41 +0200 Subject: [PATCH 11/18] move upgradeVersion refreshing to WriteUpgradeTryChannel, tidy code structure --- modules/core/04-channel/keeper/upgrade.go | 12 +++++------- modules/core/keeper/msg_server.go | 20 +++++++------------- 2 files changed, 12 insertions(+), 20 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 0a1e5ed498c..c38e47770f3 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -180,11 +180,7 @@ func (k Keeper) ChanUpgradeTry( // WriteUpgradeTryChannel writes a channel which has successfully passed the UpgradeTry handshake step. // An event is emitted for the handshake step. -func (k Keeper) WriteUpgradeTryChannel( - ctx sdk.Context, - portID, channelID string, - proposedUpgrade types.Upgrade, -) { +func (k Keeper) WriteUpgradeTryChannel(ctx sdk.Context, portID, channelID string, upgrade types.Upgrade, upgradeVersion string) { defer telemetry.IncrCounter(1, "ibc", "channel", "upgrade-try") channel, found := k.GetChannel(ctx, portID, channelID) @@ -197,11 +193,13 @@ func (k Keeper) WriteUpgradeTryChannel( // TODO: determine flush status // channel.FlushStatus = flushStatus + upgrade.Fields.Version = upgradeVersion + k.SetChannel(ctx, portID, channelID, channel) - k.SetUpgrade(ctx, portID, channelID, proposedUpgrade) + k.SetUpgrade(ctx, portID, channelID, upgrade) k.Logger(ctx).Info("channel state updated", "port-id", portID, "channel-id", channelID, "previous-state", previousState, "new-state", types.TRYUPGRADE.String()) - emitChannelUpgradeTryEvent(ctx, portID, channelID, channel, proposedUpgrade) + emitChannelUpgradeTryEvent(ctx, portID, channelID, channel, upgrade) } func (k Keeper) AbortUpgrade(ctx sdk.Context, portID, channelID string, err error) error { diff --git a/modules/core/keeper/msg_server.go b/modules/core/keeper/msg_server.go index 7307075c79f..44f558b4493 100644 --- a/modules/core/keeper/msg_server.go +++ b/modules/core/keeper/msg_server.go @@ -772,38 +772,32 @@ func (k Keeper) ChannelUpgradeTry(goCtx context.Context, msg *channeltypes.MsgCh return nil, err } - // NOTE: a failure Result is indicated to the client and an error receipt is written to state - return &channeltypes.MsgChannelUpgradeTryResponse{ - Result: channeltypes.FAILURE, - }, nil + // NOTE: a FAILURE result is returned to the client and an error receipt is written to state. + return &channeltypes.MsgChannelUpgradeTryResponse{Result: channeltypes.FAILURE}, nil } - // TODO: add meaningful comment about type of error and rejecting full tx + revert state + // NOTE: an error is returned to baseapp and transaction state is not committed. return nil, err } cacheCtx, writeFn := ctx.CacheContext() - version, err := cbs.OnChanUpgradeTry(cacheCtx, msg.PortId, msg.ChannelId, upgrade.Fields.Ordering, upgrade.Fields.ConnectionHops, upgrade.Fields.Version) + upgradeVersion, err := cbs.OnChanUpgradeTry(cacheCtx, msg.PortId, msg.ChannelId, upgrade.Fields.Ordering, upgrade.Fields.ConnectionHops, upgrade.Fields.Version) if err != nil { if err := k.ChannelKeeper.AbortUpgrade(ctx, msg.PortId, msg.ChannelId, err); err != nil { return nil, err } - return &channeltypes.MsgChannelUpgradeTryResponse{ - Result: channeltypes.FAILURE, - }, nil + return &channeltypes.MsgChannelUpgradeTryResponse{Result: channeltypes.FAILURE}, nil } - upgrade.Fields.Version = version writeFn() - k.ChannelKeeper.WriteUpgradeTryChannel(ctx, msg.PortId, msg.ChannelId, upgrade) + k.ChannelKeeper.WriteUpgradeTryChannel(ctx, msg.PortId, msg.ChannelId, upgrade, upgradeVersion) - // TODO: add upgrade sequence to response return &channeltypes.MsgChannelUpgradeTryResponse{ + Result: channeltypes.SUCCESS, ChannelId: msg.ChannelId, Upgrade: upgrade, - Result: channeltypes.SUCCESS, }, nil } From 98ce9644b8069fb144473392ef8fb1620700a935 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Sun, 11 Jun 2023 13:17:35 +0200 Subject: [PATCH 12/18] adding initial testcases for upgrade try msg server --- modules/core/keeper/msg_server_test.go | 124 +++++++++++++++++++++++++ 1 file changed, 124 insertions(+) diff --git a/modules/core/keeper/msg_server_test.go b/modules/core/keeper/msg_server_test.go index b46dbcff4c8..c552258eb10 100644 --- a/modules/core/keeper/msg_server_test.go +++ b/modules/core/keeper/msg_server_test.go @@ -2,6 +2,7 @@ package keeper_test import ( sdk "github.com/cosmos/cosmos-sdk/types" + capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types" clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types" @@ -774,3 +775,126 @@ func (suite *KeeperTestSuite) TestUpgradeClient() { } } } + +func (suite *KeeperTestSuite) TestChannelUpgradeTry() { + var ( + path *ibctesting.Path + msg *channeltypes.MsgChannelUpgradeTry + ) + + cases := []struct { + name string + malleate func() + expResult func(res *channeltypes.MsgChannelUpgradeTryResponse, err error) + }{ + { + "success", + func() {}, + func(res *channeltypes.MsgChannelUpgradeTryResponse, err error) { + suite.Require().NoError(err) + suite.Require().NotNil(res) + + suite.Require().Equal(channeltypes.SUCCESS, res.Result) + }, + }, + { + "module capability not found", + func() { + msg.PortId = "invalid-port" + msg.ChannelId = "invalid-channel" + }, + func(res *channeltypes.MsgChannelUpgradeTryResponse, err error) { + suite.Require().Error(err) + suite.Require().Nil(res) + + suite.Require().ErrorIs(err, capabilitytypes.ErrCapabilityNotFound) + }, + }, + { + "upgrade timeout has elapsed", + func() { + msg.UpgradeTimeout = channeltypes.NewTimeout(clienttypes.NewHeight(1, 10), 0) + suite.coordinator.CommitNBlocks(suite.chainB, 100) + }, + func(res *channeltypes.MsgChannelUpgradeTryResponse, err error) { + suite.Require().Nil(res) + + suite.Require().Error(err) + suite.Require().ErrorIs(err, channeltypes.ErrInvalidUpgrade) + }, + }, + { + "unsynchronized upgrade sequence writes upgrade error receipt", + func() { + channel := path.EndpointB.GetChannel() + channel.UpgradeSequence = 100 + + path.EndpointB.SetChannel(channel) + }, + func(res *channeltypes.MsgChannelUpgradeTryResponse, err error) { + suite.Require().NoError(err) + + suite.Require().NotNil(res) + suite.Require().Equal(channeltypes.FAILURE, res.Result) + + // TODO: assert error receipt exists for the upgrade sequence when RestoreChannel / AbortUpgrade is called + // errorReceipt, found := suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + // suite.Require().True(found) + }, + }, + { + "application callback error writes upgrade error receipt", + func() { + // TODO: override app callback + }, + func(res *channeltypes.MsgChannelUpgradeTryResponse, err error) { + + }, + }, + } + + for _, tc := range cases { + tc := tc + suite.Run(tc.name, func() { + suite.SetupTest() + + path = ibctesting.NewPath(suite.chainA, suite.chainB) + suite.coordinator.Setup(path) + + // configure the channel upgrade version on testing endpoints + path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = ibcmock.UpgradeVersion + path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = ibcmock.UpgradeVersion + + err := path.EndpointA.ChanUpgradeInit() + suite.Require().NoError(err) + + err = path.EndpointB.UpdateClient() + suite.Require().NoError(err) + + counterpartySequence := path.EndpointA.GetChannel().UpgradeSequence + counterpartyUpgrade, found := suite.chainA.GetSimApp().GetIBCKeeper().ChannelKeeper.GetUpgrade(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + suite.Require().True(found) + + proofChannel, proofUpgrade, proofHeight := path.EndpointB.QueryChannelUpgradeProof() + + msg = &channeltypes.MsgChannelUpgradeTry{ + PortId: path.EndpointB.ChannelConfig.PortID, + ChannelId: path.EndpointB.ChannelID, + ProposedUpgradeConnectionHops: []string{ibctesting.FirstConnectionID}, + UpgradeTimeout: channeltypes.NewTimeout(clienttypes.NewHeight(1, 100), 0), + CounterpartyUpgradeSequence: counterpartySequence, + CounterpartyProposedUpgrade: counterpartyUpgrade, + ProofChannel: proofChannel, + ProofUpgrade: proofUpgrade, + ProofHeight: proofHeight, + Signer: suite.chainB.SenderAccount.GetAddress().String(), + } + + tc.malleate() + + res, err := suite.chainB.GetSimApp().GetIBCKeeper().ChannelUpgradeTry(suite.chainB.GetContext(), msg) + + tc.expResult(res, err) + }) + } +} From 912b8da81187809abb9eff1924d4f63e648d10f0 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Sun, 11 Jun 2023 13:20:23 +0200 Subject: [PATCH 13/18] adding testing app overrides for upgrade handshake app callback handlers --- testing/mock/ibc_app.go | 36 ++++++++++++++++++++++++++++++++++++ testing/mock/ibc_module.go | 20 ++++++++++++++++++++ testing/simapp/app.go | 2 ++ 3 files changed, 58 insertions(+) diff --git a/testing/mock/ibc_app.go b/testing/mock/ibc_app.go index d3bac8889a2..e7c47153608 100644 --- a/testing/mock/ibc_app.go +++ b/testing/mock/ibc_app.go @@ -85,6 +85,42 @@ type IBCApp struct { packet channeltypes.Packet, relayer sdk.AccAddress, ) error + + OnChanUpgradeInit func( + ctx sdk.Context, + portID, channelID string, + order channeltypes.Order, + connectionHops []string, + sequence uint64, + version, previousVersion string, + ) (string, error) + + OnChanUpgradeTry func( + ctx sdk.Context, + portID, channelID string, + order channeltypes.Order, + connectionHops []string, + counterpartyVersion string, + ) (string, error) + + OnChanUpgradeAck func( + ctx sdk.Context, + portID, + channelID, + counterpartyVersion string, + ) error + + OnChanUpgradeOpen func( + ctx sdk.Context, + portID, + channelID string, + ) error + + OnChanUpgradeRestore func( + ctx sdk.Context, + portID, + channelID string, + ) error } // NewIBCApp returns a IBCApp. An empty PortID indicates the mock app doesn't bind/claim ports. diff --git a/testing/mock/ibc_module.go b/testing/mock/ibc_module.go index 49385afc415..59e12c03e90 100644 --- a/testing/mock/ibc_module.go +++ b/testing/mock/ibc_module.go @@ -164,26 +164,46 @@ func (im IBCModule) OnTimeoutPacket(ctx sdk.Context, packet channeltypes.Packet, // OnChanUpgradeInit implements the IBCModule interface func (im IBCModule) OnChanUpgradeInit(ctx sdk.Context, portID, channelID string, order channeltypes.Order, connectionHops []string, sequence uint64, version, previousVersion string) (string, error) { + if im.IBCApp.OnChanUpgradeInit != nil { + return im.IBCApp.OnChanUpgradeInit(ctx, portID, channelID, order, connectionHops, sequence, version, previousVersion) + } + return version, nil } // OnChanUpgradeTry implements the IBCModule interface func (im IBCModule) OnChanUpgradeTry(ctx sdk.Context, portID, channelID string, order channeltypes.Order, connectionHops []string, counterpartyVersion string) (string, error) { + if im.IBCApp.OnChanUpgradeTry != nil { + return im.IBCApp.OnChanUpgradeTry(ctx, portID, channelID, order, connectionHops, counterpartyVersion) + } + return counterpartyVersion, nil } // OnChanUpgradeAck implements the IBCModule interface func (im IBCModule) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, counterpartyVersion string) error { + if im.IBCApp.OnChanUpgradeAck != nil { + return im.IBCApp.OnChanUpgradeAck(ctx, portID, channelID, counterpartyVersion) + } + return nil } // OnChanUpgradeOpen implements the IBCModule interface func (im IBCModule) OnChanUpgradeOpen(ctx sdk.Context, portID, channelID string) error { + if im.IBCApp.OnChanUpgradeOpen != nil { + return im.IBCApp.OnChanUpgradeOpen(ctx, portID, channelID) + } + return nil } // OnChanUpgradeRestore implements the IBCModule interface func (im IBCModule) OnChanUpgradeRestore(ctx sdk.Context, portID, channelID string) error { + if im.IBCApp.OnChanUpgradeRestore != nil { + return im.IBCApp.OnChanUpgradeRestore(ctx, portID, channelID) + } + return nil } diff --git a/testing/simapp/app.go b/testing/simapp/app.go index 6e97d76c2fc..ac3654e4117 100644 --- a/testing/simapp/app.go +++ b/testing/simapp/app.go @@ -245,6 +245,7 @@ type SimApp struct { // make IBC modules public for test purposes // these modules are never directly routed to by the IBC Router + IBCMockModule ibcmock.IBCModule ICAAuthModule ibcmock.IBCModule FeeMockModule ibcmock.IBCModule @@ -455,6 +456,7 @@ func NewSimApp( // The mock module is used for testing IBC mockIBCModule := ibcmock.NewIBCModule(&mockModule, ibcmock.NewIBCApp(ibcmock.ModuleName, scopedIBCMockKeeper)) + app.IBCMockModule = mockIBCModule ibcRouter.AddRoute(ibcmock.ModuleName, mockIBCModule) // Create Transfer Stack From b3042e0e9dccffec6aca19edecd0eb7bec5619ee Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Sun, 11 Jun 2023 13:44:37 +0200 Subject: [PATCH 14/18] adding app callback testcases and updating assertions --- modules/core/keeper/msg_server_test.go | 29 +++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/modules/core/keeper/msg_server_test.go b/modules/core/keeper/msg_server_test.go index c552258eb10..94d44212d55 100644 --- a/modules/core/keeper/msg_server_test.go +++ b/modules/core/keeper/msg_server_test.go @@ -1,6 +1,8 @@ package keeper_test import ( + "fmt" + sdk "github.com/cosmos/cosmos-sdk/types" capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types" @@ -793,8 +795,11 @@ func (suite *KeeperTestSuite) TestChannelUpgradeTry() { func(res *channeltypes.MsgChannelUpgradeTryResponse, err error) { suite.Require().NoError(err) suite.Require().NotNil(res) - suite.Require().Equal(channeltypes.SUCCESS, res.Result) + + channel := path.EndpointB.GetChannel() + suite.Require().Equal(channeltypes.TRYUPGRADE, channel.State) + suite.Require().Equal(uint64(1), channel.UpgradeSequence) }, }, { @@ -811,16 +816,19 @@ func (suite *KeeperTestSuite) TestChannelUpgradeTry() { }, }, { - "upgrade timeout has elapsed", + "elapsed upgrade timeout returns error", func() { msg.UpgradeTimeout = channeltypes.NewTimeout(clienttypes.NewHeight(1, 10), 0) suite.coordinator.CommitNBlocks(suite.chainB, 100) }, func(res *channeltypes.MsgChannelUpgradeTryResponse, err error) { - suite.Require().Nil(res) - suite.Require().Error(err) + suite.Require().Nil(res) suite.Require().ErrorIs(err, channeltypes.ErrInvalidUpgrade) + + errorReceipt, found := suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + suite.Require().Empty(errorReceipt) + suite.Require().False(found) }, }, { @@ -845,10 +853,21 @@ func (suite *KeeperTestSuite) TestChannelUpgradeTry() { { "application callback error writes upgrade error receipt", func() { - // TODO: override app callback + suite.chainB.GetSimApp().IBCMockModule.IBCApp.OnChanUpgradeTry = func( + ctx sdk.Context, portID, channelID string, order channeltypes.Order, connectionHops []string, counterpartyVersion string, + ) (string, error) { + return "", fmt.Errorf("mock app callback failed") + } }, func(res *channeltypes.MsgChannelUpgradeTryResponse, err error) { + suite.Require().NoError(err) + + suite.Require().NotNil(res) + suite.Require().Equal(channeltypes.FAILURE, res.Result) + // TODO: assert error receipt exists for the upgrade sequence when RestoreChannel / AbortUpgrade is called + // errorReceipt, found := suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + // suite.Require().True(found) }, }, } From acc6b97a1f3dcdb5774e81913d192e9b72ea39e1 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Mon, 12 Jun 2023 14:16:28 +0200 Subject: [PATCH 15/18] updating tests to include application state changes and assert they are not committed. use GetTimeoutHeight helper on path.Endpoint --- modules/core/keeper/msg_server_test.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/modules/core/keeper/msg_server_test.go b/modules/core/keeper/msg_server_test.go index 94d44212d55..3dd53ad2d7d 100644 --- a/modules/core/keeper/msg_server_test.go +++ b/modules/core/keeper/msg_server_test.go @@ -856,6 +856,9 @@ func (suite *KeeperTestSuite) TestChannelUpgradeTry() { suite.chainB.GetSimApp().IBCMockModule.IBCApp.OnChanUpgradeTry = func( ctx sdk.Context, portID, channelID string, order channeltypes.Order, connectionHops []string, counterpartyVersion string, ) (string, error) { + // set arbitrary value in store to mock application state changes + store := ctx.KVStore(suite.chainB.GetSimApp().GetKey(exported.ModuleName)) + store.Set([]byte("foo"), []byte("bar")) return "", fmt.Errorf("mock app callback failed") } }, @@ -868,6 +871,10 @@ func (suite *KeeperTestSuite) TestChannelUpgradeTry() { // TODO: assert error receipt exists for the upgrade sequence when RestoreChannel / AbortUpgrade is called // errorReceipt, found := suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) // suite.Require().True(found) + + // assert application state changes are not committed + store := suite.chainB.GetContext().KVStore(suite.chainB.GetSimApp().GetKey(exported.ModuleName)) + suite.Require().False(store.Has([]byte("foo"))) }, }, } @@ -900,7 +907,7 @@ func (suite *KeeperTestSuite) TestChannelUpgradeTry() { PortId: path.EndpointB.ChannelConfig.PortID, ChannelId: path.EndpointB.ChannelID, ProposedUpgradeConnectionHops: []string{ibctesting.FirstConnectionID}, - UpgradeTimeout: channeltypes.NewTimeout(clienttypes.NewHeight(1, 100), 0), + UpgradeTimeout: channeltypes.NewTimeout(path.EndpointA.Chain.GetTimeoutHeight(), 0), CounterpartyUpgradeSequence: counterpartySequence, CounterpartyProposedUpgrade: counterpartyUpgrade, ProofChannel: proofChannel, From d6e358c5e5d24058a21779929dfcca13cf10a77a Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Mon, 12 Jun 2023 14:20:46 +0200 Subject: [PATCH 16/18] adding extra context to inline comment for upgrade cancellation subprotocol --- modules/core/keeper/msg_server.go | 1 + 1 file changed, 1 insertion(+) diff --git a/modules/core/keeper/msg_server.go b/modules/core/keeper/msg_server.go index 44f558b4493..a832cd87cb1 100644 --- a/modules/core/keeper/msg_server.go +++ b/modules/core/keeper/msg_server.go @@ -773,6 +773,7 @@ func (k Keeper) ChannelUpgradeTry(goCtx context.Context, msg *channeltypes.MsgCh } // NOTE: a FAILURE result is returned to the client and an error receipt is written to state. + // This signals to the relayer to begin the cancel upgrade handshake subprotocol. return &channeltypes.MsgChannelUpgradeTryResponse{Result: channeltypes.FAILURE}, nil } From bb422e45558a596a80c59ac6daa8f3230699468f Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Mon, 12 Jun 2023 14:35:53 +0200 Subject: [PATCH 17/18] updating WriteUpgradetryChannel godoc --- modules/core/04-channel/keeper/upgrade.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index c38e47770f3..2b868dab72e 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -178,7 +178,7 @@ func (k Keeper) ChanUpgradeTry( return upgrade, nil } -// WriteUpgradeTryChannel writes a channel which has successfully passed the UpgradeTry handshake step. +// WriteUpgradeTryChannel writes the channel end and upgrade to state after successfully passing the UpgradeTry handshake step. // An event is emitted for the handshake step. func (k Keeper) WriteUpgradeTryChannel(ctx sdk.Context, portID, channelID string, upgrade types.Upgrade, upgradeVersion string) { defer telemetry.IncrCounter(1, "ibc", "channel", "upgrade-try") From aaee7a76f2ea1034a478a2022df6347e85f156a3 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Mon, 12 Jun 2023 15:33:34 +0200 Subject: [PATCH 18/18] add return args to WriteUpgradeTryChannel to fulfil response args --- modules/core/04-channel/keeper/upgrade.go | 4 +++- modules/core/keeper/msg_server.go | 9 +++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 2b868dab72e..600cb4f8bee 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -180,7 +180,7 @@ func (k Keeper) ChanUpgradeTry( // WriteUpgradeTryChannel writes the channel end and upgrade to state after successfully passing the UpgradeTry handshake step. // An event is emitted for the handshake step. -func (k Keeper) WriteUpgradeTryChannel(ctx sdk.Context, portID, channelID string, upgrade types.Upgrade, upgradeVersion string) { +func (k Keeper) WriteUpgradeTryChannel(ctx sdk.Context, portID, channelID string, upgrade types.Upgrade, upgradeVersion string) (types.Channel, types.Upgrade) { defer telemetry.IncrCounter(1, "ibc", "channel", "upgrade-try") channel, found := k.GetChannel(ctx, portID, channelID) @@ -200,6 +200,8 @@ func (k Keeper) WriteUpgradeTryChannel(ctx sdk.Context, portID, channelID string k.Logger(ctx).Info("channel state updated", "port-id", portID, "channel-id", channelID, "previous-state", previousState, "new-state", types.TRYUPGRADE.String()) emitChannelUpgradeTryEvent(ctx, portID, channelID, channel, upgrade) + + return channel, upgrade } func (k Keeper) AbortUpgrade(ctx sdk.Context, portID, channelID string, err error) error { diff --git a/modules/core/keeper/msg_server.go b/modules/core/keeper/msg_server.go index a832cd87cb1..33df77fb5b9 100644 --- a/modules/core/keeper/msg_server.go +++ b/modules/core/keeper/msg_server.go @@ -793,12 +793,13 @@ func (k Keeper) ChannelUpgradeTry(goCtx context.Context, msg *channeltypes.MsgCh writeFn() - k.ChannelKeeper.WriteUpgradeTryChannel(ctx, msg.PortId, msg.ChannelId, upgrade, upgradeVersion) + channel, upgrade := k.ChannelKeeper.WriteUpgradeTryChannel(ctx, msg.PortId, msg.ChannelId, upgrade, upgradeVersion) return &channeltypes.MsgChannelUpgradeTryResponse{ - Result: channeltypes.SUCCESS, - ChannelId: msg.ChannelId, - Upgrade: upgrade, + Result: channeltypes.SUCCESS, + ChannelId: msg.ChannelId, + Upgrade: upgrade, + UpgradeSequence: channel.UpgradeSequence, }, nil }