From b5a5acb44a0ad14fcd1392f16b7851da8cfebd5e Mon Sep 17 00:00:00 2001 From: niftynei Date: Mon, 24 Aug 2026 19:05:58 -0500 Subject: [PATCH 1/8] ccan/io: retain registrations during poll dispatch A callback can close a connection while readiness from the current poll call is still being dispatched. Deleting that connection compacts the fd table, and registering a replacement can restore its previous length with different occupants. A snapshot tied to mutable table positions can consequently skip an original event or deliver stale readiness to the replacement. Give each fd a stable registration object and retain that registration while a poll result refers to it. Deletion clears the registration's fd pointer, so pending readiness for a retired connection is ignored, while a replacement receives a distinct registration and cannot inherit the old event. Snapshot only entries with nonzero revents and preserve the IO_ALWAYS marker in the existing fairness order. Reuse a growable snapshot buffer between loop iterations and dispatch directly through the retained registration, avoiding both a fresh allocation on every poll and the repeated linear lookup required by the earlier generation-based approach. Register one process-exit cleanup for that reusable buffer. Retaining it for the daemon lifetime preserves allocation reuse, while releasing it after event dispatch can no longer be active avoids a reachable allocation in Valgrind and LeakSanitizer runs. Changelog-Fixed: io: replacing a file descriptor during poll dispatch no longer delivers stale readiness events to the replacement connection. --- ccan/ccan/io/backend.h | 2 + ccan/ccan/io/io.h | 5 ++ ccan/ccan/io/poll.c | 169 +++++++++++++++++++++++++++++++++++++++- connectd/connectd.c | 1 + lightningd/lightningd.c | 1 + 5 files changed, 177 insertions(+), 1 deletion(-) diff --git a/ccan/ccan/io/backend.h b/ccan/ccan/io/backend.h index 714972d15ea4..5934e154cd2c 100644 --- a/ccan/ccan/io/backend.h +++ b/ccan/ccan/io/backend.h @@ -11,6 +11,8 @@ struct fd { /* We could put these in io_plan, but they pack nicely here */ bool exclusive[2]; size_t backend_info; + /* Stable while a poll result still refers to this registration. */ + struct io_fd_registration *registration; }; /* Listeners create connections. */ diff --git a/ccan/ccan/io/io.h b/ccan/ccan/io/io.h index 5d084828b96a..b4eecb3cfd77 100644 --- a/ccan/ccan/io/io.h +++ b/ccan/ccan/io/io.h @@ -816,6 +816,11 @@ struct timemono (*io_time_override(struct timemono (*now)(void)))(void); */ int (*io_poll_override(int (*poll)(struct pollfd *fds, nfds_t nfds, int timeout)))(struct pollfd *, nfds_t, int); +/* Protect daemons which replace connections inside readiness callbacks from + * delivering an old poll result to a newly registered fd object. */ +void io_poll_protect_stale_fds(void); + + /** * io_have_fd - do we own this file descriptor? * @fd: the file descriptor. diff --git a/ccan/ccan/io/poll.c b/ccan/ccan/io/poll.c index c4cbaee85678..a6b3b45941e2 100644 --- a/ccan/ccan/io/poll.c +++ b/ccan/ccan/io/poll.c @@ -11,13 +11,69 @@ #include #include +struct io_fd_registration { + struct fd *fd; + size_t refs; +}; + +struct ready_fd { + /* NULL is the position at which to service an IO_ALWAYS plan. */ + struct io_fd_registration *registration; + short revents; +}; + static size_t num_fds = 0, max_fds = 0, num_waiting = 0, num_always = 0, max_always = 0, num_exclusive = 0; static struct pollfd *pollfds = NULL; static struct fd **fds = NULL; +static bool protect_stale_fds = false; +static struct ready_fd *ready_fds = NULL; +static size_t ready_fds_capacity = 0; static struct io_plan **always = NULL; static struct timemono (*nowfn)(void) = time_mono; static int (*pollfn)(struct pollfd *fds, nfds_t nfds, int timeout) = poll; +static void cleanup_ready_fds(void) +{ + free(ready_fds); + ready_fds = NULL; + ready_fds_capacity = 0; +} + +static struct io_fd_registration *new_registration(struct fd *fd) +{ + struct io_fd_registration *registration = malloc(sizeof(*registration)); + + if (!registration) + return NULL; + registration->fd = fd; + registration->refs = 1; + return registration; +} + +static void registration_put(struct io_fd_registration *registration) +{ + assert(registration->refs != 0); + registration->refs--; + if (registration->refs == 0) + free(registration); +} + +void io_poll_protect_stale_fds(void) +{ + if (protect_stale_fds) + return; + if (atexit(cleanup_ready_fds) != 0) + abort(); + + /* This can be enabled after callers have already registered fds. */ + for (size_t i = 0; i < num_fds; i++) { + fds[i]->registration = new_registration(fds[i]); + if (!fds[i]->registration) + abort(); + } + protect_stale_fds = true; +} + struct timemono (*io_time_override(struct timemono (*now)(void)))(void) { struct timemono (*old)(void) = nowfn; @@ -54,6 +110,12 @@ static bool add_fd(struct fd *fd, short events) return false; max_fds = num; } + if (protect_stale_fds) { + fd->registration = new_registration(fd); + if (!fd->registration) + return false; + } else + fd->registration = NULL; pollfds[num_fds].events = events; /* In case it's idle. */ @@ -75,6 +137,7 @@ static bool add_fd(struct fd *fd, short events) static void del_fd(struct fd *fd) { size_t n = fd->backend_info; + struct io_fd_registration *registration = fd->registration; assert(n != -1); assert(n < num_fds); @@ -98,6 +161,12 @@ static void del_fd(struct fd *fd) } num_fds--; fd->backend_info = -1; + if (registration) { + assert(registration->fd == fd); + registration->fd = NULL; + fd->registration = NULL; + registration_put(registration); + } if (fd->exclusive[IO_IN]) num_exclusive--; @@ -369,6 +438,26 @@ static void restore_pollfds(void) } } +static void append_ready_fd(size_t *num_ready, + struct io_fd_registration *registration, + short revents) +{ + if (*num_ready == ready_fds_capacity) { + size_t capacity = ready_fds_capacity ? ready_fds_capacity * 2 : 8; + struct ready_fd *new_ready_fds; + + new_ready_fds = realloc(ready_fds, + sizeof(*ready_fds) * capacity); + if (!new_ready_fds) + abort(); + ready_fds = new_ready_fds; + ready_fds_capacity = capacity; + } + ready_fds[*num_ready].registration = registration; + ready_fds[*num_ready].revents = revents; + (*num_ready)++; +} + /* This is the main loop. */ void *io_loop(struct timers *timers, struct timer **expired) { @@ -431,6 +520,85 @@ void *io_loop(struct timers *timers, struct timer **expired) break; } + if (protect_stale_fds) { + size_t num_polled = num_fds; + size_t num_ready = 0; + + if (r == 0) { + handle_always(); + continue; + } + + /* Preserve the old fairness order, but retain only ready fds and + * the point where IO_ALWAYS work was interleaved. */ + fairness_counter++; + for (size_t rotation = 0; rotation < num_polled; rotation++) { + struct io_fd_registration *registration; + + i = (rotation + fairness_counter) % num_polled; + if (i == 0) + append_ready_fd(&num_ready, NULL, 0); + + if (!pollfds[i].revents) + continue; + registration = fds[i]->registration; + assert(registration); + registration->refs++; + append_ready_fd(&num_ready, registration, + pollfds[i].revents); + } + for (size_t n = 0; n < num_polled; n++) + pollfds[n].revents = 0; + + for (size_t n = 0; n < num_ready && !io_loop_return; n++) { + socklen_t errno_len = sizeof(errno); + struct io_fd_registration *registration; + struct fd *fd; + struct io_conn *c; + int events; + + registration = ready_fds[n].registration; + if (!registration) { + if (handle_always()) + break; + continue; + } + fd = registration->fd; + if (!fd) + continue; + events = ready_fds[n].revents; + c = (void *)fd; + if (fd->listener) { + struct io_listener *l = (void *)fd; + if (events & POLLIN) { + accept_conn(l); + r--; + } else if (events & (POLLHUP|POLLNVAL|POLLERR)) { + r--; + errno = EBADF; + io_close_listener(l); + } + } else if (events & (POLLIN|POLLOUT)) { + r--; + io_ready(c, events); + } else if (events & (POLLHUP|POLLNVAL|POLLERR)) { + r--; + if (getsockopt(fd->fd, SOL_SOCKET, + SO_ERROR, &errno, + &errno_len) == -1) + errno = EBADF; + io_close(c); + } + } + /* Callbacks can retire registrations, so release snapshot + * references only after dispatch has stopped using the buffer. */ + for (size_t n = 0; n < num_ready; n++) { + if (ready_fds[n].registration) + registration_put(ready_fds[n].registration); + } + continue; + } + fairness_counter++; for (size_t rotation = 0; rotation < num_fds && !io_loop_return; rotation++) { socklen_t errno_len = sizeof(errno); @@ -486,7 +654,6 @@ void *io_loop(struct timers *timers, struct timer **expired) } } } - ret = io_loop_return; io_loop_return = NULL; diff --git a/connectd/connectd.c b/connectd/connectd.c index cb95ef9b0cac..9e96a31ec8f9 100644 --- a/connectd/connectd.c +++ b/connectd/connectd.c @@ -2532,6 +2532,7 @@ int main(int argc, char *argv[]) /* Common subdaemon setup code. */ developer = subdaemon_setup(argc, argv); + io_poll_protect_stale_fds(); /* Allocate and set up our simple top-level structure. */ daemon = tal(NULL, struct daemon); diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index 44ed3fa66425..dfd159efa51d 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -1199,6 +1199,7 @@ int main(int argc, char *argv[]) /*~ What happens in strange locales should stay there. */ setup_locale(); + io_poll_protect_stale_fds(); /*~ This handles --dev-debug-self really early, which we otherwise ignore */ daemon_developer_mode(argv); From aedb1455934fe7dde24a33042f7566e180592465 Mon Sep 17 00:00:00 2001 From: niftynei Date: Mon, 24 Aug 2026 17:12:02 -0500 Subject: [PATCH 2/8] ccan/io: test fd replacement during poll dispatch Closing a connection while dispatching poll results compacts the fd table. If the callback immediately registers a replacement, the table can return to its original length with a different connection occupying a slot represented in the current poll result. Dispatching readiness by mutable table position can then deliver an old event to the replacement or skip another ready connection. Use a fake poll implementation to make three original connections ready. The connection selected first closes itself and installs a replacement while the other readiness events remain pending. Verify that every original event is delivered exactly once, that the replacement does not inherit stale readiness, and that it is handled normally by the following poll call. Register an exit assertion before enabling protection so atexit LIFO ordering checks that the backend releases its reusable snapshot buffer at process exit. This keeps leak-sensitive unit runs from accepting a permanently reachable allocation. Add the regression to check-units so this CCAN behavior is covered by the project's normal unit-test suite. Changelog-None --- ccan/ccan/io/test/run-49-stale-fd-readiness.c | 142 ++++++++++++++++++ common/test/Makefile | 9 +- 2 files changed, 148 insertions(+), 3 deletions(-) create mode 100644 ccan/ccan/io/test/run-49-stale-fd-readiness.c diff --git a/ccan/ccan/io/test/run-49-stale-fd-readiness.c b/ccan/ccan/io/test/run-49-stale-fd-readiness.c new file mode 100644 index 000000000000..6c5618424d5d --- /dev/null +++ b/ccan/ccan/io/test/run-49-stale-fd-readiness.c @@ -0,0 +1,142 @@ +#include "config.h" +#include +#include +/* Include the C files directly to make each readiness delivery observable. */ +#include +#include +#include + +static int replacement_fd; +static unsigned int poll_calls, actor_events, victim_events, + survivor_events, replacement_events; +static char completed; + +static void check_ready_fds_cleaned(void) +{ + assert(ready_fds == NULL); + assert(ready_fds_capacity == 0); +} + +static int replacement_event(int fd, struct io_plan_arg *arg) +{ + replacement_events++; + return 1; +} + +static struct io_plan *replacement_ready(struct io_conn *conn, void *unused) +{ + io_break(&completed); + return io_close(conn); +} + +static struct io_plan *replacement_init(struct io_conn *conn, void *unused) +{ + io_plan_arg(conn, IO_IN); + return io_set_plan(conn, IO_IN, replacement_event, + replacement_ready, NULL); +} + +static int actor_event(int fd, struct io_plan_arg *arg) +{ + actor_events++; + return 1; +} + +static struct io_plan *actor_ready(struct io_conn *conn, void *unused) +{ + struct io_plan *closed = io_close(conn); + + /* Closing the current fd compacts the table. Adding its replacement + * restores the old table length, but must not make it part of the poll + * result currently being dispatched. */ + if (!io_new_conn(NULL, replacement_fd, replacement_init, NULL)) + abort(); + return closed; +} + +static struct io_plan *actor_init(struct io_conn *conn, void *unused) +{ + io_plan_arg(conn, IO_IN); + return io_set_plan(conn, IO_IN, actor_event, actor_ready, NULL); +} + +static int victim_event(int fd, struct io_plan_arg *arg) +{ + victim_events++; + return 1; +} + +static struct io_plan *victim_init(struct io_conn *conn, void *unused) +{ + io_plan_arg(conn, IO_IN); + return io_set_plan(conn, IO_IN, victim_event, io_close_cb, NULL); +} + +static int survivor_event(int fd, struct io_plan_arg *arg) +{ + survivor_events++; + return 1; +} + +static struct io_plan *survivor_init(struct io_conn *conn, void *unused) +{ + io_plan_arg(conn, IO_IN); + return io_set_plan(conn, IO_IN, survivor_event, io_close_cb, NULL); +} + +static int fake_poll(struct pollfd *fds, nfds_t nfds, int timeout) +{ + poll_calls++; + for (size_t i = 0; i < nfds; i++) + fds[i].revents = 0; + + if (poll_calls == 1) { + /* Fairness rotation handles slot 1 first. That actor replaces + * itself while slots 0 and 2 still have readiness pending. */ + assert(nfds == 3); + for (size_t i = 0; i < nfds; i++) + fds[i].revents = POLLIN; + return 3; + } + + assert(poll_calls == 2); + for (size_t i = 0; i < nfds; i++) { + if (fds[i].fd != replacement_fd) + continue; + fds[i].revents = POLLIN; + return 1; + } + abort(); +} + +int main(void) +{ + int actor_pipe[2], victim_pipe[2], survivor_pipe[2], replacement_pipe[2]; + + assert(pipe(actor_pipe) == 0); + assert(pipe(victim_pipe) == 0); + assert(pipe(survivor_pipe) == 0); + assert(pipe(replacement_pipe) == 0); + replacement_fd = replacement_pipe[0]; + + assert(io_poll_override(fake_poll) == poll); + assert(io_new_conn(NULL, victim_pipe[0], victim_init, NULL)); + assert(io_new_conn(NULL, actor_pipe[0], actor_init, NULL)); + assert(io_new_conn(NULL, survivor_pipe[0], survivor_init, NULL)); + + /* Registered first so this runs after the backend's LIFO cleanup. */ + assert(atexit(check_ready_fds_cleaned) == 0); + io_poll_protect_stale_fds(); + assert(io_loop(NULL, NULL) == &completed); + assert(poll_calls == 2); + assert(actor_events == 1); + assert(victim_events == 1); + assert(survivor_events == 1); + assert(replacement_events == 1); + + close(actor_pipe[1]); + close(victim_pipe[1]); + close(survivor_pipe[1]); + close(replacement_pipe[1]); + return 0; +} diff --git a/common/test/Makefile b/common/test/Makefile index 71f9dc17d981..dd165bef0f45 100644 --- a/common/test/Makefile +++ b/common/test/Makefile @@ -1,7 +1,10 @@ +CCAN_IO_TEST_SRC := ccan/ccan/io/test/run-49-stale-fd-readiness.c COMMON_TEST_SRC := $(wildcard common/test/run-*.c) COMMON_TEST_OBJS := $(COMMON_TEST_SRC:.c=.o) COMMON_TEST_PROGRAMS := $(COMMON_TEST_OBJS:.o=) +CCAN_IO_TEST_OBJS := $(CCAN_IO_TEST_SRC:.c=.o) +CCAN_IO_TEST_PROGRAMS := $(CCAN_IO_TEST_OBJS:.o=) COMMON_TEST_COMMON_OBJS := \ common/autodata.o \ @@ -13,8 +16,8 @@ COMMON_TEST_COMMON_OBJS := \ $(COMMON_TEST_PROGRAMS): $(COMMON_TEST_COMMON_OBJS) $(BITCOIN_OBJS) $(COMMON_TEST_OBJS): $(COMMON_HEADERS) $(WIRE_HEADERS) $(COMMON_SRC) common/test/Makefile -ALL_C_SOURCES += $(COMMON_TEST_SRC) -ALL_TEST_PROGRAMS += $(COMMON_TEST_PROGRAMS) +ALL_C_SOURCES += $(COMMON_TEST_SRC) $(CCAN_IO_TEST_SRC) +ALL_TEST_PROGRAMS += $(COMMON_TEST_PROGRAMS) $(CCAN_IO_TEST_PROGRAMS) # Make them all depend on common/ files, for simplicity (they directly #include some) $(COMMON_TEST_OBJS): $(COMMON_SRC) @@ -178,4 +181,4 @@ common/test/run-close_tx: \ wire/fromwire.o \ wire/towire.o -check-units: $(COMMON_TEST_PROGRAMS:%=unittest/%) +check-units: $(COMMON_TEST_PROGRAMS:%=unittest/%) $(CCAN_IO_TEST_PROGRAMS:%=unittest/%) From aedac156a26371691499fa4668a7b6d0da829c94 Mon Sep 17 00:00:00 2001 From: Vincenzo Palazzo Date: Wed, 25 Mar 2026 14:51:40 +0100 Subject: [PATCH 3/8] tests: add regression test for #8902 dual-fund disconnect Add test_inflight_disconnect_commitment_v2 which triggers a disconnect at +WIRE_COMMITMENT_SIGNED during a dual-funded open. Changelog-None --- tests/test_opening.py | 42 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/tests/test_opening.py b/tests/test_opening.py index e23283f7251c..f67ea053ab7b 100644 --- a/tests/test_opening.py +++ b/tests/test_opening.py @@ -2,7 +2,7 @@ from fixtures import TEST_NETWORK from pyln.client import RpcError, Millisatoshi from utils import ( - only_one, wait_for, sync_blockheight, first_channel_id, calc_lease_fee, check_coin_moves + TIMEOUT, only_one, wait_for, sync_blockheight, first_channel_id, calc_lease_fee, check_coin_moves ) from pyln.testing.utils import FUNDAMOUNT @@ -3220,3 +3220,43 @@ def test_no_retransmit_confirmed_funding(node_factory): # Should not have attempted (and failed) to re-broadcast the funding tx. assert not l1.daemon.is_in_log('Failed to re-transmit funding tx') assert not l1.daemon.is_in_log('Successfully rexmitted funding tx') + + +@unittest.skipIf(TEST_NETWORK != 'regtest', 'elementsd doesnt yet support PSBT features we need') +@pytest.mark.openchannel('v2') +@pytest.mark.xfail( + reason="Issue #8902 is not fixed at this commit", + strict=True, +) +def test_inflight_disconnect_commitment_v2(node_factory, bitcoind): + """Disconnect during dual-fund commitment signing should not trigger spurious BROKEN messages. + """ + disconnects = ["+WIRE_COMMITMENT_SIGNED"] + + opts = [{'experimental-dual-fund': None, 'dev-no-reconnect': None, + 'may_reconnect': True, 'disconnect': disconnects}, + {'experimental-dual-fund': None, 'dev-no-reconnect': None, + 'may_reconnect': True}] + + opener, funder = node_factory.get_nodes(2, opts=opts) + + amount = 500000 + opener.fundwallet(20000000) + funder.fundwallet(20000000) + + funder.rpc.call('funderupdate', + {'policy': 'available', + 'policy_mod': 100, + 'per_channel_max_msat': '1btc', + 'reserve_tank_msat': '0msat', + 'fund_probability': 100, + 'fuzz_percent': 0, + 'leases_only': False}) + + opener.rpc.connect(funder.info['id'], 'localhost', funder.port) + fut = node_factory.executor.submit(opener.rpc.fundchannel, + funder.info['id'], amount) + + opener.daemon.wait_for_log(r'dev_disconnect: .WIRE_COMMITMENT_SIGNED') + opener.rpc.connect(funder.info['id'], 'localhost', funder.port) + fut.result(timeout=TIMEOUT) From 556a082a8c3fb9fd9fa9c8b0eda4a3f78ced83fa Mon Sep 17 00:00:00 2001 From: niftynei Date: Sat, 8 Aug 2026 16:35:57 -0500 Subject: [PATCH 4/8] dualopend: Parse error messages returned from lightningd correctly The error message in #8902 indicates that we're failing to correctly parse an error message from lightningd lightningd-2 2026-02-16T00:50:21.721Z **BROKEN** 038194b5f32bdf0aa59812c86c4ef7ad2f294104fa027d1ace9b469bb6f88cf37b-dualopend-chan#2: STATUS_FAIL_MASTER_IO: Error parsing 7011: 1b5b50656572206572726f7220776974682050534254207369676e6174757265732e00 The openchannel2_sign_hook_cb in lightningd can return error messages, not just the DUALOPEND_SEND_TX_SIGS message at this point. We handle this here. Changelog-Fixed: dualopend: dual-funding signing-hook errors are now reported correctly instead of causing a `dualopend` master-reply parse failure. --- openingd/dualopend.c | 66 ++++++++++++++++++++++++++++++-------------- 1 file changed, 46 insertions(+), 20 deletions(-) diff --git a/openingd/dualopend.c b/openingd/dualopend.c index 2f1849773496..978d8c95f304 100644 --- a/openingd/dualopend.c +++ b/openingd/dualopend.c @@ -552,6 +552,32 @@ static void handle_failure_fatal(struct state *state, u8 *msg) open_err_fatal(state, "%s", err); } +static bool check_accepter_error(struct state *state, + u8 *msg, + char *err_reason) +{ + if (!msg) { + if (err_reason) + negotiation_failed(state, "%s", err_reason); + else + /* FIXME: what do we do here?? */ + return false; + } + + /* `msg` could be a failure message */ + if (fromwire_peektype(msg) == WIRE_DUALOPEND_FAIL) { + handle_failure_fatal(state, msg); + return false; + } + + if (fromwire_peektype(msg) != WIRE_DUALOPEND_SEND_TX_SIGS) { + master_badmsg(WIRE_DUALOPEND_SEND_TX_SIGS, msg); + return false; + } + + return true; +} + static void check_channel_id(struct state *state, struct channel_id *id_in, struct channel_id *orig_id) @@ -2305,9 +2331,6 @@ static u8 *accepter_commits(struct state *state, wire_sync_write(REQ_FD, take(msg)); msg = wire_sync_read(tmpctx, REQ_FD); - if (fromwire_peektype(msg) != WIRE_DUALOPEND_SEND_TX_SIGS) - master_badmsg(WIRE_DUALOPEND_SEND_TX_SIGS, msg); - return msg; } @@ -2737,11 +2760,8 @@ static void accepter_start(struct state *state, const u8 *oc2_msg) } msg = accepter_commits(state, tx_state, total, &err_reason); - if (!msg) { - if (err_reason) - negotiation_failed(state, "%s", err_reason); - return; - } + if (!check_accepter_error(state, msg, err_reason)) + return; /* Finally, send our funding tx sigs */ handle_send_tx_sigs(state, msg); @@ -3441,19 +3461,25 @@ static void rbf_wrap_up(struct state *state, else msg = opener_commits(state, tx_state, total, &err_reason); - if (!msg) { - if (err_reason) - open_abort(state, "%s", err_reason); - else - open_abort(state, "%s", "Unable to commit"); - /* We need to 'reset' the channel to what it - * was before we did this. */ - return; - } - - if (state->our_role == TX_ACCEPTER) + /* in TX_ACCEPTER case, `msg` could be a failure message */ + if (msg && (fromwire_peektype(msg) == WIRE_DUALOPEND_FAIL)) { + if (fromwire_dualopend_fail(msg, msg, &err_reason)) + msg = tal_free(msg); + } + + if (!msg) { + if (err_reason) + open_abort(state, "%s", err_reason); + else + open_abort(state, "%s", "Unable to commit"); + /* We need to 'reset' the channel to what it + * was before we did this. */ + return; + } + + if (state->our_role == TX_ACCEPTER) { handle_send_tx_sigs(state, msg); - else + } else wire_sync_write(REQ_FD, take(msg)); } From 392c3633dff8455848969a245ff6d9d679272f2c Mon Sep 17 00:00:00 2001 From: niftynei Date: Sat, 8 Aug 2026 18:11:43 -0500 Subject: [PATCH 5/8] funder: remove listener for peer disconnects Issue #8902 demonstrates that there are races conditions ocurring when we use the peer disconnection notifications. In theory, we don't actually need to listen for peer disconnects, as we're already listening for open attempt failures with both the state_change and the channel_open_failed notifications. Changelog-Fixed: funder: peer disconnects no longer race channel-open failure handling when cleaning up pending dual-funded opens. --- plugins/funder.c | 41 ----------------------------------------- tests/test_opening.py | 4 ---- 2 files changed, 45 deletions(-) diff --git a/plugins/funder.c b/plugins/funder.c index 6ce91b6959de..b172e72ce3be 100644 --- a/plugins/funder.c +++ b/plugins/funder.c @@ -117,17 +117,6 @@ static struct command_result *unreserve_psbt(struct command *cmd, return command_still_pending(aux); } -static void cleanup_peer_pending_opens(struct command *cmd, - const struct node_id *id) -{ - struct pending_open *i, *next; - list_for_each_safe(&pending_opens, i, next, list) { - if (node_id_eq(&i->peer_id, id)) { - unreserve_psbt(cmd, i); - } - } -} - static struct command_result * command_hook_cont_psbt(struct command *cmd, struct wally_psbt *psbt) { @@ -1086,32 +1075,6 @@ json_rbf_channel_call(struct command *cmd, return send_outreq(req); } -static struct command_result *json_disconnect(struct command *cmd, - const char *buf, - const jsmntok_t *params) -{ - struct node_id id; - const char *err; - - err = json_scan(tmpctx, buf, params, - "{disconnect:{id:%}}", - JSON_SCAN(json_to_node_id, &id)); - if (err) - plugin_err(cmd->plugin, - "`disconnect` notification payload did not" - " scan %s: %.*s", - err, json_tok_full_len(params), - json_tok_full(buf, params)); - - plugin_log(cmd->plugin, LOG_DBG, - "Cleaning up inflights for peer id %s", - fmt_node_id(tmpctx, &id)); - - cleanup_peer_pending_opens(cmd, &id); - - return notification_handled(cmd); -} - static struct command_result * delete_channel_from_datastore(struct command *cmd, struct channel_id *cid) @@ -1552,10 +1515,6 @@ const struct plugin_notification notifs[] = { "channel_open_failed", json_channel_open_failed, }, - { - "disconnect", - json_disconnect, - }, { "channel_state_changed", json_channel_state_changed, diff --git a/tests/test_opening.py b/tests/test_opening.py index f67ea053ab7b..5fac530779d3 100644 --- a/tests/test_opening.py +++ b/tests/test_opening.py @@ -3224,10 +3224,6 @@ def test_no_retransmit_confirmed_funding(node_factory): @unittest.skipIf(TEST_NETWORK != 'regtest', 'elementsd doesnt yet support PSBT features we need') @pytest.mark.openchannel('v2') -@pytest.mark.xfail( - reason="Issue #8902 is not fixed at this commit", - strict=True, -) def test_inflight_disconnect_commitment_v2(node_factory, bitcoind): """Disconnect during dual-fund commitment signing should not trigger spurious BROKEN messages. """ From 79bfe7c5cbe30f5c95435da68c0e999c3e6898e0 Mon Sep 17 00:00:00 2001 From: niftynei Date: Fri, 21 Aug 2026 21:09:45 -0500 Subject: [PATCH 6/8] dualopend: free penalty base on commit failure opener_commits allocates a temporary penalty base before negotiating the commitment and validating the remote signature. Both failure paths revert the channel state and return without transferring ownership, so they must release that allocation explicitly. Changelog-Fixed: dualopend: failed commitment negotiations no longer leak their temporary penalty-base allocation. --- openingd/dualopend.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/openingd/dualopend.c b/openingd/dualopend.c index 978d8c95f304..169311273e08 100644 --- a/openingd/dualopend.c +++ b/openingd/dualopend.c @@ -2928,6 +2928,7 @@ static u8 *opener_commits(struct state *state, msg = opening_negotiate_msg(tmpctx, state); if (!msg) { *err_reason = NULL; + tal_free(pbase); revert_channel_state(state); return NULL; } @@ -2937,6 +2938,7 @@ static u8 *opener_commits(struct state *state, &remote_sig); if (error) { *err_reason = tal_fmt(tmpctx, "Commit sig error: %s", error); + tal_free(pbase); revert_channel_state(state); return NULL; } From d470471ea0518af295a5fb22aebab099c06c5076 Mon Sep 17 00:00:00 2001 From: niftynei Date: Fri, 21 Aug 2026 21:09:59 -0500 Subject: [PATCH 7/8] dualopend: report HSM transport failures explicitly fetch_per_commitment_point previously ignored a failed write and passed a NULL read result to the wire decoder. An HSM disconnect could therefore appear as a malformed reply or crash instead of exposing the underlying transport failure. Check both sides of the synchronous HSM exchange and fail with STATUS_FAIL_HSM_IO at the point of failure. Clear errno before reading so a NULL result reports EOF rather than an unrelated stale error. Changelog-Fixed: dualopend: HSM transport failures are now reported directly instead of appearing as malformed replies or crashes. --- openingd/dualopend.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/openingd/dualopend.c b/openingd/dualopend.c index 169311273e08..3d1023cacdd8 100644 --- a/openingd/dualopend.c +++ b/openingd/dualopend.c @@ -4316,9 +4316,18 @@ static void fetch_per_commitment_point(u32 point_count, u8 *msg; struct secret *none; - wire_sync_write(HSM_FD, - take(towire_hsmd_get_per_commitment_point(NULL, point_count))); + if (!wire_sync_write(HSM_FD, + take(towire_hsmd_get_per_commitment_point(NULL, + point_count)))) + status_failed(STATUS_FAIL_HSM_IO, + "Writing get_per_commitment_point: %s", + strerror(errno)); + errno = 0; msg = wire_sync_read(tmpctx, HSM_FD); + if (!msg) + status_failed(STATUS_FAIL_HSM_IO, + "Reading get_per_commitment_point reply: %s", + errno == 0 ? "EOF" : strerror(errno)); if (!fromwire_hsmd_get_per_commitment_point_reply(tmpctx, msg, commit_point, &none)) From eb39e182ef1a7d7767a89d82f122bbcc68201994 Mon Sep 17 00:00:00 2001 From: niftynei Date: Fri, 21 Aug 2026 21:10:26 -0500 Subject: [PATCH 8/8] lightningd: retire openingd before starting channeld hsmd permits only one client for a channel database ID. Starting channeld before the completed openingd owner has exited can therefore race creation of the replacement HSM client, particularly when several v1 opens finish at the same time. Release openingd after preserving its peer endpoint and before asking channeld to acquire the channel's HSM client. This makes owner exit an explicit barrier without blocking the handling of other completed opens. Changelog-Fixed: openingd: completed v1 channel opens no longer race `openingd` teardown against `channeld` HSM client creation. --- lightningd/opening_control.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/lightningd/opening_control.c b/lightningd/opening_control.c index 6cc3d82f1e61..eae76b982a6d 100644 --- a/lightningd/opening_control.c +++ b/lightningd/opening_control.c @@ -453,6 +453,13 @@ static void opening_funder_finished(struct subd *openingd, const u8 *resp, goto cleanup; } + /* The peer endpoint is safely held by peer_fd now, and openingd has + * completed its work. Retire it before peer_start_channeld() asks hsmd + * for another client with this dbid: hsmd deliberately will not create + * that client until the previous owner has gone away. */ + fc->uc->open_daemon = NULL; + subd_release_channel(openingd, fc->uc); + /* Watch for funding confirms */ channel_watch_funding(ld, channel); @@ -567,6 +574,13 @@ static void opening_fundee_finished(struct subd *openingd, goto failed; } + /* Establish an explicit owner-exit barrier before requesting channeld's + * HSM client. This is particularly important when several v1 opens + * finish concurrently: waiting synchronously for one lingering openingd + * must not delay processing another openingd's returned peer endpoint. */ + uc->open_daemon = NULL; + subd_release_channel(openingd, uc); + log_debug(channel->log, "Watching funding tx %s", fmt_bitcoin_txid(reply, &channel->funding.txid));