diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 325920b77..57e604993 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -487,12 +487,14 @@ pub fn get_route(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 { @@ -500,7 +502,8 @@ pub fn get_route(our_node_id: &PublicKey, network: &NetworkGraph, paye 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(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(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(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(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(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(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(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(); - } else { - ordered_hops.last_mut().unwrap().1 = NodeFeatures::empty(); + 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 { + // 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(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(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(our_node_id: &PublicKey, network: &NetworkGraph, paye let mut cur_route = Vec::::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(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(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::()); let mut selected_paths = Vec::>::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 {