Add support for custom HTLC TLVs by alecchendev · Pull Request #2308 · lightningdevkit/rust-lightning

alecchendev · 2023-05-19T22:32:52Z

Closes#1298. This PR adds support for sending and receiving custom HTLC TLVs.

Custom TLVs allow users to send extra application-specific data with a payment. These have the additional flexibility compared to payment_metadata that they don't have to reflect recipient generated data provided in an invoice, in which payment_metadata could (probably) be reused.

On the send side, a user can provide their serialized TLVs as a Vec<(u64, Vec<u8>)> to RecipientOnionFields::with_custom_tlvs which checks whether the type numbers are unique, increasing, and in the range reserved for custom values. They'll then pass this into whichever send payment function they use, it'll be serialized in the onion payload, and sent with the payment.

On the receive side, when deserializing the onion payload, we just add the bytes back into this type, then pipe it from the onion through to Event::PaymentClaimable, dropping non-matching TLVs between payment parts.

alecchendev

A couple questions for reviewers:

codecov-commenter · 2023-05-20T23:31:56Z

Codecov Report

Patch coverage: 95.32% and project coverage change: +0.34% 🎉

Comparison is base (e13ff10) 90.24% compared to head (93a8fb6) 90.58%.
Report is 21 commits behind head on main.

❗ Current head 93a8fb6 differs from pull request most recent head 1db481f. Consider uploading reports for the commit 1db481f to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@ Coverage Diff @@## main #2308 +/- ##
==========================================
+ Coverage 90.24% 90.58% +0.34% 
==========================================
Files 106 106 Lines 55817 59617 +3800 Branches 55817 59617 +3800 ==========================================
+ Hits 50370 54007 +3637 - Misses 5447 5610 +163

Files Changed	Coverage Δ
lightning/src/events/mod.rs	`41.98% <ø> (+0.37%)`	⬆️
lightning/src/util/ser_macros.rs	`67.75% <0.00%> (-2.81%)`	⬇️
lightning/src/ln/onion_route_tests.rs	`98.51% <85.71%> (+0.01%)`	⬆️
lightning/src/ln/payment_tests.rs	`97.38% <95.04%> (-0.28%)`	⬇️
lightning/src/ln/msgs.rs	`85.10% <96.55%> (+0.36%)`	⬆️
lightning/src/ln/channelmanager.rs	`86.73% <97.91%> (+1.28%)`	⬆️
lightning-invoice/src/payment.rs	`88.59% <100.00%> (-0.08%)`	⬇️
lightning/src/ln/functional_test_utils.rs	`88.93% <100.00%> (+0.01%)`	⬆️
lightning/src/ln/onion_utils.rs	`90.90% <100.00%> (-0.16%)`	⬇️
lightning/src/ln/outbound_payment.rs	`92.77% <100.00%> (+3.57%)`	⬆️

... and 4 files with indirect coverage changes

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

TheBlueMatt

Sorry for the delay here, just getting caught up post-travels.

alecchendev · 2023-06-10T23:03:22Z

Rebased due to conflicts.

TheBlueMatt

This basically looks good, I think, needs another reviewer.

wpaulino

Looks good! Since a user can now include arbitrary data, do we know if the router can handle restricting the number of hops based on the existing recipient payload?

TheBlueMatt · 2023-06-17T23:03:05Z

do we know if the router can handle restricting the number of hops based on the existing recipient payload?

It currently cannot, though this is an issue that was really introduced in payment metadata, this just makes it worse. Tracked at #2201.

TheBlueMatt · 2023-06-17T23:03:22Z

Feel free to squash fixups when you next push, I think.

TheBlueMatt · 2023-07-30T20:41:50Z

While we're still debating nits on the docs, can you go ahead and squash so we're set up to land? @wpaulino or @dunxen might want to take a look thereafter.

wpaulino

LGTM

alecchendev · 2023-08-01T02:27:57Z

Fixed and squashed immediately since they were just small nits

ariard · 2023-08-01T02:37:35Z

this is why blinded paths have no error returned at all.

yes, though i’m not sure no returning error is workable on the long-term due to onion bandwidth cost and payment reliability (at least end-to-end), though here more conversation at the spec-level.

answered on the two comments:

i still wish we do our best to mask user agent and version from deanonymization attacks like done for browser / tor services
i still think “authentication” should be a strong suggestion, seen example of multiple payer not compatible with current payment_secret

though overall i won’t insist and if / when we have users privacy screwed up on those vectors, i’ll just pointed it back to you guys :)

ariard · 2023-08-02T03:19:05Z

i still wish we do our best to mask user agent and version from deanonymization attacks like done for browser / tor services

here the code is good in fact after looking (though doc confusing), just good if we don’t change it in the future to match BOLT4 bad suggestion imho

alecchendev · 2023-08-03T17:33:33Z

Squashed immediately for minor doc change:

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 968b0765..daa0b34f 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -371,7 +371,7 @@ pub enum FailureCode {
/// We failed to process the payload after the onion was decrypted. You may wish to
/// use this when receiving custom HTLC TLVs with even type numbers that you don't recognize.
///
- /// If available, the tuple data should include the type number and byte offset in the+ /// If available, the tuple data may include the type number and byte offset in the
/// decrypted byte stream where the failure occurred.
InvalidOnionPayload(Option<(u64, u16)>),
}

ariard · 2023-08-04T00:19:34Z

