Limit route hints to three channels #2044

freddiekrugerrand · 2023-02-22T14:11:46Z

This PR makes the following changes to hint selection:

Only include the top 3 channels from this sorted list
Select up to 3 hints from distinct nodes for phantom invoices.
~~Sort eligible channels by available inbound capacity: Rationale: Inbound capacity seems like a rational value to sort by, and it makes selection deterministic for easier testing.~~ See discussion.

Fixes #1782, replaces #1919.

freddiekrugerrand · 2023-02-22T14:14:04Z

This PR does not limit the number of hints for phantom invoices to one.
New to rust/LDK, so I'm still figuring out how to do it without breaking tests.

lightning-invoice/src/utils.rs

TheBlueMatt · 2023-02-22T23:10:59Z

New to rust/LDK, so I'm still figuring out how to do it without #1919 (comment).

Those tests are super prescriptive, which is okay, but it probably means any change will result in the tests breaking, which means you'll just need to update them :).

TheBlueMatt

LGTM, but, yea, would be cool to limit the phantom hints as well.

lightning-invoice/src/utils.rs

freddiekrugerrand · 2023-02-23T19:01:28Z

Would be cool to limit the phantom hints as well.

Working on adding this + updating phantom tests!

codecov-commenter · 2023-02-24T22:17:33Z

Codecov Report

Patch coverage: 98.66% and project coverage change: -0.03 ⚠️

Comparison is base (0e28bcb) 91.38% compared to head (1af848f) 91.36%.

❗ Current head 1af848f differs from pull request most recent head 905cd8b. Consider uploading reports for the commit 905cd8b to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2044      +/-   ##
==========================================
- Coverage   91.38%   91.36%   -0.03%     
==========================================
  Files         102      102              
  Lines       49767    50878    +1111     
  Branches    49767    50878    +1111     
==========================================
+ Hits        45482    46487    +1005     
- Misses       4285     4391     +106

Impacted Files	Coverage Δ
lightning-invoice/src/utils.rs	`97.41% <98.66%> (+0.35%)`	⬆️

... and 45 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

lightning-invoice/src/utils.rs

benthecarman · 2023-02-28T20:22:13Z

This PR makes the following changes to hint selection:
* Sort eligible channels by available inbound capacity

* Only include the top 3 channels from this sorted list

This is a pretty huge privacy leak, this will reveal your largest channels and can be a fingerprint for wallets. Would be better to add some randomness imo.

jkczyz · 2023-02-28T21:35:12Z

This is a pretty huge privacy leak, this will reveal your largest channels and can be a fingerprint for wallets. Would be better to add some randomness imo.

We can probably get away without sorting given the eligible channels should have enough capacity unless none do. Then it's just a matter of probability of success vs privacy tradeoff.

freddiekrugerrand · 2023-03-01T16:02:47Z

lightning-invoice/src/utils.rs

+	// hints across our nodes we add one hint from each node in turn until no node has any hints
+	// left (if one node has more hints than any other, these will accumulate at the end of the
+	// vector).
+	let mut invoice_hints :Vec<RouteHint> = Vec::new();


I suspect there is a smarter way to do this with iterators, but couldn't find an elegant one without using itertools interleave (not currently a dependency?), and even that doesn't work well for > 2 sets. New to rust, so certainly open to better suggestions!

Not sure if there is an easy way to do this without writing a custom Interleave iterator.

Regardless, we shouldn't clone anything we don't ultimately take. So at very least we should push references into a Vec<&RouteHint> and later do invoice_hints.into_iter().take(3).cloned().collect().

Yeah, that seems like a fair solution right now.

freddiekrugerrand · 2023-03-01T16:05:14Z

Updated PR:

Fix tabs + typos
Remove sort by inbound (+ update tests to just do length assertions)
Limit phantom invoice hint selection to 3 hints (selecting one per phantom node until we reach 3)

Two refactor commits can probably be squashed into one.

lightning-invoice/src/utils.rs

jkczyz · 2023-03-01T19:45:33Z

lightning-invoice/src/utils.rs

+	loop{	
+	    let mut remaining_hints = false;
+
+	    for hints in phantom_hints.iter(){
+		if hint_idx < hints.len(){
+		    invoice_hints.push(hints[hint_idx].clone());
+		    remaining_hints = true;
+		}
+	    }
+
+	    if !remaining_hints{
+		break
+	    }
+
+	    hint_idx +=1;
 	}
+
+	invoice_hints.iter().take(3).map(|x| x.clone()).collect()


