Enable Creation of Offers and Refunds Without Blinded Path #3246

shaavan · 2024-08-16T08:09:12Z

PR Description:

An Offer and a Refund can exist without a blinded path. In such cases, the corresponding signing_pubkey is used to determine the destination. However, while we currently have the ability to handle Offers and Refunds without a blinded path, we lack the ability to create them this way.

This PR addresses that gap by introducing a Blinded Path Type enum, allowing for a more flexible creation process.

The Key Changes:

Blinded Path Type enum: The creator of an Offer or Refund can now specify the type of blinded path they preferably want.
Additionally, they can opt to specify None, allowing the Offer or Refund to be created without a blinded path.

Important Notes:

While the creator can specify their preference, the final decision on the blinded path properties will rest with the MessageRouter, which works on a best-effort basis.
This design ensures that unnecessary scid lookups required for compact blinded paths are avoided when the creator does not preferably want them to begin with.

By introducing this simple yet effective enum, we provide the flexibility to create Offers and Refunds without a blinded path, while still preserving the core logic of the MessageRouter.

codecov · 2024-08-16T08:18:56Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.01%. Comparing base (d49a08a) to head (6817512).
Report is 21 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3246      +/-   ##
==========================================
+ Coverage   89.59%   90.01%   +0.42%     
==========================================
  Files         127      127              
  Lines      103438   107553    +4115     
  Branches   103438   107553    +4115     
==========================================
+ Hits        92672    96812    +4140     
- Misses       8049     8097      +48     
+ Partials     2717     2644      -73

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

shaavan · 2024-08-19T12:38:27Z

Updated from pr3246.01 to pr3246.02 (diff):

Changes:

Rebase on main to resolve merge conflicts.

shaavan · 2024-08-19T13:29:40Z

Updated from pr3246.02 to pr3246.03 (diff):

Changes:

Fix docs.
Introduce tests for offer and refund with no blinded paths.

shaavan · 2024-08-20T12:17:51Z

Updated from pr3246.03 to pr3246.04 (diff):

Changes:

Rebase on main, to resolve merge conflicts.
Fix CI.

lightning/src/onion_message/messenger.rs

lightning/src/ln/channelmanager.rs

jkczyz · 2024-09-04T03:37:00Z

lightning/src/ln/offers_tests.rs

