Merge pull request #1100 from TheBlueMatt/2021-09-multihop-route-hint-fix

Consider many first-hop paths to the same counterparty in routing

Author: TheBlueMatt

lightning/src/routing/router.rs

@@ -487,20 +487,23 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
 			node_info.features.supports_basic_mpp()
 		} else { false }
 	} else { false };
+	log_trace!(logger, "Searching for a route from payer {} to payee {} {} MPP", our_node_id, payee,
+		if allow_mpp { "with" } else { "without" });
 
 	// Step (1).
 	// Prepare the data we'll use for payee-to-payer search by
 	// inserting first hops suggested by the caller as targets.
 	// Our search will then attempt to reach them while traversing from the payee node.
-	let mut first_hop_targets: HashMap<_, (_, ChannelFeatures, _, NodeFeatures)> =
+	let mut first_hop_targets: HashMap<_, Vec<(_, ChannelFeatures, _, NodeFeatures)>> =
 		HashMap::with_capacity(if first_hops.is_some() { first_hops.as_ref().unwrap().len() } else { 0 });
 	if let Some(hops) = first_hops {
 		for chan in hops {
 			let short_channel_id = chan.short_channel_id.expect("first_hops should be filled in with usable channels, not pending ones");
 			if chan.counterparty.node_id == *our_node_id {
 				return Err(LightningError{err: "First hop cannot have our_node_id as a destination.".to_owned(), action: ErrorAction::IgnoreError});
 			}
-			first_hop_targets.insert(chan.counterparty.node_id, (short_channel_id, chan.counterparty.features.to_context(), chan.outbound_capacity_msat, chan.counterparty.features.to_context()));
+			first_hop_targets.entry(chan.counterparty.node_id).or_insert(Vec::new())
+				.push((short_channel_id, chan.counterparty.features.to_context(), chan.outbound_capacity_msat, chan.counterparty.features.to_context()));
 		}
 		if first_hop_targets.is_empty() {
 			return Err(LightningError{err: "Cannot route when there are no outbound routes away from us".to_owned(), action: ErrorAction::IgnoreError});
@@ -824,8 +827,8 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
 			};
 
 			if !skip_node {
-				if first_hops.is_some() {
-					if let Some(&(ref first_hop, ref features, ref outbound_capacity_msat, _)) = first_hop_targets.get(&$node_id) {
+				if let Some(first_channels) = first_hop_targets.get(&$node_id) {
+					for (ref first_hop, ref features, ref outbound_capacity_msat, _) in first_channels {
 						add_entry!(first_hop, *our_node_id, $node_id, dummy_directional_info, Some(outbound_capacity_msat / 1000), features, $fee_to_target_msat, $next_hops_value_contribution, $next_hops_path_htlc_minimum_msat);
 					}
 				}
@@ -878,9 +881,10 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
 
 		// 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, 0, path_value_msat, 0);
+		if let Some(first_channels) = first_hop_targets.get(&payee) {
+			for (ref first_hop, ref features, ref outbound_capacity_msat, _) in first_channels {
+				let added = add_entry!(first_hop, *our_node_id, payee, dummy_directional_info, Some(outbound_capacity_msat / 1000), features, 0, path_value_msat, 0);
+				log_trace!(logger, "{} direct route to payee via SCID {}", if added { "Added" } else { "Skipped" }, first_hop);
 			}
 		}
 
@@ -897,7 +901,7 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
 			},
 		}
 
-		// Step (1).
+		// Step (2).
 		// If a caller provided us with last hops, add them to routing targets. Since this happens
 		// earlier than general path finding, they will be somewhat prioritized, although currently
 		// it matters only if the fees are exactly the same.
@@ -949,8 +953,10 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
 					}
 
 					// Searching for a direct channel between last checked hop and first_hop_targets
-					if let Some(&(ref first_hop, ref features, ref outbound_capacity_msat, _)) = first_hop_targets.get(&prev_hop_id) {
-						add_entry!(first_hop, *our_node_id , prev_hop_id, dummy_directional_info, Some(outbound_capacity_msat / 1000), features, aggregate_next_hops_fee_msat, path_value_msat, aggregate_next_hops_path_htlc_minimum_msat);
+					if let Some(first_channels) = first_hop_targets.get(&prev_hop_id) {
+						for (ref first_hop, ref features, ref outbound_capacity_msat, _) in first_channels {
+							add_entry!(first_hop, *our_node_id , prev_hop_id, dummy_directional_info, Some(outbound_capacity_msat / 1000), features, aggregate_next_hops_fee_msat, path_value_msat, aggregate_next_hops_path_htlc_minimum_msat);
+						}
 					}
 
 					if !hop_used {
@@ -981,8 +987,10 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
 						// 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(&(ref first_hop, ref features, ref outbound_capacity_msat, _)) = first_hop_targets.get(&hop.src_node_id) {
-							add_entry!(first_hop, *our_node_id , hop.src_node_id, dummy_directional_info, Some(outbound_capacity_msat / 1000), features, aggregate_next_hops_fee_msat, path_value_msat, aggregate_next_hops_path_htlc_minimum_msat);
+						if let Some(first_channels) = first_hop_targets.get(&hop.src_node_id) {
+							for (ref first_hop, ref features, ref outbound_capacity_msat, _) in first_channels {
+								add_entry!(first_hop, *our_node_id , hop.src_node_id, dummy_directional_info, Some(outbound_capacity_msat / 1000), features, aggregate_next_hops_fee_msat, path_value_msat, aggregate_next_hops_path_htlc_minimum_msat);
+							}
 						}
 					}
 				}
@@ -995,7 +1003,7 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
 		// last hops communicated by the caller, and the payment receiver.
 		let mut found_new_path = false;
 
-		// Step (2).
+		// Step (3).
 		// If this loop terminates due the exhaustion of targets, two situations are possible:
 		// - not enough outgoing liquidity:
 		//   0 < already_collected_value_msat < final_value_msat
@@ -1013,20 +1021,30 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
 				let mut ordered_hops = vec!((new_entry.clone(), NodeFeatures::empty()));
 
 				'path_walk: loop {
-					if let Some(&(_, _, _, ref features)) = first_hop_targets.get(&ordered_hops.last().unwrap().0.pubkey) {
-						ordered_hops.last_mut().unwrap().1 = features.clone();
-					} else if let Some(node) = network_nodes.get(&ordered_hops.last().unwrap().0.pubkey) {
-						if let Some(node_info) = node.announcement_info.as_ref() {
-							ordered_hops.last_mut().unwrap().1 = node_info.features.clone();
+					let mut features_set = false;
+					if let Some(first_channels) = first_hop_targets.get(&ordered_hops.last().unwrap().0.pubkey) {
+						for (scid, _, _, ref features) in first_channels {
+							if *scid == ordered_hops.last().unwrap().0.short_channel_id {
+								ordered_hops.last_mut().unwrap().1 = features.clone();
+								features_set = true;
+								break;
+							}
+						}
+					}
+					if !features_set {
+						if let Some(node) = network_nodes.get(&ordered_hops.last().unwrap().0.pubkey) {
+							if let Some(node_info) = node.announcement_info.as_ref() {
+								ordered_hops.last_mut().unwrap().1 = node_info.features.clone();
+							} else {
+								ordered_hops.last_mut().unwrap().1 = NodeFeatures::empty();
+							}
 						} else {
-							ordered_hops.last_mut().unwrap().1 = NodeFeatures::empty();
+							// We should be able to fill in features for everything except the last
+							// hop, if the last hop was provided via a BOLT 11 invoice (though we
+							// should be able to extend it further as BOLT 11 does have feature
+							// flags for the last hop node itself).
+							assert!(ordered_hops.last().unwrap().0.pubkey == *payee);
 						}
-					} else {
-						// We should be able to fill in features for everything except the last
-						// hop, if the last hop was provided via a BOLT 11 invoice (though we
-						// should be able to extend it further as BOLT 11 does have feature
-						// flags for the last hop node itself).
-						assert!(ordered_hops.last().unwrap().0.pubkey == *payee);
 					}
 
 					// Means we succesfully traversed from the payer to the payee, now
@@ -1130,7 +1148,7 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
 			break 'paths_collection;
 		}
 
-		// Step (3).
+		// Step (4).
 		// 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,
@@ -1154,7 +1172,7 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
 		}
 	}
 
