Skip to content

Commit 1e6707d

Browse files
TheBlueMattTheBlueMatt
authored
Merge pull request #2575 from tnull/2023-09-fix-debug-panic
Various router fixes and #2417 follow-ups
2 parents 4ab6c55 + be1088a commit 1e6707d

File tree

4 files changed

+117
-87
lines changed

4 files changed

+117
-87
lines changed

‎lightning/src/ln/outbound_payment.rs

+29-6
Original file line numberDiff line numberDiff line change
@@ -875,7 +875,7 @@ impl OutboundPayments {
875875
}
876876
}
877877

878-
let route = router.find_route_with_id(
878+
let mut route = router.find_route_with_id(
879879
&node_signer.get_node_id(Recipient::Node).unwrap(), &route_params,
880880
Some(&first_hops.iter().collect::<Vec<_>>()), inflight_htlcs(),
881881
payment_hash, payment_id,
@@ -885,6 +885,14 @@ impl OutboundPayments {
885885
RetryableSendFailure::RouteNotFound
886886
})?;
887887

888+
if let Some(route_route_params) = route.route_params.as_mut() {
889+
if route_route_params.final_value_msat != route_params.final_value_msat {
890+
debug_assert!(false,
891+
"Routers are expected to return a route which includes the requested final_value_msat");
892+
route_route_params.final_value_msat = route_params.final_value_msat;
893+
}
894+
}
895+
888896
let onion_session_privs = self.add_new_pending_payment(payment_hash,
889897
recipient_onion.clone(), payment_id, keysend_preimage, &route, Some(retry_strategy),
890898
Some(route_params.payment_params.clone()), entropy_source, best_block_height)
@@ -926,7 +934,7 @@ impl OutboundPayments {
926934
}
927935
}
928936

