#36482 wallet: Only broadcast resubmitted transactions that were added to the mempool
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #36482 wallet: Only broadcast resubmitted transactions that were added to the mempool | Waiting on author | 0 | Mild | 15 + 148 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Adds a new broadcast mode, MEMPOOL_AND_BROADCAST_ADDED, to node transaction broadcast handling and switches periodic wallet rebroadcasts to use it. Transactions already present in the local mempool are retained without being rebroadcast to peers, while transactions that had dropped out and were re-added to the mempool are broadcast. The wallet resend functional test suite is refactored into isolated test methods and expanded to cover mempool-present rebroadcast suppression and wallet-load resubmission.
Problem: Periodic wallet rebroadcasts unconditionally announce unconfirmed transactions to any connected peers that have not yet received an inventory for them, even if the transaction has resided continuously in the node's mempool. This redundant relay signals to peers that the node has a special interest in the transaction, leaking the user's IP and wallet ownership.
Evidence of importance:
- PR description references issue #36403 ('Rethinking Transaction Rebroadcasting') and #3828: 'one of the issues with current rebroadcast behavior is that the rebroadcast begins an unconditional relay of a transaction to peers... there is still a privacy leak in this double relay of a transaction, especially so for transactions that are still in nodes' mempools.'
Scope: Most of the diff (+141/-63) is in `test/functional/wallet_resendwallettransactions.py`, refactoring a single procedural test into isolated helper methods and adding test coverage for load-time resubmissions and rebroadcast suppression.
Risk of a mistake: medium (wallet, p2p). A flaw in mempool detection or broadcast mode handling could prevent dropped wallet transactions from rebroadcasting to peers, leaving them unconfirmed until manual user intervention.
Reviewability
Paused · Waiting on author
Open objections from fametrano, mzumsande with no author reply for 7 days; review would repeat what has been asked.
Waiting on (from the thread read):
- author: response to mzumsande's objection regarding address probing 2026-10-09
- author: response to fametrano's testing results and suggestions 2026-10-25
Agreement
Mild
objection that needs an answer has none yet (mzumsande)
Thread notes: fametrano answered murchandamus's question by testing on regtest, confirming that transactions kept in the mempool are not rebroadcast after restart, and suggested test extensions and updating comments and log strings.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| mzumsande (member) | objection, safety | An attacker can use the replacement pattern to probe which nodes an address belongs to, leaving the receiver privacy issue from #3828 unresolved. | open | answered | no | 2026-10-09: 'This replacement pattern with `indep_utxo` is just the mechanism that an attacker could do to probe for which nodes a given address belong to, so this doesn't appear to completely solve the receiver i' |
| murchandamus (member) | question, correctness | Whether the combination of MEMPOOL_NO_BROADCAST on wallet loading and MEMPOOL_AND_BROADCAST_ADDED on rebroadcast could prevent a transaction from being resubmitted if the node was offline during a mempool spike that purged the transaction from the network | answered · fametrano confirmed with regtest tests that transactions retained in the local mempool are not reannounced after restart | answered | by others | 2026-10-21: 'The only open question I have at the moment is whether the combination of using `MEMPOOL_NO_BROADCAST` on wallet loading and then `MEMPOOL_AND_BROADCAST_ADDED` on the wallet’s rebroadcast could preven' Status from: 2026-10-25: fametrano: 'After a restart, this branch does not announce a wallet transaction that peers already received while our own mempool keeps it, so the answer to murch's question is yes.' |
| fametrano (contributor) | suggestion, maintenance | Extend test_resubmit_on_load past the first periodic resubmission to pin the intended rebroadcast behaviour | open | answered | no | 2026-10-25: 'test_resubmit_on_load stops before the first periodic resubmission; extending it would pin whichever behaviour is intended.' |
| fametrano (contributor) | suggestion, maintenance | Update the comment above ResubmitWalletTransactions referencing MEMPOOL_AND_BROADCAST_TO_ALL and adjust the log message counting unconfirmed transactions | open | answered | no | 2026-10-25: 'Smaller: the comment above ResubmitWalletTransactions still names MEMPOOL_AND_BROADCAST_TO_ALL for the periodic path, and "resubmit N unconfirmed transactions" now counts transactions not relayed.' |
Support:
- polespinasa (member) (Concept ACK, 2026-10-08): (no reason given) [not substantive]
- murchandamus (member) (2026-10-21) [read]: This is an obvious privacy improvement.
Participants:
- polespinasa (member, support, 1 statements 2026-10-08 to 2026-10-08): Concept ACK with no rationale
- mzumsande (member, objection, 1 statements 2026-10-09 to 2026-10-09): points out an attacker can use this replacement pattern to probe node addresses, leaving the receiver issue open
- murchandamus (member, support, 1 statements 2026-10-21 to 2026-10-21): praises the privacy improvement while asking whether transactions could fail to rebroadcast after being offline
- fametrano (contributor, neutral, 1 statements 2026-10-25 to 2026-10-25): tested rebroadcast behavior, answered murchandamus's question, and suggested test and comment updates
Files
211 lines under test/bench/ci.
- test/functional/wallet_resendwallettransactions.py +141/-63
- src/node/transaction.cpp +9/-1
- test/functional/test_framework/p2p.py +7/-0
- src/wallet/wallet.cpp +4/-1
- src/node/types.h +2/-0
Card
The short text the ranking pass compares across PRs.
Restricts periodic wallet transaction rebroadcasting to only announce transactions to peers if they were missing from the mempool and successfully re-added. Previously, unconfirmed transactions were unconditionally rebroadcast to peers that had not yet seen an inv, even when already present in the local mempool, leaking transaction ownership and node IP. Addresses privacy leak concerns raised in issue #36403.
Data
thread read through 2026-10-25 (4 statements), head 4dbd27f37b
labels: Wallet
Categories
Wallet
P2 · bug fix
Fixes a wallet privacy leak where periodic transaction rebroadcasts unconditionally relay transactions already resident in the local mempool. As noted in the description citing issue #36403, redundant rebroadcasts leak user IP and transaction ownership by signaling to peers that the node has special interest in those transactions.
Membership: Has the Wallet label, modifies wallet rebroadcast logic in src/wallet/wallet.cpp, and updates wallet_resendwallettransactions.py.
Factors: security/stability 1, bug 1, performance 0, user value 2, leverage 0