Merge pull request #1610 from TheBlueMatt/2022-07-no-rand-path-selection

Author: TheBlueMatt

lightning/src/ln/functional_tests.rs

@@ -1059,26 +1059,6 @@ fn fake_network_test() {
 	fail_payment(&nodes[1], &vec!(&nodes[3], &nodes[2], &nodes[1])[..], payment_hash_2);
 	claim_payment(&nodes[1], &vec!(&nodes[2], &nodes[3], &nodes[1])[..], payment_preimage_1);
 
-	// Add a duplicate new channel from 2 to 4
-	let chan_5 = create_announced_chan_between_nodes(&nodes, 1, 3, InitFeatures::known(), InitFeatures::known());
-
-	// Send some payments across both channels
-	let payment_preimage_3 = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[3])[..], 3000000).0;
-	let payment_preimage_4 = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[3])[..], 3000000).0;
-	let payment_preimage_5 = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[3])[..], 3000000).0;
-
-
-	route_over_limit(&nodes[0], &vec!(&nodes[1], &nodes[3])[..], 3000000);
-	let events = nodes[0].node.get_and_clear_pending_msg_events();
-	assert_eq!(events.len(), 0);
-	nodes[0].logger.assert_log_regex("lightning::ln::channelmanager".to_string(), regex::Regex::new(r"Cannot send value that would put us over the max HTLC value in flight our peer will accept \(\d+\)").unwrap(), 1);
-
-	//TODO: Test that routes work again here as we've been notified that the channel is full
-
-	claim_payment(&nodes[0], &vec!(&nodes[1], &nodes[3])[..], payment_preimage_3);
-	claim_payment(&nodes[0], &vec!(&nodes[1], &nodes[3])[..], payment_preimage_4);
-	claim_payment(&nodes[0], &vec!(&nodes[1], &nodes[3])[..], payment_preimage_5);
-
 	// Close down the channels...
 	close_channel(&nodes[0], &nodes[1], &chan_1.2, chan_1.3, true);
 	check_closed_event!(nodes[0], 1, ClosureReason::CooperativeClosure);
@@ -1092,9 +1072,6 @@ fn fake_network_test() {
 	close_channel(&nodes[1], &nodes[3], &chan_4.2, chan_4.3, false);
 	check_closed_event!(nodes[1], 1, ClosureReason::CooperativeClosure);
 	check_closed_event!(nodes[3], 1, ClosureReason::CooperativeClosure);
-	close_channel(&nodes[1], &nodes[3], &chan_5.2, chan_5.3, false);
-	check_closed_event!(nodes[1], 1, ClosureReason::CooperativeClosure);
-	check_closed_event!(nodes[3], 1, ClosureReason::CooperativeClosure);
 }
 
 #[test]

lightning/src/routing/router.rs

@@ -238,7 +238,7 @@ pub struct PaymentParameters {
 	/// A value of 0 will allow payments up to and including a channel's total announced usable
 	/// capacity, a value of one will only use up to half its capacity, two 1/4, etc.
 	///
-	/// Default value: 1
+	/// Default value: 2
 	pub max_channel_saturation_power_of_half: u8,
 
 	/// A list of SCIDs which this payment was previously attempted over and which caused the
@@ -253,7 +253,7 @@ impl_writeable_tlv_based!(PaymentParameters, {
 	(2, features, option),
 	(3, max_path_count, (default_value, DEFAULT_MAX_PATH_COUNT)),
 	(4, route_hints, vec_type),
-	(5, max_channel_saturation_power_of_half, (default_value, 1)),
+	(5, max_channel_saturation_power_of_half, (default_value, 2)),
 	(6, expiry_time, option),
 	(7, previously_failed_channels, vec_type),
 });
@@ -268,7 +268,7 @@ impl PaymentParameters {
 			expiry_time: None,
 			max_total_cltv_expiry_delta: DEFAULT_MAX_TOTAL_CLTV_EXPIRY_DELTA,
 			max_path_count: DEFAULT_MAX_PATH_COUNT,
-			max_channel_saturation_power_of_half: 1,
+			max_channel_saturation_power_of_half: 2,
 			previously_failed_channels: Vec::new(),
 		}
 	}
