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

dongcarl · 2022-11-07T17:31:48Z

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

vincenzopalazzo

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

vincenzopalazzo · 2022-11-08T13:34:02Z

doc/schemas/signinvoice.schema.json

+  "required": [
+    "bolt11"
+  ],


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

vincenzopalazzo · 2022-11-08T13:40:02Z

tests/test_invoices.py

@@ -533,6 +533,19 @@ def test_waitanyinvoice(node_factory, executor):
        l2.rpc.waitanyinvoice('non-number')


+def test_signinvoice(node_factory, executor):
+    # Setup


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 · 2022-11-08T14:18:03Z

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 · 2022-11-10T01:14:41Z

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 · 2022-11-14T20:08:59Z

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:

Modify json_add_invoice_fields to take more parameters and not require a real db entry
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 · 2022-11-30T17:55:07Z

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 · 2023-02-06T01:55:55Z

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 · 2023-02-06T02:20:45Z

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;

lightningd/invoice.c

vincenzopalazzo

happy to see the CI up and running again!

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

rustyrussell · 2023-02-06T20:12:11Z

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 · 2023-02-06T20:25:51Z

Ack 813a93e

vincenzopalazzo

ACK 813a93e

vincenzopalazzo reviewed Nov 8, 2022

View reviewed changes

vincenzopalazzo requested a review from cdecker November 8, 2022 13:43

rustyrussell added this to the v23.02 milestone Nov 10, 2022

rustyrussell force-pushed the 2022-11-signinvoice-rpc branch from 6ca1cdc to 3e6a277 Compare February 6, 2023 02:20

rustyrussell force-pushed the 2022-11-signinvoice-rpc branch from 3e6a277 to 716d543 Compare February 6, 2023 03:00

dongcarl force-pushed the 2022-11-signinvoice-rpc branch 3 times, most recently from 8156e22 to c0cfebc Compare February 6, 2023 18:34

vincenzopalazzo reviewed Feb 6, 2023

View reviewed changes

lightningd/invoice.c Show resolved Hide resolved

vincenzopalazzo approved these changes Feb 6, 2023

View reviewed changes

dongcarl force-pushed the 2022-11-signinvoice-rpc branch from c0cfebc to 813a93e Compare February 6, 2023 20:18

dongcarl added 3 commits February 6, 2023 15:22

msggen: Enable SignInvoice

863c32d

msggen: Regenerate for addition of SignInvoice

813a93e

Performed using: PYTHONPATH=contrib/msggen python3 contrib/msggen/msggen/__main__.py

vincenzopalazzo approved these changes Feb 6, 2023

View reviewed changes

endothermicdev merged commit dc4ae9d into ElementsProject:master Feb 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dongcarl commented Nov 7, 2022 •

edited

vincenzopalazzo left a comment

vincenzopalazzo Nov 8, 2022

vincenzopalazzo Nov 8, 2022

cdecker commented Nov 8, 2022

rustyrussell commented Nov 10, 2022

dongcarl commented Nov 14, 2022 •

edited

dongcarl commented Nov 30, 2022

rustyrussell commented Feb 6, 2023

rustyrussell commented Feb 6, 2023

vincenzopalazzo left a comment

rustyrussell commented Feb 6, 2023

rustyrussell commented Feb 6, 2023

vincenzopalazzo left a comment

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

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

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

Conversation

dongcarl commented Nov 7, 2022 • edited

vincenzopalazzo left a comment

Choose a reason for hiding this comment

vincenzopalazzo Nov 8, 2022

Choose a reason for hiding this comment

vincenzopalazzo Nov 8, 2022

Choose a reason for hiding this comment

cdecker commented Nov 8, 2022

rustyrussell commented Nov 10, 2022

dongcarl commented Nov 14, 2022 • edited

dongcarl commented Nov 30, 2022

rustyrussell commented Feb 6, 2023

rustyrussell commented Feb 6, 2023

vincenzopalazzo left a comment

Choose a reason for hiding this comment

rustyrussell commented Feb 6, 2023

rustyrussell commented Feb 6, 2023

vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

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

dongcarl commented Nov 7, 2022 •

edited

dongcarl commented Nov 14, 2022 •

edited