Optional compact blinded path creation #3080

jkczyz · 2024-05-22T22:09:19Z

Follow-up to #3011, which expands the MessageRouter trait with a separate create_compact_blinded_path method. This allows callers to decided whether or not to use compact blinded paths depending on the context. Updates ChannelManager to only use compact blinded paths for Offer::paths and Refund::paths. Reply paths use non-compact blinded paths instead.

Based on #3011.

codecov-commenter · 2024-05-22T22:49:14Z

Codecov Report

Attention: Patch coverage is 93.36870% with 25 lines in your changes missing coverage. Please review.

Project coverage is 89.86%. Comparing base (2701bc5) to head (c17a026).

Files	Patch %	Lines
lightning/src/onion_message/messenger.rs	78.68%	13 Missing ⚠️
lightning/src/ln/offers_tests.rs	97.29%	4 Missing and 2 partials ⚠️
lightning/src/util/test_utils.rs	57.14%	6 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3080      +/-   ##
==========================================
+ Coverage   89.84%   89.86%   +0.01%     
==========================================
  Files         119      119              
  Lines       97551    97834     +283     
  Branches    97551    97834     +283     
==========================================
+ Hits        87644    87916     +272     
- Misses       7332     7350      +18     
+ Partials     2575     2568       -7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TheBlueMatt · 2024-05-28T19:35:03Z

lightning/src/onion_message/messenger.rs

-				}
-			},
-		}
+	fn create_compact_blinded_paths<


Hmm, this is a pretty awkward API. IMO whether the intro and forwarding hops are SCID-based should all be one thing (is it a "long-lived" path), but here we have the caller pick whether the middle nodes are compact in the argument and the function call decides if the intro node is compact, splitting the two.

We can't really have the implementation pick without parameterizing it with a ShortChannelIdLookUp, which we removed in the #3011 (see #3011 (comment)). Also, once we have three-hop blinded paths, the implementation will need to make the decision for the additional hop using a NetworkGraph, as it does for the introduction node. Do you prefer re-introducing ShortChannelIdLookUp so that it is all in one place?

We can't really have the implementation pick without parameterizing it with a ShortChannelIdLookUp, which we removed in the #3011 (see #3011 (comment)).

We could go the other way, right? Always create a non-compact path (just do whatever the hops passed say to do) and then have the caller compact the path if they want. That doesn't allow us to communicate to the MessageRouter that we want a fewer-hop-count path, though.

I guess maybe the variation between create_blinded_paths taking PublicKeys and create_compact_blinded_paths taking ForwardNodes is maybe enough, it just seems weird that the scids in ForwardNodes are Optional in that API.

We could go the other way, right? Always create a non-compact path (just do whatever the hops passed say to do) and then have the caller compact the path if they want. That doesn't allow us to communicate to the MessageRouter that we want a fewer-hop-count path, though.

For the introduction node, the caller can't compact if they don't have a NetworkGraph to use, which ChannelManager doesn't directly (only via DefaultRouter). For intermediate nodes, the caller can't compact after the path is already built since the scid is in the encrypted data. And for three-hop paths, the same NetworkGraph issues arises.

I guess maybe the variation between create_blinded_paths taking PublicKeys and create_compact_blinded_paths taking ForwardNodes is maybe enough, it just seems weird that the scids in ForwardNodes are Optional in that API.

Yeah, I think that may be the most straight-forward option. As for the Optional scid in ForwardNode, it makes degrading to a partial compact path (i.e., only introduction node compact) simple. See #3011 (comment).

Yea, maybe I'm just being grouchy, I guess its fine.

TheBlueMatt · 2024-05-28T19:35:45Z

lightning/src/ln/channelmanager.rs

