Respect route hint `max_htlc` in pathfinding #2305

valentinewallace · 2023-05-17T23:18:17Z

We have an htlc_maximum_msat field in RouteHint, but we've so far ignored it in pathfinding because BOLT 11 route hints don't provide this field. We'll need to start abiding by route hint max_htlcs when we add support for blinded pathfinding in #2120 because BOLT 12 route hints provide this field, so let's start incorporating it now.

Also get rid of some trailing whitespace because my text editor likes to do that. We'll next add a new variant for max_htlc provided in route hints, which will be treated differently in scoring.

codecov-commenter · 2023-05-18T00:34:31Z

Codecov Report

Patch coverage: 99.20% and project coverage change: -0.06 ⚠️

Comparison is base (5c89d01) 90.87% compared to head (60987e0) 90.82%.

❗ Current head 60987e0 differs from pull request most recent head 0d5ac88. Consider uploading reports for the commit 0d5ac88 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2305      +/-   ##
==========================================
- Coverage   90.87%   90.82%   -0.06%     
==========================================
  Files         104      104              
  Lines       52789    55111    +2322     
  Branches    52789    55111    +2322     
==========================================
+ Hits        47972    50052    +2080     
- Misses       4817     5059     +242

Impacted Files	Coverage Δ
lightning/src/routing/gossip.rs	`89.71% <50.00%> (-0.07%)`	⬇️
lightning/src/routing/router.rs	`93.09% <100.00%> (-1.35%)`	⬇️
lightning/src/routing/scoring.rs	`94.12% <100.00%> (ø)`

... and 13 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

dunxen

LGTM

valentinewallace · 2023-05-18T14:35:31Z

I found a bug where we'll fail to split a payment over two route hints if they have the same source node id and we have a direct channel to said source node id, hitting the debug_assert used_liquidity_msat <= hop_max_msat. Working on a fix

dunxen · 2023-05-18T14:41:33Z

I found a bug where we'll fail to split a payment over two route hints if they have the same source node id and we have a direct channel to said source node id, hitting the debug_assert used_liquidity_msat <= hop_max_msat. Working on a fix

Oh interesting. Probably good to include a test for that case.

carlaKC

LGTM

carlaKC · 2023-05-18T18:43:03Z

lightning/src/routing/gossip.rs

