Close and remove unfunded inbound/outbound channels that are older than an hour
authorDuncan Dean <git@dunxen.dev>
Fri, 14 Jul 2023 14:59:29 +0000 (16:59 +0200)
committerDuncan Dean <git@dunxen.dev>
Wed, 19 Jul 2023 17:12:10 +0000 (19:12 +0200)
We introduce a `UnfundedChannelContext` which contains a counter for the
current age of an unfunded channel in timer ticks. This age is incremented
for every `ChannelManager::timer_tick_ocurred` and the unfunded channel
is removed if it exceeds `UNFUNDED_CHANNEL_AGE_LIMIT_TICKS`.

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

lightning/src/ln/channel.rs
lightning/src/ln/channelmanager.rs
lightning/src/ln/functional_tests.rs

index 31aecf68eb56344d9f17ba356ab9b8f76550c827..e74f659fbd09ba04a6fc537fd9525b3c8f56a7c6 100644 (file)
@@ -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 an unfunded outbound/inbound channel
+/// to be promoted to a [`Channel`] since the unfunded channel was created. An unfunded channel
+/// exceeding this age limit will be force-closed and purged from memory.
+pub(crate) const UNFUNDED_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 unfunded inbound/outbound channels.
+pub(super) struct UnfundedChannelContext {
+       /// A counter tracking how many ticks have elapsed since this unfunded channel was
+       /// created. If this unfunded channel reaches peer has yet to respond after reaching
+       /// `UNFUNDED_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.
+       unfunded_channel_age_ticks: usize,
+}
+
+impl UnfundedChannelContext {
+       /// Determines whether we should force-close and purge this unfunded channel from memory due to it
+       /// having reached the unfunded channel age limit.
+       ///
+       /// This should be called on every [`super::channelmanager::ChannelManager::timer_tick_occurred`].
+       pub fn should_expire_unfunded_channel(&mut self) -> bool {
+               self.unfunded_channel_age_ticks += 1;
+               self.unfunded_channel_age_ticks >= UNFUNDED_CHANNEL_AGE_LIMIT_TICKS
+       }
+}
+
 /// Contains everything about the channel including state, and various flags.
 pub(super) struct ChannelContext<Signer: ChannelSigner> {
        config: LegacyChannelConfig,
@@ -5477,6 +5504,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 unfunded_context: UnfundedChannelContext,
 }
 
 impl<Signer: WriteableEcdsaChannelSigner> OutboundV1Channel<Signer> {
@@ -5678,7 +5706,8 @@ impl<Signer: WriteableEcdsaChannelSigner> OutboundV1Channel<Signer> {
                                channel_keys_id,
 
                                blocked_monitor_updates: Vec::new(),
-                       }
+                       },
+                       unfunded_context: UnfundedChannelContext { unfunded_channel_age_ticks: 0 }
                })
        }
 
@@ -5983,6 +6012,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 unfunded_context: UnfundedChannelContext,
 }
 
 impl<Signer: WriteableEcdsaChannelSigner> InboundV1Channel<Signer> {
@@ -6310,7 +6340,8 @@ impl<Signer: WriteableEcdsaChannelSigner> InboundV1Channel<Signer> {
                                channel_keys_id,
 
                                blocked_monitor_updates: Vec::new(),
-                       }
+                       },
+                       unfunded_context: UnfundedChannelContext { unfunded_channel_age_ticks: 0 }
                };
 
                Ok(chan)
index 31e719cd4f48f6f063959b854b72fefba4fe9b2b..b22c3716ccbbdb795981d94e270424e089e25392 100644 (file)
@@ -40,7 +40,7 @@ use crate::events::{Event, EventHandler, EventsProvider, MessageSendEvent, Messa
 // Since this struct is returned in `list_channels` methods, expose it here in case users want to
 // construct one themselves.
 use crate::ln::{inbound_payment, PaymentHash, PaymentPreimage, PaymentSecret};
-use crate::ln::channel::{Channel, ChannelContext, ChannelError, ChannelUpdateStatus, ShutdownResult, UpdateFulfillCommitFetch, OutboundV1Channel, InboundV1Channel};
+use crate::ln::channel::{Channel, ChannelContext, ChannelError, ChannelUpdateStatus, ShutdownResult, UnfundedChannelContext, UpdateFulfillCommitFetch, OutboundV1Channel, InboundV1Channel};
 use crate::ln::features::{ChannelFeatures, ChannelTypeFeatures, InitFeatures, NodeFeatures};
 #[cfg(any(feature = "_test_utils", test))]
 use crate::ln::features::Bolt11InvoiceFeatures;
@@ -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.
@@ -4420,6 +4421,26 @@ where
 
                                                true
                                        });
+
+                                       let process_unfunded_channel_tick = |
+                                               chan_id: &[u8; 32],
+                                               chan_context: &mut ChannelContext<<SP::Target as SignerProvider>::Signer>,
+                                               unfunded_chan_context: &mut UnfundedChannelContext,
+                                       | {
+                                               chan_context.maybe_expire_prev_config();
+                                               if unfunded_chan_context.should_expire_unfunded_channel() {
+                                                       log_error!(self.logger, "Force-closing pending outbound channel {} for not establishing in a timely manner", log_bytes!(&chan_id[..]));
+                                                       update_maps_on_chan_removal!(self, &chan_context);
+                                                       self.issue_channel_close_events(&chan_context, ClosureReason::HolderForceClosed);
+                                                       self.finish_force_close_channel(chan_context.force_shutdown(false));
+                                                       false
+                                               } else {
+                                                       true
+                                               }
+                                       };
+                                       peer_state.outbound_v1_channel_by_id.retain(|chan_id, chan| process_unfunded_channel_tick(chan_id, &mut chan.context, &mut chan.unfunded_context));
+                                       peer_state.inbound_v1_channel_by_id.retain(|chan_id, chan| process_unfunded_channel_tick(chan_id, &mut chan.context, &mut chan.unfunded_context));
+
                                        if peer_state.ok_to_remove(true) {
                                                pending_peers_awaiting_removal.push(counterparty_node_id);
                                        }
index 0e35f35f0c80377855a8300d20fad01fb9ccf498..8de2e9f631a53325ba5b6ba6b384392762017967 100644 (file)
@@ -61,6 +61,8 @@ use crate::sync::{Arc, Mutex};
 use crate::ln::functional_test_utils::*;
 use crate::ln::chan_utils::CommitmentTransaction;
 
+use super::channel::UNFUNDED_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_unfunded_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..UNFUNDED_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_unfunded_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..UNFUNDED_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);
+}