@@ -754,7 +754,7 @@ where L::Target: Logger, GL::Target: Logger {
 pub(crate) fn get_route<L: Deref, S: Score>(
 	our_node_pubkey: &PublicKey, payment_params: &PaymentParameters, network_graph: &ReadOnlyNetworkGraph,
 	first_hops: Option<&[&ChannelDetails]>, final_value_msat: u64, final_cltv_expiry_delta: u32,
-	logger: L, scorer: &S, random_seed_bytes: &[u8; 32]
+	logger: L, scorer: &S, _random_seed_bytes: &[u8; 32]
 ) -> Result<Route, LightningError>
 where L::Target: Logger {
 	let payee_node_id = NodeId::from_pubkey(&payment_params.payee_pubkey);
@@ -796,11 +796,11 @@ where L::Target: Logger {
 	// 4. See if we managed to collect paths which aggregately are able to transfer target value
 	//    (not recommended value).
 	// 5. If yes, proceed. If not, fail routing.
-	// 6. Randomly combine paths into routes having enough to fulfill the payment. (TODO: knapsack)
-	// 7. Of all the found paths, select only those with the lowest total fee.
-	// 8. The last path in every selected route is likely to be more than we need.
-	//    Reduce its value-to-transfer and recompute fees.
-	// 9. Choose the best route by the lowest total fee.
+	// 6. Select the paths which have the lowest cost (fee plus scorer penalty) per amount
+	//    transferred up to the transfer target value.
+	// 7. Reduce the value of the last path until we are sending only the target value.
+	// 8. If our maximum channel saturation limit caused us to pick two identical paths, combine
+	//    them so that we're not sending two HTLCs along the same path.
 
 	// As for the actual search algorithm,
 	// we do a payee-to-payer pseudo-Dijkstra's sorting by each node's distance from the payee
@@ -1476,7 +1476,7 @@ 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, lowest_fee_to_node, total_cltv_delta, value_contribution_msat, path_htlc_minimum_msat, path_penalty_msat, path_length_to_node, .. }) = targets.pop() {
+		'path_construction: while let Some(RouteGraphNode { node_id, lowest_fee_to_node, total_cltv_delta, mut value_contribution_msat, path_htlc_minimum_msat, path_penalty_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.
@@ -1542,7 +1542,9 @@ where L::Target: Logger {
 				// on some channels we already passed (assuming dest->source direction). Here, we
 				// recompute the fees again, so that if that's the case, we match the currently
 				// underpaid htlc_minimum_msat with fees.
-				payment_path.update_value_and_recompute_fees(cmp::min(value_contribution_msat, final_value_msat));
+				debug_assert_eq!(payment_path.get_value_msat(), value_contribution_msat);
+				value_contribution_msat = cmp::min(value_contribution_msat, final_value_msat);
+				payment_path.update_value_and_recompute_fees(value_contribution_msat);
 
 				// Since a path allows to transfer as much value as
 				// the smallest channel it has ("bottleneck"), we should recompute
@@ -1653,96 +1655,55 @@ where L::Target: Logger {
 		return Err(LightningError{err: "Failed to find a sufficient route to the given destination".to_owned(), action: ErrorAction::IgnoreError});
 	}
 
-	// Sort by total fees and take the best paths.
-	payment_paths.sort_unstable_by_key(|path| path.get_total_fee_paid_msat());
-	if payment_paths.len() > 50 {
-		payment_paths.truncate(50);
-	}
+	// Step (6).
+	let mut selected_route = payment_paths;
 
-	// Draw multiple sufficient routes by randomly combining the selected paths.
-	let mut drawn_routes = Vec::new();
-	let mut prng = ChaCha20::new(random_seed_bytes, &[0u8; 12]);
-	let mut random_index_bytes = [0u8; ::core::mem::size_of::<usize>()];
+	debug_assert_eq!(selected_route.iter().map(|p| p.get_value_msat()).sum::<u64>(), already_collected_value_msat);
+	let mut overpaid_value_msat = already_collected_value_msat - final_value_msat;
 
-	let num_permutations = payment_paths.len();
-	for _ in 0..num_permutations {
-		let mut cur_route = Vec::<PaymentPath>::new();
-		let mut aggregate_route_value_msat = 0;
+	// 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)))
+	);
 
-		// Step (6).
-		// Do a Fisher-Yates shuffle to create a random permutation of the payment paths
-		for cur_index in (1..payment_paths.len()).rev() {
-			prng.process_in_place(&mut random_index_bytes);
-			let random_index = usize::from_be_bytes(random_index_bytes).wrapping_rem(cur_index+1);
-			payment_paths.swap(cur_index, random_index);
+	// We should make sure that at least 1 path left.
+	let mut paths_left = selected_route.len();
+	selected_route.retain(|path| {
+		if paths_left == 1 {
+			return true
+		}
+		let path_value_msat = path.get_value_msat();
+		if path_value_msat <= overpaid_value_msat {
+			overpaid_value_msat -= path_value_msat;
+			paths_left -= 1;
+			return false;
 		}
+		true
+	});
+	debug_assert!(selected_route.len() > 0);
 
+	if overpaid_value_msat != 0 {
 		// Step (7).
-		for payment_path in &payment_paths {
-			cur_route.push(payment_path.clone());
-			aggregate_route_value_msat += payment_path.get_value_msat();
-			if aggregate_route_value_msat > final_value_msat {
-				// Last path likely overpaid. Substract it from the most expensive
-				// (in terms of proportional fee) path in this route and recompute fees.
-				// This might be not the most economically efficient way, but fewer paths
-				// also makes routing more reliable.
-				let mut overpaid_value_msat = aggregate_route_value_msat - final_value_msat;
-
-				// First, we drop some expensive low-value paths entirely if possible, since fewer
-				// paths is better: the payment is less likely to fail. In order to do so, we sort
-				// by value and fall back to total fees paid, i.e., in case of equal values we
-				// prefer lower cost paths.
-				cur_route.sort_unstable_by(|a, b| {
-					a.get_value_msat().cmp(&b.get_value_msat())
-						// Reverse ordering for cost, so we drop higher-cost paths first
-						.then_with(|| b.get_cost_msat().cmp(&a.get_cost_msat()))
-				});
-
-				// We should make sure that at least 1 path left.
-				let mut paths_left = cur_route.len();
-				cur_route.retain(|path| {
-					if paths_left == 1 {
-						return true
-					}
-					let mut keep = true;
-					let path_value_msat = path.get_value_msat();
-					if path_value_msat <= overpaid_value_msat {
-						keep = false;
-						overpaid_value_msat -= path_value_msat;
-						paths_left -= 1;
-					}
-					keep
-				});
-
-				if overpaid_value_msat == 0 {
-					break;
-				}
+		// Now, subtract the remaining overpaid value from the most-expensive path.
+		// TODO: this could also be optimized by also sorting by feerate_per_sat_routed,
+		// so that the sender pays less fees overall. And also htlc_minimum_msat.
+		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()))
+		});
+		let expensive_payment_path = selected_route.first_mut().unwrap();
 
-				assert!(cur_route.len() > 0);
-
-				// Step (8).
-				// Now, subtract the overpaid value from the most-expensive path.
-				// TODO: this could also be optimized by also sorting by feerate_per_sat_routed,
-				// so that the sender pays less fees overall. And also htlc_minimum_msat.
-				cur_route.sort_unstable_by_key(|path| { path.hops.iter().map(|hop| hop.0.candidate.fees().proportional_millionths as u64).sum::<u64>() });
-				let expensive_payment_path = cur_route.first_mut().unwrap();
-
-				// We already dropped all the small value paths above, meaning all the
-				// remaining paths are larger than remaining overpaid_value_msat.
-				// Thus, this can't be negative.
-				let expensive_path_new_value_msat = expensive_payment_path.get_value_msat() - overpaid_value_msat;
-				expensive_payment_path.update_value_and_recompute_fees(expensive_path_new_value_msat);
-				break;
-			}
-		}
-		drawn_routes.push(cur_route);
+		// We already dropped all the paths with value below `overpaid_value_msat` above, thus this
+		// can't go negative.
+		let expensive_path_new_value_msat = expensive_payment_path.get_value_msat() - overpaid_value_msat;
+		expensive_payment_path.update_value_and_recompute_fees(expensive_path_new_value_msat);
 	}
 
