From 5861dde6923f0dbbd9145efe2747be3c13e60b92 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 28 Sep 2023 18:19:50 +0000 Subject: [PATCH 1/4] Allow routefinding again due to HTLC limit based on >= not > While this doesn't matter much in practice, if we go around again when route-finding to try to meet an htlc_minimum_msat, we use the `recommended_value_msat` which can work if we meet the `htlc_minimum_msat` on a channel exactly, so using >= rather than > can capture cases with 1msat more. --- lightning/src/routing/router.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 193fe352334..1c51bb985e9 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -1788,9 +1788,9 @@ where L::Target: Logger { #[allow(unused_comparisons)] // $next_hops_path_htlc_minimum_msat is 0 in some calls so rustc complains let may_overpay_to_meet_path_minimum_msat = ((amount_to_transfer_over_msat < $candidate.htlc_minimum_msat() && - recommended_value_msat > $candidate.htlc_minimum_msat()) || + recommended_value_msat >= $candidate.htlc_minimum_msat()) || (amount_to_transfer_over_msat < $next_hops_path_htlc_minimum_msat && - recommended_value_msat > $next_hops_path_htlc_minimum_msat)); + recommended_value_msat >= $next_hops_path_htlc_minimum_msat)); let payment_failed_on_this_channel = scid_opt.map_or(false, |scid| payment_params.previously_failed_channels.contains(&scid)); From f3e33f40742e0543850a181718335e5d54e3bb7c Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 26 Sep 2023 21:58:53 +0000 Subject: [PATCH 2/4] Try to overpay the recipient if we fail to find a path at all Previously we'd only try to overpay if we managed to find a path to the recipient which was sufficient. However, if we fail to find any path to the recipient at all we should still retry overpaying the recipient. Ultimately we should be silling to pay whatever reasonable performance penalty if the alternative is not finding a path at all, which we do here. --- lightning/src/routing/router.rs | 53 +++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 1c51bb985e9..bca681dfc9b 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -2454,6 +2454,9 @@ where L::Target: Logger { // because we deterministically terminated the search due to low liquidity. if !found_new_path && channel_saturation_pow_half != 0 { channel_saturation_pow_half = 0; + } else if !found_new_path && hit_minimum_limit && already_collected_value_msat < final_value_msat && path_value_msat != recommended_value_msat { + log_trace!(logger, "Failed to collect enough value, but running again to collect extra paths with a potentially higher limit."); + path_value_msat = recommended_value_msat; } else if already_collected_value_msat >= recommended_value_msat || !found_new_path { log_trace!(logger, "Have now collected {} msat (seeking {} msat) in paths. Last path loop {} a new path.", already_collected_value_msat, recommended_value_msat, if found_new_path { "found" } else { "did not find" }); @@ -3223,6 +3226,56 @@ mod tests { assert_eq!(fees, 5_000); } + #[test] + fn htlc_minimum_recipient_overpay_test() { + let (secp_ctx, network_graph, gossip_sync, _, logger) = build_graph(); + let (_, our_id, privkeys, nodes) = get_nodes(&secp_ctx); + let config = UserConfig::default(); + let payment_params = PaymentParameters::from_node_id(nodes[2], 42).with_bolt11_features(channelmanager::provided_invoice_features(&config)).unwrap(); + let scorer = ln_test_utils::TestScorer::new(); + let keys_manager = ln_test_utils::TestKeysInterface::new(&[0u8; 32], Network::Testnet); + let random_seed_bytes = keys_manager.get_secure_random_bytes(); + + // Route to node2 over a single path which requires overpaying the recipient themselves. + + // First disable all paths except the us -> node1 -> node2 path + update_channel(&gossip_sync, &secp_ctx, &privkeys[2], UnsignedChannelUpdate { + chain_hash: genesis_block(Network::Testnet).header.block_hash(), + short_channel_id: 13, + timestamp: 2, + flags: 3, + cltv_expiry_delta: 0, + htlc_minimum_msat: 0, + htlc_maximum_msat: 0, + fee_base_msat: 0, + fee_proportional_millionths: 0, + excess_data: Vec::new() + }); + + // Set channel 4 to free but with a high htlc_minimum_msat + update_channel(&gossip_sync, &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: 0, + htlc_minimum_msat: 15_000, + htlc_maximum_msat: MAX_VALUE_MSAT, + fee_base_msat: 0, + fee_proportional_millionths: 0, + excess_data: Vec::new() + }); + + // Now check that we'll find a path if the htlc_minimum is overrun substantially. + let mut route_params = RouteParameters::from_payment_params_and_value( + payment_params.clone(), 5_000); + // TODO: This can even overrun the fee limit set by the recipient! + route_params.max_total_routing_fee_msat = Some(9_999); + let route = get_route(&our_id, &route_params, &network_graph.read_only(), None, + Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes).unwrap(); + assert_eq!(route.get_total_fees(), 10_000); + } + #[test] fn disable_channels_test() { let (secp_ctx, network_graph, gossip_sync, _, logger) = build_graph(); From 8effd86c217ab915e11c36a29f147dc5735d9593 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 28 Sep 2023 18:19:36 +0000 Subject: [PATCH 3/4] Include any recipient overpayment amounts in the route fee limit If the user told us to limit their total fee exposure, we should do so including any potential overpayment to the recipient, which is ultimately a part of the "fee" as far as the user is concerned. --- lightning/src/routing/router.rs | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index bca681dfc9b..19cf4e05d95 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -2628,14 +2628,6 @@ where L::Target: Logger { // Make sure we would never create a route with more paths than we allow. debug_assert!(paths.len() <= payment_params.max_path_count.into()); - // Make sure we would never create a route whose total fees exceed max_total_routing_fee_msat. - if let Some(max_total_routing_fee_msat) = route_params.max_total_routing_fee_msat { - if paths.iter().map(|p| p.fee_msat()).sum::() > max_total_routing_fee_msat { - return Err(LightningError{err: format!("Failed to find route that adheres to the maximum total fee limit of {}msat", - max_total_routing_fee_msat), action: ErrorAction::IgnoreError}); - } - } - if let Some(node_features) = payment_params.payee.node_features() { for path in paths.iter_mut() { path.hops.last_mut().unwrap().node_features = node_features.clone(); @@ -2643,6 +2635,15 @@ where L::Target: Logger { } let route = Route { paths, route_params: Some(route_params.clone()) }; + + // Make sure we would never create a route whose total fees exceed max_total_routing_fee_msat. + if let Some(max_total_routing_fee_msat) = route_params.max_total_routing_fee_msat { + if route.get_total_fees() > max_total_routing_fee_msat { + return Err(LightningError{err: format!("Failed to find route that adheres to the maximum total fee limit of {}msat", + max_total_routing_fee_msat), action: ErrorAction::IgnoreError}); + } + } + log_info!(logger, "Got route: {}", log_route!(route)); Ok(route) } @@ -3266,11 +3267,22 @@ mod tests { excess_data: Vec::new() }); - // Now check that we'll find a path if the htlc_minimum is overrun substantially. + // Now check that we'll fail to find a path if we fail to find a path if the htlc_minimum + // is overrun. Note that the fees are actually calculated on 3*payment amount as that's + // what we try to find a route for, so this test only just happens to work out to exactly + // the fee limit. let mut route_params = RouteParameters::from_payment_params_and_value( payment_params.clone(), 5_000); - // TODO: This can even overrun the fee limit set by the recipient! route_params.max_total_routing_fee_msat = Some(9_999); + if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route(&our_id, + &route_params, &network_graph.read_only(), None, Arc::clone(&logger), &scorer, + &Default::default(), &random_seed_bytes) { + assert_eq!(err, "Failed to find route that adheres to the maximum total fee limit of 9999msat"); + } else { panic!(); } + + let mut route_params = RouteParameters::from_payment_params_and_value( + payment_params.clone(), 5_000); + route_params.max_total_routing_fee_msat = Some(10_000); let route = get_route(&our_id, &route_params, &network_graph.read_only(), None, Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes).unwrap(); assert_eq!(route.get_total_fees(), 10_000); From fa48df60490d1297ac523e667c45f9c793e274d0 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 28 Sep 2023 17:51:05 +0000 Subject: [PATCH 4/4] Log max routing fee before we start pathfinding This may be useful in debugging routing failures in the future. --- lightning/src/routing/router.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 19cf4e05d95..33182f7cb0d 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -1622,9 +1622,13 @@ where L::Target: Logger { |info| info.features.supports_basic_mpp())) } else { false }; - log_trace!(logger, "Searching for a route from payer {} to {} {} MPP and {} first hops {}overriding the network graph", our_node_pubkey, - LoggedPayeePubkey(payment_params.payee.node_id()), if allow_mpp { "with" } else { "without" }, - first_hops.map(|hops| hops.len()).unwrap_or(0), if first_hops.is_some() { "" } else { "not " }); + let max_total_routing_fee_msat = route_params.max_total_routing_fee_msat.unwrap_or(u64::max_value()); + + log_trace!(logger, "Searching for a route from payer {} to {} {} MPP and {} first hops {}overriding the network graph with a fee limit of {} msat", + our_node_pubkey, LoggedPayeePubkey(payment_params.payee.node_id()), + if allow_mpp { "with" } else { "without" }, + first_hops.map(|hops| hops.len()).unwrap_or(0), if first_hops.is_some() { "" } else { "not " }, + max_total_routing_fee_msat); // Step (1). // Prepare the data we'll use for payee-to-payer search by @@ -1890,7 +1894,6 @@ where L::Target: Logger { } // Ignore hops if augmenting the current path to them would put us over `max_total_routing_fee_msat` - let max_total_routing_fee_msat = route_params.max_total_routing_fee_msat.unwrap_or(u64::max_value()); if total_fee_msat > max_total_routing_fee_msat { if should_log_candidate { log_trace!(logger, "Ignoring {} due to exceeding max total routing fee limit.", LoggedCandidateHop(&$candidate));