Plugin implementation of `keysend` spontaneous payments #3611

cdecker · 2020-03-31T14:44:18Z

The keysend plugin implements the ability to receive spontaneous payments by
including the payment_preimage in the onion payload, thus giving the
recipient the ability to claim the payment without having to exchange invoices
first. This sometimes erroneously called a sphinx-send, and can lead to
confusion with the onion routing protocol based on the sphinx paper.

The plugin has the ability to extract the preimage from the payload and will
tell lightningd to resolve the HTLC with the provided preimage. As proposed
by @rustyrussell, in order to keep track of funds, lightningd will create an
ad-hoc invoice that is immediately fulfilled via the resolved HTLC. This
ensures that we have an entry in the incoming payments, even though we didn't
really have a matching invoice. The ad-hoc invoice is generated whenever a
plugin tells lightningd to resolve an HTLC, so that these payments
terminated by plugins now always leave an accounting entry in the database.

Due to the uniqueness constraint on payment_hash in the invoices table we
cannot always insert an invoice that matches the resolved HTLC, in which case
we will simply not add an entry. This should happen rarely since re-using a
payment_hash is inherently insecure, e.g., it allows any node involved in
routing the first payment to grab the second payment. Alternatively we could
also reject the payment due to this protocol violation on the sender side,
however that'd be handing the funds to a (so far) well behaved node that
forwarded the payment despite being able to grab it, leaving the sender out of
pocket, and the recipient without the funds it could have gotten.

Since ad-hoc invoices do not need a features fields and bolt11 string,
which in turn would require asking the hsmd for a signature, we made those
two fields nullable (changelog warning added).

Sending support for keysend payments will be added in a separate pull
request since it requires abstracting the internals of the pay plugin so we
can reuse them here.

As a side note: the keysend plugin will make sure it understands all the
mandatory TLV fields, and issue a continue otherwise. This was done in order
to allow alternative plugins to terminate keysends that have additional
information. One example is the noise plugin that terminates keysend
payments that are sent along a chat message (which has a mandatory TLV-type)
and associates the message with the incoming payment.

cdecker · 2020-03-31T14:59:37Z

In order to avoid conflicts I self-assigned featurebit 55 since whatsat does not seem to be announcing support at all.

cdecker · 2020-03-31T15:33:41Z

Having a bit of trouble with these changes due to a missing dependency from json_tok.h to wire/gen_onion_wire.h:

In file included from plugins/keysend.c:2:
In file included from ./plugins/libplugin.h:16:
In file included from ./common/param.h:5:
In file included from ./common/json_tok.h:8:
./common/sphinx.h:13:10: fatal error: 'wire/gen_onion_wire.h' file not found
#include <wire/gen_onion_wire.h>
         ^~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
<builtin>: recipe for target 'plugins/keysend.o' failed
make: *** [plugins/keysend.o] Error 1

Trying to debug that at the moment, but hints would be appreciated 😉

cdecker · 2020-03-31T16:40:47Z

Trying to debug that at the moment, but hints would be appreciated wink

But.. Didn't the common objects depends on the wire/ objects ? Could build with :

diff --git a/common/Makefile b/common/Makefile
index 9c24168f3..e04fc0b3a 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -78,7 +78,7 @@ COMMON_HEADERS_NOGEN := $(COMMON_SRC_NOGEN:.c=.h)     \
        common/jsonrpc_errors.h                         \
        common/overflows.h                              \
        common/status_levels.h
-COMMON_HEADERS_GEN := common/gen_htlc_state_names.h common/gen_status_wire.h common/gen_peer_status_wire.h
+COMMON_HEADERS_GEN := common/gen_htlc_state_names.h common/gen_status_wire.h common/gen_peer_status_wire.h wire/gen_onion_wire.h
 
 COMMON_HEADERS := $(COMMON_HEADERS_GEN) $(COMMON_HEADERS_NOGEN)
 COMMON_SRC := $(COMMON_SRC_NOGEN) $(COMMON_SRC_GEN)

No luck, I really tried adding the missing header almost everywhere, and the behaviour doesn't change.

darosior · 2020-03-31T16:43:44Z

I deleted this comment after making use of my brain (not about linking objects but finding headers), sorry 😅.

cdecker · 2020-03-31T16:50:09Z

Interestingly this doesn't happen with make -j 1, but with increased parallelism it starts to happen (make -j 8 is my usual command because I'm impatient).

BTW this seems to work, but I have no idea why:

diff --git a/plugins/Makefile b/plugins/Makefile
index a15a1ff54..06bf35264 100644
--- a/plugins/Makefile
+++ b/plugins/Makefile
@@ -51,6 +51,8 @@ PLUGIN_COMMON_OBJS :=				\
 	wire/fromwire.o				\
 	wire/towire.o
 
