Part of #9222.
Seen on a valgrind CI run of #9344 (an unrelated pyln-testing change):
https://github.com/ElementsProject/lightning/actions/runs/30033838024/job/89299664724
FAILED tests/test_closing.py::test_onchain_rbf_stops_after_confirmation - AssertionError: node kept RBF-ing penalty tx after the replacement was confirmed
The test (added in 3e0f48e) censors l2's sendrawtransaction with an rpcproxy mock while the node RBFs its penalty tx, then removes the mock and expects the next RBF version to be broadcast for real, mined, and to stop further RBF-ing. The failure is a race between removing the mock and the node's in-flight broadcast, reconstructed from the node log of the run above:
18:44:21.866 block 107: third loop iteration logs 'RBF onchain txid a6b2... with txid 8e476... (fee 5521sat)'; the test's wait_for_log returns and the test un-mocks sendrawtransaction
18:44:21.938 the node's broadcast of 8e476 reaches the proxy 72ms later -- after the un-mock -- so this older version enters bitcoind's real mempool
18:44:23.137 block 108: node RBFs 8e476 -> f246 (5830sat) and broadcasts f246 for real; bitcoind rejects it (sendrawtx exit 25; 8e476 already occupies the mempool)
18:44:24.221 block 109 (wait_for_mempool=1) mines 8e476 -- an older version than the node's tracked f246
18:44:24.319 onchaind: 'Resolved THEIR_REVOKED_UNILATERAL/DELAYED_CHEAT_OUTPUT_TO_THEM by our proposal OUR_PENALTY_TX (8e476...)'; the test records its log position here
18:44:24.412 block 109's rebroadcast pass: the tracked replacement f246 never confirmed, so the RBF logic fires again and logs 'RBF onchain txid f246... with txid 941ba...' -- the line the assertion forbids. This is before the test's set_feerates bump; the deadline-driven fee increase alone triggers it.
18:44:25.857 RBF continues on blocks 110/111 (941ba -> e72b); those broadcasts fail with -25 since the input is already spent, so no on-chain effect, but the assertion has already failed.
The test's design assumes the version that confirms is the version the node currently tracks. Under valgrind the window between the RBF log line and the corresponding broadcast is easy to hit, letting the previous version leak into the mempool and get mined instead.
Proposed fix: make the un-mock deterministic by having the censoring mock count the calls it swallows, and wait for the fourth broadcast (initial penalty tx plus three RBF replacements) to have reached the proxy before removing the mock. Then no broadcast can be in flight when censoring stops, and the next real broadcast is the block-108 RBF version the test expects to be mined.
A side observation from the same trace: the stop-RBF check keys on the currently tracked txid, so if an older replacement version is what actually confirms (which miners can do -- they are not obliged to have seen the latest replacement), lightningd keeps attempting RBF broadcasts that fail with -25. Harmless on-chain but noisy; noting it in case a follow-up is worthwhile.
Part of #9222.
Seen on a valgrind CI run of #9344 (an unrelated pyln-testing change):
https://github.com/ElementsProject/lightning/actions/runs/30033838024/job/89299664724
The test (added in 3e0f48e) censors l2's sendrawtransaction with an rpcproxy mock while the node RBFs its penalty tx, then removes the mock and expects the next RBF version to be broadcast for real, mined, and to stop further RBF-ing. The failure is a race between removing the mock and the node's in-flight broadcast, reconstructed from the node log of the run above:
The test's design assumes the version that confirms is the version the node currently tracks. Under valgrind the window between the RBF log line and the corresponding broadcast is easy to hit, letting the previous version leak into the mempool and get mined instead.
Proposed fix: make the un-mock deterministic by having the censoring mock count the calls it swallows, and wait for the fourth broadcast (initial penalty tx plus three RBF replacements) to have reached the proxy before removing the mock. Then no broadcast can be in flight when censoring stops, and the next real broadcast is the block-108 RBF version the test expects to be mined.
A side observation from the same trace: the stop-RBF check keys on the currently tracked txid, so if an older replacement version is what actually confirms (which miners can do -- they are not obliged to have seen the latest replacement), lightningd keeps attempting RBF broadcasts that fail with -25. Harmless on-chain but noisy; noting it in case a follow-up is worthwhile.