Skip to content

Use cost / path amt limit as the pathfinding score, not cost #3890

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+58 −36
Open

Use cost / path amt limit as the pathfinding score, not cost #3890

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a437b77
Skip storing an explicit `node_id` in `RouteGraphNode`
TheBlueMatt Jun 25, 2025
a31da0a
Reduce `total_cltv_delta` size in `RouteGraphNode`
TheBlueMatt Jun 25, 2025
51414fb
Use `cost / path amt limit` as the pathfinding score, not `cost`
TheBlueMatt Jun 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 58 additions & 36 deletions lightning/src/routing/router.rs
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you run a benchmark to check what impact this first commit actually has on performance?

Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
Expand All @@ -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::<RouteGraphNode>();
const _GRAPH_NODE_FIXED_SIZE: usize = core::mem::size_of::<RouteGraphNode>() - 64;
// substantially when it is laid out at exactly 32 bytes.
const _GRAPH_NODE_32: () = assert!(core::mem::size_of::<RouteGraphNode>() == 32);

/// A [`CandidateRouteHop::FirstHop`] entry.
#[derive(Clone, Debug)]
Expand Down Expand Up @@ -2124,7 +2122,16 @@ impl<'a> PaymentPath<'a> {
return result;
}

fn get_cost_msat(&self) -> u64 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs on this method?

fn get_cost(&self) -> u128 {
let fee_cost = self.get_fee_cost_msat();
if fee_cost == u64::MAX {
u64::MAX.into()
} else {
((fee_cost as u128) << 64) / self.get_value_msat() as u128
}
}

fn get_fee_cost_msat(&self) -> u64 {
self.get_total_fee_paid_msat().saturating_add(self.get_path_penalty_msat())
}

Expand Down Expand Up @@ -2699,6 +2706,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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we already do this, can we adjust the field type in PaymentParameters, and maybe also final_cltv_expiry_delta? It's kind of odd to have a more precise type in the API, just to always shrink it to u16 in practice, no?


// 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
Expand Down Expand Up @@ -2788,15 +2805,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.
Expand Down Expand Up @@ -2989,25 +3000,32 @@ 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);
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
};
Comment on lines +3007 to +3016
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add some comments in this block explaining what's going on?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please add some comments explaining all the shifting and arbitrary casts. Also, please introduce well-named consts for these magic number where possible.


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,
};
Expand Down Expand Up @@ -3082,7 +3100,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;
Expand Down Expand Up @@ -3138,7 +3156,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 {
Expand Down Expand Up @@ -3265,7 +3283,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
);
}

Expand Down Expand Up @@ -3340,11 +3358,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()));

Expand Down Expand Up @@ -3468,13 +3486,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
);
Expand Down Expand Up @@ -3549,10 +3574,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().cmp(&a.get_cost()));

// We should make sure that at least 1 path left.
let mut paths_left = selected_route.len();
Expand All @@ -3578,7 +3600,7 @@ where L::Target: Logger {
selected_route.sort_unstable_by(|a, b| {
let a_f = a.hops.iter().map(|hop| hop.0.candidate.fees().proportional_millionths as u64).sum::<u64>();
let b_f = b.hops.iter().map(|hop| hop.0.candidate.fees().proportional_millionths as u64).sum::<u64>();
a_f.cmp(&b_f).then_with(|| b.get_cost_msat().cmp(&a.get_cost_msat()))
a_f.cmp(&b_f).then_with(|| b.get_fee_cost_msat().cmp(&a.get_fee_cost_msat()))
});
let expensive_payment_path = selected_route.first_mut().unwrap();

Expand Down
Loading