Fix router fuzz failure #2570

valentinewallace · 2023-09-12T19:04:30Z

Fixes #2523 and another panic from fuzz input https://pastebin.com/9ZkmyzHE. See test comments for more details on the fixes.

TheBlueMatt · 2023-09-12T21:53:30Z

LGTM, but will give this some fuzzing cycles to see if it finds anything else. Will wait to assign a second reviewer for a minute cause folks are super busy lol.

valentinewallace · 2023-09-15T17:44:26Z

Added another fuzz fix, see latest commit.

codecov-commenter · 2023-09-15T19:39:08Z

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (0f87ee8) 88.97% compared to head (5e742b6) 88.98%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #2570    +/-   ##
========================================
  Coverage   88.97%   88.98%            
========================================
  Files         113      113            
  Lines       85890    86292   +402     
  Branches    85890    86292   +402     
========================================
+ Hits        76424    76784   +360     
- Misses       7239     7270    +31     
- Partials     2227     2238    +11

Files	Coverage Δ
lightning/src/routing/router.rs	`93.84% <98.37%> (+0.36%)`	⬆️

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tnull

LGTM, checked that changes fix the otherwise failing tests, and that they play together nicely with #2575 after a trivial rebase.

valentinewallace · 2023-09-18T15:34:15Z

Thanks for taking a look! I'm gonna tack on one more fuzz fix that changes the last commit slightly.

valentinewallace · 2023-09-19T20:10:07Z

Rewrote the last commit to generalize the solution and added another test courtesy of the fuzzer.

lightning/src/routing/router.rs

TheBlueMatt · 2023-09-21T19:28:29Z

Ugh, this is failing again - https://pastebin.com/XyrjGm3U

The fuzzer managed to hit this and it causes some invalid paths to be generated internally.

Because my text editor loves to do that.

valentinewallace · 2023-09-27T13:44:00Z

I fuzzed this latest branch overnight and it didn't find anything, when previously it found a crash within an hour. Also rebased to fix conflicts.

tnull · 2023-09-27T14:15:55Z

lightning/src/routing/router.rs

@@ -1280,6 +1284,7 @@ impl<'a> PaymentPath<'a> {

 			let cur_hop = &mut self.hops.get_mut(i).unwrap().0;
 			cur_hop.next_hops_fee_msat = total_fee_paid_msat;
+			cur_hop.path_penalty_msat += extra_contribution_msat;


I don't quite understand the extra penalization here. Wouldn't the path just be deprioritized anyways as it's higher cost?

It’s not higher cost with this solution because we don’t add the overpayment amount to the fees, just to the path contribution amount. The test fails without this as it will select an overpaying path when a path for the exact payment value is available.

Ah, makes sense. Doesn't make update_value_and_recompute_fees much more readable though..

Maybe would be worth another comment to explain why we carve out the last hop?

Yeah, I'm tempted to try to refactor this method, it's not fun to parse..

Maybe would be worth another comment to explain why we carve out the last hop?

I kept your comment stating that we return the extra fees to account for them in the path's value contribution. Is something missing there?

I kept your comment stating that we return the extra fees to account for them in the path's value contribution. Is something missing there?

Could mention why we don't count the fees in this case, but probably doesn't make a big difference. Fine to leave as is.

lightning/src/routing/router.rs

tnull

So far looks good to me, I checked that rebasing #2575 works without issue if I drop the first commit there, that the LDK Node issue is still fixed, and that #2604 rebased on top of the rebase still has all tests pass. Only nit would be that I'd prefer the more general solution in 7144866, but of course we'll want to keep the tests here.

So, how should we go about landing all these? 😁

Edit: Rebased #2575 on top of this and included the requested change as part of that.

valentinewallace · 2023-09-27T15:22:47Z

Pushed a fix for 1.48 CI:

diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs
index 17df884de..79a51b66a 100644
--- a/lightning/src/routing/router.rs
+++ b/lightning/src/routing/router.rs
@@ -7570,12 +7570,12 @@ mod tests {
                                ],
                        };
                        let mut blinded_hints = Vec::new();
