Filter route hints when creating invoices #1325

ViktorTigerstrom · 2022-02-21T21:01:04Z

Filter route hints when creating invoices as described in issue #1279 .

Just a few things worth mentioning:

For create_phantom_invoice I chose not to implement filtering if any of the channels are private, as from my understanding of the phantom node/invoice concept, that would make no sense. If my understanding is incorrect, please let me know.

The test code can be cleaned up a bit, by removing some recurring code. I wanted to push out the filtering code before taking care of that though, to find out if there's anything I need to improve with the filtering. Is there some macro to match channels with the invoice route hints, or would it make sense for me to make one?

Something worth mentioning is that as noted in the issue comment I made, this filtering logic when we filter out all channels that don't exceed or match the invoice amount, makes it easier for an external entity to use the invoice creation to potentially determine the inbound capacity of the channel. This can be seen in the test_hints_has_no_channels_with_lower_inbound_capacity_than_invoice_amt test. If this is not acceptable, I'd suggest that we'd remove this specific filtering and always return the highest capacity channel regardless of it's capacity.

Other than that, I'd more than happy to address any feedback!

ViktorTigerstrom · 2022-02-21T22:05:02Z

Oh sorry, some checks fails. I'll update asap

codecov-commenter · 2022-02-21T22:32:06Z

Codecov Report

Merging #1325 (0e1ccd8) into main (ca163c3) will increase coverage by 0.01%.
The diff coverage is 98.87%.

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

@@            Coverage Diff             @@
##             main    #1325      +/-   ##
==========================================
+ Coverage   90.65%   90.67%   +0.01%     
==========================================
  Files          73       72       -1     
  Lines       40462    40464       +2     
==========================================
+ Hits        36682    36691       +9     
+ Misses       3780     3773       -7

Impacted Files	Coverage Δ
lightning/src/ln/functional_test_utils.rs	`95.10% <88.46%> (-0.45%)`	⬇️
lightning-invoice/src/utils.rs	`97.14% <99.52%> (+6.61%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.02% <100.00%> (-0.12%)`	⬇️
lightning-block-sync/src/init.rs	`93.56% <0.00%> (-2.25%)`	⬇️
lightning/src/routing/router.rs	`92.10% <0.00%> (-0.30%)`	⬇️
lightning/src/ln/channelmanager.rs	`84.60% <0.00%> (-0.18%)`	⬇️
lightning/src/routing/scoring.rs	`94.16% <0.00%> (-0.14%)`	⬇️
lightning-background-processor/src/lib.rs	`93.04% <0.00%> (-0.06%)`	⬇️
... and 13 more

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 ca163c3...e13f318. Read the comment docs.

TheBlueMatt

Hmm, it'd be really nice to DRY this up somewhat, either via some method that accepts a &[ChannelDetails] and returns an iterator over ChannelDetails's to use, or a macro if you can't figure out the typing on that.

TheBlueMatt · 2022-02-21T22:39:02Z

lightning-invoice/src/utils.rs

@@ -62,9 +62,17 @@ pub fn create_phantom_invoice<Signer: Sign, K: Deref>(
 	if let Some(amt) = amt_msat {
 		invoice = invoice.amount_milli_satoshis(amt);
 	}
+	let mut route_hints: HashMap<PublicKey, (u64, RouteHint)> = HashMap::new();


For phantoms, this should be per-"real"-node, not across the full outer loop (this will make it more similar to the non-phantom case as well).

ViktorTigerstrom · 2022-02-22T22:28:55Z

Hmm, it'd be really nice to DRY this up somewhat, either via some method that accepts a &[ChannelDetails] and returns an iterator over ChannelDetails's to use, or a macro if you can't figure out the typing on that.

Sure, I'll DRY that and the tests up!

ViktorTigerstrom · 2022-02-23T23:04:28Z

Just pushed some DRY changes! Hope the trade-offs I had to make in regards to code readability are ok :)

TheBlueMatt

Cleanups look quite nice. A few comments that may enable further cleanups.

TheBlueMatt · 2022-02-24T16:28:28Z

lightning-invoice/src/utils.rs

