From 391da3f3b237fce4a6f4a9eeb44ee8583bf91185 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 14 Sep 2023 14:14:03 +0200 Subject: [PATCH 1/8] Use `saturating_add` when adding inflight HTLCs values Previously this calculation could overflow, leading to panicking in `debug`. --- lightning/src/routing/router.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 09d83bb785c..0e3bc96ea00 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -139,7 +139,7 @@ impl<'a, SP: Sized, Sc: 'a + ScoreLookUp, S: Deref Date: Fri, 22 Sep 2023 15:00:43 +0200 Subject: [PATCH 2/8] Extend logging of ignored candidates --- lightning/src/routing/router.rs | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 0e3bc96ea00..8db682ebdfe 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -1718,6 +1718,8 @@ where L::Target: Logger { let mut num_ignored_cltv_delta_limit = 0; let mut num_ignored_previously_failed = 0; let mut num_ignored_total_fee_limit = 0; + let mut num_ignored_avoid_overpayment = 0; + let mut num_ignored_htlc_minimum_msat_limit = 0; macro_rules! add_entry { // Adds entry which goes from $src_node_id to $dest_node_id over the $candidate hop. @@ -1826,6 +1828,12 @@ where L::Target: Logger { } num_ignored_previously_failed += 1; } else if may_overpay_to_meet_path_minimum_msat { + if should_log_candidate { + log_trace!(logger, + "Ignoring {} to avoid overpaying to meet htlc_minimum_msat limit.", + LoggedCandidateHop(&$candidate)); + } + num_ignored_avoid_overpayment += 1; hit_minimum_limit = true; } else if over_path_minimum_msat { // Note that low contribution here (limited by available_liquidity_msat) @@ -1976,6 +1984,13 @@ where L::Target: Logger { } } } + } else { + if should_log_candidate { + log_trace!(logger, + "Ignoring {} due to its htlc_minimum_msat limit.", + LoggedCandidateHop(&$candidate)); + } + num_ignored_htlc_minimum_msat_limit += 1; } } } @@ -2443,15 +2458,22 @@ where L::Target: Logger { log_trace!(logger, "Collected exactly our payment amount on the first pass, without hitting an htlc_minimum_msat limit, exiting."); break 'paths_collection; } - log_trace!(logger, "Collected our payment amount on the first pass, but running again to collect extra paths with a potentially higher limit."); + log_trace!(logger, "Collected our payment amount on the first pass, but running again to collect extra paths with a potentially higher value to meet htlc_minimum_msat limit."); path_value_msat = recommended_value_msat; } } let num_ignored_total = num_ignored_value_contribution + num_ignored_path_length_limit + - num_ignored_cltv_delta_limit + num_ignored_previously_failed + num_ignored_total_fee_limit; + num_ignored_cltv_delta_limit + num_ignored_previously_failed + + num_ignored_avoid_overpayment + num_ignored_htlc_minimum_msat_limit + + num_ignored_total_fee_limit; if num_ignored_total > 0 { - log_trace!(logger, "Ignored {} candidate hops due to insufficient value contribution, {} due to path length limit, {} due to CLTV delta limit, {} due to previous payment failure, {} due to maximum total fee limit. Total: {} ignored candidates.", num_ignored_value_contribution, num_ignored_path_length_limit, num_ignored_cltv_delta_limit, num_ignored_previously_failed, num_ignored_total_fee_limit, num_ignored_total); + log_trace!(logger, + "Ignored {} candidate hops due to insufficient value contribution, {} due to path length limit, {} due to CLTV delta limit, {} due to previous payment failure, {} due to htlc_minimum_msat limit, {} to avoid overpaying, {} due to maximum total fee limit. Total: {} ignored candidates.", + num_ignored_value_contribution, num_ignored_path_length_limit, + num_ignored_cltv_delta_limit, num_ignored_previously_failed, + num_ignored_htlc_minimum_msat_limit, num_ignored_avoid_overpayment, + num_ignored_total_fee_limit, num_ignored_total); } // Step (5). From 7a6d3097a6aa59b5aa2e8e83527cd3ee79560750 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Fri, 22 Sep 2023 15:56:07 +0200 Subject: [PATCH 3/8] Also add route hints if we are the source Previously, we would only consider route hints if we had a direct channel to the first node in the hint or if the first node in the hint was part of the public network graph. However, this left out the possiblity of us being part of the first hop, especially if our own node is not announced and part of the graph. --- lightning/src/routing/router.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 8db682ebdfe..0a8ee425881 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -2142,14 +2142,15 @@ where L::Target: Logger { for route in payment_params.payee.unblinded_route_hints().iter() .filter(|route| !route.0.is_empty()) { - let first_hop_in_route = &(route.0)[0]; - let have_hop_src_in_graph = - // Only add the hops in this route to our candidate set if either - // we have a direct channel to the first hop or the first hop is - // in the regular network graph. - first_hop_targets.get(&NodeId::from_pubkey(&first_hop_in_route.src_node_id)).is_some() || - network_nodes.get(&NodeId::from_pubkey(&first_hop_in_route.src_node_id)).is_some(); - if have_hop_src_in_graph { + let first_hop_src_id = NodeId::from_pubkey(&route.0.first().unwrap().src_node_id); + let first_hop_src_is_reachable = + // Only add the hops in this route to our candidate set if either we are part of + // the first hop, we have a direct channel to the first hop, or the first hop is in + // the regular network graph. + our_node_id == first_hop_src_id || + first_hop_targets.get(&first_hop_src_id).is_some() || + network_nodes.get(&first_hop_src_id).is_some(); + if first_hop_src_is_reachable { // We start building the path from reverse, i.e., from payee // to the first RouteHintHop in the path. let hop_iter = route.0.iter().rev(); From c480b0c857d0999c12c1c2507a5d8c93592d91cc Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Fri, 22 Sep 2023 16:38:30 +0200 Subject: [PATCH 4/8] Avoid adding duplicate hint candidates if they are first hops If we have a direct channel to a node generating an invoice with route hints, we'd previously happily add multiple candidates that all refer to the same channel. To keep our candidate set small and unify our tracking where possible, we now check if its `short_channel_id` is an `outbound_scid_alias` of any of our first hops and refrain from adding another candidate if it's the case. --- lightning/src/routing/router.rs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 0a8ee425881..940b846bbad 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -2167,6 +2167,15 @@ where L::Target: Logger { for (idx, (hop, prev_hop_id)) in hop_iter.zip(prev_hop_iter).enumerate() { let source = NodeId::from_pubkey(&hop.src_node_id); let target = NodeId::from_pubkey(&prev_hop_id); + + if let Some(first_channels) = first_hop_targets.get(&target) { + if first_channels.iter().any(|d| d.outbound_scid_alias == Some(hop.short_channel_id)) { + log_trace!(logger, "Ignoring route hint with SCID {} (and any previous) due to it being a direct channel of ours.", + hop.short_channel_id); + break; + } + } + let candidate = network_channels .get(&hop.short_channel_id) .and_then(|channel| channel.as_directed_to(&target)) @@ -2210,12 +2219,12 @@ where L::Target: Logger { .saturating_add(1); // Searching for a direct channel between last checked hop and first_hop_targets - if let Some(first_channels) = first_hop_targets.get_mut(&NodeId::from_pubkey(&prev_hop_id)) { + if let Some(first_channels) = first_hop_targets.get_mut(&target) { sort_first_hop_channels(first_channels, &used_liquidities, recommended_value_msat, our_node_pubkey); for details in first_channels { let first_hop_candidate = CandidateRouteHop::FirstHop { details }; - add_entry!(first_hop_candidate, our_node_id, NodeId::from_pubkey(&prev_hop_id), + add_entry!(first_hop_candidate, our_node_id, target, aggregate_next_hops_fee_msat, aggregate_path_contribution_msat, aggregate_next_hops_path_htlc_minimum_msat, aggregate_next_hops_path_penalty_msat, aggregate_next_hops_cltv_delta, aggregate_next_hops_path_length); From 39c5bbc0bb3b69941a355250859dbadf1203a499 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Wed, 27 Sep 2023 09:28:43 +0200 Subject: [PATCH 5/8] Make ignored candidate counts `u32` .. as a follow-up from #2417. --- lightning/src/routing/router.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 940b846bbad..193fe352334 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -1713,13 +1713,13 @@ where L::Target: Logger { LoggedPayeePubkey(payment_params.payee.node_id()), our_node_pubkey, final_value_msat); // Remember how many candidates we ignored to allow for some logging afterwards. - let mut num_ignored_value_contribution = 0; - let mut num_ignored_path_length_limit = 0; - let mut num_ignored_cltv_delta_limit = 0; - let mut num_ignored_previously_failed = 0; - let mut num_ignored_total_fee_limit = 0; - let mut num_ignored_avoid_overpayment = 0; - let mut num_ignored_htlc_minimum_msat_limit = 0; + let mut num_ignored_value_contribution: u32 = 0; + let mut num_ignored_path_length_limit: u32 = 0; + let mut num_ignored_cltv_delta_limit: u32 = 0; + let mut num_ignored_previously_failed: u32 = 0; + let mut num_ignored_total_fee_limit: u32 = 0; + let mut num_ignored_avoid_overpayment: u32 = 0; + let mut num_ignored_htlc_minimum_msat_limit: u32 = 0; macro_rules! add_entry { // Adds entry which goes from $src_node_id to $dest_node_id over the $candidate hop. From fb2c959b40317505383e361997f3cde3f6b5d943 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Wed, 27 Sep 2023 09:57:20 +0200 Subject: [PATCH 6/8] Refactor `mpp_retry[_overpay]` tests to use `SendEvent::from_event` .. as a follow-up from #2417. --- lightning/src/ln/payment_tests.rs | 38 +++++-------------------------- 1 file changed, 6 insertions(+), 32 deletions(-) diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 22d85ecc76c..0af1d69f2aa 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -114,19 +114,9 @@ fn mpp_retry() { // Add the HTLC along the first hop. let fail_path_msgs_1 = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &mut events); - let (update_add, commitment_signed) = match fail_path_msgs_1 { - MessageSendEvent::UpdateHTLCs { node_id: _, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, ref update_fail_malformed_htlcs, ref update_fee, ref commitment_signed } } => { - assert_eq!(update_add_htlcs.len(), 1); - assert!(update_fail_htlcs.is_empty()); - assert!(update_fulfill_htlcs.is_empty()); - assert!(update_fail_malformed_htlcs.is_empty()); - assert!(update_fee.is_none()); - (update_add_htlcs[0].clone(), commitment_signed.clone()) - }, - _ => panic!("Unexpected event"), - }; - nodes[2].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &update_add); - commitment_signed_dance!(nodes[2], nodes[0], commitment_signed, false); + let send_event = SendEvent::from_event(fail_path_msgs_1); + nodes[2].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &send_event.msgs[0]); + commitment_signed_dance!(nodes[2], nodes[0], &send_event.commitment_msg, false); // Attempt to forward the payment and complete the 2nd path's failure. expect_pending_htlcs_forwardable!(&nodes[2]); @@ -225,25 +215,9 @@ fn mpp_retry_overpay() { // Add the HTLC along the first hop. let fail_path_msgs_1 = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &mut events); - let (update_add, commitment_signed) = match fail_path_msgs_1 { - MessageSendEvent::UpdateHTLCs { - node_id: _, - updates: msgs::CommitmentUpdate { - ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, - ref update_fail_malformed_htlcs, ref update_fee, ref commitment_signed - } - } => { - assert_eq!(update_add_htlcs.len(), 1); - assert!(update_fail_htlcs.is_empty()); - assert!(update_fulfill_htlcs.is_empty()); - assert!(update_fail_malformed_htlcs.is_empty()); - assert!(update_fee.is_none()); - (update_add_htlcs[0].clone(), commitment_signed.clone()) - }, - _ => panic!("Unexpected event"), - }; - nodes[2].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &update_add); - commitment_signed_dance!(nodes[2], nodes[0], commitment_signed, false); + let send_event = SendEvent::from_event(fail_path_msgs_1); + nodes[2].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &send_event.msgs[0]); + commitment_signed_dance!(nodes[2], nodes[0], &send_event.commitment_msg, false); // Attempt to forward the payment and complete the 2nd path's failure. expect_pending_htlcs_forwardable!(&nodes[2]); From 8c99e34b9321db8ba22eb6b1836b436bbb652271 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 28 Sep 2023 10:04:35 +0200 Subject: [PATCH 7/8] Assert query's and route's `final_value_msat` are equal --- lightning/src/ln/outbound_payment.rs | 21 ++++++++++- lightning/src/ln/payment_tests.rs | 56 ++++++++++++++-------------- lightning/src/util/test_utils.rs | 1 + 3 files changed, 49 insertions(+), 29 deletions(-) diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 025a197348a..4cc8d85ad19 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -875,7 +875,7 @@ impl OutboundPayments { } } - let route = router.find_route_with_id( + let mut route = router.find_route_with_id( &node_signer.get_node_id(Recipient::Node).unwrap(), &route_params, Some(&first_hops.iter().collect::>()), inflight_htlcs(), payment_hash, payment_id, @@ -885,6 +885,14 @@ impl OutboundPayments { RetryableSendFailure::RouteNotFound })?; + if let Some(route_route_params) = route.route_params.as_mut() { + if route_route_params.final_value_msat != route_params.final_value_msat { + debug_assert!(false, + "Routers are expected to return a route which includes the requested final_value_msat"); + route_route_params.final_value_msat = route_params.final_value_msat; + } + } + let onion_session_privs = self.add_new_pending_payment(payment_hash, recipient_onion.clone(), payment_id, keysend_preimage, &route, Some(retry_strategy), Some(route_params.payment_params.clone()), entropy_source, best_block_height) @@ -926,7 +934,7 @@ impl OutboundPayments { } } - let route = match router.find_route_with_id( + let mut route = match router.find_route_with_id( &node_signer.get_node_id(Recipient::Node).unwrap(), &route_params, Some(&first_hops.iter().collect::>()), inflight_htlcs(), payment_hash, payment_id, @@ -938,6 +946,15 @@ impl OutboundPayments { return } }; + + if let Some(route_route_params) = route.route_params.as_mut() { + if route_route_params.final_value_msat != route_params.final_value_msat { + debug_assert!(false, + "Routers are expected to return a route which includes the requested final_value_msat"); + route_route_params.final_value_msat = route_params.final_value_msat; + } + } + for path in route.paths.iter() { if path.hops.len() == 0 { log_error!(logger, "Unusable path in route (path.hops.len() must be at least 1"); diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 0af1d69f2aa..e48ae7bd8dc 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -253,6 +253,7 @@ fn mpp_retry_overpay() { route.paths.remove(0); route_params.final_value_msat -= first_path_value; + route.route_params.as_mut().map(|p| p.final_value_msat -= first_path_value); route_params.payment_params.previously_failed_channels.push(chan_4_update.contents.short_channel_id); // Check the remaining max total routing fee for the second attempt accounts only for 1_000 msat @@ -2395,10 +2396,11 @@ fn auto_retry_partial_failure() { let payment_params = PaymentParameters::from_node_id(nodes[1].node.get_our_node_id(), TEST_FINAL_CLTV) .with_expiry_time(payment_expiry_secs as u64) .with_bolt11_features(invoice_features).unwrap(); + + // Configure the initial send path let mut route_params = RouteParameters::from_payment_params_and_value(payment_params, amt_msat); route_params.max_total_routing_fee_msat = None; - // Configure the initial send, retry1 and retry2's paths. let send_route = Route { paths: vec![ Path { hops: vec![RouteHop { @@ -2422,6 +2424,14 @@ fn auto_retry_partial_failure() { ], route_params: Some(route_params.clone()), }; + nodes[0].router.expect_find_route(route_params.clone(), Ok(send_route)); + + // Configure the retry1 paths + let mut payment_params = route_params.payment_params.clone(); + payment_params.previously_failed_channels.push(chan_2_id); + let mut retry_1_params = RouteParameters::from_payment_params_and_value(payment_params, amt_msat / 2); + retry_1_params.max_total_routing_fee_msat = None; + let retry_1_route = Route { paths: vec![ Path { hops: vec![RouteHop { @@ -2443,8 +2453,16 @@ fn auto_retry_partial_failure() { maybe_announced_channel: true, }], blinded_tail: None }, ], - route_params: Some(route_params.clone()), + route_params: Some(retry_1_params.clone()), }; + nodes[0].router.expect_find_route(retry_1_params.clone(), Ok(retry_1_route)); + + // Configure the retry2 path + let mut payment_params = retry_1_params.payment_params.clone(); + payment_params.previously_failed_channels.push(chan_3_id); + let mut retry_2_params = RouteParameters::from_payment_params_and_value(payment_params, amt_msat / 4); + retry_2_params.max_total_routing_fee_msat = None; + let retry_2_route = Route { paths: vec![ Path { hops: vec![RouteHop { @@ -2457,20 +2475,8 @@ fn auto_retry_partial_failure() { maybe_announced_channel: true, }], blinded_tail: None }, ], - route_params: Some(route_params.clone()), + route_params: Some(retry_2_params.clone()), }; - nodes[0].router.expect_find_route(route_params.clone(), Ok(send_route)); - let mut payment_params = route_params.payment_params.clone(); - payment_params.previously_failed_channels.push(chan_2_id); - - let mut retry_1_params = RouteParameters::from_payment_params_and_value(payment_params, amt_msat / 2); - retry_1_params.max_total_routing_fee_msat = None; - nodes[0].router.expect_find_route(retry_1_params, Ok(retry_1_route)); - - let mut payment_params = route_params.payment_params.clone(); - payment_params.previously_failed_channels.push(chan_3_id); - let mut retry_2_params = RouteParameters::from_payment_params_and_value(payment_params, amt_msat / 4); - retry_2_params.max_total_routing_fee_msat = None; nodes[0].router.expect_find_route(retry_2_params, Ok(retry_2_route)); // Send a payment that will partially fail on send, then partially fail on retry, then succeed. @@ -2734,6 +2740,7 @@ fn retry_multi_path_single_failed_payment() { // not the amount remaining on the full payment, which should be changed. let mut retry_params = RouteParameters::from_payment_params_and_value(pay_params, 100_000_001); retry_params.max_total_routing_fee_msat = None; + route.route_params.as_mut().unwrap().final_value_msat = 100_000_000; nodes[0].router.expect_find_route(retry_params, Ok(route.clone())); { @@ -2917,9 +2924,7 @@ fn no_extra_retries_on_back_to_back_fail() { maybe_announced_channel: true, }], blinded_tail: None } ], - route_params: Some(RouteParameters::from_payment_params_and_value( - PaymentParameters::from_node_id(nodes[2].node.get_our_node_id(), TEST_FINAL_CLTV), - 100_000_000)), + route_params: Some(route_params.clone()), }; route.route_params.as_mut().unwrap().max_total_routing_fee_msat = None; nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone())); @@ -3123,18 +3128,18 @@ fn test_simple_partial_retry() { maybe_announced_channel: true, }], blinded_tail: None } ], - route_params: Some(RouteParameters::from_payment_params_and_value( - PaymentParameters::from_node_id(nodes[2].node.get_our_node_id(), TEST_FINAL_CLTV), - 100_000_000)), + route_params: Some(route_params.clone()), }; - route.route_params.as_mut().unwrap().max_total_routing_fee_msat = None; + nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone())); + let mut second_payment_params = route_params.payment_params.clone(); second_payment_params.previously_failed_channels = vec![chan_2_scid]; // On retry, we'll only be asked for one path (or 100k sats) route.paths.remove(0); let mut retry_params = RouteParameters::from_payment_params_and_value(second_payment_params, amt_msat / 2); retry_params.max_total_routing_fee_msat = None; + route.route_params.as_mut().unwrap().final_value_msat = amt_msat / 2; nodes[0].router.expect_find_route(retry_params, Ok(route.clone())); nodes[0].node.send_payment(payment_hash, RecipientOnionFields::secret_only(payment_secret), @@ -3294,11 +3299,7 @@ fn test_threaded_payment_retries() { maybe_announced_channel: true, }], blinded_tail: None } ], - route_params: Some(RouteParameters { - payment_params: PaymentParameters::from_node_id(nodes[1].node.get_our_node_id(), TEST_FINAL_CLTV), - final_value_msat: amt_msat - amt_msat / 1000, - max_total_routing_fee_msat: Some(500_000), - }), + route_params: Some(route_params.clone()), }; nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone())); @@ -3317,6 +3318,7 @@ fn test_threaded_payment_retries() { // from here on out, the retry `RouteParameters` amount will be amt/1000 route_params.final_value_msat /= 1000; + route.route_params.as_mut().unwrap().final_value_msat /= 1000; route.paths.pop(); let end_time = Instant::now() + Duration::from_secs(1); diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index d53cd39b119..50467a1735f 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -124,6 +124,7 @@ impl<'a> Router for TestRouter<'a> { if let Some((find_route_query, find_route_res)) = self.next_routes.lock().unwrap().pop_front() { assert_eq!(find_route_query, *params); if let Ok(ref route) = find_route_res { + assert_eq!(route.route_params.as_ref().unwrap().final_value_msat, find_route_query.final_value_msat); let scorer = self.scorer.read().unwrap(); let scorer = ScorerAccountingForInFlightHtlcs::new(scorer, &inflight_htlcs); for path in &route.paths { From be1088ac080f05398ed009b1bee2730a4371341f Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Wed, 27 Sep 2023 12:43:31 +0200 Subject: [PATCH 8/8] Don't retry overpaid values for `PartialFailure`s Previously, if an overpaid path would fail immediately, we'd retry a `PartialFailure` with the full path amount, _including_ any overpayment. Here, we now subtract the succeeded paths' values from the net. value to exclude the overpaid amounts on retry. --- lightning/src/ln/outbound_payment.rs | 14 ++++++++++---- lightning/src/ln/payment_tests.rs | 4 +--- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 4cc8d85ad19..2522f99fbe8 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -1354,12 +1354,14 @@ impl OutboundPayments { } let mut has_ok = false; let mut has_err = false; - let mut pending_amt_unsent = 0; + let mut has_unsent = false; let mut total_ok_fees_msat = 0; + let mut total_ok_amt_sent_msat = 0; for (res, path) in results.iter().zip(route.paths.iter()) { if res.is_ok() { has_ok = true; total_ok_fees_msat += path.fee_msat(); + total_ok_amt_sent_msat += path.final_value_msat(); } if res.is_err() { has_err = true; } if let &Err(APIError::MonitorUpdateInProgress) = res { @@ -1368,23 +1370,27 @@ impl OutboundPayments { has_err = true; has_ok = true; total_ok_fees_msat += path.fee_msat(); + total_ok_amt_sent_msat += path.final_value_msat(); } else if res.is_err() { - pending_amt_unsent += path.final_value_msat(); + has_unsent = true; } } if has_err && has_ok { Err(PaymentSendFailure::PartialFailure { results, payment_id, - failed_paths_retry: if pending_amt_unsent != 0 { + failed_paths_retry: if has_unsent { if let Some(route_params) = &route.route_params { let mut route_params = route_params.clone(); // We calculate the leftover fee budget we're allowed to spend by // subtracting the used fee from the total fee budget. route_params.max_total_routing_fee_msat = route_params .max_total_routing_fee_msat.map(|m| m.saturating_sub(total_ok_fees_msat)); - route_params.final_value_msat = pending_amt_unsent; + // We calculate the remaining target amount by subtracting the succeded + // path values. + route_params.final_value_msat = route_params.final_value_msat + .saturating_sub(total_ok_amt_sent_msat); Some(route_params) } else { None } } else { None }, diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index e48ae7bd8dc..72176760243 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -2736,9 +2736,7 @@ fn retry_multi_path_single_failed_payment() { let mut pay_params = route.route_params.clone().unwrap().payment_params; pay_params.previously_failed_channels.push(chans[1].short_channel_id.unwrap()); - // Note that the second request here requests the amount we originally failed to send, - // not the amount remaining on the full payment, which should be changed. - let mut retry_params = RouteParameters::from_payment_params_and_value(pay_params, 100_000_001); + let mut retry_params = RouteParameters::from_payment_params_and_value(pay_params, 100_000_000); retry_params.max_total_routing_fee_msat = None; route.route_params.as_mut().unwrap().final_value_msat = 100_000_000; nodes[0].router.expect_find_route(retry_params, Ok(route.clone()));