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

Amend AcknowledgePacket to handle flushing #3922

Merged
merged 11 commits into from
Jul 6, 2023
Merged

Amend AcknowledgePacket to handle flushing #3922

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
39070be
Amend Ack packet to keep acknowledging if we're in the process of flu…
DimitrisJim Jun 22, 2023
ffa311e
Use handshake to reach correct state before mutating any fields.
DimitrisJim Jun 23, 2023
864011a
Add test to verify post-ack channel state after last in-flight packet.
DimitrisJim Jun 28, 2023
aef4e8f
Remove unecessary modifications of version for non initializing chann…
DimitrisJim Jun 28, 2023
27bbd5e
Test both cases: final in-flight packet and non-final one.
DimitrisJim Jun 28, 2023
f76b257
Apply suggestions from code review
DimitrisJim Jun 30, 2023
3ed00e9
Remove manual setting of flush status.
DimitrisJim Jul 3, 2023
6e3e520
Merge branch '04-channel-upgrades' into jim/flush-status-packet-ack
DimitrisJim Jul 4, 2023
f794a4b
Merge branch '04-channel-upgrades' into jim/flush-status-packet-ack
DimitrisJim Jul 5, 2023
d247777
Update test name, pass mock version to both channels.
DimitrisJim Jul 5, 2023
6be48f0
Merge branch '04-channel-upgrades' into jim/flush-status-packet-ack
crodriguezvega 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
9 changes: 7 additions & 2 deletions modules/core/04-channel/keeper/packet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,10 +380,10 @@ func (k Keeper) AcknowledgePacket(
)
}

if channel.State != types.OPEN {
if channel.State != types.OPEN && channel.FlushStatus != types.FLUSHING {
DimitrisJim marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor Author

Choose a reason for hiding this comment

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

how does this read to folks? Can refactor in a similar way to SendPacket but this conditional isn't as beefy.

Copy link
Member

Choose a reason for hiding this comment

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

Don't we want to be able to process acks in TRYUPGRADE and ACKUPGRADE states?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah but only if we're flushing, right?

Copy link
Member

Choose a reason for hiding this comment

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

aha, yea this is kind of subtle

return errorsmod.Wrapf(
types.ErrInvalidChannelState,
"channel state is not OPEN (got %s)", channel.State.String(),
"packets cannot be acknowledged on channel with state (%s) and flush status (%s)", channel.State, channel.FlushStatus,
)
}

Expand Down Expand Up @@ -477,6 +477,11 @@ func (k Keeper) AcknowledgePacket(
// Delete packet commitment, since the packet has been acknowledged, the commitement is no longer necessary
k.deletePacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence())

if channel.FlushStatus == types.FLUSHING && !k.hasInflightPackets(ctx, packet.GetSourcePort(), packet.GetSourceChannel()) {
DimitrisJim marked this conversation as resolved.
Show resolved Hide resolved
channel.FlushStatus = types.FLUSHCOMPLETE
k.SetChannel(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), channel)
}

// log that a packet has been acknowledged
k.Logger(ctx).Info(
"packet acknowledged",
Expand Down
117 changes: 117 additions & 0 deletions 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 @@ -773,6 +773,31 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() {

channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
}, true},
{"success on channel in tryupgrade and flush status in flushing", func() {
// setup uses an UNORDERED channel
suite.coordinator.Setup(path)

// create packet commitment
sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData)
suite.Require().NoError(err)

// create packet receipt and acknowledgement
packet = types.NewPacket(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, defaultTimeoutHeight, disabledTimeoutTimestamp)
err = path.EndpointB.RecvPacket(packet)
Copy link
Contributor

Choose a reason for hiding this comment

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

super duper nit:

Suggested change
err = path.EndpointB.RecvPacket(packet)
suite.Require().NoError(path.EndpointB.RecvPacket(packet))

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'm honestly not sure what avenue to chose when doing this sort of error checking. suite.Require.NoError is super nice when it doesn't result in a behemoth of a line. When the line length gets excessive, I'm more in favor of having the assignment since it helps when visually parsing things.

In the current file, we do go with err = ... followed by suite.Require().NoError(err) with RecvPacket, I'm happy to switch it up here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am honestly not so opinionated, it's just some feedback I've gotten before that I thought made sense so I'm passing it along now. but I also see your reasoning with the line length, so I think it's just up to you here :) no need to push another commit if you feel it's more readable this way!

suite.Require().NoError(err)

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

// Move channel to correct state.
path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = ibcmock.UpgradeVersion
path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = ibcmock.UpgradeVersion
DimitrisJim marked this conversation as resolved.
Show resolved Hide resolved

err = path.EndpointB.ChanUpgradeInit()
suite.Require().NoError(err)

err = path.EndpointA.ChanUpgradeTry()
suite.Require().NoError(err)
}, true},
{"packet already acknowledged ordered channel (no-op)", func() {
expError = types.ErrNoOpMsg

Expand Down Expand Up @@ -830,6 +855,26 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() {
suite.Require().NoError(err)
channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
}, false},
{"channel in ackupgrade and flush status flush complete", func() {
expError = types.ErrInvalidChannelState

suite.coordinator.Setup(path)
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)

// Move channel to correct state.
path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = ibcmock.UpgradeVersion
path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = ibcmock.UpgradeVersion
DimitrisJim marked this conversation as resolved.
Show resolved Hide resolved

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

err = path.EndpointB.ChanUpgradeTry()
suite.Require().NoError(err)

err = path.EndpointA.ChanUpgradeAck()
suite.Require().NoError(err)
}, false},
{"capability authentication failed ORDERED", func() {
expError = types.ErrInvalidChannelCapability

Expand Down Expand Up @@ -1021,3 +1066,75 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() {
})
}
}

