From 0c31c6f162df1ae280fd232e26d0c1f173959ee6 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 26 Sep 2023 21:28:29 +0000 Subject: [PATCH] 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. --- lightning/src/ln/outbound_payment.rs | 22 ++++++------ lightning/src/ln/payment_tests.rs | 51 ++++++++++++++++------------ lightning/src/routing/router.rs | 24 ++++++++----- 3 files changed, 58 insertions(+), 39 deletions(-) diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index cdf5627a7..025a19734 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 21c4881ab..22d85ecc7 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 7862baf63..92e5dadf9 100644 --- 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, } 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); -- 2.39.5