From 83b2d3ec70c36938209566ee7086674b531c9a7b Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Thu, 22 Jan 2026 09:49:53 -0800 Subject: [PATCH 1/3] 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/ln/channel.rs | 207 +++++++++------- lightning/src/ln/channelmanager.rs | 242 ++++++++++--------- lightning/src/ln/functional_test_utils.rs | 2 +- lightning/src/ln/interactivetxs.rs | 5 +- lightning/src/ln/splicing_tests.rs | 278 +++++++++------------- pending_changelog/4336.txt | 5 + 6 files changed, 365 insertions(+), 374 deletions(-) create mode 100644 pending_changelog/4336.txt diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index fd780da8d91..23edf616dae 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1904,10 +1904,7 @@ where pub fn tx_complete( &mut self, msg: &msgs::TxComplete, logger: &L, - ) -> Result< - (Option, Option), - (ChannelError, Option), - > + ) -> Result)> where L::Target: Logger, { @@ -1934,13 +1931,38 @@ where let funding_outpoint = if let Some(funding_outpoint) = negotiation_complete { funding_outpoint } else { - return Ok((interactive_tx_msg_send, None)); + return Ok(TxCompleteResult { + interactive_tx_msg_send, + event_unsigned_tx: None, + funding_tx_signed: None, + }); }; - let commitment_signed = self - .funding_tx_constructed(funding_outpoint, logger) + self.funding_tx_constructed(funding_outpoint) .map_err(|abort_reason| self.fail_interactive_tx_negotiation(abort_reason, logger))?; - Ok((interactive_tx_msg_send, Some(commitment_signed))) + + let signing_session = self + .context() + .interactive_tx_signing_session + .as_ref() + .expect("The signing session must have been initialized in funding_tx_constructed"); + let has_local_contribution = signing_session.has_local_contribution(); + + let event_unsigned_tx = + has_local_contribution.then(|| signing_session.unsigned_tx().tx().clone()); + + let funding_tx_signed = if !has_local_contribution { + let funding_txid = signing_session.unsigned_tx().tx().compute_txid(); + if let ChannelPhase::Funded(chan) = &mut self.phase { + chan.funding_transaction_signed(funding_txid, vec![], 0, logger).ok() + } else { + None + } + } else { + None + }; + + Ok(TxCompleteResult { interactive_tx_msg_send, event_unsigned_tx, funding_tx_signed }) } pub fn tx_abort( @@ -2046,14 +2068,8 @@ where result.map(|monitor| (self.as_funded_mut().expect("Channel should be funded"), monitor)) } - fn funding_tx_constructed( - &mut self, funding_outpoint: OutPoint, logger: &L, - ) -> Result - where - L::Target: Logger, - { - let logger = WithChannelContext::from(logger, self.context(), None); - let (interactive_tx_constructor, commitment_signed) = match &mut self.phase { + fn funding_tx_constructed(&mut self, funding_outpoint: OutPoint) -> Result<(), AbortReason> { + let interactive_tx_constructor = match &mut self.phase { ChannelPhase::UnfundedV2(chan) => { debug_assert_eq!( chan.context.channel_state, @@ -2072,77 +2088,31 @@ where chan.funding.channel_transaction_parameters.funding_outpoint = Some(funding_outpoint); - let interactive_tx_constructor = chan - .interactive_tx_constructor + chan.interactive_tx_constructor .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) + .expect("PendingV2Channel::interactive_tx_constructor should be set") }, ChannelPhase::Funded(chan) => { if let Some(pending_splice) = chan.pending_splice.as_mut() { - pending_splice - .funding_negotiation - .take() - .and_then(|funding_negotiation| { - if let FundingNegotiation::ConstructingTransaction { - funding, - interactive_tx_constructor, - } = funding_negotiation - { - let is_initiator = interactive_tx_constructor.is_initiator(); - Some((is_initiator, funding, interactive_tx_constructor)) - } else { - // Replace the taken state for later error handling - pending_splice.funding_negotiation = Some(funding_negotiation); - None - } - }) - .ok_or_else(|| { - AbortReason::InternalError( - "Got a tx_complete message in an invalid state", - ) - }) - .and_then(|(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", - )) - }, - } - })? + let funding_negotiation = pending_splice.funding_negotiation.take(); + if let Some(FundingNegotiation::ConstructingTransaction { + mut funding, + interactive_tx_constructor, + }) = funding_negotiation + { + let is_initiator = interactive_tx_constructor.is_initiator(); + funding.channel_transaction_parameters.funding_outpoint = + Some(funding_outpoint); + pending_splice.funding_negotiation = + Some(FundingNegotiation::AwaitingSignatures { is_initiator, funding }); + interactive_tx_constructor + } else { + // Replace the taken state for later error handling + pending_splice.funding_negotiation = funding_negotiation; + return Err(AbortReason::InternalError( + "Got a tx_complete message in an invalid state", + )); + } } else { return Err(AbortReason::InternalError( "Got a tx_complete message in an invalid state", @@ -2159,7 +2129,7 @@ where let signing_session = interactive_tx_constructor.into_signing_session(); self.context_mut().interactive_tx_signing_session = Some(signing_session); - Ok(commitment_signed) + Ok(()) } pub fn force_shutdown(&mut self, closure_reason: ClosureReason) -> ShutdownResult { @@ -2911,6 +2881,7 @@ where /// send it first. resend_order: RAACommitmentOrder, + monitor_pending_tx_signatures: bool, monitor_pending_channel_ready: bool, monitor_pending_revoke_and_ack: bool, monitor_pending_commitment_signed: bool, @@ -3642,6 +3613,7 @@ where resend_order: RAACommitmentOrder::CommitmentFirst, + monitor_pending_tx_signatures: false, monitor_pending_channel_ready: false, monitor_pending_revoke_and_ack: false, monitor_pending_commitment_signed: false, @@ -3881,6 +3853,7 @@ where resend_order: RAACommitmentOrder::CommitmentFirst, + monitor_pending_tx_signatures: false, monitor_pending_channel_ready: false, monitor_pending_revoke_and_ack: false, monitor_pending_commitment_signed: false, @@ -6860,8 +6833,25 @@ type BestBlockUpdatedRes = ( Option, ); +/// The result of handling a `tx_complete` message during interactive transaction construction. +pub(crate) struct TxCompleteResult { + /// The message to send to the counterparty, if any. + pub interactive_tx_msg_send: Option, + + /// If the negotiation completed and the holder has local contributions, this contains the + /// unsigned funding transaction for the `FundingTransactionReadyForSigning` event. + pub event_unsigned_tx: Option, + + /// If the negotiation completed and the holder has no local contributions, this contains + /// the result of automatically calling `funding_transaction_signed` with empty witnesses. + pub funding_tx_signed: Option, +} + /// The result of signing a funding transaction negotiated using the interactive-tx protocol. pub struct FundingTxSigned { + /// The initial `commitment_signed` message to send to the counterparty, if necessary. + pub commitment_signed: Option, + /// Signatures that should be sent to the counterparty, if necessary. pub tx_signatures: Option, @@ -8051,6 +8041,7 @@ where Vec::new(), logger, ); + self.context.monitor_pending_tx_signatures = true; Ok(self.push_ret_blockable_mon_update(monitor_update)) } @@ -9104,6 +9095,7 @@ where // 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 { + commitment_signed: None, tx_signatures: None, funding_tx: None, splice_negotiated: None, @@ -9117,6 +9109,7 @@ where // 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 { + commitment_signed: None, tx_signatures: None, funding_tx: None, splice_negotiated: None, @@ -9178,7 +9171,21 @@ where (None, None) }; - Ok(FundingTxSigned { tx_signatures, funding_tx, splice_negotiated, splice_locked }) + let funding = self + .pending_splice + .as_ref() + .and_then(|pending_splice| pending_splice.funding_negotiation.as_ref()) + .and_then(|funding_negotiation| funding_negotiation.as_funding()) + .unwrap_or(&self.funding); + let commitment_signed = self.context.get_initial_commitment_signed_v2(funding, &&logger); + + Ok(FundingTxSigned { + commitment_signed, + tx_signatures, + funding_tx, + splice_negotiated, + splice_locked, + }) } pub fn tx_signatures( @@ -9246,6 +9253,7 @@ where }; Ok(FundingTxSigned { + commitment_signed: None, tx_signatures: holder_tx_signatures, funding_tx, splice_negotiated, @@ -9451,6 +9459,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. @@ -9539,7 +9566,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, } } @@ -15074,6 +15101,9 @@ where let pending_splice = self.pending_splice.as_ref().filter(|_| !self.should_reset_pending_splice_state(false)); + let monitor_pending_tx_signatures = + self.context.monitor_pending_tx_signatures.then_some(()); + write_tlv_fields!(writer, { (0, self.context.announcement_sigs, option), // minimum_depth and counterparty_selected_channel_reserve_satoshis used to have a @@ -15093,6 +15123,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), // Added in 0.3 (13, self.context.channel_creation_height, required), (15, preimages, required_vec), (17, self.context.announcement_sigs_state, required), @@ -15524,6 +15555,8 @@ where let mut holding_cell_accountable: Option> = None; let mut pending_outbound_accountable: Option> = None; + let mut monitor_pending_tx_signatures: Option<()> = None; + read_tlv_fields!(reader, { (0, announcement_sigs, option), (1, minimum_depth, option), @@ -15537,6 +15570,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), // Added in 0.3 (13, channel_creation_height, required), (15, preimages, required_vec), // The preimages transitioned from optional to required in 0.2 (17, announcement_sigs_state, required), @@ -15949,6 +15983,7 @@ where resend_order, + monitor_pending_tx_signatures: monitor_pending_tx_signatures.is_some(), monitor_pending_channel_ready, monitor_pending_revoke_and_ack, monitor_pending_commitment_signed, diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index dafeffe98bf..598e1d3b554 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -6455,7 +6455,8 @@ where &self.logger, ) { Ok(FundingTxSigned { - tx_signatures: Some(tx_signatures), + commitment_signed, + tx_signatures, funding_tx, splice_negotiated, splice_locked, @@ -6481,19 +6482,39 @@ where 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, - }, - ); + if chan.context.is_connected() { + if let Some(commitment_signed) = commitment_signed { + peer_state.pending_msg_events.push( + MessageSendEvent::UpdateHTLCs { + node_id: *counterparty_node_id, + channel_id: *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(tx_signatures) = tx_signatures { + 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; }, @@ -6501,17 +6522,6 @@ where 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 => { @@ -10151,79 +10161,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); @@ -11206,30 +11143,69 @@ 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() { - NotifyOption::SkipPersistHandleEvents - } else { - NotifyOption::SkipPersistNoEvents - }; - if let Some(interactive_tx_msg_send) = interactive_tx_msg_send { + Ok(tx_complete_result) => { + let mut persist = NotifyOption::SkipPersistNoEvents; + + if let Some(interactive_tx_msg_send) = tx_complete_result.interactive_tx_msg_send { let msg_send_event = interactive_tx_msg_send.into_msg_send_event(counterparty_node_id); peer_state.pending_msg_events.push(msg_send_event); + persist = NotifyOption::SkipPersistHandleEvents; }; - 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) = tx_complete_result.event_unsigned_tx { + self.pending_events.lock().unwrap().push_back(( + events::Event::FundingTransactionReadyForSigning { + unsigned_transaction, + counterparty_node_id, + channel_id: msg.channel_id, + user_channel_id: chan.context().get_user_id(), }, - }); + None, + )); + // + // We have a successful signing session that we need to persist. + persist = NotifyOption::DoPersist; + } + + if let Some(FundingTxSigned { + commitment_signed, + tx_signatures, + funding_tx, + splice_negotiated, + splice_locked, + }) = tx_complete_result.funding_tx_signed + { + // We shouldn't expect to see the splice negotiated or locked yet as we + // haven't exchanged `tx_signatures` at this point. + debug_assert!(funding_tx.is_none()); + debug_assert!(splice_negotiated.is_none()); + debug_assert!(splice_locked.is_none()); + + 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(tx_signatures) = tx_signatures { + peer_state.pending_msg_events.push(MessageSendEvent::SendTxSignatures { + node_id: counterparty_node_id, + msg: tx_signatures, + }); + } + + // We have a successful signing session that we need to persist. + persist = NotifyOption::DoPersist; } + Ok(persist) }, Err((error, splice_funding_failed)) => { @@ -11274,6 +11250,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ Some(chan) => { let best_block_height = self.best_block.read().unwrap().height; let FundingTxSigned { + commitment_signed, tx_signatures, funding_tx, splice_negotiated, @@ -11284,6 +11261,11 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ chan.tx_signatures(msg, best_block_height, &self.logger), chan_entry ); + + // We should never be sending a `commitment_signed` in response to their + // `tx_signatures`. + debug_assert!(commitment_signed.is_none()); + if let Some(tx_signatures) = tx_signatures { peer_state.pending_msg_events.push(MessageSendEvent::SendTxSignatures { node_id: *counterparty_node_id, @@ -19013,6 +18995,32 @@ where } } + // We may need to regenerate [`Event::FundingTransactionReadyForSigning`] for channels that + // still need their holder `tx_signatures`. + for (counterparty_node_id, peer_state_mutex) in per_peer_state.iter() { + let peer_state = peer_state_mutex.lock().unwrap(); + for (channel_id, chan) in peer_state.channel_by_id.iter() { + if let Some(signing_session) = + chan.context().interactive_tx_signing_session.as_ref() + { + if signing_session.holder_tx_signatures().is_none() + && signing_session.has_local_contribution() + { + let unsigned_transaction = signing_session.unsigned_tx().tx().clone(); + pending_events_read.push_back(( + Event::FundingTransactionReadyForSigning { + unsigned_transaction, + counterparty_node_id: *counterparty_node_id, + channel_id: *channel_id, + user_channel_id: chan.context().get_user_id(), + }, + None, + )); + } + } + } + } + let best_block = BestBlock::new(best_block_hash, best_block_height); let flow = OffersMessageFlow::new( chain_hash, diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 2cf5ea96acb..a70879718e2 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -1086,7 +1086,7 @@ macro_rules! get_event { let ev = events.pop().unwrap(); match ev { $event_type { .. } => ev, - _ => panic!("Unexpected event"), + _ => panic!("Unexpected event {ev:?}"), } }}; } diff --git a/lightning/src/ln/interactivetxs.rs b/lightning/src/ln/interactivetxs.rs index 7ed829886c6..f402ac5efa6 100644 --- a/lightning/src/ln/interactivetxs.rs +++ b/lightning/src/ln/interactivetxs.rs @@ -668,10 +668,9 @@ 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()) + let holder_tx_signatures = (self.has_received_commitment_signed + && (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") }); diff --git a/lightning/src/ln/splicing_tests.rs b/lightning/src/ln/splicing_tests.rs index db6680d963c..c0ba401cfae 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::hashes::Hash; use bitcoin::secp256k1::PublicKey; @@ -132,7 +131,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( @@ -141,7 +140,7 @@ pub fn negotiate_splice_tx<'a, 'b, 'c, 'd>( channel_id, initiator_contribution, new_funding_script, - ) + ); } pub fn complete_splice_handshake<'a, 'b, 'c, 'd>( @@ -184,7 +183,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(); @@ -240,22 +239,15 @@ 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:?}" - ); - if let MessageSendEvent::SendTxComplete { ref msg, .. } = msg_events.remove(0) { + let msg_events = initiator.node.get_and_clear_pending_msg_events(); + assert_eq!(msg_events.len(), 1, "{msg_events:?}"); + if let MessageSendEvent::SendTxComplete { ref msg, .. } = &msg_events[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!(); + break; } } @@ -271,13 +263,38 @@ 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()); + + let event = get_event!(initiator, Event::FundingTransactionReadyForSigning); + if let Event::FundingTransactionReadyForSigning { + channel_id, + counterparty_node_id, + unsigned_transaction, + .. + } = event + { + let partially_signed_tx = initiator.wallet_source.sign_tx(unsigned_transaction).unwrap(); + initiator + .node + .funding_transaction_signed(&channel_id, &counterparty_node_id, partially_signed_tx) + .unwrap(); + } else { + panic!(); + } + + let msg_events = initiator.node.get_and_clear_pending_msg_events(); + assert_eq!(msg_events.len(), 1, "{msg_events:?}"); + let initial_commit_sig_for_acceptor = + if let MessageSendEvent::UpdateHTLCs { ref updates, .. } = &msg_events[0] { + updates.commitment_signed[0].clone() + } else { + panic!(); + }; acceptor.node.handle_commitment_signed(node_id_initiator, &initial_commit_sig_for_acceptor); let msg_events = acceptor.node.get_and_clear_pending_msg_events(); @@ -294,20 +311,6 @@ pub fn sign_interactive_funding_tx<'a, 'b, 'c, 'd>( panic!(); } - let event = get_event!(initiator, Event::FundingTransactionReadyForSigning); - if let Event::FundingTransactionReadyForSigning { - channel_id, - counterparty_node_id, - unsigned_transaction, - .. - } = event - { - let partially_signed_tx = initiator.wallet_source.sign_tx(unsigned_transaction).unwrap(); - initiator - .node - .funding_transaction_signed(&channel_id, &counterparty_node_id, partially_signed_tx) - .unwrap(); - } 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] { @@ -348,15 +351,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); @@ -642,15 +644,13 @@ 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 _event = get_event!(nodes[0], Event::FundingTransactionReadyForSigning); if reload { let encoded_monitor_0 = get_monitor!(nodes[0], channel_id).encode(); @@ -671,6 +671,8 @@ fn do_test_splice_state_reset_on_disconnect(reload: bool) { chain_monitor_1c, node_1c ); + // We should have another signing event generated upon reload as they're not persisted. + let _ = get_event!(nodes[0], Event::FundingTransactionReadyForSigning); } else { nodes[0].node.peer_disconnected(node_id_1); nodes[1].node.peer_disconnected(node_id_0); @@ -1290,25 +1292,17 @@ fn do_test_splice_reestablish(reload: bool, async_monitor_update: bool) { script_pubkey: nodes[1].wallet_source.get_change_script().unwrap(), }, ]); - 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); - 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); + negotiate_splice_tx(&nodes[0], &nodes[1], channel_id, initiator_contribution); - macro_rules! reconnect_nodes { - ($f: expr) => { - nodes[0].node.peer_disconnected(node_id_1); - nodes[1].node.peer_disconnected(node_id_0); - let mut reconnect_args = ReconnectArgs::new(&nodes[0], &nodes[1]); - $f(&mut reconnect_args); - reconnect_nodes(reconnect_args); - }; - } + // Node 0 should have a signing event to handle since they had a contribution in the splice. + // Node 1 won't and will immediately try to send their initial `commitment_signed`. + let signing_event = get_event!(nodes[0], Event::FundingTransactionReadyForSigning); + assert!(nodes[1].node.get_and_clear_pending_events().is_empty()); - // Reestablishing now should force both nodes to retransmit their initial `commitment_signed` - // message as they were never delivered. + assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); + let _ = get_htlc_update_msgs(&nodes[1], &node_id_0); + + // Disconnect them, and handle the signing event on the initiator side. if reload { let encoded_monitor_0 = get_monitor!(nodes[0], channel_id).encode(); reload_node!( @@ -1328,6 +1322,8 @@ fn do_test_splice_reestablish(reload: bool, async_monitor_update: bool) { chain_monitor_1a, node_1a ); + // We should have another signing event generated upon reload as they're not persisted. + let _ = get_event!(nodes[0], Event::FundingTransactionReadyForSigning); if async_monitor_update { persister_0a.set_update_ret(ChannelMonitorUpdateStatus::InProgress); persister_1a.set_update_ret(ChannelMonitorUpdateStatus::InProgress); @@ -1341,6 +1337,17 @@ fn do_test_splice_reestablish(reload: bool, async_monitor_update: bool) { } } + 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(); + } + + // Since they're not connected, no messages should be sent. + assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); + assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); + + // Reestablishing now should force both nodes to retransmit their initial `commitment_signed` + // message as they were never delivered. let mut reconnect_args = ReconnectArgs::new(&nodes[0], &nodes[1]); reconnect_args.send_interactive_tx_commit_sig = (true, true); reconnect_nodes(reconnect_args); @@ -1350,6 +1357,16 @@ fn do_test_splice_reestablish(reload: bool, async_monitor_update: bool) { check_added_monitors(&nodes[0], 1); check_added_monitors(&nodes[1], 1); + macro_rules! reconnect_nodes { + ($f: expr) => { + nodes[0].node.peer_disconnected(node_id_1); + nodes[1].node.peer_disconnected(node_id_0); + let mut reconnect_args = ReconnectArgs::new(&nodes[0], &nodes[1]); + $f(&mut reconnect_args); + reconnect_nodes(reconnect_args); + }; + } + if async_monitor_update { // Reconnecting again should result in no messages/events being generated as the monitor // update is pending. @@ -1364,11 +1381,9 @@ 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); + // Both nodes should have their `tx_signatures` ready after completing the monitor update, but + // node 0 has to wait for node 1 to send theirs first. 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); // Reconnecting now should force node 1 to retransmit their `tx_signatures` since it was never @@ -1377,18 +1392,6 @@ fn do_test_splice_reestablish(reload: bool, async_monitor_update: bool) { 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); @@ -1639,25 +1642,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); @@ -1781,25 +1781,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); @@ -1829,7 +1826,7 @@ fn disconnect_on_unexpected_interactive_tx_message() { let initiator = &nodes[0]; let acceptor = &nodes[1]; - let _node_id_initiator = initiator.node.get_our_node_id(); + let node_id_initiator = initiator.node.get_our_node_id(); let node_id_acceptor = acceptor.node.get_our_node_id(); let initial_channel_capacity = 100_000; @@ -1846,11 +1843,10 @@ 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 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 { .. })); + let _ = get_event!(initiator, Event::FundingTransactionReadyForSigning); + let _ = get_htlc_update_msgs(acceptor, &node_id_initiator); let tx_complete = msgs::TxComplete { channel_id }; initiator.node.handle_tx_complete(node_id_acceptor, &tx_complete); @@ -1910,58 +1906,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] diff --git a/pending_changelog/4336.txt b/pending_changelog/4336.txt new file mode 100644 index 00000000000..a41c71ca2b4 --- /dev/null +++ b/pending_changelog/4336.txt @@ -0,0 +1,5 @@ +Forward Compatibility +===================== + +* Downgrading from 0.3 will not be possible while a splice is pending when using async monitor + updates. From 7e226a039c977c9a96050e6fd5fa23cb2296e8bc Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Thu, 22 Jan 2026 09:49:59 -0800 Subject: [PATCH 2/3] Buffer interactive-tx initial commitment signed from counterparty This is crucial to enable the splice cancellation use case. When we process the initial commitment signed from our counterparty, we queue a monitor update that cannot be undone. To give the user a chance to abort the splice negotiation before it's committed to, we buffer the message until a successful call to `Channel::funding_transaction_signed` and process it then. Note that this is currently only done for splice and RBF attempts, as if we want to abort a dual funding negotiation, we can just force close the channel as it hasn't been funded yet. --- lightning/src/ln/channel.rs | 94 ++++++++-- lightning/src/ln/channelmanager.rs | 266 ++++++++++++++++++----------- lightning/src/ln/splicing_tests.rs | 231 +++++++++++++++++++++++++ 3 files changed, 481 insertions(+), 110 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 23edf616dae..de42f413cb3 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1902,10 +1902,11 @@ where } } - pub fn tx_complete( - &mut self, msg: &msgs::TxComplete, logger: &L, + pub fn tx_complete( + &mut self, msg: &msgs::TxComplete, fee_estimator: &LowerBoundedFeeEstimator, logger: &L, ) -> Result)> where + F::Target: FeeEstimator, L::Target: Logger, { let tx_complete_action = match self.interactive_tx_constructor_mut() { @@ -1954,7 +1955,7 @@ where let funding_tx_signed = if !has_local_contribution { let funding_txid = signing_session.unsigned_tx().tx().compute_txid(); if let ChannelPhase::Funded(chan) = &mut self.phase { - chan.funding_transaction_signed(funding_txid, vec![], 0, logger).ok() + chan.funding_transaction_signed(funding_txid, vec![], 0, fee_estimator, logger).ok() } else { None } @@ -2104,7 +2105,11 @@ where funding.channel_transaction_parameters.funding_outpoint = Some(funding_outpoint); pending_splice.funding_negotiation = - Some(FundingNegotiation::AwaitingSignatures { is_initiator, funding }); + Some(FundingNegotiation::AwaitingSignatures { + is_initiator, + funding, + initial_commitment_signed_from_counterparty: None, + }); interactive_tx_constructor } else { // Replace the taken state for later error handling @@ -2193,9 +2198,33 @@ 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)) + 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); + + // We delay processing this until the user manually approves the splice via + // [`FundedChannel::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_commitment_signed_from_counterparty, .. + } = funding_negotiation { + *initial_commitment_signed_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)) @@ -2679,6 +2708,17 @@ enum FundingNegotiation { AwaitingSignatures { funding: FundingScope, is_initiator: bool, + /// The initial [`msgs::CommitmentSigned`] message received for the [`FundingScope`] above. + /// We delay processing this until the user manually approves the splice via + /// [`FundedChannel::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, and we can just force close the channel. + /// + /// This field is not persisted as the message should be resent on reconnections. + initial_commitment_signed_from_counterparty: Option, }, } @@ -2686,6 +2726,7 @@ impl_writeable_tlv_based_enum_upgradable!(FundingNegotiation, (0, AwaitingSignatures) => { (1, funding, required), (3, is_initiator, required), + (_unused, initial_commitment_signed_from_counterparty, (static_value, None)), }, unread_variants: AwaitingAck, ConstructingTransaction ); @@ -6834,7 +6875,7 @@ type BestBlockUpdatedRes = ( ); /// The result of handling a `tx_complete` message during interactive transaction construction. -pub(crate) struct TxCompleteResult { +pub(super) struct TxCompleteResult { /// The message to send to the counterparty, if any. pub interactive_tx_msg_send: Option, @@ -6848,10 +6889,15 @@ pub(crate) struct TxCompleteResult { } /// The result of signing a funding transaction negotiated using the interactive-tx protocol. -pub struct FundingTxSigned { +pub(super) struct FundingTxSigned { /// The initial `commitment_signed` message to send to the counterparty, if necessary. pub commitment_signed: Option, + /// The result of processing a buffered initial commitment signed from our counterparty, + /// if any. + pub counterparty_initial_commitment_signed_result: + Option, ChannelError>>, + /// Signatures that should be sent to the counterparty, if necessary. pub tx_signatures: Option, @@ -9071,11 +9117,12 @@ where } } - pub fn funding_transaction_signed( + pub fn funding_transaction_signed( &mut self, funding_txid_signed: Txid, witnesses: Vec, best_block_height: u32, - logger: &L, + fee_estimator: &LowerBoundedFeeEstimator, logger: &L, ) -> Result where + F::Target: FeeEstimator, L::Target: Logger, { let signing_session = @@ -9096,6 +9143,7 @@ where // or we're waiting for our counterparty to send theirs first. return Ok(FundingTxSigned { commitment_signed: None, + counterparty_initial_commitment_signed_result: None, tx_signatures: None, funding_tx: None, splice_negotiated: None, @@ -9110,6 +9158,7 @@ where // no longer have the signing session present. return Ok(FundingTxSigned { commitment_signed: None, + counterparty_initial_commitment_signed_result: None, tx_signatures: None, funding_tx: None, splice_negotiated: None, @@ -9179,8 +9228,30 @@ where .unwrap_or(&self.funding); let commitment_signed = self.context.get_initial_commitment_signed_v2(funding, &&logger); + // If we have a pending splice with a buffered initial commitment_signed from our + // counterparty, process it now that we have provided our signatures. + let counterparty_initial_commitment_signed_result = 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_commitment_signed_from_counterparty, + .. + } = funding_negotiation + { + initial_commitment_signed_from_counterparty.take() + } else { + None + } + }) + .map(|commit_sig| { + self.splice_initial_commitment_signed(&commit_sig, fee_estimator, &&logger) + }); + Ok(FundingTxSigned { commitment_signed, + counterparty_initial_commitment_signed_result, tx_signatures, funding_tx, splice_negotiated, @@ -9254,6 +9325,7 @@ where Ok(FundingTxSigned { commitment_signed: None, + counterparty_initial_commitment_signed_result: None, tx_signatures: holder_tx_signatures, funding_tx, splice_negotiated, diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 598e1d3b554..63e11d2ac2f 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -6424,118 +6424,164 @@ where pub fn funding_transaction_signed( &self, channel_id: &ChannelId, counterparty_node_id: &PublicKey, transaction: Transaction, ) -> Result<(), APIError> { - let mut result = Ok(()); + let mut funding_tx_signed_result = Ok(()); + let mut monitor_update_result: Option< + Result, + > = None; + PersistenceNotifierGuard::optionally_notify(self, || { let per_peer_state = self.per_peer_state.read().unwrap(); let peer_state_mutex_opt = per_peer_state.get(counterparty_node_id); if peer_state_mutex_opt.is_none() { - result = Err(APIError::ChannelUnavailable { + funding_tx_signed_result = Err(APIError::ChannelUnavailable { err: format!("Can't find a peer matching the passed counterparty node_id {counterparty_node_id}") }); return NotifyOption::SkipPersistNoEvents; } - let mut peer_state = peer_state_mutex_opt.unwrap().lock().unwrap(); + let mut peer_state_lock = peer_state_mutex_opt.unwrap().lock().unwrap(); + let peer_state = &mut *peer_state_lock; - 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 { - commitment_signed, - 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, - )); - } - if chan.context.is_connected() { - if let Some(commitment_signed) = commitment_signed { - peer_state.pending_msg_events.push( - MessageSendEvent::UpdateHTLCs { - node_id: *counterparty_node_id, - channel_id: *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, - }, - }, + match peer_state.channel_by_id.entry(*channel_id) { + hash_map::Entry::Occupied(mut chan_entry) => { + match chan_entry.get_mut().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.fee_estimator, + &self.logger, + ) { + Ok(FundingTxSigned { + commitment_signed, + counterparty_initial_commitment_signed_result, + 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(tx_signatures) = tx_signatures { - peer_state.pending_msg_events.push( - MessageSendEvent::SendTxSignatures { - node_id: *counterparty_node_id, - msg: tx_signatures, + 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, + )); } - if let Some(splice_locked) = splice_locked { - peer_state.pending_msg_events.push( - MessageSendEvent::SendSpliceLocked { - node_id: *counterparty_node_id, - msg: splice_locked, - }, - ); + + if chan.context.is_connected() { + if let Some(commitment_signed) = commitment_signed { + peer_state.pending_msg_events.push( + MessageSendEvent::UpdateHTLCs { + node_id: *counterparty_node_id, + channel_id: *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(tx_signatures) = tx_signatures { + 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; - }, - } - }, - None => { - result = Err(APIError::APIMisuseError { - err: format!( - "Channel with id {} not expecting funding signatures", - channel_id - ), - }); - return NotifyOption::SkipPersistNoEvents; - }, + + match counterparty_initial_commitment_signed_result { + Some(Ok(Some(monitor_update))) => { + let funding_txo = chan.funding.get_funding_txo(); + if let Some(post_update_data) = self + .handle_new_monitor_update( + &mut peer_state.in_flight_monitor_updates, + &mut peer_state.monitor_update_blocked_actions, + &mut peer_state.pending_msg_events, + peer_state.is_connected, + chan, + funding_txo.unwrap(), + monitor_update, + ) { + monitor_update_result = Some(Ok(post_update_data)); + } + }, + Some(Err(err)) => { + let (drop, err) = self + .locked_handle_funded_force_close( + &mut peer_state + .closed_channel_monitor_update_ids, + &mut peer_state.in_flight_monitor_updates, + err, + chan, + ); + if drop { + chan_entry.remove_entry(); + } + + monitor_update_result = Some(Err(err)); + }, + Some(Ok(None)) | None => {}, + } + + funding_tx_signed_result = Ok(()); + }, + Err(err) => { + funding_tx_signed_result = Err(err); + return NotifyOption::SkipPersistNoEvents; + }, + } + }, + None => { + funding_tx_signed_result = Err(APIError::APIMisuseError { + err: format!( + "Channel with id {} not expecting funding signatures", + channel_id + ), + }); + return NotifyOption::SkipPersistNoEvents; + }, + } }, - None => { - result = Err(APIError::ChannelUnavailable { + hash_map::Entry::Vacant(_) => { + funding_tx_signed_result = Err(APIError::ChannelUnavailable { err: format!( "Channel with id {} not found for the passed counterparty node_id {}", channel_id, counterparty_node_id @@ -6544,9 +6590,25 @@ where return NotifyOption::SkipPersistNoEvents; }, } + + mem::drop(peer_state_lock); + mem::drop(per_peer_state); + + if let Some(monitor_update_result) = monitor_update_result { + match monitor_update_result { + Ok(post_update_data) => { + self.handle_post_monitor_update_chan_resume(post_update_data); + }, + Err(_) => { + let _ = self.handle_error(monitor_update_result, *counterparty_node_id); + }, + } + } + + NotifyOption::DoPersist }); - result + funding_tx_signed_result } fn broadcast_interactive_funding( @@ -11142,7 +11204,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ match peer_state.channel_by_id.entry(msg.channel_id) { hash_map::Entry::Occupied(mut chan_entry) => { let chan = chan_entry.get_mut(); - match chan.tx_complete(msg, &self.logger) { + match chan.tx_complete(msg, &self.fee_estimator, &self.logger) { Ok(tx_complete_result) => { let mut persist = NotifyOption::SkipPersistNoEvents; @@ -11169,6 +11231,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ if let Some(FundingTxSigned { commitment_signed, + counterparty_initial_commitment_signed_result, tx_signatures, funding_tx, splice_negotiated, @@ -11176,10 +11239,13 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ }) = tx_complete_result.funding_tx_signed { // We shouldn't expect to see the splice negotiated or locked yet as we - // haven't exchanged `tx_signatures` at this point. + // haven't exchanged `tx_signatures` at this point. Similarly, we + // shouldn't have a result for the counterparty's initial commitment + // signed as they haven't sent it yet. debug_assert!(funding_tx.is_none()); debug_assert!(splice_negotiated.is_none()); debug_assert!(splice_locked.is_none()); + debug_assert!(counterparty_initial_commitment_signed_result.is_none()); if let Some(commitment_signed) = commitment_signed { peer_state.pending_msg_events.push(MessageSendEvent::UpdateHTLCs { @@ -11251,6 +11317,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ let best_block_height = self.best_block.read().unwrap().height; let FundingTxSigned { commitment_signed, + counterparty_initial_commitment_signed_result, tx_signatures, funding_tx, splice_negotiated, @@ -11265,6 +11332,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ // We should never be sending a `commitment_signed` in response to their // `tx_signatures`. debug_assert!(commitment_signed.is_none()); + debug_assert!(counterparty_initial_commitment_signed_result.is_none()); if let Some(tx_signatures) = tx_signatures { peer_state.pending_msg_events.push(MessageSendEvent::SendTxSignatures { diff --git a/lightning/src/ln/splicing_tests.rs b/lightning/src/ln/splicing_tests.rs index c0ba401cfae..ef524db6be3 100644 --- a/lightning/src/ln/splicing_tests.rs +++ b/lightning/src/ln/splicing_tests.rs @@ -29,6 +29,7 @@ use crate::util::errors::APIError; use crate::util::ser::Writeable; use bitcoin::hashes::Hash; +use bitcoin::secp256k1::ecdsa::Signature; use bitcoin::secp256k1::PublicKey; use bitcoin::{Amount, OutPoint as BitcoinOutPoint, ScriptBuf, Transaction, TxOut, WPubkeyHash}; @@ -2221,3 +2222,233 @@ fn test_splice_with_inflight_htlc_forward_and_resolution() { do_test_splice_with_inflight_htlc_forward_and_resolution(true); do_test_splice_with_inflight_htlc_forward_and_resolution(false); } + +#[test] +fn test_splice_buffer_commitment_signed_until_funding_tx_signed() { + // Test that when the counterparty sends their initial `commitment_signed` before the user has + // called `funding_transaction_signed`, we buffer the message and process it at the end of + // `funding_transaction_signed`. This allows the user to cancel the splice negotiation if + // desired without having queued an irreversible monitor update. + let chanmon_cfgs = create_chanmon_cfgs(2); + let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); + let nodes = create_network(2, &node_cfgs, &node_chanmgrs); + + let node_id_0 = nodes[0].node.get_our_node_id(); + let node_id_1 = nodes[1].node.get_our_node_id(); + + let initial_channel_value_sat = 100_000; + let (_, _, channel_id, _) = + create_announced_chan_between_nodes_with_value(&nodes, 0, 1, initial_channel_value_sat, 0); + + // Negotiate a splice-out where only the initiator (node 0) has a contribution. + // This means node 1 will send their commitment_signed immediately after tx_complete. + let initiator_contribution = SpliceContribution::splice_out(vec![TxOut { + value: Amount::from_sat(1_000), + script_pubkey: nodes[0].wallet_source.get_change_script().unwrap(), + }]); + negotiate_splice_tx(&nodes[0], &nodes[1], channel_id, initiator_contribution); + + // Node 0 (initiator with contribution) should have a signing event to handle. + let signing_event = get_event!(nodes[0], Event::FundingTransactionReadyForSigning); + + // Node 1 (acceptor with no contribution) won't have a signing event and will immediately + // send their initial commitment_signed. + assert!(nodes[1].node.get_and_clear_pending_events().is_empty()); + assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); + let acceptor_commit_sig = get_htlc_update_msgs(&nodes[1], &node_id_0); + + // Deliver the acceptor's commitment_signed to the initiator BEFORE the initiator has called + // funding_transaction_signed. The message should be buffered, not processed. + nodes[0].node.handle_commitment_signed(node_id_1, &acceptor_commit_sig.commitment_signed[0]); + + // No monitor update should have happened since the message is buffered. + check_added_monitors(&nodes[0], 0); + assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); + + // Now handle the signing event and call `funding_transaction_signed`. + if let Event::FundingTransactionReadyForSigning { + channel_id: event_channel_id, + counterparty_node_id, + unsigned_transaction, + .. + } = signing_event + { + assert_eq!(event_channel_id, channel_id); + assert_eq!(counterparty_node_id, node_id_1); + + let partially_signed_tx = nodes[0].wallet_source.sign_tx(unsigned_transaction).unwrap(); + nodes[0] + .node + .funding_transaction_signed(&channel_id, &node_id_1, partially_signed_tx) + .unwrap(); + } else { + panic!("Expected FundingTransactionReadyForSigning event"); + } + + // After funding_transaction_signed: + // 1. The initiator should send their commitment_signed + // 2. The buffered commitment_signed from the acceptor should be processed (monitor update) + let msg_events = nodes[0].node.get_and_clear_pending_msg_events(); + assert_eq!(msg_events.len(), 1, "{msg_events:?}"); + let initiator_commit_sig = + if let MessageSendEvent::UpdateHTLCs { ref updates, .. } = &msg_events[0] { + updates.commitment_signed[0].clone() + } else { + panic!("Expected UpdateHTLCs message"); + }; + + // The buffered commitment_signed should have been processed, resulting in a monitor update. + check_added_monitors(&nodes[0], 1); + + // Complete the rest of the flow normally. + nodes[1].node.handle_commitment_signed(node_id_0, &initiator_commit_sig); + let msg_events = nodes[1].node.get_and_clear_pending_msg_events(); + assert_eq!(msg_events.len(), 1, "{msg_events:?}"); + if let MessageSendEvent::SendTxSignatures { ref msg, .. } = &msg_events[0] { + nodes[0].node.handle_tx_signatures(node_id_1, msg); + } else { + panic!("Expected SendTxSignatures message"); + } + check_added_monitors(&nodes[1], 1); + + let msg_events = nodes[0].node.get_and_clear_pending_msg_events(); + assert_eq!(msg_events.len(), 1, "{msg_events:?}"); + if let MessageSendEvent::SendTxSignatures { ref msg, .. } = &msg_events[0] { + nodes[1].node.handle_tx_signatures(node_id_0, msg); + } else { + panic!("Expected SendTxSignatures message"); + } + + expect_splice_pending_event(&nodes[0], &node_id_1); + expect_splice_pending_event(&nodes[1], &node_id_0); + + // Both nodes should broadcast the splice transaction. + let splice_tx = { + let mut txn_0 = nodes[0].tx_broadcaster.txn_broadcast(); + assert_eq!(txn_0.len(), 1); + let txn_1 = nodes[1].tx_broadcaster.txn_broadcast(); + assert_eq!(txn_0, txn_1); + txn_0.remove(0) + }; + + // Verify the channel is operational by sending a payment. + send_payment(&nodes[0], &[&nodes[1]], 1_000_000); + + // Lock the splice by confirming the transaction. + mine_transaction(&nodes[0], &splice_tx); + mine_transaction(&nodes[1], &splice_tx); + lock_splice_after_blocks(&nodes[0], &nodes[1], ANTI_REORG_DELAY - 1); + + // Verify the channel is still operational by sending another payment. + send_payment(&nodes[0], &[&nodes[1]], 1_000_000); +} + +#[test] +fn test_splice_buffer_invalid_commitment_signed_closes_channel() { + // Test that when the counterparty sends an invalid `commitment_signed` (with a bad signature) + // before the user has called `funding_transaction_signed`, the channel is closed with an error + // when `ChannelManager::funding_transaction_signed` processes the buffered message. + let chanmon_cfgs = create_chanmon_cfgs(2); + let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); + let nodes = create_network(2, &node_cfgs, &node_chanmgrs); + + let node_id_0 = nodes[0].node.get_our_node_id(); + let node_id_1 = nodes[1].node.get_our_node_id(); + + let initial_channel_value_sat = 100_000; + let (_, _, channel_id, _) = + create_announced_chan_between_nodes_with_value(&nodes, 0, 1, initial_channel_value_sat, 0); + + // Negotiate a splice-out where only the initiator (node 0) has a contribution. + // This means node 1 will send their commitment_signed immediately after tx_complete. + let initiator_contribution = SpliceContribution::splice_out(vec![TxOut { + value: Amount::from_sat(1_000), + script_pubkey: nodes[0].wallet_source.get_change_script().unwrap(), + }]); + negotiate_splice_tx(&nodes[0], &nodes[1], channel_id, initiator_contribution); + + // Node 0 (initiator with contribution) should have a signing event to handle. + let signing_event = get_event!(nodes[0], Event::FundingTransactionReadyForSigning); + + // Node 1 (acceptor with no contribution) won't have a signing event and will immediately + // send their initial commitment_signed. + assert!(nodes[1].node.get_and_clear_pending_events().is_empty()); + assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); + let mut acceptor_commit_sig = get_htlc_update_msgs(&nodes[1], &node_id_0); + + // Invalidate the signature by modifying one byte. This will cause signature verification + // to fail when the buffered message is processed. + let original_sig = acceptor_commit_sig.commitment_signed[0].signature; + let mut sig_bytes = original_sig.serialize_compact(); + sig_bytes[0] ^= 0x01; // Flip a bit to corrupt the signature + acceptor_commit_sig.commitment_signed[0].signature = + Signature::from_compact(&sig_bytes).unwrap(); + + // Deliver the acceptor's invalid commitment_signed to the initiator BEFORE the initiator has + // called funding_transaction_signed. The message should be buffered, not processed. + nodes[0].node.handle_commitment_signed(node_id_1, &acceptor_commit_sig.commitment_signed[0]); + + // No monitor update should have happened since the message is buffered. + check_added_monitors(&nodes[0], 0); + assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); + + // Now handle the signing event and call `funding_transaction_signed`. + // This should process the buffered invalid commitment_signed and close the channel. + if let Event::FundingTransactionReadyForSigning { + channel_id: event_channel_id, + counterparty_node_id, + unsigned_transaction, + .. + } = signing_event + { + assert_eq!(event_channel_id, channel_id); + assert_eq!(counterparty_node_id, node_id_1); + + let partially_signed_tx = nodes[0].wallet_source.sign_tx(unsigned_transaction).unwrap(); + nodes[0] + .node + .funding_transaction_signed(&channel_id, &node_id_1, partially_signed_tx) + .unwrap(); + } else { + panic!("Expected FundingTransactionReadyForSigning event"); + } + + // After funding_transaction_signed: + // 1. The initiator sends its commitment_signed (UpdateHTLCs message). + // 2. The buffered invalid commitment_signed from the acceptor is processed, causing the + // channel to close due to the invalid signature. + // We expect 3 message events: UpdateHTLCs, BroadcastChannelUpdate, and HandleError. + let msg_events = nodes[0].node.get_and_clear_pending_msg_events(); + assert_eq!(msg_events.len(), 3, "{msg_events:?}"); + match &msg_events[0] { + MessageSendEvent::UpdateHTLCs { ref updates, .. } => { + assert!(!updates.commitment_signed.is_empty()); + }, + _ => panic!("Expected UpdateHTLCs message, got {:?}", msg_events[0]), + } + match &msg_events[1] { + MessageSendEvent::HandleError { + action: msgs::ErrorAction::SendErrorMessage { ref msg }, + .. + } => { + assert!(msg.data.contains("Invalid commitment tx signature from peer")); + }, + _ => panic!("Expected HandleError with SendErrorMessage, got {:?}", msg_events[1]), + } + match &msg_events[2] { + MessageSendEvent::BroadcastChannelUpdate { ref msg, .. } => { + assert_eq!(msg.contents.channel_flags & 2, 2); + }, + _ => panic!("Expected BroadcastChannelUpdate, got {:?}", msg_events[2]), + } + + let err = "Invalid commitment tx signature from peer".to_owned(); + let reason = ClosureReason::ProcessingError { err }; + check_closed_events( + &nodes[0], + &[ExpectedCloseEvent::from_id_reason(channel_id, false, reason)], + ); + check_added_monitors(&nodes[0], 1); +} From be67c67f528b6ff175323a752673792377e26ad5 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Thu, 22 Jan 2026 11:52:42 -0800 Subject: [PATCH 3/3] Support funding_transaction_signed for unfunded dual-funded channels Now that we require users to first call `ChannelManager::funding_transaction_signed` before releasing any signatures, it's possible that it is called before we receive the initial commitment signed from our counterparty, which would transition the channel to funded. Because of this, we need to support the API call while the channel is still in the unfunded phase. Note that this commit is mostly a code move of `FundedChannel::funding_transaction_signed` to `Channel::funding_transaction_signed` that doesn't alter the signing logic. --- lightning/src/ln/channel.rs | 335 ++++++++++++++++------------- lightning/src/ln/channelmanager.rs | 241 ++++++++++----------- 2 files changed, 302 insertions(+), 274 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index de42f413cb3..65a627f6282 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1954,11 +1954,18 @@ where let funding_tx_signed = if !has_local_contribution { let funding_txid = signing_session.unsigned_tx().tx().compute_txid(); - if let ChannelPhase::Funded(chan) = &mut self.phase { - chan.funding_transaction_signed(funding_txid, vec![], 0, fee_estimator, logger).ok() - } else { - None - } + self.funding_transaction_signed(funding_txid, vec![], 0, fee_estimator, logger) + .map(Some) + .map_err(|err| { + log_error!( + logger, + "Failed signing funding transaction without local contribution: {err:?}" + ); + self.fail_interactive_tx_negotiation( + AbortReason::InternalError("Signing failed"), + logger, + ) + })? } else { None }; @@ -2137,6 +2144,178 @@ where Ok(()) } + pub fn funding_transaction_signed( + &mut self, funding_txid_signed: Txid, witnesses: Vec, best_block_height: u32, + fee_estimator: &LowerBoundedFeeEstimator, logger: &L, + ) -> Result + where + F::Target: FeeEstimator, + L::Target: Logger, + { + let (context, funding, pending_splice) = match &mut self.phase { + ChannelPhase::Undefined => unreachable!(), + ChannelPhase::UnfundedV2(channel) => (&mut channel.context, &channel.funding, None), + ChannelPhase::Funded(channel) => { + (&mut channel.context, &channel.funding, channel.pending_splice.as_ref()) + }, + _ => { + return Err(APIError::APIMisuseError { + err: format!( + "Channel with id {} not expecting funding signatures", + self.context().channel_id + ), + }); + }, + }; + + let signing_session = if let Some(signing_session) = + context.interactive_tx_signing_session.as_mut() + { + if let Some(pending_splice) = 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 { + commitment_signed: None, + counterparty_initial_commitment_signed_result: None, + tx_signatures: None, + funding_tx: None, + splice_negotiated: None, + splice_locked: None, + }); + } + + signing_session + } else { + if Some(funding_txid_signed) == 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 { + commitment_signed: None, + counterparty_initial_commitment_signed_result: None, + tx_signatures: None, + funding_tx: None, + splice_negotiated: None, + splice_locked: None, + }); + } + let err = format!("Channel {} not expecting funding signatures", 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 &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, + }; + let (tx_signatures, funding_tx) = signing_session + .provide_holder_witnesses(tx_signatures, &context.secp_ctx) + .map_err(|err| APIError::APIMisuseError { err })?; + + let logger = WithChannelContext::from(logger, &context, None); + if tx_signatures.is_some() { + log_info!( + logger, + "Sending tx_signatures for interactive funding transaction {funding_txid_signed}" + ); + } + + let funding = pending_splice + .as_ref() + .and_then(|pending_splice| pending_splice.funding_negotiation.as_ref()) + .and_then(|funding_negotiation| funding_negotiation.as_funding()) + .unwrap_or(funding); + let commitment_signed = context.get_initial_commitment_signed_v2(funding, &&logger); + + // For zero conf channels, we don't expect the funding transaction to be ready for broadcast + // yet as, according to the spec, our counterparty shouldn't have sent their `tx_signatures` + // without us having sent our initial commitment signed to them first. However, in the event + // they do, we choose to handle it anyway. Note that because of this behavior not being + // spec-compliant, we're not able to test this without custom logic. + let (splice_negotiated, splice_locked) = if let Some(funding_tx) = funding_tx.clone() { + debug_assert!(tx_signatures.is_some()); + let funded_channel = self.as_funded_mut().expect( + "Funding transactions ready for broadcast can only exist for funded channels", + ); + funded_channel.on_tx_signatures_exchange(funding_tx, best_block_height, &logger) + } else { + (None, None) + }; + + // If we have a pending splice with a buffered initial commitment signed from our + // counterparty, process it now that we have provided our signatures. + let counterparty_initial_commitment_signed_result = + self.as_funded_mut().and_then(|funded_channel| { + funded_channel + .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_commitment_signed_from_counterparty, + .. + } = funding_negotiation + { + initial_commitment_signed_from_counterparty.take() + } else { + None + } + }) + .map(|commit_sig| { + funded_channel.splice_initial_commitment_signed( + &commit_sig, + fee_estimator, + &&logger, + ) + }) + }); + + Ok(FundingTxSigned { + commitment_signed, + counterparty_initial_commitment_signed_result, + tx_signatures, + funding_tx, + splice_negotiated, + splice_locked, + }) + } + pub fn force_shutdown(&mut self, closure_reason: ClosureReason) -> ShutdownResult { let (funding, context) = self.funding_and_context_mut(); context.force_shutdown(funding, closure_reason) @@ -2206,7 +2385,7 @@ where .unwrap_or(false); // We delay processing this until the user manually approves the splice via - // [`FundedChannel::funding_transaction_signed`], as otherwise, there would be a + // [`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 { @@ -2710,7 +2889,7 @@ enum FundingNegotiation { is_initiator: bool, /// The initial [`msgs::CommitmentSigned`] message received for the [`FundingScope`] above. /// We delay processing this until the user manually approves the splice via - /// [`FundedChannel::funding_transaction_signed`], as otherwise, there would be a + /// [`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. /// @@ -9117,148 +9296,6 @@ where } } - pub fn funding_transaction_signed( - &mut self, funding_txid_signed: Txid, witnesses: Vec, best_block_height: u32, - fee_estimator: &LowerBoundedFeeEstimator, logger: &L, - ) -> Result - where - F::Target: FeeEstimator, - 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 { - commitment_signed: None, - counterparty_initial_commitment_signed_result: None, - 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 { - commitment_signed: None, - counterparty_initial_commitment_signed_result: None, - 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) - }; - - let funding = self - .pending_splice - .as_ref() - .and_then(|pending_splice| pending_splice.funding_negotiation.as_ref()) - .and_then(|funding_negotiation| funding_negotiation.as_funding()) - .unwrap_or(&self.funding); - let commitment_signed = self.context.get_initial_commitment_signed_v2(funding, &&logger); - - // If we have a pending splice with a buffered initial commitment_signed from our - // counterparty, process it now that we have provided our signatures. - let counterparty_initial_commitment_signed_result = 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_commitment_signed_from_counterparty, - .. - } = funding_negotiation - { - initial_commitment_signed_from_counterparty.take() - } else { - None - } - }) - .map(|commit_sig| { - self.splice_initial_commitment_signed(&commit_sig, fee_estimator, &&logger) - }); - - Ok(FundingTxSigned { - commitment_signed, - counterparty_initial_commitment_signed_result, - tx_signatures, - funding_tx, - splice_negotiated, - splice_locked, - }) - } - pub fn tx_signatures( &mut self, msg: &msgs::TxSignatures, best_block_height: u32, logger: &L, ) -> Result diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 63e11d2ac2f..23e94e76f1c 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -6444,138 +6444,129 @@ where match peer_state.channel_by_id.entry(*channel_id) { hash_map::Entry::Occupied(mut chan_entry) => { - match chan_entry.get_mut().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.fee_estimator, - &self.logger, - ) { - Ok(FundingTxSigned { - commitment_signed, - counterparty_initial_commitment_signed_result, - 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, + 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; + + let chan = chan_entry.get_mut(); + match chan.funding_transaction_signed( + txid, + witnesses, + best_block_height, + &self.fee_estimator, + &self.logger, + ) { + Ok(FundingTxSigned { + commitment_signed, + counterparty_initial_commitment_signed_result, + tx_signatures, + funding_tx, + splice_negotiated, + splice_locked, + }) => { + if let Some(funding_tx) = funding_tx { + let funded_chan = chan.as_funded_mut().expect( + "Funding transactions ready for broadcast can only exist for funded channels", + ); + self.broadcast_interactive_funding( + funded_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, + )); + } + + if chan.context().is_connected() { + if let Some(commitment_signed) = commitment_signed { + peer_state.pending_msg_events.push( + MessageSendEvent::UpdateHTLCs { + node_id: *counterparty_node_id, + channel_id: *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, }, - None, - )); - } + }, + ); + } + if let Some(tx_signatures) = tx_signatures { + 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, + }, + ); + } + } - if chan.context.is_connected() { - if let Some(commitment_signed) = commitment_signed { - peer_state.pending_msg_events.push( - MessageSendEvent::UpdateHTLCs { - node_id: *counterparty_node_id, - channel_id: *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(tx_signatures) = tx_signatures { - peer_state.pending_msg_events.push( - MessageSendEvent::SendTxSignatures { - node_id: *counterparty_node_id, - msg: tx_signatures, - }, - ); + if let Some(funded_chan) = chan.as_funded_mut() { + match counterparty_initial_commitment_signed_result { + Some(Ok(Some(monitor_update))) => { + let funding_txo = funded_chan.funding.get_funding_txo(); + if let Some(post_update_data) = self + .handle_new_monitor_update( + &mut peer_state.in_flight_monitor_updates, + &mut peer_state.monitor_update_blocked_actions, + &mut peer_state.pending_msg_events, + peer_state.is_connected, + funded_chan, + funding_txo.unwrap(), + monitor_update, + ) { + monitor_update_result = Some(Ok(post_update_data)); } - if let Some(splice_locked) = splice_locked { - peer_state.pending_msg_events.push( - MessageSendEvent::SendSpliceLocked { - node_id: *counterparty_node_id, - msg: splice_locked, - }, - ); + }, + Some(Err(err)) => { + let (drop, err) = self.locked_handle_funded_force_close( + &mut peer_state.closed_channel_monitor_update_ids, + &mut peer_state.in_flight_monitor_updates, + err, + funded_chan, + ); + if drop { + chan_entry.remove_entry(); } - } - match counterparty_initial_commitment_signed_result { - Some(Ok(Some(monitor_update))) => { - let funding_txo = chan.funding.get_funding_txo(); - if let Some(post_update_data) = self - .handle_new_monitor_update( - &mut peer_state.in_flight_monitor_updates, - &mut peer_state.monitor_update_blocked_actions, - &mut peer_state.pending_msg_events, - peer_state.is_connected, - chan, - funding_txo.unwrap(), - monitor_update, - ) { - monitor_update_result = Some(Ok(post_update_data)); - } - }, - Some(Err(err)) => { - let (drop, err) = self - .locked_handle_funded_force_close( - &mut peer_state - .closed_channel_monitor_update_ids, - &mut peer_state.in_flight_monitor_updates, - err, - chan, - ); - if drop { - chan_entry.remove_entry(); - } - - monitor_update_result = Some(Err(err)); - }, - Some(Ok(None)) | None => {}, - } - - funding_tx_signed_result = Ok(()); - }, - Err(err) => { - funding_tx_signed_result = Err(err); - return NotifyOption::SkipPersistNoEvents; - }, + monitor_update_result = Some(Err(err)); + }, + Some(Ok(None)) | None => {}, + } } + + funding_tx_signed_result = Ok(()); }, - None => { - funding_tx_signed_result = Err(APIError::APIMisuseError { - err: format!( - "Channel with id {} not expecting funding signatures", - channel_id - ), - }); + Err(err) => { + funding_tx_signed_result = Err(err); return NotifyOption::SkipPersistNoEvents; }, }