Informational 17 Bitcoin Committed Aug 19, 2025 · 04:07 UTC

common: fix values of enum mvt_tag since they're now embedded in the db.

74abe33f View source ↗

Public commit record

What the developer wrote

Authored by Rusty Russell

65/100 · Adequate

common: fix values of enum mvt_tag since they're now embedded in the db.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

✓ Specific, descriptive subject✓ Names a concrete action or component✓ Provides an explanatory body

The short version

What changed, and why it matters

This commit hardens how Core Lightning records coin-movement tags in its database. It locks the numeric values of an internal enum so future code changes cannot silently reorder them, adds a translation helper to ensure the stored database values stay stable, and validates tag sets before writing them. There is no direct exploit here; it is a defensive fix to prevent data corruption or accounting mismatches if the enum were ever changed.

Recommended action

No urgent action required. Treat as routine hardening. Reviewers should verify that all call sites writing mvt_tags to the database now use db_bind_mvt_tags() and that the BUILD_ASSERTs cover every enum member.

Security signals we found

Defensive hardening of persisted enum values

Addition of runtime validation before database writes

BUILD_ASSERTs to prevent future enum reordering

No memory safety, cryptographic, or network vulnerability evident

Technical analysis

Evidence from the diff

The patch makes enum mvt_tag values explicit and immutable in the database schema by adding BUILD_ASSERTs for each value in mvt_tag_in_db(), introducing db_bind_mvt_tags() with an assert(mvt_tags_valid(tags)) check, and replacing raw db_bind_u64() calls for tags.bits with the new wrapper. It also exposes mvt_tags_valid() for assertions and updates test stubs. The change prevents accidental enum reordering from corrupting persisted bitfields and adds runtime validation that exactly one primary tag is present.

Changed components

common/coin_mvt.ccommon/coin_mvt.hwallet/wallet.cwallet/test/run-db.cwallet/test/run-wallet.c

Inspect captured patch +106 / −5

diff --git a/common/coin_mvt.c b/common/coin_mvt.c
index cadd7b45..5017f0bd 100644
--- a/common/coin_mvt.c
+++ b/common/coin_mvt.c
@@ -60,6 +60,91 @@ static const char *mvt_tags[] = {
 			  (1ULL << MVT_JOURNAL) |	\
 			  (1ULL << MVT_CHANNEL_PROPOSED))
 
