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

blip-xx: BOLT 11 Invoice Blinded Path Tagged Field #1

Closed
wants to merge 1 commit into from

Conversation

ellemouton
Copy link
Owner

No description provided.

Copy link

@ziggie1984 ziggie1984 left a comment

Choose a reason for hiding this comment

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

I like this idea of using a bLIP for the optional Blinded Path Feature for BOLT 11 invoices, at until BOLT12 is widely adopted.

blip-00xx.md Outdated
* [`2*byte`:`cipher_text_length`]
* [`cipher_text_length*byte`:`cipher_text`]

The `blinded_node_pubkey` of the first `blinded_hop` in a `blinded_payinfo` is

Choose a reason for hiding this comment

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

Why do we need to do it this way, meaning that although the field is called blinded, we use the real pubkey ?

Copy link
Owner Author

Choose a reason for hiding this comment

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

it's just for the very first hop (the intro node) and is done as a way to save space. Else we would need a whole separate 33 byte field for the intro node pub key. The intro node's blinded pub key is never used.

I can add this explanation though if it is unclear 👍

The other option is of course just special casing the first hop's encoding and letting it only contain cipher text but I opted for a simpler encoding here

Choose a reason for hiding this comment

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

I think carving out the intro node's real key into its own field in the root blinded_payinfo structure is a better way to organize this so that we have full homogeneity within the blinded_hop vector. I don't think space is saved either way since any data that we include in the main message wouldn't be included in blinded_hops.

Having exceptions, asterisks, or other caveats makes the spec harder to implement correctly.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I don't think space is saved either way

space saved in the invoice itself though :) ie trying to make it still fit in a qr code.

but yeah - can change this to be more explicit if enough people agree. Just seems strange to include the intro node's blinded key when that is never used

Choose a reason for hiding this comment

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

No I mean why not omit the intro node's "hop" from blinded_hops completely, and then just include the intro node's ciphertext in its own field?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No I mean why not omit the intro node's "hop" from blinded_hops completely, and then just include the intro node's ciphertext in its own field?

IIUC it's done this way to mirror the Offers encoding.

Copy link
Owner Author

Choose a reason for hiding this comment

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

ah ok i see what you mean now, like a separate intro_hop and intro_encrypted_data.

The only arg i see against this is that then the num_hops isnt really the true number of hops. But yeah happy to change this. I think what i'll do is leave it as is for now and then will add this note to the "discussions/questions" section for discussion.

blip-00xx.md Outdated
1. subtype: `blinded_hop`
2. data:
* [`33*byte`:`blinded_node_pubkey`]
* [`2*byte`:`cipher_text_length`]

Choose a reason for hiding this comment

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

can we restrict the cipher_text length field to 2 bytes because we have the data_length limit anyways ? Normally the tlv length values are [bigsize: length] ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unless we make a custom carve out here, it'll be the BigSize encoding. Either way though, if the payload is under 253 bytes, then we can encode with only a single byte.

Copy link
Owner Author

Choose a reason for hiding this comment

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

originally I was just mimicking what is used for onionmsg_hop defined in the Offers spec

But I think you are right in that bigsize is better for this especially since the estimated size for this is around 79 bytes (see calc below) and so with bigsize, we then dont need that 2 byte overhead in most cases 👍

blip-00xx.md Outdated
* 8 byte `htlc_minimum_msat`
- `allowed_features`: optional

If we assume that the `allowed_features` vector is not set, then this comes to

Choose a reason for hiding this comment

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

Maybe add a reference to the BOLT04 spec, where there is a reference that the feature vector is currently not used ?

Copy link
Owner Author

Choose a reason for hiding this comment

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

added 👍

blip-00xx.md Outdated
- `path_id`: let's assume that this is 32 bytes like the existing payment
address.
- `payment_constraints`:
* 4 byte `max_cltv_expiry`

Choose a reason for hiding this comment

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

still unsure why the receiver needs the payment_contraint data, it created the invoice in the first place ?

Copy link
Owner Author

Choose a reason for hiding this comment

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

so i think the idea here is so that the recipient doesnt need to go store this expiry as a separate thing. It basically just sends this message to itself. Got the idea from the example used in the proposal doc

Copy link
Owner Author

Choose a reason for hiding this comment

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

defs an open discussion though. Noted in the missing pieces part here: lightningnetwork/lnd#8752 (comment)

blip-00xx.md Outdated

