Only disable channels ~10 min after disconnect, rather than one

We correctly send out a gossip channel disable update after one
full time tick being down (1-2 minutes). This is pretty nice in
that it avoids nodes trying to route through our nodes too often
if they're down. Other nodes have a much longer time window,
causing them to have much less aggressive channel disables. Sadly,
at one minute it's not super uncommon for tor nodes to get disabled
(once a day or so on two nodes I looked at), and this causes the
lightning terminal scorer to consider the LDK node unstable (even
though it's the one doing the disabling - so is online). This
causes user frustration and makes LDK look bad (even though it's
probably failing fewer payments).

Given this, and future switches to block-based `channel_update`
timestamp fields, it makes sense to go ahead and switch to delaying
channel disable announcements for 10 minutes. This puts us more in
line with other implementations and reduces gossip spam, at the
cost of less reliable payments.

Fixes #2175, at least the currently visible parts.

Author: TheBlueMatt

lightning/src/ln/channel.rs

@@ -312,9 +312,9 @@ pub(super) enum ChannelUpdateStatus {
 	/// We've announced the channel as enabled and are connected to our peer.
 	Enabled,
 	/// Our channel is no longer live, but we haven't announced the channel as disabled yet.
-	DisabledStaged,
+	DisabledStaged(u8),
 	/// Our channel is live again, but we haven't announced the channel as enabled yet.
-	EnabledStaged,
+	EnabledStaged(u8),
 	/// We've announced the channel as disabled.
 	Disabled,
 }
@@ -6193,8 +6193,8 @@ impl Writeable for ChannelUpdateStatus {
 		// channel as enabled, so we write 0. For EnabledStaged, we similarly write a 1.
 		match self {
 			ChannelUpdateStatus::Enabled => 0u8.write(writer)?,
-			ChannelUpdateStatus::DisabledStaged => 0u8.write(writer)?,
-			ChannelUpdateStatus::EnabledStaged => 1u8.write(writer)?,
+			ChannelUpdateStatus::DisabledStaged(_) => 0u8.write(writer)?,
+			ChannelUpdateStatus::EnabledStaged(_) => 1u8.write(writer)?,
 			ChannelUpdateStatus::Disabled => 1u8.write(writer)?,
 		}
 		Ok(())

lightning/src/ln/channelmanager.rs

@@ -1070,6 +1070,14 @@ pub(crate) const MPP_TIMEOUT_TICKS: u8 = 3;
 /// [`OutboundPayments::remove_stale_resolved_payments`].
 pub(crate) const IDEMPOTENCY_TIMEOUT_TICKS: u8 = 7;
 
+/// The number of ticks of [`ChannelManager::timer_tick_occurred`] where a peer is disconnected
+/// until we mark the channel disabled and gossip the update.
+pub(crate) const DISABLE_GOSSIP_TICKS: u8 = 10;
+
+/// The number of ticks of [`ChannelManager::timer_tick_occurred`] where a peer is connected until
+/// we mark the channel enabled and gossip the update.
+pub(crate) const ENABLE_GOSSIP_TICKS: u8 = 5;
+
 /// The maximum number of unfunded channels we can have per-peer before we start rejecting new
 /// (inbound) ones. The number of peers with unfunded channels is limited separately in
 /// [`MAX_UNFUNDED_CHANNEL_PEERS`].
@@ -2591,9 +2599,9 @@ where
 
 		let enabled = chan.is_usable() && match chan.channel_update_status() {
 			ChannelUpdateStatus::Enabled => true,
-			ChannelUpdateStatus::DisabledStaged => true,
+			ChannelUpdateStatus::DisabledStaged(_) => true,
 			ChannelUpdateStatus::Disabled => false,
-			ChannelUpdateStatus::EnabledStaged => false,
+			ChannelUpdateStatus::EnabledStaged(_) => false,
 		};
 
 		let unsigned = msgs::UnsignedChannelUpdate {
@@ -3750,27 +3758,39 @@ where
 						}
 
 						match chan.channel_update_status() {
-							ChannelUpdateStatus::Enabled if !chan.is_live() => chan.set_channel_update_status(ChannelUpdateStatus::DisabledStaged),
-							ChannelUpdateStatus::Disabled if chan.is_live() => chan.set_channel_update_status(ChannelUpdateStatus::EnabledStaged),
-							ChannelUpdateStatus::DisabledStaged if chan.is_live() => chan.set_channel_update_status(ChannelUpdateStatus::Enabled),
-							ChannelUpdateStatus::EnabledStaged if !chan.is_live() => chan.set_channel_update_status(ChannelUpdateStatus::Disabled),
-							ChannelUpdateStatus::DisabledStaged if !chan.is_live() => {
-								chan.set_channel_update_status(ChannelUpdateStatus::Disabled);
-								if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
-									pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
-										msg: update
-									});
+							ChannelUpdateStatus::Enabled if !chan.is_live() => chan.set_channel_update_status(ChannelUpdateStatus::DisabledStaged(0)),
+							ChannelUpdateStatus::Disabled if chan.is_live() => chan.set_channel_update_status(ChannelUpdateStatus::EnabledStaged(0)),
+							ChannelUpdateStatus::DisabledStaged(_) if chan.is_live()
+								=> chan.set_channel_update_status(ChannelUpdateStatus::Enabled),
+							ChannelUpdateStatus::EnabledStaged(_) if !chan.is_live()
+								=> chan.set_channel_update_status(ChannelUpdateStatus::Disabled),
+							ChannelUpdateStatus::DisabledStaged(mut n) if !chan.is_live() => {
+								n += 1;
+								if n >= DISABLE_GOSSIP_TICKS {
+									chan.set_channel_update_status(ChannelUpdateStatus::Disabled);
+									if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
+										pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
+											msg: update
+										});
+									}
+									should_persist = NotifyOption::DoPersist;
+								} else {
+									chan.set_channel_update_status(ChannelUpdateStatus::DisabledStaged(n));
 								}
