hsmd: add preapprove invoice and keysend messages #5821

ksedgwic · 2022-12-14T00:27:34Z

These messages request payment approval from the hsmd for bolt11 and keysend payments:

default implementation automatically approves all payments
remote signer might not approve based on velocity controls, allowlist controls, manual approval, ...
remote signer will not allow unapproved payments to be made (will refuse to sign commitment)
declining a preapproval is much cleaner from a UX standpoint than preventing payment later

plugins/keysend.c

cdecker · 2022-12-16T09:46:45Z

Thanks @ksedgwic, just a couple of questions for my own understanding:

I'm guessing the pre-approval is there in order to avoid situations where lightningd would go ahead and attempt to pay or keysend, but then the signer would run into a policy violation, refusing to sign. This would then end up in the requesting daemon to hang indefinitely (restart seems like the only option here). Is that interpretation correct?
Would pre-approval result in any state changes on the signer? I.e., does the signer remember it approved a send, and would consider that for other decisions as well? Is this persisted?
How is concurrency handled? If I have two pay commands, that individually would be ok, but not together (velocity controls for example), how is the following sequence resolved:
- Preapproval A
- Preapproval B
- Signature requests for A
- Signature requests for B
If we track approvals persistently, how are these cleared again? Take for example a pay that was approved, but ultimately fails to complete, when is the approval cleared in order to allow future approvals to tap into the reservation for the failed pay?

On a more design-related thing: I'm assuming that keysend is the reason why we expose the pre-approval on the JSON-RPC. It being a plugin, it can't ask the signer to pre-approve, is that correct?

I'll review the code as soon as possible, but thought I needed a bit of a wider picture of how things are intended to work on a conceptual level 🙂

devrandom · 2022-12-16T10:21:59Z

Thanks @ksedgwic, just a couple of questions for my own understanding:

* I'm guessing the pre-approval is there in order to avoid situations where `lightningd` would go ahead and attempt to `pay` or `keysend`, but then the signer would run into a policy violation, refusing to sign. This would then end up in the requesting daemon to hang indefinitely (restart seems like the only option here). Is that interpretation correct?

Right, the other option is to return an error, but that would crash or at least restart the node.

There is another reason. Seeing the invoice allows us to bind HTLCs (the payment hash) to payees (the entity who signed the invoice), which in turn allows payee to be allowlisted.

* Would pre-approval result in any state changes on the signer? I.e., does the signer remember it approved a send, and would consider that for other decisions as well? Is this persisted?

Yes, the signer would start doing the following:

track the payment hash, the amount approved and the amount actually sent
adjust any velocity control state to account for the amount approved
prevent sending more than was approved

* How is concurrency handled? If I have two `pay` commands, that individually would be ok, but not together (velocity controls for example), how is the following sequence resolved:
  
  * Preapproval `A`
  * Preapproval `B`
  * Signature requests for `A`
  * Signature requests for `B`

The signer would serialize the preapprovals internally, so the later request for preapproval would fail if the velocity is exceeded. Note that velocity measures amount approved rather than amount actually sent, so that velocity controls can only fail a payment early - in the preapproval stage.

* If we track approvals persistently, how are these cleared again? Take for example a `pay` that was approved, but ultimately fails to complete, when is the approval cleared in order to allow future approvals to tap into the reservation for the failed `pay`?

We were currently planning to expire these on invoice expiry, but on second thought it would be useful to be able to withdraw a preapproval for a payment we give up on. I suppose it should only succeed in removal if there are no pending HTLCs involving that payment hash.

On a more design-related thing: I'm assuming that keysend is the reason why we expose the pre-approval on the JSON-RPC. It being a plugin, it can't ask the signer to pre-approve, is that correct?

I'll defer to @ksedgwic on this question.

ksedgwic · 2022-12-16T18:27:34Z

On a more design-related thing: I'm assuming that keysend is the reason why we expose the pre-approval on the JSON-RPC. It being a plugin, it can't ask the signer to pre-approve, is that correct?

Yes, that's right

ksedgwic · 2022-12-16T18:30:04Z

Would pre-approval result in any state changes on the signer? I.e., does the signer remember it approved a send, and would consider that for other decisions as well? Is this persisted?

Yes, the approval is remembered and persisted. Subsequent preapproval requests for an approved invoice or keysend are also immediately accepted.

lightningd/invoice.c

cdecker

Just a minor thing about the dangers of sync comms when the outcome itself is not critical itself. It's a relatively new consideration, and therefore not consistently applied to the existing code. I'd be more at ease with async comms :-)

cdecker · 2023-01-20T15:42:28Z

lightningd/invoice.c

+
+	u8 *msg = towire_hsmd_preapprove_invoice(NULL, invstring);
+
+	if (!wire_sync_write(cmd->ld->hsm_fd, take(msg)))


Doing synchronous comms here may not be what we want. It ends up blocking the entire lightningd until the signer has responded. This means that if we have a long RTT from lightningd to hsmd we are unresponsive for that roundtrip. Worse, if the signer does not respond we are blocked forever and can't react to on-chain events and peer messsages.

We should always prefer async comms in lightningd.

cdecker · 2023-01-20T15:42:55Z

lightningd/invoice.c

+
+	u8 *msg = towire_hsmd_preapprove_keysend(NULL, destination, payment_hash, *amount);
+
+	if (!wire_sync_write(cmd->ld->hsm_fd, take(msg)))


Same thing about sync comms above.

Async comms are not possible at this point in time.

cdecker · 2023-01-25T09:26:09Z