No need for the map and clone if you use into_iter instead of iter. The difference is that it consumes the Vec so you get values instead of references.

lightning-invoice/src/utils.rs

jkczyz · 2023-03-02T04:53:29Z

lightning-invoice/src/utils.rs

+	// We have one vector per phantom node involved in creating the invoice. To distribute the
+	// hints across our nodes we add one hint from each node in turn until no node has any hints
+	// left (if one node has more hints than any other, these will accumulate at the end of the
+	// vector).


This is not quite accurate. There is only one phantom node but many real nodes, each with its own PhantomRouteHints. Essentially, the channels represent channels to real_node_pubkey, which has a fake channel with phantom_scid to the phantom node.

jkczyz · 2023-03-02T05:13:25Z

lightning-invoice/src/utils.rs

+	// hints across our nodes we add one hint from each node in turn until no node has any hints
+	// left (if one node has more hints than any other, these will accumulate at the end of the
+	// vector).
+	let mut invoice_hints :Vec<RouteHint> = Vec::new();


Not sure if there is an easy way to do this without writing a custom Interleave iterator.

Regardless, we shouldn't clone anything we don't ultimately take. So at very least we should push references into a Vec<&RouteHint> and later do invoice_hints.into_iter().take(3).cloned().collect().

dunxen · 2023-03-02T19:42:38Z

lightning-invoice/src/utils.rs

@@ -200,6 +202,37 @@ where
 		invoice = invoice.amount_milli_satoshis(amt);
 	}

+	for route_hint in select_phantom_hints(amt_msat, phantom_route_hints, logger) {
+		invoice = invoice.private_route(route_hint.clone());


I believe we can just let route_hint move here and don't need to clone.

Suggested change

invoice = invoice.private_route(route_hint.clone());

invoice = invoice.private_route(route_hint);

dunxen · 2023-03-02T20:46:14Z

This is looking good to me now besides a few minor formatting nits. I'll give it another pass once reviewers with more context have checked again.

TheBlueMatt · 2023-03-06T19:22:57Z

lightning-invoice/src/utils.rs

@@ -629,6 +677,7 @@ fn filter_channels<L: Deref>(

 			include_channel
 		})
+		.take(3)


I believe this is picking three nodes at random (i.e. based on the order they appear in the hash map), I'm not sure we want to do that, ideally we should have some kind of sorting logic instead imo.

Originally sorted by inbound but removed that due to privacy concerns raised.

I think sorting by some factor that makes a channel more desirable for hints makes sense -maybe just channel capacity?

If we sort by something unrelated to the ability to route, then maybe some randomness is just as good?

I think channel capacity is a decent privacy vs utility trade-off actually, otherwise yeah I'd say it would probably just be fine to have it random.

Sorry missed that comment, left a top level comment with my thinking.

lightning-invoice/src/utils.rs

dunxen

Nice. I've read through the comments on some functions and picked up some nits. Feel free to address them in the impl Iterator follow-up PR.

dunxen · 2023-04-04T09:09:17Z

lightning-invoice/src/utils.rs