-								should_persist = NotifyOption::DoPersist;
 							},
-							ChannelUpdateStatus::EnabledStaged if chan.is_live() => {
-								chan.set_channel_update_status(ChannelUpdateStatus::Enabled);
-								if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
-									pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
-										msg: update
-									});
+							ChannelUpdateStatus::EnabledStaged(mut n) if chan.is_live() => {
+								n += 1;
+								if n >= ENABLE_GOSSIP_TICKS {
+									chan.set_channel_update_status(ChannelUpdateStatus::Enabled);
+									if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
+										pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
+											msg: update
+										});
+									}
+									should_persist = NotifyOption::DoPersist;
+								} else {
+									chan.set_channel_update_status(ChannelUpdateStatus::EnabledStaged(n));
 								}
-								should_persist = NotifyOption::DoPersist;
 							},
 							_ => {},
 						}

lightning/src/ln/functional_tests.rs

@@ -21,7 +21,7 @@ use crate::chain::keysinterface::{ChannelSigner, EcdsaChannelSigner, EntropySour
 use crate::events::{Event, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentPurpose, ClosureReason, HTLCDestination, PaymentFailureReason};
 use crate::ln::{PaymentPreimage, PaymentSecret, PaymentHash};
 use crate::ln::channel::{commitment_tx_base_weight, COMMITMENT_TX_WEIGHT_PER_HTLC, CONCURRENT_INBOUND_HTLC_FEE_BUFFER, FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE, MIN_AFFORDABLE_HTLC_COUNT};
-use crate::ln::channelmanager::{self, PaymentId, RAACommitmentOrder, PaymentSendFailure, RecipientOnionFields, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA};
+use crate::ln::channelmanager::{self, PaymentId, RAACommitmentOrder, PaymentSendFailure, RecipientOnionFields, BREAKDOWN_TIMEOUT, ENABLE_GOSSIP_TICKS, DISABLE_GOSSIP_TICKS, MIN_CLTV_EXPIRY_DELTA};
 use crate::ln::channel::{Channel, ChannelError};
 use crate::ln::{chan_utils, onion_utils};
 use crate::ln::chan_utils::{OFFERED_HTLC_SCRIPT_WEIGHT, htlc_success_tx_weight, htlc_timeout_tx_weight, HTLCOutputInCommitment};
@@ -7100,8 +7100,9 @@ fn test_announce_disable_channels() {
 	nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id());
 	nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id());
 