+fn filter_channels<F1, F2>(
+	channels: Vec<ChannelDetails>,
+	min_inbound_capacity_required: Option<u64>,
+	empty_result_if_private_channels_exists: bool,


Hmm, I'm not sure if this captures what we want exactly, if our channels are public and we're creating a phantom route hint, instead of including all the channels, we can instead just create route hints that are one hop instead of two. id instead of creating the route hint in a callback, we can instead have the create_phantom_invoice callsite push a second hop onto all the route hints we get (or, if we get none, create a single route hint that is just one hop long). Then we could remove this option, I believe.

Thanks! If the filtering logic explained below is correct, we can indeed remove it.

TheBlueMatt · 2022-02-24T16:31:56Z

lightning-invoice/src/utils.rs

+	channels: Vec<ChannelDetails>,
+	min_inbound_capacity_required: Option<u64>,
+	empty_result_if_private_channels_exists: bool,
+	channel_pubkey_selector: F1,


Given this always returns a constant (ie cause we call filter_channels inside the per-node loop in create_phantom_invoice), do we need this at all?

In the create_invoice_from_channelmanager it actually does vary, given that the channel counterparty node is different. However if the filtering logic explained below is correct, it will always be the channel counterparty node id we are interested in, and this parameter wouldn't be needed at all!
That would make the code a lot cleaner :)

ViktorTigerstrom · 2022-02-24T21:22:49Z

@TheBlueMatt Thanks a lot for the feedback!
Hmmmm, I've realized that I probably had an incorrect understanding of how the phantom invoice hints are supposed to be filtered. Sorry about that! Therefore I just to ensure that my current understanding is correct so I don't end up repeating my mistakes. Just to clarify, imagine the following scenario:

We have two nodes that are our phantom payment-receiving nodes that share same cross_node_seed:

node1
node2

Our nodes have channels connected to two counterparty nodes:

cp_node3
cp_node4

node1 have the following channels with inbound capacity amount:

channel_1 for 100k sat:
node1 -> cp_node3
channel_2 for 200k sat:
node1 -> cp_node3

node2 have the following channels with inbound capacity amount:

channel_3 for 100k sat:
node2 -> cp_node3
channel_4 for 50k sat:
node2 -> cp_node4

After the filtering is done we want the invoice to include the following channels in the hints (plus the RouteHintHop with src_node_id: hint.real_node_pubkey and short_channel_id: hint.phantom_scid):

channel_2
channel_3
channel_4

In other words, the highest inbound capacity channel for every counterparty node for each of our participating phantom payment-receiving nodes.

I previously thought we wanted to only include one channel per phantom payment-receiving node, which would have resulted in channel_2 and channel_3 (which is what the currently implemented code does).

Is this new filtering logic explained above correct?
Given the latest feedback, do I understand you correctly that if a phantom payment-receiving node has any private channel, we would like to just push a single RouteHintHop to the invoice for that node with the src_node_id: hint.real_node_pubkey and the short_channel_id: hint.phantom_scid, and let the sender find the path to the hint.real_node_pubkey phantom payment-receiving node by looking at the public channels? Thinking about it, that would make a lot of sense.

Ps. if I'm completely incorrect about how the filtering for phantom invoices are supposed to work, please let me know :).

TheBlueMatt · 2022-02-25T22:20:34Z

In other words, the highest inbound capacity channel for every counterparty node for each of our participating phantom payment-receiving nodes.

Correct.

Given the latest feedback, do I understand you correctly that if a phantom payment-receiving node has any private channel, we would like to just push a single RouteHintHop to the invoice for that node with the src_node_id: hint.real_node_pubkey and the short_channel_id: hint.phantom_scid, and let the sender find the path to the hint.real_node_pubkey phantom payment-receiving node by looking at the public channels?

Correct.

ViktorTigerstrom · 2022-02-26T17:20:01Z

Ok great, thanks! I'll cleanup the code some more then, add support for the feedback and add a few more test cases to cover those scenarios.

ViktorTigerstrom · 2022-02-28T21:21:26Z

Just push some changes! If it's not suitable that commit a094d7d and commit e29c401 are part of this PR, let me know and I'll drop them and create a new PR with them and base this upon that PR.

TheBlueMatt · 2022-03-02T18:51:00Z

lightning/src/ln/functional_tests.rs

@@ -7586,33 +7586,7 @@ fn test_priv_forwarding_rejection() {

 	let chan_id_1 = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 500_000_000, InitFeatures::known(), InitFeatures::known()).2;

-	// Note that the create_*_chan functions in utils requires announcement_signatures, which we do


Looks like you somehow ended up with some upstream changes here - can you rebase on upstream/main?

That specific line/section is actually intentionally changed by me, to use the new create_private_chan_between_nodes_with_value utility function I created in lightning/src/ln/functional_test_utils.rs in the functional_tests as well. Hence my comment above if it's suitable to include that commit e29c401 (now cc5b7cb) in this PR or if I should drop it :)

But I'll make sure to fetch the latest upstream and rebase this on that!

jkczyz · 2022-03-02T20:02:25Z

lightning/src/ln/functional_test_utils.rs

+	if nodes[a].node.get_current_default_configuration().channel_options.announced_channel {
+		panic!("`Nodes[a]` must have channel_options.announced_channel set to false")
+	}
+	nodes[a].node.create_channel(nodes[b].node.get_our_node_id(), channel_value, push_msat, 42, None).unwrap();


