routing: send payment metadata #5810

joostjager · 2021-09-30T10:20:21Z

Please attach comments to the most appropriate line of the diff (or a random line if none is appropriate), so that discussions remain threaded and can eventually be resolved.

joostjager · 2021-09-30T10:23:55Z

record/hop.go

+
+	// MetadataOnionType is the type used in the onion for the payment
+	// metadata.
+	MetadataOnionType tlv.Type = 10


@Roasbeef, I've created a draft PR to evaluate touch points. Do you want to go for this?

Makes a ton of sense IMO, super useful for the AMP recurring case as well. Could also bundle it w/ the pubkey based routing stuff as well...

Why is it useful for the AMP recurring case actually? The sending of metadata I get. This could be something like a donation comment. But what would be the purpose of embedding metadata in the payment request? Recurring payment senders would all pass through that same metadata, but what does the receiver learn from this that they didn't already know?

joostjager · 2021-11-02T11:04:12Z

invoices/invoiceregistry.go

@@ -938,6 +938,7 @@ func (i *InvoiceRegistry) NotifyExitHopHtlc(rHash lntypes.Hash,
 		customRecords:        payload.CustomRecords(),
 		mpp:                  payload.MultiPath(),
 		amp:                  payload.AMPRecord(),
+		metadata:             payload.Metadata(),


What is missing here is that we are assuming that an invoice already exists. The goal of the new metadata field is to enable stateless invoices. In that case, there isn't an invoice yet. It is a kind of spontaneous payment (not really spontaneous - the receiver just 'forgot' the invoice), but the difference with keysend/amp is that the receiver is able to re-derive the preimage. The preimage can be used as proof of payment.

I am wondering what the options are to implement this. Will it be yet another payment type? Or is this the point where it becomes necessary to allow invoice registry plugins so that users can implement the logic themselves?

LDK approach with this: lightningdevkit/rust-lightning#1171

I am wondering what the options are to implement this. Will it be yet another payment type

I think we can just use the existing keysend flow here, which ends up inserting the the database, or using the existing AMP pattern to re-insert a new sub-invoice with a main tracking identifier. IIUC, with the proposal you'd still write the invoice to disk, but the "stateless" part is what lets you not have to remember the invoice until its paid?

Alternatively, the existing HTLC interceptor logic can handle this as well assuming you have the invoice logic already living outside of lnd.

I think "stateless" can be implemented in multiple ways, but doing a JIT insert similar to keysend seems definitely one of the options.

carlaKC

Minimal change for a nifty feature!

carlaKC · 2022-01-04T08:59:02Z

cmd/lncli/cmd_invoice.go

@@ -26,6 +26,11 @@ var addInvoiceCommand = cli.Command{
 			Usage: "a description of the payment to attach along " +
 				"with the invoice (default=\"\")",
 		},
+		cli.StringFlag{
+			Name: "metadata",
+			Usage: "the hex-encoded metadata that will be sent " +


Why hex-encoded on the cli? Depending on the sort of values we're expecting here, I'd think that most people would just want to provide a string + have it hex-encoded on their behalf by the cli?

I think it depends on what the purpose of the metadata is. With this PR, we can't reap the fruits of the metadata yet. It is only preparatory. Yes, we store the metadata in the invoice database upon receipt, but I don't think that is much different from just using description to store additional data locally.

A next step is to no longer store the invoices that we create in the invoice registry. Then when a payment comes in, we use the metadata in the tlv to construct and insert an invoice just-in-time. One of the things that metadata can contain is a preimage encrypted to ourselves.

In those kind of setups, I'd expect metadata to be structured, machine-readable data. Therefore the hex encoding.

carlaKC · 2022-01-04T09:13:55Z

lnwire/features.go

+	// PaymentMetadataRequired is a required bit that denotes that an
+	// invoice contains metadata that must be passed along with the payment
+	// htlc(s).
+	PaymentMetadataRequired = 48


I saw there was some discussion in the spec PR about this, seems like the conclusion was to add the optional feature bit as well? Even if we're not going to use it, might be worth adding it here? (more confusing to not have it, than have it and not use it IMO)

Added the optional feature bit.

The Eclair team is going to use this to get an idea of the proportion of senders that have upgraded to node software that supports metadata sending. They want to include some metadata with every invoice generated and then observe the incoming htlcs to see if it is echoed in tlv.

This is something for us to decide on too. Do we want to increase the size of every invoice slightly to include the marker metadata and the optional bit?

I think its clearer to just have it there, rather than explain why it's not there.

This is something for us to decide on too. Do we want to increase the size of every invoice slightly to include the marker metadata and the optional bit?

Wouldn't this decision fall on implementors rather than lnd itself?

With this PR as is, there is no way they can add that marker metadata.

htlcswitch/hop/payload.go

invoices/interface.go

joostjager · 2022-01-11T13:35:46Z

I've marked the last commits as [TO BE REMOVED]. I think it is better to just merge the sending part with some light logging receiver-side.

Real receiver-side logic for stateless invoices needs more thinking, but at least with this PR we can start the upgrade process for senders.

joostjager · 2022-01-19T11:29:08Z

Added test coverage

carlaKC

Only minor comments. I think you can drop the to remove commits and I'll do a final approval pass!

zpay32/encode.go

carlaKC · 2022-02-03T08:56:06Z

zpay32/invoice_test.go

@@ -726,6 +751,10 @@ func TestDecodeEncode(t *testing.T) {
 		}

 		if decodedInvoice != nil {
+			if test.implicitDest {


Add a comment to explain this? I'm not quite following

Removed again. I discovered that there was already a mechanism for this with the beforeEncoding closure.

carlaKC · 2022-02-03T09:01:40Z

lnwire/features.go

+	// PaymentMetadataRequired is a required bit that denotes that an
+	// invoice contains metadata that must be passed along with the payment
+	// htlc(s).
+	PaymentMetadataRequired = 48


I think its clearer to just have it there, rather than explain why it's not there.

This is something for us to decide on too. Do we want to increase the size of every invoice slightly to include the marker metadata and the optional bit?

Wouldn't this decision fall on implementors rather than lnd itself?

lnwire/features.go

routing/pathfind_test.go

joostjager · 2022-03-24T13:44:34Z

Rebased.

As a reminder, this PR is a small change that unlocks interesting new ways to accept payments statelessly. It does require senders on the network to upgrade, which takes time. It would be great to include this in the next release.

Other implementations are moving too:

ElementsProject/lightning#5086
ACINQ/eclair#2063
lightningdevkit/rust-lightning#1221

@carlaKC is this PR still on the safe side for the 0.15 cut and does it need another reviewer?

lightninglabs-deploy · 2022-04-07T14:27:35Z

@carlaKC: review reminder
@joostjager, remember to re-request review from reviewers when ready

Roasbeef

Only thing I think this is missing is extending the InterceptedPacket type with the payment metadata field (as is since this isn't a "custom" record, it won't show up) to also add this field. With this addition, then the full "stateless" invoice concept can be fully realized with a custom HTLC interceptor.

Roasbeef · 2022-04-12T23:43:08Z

zpay32/encode.go

@@ -164,6 +164,17 @@ func writeTaggedFields(bufferBase32 *bytes.Buffer, invoice *Invoice) error {
 		}
 	}

+	if invoice.Metadata != nil {
+		base32, err := bech32.ConvertBits(invoice.Metadata, 8, 5, true)


writeBytes32 can be used here instead.

writeBytes32 always writes 32 bytes, where as the invoice metadata is variable length?

Roasbeef · 2022-04-12T23:50:09Z

invoices/invoiceregistry.go

@@ -938,6 +938,7 @@ func (i *InvoiceRegistry) NotifyExitHopHtlc(rHash lntypes.Hash,
 		customRecords:        payload.CustomRecords(),
 		mpp:                  payload.MultiPath(),
 		amp:                  payload.AMPRecord(),
+		metadata:             payload.Metadata(),


I am wondering what the options are to implement this. Will it be yet another payment type

I think we can just use the existing keysend flow here, which ends up inserting the the database, or using the existing AMP pattern to re-insert a new sub-invoice with a main tracking identifier. IIUC, with the proposal you'd still write the invoice to disk, but the "stateless" part is what lets you not have to remember the invoice until its paid?

Roasbeef · 2022-04-12T23:50:34Z

invoices/invoiceregistry.go

@@ -938,6 +938,7 @@ func (i *InvoiceRegistry) NotifyExitHopHtlc(rHash lntypes.Hash,
 		customRecords:        payload.CustomRecords(),
 		mpp:                  payload.MultiPath(),
 		amp:                  payload.AMPRecord(),
+		metadata:             payload.Metadata(),


Alternatively, the existing HTLC interceptor logic can handle this as well assuming you have the invoice logic already living outside of lnd.

joostjager · 2022-04-13T11:21:24Z

Only thing I think this is missing is extending the InterceptedPacket type with the payment metadata field (as is since this isn't a "custom" record, it won't show up) to also add this field. With this addition, then the full "stateless" invoice concept can be fully realized with a custom HTLC interceptor.

I don't think this is needed. HTLCs can't be intercepted at the exit hop, so anything that is intercepted will be a forward. For external invoice handling, the intercepted forward will generally be to the exit hop. This means that the forwarding node, where the interceptor is connected to, won't be able to decrypt the payment metadata. It is still packed in the next hop payload in encrypted form and this payload is already available to the interceptor.

Roasbeef · 2022-04-13T22:00:55Z

HTLCs can't be intercepted at the exit hop, so anything that is intercepted will be a forward.

Ah true, and even if you used hop hints to have the node be seen as the penultimate hop to the sender, the payload itself is in the onion packet which is already available and can be decrypted by the interceptor if things are well formed.

Roasbeef

LGTM 💐

joostjager commented Sep 30, 2021

View reviewed changes

Roasbeef added this to the v0.15.0 milestone Sep 30, 2021

joostjager force-pushed the payment-metadata branch from 396eca0 to 916ad5f Compare October 4, 2021 07:34

joostjager force-pushed the payment-metadata branch 2 times, most recently from 904ce49 to bc28d7e Compare November 2, 2021 10:06

joostjager commented Nov 2, 2021

View reviewed changes

joostjager force-pushed the payment-metadata branch from ed27191 to 7b691dd Compare November 22, 2021 09:37

carlaKC self-requested a review November 29, 2021 11:30

joostjager force-pushed the payment-metadata branch from 7b691dd to 13fe949 Compare December 8, 2021 08:26

joostjager force-pushed the payment-metadata branch from 13fe949 to 86a3c59 Compare December 21, 2021 08:46

carlaKC reviewed Jan 4, 2022

View reviewed changes

joostjager force-pushed the payment-metadata branch from 86a3c59 to 0ce379c Compare January 11, 2022 13:32

joostjager changed the title ~~routing: payment metadata~~ routing: send payment metadata Jan 11, 2022

joostjager force-pushed the payment-metadata branch 2 times, most recently from e9585cd to c2fa9aa Compare January 11, 2022 14:54

joostjager force-pushed the payment-metadata branch 2 times, most recently from cb00b8f to f67bc51 Compare January 19, 2022 11:27

joostjager requested a review from carlaKC January 19, 2022 11:28

joostjager force-pushed the payment-metadata branch from f67bc51 to 2ae37a0 Compare January 19, 2022 11:38

joostjager marked this pull request as ready for review January 19, 2022 12:56

Roasbeef added this to In progress in v0.15.0-beta via automation Feb 2, 2022

Roasbeef moved this from In progress to Review in progress in v0.15.0-beta Feb 2, 2022

carlaKC reviewed Feb 3, 2022

View reviewed changes

joostjager force-pushed the payment-metadata branch 2 times, most recently from 3a656ce to 1c34595 Compare February 4, 2022 10:49

joostjager requested a review from carlaKC February 4, 2022 10:52

Roasbeef added the P3 might get fixed, nice to have label Feb 14, 2022

joostjager force-pushed the payment-metadata branch from 1c34595 to 348b9ed Compare March 24, 2022 13:37

Roasbeef requested changes Apr 12, 2022

View reviewed changes

joostjager force-pushed the payment-metadata branch 3 times, most recently from 9fd64ab to 52cecfa Compare April 13, 2022 11:16

joostjager requested a review from Roasbeef April 13, 2022 11:22

joostjager force-pushed the payment-metadata branch from 52cecfa to 3730002 Compare April 13, 2022 11:23

Roasbeef moved this from Review in progress to Reviewer approved + active testing in v0.15.0-beta Apr 13, 2022

joostjager added 3 commits April 13, 2022 22:55

zpay32: add payment metadata field

135e27d

routing+channeldb: send payment metadata from invoice

9195f29

htlcswitch+invoices: log payment metadata

62ae038

joostjager force-pushed the payment-metadata branch from 3730002 to 62ae038 Compare April 13, 2022 20:55

Roasbeef approved these changes Apr 13, 2022

View reviewed changes

Roasbeef merged commit 440f4d9 into lightningnetwork:master Apr 13, 2022

v0.15.0-beta automation moved this from Reviewer approved + active testing to Done Apr 13, 2022

Roasbeef mentioned this pull request Apr 13, 2022

invoices: implement statelss invoice functinoality #6413

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

routing: send payment metadata #5810

routing: send payment metadata #5810

joostjager commented Sep 30, 2021 •

edited

Loading

joostjager Sep 30, 2021

Roasbeef Sep 30, 2021

joostjager Nov 2, 2021

joostjager Nov 2, 2021 •

edited

Loading

joostjager Nov 22, 2021

Roasbeef Apr 12, 2022

Roasbeef Apr 12, 2022

joostjager Apr 13, 2022

carlaKC left a comment

carlaKC Jan 4, 2022

joostjager Jan 11, 2022 •

edited

Loading

carlaKC Jan 4, 2022

joostjager Jan 11, 2022 •

edited

Loading

carlaKC Feb 3, 2022

joostjager Feb 4, 2022

joostjager commented Jan 11, 2022

joostjager commented Jan 19, 2022

carlaKC left a comment

carlaKC Feb 3, 2022

joostjager Feb 4, 2022

carlaKC Feb 3, 2022

joostjager commented Mar 24, 2022

lightninglabs-deploy commented Apr 7, 2022

Roasbeef left a comment

Roasbeef Apr 12, 2022

joostjager Apr 13, 2022

Roasbeef Apr 12, 2022

Roasbeef Apr 12, 2022

joostjager commented Apr 13, 2022

Roasbeef commented Apr 13, 2022

Roasbeef left a comment

routing: send payment metadata #5810

routing: send payment metadata #5810

Conversation

joostjager commented Sep 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joostjager Nov 2, 2021 • edited Loading

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

carlaKC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joostjager Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joostjager Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joostjager commented Jan 11, 2022

joostjager commented Jan 19, 2022

carlaKC 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

joostjager commented Mar 24, 2022

lightninglabs-deploy commented Apr 7, 2022

Roasbeef 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

joostjager commented Apr 13, 2022

Roasbeef commented Apr 13, 2022

Roasbeef left a comment

Choose a reason for hiding this comment

joostjager commented Sep 30, 2021 •

edited

Loading

joostjager Nov 2, 2021 •

edited

Loading

joostjager Jan 11, 2022 •

edited

Loading

joostjager Jan 11, 2022 •

edited

Loading