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.

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

Multi-hop BlindedPath creation interface #2781

Merged
merged 14 commits into from
Dec 15, 2023

Conversation

jkczyz
Copy link
Contributor

@jkczyz jkczyz commented Dec 8, 2023

Expands Router and MessageRouter traits with methods for constructing BlindedPaths for payment and onion messaging, respectively. Also makes MessageRouter a super trait of Router so that ChannelManager doesn't need another parameter.

ChannelManager uses the new interface in its Offers flow to construct:

  • paths for Offers and Refunds,
  • reply paths for InvoiceRequest and Bolt12Invoice, and
  • payment paths for Bolt12Invoice.

DefaultMessageRouter and DefaultRouter implement the new methods by constructing three-hop blinded paths (i.e., where the counterparty of the peer is the introduction node).

Resolves #2690

@jkczyz
Copy link
Contributor Author

jkczyz commented Dec 8, 2023

@TheBlueMatt @valentinewallace Looking for high-level feedback on the new interface and how it is used by ChannelManager. I didn't implement the new methods for DefaultRouter or DefaultMessageRouter just yet. Instead, I simply provided trait-level defaults returning the naive two-hop blinded paths.

@codecov-commenter
Copy link

codecov-commenter commented Dec 9, 2023

Codecov Report

Attention: 231 lines in your changes are missing coverage. Please review.

Comparison is base (c92db69) 88.59% compared to head (37319a6) 88.46%.

Files Patch % Lines
lightning/src/ln/channelmanager.rs 0.00% 80 Missing ⚠️
lightning/src/routing/router.rs 2.46% 79 Missing ⚠️
lightning/src/onion_message/messenger.rs 0.00% 34 Missing ⚠️
lightning/src/util/test_utils.rs 4.54% 21 Missing ⚠️
lightning/src/onion_message/functional_tests.rs 0.00% 8 Missing ⚠️
lightning/src/blinded_path/payment.rs 0.00% 6 Missing ⚠️
lightning/src/ln/features.rs 57.14% 3 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2781      +/-   ##
==========================================
- Coverage   88.59%   88.46%   -0.14%     
==========================================
  Files         114      114              
  Lines       91561    91718     +157     
  Branches    91561    91718     +157     
==========================================
+ Hits        81120    81138      +18     
- Misses       7962     8103     +141     
+ Partials     2479     2477       -2     

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

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

The API is fine, I think. A few minor comments.

