From 3d1200a585c941de7563a959200aafb047cd9dc9 Mon Sep 17 00:00:00 2001 From: Dave Lund Date: Tue, 6 Oct 2026 11:46:57 +0200 Subject: [PATCH] lightningd: count the witness stack element when sizing onchain spends onchaind_tx_unsigned() estimates a spend's weight as the unsigned transaction plus a worst-case signature and the witness script, but leaves out the stack element between them: the revocation pubkey of a penalty for an HTLC output (33 bytes), or the preimage of an HTLC fulfill (32 bytes). The fee then falls a few percent short of the feerate it was meant to pay. Usually that only costs a little time, but when every estimate is at the floor (253 sat/kw, just above a minimum relay fee of 1 sat/vbyte) the transaction pays less than the minimum relay fee, and bitcoind refuses it each time it is rebroadcast: min relay fee not met, 148 < 155 For an HTLC penalty, the cheater's HTLC-timeout or HTLC-success transaction can then confirm first, and we have to penalise its output instead. For a fulfill, the peer can time the HTLC out. test_penalty_htlc_min_feerate is test_penalty_inhtlc with every feerate at the floor: without this change its HTLC penalty never gets into the mempool. Changelog-Fixed: Onchain spends with a revocation pubkey or preimage in their witness (HTLC penalties and fulfills) no longer pay less than the minimum relay fee when feerate estimates are at the floor. --- lightningd/onchain_control.c | 7 +++-- tests/test_closing.py | 51 ++++++++++++++++++++++++++++++++++++ tests/test_coinmoves.py | 12 ++++----- 3 files changed, 62 insertions(+), 8 deletions(-) diff --git a/lightningd/onchain_control.c b/lightningd/onchain_control.c index 88d441810f2c..908a238a1617 100644 --- a/lightningd/onchain_control.c +++ b/lightningd/onchain_control.c @@ -977,8 +977,11 @@ static struct bitcoin_tx *onchaind_tx_unsigned(const tal_t *ctx, channel_internal_error(channel, "Could not add keypath?"); return tal_free(tx); } - /* Worst-case sig is 73 bytes */ - weight = bitcoin_tx_weight(tx) + 1 + 3 + 73 + 0 + tal_count(info->wscript); + /* Worst-case sig is 73 bytes. The stack element (a revocation + * pubkey or a preimage, if any) goes in the witness too: without it, + * at the minimum feerate the fee falls below the minimum relay fee. */ + weight = bitcoin_tx_weight(tx) + 1 + 3 + 73 + + tal_bytelen(info->stack_elem) + tal_count(info->wscript); weight += elements_tx_overhead(chainparams, 1, 1); /* BOLT #5: diff --git a/tests/test_closing.py b/tests/test_closing.py index 72fd3631bd9d..06928c6a6053 100644 --- a/tests/test_closing.py +++ b/tests/test_closing.py @@ -640,6 +640,57 @@ def test_penalty_inhtlc(node_factory, bitcoind, executor, chainparams, anchors): check_utxos_channel(l2, [channel_id], expected_2, tags) +@pytest.mark.parametrize("anchors", [False, True]) +def test_penalty_htlc_min_feerate(node_factory, bitcoind, executor, chainparams, anchors): + """As test_penalty_inhtlc, but with every feerate estimate at the floor: + the penalty for the HTLC must still pay the minimum relay fee, so its + weight has to include the revocation pubkey in its witness.""" + + if chainparams['elements'] and anchors: + pytest.skip('elementsd anchors unsupported') + + opts = {'dev-disable-commit-after': 1, + 'feerates': (253, 253, 253, 253)} + if anchors is False: + opts['dev-force-features'] = "-23" + + l1, l2 = node_factory.line_graph(2, opts=[{**opts, 'may_fail': True, + # We try to cheat! + 'broken_log': r"onchaind-chan#[0-9]*: Could not find resolution for output .*: did \*we\* cheat\?"}, + opts]) + scid = first_scid(l1, l2) + + # This gets stuck once the HTLC is in l1's commitment. + t = executor.submit(l1.pay, l2, 100000000) + l1.daemon.wait_for_log('dev-disable-commit-after: disabling') + l2.daemon.wait_for_log('dev-disable-commit-after: disabling') + l1.daemon.wait_for_log('got commitsig') + + tx = l1.rpc.dev_sign_last_tx(l2.info['id'])['tx'] + + l1.rpc.dev_reenable_commit(l2.info['id']) + l2.rpc.dev_reenable_commit(l1.info['id']) + t.result(timeout=TIMEOUT) + wait_for(lambda: all([only_one(n.rpc.listpeerchannels()['channels'])['htlcs'] == [] for n in (l1, l2)])) + + bitcoind.rpc.sendrawtransaction(tx) + bitcoind.generate_block(1) + l2.daemon.wait_for_log(' to ONCHAIN') + wait_for(lambda: l2.is_local_channel_active(scid) is False) + + ((_, txid1, _), (_, txid2, _)) = \ + l2.wait_for_onchaind_txs(('OUR_PENALTY_TX', + 'THEIR_REVOKED_UNILATERAL/DELAYED_CHEAT_OUTPUT_TO_THEM'), + ('OUR_PENALTY_TX', + 'THEIR_REVOKED_UNILATERAL/THEIR_HTLC')) + assert not l2.daemon.is_in_log('min relay fee not met') + + # Both get into the mempool, and are mined. + bitcoind.generate_block(1, wait_for_mempool=[txid1, txid2]) + l2.daemon.wait_for_logs([r'Resolved THEIR_REVOKED_UNILATERAL/DELAYED_CHEAT_OUTPUT_TO_THEM by our proposal OUR_PENALTY_TX', + r'Resolved THEIR_REVOKED_UNILATERAL/THEIR_HTLC by our proposal OUR_PENALTY_TX']) + + @pytest.mark.parametrize("anchors", [False, True]) def test_penalty_outhtlc(node_factory, bitcoind, executor, chainparams, anchors): """Test penalty transaction with an outgoing HTLC""" diff --git a/tests/test_coinmoves.py b/tests/test_coinmoves.py index 70789e9d59fd..fdef3bc14298 100644 --- a/tests/test_coinmoves.py +++ b/tests/test_coinmoves.py @@ -1398,10 +1398,10 @@ def test_coinmoves_unilateral_htlc_fulfill(node_factory, bitcoind): # Note: the invoice is fulfilled in the *chain* moves, not *channel*. expected_chain2 += [{'account_id': 'wallet', 'blockheight': 105, - 'credit_msat': 94534000, + 'credit_msat': 94238000, 'debit_msat': 0, 'extra_tags': [], - 'output_msat': 94534000, + 'output_msat': 94238000, 'primary_tag': 'deposit', 'utxo': f"{htlc_success_txid}:0"}, {'account_id': fundchannel['channel_id'], @@ -1829,18 +1829,18 @@ def test_coinmoves_unilateral_htlc_penalty(node_factory, bitcoind): expected_chain2 += [{'account_id': 'wallet', 'blockheight': 105, - 'credit_msat': 49858187000, + 'credit_msat': 49858175000, 'debit_msat': 0, 'extra_tags': [], - 'output_msat': 49858187000, + 'output_msat': 49858175000, 'primary_tag': 'deposit', 'utxo': f"{to_l1_penalty}:0"}, {'account_id': 'wallet', 'blockheight': 105, - 'credit_msat': 92908000, + 'credit_msat': 92512000, 'debit_msat': 0, 'extra_tags': [], - 'output_msat': 92908000, + 'output_msat': 92512000, 'primary_tag': 'deposit', 'utxo': f"{htlc_penalty}:0"}, {'account_id': fundchannel['channel_id'],