Introduce RecipientInfo struct for send_payment #1445

andozw · 2022-04-22T01:09:19Z

This work is a pre-req for Issue-1298.

This is a rebase of PR-1221 with an
additional commit to move the payment_secret and the payment_metadata into a new struct called
RecipientInfo. This will allow us to add custom onion TLVs, see discussion

No logic change, just lots of plumbing.

andozw · 2022-04-22T01:21:49Z

Looks like some CI failures because I didn't add docs to the new struct fields:

error: missing documentation for a struct field
  --> lightning/src/ln/mod.rs:92:2
   |
92 |     pub payment_secret: Option<PaymentSecret>,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

These look different than clippy. Probably a silly newbie rust question, but is there a way to run this rust linting locally?

codecov-commenter · 2022-04-22T03:17:18Z

Codecov Report

Merging #1445 (f76e7d9) into main (b5a6307) will increase coverage by 0.66%.
The diff coverage is 95.09%.

❗ Current head f76e7d9 differs from pull request most recent head 40e436e. Consider uploading reports for the commit 40e436e to get more accurate results

@@            Coverage Diff             @@
##             main    #1445      +/-   ##
==========================================
+ Coverage   90.89%   91.56%   +0.66%     
==========================================
  Files          76       75       -1     
  Lines       42067    45384    +3317     
  Branches    42067    45384    +3317     
==========================================
+ Hits        38238    41554    +3316     
- Misses       3829     3830       +1

Impacted Files	Coverage Δ
lightning-invoice/src/de.rs	`82.21% <0.00%> (-0.18%)`	⬇️
lightning-invoice/src/ser.rs	`91.51% <0.00%> (-0.69%)`	⬇️
lightning/src/ln/mod.rs	`82.60% <0.00%> (-12.40%)`	⬇️
lightning/src/routing/router.rs	`92.51% <ø> (-0.01%)`	⬇️
lightning/src/util/events.rs	`33.22% <14.28%> (+<0.01%)`	⬆️
lightning-invoice/src/lib.rs	`86.83% <80.95%> (-0.57%)`	⬇️
lightning-invoice/src/payment.rs	`92.66% <100.00%> (-0.10%)`	⬇️
lightning-invoice/src/utils.rs	`96.87% <100.00%> (+0.10%)`	⬆️
lightning/src/chain/chainmonitor.rs	`97.65% <100.00%> (+0.01%)`	⬆️
lightning/src/chain/channelmonitor.rs	`91.07% <100.00%> (+<0.01%)`	⬆️
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5a6307...40e436e. Read the comment docs.

TheBlueMatt · 2022-04-22T15:12:47Z

These look different than clippy. Probably a silly newbie rust question, but is there a way to run this rust linting locally?

The "requires doc" lint is defines as a compiler error in the lightning crate (at least for non-test builds - we dont care if test-only stuff is missing docs), so it should show up with cargo test && cargo doc. The second is required to catch the "bad doclinks" lint. We don't generally require clippy on the crate because its historically "required" us to do stuff that violates our MSRV, though there's a few clippy lints we do apply.

TheBlueMatt · 2022-04-22T15:41:18Z

Looks like some code in fuzz/bench wasn't updated. You may want to cd fuzz && RUSTFLAGS="--cfg=fuzzing" cargo test and RUSTC_BOOTSTRAP=1 cargo bench --features _bench_unstable.

TheBlueMatt

Thanks for picking this up! Looks good, basically, I think, I realized my original PR really should have split out the FinalOnionHopData-in-ClaimableHTLC stuff out into a separate PR because its free-standing and makes sense on its own, plus is a bit of tricky code that would be nice to land separately, and probably would speed up the whole process, so if you don't mind taking those commits out into a separate PR, that'd be great.

TheBlueMatt · 2022-04-22T15:18:57Z

lightning/src/ln/channelmanager.rs