+plugins/keysend.o: wire/gen_onion_wire.h
+
 plugins/pay: bitcoin/chainparams.o $(PLUGIN_PAY_OBJS) $(PLUGIN_LIB_OBJS) $(PLUGIN_COMMON_OBJS) $(JSMN_OBJS) $(CCAN_OBJS)
 
 plugins/autoclean: bitcoin/chainparams.o $(PLUGIN_AUTOCLEAN_OBJS) $(PLUGIN_LIB_OBJS) $(PLUGIN_COMMON_OBJS) $(JSMN_OBJS) $(CCAN_OBJS)

darosior · 2020-03-31T16:51:40Z

I experienced this too, both for the non-aggressive build and for the addition of the header file in plugins/Makefile (or common/Makefile).

darosior · 2020-04-01T15:49:12Z

plugins/Makefile

 $(PLUGIN_PAY_OBJS) $(PLUGIN_AUTOCLEAN_OBJS) $(PLUGIN_FUNDCHANNEL_OBJS) $(PLUGIN_BCLI_OBJS) $(PLUGIN_LIB_OBJS): $(PLUGIN_LIB_HEADER)

 # Make sure these depend on everything.
-ALL_PROGRAMS += plugins/pay plugins/autoclean plugins/fundchannel plugins/bcli
+ALL_PROGRAMS += plugins/pay plugins/autoclean plugins/fundchannel plugins/bcli plugins/keysend
 ALL_OBJS += $(PLUGIN_PAY_OBJS) $(PLUGIN_AUTOCLEAN_OBJS) $(PLUGIN_FUNDCHANNEL_OBJS) $(PLUGIN_BCLI_OBJS) $(PLUGIN_LIB_OBJS)


Got it. You forgot to add $(PLUGIN_KEYSEND_OBJS) here. ALL_OBJS will later be set to depend on WIRE_HEADERS !

"later" <==> in the main Makefile

Whoa, that's awesome work, thanks a million 👍

darosior

Looks good to me, not really tested yet

darosior · 2020-04-01T16:40:41Z

plugins/keysend.c

 	plugin_main(argv, init, PLUGIN_RESTARTABLE, commands, ARRAY_SIZE(commands),
 	            NULL, 0, hooks, ARRAY_SIZE(hooks), NULL);
+	plugin_main(argv, init, PLUGIN_RESTARTABLE, NULL, commands,
+		    ARRAY_SIZE(commands), NULL, 0, hooks, ARRAY_SIZE(hooks),


nvm, resolved innext commit

darosior · 2020-04-01T16:47:47Z

plugins/keysend.c

@@ -3,6 +3,7 @@
 #include <wire/gen_onion_wire.h>

 #define PREIMAGE_TLV_TYPE 5482373484
+#define KEYSEND_FEATUREBIT 0x81


Why isn't it 55 ?

I presume it's because 'bit 55' corresponds to 0x81 in hex, in other words '55' here is not an integer but a bit-count.

.... that doesn't make sense, @niftynei ? 0x81 == 129. I assume this was supposed to be 0x37?

darosior · 2020-04-01T16:50:39Z

Maybe this deserves the optech make me famous tag ? :)

rustyrussell · 2020-04-07T07:12:01Z

Can we just do #3628 and use that instead please?

Then we can remove the tlv option we don't want lightningd to see, and the rest is simple?

rustyrussell

I really prefer replacing the payload, and just doing the rest using existing infrastructure. See #3628

rustyrussell · 2020-04-07T05:28:40Z

plugins/keysend.c

+	if (s != max) {
+		return htlc_accepted_continue(cmd);
+	}
+	fromwire_tlv_payload(&rawpayload, &max, payload);


Should check if this fails?

rustyrussell · 2020-04-07T05:28:53Z

plugins/keysend.c

