#36330 wallet: don't resubmit transactions under -privatebroadcast
| Reviewability | Reviews | Agreement | Size | |||||||
|---|---|---|---|---|---|---|---|---|---|---|
| #36330 wallet: don't resubmit transactions under -privatebroadcast | Waiting on author | 3 -2 | Blocked | 10 + 21 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
Disables unconfirmed wallet transaction resubmission when the node is configured with `-privatebroadcast`. The wallet checks this setting and skips re-adding unconfirmed transactions to the local mempool during startup, transaction imports, and scheduled periodic rebroadcasts. Initial broadcasts of newly created transactions remain unaffected.
Problem: When the wider network drops an unconfirmed transaction from mempools, the local wallet periodically re-submits it to the node's mempool and announces or serves it to peers on request. Users running with `-privatebroadcast` expect privacy protections against origin inference, but serving a locally retained transaction that no other peer holds exposes the node as the sender or receiver over clearnet.
Evidence of importance:
- Commit 6d5450244b: "A transaction that the rest of the network has dropped is then held by this node alone, and served to any peer that asks for it, which marks it as the node's own. Not announcing the re-added transaction does not hide this: any relay peer can fetch anything in the mempool once the next inv round has passed."
- PR description points to long-standing origin privacy issues: "e.g. an incoming dust payment triggering your node to rebroadcast 24-36th later over clearnet... see https://github.com/bitcoin/bitcoin/issues/3828 for running broader issue"
Risk of a mistake: low (wallet, tests). A bug could cause unconfirmed wallet transactions under -privatebroadcast to fail to reach the mempool after a restart until manually rebroadcast, but normal broadcast flows and non-privatebroadcast configurations are unaffected.
Changed since the previous assessment: No substantial changes to the approach or mechanism; the logic in `CWallet::ResubmitWalletTransactions` and configuration loading remains the same.
Reviewability
Paused · Waiting on author
Open objection from polespinasa with no author reply for 23 days; review would repeat what has been asked.
Waiting on (from the thread read):
- author: author to reply to polespinasa's objection against disabling rebroadcast 2026-10-09
- reviewer: vasild to respond to author's reply on feature interactions and footguns 2026-10-09
Agreement
Blocked
objection that needs a change, nobody has replied (polespinasa)
Thread notes: vasild Concept NACKed the PR in c:6074151790 arguing it cripples wallet rebroadcast, which the author answered in c:6081186947. polespinasa also objected / tended to Concept NACK in c:6084798078, which remains open.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| achow101 (member) | suggestion, approach | Questioned whether resubmit should differ from CommitTransaction, which does not use private broadcast | answered · andrewtoth explained that it protects against rebroadcast of transactions originating outside the wallet (sendrawtransaction, watch-only, dust) | answered | by others | 2026-09-24: 'The current behavior is that all transactions being sent via the wallet do not use private broadcast at all; `CommitTransaction` uses `MEMPOOL_AND_BROADCAST_TO_ALL`, and it follows that resubmit behav' Status from: 2026-09-24: andrewtoth: 'This is true, but the case this change is protecting against is transactions sent by other means that also belong to the' |
| davidgumberg (member) | objection, safety | Resubmitting transactions to the mempool still trivially reveals them to an active observer under -privatebroadcast | fixed · fix pushed · author agreed and pushed an update to never resubmit when pb is set | answered | yes | 2026-09-24: 'IMO should return here without ever being resubmitted to the mempool since learning a peer's mempool is trivial for an active observer.' Status from: 2026-09-25: instagibbs: 'pushed an update so the wallet never resubmits when pb is set. Avoids restarts and loading wallets triggering it' |
| andrewtoth (member) | question, approach | Asked whether the change is intended for backport, noting that full wallet private broadcast would be preferable if not backporting | answered · author replied that backporting makes sense since full wallet integration is not close to done | answered | yes | 2026-09-25: 'Is this intended for backport?' Status from: 2026-09-25: instagibbs: 'Given that wallet integration isn't close to done, I think we should remove footguns until we get there. This is easily ' |
| andrewtoth (member) | suggestion, approach | Use -persistmempool=0 instead of -mempoolexpiry=1 in wallet_resendwallettransactions.py to make the test more concise with identical coverage | fixed · fix pushed · author adopted the test suggestion in an update | answered | yes | 2026-09-27: 'I think we can use `-persistmempool=0` here instead of `-mempoolexpiry=1` to make this more concise with identical coverage' Status from: 2026-09-28: instagibbs: 'largely took, thanks' |
| vasild (member) | objection, approach | Tying wallet rebroadcast to -privatebroadcast contradicts documentation that -privatebroadcast does not affect the wallet and prevents having both -privatebroadcast and wallet rebroadcast | contested · vasild pushed back against parameter interactions between -privatebroadcast and wallet settings | answered | yes | 2026-09-29: 'It adds a relationship between wallet behavior and `-privatebroadcast`. It is documented that `-privatebroadcast` does not affect the wallet.' Status from: 2026-09-30: vasild: 'I just don't like those parameter interactions - in general, they could be hard to reason about.' |
| vasild (member) | objection, approach | Disabling wallet rebroadcast cripples the wallet due to an unrelated feature being enabled, whereas users sending via RPC should use -walletbroadcast=0 | answered · author replied that feature interactions exist and disabling footguns is better than relying on docs | changed | yes | 2026-10-09: 'The wallet rebroadcast is and has always been bad for privacy, but it has its use cases. This PR would cripple the wallet due to an unrelated feature being enabled.' Status from: 2026-10-09: instagibbs: 'Interactions between features exist and should not be ignored. It's a wallet issue, for sure, but for a wallet + pb user, it would be extremely surprising.' |
| polespinasa (member) | objection, approach | Disabling rebroadcast is the wrong solution as it removes a useful feature, whereas the dust-resubmit privacy attack affects standard broadcast too and should be fixed generally or limited to mempool evictions | open | changed | no | 2026-10-09: 'I don't feel disabling a useful feature such as rebroadcast is the right step here.' |
Support:
- andrewtoth (member) (2026-09-24) [read]: protecting against transactions sent by other means that also belong to the wallet (e.g., watch-only, dust) being unexpectedly rebroadcast under -privatebroadcast
- w0xlt (contributor) (ACK, 2026-09-24): (no reason given) [not substantive]
- andrewtoth (member) (ACK, 2026-09-27): (no reason given) [not substantive]
- davidgumberg (member) (ACK, 2026-09-28) [read]: reviewed commit 6d54502 and supported dropping wallet rebroadcast completely
- darosior (member) (Concept ACK, 2026-09-28): (no reason given) [not substantive]
- davidgumberg (member) (ACK, 2026-10-08) [read]: simple and good enough fix that can be backported into 32.x and 31.x
- andrewtoth (member) (ACK, 2026-10-08): Code Review ACK 3544df277deb658f150ed44f4a5f758fd8ef42af [not substantive]
- w0xlt (contributor) (ACK, 2026-10-08): reACK 3544df277deb658f150ed44f4a5f758fd8ef42af [not substantive]
Participants:
- achow101 (member, question, 2 statements 2026-09-24 to 2026-10-01): questioned why resubmit should differ from wallet broadcast under -privatebroadcast; opened #36403 for broader discussion
- andrewtoth (member, support, 7 statements 2026-09-24 to 2026-10-08): ACKed the PR and updates, supported addressing unexpected rebroadcast, and suggested simplifying the test
- davidgumberg (member, objection, 5 statements 2026-09-24 to 2026-10-08): objected that resubmitting to mempool leaks transactions, then ACKed the updates and supported backporting
- fanquake (member, neutral, 1 statements 2026-09-24 to 2026-09-24): pinged reviewers
- w0xlt (contributor, support, 2 statements 2026-09-24 to 2026-10-08): ACKed and reACKed without rationale
- darosior (member, support, 1 statements 2026-09-28 to 2026-09-28): Concept ACK
- mzumsande (member, neutral, 1 statements 2026-09-28 to 2026-09-28): commented that private broadcast is optional and restarting without it could still reveal transactions
- vasild (member, objection, 3 statements 2026-09-29 to 2026-10-09): Concept NACKed; objected that the change cripples wallet rebroadcast over an unrelated feature
- polespinasa (member, objection, 1 statements 2026-10-09 to 2026-10-09): tended to Concept NACK; objected to disabling useful rebroadcast functionality rather than fixing dust resubmission generally
Files
21 lines under test/bench/ci.
- test/functional/wallet_resendwallettransactions.py +21/-0
- src/wallet/wallet.cpp +7/-0
- src/wallet/wallet.h +3/-0
Card
The short text the ranking pass compares across PRs.
Suppresses unconfirmed wallet transaction resubmission to the mempool when running with -privatebroadcast. Currently, when the network evicts a transaction, a wallet periodically re-adds it to the local mempool and serves it to peers upon request, identifying the node as the transaction's originator or recipient over clearnet. Skipping resubmission on startup, import, and timer intervals prevents deanonymizing users who opted into private broadcast. Initial transaction broadcasts remain unaffected.
Data
thread read through 2026-10-09 (32 statements), head 3544df277d
labels: Wallet, Private Broadcast
Categories
P2P
P2 · bug fix
Resolves a privacy leak where wallet rebroadcasts identify the node as the origin over clearnet despite `-privatebroadcast`. As noted in the commit, holding and serving an evicted transaction that no other peer holds 'marks it as the node's own,' undermining the core privacy objective of private broadcast.
Membership: Matches the 'Private Broadcast' label hint and directly addresses transaction origin privacy on the P2P network.
Factors: security/stability 2, bug 2, performance 0, user value 1, leverage 0
Wallet
P2 · bug fix (ranking pass: Moved up to sit directly beside #34533, which addresses the identical defect by the opposite method.)
Prevents the wallet from undermining user privacy expectations by suppressing periodic resubmissions of unconfirmed transactions under `-privatebroadcast`. Leaving this unresolved allows unexpected wallet rebroadcasts (such as incoming dust payments) to trigger clearnet announcements.
Membership: Changes wallet resubmission logic in src/wallet/wallet.cpp and src/wallet/wallet.h, and carries the 'Wallet' label.
Factors: security/stability 2, bug 2, performance 0, user value 1, leverage 0