lightningd: mutual close no longer falls back to broadcasting the commitment by ksedgwic · Pull Request #9516 · ElementsProject/lightning

ksedgwic · 2026-09-14T23:08:28Z

When lightningd rejects the closing fee closingd negotiated, closingd
never hears about it: the reply carries only a txid. closingd reports
"We agreed", the close completes, and drop_to_chain broadcasts
last_tx, still the commitment, while close returns type mutual. Two
things make lightningd reject a fee closingd negotiated in good faith,
and both are deterministic.

Rounding. The outputs are rounded down to whole satoshis, so the
transaction pays up to one satoshi more than the agreed fee. The
maximum bound added in 26.06.7 (fc646ea) compares that against a
ceiling which, with estimates at the floor, equals the agreed fee.
Fix: bound the fee closingd negotiated, derived from our output.
The close feerange minimum. closingd is given the range minimum as
its floor, but the acceptance check used the floor derived from the
fee estimates, so a range below the estimates was rejected as too
low. Fix: the acceptance check uses the range minimum as well.
The fall-through itself. The reply now carries the verdict; on a
rejection closingd warns the peer and exits instead of agreeing, and
the close command's timeout decides when to close unilaterally.
--dev-reject-closing-fee forces a rejection for the test.

Each fix is preceded by a regression test marked xfail, removed by the
fix. Based on release-26.06.7 because the maximum bound is not on
master yet; happy to rebase onto master once the release branch is
merged.

vincenzopalazzo

Concept ACK — a few nits below, none blocking.

Production Readiness: GO

The three commits match the three ways this fires. Silent ACK is from 2018, fc646eaba in v26.06.7 made the rounding case deterministic.

BTW this needs to land on master too: the fallthrough and the feerange min mismatch are still there even without the max bound.

just a FYI that you will find probably later with the CI... pre-build died on check-wire-format. I think is release-branch vs bolts, not this diff.

Code Review

Positive Aspects

Commit split is how this should be done: xfail then the fix. 60fda9082 is the actual safety fix.
Bounding the negotiated fee for max and the tx fee for min is the right split.

Issues & Suggestions

Inline comments capture the specifics. None blocking.

Summary

Concept ACK. Nothing blocking.

vincenzopalazzo · 2026-09-15T01:43:31Z

+		if (!tell_master_their_offer(&their_sig, tx, closing_txid))
+			peer_failed_warn(pps, channel_id,
+					 "Closing fee %s is outside our fee limits",
+					 fmt_amount_sat(tmpctx, received_fee));


Medium: we already sent closing_signed before asking master.

If lightningd says no, we warn and disconnect, but the peer has our sig and can still broadcast. Test puts --dev-reject-closing-fee on the non-opener so it does not show.

After the other two fixes this should not happen in practice — leftover, not blocking. Maybe just a comment here?

Agreed. Once the opener's closing_signed is out, a peer whose fee matched holds both signatures and can broadcast regardless; the reject only stops closingd from recording the close as agreed. The BOLT quote is now a comment saying exactly that: fddb967.

vincenzopalazzo · 2026-09-15T01:43:31Z

+		/* BOLT #2:
+		 *   - if the message contains a `fee_range`:
+		 *     - if there is no overlap between that and its own `fee_range`:
+		 *       - SHOULD send a warning
+		 */


Cleanup: this BOLT quote is the wrong rule (no fee_range overlap). What happens here is "master said no".

do_quickclose already has the real one. Can we drop this quote?

Dropped in fddb967, same hunk as the comment above.

vincenzopalazzo · 2026-09-15T01:43:31Z

+		return amount_sat_sub(fee, ours,
+				      bitcoin_tx_output_get_amount_sat(tx, i));
+	}
+	/* Our output was trimmed: the fee is everything. */
+	return false;


Cleanup: amount_sat_sub failing and "our output was trimmed" both return false, then we take the full tx fee.

Underflow should not happen after close_tx_check, but maybe a log_broken so we notice?

Added in 63cca63: log_broken with the output index, its amount and our balance, still counting the whole tx fee afterwards. close_tx_check verifies scripts and shape rather than amounts; what rules the underflow out is that closingd built the tx from this same balance, which the comment now says.

daywalker90 · 2026-09-15T12:07:13Z

