wallet: clamp absurd inflight funding feerates on upgrade
What the developer wrote
Authored by cdecker
wallet: clamp absurd inflight funding feerates on upgrade
The bounds in the previous commits only apply to feerates arriving from
now on. Nodes that already recorded an absurd `funding_feerate` for an
inflight splice or dual-fund RBF are stuck: the BOLT #2 rule that the
next RBF pays 25/24 times the last feerate is computed on a u32, so
anything above UINT_MAX/25 overflows and trips the
assert(next_feerate > last_feerate) in listpeerchannels. Plugins call
listpeerchannels at startup, so such a node crash-loops with no way out
but to rewrite the stored value.
Clamp it on upgrade. Note we write the feerate with db_bind_int(), so a
u32 above INT_MAX reads back negative; those are exactly the values that
overflow, hence the second clause.
Rewriting is safe: this feerate only sanity checks the fee the funding
transaction already pays and tells the user what the next RBF has to
beat. It never feeds anything we have signed, and lowering it only
relaxes the check.
Changelog-Fixed: lightningd: nodes that stored an out-of-range feerate for an in-progress splice no longer crash-loop on `listpeerchannels`; the stored feerate is clamped on upgrade.
(cherry picked from commit 0393174cb312079da1e0d272640cac145ccdf4c7)
What changed, and why it matters
This commit fixes a bug where Core Lightning nodes could get stuck in a crash loop. If a node had previously stored an extremely high or zero fee rate for an in-progress channel funding operation (a 'splice' or dual-funded channel RBF), a later code check would overflow or fail an assertion when listing peer channels. Since plugins call that list at startup, the node would keep crashing. The fix adds a database upgrade step that clamps those bad stored values to safe limits when the node starts up after updating.
Apply the patch and ensure nodes are upgraded so the migration runs before any plugin triggers listpeerchannels. Operators of nodes that have done splicing or dual-funded RBF should upgrade promptly to avoid crash loops. No manual database editing is needed after this migration runs.
Security signals we found
Integer overflow in fee-rate calculation (u32 overflow when multiplying by 25/24)
Assertion failure leading to daemon crash loop at startup
Database migration clamps out-of-range stored funding feerates
Negative stored values caused by signed/unsigned mismatch (db_bind_int vs u32)
Crash triggered by plugin calls to listpeerchannels during startup
Evidence from the diff
The patch adds a database migration in wallet/migrations.c that clamps invalid values in channel_funding_inflights.funding_feerate. Values above 1,000,000 or negative (caused by storing a u32 above INT_MAX via db_bind_int()) are set to 1,000,000. Values of 0 or NULL are set to 253. This prevents u32 overflow in the BOLT #2 rule next_feerate = last_feerate * 25 / 24 and avoids the assertion failure assert(next_feerate > last_feerate) in listpeerchannels, which was triggered at startup by plugins and caused crash loops. The commit notes that the preceding commits prevent new bad values from being stored; this migration repairs existing stored values.
Changed components
wallet/migrations.cchannel_funding_inflights database tablelistpeerchannels RPC pathinflight splice and dual-funding RBF logicInspect captured patch +34 / −0
### wallet/migrations.c
@@ -1186,6 +1186,40 @@ static const struct db_migration dbmigrations[] = {
* after the failure was recorded (issue #9341). */
{SQL("ALTER TABLE payments ADD failmsg BLOB;"), NULL,
SQL("ALTER TABLE payments DROP COLUMN failmsg"), NULL},
+ /* Nothing used to bound the feerate we record for an inflight funding
+ * transaction, so a broken fee estimator (ours or a peer's) could get an
+ * absurd value in here. That is not merely cosmetic: the BOLT #2 rule
+ * that the next RBF attempt pays 25/24 times the last feerate is computed
+ * on a u32, so anything above UINT_MAX/25 overflows and trips the
+ * assert(next_feerate > last_feerate) in listpeerchannels, which plugins
+ * call at startup: the node crash-loops with no way out but to rewrite the
+ * stored value. A stored 0 trips the assert immediately above it.
+ *
+ * The preceding commits close every path such a value could arrive on.
+ * This repairs what is already there, so that from here the bounds hold
+ * for stored feerates too and the read path can rely on them.
+ *
+ * We write the feerate with db_bind_int(), so a u32 above INT_MAX reads
+ * back negative; those are exactly the values that overflow, hence the
+ * second clause below.
+ *
+ * The bounds are spelled out rather than written as FEERATE_CEILING and
+ * FEERATE_FLOOR on purpose: a migration has to keep doing exactly what it
+ * did on the day it shipped, so it must not move when those constants do.
+ *
+ * Rewriting is safe: this feerate only sanity checks the fee the funding
+ * transaction already pays, and tells the user what the next RBF must
+ * beat. It never feeds anything we have signed. */
+ {SQL("UPDATE channel_funding_inflights"
+ " SET funding_feerate = 1000000"
+ " WHERE funding_feerate > 1000000 OR funding_feerate < 0;"), NULL,
+ /* Clamping is idempotent, so no revert needed */
+ NULL, NULL},
+ {SQL("UPDATE channel_funding_inflights"
+ " SET funding_feerate = 253"
+ " WHERE funding_feerate = 0 OR funding_feerate IS NULL;"), NULL,
+ /* Clamping is idempotent, so no revert needed */
+ NULL, NULL},
/* ^v26.09 */
};
Why this scored 53/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.