openingd: fail open_channel at receipt when both initial balances <= their reserve - #9480
openingd: fail open_channel at receipt when both initial balances <= their reserve#9480Amperstrand wants to merge 5 commits into
Conversation
Andezion
left a comment
Andezion
left a comment
There was a problem hiding this comment.
What do you think, do we need a test for this change? contrib/pyln-proto/pyln/proto/wire.py already implements a BOLT8 wire client and is already used for raw-wire tests in tests/test_connection.py (test_connect_basic), so a from-scratch ~~300-line wire peer isntt really necessary
| *funder_pay = AMOUNT_MSAT(0); | ||
|
|
||
| return amount_msat_greater_sat(*funder_pay, their_reserve) | ||
| || amount_msat_greater_sat(*accepter_pay, their_reserve); |
There was a problem hiding this comment.
On the two return true paths (anchor-fee overflow, funding -> msat overflow), *funder_pay/*accepter_pay are left uninitialized. Its not currently exploitable - the caller only reads them inside the if (!initial_balances_exceed_reserve(. . .)) branch, which is never taken when the function returns true - but its a silent invariant a future refactor could break
| * the two 330-sat anchor outputs come off the funder); the accepter's | ||
| * to_remote is push. Returns false and fills the (saturating) balances | ||
| * if NEITHER exceeds their channel_reserve_satoshis. */ | ||
| static bool initial_balances_exceed_reserve(struct amount_sat funding_sats, |
There was a problem hiding this comment.
in common/initial_commit_tx.h we have try_subtract_fee(REMOTE, REMOTE, base_fee, &funder_pay, &accepter_pay) (fundee is always LOCAL, peer/funder always REMOTE here). Maybe we can use it instead?
|
We recently found a similar spec compliance issue in #9491. IMO it would also be worth adding that missing check in this PR. |
…their reserve BOLT ElementsProject#2 requires the receiving node to fail the channel if both to_local and to_remote of the initial commitment transaction are <= the opener's channel_reserve_satoshis (a receiving-node MUST under open_channel receipt handling). CLN implements the comparison, but in initial_commit_tx() (common/initial_commit_tx.c, whose FIXME says it should be in ElementsProject#2), so it only fires at funding_created receipt -- after accept_channel has already gone out. Project the initial balances at open_channel receipt (funder to_local = funding - push - base fee - 2x330 anchor outputs; accepter to_remote = push) and fail the negotiation before accept_channel is sent, using the same fee math as initial_commit_tx() (commit_tx_base_fee + the 660-sat anchor correction). The misplaced check stays as the authoritative backstop at funding_created. An in-suite test would need a raw-wire opener: a stock fundchannel reserve is pre-checked with the reserve doubled ('Not opening because if they used the same setting as us ... below 10000sat'), which blocks every shape that trips this check. Validated with a BOLT8 wire peer driving the reporter's exact parameters (100k funding, 20k push, 87k reserve: pre-fix accept_channel, post-fix rejection citing the projected balances 78778000msat / 20000000msat). Changelog-Fixes: ElementsProject#9475 Fixes: ElementsProject#9475 Signed-off-by: Amperstrand <amperstrand@localhost>
…the initial commitment fee BOLT ElementsProject#2 requires the receiving node to fail the channel when the funder's amount for the initial commitment transaction is not sufficient for full fee payment (ElementsProject#9491). CLN implements the rule in initial_commit_tx(), so like the reserve check it only fires at funding_created receipt, after accept_channel has gone out -- and a full push (push_msat = funding_satoshis * 1000) sails past the reserve projection added for ElementsProject#9475, because the accepter's balance exceeds the reserve while the funder is left at zero. Fold the check into the same open_channel receipt projection: deduct push first, then try_subtract_fee(REMOTE, REMOTE, ...) for the base fee (with the 660-sat anchor correction), failing with the backstop's exact wording when the funder comes up short. The projected balances are now filled on every path, and the fee deduction reuses try_subtract_fee() from common/initial_commit_tx.h instead of hand-rolled saturating arithmetic. Adds in-suite raw-wire tests on the pyln-proto LightningConnection (same pattern as test_open_channel_funding_above_max_supply): the full-push rejection from ElementsProject#9491, one-msat fee boundaries on both the anchors and static_remotekey weight paths, and the reserve boundary from ElementsProject#9475. All three reject on stock (and the fee pair on the reserve-only parent) and pass here. Changelog-Fixes: ElementsProject#9491 Fixes: ElementsProject#9491 Signed-off-by: Amperstrand <amperstrand@localhost>
2b2f5f2 to
b563df6
Compare
The fee helper used ceiling division while amount_tx_fee() truncates (fee_per_kw * weight / 1000), so it is one sat over whenever the product is not a multiple of 1000 -- the one-msat boundary cells go stale on any runner whose opening feerate does not divide evenly (a boundary-matrix sweep against the built node caught it at feerate 1875: 2107.5 -> 2107). Also assert the projected balances in order in the reserve test's error message, so a swap of the two amounts cannot pass silently (this was the one survivor of a seven-mutation kill matrix). Changelog-None: test-only Signed-off-by: Amperstrand <amperstrand@localhost>
CI runs the suite with TEST_NETWORK=liquid-regtest too, where
commit_tx_base_fee() carries elements_tx_overhead() (610 extra
weight units on the anchors shape, 470 on static_remotekey) -- the
tests' boundary arithmetic is Bitcoin-weight only, so the exact-afford
cells would mispredict by kilosats and fail spuriously. Same guard
the suite already uses for fee math ("Fee computation and limits are
network specific"); the full-push rejection test stays unguarded
(fee-independent) so the new checks still run on the liquid arm.
Changelog-None: test-only
Signed-off-by: Amperstrand <amperstrand@localhost>
The raw-wire open_channel tests built chain_hash as getblockhash(0) reversed -- right on bitcoin networks, wrong on liquid-regtest, where CLN's elements chainparams store the genesis hash in display byte order (bitcoin/chainparams.c) and the node answers "Unknown chain-hash". test_open_channel_funding_above_max_supply passed there only vacuously (any rejection satisfies it); the new full-push test needs the right hash AND the specific error, which is what surfaced this. wire_chain_hash() picks the form per network, and all four raw-wire call sites use it -- the receipt checks now run on the liquid arm for real (validated against elementsd 23.2.1, the CI pin, and the just-released 23.3.4). Changelog-None: test-only Signed-off-by: Amperstrand <amperstrand@localhost>
|
@morehouse Done — folded in as a second commit (b563df6). The projection already had the base fee, so the check is |
|
@Andezion Test added, on the in-repo pyln-proto client as you suggested — the existing Sorry for the SHA churn on the earlier push — I re-signed the first commit for DCO and that detached your inline threads; same tree, message-only change. Updates from here are additive commits. Three test-only commits followed your review (fee-helper rounding, elements skip, chain-hash for liquid-regtest), mapped in the description. Re-requesting your review. |
|
One follow-up to the earlier replies: |
3 participants
BOLT #2 requires the receiver of
open_channelto fail the channel in two cases CLN only checks later, ininitial_commit_tx()atfunding_created— afteraccept_channelhas already gone out:to_localandto_remoteof the initial commitment transaction are <=channel_reserve_satoshis(Failure to reject absurdly high channel reserves #9475)open_channelwhere funder cannot pay commitment fee #9491)This projects the initial balances at
open_channelreceipt (funderto_local= funding − push, minus the base fee and the 2×330-sat anchors viatry_subtract_fee(REMOTE, REMOTE, …); accepterto_remote= push) and fails the negotiation beforeaccept_channelis sent, with the same wording theinitial_commit_tx()backstop uses. The backstop stays as the authority atfunding_created.Commits:
a842468ff— the Failure to reject absurdly high channel reserves #9475 reserve check.b563df679— the Failure to rejectopen_channelwhere funder cannot pay commitment fee #9491 fee check (a full push sails past the reserve projection because the accepter's balance exceeds the reserve while the funder is left at zero) plus the review cleanups (out-params filled on every path,try_subtract_feereuse).d732d395a,3c6e5195f,e5abc004d— test hardening: the fee helper's truncating division now matchesamount_tx_fee(latent CI flake at non-dividing feerates), the reserve test pins the message order, the raw-wire helpers carry the network-correctchain_hash, and the fee-boundary tests skip on elements networks.Tests (tests/test_connection.py, on the existing pyln-proto
raw_peer_connect/send_open_channelhelpers): the #9491 full-push rejection, one-msat fee boundaries on the anchors and static_remotekey weight paths, and the #9475 reserve boundary — each rejects on stock, and the fee pair also rejects on commit 1 alone.