From e742894492c55802b241eebc585bbd28aa16481b Mon Sep 17 00:00:00 2001 From: bmancini55 Date: Thu, 3 Dec 2020 12:48:40 -0500 Subject: [PATCH] Change routing table sync to use gossip_queries This commit changes outbound routing table sync to use gossip_queries instead of the effectively deprecated initial_routing_sync feature. This change removes setting of initial_routing_sync in our outbound Init message. Instead we now call sync_routing_table after receiving an Init message from a peer. If the peer supports gossip_queries and should_request_full_sync returns true, we initiate a full gossip_queries sync. --- lightning/src/ln/features.rs | 5 +++ lightning/src/ln/msgs.rs | 14 ++++--- lightning/src/ln/peer_handler.rs | 58 ++------------------------ lightning/src/routing/network_graph.rs | 45 ++++++++++++++++---- 4 files changed, 55 insertions(+), 67 deletions(-) diff --git a/lightning/src/ln/features.rs b/lightning/src/ln/features.rs index 0c3c3601..3ce3d4fa 100644 --- a/lightning/src/ln/features.rs +++ b/lightning/src/ln/features.rs @@ -484,6 +484,7 @@ impl Features { pub(crate) fn supports_gossip_queries(&self) -> bool { ::supports_feature(&self.flags) } + #[cfg(test)] pub(crate) fn clear_gossip_queries(mut self) -> Self { ::clear_bits(&mut self.flags); self @@ -514,6 +515,10 @@ impl Features { pub(crate) fn initial_routing_sync(&self) -> bool { ::supports_feature(&self.flags) } + // We are no longer setting initial_routing_sync now that gossip_queries + // is enabled. This feature is ignored by a peer when gossip_queries has + // been negotiated. + #[cfg(test)] pub(crate) fn clear_initial_routing_sync(&mut self) { ::clear_bits(&mut self.flags) } diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs index 07363741..55d3acdf 100644 --- a/lightning/src/ln/msgs.rs +++ b/lightning/src/ln/msgs.rs @@ -804,6 +804,12 @@ pub trait ChannelMessageHandler : events::MessageSendEventsProvider + Send + Syn } /// A trait to describe an object which can receive routing messages. +/// +/// # Implementor DoS Warnings +/// +/// For `gossip_queries` messages there are potential DoS vectors when handling +/// inbound queries. Implementors using an on-disk network graph should be aware of +/// repeated disk I/O for queries accessing different parts of the network graph. pub trait RoutingMessageHandler : Send + Sync + events::MessageSendEventsProvider { /// Handle an incoming node_announcement message, returning true if it should be forwarded on, /// false or returning an Err otherwise. @@ -841,14 +847,10 @@ pub trait RoutingMessageHandler : Send + Sync + events::MessageSendEventsProvide /// gossip messages. fn handle_reply_short_channel_ids_end(&self, their_node_id: &PublicKey, msg: ReplyShortChannelIdsEnd) -> Result<(), LightningError>; /// Handles when a peer asks us to send a list of short_channel_ids - /// for the requested range of blocks. There are potential DoS vectors when - /// handling inbound queries. Handling requests with first_blocknum very far - /// away may trigger repeated disk I/O if the NetworkGraph is not fully in-memory. + /// for the requested range of blocks. fn handle_query_channel_range(&self, their_node_id: &PublicKey, msg: QueryChannelRange) -> Result<(), LightningError>; /// Handles when a peer asks us to send routing gossip messages for a - /// list of short_channel_ids. There are potential DoS vectors when handling - /// inbound queries. Handling requests with first_blocknum very far away may - /// trigger repeated disk I/O if the NetworkGraph is not fully in-memory. + /// list of short_channel_ids. fn handle_query_short_channel_ids(&self, their_node_id: &PublicKey, msg: QueryShortChannelIds) -> Result<(), LightningError>; } diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs index daa74ad5..890df356 100644 --- a/lightning/src/ln/peer_handler.rs +++ b/lightning/src/ln/peer_handler.rs @@ -584,11 +584,7 @@ impl PeerManager PeerManager(peer_a: &PeerManager, peer_b: &PeerManager) -> (FileDescriptor, FileDescriptor) { - let (mut fd_a, mut fd_b) = establish_connection(peer_a, peer_b); - assert_eq!(peer_b.read_event(&mut fd_b, &fd_a.outbound_data.lock().unwrap().split_off(0)).unwrap(), false); - assert_eq!(peer_a.read_event(&mut fd_a, &fd_b.outbound_data.lock().unwrap().split_off(0)).unwrap(), false); - (fd_a.clone(), fd_b.clone()) - } - #[test] fn test_disconnect_peer() { // Simple test which builds a network of PeerManager, connects and brings them to NoiseState::Finished and @@ -1404,41 +1391,4 @@ mod tests { assert_eq!(cfgs[1].routing_handler.chan_upds_recvd.load(Ordering::Acquire), 100); assert_eq!(cfgs[1].routing_handler.chan_anns_recvd.load(Ordering::Acquire), 50); } - - #[test] - fn limit_initial_routing_sync_requests() { - // Inbound peer 0 requests initial_routing_sync, but outbound peer 1 does not. - { - let cfgs = create_peermgr_cfgs(2); - cfgs[0].routing_handler.request_full_sync.store(true, Ordering::Release); - let peers = create_network(2, &cfgs); - let (fd_0_to_1, fd_1_to_0) = establish_connection_and_read_events(&peers[0], &peers[1]); - - let peer_0 = peers[0].peers.lock().unwrap(); - let peer_1 = peers[1].peers.lock().unwrap(); - - let peer_0_features = peer_1.peers.get(&fd_1_to_0).unwrap().their_features.as_ref(); - let peer_1_features = peer_0.peers.get(&fd_0_to_1).unwrap().their_features.as_ref(); - - assert!(peer_0_features.unwrap().initial_routing_sync()); - assert!(!peer_1_features.unwrap().initial_routing_sync()); - } - - // Outbound peer 1 requests initial_routing_sync, but inbound peer 0 does not. - { - let cfgs = create_peermgr_cfgs(2); - cfgs[1].routing_handler.request_full_sync.store(true, Ordering::Release); - let peers = create_network(2, &cfgs); - let (fd_0_to_1, fd_1_to_0) = establish_connection_and_read_events(&peers[0], &peers[1]); - - let peer_0 = peers[0].peers.lock().unwrap(); - let peer_1 = peers[1].peers.lock().unwrap(); - - let peer_0_features = peer_1.peers.get(&fd_1_to_0).unwrap().their_features.as_ref(); - let peer_1_features = peer_0.peers.get(&fd_0_to_1).unwrap().their_features.as_ref(); - - assert!(!peer_0_features.unwrap().initial_routing_sync()); - assert!(peer_1_features.unwrap().initial_routing_sync()); - } - } } diff --git a/lightning/src/routing/network_graph.rs b/lightning/src/routing/network_graph.rs index 3a7cd4eb..a16e6752 100644 --- a/lightning/src/routing/network_graph.rs +++ b/lightning/src/routing/network_graph.rs @@ -219,17 +219,26 @@ impl RoutingMessageHandler for N } /// Initiates a stateless sync of routing gossip information with a peer - /// by calling query_channel_range. The default strategy used by this - /// implementation is to sync for the full block range with several peers. + /// using gossip_queries. The default strategy used by this implementation + /// is to sync the full block range with several peers. + /// /// We should expect one or more reply_channel_range messages in response - /// to our query. Each reply will enqueue a query_scid message to request - /// gossip messages for each channel. The sync is considered complete when - /// the final reply_scids_end message is received, though we are not + /// to our query_channel_range. Each reply will enqueue a query_scid message + /// to request gossip messages for each channel. The sync is considered complete + /// when the final reply_scids_end message is received, though we are not /// tracking this directly. fn sync_routing_table(&self, their_node_id: &PublicKey, init_msg: &Init) { + + // We will only perform a sync with peers that support gossip_queries. if !init_msg.features.supports_gossip_queries() { return (); } + + // Check if we need to perform a full synchronization with this peer + if !self.should_request_full_sync(their_node_id) { + return (); + } + let first_blocknum = 0; let number_of_blocks = 0xffffffff; log_debug!(self.logger, "Sending query_channel_range peer={}, first_blocknum={}, number_of_blocks={}", log_pubkey!(their_node_id), first_blocknum, number_of_blocks); @@ -249,7 +258,10 @@ impl RoutingMessageHandler for N /// stateless, it does not validate the sequencing of replies for multi- /// reply ranges. It does not validate whether the reply(ies) cover the /// queried range. It also does not filter SCIDs to only those in the - /// original query range. + /// original query range. We also do not validate that the chain_hash + /// matches the chain_hash of the NetworkGraph. Any chan_ann message that + /// does not match our chain_hash will be rejected when the announcement is + /// processed. fn handle_reply_channel_range(&self, their_node_id: &PublicKey, msg: ReplyChannelRange) -> Result<(), LightningError> { log_debug!(self.logger, "Handling reply_channel_range peer={}, first_blocknum={}, number_of_blocks={}, full_information={}, scids={}", log_pubkey!(their_node_id), msg.first_blocknum, msg.number_of_blocks, msg.full_information, msg.short_channel_ids.len(),); @@ -1967,6 +1979,26 @@ mod tests { _ => panic!("Expected MessageSendEvent::SendChannelRangeQuery") }; } + + // It should not enqueue a query when should_request_full_sync return false. + // The initial implementation allows syncing with the first 5 peers after + // which should_request_full_sync will return false + { + let (secp_ctx, net_graph_msg_handler) = create_net_graph_msg_handler(); + let init_msg = Init { features: InitFeatures::known() }; + for n in 1..7 { + let node_privkey = &SecretKey::from_slice(&[n; 32]).unwrap(); + let node_id = PublicKey::from_secret_key(&secp_ctx, node_privkey); + net_graph_msg_handler.sync_routing_table(&node_id, &init_msg); + let events = net_graph_msg_handler.get_and_clear_pending_msg_events(); + if n <= 5 { + assert_eq!(events.len(), 1); + } else { + assert_eq!(events.len(), 0); + } + + } + } } #[test] @@ -1980,7 +2012,6 @@ mod tests { // Test receipt of a single reply that should enqueue an SCID query // matching the SCIDs in the reply { - // Handle a single successful reply that encompasses the queried channel range let result = net_graph_msg_handler.handle_reply_channel_range(&node_id_1, ReplyChannelRange { chain_hash, full_information: true, -- 2.30.2