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 @jkczyz 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 thorough review of the entire diff, I verified:
No new issues found beyond those already flagged in my prior review (the No issues found. |
| } | ||
|
|
||
| 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.
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.
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.