@@ -1068,7 +1068,7 @@ mod tests {
// Check that the flags are as expected:
// - option_data_loss_protect (req)
// - var_onion_optin (req) | static_remote_key (req) | payment_secret(req)
// - basic_mpp | wumbo | anchors_zero_fee_htlc_tx
// - basic_mpp | wumbo | option_anchors_zero_fee_htlc_tx
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: drop the the^H^H^Hoption, its cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly was aiming for internal consistency here and with BOLT 9, though I get why we don't use option in the identifier names.

fn create_blinded_paths<
ES: EntropySource + ?Sized, T: secp256k1::Signing + secp256k1::Verification
>(
&self, recipient: PublicKey, peers: Vec<PublicKey>, count: usize, entropy_source: &ES,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the count need to be an argument here, or can the selected hop count just be up to the implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I didn't spell it out but count is the number of BlindedPaths, not the number of hops which is left up to the implementation. Currently, we only use one path, though someone may want to add more paths to an Offer or Refund. We also use the same reply path when sending multiple InvoiceRequests, but could ask for more and cycle through whatever the MessageRouter returns.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt Dec 11, 2023

Choose a reason for hiding this comment

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

Oh, sorry, I wrote that comment when I was confused, but figured it out later in the patchset. Still, I'm not sure we need count specified in either case - shouldn't we leave it up to the trait impl how many paths to return? We'll need to give them enough info to figure out what we're using it for, but I think that's true already just looking at it.

Copy link
Contributor Author

@jkczyz jkczyz Dec 11, 2023

Choose a reason for hiding this comment

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

Router::create_blinded_payment_paths is used for Invoice paths, so presumably the implementation could decide, yeah.

Message::create_blinded_paths as of now just needs one as mentioned above. If we allow implementations to decide, are you saying if an implementation returns more than one then we should set all of them on an Offer or Refund (where QR code size may be a factor)? For reply paths, would we simply use an arbitrary (first?) one? Or even cycle through them in case of sending multiple InvoiceRequests?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yea, I mean I don't have a concrete answer, it just seems like if we have an interface for it it'd be nice if we let the implementor decide (cause sometimes they're not going in a QR code, even offers). Maybe we set some enum/flag to say what its for/if a qr code may matter?

lightning/src/onion_message/messenger.rs Outdated Show resolved Hide resolved
&self, recipient: PublicKey, first_hops: Vec<ChannelDetails>, tlvs: ReceiveTlvs,
amount_msats: u64, count: usize, entropy_source: &ES, secp_ctx: &Secp256k1<T>
) -> Result<Vec<(BlindedPayInfo, BlindedPath)>, ()> {
first_hops.into_iter()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, we probably need to check if the peer has at least one (or maybe three) public channel(s) before we select it as an introduction point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would require a NetworkGraph, which means we can't provide a default (i.e., provided) implementation. That's fine, but would mean anyone with their own Rotuer implementation now needs to implement this method. Unless we just have the provided one return an empty Vec, allowing ChannelManager to fallback to one-hop paths? So I guess you'd still expose your node id -- and counterparties for anyone looking at the NetworkGraph.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt Dec 11, 2023

Choose a reason for hiding this comment

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

Yea, I think that's okay to not have a default impl. DefaultRouter kinda is that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, just noting others will need to implement this if they use their own Router implementation, possibly just wrapping DefaultRouter assuming they have a NetworkGraph on hand.

@TheBlueMatt TheBlueMatt added this to the 0.0.119 milestone Dec 11, 2023
@TheBlueMatt
Copy link
Collaborator

Tentatively giving this an 0.0.119 tag to indicate to reviewers to focus on it, but I kinda doubt we'll get it over the line in two days.

Copy link
Contributor Author

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

PTAL. I think expanding the number of hops by one shouldn't be too difficult.

&[forward_node], recipient, tlvs.clone(), u64::MAX, entropy_source, secp_ctx
)
})
.take(MAX_PAYMENT_PATHS)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Chose to limit to three paths in the implementation for DefaultRouter. I think we want some limit to avoid computing too many BlindedPaths.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wondering if should increase this substantially to avoid failed payments. Though, maybe the best strategy is to sort ascending all possible paths by scorer penalty and take some N.

