Expose blinded paths' advance_path_by_one

[valentinewallace]

Useful for LDK users that are only using the onion messages module, like LNDK.

Fixes#3134.

[valentinewallace]

@orbitalturtle does this work? I feel like I'm missing something since Matt said it was optimistic that this would make the release.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 90.90909% with 49 lines in your changes missing coverage. Please review.

Project coverage is 89.73%. Comparing base (33e6995) to head (b204a6a).
Report is 15 commits behind head on main.

Files	Patch %	Lines
lightning/src/blinded_path/message.rs	81.48%	16 Missing and 4 partials ⚠️
lightning/src/blinded_path/payment.rs	79.12%	14 Missing and 5 partials ⚠️
lightning/src/onion_message/messenger.rs	85.71%	5 Missing ⚠️
lightning/src/ln/offers_tests.rs	94.73%	0 Missing and 2 partials ⚠️
lightning/src/util/test_utils.rs	66.66%	2 Missing ⚠️
lightning/src/ln/outbound_payment.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3182 +/- ##
==========================================
- Coverage 89.78% 89.73% -0.06% 
==========================================
Files 123 123 Lines 102330 102512 +182 Branches 102330 102512 +182 ==========================================
+ Hits 91876 91988 +112 - Misses 7763 7825 +62 - Partials 2691 2699 +8 

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

[TheBlueMatt]

Okay, this is incredibly confusing, two methods in different contexts with the same name and arguments? Can we please split BlindedPath into two types now?

[valentinewallace]

Since that's somewhat orthogonal, how about renaming the methods to include _message and _payment for this PR?

[TheBlueMatt]

I wouldn't call it orthogonal at all, its very much relevant to this PR :). If you really want to avoid it, sure, we can rename the methods here, but IMO we should bite the bullet and do the split now, before we have any more code...

[valentinewallace]

@TheBlueMatt I scoped this refactor out a bit and it looks like a fair amount of work. We also currently support constructing non-LDK-specific blinded paths in our API, and it looks like that support would be removed since presumably we’d be making the BlindedPath struct private in favor of the new Blinded{Message,Payment}Path structs.

My impression is that the only users that will be interacting with blinded paths directly are advanced ones like LNDK that aren’t likely to confuse the two path types. Do you disagree with that? I’m still trying to understand the benefits + urgency here.

before we have any more code...

