Support only one `GossipSync` in `BackgroundProcessor` #1517

jkczyz · 2022-06-03T17:56:04Z

BackgroundProcessor can take an optional P2PGossipSync and an optional RapidGossipSync, but doing so may be easy to misuse. Each has a reference to a NetworkGraph, which could be different between the two, but only one is actually used.

Instead, allow passing one object wrapped in a GossipSync enum. Also, fix a bug where the NetworkGraph is not persisted on shutdown if only a RapidGossipSync is given.

P2PGossipSync has a Secp256k1 context field, which it only uses to pass to NetworkGraph methods. Move the field to NetworkGraph so other callers don't need to pass in a Secp256k1 context.

codecov-commenter · 2022-06-03T18:19:45Z

Codecov Report

Merging #1517 (498ae1b) into main (0017bc8) will decrease coverage by 0.02%.
The diff coverage is 94.84%.

@@            Coverage Diff             @@
##             main    #1517      +/-   ##
==========================================
- Coverage   90.95%   90.93%   -0.03%     
==========================================
  Files          80       80              
  Lines       43347    43426      +79     
  Branches    43347    43426      +79     
==========================================
+ Hits        39428    39488      +60     
- Misses       3919     3938      +19

Impacted Files	Coverage Δ
lightning/src/ln/peer_handler.rs	`57.06% <ø> (ø)`
lightning/src/util/events.rs	`41.66% <ø> (ø)`
lightning-invoice/src/utils.rs	`96.63% <66.66%> (-0.14%)`	⬇️
lightning/src/routing/router.rs	`92.46% <78.94%> (+0.01%)`	⬆️
lightning/src/routing/gossip.rs	`91.69% <93.54%> (+0.63%)`	⬆️
lightning-background-processor/src/lib.rs	`95.20% <100.00%> (+0.21%)`	⬆️
lightning-rapid-gossip-sync/src/lib.rs	`90.90% <100.00%> (+0.24%)`	⬆️
lightning-rapid-gossip-sync/src/processing.rs	`90.43% <100.00%> (+0.25%)`	⬆️
lightning/src/ln/channelmanager.rs	`84.41% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`95.41% <100.00%> (-0.01%)`	⬇️
... and 14 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 0017bc8...498ae1b. Read the comment docs.

TheBlueMatt · 2022-06-03T23:33:05Z

lightning/src/routing/gossip.rs

-				log_debug!(self.logger, "{} node graph entry for {} due to a payment failure.", action, node_id);
-				self.network_graph.node_failed(node_id, is_permanent);
-			},
+impl<G: Deref<Target = NetworkGraph>, L: Deref> EventHandler for (&G, &L)


This is pretty awkward, and I don't see why its worth the effort just to avoid storing a Deref<Logger> in the NetworkGraph itself - IMO there's not really any reason we shouldn't have a logger reference in basically any struct, and definitely so if it causes awkward stuff here instead.

Figured this was the simplest approach, but doesn't look like there's many places where we instantiate a NetworkGraph. So I can update it to take a Logger, if you prefer.

Any reason why we implement Clone for NetworkGraph? That means we'd need the parameterization to be L::Target: Logger + Clone, I think. Removing the Clone implementation doesn't break any tests, AFAICT.

Ah, found 6c569d8. 🙁

Maybe the reasoning isn't relevant anymore given P2PGossipSync doesn't own the NetworkGraph?

Hmmm, it's probably still relevant, but we can deal with it downstream (maybe make Logger require clone for bindings?). Let's Do It Right in Rust and we can deal with bindings things in bindings.

Alright, this is turning out to be more involved than I thought. Need a second pair of eyes on how to resolve this:

Checking lightning-invoice v0.14.0 (/Users/jkczyz/src/rust-lightning/lightning-invoice) error[E0308]: mismatched types --> lightning-invoice/src/utils.rs:469:63 | 458 | impl<G: Deref<Target = NetworkGraph<L>>, L: Deref, S: Score> Router<S> for DefaultRouter<G, L> | - this type parameter ... 469 | find_route(payer, params, &*self.network_graph, first_hops, &*self.logger, scorer, &random_seed_bytes) | ^^^^^^^^^^^^^ expected type parameter `L`, found reference | = note: expected type parameter `L` found reference `&<L as Deref>::Target`

From what I can tell, we're trying to pass find_route two different types for L but it wants them to be the same. Is the solution to just make two type parameters? (i.e., one for the network graph and one for the logger)

Here's the WIP commit with the compilation error: jkczyz@0f5bcd1

Or maybe update find_route to take &ReadOnlyNetworkGraph like get_route to avoid this entirely?

Have it working with ReadOnlyNetworkGraph, FYI.

lightning-background-processor/src/lib.rs

arik-so · 2022-06-04T10:57:45Z

fuzz/src/full_stack.rs

@@ -460,7 +460,7 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
 					final_cltv_expiry_delta: 42,
 				};
 				let random_seed_bytes: [u8; 32] = keys_manager.get_secure_random_bytes();
-				let route = match find_route(&our_id, &params, &network_graph, None, Arc::clone(&logger), &scorer, &random_seed_bytes) {
+				let route = match find_route(&our_id, &params, &network_graph.read_only(), None, Arc::clone(&logger), &scorer, &random_seed_bytes) {


great idea!

arik-so · 2022-06-05T14:32:07Z

lightning-rapid-gossip-sync/src/lib.rs

@@ -30,10 +30,12 @@
 //! use bitcoin::blockdata::constants::genesis_block;
 //! use bitcoin::Network;
 //! use lightning::routing::gossip::NetworkGraph;
+//! use lightning::util::test_utils::TestLogger;


should the documentation be relying on a test utility?

Added a hidden FakeLogger definition and instantiation. Let me know if you prefer something else.

lightning-rapid-gossip-sync/src/processing.rs

arik-so · 2022-06-05T14:40:04Z

lightning-background-processor/src/lib.rs

 	>(
 		persister: PS, event_handler: EH, chain_monitor: M, channel_manager: CM,
-		p2p_gossip_sync: Option<PGS>, peer_manager: PM, logger: L, scorer: Option<S>,
-		rapid_gossip_sync: Option<RGS>
+		gossip_sync: Option<GossipSync<PGS, RGS, G, CA, L>>, peer_manager: PM, logger: L,


should gossip sync still be an option? I don't recall the net graph message handler being an option, however, I guess it could have been an ignoring message handler

Yeah, it was previously an option in case a user wants to delegate to a trusted server for constructing a route. NetGraphMsgHandler was simply renamed to P2PGossipSync.

Maybe its easier to just have an enum variant for NoGossip or whatever?

Yeah, that's a bit cleaner actually

lightning-background-processor/src/lib.rs

arik-so · 2022-06-06T03:28:26Z

lightning-background-processor/src/lib.rs

+	P2P(P),
+	/// Rapid gossip sync from a trusted server.
+	Rapid(R),
+	/// No gossip sync.


hm, I actually think an option might convey nullability better than a none, although this definitely takes much less code. However, for people who want to do an initial rapid catch up, but then supplement it with real data as it becomes available, do you think both makes sense?

hm, I actually think an option might convey nullability better than a none, although this definitely takes much less code.

I also kinda imagine it's simpler in the bindings, no?

However, for people who want to do an initial rapid catch up, but then supplement it with real data as it becomes available, do you think both makes sense?

We currently can't really support that - the lightning protocol doesn't have a way of asking for new gossip without missing lots of gossip, so users probably can't really do this.

arik-so · 2022-06-06T17:42:19Z

Feel free to squash at this point, I don't have anything to complain about right now :)

P2PGossipSync logs before delegating to NetworkGraph in its EventHandler. In order to share this handling with RapidGossipSync, NetworkGraph needs to take a logger so that it can implement EventHandler instead.

Instead of implementing EventHandler for P2PGossipSync, implement it on NetworkGraph. This allows RapidGossipSync to handle events, too, by delegating to its NetworkGraph.

BackgroundProcessor can take an optional P2PGossipSync and an optional RapidGossipSync, but doing so may be easy to misuse. Each has a reference to a NetworkGraph, which could be different between the two, but only one is actually used. Instead, allow passing one object wrapped in a GossipSync enum. Also, fix a bug where the NetworkGraph is not persisted on shutdown if only a RapidGossipSync is given.

TheBlueMatt

This is fine for release, but would be good to drop the unnecessary Arc in fuzzing in a followup.

TheBlueMatt · 2022-06-06T22:34:19Z

fuzz/src/process_network_graph.rs

 	let block_hash = bitcoin::BlockHash::default();
-	let network_graph = lightning::routing::gossip::NetworkGraph::new(block_hash);
+	let logger: Arc<dyn Logger> = Arc::new(test_logger::TestLogger::new("".to_owned(), out));


No need for dyn or Arc here?

TheBlueMatt · 2022-06-06T22:53:41Z

lightning/src/routing/gossip.rs

@@ -2380,7 +2366,9 @@ mod tests {
 		assert!(!network_graph.read_only().nodes().is_empty());
 		assert!(!network_graph.read_only().channels().is_empty());
 		network_graph.write(&mut w).unwrap();
-		assert!(<NetworkGraph>::read(&mut io::Cursor::new(&w.0)).unwrap() == network_graph);
+
+		let logger = Arc::new(test_utils::TestLogger::new());


nit: here and the next few tests: no need for Arcs, not that it matters in tests here.

It's actually needed here because of the later assertion against the NetworkGraph<Arg<test_utils::TestLogger>> returned by create_network_graph. I guess using Arc was a convenience to avoid passing or returning a TestLogger to/from that function, though an Rc would have sufficed.

TheBlueMatt · 2022-06-07T09:59:54Z

lightning/src/routing/gossip.rs

-			},
+impl<L: Deref> EventHandler for NetworkGraph<L> where L::Target: Logger {
+	fn handle_event(&self, event: &Event) {
+		if let Event::PaymentPathFailed { payment_hash: _, rejected_by_dest: _, network_update, .. } = event {


lol not sure why it was there before but we don't need the field: _ crap if we also have a .. at the end.

TheBlueMatt · 2022-06-07T10:09:14Z

lightning-background-processor/src/lib.rs

-	/// If rapid gossip sync is meant to run at startup, pass an optional [`RapidGossipSync`]
-	/// to `rapid_gossip_sync` to indicate to [`BackgroundProcessor`] not to prune the
-	/// [`NetworkGraph`] instance until the [`RapidGossipSync`] instance completes its first sync.
+	/// If rapid gossip sync is meant to run at startup, pass a [`RapidGossipSync`] to `gossip_sync`


Maybe via gossip_sync instead of to gossip_sync?

jkczyz · 2022-06-14T13:52:40Z

Opened #1541 with follow-ups.

Move Secp256k1 context to NetworkGraph

0f73d6a

P2PGossipSync has a Secp256k1 context field, which it only uses to pass to NetworkGraph methods. Move the field to NetworkGraph so other callers don't need to pass in a Secp256k1 context.

wpaulino previously approved these changes Jun 3, 2022

View reviewed changes

TheBlueMatt reviewed Jun 3, 2022

View reviewed changes

lightning-background-processor/src/lib.rs Outdated Show resolved Hide resolved

TheBlueMatt added this to the 0.0.107 milestone Jun 4, 2022

jkczyz dismissed wpaulino’s stale review via 2e639f0 June 4, 2022 07:26

jkczyz force-pushed the 2022-06-gossip-sync-enum branch 4 times, most recently from 451c915 to 39af99d Compare June 4, 2022 17:03

arik-so reviewed Jun 5, 2022

View reviewed changes

jkczyz force-pushed the 2022-06-gossip-sync-enum branch from 39af99d to 9de98e3 Compare June 5, 2022 22:48

arik-so reviewed Jun 6, 2022

View reviewed changes

jkczyz added 3 commits June 6, 2022 13:02

Parameterize NetworkGraph with Logger

67736b7

P2PGossipSync logs before delegating to NetworkGraph in its EventHandler. In order to share this handling with RapidGossipSync, NetworkGraph needs to take a logger so that it can implement EventHandler instead.

Implement EventHandler for NetworkGraph

4ccf445

Instead of implementing EventHandler for P2PGossipSync, implement it on NetworkGraph. This allows RapidGossipSync to handle events, too, by delegating to its NetworkGraph.

jkczyz force-pushed the 2022-06-gossip-sync-enum branch from 498ae1b to c032e28 Compare June 6, 2022 18:04

jkczyz mentioned this pull request Jun 6, 2022

Cut 0.0.107 #1525

Merged

arik-so approved these changes Jun 6, 2022

View reviewed changes

TheBlueMatt approved these changes Jun 7, 2022

View reviewed changes

TheBlueMatt merged commit a551a62 into lightningdevkit:main Jun 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support only one `GossipSync` in `BackgroundProcessor` #1517

Support only one `GossipSync` in `BackgroundProcessor` #1517

jkczyz commented Jun 3, 2022

codecov-commenter commented Jun 3, 2022 •

edited

Loading

TheBlueMatt Jun 3, 2022

jkczyz Jun 4, 2022

jkczyz Jun 4, 2022

jkczyz Jun 4, 2022

TheBlueMatt Jun 4, 2022

jkczyz Jun 4, 2022

jkczyz Jun 4, 2022

jkczyz Jun 4, 2022

arik-so Jun 4, 2022

arik-so Jun 5, 2022

jkczyz Jun 5, 2022

arik-so Jun 5, 2022

jkczyz Jun 5, 2022

TheBlueMatt Jun 5, 2022

jkczyz Jun 6, 2022 •

edited

Loading

arik-so Jun 6, 2022

TheBlueMatt Jun 6, 2022

arik-so commented Jun 6, 2022

TheBlueMatt left a comment

TheBlueMatt Jun 6, 2022

TheBlueMatt Jun 6, 2022

jkczyz Jun 14, 2022

TheBlueMatt Jun 7, 2022

TheBlueMatt Jun 7, 2022

jkczyz commented Jun 14, 2022

Support only one GossipSync in BackgroundProcessor #1517

Support only one GossipSync in BackgroundProcessor #1517

Conversation

jkczyz commented Jun 3, 2022

codecov-commenter commented Jun 3, 2022 • edited Loading

Codecov Report

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

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

jkczyz Jun 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arik-so commented Jun 6, 2022

TheBlueMatt 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

jkczyz commented Jun 14, 2022

Support only one `GossipSync` in `BackgroundProcessor` #1517

Support only one `GossipSync` in `BackgroundProcessor` #1517

codecov-commenter commented Jun 3, 2022 •

edited

Loading

jkczyz Jun 6, 2022 •

edited

Loading