The last parameter to create_channel can be used to override the configuration values, I believe. So I think this function can simply override channel_options.announced_channel and re-use the other helpers instead of writing everything below this.

Thanks! Ok, I'll look into which other helpers I can use that doesn't require announcement signatures.

jkczyz · 2022-03-02T20:05:35Z

lightning-invoice/src/utils.rs

+        RouteHint(vec![RouteHintHop {
+            src_node_id: channel.counterparty.node_id,
+            short_channel_id,
+            fees: RoutingFees {
+                base_msat: forwarding_info.fee_base_msat,
+                proportional_millionths: forwarding_info.fee_proportional_millionths,
+            },
+            cltv_expiry_delta: forwarding_info.cltv_expiry_delta,
+            htlc_minimum_msat: None,
+            htlc_maximum_msat: None,}])
+    };


Please update to use tabs rather than spaces.

jkczyz · 2022-03-02T20:11:02Z

lightning-invoice/src/utils.rs

@@ -218,6 +198,85 @@ where
 	}
 }

+/// Utility to filter channels for an invoice, and return the corresponding route hints to include


nit: No need to say "Utility to". Simply say "Filters channels ... and returns ... ".

jkczyz · 2022-03-02T20:12:37Z

lightning-invoice/src/utils.rs

+/// Utility to filter channels for an invoice, and return the corresponding route hints to include
+/// in the invoice.
+///
+/// The filtering ensures that the RouteHints returned only include the highest inbound capacity


Use ticks on identifiers: RouteHints. Likewise throughout.

jkczyz · 2022-03-02T20:14:23Z

lightning-invoice/src/utils.rs

+/// in the invoice.
+///
+/// The filtering ensures that the RouteHints returned only include the highest inbound capacity
+/// channel per counterparty node for which channels are present. Channels with a lower inbound


Drop "for which channels are present" since the previous part of the sentence already establishes a channel exists.

jkczyz · 2022-03-02T20:26:53Z

lightning-invoice/src/utils.rs

