Allow cancellation of pending splice funding negotiations#4490
Allow cancellation of pending splice funding negotiations#4490wpaulino wants to merge 2 commits intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
| let splice_funding_failed = splice_funding_failed | ||
| .expect("Only splices with local contributions can be canceled"); |
There was a problem hiding this comment.
This .expect() can panic in release builds. While cancel_splice in channel.rs verifies made_contribution is true, the maybe_create_splice_funding_failed! macro additionally subtracts contributions that overlap with prior RBF rounds (via prior_contributed_inputs/outputs). For a non-initiator who reuses the same UTXOs across RBF attempts with no explicit output contributions, the subtraction can empty the lists, causing the macro to return None (line 6740-6742 of channel.rs: if !is_initiator && contributed_inputs.is_empty() && contributed_outputs.is_empty() { return None; }).
The debug_assert!(splice_funding_failed.is_some()) at channel.rs:12323 catches this in debug, but this expect will crash in release for that edge case. Consider handling None gracefully, e.g. by returning an APIError or skipping the events.
There was a problem hiding this comment.
@jkczyz looks like that if statement it's referring to is indeed happening after the filtering. We should check whether the contribution is empty prior to filtering. I also noticed that we'll always emit DiscardFunding even when both contributed inputs and outputs are empty, we should only do so when there is actually something to discard.
There was a problem hiding this comment.
@jkczyz looks like that
ifstatement it's referring to is indeed happening after the filtering. We should check whether the contribution is empty prior to filtering.
Hmm... we may want to base this on #4514. It refactors that code a bit and removes maybe_create_splice_funding_failed!. I believe the new splice_funding_failed_for! macro does it correctly now (dc0609d)
I also noticed that we'll always emit
DiscardFundingeven when both contributed inputs and outputs are empty, we should only do so when there is actually something to discard.
I believe that is fixed in #4514, too. Can't recall if it's the same commit, but the PR should consolidate the filtering logic to FundingContribution::into_unique_contributions.
|
After a thorough review of the entire PR diff, I confirm that my previously flagged issues remain applicable and no new issues were found in this review pass. All call sites for the Review SummaryNo new issues found beyond the four previously flagged. All prior issues remain applicable to the current diff: Previously posted inline comments (still active):
Verification notes:
|
| } | ||
|
|
||
| debug_assert!(self.context.channel_state.is_quiescent()); | ||
| let splice_funding_failed = self.reset_pending_splice_state(); |
There was a problem hiding this comment.
Do we need to worry about updating PendingFunding::contributions when reseting? This may be a pre-existing issue, though.
|
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @jkczyz! This PR has been waiting for your review. |
| let mut result = Ok(()); | ||
| PersistenceNotifierGuard::manually_notify(self, || { |
There was a problem hiding this comment.
The previous abandon_splice used PersistenceNotifierGuard::optionally_notify and returned NotifyOption::SkipPersistHandleEvents on success, which would wake up the background processor to deliver the newly-added SpliceFailed/DiscardFunding events and the tx_abort message promptly.
manually_notify delegates the notification decision to process_background_events(), which returns SkipPersistNoEvents when there are no pending background events. This means when no holding-cell HTLCs are freed (the handle_holding_cell_free_result path that calls notify()), the background processor is never woken up. Events and messages sit in memory until the next polling interval or until some unrelated activity triggers notification.
This is also a persistence concern: the splice state reset and quiescence exit modify channel state, but without notification, persistence may not happen until the next trigger. If the node crashes in between, the cancel is effectively lost (though the reestablishment protocol handles recovery).
Consider using optionally_notify (returning SkipPersistHandleEvents on success, SkipPersistNoEvents on error) to match the behavior of other public APIs like funding_contributed and funding_transaction_signed.
| acceptor.node.handle_tx_abort(node_id_initiator, tx_abort); | ||
| if state == 0 { | ||
| assert!(acceptor.node.get_and_clear_pending_msg_events().is_empty()); |
There was a problem hiding this comment.
Dead code: state == 0 can never be true here because that case returned early at line 2990. The if branch is unreachable — only the else ever runs.
| acceptor.node.handle_tx_abort(node_id_initiator, tx_abort); | |
| if state == 0 { | |
| assert!(acceptor.node.get_and_clear_pending_msg_events().is_empty()); | |
| acceptor.node.handle_tx_abort(node_id_initiator, tx_abort); | |
| { | |
| let tx_abort = get_event_msg!(acceptor, MessageSendEvent::SendTxAbort, node_id_initiator); |
A user may wish to cancel an in-flight funding negotiation for whatever reason (e.g., mempool feerates have gone down, inability to sign, etc.), so we should make it possible for them to do so. Note that this can only be done for splice funding negotiations for which the user has made a contribution to.
There's a case in `should_reset_pending_splice_state` where we are awaiting signatures, but still want to preserve the pending negotiation upon a disconnection. We previously used `counterparty_aborted` as a way to toggle this behavior. Now that we support the user manually canceling an ongoing negotiation, we interpret the argument a bit more generically in terms of whether we wish to resume the negotiation or not when we are found in such a state.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4490 +/- ##
==========================================
+ Coverage 87.15% 87.16% +0.01%
==========================================
Files 161 161
Lines 109251 109278 +27
Branches 109251 109278 +27
==========================================
+ Hits 95215 95256 +41
+ Misses 11560 11549 -11
+ Partials 2476 2473 -3
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:
|
4 participants
A user may wish to cancel an in-flight funding negotiation for whatever reason (e.g., mempool feerates have gone down, inability to sign, etc.), so we should make it possible for them to do so. Note that this can only be done for splice funding negotiations for which the user has made a contribution to.