diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 025a197348a..2522f99fbe8 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"); @@ -1337,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 { @@ -1351,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 22d85ecc76c..72176760243 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]); @@ -279,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 @@ -2421,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 { @@ -2448,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 { @@ -2469,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 { @@ -2483,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. @@ -2756,10 +2736,9 @@ 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())); { @@ -2943,9 +2922,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())); @@ -3149,18 +3126,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), @@ -3320,11 +3297,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())); @@ -3343,6 +3316,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/routing/router.rs b/lightning/src/routing/router.rs index 09d83bb785c..193fe352334 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 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). 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 {