Plugin and broadcast cleanups #6751
Merged
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
rustyrussell
force-pushed
the
plugin-and-broadcast-cleanups
branch
from
09174dd to
f5e6740
Compare
rustyrussell
force-pushed
the
plugin-and-broadcast-cleanups
branch
from
f5e6740 to
28c1f72
Compare
There was a problem hiding this comment.
This was a very good Pandora Box's Eh?
BTW In the CI I found this but looks like just a daemon dies in an unlucky time? but maybe also that the new change makes some problem that It is not so evident from the PR?
ightningd-2 2023-10-11T07:17:21.749Z DEBUG 0266e4598d1d3c415f572a8488830b60f7e744ed9235eb0b1ba93283b315c03518-lightningd: Not reconnecting: no active channel
lightningd-2 2023-10-11T07:17:21.750Z DEBUG connectd: maybe_free_peer freeing peer!
lightningd-2 2023-10-11T07:17:21.750Z DEBUG plugin-funder: Cleaning up inflights for peer id 0266e4598d1d3c415f572a8488830b60f7e744ed9235eb0b1ba93283b315c03518
----------------------------- Captured stderr call -----------------------------
Sending closingd an invalid message 03f40000006d
lightningd: FATAL SIGNAL 6 (version 027c8e8-modded)
0x55ac2b2f739e send_backtrace
common/daemon.c:33
0x55ac2b2f7589 crashdump
common/daemon.c:75
0x7fd34344251f ???
???:0
0x7fd343496a7c ???
???:0
0x7fd343442475 ???
???:0
0x7fd3434287f2 ???
???:0
0x55ac2b27ff35 fatal_vfmt
lightningd/log.c:1025
0x55ac2b27ffea fatal
lightningd/log.c:1035
0x55ac2b2c044b subd_send_msg
lightningd/subd.c:841
0x55ac2b2509d2 try_update_blockheight
lightningd/channel_control.c:143
0x55ac2b255175 channel_notify_new_block
lightningd/channel_control.c:1688
0x55ac2b27b366 notify_new_block
lightningd/lightningd.c:743
0x55ac2b24aa5f updates_complete
lightningd/chaintopology.c:840
0x55ac2b24b59f get_new_block
lightningd/chaintopology.c:1055
0x55ac2b246b11 getrawblockbyheight_callback
lightningd/bitcoind.c:474
0x55ac2b2b1836 plugin_response_handle
lightningd/plugin.c:627
0x55ac2b2b1ab2 plugin_read_json_one
lightningd/plugin.c:739
0x55ac2b2b1cef plugin_read_json
lightningd/plugin.c:790
0x55ac2b48ff60 next_plan
ccan/ccan/io/io.c:59
0x55ac2b490b95 do_plan
ccan/ccan/io/io.c:407
0x55ac2b490bd7 io_ready
ccan/ccan/io/io.c:417
0x55ac2b492f73 io_loop
ccan/ccan/io/poll.c:453
0x55ac2b275183 io_loop_with_timers
lightningd/io_loop_with_timers.c:22
0x55ac2b27c3ba main
lightningd/lightningd.c:1328
0x7fd343429d8f ???
???:0
0x7fd343429e3f ???
???:0
0x55ac2b243d64 ???
???:0
0xffffffffffffffff ???
???:0
Log dumped in crash.log.20231011071721
Fixed now in master... |
When using DEBUG_SUBD with pytest: ``` lightningd: Unknown decode for --dev-debugger=<subprocess> ``` Signed-off-by: Rusty Russell <[email protected]>
It's per-plugin, so why is there a single map for all plugins? It works because we always make unique ids, but it's weird. Signed-off-by: Rusty Russell <[email protected]>
It was always a bit weird they weren't, and it seems a premature optimization to make the callbacks to this themselves. Signed-off-by: Rusty Russell <[email protected]>
We had special code to fail a forwarded request, but not for an internally-generated request. Instead, we should pretend the (dead) plugin responded with a PLUGIN_TERMINATED error, and handle the request through the normal paths. This breaks the case where a plugin crashes (or stops itself) in a hook, so we handle that next. Signed-off-by: Rusty Russell <[email protected]>
Now the internal code will generate a "PLUGIN_TERMINATED" response when the plugin dies, we can handle it in plugin_hook. But we can also simplify it by turning the snapshot of hooks into a simple array: this means we are robust against any combination of plugins exiting at any time. Note: this reveals an issue with test_rpc_command_hook where we run the request hook again (unexpectedly), so we disable that for the next patch. Signed-off-by: Rusty Russell <[email protected]>
We should really unify the cases of a local request, vs a forwarded request, but for now, don't steal the request onto the plugin, and if we return from the plugin and the request is gone, don't get upset. This uncovered a case where we weren't inside a transaction, in test_hook_crash, so fix that. Signed-off-by: Rusty Russell <[email protected]>
We remove it from the pending_requests strmap before calling it, so it doesn't get called again by destroy_plugin. Signed-off-by: Rusty Russell <[email protected]>
If the context is freed, the callback isn't called. This doesn't matter yet, since our callbacks tend to be such that the callback itself is required to free things, but it's clearer this way and allows more flexible usage in following patches. Signed-off-by: Rusty Russell <[email protected]>
Previously, every broadcast was attached to a channel, but we can make it explicit, so when the context is freed, the re-broadcast stops (if rebroadcast is set). Signed-off-by: Rusty Russell <[email protected]>
…ion. We have to work quite hard to do this, since we don't want to call finish if the broadcast has been freed in the meantime. Signed-off-by: Rusty Russell <[email protected]>
rustyrussell
force-pushed
the
plugin-and-broadcast-cleanups
branch
from
28c1f72 to
c2f0c79
Compare
|
Rebased due to trivial conflict. |
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.
These were pre-work I had to do when cleaning anchors. The plugin code was confusing, so I unified the "forwarding a JSON request" vs "our own JSON request", especially handling the plugin dying during these (we didn't handle the latter case at all, we would deref freed memory).
I then go on to refine our broadcast interfaces, since we're going to need a more powerful mechanism for using anchors on the peer's commitment txs.