From 3fe1bae8d84ea0530012b8ac279c4f319b8c8cd5 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 29 Apr 2025 09:39:20 +0930 Subject: [PATCH 1/8] wallet: fix erroneous allocation of db_col_optional_scid. We are supposed to allocate of the ctx we're passed, not tmpctx. Doesn't matter for now, because we don't use this result with anything which outlives tmpctx, but we're going to: ``` ==47574==ERROR: AddressSanitizer: heap-use-after-free on address 0x6040005a8f38 at pc 0x55d3c584d252 bp 0x7ffddfb1b090 sp 0x7ffddfb1b088 READ of size 8 at 0x6040005a8f38 thread T0 #0 0x55d3c584d251 in json_add_closed_channel /home/runner/work/lightning/lightning/lightningd/closed_channel.c:27:3 #1 0x55d3c584ca5a in json_listclosedchannels /home/runner/work/lightning/lightning/lightningd/closed_channel.c:118:3 #2 0x55d3c58c0cbe in command_exec /home/runner/work/lightning/lightning/lightningd/jsonrpc.c:808:8 ``` Signed-off-by: Rusty Russell --- wallet/wallet.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wallet/wallet.c b/wallet/wallet.c index 63dba3b5f9ef..9b83dbdb5f89 100644 --- a/wallet/wallet.c +++ b/wallet/wallet.c @@ -1672,7 +1672,7 @@ static struct short_channel_id *db_col_optional_scid(const tal_t *ctx, if (db_col_is_null(stmt, colname)) return NULL; - scid = tal(tmpctx, struct short_channel_id); + scid = tal(ctx, struct short_channel_id); *scid = db_col_short_channel_id(stmt, colname); return scid; } From e7296fcf919da4919b61784f8edf7d3b586f250d Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 29 Apr 2025 09:54:33 +0930 Subject: [PATCH 2/8] lightningd: delete all trace of nonexistent channels. We're going to start loading them into memory for nicer responses if people try to reestablish closed channels, but we don't care about ones which were never actually opened. We could add a new state, but easier to simply remove them. Signed-off-by: Rusty Russell --- lightningd/channel.c | 14 ++++-- lightningd/channel.h | 3 +- lightningd/channel_control.c | 6 +-- lightningd/dual_open_control.c | 8 ++-- lightningd/onchain_control.c | 2 +- lightningd/peer_control.c | 4 +- lightningd/test/run-invoice-select-inchan.c | 2 +- wallet/test/run-db.c | 3 +- wallet/test/run-wallet.c | 8 ++-- wallet/wallet.c | 52 ++++++++++++++++++--- wallet/wallet.h | 5 ++ 11 files changed, 80 insertions(+), 27 deletions(-) diff --git a/lightningd/channel.c b/lightningd/channel.c index b971ebe8eabf..90fede3cd4ca 100644 --- a/lightningd/channel.c +++ b/lightningd/channel.c @@ -88,13 +88,17 @@ static void destroy_channel(struct channel *channel) list_del_from(&channel->peer->channels, &channel->list); } -void delete_channel(struct channel *channel STEALS) +void delete_channel(struct channel *channel STEALS, bool completely_eliminate) { const u8 *msg; struct peer *peer = channel->peer; - if (channel->dbid != 0) + if (channel->dbid != 0) { wallet_channel_close(channel->peer->ld->wallet, channel); + /* Never open at all, not ours. */ + if (completely_eliminate) + wallet_channel_delete(channel->peer->ld->wallet, channel); + } /* Tell the hsm to forget the channel, needs to be after it's * been forgotten here */ @@ -1035,7 +1039,7 @@ static void channel_fail_perm(struct channel *channel, drop_to_chain(ld, channel, false, spent_by); if (channel_state_open_uncommitted(channel->state)) - delete_channel(channel); + delete_channel(channel, false); } void channel_fail_permanent(struct channel *channel, @@ -1093,7 +1097,7 @@ void channel_fail_forget(struct channel *channel, const char *fmt, ...) channel->error = towire_errorfmt(channel, &channel->cid, "%s", why); - delete_channel(channel); + delete_channel(channel, false); tal_free(why); } @@ -1162,7 +1166,7 @@ void channel_internal_error(struct channel *channel, const char *fmt, ...) /* Nothing ventured, nothing lost! */ if (channel_state_uncommitted(channel->state)) { channel_set_owner(channel, NULL); - delete_channel(channel); + delete_channel(channel, false); return; } diff --git a/lightningd/channel.h b/lightningd/channel.h index 51b6353c3fa7..5a802ce6b038 100644 --- a/lightningd/channel.h +++ b/lightningd/channel.h @@ -479,7 +479,8 @@ channel_current_inflight(const struct channel *channel); /* What's the last feerate used for a funding tx on this channel? */ u32 channel_last_funding_feerate(const struct channel *channel); -void delete_channel(struct channel *channel STEALS); +/* Only set completely_eliminate for never-existed channels */ +void delete_channel(struct channel *channel STEALS, bool completely_eliminate); const char *channel_state_name(const struct channel *channel); const char *channel_state_str(enum channel_state state); diff --git a/lightningd/channel_control.c b/lightningd/channel_control.c index 64d52cc16079..f1d3d64777f6 100644 --- a/lightningd/channel_control.c +++ b/lightningd/channel_control.c @@ -1321,7 +1321,7 @@ static void forget(struct channel *channel) channel->forgets = tal_arr(channel, struct command *, 0); /* Forget the channel. */ - delete_channel(channel); + delete_channel(channel, false); for (size_t i = 0; i < tal_count(forgets); i++) { assert(!forgets[i]->json_stream); @@ -2017,8 +2017,8 @@ void channel_notify_new_block(struct lightningd *ld, block_height - channel->first_blocknum, fmt_bitcoin_txid(tmpctx, &channel->funding.txid)); /* FIXME: Send an error packet for this case! */ - /* And forget it. */ - delete_channel(channel); + /* And forget it. COMPLETELY. */ + delete_channel(channel, true); } tal_free(to_forget); diff --git a/lightningd/dual_open_control.c b/lightningd/dual_open_control.c index c1863d2ee158..925ab1fe1db8 100644 --- a/lightningd/dual_open_control.c +++ b/lightningd/dual_open_control.c @@ -67,7 +67,7 @@ void channel_unsaved_close_conn(struct channel *channel, const char *why) assert(channel->owner); channel_set_owner(channel, NULL); - delete_channel(channel); + delete_channel(channel, false); } static void channel_saved_err_broken_reconn(struct channel *channel, @@ -3959,13 +3959,13 @@ static void dualopen_errmsg(struct channel *channel, if (channel_state_uncommitted(channel->state)) { log_info(channel->log, "%s", "Unsaved peer failed." " Deleting channel."); - delete_channel(channel); + delete_channel(channel, false); return; } if ((warning || disconnect) && channel_state_open_uncommitted(channel->state)) { log_info(channel->log, "%s", "Commit ready peer failed." " Deleting channel."); - delete_channel(channel); + delete_channel(channel, false); return; } @@ -4009,7 +4009,7 @@ static void dualopen_errmsg(struct channel *channel, if (channel_state_open_uncommitted(channel->state)) { log_info(channel->log, "%s", "Commit ready peer can't reconnect." " Deleting channel."); - delete_channel(channel); + delete_channel(channel, false); return; } char *err = restart_dualopend(tmpctx, diff --git a/lightningd/onchain_control.c b/lightningd/onchain_control.c index cb09b1ee4c0b..3ea834ee0d64 100644 --- a/lightningd/onchain_control.c +++ b/lightningd/onchain_control.c @@ -528,7 +528,7 @@ static void handle_irrevocably_resolved(struct channel *channel, const u8 *msg U log_info(channel->log, "onchaind complete, forgetting peer"); /* This will also free onchaind. */ - delete_channel(channel); + delete_channel(channel, false); } diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index 38fec696135b..e9092d74a304 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -519,7 +519,7 @@ void channel_errmsg(struct channel *channel, if (channel_state_uncommitted(channel->state)) { log_info(channel->log, "%s", "Unsaved peer failed." " Deleting channel."); - delete_channel(channel); + delete_channel(channel, false); return; } @@ -3432,7 +3432,7 @@ static void process_dev_forget_channel(struct bitcoind *bitcoind UNUSED, forget->channel->error = towire_errorfmt(forget->channel, &forget->channel->cid, "dev_forget_channel"); - delete_channel(forget->channel); + delete_channel(forget->channel, false); was_pending(command_success(forget->cmd, response)); } diff --git a/lightningd/test/run-invoice-select-inchan.c b/lightningd/test/run-invoice-select-inchan.c index ca8d78e48f09..188b6a5c47fb 100644 --- a/lightningd/test/run-invoice-select-inchan.c +++ b/lightningd/test/run-invoice-select-inchan.c @@ -243,7 +243,7 @@ struct anchor_details *create_anchor_details(const tal_t *ctx UNNEEDED, const struct bitcoin_tx *tx UNNEEDED) { fprintf(stderr, "create_anchor_details called!\n"); abort(); } /* Generated stub for delete_channel */ -void delete_channel(struct channel *channel STEALS UNNEEDED) +void delete_channel(struct channel *channel STEALS UNNEEDED, bool completely_eliminate UNNEEDED) { fprintf(stderr, "delete_channel called!\n"); abort(); } /* Generated stub for depthcb_update_scid */ bool depthcb_update_scid(struct channel *channel UNNEEDED, diff --git a/wallet/test/run-db.c b/wallet/test/run-db.c index c68a5f31426f..492527cdf65c 100644 --- a/wallet/test/run-db.c +++ b/wallet/test/run-db.c @@ -285,7 +285,8 @@ struct peer *new_peer(struct lightningd *ld UNNEEDED, u64 dbid UNNEEDED, bool connected_incoming UNNEEDED) { fprintf(stderr, "new_peer called!\n"); abort(); } /* Generated stub for notify_chain_mvt */ -void notify_chain_mvt(struct lightningd *ld UNNEEDED, const struct chain_coin_mvt *mvt UNNEEDED) +void notify_chain_mvt(struct lightningd *ld UNNEEDED, + const struct chain_coin_mvt *chain_mvt UNNEEDED) { fprintf(stderr, "notify_chain_mvt called!\n"); abort(); } /* Generated stub for notify_forward_event */ void notify_forward_event(struct lightningd *ld UNNEEDED, diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c index de5787ed3d71..590cb1956bd0 100644 --- a/wallet/test/run-wallet.c +++ b/wallet/test/run-wallet.c @@ -701,10 +701,12 @@ struct uncommitted_channel *new_uncommitted_channel(struct peer *peer UNNEEDED) bool node_announcement_same(const u8 *nann1 UNNEEDED, const u8 *nann2 UNNEEDED) { fprintf(stderr, "node_announcement_same called!\n"); abort(); } /* Generated stub for notify_chain_mvt */ -void notify_chain_mvt(struct lightningd *ld UNNEEDED, const struct chain_coin_mvt *mvt UNNEEDED) +void notify_chain_mvt(struct lightningd *ld UNNEEDED, + const struct chain_coin_mvt *chain_mvt UNNEEDED) { fprintf(stderr, "notify_chain_mvt called!\n"); abort(); } /* Generated stub for notify_channel_mvt */ -void notify_channel_mvt(struct lightningd *ld UNNEEDED, const struct channel_coin_mvt *mvt UNNEEDED) +void notify_channel_mvt(struct lightningd *ld UNNEEDED, + const struct channel_coin_mvt *chan_mvt UNNEEDED) { fprintf(stderr, "notify_channel_mvt called!\n"); abort(); } /* Generated stub for notify_channel_open_failed */ void notify_channel_open_failed(struct lightningd *ld UNNEEDED, @@ -2116,7 +2118,7 @@ static bool test_channel_inflight_crud(struct lightningd *ld, const tal_t *ctx) /* do inflights get cleared when the channel is closed?*/ dbid = chan->dbid; - delete_channel(chan); /* Also clears up peer! */ + delete_channel(chan, false); /* Also clears up peer! */ CHECK_MSG(count_inflights(w, dbid) == 0, "inflights cleaned up"); db_commit_transaction(w->db); CHECK_MSG(!wallet_err, wallet_err); diff --git a/wallet/wallet.c b/wallet/wallet.c index 9b83dbdb5f89..b8b2f5d64e6e 100644 --- a/wallet/wallet.c +++ b/wallet/wallet.c @@ -2814,12 +2814,8 @@ void wallet_channel_insert(struct wallet *w, struct channel *chan) void wallet_channel_close(struct wallet *w, const struct channel *chan) { - /* We keep a couple of dependent tables around as well, such as the - * channel_configs table, since that might help us debug some issues, - * and it is rather limited in size. Tables that can grow quite - * considerably and that are of limited use after channel closure will - * be pruned as well. */ - + /* We keep the entry in the channel_configs table, since that might + * help us debug some issues, and it is rather limited in size. */ struct db_stmt *stmt; /* Delete entries from `channel_htlcs` */ @@ -2861,6 +2857,24 @@ void wallet_channel_close(struct wallet *w, db_bind_u64(stmt, chan->dbid); db_exec_prepared_v2(take(stmt)); + /* Delete transaction annotations */ + stmt = db_prepare_v2(w->db, SQL("DELETE FROM transaction_annotations " + "WHERE channel=?")); + db_bind_u64(stmt, chan->dbid); + db_exec_prepared_v2(take(stmt)); + + /* Delete feerates */ + stmt = db_prepare_v2(w->db, SQL("DELETE FROM channel_feerates " + "WHERE channel_id=?")); + db_bind_u64(stmt, chan->dbid); + db_exec_prepared_v2(take(stmt)); + + /* Delete anchor information */ + stmt = db_prepare_v2(w->db, SQL("DELETE FROM local_anchors " + "WHERE channel_id=?")); + db_bind_u64(stmt, chan->dbid); + db_exec_prepared_v2(take(stmt)); + /* Set the channel to closed */ stmt = db_prepare_v2(w->db, SQL("UPDATE channels " "SET state=? " @@ -2870,6 +2884,32 @@ void wallet_channel_close(struct wallet *w, db_exec_prepared_v2(take(stmt)); } +/* Completely unused channels get wiped entirely (we've already closed it above) */ +void wallet_channel_delete(struct wallet *w, const struct channel *channel) +{ + struct db_stmt *stmt; + + /* Delete channel configuration for both sides */ + stmt = db_prepare_v2(w->db, SQL("DELETE FROM channel_configs" + " WHERE id=? OR id=?")); + db_bind_u64(stmt, channel->channel_info.their_config.id); + db_bind_u64(stmt, channel->our_config.id); + db_exec_prepared_v2(stmt); + + assert(db_count_changes(stmt) == 2); + tal_free(stmt); + + stmt = db_prepare_v2(w->db, SQL("DELETE FROM channels" + " WHERE state = ?" + " AND id=?")); + db_bind_u64(stmt, channel_state_in_db(CLOSED)); + db_bind_u64(stmt, channel->dbid); + db_exec_prepared_v2(stmt); + + assert(db_count_changes(stmt) == 1); + tal_free(stmt); +} + void wallet_channel_inflight_cleanup_incomplete(struct wallet *w, u64 wallet_id) { struct db_stmt *stmt; diff --git a/wallet/wallet.h b/wallet/wallet.h index ea085352813e..b2e9430e0895 100644 --- a/wallet/wallet.h +++ b/wallet/wallet.h @@ -699,6 +699,11 @@ void wallet_channel_clear_inflights(struct wallet *w, void wallet_channel_close(struct wallet *w, const struct channel *chan); +/** + * If it was never used, we can forget it entirely after wallet_channel_close. + */ +void wallet_channel_delete(struct wallet *w, const struct channel *channel); + /** * Adds a channel state change history entry into the database */ From 71652a2a02c75b7728e911a5e7a5cd5ef99f367f Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 29 Apr 2025 09:55:04 +0930 Subject: [PATCH 3/8] lightningd: neaten delete_channel. Use convenience variables. Signed-off-by: Rusty Russell --- lightningd/channel.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/lightningd/channel.c b/lightningd/channel.c index 90fede3cd4ca..24fd5ff9c168 100644 --- a/lightningd/channel.c +++ b/lightningd/channel.c @@ -91,20 +91,21 @@ static void destroy_channel(struct channel *channel) void delete_channel(struct channel *channel STEALS, bool completely_eliminate) { const u8 *msg; - struct peer *peer = channel->peer; + struct lightningd *ld = peer->ld; + if (channel->dbid != 0) { - wallet_channel_close(channel->peer->ld->wallet, channel); + wallet_channel_close(ld->wallet, channel); /* Never open at all, not ours. */ if (completely_eliminate) - wallet_channel_delete(channel->peer->ld->wallet, channel); + wallet_channel_delete(ld->wallet, channel); } /* Tell the hsm to forget the channel, needs to be after it's * been forgotten here */ - if (hsm_capable(channel->peer->ld, WIRE_HSMD_FORGET_CHANNEL)) { - msg = towire_hsmd_forget_channel(NULL, &channel->peer->id, channel->dbid); - msg = hsm_sync_req(tmpctx, channel->peer->ld, take(msg)); + if (hsm_capable(ld, WIRE_HSMD_FORGET_CHANNEL)) { + msg = towire_hsmd_forget_channel(NULL, &peer->id, channel->dbid); + msg = hsm_sync_req(tmpctx, ld, take(msg)); if (!fromwire_hsmd_forget_channel_reply(msg)) fatal("HSM gave bad hsm_forget_channel_reply %s", tal_hex(msg, msg)); } From 1903d748bdf9faeb31740beace29caad8223e6e8 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 29 Apr 2025 09:55:08 +0930 Subject: [PATCH 4/8] lightningd: allow up to 100 "slow open" channels before forgetting them. Michael of Boltz told me this long ago, that when fees spiked some of their clients' opens got stuck. After two weeks their node forgot them, and when fees came back down the opens still failed. Unfortunately, I can't see an issue for this! We can give some grace: we don't want to waste too many resources, but 100 channels is nothing. The test needs to be adjusted to have *two* slow open channels, now. Changelog-Changed: Protocol: we won't forget still-opening channels after 2016 blocks, unless there are more than 100. Signed-off-by: Rusty Russell --- lightningd/channel_control.c | 62 +++++++++++++++++++++++------------- tests/test_connection.py | 52 +++++++++++++++++++----------- tests/test_opening.py | 23 +++++++++---- 3 files changed, 90 insertions(+), 47 deletions(-) diff --git a/lightningd/channel_control.c b/lightningd/channel_control.c index f1d3d64777f6..bcfe93af9c6f 100644 --- a/lightningd/channel_control.c +++ b/lightningd/channel_control.c @@ -1,4 +1,5 @@ #include "config.h" +#include #include #include #include @@ -1927,18 +1928,8 @@ is_fundee_should_forget(struct lightningd *ld, struct channel *channel, u32 block_height) { - /* BOLT #2: - * - * A non-funding node (fundee): - * - SHOULD forget the channel if it does not see the - * correct funding transaction after a timeout of 2016 blocks. - */ - u32 max_funding_unconfirmed; - - if (ld->developer) - max_funding_unconfirmed = ld->dev_max_funding_unconfirmed; - else - max_funding_unconfirmed = 2016; + /* 2016 by default */ + u32 max_funding_unconfirmed = ld->dev_max_funding_unconfirmed; /* Only applies if we are fundee. */ if (channel->opener == LOCAL) @@ -1969,16 +1960,42 @@ is_fundee_should_forget(struct lightningd *ld, return true; } +/* We want in ascending order, so oldest channels first */ +static int cmp_channel_start(struct channel *const *a, struct channel *const *b, void *unused) +{ + if ((*a)->first_blocknum < (*b)->first_blocknum) + return -1; + else if ((*a)->first_blocknum > (*b)->first_blocknum) + return 1; + return 0; +} + /* Notify all channels of new blocks. */ void channel_notify_new_block(struct lightningd *ld, u32 block_height) { struct peer *peer; struct channel *channel; - struct channel **to_forget = tal_arr(NULL, struct channel *, 0); + struct channel **to_forget = tal_arr(tmpctx, struct channel *, 0); size_t i; struct peer_node_id_map_iter it; + /* BOLT #2: + * + * A non-funding node (fundee): + * - SHOULD forget the channel if it does not see the + * correct funding transaction after a timeout of 2016 blocks. + */ + + /* But we give some latitude! Boltz reported that after a months-long + * fee spike, they had some failed opens when the tx finally got mined. + * They're individually cheap, keep the latest 100. */ + size_t forgettable_channels_to_keep = 100; + + /* For testing */ + if (ld->dev_max_funding_unconfirmed != 2016) + forgettable_channels_to_keep = 1; + /* FIXME: keep separate block-aware channel structure instead? */ for (peer = peer_node_id_map_first(ld->peers, &it); peer; @@ -1986,13 +2003,12 @@ void channel_notify_new_block(struct lightningd *ld, list_for_each(&peer->channels, channel, list) { if (channel_state_uncommitted(channel->state)) continue; - if (is_fundee_should_forget(ld, channel, block_height)) { + if (is_fundee_should_forget(ld, channel, block_height)) tal_arr_expand(&to_forget, channel); - } else - /* Let channels know about new blocks, - * required for lease updates */ - try_update_blockheight(ld, channel, - block_height); + + /* Let channels know about new blocks, + * required for lease updates */ + try_update_blockheight(ld, channel, block_height); } } @@ -2004,7 +2020,11 @@ void channel_notify_new_block(struct lightningd *ld, * just the freeing of the channel that occurs, but the * potential destruction of the peer that invalidates * memory the inner loop is accessing. */ - for (i = 0; i < tal_count(to_forget); ++i) { + if (tal_count(to_forget) < forgettable_channels_to_keep) + return; + + asort(to_forget, tal_count(to_forget), cmp_channel_start, NULL); + for (i = 0; i + forgettable_channels_to_keep < tal_count(to_forget); ++i) { channel = to_forget[i]; /* Report it first. */ log_unusual(channel->log, @@ -2020,8 +2040,6 @@ void channel_notify_new_block(struct lightningd *ld, /* And forget it. COMPLETELY. */ delete_channel(channel, true); } - - tal_free(to_forget); } /* Since this could vanish while we're checking with bitcoind, we need to save diff --git a/tests/test_connection.py b/tests/test_connection.py index 5d8e150f11af..d52d12bc35b8 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -2791,19 +2791,19 @@ def test_fundee_forget_funding_tx_unconfirmed(node_factory, bitcoind): """Test that fundee will forget the channel if the funding tx has been unconfirmed for too long. """ - # Keep this low (default is 2016), since everything + # Keep confirms low (default is 2016), since everything # is much slower in VALGRIND mode and wait_for_log # could time out before lightningd processes all the - # blocks. - blocks = 50 - # opener - l1 = node_factory.get_node() - # peer - l2 = node_factory.get_node(options={"dev-max-funding-unconfirmed-blocks": blocks}) - l1.rpc.connect(l2.info['id'], 'localhost', l2.port) + # blocks. This also reduces the number of "slow confirm" peers + # from 100, to 1. + blocks = 10 + l1, l2, l3 = node_factory.line_graph(3, fundchannel=False, + opts={"dev-max-funding-unconfirmed-blocks": blocks}) - # Give opener some funds. + # Give openers some funds. l1.fundwallet(10**7) + l3.fundwallet(10**7) + sync_blockheight(bitcoind, [l1, l2, l3]) def mock_sendrawtransaction(r): return {'id': r['id'], 'error': {'code': 100, 'message': 'sendrawtransaction disabled'}} @@ -2813,23 +2813,37 @@ def mock_donothing(r): # Prevent opener from broadcasting funding tx (any tx really). l1.daemon.rpcproxy.mock_rpc('sendrawtransaction', mock_sendrawtransaction) + # (for EXPERIMENTAL_DUAL_FUND=1, have to prevent l2 doing it too!) l2.daemon.rpcproxy.mock_rpc('sendrawtransaction', mock_donothing) - # Fund the channel. - # The process will complete, but opener will be unable - # to broadcast and confirm funding tx. + # l1 tries to open, fails. with pytest.raises(RpcError, match=r'sendrawtransaction disabled'): l1.rpc.fundchannel(l2.info['id'], 10**6) - # Generate blocks until unconfirmed. - bitcoind.generate_block(blocks) + # One block later, l3 tries, fails silently. + bitcoind.generate_block(1) + sync_blockheight(bitcoind, [l1, l2, l3]) + + l3.daemon.rpcproxy.mock_rpc('sendrawtransaction', mock_donothing) + l3.rpc.fundchannel(l2.info['id'], 10**6) + + # After 10 blocks, l1's is due to be forgotten, but doesn't since we let 1 linger. + bitcoind.generate_block(blocks - 1) + assert not l2.daemon.is_in_log(r'Forgetting channel') - # fundee will forget channel! - # (Note that we let the last number be anything (hence the {}\d) - l2.daemon.wait_for_log(r'Forgetting channel: It has been {}\d blocks'.format(str(blocks)[:-1])) + if EXPERIMENTAL_DUAL_FUND: + state = 'DUALOPEND_AWAITING_LOCKIN' + else: + state = 'CHANNELD_AWAITING_LOCKIN' + assert [c['state'] for c in l2.rpc.listpeerchannels()['channels']] == [state] * 2 + + # But once l3 is also delayed, l1 gets kicked out. + bitcoind.generate_block(1) - # fundee will also forget, but not disconnect from peer. - wait_for(lambda: l2.rpc.listpeerchannels(l1.info['id'])['channels'] == []) + # fundee will forget oldest channel: the one with l1! + l2.daemon.wait_for_log(rf'Forgetting channel: It has been {blocks + 1} blocks') + assert [c['state'] for c in l2.rpc.listpeerchannels()['channels']] == [state] + assert l2.rpc.listpeerchannels(l1.info['id']) == {'channels': []} @pytest.mark.openchannel('v2') diff --git a/tests/test_opening.py b/tests/test_opening.py index 9fdb22a66dd6..0fd52670bec7 100644 --- a/tests/test_opening.py +++ b/tests/test_opening.py @@ -10,6 +10,7 @@ import pytest import re import unittest +import time def find_next_feerate(node, peer): @@ -2697,8 +2698,8 @@ def test_zeroconf_forget(node_factory, bitcoind, dopay: bool): """ blocks = 50 plugin_path = Path(__file__).parent / "plugins" / "zeroconf-selective.py" - l1, l2 = node_factory.get_nodes( - 2, + l1, l2, l3 = node_factory.get_nodes( + 3, opts=[ {}, { @@ -2707,6 +2708,7 @@ def test_zeroconf_forget(node_factory, bitcoind, dopay: bool): "zeroconf-mindepth": "0", "dev-max-funding-unconfirmed-blocks": blocks, }, + {}, ], ) @@ -2732,11 +2734,17 @@ def censoring_sendrawtx(tx): l1.rpc.pay(inv["bolt11"]) wait_for(lambda: only_one(l2.rpc.listpeerchannels()['channels'])['to_us_msat'] == 1) + # We need *another* channel to make it forget the first though! + l3.connect(l2) + l3.fundwallet(10**7) + l3.rpc.fundchannel(l2.info["id"], 10**6, mindepth=0) + bitcoind.generate_block(1) + # Now stop, in order to cause catchup and re-evaluate whether to forget the channel l2.stop() # Now we generate enough blocks to cause l2 to forget the channel. - bitcoind.generate_block(blocks) # > blocks + bitcoind.generate_block(blocks - 1) # > blocks l2.start() sync_blockheight(bitcoind, [l1, l2]) @@ -2746,13 +2754,16 @@ def censoring_sendrawtx(tx): bitcoind.generate_block(1) sync_blockheight(bitcoind, [l1, l2]) + # This may take a moment! + time.sleep(5) + have_forgotten = l2.daemon.is_in_log( - r"Forgetting channel: It has been [0-9]+ blocks without the funding transaction" + r"UNUSUAL {}-chan#1: Forgetting channel: It has been 51 blocks without the funding transaction ".format(l1.info['id']) ) if dopay: assert not have_forgotten - assert len(l2.rpc.listpeerchannels()["channels"]) == 1 + assert set([c['peer_id'] for c in l2.rpc.listpeerchannels()["channels"]]) == set([l1.info['id'], l3.info['id']]) else: assert have_forgotten - assert l2.rpc.listpeerchannels() == {"channels": []} + assert [c['peer_id'] for c in l2.rpc.listpeerchannels()["channels"]] == [l3.info['id']] From d4253fc8ffff116831f62121b1276bca900524ca Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 29 Apr 2025 09:55:08 +0930 Subject: [PATCH 5/8] lightningd: keep closed channels in memory. They're small, and this will allow us to efficiently respond to reestablish on them. Signed-off-by: Rusty Russell --- lightningd/channel.c | 2 ++ lightningd/closed_channel.c | 19 ++++++---- lightningd/closed_channel.h | 15 ++++++++ lightningd/lightningd.c | 6 ++++ lightningd/lightningd.h | 3 ++ lightningd/memdump.c | 2 ++ lightningd/test/run-find_my_abspath.c | 3 ++ wallet/test/run-db.c | 6 ++-- wallet/test/run-wallet.c | 13 ++++--- wallet/wallet.c | 52 +++++++++++++++++++++++---- wallet/wallet.h | 19 ++++++++-- 11 files changed, 118 insertions(+), 22 deletions(-) diff --git a/lightningd/channel.c b/lightningd/channel.c index 24fd5ff9c168..87e3f2d96e56 100644 --- a/lightningd/channel.c +++ b/lightningd/channel.c @@ -99,6 +99,8 @@ void delete_channel(struct channel *channel STEALS, bool completely_eliminate) /* Never open at all, not ours. */ if (completely_eliminate) wallet_channel_delete(ld->wallet, channel); + else + wallet_load_one_closed_channel(ld->wallet, ld->closed_channels, channel->dbid); } /* Tell the hsm to forget the channel, needs to be after it's diff --git a/lightningd/closed_channel.c b/lightningd/closed_channel.c index da69e75279de..dd85f4fc2542 100644 --- a/lightningd/closed_channel.c +++ b/lightningd/closed_channel.c @@ -10,6 +10,11 @@ #include #include +size_t hash_cid(const struct channel_id *cid) +{ + return siphash24(siphash_seed(), cid->id, sizeof(cid->id)); +} + static void json_add_closed_channel(struct json_stream *response, const char *fieldname, const struct closed_channel *channel) @@ -90,7 +95,8 @@ static struct command_result *json_listclosedchannels(struct command *cmd, { struct node_id *peer_id; struct json_stream *response; - struct closed_channel **chans; + struct closed_channel *cc; + struct closed_channel_map_iter it; if (!param(cmd, buffer, params, p_opt("id", param_node_id, &peer_id), @@ -100,15 +106,16 @@ static struct command_result *json_listclosedchannels(struct command *cmd, response = json_stream_success(cmd); json_array_start(response, "closedchannels"); - chans = wallet_load_closed_channels(cmd, cmd->ld->wallet); - for (size_t i = 0; i < tal_count(chans); i++) { + for (cc = closed_channel_map_first(cmd->ld->closed_channels, &it); + cc; + cc = closed_channel_map_next(cmd->ld->closed_channels, &it)) { if (peer_id) { - if (!chans[i]->peer_id) + if (!cc->peer_id) continue; - if (!node_id_eq(chans[i]->peer_id, peer_id)) + if (!node_id_eq(cc->peer_id, peer_id)) continue; } - json_add_closed_channel(response, NULL, chans[i]); + json_add_closed_channel(response, NULL, cc); } json_array_end(response); diff --git a/lightningd/closed_channel.h b/lightningd/closed_channel.h index 13f58d62b4fe..f3643aaac7ad 100644 --- a/lightningd/closed_channel.h +++ b/lightningd/closed_channel.h @@ -30,4 +30,19 @@ struct closed_channel { u64 last_stable_connection; }; +static inline const struct channel_id *keyof_closed_channel(const struct closed_channel *cc) +{ + return &cc->cid; +} + +size_t hash_cid(const struct channel_id *cid); + +static inline bool closed_channel_eq_cid(const struct closed_channel *cc, const struct channel_id *cid) +{ + return channel_id_eq(cid, &cc->cid); +} + +HTABLE_DEFINE_NODUPS_TYPE(struct closed_channel, keyof_closed_channel, hash_cid, closed_channel_eq_cid, + closed_channel_map); + #endif /* LIGHTNING_LIGHTNINGD_CLOSED_CHANNEL_H */ diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index 756cae072c23..49473c1c72e8 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -67,6 +67,7 @@ #include #include #include +#include #include #include #include @@ -212,6 +213,11 @@ static struct lightningd *new_lightningd(const tal_t *ctx) ld->htlc_sets = tal(ld, struct htlc_set_map); htlc_set_map_init(ld->htlc_sets); + /*~ We keep a map of closed channels. Mainly so we can respond to peers + * who talk to us about long-closed channels. */ + ld->closed_channels = tal(ld, struct closed_channel_map); + closed_channel_map_init(ld->closed_channels); + /*~ We have a multi-entry log-book infrastructure: we define a 10MB log * book to hold all the entries (and trims as necessary), and multiple * log objects which each can write into it, each with a unique diff --git a/lightningd/lightningd.h b/lightningd/lightningd.h index 2f562f6c2f41..c1ba87bbfa35 100644 --- a/lightningd/lightningd.h +++ b/lightningd/lightningd.h @@ -280,6 +280,9 @@ struct lightningd { /* Contains the codex32 string used with --recover flag */ char *recover; + /* Any channels which are already closed */ + struct closed_channel_map *closed_channels; + /* 2, unless overridden by --dev-fd-limit-multiplier */ u32 fd_limit_multiplier; diff --git a/lightningd/memdump.c b/lightningd/memdump.c index cea4e86504bb..c999db71a0d0 100644 --- a/lightningd/memdump.c +++ b/lightningd/memdump.c @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -201,6 +202,7 @@ static bool lightningd_check_leaks(struct command *cmd) memleak_scan_htable(memtable, &ld->htlc_sets->raw); memleak_scan_htable(memtable, &ld->peers->raw); memleak_scan_htable(memtable, &ld->peers_by_dbid->raw); + memleak_scan_htable(memtable, &ld->closed_channels->raw); wallet_memleak_scan(memtable, ld->wallet); /* Now delete ld and those which it has pointers to. */ diff --git a/lightningd/test/run-find_my_abspath.c b/lightningd/test/run-find_my_abspath.c index 94ccb892ca93..21804a92f004 100644 --- a/lightningd/test/run-find_my_abspath.c +++ b/lightningd/test/run-find_my_abspath.c @@ -120,6 +120,9 @@ void handle_early_opts(struct lightningd *ld UNNEEDED, int argc UNNEEDED, char * /* Generated stub for handle_opts */ void handle_opts(struct lightningd *ld UNNEEDED) { fprintf(stderr, "handle_opts called!\n"); abort(); } +/* Generated stub for hash_cid */ +size_t hash_cid(const struct channel_id *cid UNNEEDED) +{ fprintf(stderr, "hash_cid called!\n"); abort(); } /* Generated stub for hash_htlc_key */ size_t hash_htlc_key(const struct htlc_key *htlc_key UNNEEDED) { fprintf(stderr, "hash_htlc_key called!\n"); abort(); } diff --git a/wallet/test/run-db.c b/wallet/test/run-db.c index 492527cdf65c..7c18dede5127 100644 --- a/wallet/test/run-db.c +++ b/wallet/test/run-db.c @@ -112,6 +112,9 @@ void get_channel_basepoints(struct lightningd *ld UNNEEDED, struct basepoints *local_basepoints UNNEEDED, struct pubkey *local_funding_pubkey UNNEEDED) { fprintf(stderr, "get_channel_basepoints called!\n"); abort(); } +/* Generated stub for hash_cid */ +size_t hash_cid(const struct channel_id *cid UNNEEDED) +{ fprintf(stderr, "hash_cid called!\n"); abort(); } /* Generated stub for htlc_in_check */ struct htlc_in *htlc_in_check(const struct htlc_in *hin UNNEEDED, const char *abortstr UNNEEDED) { fprintf(stderr, "htlc_in_check called!\n"); abort(); } @@ -285,8 +288,7 @@ struct peer *new_peer(struct lightningd *ld UNNEEDED, u64 dbid UNNEEDED, bool connected_incoming UNNEEDED) { fprintf(stderr, "new_peer called!\n"); abort(); } /* Generated stub for notify_chain_mvt */ -void notify_chain_mvt(struct lightningd *ld UNNEEDED, - const struct chain_coin_mvt *chain_mvt UNNEEDED) +void notify_chain_mvt(struct lightningd *ld UNNEEDED, const struct chain_coin_mvt *mvt UNNEEDED) { fprintf(stderr, "notify_chain_mvt called!\n"); abort(); } /* Generated stub for notify_forward_event */ void notify_forward_event(struct lightningd *ld UNNEEDED, diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c index 590cb1956bd0..507cd14b6d52 100644 --- a/wallet/test/run-wallet.c +++ b/wallet/test/run-wallet.c @@ -380,6 +380,9 @@ u32 get_block_height(const struct chain_topology *topo UNNEEDED) /* Generated stub for get_network_blockheight */ u32 get_network_blockheight(const struct chain_topology *topo UNNEEDED) { fprintf(stderr, "get_network_blockheight called!\n"); abort(); } +/* Generated stub for hash_cid */ +size_t hash_cid(const struct channel_id *cid UNNEEDED) +{ fprintf(stderr, "hash_cid called!\n"); abort(); } /* Generated stub for hsmd_wire_name */ const char *hsmd_wire_name(int e UNNEEDED) { fprintf(stderr, "hsmd_wire_name called!\n"); abort(); } @@ -701,12 +704,10 @@ struct uncommitted_channel *new_uncommitted_channel(struct peer *peer UNNEEDED) bool node_announcement_same(const u8 *nann1 UNNEEDED, const u8 *nann2 UNNEEDED) { fprintf(stderr, "node_announcement_same called!\n"); abort(); } /* Generated stub for notify_chain_mvt */ -void notify_chain_mvt(struct lightningd *ld UNNEEDED, - const struct chain_coin_mvt *chain_mvt UNNEEDED) +void notify_chain_mvt(struct lightningd *ld UNNEEDED, const struct chain_coin_mvt *mvt UNNEEDED) { fprintf(stderr, "notify_chain_mvt called!\n"); abort(); } /* Generated stub for notify_channel_mvt */ -void notify_channel_mvt(struct lightningd *ld UNNEEDED, - const struct channel_coin_mvt *chan_mvt UNNEEDED) +void notify_channel_mvt(struct lightningd *ld UNNEEDED, const struct channel_coin_mvt *mvt UNNEEDED) { fprintf(stderr, "notify_channel_mvt called!\n"); abort(); } /* Generated stub for notify_channel_open_failed */ void notify_channel_open_failed(struct lightningd *ld UNNEEDED, @@ -2118,7 +2119,7 @@ static bool test_channel_inflight_crud(struct lightningd *ld, const tal_t *ctx) /* do inflights get cleared when the channel is closed?*/ dbid = chan->dbid; - delete_channel(chan, false); /* Also clears up peer! */ + delete_channel(chan, true); /* Also clears up peer! */ CHECK_MSG(count_inflights(w, dbid) == 0, "inflights cleaned up"); db_commit_transaction(w->db); CHECK_MSG(!wallet_err, wallet_err); @@ -2364,6 +2365,8 @@ int main(int argc, const char *argv[]) ld->htlcs_out = tal(ld, struct htlc_out_map); htlc_out_map_init(ld->htlcs_out); list_head_init(&ld->wait_commands); + ld->closed_channels = tal(ld, struct closed_channel_map); + closed_channel_map_init(ld->closed_channels); /* We do a runtime test here, so we still check compile! */ if (HAVE_SQLITE3) { diff --git a/wallet/wallet.c b/wallet/wallet.c index b8b2f5d64e6e..e87908d162af 100644 --- a/wallet/wallet.c +++ b/wallet/wallet.c @@ -2069,11 +2069,10 @@ static struct closed_channel *wallet_stmt2closed_channel(const tal_t *ctx, return cc; } -struct closed_channel **wallet_load_closed_channels(const tal_t *ctx, - struct wallet *w) +void wallet_load_closed_channels(struct wallet *w, + struct closed_channel_map *cc_map) { struct db_stmt *stmt; - struct closed_channel **chans = tal_arr(ctx, struct closed_channel *, 0); /* We load all channels */ stmt = db_prepare_v2(w->db, SQL("SELECT " @@ -2107,12 +2106,53 @@ struct closed_channel **wallet_load_closed_channels(const tal_t *ctx, db_query_prepared(stmt); while (db_step(stmt)) { - struct closed_channel *cc = wallet_stmt2closed_channel(chans, + struct closed_channel *cc = wallet_stmt2closed_channel(cc_map, w, stmt); - tal_arr_expand(&chans, cc); + closed_channel_map_add(cc_map, cc); } tal_free(stmt); - return chans; +} + +void wallet_load_one_closed_channel(struct wallet *w, + struct closed_channel_map *cc_map, + u64 wallet_id) +{ + struct db_stmt *stmt; + + stmt = db_prepare_v2(w->db, SQL("SELECT " + " p.node_id" + ", full_channel_id" + ", scid" + ", alias_local" + ", alias_remote" + ", funder" + ", closer" + ", channel_flags" + ", next_index_local" + ", next_index_remote" + ", next_htlc_id" + ", funding_tx_id" + ", funding_tx_outnum" + ", funding_satoshi" + ", push_msatoshi" + ", msatoshi_local" + ", msatoshi_to_us_min" + ", msatoshi_to_us_max" + ", last_tx" + ", channel_type" + ", state_change_reason" + ", lease_commit_sig" + ", last_stable_connection" + " FROM channels" + " LEFT JOIN peers p ON p.id = peer_id" + " WHERE channels.id = ?;")); + db_bind_u64(stmt, wallet_id); + db_query_prepared(stmt); + + db_step(stmt); + closed_channel_map_add(cc_map, + wallet_stmt2closed_channel(cc_map, w, stmt)); + tal_free(stmt); } static void set_max_channel_dbid(struct wallet *w) diff --git a/wallet/wallet.h b/wallet/wallet.h index b2e9430e0895..94d7be7cc672 100644 --- a/wallet/wallet.h +++ b/wallet/wallet.h @@ -21,6 +21,7 @@ struct amount_msat; struct invoices; struct channel; struct channel_inflight; +struct closed_channel_map; struct htlc_in; struct htlc_in_map; struct htlc_out; @@ -733,13 +734,25 @@ bool wallet_init_channels(struct wallet *w); /** * wallet_load_closed_channels -- Loads dead channels. - * @ctx: context to allocate returned array from * @w: wallet to load from + * @cc_map: the map to fill * * These will be all state CLOSED. */ -struct closed_channel **wallet_load_closed_channels(const tal_t *ctx, - struct wallet *w); +void wallet_load_closed_channels(struct wallet *w, + struct closed_channel_map *cc_map); + +/** + * wallet_load_one_closed_channel -- Loads one single (just-closed) channel. + * @w: wallet to load from + * @cc_map: the map to fill + * @wallet_id: the id of the channel. + * + * Must be newly closed via wallet_channel_close(). + */ +void wallet_load_one_closed_channel(struct wallet *w, + struct closed_channel_map *cc_map, + u64 wallet_id); /** * wallet_channel_stats_incr_* - Increase channel statistics. From aa117f7b874667db1a7b30ec4501e52ff04de200 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 29 Apr 2025 09:55:09 +0930 Subject: [PATCH 6/8] channeld: remove never-used "reestablish_only" option. This was always false. peer_start_channeld was called in various places with the argument "NULL" instead of "false", which unfortunately compilers didn't complain about :( Signed-off-by: Rusty Russell --- channeld/channeld.c | 35 ++------------------- channeld/channeld_wire.csv | 1 - lightningd/channel_control.c | 6 ++-- lightningd/channel_control.h | 3 +- lightningd/dual_open_control.c | 2 +- lightningd/opening_control.c | 4 +-- lightningd/peer_control.c | 6 ++-- lightningd/test/run-invoice-select-inchan.c | 3 +- tests/plugins/channeld_fakenet.c | 2 -- wallet/test/run-wallet.c | 3 +- 10 files changed, 12 insertions(+), 53 deletions(-) diff --git a/channeld/channeld.c b/channeld/channeld.c index b9b6adfcc594..5db9df60194f 100644 --- a/channeld/channeld.c +++ b/channeld/channeld.c @@ -4987,8 +4987,7 @@ static u8 *to_bytearr(const tal_t *ctx, } static void peer_reconnect(struct peer *peer, - const struct secret *last_remote_per_commit_secret, - bool reestablish_only) + const struct secret *last_remote_per_commit_secret) { struct channel_id channel_id; /* Note: BOLT #2 uses these names! */ @@ -5146,14 +5145,6 @@ static void peer_reconnect(struct peer *peer, do { clean_tmpctx(); msg = peer_read(tmpctx, peer->pps); - - /* connectd promised us the msg was reestablish? */ - if (reestablish_only) { - if (fromwire_peektype(msg) != WIRE_CHANNEL_REESTABLISH) - status_failed(STATUS_FAIL_INTERNAL_ERROR, - "Expected reestablish, got: %s", - tal_hex(tmpctx, msg)); - } } while (handle_peer_error_or_warning(peer->pps, msg) || capture_premature_msg(&premature_msgs, msg)); @@ -5501,23 +5492,6 @@ static void peer_reconnect(struct peer *peer, set_channel_type(peer->channel, type); } - /* Now stop, we've been polite long enough. */ - if (reestablish_only) { - /* We've reestablished! */ - wire_sync_write(MASTER_FD, - take(towire_channeld_reestablished(NULL))); - - /* If we were successfully closing, we still go to closingd. */ - if (shutdown_complete(peer)) { - send_shutdown_complete(peer); - daemon_shutdown(); - exit(0); - } - peer_failed_err(peer->pps, - &peer->channel_id, - "Channel is already closed"); - } - tal_free(send_tlvs); /* We've reestablished! */ @@ -6103,7 +6077,6 @@ static void init_channel(struct peer *peer) u32 minimum_depth, lease_expiry; struct secret last_remote_per_commit_secret; struct penalty_base *pbases; - bool reestablish_only; struct channel_type *channel_type; assert(!(fcntl(MASTER_FD, F_GETFL) & O_NONBLOCK)); @@ -6159,7 +6132,6 @@ static void init_channel(struct peer *peer) &channel_type, &peer->dev_disable_commit, &pbases, - &reestablish_only, &peer->experimental_upgrade, &peer->splice_state->inflights, &peer->local_alias)) { @@ -6249,10 +6221,7 @@ static void init_channel(struct peer *peer) /* OK, now we can process peer messages. */ if (reconnected) - peer_reconnect(peer, &last_remote_per_commit_secret, - reestablish_only); - else - assert(!reestablish_only); + peer_reconnect(peer, &last_remote_per_commit_secret); /* If we have a messages to send, send them immediately */ if (fwd_msg) diff --git a/channeld/channeld_wire.csv b/channeld/channeld_wire.csv index e9139ef435c0..a05a3acbc4df 100644 --- a/channeld/channeld_wire.csv +++ b/channeld/channeld_wire.csv @@ -72,7 +72,6 @@ msgdata,channeld_init,desired_type,channel_type, msgdata,channeld_init,dev_disable_commit,?u32, msgdata,channeld_init,num_penalty_bases,u32, msgdata,channeld_init,pbases,penalty_base,num_penalty_bases -msgdata,channeld_init,reestablish_only,bool, msgdata,channeld_init,experimental_upgrade,bool, msgdata,channeld_init,num_inflights,u16, msgdata,channeld_init,inflights,inflight,num_inflights diff --git a/lightningd/channel_control.c b/lightningd/channel_control.c index bcfe93af9c6f..2f6c17338594 100644 --- a/lightningd/channel_control.c +++ b/lightningd/channel_control.c @@ -363,7 +363,7 @@ static void handle_splice_abort(struct lightningd *ld, return; } - if (peer_start_channeld(channel, pfd, NULL, false, false)) { + if (peer_start_channeld(channel, pfd, NULL, false)) { subd_send_msg(ld->connectd, take(towire_connectd_peer_connect_subd(NULL, &peer->id, @@ -1641,8 +1641,7 @@ static void channel_control_errmsg(struct channel *channel, bool peer_start_channeld(struct channel *channel, struct peer_fd *peer_fd, const u8 *fwd_msg, - bool reconnected, - bool reestablish_only) + bool reconnected) { u8 *initmsg; int hsmfd; @@ -1865,7 +1864,6 @@ bool peer_start_channeld(struct channel *channel, ? NULL : (u32 *)&ld->dev_disable_commit, pbases, - reestablish_only, ld->experimental_upgrade_protocol, cast_const2(const struct inflight **, inflights), diff --git a/lightningd/channel_control.h b/lightningd/channel_control.h index 199657abe1e0..cb582573bcb2 100644 --- a/lightningd/channel_control.h +++ b/lightningd/channel_control.h @@ -13,8 +13,7 @@ struct peer; bool peer_start_channeld(struct channel *channel, struct peer_fd *peer_fd, const u8 *fwd_msg, - bool reconnected, - bool reestablish_only); + bool reconnected); /* Send message to channeld (if connected) to tell it about depth * c.f. dualopen_tell_depth! */ diff --git a/lightningd/dual_open_control.c b/lightningd/dual_open_control.c index 925ab1fe1db8..aa0b83d33406 100644 --- a/lightningd/dual_open_control.c +++ b/lightningd/dual_open_control.c @@ -1990,7 +1990,7 @@ static void handle_channel_locked(struct subd *dualopend, channel_watch_funding(dualopend->ld, channel); /* FIXME: LND sigs/update_fee msgs? */ - peer_start_channeld(channel, peer_fd, NULL, false, NULL); + peer_start_channeld(channel, peer_fd, NULL, false); return; } diff --git a/lightningd/opening_control.c b/lightningd/opening_control.c index 20461cba0ab7..65c00ae0cb6f 100644 --- a/lightningd/opening_control.c +++ b/lightningd/opening_control.c @@ -455,7 +455,7 @@ static void opening_funder_finished(struct subd *openingd, const u8 *resp, tell_connectd_peer_importance(channel->peer, was_important); /* If this fails, it cleans up */ - if (!peer_start_channeld(channel, peer_fd, NULL, false, NULL)) + if (!peer_start_channeld(channel, peer_fd, NULL, false)) return; funding_success(channel); @@ -562,7 +562,7 @@ static void opening_fundee_finished(struct subd *openingd, wallet_penalty_base_add(ld->wallet, channel->dbid, pbase); /* On to normal operation (frees if it fails!) */ - if (peer_start_channeld(channel, peer_fd, fwd_msg, false, NULL)) + if (peer_start_channeld(channel, peer_fd, fwd_msg, false)) tal_free(uc); return; diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index e9092d74a304..b46db46036ad 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -1362,8 +1362,7 @@ static void connect_activate_subd(struct lightningd *ld, struct channel *channel if (peer_start_channeld(channel, pfd, - NULL, true, - NULL)) { + NULL, true)) { goto tell_connectd; } close(other_fd); @@ -1879,8 +1878,7 @@ void peer_spoke(struct lightningd *ld, const u8 *msg) /* Tell channeld to handle reestablish, then it will call closingd */ if (peer_start_channeld(channel, pfd, - NULL, true, - NULL)) { + NULL, true)) { goto tell_connectd; } error = towire_warningfmt(tmpctx, &channel_id, diff --git a/lightningd/test/run-invoice-select-inchan.c b/lightningd/test/run-invoice-select-inchan.c index 188b6a5c47fb..c01317e100a4 100644 --- a/lightningd/test/run-invoice-select-inchan.c +++ b/lightningd/test/run-invoice-select-inchan.c @@ -904,8 +904,7 @@ bool peer_restart_dualopend(struct peer *peer UNNEEDED, bool peer_start_channeld(struct channel *channel UNNEEDED, struct peer_fd *peer_fd UNNEEDED, const u8 *fwd_msg UNNEEDED, - bool reconnected UNNEEDED, - bool reestablish_only UNNEEDED) + bool reconnected UNNEEDED) { fprintf(stderr, "peer_start_channeld called!\n"); abort(); } /* Generated stub for peer_start_dualopend */ bool peer_start_dualopend(struct peer *peer UNNEEDED, struct peer_fd *peer_fd UNNEEDED, diff --git a/tests/plugins/channeld_fakenet.c b/tests/plugins/channeld_fakenet.c index 2d848a5abdda..8151b875c01f 100644 --- a/tests/plugins/channeld_fakenet.c +++ b/tests/plugins/channeld_fakenet.c @@ -1057,7 +1057,6 @@ static struct channel *handle_init(struct info *info, const u8 *init_msg) u32 minimum_depth, lease_expiry; struct secret last_remote_per_commit_secret; struct penalty_base *pbases; - bool reestablish_only; struct channel_type *channel_type; u32 feerate_min, feerate_max, feerate_penalty; struct pubkey remote_per_commit; @@ -1137,7 +1136,6 @@ static struct channel *handle_init(struct info *info, const u8 *init_msg) &channel_type, &dev_disable_commit, &pbases, - &reestablish_only, &experimental_upgrade, &inflights, &local_alias)) diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c index 507cd14b6d52..7e03d279e1ff 100644 --- a/wallet/test/run-wallet.c +++ b/wallet/test/run-wallet.c @@ -940,8 +940,7 @@ bool peer_restart_dualopend(struct peer *peer UNNEEDED, bool peer_start_channeld(struct channel *channel UNNEEDED, struct peer_fd *peer_fd UNNEEDED, const u8 *fwd_msg UNNEEDED, - bool reconnected UNNEEDED, - bool reestablish_only UNNEEDED) + bool reconnected UNNEEDED) { fprintf(stderr, "peer_start_channeld called!\n"); abort(); } /* Generated stub for peer_start_dualopend */ bool peer_start_dualopend(struct peer *peer UNNEEDED, struct peer_fd *peer_fd UNNEEDED, From 1ac3a4639a1bd4f3ff0a643668f1f98e85b69e4a Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 29 Apr 2025 09:55:56 +0930 Subject: [PATCH 7/8] lightningd: save shachain for closed channels. We'll need this to send reestablish, and it is only small (max 47 sha256 per channel). Signed-off-by: Rusty Russell --- lightningd/closed_channel.h | 2 ++ wallet/wallet.c | 38 +++++++++++++++++++++++++------------ 2 files changed, 28 insertions(+), 12 deletions(-) diff --git a/lightningd/closed_channel.h b/lightningd/closed_channel.h index f3643aaac7ad..fc8382d0f142 100644 --- a/lightningd/closed_channel.h +++ b/lightningd/closed_channel.h @@ -28,6 +28,8 @@ struct closed_channel { enum state_change state_change_cause; bool leased; u64 last_stable_connection; + /* NULL for older closed channels */ + const struct shachain *their_shachain; }; static inline const struct channel_id *keyof_closed_channel(const struct closed_channel *cc) diff --git a/wallet/wallet.c b/wallet/wallet.c index e87908d162af..67a67c6b72aa 100644 --- a/wallet/wallet.c +++ b/wallet/wallet.c @@ -1763,7 +1763,7 @@ static struct channel *wallet_stmt2channel(struct wallet *w, struct db_stmt *stm alias[LOCAL] = db_col_optional_scid(tmpctx, stmt, "alias_local"); alias[REMOTE] = db_col_optional_scid(tmpctx, stmt, "alias_remote"); - ok &= wallet_shachain_load(w, db_col_u64(stmt, "shachain_remote_id"), + ok &= wallet_shachain_load(w, db_col_u64(stmt, "shachain_remote_id"), &wshachain); remote_shutdown_scriptpubkey = db_col_arr(tmpctx, stmt, @@ -2033,6 +2033,7 @@ static struct closed_channel *wallet_stmt2closed_channel(const tal_t *ctx, struct db_stmt *stmt) { struct closed_channel *cc = tal(ctx, struct closed_channel); + struct wallet_shachain wshachain; /* Can be missing in older dbs! */ cc->peer_id = db_col_optional(cc, stmt, "p.node_id", node_id); @@ -2065,6 +2066,11 @@ static struct closed_channel *wallet_stmt2closed_channel(const tal_t *ctx, cc->state_change_cause = state_change_in_db(db_col_int(stmt, "state_change_reason")); cc->leased = !db_col_is_null(stmt, "lease_commit_sig"); + /* This was deleted on channel closure for older dbs! */ + if (wallet_shachain_load(w, db_col_u64(stmt, "shachain_remote_id"), &wshachain)) + cc->their_shachain = tal_dup(cc, struct shachain, &wshachain.chain); + else + cc->their_shachain = NULL; return cc; } @@ -2099,6 +2105,7 @@ void wallet_load_closed_channels(struct wallet *w, ", state_change_reason" ", lease_commit_sig" ", last_stable_connection" + ", shachain_remote_id" " FROM channels" " LEFT JOIN peers p ON p.id = peer_id" " WHERE state = ?;")); @@ -2143,6 +2150,7 @@ void wallet_load_one_closed_channel(struct wallet *w, ", state_change_reason" ", lease_commit_sig" ", last_stable_connection" + ", shachain_remote_id" " FROM channels" " LEFT JOIN peers p ON p.id = peer_id" " WHERE channels.id = ?;")); @@ -2855,7 +2863,10 @@ void wallet_channel_close(struct wallet *w, const struct channel *chan) { /* We keep the entry in the channel_configs table, since that might - * help us debug some issues, and it is rather limited in size. */ + * help us debug some issues, and it is rather limited in size. We + * also keep shachains: it's limited and we can use it for sending + * reestablish messages with enough information for nodes with lost + * dbs to recover. */ struct db_stmt *stmt; /* Delete entries from `channel_htlcs` */ @@ -2887,16 +2898,6 @@ void wallet_channel_close(struct wallet *w, db_bind_u64(stmt, chan->dbid); db_exec_prepared_v2(take(stmt)); - /* Delete shachains */ - stmt = db_prepare_v2(w->db, SQL("DELETE FROM shachains " - "WHERE id IN (" - " SELECT shachain_remote_id " - " FROM channels " - " WHERE channels.id=?" - ")")); - db_bind_u64(stmt, chan->dbid); - db_exec_prepared_v2(take(stmt)); - /* Delete transaction annotations */ stmt = db_prepare_v2(w->db, SQL("DELETE FROM transaction_annotations " "WHERE channel=?")); @@ -2939,6 +2940,19 @@ void wallet_channel_delete(struct wallet *w, const struct channel *channel) assert(db_count_changes(stmt) == 2); tal_free(stmt); + /* Delete shachains */ + stmt = db_prepare_v2(w->db, SQL("DELETE FROM shachains " + "WHERE id IN (" + " SELECT shachain_remote_id " + " FROM channels " + " WHERE channels.id=?" + ")")); + db_bind_u64(stmt, channel->dbid); + db_exec_prepared_v2(stmt); + + assert(db_count_changes(stmt) == 1); + tal_free(stmt); + stmt = db_prepare_v2(w->db, SQL("DELETE FROM channels" " WHERE state = ?" " AND id=?")); From ca8a10d079fe11073eac3904c920ebc5b11710bb Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 29 Apr 2025 09:55:59 +0930 Subject: [PATCH 8/8] lightningd: respond with channel_reestablish if contacted about long-closed channels. This may be useful for their recovery, though they should see the spend onchain. Signed-off-by: Rusty Russell Changelog-Added: Protocol: We now reply to `channel_reestablish` even on long-closed channels. --- lightningd/peer_control.c | 61 ++++++++++++++++----- lightningd/test/run-invoice-select-inchan.c | 3 + tests/test_closing.py | 34 ++++++++++++ tests/test_connection.py | 2 +- 4 files changed, 85 insertions(+), 15 deletions(-) diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index b46db46036ad..97f50b8b9a20 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -44,6 +44,7 @@ #include #include #include +#include #include #include #include @@ -1797,7 +1798,10 @@ void peer_connected(struct lightningd *ld, const u8 *msg) plugin_hook_call_peer_connected(ld, cmd_id, hook_payload); } -static void send_reestablish(struct lightningd *ld, struct channel *channel) +static void send_reestablish(struct peer *peer, + const struct channel_id *cid, + const struct shachain *their_shachain, + u64 local_next_index) { u8 *msg; struct secret last_remote_per_commit_secret; @@ -1810,30 +1814,42 @@ static void send_reestablish(struct lightningd *ld, struct channel *channel) * - MUST set `your_last_per_commitment_secret` to the last * `per_commitment_secret` it received */ - num_revocations = revocations_received(&channel->their_shachain.chain); + num_revocations = revocations_received(their_shachain); if (num_revocations == 0) memset(&last_remote_per_commit_secret, 0, sizeof(last_remote_per_commit_secret)); - else if (!shachain_get_secret(&channel->their_shachain.chain, + else if (!shachain_get_secret(their_shachain, num_revocations-1, &last_remote_per_commit_secret)) { - channel_fail_permanent(channel, - REASON_LOCAL, - "Could not get revocation secret %"PRIu64, - num_revocations-1); + log_peer_broken(peer->ld->log, &peer->id, + "%s: cannot get shachain secret %"PRIu64" to send reestablish", + fmt_channel_id(tmpctx, cid), num_revocations-1); return; } - msg = towire_channel_reestablish(tmpctx, &channel->cid, - channel->next_index[LOCAL], + /* BOLT #2: + * The sending node: + * - MUST set `next_commitment_number` to the commitment number of the + * next `commitment_signed` it expects to receive. + * - MUST set `next_revocation_number` to the commitment number of the + * next `revoke_and_ack` message it expects to receive. + * - MUST set `my_current_per_commitment_point` to a valid point. + * - if `next_revocation_number` equals 0: + * - MUST set `your_last_per_commitment_secret` to all zeroes + * - otherwise: + * - MUST set `your_last_per_commitment_secret` to the last `per_commitment_secret` it received + */ + msg = towire_channel_reestablish(tmpctx, cid, + local_next_index, num_revocations, &last_remote_per_commit_secret, - &channel->channel_info.remote_per_commit, + /* Any valid point works, since static_remotekey */ + &peer->ld->our_pubkey, /* No upgrade for you, since we're closed! */ NULL); - subd_send_msg(ld->connectd, - take(towire_connectd_peer_send_msg(NULL, &channel->peer->id, - channel->peer->connectd_counter, + subd_send_msg(peer->ld->connectd, + take(towire_connectd_peer_send_msg(NULL, &peer->id, + peer->connectd_counter, msg))); } @@ -1847,6 +1863,7 @@ void peer_spoke(struct lightningd *ld, const u8 *msg) u16 msgtype; u64 connectd_counter; struct channel *channel; + struct closed_channel *closed_channel; struct channel_id channel_id; struct peer *peer; bool dual_fund; @@ -1885,7 +1902,9 @@ void peer_spoke(struct lightningd *ld, const u8 *msg) "Trouble in paradise?"); goto send_error; } - send_reestablish(ld, channel); + send_reestablish(peer, &channel->cid, + &channel->their_shachain.chain, + channel->next_index[LOCAL]); } /* If we have a canned error for this channel, send it now */ @@ -1978,6 +1997,20 @@ void peer_spoke(struct lightningd *ld, const u8 *msg) /* FIXME: Send informative error? */ close(other_fd); return; + + case WIRE_CHANNEL_REESTABLISH: + /* Maybe a previously closed channel? */ + closed_channel = closed_channel_map_get(peer->ld->closed_channels, &channel_id); + if (closed_channel && closed_channel->their_shachain) { + send_reestablish(peer, &closed_channel->cid, + closed_channel->their_shachain, + closed_channel->next_index[LOCAL]); + log_peer_info(ld->log, &peer->id, "Responded to reestablish for long-closed channel %s", + fmt_channel_id(tmpctx, &channel_id)); + error = towire_errorfmt(tmpctx, &channel_id, + "Channel is closed and forgotten"); + goto send_error; + } } /* Weird message? Log and reply with error. */ diff --git a/lightningd/test/run-invoice-select-inchan.c b/lightningd/test/run-invoice-select-inchan.c index c01317e100a4..5dfcd4e2742b 100644 --- a/lightningd/test/run-invoice-select-inchan.c +++ b/lightningd/test/run-invoice-select-inchan.c @@ -362,6 +362,9 @@ u32 get_feerate(const struct fee_states *fee_states UNNEEDED, enum side opener UNNEEDED, enum side side UNNEEDED) { fprintf(stderr, "get_feerate called!\n"); abort(); } +/* Generated stub for hash_cid */ +size_t hash_cid(const struct channel_id *cid UNNEEDED) +{ fprintf(stderr, "hash_cid called!\n"); abort(); } /* Generated stub for hash_htlc_key */ size_t hash_htlc_key(const struct htlc_key *htlc_key UNNEEDED) { fprintf(stderr, "hash_htlc_key called!\n"); abort(); } diff --git a/tests/test_closing.py b/tests/test_closing.py index ac60117e7418..dd2eaaedaedd 100644 --- a/tests/test_closing.py +++ b/tests/test_closing.py @@ -4277,6 +4277,40 @@ def censoring_sendrawtx(r): l1.daemon.wait_for_log(r"Low-priority anchorspend aiming for block {} \(feerate 7500\)".format(height + 12)) +def test_reestablish_closed_channels(node_factory, bitcoind): + """Even long-forgotten channels respond to WIRE_REESTABLISH""" + l1, l2 = node_factory.line_graph(2, opts={'may_reconnect': True, + 'dev-no-reconnect': None}) + + # We block l2 from seeing close, so it will try to reestablish. + def no_new_blocks(req): + return {"error": "go away"} + l2.daemon.rpcproxy.mock_rpc('getblockhash', no_new_blocks) + + # Make a payment, make sure it's entirely finished before we close. + l1.rpc.pay(l2.rpc.invoice(200000000, 'test', 'test')['bolt11']) + wait_for(lambda: only_one(l1.rpc.listpeerchannels()['channels'])['htlcs'] == []) + + # l1 closes, unilaterally. + l1.rpc.disconnect(l2.info['id'], force=True) + l1.rpc.close(l2.info['id'], unilateraltimeout=1) + + # 5 blocks before we can spend to-us. + bitcoind.generate_block(5, wait_for_mempool=1) + # 100 more to forget channel + bitcoind.generate_block(100, wait_for_mempool=1) + wait_for(lambda: l1.rpc.listclosedchannels()['closedchannels'] != []) + + # l2 reconnects, gets reestablish before error. + l1.rpc.connect(l2.info['id'], 'localhost', l2.port) + l1.daemon.wait_for_log('Responded to reestablish for long-closed channel') + l2.daemon.wait_for_log('peer_in WIRE_CHANNEL_REESTABLISH') + l2.daemon.wait_for_log('peer_in WIRE_ERROR') + + # Make sure l2 was happy with the reestablish message. + assert not l2.daemon.is_in_log('bad reestablish') + + @unittest.skipIf(TEST_NETWORK != 'regtest', "elementsd doesn't use p2tr anyway") def test_onchain_close_no_p2tr(node_factory, bitcoind): """Closing with a peer which doesn't support OPT_SHUTDOWN_ANYSEGWIT""" diff --git a/tests/test_connection.py b/tests/test_connection.py index d52d12bc35b8..c5f9a8f0c76d 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -1393,7 +1393,7 @@ def test_funding_external_wallet_corners(node_factory, bitcoind): except RpcError as err: assert "disconnected during connection" in err.error - l1.daemon.wait_for_log('Unknown channel .* for WIRE_CHANNEL_REESTABLISH') + l1.daemon.wait_for_log('Responded to reestablish for long-closed channel') wait_for(lambda: len(l1.rpc.listpeers()['peers']) == 0) wait_for(lambda: len(l2.rpc.listpeers()['peers']) == 0)