Gossip filtration fix #3390

arik-so · 2024-10-31T05:10:48Z

Previously, upon receipt of a GossipTimestampFilter message, we would immediately start unloading the entire network graph on our unsuspecting peer.

This PR modifies our behavior to only do so if the timestamp of the filter message is at least an hour old. Otherwise, we only send updated sync data as it comes in.

TheBlueMatt · 2024-10-31T15:59:18Z

lightning/src/ln/peer_handler.rs

+								(f.first_timestamp, f.first_timestamp.saturating_add(f.timestamp_range))
+							});
+
+							if msg.contents.timestamp >= min && msg.contents.timestamp <= max {


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

lightning/src/ln/peer_handler.rs

codecov · 2024-11-07T15:40:31Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.94%. Comparing base (8da30df) to head (249cf2c).
Report is 40 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3390      +/-   ##
==========================================
+ Coverage   89.61%   90.94%   +1.33%     
==========================================
  Files         129      130       +1     
  Lines      105506   114915    +9409     
  Branches   105506   114915    +9409     
==========================================
+ Hits        94544   104508    +9964     
+ Misses       8208     7986     -222     
+ Partials     2754     2421     -333

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lightning/src/ln/peer_handler.rs

arik-so · 2024-11-21T17:23:28Z

And if the newly added check doesn't pass, we won't send any gossip.

I have split the check in two to address that, @jkczyz.

jkczyz

LGTM. May want to add a test as mentioned offline. Also, would be good to document InitSyncTracker as otherwise you need to read the code to figure out the behavior.

lightning/src/ln/peer_handler.rs

arik-so · 2024-12-02T14:20:55Z

peer_connected in test_utils sets the gossip start time for the GossipTimestampFilter message to either an hour ago for no sync, or two weeks ago for a full sync, which basically covers the two scenarios this change discerns. Only additional thing we could test would be 6 hours ± 1 second, but I'm not sure that's even worth it.

lightning/src/ln/peer_handler.rs

G8XSU · 2024-12-04T02:31:13Z

lightning/src/ln/peer_handler.rs

+					use std::time::{SystemTime, UNIX_EPOCH};
+					full_sync_threshold = SystemTime::now().duration_since(UNIX_EPOCH).expect("Time must be > 1970").as_secs() - 6 * 3600;
+				}
+				if (msg.first_timestamp as u64) <= full_sync_threshold {


for non-std builds, full_sync_threshold will be left unchanged to 0, and we will never trigger a full gossip sync even if message contained now-2weeks ?

my understanding is that for non-std builds, we rather always trigger a full gossip sync

I made the code a bit clearer for default no-std behavior.

iiuc, it wouldn't have worked as expected earlier. lmk if that was not the case.

G8XSU

Does what it says, and handles non-std behavior correctly.
Lgtm, unless other reviewers have more feedback!

G8XSU · 2024-12-05T00:37:25Z

lightning/src/ln/peer_handler.rs

@@ -1724,10 +1733,26 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, OM: Deref, L: Deref, CM
 		if let wire::Message::GossipTimestampFilter(_msg) = message {
 			// When supporting gossip messages, start initial gossip sync only after we receive
 			// a GossipTimestampFilter
+


nit: newline not required.

G8XSU · 2024-12-05T00:37:27Z

lightning/src/ln/peer_handler.rs

+				#[cfg(feature = "std")]
+				{
+					// if the timestamp range starts more than six hours ago, do a full sync
+					// otherwise, start at the current time


nit: "otherwise, do not send historical gossip."

lightning/src/ln/peer_handler.rs

G8XSU · 2024-12-05T00:39:50Z

lightning/src/ln/peer_handler.rs

 			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.sync_status = InitSyncTracker::ChannelsSyncing(0);
+
+				#[allow(unused_mut, unused_assignments)]


nit: unused_assignments should no longer be required.

TheBlueMatt

Presumably the second commit should be squashed as a fixup?

lightning/src/ln/peer_handler.rs

arik-so · 2024-12-05T17:09:02Z

squashed

jkczyz

LGTM other than re-wording a comment.

jkczyz · 2024-12-05T17:13:00Z

lightning/src/ln/peer_handler.rs

+					// if the timestamp range starts more than six hours ago, do a full sync
+					// otherwise, only forward ad-hoc gossip


This comment would be less confusing if it was re-phrased to say "less than six hours ago" as that is the condition where should_do_full_sync is modified below.

eh, I think it's quite clear honestly

It describes the opposite of the if clause as worded.

how about this phrasing I just pushed:

// determine if the timestamp range starts more than six hours ago
// if it doesn't, only forward ad-hoc gossip

Eh, I mean, I read the comment then see if the code does what it says, but I need to reverse the logic to determine that. Can't we just say: "Forward ad-hoc gossip if the timestamp range is less than six hours ago. Otherwise, do a full sync."?

Looks the same?

oops, push had failed. Worked now

G8XSU · 2024-12-05T17:49:54Z

Current git commit message is confusing though.
"Trigger full sync on old GossipTimestampFilter",
The real thing we wanted to achieve/fix was "Do not trigger full gossip sync on every GossipTimestampFilter."

arik-so · 2024-12-05T17:50:58Z

Would you like me to add an "only" to the commit message? Or rephrase it?

G8XSU · 2024-12-05T17:52:13Z

anything is fine i guess, upto you

arik-so · 2024-12-05T17:54:04Z

ok, sure, I rephrased it

G8XSU · 2024-12-05T17:57:07Z

Waiting on ci to merge.

Previously, upon receipt of a GossipTimestampFilter message, we would immediately start unloading the entire network graph on our unsuspecting peer. This commit modifies our behavior to only do so if the timestamp of the filter message is at least six hour old. Otherwise, we only send updated sync data as it comes in.

jkczyz · 2024-12-05T18:12:45Z

Let's expanded the commit message with the why from the PR description, as discussed offline.

arik-so · 2024-12-05T18:14:39Z

hm, the expanded message seems to show up for me, maybe Github doesn't auto-refresh?

G8XSU · 2024-12-05T19:30:51Z

There is a fuzz test failure for 'test_gossip_exchange_breakage', looks like it is related.

valentinewallace · 2024-12-05T20:05:27Z

There is a fuzz test failure for 'test_gossip_exchange_breakage', looks like it is related.

I believe this is from #3439, working on a fix for this

G8XSU · 2024-12-05T20:06:10Z

I believe this is from #3439, working on a fix for this

Sounds good, will merge this after ci completes in that case.

TheBlueMatt reviewed Oct 31, 2024

View reviewed changes

jkczyz reviewed Oct 31, 2024

View reviewed changes

lightning/src/ln/peer_handler.rs Outdated Show resolved Hide resolved

lightning/src/ln/peer_handler.rs Outdated Show resolved Hide resolved

arik-so force-pushed the gossip-filtration-fix branch from 268ecea to 3854be0 Compare November 15, 2024 21:41

jkczyz reviewed Nov 20, 2024

View reviewed changes

lightning/src/ln/peer_handler.rs Outdated Show resolved Hide resolved

lightning/src/ln/peer_handler.rs Outdated Show resolved Hide resolved

arik-so force-pushed the gossip-filtration-fix branch from 47f4228 to e5f9a11 Compare November 21, 2024 16:25

jkczyz reviewed Nov 22, 2024

View reviewed changes

TheBlueMatt reviewed Nov 25, 2024

View reviewed changes

lightning/src/ln/peer_handler.rs Outdated Show resolved Hide resolved

arik-so requested a review from jkczyz December 4, 2024 01:48

G8XSU reviewed Dec 4, 2024

View reviewed changes

G8XSU reviewed Dec 5, 2024

View reviewed changes

TheBlueMatt added this to the 0.1 milestone Dec 5, 2024

arik-so force-pushed the gossip-filtration-fix branch 2 times, most recently from 249cf2c to e582222 Compare December 5, 2024 06:39

TheBlueMatt reviewed Dec 5, 2024

View reviewed changes

lightning/src/ln/peer_handler.rs Show resolved Hide resolved

arik-so force-pushed the gossip-filtration-fix branch from e582222 to 0247ad0 Compare December 5, 2024 17:08

jkczyz reviewed Dec 5, 2024

View reviewed changes

arik-so force-pushed the gossip-filtration-fix branch 2 times, most recently from b7a7ef6 to b350233 Compare December 5, 2024 17:45

G8XSU approved these changes Dec 5, 2024

View reviewed changes

jkczyz approved these changes Dec 5, 2024

View reviewed changes

arik-so force-pushed the gossip-filtration-fix branch from b350233 to a837f2b Compare December 5, 2024 17:53

arik-so force-pushed the gossip-filtration-fix branch from a837f2b to 145e1ab Compare December 5, 2024 18:06

jkczyz approved these changes Dec 5, 2024

View reviewed changes

G8XSU merged commit 8a54da6 into lightningdevkit:main Dec 6, 2024
17 of 19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gossip filtration fix #3390

Gossip filtration fix #3390

arik-so commented Oct 31, 2024 •

edited

Loading

TheBlueMatt Oct 31, 2024

codecov bot commented Nov 7, 2024 •

edited

Loading

arik-so commented Nov 21, 2024

jkczyz left a comment

arik-so commented Dec 2, 2024

G8XSU Dec 4, 2024

arik-so Dec 4, 2024

arik-so Dec 4, 2024

G8XSU Dec 5, 2024

G8XSU left a comment

G8XSU Dec 5, 2024

G8XSU Dec 5, 2024

G8XSU Dec 5, 2024

TheBlueMatt left a comment

arik-so commented Dec 5, 2024

jkczyz left a comment

jkczyz Dec 5, 2024

arik-so Dec 5, 2024

jkczyz Dec 5, 2024

arik-so Dec 5, 2024

jkczyz Dec 5, 2024

arik-so Dec 5, 2024

jkczyz Dec 5, 2024

arik-so Dec 5, 2024

G8XSU commented Dec 5, 2024

arik-so commented Dec 5, 2024

G8XSU commented Dec 5, 2024

arik-so commented Dec 5, 2024 •

edited

Loading

G8XSU commented Dec 5, 2024

jkczyz commented Dec 5, 2024

arik-so commented Dec 5, 2024

G8XSU commented Dec 5, 2024

valentinewallace commented Dec 5, 2024

G8XSU commented Dec 5, 2024

		// if the timestamp range starts more than six hours ago, do a full sync
		// otherwise, only forward ad-hoc gossip

Gossip filtration fix #3390

Gossip filtration fix #3390

Conversation

arik-so commented Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Nov 7, 2024 • edited Loading

Codecov Report

arik-so commented Nov 21, 2024

jkczyz left a comment

Choose a reason for hiding this comment

arik-so commented Dec 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

G8XSU left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

arik-so commented Dec 5, 2024

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

G8XSU commented Dec 5, 2024

arik-so commented Dec 5, 2024

G8XSU commented Dec 5, 2024

arik-so commented Dec 5, 2024 • edited Loading

G8XSU commented Dec 5, 2024

jkczyz commented Dec 5, 2024

arik-so commented Dec 5, 2024

G8XSU commented Dec 5, 2024

valentinewallace commented Dec 5, 2024

G8XSU commented Dec 5, 2024

arik-so commented Oct 31, 2024 •

edited

Loading

codecov bot commented Nov 7, 2024 •

edited

Loading

arik-so commented Dec 5, 2024 •

edited

Loading