Include a route hint for public, not-yet-announced channels #2024

TheBlueMatt · 2023-02-09T19:03:52Z

If we have a public channel which doesn't yet have six confirmations the network can't possibly know about it as we cannot have announced it yet. However, because we refuse to include route-hints if we have any public channels, we will generate invoices that no one can pay.

Thus, if we have any public, not-yet-announced channels, include them as a route-hint.

codecov-commenter · 2023-02-09T19:57:52Z

Codecov Report

Patch coverage: 97.70% and project coverage change: +1.56 🎉

Comparison is base (137b77c) 91.06% compared to head (f2314dc) 92.63%.

📣 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    #2024      +/-   ##
==========================================
+ Coverage   91.06%   92.63%   +1.56%     
==========================================
  Files          99      103       +4     
  Lines       51724    67426   +15702     
  Branches    51724    67426   +15702     
==========================================
+ Hits        47105    62462   +15357     
- Misses       4619     4964     +345

Impacted Files	Coverage Δ
lightning-invoice/src/utils.rs	`98.67% <97.56%> (+1.08%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`93.78% <100.00%> (+2.31%)`	⬆️

... and 72 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.

dunxen

LGTM

tnull

I'm not sure about the benefit of including route hints for channels that are not ready yet. IIUC, route hints have the semantics of "you can use these channels to route". If the included channel can't be used to route payments yet, wouldn't we just set the third party trying to pay the invoice up for payment failures that are even worse than the status quo (because they fail only after a delay)?

I see the utility of smoothing out the propagation delay of channel announcements, but maybe for this it would be sufficient to start including the route hints after 5 or 6 confirmations, i.e., when channel readiness is immanent?

TheBlueMatt · 2023-02-15T23:36:28Z

We have a separate check for channel.is_usable. The intent here is around nodes that accept channels before 6 confs. Its not uncommon (though I'd argue inadvisable) for nodes to accept channels at 3 confs, but we can't announce that channel until 6. Thus, we have a usable channel, but our peer won't know about it. I'm not sure this is a huge win, but a common issue I've seen here and there is "I just opened a channel and tried to receive a payment, why hasn't it worked?". Shifting from that happening for the first 6/7 blocks to only the first 3 is a nice win, especially if the user pays enough attention to wait for that is_usable flag to flip.

tnull · 2023-02-16T00:39:16Z

We have a separate check for channel.is_usable. The intent here is around nodes that accept channels before 6 confs. Its not uncommon (though I'd argue inadvisable) for nodes to accept channels at 3 confs, but we can't announce that channel until 6. Thus, we have a usable channel, but our peer won't know about it. I'm not sure this is a huge win, but a common issue I've seen here and there is "I just opened a channel and tried to receive a payment, why hasn't it worked?". Shifting from that happening for the first 6/7 blocks to only the first 3 is a nice win, especially if the user pays enough attention to wait for that is_usable flag to flip.

Mh, I'm aware of the recent support requests and assumed this may be the use case. If we want to support less than 6 conf channels, couldn't we make the pre-announcement via invoices a) configurable and/or b) dependent on minimum_depth - 1 or similar?

TheBlueMatt · 2023-02-16T01:14:37Z

Currently if we have any public channel (even an unconfirmed, unusable one) we'll return zero route hints, which guarantees the payment fails if the channel isn't confirmed at 6+ blocks and usable. After this PR, if we have a public channel that has < 7 confirmations but is confirmed we'll include it as a route hint (as long as there are no confirmed public channels), and otherwise fall back to the old behavior.

I'm not 100% sure what changes you're asking for - we could make the selection of a channel entirely dependent on channels being is_usable, i.e. always ignore channels that aren't usable yet, whether public or not, and then continue with this new behavior.

Previously we haven't done that because we've tried to be careful around "accidentally" including route hints for private channels (potentially hurting the users' privacy) if there's any public channels. I'm not sure it matters as much anymore with SCID aliases broadly supported, you just reveal that you have a channel, not your specific UTXO used, but its still nice to avoid.

We've also tried to include a channel even if its not usable because its better to include something than not, but the code here is starting to get really unwieldy, and I'm not sure its worth trying to maintain that behavior - your channel is probably not going to get confirmed between the time when you issue the invoice and the time someone goes to pay it.

tnull · 2023-02-16T14:23:22Z

Currently if we have any public channel (even an unconfirmed, unusable one) we'll return zero route hints, which guarantees the payment fails if the channel isn't confirmed at 6+ blocks and usable. After this PR, if we have a public channel that has < 7 confirmations but is confirmed we'll include it as a route hint (as long as there are no confirmed public channels), and otherwise fall back to the old behavior.

I'm not 100% sure what changes you're asking for - we could make the selection of a channel entirely dependent on channels being is_usable, i.e. always ignore channels that aren't usable yet, whether public or not, and then continue with this new behavior.

Ah, from the PR description I was under the impression that we'd now include public channels with some confirmations, even if they are not ready yet. Only now realized that we'd of course still filter by chan.is_channel_ready, i.e., we'll only include channels that are at least potentially usable at the time of inclusion. This means we're only including route hints for invoices generated during [minimum_depth, 7), which essentially was my main request above.

Previously we haven't done that because we've tried to be careful around "accidentally" including route hints for private channels (potentially hurting the users' privacy) if there's any public channels. I'm not sure it matters as much anymore with SCID aliases broadly supported, you just reveal that you have a channel, not your specific UTXO used, but its still nice to avoid.

Yes, still think we should continue to exclude private hints if any public channels are available.

tnull · 2023-02-16T14:37:39Z

lightning-invoice/src/utils.rs

+				// If we have a public channel, but it doesn't have enough confirmations to (yet)
+				// be in the public network graph (and have gotten a chance to propagate), include
+				// route hints but only for public channels to protect private channel privacy.
+				channel.is_public


Shouldn't this still only include the public channel if it has sufficient capacity to route the payment?

Suggested change

channel.is_public

has_enough_capacity && channel.is_public

Won't this result in not including any hints if we have a public channel that isnt yet broadcasted and also doesnt have enough capacity? I mean basically in that case we're screwed anyway, but we might as well include the hint?

Right, and I argue that we should never include a hint for a channel that never will become usable for paying the given invoice. I agree we're screwed anyways, but the UX of the payer side is different: with no route hints the payment fails immediately, with the unusable channel included there is likely a delay for establishing the HTLC along the route, plus some liquidity will be locked unnecessarily while this is in flight. The payer won't be able to simply ignore the route hint as it's the only link to the destination and doesn't include the channel capacity.

I think the reason we did that is because we may have multiple such channels and its possible the sender may find a path through MPP. We could filter them back out at the end if there's only one channel, but...yet more code.

Yes, for the MPP case it's nice to include them anyways, however, we don't do the same for unannounced channels, no? So if we do it here, there will be an inconsistency, as we'll only include hints to private channels that could route the full amount, while for public channels we trust on an aggregate MPP route being present?

Hmm? the branches below check for min_capacity_channel_exists before checking has_enough_capacity, so I believe we do?

Sure, just meant to highlight that in case that in case of private channels we have at least a strong preference towards channels that can route the full capacity, while we here now don't for any public channels. Doesn't matter all too much though.

ViktorTigerstrom

Generally looks good :)! Though leaving some general suggestions and questions:

If a user has 2 public channels (to different nodes), one with 7+ confirmations, and second channel that falls within the has_pub_unconf_chan case, I think it probably still would make sense to include the second channel as a route hint. Or would we like to keep the current behavior which wouldn't include any route hints at all in this case?
I guess this is a general comment about our filtering when we have public channels:
If we both public and private channels, and all public channels have is_useable == false, but we have private channels with is_useable == true, I think it would make sense to include those private channels in the route hints. If you agree, we should have the same behavior for ready public channels with < 7 confs, ie. we should only set has_pub_unconf_chan to true if is_useable is true for that channel.
Maybe this should be done in a separate PR though, if you agree that this should be the intended behavior.

lightning-invoice/src/utils.rs

ViktorTigerstrom

LGTM after squash! Leaving some very nit-picky comments that you may ignore if you prefer to.

lightning-invoice/src/utils.rs

TheBlueMatt · 2023-03-06T19:18:30Z

Squashed with trivial doc comments applied -

$ git diff-tree -U1 10ad8e289 3028e88f8
diff --git a/lightning-invoice/src/utils.rs b/lightning-invoice/src/utils.rs
index 6b049c700..ad7b03017 100644
--- a/lightning-invoice/src/utils.rs
+++ b/lightning-invoice/src/utils.rs
@@ -514,5 +514,6 @@ fn _create_invoice_from_channelmanager_and_duration_since_epoch_with_payment_has
 ///   `is_usable` (i.e. the peer is connected).
-/// * If any public channel exists, only public [`RouteHint`]s will be returned,
-/// * If any public, announced channel exists, no [`RouteHint`]s will be returned, as the sender
-///   is expected to find the path by looking at the public channels instead.
+/// * If any public channel exists, only public [`RouteHint`]s will be returned.
+/// * If any public, announced, channel exists (i.e. a channel with 7+ confs, to ensure the
+///   announcement has had a chance to propagate), no [`RouteHint`]s will be returned, as the
+///   sender is expected to find the path by looking at the public channels instead.
 fn filter_channels<L: Deref>(

alecchendev

LGTM! From all the discussion of the different cases to include or filter route hints, would it maybe be helpful to allow the user to make these decisions? It's probably not super necessary until users ask for it, but just a thought

lightning-invoice/src/utils.rs

alecchendev · 2023-03-08T23:41:32Z

lightning-invoice/src/utils.rs

-			log_trace!(logger, "Not including channels in invoice route hints on account of public channel {}",
-				log_bytes!(channel.channel_id));
-			return vec![]
+			if channel.confirmations.is_some() && channel.confirmations < Some(7) {


Is there no constant for min confirmations required for broadcast? The comment below explains things well but just wondering

Looks like no, Channel::get_announcement_sigs just has a hard-coded conf_height + 5 > cur_height.

tnull

Please excuse the delay here! Generally LGTM, possibly mod the inconsistent behavior regarding the capacity-based filtering discussed above.

tnull · 2023-03-10T13:20:06Z

lightning-invoice/src/utils.rs

+				// If we have a public channel, but it doesn't have enough confirmations to (yet)
+				// be in the public network graph (and have gotten a chance to propagate), include
+				// route hints but only for public channels to protect private channel privacy.
+				channel.is_public


Yes, for the MPP case it's nice to include them anyways, however, we don't do the same for unannounced channels, no? So if we do it here, there will be an inconsistency, as we'll only include hints to private channels that could route the full amount, while for public channels we trust on an aggregate MPP route being present?

lightning-invoice/src/utils.rs

tnull

LGTM, feel free to squash fixups.

If we have a public channel which doesn't yet have six confirmations the network can't possibly know about it as we cannot have announced it yet. However, because we refuse to include route-hints if we have any public channels, we will generate invoices that no one can pay. Thus, if we have any public, not-yet-announced channels, include them as a route-hint.

TheBlueMatt · 2023-03-18T00:45:52Z

Squashed fixups.

tnull

LGTM, CI failures are unrelated.

TheBlueMatt added the Seeking Code Review label Feb 10, 2023

dunxen previously approved these changes Feb 15, 2023

View reviewed changes

tnull reviewed Feb 15, 2023

View reviewed changes

tnull removed the Seeking Code Review label Feb 15, 2023

tnull reviewed Feb 16, 2023

View reviewed changes

ViktorTigerstrom reviewed Feb 20, 2023

View reviewed changes

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

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

TheBlueMatt dismissed dunxen’s stale review via fe5d901 February 21, 2023 18:27

TheBlueMatt force-pushed the 2023-02-6conf-pub-hints branch from fe5d901 to 10ad8e2 Compare February 21, 2023 18:29

ViktorTigerstrom reviewed Feb 25, 2023

View reviewed changes

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

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

TheBlueMatt force-pushed the 2023-02-6conf-pub-hints branch from 10ad8e2 to 3028e88 Compare March 6, 2023 19:18

alecchendev reviewed Mar 8, 2023

View reviewed changes

tnull reviewed Mar 10, 2023

View reviewed changes

freddiekrugerrand reviewed Mar 14, 2023

View reviewed changes

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

tnull reviewed Mar 16, 2023

View reviewed changes

TheBlueMatt force-pushed the 2023-02-6conf-pub-hints branch from f2314dc to cab6c97 Compare March 18, 2023 00:45

tnull approved these changes Mar 18, 2023

View reviewed changes

dunxen approved these changes Mar 18, 2023

View reviewed changes

TheBlueMatt merged commit 48fa2fd into lightningdevkit:main Mar 19, 2023

TheBlueMatt mentioned this pull request Mar 20, 2023

Limit route hints to three channels #2044

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include a route hint for public, not-yet-announced channels #2024

Include a route hint for public, not-yet-announced channels #2024

TheBlueMatt commented Feb 9, 2023

codecov-commenter commented Feb 9, 2023 •

edited

Loading

dunxen left a comment

tnull left a comment

TheBlueMatt commented Feb 15, 2023

tnull commented Feb 16, 2023 •

edited

Loading

TheBlueMatt commented Feb 16, 2023

tnull commented Feb 16, 2023

tnull Feb 16, 2023

TheBlueMatt Feb 16, 2023

tnull Feb 16, 2023 •

edited

Loading

TheBlueMatt Feb 16, 2023

tnull Mar 10, 2023

TheBlueMatt Mar 15, 2023

tnull Mar 16, 2023

ViktorTigerstrom left a comment

ViktorTigerstrom left a comment

TheBlueMatt commented Mar 6, 2023

alecchendev left a comment

alecchendev Mar 8, 2023

TheBlueMatt Mar 9, 2023

tnull left a comment

tnull Mar 10, 2023

tnull left a comment

TheBlueMatt commented Mar 18, 2023

tnull left a comment

Include a route hint for public, not-yet-announced channels #2024

Include a route hint for public, not-yet-announced channels #2024

Conversation

TheBlueMatt commented Feb 9, 2023

codecov-commenter commented Feb 9, 2023 • edited Loading

Codecov Report

dunxen left a comment

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Feb 15, 2023

tnull commented Feb 16, 2023 • edited Loading

TheBlueMatt commented Feb 16, 2023

tnull commented Feb 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull Feb 16, 2023 • 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

ViktorTigerstrom left a comment

Choose a reason for hiding this comment

ViktorTigerstrom left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Mar 6, 2023

alecchendev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Mar 18, 2023

tnull left a comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 9, 2023 •

edited

Loading

tnull commented Feb 16, 2023 •

edited

Loading

tnull Feb 16, 2023 •

edited

Loading