diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index f20f93c789c..527670e1317 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -2015,7 +2015,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, @@ -2027,7 +2027,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 c1d7982e5e4..6de7308e27e 100644 --- a/fuzz/src/full_stack.rs +++ b/fuzz/src/full_stack.rs @@ -1141,10 +1141,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..065c2dbb5ab 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,109 @@ 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, +} + +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 => 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"), + } + } +} + +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) => {}, +); + /// 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 +1645,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 +1664,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 +1687,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 +2467,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 +2485,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 +3124,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 +3139,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 32c0e94bdc8..33645b2c9cf 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 { @@ -6851,13 +6861,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) } @@ -6865,10 +6868,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. @@ -7019,72 +7018,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(); - - let funding_txo = funding_negotiation - .as_funding() - .and_then(|funding| funding.get_funding_txo()) - .map(|txo| txo.into_bitcoin_outpoint()); + contributed_outputs: Vec, - 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), + }), + } }}; } @@ -7114,28 +7097,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, } @@ -7144,7 +7123,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, _ => { @@ -7271,30 +7250,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(); } @@ -7312,12 +7305,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 ) } @@ -12309,7 +12313,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(|| { @@ -12540,7 +12544,10 @@ where }) { 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() { @@ -12556,6 +12563,7 @@ where ); return Err(QuiescentError::FailSplice( self.splice_funding_failed_for(contribution), + NegotiationFailureReason::FeeRateTooLow, )); } } @@ -13996,9 +14004,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!( @@ -14115,7 +14132,10 @@ where self.context.channel_id(), e, )), - QuiescentError::FailSplice(failed), + QuiescentError::FailSplice( + failed, + NegotiationFailureReason::ContributionInvalid, + ), )); } let prior_contribution = contribution.clone(); diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 2e782701e47..b927207e771 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}; @@ -4107,24 +4107,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, )); @@ -4414,23 +4414,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, )); @@ -4919,24 +4919,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, )); @@ -6609,35 +6609,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, - )); - } }, } } @@ -6675,14 +6665,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. /// @@ -6764,7 +6754,7 @@ impl< "Channel {} already has a pending funding contribution", channel_id, ), - QuiescentError::FailSplice(_) => format!( + QuiescentError::FailSplice(..) => format!( "Channel {} cannot accept funding contribution", channel_id, ), @@ -6901,7 +6891,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(), @@ -10994,7 +10984,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(), @@ -11850,23 +11840,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, @@ -12009,23 +11998,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, @@ -12108,7 +12099,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(), @@ -12179,23 +12170,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, @@ -12327,24 +12322,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, )); @@ -15445,19 +15440,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, }); } @@ -18108,31 +18103,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, )); @@ -18256,7 +18252,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 84cdf785da5..3444c82b47a 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -17,8 +17,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, @@ -2389,7 +2389,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 ); } @@ -3232,7 +3232,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 }, @@ -3243,21 +3243,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 { @@ -3266,6 +3260,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 c08a0a9f471..decf8f2b62e 100644 --- a/lightning/src/ln/funding.rs +++ b/lightning/src/ln/funding.rs @@ -120,10 +120,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. @@ -710,7 +710,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 } @@ -731,6 +732,11 @@ impl FundingContribution { self.value_added } + /// 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`]. @@ -771,7 +777,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 9957205716c..d5da65f8b3a 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 9adccd17627..dd8ae3bfe58 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; @@ -282,7 +285,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) }, } @@ -888,7 +896,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); @@ -939,7 +952,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); @@ -1021,7 +1039,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. @@ -1105,7 +1130,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); @@ -2560,7 +2590,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. @@ -2631,7 +2668,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. @@ -2727,7 +2769,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); @@ -3001,7 +3052,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]) }; @@ -3017,12 +3068,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 }, .. @@ -3036,8 +3081,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); } @@ -3708,11 +3766,11 @@ fn test_funding_contributed_splice_already_pending() { ) .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 @@ -3745,8 +3803,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!( @@ -3766,11 +3823,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"); } @@ -3836,7 +3892,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) @@ -3863,14 +3919,26 @@ 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 + .splice_in_and_out_sync( + splice_in_amount, + vec![splice_out_output.clone()], + feerate, + FeeRate::MAX, + &wallet, + ) + .unwrap(); // First funding_contributed - sets up the quiescent action and queues STFU nodes[0] @@ -3915,10 +3983,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 { @@ -3926,15 +3994,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"); } @@ -3972,7 +4042,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); @@ -4001,7 +4071,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, @@ -4014,7 +4084,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] @@ -4195,7 +4270,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. @@ -4230,7 +4310,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; @@ -4341,7 +4426,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); @@ -4380,19 +4465,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 }, .. @@ -4402,6 +4488,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]); @@ -5762,7 +5856,7 @@ fn test_splice_rbf_sequential() { 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]); @@ -5837,21 +5931,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:?}"); @@ -5916,16 +6011,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 }, .. @@ -5938,6 +6027,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]); @@ -5959,14 +6056,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]); @@ -6368,7 +6465,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 @@ -6498,7 +6595,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] @@ -6638,12 +6740,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.