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 <[email protected]>
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: #7962

Author: rustyrussell

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

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(); }

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)

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` */

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.