Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

routing: send payment metadata #5810

Merged
merged 3 commits into from
Apr 13, 2022

Conversation

joostjager
Copy link
Contributor

@joostjager joostjager commented Sep 30, 2021

Implements lightning/bolts#912

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.

record/hop.go Outdated

// MetadataOnionType is the type used in the onion for the payment
// metadata.
MetadataOnionType tlv.Type = 10
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

@Roasbeef Roasbeef added this to the v0.15.0 milestone Sep 30, 2021
@joostjager joostjager force-pushed the payment-metadata branch 2 times, most recently from 904ce49 to bc28d7e Compare November 2, 2021 10:06
@@ -938,6 +938,7 @@ func (i *InvoiceRegistry) NotifyExitHopHtlc(rHash lntypes.Hash,
customRecords: payload.CustomRecords(),
mpp: payload.MultiPath(),
amp: payload.AMPRecord(),
metadata: payload.Metadata(),
Copy link
Contributor Author

@joostjager joostjager Nov 2, 2021

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

Minimal change for a nifty feature!

@@ -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 " +
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@joostjager joostjager Jan 11, 2022

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

@joostjager joostjager Jan 11, 2022

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

htlcswitch/hop/payload.go Show resolved Hide resolved
invoices/interface.go Show resolved Hide resolved
@joostjager joostjager changed the title routing: payment metadata routing: send payment metadata Jan 11, 2022
@joostjager
Copy link
Contributor Author

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 joostjager force-pushed the payment-metadata branch 2 times, most recently from e9585cd to c2fa9aa Compare January 11, 2022 14:54
@joostjager joostjager force-pushed the payment-metadata branch 2 times, most recently from cb00b8f to f67bc51 Compare January 19, 2022 11:27
@joostjager
Copy link
Contributor Author

Added test coverage

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

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

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

zpay32/encode.go Outdated Show resolved Hide resolved
@@ -726,6 +751,10 @@ func TestDecodeEncode(t *testing.T) {
}

if decodedInvoice != nil {
if test.implicitDest {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
routing/pathfind_test.go Show resolved Hide resolved
@joostjager joostjager force-pushed the payment-metadata branch 2 times, most recently from 3a656ce to 1c34595 Compare February 4, 2022 10:49
@Roasbeef Roasbeef added the P3 might get fixed, nice to have label Feb 14, 2022
@joostjager
Copy link
Contributor Author

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
Copy link

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

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

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.

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

writeBytes32 can be used here instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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?

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

Choose a reason for hiding this comment

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

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

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

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
Copy link
Member

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.

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 💐

@Roasbeef Roasbeef merged commit 440f4d9 into lightningnetwork:master Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 might get fixed, nice to have
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants