diff --git a/lightning/src/ln/async_signer_tests.rs b/lightning/src/ln/async_signer_tests.rs index 451af3918bf..fd9c0ad7305 100644 --- a/lightning/src/ln/async_signer_tests.rs +++ b/lightning/src/ln/async_signer_tests.rs @@ -1648,3 +1648,95 @@ fn test_async_splice_initial_commit_sig() { let _ = get_event!(initiator, Event::SplicePending); let _ = get_event!(acceptor, Event::SplicePending); } + +#[test] +fn test_async_splice_initial_commit_sig_waits_for_monitor_before_tx_signatures() { + 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 channel_id = create_announced_chan_between_nodes(&nodes, 0, 1).2; + + let (initiator, acceptor) = (&nodes[0], &nodes[1]); + let initiator_node_id = initiator.node.get_our_node_id(); + let acceptor_node_id = acceptor.node.get_our_node_id(); + + acceptor.disable_channel_signer_op( + &initiator_node_id, + &channel_id, + SignerOp::SignCounterpartyCommitment, + ); + + // Negotiate a splice up until the signature exchange. + let outputs = vec![TxOut { + value: Amount::from_sat(1_000), + script_pubkey: nodes[0].wallet_source.get_change_script().unwrap(), + }]; + let contribution = initiate_splice_out(initiator, acceptor, channel_id, outputs).unwrap(); + negotiate_splice_tx(initiator, acceptor, channel_id, contribution); + + let event = get_event!(initiator, Event::FundingTransactionReadyForSigning); + if let Event::FundingTransactionReadyForSigning { unsigned_transaction, .. } = event { + let partially_signed_tx = initiator.wallet_source.sign_tx(unsigned_transaction).unwrap(); + initiator + .node + .funding_transaction_signed(&channel_id, &acceptor_node_id, partially_signed_tx) + .unwrap(); + } + + let initiator_commit_sig = get_htlc_update_msgs(initiator, &acceptor_node_id); + + // Keep the monitor update from processing the initiator's initial commitment signed pending on + // the acceptor. + chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); + acceptor + .node + .handle_commitment_signed(initiator_node_id, &initiator_commit_sig.commitment_signed[0]); + check_added_monitors(acceptor, 1); + assert!(acceptor.node.get_and_clear_pending_msg_events().is_empty()); + + // Once the async signer is unblocked, we should send the initial commitment_signed, but still + // hold back tx_signatures until the monitor update is completed. + acceptor.enable_channel_signer_op( + &initiator_node_id, + &channel_id, + SignerOp::SignCounterpartyCommitment, + ); + acceptor.node.signer_unblocked(None); + + let msg_events = acceptor.node.get_and_clear_pending_msg_events(); + assert_eq!(msg_events.len(), 1, "{msg_events:?}"); + if let MessageSendEvent::UpdateHTLCs { updates, .. } = &msg_events[0] { + initiator.node.handle_commitment_signed(acceptor_node_id, &updates.commitment_signed[0]); + check_added_monitors(initiator, 1); + } else { + panic!("Unexpected event"); + } + + assert!(initiator.node.get_and_clear_pending_msg_events().is_empty()); + assert!(acceptor.node.get_and_clear_pending_msg_events().is_empty()); + + // Reestablishing before the monitor update completes should still not release `tx_signatures`. + initiator.node.peer_disconnected(acceptor_node_id); + acceptor.node.peer_disconnected(initiator_node_id); + let mut reconnect_args = ReconnectArgs::new(initiator, acceptor); + reconnect_args.send_announcement_sigs = (true, true); + reconnect_nodes(reconnect_args); + assert!(initiator.node.get_and_clear_pending_msg_events().is_empty()); + assert!(acceptor.node.get_and_clear_pending_msg_events().is_empty()); + + acceptor.chain_monitor.complete_sole_pending_chan_update(&channel_id); + chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::Completed); + + let tx_signatures = + get_event_msg!(acceptor, MessageSendEvent::SendTxSignatures, initiator_node_id); + initiator.node.handle_tx_signatures(acceptor_node_id, &tx_signatures); + + let tx_signatures = + get_event_msg!(initiator, MessageSendEvent::SendTxSignatures, acceptor_node_id); + acceptor.node.handle_tx_signatures(initiator_node_id, &tx_signatures); + + let _ = get_event!(initiator, Event::SplicePending); + let _ = get_event!(acceptor, Event::SplicePending); +} diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 9361cd3c749..cdd2db49ad7 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1189,7 +1189,7 @@ pub(super) struct MonitorRestoreUpdates { pub channel_ready: Option, pub channel_ready_order: ChannelReadyOrder, pub announcement_sigs: Option, - pub tx_signatures: Option, + pub funding_tx_signed: Option, /// The sources of outbound HTLCs that were forwarded and irrevocably committed on this channel /// (the outbound edge), along with their outbound amounts. Useful to store in the inbound HTLC /// to ensure it gets resolved. @@ -2165,9 +2165,6 @@ where }, }; - let channel_id = context.channel_id; - let counterparty_node_id = context.counterparty_node_id; - let signing_session = if let Some(signing_session) = context.interactive_tx_signing_session.as_mut() { @@ -2263,33 +2260,18 @@ where .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) + let mut funding_tx_signed = FundingTxSigned { + commitment_signed, + counterparty_initial_commitment_signed_result: None, + tx_signatures, + funding_tx: None, + splice_negotiated: None, + splice_locked: None, }; - let funding_tx = funding_tx.map(|tx| { - let tx_type = if splice_negotiated.is_some() { - TransactionType::Splice { counterparty_node_id, channel_id } - } else { - TransactionType::Funding { channels: vec![(counterparty_node_id, channel_id)] } - }; - (tx, tx_type) - }); - // 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 = + funding_tx_signed.counterparty_initial_commitment_signed_result = self.as_funded_mut().and_then(|funded_channel| { funded_channel .pending_splice @@ -2315,14 +2297,25 @@ where }) }); - Ok(FundingTxSigned { - commitment_signed, - counterparty_initial_commitment_signed_result, - tx_signatures, - funding_tx, - splice_negotiated, - splice_locked, - }) + // 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. + if let Some(funding_tx) = funding_tx { + debug_assert!(funding_tx_signed.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( + &mut funding_tx_signed, + funding_tx, + best_block_height, + &logger, + ) + }; + + Ok(funding_tx_signed) } pub fn force_shutdown(&mut self, closure_reason: ClosureReason) -> ShutdownResult { @@ -2402,6 +2395,7 @@ where .expect("We have a pending splice negotiated"); let funding_negotiation = pending_splice.funding_negotiation.as_mut() .expect("We have a pending splice negotiated"); + log_debug!(logger, "Stashing counterparty initial commitment_signed to process after funding_transaction_signed"); if let FundingNegotiation::AwaitingSignatures { ref mut initial_commitment_signed_from_counterparty, .. } = funding_negotiation { @@ -6515,6 +6509,7 @@ pub(super) struct TxCompleteResult { } /// The result of signing a funding transaction negotiated using the interactive-tx protocol. +#[derive(Default)] pub(super) struct FundingTxSigned { /// The initial `commitment_signed` message to send to the counterparty, if necessary. pub commitment_signed: Option, @@ -8780,9 +8775,9 @@ where } fn on_tx_signatures_exchange<'a, L: Logger>( - &mut self, funding_tx: Transaction, best_block_height: u32, - logger: &WithChannelContext<'a, L>, - ) -> (Option, Option) { + &mut self, funding_tx_signed: &mut FundingTxSigned, funding_tx: Transaction, + best_block_height: u32, logger: &WithChannelContext<'a, L>, + ) { debug_assert!(!self.context.channel_state.is_monitor_update_in_progress()); debug_assert!(!self.context.channel_state.is_awaiting_remote_revoke()); @@ -8791,7 +8786,7 @@ where if let Some(FundingNegotiation::AwaitingSignatures { mut funding, .. }) = pending_splice.funding_negotiation.take() { - funding.funding_transaction = Some(funding_tx); + funding.funding_transaction = Some(funding_tx.clone()); let funding_txo = funding.get_funding_txo().expect("funding outpoint should be set"); @@ -8821,16 +8816,24 @@ where ); } - (Some(splice_negotiated), splice_locked) + let tx_type = TransactionType::Splice { + counterparty_node_id: self.context.counterparty_node_id, + channel_id: self.context.channel_id, + }; + funding_tx_signed.funding_tx = Some((funding_tx, tx_type)); + funding_tx_signed.splice_negotiated = Some(splice_negotiated); + funding_tx_signed.splice_locked = splice_locked; } else { debug_assert!(false); - (None, None) } } else { - self.funding.funding_transaction = Some(funding_tx); + self.funding.funding_transaction = Some(funding_tx.clone()); self.context.channel_state = ChannelState::AwaitingChannelReady(AwaitingChannelReadyFlags::new()); - (None, None) + let tx_type = TransactionType::Funding { + channels: vec![(self.context.counterparty_node_id, self.context.channel_id)], + }; + funding_tx_signed.funding_tx = Some((funding_tx, tx_type)); } } @@ -8889,34 +8892,41 @@ where msg.tx_hash ); - let (splice_negotiated, splice_locked) = if let Some(funding_tx) = funding_tx.clone() { - self.on_tx_signatures_exchange(funding_tx, best_block_height, &logger) - } else { - (None, None) - }; - - let funding_tx = funding_tx.map(|tx| { - let tx_type = if splice_negotiated.is_some() { - TransactionType::Splice { - counterparty_node_id: self.context.counterparty_node_id, - channel_id: self.context.channel_id, - } - } else { - TransactionType::Funding { - channels: vec![(self.context.counterparty_node_id, self.context.channel_id)], - } - }; - (tx, tx_type) - }); - - Ok(FundingTxSigned { + let mut funding_tx_signed = FundingTxSigned { commitment_signed: None, counterparty_initial_commitment_signed_result: None, - tx_signatures: holder_tx_signatures, - funding_tx, - splice_negotiated, - splice_locked, - }) + tx_signatures: None, + funding_tx: None, + splice_negotiated: None, + splice_locked: None, + }; + if holder_tx_signatures.is_some() && self.is_awaiting_monitor_update() { + // Although the user may have already provided our `tx_signatures`, we must not send + // them if we're waiting for the monitor to durably persist the counterparty's signature + // for our initial commitment post-splice. + debug_assert!(self.context.monitor_pending_tx_signatures); + log_debug!( + logger, + "Waiting for async monitor update to complete prior to releasing our tx_signatures" + ); + return Ok(funding_tx_signed); + } + + funding_tx_signed.tx_signatures = holder_tx_signatures; + if let Some(funding_tx) = funding_tx { + self.on_tx_signatures_exchange( + &mut funding_tx_signed, + funding_tx, + best_block_height, + &logger, + ); + } else { + debug_assert!( + false, + "Signed funding transaction should be available upon tx_signatures exchange" + ); + } + Ok(funding_tx_signed) } /// Queues up an outbound update fee by placing it in the holding cell. You should call @@ -9097,8 +9107,8 @@ where /// successfully and we should restore normal operation. Returns messages which should be sent /// to the remote side. #[rustfmt::skip] - pub fn monitor_updating_restored( - &mut self, logger: &L, node_signer: &NS, chain_hash: ChainHash, + pub fn monitor_updating_restored<'a, L: Logger, NS: NodeSigner, CBP>( + &mut self, logger: &WithChannelContext<'a, L>, node_signer: &NS, chain_hash: ChainHash, user_config: &UserConfig, best_block_height: u32, path_for_release_htlc: CBP ) -> MonitorRestoreUpdates where @@ -9108,6 +9118,7 @@ where self.context.channel_state.clear_monitor_update_in_progress(); assert_eq!(self.blocked_monitor_updates_pending(), 0); + let mut funding_tx_signed = None; let mut tx_signatures = self .context .monitor_pending_tx_signatures @@ -9118,17 +9129,34 @@ where // 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; - - if self.context.signer_pending_funding { - tx_signatures.take(); - } - 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() - { + let is_waiting_for_counterparty_tx_signatures = + !signing_session.holder_sends_tx_signatures_first() + && !signing_session.has_received_tx_signatures(); + + if self.context.signer_pending_funding || is_waiting_for_counterparty_tx_signatures { tx_signatures.take(); + } else if !is_waiting_for_counterparty_tx_signatures { + debug_assert!(tx_signatures.is_some()); + funding_tx_signed = Some(FundingTxSigned { + commitment_signed: None, + counterparty_initial_commitment_signed_result: None, + tx_signatures, + funding_tx: None, + splice_negotiated: None, + splice_locked: None, + }); + if let Some(funding_tx) = signing_session.signed_tx() { + self.on_tx_signatures_exchange( + funding_tx_signed.as_mut().unwrap(), + funding_tx, + best_block_height, + logger, + ); + } else if signing_session.has_received_tx_signatures() { + debug_assert!(false, "Signed funding transaction should be available upon tx_signatures exchange"); + } } } @@ -9197,7 +9225,7 @@ where return MonitorRestoreUpdates { raa: None, commitment_update: None, commitment_order: RAACommitmentOrder::RevokeAndACKFirst, accepted_htlcs, failed_htlcs, finalized_claimed_htlcs, pending_update_adds, - funding_broadcastable, channel_ready, announcement_sigs, tx_signatures: None, + funding_broadcastable, channel_ready, announcement_sigs, funding_tx_signed: None, channel_ready_order, committed_outbound_htlc_sources }; } @@ -9228,7 +9256,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, + pending_update_adds, funding_broadcastable, channel_ready, announcement_sigs, funding_tx_signed, channel_ready_order, committed_outbound_htlc_sources } } @@ -9345,8 +9373,9 @@ where let tx_signatures = if funding_commit_sig.is_some() { if let Some(signing_session) = self.context.interactive_tx_signing_session.as_ref() { - let should_send_tx_signatures = signing_session.holder_sends_tx_signatures_first() - || signing_session.has_received_tx_signatures(); + let should_send_tx_signatures = !self.is_awaiting_monitor_update() + && (signing_session.holder_sends_tx_signatures_first() + || signing_session.has_received_tx_signatures()); should_send_tx_signatures .then(|| ()) .and_then(|_| signing_session.holder_tx_signatures().clone()) @@ -9776,6 +9805,8 @@ where log_debug!(logger, "Waiting for funding transaction signatures to be provided"); } else if self.context.channel_state.is_monitor_update_in_progress() { log_debug!(logger, "Waiting for monitor update before providing funding transaction signatures"); + } else if self.context.signer_pending_funding { + log_debug!(logger, "Waiting for signer to provide counterparty commitment_signed before releasing funding transaction signatures"); } else { tx_signatures = session.holder_tx_signatures().clone(); } @@ -15703,7 +15734,8 @@ mod tests { use crate::ln::channel::{ AwaitingChannelReadyFlags, ChannelState, FundedChannel, HTLCUpdateAwaitingACK, InboundHTLCOutput, InboundHTLCState, InboundUpdateAdd, InboundV1Channel, - OutboundHTLCOutput, OutboundHTLCState, OutboundV1Channel, MIN_THEIR_CHAN_RESERVE_SATOSHIS, + OutboundHTLCOutput, OutboundHTLCState, OutboundV1Channel, WithChannelContext, + MIN_THEIR_CHAN_RESERVE_SATOSHIS, }; use crate::ln::channel_keys::{RevocationBasepoint, RevocationKey}; use crate::ln::channelmanager::{self, HTLCSource, PaymentId}; @@ -18044,7 +18076,7 @@ mod tests { &&logger, ).map_err(|_| ()).unwrap(); let node_b_updates = node_b_chan.monitor_updating_restored( - &&logger, + &WithChannelContext::from(&logger, &node_b_chan.context, None), &&keys_provider, chain_hash, &config, @@ -18059,7 +18091,7 @@ mod tests { ); let (mut node_a_chan, _) = if let Ok(res) = res { res } else { panic!(); }; let node_a_updates = node_a_chan.monitor_updating_restored( - &&logger, + &WithChannelContext::from(&logger, &node_a_chan.context, None), &&keys_provider, chain_hash, &config, diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index ada27af749f..5578d16826f 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -10298,7 +10298,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ } else { log_debug!(logger, "Channel is open and awaiting update, resuming it"); let updates = chan.monitor_updating_restored( - &&logger, + &logger, &self.node_signer, self.chain_hash, &*self.config.read().unwrap(), @@ -10336,7 +10336,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ updates.funding_broadcastable, updates.channel_ready, updates.announcement_sigs, - updates.tx_signatures, + updates.funding_tx_signed, None, updates.channel_ready_order, ); @@ -10488,19 +10488,20 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ pending_forwards: Vec<(PendingHTLCInfo, u64)>, pending_update_adds: Vec, funding_broadcastable: Option, channel_ready: Option, announcement_sigs: Option, - tx_signatures: Option, tx_abort: Option, + mut funding_tx_signed: Option, tx_abort: Option, channel_ready_order: ChannelReadyOrder, ) -> (Vec, Option<(u64, Vec)>) { let logger = WithChannelContext::from(&self.logger, &channel.context, None); - log_trace!(logger, "Handling channel resumption with {} RAA, {} commitment update, {} pending forwards, {} pending update_add_htlcs, {}broadcasting funding, {} channel ready, {} announcement, {} tx_signatures, {} tx_abort", + log_trace!(logger, "Handling channel resumption with {} RAA, {} commitment update, {} pending forwards, {} pending update_add_htlcs, {}broadcasting funding, {} channel ready, {} announcement, {} tx_signatures, {} tx_abort, {} splice_locked", if raa.is_some() { "an" } else { "no" }, if commitment_update.is_some() { "a" } else { "no" }, pending_forwards.len(), pending_update_adds.len(), if funding_broadcastable.is_some() { "" } else { "not " }, if channel_ready.is_some() { "sending" } else { "without" }, if announcement_sigs.is_some() { "sending" } else { "without" }, - if tx_signatures.is_some() { "sending" } else { "without" }, + if funding_tx_signed.as_ref().map(|v| v.tx_signatures.is_some()).unwrap_or(false) { "sending" } else { "without" }, if tx_abort.is_some() { "sending" } else { "without" }, + if funding_tx_signed.as_ref().map(|v| v.splice_locked.is_some()).unwrap_or(false) { "sending" } else { "without" }, ); let counterparty_node_id = channel.context.get_counterparty_node_id(); @@ -10567,7 +10568,13 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ }, } - if let Some(msg) = tx_signatures { + if let Some(funding_tx_signed) = funding_tx_signed.as_ref() { + // These [`FundingTxSigned`] fields are only expected as a result of calling + // [`ChannelManager::funding_transaction_signed`]. + debug_assert!(funding_tx_signed.commitment_signed.is_none()); + debug_assert!(funding_tx_signed.counterparty_initial_commitment_signed_result.is_none()); + } + if let Some(msg) = funding_tx_signed.as_mut().and_then(|v| v.tx_signatures.take()) { pending_msg_events.push(MessageSendEvent::SendTxSignatures { node_id: counterparty_node_id, msg, @@ -10592,10 +10599,20 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ }); } } + + if let Some(msg) = funding_tx_signed.as_mut().and_then(|v| v.splice_locked.take()) { + pending_msg_events.push(MessageSendEvent::SendSpliceLocked { + node_id: counterparty_node_id, + msg, + }); + } } else if let Some(msg) = channel_ready { self.send_channel_ready(pending_msg_events, channel, msg); } + // If we just finished a pending interactive funding negotiation and are ready to broadcast + // the transaction, `funding_broadcastable` will only contain the transaction for a + // dual-funded channel. Splice transactions need to be broadcast separately. if let Some(tx) = funding_broadcastable { if channel.context.is_manual_broadcast() { log_info!(logger, "Not broadcasting funding transaction with txid {} as it is manually managed", tx.compute_txid()); @@ -10610,18 +10627,45 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ } }; } else { + if let Some((funding_tx, tx_type)) = funding_tx_signed.as_ref().and_then(|v| v.funding_tx.as_ref()) { + debug_assert_eq!(&tx, funding_tx); + debug_assert!(matches!(tx_type, TransactionType::Funding { .. })); + } log_info!(logger, "Broadcasting funding transaction with txid {}", tx.compute_txid()); self.tx_broadcaster.broadcast_transactions(&[( &tx, TransactionType::Funding { channels: vec![(counterparty_node_id, channel.context.channel_id())] }, )]); } + } else if let Some((splice_tx, tx_type)) = funding_tx_signed + .as_mut() + .and_then(|v| v.funding_tx.take()) + .filter(|(_, tx_type)| matches!(tx_type, TransactionType::Splice { .. })) + { + log_info!(logger, "Broadcasting signed splice transaction with txid {}", splice_tx.compute_txid()); + self.tx_broadcaster.broadcast_transactions(&[(&splice_tx, tx_type)]); } { let mut pending_events = self.pending_events.lock().unwrap(); emit_channel_pending_event!(pending_events, channel); emit_initial_channel_ready_event!(pending_events, channel); + if let Some(splice_negotiated) = funding_tx_signed + .as_mut() + .and_then(|v| v.splice_negotiated.take()) + { + pending_events.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, + )); + } } (htlc_forwards, decode_update_add_htlcs) @@ -12759,10 +12803,14 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ } } let need_lnd_workaround = chan.context.workaround_lnd_bug_4006.take(); + let funding_tx_signed = responses.tx_signatures.map(|tx_signatures| FundingTxSigned { + tx_signatures: Some(tx_signatures), + ..Default::default() + }); let (htlc_forwards, decode_update_add_htlcs) = self.handle_channel_resumption( &mut peer_state.pending_msg_events, chan, responses.raa, responses.commitment_update, responses.commitment_order, Vec::new(), Vec::new(), None, responses.channel_ready, responses.announcement_sigs, - responses.tx_signatures, responses.tx_abort, responses.channel_ready_order, + funding_tx_signed, responses.tx_abort, responses.channel_ready_order, ); debug_assert!(htlc_forwards.is_empty()); debug_assert!(decode_update_add_htlcs.is_none()); diff --git a/lightning/src/ln/interactivetxs.rs b/lightning/src/ln/interactivetxs.rs index f7e0ce34346..faa9aea0d94 100644 --- a/lightning/src/ln/interactivetxs.rs +++ b/lightning/src/ln/interactivetxs.rs @@ -637,7 +637,7 @@ impl InteractiveTxSigningSession { None }; - let funding_tx_opt = self.maybe_finalize_funding_tx(); + let funding_tx_opt = self.signed_tx(); Ok((holder_tx_signatures, funding_tx_opt)) } @@ -666,7 +666,7 @@ impl InteractiveTxSigningSession { self.holder_tx_signatures = Some(tx_signatures); - let funding_tx_opt = self.maybe_finalize_funding_tx(); + let funding_tx_opt = self.signed_tx(); let holder_tx_signatures = (self.has_received_commitment_signed && (self.holder_sends_tx_signatures_first || self.has_received_tx_signatures())) .then(|| { @@ -723,7 +723,9 @@ impl InteractiveTxSigningSession { }) } - fn maybe_finalize_funding_tx(&mut self) -> Option { + /// Returns `Some` with the fully signed transaction if both holder and counterparty signatures + /// are available. + pub fn signed_tx(&self) -> Option { let holder_tx_signatures = self.holder_tx_signatures.as_ref()?; let counterparty_tx_signatures = self.counterparty_tx_signatures.as_ref()?; let shared_input_signature = self.shared_input_signature.as_ref(); diff --git a/lightning/src/ln/splicing_tests.rs b/lightning/src/ln/splicing_tests.rs index 486e386be87..32b45996f11 100644 --- a/lightning/src/ln/splicing_tests.rs +++ b/lightning/src/ln/splicing_tests.rs @@ -3236,6 +3236,101 @@ fn test_splice_buffer_invalid_commitment_signed_closes_channel() { check_added_monitors(&nodes[0], 1); } +#[test] +fn test_splice_waits_for_initial_commitment_monitor_update_before_releasing_tx_signatures() { + // Test that if processing the counterparty's initial `commitment_signed` returns + // `ChannelMonitorUpdateStatus::InProgress`, we do not release our `tx_signatures` when their + // `tx_signatures` is received. We should only release ours once the monitor update completes. + 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 (_, _, channel_id, _) = + create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100_000, 0); + + let outputs = vec![TxOut { + value: Amount::from_sat(1_000), + script_pubkey: nodes[0].wallet_source.get_change_script().unwrap(), + }]; + let initiator_contribution = + initiate_splice_out(&nodes[0], &nodes[1], channel_id, outputs).unwrap(); + negotiate_splice_tx(&nodes[0], &nodes[1], channel_id, initiator_contribution); + + let signing_event = get_event!(nodes[0], Event::FundingTransactionReadyForSigning); + 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"); + } + + let initiator_commit_sig = get_htlc_update_msgs(&nodes[0], &node_id_1); + nodes[1].node.handle_commitment_signed(node_id_0, &initiator_commit_sig.commitment_signed[0]); + check_added_monitors(&nodes[1], 1); + + // Leave the monitor update for node 0's processing of the initial `commitment_signed` pending. + chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); + + let msg_events = nodes[1].node.get_and_clear_pending_msg_events(); + assert_eq!(msg_events.len(), 2, "{msg_events:?}"); + let counterparty_commit_sig = + if let MessageSendEvent::UpdateHTLCs { ref updates, .. } = &msg_events[0] { + updates.commitment_signed[0].clone() + } else { + panic!("Expected UpdateHTLCs message"); + }; + let counterparty_tx_signatures = + if let MessageSendEvent::SendTxSignatures { ref msg, .. } = &msg_events[1] { + msg.clone() + } else { + panic!("Expected SendTxSignatures message"); + }; + + nodes[0].node.handle_commitment_signed(node_id_1, &counterparty_commit_sig); + check_added_monitors(&nodes[0], 1); + assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); + + nodes[0].node.handle_tx_signatures(node_id_1, &counterparty_tx_signatures); + + // We should not send our `tx_signatures` while the monitor update is still in progress. + assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); + + // Reestablishing before the monitor update completes should still not release `tx_signatures`. + 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]); + reconnect_args.send_announcement_sigs = (true, true); + reconnect_nodes(reconnect_args); + assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); + assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); + + nodes[0].chain_monitor.complete_sole_pending_chan_update(&channel_id); + chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::Completed); + + let initiator_tx_signatures = + get_event_msg!(nodes[0], MessageSendEvent::SendTxSignatures, node_id_1); + nodes[1].node.handle_tx_signatures(node_id_0, &initiator_tx_signatures); + + expect_splice_pending_event(&nodes[0], &node_id_1); + expect_splice_pending_event(&nodes[1], &node_id_0); +} + #[test] fn test_splice_balance_falls_below_reserve() { // Test that we're able to proceed with a splice where the acceptor does not contribute