The total number of bytes required for a single `blinded_hop` is:

= 33+2+len(cipher_text)

Choose a reason for hiding this comment

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

don't we need to add the padding value here or is it already included in the cipher_text.

Copy link
Owner Author

Choose a reason for hiding this comment

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

see the ##### Cipher Text Size Estimation section above - the cipher text will already contain the padding

blip-00xx.md Outdated
- Does it make sense to add an experimental tagged field range for BOLT 11 in
the bLIP2 spec or would this be over-kill since BOLT 11 is in any-case being
phased out?
- Would a new experimental feature bit (in the `I` context) make sense here so

Choose a reason for hiding this comment

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

I think this is a good idea, adding a new feature bit for the bolt11 blinded path case.

Choose a reason for hiding this comment

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

Seconded. Fail fast.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, it should be a required feature bit.

blip-00xx.md Outdated
the bLIP2 spec or would this be over-kill since BOLT 11 is in any-case being
phased out?
- Would a new experimental feature bit (in the `I` context) make sense here so
that the reader can fail fast instead of attempting to find a node that does

Choose a reason for hiding this comment

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

Should we include some test-vectors for the invoice with a blinded path, I think we have them ready in the LND PR I think ?

Copy link
Owner Author

Choose a reason for hiding this comment

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

ah indeed - good idea. Will add

blip-00xx.md Outdated
- MUST not contain the `s` field type since a payment address in the context of
blinded payments does not make sense since the recipient is able to use the
`path_id` in the `encrypted_recipient_data` for the same purpose.
- SHOULD sign the invoice with a private key that is not the same as their

Choose a reason for hiding this comment

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

Maybe a dumb question but what's the point of signing the data if we don't have a key to validate against?

Copy link
Owner Author

Choose a reason for hiding this comment

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

well we do have the random key in the "destination" field. so we can atleast verify that "invoice hasnt been changed since original creation", ie, no-one was able to take the invoice that looks like it is coming from you but then quickly sneak their own blinded paths to themselves in it.

Perhaps could do something fancier like: the sig is the n-of-n musig2 combo created by combining sigs from all the final blinded pubkeys in the n blinded paths the invoice contains ...?

Choose a reason for hiding this comment

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

Hypothetically you could just craft an entirely new invoice though and sign it with a random privkey. So maybe it guards against a super naive tampering but even a modestly intentional tampering is still possible, no? Maybe I'm not understanding completely (I haven't read BOLT11 end-to-end).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah this came up in the spec meeting the other week. I think some plan to use this key in a sort of "address book" entry. It doesn't need to be your actual node's public key, and you can rotate it, but it can be useful as a pseudonym on the network.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The signature lets us verify the integrity of the invoice (nothing has been tampered with). As for authentication, when dealing with hop hints, it's already the case today that the public key included won't be in the public graph, so you don't have a way to verify that the node actually "exists".

blip-00xx.md Outdated
* [`2*byte`:`cipher_text_length`]
* [`cipher_text_length*byte`:`cipher_text`]

The `blinded_node_pubkey` of the first `blinded_hop` in a `blinded_payinfo` is

Choose a reason for hiding this comment

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

I think carving out the intro node's real key into its own field in the root blinded_payinfo structure is a better way to organize this so that we have full homogeneity within the blinded_hop vector. I don't think space is saved either way since any data that we include in the main message wouldn't be included in blinded_hops.

Having exceptions, asterisks, or other caveats makes the spec harder to implement correctly.

blip-00xx.md Outdated
This proposal is a temporary measure that will allow users to start making use
of blinded paths in the context of payments and thereby taking advantage of the
potential privacy and payment success rate benefits that in they will in theory
provide. Once the Offers protocol along with its new invoice format has been

Choose a reason for hiding this comment

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

link Offers

Copy link
Owner Author

Choose a reason for hiding this comment

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

cool can add - tbh my assumption was always that if you've linked something once then you dont need to link it again so as not to litter the doc with blue links? but happy to update - just had this assumption

blip-00xx.md Outdated
- Does it make sense to add an experimental tagged field range for BOLT 11 in
the bLIP2 spec or would this be over-kill since BOLT 11 is in any-case being
phased out?
- Would a new experimental feature bit (in the `I` context) make sense here so

Choose a reason for hiding this comment

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

Seconded. Fail fast.

