Close and remove pending inbound/outbound channels that are older than an hour

We introduce a `PendingChannelContext` which contains a counter for the
current age of a pending channel in timer ticks. This age is incremented
for every `ChannelManager::timer_tick_ocurred` and the pending channel
is removed if it exceeds `ENDING_CHANNEL_AGE_LIMIT_TICKS`.

The value will not be persisted as pending channels themselves are not
persisted.

Author: dunxen

lightning/src/ln/channel.rs

@@ -590,6 +590,11 @@ pub(crate) const EXPIRE_PREV_CONFIG_TICKS: usize = 5;
 /// See [`ChannelContext::sent_message_awaiting_response`] for more information.
 pub(crate) const DISCONNECT_PEER_AWAITING_RESPONSE_TICKS: usize = 2;
 
+/// The number of ticks that may elapse while we're waiting for a pending outbound/inbound channel
+/// to be promoted to a [`Channel`] since the pending channel was created. A pending channel
+/// exceeding this age limit will be force closed and purged from memory.
+pub(crate) const  PENDING_CHANNEL_AGE_LIMIT_TICKS: usize = 60;
+
 struct PendingChannelMonitorUpdate {
 	update: ChannelMonitorUpdate,
 }
@@ -598,6 +603,28 @@ impl_writeable_tlv_based!(PendingChannelMonitorUpdate, {
 	(0, update, required),
 });
 
+/// Contains all state common to pending (unfunded) inbound/outbound channels.
+pub(super) struct PendingChannelContext {
+	/// A counter tracking how many ticks have elapsed since this pending channel was
+	/// created. If this pending channel reaches peer has yet to respond after reaching
+	/// `PENDING_CHANNEL_AGE_LIMIT_TICKS`, it will be force-closed and purged from memory.
+	///
+	/// This is so that we don't keep channels around that haven't progressed to a funded state
+	/// in a timely manner.
+	pending_channel_age_ticks: usize,
+}
+
+impl PendingChannelContext {
+	/// Determines whether we should force-close and purge this pending channel from memory due to it
+	/// having reached the pending channel age limit.
+	///
+	/// This should be called on every [`super::channelmanager::ChannelManager::timer_tick_occurred`].
+	pub fn should_expire_pending_channel(&mut self) -> bool {
+		self.pending_channel_age_ticks += 1;
+		self.pending_channel_age_ticks >= PENDING_CHANNEL_AGE_LIMIT_TICKS
+	}
+}
+
 /// Contains everything about the channel including state, and various flags.
 pub(super) struct ChannelContext<Signer: ChannelSigner> {
 	config: LegacyChannelConfig,
@@ -5462,6 +5489,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
 /// A not-yet-funded outbound (from holder) channel using V1 channel establishment.
 pub(super) struct OutboundV1Channel<Signer: ChannelSigner> {
 	pub context: ChannelContext<Signer>,
+	pub pending_context: PendingChannelContext,
 }
 
 impl<Signer: WriteableEcdsaChannelSigner> OutboundV1Channel<Signer> {
@@ -5658,7 +5686,8 @@ impl<Signer: WriteableEcdsaChannelSigner> OutboundV1Channel<Signer> {
 				channel_keys_id,
 
 				blocked_monitor_updates: Vec::new(),
-			}
+			},
+			pending_context: PendingChannelContext { pending_channel_age_ticks: 0 }
 		})
 	}
 
@@ -5957,6 +5986,7 @@ impl<Signer: WriteableEcdsaChannelSigner> OutboundV1Channel<Signer> {
 /// A not-yet-funded inbound (from counterparty) channel using V1 channel establishment.
 pub(super) struct InboundV1Channel<Signer: ChannelSigner> {
 	pub context: ChannelContext<Signer>,
+	pub pending_context: PendingChannelContext,
 }
 
 impl<Signer: WriteableEcdsaChannelSigner> InboundV1Channel<Signer> {
@@ -6284,7 +6314,8 @@ impl<Signer: WriteableEcdsaChannelSigner> InboundV1Channel<Signer> {
 				channel_keys_id,
 
 				blocked_monitor_updates: Vec::new(),
-			}
+			},
+			pending_context: PendingChannelContext { pending_channel_age_ticks: 0 }
 		};
 
 		Ok(chan)

lightning/src/ln/channelmanager.rs

@@ -4326,6 +4326,7 @@ where
 	///  * Expiring a channel's previous [`ChannelConfig`] if necessary to only allow forwarding HTLCs
 	///    with the current [`ChannelConfig`].
 	///  * Removing peers which have disconnected but and no longer have any channels.
+	///  * Force-closing and removing channels which have not completed establishment in a timely manner.
 	///
 	/// Note that this may cause reentrancy through [`chain::Watch::update_channel`] calls or feerate
 	/// estimate fetches.
@@ -4414,6 +4415,28 @@ where
 
 						true
 					});
