From b33f21f8ba961022865884ee021216600a355f33 Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Tue, 13 Jun 2023 18:26:18 +0300 Subject: [PATCH 1/8] Add ChanUpgradeOpen core handler. --- modules/core/04-channel/keeper/upgrade.go | 88 +++++++++++++++++ .../core/04-channel/keeper/upgrade_test.go | 99 +++++++++++++++++++ modules/core/04-channel/types/errors.go | 1 + 3 files changed, 188 insertions(+) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 660fb1f33b7..051da97ea01 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -597,6 +597,94 @@ func (k Keeper) startFlushUpgradeHandshake( return nil } +// ChanUpgradeOpen is called by a module to complete the channel upgrade handshake and move the channel back to an OPEN state. +// This method should only be called after both channels have flushed any in-flight packets. +func (k Keeper) ChanUpgradeOpen( + ctx sdk.Context, + portID, + channelID string, + counterpartyChannelState types.State, + proofChannel []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) + } + + if k.hasInflightPackets(ctx, portID, channelID) { + return errorsmod.Wrapf(types.ErrPendingInflightPackets, "port ID (%s) channel ID (%s)", portID, channelID) + } + + if !collections.Contains(channel.State, []types.State{types.TRYUPGRADE, types.ACKUPGRADE}) { + return errorsmod.Wrapf(types.ErrInvalidChannelState, "expected one of [%s, %s], got %s", types.TRYUPGRADE, types.ACKUPGRADE, channel.State) + } + + if channel.FlushStatus != types.FLUSHCOMPLETE { + return errorsmod.Wrapf(types.ErrInvalidFlushStatus, "expected %s, got %s", types.FLUSHCOMPLETE, channel.FlushStatus) + } + + 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 connection.GetState() != int32(connectiontypes.OPEN) { + return errorsmod.Wrapf(connectiontypes.ErrInvalidConnectionState, "connection state is not OPEN (got %s)", connectiontypes.State(connection.GetState()).String()) + } + + var counterpartyChannel types.Channel + counterpartyHops := []string{connection.GetCounterparty().GetConnectionID()} + switch counterpartyChannelState { + case types.OPEN: + upgrade, found := k.GetUpgrade(ctx, portID, channelID) + if !found { + return errorsmod.Wrapf(types.ErrUpgradeNotFound, "failed to retrieve channel upgrade: port ID (%s) channel ID (%s)", portID, channelID) + } + counterpartyChannel = types.Channel{ + State: types.OPEN, + Ordering: channel.Ordering, + ConnectionHops: upgrade.Fields.ConnectionHops, + Counterparty: types.NewCounterparty(portID, channelID), + Version: upgrade.Fields.GetVersion(), + UpgradeSequence: channel.UpgradeSequence, + FlushStatus: types.NOTINFLUSH, + } + + case types.TRYUPGRADE: + // If the counterparty is in TRYUPGRADE, then we must have gone through the ACKUPGRADE step. + if channel.State != types.ACKUPGRADE { + return errorsmod.Wrapf(types.ErrInvalidChannelState, "expected %s, got %s", types.ACKUPGRADE, channel.State) + } + counterpartyChannel = types.Channel{ + State: types.TRYUPGRADE, + Ordering: channel.Ordering, + ConnectionHops: counterpartyHops, + Counterparty: types.NewCounterparty(portID, channelID), + Version: channel.Version, + UpgradeSequence: channel.UpgradeSequence, + FlushStatus: types.FLUSHCOMPLETE, + } + + case types.ACKUPGRADE: + counterpartyChannel = types.Channel{ + State: types.ACKUPGRADE, + Ordering: channel.Ordering, + ConnectionHops: counterpartyHops, + Counterparty: types.NewCounterparty(portID, channelID), + Version: channel.Version, + UpgradeSequence: channel.UpgradeSequence, + FlushStatus: types.FLUSHCOMPLETE, + } + default: + panic(fmt.Sprintf("counterparty channel state should be in one of [%s, %s, %s]; got %s", types.TRYUPGRADE, types.ACKUPGRADE, types.OPEN, counterpartyChannelState)) + } + + return k.connectionKeeper.VerifyChannelState( + ctx, connection, proofHeight, proofChannel, portID, channelID, counterpartyChannel, + ) +} + // WriteUpgradeOpenChannel writes the agreed upon upgrade fields to the channel, sets the channel flush status to NOTINFLUSH and sets the channel state back to OPEN. This can be called in one of two cases: // - In the UpgradeAck step of the handshake if both sides have already flushed all in-flight packets. // - In the UpgradeOpen step of the handshake. diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index 0dc7c0c71c8..fb5ce7f7109 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -487,6 +487,105 @@ func (suite *KeeperTestSuite) TestChanUpgradeAck() { } } +func (suite *KeeperTestSuite) TestChanUpgradeOpen() { + var path *ibctesting.Path + testCases := []struct { + name string + malleate func() + expError error + }{ + { + "success, counterparty in TRYUPGRADE", + func() {}, + nil, + }, + // TODO: Rest of combinations for counterparty state. + { + "channel not found", + func() { + path.EndpointA.ChannelID = ibctesting.InvalidID + path.EndpointA.ChannelConfig.PortID = ibctesting.InvalidID + }, + types.ErrChannelNotFound, + }, + { + "in-flight packets still exist", + func() { + // TODO: + }, + types.ErrPendingInflightPackets, + }, + { + "flush status is not FLUSHCOMPLETE", + func() { + channel := path.EndpointB.GetChannel() + channel.FlushStatus = types.FLUSHING + path.EndpointB.SetChannel(channel) + }, + types.ErrInvalidFlushStatus, + }, + { + "connection not found", + func() { + channel := path.EndpointA.GetChannel() + channel.ConnectionHops = []string{"connection-100"} + path.EndpointA.SetChannel(channel) + }, + connectiontypes.ErrConnectionNotFound, + }, + { + "invalid connection state", + func() { + connectionEnd := path.EndpointA.GetConnection() + connectionEnd.State = connectiontypes.UNINITIALIZED + path.EndpointA.SetConnection(connectionEnd) + }, + connectiontypes.ErrInvalidConnectionState, + }, + } + + for _, tc := range testCases { + tc := tc + suite.Run(tc.name, func() { + expPass := tc.expError == nil + 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) + + err = path.EndpointB.ChanUpgradeTry() + suite.Require().NoError(err) + + // Pending implementation of ack on msg_server to move channel state for A. + // Currently fails on validation. + err = path.EndpointA.ChanUpgradeAck() + suite.Require().NoError(err) + + tc.malleate() + + // ensure clients are up to date to receive valid proofs + suite.Require().NoError(path.EndpointA.UpdateClient()) + proofCounterpartyChannel, _, proofHeight := path.EndpointB.QueryChannelUpgradeProof() + + err = suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.ChanUpgradeOpen( + suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, + path.EndpointB.GetChannel().State, proofCounterpartyChannel, proofHeight, + ) + if expPass { + suite.Require().NoError(err) + } else { + suite.Require().ErrorIs(err, tc.expError) + } + }) + } +} + func (suite *KeeperTestSuite) TestChanUpgradeTimeout() { var ( path *ibctesting.Path diff --git a/modules/core/04-channel/types/errors.go b/modules/core/04-channel/types/errors.go index 50f3e9f8a0b..e1efa4e369e 100644 --- a/modules/core/04-channel/types/errors.go +++ b/modules/core/04-channel/types/errors.go @@ -50,4 +50,5 @@ var ( ErrUpgradeRestoreFailed = errorsmod.Register(SubModuleName, 34, "restore failed") ErrUpgradeTimeout = errorsmod.Register(SubModuleName, 35, "upgrade timed-out") ErrInvalidUpgradeTimeout = errorsmod.Register(SubModuleName, 36, "upgrade timeout is invalid") + ErrPendingInflightPackets = errorsmod.Register(SubModuleName, 37, "pending inflight packets exist") ) From 9ffd9ee30c289f0967f2e1be59108d91f2de8916 Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Thu, 22 Jun 2023 07:22:50 +0300 Subject: [PATCH 2/8] Tests tests tests. --- modules/core/04-channel/keeper/upgrade.go | 2 +- .../core/04-channel/keeper/upgrade_test.go | 108 ++++++++++++++---- 2 files changed, 86 insertions(+), 24 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 051da97ea01..10e89c69aff 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -643,7 +643,7 @@ func (k Keeper) ChanUpgradeOpen( } counterpartyChannel = types.Channel{ State: types.OPEN, - Ordering: channel.Ordering, + Ordering: upgrade.Fields.Ordering, ConnectionHops: upgrade.Fields.ConnectionHops, Counterparty: types.NewCounterparty(portID, channelID), Version: upgrade.Fields.GetVersion(), diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index fb5ce7f7109..052ad8493ce 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -496,31 +496,97 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpen() { }{ { "success, counterparty in TRYUPGRADE", - func() {}, + func() { + err := path.EndpointA.ChanUpgradeInit() + suite.Require().NoError(err) + + err = path.EndpointB.ChanUpgradeTry() + suite.Require().NoError(err) + + err = path.EndpointA.ChanUpgradeAck() + suite.Require().NoError(err) + + channelB := path.EndpointB.GetChannel() + channelB.FlushStatus = types.FLUSHCOMPLETE + path.EndpointB.SetChannel(channelB) + + channelA := path.EndpointA.GetChannel() + channelA.FlushStatus = types.FLUSHCOMPLETE + path.EndpointA.SetChannel(channelA) + + suite.coordinator.CommitBlock(suite.chainA, suite.chainB) + suite.Require().NoError(path.EndpointA.UpdateClient()) + }, nil, }, - // TODO: Rest of combinations for counterparty state. + { + "success, counterparty in ACKUPGRADE", + func() { + err := path.EndpointB.ChanUpgradeInit() + suite.Require().NoError(err) + + err = path.EndpointA.ChanUpgradeTry() + suite.Require().NoError(err) + + err = path.EndpointB.ChanUpgradeAck() + suite.Require().NoError(err) + + channelB := path.EndpointB.GetChannel() + channelB.FlushStatus = types.FLUSHCOMPLETE + path.EndpointB.SetChannel(channelB) + + channelA := path.EndpointA.GetChannel() + channelA.FlushStatus = types.FLUSHCOMPLETE + path.EndpointA.SetChannel(channelA) + + suite.coordinator.CommitBlock(suite.chainA, suite.chainB) + suite.Require().NoError(path.EndpointA.UpdateClient()) + }, + nil, + }, + /* + { + // TODO: Pending implementation of open handling on msg_server + "success, counterparty in OPEN", + func() {}, + nil, + }, + */ { "channel not found", func() { - path.EndpointA.ChannelID = ibctesting.InvalidID path.EndpointA.ChannelConfig.PortID = ibctesting.InvalidID }, types.ErrChannelNotFound, }, { - "in-flight packets still exist", + "channel state is not in TRYUPGRADE or ACKUPGRADE", func() { - // TODO: + channel := path.EndpointA.GetChannel() + channel.State = types.OPEN + path.EndpointA.SetChannel(channel) + }, + types.ErrInvalidChannelState, + }, + { + "channel has in-flight packets", + func() { + portID := path.EndpointA.ChannelConfig.PortID + channelID := path.EndpointA.ChannelID + // Set a dummy packet commitment to simulate in-flight packets + suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.SetPacketCommitment(suite.chainA.GetContext(), portID, channelID, 1, []byte("hash")) }, types.ErrPendingInflightPackets, }, { "flush status is not FLUSHCOMPLETE", func() { - channel := path.EndpointB.GetChannel() + channel := path.EndpointA.GetChannel() + // Set in acceptable state + channel.State = types.TRYUPGRADE + channel.FlushStatus = types.FLUSHING - path.EndpointB.SetChannel(channel) + path.EndpointA.SetChannel(channel) }, types.ErrInvalidFlushStatus, }, @@ -528,6 +594,10 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpen() { "connection not found", func() { channel := path.EndpointA.GetChannel() + // Set in acceptable state + channel.State = types.TRYUPGRADE + channel.FlushStatus = types.FLUSHCOMPLETE + channel.ConnectionHops = []string{"connection-100"} path.EndpointA.SetChannel(channel) }, @@ -536,6 +606,12 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpen() { { "invalid connection state", func() { + channel := path.EndpointA.GetChannel() + // Set in acceptable state + channel.State = types.TRYUPGRADE + channel.FlushStatus = types.FLUSHCOMPLETE + path.EndpointA.SetChannel(channel) + connectionEnd := path.EndpointA.GetConnection() connectionEnd.State = connectiontypes.UNINITIALIZED path.EndpointA.SetConnection(connectionEnd) @@ -556,24 +632,10 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpen() { path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion - err := path.EndpointA.ChanUpgradeInit() - suite.Require().NoError(err) - - err = path.EndpointB.ChanUpgradeTry() - suite.Require().NoError(err) - - // Pending implementation of ack on msg_server to move channel state for A. - // Currently fails on validation. - err = path.EndpointA.ChanUpgradeAck() - suite.Require().NoError(err) - tc.malleate() - // ensure clients are up to date to receive valid proofs - suite.Require().NoError(path.EndpointA.UpdateClient()) - proofCounterpartyChannel, _, proofHeight := path.EndpointB.QueryChannelUpgradeProof() - - err = suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.ChanUpgradeOpen( + proofCounterpartyChannel, _, proofHeight := path.EndpointA.QueryChannelUpgradeProof() + err := suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.ChanUpgradeOpen( suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.GetChannel().State, proofCounterpartyChannel, proofHeight, ) From dcffa8074e42ef1b1aa022a04f3286f835169a51 Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Sat, 24 Jun 2023 04:25:54 +0300 Subject: [PATCH 3/8] Update upgrade open handler based on feedback. --- modules/core/04-channel/keeper/upgrade.go | 247 +++++++++++----------- 1 file changed, 127 insertions(+), 120 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 10e89c69aff..80f03089e09 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -302,6 +302,133 @@ func (k Keeper) WriteUpgradeAckChannel( emitChannelUpgradeAckEvent(ctx, portID, channelID, channel, upgrade) } +// ChanUpgradeOpen is called by a module to complete the channel upgrade handshake and move the channel back to an OPEN state. +// This method should only be called after both channels have flushed any in-flight packets. +// This method should only be called directly by the core IBC message server. +func (k Keeper) ChanUpgradeOpen( + ctx sdk.Context, + portID, + channelID string, + counterpartyChannelState types.State, + proofChannel []byte, + proofHeight clienttypes.Height, +) error { + if k.hasInflightPackets(ctx, portID, channelID) { + return errorsmod.Wrapf(types.ErrPendingInflightPackets, "port ID (%s) channel ID (%s)", portID, channelID) + } + + channel, found := k.GetChannel(ctx, portID, channelID) + if !found { + return errorsmod.Wrapf(types.ErrChannelNotFound, "port ID (%s) channel ID (%s)", portID, channelID) + } + + if !collections.Contains(channel.State, []types.State{types.TRYUPGRADE, types.ACKUPGRADE}) { + return errorsmod.Wrapf(types.ErrInvalidChannelState, "expected one of [%s, %s], got %s", types.TRYUPGRADE, types.ACKUPGRADE, channel.State) + } + + if channel.FlushStatus != types.FLUSHCOMPLETE { + return errorsmod.Wrapf(types.ErrInvalidFlushStatus, "expected %s, got %s", types.FLUSHCOMPLETE, channel.FlushStatus) + } + + 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 connection.GetState() != int32(connectiontypes.OPEN) { + return errorsmod.Wrapf(connectiontypes.ErrInvalidConnectionState, "connection state is not OPEN (got %s)", connectiontypes.State(connection.GetState()).String()) + } + + var counterpartyChannel types.Channel + counterpartyHops := []string{connection.GetCounterparty().GetConnectionID()} + switch counterpartyChannelState { + case types.OPEN: + upgrade, found := k.GetUpgrade(ctx, portID, channelID) + if !found { + return errorsmod.Wrapf(types.ErrUpgradeNotFound, "failed to retrieve channel upgrade: port ID (%s) channel ID (%s)", portID, channelID) + } + + counterpartyChannel = types.Channel{ + State: types.OPEN, + Ordering: upgrade.Fields.Ordering, + ConnectionHops: upgrade.Fields.ConnectionHops, + Counterparty: types.NewCounterparty(portID, channelID), + Version: upgrade.Fields.Version, + UpgradeSequence: channel.UpgradeSequence, + FlushStatus: types.NOTINFLUSH, + } + + case types.TRYUPGRADE: + // If the counterparty is in TRYUPGRADE, then we must have gone through the ACKUPGRADE step. + if channel.State != types.ACKUPGRADE { + return errorsmod.Wrapf(types.ErrInvalidChannelState, "expected %s, got %s", types.ACKUPGRADE, channel.State) + } + + counterpartyChannel = types.Channel{ + State: types.TRYUPGRADE, + Ordering: channel.Ordering, + ConnectionHops: counterpartyHops, + Counterparty: types.NewCounterparty(portID, channelID), + Version: channel.Version, + UpgradeSequence: channel.UpgradeSequence, + FlushStatus: types.FLUSHCOMPLETE, + } + + case types.ACKUPGRADE: + counterpartyChannel = types.Channel{ + State: types.ACKUPGRADE, + Ordering: channel.Ordering, + ConnectionHops: counterpartyHops, + Counterparty: types.NewCounterparty(portID, channelID), + Version: channel.Version, + UpgradeSequence: channel.UpgradeSequence, + FlushStatus: types.FLUSHCOMPLETE, + } + + default: + panic(fmt.Sprintf("counterparty channel state should be in one of [%s, %s, %s]; got %s", types.TRYUPGRADE, types.ACKUPGRADE, types.OPEN, counterpartyChannelState)) + } + + err = k.connectionKeeper.VerifyChannelState(ctx, connection, proofHeight, proofChannel, portID, channelID, counterpartyChannel) + if err != nil { + return errorsmod.Wrapf(err, "failed to verify counterparty channel, expected counterparty channel state: %s", counterpartyChannel.String()) + } + + return nil +} + +// WriteUpgradeOpenChannel writes the agreed upon upgrade fields to the channel, sets the channel flush status to NOTINFLUSH and sets the channel state back to OPEN. This can be called in one of two cases: +// - In the UpgradeAck step of the handshake if both sides have already flushed all in-flight packets. +// - In the UpgradeOpen step of the handshake. +func (k Keeper) writeUpgradeOpenChannel(ctx sdk.Context, portID, channelID string) { + channel, found := k.GetChannel(ctx, portID, channelID) + if !found { + panic(fmt.Sprintf("could not find existing channel when updating channel state, channelID: %s, portID: %s", channelID, portID)) + } + + upgrade, found := k.GetUpgrade(ctx, portID, channelID) + if !found { + panic(fmt.Sprintf("could not find upgrade when updating channel state, channelID: %s, portID: %s", channelID, portID)) + } + + // Switch channel fields to upgrade fields and set channel state to OPEN + previousState := channel.State + channel.Ordering = upgrade.Fields.Ordering + channel.Version = upgrade.Fields.Version + channel.ConnectionHops = upgrade.Fields.ConnectionHops + channel.State = types.OPEN + channel.FlushStatus = types.NOTINFLUSH + + k.SetChannel(ctx, portID, channelID, channel) + + // Delete auxiliary state. + k.deleteUpgrade(ctx, portID, channelID) + k.deleteCounterpartyLastPacketSequence(ctx, portID, channelID) + + k.Logger(ctx).Info("channel state updated", "port-id", portID, "channel-id", channelID, "previous-state", previousState.String(), "new-state", types.OPEN.String()) + emitChannelUpgradeOpenEvent(ctx, portID, channelID, channel) +} + // 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) @@ -597,126 +724,6 @@ func (k Keeper) startFlushUpgradeHandshake( return nil } -// ChanUpgradeOpen is called by a module to complete the channel upgrade handshake and move the channel back to an OPEN state. -// This method should only be called after both channels have flushed any in-flight packets. -func (k Keeper) ChanUpgradeOpen( - ctx sdk.Context, - portID, - channelID string, - counterpartyChannelState types.State, - proofChannel []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) - } - - if k.hasInflightPackets(ctx, portID, channelID) { - return errorsmod.Wrapf(types.ErrPendingInflightPackets, "port ID (%s) channel ID (%s)", portID, channelID) - } - - if !collections.Contains(channel.State, []types.State{types.TRYUPGRADE, types.ACKUPGRADE}) { - return errorsmod.Wrapf(types.ErrInvalidChannelState, "expected one of [%s, %s], got %s", types.TRYUPGRADE, types.ACKUPGRADE, channel.State) - } - - if channel.FlushStatus != types.FLUSHCOMPLETE { - return errorsmod.Wrapf(types.ErrInvalidFlushStatus, "expected %s, got %s", types.FLUSHCOMPLETE, channel.FlushStatus) - } - - 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 connection.GetState() != int32(connectiontypes.OPEN) { - return errorsmod.Wrapf(connectiontypes.ErrInvalidConnectionState, "connection state is not OPEN (got %s)", connectiontypes.State(connection.GetState()).String()) - } - - var counterpartyChannel types.Channel - counterpartyHops := []string{connection.GetCounterparty().GetConnectionID()} - switch counterpartyChannelState { - case types.OPEN: - upgrade, found := k.GetUpgrade(ctx, portID, channelID) - if !found { - return errorsmod.Wrapf(types.ErrUpgradeNotFound, "failed to retrieve channel upgrade: port ID (%s) channel ID (%s)", portID, channelID) - } - counterpartyChannel = types.Channel{ - State: types.OPEN, - Ordering: upgrade.Fields.Ordering, - ConnectionHops: upgrade.Fields.ConnectionHops, - Counterparty: types.NewCounterparty(portID, channelID), - Version: upgrade.Fields.GetVersion(), - UpgradeSequence: channel.UpgradeSequence, - FlushStatus: types.NOTINFLUSH, - } - - case types.TRYUPGRADE: - // If the counterparty is in TRYUPGRADE, then we must have gone through the ACKUPGRADE step. - if channel.State != types.ACKUPGRADE { - return errorsmod.Wrapf(types.ErrInvalidChannelState, "expected %s, got %s", types.ACKUPGRADE, channel.State) - } - counterpartyChannel = types.Channel{ - State: types.TRYUPGRADE, - Ordering: channel.Ordering, - ConnectionHops: counterpartyHops, - Counterparty: types.NewCounterparty(portID, channelID), - Version: channel.Version, - UpgradeSequence: channel.UpgradeSequence, - FlushStatus: types.FLUSHCOMPLETE, - } - - case types.ACKUPGRADE: - counterpartyChannel = types.Channel{ - State: types.ACKUPGRADE, - Ordering: channel.Ordering, - ConnectionHops: counterpartyHops, - Counterparty: types.NewCounterparty(portID, channelID), - Version: channel.Version, - UpgradeSequence: channel.UpgradeSequence, - FlushStatus: types.FLUSHCOMPLETE, - } - default: - panic(fmt.Sprintf("counterparty channel state should be in one of [%s, %s, %s]; got %s", types.TRYUPGRADE, types.ACKUPGRADE, types.OPEN, counterpartyChannelState)) - } - - return k.connectionKeeper.VerifyChannelState( - ctx, connection, proofHeight, proofChannel, portID, channelID, counterpartyChannel, - ) -} - -// WriteUpgradeOpenChannel writes the agreed upon upgrade fields to the channel, sets the channel flush status to NOTINFLUSH and sets the channel state back to OPEN. This can be called in one of two cases: -// - In the UpgradeAck step of the handshake if both sides have already flushed all in-flight packets. -// - In the UpgradeOpen step of the handshake. -func (k Keeper) writeUpgradeOpenChannel(ctx sdk.Context, portID, channelID string) { - channel, found := k.GetChannel(ctx, portID, channelID) - if !found { - panic(fmt.Sprintf("could not find existing channel when updating channel state, channelID: %s, portID: %s", channelID, portID)) - } - - upgrade, found := k.GetUpgrade(ctx, portID, channelID) - if !found { - panic(fmt.Sprintf("could not find upgrade when updating channel state, channelID: %s, portID: %s", channelID, portID)) - } - - // Switch channel fields to upgrade fields and set channel state to OPEN - previousState := channel.State - channel.Ordering = upgrade.Fields.Ordering - channel.Version = upgrade.Fields.Version - channel.ConnectionHops = upgrade.Fields.ConnectionHops - channel.State = types.OPEN - channel.FlushStatus = types.NOTINFLUSH - - k.SetChannel(ctx, portID, channelID, channel) - - // Delete auxiliary state. - k.deleteUpgrade(ctx, portID, channelID) - k.deleteCounterpartyLastPacketSequence(ctx, portID, channelID) - - k.Logger(ctx).Info("channel state updated", "port-id", portID, "channel-id", channelID, "previous-state", previousState.String(), "new-state", types.OPEN.String()) - emitChannelUpgradeOpenEvent(ctx, portID, channelID, channel) -} - // 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 From 85ffd3b4136635189b14cd79f91f0092a150d1bd Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Wed, 28 Jun 2023 10:55:11 +0300 Subject: [PATCH 4/8] Reformat testing approach. --- .../core/04-channel/keeper/upgrade_test.go | 99 +++++-------------- 1 file changed, 27 insertions(+), 72 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index 052ad8493ce..8beac7fc3a0 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -495,63 +495,10 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpen() { expError error }{ { - "success, counterparty in TRYUPGRADE", - func() { - err := path.EndpointA.ChanUpgradeInit() - suite.Require().NoError(err) - - err = path.EndpointB.ChanUpgradeTry() - suite.Require().NoError(err) - - err = path.EndpointA.ChanUpgradeAck() - suite.Require().NoError(err) - - channelB := path.EndpointB.GetChannel() - channelB.FlushStatus = types.FLUSHCOMPLETE - path.EndpointB.SetChannel(channelB) - - channelA := path.EndpointA.GetChannel() - channelA.FlushStatus = types.FLUSHCOMPLETE - path.EndpointA.SetChannel(channelA) - - suite.coordinator.CommitBlock(suite.chainA, suite.chainB) - suite.Require().NoError(path.EndpointA.UpdateClient()) - }, - nil, - }, - { - "success, counterparty in ACKUPGRADE", - func() { - err := path.EndpointB.ChanUpgradeInit() - suite.Require().NoError(err) - - err = path.EndpointA.ChanUpgradeTry() - suite.Require().NoError(err) - - err = path.EndpointB.ChanUpgradeAck() - suite.Require().NoError(err) - - channelB := path.EndpointB.GetChannel() - channelB.FlushStatus = types.FLUSHCOMPLETE - path.EndpointB.SetChannel(channelB) - - channelA := path.EndpointA.GetChannel() - channelA.FlushStatus = types.FLUSHCOMPLETE - path.EndpointA.SetChannel(channelA) - - suite.coordinator.CommitBlock(suite.chainA, suite.chainB) - suite.Require().NoError(path.EndpointA.UpdateClient()) - }, + "success", + func() {}, nil, }, - /* - { - // TODO: Pending implementation of open handling on msg_server - "success, counterparty in OPEN", - func() {}, - nil, - }, - */ { "channel not found", func() { @@ -559,15 +506,15 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpen() { }, types.ErrChannelNotFound, }, + { "channel state is not in TRYUPGRADE or ACKUPGRADE", func() { - channel := path.EndpointA.GetChannel() - channel.State = types.OPEN - path.EndpointA.SetChannel(channel) + path.EndpointA.SetChannelState(types.OPEN) }, types.ErrInvalidChannelState, }, + { "channel has in-flight packets", func() { @@ -582,9 +529,6 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpen() { "flush status is not FLUSHCOMPLETE", func() { channel := path.EndpointA.GetChannel() - // Set in acceptable state - channel.State = types.TRYUPGRADE - channel.FlushStatus = types.FLUSHING path.EndpointA.SetChannel(channel) }, @@ -594,10 +538,6 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpen() { "connection not found", func() { channel := path.EndpointA.GetChannel() - // Set in acceptable state - channel.State = types.TRYUPGRADE - channel.FlushStatus = types.FLUSHCOMPLETE - channel.ConnectionHops = []string{"connection-100"} path.EndpointA.SetChannel(channel) }, @@ -606,12 +546,6 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpen() { { "invalid connection state", func() { - channel := path.EndpointA.GetChannel() - // Set in acceptable state - channel.State = types.TRYUPGRADE - channel.FlushStatus = types.FLUSHCOMPLETE - path.EndpointA.SetChannel(channel) - connectionEnd := path.EndpointA.GetConnection() connectionEnd.State = connectiontypes.UNINITIALIZED path.EndpointA.SetConnection(connectionEnd) @@ -632,10 +566,31 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpen() { path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion + err := path.EndpointB.ChanUpgradeInit() + suite.Require().NoError(err) + + err = path.EndpointA.ChanUpgradeTry() + suite.Require().NoError(err) + + err = path.EndpointB.ChanUpgradeAck() + suite.Require().NoError(err) + + // TODO: Remove setting of FLUSHCOMPLETE once #3928 is completed + channelB := path.EndpointB.GetChannel() + channelB.FlushStatus = types.FLUSHCOMPLETE + path.EndpointB.SetChannel(channelB) + + channelA := path.EndpointA.GetChannel() + channelA.FlushStatus = types.FLUSHCOMPLETE + path.EndpointA.SetChannel(channelA) + + suite.coordinator.CommitBlock(suite.chainA, suite.chainB) + suite.Require().NoError(path.EndpointA.UpdateClient()) + tc.malleate() proofCounterpartyChannel, _, proofHeight := path.EndpointA.QueryChannelUpgradeProof() - err := suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.ChanUpgradeOpen( + err = suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.ChanUpgradeOpen( suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.GetChannel().State, proofCounterpartyChannel, proofHeight, ) From 17191a2c480afffc546d708007776fd21a86a0bb Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Wed, 28 Jun 2023 10:59:24 +0300 Subject: [PATCH 5/8] Move counterpartyhops assignment inline. --- modules/core/04-channel/keeper/upgrade.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 80f03089e09..254d903cf2c 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -340,7 +340,6 @@ func (k Keeper) ChanUpgradeOpen( } var counterpartyChannel types.Channel - counterpartyHops := []string{connection.GetCounterparty().GetConnectionID()} switch counterpartyChannelState { case types.OPEN: upgrade, found := k.GetUpgrade(ctx, portID, channelID) @@ -367,7 +366,7 @@ func (k Keeper) ChanUpgradeOpen( counterpartyChannel = types.Channel{ State: types.TRYUPGRADE, Ordering: channel.Ordering, - ConnectionHops: counterpartyHops, + ConnectionHops: []string{connection.GetCounterparty().GetConnectionID()}, Counterparty: types.NewCounterparty(portID, channelID), Version: channel.Version, UpgradeSequence: channel.UpgradeSequence, @@ -378,7 +377,7 @@ func (k Keeper) ChanUpgradeOpen( counterpartyChannel = types.Channel{ State: types.ACKUPGRADE, Ordering: channel.Ordering, - ConnectionHops: counterpartyHops, + ConnectionHops: []string{connection.GetCounterparty().GetConnectionID()}, Counterparty: types.NewCounterparty(portID, channelID), Version: channel.Version, UpgradeSequence: channel.UpgradeSequence, From 112423e8ab5b71ccd7558632b64cb54e9a7f2fe2 Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Wed, 28 Jun 2023 11:30:15 +0300 Subject: [PATCH 6/8] Check err of SetChannelState. --- 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 8beac7fc3a0..332bbc46167 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -510,7 +510,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpen() { { "channel state is not in TRYUPGRADE or ACKUPGRADE", func() { - path.EndpointA.SetChannelState(types.OPEN) + suite.Require().NoError(path.EndpointA.SetChannelState(types.OPEN)) }, types.ErrInvalidChannelState, }, From 77d91c230a10b24595a37c8a94620de9ff65bdcb Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Wed, 28 Jun 2023 12:05:45 +0300 Subject: [PATCH 7/8] Address feedback. --- modules/core/04-channel/keeper/upgrade_test.go | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index 332bbc46167..bedbdde145b 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -526,7 +526,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpen() { types.ErrPendingInflightPackets, }, { - "flush status is not FLUSHCOMPLETE", + "flush status is FLUSHING", func() { channel := path.EndpointA.GetChannel() channel.FlushStatus = types.FLUSHING @@ -534,6 +534,15 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpen() { }, types.ErrInvalidFlushStatus, }, + { + "flush status is NOTINFLUSH", + func() { + channel := path.EndpointA.GetChannel() + channel.FlushStatus = types.NOTINFLUSH + path.EndpointA.SetChannel(channel) + }, + types.ErrInvalidFlushStatus, + }, { "connection not found", func() { @@ -557,7 +566,6 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpen() { for _, tc := range testCases { tc := tc suite.Run(tc.name, func() { - expPass := tc.expError == nil suite.SetupTest() path = ibctesting.NewPath(suite.chainA, suite.chainB) @@ -594,7 +602,8 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpen() { suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.GetChannel().State, proofCounterpartyChannel, proofHeight, ) - if expPass { + + if tc.expError == nil { suite.Require().NoError(err) } else { suite.Require().ErrorIs(err, tc.expError) From 151f71e313857e7a9bb115762417e077961c5d92 Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Wed, 28 Jun 2023 16:29:59 +0300 Subject: [PATCH 8/8] Remove uneeded modification of version. --- modules/core/04-channel/keeper/upgrade_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index bedbdde145b..edb04944c0e 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -571,7 +571,6 @@ func (suite *KeeperTestSuite) TestChanUpgradeOpen() { 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.EndpointB.ChanUpgradeInit()