Happy after following diff applied, payment_secret does not work for N-payer / N-payee flows, though it sounds reasonable for one payer / one payer flow, as far as I know about things like cancellable payments

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index daa0b34f..db1108b4 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -4769,6 +4769,8 @@ where
/// You MUST check you've understood all even TLVs before using this to
/// claim, otherwise you may unintentionally agree to some protocol you do not understand.
///
+ /// You MAY authenticate the even TLVs additionally of `payment_secret` if the protocol
+ /// flow is more sophisticated than one payer / one payee.
/// [`claim_funds`]: Self::claim_funds
pub fn claim_funds_with_known_custom_tlvs(&self, payment_preimage: PaymentPreimage) {
self.claim_payment_internal(payment_preimage, true);

ariard

Code Review ACK 1db481f

Nevermind the above comment effectively multi-payer scheme would need additional code changes in our MPP validation and as such wider custom HTLC support (though they’re the ones more exposed in term of potential deanonymization attacks)

ariard · 2023-08-07T23:54:21Z

+ FailureCode::InvalidOnionPayload(data) => {
+ let fail_data = match data {
+ Some((typ, offset)) => [BigSize(typ).encode(), offset.encode()].concat(),
+ None => Vec::new(),


nit: I think the debug_assert(data.is_none()) could be added due to its only usage in claim_payment_internal with a None value given to InvalidOnionPayload.

TheBlueMatt · 2023-08-08T20:21:54Z

Grrr, needs rebase, sorry about that.

Custom TLVs allow users to send extra application-specific data with a payment. These have the additional flexibility compared to `payment_metadata` that they don't have to reflect recipient generated data provided in an invoice, in which `payment_metadata` could be reused. We ensure provided type numbers are unique, increasing, and within the experimental range with the `RecipientOnionFields::with_custom_tlvs` method. This begins sender-side support for custom TLVs.

When serialized, the TLVs in `OutboundOnionPayload`, unlike a normal TLV stream, are prefixed with the length of the stream. To allow a user to add arbitrary custom TLVs, we aren't able to communicate to our serialization macros exactly which fields to expect, so this commit adds new macro variants to allow appending an extra set of bytes (and modifying the prefixed length accordingly). Because the keysend preimage TLV has a type number in the custom type range, and a user's TLVs may have type numbers above and/or below keysend's type number, and because TLV streams must be serialized in increasing order by type number, this commit also ensures the keysend TLV is properly sorted/serialized amongst the custom TLVs.

This completes basic receiver-side support for custom TLVs and adds functional testing for sending and receiving.

Upon receiving multiple payment parts with custom TLVs, we fail payments if they have any non-matching or missing even TLVs, and otherwise just drop non-matching TLVs if they're odd.

When a user decodes custom TLVs, if they fail to recognize even type numbers they should fail back with the correct failure code and fail data. This new variant adds the proper failure variant for the user to pass into `ChannelManager::fail_htlc_backwards_with_reason`. Note that the enum discriminants were removed because when adding a struct variant we can no longer make use of the discriminant through casting like we previously did, and instead have to manually define the associated failure code anyway.

Because we don't know which custom TLV type numbers the user is expecting (and it would be cumbersome for them to tell us), instead of failing unknown even custom TLVs on deserialization, we accept all custom TLVs, and pass them to the user to check whether they recognize them and choose to fail back if they don't. However, a user may not check for custom TLVs, in which case we should reject any even custom TLVs as unknown. This commit makes sure a user must explicitly accept a payment with even custom TLVs, by (1) making the default `ChannelManager::claim_funds` fail if the payment had even custom TLVs and (2) adding a new function `ChannelManager::claim_funds_with_known_custom_tlvs` that accepts them. This commit also refactors our custom TLVs test and updates various documentation to account for this.

alecchendev · 2023-08-08T21:19:23Z

Rebased

TheBlueMatt · 2023-08-10T16:23:19Z

+ }
+
+ #[cfg(debug_assertions)] {
+ let mut last_seen: Option<u64> = None;


it'd be nice to have this in one macro body with the above non-custom TLVs so that we can debug_assert!() that the ordering including custom TLVs is correct.

TheBlueMatt · 2023-08-10T16:38:36Z

+ let tlvs = &mut self.custom_tlvs;
+ let further_tlvs = &mut further_htlc_fields.custom_tlvs;
+
+ let even_tlvs: Vec<&(u64, Vec<u8>)> = tlvs.iter().filter(|(typ, _)| *typ % 2 == 0).collect();


You should be able to do the comparison here without collecting into a Vec.

TheBlueMatt · 2023-08-10T17:21:03Z

+ let preimage = if let Some(ref preimage) = keysend_preimage {
+ Some((5482373484, preimage.encode()))
+ } else { None };
+ let mut custom_tlvs: Vec<&(u64, Vec<u8>)> = custom_tlvs.iter().chain(preimage.iter()).collect();


We are trying to build an interator, we shouldn't have to collect here (though the macro may want it twice, so we may have to do the iter().chain() in the macro argument rather than here.

Though AFAIK it wouldn't be possible to sort the tlvs as an iterator?

TheBlueMatt · 2023-08-10T17:25:11Z

+ match &purpose {
+ PaymentPurpose::InvoicePayment { payment_secret, .. } => {
+ assert_eq!(our_payment_secret, *payment_secret);
+ assert_eq!(Some(*payment_secret), onion_fields.as_ref().unwrap().payment_secret);


nit: just for your reference, if we're copying test util bits into a test, we can usually skip a bunch of the assertions - the test utility methods tend to be a bit verbose and check everything, which is great in the general code, but in an individual test we don't need to test too much more than we care about for the purpose of the specific test.

TheBlueMatt · 2023-08-10T17:48:50Z

+	if let Some(expected_tlvs) = expected_receive_tlvs {
+ // Claim and match expected
+ let events = nodes[3].node.get_and_clear_pending_events();
+ println!("events: {:?}", events);


Oops, I missed this before hitting merge, let's do a followup and remove this (and maybe hit a few other nits, though no real pressure on those).

alecchendev · 2023-08-10T18:59:58Z

Thanks for all the review everybody, will get a follow up up soon :)

alecchendev · 2023-05-19T22:32:52Z

Closes#1298. This PR adds support for sending and receiving custom HTLC TLVs.

Custom TLVs allow users to send extra application-specific data with a payment. These have the additional flexibility compared to payment_metadata that they don't have to reflect recipient generated data provided in an invoice, in which payment_metadata could (probably) be reused.

On the send side, a user can provide their serialized TLVs as a Vec<(u64, Vec<u8>)> to RecipientOnionFields::with_custom_tlvs which checks whether the type numbers are unique, increasing, and in the range reserved for custom values. They'll then pass this into whichever send payment function they use, it'll be serialized in the onion payload, and sent with the payment.

On the receive side, when deserializing the onion payload, we just add the bytes back into this type, then pipe it from the onion through to Event::PaymentClaimable, dropping non-matching TLVs between payment parts.

alecchendev

A couple questions for reviewers:

codecov-commenter · 2023-05-20T23:31:56Z

Codecov Report

Patch coverage: 95.32% and project coverage change: +0.34% 🎉

Comparison is base (e13ff10) 90.24% compared to head (93a8fb6) 90.58%.
Report is 21 commits behind head on main.

❗ Current head 93a8fb6 differs from pull request most recent head 1db481f. Consider uploading reports for the commit 1db481f to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@ Coverage Diff @@## main #2308 +/- ##
==========================================
+ Coverage 90.24% 90.58% +0.34% 
==========================================
Files 106 106 Lines 55817 59617 +3800 Branches 55817 59617 +3800 ==========================================
+ Hits 50370 54007 +3637 - Misses 5447 5610 +163

Files Changed	Coverage Δ
lightning/src/events/mod.rs	`41.98% <ø> (+0.37%)`	⬆️
lightning/src/util/ser_macros.rs	`67.75% <0.00%> (-2.81%)`	⬇️
lightning/src/ln/onion_route_tests.rs	`98.51% <85.71%> (+0.01%)`	⬆️
lightning/src/ln/payment_tests.rs	`97.38% <95.04%> (-0.28%)`	⬇️
lightning/src/ln/msgs.rs	`85.10% <96.55%> (+0.36%)`	⬆️
lightning/src/ln/channelmanager.rs	`86.73% <97.91%> (+1.28%)`	⬆️
lightning-invoice/src/payment.rs	`88.59% <100.00%> (-0.08%)`	⬇️
lightning/src/ln/functional_test_utils.rs	`88.93% <100.00%> (+0.01%)`	⬆️
lightning/src/ln/onion_utils.rs	`90.90% <100.00%> (-0.16%)`	⬇️
lightning/src/ln/outbound_payment.rs	`92.77% <100.00%> (+3.57%)`	⬆️

... and 4 files with indirect coverage changes

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

TheBlueMatt

Sorry for the delay here, just getting caught up post-travels.

alecchendev · 2023-06-10T23:03:22Z

Rebased due to conflicts.

TheBlueMatt

This basically looks good, I think, needs another reviewer.

wpaulino

Looks good! Since a user can now include arbitrary data, do we know if the router can handle restricting the number of hops based on the existing recipient payload?

TheBlueMatt · 2023-06-17T23:03:05Z

do we know if the router can handle restricting the number of hops based on the existing recipient payload?

It currently cannot, though this is an issue that was really introduced in payment metadata, this just makes it worse. Tracked at #2201.

TheBlueMatt · 2023-06-17T23:03:22Z

Feel free to squash fixups when you next push, I think.

TheBlueMatt · 2023-07-30T20:41:50Z

While we're still debating nits on the docs, can you go ahead and squash so we're set up to land? @wpaulino or @dunxen might want to take a look thereafter.

wpaulino

LGTM

alecchendev · 2023-08-01T02:27:57Z

Fixed and squashed immediately since they were just small nits

ariard · 2023-08-01T02:37:35Z

this is why blinded paths have no error returned at all.

yes, though i’m not sure no returning error is workable on the long-term due to onion bandwidth cost and payment reliability (at least end-to-end), though here more conversation at the spec-level.

answered on the two comments:

i still wish we do our best to mask user agent and version from deanonymization attacks like done for browser / tor services
i still think “authentication” should be a strong suggestion, seen example of multiple payer not compatible with current payment_secret

though overall i won’t insist and if / when we have users privacy screwed up on those vectors, i’ll just pointed it back to you guys :)

ariard · 2023-08-02T03:19:05Z

i still wish we do our best to mask user agent and version from deanonymization attacks like done for browser / tor services

here the code is good in fact after looking (though doc confusing), just good if we don’t change it in the future to match BOLT4 bad suggestion imho

alecchendev · 2023-08-03T17:33:33Z

Squashed immediately for minor doc change:

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 968b0765..daa0b34f 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -371,7 +371,7 @@ pub enum FailureCode {
/// We failed to process the payload after the onion was decrypted. You may wish to
/// use this when receiving custom HTLC TLVs with even type numbers that you don't recognize.
///
- /// If available, the tuple data should include the type number and byte offset in the+ /// If available, the tuple data may include the type number and byte offset in the
/// decrypted byte stream where the failure occurred.
InvalidOnionPayload(Option<(u64, u16)>),
}

ariard · 2023-08-04T00:19:34Z

Happy after following diff applied, payment_secret does not work for N-payer / N-payee flows, though it sounds reasonable for one payer / one payer flow, as far as I know about things like cancellable payments

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index daa0b34f..db1108b4 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -4769,6 +4769,8 @@ where
/// You MUST check you've understood all even TLVs before using this to
/// claim, otherwise you may unintentionally agree to some protocol you do not understand.
///
+ /// You MAY authenticate the even TLVs additionally of `payment_secret` if the protocol
+ /// flow is more sophisticated than one payer / one payee.
/// [`claim_funds`]: Self::claim_funds
pub fn claim_funds_with_known_custom_tlvs(&self, payment_preimage: PaymentPreimage) {
self.claim_payment_internal(payment_preimage, true);

ariard

Code Review ACK 1db481f

Nevermind the above comment effectively multi-payer scheme would need additional code changes in our MPP validation and as such wider custom HTLC support (though they’re the ones more exposed in term of potential deanonymization attacks)

ariard · 2023-08-07T23:54:21Z

+ FailureCode::InvalidOnionPayload(data) => {
+ let fail_data = match data {
+ Some((typ, offset)) => [BigSize(typ).encode(), offset.encode()].concat(),
+ None => Vec::new(),


nit: I think the debug_assert(data.is_none()) could be added due to its only usage in claim_payment_internal with a None value given to InvalidOnionPayload.

TheBlueMatt · 2023-08-08T20:21:54Z

Grrr, needs rebase, sorry about that.

Custom TLVs allow users to send extra application-specific data with a payment. These have the additional flexibility compared to `payment_metadata` that they don't have to reflect recipient generated data provided in an invoice, in which `payment_metadata` could be reused. We ensure provided type numbers are unique, increasing, and within the experimental range with the `RecipientOnionFields::with_custom_tlvs` method. This begins sender-side support for custom TLVs.

When serialized, the TLVs in `OutboundOnionPayload`, unlike a normal TLV stream, are prefixed with the length of the stream. To allow a user to add arbitrary custom TLVs, we aren't able to communicate to our serialization macros exactly which fields to expect, so this commit adds new macro variants to allow appending an extra set of bytes (and modifying the prefixed length accordingly). Because the keysend preimage TLV has a type number in the custom type range, and a user's TLVs may have type numbers above and/or below keysend's type number, and because TLV streams must be serialized in increasing order by type number, this commit also ensures the keysend TLV is properly sorted/serialized amongst the custom TLVs.

This completes basic receiver-side support for custom TLVs and adds functional testing for sending and receiving.

Upon receiving multiple payment parts with custom TLVs, we fail payments if they have any non-matching or missing even TLVs, and otherwise just drop non-matching TLVs if they're odd.

When a user decodes custom TLVs, if they fail to recognize even type numbers they should fail back with the correct failure code and fail data. This new variant adds the proper failure variant for the user to pass into `ChannelManager::fail_htlc_backwards_with_reason`. Note that the enum discriminants were removed because when adding a struct variant we can no longer make use of the discriminant through casting like we previously did, and instead have to manually define the associated failure code anyway.

Because we don't know which custom TLV type numbers the user is expecting (and it would be cumbersome for them to tell us), instead of failing unknown even custom TLVs on deserialization, we accept all custom TLVs, and pass them to the user to check whether they recognize them and choose to fail back if they don't. However, a user may not check for custom TLVs, in which case we should reject any even custom TLVs as unknown. This commit makes sure a user must explicitly accept a payment with even custom TLVs, by (1) making the default `ChannelManager::claim_funds` fail if the payment had even custom TLVs and (2) adding a new function `ChannelManager::claim_funds_with_known_custom_tlvs` that accepts them. This commit also refactors our custom TLVs test and updates various documentation to account for this.

alecchendev · 2023-08-08T21:19:23Z

Rebased

TheBlueMatt · 2023-08-10T16:23:19Z

+ }
+
+ #[cfg(debug_assertions)] {
+ let mut last_seen: Option<u64> = None;


it'd be nice to have this in one macro body with the above non-custom TLVs so that we can debug_assert!() that the ordering including custom TLVs is correct.

TheBlueMatt · 2023-08-10T16:38:36Z

+ let tlvs = &mut self.custom_tlvs;
+ let further_tlvs = &mut further_htlc_fields.custom_tlvs;
+
+ let even_tlvs: Vec<&(u64, Vec<u8>)> = tlvs.iter().filter(|(typ, _)| *typ % 2 == 0).collect();


You should be able to do the comparison here without collecting into a Vec.

TheBlueMatt · 2023-08-10T17:21:03Z

+ let preimage = if let Some(ref preimage) = keysend_preimage {
+ Some((5482373484, preimage.encode()))
+ } else { None };
+ let mut custom_tlvs: Vec<&(u64, Vec<u8>)> = custom_tlvs.iter().chain(preimage.iter()).collect();


We are trying to build an interator, we shouldn't have to collect here (though the macro may want it twice, so we may have to do the iter().chain() in the macro argument rather than here.

Though AFAIK it wouldn't be possible to sort the tlvs as an iterator?

TheBlueMatt · 2023-08-10T17:25:11Z

+ match &purpose {
+ PaymentPurpose::InvoicePayment { payment_secret, .. } => {
+ assert_eq!(our_payment_secret, *payment_secret);
+ assert_eq!(Some(*payment_secret), onion_fields.as_ref().unwrap().payment_secret);


nit: just for your reference, if we're copying test util bits into a test, we can usually skip a bunch of the assertions - the test utility methods tend to be a bit verbose and check everything, which is great in the general code, but in an individual test we don't need to test too much more than we care about for the purpose of the specific test.

TheBlueMatt · 2023-08-10T17:48:50Z

+	if let Some(expected_tlvs) = expected_receive_tlvs {
+ // Claim and match expected
+ let events = nodes[3].node.get_and_clear_pending_events();
+ println!("events: {:?}", events);


Oops, I missed this before hitting merge, let's do a followup and remove this (and maybe hit a few other nits, though no real pressure on those).

alecchendev · 2023-08-10T18:59:58Z

Thanks for all the review everybody, will get a follow up up soon :)

alecchendev · 2023-05-19T22:32:52Z

Closes#1298. This PR adds support for sending and receiving custom HTLC TLVs.

Custom TLVs allow users to send extra application-specific data with a payment. These have the additional flexibility compared to payment_metadata that they don't have to reflect recipient generated data provided in an invoice, in which payment_metadata could (probably) be reused.

On the send side, a user can provide their serialized TLVs as a Vec<(u64, Vec<u8>)> to RecipientOnionFields::with_custom_tlvs which checks whether the type numbers are unique, increasing, and in the range reserved for custom values. They'll then pass this into whichever send payment function they use, it'll be serialized in the onion payload, and sent with the payment.

On the receive side, when deserializing the onion payload, we just add the bytes back into this type, then pipe it from the onion through to Event::PaymentClaimable, dropping non-matching TLVs between payment parts.

alecchendev

A couple questions for reviewers:

codecov-commenter · 2023-05-20T23:31:56Z

Codecov Report

Patch coverage: 95.32% and project coverage change: +0.34% 🎉

Comparison is base (e13ff10) 90.24% compared to head (93a8fb6) 90.58%.
Report is 21 commits behind head on main.

❗ Current head 93a8fb6 differs from pull request most recent head 1db481f. Consider uploading reports for the commit 1db481f to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@ Coverage Diff @@## main #2308 +/- ##
==========================================
+ Coverage 90.24% 90.58% +0.34% 
==========================================
Files 106 106 Lines 55817 59617 +3800 Branches 55817 59617 +3800 ==========================================
+ Hits 50370 54007 +3637 - Misses 5447 5610 +163

Files Changed	Coverage Δ
lightning/src/events/mod.rs	`41.98% <ø> (+0.37%)`	⬆️
lightning/src/util/ser_macros.rs	`67.75% <0.00%> (-2.81%)`	⬇️
lightning/src/ln/onion_route_tests.rs	`98.51% <85.71%> (+0.01%)`	⬆️
lightning/src/ln/payment_tests.rs	`97.38% <95.04%> (-0.28%)`	⬇️
lightning/src/ln/msgs.rs	`85.10% <96.55%> (+0.36%)`	⬆️
lightning/src/ln/channelmanager.rs	`86.73% <97.91%> (+1.28%)`	⬆️
lightning-invoice/src/payment.rs	`88.59% <100.00%> (-0.08%)`	⬇️
lightning/src/ln/functional_test_utils.rs	`88.93% <100.00%> (+0.01%)`	⬆️
lightning/src/ln/onion_utils.rs	`90.90% <100.00%> (-0.16%)`	⬇️
lightning/src/ln/outbound_payment.rs	`92.77% <100.00%> (+3.57%)`	⬆️

... and 4 files with indirect coverage changes

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

TheBlueMatt

Sorry for the delay here, just getting caught up post-travels.

alecchendev · 2023-06-10T23:03:22Z

Rebased due to conflicts.

TheBlueMatt

This basically looks good, I think, needs another reviewer.

wpaulino

Looks good! Since a user can now include arbitrary data, do we know if the router can handle restricting the number of hops based on the existing recipient payload?

TheBlueMatt · 2023-06-17T23:03:05Z

do we know if the router can handle restricting the number of hops based on the existing recipient payload?

It currently cannot, though this is an issue that was really introduced in payment metadata, this just makes it worse. Tracked at #2201.

TheBlueMatt · 2023-06-17T23:03:22Z

Feel free to squash fixups when you next push, I think.

TheBlueMatt · 2023-07-30T20:41:50Z

While we're still debating nits on the docs, can you go ahead and squash so we're set up to land? @wpaulino or @dunxen might want to take a look thereafter.

wpaulino

LGTM

alecchendev · 2023-08-01T02:27:57Z

Fixed and squashed immediately since they were just small nits

ariard · 2023-08-01T02:37:35Z

this is why blinded paths have no error returned at all.

yes, though i’m not sure no returning error is workable on the long-term due to onion bandwidth cost and payment reliability (at least end-to-end), though here more conversation at the spec-level.

answered on the two comments:

i still wish we do our best to mask user agent and version from deanonymization attacks like done for browser / tor services
i still think “authentication” should be a strong suggestion, seen example of multiple payer not compatible with current payment_secret

though overall i won’t insist and if / when we have users privacy screwed up on those vectors, i’ll just pointed it back to you guys :)

ariard · 2023-08-02T03:19:05Z

i still wish we do our best to mask user agent and version from deanonymization attacks like done for browser / tor services

here the code is good in fact after looking (though doc confusing), just good if we don’t change it in the future to match BOLT4 bad suggestion imho

alecchendev · 2023-08-03T17:33:33Z

Squashed immediately for minor doc change:

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 968b0765..daa0b34f 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -371,7 +371,7 @@ pub enum FailureCode {
/// We failed to process the payload after the onion was decrypted. You may wish to
/// use this when receiving custom HTLC TLVs with even type numbers that you don't recognize.
///
- /// If available, the tuple data should include the type number and byte offset in the+ /// If available, the tuple data may include the type number and byte offset in the
/// decrypted byte stream where the failure occurred.
InvalidOnionPayload(Option<(u64, u16)>),
}

ariard · 2023-08-04T00:19:34Z

Happy after following diff applied, payment_secret does not work for N-payer / N-payee flows, though it sounds reasonable for one payer / one payer flow, as far as I know about things like cancellable payments

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index daa0b34f..db1108b4 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -4769,6 +4769,8 @@ where
/// You MUST check you've understood all even TLVs before using this to
/// claim, otherwise you may unintentionally agree to some protocol you do not understand.
///
+ /// You MAY authenticate the even TLVs additionally of `payment_secret` if the protocol
+ /// flow is more sophisticated than one payer / one payee.
/// [`claim_funds`]: Self::claim_funds
pub fn claim_funds_with_known_custom_tlvs(&self, payment_preimage: PaymentPreimage) {
self.claim_payment_internal(payment_preimage, true);

ariard

Code Review ACK 1db481f

Nevermind the above comment effectively multi-payer scheme would need additional code changes in our MPP validation and as such wider custom HTLC support (though they’re the ones more exposed in term of potential deanonymization attacks)

ariard · 2023-08-07T23:54:21Z

+ FailureCode::InvalidOnionPayload(data) => {
+ let fail_data = match data {
+ Some((typ, offset)) => [BigSize(typ).encode(), offset.encode()].concat(),
+ None => Vec::new(),


nit: I think the debug_assert(data.is_none()) could be added due to its only usage in claim_payment_internal with a None value given to InvalidOnionPayload.

TheBlueMatt · 2023-08-08T20:21:54Z

Grrr, needs rebase, sorry about that.

Custom TLVs allow users to send extra application-specific data with a payment. These have the additional flexibility compared to `payment_metadata` that they don't have to reflect recipient generated data provided in an invoice, in which `payment_metadata` could be reused. We ensure provided type numbers are unique, increasing, and within the experimental range with the `RecipientOnionFields::with_custom_tlvs` method. This begins sender-side support for custom TLVs.

When serialized, the TLVs in `OutboundOnionPayload`, unlike a normal TLV stream, are prefixed with the length of the stream. To allow a user to add arbitrary custom TLVs, we aren't able to communicate to our serialization macros exactly which fields to expect, so this commit adds new macro variants to allow appending an extra set of bytes (and modifying the prefixed length accordingly). Because the keysend preimage TLV has a type number in the custom type range, and a user's TLVs may have type numbers above and/or below keysend's type number, and because TLV streams must be serialized in increasing order by type number, this commit also ensures the keysend TLV is properly sorted/serialized amongst the custom TLVs.

This completes basic receiver-side support for custom TLVs and adds functional testing for sending and receiving.

Upon receiving multiple payment parts with custom TLVs, we fail payments if they have any non-matching or missing even TLVs, and otherwise just drop non-matching TLVs if they're odd.

When a user decodes custom TLVs, if they fail to recognize even type numbers they should fail back with the correct failure code and fail data. This new variant adds the proper failure variant for the user to pass into `ChannelManager::fail_htlc_backwards_with_reason`. Note that the enum discriminants were removed because when adding a struct variant we can no longer make use of the discriminant through casting like we previously did, and instead have to manually define the associated failure code anyway.

Because we don't know which custom TLV type numbers the user is expecting (and it would be cumbersome for them to tell us), instead of failing unknown even custom TLVs on deserialization, we accept all custom TLVs, and pass them to the user to check whether they recognize them and choose to fail back if they don't. However, a user may not check for custom TLVs, in which case we should reject any even custom TLVs as unknown. This commit makes sure a user must explicitly accept a payment with even custom TLVs, by (1) making the default `ChannelManager::claim_funds` fail if the payment had even custom TLVs and (2) adding a new function `ChannelManager::claim_funds_with_known_custom_tlvs` that accepts them. This commit also refactors our custom TLVs test and updates various documentation to account for this.

alecchendev · 2023-08-08T21:19:23Z

Rebased

TheBlueMatt · 2023-08-10T16:23:19Z

+ }
+
+ #[cfg(debug_assertions)] {
+ let mut last_seen: Option<u64> = None;


it'd be nice to have this in one macro body with the above non-custom TLVs so that we can debug_assert!() that the ordering including custom TLVs is correct.

TheBlueMatt · 2023-08-10T16:38:36Z

+ let tlvs = &mut self.custom_tlvs;
+ let further_tlvs = &mut further_htlc_fields.custom_tlvs;
+
+ let even_tlvs: Vec<&(u64, Vec<u8>)> = tlvs.iter().filter(|(typ, _)| *typ % 2 == 0).collect();


You should be able to do the comparison here without collecting into a Vec.

TheBlueMatt · 2023-08-10T17:21:03Z

+ let preimage = if let Some(ref preimage) = keysend_preimage {
+ Some((5482373484, preimage.encode()))
+ } else { None };
+ let mut custom_tlvs: Vec<&(u64, Vec<u8>)> = custom_tlvs.iter().chain(preimage.iter()).collect();


We are trying to build an interator, we shouldn't have to collect here (though the macro may want it twice, so we may have to do the iter().chain() in the macro argument rather than here.

Though AFAIK it wouldn't be possible to sort the tlvs as an iterator?

TheBlueMatt · 2023-08-10T17:25:11Z

+ match &purpose {
+ PaymentPurpose::InvoicePayment { payment_secret, .. } => {
+ assert_eq!(our_payment_secret, *payment_secret);
+ assert_eq!(Some(*payment_secret), onion_fields.as_ref().unwrap().payment_secret);


nit: just for your reference, if we're copying test util bits into a test, we can usually skip a bunch of the assertions - the test utility methods tend to be a bit verbose and check everything, which is great in the general code, but in an individual test we don't need to test too much more than we care about for the purpose of the specific test.

TheBlueMatt · 2023-08-10T17:48:50Z

+	if let Some(expected_tlvs) = expected_receive_tlvs {
+ // Claim and match expected
+ let events = nodes[3].node.get_and_clear_pending_events();
+ println!("events: {:?}", events);


Oops, I missed this before hitting merge, let's do a followup and remove this (and maybe hit a few other nits, though no real pressure on those).

alecchendev · 2023-08-10T18:59:58Z

Thanks for all the review everybody, will get a follow up up soon :)

alecchendev · 2023-05-19T22:32:52Z

Closes#1298. This PR adds support for sending and receiving custom HTLC TLVs.

Custom TLVs allow users to send extra application-specific data with a payment. These have the additional flexibility compared to payment_metadata that they don't have to reflect recipient generated data provided in an invoice, in which payment_metadata could (probably) be reused.

On the send side, a user can provide their serialized TLVs as a Vec<(u64, Vec<u8>)> to RecipientOnionFields::with_custom_tlvs which checks whether the type numbers are unique, increasing, and in the range reserved for custom values. They'll then pass this into whichever send payment function they use, it'll be serialized in the onion payload, and sent with the payment.

On the receive side, when deserializing the onion payload, we just add the bytes back into this type, then pipe it from the onion through to Event::PaymentClaimable, dropping non-matching TLVs between payment parts.

alecchendev

A couple questions for reviewers:

codecov-commenter · 2023-05-20T23:31:56Z

Codecov Report

Patch coverage: 95.32% and project coverage change: +0.34% 🎉

Comparison is base (e13ff10) 90.24% compared to head (93a8fb6) 90.58%.
Report is 21 commits behind head on main.

❗ Current head 93a8fb6 differs from pull request most recent head 1db481f. Consider uploading reports for the commit 1db481f to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@ Coverage Diff @@## main #2308 +/- ##
==========================================
+ Coverage 90.24% 90.58% +0.34% 
==========================================
Files 106 106 Lines 55817 59617 +3800 Branches 55817 59617 +3800 ==========================================
+ Hits 50370 54007 +3637 - Misses 5447 5610 +163

Files Changed	Coverage Δ
lightning/src/events/mod.rs	`41.98% <ø> (+0.37%)`	⬆️
lightning/src/util/ser_macros.rs	`67.75% <0.00%> (-2.81%)`	⬇️
lightning/src/ln/onion_route_tests.rs	`98.51% <85.71%> (+0.01%)`	⬆️
lightning/src/ln/payment_tests.rs	`97.38% <95.04%> (-0.28%)`	⬇️
lightning/src/ln/msgs.rs	`85.10% <96.55%> (+0.36%)`	⬆️
lightning/src/ln/channelmanager.rs	`86.73% <97.91%> (+1.28%)`	⬆️
lightning-invoice/src/payment.rs	`88.59% <100.00%> (-0.08%)`	⬇️
lightning/src/ln/functional_test_utils.rs	`88.93% <100.00%> (+0.01%)`	⬆️
lightning/src/ln/onion_utils.rs	`90.90% <100.00%> (-0.16%)`	⬇️
lightning/src/ln/outbound_payment.rs	`92.77% <100.00%> (+3.57%)`	⬆️

... and 4 files with indirect coverage changes

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

TheBlueMatt

Sorry for the delay here, just getting caught up post-travels.

alecchendev · 2023-06-10T23:03:22Z

Rebased due to conflicts.

TheBlueMatt

This basically looks good, I think, needs another reviewer.

wpaulino

Looks good! Since a user can now include arbitrary data, do we know if the router can handle restricting the number of hops based on the existing recipient payload?

TheBlueMatt · 2023-06-17T23:03:05Z

do we know if the router can handle restricting the number of hops based on the existing recipient payload?

It currently cannot, though this is an issue that was really introduced in payment metadata, this just makes it worse. Tracked at #2201.

TheBlueMatt · 2023-06-17T23:03:22Z

Feel free to squash fixups when you next push, I think.

TheBlueMatt · 2023-07-30T20:41:50Z

While we're still debating nits on the docs, can you go ahead and squash so we're set up to land? @wpaulino or @dunxen might want to take a look thereafter.

wpaulino

LGTM

alecchendev · 2023-08-01T02:27:57Z

Fixed and squashed immediately since they were just small nits

ariard · 2023-08-01T02:37:35Z

this is why blinded paths have no error returned at all.

yes, though i’m not sure no returning error is workable on the long-term due to onion bandwidth cost and payment reliability (at least end-to-end), though here more conversation at the spec-level.

answered on the two comments:

i still wish we do our best to mask user agent and version from deanonymization attacks like done for browser / tor services
i still think “authentication” should be a strong suggestion, seen example of multiple payer not compatible with current payment_secret

though overall i won’t insist and if / when we have users privacy screwed up on those vectors, i’ll just pointed it back to you guys :)

ariard · 2023-08-02T03:19:05Z

i still wish we do our best to mask user agent and version from deanonymization attacks like done for browser / tor services

here the code is good in fact after looking (though doc confusing), just good if we don’t change it in the future to match BOLT4 bad suggestion imho

alecchendev · 2023-08-03T17:33:33Z

Squashed immediately for minor doc change:

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 968b0765..daa0b34f 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -371,7 +371,7 @@ pub enum FailureCode {
/// We failed to process the payload after the onion was decrypted. You may wish to
/// use this when receiving custom HTLC TLVs with even type numbers that you don't recognize.
///
- /// If available, the tuple data should include the type number and byte offset in the+ /// If available, the tuple data may include the type number and byte offset in the
/// decrypted byte stream where the failure occurred.
InvalidOnionPayload(Option<(u64, u16)>),
}

ariard · 2023-08-04T00:19:34Z

Happy after following diff applied, payment_secret does not work for N-payer / N-payee flows, though it sounds reasonable for one payer / one payer flow, as far as I know about things like cancellable payments

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index daa0b34f..db1108b4 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -4769,6 +4769,8 @@ where
/// You MUST check you've understood all even TLVs before using this to
/// claim, otherwise you may unintentionally agree to some protocol you do not understand.
///
+ /// You MAY authenticate the even TLVs additionally of `payment_secret` if the protocol
+ /// flow is more sophisticated than one payer / one payee.
/// [`claim_funds`]: Self::claim_funds
pub fn claim_funds_with_known_custom_tlvs(&self, payment_preimage: PaymentPreimage) {
self.claim_payment_internal(payment_preimage, true);

ariard

Code Review ACK 1db481f

Nevermind the above comment effectively multi-payer scheme would need additional code changes in our MPP validation and as such wider custom HTLC support (though they’re the ones more exposed in term of potential deanonymization attacks)

ariard · 2023-08-07T23:54:21Z

+ FailureCode::InvalidOnionPayload(data) => {
+ let fail_data = match data {
+ Some((typ, offset)) => [BigSize(typ).encode(), offset.encode()].concat(),
+ None => Vec::new(),


nit: I think the debug_assert(data.is_none()) could be added due to its only usage in claim_payment_internal with a None value given to InvalidOnionPayload.

TheBlueMatt · 2023-08-08T20:21:54Z

Grrr, needs rebase, sorry about that.

Custom TLVs allow users to send extra application-specific data with a payment. These have the additional flexibility compared to `payment_metadata` that they don't have to reflect recipient generated data provided in an invoice, in which `payment_metadata` could be reused. We ensure provided type numbers are unique, increasing, and within the experimental range with the `RecipientOnionFields::with_custom_tlvs` method. This begins sender-side support for custom TLVs.

When serialized, the TLVs in `OutboundOnionPayload`, unlike a normal TLV stream, are prefixed with the length of the stream. To allow a user to add arbitrary custom TLVs, we aren't able to communicate to our serialization macros exactly which fields to expect, so this commit adds new macro variants to allow appending an extra set of bytes (and modifying the prefixed length accordingly). Because the keysend preimage TLV has a type number in the custom type range, and a user's TLVs may have type numbers above and/or below keysend's type number, and because TLV streams must be serialized in increasing order by type number, this commit also ensures the keysend TLV is properly sorted/serialized amongst the custom TLVs.

This completes basic receiver-side support for custom TLVs and adds functional testing for sending and receiving.

Upon receiving multiple payment parts with custom TLVs, we fail payments if they have any non-matching or missing even TLVs, and otherwise just drop non-matching TLVs if they're odd.

When a user decodes custom TLVs, if they fail to recognize even type numbers they should fail back with the correct failure code and fail data. This new variant adds the proper failure variant for the user to pass into `ChannelManager::fail_htlc_backwards_with_reason`. Note that the enum discriminants were removed because when adding a struct variant we can no longer make use of the discriminant through casting like we previously did, and instead have to manually define the associated failure code anyway.

Because we don't know which custom TLV type numbers the user is expecting (and it would be cumbersome for them to tell us), instead of failing unknown even custom TLVs on deserialization, we accept all custom TLVs, and pass them to the user to check whether they recognize them and choose to fail back if they don't. However, a user may not check for custom TLVs, in which case we should reject any even custom TLVs as unknown. This commit makes sure a user must explicitly accept a payment with even custom TLVs, by (1) making the default `ChannelManager::claim_funds` fail if the payment had even custom TLVs and (2) adding a new function `ChannelManager::claim_funds_with_known_custom_tlvs` that accepts them. This commit also refactors our custom TLVs test and updates various documentation to account for this.

alecchendev · 2023-08-08T21:19:23Z

Rebased

TheBlueMatt · 2023-08-10T16:23:19Z

+ }
+
+ #[cfg(debug_assertions)] {
+ let mut last_seen: Option<u64> = None;


it'd be nice to have this in one macro body with the above non-custom TLVs so that we can debug_assert!() that the ordering including custom TLVs is correct.

TheBlueMatt · 2023-08-10T16:38:36Z

+ let tlvs = &mut self.custom_tlvs;
+ let further_tlvs = &mut further_htlc_fields.custom_tlvs;
+
+ let even_tlvs: Vec<&(u64, Vec<u8>)> = tlvs.iter().filter(|(typ, _)| *typ % 2 == 0).collect();


You should be able to do the comparison here without collecting into a Vec.

TheBlueMatt · 2023-08-10T17:21:03Z

+ let preimage = if let Some(ref preimage) = keysend_preimage {
+ Some((5482373484, preimage.encode()))
+ } else { None };
+ let mut custom_tlvs: Vec<&(u64, Vec<u8>)> = custom_tlvs.iter().chain(preimage.iter()).collect();


We are trying to build an interator, we shouldn't have to collect here (though the macro may want it twice, so we may have to do the iter().chain() in the macro argument rather than here.

Though AFAIK it wouldn't be possible to sort the tlvs as an iterator?

TheBlueMatt · 2023-08-10T17:25:11Z

+ match &purpose {
+ PaymentPurpose::InvoicePayment { payment_secret, .. } => {
+ assert_eq!(our_payment_secret, *payment_secret);
+ assert_eq!(Some(*payment_secret), onion_fields.as_ref().unwrap().payment_secret);


nit: just for your reference, if we're copying test util bits into a test, we can usually skip a bunch of the assertions - the test utility methods tend to be a bit verbose and check everything, which is great in the general code, but in an individual test we don't need to test too much more than we care about for the purpose of the specific test.

TheBlueMatt · 2023-08-10T17:48:50Z

+	if let Some(expected_tlvs) = expected_receive_tlvs {
+ // Claim and match expected
+ let events = nodes[3].node.get_and_clear_pending_events();
+ println!("events: {:?}", events);


Oops, I missed this before hitting merge, let's do a followup and remove this (and maybe hit a few other nits, though no real pressure on those).

alecchendev · 2023-08-10T18:59:58Z

Thanks for all the review everybody, will get a follow up up soon :)

alecchendev · 2023-05-19T22:32:52Z

Closes#1298. This PR adds support for sending and receiving custom HTLC TLVs.

Custom TLVs allow users to send extra application-specific data with a payment. These have the additional flexibility compared to payment_metadata that they don't have to reflect recipient generated data provided in an invoice, in which payment_metadata could (probably) be reused.

On the send side, a user can provide their serialized TLVs as a Vec<(u64, Vec<u8>)> to RecipientOnionFields::with_custom_tlvs which checks whether the type numbers are unique, increasing, and in the range reserved for custom values. They'll then pass this into whichever send payment function they use, it'll be serialized in the onion payload, and sent with the payment.

On the receive side, when deserializing the onion payload, we just add the bytes back into this type, then pipe it from the onion through to Event::PaymentClaimable, dropping non-matching TLVs between payment parts.

alecchendev

A couple questions for reviewers:

codecov-commenter · 2023-05-20T23:31:56Z

Codecov Report

Patch coverage: 95.32% and project coverage change: +0.34% 🎉

Comparison is base (e13ff10) 90.24% compared to head (93a8fb6) 90.58%.
Report is 21 commits behind head on main.

❗ Current head 93a8fb6 differs from pull request most recent head 1db481f. Consider uploading reports for the commit 1db481f to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@ Coverage Diff @@## main #2308 +/- ##
==========================================
+ Coverage 90.24% 90.58% +0.34% 
==========================================
Files 106 106 Lines 55817 59617 +3800 Branches 55817 59617 +3800 ==========================================
+ Hits 50370 54007 +3637 - Misses 5447 5610 +163

Files Changed	Coverage Δ
lightning/src/events/mod.rs	`41.98% <ø> (+0.37%)`	⬆️
lightning/src/util/ser_macros.rs	`67.75% <0.00%> (-2.81%)`	⬇️
lightning/src/ln/onion_route_tests.rs	`98.51% <85.71%> (+0.01%)`	⬆️
lightning/src/ln/payment_tests.rs	`97.38% <95.04%> (-0.28%)`	⬇️
lightning/src/ln/msgs.rs	`85.10% <96.55%> (+0.36%)`	⬆️
lightning/src/ln/channelmanager.rs	`86.73% <97.91%> (+1.28%)`	⬆️
lightning-invoice/src/payment.rs	`88.59% <100.00%> (-0.08%)`	⬇️
lightning/src/ln/functional_test_utils.rs	`88.93% <100.00%> (+0.01%)`	⬆️
lightning/src/ln/onion_utils.rs	`90.90% <100.00%> (-0.16%)`	⬇️
lightning/src/ln/outbound_payment.rs	`92.77% <100.00%> (+3.57%)`	⬆️

... and 4 files with indirect coverage changes

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

TheBlueMatt

Sorry for the delay here, just getting caught up post-travels.

alecchendev · 2023-06-10T23:03:22Z

Rebased due to conflicts.

TheBlueMatt

This basically looks good, I think, needs another reviewer.

wpaulino

Looks good! Since a user can now include arbitrary data, do we know if the router can handle restricting the number of hops based on the existing recipient payload?

TheBlueMatt · 2023-06-17T23:03:05Z

do we know if the router can handle restricting the number of hops based on the existing recipient payload?

It currently cannot, though this is an issue that was really introduced in payment metadata, this just makes it worse. Tracked at #2201.

TheBlueMatt · 2023-06-17T23:03:22Z

Feel free to squash fixups when you next push, I think.

TheBlueMatt · 2023-07-30T20:41:50Z

While we're still debating nits on the docs, can you go ahead and squash so we're set up to land? @wpaulino or @dunxen might want to take a look thereafter.

wpaulino

LGTM

alecchendev · 2023-08-01T02:27:57Z

Fixed and squashed immediately since they were just small nits

ariard · 2023-08-01T02:37:35Z

this is why blinded paths have no error returned at all.

yes, though i’m not sure no returning error is workable on the long-term due to onion bandwidth cost and payment reliability (at least end-to-end), though here more conversation at the spec-level.

answered on the two comments:

i still wish we do our best to mask user agent and version from deanonymization attacks like done for browser / tor services
i still think “authentication” should be a strong suggestion, seen example of multiple payer not compatible with current payment_secret

though overall i won’t insist and if / when we have users privacy screwed up on those vectors, i’ll just pointed it back to you guys :)

ariard · 2023-08-02T03:19:05Z

i still wish we do our best to mask user agent and version from deanonymization attacks like done for browser / tor services

here the code is good in fact after looking (though doc confusing), just good if we don’t change it in the future to match BOLT4 bad suggestion imho

alecchendev · 2023-08-03T17:33:33Z

Squashed immediately for minor doc change:

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 968b0765..daa0b34f 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -371,7 +371,7 @@ pub enum FailureCode {
/// We failed to process the payload after the onion was decrypted. You may wish to
/// use this when receiving custom HTLC TLVs with even type numbers that you don't recognize.
///
- /// If available, the tuple data should include the type number and byte offset in the+ /// If available, the tuple data may include the type number and byte offset in the
/// decrypted byte stream where the failure occurred.
InvalidOnionPayload(Option<(u64, u16)>),
}

ariard · 2023-08-04T00:19:34Z

Happy after following diff applied, payment_secret does not work for N-payer / N-payee flows, though it sounds reasonable for one payer / one payer flow, as far as I know about things like cancellable payments

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index daa0b34f..db1108b4 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -4769,6 +4769,8 @@ where
/// You MUST check you've understood all even TLVs before using this to
/// claim, otherwise you may unintentionally agree to some protocol you do not understand.
///
+ /// You MAY authenticate the even TLVs additionally of `payment_secret` if the protocol
+ /// flow is more sophisticated than one payer / one payee.
/// [`claim_funds`]: Self::claim_funds
pub fn claim_funds_with_known_custom_tlvs(&self, payment_preimage: PaymentPreimage) {
self.claim_payment_internal(payment_preimage, true);

ariard

Code Review ACK 1db481f

Nevermind the above comment effectively multi-payer scheme would need additional code changes in our MPP validation and as such wider custom HTLC support (though they’re the ones more exposed in term of potential deanonymization attacks)

ariard · 2023-08-07T23:54:21Z

+ FailureCode::InvalidOnionPayload(data) => {
+ let fail_data = match data {
+ Some((typ, offset)) => [BigSize(typ).encode(), offset.encode()].concat(),
+ None => Vec::new(),


nit: I think the debug_assert(data.is_none()) could be added due to its only usage in claim_payment_internal with a None value given to InvalidOnionPayload.

TheBlueMatt · 2023-08-08T20:21:54Z

Grrr, needs rebase, sorry about that.

Custom TLVs allow users to send extra application-specific data with a payment. These have the additional flexibility compared to `payment_metadata` that they don't have to reflect recipient generated data provided in an invoice, in which `payment_metadata` could be reused. We ensure provided type numbers are unique, increasing, and within the experimental range with the `RecipientOnionFields::with_custom_tlvs` method. This begins sender-side support for custom TLVs.

When serialized, the TLVs in `OutboundOnionPayload`, unlike a normal TLV stream, are prefixed with the length of the stream. To allow a user to add arbitrary custom TLVs, we aren't able to communicate to our serialization macros exactly which fields to expect, so this commit adds new macro variants to allow appending an extra set of bytes (and modifying the prefixed length accordingly). Because the keysend preimage TLV has a type number in the custom type range, and a user's TLVs may have type numbers above and/or below keysend's type number, and because TLV streams must be serialized in increasing order by type number, this commit also ensures the keysend TLV is properly sorted/serialized amongst the custom TLVs.

This completes basic receiver-side support for custom TLVs and adds functional testing for sending and receiving.

Upon receiving multiple payment parts with custom TLVs, we fail payments if they have any non-matching or missing even TLVs, and otherwise just drop non-matching TLVs if they're odd.

When a user decodes custom TLVs, if they fail to recognize even type numbers they should fail back with the correct failure code and fail data. This new variant adds the proper failure variant for the user to pass into `ChannelManager::fail_htlc_backwards_with_reason`. Note that the enum discriminants were removed because when adding a struct variant we can no longer make use of the discriminant through casting like we previously did, and instead have to manually define the associated failure code anyway.

Because we don't know which custom TLV type numbers the user is expecting (and it would be cumbersome for them to tell us), instead of failing unknown even custom TLVs on deserialization, we accept all custom TLVs, and pass them to the user to check whether they recognize them and choose to fail back if they don't. However, a user may not check for custom TLVs, in which case we should reject any even custom TLVs as unknown. This commit makes sure a user must explicitly accept a payment with even custom TLVs, by (1) making the default `ChannelManager::claim_funds` fail if the payment had even custom TLVs and (2) adding a new function `ChannelManager::claim_funds_with_known_custom_tlvs` that accepts them. This commit also refactors our custom TLVs test and updates various documentation to account for this.

alecchendev · 2023-08-08T21:19:23Z

Rebased

TheBlueMatt · 2023-08-10T16:23:19Z

+ }
+
+ #[cfg(debug_assertions)] {
+ let mut last_seen: Option<u64> = None;


it'd be nice to have this in one macro body with the above non-custom TLVs so that we can debug_assert!() that the ordering including custom TLVs is correct.

TheBlueMatt · 2023-08-10T16:38:36Z

+ let tlvs = &mut self.custom_tlvs;
+ let further_tlvs = &mut further_htlc_fields.custom_tlvs;
+
+ let even_tlvs: Vec<&(u64, Vec<u8>)> = tlvs.iter().filter(|(typ, _)| *typ % 2 == 0).collect();


You should be able to do the comparison here without collecting into a Vec.

TheBlueMatt · 2023-08-10T17:21:03Z

+ let preimage = if let Some(ref preimage) = keysend_preimage {
+ Some((5482373484, preimage.encode()))
+ } else { None };
+ let mut custom_tlvs: Vec<&(u64, Vec<u8>)> = custom_tlvs.iter().chain(preimage.iter()).collect();


We are trying to build an interator, we shouldn't have to collect here (though the macro may want it twice, so we may have to do the iter().chain() in the macro argument rather than here.

Though AFAIK it wouldn't be possible to sort the tlvs as an iterator?

TheBlueMatt · 2023-08-10T17:25:11Z

+ match &purpose {
+ PaymentPurpose::InvoicePayment { payment_secret, .. } => {
+ assert_eq!(our_payment_secret, *payment_secret);
+ assert_eq!(Some(*payment_secret), onion_fields.as_ref().unwrap().payment_secret);


nit: just for your reference, if we're copying test util bits into a test, we can usually skip a bunch of the assertions - the test utility methods tend to be a bit verbose and check everything, which is great in the general code, but in an individual test we don't need to test too much more than we care about for the purpose of the specific test.

TheBlueMatt · 2023-08-10T17:48:50Z

+	if let Some(expected_tlvs) = expected_receive_tlvs {
+ // Claim and match expected
+ let events = nodes[3].node.get_and_clear_pending_events();
+ println!("events: {:?}", events);


Oops, I missed this before hitting merge, let's do a followup and remove this (and maybe hit a few other nits, though no real pressure on those).

alecchendev · 2023-08-10T18:59:58Z

Thanks for all the review everybody, will get a follow up up soon :)

alecchendev · 2023-05-19T22:32:52Z

Closes#1298. This PR adds support for sending and receiving custom HTLC TLVs.

Custom TLVs allow users to send extra application-specific data with a payment. These have the additional flexibility compared to payment_metadata that they don't have to reflect recipient generated data provided in an invoice, in which payment_metadata could (probably) be reused.

On the send side, a user can provide their serialized TLVs as a Vec<(u64, Vec<u8>)> to RecipientOnionFields::with_custom_tlvs which checks whether the type numbers are unique, increasing, and in the range reserved for custom values. They'll then pass this into whichever send payment function they use, it'll be serialized in the onion payload, and sent with the payment.

On the receive side, when deserializing the onion payload, we just add the bytes back into this type, then pipe it from the onion through to Event::PaymentClaimable, dropping non-matching TLVs between payment parts.

alecchendev

A couple questions for reviewers:

codecov-commenter · 2023-05-20T23:31:56Z

Codecov Report

Patch coverage: 95.32% and project coverage change: +0.34% 🎉

Comparison is base (e13ff10) 90.24% compared to head (93a8fb6) 90.58%.
Report is 21 commits behind head on main.

❗ Current head 93a8fb6 differs from pull request most recent head 1db481f. Consider uploading reports for the commit 1db481f to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@ Coverage Diff @@## main #2308 +/- ##
==========================================
+ Coverage 90.24% 90.58% +0.34% 
==========================================
Files 106 106 Lines 55817 59617 +3800 Branches 55817 59617 +3800 ==========================================
+ Hits 50370 54007 +3637 - Misses 5447 5610 +163

Files Changed	Coverage Δ
lightning/src/events/mod.rs	`41.98% <ø> (+0.37%)`	⬆️
lightning/src/util/ser_macros.rs	`67.75% <0.00%> (-2.81%)`	⬇️
lightning/src/ln/onion_route_tests.rs	`98.51% <85.71%> (+0.01%)`	⬆️
lightning/src/ln/payment_tests.rs	`97.38% <95.04%> (-0.28%)`	⬇️
lightning/src/ln/msgs.rs	`85.10% <96.55%> (+0.36%)`	⬆️
lightning/src/ln/channelmanager.rs	`86.73% <97.91%> (+1.28%)`	⬆️
lightning-invoice/src/payment.rs	`88.59% <100.00%> (-0.08%)`	⬇️
lightning/src/ln/functional_test_utils.rs	`88.93% <100.00%> (+0.01%)`	⬆️
lightning/src/ln/onion_utils.rs	`90.90% <100.00%> (-0.16%)`	⬇️
lightning/src/ln/outbound_payment.rs	`92.77% <100.00%> (+3.57%)`	⬆️

... and 4 files with indirect coverage changes

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

TheBlueMatt

Sorry for the delay here, just getting caught up post-travels.

alecchendev · 2023-06-10T23:03:22Z

Rebased due to conflicts.

TheBlueMatt

This basically looks good, I think, needs another reviewer.

wpaulino

Looks good! Since a user can now include arbitrary data, do we know if the router can handle restricting the number of hops based on the existing recipient payload?

TheBlueMatt · 2023-06-17T23:03:05Z

do we know if the router can handle restricting the number of hops based on the existing recipient payload?

It currently cannot, though this is an issue that was really introduced in payment metadata, this just makes it worse. Tracked at #2201.

TheBlueMatt · 2023-06-17T23:03:22Z

Feel free to squash fixups when you next push, I think.

TheBlueMatt · 2023-07-30T20:41:50Z

While we're still debating nits on the docs, can you go ahead and squash so we're set up to land? @wpaulino or @dunxen might want to take a look thereafter.

wpaulino

LGTM

alecchendev · 2023-08-01T02:27:57Z

Fixed and squashed immediately since they were just small nits

ariard · 2023-08-01T02:37:35Z

this is why blinded paths have no error returned at all.

yes, though i’m not sure no returning error is workable on the long-term due to onion bandwidth cost and payment reliability (at least end-to-end), though here more conversation at the spec-level.

answered on the two comments:

i still wish we do our best to mask user agent and version from deanonymization attacks like done for browser / tor services
i still think “authentication” should be a strong suggestion, seen example of multiple payer not compatible with current payment_secret

though overall i won’t insist and if / when we have users privacy screwed up on those vectors, i’ll just pointed it back to you guys :)

ariard · 2023-08-02T03:19:05Z

i still wish we do our best to mask user agent and version from deanonymization attacks like done for browser / tor services

here the code is good in fact after looking (though doc confusing), just good if we don’t change it in the future to match BOLT4 bad suggestion imho

alecchendev · 2023-08-03T17:33:33Z

Squashed immediately for minor doc change:

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 968b0765..daa0b34f 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -371,7 +371,7 @@ pub enum FailureCode {
/// We failed to process the payload after the onion was decrypted. You may wish to
/// use this when receiving custom HTLC TLVs with even type numbers that you don't recognize.
///
- /// If available, the tuple data should include the type number and byte offset in the+ /// If available, the tuple data may include the type number and byte offset in the
/// decrypted byte stream where the failure occurred.
InvalidOnionPayload(Option<(u64, u16)>),
}

ariard · 2023-08-04T00:19:34Z

Happy after following diff applied, payment_secret does not work for N-payer / N-payee flows, though it sounds reasonable for one payer / one payer flow, as far as I know about things like cancellable payments

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index daa0b34f..db1108b4 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -4769,6 +4769,8 @@ where
/// You MUST check you've understood all even TLVs before using this to
/// claim, otherwise you may unintentionally agree to some protocol you do not understand.
///
+ /// You MAY authenticate the even TLVs additionally of `payment_secret` if the protocol
+ /// flow is more sophisticated than one payer / one payee.
/// [`claim_funds`]: Self::claim_funds
pub fn claim_funds_with_known_custom_tlvs(&self, payment_preimage: PaymentPreimage) {
self.claim_payment_internal(payment_preimage, true);

ariard

Code Review ACK 1db481f

Nevermind the above comment effectively multi-payer scheme would need additional code changes in our MPP validation and as such wider custom HTLC support (though they’re the ones more exposed in term of potential deanonymization attacks)

ariard · 2023-08-07T23:54:21Z

+ FailureCode::InvalidOnionPayload(data) => {
+ let fail_data = match data {
+ Some((typ, offset)) => [BigSize(typ).encode(), offset.encode()].concat(),
+ None => Vec::new(),


nit: I think the debug_assert(data.is_none()) could be added due to its only usage in claim_payment_internal with a None value given to InvalidOnionPayload.

TheBlueMatt · 2023-08-08T20:21:54Z

Grrr, needs rebase, sorry about that.

Custom TLVs allow users to send extra application-specific data with a payment. These have the additional flexibility compared to `payment_metadata` that they don't have to reflect recipient generated data provided in an invoice, in which `payment_metadata` could be reused. We ensure provided type numbers are unique, increasing, and within the experimental range with the `RecipientOnionFields::with_custom_tlvs` method. This begins sender-side support for custom TLVs.

When serialized, the TLVs in `OutboundOnionPayload`, unlike a normal TLV stream, are prefixed with the length of the stream. To allow a user to add arbitrary custom TLVs, we aren't able to communicate to our serialization macros exactly which fields to expect, so this commit adds new macro variants to allow appending an extra set of bytes (and modifying the prefixed length accordingly). Because the keysend preimage TLV has a type number in the custom type range, and a user's TLVs may have type numbers above and/or below keysend's type number, and because TLV streams must be serialized in increasing order by type number, this commit also ensures the keysend TLV is properly sorted/serialized amongst the custom TLVs.

This completes basic receiver-side support for custom TLVs and adds functional testing for sending and receiving.

Upon receiving multiple payment parts with custom TLVs, we fail payments if they have any non-matching or missing even TLVs, and otherwise just drop non-matching TLVs if they're odd.

When a user decodes custom TLVs, if they fail to recognize even type numbers they should fail back with the correct failure code and fail data. This new variant adds the proper failure variant for the user to pass into `ChannelManager::fail_htlc_backwards_with_reason`. Note that the enum discriminants were removed because when adding a struct variant we can no longer make use of the discriminant through casting like we previously did, and instead have to manually define the associated failure code anyway.

Because we don't know which custom TLV type numbers the user is expecting (and it would be cumbersome for them to tell us), instead of failing unknown even custom TLVs on deserialization, we accept all custom TLVs, and pass them to the user to check whether they recognize them and choose to fail back if they don't. However, a user may not check for custom TLVs, in which case we should reject any even custom TLVs as unknown. This commit makes sure a user must explicitly accept a payment with even custom TLVs, by (1) making the default `ChannelManager::claim_funds` fail if the payment had even custom TLVs and (2) adding a new function `ChannelManager::claim_funds_with_known_custom_tlvs` that accepts them. This commit also refactors our custom TLVs test and updates various documentation to account for this.

alecchendev · 2023-08-08T21:19:23Z

Rebased

TheBlueMatt · 2023-08-10T16:23:19Z

+ }
+
+ #[cfg(debug_assertions)] {
+ let mut last_seen: Option<u64> = None;


it'd be nice to have this in one macro body with the above non-custom TLVs so that we can debug_assert!() that the ordering including custom TLVs is correct.

TheBlueMatt · 2023-08-10T16:38:36Z

+ let tlvs = &mut self.custom_tlvs;
+ let further_tlvs = &mut further_htlc_fields.custom_tlvs;
+
+ let even_tlvs: Vec<&(u64, Vec<u8>)> = tlvs.iter().filter(|(typ, _)| *typ % 2 == 0).collect();


You should be able to do the comparison here without collecting into a Vec.

TheBlueMatt · 2023-08-10T17:21:03Z

+ let preimage = if let Some(ref preimage) = keysend_preimage {
+ Some((5482373484, preimage.encode()))
+ } else { None };
+ let mut custom_tlvs: Vec<&(u64, Vec<u8>)> = custom_tlvs.iter().chain(preimage.iter()).collect();


We are trying to build an interator, we shouldn't have to collect here (though the macro may want it twice, so we may have to do the iter().chain() in the macro argument rather than here.

Though AFAIK it wouldn't be possible to sort the tlvs as an iterator?

TheBlueMatt · 2023-08-10T17:25:11Z

+ match &purpose {
+ PaymentPurpose::InvoicePayment { payment_secret, .. } => {
+ assert_eq!(our_payment_secret, *payment_secret);
+ assert_eq!(Some(*payment_secret), onion_fields.as_ref().unwrap().payment_secret);


nit: just for your reference, if we're copying test util bits into a test, we can usually skip a bunch of the assertions - the test utility methods tend to be a bit verbose and check everything, which is great in the general code, but in an individual test we don't need to test too much more than we care about for the purpose of the specific test.

TheBlueMatt · 2023-08-10T17:48:50Z

+	if let Some(expected_tlvs) = expected_receive_tlvs {
+ // Claim and match expected
+ let events = nodes[3].node.get_and_clear_pending_events();
+ println!("events: {:?}", events);


Oops, I missed this before hitting merge, let's do a followup and remove this (and maybe hit a few other nits, though no real pressure on those).

alecchendev · 2023-08-10T18:59:58Z

Thanks for all the review everybody, will get a follow up up soon :)

alecchendev · 2023-05-19T22:32:52Z

Closes#1298. This PR adds support for sending and receiving custom HTLC TLVs.

Custom TLVs allow users to send extra application-specific data with a payment. These have the additional flexibility compared to payment_metadata that they don't have to reflect recipient generated data provided in an invoice, in which payment_metadata could (probably) be reused.

On the send side, a user can provide their serialized TLVs as a Vec<(u64, Vec<u8>)> to RecipientOnionFields::with_custom_tlvs which checks whether the type numbers are unique, increasing, and in the range reserved for custom values. They'll then pass this into whichever send payment function they use, it'll be serialized in the onion payload, and sent with the payment.

On the receive side, when deserializing the onion payload, we just add the bytes back into this type, then pipe it from the onion through to Event::PaymentClaimable, dropping non-matching TLVs between payment parts.

alecchendev

A couple questions for reviewers:

codecov-commenter · 2023-05-20T23:31:56Z

Codecov Report

Patch coverage: 95.32% and project coverage change: +0.34% 🎉

Comparison is base (e13ff10) 90.24% compared to head (93a8fb6) 90.58%.
Report is 21 commits behind head on main.

❗ Current head 93a8fb6 differs from pull request most recent head 1db481f. Consider uploading reports for the commit 1db481f to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@ Coverage Diff @@## main #2308 +/- ##
==========================================
+ Coverage 90.24% 90.58% +0.34% 
==========================================
Files 106 106 Lines 55817 59617 +3800 Branches 55817 59617 +3800 ==========================================
+ Hits 50370 54007 +3637 - Misses 5447 5610 +163

Files Changed	Coverage Δ
lightning/src/events/mod.rs	`41.98% <ø> (+0.37%)`	⬆️
lightning/src/util/ser_macros.rs	`67.75% <0.00%> (-2.81%)`	⬇️
lightning/src/ln/onion_route_tests.rs	`98.51% <85.71%> (+0.01%)`	⬆️
lightning/src/ln/payment_tests.rs	`97.38% <95.04%> (-0.28%)`	⬇️
lightning/src/ln/msgs.rs	`85.10% <96.55%> (+0.36%)`	⬆️
lightning/src/ln/channelmanager.rs	`86.73% <97.91%> (+1.28%)`	⬆️
lightning-invoice/src/payment.rs	`88.59% <100.00%> (-0.08%)`	⬇️
lightning/src/ln/functional_test_utils.rs	`88.93% <100.00%> (+0.01%)`	⬆️
lightning/src/ln/onion_utils.rs	`90.90% <100.00%> (-0.16%)`	⬇️
lightning/src/ln/outbound_payment.rs	`92.77% <100.00%> (+3.57%)`	⬆️

... and 4 files with indirect coverage changes

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

TheBlueMatt

Sorry for the delay here, just getting caught up post-travels.

alecchendev · 2023-06-10T23:03:22Z

Rebased due to conflicts.

TheBlueMatt

This basically looks good, I think, needs another reviewer.

wpaulino

Looks good! Since a user can now include arbitrary data, do we know if the router can handle restricting the number of hops based on the existing recipient payload?

TheBlueMatt · 2023-06-17T23:03:05Z

do we know if the router can handle restricting the number of hops based on the existing recipient payload?

It currently cannot, though this is an issue that was really introduced in payment metadata, this just makes it worse. Tracked at #2201.

TheBlueMatt · 2023-06-17T23:03:22Z

Feel free to squash fixups when you next push, I think.

TheBlueMatt · 2023-07-30T20:41:50Z

While we're still debating nits on the docs, can you go ahead and squash so we're set up to land? @wpaulino or @dunxen might want to take a look thereafter.

wpaulino

LGTM

alecchendev · 2023-08-01T02:27:57Z

Fixed and squashed immediately since they were just small nits

ariard · 2023-08-01T02:37:35Z

this is why blinded paths have no error returned at all.

yes, though i’m not sure no returning error is workable on the long-term due to onion bandwidth cost and payment reliability (at least end-to-end), though here more conversation at the spec-level.

answered on the two comments:

i still wish we do our best to mask user agent and version from deanonymization attacks like done for browser / tor services
i still think “authentication” should be a strong suggestion, seen example of multiple payer not compatible with current payment_secret

though overall i won’t insist and if / when we have users privacy screwed up on those vectors, i’ll just pointed it back to you guys :)

ariard · 2023-08-02T03:19:05Z

i still wish we do our best to mask user agent and version from deanonymization attacks like done for browser / tor services

here the code is good in fact after looking (though doc confusing), just good if we don’t change it in the future to match BOLT4 bad suggestion imho

alecchendev · 2023-08-03T17:33:33Z

Squashed immediately for minor doc change:

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 968b0765..daa0b34f 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -371,7 +371,7 @@ pub enum FailureCode {
/// We failed to process the payload after the onion was decrypted. You may wish to
/// use this when receiving custom HTLC TLVs with even type numbers that you don't recognize.
///
- /// If available, the tuple data should include the type number and byte offset in the+ /// If available, the tuple data may include the type number and byte offset in the
/// decrypted byte stream where the failure occurred.
InvalidOnionPayload(Option<(u64, u16)>),
}

ariard · 2023-08-04T00:19:34Z

Happy after following diff applied, payment_secret does not work for N-payer / N-payee flows, though it sounds reasonable for one payer / one payer flow, as far as I know about things like cancellable payments

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index daa0b34f..db1108b4 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -4769,6 +4769,8 @@ where
/// You MUST check you've understood all even TLVs before using this to
/// claim, otherwise you may unintentionally agree to some protocol you do not understand.
///
+ /// You MAY authenticate the even TLVs additionally of `payment_secret` if the protocol
+ /// flow is more sophisticated than one payer / one payee.
/// [`claim_funds`]: Self::claim_funds
pub fn claim_funds_with_known_custom_tlvs(&self, payment_preimage: PaymentPreimage) {
self.claim_payment_internal(payment_preimage, true);

ariard

Code Review ACK 1db481f

Nevermind the above comment effectively multi-payer scheme would need additional code changes in our MPP validation and as such wider custom HTLC support (though they’re the ones more exposed in term of potential deanonymization attacks)

ariard · 2023-08-07T23:54:21Z

+ FailureCode::InvalidOnionPayload(data) => {
+ let fail_data = match data {
+ Some((typ, offset)) => [BigSize(typ).encode(), offset.encode()].concat(),
+ None => Vec::new(),


nit: I think the debug_assert(data.is_none()) could be added due to its only usage in claim_payment_internal with a None value given to InvalidOnionPayload.

TheBlueMatt · 2023-08-08T20:21:54Z

Grrr, needs rebase, sorry about that.

Custom TLVs allow users to send extra application-specific data with a payment. These have the additional flexibility compared to `payment_metadata` that they don't have to reflect recipient generated data provided in an invoice, in which `payment_metadata` could be reused. We ensure provided type numbers are unique, increasing, and within the experimental range with the `RecipientOnionFields::with_custom_tlvs` method. This begins sender-side support for custom TLVs.

When serialized, the TLVs in `OutboundOnionPayload`, unlike a normal TLV stream, are prefixed with the length of the stream. To allow a user to add arbitrary custom TLVs, we aren't able to communicate to our serialization macros exactly which fields to expect, so this commit adds new macro variants to allow appending an extra set of bytes (and modifying the prefixed length accordingly). Because the keysend preimage TLV has a type number in the custom type range, and a user's TLVs may have type numbers above and/or below keysend's type number, and because TLV streams must be serialized in increasing order by type number, this commit also ensures the keysend TLV is properly sorted/serialized amongst the custom TLVs.

This completes basic receiver-side support for custom TLVs and adds functional testing for sending and receiving.

Upon receiving multiple payment parts with custom TLVs, we fail payments if they have any non-matching or missing even TLVs, and otherwise just drop non-matching TLVs if they're odd.

When a user decodes custom TLVs, if they fail to recognize even type numbers they should fail back with the correct failure code and fail data. This new variant adds the proper failure variant for the user to pass into `ChannelManager::fail_htlc_backwards_with_reason`. Note that the enum discriminants were removed because when adding a struct variant we can no longer make use of the discriminant through casting like we previously did, and instead have to manually define the associated failure code anyway.

Because we don't know which custom TLV type numbers the user is expecting (and it would be cumbersome for them to tell us), instead of failing unknown even custom TLVs on deserialization, we accept all custom TLVs, and pass them to the user to check whether they recognize them and choose to fail back if they don't. However, a user may not check for custom TLVs, in which case we should reject any even custom TLVs as unknown. This commit makes sure a user must explicitly accept a payment with even custom TLVs, by (1) making the default `ChannelManager::claim_funds` fail if the payment had even custom TLVs and (2) adding a new function `ChannelManager::claim_funds_with_known_custom_tlvs` that accepts them. This commit also refactors our custom TLVs test and updates various documentation to account for this.

alecchendev · 2023-08-08T21:19:23Z

Rebased

TheBlueMatt · 2023-08-10T16:23:19Z

+ }
+
+ #[cfg(debug_assertions)] {
+ let mut last_seen: Option<u64> = None;


it'd be nice to have this in one macro body with the above non-custom TLVs so that we can debug_assert!() that the ordering including custom TLVs is correct.

TheBlueMatt · 2023-08-10T16:38:36Z

+ let tlvs = &mut self.custom_tlvs;
+ let further_tlvs = &mut further_htlc_fields.custom_tlvs;
+
+ let even_tlvs: Vec<&(u64, Vec<u8>)> = tlvs.iter().filter(|(typ, _)| *typ % 2 == 0).collect();


You should be able to do the comparison here without collecting into a Vec.

TheBlueMatt · 2023-08-10T17:21:03Z

+ let preimage = if let Some(ref preimage) = keysend_preimage {
+ Some((5482373484, preimage.encode()))
+ } else { None };
+ let mut custom_tlvs: Vec<&(u64, Vec<u8>)> = custom_tlvs.iter().chain(preimage.iter()).collect();


We are trying to build an interator, we shouldn't have to collect here (though the macro may want it twice, so we may have to do the iter().chain() in the macro argument rather than here.

Though AFAIK it wouldn't be possible to sort the tlvs as an iterator?

TheBlueMatt · 2023-08-10T17:25:11Z

+ match &purpose {
+ PaymentPurpose::InvoicePayment { payment_secret, .. } => {
+ assert_eq!(our_payment_secret, *payment_secret);
+ assert_eq!(Some(*payment_secret), onion_fields.as_ref().unwrap().payment_secret);


nit: just for your reference, if we're copying test util bits into a test, we can usually skip a bunch of the assertions - the test utility methods tend to be a bit verbose and check everything, which is great in the general code, but in an individual test we don't need to test too much more than we care about for the purpose of the specific test.

TheBlueMatt · 2023-08-10T17:48:50Z

+	if let Some(expected_tlvs) = expected_receive_tlvs {
+ // Claim and match expected
+ let events = nodes[3].node.get_and_clear_pending_events();
+ println!("events: {:?}", events);


Oops, I missed this before hitting merge, let's do a followup and remove this (and maybe hit a few other nits, though no real pressure on those).

alecchendev · 2023-08-10T18:59:58Z

Thanks for all the review everybody, will get a follow up up soon :)

alecchendev · 2023-05-19T22:32:52Z

Closes#1298. This PR adds support for sending and receiving custom HTLC TLVs.

Custom TLVs allow users to send extra application-specific data with a payment. These have the additional flexibility compared to payment_metadata that they don't have to reflect recipient generated data provided in an invoice, in which payment_metadata could (probably) be reused.

On the send side, a user can provide their serialized TLVs as a Vec<(u64, Vec<u8>)> to RecipientOnionFields::with_custom_tlvs which checks whether the type numbers are unique, increasing, and in the range reserved for custom values. They'll then pass this into whichever send payment function they use, it'll be serialized in the onion payload, and sent with the payment.

On the receive side, when deserializing the onion payload, we just add the bytes back into this type, then pipe it from the onion through to Event::PaymentClaimable, dropping non-matching TLVs between payment parts.

alecchendev

A couple questions for reviewers:

codecov-commenter · 2023-05-20T23:31:56Z

Codecov Report

Patch coverage: 95.32% and project coverage change: +0.34% 🎉

Comparison is base (e13ff10) 90.24% compared to head (93a8fb6) 90.58%.
Report is 21 commits behind head on main.

❗ Current head 93a8fb6 differs from pull request most recent head 1db481f. Consider uploading reports for the commit 1db481f to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@ Coverage Diff @@## main #2308 +/- ##
==========================================
+ Coverage 90.24% 90.58% +0.34% 
==========================================
Files 106 106 Lines 55817 59617 +3800 Branches 55817 59617 +3800 ==========================================
+ Hits 50370 54007 +3637 - Misses 5447 5610 +163

Files Changed	Coverage Δ
lightning/src/events/mod.rs	`41.98% <ø> (+0.37%)`	⬆️
lightning/src/util/ser_macros.rs	`67.75% <0.00%> (-2.81%)`	⬇️
lightning/src/ln/onion_route_tests.rs	`98.51% <85.71%> (+0.01%)`	⬆️
lightning/src/ln/payment_tests.rs	`97.38% <95.04%> (-0.28%)`	⬇️
lightning/src/ln/msgs.rs	`85.10% <96.55%> (+0.36%)`	⬆️
lightning/src/ln/channelmanager.rs	`86.73% <97.91%> (+1.28%)`	⬆️
lightning-invoice/src/payment.rs	`88.59% <100.00%> (-0.08%)`	⬇️
lightning/src/ln/functional_test_utils.rs	`88.93% <100.00%> (+0.01%)`	⬆️
lightning/src/ln/onion_utils.rs	`90.90% <100.00%> (-0.16%)`	⬇️
lightning/src/ln/outbound_payment.rs	`92.77% <100.00%> (+3.57%)`	⬆️

... and 4 files with indirect coverage changes

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

TheBlueMatt

Sorry for the delay here, just getting caught up post-travels.

alecchendev · 2023-06-10T23:03:22Z

Rebased due to conflicts.

TheBlueMatt

This basically looks good, I think, needs another reviewer.

wpaulino

Looks good! Since a user can now include arbitrary data, do we know if the router can handle restricting the number of hops based on the existing recipient payload?

TheBlueMatt · 2023-06-17T23:03:05Z

do we know if the router can handle restricting the number of hops based on the existing recipient payload?

It currently cannot, though this is an issue that was really introduced in payment metadata, this just makes it worse. Tracked at #2201.

TheBlueMatt · 2023-06-17T23:03:22Z

Feel free to squash fixups when you next push, I think.

TheBlueMatt · 2023-07-30T20:41:50Z

While we're still debating nits on the docs, can you go ahead and squash so we're set up to land? @wpaulino or @dunxen might want to take a look thereafter.

wpaulino

LGTM

alecchendev · 2023-08-01T02:27:57Z

Fixed and squashed immediately since they were just small nits

ariard · 2023-08-01T02:37:35Z

this is why blinded paths have no error returned at all.

yes, though i’m not sure no returning error is workable on the long-term due to onion bandwidth cost and payment reliability (at least end-to-end), though here more conversation at the spec-level.

answered on the two comments:

i still wish we do our best to mask user agent and version from deanonymization attacks like done for browser / tor services
i still think “authentication” should be a strong suggestion, seen example of multiple payer not compatible with current payment_secret

though overall i won’t insist and if / when we have users privacy screwed up on those vectors, i’ll just pointed it back to you guys :)

ariard · 2023-08-02T03:19:05Z

i still wish we do our best to mask user agent and version from deanonymization attacks like done for browser / tor services

here the code is good in fact after looking (though doc confusing), just good if we don’t change it in the future to match BOLT4 bad suggestion imho

alecchendev · 2023-08-03T17:33:33Z

Squashed immediately for minor doc change:

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 968b0765..daa0b34f 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -371,7 +371,7 @@ pub enum FailureCode {
/// We failed to process the payload after the onion was decrypted. You may wish to
/// use this when receiving custom HTLC TLVs with even type numbers that you don't recognize.
///
- /// If available, the tuple data should include the type number and byte offset in the+ /// If available, the tuple data may include the type number and byte offset in the
/// decrypted byte stream where the failure occurred.
InvalidOnionPayload(Option<(u64, u16)>),
}

ariard · 2023-08-04T00:19:34Z

Happy after following diff applied, payment_secret does not work for N-payer / N-payee flows, though it sounds reasonable for one payer / one payer flow, as far as I know about things like cancellable payments

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index daa0b34f..db1108b4 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -4769,6 +4769,8 @@ where
/// You MUST check you've understood all even TLVs before using this to
/// claim, otherwise you may unintentionally agree to some protocol you do not understand.
///
+ /// You MAY authenticate the even TLVs additionally of `payment_secret` if the protocol
+ /// flow is more sophisticated than one payer / one payee.
/// [`claim_funds`]: Self::claim_funds
pub fn claim_funds_with_known_custom_tlvs(&self, payment_preimage: PaymentPreimage) {
self.claim_payment_internal(payment_preimage, true);

ariard

Code Review ACK 1db481f

Nevermind the above comment effectively multi-payer scheme would need additional code changes in our MPP validation and as such wider custom HTLC support (though they’re the ones more exposed in term of potential deanonymization attacks)

ariard · 2023-08-07T23:54:21Z

+ FailureCode::InvalidOnionPayload(data) => {
+ let fail_data = match data {
+ Some((typ, offset)) => [BigSize(typ).encode(), offset.encode()].concat(),
+ None => Vec::new(),


nit: I think the debug_assert(data.is_none()) could be added due to its only usage in claim_payment_internal with a None value given to InvalidOnionPayload.

TheBlueMatt · 2023-08-08T20:21:54Z

Grrr, needs rebase, sorry about that.

Custom TLVs allow users to send extra application-specific data with a payment. These have the additional flexibility compared to `payment_metadata` that they don't have to reflect recipient generated data provided in an invoice, in which `payment_metadata` could be reused. We ensure provided type numbers are unique, increasing, and within the experimental range with the `RecipientOnionFields::with_custom_tlvs` method. This begins sender-side support for custom TLVs.

When serialized, the TLVs in `OutboundOnionPayload`, unlike a normal TLV stream, are prefixed with the length of the stream. To allow a user to add arbitrary custom TLVs, we aren't able to communicate to our serialization macros exactly which fields to expect, so this commit adds new macro variants to allow appending an extra set of bytes (and modifying the prefixed length accordingly). Because the keysend preimage TLV has a type number in the custom type range, and a user's TLVs may have type numbers above and/or below keysend's type number, and because TLV streams must be serialized in increasing order by type number, this commit also ensures the keysend TLV is properly sorted/serialized amongst the custom TLVs.

This completes basic receiver-side support for custom TLVs and adds functional testing for sending and receiving.

Upon receiving multiple payment parts with custom TLVs, we fail payments if they have any non-matching or missing even TLVs, and otherwise just drop non-matching TLVs if they're odd.

When a user decodes custom TLVs, if they fail to recognize even type numbers they should fail back with the correct failure code and fail data. This new variant adds the proper failure variant for the user to pass into `ChannelManager::fail_htlc_backwards_with_reason`. Note that the enum discriminants were removed because when adding a struct variant we can no longer make use of the discriminant through casting like we previously did, and instead have to manually define the associated failure code anyway.

Because we don't know which custom TLV type numbers the user is expecting (and it would be cumbersome for them to tell us), instead of failing unknown even custom TLVs on deserialization, we accept all custom TLVs, and pass them to the user to check whether they recognize them and choose to fail back if they don't. However, a user may not check for custom TLVs, in which case we should reject any even custom TLVs as unknown. This commit makes sure a user must explicitly accept a payment with even custom TLVs, by (1) making the default `ChannelManager::claim_funds` fail if the payment had even custom TLVs and (2) adding a new function `ChannelManager::claim_funds_with_known_custom_tlvs` that accepts them. This commit also refactors our custom TLVs test and updates various documentation to account for this.

alecchendev · 2023-08-08T21:19:23Z

Rebased

TheBlueMatt · 2023-08-10T16:23:19Z

+ }
+
+ #[cfg(debug_assertions)] {
+ let mut last_seen: Option<u64> = None;


it'd be nice to have this in one macro body with the above non-custom TLVs so that we can debug_assert!() that the ordering including custom TLVs is correct.

TheBlueMatt · 2023-08-10T16:38:36Z

+ let tlvs = &mut self.custom_tlvs;
+ let further_tlvs = &mut further_htlc_fields.custom_tlvs;
+
+ let even_tlvs: Vec<&(u64, Vec<u8>)> = tlvs.iter().filter(|(typ, _)| *typ % 2 == 0).collect();


You should be able to do the comparison here without collecting into a Vec.

TheBlueMatt · 2023-08-10T17:21:03Z

+ let preimage = if let Some(ref preimage) = keysend_preimage {
+ Some((5482373484, preimage.encode()))
+ } else { None };
+ let mut custom_tlvs: Vec<&(u64, Vec<u8>)> = custom_tlvs.iter().chain(preimage.iter()).collect();


We are trying to build an interator, we shouldn't have to collect here (though the macro may want it twice, so we may have to do the iter().chain() in the macro argument rather than here.

Though AFAIK it wouldn't be possible to sort the tlvs as an iterator?

TheBlueMatt · 2023-08-10T17:25:11Z

+ match &purpose {
+ PaymentPurpose::InvoicePayment { payment_secret, .. } => {
+ assert_eq!(our_payment_secret, *payment_secret);
+ assert_eq!(Some(*payment_secret), onion_fields.as_ref().unwrap().payment_secret);


nit: just for your reference, if we're copying test util bits into a test, we can usually skip a bunch of the assertions - the test utility methods tend to be a bit verbose and check everything, which is great in the general code, but in an individual test we don't need to test too much more than we care about for the purpose of the specific test.

TheBlueMatt · 2023-08-10T17:48:50Z

+	if let Some(expected_tlvs) = expected_receive_tlvs {
+ // Claim and match expected
+ let events = nodes[3].node.get_and_clear_pending_events();
+ println!("events: {:?}", events);


Oops, I missed this before hitting merge, let's do a followup and remove this (and maybe hit a few other nits, though no real pressure on those).

alecchendev · 2023-08-10T18:59:58Z

Thanks for all the review everybody, will get a follow up up soon :)

alecchendev commented May 19, 2023
View reviewed changes

Comment threadlightning/src/offers/merkle.rs Outdated
Comment threadlightning/src/offers/merkle.rs Outdated
Comment threadlightning/src/ln/msgs.rs Outdated

alecchendev force-pushed the 2023-05-custom-htlc-tlvs branch 2 times, most recently from 87ec6b0 to 4cb0a4bCompare May 20, 2023 21:49

alecchendev commented May 20, 2023
View reviewed changes

Comment threadlightning/src/ln/msgs.rs

alecchendev force-pushed the 2023-05-custom-htlc-tlvs branch from 4cb0a4b to 07a9910Compare May 20, 2023 22:22

wpaulino self-requested a review May 21, 2023 02:11

dunxen self-requested a review May 26, 2023 07:05

TheBlueMatt reviewed May 30, 2023
View reviewed changes

Comment threadlightning/src/ln/outbound_payment.rs Outdated
Comment threadlightning/src/ln/msgs.rs
Comment threadlightning/src/ln/msgs.rs Outdated
Comment threadlightning/src/offers/merkle.rs Outdated

alecchendev force-pushed the 2023-05-custom-htlc-tlvs branch from 07a9910 to 3d848a6Compare June 2, 2023 04:00

TheBlueMatt reviewed Jun 3, 2023
View reviewed changes

Comment threadlightning/src/ln/msgs.rs Outdated
Comment threadlightning/src/ln/outbound_payment.rs Outdated
Comment threadlightning/src/ln/msgs.rs
Comment threadlightning/src/ln/outbound_payment.rs Outdated

alecchendev force-pushed the 2023-05-custom-htlc-tlvs branch 4 times, most recently from 3483289 to 59f3fb6Compare June 8, 2023 18:53

alecchendev commented Jun 8, 2023
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/msgs.rs

alecchendev marked this pull request as ready for review June 8, 2023 18:59

alecchendev force-pushed the 2023-05-custom-htlc-tlvs branch 2 times, most recently from a7a2162 to ad4376eCompare June 10, 2023 23:02

TheBlueMatt reviewed Jun 12, 2023
View reviewed changes

alecchendev force-pushed the 2023-05-custom-htlc-tlvs branch 3 times, most recently from 751c245 to a0d10a3Compare June 14, 2023 05:44

wpaulino reviewed Jun 16, 2023
View reviewed changes

Comment threadlightning/src/util/ser_macros.rs Outdated

alecchendev force-pushed the 2023-05-custom-htlc-tlvs branch from a0d10a3 to fdbcbffCompare June 20, 2023 23:35

wpaulino added this to the 0.0.117 milestone Jul 5, 2023

TheBlueMatt added the blocked on next release Should Wait Until Next Release To Land label Jul 8, 2023

alecchendev force-pushed the 2023-05-custom-htlc-tlvs branch from 93a8fb6 to e8eedd3Compare July 31, 2023 02:03

wpaulino reviewed Jul 31, 2023
View reviewed changes

Comment threadlightning/src/ln/outbound_payment.rs Outdated
Comment threadlightning/src/ln/outbound_payment.rs Outdated

alecchendev force-pushed the 2023-05-custom-htlc-tlvs branch from e8eedd3 to 9a77843Compare August 1, 2023 02:26

alecchendev force-pushed the 2023-05-custom-htlc-tlvs branch from 9a77843 to 1db481fCompare August 3, 2023 17:33

wpaulino previously approved these changes Aug 4, 2023
View reviewed changes

ariard approved these changes Aug 8, 2023
View reviewed changes

alecchendev added 7 commits August 8, 2023 15:55

Add custom tlvs to PendingOutboundPayment::Retryable
d2e9cb4

Handle receiving custom HTLC TLVs
0a2dbdf
This completes basic receiver-side support for custom TLVs and adds functional testing for sending and receiving.

Drop non-matching custom TLVs when receiving MPP
e84fb06
Upon receiving multiple payment parts with custom TLVs, we fail payments if they have any non-matching or missing even TLVs, and otherwise just drop non-matching TLVs if they're odd.

alecchendev dismissed wpaulino’s stale review via dec3fb3August 8, 2023 21:18

alecchendev force-pushed the 2023-05-custom-htlc-tlvs branch from 1db481f to dec3fb3Compare August 8, 2023 21:18

wpaulino approved these changes Aug 9, 2023
View reviewed changes

dunxen approved these changes Aug 10, 2023
View reviewed changes

TheBlueMatt approved these changes Aug 10, 2023
View reviewed changes

TheBlueMatt merged commit 9e4a35a into lightningdevkit:mainAug 10, 2023

TheBlueMatt reviewed Aug 10, 2023
View reviewed changes

alecchendev mentioned this pull request Aug 16, 2023
Followup: custom HTLC TLVs #2504
Merged