Free holding cell upon tx_signatures exchange

After cad88af, a few code paths that also lead to a quiescence exit were
not accounted for. This commit addresses the last remaining path where
we exit quiescence when we exchange `tx_signatures` with the
counterparty.

Author: wpaulino

lightning/src/ln/channelmanager.rs

@@ -11422,90 +11422,106 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 	fn internal_tx_signatures(
 		&self, counterparty_node_id: &PublicKey, msg: &msgs::TxSignatures,
 	) -> Result<(), MsgHandleErrInternal> {
-		let per_peer_state = self.per_peer_state.read().unwrap();
-		let peer_state_mutex = per_peer_state.get(counterparty_node_id).ok_or_else(|| {
-			debug_assert!(false);
-			MsgHandleErrInternal::no_such_peer(counterparty_node_id, msg.channel_id)
-		})?;
-		let mut peer_state_lock = peer_state_mutex.lock().unwrap();
-		let peer_state = &mut *peer_state_lock;
-		match peer_state.channel_by_id.entry(msg.channel_id) {
-			hash_map::Entry::Occupied(mut chan_entry) => {
-				match chan_entry.get_mut().as_funded_mut() {
-					Some(chan) => {
-						let best_block_height = self.best_block.read().unwrap().height;
-						let FundingTxSigned {
-							commitment_signed,
-							counterparty_initial_commitment_signed_result,
-							tx_signatures,
-							funding_tx,
-							splice_negotiated,
-							splice_locked,
-						} = try_channel_entry!(
-							self,
-							peer_state,
-							chan.tx_signatures(msg, best_block_height, &self.logger),
-							chan_entry
-						);
-
-						// We should never be sending a `commitment_signed` in response to their
-						// `tx_signatures`.
-						debug_assert!(commitment_signed.is_none());
-						debug_assert!(counterparty_initial_commitment_signed_result.is_none());
-
-						if let Some(tx_signatures) = tx_signatures {
-							peer_state.pending_msg_events.push(
-								MessageSendEvent::SendTxSignatures {
-									node_id: *counterparty_node_id,
-									msg: tx_signatures,
-								},
-							);
-						}
-						if let Some(splice_locked) = splice_locked {
-							peer_state.pending_msg_events.push(
-								MessageSendEvent::SendSpliceLocked {
-									node_id: *counterparty_node_id,
-									msg: splice_locked,
-								},
-							);
-						}
-						if let Some((ref funding_tx, ref tx_type)) = funding_tx {
-							self.broadcast_interactive_funding(
-								chan,
+		let (result, holding_cell_res) = {
+			let per_peer_state = self.per_peer_state.read().unwrap();
+			let peer_state_mutex = per_peer_state.get(counterparty_node_id).ok_or_else(|| {
+				debug_assert!(false);
+				MsgHandleErrInternal::no_such_peer(counterparty_node_id, msg.channel_id)
+			})?;
+			let mut peer_state_lock = peer_state_mutex.lock().unwrap();
+			let peer_state = &mut *peer_state_lock;
+			match peer_state.channel_by_id.entry(msg.channel_id) {
+				hash_map::Entry::Occupied(mut chan_entry) => {
+					match chan_entry.get_mut().as_funded_mut() {
+						Some(chan) => {
+							let best_block_height = self.best_block.read().unwrap().height;
+							let FundingTxSigned {
+								commitment_signed,
+								counterparty_initial_commitment_signed_result,
+								tx_signatures,
 								funding_tx,
-								Some(tx_type.clone()),
-								&self.logger,
+								splice_negotiated,
+								splice_locked,
+							} = try_channel_entry!(
+								self,
+								peer_state,
+								chan.tx_signatures(msg, best_block_height, &self.logger),
+								chan_entry
 							);
-						}
-						if let Some(splice_negotiated) = splice_negotiated {
-							self.pending_events.lock().unwrap().push_back((
-								events::Event::SplicePending {
-									channel_id: msg.channel_id,
-									counterparty_node_id: *counterparty_node_id,
-									user_channel_id: chan.context.get_user_id(),
-									new_funding_txo: splice_negotiated.funding_txo,
-									channel_type: splice_negotiated.channel_type,
-									new_funding_redeem_script: splice_negotiated
-										.funding_redeem_script,
-								},
-								None,
-							));
-						}
-					},
-					None => {
-						let msg = "Got an unexpected tx_signatures message";
-						let reason = ClosureReason::ProcessingError { err: msg.to_owned() };
-						let err = ChannelError::Close((msg.to_owned(), reason));
-						try_channel_entry!(self, peer_state, Err(err), chan_entry)
-					},
-				}
-				Ok(())
-			},
-			hash_map::Entry::Vacant(_) => Err(MsgHandleErrInternal::no_such_channel_for_peer(
-				counterparty_node_id,
-				msg.channel_id,
-			)),
-		}
+
+							// We should never be sending a `commitment_signed` in response to their
+							// `tx_signatures`.
+							debug_assert!(commitment_signed.is_none());
+							debug_assert!(counterparty_initial_commitment_signed_result.is_none());
+
+							if let Some(tx_signatures) = tx_signatures {
+								peer_state.pending_msg_events.push(
+									MessageSendEvent::SendTxSignatures {
+										node_id: *counterparty_node_id,
+										msg: tx_signatures,
+									},
+								);
+							}
+							if let Some(splice_locked) = splice_locked {
+								peer_state.pending_msg_events.push(
+									MessageSendEvent::SendSpliceLocked {
+										node_id: *counterparty_node_id,
+										msg: splice_locked,
+									},
+								);
+							}
+							if let Some((ref funding_tx, ref tx_type)) = funding_tx {
+								self.broadcast_interactive_funding(
+									chan,
+									funding_tx,
+									Some(tx_type.clone()),
+									&self.logger,
+								);
+							}
+							// We consider a splice negotiated when we exchange `tx_signatures`,
+							// which also terminates quiescence.
+							let exited_quiescence = splice_negotiated.is_some();
+							if let Some(splice_negotiated) = splice_negotiated {
+								self.pending_events.lock().unwrap().push_back((
+									events::Event::SplicePending {
+										channel_id: msg.channel_id,
+										counterparty_node_id: *counterparty_node_id,
+										user_channel_id: chan.context.get_user_id(),
+										new_funding_txo: splice_negotiated.funding_txo,
+										channel_type: splice_negotiated.channel_type,
+										new_funding_redeem_script: splice_negotiated
+											.funding_redeem_script,
+									},
+									None,
+								));
+							}
+							let holding_cell_res = if exited_quiescence {
+								self.check_free_peer_holding_cells(peer_state)
+							} else {
+								Vec::new()
+							};
+							(Ok(()), holding_cell_res)
+						},
+						None => {
+							let msg = "Got an unexpected tx_signatures message";
+							let reason = ClosureReason::ProcessingError { err: msg.to_owned() };
+							let err = ChannelError::Close((msg.to_owned(), reason));
+							try_channel_entry!(self, peer_state, Err(err), chan_entry)
+						},
+					}
+				},
+				hash_map::Entry::Vacant(_) => (
+					Err(MsgHandleErrInternal::no_such_channel_for_peer(
+						counterparty_node_id,
+						msg.channel_id,
+					)),
+					Vec::new(),
+				),
+			}
+		};
+
+		self.handle_holding_cell_free_result(holding_cell_res);
+		result
 	}
 
 	fn internal_tx_abort(

lightning/src/ln/splicing_tests.rs

@@ -2095,6 +2095,95 @@ fn fail_splice_on_tx_complete_error() {
 	do_commitment_signed_dance(initiator, acceptor, &update.commitment_signed, false, false);
 }
 
+#[test]
+fn free_holding_cell_on_tx_signatures_quiescence_exit() {
+	// Test that if there's an update in the holding cell while we're quiescent, that it gets freed
+	// upon exiting quiescence via the `tx_signatures` exchange.
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let config = test_default_channel_config();
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[Some(config.clone()), Some(config)]);
+	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	let initiator = &nodes[0];
+	let acceptor = &nodes[1];
+	let node_id_initiator = initiator.node.get_our_node_id();
+	let node_id_acceptor = acceptor.node.get_our_node_id();
+
+	let (_, _, channel_id, _) =
+		create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100_000, 0);
+
+	let contribution = SpliceContribution::splice_out(vec![TxOut {
+		value: Amount::from_sat(1_000),
+		script_pubkey: initiator.wallet_source.get_change_script().unwrap(),
+	}]);
+	negotiate_splice_tx(initiator, acceptor, channel_id, contribution);
+
+	// Queue an outgoing HTLC to the holding cell. It should be freed once we exit quiescence.
+	let (route, payment_hash, _payment_preimage, payment_secret) =
+		get_route_and_payment_hash!(initiator, acceptor, 1_000_000);
+	let onion = RecipientOnionFields::secret_only(payment_secret);
+	let payment_id = PaymentId(payment_hash.0);
+	initiator.node.send_payment_with_route(route, payment_hash, onion, payment_id).unwrap();
+	assert!(initiator.node.get_and_clear_pending_msg_events().is_empty());
+
+	let event = get_event!(initiator, Event::FundingTransactionReadyForSigning);
+	if let Event::FundingTransactionReadyForSigning {
+		channel_id,
+		counterparty_node_id,
+		unsigned_transaction,
+		..
+	} = event
+	{
+		let partially_signed_tx = initiator.wallet_source.sign_tx(unsigned_transaction).unwrap();
+		initiator
+			.node
+			.funding_transaction_signed(&channel_id, &counterparty_node_id, partially_signed_tx)
+			.unwrap();
+	} else {
+		unreachable!();
+	}
+
+	let update = get_htlc_update_msgs(initiator, &node_id_acceptor);
+	acceptor.node.handle_commitment_signed(node_id_initiator, &update.commitment_signed[0]);
+	check_added_monitors(&acceptor, 1);
+
+	let msg_events = acceptor.node.get_and_clear_pending_msg_events();
+	assert_eq!(msg_events.len(), 2, "{msg_events:?}");
+	if let MessageSendEvent::UpdateHTLCs { ref updates, .. } = &msg_events[0] {
+		let commitment_signed = &updates.commitment_signed[0];
+		initiator.node.handle_commitment_signed(node_id_acceptor, commitment_signed);
+		check_added_monitors(&initiator, 1);
+	} else {
+		panic!("Unexpected event {:?}", &msg_events[0]);
+	}
+	if let MessageSendEvent::SendTxSignatures { ref msg, .. } = &msg_events[1] {
+		initiator.node.handle_tx_signatures(node_id_acceptor, msg);
+	} else {
+		panic!("Unexpected event {:?}", &msg_events[1]);
+	}
+
+	// With `tx_signatures` exchanged, we've exited quiescence and should now see the outgoing HTLC
+	// update be sent.
+	let msg_events = initiator.node.get_and_clear_pending_msg_events();
+	assert_eq!(msg_events.len(), 2, "{msg_events:?}");
+	check_added_monitors(initiator, 1); // Outgoing HTLC monitor update
+	if let MessageSendEvent::SendTxSignatures { ref msg, .. } = &msg_events[0] {
+		acceptor.node.handle_tx_signatures(node_id_initiator, msg);
+	} else {
+		panic!("Unexpected event {:?}", &msg_events[0]);
+	}
+	if let MessageSendEvent::UpdateHTLCs { updates, .. } = &msg_events[1] {
+		acceptor.node.handle_update_add_htlc(node_id_initiator, &updates.update_add_htlcs[0]);
+		do_commitment_signed_dance(acceptor, initiator, &updates.commitment_signed, false, false);
+	} else {
+		panic!("Unexpected event {:?}", &msg_events[1]);
+	}
+
+	expect_splice_pending_event(initiator, &node_id_acceptor);
+	expect_splice_pending_event(acceptor, &node_id_initiator);
+}
+
 #[test]
 fn fail_splice_on_channel_close() {
 	let chanmon_cfgs = create_chanmon_cfgs(2);