Gossip filtration fix #3390
Gossip filtration fix #3390
Conversation
lightning/src/ln/peer_handler.rs
Outdated
| (f.first_timestamp, f.first_timestamp.saturating_add(f.timestamp_range)) | ||
| }); | ||
|
|
||
| if msg.contents.timestamp >= min && msg.contents.timestamp <= max { |
There was a problem hiding this comment.
You'll need to apply the same in NodesSyncing handling below.
| }); | ||
|
|
||
| if msg.contents.timestamp >= min && msg.contents.timestamp <= max { | ||
| self.enqueue_message(peer, &msg); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I'm also a bit reluctant to continue bombarding peers with gossip when they're explicitly requesting not to
There was a problem hiding this comment.
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.
lightning/src/ln/peer_handler.rs
Outdated
| 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 { | ||
| if !has_enqueued_announcement { |
There was a problem hiding this comment.
Won't this always evaluate to true?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
ah, I thought you were referring to the timestamp range. Yes, good point
lightning/src/ln/peer_handler.rs
Outdated
| 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 { |
There was a problem hiding this comment.
Should be < max according to BOLT7. Likewise below.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3390 +/- ##
==========================================
+ Coverage 89.57% 89.63% +0.05%
==========================================
Files 125 128 +3
Lines 101756 104926 +3170
Branches 101756 104926 +3170
==========================================
+ Hits 91151 94052 +2901
- Misses 7884 8165 +281
+ Partials 2721 2709 -12 ☔ View full report in Codecov by Sentry. |
Previously, upon receipt of a
GossipTimestampFiltermessage, we would immediately start unloading the entire network graph on our unsuspecting peer.This PR modifies our behavior to take their requested range into consideration. Potential for future optimization is pre-sorting our channel updates and node announcements by their timestamps so as to more effectively adhere to the filtering request.