wallet: don't insert duplicate chain_moves entries after accounts.db migration.
What changed, and why it matters
This commit fixes a bug in Core Lightning's accounting/bookkeeping system where the same on-chain money movement could be recorded twice in the database after a migration. The bug happens because the code looked for duplicates using one account identifier, but after a migration or replay it could store the same event under a different identifier. The fix makes duplicate detection check both possible identifiers. It is a data-integrity bug, not a security vulnerability that an attacker can exploit.
Treat as a routine bug fix. Users relying on accounting reports should upgrade and may need to audit or rebuild chain_moves data if duplicates were already introduced. No emergency security response is warranted.
Security signals we found
Data integrity / accounting correctness bug
Duplicate database rows possible after migration
No input validation or memory safety issue
No attacker-controlled code path identified
Evidence from the diff
The patch changes wallet_save_chain_mvt() duplicate detection. Previously, when saving a chain_coin_mvt, the code searched chain_moves by either account_channel_id or account_nonchannel_id depending on whether the account had a live channel. After migrating from accounts.db, records were inserted using account_nonchannel_id (the channel_id string), but on later block replay the same channel still being open caused insertion using account_channel_id (the numeric dbid), bypassing duplicate detection. The fix introduces find_duplicate_chain_move()/is_duplicate() which query all chain_moves for the same UTXO and then match on either the nonchannel account id or the channel dbid, plus spending_txid and primary tag. move_accounts_id() is made static and gains a ‘create’ flag so it can be used for lookup without inserting a new move_accounts row.
Changed components
wallet/wallet.cwallet/wallet.hchain_moves tablemove_accounts tablebookkeeper/accounting subsystemJSON-RPC listchainmovesInspect captured patch +97 / −53
diff --git a/wallet/wallet.c b/wallet/wallet.c
index 64eea8b4..7537b11a 100644
--- a/wallet/wallet.c
+++ b/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,
diff --git a/wallet/wallet.h b/wallet/wallet.h
index c08c03f2..08b8b7e1 100644
--- a/wallet/wallet.h
+++ b/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);
Why this scored 28/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.