[davelund110]

What

onchaind_tx_unsigned() sizes an onchain spend before signing it, so it can pick the fee. Its estimate covers the unsigned transaction, a worst-case signature and the witness script. It leaves out the stack element that sits between the signature and the script:

• the revocation pubkey, in a penalty for an HTLC output (33 bytes);
• the preimage, in an HTLC fulfill (32 bytes).

The fee then comes out about 5% short of the feerate it was meant to pay. This PR adds the element's length to the estimate. It's one line, plus a test.

Why it matters

The estimate is 30 to 31 weight units short (the missing element, less the 2 bytes the 73-byte worst-case signature over-counts, since signatures are ground to low-R), about 5%. Whenever the feerate chosen is bitcoind's own minimum, that 5% puts the transaction below it, and bitcoind refuses it every time it is rebroadcast. That happens:

• when the deadline is 101 to 200 blocks away, where feerate_for_target() uses get_feerate_floor() by design, whatever the estimates say. An HTLC penalty's deadline is to_self_delay (144 by default) after the close, so its first ~44 broadcasts always fall in this window;
• when estimates sit at the floor, as on a quiet network or a regtest;
• with a node whose minimum is about 1 sat/vbyte, as Bitcoin Knots' is by default, or with any node once a full mempool raises mempoolminfee (the error is then "mempool min fee not met").

On our regtest:

plugin-bcli: min relay fee not met, 148 < 155

• HTLC penalty: the cheater's own HTLC-timeout (or HTLC-success) transaction can confirm first. We then have to penalise its output instead, which works but takes longer and leaves more room for things to go wrong.
• HTLC fulfill on the peer's commitment: this is the real risk. If the fulfill stays below the minimum until the HTLC expires, the peer can time it out, and we lose an amount we have already paid downstream.

Upstream Core Lightning has the same line (weight = bitcoin_tx_weight(tx) + 1 + 3 + 73 + 0 + tal_count(info->wscript); in lightningd/onchain_control.c), so this is worth sending there too.

How it was found

Running Lightning Fork (lnd) against this fork on a Bitcoin Knots BLAKE2b regtest, which has no fee estimates, so everything sits at the floor. lnd broadcast a revoked commitment with an HTLC on it. CLN's penalty for the HTLC was refused at every block, until lnd's HTLC-timeout confirmed and CLN penalised that instead. Decoding the refused transaction showed a witness of 71 + 33 + 136 bytes, 622 weight units against the roughly 590 the fee was computed for.

Tests

• New test_penalty_htlc_min_feerate (anchors and not): test_penalty_inhtlc with every feerate at the floor. Without the change, the HTLC penalty is refused (min relay fee not met, 148 < 155). With it, both penalties are mined.
• test_penalty_inhtlc, test_penalty_outhtlc, test_penalty_htlc_tx_fulfill and test_penalty_htlc_tx_timeout (both variants each) still pass.
• test_coinmoves.py, as the review pointed out: three expected amounts now reflect the new fees (the fulfill, the HTLC penalty, and the to-local penalty, whose 0x01 element is now counted too). The commit is amended with them, and all 10 tests pass.
• The penalty, onchain and HTLC tests in test_closing.py, and test_bookkeeper.py, pass with the fix (66 passed), except the two test_bookkeeper.py report tests that need currency rates from the network, which fail the same way without it.
• In the lnd interop run of Add option_independent_secrets channel type #48, CLN now takes the HTLC with its first penalty.
• Rebased onto blake2b-unified at 921d739 (including the runner OOM fix and bitcoin: count the witness script when weighing an anchors to_remote input #57's to_remote anchor weight fix) without conflicts. There, test_coinmoves.py, test_penalty_htlc_min_feerate and test_wallet.py pass (74 passed).

This is independent of option_independent_secrets and applies on its own to the fork's base. A review checked the arithmetic for every case (penalties on offered and received HTLCs, with and without anchors, and fulfills): all fall 7 to 8 sat short of the minimum at 1 sat/vbyte, and all clear it with the fix. Spends whose element is 0x01 were already covered by the slack in the 73-byte signature; they now count that byte too, so their fee rises by a few sat. Spends with no element are unaffected. We found no existing issue or PR about this upstream.

[kwsantiago]

The coinmoves tests hardcode the old fees: fulfill 94534000→94238000, penalty 92908000→92512000 and 49858187000→49858175000 msat; all pass then.

[davelund110]

Thanks, updated with your values and folded into the commit (addaee3). All 10 coinmoves tests pass. The closing and bookkeeping tests pass too, except the two report tests that need live currency rates.

[kwsantiago]

Could you rebase onto blake2b-unified? #55 fixed the runner OOM that keeps failing ASan/UBSan 2/8 here.

[davelund110]

Rebased onto blake2b-unified (3d1200a), no conflicts. test_coinmoves, test_penalty_htlc_min_feerate and test_wallet pass locally (74 passed).

[kwsantiago]

Merged, thanks. Worth opening upstream too, since it's the same line there.

[davelund110]

Thanks Kyle. Opened upstream as ElementsProject#9662, tested on their master with a standard regtest: the new test fails there without the change and passes with it.