From bd92bfcbd7effe47c807830234ad1544ca32fcdf Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Thu, 13 Jul 2023 10:32:28 +0300 Subject: [PATCH 1/4] use counterparty conn hops. --- modules/core/04-channel/keeper/upgrade.go | 10 ++- .../core/04-channel/keeper/upgrade_test.go | 84 +++++++++++++++++++ 2 files changed, 93 insertions(+), 1 deletion(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index a07c2730076..b20a8431807 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -355,11 +355,19 @@ func (k Keeper) ChanUpgradeOpen( if !found { return errorsmod.Wrapf(types.ErrUpgradeNotFound, "failed to retrieve channel upgrade: port ID (%s) channel ID (%s)", portID, channelID) } + // If counterparty has reached OPEN, we must use the upgraded connection to verify the counterparty channel + upgradedConnection, err := k.GetConnection(ctx, upgrade.Fields.ConnectionHops[0]) + if err != nil { + return errorsmod.Wrap(err, "failed to retrieve connection using the upgrade connection hops") + } + if upgradedConnection.GetState() != int32(connectiontypes.OPEN) { + return errorsmod.Wrapf(connectiontypes.ErrInvalidConnectionState, "connection state is not OPEN (got %s)", connectiontypes.State(upgradedConnection.GetState()).String()) + } counterpartyChannel = types.Channel{ State: types.OPEN, Ordering: upgrade.Fields.Ordering, - ConnectionHops: upgrade.Fields.ConnectionHops, + ConnectionHops: []string{upgradedConnection.GetCounterparty().GetConnectionID()}, Counterparty: types.NewCounterparty(portID, channelID), Version: upgrade.Fields.Version, UpgradeSequence: channel.UpgradeSequence, diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index 6e383fd217d..e8112b4afc8 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -754,6 +754,90 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpen() { } } +func (suite *KeeperTestSuite) TestChanUpgradeOpenCounterPartyStates() { + var path *ibctesting.Path + testCases := []struct { + name string + malleate func() + expError error + }{ + { + "success, counterparty in OPEN", + func() { + path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion + path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion + + err := path.EndpointB.ChanUpgradeInit() + suite.Require().NoError(err) + + err = path.EndpointA.ChanUpgradeTry() + suite.Require().NoError(err) + + err = path.EndpointB.ChanUpgradeAck() + suite.Require().NoError(err) + + err = path.EndpointB.ChanUpgradeOpen() + suite.Require().NoError(err) + + suite.coordinator.CommitBlock(suite.chainA, suite.chainB) + suite.Require().NoError(path.EndpointA.UpdateClient()) + }, + nil, + }, + { + "success, counterparty in TRYUPGRADE", + func() { + path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion + path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion + + err := path.EndpointA.ChanUpgradeInit() + suite.Require().NoError(err) + + err = path.EndpointB.ChanUpgradeTry() + suite.Require().NoError(err) + + err = path.EndpointA.ChanUpgradeAck() + suite.Require().NoError(err) + }, + nil, + }, + } + + // Create an initial path used only to invoke a ChanOpenInit handshake. + // This bumps the channel identifier generated for chain A on the + // next path used to run the upgrade handshake. + // See issue 4062. + path = ibctesting.NewPath(suite.chainA, suite.chainB) + suite.coordinator.SetupClients(path) + suite.Require().NoError(path.EndpointA.ConnOpenInit()) + suite.coordinator.SetupConnections(path) + suite.Require().NoError(path.EndpointA.ChanOpenInit()) + + for _, tc := range testCases { + tc := tc + suite.Run(tc.name, func() { + suite.SetupTest() + + path = ibctesting.NewPath(suite.chainA, suite.chainB) + suite.coordinator.Setup(path) + + tc.malleate() + + proofCounterpartyChannel, _, proofHeight := path.EndpointA.QueryChannelUpgradeProof() + err := suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.ChanUpgradeOpen( + suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, + path.EndpointB.GetChannel().State, proofCounterpartyChannel, proofHeight, + ) + + if tc.expError == nil { + suite.Require().NoError(err) + } else { + suite.Require().ErrorIs(err, tc.expError) + } + }) + } +} + func (suite *KeeperTestSuite) TestChanUpgradeTimeout() { var ( path *ibctesting.Path From ffc3602471d880658f7749cd7c3052085de4f231 Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Thu, 13 Jul 2023 20:07:44 +0300 Subject: [PATCH 2/4] Clean up tests. --- modules/core/04-channel/keeper/upgrade_test.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index e8112b4afc8..1592d13a885 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -754,6 +754,8 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpen() { } } +// TestChanUpgradeOpenCounterPartyStates tests the handshake in the cases where +// the counterparty is in a state other than OPEN. func (suite *KeeperTestSuite) TestChanUpgradeOpenCounterPartyStates() { var path *ibctesting.Path testCases := []struct { @@ -764,9 +766,6 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpenCounterPartyStates() { { "success, counterparty in OPEN", func() { - path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion - path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion - err := path.EndpointB.ChanUpgradeInit() suite.Require().NoError(err) @@ -776,6 +775,8 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpenCounterPartyStates() { err = path.EndpointB.ChanUpgradeAck() suite.Require().NoError(err) + // TODO: Remove when #4030 is closed. Channel will automatically + // move to OPEN in that case. err = path.EndpointB.ChanUpgradeOpen() suite.Require().NoError(err) @@ -787,9 +788,6 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpenCounterPartyStates() { { "success, counterparty in TRYUPGRADE", func() { - path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion - path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion - err := path.EndpointA.ChanUpgradeInit() suite.Require().NoError(err) @@ -821,6 +819,9 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpenCounterPartyStates() { path = ibctesting.NewPath(suite.chainA, suite.chainB) suite.coordinator.Setup(path) + path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion + path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion + tc.malleate() proofCounterpartyChannel, _, proofHeight := path.EndpointA.QueryChannelUpgradeProof() From f66eb4b4df5111a5f56a7b12e51fae8b881e83c2 Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Wed, 19 Jul 2023 08:19:42 +0300 Subject: [PATCH 3/4] Amend inline comment slightly. --- modules/core/04-channel/keeper/upgrade_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index f5236440c17..16d98deb18b 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -801,8 +801,8 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpenCounterPartyStates() { }, } - // Create an initial path used only to invoke a ChanOpenInit handshake. - // This bumps the channel identifier generated for chain A on the + // Create an initial path used only to invoke ConnOpenInit/ChanOpenInit handlers. + // This bumps the connection/channel identifiers generated for chain A on the // next path used to run the upgrade handshake. // See issue 4062. path = ibctesting.NewPath(suite.chainA, suite.chainB) From 9ce9b152960a28d963ee4caf2c710c49a6c8fd21 Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Wed, 19 Jul 2023 14:32:30 +0300 Subject: [PATCH 4/4] Address nits --- modules/core/04-channel/keeper/upgrade.go | 9 +++++---- modules/core/04-channel/keeper/upgrade_test.go | 5 +++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 2f2d8081ab9..6aaf37b55df 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -356,18 +356,19 @@ func (k Keeper) ChanUpgradeOpen( return errorsmod.Wrapf(types.ErrUpgradeNotFound, "failed to retrieve channel upgrade: port ID (%s) channel ID (%s)", portID, channelID) } // If counterparty has reached OPEN, we must use the upgraded connection to verify the counterparty channel - upgradedConnection, err := k.GetConnection(ctx, upgrade.Fields.ConnectionHops[0]) + upgradeConnection, err := k.GetConnection(ctx, upgrade.Fields.ConnectionHops[0]) if err != nil { return errorsmod.Wrap(err, "failed to retrieve connection using the upgrade connection hops") } - if upgradedConnection.GetState() != int32(connectiontypes.OPEN) { - return errorsmod.Wrapf(connectiontypes.ErrInvalidConnectionState, "connection state is not OPEN (got %s)", connectiontypes.State(upgradedConnection.GetState()).String()) + + if upgradeConnection.GetState() != int32(connectiontypes.OPEN) { + return errorsmod.Wrapf(connectiontypes.ErrInvalidConnectionState, "connection state is not OPEN (got %s)", connectiontypes.State(upgradeConnection.GetState()).String()) } counterpartyChannel = types.Channel{ State: types.OPEN, Ordering: upgrade.Fields.Ordering, - ConnectionHops: []string{upgradedConnection.GetCounterparty().GetConnectionID()}, + ConnectionHops: []string{upgradeConnection.GetCounterparty().GetConnectionID()}, Counterparty: types.NewCounterparty(portID, channelID), Version: upgrade.Fields.Version, UpgradeSequence: channel.UpgradeSequence, diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index 16d98deb18b..df2927d207d 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -756,7 +756,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpen() { // TestChanUpgradeOpenCounterPartyStates tests the handshake in the cases where // the counterparty is in a state other than OPEN. -func (suite *KeeperTestSuite) TestChanUpgradeOpenCounterPartyStates() { +func (suite *KeeperTestSuite) TestChanUpgradeOpenCounterpartyStates() { var path *ibctesting.Path testCases := []struct { name string @@ -830,7 +830,8 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpenCounterPartyStates() { path.EndpointB.GetChannel().State, proofCounterpartyChannel, proofHeight, ) - if tc.expError == nil { + expPass := tc.expError == nil + if expPass { suite.Require().NoError(err) } else { suite.Require().ErrorIs(err, tc.expError)