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

Add FlushStatus Checks to RecvPacket #3914

Merged
merged 24 commits into from
Jul 5, 2023
Merged

Add FlushStatus Checks to RecvPacket #3914

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
a290fed
add check for not equal NOTINFLUSH for SendPacket
chatton Jun 21, 2023
adfc53e
added test cases
chatton Jun 21, 2023
31acbec
Merge branch '04-channel-upgrades' into cian/issue#3875-add-flush-sta…
chatton Jun 21, 2023
6a36bc5
adding test cases and conditional first pass
chatton Jun 21, 2023
40d9351
split out status flush check into separate conditional
chatton Jun 21, 2023
b182061
Merge branch 'cian/issue#3875-add-flush-status-check-in-sendpacket-' …
chatton Jun 21, 2023
79be234
refactoring to single if condition
chatton Jun 21, 2023
20da85f
Address feedback.
DimitrisJim Jun 23, 2023
b590bed
Tweak them tests.
DimitrisJim Jun 23, 2023
fb983cc
adding test cases and conditional first pass
chatton Jun 21, 2023
f8ac337
split out status flush check into separate conditional
chatton Jun 21, 2023
4f4ecf9
refactoring to single if condition
chatton Jun 21, 2023
cbdc846
Address feedback.
DimitrisJim Jun 23, 2023
9396ea4
Tweak them tests.
DimitrisJim Jun 23, 2023
4edd3b9
Don't set version for non-initiating chain.
DimitrisJim Jun 29, 2023
5ded370
Merge branch 'cian/issue#3876-add-flush-status-check-in-recvpacket-' …
charleenfei Jun 30, 2023
b8dce46
Merge branch '04-channel-upgrades' into cian/issue#3876-add-flush-sta…
charleenfei Jun 30, 2023
6f09439
update code+test
charleenfei Jun 30, 2023
aee4623
applied suggestion from PR feedback
chatton Jul 4, 2023
f047c40
added test cases to check individual or conditions
chatton Jul 4, 2023
919fa83
Merge branch '04-channel-upgrades' into cian/issue#3876-add-flush-sta…
chatton Jul 5, 2023
f146dc3
apply PR feedback about error messages
chatton Jul 5, 2023
3849847
Merge branch '04-channel-upgrades' into cian/issue#3876-add-flush-sta…
chatton Jul 5, 2023
fa273eb
Merge branch '04-channel-upgrades' into cian/issue#3876-add-flush-sta…
chatton Jul 5, 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
12 changes: 10 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 @@ -33,7 +33,14 @@ func (k Keeper) SendPacket(
return 0, errorsmod.Wrap(types.ErrChannelNotFound, sourceChannel)
}

