diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 32c0e94bdc8..4777c54dcbf 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1172,9 +1172,6 @@ pub(super) struct InteractiveTxMsgError { /// If a splice was in progress when processing the message, this contains the splice funding /// information for emitting a `SpliceFailed` 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. - pub(super) exited_quiescence: bool, } /// The return value of `monitor_updating_restored` @@ -1722,7 +1719,10 @@ where // We shouldn't be quiescent anymore upon reconnecting if: // - We were in quiescence but a splice/RBF was never negotiated or // - We were in quiescence but the splice negotiation failed due to disconnecting - chan.context.channel_state.clear_quiescent(); + // + // NOTE: While `exit_quiescence` clears the disconnect timer, it should already + // have been cleared by `remove_uncommitted_htlcs_and_mark_paused`. + chan.exit_quiescence(); None } else { None @@ -1818,30 +1818,24 @@ where let logger = WithChannelContext::from(logger, &self.context(), None); log_info!(logger, "Failed interactive transaction negotiation: {reason}"); - let (splice_funding_failed, exited_quiescence) = match &mut self.phase { + let splice_funding_failed = match &mut self.phase { ChannelPhase::Undefined => unreachable!(), - ChannelPhase::UnfundedOutboundV1(_) | ChannelPhase::UnfundedInboundV1(_) => { - (None, false) - }, + ChannelPhase::UnfundedOutboundV1(_) | ChannelPhase::UnfundedInboundV1(_) => None, ChannelPhase::UnfundedV2(pending_v2_channel) => { pending_v2_channel.interactive_tx_constructor.take(); - (None, false) + None }, ChannelPhase::Funded(funded_channel) => { if funded_channel.should_reset_pending_splice_state(false) { - (funded_channel.reset_pending_splice_state(), true) + funded_channel.reset_pending_splice_state() } else { debug_assert!(false, "We should never fail an interactive funding negotiation once we're exchanging tx_signatures"); - (None, false) + None } }, }; - InteractiveTxMsgError { - err: ChannelError::Abort(reason), - splice_funding_failed, - exited_quiescence, - } + InteractiveTxMsgError { err: ChannelError::Abort(reason), splice_funding_failed } } pub fn tx_add_input( @@ -1856,7 +1850,6 @@ where "Received unexpected interactive transaction negotiation message".to_owned(), ), splice_funding_failed: None, - exited_quiescence: false, }), } } @@ -1873,7 +1866,6 @@ where "Received unexpected interactive transaction negotiation message".to_owned(), ), splice_funding_failed: None, - exited_quiescence: false, }), } } @@ -1890,7 +1882,6 @@ where "Received unexpected interactive transaction negotiation message".to_owned(), ), splice_funding_failed: None, - exited_quiescence: false, }), } } @@ -1907,7 +1898,6 @@ where "Received unexpected interactive transaction negotiation message".to_owned(), ), splice_funding_failed: None, - exited_quiescence: false, }), } } @@ -1924,7 +1914,6 @@ where return Err(InteractiveTxMsgError { err: ChannelError::WarnAndDisconnect(err.to_owned()), splice_funding_failed: None, - exited_quiescence: false, }); }, }; @@ -1985,13 +1974,13 @@ where pub fn tx_abort( &mut self, msg: &msgs::TxAbort, logger: &L, - ) -> Result<(Option, Option, bool), ChannelError> { + ) -> Result<(Option, Option), ChannelError> { // If we have not sent a `tx_abort` message for this negotiation previously, we need to echo // back a tx_abort message according to the spec: // https://github.com/lightning/bolts/blob/247e83d/02-peer-protocol.md?plain=1#L560-L561 // For rationale why we echo back `tx_abort`: // https://github.com/lightning/bolts/blob/247e83d/02-peer-protocol.md?plain=1#L578-L580 - let (should_ack, splice_funding_failed, exited_quiescence) = match &mut self.phase { + let (should_ack, splice_funding_failed) = match &mut self.phase { ChannelPhase::Undefined => unreachable!(), ChannelPhase::UnfundedOutboundV1(_) | ChannelPhase::UnfundedInboundV1(_) => { let err = "Got an unexpected tx_abort message: This is an unfunded channel created with V1 channel establishment"; @@ -2000,7 +1989,7 @@ where ChannelPhase::UnfundedV2(pending_v2_channel) => { let had_constructor = pending_v2_channel.interactive_tx_constructor.take().is_some(); - (had_constructor, None, false) + (had_constructor, None) }, ChannelPhase::Funded(funded_channel) => { if funded_channel.has_pending_splice_awaiting_signatures() @@ -2028,11 +2017,11 @@ where .unwrap_or(false); debug_assert!(has_funding_negotiation); let splice_funding_failed = funded_channel.reset_pending_splice_state(); - (true, splice_funding_failed, true) + (true, splice_funding_failed) } else { // We were not tracking the pending funding negotiation state anymore, likely // due to a disconnection or already having sent our own `tx_abort`. - (false, None, false) + (false, None) } }, }; @@ -2048,7 +2037,7 @@ where } }); - Ok((tx_abort, splice_funding_failed, exited_quiescence)) + Ok((tx_abort, splice_funding_failed)) } #[rustfmt::skip] @@ -7299,7 +7288,7 @@ where self.pending_splice.take(); } - self.context.channel_state.clear_quiescent(); + self.exit_quiescence(); if current_is_awaiting_signatures { self.context.interactive_tx_signing_session.take(); } @@ -9319,7 +9308,6 @@ where debug_assert!(!self.context.channel_state.is_awaiting_remote_revoke()); if let Some(pending_splice) = self.pending_splice.as_mut() { - self.context.channel_state.clear_quiescent(); if let Some(FundingNegotiation::AwaitingSignatures { mut funding, funding_feerate_sat_per_1000_weight, @@ -9365,6 +9353,8 @@ where funding_tx_signed.funding_tx = Some((funding_tx, tx_type)); funding_tx_signed.splice_negotiated = Some(splice_negotiated); funding_tx_signed.splice_locked = splice_locked; + + self.exit_quiescence(); } else { debug_assert!(false); } @@ -12686,9 +12676,7 @@ where } /// Checks during handling splice_init - pub fn validate_splice_init( - &self, msg: &msgs::SpliceInit, our_funding_contribution: SignedAmount, - ) -> Result { + pub fn validate_splice_init(&self, msg: &msgs::SpliceInit) -> Result<(), ChannelError> { if self.holder_commitment_point.current_point().is_none() { return Err(ChannelError::WarnAndDisconnect(format!( "Channel {} commitment point needs to be advanced once before spliced", @@ -12725,32 +12713,7 @@ where ))); } - self.validate_splice_contributions(our_funding_contribution, their_funding_contribution) - .map_err(|e| ChannelError::WarnAndDisconnect(e))?; - - // Rotate the pubkeys using the prev_funding_txid as a tweak - let prev_funding_txid = self.funding.get_funding_txid(); - let funding_pubkey = match prev_funding_txid { - None => { - debug_assert!(false); - self.funding.get_holder_pubkeys().funding_pubkey - }, - Some(prev_funding_txid) => self - .context - .holder_signer - .new_funding_pubkey(prev_funding_txid, &self.context.secp_ctx), - }; - let mut new_keys = self.funding.get_holder_pubkeys().clone(); - new_keys.funding_pubkey = funding_pubkey; - - Ok(FundingScope::for_splice( - &self.funding, - &self.context, - our_funding_contribution, - their_funding_contribution, - msg.funding_pubkey, - new_keys, - )) + Ok(()) } fn validate_splice_contributions( @@ -12890,17 +12853,46 @@ where pub(crate) fn splice_init( &mut self, msg: &msgs::SpliceInit, entropy_source: &ES, holder_node_id: &PublicKey, logger: &L, - ) -> Result { + ) -> Result { + self.validate_splice_init(msg).map_err(|e| self.quiescent_negotiation_err(e))?; + let feerate = FeeRate::from_sat_per_kwu(msg.funding_feerate_per_kw as u64); - let (our_funding_contribution, holder_balance) = - self.resolve_queued_contribution(feerate, logger)?; + let (queued_net_value, holder_balance) = self + .resolve_queued_contribution(feerate, logger) + .map_err(|e| self.quiescent_negotiation_err(e))?; + + let our_funding_contribution = queued_net_value.unwrap_or(SignedAmount::ZERO); + let their_funding_contribution = SignedAmount::from_sat(msg.funding_contribution_satoshis); + self.validate_splice_contributions(our_funding_contribution, their_funding_contribution) + .map_err(|e| self.quiescent_negotiation_err(ChannelError::WarnAndDisconnect(e)))?; + + // Rotate the pubkeys using the prev_funding_txid as a tweak + let prev_funding_txid = self.funding.get_funding_txid(); + let funding_pubkey = match prev_funding_txid { + None => { + debug_assert!(false); + self.funding.get_holder_pubkeys().funding_pubkey + }, + Some(prev_funding_txid) => self + .context + .holder_signer + .new_funding_pubkey(prev_funding_txid, &self.context.secp_ctx), + }; + let mut holder_pubkeys = self.funding.get_holder_pubkeys().clone(); + holder_pubkeys.funding_pubkey = funding_pubkey; - let splice_funding = - self.validate_splice_init(msg, our_funding_contribution.unwrap_or(SignedAmount::ZERO))?; + let splice_funding = FundingScope::for_splice( + &self.funding, + &self.context, + our_funding_contribution, + their_funding_contribution, + msg.funding_pubkey, + holder_pubkeys, + ); // Adjust for the feerate and clone so we can store it for future RBF re-use. let (adjusted_contribution, our_funding_inputs, our_funding_outputs) = - if our_funding_contribution.is_some() { + if queued_net_value.is_some() { let adjusted_contribution = self .take_queued_funding_contribution() .expect("queued_funding_contribution was Some") @@ -12911,7 +12903,6 @@ where } else { (None, Default::default(), Default::default()) }; - let our_funding_contribution = our_funding_contribution.unwrap_or(SignedAmount::ZERO); log_info!( logger, @@ -12954,9 +12945,8 @@ where /// Checks during handling tx_init_rbf for an existing splice fn validate_tx_init_rbf( - &self, msg: &msgs::TxInitRbf, our_funding_contribution: SignedAmount, - fee_estimator: &LowerBoundedFeeEstimator, - ) -> Result { + &self, msg: &msgs::TxInitRbf, fee_estimator: &LowerBoundedFeeEstimator, + ) -> Result<(ChannelPublicKeys, PublicKey), ChannelError> { if self.holder_commitment_point.current_point().is_none() { return Err(ChannelError::WarnAndDisconnect(format!( "Channel {} commitment point needs to be advanced once before RBF", @@ -13022,36 +13012,26 @@ where return Err(ChannelError::Abort(AbortReason::InsufficientRbfFeerate)); } - let their_funding_contribution = match msg.funding_output_contribution { - Some(value) => SignedAmount::from_sat(value), - None => SignedAmount::ZERO, - }; - - self.validate_splice_contributions(our_funding_contribution, their_funding_contribution) - .map_err(|e| ChannelError::WarnAndDisconnect(e))?; - // Reuse funding pubkeys from the last negotiated candidate since all RBF candidates // for the same splice share the same funding output script. - let holder_pubkeys = last_candidate.get_holder_pubkeys().clone(); - let counterparty_funding_pubkey = *last_candidate.counterparty_funding_pubkey(); - - Ok(FundingScope::for_splice( - &self.funding, - &self.context, - our_funding_contribution, - their_funding_contribution, - counterparty_funding_pubkey, - holder_pubkeys, + Ok(( + last_candidate.get_holder_pubkeys().clone(), + *last_candidate.counterparty_funding_pubkey(), )) } pub(crate) fn tx_init_rbf( &mut self, msg: &msgs::TxInitRbf, entropy_source: &ES, holder_node_id: &PublicKey, fee_estimator: &LowerBoundedFeeEstimator, logger: &L, - ) -> Result { + ) -> Result { + let (holder_pubkeys, counterparty_funding_pubkey) = self + .validate_tx_init_rbf(msg, fee_estimator) + .map_err(|e| self.quiescent_negotiation_err(e))?; + let feerate = FeeRate::from_sat_per_kwu(msg.feerate_sat_per_1000_weight as u64); - let (queued_net_value, holder_balance) = - self.resolve_queued_contribution(feerate, logger)?; + let (queued_net_value, holder_balance) = self + .resolve_queued_contribution(feerate, logger) + .map_err(|e| self.quiescent_negotiation_err(e))?; // If no queued contribution, try prior contribution from previous negotiation. // Failing here means the RBF would erase our splice — reject it. @@ -13068,19 +13048,31 @@ where prior .net_value_for_acceptor_at_feerate(feerate, holder_balance) .map_err(|_| ChannelError::Abort(AbortReason::InsufficientRbfFeerate)) - })?; + }) + .map_err(|e| self.quiescent_negotiation_err(e))?; Some(net_value) } else { None }; let our_funding_contribution = queued_net_value.or(prior_net_value); + let our_funding_contribution = our_funding_contribution.unwrap_or(SignedAmount::ZERO); - let rbf_funding = self.validate_tx_init_rbf( - msg, - our_funding_contribution.unwrap_or(SignedAmount::ZERO), - fee_estimator, - )?; + let their_funding_contribution = match msg.funding_output_contribution { + Some(value) => SignedAmount::from_sat(value), + None => SignedAmount::ZERO, + }; + self.validate_splice_contributions(our_funding_contribution, their_funding_contribution) + .map_err(|e| self.quiescent_negotiation_err(ChannelError::WarnAndDisconnect(e)))?; + + let rbf_funding = FundingScope::for_splice( + &self.funding, + &self.context, + our_funding_contribution, + their_funding_contribution, + counterparty_funding_pubkey, + holder_pubkeys, + ); // Consume the appropriate contribution source. let (our_funding_inputs, our_funding_outputs) = if queued_net_value.is_some() { @@ -13117,8 +13109,6 @@ where Default::default() }; - let our_funding_contribution = our_funding_contribution.unwrap_or(SignedAmount::ZERO); - log_info!( logger, "Starting RBF funding negotiation for channel {} after receiving tx_init_rbf; channel value: {} sats", @@ -14237,8 +14227,6 @@ where Some(msgs::Stfu { channel_id: self.context.channel_id, initiator }) } - #[cfg(any(test, fuzzing, feature = "_test_utils"))] - #[rustfmt::skip] pub fn exit_quiescence(&mut self) -> bool { // Make sure we either finished the quiescence handshake and are quiescent, or we never // attempted to initiate quiescence at all. @@ -14251,6 +14239,14 @@ where was_quiescent } + fn quiescent_negotiation_err(&mut self, err: ChannelError) -> InteractiveTxMsgError { + if matches!(err, ChannelError::Abort(_)) { + debug_assert!(self.context.channel_state.is_quiescent()); + self.exit_quiescence(); + } + InteractiveTxMsgError { err, splice_funding_failed: None } + } + pub fn remove_legacy_scids_before_block(&mut self, height: u32) -> alloc::vec::Drain<'_, u64> { let end = self .funding diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 2e782701e47..f2bd2675136 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1027,7 +1027,6 @@ struct MsgHandleErrInternal { closes_channel: bool, shutdown_finish: Option<(ShutdownResult, Option<(msgs::ChannelUpdate, NodeId, NodeId)>)>, tx_abort: Option, - exited_quiescence: bool, } impl MsgHandleErrInternal { @@ -1042,7 +1041,6 @@ impl MsgHandleErrInternal { closes_channel: false, shutdown_finish: None, tx_abort: None, - exited_quiescence: false, } } @@ -1062,13 +1060,7 @@ impl MsgHandleErrInternal { } fn from_no_close(err: msgs::LightningError) -> Self { - Self { - err, - closes_channel: false, - shutdown_finish: None, - tx_abort: None, - exited_quiescence: false, - } + Self { err, closes_channel: false, shutdown_finish: None, tx_abort: None } } fn from_finish_shutdown( @@ -1089,7 +1081,6 @@ impl MsgHandleErrInternal { closes_channel: true, shutdown_finish: Some((shutdown_res, channel_update)), tx_abort: None, - exited_quiescence: false, } } @@ -1125,13 +1116,7 @@ impl MsgHandleErrInternal { }, }, }; - Self { - err, - closes_channel: false, - shutdown_finish: None, - tx_abort, - exited_quiescence: false, - } + Self { err, closes_channel: false, shutdown_finish: None, tx_abort } } fn dont_send_error_message(&mut self) { @@ -1148,9 +1133,11 @@ impl MsgHandleErrInternal { self.closes_channel } - fn with_exited_quiescence(mut self, exited_quiescence: bool) -> Self { - self.exited_quiescence = exited_quiescence; - self + /// Whether the holding cell should be released after handling this error. This is inferred + /// from the presence of a `tx_abort`, which is sent when aborting an interactive transaction + /// negotiation that was conducted during quiescence. + fn needs_holding_cell_release(&self) -> bool { + self.tx_abort.is_some() } } @@ -4569,6 +4556,7 @@ impl< internal.map_err(|err_internal| { let mut msg_event = None; + let needs_holding_cell_release = err_internal.needs_holding_cell_release(); if let Some((shutdown_res, update_option)) = err_internal.shutdown_finish { let counterparty_node_id = shutdown_res.counterparty_node_id; @@ -4610,7 +4598,7 @@ impl< } let mut holding_cell_res = None; - if msg_event.is_some() || err_internal.exited_quiescence { + if msg_event.is_some() || needs_holding_cell_release { let per_peer_state = self.per_peer_state.read().unwrap(); if let Some(peer_state_mutex) = per_peer_state.get(&counterparty_node_id) { let mut peer_state = peer_state_mutex.lock().unwrap(); @@ -4621,8 +4609,7 @@ impl< } // We need to enqueue the `tx_abort` in `pending_msg_events` above before we // enqueue any commitment updates generated by freeing holding cell HTLCs. - holding_cell_res = err_internal - .exited_quiescence + holding_cell_res = needs_holding_cell_release .then(|| self.check_free_peer_holding_cells(&mut peer_state)); } } @@ -11823,6 +11810,36 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ } } + fn handle_interactive_tx_msg_err( + &self, err: InteractiveTxMsgError, channel_id: ChannelId, counterparty_node_id: &PublicKey, + user_channel_id: u128, + ) -> MsgHandleErrInternal { + if let Some(splice_funding_failed) = err.splice_funding_failed { + let pending_events = &mut self.pending_events.lock().unwrap(); + pending_events.push_back(( + events::Event::SpliceFailed { + channel_id, + counterparty_node_id: *counterparty_node_id, + user_channel_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, + }, + }, + None, + )); + } + MsgHandleErrInternal::from_chan_no_close(err.err, channel_id) + } + fn internal_tx_msg< HandleTxMsgFn: Fn(&mut Channel) -> Result, >( @@ -11844,38 +11861,14 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ peer_state.pending_msg_events.push(msg_send_event); Ok(()) }, - Err(InteractiveTxMsgError { - err, - splice_funding_failed, - exited_quiescence, - }) => { - if let Some(splice_funding_failed) = splice_funding_failed { - let pending_events = &mut self.pending_events.lock().unwrap(); - pending_events.push_back(( - events::Event::SpliceFailed { - 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, - }, - }, - None, - )); - } - debug_assert!(!exited_quiescence || matches!(err, ChannelError::Abort(_))); - - Err(MsgHandleErrInternal::from_chan_no_close(err, channel_id) - .with_exited_quiescence(exited_quiescence)) + Err(err) => { + let user_channel_id = channel.context().get_user_id(); + Err(self.handle_interactive_tx_msg_err( + err, + channel_id, + counterparty_node_id, + user_channel_id, + )) }, } }, @@ -12003,11 +11996,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ Ok(()) }, - Err(InteractiveTxMsgError { - err, - splice_funding_failed, - exited_quiescence, - }) => { + Err(InteractiveTxMsgError { err, splice_funding_failed }) => { if let Some(splice_funding_failed) = splice_funding_failed { let pending_events = &mut self.pending_events.lock().unwrap(); pending_events.push_back(( @@ -12031,10 +12020,8 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ None, )); } - debug_assert!(!exited_quiescence || matches!(err, ChannelError::Abort(_))); - Err(MsgHandleErrInternal::from_chan_no_close(err, msg.channel_id) - .with_exited_quiescence(exited_quiescence)) + Err(MsgHandleErrInternal::from_chan_no_close(err, msg.channel_id)) }, } }, @@ -12105,7 +12092,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ } // We consider a splice negotiated when we exchange `tx_signatures`, // which also terminates quiescence. - let exited_quiescence = splice_negotiated.is_some(); + let needs_holding_cell_release = splice_negotiated.is_some(); if let Some(splice_negotiated) = splice_negotiated { self.pending_events.lock().unwrap().push_back(( events::Event::SplicePending { @@ -12120,7 +12107,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ None, )); } - let holding_cell_res = if exited_quiescence { + let holding_cell_res = if needs_holding_cell_release { self.check_free_peer_holding_cells(peer_state) } else { Vec::new() @@ -12162,7 +12149,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ match peer_state.channel_by_id.entry(msg.channel_id) { hash_map::Entry::Occupied(mut chan_entry) => { let res = chan_entry.get_mut().tx_abort(msg, &self.logger); - let (tx_abort, splice_failed, exited_quiescence) = + let (tx_abort, splice_failed) = try_channel_entry!(self, peer_state, res, chan_entry); let persist = if tx_abort.is_some() || splice_failed.is_some() { @@ -12171,6 +12158,9 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ NotifyOption::SkipPersistNoEvents }; + // Release any HTLCs held during quiescence now that we're + // exiting via tx_abort. + let needs_holding_cell_release = tx_abort.is_some(); if let Some(tx_abort_msg) = tx_abort { peer_state.pending_msg_events.push(MessageSendEvent::SendTxAbort { node_id: *counterparty_node_id, @@ -12202,7 +12192,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ )); } - let holding_cell_res = if exited_quiescence { + let holding_cell_res = if needs_holding_cell_release { self.check_free_peer_holding_cells(peer_state) } else { Vec::new() @@ -13258,18 +13248,30 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ } if let Some(ref mut funded_channel) = chan_entry.get_mut().as_funded_mut() { - let init_res = funded_channel.splice_init( + let user_channel_id = funded_channel.context.get_user_id(); + match funded_channel.splice_init( msg, &self.entropy_source, &self.get_our_node_id(), &self.logger, - ); - let splice_ack_msg = try_channel_entry!(self, peer_state, init_res, chan_entry); - peer_state.pending_msg_events.push(MessageSendEvent::SendSpliceAck { - node_id: *counterparty_node_id, - msg: splice_ack_msg, - }); - Ok(()) + ) { + Ok(splice_ack_msg) => { + peer_state.pending_msg_events.push(MessageSendEvent::SendSpliceAck { + node_id: *counterparty_node_id, + msg: splice_ack_msg, + }); + Ok(()) + }, + Err(err) => { + debug_assert!(err.splice_funding_failed.is_none()); + Err(self.handle_interactive_tx_msg_err( + err, + msg.channel_id, + counterparty_node_id, + user_channel_id, + )) + }, + } } else { try_channel_entry!( self, @@ -13302,19 +13304,31 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ }, hash_map::Entry::Occupied(mut chan_entry) => { if let Some(ref mut funded_channel) = chan_entry.get_mut().as_funded_mut() { - let init_res = funded_channel.tx_init_rbf( + let user_channel_id = funded_channel.context.get_user_id(); + match funded_channel.tx_init_rbf( msg, &self.entropy_source, &self.get_our_node_id(), &self.fee_estimator, &self.logger, - ); - let tx_ack_rbf_msg = try_channel_entry!(self, peer_state, init_res, chan_entry); - peer_state.pending_msg_events.push(MessageSendEvent::SendTxAckRbf { - node_id: *counterparty_node_id, - msg: tx_ack_rbf_msg, - }); - Ok(()) + ) { + Ok(tx_ack_rbf_msg) => { + peer_state.pending_msg_events.push(MessageSendEvent::SendTxAckRbf { + node_id: *counterparty_node_id, + msg: tx_ack_rbf_msg, + }); + Ok(()) + }, + Err(err) => { + debug_assert!(err.splice_funding_failed.is_none()); + Err(self.handle_interactive_tx_msg_err( + err, + msg.channel_id, + counterparty_node_id, + user_channel_id, + )) + }, + } } else { try_channel_entry!( self, diff --git a/lightning/src/ln/splicing_tests.rs b/lightning/src/ln/splicing_tests.rs index 9adccd17627..df8f9d0b690 100644 --- a/lightning/src/ln/splicing_tests.rs +++ b/lightning/src/ln/splicing_tests.rs @@ -16,7 +16,8 @@ use crate::chain::ChannelMonitorUpdateStatus; use crate::events::{ClosureReason, Event, FundingInfo, HTLCHandlingFailureType}; use crate::ln::chan_utils; use crate::ln::channel::{ - CHANNEL_ANNOUNCEMENT_PROPAGATION_DELAY, FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE, + CHANNEL_ANNOUNCEMENT_PROPAGATION_DELAY, DISCONNECT_PEER_AWAITING_RESPONSE_TICKS, + FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE, }; use crate::ln::channelmanager::{provided_init_features, PaymentId, BREAKDOWN_TIMEOUT}; use crate::ln::functional_test_utils::*; @@ -1816,10 +1817,25 @@ fn test_splice_tiebreak_feerate_too_high_rejected() { let splice_init = get_event_msg!(nodes[0], MessageSendEvent::SendSpliceInit, node_id_1); // Node 1 handles SpliceInit — TooHigh: target (100k) >> max (3k) and fair fee > budget. + // Node 1 exits quiescence upon rejecting with tx_abort, and since it has a pending + // QuiescentAction (from its own splice attempt), it immediately re-proposes quiescence. nodes[1].node.handle_splice_init(node_id_0, &splice_init); - let tx_abort = get_event_msg!(nodes[1], MessageSendEvent::SendTxAbort, node_id_0); - assert_eq!(tx_abort.channel_id, channel_id); + let msg_events = nodes[1].node.get_and_clear_pending_msg_events(); + assert_eq!(msg_events.len(), 2); + match &msg_events[0] { + MessageSendEvent::SendTxAbort { node_id, msg } => { + assert_eq!(*node_id, node_id_0); + assert_eq!(msg.channel_id, channel_id); + }, + _ => panic!("Expected SendTxAbort, got {:?}", msg_events[0]), + }; + match &msg_events[1] { + MessageSendEvent::SendStfu { node_id, .. } => { + assert_eq!(*node_id, node_id_0); + }, + _ => panic!("Expected SendStfu, got {:?}", msg_events[1]), + }; } #[cfg(test)] @@ -4586,33 +4602,85 @@ fn test_splice_rbf_insufficient_feerate() { .is_ok()); // Acceptor-side: tx_init_rbf with an insufficient feerate is also rejected. - reenter_quiescence(&nodes[0], &nodes[1], &channel_id); + // Node 0 initiates a proper RBF but we tamper the feerate to be insufficient. + provide_utxo_reserves(&nodes, 2, added_value * 2); + let _funding_contribution = + do_initiate_rbf_splice_in(&nodes[0], &nodes[1], channel_id, added_value, min_rbf_feerate); - let tx_init_rbf = msgs::TxInitRbf { - channel_id, - locktime: 0, - feerate_sat_per_1000_weight: FEERATE_FLOOR_SATS_PER_KW, - funding_output_contribution: Some(added_value.to_sat() as i64), - }; + let stfu_0 = get_event_msg!(nodes[0], MessageSendEvent::SendStfu, node_id_1); + nodes[1].node.handle_stfu(node_id_0, &stfu_0); + let stfu_1 = get_event_msg!(nodes[1], MessageSendEvent::SendStfu, node_id_0); + nodes[0].node.handle_stfu(node_id_1, &stfu_1); + let mut tx_init_rbf = get_event_msg!(nodes[0], MessageSendEvent::SendTxInitRbf, node_id_1); + tx_init_rbf.feerate_sat_per_1000_weight = FEERATE_FLOOR_SATS_PER_KW; nodes[1].node.handle_tx_init_rbf(node_id_0, &tx_init_rbf); let tx_abort = get_event_msg!(nodes[1], MessageSendEvent::SendTxAbort, node_id_0); assert_eq!(tx_abort.channel_id, channel_id); - // Acceptor-side: a counterparty feerate that satisfies the spec's 25/24 rule (264) is - // accepted, even though our own RBF floor (+25 sat/kwu = 278) is higher. - // After tx_abort the channel remains quiescent, so no need to re-enter quiescence. + // Queue a payment while quiescent. It should go to the holding cell and be freed once + // quiescence is exited by the tx_abort exchange. + let (route, payment_hash, _payment_preimage, payment_secret) = + get_route_and_payment_hash!(nodes[0], nodes[1], 1_000_000); + let onion = RecipientOnionFields::secret_only(payment_secret, 1_000_000); + let payment_id = PaymentId(payment_hash.0); + nodes[0].node.send_payment_with_route(route, payment_hash, onion, payment_id).unwrap(); + assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); + + // Node 0 echoes tx_abort and exits quiescence, freeing the holding cell. nodes[0].node.handle_tx_abort(node_id_1, &tx_abort); - let rbf_feerate_25_24 = ((FEERATE_FLOOR_SATS_PER_KW as u64) * 25).div_ceil(24) as u32; - let tx_init_rbf = msgs::TxInitRbf { - channel_id, - locktime: 0, - feerate_sat_per_1000_weight: rbf_feerate_25_24, - funding_output_contribution: Some(added_value.to_sat() as i64), + let events = nodes[0].node.get_and_clear_pending_events(); + assert_eq!(events.len(), 2, "{events:?}"); + assert!( + matches!(&events[0], Event::SpliceFailed { channel_id: cid, .. } if *cid == channel_id) + ); + assert!( + matches!(&events[1], Event::DiscardFunding { channel_id: cid, .. } if *cid == channel_id) + ); + + let msg_events = nodes[0].node.get_and_clear_pending_msg_events(); + assert_eq!(msg_events.len(), 2, "{msg_events:?}"); + let tx_abort_echo = match &msg_events[0] { + MessageSendEvent::SendTxAbort { msg, .. } => msg.clone(), + other => panic!("Expected SendTxAbort, got {:?}", other), }; + match &msg_events[1] { + MessageSendEvent::UpdateHTLCs { updates, .. } => { + assert_eq!(updates.update_add_htlcs.len(), 1); + }, + other => panic!("Expected UpdateHTLCs, got {:?}", other), + } + + // Complete the HTLC commitment exchange so the channel is ready for the next RBF attempt. + // The holding cell free generated a monitor update for the outgoing HTLC. + check_added_monitors(&nodes[0], 1); + if let MessageSendEvent::UpdateHTLCs { updates, .. } = &msg_events[1] { + nodes[1].node.handle_update_add_htlc(node_id_0, &updates.update_add_htlcs[0]); + do_commitment_signed_dance(&nodes[1], &nodes[0], &updates.commitment_signed, false, false); + } else { + unreachable!(); + } + + // Node 1 handles the echo (no-op since it already aborted). + nodes[1].node.handle_tx_abort(node_id_0, &tx_abort_echo); + + // Acceptor-side: a counterparty feerate that satisfies the spec's 25/24 rule (264) is + // accepted, even though our own RBF floor (+25 sat/kwu = 278) is higher. + // Node 0 initiates another proper RBF but we tamper the feerate to the 25/24 value. + provide_utxo_reserves(&nodes, 2, added_value * 2); + let _funding_contribution = + do_initiate_rbf_splice_in(&nodes[0], &nodes[1], channel_id, added_value, min_rbf_feerate); + + let stfu_0 = get_event_msg!(nodes[0], MessageSendEvent::SendStfu, node_id_1); + nodes[1].node.handle_stfu(node_id_0, &stfu_0); + let stfu_1 = get_event_msg!(nodes[1], MessageSendEvent::SendStfu, node_id_0); + nodes[0].node.handle_stfu(node_id_1, &stfu_1); + let mut tx_init_rbf = get_event_msg!(nodes[0], MessageSendEvent::SendTxInitRbf, node_id_1); + let rbf_feerate_25_24 = ((FEERATE_FLOOR_SATS_PER_KW as u64) * 25).div_ceil(24) as u32; + tx_init_rbf.feerate_sat_per_1000_weight = rbf_feerate_25_24; nodes[1].node.handle_tx_init_rbf(node_id_0, &tx_init_rbf); let _tx_ack_rbf = get_event_msg!(nodes[1], MessageSendEvent::SendTxAckRbf, node_id_0); } @@ -5300,10 +5368,25 @@ fn test_splice_rbf_tiebreak_feerate_too_high_rejected() { assert_eq!(tx_init_rbf.feerate_sat_per_1000_weight, high_feerate.to_sat_per_kwu() as u32); // Node 1 handles tx_init_rbf — TooHigh: target (100k) >> max (3k) and fair fee > budget. + // Node 1 exits quiescence upon rejecting with tx_abort, and since it has a pending + // QuiescentAction (from its own splice RBF attempt), it immediately re-proposes quiescence. nodes[1].node.handle_tx_init_rbf(node_id_0, &tx_init_rbf); - let tx_abort = get_event_msg!(nodes[1], MessageSendEvent::SendTxAbort, node_id_0); - assert_eq!(tx_abort.channel_id, channel_id); + let msg_events = nodes[1].node.get_and_clear_pending_msg_events(); + assert_eq!(msg_events.len(), 2); + match &msg_events[0] { + MessageSendEvent::SendTxAbort { node_id, msg } => { + assert_eq!(*node_id, node_id_0); + assert_eq!(msg.channel_id, channel_id); + }, + _ => panic!("Expected SendTxAbort, got {:?}", msg_events[0]), + }; + match &msg_events[1] { + MessageSendEvent::SendStfu { node_id, .. } => { + assert_eq!(*node_id, node_id_0); + }, + _ => panic!("Expected SendStfu, got {:?}", msg_events[1]), + }; } #[test] @@ -6501,6 +6584,96 @@ fn test_splice_revalidation_at_quiescence() { expect_splice_failed_events(&nodes[0], &channel_id, contribution); } +#[test] +fn test_splice_init_before_quiescence_sends_warning() { + // A misbehaving peer sends splice_init before quiescence is established. The receiver + // should send a warning and disconnect. + 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_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); + + // Node 0 initiates quiescence. + nodes[0].node.maybe_propose_quiescence(&node_id_1, &channel_id).unwrap(); + let _stfu = get_event_msg!(nodes[0], MessageSendEvent::SendStfu, node_id_1); + + // Misbehaving node 1 sends splice_init before completing the STFU handshake. + let funding_pubkey = + PublicKey::from_secret_key(&Secp256k1::new(), &SecretKey::from_slice(&[42; 32]).unwrap()); + let splice_init = msgs::SpliceInit { + channel_id, + funding_contribution_satoshis: 50_000, + funding_feerate_per_kw: FEERATE_FLOOR_SATS_PER_KW, + locktime: 0, + funding_pubkey, + require_confirmed_inputs: None, + }; + nodes[0].node.handle_splice_init(node_id_1, &splice_init); + + // Node 0 should send a warning and disconnect. + let msg_events = nodes[0].node.get_and_clear_pending_msg_events(); + assert_eq!(msg_events.len(), 1); + match &msg_events[0] { + MessageSendEvent::HandleError { node_id, .. } => assert_eq!(*node_id, node_id_1), + other => panic!("Expected HandleError, got {:?}", other), + } +} + +#[test] +fn test_tx_init_rbf_before_quiescence_sends_warning() { + // A misbehaving peer sends tx_init_rbf before quiescence is established. The receiver + // should send a warning and disconnect. + 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_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 so there's a pending splice to RBF. + let funding_contribution = do_initiate_splice_in(&nodes[0], &nodes[1], channel_id, added_value); + let (_splice_tx, _new_funding_script) = + splice_channel(&nodes[0], &nodes[1], channel_id, funding_contribution); + + // Node 0 initiates quiescence. + nodes[0].node.maybe_propose_quiescence(&node_id_1, &channel_id).unwrap(); + let _stfu = get_event_msg!(nodes[0], MessageSendEvent::SendStfu, node_id_1); + + // Misbehaving node 1 sends tx_init_rbf before completing the STFU handshake. + let tx_init_rbf = msgs::TxInitRbf { + channel_id, + locktime: 0, + feerate_sat_per_1000_weight: FEERATE_FLOOR_SATS_PER_KW + 25, + funding_output_contribution: Some(added_value.to_sat() as i64), + }; + nodes[0].node.handle_tx_init_rbf(node_id_1, &tx_init_rbf); + + // Node 0 should send a warning and disconnect. + let msg_events = nodes[0].node.get_and_clear_pending_msg_events(); + assert_eq!(msg_events.len(), 1); + match &msg_events[0] { + MessageSendEvent::HandleError { node_id, .. } => assert_eq!(*node_id, node_id_1), + other => panic!("Expected HandleError, got {:?}", other), + } + + // Clean up events from the splice setup. + nodes[0].node.get_and_clear_pending_events(); + nodes[1].node.get_and_clear_pending_events(); +} + #[test] fn test_splice_rbf_rejects_low_feerate_after_several_attempts() { // After several RBF attempts, the counterparty's RBF feerate must be high enough to @@ -6647,3 +6820,194 @@ fn test_splice_rbf_rejects_own_low_feerate_after_several_attempts() { other => panic!("Expected SpliceFailed, got {:?}", other), } } + +#[test] +fn test_no_disconnect_after_splice_completes() { + // Test that the disconnect timer is cleared when exiting quiescence after a successful splice + // negotiation. Previously, `on_tx_signatures_exchange` cleared the quiescent state but not the + // disconnect timer, causing a spurious disconnect after the splice completed. + 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 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); + + let funding_contribution = do_initiate_splice_in(&nodes[0], &nodes[1], channel_id, added_value); + let new_funding_script = complete_splice_handshake(&nodes[0], &nodes[1]); + + // Fire a tick while quiescent to arm the disconnect timer. + nodes[0].node.timer_tick_occurred(); + nodes[1].node.timer_tick_occurred(); + + // Complete the splice negotiation, which should clear the timer when exiting quiescence. + complete_interactive_funding_negotiation( + &nodes[0], + &nodes[1], + channel_id, + funding_contribution, + new_funding_script, + ); + let (_, splice_locked) = sign_interactive_funding_tx(&nodes[0], &nodes[1], false); + assert!(splice_locked.is_none()); + + let node_id_0 = nodes[0].node.get_our_node_id(); + let node_id_1 = nodes[1].node.get_our_node_id(); + expect_splice_pending_event(&nodes[0], &node_id_1); + expect_splice_pending_event(&nodes[1], &node_id_0); + + // Fire enough ticks to trigger a disconnect if the timer wasn't properly cleared. + for _ in 0..DISCONNECT_PEER_AWAITING_RESPONSE_TICKS { + nodes[0].node.timer_tick_occurred(); + nodes[1].node.timer_tick_occurred(); + } + + let has_disconnect = |events: &[MessageSendEvent]| { + events.iter().any(|event| { + matches!( + event, + MessageSendEvent::HandleError { + action: msgs::ErrorAction::DisconnectPeerWithWarning { .. }, + .. + } + ) + }) + }; + assert!(!has_disconnect(&nodes[0].node.get_and_clear_pending_msg_events())); + assert!(!has_disconnect(&nodes[1].node.get_and_clear_pending_msg_events())); +} + +#[test] +fn test_no_disconnect_after_splice_aborted() { + // Test that the disconnect timer is cleared when exiting quiescence after a splice negotiation + // is aborted via tx_abort. Previously, `reset_pending_splice_state` cleared the quiescent + // state but not the disconnect timer, causing a spurious disconnect after the abort. + 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); + + let funding_contribution = do_initiate_splice_in(&nodes[0], &nodes[1], channel_id, added_value); + complete_splice_handshake(&nodes[0], &nodes[1]); + + // Fire a tick while quiescent to arm the disconnect timer. + nodes[0].node.timer_tick_occurred(); + nodes[1].node.timer_tick_occurred(); + + // Abort the splice, which should clear the timer when exiting quiescence. + nodes[0].node.abandon_splice(&channel_id, &node_id_1).unwrap(); + + expect_splice_failed_events(&nodes[0], &channel_id, funding_contribution); + + let msg_events = nodes[0].node.get_and_clear_pending_msg_events(); + let tx_abort = msg_events + .iter() + .find_map(|event| { + if let MessageSendEvent::SendTxAbort { msg, .. } = event { + Some(msg.clone()) + } else { + None + } + }) + .expect("Expected SendTxAbort"); + + nodes[1].node.handle_tx_abort(node_id_0, &tx_abort); + let tx_abort_echo = get_event_msg!(nodes[1], MessageSendEvent::SendTxAbort, node_id_0); + nodes[1].node.get_and_clear_pending_events(); + + nodes[0].node.handle_tx_abort(node_id_1, &tx_abort_echo); + + // Fire enough ticks to trigger a disconnect if the timer wasn't properly cleared. + for _ in 0..DISCONNECT_PEER_AWAITING_RESPONSE_TICKS { + nodes[0].node.timer_tick_occurred(); + nodes[1].node.timer_tick_occurred(); + } + + let has_disconnect = |events: &[MessageSendEvent]| { + events.iter().any(|event| { + matches!( + event, + MessageSendEvent::HandleError { + action: msgs::ErrorAction::DisconnectPeerWithWarning { .. }, + .. + } + ) + }) + }; + assert!(!has_disconnect(&nodes[0].node.get_and_clear_pending_msg_events())); + assert!(!has_disconnect(&nodes[1].node.get_and_clear_pending_msg_events())); +} + +#[test] +fn test_no_disconnect_after_quiescence_on_reconnect() { + // Test that there is no spurious disconnect after reconnecting from a quiescent state. The + // disconnect timer is cleared by `remove_uncommitted_htlcs_and_mark_paused` during + // disconnection and by `exit_quiescence` during reconnection. + 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); + + let funding_contribution = do_initiate_splice_in(&nodes[0], &nodes[1], channel_id, added_value); + complete_splice_handshake(&nodes[0], &nodes[1]); + + // Fire a tick while quiescent to arm the disconnect timer. + nodes[0].node.timer_tick_occurred(); + nodes[1].node.timer_tick_occurred(); + + // Disconnect and reconnect. + 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); + + let mut reconnect_args = ReconnectArgs::new(&nodes[0], &nodes[1]); + reconnect_args.send_channel_ready = (true, true); + reconnect_args.send_announcement_sigs = (true, true); + reconnect_nodes(reconnect_args); + + // Fire enough ticks to trigger a disconnect if the timer wasn't properly cleared. + for _ in 0..DISCONNECT_PEER_AWAITING_RESPONSE_TICKS { + nodes[0].node.timer_tick_occurred(); + nodes[1].node.timer_tick_occurred(); + } + + let has_disconnect = |events: &[MessageSendEvent]| { + events.iter().any(|event| { + matches!( + event, + MessageSendEvent::HandleError { + action: msgs::ErrorAction::DisconnectPeerWithWarning { .. }, + .. + } + ) + }) + }; + assert!(!has_disconnect(&nodes[0].node.get_and_clear_pending_msg_events())); + assert!(!has_disconnect(&nodes[1].node.get_and_clear_pending_msg_events())); +}