Include all channel route hints if no connected channels exist #1769

TheBlueMatt · 2022-10-13T22:05:41Z

Mobile clients often take a second or two before they reconnect to their peers as its not always clear immediately that connections have been killed (especially on iOS). This can cause us to spuriously fail to include route hints in our invoices if we're on mobile.

The fix is simple, if we're selecting channels to include in route hints and we're not not connected to the peer for any of our channels, we should simply include the hints for all channels, even though we're disconencted.

Fixes #1768.

ViktorT-11

Generally looks good! Would be great with test coverage of this, since the current route hint filtering coverage in lightning_invoice::utils covers most of the current edge cases.

lightning-invoice/src/utils.rs

ViktorT-11 · 2022-10-13T23:29:34Z

lightning-invoice/src/utils.rs

 	L::Target: Logger,
 {
-	let route_hints = filter_channels(channelmanager.list_usable_channels(), amt_msat);
+	let route_hints = filter_channels(channelmanager.list_channels(), amt_msat);


Would we like to do this for ChannelManager::get_phantom_route_hints as well so that create_phantom_invoice will include create hints with the same filtering if all participating nodes are disconnected or have just disconnected peers (although I guess this isn't really relevant for mobile clients), as phantom invoices are useless without any meaningful route hints?

rust-lightning/lightning/src/ln/channelmanager.rs

Line 5546 in 6772609

channels: self.list_usable_channels(),

If the case that all participating nodes have only private channels which are all currently !channel.is_usable, we'll currently push route hints containing only the participating node's id, but as there are no public channels to reach those participating nodes the route hint's will be useless, which seems kinda buggy.

That would make things a bit more complicated as we'd need filter_channels to only return usable channels if any of the phantom_route_hints in _create_phantom_invoice contains a channel with channel.is_usable.

PhantomRouteHints are unlikely to be used in a mobile context. The related-utilities here are ChannelManager-less because these hints may be serialized and read for multiple nodes. We may even want to consider removing the peer connection requirement in get_phantom_route_hints.

Hmm, right, interesting question. I think for now let's leave it as-is - in a server environment the "is channel connected" concept is usually "is peer not down doing maintenance or something", which we'd ideally like to avoid including those channels for.

Ok as a follow-up/separate issue it might be worth it to at least add some validation and verify that the length of the channels vec in the PhantomRouteHints isn't 0? As it is now, we'll call filter_channels and add the phantom hop route hint that includes the participating node's id which can't be routed to in that case. Could also be worth ensuring that the final constructed invoice actually have some route hints depending on how the validation is implemented.

valentinewallace · 2022-10-17T15:18:18Z

lightning-invoice/src/utils.rs

 			return vec![]
 		}

 		if channel.inbound_capacity_msat >= min_inbound_capacity {


Should we still return an empty vec if any public channel exists (5 lines up), since the public channels may be offline now?

Hmm, good question. I think not - we're really assuming that any channels that are disconnected now may be live by the time the sender goes to pay, so if we extend that assumption to public channels we dont need to include any hints. Further, I'd be concerned about accidentally nuking privacy if we did that.

lightning-invoice/src/utils.rs

TheBlueMatt · 2022-10-17T16:16:45Z

Pushed some fixups, should actually compile now lol.

codecov-commenter · 2022-10-17T16:35:31Z

Codecov Report

Base: 90.78% // Head: 90.89% // Increases project coverage by +0.10% 🎉

Coverage data is based on head (f8d941b) compared to base (fdfd4f0).
Patch coverage: 94.87% of modified lines in pull request are covered.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1769      +/-   ##
==========================================
+ Coverage   90.78%   90.89%   +0.10%     
==========================================
  Files          87       87              
  Lines       46969    48048    +1079     
  Branches    46969    48048    +1079     
==========================================
+ Hits        42643    43673    +1030     
- Misses       4326     4375      +49

Impacted Files	Coverage Δ
lightning-invoice/src/utils.rs	`94.81% <94.87%> (-0.04%)`	⬇️
lightning-net-tokio/src/lib.rs	`76.73% <0.00%> (-0.31%)`	⬇️
lightning/src/ln/peer_channel_encryptor.rs	`93.38% <0.00%> (-0.25%)`	⬇️
lightning/src/chain/onchaintx.rs	`95.40% <0.00%> (-0.23%)`	⬇️
lightning/src/chain/channelmonitor.rs	`91.16% <0.00%> (-0.06%)`	⬇️
lightning/src/ln/functional_tests.rs	`96.90% <0.00%> (-0.05%)`	⬇️
lightning/src/ln/channelmanager.rs	`85.14% <0.00%> (-0.03%)`	⬇️
lightning/src/ln/msgs.rs	`86.41% <0.00%> (+0.17%)`	⬆️
lightning/src/ln/chan_utils.rs	`95.32% <0.00%> (+0.76%)`	⬆️
... and 1 more

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 · 2022-10-17T18:45:48Z

Squashed without changes.

ViktorT-11

ACK 80354ac

lightning-invoice/src/utils.rs

TheBlueMatt · 2022-10-17T22:47:08Z

Squash-force-pushed a change to use into_iter rather than drain:

$ git diff-tree -U1 80354acb b1b225644
diff --git a/lightning-invoice/src/utils.rs b/lightning-invoice/src/utils.rs
index 71e7fbb26..48849263e 100644
--- a/lightning-invoice/src/utils.rs
+++ b/lightning-invoice/src/utils.rs
@@ -383,3 +383,3 @@ where
 /// need to find the path by looking at the public channels instead
-fn filter_channels(mut channels: Vec<ChannelDetails>, min_inbound_capacity_msat: Option<u64>) -> Vec<RouteHint>{
+fn filter_channels(channels: Vec<ChannelDetails>, min_inbound_capacity_msat: Option<u64>) -> Vec<RouteHint>{
 	let mut filtered_channels: HashMap<PublicKey, ChannelDetails> = HashMap::new();
@@ -389,3 +389,3 @@ fn filter_channels(mut channels: Vec<ChannelDetails>, min_inbound_capacity_msat:
 
-	for channel in channels.drain(..).filter(|chan| chan.is_channel_ready) {
+	for channel in channels.into_iter().filter(|chan| chan.is_channel_ready) {
 		if channel.get_inbound_payment_scid().is_none() || channel.counterparty.forwarding_info.is_none() {

jkczyz · 2022-10-18T01:22:04Z

lightning-invoice/src/utils.rs

 		.filter(|(_counterparty_id, channel)| {
-			min_capacity_channel_exists && channel.inbound_capacity_msat >= min_inbound_capacity ||
-			!min_capacity_channel_exists
+			if min_capacity_channel_exists {
+				channel.inbound_capacity_msat >= min_inbound_capacity
+			} else { true }
+		})
+		.filter(|(_counterparty_id, channel)| {
+			if online_channel_exists {
+				channel.is_usable
+			} else { true }
 		})


Hmmm... actually, this won't work. Consider two channels where one is connected but without enough capacity and the other is disconnected but with enough capacity. The first channel will not pass the first filter even though it would pass the second one. The second channel will pass the first filter but not the second one. So no channels will be included.

Oof, good point. Pushed a fix for it.

See #1769 (comment)

lightning-invoice/src/utils.rs

valentinewallace

LGTM after squash

lightning-invoice/src/utils.rs

G8XSU · 2022-10-18T20:50:08Z

lightning-invoice/src/utils.rs

+				channel.inbound_capacity_msat >= min_inbound_capacity
+			} else if online_channel_exists {
+				channel.is_usable
+			} else { true }


is there no limit to number of route hints we want to have?
lnd limits to 20.

We probably should, but it depends a lot on the use-case. If the user is generating a QR code, 20 is way too many, if they're using it programatically in an API that may make sense. That seems like a separate concern from this PR, though, and we should do something about that in a followup. Can you open an issue and tag it, like, 0.1?

G8XSU · 2022-10-18T20:54:01Z

lightning-invoice/src/utils.rs

-			!min_capacity_channel_exists
+			if online_min_capacity_channel_exists {
+				channel.inbound_capacity_msat >= min_inbound_capacity && channel.is_usable
+			} else if min_capacity_channel_exists && online_channel_exists {


acc. to this comment "// If there are some online channels and some min_capacity channels, but no
// online-and-min_capacity channels, just include the min capacity ones and ignore
// online-ness."

check should only be on "min_capacity_channel_exists", online_channel_exists seems redundant

Well the code matches the comment, and LLVM will trivially remove the redundant checks.

TheBlueMatt · 2022-10-18T21:17:34Z

Squashed without further changes.

ViktorT-11

Looks good! Just one comment, that could be taken in a follow-up if you prefer to merge this.

lightning-invoice/src/utils.rs

Mobile clients often take a second or two before they reconnect to their peers as its not always clear immediately that connections have been killed (especially on iOS). This can cause us to spuriously fail to include route hints in our invoices if we're on mobile. The fix is simple, if we're selecting channels to include in route hints and we're not not connected to the peer for any of our channels, we should simply include the hints for all channels, even though we're disconencted. Fixes lightningdevkit#1768.

TheBlueMatt · 2022-10-18T21:46:57Z

Fixed the method docs, didn't bother to make them exact, the in-function comments leave that.

$ git diff-tree -U1 9e99577d a534dcc5
diff --git a/lightning-invoice/src/utils.rs b/lightning-invoice/src/utils.rs
index 5b9e602c1..56f4fe879 100644
--- a/lightning-invoice/src/utils.rs
+++ b/lightning-invoice/src/utils.rs
@@ -379,6 +379,6 @@ where
 /// * Always select the channel with the highest inbound capacity per counterparty node
-/// * Filter out channels with a lower inbound capacity than `min_inbound_capacity_msat`, if any
-/// channel with a higher or equal inbound capacity than `min_inbound_capacity_msat` exists
+/// * Prefer channels with capacity at least `min_inbound_capacity_msat` and where the channel
+///   `is_usable` (i.e. the peer is connected).
 /// * 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
+///   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>{
$

tanx · 2022-10-20T14:03:11Z

Merged by @valentinewallace 👋😊 Thanks for the quick fix everyone! Will test this as soon as it lands in the react-native module.

ViktorT-11 reviewed Oct 13, 2022

View reviewed changes

jkczyz self-requested a review October 14, 2022 18:11

Jasonvdb mentioned this pull request Oct 16, 2022

No route hints in payment request (LDK -> LND -> LDK) synonymdev/react-native-ldk#54

Closed

ViktorT-11 mentioned this pull request Oct 16, 2022

Add logging in route hint filtering #1770

Merged

jkczyz added this to the 0.0.112 milestone Oct 17, 2022

valentinewallace reviewed Oct 17, 2022

View reviewed changes

TheBlueMatt force-pushed the 2022-10-disconnected-hints branch from b4eb92c to 80354ac Compare October 17, 2022 18:45

ViktorT-11 previously approved these changes Oct 17, 2022

View reviewed changes

jkczyz previously approved these changes Oct 17, 2022

View reviewed changes

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

TheBlueMatt dismissed stale reviews from jkczyz and ViktorT-11 via 654a724 October 17, 2022 22:43

TheBlueMatt force-pushed the 2022-10-disconnected-hints branch from 654a724 to b1b2256 Compare October 17, 2022 22:46

jkczyz previously approved these changes Oct 17, 2022

View reviewed changes

jkczyz reviewed Oct 18, 2022

View reviewed changes

valentinewallace reviewed Oct 18, 2022

View reviewed changes

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

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

valentinewallace reviewed Oct 18, 2022

View reviewed changes

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

valentinewallace reviewed Oct 18, 2022

View reviewed changes

jkczyz previously approved these changes Oct 18, 2022

View reviewed changes

G8XSU reviewed Oct 18, 2022

View reviewed changes

TheBlueMatt dismissed jkczyz’s stale review via 9e99577 October 18, 2022 21:17

TheBlueMatt force-pushed the 2022-10-disconnected-hints branch from f8d941b to 9e99577 Compare October 18, 2022 21:17

valentinewallace previously approved these changes Oct 18, 2022

View reviewed changes

jkczyz previously approved these changes Oct 18, 2022

View reviewed changes

ViktorT-11 previously approved these changes Oct 18, 2022

View reviewed changes

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

TheBlueMatt dismissed stale reviews from ViktorT-11, jkczyz, and valentinewallace via a534dcc October 18, 2022 21:46

TheBlueMatt force-pushed the 2022-10-disconnected-hints branch from 9e99577 to a534dcc Compare October 18, 2022 21:46

ViktorT-11 approved these changes Oct 18, 2022

View reviewed changes

jkczyz approved these changes Oct 18, 2022

View reviewed changes

G8XSU mentioned this pull request Oct 18, 2022

Limit number of route hints while creating an invoice #1782

Closed

valentinewallace approved these changes Oct 19, 2022

View reviewed changes

valentinewallace merged commit 31042ab into lightningdevkit:main Oct 19, 2022

Uh oh!

Include all channel route hints if no connected channels exist #1769

Include all channel route hints if no connected channels exist #1769

Uh oh!

Conversation

TheBlueMatt commented Oct 13, 2022

Uh oh!

ViktorT-11 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ViktorT-11 Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheBlueMatt commented Oct 17, 2022

Uh oh!

codecov-commenter commented Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Oct 17, 2022

Uh oh!

ViktorT-11 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheBlueMatt commented Oct 17, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Oct 18, 2022

Uh oh!

ViktorT-11 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheBlueMatt commented Oct 18, 2022

Uh oh!

tanx commented Oct 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

ViktorT-11 left a comment •

edited

Loading

ViktorT-11 Oct 13, 2022 •

edited

Loading

codecov-commenter commented Oct 17, 2022 •

edited

Loading