-	nodes[0].node.timer_tick_occurred(); // Enabled -> DisabledStaged
-	nodes[0].node.timer_tick_occurred(); // DisabledStaged -> Disabled
+	for _ in 0..DISABLE_GOSSIP_TICKS + 1 {
+		nodes[0].node.timer_tick_occurred();
+	}
 	let msg_events = nodes[0].node.get_and_clear_pending_msg_events();
 	assert_eq!(msg_events.len(), 3);
 	let mut chans_disabled = HashMap::new();
@@ -7141,7 +7142,9 @@ fn test_announce_disable_channels() {
 	nodes[1].node.handle_channel_reestablish(&nodes[0].node.get_our_node_id(), &reestablish_1[2]);
 	handle_chan_reestablish_msgs!(nodes[1], nodes[0]);
 
-	nodes[0].node.timer_tick_occurred();
+	for _ in 0..ENABLE_GOSSIP_TICKS {
+		nodes[0].node.timer_tick_occurred();
+	}
 	assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
 	nodes[0].node.timer_tick_occurred();
 	let msg_events = nodes[0].node.get_and_clear_pending_msg_events();

lightning/src/ln/onion_route_tests.rs

@@ -16,7 +16,7 @@ use crate::chain::keysinterface::{EntropySource, NodeSigner, Recipient};
 use crate::events::{Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentFailureReason};
 use crate::ln::{PaymentHash, PaymentSecret};
 use crate::ln::channel::EXPIRE_PREV_CONFIG_TICKS;
-use crate::ln::channelmanager::{HTLCForwardInfo, FailureCode, CLTV_FAR_FAR_AWAY, MIN_CLTV_EXPIRY_DELTA, PendingAddHTLCInfo, PendingHTLCInfo, PendingHTLCRouting, PaymentId, RecipientOnionFields};
+use crate::ln::channelmanager::{HTLCForwardInfo, FailureCode, CLTV_FAR_FAR_AWAY, DISABLE_GOSSIP_TICKS, MIN_CLTV_EXPIRY_DELTA, PendingAddHTLCInfo, PendingHTLCInfo, PendingHTLCRouting, PaymentId, RecipientOnionFields};
 use crate::ln::onion_utils;
 use crate::routing::gossip::{NetworkUpdate, RoutingFees};
 use crate::routing::router::{get_route, PaymentParameters, Route, RouteHint, RouteHintHop};
@@ -589,12 +589,12 @@ fn test_onion_failure() {
 		nodes[2].node.peer_disconnected(&nodes[1].node.get_our_node_id());
 	}, true, Some(UPDATE|7), Some(NetworkUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy(short_channel_id)}), Some(short_channel_id));
 	run_onion_failure_test("channel_disabled", 0, &nodes, &route, &payment_hash, &payment_secret, |_| {}, || {
-		// Tick the timer twice on each node to mark the channel as disabled.
-		nodes[1].node.timer_tick_occurred();
-		nodes[1].node.timer_tick_occurred();
+		// disconnect event to the channel between nodes[1] ~ nodes[2]
+		for _ in 0..DISABLE_GOSSIP_TICKS + 1 {
+			nodes[1].node.timer_tick_occurred();
+			nodes[2].node.timer_tick_occurred();
+		}
 		nodes[1].node.get_and_clear_pending_msg_events();
-		nodes[2].node.timer_tick_occurred();
-		nodes[2].node.timer_tick_occurred();
 		nodes[2].node.get_and_clear_pending_msg_events();
 	}, true, Some(UPDATE|20), Some(NetworkUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy(short_channel_id)}), Some(short_channel_id));
 	reconnect_nodes(&nodes[1], &nodes[2], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));