Merge pull request #2198 from TheBlueMatt/2023-04-fewer-disables
authorMatt Corallo <649246+TheBlueMatt@users.noreply.github.com>
Tue, 18 Apr 2023 15:56:58 +0000 (15:56 +0000)
committerGitHub <noreply@github.com>
Tue, 18 Apr 2023 15:56:58 +0000 (15:56 +0000)
Only disable channels ~10 min after disconnect, rather than one

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

index e533e532f7e6d526c97fd11b2257e70dafa59c07..c67b7f695a5f7c9fdffdb81db84a804390254174 100644 (file)
@@ -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(())
index 9f403f85b2f0aec6b841961fa47fd7e6bce424e7..1a0c6f809a07cdaad4ce9bcb657501d9ba75c0a4 100644 (file)
@@ -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`].
@@ -2457,7 +2465,14 @@ where
                                                // hopefully an attacker trying to path-trace payments cannot make this occur
                                                // on a small/per-node/per-channel scale.
                                                if !chan.is_live() { // channel_disabled
-                                                       break Some(("Forwarding channel is not in a ready state.", 0x1000 | 20, chan_update_opt));
+                                                       // If the channel_update we're going to return is disabled (i.e. the
+                                                       // peer has been disabled for some time), return `channel_disabled`,
+                                                       // otherwise return `temporary_channel_failure`.
+                                                       if chan_update_opt.as_ref().map(|u| u.contents.flags & 2 == 2).unwrap_or(false) {
+                                                               break Some(("Forwarding channel has been disconnected for some time.", 0x1000 | 20, chan_update_opt));
+                                                       } else {
+                                                               break Some(("Forwarding channel is not in a ready state.", 0x1000 | 7, chan_update_opt));
+                                                       }
                                                }
                                                if *outgoing_amt_msat < chan.get_counterparty_htlc_minimum_msat() { // amount_below_minimum
                                                        break Some(("HTLC amount was below the htlc_minimum_msat", 0x1000 | 11, chan_update_opt));
@@ -2582,11 +2597,18 @@ where
                log_trace!(self.logger, "Generating channel update for channel {}", log_bytes!(chan.channel_id()));
                let were_node_one = self.our_network_pubkey.serialize()[..] < chan.get_counterparty_node_id().serialize()[..];
 
+               let enabled = chan.is_usable() && match chan.channel_update_status() {
+                       ChannelUpdateStatus::Enabled => true,
+                       ChannelUpdateStatus::DisabledStaged(_) => true,
+                       ChannelUpdateStatus::Disabled => false,
+                       ChannelUpdateStatus::EnabledStaged(_) => false,
+               };
+
                let unsigned = msgs::UnsignedChannelUpdate {
                        chain_hash: self.genesis_hash,
                        short_channel_id,
                        timestamp: chan.get_update_time_counter(),
-                       flags: (!were_node_one) as u8 | ((!chan.is_live() as u8) << 1),
+                       flags: (!were_node_one) as u8 | ((!enabled as u8) << 1),
                        cltv_expiry_delta: chan.get_cltv_expiry_delta(),
                        htlc_minimum_msat: chan.get_counterparty_htlc_minimum_msat(),
                        htlc_maximum_msat: chan.get_announced_htlc_max_msat(),
@@ -3736,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() => {
-                                                               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;
-                                                               chan.set_channel_update_status(ChannelUpdateStatus::Disabled);
                                                        },
-                                                       ChannelUpdateStatus::EnabledStaged if chan.is_live() => {
-                                                               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;
-                                                               chan.set_channel_update_status(ChannelUpdateStatus::Enabled);
                                                        },
                                                        _ => {},
                                                }
index f8ccce05847a7d426a5b05118c25670febb4d6b0..e818bd95502ba9f9869c44e7e479204a23c2a918 100644 (file)
@@ -2724,10 +2724,9 @@ macro_rules! handle_chan_reestablish_msgs {
                        }
 
                        let mut had_channel_update = false; // ChannelUpdate may be now or later, but not both
-                       if let Some(&MessageSendEvent::SendChannelUpdate { ref node_id, ref msg }) = msg_events.get(idx) {
+                       if let Some(&MessageSendEvent::SendChannelUpdate { ref node_id, .. }) = msg_events.get(idx) {
                                assert_eq!(*node_id, $dst_node.node.get_our_node_id());
                                idx += 1;
-                               assert_eq!(msg.contents.flags & 2, 0); // "disabled" flag must not be set as we just reconnected.
                                had_channel_update = true;
                        }
 
@@ -2771,10 +2770,9 @@ macro_rules! handle_chan_reestablish_msgs {
                                }
                        }
 
-                       if let Some(&MessageSendEvent::SendChannelUpdate { ref node_id, ref msg }) = msg_events.get(idx) {
+                       if let Some(&MessageSendEvent::SendChannelUpdate { ref node_id, .. }) = msg_events.get(idx) {
                                assert_eq!(*node_id, $dst_node.node.get_our_node_id());
                                idx += 1;
-                               assert_eq!(msg.contents.flags & 2, 0); // "disabled" flag must not be set as we just reconnected.
                                assert!(!had_channel_update);
                        }
 
index 5d07edc2e5360e53a474b98559832bcb5a5ac2c6..c07775c8785bfd24adbd1e1639ec37649845b283 100644 (file)
@@ -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();
index 743d41eea1ba731ca96ac0114a443660fe1f1738..aa3b3b7e8f69dac9ae897152ece49ebf813bab50 100644 (file)
@@ -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};
@@ -587,6 +587,15 @@ fn test_onion_failure() {
                // disconnect event to the channel between nodes[1] ~ nodes[2]
                nodes[1].node.peer_disconnected(&nodes[2].node.get_our_node_id());
                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, |_| {}, || {
+               // 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.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));