Skip to content

Commit

Permalink
Merge pull request #2575 from tnull/2023-09-fix-debug-panic
Browse files Browse the repository at this point in the history 
Various router fixes and #2417 follow-ups
  • Loading branch information
@TheBlueMatt
TheBlueMatt authored Sep 28, 2023
2 parents 4ab6c55 + be1088a commit 1e6707d
Show file tree
Hide file tree
Showing 4 changed files with 117 additions and 87 deletions.
35 changes: 29 additions & 6 deletions lightning/src/ln/outbound_payment.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -875,7 +875,7 @@ impl OutboundPayments {
}
}

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

if let Some(route_route_params) = route.route_params.as_mut() {
if route_route_params.final_value_msat != route_params.final_value_msat {
debug_assert!(false,
"Routers are expected to return a route which includes the requested final_value_msat");
route_route_params.final_value_msat = route_params.final_value_msat;
}
}

let onion_session_privs = self.add_new_pending_payment(payment_hash,
recipient_onion.clone(), payment_id, keysend_preimage, &route, Some(retry_strategy),
Some(route_params.payment_params.clone()), entropy_source, best_block_height)
Expand Down Expand Up @@ -926,7 +934,7 @@ impl OutboundPayments {
}
}

let route = match router.find_route_with_id(
let mut route = match router.find_route_with_id(
&node_signer.get_node_id(Recipient::Node).unwrap(), &route_params,
Some(&first_hops.iter().collect::<Vec<_>>()), inflight_htlcs(),
payment_hash, payment_id,
Expand All @@ -938,6 +946,15 @@ impl OutboundPayments {
return
}
};

if let Some(route_route_params) = route.route_params.as_mut() {
if route_route_params.final_value_msat != route_params.final_value_msat {
debug_assert!(false,
"Routers are expected to return a route which includes the requested final_value_msat");
route_route_params.final_value_msat = route_params.final_value_msat;
}
}

for path in route.paths.iter() {
if path.hops.len() == 0 {
log_error!(logger, "Unusable path in route (path.hops.len() must be at least 1");
Expand Down Expand Up @@ -1337,12 +1354,14 @@ impl OutboundPayments {
}
let mut has_ok = false;
let mut has_err = false;
let mut pending_amt_unsent = 0;
let mut has_unsent = false;
let mut total_ok_fees_msat = 0;
let mut total_ok_amt_sent_msat = 0;
for (res, path) in results.iter().zip(route.paths.iter()) {
if res.is_ok() {
has_ok = true;
total_ok_fees_msat += path.fee_msat();
total_ok_amt_sent_msat += path.final_value_msat();
}
if res.is_err() { has_err = true; }
if let &Err(APIError::MonitorUpdateInProgress) = res {
Expand All @@ -1351,23 +1370,27 @@ impl OutboundPayments {
has_err = true;
has_ok = true;
total_ok_fees_msat += path.fee_msat();
total_ok_amt_sent_msat += path.final_value_msat();
} else if res.is_err() {
pending_amt_unsent += path.final_value_msat();
has_unsent = true;
}
}
if has_err && has_ok {
Err(PaymentSendFailure::PartialFailure {
results,
payment_id,
failed_paths_retry: if pending_amt_unsent != 0 {
failed_paths_retry: if has_unsent {
if let Some(route_params) = &route.route_params {
let mut route_params = route_params.clone();
// We calculate the leftover fee budget we're allowed to spend by
// subtracting the used fee from the total fee budget.
route_params.max_total_routing_fee_msat = route_params
.max_total_routing_fee_msat.map(|m| m.saturating_sub(total_ok_fees_msat));
route_params.final_value_msat = pending_amt_unsent;

// We calculate the remaining target amount by subtracting the succeded
// path values.
route_params.final_value_msat = route_params.final_value_msat
.saturating_sub(total_ok_amt_sent_msat);
Some(route_params)
} else { None }
} else { None },
Expand Down
98 changes: 36 additions & 62 deletions lightning/src/ln/payment_tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,19 +114,9 @@ fn mpp_retry() {

// Add the HTLC along the first hop.
let fail_path_msgs_1 = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &mut events);
let (update_add, commitment_signed) = match fail_path_msgs_1 {
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 } } => {
assert_eq!(update_add_htlcs.len(), 1);
assert!(update_fail_htlcs.is_empty());
assert!(update_fulfill_htlcs.is_empty());
assert!(update_fail_malformed_htlcs.is_empty());
assert!(update_fee.is_none());
(update_add_htlcs[0].clone(), commitment_signed.clone())
},
_ => panic!("Unexpected event"),
};
nodes[2].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &update_add);
commitment_signed_dance!(nodes[2], nodes[0], commitment_signed, false);
let send_event = SendEvent::from_event(fail_path_msgs_1);
nodes[2].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &send_event.msgs[0]);
commitment_signed_dance!(nodes[2], nodes[0], &send_event.commitment_msg, false);

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

// Add the HTLC along the first hop.
let fail_path_msgs_1 = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &mut events);
let (update_add, commitment_signed) = match fail_path_msgs_1 {
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
}
} => {
assert_eq!(update_add_htlcs.len(), 1);
assert!(update_fail_htlcs.is_empty());
assert!(update_fulfill_htlcs.is_empty());
assert!(update_fail_malformed_htlcs.is_empty());
assert!(update_fee.is_none());
(update_add_htlcs[0].clone(), commitment_signed.clone())
},
_ => panic!("Unexpected event"),
};
nodes[2].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &update_add);
commitment_signed_dance!(nodes[2], nodes[0], commitment_signed, false);
let send_event = SendEvent::from_event(fail_path_msgs_1);
nodes[2].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &send_event.msgs[0]);
commitment_signed_dance!(nodes[2], nodes[0], &send_event.commitment_msg, false);

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

