wallet: add private broadcast support for wallet transactions#34457
wallet: add private broadcast support for wallet transactions#34457w0xlt wants to merge 5 commits intobitcoin:masterfrom
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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. LLM Linter (✨ experimental)Possible places where named args for integral literals may be used (e.g.
2026-02-23 06:43:46 |
|
Concept ACK I don't think we need to skip rebroadcast if we initially created the transaction with private broadcast disabled. This is only important the other way - if we initially broadcast via private broadcast, but then restart with private broadcast disabled.
Ideally every node has this enabled in the future. But also, one cannot conclude this if they receive a tx both from a persistently connected node and then later via a private broadcast. This would actually make the original broadcast more private, since this would indicate that perhaps the originator is not actually the originator. Sending decoys will also help here. |
|
@andrewtoth Thanks for the insight about plausible deniability. Updated to only skip private→public (the other direction is now allowed):
|
mzumsande
left a comment
There was a problem hiding this comment.
Some conceptual thoughts:
- The rebroadcasting behavior will probably lead to frequent 3 minute timeouts. If the tx didn't get mined within ~24 hours due to insufficient fees, but remains in our and our peer's mempools during this time, the peers wont's send us a GETDATA to our repeated submission. However, I think that this would not be a huge problem because after 3 attempts, we'd abort during the retry because we check there if the tx is already in the mempool.
- while this improves the rebroadcast behavior, there is still the issue that transactions relevant for the wallet but submitted via RPC (
sendrawtransaction) will be received back from the network, added tomapWallet, and then rebroadcast over clearnet if the user restarted without-privatebroadcast. In master, these txns would be rebroadcast normally over clearnet even without a restart. - should something be done about feebumping behavior? I think a user sending via private broadcast, restarting without the flag and then bumping a tx would lead to the tx being sent via clearnet.
e22c774 to
da22821
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
Agreed.
As far as I can see there is no simple solution to this scenario. Even if we have a persistent database of privately broadcast transactions (like proposed in #34322) and the wallet could query it when a relevant transaction is received, the user can submit the transaction from a different node than the wallet's node. One approach would be for the wallet to mark all
Added a solution and a functional test for this case. Thanks for raising it. |
|
CI error is unrelated |
caad89c to
3645ccc
Compare
|
Concept ACK
I was worried about using this approach for one case:
Buut I modified the The only case where it might cause trouble is if a user already has the same wallet in both the original node and new node and then sends a private transaction from one of them. Then the other one will not know whether the transaction is private or not. It is a weird case, and I am not even sure it would behave like this (I have not tested it). I also left some comments in |
| self.restart_node(0, extra_args=[ | ||
| "-privatebroadcast=1", | ||
| "-v2transport=0", | ||
| "-listenonion", | ||
| "-torcontrol=127.0.0.1:1", | ||
| ]) |
There was a problem hiding this comment.
| self.restart_node(0, extra_args=[ | |
| "-privatebroadcast=1", | |
| "-v2transport=0", | |
| "-listenonion", | |
| "-torcontrol=127.0.0.1:1", | |
| ]) | |
| time.wait(0.1) | |
| self.restart_node(0, extra_args=[ | |
| "-privatebroadcast=1", | |
| "-v2transport=0", | |
| "-listenonion", | |
| "-torcontrol=127.0.0.1:1", | |
| ]) |
I had this error:
TestFramework.socks5 (ERROR): socks5 request handling failed.
Traceback (most recent call last):
File "/home/bitcoin/test/functional/test_framework/socks5.py", line 185, in handle
forward_sockets(self.conn, conn_to)
~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^
File "/home/bitcoin/test/functional/test_framework/socks5.py", line 68, in forward_sockets
data = s.recv(4096)
ConnectionResetError: [Errno 104] Connection reset by peer
After investigating, by putting the time.sleep(0.1) I was able to solve it. I believe the error is because the proxy does not have enough time to create the connexion.
PS: I am not sure if putting a time.sleep is the best approach.
| self.log.info("All wallet private broadcast tests passed!") | ||
|
|
There was a problem hiding this comment.
I had the same error after the log message Stopping nodes. I've solved it doing the same.
| self.log.info("All wallet private broadcast tests passed!") | |
| self.log.info("All wallet private broadcast tests passed!") | |
| time.sleep(0.1) |
@Bicaru20 Yes, in this case, the wallet that didn’t originate the transaction has no way of knowing how it was broadcast after receiving it through the mempool. That’s why I suggested that such a wallet, for example, should treat any transaction received via the mempool with owned inputs as private. Regarding the import, I assumed you were backing up the wallet file and restoring it. If you restore the wallet using |
Okay. I don't know why I understood another thing, my bad. Thanks for the clarification. I agree with your approach, then. It’s better to be much too private than too little.
Yes, I just did a quick test to see the behavior in this case. |
…on() Move the Tor/I2P network reachability check from the sendrawtransaction RPC to BroadcastTransaction(). This centralizes the check so that all callers (including wallet RPCs) get consistent behavior when using private broadcast. Changes: - Add TransactionError::PRIVATE_BROADCAST_UNAVAILABLE enum value - Add corresponding error message in common/messages.cpp - Add RPC error code mapping in rpc/util.cpp - Move the check from rpc/mempool.cpp to node/transaction.cpp - Set err_string when the error occurs for proper error propagation
CommitTransaction() can throw std::runtime_error (e.g., on wallet DB write failure), but sendButtonClicked() did not catch it, which would crash the GUI. Add a try-catch around sendCoins() to display the error in a message box instead.
Enable private broadcast for wallet transaction submission when -privatebroadcast is set. Changes to CommitTransaction(): - Fail early before AddToWallet() when -privatebroadcast is enabled but Tor/I2P is unreachable, so no wallet state is modified on permanent failure - Use NO_MEMPOOL_PRIVATE_BROADCAST method when -privatebroadcast enabled Changes to MaybeResendWalletTxs(): - Use private broadcast for resubmission when -privatebroadcast enabled, so periodic rebroadcasts also go through Tor/I2P
Add helper utilities to test_framework/socks5.py to reduce code duplication in private broadcast tests: - Socks5ProxyHelper: Simplifies SOCKS5 proxy setup with ephemeral ports, thread-safe destination tracking, and simple redirect factory - add_addresses_to_addrman(): Helper to populate a node's addrman with test addresses - FAKE_ONION_ADDRESSES: Shared list of fake .onion addresses for tests Update p2p_private_broadcast.py to use ephemeral ports and the new add_addresses_to_addrman helper.
Add a functional test for wallet-specific private broadcast behavior: - Test 1: sendtoaddress with -privatebroadcast sends via SOCKS5 proxy - Test 2: Error when Tor/I2P not reachable - Test 3: Rebroadcast with -privatebroadcast uses private broadcast method - Test 4: Rebroadcast without -privatebroadcast uses normal method Uses Socks5ProxyHelper for simplified proxy setup and connection tracking.
|
Force-pushed with a simplified approach (rationale: #34533 (review)). The per-transaction This simplification aligns with #34533 (comment) approach: "Hmm, as a user, this is what I would expect - if restarted with -privatebroadcast=0, then the wallet rebroadcasts should be done using the send-to-all method ("via the mempool"). I would expect that because I have switched off private broadcast." Changes against master (5 commits):
|
|
Why was this marked draft? |
|
I moved it to draft due to the discussion about the transaction private flag. It’s now marked as ready. |
| for (const std::shared_ptr<CWallet>& pwallet : GetWallets(context)) { | ||
| if (!pwallet->ShouldResend()) continue; | ||
| pwallet->ResubmitWalletTransactions(node::TxBroadcast::MEMPOOL_AND_BROADCAST_TO_ALL, /*force=*/false); | ||
| const bool private_broadcast{gArgs.GetBoolArg("-privatebroadcast", DEFAULT_PRIVATE_BROADCAST)}; |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
Based on an initial glance:
|
Summary
Extends the private broadcast feature (#29415) to wallet transactions. When
-privatebroadcast=1is enabled, wallet RPCs (sendtoaddress,send,sendall,sendmany) now broadcast transactions through short-lived Tor/I2P connections instead of announcing to all connected peers.EDIT: The per-transaction private_broadcast flag in mapValue of the original proposal has been removed (rationale: #34533 (review)).
Previous approach
Key Changes
Centralized availability check (commit 1)
sendrawtransactionRPC toBroadcastTransaction()Wallet integration (commit 2)
CommitTransaction(): Uses private broadcast when enabled, fails loudly if Tor/I2P unavailableResubmitWalletTransactions(): Rebroadcasts using the original broadcast methodprivate_broadcastflag in the wallet transaction'smapValueFunctional tests (commits 3-4)
Behavior Matrix
Why skip on mode mismatch? Rebroadcasting via a different method than originally used may allow correlation of transaction to origin, or may indicate origin has
-privatebroadcastenabled.