Router fixes #1398

jkczyz · 2022-03-30T19:51:48Z

Various fixes to find_route and a commit to remove build warnings in cargo test.

codecov-commenter · 2022-03-30T20:17:32Z

Codecov Report

Merging #1398 (912242d) into main (7671ae5) will increase coverage by 0.08%.
The diff coverage is 50.00%.

❗ Current head 912242d differs from pull request most recent head 8ddfe66. Consider uploading reports for the commit 8ddfe66 to get more accurate results

@@            Coverage Diff             @@
##             main    #1398      +/-   ##
==========================================
+ Coverage   90.76%   90.85%   +0.08%     
==========================================
  Files          73       73              
  Lines       41195    41439     +244     
  Branches    41195    41439     +244     
==========================================
+ Hits        37392    37648     +256     
+ Misses       3803     3791      -12

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	`88.37% <ø> (+0.07%)`	⬆️
lightning/src/ln/channelmanager.rs	`84.94% <ø> (+0.19%)`	⬆️
lightning/src/routing/router.rs	`92.49% <50.00%> (-0.14%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.14% <0.00%> (+0.06%)`	⬆️
lightning-persister/src/lib.rs	`93.93% <0.00%> (+0.33%)`	⬆️
lightning/src/routing/network_graph.rs	`89.61% <0.00%> (+0.55%)`	⬆️
lightning-background-processor/src/lib.rs	`95.35% <0.00%> (+2.25%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7671ae5...8ddfe66. Read the comment docs.

TheBlueMatt · 2022-03-30T21:01:46Z

Actually, one more thing, on L1552, can you add a .saturating_add(path.get_path_penalty_msat())?

lightning/src/routing/router.rs

TheBlueMatt · 2022-03-30T22:13:05Z

lightning/src/routing/router.rs

@@ -917,7 +917,7 @@ where L::Target: Logger {
 					// Since we're choosing amount_to_transfer_over_msat as maximum possible, it can
 					// be only reduced later (not increased), so this channel should just be skipped
 					// as not sufficient.
-					if !over_path_minimum_msat && doesnt_exceed_cltv_delta_limit {
+					if contributes_sufficient_value && !over_path_minimum_msat && doesnt_exceed_cltv_delta_limit {


Oh, this isn't quite sufficient, either, lets also add a && recommended_value_msat <= both of the two comparisons for over_path_minimum_msat`

As in, don't bother setting hit_minimum_limit unless we both were not under the path minimum msat, and also the next amount we'll try (the recommended_value_msat amount) is below the path minimum msat (and channel minimum msat).

Specifically, the issue here, I believe, is that some people are disabling channels by setting the minimum to 21 million bitcoin, and as a result we're always setting hit_minimum_limit and going around the dijkstras again, which is nonsense.

Is this what you mean? Seeing a lot of failing tests.

diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 5a4ddb26..4d79cf63 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -913,8 +913,11 @@ where L::Target: Logger { None => unreachable!(), }; #[allow(unused_comparisons)] // $next_hops_path_htlc_minimum_msat is 0 in some calls so rustc complains - let over_path_minimum_msat = amount_to_transfer_over_msat >= $candidate.htlc_minimum_msat() && - amount_to_transfer_over_msat >= $next_hops_path_htlc_minimum_msat; + let over_path_minimum_msat = + amount_to_transfer_over_msat >= $candidate.htlc_minimum_msat() && + amount_to_transfer_over_msat >= $next_hops_path_htlc_minimum_msat && + recommended_value_msat <= $candidate.htlc_minimum_msat() && + recommended_value_msat <= $next_hops_path_htlc_minimum_msat; // If HTLC minimum is larger than the amount we're going to transfer, we shouldn't // bother considering this channel.

Oh, yes, that's what i meant, but not what I should have meant. I should have meant to put the new &&... stuff on the if statement there, not in the over_path_minimum_msat, that makes no sense. With that tests seem to pass. (Feel free to move the if around so that its more readable).

Okay, its a bittt more subtle for a few reasons, but this works:

let over_path_minimum_msat = amount_to_transfer_over_msat >= $candidate.htlc_minimum_msat() && amount_to_transfer_over_msat >= $next_hops_path_htlc_minimum_msat; #[allow(unused_comparisons)] // $next_hops_path_htlc_minimum_msat is 0 in some calls so rustc complains let may_overpay_to_meet_path_minimum_msat = ((amount_to_transfer_over_msat < $candidate.htlc_minimum_msat() && recommended_value_msat > $candidate.htlc_minimum_msat()) || (amount_to_transfer_over_msat < $next_hops_path_htlc_minimum_msat && recommended_value_msat > $next_hops_path_htlc_minimum_msat)); ..... if contributes_sufficient_value && may_overpay_to_meet_path_minimum_msat && doesnt_exceed_cltv_delta_limit { hit_minimum_limit = true; } else if contributes_sufficient_value && doesnt_exceed_cltv_delta_limit && over_path_minimum_msat {

