Add API for constructing blinded payment paths by valentinewallace · Pull Request #2412 · lightningdevkit/rust-lightning

valentinewallace · 2023-07-13T19:14:09Z

Lays some groundwork for route blinding support, helping address #1970. Lets us build blinded payment paths from the TLV payloads contained within them. A lot of the diff is code moves so we can separate out onion message-specific code from blinded payment-specific code.

We don't pad any values at the moment, which would increase privacy.

More testing is added in #2413.

alecchendev

Nice!

codecov-commenter · 2023-07-20T20:37:43Z

Codecov Report

❌ Patch coverage is 88.34951% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.03%. Comparing base (4fb5708) to head (ea84f2a).
⚠️ Report is 5537 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/blinded_path/payment.rs	0.00%	5 Missing ⚠️
lightning/src/blinded_path/message.rs	90.32%	3 Missing ⚠️
lightning/src/events/mod.rs	25.00%	3 Missing ⚠️
lightning/src/blinded_path/utils.rs	95.23%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #2412 +/- ##
==========================================
+ Coverage 90.59% 91.03% +0.43% 
==========================================
Files 106 109 +3 Lines 56577 62958 +6381 Branches 56577 62958 +6381 ==========================================
+ Hits 51258 57316 +6058 - Misses 5319 5642 +323

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

valentinewallace · 2023-08-01T18:34:25Z

+ /// Used to authenticate the sender of a payment to the receiver and tie MPP HTLCs together.
+ payment_secret: PaymentSecret,


Note for reviewers: it's a bit LDK-specific and not generic that we take a PaymentSecret here. But as discussed previously with @TheBlueMatt, the spec should not be prescribing anything for the final payload at all because it's constructed for the receiver, by the receiver, so I don't see an issue with doing this instead of calling this field path_id and taking a Vec<u8> (which is an unnecessary allocation as well). But I get that this might be a bit confusing coming from the spec so thoughts welcome here.

Where is this encoded in the BlindedPath?

Not sure I understood the question but the payment_secret is written in the Writeable implementation for BlindedPaymentTlvs

Ah, I see. It's encoded in the BlindedPath when utils::encrypt_payload is called.

valentinewallace · 2023-08-01T18:42:10Z

Rebased due to conflicts and squashed minor fixups.

jkczyz · 2023-08-04T20:41:44Z

+ /// Used to authenticate the sender of a payment to the receiver and tie MPP HTLCs together.
+ payment_secret: PaymentSecret,


Where is this encoded in the BlindedPath?

jkczyz · 2023-08-08T19:33:27Z

+ /// Used to authenticate the sender of a payment to the receiver and tie MPP HTLCs together.
+ payment_secret: PaymentSecret,


Ah, I see. It's encoded in the BlindedPath when utils::encrypt_payload is called.

TheBlueMatt

Basically lg

TheBlueMatt · 2023-08-14T00:32:57Z

+
+// Advance the blinded onion message path by one hop, so make the second hop into the new
+// introduction node.
+pub(crate) fn advance_path_by_one<NS: Deref, T: secp256k1::Signing + secp256k1::Verification>(


Don't we need the same utility for blinded payment paths (and do they have the same control tlvs)?

I was planning to do this as a follow-up to #2413, is your preference to get it out sooner? The control TLVs aren't the same.

No, that's alright, I just wanted to make sure that the move made sense - I guess the follow-up will mean basically copying this and writing some new code to do the same thing for blinded payment paths?

No, that's alright, I just wanted to make sure that the move made sense - I guess the follow-up will mean basically copying this and writing some new code to do the same thing for blinded payment paths?
Yep, that's the thinking

TheBlueMatt · 2023-08-14T00:49:17Z

+ cltv_expiry_delta: path.iter().map(|(_, tlvs)| tlvs.cltv_expiry_delta())
+ .try_fold(0u16, |acc, delta| acc.checked_add(delta)).ok_or(())?,
+ htlc_minimum_msat: path.iter().map(|(_, tlvs)| tlvs.htlc_minimum_msat()).max().unwrap_or(0),
+ // TODO: this field isn't present in route blinding encrypted data


What's this TODO? Get the spec updated, or...?

I think the spec needs an update here, I commented on the route blinding PR lightning/bolts#765 (comment) and t-bast is supposed to get back to me soon.

Ended up adding this field as a separate parameter to BlindedPath::new_for_payment. See linked thread but it shouldn't be part of the blinded payment TLVs themselves.

valentinewallace · 2023-08-15T16:11:06Z

Rebased because the new spec test vector failed with the new code, fixed now.

TheBlueMatt · 2023-08-16T21:22:29Z

+ .map(|f| f / 1_000_000)
+ .ok_or(())?;
+	}
+	let htlc_minimum_msat = path.iter().map(|(_, tlvs)| tlvs.htlc_minimum_msat()).max().unwrap_or(0);


