This doesn't fix the general case of rebroadcasting potentially surprising users by divulging the originator IP trivially, but for users opting into private broadcast, it seems a bridge too far for them to expect the current behavior.
e.g. an incoming dust payment triggering your node to rebroadcast 24-36th later over clearnet
This can be removed if/when wallet drives through private broadcast.
see #3828 for running broader issue
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | davidgumberg, andrewtoth, w0xlt |
| Concept NACK | vasild, polespinasa |
| Concept ACK | darosior |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
ACK b7ea5786968f8cf9a0fe82628f9627fd27ec048b
but for users opting into private broadcast, it seems a bridge too far for them to expect the current behavior.
Is it? 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 behaves in the same way.
See also: #34533 (comment)
The current behavior is that all transactions being sent via the wallet do not use private broadcast at all
This is true, but the case this change is protecting against is transactions sent by other means that also belong to the wallet.
So if a user is running with -privatebroadcast and is sending transactions via sendrawtransaction, if they have a watch only wallet or their addresses are dusted with low fee transactions, then the wallet will still rebroadcast these transactions. This could be unexpected if the user thinks they are protected by -privatebroadcast.
1951 | + // back from the network. Announcing it again from this node's own connections, long after 1952 | + // every other node went quiet about it, would mark it as this node's: keep it in the mempool 1953 | + // without announcing it. 1954 | + const auto broadcast_method{context.args->GetBoolArg("-privatebroadcast", DEFAULT_PRIVATE_BROADCAST) 1955 | + ? node::TxBroadcast::MEMPOOL_NO_BROADCAST 1956 | + : node::TxBroadcast::MEMPOOL_AND_BROADCAST_TO_ALL};
if (context.args->GetBoolArg("-privatebroadcast", DEFAULT_PRIVATE_BROADCAST)) {
return;
}
IMO should return here without ever being resubmitted to the mempool since learning a peer's mempool is trivial for an active observer. @andrewtoth @vasild
right, if it comes back, then is evicted from the mempool, we don't want to resubmit to the mempool. I think this is correct.
pushed an update so the wallet never resubmits when pb is set. Avoids restarts and loading wallets triggering it
Is this intended for backport?
I ask because if we were not going to backport, then we could instead fix this by having the wallet use private broadcast. But for a backport fix this would be sufficient for existing users, with the tradeoff of reducing reliability of wallet transactions confirming a little bit.
@andrewtoth Given that wallet integration isn't close to done, I think we should remove footguns until we get there. This is easily reversed. I think backporting makes sense for the same reason.
156 | @@ -157,6 +157,39 @@ def run_test(self): 157 | node1.mockscheduler(60) 158 | peer.wait_for_broadcast([recv_wtxid]) 159 | 160 | + self.log.info("With -privatebroadcast, the wallet never re-adds a transaction to the mempool") 161 | + node1.replace_in_config([("connect=0\n", "")]) # -privatebroadcast refuses -connect 162 | + privbcast_args = ["-privatebroadcast", "-onion=127.0.0.1:9", "-mempoolexpiry=1"] # the proxy is never used here
I think we can use -persistmempool=0 here instead of -mempoolexpiry=1 to make this more concise with identical coverage:
diff --git a/test/functional/wallet_resendwallettransactions.py b/test/functional/wallet_resendwallettransactions.py
index 69760ad0d0..36715cd007 100755
--- a/test/functional/wallet_resendwallettransactions.py
+++ b/test/functional/wallet_resendwallettransactions.py
@@ -159,35 +159,21 @@ class ResendWalletTransactionsTest(BitcoinTestFramework):
self.log.info("With -privatebroadcast, the wallet never re-adds a transaction to the mempool")
node1.replace_in_config([("connect=0\n", "")]) # -privatebroadcast refuses -connect
- privbcast_args = ["-privatebroadcast", "-onion=127.0.0.1:9", "-mempoolexpiry=1"] # the proxy is never used here
+ privbcast_args = ["-privatebroadcast", "-onion=127.0.0.1:9", "-persistmempool=0"] # the proxy is never used here
self.restart_node(1, extra_args=privbcast_args + [f"-mocktime={node1.mocktime}"])
- # Resending needs a recent tip (the restart put node1 back in IBD) and a block seen since startup
- block = create_block(int(node1.getbestblockhash(), 16), height=node1.getblockcount() + 1, ntime=node1.mocktime)
- block.solve()
- node1.submitblock(block.serialize().hex())
- node1.syncwithvalidationinterfacequeue()
- # Expire the transaction so that a resubmit would visibly re-add it
- node1.bumpmocktime(2 * 60 * 60)
- node1.sendtoaddress(node1.getnewaddress(), 1)
assert recv_txid not in node1.getrawmempool()
self.log.info("The periodic resend does not re-add it")
- peer = node1.add_p2p_connection(P2PTxInvStore())
- with node1.assert_debug_log(expected_msgs=[], unexpected_msgs=['resubmit']):
- node1.bumpmocktime(RESEND_TIMER_LIMIT)
- node1.mockscheduler(60)
- node1.syncwithvalidationinterfacequeue() # runs on the scheduler thread, after the resend
- assert recv_txid not in node1.getrawmempool()
- node1.bumpmocktime(10 * 60) # past the peer's announcement timer
- peer.sync_with_ping()
- assert int(recv_wtxid, 16) not in peer.get_invs()
-
- self.log.info("Loading the wallet does not re-add it either")
- self.restart_node(1, extra_args=privbcast_args + [f"-mocktime={node1.mocktime}"])
+ # Resending needs a recent tip (the restart put node1 back in IBD) and a block seen since startup
+ self.generate(node1, 1, sync_fun=self.no_op)
+ node1.syncwithvalidationinterfacequeue()
+ node1.bumpmocktime(RESEND_TIMER_LIMIT)
+ node1.mockscheduler(60)
+ node1.syncwithvalidationinterfacequeue()
assert recv_txid not in node1.getrawmempool()
self.log.info("Without -privatebroadcast, loading the wallet re-adds it")
- self.restart_node(1, extra_args=["-mempoolexpiry=1", f"-mocktime={node1.mocktime}"])
+ self.restart_node(1, extra_args=["-persistmempool=0", f"-mocktime={node1.mocktime}"])
assert recv_txid in node1.getrawmempool()
largely took, thanks
ACK 704238cf4f110e5c900d1240769dc673e88c46d3
Concept ACK.
163 | + self.restart_node(1, extra_args=privbcast_args + [f"-mocktime={node1.mocktime}"]) 164 | + assert recv_txid not in node1.getrawmempool() # loading the wallet did not re-add it 165 | + 166 | + self.log.info("The periodic resend does not re-add it") 167 | + # Resending needs a recent tip (the restart put node1 back in IBD) and a block seen since startup 168 | + self.generate(node1, 1, sync_fun=self.no_op)
nit:
This second test that tx's are not readded at the resubmit timer is dependent on the test above, e.g. consider this diff:
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 93851a8736..8d15b02f37 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -1917,7 +1917,7 @@ void CWallet::ResubmitWalletTransactions(node::TxBroadcast broadcast_method, boo
// Don't attempt to resubmit if the wallet is configured to not broadcast,
// even if forcing.
if (!fBroadcastTransactions) return;
- if (m_private_broadcast) return;
+ // if (m_private_broadcast) return;
int submitted_tx_count = 0;
diff --git a/test/functional/wallet_resendwallettransactions.py b/test/functional/wallet_resendwallettransactions.py
index e4afad1d42..8f1da010e3 100755
--- a/test/functional/wallet_resendwallettransactions.py
+++ b/test/functional/wallet_resendwallettransactions.py
@@ -161,7 +161,8 @@ class ResendWalletTransactionsTest(BitcoinTestFramework):
node1.replace_in_config([("connect=0\n", "")]) # -privatebroadcast refuses -connect
privbcast_args = ["-privatebroadcast", "-onion=127.0.0.1:9", "-persistmempool=0"] # the proxy is never used here
self.restart_node(1, extra_args=privbcast_args + [f"-mocktime={node1.mocktime}"])
- assert recv_txid not in node1.getrawmempool() # loading the wallet did not re-add it
+ # [first test disabled]
+ # assert recv_txid not in node1.getrawmempool() # loading the wallet did not re-add it
self.log.info("The periodic resend does not re-add it")
# Resending needs a recent tip (the restart put node1 back in IBD) and a block seen since startup
Then this second check passes (since the generate line included the tx in a block), maybe add a (redundant) check here, e.g.:
assert recv_txid not in node1.getrawmempool()
self.generate(node1, 1, sync_fun=self.no_op)
maybe spurious [why would somebody delete or move one of the tests], but IMO these long winded functional tests where each of the checks are not scoped and there's stateful stuff are hard enough to read and even though it's obvious to us reviewing this now, to my grug brain at least it would not be obvious that the properties this claims to test are only there if the test above also passes
I had it mine an empty block, then did the assertion it's not in mempool. that help grug? (I agree these should all be subtests split off, easy cleanup task)
I ask because if we were not going to backport, then we could instead fix this by having the wallet use private broadcast.
I think that promoting private broadcast into an essential feature (and with it, connectivity to privacy networks) that is required for basic functionality such as rebroadcasting is a major change that needs to be very well discussed, to say the least.
As of now I view it as an optional feature.
But as such, I could see users starting with -privatebroadcast just to send out some transactions, and then restarting back to regular mode without private broadcast with a wallet that rebroadcasts them. This could reveal the origin of their transactions after the fact, so there is only so much this PR can achieve.
156 | @@ -157,6 +157,25 @@ def run_test(self): 157 | node1.mockscheduler(60) 158 | peer.wait_for_broadcast([recv_wtxid]) 159 | 160 | + self.log.info("With -privatebroadcast, the wallet never re-adds a transaction to the mempool") 161 | + node1.replace_in_config([("connect=0\n", "")]) # -privatebroadcast refuses -connect
nit: consider cleaning this up so the test is less stateful:
diff --git a/test/functional/wallet_resendwallettransactions.py b/test/functional/wallet_resendwallettransactions.py
index e4afad1d42..3367468148 100755
--- a/test/functional/wallet_resendwallettransactions.py
+++ b/test/functional/wallet_resendwallettransactions.py
@@ -158,7 +158,7 @@ class ResendWalletTransactionsTest(BitcoinTestFramework):
peer.wait_for_broadcast([recv_wtxid])
self.log.info("With -privatebroadcast, the wallet never re-adds a transaction to the mempool")
- node1.replace_in_config([("connect=0\n", "")]) # -privatebroadcast refuses -connect
+ node1.replace_in_config([("connect=0\n", "#connect=0\n")]) # -privatebroadcast refuses -connect
privbcast_args = ["-privatebroadcast", "-onion=127.0.0.1:9", "-persistmempool=0"] # the proxy is never used here
self.restart_node(1, extra_args=privbcast_args + [f"-mocktime={node1.mocktime}"])
assert recv_txid not in node1.getrawmempool() # loading the wallet did not re-add it
@@ -173,6 +173,7 @@ class ResendWalletTransactionsTest(BitcoinTestFramework):
assert recv_txid not in node1.getrawmempool()
self.log.info("Without -privatebroadcast, loading the wallet re-adds it")
+ node1.replace_in_config([("#connect=0\n", "connect=0\n")]) # clean up the cfg changes made
self.restart_node(1, extra_args=["-persistmempool=0", f"-mocktime={node1.mocktime}"])
assert recv_txid in node1.getrawmempool()
taken
crACK https://github.com/bitcoin/bitcoin/commit/6d5450244b5539a454aee3f88152ef7c78e27055
It is unfortunate that restarting subjects a user to potentially losing privacy if a TX is not confirmed. I would personally be in favor of dropping wallet rebroadcasting completely, it is a massive privacy footgun for non-privatebroadcast users as well, for little benefit. At least, for little benefit that couldn't be provided with other solutions that are less harmful.
I think that promoting private broadcast into an essential feature (and with it, connectivity to privacy networks) that is required for basic functionality such as rebroadcasting is a major change that needs to be very well discussed
I was merely suggesting to have the wallet rebroadcast via private broadcast if -privatebroadcast was set, not fix the entire unexpected rebroadcasting issue by requiring -privatebroadcast. (Although I might have shared that idea in another channel, I am not advocating we do that here.)
Improving privacy is good, but I think this has taken an odd approach:
It adds a relationship between wallet behavior and -privatebroadcast. It is documented that -privatebroadcast does not affect the wallet.
It makes it impossible to have -privatebroadcast=1 and wallet rebroadcast. Wallet rebroadcast indeed looks bad wrt privacy, but it is so even without considering -privatebroadcast. There may be setups where the wallet rebroadcasts are not so bad (e.g. if the node is not directly connected to clearnet) and are desired. Wallet rebroadcasts shouldn't really be tied to the value of -privatebroadcast.
I think it is better to make wallet rebroadcasts separately controllable, independently of -privatebroadcast. Opened #36380 to explore that.
@vasild I don't think adding more knobs is the right direction, we already have -walletbroadcast which arguably could the a soft default with private broadcast on. Users could over-ride that and retain old behavior. But of course, users may turn the feature on, then turn it back off, rebroadcasting anyways.
It is documented that -privatebroadcast does not affect the wallet.
We can change documentation if we change behavior. Only matters if the change is correct/good/etc. @davidgumberg yeah, I am quite skeptical of the wallet rebroadcast feature, it's built around corner cases and is a massive leak. Would be willing to discuss alternatives in an issue regardless of how this PR goes.
The wallet re-adds its unconfirmed transactions to the mempool periodically
and whenever a wallet is loaded or its transactions are imported. 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.
With -privatebroadcast, skip the resubmission entirely. Transactions sent
through the wallet are still submitted and announced as before, since those
never went through private broadcast in the first place.
Would be willing to discuss alternatives in an issue
One such alternative has an issue open #30471.
@vasild I don't think adding more knobs is the right direction
Right, I am not convinced either. Maybe the existent option can be extended, see at the bottom of #36380 (comment)
we already have
-walletbroadcastwhich arguably could the a soft default with private broadcast on. Users could over-ride that and retain old behavior. But of course, users may turn the feature on, then turn it back off, rebroadcasting anyways.
I just don't like those parameter interactions - in general, they could be hard to reason about. So to have -privatebroadcast=1 soft set -walletbroadcast=0? To stop also the initial wallet broadcast, given that -privatebroadcast is not used by the wallet... sounds odd to me.
It is documented that -privatebroadcast does not affect the wallet.
We can change documentation if we change behavior. Only matters if the change is correct/good/etc.
Right. And it would be nice to have the wallet use the private broadcast for its transactions even if currently it is documented that it does not :)
the wallet rebroadcast feature ... is a massive leak
:100:
moving to draft due to lack of consensus
reACK 3544df277deb658f150ed44f4a5f758fd8ef42af
I think this is a simple and good enough fix that can be backported into 32.x and 31.x
Also see discussion in #36403.
Code Review ACK 3544df277deb658f150ed44f4a5f758fd8ef42af
reACK 3544df277deb658f150ed44f4a5f758fd8ef42af
The wallet transactions are initially sent to all peers and periodically rebroadcast. This has not changed in 10 years and was not changed by the introduction of private broadcast. The wallet does not use private broadcast.
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.
I see the idea here is to try to protect users that create transactions in the wallet, but do not broadcast them via the wallet and use sendrawtransaction RPC instead with private broadcast. For these users, that do not use the wallet initial send either, -walletbroadcast=0 fits very well. I think this should be resolved via some communication channel to advise people of that.
Concept NACK
This has not changed in 10 years and was not changed by the introduction of private broadcast
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.
I think this should be resolved via some communication channel to advise people of that.
I don't think handing users footguns and hoping they read the docs is the right approach. Ideally we'd remove / replace rebroadcasts entirely, then this particular coupling can disappear safely. #36403 (comment) for some discussion for those following at home.
I tend to concept NACK this although I cannot come with a better proposal that 100% fixes this right now.
I don't think disallowing re-broadcast is the right solution (I know this is proposed as a temporal fix). Although it is not perfect, I prefer something like #36482 which I consider a good half-patch that limits this issue to a really specific case were a transaction disappears from node's mempool and is when we should consider rebroadcast necessary (in its current implementation).
I don't think the incoming dust privacy attack is an argument for pushing this PR forward. That attack affects both, private and standard broadcast, and as I understand private broadcast, it improves your privacy when you send a transaction, not when you receive one. It helps hiding you are the origin of a transaction, but that doesn't mean that it makes impossible to know that you are the origin of a transaction. In other words, I think the dust-resubmit attack is worth fixing and we should fix it, but it is worth fixing for all cases, not only private broadcast. I don't feel disabling a useful feature such as rebroadcast is the right step here. We also shouldn't assume and say that private broadcast makes impossible to know the origin of a transaction, it does improve privacy and makes it more difficult and expensive to know.