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

lncli+invoices: experimental key send mode #3795

Merged
merged 10 commits into from Dec 24, 2019

Conversation

@joostjager
Copy link
Collaborator

joostjager commented Dec 5, 2019

This PR implements the key send functionality that is described in #2455 in lnd as it is today.

Throughout the year 2019, many PRs have been merged that together created a solid foundation to implement key send on. This is reflected in the size of this PR. The core changes to the invoice registry are relatively small and non-intrusive.

Key send isn't part of the Lightning spec yet. Therefore the preimage is sent as a custom record (in the type range >= 65536):

KeySendRecord uint64 = 5482373484

It is likely that this custom record will be deprecated eventually and replaced by either spontaneous sending as an AMP payment or a an official key send payload record.

What this means is that applications using the functionality as it is introduced in this PR may break when either the sender or receiver upgrades to a newer version of lnd.

As before, intermediate hops will support key send with any version of the software.

To enable receiving key send spontaneous payments, start lnd with the --accept-key-send flag. Without this flag set, spontaneous payments will be rejected.

Sending a spontaneous payment can be done via lncli:

lncli sendpayment -d <dest> -a <amount> --final_cltv_delta 40 --key_send

This PR is also the final step in enabling mainline lnd to send and receive chat messages (Whatsat).

@joostjager joostjager requested a review from Roasbeef as a code owner Dec 5, 2019
@joostjager joostjager force-pushed the joostjager:keysend branch from bdc7c7a to b84ab5d Dec 5, 2019
@joostjager joostjager added the payments label Dec 5, 2019
@joostjager joostjager force-pushed the joostjager:keysend branch 6 times, most recently from 1516edf to ce4ed09 Dec 5, 2019
@joostjager joostjager requested a review from cfromknecht as a code owner Dec 5, 2019
@joostjager joostjager force-pushed the joostjager:keysend branch from ce4ed09 to 0694843 Dec 6, 2019
@joostjager joostjager self-assigned this Dec 6, 2019
@joostjager joostjager added this to WIP in v0.9.0-beta via automation Dec 6, 2019
@joostjager joostjager added this to the 0.9.0 milestone Dec 6, 2019
@joostjager joostjager moved this from WIP to Needs Review in v0.9.0-beta Dec 9, 2019
@joostjager joostjager force-pushed the joostjager:keysend branch 4 times, most recently from b0107e9 to 8442b69 Dec 10, 2019
Copy link
Member

Roasbeef left a comment

The Phoenix, Sphinx, keysend rises!


paymentRequest := string(invoice.PaymentRequest)
if paymentRequest == "" {
preimage := invoice.Terms.PaymentPreimage

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Dec 11, 2019

Member

Do we also want to manually set the feature bit we'll use to signal this on the graph level (in the NodeAnnouncement)?

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Dec 11, 2019

Member

This would be another way to differentiate this payment from other payments when looking at the set of received payments.

This comment has been minimized.

Copy link
@joostjager

joostjager Dec 11, 2019

Author Collaborator

My idea was to keep it minimal as long as it is experimental. The node announcement isn't very critical. You can just try to key send and see if it gets pulled.

The payment can be differentiated because there is no pay req, but I think that can also happen with very old payments. It was already an optional field before. Would you want to add a new (experimental) field on the invoices returns by for example ListInvoices?

This comment has been minimized.

Copy link
@joostjager

joostjager Dec 11, 2019

Author Collaborator

The can also be recognized by the custom record that is present and exposed on invoices.

This comment has been minimized.

Copy link
@joostjager

joostjager Dec 12, 2019

Author Collaborator

The differentation part is solved now with a is_key_send rpc bool

invoices/invoiceregistry.go Outdated Show resolved Hide resolved
invoices/invoiceregistry.go Show resolved Hide resolved
invoices/invoiceregistry.go Outdated Show resolved Hide resolved
invoices/invoiceregistry.go Outdated Show resolved Hide resolved
invoices/invoiceregistry_test.go Outdated Show resolved Hide resolved
cmd/lncli/commands.go Show resolved Hide resolved
}

