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.

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index e533e532f7e6d526c97fd11b2257e70dafa59c07..c67b7f695a5f7c9fdffdb81db84a804390254174 100644 (file)
--- a/lightning/src/ln/channel.rs
+++ b/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(())

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 5cdc07ef0b1a4718bc577a7a7239c88cc723ee46..1a0c6f809a07cdaad4ce9bcb657501d9ba75c0a4 100644 (file)
--- a/lightning/src/ln/channelmanager.rs
+++ b/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;
                                                        },
                                                        _ => {},
                                                }

diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index 5d07edc2e5360e53a474b98559832bcb5a5ac2c6..c07775c8785bfd24adbd1e1639ec37649845b283 100644 (file)
--- a/lightning/src/ln/functional_tests.rs
+++ b/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();

diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs
index c7fae84af53875f3c526daa5ed6a157bbe7e8c9a..aa3b3b7e8f69dac9ae897152ece49ebf813bab50 100644 (file)
--- a/lightning/src/ln/onion_route_tests.rs
+++ b/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));