Limit number of route hints while creating an invoice

[G8XSU]

Currently, there is no limit to number of route hints that we add to invoice.
Depending on how many channels a node has, this can lead to pretty large invoices being generated which might break multiple client systems.

Lnd currently limits route hints to 20.

Relevant comment:
Originally posted by @G8XSU in #1769 (comment)

[G8XSU]

It might be helpful to shuffle the route hints as well, to avoid exhausting channels in same order.

[cdmoss]

I'm taking a look at this. I'm new to rust, lightning and oss contribution so bear with me as I may be overlooking some obvious considerations, and there may be some trivial aspects to my question.

It looks like the changes belong in lightning-invoice/src/utils.rs::filter_channels, correct? Should the map/filter at the end of the method be refactored into a for loop, to avoid checking all the channels even after the max hint number has been reached? I considered incrementing a counter inside the main filter predicate after each time include_channel is found true, then adding counter <= 20 (if we are to follow LND) as the first condition in include_channel's assignment. But this feels a bit hacky, since this would result in all the channels being looked at, even after the counter hits the max number, and it would make logging awkward since there would potentially be multiple "max hint reached logs" (unless I added another flag to ensure only one log message is written). It seems like a for loop which converts each filtered channel to a hint and adds it to the final hint collection would be more appropriate, since it could be broken out of the moment the max hint count is reached.

Also, should the channels be sorted by some priority beforehand to ensure the best channels make the cut (i.e. should online, min capacity channels be prioritised, then min_capacity channels, etc)?

Any guidance, or alternative suggestions here would be greatly appreciated. :)

[jkczyz]

It looks like the changes belong in lightning-invoice/src/utils.rs::filter_channels, correct? Should the map/filter at the end of the method be refactored into a for loop, to avoid checking all the channels even after the max hint number has been reached? I considered incrementing a counter inside the main filter predicate after each time include_channel is found true, then adding counter <= 20 (if we are to follow LND) as the first condition in include_channel's assignment. But this feels a bit hacky, since this would result in all the channels being looked at, even after the counter hits the max number, and it would make logging awkward since there would potentially be multiple "max hint reached logs" (unless I added another flag to ensure only one log message is written). It seems like a for loop which converts each filtered channel to a hint and adds it to the final hint collection would be more appropriate, since it could be broken out of the moment the max hint count is reached.

No need to convert to a for loop. Iterators in Rust are lazy, so you can simple chain .take(n).

Also, should the channels be sorted by some priority beforehand to ensure the best channels make the cut (i.e. should online, min capacity channels be prioritised, then min_capacity channels, etc)?

Not sure about sorting, but note that when assigning include_channel in the filter, the same branch will be hit for all channels. So using that as a sorting criteria wouldn't make a difference.

Any guidance, or alternative suggestions here would be greatly appreciated. :)

No problem. Thanks for taking on this issue!

[cdmoss]

Fantastic, thanks for the clarification!