+	fromwire_tlv_payload(&rawpayload, &max, payload);
+
+	/* Try looking for the field that contains the preimage */
+	for (int i=0; i<tal_count(payload->fields); i++) {


Prefer size_t for sizes...

cdecker · 2020-04-08T19:09:07Z

Ok, I went ahead and changed the plugin to create the invoice itself, modify
the payload, and then telling lightningd to continue instead of
resolve. I dropped the nullability patch for some invoice fields which is
great, the downsides are that we now have to guess a bit more to give the
invoice a unique label (I went for a time-based suffix), and there is no way
for us to accept a payment with a duplicate payment_hash, potentially
leaving it to another node on the path (shouldn't happen, since the node could
have grabbed that payment on the forward path, but still...).

@rustyrussell's patch didn't work for me, so I disabled checking the payment
was terminated by looking at the invoice, but it's easy to re-enable once the
patch is working again.

cdecker · 2020-04-09T16:21:56Z

Rebased on top of master, added @rustyrussell's commit allowing payload modifications, and fixed up the test (I forgot about the payload having a length prefix again).

~~Should be good now. I'll just roll in the spurious fixup into the correct commit and then this is finally done.~~

valgrind found some more to complain about on restarting tests, need to debug that.

cdecker · 2020-04-15T18:43:04Z

Rebased on top of #3647, hopefully now it doesn't fail when replacing the payload anymore.

We use the new function `plugins_free` to define the correct deallocation order on shutdown, since under normal operation the allocation tree is organized to allow plugins to terminate and automatically free all dependent resources. During shutdown the deallocation order is under-defined since siblings may get freed in any order, but we implicitly rely on them staying around.

The plugin can basically return whatever it thinks the preimage is, but we weren't handling the case in which it doesn't actually match the hash. If it doesn't match now we just return an error claiming we don't have any matching invoice.

This still uses the experimental TLV-type, but once the type is standardized we can add detection for the new type quite easily. Changelog-Added: pay: The `keysend` plugin implements the ability to receive spontaneous payments (keysend)

The generated wrappers will ignore the raw fields and will only consider the shortcut fields. This function takes the raw fields and serializes them instead.

It currently uses a borrowed sending implementation from the noise plugin, but we'll implement that functionality in the native keysend plugin next.

We must really make sure that we understand the entire payload, not just the fields we are interested in.

Better not duplicate these, we might end up mixing them.

The featurebit isn't quite settled.

These are necessary for the interim keysend plugin

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Changelog-added: `htlc_accepted` hook can now offer a replacement onion `payload`.

So far we were relying on `lightningd` to create an ad-hoc invoice when telling it to `resolve` with a given preimage. We now switch to having the plugin create the invoice, remove the mandatory `keysend_preimage` field (which would upset `lightningd` otherwise), and then return the modified payload with the instructions to `continue` instead of resolving. This ties back in with the existing payment/invoice handling code. Invoices are created only if we don't have a label clash (unlikely since we have the nano-time in the label), or the `payment_hash` was already used for another invoice (at which point `lightningd` will automatically reject the payment and we're a bit poorer for it, but meh :-)

As discussed with Christian, prepending the length to the payload returned is awkward, but it's the only way to set a legacy payload. As this will be soon deprecated, simplify the external API. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell · 2020-04-16T02:08:52Z

Trivial rebase onto master.

Ack ea931f0

whitslack · 2020-04-19T06:31:57Z

Should this plugin have been added to PLUGINS in Makefile? As is, the keysend binary is not installed into $(plugindir).

Support for receiving and sending KeySend payment is added. For an explanation of the KeySend feature see: ElementsProject/lightning#3611

cdecker added plugin pay-plugin labels Mar 31, 2020

cdecker added this to the 0.8.2 milestone Mar 31, 2020

cdecker requested review from rustyrussell and darosior March 31, 2020 14:44

cdecker requested a review from ZmnSCPxj as a code owner March 31, 2020 14:44

cdecker self-assigned this Mar 31, 2020

cdecker force-pushed the keysend branch from 78eda06 to afd185f Compare March 31, 2020 14:45

darosior reviewed Apr 1, 2020

View reviewed changes

darosior mentioned this pull request Apr 2, 2020

Fix fundchannel in deprecated-apis=false mode ('satoshi' param in fundchannel_start) #3615

Merged

niftynei added the Optech Make Me Famous! Look! Look! Look! COOL NEW FEATURE! label Apr 4, 2020

rustyrussell reviewed Apr 7, 2020

View reviewed changes

cdecker force-pushed the keysend branch 2 times, most recently from e0e4031 to dfd9f8f Compare April 8, 2020 19:02

cdecker force-pushed the keysend branch from dfd9f8f to c346a19 Compare April 8, 2020 19:09

cdecker mentioned this pull request Apr 9, 2020

lightningd: allow htlc_accepted hook to replace onion payload. #3628

Closed

cdecker force-pushed the keysend branch 3 times, most recently from a4e0f74 to 4adba79 Compare April 9, 2020 16:19

rustyrussell force-pushed the keysend branch from e211880 to a866a53 Compare April 14, 2020 10:10

cdecker force-pushed the keysend branch from a866a53 to 7b23db2 Compare April 14, 2020 13:03

rustyrussell mentioned this pull request Apr 15, 2020

