Skip to content

Commit 0c31c6f

Browse files
TheBlueMattTheBlueMatt
committed
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.
1 parent 793a1bf commit 0c31c6f

File tree

3 files changed

+58
-39
lines changed

3 files changed

+58
-39
lines changed

‎lightning/src/ln/outbound_payment.rs

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2084,11 +2084,6 @@ mod tests {
20842084
let outbound_payments = OutboundPayments::new();
20852085
let payment_id = PaymentId([0; 32]);
20862086

2087-
assert!(
2088-
outbound_payments.add_new_awaiting_invoice(payment_id, Retry::Attempts(0), None).is_ok()
2089-
);
2090-
assert!(outbound_payments.has_pending_payments());
2091-
20922087
let invoice = OfferBuilder::new("foo".into(), recipient_pubkey())
20932088
.amount_msats(1000)
20942089
.build().unwrap()
@@ -2099,6 +2094,12 @@ mod tests {
20992094
.build().unwrap()
21002095
.sign(recipient_sign).unwrap();
21012096

2097+
assert!(outbound_payments.add_new_awaiting_invoice(
2098+
payment_id, Retry::Attempts(0), Some(invoice.amount_msats() / 100 + 50_000))
2099+
.is_ok()
2100+
);
2101+
assert!(outbound_payments.has_pending_payments());
2102+
21022103
router.expect_find_route(
21032104
RouteParameters::from_payment_params_and_value(
21042105
PaymentParameters::from_bolt12_invoice(&invoice),
@@ -2139,11 +2140,6 @@ mod tests {
21392140
let outbound_payments = OutboundPayments::new();
21402141
let payment_id = PaymentId([0; 32]);
21412142

2142-
assert!(
2143-
outbound_payments.add_new_awaiting_invoice(payment_id, Retry::Attempts(0), None).is_ok()
2144-
);
2145-
assert!(outbound_payments.has_pending_payments());
2146-
21472143
let invoice = OfferBuilder::new("foo".into(), recipient_pubkey())
21482144
.amount_msats(1000)
21492145
.build().unwrap()
@@ -2154,6 +2150,12 @@ mod tests {
21542150
.build().unwrap()
21552151
.sign(recipient_sign).unwrap();
21562152

2153+
assert!(outbound_payments.add_new_awaiting_invoice(
2154+
payment_id, Retry::Attempts(0), Some(invoice.amount_msats() / 100 + 50_000))
2155+
.is_ok()
2156+
);
2157+
assert!(outbound_payments.has_pending_payments());
2158+
21572159
let route_params = RouteParameters::from_payment_params_and_value(
21582160
PaymentParameters::from_bolt12_invoice(&invoice),
21592161
invoice.amount_msats(),

‎lightning/src/ln/payment_tests.rs

Lines changed: 30 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -2421,7 +2421,8 @@ fn auto_retry_partial_failure() {
24212421
let payment_params = PaymentParameters::from_node_id(nodes[1].node.get_our_node_id(), TEST_FINAL_CLTV)
24222422
.with_expiry_time(payment_expiry_secs as u64)
24232423
.with_bolt11_features(invoice_features).unwrap();
2424-
let route_params = RouteParameters::from_payment_params_and_value(payment_params, amt_msat);
2424+
let mut route_params = RouteParameters::from_payment_params_and_value(payment_params, amt_msat);
2425+
route_params.max_total_routing_fee_msat = None;
24252426

24262427
// Configure the initial send, retry1 and retry2's paths.
24272428
let send_route = Route {
@@ -2487,14 +2488,16 @@ fn auto_retry_partial_failure() {
24872488
nodes[0].router.expect_find_route(route_params.clone(), Ok(send_route));
24882489
let mut payment_params = route_params.payment_params.clone();
24892490
payment_params.previously_failed_channels.push(chan_2_id);
2490-
nodes[0].router.expect_find_route(
2491-
RouteParameters::from_payment_params_and_value(payment_params, amt_msat / 2),
2492-
Ok(retry_1_route));
2491+
2492+
let mut retry_1_params = RouteParameters::from_payment_params_and_value(payment_params, amt_msat / 2);
2493+
retry_1_params.max_total_routing_fee_msat = None;
2494+
nodes[0].router.expect_find_route(retry_1_params, Ok(retry_1_route));
2495+
24932496
let mut payment_params = route_params.payment_params.clone();
24942497
payment_params.previously_failed_channels.push(chan_3_id);
2495-
nodes[0].router.expect_find_route(
2496-
RouteParameters::from_payment_params_and_value(payment_params, amt_msat / 4),
2497-
Ok(retry_2_route));
2498+
let mut retry_2_params = RouteParameters::from_payment_params_and_value(payment_params, amt_msat / 4);
2499+
retry_2_params.max_total_routing_fee_msat = None;
2500+
nodes[0].router.expect_find_route(retry_2_params, Ok(retry_2_route));
24982501

24992502
// Send a payment that will partially fail on send, then partially fail on retry, then succeed.
25002503
nodes[0].node.send_payment(payment_hash, RecipientOnionFields::secret_only(payment_secret),
@@ -2718,8 +2721,9 @@ fn retry_multi_path_single_failed_payment() {
27182721
let payment_params = PaymentParameters::from_node_id(nodes[1].node.get_our_node_id(), TEST_FINAL_CLTV)
27192722
.with_expiry_time(payment_expiry_secs as u64)
27202723
.with_bolt11_features(invoice_features).unwrap();
2721-
let route_params = RouteParameters::from_payment_params_and_value(
2724+
let mut route_params = RouteParameters::from_payment_params_and_value(
27222725
payment_params.clone(), amt_msat);
2726+
route_params.max_total_routing_fee_msat = None;
27232727

27242728
let chans = nodes[0].node.list_usable_channels();
27252729
let mut route = Route {
@@ -2751,11 +2755,12 @@ fn retry_multi_path_single_failed_payment() {
27512755
route.paths[1].hops[0].fee_msat = 50_000_000;
27522756
let mut pay_params = route.route_params.clone().unwrap().payment_params;
27532757
pay_params.previously_failed_channels.push(chans[1].short_channel_id.unwrap());
2754-
nodes[0].router.expect_find_route(
2755-
// Note that the second request here requests the amount we originally failed to send,
2756-
// not the amount remaining on the full payment, which should be changed.
2757-
RouteParameters::from_payment_params_and_value(pay_params, 100_000_001),
2758-
Ok(route.clone()));
2758+
2759+
// Note that the second request here requests the amount we originally failed to send,
2760+
// not the amount remaining on the full payment, which should be changed.
2761+
let mut retry_params = RouteParameters::from_payment_params_and_value(pay_params, 100_000_001);
2762+
retry_params.max_total_routing_fee_msat = None;
2763+
nodes[0].router.expect_find_route(retry_params, Ok(route.clone()));
27592764

27602765
{
27612766
let scorer = chanmon_cfgs[0].scorer.read().unwrap();
@@ -2898,7 +2903,8 @@ fn no_extra_retries_on_back_to_back_fail() {
28982903
let payment_params = PaymentParameters::from_node_id(nodes[1].node.get_our_node_id(), TEST_FINAL_CLTV)
28992904
.with_expiry_time(payment_expiry_secs as u64)
29002905
.with_bolt11_features(invoice_features).unwrap();
2901-
let route_params = RouteParameters::from_payment_params_and_value(payment_params, amt_msat);
2906+
let mut route_params = RouteParameters::from_payment_params_and_value(payment_params, amt_msat);
2907+
route_params.max_total_routing_fee_msat = None;
29022908

29032909
let mut route = Route {
29042910
paths: vec![
@@ -2941,15 +2947,16 @@ fn no_extra_retries_on_back_to_back_fail() {
29412947
PaymentParameters::from_node_id(nodes[2].node.get_our_node_id(), TEST_FINAL_CLTV),
29422948
100_000_000)),
29432949
};
2950+
route.route_params.as_mut().unwrap().max_total_routing_fee_msat = None;
29442951
nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone()));
29452952
let mut second_payment_params = route_params.payment_params.clone();
29462953
second_payment_params.previously_failed_channels = vec![chan_2_scid, chan_2_scid];
29472954
// On retry, we'll only return one path
29482955
route.paths.remove(1);
29492956
route.paths[0].hops[1].fee_msat = amt_msat;
2950-
nodes[0].router.expect_find_route(
2951-
RouteParameters::from_payment_params_and_value(second_payment_params, amt_msat),
2952-
Ok(route.clone()));
2957+
let mut retry_params = RouteParameters::from_payment_params_and_value(second_payment_params, amt_msat);
2958+
retry_params.max_total_routing_fee_msat = None;
2959+
nodes[0].router.expect_find_route(retry_params, Ok(route.clone()));
29532960

29542961
nodes[0].node.send_payment(payment_hash, RecipientOnionFields::secret_only(payment_secret),
29552962
PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
@@ -3102,7 +3109,8 @@ fn test_simple_partial_retry() {
31023109
let payment_params = PaymentParameters::from_node_id(nodes[1].node.get_our_node_id(), TEST_FINAL_CLTV)
31033110
.with_expiry_time(payment_expiry_secs as u64)
31043111
.with_bolt11_features(invoice_features).unwrap();
3105-
let route_params = RouteParameters::from_payment_params_and_value(payment_params, amt_msat);
3112+
let mut route_params = RouteParameters::from_payment_params_and_value(payment_params, amt_msat);
3113+
route_params.max_total_routing_fee_msat = None;
31063114

31073115
let mut route = Route {
31083116
paths: vec![
@@ -3145,14 +3153,15 @@ fn test_simple_partial_retry() {
31453153
PaymentParameters::from_node_id(nodes[2].node.get_our_node_id(), TEST_FINAL_CLTV),
31463154
100_000_000)),
31473155
};
3156+
route.route_params.as_mut().unwrap().max_total_routing_fee_msat = None;
31483157
nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone()));
31493158
let mut second_payment_params = route_params.payment_params.clone();
31503159
second_payment_params.previously_failed_channels = vec![chan_2_scid];
31513160
// On retry, we'll only be asked for one path (or 100k sats)
31523161
route.paths.remove(0);
3153-
nodes[0].router.expect_find_route(
3154-
RouteParameters::from_payment_params_and_value(second_payment_params, amt_msat / 2),
3155-
Ok(route.clone()));
3162+
let mut retry_params = RouteParameters::from_payment_params_and_value(second_payment_params, amt_msat / 2);
3163+
retry_params.max_total_routing_fee_msat = None;
3164+
nodes[0].router.expect_find_route(retry_params, Ok(route.clone()));
31563165

31573166
nodes[0].node.send_payment(payment_hash, RecipientOnionFields::secret_only(payment_secret),
31583167
PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();

‎lightning/src/routing/router.rs

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -481,14 +481,16 @@ pub struct RouteParameters {
481481
/// This limit also applies to the total fees that may arise while retrying failed payment
482482
/// paths.
483483
///
484-
/// Default value: `None`
484+
/// Note that values below a few sats may result in some paths being spuriously ignored.
485485
pub max_total_routing_fee_msat: Option<u64>,
486486
}
487487

488488
impl RouteParameters {
489489
/// Constructs [`RouteParameters`] from the given [`PaymentParameters`] and a payment amount.
490+
///
491+
/// [`Self::max_total_routing_fee_msat`] defaults to 1% of the payment amount + 50 sats
490492
pub fn from_payment_params_and_value(payment_params: PaymentParameters, final_value_msat: u64) -> Self {
491-
Self { payment_params, final_value_msat, max_total_routing_fee_msat: None }
493+
Self { payment_params, final_value_msat, max_total_routing_fee_msat: Some(final_value_msat / 100 + 50_000) }
492494
}
493495
}
494496

@@ -3098,8 +3100,9 @@ mod tests {
30983100
excess_data: Vec::new()
30993101
});
31003102

3101-
let route_params = RouteParameters::from_payment_params_and_value(
3103+
let mut route_params = RouteParameters::from_payment_params_and_value(
31023104
payment_params.clone(), 60_000);
3105+
route_params.max_total_routing_fee_msat = Some(15_000);
31033106
let route = get_route(&our_id, &route_params, &network_graph.read_only(), None,
31043107
Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes).unwrap();
31053108
// Overpay fees to hit htlc_minimum_msat.
@@ -5757,8 +5760,9 @@ mod tests {
57575760
{
57585761
// Now, attempt to route 90 sats, which is exactly 90 sats at the last hop, plus the
57595762
// 200% fee charged channel 13 in the 1-to-2 direction.
5760-
let route_params = RouteParameters::from_payment_params_and_value(
5763+
let mut route_params = RouteParameters::from_payment_params_and_value(
57615764
payment_params, 90_000);
5765+
route_params.max_total_routing_fee_msat = Some(90_000*2);
57625766
let route = get_route(&our_id, &route_params, &network_graph.read_only(), None,
57635767
Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes).unwrap();
57645768
assert_eq!(route.paths.len(), 1);
@@ -5826,8 +5830,9 @@ mod tests {
58265830
// Now, attempt to route 90 sats, hitting the htlc_minimum on channel 4, but
58275831
// overshooting the htlc_maximum on channel 2. Thus, we should pick the (absurdly
58285832
// expensive) channels 12-13 path.
5829-
let route_params = RouteParameters::from_payment_params_and_value(
5833+
let mut route_params = RouteParameters::from_payment_params_and_value(
58305834
payment_params, 90_000);
5835+
route_params.max_total_routing_fee_msat = Some(90_000*2);
58315836
let route = get_route(&our_id, &route_params, &network_graph.read_only(), None,
58325837
Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes).unwrap();
58335838
assert_eq!(route.paths.len(), 1);
@@ -6532,8 +6537,9 @@ mod tests {
65326537

65336538
// Make sure we'll error if our route hints don't have enough liquidity according to their
65346539
// htlc_maximum_msat.
6535-
let route_params = RouteParameters::from_payment_params_and_value(
6540+
let mut route_params = RouteParameters::from_payment_params_and_value(
65366541
payment_params, max_htlc_msat + 1);
6542+
route_params.max_total_routing_fee_msat = None;
65376543
if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route(&our_id,
65386544
&route_params, &netgraph, None, Arc::clone(&logger), &scorer, &Default::default(),
65396545
&random_seed_bytes)
@@ -6547,8 +6553,9 @@ mod tests {
65476553
let payment_params = PaymentParameters::from_node_id(dest_node_id, 42)
65486554
.with_route_hints(vec![route_hint_1, route_hint_2]).unwrap()
65496555
.with_bolt11_features(channelmanager::provided_invoice_features(&config)).unwrap();
6550-
let route_params = RouteParameters::from_payment_params_and_value(
6556+
let mut route_params = RouteParameters::from_payment_params_and_value(
65516557
payment_params, max_htlc_msat + 1);
6558+
route_params.max_total_routing_fee_msat = Some(max_htlc_msat * 2);
65526559
let route = get_route(&our_id, &route_params, &netgraph, None, Arc::clone(&logger),
65536560
&scorer, &Default::default(), &random_seed_bytes).unwrap();
65546561
assert_eq!(route.paths.len(), 2);
@@ -6983,7 +6990,8 @@ mod tests {
69836990
let payment_params = PaymentParameters::blinded(blinded_hints.clone())
69846991
.with_bolt12_features(bolt12_features.clone()).unwrap();
69856992

6986-
let route_params = RouteParameters::from_payment_params_and_value(payment_params, 100_000);
6993+
let mut route_params = RouteParameters::from_payment_params_and_value(payment_params, 100_000);
6994+
route_params.max_total_routing_fee_msat = Some(100_000);
69876995
let route = get_route(&our_id, &route_params, &network_graph, None, Arc::clone(&logger),
69886996
&scorer, &Default::default(), &random_seed_bytes).unwrap();
69896997
assert_eq!(route.paths.len(), 2);

0 commit comments

Comments
 (0)