-	// Step (4).
+	// Step (5).
 	if payment_paths.len() == 0 {
 		return Err(LightningError{err: "Failed to find a path to the given destination".to_owned(), action: ErrorAction::IgnoreError});
 	}
@@ -1175,12 +1193,12 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
 		let mut cur_route = Vec::<PaymentPath>::new();
 		let mut aggregate_route_value_msat = 0;
 
-		// Step (5).
+		// Step (6).
 		// TODO: real random shuffle
 		// Currently just starts with i_th and goes up to i-1_th in a looped way.
 		let cur_payment_paths = [&payment_paths[i..], &payment_paths[..i]].concat();
 
-		// Step (6).
+		// Step (7).
 		for payment_path in cur_payment_paths {
 			cur_route.push(payment_path.clone());
 			aggregate_route_value_msat += payment_path.get_value_msat();
@@ -1219,7 +1237,7 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
 
 				assert!(cur_route.len() > 0);
 
-				// Step (7).
+				// Step (8).
 				// Now, substract 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.
@@ -1236,7 +1254,7 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
 		drawn_routes.push(cur_route);
 	}
 
-	// Step (8).
+	// Step (9).
 	// Select the best route by lowest total fee.
 	drawn_routes.sort_by_key(|paths| paths.iter().map(|path| path.get_total_fee_paid_msat()).sum::<u64>());
 	let mut selected_paths = Vec::<Vec<RouteHop>>::new();