This needs to consider the fees charged earlier - if the second node in the path wants at least 1 sat, and the previous node charges 1 sat in fee, the min should be 2 sats.

Digging into this a bit, it looks like eclair/CLN don't do this. Are we sure the htlc_minimum_msat field isn't the minimum final receive amount to the blinded path, i.e. the min amount prior to the aggregated fees being added? At first glance looks like get_route will treat the min to send to the blinded path as BlindedPayInfo::htlc_minimum_msat + aggregated_fees.

Hmm, we should ask them, then, cause that doesn't make much sense to me - if we have a blinded path along A -> B -> C, and A charges a 10% fee and B has a min_htlc of 100 sats, then the min HTLC for the full path needs to be 110 sats. The total min depends on where the max-min is in the path.

This way it can be more easily reused for blinded payment paths.

We'll similarly separate blinded path payments code into its own module.

Useful for blinded payment path construction.

We want a similar macro for reading TLV streams without a length prefix, so rename this one to disambiguate.

valentinewallace · 2023-08-22T17:26:43Z

Rebased due to conflicts and squashed.

TheBlueMatt

Basically all LGTM. Only real issue is the 8 byte restirction on the padding cause we didn't tell rustc what type we meant (which is maybe the most obvious rustc bug I've ever come across that they insist is definitely a Feature 🤦 )

TheBlueMatt · 2023-08-22T23:30:01Z

