Skip to content

Commit 09714e6

Browse files
authored
Merge pull request #1169 from TheBlueMatt/2021-11-fix-update-announcements
Fix announcements of our own gossip
2 parents 61518f9 + 0fe2aef commit 09714e6

File tree

6 files changed

+66
-27
lines changed

6 files changed

+66
-27
lines changed

lightning-background-processor/src/lib.rs

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -493,9 +493,10 @@ mod tests {
493493

494494
macro_rules! check_persisted_data {
495495
($node: expr, $filepath: expr, $expected_bytes: expr) => {
496-
match $node.write(&mut $expected_bytes) {
497-
Ok(()) => {
498-
loop {
496+
loop {
497+
$expected_bytes.clear();
498+
match $node.write(&mut $expected_bytes) {
499+
Ok(()) => {
499500
match std::fs::read($filepath) {
500501
Ok(bytes) => {
501502
if bytes == $expected_bytes {
@@ -506,9 +507,9 @@ mod tests {
506507
},
507508
Err(_) => continue
508509
}
509-
}
510-
},
511-
Err(e) => panic!("Unexpected error: {}", e)
510+
},
511+
Err(e) => panic!("Unexpected error: {}", e)
512+
}
512513
}
513514
}
514515
}

lightning/src/ln/channel.rs

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -481,9 +481,14 @@ pub(super) struct Channel<Signer: Sign> {
481481
holding_cell_update_fee: Option<u32>,
482482
next_holder_htlc_id: u64,
483483
next_counterparty_htlc_id: u64,
484-
update_time_counter: u32,
485484
feerate_per_kw: u32,
486485

486+
/// The timestamp set on our latest `channel_update` message for this channel. It is updated
487+
/// when the channel is updated in ways which may impact the `channel_update` message or when a
488+
/// new block is received, ensuring it's always at least moderately close to the current real
489+
/// time.
490+
update_time_counter: u32,
491+
487492
#[cfg(debug_assertions)]
488493
/// Max to_local and to_remote outputs in a locally-generated commitment transaction
489494
holder_max_commitment_tx_output: Mutex<(u64, u64)>,
@@ -4192,6 +4197,7 @@ impl<Signer: Sign> Channel<Signer> {
41924197
}
41934198

41944199
pub fn set_channel_update_status(&mut self, status: ChannelUpdateStatus) {
4200+
self.update_time_counter += 1;
41954201
self.channel_update_status = status;
41964202
}
41974203

lightning/src/ln/functional_tests.rs

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7369,9 +7369,9 @@ fn test_announce_disable_channels() {
73697369
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
73707370
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
73717371

7372-
let short_id_1 = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id;
7373-
let short_id_2 = create_announced_chan_between_nodes(&nodes, 1, 0, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id;
7374-
let short_id_3 = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id;
7372+
create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
7373+
create_announced_chan_between_nodes(&nodes, 1, 0, InitFeatures::known(), InitFeatures::known());
7374+
create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
73757375

73767376
// Disconnect peers
73777377
nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
@@ -7381,13 +7381,13 @@ fn test_announce_disable_channels() {
73817381
nodes[0].node.timer_tick_occurred(); // DisabledStaged -> Disabled
73827382
let msg_events = nodes[0].node.get_and_clear_pending_msg_events();
73837383
assert_eq!(msg_events.len(), 3);
7384-
let mut chans_disabled: HashSet<u64> = [short_id_1, short_id_2, short_id_3].iter().map(|a| *a).collect();
7384+
let mut chans_disabled = HashMap::new();
73857385
for e in msg_events {
73867386
match e {
73877387
MessageSendEvent::BroadcastChannelUpdate { ref msg } => {
73887388
assert_eq!(msg.contents.flags & (1<<1), 1<<1); // The "channel disabled" bit should be set
73897389
// Check that each channel gets updated exactly once
7390-
if !chans_disabled.remove(&msg.contents.short_channel_id) {
7390+
if chans_disabled.insert(msg.contents.short_channel_id, msg.contents.timestamp).is_some() {
73917391
panic!("Generated ChannelUpdate for wrong chan!");
73927392
}
73937393
},
@@ -7423,19 +7423,22 @@ fn test_announce_disable_channels() {
74237423
nodes[0].node.timer_tick_occurred();
74247424
let msg_events = nodes[0].node.get_and_clear_pending_msg_events();
74257425
assert_eq!(msg_events.len(), 3);
7426-
chans_disabled = [short_id_1, short_id_2, short_id_3].iter().map(|a| *a).collect();
74277426
for e in msg_events {
74287427
match e {
74297428
MessageSendEvent::BroadcastChannelUpdate { ref msg } => {
74307429
assert_eq!(msg.contents.flags & (1<<1), 0); // The "channel disabled" bit should be off
7431-
// Check that each channel gets updated exactly once
7432-
if !chans_disabled.remove(&msg.contents.short_channel_id) {
7433-
panic!("Generated ChannelUpdate for wrong chan!");
7430+
match chans_disabled.remove(&msg.contents.short_channel_id) {
7431+
// Each update should have a higher timestamp than the previous one, replacing
7432+
// the old one.
7433+
Some(prev_timestamp) => assert!(msg.contents.timestamp > prev_timestamp),
7434+
None => panic!("Generated ChannelUpdate for wrong chan!"),
74347435
}
74357436
},
74367437
_ => panic!("Unexpected event"),
74377438
}
74387439
}
7440+
// Check that each channel gets updated exactly once
7441+
assert!(chans_disabled.is_empty());
74397442
}
74407443

74417444
#[test]

lightning/src/ln/msgs.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -707,6 +707,10 @@ pub enum ErrorAction {
707707
/// The peer did something harmless that we weren't able to meaningfully process.
708708
/// If the error is logged, log it at the given level.
709709
IgnoreAndLog(logger::Level),
710+
/// The peer provided us with a gossip message which we'd already seen. In most cases this
711+
/// should be ignored, but it may result in the message being forwarded if it is a duplicate of
712+
/// our own channel announcements.
713+
IgnoreDuplicateGossip,
710714
/// The peer did something incorrect. Tell them.
711715
SendErrorMessage {
712716
/// The message to send.

lightning/src/ln/peer_handler.rs

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -811,6 +811,7 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref, CMH: Deref> P
811811
log_given_level!(self.logger, level, "Error handling message{}; ignoring: {}", OptionalFromDebugger(&peer.their_node_id), e.err);
812812
continue
813813
},
814+
msgs::ErrorAction::IgnoreDuplicateGossip => continue, // Don't even bother logging these
814815
msgs::ErrorAction::IgnoreError => {
815816
log_debug!(self.logger, "Error handling message{}; ignoring: {}", OptionalFromDebugger(&peer.their_node_id), e.err);
816817
continue;
@@ -1351,21 +1352,31 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref, CMH: Deref> P
13511352
},
13521353
MessageSendEvent::BroadcastChannelAnnouncement { msg, update_msg } => {
13531354
log_debug!(self.logger, "Handling BroadcastChannelAnnouncement event in peer_handler for short channel id {}", msg.contents.short_channel_id);
1354-
if self.message_handler.route_handler.handle_channel_announcement(&msg).is_ok() && self.message_handler.route_handler.handle_channel_update(&update_msg).is_ok() {
1355-
self.forward_broadcast_msg(peers, &wire::Message::ChannelAnnouncement(msg), None);
1356-
self.forward_broadcast_msg(peers, &wire::Message::ChannelUpdate(update_msg), None);
1355+
match self.message_handler.route_handler.handle_channel_announcement(&msg) {
1356+
Ok(_) | Err(LightningError { action: msgs::ErrorAction::IgnoreDuplicateGossip, .. }) =>
1357+
self.forward_broadcast_msg(peers, &wire::Message::ChannelAnnouncement(msg), None),
1358+
_ => {},
1359+
}
1360+
match self.message_handler.route_handler.handle_channel_update(&update_msg) {
1361+
Ok(_) | Err(LightningError { action: msgs::ErrorAction::IgnoreDuplicateGossip, .. }) =>
1362+
self.forward_broadcast_msg(peers, &wire::Message::ChannelUpdate(update_msg), None),
1363+
_ => {},
13571364
}
13581365
},
13591366
MessageSendEvent::BroadcastNodeAnnouncement { msg } => {
13601367
log_debug!(self.logger, "Handling BroadcastNodeAnnouncement event in peer_handler");
1361-
if self.message_handler.route_handler.handle_node_announcement(&msg).is_ok() {
1362-
self.forward_broadcast_msg(peers, &wire::Message::NodeAnnouncement(msg), None);
1368+
match self.message_handler.route_handler.handle_node_announcement(&msg) {
1369+
Ok(_) | Err(LightningError { action: msgs::ErrorAction::IgnoreDuplicateGossip, .. }) =>
1370+
self.forward_broadcast_msg(peers, &wire::Message::NodeAnnouncement(msg), None),
1371+
_ => {},
13631372
}
13641373
},
13651374
MessageSendEvent::BroadcastChannelUpdate { msg } => {
13661375
log_debug!(self.logger, "Handling BroadcastChannelUpdate event in peer_handler for short channel id {}", msg.contents.short_channel_id);
1367-
if self.message_handler.route_handler.handle_channel_update(&msg).is_ok() {
1368-
self.forward_broadcast_msg(peers, &wire::Message::ChannelUpdate(msg), None);
1376+
match self.message_handler.route_handler.handle_channel_update(&msg) {
1377+
Ok(_) | Err(LightningError { action: msgs::ErrorAction::IgnoreDuplicateGossip, .. }) =>
1378+
self.forward_broadcast_msg(peers, &wire::Message::ChannelUpdate(msg), None),
1379+
_ => {},
13691380
}
13701381
},
13711382
MessageSendEvent::SendChannelUpdate { ref node_id, ref msg } => {
@@ -1398,6 +1409,7 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref, CMH: Deref> P
13981409
msgs::ErrorAction::IgnoreAndLog(level) => {
13991410
log_given_level!(self.logger, level, "Received a HandleError event to be ignored for node {}", log_pubkey!(node_id));
14001411
},
1412+
msgs::ErrorAction::IgnoreDuplicateGossip => {},
14011413
msgs::ErrorAction::IgnoreError => {
14021414
log_debug!(self.logger, "Received a HandleError event to be ignored for node {}", log_pubkey!(node_id));
14031415
},

lightning/src/routing/network_graph.rs

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -847,8 +847,13 @@ impl NetworkGraph {
847847
None => Err(LightningError{err: "No existing channels for node_announcement".to_owned(), action: ErrorAction::IgnoreError}),
848848
Some(node) => {
849849
if let Some(node_info) = node.announcement_info.as_ref() {
850-
if node_info.last_update >= msg.timestamp {
850+
// The timestamp field is somewhat of a misnomer - the BOLTs use it to order
851+
// updates to ensure you always have the latest one, only vaguely suggesting
852+
// that it be at least the current time.
853+
if node_info.last_update > msg.timestamp {
851854
return Err(LightningError{err: "Update older than last processed update".to_owned(), action: ErrorAction::IgnoreAndLog(Level::Gossip)});
855+
} else if node_info.last_update == msg.timestamp {
856+
return Err(LightningError{err: "Update had the same timestamp as last processed update".to_owned(), action: ErrorAction::IgnoreDuplicateGossip});
852857
}
853858
}
854859

@@ -977,7 +982,7 @@ impl NetworkGraph {
977982
Self::remove_channel_in_nodes(&mut nodes, &entry.get(), msg.short_channel_id);
978983
*entry.get_mut() = chan_info;
979984
} else {
980-
return Err(LightningError{err: "Already have knowledge of channel".to_owned(), action: ErrorAction::IgnoreAndLog(Level::Gossip)})
985+
return Err(LightningError{err: "Already have knowledge of channel".to_owned(), action: ErrorAction::IgnoreDuplicateGossip});
981986
}
982987
},
983988
BtreeEntry::Vacant(entry) => {
@@ -1082,8 +1087,16 @@ impl NetworkGraph {
10821087
macro_rules! maybe_update_channel_info {
10831088
( $target: expr, $src_node: expr) => {
10841089
if let Some(existing_chan_info) = $target.as_ref() {
1085-
if existing_chan_info.last_update >= msg.timestamp {
1090+
// The timestamp field is somewhat of a misnomer - the BOLTs use it to
1091+
// order updates to ensure you always have the latest one, only
1092+
// suggesting that it be at least the current time. For
1093+
// channel_updates specifically, the BOLTs discuss the possibility of
1094+
// pruning based on the timestamp field being more than two weeks old,
1095+
// but only in the non-normative section.
1096+
if existing_chan_info.last_update > msg.timestamp {
10861097
return Err(LightningError{err: "Update older than last processed update".to_owned(), action: ErrorAction::IgnoreAndLog(Level::Gossip)});
1098+
} else if existing_chan_info.last_update == msg.timestamp {
1099+
return Err(LightningError{err: "Update had same timestamp as last processed update".to_owned(), action: ErrorAction::IgnoreDuplicateGossip});
10871100
}
10881101
chan_was_enabled = existing_chan_info.enabled;
10891102
} else {
@@ -1720,7 +1733,7 @@ mod tests {
17201733

17211734
match net_graph_msg_handler.handle_channel_update(&valid_channel_update) {
17221735
Ok(_) => panic!(),
1723-
Err(e) => assert_eq!(e.err, "Update older than last processed update")
1736+
Err(e) => assert_eq!(e.err, "Update had same timestamp as last processed update")
17241737
};
17251738
unsigned_channel_update.timestamp += 500;
17261739

0 commit comments

Comments
 (0)