From f0b50049907146d286d97d8bf78b80d0310fde2d Mon Sep 17 00:00:00 2001 From: Vincenzo Palazzo Date: Sat, 8 Aug 2026 14:49:25 +0200 Subject: [PATCH 1/2] lightningd: don't disconnect when sending error for unknown channel_reestablish When a peer sends WIRE_CHANNEL_REESTABLISH for a channel we don't know about (e.g. dual-funding, where we deleted the unsaved channel on disconnect but the peer saved it in DUALOPEND_OPEN_COMMIT_READY), we sent an error and hung up. Our error does make it onto the wire: disconnect_peer() -> drain_peer() in connectd/multiplex.c gives peer_outq 5 seconds to flush first. The race is on the receiving node: its connectd hands the error to dualopend and tears the subds down on EOF at the same time, and when dualopend loses that race lightningd only sees "Owning subdaemon dualopend died" (subd.c passes peer_fd=NULL, disconnect=false), so dualopen_errmsg() keeps the DUALOPEND_OPEN_COMMIT_READY channel for a later reconnect. It then reestablishes on every reconnect, and we error and hang up again. BOLT #1 only requires that a node sending `error` fails the channel(s) the error refers to; it never says to drop the connection, and here we don't know the channel, so there is nothing for us to fail. So send the error and stay connected: the peer's dualopend then reliably reads it and forgets the channel. Since we no longer hang up, bound how many unknown-channel reestablishes we answer on a single connection, so a peer can't use this to make us log and write errors indefinitely. Changelog-Fixed: dual-funding reconnect loop when peer doesn't know about a saved channel Fixes: https://github.com/ElementsProject/lightning/issues/8822 Signed-off-by: Vincenzo Palazzo --- lightningd/peer_control.c | 47 ++++++++++++++++++++++++++++++++++----- lightningd/peer_control.h | 5 +++++ tests/test_misc.py | 18 ++++++++++----- 3 files changed, 58 insertions(+), 12 deletions(-) diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index 935070fc000c..25e64b2c5711 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -120,6 +120,7 @@ struct peer *new_peer(struct lightningd *ld, u64 dbid, else peer->their_features = NULL; + peer->unknown_channel_reestablishes = 0; peer->dev_ignore_htlcs = false; peer_node_id_map_add(ld->peers, peer); @@ -1950,6 +1951,9 @@ void handle_peer_connected(struct lightningd *ld, const u8 *msg) * on peer commands, and it knows to ignore if it's wrong. */ peer->connectd_counter = connectd_counter; + /* Fresh connection, fresh spam allowance. */ + peer->unknown_channel_reestablishes = 0; + /* We mark peer in "connecting" state until hooks have passed. */ assert(peer->connected == PEER_DISCONNECTED); peer->connected = PEER_CONNECTING; @@ -2039,6 +2043,11 @@ static void send_reestablish(struct peer *peer, msg))); } +/* How many channel_reestablish for channels we don't know we answer without + * hanging up. An honest peer needs one per stale channel; past that it's + * cheaper for us to make them reconnect (which connectd rate-limits). */ +#define MAX_UNKNOWN_CHANNEL_REESTABLISHES 10 + /* Is this a dual-funded open which has not reached the funding transaction * yet? All of these still own a dualopend, and none of them have a funding * tx we could rely on to bound the peer's efforts. */ @@ -2150,6 +2159,10 @@ void handle_peer_spoke(struct lightningd *ld, const u8 *msg) struct peer_fd *pfd; char *errmsg; bool sent_reestablish = false; + /* Every error we send here hangs up, except the unknown-channel + * reestablish case below, and a reestablish for a channel whose + * siblings are still live. */ + bool hangup = true; if (!fromwire_connectd_peer_spoke(msg, msg, &id, &connectd_counter, &msgtype, &channel_id, &errmsg)) fatal("Connectd gave bad CONNECTD_PEER_SPOKE message %s", @@ -2328,9 +2341,30 @@ void handle_peer_spoke(struct lightningd *ld, const u8 *msg) "Channel is closed and forgotten"); goto send_error; } + /* BOLT #1: + * + * A sending node: + *... + * - when sending `error`: + * - MUST fail the channel(s) referred to by the error message. + */ + /* We don't know the channel, so there's nothing for us to + * fail, and nothing tells us to drop the connection: `error` + * exists so that we don't have to. Staying up matters here, + * because the peer needs this error to forget its own (saved, + * commitment-ready) channel: hanging up races with the error + * delivery, and then it retries on every reconnect (#8822). + * + * A hostile peer could use that to stream reestablishes for + * random channel_ids down a single connection, so we only + * tolerate a few before hanging up like we used to. */ + if (++peer->unknown_channel_reestablishes + <= MAX_UNKNOWN_CHANNEL_REESTABLISHES) + hangup = false; + break; } - /* Weird message? Log and reply with error. */ + /* Unknown channel, or a weird message? Log and reply with error. */ log_peer_unusual(ld->log, &peer->id, "Unknown channel %s for %s", fmt_channel_id(tmpctx, @@ -2354,7 +2388,7 @@ void handle_peer_spoke(struct lightningd *ld, const u8 *msg) log_peer_debug(ld->log, &peer->id, "Telling connectd to send error %s", tal_hex(tmpctx, error)); - /* Get connectd to send error. */ + /* Get connectd to send error, and (usually) close. */ subd_send_msg(ld->connectd, take(towire_connectd_peer_send_msg(NULL, &peer->id, peer->connectd_counter, @@ -2371,10 +2405,11 @@ void handle_peer_spoke(struct lightningd *ld, const u8 *msg) && peer_has_other_live_channel(peer, &channel_id)) return; - subd_send_msg(ld->connectd, - take(towire_connectd_disconnect_peer(NULL, - &peer->id, - peer->connectd_counter))); + if (hangup) + subd_send_msg(ld->connectd, + take(towire_connectd_disconnect_peer(NULL, + &peer->id, + peer->connectd_counter))); return; tell_connectd: diff --git a/lightningd/peer_control.h b/lightningd/peer_control.h index abd8f51ebebc..bd0499239355 100644 --- a/lightningd/peer_control.h +++ b/lightningd/peer_control.h @@ -65,6 +65,11 @@ struct peer { /* If we open a channel our direction will be this */ u8 direction; + /* How many channel_reestablish for channels we don't know have we seen + * on this connection? We answer those with an error and stay + * connected, so we bound it to keep it from being a spam lever. */ + u32 unknown_channel_reestablishes; + /* Swallow incoming HTLCs (for testing) */ bool dev_ignore_htlcs; }; diff --git a/tests/test_misc.py b/tests/test_misc.py index 240683b00f4a..bb660de0cc81 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -3405,17 +3405,23 @@ def test_restorefrompeer(node_factory, bitcoind): l1.start() assert l1.daemon.is_in_log('Server started with public key') - # If this happens fast enough, connect fails with "disconnected - # during connection" - try: - l1.rpc.connect(l2.info['id'], 'localhost', l2.port) - except RpcError as err: - assert "disconnected during connection" in err.error['message'] + l1.rpc.connect(l2.info['id'], 'localhost', l2.port) l1.daemon.wait_for_log('peer_in WIRE_PEER_STORAGE_RETRIEVAL') + # We lost our db, so l2's channel_reestablish is for a channel we don't + # know: we answer with an error, but we don't hang up on them any more. + l1.daemon.wait_for_log('Unknown channel .* for WIRE_CHANNEL_REESTABLISH') + assert only_one(l1.rpc.listpeers()['peers'])['connected'] + assert l1.rpc.restorefrompeer()['stubs'][0] == _['channel_id'] + # We need to reconnect so the stub channel triggers the bogus + # channel_reestablish flow in peer_connected_hook_final. + # (l1 no longer disconnects on unknown channel_reestablish.) + l1.rpc.disconnect(l2.info['id'], force=True) + l1.rpc.connect(l2.info['id'], 'localhost', l2.port) + l1.daemon.wait_for_log('Sending a bogus channel_reestablish message to make the peer unilaterally close the channel.') l1.daemon.wait_for_log('peer_out WIRE_ERROR') From 079795e53388c1a2f4bff5c361eba1673037db97 Mon Sep 17 00:00:00 2001 From: Vincenzo Palazzo Date: Sat, 8 Aug 2026 14:49:33 +0200 Subject: [PATCH 2/2] tests: check we answer an unknown channel_reestablish without hanging up Sets up #8822: l1 drops the last tx_complete after dualopend has decided the commitment is ready, so l1 saves a DUALOPEND_OPEN_COMMIT_READY channel that l2 never saved. On reconnect l1 reestablishes a channel l2 doesn't know, and l2 has to answer with an error and stay connected. Note this pins the new behaviour rather than the loop itself: the loop is a race on l1's side (see the previous commit), and locally l1 wins it and forgets the channel even without the fix, so a strict xfail reproduction would just be flaky. The test does fail without the fix, on l2 hanging up. Signed-off-by: Vincenzo Palazzo --- tests/test_connection.py | 6 ++++- tests/test_opening.py | 49 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/tests/test_connection.py b/tests/test_connection.py index 4b005b12559f..6319941df2dd 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -499,7 +499,11 @@ def test_disconnect_opener(node_factory): '-WIRE_TX_ADD_OUTPUT', '+WIRE_TX_ADD_OUTPUT', '-WIRE_TX_COMPLETE', - '=WIRE_TX_COMPLETE'] + # Close after sending tx_complete. A plain '=' used + # to abort this open only because an unknown + # channel_reestablish made us hang up (#8822); we no + # longer do that, so the open would succeed. + '+WIRE_TX_COMPLETE'] l1 = node_factory.get_node(disconnect=disconnects, may_reconnect=EXPERIMENTAL_DUAL_FUND, diff --git a/tests/test_opening.py b/tests/test_opening.py index 6d4f40a01dd8..bc230591b6b9 100644 --- a/tests/test_opening.py +++ b/tests/test_opening.py @@ -424,6 +424,55 @@ def test_v2_open_sigs_reconnect_1(node_factory, bitcoind): l2.daemon.wait_for_log(r'to CHANNELD_NORMAL') +@unittest.skipIf(TEST_NETWORK != 'regtest', 'elementsd doesnt yet support PSBT features we need') +@pytest.mark.openchannel('v2') +def test_v2_open_reestablish_unknown_channel(node_factory, bitcoind): + """ Reconnect loop from #8822. + + l1 drops the last tx_complete on the floor *after* dualopend has decided + the commitment is ready, so l1 saves the channel in + DUALOPEND_OPEN_COMMIT_READY while l2 is still waiting and throws its + unsaved channel away. On reconnect l1 reestablishes a channel l2 has + never heard of. + + l2 answers with an error and stays connected: that's what lets l1's + dualopend actually read the error and forget the channel. When we hung + up instead, the error raced the disconnect on l1's side, and whenever it + lost that race l1 reestablished again on every reconnect. + """ + l1, l2 = node_factory.get_nodes(2, + opts=[{'disconnect': ['-WIRE_TX_COMPLETE'], + 'may_reconnect': True, + 'dev-no-reconnect': None}, + {'may_reconnect': True, + 'dev-no-reconnect': None}]) + + l1.rpc.connect(l2.info['id'], 'localhost', l2.port) + bitcoind.rpc.sendtoaddress(l1.rpc.newaddr()['p2tr'], (2**24) / 10**8 + 0.01) + bitcoind.generate_block(1) + wait_for(lambda: len(l1.rpc.listfunds()['outputs']) > 0) + + with pytest.raises(RpcError): + l1.rpc.fundchannel(l2.info['id'], 100000) + + # We saved it, they didn't. + wait_for(lambda: [c['state'] for c in l1.rpc.listpeerchannels()['channels']] + == ['DUALOPEND_OPEN_COMMIT_READY']) + wait_for(lambda: l2.rpc.listpeerchannels()['channels'] == []) + + # One reconnect has to be enough: l2 tells us it doesn't know the channel + # and we forget it. + l1.rpc.connect(l2.info['id'], 'localhost', l2.port) + l2.daemon.wait_for_log('Unknown channel .* for WIRE_CHANNEL_REESTABLISH') + l1.daemon.wait_for_log('peer_in WIRE_ERROR') + wait_for(lambda: l1.rpc.listpeerchannels()['channels'] == []) + + # Neither side hung up over it: that's the whole point, an error we send + # after hanging up may never be read. + assert only_one(l1.rpc.listpeers()['peers'])['connected'] + assert only_one(l2.rpc.listpeers()['peers'])['connected'] + + @unittest.skipIf(TEST_NETWORK != 'regtest', 'elementsd doesnt yet support PSBT features we need') @pytest.mark.openchannel('v2') def test_v2_open_sigs_out_of_order(node_factory, bitcoind):