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

'invoice' command returning preimage. #3295

Merged
merged 3 commits into from Dec 3, 2019

Conversation

@fiatjaf
Copy link
Contributor

fiatjaf commented Nov 26, 2019

I'm sorry for this silly PR, but this has been bugging me for a long time. Being able to see the preimages of invoices issued is good for overall understanding of the protocol and for easy checking of payment proofs from others, not to mention other advanced uses (which may be covered by passing your own preimage to invoice as the docs imply, but still).

The point is that this doesn't hurt and makes everything better.


Some useless context

On https://t.me/lntxbot I like to show a lot of debugging information (because I'm crazy and I think users will want to understand what is happening): 2019-11-25-231518_445x806_scrot

However, because I can't get the preimages my invoices are using from the invoice command to show these preimages I have to generate them on my own using a probably-not-so-safe generator and store them in not-so-optimal ways instead of having lightningd generate them for me.

edit: just realized the above image is from the sender side, not the receiver, so completely unrelated.

@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Nov 26, 2019

For context: we aren't displaying the preimage mainly because it currently serves also as a proof-of-payment ("I have the preimage so I have paid you, how else could I have gotten it?"). If we were to show the preimage in the cli output, then the user could be tricked into sharing that with the sender, thus giving him the proof.

Worse, if any node on the path between sender and recipient learns about the preimage before the payment is attempted it can grab the transfer, short-circuiting it using the preimage. This would then be a really hard to debug situation (sender sends payment successfully, but the payment never arrives at the recipient, but sender still has proof that he paid).

A middle ground could be to show the preimage after the payment has completed and the invoice was paid, at which point users could compare the preimage and just see that it matches on both ends.

@fiatjaf

This comment has been minimized.

Copy link
Contributor Author

fiatjaf commented Nov 26, 2019

Well, but doesn't this seem like a big user failure from someone who doesn't know what they're doing? A user like that can do a lot of harm to himself already in other ways. Like being tricked into issuing pay on an invoice he didn't inspect first and end up paying 1000000 sat instead of 1000 (actually I'm very afraid of doing that in a moment of distraction).

Anyway, ignore the above paragraph. The middle ground you suggested is a big improvement, I think. And it even solves my specific issue now that I'm thinking about it. I'll change this PR to do that.

@darosior

This comment has been minimized.

Copy link
Collaborator

darosior commented Nov 26, 2019

About the middle ground: waitsendpay, listpays and listsendpays already contains the preimage. https://lightning.readthedocs.io/lightning-waitsendpay.7.html, https://lightning.readthedocs.io/lightning-listpays.7.html, https://lightning.readthedocs.io/lightning-listsendpays.7.html. There are also a hook and a notification https://lightning.readthedocs.io/PLUGINS.html#invoice-payment sent when an invoice was paid and containing the payment preimage

@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Nov 26, 2019

About the middle ground: waitsendpay, listpays and listsendpays already contains the preimage. https://lightning.readthedocs.io/lightning-waitsendpay.7.html, https://lightning.readthedocs.io/lightning-listpays.7.html, https://lightning.readthedocs.io/lightning-listsendpays.7.html. There are also a hook and a notification https://lightning.readthedocs.io/PLUGINS.html#invoice-payment sent when an invoice was paid and containing the payment preimage

All of those RPC calls however are on the sender side, and I think @fiatjaf is talking about the recipient side.

I'll implement the middle solution ASAP :-)

@cdecker cdecker force-pushed the fiatjaf:output_preimages branch from 2db31f3 to dab767b Nov 26, 2019
@fiatjaf fiatjaf requested a review from cdecker as a code owner Nov 26, 2019
@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Nov 26, 2019

I went ahead and added a couple of fixup commits that I'll squash before merging, but I wanted to get @fiatjaf's sign-off first:

  • Good work on adding json_add_preimage and consolidating it's use
  • I removed the payment_preimage in the return of invoice again as discussed before
  • I added a test for the preimage when listing paid invoices

The commit message for the first commit doesn't really match anymore, if you'd like I can reword that before squashing ^^

@cdecker cdecker added this to the 0.7.4 milestone Nov 26, 2019
@fiatjaf

This comment has been minimized.

Copy link
Contributor Author

fiatjaf commented Nov 26, 2019

I think this is great, thanks!

Please reword the commit while squashing, yes.

Also thank you for the compliment, it means a lot to my insufficient C skills.

@rustyrussell

This comment has been minimized.

Copy link
Contributor

rustyrussell commented Nov 26, 2019

I share @cdecker's reluctance to share the preimage, but this could be made a parameter to listinvoices (default off).

@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Nov 28, 2019

I share @cdecker's reluctance to share the preimage, but this could be made a parameter to listinvoices (default off).

I had thought about this, however if we assume the attacker can trick the user into sharing the output of listinvoices it'd be easy for him to also trick him to use the extra parameter ("My payment isn't working? That's strange! Can you share the output of lightning-cli listinvoices [payment_hash] true?"). By gating the reveal of the preimage on the status of the invoice we at least make sure it was paid first, which is in line with the "don't reuse invoices" practice.

@cdecker cdecker force-pushed the fiatjaf:output_preimages branch from dab767b to b483f0a Nov 29, 2019
@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Nov 29, 2019

Rebased and squashed, should be good to go, unless @rustyrussell wants to guard the payment_preimage for paid invoices using a flag.

@cdecker cdecker force-pushed the fiatjaf:output_preimages branch from b483f0a to f309dc0 Dec 2, 2019
Changelog-Added: JSON-RPC: `listinvoices` now displays the payment preimage if the invoice was paid.
@cdecker cdecker force-pushed the fiatjaf:output_preimages branch from f309dc0 to d0905df Dec 2, 2019
@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Dec 2, 2019

Ack d0905df

[ Changed ACK to Ack to see if ackbot will recognize it? --RR ]

@rustyrussell

This comment has been minimized.

Copy link
Contributor

rustyrussell commented Dec 3, 2019

Ack d0905df

@rustyrussell

This comment has been minimized.

Copy link
Contributor

rustyrussell commented Dec 3, 2019

Ack d0905df

[ Changed ACK to Ack to see if ackbot will recognize it? --RR ]

That didn't work, but once I acked it recognized @cdecker's ack too. Weird.

@rustyrussell rustyrussell merged commit 99ff86f into ElementsProject:master Dec 3, 2019
4 checks passed
4 checks passed
bitcoin-bot/acks Acks by cdecker, rustyrussell
bitcoin-bot/changelog This PR has at least one changelog entry
bitcoin-bot/fixups PR does not contain unsquashed fixups
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Dec 3, 2019

Ack d0905df
[ Changed ACK to Ack to see if ackbot will recognize it? --RR ]

That didn't work, but once I acked it recognized @cdecker's ack too. Weird.

Yeah, the bot had caught it, but calling the status RPC on Github failed, so changes to the ack message were bumping against the uniqueness constraint and not retrying the Github RPC call. Will fix 😉

@fiatjaf fiatjaf deleted the fiatjaf:output_preimages branch Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.