feerates: bound peer-supplied and stored values (v26.06.7 security, 1/7) - #9507
Merged
Merged
Conversation
This saves sats in certain scenarios by preferring a lower closing fee. Changelog-Changed: lightningd: the maximum fee accepted for a mutual close is now bounded by the unilateral-close feerate. (cherry picked from commit fc646ea)
As a final check before sending any signed mutal close, we confirm the fee remains below our max fee to accept. Changelog-None (cherry picked from commit 0aae0d5)
Nothing bounded the feerates we accept from the `estimatefees` plugin: `parse_feerate_ranges` only checks that block counts ascend and that rates do not increase with block count, and the conversion loop only applies a floor. A broken or misbehaving fee source could therefore push an arbitrary u32 into every downstream feerate calculation. Clamp both the reported floor and each individual rate to FEERATE_CEILING once they have been converted to perkw, and log loudly when we do. We clamp rather than reject because `bitcoin_plugin_error` is fatal(), and because discarding the estimate entirely is worse than capping it: with no rates at all `feerate_max` falls into its "unknown" path and returns UINT_MAX. The ceiling is deliberately not a policy limit. It sits at 4000 sat/vB, several times the highest the chain has ever seen, so that it only ever fires on a broken fee source. Picking a tighter bound would mean underestimating during genuine congestion, which is exactly when our unilateral close and HTLC timeout transactions need to confirm. The conversion has to be widened for the ceiling to see anything at all. feerate_from_style() computed (feerate + 3) / 4 on a u32, so the top three perkb values wrapped to 0 or 1 perkw, slipped under the ceiling, and were then raised to the floor, hiding the whole thing. That is also the more dangerous direction, since it underpays our own unilateral close, and it applies equally to operator input like `feerate="4294967295perkb"`. feerate_to_style() right below already widened for the same reason; the asymmetry was an oversight. Changelog-Fixed: lightningd: feerate estimates from the Bitcoin backend are now clamped to a sanity ceiling, so a misbehaving `estimatefees` plugin cannot feed absurd feerates into the rest of the daemon. Changelog-Fixed: lightningd: a feerate given in perkb near the top of the 32-bit range no longer wraps to a near-zero perkw feerate. (cherry picked from commit 2d80c3b)
added 5 commits
September 15, 2026 08:40
The previous commit clamped our own fee source on the way in. This is the counterpart for the other side of the channel: our peer's estimator can be just as broken as ours, and we are the only one in a position to notice before their money is gone. `splice_accepter` only rejected a remote `funding_feerate_perkw` below feerate_min; there was no upper bound at all. The downstream safeguard in check_balances only inspects the accepter's own fee contribution, and since accepter_relative is 0 our weight is nil, so that check never fires. A peer whose estimator has gone haywire can therefore drive a splice at an arbitrary feerate and we will cheerfully sign it, even though the fee comes out of their channel balance. Refusing costs us nothing and saves them from committing their funds to a transaction they did not mean to pay for. Reject anything above feerate_max, and apply the same bound in handle_splice_init so that we extend the same courtesy in reverse: if our own estimator is the broken one, we catch it before we ask a peer to sign for it. handle_splice_init honours `splice_force_feerate` there, as check_balances already does: that flag is the user saying they meant it, and a policy limit should not override it. feerate_max also needs a bound of its own, since it was derived purely from the estimates times config.max_fee_multiplier. But a single bound cannot serve both directions, because feerate_max gates what we accept in `update_fee` *and* what we propose ourselves. Set for the accept side it would have us paying absurd fees; set for the propose side it would pin our commitment feerate below what a congested chain demands, which is precisely when it has to confirm. So there are now two: - FEERATE_CEILING (1000000 perkw, 4000 sat/vB) is the most we let a peer drive us to. Their estimator being broken is not by itself worth dropping a channel over, so this only has to exclude the absurd. - MAX_OUR_FEERATE_PER_KW (100000 perkw, 400 sat/vB) is the most we will pay: roughly 0.011 BTC for a bare anchor commitment. Declining to propose a feerate costs us nothing, so we can afford to be strict. our_feerate_max() applies it wherever the money is ours: default_feerate, splice_feerate, opening_feerate and dual-fund RBF. Note this leaves the paths where we cannot decline -- mutual close, penalty and onchain resolution -- bounded only by the ceiling, which is the right answer for a feerate that has a deadline attached. channeld needs both, so channeld_init and channeld_feerates carry our_feerate_max alongside feerate_max, and check_balances holds whichever side is ours to the stricter one. The multiplier is settable via --dev-max-fee-multiplier, so widen to u64 before multiplying. Finally, remove the UINT_MAX sentinels. feerate_max returned UINT_MAX when there were no estimates at all, and lightningd overwrote the bound with 0xFFFFFFFF when fee limits were ignored. Both are the bound a peer's proposal is measured against, and we store what we accept -- a u32 that large overflows the 25/24 RBF calculation downstream. The unknown path now falls back to FEERATE_CEILING, exactly as feerate_min falls back to FEERATE_FLOOR, and ignore_fee_limits travels to channeld and openingd as an explicit flag which relaxes the policy bounds but never the ceiling. That last part makes a documented promise false, so the three places which say ignore-fee-limits means "any fee they want" now say what it actually means. Changelog-Fixed: channeld: a peer can no longer initiate a splice at an arbitrarily high feerate; it is now bounded like other remote-proposed feerates. Changelog-Changed: lightningd: we no longer propose a feerate above 100000perkw (400 sat/vB) of our own accord, for channel opens, splices, commitment updates and dual-fund RBFs. Changelog-Changed: lightningd: `--ignore-fee-limits` and `setchannel`'s `ignorefeelimits` still drop our policy bounds, but no longer accept a feerate above 1000000perkw (4000 sat/vB). Changelog-Changed: JSON-RPC: `feerates` reports `max_acceptable` as the feerate ceiling rather than 4294967295 when no fee estimates are available. (cherry picked from commit 4a10d71)
The previous commits bounded the feerates arriving on every path except this one. `accepter_start()` takes both `funding_feerate_perkw` and `feerate_per_kw_commitment` straight off the wire from `open_channel2` with no bound in either direction, and `dualopend_wire.csv` had no min_feerate/max_feerate on `dualopend_init` at all -- so it is not that the check was forgotten at the call site, it is that the bounds were never plumbed to the daemon. The `openchannel2` hook only *reports* `feerate_our_max`/`feerate_our_min` to plugins. With no plugin hooked nothing enforces them, so a peer could name any feerate and we would sign for it and store it. This is the `open_channel2` plant named in the advisory, and the missing lower bound is where the stored `feerate == 0` comes from. Plumb the bounds into dualopend_init and dualopend_reinit -- reinit too, since rbf_remote_start runs after a reconnect -- and apply them where the values are parsed. The two feerates get different floors: - The commitment feerate is held to feerate_min, as openingd holds open_channel's: too low and the commitment we are signing cannot be relayed at the point we need it. - The funding feerate is held only to FEERATE_FLOOR. A slow funding tx is the opener's problem and RBF is the remedy, so our policy minimum would refuse perfectly good opens; but 0 is not a feerate, and it is exactly the value that leaves no valid next RBF step. The RBF path needs the upper bound separately: check_funding_feerate() only enforces the 25/24 step *upwards*, so without this a peer can walk an accepted open's feerate up without limit, one RBF at a time. rbf_local_start() is left alone deliberately: that feerate comes from our own openchannel_bump, which lightningd already holds to our_feerate_max. As with channeld and openingd, ignore_fee_limits relaxes the policy bounds but never FEERATE_CEILING. Changelog-Fixed: dualopend: a peer opening a channel with `--experimental-dual-fund` can no longer name an arbitrary funding or commitment feerate; both are now bounded as other remote-proposed feerates are. (cherry picked from commit 4e653ad)
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 ElementsProject#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 0393174)
The 25/24 RBF escalation from BOLT ElementsProject#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 84443b5)
Three tests, one per way an absurd feerate used to get in or blow up. test_feerate_ceiling drives the fee source: bcli trims anything wider than a u32 of perkb to exactly 0xFFFFFFFF, which is also the value whose +3 wrapped in the perkb->perkw conversion, so a fee source producing an absurdly *high* feerate used to arrive as 0perkw and get quietly raised to the floor. It fails without both the widened conversion and the ceiling. test_splice_stored_feerate_repaired_on_upgrade plants the poisoned value on an inflight and then rewinds the db version so the clamping migrations run over it, which is the upgrade an already-attacked node goes through. Parametrized over the overflow value in both the form lightningd itself would have stored it (db_bind_int makes it negative) and the form you get writing it by hand, plus the zero case and a merely-large one. Asserts both that the row is repaired and that listpeerchannels -- which used to abort right here -- agrees. test_splice_feerate_too_high covers the accepter bound, which did not previously exist: force_feerate gets us past our own check on what we will pay, leaving the peer's bound as the thing under test. The RPC blocks because the splice_ack never comes, hence the thread. Changelog-None (cherry picked from commit a609189)
nGoline
force-pushed
the
port-26.06.7-feerates
branch
from
September 15, 2026 12:52
bc0d510 to
91b6031
Compare
Andezion
approved these changes
Sep 15, 2026
jaonoctus
approved these changes
Sep 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the first of seven PRs forward-porting the v26.06.7 security point release onto master. v26.06.7 is publicly a security release, and these are forward-ports.
Feerates reaching us from three directions are now bounded: what a peer proposes, what the Bitcoin backend's
estimatefeesreports, and what an older database already stored.8 commits:
lightningd: Target a lower closing feeclosingd: Add a sanity check for feeratelightningd: clamp estimatefees results to a sanity ceilingchanneld: split the feerate we'll pay from the one we'll acceptdualopend: bound the feerates a peer opens atwallet: clamp absurd inflight funding feerates on upgradelightningd: don't assert on funding feerates read from the dbtests: regression coverage for out-of-range feeratesEleven
Changelog-entries, sixFixedand fiveChanged. Worth calling out in particular:--ignore-fee-limitsandsetchannel'signorefeelimitsstill drop our policy bounds, but no longer accept a feerate above 1000000perkw (4000 sat/vB).feeratesreportsmax_acceptableas the ceiling rather than 4294967295 when no estimates are available.listpeerchannels.next_feeratebecomes optional.For reviewers
The DB migration is the part to look at.
wallet: clamp absurd inflight funding feerates on upgradeis the only commit in the whole 54-commit series that toucheswallet/migrations.c. Master has added four migrations since the release line's merge base, so the two clamps are appended to the end of master's current table rather than inserted at their release-line position. They sit inside the open/* ^v26.09 */block, which is the judgement call worth a second opinion. Both are idempotentUPDATEs with no revert.This PR gates four others. G2, G3, G5 and G7 each touch files this one touches. G4 and G6 are independent of it.
Two commits here also carry documentation wording that the binary release deliberately held back (
doc/schemas/setchannel.json,doc/schemas/listpeerchannels.json,doc/lightningd-config.5.md,doc/getting-started/getting-started/configuration.md, plus the generatedcontrib/msggen/msggen/schema.json). Master has never had either wording, so this is a choice of which text the ported commits carry, not a later revert. The restored text explains that reaching the ceiling means a broken fee estimator rather than a busy mempool, which is the more useful message for anyone who hits it.Not in this series
502b664d9 common: check value overflow for forward amountsis not ported: master already has the fix as96f026ecc. The release's test is about 15 lines larger than master's. Reconciling that is a separate follow-up.