wallet: don't delete old htlcs when we forget a channel, do it on startup.
What the developer wrote
Authored by Rusty Russell
wallet: don't delete old htlcs when we forget a channel, do it on startup.
For old channels, this can take a while, and it stops everything. But
we are only doing this to save space; it's not a *functional* necessity.
A quick and dirty test with 50,000 htlcs shows the htlc deletion took
450msec. I tried adding an index, and changing it to set hstate to
HTLC_STATE_INVALID instead of deleting entries, but it still took about 350ms.
Whereas the "COUNT(*)" only took 1.7msec, so it's worth keeping.
Reported-by: @michael1011
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: lightningd: we defer deletion of old htlcs on channel close, to avoid pausing for a long time (we clean them on startup)
Fixes: https://github.com/ElementsProject/lightning/issues/7962
What changed, and why it matters
This change moves a database cleanup job from the moment a channel closes to the next time the node starts up. Previously, deleting tens of thousands of old payment records when a channel closed could freeze the node for hundreds of milliseconds. Now the records are counted and reported immediately, but actually deleted later during startup. It is a performance and availability improvement, not a security fix.
No security action required. Treat as a normal performance/availability improvement. Operators may notice slightly faster channel closure and a small startup-time cost proportional to accumulated closed-channel htlcs.
Security signals we found
No security-relevant code path changed
Change is purely operational/performance: deferring expensive DB deletion to startup
No new attack surface introduced; function uses existing prepared-statement SQL binding
No mention of vulnerability, CVE, or security issue in commit message or diff
Evidence from the diff
The patch removes the synchronous DELETE of channel_htlcs rows from wallet_channel_close() and replaces it with a SELECT COUNT(*) so the htlcs_index_deleted notification still fires. A new wallet_delete_old_htlcs() function is added and called once during lightningd startup; it deletes htlcs whose channel state is CLOSED. A test is updated to restart the node and verify the deferred deletion occurs. No cryptographic, authorization, or input-validation changes are present.
Changed components
wallet/wallet.cwallet/wallet.hlightningd/lightningd.ctests/test_misc.pyInspect captured patch +44 / −6
diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c
index 1c3fb7e4..5a350dae 100644
--- a/lightningd/lightningd.c
+++ b/lightningd/lightningd.c
@@ -1371,6 +1371,9 @@ int main(int argc, char *argv[])
trace_span_end(ld->topology);
db_begin_transaction(ld->wallet->db);
+ trace_span_start("delete_old_htlcs", ld->wallet);
+ wallet_delete_old_htlcs(ld->wallet);
+ trace_span_end(ld->wallet);
/*~ Pull peers, channels and HTLCs from db. Needs to happen after the
* topology is initialized since some decisions rely on being able to
diff --git a/lightningd/test/run-find_my_abspath.c b/lightningd/test/run-find_my_abspath.c
index 80702af2..4c86ad9a 100644
--- a/lightningd/test/run-find_my_abspath.c
+++ b/lightningd/test/run-find_my_abspath.c
@@ -286,6 +286,9 @@ void waitblockheight_notify_new_block(struct lightningd *ld UNNEEDED)
/* Generated stub for wallet_begin_old_close_rescan */
void wallet_begin_old_close_rescan(struct lightningd *ld UNNEEDED)
{ fprintf(stderr, "wallet_begin_old_close_rescan called!\n"); abort(); }
+/* Generated stub for wallet_delete_old_htlcs */
+void wallet_delete_old_htlcs(struct wallet *w UNNEEDED)
+{ fprintf(stderr, "wallet_delete_old_htlcs called!\n"); abort(); }
/* Generated stub for wallet_new */
struct wallet *wallet_new(struct lightningd *ld UNNEEDED, struct timers *timers UNNEEDED)
{ fprintf(stderr, "wallet_new called!\n"); abort(); }
diff --git a/tests/test_misc.py b/tests/test_misc.py
index 5759bbeb..0a6aba28 100644
--- a/tests/test_misc.py
+++ b/tests/test_misc.py
@@ -3461,6 +3461,9 @@ def test_listforwards_and_listhtlcs(node_factory, bitcoind):
l2.rpc.delforward(in_channel=c12, in_htlc_id=2, status='local_failed')
assert l2.rpc.listforwards() == {'forwards': []}
+ l2.restart()
+ assert l2.rpc.wait('htlcs', 'deleted', 0)['deleted'] == 5
+
def test_listforwards_wait(node_factory, executor):
l1, l2, l3 = node_factory.line_graph(3, wait_for_announce=True)
diff --git a/wallet/wallet.c b/wallet/wallet.c
index 1c7ad233..64eea8b4 100644
--- a/wallet/wallet.c
+++ b/wallet/wallet.c
@@ -2957,6 +2957,21 @@ void wallet_channel_insert(struct wallet *w, struct channel *chan)
wallet_channel_save(w, chan);
}
+void wallet_delete_old_htlcs(struct wallet *w)
+{
+ struct db_stmt *stmt;
+
+ /* Delete htlcs for closed channels */
+ stmt = db_prepare_v2(w->db, SQL("DELETE FROM channel_htlcs"
+ " WHERE id IN ("
+ " SELECT ch.id"
+ " FROM channel_htlcs AS ch"
+ " JOIN channels AS c ON c.id = ch.channel_id"
+ " WHERE c.state = ?);"));
+ db_bind_int(stmt, channel_state_in_db(CLOSED));
+ db_exec_prepared_v2(take(stmt));
+}
+
void wallet_channel_close(struct wallet *w,
const struct channel *chan)
{
@@ -2967,15 +2982,21 @@ void wallet_channel_close(struct wallet *w,
* dbs to recover. */
struct db_stmt *stmt;
u64 new_move_id;
+ u64 htlcs;
- /* Delete entries from `channel_htlcs` */
- stmt = db_prepare_v2(w->db, SQL("DELETE FROM channel_htlcs "
+ /* The channel_htlcs table is quite large, and deleting it can take a
+ * while. So we do that on next restart by calling
+ * wallet_delete_old_htlcs. But update delete count in case anyone
+ * is watching. */
+ stmt = db_prepare_v2(w->db, SQL("SELECT COUNT(*) FROM channel_htlcs "
"WHERE channel_id=?"));
db_bind_u64(stmt, chan->dbid);
- db_exec_prepared_v2(stmt);
- /* FIXME: We don't actually tell them what was deleted! */
- if (db_count_changes(stmt) != 0)
- htlcs_index_deleted(w->ld, chan, db_count_changes(stmt));
+ db_query_prepared(stmt);
+ db_step(stmt);
+
+ htlcs = db_col_u64(stmt, "COUNT(*)");
+ if (htlcs != 0)
+ htlcs_index_deleted(w->ld, chan, htlcs);
tal_free(stmt);
/* Delete entries from `htlc_sigs` */
diff --git a/wallet/wallet.h b/wallet/wallet.h
index 584f17b0..c08c03f2 100644
--- a/wallet/wallet.h
+++ b/wallet/wallet.h
@@ -725,6 +725,14 @@ void wallet_state_change_add(struct wallet *w,
*/
void wallet_delete_peer_if_unused(struct wallet *w, u64 peer_dbid);
+/**
+ * wallet_delete_old_htlcs -- delete htlcs associated with CLOSED channels.
+ *
+ * We do this at startup, instead of when we finally CLOSED a channel, to
+ * avoid a significant pause.
+ */
+void wallet_delete_old_htlcs(struct wallet *w);
+
/**
* wallet_init_channels -- Loads active channels into peers
* and inits the dbid counter for next channel.
Why this scored 21/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.