929-
let route = match router.find_route_with_id(
937+
let mut route = match router.find_route_with_id(
930938
&node_signer.get_node_id(Recipient::Node).unwrap(), &route_params,
931939
Some(&first_hops.iter().collect::<Vec<_>>()), inflight_htlcs(),
932940
payment_hash, payment_id,
@@ -938,6 +946,15 @@ impl OutboundPayments {
938946
return
939947
}
940948
};
949+
950+
if let Some(route_route_params) = route.route_params.as_mut() {
951+
if route_route_params.final_value_msat != route_params.final_value_msat {
952+
debug_assert!(false,
953+
"Routers are expected to return a route which includes the requested final_value_msat");
954+
route_route_params.final_value_msat = route_params.final_value_msat;
955+
}
956+
}
957+
941958
for path in route.paths.iter() {
942959
if path.hops.len() == 0 {
943960
log_error!(logger, "Unusable path in route (path.hops.len() must be at least 1");
@@ -1337,12 +1354,14 @@ impl OutboundPayments {
13371354
}
13381355
let mut has_ok = false;
13391356
let mut has_err = false;
1340-
let mut pending_amt_unsent = 0;
1357+
let mut has_unsent = false;
13411358
let mut total_ok_fees_msat = 0;
1359+
let mut total_ok_amt_sent_msat = 0;
13421360
for (res, path) in results.iter().zip(route.paths.iter()) {
13431361
if res.is_ok() {
13441362
has_ok = true;
13451363
total_ok_fees_msat += path.fee_msat();
1364+
total_ok_amt_sent_msat += path.final_value_msat();
13461365
}
13471366
if res.is_err() { has_err = true; }
13481367
if let &Err(APIError::MonitorUpdateInProgress) = res {
@@ -1351,23 +1370,27 @@ impl OutboundPayments {
13511370
has_err = true;
13521371
has_ok = true;
13531372
total_ok_fees_msat += path.fee_msat();
1373+
total_ok_amt_sent_msat += path.final_value_msat();
13541374
} else if res.is_err() {
1355-
pending_amt_unsent += path.final_value_msat();
1375+
has_unsent = true;
13561376
}
13571377
}
13581378
if has_err && has_ok {
13591379
Err(PaymentSendFailure::PartialFailure {
13601380
results,
13611381
payment_id,
1362-
failed_paths_retry: if pending_amt_unsent != 0 {
1382+
failed_paths_retry: if has_unsent {
13631383
if let Some(route_params) = &route.route_params {
13641384
let mut route_params = route_params.clone();
13651385
// We calculate the leftover fee budget we're allowed to spend by
13661386
// subtracting the used fee from the total fee budget.
13671387
route_params.max_total_routing_fee_msat = route_params
13681388
.max_total_routing_fee_msat.map(|m| m.saturating_sub(total_ok_fees_msat));
1369-
route_params.final_value_msat = pending_amt_unsent;
13701389

1390+
// We calculate the remaining target amount by subtracting the succeded
1391+
// path values.
1392+
route_params.final_value_msat = route_params.final_value_msat
1393+
.saturating_sub(total_ok_amt_sent_msat);
13711394
Some(route_params)
13721395
} else { None }
13731396
} else { None },

‎lightning/src/ln/payment_tests.rs

+36-62
Original file line numberDiff line numberDiff line change
@@ -114,19 +114,9 @@ fn mpp_retry() {
114114

115115
// Add the HTLC along the first hop.
116116
let fail_path_msgs_1 = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &mut events);
117-
let (update_add, commitment_signed) = match fail_path_msgs_1 {
118-
MessageSendEvent::UpdateHTLCs { node_id: _, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, ref update_fail_malformed_htlcs, ref update_fee, ref commitment_signed } } => {
119-
assert_eq!(update_add_htlcs.len(), 1);
120-
assert!(update_fail_htlcs.is_empty());
121-
assert!(update_fulfill_htlcs.is_empty());
122-
assert!(update_fail_malformed_htlcs.is_empty());
123-
assert!(update_fee.is_none());
124-
(update_add_htlcs[0].clone(), commitment_signed.clone())
125-
},
126-
_ => panic!("Unexpected event"),
127-
};
128-
nodes[2].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &update_add);
129-
commitment_signed_dance!(nodes[2], nodes[0], commitment_signed, false);
117+
let send_event = SendEvent::from_event(fail_path_msgs_1);
118+
nodes[2].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &send_event.msgs[0]);
119+
commitment_signed_dance!(nodes[2], nodes[0], &send_event.commitment_msg, false);
130120

131121
// Attempt to forward the payment and complete the 2nd path's failure.
132122
expect_pending_htlcs_forwardable!(&nodes[2]);
@@ -225,25 +215,9 @@ fn mpp_retry_overpay() {
225215

226216
// Add the HTLC along the first hop.
227217
let fail_path_msgs_1 = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &mut events);
228-
let (update_add, commitment_signed) = match fail_path_msgs_1 {
229-
MessageSendEvent::UpdateHTLCs {
230-
node_id: _,
231-
updates: msgs::CommitmentUpdate {
232-
ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs,
233-
ref update_fail_malformed_htlcs, ref update_fee, ref commitment_signed
234-
}
235-
} => {
236-
assert_eq!(update_add_htlcs.len(), 1);
237-
assert!(update_fail_htlcs.is_empty());
238-
assert!(update_fulfill_htlcs.is_empty());
239-
assert!(update_fail_malformed_htlcs.is_empty());
240-
assert!(update_fee.is_none());
241-
(update_add_htlcs[0].clone(), commitment_signed.clone())
242-
},
243-
_ => panic!("Unexpected event"),
244-
};
245-
nodes[2].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &update_add);
246-
commitment_signed_dance!(nodes[2], nodes[0], commitment_signed, false);
218+
let send_event = SendEvent::from_event(fail_path_msgs_1);
219+
nodes[2].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &send_event.msgs[0]);
220+
commitment_signed_dance!(nodes[2], nodes[0], &send_event.commitment_msg, false);
247221

248222
// Attempt to forward the payment and complete the 2nd path's failure.
249223
expect_pending_htlcs_forwardable!(&nodes[2]);
@@ -279,6 +253,7 @@ fn mpp_retry_overpay() {
279253

280254
route.paths.remove(0);
281255
route_params.final_value_msat -= first_path_value;
256+
route.route_params.as_mut().map(|p| p.final_value_msat -= first_path_value);
282257
route_params.payment_params.previously_failed_channels.push(chan_4_update.contents.short_channel_id);
283258

284259
// Check the remaining max total routing fee for the second attempt accounts only for 1_000 msat
@@ -2421,10 +2396,11 @@ fn auto_retry_partial_failure() {
24212396
let payment_params = PaymentParameters::from_node_id(nodes[1].node.get_our_node_id(), TEST_FINAL_CLTV)
24222397
.with_expiry_time(payment_expiry_secs as u64)
24232398
.with_bolt11_features(invoice_features).unwrap();
2399+
2400+
// Configure the initial send path
24242401
let mut route_params = RouteParameters::from_payment_params_and_value(payment_params, amt_msat);
24252402
route_params.max_total_routing_fee_msat = None;
24262403

2427-
// Configure the initial send, retry1 and retry2's paths.
24282404
let send_route = Route {
24292405
paths: vec![
24302406
Path { hops: vec![RouteHop {
@@ -2448,6 +2424,14 @@ fn auto_retry_partial_failure() {
24482424
],
24492425
route_params: Some(route_params.clone()),
24502426
};
2427+
nodes[0].router.expect_find_route(route_params.clone(), Ok(send_route));
2428+
2429+
// Configure the retry1 paths
2430+
let mut payment_params = route_params.payment_params.clone();
2431+
payment_params.previously_failed_channels.push(chan_2_id);
2432+
let mut retry_1_params = RouteParameters::from_payment_params_and_value(payment_params, amt_msat / 2);
2433+
retry_1_params.max_total_routing_fee_msat = None;
2434+
24512435
let retry_1_route = Route {
24522436
paths: vec![
24532437
Path { hops: vec![RouteHop {
@@ -2469,8 +2453,16 @@ fn auto_retry_partial_failure() {
24692453
maybe_announced_channel: true,
24702454
}], blinded_tail: None },
24712455
],
2472-
route_params: Some(route_params.clone()),
2456+
route_params: Some(retry_1_params.clone()),
24732457
};
2458+
nodes[0].router.expect_find_route(retry_1_params.clone(), Ok(retry_1_route));
2459+
2460+
// Configure the retry2 path
2461+
let mut payment_params = retry_1_params.payment_params.clone();
2462+
payment_params.previously_failed_channels.push(chan_3_id);
2463+
let mut retry_2_params = RouteParameters::from_payment_params_and_value(payment_params, amt_msat / 4);
2464+
retry_2_params.max_total_routing_fee_msat = None;
2465+
24742466
let retry_2_route = Route {
24752467
paths: vec![
24762468
Path { hops: vec![RouteHop {
@@ -2483,20 +2475,8 @@ fn auto_retry_partial_failure() {
24832475
maybe_announced_channel: true,
24842476
}], blinded_tail: None },
24852477
],
2486-
route_params: Some(route_params.clone()),
2478+
route_params: Some(retry_2_params.clone()),
24872479
};
2488-
nodes[0].router.expect_find_route(route_params.clone(), Ok(send_route));
2489-
let mut payment_params = route_params.payment_params.clone();
2490-
payment_params.previously_failed_channels.push(chan_2_id);
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-
2496-
let mut payment_params = route_params.payment_params.clone();
2497-
payment_params.previously_failed_channels.push(chan_3_id);
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;
25002480
nodes[0].router.expect_find_route(retry_2_params, Ok(retry_2_route));
25012481

25022482
// Send a payment that will partially fail on send, then partially fail on retry, then succeed.
@@ -2756,10 +2736,9 @@ fn retry_multi_path_single_failed_payment() {
27562736
let mut pay_params = route.route_params.clone().unwrap().payment_params;
27572737
pay_params.previously_failed_channels.push(chans[1].short_channel_id.unwrap());
27582738

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);
2739+
let mut retry_params = RouteParameters::from_payment_params_and_value(pay_params, 100_000_000);
27622740
retry_params.max_total_routing_fee_msat = None;
2741+
route.route_params.as_mut().unwrap().final_value_msat = 100_000_000;
27632742
nodes[0].router.expect_find_route(retry_params, Ok(route.clone()));
27642743

27652744
{
@@ -2943,9 +2922,7 @@ fn no_extra_retries_on_back_to_back_fail() {
29432922
maybe_announced_channel: true,
29442923
}], blinded_tail: None }
29452924
],
2946-
route_params: Some(RouteParameters::from_payment_params_and_value(
2947-
PaymentParameters::from_node_id(nodes[2].node.get_our_node_id(), TEST_FINAL_CLTV),
2948-
100_000_000)),
2925+
route_params: Some(route_params.clone()),
29492926
};
29502927
route.route_params.as_mut().unwrap().max_total_routing_fee_msat = None;
29512928
nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone()));
@@ -3149,18 +3126,18 @@ fn test_simple_partial_retry() {
31493126
maybe_announced_channel: true,
31503127
}], blinded_tail: None }
31513128
],
3152-
route_params: Some(RouteParameters::from_payment_params_and_value(
3153-
PaymentParameters::from_node_id(nodes[2].node.get_our_node_id(), TEST_FINAL_CLTV),
3154-
100_000_000)),
3129+
route_params: Some(route_params.clone()),
31553130
};
3156-
route.route_params.as_mut().unwrap().max_total_routing_fee_msat = None;
3131+
31573132
nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone()));
3133+
31583134
let mut second_payment_params = route_params.payment_params.clone();
31593135
second_payment_params.previously_failed_channels = vec![chan_2_scid];
31603136
// On retry, we'll only be asked for one path (or 100k sats)
31613137
route.paths.remove(0);
31623138
let mut retry_params = RouteParameters::from_payment_params_and_value(second_payment_params, amt_msat / 2);
31633139
retry_params.max_total_routing_fee_msat = None;
3140+
route.route_params.as_mut().unwrap().final_value_msat = amt_msat / 2;
31643141
nodes[0].router.expect_find_route(retry_params, Ok(route.clone()));
31653142

31663143
nodes[0].node.send_payment(payment_hash, RecipientOnionFields::secret_only(payment_secret),
@@ -3320,11 +3297,7 @@ fn test_threaded_payment_retries() {
33203297
maybe_announced_channel: true,
33213298
}], blinded_tail: None }
33223299
],
3323-
route_params: Some(RouteParameters {
3324-
payment_params: PaymentParameters::from_node_id(nodes[1].node.get_our_node_id(), TEST_FINAL_CLTV),
3325-
final_value_msat: amt_msat - amt_msat / 1000,
3326-
max_total_routing_fee_msat: Some(500_000),
3327-
}),
3300+
route_params: Some(route_params.clone()),
33283301
};
33293302
nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone()));
33303303

@@ -3343,6 +3316,7 @@ fn test_threaded_payment_retries() {
33433316

33443317
// from here on out, the retry `RouteParameters` amount will be amt/1000
33453318
route_params.final_value_msat /= 1000;
3319+
route.route_params.as_mut().unwrap().final_value_msat /= 1000;
33463320
route.paths.pop();
33473321

33483322
let end_time = Instant::now() + Duration::from_secs(1);

0 commit comments

Comments
 (0)