89372213048adf37a47427112a1ff836ee84c50e doc: add release notes for 29415 (Vasil Dimov)
582016fa5f013817db650bbba0a40d9195c18e2e test: add unit test for the private broadcast storage (Vasil Dimov)
e74d54e04896a86cad4e4b1bd9641afcc3a026c2 test: add functional test for private broadcast (Vasil Dimov)
818b780a05db126dcfe7efe12c46c84b5cfc3de6 rpc: use private broadcast from sendrawtransaction RPC if -privatebroadcast is ON (Vasil Dimov)
eab595f9cf13f7cb1d25a0db51409535cfe053b1 net_processing: retry private broadcast (Vasil Dimov)
37b79f9c39db5a4a61d360a6a29c8853bb5c7ac0 net_processing: stop private broadcast of a transaction after round-trip (Vasil Dimov)
2de53eee742da11b0e3f6fc44c39f2b5b5929da1 net_processing: handle ConnectionType::PRIVATE_BROADCAST connections (Vasil Dimov)
30a9853ad35365af8545e8e766d75cf398968480 net_processing: move a debug check in VERACK processing earlier (Vasil Dimov)
d1092e5d48ce67bd517068550c78bfcab062a554 net_processing: modernize PushNodeVersion() (Vasil Dimov)
9937a12a2fd5a0033f37f4dda5d75bfc5f15c3b6 net_processing: move the debug log about receiving VERSION earlier (Vasil Dimov)
a098f37b9e240291077a7f440e9f57e61f30e158 net_processing: reorder the code that handles the VERSION message (Vasil Dimov)
679ce3a0b8df6e8cab07965301382d2036ef2368 net_processing: store transactions for private broadcast in PeerManager (Vasil Dimov)
a3faa6f944a672faccac5dd201c8d33a638d9091 node: extend node::TxBroadcast with a 3rd option (Vasil Dimov)
95c051e21051bd469fda659fe7c495d5e264d221 net_processing: rename RelayTransaction() to better describe what it does (Vasil Dimov)
bb49d26032c57714c62a4b31ff1fdd969751683f net: implement opening PRIVATE_BROADCAST connections (Vasil Dimov)
01dad4efe2b38b7a71c96b6222147f395e0c11d9 net: introduce a new connection type for private broadcast (Vasil Dimov)
94aaa5d31b6ff1d0122319fc70e70a7e27e1a0ba init: introduce a new option to enable/disable private broadcast (Vasil Dimov)
d6ee490e0a9a81b69a4751087918303163ba8869 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 89372213048adf37a47427112a1ff836ee84c50e
andrewtoth:
ACK 89372213048adf37a47427112a1ff836ee84c50e
pinheadmz:
ACK 89372213048adf37a47427112a1ff836ee84c50e
w0xlt:
ACK 8937221304 with nit https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2654849875
mzumsande:
re-ACK 89372213048adf37a47427112a1ff836ee84c50e
Tree-SHA512: d51dadc865c2eb080c903cbe2f669e69a967e5f9fc64e9a20a68f39a67bf0db6ac2ad682af7fa24ef9f0942a41c89959341a16ba7b616475e1c5ab8e563b9b96
6da6f503a6dd8de85780ca402f5202095aa8b6ea refactor: Let CCoinsViewCache::BatchWrite return void (TheCharlatan)
Pull request description:
CCoinsViewCache::BatchWrite always returns true if called from a backed cache, so just return void instead. Also return void from ::Sync and ::Flush.
This allows for dropping a FatalError condition and simplifying some dead error handling code a bit.
Since we now no longer exercise the "error path" when returning from `CCoinsView::BatchWrite`, make the method clear the cache instead. This should only be exercised by tests and not change production behaviour. This might slightly improve the coins_view fuzz test's ability to generate better coverage.
ACKs for top commit:
l0rinc:
ACK 6da6f503a6dd8de85780ca402f5202095aa8b6ea
andrewtoth:
re-ACK 6da6f503a6dd8de85780ca402f5202095aa8b6ea
achow101:
ACK 6da6f503a6dd8de85780ca402f5202095aa8b6ea
w0xlt:
ACK 6da6f503a6
Tree-SHA512: dfaa325b0cf8108910aebf1b27434aaddb639d10d860e96797c77ea42eca9035a54a7dc1d6a5d4eae2b75fcc9356206d3d5672243d2c906e80d19024c8b95408
76c092ff805833a9adf84f669f0455bc2e0bba8b wallet: warn against accidental unsafe older() import (Sjors Provoost)
592157b7594693da389e4bd9b2cdedbdba7556fc test: move SEQUENCE_LOCKTIME flags to script (Sjors Provoost)
Pull request description:
[BIP 379](https://github.com/bitcoin/bips/blob/master/bip-0379.md) ([Miniscript](https://bitcoin.sipa.be/miniscript/)) allows relative height and time locks that have no consensus meaning in [BIP 68](https://github.com/bitcoin/bips/blob/master/bip-0068.mediawiki) (relative timelocks) / [BIP 112](https://github.com/bitcoin/bips/blob/master/bip-0112.mediawiki) (`CHECKSEQUENCEVERIFY`). This is (ab)used by some protocols, e.g. [by Lightning to encode extra data](https://delvingbitcoin.org/t/exploring-extended-relative-timelocks/1818/23), but is unsafe when used unintentionally: `older(65536)` is equivalent to `older(1)`.
This PR emits a warning when `importdescriptors` contains such a descriptor.
The first commit makes `SEQUENCE_LOCKTIME` flags reusable by other tests.
The main commit adds the `ForEachNode` helper to `miniscript.h` which is then used in the `MiniscriptDescriptor` constructor to check for `Fragment::OLDER` with unsafe values. These are stored in `m_warnings`, which the RPC code then collects via `Warnings()`.
It adds both a unit and functional test.
---
A previous version of this PR prevented the import, unless the user opted in with an `unsafe` flag. It also used string parsing in the RPC code.
---
Based on:
- [x] https://github.com/bitcoin/bitcoin/pull/33914
ACKs for top commit:
pythcoiner:
reACK 76c092ff80
achow101:
ACK 76c092ff805833a9adf84f669f0455bc2e0bba8b
rkrux:
lgtm re-ACK 76c092ff805833a9adf84f669f0455bc2e0bba8b
brunoerg:
reACK 76c092ff805833a9adf84f669f0455bc2e0bba8b
Tree-SHA512: 8e944e499bd4a43cc27eeb889f262b499b9b07aa07610f4a415ccb4e34a9110f9946646f446a54ac5bf17494d8d96a89e4a1fa278385db9b950468f27283e17a
658d38106a397ca04b94d98de57afc4de140feaa policy: remove constant parameter from `IsWellFormedPackage` (Lőrinc)
Pull request description:
`IsWellFormedPackage()` already claims: "parents must appear before children." In practice the `require_sorted` argument was always passed as `true`, making the false-path dead code. It was introduced that way from the beginning in https://github.com/bitcoin/bitcoin/pull/28758/files#diff-f30090b30c9489972ee3f1181c302cf3a484bb890bade0fd7c9ca92ea8d347f6R79.
Remove the unused parameter, updating callers/tests.
ACKs for top commit:
billymcbip:
tACK 658d38106a397ca04b94d98de57afc4de140feaa
instagibbs:
ACK 658d38106a397ca04b94d98de57afc4de140feaa
Tree-SHA512: 8b86dda7e2e1f0d48947ff258f0a3b6ec60676f54d4b506604d24e15c8b6465358ed2ccf174c7620125f5cad6bfc4df0bc482d920e5fc4cd0e1d72a9b16eafa5
44e006d4383155f254f908ada91c2d9a7a65db6c [kernel] Expose reusable PrecomputedTransactionData in script valid (Josh Doman)
Pull request description:
This PR exposes a reusable `PrecomputedTransactionData` object in script validation using libkernel.
Currently, libkernel computes `PrecomputedTransactionData` each time `btck_script_pubkey_verify` is called, exposing clients to quadratic hashing when validating a transaction with multiple inputs. By externalizing `PrecomputedTransactionData` and making it reusable, libkernel can eliminate this attack vector.
I discussed this problem in [this issue](https://github.com/TheCharlatan/rust-bitcoinkernel/issues/46). The design of this PR is inspired by @sedited's comments.
The PR introduces three new APIs for managing the `btck_PrecomputedTransactionData` object:
```c
/**
* @brief Create precomputed transaction data for script verification.
*
* @param[in] tx_to Non-null.
* @param[in] spent_outputs Nullable for non-taproot verification. Points to an array of
* outputs spent by the transaction.
* @param[in] spent_outputs_len Length of the spent_outputs array.
* @return The precomputed data, or null on error.
*/
btck_PrecomputedTransactionData* btck_precomputed_transaction_data_create(
const btck_Transaction* tx_to,
const btck_TransactionOutput** spent_outputs, size_t spent_outputs_len) BITCOINKERNEL_ARG_NONNULL(1);
/**
* @brief Copy precomputed transaction data.
*
* @param[in] precomputed_txdata Non-null.
* @return The copied precomputed transaction data.
*/
btck_PrecomputedTransactionData* btck_precomputed_transaction_data_copy(
const btck_PrecomputedTransactionData* precomputed_txdata) BITCOINKERNEL_ARG_NONNULL(1);
/**
* Destroy the precomputed transaction data.
*/
void btck_precomputed_transaction_data_destroy(btck_PrecomputedTransactionData* precomputed_txdata);
```
The PR also modifies `btck_script_pubkey_verify` so that it accepts `precomputed_txdata` instead of `spent_outputs`:
```c
/**
* @brief Verify if the input at input_index of tx_to spends the script pubkey
* under the constraints specified by flags. If the
* `btck_ScriptVerificationFlags_WITNESS` flag is set in the flags bitfield, the
* amount parameter is used. If the taproot flag is set, the precomputed data
* must contain the spent outputs.
*
* @param[in] script_pubkey Non-null, script pubkey to be spent.
* @param[in] amount Amount of the script pubkey's associated output. May be zero if
* the witness flag is not set.
* @param[in] tx_to Non-null, transaction spending the script_pubkey.
* @param[in] precomputed_txdata Nullable if the taproot flag is not set. Otherwise, precomputed data
* for tx_to with the spent outputs must be provided.
* @param[in] input_index Index of the input in tx_to spending the script_pubkey.
* @param[in] flags Bitfield of btck_ScriptVerificationFlags controlling validation constraints.
* @param[out] status Nullable, will be set to an error code if the operation fails, or OK otherwise.
* @return 1 if the script is valid, 0 otherwise.
*/
int btck_script_pubkey_verify(
const btck_ScriptPubkey* script_pubkey,
int64_t amount,
const btck_Transaction* tx_to,
const btck_PrecomputedTransactionData* precomputed_txdata,
unsigned int input_index,
btck_ScriptVerificationFlags flags,
btck_ScriptVerifyStatus* status) BITCOINKERNEL_ARG_NONNULL(1, 3);
```
As before, an error is thrown if the taproot flag is set and `spent_outputs` is not provided in `precomputed_txdata` (or `precomputed_txdata` is null). For simple single-input non-taproot verification, `precomputed_txdata` may be null, and the kernel will construct the precomputed data on-the-fly.
Both the C++ wrapper and the test suite are updated with the new API. Tests cover both `precomputed_txdata` reuse and nullability.
Appreciate feedback on this concept / approach!
ACKs for top commit:
sedited:
Re-ACK 44e006d4383155f254f908ada91c2d9a7a65db6c
stringintech:
ACK 44e006d
Tree-SHA512: 1ed435173e6ff4ec82bc603194cf182c685cb79f167439a442b9b179a32f6c189c358f04d4cb56d153fab04e3424a11b73c31680e42b87b8a6efcc3ccefc366c
e44dec027ceec2a5f74b65636689a51833d78a94 add release note about supporing non-TRUC <minrelay txns (Greg Sanders)
1488315d76ee40b9d021b7d0ecd01207eee4a426 policy: Allow any transaction version with < minrelay (Greg Sanders)
Pull request description:
Prior to cluster mempool, a policy was in place that
disallowed non-TRUC transactions from being
TX_RECONSIDERABLE in a package setting if it was below
minrelay. This was meant to simplify reasoning about mempool
trimming requirements with non-trivial transaction
topologies in the mempool. This is no longer a concern
post-cluster mempool, so this is relaxed.
In effect, this makes 0-value parent transactions relayable
through the network without the TRUC restrictions and
thus the anti-pinning protections.
ACKs for top commit:
ajtowns:
ACK e44dec027ceec2a5f74b65636689a51833d78a94 - lgtm
ismaelsadeeq:
ACK e44dec027ceec2a5f74b65636689a51833d78a94
Tree-SHA512: 6fd1a2429c55ca844d9bd669ea797e29eca3f544f0b5d3484743d3c1cdf4364f7c7a058aaf707bcfd94b84c621bea03228cb39487cbc23912b9e0980a1e5b451
fa4cb13b52030c2e55c6bea170649ab69d75f758 test: [doc] Manually unify stale headers (MarcoFalke)
fa5f29774872d18febc0df38831a6e45f3de69cc scripted-diff: [doc] Unify stale copyright headers (MarcoFalke)
Pull request description:
Historically, the upper year range in file headers was bumped manually
or with a script.
This has many issues:
* The script is causing churn. See for example commit 306ccd4, or
drive-by first-time contributions bumping them one-by-one. (A few from
this year: https://github.com/bitcoin/bitcoin/pull/32008,
https://github.com/bitcoin/bitcoin/pull/31642,
https://github.com/bitcoin/bitcoin/pull/32963, ...)
* Some, or likely most, upper year values were wrong. Reasons for
incorrect dates could be code moves, cherry-picks, or simply bugs in
the script.
* The upper range is not needed for anything.
* Anyone who wants to find the initial file creation date, or file
history, can use `git log` or `git blame` to get more accurate
results.
* Many places are already using the `-present` suffix, with the meaning
that the upper range is omitted.
To fix all issues, this bumps the upper range of the copyright headers
to `-present`.
Further notes:
* Obviously, the yearly 4-line bump commit for the build system (c.f.
b537a2c02a9921235d1ecf8c3c7dc1836ec68131) is fine and will remain.
* For new code, the date range can be fully omitted, as it is done
already by some developers. Obviously, developers are free to pick
whatever style they want. One can list the commits for each style.
* For example, to list all commits that use `-present`:
`git log --format='%an (%ae) [%h: %s]' -S 'present The Bitcoin'`.
* Alternatively, to list all commits that use no range at all:
`git log --format='%an (%ae) [%h: %s]' -S '(c) The Bitcoin'`.
<!--
* The lower range can be wrong as well, so it could be omitted as well,
but this is left for a follow-up. A previous attempt was in
https://github.com/bitcoin/bitcoin/pull/26817.
ACKs for top commit:
l0rinc:
ACK fa4cb13b52030c2e55c6bea170649ab69d75f758
rkrux:
re-ACK fa4cb13b52030c2e55c6bea170649ab69d75f758
janb84:
ACK fa4cb13b52030c2e55c6bea170649ab69d75f758
Tree-SHA512: e5132781bdc4417d1e2922809b27ef4cf0abb37ffb68c65aab8a5391d3c917b61a18928ec2ec2c75ef5184cb79a5b8c8290d63e949220dbeab3bd2c0dfbdc4c5
1e94e562f76e6152dffb2a2d07dc3429137098b5 refactor: enable `readability-container-contains` clang-tidy rule (Lőrinc)
fd9f1accbda9e81b2c5290b2056b25f02152a607 Fix compilation for old Boost versions (Lőrinc)
Pull request description:
Replace the last few instances of `.count() != 0` and `.count() == 0` and bare `count()` patterns with the more expressive C++20 `.contains()` method:
* `std::set<std::string>` in `getblocktemplate` RPC;
* `std::map<std::string, ...>` in `transaction_tests`;
* other bare `std::unordered_set` and `std::map` count calls.
Also fixes https://github.com/bitcoin/bitcoin/issues/34101 by reverting `boost::multi_index::contains` calls not available in our minimum supported version.
With no remaining violations, enable the `readability-container-contains` clang-tidy check to prevent future regressions.
Follow-up to https://github.com/bitcoin/bitcoin/pull/33192
ACKs for top commit:
hebasto:
ACK 1e94e562f76e6152dffb2a2d07dc3429137098b5.
pablomartin4btc:
re-ACK 1e94e562f76e6152dffb2a2d07dc3429137098b5
janb84:
ACK 1e94e562f76e6152dffb2a2d07dc3429137098b5
rkrux:
re-ACK 1e94e562f76e6152dffb2a2d07dc3429137098b5
Tree-SHA512: d54a7821d319bf0d60b6c3a870917464a7d5b9279c6a86708c03a3516ec23bbf18f0e83de62b3b2b1607de96e1470f1144b4918d69a6c770e6b7e09863e7dbac
Replace the last few instances of `.count() != 0` and `.count() == 0` and `.count()` patterns with the more expressive C++20 `.contains()` method:
* `std::set<std::string>` in `getblocktemplate` RPC;
* `std::map<std::string, ...>` in `transaction_tests`;
* other bare `std::unordered_set` and `std::map` count calls.
With no remaining violations, enable the `readability-container-contains`
clang-tidy check to prevent future regressions.
With MergeLinearizations() gone and the LIMO-based Linearize() replaced by SFL, we do not
need a class (LinearizationChunking) that can maintain an incrementally-improving chunk
set anymore.
Replace it with a function (ChunkLinearizationInfo) that just computes the chunks as
SetInfos once, and returns them as a vector. This simplifies several call sites too.
This introduces a local RNG inside the SFL state, which is used to randomize
various decisions inside the algorithm, in order to make it hard to create
pathological clusters which predictably have bad performance.
The decisions being randomized are:
* When deciding what chunk to attempt to split, the queue order is
randomized.
* When deciding which dependency to split on, a uniformly random one is
chosen among those with higher top feerate than bottom feerate within
the chosen chunk.
* When deciding which chunks to merge, a uniformly random one among those
with the higher feerate difference is picked.
* When merging two chunks, a uniformly random dependency between them is
now activated.
* When making the state topological, the queue of chunks to process is
randomized.
This introduces a queue of chunks that still need processing, in both
MakeTopological() and OptimizationStep(). This is simultaneously:
* A preparation for introducing randomization, by allowing permuting the
queue.
* An improvement to the fairness of suboptimal solutions, by distributing
the work more fairly over chunks.
* An optimization, by avoiding retrying chunks over and over again which
are already known to be optimal.
This replaces the existing LIMO linearization algorithm (which internally uses
ancestor set finding and candidate set finding) with the much more performant
spanning-forest linearization algorithm.
This removes the old candidate-set search algorithm, and several of its tests,
benchmarks, and needed utility code.
The worst case time per cost is similar to the previous algorithm, so
ACCEPTABLE_ITERS is unchanged.
Rather than using an ad-hoc no-dependency copy of the graph when a potentially
non-topological linearization is needed in the clusterlin fuzz test, add this
directly as a feature in ReadLinearization().
This is preparation for a later commit where another use for such a function
is added.
This adds a data structure representing the optimization state for the spanning-forest
linearization algorithm (SFL), plus a fuzz test for its correctness.
This is preparation for switching over Linearize() to use this algorithm.
See https://delvingbitcoin.org/t/spanning-forest-cluster-linearization/1419 for
a description of the algorithm.
db2d39f642979f929261e5f1cd67f0c2f2ca045f fuzz: add subtest for re-downloading a previously pruned block (Eugene Siegel)
45f5b2dac330906368352a1c585183f0d75d779d fuzz: Add fuzzer for block index (Martin Zumsande)
c011e3aa542631a8857039df796ebf13a653e8a6 test: Wrap validation functions with TestChainstateManager (Martin Zumsande)
Pull request description:
This adds a fuzz target for the block index and various events in validation that interact with it.
It can create arbitrary tree-like structure of block indexes, simulating (so far) the following events:
- Adding a header
- Receiving the full block (may be valid or not)
- `ActivateBestChain()` - Reorging the chain to a new chain tip (possibly encountering invalid blocks on the way)
- Pruning a block in the best chain
- Receiving a previously pruned block again (`getblockfrompeer`)
It might be interesting / possible to extend this to more events, such as dealing with more than one chainstate (assumeutxo).
The test skips all actual validation of header/ block / transaction data by just simulating the outcome, and also doesn't interact with the data directory.
The main goal is to ensure the integrity of the block index tree in all fuzzed constellations, by calling `CheckBlockIndex()` at the end of each iteration.
Compared to #29158 this approach has a more limited scope (by skipping all actual validation), but it is fast - it doesn't do a full init sequence on each iteration, but "cleans up" after itself by resetting the global validation state after each iteration.
ACKs for top commit:
Crypt-iQ:
reACK db2d39f642
maflcko:
review ACK db2d39f642979f929261e5f1cd67f0c2f2ca045f 🍶
sedited:
Re-ACK db2d39f642979f929261e5f1cd67f0c2f2ca045f
Tree-SHA512: 76cd5f8f4d7d7258620b46d7438bad4508c3bdc98825b48b60f694b5a9838e2b2cf4967c0ead181f86f66f4939ddfe552471851b9d18f84f584c03dd7e09fc43
facd3d56ccbe2414a5f2b75be7132cd8b904f1e9 log: Use `__func__` for -logsourcelocations (MarcoFalke)
Pull request description:
The `-logsourcelocations` option was recently changed to print the full function signature, as a side-effect of moving toward `std::source_location` internally.
This is fine, but at least for me, it makes debugging functional test failures harder, because the log is just so massively verbose, with questionable benefit.
I think the historically used file name, line number, and plain `__func__` name are more than sufficient for `-logsourcelocations`.
So switch back to using that.
For reference, a verbose log may look like:
```
...
node0 2025-12-17T07:28:37.528146Z [init] [checkqueue.h:147] [CCheckQueue<T, R>::CCheckQueue(unsigned int, int) [with T = CScriptCheck; R = std::pair<ScriptError_t, std::__cxx11::basic_string<char> >]] Script verificatio
n uses 1 additional threads
...
```
I don't think there is value in printing stuff, like the (anon) namespace, the class template args, or the functionn (template) args. The following should be more than sufficient:
```
...
node0 2025-12-17T09:45:57.017122Z [init] [checkqueue.h:147] [CCheckQueue] Script verification uses 1 additional threads
...
ACKs for top commit:
ajtowns:
ACK facd3d56ccbe2414a5f2b75be7132cd8b904f1e9 -- those long signatures are terrible
stickies-v:
ACK facd3d56ccbe2414a5f2b75be7132cd8b904f1e9
Tree-SHA512: 22fd1f0074fc6e85754967f9219659f57c905005a2bea9176f0b439abec324d7e6c2f875c8951934a3b11ef7e9d7e38d5d5d307e2bd1e000bc27ee85635cd668
caf4843a59a9d2512d69f8fd88a9672112bd80ac fuzz: doc: remove any mention to address_deserialize_v2 (brunoerg)
Pull request description:
We don't have `address_deserialize_v2` target anymore since fac81affb527132945773a5315bd27fec61ec52f (we used to have `address_deserialize_v1_notime`, `address_deserialize_v1_withtime` and `address_deserialize_v2` but now we only have a single `address_deserialize` target) so it removes any mention to it.
ACKs for top commit:
maflcko:
review ACK caf4843a59a9d2512d69f8fd88a9672112bd80ac 🎾
marcofleon:
ACK caf4843a59a9d2512d69f8fd88a9672112bd80ac
Tree-SHA512: 539d69edbfe4ca11eb0701ed5c789ad81976e3e85e8a229e39e9dc1b1c72264f01d10a1c16d0a3bb4a354794412dc8b625298f4f72430905a00b65faeaa37d6b
d9319b06cf82664d55f255387a348135fd7f91c7 refactor: unify container presence checks - non-trivial counts (Lőrinc)
039307554eb311ce41648d1f9a12b543f480f871 refactor: unify container presence checks - trivial counts (Lőrinc)
8bb9219b6301215f53e43967d17445aaf1b81090 refactor: unify container presence checks - find (Lőrinc)
Pull request description:
### Summary
Instead of counting occurrences in sets and maps, the C++20 `::contains` method expresses the intent unambiguously and can return early on first encounter.
### Context
Applied clang‑tidy's [readability‑container‑contains](https://clang.llvm.org/extra/clang-tidy/checks/readability/container-contains.html) check, though many cases required manual changes since tidy couldn't fix them automatically.
### Changes
The changes made here were:
| From | To |
|------------------------|------------------|
| `m.find(k) == m.end()` | `!m.contains(k)` |
| `m.find(k) != m.end()` | `m.contains(k)` |
| `m.count(k)` | `m.contains(k)` |
| `!m.count(k)` | `!m.contains(k)` |
| `m.count(k) == 0` | `!m.contains(k)` |
| `m.count(k) != 1` | `!m.contains(k)` |
| `m.count(k) == 1` | `m.contains(k)` |
| `m.count(k) < 1` | `!m.contains(k)` |
| `m.count(k) > 0` | `m.contains(k)` |
| `m.count(k) != 0` | `m.contains(k)` |
> Note that `== 1`/`!= 1`/`< 1` only apply to simple [maps](https://en.cppreference.com/w/cpp/container/map/contains)/[sets](https://en.cppreference.com/w/cpp/container/set/contains) and had to be changed manually.
There are many other cases that could have been changed, but we've reverted most of those to reduce conflict with other open PRs.
-----
<details>
<summary>clang-tidy command on Mac</summary>
```bash
rm -rfd build && \
cmake -B build \
-DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" \
-DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" \
-DCMAKE_OSX_SYSROOT="$(xcrun --show-sdk-path)" \
-DCMAKE_C_FLAGS="-target arm64-apple-macos11" \
-DCMAKE_CXX_FLAGS="-target arm64-apple-macos11" \
-DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DBUILD_FOR_FUZZING=ON
"$(brew --prefix llvm)/bin/run-clang-tidy" -quiet -p build -j$(nproc) -checks='-*,readability-container-contains' | grep -v 'clang-tidy'
```
</details>
Note: this is a take 2 of https://github.com/bitcoin/bitcoin/pull/33094 with fewer contentious changes.
ACKs for top commit:
optout21:
reACK d9319b06cf82664d55f255387a348135fd7f91c7
sedited:
ACK d9319b06cf82664d55f255387a348135fd7f91c7
janb84:
re ACK d9319b06cf82664d55f255387a348135fd7f91c7
pablomartin4btc:
re-ACK d9319b06cf82664d55f255387a348135fd7f91c7
ryanofsky:
Code review ACK d9319b06cf82664d55f255387a348135fd7f91c7. I manually reviewed the full change, and it seems there are a lot of positive comments about this and no more very significant conflicts, so I will merge it shortly.
Tree-SHA512: e4415221676cfb88413ccc446e5f4369df7a55b6642347277667b973f515c3c8ee5bfa9ee0022479c8de945c89fbc9ff61bd8ba086e70f30298cbc1762610fe1
5ac35795206d252c9f464e967b84521ddaad38f1 refactor: Add compile-time-checked hex txid (rustaceanrob)
Pull request description:
Suggested by l0rinc as a comment in #34004.
There are tests that utilize `FromHex` that will only fail during runtime if malformed. Adds a compile time constructor that can be caught by LSPs.
ACKs for top commit:
l0rinc:
ACK 5ac35795206d252c9f464e967b84521ddaad38f1
maflcko:
review ACK 5ac35795206d252c9f464e967b84521ddaad38f1 🦎
rkrux:
crACK 5ac35795206d252c9f464e967b84521ddaad38f1
Tree-SHA512: b0bae2bf0b8cd8c9a90765a14c46146313cf8b224a29d58a253e65ca95c4205c0beddea9c49ae58901e72c8c5202b91695d074ffb1c48e448d2e5606eb1bd5b4
This imitates the use of the getblockfrompeer rpc.
Note that currently pruning is limited to blocks in the active chain.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
This fuzz target creates arbitrary tree-like structure of indices,
simulating the following events:
- Adding a header to the block tree db
- Receiving the full block (may be valid or not)
- Reorging to a new chain tip (possibly encountering invalid blocks on
the way)
- pruning
The test skips all actual validation of header/ block / transaction data
by just simulating the outcome, and also doesn't interact with the data directory.
The main goal is to test the integrity of the block index tree in
all fuzzed constellations, by calling CheckBlockIndex()
at the end of each iteration.
7e9de20c0c144f5ccea5efe6d90601dd72bc7461 fuzz: exercise `ComputeMerkleRoot` without mutated parameter (Lőrinc)
Pull request description:
The `mutated` parameter in `ComputeMerkleRoot` unlocks a different path that was always exercised in the fuzz test.
Adjusted to be fuzzer to pass `nullptr` as well to make sure that path is also tested: 24ed820d4f/src/consensus/merkle.cpp (L49-L53)
Follow-up to https://github.com/bitcoin/bitcoin/pull/33805#discussion_r2589073735
ACKs for top commit:
frankomosh:
ACK [7e9de20](7e9de20c0c)
hodlinator:
ACK 7e9de20c0c144f5ccea5efe6d90601dd72bc7461
sedited:
ACK 7e9de20c0c144f5ccea5efe6d90601dd72bc7461
Tree-SHA512: bf27029ac04003447b24a95544ec863f9ceca6c28d51ea811dd6ca2b412a2a780bb9fdbcdc82719f39dd710a746eb2446263e8377d67a8be52a1694571d03498
82be652e40ec7e1bea4b260ee804a92a3e05f496 doc: Improve ChainstateManager documentation, use consistent terms (Ryan Ofsky)
af455dcb39dbd53700105e29c87de5db65ecf43c refactor: Simplify pruning functions (TheCharlatan)
ae85c495f1b507ca5871ea98f5d884fccb15adba refactor: Delete ChainstateManager::GetAll() method (Ryan Ofsky)
6a572dbda92ceb8c5af379f51cf6f9b93fb5e486 refactor: Add ChainstateManager::ActivateBestChains() method (Ryan Ofsky)
491d827d5284ed984ee2b11daaee50321217eac5 refactor: Add ChainstateManager::m_chainstates member (Ryan Ofsky)
e514fe61168109bd467d7cb2ac7561442b17b5f6 refactor: Delete ChainstateManager::SnapshotBlockhash() method (Ryan Ofsky)
ee35250683ab9a395b70a0e90ebc68b1858387c7 refactor: Delete ChainstateManager::IsSnapshotValidated() method (Ryan Ofsky)
d9e82299fc4e45fbc0f5a34dcbb1d51397d0bd35 refactor: Delete ChainstateManager::IsSnapshotActive() method (Ryan Ofsky)
4dfe383912761669a968f8535ed43437da160ec8 refactor: Convert ChainstateRole enum to struct (Ryan Ofsky)
352ad27fc1b1b350c8dbeb26a9813b01025cad31 refactor: Add ChainstateManager::ValidatedChainstate() method (Ryan Ofsky)
a229cb9477e6622087241be7a105551d1329503b refactor: Add ChainstateManager::CurrentChainstate() method (Ryan Ofsky)
a9b7f5614c24fe6f386448604c325ec4fa6c98a5 refactor: Add Chainstate::StoragePath() method (Ryan Ofsky)
840bd2ef230ed0582fe33a90ec2636bfefa21709 refactor: Pass chainstate parameters to MaybeCompleteSnapshotValidation (Ryan Ofsky)
1598a15aedb9fd9c4e4a671785ebebf56fc1e072 refactor: Deduplicate Chainstate activation code (Ryan Ofsky)
9fe927b6d654e752dac82156e209e45d31b75779 refactor: Add Chainstate m_assumeutxo and m_target_utxohash members (Ryan Ofsky)
6082c84713f42f5fa66f9a76baef17e8ed231633 refactor: Add Chainstate::m_target_blockhash member (Ryan Ofsky)
de00e87548f7ddd623355b7094924b0387a36280 test: Fix broken chainstatemanager_snapshot_init check (Ryan Ofsky)
Pull request description:
This PR contains the first part of #28608, which tries to make assumeutxo code more maintainable, and improve it by not locking `cs_main` for a long time when the snapshot block is connected, and by deleting the snapshot validation chainstate when it is no longer used, instead of waiting until the next restart.
The changes in this PR are just refactoring. They make `Chainstate` objects self-contained, so for example, it is possible to determine what blocks to connect to a chainstate without querying `ChainstateManager`, and to determine whether a Chainstate is validated without basing it on inferences like `&cs != &ActiveChainstate()` or `GetAll().size() == 1`.
The PR also tries to make assumeutxo terminology less confusing, using "current chainstate" to refer to the chainstate targeting the current network tip, and "historical chainstate" to refer to the chainstate downloading old blocks and validating the assumeutxo snapshot. It removes uses of the terms "active chainstate," "usable chainstate," "disabled chainstate," "ibd chainstate," and "snapshot chainstate" which are confusing for various reasons.
ACKs for top commit:
maflcko:
re-review ACK 82be652e40ec7e1bea4b260ee804a92a3e05f496 🕍
fjahr:
re-ACK 82be652e40ec7e1bea4b260ee804a92a3e05f496
sedited:
Re-ACK 82be652e40ec7e1bea4b260ee804a92a3e05f496
Tree-SHA512: 81c67abba9fc5bb170e32b7bf8a1e4f7b5592315b4ef720be916d5f1f5a7088c0c59cfb697744dd385552f58aa31ee36176bae6a6e465723e65861089a1252e5
CCoinsViewCache::BatchWrite always returns true if called from a backed
cache, so just return void instead. Also return void from ::Sync and
::Flush.
This allows for dropping a FatalError condition and simplifying some
dead error handling code a bit.
Since we now no longer exercise the "error path" when returning from
`CCoinsView::BatchWrite`, make the method clear the cache instead. This
should only be exercised by tests and not change production behaviour.
This might slightly improve the coins_view fuzz test's ability to
generate better coverage.
Co-authored-by: l0rinc <pap.lorinc@gmail.com>
fa8a5d215c5a81a7282fd5dd1098f9d3fa40e5db log: Remove brittle and confusing LogPrintLevel (MarcoFalke)
fac24bbec85fc930900ff755192a9954c7c2e27c test: Clarify logging_SeverityLevels test (MarcoFalke)
f2731676619d14fa4527aa9086fb73078d20f26f ipc: separate log statements per level (stickies-v)
94c51ae540723f5f648477789c11f6395730ae05 libevent: separate log statements per level (stickies-v)
Pull request description:
`LogPrintLevel` has many issues:
* It encourages to log several levels in one source location. This is problematic, because all levels (even warnings and errors) will be rate limited equally for the same location.
* Its warning and error logs are specially formatted compared to all other warning and error logs in the codebase, making them harder to spot (both in the debug log and in the code).
* It is verbose to type and read.
* It is confusing, because the majority of code uses the `Log$LEVEL(...)` macros. Having less ways to achieve the same makes the code more consistent and easier to review.
Fix all issues by removing it
ACKs for top commit:
stickies-v:
re-ACK fa8a5d215c5a81a7282fd5dd1098f9d3fa40e5db
ajtowns:
ACK fa8a5d215c5a81a7282fd5dd1098f9d3fa40e5db
pablomartin4btc:
re-ACK fa8a5d215c5a81a7282fd5dd1098f9d3fa40e5db
Tree-SHA512: 9fbb04962d9c26e566338694a7725b3c0e88ef733322d890bcc6aeddb45266c754e7c885c69bbfebd1588cc09912c6784cfc00e69882f1271a8c87d201490478
Suggested by @l0rinc in #34004
Message by @l0rinc:
This adds a consteval constructor to transaction_identifier (Txid/Wtxid) to allow parsing hex strings at compile-time.
This replaces runtime FromHex checks in tests, ensuring that malformed hardcoded hashes cause build failures rather than runtime test failures.
Test variables are explicitly marked constexpr. This is required to workaround a regression in GCC 14 (Bug 117501) where the compiler incorrectly flags consteval initialization of non-constexpr variables as "statements with no effect".
GCC Bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117501
Reproducer: https://godbolt.org/z/xb5TMaPs6
Co-authored-by: l0rinc <pap.lorinc@gmail.com>
The test was a bit confusing, because it just referred to the "global
log level" without explicitly specifying what it is. The level is set
though the LogSetup constructor. However, it is easier to follow unit
tests, if they are self-contained. So just set the level to Debug
explicitly here.
Also, add a new debug_3 log, to further document the intended behavior
of the unit test.
Also, replace the LogPrintLevel with the shorter and exact replacements
LogTrace and LogDebug.
The test intends to verify that running `PostLinearize` a
second time on a tree-structured graph doesn't change the
result. But `PostLinearize` was being called on the original
variable, not the copy. So the check was comparing the
unmodified copy against itself, which is useless.
Fix by post-linearizing the correct variable.
The dependency graphs generated by this test can have holes
(unused indices) in them. This means some of the transactions
were skipped when using `depgraph_gen.TxCount()` as the upper
bound of the loop. Switch to using `depgraph.Positions()` to
correctly handle sparse graphs.
