Fix the long-elusive "sometimes we miss utxo spends" bug.

lightningd/chaintopology.c

@@ -888,8 +888,8 @@ static void updates_complete(struct chain_topology *topo)
 }
 
 static void record_wallet_spend(struct lightningd *ld,
-				struct bitcoin_outpoint *outpoint,
-				struct bitcoin_txid *txid,
+				const struct bitcoin_outpoint *outpoint,
+				const struct bitcoin_txid *txid,
 				u32 tx_blockheight)
 {
 	struct utxo *utxo;
@@ -911,31 +911,34 @@ static void record_wallet_spend(struct lightningd *ld,
 /**
  * topo_update_spends -- Tell the wallet about all spent outpoints
  */
-static void topo_update_spends(struct chain_topology *topo, struct block *b)
+static void topo_update_spends(struct chain_topology *topo,
+			       struct bitcoin_tx **txs,
+			       const struct bitcoin_txid *txids,
+			       u32 blockheight)
 {
 	const struct short_channel_id *spent_scids;
-	const size_t num_txs = tal_count(b->full_txs);
+	const size_t num_txs = tal_count(txs);
 	for (size_t i = 0; i < num_txs; i++) {
-		const struct bitcoin_tx *tx = b->full_txs[i];
+		const struct bitcoin_tx *tx = txs[i];
 
 		for (size_t j = 0; j < tx->wtx->num_inputs; j++) {
 			struct bitcoin_outpoint outpoint;
 
 			bitcoin_tx_input_get_outpoint(tx, j, &outpoint);
 
 			if (wallet_outpoint_spend(tmpctx, topo->ld->wallet,
-						  b->height, &outpoint))
+						  blockheight, &outpoint))
 				record_wallet_spend(topo->ld, &outpoint,
-						    &b->txids[i], b->height);
+						    &txids[i], blockheight);
 
 		}
 	}
 
 	/* Retrieve all potential channel closes from the UTXO set and
 	 * tell gossipd about them. */
 	spent_scids =
-	    wallet_utxoset_get_spent(tmpctx, topo->ld->wallet, b->height);
-	gossipd_notify_spends(topo->bitcoind->ld, b->height, spent_scids);
+	    wallet_utxoset_get_spent(tmpctx, topo->ld->wallet, blockheight);
+	gossipd_notify_spends(topo->bitcoind->ld, blockheight, spent_scids);
 }
 
 static void topo_add_utxos(struct chain_topology *topo, struct block *b)
@@ -982,7 +985,7 @@ static void add_tip(struct chain_topology *topo, struct block *b)
 	trace_span_end(b);
 
 	trace_span_start("topo_update_spends", b);
-	topo_update_spends(topo, b);
+	topo_update_spends(topo, b->full_txs, b->txids, b->height);
 	trace_span_end(b);
 
 	/* Only keep the transactions we care about. */
@@ -1388,6 +1391,7 @@ void setup_topology(struct chain_topology *topo)
 	struct bitcoin_block *blk;
 	bool blockscan_start_set;
 	u32 blockscan_start;
+	s64 fixup;
 
 	/* This waits for bitcoind. */
 	bitcoind_check_commands(topo->bitcoind);
@@ -1413,6 +1417,15 @@ void setup_topology(struct chain_topology *topo)
 			blockscan_start = blocknum_reduce(blockscan_start, topo->ld->config.rescan);
 	}
 
+	fixup = db_get_intvar(topo->ld->wallet->db, "fixup_block_scan", -1);
+	if (fixup == -1) {
+		/* Never done fixup: this is set to non-zero if we have blocks. */
+		topo->old_block_scan = wallet_blocks_minheight(topo->ld->wallet);
+		db_set_intvar(topo->ld->wallet->db, "fixup_block_scan",
+			      topo->old_block_scan);
+	} else {
+		topo->old_block_scan = fixup;
+	}
 	db_commit_transaction(topo->ld->wallet->db);
 
 	/* Sanity checks, then topology initialization. */
@@ -1509,6 +1522,36 @@ void setup_topology(struct chain_topology *topo)
 	tal_add_destructor(topo, destroy_chain_topology);
 }
 