-                       for htlc_min in htlc_mins {
+                       for htlc_min in htlc_mins.iter() {
                                blinded_hints.push((BlindedPayInfo {
                                        fee_base_msat: 0,
                                        fee_proportional_millionths: 0,
-                                       htlc_minimum_msat: htlc_min,
-                                       htlc_maximum_msat: htlc_min * 100,
+                                       htlc_minimum_msat: *htlc_min,
+                                       htlc_maximum_msat: *htlc_min * 100,
                                        cltv_expiry_delta: 10,
                                        features: BlindedHopFeatures::empty(),
                                }, blinded_path.clone()));

See tests, but the fuzzer found several panics from not fully ignoring these hints. We should support these route hints eventually, but it will involve some reworking of the Path/BlindedTail structs.

Previously this variable was a bool, but has since been updated to be an Option, so rename accordingly.

Previously, we would add a candidate hop to the list of potential hops even though its available contribution wasn't sufficient to meet the next hop's min_htlc. We'd subsequently build an invalid path using this hop and hit a debug assertion.

See previous commit, but the bug where we would underestimate how much a first hop candidate needed to be able to relay was also present in blinded paths.

Previously, we would add a first_hop<>network_node channel that did not have enough contribution amount to cover the next channel's min htlc plus fees, because we were storing the next hop as having a path_min that did not include fees, and would add a connecting first_hop node that did not have enough contribution amount, leading to a debug panic upon invalid path construction.

Previously, the fuzzer hit a debug panic because we wouldn't include the amount overpaid to meet a last hop's min_htlc in the total collected paths value. We now include this value and also penalize hops along the overpaying path to ensure that it gets deprioritized in path selection.

valentinewallace · 2023-09-27T17:26:31Z

Updated to make 2 tests more robust, later changes would make them spuriously succeed if the original change was reverted.

valentinewallace · 2023-09-27T17:51:55Z

lightning/src/routing/router.rs

+		// Values are taken from the fuzz input that uncovered this panic.
+		let amt_msat = 7_4009_8048;
+		let (_, our_id, _, nodes) = get_nodes(&secp_ctx);
+		let first_hop_outbound_capacity = 2_7345_2000;


@TheBlueMatt this was an existing issue before routing to blinded paths was supported. ISTM the router fuzzer should've caught this previously?

Yea, that's my impression too, not sure why I wasn't able to repro prior to the changes in 117.

tnull

LGTM

tnull · 2023-09-27T19:13:42Z

lightning/src/routing/router.rs

@@ -1280,6 +1284,7 @@ impl<'a> PaymentPath<'a> {

 			let cur_hop = &mut self.hops.get_mut(i).unwrap().0;
 			cur_hop.next_hops_fee_msat = total_fee_paid_msat;
+			cur_hop.path_penalty_msat += extra_contribution_msat;


I kept your comment stating that we return the extra fees to account for them in the path's value contribution. Is something missing there?

Could mention why we don't count the fees in this case, but probably doesn't make a big difference. Fine to leave as is.

TheBlueMatt

LGTM. If there's any more failures we can fix them later.

valentinewallace added this to the 0.0.117 milestone Sep 12, 2023

valentinewallace force-pushed the 2023-08-router-fuzz-fail branch from 4fb32d4 to a0d7cc7 Compare September 12, 2023 19:31

valentinewallace force-pushed the 2023-08-router-fuzz-fail branch from a0d7cc7 to 6b917c6 Compare September 15, 2023 17:44

tnull previously approved these changes Sep 18, 2023

View reviewed changes

valentinewallace dismissed tnull’s stale review via 905387b September 19, 2023 19:53

valentinewallace force-pushed the 2023-08-router-fuzz-fail branch from 6b917c6 to 905387b Compare September 19, 2023 19:53

TheBlueMatt reviewed Sep 19, 2023

View reviewed changes

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-08-router-fuzz-fail branch from 905387b to 31bf6b1 Compare September 21, 2023 17:00

valentinewallace mentioned this pull request Sep 26, 2023

Various router fixes and #2417 follow-ups #2575

Merged

valentinewallace force-pushed the 2023-08-router-fuzz-fail branch from 31bf6b1 to f1741a8 Compare September 27, 2023 13:32

valentinewallace added 3 commits September 27, 2023 09:37

Pathfinding: log when we ignore one hop blinded route hints

6cfb052

Refuse to pathfind when provided our_node_id matches internal dummy pk

7632cfe

The fuzzer managed to hit this and it causes some invalid paths to be generated internally.

Remove trailing whitespace in get_route

b16d6a1

Because my text editor loves to do that.

valentinewallace force-pushed the 2023-08-router-fuzz-fail branch from f1741a8 to 5e742b6 Compare September 27, 2023 13:40

tnull reviewed Sep 27, 2023

View reviewed changes

valentinewallace force-pushed the 2023-08-router-fuzz-fail branch from 5e742b6 to 6017660 Compare September 27, 2023 15:22

valentinewallace added 5 commits September 27, 2023 13:19

Pathfinding: ignore blinded route hints where we are the intro node

29c67d5

See tests, but the fuzzer found several panics from not fully ignoring these hints. We should support these route hints eventually, but it will involve some reworking of the Path/BlindedTail structs.

get_route: fix outdated var name

5263b07

Previously this variable was a bool, but has since been updated to be an Option, so rename accordingly.

get_route: fix path_min for first_hop<>blinded_hint candidates

d83295f

See previous commit, but the bug where we would underestimate how much a first hop candidate needed to be able to relay was also present in blinded paths.

valentinewallace force-pushed the 2023-08-router-fuzz-fail branch from 6017660 to f3857d0 Compare September 27, 2023 17:25

valentinewallace commented Sep 27, 2023

View reviewed changes

tnull approved these changes Sep 27, 2023

View reviewed changes

TheBlueMatt approved these changes Sep 27, 2023

View reviewed changes

TheBlueMatt merged commit 7b4fb9d into lightningdevkit:main Sep 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix router fuzz failure #2570

Fix router fuzz failure #2570

valentinewallace commented Sep 12, 2023 •

edited

Loading

TheBlueMatt commented Sep 12, 2023

valentinewallace commented Sep 15, 2023

codecov-commenter commented Sep 15, 2023 •

edited

Loading

tnull left a comment •

edited

Loading

valentinewallace commented Sep 18, 2023

valentinewallace commented Sep 19, 2023

TheBlueMatt commented Sep 21, 2023

valentinewallace commented Sep 27, 2023

tnull Sep 27, 2023

valentinewallace Sep 27, 2023

tnull Sep 27, 2023

valentinewallace Sep 27, 2023

tnull Sep 27, 2023

tnull left a comment •

edited

Loading

valentinewallace commented Sep 27, 2023

valentinewallace commented Sep 27, 2023

valentinewallace Sep 27, 2023 •

edited

Loading

TheBlueMatt Sep 27, 2023

tnull left a comment

tnull Sep 27, 2023

TheBlueMatt left a comment

Fix router fuzz failure #2570

Fix router fuzz failure #2570

Conversation

valentinewallace commented Sep 12, 2023 • edited Loading

TheBlueMatt commented Sep 12, 2023

valentinewallace commented Sep 15, 2023

codecov-commenter commented Sep 15, 2023 • edited Loading

Codecov Report

tnull left a comment • edited Loading

Choose a reason for hiding this comment

valentinewallace commented Sep 18, 2023

valentinewallace commented Sep 19, 2023

TheBlueMatt commented Sep 21, 2023

valentinewallace commented Sep 27, 2023

tnull Sep 27, 2023

Choose a reason for hiding this comment

valentinewallace Sep 27, 2023

Choose a reason for hiding this comment

tnull Sep 27, 2023

Choose a reason for hiding this comment

valentinewallace Sep 27, 2023

Choose a reason for hiding this comment

tnull Sep 27, 2023

Choose a reason for hiding this comment

tnull left a comment • edited Loading

Choose a reason for hiding this comment

valentinewallace commented Sep 27, 2023

valentinewallace commented Sep 27, 2023

valentinewallace Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

TheBlueMatt Sep 27, 2023

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

tnull Sep 27, 2023

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

valentinewallace commented Sep 12, 2023 •

edited

Loading

codecov-commenter commented Sep 15, 2023 •

edited

Loading

tnull left a comment •

edited

Loading

tnull left a comment •

edited

Loading

valentinewallace Sep 27, 2023 •

edited

Loading