Allow cancellation of pending splice funding negotiations#4490
Allow cancellation of pending splice funding negotiations#4490wpaulino wants to merge 1 commit intolightningdevkit:mainfrom
Conversation
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.
|
👋 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.
| FundingNegotiation::AwaitingSignatures { .. } => self | ||
| .context | ||
| .interactive_tx_signing_session | ||
| .as_ref() | ||
| .expect("We have a pending splice awaiting signatures") | ||
| .has_local_contribution(), |
There was a problem hiding this comment.
Nit: the contribution check here (has_local_contribution) differs in how it computes "has contributions" compared to the AwaitingAck/ConstructingTransaction branches. has_local_contribution() uses the signing session's local_inputs_count/local_outputs_count (which filter by is_local on the constructed transaction metadata), while the other branches use contributed_inputs()/contributed_outputs() from the context/constructor (which use our_funding_inputs/our_funding_outputs).
These should be semantically equivalent since both exclude the shared input and reflect local contributions, but the different computation paths mean a divergence in one path wouldn't be caught by the other. Consider adding a comment noting the equivalence, or unifying the approach.
Review SummaryThe approach is sound—making
🤖 Generated with Claude Code |
| } | ||
|
|
||
| debug_assert!(self.context.channel_state.is_quiescent()); | ||
| let splice_funding_failed = self.reset_pending_splice_state(); |
There was a problem hiding this comment.
When this asserts on should_reset_pending_splice_state(true), the parameter means counterparty_aborted which isn't the case here. Does that logic still hold? Should the parameter be renamed?
| FundingNegotiation::AwaitingAck { context, .. } => { | ||
| context.contributed_inputs().next().is_some() | ||
| || context.contributed_outputs().next().is_some() | ||
| }, | ||
| FundingNegotiation::ConstructingTransaction { interactive_tx_constructor, .. } => { | ||
| interactive_tx_constructor.contributed_inputs().next().is_some() | ||
| || interactive_tx_constructor.contributed_outputs().next().is_some() | ||
| }, |
There was a problem hiding this comment.
Should we parameterize the tests to allow coverage here?
| /// This is primarily useful after receiving an [`Event::FundingTransactionReadyForSigning`] for | ||
| /// a splice if you no longer wish to proceed. The pending splice must still be pending | ||
| /// negotiation, which for the final signing stage means | ||
| /// [`ChannelManager::funding_transaction_signed`] must not have been called yet. |
There was a problem hiding this comment.
We should clarify what happens when the negotiation is for an RBF. Perhaps we should rename the method to cancel_funding_negotiation or cancel_funding_contributed?
| } | ||
|
|
||
| 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.
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.