blip-00xx.md Outdated
useful tool for privacy and potentially more reliable payment delivery and so
this document proposes a carve-out to the existing [BOLT 11] invoice format so
that advantage can be taken of the blinded paths protocol in the context of
payments in a sooner time frame. This will be done by adding a new tagged-field
Copy link

Choose a reason for hiding this comment

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

nit: in a quick time frame

blip-00xx.md Outdated

Another soft maximum value to keep in mind is the maximum number of bytes that
can fit into a [QR code][qr] which is 2,953 bytes. This is a soft maximum
because this only applies if the invoice is in fact being transmitted via QR
Copy link

Choose a reason for hiding this comment

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

Question: How is this limit being used in this proposal? Is it enforced in anyway to limit the data which can be included?

Given that this constraint is applicable only if the invoice is being displayed as a QR code, would it make sense to add an explicit logic to indicate modality of invoice transmission and enforce the limits based on that?

Copy link
Owner Author

Choose a reason for hiding this comment

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

How is this limit being used in this proposal?

It's not. It's just informative. This is purely for implementation help.

add an explicit logic to indicate modality of invoice transmission and enforce the limits based on that

That is really up to the implementation I'd say

@ellemouton ellemouton force-pushed the bolt11-blinded-path branch from 57b87c1 to 8c3f40a Compare June 25, 2024 23:55
Copy link
Owner Author

@ellemouton ellemouton left a comment

Choose a reason for hiding this comment

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

Thanks for taking a look y'all! 🙏

Pushed up some changes. Gonna post this to delving now to get the broader discussion going. I'll add test vectors for the actual bLIP PR 👍

blip-00xx.md Outdated
1. subtype: `blinded_hop`
2. data:
* [`33*byte`:`blinded_node_pubkey`]
* [`2*byte`:`cipher_text_length`]
Copy link
Owner Author

Choose a reason for hiding this comment

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

originally I was just mimicking what is used for onionmsg_hop defined in the Offers spec

But I think you are right in that bigsize is better for this especially since the estimated size for this is around 79 bytes (see calc below) and so with bigsize, we then dont need that 2 byte overhead in most cases 👍

blip-00xx.md Outdated
* [`2*byte`:`cipher_text_length`]
* [`cipher_text_length*byte`:`cipher_text`]

The `blinded_node_pubkey` of the first `blinded_hop` in a `blinded_payinfo` is
Copy link
Owner Author

Choose a reason for hiding this comment

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

ah ok i see what you mean now, like a separate intro_hop and intro_encrypted_data.

The only arg i see against this is that then the num_hops isnt really the true number of hops. But yeah happy to change this. I think what i'll do is leave it as is for now and then will add this note to the "discussions/questions" section for discussion.

blip-00xx.md Outdated
* 8 byte `htlc_minimum_msat`
- `allowed_features`: optional

If we assume that the `allowed_features` vector is not set, then this comes to
Copy link
Owner Author

Choose a reason for hiding this comment

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

added 👍

blip-00xx.md Outdated
- `path_id`: let's assume that this is 32 bytes like the existing payment
address.
- `payment_constraints`:
* 4 byte `max_cltv_expiry`
Copy link
Owner Author

Choose a reason for hiding this comment

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

defs an open discussion though. Noted in the missing pieces part here: lightningnetwork/lnd#8752 (comment)

blip-00xx.md Outdated

The total number of bytes required for a single `blinded_hop` is:

= 33+2+len(cipher_text)
Copy link
Owner Author

Choose a reason for hiding this comment

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

see the ##### Cipher Text Size Estimation section above - the cipher text will already contain the padding

blip-00xx.md Outdated
This proposal is a temporary measure that will allow users to start making use
of blinded paths in the context of payments and thereby taking advantage of the
potential privacy and payment success rate benefits that in they will in theory
provide. Once the Offers protocol along with its new invoice format has been
Copy link
Owner Author

Choose a reason for hiding this comment

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

cool can add - tbh my assumption was always that if you've linked something once then you dont need to link it again so as not to litter the doc with blue links? but happy to update - just had this assumption

@ellemouton ellemouton force-pushed the bolt11-blinded-path branch from 8c3f40a to 4803947 Compare June 26, 2024 00:02
@ellemouton ellemouton force-pushed the bolt11-blinded-path branch from 98ea720 to d4e6787 Compare July 8, 2024 11:18
@ellemouton
Copy link
Owner Author

closing here & opening on the upstream repo - let's continue discussion their 🙏

@ellemouton ellemouton closed this Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants