diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index fb1e518549f..ded5e592fde 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -1393,18 +1393,17 @@ impl_writeable_tlv_based!(RouteHintHop, { }); #[derive(Eq, PartialEq)] -#[repr(align(64))] // Force the size to 64 bytes +#[repr(align(32))] // Force the size to 32 bytes struct RouteGraphNode { - node_id: NodeId, node_counter: u32, - score: u64, + score: u128, // The maximum value a yet-to-be-constructed payment path might flow through this node. // This value is upper-bounded by us by: // - how much is needed for a path being constructed // - how much value can channels following this node (up to the destination) can contribute, // considering their capacity and fees value_contribution_msat: u64, - total_cltv_delta: u32, + total_cltv_delta: u16, /// The number of hops walked up to this node. path_length_to_node: u8, } @@ -1426,9 +1425,8 @@ impl cmp::PartialOrd for RouteGraphNode { } // While RouteGraphNode can be laid out with fewer bytes, performance appears to be improved -// substantially when it is laid out at exactly 64 bytes. -const _GRAPH_NODE_SMALL: usize = 64 - core::mem::size_of::(); -const _GRAPH_NODE_FIXED_SIZE: usize = core::mem::size_of::() - 64; +// substantially when it is laid out at exactly 32 bytes. +const _GRAPH_NODE_32: () = assert!(core::mem::size_of::() == 32); /// A [`CandidateRouteHop::FirstHop`] entry. #[derive(Clone, Debug)] @@ -2124,6 +2122,18 @@ impl<'a> PaymentPath<'a> { return result; } + /// Gets the cost (fees plus scorer penalty in msats) of the path divided by the value we + /// can/will send over the path. This is also the heap score during our Dijkstra's walk. + fn get_cost_per_msat(&self) -> u128 { + let fee_cost = self.get_cost_msat(); + if fee_cost == u64::MAX { + u64::MAX.into() + } else { + ((fee_cost as u128) << 64) / self.get_value_msat() as u128 + } + } + + /// Gets the fees plus scorer penalty in msats of the path. fn get_cost_msat(&self) -> u64 { self.get_total_fee_paid_msat().saturating_add(self.get_path_penalty_msat()) } @@ -2699,6 +2709,16 @@ where L::Target: Logger { // drop the requirement by setting this to 0. let mut channel_saturation_pow_half = payment_params.max_channel_saturation_power_of_half; + // In order to already account for some of the privacy enhancing random CLTV + // expiry delta offset we add on top later, we subtract a rough estimate + // (2*MEDIAN_HOP_CLTV_EXPIRY_DELTA) here. + let max_total_cltv_expiry_delta: u16 = + (payment_params.max_total_cltv_expiry_delta - final_cltv_expiry_delta) + .checked_sub(2*MEDIAN_HOP_CLTV_EXPIRY_DELTA) + .unwrap_or(payment_params.max_total_cltv_expiry_delta - final_cltv_expiry_delta) + .try_into() + .unwrap_or(u16::MAX); + // Keep track of how much liquidity has been used in selected channels or blinded paths. Used to // determine if the channel can be used by additional MPP paths or to inform path finding // decisions. It is aware of direction *only* to ensure that the correct htlc_maximum_msat value @@ -2788,15 +2808,9 @@ where L::Target: Logger { let exceeds_max_path_length = path_length_to_node > max_path_length; // Do not consider candidates that exceed the maximum total cltv expiry limit. - // In order to already account for some of the privacy enhancing random CLTV - // expiry delta offset we add on top later, we subtract a rough estimate - // (2*MEDIAN_HOP_CLTV_EXPIRY_DELTA) here. - let max_total_cltv_expiry_delta = (payment_params.max_total_cltv_expiry_delta - final_cltv_expiry_delta) - .checked_sub(2*MEDIAN_HOP_CLTV_EXPIRY_DELTA) - .unwrap_or(payment_params.max_total_cltv_expiry_delta - final_cltv_expiry_delta); let hop_total_cltv_delta = ($next_hops_cltv_delta as u32) .saturating_add(cltv_expiry_delta); - let exceeds_cltv_delta_limit = hop_total_cltv_delta > max_total_cltv_expiry_delta; + let exceeds_cltv_delta_limit = hop_total_cltv_delta > max_total_cltv_expiry_delta as u32; let value_contribution_msat = cmp::min(available_value_contribution_msat, $next_hops_value_contribution); // Includes paying fees for the use of the following channels. @@ -2989,25 +3003,39 @@ where L::Target: Logger { // but it may require additional tracking - we don't want to double-count // the fees included in $next_hops_path_htlc_minimum_msat, but also // can't use something that may decrease on future hops. - let old_cost = cmp::max(old_entry.total_fee_msat, old_entry.path_htlc_minimum_msat) + let old_fee_cost = cmp::max(old_entry.total_fee_msat, old_entry.path_htlc_minimum_msat) .saturating_add(old_entry.path_penalty_msat); - let new_cost = cmp::max(total_fee_msat, path_htlc_minimum_msat) + let new_fee_cost = cmp::max(total_fee_msat, path_htlc_minimum_msat) .saturating_add(path_penalty_msat); - let should_replace = - new_cost < old_cost - || (new_cost == old_cost && old_entry.value_contribution_msat < value_contribution_msat); + // The actual score we use for our heap is the cost divided by how + // much we are thinking of sending over this channel. This avoids + // prioritizing channels that have a very low fee because we aren't + // sending very much over them. + // In order to avoid integer division precision loss, we simply + // shift the costs up to the top half of a u128 and divide by the + // value (which is, at max, just under a u64). + let old_cost = if old_fee_cost != u64::MAX && old_entry.value_contribution_msat != 0 { + ((old_fee_cost as u128) << 64) / old_entry.value_contribution_msat as u128 + } else { + u128::MAX + }; + let new_cost = if new_fee_cost != u64::MAX { + ((new_fee_cost as u128) << 64) / value_contribution_msat as u128 + } else { + u128::MAX + }; - if !old_entry.was_processed && should_replace { + if !old_entry.was_processed && new_cost < old_cost { #[cfg(all(not(ldk_bench), any(test, fuzzing)))] { assert!(!old_entry.best_path_from_hop_selected); + assert!(hop_total_cltv_delta <= u16::MAX as u32); } let new_graph_node = RouteGraphNode { - node_id: src_node_id, node_counter: src_node_counter, - score: cmp::max(total_fee_msat, path_htlc_minimum_msat).saturating_add(path_penalty_msat), - total_cltv_delta: hop_total_cltv_delta, + score: new_cost, + total_cltv_delta: hop_total_cltv_delta as u16, value_contribution_msat, path_length_to_node, }; @@ -3082,7 +3110,7 @@ where L::Target: Logger { // This data can later be helpful to optimize routing (pay lower fees). #[rustfmt::skip] macro_rules! add_entries_to_cheapest_to_target_node { - ( $node: expr, $node_counter: expr, $node_id: expr, $next_hops_value_contribution: expr, + ( $node_counter: expr, $node_id: expr, $next_hops_value_contribution: expr, $next_hops_cltv_delta: expr, $next_hops_path_length: expr ) => { let fee_to_target_msat; let next_hops_path_htlc_minimum_msat; @@ -3138,7 +3166,7 @@ where L::Target: Logger { } } - if let Some(node) = $node { + if let Some(node) = network_nodes.get(&$node_id) { let features = if let Some(node_info) = node.announcement_info.as_ref() { &node_info.features() } else { @@ -3265,7 +3293,7 @@ where L::Target: Logger { entry.value_contribution_msat = path_value_msat; } add_entries_to_cheapest_to_target_node!( - network_nodes.get(&payee), payee_node_counter, payee, path_value_msat, 0, 0 + payee_node_counter, payee, path_value_msat, 0, 0 ); } @@ -3340,11 +3368,11 @@ where L::Target: Logger { // Both these cases (and other cases except reaching recommended_value_msat) mean that // paths_collection will be stopped because found_new_path==false. // This is not necessarily a routing failure. - 'path_construction: while let Some(RouteGraphNode { node_id, node_counter, total_cltv_delta, mut value_contribution_msat, path_length_to_node, .. }) = targets.pop() { + 'path_construction: while let Some(RouteGraphNode { node_counter, total_cltv_delta, mut value_contribution_msat, path_length_to_node, .. }) = targets.pop() { // Since we're going payee-to-payer, hitting our node as a target means we should stop // traversing the graph and arrange the path out of what we found. - if node_id == our_node_id { + if node_counter == payer_node_counter { let mut new_entry = dist[payer_node_counter as usize].take().unwrap(); let mut ordered_hops: Vec<(PathBuildingHop, NodeFeatures)> = vec!((new_entry.clone(), default_node_features.clone())); @@ -3468,13 +3496,20 @@ where L::Target: Logger { // If we found a path back to the payee, we shouldn't try to process it again. This is // the equivalent of the `elem.was_processed` check in // add_entries_to_cheapest_to_target_node!() (see comment there for more info). - if node_id == maybe_dummy_payee_node_id { continue 'path_construction; } + if node_counter == payee_node_counter { continue 'path_construction; } + + let node_id = if let Some(entry) = &dist[node_counter as usize] { + entry.candidate.source() + } else { + debug_assert!(false, "Best nodes in the heap should have entries in dist"); + continue 'path_construction; + }; // Otherwise, since the current target node is not us, // keep "unrolling" the payment graph from payee to payer by // finding a way to reach the current target from the payer side. add_entries_to_cheapest_to_target_node!( - network_nodes.get(&node_id), node_counter, node_id, + node_counter, node_id, value_contribution_msat, total_cltv_delta, path_length_to_node ); @@ -3549,10 +3584,7 @@ where L::Target: Logger { // First, sort by the cost-per-value of the path, dropping the paths that cost the most for // the value they contribute towards the payment amount. // We sort in descending order as we will remove from the front in `retain`, next. - selected_route.sort_unstable_by(|a, b| - (((b.get_cost_msat() as u128) << 64) / (b.get_value_msat() as u128)) - .cmp(&(((a.get_cost_msat() as u128) << 64) / (a.get_value_msat() as u128))) - ); + selected_route.sort_unstable_by(|a, b| b.get_cost_per_msat().cmp(&a.get_cost_per_msat())); // We should make sure that at least 1 path left. let mut paths_left = selected_route.len();