Support sending `PaymentMetadata` in HTLCs #2127

TheBlueMatt · 2023-03-24T17:52:39Z

This adds all the required infrastructure to send PaymentMetadata in HTLCs, largely as a rebase of #1445 (with commit ordering shuffled around so that the new struct is added first - I ended up rewriting that commit as it makes sense to go first, rather than strictly rebasing the existing work).

While this does deserialize the new field in the onion, it doesn't actually store it as HTLCs are received and expose it to users.

By switching the payment secret public API parameters to a new struct we also have more space to grow when we add the ability to have custom TLVs in the onion.

alecchendev

Nice! Had some nits and questions

alecchendev · 2023-03-28T01:33:56Z

lightning-invoice/src/lib.rs

 ///
 /// (C-not exported) as we likely need to manually select one set of boolean type parameters.
 #[derive(Eq, PartialEq, Debug, Clone)]
-pub struct InvoiceBuilder<D: tb::Bool, H: tb::Bool, T: tb::Bool, C: tb::Bool, S: tb::Bool> {
+pub struct InvoiceBuilder<D: tb::Bool, H: tb::Bool, T: tb::Bool, C: tb::Bool, S: tb::Bool, M: tb::Bool> {


Woah, this is the first time I'm really looking at this part of the repo, this is so cool. Just to make sure I'm getting things right, these type parameters are here to basically conditionally implement/expose functions to the user based on the state of the invoice (what fields have been added) (such that a user's error will be caught at compile time), and then the PhantomData is there to just use the types because rust doesn't allow unused type params?

alecchendev · 2023-03-28T01:34:03Z

lightning-invoice/src/lib.rs

 		self.set_flags()
 	}
 }

-impl<D: tb::Bool, H: tb::Bool, T: tb::Bool, C: tb::Bool> InvoiceBuilder<D, H, T, C, tb::True> {
+impl<D: tb::Bool, H: tb::Bool, T: tb::Bool, C: tb::Bool, S: tb::Bool> InvoiceBuilder<D, H, T, C, S, tb::True> {
+	/// Sets the payment secret and relevant features.


Comment needs to be changed :)

^ This wasn't resolved in the right commit

lightning/src/ln/features.rs

alecchendev · 2023-03-28T01:34:48Z

lightning/src/ln/msgs.rs

 			OnionHopDataFormat::NonFinalNode {
 				short_channel_id,
 			}
 		} else {
-			if let &Some(ref data) = &payment_data {
+			if let Some(data) = &mut payment_data {


I think the mut may be unnecessary here?

Huh, funny that didn't generate a compilation warning.

codecov-commenter · 2023-03-28T22:54:21Z

Codecov Report

Patch coverage: 92.44% and project coverage change: +0.68 🎉

Comparison is base (1ceb41e) 91.40% compared to head (ef8e377) 92.08%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2127      +/-   ##
==========================================
+ Coverage   91.40%   92.08%   +0.68%     
==========================================
  Files         102      102              
  Lines       50306    58635    +8329     
  Branches    50306    58635    +8329     
==========================================
+ Hits        45982    53996    +8014     
- Misses       4324     4639     +315

Impacted Files	Coverage Δ
lightning-invoice/src/de.rs	`78.14% <0.00%> (+2.32%)`	⬆️
lightning/src/events/mod.rs	`35.82% <66.66%> (+2.14%)`	⬆️
lightning/src/ln/channelmanager.rs	`91.76% <84.15%> (+2.40%)`	⬆️
lightning-invoice/src/lib.rs	`84.46% <88.70%> (+4.87%)`	⬆️
lightning/src/ln/outbound_payment.rs	`92.60% <92.00%> (+1.91%)`	⬆️
lightning-invoice/src/payment.rs	`89.11% <96.96%> (+6.09%)`	⬆️
lightning-invoice/src/ser.rs	`93.33% <100.00%> (+0.89%)`	⬆️
lightning/src/ln/features.rs	`99.05% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`93.96% <100.00%> (+1.49%)`	⬆️
lightning/src/ln/msgs.rs	`83.99% <100.00%> (-0.11%)`	⬇️
... and 2 more

... and 20 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

lightning/src/ln/channelmanager.rs

valentinewallace · 2023-03-29T00:07:03Z

lightning/src/ln/chanmon_update_fail_tests.rs

+			RecipientOnionFields::secret_only(payment_secret_1), PaymentId(payment_hash_1.0)),
+		true, APIError::ChannelUnavailable {..}, {});


nit: weird indentation and below

Yea, I was trying to find something compact that also showed which parameters went with which method? I guess should have moved the closing ) to the next line, would that address this? Its repeated in a bunch of places so only want to fix it once.

valentinewallace · 2023-03-29T01:30:28Z

lightning/src/ln/channelmanager.rs

@@ -2711,14 +2744,14 @@ where
 	/// Note that `route` must have exactly one path.
 	///
 	/// [`send_payment`]: Self::send_payment
-	pub fn send_spontaneous_payment(&self, route: &Route, payment_preimage: Option<PaymentPreimage>, payment_id: PaymentId) -> Result<PaymentHash, PaymentSendFailure> {
+	pub fn send_spontaneous_payment(&self, route: &Route, payment_preimage: Option<PaymentPreimage>, recipient_onion: RecipientOnionFields, payment_id: PaymentId) -> Result<PaymentHash, PaymentSendFailure> {


I think we should bring back the invoice util for spontaneous payments, tbh

I mean....its still a one-line util to call a method by passing two Nones in...if we want a util we can just add a second method here that calls the first, but I'm not super convinced...

lightning-invoice/src/payment.rs

lightning/src/ln/channelmanager.rs

valentinewallace

This is looking basically good to me. Will give it another look after a 2nd reviewer comes in

lightning/src/ln/outbound_payment.rs

valentinewallace · 2023-03-29T21:47:54Z

lightning/src/ln/channelmanager.rs

+	/// receives, thus you should generally never be providing a secret here for spontaneous
+	/// payments.


Note that apparently lnd will reject keysends with payment secrets: ACINQ/eclair#2574

Right, should I indicate more strongly in the docs here that it'll be rejected (independent of the MPP context)?

valentinewallace · 2023-03-29T22:08:21Z

pending_changelog/matt-rm-retryable-secret.txt

+ * Payments sent with the legacy `*_with_route` methods on LDK 0.0.115+ will no
+   longer be retryable via the LDK 0.0.114- `retry_payment` method (#XXXX).


Is this just because we might be re-attempting a payment with payment_metadata as a required feature? I see we add an even TLV in this PR but it's only for the onion payload...

I was thinking we'd fail because the PaymentSecret was ripped out of the HTLCSource::OutboundRoute?

Ah that makes sense... Looks like we'll allow retry if it's not an MPP, but obviously the receiver will then fail it

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2023-03-29T23:52:05Z

Gonna reorder the commits here to put the refactor bulk work into another PR and then pick this back up after, maybe with receive too.

TheBlueMatt · 2023-04-07T20:59:39Z

Rebased after the dependent PR merge and added the receive end of thing. Still need to add some testing but otherwise should be good.

alecchendev

LGTM - the receive validation refactoring looks great 👍

TheBlueMatt · 2023-04-10T23:26:04Z

Okay, pushed some tests and even a doc cleanup.

dunxen

Did a first pass, but will do a more thorough one after poking around spec/current code for some more context.

dunxen · 2023-04-13T20:20:07Z

lightning/src/ln/features.rs

@@ -278,6 +278,7 @@ mod sealed {
 					}

 					flags[Self::BYTE_OFFSET] |= Self::REQUIRED_MASK;
+					flags[Self::BYTE_OFFSET] &= !Self::OPTIONAL_MASK;
 				}

 				/// Sets the feature's optional (odd) bit in the given flags.


So related question: we could technically get back to a case where both are set by calling set_optional_bit with flags. Do we care or just assume it cannot override and unset the required bit? Seems like a misuse of the methods by the caller of course, but just wondering.

I don't think we care particularly - afaik there's nothing that says we cant set both bits, it just seems like we don't need to, and mostly it made testing easier.

tnull

Looks good from a first pass, just some comments/nits.

lightning/src/ln/features.rs

lightning-invoice/src/lib.rs

tnull · 2023-04-17T13:52:21Z

lightning-invoice/src/lib.rs

+	/// Sets the payment metadata.
+	///
+	/// By default features are set to *optionally* allow the sender to include the payment metadata.
+	/// If you wish to require that the sender include the metadata (and fail to parse the invoice if


Suggested change

/// If you wish to require that the sender include the metadata (and fail to parse the invoice if

/// If you wish to require that senders include the metadata (and fail to parse the invoice if

But there's only one sender?

Alright, then it should probably be "...that the sender includes the metadata (and fails to parse the invoice if..."

lightning/src/ln/channelmanager.rs

valentinewallace · 2023-04-12T15:36:19Z

lightning-invoice/src/lib.rs

 		self.set_flags()
 	}
 }

-impl<D: tb::Bool, H: tb::Bool, T: tb::Bool, C: tb::Bool> InvoiceBuilder<D, H, T, C, tb::True> {
+impl<D: tb::Bool, H: tb::Bool, T: tb::Bool, C: tb::Bool, S: tb::Bool> InvoiceBuilder<D, H, T, C, S, tb::True> {
+	/// Sets the payment secret and relevant features.


^ This wasn't resolved in the right commit

lightning/src/ln/features.rs

valentinewallace · 2023-04-17T20:40:58Z

lightning/src/ln/features.rs

@@ -278,6 +278,7 @@ mod sealed {
 					}

 					flags[Self::BYTE_OFFSET] |= Self::REQUIRED_MASK;
+					flags[Self::BYTE_OFFSET] &= !Self::OPTIONAL_MASK;


Should we also unset the required bit when setting the optional bit?

Maybe? Let's consider that in #2193, this change is really just for testing.

valentinewallace · 2023-04-18T13:41:16Z

lightning/src/ln/channelmanager.rs

+			} else {
+				for (purpose, (payment_hash, htlcs)) in purposes.into_iter().zip(claimable_htlcs_list.into_iter()) {
+					let existing_payment = claimable_payments.insert(payment_hash, ClaimablePayment {
+						purpose, htlcs, onion_fields: None,


Could we fill this in using the purpose? Not sure it has to be Optional

We can, yes, but I don't think we should. onion_fields being Some indicates that we understood but didn't have a metadata, and maybe in the future TLV fields. We don't support retrying these anyway so I kinda preferred to leave it None.

valentinewallace · 2023-04-18T14:16:49Z

lightning/src/events/mod.rs

+		/// identical in each HTLC involved in the payment will be included here.
+		///
+		/// Payments received on LDK versions prior to 0.0.115 will have this field unset.
+		onion_fields: Option<RecipientOnionFields>,


Seems like we could get rid of PaymentPurpose::Invoice::payment_secret if we wanted to avoid including it redundantly

Yea, I started to do that but we'd have to break out the TLV serialization and kinda figured it wasn't worth it. We can do it later, too, there's no real reason to do it now given the size of this PR anyway.

valentinewallace

Just some nits. LGTM after squash

lightning/src/ln/channelmanager.rs

There is no reason to set both, and this currently makes testing the new BOLT invoice tests slightly harder, so we just unset it.

This adds support for reading the new `PaymentMetadata` BOLT11 invoice field, giving us access to the `Vec<u8>` storing arbitrary bytes we have to send to the recipient.

This adds support for setting the new payment metadata field in BOLT11 invoices, using a new type flag on the builder to enforce transition correctness. We allow users to set the payment metadata as either optional or required, defaulting to optional so that invoice parsing does not fail if the sender does not support payment metadata fields.

This adds the new `payment_metadata` to `RecipientOnionFields`, passing the metadata from BOLT11 invoices through the send pipeline and finally copying them info the onion when sending HTLCs. This completes send-side support for the new payment metadata feature.

This makes the `claimable_payments` code more upgradable allowing us to add new fields in the coming commit(s).

TheBlueMatt · 2023-04-19T03:05:39Z

Squashed all the fixup commits down, with new comments and a test failure addressed as well:

$ git diff-tree -U1 35800208 c5d52ceb
diff --git a/lightning-invoice/src/payment.rs b/lightning-invoice/src/payment.rs
index 0adc83d1a..11757be2e 100644
--- a/lightning-invoice/src/payment.rs
+++ b/lightning-invoice/src/payment.rs
@@ -358,2 +358,3 @@ mod tests {
 	#[test]
+	#[cfg(feature = "std")]
 	fn payment_metadata_end_to_end() {
@@ -376,3 +377,3 @@ mod tests {
 			.payment_secret(payment_secret)
-			.duration_since_epoch(duration_since_epoch())
+			.current_timestamp()
 			.min_final_cltv_expiry_delta(144)
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 9a6dd117c..0a65d788f 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -7751,3 +7751,2 @@ where
 				}
-
 			} else {
@@ -7767,6 +7766,6 @@ where
 				}
-				let (purpose, onion_fields) = match &htlcs[0].onion_payload {
+				let purpose = match &htlcs[0].onion_payload {
 					OnionPayload::Invoice { _legacy_hop_data } => {
 						if let Some(hop_data) = _legacy_hop_data {
-							(events::PaymentPurpose::InvoicePayment {
+							events::PaymentPurpose::InvoicePayment {
 								payment_preimage: match pending_inbound_payments.get(&payment_hash) {
@@ -7782,3 +7781,3 @@ where
 								payment_secret: hop_data.payment_secret,
-							}, None)
+							}
 						} else { return Err(DecodeError::InvalidValue); }
@@ -7786,6 +7785,6 @@ where
 					OnionPayload::Spontaneous(payment_preimage) =>
-						(events::PaymentPurpose::SpontaneousPayment(*payment_preimage), None),
+						events::PaymentPurpose::SpontaneousPayment(*payment_preimage),
 				};
 				claimable_payments.insert(payment_hash, ClaimablePayment {
-					purpose, htlcs, onion_fields,
+					purpose, htlcs, onion_fields: None,
 				});

benthecarman · 2023-04-19T09:12:33Z

lightning/src/ln/outbound_payment.rs

+	}
+
+
+


nit: extra spacing here

tnull

LGTM, happy for outstanding comments to be addressed in follow-ups.

tnull · 2023-04-19T12:58:26Z

lightning-invoice/tests/ser_de.rs

@@ -332,6 +332,56 @@ fn get_test_tuples() -> Vec<(String, SignedRawInvoice, bool, bool)> {
 			true, // Different features than set in InvoiceBuilder
 			true, // Some unknown fields
 		),
+		( // Older version of the payment metadata test with a payment_pubkey set
+			"lnbc10m1pvjluezpp5qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqypqdp9wpshjmt9de6zqmt9w3skgct5vysxjmnnd9jx2mq8q8a04uqnp4q0n326hr8v9zprg8gsvezcch06gfaqqhde2aj730yg0durunfhv66sp5zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zygs9q2gqqqqqqsgqy9gw6ymamd20jumvdgpfphkhp8fzhhdhycw36egcmla5vlrtrmhs9t7psfy3hkkdqzm9eq64fjg558znccds5nhsfmxveha5xe0dykgpspdha0".to_owned(),


nit: Could linebreak the strings here and below.

Not sure what you mean? You mean break the strings into a few lines via concat!()? We haven't done it elsewhere in the file, and not sure that's really nicer?

lightning/src/ln/channelmanager.rs

tnull · 2023-04-19T13:39:39Z

lightning-invoice/src/lib.rs

+	/// they don't support payment metadata fields), you need to call
+	/// [`InvoiceBuilder::require_payment_metadata`] after this.
+	pub fn payment_metadata(mut self, payment_metadata: Vec<u8>) -> InvoiceBuilder<D, H, T, C, S, tb::True> {
+		self.tagged_fields.push(TaggedField::PaymentMetadata(payment_metadata));


In a follow-up we might want to look into: Do we need/want to limit the size of the payment metadata field? How can we communicate the influence it might have on the maximum path length?

We'll need to rework all that code for blinded paths too, indeed let's do it in a followup - #2201

If we add an entry to `claimable_payments` we have to ensure we actually accept the HTLC we're considering, otherwise we'll end up with an empty `claimable_payments` entry.

If we receive an HTLC and are processing it a potential MPP part, we always continue in the per-HTLC loop if we call the `fail_htlc` macro, thus its nice to actually do the `continue` therein rather than at the callsites.

When we receive an HTLC, we want to pass the `payment_metadata` through to the `PaymentClaimable` event. This does most of the internal refactoring required to do so - storing a `RecipientOnionFields` in the inbound HTLC tracking structs, including the `payment_metadata`. In the future this struct will allow us to do MPP keysend receipts (as it now stores an Optional `payment_secret` for all inbound payments) as well as custom TLV receipts (as the struct is extensible to store additional fields and the internal API supports filtering for fields which are consistent across HTLCs).

This finally completes the piping of the `payment_metadata` from from the BOLT11 invoice on the sending side all the way through the onion sending + receiving ends to the user on the receive events.

TheBlueMatt · 2023-04-19T14:56:20Z

Added one commit for a new debug_assertion, and dropped some excess \n while at it:

$ git diff-tree -U1 c5d52ceb ef8e3770
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 0a65d788f..4bb830f0c 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -3333,4 +3333,7 @@ where
 
+								let mut committed_to_claimable = false;
+
 								macro_rules! fail_htlc {
 									($htlc: expr, $payment_hash: expr) => {
+										debug_assert!(!committed_to_claimable);
 										let mut htlc_msat_height_data = $htlc.value.to_be_bytes().to_vec();
@@ -3375,4 +3378,7 @@ where
 											// Note that if we insert here we MUST NOT fail_htlc!()
-											.or_insert_with(|| ClaimablePayment {
-												purpose: purpose(), htlcs: Vec::new(), onion_fields: None,
+											.or_insert_with(|| {
+												committed_to_claimable = true;
+												ClaimablePayment {
+													purpose: purpose(), htlcs: Vec::new(), onion_fields: None,
+												}
 											});
@@ -3418,2 +3424,5 @@ where
 										} else if total_value >= $payment_data.total_msat {
+											#[allow(unused_assignments)] {
+												committed_to_claimable = true;
+											}
 											let prev_channel_id = prev_funding_outpoint.to_channel_id();
@@ -3438,2 +3447,5 @@ where
 											htlcs.push(claimable_htlc);
+											#[allow(unused_assignments)] {
+												committed_to_claimable = true;
+											}
 										}
diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs
index 0343202cc..d41c62d1d 100644
--- a/lightning/src/ln/outbound_payment.rs
+++ b/lightning/src/ln/outbound_payment.rs
@@ -929,4 +929,2 @@ impl OutboundPayments {
 
-
-
 	#[cfg(test)]

0.0.115 - Apr 24, 2023 - "Rebroadcast the Bugfixes" API Updates =========== * The MSRV of the main LDK crates has been increased to 1.48 (lightningdevkit#2107). * Attempting to claim an un-expired payment on a channel which has closed no longer fails. The expiry time of payments is exposed via `PaymentClaimable::claim_deadline` (lightningdevkit#2148). * `payment_metadata` is now supported in `Invoice` deserialization, sending, and receiving (via a new `RecipientOnionFields` struct) (lightningdevkit#2139, lightningdevkit#2127). * `Event::PaymentFailed` now exposes a failure reason (lightningdevkit#2142). * BOLT12 messages now support stateless generation and validation (lightningdevkit#1989). * The `NetworkGraph` is now pruned of stale data after RGS processing (lightningdevkit#2161). * Max inbound HTLCs in-flight can be changed in the handshake config (lightningdevkit#2138). * `lightning-transaction-sync` feature `esplora-async-https` was added (lightningdevkit#2085). * A `ChannelPending` event is now emitted after the initial handshake (lightningdevkit#2098). * `PaymentForwarded::outbound_amount_forwarded_msat` was added (lightningdevkit#2136). * `ChannelManager::list_channels_by_counterparty` was added (lightningdevkit#2079). * `ChannelDetails::feerate_sat_per_1000_weight` was added (lightningdevkit#2094). * `Invoice::fallback_addresses` was added to fetch `bitcoin` types (lightningdevkit#2023). * The offer/refund description is now exposed in `Invoice{,Request}` (lightningdevkit#2206). Backwards Compatibility ======================= * Payments sent with the legacy `*_with_route` methods on LDK 0.0.115+ will no longer be retryable via the LDK 0.0.114- `retry_payment` method (lightningdevkit#2139). * `Event::PaymentPathFailed::retry` was removed and will always be `None` for payments initiated on 0.0.115 which fail on an earlier version (lightningdevkit#2063). * `Route`s and `PaymentParameters` with blinded path information will not be readable on prior versions of LDK. Such objects are not currently constructed by LDK, but may be when processing BOLT12 data in a coming release (lightningdevkit#2146). * Providing `ChannelMonitorUpdate`s generated by LDK 0.0.115 to a `ChannelMonitor` on 0.0.114 or before may panic (lightningdevkit#2059). Note that this is in general unsupported, and included here only for completeness. Bug Fixes ========= * Fixed a case where `process_events_async` may `poll` a `Future` which has already completed (lightningdevkit#2081). * Fixed deserialization of `u16` arrays. This bug may have previously corrupted the historical buckets in a `ProbabilisticScorer`. Users relying on the historical buckets may wish to wipe their scorer on upgrade to remove corrupt data rather than waiting on it to decay (lightningdevkit#2191). * The `process_events_async` task is now `Send` and can thus be polled on a multi-threaded runtime (lightningdevkit#2199). * Fixed a missing macro export causing `impl_writeable_tlv_based_enum{,_upgradable}` calls to not compile (lightningdevkit#2091). * Fixed compilation of `lightning-invoice` with both `no-std` and serde (lightningdevkit#2187) * Fix an issue where the `background-processor` would not wake when a `ChannelMonitorUpdate` completed asynchronously, causing delays (lightningdevkit#2090). * Fix an issue where `process_events_async` would exit immediately (lightningdevkit#2145). * `Router` calls from the `ChannelManager` now call `find_route_with_id` rather than `find_route`, as was intended and described in the API (lightningdevkit#2092). * Ensure `process_events_async` always exits if any sleep future returns true, not just if all sleep futures repeatedly return true (lightningdevkit#2145). * `channel_update` messages no longer set the disable bit unless the peer has been disconnected for some time. This should resolve cases where channels are disabled for extended periods of time (lightningdevkit#2198). * We no longer remove CLN nodes from the network graph for violating the BOLT spec in some cases after failing to pay through them (lightningdevkit#2220). * Fixed a debug assertion which may panic under heavy load (lightningdevkit#2172). * `CounterpartyForceClosed::peer_msg` is now wrapped in UntrustedString (lightningdevkit#2114) * Fixed a potential deadlock in `funding_transaction_generated` (lightningdevkit#2158). Security ======== * Transaction re-broadcasting is now substantially more aggressive, including a new regular rebroadcast feature called on a timer from the `background-processor` or from `ChainMonitor::rebroadcast_pending_claims`. This should substantially increase transaction confirmation reliability without relying on downstream `TransactionBroadcaster` implementations for rebroadcasting (lightningdevkit#2203, lightningdevkit#2205, lightningdevkit#2208). * Implemented the changes from BOLT PRs lightningdevkit#1031, lightningdevkit#1032, and lightningdevkit#1040 which resolve a privacy vulnerability which allows an intermediate node on the path to discover the final destination for a payment (lightningdevkit#2062). In total, this release features 110 files changed, 11928 insertions, 6368 deletions in 215 commits from 21 authors, in alphabetical order: * Advait * Alan Cohen * Alec Chen * Allan Douglas R. de Oliveira * Arik Sosman * Elias Rohrer * Evan Feenstra * Jeffrey Czyz * John Cantrell * Lucas Soriano del Pino * Marc Tyndel * Matt Corallo * Paul Miller * Steven * Steven Williamson * Steven Zhao * Tony Giorgio * Valentine Wallace * Wilmer Paulino * benthecarman * munjesi

TheBlueMatt added this to the 0.0.115 milestone Mar 24, 2023

TheBlueMatt force-pushed the 2023-03-payment-metadata branch from 2853050 to 64ed1a1 Compare March 24, 2023 22:32

alecchendev reviewed Mar 28, 2023

View reviewed changes

tnull self-requested a review March 28, 2023 07:14

TheBlueMatt force-pushed the 2023-03-payment-metadata branch 3 times, most recently from 401187d to bea53eb Compare March 28, 2023 20:49

valentinewallace self-requested a review March 29, 2023 00:16

valentinewallace reviewed Mar 29, 2023

View reviewed changes

TheBlueMatt force-pushed the 2023-03-payment-metadata branch from bea53eb to 4770ce9 Compare March 29, 2023 18:33

valentinewallace reviewed Mar 29, 2023

View reviewed changes

TheBlueMatt force-pushed the 2023-03-payment-metadata branch from 4770ce9 to 866fca8 Compare March 30, 2023 00:05

TheBlueMatt mentioned this pull request Mar 30, 2023

Add a new RecipientOnionFields and replace PaymentSecret with it #2139

Merged

TheBlueMatt force-pushed the 2023-03-payment-metadata branch 3 times, most recently from 0239343 to 91cd8c5 Compare April 4, 2023 17:43

Define the PaymentMetadata feature to be used in invoices

6ce239c

TheBlueMatt force-pushed the 2023-03-payment-metadata branch from 91cd8c5 to 567d11d Compare April 7, 2023 20:59

TheBlueMatt mentioned this pull request Apr 7, 2023

Support MPP Keysend #2156

Merged

alecchendev reviewed Apr 7, 2023

View reviewed changes

dunxen reviewed Apr 13, 2023

View reviewed changes

tnull reviewed Apr 17, 2023

View reviewed changes

TheBlueMatt force-pushed the 2023-03-payment-metadata branch from 3077967 to e1f3246 Compare April 17, 2023 15:26

valentinewallace reviewed Apr 18, 2023

View reviewed changes

This was referenced Apr 18, 2023

Enforce feature dependencies on construction-side #2193

Open

Drop redundant PaymentPurpose::Invoice::payment_secret #2200

Open

TheBlueMatt force-pushed the 2023-03-payment-metadata branch from e1f3246 to 3580020 Compare April 18, 2023 18:30

valentinewallace reviewed Apr 18, 2023

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

TheBlueMatt added 7 commits April 19, 2023 02:57

Add features module-level documentation for missing features

35b597a

Unset the optional bit for a feature when setting the required bit

1b29a55

There is no reason to set both, and this currently makes testing the new BOLT invoice tests slightly harder, so we just unset it.

Support reading the new payment_metadata field in invoices

928c9b8

This adds support for reading the new `PaymentMetadata` BOLT11 invoice field, giving us access to the `Vec<u8>` storing arbitrary bytes we have to send to the recipient.

Deserialize payment metadata fields in the onion final hop data

a90a35b

Make claimable_payments map value a struct, rather than a tuple

f57221b

This makes the `claimable_payments` code more upgradable allowing us to add new fields in the coming commit(s).

TheBlueMatt force-pushed the 2023-03-payment-metadata branch from 3580020 to c5d52ce Compare April 19, 2023 03:05

benthecarman reviewed Apr 19, 2023

View reviewed changes

lightning/src/ln/outbound_payment.rs Outdated

}

Copy link

Contributor

benthecarman Apr 19, 2023

Choose a reason for hiding this comment

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

nit: extra spacing here

tnull previously approved these changes Apr 19, 2023

View reviewed changes

TheBlueMatt added 7 commits April 19, 2023 14:55

Add a debug_assert the newly-documented (but existing) requirement

c1e6a74

If we add an entry to `claimable_payments` we have to ensure we actually accept the HTLC we're considering, otherwise we'll end up with an empty `claimable_payments` entry.

continue automatically after fail_htlc in receiving an HTLC

3dd05ab

If we receive an HTLC and are processing it a potential MPP part, we always continue in the per-HTLC loop if we call the `fail_htlc` macro, thus its nice to actually do the `continue` therein rather than at the callsites.

Expose the RecipientOnionFields in Event::PaymentClaimable

e1e7903

This finally completes the piping of the `payment_metadata` from from the BOLT11 invoice on the sending side all the way through the onion sending + receiving ends to the user on the receive events.

Add some tests of payment metadata being sent and received

a41d75f

Update and clarify the reasons for HTLCDestination::FailedPayment

934a534

Fix variable name typo

ef8e377

TheBlueMatt dismissed tnull’s stale review via ef8e377 April 19, 2023 14:55

TheBlueMatt force-pushed the 2023-03-payment-metadata branch from c5d52ce to ef8e377 Compare April 19, 2023 14:55

tnull approved these changes Apr 19, 2023

View reviewed changes

valentinewallace approved these changes Apr 19, 2023

View reviewed changes

TheBlueMatt merged commit 2e15df7 into lightningdevkit:main Apr 19, 2023

		RecipientOnionFields::secret_only(payment_secret_1), PaymentId(payment_hash_1.0)),
		true, APIError::ChannelUnavailable {..}, {});

		/// receives, thus you should generally never be providing a secret here for spontaneous
		/// payments.

		* Payments sent with the legacy `*_with_route` methods on LDK 0.0.115+ will no
		longer be retryable via the LDK 0.0.114- `retry_payment` method (#XXXX).

	/// If you wish to require that the sender include the metadata (and fail to parse the invoice if
	/// If you wish to require that senders include the metadata (and fail to parse the invoice if

Support sending PaymentMetadata in HTLCs #2127

Support sending PaymentMetadata in HTLCs #2127

Conversation

TheBlueMatt commented Mar 24, 2023

alecchendev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 28, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Mar 29, 2023

TheBlueMatt commented Apr 7, 2023

alecchendev left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Apr 10, 2023

dunxen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Apr 19, 2023

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Apr 19, 2023

Support sending `PaymentMetadata` in HTLCs #2127

Support sending `PaymentMetadata` in HTLCs #2127

codecov-commenter commented Mar 28, 2023 •

edited

Loading

tnull Apr 19, 2023 •

edited

Loading