diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index cdf5627a7aa..025a197348a 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -2084,11 +2084,6 @@ mod tests { let outbound_payments = OutboundPayments::new(); let payment_id = PaymentId([0; 32]); - assert!( - outbound_payments.add_new_awaiting_invoice(payment_id, Retry::Attempts(0), None).is_ok() - ); - assert!(outbound_payments.has_pending_payments()); - let invoice = OfferBuilder::new("foo".into(), recipient_pubkey()) .amount_msats(1000) .build().unwrap() @@ -2099,6 +2094,12 @@ mod tests { .build().unwrap() .sign(recipient_sign).unwrap(); + assert!(outbound_payments.add_new_awaiting_invoice( + payment_id, Retry::Attempts(0), Some(invoice.amount_msats() / 100 + 50_000)) + .is_ok() + ); + assert!(outbound_payments.has_pending_payments()); + router.expect_find_route( RouteParameters::from_payment_params_and_value( PaymentParameters::from_bolt12_invoice(&invoice), @@ -2139,11 +2140,6 @@ mod tests { let outbound_payments = OutboundPayments::new(); let payment_id = PaymentId([0; 32]); - assert!( - outbound_payments.add_new_awaiting_invoice(payment_id, Retry::Attempts(0), None).is_ok() - ); - assert!(outbound_payments.has_pending_payments()); - let invoice = OfferBuilder::new("foo".into(), recipient_pubkey()) .amount_msats(1000) .build().unwrap() @@ -2154,6 +2150,12 @@ mod tests { .build().unwrap() .sign(recipient_sign).unwrap(); + assert!(outbound_payments.add_new_awaiting_invoice( + payment_id, Retry::Attempts(0), Some(invoice.amount_msats() / 100 + 50_000)) + .is_ok() + ); + assert!(outbound_payments.has_pending_payments()); + let route_params = RouteParameters::from_payment_params_and_value( PaymentParameters::from_bolt12_invoice(&invoice), invoice.amount_msats(), diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 21c4881ab1a..22d85ecc76c 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -2421,7 +2421,8 @@ 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(); - let route_params = RouteParameters::from_payment_params_and_value(payment_params, amt_msat); + 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 { @@ -2487,14 +2488,16 @@ fn auto_retry_partial_failure() { 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); - nodes[0].router.expect_find_route( - RouteParameters::from_payment_params_and_value(payment_params, amt_msat / 2), - Ok(retry_1_route)); + + 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); - nodes[0].router.expect_find_route( - RouteParameters::from_payment_params_and_value(payment_params, amt_msat / 4), - Ok(retry_2_route)); + 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. nodes[0].node.send_payment(payment_hash, RecipientOnionFields::secret_only(payment_secret), @@ -2718,8 +2721,9 @@ fn retry_multi_path_single_failed_payment() { 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(); - let route_params = RouteParameters::from_payment_params_and_value( + let mut route_params = RouteParameters::from_payment_params_and_value( payment_params.clone(), amt_msat); + route_params.max_total_routing_fee_msat = None; let chans = nodes[0].node.list_usable_channels(); let mut route = Route { @@ -2751,11 +2755,12 @@ fn retry_multi_path_single_failed_payment() { route.paths[1].hops[0].fee_msat = 50_000_000; let mut pay_params = route.route_params.clone().unwrap().payment_params; pay_params.previously_failed_channels.push(chans[1].short_channel_id.unwrap()); - nodes[0].router.expect_find_route( - // 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. - RouteParameters::from_payment_params_and_value(pay_params, 100_000_001), - Ok(route.clone())); + + // 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); + retry_params.max_total_routing_fee_msat = None; + nodes[0].router.expect_find_route(retry_params, Ok(route.clone())); { let scorer = chanmon_cfgs[0].scorer.read().unwrap(); @@ -2898,7 +2903,8 @@ fn no_extra_retries_on_back_to_back_fail() { 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(); - let route_params = RouteParameters::from_payment_params_and_value(payment_params, amt_msat); + let mut route_params = RouteParameters::from_payment_params_and_value(payment_params, amt_msat); + route_params.max_total_routing_fee_msat = None; let mut route = Route { paths: vec![ @@ -2941,15 +2947,16 @@ fn no_extra_retries_on_back_to_back_fail() { PaymentParameters::from_node_id(nodes[2].node.get_our_node_id(), TEST_FINAL_CLTV), 100_000_000)), }; + 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, chan_2_scid]; // On retry, we'll only return one path route.paths.remove(1); route.paths[0].hops[1].fee_msat = amt_msat; - nodes[0].router.expect_find_route( - RouteParameters::from_payment_params_and_value(second_payment_params, amt_msat), - Ok(route.clone())); + let mut retry_params = RouteParameters::from_payment_params_and_value(second_payment_params, amt_msat); + retry_params.max_total_routing_fee_msat = None; + nodes[0].router.expect_find_route(retry_params, Ok(route.clone())); nodes[0].node.send_payment(payment_hash, RecipientOnionFields::secret_only(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap(); @@ -3102,7 +3109,8 @@ fn test_simple_partial_retry() { 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(); - let route_params = RouteParameters::from_payment_params_and_value(payment_params, amt_msat); + let mut route_params = RouteParameters::from_payment_params_and_value(payment_params, amt_msat); + route_params.max_total_routing_fee_msat = None; let mut route = Route { paths: vec![ @@ -3145,14 +3153,15 @@ fn test_simple_partial_retry() { PaymentParameters::from_node_id(nodes[2].node.get_our_node_id(), TEST_FINAL_CLTV), 100_000_000)), }; + 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); - nodes[0].router.expect_find_route( - RouteParameters::from_payment_params_and_value(second_payment_params, amt_msat / 2), - Ok(route.clone())); + let mut retry_params = RouteParameters::from_payment_params_and_value(second_payment_params, amt_msat / 2); + retry_params.max_total_routing_fee_msat = None; + nodes[0].router.expect_find_route(retry_params, Ok(route.clone())); nodes[0].node.send_payment(payment_hash, RecipientOnionFields::secret_only(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap(); diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index c20ce2e97ee..92e5dadf979 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -341,7 +341,7 @@ impl Path { /// A route directs a payment from the sender (us) to the recipient. If the recipient supports MPP, /// it can take multiple paths. Each path is composed of one or more hops through the network. -#[derive(Clone, Hash, PartialEq, Eq)] +#[derive(Clone, Debug, Hash, PartialEq, Eq)] pub struct Route { /// The list of [`Path`]s taken for a single (potentially-)multi-part payment. If no /// [`BlindedTail`]s are present, then the pubkey of the last [`RouteHop`] in each path must be @@ -380,6 +380,12 @@ impl Route { } } +impl fmt::Display for Route { + fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> { + log_route!(self).fmt(f) + } +} + const SERIALIZATION_VERSION: u8 = 1; const MIN_SERIALIZATION_VERSION: u8 = 1; @@ -475,14 +481,16 @@ pub struct RouteParameters { /// This limit also applies to the total fees that may arise while retrying failed payment /// paths. /// - /// Default value: `None` + /// Note that values below a few sats may result in some paths being spuriously ignored. pub max_total_routing_fee_msat: Option, } impl RouteParameters { /// Constructs [`RouteParameters`] from the given [`PaymentParameters`] and a payment amount. + /// + /// [`Self::max_total_routing_fee_msat`] defaults to 1% of the payment amount + 50 sats pub fn from_payment_params_and_value(payment_params: PaymentParameters, final_value_msat: u64) -> Self { - Self { payment_params, final_value_msat, max_total_routing_fee_msat: None } + Self { payment_params, final_value_msat, max_total_routing_fee_msat: Some(final_value_msat / 100 + 50_000) } } } @@ -3092,8 +3100,9 @@ mod tests { excess_data: Vec::new() }); - let route_params = RouteParameters::from_payment_params_and_value( + let mut route_params = RouteParameters::from_payment_params_and_value( payment_params.clone(), 60_000); + route_params.max_total_routing_fee_msat = Some(15_000); let route = get_route(&our_id, &route_params, &network_graph.read_only(), None, Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes).unwrap(); // Overpay fees to hit htlc_minimum_msat. @@ -5751,8 +5760,9 @@ mod tests { { // 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_params = RouteParameters::from_payment_params_and_value( + let mut route_params = RouteParameters::from_payment_params_and_value( payment_params, 90_000); + route_params.max_total_routing_fee_msat = Some(90_000*2); 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.paths.len(), 1); @@ -5820,8 +5830,9 @@ mod tests { // 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_params = RouteParameters::from_payment_params_and_value( + let mut route_params = RouteParameters::from_payment_params_and_value( payment_params, 90_000); + route_params.max_total_routing_fee_msat = Some(90_000*2); 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.paths.len(), 1); @@ -6526,8 +6537,9 @@ mod tests { // Make sure we'll error if our route hints don't have enough liquidity according to their // htlc_maximum_msat. - let route_params = RouteParameters::from_payment_params_and_value( + let mut route_params = RouteParameters::from_payment_params_and_value( payment_params, max_htlc_msat + 1); + route_params.max_total_routing_fee_msat = None; if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route(&our_id, &route_params, &netgraph, None, Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes) @@ -6541,8 +6553,9 @@ mod tests { let payment_params = PaymentParameters::from_node_id(dest_node_id, 42) .with_route_hints(vec![route_hint_1, route_hint_2]).unwrap() .with_bolt11_features(channelmanager::provided_invoice_features(&config)).unwrap(); - let route_params = RouteParameters::from_payment_params_and_value( + let mut route_params = RouteParameters::from_payment_params_and_value( payment_params, max_htlc_msat + 1); + route_params.max_total_routing_fee_msat = Some(max_htlc_msat * 2); let route = get_route(&our_id, &route_params, &netgraph, None, Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes).unwrap(); assert_eq!(route.paths.len(), 2); @@ -6977,7 +6990,8 @@ mod tests { let payment_params = PaymentParameters::blinded(blinded_hints.clone()) .with_bolt12_features(bolt12_features.clone()).unwrap(); - let route_params = RouteParameters::from_payment_params_and_value(payment_params, 100_000); + let mut route_params = RouteParameters::from_payment_params_and_value(payment_params, 100_000); + route_params.max_total_routing_fee_msat = Some(100_000); let route = get_route(&our_id, &route_params, &network_graph, None, Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes).unwrap(); assert_eq!(route.paths.len(), 2);