-
Notifications
You must be signed in to change notification settings - Fork 448
Allow cancellation of pending splice funding negotiations #4490
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12255,30 +12255,77 @@ where | |
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| pub fn abandon_splice( | ||
| &mut self, | ||
| ) -> Result<(msgs::TxAbort, Option<SpliceFundingFailed>), APIError> { | ||
| if self.should_reset_pending_splice_state(false) { | ||
| let tx_abort = | ||
| msgs::TxAbort { channel_id: self.context.channel_id(), data: Vec::new() }; | ||
| let splice_funding_failed = self.reset_pending_splice_state(); | ||
| Ok((tx_abort, splice_funding_failed)) | ||
| } else if self.has_pending_splice_awaiting_signatures() { | ||
| Err(APIError::APIMisuseError { | ||
| pub fn cancel_splice(&mut self) -> Result<InteractiveTxMsgError, APIError> { | ||
| let funding_negotiation = self | ||
| .pending_splice | ||
| .as_ref() | ||
| .and_then(|pending_splice| pending_splice.funding_negotiation.as_ref()); | ||
| let Some(funding_negotiation) = funding_negotiation else { | ||
| return Err(APIError::APIMisuseError { | ||
| err: format!( | ||
| "Channel {} splice cannot be abandoned; already awaiting signatures", | ||
| self.context.channel_id(), | ||
| "Channel {} does not have a pending splice negotiation", | ||
| self.context.channel_id() | ||
| ), | ||
| }) | ||
| } else { | ||
| Err(APIError::APIMisuseError { | ||
| }); | ||
| }; | ||
|
|
||
| let made_contribution = match funding_negotiation { | ||
| 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() | ||
| }, | ||
| FundingNegotiation::AwaitingSignatures { .. } => self | ||
| .context | ||
| .interactive_tx_signing_session | ||
| .as_ref() | ||
| .expect("We have a pending splice awaiting signatures") | ||
| .has_local_contribution(), | ||
|
Comment on lines
+12281
to
+12286
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: the contribution check here ( 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. |
||
| }; | ||
| if !made_contribution { | ||
| return Err(APIError::APIMisuseError { | ||
| err: format!( | ||
| "Channel {} splice cannot be abandoned; no pending splice", | ||
| self.context.channel_id(), | ||
| "Channel {} has a pending splice negotiation with no contribution made", | ||
| self.context.channel_id() | ||
| ), | ||
| }) | ||
| }); | ||
| } | ||
|
|
||
| // We typically don't reset the pending funding negotiation when we're in | ||
| // [`FundingNegotiation::AwaitingSignatures`] since we're able to resume it on | ||
| // re-establishment, so we still need to handle this case separately if the user wishes to | ||
| // cancel. If they've yet to call [`Channel::funding_transaction_signed`], then we can | ||
| // guarantee to never have sent any signatures to the counterparty, or have processed any | ||
| // signatures from them. | ||
| if matches!(funding_negotiation, FundingNegotiation::AwaitingSignatures { .. }) { | ||
| let already_signed = self | ||
| .context | ||
| .interactive_tx_signing_session | ||
| .as_ref() | ||
| .expect("We have a pending splice awaiting signatures") | ||
| .holder_tx_signatures() | ||
| .is_some(); | ||
| if already_signed { | ||
| return Err(APIError::APIMisuseError { | ||
| err: format!( | ||
| "Channel {} has pending splice negotiation that was already signed", | ||
| self.context.channel_id(), | ||
| ), | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| debug_assert!(self.context.channel_state.is_quiescent()); | ||
| let splice_funding_failed = self.reset_pending_splice_state(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When this asserts on
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to worry about updating |
||
| debug_assert!(splice_funding_failed.is_some()); | ||
| Ok(InteractiveTxMsgError { | ||
| err: ChannelError::Abort(AbortReason::ManualIntervention), | ||
| splice_funding_failed, | ||
| exited_quiescence: true, | ||
| }) | ||
| } | ||
|
|
||
| /// Checks during handling splice_init | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4741,7 +4741,9 @@ impl< | |
| /// [`Event::DiscardFunding`] is seen. | ||
| /// | ||
| /// After initial signatures have been exchanged, [`Event::FundingTransactionReadyForSigning`] | ||
| /// will be generated and [`ChannelManager::funding_transaction_signed`] should be called. | ||
| /// may be generated. To proceed, call [`ChannelManager::funding_transaction_signed`]. To cancel | ||
| /// the pending splice negotiation instead, call [`ChannelManager::cancel_splice`] before | ||
| /// providing the funding signatures. | ||
| /// | ||
| /// If any failures occur while negotiating the funding transaction, an [`Event::SpliceFailed`] | ||
| /// will be emitted. Any contributed inputs no longer used will be included here and thus can | ||
|
|
@@ -4887,96 +4889,108 @@ impl< | |
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| pub(crate) fn abandon_splice( | ||
| &self, channel_id: &ChannelId, counterparty_node_id: &PublicKey, | ||
| ) -> Result<(), APIError> { | ||
| let mut res = Ok(()); | ||
| PersistenceNotifierGuard::optionally_notify(self, || { | ||
| let result = self.internal_abandon_splice(channel_id, counterparty_node_id); | ||
| res = result; | ||
| match res { | ||
| Ok(_) => NotifyOption::SkipPersistHandleEvents, | ||
| Err(_) => NotifyOption::SkipPersistNoEvents, | ||
| } | ||
| }); | ||
| res | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| fn internal_abandon_splice( | ||
| /// Cancels a pending splice negotiation for which a local contribution was made and queues a | ||
| /// `tx_abort` for the counterparty. | ||
| /// | ||
| /// 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. | ||
|
Comment on lines
+4895
to
+4898
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should clarify what happens when the negotiation is for an RBF. Perhaps we should rename the method to |
||
| /// | ||
| /// Returns [`ChannelUnavailable`] when a channel is not found or an incorrect | ||
| /// `counterparty_node_id` is provided. | ||
| /// | ||
| /// Returns [`APIMisuseError`] when the channel is not funded, has no pending splice to cancel, | ||
| /// the pending splice has no local contribution to reclaim, or the pending splice can no longer | ||
| /// be canceled. | ||
| /// | ||
| /// [`Event::FundingTransactionReadyForSigning`]: events::Event::FundingTransactionReadyForSigning | ||
| /// [`ChannelUnavailable`]: APIError::ChannelUnavailable | ||
| /// [`APIMisuseError`]: APIError::APIMisuseError | ||
| pub fn cancel_splice( | ||
| &self, channel_id: &ChannelId, counterparty_node_id: &PublicKey, | ||
| ) -> Result<(), APIError> { | ||
| let per_peer_state = self.per_peer_state.read().unwrap(); | ||
|
|
||
| let peer_state_mutex = match per_peer_state | ||
| .get(counterparty_node_id) | ||
| .ok_or_else(|| APIError::no_such_peer(counterparty_node_id)) | ||
| { | ||
| Ok(p) => p, | ||
| Err(e) => return Err(e), | ||
| }; | ||
|
|
||
| let mut peer_state_lock = peer_state_mutex.lock().unwrap(); | ||
| let peer_state = &mut *peer_state_lock; | ||
| let mut result = Ok(()); | ||
| PersistenceNotifierGuard::manually_notify(self, || { | ||
| let per_peer_state = self.per_peer_state.read().unwrap(); | ||
| let peer_state_mutex = match per_peer_state | ||
| .get(counterparty_node_id) | ||
| .ok_or_else(|| APIError::no_such_peer(counterparty_node_id)) | ||
| { | ||
| Ok(p) => p, | ||
| Err(e) => { | ||
| result = Err(e); | ||
| return; | ||
| }, | ||
| }; | ||
| let mut peer_state_lock = peer_state_mutex.lock().unwrap(); | ||
| let peer_state = &mut *peer_state_lock; | ||
|
|
||
| // Look for the channel | ||
| match peer_state.channel_by_id.entry(*channel_id) { | ||
| hash_map::Entry::Occupied(mut chan_phase_entry) => { | ||
| if !chan_phase_entry.get().context().is_connected() { | ||
| // TODO: We should probably support this, but right now `splice_channel` refuses when | ||
| // the peer is disconnected, so we just check it here. | ||
| return Err(APIError::ChannelUnavailable { | ||
| err: "Cannot abandon splice while peer is disconnected".to_owned(), | ||
| }); | ||
| } | ||
| match peer_state.channel_by_id.entry(*channel_id) { | ||
| hash_map::Entry::Occupied(mut chan_entry) => { | ||
| if let Some(channel) = chan_entry.get_mut().as_funded_mut() { | ||
| let InteractiveTxMsgError { err, splice_funding_failed, exited_quiescence } = | ||
| match channel.cancel_splice() { | ||
| Ok(v) => v, | ||
| Err(e) => { | ||
| result = Err(e); | ||
| return; | ||
| }, | ||
| }; | ||
|
|
||
| if let Some(chan) = chan_phase_entry.get_mut().as_funded_mut() { | ||
| let (tx_abort, splice_funding_failed) = chan.abandon_splice()?; | ||
| let splice_funding_failed = splice_funding_failed | ||
| .expect("Only splices with local contributions can be canceled"); | ||
|
Comment on lines
+4941
to
+4942
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This The |
||
| { | ||
| let pending_events = &mut self.pending_events.lock().unwrap(); | ||
| pending_events.push_back(( | ||
| events::Event::SpliceFailed { | ||
| channel_id: *channel_id, | ||
| counterparty_node_id: *counterparty_node_id, | ||
| user_channel_id: channel.context().get_user_id(), | ||
| abandoned_funding_txo: splice_funding_failed.funding_txo, | ||
| channel_type: splice_funding_failed.channel_type.clone(), | ||
| }, | ||
| None, | ||
| )); | ||
| pending_events.push_back(( | ||
| events::Event::DiscardFunding { | ||
| channel_id: *channel_id, | ||
| funding_info: FundingInfo::Contribution { | ||
| inputs: splice_funding_failed.contributed_inputs, | ||
| outputs: splice_funding_failed.contributed_outputs, | ||
| }, | ||
| }, | ||
| None, | ||
| )); | ||
| } | ||
|
|
||
| peer_state.pending_msg_events.push(MessageSendEvent::SendTxAbort { | ||
| node_id: *counterparty_node_id, | ||
| msg: tx_abort, | ||
| }); | ||
| mem::drop(peer_state_lock); | ||
| mem::drop(per_peer_state); | ||
|
|
||
| if let Some(splice_funding_failed) = splice_funding_failed { | ||
| let pending_events = &mut self.pending_events.lock().unwrap(); | ||
| pending_events.push_back(( | ||
| events::Event::SpliceFailed { | ||
| channel_id: *channel_id, | ||
| counterparty_node_id: *counterparty_node_id, | ||
| user_channel_id: chan.context.get_user_id(), | ||
| abandoned_funding_txo: splice_funding_failed.funding_txo, | ||
| channel_type: splice_funding_failed.channel_type, | ||
| }, | ||
| None, | ||
| )); | ||
| pending_events.push_back(( | ||
| events::Event::DiscardFunding { | ||
| channel_id: *channel_id, | ||
| funding_info: FundingInfo::Contribution { | ||
| inputs: splice_funding_failed.contributed_inputs, | ||
| outputs: splice_funding_failed.contributed_outputs, | ||
| }, | ||
| }, | ||
| None, | ||
| )); | ||
| self.needs_persist_flag.store(true, Ordering::Release); | ||
| self.event_persist_notifier.notify(); | ||
| let err: Result<(), _> = | ||
| Err(MsgHandleErrInternal::from_chan_no_close(err, *channel_id) | ||
| .with_exited_quiescence(exited_quiescence)); | ||
| let _ = self.handle_error(err, *counterparty_node_id); | ||
| } else { | ||
| result = Err(APIError::ChannelUnavailable { | ||
| err: format!( | ||
| "Channel with id {} is not funded, cannot cancel splice", | ||
| channel_id | ||
| ), | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| Ok(()) | ||
| } else { | ||
| Err(APIError::ChannelUnavailable { | ||
| err: format!( | ||
| "Channel with id {} is not funded, cannot abandon splice", | ||
| channel_id | ||
| ), | ||
| }) | ||
| } | ||
| }, | ||
| hash_map::Entry::Vacant(_) => { | ||
| Err(APIError::no_such_channel_for_peer(channel_id, counterparty_node_id)) | ||
| }, | ||
| } | ||
| }, | ||
| hash_map::Entry::Vacant(_) => { | ||
| result = | ||
| Err(APIError::no_such_channel_for_peer(channel_id, counterparty_node_id)); | ||
| return; | ||
| }, | ||
| } | ||
| }); | ||
| result | ||
| } | ||
|
|
||
| fn forward_needs_intercept_to_known_chan( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we parameterize the tests to allow coverage here?