test: add test for rebroadcast of transaction received via p2p

mzumsande commented at 2:42 am on January 21, 2026: contributor

The wallet doesn’t only rebroadcast transactions it created, but also relevant transactions received via p2p. Since this is not self-evident, add test coverage for it.

DrahtBot added the label Tests on Jan 21, 2026

DrahtBot commented at 2:42 am on January 21, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34359.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, vasild, furszy, danielabrozzoni

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34533 (wallet: resubmit transactions with private broadcast if enabled by vasild)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

mzumsande force-pushed on Jan 21, 2026

DrahtBot added the label CI failed on Jan 21, 2026

vasild commented at 11:30 am on January 22, 2026: contributor

2026-01-21T03:34:27.4304152Z [0;36m test 2026-01-21T03:34:27.140268Z TestFramework (INFO): node0 sends a tx to node1 and disconnects [0m … 2026-01-21T03:34:27.4319719Z [0;34m node0 2026-01-21T03:34:27.149584Z (mocktime: 2026-02-07T03:50:31Z) [httpworker.1] [wallet/wallet.h:938] [WalletLogPrintf] [default_wallet] Submitting wtx c255f5b61919a60877d4e91eb81f656f85e743513119f5ddcc9cffc4ffb62b45 to mempool and for broadcast to peers [0m … 2026-01-21T03:34:27.4320508Z [0;34m node0 2026-01-21T03:34:27.149947Z (mocktime: 2026-02-07T03:50:31Z) [httpworker.1] [wallet/wallet.h:938] [WalletLogPrintf] [default_wallet] CommitTransaction(): Transaction cannot be broadcast immediately, bad-txns-inputs-missingorspent [0m

So, node0 did not like the transaction it generated and failed to broadcast it to node1: bad-txns-inputs-missingorspent. Happens only sometimes.

mzumsande commented at 12:03 pm on January 22, 2026: contributor

yes - because of the wallet bug, the wallet creates an invalid transaction, trying to double-spend an utxo that was already used in a block. The node then rejects that tx obviously, and the rpc querying for the tx fails. I’ll rebase on #34358 with the next push. [Edit:done now]

mzumsande force-pushed on Jan 26, 2026

DrahtBot removed the label CI failed on Jan 26, 2026

mzumsande force-pushed on Jan 29, 2026

mzumsande commented at 3:04 am on January 29, 2026: contributor

rebased/undrafted after merge of #34358

mzumsande marked this as ready for review on Jan 29, 2026

in test/functional/wallet_resendwallettransactions.py:156 in 5c9dedf1ef

147@@ -144,6 +148,44 @@ def run_test(self):
148         node.getmempoolentry(txid)
149         node.getmempoolentry(child_txid)
150 
151+        self.log.info("Test rebroadcast of transactions received by others")
152+        # clear mempool
153+        self.generate(node, 1, sync_fun=self.no_op)
154+        # Sync node1's mocktime to node0's before connecting so it accepts node0's blocks
155+        node1 = self.nodes[1]
156+        node1.setmocktime(evict_time + 36 * 60 * 60)

vasild commented at 1:24 pm on February 4, 2026:

evict_time + 36 * 60 * 60 is used above, on line 146, to set the time on nodes[0]. Then repeated here. Maybe put that expression in a variable and use the variable in both places to make it obvious that both should be the same. Will be less fragile as well.

mzumsande commented at 10:22 am on February 9, 2026:

I decided to just put 36 * 60 * 60 into a constant, because that is used all over the test.

in test/functional/wallet_resendwallettransactions.py:183 in 5c9dedf1ef

178+        self.log.info("Connect p2p who hasn't seen the tx")
179+        peer = node1.add_p2p_connection(P2PTxInvStore())
180+
181+        self.log.info("Check that rebroadcast happens after 36 hours")
182+        with node1.assert_debug_log(['resubmit 1 unconfirmed transactions']):
183+            node1.bumpmocktime(36 * 60 * 60)

vasild commented at 2:01 pm on February 4, 2026:

Maybe refer to CWallet::SetNextResend() to make it easier to get to know where these 36 hours come from. E.g.:

0            node1.bumpmocktime(36 * 60 * 60) # see CWallet::SetNextResend()

mzumsande commented at 10:23 am on February 9, 2026:

put 36 * 60 * 60 into a constant and added an explanation there.

in test/functional/wallet_resendwallettransactions.py:172 in 5c9dedf1ef outdated

167+        assert_equal(wallet_tx["confirmations"], 0)
168+        recv_wtxid = node1.getmempoolentry(recv_txid)["wtxid"]
169+        self.disconnect_nodes(0, 1)
170+
171+        self.log.info("Create a block without the transaction")
172+        node1.bumpmocktime(6 * 60)

vasild commented at 2:09 pm on February 4, 2026:

0+       # The transaction must be at least 5 minutes older than the latest block,
1+       # see CWallet::ResubmitWalletTransactions()
2        node1.bumpmocktime(6 * 60)

mzumsande commented at 10:24 am on February 9, 2026:

done (a bit shorter because it is already explained in the first part in more detail).

vasild approved

vasild commented at 2:15 pm on February 4, 2026: contributor

ACK 5c9dedf1efbc8810758f0e0fad7c97860f8da703

mzumsande force-pushed on Feb 9, 2026

mzumsande commented at 10:24 am on February 9, 2026: contributor

5c9dedf to b2c9eac: addressed feedback

vasild approved

vasild commented at 9:55 am on February 13, 2026: contributor

ACK b2c9eac583c6d3e25854c82e207d61e296891fd7

DrahtBot added the label Needs rebase on Feb 18, 2026

mzumsande force-pushed on Feb 21, 2026

mzumsande commented at 7:03 am on February 21, 2026: contributor

b2c9eac to 990e2cd: rebased

DrahtBot removed the label Needs rebase on Feb 21, 2026

vasild approved

vasild commented at 11:13 am on February 23, 2026: contributor

ACK 990e2cd0babf7a2ee90d7b40e91a0c7477511957

in test/functional/wallet_resendwallettransactions.py:31 in 990e2cd0ba

27+
28 class ResendWalletTransactionsTest(BitcoinTestFramework):
29     def set_test_params(self):
30-        self.num_nodes = 1
31+        self.num_nodes = 2
32+        self.extra_args = [["-whitelist=relay,noban@127.0.0.1"], ["-whitelist=relay,noban@127.0.0.1"]]

maflcko commented at 6:38 pm on February 26, 2026:

Is there a reason why this is needed? Without this, sending the tx fails, but I don’t see why.

vasild commented at 8:23 am on February 27, 2026:

node0 must have node1 with the noban permission in order to send the transaction to it without delay in this part of the test:

0        self.log.info("node0 sends a tx to node1 and disconnects")
1        recv_addr = node1.getnewaddress()
2        recv_txid = node.sendtoaddress(recv_addr, 1)

https://github.com/bitcoin/bitcoin/blob/b6b8f8ac555f155a44ee3780b144e0960ae73483/src/net_processing.cpp#L5981-L5989

Note that in this part of the test the time is frozen and does not flow because setmocktime() has been called on node0 so it never sets fSendTrickle to true and thus never sends the transaction to node1.

Btw this suffices:

0        self.extra_args = [["-whitelist=noban@127.0.0.1"], []]

maflcko commented at 8:40 am on February 27, 2026:

Ok, that is pretty obvious, thx. Could also add a comment, like # Needed due to mocktime. A third alternative could be self.noban_tx_relay = True # Needed due to mocktime.

vasild commented at 9:15 am on February 27, 2026:

… or to advance the mocktime across NextInvToInbounds(current_time, INBOUND_INVENTORY_BROADCAST_INTERVAL, node.m_network_key) but that would always leave, at least in theory, some chance that we did not advance enough.

mzumsande commented at 11:34 am on February 27, 2026:

used self.noban_tx_relay, which is more common

in test/functional/wallet_resendwallettransactions.py:84 in 990e2cd0ba

77@@ -70,11 +78,11 @@ def run_test(self):
78         # Transaction should be rebroadcast approximately 24 hours in the future,
79         # but can range from 12-36. So bump 36 hours to be sure.
80         with node.assert_debug_log(['resubmit 1 unconfirmed transactions'], timeout=2):
81-            node.setmocktime(now + 36 * 60 * 60)
82+            node.setmocktime(now + RESEND_TIMER_LIMIT)
83             # Tell scheduler to call MaybeResendWalletTxs now.
84             node.mockscheduler(60)
85         # Give some time for trickle to occur

maflcko commented at 8:48 am on February 27, 2026:

nit: I guess this is no longer needed and can be removed now? Also – unrelated, the peer_second.wait_for_broadcast([txid]) can be moved into the assert_debug_log scope, so that the timeout=2-polling on it can be removed?

mzumsande commented at 11:35 am on February 27, 2026:

done

in test/functional/wallet_resendwallettransactions.py:191 in 990e2cd0ba

187+        self.log.info("Check that rebroadcast happens after 36 hours")
188+        with node1.assert_debug_log(['resubmit 1 unconfirmed transactions'], timeout=2):
189+            node1.bumpmocktime(RESEND_TIMER_LIMIT)
190+            node1.mockscheduler(60)
191+
192+        peer.wait_for_broadcast([recv_wtxid])

maflcko commented at 8:55 am on February 27, 2026:

same nit:

0        with node1.assert_debug_log(['resubmit 1 unconfirmed transactions']):
1            node1.bumpmocktime(RESEND_TIMER_LIMIT)
2            node1.mockscheduler(60)
3
4            peer.wait_for_broadcast([recv_wtxid])

maflcko approved

maflcko commented at 9:00 am on February 27, 2026: member

lgtm. Feel free to ignore the nits.

I guess this doesn’t add any coverage in terms of new paths/lines, because the wallet does not differentiate between tx sources, but if this ever were to change in the future, this test would already be there.

review ACK 990e2cd0babf7a2ee90d7b40e91a0c7477511957 🦉

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK 990e2cd0babf7a2ee90d7b40e91a0c7477511957 🦉
3Qvh92TAwBuuBKdQwPindEYsKDLnaNpJ8iBuqe/CM1y4flECUTk5rYxTez0Xkgs5tWg/eb7685VUp8bnGdkSgCQ==

test: add test for rebroadcast of transaction received via p2p

The wallet doesn't only rebroadcast transactions it created, but
also relevant transactions received via p2p. Since this is not
self-evident, add test coverage for it.

73e3853110

mzumsande force-pushed on Feb 27, 2026

mzumsande commented at 11:44 am on February 27, 2026: contributor

990e2cd to 73e3853: addressed feedback

I guess this doesn’t add any coverage in terms of new paths/lines, because the wallet does not differentiate between tx sources, but if this ever were to change in the future, this test would already be there.

Yes, my main motivation is to have a test for this behavior before discussing changes to it.

I knew that rebroadcast has privacy downsides, but I always thought that just making sure your txns confirm in a couple of hours would be sufficient to mitigate it, which seemed annoying but manageable to me.

But I never realized that just passively (not submitting any txns) running a Bitcoin core node for a day with a loaded wallet used a long time ago (maybe even with private broadcast then) would put you at risk of being deanonymized via dusting / rebroadcast tracking - even though I’m sure more regular wallet contributors knew this, after all there is the ancient issue #3828.

maflcko commented at 12:20 pm on February 27, 2026: member

Just addressed some nits:

re-ACK 73e385311055eb6c0e3ddb2221f927a3872af1fb 🦌

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK 73e385311055eb6c0e3ddb2221f927a3872af1fb 🦌
3zjBj3GEv01zf0Guo3qrZXQrLOPpIZFK4Wx5VdzynhAezS9nYDqHGcmp3CUltj2Hh6GlStWdIsBrwFwXA3CVbBQ==

DrahtBot requested review from vasild on Feb 27, 2026

vasild approved

vasild commented at 1:15 pm on February 27, 2026: contributor

ACK 73e385311055eb6c0e3ddb2221f927a3872af1fb

furszy commented at 4:06 pm on February 28, 2026: member

ACK 73e385311055eb6c0e3ddb2221f927a3872af1fb

in test/functional/wallet_resendwallettransactions.py:154 in 73e3853110

150+            node.setmocktime(evict_time + RESEND_TIMER_LIMIT)
151             node.mockscheduler(60)
152         node.getmempoolentry(txid)
153         node.getmempoolentry(child_txid)
154 
155+        self.log.info("Test rebroadcast of transactions received by others")

danielabrozzoni commented at 9:19 pm on March 4, 2026:

nit: I find “received by others” to be slightly confusing, because instead of reading it as “p2p tx message that someone sent us”, I read it as “transaction whose recipient is someone else”, which is the opposite of what’s happening here (node1 does the rebroadcast and is the recipient).

Maybe “Test rebroadcast of incoming wallet transactions” is clearer.

danielabrozzoni commented at 9:30 pm on March 4, 2026: member

tACK 73e385311055eb6c0e3ddb2221f927a3872af1fb

It wasn’t obvious to me that the wallet rebroadcasts transactions it’s the recipient of, so it’s nice to have a test that makes this a bit more explicit.

I left a micro-nit in case you need to retouch, otherwise it’s good as-is.

sedited merged this on Mar 11, 2026

sedited closed this on Mar 11, 2026

test: add test for rebroadcast of transaction received via p2p #34359

Code Coverage & Benchmarks

Reviews

Conflicts