-
Notifications
You must be signed in to change notification settings - Fork 493
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
Offers #798
base: master
Are you sure you want to change the base?
Offers #798
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gave an initial read, really like the concept.
With the new developments in miniscript and descriptors the need for invoices is more generic than simple LN: maybe some common format solving bech23 and miniscript use problems as well?
As you probably remember, I am working on RGB: assets & smart contracts over bitcoin and LN with Peter Todd client-side validation paradigms (not tx space use, pure L2/L3) backed by Bitfinex and USDT. In this regard we need shared extensible multi-asset invoice format, preferably with confidential asset support as well.
c0aae68
to
19235c4
Compare
07c45e9
to
c918789
Compare
9f129f2
to
9c526ff
Compare
efdcb71
to
781004a
Compare
e02c7d8
to
4d0d10a
Compare
4d0d10a
to
fc8aab7
Compare
I have merged the million-fixup-commits into one commit, and trivially rebased onto the updated #759. The last two commits are new:
|
I have a question with respect to replacing invoices and the fact that we can proof if the payee claimed both htlcs: What is the use of that? As far as I understand we could sue the person but this won't give us a cryptographic guarantee to get our money back as even a convicted person might have run or just have already spent the money. Or do I miss something? The only fix that cryptographically would give us a guarantee to not pay twice that I can think of only seems to shift the problem of stuck payments (and would thus create a burden to the network) and is the following:
Now we have 2 scenarios:
As said initially: I don't think the reimbursement path (which is by the way inspired by the boomerang paper c.f.: https://arxiv.org/abs/1910.01834 ) solves any issue in our case because the reimbursement path is also open to stuck payment attacks and overall we bind liquidity in two directions now. |
Two new commits:
(I'm waiting for the onion rework to land before I bother rebasing on master!) |
Add test vectors from lightning/bolts#798 Update the TLV ranges for offers and invoice requests
Add test vectors from lightning/bolts#798 Update the TLV ranges for offers and invoice requests
bolt12/offers-test.json
Outdated
"bolt12": "lno1pgx9getnwss8vetrw3hhyucvqdqqqqqkyypwa3eyt44h6txtxquqh7lz5djge4afgfjn7k4rgrkuag0jsd5xvxg" | ||
}, | ||
{ | ||
"description": "Missing offer_description and offer_amount", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
offer_description
and offer_amount
are optional. Maybe you meant to test amount set but no description.
- if `offer_issuer_id` is present: | ||
- MUST set `invoice_node_id` to the `offer_issuer_id` | ||
- otherwise, if `offer_paths` is present: | ||
- MUST set `invoice_node_id` to the final `blinded_node_id` on the path it received the `invoice_request` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we name this invoice_signing_pubkey
?
from that estimate. | ||
- SHOULD not respond to an offer if the current time is after | ||
`offer_absolute_expiry`. | ||
- if it chooses to sends an `invoice_request`, it sends an onion message: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- if it chooses to sends an `invoice_request`, it sends an onion message: | |
- if it chooses to send an `invoice_request`, it sends an onion message: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also feel like this is the wrong section
invoice will be given since the request includes all the non-signature | ||
fields. | ||
|
||
The `offer_issuer_id` can be omitted for brevity, if `offer_paths` is set, as each of the final `blinded_node_id` in the paths can serve as a valid public key for the destination. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To simply development I think we could make it a MUST that either offer_issuer_id
or offer_paths
can be set but not both or neither.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's some background discussion here: #798 (comment)
Also, some implementations use offer_metadata
to derive signing keys, which it can check against offer_issuer_id
. We'd rather not invalidate these offers if we can avoid it.
- MAY omit `invreq_amount`. | ||
- if it sets `invreq_amount`: | ||
- MUST specify `invreq_amount`.`msat` as greater or equal to amount expected by `offer_amount` (and, if present, `offer_currency` and `invreq_quantity`). | ||
- MUST set `invreq_payer_id` to a transient public key. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-enforceable MUST
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing is enforceable, unfortunately. Or, perhaps, fortunately :)
Indeed, I think ACINQ were playing with the idea of weakening this in some cases...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yyforyongyu Responded to some of your comments but will let @rustyrussell or others correct me where wrong and respond to the rest.
invoice will be given since the request includes all the non-signature | ||
fields. | ||
|
||
The `offer_issuer_id` can be omitted for brevity, if `offer_paths` is set, as each of the final `blinded_node_id` in the paths can serve as a valid public key for the destination. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's some background discussion here: #798 (comment)
Also, some implementations use offer_metadata
to derive signing keys, which it can check against offer_issuer_id
. We'd rather not invalidate these offers if we can avoid it.
- MUST NOT set `invreq_quantity` | ||
- otherwise (not responding to an offer): | ||
- MUST set `offer_description` to a complete description of the purpose of the payment. | ||
- MUST set (or not set) `offer_absolute_expiry` and `offer_issuer` as it would for an offer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, though only for invoice request's without an offer (e.g., a scannable refund). Otherwise, it must be reflected back as any other TLV in an offer.
- MUST specify `offer_currency` `iso4217` as an ISO 4712 three-letter code. | ||
- MUST specify `offer_amount` in the currency unit adjusted by the ISO 4712 | ||
exponent (e.g. USD cents). | ||
- MUST set `offer_description` to a complete description of the purpose |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some background: #798 (comment)
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It's far easier to validate these on parsing than to hand-validate them elsewhere. I didn't turn `alias` or `error` into this, though they're similar (`alias` can have a nul terminator). Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Offers may contain blinded paths to allow for greater recipient privacy. However, they come at a cost of increased QR code size as each hop requires a 33-byte `point` for the `next_node_id`. Allow using `short_channel_id` instead, which only requires 8 bytes. Still allow for use of `next_node_id` for cases where the blinded path may not involve channel counterparties or for long-lived offers, which may outlive the given channels.
Offers may contain blinded paths to allow for greater recipient privacy. However, they come at a cost of increased QR code size as the introduction node requires a 33-byte `point`. Define a new `sciddir_or_pubkey` fundamental type such that either a point or a reference to one in a `channel_announcement` can be used. This is backwards compatible with `point`. Use this new type for the `blinded_path` subtype's `first_node_id`.
A BOLT11 "invoice" has proven too low-level for human use in many scenarios. Efforts like lnurl have covered the gap, but integrating some of such higher layers into the lightning protocol itself has many advantages. This draft defines three new things: 1. A new invoice format. I know, this is painful, but it maps almost 1:1 to the current format (though signatures are very different), is easier to implement, and easier to send via the lightning network itself. 2. Formats for an "offer", which for all intents and purposes serves as the new, persistent invoice for users. 3. Format for an "invoice_request": this is a message sent via the lightning network itself to receive the real invoice, or can be used directly in a send-money scenario (e.g. ATM). The offer (for accepting payments) or invoice_request (for sending payments) are usually presented via a QR code or similar, the replies are sent using onion messages. Each copies fields from the prior so it stands alone, to allow statelessness. Features which have been deliberately omitted for the initial version: - Recurrence. - Invoice replacement ("don't accept that old payment!") - Payer proof for refunds. This effort has been EPIC, and there is absolutely no way I could have done this without the often thankless task of implementing, re-implementing, revising and re-reading this text. In particular I have been delighted to receive the mental boost from the following people: 1. Thomas H of ACINQ (https://github.com/thomash-acinq) 2. Jeffrey Czyz of Square Crypto (https://github.com/jkczyz) 3. Joost Jager (https://github.com/joostjager) 4. Aditya Sharma (https://github.com/adi2011) 5. Rene Pickhardt (https://github.com/renepickhardt) 6. Bastien Teinturier of ACINQ (https://github.com/t-bast) 7. Valentine Wallace of LDK (https://github.com/valentinewallace) 8. Matt Corallo of LDK (https://github.com/BlueMatt) Also @bjarnemagnussen, @ellemouton, @animatedbarber, @617a7a, @instagibbs, and @eupn. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Three vectors: 1. Raw string decoding tests. (Start here!) 2. Offer decoding tests. 3. TLV signature tests. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
1. From https://github.com/evansmj - Make it clear that UPPERCASE is good for QR codes - Added test vectors for that too. 2. From https://github.com/yyforyongyu: - Move the use of `offers_path` requirement to the `invoice_request` reader requirements, and make them explicit. - Use the same language for invoices & invreq_paths. - Change MAY to SHOULD for implying chain values (Bitcoin). - SHOULD->MUST for expired invoices - MUST->SHOULD for onchain addresses order - Various typos 3. From https://github.com/jkczyz - Add requirement for amount if currency set. - Make it clear that the payment_preimage from the payment is what we use to prove invoice completion. - Explicitly tell reader to check `invoice_amount` == `invreq_amount` Offers test vectors fixed too. Use bit 122 not 22 (that's anchors) which is more clearly wrong. And fix the "no desc, no amount" which was supposed to be "no desc, with amount". Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
5773075
to
db73bbb
Compare
I have:
|
Offers are "static invoices", with many more features. This is mostly implemented in c-lightning already, and is now ready for wider testing.
This PR is based on onion messages specified in #759