+	assert_eq!(refund.payer_id(), alice_id);
+	assert!(refund.paths().is_empty());
+}
+
 /// Checks that blinded paths are compact for short-lived offers.
 #[test]
 fn creates_short_lived_offer() {


The naming at least for the following tests is no longer relevant, it seems.

As far as I understand, with the introduction of BlindedPathParameters, it seems like the behavior these tests were checking isn't relevant anymore. Should we think about removing these tests?

Let's wait on the resolution of #3246 (comment). We'll ultimately want to test cases allowed by whatever interface is exposed to the user.

lightning/src/ln/offers_tests.rs

shaavan · 2024-09-04T11:05:53Z

Updated from pr3246.04 to pr3246.05 (diff):

Changes:

Rebase on main.

lightning/src/onion_message/messenger.rs

shaavan · 2024-09-05T20:37:45Z

Updated from pr3246.05 to pr3246.06 (diff):
Addressed @jkczyz comments

Changes:

Removed the global constant PATHS_PLACEHOLDER, and instead use a default constructor, and local DEFAULT_VALUE.
Remove the redundant functions in their appropriate commits.
Use match to avoid mut variables.

shaavan · 2024-09-19T15:40:34Z

Updated from pr3246.06 to pr3246.07 (diff):

Changes:

Introduce a new approach using the BlindedPathType enum.
The enum allows for the specification of the type of Blinded Path (Compact or Full) that a user can specify to create the desired type of Blinded Path.
Update offer_builder and refund_builder so that user can explicitly specify the type of Blinded Path they want to create.
Update the Blinded Path creation flow so that only one function flow is responsible for creating both kinds of Blinded Paths.

shaavan · 2024-09-19T16:03:19Z

Updated from pr3246.07 to pr3246.08 (diff):

Changes:

Rebase on main to resolve merge conflicts.

shaavan · 2024-10-03T12:30:51Z

Updated from pr3246.08 to pr3246.09 (diff):

Changes:

Rebase on main, and fix ci.

lightning/src/onion_message/messenger.rs

jkczyz · 2024-10-04T22:51:51Z

lightning/src/ln/channelmanager.rs

+				let context = MessageContext::Offers(context);
+				let path = $self
+					.create_blinded_paths(context)
+					.and_then(|paths| paths.into_iter().next().ok_or(()))


Hmm... @TheBlueMatt I don't think this interface is sufficient to allow someone to specify they want more than one path in the offer, unless we include all the paths returned by the MessageRouter instead of just one.

Yea, IMO we should include all paths - if the router wants to do something funky in the offer, so be it.

What should we have DefaultMessageRouter do?

What we currently do (use the context of why we're asking for a path to decide how many paths to include)?

I suppose DefaultMessageRouter could use both MessageContex and BlindedPathType -- or which MessageRouter method is called, if we revert to that state -- to determine if more than one path should be returned. That way we'd allow users to pass BlindedPathType::Full to create_offer_builder such that they can create an offer with more than one path (e.g., when an offer doesn't need to be in a QR code).

Right, that makes sense but then I'm confused about your previous comment about keeping get interface the same as it is?

If we change MessageRouter to only have one method, then we need to pass in BlindedPathType. But then the peers parameter will need to be Vec<MessageForwardNode> even when used with BlindedPathType::Full instead of Vec<PublicKey>. So callers could pass in a MessageForwardNode with an scid even though it isn't expected, resulting in the last hop being compact.

So callers could pass in a MessageForwardNode with an scid even though it isn't expected, resulting in the last hop being compact.

Makes sense! I have reintroduced the two function flow back in pr3246.11

Also, I have updated the DefaultMessageRouter's create_compact_blinded_paths to only return a single path in pr3246.11

I'm confused why this is a concern, if the MessageRouter insists on a specific type of blinded path in general, so what? Ideally the MessageRouter gets to pick what it wants to do, though I think we're not allowed to have a compact introduction point in reply paths? Even in that case, though, ISTM we can communicate the restrictions and if the MessageRouter is buggy we can just fail.

I'm confused why this is a concern, if the MessageRouter insists on a specific type of blinded path in general, so what? Ideally the MessageRouter gets to pick what it wants to do,

Nothing is preventing a MessageRouter from doing so. It would just need access to the channels.

It's just kinda ugly for DefaultMessageRouter to need to clear short_channel_id depending on how it is called. Plus, ChannelManager needs to do additional lookups to get this information just to have it discarded. I'm fine either way, but it doesn't optimize the usual case (i.e., use non-compact paths for reply paths).

though I think we're not allowed to have a compact introduction point in reply paths? Even in that case, though, ISTM we can communicate the restrictions and if the MessageRouter is buggy we can just fail.

Yeah, though it doesn't look like we are enforcing this at the moment.

lightning/src/ln/channelmanager.rs

lightning/src/onion_message/messenger.rs

shaavan · 2024-10-11T14:28:53Z

Updated from pr3246.09 to pr3246.10 (diff):
Addressed @jkczyz comments

Changes:

Update BlindedPathType documentation.
Update code to amend all returned paths by MessageRouter to offers, and refund
DRY create_offer_builder, and create_refund_builder

lightning/src/ln/channelmanager.rs

shaavan · 2024-10-19T12:29:45Z

Updated from pr3246.10 to pr3246.11 (diff):
Addressed @bluematt & @jkczyz comments

Changes:

Reverted the "Unified Flow" commits, hence re-introducing the two function flows for blinded path creation.
Squashed the rest of the commits.
Moved BlindedPathType to channelmanager, as it is only used there.
Update DefaultMessageRouter::create_compact_blinded_paths to only return a single path.

shaavan · 2024-10-19T12:39:10Z

Updated from pr3246.11 to pr3246.12 (diff):

Changes:

Rebase on main to resolve conflicts

TheBlueMatt · 2024-10-19T15:30:27Z

lightning/src/ln/channelmanager.rs

@@ -222,6 +222,34 @@ pub enum PendingHTLCRouting {
 	},
 }

+/// Represents the types of [`BlindedMessagePath`] that can be created.


I don't think we should be requesting specific types of compact paths from the ChannelManager. This is still the ChannelManager deciding what kind of blinded path it wants, but IMO we should be moving towards the ChannelManager communicating why it wants a blinded path and letting the MessageRouter pick what kind of blinded path is best (number of paths, number of hops, type, etc).

