From d22d046fdffae7d5e2e5983b93b8756d6ce0a09b Mon Sep 17 00:00:00 2001 From: Aditya Sripal Date: Thu, 14 Dec 2023 15:25:51 +0100 Subject: [PATCH 01/11] fix sequencing logic and unit tests --- modules/core/04-channel/keeper/upgrade.go | 101 +++++++++++------- .../core/04-channel/keeper/upgrade_test.go | 14 +-- 2 files changed, 69 insertions(+), 46 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index e3b9cb77d31..4ee98c72c03 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -99,44 +99,6 @@ func (k Keeper) ChanUpgradeTry( ) } - // construct counterpartyChannel from existing information and provided counterpartyUpgradeSequence - // create upgrade fields from counterparty proposed upgrade and own verified connection hops - proposedUpgradeFields := types.UpgradeFields{ - Ordering: counterpartyUpgradeFields.Ordering, - ConnectionHops: proposedConnectionHops, - Version: counterpartyUpgradeFields.Version, - } - - var ( - err error - upgrade types.Upgrade - ) - - // NOTE: if an upgrade exists (crossing hellos) then use existing upgrade fields - // otherwise, run the upgrade init sub-protocol - upgrade, found = k.GetUpgrade(ctx, portID, channelID) - if found { - proposedUpgradeFields = upgrade.Fields - } else { - // NOTE: OnChanUpgradeInit will not be executed by the application - upgrade, err = k.ChanUpgradeInit(ctx, portID, channelID, proposedUpgradeFields) - if err != nil { - return types.Channel{}, types.Upgrade{}, errorsmod.Wrap(err, "failed to initialize upgrade") - } - - channel, upgrade = k.WriteUpgradeInitChannel(ctx, portID, channelID, upgrade, upgrade.Fields.Version) - - // if the counterparty sequence is greater than the current sequence, we fast-forward to the counterparty sequence. - if counterpartyUpgradeSequence > channel.UpgradeSequence { - channel.UpgradeSequence = counterpartyUpgradeSequence - k.SetChannel(ctx, portID, channelID, channel) - } - } - - if err := k.checkForUpgradeCompatibility(ctx, proposedUpgradeFields, counterpartyUpgradeFields); err != nil { - return types.Channel{}, types.Upgrade{}, errorsmod.Wrap(err, "failed upgrade compatibility check") - } - // construct expected counterparty channel from information in state // only the counterpartyUpgradeSequence is provided by the relayer counterpartyConnectionHops := []string{connection.GetCounterparty().GetConnectionID()} @@ -161,8 +123,37 @@ func (k Keeper) ChanUpgradeTry( return types.Channel{}, types.Upgrade{}, errorsmod.Wrap(err, "failed to verify counterparty channel state") } + var ( + err error + upgrade types.Upgrade + isCrossingHello bool + ) + + upgrade, isCrossingHello = k.GetUpgrade(ctx, portID, channelID) + // In this case, the counterparty upgrade is outdated. We want to force the counterparty + // to abort their upgrade and come back to sync with our own upgrade sequence. + // In the case, that we are in a non-crossing hello (i.e. upgrade does not exist on our side), + // the sequence on both sides should move to a fresh sequence on the next upgrade attempt. + // Thus, we write an error receipt with our own upgrade sequence which will cause the counterparty + // to cancel their upgrade and move to the same sequence. When a new upgrade attempt is started from either + // side, it will be a fresh sequence for both sides (i.e. channel.upgradeSequence + 1) + // In the crossing hello case, we already have an upgrade but it is at a higher sequence than the counterparty. + // Thus, our upgrade should take priority. We force the counterparty to abort their upgrade by invalidating all counterparty + // upgrade attempts below our own sequence by setting errorReceipt to upgradeSequence - 1. + // The upgrade handshake may then proceed on the counterparty with our sequence + // NOTE: Two possible outcomes may occur in this scenario. + // The UpgradeCancel datagram may reach the counterparty first, which will cause the counterparty to cancel. The counterparty + // may then receive a TRY with our channel upgrade sequence and correctly increment their sequence to become synced with our upgrade attempt. + // The UpgradeTRY message may arrive first, in this case, **IF** the upgrade fields are mutually compatible; the counterparty will simply + // fast forward their sequence to our own and continue the upgrade. The following Cancel message will be rejected as it is below the current sequence. if counterpartyUpgradeSequence < channel.UpgradeSequence { - return channel, upgrade, types.NewUpgradeError(channel.UpgradeSequence-1, errorsmod.Wrapf( + var upgradeSequence uint64 + if isCrossingHello { + upgradeSequence = channel.UpgradeSequence - 1 + } else { + upgradeSequence = channel.UpgradeSequence + } + return channel, upgrade, types.NewUpgradeError(upgradeSequence, errorsmod.Wrapf( types.ErrInvalidUpgradeSequence, "counterparty upgrade sequence < current upgrade sequence (%d < %d)", counterpartyUpgradeSequence, channel.UpgradeSequence, )) } @@ -179,6 +170,38 @@ func (k Keeper) ChanUpgradeTry( return types.Channel{}, types.Upgrade{}, errorsmod.Wrap(err, "failed to verify counterparty upgrade") } + // construct counterpartyChannel from existing information and provided counterpartyUpgradeSequence + // create upgrade fields from counterparty proposed upgrade and own verified connection hops + proposedUpgradeFields := types.UpgradeFields{ + Ordering: counterpartyUpgradeFields.Ordering, + ConnectionHops: proposedConnectionHops, + Version: counterpartyUpgradeFields.Version, + } + + // NOTE: if an upgrade exists (crossing hellos) then use existing upgrade fields + // otherwise, run the upgrade init sub-protocol + if isCrossingHello { + proposedUpgradeFields = upgrade.Fields + } else { + // NOTE: OnChanUpgradeInit will not be executed by the application + upgrade, err = k.ChanUpgradeInit(ctx, portID, channelID, proposedUpgradeFields) + if err != nil { + return types.Channel{}, types.Upgrade{}, errorsmod.Wrap(err, "failed to initialize upgrade") + } + + channel, upgrade = k.WriteUpgradeInitChannel(ctx, portID, channelID, upgrade, upgrade.Fields.Version) + } + + if err := k.checkForUpgradeCompatibility(ctx, proposedUpgradeFields, counterpartyUpgradeFields); err != nil { + return types.Channel{}, types.Upgrade{}, errorsmod.Wrap(err, "failed upgrade compatibility check") + } + + // if the counterparty sequence is greater than the current sequence, we fast-forward to the counterparty sequence. + if counterpartyUpgradeSequence > channel.UpgradeSequence { + channel.UpgradeSequence = counterpartyUpgradeSequence + k.SetChannel(ctx, portID, channelID, channel) + } + if err := k.startFlushing(ctx, portID, channelID, &upgrade); err != nil { return types.Channel{}, types.Upgrade{}, err } diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index 1b6ec37821c..38198be1940 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -228,7 +228,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { func() { counterpartyUpgrade.Fields.ConnectionHops = []string{ibctesting.InvalidID} }, - types.ErrIncompatibleCounterpartyUpgrade, + commitmenttypes.ErrInvalidProof, }, { "fails due to incompatible upgrades, chainB proposes a new connection hop that does not match counterparty", @@ -252,7 +252,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { channel.UpgradeSequence = 5 path.EndpointB.SetChannel(channel) }, - // channel sequence - 1 will be returned (upgrade sequence is bumped in init as this is non-crossing hellos case) + // channel sequence will be returned so that counterparty inits on completely fresh sequence for both sides types.NewUpgradeError(5, types.ErrInvalidUpgradeSequence), }, } @@ -1399,7 +1399,7 @@ func (suite *KeeperTestSuite) TestChanUpgrade_UpgradeSucceeds_AfterCancel() { path.EndpointA.SetChannel(channel) channel = path.EndpointB.GetChannel() - channel.UpgradeSequence = 2 + channel.UpgradeSequence = 5 path.EndpointB.SetChannel(channel) suite.Require().NoError(path.EndpointA.UpdateClient()) @@ -1430,8 +1430,8 @@ func (suite *KeeperTestSuite) TestChanUpgrade_UpgradeSucceeds_AfterCancel() { channel := path.EndpointA.GetChannel() suite.Require().Equal(types.OPEN, channel.State) - suite.T().Run("verify upgrade sequences are synced", func(t *testing.T) { - suite.Require().Equal(uint64(2), channel.UpgradeSequence) + suite.T().Run("verify upgrade sequence fastforwards to channelB sequence", func(t *testing.T) { + suite.Require().Equal(uint64(5), channel.UpgradeSequence) }) }) @@ -1448,8 +1448,8 @@ func (suite *KeeperTestSuite) TestChanUpgrade_UpgradeSucceeds_AfterCancel() { suite.Require().Equal(types.OPEN, channel.State, "channel should be in OPEN state") suite.Require().Equal(mock.UpgradeVersion, channel.Version, "version should be correctly upgraded") suite.Require().Equal(mock.UpgradeVersion, path.EndpointB.GetChannel().Version, "version should be correctly upgraded") - suite.Require().Equal(uint64(3), channel.UpgradeSequence, "upgrade sequence should be incremented") - suite.Require().Equal(uint64(3), path.EndpointB.GetChannel().UpgradeSequence, "upgrade sequence should be incremented on counterparty") + suite.Require().Equal(uint64(6), channel.UpgradeSequence, "upgrade sequence should be incremented") + suite.Require().Equal(uint64(6), path.EndpointB.GetChannel().UpgradeSequence, "upgrade sequence should be incremented on counterparty") }) } From 699af0c0cdbd3061225b3a47b788dc57b647a0e2 Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Fri, 15 Dec 2023 14:55:14 +0100 Subject: [PATCH 02/11] added tests --- .../core/04-channel/keeper/upgrade_test.go | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index 38198be1940..8324cf01ecc 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -255,6 +255,70 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { // channel sequence will be returned so that counterparty inits on completely fresh sequence for both sides types.NewUpgradeError(5, types.ErrInvalidUpgradeSequence), }, + { + // ChainA(Sequence: 0), ChainB(Sequence 4) + // ChainA.INIT(Sequence: 1) + // ChainB.INIT(Sequence: 5) + // ChainB.TRY(ErrorReceipt: 4) + "crossing hellos: fails due to mismatch in upgrade sequences", + func() { + channel := path.EndpointB.GetChannel() + channel.UpgradeSequence = 4 + path.EndpointB.SetChannel(channel) + + err := path.EndpointB.ChanUpgradeInit() + suite.Require().NoError(err) + }, + types.NewUpgradeError(4, types.ErrInvalidUpgradeSequence), + }, + { + // ChainA(Sequence: 0, ics20-2), ChainB(Sequence: 0, ics20-3) + // ChainA.INIT(Sequence: 1) + // ChainB.INIT(Sequence: 1) + // ChainA.TRY => error (incompatible versions) + // ChainB.TRY => error (incompatible versions) + "crossing hellos: fails due to incompatible version", + func() { + // use imcompatible version + path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = fmt.Sprintf("%s-v3", mock.Version) + proposedUpgrade = path.EndpointB.GetProposedUpgrade() + + err := path.EndpointB.ChanUpgradeInit() + suite.Require().NoError(err) + + err = path.EndpointA.ChanUpgradeTry() + suite.Require().Error(err) + suite.Require().ErrorContains(err, "incompatible counterparty upgrade") + suite.Require().Equal(uint64(1), path.EndpointA.GetChannel().UpgradeSequence) + }, + types.ErrIncompatibleCounterpartyUpgrade, + }, + // ChainA(Sequence: 0, ics20-2), ChainB(Sequence: 4, ics20-3) + // ChainA.INIT(Sequence: 1) + // ChainB.INIT(Sequence: 5) + // ChainA.TRY => error (incompatible versions) + // ChainB.TRY(ErrorReceipt: 4) + { + "crossing hellos: upgrade starts with mismatching upgrade sequences and try fails on counterparty due to incompatible version", + func() { + channel := path.EndpointB.GetChannel() + channel.UpgradeSequence = 4 + path.EndpointB.SetChannel(channel) + + // use imcompatible version + path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = fmt.Sprintf("%s-v3", mock.Version) + proposedUpgrade = path.EndpointB.GetProposedUpgrade() + + err := path.EndpointB.ChanUpgradeInit() + suite.Require().NoError(err) + + err = path.EndpointA.ChanUpgradeTry() + suite.Require().Error(err) + suite.Require().ErrorContains(err, "incompatible counterparty upgrade") + suite.Require().Equal(uint64(1), path.EndpointA.GetChannel().UpgradeSequence) + }, + types.NewUpgradeError(4, types.ErrInvalidUpgradeSequence), + }, } for _, tc := range testCases { From b46879a556924876803a73ff52b3f4e38635cdb3 Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Fri, 15 Dec 2023 14:55:23 +0100 Subject: [PATCH 03/11] reorder comments --- modules/core/04-channel/keeper/upgrade.go | 34 ++++++++++++----------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 4ee98c72c03..de1b357ec23 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -130,29 +130,31 @@ func (k Keeper) ChanUpgradeTry( ) upgrade, isCrossingHello = k.GetUpgrade(ctx, portID, channelID) - // In this case, the counterparty upgrade is outdated. We want to force the counterparty - // to abort their upgrade and come back to sync with our own upgrade sequence. - // In the case, that we are in a non-crossing hello (i.e. upgrade does not exist on our side), - // the sequence on both sides should move to a fresh sequence on the next upgrade attempt. - // Thus, we write an error receipt with our own upgrade sequence which will cause the counterparty - // to cancel their upgrade and move to the same sequence. When a new upgrade attempt is started from either - // side, it will be a fresh sequence for both sides (i.e. channel.upgradeSequence + 1) - // In the crossing hello case, we already have an upgrade but it is at a higher sequence than the counterparty. - // Thus, our upgrade should take priority. We force the counterparty to abort their upgrade by invalidating all counterparty - // upgrade attempts below our own sequence by setting errorReceipt to upgradeSequence - 1. - // The upgrade handshake may then proceed on the counterparty with our sequence - // NOTE: Two possible outcomes may occur in this scenario. - // The UpgradeCancel datagram may reach the counterparty first, which will cause the counterparty to cancel. The counterparty - // may then receive a TRY with our channel upgrade sequence and correctly increment their sequence to become synced with our upgrade attempt. - // The UpgradeTRY message may arrive first, in this case, **IF** the upgrade fields are mutually compatible; the counterparty will simply - // fast forward their sequence to our own and continue the upgrade. The following Cancel message will be rejected as it is below the current sequence. if counterpartyUpgradeSequence < channel.UpgradeSequence { + // In this case, the counterparty upgrade is outdated. We want to force the counterparty + // to abort their upgrade and come back to sync with our own upgrade sequence. var upgradeSequence uint64 if isCrossingHello { + // In the crossing hello case, we already have an upgrade but it is at a higher sequence than the counterparty. + // Thus, our upgrade should take priority. We force the counterparty to abort their upgrade by invalidating all counterparty + // upgrade attempts below our own sequence by setting errorReceipt to upgradeSequence - 1. + // The upgrade handshake may then proceed on the counterparty with our sequence upgradeSequence = channel.UpgradeSequence - 1 } else { + // In the case, that we are in a non-crossing hello (i.e. upgrade does not exist on our side), + // the sequence on both sides should move to a fresh sequence on the next upgrade attempt. + // Thus, we write an error receipt with our own upgrade sequence which will cause the counterparty + // to cancel their upgrade and move to the same sequence. When a new upgrade attempt is started from either + // side, it will be a fresh sequence for both sides (i.e. channel.upgradeSequence + 1) upgradeSequence = channel.UpgradeSequence } + + // NOTE: Two possible outcomes may occur in this scenario. + // The ChanUpgradeCancel datagram may reach the counterparty first, which will cause the counterparty to cancel. The counterparty + // may then receive a TRY with our channel upgrade sequence and correctly increment their sequence to become synced with our upgrade attempt. + // The ChanUpgradeTry message may arrive first, in this case, **IF** the upgrade fields are mutually compatible; the counterparty will simply + // fast forward their sequence to our own and continue the upgrade. The following ChanUpgradeCancel message will be rejected as it is below the current sequence. + return channel, upgrade, types.NewUpgradeError(upgradeSequence, errorsmod.Wrapf( types.ErrInvalidUpgradeSequence, "counterparty upgrade sequence < current upgrade sequence (%d < %d)", counterpartyUpgradeSequence, channel.UpgradeSequence, )) From 7eb246c942f6165b059986b6697812ac4df57922 Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Fri, 15 Dec 2023 16:38:25 +0100 Subject: [PATCH 04/11] add test --- .../core/04-channel/keeper/upgrade_test.go | 29 +++++++++++++++++-- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index 8324cf01ecc..aa236684a7f 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -2329,17 +2329,38 @@ func (suite *KeeperTestSuite) TestChanUpgradeCrossingHelloWithHistoricalProofs() var path *ibctesting.Path testCases := []struct { - name string - malleate func() - expError error + name string + sequenceFn func() + malleate func() + expError error }{ { "success", func() {}, + func() {}, nil, }, + { + // ChainA(Sequence: 0), ChainB(Sequence 4) + // ChainA.INIT(Sequence: 1) + // ChainB.INIT(Sequence: 5) + // ChainA.TRY(Sequence: 5) // fastforward + // ChainB.TRY(ErrorReceipt: 4) + "fails due to mismatch in upgrade sequences", + func() { + channel := path.EndpointB.GetChannel() + channel.UpgradeSequence = 4 + path.EndpointB.SetChannel(channel) + }, + func() { + channel := path.EndpointA.GetChannel() + suite.Require().Equal(uint64(5), channel.UpgradeSequence) + }, + types.NewUpgradeError(4, types.ErrInvalidUpgradeSequence), + }, { "counterparty (chain B) has already progressed to ACK step", + func() {}, func() { err := path.EndpointB.ChanUpgradeAck() suite.Require().NoError(err) @@ -2362,6 +2383,8 @@ func (suite *KeeperTestSuite) TestChanUpgradeCrossingHelloWithHistoricalProofs() err := path.EndpointA.ChanUpgradeInit() suite.Require().NoError(err) + tc.sequenceFn() + err = path.EndpointB.ChanUpgradeInit() suite.Require().NoError(err) From f8aa85bc3c9e44e2f0a1b81221b6cde5bf38bcd6 Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Fri, 15 Dec 2023 16:39:29 +0100 Subject: [PATCH 05/11] fix typo --- 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 aa236684a7f..c2c9478f992 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -279,7 +279,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { // ChainB.TRY => error (incompatible versions) "crossing hellos: fails due to incompatible version", func() { - // use imcompatible version + // use incompatible version path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = fmt.Sprintf("%s-v3", mock.Version) proposedUpgrade = path.EndpointB.GetProposedUpgrade() @@ -305,7 +305,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { channel.UpgradeSequence = 4 path.EndpointB.SetChannel(channel) - // use imcompatible version + // use incompatible version path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = fmt.Sprintf("%s-v3", mock.Version) proposedUpgrade = path.EndpointB.GetProposedUpgrade() From bae2f59a5eb74a263695afe09f2ed784353c0962 Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Fri, 15 Dec 2023 23:08:16 +0100 Subject: [PATCH 06/11] refactored a couple of tests --- .../core/04-channel/keeper/upgrade_test.go | 172 +++++++++++++----- 1 file changed, 130 insertions(+), 42 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index c2c9478f992..a258dba8d34 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -255,22 +255,6 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { // channel sequence will be returned so that counterparty inits on completely fresh sequence for both sides types.NewUpgradeError(5, types.ErrInvalidUpgradeSequence), }, - { - // ChainA(Sequence: 0), ChainB(Sequence 4) - // ChainA.INIT(Sequence: 1) - // ChainB.INIT(Sequence: 5) - // ChainB.TRY(ErrorReceipt: 4) - "crossing hellos: fails due to mismatch in upgrade sequences", - func() { - channel := path.EndpointB.GetChannel() - channel.UpgradeSequence = 4 - path.EndpointB.SetChannel(channel) - - err := path.EndpointB.ChanUpgradeInit() - suite.Require().NoError(err) - }, - types.NewUpgradeError(4, types.ErrInvalidUpgradeSequence), - }, { // ChainA(Sequence: 0, ics20-2), ChainB(Sequence: 0, ics20-3) // ChainA.INIT(Sequence: 1) @@ -372,6 +356,133 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { } } +// TestChanUpgrade_CrossingHellos_UpgradeSucceeds_AfterCancel verifies that under crossing hellos if upgrade +// sequences become out of sync, the upgrade can still be performed successfully after the upgrade is cancelled. +// ChainA(Sequence: 0), ChainB(Sequence 4) +// ChainA.INIT(Sequence: 1) +// ChainB.INIT(Sequence: 5) +// ChainB.TRY(ErrorReceipt: 4) +// ChainA.Cancel(Sequence: 4) +// ChainA.TRY(Sequence: 5) // fastforward +// ChainB.ACK => Success +// ChainA.Confirm => Success +// ChainB.Open => Success +func (suite *KeeperTestSuite) TestChanUpgrade_CrossingHellos_UpgradeSucceeds_AfterCancel() { + path := ibctesting.NewPath(suite.chainA, suite.chainB) + suite.coordinator.Setup(path) + + path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion + path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion + + suite.Require().NoError(path.EndpointA.ChanUpgradeInit()) + + channel := path.EndpointB.GetChannel() + channel.UpgradeSequence = 4 + path.EndpointB.SetChannel(channel) + + suite.Require().NoError(path.EndpointB.ChanUpgradeInit()) + suite.Require().NoError(path.EndpointB.ChanUpgradeTry()) + + suite.Require().NoError(path.EndpointA.ChanUpgradeCancel()) + channel = path.EndpointA.GetChannel() + suite.Require().Equal(types.OPEN, channel.State) + suite.Require().Equal(uint64(4), channel.UpgradeSequence) + + suite.Require().NoError(path.EndpointA.ChanUpgradeTry()) + channel = path.EndpointA.GetChannel() + suite.Require().Equal(types.FLUSHING, channel.State) + suite.Require().Equal(uint64(5), channel.UpgradeSequence) + + suite.T().Run("successfully completes upgrade", func(t *testing.T) { + suite.Require().NoError(path.EndpointB.ChanUpgradeAck()) + suite.Require().NoError(path.EndpointA.ChanUpgradeConfirm()) + suite.Require().NoError(path.EndpointB.ChanUpgradeOpen()) + }) + + suite.T().Run("channel in expected state", func(t *testing.T) { + channel := path.EndpointA.GetChannel() + suite.Require().Equal(types.OPEN, channel.State, "channel should be in OPEN state") + suite.Require().Equal(mock.UpgradeVersion, channel.Version, "version should be correctly upgraded") + suite.Require().Equal(mock.UpgradeVersion, path.EndpointB.GetChannel().Version, "version should be correctly upgraded") + suite.Require().Equal(uint64(5), channel.UpgradeSequence, "upgrade sequence should be incremented") + suite.Require().Equal(uint64(5), path.EndpointB.GetChannel().UpgradeSequence, "upgrade sequence should be incremented on counterparty") + }) +} + +// TestChanUpgrade_CrossingHellos_UpgradeSucceeds_AfterCancelErrors verifies that under crossing hellos if upgrade +// sequences become out of sync, the upgrade can still be performed successfully after the cancel fails. +// ChainA(Sequence: 0), ChainB(Sequence 4) +// ChainA.INIT(Sequence: 1) +// ChainB.INIT(Sequence: 5) +// ChainA.TRY(Sequence: 5) // fastforward +// ChainB.TRY(ErrorReceipt: 4) +// ChainA.Cancel => Error (errorReceipt.Sequence < channel.UpgradeSequence) +// ChainB.ACK => Success +// ChainA.Confirm => Success +// ChainB.Open => Success +func (suite *KeeperTestSuite) TestChanUpgrade_CrossingHellos_UpgradeSucceeds_AfterCancelErrors() { + path := ibctesting.NewPath(suite.chainA, suite.chainB) + suite.coordinator.Setup(path) + + path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion + path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion + + suite.Require().NoError(path.EndpointA.ChanUpgradeInit()) + + channel := path.EndpointB.GetChannel() + channel.UpgradeSequence = 4 + path.EndpointB.SetChannel(channel) + + suite.Require().NoError(path.EndpointB.ChanUpgradeInit()) + + suite.coordinator.CommitBlock(suite.chainA, suite.chainB) + suite.Require().NoError(path.EndpointB.UpdateClient()) + + // use proofs when chain A has not executed TRY yet and use them when executing TRY on chain B + historicalChannelProof, historicalUpgradeProof, proofHeight := path.EndpointA.QueryChannelUpgradeProof() + + suite.Require().NoError(path.EndpointA.ChanUpgradeTry()) + _, _, err := suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.ChanUpgradeTry( + suite.chainB.GetContext(), + path.EndpointB.ChannelConfig.PortID, + path.EndpointB.ChannelID, + path.EndpointB.GetChannelUpgrade().Fields.ConnectionHops, + path.EndpointA.GetChannelUpgrade().Fields, + 1, + historicalChannelProof, + historicalUpgradeProof, + proofHeight, + ) + suite.Require().Error(err) + suite.assertUpgradeError(err, types.NewUpgradeError(4, types.ErrInvalidUpgradeSequence)) + errorReceipt := err.(*types.UpgradeError).GetErrorReceipt() + suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, errorReceipt) + suite.coordinator.CommitBlock(suite.chainB) + + err = path.EndpointA.ChanUpgradeCancel() + suite.Require().Error(err) + suite.Require().ErrorContains(err, "invalid upgrade sequence") + + channel = path.EndpointA.GetChannel() + suite.Require().Equal(types.FLUSHING, channel.State) + suite.Require().Equal(uint64(5), channel.UpgradeSequence) + + suite.T().Run("successfully completes upgrade", func(t *testing.T) { + suite.Require().NoError(path.EndpointB.ChanUpgradeAck()) + suite.Require().NoError(path.EndpointA.ChanUpgradeConfirm()) + suite.Require().NoError(path.EndpointB.ChanUpgradeOpen()) + }) + + suite.T().Run("channel in expected state", func(t *testing.T) { + channel := path.EndpointA.GetChannel() + suite.Require().Equal(types.OPEN, channel.State, "channel should be in OPEN state") + suite.Require().Equal(mock.UpgradeVersion, channel.Version, "version should be correctly upgraded") + suite.Require().Equal(mock.UpgradeVersion, path.EndpointB.GetChannel().Version, "version should be correctly upgraded") + suite.Require().Equal(uint64(5), channel.UpgradeSequence, "upgrade sequence should be incremented") + suite.Require().Equal(uint64(5), path.EndpointB.GetChannel().UpgradeSequence, "upgrade sequence should be incremented on counterparty") + }) +} + func (suite *KeeperTestSuite) TestWriteUpgradeTry() { var ( path *ibctesting.Path @@ -2329,38 +2440,17 @@ func (suite *KeeperTestSuite) TestChanUpgradeCrossingHelloWithHistoricalProofs() var path *ibctesting.Path testCases := []struct { - name string - sequenceFn func() - malleate func() - expError error + name string + malleate func() + expError error }{ { "success", func() {}, - func() {}, nil, }, - { - // ChainA(Sequence: 0), ChainB(Sequence 4) - // ChainA.INIT(Sequence: 1) - // ChainB.INIT(Sequence: 5) - // ChainA.TRY(Sequence: 5) // fastforward - // ChainB.TRY(ErrorReceipt: 4) - "fails due to mismatch in upgrade sequences", - func() { - channel := path.EndpointB.GetChannel() - channel.UpgradeSequence = 4 - path.EndpointB.SetChannel(channel) - }, - func() { - channel := path.EndpointA.GetChannel() - suite.Require().Equal(uint64(5), channel.UpgradeSequence) - }, - types.NewUpgradeError(4, types.ErrInvalidUpgradeSequence), - }, { "counterparty (chain B) has already progressed to ACK step", - func() {}, func() { err := path.EndpointB.ChanUpgradeAck() suite.Require().NoError(err) @@ -2383,8 +2473,6 @@ func (suite *KeeperTestSuite) TestChanUpgradeCrossingHelloWithHistoricalProofs() err := path.EndpointA.ChanUpgradeInit() suite.Require().NoError(err) - tc.sequenceFn() - err = path.EndpointB.ChanUpgradeInit() suite.Require().NoError(err) From da7629fabc67a6e05977f1e73f5c83a973e1d431 Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Fri, 15 Dec 2023 23:20:03 +0100 Subject: [PATCH 07/11] relocate comment --- modules/core/04-channel/keeper/upgrade_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index a258dba8d34..3510981475e 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -277,12 +277,12 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { }, types.ErrIncompatibleCounterpartyUpgrade, }, - // ChainA(Sequence: 0, ics20-2), ChainB(Sequence: 4, ics20-3) - // ChainA.INIT(Sequence: 1) - // ChainB.INIT(Sequence: 5) - // ChainA.TRY => error (incompatible versions) - // ChainB.TRY(ErrorReceipt: 4) { + // ChainA(Sequence: 0, ics20-2), ChainB(Sequence: 4, ics20-3) + // ChainA.INIT(Sequence: 1) + // ChainB.INIT(Sequence: 5) + // ChainA.TRY => error (incompatible versions) + // ChainB.TRY(ErrorReceipt: 4) "crossing hellos: upgrade starts with mismatching upgrade sequences and try fails on counterparty due to incompatible version", func() { channel := path.EndpointB.GetChannel() From 57ab98b22cdfffefaa610945f332fb6ad7bd23f6 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Tue, 19 Dec 2023 11:03:43 +0100 Subject: [PATCH 08/11] fix: defer incrementing acc sequencing such that is happens on both success and failure tx execution --- testing/chain.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/testing/chain.go b/testing/chain.go index 857f73782ed..ccaebff7475 100644 --- a/testing/chain.go +++ b/testing/chain.go @@ -344,6 +344,14 @@ func (chain *TestChain) SendMsgs(msgs ...sdk.Msg) (*abci.ExecTxResult, error) { // ensure the chain has the latest time chain.Coordinator.UpdateTimeForChain(chain) + // increment acc sequence regardless of success or failure tx execution + defer func() { + err := chain.SenderAccount.SetSequence(chain.SenderAccount.GetSequence() + 1) + if err != nil { + panic(err) + } + }() + resp, err := simapp.SignAndDeliver( chain.TB, chain.TxConfig, @@ -370,12 +378,6 @@ func (chain *TestChain) SendMsgs(msgs ...sdk.Msg) (*abci.ExecTxResult, error) { return txResult, fmt.Errorf("%s/%d: %q", txResult.Codespace, txResult.Code, txResult.Log) } - // increment sequence for successful transaction execution - err = chain.SenderAccount.SetSequence(chain.SenderAccount.GetSequence() + 1) - if err != nil { - return nil, err - } - chain.Coordinator.IncrementTime() return txResult, nil From a5174d1cade59fcc745d1241da0ea0247e4dd190 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 20 Dec 2023 09:59:14 +0100 Subject: [PATCH 09/11] chore: update version strings in comments --- 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 7c04b377ad5..6970f4342c3 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -256,7 +256,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { types.NewUpgradeError(5, types.ErrInvalidUpgradeSequence), }, { - // ChainA(Sequence: 0, ics20-2), ChainB(Sequence: 0, ics20-3) + // ChainA(Sequence: 0, mock-version-v2), ChainB(Sequence: 0, mock-version-v3) // ChainA.INIT(Sequence: 1) // ChainB.INIT(Sequence: 1) // ChainA.TRY => error (incompatible versions) @@ -278,7 +278,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { types.ErrIncompatibleCounterpartyUpgrade, }, { - // ChainA(Sequence: 0, ics20-2), ChainB(Sequence: 4, ics20-3) + // ChainA(Sequence: 0, mock-version-v2), ChainB(Sequence: 4, mock-version-v3) // ChainA.INIT(Sequence: 1) // ChainB.INIT(Sequence: 5) // ChainA.TRY => error (incompatible versions) From 39b726277999da6b3366f67d6eab35a588054dea Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 20 Dec 2023 10:24:04 +0100 Subject: [PATCH 10/11] test: wrap test code in suite.Run and add additional assertions --- .../core/04-channel/keeper/upgrade_test.go | 92 +++++++++++++------ 1 file changed, 65 insertions(+), 27 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index 6970f4342c3..30034ae9bc1 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -371,44 +371,82 @@ func (suite *KeeperTestSuite) TestChanUpgradeTry() { // ChainA.Confirm => Success // ChainB.Open => Success func (suite *KeeperTestSuite) TestChanUpgrade_CrossingHellos_UpgradeSucceeds_AfterCancel() { - path := ibctesting.NewPath(suite.chainA, suite.chainB) - suite.coordinator.Setup(path) + var path *ibctesting.Path - path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion - path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion + suite.Run("setup path", func() { + path = ibctesting.NewPath(suite.chainA, suite.chainB) + suite.coordinator.Setup(path) - suite.Require().NoError(path.EndpointA.ChanUpgradeInit()) + path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion + path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion + }) - channel := path.EndpointB.GetChannel() - channel.UpgradeSequence = 4 - path.EndpointB.SetChannel(channel) + suite.Run("chainA upgrade init", func() { + err := path.EndpointA.ChanUpgradeInit() + suite.Require().NoError(err) + }) - suite.Require().NoError(path.EndpointB.ChanUpgradeInit()) - suite.Require().NoError(path.EndpointB.ChanUpgradeTry()) + suite.Run("set chainB upgrade sequence ahead of counterparty", func() { + channel := path.EndpointB.GetChannel() + channel.UpgradeSequence = 4 + path.EndpointB.SetChannel(channel) + }) - suite.Require().NoError(path.EndpointA.ChanUpgradeCancel()) - channel = path.EndpointA.GetChannel() - suite.Require().Equal(types.OPEN, channel.State) - suite.Require().Equal(uint64(4), channel.UpgradeSequence) + suite.Run("chainB upgrade init (crossing hello)", func() { + err := path.EndpointB.ChanUpgradeInit() + suite.Require().NoError(err) + }) - suite.Require().NoError(path.EndpointA.ChanUpgradeTry()) - channel = path.EndpointA.GetChannel() - suite.Require().Equal(types.FLUSHING, channel.State) - suite.Require().Equal(uint64(5), channel.UpgradeSequence) + suite.Run("chainB upgrade try fails with invalid sequence", func() { + err := path.EndpointB.ChanUpgradeTry() + suite.Require().NoError(err) - suite.T().Run("successfully completes upgrade", func(t *testing.T) { - suite.Require().NoError(path.EndpointB.ChanUpgradeAck()) - suite.Require().NoError(path.EndpointA.ChanUpgradeConfirm()) - suite.Require().NoError(path.EndpointB.ChanUpgradeOpen()) + errorReceipt, found := path.EndpointB.Chain.GetSimApp().GetIBCKeeper().ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + suite.Require().True(found) + suite.Require().Equal(uint64(4), errorReceipt.Sequence) }) - suite.T().Run("channel in expected state", func(t *testing.T) { + suite.Run("cancel upgrade on chainA and fast forward upgrade sequence", func() { + err := path.EndpointA.ChanUpgradeCancel() + suite.Require().NoError(err) + channel := path.EndpointA.GetChannel() - suite.Require().Equal(types.OPEN, channel.State, "channel should be in OPEN state") - suite.Require().Equal(mock.UpgradeVersion, channel.Version, "version should be correctly upgraded") + suite.Require().Equal(types.OPEN, channel.State) + suite.Require().Equal(uint64(4), channel.UpgradeSequence) + }) + + suite.Run("try chainA upgrade now succeeds with synchronized upgrade sequences", func() { + err := path.EndpointA.ChanUpgradeTry() + suite.Require().NoError(err) + + channel := path.EndpointA.GetChannel() + suite.Require().Equal(types.FLUSHING, channel.State) + suite.Require().Equal(uint64(5), channel.UpgradeSequence) + }) + + suite.Run("upgrade handshake completes successfully", func() { + err := path.EndpointB.ChanUpgradeAck() + suite.Require().NoError(err) + + err = path.EndpointA.ChanUpgradeConfirm() + suite.Require().NoError(err) + + err = path.EndpointB.ChanUpgradeOpen() + suite.Require().NoError(err) + }) + + suite.Run("assert successful upgrade expected channel state", func() { + channelA := path.EndpointA.GetChannel() + suite.Require().Equal(types.OPEN, channelA.State, "channel should be in OPEN state") + suite.Require().Equal(mock.UpgradeVersion, channelA.Version, "version should be correctly upgraded") suite.Require().Equal(mock.UpgradeVersion, path.EndpointB.GetChannel().Version, "version should be correctly upgraded") - suite.Require().Equal(uint64(5), channel.UpgradeSequence, "upgrade sequence should be incremented") - suite.Require().Equal(uint64(5), path.EndpointB.GetChannel().UpgradeSequence, "upgrade sequence should be incremented on counterparty") + suite.Require().Equal(uint64(5), channelA.UpgradeSequence, "upgrade sequence should be incremented") + + channelB := path.EndpointB.GetChannel() + suite.Require().Equal(types.OPEN, channelB.State, "channel should be in OPEN state") + suite.Require().Equal(mock.UpgradeVersion, channelB.Version, "version should be correctly upgraded") + suite.Require().Equal(mock.UpgradeVersion, channelB.Version, "version should be correctly upgraded") + suite.Require().Equal(uint64(5), channelB.UpgradeSequence, "upgrade sequence should be incremented") }) } From aefb902417a54b8c30eba453d7e42a39d0947161 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 20 Dec 2023 10:56:13 +0100 Subject: [PATCH 11/11] refactor: adapt crossing hellos cancellation test case to use suite.Run and add more assertions --- .../core/04-channel/keeper/upgrade_test.go | 142 ++++++++++++------ 1 file changed, 96 insertions(+), 46 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index 30034ae9bc1..1846a8b6538 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -462,65 +462,115 @@ func (suite *KeeperTestSuite) TestChanUpgrade_CrossingHellos_UpgradeSucceeds_Aft // ChainA.Confirm => Success // ChainB.Open => Success func (suite *KeeperTestSuite) TestChanUpgrade_CrossingHellos_UpgradeSucceeds_AfterCancelErrors() { - path := ibctesting.NewPath(suite.chainA, suite.chainB) - suite.coordinator.Setup(path) + var ( + historicalChannelProof []byte + historicalUpgradeProof []byte + proofHeight clienttypes.Height + path *ibctesting.Path + ) - path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion - path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion + suite.Run("setup path", func() { + path = ibctesting.NewPath(suite.chainA, suite.chainB) + suite.coordinator.Setup(path) - suite.Require().NoError(path.EndpointA.ChanUpgradeInit()) + path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion + path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion + }) - channel := path.EndpointB.GetChannel() - channel.UpgradeSequence = 4 - path.EndpointB.SetChannel(channel) + suite.Run("chainA upgrade init", func() { + err := path.EndpointA.ChanUpgradeInit() + suite.Require().NoError(err) - suite.Require().NoError(path.EndpointB.ChanUpgradeInit()) + channel := path.EndpointA.GetChannel() + suite.Require().Equal(uint64(1), channel.UpgradeSequence) + }) - suite.coordinator.CommitBlock(suite.chainA, suite.chainB) - suite.Require().NoError(path.EndpointB.UpdateClient()) + suite.Run("set chainB upgrade sequence ahead of counterparty", func() { + channel := path.EndpointB.GetChannel() + channel.UpgradeSequence = 4 + path.EndpointB.SetChannel(channel) + }) - // use proofs when chain A has not executed TRY yet and use them when executing TRY on chain B - historicalChannelProof, historicalUpgradeProof, proofHeight := path.EndpointA.QueryChannelUpgradeProof() - - suite.Require().NoError(path.EndpointA.ChanUpgradeTry()) - _, _, err := suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.ChanUpgradeTry( - suite.chainB.GetContext(), - path.EndpointB.ChannelConfig.PortID, - path.EndpointB.ChannelID, - path.EndpointB.GetChannelUpgrade().Fields.ConnectionHops, - path.EndpointA.GetChannelUpgrade().Fields, - 1, - historicalChannelProof, - historicalUpgradeProof, - proofHeight, - ) - suite.Require().Error(err) - suite.assertUpgradeError(err, types.NewUpgradeError(4, types.ErrInvalidUpgradeSequence)) - errorReceipt := err.(*types.UpgradeError).GetErrorReceipt() - suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, errorReceipt) - suite.coordinator.CommitBlock(suite.chainB) + suite.Run("chainB upgrade init (crossing hello)", func() { + err := path.EndpointB.ChanUpgradeInit() + suite.Require().NoError(err) - err = path.EndpointA.ChanUpgradeCancel() - suite.Require().Error(err) - suite.Require().ErrorContains(err, "invalid upgrade sequence") + channel := path.EndpointB.GetChannel() + suite.Require().Equal(uint64(5), channel.UpgradeSequence) + }) - channel = path.EndpointA.GetChannel() - suite.Require().Equal(types.FLUSHING, channel.State) - suite.Require().Equal(uint64(5), channel.UpgradeSequence) + suite.Run("query proofs at chainA upgrade sequence 1", func() { + // commit block and update client on chainB + suite.coordinator.CommitBlock(suite.chainA, suite.chainB) + suite.Require().NoError(path.EndpointB.UpdateClient()) + // use proofs when chain A has not executed TRY yet and use them when executing TRY on chain B + historicalChannelProof, historicalUpgradeProof, proofHeight = path.EndpointA.QueryChannelUpgradeProof() + }) - suite.T().Run("successfully completes upgrade", func(t *testing.T) { - suite.Require().NoError(path.EndpointB.ChanUpgradeAck()) - suite.Require().NoError(path.EndpointA.ChanUpgradeConfirm()) - suite.Require().NoError(path.EndpointB.ChanUpgradeOpen()) + suite.Run("chainA upgrade try (fast-forwards sequence)", func() { + err := path.EndpointA.ChanUpgradeTry() + suite.Require().NoError(err) + + channel := path.EndpointA.GetChannel() + suite.Require().Equal(uint64(5), channel.UpgradeSequence) }) - suite.T().Run("channel in expected state", func(t *testing.T) { + suite.Run("chainB upgrade try fails with written error receipt (4)", func() { + // NOTE: ante handlers are bypassed here and the handler is invoked directly. + // Thus, we set the upgrade error receipt explicitly below + _, _, err := suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.ChanUpgradeTry( + suite.chainB.GetContext(), + path.EndpointB.ChannelConfig.PortID, + path.EndpointB.ChannelID, + path.EndpointB.GetChannelUpgrade().Fields.ConnectionHops, + path.EndpointA.GetChannelUpgrade().Fields, + 1, // proofs queried at chainA upgrade sequence 1 + historicalChannelProof, + historicalUpgradeProof, + proofHeight, + ) + suite.Require().Error(err) + suite.assertUpgradeError(err, types.NewUpgradeError(4, types.ErrInvalidUpgradeSequence)) + + errorReceipt := err.(*types.UpgradeError).GetErrorReceipt() + suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, errorReceipt) + suite.coordinator.CommitBlock(suite.chainB) + }) + + suite.Run("chainA upgrade cancellation fails for invalid sequence", func() { + err := path.EndpointA.ChanUpgradeCancel() + suite.Require().Error(err) + suite.Require().ErrorContains(err, "invalid upgrade sequence") + + // assert channel remains in flushing state at upgrade sequence 5 channel := path.EndpointA.GetChannel() - suite.Require().Equal(types.OPEN, channel.State, "channel should be in OPEN state") - suite.Require().Equal(mock.UpgradeVersion, channel.Version, "version should be correctly upgraded") + suite.Require().Equal(types.FLUSHING, channel.State) + suite.Require().Equal(uint64(5), channel.UpgradeSequence) + }) + + suite.Run("upgrade handshake completes successfully", func() { + err := path.EndpointB.ChanUpgradeAck() + suite.Require().NoError(err) + + err = path.EndpointA.ChanUpgradeConfirm() + suite.Require().NoError(err) + + err = path.EndpointB.ChanUpgradeOpen() + suite.Require().NoError(err) + }) + + suite.Run("assert successful upgrade expected channel state", func() { + channelA := path.EndpointA.GetChannel() + suite.Require().Equal(types.OPEN, channelA.State, "channel should be in OPEN state") + suite.Require().Equal(mock.UpgradeVersion, channelA.Version, "version should be correctly upgraded") suite.Require().Equal(mock.UpgradeVersion, path.EndpointB.GetChannel().Version, "version should be correctly upgraded") - suite.Require().Equal(uint64(5), channel.UpgradeSequence, "upgrade sequence should be incremented") - suite.Require().Equal(uint64(5), path.EndpointB.GetChannel().UpgradeSequence, "upgrade sequence should be incremented on counterparty") + suite.Require().Equal(uint64(5), channelA.UpgradeSequence, "upgrade sequence should be incremented") + + channelB := path.EndpointB.GetChannel() + suite.Require().Equal(types.OPEN, channelB.State, "channel should be in OPEN state") + suite.Require().Equal(mock.UpgradeVersion, channelB.Version, "version should be correctly upgraded") + suite.Require().Equal(mock.UpgradeVersion, channelB.Version, "version should be correctly upgraded") + suite.Require().Equal(uint64(5), channelB.UpgradeSequence, "upgrade sequence should be incremented") }) }