+impl Readable for BlindedPaymentTlvs {
+	fn read<R: io::Read>(r: &mut R) -> Result<Self, DecodeError> {
+ _init_and_read_tlv_stream!(r, {
+ (1, _padding, option),


By not defining a type here, I believe rustc magically assumed you wanted an i64 🎉 , because this crate uses i64s a ton, all over the place, its definitely what we wanted.....and also we'll fail to read if the padding isn't either empty or exactly 8 bytes :(

Would the trick used in ControlTlvs work?
let _padding:Option<Padding> = _padding;

Yeah, that should work. I think I'll add some docs to impl_writeable* saying that you can't have unused read values. It would be nice if there were a way to enforce that programmatically, haven't looked into it though.

Useful for when you want to use _init_and_read_len_prefixed_tlv_fields but there is no length byte at the start of the TLV stream.

Also adds a util for general blinded hop creation to be reused for blinded payment paths.

The previous name can be confused for the shared secret that the rho is derived from.

valentinewallace mentioned this pull request Jul 13, 2023
BOLT12 Offers Tracking Issue #1970
Open
60 tasks

valentinewallace added this to the 0.0.117 milestone Jul 13, 2023

valentinewallace mentioned this pull request Jul 13, 2023
Route blinding MVP #2413
Merged
1 task

alecchendev reviewed Jul 20, 2023
View reviewed changes

Comment threadlightning/src/blinded_path/payment.rs Outdated
Comment threadlightning/src/blinded_path/payment.rs Outdated
Comment threadlightning/src/blinded_path/payment.rs Outdated
Comment threadlightning/src/blinded_path/payment.rs Outdated
Comment threadlightning/src/util/ser_macros.rs Outdated

valentinewallace force-pushed the 2023-07-construct-blinded-paths branch from cd8952d to a6d08feCompare July 20, 2023 19:27

valentinewallace commented Aug 1, 2023
View reviewed changes

valentinewallace force-pushed the 2023-07-construct-blinded-paths branch 2 times, most recently from fd88452 to 1fa4ecdCompare August 1, 2023 18:41

jkczyz reviewed Aug 4, 2023
View reviewed changes

valentinewallace force-pushed the 2023-07-construct-blinded-paths branch from 387e06e to 4b7b7e8Compare August 8, 2023 18:36

jkczyz reviewed Aug 8, 2023
View reviewed changes

alecchendev reviewed Aug 9, 2023
View reviewed changes

Comment threadlightning/src/blinded_path/payment.rs

valentinewallace force-pushed the 2023-07-construct-blinded-paths branch 2 times, most recently from 7f8cec6 to 4bb7ea6Compare August 9, 2023 21:43

TheBlueMatt reviewed Aug 14, 2023
View reviewed changes

valentinewallace force-pushed the 2023-07-construct-blinded-paths branch 2 times, most recently from bed22b0 to a52d43eCompare August 15, 2023 16:10

TheBlueMatt reviewed Aug 15, 2023
View reviewed changes

Comment threadlightning/src/blinded_path/payment.rs Outdated
Comment threadlightning/src/blinded_path/payment.rs Outdated

valentinewallace force-pushed the 2023-07-construct-blinded-paths branch from a52d43e to 9d481d1Compare August 16, 2023 18:51

TheBlueMatt reviewed Aug 16, 2023
View reviewed changes

valentinewallace force-pushed the 2023-07-construct-blinded-paths branch 2 times, most recently from 3b75164 to 74f728fCompare August 21, 2023 17:59

TheBlueMatt linked an issue Aug 21, 2023 that may be closed by this pull request
Pass RoutingFees to channel_penalty_msat#2509
Closed

TheBlueMatt removed a link to an issue Aug 21, 2023
Pass RoutingFees to channel_penalty_msat#2509
Closed

jkczyz reviewed Aug 21, 2023
View reviewed changes

Comment threadlightning/src/blinded_path/payment.rs Outdated
Comment threadlightning/src/blinded_path/payment.rs

valentinewallace force-pushed the 2023-07-construct-blinded-paths branch from 74f728f to c9d2968Compare August 22, 2023 15:16

Move blinded path util into blinded_path::utils
a5b7cf2
This way it can be more easily reused for blinded payment paths.

valentinewallace added 6 commits August 22, 2023 13:18

Move some blinded path message code into message submodule.
381cc64
We'll similarly separate blinded path payments code into its own module.

Move blinded message path util into message submodule
fe5a076

Move Padding into blinded_path module for use in blinded payments
1b35661

Update blinded path util to take iterator instead of slice
7c1726b
Useful for blinded payment path construction.

Minor BlindedHop docs update
9777485

Rename ser macro
4a30d9e
We want a similar macro for reading TLV streams without a length prefix, so rename this one to disambiguate.

valentinewallace force-pushed the 2023-07-construct-blinded-paths branch from c9d2968 to 63b60e2Compare August 22, 2023 17:26

jkczyz previously approved these changes Aug 22, 2023
View reviewed changes

valentinewallace mentioned this pull request Aug 22, 2023
Aggregate BlindedPayInfo for blinded routes #2514
Merged

TheBlueMatt reviewed Aug 22, 2023
View reviewed changes

valentinewallace added 6 commits August 23, 2023 11:24

Add new _init_and_read_tlv_stream ser macro
cf64e3f
Useful for when you want to use _init_and_read_len_prefixed_tlv_fields but there is no length byte at the start of the TLV stream.

Simplify onion message blinded hop construction
d224f98
Also adds a util for general blinded hop creation to be reused for blinded payment paths.

Support constructing BlindedPaths for payments.
76f8cc1

Blinded paths: rename encrypted_tlvs_ss to *_rho for precision
0ddd3cb
The previous name can be confused for the shared secret that the rho is derived from.

Fix documentation on onion message packet ControlTlvs
ebb0676

Document _init_and_read_* ser macro requirements
ea84f2a

valentinewallace dismissed jkczyz’s stale review via ea84f2aAugust 23, 2023 15:29

valentinewallace force-pushed the 2023-07-construct-blinded-paths branch from 63b60e2 to ea84f2aCompare August 23, 2023 15:29

TheBlueMatt approved these changes Aug 23, 2023
View reviewed changes

jkczyz approved these changes Aug 23, 2023
View reviewed changes

TheBlueMatt merged commit 0211daa into lightningdevkit:mainAug 23, 2023

		/// Used to authenticate the sender of a payment to the receiver and tie MPP HTLCs together.
		payment_secret: PaymentSecret,

Uh oh!

Conversation

valentinewallace commented Jul 13, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alecchendev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Jul 20, 2023• edited by codecov BotLoading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentinewallace commented Aug 1, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

valentinewallace commented Aug 15, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

valentinewallace commented Aug 22, 2023

Uh oh!

TheBlueMatt left a comment

valentinewallace commented Jul 13, 2023•
edited
Loading

codecov-commenter commented Jul 20, 2023•
edited by codecov Bot
Loading

valentinewallace commented Aug 15, 2023•
edited
Loading