diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 6c01c580305..e6a1825758c 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -1442,27 +1442,38 @@ where L::Target: Logger { // when we want to stop looking for new paths. let mut already_collected_value_msat = 0; + macro_rules! sort_first_hop_channels { + ($channels: expr) => { + // Sort the first_hops channels to the same node(s) in priority order of which channel we'd + // most like to use. + // + // First, if channels are below `recommended_value_msat`, sort them in descending order, + // preferring larger channels to avoid splitting the payment into more MPP parts than is + // required. + // + // Second, because simply always sorting in descending order would always use our largest + // available outbound capacity, needlessly fragmenting our available channel capacities, + // sort channels above `recommended_value_msat` in ascending order, preferring channels + // which have enough, but not too much, capacity for the payment. + $channels.sort_unstable_by(|chan_a, chan_b| { + let chan_a_outbound_limit_msat = chan_a.next_outbound_htlc_limit_msat + .saturating_sub(*used_channel_liquidities.get(&(chan_a.get_outbound_payment_scid().unwrap(), + our_node_pubkey < &chan_a.counterparty.node_id)).unwrap_or(&0)); + let chan_b_outbound_limit_msat = chan_b.next_outbound_htlc_limit_msat + .saturating_sub(*used_channel_liquidities.get(&(chan_b.get_outbound_payment_scid().unwrap(), + our_node_pubkey < &chan_b.counterparty.node_id)).unwrap_or(&0)); + if chan_b_outbound_limit_msat < recommended_value_msat || chan_a_outbound_limit_msat < recommended_value_msat { + // Sort in descending order + chan_b_outbound_limit_msat.cmp(&chan_a_outbound_limit_msat) + } else { + // Sort in ascending order + chan_a_outbound_limit_msat.cmp(&chan_b_outbound_limit_msat) + } + }); + } + } for (_, channels) in first_hop_targets.iter_mut() { - // Sort the first_hops channels to the same node(s) in priority order of which channel we'd - // most like to use. - // - // First, if channels are below `recommended_value_msat`, sort them in descending order, - // preferring larger channels to avoid splitting the payment into more MPP parts than is - // required. - // - // Second, because simply always sorting in descending order would always use our largest - // available outbound capacity, needlessly fragmenting our available channel capacities, - // sort channels above `recommended_value_msat` in ascending order, preferring channels - // which have enough, but not too much, capacity for the payment. - channels.sort_unstable_by(|chan_a, chan_b| { - if chan_b.next_outbound_htlc_limit_msat < recommended_value_msat || chan_a.next_outbound_htlc_limit_msat < recommended_value_msat { - // Sort in descending order - chan_b.next_outbound_htlc_limit_msat.cmp(&chan_a.next_outbound_htlc_limit_msat) - } else { - // Sort in ascending order - chan_a.next_outbound_htlc_limit_msat.cmp(&chan_b.next_outbound_htlc_limit_msat) - } - }); + sort_first_hop_channels!(channels); } log_trace!(logger, "Building path from {} to payer {} for value {} msat.", @@ -1871,7 +1882,8 @@ where L::Target: Logger { // Searching for a direct channel between last checked hop and first_hop_targets let hint_candidate_contribution_msat = cmp::min(path_value_msat, candidate.effective_capacity().as_msat().saturating_sub(used_liquidity_msat)); - if let Some(first_channels) = first_hop_targets.get(&NodeId::from_pubkey(&prev_hop_id)) { + if let Some(first_channels) = first_hop_targets.get_mut(&NodeId::from_pubkey(&prev_hop_id)) { + sort_first_hop_channels!(first_channels); for details in first_channels { let first_hop_candidate = CandidateRouteHop::FirstHop { details }; add_entry!(first_hop_candidate, our_node_id, NodeId::from_pubkey(&prev_hop_id), @@ -1910,7 +1922,8 @@ where L::Target: Logger { // Note that we *must* check if the last hop was added as `add_entry` // always assumes that the third argument is a node to which we have a // path. - if let Some(first_channels) = first_hop_targets.get(&NodeId::from_pubkey(&hop.src_node_id)) { + if let Some(first_channels) = first_hop_targets.get_mut(&NodeId::from_pubkey(&hop.src_node_id)) { + sort_first_hop_channels!(first_channels); for details in first_channels { let first_hop_candidate = CandidateRouteHop::FirstHop { details }; add_entry!(first_hop_candidate, our_node_id, @@ -6018,12 +6031,9 @@ mod tests { let route = get_route(&our_node_id, &payment_params, &network_graph.read_only(), Some(&first_hops.iter().collect::>()), amt_msat, Arc::clone(&logger), &scorer, &(), &random_seed_bytes).unwrap(); - // TODO: `get_route` returns a suboptimal route here because first hop channels are not - // resorted on the fly when processing route hints. - assert_eq!(route.paths.len(), 3); + assert_eq!(route.paths.len(), 2); assert!(route.paths[0].hops.last().unwrap().fee_msat <= max_htlc_msat); assert!(route.paths[1].hops.last().unwrap().fee_msat <= max_htlc_msat); - assert!(route.paths[2].hops.last().unwrap().fee_msat <= max_htlc_msat); assert_eq!(route.get_total_amount(), amt_msat); }