@@ -1033,6 +1033,11 @@ pub enum EffectiveCapacity {
 	/// A capacity sufficient to route any payment, typically used for private channels provided by
 	/// an invoice.
 	Infinite,
+	/// The maximum HTLC amount as provided by an invoice route hint.
+	HintMaxHTLC {


if this is used for blinded paths as well, Hint might not be the best name.

Ah yeah, there's several places in the code already where we refer to (BlindedPayInfo, BlindedPath) things from BOLT12 invoices as "blinded route hints" (and more in #2120).

That said, I'm not super happy with this name, so v open to alternatives

valentinewallace · 2023-05-18T19:25:18Z

Sadly I found another similar bug. Fixed now. Hope it's for real this time.

lightning/src/routing/router.rs

valentinewallace · 2023-05-19T16:55:13Z

Tacked on an additional commit because get_route was returning suboptimal routes.

tnull

I ran some benches (4 total, 2 local on notebook, 2 on server):

Notebook (Apple M1 Pro):

RUN #1:

BASE:

test routing::router::benches::generate_mpp_routes_with_probabilistic_scorer ... bench: 128,331,316 ns/iter (+/- 34,214,124)
test routing::router::benches::generate_mpp_routes_with_zero_penalty_scorer  ... bench: 105,527,599 ns/iter (+/- 51,893,095)
test routing::router::benches::generate_routes_with_probabilistic_scorer     ... bench: 122,811,941 ns/iter (+/- 28,075,038)
test routing::router::benches::generate_routes_with_zero_penalty_scorer      ... bench: 102,338,695 ns/iter (+/- 83,908,199)

HEAD:

test routing::router::benches::generate_mpp_routes_with_probabilistic_scorer ... bench: 128,418,933 ns/iter (+/- 21,778,847)
test routing::router::benches::generate_mpp_routes_with_zero_penalty_scorer  ... bench:  98,440,187 ns/iter (+/- 63,266,719)
test routing::router::benches::generate_routes_with_probabilistic_scorer     ... bench: 124,550,208 ns/iter (+/- 21,458,416)
test routing::router::benches::generate_routes_with_zero_penalty_scorer      ... bench:  99,894,774 ns/iter (+/- 72,989,638)

RUN #2:

BASE:

test routing::router::benches::generate_mpp_routes_with_probabilistic_scorer ... bench: 123,992,545 ns/iter (+/- 24,040,908)
test routing::router::benches::generate_mpp_routes_with_zero_penalty_scorer  ... bench: 100,857,441 ns/iter (+/- 65,807,407)
test routing::router::benches::generate_routes_with_probabilistic_scorer     ... bench: 118,328,937 ns/iter (+/- 13,768,676)
test routing::router::benches::generate_routes_with_zero_penalty_scorer      ... bench: 102,168,212 ns/iter (+/- 48,080,466)

HEAD:

test routing::router::benches::generate_mpp_routes_with_probabilistic_scorer ... bench: 119,668,829 ns/iter (+/- 32,563,378)
test routing::router::benches::generate_mpp_routes_with_zero_penalty_scorer  ... bench: 101,001,891 ns/iter (+/- 51,208,428)
test routing::router::benches::generate_routes_with_probabilistic_scorer     ... bench: 120,288,587 ns/iter (+/- 34,034,627)
test routing::router::benches::generate_routes_with_zero_penalty_scorer      ... bench:  98,606,216 ns/iter (+/- 59,330,778)

Server (Intel(R) Xeon(R) CPU E3-1226 v3 @ 3.30GHz):

RUN #1:

BASE:

test routing::router::benches::generate_mpp_routes_with_probabilistic_scorer ... bench: 167,627,890 ns/iter (+/- 29,241,988)
test routing::router::benches::generate_mpp_routes_with_zero_penalty_scorer  ... bench: 122,588,677 ns/iter (+/- 81,634,017)
test routing::router::benches::generate_routes_with_probabilistic_scorer     ... bench: 167,874,126 ns/iter (+/- 39,447,553)
test routing::router::benches::generate_routes_with_zero_penalty_scorer      ... bench: 120,162,977 ns/iter (+/- 78,110,553)

HEAD:

test routing::router::benches::generate_mpp_routes_with_probabilistic_scorer ... bench: 171,435,012 ns/iter (+/- 50,514,398)
test routing::router::benches::generate_mpp_routes_with_zero_penalty_scorer  ... bench: 121,795,169 ns/iter (+/- 83,967,647)
test routing::router::benches::generate_routes_with_probabilistic_scorer     ... bench: 168,357,184 ns/iter (+/- 30,977,056)
test routing::router::benches::generate_routes_with_zero_penalty_scorer      ... bench: 112,352,576 ns/iter (+/- 81,124,152)

RUN #2:

BASE:

test routing::router::benches::generate_mpp_routes_with_probabilistic_scorer ... bench: 164,439,346 ns/iter (+/- 38,028,845)
test routing::router::benches::generate_mpp_routes_with_zero_penalty_scorer  ... bench: 118,526,962 ns/iter (+/- 92,154,137)
test routing::router::benches::generate_routes_with_probabilistic_scorer     ... bench: 170,246,213 ns/iter (+/- 33,679,643)
test routing::router::benches::generate_routes_with_zero_penalty_scorer      ... bench: 128,066,773 ns/iter (+/- 66,781,456)

HEAD:

test routing::router::benches::generate_mpp_routes_with_probabilistic_scorer ... bench: 173,568,800 ns/iter (+/- 31,740,701)
test routing::router::benches::generate_mpp_routes_with_zero_penalty_scorer  ... bench: 115,892,727 ns/iter (+/- 48,088,270)
test routing::router::benches::generate_routes_with_probabilistic_scorer     ... bench: 165,457,689 ns/iter (+/- 37,772,193)
test routing::router::benches::generate_routes_with_zero_penalty_scorer      ... bench: 122,303,787 ns/iter (+/- 90,595,307)

I can't see any noticeable differences outside of standard deviation here.

lightning/src/routing/router.rs

fuzz/src/router.rs

lightning/src/routing/router.rs

tnull

LGTM, would be ready for the squash from my side.

valentinewallace · 2023-05-24T14:14:27Z

Squashed with no diff. Thanks for the benching @tnull!

TheBlueMatt

Sorry for the delay.

TheBlueMatt · 2023-05-31T00:48:53Z

lightning/src/routing/router.rs

@@ -1863,14 +1886,17 @@ where L::Target: Logger {
 						.saturating_add(1);

 					// Searching for a direct channel between last checked hop and first_hop_targets
-					if let Some(first_channels) = first_hop_targets.get(&NodeId::from_pubkey(&prev_hop_id)) {
+					let hint_candidate_contribution_msat = cmp::min(path_value_msat,


I think this is also already done in add_entry? It currently takes the $next_hops_value_contribution argument (which is what this is passed as) and min's it with available_value_contribution_msat, which is calculated as the effective_capacity (via max_htlc_from_capacity minus $next_hops_fee_msat minus used_liquidity_msat (if one exists).

I think we need this because otherwise add_entry assumes that the next hop (aka the route hint hop) can carry the full payment value, since available_value_contribution_msat only factors in the current candidate's capacity, not the next hop candidate's. direct_channel_to_hints_with_max_htlc fails without this.

Right, but I believe the current code doesn't actually propagate that backwards - if we have one hop that's limited we will "jump back" to the max value for an earlier hop that isn't limited (not that we can actually have such a path right now). It also doesn't consider any other limiting of the value, this patch seems to pass your test:

diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index da1ae9771..4d1d68242 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -1493,8 +1493,9 @@ where L::Target: Logger { ( $candidate: expr, $src_node_id: expr, $dest_node_id: expr, $next_hops_fee_msat: expr, $next_hops_value_contribution: expr, $next_hops_path_htlc_minimum_msat: expr, $next_hops_path_penalty_msat: expr, $next_hops_cltv_delta: expr, $next_hops_path_length: expr ) => { { - // We "return" whether we updated the path at the end, via this: - let mut did_add_update_path_to_src_node = false; + // We "return" whether we updated the path at the end, and how much we can route via + // this channel via this: + let mut did_add_update_path_to_src_node = None; // Channels to self should not be used. This is more of belt-and-suspenders, because in // practice these cases should be caught earlier: // - for regular channels at channel announcement (TODO) @@ -1675,7 +1676,7 @@ where L::Target: Logger { { old_entry.value_contribution_msat = value_contribution_msat; } - did_add_update_path_to_src_node = true; + did_add_update_path_to_src_node = Some(value_contribution_msat); } else if old_entry.was_processed && new_cost < old_cost { #[cfg(all(not(ldk_bench), any(test, fuzzing)))] { @@ -1796,7 +1797,7 @@ where L::Target: Logger { for details in first_channels { let candidate = CandidateRouteHop::FirstHop { details }; let added = add_entry!(candidate, our_node_id, payee, 0, path_value_msat, - 0, 0u64, 0, 0); + 0, 0u64, 0, 0).is_some(); log_trace!(logger, "{} direct route to payee via SCID {}", if added { "Added" } else { "Skipped" }, candidate.short_channel_id()); } @@ -1843,6 +1844,7 @@ where L::Target: Logger { let mut aggregate_next_hops_path_penalty_msat: u64 = 0; let mut aggregate_next_hops_cltv_delta: u32 = 0; let mut aggregate_next_hops_path_length: u8 = 0; + let mut aggregate_path_contribution_msat = path_value_msat; for (idx, (hop, prev_hop_id)) in hop_iter.zip(prev_hop_iter).enumerate() { let source = NodeId::from_pubkey(&hop.src_node_id); @@ -1856,10 +1858,13 @@ where L::Target: Logger { }) .unwrap_or_else(|| CandidateRouteHop::PrivateHop { hint: hop }); - if !add_entry!(candidate, source, target, aggregate_next_hops_fee_msat, - path_value_msat, aggregate_next_hops_path_htlc_minimum_msat, + if let Some(hop_used_msat) = add_entry!(candidate, source, target, aggregate_next_hops_fee_msat, + aggregate_path_contribution_msat, + aggregate_next_hops_path_htlc_minimum_msat, aggregate_next_hops_path_penalty_msat, aggregate_next_hops_cltv_delta, aggregate_next_hops_path_length) { + aggregate_path_contribution_msat = hop_used_msat; + } else { // If this hop was not used then there is no use checking the preceding // hops in the RouteHint. We can break by just searching for a direct // channel between last checked hop and first_hop_targets. @@ -1886,15 +1891,13 @@ where L::Target: Logger { .saturating_add(1); // Searching for a direct channel between last checked hop and first_hop_targets - let hint_candidate_contribution_msat = cmp::min(path_value_msat, - candidate.effective_capacity().as_msat().saturating_sub(used_liquidity_msat)); if let Some(first_channels) = first_hop_targets.get_mut(&NodeId::from_pubkey(&prev_hop_id)) { sort_first_hop_channels(first_channels, &used_channel_liquidities, recommended_value_msat, our_node_pubkey); for details in first_channels { let first_hop_candidate = CandidateRouteHop::FirstHop { details }; add_entry!(first_hop_candidate, our_node_id, NodeId::from_pubkey(&prev_hop_id), - aggregate_next_hops_fee_msat, hint_candidate_contribution_msat, + aggregate_next_hops_fee_msat, aggregate_path_contribution_msat, aggregate_next_hops_path_htlc_minimum_msat, aggregate_next_hops_path_penalty_msat, aggregate_next_hops_cltv_delta, aggregate_next_hops_path_length); } @@ -1937,7 +1940,7 @@ where L::Target: Logger { add_entry!(first_hop_candidate, our_node_id, NodeId::from_pubkey(&hop.src_node_id), aggregate_next_hops_fee_msat, - hint_candidate_contribution_msat, + aggregate_path_contribution_msat, aggregate_next_hops_path_htlc_minimum_msat, aggregate_next_hops_path_penalty_msat, aggregate_next_hops_cltv_delta,

Ah, good point. Fixed and added test coverage.

.. in get_route.

Rename EffectiveCapacity::MaximumHTLC to ::AdvertisedMaxHTLC

88821cb

Also get rid of some trailing whitespace because my text editor likes to do that. We'll next add a new variant for max_htlc provided in route hints, which will be treated differently in scoring.

valentinewallace force-pushed the 2023-05-respect-hint-maxhtlc branch from ded3373 to 5207fa4 Compare May 17, 2023 23:27

valentinewallace mentioned this pull request May 17, 2023

Routing to blinded payment paths #2120

Merged

1 task

tnull self-requested a review May 18, 2023 05:13

dunxen previously approved these changes May 18, 2023

View reviewed changes

valentinewallace added this to the 0.0.116 milestone May 18, 2023

valentinewallace dismissed dunxen’s stale review via 409402e May 18, 2023 17:03

valentinewallace force-pushed the 2023-05-respect-hint-maxhtlc branch 2 times, most recently from 409402e to a773553 Compare May 18, 2023 17:07

carlaKC previously approved these changes May 18, 2023

View reviewed changes

valentinewallace dismissed carlaKC’s stale review via 686f4e1 May 18, 2023 19:24

valentinewallace force-pushed the 2023-05-respect-hint-maxhtlc branch from a773553 to 686f4e1 Compare May 18, 2023 19:24

tnull reviewed May 19, 2023

View reviewed changes

lightning/src/routing/router.rs Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

valentinewallace force-pushed the 2023-05-respect-hint-maxhtlc branch from 686f4e1 to 99a123b Compare May 19, 2023 16:53

tnull reviewed May 20, 2023

View reviewed changes

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed May 21, 2023

View reviewed changes

fuzz/src/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

valentinewallace force-pushed the 2023-05-respect-hint-maxhtlc branch from 99a123b to 60987e0 Compare May 22, 2023 14:15

valentinewallace mentioned this pull request May 22, 2023

BOLT12 Offers Tracking Issue #1970

Open

60 tasks

tnull reviewed May 24, 2023

View reviewed changes

valentinewallace force-pushed the 2023-05-respect-hint-maxhtlc branch from 60987e0 to 0d5ac88 Compare May 24, 2023 14:14

tnull previously approved these changes May 24, 2023

View reviewed changes

valentinewallace mentioned this pull request May 25, 2023

Allow forwarding less than the amount in the onion #2319

Merged

TheBlueMatt reviewed May 31, 2023

View reviewed changes

Abide by route hint max_htlc in pathfinding

14cd18b

Account for used liquidity in first hops when processing route hints

f130f95

.. in get_route.

valentinewallace dismissed tnull’s stale review via f130f95 June 6, 2023 13:17

valentinewallace force-pushed the 2023-05-respect-hint-maxhtlc branch from 0d5ac88 to f130f95 Compare June 6, 2023 13:17

TheBlueMatt approved these changes Jun 6, 2023

View reviewed changes

tnull approved these changes Jun 7, 2023

View reviewed changes

valentinewallace merged commit 7a78998 into lightningdevkit:main Jun 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect route hint `max_htlc` in pathfinding #2305

Respect route hint `max_htlc` in pathfinding #2305

valentinewallace commented May 17, 2023 •

edited

Loading

codecov-commenter commented May 18, 2023 •

edited

Loading

dunxen left a comment

valentinewallace commented May 18, 2023

dunxen commented May 18, 2023

carlaKC left a comment

carlaKC May 18, 2023

valentinewallace May 18, 2023 •

edited

Loading

valentinewallace commented May 18, 2023

valentinewallace commented May 19, 2023

tnull left a comment

tnull left a comment

valentinewallace commented May 24, 2023

TheBlueMatt left a comment

TheBlueMatt May 31, 2023

valentinewallace Jun 5, 2023

TheBlueMatt Jun 5, 2023

valentinewallace Jun 6, 2023

Respect route hint max_htlc in pathfinding #2305

Respect route hint max_htlc in pathfinding #2305

Conversation

valentinewallace commented May 17, 2023 • edited Loading

codecov-commenter commented May 18, 2023 • edited Loading

Codecov Report

dunxen left a comment

Choose a reason for hiding this comment

valentinewallace commented May 18, 2023

dunxen commented May 18, 2023

carlaKC left a comment

Choose a reason for hiding this comment

carlaKC May 18, 2023

Choose a reason for hiding this comment

valentinewallace May 18, 2023 • edited Loading

Choose a reason for hiding this comment

valentinewallace commented May 18, 2023

valentinewallace commented May 19, 2023

tnull left a comment

Choose a reason for hiding this comment

Notebook (Apple M1 Pro):

RUN #1:

BASE:

HEAD:

RUN #2:

BASE:

HEAD:

Server (Intel(R) Xeon(R) CPU E3-1226 v3 @ 3.30GHz):

RUN #1:

BASE:

HEAD:

RUN #2:

BASE:

HEAD:

tnull left a comment

Choose a reason for hiding this comment

valentinewallace commented May 24, 2023

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt May 31, 2023

Choose a reason for hiding this comment

valentinewallace Jun 5, 2023

Choose a reason for hiding this comment

TheBlueMatt Jun 5, 2023

Choose a reason for hiding this comment

valentinewallace Jun 6, 2023

Choose a reason for hiding this comment

Respect route hint `max_htlc` in pathfinding #2305

Respect route hint `max_htlc` in pathfinding #2305

valentinewallace commented May 17, 2023 •

edited

Loading

codecov-commenter commented May 18, 2023 •

edited

Loading

valentinewallace May 18, 2023 •

edited

Loading