+static void fixup_scan_block(struct bitcoind *bitcoind,
+			     u32 height,
+			     struct bitcoin_blkid *blkid,
+			     struct bitcoin_block *blk,
+			     struct chain_topology *topo)
+{
+	log_debug(topo->ld->log, "fixup_scan: block %u with %zu txs", height, tal_count(blk->tx));
+	topo_update_spends(topo, blk->tx, blk->txids, height);
+
+	/* Caught up. */
+	if (height == get_block_height(topo)) {
+		log_info(topo->ld->log, "Scanning for missed UTXOs finished");
+		db_set_intvar(topo->ld->wallet->db, "fixup_block_scan", 0);
+		return;
+	}
+
+	db_set_intvar(topo->ld->wallet->db, "fixup_block_scan", ++topo->old_block_scan);
+	bitcoind_getrawblockbyheight(topo, topo->bitcoind,
+				     topo->old_block_scan,
+				     fixup_scan_block, topo);
+}
+
+static void fixup_scan(struct chain_topology *topo)
+{
+	log_info(topo->ld->log, "Scanning for missed UTXOs from block %u", topo->old_block_scan);
+	bitcoind_getrawblockbyheight(topo, topo->bitcoind,
+				     topo->old_block_scan,
+				     fixup_scan_block, topo);
+}
+
 void begin_topology(struct chain_topology *topo)
 {
 	/* If we were not synced, start looping to check */
@@ -1518,6 +1561,9 @@ void begin_topology(struct chain_topology *topo)
 	start_fee_estimate(topo);
 	/* Regular block updates */
 	try_extend_tip(topo);
+
+	if (topo->old_block_scan)
+		fixup_scan(topo);
 }
 
 void stop_topology(struct chain_topology *topo)

lightningd/chaintopology.h

@@ -143,6 +143,9 @@ struct chain_topology {
 	/* The number of headers known to the bitcoin backend at startup. Not
 	 * updated after the initial check. */
 	u32 headercount;
+
+	/* Progress on routine to look for old missed transactions.  0 = not interested. */
+	u32 old_block_scan;
 };
 
 /* Information relevant to locating a TX in a blockchain. */

tests/test_wallet.py

@@ -4,9 +4,9 @@
 from fixtures import TEST_NETWORK
 from pyln.client import RpcError, Millisatoshi
 from utils import (
-    only_one, wait_for, sync_blockheight,
+    only_one, wait_for, sync_blockheight, mine_funding_to_announce,
     VALGRIND, check_coin_moves, TailableProc, scriptpubkey_addr,
-    check_utxos_channel, check_feerate, did_short_sig
+    check_utxos_channel, check_feerate, did_short_sig, first_scid,
 )
 
 import os
@@ -2536,3 +2536,47 @@ def test_hsm_wrong_passphrase_crash(node_factory):
 
     os.close(master_fd2)
     os.close(slave_fd2)
+
+
+def test_unspend_during_reorg(node_factory, bitcoind):
+    l1, l2 = node_factory.line_graph(2)
+    scid = first_scid(l1, l2)
+    blockheight, txindex, _ = scid.split('x')
+
+    # Use mainnet settings for rescan.
+    l3 = node_factory.get_node(options={'rescan': 15})
+    l3.connect(l2)
+
+    mine_funding_to_announce(bitcoind, [l1, l2, l3])
+    bitcoind.generate_block(20)
+    sync_blockheight(bitcoind, [l3])
+    wait_for(lambda: len(l3.rpc.listchannels()['channels']) == 2)
+
+    # db shows it unspent.
+    assert only_one(l1.db_query(f"SELECT spendheight as spendheight FROM utxoset WHERE blockheight={blockheight} AND txindex={txindex}"))['spendheight'] is None
+
+    # Now, l3 sees the close, marks channel dying.
+    l1.rpc.close(l2.info['id'])
+    spentheight = bitcoind.rpc.getblockcount() + 1
+    bitcoind.generate_block(14, wait_for_mempool=1)
+    wait_for(lambda: len(l3.rpc.listchannels()['channels']) == 2)
+
+    # In one fell swoop it goes through dying, to dead (12 blocks)
+    l3.daemon.wait_for_log(f"Adding block {spentheight}")
+    l3.daemon.wait_for_log(f"gossipd: channel {scid} closing soon due to the funding outpoint being spent")
+    l3.daemon.wait_for_log(f"gossipd: Deleting channel {scid} due to the funding outpoint being spent")
+
+    # db shows it spent
+    assert only_one(l3.db_query(f"SELECT spendheight as spendheight FROM utxoset WHERE blockheight={blockheight} AND txindex={txindex}"))['spendheight'] == spentheight
+
+    # Restart, see replay.
+    l3.stop()
+    # This is enough to take channel from dying to dead.
+    bitcoind.generate_block(10)
+
+    l3.start()
+    # Channel should still be dead.
+    l3.daemon.wait_for_log(f"Adding block {spentheight}")
+
+    sync_blockheight(bitcoind, [l3])
+    assert only_one(l3.db_query(f"SELECT spendheight as spendheight FROM utxoset WHERE blockheight={blockheight} AND txindex={txindex}"))['spendheight'] == spentheight

