Limit amount of hints resulting from filter_channels to 3

[cdmoss]

As per #1782, filter_channels will only take 20 channels from the selected channels and shuffles the channel list before returning it.

[codecov-commenter]

Codecov Report

Base: 90.77% // Head: 90.73% // Decreases project coverage by -0.04% ⚠️

Coverage data is based on head (84c5b5e) compared to base (d4dc05b).
Patch has no changes to coverable lines.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1919      +/-   ##
==========================================
- Coverage   90.77%   90.73%   -0.05%     
==========================================
  Files          94       94              
  Lines       49603    49603              
  Branches    49603    49603              
==========================================
- Hits        45026    45006      -20     
- Misses       4577     4597      +20     

Impacted Files	Coverage Δ
lightning-invoice/src/utils.rs	97.76% <ø> (ø)
lightning-net-tokio/src/lib.rs	77.01% <0.00%> (-0.30%)	⬇️
lightning/src/ln/functional_tests.rs	96.93% <0.00%> (-0.23%)	⬇️
lightning/src/chain/channelmonitor.rs	90.80% <0.00%> (-0.20%)	⬇️
lightning/src/ln/channelmanager.rs	86.70% <0.00%> (-0.03%)	⬇️

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[valentinewallace]

LGTM after squash

[TheBlueMatt]

Please squash commits as described at https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

[ViktorTigerstrom]

Great thanks for the contribution!

If you have the possibility to do so, it would great if you add test coverage of this case, as our current test coverage covers all filtering variations that I know of. Have a look at the other tests in the same file you've been updating and see if you can add test coverage. If you'd prefer to not add the test coverage, I can do so in a follow-up instead.

A general discussion point I'd like to point out for everyone:

For phantom invoices, with the change of this PR, the route hints will still be really large if we for example have 3 participating nodes with 3 channels to different counterparties each. Would we like this to only include 3 (or maybe some other number) channels in total for phantom invoices given that we have users that runs a phantom node setup for mobile as well, or are we ok with including more channels for phantom invoices for now (9 in the example I gave)?

[cdmoss]

Great thanks for the contribution!

If you have the possibility to do so, it would great if you add test coverage of this case, as our current test coverage covers all filtering variations that I know of. Have a look at the other tests in the same file you've been updating and see if you can add test coverage. If you'd prefer to not add the test coverage, I can do so in a follow-up instead.

A general discussion point I'd like to point out for everyone:

For phantom invoices, with the change of this PR, the route hints will still be really large if we for example have 3 participating nodes with 3 channels to different counterparties each. Would we like this to only include 3 (or maybe some other number) channels in total for phantom invoices given that we have users that runs a phantom node setup for mobile as well, or are we ok with including more channels for phantom invoices for now (9 in the example I gave)?

My pleasure :). I will be quite busy with school over the next few days, but I will possibly have some time tomorrow or early next week to look into adding the new coverage.

[ViktorTigerstrom]

My pleasure :). I will be quite busy with school over the next few days, but I will possibly have some time tomorrow or early next week to look into adding the new coverage.

Nice, thanks! Let me know if you'd need any help once you've had time to look into it :).

[TheBlueMatt]

For phantom invoices, with the change of this PR, the route hints will still be really large if we for example have 3 participating nodes with 3 channels to different counterparties each. Would we like this to only include 3 (or maybe some other number) channels in total for phantom invoices given that we have users that runs a phantom node setup for mobile as well, or are we ok with including more channels for phantom invoices for now (9 in the example I gave)?

Yes, good point, we should really limit to one channel per phantom node.

Note that the current PR actually still randomizes! The ordering is currently controlled by the filtered_channels HashMap. In Rust, HashMaps are randomized for anti-DoS resistance, so in practice the iteration order of a HashMap will always be random (with some exceptions for no-std).

I'm not sure what the "right" answer here is, it probably depends a bit on what we think the intended use-case is - I think, mostly, private nodes simply won't have more than one or two channels to begin with, so it doesn't matter all that much. Given that, we can be lazy and just sort by something dumb, eg return the top two channels by node_id or channel_id or whatever. If we want to do something more clever, we could shuffle by the high bits in duration_since_epoch as a way to get a slowly rotating channel selection.

[TheBlueMatt]

Any chance you want to pick this back up?

[freddiekrugerrand]

Any chance you want to pick this back up?

If @cdmoss isn't free to resume this PR, I would be interested in picking it up. If I understand it, the outstanding items are:

1. Add unit test coverage for the change
2. Add some type of deterministic sorting to the output

Yes, good point, we should really limit to one channel per phantom node.

I don't understand this part.

[ViktorTigerstrom]

I don't understand this part.

In LDK there's support for generating invoices that can be paid to multiple receiving nodes, without making it obvious to the payer that those nodes may be the final destination. This concept is called Phantom payments and you can be read more about it here:
https://lightningdevkit.org/blog/introducing-phantom-node-payments/

These types of invoices are generated through the _create_phantom_invoice function, and what you'd probably like to do is to add an argument to the filter_channels function that specifies how many channels the function should maximally return, and then set that value to 1 in the _create_phantom_invoice function, as well as 3 in the "normal" _create_invoice_from_channelmanager_and_duration_since_epoch_with_payment_hash function :).

[freddiekrugerrand]

Picking this up now, sorry for the delay.

Yes, good point, we should really limit to one channel per phantom node.

Limiting hints for phantom nodes to 1 breaks two of the existing unit tests because they specifically test multi-node cases:

• test_multi_node_channels_inbound_capacity_lower_than_invoice_amt_filtering
• test_multi_node_hints_includes_one_channel_of_each_counterparty_nodes_per_participating_node

These tests don't really make much sense in a world where phantom nodes only have one hint (other than asserting that the code only selects one hint when multiple would have been eligible). Does it make sense to rather try move this coverage to _create_invoice_from_channelmanager_and_duration_since_epoch_with_payment_hash? Seems like the tests are pretty specific to phantom invoices...

[TheBlueMatt]

We should maybe consider doing the limiting at the _create_phantom_invoice layer? First walk through all peers and get up to three hints from each via filter_channels, then, if we have more than three across all our nodes, limit to three by picking one from each node until we have three. I think that will make those tests pass.

[TheBlueMatt]

Closing as abandoned and superseded by #2044.