I think that ship has sailed :( 312 occurrences...

[TheBlueMatt]

We also currently support constructing non-LDK-specific blinded paths in our API, and it looks like that support would be removed since presumably we’d be making the BlindedPath struct private in favor of the new Blinded{Message,Payment}Path structs.

I don't see why we'd break that? We'd just force users to specify if a BlindedPath is for payments or not when they deserialize/build it?

I think that ship has sailed :( 312 occurrences...

Sure, but presumably most of those are a simple sed to fix?

I'm not really sure how we expect users to use this API without randomly having broken crap because they passed the wrong kind of BlindedPath.

[valentinewallace]

Renamed the methods and rebased due to conflicts.

I don't see why we'd break that? We'd just force users to specify if a BlindedPath is for payments or not when they deserialize/build it?

So keep the BlindedPath type public and have 3 path types, then? Just want to make sure I'm understanding the suggestion. I think the concern about confusion should be largely addressed for this PR but I'll look into opening a follow-up this week. There are some async payment APIs being added soon that I think would benefit from this refactor.

[valentinewallace]

Ok, updated with the split into BlindedMessagePath/BlindedPaymentPath.

[valentinewallace]

I think/hope CI is fixed now

[TheBlueMatt]

So keep the BlindedPath type public and have 3 path types, then?

Wait why does BlindedPath need to be public? I mean we can implement (de)serialization on BlindedMessagePath and BlindedPaymentPath and have both delegate to BlindedPath and then wrap, which seems fine?

[valentinewallace]

So keep the BlindedPath type public and have 3 path types, then?

Wait why does BlindedPath need to be public? I mean we can implement (de)serialization on BlindedMessagePath and BlindedPaymentPath and have both delegate to BlindedPath and then wrap, which seems fine?

Right now, someone could theoretically construct a blinded path that isn't LDK specific (i.e. another implementation can receive over it) by manually constructing a BlindedPath. They'd have to build the keys/payloads themselves so it may not be realistic, but it is kinda supported. Making the generic BlindedPath private would be removing this support since the only way to construct paths would be through Blinded{Message,Payment}::new, which construct LDK-specific blinded paths. Thoughts?

[TheBlueMatt]

We could have a Blinded{Message,Payment}::from_raw constructor that just takes the fields we need, no?

[valentinewallace]

We could have a Blinded{Message,Payment}::from_raw constructor that just takes the fields we need, no?

Sure, but that feels like scope creep to me and I'd rather save it for follow-up. I can make the type private for now in this PR if that sounds good.

[TheBlueMatt]

I can make the type private for now in this PR if that sounds good.

Sure, seems fine.

[TheBlueMatt]

Needs rebase already :(

[TheBlueMatt]

Needs rebase :(

[valentinewallace]

Rebased

[TheBlueMatt]

Feel free to squash IMO.

[valentinewallace]

Squashed with the following diff making BlindedPath private:

diff --git a/lightning/src/blinded_path/mod.rs b/lightning/src/blinded_path/mod.rs
index 0f48044f2..6a278b7fd 100644
--- a/lightning/src/blinded_path/mod.rs+++ b/lightning/src/blinded_path/mod.rs@@ -26,7 +26,7 @@ use crate::prelude::*;
/// Onion messages and payments can be sent and received to blinded paths, which serve to hide the
/// identity of the recipient.
#[derive(Clone, Debug, Hash, PartialEq, Eq)]
-pub(super) struct BlindedPath {+struct BlindedPath {
/// To send to a blinded path, the sender first finds a route to the unblinded
/// `introduction_node`, which can unblind its [`encrypted_payload`] to find out the onion
/// message or payment's next hop and forward it along.
diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs
index ffc27cb0e..2d7450b91 100644
--- a/lightning/src/routing/router.rs+++ b/lightning/src/routing/router.rs@@ -3541,7 +3541,7 @@ fn build_route_from_hops_internal<L: Deref>(
#[cfg(test)]
mod tests {
-	use crate::blinded_path::{BlindedHop, BlindedPath, IntroductionNode};+	use crate::blinded_path::BlindedHop;
use crate::blinded_path::payment::BlindedPaymentPath;
use crate::routing::gossip::{NetworkGraph, P2PGossipSync, NodeId, EffectiveCapacity};
use crate::routing::utxo::UtxoResult;
@@ -7682,22 +7682,6 @@ mod tests {
#[test]
fn blinded_route_ser() {
- let blinded_path_1 = BlindedPath {- introduction_node: IntroductionNode::NodeId(ln_test_utils::pubkey(42)),- blinding_point: ln_test_utils::pubkey(43),- blinded_hops: vec![- BlindedHop { blinded_node_id: ln_test_utils::pubkey(44), encrypted_payload: Vec::new() },- BlindedHop { blinded_node_id: ln_test_utils::pubkey(45), encrypted_payload: Vec::new() }- ],- };- let blinded_path_2 = BlindedPath {- introduction_node: IntroductionNode::NodeId(ln_test_utils::pubkey(46)),- blinding_point: ln_test_utils::pubkey(47),- blinded_hops: vec![- BlindedHop { blinded_node_id: ln_test_utils::pubkey(48), encrypted_payload: Vec::new() },- BlindedHop { blinded_node_id: ln_test_utils::pubkey(49), encrypted_payload: Vec::new() }- ],- };
// (De)serialize a Route with 1 blinded path out of two total paths.
let mut route = Route { paths: vec![Path {
hops: vec![RouteHop {
@@ -7710,8 +7694,11 @@ mod tests {
maybe_announced_channel: true,
}],
blinded_tail: Some(BlindedTail {
- hops: blinded_path_1.blinded_hops,- blinding_point: blinded_path_1.blinding_point,+ hops: vec![+ BlindedHop { blinded_node_id: ln_test_utils::pubkey(44), encrypted_payload: Vec::new() },+ BlindedHop { blinded_node_id: ln_test_utils::pubkey(45), encrypted_payload: Vec::new() }+ ],+ blinding_point: ln_test_utils::pubkey(43),
excess_final_cltv_expiry_delta: 40,
final_value_msat: 100,
})}, Path {
@@ -7733,8 +7720,11 @@ mod tests {
// (De)serialize a Route with two paths, each containing a blinded tail.
route.paths[1].blinded_tail = Some(BlindedTail {
- hops: blinded_path_2.blinded_hops,- blinding_point: blinded_path_2.blinding_point,+ hops: vec![+ BlindedHop { blinded_node_id: ln_test_utils::pubkey(48), encrypted_payload: Vec::new() },+ BlindedHop { blinded_node_id: ln_test_utils::pubkey(49), encrypted_payload: Vec::new() }+ ],+ blinding_point: ln_test_utils::pubkey(47),
excess_final_cltv_expiry_delta: 41,
final_value_msat: 101,
});
@@ -7749,11 +7739,6 @@ mod tests {
// Ensure we'll score the channel that's inbound to a blinded path's introduction node, and
// account for the blinded tail's final amount_msat.
let mut inflight_htlcs = InFlightHtlcs::new();
- let blinded_path = BlindedPath {- introduction_node: IntroductionNode::NodeId(ln_test_utils::pubkey(43)),- blinding_point: ln_test_utils::pubkey(48),- blinded_hops: vec![BlindedHop { blinded_node_id: ln_test_utils::pubkey(49), encrypted_payload: Vec::new() }],- };
let path = Path {
hops: vec![RouteHop {
pubkey: ln_test_utils::pubkey(42),
@@ -7774,8 +7759,8 @@ mod tests {
maybe_announced_channel: false,
}],
blinded_tail: Some(BlindedTail {
- hops: blinded_path.blinded_hops,- blinding_point: blinded_path.blinding_point,+ hops: vec![BlindedHop { blinded_node_id: ln_test_utils::pubkey(49), encrypted_payload: Vec::new() }],+ blinding_point: ln_test_utils::pubkey(48),
excess_final_cltv_expiry_delta: 0,
final_value_msat: 200,
}),
@@ -7788,14 +7773,6 @@ mod tests {
#[test]
fn blinded_path_cltv_shadow_offset() {
// Make sure we add a shadow offset when sending to blinded paths.
- let blinded_path = BlindedPath {- introduction_node: IntroductionNode::NodeId(ln_test_utils::pubkey(43)),- blinding_point: ln_test_utils::pubkey(44),- blinded_hops: vec![- BlindedHop { blinded_node_id: ln_test_utils::pubkey(45), encrypted_payload: Vec::new() },- BlindedHop { blinded_node_id: ln_test_utils::pubkey(46), encrypted_payload: Vec::new() }- ],- };
let mut route = Route { paths: vec![Path {
hops: vec![RouteHop {
pubkey: ln_test_utils::pubkey(42),
@@ -7817,8 +7794,11 @@ mod tests {
}
],
blinded_tail: Some(BlindedTail {
- hops: blinded_path.blinded_hops,- blinding_point: blinded_path.blinding_point,+ hops: vec![+ BlindedHop { blinded_node_id: ln_test_utils::pubkey(45), encrypted_payload: Vec::new() },+ BlindedHop { blinded_node_id: ln_test_utils::pubkey(46), encrypted_payload: Vec::new() }+ ],+ blinding_point: ln_test_utils::pubkey(44),
excess_final_cltv_expiry_delta: 0,
final_value_msat: 200,
}),
diff --git a/lightning/src/routing/scoring.rs b/lightning/src/routing/scoring.rs
index a04a18504..7945a5b80 100644
--- a/lightning/src/routing/scoring.rs+++ b/lightning/src/routing/scoring.rs@@ -2152,7 +2152,7 @@ impl Readable for ChannelLiquidity {
#[cfg(test)]
mod tests {
use super::{ChannelLiquidity, HistoricalBucketRangeTracker, ProbabilisticScoringFeeParameters, ProbabilisticScoringDecayParameters, ProbabilisticScorer};
-	use crate::blinded_path::{BlindedHop, BlindedPath, IntroductionNode};+	use crate::blinded_path::BlindedHop;
use crate::util::config::UserConfig;
use crate::ln::channelmanager;
@@ -3567,16 +3567,9 @@ mod tests {
let mut path = payment_path_for_amount(768);
let recipient_hop = path.hops.pop().unwrap();
- let blinded_path = BlindedPath {- introduction_node: IntroductionNode::NodeId(path.hops.last().as_ref().unwrap().pubkey),- blinding_point: test_utils::pubkey(42),- blinded_hops: vec![- BlindedHop { blinded_node_id: test_utils::pubkey(44), encrypted_payload: Vec::new() }- ],- };
path.blinded_tail = Some(BlindedTail {
- hops: blinded_path.blinded_hops,- blinding_point: blinded_path.blinding_point,+ hops: vec![BlindedHop { blinded_node_id: test_utils::pubkey(44), encrypted_payload: Vec::new() }],+ blinding_point: test_utils::pubkey(42),
excess_final_cltv_expiry_delta: recipient_hop.cltv_expiry_delta,
final_value_msat: recipient_hop.fee_msat,
});

[TheBlueMatt]

nit: we could keep using the one_hop constructor to avoid the change here.

[TheBlueMatt]

I don't really see anything wrong with this being public, but I also don't feel strongly that it should be.

[jkczyz]

Keeping it private would at least prevent someone from forming a BlindedPaymentPath from the data inside of a BlindedMessagePath, which would be have the wrong encrypted data.

[valentinewallace]

If we care to go this way, the only reason I can think of is for creating a blinded payment path to be received over by another implementation. If that's the case, it would make more sense for the API to only take a raw payload for the last hop, not intermediate hops, and to still compute payinfo on behalf of the user IMO. Maybe not too high priority, though?

[TheBlueMatt]

Yea, probably something we do if we get people request it and otherwise not bother.

[jkczyz]

Keeping it private would at least prevent someone from forming a BlindedPaymentPath from the data inside of a BlindedMessagePath, which would be have the wrong encrypted data.

[valentinewallace]

Pushed with the following diff:

diff --git a/lightning/src/blinded_path/message.rs b/lightning/src/blinded_path/message.rs
index 602c447ab..93d36d621 100644
--- a/lightning/src/blinded_path/message.rs+++ b/lightning/src/blinded_path/message.rs@@ -141,14 +141,14 @@ impl BlindedMessagePath {
/// Advance the blinded onion message path by one hop, making the second hop into the new
/// introduction node.
///
- /// Will only modify `path` when returning `Ok`.+ /// Will only modify `self` when returning `Ok`.
pub fn advance_path_by_one<NS: Deref, NL: Deref, T>(
&mut self, node_signer: &NS, node_id_lookup: &NL, secp_ctx: &Secp256k1<T>
) -> Result<(), ()>
where
- NS::Target: NodeSigner,- NL::Target: NodeIdLookUp,- T: secp256k1::Signing + secp256k1::Verification,+ NS::Target: NodeSigner,+ NL::Target: NodeIdLookUp,+ T: secp256k1::Signing + secp256k1::Verification,
{
let control_tlvs_ss = node_signer.ecdh(Recipient::Node, &self.0.blinding_point, None)?;
let rho = onion_utils::gen_rho_from_shared_secret(&control_tlvs_ss.secret_bytes());
@@ -182,8 +182,8 @@ impl BlindedMessagePath {
}
}
- pub(crate) fn set_introduction_node_id(&mut self, node_id: PublicKey) {- self.0.introduction_node = IntroductionNode::NodeId(node_id);+ pub(crate) fn introduction_node_mut(&mut self) -> &mut IntroductionNode {+ &mut self.0.introduction_node
}
#[cfg(test)]
diff --git a/lightning/src/blinded_path/payment.rs b/lightning/src/blinded_path/payment.rs
index ccb866b13..765e0b91f 100644
--- a/lightning/src/blinded_path/payment.rs+++ b/lightning/src/blinded_path/payment.rs@@ -124,14 +124,14 @@ impl BlindedPaymentPath {
/// Advance the blinded onion payment path by one hop, making the second hop into the new
/// introduction node.
///
- /// Will only modify `path` when returning `Ok`.+ /// Will only modify `self` when returning `Ok`.
pub fn advance_path_by_one<NS: Deref, NL: Deref, T>(
&mut self, node_signer: &NS, node_id_lookup: &NL, secp_ctx: &Secp256k1<T>
) -> Result<(), ()>
where
- NS::Target: NodeSigner,- NL::Target: NodeIdLookUp,- T: secp256k1::Signing + secp256k1::Verification,+ NS::Target: NodeSigner,+ NL::Target: NodeIdLookUp,+ T: secp256k1::Signing + secp256k1::Verification,
{
let control_tlvs_ss = node_signer.ecdh(Recipient::Node, &self.0.blinding_point, None)?;
let rho = onion_utils::gen_rho_from_shared_secret(&control_tlvs_ss.secret_bytes());
diff --git a/lightning/src/onion_message/messenger.rs b/lightning/src/onion_message/messenger.rs
index 772c0a6c2..a5c81bb36 100644
--- a/lightning/src/onion_message/messenger.rs+++ b/lightning/src/onion_message/messenger.rs@@ -702,7 +702,7 @@ impl Destination {
.public_introduction_node_id(network_graph)
.and_then(|node_id| node_id.as_pubkey().ok())
{
- path.set_introduction_node_id(pubkey);+ *path.introduction_node_mut() = IntroductionNode::NodeId(pubkey);
}
}
}