Free holding cell upon handling an invalid interactive-tx message

wpaulino · wpaulino · commit 3e9a10d31e03 · 2026-02-12T16:57:23.000-08:00
After cad88af, a few code paths that also lead to a quiescence exit were not accounted for. This commit addresses the path where we exit quiescence due to a processing error on a counterparty's `tx_add_input/output`, `tx_remove_input/output`, or `tx_complete` message.
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
@@ -1176,6 +1176,18 @@ pub enum UpdateFulfillCommitFetch {
 	DuplicateClaim {},
 }
 
+/// Error returned when processing an invalid interactive-tx message from our counterparty.
+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.
+	pub(super) splice_funding_failed: Option<SpliceFundingFailed>,
+	/// 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`
 pub(super) struct MonitorRestoreUpdates {
 	pub raa: Option<msgs::RevokeAndACK>,
@@ -1818,104 +1830,118 @@ where
 
 	fn fail_interactive_tx_negotiation<L: Logger>(
 		&mut self, reason: AbortReason, logger: &L,
-	) -> (ChannelError, Option<SpliceFundingFailed>) {
+	) -> InteractiveTxMsgError {
 		let logger = WithChannelContext::from(logger, &self.context(), None);
 		log_info!(logger, "Failed interactive transaction negotiation: {reason}");
 
-		let splice_funding_failed = match &mut self.phase {
+		let (splice_funding_failed, exited_quiescence) = match &mut self.phase {
 			ChannelPhase::Undefined => unreachable!(),
-			ChannelPhase::UnfundedOutboundV1(_) | ChannelPhase::UnfundedInboundV1(_) => None,
+			ChannelPhase::UnfundedOutboundV1(_) | ChannelPhase::UnfundedInboundV1(_) => {
+				(None, false)
+			},
 			ChannelPhase::UnfundedV2(pending_v2_channel) => {
 				pending_v2_channel.interactive_tx_constructor.take();
-				None
+				(None, false)
 			},
 			ChannelPhase::Funded(funded_channel) => {
 				if funded_channel.should_reset_pending_splice_state(false) {
-					funded_channel.reset_pending_splice_state()
+					(funded_channel.reset_pending_splice_state(), true)
 				} else {
 					debug_assert!(false, "We should never fail an interactive funding negotiation once we're exchanging tx_signatures");
-					None
+					(None, false)
 				}
 			},
 		};
 
-		(ChannelError::Abort(reason), splice_funding_failed)
+		InteractiveTxMsgError {
+			err: ChannelError::Abort(reason),
+			splice_funding_failed,
+			exited_quiescence,
+		}
 	}
 
 	pub fn tx_add_input<L: Logger>(
 		&mut self, msg: &msgs::TxAddInput, logger: &L,
-	) -> Result<InteractiveTxMessageSend, (ChannelError, Option<SpliceFundingFailed>)> {
+	) -> Result<InteractiveTxMessageSend, InteractiveTxMsgError> {
 		match self.interactive_tx_constructor_mut() {
 			Some(interactive_tx_constructor) => interactive_tx_constructor
 				.handle_tx_add_input(msg)
 				.map_err(|reason| self.fail_interactive_tx_negotiation(reason, logger)),
-			None => Err((
-				ChannelError::WarnAndDisconnect(
+			None => Err(InteractiveTxMsgError {
+				err: ChannelError::WarnAndDisconnect(
 					"Received unexpected interactive transaction negotiation message".to_owned(),
 				),
-				None,
-			)),
+				splice_funding_failed: None,
+				exited_quiescence: false,
+			}),
 		}
 	}
 
 	pub fn tx_add_output<L: Logger>(
 		&mut self, msg: &msgs::TxAddOutput, logger: &L,
-	) -> Result<InteractiveTxMessageSend, (ChannelError, Option<SpliceFundingFailed>)> {
+	) -> Result<InteractiveTxMessageSend, InteractiveTxMsgError> {
 		match self.interactive_tx_constructor_mut() {
 			Some(interactive_tx_constructor) => interactive_tx_constructor
 				.handle_tx_add_output(msg)
 				.map_err(|reason| self.fail_interactive_tx_negotiation(reason, logger)),
-			None => Err((
-				ChannelError::WarnAndDisconnect(
+			None => Err(InteractiveTxMsgError {
+				err: ChannelError::WarnAndDisconnect(
 					"Received unexpected interactive transaction negotiation message".to_owned(),
 				),
-				None,
-			)),
+				splice_funding_failed: None,
+				exited_quiescence: false,
+			}),
 		}
 	}
 
 	pub fn tx_remove_input<L: Logger>(
 		&mut self, msg: &msgs::TxRemoveInput, logger: &L,
-	) -> Result<InteractiveTxMessageSend, (ChannelError, Option<SpliceFundingFailed>)> {
+	) -> Result<InteractiveTxMessageSend, InteractiveTxMsgError> {
 		match self.interactive_tx_constructor_mut() {
 			Some(interactive_tx_constructor) => interactive_tx_constructor
 				.handle_tx_remove_input(msg)
 				.map_err(|reason| self.fail_interactive_tx_negotiation(reason, logger)),
-			None => Err((
-				ChannelError::WarnAndDisconnect(
+			None => Err(InteractiveTxMsgError {
+				err: ChannelError::WarnAndDisconnect(
 					"Received unexpected interactive transaction negotiation message".to_owned(),
 				),
-				None,
-			)),
+				splice_funding_failed: None,
+				exited_quiescence: false,
+			}),
 		}
 	}
 
 	pub fn tx_remove_output<L: Logger>(
 		&mut self, msg: &msgs::TxRemoveOutput, logger: &L,
-	) -> Result<InteractiveTxMessageSend, (ChannelError, Option<SpliceFundingFailed>)> {
+	) -> Result<InteractiveTxMessageSend, InteractiveTxMsgError> {
 		match self.interactive_tx_constructor_mut() {
 			Some(interactive_tx_constructor) => interactive_tx_constructor
 				.handle_tx_remove_output(msg)
 				.map_err(|reason| self.fail_interactive_tx_negotiation(reason, logger)),
-			None => Err((
-				ChannelError::WarnAndDisconnect(
+			None => Err(InteractiveTxMsgError {
+				err: ChannelError::WarnAndDisconnect(
 					"Received unexpected interactive transaction negotiation message".to_owned(),
 				),
-				None,
-			)),
+				splice_funding_failed: None,
+				exited_quiescence: false,
+			}),
 		}
 	}
 
 	pub fn tx_complete<F: FeeEstimator, L: Logger>(
 		&mut self, msg: &msgs::TxComplete, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L,
-	) -> Result<TxCompleteResult, (ChannelError, Option<SpliceFundingFailed>)> {
+	) -> Result<TxCompleteResult, InteractiveTxMsgError> {
 		let tx_complete_action = match self.interactive_tx_constructor_mut() {
 			Some(interactive_tx_constructor) => interactive_tx_constructor
 				.handle_tx_complete(msg)
 				.map_err(|reason| self.fail_interactive_tx_negotiation(reason, logger))?,
 			None => {
 				let err = "Received unexpected interactive transaction negotiation message";
-				return Err((ChannelError::WarnAndDisconnect(err.to_owned()), None));
+				return Err(InteractiveTxMsgError {
+					err: ChannelError::WarnAndDisconnect(err.to_owned()),
+					splice_funding_failed: None,
+					exited_quiescence: false,
+				});
 			},
 		};
 
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
@@ -59,8 +59,8 @@ use crate::ln::chan_utils::selected_commitment_sat_per_1000_weight;
 use crate::ln::channel::QuiescentAction;
 use crate::ln::channel::{
 	self, hold_time_since, Channel, ChannelError, ChannelUpdateStatus, DisconnectResult,
-	FundedChannel, FundingTxSigned, InboundUpdateAdd, InboundV1Channel, OutboundV1Channel,
-	PendingV2Channel, ReconnectionMsg, ShutdownResult, SpliceFundingFailed, StfuResponse,
+	FundedChannel, FundingTxSigned, InboundUpdateAdd, InboundV1Channel, InteractiveTxMsgError,
+	OutboundV1Channel, PendingV2Channel, ReconnectionMsg, ShutdownResult, StfuResponse,
 	UpdateFulfillCommitFetch, WithChannelContext,
 };
 use crate::ln::channel_state::ChannelDetails;
@@ -938,6 +938,7 @@ struct MsgHandleErrInternal {
 	closes_channel: bool,
 	shutdown_finish: Option<(ShutdownResult, Option<(msgs::ChannelUpdate, NodeId, NodeId)>)>,
 	tx_abort: Option<msgs::TxAbort>,
+	exited_quiescence: bool,
 }
 
 impl MsgHandleErrInternal {
@@ -952,6 +953,7 @@ impl MsgHandleErrInternal {
 			closes_channel: false,
 			shutdown_finish: None,
 			tx_abort: None,
+			exited_quiescence: false,
 		}
 	}
 
@@ -970,7 +972,13 @@ impl MsgHandleErrInternal {
 	}
 
 	fn from_no_close(err: msgs::LightningError) -> Self {
-		Self { err, closes_channel: false, shutdown_finish: None, tx_abort: None }
+		Self {
+			err,
+			closes_channel: false,
+			shutdown_finish: None,
+			tx_abort: None,
+			exited_quiescence: false,
+		}
 	}
 
 	fn from_finish_shutdown(
@@ -991,6 +999,7 @@ impl MsgHandleErrInternal {
 			closes_channel: true,
 			shutdown_finish: Some((shutdown_res, channel_update)),
 			tx_abort: None,
+			exited_quiescence: false,
 		}
 	}
 
@@ -1026,7 +1035,13 @@ impl MsgHandleErrInternal {
 				},
 			},
 		};
-		Self { err, closes_channel: false, shutdown_finish: None, tx_abort }
+		Self {
+			err,
+			closes_channel: false,
+			shutdown_finish: None,
+			tx_abort,
+			exited_quiescence: false,
+		}
 	}
 
 	fn dont_send_error_message(&mut self) {
@@ -1042,6 +1057,11 @@ impl MsgHandleErrInternal {
 	fn closes_channel(&self) -> bool {
 		self.closes_channel
 	}
+
+	fn with_exited_quiescence(mut self, exited_quiescence: bool) -> Self {
+		self.exited_quiescence = exited_quiescence;
+		self
+	}
 }
 
 /// For events which result in both a RevokeAndACK and a CommitmentUpdate, by default they should
@@ -4348,15 +4368,26 @@ impl<
 				});
 			}
 
-			if let Some(msg_event) = msg_event {
+			let mut holding_cell_res = None;
+			if msg_event.is_some() || err_internal.exited_quiescence {
 				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();
-					if peer_state.is_connected {
-						peer_state.pending_msg_events.push(msg_event);
+					if let Some(msg_event) = msg_event {
+						if peer_state.is_connected {
+							peer_state.pending_msg_events.push(msg_event);
+						}
 					}
+					// 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
+						.then(|| self.check_free_peer_holding_cells(&mut peer_state));
 				}
 			}
+			if let Some(res) = holding_cell_res {
+				self.handle_holding_cell_free_result(res);
+			}
 
 			// Return error in case higher-API need one
 			err_internal.err
@@ -11192,9 +11223,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 	}
 
 	fn internal_tx_msg<
-		HandleTxMsgFn: Fn(
-			&mut Channel<SP>,
-		) -> Result<InteractiveTxMessageSend, (ChannelError, Option<SpliceFundingFailed>)>,
+		HandleTxMsgFn: Fn(&mut Channel<SP>) -> Result<InteractiveTxMessageSend, InteractiveTxMsgError>,
 	>(
 		&self, counterparty_node_id: &PublicKey, channel_id: ChannelId,
 		tx_msg_handler: HandleTxMsgFn,
@@ -11215,7 +11244,11 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 						peer_state.pending_msg_events.push(msg_send_event);
 						Ok(NotifyOption::SkipPersistHandleEvents)
 					},
-					Err((error, splice_funding_failed)) => {
+					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((
@@ -11231,7 +11264,10 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 								None,
 							));
 						}
-						Err(MsgHandleErrInternal::from_chan_no_close(error, channel_id))
+						debug_assert!(!exited_quiescence || matches!(err, ChannelError::Abort(_)));
+
+						Err(MsgHandleErrInternal::from_chan_no_close(err, channel_id)
+							.with_exited_quiescence(exited_quiescence))
 					},
 				}
 			},
@@ -11361,7 +11397,11 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 
 						Ok(persist)
 					},
-					Err((error, splice_funding_failed)) => {
+					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((
@@ -11377,7 +11417,10 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 								None,
 							));
 						}
-						Err(MsgHandleErrInternal::from_chan_no_close(error, msg.channel_id))
+						debug_assert!(!exited_quiescence || matches!(err, ChannelError::Abort(_)));
+
+						Err(MsgHandleErrInternal::from_chan_no_close(err, msg.channel_id)
+							.with_exited_quiescence(exited_quiescence))
 					},
 				}
 			},
diff --git a/lightning/src/ln/splicing_tests.rs b/lightning/src/ln/splicing_tests.rs
