Skip to content

Commit c8e1916

Browse files
TheBlueMattTheBlueMatt
authored
Merge pull request #2603 from TheBlueMatt/2023-09-default-route-limit
Set a default max_total_routing_fee_msat of 1% + 50sats
2 parents 7d5f137 + 0c31c6f commit c8e1916

File tree

3 files changed

+65
-40
lines changed

3 files changed

+65
-40
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: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -341,7 +341,7 @@ impl Path {
341341

342342
/// A route directs a payment from the sender (us) to the recipient. If the recipient supports MPP,
343343
/// it can take multiple paths. Each path is composed of one or more hops through the network.
344-
#[derive(Clone, Hash, PartialEq, Eq)]
344+
#[derive(Clone, Debug, Hash, PartialEq, Eq)]
345345
pub struct Route {
346346
/// The list of [`Path`]s taken for a single (potentially-)multi-part payment. If no
347347
/// [`BlindedTail`]s are present, then the pubkey of the last [`RouteHop`] in each path must be
@@ -380,6 +380,12 @@ impl Route {
380380
}
381381
}
382382

383+
impl fmt::Display for Route {
384+
fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
385+
log_route!(self).fmt(f)
386+
}
387+
}
388+
383389
const SERIALIZATION_VERSION: u8 = 1;
384390
const MIN_SERIALIZATION_VERSION: u8 = 1;
385391

@@ -475,14 +481,16 @@ pub struct RouteParameters {
475481
/// This limit also applies to the total fees that may arise while retrying failed payment
476482
/// paths.
477483
///
478-
/// Default value: `None`
484+
/// Note that values below a few sats may result in some paths being spuriously ignored.
479485
pub max_total_routing_fee_msat: Option<u64>,
480486
}
481487

482488
impl RouteParameters {
483489
/// 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
484492
pub fn from_payment_params_and_value(payment_params: PaymentParameters, final_value_msat: u64) -> Self {
485-
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) }
486494
}
487495
}
488496

@@ -3092,8 +3100,9 @@ mod tests {
30923100
excess_data: Vec::new()
30933101
});
30943102

3095-
let route_params = RouteParameters::from_payment_params_and_value(
3103+
let mut route_params = RouteParameters::from_payment_params_and_value(
30963104
payment_params.clone(), 60_000);
3105+
route_params.max_total_routing_fee_msat = Some(15_000);
30973106
let route = get_route(&our_id, &route_params, &network_graph.read_only(), None,
30983107
Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes).unwrap();
30993108
// Overpay fees to hit htlc_minimum_msat.
@@ -5751,8 +5760,9 @@ mod tests {
57515760
{
57525761
// Now, attempt to route 90 sats, which is exactly 90 sats at the last hop, plus the
57535762
// 200% fee charged channel 13 in the 1-to-2 direction.
5754-
let route_params = RouteParameters::from_payment_params_and_value(
5763+
let mut route_params = RouteParameters::from_payment_params_and_value(
57555764
payment_params, 90_000);
5765+
route_params.max_total_routing_fee_msat = Some(90_000*2);
57565766
let route = get_route(&our_id, &route_params, &network_graph.read_only(), None,
57575767
Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes).unwrap();
57585768
assert_eq!(route.paths.len(), 1);
@@ -5820,8 +5830,9 @@ mod tests {
58205830
// Now, attempt to route 90 sats, hitting the htlc_minimum on channel 4, but
58215831
// overshooting the htlc_maximum on channel 2. Thus, we should pick the (absurdly
58225832
// expensive) channels 12-13 path.
5823-
let route_params = RouteParameters::from_payment_params_and_value(
5833+
let mut route_params = RouteParameters::from_payment_params_and_value(
58245834
payment_params, 90_000);
5835+
route_params.max_total_routing_fee_msat = Some(90_000*2);
58255836
let route = get_route(&our_id, &route_params, &network_graph.read_only(), None,
58265837
Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes).unwrap();
58275838
assert_eq!(route.paths.len(), 1);
@@ -6526,8 +6537,9 @@ mod tests {
65266537

65276538
// Make sure we'll error if our route hints don't have enough liquidity according to their
65286539
// htlc_maximum_msat.
6529-
let route_params = RouteParameters::from_payment_params_and_value(
6540+
let mut route_params = RouteParameters::from_payment_params_and_value(
65306541
payment_params, max_htlc_msat + 1);
6542+
route_params.max_total_routing_fee_msat = None;
65316543
if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route(&our_id,
65326544
&route_params, &netgraph, None, Arc::clone(&logger), &scorer, &Default::default(),
65336545
&random_seed_bytes)
@@ -6541,8 +6553,9 @@ mod tests {
65416553
let payment_params = PaymentParameters::from_node_id(dest_node_id, 42)
65426554
.with_route_hints(vec![route_hint_1, route_hint_2]).unwrap()
65436555
.with_bolt11_features(channelmanager::provided_invoice_features(&config)).unwrap();
6544-
let route_params = RouteParameters::from_payment_params_and_value(
6556+
let mut route_params = RouteParameters::from_payment_params_and_value(
65456557
payment_params, max_htlc_msat + 1);
6558+
route_params.max_total_routing_fee_msat = Some(max_htlc_msat * 2);
65466559
let route = get_route(&our_id, &route_params, &netgraph, None, Arc::clone(&logger),
65476560
&scorer, &Default::default(), &random_seed_bytes).unwrap();
65486561
assert_eq!(route.paths.len(), 2);
@@ -6977,7 +6990,8 @@ mod tests {
69776990
let payment_params = PaymentParameters::blinded(blinded_hints.clone())
69786991
.with_bolt12_features(bolt12_features.clone()).unwrap();
69796992

6980-
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);
69816995
let route = get_route(&our_id, &route_params, &network_graph, None, Arc::clone(&logger),
69826996
&scorer, &Default::default(), &random_seed_bytes).unwrap();
69836997
assert_eq!(route.paths.len(), 2);

0 commit comments

Comments
 (0)