From 6e7000c5e7d246585d876e65a1aa8106fa948f7a Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 17 Apr 2023 22:59:18 +0000 Subject: [PATCH] Set `channel_update` disable bit based on staged even for onions When generating a `channel_update` either in response to a fee configuration change or an HTLC failure, we currently poll the channel to check if the peer's connected when setting the disabled bit in the `channel_update`. This could cause cases where we set the disable bit even though the peer *just* disconnected, and don't generate a followup broadcast `channel_update` with the disabled bit unset. While a node generally shouldn't rebroadcast a `channel_update` it received in an onion, there's nothing inherently stopping them from doing so. Obviously in the fee-update case we expect the message to propagate. Luckily, since we already "stage" disable-changed updates, we can check the staged state and use that to set the disabled bit in all `channel_update` cases. --- lightning/src/ln/channelmanager.rs | 22 ++++++++++++++++++---- lightning/src/ln/functional_test_utils.rs | 6 ++---- lightning/src/ln/onion_route_tests.rs | 9 +++++++++ 3 files changed, 29 insertions(+), 8 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 9f403f85..5cdc07ef 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -2457,7 +2457,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 +2589,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(), @@ -3741,22 +3755,22 @@ where 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 }); } should_persist = NotifyOption::DoPersist; - chan.set_channel_update_status(ChannelUpdateStatus::Disabled); }, 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 }); } should_persist = NotifyOption::DoPersist; - chan.set_channel_update_status(ChannelUpdateStatus::Enabled); }, _ => {}, } diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index f8ccce05..e818bd95 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -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); } diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs index 743d41ee..c7fae84a 100644 --- a/lightning/src/ln/onion_route_tests.rs +++ b/lightning/src/ln/onion_route_tests.rs @@ -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, |_| {}, || { + // 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(); + 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)); -- 2.30.2