Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move channel to OPEN if all in-flight packets have been flushed in UpgradeAck. #4075

Merged
merged 4 commits into from
Jul 19, 2023
Merged

Move channel to OPEN if all in-flight packets have been flushed in UpgradeAck. #4075

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3b41262
Move channel to OPEN if all in-flight packets have been flushed in Up…
DimitrisJim Jul 10, 2023
ec0cae8
Add comment noting that counterparty flush status has been verified b…
DimitrisJim Jul 18, 2023
58dff20
Merge branch '04-channel-upgrades' into jim/4030-chanugpradeack-calls…
DimitrisJim Jul 19, 2023
cd36f8e
Fix failing tests.
DimitrisJim Jul 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion modules/core/04-channel/keeper/packet_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -862,11 +862,16 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() {
packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, defaultTimeoutHeight, disabledTimeoutTimestamp)
channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)

// Send a packet on B to disallow channel automatically moving to OPEN on UpgradeAck
sequence, err := path.EndpointB.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData)
suite.Require().Equal(uint64(1), sequence)
suite.Require().NoError(err)

// Move channel to correct state.
path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = ibcmock.UpgradeVersion
path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = ibcmock.UpgradeVersion

err := path.EndpointA.ChanUpgradeInit()
err = path.EndpointA.ChanUpgradeInit()
suite.Require().NoError(err)

err = path.EndpointB.ChanUpgradeTry()
Expand Down
16 changes: 16 additions & 0 deletions modules/core/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -852,6 +852,22 @@ func (k Keeper) ChannelUpgradeAck(goCtx context.Context, msg *channeltypes.MsgCh

ctx.Logger().Info("channel upgrade ack succeeded", "port-id", msg.PortId, "channel-id", msg.ChannelId)

// Move channel to OPEN state if both chains have finished flushing any in-flight packets. Counterparty flush status
// has been verified in ChanUpgradeAck.
if msg.CounterpartyFlushStatus == channeltypes.FLUSHCOMPLETE {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used this instead of channel.FlushStatus == FLUSHCOMPLETE && counterpartyChannel.FlushStatus == FLUSHCOMPLETE so as to not consume gas unnecessarily when getting the channel.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guess the msg.CounterpartyFlushStatus has been proven with the CounterpartyChannelProof on the ChannelUpgradeAck handler so this seems fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah good point, might be worth adding a comment to explicitly state that this value has already been verified in k.ChannelKeeper.ChanUpgradeAck

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

happy to go ahead with this for now, we can revisit it when doing the team walkthrough/audit.

Something about looking up the channels and stuff at the core msg server layer always seems a bit off to me but I know there's so many options here. I guess we could also do something like:

if msg.CounterpartyFlushStatus == FLUSHCOMPLETE && !k.channelKeeper.HasInflightPackets() {
 // upgrade open
}

But anyways, we can talk about that later

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that should also work just fine no? Super cleaner. Will go with current approach for now but probably open a PR for ^ (need to tweak visibility of hasInflightPackets and just verify all tests pass correctly). Dope suggestion.

channel, found := k.ChannelKeeper.GetChannel(ctx, msg.PortId, msg.ChannelId)
if !found {
return nil, errorsmod.Wrapf(channeltypes.ErrChannelNotFound, "port ID (%s) channel ID (%s)", msg.PortId, msg.ChannelId)
}
if channel.FlushStatus == channeltypes.FLUSHCOMPLETE {
cbs.OnChanUpgradeOpen(ctx, msg.PortId, msg.ChannelId)

k.ChannelKeeper.WriteUpgradeOpenChannel(ctx, msg.PortId, msg.ChannelId)
Comment on lines +863 to +865
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its unnecessary to call k.ChannelKeeper.ChanUpgradeOpen(), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea, the way I'm reasoning about it is that the proof we'd end up doing would be equivalent to what we did for UPGRADEACK (with the difference being we'd expect counterparty flush status to be in FLUSH_COMPLETE which we just checked in the conditional). Well, that and that the spec says so.


ctx.Logger().Info("channel upgrade open succeeded", "port-id", msg.PortId, "channel-id", msg.ChannelId)
}
}

return &channeltypes.MsgChannelUpgradeAckResponse{Result: channeltypes.SUCCESS}, nil
}

Expand Down
23 changes: 21 additions & 2 deletions modules/core/keeper/msg_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -937,17 +937,36 @@ func (suite *KeeperTestSuite) TestChannelUpgradeAck() {
expResult func(res *channeltypes.MsgChannelUpgradeAckResponse, err error)
}{
{
"success",
"success, no pending in-flight packets",
func() {},
func(res *channeltypes.MsgChannelUpgradeAckResponse, err error) {
suite.Require().NoError(err)
suite.Require().NotNil(res)
suite.Require().Equal(channeltypes.SUCCESS, res.Result)

channel := path.EndpointA.GetChannel()
suite.Require().Equal(channeltypes.OPEN, channel.State)
suite.Require().Equal(uint64(1), channel.UpgradeSequence)
suite.Require().Equal(channeltypes.NOTINFLUSH, channel.FlushStatus)
},
},
{
"success, pending 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"))
},
func(res *channeltypes.MsgChannelUpgradeAckResponse, err error) {
suite.Require().NoError(err)
suite.Require().NotNil(res)
suite.Require().Equal(channeltypes.SUCCESS, res.Result)

channel := path.EndpointA.GetChannel()
suite.Require().Equal(channeltypes.ACKUPGRADE, channel.State)
suite.Require().Equal(uint64(1), channel.UpgradeSequence)
suite.Require().Equal(channeltypes.FLUSHCOMPLETE, channel.FlushStatus)
suite.Require().Equal(channeltypes.FLUSHING, channel.FlushStatus)
},
},
{
Expand Down
Loading