+static enum mvt_tag mvt_tag_in_db(enum mvt_tag mvt_tag)
+{
+	switch (mvt_tag) {
+	case MVT_DEPOSIT:
+		BUILD_ASSERT(MVT_DEPOSIT == 0);
+		return mvt_tag;
+	case MVT_WITHDRAWAL:
+		BUILD_ASSERT(MVT_WITHDRAWAL == 1);
+		return mvt_tag;
+	case MVT_PENALTY:
+		BUILD_ASSERT(MVT_PENALTY == 2);
+		return mvt_tag;
+	case MVT_INVOICE:
+		BUILD_ASSERT(MVT_INVOICE == 3);
+		return mvt_tag;
+	case MVT_ROUTED:
+		BUILD_ASSERT(MVT_ROUTED == 4);
+		return mvt_tag;
+	case MVT_PUSHED:
+		BUILD_ASSERT(MVT_PUSHED == 5);
+		return mvt_tag;
+	case MVT_CHANNEL_OPEN:
+		BUILD_ASSERT(MVT_CHANNEL_OPEN == 6);
+		return mvt_tag;
+	case MVT_CHANNEL_CLOSE:
+		BUILD_ASSERT(MVT_CHANNEL_CLOSE == 7);
+		return mvt_tag;
+	case MVT_CHANNEL_TO_US:
+		BUILD_ASSERT(MVT_CHANNEL_TO_US == 8);
+		return mvt_tag;
+	case MVT_HTLC_TIMEOUT:
+		BUILD_ASSERT(MVT_HTLC_TIMEOUT == 9);
+		return mvt_tag;
+	case MVT_HTLC_FULFILL:
+		BUILD_ASSERT(MVT_HTLC_FULFILL == 10);
+		return mvt_tag;
+	case MVT_HTLC_TX:
+		BUILD_ASSERT(MVT_HTLC_TX == 11);
+		return mvt_tag;
+	case MVT_TO_WALLET:
+		BUILD_ASSERT(MVT_TO_WALLET == 12);
+		return mvt_tag;
+	case MVT_ANCHOR:
+		BUILD_ASSERT(MVT_ANCHOR == 13);
+		return mvt_tag;
+	case MVT_TO_THEM:
+		BUILD_ASSERT(MVT_TO_THEM == 14);
+		return mvt_tag;
+	case MVT_PENALIZED:
+		BUILD_ASSERT(MVT_PENALIZED == 15);
+		return mvt_tag;
+	case MVT_STOLEN:
+		BUILD_ASSERT(MVT_STOLEN == 16);
+		return mvt_tag;
+	case MVT_TO_MINER:
+		BUILD_ASSERT(MVT_TO_MINER == 17);
+		return mvt_tag;
+	case MVT_OPENER:
+		BUILD_ASSERT(MVT_OPENER == 18);
+		return mvt_tag;
+	case MVT_LEASE_FEE:
+		BUILD_ASSERT(MVT_LEASE_FEE == 19);
+		return mvt_tag;
+	case MVT_LEASED:
+		BUILD_ASSERT(MVT_LEASED == 20);
+		return mvt_tag;
+	case MVT_STEALABLE:
+		BUILD_ASSERT(MVT_STEALABLE == 21);
+		return mvt_tag;
+	case MVT_CHANNEL_PROPOSED:
+		BUILD_ASSERT(MVT_CHANNEL_PROPOSED == 22);
+		return mvt_tag;
+	case MVT_SPLICE:
+		BUILD_ASSERT(MVT_SPLICE == 23);
+		return mvt_tag;
+	case MVT_PENALTY_ADJ:
+		BUILD_ASSERT(MVT_PENALTY_ADJ == 24);
+		return mvt_tag;
+	case MVT_JOURNAL:
+		BUILD_ASSERT(MVT_JOURNAL == 25);
+		return mvt_tag;
+	}
+	abort();
+}
+
 const char *mvt_tag_str(enum mvt_tag tag)
 {
 	return mvt_tags[tag];
@@ -67,14 +152,14 @@ const char *mvt_tag_str(enum mvt_tag tag)
 
 static void tag_set(struct mvt_tags *tags, enum mvt_tag tag)
 {
-	u64 bitnum = tag;
+	u64 bitnum = mvt_tag_in_db(tag);
 	assert(bitnum < NUM_MVT_TAGS);
 	/* Not already set! */
 	assert((tags->bits & (1ULL << bitnum)) == 0);
 	tags->bits |= (1ULL << bitnum);
 }
 
-static bool mvt_tags_valid(struct mvt_tags tags)
+bool mvt_tags_valid(struct mvt_tags tags)
 {
 	u64 primaries = (tags.bits & PRIMARY_TAG_BITS);
 	/* Must have exactly one primary. */
@@ -560,7 +645,7 @@ struct mvt_tags mk_mvt_tags_(enum mvt_tag tag, ...)
 	tag_set(&ret, tag);
 	va_start(ap, tag);
 	while ((tag = va_arg(ap, enum mvt_tag)) != 999)
-		tag_set(&ret, tag);
+		tag_set(&ret, mvt_tag_in_db(tag));
 	va_end(ap);
 	return ret;
 }
diff --git a/common/coin_mvt.h b/common/coin_mvt.h
index 682387a2..ad72beb6 100644
--- a/common/coin_mvt.h
+++ b/common/coin_mvt.h
@@ -11,6 +11,7 @@
 #define ACCOUNT_NAME_WALLET "wallet"
 #define ACCOUNT_NAME_EXTERNAL "external"
 
+/* /!\ You cannot change this order, it's committed to the db! /!\ */
 enum mvt_tag {
 	MVT_DEPOSIT = 0,
 	MVT_WITHDRAWAL = 1,
@@ -131,6 +132,9 @@ static inline struct mvt_tags tag_to_mvt_tags(enum mvt_tag tag)
 /* Extract the primary tag */
 enum mvt_tag primary_mvt_tag(struct mvt_tags tags);
 
+/* Useful for assertions */
+bool mvt_tags_valid(struct mvt_tags tags);
+
 /* Useful constructor for mvt_account_id: exactly one of channel/account_name must be NULL */
 void set_mvt_account_id(struct mvt_account_id *acct_id,
 			const struct channel *channel,
diff --git a/wallet/test/run-db.c b/wallet/test/run-db.c
index bb054990..fbf4f392 100644
--- a/wallet/test/run-db.c
+++ b/wallet/test/run-db.c
@@ -160,6 +160,9 @@ void memleak_scan_outpointfilter(struct htable *memtable UNNEEDED,
 /* Generated stub for mk_mvt_tags_ */
 struct mvt_tags mk_mvt_tags_(enum mvt_tag tag UNNEEDED, ...)
 { fprintf(stderr, "mk_mvt_tags_ called!\n"); abort(); }
+/* Generated stub for mvt_tags_valid */
+bool mvt_tags_valid(struct mvt_tags tags UNNEEDED)
+{ fprintf(stderr, "mvt_tags_valid called!\n"); abort(); }
 /* Generated stub for new_channel */
 struct channel *new_channel(struct peer *peer UNNEEDED, u64 dbid UNNEEDED,
 			    /* NULL or stolen */
diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c
index 72deac48..13aa9461 100644
--- a/wallet/test/run-wallet.c
+++ b/wallet/test/run-wallet.c
@@ -644,6 +644,9 @@ void memleak_scan_outpointfilter(struct htable *memtable UNNEEDED,
 /* Generated stub for mk_mvt_tags_ */
 struct mvt_tags mk_mvt_tags_(enum mvt_tag tag UNNEEDED, ...)
 { fprintf(stderr, "mk_mvt_tags_ called!\n"); abort(); }
+/* Generated stub for mvt_tags_valid */
+bool mvt_tags_valid(struct mvt_tags tags UNNEEDED)
+{ fprintf(stderr, "mvt_tags_valid called!\n"); abort(); }
 /* Generated stub for new_channel_mvt_invoice_hin */
 struct channel_coin_mvt *new_channel_mvt_invoice_hin(const tal_t *ctx UNNEEDED,
 						     const struct htlc_in *hin UNNEEDED,
diff --git a/wallet/wallet.c b/wallet/wallet.c
index 96aed660..bbe078f4 100644
--- a/wallet/wallet.c
+++ b/wallet/wallet.c
@@ -6895,6 +6895,12 @@ static void db_bind_mvt_account_id(struct db_stmt *stmt,
 	}
 }
 
+static void db_bind_mvt_tags(struct db_stmt *stmt, struct mvt_tags tags)
+{
+	assert(mvt_tags_valid(tags));
+	db_bind_u64(stmt, tags.bits);
+}
+
 void wallet_save_channel_mvt(struct lightningd *ld,
 			     const struct channel_coin_mvt *chan_mvt)
 {
@@ -6913,7 +6919,7 @@ void wallet_save_channel_mvt(struct lightningd *ld,
 				 " fees) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?);"));
 	db_bind_mvt_account_id(stmt, ld, &chan_mvt->account);
 	db_bind_credit_debit(stmt, chan_mvt->credit, chan_mvt->debit);
-	db_bind_u64(stmt, chan_mvt->tags.bits);
+	db_bind_mvt_tags(stmt, chan_mvt->tags);
 	db_bind_u64(stmt, chan_mvt->timestamp);
 	/* push funding / leases don't have a payment_hash */
 	if (chan_mvt->payment_hash)
@@ -7018,7 +7024,7 @@ void wallet_save_chain_mvt(struct lightningd *ld,
 				 " originating_nonchannel_id,"
 				 " output_count) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)"));
 	db_bind_mvt_account_id(stmt, ld, &chain_mvt->account);
-	db_bind_u64(stmt, chain_mvt->tags.bits);
+	db_bind_mvt_tags(stmt, chain_mvt->tags);
 	db_bind_credit_debit(stmt, chain_mvt->credit, chain_mvt->debit);
 	db_bind_u64(stmt, chain_mvt->timestamp);
 	db_bind_outpoint(stmt, &chain_mvt->outpoint);

Risk score

Why this scored 17/100

Our methodology →

Potential impact 2/30

Exploitability 0/25

Stealth signal 1/15

Affected reach 2/15

Confidence 8/10

Evidence quality 4/5

Human-validated context

Community notes

Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.

No validated notes yet.

The AI analysis stands alone for now. Submit a note if you can add evidence or important context.

What the developer wrote

What changed, and why it matters

Security signals we found

Evidence from the diff

Changed components

Why this scored 17/100

Community notes

Related watches

lightningd: internal cleanups since all non-command JSON IDs are strings.

downgrade: we need to remove "impressions" from askrene datastore when downgrading to v26.06.

downgrade tool: fix invalid memory bug