From: Matt Corallo Date: Mon, 17 Apr 2023 23:09:11 +0000 (+0000) Subject: Only disable channels ~10 min after disconnect, rather than one X-Git-Tag: v0.0.115~15^2 X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=refs%2Fheads%2F2023-04-fewer-disables;p=rust-lightning 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 e533e532f..c67b7f695 100644 --- 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 5cdc07ef0..1a0c6f809 100644 --- 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 5d07edc2e..c07775c87 100644 --- 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 c7fae84af..aa3b3b7e8 100644 --- 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));