lightning/src/onion_message/messenger.rs Show resolved Hide resolved
@@ -1068,7 +1068,7 @@ mod tests {
// Check that the flags are as expected:
// - option_data_loss_protect (req)
// - var_onion_optin (req) | static_remote_key (req) | payment_secret(req)
// - basic_mpp | wumbo | anchors_zero_fee_htlc_tx
// - basic_mpp | wumbo | option_anchors_zero_fee_htlc_tx
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly was aiming for internal consistency here and with BOLT 9, though I get why we don't use option in the identifier names.

lightning/src/routing/router.rs Show resolved Hide resolved
.filter(|details| amount_msats >= details.inbound_htlc_minimum_msat.unwrap_or(0))
.filter(|details| amount_msats <= details.inbound_htlc_maximum_msat.unwrap_or(0))
.map(|details| {
let short_channel_id = details.get_inbound_payment_scid().unwrap();
Copy link
Contributor

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 unwrap here and below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIUC, a usable channel will always have these set. I could add debug assertions or additional filters to be safe, if you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, while unlikely this panic is reachable from the public interface so I think we should filter.

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
@@ -78,10 +83,80 @@ impl< G: Deref<Target = NetworkGraph<L>>, L: Deref, S: Deref, SP: Sized, Sc: Sco
&random_seed_bytes
)
}

fn create_blinded_payment_paths<
Copy link
Contributor

Choose a reason for hiding this comment

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

Given we have the amount, we probably want to use something more like lightning-invoice's sort_and_filter_channels for this method, at least eventually. Currently the sum of inbound capacity of the paths we're returning may not be sufficient, IIUC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given we have the amount, we probably want to use something more like lightning-invoice's sort_and_filter_channels for this method, at least eventually.

Hmm... perhaps eventually. Unlike in lightning-invoice, these are meant to be used immediately, so the channels should be live at very least when going to pay. We may want to prefer channels based on their available liquidity, which would require a collect and sort. I was trying to avoid this given this action can be initiated by anyone sending an onion message.

Currently the sum of inbound capacity of the paths we're returning may not be sufficient, IIUC.

Added the check against inbound capacity for now as that was indeed missing. Currently, requiring that every channel has enough. I suppose if this constraint cannot be met we could relax it to a fraction of the amount based on MAX_PAYMENT_PATHS?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was trying to avoid this given this action can be initiated by anyone sending an onion message.

The action can be initiated by anyone only if this node has a public offer, is that right?

Another thing to note is that if we include one of our channels that is public but doesn't have enough confirmations to be in the public network graph, we'll be revealing to our counterparty that we are the likely destination for the payment. This is something that sort_and_filter_channels takes care of, and based on the docs for the method it also takes other considerations into account that seem relevant here. This may not be a big concern for v1, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The action can be initiated by anyone only if this node has a public offer, is that right?

Yeah, true.

Another thing to note is that if we include one of our channels that is public but doesn't have enough confirmations to be in the public network graph, we'll be revealing to our counterparty that we are the likely destination for the payment.

Does list_usable_channels mostly avoid this? I believe sort_and_filter_channels doesn't assume all channels passed are usable. Wouldn't our counterparty know the payment is likely for us if the channel is unannounced as well?

This is something that sort_and_filter_channels takes care of, and based on the docs for the method it also takes other considerations into account that seem relevant here. This may not be a big concern for v1, though.

Hmm... yeah there is some overlap there, but I think the scenarios are also different since the introduction nodes aren't our counterparties here whereas for sort_and_filter_channels the hints are. So the filtering and selection criteria aren't necessarily going to be the same as there.

Also, as mentioned elsewhere, we probably need to do some sorting after all so the payer knows which paths to prefer (e.g., ones where we have higher confidence that the hop from the introduction node has enough liquidity).

Copy link
Contributor

Choose a reason for hiding this comment

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

Does list_usable_channels mostly avoid this? I believe sort_and_filter_channels doesn't assume all channels passed are usable.

I believe list_usable_channels just checks that the channel is funded, not how many confirmations it has.

Wouldn't our counterparty know the payment is likely for us if the channel is unannounced as well?

Yeah, so if we have public and private channels we should prefer the public ones in blinded payment paths we return, IIUC.

So the filtering and selection criteria aren't necessarily going to be the same as there.

With you there, just saying we'll probably want to see which of its checks are relevant for us when we add sorting.

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 believe list_usable_channels just checks that the channel is funded, not how many confirmations it has.

AFAICT, it goes it is consider usable when both parties sent channel_ready, which happens once the funding transaction has enough confirmations.

Yeah, so if we have public and private channels we should prefer the public ones in blinded payment paths we return, IIUC.

I'm not sure I understand why. They are blinded from the payer, so the concern we have with invoice hints doesn't apply. Maybe our counterparty learns something if we have both public and private channels with them and the payer uses MPP over both? Let me know if I'm missing something.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe our counterparty learns something if we have both public and private channels with them and the payer uses MPP over both? Let me know if I'm missing something.

I was thinking that the counterparty can be fairly confident that we're the recipient if a private channel is used, but if public channels are used then we have more deniability. Privacy concerns aren't too high priority rn though, as discussed offline.

Copy link
Contributor Author

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

Added another hop to the blinded payment paths.

@@ -78,10 +83,80 @@ impl< G: Deref<Target = NetworkGraph<L>>, L: Deref, S: Deref, SP: Sized, Sc: Sco
&random_seed_bytes
)
}

