Fixing bugs & shortcomings in OfferPaymentMetadata.V1

[robbiehanson]

There are a few bugs in OfferPaymentMetadata.V1. For example, say we generate an offer with the description: "Pizza 🍕":

The description may end up withinOfferPaymentMetadata.V1 as a payerNoter. This is incorrect:

It's not a payerNote, and the UI needs to understand this. Further, if the user does add a payerNote, then the original description will be lost. Which may be important information for the user. For example, the original description may have been "Invoice #: 472648"

General idea

data class V1(
    override val offerId: ByteVector32,
    override val amount: MilliSatoshi,
    override val preimage: ByteVector32,
    val payerKey: PublicKey,
    val payerNote: String?,
    val quantity: Long,
    val createdAtMillis: Long
)

data class V2(
    override val offerId: ByteVector32,
    override val amount: MilliSatoshi,
    override val preimage: ByteVector32,
    override val createdAtSeconds: Long,
    override val relativeExpirySeconds: Long?,
    val description: String?,
    val payerKey: PublicKey?,
    val payerNote: String?,
    val quantity: Long?,
)

Optimizations

There are several optimizations we can perform to shrink the size of the encoded metadata:

• the quantity field can be null (to match quantity_opt). This saves us 8 bytes in the common case.
• we can store the timestamp in seconds instead of milliseconds. This matches the timestamp fields in the Bolt12 invoice. And allows us to use bigSize, which shrinks the size from a fixed 8 bytes to 5 bytes (at least for another ~100 years).
• the amount: MilliSatoshi can be stored as bigSize, which shrinks the size from a fixed 8 bytes, to an average of 5 bytes. It only uses more bytes if the payment amount is over ~0.043 BTC, and most payments are under this amount.

The end result is that V2 is smaller than V1.

Future-proofing

I'm experimenting in another branch (card-payment), and I need to create a Bolt12Invoice that expires in 30 seconds. But that's wasn't possible, because the relativeExpiry wasn't stored in OfferPaymentMetadata.V1, and the default value was hard-coded. So I fixed that issue.

Also in the branch, I'm experimenting with creating an "unsolicited Bolt 12 invoice". That is, a Bolt 12 Invoice that is generated not in response to an InvoiceRequest, but due to an attempted card payment. As such, it does not have a payerKey. So I've made that field optional in V2.

Note: Making it optional takes up no extra bytes.

Alternative implementations

This PR fixes all the issues I have with V1, but it may not be the ideal solution. Ultimately, one of the core issues I see is that, while LightningOutgoingPayment stores the full Bolt12Invoice, Bolt12IncomingPayment only stores the OfferPaymentMetadata.

Perhaps the ideal solution would be:

• Make Bolt12IncomingPayment store the full Bolt12Invoice
• Change OfferPaymentMetadata to be simply a signature over a subset of the offer_* & invoice_* fields. And verify the sig when receiving the payment part(s).

However... that's a very big change. Implementing it will take a lot of effort. And we're all busy with other big changes at the moment. So in the meantime, I'm proposing the minor changes in V2.

[thomash-acinq]

Nice improvement

[t-bast]

Please don't merge this PR immediately, I'd like to take a look at it as well!

[t-bast]

Perhaps the ideal solution would be:

• Make Bolt12IncomingPayment store the full Bolt12Invoice
• Change OfferPaymentMetadata to be simply a signature over a subset of the offer_* & invoice_* fields. And verify the sig when receiving the payment part(s).

We cannot do that, because we cannot store the actual invoice, otherwise we open the door to trivial DoS (this is explained in the comment on the OfferPaymentMetadata class). That's why we had to introduce this OfferPaymentMetadata class in the first place, to keep the important parts of the invoice without being exposed to DoS.

It's not a payerNote, and the UI needs to understand this. Further, if the user does add a payerNote, then the original description will be lost.

Correct, we initially decided to only keep one of the two fields because it's very important to limit the size of this encrypted blob: it needs to be included in the payment onion that the sender creates, which is limited to 1300 bytes. Every byte we use for this encrypted blob cannot be used by the payer to add hops in the payment route or data for routing nodes. This can potentially make payments fail because the payer doesn't have enough bytes left for the route it wants to take, which would be really bad.

But I agree that it's a bad UX to silently replace the description by the payer note...to minimize the impact of that change, would it be acceptable if when both a payer note and an offer description are provided, we limit each to 32 bytes (instead of the current 64)?

Good job on the flags to mark nullable fields, I think that's a very good idea, we didn't do it in the first version because we wanted to ship quickly, but definitely worth introducing now! Another improvement for the size is to stop using a signature entirely and instead simply encrypt the data: see #719 where I did this for Bolt 12 contacts (which I hope will eventually be accepted in the spec, I'm quite annoyed that we still cannot ship it). Can you change your code to use this encryption scheme instead? If you don't have time in the short term, I can spend some time on this next week and add a commit on top of your work if you prefer.

[robbiehanson]

when both a payer note and an offer description are provided, we limit each to 32 bytes (instead of the current 64)?

Done in 2946b6c & 8c13942

The maximum length between the two fields is now limited to 64 bytes. This was slightly trickier than expected since string.length != utf8Encoding.length.

[robbiehanson]

Another improvement for the size is to stop using a signature entirely and instead simply encrypt the data

Done in e81d0f9

I copied what you did in #719

Bolt 12 contacts (which I hope will eventually be accepted in the spec, I'm quite annoyed that we still cannot ship it)

Me too. It's a much needed improvement for the UI.

[t-bast]

LGTM, I've done some refactoring and bug fixing in added commits (see commit messages for details), it's ready to go 👍, I will merge once the CI passes.