-
Notifications
You must be signed in to change notification settings - Fork 906
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
Add createonion
and sendonion
RPC commands
#3260
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, only minor changes requested.
Also, might want to see if your secret parsing helpers can be used more widely?
#if !EXPERIMENTAL_FEATURES | ||
if (hop->type != SPHINX_V0_PAYLOAD) | ||
return false; | ||
#endif | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changelog should actually announce TLV! (Changelog-Added: We handle modern TLV-style payloads). This is important because we never announce EXPERIMENTAL features.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was hoping you'd add a changelog entry to #3167, since you did all the heavy lifting there, but you're right, that we don't note experimental features in the changelog.
I'd propose the following ling:
Changelog-Added: Plugins may now handle modern TLV-style payloads via the `htlc_accepted` hook
That's a less broad claim than full TLV support for sending and receiving without a plugin, which as you noted is still experimental.
lightningd/pay.c
Outdated
else | ||
sp = sphinx_path_new_with_key(cmd, assocdata, session_key); | ||
|
||
json_for_each_arr(i, hop, hopstok) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better, BTW, to do the parsing in a new param_hops_array. That way you get proper checking if you use the 'check' rpc command. Might be too awkward though?
lightningd/pay.c
Outdated
buffer + pubkeytok->start); | ||
|
||
|
||
if (!typetok || !json_tok_streq(buffer, typetok, "legacy")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer an explicit test here? Either "legacy" or "raw" (the default), rather than a random string. Particularly if we add "tlv" later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is actually no difference between raw and tlv:
Lines 526 to 530 in 21555b2
/* Backwards compatibility for the legacy hop_data format. */ | |
if (hop->type == SPHINX_V0_PAYLOAD) | |
dest[pos++] = 0x00; | |
else | |
pos += bigsize_put(dest+pos, raw_size); |
Raw really is just confusing and we should remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if there's only two options; perhaps we should make this a boolean to flag if is 'legacy'?
otherwise i'd second the idea of defining a strict 'enum' parsing and rejecting unexpected/undefined values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have an enum for that (enum sphinx_payload_type
) so I went ahead and just added an explicit check for legacy
or tlv
, and added an error message if neither is used,
lightningd/pay.c
Outdated
|
||
if (hop_count == 0) | ||
return command_fail(cmd, JSONRPC2_INVALID_REQUEST, | ||
"Cannot create an onion without hops."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Onion but no hops? The beer would be terrible!"
CHANGELOG.md
Outdated
@@ -529,3 +529,4 @@ There predate the BOLT specifications, and are only of vague historic interest: | |||
[0.3]: https://github.com/ElementsProject/lightning/releases/tag/v0.3-2016-05-26 | |||
[0.2]: https://github.com/ElementsProject/lightning/releases/tag/v0.2-2016-01-22 | |||
[0.1]: https://github.com/ElementsProject/lightning/releases/tag/v0.1-2015-08-08 | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop this?
|
||
/* FIXME: Prior to 299b280f7, we didn't put route_nodes and | ||
* route_channels in db. If this happens, it's an old payment, | ||
* so we can simply mark it failed in db and return. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's OK to delete this (released in v0.6, so over 12 months ago: a payment is unlikely to still be pending since then), but you must place a justification for it in the commit msg. Like this one :)
common/json.c
Outdated
{ | ||
size_t seclen = sizeof(struct secret), hexlen = tok->end - tok->start; | ||
if (hexlen != 2 * seclen) | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hex_decode does this check for you, BTW!
i.e. this function is a one-liner.
} | ||
} else { | ||
path_secrets = NULL; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, it's be nice to implement this as param_secrets_array so 'check' rpc works better.
doc/lightning-createonion.7.md
Outdated
``` | ||
|
||
The `onion` corresponds to 1366 hex-encoded bytes. Each shared secret consists | ||
of 23 hex-encoded bytes. Both arguments can be passed on to **sendonion**. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
32 hex-encoded bytes?
Nice. Out of curiosity: will this allow sendpay to be re-implemented as a plugin? |
Indeed that's a possibility. An example that does this can be seen here 05c5146 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you inadvertently removed the randomization initialization for the onion padding.
lightningd/pay.c
Outdated
buffer + pubkeytok->start); | ||
|
||
|
||
if (!typetok || !json_tok_streq(buffer, typetok, "legacy")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if there's only two options; perhaps we should make this a boolean to flag if is 'legacy'?
otherwise i'd second the idea of defining a strict 'enum' parsing and rejecting unexpected/undefined values.
packet = create_onionpacket(cmd, sp, &shared_secrets); | ||
if (!packet) | ||
return command_fail(cmd, LIGHTNINGD, | ||
"Could not create onion packet"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe give a hint as to what input values might have caused the failure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a whole lot we can do actually, it either failed to generate the keys in generate_hop_params
which might happen due to invalid keys, or ecdh failing, or we failed to write an onion frame, e.g., payload was too long.
lightningd/pay.c
Outdated
response = json_stream_success(cmd); | ||
json_add_hex(response, "onion", serialized, tal_bytelen(serialized)); | ||
json_array_start(response, "shared_secrets"); | ||
for (size_t i=0; i<hop_count; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're really all in on those Golang spacing rules ;)
-> i = 0; i < hop_count;
tests/test_pay.py
Outdated
l1 = node_factory.get_node() | ||
|
||
hops = [{ | ||
"type": "legacy", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reiterating comment about a flag for 'is_legacy', since that seems to be most of what the type
field is used for here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that the spec reserves one value for further extensions: that is neither legacy nor TLV.
Perhaps we'll drop legacy before then and it won't be an issue though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made sure that either the style
is not provided (defaulting to TLV) or it is one that we know how to handle (legacy
or tlv
for now).
|
||
/* Check if at destination. */ | ||
if (origin_index == tal_count(route_nodes) - 1) { | ||
assert(route_nodes == NULL || origin_index < tal_count(route_nodes)); | ||
/* If any channel is to blame, it's the last one. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the indentation on this is wrong now since there's no if
; but the else
statement below is untouched 😕
it seems like this commit would fail to compile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, that's what I get for trying to disect a change into separate commits. Fixed in a fixup.
routing_failure->channel_dir = | ||
node_id_idx(&ld->id, &payment->route_nodes[0]); | ||
} else { | ||
routing_failure->erring_channel = NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does the channel_dir
not need to be set either?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving it unset on purpose so valgrind can tell us if we touch it inadvertently. We use the erring_channel
as sentinel for the presence of both, and if we touch the direction we know it's an error (it's used in the JSON-RPC only anyway).
lightningd/pay.c
Outdated
erring_channel = NULL; | ||
erring_node = NULL; | ||
|
||
/* We don't know if there's another route, that'd depend in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: depend in -> depend on
@@ -2273,6 +2280,7 @@ wallet_payment_by_hash(const tal_t *ctx, struct wallet *wallet, | |||
", msatoshi_sent" | |||
", description" | |||
", bolt11" | |||
", failonionreply" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this column already exist in the database? i don't see a migration statement in this commit 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does indeed 👍 We just didn't display it so far.
doc/lightning-createonion.7.md
Outdated
|
||
```json | ||
{ | ||
"onion": "0003f3f80d2142b953319336d2fe4097[..snip...]6af33fcf4fb113bce01f56dd62248a9e5fcbbfba35c", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
....:scissors:.... :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should change "type" to "style" since we've used that elsewhere. Consistency in APIs is really important.
That's a really weird naming convention if you ask me, but I changed it for the sake of consistency. I'll keep pubkey
though.
This branch predates that fix. I have rebased it locally, and checked that the fix is still in there. Will push as soon as I have addressed your feedback 👍 |
@niftynei: GH just decided to forget all the replies I had pending addressing your excellent poitns. I'll try to recreate the important ones from memory. Pushing the fixups and rebased stack now. |
Nevermind, GH decided to have a reverse lobotomy when I submitted the comment :-) |
Thanks @niftynei and @rustyrussell for the extensive feedback. I tried to address everything, and the resulting fixup stack is a bit big. Here's the range-diff for an easier comparison: eefd97ff9671b99714305e296e951d4c3075539c..124773bfb14c56e0ea6d474a5872bf3b7581cd72 |
As always, happy to squash in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really cool!
|
||
try: | ||
l1.rpc.waitsendpay(payment_hash=payment_hash) | ||
raise ValueError() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or the else:
block can be used but it's strictly the same behaviour.
These are useful for the `createonion` JSON-RPC we're going to build next. The secret is used for the optional `session_key` while the hex-encoded binary is used for the `assocdata` field to which the onion commits. The latter does not have a constant size, hence the raw binary conversion.
This is what provides us with the ability to add custom fields in the payload when using `createonion` so make sure we actually have access to it. Changelog-Changed: The TLV payloads for the onion packets are no longer considered an experimental feature and generally available. Changelog-Added: Plugins may now handle modern TLV-style payloads via the `htlc_accepted` hook Signed-off-by: Christian Decker <@cdecker>
This allows us to create an onion in the JSON-RPC that we can then later inject with the `sendonion` command that we're about to implement.
If we use `sendonion` we don't actually know the destination, so we make the destination a pointer which is NULL if we don't know.
This means that c-lightning can now internally decrypt an eventual error message, and not force the caller to implement the decryption. The main difficulty was that we now have a new state (channels and nodes not specified, while shared_secrets are specified) which needed to be handled.
This makes the copy on write redundant.
If we can't decode the onion, because the onion got corrupted or we used `sendonion` without specifying the `shared_secrets` used, the best we can do is tell the caller instead.
Changelog-Added: Added `createonion` and `sendonion` JSON-RPC methods allowing the implementation of custom protocol extensions that are not directly implemented in c-lightning itself.
We're about to create a param helper for sphinx hops and this struct seems like the correct place to store the result.
Suggested-by: Rusty Russell <@rustyrussell> Signed-off-by: Christian Decker <@cdecker>
Suggested-by: Rusty Russell <@rustyrussell>
Suggested-by: Rusty Russell <@rustyrussell>
If we initiated the payment using an externally generated onion we don't know what the final hop gets, or even who it is, so we don't display the amount in these cases. I chose to show `null` instead in order not to break dependees that rely on the value being there.
I rebased and squashed the fixups. The fixups can still be seen here in the form of a range-diff Ping @rustyrussell and @niftynei 😉 |
Ack f256bd8 |
As suggested by @niftynei here: ElementsProject#3260 (comment) Suggested-by: Lisa Neigut <@niftynei> Suggested-by: Rusty Russell <@rustyrussell> Signed-off-by: Christian Decker <@cdecker>
As suggested by @niftynei here: ElementsProject#3260 (comment) Suggested-by: Lisa Neigut <@niftynei> Suggested-by: Rusty Russell <@rustyrussell> Signed-off-by: Christian Decker <@cdecker>
As suggested by @niftynei here: ElementsProject#3260 (comment) Suggested-by: Lisa Neigut <@niftynei> Suggested-by: Rusty Russell <@rustyrussell> Signed-off-by: Christian Decker <@cdecker>
As suggested by @niftynei here: ElementsProject#3260 (comment) Suggested-by: Lisa Neigut <@niftynei> Suggested-by: Rusty Russell <@rustyrussell> Signed-off-by: Christian Decker <@cdecker>
As suggested by @niftynei here: ElementsProject#3260 (comment) Suggested-by: Lisa Neigut <@niftynei> Suggested-by: Rusty Russell <@rustyrussell> Signed-off-by: Christian Decker <@cdecker>
As suggested by @niftynei here: ElementsProject#3260 (comment) Suggested-by: Lisa Neigut <@niftynei> Suggested-by: Rusty Russell <@rustyrussell> Signed-off-by: Christian Decker <@cdecker>
As suggested by @niftynei here: #3260 (comment) Suggested-by: Lisa Neigut <@niftynei> Suggested-by: Rusty Russell <@rustyrussell> Signed-off-by: Christian Decker <@cdecker>
Where is this chat plugin you speak of? |
@fontaine it can be found in https://github.com/lightningd/plugins/ along will other plugins.
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
Le vendredi, août 21, 2020 5:01 PM, Fontaine <notifications@github.com> a écrit :
… Where is this chat plugin you speak of?
—
You are receiving this because you commented.
Reply to this email directly, [view it on GitHub](#3260 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AFLK3F743W637JVOOA6P6M3SB2D3DANCNFSM4JNPINYQ).
|
I don’t see it in there... |
https://github.com/lightningd/plugins/tree/master/noise ..
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
Le vendredi, août 21, 2020 5:14 PM, Fontaine <notifications@github.com> a écrit :
…> ***@***.***(https://github.com/fontaine) it can be found in https://github.com/lightningd/plugins/ along will other plugins.
>
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> Le vendredi, août 21, 2020 5:01 PM, Fontaine ***@***.*** a écrit :
>
>> Where is this chat plugin you speak of?
>>
>> —
>> You are receiving this because you commented.
>> Reply to this email directly, [view it on GitHub](#3260 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AFLK3F743W637JVOOA6P6M3SB2D3DANCNFSM4JNPINYQ).
I don’t see it in there...
—
You are receiving this because you commented.
Reply to this email directly, [view it on GitHub](#3260 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AFLK3FYP57NM66ZUXLYT423SB2FMHANCNFSM4JNPINYQ).
|
Thank you! Oops i was looking at the table instead of the actual directory. Amazing!
Best Regards,
Peter Denton
… On Aug 21, 2020, at 11:16 PM, Darosior ***@***.***> wrote:
https://github.com/lightningd/plugins/tree/master/noise ..
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
Le vendredi, août 21, 2020 5:14 PM, Fontaine ***@***.***> a écrit :
>> ***@***.***(https://github.com/fontaine) it can be found in https://github.com/lightningd/plugins/ along will other plugins.
>>
>> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
>> Le vendredi, août 21, 2020 5:01 PM, Fontaine ***@***.*** a écrit :
>>
>>> Where is this chat plugin you speak of?
>>>
>>> —
>>> You are receiving this because you commented.
>>> Reply to this email directly, [view it on GitHub](#3260 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AFLK3F743W637JVOOA6P6M3SB2D3DANCNFSM4JNPINYQ).
>
> I don’t see it in there...
>
> —
> You are receiving this because you commented.
> Reply to this email directly, [view it on GitHub](#3260 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AFLK3FYP57NM66ZUXLYT423SB2FMHANCNFSM4JNPINYQ).
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
This PR introduces two new RPC methods:
createonion
andsendonion
.createonion
is basically an RPC version of thedevtools/onion
CLI tool allowing the caller to create a custom onion. A custom onion can be
used to send arbitrary payloads to hops along a route, and thus allows
experimenting with protocol extensions. The counterpart is the
htlc_accepted
hook which allows a plugin implementing the handling of a custom payload
without having to modify the c-lightning source itself.
The
sendonion
RPC method can be used to initiate a payment using a customonion, independently from how or where it was constructed. This was achieved
by splitting
sendpay
into the onion creation part and the HTLC additionpart, and injecting the onion into the latter. We therefore reuse the entire
payments machinery and can use
listsendpays
to inspect in-flight andprevious payments initiated with
sendonion
.Depending on whether
sendonion
was given the shared secrets it can decryptthe onion it receives, and thus also decrypt an eventual error message
returned from a node along the route. If the shared secrets have not been
provided the daemon cannot decrypt the error and any error will result in the
ecrypted error being stored and exposed in
listsendpays
so the caller, whichknows the shared secrets, can decrypt it externally. This allows oblivious
sending of payments in which the node only learns what would be revealed to
intermediate hops, but not the length or the nodes in the route.
Use cases
waiting for. It allows an incoming payment to be intercepted by a plugin on
one blockchain, and then reinjected into a node running on the other
blockchain, without having to generate an onion from scratch.
createonion
also allows the inclusion of whatever metadata is necessary to signal to
the swap node what to do (which chain to swap to, what exchange rate,
etc.).
experiments can be based on
createonion
andsendonion
. Here are a fewexamples:
key to be swapped out at the rendez-vous node. The rendez-vous point can
intercept using
htlc_accepted
mangle the onion packet as required andthen re-inject the modified onion into the node. Forwarding is then
simply handled by the plugin.
and then takes over by searching routes to the destination / next
trampoline and driving retries until the payment succeeds or must fail.
should be mostly compatible. It uses
createonion
andsendonion
tostuff the chat message in the onion payload for the destination.
wallet interface, and inject it, we can hide the route and destination from
the node itself, reducing the information that a compromised node may learn
from the node alone.
Open Questions
createonion
andsendonion
be developer-only (at least initially)?Depends #3278
Closes #3227
Fixes #1715