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

Account for moving channel state to flush complete in timeout processing #4425

Merged
merged 12 commits into from
Aug 24, 2023
Merged

Account for moving channel state to flush complete in timeout processing #4425

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
7f21276
chore: adding channel upgrade tests for TimeoutExecuted
chatton Aug 22, 2023
599aa9a
chore: reverted accidental name change
chatton Aug 22, 2023
9bd3d38
Merge branch '04-channel-upgrades' into cian/issue#4262-in-timeout-pr…
chatton Aug 24, 2023
ec86acd
Merge branch '04-channel-upgrades' into cian/issue#4262-in-timeout-pr…
chatton Aug 24, 2023
625db69
Merge branch '04-channel-upgrades' into cian/issue#4262-in-timeout-pr…
chatton Aug 24, 2023
15fb61b
Merge branch 'cian/issue#4262-in-timeout-processing-account-for-movin…
chatton Aug 24, 2023
bae3161
chore: refactor tests to not use EndPoints
chatton Aug 24, 2023
80b7006
Merge branch '04-channel-upgrades' into cian/issue#4262-in-timeout-pr…
chatton Aug 24, 2023
32f6a9b
chore: addresing PR feedback
chatton Aug 24, 2023
2276881
Merge branch '04-channel-upgrades' into cian/issue#4262-in-timeout-pr…
chatton Aug 24, 2023
5ee1271
chore: addressing PR feedback
chatton Aug 24, 2023
4929770
Merge branch '04-channel-upgrades' into cian/issue#4262-in-timeout-pr…
chatton Aug 24, 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
29 changes: 26 additions & 3 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 @@ -132,6 +132,9 @@ func (k Keeper) TimeoutPacket(

// TimeoutExecuted deletes the commitment send from this chain after it verifies timeout.
// If the timed-out packet came from an ORDERED channel then this channel will be closed.
// If the channel is in the FLUSHING state and there is a counterparty upgrade, then the
// upgrade will be aborted if the upgrade has timed out. Otherwise, if there are no more inflight packets,
// then the channel will be set to the FLUSHCOMPLETE state.
//
// CONTRACT: this function must be called in the IBC handler
func (k Keeper) TimeoutExecuted(
Expand All @@ -154,9 +157,29 @@ 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()) {
channel.FlushStatus = types.FLUSHCOMPLETE
k.SetChannel(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), channel)
// if an upgrade is in progress, handling packet flushing and update channel state appropriately
if channel.State == types.STATE_FLUSHING {
counterpartyUpgrade, found := k.GetCounterpartyUpgrade(ctx, packet.GetSourcePort(), packet.GetSourceChannel())
if !found {
return errorsmod.Wrapf(types.ErrUpgradeNotFound, "counterparty upgrade not found for channel: %s", packet.GetSourceChannel())
}

timeout := counterpartyUpgrade.Timeout
// if the timeout is valid then use it, otherwise it has not been set in the upgrade handshake yet.
if timeout.IsValid() {
if hasPassed, err := timeout.HasPassed(ctx); hasPassed {
// packet flushing timeout has expired, abort the upgrade and return nil,
// committing an error receipt to state, restoring the channel and successfully timing out the packet.
k.MustAbortUpgrade(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), err)
return nil
}

// set the channel state to flush complete if all packets have been flushed.
if !k.HasInflightPackets(ctx, packet.GetSourcePort(), packet.GetSourceChannel()) {
channel.State = types.STATE_FLUSHCOMPLETE
k.SetChannel(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), channel)
}
}
}

if channel.Ordering == types.ORDERED {
Expand Down
174 changes: 137 additions & 37 deletions modules/core/04-channel/keeper/timeout_test.go
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

Awesome, these tests already are setup with expResult!

Would also be nice to have a test where the channel remains in FLUSHING after a timeout but upgrade timeout is valid. Essentially having an additional in-flight packet so that the upgrade timeout logic is checked but the channel state transition doesn't occur because there is another commitment in state

Original file line number Diff line number Diff line change
Expand Up @@ -286,43 +286,6 @@
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)

// 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)

// // Move channel to FLUSHING state
// 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)

// 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() {
Expand Down Expand Up @@ -361,6 +324,143 @@
suite.Require().NotNil(packetCommitment)
},
},
{
"set to flush complete with no inflight packets",
func() {
suite.coordinator.Setup(path)

chanCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)

channel := path.EndpointA.GetChannel()
channel.State = types.STATE_FLUSHING
path.EndpointA.SetChannel(channel)
path.EndpointA.SetChannelCounterpartyUpgrade(types.Upgrade{
Timeout: types.DefaultTimeout,
})
},
func(packetCommitment []byte, err error) {
suite.Require().NoError(err)

channel := path.EndpointA.GetChannel()
suite.Require().Equal(types.STATE_FLUSHCOMPLETE, channel.State, "channel state should still be set to FLUSHCOMPLETE")
},
},
{
"conterparty upgrade timeout is invalid",
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)

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)

