From 2423f9e360f76ec4a3a44525f7a202df176e2629 Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 22 Nov 2023 15:32:21 +0000 Subject: [PATCH 1/2] test: adding test which verifies that an upgrade can be completed after a previous failed upgrade attempt has been cancelled --- modules/capability/module.go | 2 +- .../core/04-channel/keeper/upgrade_test.go | 71 +++++++++++++++++++ 2 files changed, 72 insertions(+), 1 deletion(-) diff --git a/modules/capability/module.go b/modules/capability/module.go index 786e41cf6e8..5f048a3b838 100644 --- a/modules/capability/module.go +++ b/modules/capability/module.go @@ -47,7 +47,7 @@ func NewAppModuleBasic(cdc codec.Codec) AppModuleBasic { } // Name returns the capability module's name. -func (am AppModuleBasic) Name() string { +func (AppModuleBasic) Name() string { return types.ModuleName } diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index 7a76c23560f..4e070d5d167 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -3,6 +3,7 @@ package keeper_test import ( "fmt" "math" + "testing" errorsmod "cosmossdk.io/errors" @@ -1374,6 +1375,76 @@ func (suite *KeeperTestSuite) TestChanUpgradeCancel() { } } +// TestChanUpgrade_UpgradeSucceeds_AfterCancel verifies that if upgrade sequences +// become out of sync, the upgrade can still be performed successfully after the upgrade is cancelled. +func (suite *KeeperTestSuite) TestChanUpgrade_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()) + + // cause the upgrade to fail on chain b so an error receipt is written. + // if the counterparty (chain A) upgrade sequence is less than the current sequence, (chain B) + // an upgrade error will be returned by chain B during ChanUpgradeTry. + channel := path.EndpointA.GetChannel() + channel.UpgradeSequence = 1 + path.EndpointA.SetChannel(channel) + + channel = path.EndpointB.GetChannel() + channel.UpgradeSequence = 2 + path.EndpointB.SetChannel(channel) + + suite.Require().NoError(path.EndpointA.UpdateClient()) + suite.Require().NoError(path.EndpointB.UpdateClient()) + + // error receipt is written to chain B here. + suite.Require().NoError(path.EndpointB.ChanUpgradeTry()) + + suite.Require().NoError(path.EndpointA.UpdateClient()) + + suite.T().Run("error receipt written", func(t *testing.T) { + _, ok := suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + suite.Require().True(ok) + }) + + suite.T().Run("upgrade cancelled successfully", func(t *testing.T) { + upgradeErrorReceiptKey := host.ChannelUpgradeErrorKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + errorReceiptProof, proofHeight := suite.chainB.QueryProof(upgradeErrorReceiptKey) + + errorReceipt, ok := suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + suite.Require().True(ok) + + err := suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.ChanUpgradeCancel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, errorReceipt, errorReceiptProof, proofHeight, true) + suite.Require().NoError(err) + + // need to explicitly call WriteUpgradeOpenChannel as this usually would happen in the msg server layer. + suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.WriteUpgradeCancelChannel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, errorReceipt) + + channel := path.EndpointA.GetChannel() + suite.Require().Equal(types.OPEN, channel.State) + suite.Require().Equal(uint64(2), channel.UpgradeSequence) + }) + + suite.T().Run("successfully completes upgrade", func(t *testing.T) { + suite.Require().NoError(path.EndpointA.ChanUpgradeInit()) + suite.Require().NoError(path.EndpointB.ChanUpgradeTry()) + suite.Require().NoError(path.EndpointA.ChanUpgradeAck()) + suite.Require().NoError(path.EndpointB.ChanUpgradeConfirm()) + suite.Require().NoError(path.EndpointA.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(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") + }) +} + func (suite *KeeperTestSuite) TestWriteUpgradeCancelChannel() { var path *ibctesting.Path From ed0e98ab1dceac55f7430992cb72965544cc414b Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 29 Nov 2023 10:52:16 +0000 Subject: [PATCH 2/2] chore: pr feedback --- modules/core/04-channel/keeper/upgrade_test.go | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index ee1e2b1f525..8c80f826ed0 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -1407,8 +1407,10 @@ func (suite *KeeperTestSuite) TestChanUpgrade_UpgradeSucceeds_AfterCancel() { suite.Require().NoError(path.EndpointA.UpdateClient()) + var errorReceipt types.ErrorReceipt suite.T().Run("error receipt written", func(t *testing.T) { - _, ok := suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + var ok bool + errorReceipt, ok = suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) suite.Require().True(ok) }) @@ -1416,9 +1418,6 @@ func (suite *KeeperTestSuite) TestChanUpgrade_UpgradeSucceeds_AfterCancel() { upgradeErrorReceiptKey := host.ChannelUpgradeErrorKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) errorReceiptProof, proofHeight := suite.chainB.QueryProof(upgradeErrorReceiptKey) - errorReceipt, ok := suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) - suite.Require().True(ok) - err := suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.ChanUpgradeCancel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, errorReceipt, errorReceiptProof, proofHeight, true) suite.Require().NoError(err) @@ -1427,7 +1426,10 @@ func (suite *KeeperTestSuite) TestChanUpgrade_UpgradeSucceeds_AfterCancel() { channel := path.EndpointA.GetChannel() suite.Require().Equal(types.OPEN, channel.State) - suite.Require().Equal(uint64(2), channel.UpgradeSequence) + + suite.T().Run("verify upgrade sequences are synced", func(t *testing.T) { + suite.Require().Equal(uint64(2), channel.UpgradeSequence) + }) }) suite.T().Run("successfully completes upgrade", func(t *testing.T) { @@ -1442,6 +1444,7 @@ func (suite *KeeperTestSuite) TestChanUpgrade_UpgradeSucceeds_AfterCancel() { 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(3), channel.UpgradeSequence, "upgrade sequence should be incremented") suite.Require().Equal(uint64(3), path.EndpointB.GetChannel().UpgradeSequence, "upgrade sequence should be incremented on counterparty") })