-				let new_higher_capacity = channel.is_public == entry.get().is_public &&
-					channel.inbound_capacity_msat > current_max_capacity;
-				if new_now_public || new_higher_capacity {
+				// `new_now_public), and this channel has more desirable inbound that the incumbent,


nit: that -> than

dunxen · 2023-04-04T09:10:23Z

lightning-invoice/src/utils.rs

+/// prefer_current_channel chooses a channel to use for route hints between a currently selected and candidate
+/// channel based on the inbound capacity of each channel and the minimum inbound capacity requested for the hints,
+/// returning true if the current channel should be preferred over the candidate channel.
+/// * If no minimum amount is requested, the channel with the most inbound is chose to maximize the chances that a


nit: chose -> chosen

TheBlueMatt · 2023-04-04T17:29:46Z

Oops, there's a dead doc link:

error: unresolved link to `PhantomKeysManager`
   --> lightning-invoice/src/utils.rs:222:11
    |
222 | /// See [`PhantomKeysManager`] for more information on phantom node payments.
    |           ^^^^^^^^^^^^^^^^^^ no item named `PhantomKeysManager` in scope
    |

jkczyz

Mostly just nits.

jkczyz · 2023-04-05T14:54:01Z

lightning-invoice/src/utils.rs

-					channel.inbound_capacity_msat > current_max_capacity;
-				if new_now_public || new_higher_capacity {
+				// `new_now_public), and this channel has more desirable inbound than the incumbent,
+				// prefer to announce this channel.


s/announce/include

jkczyz · 2023-04-05T14:55:19Z

lightning-invoice/src/utils.rs

@@ -570,12 +574,17 @@ fn filter_channels<L: Deref>(
 				// If this channel is public and the previous channel is not, ensure we replace the
 				// previous channel to avoid announcing non-public channels.
 				let new_now_public = channel.is_public && !entry.get().is_public;
+				// Decide whether we prefer the currently selected channel with the node to the new one,
+				// based on their inbound capacity. A scaling factor of 10% to pad our minimum inbound


s/A scaling factor/Add a scaling factor

Though arguably this detail doesn't need to be mentioned here.

jkczyz · 2023-04-05T14:58:41Z

lightning-invoice/src/utils.rs

+///   percentage)
+///   then we choose the lowest inbound channel with above this amount. If we have sufficient inbound channels, we don't
+///   want to deplete our larger channels with small payments (the off-chain version of "grinding our change").


jkczyz · 2023-04-05T14:59:45Z

lightning-invoice/src/utils.rs

+	}
+
+	let scaled_min_inbound = min_inbound_capacity_msat.unwrap() * 110;
+	let current_sufficient  = current_channel  * 100 >=  scaled_min_inbound;


nit: remove double spaces in three places

jkczyz · 2023-04-05T15:04:34Z

lightning-invoice/src/utils.rs

+		for test_case in test_cases {
+			assert_eq!(crate::utils::prefer_current_channel(test_case.0, test_case.1, test_case.2), test_case.3);
+		}


I'd prefer that we don't use a loop here. Otherwise, it isn't clear which case has failed.

jkczyz · 2023-04-05T15:08:28Z

lightning-invoice/src/utils.rs

+where
+	L::Target: Logger,
+{
+	let mut phantom_hints : Vec<RouteHint> = Vec::new();


nit: no space before colon

This commit updates the way that we choose our preferred channel per counterparty when selecting route hints. Previously, we would choose the largest usable channel above our requested minimum. This change updates selection to prefer the smallest channel above the minimum amount (if provided, plus a scaling factor of 10% to allow some margin for error). This is the off chain version of not "grinding our change" - we want to supply route hints for channels that have enough inbound to facilitate the receive, but not overload our high inbound channels with smaller payments (since we may need this large chunk of inbound for larger payment later on). If there is no minimum amount provided, we err on the side of caution and just select our highest inbound channels so that payments of any size have a chance of succeeding. Likewise, if we have no channels above the minimum, we select the largest channel to maximize our changes of receiving the payment in multiple parts.

TheBlueMatt · 2023-04-12T18:45:42Z

lightning-invoice/src/utils.rs

+
+		for hints in phantom_hints.iter() {
+			if hint_idx < hints.len() {
+				invoice_hints.push(hints[hint_idx].clone());


Won't this add all 4 hints if we have two nodes each with two channels? We'll walk the outer loop twice and the inner loop will push both channels both times.

Ah yeah, would also include too many if we have > 3 phantom nodes (would include the first hint for each node then exit). Thanks for the catch!

Added a fixup moving the len check up to address + test coverage for the two scenarios where this previously wouldn't work.

dunxen · 2023-04-19T20:54:18Z

Looks good for squash.

TheBlueMatt · 2023-04-24T20:03:47Z

LGTM, feel free to squash the fixup, sorry for the delay.

freddiekrugerrand · 2023-04-25T19:48:58Z

Squashed. Thanks for all the patient review!

TheBlueMatt

Thanks! Opened an issue for moving to iterators #2240

douglaz reviewed Feb 22, 2023

View reviewed changes

lightning-invoice/src/utils.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed Feb 22, 2023

View reviewed changes

lightning-invoice/src/utils.rs Outdated Show resolved Hide resolved

TheBlueMatt mentioned this pull request Feb 23, 2023

Limit amount of hints resulting from filter_channels to 3 #1919

Closed

jkczyz self-requested a review February 23, 2023 18:37

freddiekrugerrand force-pushed the 1782-limitchannelhints branch 2 times, most recently from 8e3c58b to 3647c26 Compare February 24, 2023 20:05

dunxen self-requested a review February 26, 2023 20:04

jkczyz reviewed Feb 28, 2023

View reviewed changes

lightning-invoice/src/utils.rs Outdated Show resolved Hide resolved

lightning-invoice/src/utils.rs Outdated Show resolved Hide resolved

lightning-invoice/src/utils.rs Outdated Show resolved Hide resolved

lightning-invoice/src/utils.rs Outdated Show resolved Hide resolved

freddiekrugerrand force-pushed the 1782-limitchannelhints branch from 3647c26 to c3f2029 Compare March 1, 2023 15:56

freddiekrugerrand changed the title ~~Sort route hints by inbound and limit to three channels~~ Limit route hints to three channels Mar 1, 2023

freddiekrugerrand commented Mar 1, 2023

View reviewed changes

freddiekrugerrand force-pushed the 1782-limitchannelhints branch from c3f2029 to 90f5e95 Compare March 1, 2023 16:02

jkczyz reviewed Mar 2, 2023

View reviewed changes

freddiekrugerrand force-pushed the 1782-limitchannelhints branch 3 times, most recently from 8364dcd to c4c78b2 Compare March 2, 2023 19:41

dunxen reviewed Mar 2, 2023

View reviewed changes

freddiekrugerrand force-pushed the 1782-limitchannelhints branch 4 times, most recently from 37f8fb6 to d70cfd4 Compare March 2, 2023 19:57

TheBlueMatt reviewed Mar 6, 2023

View reviewed changes

freddiekrugerrand requested review from jkczyz and dunxen April 3, 2023 06:43

TheBlueMatt assigned TheBlueMatt, dunxen and jkczyz Apr 3, 2023

dunxen previously approved these changes Apr 4, 2023

View reviewed changes

freddiekrugerrand dismissed dunxen’s stale review via d466f94 April 5, 2023 11:59

freddiekrugerrand force-pushed the 1782-limitchannelhints branch 3 times, most recently from fd6627b to 14c8a60 Compare April 5, 2023 12:28

jkczyz reviewed Apr 5, 2023

View reviewed changes

freddiekrugerrand added 2 commits April 6, 2023 11:04

Sort route hints by inbound capacity and limit to 3 hints

20ef12f

freddiekrugerrand force-pushed the 1782-limitchannelhints branch from 14c8a60 to 53b5f0b Compare April 6, 2023 09:10

dunxen previously approved these changes Apr 6, 2023

View reviewed changes

TheBlueMatt reviewed Apr 12, 2023

View reviewed changes

freddiekrugerrand dismissed dunxen’s stale review via c1c37bc April 19, 2023 18:16

freddiekrugerrand force-pushed the 1782-limitchannelhints branch from c1c37bc to 1af848f Compare April 19, 2023 18:20

Move phantom route hint selection into its own function

3e65e62

freddiekrugerrand force-pushed the 1782-limitchannelhints branch from 1af848f to 905cd8b Compare April 25, 2023 19:46

Limit phantom invoice hints to 3

905cd8b

dunxen approved these changes Apr 25, 2023

View reviewed changes

TheBlueMatt mentioned this pull request Apr 27, 2023

Replace intermediate Vecs with iterators #2240

Closed

TheBlueMatt approved these changes Apr 27, 2023

View reviewed changes

TheBlueMatt merged commit c3de782 into lightningdevkit:main Apr 27, 2023

upjohnc mentioned this pull request May 11, 2023

Set return type to Iterator for functions in file: lightning-invoice/src/utils.rs : issue #2240 #2290

Merged

	invoice = invoice.private_route(route_hint.clone());
	invoice = invoice.private_route(route_hint);

Limit route hints to three channels #2044

Limit route hints to three channels #2044

Conversation

freddiekrugerrand commented Feb 22, 2023 • edited Loading

freddiekrugerrand commented Feb 22, 2023

TheBlueMatt commented Feb 22, 2023

TheBlueMatt left a comment

Choose a reason for hiding this comment

freddiekrugerrand commented Feb 23, 2023

codecov-commenter commented Feb 24, 2023 • edited Loading

Codecov Report

benthecarman commented Feb 28, 2023

jkczyz commented Feb 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freddiekrugerrand commented Mar 1, 2023

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

dunxen commented Mar 2, 2023

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

dunxen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Apr 4, 2023

jkczyz 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

Choose a reason for hiding this comment

dunxen commented Apr 19, 2023

TheBlueMatt commented Apr 24, 2023

freddiekrugerrand commented Apr 25, 2023

TheBlueMatt left a comment

Choose a reason for hiding this comment

freddiekrugerrand commented Feb 22, 2023 •

edited

Loading

codecov-commenter commented Feb 24, 2023 •

edited

Loading