Free holding cell in remaining quiescence-exit code paths#4415
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4415 +/- ##
==========================================
- Coverage 85.91% 85.90% -0.02%
==========================================
Files 156 156
Lines 103963 103990 +27
Branches 103963 103990 +27
==========================================
+ Hits 89322 89333 +11
- Misses 12119 12135 +16
Partials 2522 2522
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
wpaulino
dismissed
TheBlueMatt’s stale review
The merge-base changed after approval.
|
👋 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. |
|
note that github wont let this be merged until its rebased. Also not sure if @jkczyz wants to take a look or not. |
wpaulino
force-pushed
the
free-holding-cells-quiescence-exit
branch
from
d1ea893 to
5a610c7
Compare
|
Rebased and pushed fixups for #4412 |
wpaulino
force-pushed
the
free-holding-cells-quiescence-exit
branch
from
5a610c7 to
826113b
Compare
jkczyz
left a comment
jkczyz
left a comment
There was a problem hiding this comment.
Looks like fail_splice_on_tx_complete_error is failing in CI.
wpaulino
force-pushed
the
free-holding-cells-quiescence-exit
branch
2 times, most recently
from
4b7cfee to
f55c452
Compare
|
Had to rebase due to an import conflict in |
|
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
wpaulino
force-pushed
the
free-holding-cells-quiescence-exit
branch
from
f55c452 to
ac2631f
Compare
After cad88af, a few code paths that also lead to a quiescence exit were not accounted for. This commit addresses the path where we exit quiescence due to a processing error on a counterparty's `tx_add_input/output`, `tx_remove_input/output`, or `tx_complete` message.
After cad88af, a few code paths that also lead to a quiescence exit were not accounted for. This commit addresses the path where we exit quiescence due to processing a counterparty's `tx_abort` message.
After cad88af, a few code paths that also lead to a quiescence exit were not accounted for. This commit addresses the last remaining path where we exit quiescence when we exchange `tx_signatures` with the counterparty.
wpaulino
force-pushed
the
free-holding-cells-quiescence-exit
branch
from
ac2631f to
0766da8
Compare
|
Had to rebase again after #4290 |
|
🔔 2nd Reminder Hey @TheBlueMatt @jkczyz! This PR has been waiting for your review. |
| debug_assert!(res.as_ref().err().map_or(true, |err| !err.closes_channel())); | ||
| let _ = self.handle_error(res, counterparty_node_id); | ||
| persist | ||
| self.event_persist_notifier.notify(); |
There was a problem hiding this comment.
Hmm... why do we need to manually notify? Shouldn't dropping the PersistenceNotifierGuard cause notification if handle_error (via handle_holding_cell_free_result) sets the persistence flag (and only if it does)? Instead of manually_notify, I was thinking something like notify_if_needed.
There was a problem hiding this comment.
handle_holding_cell_free_result is not guaranteed to happen so we still need to notify the event handling when we have a response to the message
There was a problem hiding this comment.
Ah, though it looks like we unconditionally notify now whereas before we may have not (e.g., upon exchanging tx_complete when no signatures are needed from the user). Are we fine with that?
There was a problem hiding this comment.
If tx_complete is exchanged then we're already persisting because we have a new signing session
These errors will only ever affect our in-memory state, so there's no need to persist the ChannelManager when we come across one. Note that `tx_abort` is not included here because there is a possibility we force close the channel, which we should persist.
wpaulino
force-pushed
the
free-holding-cells-quiescence-exit
branch
from
0766da8 to
1b8617c
Compare
4 participants
Depends on #4412.
According to the spec, handling a
splice_initandsplice_ackmay returntx_abortto reject the splice, but we always send a warning and disconnect instead, so those are not included here.