Register plugin hook explicit deserialize #3647

Merged

cdecker force-pushed the keysend branch 3 times, most recently from fce4412 to a2435c6 Compare April 15, 2020 18:42

cdecker and others added 14 commits April 16, 2020 11:37

plugin: Add keysend skeleton

72a0f43

wire: Add a function to serialize a raw set of TLV fields

4c0ec55

The generated wrappers will ignore the raw fields and will only consider the shortcut fields. This function takes the raw fields and serializes them instead.

pytest: Add a test for keysend

9118079

It currently uses a borrowed sending implementation from the noise plugin, but we'll implement that functionality in the native keysend plugin next.

keysend: Ignore keysends if the payload contains unknown even fields

ac5f088

We must really make sure that we understand the entire payload, not just the fields we are interested in.

plugin: Move feature_place enum to features.h

c4457fe

Better not duplicate these, we might end up mixing them.

libplugin: Add features to plugin_main and getmanifest

4f4f28b

keysend: Announce keysend featurebit in node_announcement

54a6fb0

The featurebit isn't quite settled.

pyln-proto: Add Tu32Field and Tu64Field helpers

fd2de3b

These are necessary for the interim keysend plugin

lightningd: allow htlc_accepted hook to replace onion payload.

3593a96

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Changelog-added: `htlc_accepted` hook can now offer a replacement onion `payload`.

rustyrussell force-pushed the keysend branch from a2435c6 to ea931f0 Compare April 16, 2020 02:09

rustyrussell merged commit bf053dc into ElementsProject:master Apr 16, 2020

dariuskramer mentioned this pull request Jul 10, 2020

Implement the KeySend feature, spontaneous payments ACINQ/eclair#1485

Merged

vindard mentioned this pull request Feb 17, 2021

Implement keysend sending support for spontaneous payments #3792

Merged

1 task

Ale1ster mentioned this pull request Feb 18, 2021

Keysend feature bit announcement lightningnetwork/lnd#5044

Closed

pm47 pushed a commit to ACINQ/eclair that referenced this pull request May 1, 2021

Implement the KeySend feature, spontaneous payments (#1485)

36ef588

Support for receiving and sending KeySend payment is added. For an explanation of the KeySend feature see: ElementsProject/lightning#3611

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plugin implementation of `keysend` spontaneous payments #3611

Plugin implementation of `keysend` spontaneous payments #3611

cdecker commented Mar 31, 2020

cdecker commented Mar 31, 2020

cdecker commented Mar 31, 2020

cdecker commented Mar 31, 2020

darosior commented Mar 31, 2020

cdecker commented Mar 31, 2020

darosior commented Mar 31, 2020

darosior Apr 1, 2020 •

edited

Loading

darosior Apr 1, 2020

cdecker Apr 1, 2020

darosior left a comment

darosior Apr 1, 2020

darosior Apr 1, 2020

darosior Apr 1, 2020

niftynei Apr 4, 2020 •

edited

Loading

rustyrussell Apr 8, 2020

darosior commented Apr 1, 2020

rustyrussell commented Apr 7, 2020

rustyrussell left a comment

rustyrussell Apr 7, 2020

rustyrussell Apr 7, 2020

cdecker commented Apr 8, 2020

cdecker commented Apr 9, 2020 •

edited

Loading

cdecker commented Apr 15, 2020

rustyrussell commented Apr 16, 2020 •

edited

Loading

whitslack commented Apr 19, 2020

Plugin implementation of keysend spontaneous payments #3611

Plugin implementation of keysend spontaneous payments #3611

Conversation

cdecker commented Mar 31, 2020

cdecker commented Mar 31, 2020

cdecker commented Mar 31, 2020

cdecker commented Mar 31, 2020

darosior commented Mar 31, 2020

cdecker commented Mar 31, 2020

darosior commented Mar 31, 2020

darosior Apr 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darosior left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niftynei Apr 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darosior commented Apr 1, 2020

rustyrussell commented Apr 7, 2020

rustyrussell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdecker commented Apr 8, 2020

cdecker commented Apr 9, 2020 • edited Loading

cdecker commented Apr 15, 2020

rustyrussell commented Apr 16, 2020 • edited Loading

whitslack commented Apr 19, 2020

Plugin implementation of `keysend` spontaneous payments #3611

Plugin implementation of `keysend` spontaneous payments #3611

darosior Apr 1, 2020 •

edited

Loading

niftynei Apr 4, 2020 •

edited

Loading

cdecker commented Apr 9, 2020 •

edited

Loading

rustyrussell commented Apr 16, 2020 •

edited

Loading