The wallet keeps track of transactions related to it and periodically rebroadcasts them every 12-36 hours if they are not mined.
If -privatebroadcast=1 and a transaction is submitted locally with the sendrawtransaction RPC and it is related to the wallet, then the rebroadcasts would use the send-to-all method. Change that to use the private broadcast method.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | w0xlt, andrewtoth |
| Concept ACK | pablomartin4btc |
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
Reviewers, 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.
26@@ -27,6 +27,7 @@
27 #include <key.h>
28 #include <key_io.h>
29 #include <logging.h>
30+#include <net.h>
This is just for the constant DEFAULT_PRIVATE_BROADCAST. Is there a better place for it? It is used in the following files:
0src/init.cpp
1src/net.cpp
2src/net_processing.h
3src/rpc/mempool.cpp
4src/wallet/wallet.cpp (new in this PR)
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/actions/runs/21775023286/job/62830040562
LLM reason (✨ experimental): Linting Python code failed (ruff) due to an extraneous f-string without placeholders in wallet_resendwallettransactions.py.
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
Concept ACK.
This is necessary to support the private transaction broadcast mechanism introduced in #29415 when transactions are rebroadcast or resubmitted, which does not seem to be handled currently.
Concept ACK — though I am not sure this is the best long-term approach. However, it could work as a pragmatic short-term solution if we need something quickly.
One concern with the approach here is that if a user uses sendrawtransaction to submit a wallet-related transaction while -privatebroadcast is enabled and then restarts the node without -privatebroadcast, the transaction would be rebroadcast via mempool since there is no per-transaction tracking of how it was originally sent.
This is being discussed further here: #34457 (comment). As mentioned there, a possible alternative could be for the wallet to treat all IsFromMe() transactions not originated through CommitTransaction() as private by default (that PR introduces a private flag in CWalletTx::mapValue) and only allow rebroadcasting when -privatebroadcast is enabled.
One concern with the approach here is that if a user uses sendrawtransaction to submit a wallet-related transaction while -privatebroadcast is enabled and then restarts the node without -privatebroadcast, the transaction would be rebroadcast via mempool since there is no per-transaction tracking of how it was originally sent.
An attacker always has the alternative option to just create their own tx to dust the address - this would circumvent any transaction tracking, and would be more convenient to the attacker anyway because this way the attacker can choose the fee carefully - low enough to make rebroadcast very probable, and high enough to make mempool eviction unprobable.
This PR fixes this problem for both IsFromMe() and IsMine() in a way transaction tracking wouldn’t. However, I think the problem is concerning enough that it should also be solved for clearnet-only nodes (#3828)
This is being discussed further here: #34457 (comment). As mentioned there, a possible alternative could be for the wallet to treat all IsFromMe() transactions not originated through CommitTransaction() as private by default (that PR introduces a private flag in CWalletTx::mapValue) and only allow rebroadcasting when -privatebroadcast is enabled.
As argued above, I think this is not complete unless we also do something about IsMine() transactions.
I am not sure this is the best long-term approach
Why? I think wallet support for private broadcast includes this change, which only relates to re-broadcasts. I do not see this as a short-term solution. The rest of the wallet support, for transactions initiated from the wallet (I guess in #34457, I did not study it yet, but will) goes together with this.
One concern with the approach here is that if a user uses
sendrawtransactionto submit a wallet-related transaction while-privatebroadcastis enabled and then restarts the node without-privatebroadcast, the transaction would be rebroadcast via mempool since there is no per-transaction tracking of how it was originally sent.
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.
The log message "Submitting wtx %s" might be misleading because the
printed value is the txid, not wtxid.
Change that to print both: "txid=%s wtxid=%s".
Also, include the outcome of the submit which is useful for the one
caller that does not print the error string.
The wallet keeps track of transactions related to it and periodically
rebroadcasts them every 12-36 hours if they are not mined.
If `-privatebroadcast=1` and a transaction is submitted locally with the
`sendrawtransaction` RPC and it is related to the wallet, then the
rebroadcasts would use the send-to-all method. Change that to use the
private broadcast method.
a374c53b8d30ea60e56f07b45f0645f202109eff...16cd713073f45a5ddff1cdce73a7a8459bd8c618: rebase due to conflicts
I would expect that because I have switched off private broadcast.
In the other PR, I originally proposed tracking privately broadcast transactions in the wallet and only rebroadcasting them when -privatebroadcast is enabled again.
While this works, it adds state complexity (for example, if the user never restarts the node with -privatebroadcast, the transaction would never be rebroadcast).
The main motivation was to prevent unaware users from having their originally privately broadcast transactions later rebroadcast differently if the node is restarted without -privatebroadcast, since rebroadcasting is not a deliberate user action. However, it’s probably better to keep things simple.
ACK 16cd713073f45a5ddff1cdce73a7a8459bd8c618
Disclaimer: I have not reviewed the private broadcast code yet, so some of these concerns may not make sense.
I’m not convinced that this is ready for the wallet until there is support for private broadcasting of packages, and for the wallet to submit packages when rebroadcasting.
Specifically, the wallet may contain transactions that have ancestors and descendants, and these may be complex topologies. Now, the rebroadcasting logic is quite naive as it sends all of the unconfirmed transactions into the mempool one by one, which means that if each transaction could not be accepted on its own, it cannot get rebroadcast. The wallet does at least submit them in the correct order so that transactions that depend on other transactions in the wallet won’t be rejected for missing-inputs, unless an ancestor was rejected for some other reason.
But, I believe this is made worse if private broadcast is enabled. AFAICT, each private broadcast sends the transaction to a random node. This means that if there is a dependency, a child transaction is unlikely to be sent to the node that the parent was sent to, which means that it is unlikely to be accepted by the node it is sent to due to the parent being unknown. Although looking at the code, such a scenario would result in the child never being attempted to be rebroadcast as it would fail ProcessTransaction(test_accept=true) since the parent is not in the local mempool. At best this delays the rebroadcast of the child for up to 24 hours, at worst, the child can never get rebroadcast.
Here’s a specific scenario that I think would be problematic. Consider a typical CPFP - a low feerate parent and a high feerate child. Suppose that these happen to not be in the mempool for whatever reason and the wallet is trying to rebroadcast them.
Without private broadcast, the wallet will send both to the local mempool individually, first the parent, then the child. If the parent cannot be accepted, then the child won’t either and nothing gets broadcast. If the parent is accepted, then the child will be accepted as well and both transactions can get relayed as normal, and may even benefit from opportunistic 1p1c.
With private broadcast, first the parent is tested against the local mempool, then the child. If the parent would not be accepted, then neither get broadcast and it’s the same situation as with normal relay. But if the parent would be accepted, it gets queued to be sent off to a random peer. Then the child gets tested against the local mempool, and it fails because the parent is not in the local mempool. Once the parent gets relayed and it shows up in the local mempool, if it’s still there up to 24 hours later, the child gets broadcast and now it’s the normal relay scenario, just 24 hours later.
But, suppose the parent got relayed, but it falls out of the mempool before the next rebroadcast (highly dynamic fee environment, with the parent sitting just around minrelayfeerate). Now we’re back to square one and the parent has to get relayed again. Theoretically, this could cycle indefinitely and the transactions never get mined.