// TestAcknowledgeFlushStatus tests that Acknowledging the last in-flight packet moves the channel
// flush status to FLUSHCOMPLETE.
func (suite *KeeperTestSuite) TestAcknowledgeFlushStatus() {
var path *ibctesting.Path

testCases := []struct {
msg string
malleate func()
expectedFlushStatus types.FlushStatus
}{
{
"success: final packet commitment cleared, flush status set to FLUSHCOMPLETE",
func() {},
types.FLUSHCOMPLETE,
},
{
"success: has in-flight packets, flush status remains FLUSHING",
func() {
// Send an additional packet
sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData)
suite.Require().Equal(uint64(2), sequence)
suite.Require().NoError(err)
},
types.FLUSHING,
},
}

for i, tc := range testCases {
tc := tc
suite.Run(fmt.Sprintf("Case %s, %d/%d tests", tc.msg, i, len(testCases)), func() {
suite.SetupTest() // reset
path = ibctesting.NewPath(suite.chainA, suite.chainB)
suite.coordinator.Setup(path)

// create packet commitment
sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData)
suite.Require().NoError(err)

// create packet receipt and acknowledgement
packet := types.NewPacket(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, defaultTimeoutHeight, disabledTimeoutTimestamp)
Copy link
Member

Choose a reason for hiding this comment

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

I think we could also reuse this rather than fully building the expected packet here

packet := ibctesting.ParsePacketFromEvents(suite.chainA.GetContext().EventManager().Events())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

curiously, this doesn't work, events are empty, am not seeing why

Copy link
Member

@damiannolan damiannolan Jul 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

ah, calling CommitBlock in the endpoint's SendPacket probably flushes the events from the event manager on the current context. So by the time we're using suite.chainA.GetContext() here its actually a new ctx...

Most certainly easier to leave this as is, forget my suggestion 😅

Copy link
Member

Choose a reason for hiding this comment

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

specifically its when we call NextBlock in the testing pkg, this will call abci Commit() and BeginBlock() which will create a new ctx https://github.com/cosmos/cosmos-sdk/blob/v0.47.3/baseapp/abci.go#L184-L193

Copy link
Contributor Author

Choose a reason for hiding this comment

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

now that's what I call music context ❤️

err = path.EndpointB.RecvPacket(packet)
suite.Require().NoError(err)

tc.malleate()

// Move channel to UPGRADE_ACK, flush status set to flushing due to previous SendPacket
path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = ibcmock.UpgradeVersion
path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = ibcmock.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)

packetKey := host.PacketAcknowledgementKey(packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence())
proof, proofHeight := path.EndpointB.QueryProof(packetKey)

channelCap := suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
err = suite.chainA.App.GetIBCKeeper().ChannelKeeper.AcknowledgePacket(suite.chainA.GetContext(), channelCap, packet, ibcmock.MockAcknowledgement.Acknowledgement(), proof, proofHeight)
suite.Require().NoError(err)

// Check that we've moved to expected flush status
channelA := path.EndpointA.GetChannel()
suite.Require().Equal(tc.expectedFlushStatus, channelA.FlushStatus)
})
}
}
Loading