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

lightningd: Add signinvoice to sign a BOLT11 invoice. #5697

Merged

Conversation

dongcarl
Copy link
Contributor

@dongcarl dongcarl commented Nov 7, 2022

Though there's already a `createinvoice` command, there are usecases where a
user may want to sign an invoice that they don't yet have the preimage to. For
example, they may have an htlc_accepted plugin that pays to obtain the preimage
from someone else and returns a `{ "result": "resolve", ... }`.

This RPC command addresses this usecase without overly complicating the
semantics of the existing `createinvoice` command.

Changlog-Added: JSON-RPC: `signinvoice` new command to sign BOLT11
invoices.

Also adds a commit to correct createinvoice's invstring description. (See the commit message for more details)

One main question for reviewers is whether this PR is acceptable without support for BOLT12 invoices. I'm not familiar enough with them to write code for them. Perhaps it can be left for a followup PR? If so, do we need to make the bolt11 field in doc/schemas/signinvoice.schema.json non-required?

CC: @cdecker

Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

Concept ACK

I added some comments that include some of my personal conventions, also if I'm still not able to catch the use case that this rpc enable, to me makes sense to provide an API to plugins to do somethings that it is not possible today/

Thanks

lightningd/invoice.c Outdated Show resolved Hide resolved
Comment on lines +5 to +7
"required": [
"bolt11"
],
Copy link
Collaborator

Choose a reason for hiding this comment

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

The bolt12 invoice born already signed iirc, so maybe here the options can be:

  • Support only the bolt11 and return an error in case of bolt12;
  • Support both, and return a warning in case of bolt12 (and if I'm not wrong regarding the bolt12 invoice) that tells that the invoice is already signed. This warning could be useful also in case of bolt11 is already signed?

doc/schemas/signinvoice.request.json Show resolved Hide resolved
@@ -533,6 +533,19 @@ def test_waitanyinvoice(node_factory, executor):
l2.rpc.waitanyinvoice('non-number')


def test_signinvoice(node_factory, executor):
# Setup
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Setup
""" Make sure that the signinvoice work as expected while re-sign a previous invoice """
# Setup

This is a personal preference, that I find useful in case of refactoring the test in the future

@cdecker
Copy link
Member

cdecker commented Nov 8, 2022

One main question for reviewers is whether this PR is acceptable without support for BOLT12 invoices. I'm not familiar enough with them to write code for them. Perhaps it can be left for a followup PR? If so, do we need to make the bolt11 field in doc/schemas/signinvoice.schema.json non-required?

I think we can side-step this issue by calling it unsigned, string, or raw, i.e., not referencing a specific format for now, but keeping it mandatory.

To deduplicate code it might be useful to make createinvoice an alias of signinvoice (the latter being more memorable imho) and adding optional preimage and save flags to check the hash is correct, and whether to save the invoice in the DB (requires preimage so we can terminate the payment as well).

Wdyt?

@rustyrussell rustyrussell added this to the v23.02 milestone Nov 10, 2022
@rustyrussell
Copy link
Contributor

We just merged createinvoicerequest with a savetodb parameter (default true). This could be made to match, though some work would be needed since json_add_invoice_fields assumes a db entry right now (fake one up, or make that take more parameters).

Obv preimage would be optional iff savetodb was false.

@dongcarl
Copy link
Contributor Author

dongcarl commented Nov 14, 2022

Reading through the comments... Is the general suggestion to instead modify createinvoice such that it looks like:

**createinvoice** *invstring* *label* [*preimage*] [*savetodb*]

?

It seems like this would require me to:

  1. Modify json_add_invoice_fields to take more parameters and not require a real db entry
  2. Add full support for bolt12

Which is not un-doable but expands on the scope of the PR.

Am I understanding you all correctly? @vincenzopalazzo @cdecker @rustyrussell

@dongcarl
Copy link
Contributor Author

On the open call yesterday @rustyrussell mentioned that he wanted me to just modify the createinvoicerequest call to accept bolt11 invoices, however, it seems like createinvoicerequest deals with tlv_invoice_request structs while createinvoice deals with tlv_invoice structs, is that okay? What is the difference between an invoice struct and an invoice request struct?

@rustyrussell
Copy link
Contributor

On further consideration, this would be a modification to createinvoice, but the API would be ugly: no label, no preimage. So I take it back, this API is cleaner.

@rustyrussell
Copy link
Contributor

Fairly trivial rebase, and two fixups for you to consider? Sorry for the delay!

The existing description is incorrect. `createinvoice` doesn't actually
work when supplied with a custom-encoded bolt11 invoice without the
final 520 signature bits appended. If a users tries to do so, some of
their tagged fields will be incorrectly truncated.

`createinvoice` actually expects that the signatures are there, and it
simply ignores them.

See common/bolt11.c's bolt11_decode_nosig:

         /* BOLT ElementsProject#11:
          *
          * The data part of a Lightning invoice consists of multiple sections:
          *
          * 1. `timestamp`: seconds-since-1970 (35 bits, big-endian)
          * 1. zero or more tagged parts
          * 1. `signature`: Bitcoin-style signature of above (520 bits)
          */
         if (!pull_uint(&hu5, &data, &data_len, &b11->timestamp, 35))
                 return decode_fail(b11, fail, "Can't get 35-bit timestamp");

>        while (data_len > 520 / 5) {
                 const char *problem = NULL;
                 u64 type, data_length;
@dongcarl dongcarl force-pushed the 2022-11-signinvoice-rpc branch 3 times, most recently from 8156e22 to c0cfebc Compare February 6, 2023 18:34
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

happy to see the CI up and running again!

Concept Ack, I have only a tiny comment on the error message!

@rustyrussell
Copy link
Contributor

Typo in commit message, "Changlog"! Other than that, it's looking great!

Though there's already a `createinvoice` command, there are usecases where a
user may want to sign an invoice that they don't yet have the preimage to. For
example, they may have an htlc_accepted plugin that pays to obtain the preimage
from someone else and returns a `{ "result": "resolve", ... }`.

This RPC command addresses this usecase without overly complicating the
semantics of the existing `createinvoice` command.

Changelog-Added: JSON-RPC: `signinvoice` new command to sign BOLT11
invoices.
Performed using:
  PYTHONPATH=contrib/msggen python3 contrib/msggen/msggen/__main__.py
@rustyrussell
Copy link
Contributor

Ack 813a93e

Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

ACK 813a93e

@endothermicdev endothermicdev merged commit dc4ae9d into ElementsProject:master Feb 6, 2023
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