Add payment metadata to payment request (feature 48) #912

joostjager · 2021-09-21T13:37:52Z

This PR adds a new field to bolt11 invoices for payment metadata. Senders are supposed to carry over this data to a tlv field to provide context for stateless invoice applications.

More background on stateless invoices: https://lists.linuxfoundation.org/pipermail/lightning-dev/2021-September/003236.html

Please attach comments to the most appropriate line of the diff, so that discussions remain threaded and can eventually be resolved. We all know what PRs look like with too many top level comments.

11-payment-encoding.md

t-bast · 2021-09-24T09:02:15Z

Interestingly, this will come for free with route blinding: #765
We introduce a recipient_secret in a payload that is encrypted for the recipient-only, and lets it avoid having to store data to validate a blinded route. That field can hold arbitrary data, so it does also enable your use-case.

11-payment-encoding.md

09-features.md

04-onion-routing.md

rustyrussell · 2021-10-11T19:59:13Z

So, @joostjager pointed out that you could just use "beer" and mix with an internal secret; the metadata doesn't need to be a UUID.

In general, however, this is not robust: you would not know if you have issued two such invoices simultaneously (if you did it serially, it's OK, since presumably you remember successfully-paid invoices, and could check for clashes).

And, if it serves as a UUID, it also serves as a payment secret?

joostjager · 2021-10-12T07:17:08Z

So, @joostjager pointed out that you could just use "beer" and mix with an internal secret; the metadata doesn't need to be a UUID.

In general, however, this is not robust: you would not know if you have issued two such invoices simultaneously (if you did it serially, it's OK, since presumably you remember successfully-paid invoices, and could check for clashes).

I suggested to indeed use "beer" but also append the payment address before mixing with the internal secret. So this should keep invoices apart.

t-bast

ACK once the pending comments are fixed.

I think we can (should?) integrate sender-side support ASAP, because it's trivial (just grab a field in the invoice, put it in the onion). Then after XXX months (when we see that senders have actually added support) we'll be able to leverage it on the recipient side.

11-payment-encoding.md

joostjager · 2021-10-29T10:07:23Z

I think we can (should?) integrate sender-side support ASAP, because it's trivial (just grab a field in the invoice, put it in the onion). Then after XXX months (when we see that senders have actually added support) we'll be able to leverage it on the recipient side.

Agreed. It takes time for this change to actually land in wallet software.

TheBlueMatt

Agreed, lets get payer-side support for this sooner rather than later.

t-bast

I have a draft PR ready on the eclair side if you want to do cross-compatibility tests: ACINQ/eclair#2063

t-bast · 2021-11-12T11:48:13Z

04-onion-routing.md

@@ -263,6 +263,9 @@ It is formatted according to the Type-Length-Value format defined in [BOLT #1](0
    2. data:
        * [`32*byte`:`payment_secret`]
        * [`tu64`:`total_msat`]
+    1. type: 10 (`payment_metadata`)


Route blinding already uses types 10 and 12, would it be possible to update this PR to use type 14 for payment_metadata?

Actually, it looks like AMP is using 14 already. Shall we move to 16 then?

What about AMP updating to another field? It's not documented that they use 14, not even in #658 AFAICT...

Haha, that is not a q for me to answer 🍿

To be consequent, I think we should use 10 then?

To be consequent, I think we should use 10 then?

Not quite, route blinding has documented that it uses 10 and 12 (for 6+ months) and already has two implementations' ACKs and cross-compatibility successfully tested (even before payment_metadata, so it does take precedence). AMP has no spec and only one implementation...

Your call, you can either update to 16 here or ask lnd folks. In all cases using 14 for AMP without specifying it anywhere is reckless, it just creates a big risk that another feature will ship across multiple implementations and use 14 for something else which will be a huge pain to update. Until specified, AMP should use a TLV in the high range to avoid running into this kind of compatibility problems.

Let's update to 16 so that we are unblocked.

Tagging @Roasbeef here for comment on AMP.

11-payment-encoding.md

cdecker · 2021-11-22T18:49:43Z

11-payment-encoding.md

@@ -235,6 +238,9 @@ by which the description is served is as-yet unspecified and will
 probably be transport dependent. The `h` format could change in the future,
 by changing the length, so readers ignore it if it's not 256 bits.

+The `m` field allows metadata to be attached to the payment. This supports
+applications where the recipient doesn't keep any context for the payment.


This is more a rationale for how this field could be used, but i can see other use cases as well (split PoS and recipient, ...). I'd personally prefer having these separate from the prescriptive text.

But this part is already inside the rationale section, isn't it? So it should be ok? Or do you mean that you don't want something as concrete here, and would rather have a separate doc (a spark?) explain how the payment_metadata field can be used for invoices not stored in a local DB?

t-bast

ACK 41962a7

t-bast · 2021-11-23T10:25:44Z

11-payment-encoding.md

@@ -216,7 +218,8 @@ A reader:
    - MUST use that as [`payment_secret`](04-onion-routing.md#tlv_payload-payload-format)
  - if the `c` field (`min_final_cltv_expiry`) is not provided:
    - MUST use an expiry delta of at least 18 when making the payment
-
+  - if an `m` field is provided:
+    - MUST use that as [`payment_metadata`](04-onion-routing.md#tlv_payload-payload-format)


nit: please add an empty line between this and ### Rationale

t-bast

ACK 2ab3a9f

rustyrussell · 2022-01-03T19:33:52Z

Ack 2ab3a9f

joostjager · 2022-01-03T20:12:21Z

Carried out interop test lnd->eclair, that works too.

vincenzopalazzo

ack 2ab3a9f

with latency, sorry :)

* Filter init, node and invoice features We should explicitly filter features based on where they can be included (`init`, `node_announcement` or `invoice`) as specified in Bolt 9. * Add support for sending `payment_metadata` Whenever we find a payment metadata field in an invoice, we send it in the onion payload for the final recipient. * Include `payment_metadata` in all invoices We include a payment metadata in every invoice we generate. This lets us see whether our payers support it or not, which is important data to have before we make it mandatory and use it for storage-less invoices. See lightning/bolts#912 for reference.

Add support for lightning/bolts#912 Whenever we find a payment metadata field in an invoice, we send it in the onion payload for the final recipient. We include a payment metadata in every invoice we generate. This lets us see whether our payers support it or not, which is important data to have before we make it mandatory and use it for storage-less invoices.

* Filter init, node and invoice features We should explicitly filter features based on where they can be included (`init`, `node_announcement` or `invoice`) as specified in Bolt 9. We also introduce the option_payment_metadata feature which helps our test cases since it's only allowed in invoices. * Refactor onion to dedicated namespace This commit doesn't contain any logic, it simply prefixes some classes to make it obvious that they are payment-related, rename files and moves some classes. We will update the payment onion, so it was a good time to do this small refactoring which will also be necessary for onion messages. * Add support for option_payment_metadata Add support for lightning/bolts#912 Whenever we find a payment metadata field in an invoice, we send it in the onion payload for the final recipient. We include a payment metadata in every invoice we generate. This lets us see whether our payers support it or not, which is important data to have before we make it mandatory and use it for storage-less invoices.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Changelog-Added: Protocol: `pay` (and decode, etc) supports bolt11 payment_metadata a-la lightning/bolts#912

joostjager · 2022-12-21T11:02:47Z

@t-bast I vaguely remember that you planned to add marker metadata to all invoices generated in your systems, to be able to gauge the uptake of the payment metadata feature. Did you do that in the end, and if yes, can you share how it is going?

t-bast · 2022-12-21T12:17:49Z

This was added in eclair and phoenix, but it doesn't give us a very easy way of measuring support. Only the phoenix recipient knows whether the sender included the payment metadata and this information isn't shared with our node. Our node doesn't issue invoices so we don't have any data about that.

This would be more interesting to measure at big hubs that acts as receiver nodes: Wallet of Satoshi, Muun and the likes. Those nodes are generating a lot of invoices and receiving payments and can better measure what percentage of senders do include the metadata.

joostjager commented Sep 21, 2021

View reviewed changes

11-payment-encoding.md Outdated Show resolved Hide resolved

joostjager force-pushed the custom-invoice-data branch from 819c657 to ac88cce Compare September 21, 2021 13:52

t-bast mentioned this pull request Sep 23, 2021

Lightning Specification Meeting 2021/09/27 #916

Closed

19 tasks

t-bast reviewed Sep 24, 2021

View reviewed changes

11-payment-encoding.md Show resolved Hide resolved

joostjager force-pushed the custom-invoice-data branch from ac88cce to a8e5fab Compare September 24, 2021 13:42

joostjager requested a review from t-bast September 24, 2021 13:46

joostjager force-pushed the custom-invoice-data branch from a8e5fab to 7e73756 Compare September 28, 2021 07:46

joostjager changed the title ~~Add payment metadata to payment request~~ Add payment metadata to payment request (feature 48) Sep 28, 2021

joostjager commented Sep 28, 2021

View reviewed changes

09-features.md Outdated Show resolved Hide resolved

thomash-acinq reviewed Sep 28, 2021

View reviewed changes

04-onion-routing.md Outdated Show resolved Hide resolved

joostjager mentioned this pull request Sep 30, 2021

routing: send payment metadata lightningnetwork/lnd#5810

Merged

t-bast mentioned this pull request Oct 6, 2021

Lightning Specification Meeting 2021/10/11 #920

Closed

19 tasks

t-bast mentioned this pull request Oct 21, 2021

Lightning Specification Meeting 2021/10/25 #929

Closed

21 tasks

t-bast reviewed Oct 27, 2021

View reviewed changes

11-payment-encoding.md Outdated Show resolved Hide resolved

joostjager force-pushed the custom-invoice-data branch from 7e73756 to cdd3cbc Compare October 29, 2021 10:06

t-bast mentioned this pull request Nov 5, 2021

Lightning Specification Meeting 2021/11/08 #933

Closed

21 tasks

TheBlueMatt reviewed Nov 11, 2021

View reviewed changes

t-bast mentioned this pull request Nov 12, 2021

Add support for option_payment_metadata ACINQ/eclair#2063

Merged

t-bast reviewed Nov 12, 2021

View reviewed changes

TheBlueMatt mentioned this pull request Nov 15, 2021

Drop ChannelManager::pending_inbound_payment and replace payment_secret with HMAC(payment data) lightningdevkit/rust-lightning#1171

Closed

joostjager force-pushed the custom-invoice-data branch 3 times, most recently from 8e9f89b to 41962a7 Compare November 22, 2021 09:21

t-bast mentioned this pull request Nov 22, 2021

Lightning Specification Meeting 2021/11/22 #936

Closed

21 tasks

cdecker reviewed Nov 22, 2021

View reviewed changes

t-bast reviewed Nov 23, 2021

View reviewed changes

t-bast mentioned this pull request Jan 3, 2022

Lightning Specification Meeting 2022/01/03 #949

Closed

21 tasks

Add payment metadata to payment request

2ab3a9f

joostjager force-pushed the custom-invoice-data branch from 42dc90b to 2ab3a9f Compare January 3, 2022 19:09

t-bast approved these changes Jan 3, 2022

View reviewed changes

TheBlueMatt approved these changes Jan 3, 2022

View reviewed changes

TheBlueMatt merged commit f6c4d76 into lightning:master Jan 3, 2022

vincenzopalazzo reviewed Jan 7, 2022

View reviewed changes

bitromortac mentioned this pull request Jan 11, 2022

Support for payment metadata of LN invoices spesmilo/electrum#7626

Open

rustyrussell mentioned this pull request Mar 11, 2022

BOLT update, including payment metadata support ElementsProject/lightning#5086

Merged

jb55 mentioned this pull request Mar 24, 2022

Offers #798

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add payment metadata to payment request (feature 48) #912

Add payment metadata to payment request (feature 48) #912

joostjager commented Sep 21, 2021 •

edited

Loading

t-bast commented Sep 24, 2021

rustyrussell commented Oct 11, 2021

joostjager commented Oct 12, 2021

t-bast left a comment

joostjager commented Oct 29, 2021

TheBlueMatt left a comment

t-bast left a comment

t-bast Nov 12, 2021

joostjager Nov 22, 2021

joostjager Dec 7, 2021

t-bast Dec 7, 2021

joostjager Dec 7, 2021

t-bast Dec 7, 2021

joostjager Dec 7, 2021

cdecker Nov 22, 2021

t-bast Nov 23, 2021

t-bast left a comment

t-bast Nov 23, 2021

t-bast left a comment

rustyrussell commented Jan 3, 2022

joostjager commented Jan 3, 2022 •

edited

Loading

vincenzopalazzo left a comment •

edited

Loading

joostjager commented Dec 21, 2022

t-bast commented Dec 21, 2022

Add payment metadata to payment request (feature 48) #912

Add payment metadata to payment request (feature 48) #912

Conversation

joostjager commented Sep 21, 2021 • edited Loading

t-bast commented Sep 24, 2021

rustyrussell commented Oct 11, 2021

joostjager commented Oct 12, 2021

t-bast left a comment

Choose a reason for hiding this comment

joostjager commented Oct 29, 2021

TheBlueMatt left a comment

Choose a reason for hiding this comment

t-bast 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

t-bast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-bast left a comment

Choose a reason for hiding this comment

rustyrussell commented Jan 3, 2022

joostjager commented Jan 3, 2022 • edited Loading

vincenzopalazzo left a comment • edited Loading

Choose a reason for hiding this comment

joostjager commented Dec 21, 2022

t-bast commented Dec 21, 2022

joostjager commented Sep 21, 2021 •

edited

Loading

joostjager commented Jan 3, 2022 •

edited

Loading

vincenzopalazzo left a comment •

edited

Loading