From: Matt Corallo Date: Sat, 27 Mar 2021 02:53:35 +0000 (-0400) Subject: [router] Track full-path htlc-minimum-msat while graph-walking X-Git-Tag: v0.0.14~37^2~8 X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=93311b3684c2ad90e80f888641c775b16c0c25a3;p=rust-lightning [router] Track full-path htlc-minimum-msat while graph-walking Previously, we'd happily send funds through a path where, while generating the path, in some middle hope we reduce the value being sent to meet an htlc_maximum, making a later hop invalid due to it no longer meeting its htlc_minimum. Instead, we need to track the path's htlc-minimum while we're transiting the graph. --- diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index add172662..0163ae515 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -143,7 +143,10 @@ struct RouteGraphNode { // - how much is needed for a path being constructed // - how much value can channels following this node (up to the destination) can contribute, // considering their capacity and fees - value_contribution_msat: u64 + value_contribution_msat: u64, + /// The effective htlc_minimum_msat at this hop. If a later hop on the path had a higher HTLC + /// minimum, we use it, plus the fees required at each earlier hop to meet it. + path_htlc_minimum_msat: u64, } impl cmp::Ord for RouteGraphNode { @@ -488,7 +491,7 @@ pub fn get_route(our_node_id: &PublicKey, network: &NetworkGraph, paye // $next_hops_fee_msat represents the fees paid for using all the channel *after* this one, // since that value has to be transferred over this channel. ( $chan_id: expr, $src_node_id: expr, $dest_node_id: expr, $directional_info: expr, $capacity_sats: expr, $chan_features: expr, $next_hops_fee_msat: expr, - $next_hops_value_contribution: expr ) => { + $next_hops_value_contribution: expr, $next_hops_path_htlc_minimum_msat: expr ) => { // Channels to self should not be used. This is more of belt-and-suspenders, because in // practice these cases should be caught earlier: // - for regular channels at channel announcement (TODO) @@ -555,19 +558,27 @@ pub fn get_route(our_node_id: &PublicKey, network: &NetworkGraph, paye // Can't overflow due to how the values were computed right above. None => unreachable!(), }; + #[allow(unused_comparisons)] // $next_hops_path_htlc_minimum_msat is 0 in some calls so rustc complains + let over_path_minimum_msat = amount_to_transfer_over_msat >= $directional_info.htlc_minimum_msat && + amount_to_transfer_over_msat >= $next_hops_path_htlc_minimum_msat; // If HTLC minimum is larger than the amount we're going to transfer, we shouldn't // bother considering this channel. // 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. - if amount_to_transfer_over_msat < $directional_info.htlc_minimum_msat { + if !over_path_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 // path fees knowing the final path contribution after constructing it. + let path_htlc_minimum_msat = match compute_fees($next_hops_path_htlc_minimum_msat, $directional_info.fees) + .map(|fee_msat| fee_msat.checked_add($next_hops_path_htlc_minimum_msat)) { + Some(Some(value_msat)) => cmp::max(value_msat, $directional_info.htlc_minimum_msat), + _ => u64::max_value() + }; let hm_entry = dist.entry(&$src_node_id); let old_entry = hm_entry.or_insert_with(|| { // If there was previously no known way to access @@ -639,6 +650,7 @@ pub fn get_route(our_node_id: &PublicKey, network: &NetworkGraph, paye lowest_fee_to_peer_through_node: total_fee_msat, lowest_fee_to_node: $next_hops_fee_msat as u64 + hop_use_fee_msat, value_contribution_msat: value_contribution_msat, + path_htlc_minimum_msat, }; // Update the way of reaching $src_node_id with the given $chan_id (from $dest_node_id), @@ -698,10 +710,10 @@ pub fn get_route(our_node_id: &PublicKey, network: &NetworkGraph, paye // meaning how much will be paid in fees after this node (to the best of our knowledge). // This data can later be helpful to optimize routing (pay lower fees). macro_rules! add_entries_to_cheapest_to_target_node { - ( $node: expr, $node_id: expr, $fee_to_target_msat: expr, $next_hops_value_contribution: expr ) => { + ( $node: expr, $node_id: expr, $fee_to_target_msat: expr, $next_hops_value_contribution: expr, $next_hops_path_htlc_minimum_msat: expr ) => { if first_hops.is_some() { if let Some(&(ref first_hop, ref features, ref outbound_capacity_msat)) = first_hop_targets.get(&$node_id) { - add_entry!(first_hop, *our_node_id, $node_id, dummy_directional_info, Some(outbound_capacity_msat / 1000), features.to_context(), $fee_to_target_msat, $next_hops_value_contribution); + add_entry!(first_hop, *our_node_id, $node_id, dummy_directional_info, Some(outbound_capacity_msat / 1000), features.to_context(), $fee_to_target_msat, $next_hops_value_contribution, $next_hops_path_htlc_minimum_msat); } } @@ -721,7 +733,7 @@ pub fn get_route(our_node_id: &PublicKey, network: &NetworkGraph, paye if first_hops.is_none() || chan.node_two != *our_node_id { if let Some(two_to_one) = chan.two_to_one.as_ref() { if two_to_one.enabled { - add_entry!(chan_id, chan.node_two, chan.node_one, two_to_one, chan.capacity_sats, chan.features, $fee_to_target_msat, $next_hops_value_contribution); + add_entry!(chan_id, chan.node_two, chan.node_one, two_to_one, chan.capacity_sats, chan.features, $fee_to_target_msat, $next_hops_value_contribution, $next_hops_path_htlc_minimum_msat); } } } @@ -729,7 +741,7 @@ pub fn get_route(our_node_id: &PublicKey, network: &NetworkGraph, paye if first_hops.is_none() || chan.node_one != *our_node_id { if let Some(one_to_two) = chan.one_to_two.as_ref() { if one_to_two.enabled { - add_entry!(chan_id, chan.node_one, chan.node_two, one_to_two, chan.capacity_sats, chan.features, $fee_to_target_msat, $next_hops_value_contribution); + add_entry!(chan_id, chan.node_one, chan.node_two, one_to_two, chan.capacity_sats, chan.features, $fee_to_target_msat, $next_hops_value_contribution, $next_hops_path_htlc_minimum_msat); } } @@ -756,7 +768,7 @@ pub fn get_route(our_node_id: &PublicKey, network: &NetworkGraph, paye // 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, path_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, 0); } } @@ -769,7 +781,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, path_value_msat); + add_entries_to_cheapest_to_target_node!(node, payee, 0, path_value_msat, 0); }, } @@ -788,7 +800,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, path_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, 0); true } else { // In any other case, only add the hop if the source is in the regular network @@ -808,7 +820,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, path_value_msat); + add_entry!(hop.short_channel_id, hop.src_node_id, payee, directional_info, None::, ChannelFeatures::empty(), 0, path_value_msat, 0); } } @@ -825,7 +837,7 @@ pub fn get_route(our_node_id: &PublicKey, network: &NetworkGraph, paye // 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 { pubkey, lowest_fee_to_node, value_contribution_msat, .. }) = targets.pop() { + 'path_construction: while let Some(RouteGraphNode { pubkey, lowest_fee_to_node, value_contribution_msat, path_htlc_minimum_msat, .. }) = 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. @@ -922,7 +934,7 @@ pub fn get_route(our_node_id: &PublicKey, network: &NetworkGraph, paye match network.get_nodes().get(&pubkey) { None => {}, Some(node) => { - add_entries_to_cheapest_to_target_node!(node, &pubkey, lowest_fee_to_node, value_contribution_msat); + add_entries_to_cheapest_to_target_node!(node, &pubkey, lowest_fee_to_node, value_contribution_msat, path_htlc_minimum_msat); }, } } @@ -3511,6 +3523,67 @@ mod tests { assert_eq!(route.paths[0][1].channel_features.le_flags(), &id_to_feature_flags(13)); } } + + #[test] + fn htlc_max_reduction_below_min() { + // Test that if, while walking the graph, we reduce the value being sent to meet an + // htlc_maximum_msat, we don't end up undershooting a later htlc_minimum_msat. In the + // initial version of MPP we'd accept such routes but reject them while recalculating fees, + // resulting in us thinking there is no possible path, even if other paths exist. + let (secp_ctx, net_graph_msg_handler, _, logger) = build_graph(); + let (our_privkey, our_id, privkeys, nodes) = get_nodes(&secp_ctx); + + // We modify the graph to set the htlc_minimum of channel 2 and 4 as needed - channel 2 + // gets an htlc_maximum_msat of 80_000 and channel 4 an htlc_minimum_msat of 90_000. We + // then try to send 90_000. + 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(80_000), + fee_base_msat: 0, + fee_proportional_millionths: 0, + excess_data: Vec::new() + }); + update_channel(&net_graph_msg_handler, &secp_ctx, &privkeys[1], UnsignedChannelUpdate { + chain_hash: genesis_block(Network::Testnet).header.block_hash(), + short_channel_id: 4, + timestamp: 2, + flags: 0, + cltv_expiry_delta: (4 << 8) | 1, + htlc_minimum_msat: 90_000, + htlc_maximum_msat: OptionalField::Absent, + fee_base_msat: 0, + fee_proportional_millionths: 0, + excess_data: Vec::new() + }); + + { + // Now, attempt to route 90 sats, hitting the htlc_minimum on channel 4, but + // overshooting the htlc_maximum on channel 2. Thus, we should pick the (absurdly + // expensive) channels 12-13 path. + let route = get_route(&our_id, &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[2], Some(InvoiceFeatures::known()), 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(), InvoiceFeatures::known().le_flags()); + assert_eq!(route.paths[0][1].channel_features.le_flags(), &id_to_feature_flags(13)); + } + } } #[cfg(all(test, feature = "unstable"))]