Everyone pileon modifications to bolt12 by rustyrussell · Pull Request #7 · rustyrussell/bolts

rustyrussell · 2022-08-16T10:41:48Z

Please leave feedback...

1. final node_id in blinded path cannot be the real node_id, but it's still useful if you don't care about node_id. 2. blinded_capacities is redundant since htlc amounts are in blinded_payinfo. 3. Similarly, cltv. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Use first field, arrange for it to be `payer_info`. This makes it possible to calculate the merkle (in theory, at least), in a single pass. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

…invoice. This mainly involves renaming fields but also: 1. Renumber, so 2 - 127 are offers, 128-192 are invoice_request. That makes for simply checks that offers aren't trying to sneak in new invoice_request fields etc. 2. Removes the concept of offer_id. 3. Add offer_metadata blob so you can do stateless offers more easily than having to insert data in a blinded path. 4. We do insert offer_node_id if it's implied by offer. 5. Feature fields have to be separate for each one. 6. We are no longer allowed to change description between offer and invoice (we can add a new "annotation" field if we wanted). Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

t-bast · 2022-08-16T11:07:12Z

blinded_capacities is redundant since htlc amounts are in blinded_payinfo.

What do you mean here? blinded_payinfo contains an hltc_maximum_msat field, but that's different from the path capacity. We could for example have a blinded path where:

htlc_maximum_msat is set to 10 mbtc
the available capacity is 50 mBTC
the payment we expect is 25 mBTC

The sender should ideally send three HTLCs through this blinded path for the payment to succeed.
I'm in favor of adding the capacity field to blinded_payinfo directly, instead of using a separate tlv.

I'll let @thomash-acinq review the other changes and provide feedback.

12-offer-encoding.md

carlaKC · 2022-08-16T13:54:16Z

12-offer-encoding.md

 ## Requirements for Invoice Requests

 The writer of an invoice_request:
+  - SHOULD set `payer_info` to an unpredictable series of bytes.


Why should over must? If we want "an unpredictable nonce for hashing" and we don't include payer_info, our chain tlv will be used, which is pretty predictable?

If you don't care about privacy? Should we say MUST?

I set this to the data I need to recover the payer_key from my node_id, which (1) means I don't need to remember it, but can reconstruct from any invoice and (2) it's unpredictable to anyone else.

I meant populating the payer_info in general, rather than the value that we have in it:

MUST set payer_info to a non-zero series of bytes

SHOULD set payer_info to an unpredictable series of bytes

Reasoning being that if we're going to use first-tlv in nonce calculation, not setting payer_info (which this wording allows) would mean you use the predictable offer_chains tlv?

(somewhat relevant to this comment about specifying using invoice_request_payer_info rather than just "hoping it's there")

12-offer-encoding.md

rustyrussell · 2022-08-16T23:38:43Z

blinded_capacities is redundant since htlc amounts are in blinded_payinfo.

What do you mean here? blinded_payinfo contains an hltc_maximum_msat field, but that's different from the path capacity. We could for example have a blinded path where:
* `htlc_maximum_msat` is set to 10 mbtc

* the available capacity is 50 mBTC

* the payment we expect is 25 mBTC
The sender should ideally send three HTLCs through this blinded path for the payment to succeed. I'm in favor of adding the capacity field to blinded_payinfo directly, instead of using a separate tlv.

IMO, the purpose of htlc_max_msat is to limit HTLC exposure, it's very antisocial to use multiple HTLCs of this value at once. I think the sender needs to set a value which is, if possible, a reflection of known capacity estimates, and also <= the htlc_max of any hop. Keeps it to a simple "don't send more than this down this path".

Only non-textual change is that invoice_payer_info is now type 0, not 1. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

I lazily said "reuse bolt11 features", but all of them are currently assumed or don't apply. Create a new marker, currently unused. Also make sure we *check* them everywhere, and clarify use of `features` in `blinded_payinfo`. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

It's cool, but better to add later (esp since nobody has implemented it). You can do a specific offer to a user for now. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

An obvious candidate for a new feature bit "I offer invoice replacement!". Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Now we've cleaned the fields, renumber consecutively into: * offer: 1 - 79 incl * invoice_request: 80 - 159 incl * invoice: 160 - 239 incl And clearly rename invoice_request fields to invoice_request_X. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

12-offer-encoding.md

jkczyz · 2022-08-17T15:05:37Z

12-offer-encoding.md

+    - MUST set `invoice_request_payer_info`
+	  - SHOULD set it to at least 16 random bytes.


Could we describe this more either here or in the rationale, assuming it differs from the same field in invoice_request?

Actually why is it allowed to touch this field? Shouldn't it be copied as-is from the invoice request?

There's no invoice_request in this case (above it says: - otherwise (responding to a offer_send_invoice offer):).

