From 5cd004e342928658053434c50307587965c18b5a Mon Sep 17 00:00:00 2001 From: chatton Date: Thu, 1 Jun 2023 16:40:10 +0100 Subject: [PATCH 01/20] wip: adding restoreChannel and writeErrorReceipt functions --- modules/core/04-channel/keeper/keeper.go | 12 +++++++ modules/core/04-channel/keeper/upgrade.go | 44 +++++++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/modules/core/04-channel/keeper/keeper.go b/modules/core/04-channel/keeper/keeper.go index 137559e5d9f..0221dbfa4ac 100644 --- a/modules/core/04-channel/keeper/keeper.go +++ b/modules/core/04-channel/keeper/keeper.go @@ -533,6 +533,12 @@ func (k Keeper) SetCounterpartyLastPacketSequence(ctx sdk.Context, portID, chann store.Set(host.ChannelCounterpartyLastPacketSequenceKey(portID, channelID), bz) } +// deleteCounterpartyLastPacketSequence deletes the counterparty last packet sequence from the store. +func (k Keeper) deleteCounterpartyLastPacketSequence(ctx sdk.Context, portID, channelID string) { + store := ctx.KVStore(k.storeKey) + store.Delete(host.ChannelCounterpartyLastPacketSequenceKey(portID, channelID)) +} + // SetUpgrade sets the proposed upgrade using the provided port and channel identifiers. func (k Keeper) SetUpgrade(ctx sdk.Context, portID, channelID string, upgrade types.Upgrade) { store := ctx.KVStore(k.storeKey) @@ -540,6 +546,12 @@ func (k Keeper) SetUpgrade(ctx sdk.Context, portID, channelID string, upgrade ty store.Set(host.ChannelUpgradeKey(portID, channelID), bz) } +// deleteUpgrade deletes the upgrade for the provided port and channel identifiers. +func (k Keeper) deleteUpgrade(ctx sdk.Context, portID, channelID string) { + store := ctx.KVStore(k.storeKey) + store.Delete(host.ChannelUpgradeKey(portID, channelID)) +} + // ValidateUpgradeFields validates the proposed upgrade fields against the existing channel. // It returns an error if the following constraints are not met: // - there exists at least one valid proposed change to the existing channel fields diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index c101ab01c60..7923a54ba59 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -107,6 +107,50 @@ func (k Keeper) WriteUpgradeTryChannel( emitChannelUpgradeTryEvent(ctx, portID, channelID, channel, proposedUpgrade) } +// abortHandshake will restore the channel state and flush status to their pre-upgrade state so that upgrade is aborted. +// any unnecessary state is deleted. An error receipt is written, and the OnChanUpgradeRestore callback is called. +func (k Keeper) abortHandshake(ctx sdk.Context, portID, channelID string, upgradeError types.UpgradeError) error { + if err := k.restoreChannel(ctx, portID, channelID); err != nil { + return err + } + + if err := k.writeErrorReceipt(ctx, portID, channelID, upgradeError); err != nil { + return err + } + + + // TODO: callback execution + // cbs.OnChanUpgradeRestore() + + return nil +} + +// restoreChannel will restore the channel state and flush status to their pre-upgrade state so that upgrade is aborted +// it write an error receipt to state so counterparty can restore as well. +func (k Keeper) restoreChannel(ctx sdk.Context, portID, channelID string) error { + channel, found := k.GetChannel(ctx, portID, channelID) + if !found { + return errorsmod.Wrapf(types.ErrChannelNotFound, "port ID (%s) channel ID (%s)", portID, channelID) + } + + channel.State = types.OPEN + channel.FlushStatus = types.NOTINFLUSH + + k.SetChannel(ctx, portID, channelID, channel) + + // delete state associated with upgrade which is no longer required. + k.deleteUpgrade(ctx, portID, channelID) + k.deleteCounterpartyLastPacketSequence(ctx, portID, channelID) + + return nil +} + +func (k Keeper) writeErrorReceipt(ctx sdk.Context, portID, channelID string, upgradeError types.UpgradeError) error { + k.SetUpgradeErrorReceipt(ctx, portID, channelID, upgradeError.GetErrorReceipt()) + // TODO: emit events + return nil +} + // constructProposedUpgrade returns the proposed upgrade from the provided arguments. func (k Keeper) constructProposedUpgrade(ctx sdk.Context, portID, channelID string, fields types.UpgradeFields, upgradeTimeout types.Timeout) (types.Upgrade, error) { seq, found := k.GetNextSequenceSend(ctx, portID, channelID) From d01355f01f62b612eac965cc0b327447d6c7dd66 Mon Sep 17 00:00:00 2001 From: chatton Date: Tue, 6 Jun 2023 16:28:42 +0100 Subject: [PATCH 02/20] emit error receipt events on abort --- modules/core/04-channel/keeper/events.go | 2 -- modules/core/04-channel/keeper/upgrade.go | 20 +++++++++++++++----- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/modules/core/04-channel/keeper/events.go b/modules/core/04-channel/keeper/events.go index 7f1936d3230..f23fb1283d9 100644 --- a/modules/core/04-channel/keeper/events.go +++ b/modules/core/04-channel/keeper/events.go @@ -366,8 +366,6 @@ func emitChannelUpgradeOpenEvent(ctx sdk.Context, portID string, channelID strin } // emitErrorReceiptEvent emits an error receipt event -// -//lint:ignore U1000 Ignore unused function temporarily for debugging func emitErrorReceiptEvent(ctx sdk.Context, portID string, channelID string, currentChannel types.Channel, upgrade types.Upgrade, err error) { ctx.EventManager().EmitEvents(sdk.Events{ sdk.NewEvent( diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index b48c9232fe5..8a4f8dc8399 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -111,15 +111,19 @@ func (k Keeper) WriteUpgradeTryChannel( // abortHandshake will restore the channel state and flush status to their pre-upgrade state so that upgrade is aborted. // any unnecessary state is deleted. An error receipt is written, and the OnChanUpgradeRestore callback is called. -func (k Keeper) abortHandshake(ctx sdk.Context, portID, channelID string, upgradeError types.UpgradeError) error { +func (k Keeper) abortHandshake(ctx sdk.Context, portID, channelID string, upgradeError *types.UpgradeError) error { if err := k.restoreChannel(ctx, portID, channelID); err != nil { return err } - if err := k.writeErrorReceipt(ctx, portID, channelID, upgradeError); err != nil { - return err + upgrade, found := k.GetUpgrade(ctx, portID, channelID) + if !found { + return errorsmod.Wrapf(types.ErrUpgradeNotFound, "port ID (%s) channel ID (%s)", portID, channelID) } + if err := k.writeErrorReceipt(ctx, portID, channelID, upgrade, upgradeError); err != nil { + return err + } // TODO: callback execution // cbs.OnChanUpgradeRestore() @@ -148,9 +152,15 @@ func (k Keeper) restoreChannel(ctx sdk.Context, portID, channelID string) error } // writeErrorReceipt will write an error receipt from the provided UpgradeError. -func (k Keeper) writeErrorReceipt(ctx sdk.Context, portID, channelID string, upgradeError types.UpgradeError) error { +func (k Keeper) writeErrorReceipt(ctx sdk.Context, portID, channelID string, upgrade types.Upgrade, upgradeError *types.UpgradeError) error { + channel, found := k.GetChannel(ctx, portID, channelID) + if !found { + return errorsmod.Wrapf(types.ErrChannelNotFound, "port ID (%s) channel ID (%s)", portID, channelID) + } + k.SetUpgradeErrorReceipt(ctx, portID, channelID, upgradeError.GetErrorReceipt()) - // TODO: emit events + emitErrorReceiptEvent(ctx, portID, channelID, channel, upgrade, upgradeError) + return nil } From 6f7940b68f4dbdc500612505f0420cccf87d1d3b Mon Sep 17 00:00:00 2001 From: chatton Date: Tue, 6 Jun 2023 16:31:15 +0100 Subject: [PATCH 03/20] remove unused function --- modules/core/04-channel/keeper/keeper.go | 31 ------------------------ 1 file changed, 31 deletions(-) diff --git a/modules/core/04-channel/keeper/keeper.go b/modules/core/04-channel/keeper/keeper.go index e603aa46946..e48db3b5d88 100644 --- a/modules/core/04-channel/keeper/keeper.go +++ b/modules/core/04-channel/keeper/keeper.go @@ -2,7 +2,6 @@ package keeper import ( "encoding/binary" - "reflect" "strconv" "strings" @@ -552,36 +551,6 @@ func (k Keeper) deleteUpgrade(ctx sdk.Context, portID, channelID string) { store.Delete(host.ChannelUpgradeKey(portID, channelID)) } -// ValidateUpgradeFields validates the proposed upgrade fields against the existing channel. -// It returns an error if the following constraints are not met: -// - there exists at least one valid proposed change to the existing channel fields -// - the proposed order is a subset of the existing order -// - the proposed connection hops do not exist -// - the proposed version is non-empty (checked in UpgradeFields.ValidateBasic()) -// - the proposed connection hops are not open -func (k Keeper) ValidateUpgradeFields(ctx sdk.Context, proposedUpgrade types.UpgradeFields, currentChannel types.Channel) error { - currentFields := extractUpgradeFields(currentChannel) - - if reflect.DeepEqual(proposedUpgrade, currentFields) { - return errorsmod.Wrap(types.ErrChannelExists, "existing channel end is identical to proposed upgrade channel end") - } - - connectionID := proposedUpgrade.ConnectionHops[0] - connection, err := k.GetConnection(ctx, connectionID) - if err != nil { - return errorsmod.Wrapf(connectiontypes.ErrConnectionNotFound, "failed to retrieve connection: %s", connectionID) - } - - if connection.GetState() != int32(connectiontypes.OPEN) { - return errorsmod.Wrapf( - connectiontypes.ErrInvalidConnectionState, - "connection state is not OPEN (got %s)", connectiontypes.State(connection.GetState()).String(), - ) - } - - return nil -} - // common functionality for IteratePacketCommitment and IteratePacketAcknowledgement func (k Keeper) iterateHashes(ctx sdk.Context, iterator db.Iterator, cb func(portID, channelID string, sequence uint64, hash []byte) bool) { defer sdk.LogDeferred(ctx.Logger(), func() error { return iterator.Close() }) From 6d50848a97d5fb14da1d3e13f799f0e1e04b6623 Mon Sep 17 00:00:00 2001 From: chatton Date: Tue, 6 Jun 2023 16:49:02 +0100 Subject: [PATCH 04/20] added happy path test for abort --- modules/core/04-channel/keeper/export_test.go | 5 ++ modules/core/04-channel/keeper/upgrade.go | 13 +++-- .../core/04-channel/keeper/upgrade_test.go | 49 +++++++++++++++++++ 3 files changed, 62 insertions(+), 5 deletions(-) diff --git a/modules/core/04-channel/keeper/export_test.go b/modules/core/04-channel/keeper/export_test.go index 7199dc2f89a..b5cd95f9ae3 100644 --- a/modules/core/04-channel/keeper/export_test.go +++ b/modules/core/04-channel/keeper/export_test.go @@ -14,3 +14,8 @@ import ( func (k Keeper) ValidateUpgradeFields(ctx sdk.Context, proposedUpgrade types.UpgradeFields, currentChannel types.Channel) error { return k.validateUpgradeFields(ctx, proposedUpgrade, currentChannel) } + +// ValidateUpgradeFields is a wrapper around validateUpgradeFields to allow the function to be directly called in tests. +func (k Keeper) AbortHandshake(ctx sdk.Context, portID, channelID string, upgradeError *types.UpgradeError) error { + return k.abortHandshake(ctx, portID, channelID, upgradeError) +} diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 8a4f8dc8399..4a429381b00 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -112,19 +112,22 @@ func (k Keeper) WriteUpgradeTryChannel( // abortHandshake will restore the channel state and flush status to their pre-upgrade state so that upgrade is aborted. // any unnecessary state is deleted. An error receipt is written, and the OnChanUpgradeRestore callback is called. func (k Keeper) abortHandshake(ctx sdk.Context, portID, channelID string, upgradeError *types.UpgradeError) error { - if err := k.restoreChannel(ctx, portID, channelID); err != nil { - return err - } - upgrade, found := k.GetUpgrade(ctx, portID, channelID) if !found { return errorsmod.Wrapf(types.ErrUpgradeNotFound, "port ID (%s) channel ID (%s)", portID, channelID) } - if err := k.writeErrorReceipt(ctx, portID, channelID, upgrade, upgradeError); err != nil { + if err := k.restoreChannel(ctx, portID, channelID); err != nil { return err } + // TODO: could we want to abort but have upgrade error be non-nil? + if upgradeError != nil { + if err := k.writeErrorReceipt(ctx, portID, channelID, upgrade, upgradeError); err != nil { + return err + } + } + // TODO: callback execution // cbs.OnChanUpgradeRestore() diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index f879431a299..cb46624f141 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -203,3 +203,52 @@ func (suite *KeeperTestSuite) TestValidateProposedUpgradeFields() { }) } } + +func (suite *KeeperTestSuite) TestAbortHandshake() { + var ( + path *ibctesting.Path + upgradeError *types.UpgradeError + ) + + tests := []struct { + name string + malleate func() + expPass bool + }{ + { + name: "success", + malleate: func() {}, + expPass: true, + }, + } + + for _, tc := range tests { + tc := tc + suite.Run(tc.name, func() { + suite.SetupTest() + path = ibctesting.NewPath(suite.chainA, suite.chainB) + suite.coordinator.Setup(path) + + channelKeeper := suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper + + path.EndpointA.ChannelConfig.Version = mock.Version + "-v2" + + suite.Require().NoError(path.EndpointA.ChanUpgradeInit()) + + path.EndpointA.Chain.Coordinator.CommitBlock(path.EndpointA.Chain) + + suite.Require().NoError(path.EndpointA.UpdateClient()) + suite.Require().NoError(path.EndpointB.UpdateClient()) + + tc.malleate() + + err := channelKeeper.AbortHandshake(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, upgradeError) + + if tc.expPass { + suite.Require().NoError(err) + } else { + suite.Require().Error(err) + } + }) + } +} From 795cd05f3012fa876b5fb4c053fb95a98088166c Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 7 Jun 2023 09:58:09 +0100 Subject: [PATCH 05/20] fleshed out TestAbortHandshake --- .../core/04-channel/keeper/upgrade_test.go | 67 ++++++++++++++++--- 1 file changed, 59 insertions(+), 8 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index cb46624f141..f442f3c6fa7 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -204,21 +204,41 @@ func (suite *KeeperTestSuite) TestValidateProposedUpgradeFields() { } } +// TestAbortHandshake tests that when the channel handshake is aborted, the channel state +// is restored the previous state and that an error receipt is written, and upgrade state which +// is no longer required is deleted. func (suite *KeeperTestSuite) TestAbortHandshake() { var ( - path *ibctesting.Path - upgradeError *types.UpgradeError + path *ibctesting.Path ) tests := []struct { - name string - malleate func() - expPass bool + name string + malleate func() + expPass bool + channelPresent bool }{ { - name: "success", - malleate: func() {}, - expPass: true, + name: "success", + malleate: func() {}, + expPass: true, + channelPresent: true, + }, + { + name: "upgrade does not exist", + malleate: func() { + suite.chainA.DeleteKey(host.ChannelUpgradeKey(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)) + }, + expPass: false, + channelPresent: true, + }, + { + name: "channel does not exist", + malleate: func() { + suite.chainA.DeleteKey(host.ChannelKey(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)) + }, + expPass: false, + channelPresent: false, }, } @@ -240,14 +260,45 @@ func (suite *KeeperTestSuite) TestAbortHandshake() { suite.Require().NoError(path.EndpointA.UpdateClient()) suite.Require().NoError(path.EndpointB.UpdateClient()) + upgradeError := types.NewUpgradeError(1, types.ErrInvalidChannel) + tc.malleate() err := channelKeeper.AbortHandshake(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, upgradeError) if tc.expPass { suite.Require().NoError(err) + + channel, found := channelKeeper.GetChannel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + suite.Require().True(found, "channel should be found") + + suite.Require().Equal(types.OPEN, channel.State, "channel state should be OPEN") + suite.Require().Equal(types.NOTINFLUSH, channel.FlushStatus, "channel flush status should be NOTINFLUSH") + + _, found = channelKeeper.GetUpgrade(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + suite.Require().False(found, "upgrade info should be deleted") + + errorReceipt, found := channelKeeper.GetUpgradeErrorReceipt(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + if upgradeError != nil { + suite.Require().True(found, "error receipt should be found") + suite.Require().Equal(upgradeError.GetErrorReceipt(), errorReceipt, "error receipt should be stored") + } + + _, found = channelKeeper.GetCounterpartyLastPacketSequence(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + suite.Require().False(found, "counterparty last packet sequence should be found") + } else { suite.Require().Error(err) + channel, found := channelKeeper.GetChannel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + if tc.channelPresent { + suite.Require().True(found, "channel should be found") + suite.Require().Equal(types.INITUPGRADE, channel.State, "channel state should not be restored to OPEN") + } + + _, found = channelKeeper.GetUpgradeErrorReceipt(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + suite.Require().False(found, "error receipt should not be written") + + // TODO: assertion that GetCounterpartyLastPacketSequence is present and correct } }) } From dfbc5d4fe8ce683ea8d9503a2f616888053feee2 Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 7 Jun 2023 10:02:08 +0100 Subject: [PATCH 06/20] corrected docstring and removed unrequired checks in test --- modules/core/04-channel/keeper/export_test.go | 2 +- modules/core/04-channel/keeper/upgrade_test.go | 5 ----- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/modules/core/04-channel/keeper/export_test.go b/modules/core/04-channel/keeper/export_test.go index b5cd95f9ae3..4bca0ddaab2 100644 --- a/modules/core/04-channel/keeper/export_test.go +++ b/modules/core/04-channel/keeper/export_test.go @@ -15,7 +15,7 @@ func (k Keeper) ValidateUpgradeFields(ctx sdk.Context, proposedUpgrade types.Upg return k.validateUpgradeFields(ctx, proposedUpgrade, currentChannel) } -// ValidateUpgradeFields is a wrapper around validateUpgradeFields to allow the function to be directly called in tests. +// AbortHandshake is a wrapper around abortHandshake to allow the function to be directly called in tests. func (k Keeper) AbortHandshake(ctx sdk.Context, portID, channelID string, upgradeError *types.UpgradeError) error { return k.abortHandshake(ctx, portID, channelID, upgradeError) } diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index f442f3c6fa7..f87e06d29b5 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -255,11 +255,6 @@ func (suite *KeeperTestSuite) TestAbortHandshake() { suite.Require().NoError(path.EndpointA.ChanUpgradeInit()) - path.EndpointA.Chain.Coordinator.CommitBlock(path.EndpointA.Chain) - - suite.Require().NoError(path.EndpointA.UpdateClient()) - suite.Require().NoError(path.EndpointB.UpdateClient()) - upgradeError := types.NewUpgradeError(1, types.ErrInvalidChannel) tc.malleate() From d4295145b0e3c91100719fd095e6a8e6e174553d Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 7 Jun 2023 10:31:14 +0100 Subject: [PATCH 07/20] chore: added implementation of WriteUpgradeCancelChannel --- modules/core/04-channel/keeper/events.go | 21 +++++++++++ modules/core/04-channel/keeper/keeper.go | 12 +++++++ modules/core/04-channel/keeper/upgrade.go | 43 +++++++++++++++++++++++ modules/core/04-channel/types/errors.go | 1 + modules/core/04-channel/types/events.go | 1 + 5 files changed, 78 insertions(+) diff --git a/modules/core/04-channel/keeper/events.go b/modules/core/04-channel/keeper/events.go index 7f1936d3230..0240456cfa9 100644 --- a/modules/core/04-channel/keeper/events.go +++ b/modules/core/04-channel/keeper/events.go @@ -388,3 +388,24 @@ func emitErrorReceiptEvent(ctx sdk.Context, portID string, channelID string, cur ), }) } + +// emitUpgradedCancelledEvent emits an upgraded cancelled event. +func emitUpgradedCancelledEvent(ctx sdk.Context, portID string, channelID string, currentChannel types.Channel, upgrade types.Upgrade) { + ctx.EventManager().EmitEvents(sdk.Events{ + sdk.NewEvent( + types.EventTypeChannelUpgradeCancelled, + sdk.NewAttribute(types.AttributeKeyPortID, portID), + sdk.NewAttribute(types.AttributeKeyChannelID, channelID), + sdk.NewAttribute(types.AttributeCounterpartyPortID, currentChannel.Counterparty.PortId), + sdk.NewAttribute(types.AttributeCounterpartyChannelID, currentChannel.Counterparty.ChannelId), + sdk.NewAttribute(types.AttributeKeyUpgradeConnectionHops, upgrade.Fields.ConnectionHops[0]), + sdk.NewAttribute(types.AttributeKeyUpgradeVersion, upgrade.Fields.Version), + sdk.NewAttribute(types.AttributeKeyUpgradeOrdering, upgrade.Fields.Ordering.String()), + sdk.NewAttribute(types.AttributeKeyUpgradeSequence, fmt.Sprintf("%d", currentChannel.UpgradeSequence)), + ), + sdk.NewEvent( + sdk.EventTypeMessage, + sdk.NewAttribute(sdk.AttributeKeyModule, types.AttributeValueCategory), + ), + }) +} diff --git a/modules/core/04-channel/keeper/keeper.go b/modules/core/04-channel/keeper/keeper.go index 220a9994fdf..e48db3b5d88 100644 --- a/modules/core/04-channel/keeper/keeper.go +++ b/modules/core/04-channel/keeper/keeper.go @@ -532,6 +532,12 @@ func (k Keeper) SetCounterpartyLastPacketSequence(ctx sdk.Context, portID, chann store.Set(host.ChannelCounterpartyLastPacketSequenceKey(portID, channelID), bz) } +// deleteCounterpartyLastPacketSequence deletes the counterparty last packet sequence from the store. +func (k Keeper) deleteCounterpartyLastPacketSequence(ctx sdk.Context, portID, channelID string) { + store := ctx.KVStore(k.storeKey) + store.Delete(host.ChannelCounterpartyLastPacketSequenceKey(portID, channelID)) +} + // SetUpgrade sets the proposed upgrade using the provided port and channel identifiers. func (k Keeper) SetUpgrade(ctx sdk.Context, portID, channelID string, upgrade types.Upgrade) { store := ctx.KVStore(k.storeKey) @@ -539,6 +545,12 @@ func (k Keeper) SetUpgrade(ctx sdk.Context, portID, channelID string, upgrade ty store.Set(host.ChannelUpgradeKey(portID, channelID), bz) } +// deleteUpgrade deletes the upgrade for the provided port and channel identifiers. +func (k Keeper) deleteUpgrade(ctx sdk.Context, portID, channelID string) { + store := ctx.KVStore(k.storeKey) + store.Delete(host.ChannelUpgradeKey(portID, channelID)) +} + // common functionality for IteratePacketCommitment and IteratePacketAcknowledgement func (k Keeper) iterateHashes(ctx sdk.Context, iterator db.Iterator, cb func(portID, channelID string, sequence uint64, hash []byte) bool) { defer sdk.LogDeferred(ctx.Logger(), func() error { return iterator.Close() }) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 825b3de00f5..d421ad29fde 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -110,6 +110,29 @@ func (k Keeper) WriteUpgradeTryChannel( emitChannelUpgradeTryEvent(ctx, portID, channelID, channel, proposedUpgrade) } +// WriteUpgradeCancelChannel writes a channel which has canceled the upgrade process. Auxiliary upgrade state is +// also deleted. +func (k Keeper) WriteUpgradeCancelChannel(ctx sdk.Context, portID, channelID string) error { + defer telemetry.IncrCounter(1, "ibc", "channel", "upgrade-cancel") + + channel, found := k.GetChannel(ctx, portID, channelID) + if !found { + panic(fmt.Sprintf("could not find existing channel when cancelling channel upgrade, channelID: %s, portID: %s", channelID, portID)) + } + + upgrade, found := k.GetUpgrade(ctx, portID, channelID) + if !found { + panic(fmt.Sprintf("could not find existing upgrade when cancelling channel upgrade, channelID: %s, portID: %s", channelID, portID)) + } + + if err := k.restoreChannel(ctx, portID, channelID); err != nil { + return errorsmod.Wrap(types.ErrUpgradeRestoreFailed, fmt.Sprintf("failed to restore channel, channelID: %s, portID: %s", channelID, portID)) + } + + emitUpgradedCancelledEvent(ctx, portID, channelID, channel, upgrade) + 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 @@ -265,3 +288,23 @@ func (k Keeper) constructProposedUpgrade(ctx sdk.Context, portID, channelID stri LatestSequenceSend: seq - 1, }, nil } + +// restoreChannel will restore the channel state and flush status to their pre-upgrade state so that upgrade is aborted +// it write an error receipt to state so counterparty can restore as well. +func (k Keeper) restoreChannel(ctx sdk.Context, portID, channelID string) error { + channel, found := k.GetChannel(ctx, portID, channelID) + if !found { + return errorsmod.Wrapf(types.ErrChannelNotFound, "port ID (%s) channel ID (%s)", portID, channelID) + } + + channel.State = types.OPEN + channel.FlushStatus = types.NOTINFLUSH + + k.SetChannel(ctx, portID, channelID, channel) + + // delete state associated with upgrade which is no longer required. + k.deleteUpgrade(ctx, portID, channelID) + k.deleteCounterpartyLastPacketSequence(ctx, portID, channelID) + + return nil +} diff --git a/modules/core/04-channel/types/errors.go b/modules/core/04-channel/types/errors.go index 1f3ff7890f8..a4815ce1db3 100644 --- a/modules/core/04-channel/types/errors.go +++ b/modules/core/04-channel/types/errors.go @@ -45,4 +45,5 @@ var ( ErrInvalidUpgradeSequence = errorsmod.Register(SubModuleName, 29, "invalid upgrade sequence") ErrUpgradeNotFound = errorsmod.Register(SubModuleName, 30, "upgrade not found") ErrIncompatibleCounterpartyUpgrade = errorsmod.Register(SubModuleName, 31, "incompatible counterparty upgrade") + ErrUpgradeRestoreFailed = errorsmod.Register(SubModuleName, 32, "restore failed") ) diff --git a/modules/core/04-channel/types/events.go b/modules/core/04-channel/types/events.go index e47a8e4d2af..9c7ebf028af 100644 --- a/modules/core/04-channel/types/events.go +++ b/modules/core/04-channel/types/events.go @@ -62,6 +62,7 @@ var ( EventTypeChannelUpgradeTry = "channel_upgrade_try" EventTypeChannelUpgradeAck = "channel_upgrade_ack" EventTypeChannelUpgradeOpen = "channel_upgrade_open" + EventTypeChannelUpgradeCancelled = "channel_upgrade_cancelled" AttributeValueCategory = fmt.Sprintf("%s_%s", ibcexported.ModuleName, SubModuleName) ) From c1e05b822c628f044f7e979a78678df033ab3826 Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 7 Jun 2023 10:39:55 +0100 Subject: [PATCH 08/20] fix linting --- modules/core/04-channel/keeper/upgrade.go | 1 - modules/core/04-channel/keeper/upgrade_test.go | 4 +--- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 7f0f53ed14f..475f6f84f22 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -265,7 +265,6 @@ func (k Keeper) constructProposedUpgrade(ctx sdk.Context, portID, channelID stri }, nil } - // abortHandshake will restore the channel state and flush status to their pre-upgrade state so that upgrade is aborted. // any unnecessary state is deleted. An error receipt is written, and the OnChanUpgradeRestore callback is called. func (k Keeper) abortHandshake(ctx sdk.Context, portID, channelID string, upgradeError *types.UpgradeError) error { diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index 9fddbf58358..d3a62c925cd 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -394,9 +394,7 @@ func (suite *KeeperTestSuite) assertUpgradeError(actualError, expError error) { // is restored the previous state and that an error receipt is written, and upgrade state which // is no longer required is deleted. func (suite *KeeperTestSuite) TestAbortHandshake() { - var ( - path *ibctesting.Path - ) + var path *ibctesting.Path tests := []struct { name string From 91605c134fb87fdb209e824a59e2e14b62031341 Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 7 Jun 2023 10:56:04 +0100 Subject: [PATCH 09/20] addressing PR feedback --- modules/core/04-channel/keeper/upgrade.go | 11 ++++---- .../core/04-channel/keeper/upgrade_test.go | 28 +++++++++++++------ modules/core/04-channel/types/errors.go | 1 + 3 files changed, 26 insertions(+), 14 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 475f6f84f22..249579d6231 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -268,6 +268,10 @@ func (k Keeper) constructProposedUpgrade(ctx sdk.Context, portID, channelID stri // abortHandshake will restore the channel state and flush status to their pre-upgrade state so that upgrade is aborted. // any unnecessary state is deleted. An error receipt is written, and the OnChanUpgradeRestore callback is called. func (k Keeper) abortHandshake(ctx sdk.Context, portID, channelID string, upgradeError *types.UpgradeError) error { + if upgradeError == nil { + return errorsmod.Wrap(types.ErrInvalidUpgradeError, "cannot abort upgrade handshake with nil error") + } + upgrade, found := k.GetUpgrade(ctx, portID, channelID) if !found { return errorsmod.Wrapf(types.ErrUpgradeNotFound, "port ID (%s) channel ID (%s)", portID, channelID) @@ -277,11 +281,8 @@ func (k Keeper) abortHandshake(ctx sdk.Context, portID, channelID string, upgrad return err } - // TODO: could we want to abort but have upgrade error be non-nil? - if upgradeError != nil { - if err := k.writeErrorReceipt(ctx, portID, channelID, upgrade, upgradeError); err != nil { - return err - } + if err := k.writeErrorReceipt(ctx, portID, channelID, upgrade, upgradeError); err != nil { + return err } // TODO: callback execution diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index d3a62c925cd..b21950ef5b6 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -394,7 +394,10 @@ func (suite *KeeperTestSuite) assertUpgradeError(actualError, expError error) { // is restored the previous state and that an error receipt is written, and upgrade state which // is no longer required is deleted. func (suite *KeeperTestSuite) TestAbortHandshake() { - var path *ibctesting.Path + var ( + path *ibctesting.Path + upgradeError *types.UpgradeError + ) tests := []struct { name string @@ -424,6 +427,14 @@ func (suite *KeeperTestSuite) TestAbortHandshake() { expPass: false, channelPresent: false, }, + { + name: "fails with nil upgrade error", + malleate: func() { + upgradeError = nil + }, + expPass: false, + channelPresent: true, + }, } for _, tc := range tests { @@ -439,7 +450,7 @@ func (suite *KeeperTestSuite) TestAbortHandshake() { suite.Require().NoError(path.EndpointA.ChanUpgradeInit()) - upgradeError := types.NewUpgradeError(1, types.ErrInvalidChannel) + upgradeError = types.NewUpgradeError(1, types.ErrInvalidChannel) tc.malleate() @@ -451,31 +462,30 @@ func (suite *KeeperTestSuite) TestAbortHandshake() { channel, found := channelKeeper.GetChannel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) suite.Require().True(found, "channel should be found") - suite.Require().Equal(types.OPEN, channel.State, "channel state should be OPEN") - suite.Require().Equal(types.NOTINFLUSH, channel.FlushStatus, "channel flush status should be NOTINFLUSH") - + suite.Require().Equal(types.OPEN, channel.State, "channel state should be %s", types.OPEN.String()) + suite.Require().Equal(types.NOTINFLUSH, channel.FlushStatus, "channel flush status should be %s", types.NOTINFLUSH.String()) _, found = channelKeeper.GetUpgrade(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) suite.Require().False(found, "upgrade info should be deleted") errorReceipt, found := channelKeeper.GetUpgradeErrorReceipt(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) if upgradeError != nil { suite.Require().True(found, "error receipt should be found") - suite.Require().Equal(upgradeError.GetErrorReceipt(), errorReceipt, "error receipt should be stored") + suite.Require().Equal(upgradeError.GetErrorReceipt(), errorReceipt, "error receipt does not match expected error receipt") } _, found = channelKeeper.GetCounterpartyLastPacketSequence(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) - suite.Require().False(found, "counterparty last packet sequence should be found") + suite.Require().False(found, "counterparty last packet sequence should not be found") } else { suite.Require().Error(err) channel, found := channelKeeper.GetChannel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) if tc.channelPresent { suite.Require().True(found, "channel should be found") - suite.Require().Equal(types.INITUPGRADE, channel.State, "channel state should not be restored to OPEN") + suite.Require().Equal(types.INITUPGRADE, channel.State, "channel state should not be restored to %s", types.INITUPGRADE.String()) } _, found = channelKeeper.GetUpgradeErrorReceipt(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) - suite.Require().False(found, "error receipt should not be written") + suite.Require().False(found, "error receipt should not be found") // TODO: assertion that GetCounterpartyLastPacketSequence is present and correct } diff --git a/modules/core/04-channel/types/errors.go b/modules/core/04-channel/types/errors.go index 1f3ff7890f8..f73bfd3f326 100644 --- a/modules/core/04-channel/types/errors.go +++ b/modules/core/04-channel/types/errors.go @@ -45,4 +45,5 @@ var ( ErrInvalidUpgradeSequence = errorsmod.Register(SubModuleName, 29, "invalid upgrade sequence") ErrUpgradeNotFound = errorsmod.Register(SubModuleName, 30, "upgrade not found") ErrIncompatibleCounterpartyUpgrade = errorsmod.Register(SubModuleName, 31, "incompatible counterparty upgrade") + ErrInvalidUpgradeError = errorsmod.Register(SubModuleName, 32, "invalid upgrade error") ) From eaa2b914d2920d83a492965e1ddb87d920373e4a Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 7 Jun 2023 13:33:59 +0100 Subject: [PATCH 10/20] addressing PR feedback --- modules/core/04-channel/keeper/upgrade.go | 2 +- .../core/04-channel/keeper/upgrade_test.go | 44 +++++++++---------- 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 249579d6231..f54ac9d12b4 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -292,7 +292,7 @@ func (k Keeper) abortHandshake(ctx sdk.Context, portID, channelID string, upgrad } // restoreChannel will restore the channel state and flush status to their pre-upgrade state so that upgrade is aborted -// it write an error receipt to state so counterparty can restore as well. +// It will write an error receipt to state so that the counterparty can restore as well. func (k Keeper) restoreChannel(ctx sdk.Context, portID, channelID string) error { channel, found := k.GetChannel(ctx, portID, channelID) if !found { diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index b21950ef5b6..b2f01d22027 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -400,40 +400,35 @@ func (suite *KeeperTestSuite) TestAbortHandshake() { ) tests := []struct { - name string - malleate func() - expPass bool - channelPresent bool + name string + malleate func() + expPass bool }{ { - name: "success", - malleate: func() {}, - expPass: true, - channelPresent: true, + name: "success", + malleate: func() {}, + expPass: true, }, { name: "upgrade does not exist", malleate: func() { suite.chainA.DeleteKey(host.ChannelUpgradeKey(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)) }, - expPass: false, - channelPresent: true, + expPass: false, }, { name: "channel does not exist", malleate: func() { suite.chainA.DeleteKey(host.ChannelKey(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)) }, - expPass: false, - channelPresent: false, + expPass: false, }, { name: "fails with nil upgrade error", malleate: func() { upgradeError = nil }, - expPass: false, - channelPresent: true, + expPass: false, }, } @@ -446,10 +441,13 @@ func (suite *KeeperTestSuite) TestAbortHandshake() { channelKeeper := suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper - path.EndpointA.ChannelConfig.Version = mock.Version + "-v2" - + path.EndpointA.ChannelConfig.Version = mock.UpgradeVersion suite.Require().NoError(path.EndpointA.ChanUpgradeInit()) + // fetch the upgrade before abort for assertions later on. + actualUpgrade, ok := channelKeeper.GetUpgrade(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + suite.Require().True(ok, "upgrade should be found") + upgradeError = types.NewUpgradeError(1, types.ErrInvalidChannel) tc.malleate() @@ -468,10 +466,8 @@ func (suite *KeeperTestSuite) TestAbortHandshake() { suite.Require().False(found, "upgrade info should be deleted") errorReceipt, found := channelKeeper.GetUpgradeErrorReceipt(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) - if upgradeError != nil { - suite.Require().True(found, "error receipt should be found") - suite.Require().Equal(upgradeError.GetErrorReceipt(), errorReceipt, "error receipt does not match expected error receipt") - } + suite.Require().True(found, "error receipt should be found") + suite.Require().Equal(upgradeError.GetErrorReceipt(), errorReceipt, "error receipt does not match expected error receipt") _, found = channelKeeper.GetCounterpartyLastPacketSequence(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) suite.Require().False(found, "counterparty last packet sequence should not be found") @@ -479,14 +475,18 @@ func (suite *KeeperTestSuite) TestAbortHandshake() { } else { suite.Require().Error(err) channel, found := channelKeeper.GetChannel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) - if tc.channelPresent { - suite.Require().True(found, "channel should be found") + if found { // test cases uses a channel that exists suite.Require().Equal(types.INITUPGRADE, channel.State, "channel state should not be restored to %s", types.INITUPGRADE.String()) } _, found = channelKeeper.GetUpgradeErrorReceipt(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) suite.Require().False(found, "error receipt should not be found") + upgrade, found := channelKeeper.GetUpgrade(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + if found { // this should be all test cases except for when the upgrade is explicitly deleted. + suite.Require().Equal(actualUpgrade, upgrade, "upgrade info should not be deleted") + } + // TODO: assertion that GetCounterpartyLastPacketSequence is present and correct } }) From 7d2c3d41da36ae2a3d4a2a724db9da2d3ef2bbdf Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 7 Jun 2023 15:06:06 +0100 Subject: [PATCH 11/20] chore: adding implementation of ChanUpgradeCancel --- modules/core/04-channel/keeper/upgrade.go | 44 +++++++++++++++++++ modules/core/04-channel/types/errors.go | 1 + .../core/04-channel/types/expected_keepers.go | 10 +++++ 3 files changed, 55 insertions(+) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 9c82e0713c3..da9f33c424d 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -159,6 +159,50 @@ func (k Keeper) WriteUpgradeTryChannel( emitChannelUpgradeTryEvent(ctx, portID, channelID, channel, proposedUpgrade) } +// ChanUpgradeCancel is called by a module to cancel a channel upgrade that is in progress. +func (k Keeper) ChanUpgradeCancel(ctx sdk.Context, portID, channelID string, errorReceipt types.ErrorReceipt, errorReceiptProof []byte, proofHeight clienttypes.Height) error { + channel, found := k.GetChannel(ctx, portID, channelID) + if !found { + return errorsmod.Wrapf(types.ErrChannelNotFound, "port ID (%s) channel ID (%s)", portID, channelID) + } + + // the channel state must be in INITUPGRADE or TRYUPGRADE + if !collections.Contains(channel.State, []types.State{types.INITUPGRADE, types.TRYUPGRADE}) { + return errorsmod.Wrapf(types.ErrInvalidChannelState, "expected one of [%s, %s], got %s", types.INITUPGRADE, types.TRYUPGRADE, channel.State) + } + + // get underlying connection for proof verification + connection, err := k.GetConnection(ctx, channel.ConnectionHops[0]) + if err != nil { + return errorsmod.Wrap(err, "failed to retrieve connection using the channel connection hops") + } + + if err := k.connectionKeeper.VerifyChannelUpgradeError(ctx, connection, proofHeight, errorReceiptProof, portID, channelID, errorReceipt); err != nil { + return errorsmod.Wrap(err, "failed to verify counterparty error receipt") + } + + if isEmptyReceipt(errorReceipt) { + return errorsmod.Wrap(types.ErrInvalidUpgradeErrorReceipt, "empty error receipt") + } + + // If counterparty sequence is less than the current sequence, abort transaction since this error receipt is from a previous upgrade + // Otherwise, set the sequence to counterparty's error sequence+1 so that both sides start with a fresh sequence + currentSequence := channel.UpgradeSequence + if errorReceipt.Sequence >= currentSequence { + return errorsmod.Wrap(types.ErrInvalidUpgradeSequence, "error sequence must be less than current sequence") + } + + channel.UpgradeSequence = errorReceipt.Sequence + 1 + k.SetChannel(ctx, portID, channelID, channel) + + return nil +} + +// isEmptyReceipt returns true if the receipt is empty. +func isEmptyReceipt(receipt types.ErrorReceipt) bool { + return receipt == types.ErrorReceipt{} +} + // WriteUpgradeCancelChannel writes a channel which has canceled the upgrade process. Auxiliary upgrade state is // also deleted. func (k Keeper) WriteUpgradeCancelChannel(ctx sdk.Context, portID, channelID string) error { diff --git a/modules/core/04-channel/types/errors.go b/modules/core/04-channel/types/errors.go index f169c8a2a1f..0c5869811e9 100644 --- a/modules/core/04-channel/types/errors.go +++ b/modules/core/04-channel/types/errors.go @@ -47,4 +47,5 @@ var ( ErrIncompatibleCounterpartyUpgrade = errorsmod.Register(SubModuleName, 31, "incompatible counterparty upgrade") ErrInvalidUpgradeError = errorsmod.Register(SubModuleName, 32, "invalid upgrade error") ErrUpgradeRestoreFailed = errorsmod.Register(SubModuleName, 33, "restore failed") + ErrInvalidUpgradeErrorReceipt = errorsmod.Register(SubModuleName, 34, "invalid error receipt") ) diff --git a/modules/core/04-channel/types/expected_keepers.go b/modules/core/04-channel/types/expected_keepers.go index 85a9019e353..a06c9d027d9 100644 --- a/modules/core/04-channel/types/expected_keepers.go +++ b/modules/core/04-channel/types/expected_keepers.go @@ -5,6 +5,7 @@ import ( capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" connectiontypes "github.com/cosmos/ibc-go/v7/modules/core/03-connection/types" + channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types" "github.com/cosmos/ibc-go/v7/modules/core/exported" ) @@ -80,6 +81,15 @@ type ConnectionKeeper interface { channelID string, upgrade Upgrade, ) error + VerifyChannelUpgradeError( + ctx sdk.Context, + connection exported.ConnectionI, + height exported.Height, + proof []byte, + portID, + channelID string, + errorReceipt channeltypes.ErrorReceipt, + ) error } // PortKeeper expected account IBC port keeper From 759474f47abc44951f392b8bcb0fbf17d94bf8d4 Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 7 Jun 2023 15:36:29 +0100 Subject: [PATCH 12/20] added scaffolding for TestChanUpgradeCancel --- .../core/04-channel/keeper/upgrade_test.go | 51 +++++++++++++++++++ .../core/04-channel/types/expected_keepers.go | 3 +- 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index d7daa25d886..ea65294d864 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -732,3 +732,54 @@ func (suite *KeeperTestSuite) TestAbortHandshake() { }) } } + +func (suite *KeeperTestSuite) TestChanUpgradeCancel() { + var ( + path *ibctesting.Path + ) + tests := []struct { + name string + malleate func() + expPass bool + }{ + { + name: "success", + malleate: func() {}, + expPass: true, + }, + } + + for _, tc := range tests { + 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 + path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion + + err := path.EndpointA.ChanUpgradeInit() + suite.Require().NoError(err) + + suite.Require().NoError(path.EndpointB.UpdateClient()) + + err = path.EndpointB.ChanUpgradeTry() + suite.Require().NoError(err) + + suite.Require().NoError(path.EndpointA.UpdateClient()) + + channelAKeeper := suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper + + // TODO: populate these correctly + errorReceipt := types.ErrorReceipt{} + errReceiptProof := []byte{} + proofHeight := clienttypes.Height{} + + err = channelAKeeper.ChanUpgradeCancel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, errorReceipt, errReceiptProof, proofHeight) + suite.Require().NoError(err) + + }) + } +} diff --git a/modules/core/04-channel/types/expected_keepers.go b/modules/core/04-channel/types/expected_keepers.go index a06c9d027d9..0baa27e8f03 100644 --- a/modules/core/04-channel/types/expected_keepers.go +++ b/modules/core/04-channel/types/expected_keepers.go @@ -5,7 +5,6 @@ import ( capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" connectiontypes "github.com/cosmos/ibc-go/v7/modules/core/03-connection/types" - channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types" "github.com/cosmos/ibc-go/v7/modules/core/exported" ) @@ -88,7 +87,7 @@ type ConnectionKeeper interface { proof []byte, portID, channelID string, - errorReceipt channeltypes.ErrorReceipt, + errorReceipt ErrorReceipt, ) error } From 3916332cc3b0e14867cf92bf46ca998fe9c99146 Mon Sep 17 00:00:00 2001 From: chatton Date: Mon, 12 Jun 2023 12:03:07 +0100 Subject: [PATCH 13/20] addressing PR feedback --- modules/core/04-channel/keeper/export_test.go | 6 +++--- modules/core/04-channel/keeper/upgrade.go | 18 +++++++++++++++--- modules/core/04-channel/keeper/upgrade_test.go | 18 +++++++++++++++--- 3 files changed, 33 insertions(+), 9 deletions(-) diff --git a/modules/core/04-channel/keeper/export_test.go b/modules/core/04-channel/keeper/export_test.go index baa6bd95a91..0ff36d4355b 100644 --- a/modules/core/04-channel/keeper/export_test.go +++ b/modules/core/04-channel/keeper/export_test.go @@ -31,7 +31,7 @@ func (k Keeper) ValidateUpgradeFields(ctx sdk.Context, proposedUpgrade types.Upg return k.validateUpgradeFields(ctx, proposedUpgrade, currentChannel) } -// AbortHandshake is a wrapper around abortHandshake to allow the function to be directly called in tests. -func (k Keeper) AbortHandshake(ctx sdk.Context, portID, channelID string, upgradeError *types.UpgradeError) error { - return k.abortHandshake(ctx, portID, channelID, upgradeError) +// AbortUpgrade is a wrapper around abortUpgrade to allow the function to be directly called in tests. +func (k Keeper) AbortUpgrade(ctx sdk.Context, portID, channelID string, err error) error { + return k.abortUpgrade(ctx, portID, channelID, err) } diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 7e1fd4f122f..37fdab1af26 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -326,10 +326,10 @@ func (k Keeper) constructProposedUpgrade(ctx sdk.Context, portID, channelID stri }, nil } -// abortHandshake will restore the channel state and flush status to their pre-upgrade state so that upgrade is aborted. +// abortUpgrade will restore the channel state and flush status to their pre-upgrade state so that upgrade is aborted. // any unnecessary state is deleted. An error receipt is written, and the OnChanUpgradeRestore callback is called. -func (k Keeper) abortHandshake(ctx sdk.Context, portID, channelID string, upgradeError *types.UpgradeError) error { - if upgradeError == nil { +func (k Keeper) abortUpgrade(ctx sdk.Context, portID, channelID string, err error) error { + if err == nil { return errorsmod.Wrap(types.ErrInvalidUpgradeError, "cannot abort upgrade handshake with nil error") } @@ -342,6 +342,18 @@ func (k Keeper) abortHandshake(ctx sdk.Context, portID, channelID string, upgrad return err } + // in the case of application callbacks, the error may not be an upgrade error. + // in this case we need to construct one in order to write the error receipt. + upgradeError, ok := err.(*types.UpgradeError) + if !ok { + channel, found := k.GetChannel(ctx, portID, channelID) + if !found { + return errorsmod.Wrapf(types.ErrChannelNotFound, "port ID (%s) channel ID (%s)", portID, channelID) + } + + upgradeError = types.NewUpgradeError(channel.UpgradeSequence, err) + } + if err := k.writeErrorReceipt(ctx, portID, channelID, upgrade, upgradeError); err != nil { return err } diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index b795bafb7f3..4f202889940 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -614,7 +614,7 @@ func (suite *KeeperTestSuite) assertUpgradeError(actualError, expError error) { func (suite *KeeperTestSuite) TestAbortHandshake() { var ( path *ibctesting.Path - upgradeError *types.UpgradeError + upgradeError error ) tests := []struct { @@ -627,6 +627,15 @@ func (suite *KeeperTestSuite) TestAbortHandshake() { malleate: func() {}, expPass: true, }, + { + name: "regular error", + malleate: func() { + // in app callbacks error receipts should still be written if a regular error is returned. + // i.e. not an instance of `types.UpgradeError` + upgradeError = types.ErrInvalidUpgrade + }, + expPass: true, + }, { name: "upgrade does not exist", malleate: func() { @@ -670,7 +679,7 @@ func (suite *KeeperTestSuite) TestAbortHandshake() { tc.malleate() - err := channelKeeper.AbortHandshake(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, upgradeError) + err := channelKeeper.AbortUpgrade(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, upgradeError) if tc.expPass { suite.Require().NoError(err) @@ -685,7 +694,10 @@ func (suite *KeeperTestSuite) TestAbortHandshake() { errorReceipt, found := channelKeeper.GetUpgradeErrorReceipt(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) suite.Require().True(found, "error receipt should be found") - suite.Require().Equal(upgradeError.GetErrorReceipt(), errorReceipt, "error receipt does not match expected error receipt") + + if ue, ok := upgradeError.(*types.UpgradeError); ok { + suite.Require().Equal(ue.GetErrorReceipt(), errorReceipt, "error receipt does not match expected error receipt") + } _, found = channelKeeper.GetCounterpartyLastPacketSequence(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) suite.Require().False(found, "counterparty last packet sequence should not be found") From 811c4189a7855365e2ec54dda9c4bde4ee0edf2e Mon Sep 17 00:00:00 2001 From: chatton Date: Mon, 12 Jun 2023 13:00:42 +0100 Subject: [PATCH 14/20] addressing PR feedback --- modules/core/04-channel/keeper/events.go | 6 +++--- modules/core/04-channel/keeper/upgrade.go | 4 ++-- modules/core/04-channel/types/events.go | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/modules/core/04-channel/keeper/events.go b/modules/core/04-channel/keeper/events.go index 353a8670264..8390adfd88b 100644 --- a/modules/core/04-channel/keeper/events.go +++ b/modules/core/04-channel/keeper/events.go @@ -387,11 +387,11 @@ func emitErrorReceiptEvent(ctx sdk.Context, portID string, channelID string, cur }) } -// emitUpgradedCancelledEvent emits an upgraded cancelled event. -func emitUpgradedCancelledEvent(ctx sdk.Context, portID string, channelID string, currentChannel types.Channel, upgrade types.Upgrade) { +// emitChannelUpgradeCancelEvent emits an upgraded cancelled event. +func emitChannelUpgradeCancelEvent(ctx sdk.Context, portID string, channelID string, currentChannel types.Channel, upgrade types.Upgrade) { ctx.EventManager().EmitEvents(sdk.Events{ sdk.NewEvent( - types.EventTypeChannelUpgradeCancelled, + types.EventTypeChannelUpgradeCancel, sdk.NewAttribute(types.AttributeKeyPortID, portID), sdk.NewAttribute(types.AttributeKeyChannelID, channelID), sdk.NewAttribute(types.AttributeCounterpartyPortID, currentChannel.Counterparty.PortId), diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 2584602201c..cd0dcc7704e 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -160,7 +160,7 @@ func (k Keeper) WriteUpgradeCancelChannel(ctx sdk.Context, portID, channelID str channel, found := k.GetChannel(ctx, portID, channelID) if !found { - panic(fmt.Sprintf("could not find existing channel when cancelling channel upgrade, channelID: %s, portID: %s", channelID, portID)) + panic(fmt.Sprintf("could not find existing channel, channelID: %s, portID: %s", channelID, portID)) } upgrade, found := k.GetUpgrade(ctx, portID, channelID) @@ -172,7 +172,7 @@ func (k Keeper) WriteUpgradeCancelChannel(ctx sdk.Context, portID, channelID str return errorsmod.Wrap(types.ErrUpgradeRestoreFailed, fmt.Sprintf("failed to restore channel, channelID: %s, portID: %s", channelID, portID)) } - emitUpgradedCancelledEvent(ctx, portID, channelID, channel, upgrade) + emitChannelUpgradeCancelEvent(ctx, portID, channelID, channel, upgrade) return nil } diff --git a/modules/core/04-channel/types/events.go b/modules/core/04-channel/types/events.go index c951d7876cd..5141a27e4f3 100644 --- a/modules/core/04-channel/types/events.go +++ b/modules/core/04-channel/types/events.go @@ -61,8 +61,8 @@ var ( EventTypeChannelUpgradeInit = "channel_upgrade_init" EventTypeChannelUpgradeTry = "channel_upgrade_try" EventTypeChannelUpgradeAck = "channel_upgrade_ack" - EventTypeChannelUpgradeOpen = "channel_upgrade_open" - EventTypeChannelUpgradeCancelled = "channel_upgrade_cancelled" + EventTypeChannelUpgradeOpen = "channel_upgrade_open" + EventTypeChannelUpgradeCancel = "channel_upgrade_cancelled" AttributeValueCategory = fmt.Sprintf("%s_%s", ibcexported.ModuleName, SubModuleName) ) From 51e907065f1c2a47b0e88cf52b954a1ad9ccaa56 Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 14 Jun 2023 10:58:55 +0100 Subject: [PATCH 15/20] fix merge conflicts --- modules/core/04-channel/keeper/export_test.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/modules/core/04-channel/keeper/export_test.go b/modules/core/04-channel/keeper/export_test.go index 4a7cd48a989..d97fd5e5a67 100644 --- a/modules/core/04-channel/keeper/export_test.go +++ b/modules/core/04-channel/keeper/export_test.go @@ -31,13 +31,7 @@ func (k Keeper) ValidateUpgradeFields(ctx sdk.Context, proposedUpgrade types.Upg return k.validateUpgradeFields(ctx, proposedUpgrade, currentChannel) } -<<<<<<< HEAD -// AbortUpgrade is a wrapper around abortUpgrade to allow the function to be directly called in tests. -func (k Keeper) AbortUpgrade(ctx sdk.Context, portID, channelID string, err error) error { - return k.abortUpgrade(ctx, portID, channelID, err) -======= // WriteUpgradeOpenChannel is a wrapper around writeUpgradeOpenChannel to allow the function to be directly called in tests. func (k Keeper) WriteUpgradeOpenChannel(ctx sdk.Context, portID, channelID string) { k.writeUpgradeOpenChannel(ctx, portID, channelID) ->>>>>>> 04-channel-upgrades } From 3e0bf6ebcf8285fb721a55ea241f128805fb6b90 Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 14 Jun 2023 11:05:48 +0100 Subject: [PATCH 16/20] ran linter --- modules/core/04-channel/types/events.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/modules/core/04-channel/types/events.go b/modules/core/04-channel/types/events.go index 5141a27e4f3..d82703c40f3 100644 --- a/modules/core/04-channel/types/events.go +++ b/modules/core/04-channel/types/events.go @@ -51,16 +51,16 @@ const ( // IBC channel events vars var ( - EventTypeChannelOpenInit = "channel_open_init" - EventTypeChannelOpenTry = "channel_open_try" - EventTypeChannelOpenAck = "channel_open_ack" - EventTypeChannelOpenConfirm = "channel_open_confirm" - EventTypeChannelCloseInit = "channel_close_init" - EventTypeChannelCloseConfirm = "channel_close_confirm" - EventTypeChannelClosed = "channel_close" - EventTypeChannelUpgradeInit = "channel_upgrade_init" - EventTypeChannelUpgradeTry = "channel_upgrade_try" - EventTypeChannelUpgradeAck = "channel_upgrade_ack" + EventTypeChannelOpenInit = "channel_open_init" + EventTypeChannelOpenTry = "channel_open_try" + EventTypeChannelOpenAck = "channel_open_ack" + EventTypeChannelOpenConfirm = "channel_open_confirm" + EventTypeChannelCloseInit = "channel_close_init" + EventTypeChannelCloseConfirm = "channel_close_confirm" + EventTypeChannelClosed = "channel_close" + EventTypeChannelUpgradeInit = "channel_upgrade_init" + EventTypeChannelUpgradeTry = "channel_upgrade_try" + EventTypeChannelUpgradeAck = "channel_upgrade_ack" EventTypeChannelUpgradeOpen = "channel_upgrade_open" EventTypeChannelUpgradeCancel = "channel_upgrade_cancelled" From 098d0a31fc0449cd70d9bf96f254c02811b59c8e Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 14 Jun 2023 11:49:14 +0100 Subject: [PATCH 17/20] happy path test --- modules/core/04-channel/keeper/upgrade.go | 3 ++- .../core/04-channel/keeper/upgrade_test.go | 20 +++++++++++++------ 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index c69ec892328..df65fc0d6ad 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -259,7 +259,8 @@ func (k Keeper) ChanUpgradeCancel(ctx sdk.Context, portID, channelID string, err // If counterparty sequence is less than the current sequence, abort transaction since this error receipt is from a previous upgrade // Otherwise, set the sequence to counterparty's error sequence+1 so that both sides start with a fresh sequence currentSequence := channel.UpgradeSequence - if errorReceipt.Sequence >= currentSequence { + counterpartySequence := errorReceipt.Sequence + if counterpartySequence < currentSequence { return errorsmod.Wrap(types.ErrInvalidUpgradeSequence, "error sequence must be less than current sequence") } diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index 6f9dd60e22e..016170b4f32 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -4,6 +4,7 @@ import ( "fmt" errorsmod "cosmossdk.io/errors" + sdk "github.com/cosmos/cosmos-sdk/types" clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types" connectiontypes "github.com/cosmos/ibc-go/v7/modules/core/03-connection/types" @@ -909,6 +910,13 @@ func (suite *KeeperTestSuite) TestChanUpgradeCancel() { suite.Require().NoError(path.EndpointB.UpdateClient()) + // cause the upgrade to fail on chain b so an error receipt is written. + suite.chainB.GetSimApp().IBCMockModule.IBCApp.OnChanUpgradeTry = func( + ctx sdk.Context, portID, channelID string, order types.Order, connectionHops []string, counterpartyVersion string, + ) (string, error) { + return "", fmt.Errorf("mock app callback failed") + } + err = path.EndpointB.ChanUpgradeTry() suite.Require().NoError(err) @@ -916,14 +924,14 @@ func (suite *KeeperTestSuite) TestChanUpgradeCancel() { channelAKeeper := suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper - // TODO: populate these correctly - errorReceipt := types.ErrorReceipt{} - errReceiptProof := []byte{} - proofHeight := clienttypes.Height{} + upgradeErrorReceiptKey := host.ChannelUpgradeErrorKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + errorReceiptProof, proofHeight := suite.chainB.QueryProof(upgradeErrorReceiptKey) - err = channelAKeeper.ChanUpgradeCancel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, errorReceipt, errReceiptProof, proofHeight) - suite.Require().NoError(err) + errorReceipt, ok := suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + suite.Require().True(ok) + err = channelAKeeper.ChanUpgradeCancel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, errorReceipt, errorReceiptProof, proofHeight) + suite.Require().NoError(err) }) } } From 853eae266f906bc02a691eba7804e828db98755c Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 14 Jun 2023 14:28:31 +0100 Subject: [PATCH 18/20] adding tests for ChanUpgradeCancel --- modules/core/04-channel/keeper/upgrade.go | 16 ++-- .../core/04-channel/keeper/upgrade_test.go | 75 ++++++++++++++++--- 2 files changed, 71 insertions(+), 20 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index df65fc0d6ad..595ddc78d57 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -252,12 +252,8 @@ func (k Keeper) ChanUpgradeCancel(ctx sdk.Context, portID, channelID string, err return errorsmod.Wrap(err, "failed to verify counterparty error receipt") } - if isEmptyReceipt(errorReceipt) { - return errorsmod.Wrap(types.ErrInvalidUpgradeErrorReceipt, "empty error receipt") - } - - // If counterparty sequence is less than the current sequence, abort transaction since this error receipt is from a previous upgrade - // Otherwise, set the sequence to counterparty's error sequence+1 so that both sides start with a fresh sequence + // If counterparty sequence is less than the current sequence, abort the transaction since this error receipt is from a previous upgrade. + // Otherwise, increment the counterparty's error sequence by 1 so that both sides start with a fresh sequence. currentSequence := channel.UpgradeSequence counterpartySequence := errorReceipt.Sequence if counterpartySequence < currentSequence { @@ -270,11 +266,6 @@ func (k Keeper) ChanUpgradeCancel(ctx sdk.Context, portID, channelID string, err return nil } -// isEmptyReceipt returns true if the receipt is empty. -func isEmptyReceipt(receipt types.ErrorReceipt) bool { - return receipt == types.ErrorReceipt{} -} - // WriteUpgradeCancelChannel writes a channel which has canceled the upgrade process.Auxiliary upgrade state is // also deleted. func (k Keeper) WriteUpgradeCancelChannel(ctx sdk.Context, portID, channelID string) { @@ -290,8 +281,11 @@ func (k Keeper) WriteUpgradeCancelChannel(ctx sdk.Context, portID, channelID str panic(fmt.Sprintf("could not find existing channel when updating channel state, channelID: %s, portID: %s", channelID, portID)) } + previousState := channel.State + k.restoreChannel(ctx, portID, channelID, channel) + k.Logger(ctx).Info("channel state updated", "port-id", portID, "channel-id", channelID, "previous-state", previousState, "new-state", types.OPEN.String()) emitChannelUpgradeCancelEvent(ctx, portID, channelID, channel, upgrade) } diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index 016170b4f32..bc20e0fc2f0 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -880,17 +880,65 @@ func (suite *KeeperTestSuite) TestAbortHandshake() { func (suite *KeeperTestSuite) TestChanUpgradeCancel() { var ( - path *ibctesting.Path + path *ibctesting.Path + errorReceipt types.ErrorReceipt + errorReceiptProof []byte + proofHeight clienttypes.Height ) tests := []struct { name string malleate func() - expPass bool + expError error }{ { name: "success", malleate: func() {}, - expPass: true, + expError: nil, + }, + { + name: "invalid channel state", + malleate: func() { + channel := path.EndpointA.GetChannel() + channel.State = types.INIT + path.EndpointA.SetChannel(channel) + }, + expError: types.ErrInvalidChannelState, + }, + { + name: "channel not found", + malleate: func() { + path.EndpointA.Chain.DeleteKey(host.ChannelKey(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)) + }, + expError: types.ErrChannelNotFound, + }, + { + name: "connection not found", + malleate: func() { + channel := path.EndpointA.GetChannel() + channel.ConnectionHops = []string{"connection-100"} + path.EndpointA.SetChannel(channel) + }, + expError: connectiontypes.ErrConnectionNotFound, + }, + { + name: "counter party upgrade sequence less than current sequence", + malleate: func() { + 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) + + // the channel sequence will be 1 + errorReceipt.Sequence = 0 + + suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, errorReceipt) + + suite.coordinator.CommitBlock(suite.chainB) + suite.Require().NoError(path.EndpointA.UpdateClient()) + + upgradeErrorReceiptKey := host.ChannelUpgradeErrorKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + errorReceiptProof, proofHeight = suite.chainB.QueryProof(upgradeErrorReceiptKey) + }, + expError: types.ErrInvalidUpgradeSequence, }, } @@ -922,16 +970,25 @@ func (suite *KeeperTestSuite) TestChanUpgradeCancel() { suite.Require().NoError(path.EndpointA.UpdateClient()) - channelAKeeper := suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper - upgradeErrorReceiptKey := host.ChannelUpgradeErrorKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) - errorReceiptProof, proofHeight := suite.chainB.QueryProof(upgradeErrorReceiptKey) + errorReceiptProof, proofHeight = suite.chainB.QueryProof(upgradeErrorReceiptKey) - errorReceipt, 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) - err = channelAKeeper.ChanUpgradeCancel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, errorReceipt, errorReceiptProof, proofHeight) - suite.Require().NoError(err) + tc.malleate() + + err = suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.ChanUpgradeCancel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, errorReceipt, errorReceiptProof, proofHeight) + + expPass := tc.expError == nil + if expPass { + suite.Require().NoError(err) + channel := path.EndpointA.GetChannel() + suite.Require().Equal(errorReceipt.Sequence+1, channel.UpgradeSequence, "upgrade sequence should be incremented") + } else { + suite.Require().ErrorIs(tc.expError, err) + } }) } } From bf108561c807b7b5d13bfd7e3436524d0b13bb41 Mon Sep 17 00:00:00 2001 From: chatton Date: Fri, 16 Jun 2023 09:32:38 +0100 Subject: [PATCH 19/20] addresing PR feedback --- modules/core/04-channel/keeper/upgrade.go | 9 ++++++++- modules/core/04-channel/keeper/upgrade_test.go | 10 ++++------ modules/core/04-channel/types/errors.go | 1 - 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 595ddc78d57..a92ef651627 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -248,12 +248,19 @@ func (k Keeper) ChanUpgradeCancel(ctx sdk.Context, portID, channelID string, err return errorsmod.Wrap(err, "failed to retrieve connection using the channel connection hops") } + if connection.GetState() != int32(connectiontypes.OPEN) { + return errorsmod.Wrapf( + connectiontypes.ErrInvalidConnectionState, + "connection state is not OPEN (got %s)", connectiontypes.State(connection.GetState()).String(), + ) + } + if err := k.connectionKeeper.VerifyChannelUpgradeError(ctx, portID, channelID, connection, errorReceipt, errorReceiptProof, proofHeight); err != nil { return errorsmod.Wrap(err, "failed to verify counterparty error receipt") } // If counterparty sequence is less than the current sequence, abort the transaction since this error receipt is from a previous upgrade. - // Otherwise, increment the counterparty's error sequence by 1 so that both sides start with a fresh sequence. + // Otherwise, set our upgrade sequence to the counterparty's error sequence + 1 so that both sides start with a fresh sequence. currentSequence := channel.UpgradeSequence counterpartySequence := errorReceipt.Sequence if counterpartySequence < currentSequence { diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index bc20e0fc2f0..0c76c55d3b5 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -921,7 +921,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeCancel() { expError: connectiontypes.ErrConnectionNotFound, }, { - name: "counter party upgrade sequence less than current sequence", + name: "counter partyupgrade sequence less than current sequence", malleate: func() { var ok bool errorReceipt, ok = suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) @@ -953,8 +953,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeCancel() { path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion - err := path.EndpointA.ChanUpgradeInit() - suite.Require().NoError(err) + suite.Require().NoError(path.EndpointA.ChanUpgradeInit()) suite.Require().NoError(path.EndpointB.UpdateClient()) @@ -965,8 +964,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeCancel() { return "", fmt.Errorf("mock app callback failed") } - err = path.EndpointB.ChanUpgradeTry() - suite.Require().NoError(err) + suite.Require().NoError(path.EndpointB.ChanUpgradeTry()) suite.Require().NoError(path.EndpointA.UpdateClient()) @@ -979,7 +977,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeCancel() { tc.malleate() - err = suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.ChanUpgradeCancel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, errorReceipt, errorReceiptProof, proofHeight) + err := suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.ChanUpgradeCancel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, errorReceipt, errorReceiptProof, proofHeight) expPass := tc.expError == nil if expPass { diff --git a/modules/core/04-channel/types/errors.go b/modules/core/04-channel/types/errors.go index 8a3e628bba0..3b11df9b162 100644 --- a/modules/core/04-channel/types/errors.go +++ b/modules/core/04-channel/types/errors.go @@ -47,5 +47,4 @@ var ( ErrIncompatibleCounterpartyUpgrade = errorsmod.Register(SubModuleName, 31, "incompatible counterparty upgrade") ErrInvalidUpgradeError = errorsmod.Register(SubModuleName, 32, "invalid upgrade error") ErrInvalidFlushStatus = errorsmod.Register(SubModuleName, 33, "invalid flush status") - ErrInvalidUpgradeErrorReceipt = errorsmod.Register(SubModuleName, 34, "invalid error receipt") ) From 2f540ce06ad495701831efd23a312598bda2f5dc Mon Sep 17 00:00:00 2001 From: chatton Date: Fri, 16 Jun 2023 13:04:25 +0100 Subject: [PATCH 20/20] corrected assertion arguments --- modules/core/04-channel/keeper/upgrade_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index 0c76c55d3b5..44d32871742 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -985,7 +985,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeCancel() { channel := path.EndpointA.GetChannel() suite.Require().Equal(errorReceipt.Sequence+1, channel.UpgradeSequence, "upgrade sequence should be incremented") } else { - suite.Require().ErrorIs(tc.expError, err) + suite.Require().ErrorIs(err, tc.expError) } }) }