+			let has_private_channels = true;
+			return (vec![], has_private_channels);
+		}
+		let short_channel_id = match channel.clone().short_channel_id {


Shouldn't need a clone for a primitive type.

Thanks! I'll make sure to do the referencing and cloning in a more suitable way.

jkczyz · 2022-03-02T20:30:23Z

lightning-invoice/src/utils.rs

+			Some(id) => id,
+			None => continue,
+		};
+		let forwarding_info = match channel.clone().counterparty.forwarding_info {


You can use references rather than cloning:

let forwarding_info = match &channel.counterparty.forwarding_info {

And then have the closure take references.

jkczyz · 2022-03-02T20:32:30Z

lightning-invoice/src/utils.rs

+				}
+				entry.insert((
+					channel.inbound_capacity_msat,
+					route_hint_from_channel(channel, short_channel_id, forwarding_info),


Pass channel by reference.

jkczyz · 2022-03-02T20:36:16Z

lightning-invoice/src/utils.rs

+	let route_hints = filtered_channels.iter()
+		.map(|(_channel, (_max_capacity, route_hint))| route_hint.clone())
+		.collect::<Vec<RouteHint>>();


You can avoid the clone by using into_iter instead of iter.

jkczyz · 2022-03-02T20:59:57Z

lightning-invoice/src/utils.rs

+			// If any of the channels are private, return no hints and let the sender look at the
+			// public channels instead.
+			let has_private_channels = true;
+			return (vec![], has_private_channels);


Rather than returning a bool, can the caller simply check if the returned Vec is empty?

That depends on if we would like the invoice to include a single hop hint for phantom payment-receiving nodes which only have public channels with inbound capacities below the invoice amt and therefore are filtered out.

Currently the code will not include hints to such nodes. But If we're ok with including them even though there are no channels that can be used to route to such nodes (at least at the point of the invoice creation), that would certainly work!

That would definitely make the code cleaner :)

Ah, that's an excellent point. I'd imagine we could have this method return Result<Vec<RouteHint, ()>> (or possible using a custom error type instead of ()) and let the caller determine what to do if no channels have sufficient inbound capacity.

True! Ok I'll make the function return a Result type instead.

ViktorTigerstrom · 2022-03-02T21:54:29Z

Thank a lot for the feedback @jkczyz! I'll make sure to update the PR asap.

If possible, it would be great if I could get a clarification on:

If we should include route hints for a node with only private channels
If we want the invoice to include a single hop route hint for a phantom payment-receiving node which only has channels with inbound capacities lower than the invoice amt.

TheBlueMatt · 2022-03-03T18:15:47Z

If we should include route hints for a node with only private channels

Yes, that's the primary purpose of route hints :)

If we want the invoice to include a single hop route hint for a phantom payment-receiving node which only has channels with inbound capacities lower than the invoice amt.

Yea, good question, at the risk of complexifying the PR, I think if all channels have less than the payment amount, we should just include all channels the same as we would if all channels had sufficient balance (still ignoring private channels if we have public ones).

ViktorTigerstrom · 2022-03-03T22:45:11Z

Great, thanks for the clarifications!

Yea, good question, at the risk of complexifying the PR, I think if all channels have less than the payment amount, we should just include all channels the same as we would if all channels had sufficient balance (still ignoring private channels if we have public ones).

Ok, I'll add that to the filtering function and also change the output to only return an empty Vec if we have a mix between public and private channels.

ViktorTigerstrom · 2022-03-06T23:26:55Z

Updated the filtering logic as per the above feedback.

Note that I pushed the changes in separate commits to easier visualize to see the changes. Therefore the intermediate commits won't pass the tests/checks currently. But I'll squash them with the corresponding commit if they look good to fix that!

Please note that a phantom invoice will now include route hints for phantom payment-receiving node(s) that have NO channels with higher inbound capacities than the invoice amt, even if another phantom payment-receiving node(s) exists that has such channels.

TheBlueMatt

Looks basically good, I think.

TheBlueMatt · 2022-03-07T18:17:20Z

lightning-invoice/src/utils.rs

+	let mut filtered_channels: HashMap<PublicKey, (u64, RouteHint)> = HashMap::new();
+	let min_inbound_capacity = min_inbound_capacity_msat.unwrap_or(0);
+
+	let mut is_public_values: HashSet<bool> = HashSet::new();


There's never any reason to have a HashSet<bool>, the table is on the heap and the pointers and seeds are substantially larger than just storing two bools on the stack here.

Ok thanks, I'll replace it with bools instead. It was an attempt to remove some lines of code, but given your feedback I agree that thats just silly :)

TheBlueMatt · 2022-03-07T18:20:38Z

lightning-invoice/src/utils.rs

+			}
+		}
+	}
+	if is_public_values.contains(&false) && is_public_values.contains(&true) {


I don't think we actually care about if there are private channels or not here - if there are any public channels, we don't need to return any channels at all.

Oh, I must have totally misunderstood in which scenario we want to filter the hints for the invoice then, I'm sorry about that!

Just to clarify based on this feedback:
For a node with only public channels (not a mix of public and private channels), we don't want to include any route hints in the invoice then? Even if that node has multiple channels to the same counterparty node, which otherwise would make the hints filtering signal which of those channels is the highest inbound capacity one/has enough capacity for the invoice amt.

If we have public channels, then everyone can see the channel values because we've announced which UTXOs they're associated with, so there's no need to hint anything in the invoices.

Thanks for clarifying! I'm still learning about lightning, so sorry if I'm a bit confused about when hints should be included.

I'll update the filtering so that no hints are returned if any channel is public, and I'll also need to update a bunch of the tests to account for that.

@TheBlueMatt Isn't ChannelDetails:: inbound_capacity_msat actually the channel liquidity balance, which is not publicly known?

No, and I'm not sure how it got linked here, but because of #1278 - our counterparty will generally not actually care which SCID is present, they'll use whatever has enough balance. Its kinda nice to give them the "best" one, but its really not all that important. Only LDK hasn't done #1278, as far as I understand.

I'm wondering, and I think this is what @jkczyz's question is referring to as well: Why doesn't it make sense to include route hints when our node only has public channels, if we're using non public info (the inbound capacity) to generate the hints?

Yeah, that was my thinking.

No, and I'm not sure how it got linked here, but because of #1278 - our counterparty will generally not actually care which SCID is present, they'll use whatever has enough balance. Its kinda nice to give them the "best" one, but its really not all that important. Only LDK hasn't done #1278, as far as I understand.

Hmm... can't these channels be with different counterparties?

Hmm... can't these channels be with different counterparties?

Yes, that's correct!

If a node has 2 public channels with 2 different counterparty nodes, and one of the channels inbound capacity is below the invoice amt, the current code will only include the channel to the other counterparty which has higher inbound capacity than the invoice amount in the hints.
The test test_channels_with_lower_inbound_capacity_than_invoice_amt_hints_filtering, shows that behavior in action :)

Why doesn't it make sense to include route hints when our node only has public channels, if we're using non public info (the inbound capacity) to generate the hints?

