← Watch feed
lightningd: don't assert on funding feerates read from the db
Public commit record
86/100 · Strong
What the developer wrote
Authored by cdecker
lightningd: don't assert on funding feerates read from the db
The 25/24 RBF escalation from BOLT #2 is computed on a u32 in three
places, and two of them assert that the result came out larger than the
input. It doesn't, for anything above UINT_MAX/25: the multiplication
wraps and the assert fires. That value comes out of
channel_funding_inflights, which is to say out of whatever fee estimator
(ours or a peer's) produced it, so it is not an invariant we were ever in
a position to state.
The worse of the two is in json_add_channel. listpeerchannels is
read-only introspection that plugins call at startup, so one bad row
turns into a crash-loop with no RPC left to repair it with.
check_funding_feerate() in dualopend already had the right shape for
this, so hoist it into bitcoin/feerate.c as next_funding_feerate() and
use it at all three sites rather than keeping three copies of the
calculation. It also rejects a last feerate of 0, which is the other end
of the same problem: assert(last_feerate > 0) sits immediately above the
overflow one.
That makes next_feerate a field we can fail to produce, so the schema no
longer requires it: a last feerate of 0, or one below 24 where rounding
down lands back on itself, admits no valid next feerate, and there is no
honest value to print for one. The bindings already modelled it that
way (Option<String> in cln-rpc, optional in the proto), so the required
list was the outlier.
The preceding commits bound every path a feerate arrives on, and the
migration repaired what was already stored, so the invariant does now
hold. This is the belt to their braces: a future unbounded write path
should not be able to turn straight back into a crash-loop, and finding
out about it in listpeerchannels is finding out too late. Since the
stored value is in range, the output is unchanged in every reachable
case. openchannel_bump fails the command rather than the daemon.
Changelog-Fixed: lightningd: `listpeerchannels` no longer aborts when a channel has an out-of-range funding feerate stored for an in-progress open.
Changelog-Changed: JSON-RPC: `listpeerchannels`.`next_feerate` is now optional: it is omitted for an in-progress open whose stored feerate admits no valid next feerate.
(cherry picked from commit 84443b55fc4a5cc2d6e3a557f2c58dca60f8f264)
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context✓ Links an issue, advisory, or supporting reference✓ Names security-relevant behavior explicitly
What changed, and why it matters
This update fixes a crash bug in Core Lightning. When the software tried to list details of a channel opening in progress, it could crash if a stored fee rate was extremely large or zero. The crash happened because the code used an internal sanity check (an 'assert') that treated bad stored data as an impossible condition. The fix replaces that hard crash with a safe failure: it either skips the optional 'next fee rate' field in the output, or returns an error for the specific command. This prevents a single bad database row from making the whole node crash repeatedly at startup.
Recommended action
Apply the patch. It is a defensive hardening fix that prevents crash-loops from malformed or out-of-range stored funding feerates. No immediate incident response is indicated unless the node has already entered a crash-loop, in which case the patch or the preceding migration commits are needed to restore service.
Security signals we found
01
Integer overflow in RBF escalation (u32 * 25 / 24) leading to assertion failure
02
Assertion failure in read-only introspection RPC (listpeerchannels) causing crash-loop at startup
03
Database value treated as invariant despite originating from external fee estimator
04
Replacement of asserts with explicit error handling and optional output
05
Schema change making next_feerate optional
Evidence from the diff
The commit removes three u32-overflow-prone 25/24 RBF escalation calculations and replaces them with a single helper, next_funding_feerate(), that widens the multiplication to u64 and returns false when no valid next feerate exists (last_feerate == 0, last_feerate > UINT_MAX/25, or last_feerate < 24). The most serious site was json_add_channel(), called by listpeerchannels, where an assert on the overflow result could crash lightningd at startup. The other sites are openchannel_bump (now returns JSONRPC2_INVALID_PARAMS instead of aborting) and dualopend’s check_funding_feerate (now uses the helper). The listpeerchannels schema is updated to make next_feerate optional, matching existing bindings.
Changed components
lightningd/peer_control.c (json_add_channel / listpeerchannels)lightningd/dual_open_control.c (json_openchannel_bump)openingd/dualopend.c (check_funding_feerate)bitcoin/feerate.c and bitcoin/feerate.h (new next_funding_feerate helper)doc/schemas/listpeerchannels.json and contrib/msggen/msggen/schema.jsonInspect captured patch +72 / −22
### bitcoin/feerate.c
@@ -38,6 +38,30 @@ u32 feerate_to_style(u32 feerate_perkw, enum feerate_style style)
abort();
}
+bool next_funding_feerate(u32 last_feerate, u32 *next_feerate)
+{
+ u64 next;
+
+ /* Not a feerate we could ever have proposed, and 25/24 of it is
+ * still 0. */
+ if (last_feerate == 0)
+ return false;
+
+ /* Widen: anything above UINT_MAX/25 overflows a u32 here, and that
+ * is exactly the range a broken fee estimator can leave in the db. */
+ next = (u64)last_feerate * 25 / 24;
+ if (next > UINT_MAX)
+ return false;
+
+ /* Rounding down means feerates below 24 map back onto themselves,
+ * and the rule requires strictly more to be a valid bump. */
+ if (next <= last_feerate)
+ return false;
+
+ *next_feerate = next;
+ return true;
+}
+
const char *feerate_style_name(enum feerate_style style)
{
switch (style) {
### bitcoin/feerate.h
@@ -80,4 +80,16 @@ u32 feerate_from_style(u32 feerate, enum feerate_style style);
u32 feerate_to_style(u32 feerate_perkw, enum feerate_style style);
const char *feerate_style_name(enum feerate_style style);
+/* Sets *next_feerate to the smallest feerate which satisfies the BOLT #2
+ * rule that the next funding transaction pays 25/24 times the feerate of
+ * the previously constructed one, rounded down, and returns true. Returns
+ * false, leaving *next_feerate untouched, if last_feerate admits no such
+ * value: it is 0, or 25/24 of it does not fit a u32, or rounding down lands
+ * back on last_feerate.
+ *
+ * last_feerate is generally read back out of the database, where a broken
+ * fee estimator (ours or a peer's) may have left something absurd, so
+ * callers must handle false rather than assume it away. */
+bool next_funding_feerate(u32 last_feerate, u32 *next_feerate);
+
#endif /* LIGHTNING_BITCOIN_FEERATE_H */
### contrib/msggen/msggen/schema.json
@@ -27005,7 +27005,7 @@
"next_feerate": {
"type": "string",
"description": [
- "For inflight opens, the next feerate we'll use for the channel open."
+ "For inflight opens, the next feerate we'll use for the channel open. Omitted if *last_feerate* admits no valid next feerate, which only a channel written by an older, unbounded version can do."
]
},
"next_fee_step": {
@@ -27971,8 +27971,7 @@
"additionalProperties": false,
"required": [
"initial_feerate",
- "last_feerate",
- "next_feerate"
+ "last_feerate"
],
"properties": {
"state": {},
@@ -28066,7 +28065,7 @@
"next_feerate": {
"type": "string",
"description": [
- "The minimum feerate for the next funding transaction in per-1000-weight, with `kpw` appended."
+ "The minimum feerate for the next funding transaction in per-1000-weight, with `kpw` appended. Omitted if *last_feerate* admits no valid next feerate."
]
}
}
### doc/schemas/listpeerchannels.json
@@ -360,7 +360,7 @@
"next_feerate": {
"type": "string",
"description": [
- "For inflight opens, the next feerate we'll use for the channel open."
+ "For inflight opens, the next feerate we'll use for the channel open. Omitted if *last_feerate* admits no valid next feerate, which only a channel written by an older, unbounded version can do."
]
},
"next_fee_step": {
@@ -1326,8 +1326,7 @@
"additionalProperties": false,
"required": [
"initial_feerate",
- "last_feerate",
- "next_feerate"
+ "last_feerate"
],
"properties": {
"state": {},
@@ -1421,7 +1420,7 @@
"next_feerate": {
"type": "string",
"description": [
- "The minimum feerate for the next funding transaction in per-1000-weight, with `kpw` appended."
+ "The minimum feerate for the next funding transaction in per-1000-weight, with `kpw` appended. Omitted if *last_feerate* admits no valid next feerate."
]
}
}
### lightningd/dual_open_control.c
@@ -3,6 +3,7 @@
* saves and funding tx watching for a channel open */
#include "config.h"
+#include <bitcoin/feerate.h>
#include <ccan/array_size/array_size.h>
#include <ccan/cast/cast.h>
#include <ccan/mem/mem.h>
@@ -2595,8 +2596,14 @@ json_openchannel_bump(struct command *cmd,
* down.
*/
last_feerate_perkw = channel_last_funding_feerate(channel);
- next_feerate_min = last_feerate_perkw * 25 / 24;
- assert(next_feerate_min > last_feerate_perkw);
+ /* Whatever is stored could be absurd, in which case there is no next
+ * feerate to bump to. Fail the command rather than the daemon. */
+ if (!next_funding_feerate(last_feerate_perkw, &next_feerate_min))
+ return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
+ "Can't calculate the next feerate: the"
+ " last funding feerate recorded for this"
+ " channel (%u) is out of range",
+ last_feerate_perkw);
if (!info->feerate_per_kw_funding) {
info->feerate_per_kw_funding = tal(info, u32);
*info->feerate_per_kw_funding = next_feerate_min;
### lightningd/peer_control.c
@@ -1052,26 +1052,35 @@ static void NON_NULL_ARGS(1, 2, 4, 5) json_add_channel(struct command *cmd,
initial = list_top(&channel->inflights,
struct channel_inflight, list);
json_add_string(response, "initial_feerate",
- tal_fmt(tmpctx, "%d%s",
+ tal_fmt(tmpctx, "%u%s",
initial->funding->feerate,
feerate_style_name(FEERATE_PER_KSIPA)));
last_feerate = channel_last_funding_feerate(channel);
- assert(last_feerate > 0);
json_add_string(response, "last_feerate",
- tal_fmt(tmpctx, "%d%s", last_feerate,
+ tal_fmt(tmpctx, "%u%s", last_feerate,
feerate_style_name(FEERATE_PER_KSIPA)));
/* BOLT #2:
* - MUST set `feerate` greater than or equal to 25/24
* times the `feerate` of the previously constructed
* transaction, rounded down.
*/
- next_feerate = last_feerate * 25 / 24;
- assert(next_feerate > last_feerate);
- json_add_string(response, "next_feerate",
- tal_fmt(tmpctx, "%d%s", next_feerate,
- feerate_style_name(FEERATE_PER_KSIPA)));
+ /* The bounds and the migration together keep last_feerate in
+ * range, so this holds; we used to assert it. But this is
+ * read-only introspection that plugins call at startup, and
+ * aborting here would turn one bad row into a crash-loop with
+ * no RPC left to repair it with. */
+ if (next_funding_feerate(last_feerate, &next_feerate)) {
+ json_add_string(response, "next_feerate",
+ tal_fmt(tmpctx, "%u%s", next_feerate,
+ feerate_style_name(FEERATE_PER_KSIPA)));
+ } else {
+ log_broken(channel->log,
+ "Funding feerate %u leaves no valid next"
+ " feerate: omitting next_feerate",
+ last_feerate);
+ }
/* List the inflights */
json_array_start(response, "inflight");
@@ -1086,7 +1095,7 @@ static void NON_NULL_ARGS(1, 2, 4, 5) json_add_channel(struct command *cmd,
json_add_num(response, "funding_outnum",
inflight->funding->outpoint.n);
json_add_string(response, "feerate",
- tal_fmt(tmpctx, "%d%s",
+ tal_fmt(tmpctx, "%u%s",
inflight->funding->feerate,
feerate_style_name(
FEERATE_PER_KSIPA)));
### openingd/dualopend.c
@@ -3428,10 +3428,10 @@ static bool check_funding_feerate(u32 proposed_next_feerate,
* - the `feerate` is not greater than or equal to 25/24 times `feerate`
* of the last successfully constructed transaction
*/
- u32 next_min = last_feerate * 25 / 24;
+ u32 next_min;
- if (next_min < last_feerate) {
- status_broken("Overflow calculating next feerate. last %u",
+ if (!next_funding_feerate(last_feerate, &next_min)) {
+ status_broken("Can't calculate next feerate. last %u",
last_feerate);
return false;
}Risk score
Our methodology →Why this scored 64/100
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.