The oversized `-dbcache` warning currently switches from a fixed `450 MiB` threshold below `2 GiB` of RAM to `75%` of total RAM at `2 GiB`.
This creates a cliff where a small increase in RAM can raise the warning threshold to about `1536 MiB`.
Apply the `75%` factor only to RAM above a `2 GiB` reserve while keeping `DEFAULT_DB_CACHE` as the minimum threshold.
This removes the cliff: the threshold stays at the default until the percentage term exceeds it, then grows by `0.75 MiB` per additional MiB of RAM.
This also aligns better with the recently merged parallel input prevout fetcher which performs better with slightly lower dbcache memory.
Co-authored-by: Bortlesboat <Bortlesboat@users.noreply.github.com>
fabafd91f1 refactor: Use u64 over size_t for all cache sizes to fix a 32-bit overflow (MarcoFalke)
Pull request description:
This is a refactor on 64-bit systems, because size_t is equal to u64.
However, on 32-bit systems, it fixes an integer overflow while calculating the cache sizes:
```
src/node/caches.cpp:71:49: runtime error: unsigned integer overflow: 471859200 * 10 cannot be represented in type size_t (aka "unsigned int")
```
This happens while multiplying the default cache size (450MiB) by 10:
```
index_sizes.tx_index = std::min(total_cache * 10 / 100, ...)
^^^^^^^^^^^^^^^^
```
The issue was introduced in commit d06dabf26b.
----
This change follows similar changed one in the past, like 3789215f73, ac76d94117, or 28a523fb94.
Generally, using fixed sized integer types for calculations is beneficial, because all platforms behave exactly the same way. With platform-dependent types there is a risk that the same calculation yields different results. This has several resulting benefits:
* Easier review, because there is no need to review the same code several times for each supported platform.
* Easier quality assurance, because there is less need to run the same code several times in sanitizers for each supported platform, which is [tedious](https://github.com/bitcoin/bitcoin/issues/32375#issuecomment-4825318068).
There are also no downsides, because there is no measurable overhead on 32-bit for u64 calculations that are done only once in the lifetime of the program. Also, there is no measurable memory overhead when a few fields on 32-bit store some extra zero bytes.
----
As said, testing is only possible by picking one of the tedious options:
* Apply a diff on 64-bit arch and compile with `-DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DSANITIZERS=integer`
```diff
diff --git a/src/node/caches.cpp b/src/node/caches.cpp
index c98b8ce604..cfd49b60d3 100644
--- a/src/node/caches.cpp
+++ b/src/node/caches.cpp
@@ -58,3 +58,3 @@ CacheSizes CalculateCacheSizes(const ArgsManager& args, size_t n_indexes)
{
- size_t total_cache{CalculateDbCacheBytes(args)};
+ uint32_t total_cache(CalculateDbCacheBytes(args));
@@ -72,6 +72,6 @@ CacheSizes CalculateCacheSizes(const ArgsManager& args, size_t n_indexes)
IndexCacheSizes index_sizes;
- index_sizes.tx_index = std::min(total_cache * 10 / 100, args.GetBoolArg("-txindex", DEFAULT_TXINDEX) ? MAX_TX_INDEX_CACHE : 0);
- index_sizes.txospender_index = std::min(total_cache * 5 / 100, args.GetBoolArg("-txospenderindex", DEFAULT_TXOSPENDERINDEX) ? MAX_TXOSPENDER_INDEX_CACHE : 0);
+ index_sizes.tx_index = std::min<uint32_t>(total_cache * 10 / 100, args.GetBoolArg("-txindex", DEFAULT_TXINDEX) ? MAX_TX_INDEX_CACHE : 0);
+ index_sizes.txospender_index = std::min<uint32_t>(total_cache * 5 / 100, args.GetBoolArg("-txospenderindex", DEFAULT_TXOSPENDERINDEX) ? MAX_TXOSPENDER_INDEX_CACHE : 0);
if (n_indexes > 0) {
- size_t max_cache = std::min(total_cache * 5 / 100, MAX_FILTER_INDEX_CACHE);
+ size_t max_cache = std::min<uint32_t>(total_cache * 5 / 100, MAX_FILTER_INDEX_CACHE);
index_sizes.filter_index = max_cache / n_indexes;
```
This will give a roughly similar error:
```
sh-5.3$ echo 'Bw==' | base64 -d > /tmp/blob
sh-5.3$ UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" FUZZ=block_index_tree ./bld-cmake/bin/fuzz /tmp/blob
./src/node/caches.cpp:73:59: runtime error: unsigned integer overflow: 1073741824 * 10 cannot be represented in type 'uint32_t' (aka 'unsigned int')
#0 0x55ca78da5c47 in node::CalculateCacheSizes(ArgsManager const&, unsigned long) ./src/node/caches.cpp:73:59
```
* Alternatively, to reproduce in a fresh `podman run -it --rm --platform linux/i386 debian:unstable`:
```
export DEBIAN_FRONTEND=noninteractive && apt update && apt install curl wget htop git vim ccache -y && git clone https://github.com/bitcoin/bitcoin.git ./b-c && cd b-c && apt install build-essential cmake pkg-config python3-zmq libzmq3-dev libevent-dev libboost-dev libsqlite3-dev systemtap-sdt-dev libcapnp-dev capnproto libqrencode-dev qt6-tools-dev qt6-l10n-tools qt6-base-dev clang llvm libc++-dev libc++abi-dev mold -y && cmake -B ./bld-cmake -DAPPEND_CXXFLAGS='-O3 -g2' -DAPPEND_CFLAGS='-O3 -g2' -DCMAKE_BUILD_TYPE=Debug -DCMAKE_EXE_LINKER_FLAGS=-fuse-ld=mold -DCMAKE_C_COMPILER='clang;-ftrivial-auto-var-init=pattern' -DCMAKE_CXX_COMPILER='clang++;-ftrivial-auto-var-init=pattern' -DSANITIZERS=address,float-divide-by-zero,integer,undefined --preset=dev-mode && cmake --build ./bld-cmake --parallel $(nproc)
echo 'Bw==' | base64 -d > /tmp/blob
UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" FUZZ=block_index_tree ./bld-cmake/bin/fuzz /tmp/blob
# or:
UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ASAN_OPTIONS="detect_leaks=0" ./bld-cmake/bin/test_bitcoin-qt
```
ACKs for top commit:
l0rinc:
ACK fabafd91f1
sedited:
Re-ACK fabafd91f1
theStack:
re-ACK fabafd91f1
Tree-SHA512: cebbc29636b4074917c96cf3af8fcc176dcd328821d5032dc8609475e18778dfda4e287ded889c024ab29b79d81004bc9a8e57a88eaa0a5eaefe5f8bba1462ab
dc1c17c085 doc: add release notes (Andrew Toth)
0e10937184 fuzz: add coins_view_stacked fuzz harness to test concurrent leveldb reads (Andrew Toth)
ce610a6ff4 fuzz: update harnesses to cover CoinsViewOverlay::StartFetching (Andrew Toth)
760fb22dc3 test: add unit tests for CoinsViewOverlay::StartFetching (Andrew Toth)
d69a3b20de doc: update CoinsViewOverlay docstring to describe parallel fetching (Andrew Toth)
ab2a379237 coins: fetch inputs in parallel (Andrew Toth)
fdf283036a coins: add ready flag to InputToFetch (Andrew Toth)
ede11b8314 validation: collect block inputs in CoinsViewOverlay before ConnectBlock (Andrew Toth)
f82043af50 coins: introduce thread pool in CoinsViewOverlay (Andrew Toth)
5bf1c32008 validation: add -prevoutfetchthreads configuration option (Andrew Toth)
Pull request description:
This PR is a continuation of https://github.com/bitcoin/bitcoin/pull/31132. All outstanding issues raised there have been resolved, but the volume of stale comments can make that change difficult to review.
Currently, when connecting a block, each input prevout is looked up one at a time. For every input we first check the in-memory coins cache, and on a miss we make a synchronous round-trip to the chainstate LevelDB to read the coin from disk. Because these lookups happen serially as the block is being validated, the disk read latency stacks up and dominates the time spent in `ConnectBlock` whenever many inputs are not already in the cache.
This PR moves those disk reads onto a pool of worker threads that run in parallel with block connection. Before entering `ConnectBlock` the block is handed to a `CoinsViewOverlay`, which kicks off the workers to begin fetching all of the block's prevouts from disk and warming the cache. The main validation thread continues to do exactly the same work it does today, hitting the cache for each input in order. The only difference is that by the time it asks, the coin is much more likely to already be there. There are no validation logic or consensus behavior changes. This is purely a parallelization of an existing read pattern.
The number of fetcher threads is configurable via `-prevoutfetchthreads=<n>`, defaulting to 8 and capped at 16. Setting it to 0 disables input fetching entirely and reverts to the previous serial behavior.
We have measured large performance gains for IBD and `-reindex-chainstate`, as well as worst-case steady-state block connection at the tip. l0rinc ran many thorough benchmarking passes on the original PR across multiple machines, storage types, dbcache sizes[^1], operating systems[^2], and fetcher thread counts[^3]. Many other contributors also posted their benchmark results in the original PR. IBD speedups range from 1.18× to over 3× faster[^4]. Worst-case block connection time for network-attached storage was over 2× faster[^5]. Flamegraph comparisons before and after this change are available[^6].
On safety: `ConnectBlock` runs while holding `cs_main`, so nothing else in the node can mutate the chainstate while the fetchers are reading it.
On LevelDB: [concurrent reads are fully supported](https://github.com/bitcoin/bitcoin/blob/master/src/leveldb/include/leveldb/db.h#L44) and [documented as such](https://github.com/bitcoin/bitcoin/blob/master/src/leveldb/doc/index.md#concurrency). We already rely on this in production today against our other LevelDB-backed databases. The `txindex` DB is read by multiple simultaneous HTTP RPC worker threads via the `getrawtransaction` RPC. The `blockfilterindex` DB is called concurrently from both the P2P `cfilters` / `cfheaders` / `cfcheckpt` message handlers on the `msghand` thread, and from the `getblockfilter` RPC on the HTTP RPC worker threads. We have not yet been issuing concurrent reads against the chainstate DB, but there is no LevelDB-side reason we can't. In fact, the chainstate DB is already being touched by more than one thread on master, because LevelDB schedules its own background compaction work.
For reviewers:
The main change is `CoinsViewOverlay` gets 1 new public and 2 new private methods.
- `StartFetching`: public method called in lieu of `CreateResetGuard` before we enter `ConnectBlock`. It still returns a `ResetGuard` so the view is `Reset` before the block it is working on leaves scope. This kicks off worker threads who each just run `while (ProcessInput()) {}` and then return.
- `StopFetching`: private method called on `Reset` whenever the guard leaves scope or `Flush`. Stops all threads and clears multi threaded state.
- `ProcessInput`: private method that fetches a single input prevout. Returns `true` if an input was fetched and `false` otherwise. This is the only method on `CoinsViewOverlay` that is called concurrently by multiple threads. Every other method on the overlay is still called synchronously on the main thread.
The `CoinsViewOverlay::FetchCoinFromBase` method is also extended to lookup the coins fetched from `ProcessInput` first before falling back to `base->PeekCoin`.
Mutating methods `Reset` and `Flush` are overridden in `CoinsViewOverlay` to call `StopFetching` first.
[^1]: https://github.com/bitcoin/bitcoin/pull/31132#pullrequestreview-3515011880
[^2]: https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3767758819
[^3]: https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3617721711
[^4]: https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3678847806
[^5]: https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-4071032270
[^6]: https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3617315125
ACKs for top commit:
l0rinc:
reACK dc1c17c085
willcl-ark:
ACK dc1c17c085
theStack:
re-ACK dc1c17c085
ryanofsky:
Code review ACK dc1c17c085 with changes to StopFetching and AllInputsConsumed checking behavior since last review.
Tree-SHA512: 89c1c2890f65aac5cd546edc44504956c47b6fada256d3b86ced47e6dd8c72f633a4357753b3b9805b9ba6ed02790822090d70578aba2964baf50d7eb956864c
This is a refactor on 64-bit systems, because size_t is equal to u64.
However, on 32-bit systems, it fixes an integer overflow while calculating the cache sizes:
src/node/caches.cpp:71:49: runtime error: unsigned integer overflow: 471859200 * 10 cannot be represented in type size_t (aka "unsigned int")
This happens while multiplying the default cache size (450MiB) by 10:
index_sizes.tx_index = std::min(total_cache * 10 / 100, ...)
^^^^^^^^^^^^^^^^
The issue was introduced in commit d06dabf26b.
====
Also, add missing includes in touched files, according to IWYU.
fa615bd163 refactor: Move LoadGenesisBlock to ChainstateManager (MarcoFalke)
Pull request description:
The function does not need anything from any chainstate, so it should not sit in the Chainstate class.
ACKs for top commit:
l0rinc:
Tested ACK fa615bd163
janb84:
reACK fa615bd163
sedited:
ACK fa615bd163
Tree-SHA512: 482b5c140faa35944a890c941fd185a896a3c04fec46d0cc6dd56830f62ee8fe5200fd13d25f7a80a5da0eb4fb42717f42c8bce837d933668432f5786f8380e3
9784818442 mining: add getTransactionsByWitnessID() IPC method (Sjors Provoost)
d282ae6883 mining: add getTransactionsByTxID() IPC method (Sjors Provoost)
0d5e4d4712 test: restart node after IPC option override test (Sjors Provoost)
f16b3613cd ipc: Serialize null CTransactionRef as empty Data (Sjors Provoost)
0f466e1094 mempool: add lookup by witness hash (Sjors Provoost)
Pull request description:
For Stratum v2 custom job declaration to be bandwidth efficient, the pool can request[^0] only the transactions that it doesn't know about.
The spec doesn't specify how this is achieved, but one method is to call the `getrawtransaction` RPC on each transaction id listed in [DeclareMiningJob](https://stratumprotocol.org/specification/06-Job-Declaration-Protocol?query=DeclareMiningJob#644-declareminingjob-client-server) (or a subset if the pool software maintains a cache). Using RPC is inefficient, made worse by the need to make multiple calls. It also doesn't support queuing by witness id (yet, see #34013).
This PR introduces two new IPC methods:
- `getTransactionsById()`: takes a list of `Txid`'s
- `getTransactionsByWitnessID()`: : takes a list of `Wtxid`'s
Both return a list of serialised transactions. An empty element is returned for transactions that were not found.
Unlike the RPC counterpart, the IPC methods do not take advantage of `-txindex`. This could be done in a followup. For `Wtxid` that would involve adding a `-witnesstxindex`.
I thought about having a single (or overloaded) `getTransactions()` that works with both `Txid` and `Wtxid`, but I prefer that clients are intentional about which one they want.
A unit and functional test cover the new functionality.
Sv2 probably only needs `getTransactionsByWitnessID()`, but it's easy enough to just add both.
To rest with Rust use:
- https://github.com/2140-dev/bitcoin-capnp-types/pull/11
[^0]: there's two reasons the pool requests these transactions: to approve the template and to broadcast the block if a solution is found (the miner will also broadcast via their template provider). See also https://github.com/stratum-mining/sv2-spec/issues/170
ACKs for top commit:
achow101:
ACK 9784818442
sedited:
Re-ACK 9784818442
ViniciusCestarii:
Re-ACK 9784818442
ismaelsadeeq:
Code review ACK 9784818442
Tree-SHA512: 3c6ceb572ab7d8bd090a8f31b5e331304a7a19a3d1f1551c9c2e1ee41339d76f96ca6c41bd634c87fca0a969e7d9bfa6a16c26fb06c0dd2315f6ca1c76a16a31
The function does not need anything from any chainstate, so it should
not sit in the Chainstate class.
Also, mark it [[nodiscard]], and the one place that ignores the return
value with (void).
Also, change the error log strings to not include the __func__, which is
redundant with -logsourcelocations. This is not a refactor, but this log
is only for debugging extremely rare errors.
Add a belt-and-suspenders feature, limit the amount of
memory and cpu possible when unlucky or simply misconfigured.
The worst case limit is roughly 400kB * 10,000 = 4GB, regardless
of usage pattern.
Before this change, sheer volume of broadcasts, mismatches in
standardness rules, or simply fee mismatches may result in unbounded
growth of memory usage. As the feature may be expanded in
the future, explicit bounds helps reasoning going forward.
Add a configuration option for the number of worker threads used for
parallel UTXO prevout prefetching during block connection.
Default is 8 threads, max is 16, 0 disables parallel fetching.
b847626562 test: refresh MiniWallet after node restart (Sjors Provoost)
f4e643cb15 test: merge mining options in package feerate check (Sjors Provoost)
280ce6a0ae miner: ensure block_max_weight is flattened before limit checks (Sjors Provoost)
65bd3164fb mining: clarify test_block_validity comment (Sjors Provoost)
978e7216e6 test: use shared default_ipc_timeout (Sjors Provoost)
Pull request description:
This implement the suggested followups from #33966. Each commit links to the original comment.
The most important change is the extra asserts added in `miner: ensure block_max_weight is flattened before limit checks`.
ACKs for top commit:
achow101:
ACK b847626562
enirox001:
tACK b847626562
sedited:
ACK b847626562
w0xlt:
ACK b847626562
Tree-SHA512: 47678eaed604228269bd892ccf8ff58804745bbc7675b4a93528da9a9292a2eb1e0562cdb8341edac77178563420885b48282bb9e5c2b997b28f2fc64ceeff3d
d6359937bf validation: check invariants when inserting into m_blocks_unlinked (stratospher)
0852925bd8 test/doc: remove misleading comment and improve tests (stratospher)
ca4a380281 test: add coverage for UB caused by FindMostWorkChain (stratospher)
c787b3b99b validation: avoid duplicates in m_blocks_unlinked (stratospher)
Pull request description:
This is joint work with @ mzumsande.
note: this requires a pruned node with deep reorgs to trigger. still it breaks assumptions in the codebase and is good to fix. A similar UB was fixed in https://github.com/bitcoin/bitcoin/pull/34521.
This PR prevents duplicate insertions into `m_blocks_unlinked` in `FindMostWorkChain`. There are 3 ways to insert into `m_blocks_unlinked`:
1. `LoadBlockIndex` - not problematic, as each block index is processed only once.
2. `ReceivedBlockTransactions` - not problematic, as this is usually only called once per block when it is first accepted in `AcceptBlock`. in the rare case it’s triggered again after pruning, the block would have been removed from `m_blocks_unlinked` when it was initially pruned, so duplicates still can’t arise.
3. `FindMostWorkChain` - problematic when multiple candidate tips share common chains of ancestors, traversals from each tip to the fork point may insert duplicate (`pprev`, `pindex`) entries for blocks whose parents have been pruned.
When the missing parent is later received and `ReceivedBlockTransactions` processes `m_blocks_unlinked`, the same entry may be processed multiple times. This can result in the block being re-added to `setBlockIndexCandidates` with a modified `nSequenceId`, violating its ordering invariants and leading to undefined behavior. So avoid duplicate insertions into `m_blocks_unlinked` in `FindMostWorkChain`.
### how to test:
use the updated `feature_pruning.py` which adds coverage for this scenario.
- on master: the test (with the below diff) fails since `nSequenceId` is being modified for an entry in `setBlockIndexCandidates`
- on this branch: the test (with the below diff) passes
```diff --git a/src/validation.cpp b/src/validation.cpp
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -3814,6 +3814,12 @@ void ChainstateManager::ReceivedBlockTransactions(const CBlock& block, CBlockInd
pindex->nHeight, pindex->m_chain_tx_count, prev_tx_sum(*pindex), CLIENT_NAME, FormatFullVersion(), CLIENT_BUGREPORT);
}
pindex->m_chain_tx_count = prev_tx_sum(*pindex);
+ for (const auto& c : m_chainstates) {
+ if (c->setBlockIndexCandidates.contains(pindex)) {
+ LogInfo("### pindex UB = %s", pindex);
+ assert(false);
+ }
+ }
pindex->nSequenceId = nBlockSequenceId++;
for (const auto& c : m_chainstates) {
c->TryAddBlockIndexCandidate(pindex);
```
ACKs for top commit:
sedited:
Re-ACK d6359937bf
marcofleon:
crACK d6359937bf
stringintech:
ACK d6359937bf
mzumsande:
Sure - Code Review ACK [d635993](d6359937bf)
Tree-SHA512: bb21adc2d92fe1865bbbcebf775a850ca3eccac6fe83d7bca10b78eee4c0abf782e44fa0ddfec9d9a70f42fa40bdc49b68a1d1b4905cdc371ba29117d3120619
Previously, the signal was using btcsignals::optional_last_value<bool>.
However, this only worked by accident:
The return value was influenced by the order in which the connections
were done. The noui callbacks would always overwrite the return value
with false. This makes the code overall brittle, and confusing.
For example, the following patch that changes the order of connections
would break the only and single place where the return value actually
matters:
```diff
diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp
index 0b89c605b9..976549470e 100644
--- a/src/qt/bitcoin.cpp
+++ b/src/qt/bitcoin.cpp
@@ -488,3 +488,2 @@ int GuiMain(int argc, char* argv[])
btcsignals::scoped_connection handler_message_box = ::uiInterface.ThreadSafeMessageBox_connect(noui_ThreadSafeMessageBox);
- btcsignals::scoped_connection handler_question = ::uiInterface.ThreadSafeQuestion_connect(noui_ThreadSafeQuestion);
btcsignals::scoped_connection handler_init_message = ::uiInterface.InitMessage_connect(noui_InitMessage);
@@ -663,2 +662,3 @@ int GuiMain(int argc, char* argv[])
app.createWindow(networkStyle.data());
+ btcsignals::scoped_connection handler_question = ::uiInterface.ThreadSafeQuestion_connect(noui_ThreadSafeQuestion);
// Perform base initialization before spinning up initialization/shutdown thread
```
This can be tested by applying the patch and then calling:
(May have to be started twice to trigger the question)
```
bitcoin-qt -regtest -datadir=/tmp -mocktime=123456789
```
Before the changes in this commit (on current master), pressing `OK`
would not have any effect and would abort the program.
After the changes in this commit, pressing `OK` will correctly trigger a
-reindex and leave the program running.
The message will always return false (a constant) and the return value
is never used.
Also, annotate ThreadSafeMessageBox in the GUI code as [[nodiscard]],
because it may actually return a value, which is handled for questions
(but not for messages).
For an entry A -> B in m_blocks_unlinked, the entry B was added into
m_blocks_unlinked either because:
- some ancestor of B was never received (or)
- some ancestor of B was pruned away.
Every insert must satisfy two invariants:
1. B has BLOCK_HAVE_DATA set.
2. No duplicate A -> B entries in m_blocks_unlinked (this is UB zone if
this entry gets popped twice in ReceivedBlockTransactions and
happens to be in setBlockIndexCandidates)
2 bugs (#35070 and #35168) discovered recently stemmed from the
m_blocks_unlinked insertion sites not enforcing these invariants.
So add a helper which wraps around insertion sites of m_blocks_unlinked
with these invariants.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
remove misleading comment for m_blocks_unlinked since for
pruned nodes:
- usually A is the missing data (just like in non-pruned nodes)
- in PruneOneBlockFile, we remove entries once data for B is missing.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
5a2e359213 clarify blockfilterindex cache allocation rationale (Sebastian van Staa)
d06dabf26b node: allocate index caches proportional to usage patterns (Sebastian van Staa)
Pull request description:
The current cache allocation for optional indexes (txindex, txospenderindex, blockfilterindex) uses a sequential total_cache / 8 approach where each index gets 1/8 of the remaining budget after the previous index has been allocated. This means the order in which indexes appear in the code silently determines how much cache each one gets.
|Index|Current share of total|
|---|---|
|txindex|~12%|
|txospenderindex|~11%|
|blockfilterindex|~10%|
This is unintuitive, undocumented, and probably doesn't reflect actual usage patterns. This PR replaces the sequential 1/8 allocation with explicit percentages based on how the indexes are typically used. The current values are an educated guess, and subject to further benchmark and research of typical client usage patterns.
|Index|Allocation|Rationale|
|---|---|---|
|txindex |10% |Serves getrawtransaction RPCs with mostly unique lookups across the entire blockchain: low cache reuse|
|txospenderindex|5%|Serves gettxspendingprevout RPCs with very specific outpoint queries: likely the least repetitive access pattern|
|blockfilterindex|5%|Serves BIP 157 light clients that repeatedly query the same recent blocks: highest cache benefit|
UPDATE: blockfilterindex allocation changed from 15% to 5% in the course of the discussion
This is a continuation of the related discussion: https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2809088034 and https://github.com/bitcoin/bitcoin/pull/31483.
Further feedback and input is very much appreciated.
ACKs for top commit:
fjahr:
ACK 5a2e359213
rustaceanrob:
ACK 5a2e359213
achow101:
ACK 5a2e359213
sedited:
ACK 5a2e359213
Tree-SHA512: 69be2b0c274b975da58aef2513c3042be8a4c8acf0a86af86b962d4ebfd8cf90bcb1d9251d53995652b4825d0d1da24aabe92cdada9148c627690f8ad2ad8a29
3f44f9aef7 test: Add coverage for m_blocks_unlinked invariant in LoadBlockIndex (marcofleon)
0e4b0bacec validation: Don't add pruned blocks to m_blocks_unlinked on startup (marcofleon)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/35050
The `m_blocks_unlinked` map keeps track of blocks that have transactions but whose parent (or any ancestor) does not. This happens when a block is received before its parent, or during a reorg, when `FindMostWorkChain()` encounters a block whose ancestors were pruned.
The bug this PR addresses is a rare interaction of these two cases, which happens on startup when `BlockManager::LoadBlockIndex()` rebuilds `m_blocks_unlinked`. The check there only considers whether a block has transactions, and pruned blocks keep `nTx > 0` but clear `BLOCK_HAVE_DATA`. So if there's a pruned block on a stale fork whose parent has no transactions, that block is added to `m_blocks_unlinked` without having data on disk. This violates an [assertion](ad3f73862b/src/validation.cpp (L5352)) in `CheckBlockIndex()`.
Get rid of this unintended case by gating on `BLOCK_HAVE_DATA` before adding to `m_blocks_unlinked`.
ACKs for top commit:
achow101:
ACK 3f44f9aef7
sedited:
Re-ACK 3f44f9aef7
stratospher:
ACK 3f44f9a. nice!
Tree-SHA512: 275d0f8588524c01c4e701c8635973cd4a086d31c10d252a498c1ef668bdb3895ba1cae265dbe88f8983ca7ddbe32247824753c7c1f49e59c8bce0df377b784c
ec6cf49b91 blockstorage: Remove cs_LastBlockFile recursive mutex (sedited)
Pull request description:
The `cs_LastBlockFile` mutex is redundant: all critical sections are already covered by cs_main. This is demonstrated in this patch by replacing all instances of locking `cs_LastBlockFile` with pairs of `AssertLockHeld(::cs_main)` and `EXCLUSIVE_LOCKS_REQUIRED(::cs_main)` annotations. No additional `::cs_main` LOCK(...)s are introduced (besides for test-only code).
It is also not clear for which sections `cs_LastBlockFile` is responsible for. It is annotated for `m_blockfile_cursors`, but sporadically and inconsistently also covers `m_blockfile_info` (e.g. in `LoadBlockIndexDB`).
Since it has no semantic meaning, and seems confusing to developers, remove it.
An alternative to this patch would be expanding the scope of what `cs_LastBlockFile` covers and turning it into a non-recursive mutex. I prepared such a patch some time ago, but found it unsatisfactory. It was not clear to me if the lock was now covering too much or too little, and its purpose remained unclear. If this patch is accepted, I would expect the project to eventually implement a separate, narrowly-scoped block storage lock to allow for a more parallelizable block processing routine.
ACKs for top commit:
stickies-v:
re-ACK ec6cf49b91
janb84:
re- ACK ec6cf49b91
pablomartin4btc:
ACK ec6cf49b91
Tree-SHA512: e5942bc87300b0db9a0b91d5fe26dab455049e6cef7c96bb12b28141fa04711d46c6af105c0e1a83a9f261edde2c8b8b43ecf577a27d54b4610d784676a85627
The cs_LastBlockFile mutex is redundant: all critical sections are
already covered by cs_main. This is demonstrated in this patch by
replacing all instances of locking cs_LastBlockFile with pairs of
`AssertLockHeld(::cs_main)` and `EXCLUSIVE_LOCKS_REQUIRED(::cs_main)`
annotations. No additional `::cs_main` LOCK(...)s are introduced.
It is also not clear for which sections `cs_LastBlockFile` is
responsible for. It is annotated for `m_blockfile_cursors`, but
sporadically and inconsistently also covers `m_blockfile_info`.
Since it has no semantic meaning, and seems confusing to developers,
remove it.
BIP434 defines FEATURE messages which are sent between VERSION and VERACK
to indicate support for new P2P protocol features. This commit provides
the infrastructure for easily using BIP434 negotiation when implementing
such new P2P protocol features. Note that advertised protocol version
is bumped to 70017, as per BIP434's specification.
Add a submitBlock method to the Mining IPC interface, similar to the
submitblock RPC. This accepts a fully assembled block, validates it, and
if accepted as new, processes it into chainstate.
This is needed for Stratum v2 Job Declarator Server (JDS), where accepted
solutions may correspond to jobs not tied to a Bitcoin Core BlockTemplate.
JDS receives PushSolution fields and reconstructs full blocks; without an
IPC submitBlock method, final submission requires the submitblock RPC.
The method returns detailed status (reason/debug strings) matching the
checkBlock pattern, giving callers enough information to handle
validation failures.
Introduce a SubmitBlock() helper in node/miner.cpp that wraps
ProcessNewBlock submission and captures validation state through
the BlockChecked callback.
Route submitSolution through the helper before adding any new IPC method.
No behavior change.
Read configured mining options once during startup instead of parsing
options like -blockmaxweight each time a block template is generated.
Store the parsed startup options as BlockCreateOptions. Members left unset
keep representing unset options; hardcoded defaults are applied by
FlattenMiningOptions() before the options are used. IPC overrides and node
defaults can then be merged with the same option type used by
BlockAssembler.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Check BlockCreateOptions before block template creation instead of
clamping runtime values. This makes invalid runtime block creation
options, including those passed by IPC mining clients, fail explicitly
instead of silently mining with different values than the caller
requested.
Runtime option validation now uses the same error wording as startup
option validation. Startup validation also rejects -blockmaxweight
values lower than -blockreservedweight instead of allowing them to be
clamped later.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Move block template defaulting into helper functions for
BlockCreateOptions. FlattenMiningOptions() fills hardcoded defaults and
MergeMiningOptions() overlays defaults without replacing caller-provided
values.
Use the shared option type in BlockAssembler so IPC callers and internal
callers can pass through the same options path. This commit does not
change behavior, except for dropping the "Specified " prefix from startup
option error messages.
Keep the -blockmintxfee ParseMoney check in ReadMiningArgs() instead of
CheckMiningOptions(), because CheckMiningOptions() only sees the parsed
CFeeRate value and not the original string.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
This new optional replaces nBlockMaxWeight.
Use uint64_t for the block weight options to match BlockAssembler's
nBlockWeight accounting and the IPC schema's blockReservedWeight type.
A negative -blockmaxweight value is now parsed as 0 instead of an
overflowed signed value before validation rejects it.
The new block_max_weight option is not exposed to IPC clients.
Move the argument parsing for -blockmaxweight, -blockreservedweight,
-blockmintxfee out of init.cpp to a dedicated mining_args.cpp.
This is mostly a refactor and keeps the existing validation checks. It
does switch the weight arguments to GetArg<uint64_t>, introduced in
bitcoin/bitcoin#34582, so very large or negative weight values can be
reported differently in error messages.
Have most tests, benchmarks and fuzzers go through the mining interface.
This is a refactor: it does not change what blocks are created, just how
the creation calls are made. This avoids most direct test, benchmark and
fuzzer use of node::BlockAssembler::Options, making it easier to drop in a
later commit.
Two exceptions which use BlockAssembler directly:
- one check in test/miner_tests.cpp needs m_package_feerates
- fuzz/tx_pool.cpp Finish() doesn't have access to a NodeContext
Move test_block_validity from BlockAssembler::Options to
BlockCreateOptions so bench/block_assemble.cpp can continue to set it.
Just like coinbase_output_script, this is not exposed to IPC clients.
Inline options variable in places where it's only needed once.
We also drop one unused PrepareBlock declaration and one unused
implementation.
TestChain100Setup::CreateBlock no longer needs a chainstate argument,
which in turn means it can be dropped from CreateAndProcessBlock. Using
the Mining interface here also requires marking the test
KernelNotifications chainstate as loaded after LoadVerifyActivateChainstate().
The next commit switches test helpers that take a const NodeContext& to
create blocks through the Mining interface, so MakeMining needs to
accept a const NodeContext too.
02b2c41103 logging: use util/log.h where possible (Anthony Towns)
57d7495fe5 IWYU fixes (Anthony Towns)
611878b46f scripted-diff: logging: Drop LogAcceptCategory (Anthony Towns)
34332dba2f util/log, logging: Provide ShouldDebugLog and ShouldTraceLog instead of a generic ShouldLog (Anthony Towns)
abea304dd6 logging: Move GetLogCategory into Logger class (Anthony Towns)
58113e5833 util/log: Rename LogPrintLevel_ into detail_ namespace (Anthony Towns)
f69d1ae56d util/log: Provide util::log::NO_RATE_LIMIT to avoid rate limits (Anthony Towns)
72e92d67df logging: Protect ShrinkDebugFile by m_cs (Anthony Towns)
904c0d07bb util/stdmutex: Drop StdLockGuard (Anthony Towns)
Pull request description:
`ShrinkDebugFile` now takes the logging mutex for its entire run; though it's only called in init so shouldn't have any races in the first place.
Adds a `NO_RATE_LIMIT` tag that can be used with info/warning/error logs to avoid rate-limiting. This allows `LogPrintLevel_` to be restricted to being an internal API.
The `GetLogCategory` function is moved out of the global namespace.
`ShouldLog` is split into separate `ShouldDebugLog` and `ShouldTraceLog` so that filtering checks are somewhat more enforced via function signature checks.
Redundant `LogAcceptCategory` function is removed.
More files are pointed at util/log.h instead of logging.h.
ACKs for top commit:
maflcko:
review ACK 02b2c41103📅
sedited:
Re-ACK 02b2c41103
l0rinc:
untested ACK 02b2c41103
ryanofsky:
Code review ACK 02b2c41103435d8dbaa77a526e484066471b2b8c! Overall a lot of nice improvements here.
Tree-SHA512: 3bffdca91afbe5c45a522815fe82e6f4cfa96529a4a243b29aad21234650502d6cac780126b584ee3e7ec129d8fdd50670d8a05036cc5c36e586b8c4c3563970
The previous commit made CreateNewBlock() ignore the
include_dummy_extranonce flag (OP_0 is now appended only at heights
<= 16 to satisfy bad-cb-length, regardless of caller-supplied options).
Remove the now-unused field from BlockCreateOptions and clean up all
the call sites that still set it (RPC, bench, fuzz, test setups).
Drop the include_dummy_extranonce branch from the OP_0 padding
condition in CreateNewBlock(), so that the dummy extraNonce is
only appended when consensus actually requires it (heights <= 16,
where the BIP34 height push alone would yield a 1-byte scriptSig
and trigger bad-cb-length).
The include_dummy_extranonce option struct field is now unused by
the miner and is removed in the next commit. Callers still set it,
so that this commit compiles.
Regenerate the hardcoded coinbase / block hashes throughout the
unit and functional test suites and update the regtest assumeutxo
snapshot in chainparams.
Additional side-effects:
- Without the dummy extranonce, coinbase scriptSigs are 1 byte
shorter at heights > 16, making every block 1 byte smaller.
This shifts where block files wrap and therefore where pruning
boundaries land.
- feature_assumeutxo malleation cases:
- case 1: error message changes due to UTXO reordering, similar
to 8f2078af6a
- case 4: the corruption byte is swapped from \x82 to \x83
because \x82 happened to be the actual value at that
offset in the new snapshot.
Since #32420, createNewBlock has thrown `bad-cb-length` errors when called at
low block heights because `OP_0` padding stopped being added to coinbase
transactions. (#32420 did add an `include_dummy_extranonce` option which could
bypass this, but it was not exposed to IPC clients.) Fix the problem by padding
coinbase transactions with `OP_0` when necessary to produce valid blocks.
Additionally this commit stops adding `OP_0` padding to the template
`script_sig_prefix` field when `include_dummy_extranonce` is true. This is safe
because non-IPC clients don't use this field, and IPC clients could never set
the option to true, and are expected to add their own nonces in any case.
This also improves documentation about the `script_sig_prefix` field and
`getCoinbaseTx` method.
d084bc88be doc: clarify IWYU workflow (Lőrinc)
7c7cec4567 ci: update IWYU patch reference (Lőrinc)
Pull request description:
### Problem
This was prompted by https://github.com/bitcoin/bitcoin/pull/34435#discussion_r3123255248, where it was not clear to me how (and where) exceptional IWYU cases should be documented.
### Fix
This PR documents the IWYU CI wrapper as the reproducible local entrypoint.
The developer notes now recommend reducing suspected IWYU false positives to a minimal upstream reproducer, treat `IWYU pragma` as a narrow workarounds, and ask for nearby rationale comments on non-obvious IWYU pragma use. An example comment was also added.
The IWYU patch comment is also updated to point at the current `clang_22` include picker reference.
### Reproducer
Create a dummy commit on top that adds an unused include, then run the command from the developer notes.
Without the dummy commit, the command should pass.
<details><summary>IWYU demo commit</summary>
```diff
diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
--- a/src/kernel/bitcoinkernel.cpp(revision c92b329e7b7d49476b5977d26c24d7c4982c6024)
+++ b/src/kernel/bitcoinkernel.cpp(revision ad2c5ba2ba69156e77061c1e6c098b725c28f322)
@@ -43,6 +43,7 @@
#include <functional>
#include <list>
#include <memory>
+#include <vector>
#include <span>
#include <stdexcept>
#include <string>
```
</details>
> [!NOTE]
> After repeated failing runs, `docker container rm -f ci_native_iwyu` may be needed because the local CI wrapper can leave the detached container running when the inner test command fails.
ACKs for top commit:
hebasto:
ACK d084bc88be.
sedited:
ACK d084bc88be
Tree-SHA512: 0aac42d468a1fdfa9f4a3856372e05fb43ec9f0973aeb3a4194fff948fc61e8e72e3b280cde10e74b8da88b6cff93962b3a7f7390eb042113ef92aa6b51d6d8f
9fa4076b20 test: Test merging implicit PSBTv0 with explicit PSBTv0 (w0xlt)
1660c18232 doc: Release notes for psbtv2 (Ava Chow)
470e52a5f8 fuzz: Enforce additional version invariants in PSBT fuzzer (Antoine Poinsot)
5bd0579c09 test: Tests for PSBT AddInput and AddOutput (Ava Chow)
b8b6e7f0c2 tests: Add PSBT unit test for ComputeTimeLock (Ava Chow)
0bc1c2e508 tests: Add test vectors from BIP 370 (Ava Chow)
e0e4dbdeb5 psbt: Change default psbt version to 2 (Ava Chow)
bcc1dca77b Add psbt_version to PSBT RPCs and default to v2 (Ava Chow)
ab38c30195 Implement PSBTv2 field merging (Ava Chow)
93e339e29f Implement PSBTv2 AddInput and AddOutput (Ava Chow)
b39c86ae60 Allow specifying PSBT version in constructor (Ava Chow)
dcc9a3c8df Implement PSBTv2 in decodepsbt (Ava Chow)
5770dbd39f Add PSBT::ComputeLockTime() (Ava Chow)
863cf47b33 Update test_framework/psbt.py for PSBTv2 (Ava Chow)
925161eaf0 Implement PSBTv2 fields de/ser (Ava Chow)
d9cf658ee0 Restrict joinpsbts to PSBTv0 only (Ava Chow)
3da0e16012 Replace PSBT.tx with PSBT::GetUnsignedTx and PSBT::GetUniqueID (Ava Chow)
c568624ff2 psbt: Return std::optional from PrecomputePSBTData (Ava Chow)
092de4f1f6 Replace PSBT::GetInputUTXO with PSBTInput::GetUTXO (Ava Chow)
82c9fe3179 psbt: Use PSBTInput and PSBTOutput fields instead of accessing global tx (Ava Chow)
95897507e9 psbt: AddInput and AddOutput should take only PSBTInput and PSBTOutput (Ava Chow)
1b7d323a72 Add PSBTInput::GetOutPoint (Ava Chow)
543d3e1cdc psbt: add PSBTv2 global tx fields (Ava Chow)
c01c7f068c psbt: Remove default constructor (Ava Chow)
9671aa08c2 psbt: add tx input and output fields in PSBTInput and PSBTOutput (Ava Chow)
990b084f11 Have PSBTInput and PSBTOutput know the PSBT's version (Ava Chow)
7eacc21ff6 psbt: make PSBT structs into classes (Ava Chow)
f926c326bb gui: Store PSBT in std::optional in PSBTOperationsDialog (Ava Chow)
1e2d146b47 psbt: Refactor duplicate key lookup and size checks (Ava Chow)
88384180d3 test: PSBTs should roundtrip through RPCs that do nothing (Ava Chow)
001877500d test: construct psbt with unknown field programmatically (David Gumberg)
0cb884e6df psbt: Fill hash preimages and taproot builder from SignatureData (Ava Chow)
Pull request description:
BIP 370 PSBTv2 introduces several new fields and different invariants for PSBT. This PR implements those new fields and restructures the PSBT implementation to match PSBTv2 but still remain compatible with PSBTv0.
ACKs for top commit:
nervana21:
re-ACK 9fa4076b20
theStack:
re-ACK 9fa4076b20
w0xlt:
ACK 9fa4076b20
Tree-SHA512: ab0a5ada4fa5fca27ba9ec9c291a44b30e69d6db11971957572d86c58c71c4caa4557dc25f403e1170ba4fac751306d074cc582defefc6e2fdd37be51c3d9dd0
1aa78cdab6 clusterlin: adopt STL ranges algorithms (refactor) (Pieter Wuille)
747da25360 feefrac: drop comparison and operator{<<,>>} for sorted wrappers (Pieter Wuille)
Pull request description:
Instead of having an unintuitive but strong implicit sort order on `FeeFrac` (first increasing feerate, then decreasing size), and separate overloaded `operator<<` and `operator>>` that implement a weak ordering that only looks at feerate, replace these with explicit wrapper classes which make the behavior more explicit (`ByRatio` and `ByRatioNegSize`).
This allows for things like `ByRatio{a} <= ByRatio{b}`, instead of the earlier `!(a >> b)`. It also supports usage inside `std::min`/`std::max`/`std::less`, and `std::greater`, so one can use:
* `std::max<ByRatioNegSize<FeeFrac>>(a, b)` to get the highest-feerate `FeeFrac`, tie-breaking by smallest size.
* `std::ranges::sort(v, std::greater<ByRatioNegSize<FeeFrac>>{});` to sort a vector that way.
Suggested in https://github.com/bitcoin/bitcoin/pull/34257#discussion_r2780475893.
ACKs for top commit:
achow101:
ACK 1aa78cdab6
sedited:
ACK 1aa78cdab6
ajtowns:
ACK 1aa78cdab6
Tree-SHA512: d76657b15f6d745e5ca01c67fd5b101fdc418e6301646d14e575b6564bfa2fe0eb40a95a7ff95a4420624ef6b67224d35e4713aa5bbc0d293e08fe44c0cc6db0
The global unsigned tx is decomposed into separate fields inside of
PSBT, which mirrors what PSBTv2 will do. However, we still need to get
the global unsigned tx so PSBT::GetUnsignedTx is introduced to do that.
In order to also have a stable unique ID, we also introduce
PSBT::GetUniqueID to replace uses of PSBT.tx.GetHash().
