New onion_message support 3/3: Make fetchinvoice/offers send/receive both #4800
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cdecker
approved these changes
Sep 21, 2021
| "blinding_in" field, the former never will. | ||
| These three hooks are almost identical, in that they are called when | ||
| an onion message is received. The `onion_message` hook is only used | ||
| for obsolete unblinded messages, and can be ignored for modern usage. |
There was a problem hiding this comment.
obsolete sounds very harsh, especially in the docs, but I can't come up with a better word.
There was a problem hiding this comment.
It will get removed next version anyway, so that will resolve the wording debate :)
| &payload->reply_first_node, | ||
| &payload->reply_path, | ||
| &submsg)) { | ||
| log_broken(ld->log, "bad got_onionmsg_tous: %s", |
There was a problem hiding this comment.
nit: got_onionmsg_tous -> got_onionmsg_to_us
There was a problem hiding this comment.
Oops, this was actually in 2/3 and I already merged that. Oh well, this code is obsolete anyway....
|
Seems like |
|
Found it: actually a longstanding issue. See commit message for first commit... |
rustyrussell
force-pushed
the
guilt/new-onionmessage
branch
from
8d76487
to
b9e1782
Compare
When we are calling hooks, we track them via a linked list. As they execute, we pop them off the list in plugin_hook_killed(). When we kill a plugin, we have a destructor which remove its entry from the linked list: plugin_hook_killed. If it's at the head of the list, that means the plugin died while processing the hook, so instead of just deleting it, we call plugin_hook_killed() which behaves as if it said "result: continue". But plugin_hook_killed() just returns if we're shutting down; this leaves the link (then freed) on the list, and the *next* plugin tries to unlink from the list, accessing the previous free entry. The fix is simple: unlink from the list in plugin_hook_killed() even if we're shutting down. ``` Valgrind error file: valgrind-errors.78570 ==78570== Invalid write of size 8 ==78570== at 0x174B55: list_del_ (list.h:328) ==78570== by 0x174FCC: plugin_hook_killed (plugin_hook.c:135) ==78570== by 0x21DC3F: notify (tal.c:240) ==78570== by 0x21E156: del_tree (tal.c:402) ==78570== by 0x21E1A8: del_tree (tal.c:412) ==78570== by 0x21E4F2: tal_free (tal.c:486) ==78570== by 0x16EBD1: plugin_kill (plugin.c:345) ==78570== by 0x16F9C4: plugin_conn_finish (plugin.c:724) ==78570== by 0x20F1A5: destroy_conn (poll.c:244) ==78570== by 0x20F1C9: destroy_conn_close_fd (poll.c:250) ==78570== by 0x21DC3F: notify (tal.c:240) ==78570== by 0x21E156: del_tree (tal.c:402) ==78570== Address 0x6aee688 is 40 bytes inside a block of size 72 free'd ==78570== at 0x483CA3F: free (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==78570== by 0x21E224: del_tree (tal.c:421) ==78570== by 0x21E1A8: del_tree (tal.c:412) ==78570== by 0x21E4F2: tal_free (tal.c:486) ==78570== by 0x16EBD1: plugin_kill (plugin.c:345) ==78570== by 0x16F9C4: plugin_conn_finish (plugin.c:724) ==78570== by 0x20F1A5: destroy_conn (poll.c:244) ==78570== by 0x20F1C9: destroy_conn_close_fd (poll.c:250) ==78570== by 0x21DC3F: notify (tal.c:240) ==78570== by 0x21E156: del_tree (tal.c:402) ==78570== by 0x21E4F2: tal_free (tal.c:486) ==78570== by 0x20D7B6: io_close (io.c:450) ==78570== Block was alloc'd at ==78570== at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==78570== by 0x21DCAD: allocate (tal.c:250) ==78570== by 0x21E26E: tal_alloc_ (tal.c:428) ==78570== by 0x175599: plugin_hook_call_ (plugin_hook.c:259) ==78570== by 0x13616F: plugin_hook_call_onion_message_blinded (onion_message.c:126) ==78570== by 0x13643B: handle_obs_onionmsg_to_us (onion_message.c:187) ==78570== by 0x138BBD: gossip_msg (gossip_control.c:140) ==78570== by 0x178AEC: sd_msg_read (subd.c:495) ==78570== by 0x20CA00: next_plan (io.c:59) ==78570== by 0x20D608: do_plan (io.c:407) ==78570== by 0x20D64A: io_ready (io.c:417) ==78570== by 0x20F8F1: io_loop (poll.c:445) ``` Signed-off-by: Rusty Russell <[email protected]>
This adds a new hook: onion_message_ourpath for when we know a message came in via a blinded path we created. The onion_message_blinded hook is now called for all other messages, since all messages are now blinded. Signed-off-by: Rusty Russell <[email protected]>
Signed-off-by: Rusty Russell <[email protected]>
Signed-off-by: Rusty Russell <[email protected]>
This comes in via the onion_message_ourpath hook, and we identify the path by checking the node alias it came to (vs the obsolete version which used the blinding). Signed-off-by: Rusty Russell <[email protected]>
We carefully copied the buffer, but the tok is inside an array. We get away with it for now, but with coming changes it gets freed. We need to copy the token and all the tokens within it. Signed-off-by: Rusty Russell <[email protected]>
Signed-off-by: Rusty Russell <[email protected]> Changelog-Experimental: Protocol: Updated onion_message support to match updated draft specification (with backwards compat for old version)
This lets us test that both work, as expected. Signed-off-by: Rusty Russell <[email protected]>
rustyrussell
force-pushed
the
guilt/new-onionmessage
branch
from
b9e1782
to
c46211d
Compare
|
ACK c46211d |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(Depends on #4799)