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

Amend timeout to handle in-flight packets. #3923

Merged
merged 8 commits into from
Jul 6, 2023
Merged

Amend timeout to handle in-flight packets. #3923

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
65d290f
Amend timeout to handle in-flight packets.
DimitrisJim Jun 22, 2023
b601808
Update timeout handler per spec.
DimitrisJim Jun 28, 2023
e15d3af
Merge branch '04-channel-upgrades' into jim/3879-packet-flushing-timeout
DimitrisJim Jun 29, 2023
9adb05a
Merge branch '04-channel-upgrades' into jim/3879-packet-flushing-timeout
DimitrisJim Jul 5, 2023
e74bced
Update tests to test for toggling of flush status.
DimitrisJim Jul 5, 2023
2e73fd2
Merge branch '04-channel-upgrades' into jim/3879-packet-flushing-timeout
crodriguezvega Jul 6, 2023
d3a60ee
Merge branch '04-channel-upgrades' into jim/3879-packet-flushing-timeout
DimitrisJim Jul 6, 2023
91dc1ad
Fix small typos in docstring.
DimitrisJim Jul 6, 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
5 changes: 5 additions & 0 deletions modules/core/04-channel/keeper/timeout.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,11 @@ func (k Keeper) TimeoutExecuted(

k.deletePacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence())

if channel.FlushStatus == types.FLUSHING && !k.hasInflightPackets(ctx, packet.GetSourcePort(), packet.GetSourceChannel()) {
Copy link
Contributor Author

@DimitrisJim DimitrisJim Jun 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

P.s, these could also be:

switch channel.Ordering {
case types.ORDERED:
	channel.State = types.CLOSED
	channel.FlushStatus = types.NOTINFLUSH
case types.UNORDERED:
	if channel.FlushStatus == types.FLUSHING && !k.hasInflightPackets(ctx, packet.GetSourcePort(), packet.GetSourceChannel()) {
		channel.FlushStatus = types.FLUSHCOMPLETE
	}
}
k.SetChannel(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), channel)

and, considering ORDERED_ALLOW_TIMEOUT, this could be amended in the future to possible be case types.UNORDERED, types.ORDERED_ALLOW_TIMEOUT since it looks like it would require similar handling.

Happy to leave as is but dropping this sionce I noticed it.

Copy link
Contributor

@charleenfei charleenfei Jun 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

i think the switch statement is nice for readability!

but i guess we probably need to check if channel.FlushStatus == types.FLUSHING && !k.hasInflightPackets(ctx, packet.GetSourcePort(), packet.GetSourceChannel()) in both cases rather than just in the UNORDERED case, and then basically if it's ORDERED set to CLOSED/NOTINFLUSH, and do the ChannelClosedEvent in that case, and if it's UNORDERED set to FLUSHCOMPLETE. but i'm not super opinionated on which style we go with.

channel.FlushStatus = types.FLUSHCOMPLETE
k.SetChannel(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), channel)
}

if channel.Ordering == types.ORDERED {
channel.Close()
k.SetChannel(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), channel)
Expand Down
136 changes: 103 additions & 33 deletions modules/core/04-channel/keeper/timeout_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
host "github.com/cosmos/ibc-go/v7/modules/core/24-host"
"github.com/cosmos/ibc-go/v7/modules/core/exported"
ibctesting "github.com/cosmos/ibc-go/v7/testing"
"github.com/cosmos/ibc-go/v7/testing/mock"
)

// TestTimeoutPacket test the TimeoutPacket call on chainA by ensuring the timeout has passed
Expand Down Expand Up @@ -247,46 +248,120 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() {
}

