From a5aba71fe1061fcb05fd3cc38061229d3d2d2617 Mon Sep 17 00:00:00 2001 From: Vincenzo Palazzo Date: Wed, 25 Mar 2026 14:51:40 +0100 Subject: [PATCH 1/5] tests: add regression test for #8902 dual-fund disconnect BROKEN Add test_inflight_disconnect_commitment_v2 which triggers a disconnect at +WIRE_COMMITMENT_SIGNED during a dual-funded open. --- 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 82f2e6baf631..daf288fde1b5 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 @@ -3089,3 +3089,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 9552117893fd9df8dd98397e210e243f16727c49 Mon Sep 17 00:00:00 2001 From: niftynei Date: Sat, 8 Aug 2026 16:35:57 -0500 Subject: [PATCH 2/5] 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. --- openingd/dualopend.c | 66 ++++++++++++++++++++++++++++++-------------- 1 file changed, 46 insertions(+), 20 deletions(-) diff --git a/openingd/dualopend.c b/openingd/dualopend.c index e1940a44e516..557a855575f9 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; } @@ -2751,11 +2774,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); @@ -3462,19 +3482,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 344a66112b13e613b03e95005436668004584f6e Mon Sep 17 00:00:00 2001 From: niftynei Date: Sat, 8 Aug 2026 18:11:43 -0500 Subject: [PATCH 3/5] 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-None --- 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 daf288fde1b5..02c1a2e7d65d 100644 --- a/tests/test_opening.py +++ b/tests/test_opening.py @@ -3093,10 +3093,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 8238f79453a205a2e1cfacdcd5d5640c6b1d27fe Mon Sep 17 00:00:00 2001 From: daywalker90 Date: Mon, 17 Aug 2026 13:35:37 +0200 Subject: [PATCH 4/5] fixup! dualopend: Parse error messages returned from lightningd correctly --- openingd/dualopend.c | 44 +++++++++++++++++++++----------------------- 1 file changed, 21 insertions(+), 23 deletions(-) diff --git a/openingd/dualopend.c b/openingd/dualopend.c index 557a855575f9..f5535fdd87f0 100644 --- a/openingd/dualopend.c +++ b/openingd/dualopend.c @@ -553,29 +553,27 @@ static void handle_failure_fatal(struct state *state, u8 *msg) } static bool check_accepter_error(struct state *state, - u8 *msg, - char *err_reason) + 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 */ + /* `msg` could be a failure message */ if (fromwire_peektype(msg) == WIRE_DUALOPEND_FAIL) { - handle_failure_fatal(state, msg); + 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 false; + } - return true; + return true; } static void check_channel_id(struct state *state, @@ -3482,21 +3480,21 @@ static void rbf_wrap_up(struct state *state, else msg = opener_commits(state, tx_state, total, &err_reason); - /* in TX_ACCEPTER case, `msg` could be a failure message */ + /* 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 (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); From 55a0bd671b6f68f4c31f002044c002e4f47d5333 Mon Sep 17 00:00:00 2001 From: daywalker90 Date: Mon, 17 Aug 2026 13:35:52 +0200 Subject: [PATCH 5/5] fixup! tests: add regression test for #8902 dual-fund disconnect BROKEN --- tests/test_opening.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/tests/test_opening.py b/tests/test_opening.py index 02c1a2e7d65d..5d8309c0be50 100644 --- a/tests/test_opening.py +++ b/tests/test_opening.py @@ -3124,4 +3124,15 @@ def test_inflight_disconnect_commitment_v2(node_factory, bitcoind): opener.daemon.wait_for_log(r'dev_disconnect: .WIRE_COMMITMENT_SIGNED') opener.rpc.connect(funder.info['id'], 'localhost', funder.port) - fut.result(timeout=TIMEOUT) + + # The disconnect kills the opener's dualopend mid-commitment. Depending on + # timing, the opener's `fundchannel` (via spenderp) either completes the + # open over the reconnected link, or fails: both are acceptable. The + # regression we guard against here is the funder going BROKEN (#8902). + try: + fut.result(timeout=TIMEOUT) + except RpcError: + pass + + # The funder must not have gone BROKEN. + assert not funder.daemon.is_in_log('BROKEN')