The caption was empty for all call-sites, so this refactor does not
change any behavior.
Note that noui_ThreadSafeMessageBoxRedirect is test-only, so no end-user
behavior is changed here.
There is only one call-site, which provided an empty caption.
Note that noui_ThreadSafeQuestionRedirect is test-only and currently
entrirely unused, so the logging format string change is not a behavior
change.
This refactor does not change any behavior.
48f57bb35bbdbce509b8ef195de69e2a61a2511e mining: add new getCoinbaseTx() returning a struct (Sjors Provoost)
d59b4cdb5772917ee13e48552d51662160104b62 mining: rename getCoinbaseTx() to ..RawTx() (Sjors Provoost)
Pull request description:
The first commit renames `getCoinbaseTx()` to `getCoinbaseRawTx()` to reflect that it returns a serialised transaction. This does not impact IPC clients, because they do not use the function name.
The second commit then introduces a replacement `getCoinbase()` that provides a struct with everything clients need to construct a coinbase. This avoids clients having to parse and manipulate our dummy transaction.
Deprecate but don't remove `getCoinbaseRawTx()`, `getCoinbaseCommitment()` and `getWitnessCommitmentIndex()`.
After this change we can drop these deprecated methods, which in turn would allow us to clear the dummy transaction from the `getBlock()` result. But that is left for a followup to keep this PR focussed. See https://github.com/Sjors/bitcoin/pull/106 for an approach.
Expand the `interface_ipc.py` functional test to document its usage.
Can be tested using:
- https://github.com/stratum-mining/sv2-tp/pull/59
ACKs for top commit:
ryanofsky:
Code review ACK 48f57bb35bbdbce509b8ef195de69e2a61a2511e. Just rebased and addressed comments and dropped coinbase tx "template" suffix, which is a nice change
ismaelsadeeq:
code review ACK 48f57bb35bbdbce509b8ef195de69e2a61a2511e
vasild:
ACK 48f57bb35bbdbce509b8ef195de69e2a61a2511e
Tree-SHA512: c4f1d752777fb3086a1a0b7b8b06e4205dbe2f3adb41f218855ad1dee952adccc263cf82acd3bf9300cc83c2c64cebd2b27f66a69beee32d325b9a85e3643b0d
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
Introduce a new method intended to replace getCoinbaseRawTx(), which
provides a struct with everything clients need to construct a coinbase.
This is safer than providing a raw dummy coinbase that clients then have
to manipulate.
The CoinbaseTx data is populated during the dummy transaction generation
and stored in struct CBlockTemplate.
Expand the interface_ipc.py functional test to document its usage
and ensure equivalence.
d7de5b109f69293b375729363b4e5038f3fb6b15 logs: show reindex progress in `ImportBlocks` (Lőrinc)
Pull request description:
### Summary
When triggering a reindex, users have no indication of progress.
### Fix
This patch precomputes the total number of block files so progress can be shown.
Instead of only displaying which block file is being processed, it now shows the percent complete.
### Reproducer + expected results
```bash
cmake -B build -DCMAKE_BUILD_TYPE=Release && make -C build -j && ./build/bin/bitcoind -datadir=demo -reindex
```
Before, the block files were shown one-by-one, there's no way to see how much work is left:
```
Reindexing block file blk00000.dat...
Loaded 119920 blocks from external file in 1228ms
Reindexing block file blk00001.dat...
Loaded 10671 blocks from external file in 284ms
Reindexing block file blk00002.dat...
Loaded 5459 blocks from external file in 263ms
Reindexing block file blk00003.dat...
Loaded 5595 blocks from external file in 267ms
```
After the change we add a percentage:
```
Reindexing block file blk00000.dat (0% complete)...
Loaded 119920 blocks from external file in 1255ms
Reindexing block file blk00001.dat (1% complete)...
Loaded 10671 blocks from external file in 303ms
Reindexing block file blk00002.dat (2% complete)...
Loaded 5459 blocks from external file in 278ms
Reindexing block file blk00003.dat (3% complete)...
Loaded 5595 blocks from external file in 285ms
```
ACKs for top commit:
enirox001:
Concept ACK d7de5b1
rkrux:
lgtm ACK d7de5b109f69293b375729363b4e5038f3fb6b15
danielabrozzoni:
tACK d7de5b109f69293b375729363b4e5038f3fb6b15 - code reviewed and tested on my archival node.
maflcko:
review ACK d7de5b109f69293b375729363b4e5038f3fb6b15 💇
Tree-SHA512: 359a539b781ad8b73e2a616c951567062a76be27cf90e5b88bb5309295af9cd7994e327f185bacc1482b43b892b38329593b4043a5e71d8800e3e4b7a3954310
Specifically gets rid of batchpriority, chainparams, script/sign.h and
system includes.
Also take the opportunity of cleaning up the headers for the effected
files and adding them to the iwyu-enforced set.
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
This frees up the name getCoinbaseTx() for the next commit.
Changing a function name does not impact IPC clients, as they only
consider the function signature and sequence number.
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
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
59b93f11e8600d5224359f4d05619c0f56aef1e6 rest: print also HTTP response reason in case of an error (Roman Zeyde)
7fe94a04934a89b63f1248cb46d59f0ab45439b5 rest: add a test for unsuported `/blockpart/` request type (Roman Zeyde)
55d0d19b5c02a65d8dfafd99f352769224ab51a4 rest: deduplicate `interface_rest.py` negative tests (Roman Zeyde)
89eb531024d9921f5c825d390b90c0a7bd3756cc rest: update release notes for `/blockpart/` endpoint (Roman Zeyde)
41118e17f87561afc8cbe1f3dd528624f06906a7 blockstorage: simplify partial block read validation (Roman Zeyde)
599effdeab4d6687da783de04f8edf1d88959169 rest: reformat `uri_prefixes` initializer list (Roman Zeyde)
Pull request description:
The commits below should resolve a few leftovers from #33657.
ACKs for top commit:
l0rinc:
ACK 59b93f11e8600d5224359f4d05619c0f56aef1e6
hodlinator:
re-ACK 59b93f11e8600d5224359f4d05619c0f56aef1e6
Tree-SHA512: ae45e08edd315018e11283b354fb32f9658f5829c956554dc662a81c2e16397def7c3700e6354e0a91ff03c850def35638a69ec2668b7c015d25d6fed42b92bb
Extend `PeerManager` with a transaction storage and a new method
`InitiateTxBroadcastPrivate()` which:
* adds a transaction to that storage and
* calls `CConnman::PrivateBroadcast::NumToOpenAdd()` to open dedicated
privacy connections that will pick an entry from the transaction
storage and broadcast it.
Extend `node::TxBroadcast` with a 3rd option to not add the
transaction to the mempool and broadcast privately.
This is a non-functional change - `BroadcastTransaction()` will not
do anything if the 3rd options is passed and is not used by any of
its callers.
Rename `PeerManager::RelayTransaction()` to
`PeerManager::InitiateTxBroadcastToAll()`. The transaction is not
relayed when the method returns. It is only enqueued for a possible
broadcasting at a later time. Also, there will be another method which
only does so to Tor or I2P peers.
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
Deduplicate code looping over chainstate objects and calling
ActivateBestChain() and avoid need for code outside ChainstateManager to use
the GetAll() method.
Use to replace m_active_chainstate, m_ibd_chainstate, and m_snapshot_chainstate
members. This has several benefits:
- Ensures ChainstateManager treats chainstates instances equally, making
distinctions based on their attributes, not having special cases and making
assumptions based on their identities.
- Normalizes ChainstateManager representation so states that should be
impossible to reach and validation code has no handling for (like
m_snapshot_chainstate being set and m_ibd_chainstate being unset, or both
being set but m_active_chainstate pointing to the m_ibd_chainstate) can no
longer be represented.
- Makes ChainstateManager more extensible so new chainstates can be added for
different purposes, like indexing or generating and validating assumeutxo
snapshots without interrupting regular node operations. With the
m_chainstates member, new chainstates can be added and handled without needing
to make changes all over validation code or to copy/paste/modify the existing
code that's been already been written to handle m_ibd_chainstate and
m_snapshot_chainstate.
- Avoids terms that are confusing and misleading:
- The term "active chainstate" term is confusing because multiple chainstates
will be active and in use at the same time. Before a snapshot is validated,
wallet code will use the snapshot chainstate, while indexes will use the IBD
chainstate, and netorking code will use both chainstates, downloading
snapshot blocks at higher priority, but also IBD blocks simultaneously.
- The term "snapshot chainstate" is ambiguous because it could refer either
to the chainstate originally loaded from a snapshot, or to the chainstate
being used to validate a snapshot that was loaded, or to a chainstate being
used to produce a snapshot, but it is arbitrary used to refer the first
thing. The terms "most-work chainstate" or "assumed-valid chainstate" should
be less ambiguous ways to refer to chainstates loaded from snapshots.
- The term "IBD chainstate" is not just ambiguous but actively confusing
because technically IBD ends and the node is considered synced when the
snapshot chainstate finishes syncing, so in practice the IBD chainstate
will mostly by synced after IBD is complete. The term "fully-validated" is
a better way of describing the characteristics and purpose of this
chainstate.
IsSnapshotValidated() is only called one place outside of tests, and is use
redundantly in some tests, asserting that a snapshot is not validated when a
snapshot chainstate does not even exist. Simplify by dropping the method and
checking Chainstate m_assumeutxo field directly.
IsSnapshotActive() method is only called one place outside of tests and
asserts, and is confusing because it returns true even after the snapshot is
fully validated.
The documentation which said this "implies that a background validation
chainstate is also in use" is also incorrect, because after the snapshot is
validated, the background chainstate gets disabled and IsUsable() would return
false.
Change ChainstateRole parameter passed to wallets and indexes. Wallets and
indexes need to know whether chainstate is historical and whether it is fully
validated. They should not be aware of the assumeutxo snapshot validation
process.
Use to simplify code determining the chainstate leveldb paths. New method is
the now the only code that needs to figure out the storage path, so the path
doesn't need to be constructed multiple places and backed out of leveldb.
Remove hardcoded references to m_ibd_chainstate and m_snapshot_chainstate so
MaybeCompleteSnapshotValidation function can be simpler and focus on validating
the snapshot without dealing with internal ChainstateManager states.
This is a step towards being able to validate the snapshot outside of
ActivateBestChain loop so cs_main is not locked for minutes when the snapshot
block is connected.
Make Chainstate objects aware of what block they are targeting. This makes
Chainstate objects more self contained, so it's possible for validation code to
look at one Chainstate object and know what blocks to connect to it without
needing to consider global validation state or look at other Chainstate
objects.
The motivation for this change is to make validation and networking code more
readable, so understanding it just requires knowing about chains and blocks,
not reasoning about assumeutxo download states. This change also enables
simplifications to the ChainstateManager interface in subsequent commits, and
could make it easier to implement new features like creating new Chainstate
objects to generate UTXO snapshots or index UTXO data.
Note that behavior of the MaybeCompleteSnapshotValidation function is not
changing here but some checks that were previously impossible to trigger like
the BASE_BLOCKHASH_MISMATCH case have been turned into asserts.
It will allow fetching specific transactions using an external index,
following https://github.com/bitcoin/bitcoin/pull/32541#issuecomment-3267485313.
No logging takes place in case of an invalid offset/size (to avoid spamming the log),
by using a new `ReadRawError::BadPartRange` error variant.
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
It will enable different error handling flows for different error types.
Also, `ReadRawBlockBench` performance has decreased due to no longer reusing a vector
with an unchanging capacity - mirroring our production code behavior.
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
This is a minimal behavior change and changes log output from:
[net:error] Something bad happened
[net:warning] Something problematic happened
to either
[error] Something bad happened
[warning] Something problematic happened
or, when -loglevelalways=1 is enabled:
[all:error] Something bad happened
[all:warning] Something problematic happened
Such a behavior change is desired, because all warning and error logs
are written in the same style in the source code and they are logged in
the same format for log consumers.
-BEGIN VERIFY SCRIPT-
sed --regexp-extended --in-place \
's/LogPrintLevel\((BCLog::[^,]*), BCLog::Level::(Error|Warning), */Log\2(/g' \
$( git grep -l LogPrintLevel ':(exclude)src/test/logging_tests.cpp' )
-END VERIFY SCRIPT-
The changes made here were:
| From | To |
|-------------------|------------------|
| `m.count(k)` | `m.contains(k)` |
| `!m.count(k)` | `!m.contains(k)` |
| `m.count(k) == 0` | `!m.contains(k)` |
| `m.count(k) != 0` | `m.contains(k)` |
| `m.count(k) > 0` | `m.contains(k)` |
The commit contains the trivial, mechanical refactors where it doesn't matter if the container can have multiple elements or not
Co-authored-by: Jan B <608446+janb84@users.noreply.github.com>
The changes made here were:
| From | To |
|------------------------|------------------|
| `m.find(k) == m.end()` | `!m.contains(k)` |
| `m.find(k) != m.end()` | `m.contains(k)` |
fa45a1503eee603059166071857215ea9bd7242a log: Use LogWarning for non-critical logs (MarcoFalke)
fa0018d01102ad1d358eee20d8bae1e438ceebf8 log: Use LogError for fatal errors (MarcoFalke)
22229de7288fed6369bc70b2af674906e6777ce4 doc: Fix typo in init log (MarcoFalke)
Pull request description:
Logging supports severity levels above info via the legacy `LogPrintf`. So use the more appropriate `LogError` or `LogWarning`, where it applies.
This has a few small benefits:
* It often allows to remove the manual and literal "error: ", "Warning:", ... prefixes. Instead the uniform log level formatting is used.
* It is easier to grep or glance for more severe logs, which indicate some kind of alert.
* `LogPrintf` didn't indicate any severity level, but it is an alias for `LogInfo`. So having the log level explicitly spelled out makes it easier to read the code.
* Also, remove the redundant trailing `\n` newline, while touching.
* Also, remove the `__func__` formatting in the log string, which is redundant with `-logsourcelocations`. Instead, use a unique log string for each location.
ACKs for top commit:
l0rinc:
Code review ACK fa45a1503eee603059166071857215ea9bd7242a
stickies-v:
ACK fa45a1503eee603059166071857215ea9bd7242a
rkrux:
crACK fa45a1503eee603059166071857215ea9bd7242a
Tree-SHA512: 516d439c36716f969c6e82d00bcda03c92c8765a9e41593b90052c86f8fa3a3dacbb2c3dc98bfc862cefa54cae34842b488671a20dd86cf1d15fb94aa5563406
b8d279a81c16fe9f5b6d422e518c77344e217d4f doc: add comment to explain correctness of GatherClusters() (Suhas Daftuar)
aba7500a30eecf742c56e292e9a385ca57066a6c Fix parameter name in getmempoolcluster rpc (Suhas Daftuar)
6c1325a0913e22258ab6b62f381e56c7bebbd462 Rename weight -> clusterweight in RPC output, and add doc explaining mempool terminology (Suhas Daftuar)
bc2eb931da30bd98670528c0b96f6ca05f14f8b9 Require mempool lock to be held when invoking TRUC checks (Suhas Daftuar)
957ae232414b38adcf9358e198fded42f7c1feea Improve comments for getTransactionAncestry to reference cluster counts instead of descendants (Suhas Daftuar)
d97d6199ce506cda858afa867f2582c8138953a5 Fix comment to reference cluster limits, not chain limits (Suhas Daftuar)
a1b341ef9875a8a160464f320886f8dac7491237 Sanity check feerate diagram in CTxMemPool::check() (Suhas Daftuar)
23d6f457c4c06e405464594c7a2be1a11e9bcc1b rpc: improve getmempoolcluster output (Suhas Daftuar)
d2dcd37aac1e723a4103f2d6fefaa492141f5d42 Avoid using mapTx.modify() to update modified fees (Suhas Daftuar)
d84ffc24d2dc35642864924aaf7466fa17ac5875 doc: add release notes snippet for cluster mempool (Suhas Daftuar)
b0417ba94437d8bb23a7b66a3641ee8f3682a2dc doc: Add design notes for cluster mempool and explain new mempool limits (Suhas Daftuar)
2d88966e43c6c6323d8af5272ab7841f5c896f12 miner: replace "package" with "chunk" (Suhas Daftuar)
6f3e8eb3001a87d0a6d9ec8662ddb40ce7a673f4 Add a GetFeePerVSize() accessor to CFeeRate, and use it in the BlockAssembler (Suhas Daftuar)
b5f245f6f2193a3c19bea3eed7ceda1e80b83160 Remove unused DEFAULT_ANCESTOR_SIZE_LIMIT_KVB and DEFAULT_DESCENDANT_SIZE_LIMIT_KVB (Suhas Daftuar)
1dac54d506b5765f3d86a6efc30538931305b000 Use cluster size limit instead of ancestor size limit in txpackage unit test (Suhas Daftuar)
04f65488ca3e8e8eb7d290982e55e70be96491bb Use cluster size limit instead of ancestor/descendant size limits when sanity checking TRUC policy limits (Suhas Daftuar)
634291a7dc4485942cc9cbde510b92f9580d5c5e Use cluster limits instead of ancestor/descendant limits when sanity checking package policy limits (Suhas Daftuar)
fc18ef1f3f333dd28d8cc7e3571d76a985d90240 Remove ancestor and descendant vsize limits from MemPoolLimits (Suhas Daftuar)
ed8e819121d7065c6e34a6ae422842369c4a1659 Warn user if using -limitancestorsize/-limitdescendantsize that the options have no effect (Suhas Daftuar)
80d8df2d47c25851b51fe3319605fe41c34ca9f8 Invoke removeUnchecked() directly in removeForBlock() (Suhas Daftuar)
9292570f4cb85fc6690dfeeb55ea867d575ebba3 Rewrite GetChildren without sets (Suhas Daftuar)
3e39ea8c307010bc0132615ecef55b39851f7437 Rewrite removeForReorg to avoid using sets (Suhas Daftuar)
a3c31dfd71def7ce4414c627261fa4516f943547 scripted-diff: rename AddToMempool -> TryAddToMempool (Suhas Daftuar)
a5a7905d83dfa8a5173f886f7007132e18b53e3a Simplify removeRecursive (Suhas Daftuar)
01d8520038eafa0e00eeddcea29cba2b1b87917e Remove unused argument to RemoveStaged (Suhas Daftuar)
bc64013e6fad2d054bc5a31630c09f33a62b8f4f Remove unused variable (cacheMap) in mempool (Suhas Daftuar)
Pull request description:
As suggested in the main cluster mempool PR (https://github.com/bitcoin/bitcoin/pull/28676#pullrequestreview-3177119367), I've pulled out some of the non-essential optimizations and cleanups into this separate PR.
Will continue to add more commits here to address non-blocking suggestions/improvements as they come up.
ACKs for top commit:
instagibbs:
ACK b8d279a81c
sipa:
ACK b8d279a81c16fe9f5b6d422e518c77344e217d4f
Tree-SHA512: 1a05e99eaf8db2e274a1801307fed5d82f8f917e75ccb9ab0e1b0eb2f9672b13c79d691d78ea7cd96900d0e7d5031a3dd582ebcccc9b1d66eb7455b1d3642235
