[router] Do not fail if we are exactly (or 3x) limited by a maximum

The new MPP routing algorithm attempts to build paths with a higher
value than our payment to find paths which may allow us to pay a
fee to meet an htlc_minimum limit. This is great if we're
min-bounded, however it results in us rejecting paths where we are
bounded by a maximum near the end of the path, with fees, and then
bounded by a minimum earlier in the path. Further, it means we will
not find the cheapest path where paths have a lower relative fee
but a higher absolute fee.

Instead, we calculate routes using the actual amount we wish to
send. To maintain the previous behavior of searching for cheaper
paths where we can "pay the difference" to meet an htlc_minimum, we
detect if we were minimum-bounded during graph walking and, if we
are, we walk the graph again with a higher value.

Author: TheBlueMatt

lightning/src/routing/router.rs

@@ -425,13 +425,18 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
 	let mut targets = BinaryHeap::new(); //TODO: Do we care about switching to eg Fibbonaci heap?
 	let mut dist = HashMap::with_capacity(network.get_nodes().len());
 
+	// During routing, if we ignore a path due to an htlc_minimum_msat limit, we set this,
+	// indicating that we may wish to try again with a higher value, potentially paying to meet an
+	// htlc_minimum with extra fees while still finding a cheaper path.
+	let mut hit_minimum_limit;
+
 	// When arranging a route, we select multiple paths so that we can make a multi-path payment.
-	// Don't stop searching for paths when we think they're
-	// sufficient to transfer a given value aggregately.
-	// Search for higher value, so that we collect many more paths,
-	// and then select the best combination among them.
+	// We start with a path_value of the exact amount we want, and if that generates a route we may
+	// return it immediately. Otherwise, we don't stop searching for paths until we have 3x the
+	// amount we want in total across paths, selecting the best subset at the end.
 	const ROUTE_CAPACITY_PROVISION_FACTOR: u64 = 3;
 	let recommended_value_msat = final_value_msat * ROUTE_CAPACITY_PROVISION_FACTOR as u64;
+	let mut path_value_msat = final_value_msat;
 
 	// Allow MPP only if we have a features set from somewhere that indicates the payee supports
 	// it. If the payee supports it they're supposed to include it in the invoice, so that should
@@ -555,8 +560,9 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
 					// Since we're choosing amount_to_transfer_over_msat as maximum possible, it can
 					// be only reduced later (not increased), so this channel should just be skipped
 					// as not sufficient.
-					// TODO: Explore simply adding fee to hit htlc_minimum_msat
-					if contributes_sufficient_value && amount_to_transfer_over_msat >= $directional_info.htlc_minimum_msat {
+					if amount_to_transfer_over_msat < $directional_info.htlc_minimum_msat {
+						hit_minimum_limit = true;
+					} else if contributes_sufficient_value {
 						// Note that low contribution here (limited by available_liquidity_msat)
 						// might violate htlc_minimum_msat on the hops which are next along the
 						// payment path (upstream to the payee). To avoid that, we recompute path
@@ -743,12 +749,13 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
 		// the further iterations of path finding. Also don't erase first_hop_targets.
 		targets.clear();
 		dist.clear();
+		hit_minimum_limit = false;
 
 		// If first hop is a private channel and the only way to reach the payee, this is the only
 		// place where it could be added.
 		if first_hops.is_some() {
 			if let Some(&(ref first_hop, ref features, ref outbound_capacity_msat)) = first_hop_targets.get(&payee) {
-				add_entry!(first_hop, *our_node_id, payee, dummy_directional_info, Some(outbound_capacity_msat / 1000), features.to_context(), 0, recommended_value_msat);
+				add_entry!(first_hop, *our_node_id, payee, dummy_directional_info, Some(outbound_capacity_msat / 1000), features.to_context(), 0, path_value_msat);
 			}
 		}
 
@@ -761,7 +768,7 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
 			// If not, targets.pop() will not even let us enter the loop in step 2.
 			None => {},
 			Some(node) => {
-				add_entries_to_cheapest_to_target_node!(node, payee, 0, recommended_value_msat);
+				add_entries_to_cheapest_to_target_node!(node, payee, 0, path_value_msat);
 			},
 		}
 
@@ -780,7 +787,7 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
 					// bit lazy here. In the future, we should pull them out via our
 					// ChannelManager, but there's no reason to waste the space until we
 					// need them.
-					add_entry!(first_hop, *our_node_id , hop.src_node_id, dummy_directional_info, Some(outbound_capacity_msat / 1000), features.to_context(), 0, recommended_value_msat);
+					add_entry!(first_hop, *our_node_id , hop.src_node_id, dummy_directional_info, Some(outbound_capacity_msat / 1000), features.to_context(), 0, path_value_msat);
 					true
 				} else {
 					// In any other case, only add the hop if the source is in the regular network
@@ -800,7 +807,7 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
 					htlc_maximum_msat: hop.htlc_maximum_msat,
 					fees: hop.fees,
 				};
-				add_entry!(hop.short_channel_id, hop.src_node_id, payee, directional_info, None::<u64>, ChannelFeatures::empty(), 0, recommended_value_msat);
+				add_entry!(hop.short_channel_id, hop.src_node_id, payee, directional_info, None::<u64>, ChannelFeatures::empty(), 0, path_value_msat);
 			}
 		}
 