fn create_blinded_payment_paths<
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given we have the amount, we probably want to use something more like lightning-invoice's sort_and_filter_channels for this method, at least eventually.

Hmm... perhaps eventually. Unlike in lightning-invoice, these are meant to be used immediately, so the channels should be live at very least when going to pay. We may want to prefer channels based on their available liquidity, which would require a collect and sort. I was trying to avoid this given this action can be initiated by anyone sending an onion message.

Currently the sum of inbound capacity of the paths we're returning may not be sufficient, IIUC.

Added the check against inbound capacity for now as that was indeed missing. Currently, requiring that every channel has enough. I suppose if this constraint cannot be met we could relax it to a fraction of the amount based on MAX_PAYMENT_PATHS?

.filter(|details| amount_msats >= details.inbound_htlc_minimum_msat.unwrap_or(0))
.filter(|details| amount_msats <= details.inbound_htlc_maximum_msat.unwrap_or(0))
.map(|details| {
let short_channel_id = details.get_inbound_payment_scid().unwrap();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIUC, a usable channel will always have these set. I could add debug assertions or additional filters to be safe, if you prefer.

@jkczyz jkczyz force-pushed the 2023-09-multihop-paths branch 2 times, most recently from 0e33921 to 2b8170b Compare December 13, 2023 02:57
@jkczyz jkczyz marked this pull request as ready for review December 13, 2023 02:57
>(
&self, recipient: PublicKey, first_hops: Vec<ChannelDetails>, tlvs: ReceiveTlvs,
amount_msats: u64, entropy_source: &ES, secp_ctx: &Secp256k1<T>
) -> Result<Vec<(BlindedPayInfo, BlindedPath)>, ()> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you planning to add some testing for this method/others? I guess e2e tests are also an option now that #2688 landed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I need to do that but haven't decided on exactly how given the returned paths are blinded. Would e2e test be possible if we can't forward on the blinded path? Otherwise, might be better to refactor out the filtering / sorting logic and test that independently.

Copy link
Contributor

Choose a reason for hiding this comment

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

e2e would be possible with 2-hop blinded payment paths but not 3 :/

}
};
BlindedPath::new_for_payment(
&[introduction_forward_node, counterparty_forward_node], recipient,
Copy link
Contributor

@valentinewallace valentinewallace Dec 13, 2023

Choose a reason for hiding this comment

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

It seems potentially likely for users with some LDKServer<>LDKClient architecture that the counterparty will support route blinding, but none of the counterparty's public counterparties do. I don't think we should fail to construct a path in that situation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How does the architecture play a role here? The same could be said for a standard one, no? In that case, we fallback to the one-hop path. Could do two-hop fallback before that?

Copy link
Contributor

Choose a reason for hiding this comment

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

How does the architecture play a role here?

The architecture/situation outlined seems like a somewhat common LDK use case where 3-hop wouldn't work, and 1-hop also wouldn't work (due to the client likely being private).

To me it would make more sense if we always tried for 2-hop in this method, and fall back to 1-hop if the recipient is a public node, otherwise error. Then we wouldn't need to have 1-hop fallback logic in channelmanager as well. I may be missing something here, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How does the architecture play a role here?

The architecture/situation outlined seems like a somewhat common LDK use case where 3-hop wouldn't work, and 1-hop also wouldn't work (due to the client likely being private).

Still a little confused about this. I thought you were talking about splitting an LDK node. But are you saying LDKServer is an LSP node and LSPClient is a separate node with a private channel with the LSP?

To me it would make more sense if we always tried for 2-hop in this method, and fall back to 1-hop if the recipient is a public node, otherwise error. Then we wouldn't need to have 1-hop fallback logic in channelmanager as well. I may be missing something here, though.

When would we try 3-hops? Are you thinking something like this?

let paths = if has_public_channels {
    create_three_hop_paths()
} else {
    create_two_hop_paths()
};

if !paths.is_empty() {
    paths
} else if has_public_channels {
    create_one_hop_path()
} else {
    Err(())
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Still a little confused about this. I thought you were talking about splitting an LDK node. But are you saying LDKServer is an LSP node and LSPClient is a separate node with a private channel with the LSP?

That was my understanding - an LSP and a client where the LSP has no route-blinding-supporting peers, but a client that wants a blinded path (through the LSP, cause they have no public channels).

We should still presumably try 3 hop in that case, but if we fail (cause the LSP has no blinding-supporting peers) we should build a 2 hop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -78,10 +83,80 @@ impl< G: Deref<Target = NetworkGraph<L>>, L: Deref, S: Deref, SP: Sized, Sc: Sco
&random_seed_bytes
)
}