@@ -4220,6 +4238,50 @@ mod tests {
 		}
 	}
 
+	#[test]
+	fn multiple_direct_first_hops() {
+		// Previously we'd only ever considered one first hop path per counterparty.
+		// However, as we don't restrict users to one channel per peer, we really need to support
+		// looking at all first hop paths.
+		// Here we test that we do not ignore all-but-the-last first hop paths per counterparty (as
+		// we used to do by overwriting the `first_hop_targets` hashmap entry) and that we can MPP
+		// route over multiple channels with the same first hop.
+		let secp_ctx = Secp256k1::new();
+		let (_, our_id, _, nodes) = get_nodes(&secp_ctx);
+		let logger = Arc::new(test_utils::TestLogger::new());
+		let network_graph = NetworkGraph::new(genesis_block(Network::Testnet).header.block_hash());
+
+		{
+			let route = get_route(&our_id, &network_graph, &nodes[0], Some(InvoiceFeatures::known()), Some(&[
+				&get_channel_details(Some(3), nodes[0], InitFeatures::known(), 200_000),
+				&get_channel_details(Some(2), nodes[0], InitFeatures::known(), 10_000),
+			]), &[], 100_000, 42, Arc::clone(&logger)).unwrap();
+			assert_eq!(route.paths.len(), 1);
+			assert_eq!(route.paths[0].len(), 1);
+
+			assert_eq!(route.paths[0][0].pubkey, nodes[0]);
+			assert_eq!(route.paths[0][0].short_channel_id, 3);
+			assert_eq!(route.paths[0][0].fee_msat, 100_000);
+		}
+		{
+			let route = get_route(&our_id, &network_graph, &nodes[0], Some(InvoiceFeatures::known()), Some(&[
+				&get_channel_details(Some(3), nodes[0], InitFeatures::known(), 50_000),
+				&get_channel_details(Some(2), nodes[0], InitFeatures::known(), 50_000),
+			]), &[], 100_000, 42, Arc::clone(&logger)).unwrap();
+			assert_eq!(route.paths.len(), 2);
+			assert_eq!(route.paths[0].len(), 1);
+			assert_eq!(route.paths[1].len(), 1);
+
+			assert_eq!(route.paths[0][0].pubkey, nodes[0]);
+			assert_eq!(route.paths[0][0].short_channel_id, 3);
+			assert_eq!(route.paths[0][0].fee_msat, 50_000);
+
+			assert_eq!(route.paths[1][0].pubkey, nodes[0]);
+			assert_eq!(route.paths[1][0].short_channel_id, 2);
+			assert_eq!(route.paths[1][0].fee_msat, 50_000);
+		}
+	}
+
 	#[test]
 	fn total_fees_single_path() {
 		let route = Route {