Hi @ksedgwic, sorry for suggesting this without first checking if we do any async comms with the signer. It turns out we don't, and introducing it in this PR would massively expand the scope of this PR. I therefore retract my previous comments, and think the changes should go in as they are.

ACK 31da26c

cdecker · 2023-01-25T09:27:46Z

@endothermicdev would you like me to rebase this to get rid of the kick ci commit at the end? It's not semantically meaningful and was there solely to convince CI to give it another shot.

@ksedgwic if CI gets stuck again, ping me, and I can kick it on the console :-)

endothermicdev · 2023-01-25T13:21:58Z

@cdecker yes, I would appreciate that. Thanks for the updating your review as well.

cdecker · 2023-01-26T12:52:46Z

@ksedgwic it seems like the PR disallows maintainers to push to your branch, can you rebase and drop the last commit? Then we'll fold the PR into the release 🎉

…ifier Changelog-added: hsmd: A new message `hsmd_preapprove_invoice` is added. Changelog-added: JSON-RPC: A new command `preapproveinvoice` is added.

devrandom · 2023-01-26T13:04:47Z

I can't change that flag from here, but I was able to rebase and drop the extra commit.

…ifier Changelog-added: hsmd: A new message `hsmd_preapprove_keysend` is added. Changelog-added: JSON-RPC: A new command `preapprovekeysend` is added.

devrandom · 2023-01-26T14:37:43Z

CI fail seems unrelated...

endothermicdev · 2023-01-27T03:09:33Z

Yes, unrelated but I haven't identified the fix to that one yet. Thanks!

ACK 0fa933e

ksedgwic force-pushed the hsmd-preapprove-payments branch from 938acf7 to 72b1383 Compare December 14, 2022 00:35

devrandom reviewed Dec 14, 2022

View reviewed changes

plugins/keysend.c Show resolved Hide resolved

ksedgwic force-pushed the hsmd-preapprove-payments branch 2 times, most recently from ebaee21 to a72e684 Compare December 14, 2022 21:40

ksedgwic marked this pull request as ready for review December 15, 2022 05:39

ksedgwic requested a review from cdecker as a code owner December 15, 2022 05:39

cdecker self-assigned this Dec 16, 2022

rustyrussell added this to the v23.02 milestone Jan 9, 2023

cdecker reviewed Jan 20, 2023

View reviewed changes

lightningd/invoice.c Show resolved Hide resolved

cdecker previously requested changes Jan 20, 2023

View reviewed changes

hsmd: add hsmd_preapprove_invoice and check_preapproveinvoice pay mod…

1bec5ff

…ifier Changelog-added: hsmd: A new message `hsmd_preapprove_invoice` is added. Changelog-added: JSON-RPC: A new command `preapproveinvoice` is added.

devrandom force-pushed the hsmd-preapprove-payments branch from 31da26c to 3124608 Compare January 26, 2023 13:03

devrandom force-pushed the hsmd-preapprove-payments branch from 3124608 to 03aae88 Compare January 26, 2023 13:16

ksedgwic added 2 commits January 26, 2023 14:18

hsmd: add hsmd_preapprove_keysend and check_preapprovekeysend pay mod…

b0e2911

…ifier Changelog-added: hsmd: A new message `hsmd_preapprove_keysend` is added. Changelog-added: JSON-RPC: A new command `preapprovekeysend` is added.

hsmd: increase HSM_MAX_VERSION to 3

0fa933e

devrandom force-pushed the hsmd-preapprove-payments branch from 03aae88 to 0fa933e Compare January 26, 2023 13:18

endothermicdev merged commit 7b2c561 into ElementsProject:master Jan 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hsmd: add preapprove invoice and keysend messages #5821

hsmd: add preapprove invoice and keysend messages #5821

ksedgwic commented Dec 14, 2022 •

edited

Loading

cdecker commented Dec 16, 2022

devrandom commented Dec 16, 2022

ksedgwic commented Dec 16, 2022

ksedgwic commented Dec 16, 2022

cdecker left a comment

cdecker Jan 20, 2023

cdecker Jan 20, 2023

cdecker commented Jan 25, 2023

cdecker commented Jan 25, 2023 •

edited

Loading

endothermicdev commented Jan 25, 2023

cdecker commented Jan 26, 2023

devrandom commented Jan 26, 2023

devrandom commented Jan 26, 2023

endothermicdev commented Jan 27, 2023


		u8 *msg = towire_hsmd_preapprove_invoice(NULL, invstring);

		if (!wire_sync_write(cmd->ld->hsm_fd, take(msg)))


		u8 msg = towire_hsmd_preapprove_keysend(NULL, destination, payment_hash, amount);

		if (!wire_sync_write(cmd->ld->hsm_fd, take(msg)))

hsmd: add preapprove invoice and keysend messages #5821

hsmd: add preapprove invoice and keysend messages #5821

Conversation

ksedgwic commented Dec 14, 2022 • edited Loading

cdecker commented Dec 16, 2022

devrandom commented Dec 16, 2022

ksedgwic commented Dec 16, 2022

ksedgwic commented Dec 16, 2022

cdecker left a comment

Choose a reason for hiding this comment

cdecker Jan 20, 2023

Choose a reason for hiding this comment

cdecker Jan 20, 2023

Choose a reason for hiding this comment

cdecker commented Jan 25, 2023

cdecker commented Jan 25, 2023 • edited Loading

endothermicdev commented Jan 25, 2023

cdecker commented Jan 26, 2023

devrandom commented Jan 26, 2023

devrandom commented Jan 26, 2023

endothermicdev commented Jan 27, 2023

ksedgwic commented Dec 14, 2022 •

edited

Loading

cdecker commented Jan 25, 2023 •

edited

Loading