if channel.State != types.OPEN {
if channel.FlushStatus != types.NOTINFLUSH {
Copy link
Member

@damiannolan damiannolan Jul 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The following is duplicated below on L50?

if channel.FlushStatus != types.NOTINFLUSH {
	return 0, errorsmod.Wrapf(types.ErrInvalidFlushStatus, "expected flush status to be %s during packet send, got %s", types.NOTINFLUSH, channel.FlushStatus)
}

This check is also included in the expression on L43?

We can do both checks and use || but the error info would need to include both channel state and flush status. Or we can do both checks one after another which I'd be happy enough with too.

fwiw I think its an invalid/unreachable state to have state == OPEN and flushStatus != NOTINFLUSH

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, this check is duplicated, I think this is due to branching off the SendPacket branch before this was fixed. I think we can remove all changes to SendPacket in this PR.

return 0, errorsmod.Wrapf(
types.ErrInvalidChannelState,
"channel should not be in %s state", types.NOTINFLUSH.String(),
)
}

if channel.State != types.OPEN || channel.FlushStatus != types.NOTINFLUSH {
return 0, errorsmod.Wrapf(
types.ErrInvalidChannelState,
"channel is not OPEN (got %s)", channel.State.String(),
Expand Down Expand Up @@ -129,7 +136,8 @@ func (k Keeper) RecvPacket(
return errorsmod.Wrap(types.ErrChannelNotFound, packet.GetDestChannel())
}

if channel.State != types.OPEN {
counterpartyLastPacketSent, _ := k.GetCounterpartyLastPacketSequence(ctx, packet.GetDestPort(), packet.GetDestChannel())
if channel.State != types.OPEN && (channel.FlushStatus != types.FLUSHING || counterpartyLastPacketSent > packet.GetSequence()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I find it easier to reason about like this:

Suggested change
if channel.State != types.OPEN && (channel.FlushStatus != types.FLUSHING || counterpartyLastPacketSent > packet.GetSequence()) {
if !(channel.State == types.OPEN || (channel.FlushStatus == types.FLUSHING && packet.GetSequence() <= counterpartyLastPacketSent)) {

Copy link
Contributor

Choose a reason for hiding this comment

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

we typically go with the approach of negating the spec conditions right? I do agree with you though that this makes it easier to reason (and, as a result, implement)

Copy link
Contributor

@charleenfei charleenfei Jun 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

agree with @crodriguezvega it's easier to reason about this way, made a small adjustment to the code

Copy link
Contributor

Choose a reason for hiding this comment

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

should we actually add a condition in the code that if the channel is CLOSED then we can't recv packets? kinda weird if we can receive packets then right?

Copy link
Contributor

@DimitrisJim DimitrisJim Jun 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

we shouldn't be able to. If state is CLOSED, flush status should be NOTINFLUSH and the condition should resolve to true (but see this comment #3914 (comment), I don't think its currently defined when a close can occur)

Copy link
Contributor

Choose a reason for hiding this comment

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

oh! didnt see that comment, thanks! yea. that makes sense.

return errorsmod.Wrapf(
types.ErrInvalidChannelState,
"channel state is not OPEN (got %s)", channel.State.String(),
Copy link
Contributor

Choose a reason for hiding this comment

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

The error message should be updated as well, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

let me know what you think of the updated message

Expand Down
76 changes: 76 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 @@ -217,6 +217,30 @@ func (suite *KeeperTestSuite) TestSendPacket() {

channelCap = capabilitytypes.NewCapability(5)
}, false},
{
"invalid flush status: FLUSHING",
func() {
suite.coordinator.Setup(path)
sourceChannel = path.EndpointA.ChannelID

channel := path.EndpointA.GetChannel()
channel.FlushStatus = types.FLUSHING
path.EndpointA.SetChannel(channel)
},
false,
},
{
"invalid flush status: FLUSHCOMPLETE",
func() {
suite.coordinator.Setup(path)
sourceChannel = path.EndpointA.ChannelID

channel := path.EndpointA.GetChannel()
channel.FlushStatus = types.FLUSHCOMPLETE
path.EndpointA.SetChannel(channel)
},
false,
},
}

for i, tc := range testCases {
Expand Down Expand Up @@ -294,6 +318,58 @@ func (suite *KeeperTestSuite) TestRecvPacket() {
// attempts to receive packet 2 without receiving packet 1
channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
}, true},
{
msg: "success with closed channel: FLUSHING status",
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, maybe the condition in RecvPacket needs to be tweaked a bit more? Because I would find it strange that the channel is closed but you can still receive the packet... Is it possible to even be in such state (channel closed and in flushing status)? I would expect the channel state in the test to be more like INITUPGRADE or TRYUPGRADE, right?

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 it currently is possible to be in such a state if a message to close the channel is submitted during the upgrade handshake? Even if I'm missing something and that isn't possible we should definitely amend the channel closing handlers to set the flushing status to NOTINFLUSH.

malleate: func() {
suite.coordinator.Setup(path)
sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, 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, defaultTimeoutHeight, disabledTimeoutTimestamp)
channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)

channel := path.EndpointB.GetChannel()
channel.State = types.CLOSED
channel.FlushStatus = types.FLUSHING
path.EndpointB.SetChannel(channel)
},
expPass: true,
},
{
msg: "failure with closed channel counterparty sequence > packet sequence",
malleate: func() {
suite.coordinator.Setup(path)
sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, 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, defaultTimeoutHeight, disabledTimeoutTimestamp)
channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)

channel := path.EndpointB.GetChannel()
channel.State = types.CLOSED
channel.FlushStatus = types.FLUSHING
path.EndpointB.SetChannel(channel)

suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetCounterpartyLastPacketSequence(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, sequence+1)
},
expPass: false,
},
{
msg: "failure with closed channel not flushing",
malleate: func() {
suite.coordinator.Setup(path)
sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, 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, defaultTimeoutHeight, disabledTimeoutTimestamp)
channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)

channel := path.EndpointB.GetChannel()
channel.State = types.CLOSED
channel.FlushStatus = types.FLUSHCOMPLETE
path.EndpointB.SetChannel(channel)

suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetCounterpartyLastPacketSequence(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, sequence)
},
expPass: false,
},
{"packet already relayed ORDERED channel (no-op)", func() {
expError = types.ErrNoOpMsg

Expand Down