channel := path.EndpointA.GetChannel()
channel.State = types.STATE_FLUSHING
path.EndpointA.SetChannel(channel)
path.EndpointA.SetChannelCounterpartyUpgrade(types.Upgrade{})
},
func(packetCommitment []byte, err error) {
suite.Require().NoError(err)

channel := path.EndpointA.GetChannel()
suite.Require().Equal(types.STATE_FLUSHING, channel.State, "channel state should still be FLUSHING")
},
},
{
"conterparty upgrade timed out (abort)",
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)

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)

channel := path.EndpointA.GetChannel()
channel.State = types.STATE_FLUSHING
path.EndpointA.SetChannel(channel)
path.EndpointA.SetChannelUpgrade(types.Upgrade{
Fields: path.EndpointA.GetProposedUpgrade().Fields,
Timeout: types.NewTimeout(clienttypes.ZeroHeight(), 1),
})
path.EndpointA.SetChannelCounterpartyUpgrade(types.Upgrade{
Timeout: types.NewTimeout(clienttypes.ZeroHeight(), 1),
})
},
func(packetCommitment []byte, err error) {
suite.Require().NoError(err)

channel := path.EndpointA.GetChannel()
suite.Require().Equal(types.OPEN, channel.State, "channel state should still be OPEN")

upgrade, found := suite.chainA.App.GetIBCKeeper().ChannelKeeper.GetUpgrade(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
suite.Require().False(found, "upgrade should not be present")
suite.Require().Equal(types.Upgrade{}, upgrade, "upgrade should be zero value")

upgrade, found = suite.chainA.App.GetIBCKeeper().ChannelKeeper.GetCounterpartyUpgrade(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
suite.Require().False(found, "counterparty upgrade should not be present")
suite.Require().Equal(types.Upgrade{}, upgrade, "counterparty upgrade should be zero value")

errorReceipt, found := suite.chainA.App.GetIBCKeeper().ChannelKeeper.GetUpgradeErrorReceipt(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
suite.Require().True(found, "error receipt should be present")
suite.Require().Equal(channel.UpgradeSequence, errorReceipt.Sequence, "error receipt sequence should be equal to channel upgrade sequence")
},
},
{
"conterparty upgrade has not timed out with in-flight packets",
func() {
suite.coordinator.Setup(path)

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

// we are sending two packets here as one will be removed in TimeoutExecuted. This is to ensure that
// there is still an in-flight packet so that the channel remains in the flushing state.
sequence, err := path.EndpointA.SendPacket(timeoutHeight, timeoutTimestamp, ibctesting.MockPacketData)

Check failure on line 428 in modules/core/04-channel/keeper/timeout_test.go

View workflow job for this annotation

GitHub Actions / lint 

ineffectual assignment to sequence (ineffassign)

Check failure on line 428 in modules/core/04-channel/keeper/timeout_test.go

View workflow job for this annotation

GitHub Actions / lint 

ineffectual assignment to sequence (ineffassign)
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)

channel := path.EndpointA.GetChannel()
channel.State = types.STATE_FLUSHING
path.EndpointA.SetChannel(channel)
path.EndpointA.SetChannelUpgrade(types.Upgrade{
Fields: path.EndpointA.GetProposedUpgrade().Fields,
Timeout: types.DefaultTimeout,
})
path.EndpointA.SetChannelCounterpartyUpgrade(types.Upgrade{
Timeout: types.DefaultTimeout,
})
},
func(packetCommitment []byte, err error) {
suite.Require().NoError(err)

channel := path.EndpointA.GetChannel()
suite.Require().Equal(types.STATE_FLUSHING, channel.State, "channel state should still be FLUSHING")

_, found := suite.chainA.App.GetIBCKeeper().ChannelKeeper.GetUpgrade(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
suite.Require().True(found, "upgrade should not be deleted")

_, found = suite.chainA.App.GetIBCKeeper().ChannelKeeper.GetCounterpartyUpgrade(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
suite.Require().True(found, "counterparty upgrade should not be deleted")

_, found = suite.chainA.App.GetIBCKeeper().ChannelKeeper.GetUpgradeErrorReceipt(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
suite.Require().False(found, "error receipt should not be written")
},
},
}

for i, tc := range testCases {
Expand Down
Loading