Skip to content

Commit a332bfc

Browse files
TheBlueMattTheBlueMatt
authored
Merge pull request #2198 from TheBlueMatt/2023-04-fewer-disables
Only disable channels ~10 min after disconnect, rather than one
2 parents 2ebbe6f + 89e063b commit a332bfc

File tree

5 files changed

+77
-33
lines changed

5 files changed

+77
-33
lines changed

‎lightning/src/ln/channel.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -312,9 +312,9 @@ pub(super) enum ChannelUpdateStatus {
312312
/// We've announced the channel as enabled and are connected to our peer.
313313
Enabled,
314314
/// Our channel is no longer live, but we haven't announced the channel as disabled yet.
315-
DisabledStaged,
315+
DisabledStaged(u8),
316316
/// Our channel is live again, but we haven't announced the channel as enabled yet.
317-
EnabledStaged,
317+
EnabledStaged(u8),
318318
/// We've announced the channel as disabled.
319319
Disabled,
320320
}
@@ -6193,8 +6193,8 @@ impl Writeable for ChannelUpdateStatus {
61936193
// channel as enabled, so we write 0. For EnabledStaged, we similarly write a 1.
61946194
match self {
61956195
ChannelUpdateStatus::Enabled => 0u8.write(writer)?,
6196-
ChannelUpdateStatus::DisabledStaged => 0u8.write(writer)?,
6197-
ChannelUpdateStatus::EnabledStaged => 1u8.write(writer)?,
6196+
ChannelUpdateStatus::DisabledStaged(_) => 0u8.write(writer)?,
6197+
ChannelUpdateStatus::EnabledStaged(_) => 1u8.write(writer)?,
61986198
ChannelUpdateStatus::Disabled => 1u8.write(writer)?,
61996199
}
62006200
Ok(())

‎lightning/src/ln/channelmanager.rs

Lines changed: 54 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1070,6 +1070,14 @@ pub(crate) const MPP_TIMEOUT_TICKS: u8 = 3;
10701070
/// [`OutboundPayments::remove_stale_resolved_payments`].
10711071
pub(crate) const IDEMPOTENCY_TIMEOUT_TICKS: u8 = 7;
10721072

1073+
/// The number of ticks of [`ChannelManager::timer_tick_occurred`] where a peer is disconnected
1074+
/// until we mark the channel disabled and gossip the update.
1075+
pub(crate) const DISABLE_GOSSIP_TICKS: u8 = 10;
1076+
1077+
/// The number of ticks of [`ChannelManager::timer_tick_occurred`] where a peer is connected until
1078+
/// we mark the channel enabled and gossip the update.
1079+
pub(crate) const ENABLE_GOSSIP_TICKS: u8 = 5;
1080+
10731081
/// The maximum number of unfunded channels we can have per-peer before we start rejecting new
10741082
/// (inbound) ones. The number of peers with unfunded channels is limited separately in
10751083
/// [`MAX_UNFUNDED_CHANNEL_PEERS`].
@@ -2457,7 +2465,14 @@ where
24572465
// hopefully an attacker trying to path-trace payments cannot make this occur
24582466
// on a small/per-node/per-channel scale.
24592467
if !chan.is_live() { // channel_disabled
2460-
break Some(("Forwarding channel is not in a ready state.", 0x1000 | 20, chan_update_opt));
2468+
// If the channel_update we're going to return is disabled (i.e. the
2469+
// peer has been disabled for some time), return `channel_disabled`,
2470+
// otherwise return `temporary_channel_failure`.
2471+
if chan_update_opt.as_ref().map(|u| u.contents.flags & 2 == 2).unwrap_or(false) {
2472+
break Some(("Forwarding channel has been disconnected for some time.", 0x1000 | 20, chan_update_opt));
2473+
} else {
2474+
break Some(("Forwarding channel is not in a ready state.", 0x1000 | 7, chan_update_opt));
2475+
}
24612476
}
24622477
if *outgoing_amt_msat < chan.get_counterparty_htlc_minimum_msat() { // amount_below_minimum
24632478
break Some(("HTLC amount was below the htlc_minimum_msat", 0x1000 | 11, chan_update_opt));
@@ -2582,11 +2597,18 @@ where
25822597
log_trace!(self.logger, "Generating channel update for channel {}", log_bytes!(chan.channel_id()));
25832598
let were_node_one = self.our_network_pubkey.serialize()[..] < chan.get_counterparty_node_id().serialize()[..];
25842599

2600+
let enabled = chan.is_usable() && match chan.channel_update_status() {
2601+
ChannelUpdateStatus::Enabled => true,
2602+
ChannelUpdateStatus::DisabledStaged(_) => true,
2603+
ChannelUpdateStatus::Disabled => false,
2604+
ChannelUpdateStatus::EnabledStaged(_) => false,
2605+
};
2606+
25852607
let unsigned = msgs::UnsignedChannelUpdate {
25862608
chain_hash: self.genesis_hash,
25872609
short_channel_id,
25882610
timestamp: chan.get_update_time_counter(),
2589-
flags: (!were_node_one) as u8 | ((!chan.is_live() as u8) << 1),
2611+
flags: (!were_node_one) as u8 | ((!enabled as u8) << 1),
25902612
cltv_expiry_delta: chan.get_cltv_expiry_delta(),
25912613
htlc_minimum_msat: chan.get_counterparty_htlc_minimum_msat(),
25922614
htlc_maximum_msat: chan.get_announced_htlc_max_msat(),
@@ -3736,27 +3758,39 @@ where
37363758
}
37373759

37383760
match chan.channel_update_status() {
3739-
ChannelUpdateStatus::Enabled if !chan.is_live() => chan.set_channel_update_status(ChannelUpdateStatus::DisabledStaged),
3740-
ChannelUpdateStatus::Disabled if chan.is_live() => chan.set_channel_update_status(ChannelUpdateStatus::EnabledStaged),
3741-
ChannelUpdateStatus::DisabledStaged if chan.is_live() => chan.set_channel_update_status(ChannelUpdateStatus::Enabled),
3742-
ChannelUpdateStatus::EnabledStaged if !chan.is_live() => chan.set_channel_update_status(ChannelUpdateStatus::Disabled),
3743-
ChannelUpdateStatus::DisabledStaged if !chan.is_live() => {
3744-
if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
3745-
pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
3746-
msg: update
3747-
});
3761+
ChannelUpdateStatus::Enabled if !chan.is_live() => chan.set_channel_update_status(ChannelUpdateStatus::DisabledStaged(0)),
3762+
ChannelUpdateStatus::Disabled if chan.is_live() => chan.set_channel_update_status(ChannelUpdateStatus::EnabledStaged(0)),
3763+
ChannelUpdateStatus::DisabledStaged(_) if chan.is_live()
3764+
=> chan.set_channel_update_status(ChannelUpdateStatus::Enabled),
3765+
ChannelUpdateStatus::EnabledStaged(_) if !chan.is_live()
3766+
=> chan.set_channel_update_status(ChannelUpdateStatus::Disabled),
3767+
ChannelUpdateStatus::DisabledStaged(mut n) if !chan.is_live() => {
3768+
n += 1;
3769+
if n >= DISABLE_GOSSIP_TICKS {
3770+
chan.set_channel_update_status(ChannelUpdateStatus::Disabled);
3771+
if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
3772+
pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
3773+
msg: update
3774+
});
3775+
}
3776+
should_persist = NotifyOption::DoPersist;
3777+
} else {
3778+
chan.set_channel_update_status(ChannelUpdateStatus::DisabledStaged(n));
37483779
}
3749-
should_persist = NotifyOption::DoPersist;
3750-
chan.set_channel_update_status(ChannelUpdateStatus::Disabled);
37513780
},
3752-
ChannelUpdateStatus::EnabledStaged if chan.is_live() => {
3753-
if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
3754-
pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
3755-
msg: update
3756-
});
3781+
ChannelUpdateStatus::EnabledStaged(mut n) if chan.is_live() => {
3782+
n += 1;
3783+
if n >= ENABLE_GOSSIP_TICKS {
3784+
chan.set_channel_update_status(ChannelUpdateStatus::Enabled);
3785+
if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
3786+
pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
3787+
msg: update
3788+
});
3789+
}
3790+
should_persist = NotifyOption::DoPersist;
3791+
} else {
3792+
chan.set_channel_update_status(ChannelUpdateStatus::EnabledStaged(n));
37573793
}
3758-
should_persist = NotifyOption::DoPersist;
3759-
chan.set_channel_update_status(ChannelUpdateStatus::Enabled);
37603794
},
37613795
_ => {},
37623796
}

