Various router fixes and #2417 follow-ups#2575

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

Merged
TheBlueMatt 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 numberDiff line numberDiff 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 DownExpand 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 DownExpand 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 numberDiff line numberDiff 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 DownExpand 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 DownExpand 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 DownExpand 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 DownExpand 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);
Comment thread
tnull marked this conversation as resolved.
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 DownExpand 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 DownExpand 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 DownExpand 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
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Various router fixes and #2417 follow-ups#2575

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

 Merged

TheBlueMatt
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 numberDiff line numberDiff 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 DownExpand 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 DownExpand 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 numberDiff line numberDiff 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 DownExpand 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 DownExpand 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 DownExpand 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 DownExpand 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);
Comment thread
tnull marked this conversation as resolved.

	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 DownExpand 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 DownExpand 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 DownExpand 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

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Various router fixes and #2417 follow-ups#2575

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

 Merged

TheBlueMatt
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 numberDiff line numberDiff 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 DownExpand 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 DownExpand 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 numberDiff line numberDiff 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 DownExpand 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 DownExpand 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 DownExpand 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 DownExpand 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);
Comment thread
tnull marked this conversation as resolved.

	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 DownExpand 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 DownExpand 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 DownExpand 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

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Various router fixes and #2417 follow-ups#2575

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

 Merged

TheBlueMatt
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 numberDiff line numberDiff 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 DownExpand 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 DownExpand 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 numberDiff line numberDiff 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 DownExpand 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 DownExpand 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 DownExpand 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 DownExpand 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);
Comment thread
tnull marked this conversation as resolved.

	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 DownExpand 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 DownExpand 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 DownExpand 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

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Various router fixes and #2417 follow-ups#2575

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

 Merged

TheBlueMatt
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 numberDiff line numberDiff 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 DownExpand 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 DownExpand 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 numberDiff line numberDiff 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 DownExpand 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 DownExpand 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 DownExpand 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 DownExpand 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);
Comment thread
tnull marked this conversation as resolved.

	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 DownExpand 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 DownExpand 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 DownExpand 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

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Various router fixes and #2417 follow-ups#2575

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

 Merged

TheBlueMatt
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 numberDiff line numberDiff 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 DownExpand 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 DownExpand 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 numberDiff line numberDiff 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 DownExpand 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 DownExpand 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 DownExpand 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 DownExpand 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);
Comment thread
tnull marked this conversation as resolved.

	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 DownExpand 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 DownExpand 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 DownExpand 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

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Various router fixes and #2417 follow-ups#2575

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

 Merged

TheBlueMatt
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 numberDiff line numberDiff 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 DownExpand 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 DownExpand 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 numberDiff line numberDiff 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 DownExpand 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 DownExpand 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 DownExpand 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 DownExpand 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);
Comment thread
tnull marked this conversation as resolved.

	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 DownExpand 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 DownExpand 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 DownExpand 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

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Various router fixes and #2417 follow-ups#2575

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

 Merged

TheBlueMatt
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 numberDiff line numberDiff 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 DownExpand 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 DownExpand 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 numberDiff line numberDiff 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 DownExpand 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 DownExpand 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 DownExpand 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 DownExpand 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);
Comment thread
tnull marked this conversation as resolved.

	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 DownExpand 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 DownExpand 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 DownExpand 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