Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable Creation of Offers and Refunds Without Blinded Path #3246

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Enable Creation of Offers and Refunds Without Blinded Path #3246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b9645d8
Introduce `BlindedPathType`
shaavan Sep 18, 2024
fab8816
Allow explicit specification of Blinded Path type in `create_offer_bu…
shaavan Sep 18, 2024
e297444
Allow Explicit Specification of Blinded Path Type in Refund
shaavan Sep 18, 2024
ffb13c3
Update DefaultMessageRouter's implmentation
shaavan Oct 19, 2024
44e9789
Use all the paths returned by MessageRouter
shaavan Oct 19, 2024
863dcd6
Introduce Tests for Offers and Refunds Without Blinded Paths
shaavan Oct 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 67 additions & 43 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,34 @@ pub enum PendingHTLCRouting {
},
}

/// Represents the types of [`BlindedMessagePath`] that can be created.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

pub enum BlindedPathType {
/// A compact version of [`BlindedMessagePath`].
///
/// Compact blinded paths use a [`ShortChannelId`] instead of a [`NodeId`] to represent forward nodes.
/// This approach drastically reduces the size of each individual forward packet but requires
/// knowledge of the local channel graph to find the corresponding channel for each node.
///
/// Compact blinded paths are especially useful when size is a constraint, such as when offers
/// and refund information must be represented in QR code form.
///
/// [`ShortChannelId`]: crate::blinded_path::message::NextMessageHop::ShortChannelId
/// [`NodeId`]: crate::blinded_path::message::NextMessageHop::NodeId
Compact,

/// A full-length version of [`BlindedMessagePath`].
///
/// Unlike compact blinded paths, full-length paths use each individual forward node's [`NodeId`] for representation.
/// This increases the size of each forward packet as more space is required for representation.
/// However, it does not require knowledge of the local channel graph to create the path.
///
/// Full-length blinded paths are useful when onion messages are communicated over the wire and size constraints are not an issue,
/// such as when sending a response to an onion message.
///
/// [`NodeId`]: crate::blinded_path::message::NextMessageHop::NodeId
Full,
}

/// Information used to forward or fail this HTLC that is being forwarded within a blinded path.
#[derive(Clone, Copy, Debug, Hash, PartialEq, Eq)]
pub struct BlindedForward {
Expand Down Expand Up @@ -1852,14 +1880,14 @@ where
///
/// ```
/// # use lightning::events::{Event, EventsProvider, PaymentPurpose};
/// # use lightning::ln::channelmanager::AChannelManager;
/// # use lightning::ln::channelmanager::{AChannelManager, BlindedPathType};
/// # use lightning::offers::parse::Bolt12SemanticError;
/// #
/// # fn example<T: AChannelManager>(channel_manager: T) -> Result<(), Bolt12SemanticError> {
/// # let channel_manager = channel_manager.get_cm();
/// # let absolute_expiry = None;
/// # let blinded_path = BlindedPathType::Full;
/// let offer = channel_manager
/// .create_offer_builder(absolute_expiry)?
/// .create_offer_builder(Some(blinded_path))?
/// # ;
/// # // Needed for compiling for c_bindings
/// # let builder: lightning::offers::offer::OfferBuilder<_, _> = offer.into();
Expand Down Expand Up @@ -1955,7 +1983,7 @@ where
/// ```
/// # use core::time::Duration;
/// # use lightning::events::{Event, EventsProvider};
/// # use lightning::ln::channelmanager::{AChannelManager, PaymentId, RecentPaymentDetails, Retry};
/// # use lightning::ln::channelmanager::{AChannelManager, BlindedPathType, PaymentId, RecentPaymentDetails, Retry};
/// # use lightning::offers::parse::Bolt12SemanticError;
/// #
/// # fn example<T: AChannelManager>(
Expand All @@ -1964,9 +1992,10 @@ where
/// # ) -> Result<(), Bolt12SemanticError> {
/// # let channel_manager = channel_manager.get_cm();
/// let payment_id = PaymentId([42; 32]);
/// let blinded_path = Some(BlindedPathType::Full);
/// let refund = channel_manager
/// .create_refund_builder(
/// amount_msats, absolute_expiry, payment_id, retry, max_total_routing_fee_msat
/// amount_msats, absolute_expiry, payment_id, retry, max_total_routing_fee_msat, blinded_path
/// )?
/// # ;
/// # // Needed for compiling for c_bindings
Expand Down Expand Up @@ -9121,7 +9150,7 @@ macro_rules! create_offer_builder { ($self: ident, $builder: ty) => {
/// [`Offer`]: crate::offers::offer::Offer
/// [`InvoiceRequest`]: crate::offers::invoice_request::InvoiceRequest
pub fn create_offer_builder(
&$self, absolute_expiry: Option<Duration>
&$self, blinded_path: Option<BlindedPathType>,
) -> Result<$builder, Bolt12SemanticError> {
let node_id = $self.get_our_node_id();
let expanded_key = &$self.inbound_payment_key;
Expand All @@ -9130,17 +9159,21 @@ macro_rules! create_offer_builder { ($self: ident, $builder: ty) => {

let nonce = Nonce::from_entropy_source(entropy);
let context = OffersContext::InvoiceRequest { nonce };
let path = $self.create_blinded_paths_using_absolute_expiry(context, absolute_expiry)
.and_then(|paths| paths.into_iter().next().ok_or(()))

let mut builder = OfferBuilder::deriving_signing_pubkey(node_id, expanded_key, nonce, secp_ctx)
.chain_hash($self.chain_hash);

if let Some(path_type) = blinded_path {
let paths = match path_type {
BlindedPathType::Compact => $self.create_compact_blinded_paths(context),
BlindedPathType::Full => $self.create_blinded_paths(MessageContext::Offers(context)),
}
.map_err(|_| Bolt12SemanticError::MissingPaths)?;
let builder = OfferBuilder::deriving_signing_pubkey(node_id, expanded_key, nonce, secp_ctx)
.chain_hash($self.chain_hash)
.path(path);

let builder = match absolute_expiry {
None => builder,
Some(absolute_expiry) => builder.absolute_expiry(absolute_expiry),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
	...
}
  1. Passing None supports creating an offer without a blinded path.
  2. Passing WhyWeWantABlindedPath::ForOffer { offer_expiry_time, is_size_constraint: true, ... } covers the offer for QR codes.
  3. 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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suppose that OffersMessageFlow was parameterized by some ResponseStrategy trait (for lack of a better name), which the user must either implement on their own or use a DefaultResponseStrategy provided by LDK. The trait could be responsible not only for whether a response should be sent (e.g., if the InvoiceRequest's msat-denominated amount is sufficient for a fiat-denominated offer) but also for how to construct the reply path (i.e., by returning a specific MessageRouter as defined by an associated type on the trait).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validating the amount makes sense to me, but this is really what I want to get away from in the router - if we give the MessageRouter enough context, we don't need to worry about making sure the user can override the router in cases where they want to, which I'd very much prefer over building a new trait whose purpose is (in part) allowing the user to override a trait they already provided.

Can we just straight up give the MessageRouter the Offer we're replying to?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I'm not convinced we need to do what I suggested. Reply path construction doesn't really need much information outside what we'd already provide the MessageRouter with the MessageContext. I was mostly thinking out loud about how we might remove the MessageRouter trait entirely from OffersMessageFlow. For creation / initiation uses, the user would provide it directly to whichever method. For the handler, it would need to come from the parameterized ResponseStrategy. But that simply moves the parameterization from OffersMessageFlow to an associated type on ResponseStrategy. So probably doesn't really gain us much.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just straight up give the MessageRouter the Offer we're replying to?

MessageContext is probably better as it would include any new uses.

};
builder = paths.into_iter().fold(builder, |builder, path| {
builder.path(path)
});
}

Ok(builder.into())
}
Expand Down Expand Up @@ -9194,7 +9227,7 @@ macro_rules! create_refund_builder { ($self: ident, $builder: ty) => {
/// [Avoiding Duplicate Payments]: #avoiding-duplicate-payments
pub fn create_refund_builder(
&$self, amount_msats: u64, absolute_expiry: Duration, payment_id: PaymentId,
retry_strategy: Retry, max_total_routing_fee_msat: Option<u64>
retry_strategy: Retry, max_total_routing_fee_msat: Option<u64>, blinded_path: Option<BlindedPathType>
) -> Result<$builder, Bolt12SemanticError> {
let node_id = $self.get_our_node_id();
let expanded_key = &$self.inbound_payment_key;
Expand All @@ -9203,16 +9236,25 @@ macro_rules! create_refund_builder { ($self: ident, $builder: ty) => {

let nonce = Nonce::from_entropy_source(entropy);
let context = OffersContext::OutboundPayment { payment_id, nonce, hmac: None };
let path = $self.create_blinded_paths_using_absolute_expiry(context, Some(absolute_expiry))
.and_then(|paths| paths.into_iter().next().ok_or(()))
.map_err(|_| Bolt12SemanticError::MissingPaths)?;

let builder = RefundBuilder::deriving_signing_pubkey(
node_id, expanded_key, nonce, secp_ctx, amount_msats, payment_id
let mut builder = RefundBuilder::deriving_signing_pubkey(
node_id, expanded_key, nonce, secp_ctx,
amount_msats, payment_id,
)?
.chain_hash($self.chain_hash)
.absolute_expiry(absolute_expiry)
.path(path);
.chain_hash($self.chain_hash)
.absolute_expiry(absolute_expiry);

if let Some(path_type) = blinded_path {
let paths = match path_type {
BlindedPathType::Compact => $self.create_compact_blinded_paths(context),
BlindedPathType::Full => $self.create_blinded_paths(MessageContext::Offers(context)),
}
.map_err(|_| Bolt12SemanticError::MissingPaths)?;

builder = paths.into_iter().fold(builder, |builder, path| {
builder.path(path)
})
}

let _persistence_guard = PersistenceNotifierGuard::notify_on_drop($self);

Expand Down Expand Up @@ -9596,25 +9638,7 @@ where
inbound_payment::get_payment_preimage(payment_hash, payment_secret, &self.inbound_payment_key)
}

/// Creates a collection of blinded paths by delegating to [`MessageRouter`] based on
/// the path's intended lifetime.
///
/// Whether or not the path is compact depends on whether the path is short-lived or long-lived,
/// respectively, based on the given `absolute_expiry` as seconds since the Unix epoch. See
/// [`MAX_SHORT_LIVED_RELATIVE_EXPIRY`].
fn create_blinded_paths_using_absolute_expiry(
&self, context: OffersContext, absolute_expiry: Option<Duration>,
) -> Result<Vec<BlindedMessagePath>, ()> {
let now = self.duration_since_epoch();
let max_short_lived_absolute_expiry = now.saturating_add(MAX_SHORT_LIVED_RELATIVE_EXPIRY);

if absolute_expiry.unwrap_or(Duration::MAX) <= max_short_lived_absolute_expiry {
self.create_compact_blinded_paths(context)
} else {
self.create_blinded_paths(MessageContext::Offers(context))
}
}

#[cfg(test)]
pub(super) fn duration_since_epoch(&self) -> Duration {
#[cfg(not(feature = "std"))]
let now = Duration::from_secs(
Expand Down
Loading
Loading