‎lightning/src/ln/functional_test_utils.rs

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2724,10 +2724,9 @@ macro_rules! handle_chan_reestablish_msgs {
27242724
}
27252725

27262726
let mut had_channel_update = false; // ChannelUpdate may be now or later, but not both
2727-
if let Some(&MessageSendEvent::SendChannelUpdate { ref node_id, ref msg }) = msg_events.get(idx) {
2727+
if let Some(&MessageSendEvent::SendChannelUpdate { ref node_id, .. }) = msg_events.get(idx) {
27282728
assert_eq!(*node_id, $dst_node.node.get_our_node_id());
27292729
idx += 1;
2730-
assert_eq!(msg.contents.flags & 2, 0); // "disabled" flag must not be set as we just reconnected.
27312730
had_channel_update = true;
27322731
}
27332732

@@ -2771,10 +2770,9 @@ macro_rules! handle_chan_reestablish_msgs {
27712770
}
27722771
}
27732772

2774-
if let Some(&MessageSendEvent::SendChannelUpdate { ref node_id, ref msg }) = msg_events.get(idx) {
2773+
if let Some(&MessageSendEvent::SendChannelUpdate { ref node_id, .. }) = msg_events.get(idx) {
27752774
assert_eq!(*node_id, $dst_node.node.get_our_node_id());
27762775
idx += 1;
2777-
assert_eq!(msg.contents.flags & 2, 0); // "disabled" flag must not be set as we just reconnected.
27782776
assert!(!had_channel_update);
27792777
}
27802778

‎lightning/src/ln/functional_tests.rs

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ use crate::chain::keysinterface::{ChannelSigner, EcdsaChannelSigner, EntropySour
2121
use crate::events::{Event, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentPurpose, ClosureReason, HTLCDestination, PaymentFailureReason};
2222
use crate::ln::{PaymentPreimage, PaymentSecret, PaymentHash};
2323
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};
24-
use crate::ln::channelmanager::{self, PaymentId, RAACommitmentOrder, PaymentSendFailure, RecipientOnionFields, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA};
24+
use crate::ln::channelmanager::{self, PaymentId, RAACommitmentOrder, PaymentSendFailure, RecipientOnionFields, BREAKDOWN_TIMEOUT, ENABLE_GOSSIP_TICKS, DISABLE_GOSSIP_TICKS, MIN_CLTV_EXPIRY_DELTA};
2525
use crate::ln::channel::{Channel, ChannelError};
2626
use crate::ln::{chan_utils, onion_utils};
2727
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() {
71007100
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id());
71017101
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id());
71027102