wallet/wallet.c

@@ -180,32 +180,39 @@ static void our_addresses_init(struct wallet *w)
 	w->our_addresses_maxindex = w->keyscan_gap;
 }
 
-static void outpointfilters_init(struct wallet *w)
+/* Idempotent: outpointfilter_add is a noop if it already exists. */
+static void refill_outpointfilters(struct wallet *w)
 {
 	struct db_stmt *stmt;
-	struct utxo **utxos = wallet_get_all_utxos(NULL, w);
-	struct bitcoin_outpoint outpoint;
 
-	w->owned_outpoints = outpointfilter_new(w);
-	for (size_t i = 0; i < tal_count(utxos); i++)
-		outpointfilter_add(w->owned_outpoints, &utxos[i]->outpoint);
-
-	tal_free(utxos);
-
-	w->utxoset_outpoints = outpointfilter_new(w);
 	stmt = db_prepare_v2(
 	    w->db,
 	    SQL("SELECT txid, outnum FROM utxoset WHERE spendheight is NULL"));
 	db_query_prepared(stmt);
 
 	while (db_step(stmt)) {
+		struct bitcoin_outpoint outpoint;
 		db_col_txid(stmt, "txid", &outpoint.txid);
 		outpoint.n = db_col_int(stmt, "outnum");
 		outpointfilter_add(w->utxoset_outpoints, &outpoint);
 	}
 	tal_free(stmt);
 }
 
+static void outpointfilters_init(struct wallet *w)
+{
+	struct utxo **utxos = wallet_get_all_utxos(NULL, w);
+
+	w->owned_outpoints = outpointfilter_new(w);
+	for (size_t i = 0; i < tal_count(utxos); i++)
+		outpointfilter_add(w->owned_outpoints, &utxos[i]->outpoint);
+
+	tal_free(utxos);
+
+	w->utxoset_outpoints = outpointfilter_new(w);
+	refill_outpointfilters(w);
+}
+
 struct wallet *wallet_new(struct lightningd *ld, struct timers *timers)
 {
 	struct wallet *wallet = tal(ld, struct wallet);
@@ -2547,6 +2554,22 @@ u32 wallet_blocks_maxheight(struct wallet *w)
 	return max;
 }
 
+u32 wallet_blocks_minheight(struct wallet *w)
+{
+	u32 min = 0;
+	struct db_stmt *stmt = db_prepare_v2(w->db, SQL("SELECT MIN(height) FROM blocks;"));
+	db_query_prepared(stmt);
+
+	/* If we ever processed a block we'll get the latest block in the chain */
+	if (db_step(stmt)) {
+		if (!db_col_is_null(stmt, "MIN(height)")) {
+			min = db_col_int(stmt, "MIN(height)");
+		}
+	}
+	tal_free(stmt);
+	return min;
+}
+
 static void wallet_channel_config_insert(struct wallet *w,
 					 struct channel_config *cc)
 {
@@ -4867,6 +4890,9 @@ void wallet_block_remove(struct wallet *w, struct block *b)
 	db_query_prepared(stmt);
 	assert(!db_step(stmt));
 	tal_free(stmt);
+
+	/* We might need to watch more now-unspent UTXOs */
+	refill_outpointfilters(w);
 }
 
 void wallet_blocks_rollback(struct wallet *w, u32 height)
@@ -4875,6 +4901,7 @@ void wallet_blocks_rollback(struct wallet *w, u32 height)
 							"WHERE height > ?"));
 	db_bind_int(stmt, height);
 	db_exec_prepared_v2(take(stmt));
+	refill_outpointfilters(w);
 }
 
 bool wallet_outpoint_spend(const tal_t *ctx, struct wallet *w, const u32 blockheight,

wallet/wallet.h

@@ -789,6 +789,15 @@ void wallet_channel_stats_incr_out_fulfilled(struct wallet *w, u64 cdbid, struct
  */
 u32 wallet_blocks_maxheight(struct wallet *w);
 
+/**
+ * Retrieve the blockheight of the first block processed by lightningd.
+ *
+ * Will return the 0 if the wallet was never used before.
+ *
+ * @w: wallet to load from.
+ */
+u32 wallet_blocks_minheight(struct wallet *w);
+
 /**
  * wallet_extract_owned_outputs - given a tx, extract all of our outputs
  */