tests: test RBF stops after closed tx is confirmed
What the developer wrote
Authored by enaples
tests: test RBF stops after closed tx is confirmed
What changed, and why it matters
This commit adds a test that checks whether Core Lightning stops trying to replace (RBF) a penalty transaction once a replacement version has already been confirmed on the blockchain. The test documents a bug where the node would keep firing replacement attempts indefinitely because it looked up the wrong (stale) transaction ID and thought the original was still unconfirmed. The commit itself only adds a regression test, not the fix, so the actual vulnerability is in the pre-existing code being tested.
Review the corresponding fix (not present in this commit) to ensure `consider_onchain_rebroadcast` checks the confirmation height of the current in-flight/replacement transaction rather than the original stale txid. Run the new regression test against the pre-fix code to confirm it fails, and against the fixed code to confirm it passes. Monitor for related RBF termination logic in other on-chain paths.
Security signals we found
RBF loop does not terminate after replacement tx is confirmed
Use of stale original_txid instead of current in-flight txid for confirmation check
Indefinite rebroadcast of on-chain penalty transaction
Regression test added for a bug with direct security/fee-loss implications
Evidence from the diff
The new test test_onchain_rbf_stops_after_confirmation simulates a revoked commitment broadcast by a malicious peer, triggering the honest node’s onchaind to create a penalty transaction. By censoring sendrawtransaction initially, the test forces onchaind to repeatedly RBF the penalty tx as blocks advance. Once the replacement penalty tx is finally mined, the test asserts that no further ‘RBF onchain txid’ log lines appear after confirmation. The comment explicitly states the bug: without the fix, wallet_transaction_height(original_txid) returns 0 because the stale original txid was never mined, causing consider_onchain_rebroadcast to keep firing indefinitely. This could waste fees, spam the network, and potentially keep the node’s wallet locked up in repeated RBF attempts.
Changed components
onchaindwallet transaction lookupconsider_onchain_rebroadcastrebroadcast_txsRBF fee-bumping logicInspect captured patch +71 / −0
diff --git a/tests/test_closing.py b/tests/test_closing.py
index 88a9fb19..09c1dc0e 100644
--- a/tests/test_closing.py
+++ b/tests/test_closing.py
@@ -1725,6 +1725,77 @@ def test_penalty_rbf_normal(node_factory, bitcoind, executor, chainparams, ancho
check_utxos_channel(l2, [channel_id], expected_2)
+def test_onchain_rbf_stops_after_confirmation(node_factory, bitcoind):
+ """Penalty tx RBF stops once the replacement tx is confirmed."""
+
+ to_self_delay = 10
+ opts = {'watchtime-blocks': to_self_delay, 'dev-force-features': "-23"}
+
+ # l1 is the thief; allow it to fail.
+ l1 = node_factory.get_node(options=opts, may_fail=True)
+ l2 = node_factory.get_node(options=opts)
+
+ l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
+ l1.fundchannel(l2, 10**7)
+
+ # Save l1's current commitment before it is revoked.
+ theft_tx = l1.rpc.dev_sign_last_tx(l2.info['id'])['tx']
+
+ # Advance the commitment state — the saved commitment is now revoked.
+ l1.pay(l2, 1000000)
+ l1.rpc.stop()
+
+ # Censor l2 so the penalty tx never reaches miners.
+ def censoring_sendrawtx(r):
+ return {'id': r['id'], 'result': {}}
+ l2.daemon.rpcproxy.mock_rpc('sendrawtransaction', censoring_sendrawtx)
+
+ # l1 broadcasts the revoked commitment.
+ bitcoind.rpc.sendrawtransaction(theft_tx)
+ bitcoind.generate_block(1)
+ l2.daemon.wait_for_log(' to ONCHAIN')
+
+ _, txid, blocks = l2.wait_for_onchaind_tx('OUR_PENALTY_TX',
+ 'THEIR_REVOKED_UNILATERAL/DELAYED_CHEAT_OUTPUT_TO_THEM')
+ assert blocks == 0 # penalty tx is immediately broadcastable
+
+ # Each block brings the deadline (close_blockheight + to_self_delay) closer
+ # so feerate_for_target() returns a higher fee and onchaind emits INFO-level "RBF onchain txid".
+ for _ in range(3):
+ bitcoind.generate_block(1)
+ l2.daemon.wait_for_log('RBF onchain txid')
+
+ # Stop censoring. Generate a block to trigger rebroadcast (the penalty tx
+ # enters bitcoind's mempool) but filter it out so the next block mines it.
+ l2.daemon.rpcproxy.mock_rpc('sendrawtransaction', None)
+ bitcoind.generate_block(1, needfeerate=10000000)
+ l2.daemon.wait_for_log('RBF onchain txid')
+
+ # Mine the penalty tx (single output, no in-flight HTLCs).
+ bitcoind.generate_block(1, wait_for_mempool=1)
+ sync_blockheight(bitcoind, [l2])
+
+ l2.daemon.wait_for_log('Resolved THEIR_REVOKED_UNILATERAL/DELAYED_CHEAT_OUTPUT_TO_THEM'
+ ' by our proposal OUR_PENALTY_TX')
+
+ # Record log position right after confirmation.
+ log_pos = l2.daemon.logsearch_start
+
+ # Bump feerate: any spurious rebroadcast would compute a higher fee
+ # (newfee > info->fee) and emit "RBF onchain txid" at INFO level.
+ # Without the fix: wallet_transaction_height(original_txid) returns 0
+ # because the stale original txid was never mined (only the replacement
+ # was), so consider_onchain_rebroadcast keeps firing indefinitely.
+ l2.set_feerates([10000] * 4, False)
+
+ # rebroadcast_txs() fires on every new block (chaintopology.c ~line 854).
+ bitcoind.generate_block(2)
+ sync_blockheight(bitcoind, [l2])
+
+ assert not l2.daemon.is_in_log('RBF onchain txid', start=log_pos), \
+ "node kept RBF-ing penalty tx after the replacement was confirmed"
+
+
def test_onchain_first_commit(node_factory, bitcoind):
"""Onchain handling where opener immediately drops to chain"""
Why this scored 61/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.