@@ -925,13 +932,23 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
 		}
 
 		// Step (3).
-		// Stop either when recommended value is reached,
-		// or if during last iteration no new path was found.
-		// In the latter case, making another path finding attempt could not help,
-		// because we deterministically terminate the search due to low liquidity.
+		// Stop either when the recommended value is reached or if no new path was found in this
+		// iteration.
+		// In the latter case, making another path finding attempt won't help,
+		// because we deterministically terminated the search due to low liquidity.
 		if already_collected_value_msat >= recommended_value_msat || !found_new_path {
 			break 'paths_collection;
 		}
+		// Further, if this was our first walk of the graph, and we weren't limited by an
+		// htlc_minim_msat, return immediately because this path should suffice. If we were limited
+		// by an htlc_minimum_msat value, find another path with a higher value, potentially
+		// allowing us to pay fees to meet the htlc_minimum while still keeping a lower total fee.
+		if already_collected_value_msat == final_value_msat && payment_paths.len() == 1 {
+			if !hit_minimum_limit {
+				break 'paths_collection;
+			}
+			path_value_msat = recommended_value_msat;
+		}
 	}
 
 	// Step (4).
@@ -3434,6 +3451,78 @@ mod tests {
 			assert_eq!(total_amount_paid_msat, 90_000);
 		}
 	}
+
+	#[test]
+	fn exact_fee_liquidity_limit() {
+		// Test that if, while walking the graph, we find a hop that has exactly enough liquidity
+		// for us, including later hop fees, we take it. In the first version of our MPP algorithm
+		// we calculated fees on a higher value, resulting in us ignoring such paths.
+		let (secp_ctx, net_graph_msg_handler, _, logger) = build_graph();
+		let (our_privkey, our_id, privkeys, nodes) = get_nodes(&secp_ctx);
+
+		// We modify the graph to set the htlc_maximum of channel 2 to below the value we wish to
+		// send.
+		update_channel(&net_graph_msg_handler, &secp_ctx, &our_privkey, UnsignedChannelUpdate {
+			chain_hash: genesis_block(Network::Testnet).header.block_hash(),
+			short_channel_id: 2,
+			timestamp: 2,
+			flags: 0,
+			cltv_expiry_delta: 0,
+			htlc_minimum_msat: 0,
+			htlc_maximum_msat: OptionalField::Present(85_000),
+			fee_base_msat: 0,
+			fee_proportional_millionths: 0,
+			excess_data: Vec::new()
+		});
+
+		update_channel(&net_graph_msg_handler, &secp_ctx, &our_privkey, UnsignedChannelUpdate {
+			chain_hash: genesis_block(Network::Testnet).header.block_hash(),
+			short_channel_id: 12,
+			timestamp: 2,
+			flags: 0,
+			cltv_expiry_delta: (4 << 8) | 1,
+			htlc_minimum_msat: 0,
+			htlc_maximum_msat: OptionalField::Present(270_000),
+			fee_base_msat: 0,
+			fee_proportional_millionths: 1000000,
+			excess_data: Vec::new()
+		});
+
+		update_channel(&net_graph_msg_handler, &secp_ctx, &privkeys[2], UnsignedChannelUpdate {
+			chain_hash: genesis_block(Network::Testnet).header.block_hash(),
+			short_channel_id: 13,
+			timestamp: 2,
+			flags: 1 | 2,
+			cltv_expiry_delta: (13 << 8) | 2,
+			htlc_minimum_msat: 0,
+			htlc_maximum_msat: OptionalField::Absent,
+			fee_base_msat: 0,
+			fee_proportional_millionths: 0,
+			excess_data: Vec::new()
+		});
+
+		{
+			// Now, attempt to route 125 sats (just a bit below the capacity of 3 channels).
+			// Our algorithm should provide us with these 3 paths.
+			let route = get_route(&our_id, &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[2], None, None, &Vec::new(), 90_000, 42, Arc::clone(&logger)).unwrap();
+			assert_eq!(route.paths.len(), 1);
+			assert_eq!(route.paths[0].len(), 2);
+
+			assert_eq!(route.paths[0][0].pubkey, nodes[7]);
+			assert_eq!(route.paths[0][0].short_channel_id, 12);
+			assert_eq!(route.paths[0][0].fee_msat, 90_000*2);
+			assert_eq!(route.paths[0][0].cltv_expiry_delta, (13 << 8) | 1);
+			assert_eq!(route.paths[0][0].node_features.le_flags(), &id_to_feature_flags(8));
+			assert_eq!(route.paths[0][0].channel_features.le_flags(), &id_to_feature_flags(12));
+
+			assert_eq!(route.paths[0][1].pubkey, nodes[2]);
+			assert_eq!(route.paths[0][1].short_channel_id, 13);
+			assert_eq!(route.paths[0][1].fee_msat, 90_000);
+			assert_eq!(route.paths[0][1].cltv_expiry_delta, 42);
+			assert_eq!(route.paths[0][1].node_features.le_flags(), &id_to_feature_flags(3));
+			assert_eq!(route.paths[0][1].channel_features.le_flags(), &id_to_feature_flags(13));
+		}
+	}
 }
 
 #[cfg(all(test, feature = "unstable"))]