// TestTimeoutExectued verifies that packet commitments are deleted on chainA after the
// channel capabilities are verified.
// channel capabilities are verified. In addition, the test verified that the channel state
// after a timeout is updated accordingly.
func (suite *KeeperTestSuite) TestTimeoutExecuted() {
var (
path *ibctesting.Path
packet types.Packet
chanCap *capabilitytypes.Capability
)

testCases := []testCase{
{"success ORDERED", func() {
path.SetChannelOrdered()
suite.coordinator.Setup(path)
testCases := []struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

completely revamped this test to allow checking for how the state of affairs is post timeoutexecuted (also, the error returned). I think this is a way better approach than what previously existed though its unfortunate it added an additional diff to check.

msg string
malleate func()
expResult func(packetCommitment []byte, err error)
}{
{
"success ORDERED",
func() {
path.SetChannelOrdered()
suite.coordinator.Setup(path)

timeoutHeight := clienttypes.GetSelfHeight(suite.chainB.GetContext())
Copy link
Contributor

Choose a reason for hiding this comment

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

i think there's also a default timeout height we could use suite.chainB.GetTimeoutHeight()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yeah, works out with the suggestion. I'm seeing the rest of the tests here use the old version so might be better to open an issue and change this altogether in a later point? wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely works for me!

timeoutTimestamp := uint64(suite.chainB.GetContext().BlockTime().UnixNano())

sequence, err := path.EndpointA.SendPacket(timeoutHeight, timeoutTimestamp, ibctesting.MockPacketData)
suite.Require().NoError(err)

timeoutHeight := clienttypes.GetSelfHeight(suite.chainB.GetContext())
timeoutTimestamp := uint64(suite.chainB.GetContext().BlockTime().UnixNano())
packet = types.NewPacket(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, timeoutTimestamp)
chanCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
},
func(packetCommitment []byte, err error) {
suite.Require().NoError(err)
suite.Require().Nil(packetCommitment)

// Check channel has been closed and flush status is set to NOTINFLUSH
channel := path.EndpointA.GetChannel()
suite.Require().Equal(channel.State, types.CLOSED)
suite.Require().Equal(channel.FlushStatus, types.NOTINFLUSH)
},
},
{
"success UNORDERED channel in FLUSHING state",
func() {
suite.coordinator.Setup(path)

timeoutHeight := clienttypes.GetSelfHeight(suite.chainB.GetContext())
timeoutTimestamp := uint64(suite.chainB.GetContext().BlockTime().UnixNano())

sequence, err := path.EndpointA.SendPacket(timeoutHeight, timeoutTimestamp, ibctesting.MockPacketData)
suite.Require().NoError(err)

sequence, err := path.EndpointA.SendPacket(timeoutHeight, timeoutTimestamp, ibctesting.MockPacketData)
suite.Require().NoError(err)
packet = types.NewPacket(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, timeoutTimestamp)
chanCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)

packet = types.NewPacket(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, timeoutTimestamp)
chanCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
}, true},
{"channel not found", func() {
// use wrong channel naming
suite.coordinator.Setup(path)
packet = types.NewPacket(ibctesting.MockPacketData, 1, ibctesting.InvalidID, ibctesting.InvalidID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, defaultTimeoutHeight, disabledTimeoutTimestamp)
}, false},
{"incorrect capability ORDERED", func() {
path.SetChannelOrdered()
suite.coordinator.Setup(path)
// Move channel to FLUSHING state
path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion
path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion

timeoutHeight := clienttypes.GetSelfHeight(suite.chainB.GetContext())
timeoutTimestamp := uint64(suite.chainB.GetContext().BlockTime().UnixNano())
err = path.EndpointA.ChanUpgradeInit()
suite.Require().NoError(err)

sequence, err := path.EndpointA.SendPacket(timeoutHeight, timeoutTimestamp, ibctesting.MockPacketData)
suite.Require().NoError(err)
err = path.EndpointB.ChanUpgradeTry()
suite.Require().NoError(err)

packet = types.NewPacket(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, timeoutTimestamp)
chanCap = capabilitytypes.NewCapability(100)
}, false},
err = path.EndpointA.ChanUpgradeAck()
suite.Require().NoError(err)
},
func(packetCommitment []byte, err error) {
suite.Require().NoError(err)
suite.Require().Nil(packetCommitment)

// Check flush status has been set to FLUSHCOMPLETE
channel := path.EndpointA.GetChannel()
suite.Require().Equal(channel.State, types.ACKUPGRADE)
suite.Require().Equal(channel.FlushStatus, types.FLUSHCOMPLETE)
},
},
{
"channel not found",
func() {
// use wrong channel naming
suite.coordinator.Setup(path)
packet = types.NewPacket(ibctesting.MockPacketData, 1, ibctesting.InvalidID, ibctesting.InvalidID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, defaultTimeoutHeight, disabledTimeoutTimestamp)
},
func(packetCommitment []byte, err error) {
suite.Require().Error(err)
suite.Require().ErrorIs(err, types.ErrChannelNotFound)

// packet never sent.
suite.Require().Nil(packetCommitment)
},
},
{
"incorrect capability ORDERED",
func() {
path.SetChannelOrdered()
suite.coordinator.Setup(path)

timeoutHeight := clienttypes.GetSelfHeight(suite.chainB.GetContext())
timeoutTimestamp := uint64(suite.chainB.GetContext().BlockTime().UnixNano())

sequence, err := path.EndpointA.SendPacket(timeoutHeight, timeoutTimestamp, ibctesting.MockPacketData)
suite.Require().NoError(err)

packet = types.NewPacket(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, timeoutTimestamp)
chanCap = capabilitytypes.NewCapability(100)
},
func(packetCommitment []byte, err error) {
suite.Require().Error(err)
suite.Require().ErrorIs(err, types.ErrChannelCapabilityNotFound)

// packet sent, never deleted.
suite.Require().NotNil(packetCommitment)
},
},
}

for i, tc := range testCases {
Expand All @@ -300,12 +375,7 @@ func (suite *KeeperTestSuite) TestTimeoutExecuted() {
err := suite.chainA.App.GetIBCKeeper().ChannelKeeper.TimeoutExecuted(suite.chainA.GetContext(), chanCap, packet)
pc := suite.chainA.App.GetIBCKeeper().ChannelKeeper.GetPacketCommitment(suite.chainA.GetContext(), packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence())

if tc.expPass {
suite.NoError(err)
suite.Nil(pc)
} else {
suite.Error(err)
}
tc.expResult(pc, err)
})
}
}
Expand Down