From a high level, an 'send_invoice' offer is an invoice request. Now invoice_request has all the offer fields, do we just, y'know, use it directly instead of having this weird kind of offer? I need to think through this...

12-offer-encoding.md

thomash-acinq

I don't have objections to these changes but I'm not convinced that they improve anything either.

12-offer-encoding.md

thomash-acinq · 2022-08-17T15:55:49Z

12-offer-encoding.md

+    - MUST set `invoice_request_payer_info`
+	  - SHOULD set it to at least 16 random bytes.


Actually why is it allowed to touch this field? Shouldn't it be copied as-is from the invoice request?

valentinewallace · 2022-08-17T18:56:54Z

09-features.md

 * `C-`: presented in the `channel_announcement` message, but always odd (optional).
 * `C+`: presented in the `channel_announcement` message, but always even (required).
 * `9`: presented in [BOLT 11](11-payment-encoding.md) invoices.
+* `2`: presented in [BOLT 12](12-offer-encoding.md) messages.


Is MPP support assumed in offers? LDK likely wants to support phantom invoices for offers, which don't support MPP..

I'm not sure what phantom invoices are, but yes, MPP is assumed?

Ah, LDK will need MPP support to be optional in invoice OMs at least.. I'm not sure the best way of doing this, maybe it would be easiest to put the MPP feature bit in offer features (though it has a dependency on the payment_secret feature currently, which is kinda awkward)?