route.paths.remove(0);
route_params.final_value_msat -= first_path_value;
route.route_params.as_mut().map(|p| p.final_value_msat -= first_path_value);
route_params.payment_params.previously_failed_channels.push(chan_4_update.contents.short_channel_id);

// Check the remaining max total routing fee for the second attempt accounts only for 1_000 msat
Expand Down Expand Up @@ -2421,10 +2396,11 @@ 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();

// Configure the initial send path
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 {
paths: vec![
Path { hops: vec![RouteHop {
Expand All @@ -2448,6 +2424,14 @@ fn auto_retry_partial_failure() {
],
route_params: Some(route_params.clone()),
};
nodes[0].router.expect_find_route(route_params.clone(), Ok(send_route));

// Configure the retry1 paths
let mut payment_params = route_params.payment_params.clone();
payment_params.previously_failed_channels.push(chan_2_id);
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;

let retry_1_route = Route {
paths: vec![
Path { hops: vec![RouteHop {
Expand All @@ -2469,8 +2453,16 @@ fn auto_retry_partial_failure() {
maybe_announced_channel: true,
}], blinded_tail: None },
],
route_params: Some(route_params.clone()),
route_params: Some(retry_1_params.clone()),
};
nodes[0].router.expect_find_route(retry_1_params.clone(), Ok(retry_1_route));

// Configure the retry2 path
let mut payment_params = retry_1_params.payment_params.clone();
payment_params.previously_failed_channels.push(chan_3_id);
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;

let retry_2_route = Route {
paths: vec![
Path { hops: vec![RouteHop {
Expand All @@ -2483,20 +2475,8 @@ fn auto_retry_partial_failure() {
maybe_announced_channel: true,
}], blinded_tail: None },
],
route_params: Some(route_params.clone()),
route_params: Some(retry_2_params.clone()),
};
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);

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);
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.
Expand Down Expand Up @@ -2756,10 +2736,9 @@ fn retry_multi_path_single_failed_payment() {
let mut pay_params = route.route_params.clone().unwrap().payment_params;
pay_params.previously_failed_channels.push(chans[1].short_channel_id.unwrap());

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

{
Expand Down Expand Up @@ -2943,9 +2922,7 @@ fn no_extra_retries_on_back_to_back_fail() {
maybe_announced_channel: true,
}], blinded_tail: None }
],
route_params: Some(RouteParameters::from_payment_params_and_value(
PaymentParameters::from_node_id(nodes[2].node.get_our_node_id(), TEST_FINAL_CLTV),
100_000_000)),
route_params: Some(route_params.clone()),
};
route.route_params.as_mut().unwrap().max_total_routing_fee_msat = None;
nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone()));
Expand Down Expand Up @@ -3149,18 +3126,18 @@ fn test_simple_partial_retry() {
maybe_announced_channel: true,
}], blinded_tail: None }
],
route_params: Some(RouteParameters::from_payment_params_and_value(
PaymentParameters::from_node_id(nodes[2].node.get_our_node_id(), TEST_FINAL_CLTV),
100_000_000)),
route_params: Some(route_params.clone()),
};
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);
let mut retry_params = RouteParameters::from_payment_params_and_value(second_payment_params, amt_msat / 2);
retry_params.max_total_routing_fee_msat = None;
route.route_params.as_mut().unwrap().final_value_msat = amt_msat / 2;
nodes[0].router.expect_find_route(retry_params, Ok(route.clone()));

nodes[0].node.send_payment(payment_hash, RecipientOnionFields::secret_only(payment_secret),
Expand Down Expand Up @@ -3320,11 +3297,7 @@ fn test_threaded_payment_retries() {
maybe_announced_channel: true,
}], blinded_tail: None }
],
route_params: Some(RouteParameters {
payment_params: PaymentParameters::from_node_id(nodes[1].node.get_our_node_id(), TEST_FINAL_CLTV),
final_value_msat: amt_msat - amt_msat / 1000,
max_total_routing_fee_msat: Some(500_000),
}),
route_params: Some(route_params.clone()),
};
nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone()));

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

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

let end_time = Instant::now() + Duration::from_secs(1);
Expand Down
Loading

0 comments on commit 1e6707d

Please sign in to comment.