router: Don't disable central hops in found paths

The original motivation for the block was to ensure MPPs vary their
paths a bit by disabling the 'central' hop in any found paths. However,
we are disabling this block, as it:

1) causes MPP routing failures if there is only one path from payer -> payee
2) forces MPPs to be routed across less optimal paths

Issue: lightningdevkit#3685

The one possible privacy concern is that any hops which forward multiple shards
of the same MPP can link those shards together, as each shard uses the same
payment hash (since LDK implements Simplified Multipath Payments, not Atomic
Multipath Payments). But this concern seems moot, as sending the payment over a
single shard leaks almost the same information anyway.

As far as I (and SOTA AI models) can tell, disabling this block doesn't violate
any correctness constraints or security concerns in surrounding code.

License: PolyForm Noncommercial License 1.0.0

Author: MaxFangX

lightning/src/routing/router.rs

@@ -2156,21 +2156,21 @@ where L::Target: Logger {
 	// The general routing idea is the following:
 	// 1. Fill first/last hops communicated by the caller.
 	// 2. Attempt to construct a path from payer to payee for transferring
-	//    any ~sufficient (described later) value.
-	//    If succeed, remember which channels were used and how much liquidity they have available,
-	//    so that future paths don't rely on the same liquidity.
+	//	any ~sufficient (described later) value.
+	//	If succeed, remember which channels were used and how much liquidity they have available,
+	//	so that future paths don't rely on the same liquidity.
 	// 3. Proceed to the next step if:
-	//    - we hit the recommended target value;
-	//    - OR if we could not construct a new path. Any next attempt will fail too.
-	//    Otherwise, repeat step 2.
+	//	- we hit the recommended target value;
+	//	- OR if we could not construct a new path. Any next attempt will fail too.
+	//	Otherwise, repeat step 2.
 	// 4. See if we managed to collect paths which aggregately are able to transfer target value
-	//    (not recommended value).
+	//	(not recommended value).
 	// 5. If yes, proceed. If not, fail routing.
 	// 6. Select the paths which have the lowest cost (fee plus scorer penalty) per amount
-	//    transferred up to the transfer target value.
+	//	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.
+	//	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 Dijkstra's sorting by each node's
 	// distance from the payee
@@ -2917,7 +2917,7 @@ where L::Target: Logger {
 				for (idx, (hop, prev_hop_id)) in hop_iter.zip(prev_hop_iter).enumerate() {
 					let (target, private_target_node_counter) =
 						node_counters.private_node_counter_from_pubkey(&prev_hop_id)
-    						.expect("node_counter_from_pubkey is called on all unblinded_route_hints keys during setup, so is always Some here");
+							.expect("node_counter_from_pubkey is called on all unblinded_route_hints keys during setup, so is always Some here");
 					let (_src_id, private_source_node_counter) =
 						node_counters.private_node_counter_from_pubkey(&hop.src_node_id)
 							.expect("node_counter_from_pubkey is called on all unblinded_route_hints keys during setup, so is always Some here");
@@ -3167,6 +3167,27 @@ where L::Target: Logger {
 					}
 					debug_assert!(*used_liquidity_msat <= hop_max_msat);
 				}
+
+				// NOTE: The original motivation for the block was to ensure MPPs vary their paths a
+				// bit by disabling the 'central' hop in any found paths. However, we are disabling
+				// this block, as it:
+				//
+				// 1) causes MPP routing failures if there is only one path from payer -> payee
+				// 2) forces MPPs to be routed across multiple paths; the additional paths are
+				//    usually more expensive and less reliable (less liquidity).
+				//
+				// Issue: https://github.com/lightningdevkit/rust-lightning/issues/3685
+				//
+				// The one possible privacy concern is that any hops which forward multiple shards
+				// of the same MPP can link those shards together, as each shard uses the same
+				// payment hash (since LDK implements Simplified Multipath Payments, not Atomic
+				// Multipath Payments). But this concern seems moot, as sending the payment over a
+				// single shard leaks almost the same information anyway.
+				//
+				// As far as I (and SOTA AI models) can tell, disabling this block doesn't violate
+				// any correctness constraints or security concerns in surrounding code.
+				let _ = prevented_redundant_path_selection;
+				/*
 				if !prevented_redundant_path_selection {
 					// If we weren't capped by hitting a liquidity limit on a channel in the path,
 					// we'll probably end up picking the same path again on the next iteration.
@@ -3181,6 +3202,7 @@ where L::Target: Logger {
 						*used_liquidities.entry(CandidateHopId::Clear((scid, true))).or_default() = exhausted;
 					}
 				}
+				*/
 
 				// Track the total amount all our collected paths allow to send so that we know
 				// when to stop looking for more paths
@@ -3332,8 +3354,8 @@ where L::Target: Logger {
 	});
 	for idx in 0..(selected_route.len() - 1) {
 		if idx + 1 >= selected_route.len() { break; }
-		if iter_equal(selected_route[idx    ].hops.iter().map(|h| (h.0.candidate.id(), h.0.candidate.target())),
-		              selected_route[idx + 1].hops.iter().map(|h| (h.0.candidate.id(), h.0.candidate.target()))) {
+		if iter_equal(selected_route[idx	].hops.iter().map(|h| (h.0.candidate.id(), h.0.candidate.target())),
+					  selected_route[idx + 1].hops.iter().map(|h| (h.0.candidate.id(), h.0.candidate.target()))) {
 			let new_value = selected_route[idx].get_value_msat() + selected_route[idx + 1].get_value_msat();
 			selected_route[idx].update_value_and_recompute_fees(new_value);
 			selected_route.remove(idx + 1);
@@ -6602,12 +6624,12 @@ mod tests {
 
 		// We construct a network that looks like this:
 		//
-		//            node2 -1(3)2- node3
-		//              2          2
-		//               (2)     (4)
-		//                  1   1
-		//    node1 -1(5)2- node4 -1(1)2- node6
-		//    2
+		//			node2 -1(3)2- node3
+		//			  2		  2
+		//			   (2)	 (4)
+		//				  1   1
+		//	node1 -1(5)2- node4 -1(1)2- node6
+		//	2
 		//   (6)
 		//	  1
 		// our_node
@@ -8113,14 +8135,14 @@ mod tests {
 		// 1. We add a route candidate for intro_node contributing a high amount
 		// 2. We add a first_hop<>intro_node route candidate for the same high amount
 		// 3. We see a cheaper blinded route hint for the same intro node but a much lower contribution
-		//    amount, and update our route candidate for intro_node for the lower amount
+		//	amount, and update our route candidate for intro_node for the lower amount
 		// 4. We then attempt to update the aforementioned first_hop<>intro_node route candidate for the
-		//    lower contribution amount, but fail (this was previously caused by failure to account for
-		//    blinded path fees when adding first_hop<>intro_node candidates)
+		//	lower contribution amount, but fail (this was previously caused by failure to account for
+		//	blinded path fees when adding first_hop<>intro_node candidates)
 		// 5. We go to construct the path from these route candidates and our first_hop<>intro_node
-		//    candidate still thinks its path is contributing the original higher amount. This caused us
-		//    to hit an `unreachable` overflow when calculating the cheaper intro_node fees over the
-		//    larger amount
+		//	candidate still thinks its path is contributing the original higher amount. This caused us
+		//	to hit an `unreachable` overflow when calculating the cheaper intro_node fees over the
+		//	larger amount
 		let secp_ctx = Secp256k1::new();
 		let logger = Arc::new(ln_test_utils::TestLogger::new());
 		let network_graph = Arc::new(NetworkGraph::new(Network::Testnet, Arc::clone(&logger)));