This makes the code more consistent.
Also, use "using BenchFunction = ..." while touching the header.
Also, fixup the whitespace after and earlier scripted-diff.
Duplicate benchmarks with the same name are not supported. Expanding the
name with __LINE__ is confusing and brittle, because it makes duplication
bugs silent.
Fix this twofold:
* By enforcing unique benchmarks at compile-time and link-time. For
example, a link failure may now look like:
"mold: error: duplicate symbol: bench_runner_AddrManAdd"
* By enforcing unique benchmarks at run-time. This should never happen,
due to the build-failure, but a failure may look like:
"Assertion `benchmarks().try_emplace(std::move(name), std::move(func)).second' failed."
The Restore Wallet dialog rejects wallet names that are empty, but was
doing so silently. This is confusing, we should be presenting an error
message to the user.
After the normal optimization process finishes, and finds an optimal
spanning forest, run a second process (while computation budget remains)
to split chunks into minimal equal-feerate chunks.
8937221304 doc: add release notes for 29415 (Vasil Dimov)
582016fa5f test: add unit test for the private broadcast storage (Vasil Dimov)
e74d54e048 test: add functional test for private broadcast (Vasil Dimov)
818b780a05 rpc: use private broadcast from sendrawtransaction RPC if -privatebroadcast is ON (Vasil Dimov)
eab595f9cf net_processing: retry private broadcast (Vasil Dimov)
37b79f9c39 net_processing: stop private broadcast of a transaction after round-trip (Vasil Dimov)
2de53eee74 net_processing: handle ConnectionType::PRIVATE_BROADCAST connections (Vasil Dimov)
30a9853ad3 net_processing: move a debug check in VERACK processing earlier (Vasil Dimov)
d1092e5d48 net_processing: modernize PushNodeVersion() (Vasil Dimov)
9937a12a2f net_processing: move the debug log about receiving VERSION earlier (Vasil Dimov)
a098f37b9e net_processing: reorder the code that handles the VERSION message (Vasil Dimov)
679ce3a0b8 net_processing: store transactions for private broadcast in PeerManager (Vasil Dimov)
a3faa6f944 node: extend node::TxBroadcast with a 3rd option (Vasil Dimov)
95c051e210 net_processing: rename RelayTransaction() to better describe what it does (Vasil Dimov)
bb49d26032 net: implement opening PRIVATE_BROADCAST connections (Vasil Dimov)
01dad4efe2 net: introduce a new connection type for private broadcast (Vasil Dimov)
94aaa5d31b init: introduce a new option to enable/disable private broadcast (Vasil Dimov)
d6ee490e0a log: introduce a new category for private broadcast (Vasil Dimov)
Pull request description:
_Parts of this PR are isolated in independent smaller PRs to ease review:_
* [x] _https://github.com/bitcoin/bitcoin/pull/29420_
* [x] _https://github.com/bitcoin/bitcoin/pull/33454_
* [x] _https://github.com/bitcoin/bitcoin/pull/33567_
* [x] _https://github.com/bitcoin/bitcoin/pull/33793_
---
To improve privacy, broadcast locally submitted transactions (from the `sendrawtransaction` RPC) to the P2P network only via Tor or I2P short-lived connections, or to IPv4/IPv6 peers but through the Tor network.
* Introduce a new connection type for private broadcast of transactions with the following properties:
* started whenever there are local transactions to be sent
* opened to Tor or I2P peers or IPv4/IPv6 via the Tor proxy
* opened regardless of max connections limits
* after handshake is completed one local transaction is pushed to the peer, `PING` is sent and after receiving `PONG` the connection is closed
* ignore all incoming messages after handshake is completed (except `PONG`)
* Broadcast transactions submitted via `sendrawtransaction` using this new mechanism, to a few peers. Keep doing this until we receive back this transaction from one of our ordinary peers (this takes about 1 second on mainnet).
* The transaction is stored in peerman and does not enter the mempool.
* Once we get an `INV` from one of our ordinary peers, then the normal flow executes: we request the transaction with `GETDATA`, receive it with a `TX` message, put it in our mempool and broadcast it to all our existent connections (as if we see it for the first time).
* After we receive the full transaction as a `TX` message, in reply to our `GETDATA` request, only then consider the transaction has propagated through the network and remove it from the storage in peerman, ending the private broadcast attempts.
The messages exchange should look like this:
```
tx-sender >--- connect -------> tx-recipient
tx-sender >--- VERSION -------> tx-recipient (dummy VERSION with no revealing data)
tx-sender <--- VERSION -------< tx-recipient
tx-sender <--- WTXIDRELAY ----< tx-recipient (maybe)
tx-sender <--- SENDADDRV2 ----< tx-recipient (maybe)
tx-sender <--- SENDTXRCNCL ---< tx-recipient (maybe)
tx-sender <--- VERACK --------< tx-recipient
tx-sender >--- VERACK --------> tx-recipient
tx-sender >--- INV/TX --------> tx-recipient
tx-sender <--- GETDATA/TX ----< tx-recipient
tx-sender >--- TX ------------> tx-recipient
tx-sender >--- PING ----------> tx-recipient
tx-sender <--- PONG ----------< tx-recipient
tx-sender disconnects
```
Whenever a new transaction is received from `sendrawtransaction` RPC, the node will send it to a few (`NUM_PRIVATE_BROADCAST_PER_TX`) recipients right away. If after some time we still have not heard anything about the transaction from the network, then it will be sent to 1 more peer (see `PeerManagerImpl::ReattemptPrivateBroadcast()`).
A few considerations:
* The short-lived private broadcast connections are very cheap and fast wrt network traffic. It is expected that some of those peers could blackhole the transaction. Just one honest/proper peer is enough for successful propagation.
* The peers that receive the transaction could deduce that this is initial transaction broadcast from the transaction originator. This is ok, they can't identify the sender.
---
<details>
<summary>How to test this?</summary>
Thank you, @stratospher and @andrewtoth!
Start `bitcoind` with `-privatebroadcast=1 -debug=privatebroadcast`.
Create a wallet and get a new address, go to the Signet faucet and request some coins to that address:
```bash
build/bin/bitcoin-cli -chain="signet" createwallet test
build/bin/bitcoin-cli -chain="signet" getnewaddress
```
Get a new address for the test transaction recipient:
```bash
build/bin/bitcoin-cli -chain="signet" loadwallet test
new_address=$(build/bin/bitcoin-cli -chain="signet" getnewaddress)
```
Create the transaction:
```bash
# Option 1: `createrawtransaction` and `signrawtransactionwithwallet`:
txid=$(build/bin/bitcoin-cli -chain="signet" listunspent | jq -r '.[0] | .txid')
vout=$(build/bin/bitcoin-cli -chain="signet" listunspent | jq -r '.[0] | .vout')
echo "txid: $txid"
echo "vout: $vout"
tx=$(build/bin/bitcoin-cli -chain="signet" createrawtransaction "[{\"txid\": \"$txid\", \"vout\": $vout}]" "[{\"$new_address\": 0.00001000}]" 0 false)
echo "tx: $tx"
signed_tx=$(build/bin/bitcoin-cli -chain="signet" signrawtransactionwithwallet "$tx" | jq -r '.hex')
echo "signed_tx: $signed_tx"
# OR Option 2: `walletcreatefundedpsbt` and `walletprocesspsbt`:
# This makes it not have to worry about inputs and also automatically sends back change to the wallet.
# Start `bitcoind` with `-fallbackfee=0.00003000` for instance for 3 sat/vbyte fee.
psbt=$(build/bin/bitcoin-cli -chain="signet" walletcreatefundedpsbt "[]" "[{\"$new_address\": 0.00001000}]" | jq -r '.psbt')
echo "psbt: $psbt"
signed_tx=$(build/bin/bitcoin-cli -chain="signet" walletprocesspsbt "$psbt" | jq -r '.hex')
echo "signed_tx: $signed_tx"
```
Finally, send the transaction:
```bash
raw_tx=$(build/bin/bitcoin-cli -chain="signet" sendrawtransaction "$signed_tx")
echo "raw_tx: $raw_tx"
```
</details>
---
<details>
<summary>High-level explanation of the commits</summary>
* New logging category and config option to enable private broadcast
* `log: introduce a new category for private broadcast`
* `init: introduce a new option to enable/disable private broadcast`
* Implement the private broadcast connection handling on the `CConnman` side:
* `net: introduce a new connection type for private broadcast`
* `net: implement opening PRIVATE_BROADCAST connections`
* Prepare `BroadcastTransaction()` for private broadcast requests:
* `net_processing: rename RelayTransaction to better describe what it does`
* `node: extend node::TxBroadcast with a 3rd option`
* `net_processing: store transactions for private broadcast in PeerManager`
* Implement the private broadcast connection handling on the `PeerManager` side:
* `net_processing: reorder the code that handles the VERSION message`
* `net_processing: move the debug log about receiving VERSION earlier`
* `net_processing: modernize PushNodeVersion()`
* `net_processing: move a debug check in VERACK processing earlier`
* `net_processing: handle ConnectionType::PRIVATE_BROADCAST connections`
* `net_processing: stop private broadcast of a transaction after round-trip`
* `net_processing: retry private broadcast`
* Engage the new functionality from `sendrawtransaction`:
* `rpc: use private broadcast from sendrawtransaction RPC if -privatebroadcast is ON`
* New tests:
* `test: add functional test for private broadcast`
* `test: add unit test for the private broadcast storage`
</details>
---
**This PR would resolve the following issues:**
https://github.com/bitcoin/bitcoin/issues/3828 Clients leak IPs if they are recipients of a transaction
https://github.com/bitcoin/bitcoin/issues/14692 Can't configure bitocoind to only send tx via Tor but receive clearnet transactions
https://github.com/bitcoin/bitcoin/issues/19042 Tor-only transaction broadcast onlynet=onion alternative
https://github.com/bitcoin/bitcoin/issues/24557 Option for receive events with all networks, but send transactions and/or blocks only with anonymous network[s]?
https://github.com/bitcoin/bitcoin/issues/25450 Ability to broadcast wallet transactions only via dedicated oneshot Tor connections
https://github.com/bitcoin/bitcoin/issues/32235 Tor: TX circuit isolation
**Issues that are related, but (maybe?) not to be resolved by this PR:**
https://github.com/bitcoin/bitcoin/issues/21876 Broadcast a transaction to specific nodes
https://github.com/bitcoin/bitcoin/issues/28636 new RPC: sendrawtransactiontopeer
---
Further extensions:
* Have the wallet do the private broadcast as well, https://github.com/bitcoin/bitcoin/issues/11887 would have to be resolved.
* Have the `submitpackage` RPC do the private broadcast as well, [draft diff in the comment below](https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-2972293733), thanks ismaelsadeeq!
* Add some stats via RPC, so that the user can better monitor what is going on during and after the broadcast. Currently this can be done via the debug log, but that is not convenient.
* Make the private broadcast storage, currently in peerman, persistent over node restarts.
* Add (optional) random delay before starting to broadcast the transaction in order to avoid correlating unrelated transactions based on the time when they were broadcast. Suggested independently of this PR [here](https://github.com/bitcoin/bitcoin/issues/30471).
* Consider periodically sending transactions that did not originate from the node as decoy, discussed [here](https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2035414972).
* Consider waiting for peer's FEEFILTER message and if the transaction that was sent to the peer is below that threshold, then assume the peer is going to drop it. Then use this knowledge to retry more aggressively with another peer, instead of the current 10 min. See [comment below](https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3258611648).
* It may make sense to be able to override the default policy -- eg so submitrawtransaction can go straight to the mempool and relay, even if txs are normally privately relayed. See [comment below](https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3427086681).
* As a side effect we have a new metric available - the time it takes for a transaction to reach a random node in the network (from the point of view of the private broadcast recipient the tx originator is a random node somewhere in the network). This can be useful for monitoring, unrelated to privacy characteristics of this feature.
---
_A previous incarnation of this can be found at https://github.com/bitcoin/bitcoin/pull/27509. It puts the transaction in the mempool and (tries to) hide it from the outside observers. This turned out to be too error prone or maybe even impossible._
ACKs for top commit:
l0rinc:
code review diff ACK 8937221304
andrewtoth:
ACK 8937221304
pinheadmz:
ACK 8937221304
w0xlt:
ACK 8937221304 with nit https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2654849875
mzumsande:
re-ACK 8937221304
Tree-SHA512: d51dadc865c2eb080c903cbe2f669e69a967e5f9fc64e9a20a68f39a67bf0db6ac2ad682af7fa24ef9f0942a41c89959341a16ba7b616475e1c5ab8e563b9b96
`ChainstateManager::IsInitialBlockDownload()` is queried on hot paths and previously acquired `cs_main` internally, contributing to lock contention.
Cache the IBD status in `m_cached_is_ibd`, and introduce `ChainstateManager::UpdateIBDStatus()` to latch it once block loading has finished and the current chain tip has enough work and is recent.
Call the updater after tip updates and after `ImportBlocks()` completes.
Since `IsInitialBlockDownload()` no longer updates the cache, drop `mutable` from `m_cached_is_ibd` and only update it from `UpdateIBDStatus()` under `cs_main`.
Update the new unit test to showcase the new `UpdateIBDStatus()`.
Co-authored-by: Patrick Strateman <patrick.strateman@gmail.com>
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
Factor the chain tip work/recency check out of `ChainstateManager::IsInitialBlockDownload()` into a reusable `CChain::IsTipRecent()` helper, and annotate it as requiring `cs_main` since it's reading mutable state.
Also introduce a local `chainman_ref` in the kernel import-blocks wrapper to reduce repetition and keep follow-up diffs small.
`IsInitialBlockDownload` returns were also unified to make the followup move clean.
Co-authored-by: Patrick Strateman <patrick.strateman@gmail.com>
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
Rename and invert the internal IBD latch so the cached value directly matches `IsInitialBlockDownload()` (true while in IBD, then latched to false).
This is a behavior-preserving refactor to avoid double negatives.
Add a unit test that exercises the `IsInitialBlockDownload()` decision matrix by varying the cached latch, `BlockManager::LoadingBlocks()`, and tip work/recency inputs.
This documents the current latching behavior and provides a baseline for later refactors.
44b12cdb11 doc: add 433 (Pay to Anchor) to bips.md (Sebastian Falbesoner)
Pull request description:
See PR https://github.com/bitcoin/bips/pull/1982 (merged recently) and #30352 (first released in v28.0).
ACKs for top commit:
instagibbs:
ACK 44b12cdb11
glozow:
ACK 44b12cdb11
Tree-SHA512: 0389bfd9b57a354ef15eff726783c83969586a73de52d27b6d3b042dba14c1361af29303b6ce42330a841310811a81c5e45b5c7d25e307787f1cfbfd75ab3e62
`CCoinsViewCache::FetchCoin()` had special handling for a spent `Coin` returned by the parent view.
Production parents (`CCoinsViewCache` and `CCoinsViewDB`) do not return spent coins, so this path is unreachable.
Replace it with an `Assume(!coin.IsSpent())`, drop outdated documentation about spent+FRESH cache entries, and simplify `SanityCheck()` to assert the remaining possible state invariants.
This is safe because it does not change behavior for valid backends and will fail fast if the `GetCoin()` contract is violated.
Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
Use port=0 for dynamic port allocation in test framework components
to avoid "address already in use" errors from concurrent tests or
ports stuck in TIME_WAIT state from previous test runs.
Changes:
- socks5.py: Update conf.addr after bind() to reflect actual port
- p2p.py: Retrieve actual port after create_server() when port=0
- feature_proxy.py: Use port=0 for all SOCKS5 proxy servers
- feature_anchors.py: Use port=0 for onion proxy server
- No need to jump into the next subroutine when there is no conflict.
- This makes it clear why it is necessary to have two calls of
CheckMempoolPolicyLimts in both PackageMempoolChecks and after in
AcceptMultipleTransactionsInternal, there is a possibilty that we
we want to accept multiple transaction but they are not conflicting
with any in-mempool transaction, in that case also we want to check
that they do not bust the cluster limits.
2cade5d5d1 [miniminer] stop assuming ancestor fees >= self fees (glozow)
Pull request description:
These assertions exist to detect double-deducting values when we update descendants. However, negative fees are possible with `prioritisetransaction` so it doesn't make sense to check this.
Leave the check for sizes because those are never negative.
Fixes#34234
ACKs for top commit:
instagibbs:
ACK 2cade5d5d1
dergoegge:
utACK 2cade5d5d1
Tree-SHA512: 935bbc8bd9a0d508eea43bb49aa43c22735e3f2c1012598f6843e229c13b76e44f9fd3eb8b61c437fa0b32353b4e7b15afa3e31002bdfa382d3d711d16419fde
eeaf28dbe0 wallet: test: Failed migration cleanup (David Gumberg)
Pull request description:
Prior to https://github.com/bitcoin/bitcoin/pull/34156, an issue existed where if migration of a wallet with a relative pathname failed, the relatively specified path where the legacy wallet is would be deleted. This issue predates #32273, because the relative pathnames get stacked together, e.g. "../../", the copy conflict bug that caused migration to abort early instead of getting far enough to attempt clean-up that was fixed in #32273 is avoided.
This is a functional test demonstrating that we handle failed migration clean-up correctly for relatively-named wallets. To see the issue, you can backport this test onto 29.x: https://github.com/davidgumberg/bitcoin/tree/2026-01-07-rel-migration-test-backport
I've also added an absolute path failed migration cleanup test. WRT this and #34156, absolute paths exhibit similar behavior to unnamed wallets. Because of the name-conflict bug prior to #32273 an absolute-path migration would fail no matter what because migration would attempt to copy a file to a destination that already exists. But after #32273, absolute-path migration gets past there, and if it fails for some other reason, the same behavior that's fixed in #34156 occurs where the directory containing the wallet file is deleted.
ACKs for top commit:
achow101:
ACK eeaf28dbe0
furszy:
ACK eeaf28dbe0
rkrux:
lgtm ACK eeaf28dbe0
Tree-SHA512: ee366fe526d0328654a86c2e9e6f228ca81554c8f8a78c259fa7aab90f024f9e5694ecf3f1d188938355f4e6d351c5a6a8ad236701bdd0ce63005e5d42c15e15
An interrupt does not create a failure exit code during normal
operation. This should also be the case when interrupt is triggered
during initialization. However a failure exit code is currently returned
if an interrupt occurs during init. Fix this by making `AppInitMain` return
true instead of false, which further up the call stack sets the
`EXIT_FAILURE` code. Also add a check for the interrupt condition during
GUI startup.
301d9eea66 qt: Remove "Starting Block" from Peer Detail. Following Deprecation in `bitcoin#34197` (WakeTrainDev)
Pull request description:
the `startingheight` rpc field got deprecated in https://github.com/bitcoin/bitcoin/pull/34197
this pr removes it from peer detail
ACKs for top commit:
maflcko:
review lgtm ACK 301d9eea66
theStack:
ACK 301d9eea66
hebasto:
ACK 301d9eea66, I verified `forms/debugwindow.ui` using Qt Designer.
Tree-SHA512: b870b4cff8ead073a17d171c01c46fc7e750c0343b4578ffb63abc8f40b33abdf08beb6733fead5307ef5d48b078b60d29ac0e0e41190a98f50f92154f0878cf
They are exactly the same, but the descriptor utils should not prescribe
to use the FuzzBufferType. Using a dedicated type for them clarifies
that the utils are not tied to FuzzBufferType.
Also, while touching the lines, use `const` only where it is meaningful.
cbf0bd35bb test: migration, avoid backup name mismatch in default_wallet_failure (furszy)
Pull request description:
This is a possible test failure, pushing it in case the CI starts complaining.
The change affects only test code; no cpp logic is involved.
The `test_default_wallet_failure` migration test calls the function
`migrate_and_get_rpc()`, which sets the mock time internally. But, at the
same time, the test already caches the mock time value, to later use it
in the backup existence check.
Setting the mock time twice can lead to a name mismatch during the
mentioned check (diff timestamp == diff backup names), which could
cause the test to fail.
The fix is very simple, just need to call the migration RPC directly.
Since the test expects the migration to fail, `migrate_and_get_rpc()` is
unnecessary here. I'm surprised the CI hasn't complained about this yet.
ACKs for top commit:
achow101:
ACK cbf0bd35bb
bensig:
ACK cbf0bd35bb
Tree-SHA512: 10b43a491b8ad0c5bf53e423b7d7587fc631551bf5d598e145e1defe9d8e5786c0869a9aee26209e63ccafd828ece34fc40c75abe246c1301b9f17467d64ef28
The test calls migrate_and_get_rpc(), which sets mock time internally.
The caller caches a mock time value and later relies on it to predict the
backup filename, so setting the mock time again could cause a naming
mismatch.
Fix this by calling the migration RPC directly. Since the test expects the
migration to fail, migrate_and_get_rpc() is unnecessary here.
f78f6f1dc8 wallettool: do not use fs::remove_all in createfromdump cleanup (Ava Chow)
Pull request description:
As pointed out in https://github.com/bitcoin/bitcoin/pull/34156#issuecomment-3716728670, it is possible for `createfromdump` to also accidentally delete the entire wallets directory if the wallet name is the empty string and the dumpfile contains a checksum error.
This is also fixed by removing the files created by only removing the directory for named wallets, and avoiding the use of `fs::remove_all`.
ACKs for top commit:
waketraindev:
lgtm ACK f78f6f1dc8
polespinasa:
code review and tACK f78f6f1dc8
rkrux:
Code review and tACK f78f6f1dc8
willcl-ark:
ACK f78f6f1dc8
pablomartin4btc:
ACK f78f6f1dc8
Tree-SHA512: ff1e7668131ec3632c67d990c99e8fddff28605e7e553c7e20695e61017c88476c3636e22f2007e763a00d527e80e4d1d3d45409f6678d28729b8397430bfe7a
b7c34d08dd test: coverage for migration failure when last sync is beyond prune height (furszy)
82caa8193a wallet: migration, fix watch-only and solvables wallets names (furszy)
d70b159c42 wallet: improve post-migration logging (furszy)
f011e0f068 test: restorewallet, coverage for existing dirs, unnamed wallet and prune failure (furszy)
36093bde63 test: add coverage for unnamed wallet migration failure (furszy)
f4c7e28e80 wallet: fix unnamed wallet migration failure (furszy)
4ed0693a3f wallet: RestoreWallet failure, erase only what was created (furszy)
Pull request description:
Minimal fix for #34128.
The issue occurs during the migration of a legacy unnamed wallet
(the legacy "default" wallet). When the migration fails, the cleanup
logic is triggered to roll back the state, which involves erasing the
newly created descriptor wallets directories. Normally, this only
affects the parent directories of named wallets, since they each
reside in their own directories. However, because the unnamed
wallet resides directly in the top-level `/wallets/` folder, this
logic accidentally deletes the main directory.
The fix ensures that only the wallet.dat file of the unnamed wallet
is touched and restored, preserving the wallet in BDB format and
leaving the main `/wallets/` directory intact.
#### Story Line:
#32273 fixed a different set of issues and, in doing so, uncovered
this one.
Before the mentioned PR, backups were stored in the same directory
as the wallet.dat file. On a migration failure, the backup was then
copied to the top-level `/wallets/` directory. For the unnamed legacy
wallet, the wallet directory is the `/wallets/` directory, so the source
and destination paths were identical. As a result, we threw early in the
`fs::copy_file` call ([here](https://github.com/bitcoin/bitcoin/blob/29.x/src/wallet/wallet.cpp#L4572)) because the file already existed, as we
were trying to copy the file onto itself. This caused the cleanup logic
to abort early on and never reach the removal line.
#### Testing Notes:
Cherry-pick the test commit on top of master and run it. You will
see the failure and realize the reason by reading the test code.
ACKs for top commit:
achow101:
ACK b7c34d08dd
davidgumberg:
crACK b7c34d08dd
w0xlt:
ACK b7c34d08dd
willcl-ark:
ACK b7c34d08dd
Tree-SHA512: d0be14c0ed6417f999c3f2f429652c2407097d0cc18453c91653e57ae4b5375b327ad3b2553d9ea6ff46a3ae00cdbd5ab325b94eba763072c4fc5a773b85618b
- Refactor Descriptor::ToPrivateString() to allow descriptors with
missing private keys to be printed. Useful in descriptors with
multiple keys e.g tr() etc.
- The existing behaviour of listdescriptors is preserved as much as
possible, if no private keys are availablle ToPrivateString will
return false