Ah, I see your point. Yea, I mean that's not entirely unreasonable, but its also not crazy to just have them retry if they try to use a channel that doesn't have enough capacity. My other concern here is that if a node has some public channels there's a good chance they're a routing node and they have lots of public channels. One goal is to cut down the number of channels listed in the invoice, which can prevent QR code usage, and its easier to just include nothing if they're a routing node than it is to try to have some channel count limit behavior.

Ah, that makes sense! Ok I'll update the PR asap

ViktorTigerstrom · 2022-03-10T20:20:32Z

Updated the code based on latest feedback, which makes the invoice filtering include no hints for a node if it has any public channels.

If it looks good, I'll squash the commits and also rebase on upstream as I see that there are now conflicts with upstream.

ViktorTigerstrom · 2022-03-10T20:23:14Z

lightning-invoice/src/utils.rs

+			htlc_maximum_msat: None,}])
+	};
+	for channel in channels {
+		if channel.is_public {


Please check that it's correct to place the public channel filtering here, before checking if any channel.counterparty.forwarding_info exists.

The reason why I ask this is that channel.counterparty.forwarding_info will only be assigned after the first node.handle_channel_update with the ChannelUpdate for the channel has been executed.

Oh, yea, I mean wont matter much, but might as well move this check to after the forwarding_info check/continue.

Ok great thanks! Fixed this in the latest pushed version

TheBlueMatt · 2022-03-11T22:01:58Z

This needs rebase, sadly.

TheBlueMatt · 2022-03-11T22:28:35Z

The bulk of the code LGTM, though, haven't reviewed the tests in detail.

ViktorTigerstrom · 2022-03-13T23:21:22Z

Rebased on upstream, and therefore also added scid alias support.

In the latest upstream, I also found that a util to create unannounced channels had been added. I therefore dropped my version, and used the upstream one for this PR.

Let me know if there's anything more that needs to be fixed :)

TheBlueMatt

These tests are great, thanks! Two trivial comments, LGTM!

TheBlueMatt · 2022-03-15T02:29:57Z

lightning-invoice/src/utils.rs

+		let accept_channel = get_event_msg!(nodes[0], MessageSendEvent::SendAcceptChannel, nodes[2].node.get_our_node_id());
+		nodes[2].node.handle_accept_channel(&nodes[0].node.get_our_node_id(), InitFeatures::known(), &accept_channel);
+
+		let (temporary_channel_id, tx, _) = create_funding_transaction(&nodes[2], 1_000_000, 42);


nit: this hunk and the next hunk could be replaced with sign_funding_transaction.

TheBlueMatt · 2022-03-15T04:28:43Z

lightning-invoice/src/utils.rs


 	#[test]
 	fn test_from_channelmanager() {
 		let chanmon_cfgs = create_chanmon_cfgs(2);
 		let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
 		let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
 		let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
-		let _chan = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
+		let mut private_chan_cfg = UserConfig::default();


Looks like this is now unused.

ViktorTigerstrom · 2022-03-15T13:29:39Z

Thanks! Fixed those comments :)

jkczyz

Yes, looks much simpler and shaping up!

jkczyz · 2022-03-15T20:29:45Z

lightning-invoice/src/utils.rs

@@ -10,7 +10,7 @@ use lightning::chain;
 use lightning::chain::chaininterface::{BroadcasterInterface, FeeEstimator};
 use lightning::chain::keysinterface::{Recipient, KeysInterface, Sign};
 use lightning::ln::{PaymentHash, PaymentPreimage, PaymentSecret};
-use lightning::ln::channelmanager::{ChannelDetails, ChannelManager, PaymentId, PaymentSendFailure, MIN_FINAL_CLTV_EXPIRY};
+use lightning::ln::channelmanager::{ChannelDetails, ChannelManager, PaymentId, PaymentSendFailure, CounterpartyForwardingInfo, MIN_FINAL_CLTV_EXPIRY};


nit: could you move CounterpartyForwardingInfo immediately after ChannelManager?

jkczyz · 2022-03-15T20:32:25Z

lightning-invoice/src/utils.rs

+	let route_hints = filter_channels(
+		channelmanager.list_usable_channels(),
+		amt_msat,
+	);


Though not entirely consistent, we try to wrap at 100 characters, so this should fit on one line.

Ok thanks for clarifying, will keep that in mind from now on

jkczyz · 2022-03-15T20:41:41Z

lightning-invoice/src/utils.rs

+/// channel per counterparty node. If any channel with a higher inbound capacity than the given
+/// `min_inbound_capacity_msat` exists, channels to other counterparty nodes with a lower inbound
+/// capacity than `min_inbound_capacity_msat` will be filtered out, even if they are the highest
+/// inbound capacity channel for that specific counterparty node.


The second sentence here is a bit verbose. IIUC, I think the same thing could be conveyed by extending the previous sentence to say "... with at least min_inbound_capacity_msat balance or the highest for each counterparty if min_inbound_capacity_msat is not met". I don't think that last part is mentioned until later in the docs.

Alternatively, for this comment and the one that follows, using the list from the commit message would suffice. I thought that was pretty well written, FWIW.

jkczyz · 2022-03-15T20:52:21Z

lightning-invoice/src/utils.rs

+				}
+				entry.insert((
+					channel.inbound_capacity_msat,
+					route_hint_from_channel(&channel, short_channel_id, forwarding_info),


I think we can get by having the map contain &ChannelDetails rather than (u64, RouteHint) and only creating the RouteHint in the map call below. Then you won't need the earlier lambda or create RouteHints if they are never used (e.g., if another channel has a larger capacity or if all channels are public).

jkczyz · 2022-03-15T21:00:47Z

lightning-invoice/src/utils.rs

+/// Input:
+/// `channels`: The channels to filter.
+/// `min_inbound_capacity_msat`: Defines the lowest inbound capacity channels must have to not
+/// be filtered out, if any other channel above that amount exists.
+///
+/// Result:
+/// `Vec<RouteHint>`: The filtered `RouteHints`, which will be empty if any public channel exists.


I'd recommend removing this especially if it's repeating what's in the previous paragraphs in a slightly different way. Prefer to say it just once in a succinct way without loss of meaning.

jkczyz · 2022-03-15T21:10:54Z

lightning-invoice/src/utils.rs

-					}])
-			);
+		let mut route_hints = filter_channels(
+			hint.channels.clone(),


Since the function takes hints by value, you can avoid this clone by using the de-structuring syntax in the for loop:

for PhantomRouteHints { channels, phantom_scid, real_node_pubkey } in phantom_route_hints {

Ah that's neat! Thanks for letting me know

jkczyz · 2022-03-15T21:18:13Z

lightning-invoice/src/utils.rs

+		for hint in hints {
+			let hint_short_chan_id = (hint.0).0[0].short_channel_id;
+			assert!(chan_ids_to_match.contains(&hint_short_chan_id));
+			chan_ids_to_match.remove(&hint_short_chan_id);


Did you want to check that chan_ids_to_match is empty after iterating through the hints? This could be more useful than the earlier length check because you could include in the assertion failure which channels had been missed.

jkczyz · 2022-03-15T21:18:51Z

lightning-invoice/src/utils.rs

+		invoice_amt: Option<u64>,
+		invoice_node: &Node<'a, 'b, 'c>,
+		mut chan_ids_to_match: HashSet<u64>
+	){


nit: space between: ) {

jkczyz · 2022-03-15T21:21:22Z

lightning-invoice/src/utils.rs

+			assert!(chan_ids_to_match.contains(&hint_short_chan_id));
+			chan_ids_to_match.remove(&hint_short_chan_id);


Could combine these by checking the result of remove instead:

assert!(chan_ids_to_match.remove(&hint_short_chan_id))

jkczyz · 2022-03-15T21:31:58Z

lightning-invoice/src/utils.rs

+					assert!(chan_ids_to_match.contains(&hint_short_chan_id));
+					chan_ids_to_match.remove(&hint_short_chan_id);


Similarly here.

ViktorTigerstrom · 2022-03-15T23:29:51Z

Thanks a lot for the feedback! Will try to address it shortly :)

ViktorTigerstrom · 2022-03-16T01:23:18Z

Pushed changes that addresses the feedback. I pushed the cleanup of the filtering function in a separate commit that I'll squash if it looks good.

ViktorTigerstrom · 2022-03-16T01:25:26Z

lightning-invoice/src/utils.rs

+/// * If any public channel exists, the returned `RouteHint`s will be empty, and the sender will
+/// need to find the path by looking at the public channels instead
+fn filter_channels(channels: Vec<ChannelDetails>, min_inbound_capacity_msat: Option<u64>) -> Vec<RouteHint>{
+	let mut filtered_channels: HashMap<PublicKey, ChannelDetails> = HashMap::new();


I wasn't able to make this take a borrowed &ChannelDetails, as the borrow doesn't live outside of the channel loop context. Is that ok?

Instead of borrowing channel directly within the loop, you can change the loop to iterate over references using:

for channel in channels.iter() {

Otherwise, into_iter is implicitly used, which consumes channels and yields values instead of references.

Oh nice, thanks for clarifying!

ViktorTigerstrom · 2022-03-16T01:28:03Z

lightning-invoice/src/utils.rs

+	let mut min_capacity_channel_exists = false;
+
+	for channel in channels {
+		if channel.get_inbound_payment_scid().is_none() || channel.counterparty.forwarding_info.is_none() {


Made this over 100 characters intentionally as I thought it looked cleaner than the alternative.

jkczyz

Looking good. Thanks for bearing with me. Just some minor improvements left.

jkczyz · 2022-03-16T05:21:14Z

lightning-invoice/src/utils.rs

+/// * If any public channel exists, the returned `RouteHint`s will be empty, and the sender will
+/// need to find the path by looking at the public channels instead
+fn filter_channels(channels: Vec<ChannelDetails>, min_inbound_capacity_msat: Option<u64>) -> Vec<RouteHint>{
+	let mut filtered_channels: HashMap<PublicKey, ChannelDetails> = HashMap::new();


Instead of borrowing channel directly within the loop, you can change the loop to iterate over references using:

for channel in channels.iter() {

Otherwise, into_iter is implicitly used, which consumes channels and yields values instead of references.

jkczyz · 2022-03-16T05:27:19Z

lightning-invoice/src/utils.rs

+			let hint_short_chan_id = (hint.0).0[0].short_channel_id;
+			assert!(chan_ids_to_match.remove(&hint_short_chan_id));
+		}
+		assert!(chan_ids_to_match.is_empty());


Could add a message so that unmatched ids are printed, something like this:

assert!(chan_ids_to_match.is_empty(), "Unmatched short channel ids: {:?}", chan_ids_to_match);

Ah snap, missed that one.. thanks!

jkczyz · 2022-03-16T05:28:44Z

lightning-invoice/src/utils.rs

+				_ => panic!("Incorrect hint length generated")
+			}
+		}
+		assert!(chan_ids_to_match.is_empty());


Likewise here.

Filter the route hints in `create_invoice_from_channelmanager` based on the following criteria: * Only one channel per counterparty node * Always select the channel with the highest inbound capacity * Filter out channels with a lower inbound capacity than the invoice amount, if any channel exists with enough capacity to cover the invoice amount * If any public channel exists, the invoice route_hints should be empty, and the sender will need to find the path to the payment-receiving node by looking at the public channels instead

Filter the route hints in `create_phantom_invoice` based on the following criteria: * Only one channel for every counterparty node per phantom payment-receiving node in the invoice * Always select the channel with the highest inbound capacity * For each payment-receiving node, filter out channels with a lower inbound capacity than the invoice amount, if any channel exists with enough capacity to cover the invoice amount * If any public channels exists for a payment-receiving node, push a single RouteHintHop with the phantom route and let the sender find the path to the payment-receiving node through the public channels.

ViktorTigerstrom · 2022-03-16T10:27:29Z

Fixed the latest feedback, and squashed the temporary commit

ViktorTigerstrom force-pushed the 2022-02-filter-route-hints branch from 4e4ca11 to aa3478b Compare February 21, 2022 22:05

TheBlueMatt reviewed Feb 21, 2022

View reviewed changes

ViktorTigerstrom force-pushed the 2022-02-filter-route-hints branch from aa3478b to b929554 Compare February 23, 2022 23:01

ViktorTigerstrom force-pushed the 2022-02-filter-route-hints branch 2 times, most recently from 945816a to 7585bcf Compare February 23, 2022 23:08

TheBlueMatt reviewed Feb 24, 2022

View reviewed changes

ViktorTigerstrom force-pushed the 2022-02-filter-route-hints branch from 7585bcf to 0712091 Compare February 28, 2022 21:18

jkczyz self-requested a review March 2, 2022 07:49

TheBlueMatt reviewed Mar 2, 2022

View reviewed changes

ViktorTigerstrom force-pushed the 2022-02-filter-route-hints branch 2 times, most recently from fd43e2d to 8bac21f Compare March 2, 2022 19:15

jkczyz reviewed Mar 2, 2022

View reviewed changes

ViktorTigerstrom force-pushed the 2022-02-filter-route-hints branch from 8bac21f to 0e1ccd8 Compare March 6, 2022 23:24

TheBlueMatt reviewed Mar 7, 2022

View reviewed changes

jurvis mentioned this pull request Mar 8, 2022

Use substitude channel for forwarding if we have another with the same peer #1278

Closed

TheBlueMatt mentioned this pull request Mar 9, 2022

Support for SCID Aliases #1311

Merged

ViktorTigerstrom force-pushed the 2022-02-filter-route-hints branch from 0e1ccd8 to a5ad4aa Compare March 10, 2022 20:17

ViktorTigerstrom commented Mar 10, 2022

View reviewed changes

ViktorTigerstrom force-pushed the 2022-02-filter-route-hints branch from a5ad4aa to e13f318 Compare March 13, 2022 23:18

ViktorTigerstrom mentioned this pull request Mar 14, 2022

Create normal/phantom invoice with description hash #1361

Merged

TheBlueMatt previously approved these changes Mar 15, 2022

View reviewed changes

ViktorTigerstrom dismissed TheBlueMatt’s stale review via 2b9c02c March 15, 2022 13:28

ViktorTigerstrom force-pushed the 2022-02-filter-route-hints branch from e13f318 to 2b9c02c Compare March 15, 2022 13:28

TheBlueMatt previously approved these changes Mar 15, 2022

View reviewed changes

jkczyz reviewed Mar 15, 2022

View reviewed changes

ViktorTigerstrom dismissed TheBlueMatt’s stale review via 4632ca7 March 16, 2022 01:19

ViktorTigerstrom force-pushed the 2022-02-filter-route-hints branch from 2b9c02c to 4632ca7 Compare March 16, 2022 01:19

ViktorTigerstrom commented Mar 16, 2022

View reviewed changes

jkczyz reviewed Mar 16, 2022

View reviewed changes

ViktorTigerstrom added 4 commits March 16, 2022 11:24

Add tests for create invoice route hints filtering

eae5086

Add phantom invoice route hints filtering tests

ff792e0

ViktorTigerstrom force-pushed the 2022-02-filter-route-hints branch from 4632ca7 to ff792e0 Compare March 16, 2022 10:26

jkczyz approved these changes Mar 16, 2022

View reviewed changes

TheBlueMatt approved these changes Mar 16, 2022

View reviewed changes

TheBlueMatt merged commit 699ac83 into lightningdevkit:main Mar 16, 2022

ViktorTigerstrom mentioned this pull request May 26, 2022

Filtering route hints included in lightning-invoice::utils-constructed invoicse #1279

Closed

		@@ -7586,33 +7586,7 @@ fn test_priv_forwarding_rejection() {

		let chan_id_1 = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 500_000_000, InitFeatures::known(), InitFeatures::known()).2;

		// Note that the create_*_chan functions in utils requires announcement_signatures, which we do

		assert!(chan_ids_to_match.contains(&hint_short_chan_id));
		chan_ids_to_match.remove(&hint_short_chan_id);

Filter route hints when creating invoices #1325

Filter route hints when creating invoices #1325

Conversation

ViktorTigerstrom commented Feb 21, 2022 • edited Loading

ViktorTigerstrom commented Feb 21, 2022

codecov-commenter commented Feb 21, 2022 • edited Loading

Codecov Report

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

ViktorTigerstrom commented Feb 22, 2022

ViktorTigerstrom commented Feb 23, 2022

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorTigerstrom Feb 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorTigerstrom Feb 24, 2022 • edited Loading

Choose a reason for hiding this comment

ViktorTigerstrom commented Feb 24, 2022 • edited Loading

TheBlueMatt commented Feb 25, 2022

ViktorTigerstrom commented Feb 26, 2022

ViktorTigerstrom commented Feb 28, 2022

Choose a reason for hiding this comment

ViktorTigerstrom Mar 2, 2022 • edited Loading

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

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

ViktorTigerstrom Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorTigerstrom commented Mar 2, 2022 • edited Loading

TheBlueMatt commented Mar 3, 2022

ViktorTigerstrom commented Mar 3, 2022

ViktorTigerstrom commented Mar 6, 2022 • edited Loading

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

ViktorTigerstrom Mar 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorTigerstrom Mar 7, 2022 • edited Loading

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

ViktorTigerstrom Mar 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorTigerstrom commented Mar 10, 2022

ViktorTigerstrom Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Mar 11, 2022

TheBlueMatt commented Mar 11, 2022

ViktorTigerstrom commented Mar 13, 2022

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

ViktorTigerstrom commented Mar 15, 2022

ViktorTigerstrom commented Feb 21, 2022 •

edited

Loading

codecov-commenter commented Feb 21, 2022 •

edited

Loading

ViktorTigerstrom Feb 24, 2022 •

edited

Loading

ViktorTigerstrom Feb 24, 2022 •

edited

Loading

ViktorTigerstrom commented Feb 24, 2022 •

edited

Loading

ViktorTigerstrom Mar 2, 2022 •

edited

Loading

ViktorTigerstrom Mar 2, 2022 •

edited

Loading

ViktorTigerstrom commented Mar 2, 2022 •

edited

Loading

ViktorTigerstrom commented Mar 6, 2022 •

edited

Loading

ViktorTigerstrom Mar 7, 2022 •

edited

Loading

ViktorTigerstrom Mar 7, 2022 •

edited

Loading

ViktorTigerstrom Mar 9, 2022 •

edited

Loading

ViktorTigerstrom Mar 10, 2022 •

edited

Loading

jkczyz Mar 15, 2022 •

edited

Loading

jkczyz Mar 15, 2022 •

edited

Loading