Initiate gossip sync only after receiving GossipTimestampFilter. #1452

tnull · 2022-04-26T14:22:18Z

For peers signalling support for gossip queries, this PR delays sending of gossip updates and the initial sync until we receive a GossipTimestampFilter from the counterparty.

Closes #1402.

As suggested there, this PR ignores the particular values in the set filter and only considers whether a filter is set or not (i.e., it provides a boolean option to disable receiving gossip messages).

tnull · 2022-04-26T14:26:43Z

A test case is currently failing since the peers are now signalling support for gossip queries but do not send a GossipTimestampFilter and therefore never start syncing. I'm still looking for a clean way to either inject such messages, or set the sent_gossip_timestamp_filter: bool field from the outside.

TheBlueMatt · 2022-04-26T17:18:46Z

lightning/src/util/test_utils.rs

 pub struct TestRoutingMessageHandler {
 	pub chan_upds_recvd: AtomicUsize,
 	pub chan_anns_recvd: AtomicUsize,
-	pub request_full_sync: AtomicBool,


Seems like we should just fix this to honor request_full_sync and send a gossip filter on connected?

Well, the filter is currently already sent in peer_connected() and since request_full_sync was not used at all, I figured it would sense to simply remove it rather than to add much more logic to the so-far pretty slim TestRoutingMessageHandler. Or are you suggesting to introduce request_full_sync to other places, e.g., NetGraphMsgHandler, so it could be used by should_request_full_sync()?

Wait, who is sending the filter in peer_connected when we're using a TestRoutingMessageHandler? Its peer_connected method is empty.

Ah, now I get what you're saying. You're right, its probably easiest here to let the TestRoutingMessageHandler implementation of peer_connected mirror the functionality of NetGraphMsgHandler.

I went that route (replicating the peer_connected functionality in TestRoutingMessgaeHandler) and it generally seems to work nicely. However, now the second peer receives only 86/100 channel updates and 43/50 channel announcements, i.e., the test now fails at the last two assert_eqs. I currently struggle to find why this would be the case, especially since the logs show that the gossip is indeed just postponed until the peers have received gossip_timestamp_filter messages. I'll come around to continue staring at the code next week.

Yes, and before I adapt the test I at least would like to understand why suddenly not the same number of channel updates are delivered, because its not intuitive to me from the changes I made...

I believe the channel updates will be delivered eventually, but the test currently handles a specific number of round-trips between the peers and then makes sure the messages were all delivered. We now need one more round-trip, I believe (but its not quite in the same set of steps as the existing loop, so we can't just add one).

In 7a99de2 I now moved some things in the loop around. This makes the test succeed hopefully without reducing the coverage too much (i.e., it now checks for B's empty outbound_data once in the end, not in every iteration).

Yea, I mean I think that's fine. That said, I'd kinda prefer to update the pre-loop code to add a few message delivery calls that match the new message flow, but if its too annoying to figure that out exactly I'd be okay landing this as-is. Its kinda nice to use this test to ensure that we don't write more gossip messages to the buffer after we reach our limit, even if it makes the test kinda brittle.

Ah, you're right. The gossip filter messages basically added half a round trip. a31362a should be a cleaner fix now.

TheBlueMatt

Thanks, I'd be happy landing this as-is, but see comment.

lightning/src/ln/peer_handler.rs

TheBlueMatt · 2022-05-02T15:42:09Z

lightning/src/util/test_utils.rs

 pub struct TestRoutingMessageHandler {
 	pub chan_upds_recvd: AtomicUsize,
 	pub chan_anns_recvd: AtomicUsize,
-	pub request_full_sync: AtomicBool,


Yea, I mean I think that's fine. That said, I'd kinda prefer to update the pre-loop code to add a few message delivery calls that match the new message flow, but if its too annoying to figure that out exactly I'd be okay landing this as-is. Its kinda nice to use this test to ensure that we don't write more gossip messages to the buffer after we reach our limit, even if it makes the test kinda brittle.

codecov-commenter · 2022-05-02T16:34:14Z

Codecov Report

Merging #1452 (f8a196c) into main (72069bf) will increase coverage by 1.33%.
The diff coverage is 81.39%.

@@            Coverage Diff             @@
##             main    #1452      +/-   ##
==========================================
+ Coverage   90.87%   92.21%   +1.33%     
==========================================
  Files          75       75              
  Lines       41474    48770    +7296     
  Branches    41474    48770    +7296     
==========================================
+ Hits        37691    44971    +7280     
- Misses       3783     3799      +16

Impacted Files	Coverage Δ
lightning/src/ln/peer_handler.rs	`55.94% <66.66%> (+3.32%)`	⬆️
lightning/src/util/test_utils.rs	`82.84% <95.45%> (+0.41%)`	⬆️
lightning/src/chain/mod.rs	`59.25% <0.00%> (-1.86%)`	⬇️
lightning/src/util/config.rs	`45.05% <0.00%> (-0.78%)`	⬇️
lightning-net-tokio/src/lib.rs	`75.17% <0.00%> (-0.71%)`	⬇️
lightning/src/routing/scoring.rs	`94.00% <0.00%> (-0.36%)`	⬇️
lightning/src/ln/features.rs	`99.59% <0.00%> (+0.15%)`	⬆️
lightning/src/ln/msgs.rs	`86.66% <0.00%> (+0.29%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`95.96% <0.00%> (+0.40%)`	⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72069bf...f8a196c. Read the comment docs.

TheBlueMatt

Good enough for me. LGTM. Lets get another reviewer, but feel free to squash the fixup commits down to clean up the git history in the meantime.

tnull · 2022-05-03T07:13:35Z

Squashed!

arik-so

lgtm

arik-so · 2022-05-03T18:07:56Z

lightning/src/util/test_utils.rs

-		vec![]
+		let mut ret = Vec::new();
+		let mut pending_events = self.pending_events.lock().unwrap();
+		core::mem::swap(&mut ret, &mut pending_events);


purely out of curiosity, what's the benefit of mem::swap over drain?

mem::swap exploits the Rust fact that an object's in-memory representation can always be copied for a move. Thus, this will just copy the (pointer, length, capacity) tuple from one to the other, drain(..) will (or could, I'm not sure how LLVM will optimize it in practice) actually allocate a new vec, move all the old vec elements, and then return that.

arik-so · 2022-05-03T18:11:24Z

lightning/src/util/test_utils.rs

+		#[cfg(feature = "std")]
+		{
+			gossip_start_time = SystemTime::now().duration_since(UNIX_EPOCH).expect("Time must be > 1970").as_secs();
+			if should_request_full_sync {


would it be more legible to write

let delta = if should_request_full_sync { 60 * 60 * 24 * 7 * 2 // two weeks ago } else { 60 * 60 // an hour ago }; gossip_start_time = SystemTime::now().duration_since(UNIX_EPOCH).expect("Time must be > 1970").as_secs() - delta;

?

I believe it was copied from the non-test code, so presumably fine-enough.

tnull changed the title ~~Initiate initial gossip sync only after receiving GossipTimestampFilter.~~ Initiate gossip sync only after receiving GossipTimestampFilter. Apr 26, 2022

TheBlueMatt reviewed Apr 26, 2022

View reviewed changes

tnull force-pushed the 2022-04-honor-gossip-timestamp-filters branch from 4d647d8 to b24ee4c Compare April 28, 2022 11:40

TheBlueMatt reviewed May 2, 2022

View reviewed changes

tnull force-pushed the 2022-04-honor-gossip-timestamp-filters branch from 7a99de2 to a31362a Compare May 2, 2022 16:09

TheBlueMatt reviewed May 2, 2022

View reviewed changes

TheBlueMatt added the Seeking Code Review label May 2, 2022

Initiate sync only after receiving GossipTimestampFilter.

f8a196c

tnull force-pushed the 2022-04-honor-gossip-timestamp-filters branch from a31362a to f8a196c Compare May 3, 2022 07:13

arik-so approved these changes May 3, 2022

View reviewed changes

TheBlueMatt approved these changes May 3, 2022

View reviewed changes

TheBlueMatt merged commit 171dfee into lightningdevkit:main May 3, 2022

Initiate gossip sync only after receiving GossipTimestampFilter. #1452

Initiate gossip sync only after receiving GossipTimestampFilter. #1452

Uh oh!

Conversation

tnull commented Apr 26, 2022

Uh oh!

tnull commented Apr 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull Apr 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

tnull commented May 3, 2022

Uh oh!

arik-so left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arik-so May 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tnull Apr 28, 2022 •

edited

Loading

codecov-commenter commented May 2, 2022 •

edited

Loading

arik-so May 3, 2022 •

edited

Loading