@@ -2462,6 +2473,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 	/// irrevocably committed to on our end. In such a case, do NOT retry the payment with a
 	/// different route unless you intend to pay twice!
 	///
+	/// Provide recipient_info to include payment_secret or payment_metadata), note that the 


Looks half-updated?

typo on the ( but I meant for the sentence to flow to the next line. Wrapped to try to match the style from the file. Any tips for better wording?

TheBlueMatt · 2022-04-22T15:42:34Z

lightning/src/util/events.rs

@@ -60,6 +60,8 @@ pub enum PaymentPurpose {
 		/// [`ChannelManager::create_inbound_payment`]: crate::ln::channelmanager::ChannelManager::create_inbound_payment
 		/// [`ChannelManager::create_inbound_payment_for_hash`]: crate::ln::channelmanager::ChannelManager::create_inbound_payment_for_hash
 		payment_secret: PaymentSecret,
+		///XXX


Heh, needs some docs :)

TheBlueMatt · 2022-04-22T15:46:04Z

lightning/src/ln/mod.rs

@@ -86,6 +86,14 @@ pub struct PaymentPreimage(pub [u8;32]);
 /// (C-not exported) as we just use [u8; 32] directly
 #[derive(Hash, Copy, Clone, PartialEq, Eq, Debug)]
 pub struct PaymentSecret(pub [u8;32]);
+/// recipient-info type, use to provide payment_secret or bolt11 payment_metadata


These need to be filled out a bit more - something about how this is arbitrary data to be communicated to the recipient - mostly came from the invoice. "recipient-info type" doesn't tell me much, maybe drop that?

👍
makes sense, was trying to match the style from the comment for PaymentSecret, but yeah, redundant. I'll take a stab at some of these comments with a fresh fixup commit and maybe we can iterate on them over github?

TheBlueMatt · 2022-04-22T15:47:28Z

lightning/src/ln/mod.rs

+/// recipient-info type, use to provide payment_secret or bolt11 payment_metadata
+#[derive(Clone, Debug, PartialEq)]
+pub struct RecipientInfo {
+	/// optional payment_secret


Given we're gonna add more fields that are "arbitrary data we send to the recipient" we should note in the two fields here are things the recipient provided us, not stuff we picked. Probably note in both of them that they are (optionally) included in the BOLT11 invoice.

TheBlueMatt · 2022-05-03T15:09:39Z

Awesome, this needs a rebase now and should be a much more straightforward PR to review.

andozw · 2022-05-09T22:12:47Z

rebased and attempted to fixup comments

TheBlueMatt

Oops, two functional changes I think we should do. If possible, could you split the fixup commit you added into separate parts that apply to each commit and then rebase -i to place each fixup commit in the "right place" in the commit history?

TheBlueMatt · 2022-05-10T00:08:13Z

lightning/src/util/events.rs

@@ -60,6 +60,9 @@ pub enum PaymentPurpose {
 		/// [`ChannelManager::create_inbound_payment`]: crate::ln::channelmanager::ChannelManager::create_inbound_payment
 		/// [`ChannelManager::create_inbound_payment_for_hash`]: crate::ln::channelmanager::ChannelManager::create_inbound_payment_for_hash
 		payment_secret: PaymentSecret,
+		/// Additional metadata to attach to the payment. This supports applications where the recipient doesn't keep any context for the payment.
+		/// Note that the size of this field is limited by the maximum hop payload size. Long metadata fields reduce the maximum route length.
+		payment_metadata: Option<Vec<u8>>,


Should we use the RecipientInfo struct here?

TheBlueMatt · 2022-05-10T00:09:01Z

lightning/src/util/events.rs

@@ -60,6 +60,9 @@ pub enum PaymentPurpose {
 		/// [`ChannelManager::create_inbound_payment`]: crate::ln::channelmanager::ChannelManager::create_inbound_payment
 		/// [`ChannelManager::create_inbound_payment_for_hash`]: crate::ln::channelmanager::ChannelManager::create_inbound_payment_for_hash
 		payment_secret: PaymentSecret,
+		/// Additional metadata to attach to the payment. This supports applications where the recipient doesn't keep any context for the payment.


While maybe we'll replace this so nbd, in general please wrap comments/docs at around 100 chars. Some of the immediate above lines aren't wrapped because the rustdoc link format doesn't allow wrapping, but where we're allowed to we should wrap.

Sweet! Wrapping at 100 ftw. But how can you tell when it is allowed vs isn't? Should I assume I can always wrap and then if cargo doc fails, unwrap?

Related, I can't discern a pattern for code wrapping. There seem to be quite a few function definitions that don't wrap which makes for a lot of horizontal scrolling. Is there a reason why there is not a lint rule that enforces 100 on code?

For docs, always wrap unless it's the link override stuff (where we take a []-contained string and specify the full path to change the link). For code, readability above all else. There's some older code that hasn't been touched in some time that is pretty bad, but for new stuff try to break around 100, but it's not a hard limit so much as a guideline to encourage readability.

TheBlueMatt · 2022-05-10T00:11:47Z

lightning/src/ln/channelmanager.rs

@@ -3175,6 +3186,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 												payment_hash,
 												purpose: events::PaymentPurpose::InvoicePayment {
 													payment_preimage: $payment_preimage,
+													payment_metadata: $payment_data.payment_metadata,


Hmm, on second thought can we add a commit that checks that the metadata matches between all the MPP parts? Same as we do for the payment secrets.

do we just want to do a byte for byte comparison of the payment_data byte Vec and call the same fail_htlc macro if there is a difference?

Yea, I think so.

BOLTs say is has to be included identically on all the HTLCs, too - https://github.com/lightning/bolts/pull/912/files#diff-ad65f0beaac5cef88f5fd7a8b9ca36cbc5a790f4815b4b50b9ea794a55aaf012R287

TheBlueMatt · 2022-05-10T00:18:07Z

Also, looks like benchmark builds (RUSTC_BOOTSTRAP=1 cargo bench --features _bench_unstable) and fuzz builds (cd fuzz && RUSTFLAGS=--cfg=fuzzing cargo test) are failing in CI as they need to be updated to the latest API.

andozw · 2022-05-10T17:23:44Z

Also, looks like benchmark builds (RUSTC_BOOTSTRAP=1 cargo bench --features _bench_unstable) and fuzz builds (cd fuzz && RUSTFLAGS=--cfg=fuzzing cargo test) are failing in CI as they need to be updated to the latest API.

Ah damn I pushed right before I got on a plane and I forgot to run those.

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

...without exposing it to the public `send_payment` API yet.

This work is a pre-req for lightningdevkit#1298. This moves the `payment_secret` and the `payment_metadata` into a new struct called `RecipientInfo`. This will allow us to add custom onion TLVs, see lightningdevkit#1298 (comment) No logic change, just lots of plumbing.

andozw · 2022-05-13T12:51:19Z

rebased

tnull

Thanks! Generally looks good, just some comments.

tnull · 2022-05-17T11:31:53Z

lightning/src/ln/mod.rs

+/// Additional BOLT 11 data to attach to the payment, useful for applications where the recipient
+/// doesn't keep any context for the payment.
+#[derive(Clone, Debug, PartialEq)]
+pub struct RecipientInfo {


I find the naming somewhat confusing: I read RecipientInfo as 'information about the recipient', but not as 'information from/supplied by the recipient'. Therefore I'd like to suggest changing the name of the struct to something more descriptive. For example RecipientProvidedData or RecipientProvidedInfo would work I guess, but feel free to come up with something better. Unfortunately, the rather simple PaymentMetadata could lead to some confusion because it is already used for the field itself.

@TheBlueMatt any preference on the naming?

Hmm, not a strong preference, no, though I see @tnull's point and generally agree.

tnull · 2022-05-17T12:37:29Z

lightning/src/ln/msgs.rs

@@ -1301,6 +1302,11 @@ impl_writeable_msg!(UpdateAddHTLC, {
 	onion_routing_packet
 }, {});

+// The serialization here is really funky - FinalOnionHopData somewhat serves as two different
+// things. First, it is *the* object which is written with type 8 in the onion TLV. Second, it is
+// the data which a node may expect to receive when we are the recipient of an invoice payment.


Should probably be "when it is the recipient of an invoice payment."

tnull · 2022-05-17T12:41:21Z

lightning/src/ln/msgs.rs

@@ -1301,6 +1302,11 @@ impl_writeable_msg!(UpdateAddHTLC, {
 	onion_routing_packet
 }, {});

+// The serialization here is really funky - FinalOnionHopData somewhat serves as two different


Since this is indeed a bit weird and only seems to be needed for backwards compat, maybe we want to mark FinalOnionHopData as #[deprecated] so we'll be annoyed by the warnings into removing it soon/eventually?

tnull · 2022-05-17T12:48:28Z

lightning/src/ln/channelmanager.rs

@@ -223,12 +224,13 @@ impl core::hash::Hash for HTLCSource {
 				0u8.hash(hasher);
 				prev_hop_data.hash(hasher);
 			},
-			HTLCSource::OutboundRoute { path, session_priv, payment_id, payment_secret, first_hop_htlc_msat, payment_params } => {
+			HTLCSource::OutboundRoute { path, session_priv, payment_id, payment_secret, payment_metadata, first_hop_htlc_msat, payment_params } => {


If we're touching long lines like this, we probably want to start using recommended text width of 100 characters.

tnull · 2022-05-17T12:49:06Z

lightning/src/ln/channelmanager.rs

@@ -2331,15 +2335,15 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 	}

 	// Only public for testing, this should otherwise never be called direcly
-	pub(crate) fn send_payment_along_path(&self, path: &Vec<RouteHop>, payment_params: &Option<PaymentParameters>, payment_hash: &PaymentHash, payment_secret: &Option<PaymentSecret>, total_value: u64, cur_height: u32, payment_id: PaymentId, keysend_preimage: &Option<PaymentPreimage>) -> Result<(), APIError> {
+	pub(crate) fn send_payment_along_path(&self, path: &Vec<RouteHop>, payment_params: &Option<PaymentParameters>, payment_hash: &PaymentHash, payment_secret: &Option<PaymentSecret>, payment_metadata: &Option<Vec<u8>>, total_value: u64, cur_height: u32, payment_id: PaymentId, keysend_preimage: &Option<PaymentPreimage>) -> Result<(), APIError> {


Same here, would be great to consider the 100 char text width.

tnull · 2022-05-17T12:49:18Z

lightning/src/ln/channelmanager.rs

 		log_trace!(self.logger, "Attempting to send payment for path with next hop {}", path.first().unwrap().short_channel_id);
 		let prng_seed = self.keys_manager.get_secure_random_bytes();
 		let session_priv_bytes = self.keys_manager.get_secure_random_bytes();
 		let session_priv = SecretKey::from_slice(&session_priv_bytes[..]).expect("RNG is busted");

 		let onion_keys = onion_utils::construct_onion_keys(&self.secp_ctx, &path, &session_priv)
 			.map_err(|_| APIError::RouteError{err: "Pubkey along hop was maliciously selected"})?;
-		let (onion_payloads, htlc_msat, htlc_cltv) = onion_utils::build_onion_payloads(path, total_value, payment_secret, cur_height, keysend_preimage)?;
+		let (onion_payloads, htlc_msat, htlc_cltv) = onion_utils::build_onion_payloads(path, total_value, payment_secret, payment_metadata.clone(), cur_height, keysend_preimage)?;


Same here, 100 char text width, etc.

lightning/src/ln/mod.rs

TheBlueMatt · 2022-11-30T19:27:02Z

Any desire to keep working on this @andozw? This got super stale and needs rebase :(

andozw · 2022-12-04T16:34:55Z

Any desire to keep working on this @andozw? This got super stale and needs rebase :(

Oops. I'll take a look at the rebase this week. Was this work a pre-requisite for something else beyond PeerSwap?

TheBlueMatt · 2022-12-04T18:55:04Z

We may well want to reuse this struct to support sending custom TLVs in the onion to the recipient, something people use for messaging and the like.

TheBlueMatt · 2023-02-08T01:27:21Z

Tagging 115 as payment metadata support is increasingly commonly supported by nodes so we should really do it sooner rather than later.

TheBlueMatt · 2023-03-22T22:55:24Z

Gonna take this over for 115.

andozw force-pushed the seana.20220421.add-recipient-info branch from 154e179 to 3ec07c0 Compare April 22, 2022 02:51

andozw force-pushed the seana.20220421.add-recipient-info branch from 3ec07c0 to 50afa37 Compare April 22, 2022 12:16

TheBlueMatt reviewed Apr 22, 2022

View reviewed changes

TheBlueMatt mentioned this pull request Apr 22, 2022

Handle Payment Metadata in the Invoice/Send/Receive Pipelines #1221

Closed

This was referenced Apr 22, 2022

[sean] Avoid storing a full FinalOnionHopData in OnionPayload::Invoice andozw/rust-lightning#3

Merged

Avoid storing a full FinalOnionHopData in OnionPayload::Invoice #1447

Merged

TheBlueMatt added the blocked on dependent pr label Apr 24, 2022

TheBlueMatt removed the blocked on dependent pr label May 3, 2022

andozw force-pushed the seana.20220421.add-recipient-info branch from 50afa37 to f76e7d9 Compare May 9, 2022 22:11

TheBlueMatt reviewed May 10, 2022

View reviewed changes

TheBlueMatt and others added 10 commits May 13, 2022 05:50

Define the PaymentMetadata feature to be used in invoices

3876fbf

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

b690365

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

f90cf53

Support setting the new payment metadata field in invoices

fda77f2

Deserialize payment metadata fields in the onion final hop data

3fa3694

Provide users the payment metadata via Event::PaymentReceived

c90e20c

Pipe payment metadata through the HTLC send pipeline

a523f10

...without exposing it to the public `send_payment` API yet.

Expose payment metadata in the sending path via send_payment.

4d5b91e

f fix comments

40e436e

andozw force-pushed the seana.20220421.add-recipient-info branch from f76e7d9 to 40e436e Compare May 13, 2022 12:50

tnull reviewed May 17, 2022

View reviewed changes

TheBlueMatt added this to the 0.0.115 milestone Feb 8, 2023

TheBlueMatt closed this Mar 22, 2023

TheBlueMatt mentioned this pull request Mar 24, 2023

Support sending PaymentMetadata in HTLCs #2127

Merged

Introduce RecipientInfo struct for send_payment #1445

Introduce RecipientInfo struct for send_payment #1445

Conversation

andozw commented Apr 22, 2022

andozw commented Apr 22, 2022

codecov-commenter commented Apr 22, 2022 • edited Loading

Codecov Report

TheBlueMatt commented Apr 22, 2022 • edited Loading

TheBlueMatt commented Apr 22, 2022

TheBlueMatt 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

TheBlueMatt commented May 3, 2022

andozw commented May 9, 2022

TheBlueMatt 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

TheBlueMatt commented May 10, 2022

andozw commented May 10, 2022

andozw commented May 13, 2022

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

TheBlueMatt commented Nov 30, 2022

andozw commented Dec 4, 2022

TheBlueMatt commented Dec 4, 2022

TheBlueMatt commented Feb 8, 2023

TheBlueMatt commented Mar 22, 2023

codecov-commenter commented Apr 22, 2022 •

edited

Loading

TheBlueMatt commented Apr 22, 2022 •

edited

Loading