wallet: don't insert duplicate chain_moves entries after accounts.db migration.

When we migrate from accounts.db, we use the `account_nonchannel_id`
field.  But we can replay the block chain and the channel involved is
still open, we will use the `account_channel_id` field, and our duplicate
detection fails.

As a result, we can end up with duplicate entries in the database, which
make accounting incorrect.

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Fixed: JSON-RPC: `listchainmoves` could contain bogus duplicate entries after 25.09 bookkeeper migration.

Author: rustyrussell

wallet/wallet.c

@@ -246,7 +246,7 @@ struct wallet *wallet_new(struct lightningd *ld, struct timers *timers)
 }
 
 /* Get id for move_accounts; create if necessary */
-u64 move_accounts_id(struct db *db, const char *name)
+static u64 move_accounts_id(struct db *db, const char *name, bool create)
 {
 	struct db_stmt *stmt;
 	u64 ret;
@@ -268,6 +268,9 @@ u64 move_accounts_id(struct db *db, const char *name)
 	}
 	tal_free(stmt);
 
+	if (!create)
+		return 0;
+
 	/* Does not exist, so create */
 	stmt = db_prepare_v2(db,
 			     SQL("INSERT INTO move_accounts (name) VALUES (?)"));
@@ -3047,7 +3050,7 @@ void wallet_channel_close(struct wallet *w,
 	/* Update all accouting records to use channel_id string, instead of
 	 * referring to dbid.  This is robust if we delete in future, and saves
 	 * a lookup in the load path. */
-	new_move_id = move_accounts_id(w->db, fmt_channel_id(tmpctx, &chan->cid));
+	new_move_id = move_accounts_id(w->db, fmt_channel_id(tmpctx, &chan->cid), true);
 	stmt = db_prepare_v2(w->db, SQL("UPDATE chain_moves "
 					"SET account_channel_id=?,"
 					" account_nonchannel_id=? "
@@ -6959,7 +6962,7 @@ void db_bind_mvt_account_id(struct db_stmt *stmt,
 		db_bind_null(stmt);
 	} else {
 		db_bind_null(stmt);
-		db_bind_u64(stmt, move_accounts_id(db, account->alt_account));
+		db_bind_u64(stmt, move_accounts_id(db, account->alt_account, true));
 	}
 }
 
@@ -7085,73 +7088,124 @@ static u64 insert_chain_mvt(struct lightningd *ld,
 	return id;
 }
 
-void wallet_save_chain_mvt(struct lightningd *ld,
-			   const struct chain_coin_mvt *chain_mvt)
+static struct mvt_tags db_col_mvt_tags(struct db_stmt *stmt,
+				       const char *colname)
+{
+	struct mvt_tags tags;
+	tags.bits = db_col_u64(stmt, colname);
+	assert(mvt_tags_valid(tags));
+	return tags;
+}
+
+static bool find_duplicate_chain_move(struct db *db,
+				      const char *nonchannel_acctname,
+				      /* 0 if none */
+				      u64 channel_dbid,
+				      const struct bitcoin_outpoint *outpoint,
+				      /* optional */
+				      const struct bitcoin_txid *spending_txid,
+				      struct mvt_tags tags,
+				      u64 id_ceiling)
 {
 	struct db_stmt *stmt;
-	u64 id;
+	u64 nonchannel_id;
 
-	/* On restart, we do chain replay.  For this (and other
-	 * reorgs) we need to de-duplicate here.  The other db tables
-	 * do this by deleting old entries on reorg, but we never
-	 * delete. */
-	if (chain_mvt->account.channel) {
-		stmt = db_prepare_v2(ld->wallet->db,
-				     SQL("SELECT"
-					 "  cm.spending_txid, cm.tag_bitmap, cm.id"
-					 " FROM chain_moves cm"
-					 " WHERE "
-					 "  account_channel_id = ?"
-					 "  AND utxo = ?"));
-		db_bind_u64(stmt, chain_mvt->account.channel->dbid);
-	} else {
-		stmt = db_prepare_v2(ld->wallet->db,
-				     SQL("SELECT"
-					 "  cm.spending_txid, cm.tag_bitmap, cm.id"
-					 " FROM chain_moves cm"
-					 " JOIN move_accounts ma ON cm.account_nonchannel_id = ma.id"
-					 " WHERE"
-					 "  ma.name = ?"
-					 "  AND utxo = ?"));
-		db_bind_text(stmt, chain_mvt->account.alt_account);
-	}
-	db_bind_outpoint(stmt, &chain_mvt->outpoint);
+	nonchannel_id = move_accounts_id(db, nonchannel_acctname, false);
+
+	stmt = db_prepare_v2(db,
+			     SQL("SELECT"
+				 "  spending_txid, tag_bitmap, account_channel_id, account_nonchannel_id"
+				 " FROM chain_moves"
+				 " WHERE "
+				 "  utxo = ?"
+				 " AND "
+				 "  id < ?"));
+	db_bind_outpoint(stmt, outpoint);
+	db_bind_u64(stmt, id_ceiling);
 	db_query_prepared(stmt);
 
 	/* Check that spending_txid and primary_tag match.  We could
 	 * probably just match on spending_txid, but this is robust. */
 	while (db_step(stmt)) {
-		struct mvt_tags tags;
+		struct mvt_tags these_tags;
+		u64 this_nonchannel_id, this_channel_id;
+		bool have_spending_txid;
 
 		/* Access this now so it never complains we don't */
-		tags.bits = db_col_u64(stmt, "cm.tag_bitmap");
-		id = db_col_u64(stmt, "cm.id");
+		these_tags = db_col_mvt_tags(stmt, "tag_bitmap");
+		have_spending_txid = !db_col_is_null(stmt, "spending_txid");
+		this_nonchannel_id = db_col_is_null(stmt, "account_nonchannel_id") ? 0 : db_col_u64(stmt, "account_nonchannel_id");
+		this_channel_id = db_col_is_null(stmt, "account_channel_id") ? 0 : db_col_u64(stmt, "account_channel_id");
+
+		/* Either nonchannel id, or channel id must match */
+		if (nonchannel_id != this_nonchannel_id
+		    && channel_dbid != this_channel_id) {
+			continue;
+		}
 
 		/* spending_txid must match */
-		if (chain_mvt->spending_txid) {
+		if (spending_txid) {
 			struct bitcoin_txid txid;
-			if (db_col_is_null(stmt, "cm.spending_txid"))
+			if (!have_spending_txid)
 				continue;
-			db_col_txid(stmt, "cm.spending_txid", &txid);
+			db_col_txid(stmt, "spending_txid", &txid);
 			/* This would only happen for reorgs */
-			if (!bitcoin_txid_eq(&txid, chain_mvt->spending_txid))
+			if (!bitcoin_txid_eq(&txid, spending_txid))
 				continue;
 		} else {
-			if (!db_col_is_null(stmt, "cm.spending_txid"))
+			if (have_spending_txid)
 				continue;
 		}
 
 		/* Tags must match */
-		if (primary_mvt_tag(tags) != primary_mvt_tag(chain_mvt->tags))
+		if (primary_mvt_tag(these_tags) != primary_mvt_tag(tags))
 			continue;
 
-		/* It's a duplicate.  Don't re-add. */
+		/* It's a duplicate. */
 		tal_free(stmt);
-		goto out;
+		return true;
 	}
 	tal_free(stmt);
+	return false;
+}
+
+static bool is_duplicate(struct db *db, const struct chain_coin_mvt *chain_mvt)
+{
+	const char *nonchannel_acctname;
+	u64 channel_dbid;
+
+	/* If we migrated in account_migration.c, it will use the
+	 * nonchannel id!  But we don't worry about adding a
+	 * non-channel which conflicts with a channel. */
+	if (chain_mvt->account.channel) {
+		nonchannel_acctname = fmt_channel_id(tmpctx, &chain_mvt->account.channel->cid);
+		channel_dbid = chain_mvt->account.channel->dbid;
+	} else {
+		nonchannel_acctname = chain_mvt->account.alt_account;
+		channel_dbid = 0;
+	}
 
-	id = insert_chain_mvt(ld, ld->wallet->db, chain_mvt),
+	return find_duplicate_chain_move(db, nonchannel_acctname,
+					 channel_dbid,
+					 &chain_mvt->outpoint,
+					 chain_mvt->spending_txid,
+					 chain_mvt->tags,
+					 INT64_MAX);
+}
+
+void wallet_save_chain_mvt(struct lightningd *ld,
+			   const struct chain_coin_mvt *chain_mvt)
+{
+	u64 id;
+
+	/* On restart, we do chain replay.  For this (and other
+	 * reorgs) we need to de-duplicate here.  The other db tables
+	 * do this by deleting old entries on reorg, but we never
+	 * delete. */
+	if (is_duplicate(ld->wallet->db, chain_mvt))
+		goto out;
+
+	id = insert_chain_mvt(ld, ld->wallet->db, chain_mvt);
 	notify_chain_mvt(ld, chain_mvt, id);
 out:
 	if (taken(chain_mvt))
@@ -7190,15 +7244,6 @@ static void db_col_credit_or_debit(struct db_stmt *stmt,
 	}
 }
 
-static struct mvt_tags db_col_mvt_tags(struct db_stmt *stmt,
-				       const char *colname)
-{
-	struct mvt_tags tags;
-	tags.bits = db_col_u64(stmt, colname);
-	assert(mvt_tags_valid(tags));
-	return tags;
-}
-
 struct chain_coin_mvt *wallet_chain_move_extract(const tal_t *ctx,
 						 struct db_stmt *stmt,
 						 struct lightningd *ld,

wallet/wallet.h

@@ -1919,7 +1919,6 @@ void db_bind_mvt_account_id(struct db_stmt *stmt,
 void db_bind_credit_debit(struct db_stmt *stmt,
 			  struct amount_msat credit,
 			  struct amount_msat debit);
-u64 move_accounts_id(struct db *db, const char *name);
 void wallet_datastore_save_utxo_description(struct db *db,
 					    const struct bitcoin_outpoint *outpoint,
 					    const char *desc);