From cf1aeb81e21fa731fd62a5dda343474a91483700 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 29 Jun 2022 17:41:30 +0000 Subject: [PATCH] Have `find_route` take a `NetworkGraph` instead of a `ReadOnly` one Because downstream languages are often garbage-collected, having the user directly allocate a `ReadOnlyNetworkGraph` and pass a reference to it to `find_route` often results in holding a read lock long in excess of the `find_route` call. Worse, some languages (like JavaScript) tend to only garbage collect when other code is not running, possibly leading to deadlocks. --- fuzz/src/full_stack.rs | 4 ++-- fuzz/src/router.rs | 2 +- lightning-invoice/src/utils.rs | 7 +++---- lightning/src/ln/channelmanager.rs | 14 ++++++-------- lightning/src/ln/functional_tests.rs | 6 +++--- lightning/src/routing/router.rs | 24 +++++++++++++----------- lightning/src/routing/scoring.rs | 2 +- 7 files changed, 29 insertions(+), 30 deletions(-) diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs index eb270f679..72f081206 100644 --- a/fuzz/src/full_stack.rs +++ b/fuzz/src/full_stack.rs @@ -460,7 +460,7 @@ pub fn do_test(data: &[u8], logger: &Arc) { final_cltv_expiry_delta: 42, }; let random_seed_bytes: [u8; 32] = keys_manager.get_secure_random_bytes(); - let route = match find_route(&our_id, ¶ms, &network_graph.read_only(), None, Arc::clone(&logger), &scorer, &random_seed_bytes) { + let route = match find_route(&our_id, ¶ms, &network_graph, None, Arc::clone(&logger), &scorer, &random_seed_bytes) { Ok(route) => route, Err(_) => return, }; @@ -484,7 +484,7 @@ pub fn do_test(data: &[u8], logger: &Arc) { final_cltv_expiry_delta: 42, }; let random_seed_bytes: [u8; 32] = keys_manager.get_secure_random_bytes(); - let mut route = match find_route(&our_id, ¶ms, &network_graph.read_only(), None, Arc::clone(&logger), &scorer, &random_seed_bytes) { + let mut route = match find_route(&our_id, ¶ms, &network_graph, None, Arc::clone(&logger), &scorer, &random_seed_bytes) { Ok(route) => route, Err(_) => return, }; diff --git a/fuzz/src/router.rs b/fuzz/src/router.rs index 21fb2dc15..29af00acc 100644 --- a/fuzz/src/router.rs +++ b/fuzz/src/router.rs @@ -267,7 +267,7 @@ pub fn do_test(data: &[u8], out: Out) { final_value_msat: slice_to_be64(get_slice!(8)), final_cltv_expiry_delta: slice_to_be32(get_slice!(4)), }; - let _ = find_route(&our_pubkey, &route_params, &net_graph.read_only(), + let _ = find_route(&our_pubkey, &route_params, &net_graph, first_hops.map(|c| c.iter().collect::>()).as_ref().map(|a| a.as_slice()), Arc::clone(&logger), &scorer, &random_seed_bytes); } diff --git a/lightning-invoice/src/utils.rs b/lightning-invoice/src/utils.rs index d42bc503f..e7cb2af7c 100644 --- a/lightning-invoice/src/utils.rs +++ b/lightning-invoice/src/utils.rs @@ -461,13 +461,12 @@ where L::Target: Logger { &self, payer: &PublicKey, params: &RouteParameters, _payment_hash: &PaymentHash, first_hops: Option<&[&ChannelDetails]>, scorer: &S ) -> Result { - let network_graph = self.network_graph.read_only(); let random_seed_bytes = { let mut locked_random_seed_bytes = self.random_seed_bytes.lock().unwrap(); *locked_random_seed_bytes = sha256::Hash::hash(&*locked_random_seed_bytes).into_inner(); *locked_random_seed_bytes }; - find_route(payer, params, &network_graph, first_hops, &*self.logger, scorer, &random_seed_bytes) + find_route(payer, params, &self.network_graph, first_hops, &*self.logger, scorer, &random_seed_bytes) } } @@ -572,7 +571,7 @@ mod test { let scorer = test_utils::TestScorer::with_penalty(0); let random_seed_bytes = chanmon_cfgs[1].keys_manager.get_secure_random_bytes(); let route = find_route( - &nodes[0].node.get_our_node_id(), &route_params, &network_graph.read_only(), + &nodes[0].node.get_our_node_id(), &route_params, &network_graph, Some(&first_hops.iter().collect::>()), &logger, &scorer, &random_seed_bytes ).unwrap(); @@ -848,7 +847,7 @@ mod test { let scorer = test_utils::TestScorer::with_penalty(0); let random_seed_bytes = chanmon_cfgs[1].keys_manager.get_secure_random_bytes(); let route = find_route( - &nodes[0].node.get_our_node_id(), ¶ms, &network_graph.read_only(), + &nodes[0].node.get_our_node_id(), ¶ms, &network_graph, Some(&first_hops.iter().collect::>()), &logger, &scorer, &random_seed_bytes ).unwrap(); let (payment_event, fwd_idx) = { diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 3fa136e9a..d3add5dba 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -7365,7 +7365,7 @@ mod tests { final_cltv_expiry_delta: TEST_FINAL_CLTV, }; let route = find_route( - &nodes[0].node.get_our_node_id(), &route_params, &nodes[0].network_graph.read_only(), + &nodes[0].node.get_our_node_id(), &route_params, &nodes[0].network_graph, None, nodes[0].logger, &scorer, &random_seed_bytes ).unwrap(); nodes[0].node.send_spontaneous_payment(&route, Some(payment_preimage)).unwrap(); @@ -7396,7 +7396,7 @@ mod tests { // To start (2), send a keysend payment but don't claim it. let payment_preimage = PaymentPreimage([42; 32]); let route = find_route( - &nodes[0].node.get_our_node_id(), &route_params, &nodes[0].network_graph.read_only(), + &nodes[0].node.get_our_node_id(), &route_params, &nodes[0].network_graph, None, nodes[0].logger, &scorer, &random_seed_bytes ).unwrap(); let (payment_hash, _) = nodes[0].node.send_spontaneous_payment(&route, Some(payment_preimage)).unwrap(); @@ -7460,9 +7460,8 @@ mod tests { let scorer = test_utils::TestScorer::with_penalty(0); let random_seed_bytes = chanmon_cfgs[1].keys_manager.get_secure_random_bytes(); let route = find_route( - &payer_pubkey, &route_params, &network_graph.read_only(), - Some(&first_hops.iter().collect::>()), nodes[0].logger, &scorer, - &random_seed_bytes + &payer_pubkey, &route_params, &network_graph, Some(&first_hops.iter().collect::>()), + nodes[0].logger, &scorer, &random_seed_bytes ).unwrap(); let test_preimage = PaymentPreimage([42; 32]); @@ -7505,9 +7504,8 @@ mod tests { let scorer = test_utils::TestScorer::with_penalty(0); let random_seed_bytes = chanmon_cfgs[1].keys_manager.get_secure_random_bytes(); let route = find_route( - &payer_pubkey, &route_params, &network_graph.read_only(), - Some(&first_hops.iter().collect::>()), nodes[0].logger, &scorer, - &random_seed_bytes + &payer_pubkey, &route_params, &network_graph, Some(&first_hops.iter().collect::>()), + nodes[0].logger, &scorer, &random_seed_bytes ).unwrap(); let test_preimage = PaymentPreimage([42; 32]); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 3298db0db..2c6655f02 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -9862,7 +9862,7 @@ fn test_keysend_payments_to_public_node() { }; let scorer = test_utils::TestScorer::with_penalty(0); let random_seed_bytes = chanmon_cfgs[1].keys_manager.get_secure_random_bytes(); - let route = find_route(&payer_pubkey, &route_params, &network_graph.read_only(), None, nodes[0].logger, &scorer, &random_seed_bytes).unwrap(); + let route = find_route(&payer_pubkey, &route_params, &network_graph, None, nodes[0].logger, &scorer, &random_seed_bytes).unwrap(); let test_preimage = PaymentPreimage([42; 32]); let (payment_hash, _) = nodes[0].node.send_spontaneous_payment(&route, Some(test_preimage)).unwrap(); @@ -9898,8 +9898,8 @@ fn test_keysend_payments_to_private_node() { let scorer = test_utils::TestScorer::with_penalty(0); let random_seed_bytes = chanmon_cfgs[1].keys_manager.get_secure_random_bytes(); let route = find_route( - &payer_pubkey, &route_params, &network_graph.read_only(), - Some(&first_hops.iter().collect::>()), nodes[0].logger, &scorer, &random_seed_bytes + &payer_pubkey, &route_params, &network_graph, Some(&first_hops.iter().collect::>()), + nodes[0].logger, &scorer, &random_seed_bytes ).unwrap(); let test_preimage = PaymentPreimage([42; 32]); diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 90e658604..cca2ff5f5 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -17,7 +17,7 @@ use bitcoin::secp256k1::PublicKey; use ln::channelmanager::ChannelDetails; use ln::features::{ChannelFeatures, InvoiceFeatures, NodeFeatures}; use ln::msgs::{DecodeError, ErrorAction, LightningError, MAX_VALUE_MSAT}; -use routing::gossip::{DirectedChannelInfoWithUpdate, EffectiveCapacity, ReadOnlyNetworkGraph, NodeId, RoutingFees}; +use routing::gossip::{DirectedChannelInfoWithUpdate, EffectiveCapacity, ReadOnlyNetworkGraph, NetworkGraph, NodeId, RoutingFees}; use routing::scoring::{ChannelUsage, Score}; use util::ser::{Writeable, Readable, Writer}; use util::logger::{Level, Logger}; @@ -671,16 +671,17 @@ fn default_node_features() -> NodeFeatures { /// [`ChannelManager::list_usable_channels`]: crate::ln::channelmanager::ChannelManager::list_usable_channels /// [`Event::PaymentPathFailed`]: crate::util::events::Event::PaymentPathFailed /// [`NetworkGraph`]: crate::routing::gossip::NetworkGraph -pub fn find_route( +pub fn find_route( our_node_pubkey: &PublicKey, route_params: &RouteParameters, - network_graph: &ReadOnlyNetworkGraph, first_hops: Option<&[&ChannelDetails]>, logger: L, + network_graph: &NetworkGraph, first_hops: Option<&[&ChannelDetails]>, logger: L, scorer: &S, random_seed_bytes: &[u8; 32] ) -> Result -where L::Target: Logger { - let mut route = get_route(our_node_pubkey, &route_params.payment_params, network_graph, first_hops, +where L::Target: Logger, GL::Target: Logger { + let graph_lock = network_graph.read_only(); + let mut route = get_route(our_node_pubkey, &route_params.payment_params, &graph_lock, first_hops, route_params.final_value_msat, route_params.final_cltv_expiry_delta, logger, scorer, random_seed_bytes)?; - add_random_cltv_offset(&mut route, &route_params.payment_params, network_graph, random_seed_bytes); + add_random_cltv_offset(&mut route, &route_params.payment_params, &graph_lock, random_seed_bytes); Ok(route) } @@ -1787,15 +1788,16 @@ fn add_random_cltv_offset(route: &mut Route, payment_params: &PaymentParameters, /// exclude the payer, but include the payee). This may be useful, e.g., for probing the chosen path. /// /// Re-uses logic from `find_route`, so the restrictions described there also apply here. -pub fn build_route_from_hops( +pub fn build_route_from_hops( our_node_pubkey: &PublicKey, hops: &[PublicKey], route_params: &RouteParameters, - network_graph: &ReadOnlyNetworkGraph, logger: L, random_seed_bytes: &[u8; 32] + network_graph: &NetworkGraph, logger: L, random_seed_bytes: &[u8; 32] ) -> Result -where L::Target: Logger { +where L::Target: Logger, GL::Target: Logger { + let graph_lock = network_graph.read_only(); let mut route = build_route_from_hops_internal( - our_node_pubkey, hops, &route_params.payment_params, &network_graph, + our_node_pubkey, hops, &route_params.payment_params, &graph_lock, route_params.final_value_msat, route_params.final_cltv_expiry_delta, logger, random_seed_bytes)?; - add_random_cltv_offset(&mut route, &route_params.payment_params, &network_graph, random_seed_bytes); + add_random_cltv_offset(&mut route, &route_params.payment_params, &graph_lock, random_seed_bytes); Ok(route) } diff --git a/lightning/src/routing/scoring.rs b/lightning/src/routing/scoring.rs index 72edbde9e..3ce8448e5 100644 --- a/lightning/src/routing/scoring.rs +++ b/lightning/src/routing/scoring.rs @@ -43,7 +43,7 @@ //! let scorer = ProbabilisticScorer::new(params, &network_graph, &logger); //! # let random_seed_bytes = [42u8; 32]; //! -//! let route = find_route(&payer, &route_params, &network_graph.read_only(), None, &logger, &scorer, &random_seed_bytes); +//! let route = find_route(&payer, &route_params, &network_graph, None, &logger, &scorer, &random_seed_bytes); //! # } //! ``` //! -- 2.39.5