Could you clarify what sort of interface you want for create_offer_builder and create_refund_builder? It's not clear to me what you're looking for that will satisfy the three use cases that I outlined in #3246 (comment). Are you just asking for a rename or something else?

TheBlueMatt · 2024-10-19T15:45:21Z

lightning/src/ln/channelmanager.rs

+				let context = MessageContext::Offers(context);
+				let path = $self
+					.create_blinded_paths(context)
+					.and_then(|paths| paths.into_iter().next().ok_or(()))


I'm confused why this is a concern, if the MessageRouter insists on a specific type of blinded path in general, so what? Ideally the MessageRouter gets to pick what it wants to do, though I think we're not allowed to have a compact introduction point in reply paths? Even in that case, though, ISTM we can communicate the restrictions and if the MessageRouter is buggy we can just fail.

TheBlueMatt · 2024-10-19T15:46:03Z

lightning/src/ln/channelmanager.rs


-		let builder = match absolute_expiry {
-			None => builder,
-			Some(absolute_expiry) => builder.absolute_expiry(absolute_expiry),


Don't we need to keep passing absolute expiry? Presumably this would be a part of the information communicated to the router, as well.

Could you be explicit about what you want the MessageRouter interface to look like? Feels like we are going back and forth without much progress...

Currently this PR isn't changing MessageRouter, but I was thinking basically (but with better names):

enum WhyWeWantABlindedPath { ForOffer { offer_expiry_time, ... }, ForRefund { ... }, ForResponsePath { ... }, } trait MessageRouter { fn create_blinded_path(&self, recipient: PublicKey, context: MessageContext, why: WhyWeWantABlindedPath, peers: Vec<MessageForwardNode>, secp_ctx: &Secp256k1<T>) -> Result<Vec<BlindedMessagePath>, ...>; }

ie focusing very much on the why, not the how in the interface. You're right that building the MessageForwardNode when we don't actually want a compact path is a bit more annoying, but iterating the channel list for each peer after we already have the peer look should be basically free.

I think this approach makes sense!

Assuming the scid lookup is almost free, if MessageRouter has access to MessageForwardNode and WhyWeWantABlindedPath, it can make the right choice between creating a full-length or compact blinded path.

Additionally, if it’s the main decider on path compactness, it could manage the following setting:

for path in &mut paths { path.use_compact_introduction_node(&network_graph); }

This way, it can help avoid creating a compact last hop for full-length paths.

To support the three cases Jeff mentioned in his comment, we could consider updating the create_offer_builder input fields like so:

fn create_offer_builder(..., why: Option<WhyWeWantABlindedPath>) -> Offer { ... }

Passing None supports creating an offer without a blinded path.

Passing WhyWeWantABlindedPath::ForOffer { offer_expiry_time, is_size_constraint: true, ... } covers the offer for QR codes.

Passing WhyWeWantABlindedPath::ForOffer { offer_expiry_time, is_size_constraint: false, ... } covers the unconstrained setting (e.g., an offer on wire).

@TheBlueMatt

I do have a couple of things I’m curious about with this approach:

→ Since MessageRouter is a pub trait and could be implemented outside of LDK, what if someone uses it in a setting where scid lookup for creating MessageForwardNode is resource-intensive?

→ Would it be useful for MessageRouter to send a note upstream on which type of Blinded Path it ultimately created?

Seems like there would be some duplication with MessageContext. And if a new variant is added to MessageContext, then one would need to be added to WhyWeWantABlindedPath, too. How about:

pub enum BlindedPathUse { Persistent, Ephemeral { expiry: Duration }, ReplyPath, }

That way were aren't tying it strictly to existing BOLT12 use cases.

Hmm, maybe? I was thinking about the context because someone might want to have a different policy for refunds vs offers, no matter the specific expiry, but maybe we don't need to support that?

It's still possible to differentiate using the passed MessageContext.

Indeed, supporting non-BOLT12 is a bit difficult, though, so maybe the more generic terminology is right.

Yeah, I think I'd prefer that, but we still need to figure out the ChannelManager API. We'll need to continue to pass the absolute expiry, but now we need another parameter to indicate whether to include any blinded paths. Using Option<BlindedPathUse> won't work since BlindedPathUse::ReplyPath isn't applicable.

We could drop that variant in favor of using Persistent for both that and long-lived offers/refund given we don't need to use the compact representation for reply paths, even though their lifetime is ephemeral.

but we still need to figure out the ChannelManager API. We'll need to continue to pass the absolute expiry, but now we need another parameter to indicate whether to include any blinded paths. Using Option won't work since BlindedPathUse::ReplyPath isn't applicable.

I'm not sure I'm convinced it needs to change. The interface currently allows someone to decide to/not to include a blinded path based on the type of thing we're building, so the question is whether someone is going to want to generate both blinded and non-blinded versions of the same object(s) at the same time in the same node. I can see someone wanting an option to switch it, but that option can be in their route-wrapper, the question is if they will hit races in doing so.

If a user does want this, however, they might also want some different path selection that isn't just "blinded path or not", they might want more/fewer/longer/shorter/compact/non-compact paths based on the specific call. The only way to allow for that, I think, is to pass some kind of opaque "ID" through the ChannelManager to the router which the user can map on.

I'm not sure I'm convinced it needs to change. The interface currently allows someone to decide to/not to include a blinded path based on the type of thing we're building, so the question is whether someone is going to want to generate both blinded and non-blinded versions of the same object(s) at the same time in the same node. I can see someone wanting an option to switch it, but that option can be in their route-wrapper, the question is if they will hit races in doing so.

Regarding races, seems we could make methods like create_offer_builder_using_router in order to pass a custom router. DefaultMessageRouter is cheap to construct, so I can imagine we could also provide methods on it to customize it for one-time use (i.e., by wrapping it). For example:

let router = DefaultMessageRouter::new(network_graph, entropy_source).without_blinded_paths(); let offer = channel_manager.create_offer_builder_with_router(router).build().unwrap();

Or provide a separate MessageRouter implementation that returns no paths. Either approach would have equivalent behavior.

If a user does want this, however, they might also want some different path selection that isn't just "blinded path or not", they might want more/fewer/longer/shorter/compact/non-compact paths based on the specific call. The only way to allow for that, I think, is to pass some kind of opaque "ID" through the ChannelManager to the router which the user can map on.

Passing a custom router would be cleaner, IMO, given it can be essentially stateless. ChannelManager only needs to hold one for replies. Though arguably a future OffersMessageFlow would own that instead. In that world, we could have the OffersMessageFlow's user-provided trait parameterization specify how to create reply routes. It would make it such that OffersMessageFlow wouldn't need a direct MessageRouter parameterization, though that may not be an entirely necessary thing to do.

Regarding races, seems we could make methods like create_offer_builder_using_router in order to pass a custom router.

Makes sense to me. Hadn't considered that. Keeping today's methods and default to the ChannelManager MessageRouter, and then we can add methods to override it if a user wants, makes sense.

In that world, we could have the OffersMessageFlow's user-provided trait parameterization specify how to create reply routes. It would make it such that OffersMessageFlow wouldn't need a direct MessageRouter parameterization, though that may not be an entirely necessary thing to do.

Hmm, not quite visualizing this one but ISTM that we'd want OffersMessageFlow to do most of the things ChannelManager does today, mostly just moving the code into the new flow manager.

1. Introduce a new enum that allows the user to specify the type of Blinded Path (whether Compact or Full-Length) when creating a Blinded Path. 2. This PR utilizes this new enum in two ways in the upcoming commits: - Allows explicitly providing the type of Blinded Path to be created instead of relying on the absolute expiry time for offers and refunds. - Simplifies the Blinded Path creation process by enabling a single function flow for creating both types of Blinded Path.

…ilder` 1. This commit introduces flexibility for the user to specify the type of Blinded Path they want, rather than relying on the Offers' absolute expiry to determine the Blinded Path type. 2. It also adds support for creating an Offer without a Blinded Path, using its signing public key for responding to the offer instead.

Similar to the offer case, this commit introduces the ability to explicitly specify the Blinded Path type.

- Instead of appending just a single blinded path to offers and refunds, update the code to append all paths returned by MessageRouter. - This change allows MessageRouter to control the number of blinded paths appended to offers and refunds, providing flexibility for them to contain more than one path.

shaavan · 2024-10-24T12:17:53Z

Updated from pr3246.12 to pr3246.13 (diff):

Changes:

Rebase on main.

shaavan marked this pull request as draft August 16, 2024 12:09

shaavan force-pushed the blinded_api branch from 53a3d18 to 6fa28be Compare August 16, 2024 12:09

jkczyz self-requested a review August 16, 2024 14:36

shaavan mentioned this pull request Aug 19, 2024

Exploring the Integration of Dummy Hops into Blinded Paths #3252

Open

shaavan force-pushed the blinded_api branch from 6fa28be to 52efbc6 Compare August 19, 2024 12:38

shaavan force-pushed the blinded_api branch from 52efbc6 to 79d6fdd Compare August 19, 2024 13:29

shaavan marked this pull request as ready for review August 19, 2024 13:30

shaavan force-pushed the blinded_api branch from 79d6fdd to 0d9ac2a Compare August 20, 2024 12:17

jkczyz reviewed Sep 4, 2024

View reviewed changes

shaavan force-pushed the blinded_api branch from 0d9ac2a to cbaf88a Compare September 4, 2024 11:05

TheBlueMatt reviewed Sep 5, 2024

View reviewed changes

lightning/src/onion_message/messenger.rs Outdated Show resolved Hide resolved

shaavan force-pushed the blinded_api branch from cbaf88a to 7a18051 Compare September 5, 2024 20:37

shaavan force-pushed the blinded_api branch from 7a18051 to bc00884 Compare September 19, 2024 15:40

shaavan force-pushed the blinded_api branch from bc00884 to 3bcc420 Compare September 19, 2024 16:03

shaavan changed the title ~~Introduce BlindedPathParams~~ Introduce BlindedPathType enum Sep 24, 2024

shaavan force-pushed the blinded_api branch from 3bcc420 to d6dde1e Compare October 3, 2024 12:30

jkczyz reviewed Oct 4, 2024

View reviewed changes

shaavan force-pushed the blinded_api branch from d6dde1e to 6817512 Compare October 11, 2024 14:28

jkczyz reviewed Oct 11, 2024

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

shaavan force-pushed the blinded_api branch from 6817512 to fe33775 Compare October 19, 2024 11:57

shaavan force-pushed the blinded_api branch from fe33775 to 0db4aa9 Compare October 19, 2024 12:29

shaavan force-pushed the blinded_api branch from 0db4aa9 to 0728e14 Compare October 19, 2024 12:39

TheBlueMatt reviewed Oct 19, 2024

View reviewed changes

shaavan changed the title ~~Introduce BlindedPathType enum~~ Enable Creation of Offers and Refunds Without Blinded Path Oct 24, 2024

shaavan added 6 commits October 24, 2024 17:46

Allow Explicit Specification of Blinded Path Type in Refund

e297444

Similar to the offer case, this commit introduces the ability to explicitly specify the Blinded Path type.

Update DefaultMessageRouter's implmentation

ffb13c3

Introduce Tests for Offers and Refunds Without Blinded Paths

863dcd6

shaavan force-pushed the blinded_api branch from 0728e14 to 863dcd6 Compare October 24, 2024 12:17

Enable Creation of Offers and Refunds Without Blinded Path #3246

Are you sure you want to change the base?

Enable Creation of Offers and Refunds Without Blinded Path #3246

Conversation

shaavan commented Aug 16, 2024 • edited Loading

PR Description:

The Key Changes:

Important Notes:

codecov bot commented Aug 16, 2024 • edited Loading

Codecov Report

shaavan commented Aug 19, 2024

shaavan commented Aug 19, 2024

shaavan commented Aug 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaavan commented Sep 4, 2024

shaavan commented Sep 5, 2024

shaavan commented Sep 19, 2024

shaavan commented Sep 19, 2024

shaavan commented Oct 3, 2024

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

shaavan Oct 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaavan commented Oct 11, 2024

shaavan commented Oct 19, 2024 • edited Loading

shaavan commented Oct 19, 2024

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

shaavan commented Oct 24, 2024

shaavan commented Aug 16, 2024 •

edited

Loading

codecov bot commented Aug 16, 2024 •

edited

Loading

shaavan Oct 19, 2024 •

edited

Loading

shaavan commented Oct 19, 2024 •

edited

Loading