-	// Step (9).
-	// Select the best route by lowest total cost.
-	drawn_routes.sort_unstable_by_key(|paths| paths.iter().map(|path| path.get_cost_msat()).sum::<u64>());
-	let selected_route = drawn_routes.first_mut().unwrap();
-
+	// Step (8).
 	// Sort by the path itself and combine redundant paths.
 	// Note that we sort by SCIDs alone as its simpler but when combining we have to ensure we
 	// compare both SCIDs and NodeIds as individual nodes may use random aliases causing collisions
@@ -4796,7 +4757,7 @@ mod tests {
 				cltv_expiry_delta: 42,
 				htlc_minimum_msat: None,
 				htlc_maximum_msat: None,
-			}])]);
+			}])]).with_max_channel_saturation_power_of_half(0);
 
 		// Keep only two paths from us to nodes[2], both with a 99sat HTLC maximum, with one with
 		// no fee and one with a 1msat fee. Previously, trying to route 100 sats to nodes[2] here
@@ -5780,7 +5741,7 @@ mod tests {
 			flags: 0,
 			cltv_expiry_delta: (4 << 4) | 1,
 			htlc_minimum_msat: 0,
-			htlc_maximum_msat: OptionalField::Present(200_000_000),
+			htlc_maximum_msat: OptionalField::Present(250_000_000),
 			fee_base_msat: 0,
 			fee_proportional_millionths: 0,
 			excess_data: Vec::new()
@@ -5792,7 +5753,7 @@ mod tests {
 			flags: 0,
 			cltv_expiry_delta: (13 << 4) | 1,
 			htlc_minimum_msat: 0,
-			htlc_maximum_msat: OptionalField::Present(200_000_000),
+			htlc_maximum_msat: OptionalField::Present(250_000_000),
 			fee_base_msat: 0,
 			fee_proportional_millionths: 0,
 			excess_data: Vec::new()
@@ -5801,8 +5762,8 @@ mod tests {
 		let payment_params = PaymentParameters::from_node_id(nodes[2]).with_features(InvoiceFeatures::known());
 		let keys_manager = test_utils::TestKeysInterface::new(&[0u8; 32], Network::Testnet);
 		let random_seed_bytes = keys_manager.get_secure_random_bytes();
-		// 150,000 sat is less than the available liquidity on each channel, set above.
-		let route = get_route(&our_id, &payment_params, &network_graph.read_only(), None, 150_000_000, 42, Arc::clone(&logger), &scorer, &random_seed_bytes).unwrap();
+		// 100,000 sats is less than the available liquidity on each channel, set above.
+		let route = get_route(&our_id, &payment_params, &network_graph.read_only(), None, 100_000_000, 42, Arc::clone(&logger), &scorer, &random_seed_bytes).unwrap();
 		assert_eq!(route.paths.len(), 2);
 		assert!((route.paths[0][1].short_channel_id == 4 && route.paths[1][1].short_channel_id == 13) ||
 			(route.paths[1][1].short_channel_id == 4 && route.paths[0][1].short_channel_id == 13));