funding+lnwallet: reject absurdly high channel feerates - #11151
Conversation
bc34fc1 to
df165b4
Compare
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (4 files)
🟢 Low (3 files)
AnalysisThis PR modifies Excluding test files and docs, the change touches 4 non-test Go files across two distinct critical packages ( To override, add a |
In this commit, we add the BOLT-02 check that the receiver of open_channel must fail the channel if both to_local and to_remote on the initial commitment are less than or equal to the channel reserve. If neither side starts out above its reserve, then neither side can ever add an HTLC without dipping below it, so the channel is dead on arrival. We run the check in handleSingleContribution rather than in NewChannelReservation, as that's the first point where both reserves are known: the one the initiator imposes on us arrives in open_channel, while the one we impose on them is only settled once the channel acceptor (which may override our default) has had its say.
In this commit, we add the other half of the BOLT-02 requirement here: the receiver MUST fail the channel if it considers feerate_per_kw unreasonably large. The spec leaves the threshold up to us, so we take the same approach as CLN and Eclair and cap at 10x our own estimate for the same conf target (3) we'd use for a channel we open ourselves. A pure multiple of our estimate is a bit brittle on its own: on a quiet chain, or when the estimator falls back to the relay fee, 10x of a very small number is still a very small number, and we'd start turning away channels opened at a rate that's perfectly sane in absolute terms. So we floor the bound at 100 sat/vbyte, which no realistic mempool needs a commitment to beat. If the estimator errors out entirely we fall back to that floor rather than failing the flow. Since all channels are single funder today, an initiator that proposes an absurd rate mostly burns its own capacity. But it doesn't only do that: the open still costs us a pending channel slot, and left unchecked it lands us with a confirmed channel that can never be used and has to be closed on chain. Rejecting up front is cheaper for both of us.
df165b4 to
5730887
Compare
Lrifton92
left a comment
Lrifton92
left a comment
There was a problem hiding this comment.
Reviewed the two BOLT-02 checks end to end — logic looks correct and the wiring is in the right place. Nice touch flooring the feerate bound at minCommitFeeRateCap and falling back to it on an EstimateFeePerKW error in maxRemoteCommitFeeRate; that neatly avoids the 10x-a-relay-fee-fallback false-reject on a quiet chain, and the sat/kw conversion + uint32 feerate mean neither the cast nor the x10 can overflow.
One thing worth spelling out for future readers: validateInitialBalances overlaps the existing !initiator && theirBalance <= 2*defaultDust guard in NewChannelReservation (reservation.go:386), which fires earlier and returns ErrFunderBalanceDust. So ErrBalancesBelowReserve only surfaces in the (2*dust, reserve] band — which does cover the reporter's 9k-sat example, so it's additive, but for the exact feerate_per_kw=2_750_000 case the feerate cap trips first and you'll see ErrCommitFeeRateTooLarge instead. Might be worth a sentence in the PR body clarifying the layering of the three error paths.
Also: the reserve check is only invoked from handleSingleContribution, not the dual-funder handleContributionMsg path. Correct for single-funder-only inbound today, but might deserve a // TODO when v2 inbound lands. Non-blocking — LGTM.
2 participants
Fixes #11149.
In this PR, we add the two BOLT-02 checks that bound the commitment feerate a
channel initiator can impose on us. Today we implement neither, so a peer can
open a channel at a
feerate_per_kwso high that the initial commitment leavesboth sides at or under their channel reserve, i.e. a channel that can never be
used. The reporter's example: 2 BTC of capacity at
feerate_per_kw = 2_750_000lands
to_localat 9k sat andto_remoteat 0, against a 20k sat reserve.Worth being clear on the framing, since it came up in the issue: all channels
are single funder today, so the initiator proposing an absurd rate is mostly
burning its own capacity. It isn't purely self-harm though. The open still costs
us a pending channel slot on the way in, and if we let it through we end up with
a confirmed channel that can never route and has to be closed on chain. So this
is spec compliance plus a bit of cheap resource hygiene, not a fund-loss bug.
The reserve check
lnwalletnow fails the reservation when bothto_localandto_remoteon theinitial commitment are less than or equal to the reserve each side has to
maintain. The check lives in
handleSingleContributionrather than inNewChannelReservation, as that's the first point where both reserves areknown: the one the initiator imposes on us rides in on
open_channel, while theone we impose on them is only settled once the channel acceptor (which can
override our default) has had its say.
The feerate cap
BOLT-02 leaves "unreasonably large" up to each implementation. We take the same
approach as CLN
and Eclair
and cap at 10x our own estimate, sampled at the same conf target (3) we'd use
for a channel we open ourselves.
A pure multiple of our estimate is brittle on its own: on a quiet chain, or when
the estimator falls back to the relay fee, 10x of a very small number is still a
very small number, and we'd start turning away channels opened at a rate that's
high relative to our estimate yet perfectly sane in absolute terms. So the bound
is floored at 100 sat/vbyte, which no realistic mempool needs a commitment to
beat. If the estimator errors out entirely we fall back to that floor rather
than failing the flow over a transient hiccup.
Note that this shouldn't disturb anchor channels either way: we already cap our
own anchor commitments at
--max-commit-fee-rate-anchors(10 sat/vbyte bydefault), well under the floor.
Follow-up
The new check reports its error over
req.errto match the code around it.That channel-of-errors pattern in
lnwallet/wallet.gois due for a pass tofn.Future, same as we did in the gossiper, but I've kept that out of here sothe baseline behavior lands on its own.