Set a default `max_total_routing_fee_msat` of 1% + 50sats

When using the normal default constructors, we should have some
fee maximum to ensure our default behavior is safe. Here we pick
1% + 50 sats to ensure we're always willing to pay
reasonabl(y high) fees, but not anything too wild.

diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs
index cdf5627a7aac6013371cd2b55e3cf14224a8423a..025a197348ad18c616ffc0ea114ecaa6c3f116f5 100644 (file)
--- 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 21c4881ab1a08295cf7989ea99d3a47561f9656f..22d85ecc76c3af0a5bbb8d5df0a74fb30db6fe0e 100644 (file)
--- 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 7862baf6322d7fd004c1cf7b3652beb35a336f3c..92e5dadf979e391371d8dd05d4a5258c73238184 100644 (file)
--- a/lightning/src/routing/router.rs
+++ b/lightning/src/routing/router.rs
@@ -481,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<u64>,
 }
 
 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) }
        }
 }
 
@@ -3098,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.
@@ -5757,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);
@@ -5826,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);
@@ -6532,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)
@@ -6547,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);
@@ -6983,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);