req.DestCustomRecords = map[uint64][]byte{
invoices.KeySendRecord: preimage[:],

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Dec 11, 2019

Member

Re payload what about also adding a set of optional 32 bytes? This kind of nudges users towards an intended use cases of specifying some sort of account/payment ID which is to be looked up on the backend to accept/deny the payments.

This comment has been minimized.

Copy link
@joostjager

joostjager Dec 11, 2019

Author Collaborator

People can do that easily through the rpc interface if they want. I am not sure if we should make assumptions like that they require 32 bytes in lncli. Also, if we always use the same record id for that, different uses may interfere with each other.

invoices/invoiceregistry.go Show resolved Hide resolved
invoices/invoiceregistry_test.go Show resolved Hide resolved
cmd/lncli/commands.go Show resolved Hide resolved
invoices/invoiceregistry.go Show resolved Hide resolved
lnrpc/invoicesrpc/utils.go Show resolved Hide resolved
@joostjager joostjager force-pushed the joostjager:keysend branch from 8442b69 to 6a8b54a Dec 11, 2019
@joostjager joostjager requested a review from Roasbeef Dec 11, 2019
@joostjager

This comment has been minimized.

Copy link
Collaborator Author

joostjager commented Dec 11, 2019

Main open question is at what level(s) we want to support key send. My philosophy for this pr was to keep it as non-intrusive as possible. Postpone adding any new rpc fields until we reconcile with amp. What is in now, is enough for people to start building off of it without requiring a fork of lnd. I think that is good enough given the experimental status of it.

@joostjager joostjager force-pushed the joostjager:keysend branch from 6a8b54a to f6b4844 Dec 11, 2019
config.go Outdated Show resolved Hide resolved
invoices/invoiceregistry.go Outdated Show resolved Hide resolved
invoices/invoiceregistry.go Outdated Show resolved Hide resolved
invoices/invoiceregistry.go Outdated Show resolved Hide resolved
cmd/lncli/commands.go Show resolved Hide resolved
@joostjager joostjager force-pushed the joostjager:keysend branch 2 times, most recently from 5d0872f to b133385 Dec 12, 2019
@joostjager joostjager force-pushed the joostjager:keysend branch from 5906b2d to a7ecc72 Dec 19, 2019
@joostjager joostjager force-pushed the joostjager:keysend branch from a7ecc72 to f62b798 Dec 20, 2019
@joostjager

This comment has been minimized.

Copy link
Collaborator Author

joostjager commented Dec 20, 2019

Nil dest_features pr is merged, key send is operational again

@joostjager joostjager requested a review from cfromknecht Dec 20, 2019
Copy link
Collaborator

cfromknecht left a comment

LGTM 🔥

@joostjager joostjager force-pushed the joostjager:keysend branch 5 times, most recently from b246cc6 to 7aad2b6 Dec 20, 2019
@joostjager

This comment has been minimized.

Copy link
Collaborator Author

joostjager commented Dec 22, 2019

Itest was failing because I happened to choose the same preimage for the key send test that was used in a later test for a hodl invoice. Fixed now.

joostjager added 10 commits Dec 14, 2019
Previously 5 seconds was used, which made the otherwise fast registry
tests relatively slow.
This field isn't optional. It was introduced in
5ed31b1 which was part of release
0.4.0. This release contained breaking database changes, so it is safe
to assume that from there on the field is always populated.

If there would still be empty payment request fields in the wild, users
would also experience issues with ListInvoices. ListInvoices decodes the
payment request.
Debug send has been removed some time ago. This is a left over.
Prepares for spontaneous key send payments that do not have a payment
request.
Move in separate commit to make next commit more readable.
This commit adds handling code for the key send custom record. If this
record is present and its hash matches the payment hash, invoice
registry will insert a new invoice into the database "just in time". The
subsequent settle flow is unchanged. The newly inserted invoice is
picked up and settled. Notifications will be broadcast as usual.
Extend lncli with a flag that indicates the intention to send
a spontaneous payment. lncli will generate a preimage/hash combination
and send the preimage as a custom record to the recipient.
@joostjager joostjager force-pushed the joostjager:keysend branch from 7aad2b6 to 4273bc0 Dec 23, 2019
v0.9.0-beta automation moved this from Needs Review to Approved Dec 24, 2019
Copy link
Member

Roasbeef left a comment

LGTM 🛤

Works as advertised on the tin:

   lncli --network=simnet sendpayment --key_send --dest=03d645f08fec4abe8ce02e17c25c3f2ed2b7c40b6aebb725e86bccc92665980054 --amt=100000 --final_cltv_delta=40
{
    "payment_error": "",
    "payment_preimage": "7e85e6bb1fdccd04c1085f1724ee11ebbe32ab6ff9411173fe53032635f37562",
    "payment_route": {
        "total_time_lock": 1704,
        "total_fees": "0",
        "total_amt": "100000",
        "hops": [
            {
                "chan_id": "1820791255662592",
                "chan_capacity": "1000000",
                "amt_to_forward": "100000",
                "fee": "0",
                "expiry": 1704,
                "amt_to_forward_msat": "100000000",
                "fee_msat": "0",
                "pub_key": "03d645f08fec4abe8ce02e17c25c3f2ed2b7c40b6aebb725e86bccc92665980054",
                "tlv_payload": true,
                "mpp_record": null,
                "custom_records": {
                    "5482373484": "7e85e6bb1fdccd04c1085f1724ee11ebbe32ab6ff9411173fe53032635f37562"
                }
            }
        ],
        "total_fees_msat": "0",
        "total_amt_msat": "100000000"
    },
    "payment_hash": "972f041144a5d97d081360f2e8cb84f3bd569d93a1256a4cf721566e553f5ab0"
}

One follow up that I think we should consider is exposing setting custom records on the command line as well, while merging that with the keysend feature if the flag is set. This makes the entire suite a bit more useful, as users can then specify other custom information along with the core keysend record.

@Roasbeef Roasbeef merged commit f289a39 into lightningnetwork:master Dec 24, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.05%) to 62.92%
Details
v0.9.0-beta automation moved this from Approved to Done Dec 24, 2019
@landgenoot landgenoot mentioned this pull request Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v0.9.0-beta
  
Done
3 participants
You can’t perform that action at this time.