fn create_blinded_payment_paths<
Copy link
Contributor

Choose a reason for hiding this comment

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

Does list_usable_channels mostly avoid this? I believe sort_and_filter_channels doesn't assume all channels passed are usable.

I believe list_usable_channels just checks that the channel is funded, not how many confirmations it has.

Wouldn't our counterparty know the payment is likely for us if the channel is unannounced as well?

Yeah, so if we have public and private channels we should prefer the public ones in blinded payment paths we return, IIUC.

So the filtering and selection criteria aren't necessarily going to be the same as there.

With you there, just saying we'll probably want to see which of its checks are relevant for us when we add sorting.

)
})
.take(MAX_PAYMENT_PATHS)
.collect()
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 this should error if no paths are found?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We treat these the same in ChannelManager by falling back to 1-hop paths. Not sure if we should move that code here, but failing when we don't have public channels means we can't send an invoice, FWIW.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess to me it seems like a feature that we'll fail to send an invoice if we're a private node and none of our channel peers support route blinding, because the payer would most likely have no way to find a route to our 1-hop blinded path anyway. Also nice to keep the fallback code out of channelmanager when it seems more like the domain of the router. Not sure it's worth holding up the PR over.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved fallback code to the respective default implementations, FYI.

@TheBlueMatt
Copy link
Collaborator

i kinda wonder if we shouldn't max out at 2 hops for now. Its not great for privacy, but until we use scoring data in our blinding selection logic i fear we're just gonna always fail to receive, which is just gonna kill B12 adoption until we fix it.

@jkczyz
Copy link
Contributor Author

jkczyz commented Dec 14, 2023

i kinda wonder if we shouldn't max out at 2 hops for now. Its not great for privacy, but until we use scoring data in our blinding selection logic i fear we're just gonna always fail to receive, which is just gonna kill B12 adoption until we fix it.

Discussed offline. We'll use 2-hop paths for now and do 3-hop paths in another PR. Moved relevant commits to #2793 and squashed.

}

