wallet: resubmit transactions with private broadcast if enabled#34533
wallet: resubmit transactions with private broadcast if enabled#34533vasild wants to merge 2 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. |
| #include <key.h> | ||
| #include <key_io.h> | ||
| #include <logging.h> | ||
| #include <net.h> |
There was a problem hiding this comment.
This is just for the constant DEFAULT_PRIVATE_BROADCAST. Is there a better place for it? It is used in the following files:
src/init.cpp
src/net.cpp
src/net_processing.h
src/rpc/mempool.cpp
src/wallet/wallet.cpp (new in this PR)
f3976df to
a374c53
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. |
pablomartin4btc
left a comment
There was a problem hiding this comment.
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 This is being discussed further here: #34457 (comment). As mentioned there, a possible alternative could be for the wallet to treat all |
There was a problem hiding this comment.
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.
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.
Hmm, as a user, this is what I would expect - if restarted with |
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.
a374c53 to
16cd713
Compare
|
|
w0xlt
left a comment
There was a problem hiding this comment.
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 16cd713
|
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 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. |
Correct. Possible solutions to that:
|
|
I believe #34707 fixes this CPFP issue. If both txs are submitted to private broadcast, then when the stale tx check occurs it will mark the child as mempool-rejected but not remove it from the queue. Once the parent makes it back into the mempool, the stale tx check will see that the child is now acceptable and begin broadcasting it again. |
Should this PR be drafted while those things are being worked on? |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
The wallet keeps track of transactions related to it and periodically rebroadcasts them every 12-36 hours if they are not mined.
If
-privatebroadcast=1and a transaction is submitted locally with thesendrawtransactionRPC 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.