Free holding cell in remaining quiescence-exit code paths#4415
Free holding cell in remaining quiescence-exit code paths#4415wpaulino wants to merge 6 commits intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4415 +/- ##
=======================================
Coverage 86.07% 86.07%
=======================================
Files 156 156
Lines 103442 103466 +24
Branches 103442 103466 +24
=======================================
+ Hits 89033 89063 +30
+ Misses 11894 11887 -7
- Partials 2515 2516 +1
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 |
lightning/src/ln/channelmanager.rs
Outdated
| // Note that we're not able to do this inline in `internal_tx_add_input` as we | ||
| // usually do because we first need to send the `tx_abort` in `handle_error` above | ||
| // before sending any commitment updates. |
There was a problem hiding this comment.
Any reason why we can't include exited_quiescence in MsgHandleErrInternal and do this in handle_error?
There was a problem hiding this comment.
Good idea, this cleaned up nicely
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.
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
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.
| debug_assert!(res.as_ref().err().map_or(true, |err| !err.closes_channel())); | ||
| let _ = self.handle_error(res, counterparty_node_id); | ||
| persist | ||
| NotifyOption::SkipPersistHandleEvents |
There was a problem hiding this comment.
IIUC, handle_holding_cell_free_result may "override" this and force a persistence?
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.