+
+					let force_close_expired_pending_channel = |chan_id: &[u8; 32], chan_context: &mut ChannelContext<<SP::Target as SignerProvider>::Signer>| {
+						log_error!(self.logger, "Force-closing pending outbound channel {} for not establishing in a timely manner", log_bytes!(&chan_id[..]));
+						self.issue_channel_close_events(&chan_context, ClosureReason::HolderForceClosed);
+						self.finish_force_close_channel(chan_context.force_shutdown(false));
+						false
+					};
+					peer_state.outbound_v1_channel_by_id.retain(|chan_id, chan| {
+						if chan.pending_context.should_expire_pending_channel() {
+							force_close_expired_pending_channel(chan_id, &mut chan.context)
+						} else {
+							true
+						}
+					});
+					peer_state.inbound_v1_channel_by_id.retain(|chan_id, chan| {
+						if chan.pending_context.should_expire_pending_channel() {
+							force_close_expired_pending_channel(chan_id, &mut chan.context)
+						} else {
+							true
+						}
+					});
+
 					if peer_state.ok_to_remove(true) {
 						pending_peers_awaiting_removal.push(counterparty_node_id);
 					}

lightning/src/ln/functional_tests.rs

@@ -61,6 +61,8 @@ use crate::sync::{Arc, Mutex};
 use crate::ln::functional_test_utils::*;
 use crate::ln::chan_utils::CommitmentTransaction;
 
+use super::channel::PENDING_CHANNEL_AGE_LIMIT_TICKS;
+
 #[test]
 fn test_insane_channel_opens() {
 	// Stand up a network of 2 nodes
@@ -10017,3 +10019,89 @@ fn test_disconnects_peer_awaiting_response_ticks() {
 		}
 	}
 }
+
+#[test]
+fn test_remove_expired_outbound_pending_channels() {
+	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 temp_channel_id = nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100_000, 0, 42, None).unwrap();
+	let open_channel_message = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
+	nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), &open_channel_message);
+	let accept_channel_message = get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id());
+	nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), &accept_channel_message);
+
+	let events = nodes[0].node.get_and_clear_pending_events();
+	assert_eq!(events.len(), 1);
+	match events[0] {
+		Event::FundingGenerationReady { .. } => (),
+		_ => panic!("Unexpected event"),
+	};
+
+	// Asserts the outbound channel has been removed from a nodes[0]'s peer state map.
+	let check_outbound_channel_existence = |should_exist: bool| {
+		let per_peer_state = nodes[0].node.per_peer_state.read().unwrap();
+		let chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap();
+		assert_eq!(chan_lock.outbound_v1_channel_by_id.contains_key(&temp_channel_id), should_exist);
+	};
+
+	// Channel should exist without any timer ticks.
+	check_outbound_channel_existence(true);
+
+	// Channel should exist with 1 timer tick less than required.
+	for _ in 0..PENDING_CHANNEL_AGE_LIMIT_TICKS - 1 {
+		nodes[0].node.timer_tick_occurred();
+		check_outbound_channel_existence(true)
+	}
+
+	// Remove channel after reaching the required ticks.
+	nodes[0].node.timer_tick_occurred();
+	check_outbound_channel_existence(false);
+
+	check_closed_event!(nodes[0], 1, ClosureReason::HolderForceClosed);
+}
+
+#[test]
+fn test_remove_expired_inbound_pending_channels() {
+	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 temp_channel_id = nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100_000, 0, 42, None).unwrap();
+	let open_channel_message = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
+	nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), &open_channel_message);
+	let accept_channel_message = get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id());
+	nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), &accept_channel_message);
+
+	let events = nodes[0].node.get_and_clear_pending_events();
+	assert_eq!(events.len(), 1);
+	match events[0] {
+		Event::FundingGenerationReady { .. } => (),
+		_ => panic!("Unexpected event"),
+	};
+
+	// Asserts the inbound channel has been removed from a nodes[1]'s peer state map.
+	let check_inbound_channel_existence = |should_exist: bool| {
+		let per_peer_state = nodes[1].node.per_peer_state.read().unwrap();
+		let chan_lock = per_peer_state.get(&nodes[0].node.get_our_node_id()).unwrap().lock().unwrap();
+		assert_eq!(chan_lock.inbound_v1_channel_by_id.contains_key(&temp_channel_id), should_exist);
+	};
+
+	// Channel should exist without any timer ticks.
+	check_inbound_channel_existence(true);
+
+	// Channel should exist with 1 timer tick less than required.
+	for _ in 0..PENDING_CHANNEL_AGE_LIMIT_TICKS - 1 {
+		nodes[1].node.timer_tick_occurred();
+		check_inbound_channel_existence(true)
+	}
+
+	// Remove channel after reaching the required ticks.
+	nodes[1].node.timer_tick_occurred();
+	check_inbound_channel_existence(false);
+
+	check_closed_event!(nodes[1], 1, ClosureReason::HolderForceClosed);
+}