diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index 5b5c6391b4b..74f6e3c92cd 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -866,6 +866,7 @@ fn assert_action_timeout_awaiting_response(action: &msgs::ErrorAction) { action, msgs::ErrorAction::DisconnectPeerWithWarning { msg } if msg.data.contains("Disconnecting due to timeout awaiting response") + || msg.data.contains("already sent splice_locked, cannot RBF") )); } @@ -2023,7 +2024,7 @@ pub fn do_test(data: &[u8], out: Out) { ) .unwrap(); }, - events::Event::SplicePending { new_funding_txo, .. } => { + events::Event::SpliceNegotiated { new_funding_txo, .. } => { let broadcaster = match $node { 0 => &broadcast_a, 1 => &broadcast_b, @@ -2035,7 +2036,7 @@ pub fn do_test(data: &[u8], out: Out) { assert_eq!(new_funding_txo.txid, splice_tx.compute_txid()); chain_state.confirm_tx(splice_tx); }, - events::Event::SpliceFailed { .. } => {}, + events::Event::SpliceNegotiationFailed { .. } => {}, events::Event::DiscardFunding { funding_info: events::FundingInfo::Contribution { .. }, .. diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs index 1f1cf425c92..bf8230ceccf 100644 --- a/fuzz/src/full_stack.rs +++ b/fuzz/src/full_stack.rs @@ -1137,10 +1137,10 @@ pub fn do_test(mut data: &[u8], logger: &Arc signed_tx, ); }, - Event::SplicePending { .. } => { + Event::SpliceNegotiated { .. } => { // Splice negotiation completed, waiting for confirmation }, - Event::SpliceFailed { .. } => { + Event::SpliceNegotiationFailed { .. } => { // Splice failed, inputs can be re-spent }, Event::OpenChannelRequest { diff --git a/lightning/src/events/mod.rs b/lightning/src/events/mod.rs index 73c4a39c76f..5315eeece52 100644 --- a/lightning/src/events/mod.rs +++ b/lightning/src/events/mod.rs @@ -25,6 +25,7 @@ use crate::blinded_path::payment::{ use crate::chain::transaction; use crate::ln::channel::FUNDING_CONF_DEADLINE_BLOCKS; use crate::ln::channelmanager::{InterceptId, PaymentId}; +use crate::ln::funding::FundingContribution; use crate::ln::msgs; use crate::ln::onion_utils::LocalHTLCFailureReason; use crate::ln::outbound_payment::RecipientOnionFields; @@ -99,6 +100,117 @@ impl_writeable_tlv_based_enum!(FundingInfo, } ); +/// The reason a funding negotiation round failed. +/// +/// Each negotiation attempt (initial or RBF) resolves to either success or failure. This enum +/// indicates what caused the failure. Use [`is_retriable`] to determine whether the splice can +/// be reattempted on this channel by calling [`ChannelManager::splice_channel`]. +/// +/// [`is_retriable`]: Self::is_retriable +/// [`ChannelManager::splice_channel`]: crate::ln::channelmanager::ChannelManager::splice_channel +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum NegotiationFailureReason { + /// The reason was not available (e.g., from an older serialization). + Unknown, + /// The peer disconnected during negotiation. Wait for the peer to reconnect, then retry. + PeerDisconnected, + /// The counterparty explicitly aborted the negotiation by sending `tx_abort`. Retrying with + /// the same parameters is unlikely to succeed — consider adjusting the contribution or + /// waiting for the counterparty to initiate. + CounterpartyAborted { + /// The counterparty's abort message. + /// + /// This is counterparty-provided data. Use `Display` on [`UntrustedString`] for safe + /// logging. + msg: UntrustedString, + }, + /// An error occurred during interactive transaction negotiation (e.g., the counterparty sent + /// an invalid message). The negotiation was aborted. + NegotiationError { + /// A developer-readable error message. + msg: String, + }, + /// The funding contribution was invalid (e.g., insufficient balance for the splice amount). + /// Call [`ChannelManager::splice_channel`] for a fresh [`FundingTemplate`] and build a new + /// contribution with adjusted parameters. + /// + /// [`ChannelManager::splice_channel`]: crate::ln::channelmanager::ChannelManager::splice_channel + /// [`FundingTemplate`]: crate::ln::funding::FundingTemplate + ContributionInvalid, + /// The negotiation was locally abandoned via `ChannelManager::abandon_splice`. + LocallyAbandoned, + /// The channel is closing, so the negotiation cannot continue. See [`Event::ChannelClosed`] + /// for the closure reason. + ChannelClosing, + /// The contribution's feerate was too low for RBF. Call [`ChannelManager::splice_channel`] + /// for a fresh [`FundingTemplate`] (which includes the updated minimum feerate) and build a + /// new contribution with a higher feerate. + /// + /// [`ChannelManager::splice_channel`]: crate::ln::channelmanager::ChannelManager::splice_channel + /// [`FundingTemplate`]: crate::ln::funding::FundingTemplate + FeeRateTooLow, + /// An RBF attempt could not be initiated (e.g., a prior splice transaction already + /// confirmed). The channel remains operational — start a new splice with + /// [`ChannelManager::splice_channel`] if further changes are needed. + /// + /// [`ChannelManager::splice_channel`]: crate::ln::channelmanager::ChannelManager::splice_channel + CannotInitiateRbf, +} + +impl NegotiationFailureReason { + /// Whether the splice negotiation is likely to succeed if retried on this channel. When `true`, + /// call [`ChannelManager::splice_channel`] to obtain a fresh [`FundingTemplate`] and retry. + /// + /// [`ChannelManager::splice_channel`]: crate::ln::channelmanager::ChannelManager::splice_channel + /// [`FundingTemplate`]: crate::ln::funding::FundingTemplate + pub fn is_retriable(&self) -> bool { + match self { + Self::Unknown + | Self::PeerDisconnected + | Self::CounterpartyAborted { .. } + | Self::NegotiationError { .. } + | Self::ContributionInvalid + | Self::FeeRateTooLow => true, + Self::LocallyAbandoned | Self::ChannelClosing | Self::CannotInitiateRbf => false, + } + } +} + +impl core::fmt::Display for NegotiationFailureReason { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + match self { + Self::Unknown => f.write_str("unknown reason"), + Self::PeerDisconnected => f.write_str("peer disconnected during negotiation"), + Self::CounterpartyAborted { msg } => { + write!(f, "counterparty aborted: {}", msg) + }, + Self::NegotiationError { msg } => write!(f, "negotiation error: {}", msg), + Self::ContributionInvalid => f.write_str("funding contribution was invalid"), + Self::LocallyAbandoned => f.write_str("splice locally abandoned"), + + Self::ChannelClosing => f.write_str("channel is closing"), + Self::FeeRateTooLow => f.write_str("feerate too low for RBF"), + Self::CannotInitiateRbf => f.write_str("cannot initiate RBF"), + } + } +} + +impl_writeable_tlv_based_enum_upgradable!(NegotiationFailureReason, + (0, Unknown) => {}, + (2, PeerDisconnected) => {}, + (4, CounterpartyAborted) => { + (1, msg, required), + }, + (6, NegotiationError) => { + (1, msg, required), + }, + (8, ContributionInvalid) => {}, + (10, LocallyAbandoned) => {}, + (12, ChannelClosing) => {}, + (14, FeeRateTooLow) => {}, + (16, CannotInitiateRbf) => {}, +); + /// Some information provided on receipt of payment depends on whether the payment received is a /// spontaneous payment or a "conventional" lightning payment that's paying an invoice. #[derive(Clone, Debug, PartialEq, Eq)] @@ -1541,8 +1653,8 @@ pub enum Event { /// # Failure Behavior and Persistence /// This event will eventually be replayed after failures-to-handle (i.e., the event handler /// returning `Err(ReplayEvent ())`) and will be persisted across restarts. - SplicePending { - /// The `channel_id` of the channel that has a pending splice funding transaction. + SpliceNegotiated { + /// The `channel_id` of the channel with the negotiated splice funding transaction. channel_id: ChannelId, /// The `user_channel_id` value passed in to [`ChannelManager::create_channel`] for outbound /// channels, or to [`ChannelManager::accept_inbound_channel`] for inbound channels. @@ -1560,19 +1672,20 @@ pub enum Event { /// The witness script that is used to lock the channel's funding output to commitment transactions. new_funding_redeem_script: ScriptBuf, }, - /// Used to indicate that a splice for the given `channel_id` has failed. + /// Used to indicate that a splice negotiation round for the given `channel_id` has failed. /// - /// This event may be emitted if a splice fails after it has been initiated but prior to signing - /// any negotiated funding transaction. + /// Each splice attempt (initial or RBF) resolves to either [`Event::SpliceNegotiated`] on + /// success or this event on failure. Prior successfully negotiated splice transactions are + /// unaffected. /// - /// Any UTXOs contributed to be spent by the funding transaction may be reused and will be - /// given in `contributed_inputs`. + /// Any UTXOs contributed to the failed round that are not committed to a prior negotiated + /// splice transaction will be returned via a preceding [`Event::DiscardFunding`]. /// /// # Failure Behavior and Persistence /// This event will eventually be replayed after failures-to-handle (i.e., the event handler /// returning `Err(ReplayEvent ())`) and will be persisted across restarts. - SpliceFailed { - /// The `channel_id` of the channel for which the splice failed. + SpliceNegotiationFailed { + /// The `channel_id` of the channel for which the splice negotiation round failed. channel_id: ChannelId, /// The `user_channel_id` value passed in to [`ChannelManager::create_channel`] for outbound /// channels, or to [`ChannelManager::accept_inbound_channel`] for inbound channels. @@ -1582,10 +1695,17 @@ pub enum Event { user_channel_id: u128, /// The `node_id` of the channel counterparty. counterparty_node_id: PublicKey, - /// The outpoint of the channel's splice funding transaction, if one was created. - abandoned_funding_txo: Option, - /// The features that this channel will operate with, if available. - channel_type: Option, + /// The reason the splice negotiation failed. + reason: NegotiationFailureReason, + /// The funding contribution from the failed negotiation round, if available. This can be + /// fed back to [`ChannelManager::funding_contributed`] to retry with the same parameters. + /// Alternatively, call [`ChannelManager::splice_channel`] to obtain a fresh + /// [`FundingTemplate`] and build a new contribution. + /// + /// [`ChannelManager::funding_contributed`]: crate::ln::channelmanager::ChannelManager::funding_contributed + /// [`ChannelManager::splice_channel`]: crate::ln::channelmanager::ChannelManager::splice_channel + /// [`FundingTemplate`]: crate::ln::funding::FundingTemplate + contribution: Option, }, /// Used to indicate to the user that they can abandon the funding transaction and recycle the /// inputs for another purpose. @@ -2355,7 +2475,7 @@ impl Writeable for Event { // We never write out FundingTransactionReadyForSigning events as they will be regenerated when // necessary. }, - &Event::SplicePending { + &Event::SpliceNegotiated { ref channel_id, ref user_channel_id, ref counterparty_node_id, @@ -2373,20 +2493,20 @@ impl Writeable for Event { (11, new_funding_redeem_script, required), }); }, - &Event::SpliceFailed { + &Event::SpliceNegotiationFailed { ref channel_id, ref user_channel_id, ref counterparty_node_id, - ref abandoned_funding_txo, - ref channel_type, + ref reason, + ref contribution, } => { 52u8.write(writer)?; write_tlv_fields!(writer, { (1, channel_id, required), - (3, channel_type, option), (5, user_channel_id, required), (7, counterparty_node_id, required), - (9, abandoned_funding_txo, option), + (11, reason, required), + (13, contribution, option), }); }, // Note that, going forward, all new events must only write data inside of @@ -3012,7 +3132,7 @@ impl MaybeReadable for Event { (11, new_funding_redeem_script, required), }); - Ok(Some(Event::SplicePending { + Ok(Some(Event::SpliceNegotiated { channel_id: channel_id.0.unwrap(), user_channel_id: user_channel_id.0.unwrap(), counterparty_node_id: counterparty_node_id.0.unwrap(), @@ -3027,18 +3147,18 @@ impl MaybeReadable for Event { let mut f = || { _init_and_read_len_prefixed_tlv_fields!(reader, { (1, channel_id, required), - (3, channel_type, option), (5, user_channel_id, required), (7, counterparty_node_id, required), - (9, abandoned_funding_txo, option), + (11, reason, upgradable_option), + (13, contribution, option), }); - Ok(Some(Event::SpliceFailed { + Ok(Some(Event::SpliceNegotiationFailed { channel_id: channel_id.0.unwrap(), user_channel_id: user_channel_id.0.unwrap(), counterparty_node_id: counterparty_node_id.0.unwrap(), - abandoned_funding_txo, - channel_type, + reason: reason.unwrap_or(NegotiationFailureReason::Unknown), + contribution, })) }; f() diff --git a/lightning/src/ln/async_signer_tests.rs b/lightning/src/ln/async_signer_tests.rs index f238c1db060..ae73dd830ac 100644 --- a/lightning/src/ln/async_signer_tests.rs +++ b/lightning/src/ln/async_signer_tests.rs @@ -1647,8 +1647,8 @@ fn test_async_splice_initial_commit_sig() { 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); + let _ = get_event!(initiator, Event::SpliceNegotiated); + let _ = get_event!(acceptor, Event::SpliceNegotiated); } #[test] @@ -1739,6 +1739,6 @@ fn test_async_splice_initial_commit_sig_waits_for_monitor_before_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); + let _ = get_event!(initiator, Event::SpliceNegotiated); + let _ = get_event!(acceptor, Event::SpliceNegotiated); } diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 10801edef01..0b810bda38b 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -36,7 +36,7 @@ use crate::chain::channelmonitor::{ }; use crate::chain::transaction::{OutPoint, TransactionData}; use crate::chain::BestBlock; -use crate::events::{ClosureReason, FundingInfo}; +use crate::events::{ClosureReason, FundingInfo, NegotiationFailureReason}; use crate::ln::chan_utils; use crate::ln::chan_utils::{ get_commitment_transaction_number_obscure_factor, max_htlcs, second_stage_tx_fees_sat, @@ -1170,7 +1170,7 @@ pub(super) struct InteractiveTxMsgError { /// The underlying error. pub(super) err: ChannelError, /// If a splice was in progress when processing the message, this contains the splice funding - /// information for emitting a `SpliceFailed` event. + /// information for emitting a `SpliceNegotiationFailed` event. pub(super) splice_funding_failed: Option, /// Whether we were quiescent when we received the message, and are no longer due to aborting /// the session. @@ -1258,7 +1258,7 @@ pub(crate) struct ShutdownResult { pub(crate) channel_funding_txo: Option, pub(crate) last_local_balance_msat: u64, /// If a splice was in progress when the channel was shut down, this contains - /// the splice funding information for emitting a SpliceFailed event. + /// the splice funding information for emitting a SpliceNegotiationFailed event. pub(crate) splice_funding_failed: Option, } @@ -1266,7 +1266,7 @@ pub(crate) struct ShutdownResult { pub(crate) struct DisconnectResult { pub(crate) is_resumable: bool, /// If a splice was in progress when the channel was shut down, this contains - /// the splice funding information for emitting a SpliceFailed event. + /// the splice funding information for emitting a SpliceNegotiationFailed event. pub(crate) splice_funding_failed: Option, } @@ -3174,7 +3174,17 @@ pub(crate) enum QuiescentAction { pub(super) enum QuiescentError { DoNothing, DiscardFunding { inputs: Vec, outputs: Vec }, - FailSplice(SpliceFundingFailed), + FailSplice(SpliceFundingFailed, NegotiationFailureReason), +} + +impl QuiescentError { + fn with_negotiation_failure_reason(mut self, reason: NegotiationFailureReason) -> Self { + match self { + QuiescentError::FailSplice(_, ref mut r) => *r = reason, + _ => debug_assert!(false, "Expected FailSplice variant"), + } + self + } } pub(crate) enum StfuResponse { @@ -6850,13 +6860,6 @@ impl FundingNegotiationContext { } } - fn into_contributed_inputs_and_outputs(self) -> (Vec, Vec) { - let contributed_inputs = - self.our_funding_inputs.into_iter().map(|input| input.utxo.outpoint).collect(); - let contributed_outputs = self.our_funding_outputs; - (contributed_inputs, contributed_outputs) - } - fn contributed_inputs(&self) -> impl Iterator + '_ { self.our_funding_inputs.iter().map(|input| input.utxo.outpoint) } @@ -6864,10 +6867,6 @@ impl FundingNegotiationContext { fn contributed_outputs(&self) -> impl Iterator + '_ { self.our_funding_outputs.iter() } - - fn to_contributed_inputs_and_outputs(&self) -> (Vec, Vec) { - (self.contributed_inputs().collect(), self.contributed_outputs().cloned().collect()) - } } // Holder designates channel data owned for the benefit of the user client. @@ -7018,72 +7017,56 @@ pub struct SpliceFundingNegotiated { /// Information about a splice funding negotiation that has failed. pub struct SpliceFundingFailed { - /// The outpoint of the channel's splice funding transaction, if one was created. - pub funding_txo: Option, - - /// The features that this channel will operate with, if available. - pub channel_type: Option, - /// UTXOs spent as inputs contributed to the splice transaction. - pub contributed_inputs: Vec, + contributed_inputs: Vec, /// Outputs contributed to the splice transaction. - pub contributed_outputs: Vec, -} - -macro_rules! maybe_create_splice_funding_failed { - ($funded_channel: expr, $pending_splice: expr, $pending_splice_ref: expr, $get: ident, $contributed_inputs_and_outputs: ident) => {{ - $pending_splice - .and_then(|pending_splice| pending_splice.funding_negotiation.$get()) - .and_then(|funding_negotiation| { - let is_initiator = funding_negotiation.is_initiator(); + contributed_outputs: Vec, - let funding_txo = funding_negotiation - .as_funding() - .and_then(|funding| funding.get_funding_txo()) - .map(|txo| txo.into_bitcoin_outpoint()); - - let channel_type = funding_negotiation - .as_funding() - .map(|funding| funding.get_channel_type().clone()); - - let (mut contributed_inputs, mut contributed_outputs) = match funding_negotiation { - FundingNegotiation::AwaitingAck { context, .. } => { - context.$contributed_inputs_and_outputs() - }, - FundingNegotiation::ConstructingTransaction { - interactive_tx_constructor, - .. - } => interactive_tx_constructor.$contributed_inputs_and_outputs(), - FundingNegotiation::AwaitingSignatures { .. } => $funded_channel - .context - .interactive_tx_signing_session - .$get() - .expect("We have a pending splice awaiting signatures") - .$contributed_inputs_and_outputs(), - }; - - if let Some(pending_splice) = $pending_splice_ref { - for input in pending_splice.prior_contributed_inputs() { - contributed_inputs.retain(|i| *i != input); - } - for output in pending_splice.prior_contributed_outputs() { - contributed_outputs.retain(|o| o.script_pubkey != output.script_pubkey); - } - } - - if !is_initiator && contributed_inputs.is_empty() && contributed_outputs.is_empty() - { - return None; - } + /// The funding contribution from the failed round, if available. + contribution: Option, +} - Some(SpliceFundingFailed { - funding_txo, - channel_type, - contributed_inputs, - contributed_outputs, +impl SpliceFundingFailed { + /// Splits into the funding info for `DiscardFunding` (if there are inputs or outputs to + /// discard) and the contribution for `SpliceNegotiationFailed`. + pub(super) fn into_parts(self) -> (Option, Option) { + let funding_info = + if !self.contributed_inputs.is_empty() || !self.contributed_outputs.is_empty() { + Some(FundingInfo::Contribution { + inputs: self.contributed_inputs, + outputs: self.contributed_outputs, }) - }) + } else { + None + }; + (funding_info, self.contribution) + } +} + +macro_rules! splice_funding_failed_for { + ($self: expr, $is_initiator: expr, $contribution: expr, + $contributed_inputs: ident, $contributed_outputs: ident) => {{ + let contribution = $contribution; + let existing_inputs = + $self.pending_splice.as_ref().into_iter().flat_map(|ps| ps.$contributed_inputs()); + let existing_outputs = + $self.pending_splice.as_ref().into_iter().flat_map(|ps| ps.$contributed_outputs()); + let filtered = + contribution.clone().into_unique_contributions(existing_inputs, existing_outputs); + match filtered { + None if !$is_initiator => None, + None => Some(SpliceFundingFailed { + contributed_inputs: vec![], + contributed_outputs: vec![], + contribution: Some(contribution), + }), + Some((contributed_inputs, contributed_outputs)) => Some(SpliceFundingFailed { + contributed_inputs, + contributed_outputs, + contribution: Some(contribution), + }), + } }}; } @@ -7113,28 +7096,24 @@ where /// Builds a [`SpliceFundingFailed`] from a contribution, filtering out inputs/outputs /// that are still committed to a prior splice round. fn splice_funding_failed_for(&self, contribution: FundingContribution) -> SpliceFundingFailed { - let (mut inputs, mut outputs) = contribution.into_contributed_inputs_and_outputs(); - if let Some(ref pending_splice) = self.pending_splice { - for input in pending_splice.contributed_inputs() { - inputs.retain(|i| *i != input); - } - for output in pending_splice.contributed_outputs() { - outputs.retain(|o| o.script_pubkey != output.script_pubkey); - } - } - SpliceFundingFailed { - funding_txo: None, - channel_type: None, - contributed_inputs: inputs, - contributed_outputs: outputs, - } + // The contribution was never pushed to `contributions`, so `contributed_inputs()` and + // `contributed_outputs()` return only prior rounds' entries for filtering. + splice_funding_failed_for!( + self, + true, + contribution, + contributed_inputs, + contributed_outputs + ) + .expect("is_initiator is true so this always returns Some") } fn quiescent_action_into_error(&self, action: QuiescentAction) -> QuiescentError { match action { - QuiescentAction::Splice { contribution, .. } => { - QuiescentError::FailSplice(self.splice_funding_failed_for(contribution)) - }, + QuiescentAction::Splice { contribution, .. } => QuiescentError::FailSplice( + self.splice_funding_failed_for(contribution), + NegotiationFailureReason::Unknown, + ), #[cfg(any(test, fuzzing, feature = "_test_utils"))] QuiescentAction::DoNothing => QuiescentError::DoNothing, } @@ -7143,7 +7122,7 @@ where fn abandon_quiescent_action(&mut self) -> Option { let action = self.quiescent_action.take()?; match self.quiescent_action_into_error(action) { - QuiescentError::FailSplice(failed) => Some(failed), + QuiescentError::FailSplice(failed, _) => Some(failed), #[cfg(any(test, fuzzing, feature = "_test_utils"))] QuiescentError::DoNothing => None, _ => { @@ -7270,30 +7249,44 @@ where ); } - let splice_funding_failed = maybe_create_splice_funding_failed!( - self, - self.pending_splice.as_mut(), - self.pending_splice.as_ref(), - take, - into_contributed_inputs_and_outputs + // Take the funding negotiation and pop the current round's contribution, if any + // (acceptors may not have one). + let pending_splice = self + .pending_splice + .as_mut() + .expect("reset_pending_splice_state requires pending_splice"); + debug_assert!( + pending_splice.funding_negotiation.is_some(), + "reset_pending_splice_state requires an active funding negotiation" ); - - // Pop the current round's contribution if it wasn't from a negotiated round. Each round - // pushes a new entry to `contributions`; if the round aborts, we undo the push so that - // `contributions.last()` reflects the most recent negotiated round's contribution. This - // must happen after `maybe_create_splice_funding_failed` so that - // `prior_contributed_inputs` still includes the prior rounds' entries for filtering. - if let Some(pending_splice) = self.pending_splice.as_mut() { - if let Some(last) = pending_splice.contributions.last() { - let was_negotiated = pending_splice + let is_initiator = pending_splice + .funding_negotiation + .take() + .map(|negotiation| negotiation.is_initiator()) + .unwrap_or(false); + let contribution = pending_splice.contributions.pop(); + if let Some(ref contribution) = contribution { + debug_assert!( + pending_splice .last_funding_feerate_sat_per_1000_weight - .is_some_and(|f| last.feerate() == FeeRate::from_sat_per_kwu(f as u64)); - if !was_negotiated { - pending_splice.contributions.pop(); - } - } + .map(|f| contribution.feerate() > FeeRate::from_sat_per_kwu(f as u64)) + .unwrap_or(true), + "current round's feerate should be greater than the last negotiated feerate", + ); } + // After pop, `contributed_inputs()` / `contributed_outputs()` return only prior + // rounds for filtering. + let splice_funding_failed = contribution.and_then(|contribution| { + splice_funding_failed_for!( + self, + is_initiator, + contribution, + contributed_inputs, + contributed_outputs + ) + }); + if self.pending_funding().is_empty() { self.pending_splice.take(); } @@ -7311,12 +7304,23 @@ where return None; } - maybe_create_splice_funding_failed!( + let pending_splice = self.pending_splice.as_ref()?; + debug_assert!( + pending_splice.funding_negotiation.is_some(), + "maybe_splice_funding_failed requires an active funding negotiation" + ); + let is_initiator = pending_splice + .funding_negotiation + .as_ref() + .map(|negotiation| negotiation.is_initiator()) + .unwrap_or(false); + let contribution = pending_splice.contributions.last().cloned()?; + splice_funding_failed_for!( self, - self.pending_splice.as_ref(), - self.pending_splice.as_ref(), - as_ref, - to_contributed_inputs_and_outputs + is_initiator, + contribution, + prior_contributed_inputs, + prior_contributed_outputs ) } @@ -11601,7 +11605,6 @@ where .iter_mut() .find(|funding| funding.get_funding_txid() == Some(splice_txid)) .unwrap(); - let prev_funding_txid = self.funding.get_funding_txid(); if let Some(scid) = self.funding.short_channel_id { self.context.historical_scids.push(scid); @@ -11609,22 +11612,21 @@ where core::mem::swap(&mut self.funding, funding); - // The swap above places the previous `FundingScope` into `pending_funding`. - pending_splice - .negotiated_candidates - .drain(..) - .filter(|funding| funding.get_funding_txid() != prev_funding_txid) - .map(|mut funding| { - funding - .funding_transaction - .take() - .map(|tx| FundingInfo::Tx { transaction: tx }) - .unwrap_or_else(|| FundingInfo::OutPoint { - outpoint: funding - .get_funding_txo() - .expect("Negotiated splices must have a known funding outpoint"), - }) + let promoted_tx = self + .funding + .funding_transaction + .as_ref() + .expect("Promoted splice funding should have a funding transaction"); + let contributions = core::mem::take(&mut pending_splice.contributions); + contributions + .into_iter() + .filter_map(|contribution| { + contribution.into_unique_contributions( + promoted_tx.input.iter().map(|i| i.previous_output), + promoted_tx.output.iter(), + ) }) + .map(|(inputs, outputs)| FundingInfo::Contribution { inputs, outputs }) .collect::>() }; @@ -12345,7 +12347,7 @@ where // // If the in-progress negotiation later fails (e.g., tx_abort), the derived // min_rbf_feerate becomes stale, causing a slightly higher feerate than - // necessary. Call splice_channel again after receiving SpliceFailed to get a + // necessary. Call splice_channel again after receiving SpliceNegotiationFailed to get a // fresh template without the stale RBF constraint. let prev_feerate = pending_splice.last_funding_feerate_sat_per_1000_weight.or_else(|| { @@ -12577,7 +12579,10 @@ where self.validate_splice_contributions(our_funding_contribution, SignedAmount::ZERO) { log_error!(logger, "Channel {} cannot be funded: {}", self.context.channel_id(), e); - return Err(QuiescentError::FailSplice(self.splice_funding_failed_for(contribution))); + return Err(QuiescentError::FailSplice( + self.splice_funding_failed_for(contribution), + NegotiationFailureReason::ContributionInvalid, + )); } if let Some(pending_splice) = self.pending_splice.as_ref() { @@ -12593,6 +12598,7 @@ where ); return Err(QuiescentError::FailSplice( self.splice_funding_failed_for(contribution), + NegotiationFailureReason::FeeRateTooLow, )); } } @@ -14034,9 +14040,18 @@ where ) -> Result, QuiescentError> { log_debug!(logger, "Attempting to initiate quiescence"); + // TODO: NegotiationFailureReason is splice-specific, but propose_quiescence is + // generic. The reason should be selected by the caller, but it currently can't + // distinguish why quiescence failed. Revisit when a second quiescent protocol is added. if !self.context.is_usable() { + debug_assert!( + self.context.channel_state.is_local_shutdown_sent() + || self.context.channel_state.is_remote_shutdown_sent(), + "splice_channel should have prevented reaching propose_quiescence on a non-ready channel" + ); log_debug!(logger, "Channel is not in a usable state to propose quiescence"); - return Err(self.quiescent_action_into_error(action)); + return Err(self.quiescent_action_into_error(action) + .with_negotiation_failure_reason(NegotiationFailureReason::ChannelClosing)); } if self.quiescent_action.is_some() { log_debug!( @@ -14151,7 +14166,10 @@ where self.context.channel_id(), e, )), - QuiescentError::FailSplice(failed), + QuiescentError::FailSplice( + failed, + NegotiationFailureReason::ContributionInvalid, + ), )); } let prior_contribution = contribution.clone(); @@ -14171,6 +14189,16 @@ where }; if self.pending_splice.is_some() { + if let Err(e) = self.can_initiate_rbf() { + let failed = self.splice_funding_failed_for(prior_contribution); + return Err(( + ChannelError::WarnAndDisconnect(e), + QuiescentError::FailSplice( + failed, + NegotiationFailureReason::CannotInitiateRbf, + ), + )); + } let tx_init_rbf = self.send_tx_init_rbf(context); self.pending_splice.as_mut().unwrap() .contributions.push(prior_contribution); diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 73d9a67f50f..4ebd0c65f66 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -61,8 +61,8 @@ use crate::ln::channel::QuiescentError; use crate::ln::channel::{ self, hold_time_since, Channel, ChannelError, ChannelUpdateStatus, DisconnectResult, FundedChannel, FundingTxSigned, InboundV1Channel, InteractiveTxMsgError, OutboundHop, - OutboundV1Channel, PendingV2Channel, ReconnectionMsg, ShutdownResult, SpliceFundingFailed, - StfuResponse, UpdateFulfillCommitFetch, WithChannelContext, + OutboundV1Channel, PendingV2Channel, ReconnectionMsg, ShutdownResult, StfuResponse, + UpdateFulfillCommitFetch, WithChannelContext, }; use crate::ln::channel_state::ChannelDetails; use crate::ln::funding::{FundingContribution, FundingTemplate}; @@ -4173,24 +4173,24 @@ impl< failed_htlcs = htlcs; if let Some(splice_funding_failed) = splice_funding_failed { + let (funding_info, contribution) = splice_funding_failed.into_parts(); let mut pending_events = self.pending_events.lock().unwrap(); + if let Some(funding_info) = funding_info { + pending_events.push_back(( + events::Event::DiscardFunding { + channel_id: *chan_id, + funding_info, + }, + None, + )); + } pending_events.push_back(( - events::Event::SpliceFailed { + events::Event::SpliceNegotiationFailed { channel_id: *chan_id, counterparty_node_id: *counterparty_node_id, user_channel_id: chan.context().get_user_id(), - abandoned_funding_txo: splice_funding_failed.funding_txo, - channel_type: splice_funding_failed.channel_type, - }, - None, - )); - pending_events.push_back(( - events::Event::DiscardFunding { - channel_id: *chan_id, - funding_info: FundingInfo::Contribution { - inputs: splice_funding_failed.contributed_inputs, - outputs: splice_funding_failed.contributed_outputs, - }, + contribution, + reason: events::NegotiationFailureReason::ChannelClosing, }, None, )); @@ -4480,23 +4480,23 @@ impl< )); if let Some(splice_funding_failed) = shutdown_res.splice_funding_failed.take() { + let (funding_info, contribution) = splice_funding_failed.into_parts(); + if let Some(funding_info) = funding_info { + pending_events.push_back(( + events::Event::DiscardFunding { + channel_id: shutdown_res.channel_id, + funding_info, + }, + None, + )); + } pending_events.push_back(( - events::Event::SpliceFailed { + events::Event::SpliceNegotiationFailed { channel_id: shutdown_res.channel_id, counterparty_node_id: shutdown_res.counterparty_node_id, user_channel_id: shutdown_res.user_channel_id, - abandoned_funding_txo: splice_funding_failed.funding_txo, - channel_type: splice_funding_failed.channel_type, - }, - None, - )); - pending_events.push_back(( - events::Event::DiscardFunding { - channel_id: shutdown_res.channel_id, - funding_info: FundingInfo::Contribution { - inputs: splice_funding_failed.contributed_inputs, - outputs: splice_funding_failed.contributed_outputs, - }, + contribution, + reason: events::NegotiationFailureReason::ChannelClosing, }, None, )); @@ -4985,24 +4985,24 @@ impl< }); if let Some(splice_funding_failed) = splice_funding_failed { + let (funding_info, contribution) = splice_funding_failed.into_parts(); let pending_events = &mut self.pending_events.lock().unwrap(); + if let Some(funding_info) = funding_info { + pending_events.push_back(( + events::Event::DiscardFunding { + channel_id: *channel_id, + funding_info, + }, + None, + )); + } pending_events.push_back(( - events::Event::SpliceFailed { + events::Event::SpliceNegotiationFailed { channel_id: *channel_id, counterparty_node_id: *counterparty_node_id, user_channel_id: chan.context.get_user_id(), - abandoned_funding_txo: splice_funding_failed.funding_txo, - channel_type: splice_funding_failed.channel_type, - }, - None, - )); - pending_events.push_back(( - events::Event::DiscardFunding { - channel_id: *channel_id, - funding_info: FundingInfo::Contribution { - inputs: splice_funding_failed.contributed_inputs, - outputs: splice_funding_failed.contributed_outputs, - }, + contribution, + reason: events::NegotiationFailureReason::LocallyAbandoned, }, None, )); @@ -6685,35 +6685,25 @@ impl< )); } }, - QuiescentError::FailSplice(SpliceFundingFailed { - funding_txo, - channel_type, - contributed_inputs, - contributed_outputs, - }) => { + QuiescentError::FailSplice(splice_funding_failed, reason) => { + let (funding_info, contribution) = splice_funding_failed.into_parts(); let pending_events = &mut self.pending_events.lock().unwrap(); + if let Some(funding_info) = funding_info { + pending_events.push_back(( + events::Event::DiscardFunding { channel_id, funding_info }, + None, + )); + } pending_events.push_back(( - events::Event::SpliceFailed { + events::Event::SpliceNegotiationFailed { channel_id, counterparty_node_id, user_channel_id, - abandoned_funding_txo: funding_txo, - channel_type, + reason, + contribution, }, None, )); - if !contributed_inputs.is_empty() || !contributed_outputs.is_empty() { - pending_events.push_back(( - events::Event::DiscardFunding { - channel_id, - funding_info: FundingInfo::Contribution { - inputs: contributed_inputs, - outputs: contributed_outputs, - }, - }, - None, - )); - } }, } } @@ -6763,14 +6753,14 @@ impl< /// # Events /// /// Calling this method will commence the process of creating a new funding transaction for the - /// channel. Once the funding transaction has been constructed, an [`Event::SplicePending`] + /// channel. Once the funding transaction has been constructed, an [`Event::SpliceNegotiated`] /// will be emitted. At this point, any inputs contributed to the splice can only be re-spent /// if an [`Event::DiscardFunding`] is seen. /// - /// If any failures occur while negotiating the funding transaction, an [`Event::SpliceFailed`] - /// will be emitted. Any contributed inputs no longer used will be included in an - /// [`Event::DiscardFunding`] and thus can be re-spent. If a [`FundingTemplate`] was obtained - /// while a previous splice was still being negotiated, its + /// If any failures occur while negotiating the funding transaction, an + /// [`Event::SpliceNegotiationFailed`] will be emitted. Any contributed inputs no longer used + /// will be included in an [`Event::DiscardFunding`] and thus can be re-spent. If a + /// [`FundingTemplate`] was obtained while a previous splice was still being negotiated, its /// [`min_rbf_feerate`][FundingTemplate::min_rbf_feerate] may be stale after the failure. /// Call [`ChannelManager::splice_channel`] again to get a fresh template. /// @@ -6852,7 +6842,7 @@ impl< "Channel {} already has a pending funding contribution", channel_id, ), - QuiescentError::FailSplice(_) => format!( + QuiescentError::FailSplice(..) => format!( "Channel {} cannot accept funding contribution", channel_id, ), @@ -6989,7 +6979,7 @@ impl< } if let Some(splice_negotiated) = splice_negotiated { self.pending_events.lock().unwrap().push_back(( - events::Event::SplicePending { + events::Event::SpliceNegotiated { channel_id: *channel_id, counterparty_node_id: *counterparty_node_id, user_channel_id: chan.context().get_user_id(), @@ -11151,7 +11141,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ .and_then(|v| v.splice_negotiated.take()) { pending_events.push_back(( - events::Event::SplicePending { + events::Event::SpliceNegotiated { channel_id: channel.context.channel_id(), counterparty_node_id, user_channel_id: channel.context.get_user_id(), @@ -12007,23 +11997,22 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ exited_quiescence, }) => { if let Some(splice_funding_failed) = splice_funding_failed { + let (funding_info, contribution) = splice_funding_failed.into_parts(); let pending_events = &mut self.pending_events.lock().unwrap(); + if let Some(funding_info) = funding_info { + pending_events.push_back(( + events::Event::DiscardFunding { channel_id, funding_info }, + None, + )); + } pending_events.push_back(( - events::Event::SpliceFailed { + events::Event::SpliceNegotiationFailed { channel_id, counterparty_node_id: *counterparty_node_id, user_channel_id: channel.context().get_user_id(), - abandoned_funding_txo: splice_funding_failed.funding_txo, - channel_type: splice_funding_failed.channel_type.clone(), - }, - None, - )); - pending_events.push_back(( - events::Event::DiscardFunding { - channel_id, - funding_info: FundingInfo::Contribution { - inputs: splice_funding_failed.contributed_inputs, - outputs: splice_funding_failed.contributed_outputs, + contribution, + reason: events::NegotiationFailureReason::NegotiationError { + msg: format!("{:?}", err), }, }, None, @@ -12166,23 +12155,25 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ exited_quiescence, }) => { if let Some(splice_funding_failed) = splice_funding_failed { + let (funding_info, contribution) = splice_funding_failed.into_parts(); let pending_events = &mut self.pending_events.lock().unwrap(); + if let Some(funding_info) = funding_info { + pending_events.push_back(( + events::Event::DiscardFunding { + channel_id: msg.channel_id, + funding_info, + }, + None, + )); + } pending_events.push_back(( - events::Event::SpliceFailed { + events::Event::SpliceNegotiationFailed { channel_id: msg.channel_id, counterparty_node_id, user_channel_id: chan.context().get_user_id(), - abandoned_funding_txo: splice_funding_failed.funding_txo, - channel_type: splice_funding_failed.channel_type.clone(), - }, - None, - )); - pending_events.push_back(( - events::Event::DiscardFunding { - channel_id: msg.channel_id, - funding_info: FundingInfo::Contribution { - inputs: splice_funding_failed.contributed_inputs, - outputs: splice_funding_failed.contributed_outputs, + contribution, + reason: events::NegotiationFailureReason::NegotiationError { + msg: format!("{:?}", err), }, }, None, @@ -12265,7 +12256,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ let exited_quiescence = splice_negotiated.is_some(); if let Some(splice_negotiated) = splice_negotiated { self.pending_events.lock().unwrap().push_back(( - events::Event::SplicePending { + events::Event::SpliceNegotiated { channel_id: msg.channel_id, counterparty_node_id: *counterparty_node_id, user_channel_id: chan.context.get_user_id(), @@ -12336,23 +12327,27 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ } if let Some(splice_funding_failed) = splice_failed { + let (funding_info, contribution) = splice_funding_failed.into_parts(); let pending_events = &mut self.pending_events.lock().unwrap(); + if let Some(funding_info) = funding_info { + pending_events.push_back(( + events::Event::DiscardFunding { + channel_id: msg.channel_id, + funding_info, + }, + None, + )); + } pending_events.push_back(( - events::Event::SpliceFailed { + events::Event::SpliceNegotiationFailed { channel_id: msg.channel_id, counterparty_node_id: *counterparty_node_id, user_channel_id: chan_entry.get().context().get_user_id(), - abandoned_funding_txo: splice_funding_failed.funding_txo, - channel_type: splice_funding_failed.channel_type, - }, - None, - )); - pending_events.push_back(( - events::Event::DiscardFunding { - channel_id: msg.channel_id, - funding_info: FundingInfo::Contribution { - inputs: splice_funding_failed.contributed_inputs, - outputs: splice_funding_failed.contributed_outputs, + contribution, + reason: events::NegotiationFailureReason::CounterpartyAborted { + msg: UntrustedString( + String::from_utf8_lossy(&msg.data).to_string(), + ), }, }, None, @@ -12484,24 +12479,24 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ dropped_htlcs = htlcs; if let Some(splice_funding_failed) = splice_funding_failed { + let (funding_info, contribution) = splice_funding_failed.into_parts(); let mut pending_events = self.pending_events.lock().unwrap(); + if let Some(funding_info) = funding_info { + pending_events.push_back(( + events::Event::DiscardFunding { + channel_id: msg.channel_id, + funding_info, + }, + None, + )); + } pending_events.push_back(( - events::Event::SpliceFailed { + events::Event::SpliceNegotiationFailed { channel_id: msg.channel_id, counterparty_node_id: *counterparty_node_id, user_channel_id: chan.context().get_user_id(), - abandoned_funding_txo: splice_funding_failed.funding_txo, - channel_type: splice_funding_failed.channel_type, - }, - None, - )); - pending_events.push_back(( - events::Event::DiscardFunding { - channel_id: msg.channel_id, - funding_info: FundingInfo::Contribution { - inputs: splice_funding_failed.contributed_inputs, - outputs: splice_funding_failed.contributed_outputs, - }, + contribution, + reason: events::NegotiationFailureReason::ChannelClosing, }, None, )); @@ -15263,6 +15258,22 @@ impl< self.process_pending_events(&event_handler); let collected_events = events.into_inner(); + // When both DiscardFunding and SpliceNegotiationFailed are emitted for the same + // channel, DiscardFunding must come first so that inputs are unlocked before any + // retry. Each pair is emitted adjacently under a single lock, so checking + // adjacent events is sufficient. + for window in collected_events.windows(2) { + if let events::Event::SpliceNegotiationFailed { channel_id, .. } = &window[0] { + if let events::Event::DiscardFunding { channel_id: cid, .. } = &window[1] { + assert!( + channel_id != cid, + "DiscardFunding must precede SpliceNegotiationFailed for channel {}", + channel_id, + ); + } + } + } + // To expand the coverage and make sure all events are properly serialised and deserialised, // we test all generated events round-trip: for event in &collected_events { @@ -15538,19 +15549,19 @@ impl< chan.peer_disconnected_is_resumable(&&logger); if let Some(splice_funding_failed) = splice_funding_failed { - splice_failed_events.push(events::Event::SpliceFailed { + let (funding_info, contribution) = splice_funding_failed.into_parts(); + if let Some(funding_info) = funding_info { + splice_failed_events.push(events::Event::DiscardFunding { + channel_id: chan.context().channel_id(), + funding_info, + }); + } + splice_failed_events.push(events::Event::SpliceNegotiationFailed { channel_id: chan.context().channel_id(), counterparty_node_id, user_channel_id: chan.context().get_user_id(), - abandoned_funding_txo: splice_funding_failed.funding_txo, - channel_type: splice_funding_failed.channel_type, - }); - splice_failed_events.push(events::Event::DiscardFunding { - channel_id: chan.context().channel_id(), - funding_info: FundingInfo::Contribution { - inputs: splice_funding_failed.contributed_inputs, - outputs: splice_funding_failed.contributed_outputs, - }, + contribution, + reason: events::NegotiationFailureReason::PeerDisconnected, }); } @@ -18157,31 +18168,32 @@ impl< let our_pending_intercepts = self.pending_intercepted_htlcs.lock().unwrap(); // Since some FundingNegotiation variants are not persisted, any splice in such state must - // be failed upon reload. However, as the necessary information for the SpliceFailed and - // DiscardFunding events is not persisted, the events need to be persisted even though they + // be failed upon reload. However, as the necessary information for the + // SpliceNegotiationFailed and DiscardFunding events is not persisted, the events need to + // be persisted even though they // haven't been emitted yet. These are removed after the events are written. let mut events = self.pending_events.lock().unwrap(); let event_count = events.len(); for peer_state in peer_states.iter() { for chan in peer_state.channel_by_id.values().filter_map(Channel::as_funded) { if let Some(splice_funding_failed) = chan.maybe_splice_funding_failed() { + let (funding_info, contribution) = splice_funding_failed.into_parts(); + if let Some(funding_info) = funding_info { + events.push_back(( + events::Event::DiscardFunding { + channel_id: chan.context().channel_id(), + funding_info, + }, + None, + )); + } events.push_back(( - events::Event::SpliceFailed { + events::Event::SpliceNegotiationFailed { channel_id: chan.context.channel_id(), counterparty_node_id: chan.context.get_counterparty_node_id(), user_channel_id: chan.context.get_user_id(), - abandoned_funding_txo: splice_funding_failed.funding_txo, - channel_type: splice_funding_failed.channel_type, - }, - None, - )); - events.push_back(( - events::Event::DiscardFunding { - channel_id: chan.context().channel_id(), - funding_info: FundingInfo::Contribution { - inputs: splice_funding_failed.contributed_inputs, - outputs: splice_funding_failed.contributed_outputs, - }, + reason: events::NegotiationFailureReason::PeerDisconnected, + contribution, }, None, )); @@ -18305,7 +18317,7 @@ impl< (23, self.best_block.read().unwrap().previous_blocks, required), }); - // Remove the SpliceFailed and DiscardFunding events added earlier. + // Remove the SpliceNegotiationFailed and DiscardFunding events added earlier. events.truncate(event_count); Ok(()) diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index c1923730a3d..7d1f34bca5c 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -19,8 +19,8 @@ use crate::chain::{BestBlock, ChannelMonitorUpdateStatus, Confirm, Listen, Watch use crate::events::bump_transaction::sync::BumpTransactionEventHandlerSync; use crate::events::bump_transaction::BumpTransactionEvent; use crate::events::{ - ClaimedHTLC, ClosureReason, Event, FundingInfo, HTLCHandlingFailureType, PaidBolt12Invoice, - PathFailure, PaymentFailureReason, PaymentPurpose, + ClaimedHTLC, ClosureReason, Event, FundingInfo, HTLCHandlingFailureType, + NegotiationFailureReason, PaidBolt12Invoice, PathFailure, PaymentFailureReason, PaymentPurpose, }; use crate::ln::chan_utils::{ commitment_tx_base_weight, COMMITMENT_TX_WEIGHT_PER_HTLC, TRUC_MAX_WEIGHT, @@ -2378,7 +2378,7 @@ pub fn check_closed_events(node: &Node, expected_close_events: &[ExpectedCloseEv discard_events_count ); assert_eq!( - events.iter().filter(|e| matches!(e, Event::SpliceFailed { .. },)).count(), + events.iter().filter(|e| matches!(e, Event::SpliceNegotiationFailed { .. },)).count(), splice_events_count ); } @@ -3221,7 +3221,7 @@ pub fn expect_splice_pending_event<'a, 'b, 'c, 'd>( let events = node.node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); match &events[0] { - crate::events::Event::SplicePending { channel_id, counterparty_node_id, .. } => { + crate::events::Event::SpliceNegotiated { channel_id, counterparty_node_id, .. } => { assert_eq!(*expected_counterparty_node_id, *counterparty_node_id); *channel_id }, @@ -3232,21 +3232,15 @@ pub fn expect_splice_pending_event<'a, 'b, 'c, 'd>( #[cfg(any(test, ldk_bench, feature = "_test_utils"))] pub fn expect_splice_failed_events<'a, 'b, 'c, 'd>( node: &'a Node<'b, 'c, 'd>, expected_channel_id: &ChannelId, - funding_contribution: FundingContribution, + funding_contribution: FundingContribution, expected_reason: NegotiationFailureReason, ) { let events = node.node.get_and_clear_pending_events(); assert_eq!(events.len(), 2); match &events[0] { - Event::SpliceFailed { channel_id, .. } => { - assert_eq!(*expected_channel_id, *channel_id); - }, - _ => panic!("Unexpected event"), - } - match &events[1] { Event::DiscardFunding { funding_info, .. } => { if let FundingInfo::Contribution { inputs, outputs } = &funding_info { let (expected_inputs, expected_outputs) = - funding_contribution.into_contributed_inputs_and_outputs(); + funding_contribution.clone().into_contributed_inputs_and_outputs(); assert_eq!(*inputs, expected_inputs); assert_eq!(*outputs, expected_outputs); } else { @@ -3255,6 +3249,14 @@ pub fn expect_splice_failed_events<'a, 'b, 'c, 'd>( }, _ => panic!("Unexpected event"), } + match &events[1] { + Event::SpliceNegotiationFailed { channel_id, reason, contribution, .. } => { + assert_eq!(*expected_channel_id, *channel_id); + assert_eq!(expected_reason, *reason); + assert_eq!(contribution.as_ref(), Some(&funding_contribution)); + }, + _ => panic!("Unexpected event"), + } } #[cfg(any(test, ldk_bench, feature = "_test_utils"))] diff --git a/lightning/src/ln/funding.rs b/lightning/src/ln/funding.rs index 20366fe772a..d673ce1b02f 100644 --- a/lightning/src/ln/funding.rs +++ b/lightning/src/ln/funding.rs @@ -121,10 +121,10 @@ pub enum FundingContributionError { /// /// Note: [`FundingTemplate::min_rbf_feerate`] may be derived from an in-progress /// negotiation that later aborts, leaving a stale (higher than necessary) minimum. If - /// this error occurs after receiving [`Event::SpliceFailed`], call + /// this error occurs after receiving [`Event::SpliceNegotiationFailed`], call /// [`ChannelManager::splice_channel`] again to get a fresh template. /// - /// [`Event::SpliceFailed`]: crate::events::Event::SpliceFailed + /// [`Event::SpliceNegotiationFailed`]: crate::events::Event::SpliceNegotiationFailed /// [`ChannelManager::splice_channel`]: crate::ln::channelmanager::ChannelManager::splice_channel FeeRateBelowRbfMinimum { /// The requested feerate. @@ -578,7 +578,8 @@ impl_writeable_tlv_based!(FundingContribution, { }); impl FundingContribution { - pub(super) fn feerate(&self) -> FeeRate { + /// Returns the feerate of this contribution. + pub fn feerate(&self) -> FeeRate { self.feerate } @@ -610,6 +611,11 @@ impl FundingContribution { .unwrap_or(Amount::ZERO) } + /// Returns the inputs included in this contribution. + pub fn inputs(&self) -> &[FundingTxInput] { + &self.inputs + } + /// Returns the outputs (e.g., withdrawal destinations) included in this contribution. /// /// This does not include the change output; see [`FundingContribution::change_output`]. @@ -749,7 +755,7 @@ impl FundingContribution { inputs.retain(|input| *input != existing); } for existing in existing_outputs { - outputs.retain(|output| *output != *existing); + outputs.retain(|output| output.script_pubkey != existing.script_pubkey); } if inputs.is_empty() && outputs.is_empty() { None diff --git a/lightning/src/ln/interactivetxs.rs b/lightning/src/ln/interactivetxs.rs index ca8c4450012..10dae95cefa 100644 --- a/lightning/src/ln/interactivetxs.rs +++ b/lightning/src/ln/interactivetxs.rs @@ -90,13 +90,6 @@ impl SerialIdExt for SerialId { } } -#[derive(Clone, Debug)] -pub(crate) struct NegotiationError { - pub reason: AbortReason, - pub contributed_inputs: Vec, - pub contributed_outputs: Vec, -} - #[derive(Debug, Clone, Copy, PartialEq)] pub(crate) enum AbortReason { InvalidStateTransition, @@ -370,11 +363,6 @@ impl ConstructedTransaction { Ok(tx) } - fn into_negotiation_error(self, reason: AbortReason) -> NegotiationError { - let (contributed_inputs, contributed_outputs) = self.into_contributed_inputs_and_outputs(); - NegotiationError { reason, contributed_inputs, contributed_outputs } - } - fn contributed_inputs(&self) -> impl Iterator + '_ { self.tx .input @@ -401,40 +389,6 @@ impl ConstructedTransaction { .map(|(_, (txout, _))| txout) } - fn to_contributed_inputs_and_outputs(&self) -> (Vec, Vec) { - (self.contributed_inputs().collect(), self.contributed_outputs().cloned().collect()) - } - - fn into_contributed_inputs_and_outputs(self) -> (Vec, Vec) { - let contributed_inputs = self - .tx - .input - .into_iter() - .zip(self.input_metadata.iter()) - .enumerate() - .filter(|(_, (_, input))| input.is_local(self.holder_is_initiator)) - .filter(|(index, _)| { - self.shared_input_index - .map(|shared_index| *index != shared_index as usize) - .unwrap_or(true) - }) - .map(|(_, (txin, _))| txin.previous_output) - .collect(); - - let contributed_outputs = self - .tx - .output - .into_iter() - .zip(self.output_metadata.iter()) - .enumerate() - .filter(|(_, (_, output))| output.is_local(self.holder_is_initiator)) - .filter(|(index, _)| *index != self.shared_output_index as usize) - .map(|(_, (txout, _))| txout) - .collect(); - - (contributed_inputs, contributed_outputs) - } - pub fn tx(&self) -> &Transaction { &self.tx } @@ -921,10 +875,6 @@ impl InteractiveTxSigningSession { Ok(()) } - pub(crate) fn into_negotiation_error(self, reason: AbortReason) -> NegotiationError { - self.unsigned_tx.into_negotiation_error(reason) - } - pub(super) fn contributed_inputs(&self) -> impl Iterator + '_ { self.unsigned_tx.contributed_inputs() } @@ -932,14 +882,6 @@ impl InteractiveTxSigningSession { pub(super) fn contributed_outputs(&self) -> impl Iterator + '_ { self.unsigned_tx.contributed_outputs() } - - pub(super) fn to_contributed_inputs_and_outputs(&self) -> (Vec, Vec) { - (self.contributed_inputs().collect(), self.contributed_outputs().cloned().collect()) - } - - pub(super) fn into_contributed_inputs_and_outputs(self) -> (Vec, Vec) { - self.unsigned_tx.into_contributed_inputs_and_outputs() - } } impl_writeable_tlv_based!(InteractiveTxSigningSession, { @@ -2172,27 +2114,6 @@ impl InteractiveTxConstructor { Self::new(args, false) } - fn into_negotiation_error(self, reason: AbortReason) -> NegotiationError { - let (contributed_inputs, contributed_outputs) = self.into_contributed_inputs_and_outputs(); - NegotiationError { reason, contributed_inputs, contributed_outputs } - } - - pub(super) fn into_contributed_inputs_and_outputs(self) -> (Vec, Vec) { - let contributed_inputs = self - .inputs_to_contribute - .into_iter() - .filter(|(_, input)| !input.is_shared()) - .map(|(_, input)| input.into_tx_in().previous_output) - .collect(); - let contributed_outputs = self - .outputs_to_contribute - .into_iter() - .filter(|(_, output)| !output.is_shared()) - .map(|(_, output)| output.into_tx_out()) - .collect(); - (contributed_inputs, contributed_outputs) - } - pub(super) fn contributed_inputs(&self) -> impl Iterator + '_ { self.inputs_to_contribute .iter() @@ -2207,10 +2128,6 @@ impl InteractiveTxConstructor { .map(|(_, output)| output.tx_out()) } - pub(super) fn to_contributed_inputs_and_outputs(&self) -> (Vec, Vec) { - (self.contributed_inputs().collect(), self.contributed_outputs().cloned().collect()) - } - pub fn is_initiator(&self) -> bool { self.is_initiator } diff --git a/lightning/src/ln/splicing_tests.rs b/lightning/src/ln/splicing_tests.rs index fa22ccb61c7..2f5e25d3ffa 100644 --- a/lightning/src/ln/splicing_tests.rs +++ b/lightning/src/ln/splicing_tests.rs @@ -13,7 +13,9 @@ use crate::chain::chaininterface::{TransactionType, FEERATE_FLOOR_SATS_PER_KW}; use crate::chain::channelmonitor::{ANTI_REORG_DELAY, LATENCY_GRACE_PERIOD_BLOCKS}; use crate::chain::transaction::OutPoint; use crate::chain::ChannelMonitorUpdateStatus; -use crate::events::{ClosureReason, Event, FundingInfo, HTLCHandlingFailureType}; +use crate::events::{ + ClosureReason, Event, FundingInfo, HTLCHandlingFailureType, NegotiationFailureReason, +}; use crate::ln::chan_utils; use crate::ln::channel::{ CHANNEL_ANNOUNCEMENT_PROPAGATION_DELAY, FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE, @@ -26,6 +28,7 @@ use crate::ln::outbound_payment::RecipientOnionFields; use crate::ln::types::ChannelId; use crate::routing::router::{PaymentParameters, RouteParameters}; use crate::types::features::ChannelTypeFeatures; +use crate::types::string::UntrustedString; use crate::util::config::UserConfig; use crate::util::errors::APIError; use crate::util::ser::Writeable; @@ -205,8 +208,12 @@ pub fn do_initiate_rbf_splice_in<'a, 'b, 'c, 'd>( let node_id_counterparty = counterparty.node.get_our_node_id(); let funding_template = node.node.splice_channel(&channel_id, &node_id_counterparty).unwrap(); let wallet = WalletSync::new(Arc::clone(&node.wallet_source), node.logger); - let funding_contribution = - funding_template.splice_in_sync(value_added, feerate, FeeRate::MAX, &wallet).unwrap(); + let funding_contribution = funding_template + .without_prior_contribution(feerate, FeeRate::MAX) + .with_coin_selection_source_sync(&wallet) + .add_value(value_added) + .build() + .unwrap(); node.node .funding_contributed(&channel_id, &node_id_counterparty, funding_contribution.clone(), None) .unwrap(); @@ -250,7 +257,12 @@ pub fn initiate_splice_out<'a, 'b, 'c, 'd>( ) { Ok(()) => Ok(funding_contribution), Err(e) => { - expect_splice_failed_events(initiator, &channel_id, funding_contribution); + expect_splice_failed_events( + initiator, + &channel_id, + funding_contribution, + NegotiationFailureReason::ContributionInvalid, + ); Err(e) }, } @@ -639,9 +651,15 @@ pub fn splice_channel<'a, 'b, 'c, 'd>( (splice_tx, new_funding_script) } +pub struct SpliceLockedResult { + pub stfu: Option, + pub node_a_discarded: Vec<(Vec, Vec)>, + pub node_b_discarded: Vec<(Vec, Vec)>, +} + pub fn lock_splice_after_blocks<'a, 'b, 'c, 'd>( node_a: &'a Node<'b, 'c, 'd>, node_b: &'a Node<'b, 'c, 'd>, num_blocks: u32, -) -> Option { +) -> SpliceLockedResult { connect_blocks(node_a, num_blocks); connect_blocks(node_b, num_blocks); @@ -654,7 +672,7 @@ pub fn lock_splice_after_blocks<'a, 'b, 'c, 'd>( pub fn lock_splice<'a, 'b, 'c, 'd>( node_a: &'a Node<'b, 'c, 'd>, node_b: &'a Node<'b, 'c, 'd>, splice_locked_for_node_b: &msgs::SpliceLocked, is_0conf: bool, expected_discard_txids: &[Txid], -) -> Option { +) -> SpliceLockedResult { let prev_funding_txid = node_a .chain_monitor .chain_monitor @@ -691,29 +709,23 @@ pub fn lock_splice<'a, 'b, 'c, 'd>( } } - let mut all_discard_txids = Vec::new(); - let expected_num_events = 1 + expected_discard_txids.len(); - for node in [node_a, node_b] { + let mut node_a_discarded = Vec::new(); + let mut node_b_discarded = Vec::new(); + for (idx, node) in [node_a, node_b].into_iter().enumerate() { let events = node.node.get_and_clear_pending_events(); - assert_eq!(events.len(), expected_num_events, "{events:?}"); + assert!(!events.is_empty(), "Expected at least ChannelReady, got {events:?}"); assert!(matches!(events[0], Event::ChannelReady { .. })); - let discard_txids: Vec<_> = events[1..] - .iter() - .map(|e| match e { - Event::DiscardFunding { funding_info: FundingInfo::Tx { transaction }, .. } => { - transaction.compute_txid() - }, + let discarded = if idx == 0 { &mut node_a_discarded } else { &mut node_b_discarded }; + for event in &events[1..] { + match event { Event::DiscardFunding { - funding_info: FundingInfo::OutPoint { outpoint }, .. - } => outpoint.txid, - other => panic!("Expected DiscardFunding, got {:?}", other), - }) - .collect(); - for txid in expected_discard_txids { - assert!(discard_txids.contains(txid), "Missing DiscardFunding for txid {}", txid); - } - if all_discard_txids.is_empty() { - all_discard_txids = discard_txids; + funding_info: FundingInfo::Contribution { inputs, outputs }, + .. + } => { + discarded.push((inputs.clone(), outputs.clone())); + }, + other => panic!("Expected DiscardFunding with Contribution, got {:?}", other), + } } check_added_monitors(node, 1); } @@ -751,18 +763,18 @@ pub fn lock_splice<'a, 'b, 'c, 'd>( // old funding as it is no longer being tracked. for node in [node_a, node_b] { node.chain_source.remove_watched_by_txid(prev_funding_txid); - for txid in &all_discard_txids { + for txid in expected_discard_txids { node.chain_source.remove_watched_by_txid(*txid); } } - node_a_stfu.or(node_b_stfu) + SpliceLockedResult { stfu: node_a_stfu.or(node_b_stfu), node_a_discarded, node_b_discarded } } pub fn lock_rbf_splice_after_blocks<'a, 'b, 'c, 'd>( node_a: &'a Node<'b, 'c, 'd>, node_b: &'a Node<'b, 'c, 'd>, tx: &Transaction, num_blocks: u32, expected_discard_txids: &[Txid], -) -> Option { +) -> SpliceLockedResult { mine_transaction(node_a, tx); mine_transaction(node_b, tx); @@ -860,7 +872,12 @@ fn do_test_splice_state_reset_on_disconnect(reload: bool) { nodes[1].node.peer_disconnected(node_id_0); } - expect_splice_failed_events(&nodes[0], &channel_id, funding_contribution); + expect_splice_failed_events( + &nodes[0], + &channel_id, + funding_contribution, + NegotiationFailureReason::PeerDisconnected, + ); let mut reconnect_args = ReconnectArgs::new(&nodes[0], &nodes[1]); reconnect_args.send_channel_ready = (true, true); @@ -911,7 +928,12 @@ fn do_test_splice_state_reset_on_disconnect(reload: bool) { nodes[1].node.peer_disconnected(node_id_0); } - expect_splice_failed_events(&nodes[0], &channel_id, funding_contribution); + expect_splice_failed_events( + &nodes[0], + &channel_id, + funding_contribution, + NegotiationFailureReason::PeerDisconnected, + ); let mut reconnect_args = ReconnectArgs::new(&nodes[0], &nodes[1]); reconnect_args.send_channel_ready = (true, true); @@ -993,7 +1015,14 @@ fn do_test_splice_state_reset_on_disconnect(reload: bool) { let tx_abort = get_event_msg!(nodes[0], MessageSendEvent::SendTxAbort, node_id_1); nodes[1].node.handle_tx_abort(node_id_0, &tx_abort); - expect_splice_failed_events(&nodes[0], &channel_id, funding_contribution); + expect_splice_failed_events( + &nodes[0], + &channel_id, + funding_contribution, + NegotiationFailureReason::CounterpartyAborted { + msg: UntrustedString("No active signing session. The associated funding transaction may have already been broadcast.".to_string()), + }, + ); // Attempt a splice negotiation that completes, (i.e. `tx_signatures` are exchanged). Reconnecting // should not abort the negotiation or reset the splice state. @@ -1077,7 +1106,12 @@ fn test_config_reject_inbound_splices() { nodes[0].node.peer_disconnected(node_id_1); nodes[1].node.peer_disconnected(node_id_0); - expect_splice_failed_events(&nodes[0], &channel_id, funding_contribution); + expect_splice_failed_events( + &nodes[0], + &channel_id, + funding_contribution, + NegotiationFailureReason::PeerDisconnected, + ); let mut reconnect_args = ReconnectArgs::new(&nodes[0], &nodes[1]); reconnect_args.send_channel_ready = (true, true); @@ -1331,7 +1365,7 @@ fn fails_initiating_concurrent_splices(reconnect: bool) { mine_transaction(&nodes[0], &splice_tx); mine_transaction(&nodes[1], &splice_tx); - let stfu = lock_splice_after_blocks(&nodes[0], &nodes[1], ANTI_REORG_DELAY - 1); + let stfu = lock_splice_after_blocks(&nodes[0], &nodes[1], ANTI_REORG_DELAY - 1).stfu; // Node 0 had called splice_channel (line above) but never funding_contributed, so no stfu // is expected from node 0 at this point. assert!(stfu.is_none()); @@ -1359,7 +1393,7 @@ fn test_initiating_splice_holds_stfu_with_pending_splice() { // Mine and lock the splice. mine_transaction(&nodes[0], &splice_tx); mine_transaction(&nodes[1], &splice_tx); - let stfu = lock_splice_after_blocks(&nodes[0], &nodes[1], 5); + let stfu = lock_splice_after_blocks(&nodes[0], &nodes[1], 5).stfu; assert!(stfu.is_none()); } @@ -1595,7 +1629,7 @@ fn do_test_splice_tiebreak( mine_transaction(&nodes[1], &tx); // After splice_locked, node 1's preserved QuiescentAction triggers STFU for retry. - let node_1_stfu = lock_splice_after_blocks(&nodes[0], &nodes[1], ANTI_REORG_DELAY - 1); + let node_1_stfu = lock_splice_after_blocks(&nodes[0], &nodes[1], ANTI_REORG_DELAY - 1).stfu; let stfu_1 = if let Some(MessageSendEvent::SendStfu { msg, .. }) = node_1_stfu { assert!(msg.initiator); msg @@ -2454,7 +2488,14 @@ fn fail_splice_on_interactive_tx_error() { get_event_msg!(acceptor, MessageSendEvent::SendTxComplete, node_id_initiator); initiator.node.handle_tx_add_input(node_id_acceptor, &tx_add_input); - expect_splice_failed_events(initiator, &channel_id, funding_contribution); + expect_splice_failed_events( + initiator, + &channel_id, + funding_contribution, + NegotiationFailureReason::NegotiationError { + msg: "Abort: Parity for `serial_id` was incorrect".to_string(), + }, + ); // We exit quiescence upon sending `tx_abort`, so we should see the holding cell be immediately // freed. @@ -2525,7 +2566,12 @@ fn fail_splice_on_tx_abort() { let tx_abort = get_event_msg!(acceptor, MessageSendEvent::SendTxAbort, node_id_initiator); initiator.node.handle_tx_abort(node_id_acceptor, &tx_abort); - expect_splice_failed_events(initiator, &channel_id, funding_contribution); + expect_splice_failed_events( + initiator, + &channel_id, + funding_contribution, + NegotiationFailureReason::CounterpartyAborted { msg: UntrustedString(String::new()) }, + ); // We exit quiescence upon receiving `tx_abort`, so we should see our `tx_abort` echo and the // holding cell be immediately freed. @@ -2621,7 +2667,16 @@ fn fail_splice_on_tx_complete_error() { }; initiator.node.handle_tx_abort(node_id_acceptor, tx_abort); - expect_splice_failed_events(initiator, &channel_id, funding_contribution); + expect_splice_failed_events( + initiator, + &channel_id, + funding_contribution, + NegotiationFailureReason::CounterpartyAborted { + msg: UntrustedString( + "Total value of outputs exceeds total value of inputs".to_string(), + ), + }, + ); let tx_abort = get_event_msg!(initiator, MessageSendEvent::SendTxAbort, node_id_acceptor); acceptor.node.handle_tx_abort(node_id_initiator, &tx_abort); @@ -2895,7 +2950,7 @@ fn do_abandon_splice_quiescent_action_on_shutdown(local_shutdown: bool, pending_ }; assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); - // Close the channel. We should see a `SpliceFailed` event for the pending splice + // Close the channel. We should see a `SpliceNegotiationFailed` event for the pending splice // `QuiescentAction`. let (closer_node, closee_node) = if local_shutdown { (&nodes[0], &nodes[1]) } else { (&nodes[1], &nodes[0]) }; @@ -2911,12 +2966,6 @@ fn do_abandon_splice_quiescent_action_on_shutdown(local_shutdown: bool, pending_ let events = nodes[0].node.get_and_clear_pending_events(); assert_eq!(events.len(), 2, "{events:?}"); match &events[0] { - Event::SpliceFailed { channel_id: cid, .. } => { - assert_eq!(*cid, channel_id); - }, - other => panic!("Expected SpliceFailed, got {:?}", other), - } - match &events[1] { Event::DiscardFunding { funding_info: FundingInfo::Contribution { inputs, outputs }, .. @@ -2930,8 +2979,21 @@ fn do_abandon_splice_quiescent_action_on_shutdown(local_shutdown: bool, pending_ }, other => panic!("Expected DiscardFunding with Contribution, got {:?}", other), } + match &events[1] { + Event::SpliceNegotiationFailed { channel_id: cid, reason, contribution, .. } => { + assert_eq!(*cid, channel_id); + assert_eq!(*reason, NegotiationFailureReason::ChannelClosing); + assert!(contribution.is_some()); + }, + other => panic!("Expected SpliceNegotiationFailed, got {:?}", other), + } } else { - expect_splice_failed_events(&nodes[0], &channel_id, funding_contribution); + expect_splice_failed_events( + &nodes[0], + &channel_id, + funding_contribution, + NegotiationFailureReason::ChannelClosing, + ); } let _ = get_event_msg!(closee_node, MessageSendEvent::SendShutdown, closer_node_id); } @@ -3600,11 +3662,11 @@ fn test_funding_contributed_splice_already_pending() { .build() .unwrap(); - // Initiate a second splice with a DIFFERENT output to test that different outputs - // are included in DiscardFunding (not filtered out) + // Initiate a second splice with a DIFFERENT output (different script_pubkey) to test that + // non-overlapping outputs are included in DiscardFunding (not filtered out). let second_splice_out = TxOut { - value: Amount::from_sat(6_000), // Different amount - script_pubkey: ScriptBuf::new_p2wpkh(&WPubkeyHash::from_raw_hash(Hash::all_zeros())), + value: Amount::from_sat(6_000), + script_pubkey: nodes[1].wallet_source.get_change_script().unwrap(), }; // Clear UTXOs and add a LARGER one for the second contribution to ensure @@ -3635,8 +3697,7 @@ fn test_funding_contributed_splice_already_pending() { // Second funding_contributed with a different contribution - this should trigger // DiscardFunding because there's already a pending quiescent action (splice contribution). // Only inputs/outputs NOT in the existing contribution should be discarded. - let (expected_inputs, expected_outputs) = - second_contribution.clone().into_contributed_inputs_and_outputs(); + let expected_inputs: Vec<_> = second_contribution.contributed_inputs().collect(); // Returns Err(APIMisuseError) and emits DiscardFunding for the non-duplicate parts of the second contribution assert_eq!( @@ -3656,11 +3717,10 @@ fn test_funding_contributed_splice_already_pending() { if let FundingInfo::Contribution { inputs, outputs } = funding_info { // The input is different, so it should be in the discard event assert_eq!(*inputs, expected_inputs); - // The splice-out output is different (6000 vs 5000), so it should be in discard event - assert!(expected_outputs.contains(&second_splice_out)); - assert!(!expected_outputs.contains(&first_splice_out)); - // The different outputs should NOT be filtered out - assert_eq!(*outputs, expected_outputs); + // The splice-out output (different script_pubkey) survives filtering; + // the change output (same script_pubkey as first contribution) is filtered. + assert_eq!(outputs.len(), 1); + assert!(outputs.contains(&second_splice_out)); } else { panic!("Expected FundingInfo::Contribution"); } @@ -3726,7 +3786,7 @@ fn test_funding_contributed_active_funding_negotiation() { fn do_test_funding_contributed_active_funding_negotiation(state: u8) { // Tests that calling funding_contributed when a splice is already being actively negotiated // (pending_splice.funding_negotiation exists and is_initiator()) returns Err(APIMisuseError) - // and emits SpliceFailed + DiscardFunding events for non-duplicate contributions, or + // and emits SpliceNegotiationFailed + DiscardFunding events for non-duplicate contributions, or // returns Err(APIMisuseError) with no events for duplicate contributions. // // State 0: AwaitingAck (splice_init sent, splice_ack not yet received) @@ -3753,14 +3813,24 @@ fn do_test_funding_contributed_active_funding_negotiation(state: u8) { let first_contribution = funding_template.splice_in_sync(splice_in_amount, feerate, FeeRate::MAX, &wallet).unwrap(); - // Build second contribution with different UTXOs so inputs/outputs don't overlap + // Build second contribution with different UTXOs and a splice-out output using a different + // script_pubkey (node 1's address) so it survives script_pubkey-based filtering. nodes[0].wallet_source.clear_utxos(); provide_utxo_reserves(&nodes, 1, splice_in_amount * 3); + let splice_out_output = TxOut { + value: Amount::from_sat(1_000), + script_pubkey: nodes[1].wallet_source.get_change_script().unwrap(), + }; let funding_template = nodes[0].node.splice_channel(&channel_id, &node_id_1).unwrap(); let wallet = WalletSync::new(Arc::clone(&nodes[0].wallet_source), nodes[0].logger); - let second_contribution = - funding_template.splice_in_sync(splice_in_amount, feerate, FeeRate::MAX, &wallet).unwrap(); + let second_contribution = funding_template + .without_prior_contribution(feerate, FeeRate::MAX) + .with_coin_selection_source_sync(&wallet) + .add_value(splice_in_amount) + .add_outputs(vec![splice_out_output.clone()]) + .build() + .unwrap(); // First funding_contributed - sets up the quiescent action and queues STFU nodes[0] @@ -3805,10 +3875,10 @@ fn do_test_funding_contributed_active_funding_negotiation(state: u8) { } } - // Call funding_contributed with a different contribution (non-overlapping inputs/outputs). - // This hits the funding_negotiation path and returns DiscardFunding. - let (expected_inputs, expected_outputs) = - second_contribution.clone().into_contributed_inputs_and_outputs(); + // Call funding_contributed with the second contribution. Inputs don't overlap (different + // UTXOs) so they all survive. The splice-out output (different script_pubkey) survives + // while the change output (same script_pubkey as first contribution) is filtered. + let expected_inputs: Vec<_> = second_contribution.contributed_inputs().collect(); assert_eq!( nodes[0].node.funding_contributed(&channel_id, &node_id_1, second_contribution, None), Err(APIError::APIMisuseError { @@ -3816,15 +3886,17 @@ fn do_test_funding_contributed_active_funding_negotiation(state: u8) { }) ); - // Assert DiscardFunding event with the non-duplicate inputs/outputs let events = nodes[0].node.get_and_clear_pending_events(); assert_eq!(events.len(), 1, "{events:?}"); match &events[0] { Event::DiscardFunding { channel_id: event_channel_id, funding_info } => { assert_eq!(*event_channel_id, channel_id); if let FundingInfo::Contribution { inputs, outputs } = funding_info { + // Inputs are unique (different UTXOs) so none are filtered. assert_eq!(*inputs, expected_inputs); - assert_eq!(*outputs, expected_outputs); + // Only the splice-out output survives; the change output is filtered + // (same script_pubkey as first contribution's change). + assert_eq!(*outputs, vec![splice_out_output]); } else { panic!("Expected FundingInfo::Contribution"); } @@ -3862,7 +3934,7 @@ fn do_test_funding_contributed_active_funding_negotiation(state: u8) { #[test] fn test_funding_contributed_channel_shutdown() { // Tests that calling funding_contributed after initiating channel shutdown returns Err(APIMisuseError) - // and emits both SpliceFailed and DiscardFunding events. The channel is no longer usable + // and emits both SpliceNegotiationFailed and DiscardFunding events. The channel is no longer usable // after shutdown is initiated, so quiescence cannot be proposed. let chanmon_cfgs = create_chanmon_cfgs(2); let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); @@ -3891,7 +3963,7 @@ fn test_funding_contributed_channel_shutdown() { // Now call funding_contributed - this should trigger FailSplice because // propose_quiescence() will fail when is_usable() returns false. - // Returns Err(APIMisuseError) and emits both SpliceFailed and DiscardFunding. + // Returns Err(APIMisuseError) and emits both SpliceNegotiationFailed and DiscardFunding. assert_eq!( nodes[0].node.funding_contributed( &channel_id, @@ -3904,7 +3976,12 @@ fn test_funding_contributed_channel_shutdown() { }) ); - expect_splice_failed_events(&nodes[0], &channel_id, funding_contribution); + expect_splice_failed_events( + &nodes[0], + &channel_id, + funding_contribution, + NegotiationFailureReason::ChannelClosing, + ); } #[test] @@ -4085,7 +4162,12 @@ fn do_test_splice_pending_htlcs(config: UserConfig) { let reconnect_args = ReconnectArgs::new(initiator, acceptor); reconnect_nodes(reconnect_args); - expect_splice_failed_events(initiator, &channel_id, contribution); + expect_splice_failed_events( + initiator, + &channel_id, + contribution, + NegotiationFailureReason::PeerDisconnected, + ); // 4) Try again with the additional satoshi removed from the splice-out message, and check that it passes // validation on the receiver's side. @@ -4120,7 +4202,12 @@ fn do_test_splice_pending_htlcs(config: UserConfig) { nodes[1].node.peer_disconnected(node_id_0); let reconnect_args = ReconnectArgs::new(&nodes[0], &nodes[1]); reconnect_nodes(reconnect_args); - expect_splice_failed_events(&nodes[1], &channel_id, contribution); + expect_splice_failed_events( + &nodes[1], + &channel_id, + contribution, + NegotiationFailureReason::PeerDisconnected, + ); let details = &nodes[1].node.list_channels()[0]; let expected_outbound_htlc_max = (pre_splice_balance.to_sat() - details.unspendable_punishment_reserve.unwrap()) * 1000; @@ -4231,7 +4318,7 @@ pub fn reenter_quiescence<'a, 'b, 'c>( #[test] fn test_splice_acceptor_disconnect_emits_events() { // When both nodes contribute to a splice and the negotiation fails due to disconnect, - // both the initiator and acceptor should receive SpliceFailed + DiscardFunding events + // both the initiator and acceptor should receive SpliceNegotiationFailed + DiscardFunding events // so each can reclaim their UTXOs. let chanmon_cfgs = create_chanmon_cfgs(2); let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); @@ -4270,19 +4357,20 @@ fn test_splice_acceptor_disconnect_emits_events() { nodes[0].node.peer_disconnected(node_id_1); nodes[1].node.peer_disconnected(node_id_0); - // The initiator should get SpliceFailed + DiscardFunding. - expect_splice_failed_events(&nodes[0], &channel_id, node_0_funding_contribution); + // The initiator should get SpliceNegotiationFailed + DiscardFunding. + expect_splice_failed_events( + &nodes[0], + &channel_id, + node_0_funding_contribution, + NegotiationFailureReason::PeerDisconnected, + ); - // The acceptor should also get SpliceFailed + DiscardFunding with its contributions + // The acceptor should also get SpliceNegotiationFailed + DiscardFunding with its contributions // so it can reclaim its UTXOs. The contribution is feerate-adjusted by handle_splice_init, // so we check for non-empty inputs/outputs rather than exact values. let events = nodes[1].node.get_and_clear_pending_events(); assert_eq!(events.len(), 2, "{events:?}"); match &events[0] { - Event::SpliceFailed { channel_id: cid, .. } => assert_eq!(*cid, channel_id), - other => panic!("Expected SpliceFailed, got {:?}", other), - } - match &events[1] { Event::DiscardFunding { funding_info: FundingInfo::Contribution { inputs, outputs }, .. @@ -4292,6 +4380,14 @@ fn test_splice_acceptor_disconnect_emits_events() { }, other => panic!("Expected DiscardFunding with Contribution, got {:?}", other), } + match &events[1] { + Event::SpliceNegotiationFailed { channel_id: cid, reason, contribution, .. } => { + assert_eq!(*cid, channel_id); + assert_eq!(*reason, NegotiationFailureReason::PeerDisconnected); + assert!(contribution.is_some()); + }, + other => panic!("Expected SpliceNegotiationFailed, got {:?}", other), + } // Reconnect and verify the channel is still operational. let mut reconnect_args = ReconnectArgs::new(&nodes[0], &nodes[1]); @@ -4357,13 +4453,109 @@ fn test_splice_rbf_acceptor_basic() { expect_splice_pending_event(&nodes[1], &node_id_0); // Step 11: Mine, lock, and verify DiscardFunding for the replaced splice candidate. - lock_rbf_splice_after_blocks( + let result = lock_rbf_splice_after_blocks( &nodes[0], &nodes[1], &rbf_tx, ANTI_REORG_DELAY - 1, &[first_splice_tx.compute_txid()], ); + + // The test wallet reuses the same UTXO across RBF rounds (the wallet doesn't track + // in-flight spends), so all contributed inputs are in the promoted tx. No unique + // contributions to discard. + assert!(result.node_a_discarded.is_empty()); + assert!(result.node_b_discarded.is_empty()); +} + +#[test] +fn test_splice_rbf_discard_unique_contribution() { + // Verify that DiscardFunding events contain the correct unique inputs and outputs when the + // RBF round uses different UTXOs than the initial splice. By clearing the wallet between + // rounds and providing fresh UTXOs, we force distinct inputs per round. Round 0 also + // includes a splice-out output with a unique script_pubkey not present in the RBF tx. + // When the RBF is promoted, round 0's inputs and splice-out output should appear in + // DiscardFunding. The change output is filtered because it shares a script_pubkey with the + // promoted tx's change output. + 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); + + let added_value = Amount::from_sat(50_000); + provide_utxo_reserves(&nodes, 2, added_value * 2); + + // Round 0: Splice-in-and-out from node 0 with a splice-out output. + let splice_out_output = TxOut { + value: Amount::from_sat(5_000), + script_pubkey: ScriptBuf::new_p2wpkh(&WPubkeyHash::all_zeros()), + }; + let funding_contribution = do_initiate_splice_in_and_out( + &nodes[0], + &nodes[1], + channel_id, + added_value, + vec![splice_out_output.clone()], + ); + let round_0_inputs: Vec<_> = funding_contribution.contributed_inputs().collect(); + assert!(!round_0_inputs.is_empty()); + + let (first_splice_tx, new_funding_script) = + splice_channel(&nodes[0], &nodes[1], channel_id, funding_contribution); + + // Clear node 0's wallet so round 1 must use different UTXOs. + nodes[0].wallet_source.clear_utxos(); + provide_utxo_reserves(&nodes, 2, added_value * 2); + + // Round 1: RBF with fresh UTXOs, splice-in only (no splice-out output). + let rbf_feerate = FeeRate::from_sat_per_kwu(FEERATE_FLOOR_SATS_PER_KW as u64 + 25); + let funding_contribution = + do_initiate_rbf_splice_in(&nodes[0], &nodes[1], channel_id, added_value, rbf_feerate); + let round_1_inputs: Vec<_> = funding_contribution.contributed_inputs().collect(); + assert_ne!(round_0_inputs, round_1_inputs, "Rounds must use different UTXOs"); + + complete_rbf_handshake(&nodes[0], &nodes[1]); + + complete_interactive_funding_negotiation( + &nodes[0], + &nodes[1], + channel_id, + funding_contribution, + new_funding_script.clone(), + ); + + let (rbf_tx, splice_locked) = sign_interactive_funding_tx(&nodes[0], &nodes[1], false); + assert!(splice_locked.is_none()); + + expect_splice_pending_event(&nodes[0], &node_id_1); + expect_splice_pending_event(&nodes[1], &node_id_0); + + let result = lock_rbf_splice_after_blocks( + &nodes[0], + &nodes[1], + &rbf_tx, + ANTI_REORG_DELAY - 1, + &[first_splice_tx.compute_txid()], + ); + + // Node 0's round 0 inputs are NOT in the promoted tx (which uses round 1's fresh UTXOs), + // so they appear as unique contributions to discard. The splice-out output also survives + // because its script_pubkey is not in the promoted tx. The change output is filtered + // because it shares a script_pubkey with the promoted tx's change output. + assert_eq!(result.node_a_discarded.len(), 1); + let (ref inputs, ref outputs) = result.node_a_discarded[0]; + assert_eq!(*inputs, round_0_inputs); + assert_eq!(*outputs, vec![splice_out_output]); + + // Node 1 (non-contributing acceptor) has no contributions to discard. + assert!(result.node_b_discarded.is_empty()); } #[test] @@ -4753,6 +4945,104 @@ fn test_splice_rbf_after_splice_locked() { } } +#[test] +fn test_splice_rbf_stfu_after_splice_locked() { + // Test that we don't send tx_init_rbf when we've already sent splice_locked. + // + // Scenario: node 0 initiates an RBF and sends STFU, but before receiving the counterparty's + // STFU response, it mines enough blocks to send splice_locked (setting sent_funding_txid). + // When node 1's STFU arrives, the stfu() handler should detect that RBF is no longer valid + // and return WarnAndDisconnect instead of sending tx_init_rbf. + 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); + + let added_value = Amount::from_sat(50_000); + provide_utxo_reserves(&nodes, 2, added_value * 2); + + // Complete a splice-in from node 0. + let funding_contribution = do_initiate_splice_in(&nodes[0], &nodes[1], channel_id, added_value); + let (splice_tx, _) = splice_channel(&nodes[0], &nodes[1], channel_id, funding_contribution); + + // Mine the splice tx on both nodes (not enough for splice_locked yet). + mine_transaction(&nodes[0], &splice_tx); + mine_transaction(&nodes[1], &splice_tx); + + // Provide more UTXOs for the RBF attempt. + provide_utxo_reserves(&nodes, 2, added_value * 2); + + // Initiate RBF from node 0. + let rbf_feerate = FeeRate::from_sat_per_kwu(FEERATE_FLOOR_SATS_PER_KW as u64 + 25); + let _funding_contribution = + do_initiate_rbf_splice_in(&nodes[0], &nodes[1], channel_id, added_value, rbf_feerate); + + // Node 0 sends STFU (can_initiate_rbf passes since no splice_locked yet). + let stfu_init = get_event_msg!(nodes[0], MessageSendEvent::SendStfu, node_id_1); + + // Deliver STFU to node 1; extract node 1's STFU response but don't deliver it yet. + nodes[1].node.handle_stfu(node_id_0, &stfu_init); + let stfu_ack = get_event_msg!(nodes[1], MessageSendEvent::SendStfu, node_id_0); + + // Mine enough blocks on node 0 so it sends splice_locked (sets sent_funding_txid). + connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1); + let _splice_locked = get_event_msg!(nodes[0], MessageSendEvent::SendSpliceLocked, node_id_1); + + // Now deliver node 1's STFU to node 0. The stfu() handler should detect that RBF is no + // longer valid (we already sent splice_locked) and return WarnAndDisconnect. + nodes[0].node.handle_stfu(node_id_1, &stfu_ack); + + let msg_events = nodes[0].node.get_and_clear_pending_msg_events(); + assert_eq!(msg_events.len(), 1, "{msg_events:?}"); + match &msg_events[0] { + MessageSendEvent::HandleError { action, .. } => { + assert_eq!( + *action, + msgs::ErrorAction::DisconnectPeerWithWarning { + msg: msgs::WarningMessage { + channel_id, + data: format!( + "Channel {} already sent splice_locked, cannot RBF", + channel_id, + ), + }, + } + ); + }, + _ => panic!("Expected HandleError, got {:?}", msg_events[0]), + } + + // Node 0 should emit DiscardFunding + SpliceNegotiationFailed for the RBF contribution. + // The change output is filtered (same script_pubkey as the first splice's change output), + // but the input survives because it's a different UTXO from the first splice. + let events = nodes[0].node.get_and_clear_pending_events(); + assert_eq!(events.len(), 2, "{events:?}"); + match &events[0] { + Event::DiscardFunding { + funding_info: FundingInfo::Contribution { inputs, outputs }, + .. + } => { + assert!(!inputs.is_empty()); + assert!(outputs.is_empty()); + }, + other => panic!("Expected DiscardFunding, got {:?}", other), + } + match &events[1] { + Event::SpliceNegotiationFailed { channel_id: cid, reason, .. } => { + assert_eq!(*cid, channel_id); + assert_eq!(*reason, NegotiationFailureReason::CannotInitiateRbf); + }, + other => panic!("Expected SpliceNegotiationFailed, got {:?}", other), + } +} + #[test] fn test_splice_zeroconf_no_rbf_feerate() { // Test that splice_channel returns a FundingTemplate with min_rbf_feerate = None for a @@ -5105,13 +5395,18 @@ pub fn do_test_splice_rbf_tiebreak( expect_splice_pending_event(&nodes[1], &node_id_0); // Mine, lock, and verify DiscardFunding for the replaced splice candidate. - lock_rbf_splice_after_blocks( + let result = lock_rbf_splice_after_blocks( &nodes[0], &nodes[1], &rbf_tx, ANTI_REORG_DELAY - 1, &[first_splice_tx.compute_txid()], ); + + // The test wallet reuses the same UTXOs across RBF rounds, so all contributed inputs + // are in the promoted tx and nothing is unique to discard. + assert!(result.node_a_discarded.is_empty()); + assert!(result.node_b_discarded.is_empty()); } else { // Acceptor does not contribute — complete with only node 0's inputs/outputs. complete_interactive_funding_negotiation_for_both( @@ -5136,14 +5431,14 @@ pub fn do_test_splice_rbf_tiebreak( // Mine, lock, and verify DiscardFunding for the replaced splice candidate. // Node 1's QuiescentAction was preserved, so after splice_locked it re-initiates // quiescence to retry its contribution in a future splice. - let node_b_stfu = lock_rbf_splice_after_blocks( + let result = lock_rbf_splice_after_blocks( &nodes[0], &nodes[1], &rbf_tx, ANTI_REORG_DELAY - 1, &[first_splice_tx.compute_txid()], ); - let stfu_1 = if let Some(MessageSendEvent::SendStfu { msg, .. }) = node_b_stfu { + let stfu_1 = if let Some(MessageSendEvent::SendStfu { msg, .. }) = result.stfu { msg } else { panic!("Expected SendStfu from node 1"); @@ -5402,13 +5697,18 @@ fn test_splice_rbf_acceptor_recontributes() { expect_splice_pending_event(&nodes[1], &node_id_0); // Step 12: Mine, lock, and verify DiscardFunding for the replaced splice candidate. - lock_rbf_splice_after_blocks( + let result = lock_rbf_splice_after_blocks( &nodes[0], &nodes[1], &rbf_tx, ANTI_REORG_DELAY - 1, &[first_splice_tx.compute_txid()], ); + + // The test wallet reuses the same UTXOs across RBF rounds, so all contributed inputs + // are in the promoted tx and nothing is unique to discard. + assert!(result.node_a_discarded.is_empty()); + assert!(result.node_b_discarded.is_empty()); } #[test] @@ -5727,13 +6027,18 @@ fn test_splice_rbf_sequential() { // --- Mine and lock the final RBF, verifying DiscardFunding for both replaced candidates. --- let splice_tx_0_txid = splice_tx_0.compute_txid(); let splice_tx_1_txid = splice_tx_1.compute_txid(); - lock_rbf_splice_after_blocks( + let result = lock_rbf_splice_after_blocks( &nodes[0], &nodes[1], &rbf_tx_final, ANTI_REORG_DELAY - 1, &[splice_tx_0_txid, splice_tx_1_txid], ); + + // The test wallet reuses the same UTXOs across RBF rounds, so all contributed inputs + // are in the promoted tx and nothing is unique to discard. + assert!(result.node_a_discarded.is_empty()); + assert!(result.node_b_discarded.is_empty()); } #[test] @@ -5969,7 +6274,7 @@ fn test_splice_rbf_amends_prior_net_negative_contribution_request() { fn test_splice_rbf_acceptor_contributes_then_disconnects() { // When both nodes contribute to a splice and the initiator RBFs (with the acceptor // re-contributing via prior contribution), disconnecting mid-interactive-TX should emit - // SpliceFailed + DiscardFunding for both nodes so each can reclaim their UTXOs. + // SpliceNegotiationFailed + DiscardFunding for both nodes so each can reclaim their UTXOs. 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]); @@ -6044,21 +6349,22 @@ fn test_splice_rbf_acceptor_contributes_then_disconnects() { nodes[0].node.peer_disconnected(node_id_1); nodes[1].node.peer_disconnected(node_id_0); - // The initiator should get SpliceFailed + DiscardFunding. + // The initiator re-used the same UTXOs as round 0. Since those UTXOs are still committed + // to round 0's splice, they are filtered and no DiscardFunding is emitted. let events = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(events.len(), 2, "{events:?}"); + assert_eq!(events.len(), 1, "{events:?}"); match &events[0] { - Event::SpliceFailed { channel_id: cid, .. } => assert_eq!(*cid, channel_id), - other => panic!("Expected SpliceFailed, got {:?}", other), - } - match &events[1] { - Event::DiscardFunding { funding_info: FundingInfo::Contribution { .. }, .. } => {}, - other => panic!("Expected DiscardFunding with Contribution, got {:?}", other), + Event::SpliceNegotiationFailed { channel_id: cid, reason, contribution, .. } => { + assert_eq!(*cid, channel_id); + assert_eq!(*reason, NegotiationFailureReason::PeerDisconnected); + assert!(contribution.is_some()); + }, + other => panic!("Expected SpliceNegotiationFailed, got {:?}", other), } // The acceptor re-contributed the same UTXOs as round 0 (via prior contribution // adjustment). Since those UTXOs are still committed to round 0's splice, they are - // filtered from the DiscardFunding event. With all inputs/outputs filtered, no events + // filtered and no DiscardFunding is emitted. With all inputs/outputs filtered, no events // are emitted for the acceptor. let events = nodes[1].node.get_and_clear_pending_events(); assert_eq!(events.len(), 0, "{events:?}"); @@ -6123,16 +6429,10 @@ fn test_splice_rbf_disconnect_filters_prior_contributions() { nodes[0].node.peer_disconnected(node_id_1); nodes[1].node.peer_disconnected(node_id_0); - // The initiator should get SpliceFailed + DiscardFunding with filtered contributions. + // The initiator should get DiscardFunding + SpliceNegotiationFailed with filtered contributions. let events = nodes[0].node.get_and_clear_pending_events(); assert_eq!(events.len(), 2, "{events:?}"); match &events[0] { - Event::SpliceFailed { channel_id: cid, .. } => { - assert_eq!(*cid, channel_id); - }, - other => panic!("Expected SpliceFailed, got {:?}", other), - } - match &events[1] { Event::DiscardFunding { funding_info: FundingInfo::Contribution { inputs, outputs }, .. @@ -6145,6 +6445,14 @@ fn test_splice_rbf_disconnect_filters_prior_contributions() { }, other => panic!("Expected DiscardFunding with Contribution, got {:?}", other), } + match &events[1] { + Event::SpliceNegotiationFailed { channel_id: cid, reason, contribution, .. } => { + assert_eq!(*cid, channel_id); + assert_eq!(*reason, NegotiationFailureReason::PeerDisconnected); + assert!(contribution.is_some()); + }, + other => panic!("Expected SpliceNegotiationFailed, got {:?}", other), + } // Reconnect. After a completed splice, channel_ready is not re-sent. let mut reconnect_args = ReconnectArgs::new(&nodes[0], &nodes[1]); @@ -6166,14 +6474,14 @@ fn test_splice_rbf_disconnect_filters_prior_contributions() { nodes[1].node.peer_disconnected(node_id_0); let events = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(events.len(), 2, "{events:?}"); + assert_eq!(events.len(), 1, "{events:?}"); match &events[0] { - Event::SpliceFailed { channel_id: cid, .. } => assert_eq!(*cid, channel_id), - other => panic!("Expected SpliceFailed, got {:?}", other), - } - match &events[1] { - Event::DiscardFunding { .. } => {}, - other => panic!("Expected DiscardFunding, got {:?}", other), + Event::SpliceNegotiationFailed { channel_id: cid, reason, contribution, .. } => { + assert_eq!(*cid, channel_id); + assert_eq!(*reason, NegotiationFailureReason::PeerDisconnected); + assert!(contribution.is_some()); + }, + other => panic!("Expected SpliceNegotiationFailed, got {:?}", other), } let mut reconnect_args = ReconnectArgs::new(&nodes[0], &nodes[1]); @@ -6321,7 +6629,7 @@ fn test_funding_contributed_rbf_adjustment_exceeds_max_feerate() { // Mine and lock the pending splice → pending_splice is cleared. mine_transaction(&nodes[0], &_splice_tx); mine_transaction(&nodes[1], &_splice_tx); - let stfu = lock_splice_after_blocks(&nodes[0], &nodes[1], ANTI_REORG_DELAY - 1); + let stfu = lock_splice_after_blocks(&nodes[0], &nodes[1], ANTI_REORG_DELAY - 1).stfu; // STFU is sent during lock — the splice proceeds as a fresh splice (not RBF). let stfu = match stfu { @@ -6398,7 +6706,7 @@ fn test_funding_contributed_rbf_adjustment_insufficient_budget() { // Mine and lock the pending splice → pending_splice is cleared. mine_transaction(&nodes[0], &_splice_tx); mine_transaction(&nodes[1], &_splice_tx); - let stfu = lock_splice_after_blocks(&nodes[0], &nodes[1], ANTI_REORG_DELAY - 1); + let stfu = lock_splice_after_blocks(&nodes[0], &nodes[1], ANTI_REORG_DELAY - 1).stfu; // STFU is sent during lock — the splice proceeds as a fresh splice (not RBF). let stfu = match stfu { @@ -6577,7 +6885,7 @@ fn test_rbf_sync_returns_err_when_max_feerate_below_min_rbf() { fn test_splice_revalidation_at_quiescence() { // When an outbound HTLC is committed between funding_contributed and quiescence, the // holder's balance decreases. If the splice-out was marginal at funding_contributed time, - // the re-validation at quiescence should fail and emit SpliceFailed + DiscardFunding. + // the re-validation at quiescence should fail and emit SpliceNegotiationFailed + DiscardFunding. // // Flow: // 1. Send payment #1 (update_add + CS) → node 0 awaits RAA @@ -6705,7 +7013,12 @@ fn test_splice_revalidation_at_quiescence() { assert_eq!(msg_events.len(), 1, "{msg_events:?}"); assert!(matches!(msg_events[0], MessageSendEvent::HandleError { .. })); - expect_splice_failed_events(&nodes[0], &channel_id, contribution); + expect_splice_failed_events( + &nodes[0], + &channel_id, + contribution, + NegotiationFailureReason::ContributionInvalid, + ); } #[test] @@ -6848,12 +7161,16 @@ fn test_splice_rbf_rejects_own_low_feerate_after_several_attempts() { let result = nodes[0].node.funding_contributed(&channel_id, &node_id_1, contribution, None); assert!(result.is_err(), "Expected rejection for low feerate: {:?}", result); - // SpliceFailed is emitted. DiscardFunding is not emitted because all inputs/outputs + // SpliceNegotiationFailed is emitted. DiscardFunding is not emitted because all inputs/outputs // are filtered out (same UTXOs reused for RBF, still committed to the prior splice tx). let events = nodes[0].node.get_and_clear_pending_events(); assert_eq!(events.len(), 1, "{events:?}"); match &events[0] { - Event::SpliceFailed { channel_id: cid, .. } => assert_eq!(*cid, channel_id), - other => panic!("Expected SpliceFailed, got {:?}", other), + Event::SpliceNegotiationFailed { channel_id: cid, reason, contribution, .. } => { + assert_eq!(*cid, channel_id); + assert_eq!(*reason, NegotiationFailureReason::FeeRateTooLow); + assert!(contribution.is_some()); + }, + other => panic!("Expected SpliceNegotiationFailed, got {:?}", other), } } diff --git a/pending_changelog/4388-splice-failed-discard-funding.txt b/pending_changelog/4388-splice-failed-discard-funding.txt index 64fc4ab4e26..67680f49cb1 100644 --- a/pending_changelog/4388-splice-failed-discard-funding.txt +++ b/pending_changelog/4388-splice-failed-discard-funding.txt @@ -1,21 +1,21 @@ # API Updates - * `Event::SpliceFailed` no longer carries `contributed_inputs` or `contributed_outputs` fields. + * `Event::SpliceNegotiationFailed` no longer carries `contributed_inputs` or `contributed_outputs` fields. Instead, a separate `Event::DiscardFunding` event with `FundingInfo::Contribution` is emitted for UTXO cleanup. * `Event::DiscardFunding` with `FundingInfo::Contribution` is also emitted without a - corresponding `Event::SpliceFailed` when `ChannelManager::funding_contributed` returns an + corresponding `Event::SpliceNegotiationFailed` when `ChannelManager::funding_contributed` returns an error (e.g., channel or peer not found, wrong channel state, duplicate contribution). # Backwards Compatibility * Older serializations that included `contributed_inputs` and `contributed_outputs` in - `SpliceFailed` will have those fields silently ignored on deserialization (they were odd TLV + `SpliceNegotiationFailed` will have those fields silently ignored on deserialization (they were odd TLV fields). A `DiscardFunding` event will not be produced when reading these older serializations. # Forward Compatibility * Downgrading will not set the removed `contributed_inputs`/`contributed_outputs` fields on - `SpliceFailed`, so older code expecting those fields will see empty vectors for splice + `SpliceNegotiationFailed`, so older code expecting those fields will see empty vectors for splice failures. diff --git a/pending_changelog/4514-splice-negotiation-failed.txt b/pending_changelog/4514-splice-negotiation-failed.txt new file mode 100644 index 00000000000..809bf7cb86d --- /dev/null +++ b/pending_changelog/4514-splice-negotiation-failed.txt @@ -0,0 +1,11 @@ +# API Updates + + * `Event::SplicePending` has been renamed to `Event::SpliceNegotiated`. + + * `Event::SpliceFailed` has been renamed to `Event::SpliceNegotiationFailed`. + + * `Event::SpliceNegotiationFailed` now includes a `reason` field + (`NegotiationFailureReason`) indicating why the negotiation round failed, + and a `contribution` field returning the `FundingContribution` for retry. + + * `FundingContribution` now exposes `feerate()` and `inputs()` accessor methods.