We propably want this merged into master instead and then cherry-pick from there.

vincenzopalazzo · 2026-09-15T16:23:44Z

We propably want this merged into master instead and then cherry-pick from there.

Do you think that it is possible to have a point release, this bug looks pretty serius

ksedgwic · 2026-09-15T18:16:55Z

We propably want this merged into master instead and then cherry-pick from there.

Happy to rebase onto master, but it has to follow the master ports of the commits it fixes: dbb53f2 "lightningd: Target a lower closing fee" and 869e14b "closingd: Add a sanity check for feerate" (#9507), plus b35280b "closing: sanity-check close transactions" (#9509), which changes the same function as the last fix here. Parts 2 and 3 apply to master as they are, but part 1 fixes a regression from the max-fee bound in dbb53f2, and its test only fails once that bound is in. So the plan would be: leave this on release-26.06.7 until #9507 and #9509 land, then retarget to master and rebase, with the release branch getting the cherry-picks from master as usual.

…e commitment With every fee estimate at the floor, the opener's closing fee range is a single value. Each output is rounded down to whole satoshis, so the msat remainders end up in the fee and the closing transaction pays one satoshi more than the fee both sides agreed on. lightningd rejects that transaction as above its maximum, keeps the commitment as last_tx, and the close proceeds to broadcast the commitment while reporting a mutual close. test_closing_fee_rounding_at_ceiling pins both nodes at the floor, leaves remainders of 999 and 1 msat, closes from the opener, and checks that the transaction returned by close is a two-output closing transaction paying the agreed fee plus one satoshi, and that both nodes see the output after one block. Marked xfail until the following commits. Changelog-None.

closing_fee_is_acceptable checked the fee the closing transaction pays against the maximum derived from the unilateral feerate. That fee is larger than the fee_satoshis both sides agreed on whenever the outputs' msat remainders were rounded away or an output was trimmed as dust. With every estimate at the floor, closingd's minimum and maximum are the same value, so any rounding put the transaction one satoshi over the maximum and lightningd rejected it. The rejection left the commitment as last_tx and the close broadcast it while reporting a mutual close. Derive the negotiated fee from the transaction: our rounded-down balance minus the output paying our shutdown script. closingd subtracts fee_satoshis from the opener's output, and the maximum only applies when we are the opener, so that difference is exactly the fee we agreed to pay. Bound that instead. The minimum is still checked against the fee the transaction actually pays, which is what relay depends on. If our output was trimmed the whole fee is ours and is bounded as before. Changelog-Fixed: lightningd: a mutual close at the fee ceiling no longer falls back to broadcasting the commitment because of satoshi rounding. Fixes: ElementsProject#9495

…ling back to the commitment closingd negotiates within the feerange given to `close`, and lightningd hands it the range minimum as its floor. lightningd's own acceptance check does not use that minimum: it checks the agreed fee against the floor derived from its fee estimates. When the estimates sit above the range, the agreed fee is rejected as too low, the commitment stays as last_tx, and the close broadcasts it while reporting a mutual close. test_closing_feerange_below_estimates opens at the floor, raises the opener's estimates, closes with a feerange pinned at the floor, and checks that the transaction returned by close is a two-output closing transaction at the agreed fee. Marked xfail until the following commit. Changelog-None.

…g fee peer_start_closingd hands closingd the minimum of the feerange given to `close` as its floor, so closingd negotiates down to it. The acceptance check in closing_fee_is_acceptable kept using the floor derived from the fee estimates, so with estimates above the range the agreed fee was rejected as too low, the commitment stayed as last_tx, and the close broadcast it while reporting a mutual close. Use the range minimum as the floor there too, as calc_max_close_feerate already does for the maximum. Both ends of the check now match the bounds closingd negotiated within. Changelog-Fixed: lightningd: `close` with a feerange below the fee estimates no longer falls back to broadcasting the commitment.

…cted When lightningd rejects a closing fee, closingd never learns it: the reply only carries a txid, which closingd uses for its billboard. It agrees to the offer, reports the close complete, and lightningd broadcasts last_tx, still the commitment, as if it were the mutual close. The previous commits removed the known reasons for a rejection, so add --dev-reject-closing-fee, which makes lightningd reject every closing fee the peer offers. test_closing_rejected_fee_fails_negotiation runs it on the non-opener: the opener's close must end in a unilateral close at its timeout, and the rejecting node must never agree to a fee or broadcast anything. Marked xfail until the following commit. Changelog-None.

…sing fee lightningd's reply to closingd_received_signature carried only a txid. closingd used it for the billboard and went on to agree to the offer, so when lightningd had rejected the fee the close still completed and drop_to_chain broadcast last_tx, which was still the commitment, while the close command reported a mutual close. Add the verdict to the reply. On a rejection lightningd logs it at UNUSUAL, and closingd sends the peer a warning and exits instead of agreeing, the same way it handles a fee range with no overlap. The channel stays in CLOSINGD_SIGEXCHANGE: negotiation restarts on reconnect with lightningd's current bounds, and the close command's timeout decides when to close unilaterally. Changelog-Fixed: lightningd: a closing fee lightningd rejects fails the negotiation instead of broadcasting the commitment as a mutual close.

daywalker90 · 2026-09-16T10:57:47Z

rebased onto master and squashed fixup commits

nGoline

Reviewed all six commits; each of the three fixes does what the description says, the three reproductions are real (xfail strict, lifted by the matching fix) and deterministic, and the new wire field is lightningd to closingd only with the dev option gated behind --developer. One residual case worth closing, and two nits, inline. Happy to approve once the first is addressed or argued away.

nGoline · 2026-09-16T11:46:57Z

-		if (channel->opener == LOCAL && amount_sat_less(max_fee, fee)) {
-			log_debug(channel->log, "... That's above our max %s"
-				  " for weight %"PRIu64" at feerate %u",
+		if (!negotiated_close_fee(channel, tx, &negotiated))


When negotiated_close_fee() returns false because our own output was trimmed as dust, this falls back to comparing the whole tx fee against a max_fee computed on the 1-output weight, while closingd's max_fee_to_accept used the 2-output estimate. So an opener whose remaining balance is below dust_limit still has its own agreed fee rejected at floor feerates (now loudly, warning plus unilateral at the timeout, instead of silently). Either compare against max_fee at the same 2-output weight (closing_tx_weight_estimate) in that branch, or subtract the trimmed dust from fee before comparing. A regression test with the opener drained to under dust_limit would pin it.

nGoline · 2026-09-16T11:46:57Z

+		 * the fee against the same bounds we negotiate within, so
+		 * this is not expected to fire. */
+		if (!tell_master_their_offer(&their_sig, tx, closing_txid))
+			peer_failed_warn(pps, channel_id,


Nit, pre-existing pattern: peer_failed_warn is NORETURN, so the return; after it is dead. Fine to leave.

nGoline · 2026-09-16T11:46:57Z

+
+    # The only transaction on the wire is that unilateral close.
+    txid = only_one(res['txids'])
+    wait_for(lambda: bitcoind.rpc.getrawmempool() == [txid])


Nit: this also implicitly asserts no anchor CPFP tx alongside the commitment. A one-line comment saying so would stop a future anchor change from reading as a regression here.

ksedgwic requested review from daywalker90 and nGoline September 14, 2026 23:12

vincenzopalazzo reviewed Sep 15, 2026

View reviewed changes

daywalker90 added the 26.06.x label Sep 15, 2026

ksedgwic added 6 commits September 16, 2026 11:46

daywalker90 changed the base branch from release-26.06.7 to master September 16, 2026 10:04

daywalker90 requested review from Lagrang3 and cdecker as code owners September 16, 2026 10:04

daywalker90 force-pushed the fix-close-fee-reject-fallthrough branch from fddb967 to c7a325e Compare September 16, 2026 10:09

nGoline reviewed Sep 16, 2026

View reviewed changes

Uh oh!

Conversation

ksedgwic commented Sep 14, 2026

Uh oh!

vincenzopalazzo left a comment

Choose a reason for hiding this comment

Code Review

Positive Aspects

Issues & Suggestions

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daywalker90 commented Sep 15, 2026

Uh oh!

vincenzopalazzo commented Sep 15, 2026

Uh oh!

ksedgwic commented Sep 15, 2026

Uh oh!

daywalker90 commented Sep 16, 2026

Uh oh!

nGoline left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants