From 74828d243567af448cec5f09eb2d6a8eeed3ca48 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sat, 13 Nov 2021 00:27:05 +0000 Subject: [PATCH 1/5] Separate ChannelAnnouncement and ChannelUpdate broadcast conditions When a `ChannelUpdate` message is generated for broadcast as a part of a `BroadcastChannelAnnouncement` event, it may be newer than our previous `ChannelUpdate` and need to be broadcast. However, if the `ChannelAnnouncement` had already been seen we wouldn't re-broadcast either message as the `handle_channel_announcement` call would fail, short-circuiting the condition to broadcast both. Instead, we split the broadcast of each message as well as the conditional so that we always attempt to handle each message and update our local graph state, then broadcast the message if its update was processed successfully. --- lightning/src/ln/peer_handler.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs index 89f3e9ff5ff..a30e498a62e 100644 --- a/lightning/src/ln/peer_handler.rs +++ b/lightning/src/ln/peer_handler.rs @@ -1351,8 +1351,10 @@ impl P }, MessageSendEvent::BroadcastChannelAnnouncement { msg, update_msg } => { log_debug!(self.logger, "Handling BroadcastChannelAnnouncement event in peer_handler for short channel id {}", msg.contents.short_channel_id); - if self.message_handler.route_handler.handle_channel_announcement(&msg).is_ok() && self.message_handler.route_handler.handle_channel_update(&update_msg).is_ok() { + if self.message_handler.route_handler.handle_channel_announcement(&msg).is_ok() { self.forward_broadcast_msg(peers, &wire::Message::ChannelAnnouncement(msg), None); + } + if self.message_handler.route_handler.handle_channel_update(&update_msg).is_ok() { self.forward_broadcast_msg(peers, &wire::Message::ChannelUpdate(update_msg), None); } }, From 8f89371bae42d127f28b4362322682fe22718175 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sat, 13 Nov 2021 01:06:09 +0000 Subject: [PATCH 2/5] Update `ChannelUpdate::timestamp` when channels are dis-/en-abled We update the `Channel::update_time_counter` field (which is copied into `ChannelUpdate::timestamp`) only when the channel is initialized or closes, and when a new block is connected. However, if a peer disconnects or reconnects, we may wish to generate `ChannelUpdate` updates in between new blocks. In such a case, we need to make sure the `timestamp` field is newer than any previous updates' `timestamp` fields, which we do here by simply incrementing it when the channel status is changed. As a side effect of this we have to update `test_background_processor` to ensure it eventually succeeds even if the serialization of the `ChannelManager` changes after the test begins. --- lightning-background-processor/src/lib.rs | 13 +++++++------ lightning/src/ln/channel.rs | 1 + lightning/src/ln/functional_tests.rs | 21 ++++++++++++--------- 3 files changed, 20 insertions(+), 15 deletions(-) diff --git a/lightning-background-processor/src/lib.rs b/lightning-background-processor/src/lib.rs index 39ecc316a5a..4e98ba9ea0f 100644 --- a/lightning-background-processor/src/lib.rs +++ b/lightning-background-processor/src/lib.rs @@ -493,9 +493,10 @@ mod tests { macro_rules! check_persisted_data { ($node: expr, $filepath: expr, $expected_bytes: expr) => { - match $node.write(&mut $expected_bytes) { - Ok(()) => { - loop { + loop { + $expected_bytes.clear(); + match $node.write(&mut $expected_bytes) { + Ok(()) => { match std::fs::read($filepath) { Ok(bytes) => { if bytes == $expected_bytes { @@ -506,9 +507,9 @@ mod tests { }, Err(_) => continue } - } - }, - Err(e) => panic!("Unexpected error: {}", e) + }, + Err(e) => panic!("Unexpected error: {}", e) + } } } } diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index d76840236a1..0cf7995bce3 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -4167,6 +4167,7 @@ impl Channel { } pub fn set_channel_update_status(&mut self, status: ChannelUpdateStatus) { + self.update_time_counter += 1; self.channel_update_status = status; } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index f144dbd9ad6..af54f9f8d29 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -7313,9 +7313,9 @@ fn test_announce_disable_channels() { let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); let nodes = create_network(2, &node_cfgs, &node_chanmgrs); - let short_id_1 = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id; - let short_id_2 = create_announced_chan_between_nodes(&nodes, 1, 0, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id; - let short_id_3 = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id; + create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()); + create_announced_chan_between_nodes(&nodes, 1, 0, InitFeatures::known(), InitFeatures::known()); + create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()); // Disconnect peers nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false); @@ -7325,13 +7325,13 @@ fn test_announce_disable_channels() { nodes[0].node.timer_tick_occurred(); // DisabledStaged -> Disabled let msg_events = nodes[0].node.get_and_clear_pending_msg_events(); assert_eq!(msg_events.len(), 3); - let mut chans_disabled: HashSet = [short_id_1, short_id_2, short_id_3].iter().map(|a| *a).collect(); + let mut chans_disabled = HashMap::new(); for e in msg_events { match e { MessageSendEvent::BroadcastChannelUpdate { ref msg } => { assert_eq!(msg.contents.flags & (1<<1), 1<<1); // The "channel disabled" bit should be set // Check that each channel gets updated exactly once - if !chans_disabled.remove(&msg.contents.short_channel_id) { + if chans_disabled.insert(msg.contents.short_channel_id, msg.contents.timestamp).is_some() { panic!("Generated ChannelUpdate for wrong chan!"); } }, @@ -7367,19 +7367,22 @@ fn test_announce_disable_channels() { nodes[0].node.timer_tick_occurred(); let msg_events = nodes[0].node.get_and_clear_pending_msg_events(); assert_eq!(msg_events.len(), 3); - chans_disabled = [short_id_1, short_id_2, short_id_3].iter().map(|a| *a).collect(); for e in msg_events { match e { MessageSendEvent::BroadcastChannelUpdate { ref msg } => { assert_eq!(msg.contents.flags & (1<<1), 0); // The "channel disabled" bit should be off - // Check that each channel gets updated exactly once - if !chans_disabled.remove(&msg.contents.short_channel_id) { - panic!("Generated ChannelUpdate for wrong chan!"); + match chans_disabled.remove(&msg.contents.short_channel_id) { + // Each update should have a higher timestamp than the previous one, replacing + // the old one. + Some(prev_timestamp) => assert!(msg.contents.timestamp > prev_timestamp), + None => panic!("Generated ChannelUpdate for wrong chan!"), } }, _ => panic!("Unexpected event"), } } + // Check that each channel gets updated exactly once + assert!(chans_disabled.is_empty()); } #[test] From 391fbfbe1abbd011395542f5f0f96dbc57b8655e Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sat, 13 Nov 2021 01:54:54 +0000 Subject: [PATCH 3/5] Re-broadcast our own gossip even if its same as the last broadcast Even if our gossip hasn't changed, we should be willing to re-broadcast it to our peers. All our peers may have been disconnected the last time we broadcasted it. --- lightning/src/ln/msgs.rs | 4 ++++ lightning/src/ln/peer_handler.rs | 26 ++++++++++++++++++-------- lightning/src/routing/network_graph.rs | 12 ++++++++---- 3 files changed, 30 insertions(+), 12 deletions(-) diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs index 87944ccae63..9662ffa7d48 100644 --- a/lightning/src/ln/msgs.rs +++ b/lightning/src/ln/msgs.rs @@ -715,6 +715,10 @@ pub enum ErrorAction { /// The peer did something harmless that we weren't able to meaningfully process. /// If the error is logged, log it at the given level. IgnoreAndLog(logger::Level), + /// The peer provided us with a gossip message which we'd already seen. In most cases this + /// should be ignored, but it may result in the message being forwarded if it is a duplicate of + /// our own channel announcements. + IgnoreDuplicateGossip, /// The peer did something incorrect. Tell them. SendErrorMessage { /// The message to send. diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs index a30e498a62e..9157dc87dc2 100644 --- a/lightning/src/ln/peer_handler.rs +++ b/lightning/src/ln/peer_handler.rs @@ -811,6 +811,7 @@ impl P log_given_level!(self.logger, level, "Error handling message{}; ignoring: {}", OptionalFromDebugger(&peer.their_node_id), e.err); continue }, + msgs::ErrorAction::IgnoreDuplicateGossip => continue, // Don't even bother logging these msgs::ErrorAction::IgnoreError => { log_debug!(self.logger, "Error handling message{}; ignoring: {}", OptionalFromDebugger(&peer.their_node_id), e.err); continue; @@ -1351,23 +1352,31 @@ impl P }, MessageSendEvent::BroadcastChannelAnnouncement { msg, update_msg } => { log_debug!(self.logger, "Handling BroadcastChannelAnnouncement event in peer_handler for short channel id {}", msg.contents.short_channel_id); - if self.message_handler.route_handler.handle_channel_announcement(&msg).is_ok() { - self.forward_broadcast_msg(peers, &wire::Message::ChannelAnnouncement(msg), None); + match self.message_handler.route_handler.handle_channel_announcement(&msg) { + Ok(_) | Err(LightningError { action: msgs::ErrorAction::IgnoreDuplicateGossip, .. }) => + self.forward_broadcast_msg(peers, &wire::Message::ChannelAnnouncement(msg), None), + _ => {}, } - if self.message_handler.route_handler.handle_channel_update(&update_msg).is_ok() { - self.forward_broadcast_msg(peers, &wire::Message::ChannelUpdate(update_msg), None); + match self.message_handler.route_handler.handle_channel_update(&update_msg) { + Ok(_) | Err(LightningError { action: msgs::ErrorAction::IgnoreDuplicateGossip, .. }) => + self.forward_broadcast_msg(peers, &wire::Message::ChannelUpdate(update_msg), None), + _ => {}, } }, MessageSendEvent::BroadcastNodeAnnouncement { msg } => { log_debug!(self.logger, "Handling BroadcastNodeAnnouncement event in peer_handler"); - if self.message_handler.route_handler.handle_node_announcement(&msg).is_ok() { - self.forward_broadcast_msg(peers, &wire::Message::NodeAnnouncement(msg), None); + match self.message_handler.route_handler.handle_node_announcement(&msg) { + Ok(_) | Err(LightningError { action: msgs::ErrorAction::IgnoreDuplicateGossip, .. }) => + self.forward_broadcast_msg(peers, &wire::Message::NodeAnnouncement(msg), None), + _ => {}, } }, MessageSendEvent::BroadcastChannelUpdate { msg } => { log_debug!(self.logger, "Handling BroadcastChannelUpdate event in peer_handler for short channel id {}", msg.contents.short_channel_id); - if self.message_handler.route_handler.handle_channel_update(&msg).is_ok() { - self.forward_broadcast_msg(peers, &wire::Message::ChannelUpdate(msg), None); + match self.message_handler.route_handler.handle_channel_update(&msg) { + Ok(_) | Err(LightningError { action: msgs::ErrorAction::IgnoreDuplicateGossip, .. }) => + self.forward_broadcast_msg(peers, &wire::Message::ChannelUpdate(msg), None), + _ => {}, } }, MessageSendEvent::SendChannelUpdate { ref node_id, ref msg } => { @@ -1400,6 +1409,7 @@ impl P msgs::ErrorAction::IgnoreAndLog(level) => { log_given_level!(self.logger, level, "Received a HandleError event to be ignored for node {}", log_pubkey!(node_id)); }, + msgs::ErrorAction::IgnoreDuplicateGossip => {}, msgs::ErrorAction::IgnoreError => { log_debug!(self.logger, "Received a HandleError event to be ignored for node {}", log_pubkey!(node_id)); }, diff --git a/lightning/src/routing/network_graph.rs b/lightning/src/routing/network_graph.rs index 4d33bbdee92..2dba12b630f 100644 --- a/lightning/src/routing/network_graph.rs +++ b/lightning/src/routing/network_graph.rs @@ -847,8 +847,10 @@ impl NetworkGraph { None => Err(LightningError{err: "No existing channels for node_announcement".to_owned(), action: ErrorAction::IgnoreError}), Some(node) => { if let Some(node_info) = node.announcement_info.as_ref() { - if node_info.last_update >= msg.timestamp { + if node_info.last_update > msg.timestamp { return Err(LightningError{err: "Update older than last processed update".to_owned(), action: ErrorAction::IgnoreAndLog(Level::Gossip)}); + } else if node_info.last_update == msg.timestamp { + return Err(LightningError{err: "Update had the same timestamp as last processed update".to_owned(), action: ErrorAction::IgnoreDuplicateGossip}); } } @@ -977,7 +979,7 @@ impl NetworkGraph { Self::remove_channel_in_nodes(&mut nodes, &entry.get(), msg.short_channel_id); *entry.get_mut() = chan_info; } else { - return Err(LightningError{err: "Already have knowledge of channel".to_owned(), action: ErrorAction::IgnoreAndLog(Level::Gossip)}) + return Err(LightningError{err: "Already have knowledge of channel".to_owned(), action: ErrorAction::IgnoreDuplicateGossip}); } }, BtreeEntry::Vacant(entry) => { @@ -1082,8 +1084,10 @@ impl NetworkGraph { macro_rules! maybe_update_channel_info { ( $target: expr, $src_node: expr) => { if let Some(existing_chan_info) = $target.as_ref() { - if existing_chan_info.last_update >= msg.timestamp { + if existing_chan_info.last_update > msg.timestamp { return Err(LightningError{err: "Update older than last processed update".to_owned(), action: ErrorAction::IgnoreAndLog(Level::Gossip)}); + } else if existing_chan_info.last_update == msg.timestamp { + return Err(LightningError{err: "Update had same timestamp as last processed update".to_owned(), action: ErrorAction::IgnoreDuplicateGossip}); } chan_was_enabled = existing_chan_info.enabled; } else { @@ -1720,7 +1724,7 @@ mod tests { match net_graph_msg_handler.handle_channel_update(&valid_channel_update) { Ok(_) => panic!(), - Err(e) => assert_eq!(e.err, "Update older than last processed update") + Err(e) => assert_eq!(e.err, "Update had same timestamp as last processed update") }; unsigned_channel_update.timestamp += 500; From d2ac683f4e6267b1ae8997a97b1c1dec1cd540c2 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 16 Nov 2021 20:55:10 +0000 Subject: [PATCH 4/5] Add a comment describing `update_time_counter` and when its updated --- lightning/src/ln/channel.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 0cf7995bce3..55dd76cc033 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -481,9 +481,14 @@ pub(super) struct Channel { holding_cell_update_fee: Option, next_holder_htlc_id: u64, next_counterparty_htlc_id: u64, - update_time_counter: u32, feerate_per_kw: u32, + /// The timestamp set on our latest `channel_update` message for this channel. It is updated + /// when the channel is updated in ways which may impact the `channel_update` message or when a + /// new block is received, ensuring it's always at least moderately close to the current real + /// time. + update_time_counter: u32, + #[cfg(debug_assertions)] /// Max to_local and to_remote outputs in a locally-generated commitment transaction holder_max_commitment_tx_output: Mutex<(u64, u64)>, From 0fe2aef0e6f12838e05521d3f38a51e7c03e5380 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 9 Dec 2021 03:49:50 +0000 Subject: [PATCH 5/5] Add a comment describing the timestamp field's use --- lightning/src/routing/network_graph.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/lightning/src/routing/network_graph.rs b/lightning/src/routing/network_graph.rs index 2dba12b630f..10c0ba57b58 100644 --- a/lightning/src/routing/network_graph.rs +++ b/lightning/src/routing/network_graph.rs @@ -847,6 +847,9 @@ impl NetworkGraph { None => Err(LightningError{err: "No existing channels for node_announcement".to_owned(), action: ErrorAction::IgnoreError}), Some(node) => { if let Some(node_info) = node.announcement_info.as_ref() { + // The timestamp field is somewhat of a misnomer - the BOLTs use it to order + // updates to ensure you always have the latest one, only vaguely suggesting + // that it be at least the current time. if node_info.last_update > msg.timestamp { return Err(LightningError{err: "Update older than last processed update".to_owned(), action: ErrorAction::IgnoreAndLog(Level::Gossip)}); } else if node_info.last_update == msg.timestamp { @@ -1084,6 +1087,12 @@ impl NetworkGraph { macro_rules! maybe_update_channel_info { ( $target: expr, $src_node: expr) => { if let Some(existing_chan_info) = $target.as_ref() { + // The timestamp field is somewhat of a misnomer - the BOLTs use it to + // order updates to ensure you always have the latest one, only + // suggesting that it be at least the current time. For + // channel_updates specifically, the BOLTs discuss the possibility of + // pruning based on the timestamp field being more than two weeks old, + // but only in the non-normative section. if existing_chan_info.last_update > msg.timestamp { return Err(LightningError{err: "Update older than last processed update".to_owned(), action: ErrorAction::IgnoreAndLog(Level::Gossip)}); } else if existing_chan_info.last_update == msg.timestamp {