7103-
nodes[0].node.timer_tick_occurred(); // Enabled -> DisabledStaged
7104-
nodes[0].node.timer_tick_occurred(); // DisabledStaged -> Disabled
7103+
for _ in 0..DISABLE_GOSSIP_TICKS + 1 {
7104+
nodes[0].node.timer_tick_occurred();
7105+
}
71057106
let msg_events = nodes[0].node.get_and_clear_pending_msg_events();
71067107
assert_eq!(msg_events.len(), 3);
71077108
let mut chans_disabled = HashMap::new();
@@ -7141,7 +7142,9 @@ fn test_announce_disable_channels() {
71417142
nodes[1].node.handle_channel_reestablish(&nodes[0].node.get_our_node_id(), &reestablish_1[2]);
71427143
handle_chan_reestablish_msgs!(nodes[1], nodes[0]);
71437144

7144-
nodes[0].node.timer_tick_occurred();
7145+
for _ in 0..ENABLE_GOSSIP_TICKS {
7146+
nodes[0].node.timer_tick_occurred();
7147+
}
71457148
assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
71467149
nodes[0].node.timer_tick_occurred();
71477150
let msg_events = nodes[0].node.get_and_clear_pending_msg_events();

‎lightning/src/ln/onion_route_tests.rs

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ use crate::chain::keysinterface::{EntropySource, NodeSigner, Recipient};
1616
use crate::events::{Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentFailureReason};
1717
use crate::ln::{PaymentHash, PaymentSecret};
1818
use crate::ln::channel::EXPIRE_PREV_CONFIG_TICKS;
19-
use crate::ln::channelmanager::{HTLCForwardInfo, FailureCode, CLTV_FAR_FAR_AWAY, MIN_CLTV_EXPIRY_DELTA, PendingAddHTLCInfo, PendingHTLCInfo, PendingHTLCRouting, PaymentId, RecipientOnionFields};
19+
use crate::ln::channelmanager::{HTLCForwardInfo, FailureCode, CLTV_FAR_FAR_AWAY, DISABLE_GOSSIP_TICKS, MIN_CLTV_EXPIRY_DELTA, PendingAddHTLCInfo, PendingHTLCInfo, PendingHTLCRouting, PaymentId, RecipientOnionFields};
2020
use crate::ln::onion_utils;
2121
use crate::routing::gossip::{NetworkUpdate, RoutingFees};
2222
use crate::routing::router::{get_route, PaymentParameters, Route, RouteHint, RouteHintHop};
@@ -587,6 +587,15 @@ fn test_onion_failure() {
587587
// disconnect event to the channel between nodes[1] ~ nodes[2]
588588
nodes[1].node.peer_disconnected(&nodes[2].node.get_our_node_id());
589589
nodes[2].node.peer_disconnected(&nodes[1].node.get_our_node_id());
590+
}, true, Some(UPDATE|7), Some(NetworkUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy(short_channel_id)}), Some(short_channel_id));
591+
run_onion_failure_test("channel_disabled", 0, &nodes, &route, &payment_hash, &payment_secret, |_| {}, || {
592+
// disconnect event to the channel between nodes[1] ~ nodes[2]
593+
for _ in 0..DISABLE_GOSSIP_TICKS + 1 {
594+
nodes[1].node.timer_tick_occurred();
595+
nodes[2].node.timer_tick_occurred();
596+
}
597+
nodes[1].node.get_and_clear_pending_msg_events();
598+
nodes[2].node.get_and_clear_pending_msg_events();
590599
}, true, Some(UPDATE|20), Some(NetworkUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy(short_channel_id)}), Some(short_channel_id));
591600
reconnect_nodes(&nodes[1], &nodes[2], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
592601

0 commit comments

Comments
 (0)