@@ -8546,8 +8547,8 @@ macro_rules! create_offer_builder { ($self: ident, $builder: ty) => {
 	///
 	/// # Privacy
 	///
-	/// Uses [`MessageRouter::create_blinded_paths`] to construct a [`BlindedPath`] for the offer.
-	/// However, if one is not found, uses a one-hop [`BlindedPath`] with
+	/// Uses [`MessageRouter::create_compact_blinded_paths`] to construct a [`BlindedPath`] for the


IMO this should be configurable - I'd like to be able to communicate to LDK whether I want a "long-lived" offer or a "short-lived" one, and that should feed into the number of hops and whether we use compact paths.

How we do that is the open question that I posed in #3011 (comment).

Regarding number of hops, should we prefer more hops for short-lived offers? Should we communicate this to MessageRouter in some way?

How we do that is the open question that I posed in #3011 (comment).

I don't have a super strong opinion on how, the options you list there all seem fine.

Regarding number of hops, should we prefer more hops for short-lived offers? Should we communicate this to MessageRouter in some way?

IMO yes and yes. Presumably when we do three-hop blinded paths it should only be for short-lived offers.

Went with the approach of passing the absolute expiry to create_offer_builder.

valentinewallace · 2024-05-29T15:06:33Z

Can be rebased now that #3011 landed, also CI needs a fix.

valentinewallace

Basically LGTM. CI needs another fix.

valentinewallace · 2024-05-23T18:43:49Z

lightning/src/onion_message/messenger.rs

+	/// Creates [`BlindedPath`]s to the `recipient` node. The nodes in `peers` are assumed to be
+	/// direct peers with the `recipient`.


These docs are identical to create_blinded_paths above, maybe could use some more information about what makes this different/when this should be used instead?

Thanks for catching that. I meant to update them before moving out of draft, but it seems I forgot.

lightning/src/onion_message/messenger.rs

TheBlueMatt

Basically LGTM

TheBlueMatt · 2024-05-29T20:55:57Z

lightning/src/ln/channelmanager.rs

+	/// Uses [`MessageRouter`] to construct a [`BlindedPath`] for the offer. However, if one is not
+	/// found, uses a one-hop [`BlindedPath`] with [`ChannelManager::get_our_node_id`] as the
+	/// introduction node instead. In the latter case, the node must be announced, otherwise, there
+	/// is no way to find a path to the introduction in order to send the [`InvoiceRequest`].


We should probably include some text about how offers with an expiry less than $PUBLIC_CONSTANT may have somewhat better privacy and will be more compact whereas long-lived offers will use additional data to improve longevity of the offer.

Would it be worth making a UserConfig parameter for this instead of using the hardcoded constant?

Hmm, if we're gonna do that we should just expose a bool to select if the offer is compact directly. I'm not sure sure if it's worth bothering, though, people really shouldn't have a compact offer for an offer that doesn't expire and I'm not really sure how much anyone wants an offer that expires in a month (vs forever or in an hour), it seems like there's going to be a very strong bimodal distribution (if there's even a top vs just being no-expiry).

Fair enough. Leaving as is except using a public constant. Updated docs accordingly.

lightning/src/ln/channelmanager.rs

valentinewallace · 2024-05-31T15:02:25Z

lightning/src/ln/channelmanager.rs

+	/// `absolute_expiry` according to [`MAX_SHORT_LIVED_RELATIVE_EXPIRY`]. When parameterized by
+	/// [`DefaultRouter`] (and thus using [`DefaultMessageRouter`), however, if a path is not found,


"When parameterized by... (and thus using ..), however, if a path is not found, ..." sounds pretty awkward to me, is there a way to rephrase?

Re-phrased this quite a bit now. Let me know if it is less awkward.

lightning/src/ln/offers_tests.rs

TheBlueMatt · 2024-05-31T22:00:29Z

lightning/src/ln/channelmanager.rs

+	/// `absolute_expiry` according to [`MAX_SHORT_LIVED_RELATIVE_EXPIRY`]. See those docs for
+	/// privacy implications.
+	///
+	/// The [`Router`] used to parameterized [`ChannelManager`] may also affect privacy since it


If we're gonna describe the specific behavior of DefaultMessageRouter here we should leave a comment in DefaultMessageRouter reminding us to update the docs here if we change the implementation (but it might just be better to link to DefaultMessageRouter and discuss its behavior there).

Fair point. Re-arranged the docs accordingly.

TheBlueMatt · 2024-05-31T22:01:56Z

lightning/src/ln/channelmanager.rs

+/// - [`MessageRouter::create_blinded_paths`] when long-lived.
+///
+/// Using compact [`BlindedPath`]s may provide better privacy as more hops can be used with the same
+/// amount of bytes. However, since they use short channel ids instead of pubkeys, they are more


I'm not sure its because of the byte count (I guess it is, in part), but more that the router could/should be selecting a longer path if its compact.

lightning/src/onion_message/messenger.rs

TheBlueMatt · 2024-06-04T13:57:55Z

lightning/src/ln/channelmanager.rs

 	/// Creates a blinded path by delegating to [`MessageRouter::create_blinded_paths`].
 	///
 	/// Errors if the `MessageRouter` errors or returns an empty `Vec`.
 	fn create_blinded_path(&self) -> Result<BlindedPath, ()> {
 		let recipient = self.get_our_node_id();
 		let secp_ctx = &self.secp_ctx;

+		let peers = self.per_peer_state.read().unwrap()
+			.iter()
+			.filter(|(_, peer)| peer.lock().unwrap().latest_features.supports_onion_messages())


Not a change in this PR, but is there a reason we don't filter by connected peers here and below?

Ugh, looks like a mistake. I've added a fix.

TheBlueMatt · 2024-06-04T14:06:23Z

lightning/src/ln/channelmanager.rs

+		let now = Duration::from_secs(
+			self.highest_seen_timestamp.load(Ordering::Acquire) as u64
+		);
+		#[cfg(feature = "std")]


Until we upgrade to rust-bitcoin 0.32, we should avoid assuming we can access time even with std :(.

Hmmm... we are already doing so in timer_tick_occurred and OffersMessageHandler.

Ah, I'd missed that, we need to fix that (or mutiny is gonna be mad...).

Is there anything blocking us from upgrading now?

Just someone doing the work, I think. @tnull should chime in though, it may be that BDK is on 0.31 and we want to ship an 0.31 release first so that we can sync with the existing BDK code before jumping to 0.32.

I discussed that with the BDK team in the meeting last week. BDK is close to finishing the upgrade and it will land ~next release, at the very least before the feature that keeps LDK Node from upgrading (data model backwards compat). So we're free to (and in fact should ASAP) upgrade to 0.32. I can see to prioritize it by the end of the week. Would this be sufficient to have it land it first, so that we don't have to deal with the things mentioned above here?

Yeah, preferably so. I'm not sure what our alternative is, though I don't think we're in any big rush to land this. Presumably we'll want it for the next release but waiting on 0.32 shouldn't hold up the review at least.

I think we need to fix this for the next release one way or another (#3097). Given its already a bug IMO we should just land this and then handle the fixups as a part of #3097.

Alright, now tracking here: #3100

Will see to pick it up soon if nobody is already working on it.

TheBlueMatt · 2024-06-05T21:48:58Z

Needs rebase, and feel free to squash.

jkczyz · 2024-06-05T21:57:56Z

@TheBlueMatt Currently we don't use the compact representation for reply paths. In #2793, we'll soon only create three-hop paths for the compact representation. Are we ok with using only two hops for reply paths?

An upcoming change to the MessageRouter trait will require reusing DefaultMessageRouter::create_blinded_paths. Move the code to a utility function so facilitate this.

Using compact blinded paths isn't always necessary or desirable. For instance, reply paths are communicated via onion messages where space isn't a premium unlike in QR codes. Additionally, long-lived paths could become invalid if the channel associated with the SCID is closed. Refactor MessageRouter::create_blinded_paths into two methods: one for compact blinded paths and one for normal blinded paths.

There's no need to save space when creating reply paths since they are part of onion messages rather than in QR codes. Use normal blinded paths for these instead as they are less likely to become invalid in case of channel closure.

When an offer is short-lived, the likelihood of a channel used in a compact blinded path going away is low. Require passing the absolute expiry of an offer to ChannelManager::create_offer_builder so that it can be used to determine whether or not compact blinded path should be used. Use the same criteria for creating blinded paths for refunds as well.

When reconnecting nodes, make sure to notify OnionMessenger that the nodes are now connected.

When calling MessageRouter::create_blinded_path, ChannelManager was including disconnected peers. Filter peers such that only connected ones are included. Otherwise, the resulting BlindedPath may not work.

The docs assumed ChannelManager is parameterized by DefaultRouter, which may not be the case. Clarify the behavior is specific to using DefaultRouter.

TheBlueMatt · 2024-06-06T14:25:13Z

Ah, good point. Yea, we might need a more holistic way to think about how much we're revealing when we expose a different blinded path for payments than we do for messaging. Really we use the same path for both to avoid revealing info that can be used to triangulate, but that's a somewhat larger change so I think we can let it slide for now and come back to it.

valentinewallace · 2024-06-07T18:18:42Z

lightning/src/ln/channelmanager.rs

+/// Using [`ChannelManager::create_offer_builder`] or [`ChannelManager::create_refund_builder`],
+/// will included a [`BlindedPath`] created using:


nit:

Suggested change

/// Using [`ChannelManager::create_offer_builder`] or [`ChannelManager::create_refund_builder`],

/// will included a [`BlindedPath`] created using:

/// Using [`ChannelManager::create_offer_builder`] or [`ChannelManager::create_refund_builder`]

/// will include a [`BlindedPath`] created using:

jkczyz mentioned this pull request May 22, 2024

Compact blinded path creation #3011

Merged

jkczyz force-pushed the 2024-05-compact-blinded-path-creation-trait branch 2 times, most recently from 2d2a087 to 3433b70 Compare May 22, 2024 22:28

jkczyz force-pushed the 2024-05-compact-blinded-path-creation-trait branch from 3433b70 to 1d1bcba Compare May 23, 2024 20:53

TheBlueMatt reviewed May 28, 2024

View reviewed changes

jkczyz force-pushed the 2024-05-compact-blinded-path-creation-trait branch from 1d1bcba to 5c28c67 Compare May 28, 2024 21:42

jkczyz marked this pull request as ready for review May 28, 2024 23:52

jkczyz force-pushed the 2024-05-compact-blinded-path-creation-trait branch from d8f306f to 102ffe4 Compare May 29, 2024 15:16

valentinewallace reviewed May 29, 2024

View reviewed changes

jkczyz force-pushed the 2024-05-compact-blinded-path-creation-trait branch from 102ffe4 to e9b2cf4 Compare May 29, 2024 16:57

TheBlueMatt reviewed May 29, 2024

View reviewed changes

valentinewallace reviewed May 31, 2024

View reviewed changes

jkczyz force-pushed the 2024-05-compact-blinded-path-creation-trait branch from 51036ec to 96ad768 Compare May 31, 2024 19:05

jkczyz mentioned this pull request May 31, 2024

Three-hop blinded paths #2793

Draft

TheBlueMatt reviewed May 31, 2024

View reviewed changes

jkczyz force-pushed the 2024-05-compact-blinded-path-creation-trait branch from 96ad768 to ffbe363 Compare May 31, 2024 22:50

TheBlueMatt reviewed Jun 4, 2024

View reviewed changes

jkczyz force-pushed the 2024-05-compact-blinded-path-creation-trait branch from ffbe363 to d2d1694 Compare June 4, 2024 23:55

Move DefaultMessageRouter::create_blinded_paths

defc540

An upcoming change to the MessageRouter trait will require reusing DefaultMessageRouter::create_blinded_paths. Move the code to a utility function so facilitate this.

jkczyz force-pushed the 2024-05-compact-blinded-path-creation-trait branch from d2d1694 to ae6e5ff Compare June 5, 2024 22:46

jkczyz added 6 commits June 5, 2024 17:52

Don't use compact blinded paths for reply paths

411b9b4

There's no need to save space when creating reply paths since they are part of onion messages rather than in QR codes. Use normal blinded paths for these instead as they are less likely to become invalid in case of channel closure.

Fix functional_test_utils::reconnect_nodes

15fa0d8

When reconnecting nodes, make sure to notify OnionMessenger that the nodes are now connected.

Exclude disconnected peers from BlindedPath

7db6616

When calling MessageRouter::create_blinded_path, ChannelManager was including disconnected peers. Filter peers such that only connected ones are included. Otherwise, the resulting BlindedPath may not work.

Clarify docs regarding one-hop blinded paths

c17a026

The docs assumed ChannelManager is parameterized by DefaultRouter, which may not be the case. Clarify the behavior is specific to using DefaultRouter.

jkczyz force-pushed the 2024-05-compact-blinded-path-creation-trait branch from ae6e5ff to c17a026 Compare June 5, 2024 22:52

jkczyz mentioned this pull request Jun 6, 2024

Allow blinded path diversification by expanding create_blinded_paths #3087

Open

TheBlueMatt mentioned this pull request Jun 7, 2024

rustfmt: Reformat the fuzz dir – the easy part #3111

Merged

TheBlueMatt approved these changes Jun 7, 2024

View reviewed changes

valentinewallace approved these changes Jun 7, 2024

View reviewed changes

valentinewallace merged commit 88124a9 into lightningdevkit:main Jun 7, 2024
15 of 16 checks passed

		/// Creates [`BlindedPath`]s to the `recipient` node. The nodes in `peers` are assumed to be
		/// direct peers with the `recipient`.

		/// `absolute_expiry` according to [`MAX_SHORT_LIVED_RELATIVE_EXPIRY`]. When parameterized by
		/// [`DefaultRouter`] (and thus using [`DefaultMessageRouter`), however, if a path is not found,

		/// Using [`ChannelManager::create_offer_builder`] or [`ChannelManager::create_refund_builder`],
		/// will included a [`BlindedPath`] created using:

Optional compact blinded path creation #3080

Optional compact blinded path creation #3080

Conversation

jkczyz commented May 22, 2024

codecov-commenter commented May 22, 2024 • edited Loading

Codecov Report

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

jkczyz May 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace commented May 29, 2024

valentinewallace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

tnull Jun 5, 2024 • 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

TheBlueMatt commented Jun 5, 2024

jkczyz commented Jun 5, 2024

TheBlueMatt commented Jun 6, 2024

Choose a reason for hiding this comment

codecov-commenter commented May 22, 2024 •

edited

Loading

jkczyz May 28, 2024 •

edited

Loading

tnull Jun 5, 2024 •

edited

Loading