Account for used liquidity in first hops when processing route hints

.. in get_route.

Author: valentinewallace

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 {
 						.saturating_add(1);
 
 					// Searching for a direct channel between last checked hop and first_hop_targets
-					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::<Vec<_>>()), 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);
 	}