Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Various router fixes and #2417 follow-ups #2575

Merged
merged 8 commits into from
Sep 28, 2023
Merged

Various router fixes and #2417 follow-ups #2575

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
391da3f
Use `saturating_add` when adding inflight HTLCs values
tnull Sep 14, 2023
a8eef29
Extend logging of ignored candidates
tnull Sep 22, 2023
7a6d309
Also add route hints if we are the source
tnull Sep 22, 2023
c480b0c
Avoid adding duplicate hint candidates if they are first hops
tnull Sep 22, 2023
39c5bbc
Make ignored candidate counts `u32`
tnull Sep 27, 2023
fb2c959
Refactor `mpp_retry[_overpay]` tests to use `SendEvent::from_event`
tnull Sep 27, 2023
8c99e34
Assert query's and route's `final_value_msat` are equal
tnull Sep 28, 2023
be1088a
Don't retry overpaid values for `PartialFailure`s
tnull Sep 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 29 additions & 6 deletions lightning/src/ln/outbound_payment.rs
View file
Open in desktop
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
Open in desktop
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