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-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36330.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | andrewtoth |
| Stale ACK | w0xlt |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--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.
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.
@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()
ACK 704238cf4f110e5c900d1240769dc673e88c46d3