Add FlushStatus Checks to RecvPacket

[chatton]

Description

closes: #3876

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against correct branch (see CONTRIBUTING.md).
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards and Go style guide.
• Wrote unit and integration tests.
• Updated relevant documentation (docs/) or specification (x/<module>/spec/).
• Added relevant godoc comments.
• Provide a commit message to be used for the changelog entry in the PR description for review.
• Re-reviewed Files changed in the Github PR explorer.
• Review Codecov Report in the comment section below once CI passes.

[crodriguezvega]

Thanks, @chatton! I left a couple of comments.

[crodriguezvega]

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

[DimitrisJim]

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)

[charleenfei]

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

[charleenfei]

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?

[DimitrisJim]

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)

[charleenfei]

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

[crodriguezvega]

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?

[DimitrisJim]

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.

[crodriguezvega]

The error message should be updated as well, right?

[DimitrisJim]

let me know what you think of the updated message

[DimitrisJim]

pushing here too with Cian's blessing

[DimitrisJim]

by default ends up in [ACKUPGRADE, FLUSHING]. The fact that we have sent a packet before the upgrade should mean even after we make the changes in #3928, this will still be in FLUSHING. Correct me if I'm wrong (and we can set the flush status manually here).

[DimitrisJim]

only setting channel to closed, flush status stays NOTINFLUSH so we are only checking the counterparty sequence > packet sequence part of the if.

[charleenfei]

also, if the counterparty sequence > packet sequence shouldn't the tx actually work? in this case it's only failing bc the flushstatus is NOTINFLUSH

[DimitrisJim]

rebased since history got messy after the base branch was automatically changed and tweaked a final thing. believe this should be r4r

[crodriguezvega]

The error message will not provide enough information if the channel is not open and flushing, but the packet sequence is > counterpartyLastPacketSent. I guess it would be very informative if we have different error messages depending on the condition that was not satisfied?

[chatton]

ACK the latest changes made by @charleenfei, the new condition looks good. I think it is still quite complex, but the added context to the error message should provide enough clarity as to the purpose of the checks!

[DimitrisJim]

Also lgtm but since I was also slightly involved with this I'd rather leave someone else to also give it a view.

[damiannolan]

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

[chatton]

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.

[damiannolan]

I definitely found this section pretty hard to parse and reason about. Looks like there was already some discussion in another thread about it.

I find it a bit complex to mix a channel state assertion with the && expression which opens the door for unexpected channel states to slip through. So here, we are only going to fail the recv packet tx if the channel state is not OPEN AND the channel state is not flushing and the sequencing is all good.

So if we are in UPGRADETRY or UPGRADEACK, or any other state for that matter then the first part of the expression will evaluate as false.

What do you think about explicitly checking the expected channel states? I would be more confident in the code and see less room for unexpected states to slip through the cracks with something like:

if !collections.Contains(channel.State, []types.State{types.OPEN, types.TRYUPGRADE, types.ACKUPGRADE}) {
	return errorsmod.Wrapf(types.ErrInvalidChannelState, "channel state is not OPEN (got %s)", channel.State.String())
}

if counterpartyLastSequenceSend, found := k.GetCounterpartyLastPacketSequence(ctx, packet.GetDestPort(), packet.GetDestChannel()); found {
	if channel.FlushStatus != types.FLUSHING && packet.GetSequence() > counterpartyLastSequenceSend {
		return errorsmod.Wrap(types.ErrInvalidChannelState, "something meaningful")
	}
}

I think we can also make use of the found bool with last sequence send instead of discarding it

[damiannolan]

Slightly related #4001 - I noticed counterpartyLastSequenceSend was never being set in store within the upgrade handshake

[chatton]

I'm a big fan of this change actually, it makes things far more readable.

[DimitrisJim]

note the last conditional should be ||, righto?

[DimitrisJim]

I think it might also make sense to add a comment that the second check for the last seq sent is only meaningful for UPGRADETRY/ACK?

[chatton]

Currently the tests pass with both || and && so I think we don't have enough test coverage 😅

[chatton]

But yes I believe || is correct here! If either of those conditions are false we should error.

[DimitrisJim]

also, for anyone else reviewing, the original condition was completely flipped with Damians suggestion, hence why we can break the giant if in two smaller ones. && cannot be broken directly like this but since it was flipped to an || it can, this kept me up a bit.

[DimitrisJim]

Note that directly calling String here isn't necessary (also see #3946). I'll leave it to your discretion to choose, we can change all these in a later PR.

[damiannolan]

Nice! I just left some suggestions on error msgs :)

[damiannolan]

Suggested change

	return errorsmod.Wrapf(types.ErrInvalidChannelState, "channel state was not one of [%s, %s, %s] (got %s)", types.OPEN.String(), types.TRYUPGRADE.String(), types.ACKUPGRADE.String(), channel.State.String())
	return errorsmod.Wrapf(types.ErrInvalidChannelState, "expected channel state to be one of [%s, %s, %s], but got %s", types.OPEN.String(), types.TRYUPGRADE.String(), types.ACKUPGRADE.String(), channel.State.String())

[damiannolan]

I wonder could we improve this error and msg a bit. I'm not sure if this is much better but it would be good to indicate that we expect the channel to be flushing when the counterparty last seq send is set.

"expected channel flush status to be FLUSHING when counterparty last sequence send (100) is set, failed to recv packet at sequence (99)"

we could also change to ErrInvalidFlushStatus maybe? what do you think?

Suggested change

	return errorsmod.Wrapf(types.ErrInvalidChannelState, "expected channel flush status to not be (%s) and for the packet sequence (%d) to be less than or equal to the counterparty last sequence send (%d)", types.FLUSHING.String(), packet.GetSequence(), counterpartyLastSequenceSend)
	return errorsmod.Wrapf(types.ErrInvalidFlushStatus, "expected channel flush status to be (%s) when counterparty last sequence send (%d) is set, failed to recv packet (%d)", types.FLUSHING, counterpartyLastSequenceSend, packet.GetSequence())

[charleenfei]

won't this fail if the channel is just in a normal OPEN state and NOTINFLUSH? am i misreading something

[charleenfei]

ah, it's only run if the counterpartyLastSeq send is found, which would only happen if we are in the upgrade process I guess.

[charleenfei]

nice one bringing it over the line <3