/// Creates a one-hop blinded payment path with [`ChannelManager::get_our_node_id`] as the
/// introduction node.
fn create_one_hop_blinded_payment_path(
Copy link
Collaborator

Choose a reason for hiding this comment

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

We currently dont fail create_offer_builder if we have no public channels and fail to build a multi-hop path, but ISTM this (and create_one_hop_blinded_path) should fail if we don't have any public channels.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So now that this is moved to Router (and MessageRouter, respectively), we can check there but only in Router. In MessageRouter, we don't have channel information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, actually, we can just use the NetworkGraph to see if our node was announced.

lightning/src/routing/router.rs Outdated Show resolved Hide resolved
let payment_relay: PaymentRelay = details.counterparty.forwarding_info.unwrap().into();
let payment_constraints = PaymentConstraints {
max_cltv_expiry: tlvs.payment_constraints.max_cltv_expiry
+ payment_relay.cltv_expiry_delta as u32,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to use some kind of max here to not expose esoteric CLTV expiry deltas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Let me know if that is what you were thinking.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

LGTM, with one nit about needing docs.

lightning/src/ln/channelmanager.rs Show resolved Hide resolved
None => return None,
};
let payment_relay: PaymentRelay = match details.counterparty.forwarding_info {
Some(forwarding_info) => forwarding_info.into(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops, also the fees here and the CLTV here need normalizing. Maybe this doesn't belong in the router but in new_for_payment? I'm fine leaving this for a followup in 120 if you prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will leave to a follow-up.

@@ -99,7 +99,6 @@ impl<G: Deref<Target = NetworkGraph<L>> + Clone, L: Deref, S: Deref, SP: Sized,

let network_graph = self.network_graph.deref().read_only();
let paths = first_hops.into_iter()
.filter(|details| details.is_public)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed to drop this since we want two-hop paths that contain an unannounced channel between the intro node and us.

@jkczyz
Copy link
Contributor Author

jkczyz commented Dec 15, 2023

I wrote a test in #2697, but I'm getting the following failure when making the payment (debugging added):

[lightning/src/ln/onion_payment.rs:46] inbound_amt_msat = 10001000
[lightning/src/ln/onion_payment.rs:46] payment_constraints.htlc_minimum_msat = 1000
[lightning/src/ln/onion_payment.rs:46] dbg!(inbound_amt_msat) < dbg!(payment_constraints.htlc_minimum_msat) = false
[lightning/src/ln/onion_payment.rs:47] outgoing_cltv_value = 259
[lightning/src/ln/onion_payment.rs:47] payment_constraints.max_cltv_expiry = 93
[lightning/src/ln/onion_payment.rs:47] dbg!(outgoing_cltv_value) > dbg!(payment_constraints.max_cltv_expiry) = true
node 1 INFO [lightning::ln::onion_payment:393]         Failed to accept/forward incoming HTLC: Underflow calculating outbound amount or cltv value for blinded forward

So the CLTV check is causing the failure: https://github.com/jkczyz/rust-lightning/blob/181e621d26f9454c1e4726b7a923b9c8e26addf2/lightning/src/ln/onion_payment.rs#L47

Can be reproduced on that PR using:

cargo test -p lightning -- creates_and_pays_for_offer_using_two_hop_blinded_path

@valentinewallace Do you see anything wrong in the test setup or how I'm calculating the payment constraints? Note that I haven't rebased on your current work yet, so maybe that is it?

let entropy_source = self.entropy_source.deref();
let secp_ctx = &self.secp_ctx;

let first_hops = self.list_usable_channels();
let payee_node_id = self.get_our_node_id();
let max_cltv_expiry = self.best_block.read().unwrap().height() + LATENCY_GRACE_PERIOD_BLOCKS;
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this doesn't allow for the router adding the shadow offset to the CLTV during route construction. Will probably want to add MAX_SHADOW_CLTV_EXPIRY_DELTA_OFFSET (from router.rs) to this value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... but the BlindedPath is created by the recipient, and the Route is created by the payer. Is that value common across implementations? Or just something we expect to be good enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

True. I think it will be good enough, but this does mean that our router should be taking max_cltv_expiry into account and currently isn't :/

Copy link
Contributor

@valentinewallace valentinewallace Dec 15, 2023

Choose a reason for hiding this comment

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

Looks like this doesn't allow for the router adding the shadow offset to the CLTV during route construction. Will probably want to add MAX_SHADOW_CLTV_EXPIRY_DELTA_OFFSET (from router.rs) to this value.

Thinking more, currently the way we set max_cltv_expiry here doesn't allow for any CLTV delta along the route, not just the shadow offset. So I think the value we set here should increase even further, actually.
Never mind, nodes subtract their delta at each payment hop...

Copy link
Contributor

Choose a reason for hiding this comment

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

True. I think it will be good enough, but this does mean that our router should be taking max_cltv_expiry into account and currently isn't :/

Ok took me a minute to reload all the context here. max_cltv_expiry isn't part of BlindedPayInfo so there's no way to take it into account when pathfinding. This confused me originally so I opened lightning/bolts#1069 to clarify it. But I don't think we should stop using the shadow offset in pathfinding just because invoice expiries tend to be short, so I'm not exactly sure what the resolution is here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed offline to add CLTV_FAR_FAR_AWAY.

Move AnchorsZeroFeeHtlcTx after Wumbo to keep order by feature bit.
Also, update setting order and comment in tests.
The RouteBlinding feature flag is signals support for relaying payments
over blinded paths. It is used for paying BOLT 12 invoices, which are
required to included at least one blinded path.
The MessageRouter trait is used to find an OnionMessagePath to a
Destination (e.g., to a BlindedPath). Expand the interface with a
create_blinded_paths method for creating such paths to a recipient.
Provide a default implementation creating two-hop blinded paths where
the recipient's peers serve as introduction nodes.
ChannelManager is parameterized by a Router in order to find routes when
sending and retrying payments. For the offers flow, it needs to be able
to construct blinded paths (e.g., in the offer and in reply paths).
Instead of adding yet another parameter to ChannelManager, require that
any Router also implements MessageRouter. Implement this for
DefaultRouter by delegating to a DefaultMessageRouter.
let entropy_source = self.entropy_source.deref();
let secp_ctx = &self.secp_ctx;

let first_hops = self.list_usable_channels();
let payee_node_id = self.get_our_node_id();
let max_cltv_expiry = self.best_block.read().unwrap().height() + LATENCY_GRACE_PERIOD_BLOCKS;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should also account for the invoice expiry, IIUC. We default to 60 minutes right now, so I think we should add 6 blocks on for that (assuming 10 minute blocks), per lightning/bolts#1069.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed offline to add CLTV_FAR_FAR_AWAY.

When finding a route through a blinded path, a random CLTV offset may be
added to the path in order to preserve privacy. This needs to be
accounted for in the blinded path's PaymentConstraints. Add
CLTV_FAR_FAR_AWAY to the max_cltv_expiry constraint to allow for such
offsets.
When constructing blinded paths for Offer and Refund, delegate to
MessageRouter::create_blinded_paths which may produce multi-hop blinded
paths. Fallback to one-hop blinded paths if the MessageRouter fails or
returns no paths.

Likewise, do the same for InvoiceRequest and Bolt12Invoice reply paths.
The Router trait is used to find a Route for paying a node. Expand the
interface with a create_blinded_payment paths method for creating such
paths to a recipient node.

Provide an implementation for DefaultRouter that creates two-hop
blinded paths where the recipient's peers serve as the introduction
nodes.
When constructing blinded payment paths for Bolt12Invoice, delegate to
Router::create_blinded_payment_paths which may produce multi-hop blinded
paths. Fallback to one-hop blinded paths if the Router fails or returns
no paths.
To avoid exposing a node's identity in a blinded path, only create
one-hop blinded paths if the node has been announced, and thus has
public channels. Otherwise, there is no way to route a payment to the
node, exposing its identity needlessly.
@jkczyz
Copy link
Contributor Author

jkczyz commented Dec 15, 2023

Rebased and squashed.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Oops, two issues, but I can fix in #2798.

.filter(|details| details.counterparty.features.supports_route_blinding())
.filter(|details| amount_msats <= details.inbound_capacity_msat)
.filter(|details| amount_msats >= details.inbound_htlc_minimum_msat.unwrap_or(0))
.filter(|details| amount_msats <= details.inbound_htlc_maximum_msat.unwrap_or(0))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops, this should read unwrap_or(u64::max_value()).

features: BlindedHopFeatures::empty(),
},
node_id: details.counterparty.node_id,
htlc_maximum_msat: details.inbound_htlc_maximum_msat.unwrap_or(0),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

@TheBlueMatt TheBlueMatt merged commit b9797eb into lightningdevkit:main Dec 15, 2023
12 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multi-hop blinded paths in Offers
4 participants