From 51380bfc333f721eba185b97143fbd1754f0ba5b Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Wed, 14 Jan 2026 14:20:39 -0800 Subject: [PATCH 1/2] Rework ChannelManager::funding_transaction_signed Previously, we'd emit a `FundingTransactionReadyForSigning` event once the initial `commitment_signed` is exchanged for a splicing/dual-funding attempt and require users to call back with their signed inputs using `ChannelManager::funding_transaction_signed`. While this approach worked in practice, it prevents us from abandoning a splice if we cannot or no longer wish to sign as the splice has already been committed to by this point. This commit reworks the API such that this is now possible. After exchanging `tx_complete`, we will no longer immediately send our initial `commitment_signed`. We will now emit the `FundingTransactionReadyForSigning` event and wait for the user to call back before releasing both our initial `commitment_signed` and our `tx_signatures`. As a result, the event is now persisted, as there is only one possible path in which it is generated. Note that we continue to only emit the event if a local contribution to negotiated transaction was made. Future work will expose a cancellation API such that we can abandon splice attempts safely (we can just force close the channel with dual-funding). --- lightning/src/events/mod.rs | 39 +++- lightning/src/ln/channel.rs | 328 ++++++++++++++++------------- lightning/src/ln/channelmanager.rs | 270 +++++++++--------------- lightning/src/ln/interactivetxs.rs | 12 +- lightning/src/ln/splicing_tests.rs | 255 +++++++++------------- 5 files changed, 413 insertions(+), 491 deletions(-) diff --git a/lightning/src/events/mod.rs b/lightning/src/events/mod.rs index d97ae6097b6..4f2b8fa372b 100644 --- a/lightning/src/events/mod.rs +++ b/lightning/src/events/mod.rs @@ -1836,7 +1836,7 @@ pub enum Event { /// /// # Failure Behavior and Persistence /// This event will eventually be replayed after failures-to-handle (i.e., the event handler - /// returning `Err(ReplayEvent ())`), but will only be regenerated as needed after restarts. + /// returning `Err(ReplayEvent ())`) and will be persisted across restarts. /// /// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager /// [`ChannelManager::funding_transaction_signed`]: crate::ln::channelmanager::ChannelManager::funding_transaction_signed @@ -2305,10 +2305,19 @@ impl Writeable for Event { 47u8.write(writer)?; // Never write StaticInvoiceRequested events as buffered onion messages aren't serialized. }, - &Event::FundingTransactionReadyForSigning { .. } => { - 49u8.write(writer)?; - // We never write out FundingTransactionReadyForSigning events as they will be regenerated when - // necessary. + &Event::FundingTransactionReadyForSigning { + ref channel_id, + ref counterparty_node_id, + ref user_channel_id, + ref unsigned_transaction, + } => { + 48u8.write(writer)?; + write_tlv_fields!(writer, { + (1, channel_id, required), + (3, counterparty_node_id, required), + (5, user_channel_id, required), + (7, unsigned_transaction, required), + }); }, &Event::SplicePending { ref channel_id, @@ -2931,8 +2940,24 @@ impl MaybeReadable for Event { 45u8 => Ok(None), // Note that we do not write a length-prefixed TLV for StaticInvoiceRequested events. 47u8 => Ok(None), - // Note that we do not write a length-prefixed TLV for FundingTransactionReadyForSigning events. - 49u8 => Ok(None), + 48u8 => { + let mut f = || { + _init_and_read_len_prefixed_tlv_fields!(reader, { + (1, channel_id, required), + (3, counterparty_node_id, required), + (5, user_channel_id, required), + (7, unsigned_transaction, required), + }); + + Ok(Some(Event::FundingTransactionReadyForSigning { + channel_id: channel_id.0.unwrap(), + user_channel_id: user_channel_id.0.unwrap(), + counterparty_node_id: counterparty_node_id.0.unwrap(), + unsigned_transaction: unsigned_transaction.0.unwrap(), + })) + }; + f() + }, 50u8 => { let mut f = || { _init_and_read_len_prefixed_tlv_fields!(reader, { diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index d22359935fa..54a650c365d 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1905,7 +1905,7 @@ where pub fn tx_complete( &mut self, msg: &msgs::TxComplete, logger: &L, ) -> Result< - (Option, Option), + (Option, Option), (ChannelError, Option), > where @@ -1937,10 +1937,12 @@ where return Ok((interactive_tx_msg_send, None)); }; - let commitment_signed = self - .funding_tx_constructed(funding_outpoint, logger) - .map_err(|abort_reason| self.fail_interactive_tx_negotiation(abort_reason, logger))?; - Ok((interactive_tx_msg_send, Some(commitment_signed))) + let unsigned_tx_for_signing_event = + self.funding_tx_constructed(funding_outpoint, logger).map_err(|abort_reason| { + self.fail_interactive_tx_negotiation(abort_reason, logger) + })?; + + Ok((interactive_tx_msg_send, unsigned_tx_for_signing_event)) } pub fn tx_abort( @@ -2046,14 +2048,94 @@ where result.map(|monitor| (self.as_funded_mut().expect("Channel should be funded"), monitor)) } + pub fn funding_transaction_signed( + &mut self, funding_txid_signed: Txid, witnesses: Vec, + ) -> Result<(), APIError> { + if let Some(signing_session) = self.context().interactive_tx_signing_session.as_ref() { + if signing_session.holder_tx_signatures().is_some() { + // Our `tx_signatures` either should've been the first time we processed them, + // or we're waiting for our counterparty to send theirs first. + return Ok(()); + } + } else { + if Some(funding_txid_signed) == self.funding().get_funding_txid() { + // We may be handling a duplicate call and the funding was already locked so we + // no longer have the signing session present. + return Ok(()); + } + let err = + format!("Channel {} not expecting funding signatures", self.context().channel_id); + return Err(APIError::APIMisuseError { err }); + }; + + let (funding, context, pending_splice) = match &mut self.phase { + ChannelPhase::Undefined => unreachable!(), + ChannelPhase::UnfundedOutboundV1(_) | ChannelPhase::UnfundedInboundV1(_) => { + let err = format!( + "Channel {} not expecting funding signatures", + self.context().channel_id + ); + return Err(APIError::APIMisuseError { err }); + }, + ChannelPhase::UnfundedV2(channel) => (&channel.funding, &mut channel.context, None), + ChannelPhase::Funded(channel) => { + (&channel.funding, &mut channel.context, channel.pending_splice.as_ref()) + }, + }; + let signing_session = context + .interactive_tx_signing_session + .as_mut() + .ok_or(APIError::APIMisuseError { err: "Missing signing session".to_owned() })?; + + let tx = signing_session.unsigned_tx().tx(); + if funding_txid_signed != tx.compute_txid() { + let err = "Transaction was malleated prior to signing".to_owned(); + return Err(APIError::APIMisuseError { err }); + } + + let shared_input_signature = + if let Some(splice_input_index) = signing_session.unsigned_tx().shared_input_index() { + let sig = match &context.holder_signer { + ChannelSignerType::Ecdsa(signer) => signer.sign_splice_shared_input( + &funding.channel_transaction_parameters, + tx, + splice_input_index as usize, + &context.secp_ctx, + ), + #[cfg(taproot)] + ChannelSignerType::Taproot(_) => todo!(), + }; + Some(sig) + } else { + None + }; + debug_assert_eq!(pending_splice.is_some(), shared_input_signature.is_some()); + + let tx_signatures = msgs::TxSignatures { + channel_id: context.channel_id, + tx_hash: funding_txid_signed, + witnesses, + shared_input_signature, + }; + signing_session + .provide_holder_witnesses(tx_signatures, &context.secp_ctx) + .map_err(|err| APIError::APIMisuseError { err })?; + + // Even if we know the signer to not be async, we use `signer_pending_funding` here to + // signal that we need to send our initial commitment signed for the splice within + // [`FundedChannel::maybe_handle_funding_transaction_signed_post_action`]. + context.signer_pending_funding = true; + Ok(()) + } + fn funding_tx_constructed( &mut self, funding_outpoint: OutPoint, logger: &L, - ) -> Result + ) -> Result, AbortReason> where L::Target: Logger, { let logger = WithChannelContext::from(logger, self.context(), None); - let (interactive_tx_constructor, commitment_signed) = match &mut self.phase { + let interactive_tx_constructor = match &mut self.phase { ChannelPhase::UnfundedV2(chan) => { debug_assert_eq!( chan.context.channel_state, @@ -2077,19 +2159,7 @@ where .take() .expect("PendingV2Channel::interactive_tx_constructor should be set"); - let commitment_signed = - chan.context.get_initial_commitment_signed_v2(&chan.funding, &&logger); - let commitment_signed = match commitment_signed { - Some(commitment_signed) => commitment_signed, - // TODO(dual_funding): Support async signing - None => { - return Err(AbortReason::InternalError( - "Failed to compute commitment_signed signatures", - )); - }, - }; - - (interactive_tx_constructor, commitment_signed) + interactive_tx_constructor }, ChannelPhase::Funded(chan) => { if let Some(pending_splice) = chan.pending_splice.as_mut() { @@ -2115,33 +2185,15 @@ where "Got a tx_complete message in an invalid state", ) }) - .and_then(|(is_initiator, mut funding, interactive_tx_constructor)| { + .map(|(is_initiator, mut funding, interactive_tx_constructor)| { funding.channel_transaction_parameters.funding_outpoint = Some(funding_outpoint); - match chan.context.get_initial_commitment_signed_v2(&funding, &&logger) - { - Some(commitment_signed) => { - // Advance the state - pending_splice.funding_negotiation = - Some(FundingNegotiation::AwaitingSignatures { - is_initiator, - funding, - }); - Ok((interactive_tx_constructor, commitment_signed)) - }, - // TODO(splicing): Support async signing - None => { - // Restore the taken state for later error handling - pending_splice.funding_negotiation = - Some(FundingNegotiation::ConstructingTransaction { - funding, - interactive_tx_constructor, - }); - Err(AbortReason::InternalError( - "Failed to compute commitment_signed signatures", - )) - }, - } + pending_splice.funding_negotiation = + Some(FundingNegotiation::AwaitingSignatures { + is_initiator, + funding, + }); + interactive_tx_constructor })? } else { return Err(AbortReason::InternalError( @@ -2158,8 +2210,19 @@ where }; let signing_session = interactive_tx_constructor.into_signing_session(); + let has_local_contribution = signing_session.has_local_contribution(); + let unsigned_funding_tx = signing_session.unsigned_tx().tx().clone(); self.context_mut().interactive_tx_signing_session = Some(signing_session); - Ok(commitment_signed) + + if has_local_contribution { + Ok(Some(unsigned_funding_tx)) + } else { + let txid = unsigned_funding_tx.compute_txid(); + self.funding_transaction_signed(txid, vec![]).map(|_| None).map_err(|err| { + log_warn!(logger, "Failed signing interactive funding transaction: {err:?}"); + AbortReason::InternalError("Failed signing interactive funding transcation") + }) + } } pub fn force_shutdown(&mut self, closure_reason: ClosureReason) -> ShutdownResult { @@ -2914,6 +2977,7 @@ where monitor_pending_channel_ready: bool, monitor_pending_revoke_and_ack: bool, monitor_pending_commitment_signed: bool, + monitor_pending_tx_signatures: bool, // TODO: If a channel is drop'd, we don't know whether the `ChannelMonitor` is ultimately // responsible for some of the HTLCs here or not - we don't know whether the update in question @@ -3645,6 +3709,7 @@ where monitor_pending_channel_ready: false, monitor_pending_revoke_and_ack: false, monitor_pending_commitment_signed: false, + monitor_pending_tx_signatures: false, monitor_pending_forwards: Vec::new(), monitor_pending_failures: Vec::new(), monitor_pending_finalized_fulfills: Vec::new(), @@ -3884,6 +3949,7 @@ where monitor_pending_channel_ready: false, monitor_pending_revoke_and_ack: false, monitor_pending_commitment_signed: false, + monitor_pending_tx_signatures: false, monitor_pending_forwards: Vec::new(), monitor_pending_failures: Vec::new(), monitor_pending_finalized_fulfills: Vec::new(), @@ -6965,6 +7031,43 @@ where shutdown_result } + pub(crate) fn maybe_handle_funding_transaction_signed_post_action( + &mut self, fee_estimator: &LowerBoundedFeeEstimator, logger: &L, + ) -> Result, ChannelError> + where + F::Target: FeeEstimator, + L::Target: Logger, + { + let Some(signing_session) = self.context.interactive_tx_signing_session.as_ref() else { + return Ok(None); + }; + if signing_session.holder_tx_signatures().is_none() { + return Ok(None); + } + + let mut commit_sig = None; + if self.context.signer_pending_funding { + let funding = self + .pending_splice + .as_ref() + .and_then(|pending_splice| pending_splice.funding_negotiation.as_ref()) + .and_then(|funding_negotiation| { + debug_assert!(matches!( + funding_negotiation, + FundingNegotiation::AwaitingSignatures { .. } + )); + funding_negotiation.as_funding() + }) + .unwrap_or(&self.funding); + commit_sig = self.context.get_initial_commitment_signed_v2(funding, logger); + if commit_sig.is_some() { + self.context.signer_pending_funding = false; + } + } + + Ok(commit_sig) + } + fn maybe_fail_splice_negotiation(&mut self) -> Option { if matches!(self.context.channel_state, ChannelState::ChannelReady(_)) { if self.should_reset_pending_splice_state(false) { @@ -8049,6 +8152,7 @@ where Vec::new(), logger, ); + self.context.monitor_pending_tx_signatures = true; Ok(self.push_ret_blockable_mon_update(monitor_update)) } @@ -9079,107 +9183,6 @@ where } } - pub fn funding_transaction_signed( - &mut self, funding_txid_signed: Txid, witnesses: Vec, best_block_height: u32, - logger: &L, - ) -> Result - where - L::Target: Logger, - { - let signing_session = - if let Some(signing_session) = self.context.interactive_tx_signing_session.as_mut() { - if let Some(pending_splice) = self.pending_splice.as_ref() { - debug_assert!(pending_splice - .funding_negotiation - .as_ref() - .map(|funding_negotiation| matches!( - funding_negotiation, - FundingNegotiation::AwaitingSignatures { .. } - )) - .unwrap_or(false)); - } - - if signing_session.holder_tx_signatures().is_some() { - // Our `tx_signatures` either should've been the first time we processed them, - // or we're waiting for our counterparty to send theirs first. - return Ok(FundingTxSigned { - tx_signatures: None, - funding_tx: None, - splice_negotiated: None, - splice_locked: None, - }); - } - - signing_session - } else { - if Some(funding_txid_signed) == self.funding.get_funding_txid() { - // We may be handling a duplicate call and the funding was already locked so we - // no longer have the signing session present. - return Ok(FundingTxSigned { - tx_signatures: None, - funding_tx: None, - splice_negotiated: None, - splice_locked: None, - }); - } - let err = - format!("Channel {} not expecting funding signatures", self.context.channel_id); - return Err(APIError::APIMisuseError { err }); - }; - - let tx = signing_session.unsigned_tx().tx(); - if funding_txid_signed != tx.compute_txid() { - return Err(APIError::APIMisuseError { - err: "Transaction was malleated prior to signing".to_owned(), - }); - } - - let shared_input_signature = - if let Some(splice_input_index) = signing_session.unsigned_tx().shared_input_index() { - let sig = match &self.context.holder_signer { - ChannelSignerType::Ecdsa(signer) => signer.sign_splice_shared_input( - &self.funding.channel_transaction_parameters, - tx, - splice_input_index as usize, - &self.context.secp_ctx, - ), - #[cfg(taproot)] - ChannelSignerType::Taproot(_) => todo!(), - }; - Some(sig) - } else { - None - }; - debug_assert_eq!(self.pending_splice.is_some(), shared_input_signature.is_some()); - - let tx_signatures = msgs::TxSignatures { - channel_id: self.context.channel_id, - tx_hash: funding_txid_signed, - witnesses, - shared_input_signature, - }; - let (tx_signatures, funding_tx) = signing_session - .provide_holder_witnesses(tx_signatures, &self.context.secp_ctx) - .map_err(|err| APIError::APIMisuseError { err })?; - - let logger = WithChannelContext::from(logger, &self.context, None); - if tx_signatures.is_some() { - log_info!( - logger, - "Sending tx_signatures for interactive funding transaction {funding_txid_signed}" - ); - } - - let (splice_negotiated, splice_locked) = if let Some(funding_tx) = funding_tx.clone() { - debug_assert!(tx_signatures.is_some()); - self.on_tx_signatures_exchange(funding_tx, best_block_height, &logger) - } else { - (None, None) - }; - - Ok(FundingTxSigned { tx_signatures, funding_tx, splice_negotiated, splice_locked }) - } - pub fn tx_signatures( &mut self, msg: &msgs::TxSignatures, best_block_height: u32, logger: &L, ) -> Result @@ -9450,6 +9453,25 @@ where self.context.channel_state.clear_monitor_update_in_progress(); assert_eq!(self.blocked_monitor_updates_pending(), 0); + let mut tx_signatures = self + .context + .monitor_pending_tx_signatures + .then(|| ()) + .and_then(|_| self.context.interactive_tx_signing_session.as_ref()) + .and_then(|signing_session| signing_session.holder_tx_signatures().clone()); + if tx_signatures.is_some() { + // We want to clear that the monitor update for our `tx_signatures` has completed, but + // we may still need to hold back the message until it's ready to be sent. + self.context.monitor_pending_tx_signatures = false; + let signing_session = self.context.interactive_tx_signing_session.as_ref() + .expect("We have a tx_signatures message so we must have a valid signing session"); + if !signing_session.holder_sends_tx_signatures_first() + && !signing_session.has_received_tx_signatures() + { + tx_signatures.take(); + } + } + // If we're past (or at) the AwaitingChannelReady stage on an outbound (or V2-established) channel, // try to (re-)broadcast the funding transaction as we may have declined to broadcast it when we // first received the funding_signed. @@ -9538,7 +9560,7 @@ where match commitment_order { RAACommitmentOrder::CommitmentFirst => "commitment", RAACommitmentOrder::RevokeAndACKFirst => "RAA"}); MonitorRestoreUpdates { raa, commitment_update, commitment_order, accepted_htlcs, failed_htlcs, finalized_claimed_htlcs, - pending_update_adds, funding_broadcastable, channel_ready, announcement_sigs, tx_signatures: None, + pending_update_adds, funding_broadcastable, channel_ready, announcement_sigs, tx_signatures, channel_ready_order, } } @@ -10030,6 +10052,10 @@ where // - if the `commitment_signed` bit is set in `retransmit_flags`: if !session.has_received_tx_signatures() && next_funding.should_retransmit(msgs::NextFundingFlag::CommitmentSigned) + // We intentionally hold back our commitment signed until the user decides to + // approve the interactively funded transaction via + // [`Channel::funding_transaction_signed`]. + && session.holder_tx_signatures().is_some() { // - MUST retransmit its `commitment_signed` for that funding transaction. let funding = self @@ -15058,6 +15084,8 @@ where if !self.context.monitor_pending_update_adds.is_empty() { monitor_pending_update_adds = Some(&self.context.monitor_pending_update_adds); } + let monitor_pending_tx_signatures = + self.context.monitor_pending_tx_signatures.then_some(()); let is_manual_broadcast = Some(self.context.is_manual_broadcast); let holder_commitment_point_previous_revoked = @@ -15092,6 +15120,7 @@ where (9, self.context.target_closing_feerate_sats_per_kw, option), (10, monitor_pending_update_adds, option), // Added in 0.0.122 (11, self.context.monitor_pending_finalized_fulfills, required_vec), + (12, monitor_pending_tx_signatures, option), (13, self.context.channel_creation_height, required), (15, preimages, required_vec), (17, self.context.announcement_sigs_state, required), @@ -15500,6 +15529,7 @@ where let mut malformed_htlcs: Option> = None; let mut monitor_pending_update_adds: Option> = None; + let mut monitor_pending_tx_signatures: Option<()> = None; let mut holder_commitment_point_previous_revoked_opt: Option = None; let mut holder_commitment_point_last_revoked_opt: Option = None; @@ -15536,6 +15566,7 @@ where (9, target_closing_feerate_sats_per_kw, option), (10, monitor_pending_update_adds, option), // Added in 0.0.122 (11, monitor_pending_finalized_fulfills, optional_vec), + (12, monitor_pending_tx_signatures, option), (13, channel_creation_height, required), (15, preimages, required_vec), // The preimages transitioned from optional to required in 0.2 (17, announcement_sigs_state, required), @@ -15951,6 +15982,7 @@ where monitor_pending_channel_ready, monitor_pending_revoke_and_ack, monitor_pending_commitment_signed, + monitor_pending_tx_signatures: monitor_pending_tx_signatures.is_some(), monitor_pending_forwards, monitor_pending_failures, monitor_pending_finalized_fulfills: monitor_pending_finalized_fulfills.unwrap(), diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 69a2d2f19a6..5a609bfbd46 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -6611,7 +6611,9 @@ where /// which is an alias of `SIGHASH_ALL`. /// /// Returns [`ChannelUnavailable`] when a channel is not found or an incorrect - /// `counterparty_node_id` is provided. + /// `counterparty_node_id` is provided. Note that this may be expected when a + /// [`FundingTransactionReadyForSigning`] is queued for a dual-funded channel but not yet + /// processed, and a node restart occurs. /// /// Returns [`APIMisuseError`] when a channel is not in a state where it is expecting funding /// signatures or if any of the checks described above fail. @@ -6636,91 +6638,23 @@ where let mut peer_state = peer_state_mutex_opt.unwrap().lock().unwrap(); match peer_state.channel_by_id.get_mut(channel_id) { - Some(channel) => match channel.as_funded_mut() { - Some(chan) => { - let txid = transaction.compute_txid(); - let witnesses: Vec<_> = transaction - .input - .into_iter() - .map(|input| input.witness) - .filter(|witness| !witness.is_empty()) - .collect(); - let best_block_height = self.best_block.read().unwrap().height; - match chan.funding_transaction_signed( - txid, - witnesses, - best_block_height, - &self.logger, - ) { - Ok(FundingTxSigned { - tx_signatures: Some(tx_signatures), - funding_tx, - splice_negotiated, - splice_locked, - }) => { - if let Some(funding_tx) = funding_tx { - self.broadcast_interactive_funding( - chan, - &funding_tx, - &self.logger, - ); - } - if let Some(splice_negotiated) = splice_negotiated { - self.pending_events.lock().unwrap().push_back(( - events::Event::SplicePending { - channel_id: *channel_id, - counterparty_node_id: *counterparty_node_id, - user_channel_id: chan.context.get_user_id(), - new_funding_txo: splice_negotiated.funding_txo, - channel_type: splice_negotiated.channel_type, - new_funding_redeem_script: splice_negotiated - .funding_redeem_script, - }, - None, - )); - } - peer_state.pending_msg_events.push( - MessageSendEvent::SendTxSignatures { - node_id: *counterparty_node_id, - msg: tx_signatures, - }, - ); - if let Some(splice_locked) = splice_locked { - peer_state.pending_msg_events.push( - MessageSendEvent::SendSpliceLocked { - node_id: *counterparty_node_id, - msg: splice_locked, - }, - ); - } - return NotifyOption::DoPersist; - }, - Err(err) => { - result = Err(err); - return NotifyOption::SkipPersistNoEvents; - }, - Ok(FundingTxSigned { - tx_signatures: None, - funding_tx, - splice_negotiated, - splice_locked, - }) => { - debug_assert!(funding_tx.is_none()); - debug_assert!(splice_negotiated.is_none()); - debug_assert!(splice_locked.is_none()); - return NotifyOption::SkipPersistNoEvents; - }, - } - }, - None => { - result = Err(APIError::APIMisuseError { - err: format!( - "Channel with id {} not expecting funding signatures", - channel_id - ), - }); - return NotifyOption::SkipPersistNoEvents; - }, + Some(channel) => { + let txid = transaction.compute_txid(); + let witnesses: Vec<_> = transaction + .input + .into_iter() + .map(|input| input.witness) + .filter(|witness| !witness.is_empty()) + .collect(); + match channel.funding_transaction_signed(txid, witnesses) { + Ok(_) => { + return NotifyOption::DoPersist; + }, + Err(err) => { + result = Err(err); + return NotifyOption::SkipPersistNoEvents; + }, + } }, None => { result = Err(APIError::ChannelUnavailable { @@ -9924,79 +9858,6 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ } } - if let Some(signing_session) = (!channel.is_awaiting_monitor_update()) - .then(|| ()) - .and_then(|_| channel.context.interactive_tx_signing_session.as_mut()) - .filter(|signing_session| signing_session.has_received_commitment_signed()) - .filter(|signing_session| signing_session.holder_tx_signatures().is_none()) - { - if signing_session.has_local_contribution() { - let mut pending_events = self.pending_events.lock().unwrap(); - let unsigned_transaction = signing_session.unsigned_tx().tx().clone(); - let event_action = ( - Event::FundingTransactionReadyForSigning { - unsigned_transaction, - counterparty_node_id, - channel_id: channel.context.channel_id(), - user_channel_id: channel.context.get_user_id(), - }, - None, - ); - - if !pending_events.contains(&event_action) { - pending_events.push_back(event_action); - } - } else { - let txid = signing_session.unsigned_tx().compute_txid(); - let best_block_height = self.best_block.read().unwrap().height; - match channel.funding_transaction_signed(txid, vec![], best_block_height, &self.logger) { - Ok(FundingTxSigned { - tx_signatures: Some(tx_signatures), - funding_tx, - splice_negotiated, - splice_locked, - }) => { - if let Some(funding_tx) = funding_tx { - self.broadcast_interactive_funding(channel, &funding_tx, &self.logger); - } - - if let Some(splice_negotiated) = splice_negotiated { - self.pending_events.lock().unwrap().push_back(( - events::Event::SplicePending { - channel_id: channel.context.channel_id(), - counterparty_node_id, - user_channel_id: channel.context.get_user_id(), - new_funding_txo: splice_negotiated.funding_txo, - channel_type: splice_negotiated.channel_type, - new_funding_redeem_script: splice_negotiated.funding_redeem_script, - }, - None, - )); - } - - if channel.context.is_connected() { - pending_msg_events.push(MessageSendEvent::SendTxSignatures { - node_id: counterparty_node_id, - msg: tx_signatures, - }); - if let Some(splice_locked) = splice_locked { - pending_msg_events.push(MessageSendEvent::SendSpliceLocked { - node_id: counterparty_node_id, - msg: splice_locked, - }); - } - } - }, - Ok(FundingTxSigned { tx_signatures: None, .. }) => { - debug_assert!(false, "If our tx_signatures is empty, then we should send it first!"); - }, - Err(err) => { - log_warn!(logger, "Failed signing interactive funding transaction: {err:?}"); - }, - } - } - } - { let mut pending_events = self.pending_events.lock().unwrap(); emit_channel_pending_event!(pending_events, channel); @@ -10950,8 +10811,8 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ hash_map::Entry::Occupied(mut chan_entry) => { let chan = chan_entry.get_mut(); match chan.tx_complete(msg, &self.logger) { - Ok((interactive_tx_msg_send, commitment_signed)) => { - let persist = if interactive_tx_msg_send.is_some() || commitment_signed.is_some() { + Ok((interactive_tx_msg_send, unsigned_tx_for_signing_event)) => { + let persist = if interactive_tx_msg_send.is_some() { NotifyOption::SkipPersistHandleEvents } else { NotifyOption::SkipPersistNoEvents @@ -10960,19 +10821,17 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ let msg_send_event = interactive_tx_msg_send.into_msg_send_event(counterparty_node_id); peer_state.pending_msg_events.push(msg_send_event); }; - if let Some(commitment_signed) = commitment_signed { - peer_state.pending_msg_events.push(MessageSendEvent::UpdateHTLCs { - node_id: counterparty_node_id, - channel_id: msg.channel_id, - updates: CommitmentUpdate { - commitment_signed: vec![commitment_signed], - update_add_htlcs: vec![], - update_fulfill_htlcs: vec![], - update_fail_htlcs: vec![], - update_fail_malformed_htlcs: vec![], - update_fee: None, + if let Some(unsigned_transaction) = unsigned_tx_for_signing_event { + let mut pending_events = self.pending_events.lock().unwrap(); + pending_events.push_back(( + Event::FundingTransactionReadyForSigning { + unsigned_transaction, + counterparty_node_id, + channel_id: chan.context().channel_id(), + user_channel_id: chan.context().get_user_id(), }, - }); + None, + )); } Ok(persist) }, @@ -12773,6 +12632,67 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ has_update } + fn maybe_handle_funding_transaction_signed_post_action(&self) -> bool { + let mut has_update = false; + let mut handle_errors: Vec<(PublicKey, ChannelId, Result<(), MsgHandleErrInternal>)> = + Vec::new(); + + let per_peer_state = self.per_peer_state.read().unwrap(); + for (counterparty_node_id, peer_state_mutex) in per_peer_state.iter() { + let mut peer_state_lock = peer_state_mutex.lock().unwrap(); + let peer_state = &mut *peer_state_lock; + let pending_msg_events = &mut peer_state.pending_msg_events; + for (channel_id, chan) in &mut peer_state.channel_by_id { + if let Some(funded_chan) = chan.as_funded_mut() { + let logger = WithChannelContext::from(&self.logger, &funded_chan.context, None); + match funded_chan.maybe_handle_funding_transaction_signed_post_action( + &self.fee_estimator, + &&logger, + ) { + Ok(commit_sig) => { + if let Some(commit_sig) = commit_sig { + pending_msg_events.push(MessageSendEvent::UpdateHTLCs { + node_id: *counterparty_node_id, + channel_id: *channel_id, + updates: CommitmentUpdate { + update_add_htlcs: vec![], + update_fulfill_htlcs: vec![], + update_fail_htlcs: vec![], + update_fail_malformed_htlcs: vec![], + update_fee: None, + commitment_signed: vec![commit_sig], + }, + }); + } + }, + Err(e) => { + let (_, err) = self.locked_handle_funded_force_close( + &mut peer_state.closed_channel_monitor_update_ids, + &mut peer_state.in_flight_monitor_updates, + e, + funded_chan, + ); + handle_errors.push((*counterparty_node_id, *channel_id, Err(err))); + has_update = true; + }, + } + } + } + + for (cp_node_id, channel_id, _) in &handle_errors { + if cp_node_id == counterparty_node_id { + peer_state.channel_by_id.remove_entry(channel_id); + } + } + } + + for (counterparty_node_id, _, err) in handle_errors { + let _ = self.handle_error(err, counterparty_node_id); + } + + has_update + } + #[rustfmt::skip] fn maybe_send_stfu(&self) { let per_peer_state = self.per_peer_state.read().unwrap(); @@ -14381,6 +14301,10 @@ where // Quiescence is an in-memory protocol, so we don't have to persist because of it. self.maybe_send_stfu(); + if self.maybe_handle_funding_transaction_signed_post_action() { + result = NotifyOption::DoPersist; + } + let mut is_any_peer_connected = false; let mut pending_events = Vec::new(); let per_peer_state = self.per_peer_state.read().unwrap(); diff --git a/lightning/src/ln/interactivetxs.rs b/lightning/src/ln/interactivetxs.rs index 4340aad420a..32ad4e50f10 100644 --- a/lightning/src/ln/interactivetxs.rs +++ b/lightning/src/ln/interactivetxs.rs @@ -649,7 +649,7 @@ impl InteractiveTxSigningSession { /// unsigned transaction. pub fn provide_holder_witnesses( &mut self, tx_signatures: TxSignatures, secp_ctx: &Secp256k1, - ) -> Result<(Option, Option), String> { + ) -> Result<(), String> { if self.holder_tx_signatures.is_some() { return Err("Holder witnesses were already provided".to_string()); } @@ -667,15 +667,7 @@ impl InteractiveTxSigningSession { self.holder_tx_signatures = Some(tx_signatures); - let funding_tx_opt = self.maybe_finalize_funding_tx(); - let holder_tx_signatures = (self.holder_sends_tx_signatures_first - || self.has_received_tx_signatures()) - .then(|| { - debug_assert!(self.has_received_commitment_signed); - self.holder_tx_signatures.clone().expect("Holder tx_signatures were just provided") - }); - - Ok((holder_tx_signatures, funding_tx_opt)) + Ok(()) } pub fn remote_inputs_count(&self) -> usize { diff --git a/lightning/src/ln/splicing_tests.rs b/lightning/src/ln/splicing_tests.rs index a05c0bd92d8..66d9e748583 100644 --- a/lightning/src/ln/splicing_tests.rs +++ b/lightning/src/ln/splicing_tests.rs @@ -27,7 +27,6 @@ use crate::ln::types::ChannelId; use crate::routing::router::{PaymentParameters, RouteParameters}; use crate::util::errors::APIError; use crate::util::ser::Writeable; -use crate::util::test_channel_signer::SignerOp; use bitcoin::secp256k1::PublicKey; use bitcoin::{Amount, OutPoint as BitcoinOutPoint, ScriptBuf, Transaction, TxOut}; @@ -138,7 +137,7 @@ fn test_v1_splice_in_negative_insufficient_inputs() { pub fn negotiate_splice_tx<'a, 'b, 'c, 'd>( initiator: &'a Node<'b, 'c, 'd>, acceptor: &'a Node<'b, 'c, 'd>, channel_id: ChannelId, initiator_contribution: SpliceContribution, -) -> msgs::CommitmentSigned { +) { let new_funding_script = complete_splice_handshake(initiator, acceptor, channel_id, initiator_contribution.clone()); complete_interactive_funding_negotiation( @@ -190,7 +189,7 @@ pub fn complete_splice_handshake<'a, 'b, 'c, 'd>( pub fn complete_interactive_funding_negotiation<'a, 'b, 'c, 'd>( initiator: &'a Node<'b, 'c, 'd>, acceptor: &'a Node<'b, 'c, 'd>, channel_id: ChannelId, initiator_contribution: SpliceContribution, new_funding_script: ScriptBuf, -) -> msgs::CommitmentSigned { +) { let node_id_initiator = initiator.node.get_our_node_id(); let node_id_acceptor = acceptor.node.get_our_node_id(); @@ -247,21 +246,16 @@ pub fn complete_interactive_funding_negotiation<'a, 'b, 'c, 'd>( acceptor.node.handle_tx_add_output(node_id_initiator, &tx_add_output); } else { let mut msg_events = initiator.node.get_and_clear_pending_msg_events(); - assert_eq!( - msg_events.len(), - if acceptor_sent_tx_complete { 2 } else { 1 }, - "{msg_events:?}" - ); + assert_eq!(msg_events.len(), 1, "{msg_events:?}"); if let MessageSendEvent::SendTxComplete { ref msg, .. } = msg_events.remove(0) { acceptor.node.handle_tx_complete(node_id_initiator, msg); } else { panic!(); } if acceptor_sent_tx_complete { - if let MessageSendEvent::UpdateHTLCs { mut updates, .. } = msg_events.remove(0) { - return updates.commitment_signed.remove(0); - } - panic!(); + assert!(expected_initiator_inputs.is_empty()); + assert!(expected_initiator_scripts.is_empty()); + return; } } @@ -277,28 +271,21 @@ pub fn complete_interactive_funding_negotiation<'a, 'b, 'c, 'd>( } pub fn sign_interactive_funding_tx<'a, 'b, 'c, 'd>( - initiator: &'a Node<'b, 'c, 'd>, acceptor: &'a Node<'b, 'c, 'd>, - initial_commit_sig_for_acceptor: msgs::CommitmentSigned, is_0conf: bool, + initiator: &'a Node<'b, 'c, 'd>, acceptor: &'a Node<'b, 'c, 'd>, is_0conf: bool, ) -> (Transaction, Option<(msgs::SpliceLocked, PublicKey)>) { let node_id_initiator = initiator.node.get_our_node_id(); let node_id_acceptor = acceptor.node.get_our_node_id(); assert!(initiator.node.get_and_clear_pending_msg_events().is_empty()); - acceptor.node.handle_commitment_signed(node_id_initiator, &initial_commit_sig_for_acceptor); let msg_events = acceptor.node.get_and_clear_pending_msg_events(); - assert_eq!(msg_events.len(), 2, "{msg_events:?}"); + assert_eq!(msg_events.len(), 1, "{msg_events:?}"); if let MessageSendEvent::UpdateHTLCs { ref updates, .. } = &msg_events[0] { let commitment_signed = &updates.commitment_signed[0]; initiator.node.handle_commitment_signed(node_id_acceptor, commitment_signed); } else { panic!(); } - if let MessageSendEvent::SendTxSignatures { ref msg, .. } = &msg_events[1] { - initiator.node.handle_tx_signatures(node_id_acceptor, msg); - } else { - panic!(); - } let event = get_event!(initiator, Event::FundingTransactionReadyForSigning); if let Event::FundingTransactionReadyForSigning { @@ -314,6 +301,23 @@ pub fn sign_interactive_funding_tx<'a, 'b, 'c, 'd>( .funding_transaction_signed(&channel_id, &counterparty_node_id, partially_signed_tx) .unwrap(); } + + let msg_events = initiator.node.get_and_clear_pending_msg_events(); + assert_eq!(msg_events.len(), 1, "{msg_events:?}"); + if let MessageSendEvent::UpdateHTLCs { ref updates, .. } = &msg_events[0] { + acceptor.node.handle_commitment_signed(node_id_initiator, &updates.commitment_signed[0]); + } else { + panic!(); + } + + let msg_events = acceptor.node.get_and_clear_pending_msg_events(); + assert_eq!(msg_events.len(), 1, "{msg_events:?}"); + if let MessageSendEvent::SendTxSignatures { ref msg, .. } = &msg_events[0] { + initiator.node.handle_tx_signatures(node_id_acceptor, msg); + } else { + panic!(); + } + let mut msg_events = initiator.node.get_and_clear_pending_msg_events(); assert_eq!(msg_events.len(), if is_0conf { 2 } else { 1 }, "{msg_events:?}"); if let MessageSendEvent::SendTxSignatures { ref msg, .. } = &msg_events[0] { @@ -338,7 +342,7 @@ pub fn sign_interactive_funding_tx<'a, 'b, 'c, 'd>( let mut initiator_txn = initiator.tx_broadcaster.txn_broadcast(); assert_eq!(initiator_txn.len(), 1); let acceptor_txn = acceptor.tx_broadcaster.txn_broadcast(); - assert_eq!(initiator_txn, acceptor_txn,); + assert_eq!(initiator_txn, acceptor_txn); initiator_txn.remove(0) }; (tx, splice_locked) @@ -354,15 +358,14 @@ pub fn splice_channel<'a, 'b, 'c, 'd>( let new_funding_script = complete_splice_handshake(initiator, acceptor, channel_id, initiator_contribution.clone()); - let initial_commit_sig_for_acceptor = complete_interactive_funding_negotiation( + complete_interactive_funding_negotiation( initiator, acceptor, channel_id, initiator_contribution, new_funding_script, ); - let (splice_tx, splice_locked) = - sign_interactive_funding_tx(initiator, acceptor, initial_commit_sig_for_acceptor, false); + let (splice_tx, splice_locked) = sign_interactive_funding_tx(initiator, acceptor, false); assert!(splice_locked.is_none()); expect_splice_pending_event(initiator, &node_id_acceptor); @@ -650,15 +653,12 @@ fn do_test_splice_state_reset_on_disconnect(reload: bool) { nodes[0].node.handle_tx_complete(node_id_1, &tx_complete); let msg_events = nodes[0].node.get_and_clear_pending_msg_events(); - assert_eq!(msg_events.len(), 2); + assert_eq!(msg_events.len(), 1); if let MessageSendEvent::SendTxComplete { .. } = &msg_events[0] { } else { panic!("Unexpected event"); } - if let MessageSendEvent::UpdateHTLCs { .. } = &msg_events[1] { - } else { - panic!("Unexpected event"); - } + let _ = get_event!(nodes[0], Event::FundingTransactionReadyForSigning); if reload { let encoded_monitor_0 = get_monitor!(nodes[0], channel_id).encode(); @@ -1129,9 +1129,14 @@ fn do_test_splice_reestablish(reload: bool, async_monitor_update: bool) { }, ], }; - let initial_commit_sig_for_acceptor = - negotiate_splice_tx(&nodes[0], &nodes[1], channel_id, initiator_contribution); - assert_eq!(initial_commit_sig_for_acceptor.htlc_signatures.len(), 1); + negotiate_splice_tx(&nodes[0], &nodes[1], channel_id, initiator_contribution); + + // Node 0 should have a signing event to handle since they had a contribution in the splice. + // Node 1 won't and will immediately send their `commitment_signed`. + let signing_event = get_event!(nodes[0], Event::FundingTransactionReadyForSigning); + assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); + + assert!(nodes[1].node.get_and_clear_pending_events().is_empty()); let initial_commit_sig_for_initiator = get_htlc_update_msgs(&nodes[1], &node_id_0); assert_eq!(initial_commit_sig_for_initiator.commitment_signed.len(), 1); assert_eq!(initial_commit_sig_for_initiator.commitment_signed[0].htlc_signatures.len(), 1); @@ -1146,8 +1151,8 @@ fn do_test_splice_reestablish(reload: bool, async_monitor_update: bool) { }; } - // Reestablishing now should force both nodes to retransmit their initial `commitment_signed` - // message as they were never delivered. + // Reestablishing now should force node 1 to retransmit their initial `commitment_signed` + // message as it was never delivered. if reload { let encoded_monitor_0 = get_monitor!(nodes[0], channel_id).encode(); reload_node!( @@ -1181,12 +1186,30 @@ fn do_test_splice_reestablish(reload: bool, async_monitor_update: bool) { } let mut reconnect_args = ReconnectArgs::new(&nodes[0], &nodes[1]); - reconnect_args.send_interactive_tx_commit_sig = (true, true); + reconnect_args.send_interactive_tx_commit_sig = (true, false); reconnect_nodes(reconnect_args); - // The `commitment_signed` messages were delivered in the reestablishment, so we should expect - // to see a `RenegotiatedFunding` monitor update on both nodes. + // The `commitment_signed` message was only delivered to nodes[0] in the reestablishment, so we + // should expect to see a `RenegotiatedFunding` monitor update, but it won't actually be + // processed until nodes[0] approves the splice via `funding_transaction_signed`. + check_added_monitors(&nodes[0], 0); + check_added_monitors(&nodes[1], 0); + + // Have node 0 sign, we should see its `commitment_signed` go out and the `RenegotiatedFunding` + // monitor update processed. + if let Event::FundingTransactionReadyForSigning { unsigned_transaction, .. } = signing_event { + let tx = nodes[0].wallet_source.sign_tx(unsigned_transaction).unwrap(); + nodes[0].node.funding_transaction_signed(&channel_id, &node_id_1, tx).unwrap(); + } + let _ = get_htlc_update_msgs(&nodes[0], &node_id_1); check_added_monitors(&nodes[0], 1); + + // Reconnect to make sure node 0 retransmits its `commitment_signed` as it was never delivered. + reconnect_nodes!(|reconnect_args: &mut ReconnectArgs| { + reconnect_args.send_interactive_tx_commit_sig = (false, true); + }); + + check_added_monitors(&nodes[0], 0); check_added_monitors(&nodes[1], 1); if async_monitor_update { @@ -1203,38 +1226,22 @@ fn do_test_splice_reestablish(reload: bool, async_monitor_update: bool) { chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::Completed); } - // Node 0 should have a signing event to handle since they had a contribution in the splice. - // Node 1 won't and will immediately send `tx_signatures`. - let _ = get_event!(nodes[0], Event::FundingTransactionReadyForSigning); - assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); - assert!(nodes[1].node.get_and_clear_pending_events().is_empty()); - let _ = get_event_msg!(nodes[1], MessageSendEvent::SendTxSignatures, node_id_0); + // Node 1 should now send its `tx_signatures` as it goes first. Node 0 will wait to send theirs + // until they receive it. + assert!(nodes[0].node.get_and_clear_pending_events().is_empty()); + let _ = get_event_msg!(&nodes[1], MessageSendEvent::SendTxSignatures, node_id_0); - // Reconnecting now should force node 1 to retransmit their `tx_signatures` since it was never - // delivered. Node 0 still hasn't called back with `funding_transaction_signed`, so its - // `tx_signatures` is not ready yet. + // Reconnect to make sure node 1 retransmits its `tx_signatures` as it was never delivered. reconnect_nodes!(|reconnect_args: &mut ReconnectArgs| { reconnect_args.send_interactive_tx_sigs = (true, false); }); - let _ = get_event!(nodes[0], Event::FundingTransactionReadyForSigning); - - // Reconnect again to make sure node 1 doesn't retransmit `tx_signatures` unnecessarily as it - // was delivered in the previous reestablishment. - reconnect_nodes!(|_| {}); - - // Have node 0 sign, we should see its `tx_signatures` go out. - let event = get_event!(nodes[0], Event::FundingTransactionReadyForSigning); - if let Event::FundingTransactionReadyForSigning { unsigned_transaction, .. } = event { - let tx = nodes[0].wallet_source.sign_tx(unsigned_transaction).unwrap(); - nodes[0].node.funding_transaction_signed(&channel_id, &node_id_1, tx).unwrap(); - } - let _ = get_event_msg!(nodes[0], MessageSendEvent::SendTxSignatures, node_id_1); - expect_splice_pending_event(&nodes[0], &node_id_1); + let _ = get_event_msg!(&nodes[0], MessageSendEvent::SendTxSignatures, node_id_1); // Reconnect to make sure node 0 retransmits its `tx_signatures` as it was never delivered. reconnect_nodes!(|reconnect_args: &mut ReconnectArgs| { reconnect_args.send_interactive_tx_sigs = (false, true); }); + expect_splice_pending_event(&nodes[0], &node_id_1); expect_splice_pending_event(&nodes[1], &node_id_0); // Reestablish the channel again to make sure node 0 doesn't retransmit `tx_signatures` @@ -1482,25 +1489,22 @@ fn do_test_propose_splice_while_disconnected(reload: bool, use_0conf: bool) { .unwrap(); // Negotiate the first splice to completion. - let initial_commit_sig = { - nodes[1].node.handle_splice_init(node_id_0, &splice_init); - let splice_ack = get_event_msg!(nodes[1], MessageSendEvent::SendSpliceAck, node_id_0); - nodes[0].node.handle_splice_ack(node_id_1, &splice_ack); - let new_funding_script = chan_utils::make_funding_redeemscript( - &splice_init.funding_pubkey, - &splice_ack.funding_pubkey, - ) - .to_p2wsh(); - complete_interactive_funding_negotiation( - &nodes[0], - &nodes[1], - channel_id, - node_0_contribution, - new_funding_script, - ) - }; - let (splice_tx, splice_locked) = - sign_interactive_funding_tx(&nodes[0], &nodes[1], initial_commit_sig, use_0conf); + nodes[1].node.handle_splice_init(node_id_0, &splice_init); + let splice_ack = get_event_msg!(nodes[1], MessageSendEvent::SendSpliceAck, node_id_0); + nodes[0].node.handle_splice_ack(node_id_1, &splice_ack); + let new_funding_script = chan_utils::make_funding_redeemscript( + &splice_init.funding_pubkey, + &splice_ack.funding_pubkey, + ) + .to_p2wsh(); + complete_interactive_funding_negotiation( + &nodes[0], + &nodes[1], + channel_id, + node_0_contribution, + new_funding_script, + ); + let (splice_tx, splice_locked) = sign_interactive_funding_tx(&nodes[0], &nodes[1], use_0conf); expect_splice_pending_event(&nodes[0], &node_id_1); expect_splice_pending_event(&nodes[1], &node_id_0); @@ -1624,25 +1628,22 @@ fn do_test_propose_splice_while_disconnected(reload: bool, use_0conf: bool) { } let splice_init = get_event_msg!(nodes[1], MessageSendEvent::SendSpliceInit, node_id_0); - let initial_commit_sig = { - nodes[0].node.handle_splice_init(node_id_1, &splice_init); - let splice_ack = get_event_msg!(nodes[0], MessageSendEvent::SendSpliceAck, node_id_1); - nodes[1].node.handle_splice_ack(node_id_0, &splice_ack); - let new_funding_script = chan_utils::make_funding_redeemscript( - &splice_init.funding_pubkey, - &splice_ack.funding_pubkey, - ) - .to_p2wsh(); - complete_interactive_funding_negotiation( - &nodes[1], - &nodes[0], - channel_id, - node_1_contribution, - new_funding_script, - ) - }; - let (splice_tx, splice_locked) = - sign_interactive_funding_tx(&nodes[1], &nodes[0], initial_commit_sig, use_0conf); + nodes[0].node.handle_splice_init(node_id_1, &splice_init); + let splice_ack = get_event_msg!(nodes[0], MessageSendEvent::SendSpliceAck, node_id_1); + nodes[1].node.handle_splice_ack(node_id_0, &splice_ack); + let new_funding_script = chan_utils::make_funding_redeemscript( + &splice_init.funding_pubkey, + &splice_ack.funding_pubkey, + ) + .to_p2wsh(); + complete_interactive_funding_negotiation( + &nodes[1], + &nodes[0], + channel_id, + node_1_contribution, + new_funding_script, + ); + let (splice_tx, splice_locked) = sign_interactive_funding_tx(&nodes[1], &nodes[0], use_0conf); expect_splice_pending_event(&nodes[0], &node_id_1); expect_splice_pending_event(&nodes[1], &node_id_0); @@ -1689,8 +1690,8 @@ fn disconnect_on_unexpected_interactive_tx_message() { // Complete interactive-tx construction, but fail by having the acceptor send a duplicate // tx_complete instead of commitment_signed. - let _ = negotiate_splice_tx(initiator, acceptor, channel_id, contribution.clone()); - + negotiate_splice_tx(initiator, acceptor, channel_id, contribution.clone()); + let _ = get_event!(initiator, Event::FundingTransactionReadyForSigning); let mut msg_events = acceptor.node.get_and_clear_pending_msg_events(); assert_eq!(msg_events.len(), 1); assert!(matches!(msg_events.remove(0), MessageSendEvent::UpdateHTLCs { .. })); @@ -1753,58 +1754,6 @@ fn fail_splice_on_interactive_tx_error() { let tx_abort = get_event_msg!(acceptor, MessageSendEvent::SendTxAbort, node_id_initiator); initiator.node.handle_tx_abort(node_id_acceptor, &tx_abort); - - // Fail signing the commitment transaction, which prevents the initiator from sending - // tx_complete. - initiator.disable_channel_signer_op( - &node_id_acceptor, - &channel_id, - SignerOp::SignCounterpartyCommitment, - ); - let _ = complete_splice_handshake(initiator, acceptor, channel_id, contribution.clone()); - - let tx_add_input = - get_event_msg!(initiator, MessageSendEvent::SendTxAddInput, node_id_acceptor); - acceptor.node.handle_tx_add_input(node_id_initiator, &tx_add_input); - - let tx_complete = get_event_msg!(acceptor, MessageSendEvent::SendTxComplete, node_id_initiator); - initiator.node.handle_tx_complete(node_id_acceptor, &tx_complete); - - let tx_add_input = - get_event_msg!(initiator, MessageSendEvent::SendTxAddInput, node_id_acceptor); - acceptor.node.handle_tx_add_input(node_id_initiator, &tx_add_input); - - let tx_complete = get_event_msg!(acceptor, MessageSendEvent::SendTxComplete, node_id_initiator); - initiator.node.handle_tx_complete(node_id_acceptor, &tx_complete); - - let tx_add_output = - get_event_msg!(initiator, MessageSendEvent::SendTxAddOutput, node_id_acceptor); - acceptor.node.handle_tx_add_output(node_id_initiator, &tx_add_output); - - let tx_complete = get_event_msg!(acceptor, MessageSendEvent::SendTxComplete, node_id_initiator); - initiator.node.handle_tx_complete(node_id_acceptor, &tx_complete); - - let tx_add_output = - get_event_msg!(initiator, MessageSendEvent::SendTxAddOutput, node_id_acceptor); - acceptor.node.handle_tx_add_output(node_id_initiator, &tx_add_output); - - let tx_complete = get_event_msg!(acceptor, MessageSendEvent::SendTxComplete, node_id_initiator); - initiator.node.handle_tx_complete(node_id_acceptor, &tx_complete); - - let event = get_event!(initiator, Event::SpliceFailed); - match event { - Event::SpliceFailed { contributed_inputs, .. } => { - assert_eq!(contributed_inputs.len(), 1); - assert_eq!(contributed_inputs[0], contribution.inputs()[0].outpoint()); - }, - _ => panic!("Expected Event::SpliceFailed"), - } - - let tx_abort = get_event_msg!(initiator, MessageSendEvent::SendTxAbort, node_id_acceptor); - acceptor.node.handle_tx_abort(node_id_initiator, &tx_abort); - - let tx_abort = get_event_msg!(acceptor, MessageSendEvent::SendTxAbort, node_id_initiator); - initiator.node.handle_tx_abort(node_id_acceptor, &tx_abort); } #[test] From e502ce3bfcca424fbaaaa1e02a992df71481dcf2 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Wed, 14 Jan 2026 14:20:43 -0800 Subject: [PATCH 2/2] Delay processing splice initial commitment signed from counterparty We delay processing it until the user manually approves the splice via `Channel::funding_transaction_signed`, as otherwise, there would be a [`ChannelMonitorUpdateStep::RenegotiatedFunding`] committed that we would need to undo if they no longer wish to proceed. Note that this doesn't need to be done with dual-funded channels as there is no equivalent monitor update for them. --- lightning/src/ln/channel.rs | 70 ++++++++++++++++-- lightning/src/ln/channelmanager.rs | 110 +++++++++++++++++------------ 2 files changed, 128 insertions(+), 52 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 54a650c365d..331c35fa198 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -2192,6 +2192,7 @@ where Some(FundingNegotiation::AwaitingSignatures { is_initiator, funding, + initial_commit_sig_from_counterparty: None, }); interactive_tx_constructor })? @@ -2277,6 +2278,12 @@ where }) .map(|funding_negotiation| funding_negotiation.as_funding().is_some()) .unwrap_or(false); + let has_holder_tx_signatures = funded_channel + .context + .interactive_tx_signing_session + .as_ref() + .map(|session| session.holder_tx_signatures().is_some()) + .unwrap_or(false); let session_received_commitment_signed = funded_channel .context .interactive_tx_signing_session @@ -2286,9 +2293,26 @@ where // which must always come after the initial commitment signed is sent. .unwrap_or(true); let res = if has_negotiated_pending_splice && !session_received_commitment_signed { - funded_channel - .splice_initial_commitment_signed(msg, fee_estimator, logger) - .map(|monitor_update_opt| (None, monitor_update_opt)) + // We delay processing this until the user manually approves the splice via + // [`Channel::funding_transaction_signed`], as otherwise, there would be a + // [`ChannelMonitorUpdateStep::RenegotiatedFunding`] committed that we would + // need to undo if they no longer wish to proceed. + if has_holder_tx_signatures { + funded_channel + .splice_initial_commitment_signed(msg, fee_estimator, logger) + .map(|monitor_update_opt| (None, monitor_update_opt)) + } else { + let pending_splice = funded_channel.pending_splice.as_mut() + .expect("We have a pending splice negotiated"); + let funding_negotiation = pending_splice.funding_negotiation.as_mut() + .expect("We have a pending splice negotiated"); + if let FundingNegotiation::AwaitingSignatures { + ref mut initial_commit_sig_from_counterparty, .. + } = funding_negotiation { + *initial_commit_sig_from_counterparty = Some(msg.clone()); + } + Ok((None, None)) + } } else { funded_channel.commitment_signed(msg, fee_estimator, logger) .map(|monitor_update_opt| (None, monitor_update_opt)) @@ -2772,6 +2796,17 @@ enum FundingNegotiation { AwaitingSignatures { funding: FundingScope, is_initiator: bool, + /// The initial `commitment_signed` message received for the [`FundingScope`] above. We + /// delay processing this until the user manually approves the splice via + /// [`Channel::funding_transaction_signed`], as otherwise, there would be a + /// [`ChannelMonitorUpdateStep::RenegotiatedFunding`] committed that we would need to undo + /// if they no longer wish to proceed. + /// + /// Note that this doesn't need to be done with dual-funded channels as there is no + /// equivalent monitor update for them. + /// + /// This field is not persisted as the message should be resent on reconnections. + initial_commit_sig_from_counterparty: Option, }, } @@ -2779,6 +2814,7 @@ impl_writeable_tlv_based_enum_upgradable!(FundingNegotiation, (0, AwaitingSignatures) => { (1, funding, required), (3, is_initiator, required), + (_unused, initial_commit_sig_from_counterparty, (static_value, None)), }, unread_variants: AwaitingAck, ConstructingTransaction ); @@ -7033,16 +7069,16 @@ where pub(crate) fn maybe_handle_funding_transaction_signed_post_action( &mut self, fee_estimator: &LowerBoundedFeeEstimator, logger: &L, - ) -> Result, ChannelError> + ) -> Result<(Option, Option), ChannelError> where F::Target: FeeEstimator, L::Target: Logger, { let Some(signing_session) = self.context.interactive_tx_signing_session.as_ref() else { - return Ok(None); + return Ok((None, None)); }; if signing_session.holder_tx_signatures().is_none() { - return Ok(None); + return Ok((None, None)); } let mut commit_sig = None; @@ -7065,7 +7101,27 @@ where } } - Ok(commit_sig) + let mut monitor_update = None; + if let Some(initial_commit_sig) = self + .pending_splice + .as_mut() + .and_then(|pending_splice| pending_splice.funding_negotiation.as_mut()) + .and_then(|funding_negotiation| { + if let FundingNegotiation::AwaitingSignatures { + ref mut initial_commit_sig_from_counterparty, + .. + } = funding_negotiation + { + initial_commit_sig_from_counterparty.take() + } else { + None + } + }) { + monitor_update = + self.splice_initial_commitment_signed(&initial_commit_sig, fee_estimator, logger)?; + } + + Ok((commit_sig, monitor_update)) } fn maybe_fail_splice_negotiation(&mut self) -> Option { diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 5a609bfbd46..581a94ce957 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -12637,57 +12637,77 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ let mut handle_errors: Vec<(PublicKey, ChannelId, Result<(), MsgHandleErrInternal>)> = Vec::new(); - let per_peer_state = self.per_peer_state.read().unwrap(); - for (counterparty_node_id, peer_state_mutex) in per_peer_state.iter() { - let mut peer_state_lock = peer_state_mutex.lock().unwrap(); - let peer_state = &mut *peer_state_lock; - let pending_msg_events = &mut peer_state.pending_msg_events; - for (channel_id, chan) in &mut peer_state.channel_by_id { - if let Some(funded_chan) = chan.as_funded_mut() { - let logger = WithChannelContext::from(&self.logger, &funded_chan.context, None); - match funded_chan.maybe_handle_funding_transaction_signed_post_action( - &self.fee_estimator, - &&logger, - ) { - Ok(commit_sig) => { - if let Some(commit_sig) = commit_sig { - pending_msg_events.push(MessageSendEvent::UpdateHTLCs { - node_id: *counterparty_node_id, - channel_id: *channel_id, - updates: CommitmentUpdate { - update_add_htlcs: vec![], - update_fulfill_htlcs: vec![], - update_fail_htlcs: vec![], - update_fail_malformed_htlcs: vec![], - update_fee: None, - commitment_signed: vec![commit_sig], - }, - }); - } - }, - Err(e) => { - let (_, err) = self.locked_handle_funded_force_close( - &mut peer_state.closed_channel_monitor_update_ids, - &mut peer_state.in_flight_monitor_updates, - e, - funded_chan, - ); - handle_errors.push((*counterparty_node_id, *channel_id, Err(err))); - has_update = true; - }, + // Handling a monitor update requires dropping peer state locks, so we use the labeled loop + // as a goto statement. + 'peer_loop: loop { + let per_peer_state = self.per_peer_state.read().unwrap(); + for (counterparty_node_id, peer_state_mutex) in per_peer_state.iter() { + let mut peer_state_lock = peer_state_mutex.lock().unwrap(); + let peer_state = &mut *peer_state_lock; + let pending_msg_events = &mut peer_state.pending_msg_events; + for (channel_id, chan) in &mut peer_state.channel_by_id { + if let Some(funded_chan) = chan.as_funded_mut() { + let logger = + WithChannelContext::from(&self.logger, &funded_chan.context, None); + match funded_chan.maybe_handle_funding_transaction_signed_post_action( + &self.fee_estimator, + &&logger, + ) { + Ok((commit_sig, monitor_update)) => { + if let Some(commit_sig) = commit_sig { + pending_msg_events.push(MessageSendEvent::UpdateHTLCs { + node_id: *counterparty_node_id, + channel_id: *channel_id, + updates: CommitmentUpdate { + update_add_htlcs: vec![], + update_fulfill_htlcs: vec![], + update_fail_htlcs: vec![], + update_fail_malformed_htlcs: vec![], + update_fee: None, + commitment_signed: vec![commit_sig], + }, + }); + } + + if let Some(monitor_update) = monitor_update { + handle_new_monitor_update!( + self, + funded_chan.funding_outpoint(), + monitor_update, + peer_state_lock, + peer_state, + per_peer_state, + funded_chan + ); + has_update = true; + continue 'peer_loop; + } + }, + Err(e) => { + let (_, err) = self.locked_handle_funded_force_close( + &mut peer_state.closed_channel_monitor_update_ids, + &mut peer_state.in_flight_monitor_updates, + e, + funded_chan, + ); + handle_errors.push((*counterparty_node_id, *channel_id, Err(err))); + has_update = true; + }, + } } } - } - for (cp_node_id, channel_id, _) in &handle_errors { - if cp_node_id == counterparty_node_id { - peer_state.channel_by_id.remove_entry(channel_id); + for (cp_node_id, channel_id, _) in &handle_errors { + if cp_node_id == counterparty_node_id { + peer_state.channel_by_id.remove_entry(channel_id); + } } } - } - for (counterparty_node_id, _, err) in handle_errors { - let _ = self.handle_error(err, counterparty_node_id); + for (counterparty_node_id, _, err) in handle_errors { + let _ = self.handle_error(err, counterparty_node_id); + } + break; } has_update