common: treat a reorged close output as csv-locked
What the developer wrote
Authored by cdecker
common: treat a reorged close output as csv-locked
If, during a run of CLN, a close gets confirmed, its outputs are added
to the wallet. If then the close is reorged out, i.e., it loses its
confirmation, using it in this inconsistent state, can cause a crash,
due to a derefence on the confirmation height which is unset.
Changelog-Fixed: wallet: Addressed a crash when attempting to spend a previously confirmed but reorged out close transaction output.
(cherry picked from commit 4c078a0e61c8eea7ec3549c71688808dcfe479f0)
What changed, and why it matters
This update fixes a crash in Core Lightning's wallet. If a channel-closing transaction had been confirmed and then a blockchain reorganization removed that confirmation, the wallet could try to spend the output while its confirmation height was missing. That led to a null-pointer crash. The fix treats such outputs as still time-locked so they are not spent until re-confirmed.
Apply the patch and run the new unit test. Nodes should upgrade to avoid denial-of-service crashes when a close transaction is reorganized out. Operators running nodes on chains with frequent reorgs are most affected.
Security signals we found
Null-pointer dereference crash on reorged close output
Missing validation of blockheight before dereference
Fix generalizes CSV lock to all unconfirmed close outputs
Regression test added for anchor and non-anchor cases
Evidence from the diff
The helper utxo_is_csv_locked() in common/utxo.h previously returned true for an unconfirmed close output only when option_anchors was set. For non-anchor close outputs with a NULL blockheight, execution fell through to an assertion and dereference of *utxo->blockheight, causing a SIGSEGV when the height had been cleared after a reorg. The patch makes any close output with a missing blockheight be treated as CSV-locked, regardless of option_anchors. A unit test covers the reorged-output case for both anchor and non-anchor channels.
Changed components
common/utxo.hwallet/test/run-wallet.cwallet UTXO selection / close-output spending pathInspect captured patch +52 / −5
### common/utxo.h
@@ -73,11 +73,10 @@ static inline bool utxo_is_csv_locked(const struct utxo *utxo, u32 current_heigh
{
if (!utxo->close_info)
return false;
- /* BOLT #3:
- * If `option_anchors` applies to the commitment transaction, the
- * `to_remote` output is encumbered by a one block csv lock.
- */
- if (!utxo->blockheight && utxo->close_info->option_anchors)
+ /* An unconfirmed close output is unavailable regardless of channel
+ * type: the wallet may retain close metadata (e.g. after a reorg)
+ * even when the block height is gone. */
+ if (!utxo->blockheight)
return true;
assert(*utxo->blockheight + utxo->close_info->csv > *utxo->blockheight);
return *utxo->blockheight + utxo->close_info->csv > current_height;
### wallet/test/run-wallet.c
@@ -2045,6 +2045,52 @@ static bool test_wallet_payment_status_enum(void)
return true;
}
+/* A reorged close output retains close_info but loses its blockheight.
+ * utxo_is_csv_locked() must treat that as locked for both anchor and
+ * non-anchor close outputs, and never dereference a missing height. */
+static bool test_utxo_csv_locked(void)
+{
+ struct utxo u;
+ struct unilateral_close_info ci;
+ u32 bh = 100;
+
+ memset(&u, 0, sizeof(u));
+ memset(&ci, 0, sizeof(ci));
+
+ /* Unconfirmed non-anchor close output: the SIGSEGV path before the
+ * fix; must be reported as locked without touching blockheight. */
+ ci.csv = 5;
+ ci.option_anchors = false;
+ u.close_info = &ci;
+ u.blockheight = NULL;
+ CHECK(utxo_is_csv_locked(&u, 100));
+
+ /* Unconfirmed anchor close output: already locked, keep that. */
+ ci.option_anchors = true;
+ CHECK(utxo_is_csv_locked(&u, 100));
+
+ /* No close metadata: never csv-locked, even unconfirmed. */
+ u.close_info = NULL;
+ CHECK(!utxo_is_csv_locked(&u, 100));
+
+ /* Confirmed non-anchor close output, csv 5: locked until height+5. */
+ ci.csv = 5;
+ ci.option_anchors = false;
+ u.close_info = &ci;
+ u.blockheight = &bh;
+ CHECK(utxo_is_csv_locked(&u, 100));
+ CHECK(utxo_is_csv_locked(&u, 104));
+ CHECK(!utxo_is_csv_locked(&u, 105));
+
+ /* Confirmed anchor close output, csv 1: locked until height+1. */
+ ci.csv = 1;
+ ci.option_anchors = true;
+ CHECK(utxo_is_csv_locked(&u, 100));
+ CHECK(!utxo_is_csv_locked(&u, 101));
+
+ return true;
+}
+
int main(int argc, const char *argv[])
{
common_setup(argv[0]);
@@ -2078,6 +2124,8 @@ int main(int argc, const char *argv[])
ld->channels_by_dbid = tal(ld, struct channel_dbid_map);
channel_dbid_map_init(ld->channels_by_dbid);
+ ok &= test_utxo_csv_locked();
+
/* We do a runtime test here, so we still check compile! */
if (HAVE_SQLITE3) {
for (int bip86 = 0; bip86 < 2; bip86++) {Why this scored 60/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.