Allow counterparty pending monitor update within quiescence handshake #3806
Conversation
Previously, if we were negotiating quiescence, and we had already sent our `stfu`, we'd disconnect upon receiving the counterparty's `stfu` if we had a pending monitor update. This could result from processing a counterparty's final `revoke_and_ack` to an update, and immediately processing their `stfu` (which is valid from their point of view) without complete the monitor update. This was unintended, as we are able to track the quiescent and pending monitor update flags at the same time. Note that this commit still considers whether our signer owes any messages, as these are indicative of a channel update still pending.
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3806 +/- ##
==========================================
- Coverage 89.77% 89.73% -0.04%
==========================================
Files 159 159
Lines 128828 128878 +50
Branches 128828 128878 +50
==========================================
+ Hits 115650 115653 +3
- Misses 10500 10537 +37
- Partials 2678 2688 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
|
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
There was a problem hiding this comment.
Confirmed this also fixes the issue for seed
export HEX="06000000001b000000000000020000000000000000200000be0000000000
000000000000ff0e0000a3a3a3a3a3a3a3a3a30015ffffffffffefff0000
000000ff"
which we just hit once more in #3804.
|
🔔 2nd Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
| @@ -9675,10 +9675,14 @@ impl<SP: Deref> FundedChannel<SP> where | |||
| self.mark_response_received(); | |||
|
|
|||
| if self.context.is_waiting_on_peer_pending_channel_update() | |||
| || self.context.is_monitor_or_signer_pending_channel_update() | |||
There was a problem hiding this comment.
What happens if we receive a commitment_signed, causing us to go monitor_update_in_progress and prep to send an RAA in response? I guess in theory they're not supposed to send their stfu after they sent their CS because they're waiting on a response from us, but also probably we want to reject that because trying to handle splicing when we have two pending commitments seems annoying?
There was a problem hiding this comment.
I guess in theory they're not supposed to send their stfu after they sent their CS because they're waiting on a response from us
This is allowed as long as the commitment_signed is not sent as a result of a local update. In any case, our implementation will accept an inbound stfu at any point, but it will hold back sending its own as a response until both is_waiting_on_peer_pending_channel_update and is_monitor_or_signer_pending_channel_update are false.
There was a problem hiding this comment.
I was thinking in the context of where we'd already sent our stfu. AFAIU if we send a stfu, then they send a CS followed by an stfu (for a local update, because if it was in response to our update we shouldn't have sent our stfu). They may violate the protocol, but we really should be rejecting it explicitly because otherwise we're gonna respond with an RAA+CS and mark ourselves quiescent even though there are two valid commitment transactions pending (and have to deal with that when splicing, which we shouldn't have to).
There was a problem hiding this comment.
I believe this case is already covered. We can receive commitment_signed if:
- a local update was made, and we now owe them a final
revoke_and_ackor - a remote update was made, and we now owe them a
revoke_and_ack+commitment_signed, and they owe us a finalrevoke_and_ack
In case 1, we cannot send stfu until the local update is no longer pending, so receiving stfu here doesn't change anything.
In case 2, we'll disconnect them if they send stfu immediately after commitment_signed because we're awaiting_remote_revoke (covered by is_waiting_on_peer_pending_channel_update).
Previously, if we were negotiating quiescence, and we had already sent our
stfu, we'd disconnect upon receiving the counterparty'sstfuif we had a pending monitor update. This could result from processing a counterparty's finalrevoke_and_ackto an update, and immediately processing theirstfu(which is valid from their point of view) without complete the monitor update. This was unintended, as we are able to track the quiescent and pending monitor update flags at the same time. Note that this commit still considers whether our signer owes any messages, as these are indicative of a channel update still pending.Fixes #3805.