In english: (a) we need to add an over_path_minimum_msat comparison on the second if, as we shouldn't use a channel at all if its not over the path min, even if we don't set hit_minimum_limit, (b) the new checks need to apply on the equivalent path-specific comparison - if we fail to meet the path htlc min, then recommended_value_msat has to be over the path htlc min (but not also the candidate min), if we fail to meet the hop htlc min, then the recommended value has to be over the hop htlc min (not the path htlc min).

I added a new variable above to make it a bit more readable, but its not clear to me that its the most readable version of the logic, feel free to swap it around some if you see a better way.

I just swapped the ordering within the first if-condition so that it is symmetric with the second.

For even-length paths, preferring a later hop avoids overly limiting the possible paths on the next iteration.

valentinewallace · 2022-03-31T01:43:38Z

lightning/src/routing/router.rs

@@ -917,7 +917,7 @@ where L::Target: Logger {
 					// Since we're choosing amount_to_transfer_over_msat as maximum possible, it can
 					// be only reduced later (not increased), so this channel should just be skipped
 					// as not sufficient.
-					if !over_path_minimum_msat && doesnt_exceed_cltv_delta_limit {
+					if contributes_sufficient_value && !over_path_minimum_msat && doesnt_exceed_cltv_delta_limit {


Taking a stab at a clearer commit message (sorry long): "During the first pass of path finding, we seek a single path with the exact payment amount, and only seek additional paths if (a) no single path can carry the entire balance of the payment or (b) we found a good path, but along the way we found candidate paths with the potential to result in a lower total fee. This commit fixes the behavior of (b) -- we were previously considering some paths to be candidates for a lower fee when in fact they never would have worked. This caused us to re-run Dijkstra's when it might not have been beneficial."

Thanks, @valentinewallace! @TheBlueMatt I took this for the commit message. Let me know if this sounds good to you.

jkczyz · 2022-03-31T03:16:22Z

FYI, I re-ordered the commits a bit.

TheBlueMatt

Thanks for the quick turnaround!

lightning/src/routing/router.rs

valentinewallace · 2022-03-31T03:35:52Z

lightning/src/routing/router.rs

+					#[allow(unused_comparisons)] // $next_hops_path_htlc_minimum_msat is 0 in some calls so rustc complains
+					let may_overpay_to_meet_path_minimum_msat =
+						((amount_to_transfer_over_msat < $candidate.htlc_minimum_msat() &&
+						  recommended_value_msat > $candidate.htlc_minimum_msat()) ||


Fwiw I think this can be >= and below.

Would it be a good first issue to add a test for this case?

I dunno if that's a good first issue, but an issue yes. Kinda convoluted to hit those cases, though a trivial "make sure we don't run twice if someone disabled a channel by setting the min-HTLC to 21m BTC" test shouldn't be too much work.

lightning/src/routing/router.rs

TheBlueMatt

LGTM. I'd say feel free to squash. cc @valentinewallace

Selecting only by fees rather than cost (fees plus penalty) may result in preferring higher cost routes over lower cost ones.

During the first pass of path finding, we seek a single path with the exact payment amount, and only seek additional paths if (a) no single path can carry the entire balance of the payment or (b) we found a good path, but along the way we found candidate paths with the potential to result in a lower total fee. This commit fixes the behavior of (b) -- we were previously considering some paths to be candidates for a lower fee when in fact they never would have worked. This caused us to re-run Dijkstra's when it might not have been beneficial.

arik-so · 2022-04-01T16:46:47Z

lightning/src/routing/router.rs

@@ -912,14 +916,20 @@ where L::Target: Logger {
 					let over_path_minimum_msat = amount_to_transfer_over_msat >= $candidate.htlc_minimum_msat() &&
 						amount_to_transfer_over_msat >= $next_hops_path_htlc_minimum_msat;

+					#[allow(unused_comparisons)] // $next_hops_path_htlc_minimum_msat is 0 in some calls so rustc complains
+					let may_overpay_to_meet_path_minimum_msat =
+						((amount_to_transfer_over_msat < $candidate.htlc_minimum_msat() &&


this is a rather convoluted comparison. Do you think something like what you do in line 930, having each boolean value be assigned to a variable (maybe with some comments for each one) would improve legibility?

I imagine it can be broken up into the two variables where one is $candidate.htlc_minimum_msat() falls between amount_to_transfer_over_msat and recommended_value_msat and the other is the same but for $next_hops_path_htlc_minimum_msat. Do you have any recommendations on naming either?

is_htlc_minimum_msat_within_tolerance_range perhaps?

Hmm, that doesn't seem any more clear to me - its not a "tolerance" range more of a "well, its within the range where we should try to meet it instead of ignoring the channel" range.

well, nomenclature aside, the logic LGTM, so I'm gonna approve and then re-ack if we wanna change this part

Lets land it - its blocking release and its probably clearer than it was given there's now some comment verbiage lol.

arik-so · 2022-04-01T19:14:00Z

lightning/src/routing/router.rs

@@ -912,14 +916,20 @@ where L::Target: Logger {
 					let over_path_minimum_msat = amount_to_transfer_over_msat >= $candidate.htlc_minimum_msat() &&
 						amount_to_transfer_over_msat >= $next_hops_path_htlc_minimum_msat;

+					#[allow(unused_comparisons)] // $next_hops_path_htlc_minimum_msat is 0 in some calls so rustc complains
+					let may_overpay_to_meet_path_minimum_msat =
+						((amount_to_transfer_over_msat < $candidate.htlc_minimum_msat() &&


well, nomenclature aside, the logic LGTM, so I'm gonna approve and then re-ack if we wanna change this part

jkczyz added this to the 0.0.106 milestone Mar 30, 2022

TheBlueMatt previously approved these changes Mar 30, 2022

View reviewed changes

valentinewallace reviewed Mar 30, 2022

View reviewed changes

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

TheBlueMatt reviewed Mar 30, 2022

View reviewed changes

jkczyz added 2 commits March 30, 2022 17:41

Fix build warnings without grind_signatures

37a947b

Correctly pick middle hop to victimize

30b6873

For even-length paths, preferring a later hop avoids overly limiting the possible paths on the next iteration.

valentinewallace reviewed Mar 31, 2022

View reviewed changes

jkczyz dismissed TheBlueMatt’s stale review via d4c6b10 March 31, 2022 03:12

jkczyz force-pushed the 2022-03-middle-hop-fix branch from 1e09974 to d4c6b10 Compare March 31, 2022 03:12

TheBlueMatt previously approved these changes Mar 31, 2022

View reviewed changes

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

jkczyz dismissed TheBlueMatt’s stale review via 39f1470 March 31, 2022 14:32

jkczyz force-pushed the 2022-03-middle-hop-fix branch from d4c6b10 to 39f1470 Compare March 31, 2022 14:32

valentinewallace reviewed Mar 31, 2022

View reviewed changes

TheBlueMatt reviewed Mar 31, 2022

View reviewed changes

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

TheBlueMatt reviewed Apr 1, 2022

View reviewed changes

jkczyz and others added 2 commits March 31, 2022 21:36

Select best route by lowest total cost

f9cdf93

Selecting only by fees rather than cost (fees plus penalty) may result in preferring higher cost routes over lower cost ones.

jkczyz force-pushed the 2022-03-middle-hop-fix branch from 912242d to 8ddfe66 Compare April 1, 2022 02:36

TheBlueMatt approved these changes Apr 1, 2022

View reviewed changes

valentinewallace approved these changes Apr 1, 2022

View reviewed changes

arik-so reviewed Apr 1, 2022

View reviewed changes

arik-so approved these changes Apr 1, 2022

View reviewed changes

TheBlueMatt merged commit 6b8ad4e into lightningdevkit:main Apr 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Router fixes #1398

Router fixes #1398

jkczyz commented Mar 30, 2022

codecov-commenter commented Mar 30, 2022 •

edited

Loading

TheBlueMatt commented Mar 30, 2022

TheBlueMatt Mar 30, 2022

TheBlueMatt Mar 30, 2022

TheBlueMatt Mar 30, 2022

jkczyz Mar 30, 2022

TheBlueMatt Mar 30, 2022

TheBlueMatt Mar 31, 2022

jkczyz Mar 31, 2022

valentinewallace Mar 31, 2022

jkczyz Mar 31, 2022

jkczyz commented Mar 31, 2022

TheBlueMatt left a comment

valentinewallace Mar 31, 2022

TheBlueMatt Mar 31, 2022 •

edited

Loading

TheBlueMatt left a comment

arik-so Apr 1, 2022

jkczyz Apr 1, 2022

arik-so Apr 1, 2022

TheBlueMatt Apr 1, 2022

arik-so Apr 1, 2022

TheBlueMatt Apr 1, 2022

arik-so Apr 1, 2022

Router fixes #1398

Router fixes #1398

Conversation

jkczyz commented Mar 30, 2022

codecov-commenter commented Mar 30, 2022 • edited Loading

Codecov Report

TheBlueMatt commented Mar 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz commented Mar 31, 2022

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt Mar 31, 2022 • edited Loading

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 30, 2022 •

edited

Loading

TheBlueMatt Mar 31, 2022 •

edited

Loading