Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Gossip filtration fix #3390

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Gossip filtration fix #3390

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8b991c3
Consider received timestamp filters when sharing gossip
arik-so Oct 31, 2024
4630c45
Make dummy ChannelUpdate timestamp dynamic in tests
arik-so Oct 31, 2024
5de1f49
f: respond to comments
arik-so Nov 7, 2024
268ecea
Filter NodeAnnouncement gossip by timestamp.
arik-so Nov 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 37 additions & 12 deletions lightning/src/ln/peer_handler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -586,7 +586,7 @@ struct Peer {
msgs_sent_since_pong: usize,
awaiting_pong_timer_tick_intervals: i64,
received_message_since_timer_tick: bool,
sent_gossip_timestamp_filter: bool,
sent_gossip_timestamp_filter: Option<msgs::GossipTimestampFilter>,

/// Indicates we've received a `channel_announcement` since the last time we had
/// [`PeerManager::gossip_processing_backlogged`] set (or, really, that we've received a
Expand Down Expand Up @@ -614,7 +614,7 @@ impl Peer {
fn should_forward_channel_announcement(&self, channel_id: u64) -> bool {
if !self.handshake_complete() { return false; }
if self.their_features.as_ref().unwrap().supports_gossip_queries() &&
!self.sent_gossip_timestamp_filter {
self.sent_gossip_timestamp_filter.is_none() {
return false;
}
match self.sync_status {
Expand All @@ -628,7 +628,7 @@ impl Peer {
fn should_forward_node_announcement(&self, node_id: NodeId) -> bool {
if !self.handshake_complete() { return false; }
if self.their_features.as_ref().unwrap().supports_gossip_queries() &&
!self.sent_gossip_timestamp_filter {
self.sent_gossip_timestamp_filter.is_none() {
return false;
}
match self.sync_status {
Expand Down Expand Up @@ -1123,7 +1123,7 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, OM: Deref, L: Deref, CM
msgs_sent_since_pong: 0,
awaiting_pong_timer_tick_intervals: 0,
received_message_since_timer_tick: false,
sent_gossip_timestamp_filter: false,
sent_gossip_timestamp_filter: None,

received_channel_announce_since_backlogged: false,
inbound_connection: false,
Expand Down Expand Up @@ -1179,7 +1179,7 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, OM: Deref, L: Deref, CM
msgs_sent_since_pong: 0,
awaiting_pong_timer_tick_intervals: 0,
received_message_since_timer_tick: false,
sent_gossip_timestamp_filter: false,
sent_gossip_timestamp_filter: None,

received_channel_announce_since_backlogged: false,
inbound_connection: true,
Expand Down Expand Up @@ -1224,12 +1224,30 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, OM: Deref, L: Deref, CM
if let Some((announce, update_a_option, update_b_option)) =
self.message_handler.route_handler.get_next_channel_announcement(c)
{
self.enqueue_message(peer, &announce);
let (min, max) = peer.sent_gossip_timestamp_filter.as_ref().map_or((0, u32::MAX), |f| {
(f.first_timestamp, f.first_timestamp.saturating_add(f.timestamp_range))
});

// we only wish to enqueue the announcement if at least one directional
// update is going to accompany it
let mut has_enqueued_announcement = false;

if let Some(update_a) = update_a_option {
self.enqueue_message(peer, &update_a);
if update_a.contents.timestamp >= min && update_a.contents.timestamp <= max {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be < max according to BOLT7. Likewise below.

if !has_enqueued_announcement {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this always evaluate to true?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Copy link
Contributor

@jkczyz jkczyz Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because has_enqueued_announcement is initialized to false and never modified before reaching this line. The variable is actually only used to prevent enqueuing the announcement more than once. The reason given in the comment above is really covered by the let if let Some(update_a|b) checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, I thought you were referring to the timestamp range. Yes, good point

self.enqueue_message(peer, &announce);
has_enqueued_announcement = true;
}
self.enqueue_message(peer, &update_a);
}
}
if let Some(update_b) = update_b_option {
self.enqueue_message(peer, &update_b);
if update_b.contents.timestamp >= min && update_b.contents.timestamp <= max {
if !has_enqueued_announcement {
self.enqueue_message(peer, &announce);
}
self.enqueue_message(peer, &update_b);
}
}
peer.sync_status = InitSyncTracker::ChannelsSyncing(announce.contents.short_channel_id + 1);
} else {
Expand All @@ -1238,7 +1256,14 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, OM: Deref, L: Deref, CM
},
InitSyncTracker::ChannelsSyncing(c) if c == 0xffff_ffff_ffff_ffff => {
if let Some(msg) = self.message_handler.route_handler.get_next_node_announcement(None) {
self.enqueue_message(peer, &msg);
let (min, max) = peer.sent_gossip_timestamp_filter.as_ref().map_or((0, u32::MAX), |f| {
(f.first_timestamp, f.first_timestamp.saturating_add(f.timestamp_range))
});

if msg.contents.timestamp >= min && msg.contents.timestamp <= max {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need to apply the same in NodesSyncing handling below.

self.enqueue_message(peer, &msg);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this now results in us sending node_announcements for nodes for which we didn't send the requisite channel_announcements :/. Maybe its fine, but I'm kinda inclined to just skip all historical state for a peer that sends us a time more recent than now - an hour and otherwise give them a full dump.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the delivery order notwithstanding, don't you think the update timestamps are typically more likely to be higher than their corresponding node announcements' timestamps, meaning that it would be less likely to see a node announcement without a channel announcement than vice versa?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also a bit reluctant to continue bombarding peers with gossip when they're explicitly requesting not to

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't you think the update timestamps are typically more likely to be higher than their corresponding node announcements' timestamps, meaning that it would be less likely to see a node announcement without a channel announcement than vice versa?

Not particularly? Both are generally on a timer, they may be independent timers.

I'm also a bit reluctant to continue bombarding peers with gossip when they're explicitly requesting not to

I don't think I suggested that. Rather, I suggested just skipping historical state sync entirely if we're gonna filter a material number of channels.

}

peer.sync_status = InitSyncTracker::NodesSyncing(msg.contents.node_id);
} else {
peer.sync_status = InitSyncTracker::NoSyncRequested;
Expand Down Expand Up @@ -1722,12 +1747,12 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, OM: Deref, L: Deref, CM
return Err(PeerHandleError { }.into());
}

if let wire::Message::GossipTimestampFilter(_msg) = message {
if let wire::Message::GossipTimestampFilter(msg) = message {
// When supporting gossip messages, start initial gossip sync only after we receive
// a GossipTimestampFilter
if peer_lock.their_features.as_ref().unwrap().supports_gossip_queries() &&
!peer_lock.sent_gossip_timestamp_filter {
peer_lock.sent_gossip_timestamp_filter = true;
peer_lock.sent_gossip_timestamp_filter.is_none() {
peer_lock.sent_gossip_timestamp_filter = Some(msg);
peer_lock.sync_status = InitSyncTracker::ChannelsSyncing(0);
}
return Ok(None);
Expand Down
11 changes: 10 additions & 1 deletion lightning/src/util/test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -963,12 +963,21 @@ fn get_dummy_channel_announcement(short_chan_id: u64) -> msgs::ChannelAnnounceme
fn get_dummy_channel_update(short_chan_id: u64) -> msgs::ChannelUpdate {
use bitcoin::secp256k1::ffi::Signature as FFISignature;
let network = Network::Testnet;

#[allow(unused_mut, unused_assignments)]
let mut timestamp = 0;
#[cfg(feature = "std")]
{
use std::time::{SystemTime, UNIX_EPOCH};
timestamp = SystemTime::now().duration_since(UNIX_EPOCH).expect("Time must be > 1970").as_secs() - 60 * 60 * 24 * 7 * 2;
}

msgs::ChannelUpdate {
signature: Signature::from(unsafe { FFISignature::new() }),
contents: msgs::UnsignedChannelUpdate {
chain_hash: ChainHash::using_genesis_block(network),
short_channel_id: short_chan_id,
timestamp: 0,
timestamp: timestamp as u32,
message_flags: 1, // Only must_be_one
channel_flags: 0,
cltv_expiry_delta: 0,
Expand Down
Loading