X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=blobdiff_plain;f=lightning%2Fsrc%2Frouting%2Frouter.rs;h=add1726623541d11f328635b56ab3e6622ef76e4;hb=5c38e09b2616ee78ccaa257dc4502f2f373c4a9e;hp=fd667107dbf692894b2f6fe5122e1cd94e104e0a;hpb=512642c32d33ada109d5a2c87d5cd0579c2cb91f;p=rust-lightning diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index fd667107..add17266 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -426,13 +426,18 @@ pub fn get_route(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 @@ -556,8 +561,9 @@ pub fn get_route(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 @@ -744,12 +750,13 @@ pub fn get_route(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); } } @@ -762,7 +769,7 @@ pub fn get_route(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); }, } @@ -781,7 +788,7 @@ pub fn get_route(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 @@ -801,7 +808,7 @@ pub fn get_route(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::, ChannelFeatures::empty(), 0, recommended_value_msat); + add_entry!(hop.short_channel_id, hop.src_node_id, payee, directional_info, None::, ChannelFeatures::empty(), 0, path_value_msat); } } @@ -926,12 +933,22 @@ pub fn get_route(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; + } else if found_new_path && already_collected_value_msat == final_value_msat && payment_paths.len() == 1 { + // Further, if this was our first walk of the graph, and we weren't limited by an + // htlc_minimum_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 on the new path while + // still keeping a lower total fee than this path. + if !hit_minimum_limit { + break 'paths_collection; + } + path_value_msat = recommended_value_msat; } } @@ -3435,6 +3452,65 @@ 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, _, 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() + }); + + { + // Now, attempt to route 90 sats, which is exactly 90 sats at the last hop, plus the + // 200% fee charged channel 13 in the 1-to-2 direction. + 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"))]