Wait, how do you not support MPP? In 2022, that seems like more work than supporting it everywhere (we don't send MPP for keysend, but of course we have no problem receiving it...).

Heh, we support MPP for regular invoices. But for multi-node receive invoices where the payment can be received by any backing node, we don't support MPP (because the nodes would have to coordinate with each other before releasing the preimage, which is not supported).

I don't buy this at all - its not really all that clear that MPP is really important for small-medium payments today, and skipping it can be very useful for small-medium payments today, as Val points out. For larger payments, indeed, its definitely required, but that's a separate issue (and actually, offers is really great there - the recipient can change the features based on the payment size).

Frankly, if we can't get an MPP feature bit we'll probably not realistically ship BOLT12, cause phantom invoices is a pretty useful feature for large scale deployments.

It seems that what you want is to force the sender to send everything to the same blinded path. Disabling MPP completely is a crude way of doing it, the sender could still split the payment as long as all the part have the same destination.
If you want to enable a new design, you should add a new feature for it, not disable an existing feature that's considered part of the base package.

I'm not sure who determines what is a "part of the base package", but it's not clear to me MPP provides any value for small-medium payments. Indeed, eventually the solution to enable seamless failover without trusting a coordinator would be trampoline, but in the mean time things don't seem to break.

It seems that what you want is to force the sender to send everything to the same blinded path.

FYI, see the following for full context: https://lightningdevkit.org/blog/introducing-phantom-node-payments/

The key point is being able to retry on a different path (i.e., failover as @TheBlueMatt mentioned) without coordination between nodes.

12-offer-encoding.md

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell · 2022-08-18T04:56:02Z

I don't have objections to these changes but I'm not convinced that they improve anything either.

But we finally got rid of the "LnAll" string constant you hated!

12-offer-encoding.md

1. Remove the "offer_node_id is optional". It's messy. Put it in even if it's a temporary key. 2. Note why we use 0 for the invoice_request_payer_info. 3. Use SHA256 instead of 256-bit SHA-2, as everyone knows what SHA256 means here; so much that the latter is confusing. 4. Indent fix. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

@thomash-acinq

@thomash-acinq points out that changing invoice_request_amount is a hack. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

12-offer-encoding.md

valentinewallace · 2022-09-02T02:51:10Z

12-offer-encoding.md


 Invoice Requests are a request for an invoice; the human-readable prefix for
-invoices is `lnr`.
+invoices is `lnr`.  It mirrors all the fields from the offer, except


Seems the current spec doesn't support custom TLVs in offers/invoice(_req)s? May need to add a type range for each message's custom tlvs to be in. Otherwise we can't tell whether an inbound custom TLV is being reflected back to us or was added by the counterparty, IIUC.

As written, custom things would have to be in the corresponding ranges if you want them to be sent back to you.

If we want more room, we can either:

Add more ranges, eg. 1000-1999, 2000-2999, 3000-3999.

Do generic interleaving "if > 1000, mod 6 (0/1 == offer, 2/3 == invreq, 4/5 == invoice)".

We could do generic interleaving on these fields, too (damn, renumber again!).

Add more ranges, eg. 1000-1999, 2000-2999, 3000-3999.

ACK on this 👍 the other ones sound a bit weird but I'm fine as long as there's some dedicated space somewhere

12-offer-encoding.md

* Make a note about TLV ranges. * rename invoice_request_payer_info to invoice_request_metadata. * Make invoice_request_chain's "bitcoin default" explicit. * Renumber to place invoice_relative_expiry next to invoice_created_at, close gap in numbering. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell added 4 commits August 16, 2022 15:06

BOLT 12: remove previously-optional signature from offers.

4eb8b86

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

BOLT 12: simplify nonce calculation

872c1c6

Use first field, arrange for it to be `payer_info`. This makes it possible to calculate the merkle (in theory, at least), in a single pass. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

jkczyz reviewed Aug 16, 2022

View reviewed changes

12-offer-encoding.md Show resolved Hide resolved

12-offer-encoding.md Outdated Show resolved Hide resolved

12-offer-encoding.md Outdated Show resolved Hide resolved

12-offer-encoding.md Show resolved Hide resolved

12-offer-encoding.md Outdated Show resolved Hide resolved

carlaKC reviewed Aug 16, 2022

View reviewed changes

rustyrussell added 5 commits August 17, 2022 09:23

Excellent feedback from @jkczyz

8290399

Only non-textual change is that invoice_payer_info is now type 0, not 1. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

BOLT 12: remove refund payer proof.

28191db

It's cool, but better to add later (esp since nobody has implemented it). You can do a specific offer to a user for now. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

BOLT 12: remove invoice replacement.

721ccb9

An obvious candidate for a new feature bit "I offer invoice replacement!". Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

BOLT 12: renumber fields.

0dab28d

Now we've cleaned the fields, renumber consecutively into: * offer: 1 - 79 incl * invoice_request: 80 - 159 incl * invoice: 160 - 239 incl And clearly rename invoice_request fields to invoice_request_X. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

jkczyz reviewed Aug 17, 2022

View reviewed changes

thomash-acinq reviewed Aug 17, 2022

View reviewed changes

valentinewallace reviewed Aug 17, 2022

View reviewed changes

12-offer-encoding.md Show resolved Hide resolved

jkczyz reviewed Aug 18, 2022

View reviewed changes

12-offer-encoding.md Outdated Show resolved Hide resolved

BOLT 12: even more feedback and fixes.

93438c9

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

thomash-acinq reviewed Aug 18, 2022

View reviewed changes

12-offer-encoding.md Outdated Show resolved Hide resolved

12-offer-encoding.md Outdated Show resolved Hide resolved

valentinewallace reviewed Aug 18, 2022

View reviewed changes

12-offer-encoding.md Show resolved Hide resolved

valentinewallace reviewed Aug 19, 2022

View reviewed changes

12-offer-encoding.md Outdated Show resolved Hide resolved

12-offer-encoding.md Outdated Show resolved Hide resolved

12-offer-encoding.md Show resolved Hide resolved

12-offer-encoding.md Show resolved Hide resolved

12-offer-encoding.md Show resolved Hide resolved

valentinewallace reviewed Aug 19, 2022

View reviewed changes

12-offer-encoding.md Show resolved Hide resolved

12-offer-encoding.md Outdated Show resolved Hide resolved

12-offer-encoding.md Show resolved Hide resolved

rustyrussell added 3 commits August 19, 2022 12:57

BOLT 12: add invoice_amount explicitly.

9124f3c

@thomash-acinq points out that changing invoice_request_amount is a hack. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

BOLT 12: make it clear that unhashed elements are 240-1000 *inclusive*.

75d9e05

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell changed the title ~~BlueMatt modifications to bolt12~~ Everyone pileon modifications to bolt12 Aug 19, 2022

jkczyz reviewed Aug 24, 2022

View reviewed changes

12-offer-encoding.md Show resolved Hide resolved

12-offer-encoding.md Outdated Show resolved Hide resolved

jkczyz mentioned this pull request Sep 1, 2022

BOLT 12 offer encoding lightningdevkit/rust-lightning#1597

Closed

8 tasks

valentinewallace reviewed Sep 1, 2022

View reviewed changes

12-offer-encoding.md Outdated Show resolved Hide resolved

valentinewallace reviewed Sep 2, 2022

View reviewed changes

carlaKC reviewed Sep 2, 2022

View reviewed changes

12-offer-encoding.md Show resolved Hide resolved

12-offer-encoding.md Show resolved Hide resolved

jkczyz reviewed Sep 6, 2022

View reviewed changes

12-offer-encoding.md Outdated Show resolved Hide resolved

jkczyz reviewed Sep 11, 2022

View reviewed changes

12-offer-encoding.md Show resolved Hide resolved

rustyrussell merged commit 3327d28 into guilt/offers Sep 12, 2022

		- MUST set `invoice_request_payer_info`
		- SHOULD set it to at least 16 random bytes.

Conversation

rustyrussell commented Aug 16, 2022

Uh oh!

t-bast commented Aug 16, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rustyrussell commented Aug 16, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thomash-acinq left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rustyrussell commented Aug 18, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!