24bcad3d4df59690f30c9df8ebb62f0bddd0f1c7 refactor: remove dead code in `CountWitnessSigOps` (Lőrinc)
Pull request description:
Found while reviewing #32840
The `nullptr` witness path was dead in normal code paths: replacing it with reference enables us deleting unreachable logic.
Code coverage proof:
https://maflcko.github.io/b-c-cov/total.coverage/src/script/interpreter.cpp.gcov.html#L2135
ACKs for top commit:
kevkevinpal:
ACK [24bcad3](24bcad3d4d)
maflcko:
review ACK 24bcad3d4df59690f30c9df8ebb62f0bddd0f1c7 🐏
darosior:
Neat. utACK 24bcad3d4df59690f30c9df8ebb62f0bddd0f1c7.
stickies-v:
ACK 24bcad3d4df59690f30c9df8ebb62f0bddd0f1c7
Tree-SHA512: 92c87e431f06a15d8eeb02e20e9154b272c4586ddacf77c8d83783091485fb82c24ecbd711db7043a92cf6169746db24ad46a5904d694aea9d3c3aa96da725f0
2d23820ee11678d567c75f94c40011ed9f0e274f refactor: remove dead branches in `SingletonClusterImpl` (Lőrinc)
Pull request description:
Found during review: [cluster mempool: control/optimize TxGraph memory usage](https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423058928)
### Fixes
`SplitAll()` always calls `ApplyRemovals()` first, for a singleton, it empties the cluster, therefore any `SingletonClusterImpl` passed to `Split()` must be empty.
`TxGraphImpl::ApplyDependencies()` first merges each dependency group and asserts the group has at least one dependency. Since `parent` != `child`, `TxGraphImpl::Merge()` upgrades the merge target to `GenericClusterImpl`, therefore the `ApplyDependencies()` is never dispatched to `SingletonClusterImpl`.
### Coverage proof:
* https://maflcko.github.io/b-c-cov/fuzz.coverage/src/txgraph.cpp.gcov.html#L1446
* https://storage.googleapis.com/oss-fuzz-coverage/bitcoin-core/reports/20251103/linux/src/bitcoin-core/src/txgraph.cpp.html#L1446
ACKs for top commit:
instagibbs:
ACK 2d23820ee11678d567c75f94c40011ed9f0e274f
sipa:
ACK 2d23820ee11678d567c75f94c40011ed9f0e274f
Tree-SHA512: 5135913206c800d5344df61c6654f00917cb85567bc5b821576c7891805cf7689bf47968434a06517d09183dadfefc257d24c42b55a7b99486a4c9b11fc523af
038849e2e09bb9f4ce1fb5a1f291745506c6a52d clang-tidy: Remove no longer needed NOLINT (Hennadii Stepanov)
Pull request description:
From https://github.com/bitcoin/bitcoin/pull/33714/files#r2491476516:
> Actually, the `NOLINT` was fixed and can be removed? You've confirmed that it is undeclared on the listed platforms, so it can't be hit by `readability-redundant-declaration`
ACKs for top commit:
maflcko:
lgtm ACK 038849e2e09bb9f4ce1fb5a1f291745506c6a52d
l0rinc:
I wanted to ask the same on the original PR but forgot - ACK 038849e2e09bb9f4ce1fb5a1f291745506c6a52d
Tree-SHA512: c0b24235425e80baeac3158c7169122364f31140367bc289430d34f01cd38f9f6a3931319f6fe4e1dc86bc4d87e21a5b4b8a2263c199e8083593f89ce592a177
6c7a34f3b0bd39ef7a1520aac56e12f78e5cc969 kernel: Add Purpose section to header documentation (TheCharlatan)
7e9f00bcc1742932e40426dddd906851b46c24d3 kernel: Allowing reducing exports (TheCharlatan)
7990463b1059ba5fc4ebe37fd1105a9e168ae20d kernel: Add pure kernel bitcoin-chainstate (TheCharlatan)
36ec9a3ea2322adf8d73e711fb17cf2a64f5bcaa Kernel: Add functions for working with outpoints (TheCharlatan)
5eec7fa96aa3042025181c4c4b57263beb869244 kernel: Add block hash type and block tree utility functions to C header (TheCharlatan)
f5d5d1213cc4f4ef8bfe335736c665ed7bc3137d kernel: Add function to read block undo data from disk to C header (TheCharlatan)
09d0f626388a10eed1f264386014665fcae4fa22 kernel: Add functions to read block from disk to C header (TheCharlatan)
a263a4caf2311bc31dc2ef1c04dab9517ee0d28f kernel: Add function for copying block data to C header (TheCharlatan)
b30e15f4329ab0ee6bb5c4c1d1f6067be364c59e kernel: Add functions for the block validation state to C header (TheCharlatan)
aa262da7bcfa9bf3d0105e6f689eae7c6e95a0e5 kernel: Add validation interface to C header (TheCharlatan)
d27e27758d51bc2aa125dc967691aacc4f3811d3 kernel: Add interrupt function to C header (TheCharlatan)
1976b13be9c87baa1229b1573bdc8a1da562db0d kernel: Add import blocks function to C header (TheCharlatan)
a747ca1f516e7ec73758c6017e2eca5635ab2b74 kernel: Add chainstate load options for in-memory dbs in C header (TheCharlatan)
070e77732cdb927cc27ddd39c52dec22c5d717a0 kernel: Add options for reindexing in C header (TheCharlatan)
ad80abc73df38f94d887a905773c4500ca0c2961 kernel: Add block validation to C header (TheCharlatan)
cb1590b05efd090bc2e4be49b5a649f8d248afa0 kernel: Add chainstate loading when instantiating a ChainstateManager (TheCharlatan)
e2c1bd3d713ffe0b8eede711e84f64e0fe4ae836 kernel: Add chainstate manager option for setting worker threads (TheCharlatan)
65571c36a265ec340343b555d1537c58ab335538 kernel: Add chainstate manager object to C header (TheCharlatan)
c62f657ba330572969ab5e86c739712e800bcbcb kernel: Add notifications context option to C header (TheCharlatan)
9e1bac45852d177cf387314a54053a3f7ec8ce99 kernel: Add chain params context option to C header (TheCharlatan)
337ea860dfda12dac084209027a54fba857e7a89 kernel: Add kernel library context object (TheCharlatan)
28d679bad9fda3f180ab0f7d34353e1fa9294d68 kernel: Add logging to kernel library C header (TheCharlatan)
2cf136dec4ce16c8a7c47b35c7c9244dfc3b6da8 kernel: Introduce initial kernel C header API (TheCharlatan)
Pull request description:
This is a first attempt at introducing a C header for the libbitcoinkernel library that may be used by external applications for interfacing with Bitcoin Core's validation logic. It currently is limited to operations on blocks. This is a conscious choice, since it already offers a lot of powerful functionality, but sits just on the cusp of still being reviewable scope-wise while giving some pointers on how the rest of the API could look like.
The current design was informed by the development of some tools using the C header:
* A re-implementation (part of this pull request) of [bitcoin-chainstate](https://github.com/bitcoin/bitcoin/blob/master/src/bitcoin-chainstate.cpp).
* A re-implementation of the python [block linearize](https://github.com/bitcoin/bitcoin/tree/master/contrib/linearize) scripts: https://github.com/TheCharlatan/bitcoin/tree/kernelLinearize
* A silent payment scanner: https://github.com/josibake/silent-payments-scanner
* An electrs index builder: https://github.com/josibake/electrs/commits/electrs-kernel-integration
* A rust bitcoin node: https://github.com/TheCharlatan/kernel-node
* A reindexer: https://github.com/TheCharlatan/bitcoin/tree/kernelApi_Reindexer
The library has also been used by other developers already:
* A historical block analysis tool: https://github.com/ismaelsadeeq/mining-analysis
* A swiftsync hints generator: https://github.com/theStack/swiftsync-hints-gen
* Fast script validation in floresta: https://github.com/vinteumorg/Floresta/pull/456
* A swiftsync node implementation: https://github.com/2140-dev/swiftsync/tree/master/node
Next to the C++ header also made available in this pull request, bindings for other languages are available here:
* Rust: https://github.com/TheCharlatan/rust-bitcoinkernel
* Python: https://github.com/stickies-v/py-bitcoinkernel
* Go: https://github.com/stringintech/go-bitcoinkernel
* Java: https://github.com/yuvicc/java-bitcoinkernel
The rust bindings include unit and fuzz tests for the API.
The header currently exposes logic for enabling the following functionality:
* Feature-parity with the now deprecated libbitcoin-consensus
* Optimized sha256 implementations that were not available to previous users of libbitcoin-consensus thanks to a static kernel context
* Full support for logging as well as control over categories and severity
* Feature parity with the existing experimental bitcoin-chainstate
* Traversing the block index as well as using block index entries for reading block and undo data.
* Running the chainstate in memory
* Reindexing (both full and chainstate-only)
* Interrupting long-running functions
The pull request introduces a new kernel-only test binary that purely relies on the kernel C header and the C++ standard library. This is intentionally done to show its capabilities without relying on other code inside the project. This may be relaxed to include some of the existing utilities, or even be merged into the existing test suite.
The complete docs for the API as well as some usage examples are hosted on [thecharlatan.ch/kernel-docs](https://thecharlatan.ch/kernel-docs/index.html). The docs are generated from the following repository (which also holds the examples): [github.com/TheCharlatan/kernel-docs](https://github.com/TheCharlatan/kernel-docs).
#### How can I review this PR?
Scrutinize the commit messages, run the tests, write your own little applications using the library, let your favorite code sanitizer loose on it, hook it up to your fuzzing infrastructure, profile the difference between the existing bitcoin-chainstate and the bitcoin-chainstate introduced here, be nitty on the documentation, police the C interface, opine on your own API design philosophy.
To get a feeling for the API, read through the tests, or one of the examples.
To configure this PR for making the shared library and the bitcoin-chainstate and test_kernel utilities available:
```
cmake -B build -DBUILD_KERNEL_LIB=ON -DBUILD_UTIL_CHAINSTATE=ON
```
Once compiled the library is part of the build artifacts that can be installed with:
```
cmake --install build
```
#### Why a C header (and not a C++ header)
* Shipping a shared library with a C++ header is hard, because of name mangling and an unstable ABI.
* Mature and well-supported tooling for integrating C exists for nearly every popular language.
* C offers a reasonably stable ABI
Also see https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2285719575.
#### What about versioning?
The header and library are still experimental and I would expect this to remain so for some time, so best not to worry about versioning yet.
#### Potential future additions
In future, the C header could be expanded to support (some of these have been roughly implemented):
* Handling transactions, block headers, coins cache, utxo set, meta data, and the mempool
* Adapters for an abstract coins store
* Adapters for an abstract block store
* Adapters for an abstract block tree store
* Allocators and buffers for more efficient memory usage
* An "[io-less](https://sans-io.readthedocs.io/how-to-sans-io.html)" interface
* Hooks for an external mempool, or external policy rules
#### Current drawbacks
* For external applications to read the block index of an existing Bitcoin Core node, Bitcoin Core needs to shut down first, since leveldb does not support reading across multiple processes. Other than migrating away from leveldb, there does not seem to be a solution for this problem. Such a migration is implemented in #32427.
* The fatal error handling through the notifications is awkward. This is partly improved through #29642.
* Handling shared pointers in the interfaces is unfortunate. They make ownership and freeing of the resources fuzzy and poison the interfaces with additional types and complexity. However, they seem to be an artifact of the current code that interfaces with the validation engine. The validation engine itself does not seem to make extensive use of these shared pointers.
* If multiple instances of the same type of objects are used, there is no mechanism for distinguishing the log messages produced by each of them. A potential solution is #30342.
* The background leveldb compaction thread may not finish in time leading to a non-clean exit. There seems to be nothing we can do about this, outside of patching leveldb.
ACKs for top commit:
alexanderwiederin:
re-ACK 6c7a34f3b0
stringintech:
re-ACK 6c7a34f
laanwj:
Code review ACK 6c7a34f3b0bd39ef7a1520aac56e12f78e5cc969
ismaelsadeeq:
reACK 6c7a34f3b0bd39ef7a1520aac56e12f78e5cc969 👾
fanquake:
ACK 6c7a34f3b0bd39ef7a1520aac56e12f78e5cc969 - soon we'll be running bitcoin (kernel)
Tree-SHA512: ffe7d4581facb7017d06da8b685b81f4b5e4840576e878bb6845595021730eab808d8f9780ed0eb0d2b57f2647c85dcb36b6325180caaac469eaf339f7258030
79d6f458e2300e1f47b94467cda233e1c761f8be random: scope environ extern to macOS, BSDs and Illumos (fanquake)
Pull request description:
These platforms fail to compile with it removed.
macOS: #33675
BSDs / Illumos: https://github.com/hebasto/bitcoin-core-nightly/pull/79.
ACKs for top commit:
l0rinc:
ACK 79d6f458e2300e1f47b94467cda233e1c761f8be
hebasto:
re-ACK 79d6f458e2300e1f47b94467cda233e1c761f8be.
Tree-SHA512: dcaa15f0939d65a804107ceb110037f44d0ff70759f4d42fcc497a9c173ac28b1287b867f01732224788d1c1f9c883565bafc3abed3ccf28f1b67f23997ce3cf
5d784bebaff5e3acc0b5180ee51d9a16aec0e356 clang-tidy: Disable `ArrayBound` check in src/ipc and src/test (Hennadii Stepanov)
5efdb0ef305624e5f3666441e761c658f38a8b39 ci: Update Clang in "tidy" job (Hennadii Stepanov)
Pull request description:
This PR:
1. Updates to [IWYU 0.25](https://github.com/include-what-you-use/include-what-you-use/releases/tag/0.25), which is compatible with Clang 21.
2. Fixes new "modernize-use-default-member-init" warnings. The warning in `interpreter.cpp` is a [false positive](https://github.com/llvm/llvm-project/issues/160394), so it has been suppressed.
ACKs for top commit:
maflcko:
review ACK 5d784bebaff5e3acc0b5180ee51d9a16aec0e356 🎒
ryanofsky:
Code review ACK 5d784bebaff5e3acc0b5180ee51d9a16aec0e356, just adding clang version comment since last review.
Tree-SHA512: a1d853675ec064170ee0f1cd16be6a900676588d4a1e7b5def8733933b140ba1a9520ec6f6a42bf7638b2ff7cf2fe4d5866d407f68b677b49d2bd68ff345f735
Now that an API has been defined, remove the override for symbol
visibility of the library. It is now possible to build the library with
reduced exports.
This introduces the transaction outpoint, input and id types. This now
allows a user to retrieve a transaction output from a prior transaction
that a transaction outpoint is pointing to by either scanning through
all available transactions, or maintaining a data structure for lookups.
This is exercised in the tests by verifying the script of every
transaction in the test chain.
Introduce btck_BlockHash as a type-safe identifier for a block. Adds
functions to retrieve block tree entries by hash or height, get block
hashes and heights from entries. access the genesis block, and check if
blocks are in the active chain.
This adds functions for reading the undo data from disk with a retrieved
block tree entry. The undo data of a block contains all the spent
script pubkeys of all the transactions in a block. For ease of
understanding the undo data is renamed to spent outputs with seperate
data structures exposed for a block's and a transaction's spent outputs.
In normal operations undo data is used during re-orgs. This data might
also be useful for building external indexes, or to scan for silent
payment transactions.
Internally the block undo data contains a vector of transaction undo
data which contains a vector of the coins consumed. The coins are all
int the order of the transaction inputs of the consuming transactions.
Each coin can be used to retrieve a transaction output and in turn a
script pubkey and amount.
This translates to the three-level hierarchy the api provides: Block
spent outputs contain transaction spent outputs, which contain
individual coins. Each coin includes the associated output, the height
of the block is contained in, and whether it is from a coinbase
transaction.
This adds functions for reading a block from disk with a retrieved block
tree entry. External services that wish to build their own index, or
analyze blocks can use this to retrieve block data.
The block tree can now be traversed from the tip backwards. This is
guaranteed to work, since the chainstate maintains an internal block
tree index in memory and every block (besides the genesis) has an
ancestor.
The user can use this function to iterate through all blocks in the
chain (starting from the tip). The tip is retrieved from a separate
`Chain` object, which allows distinguishing whether entries are
currently in the best chain. Once the block tree entry for the genesis
block is reached a nullptr is returned if the user attempts to get the
previous entry.
This adds a function for streaming bytes into a user-owned data
structure.
Use it in the tests for verifying the implementation of the validation
interface's `BlockChecked` method.
These allow for the interpretation of the data in a `BlockChecked`
validation interface callback. The validation state passed through
`BlockChecked` is the source of truth for the validity of a block (the
mode). It is
also useful to get richer information in case a block failed to
validate (the result).
This adds the infrastructure required to process validation events. For
now the external validation interface only has support for the
`BlockChecked` , `NewPoWValidBlock`, `BlockConnected`, and
`BlockDisconnected` callback. Support for the other internal
validation interface methods can be added in the future.
The validation interface follows an architecture for defining its
callbacks and ownership that is similar to the notifications.
The task runner is created internally with a context, which itself
internally creates a unique ValidationSignals object. When the user
creates a new chainstate manager the validation signals are internally
passed to the chainstate manager through the context.
A validation interface can register for validation events with a
context. Internally the passed in validation interface is registerd with
the validation signals of a context.
The callbacks block any further validation execution when they are
called. It is up to the user to either multiplex them, or use them
otherwise in a multithreaded mechanism to make processing the validation
events non-blocking.
I.e. for a synchronous mechanism, the user executes instructions
directly at the end of the callback function:
```mermaid
sequenceDiagram
participant V as Validation
participant C as Callback
V->>C: Call callback
Note over C: Process event (blocks)
C-->>V: Return
Note over V: Validation resumes
```
To avoid blocking, the user can submit the data to e.g. a worker thread
or event manager, so processing happens asynchronously:
```mermaid
sequenceDiagram
participant V as Validation
participant C as Callback
participant W as Worker Thread
V->>C: Call callback
C->>W: Submit to worker thread
C-->>V: Return immediately
Note over V: Validation continues
Note over W: Process event async
```
Add `btck_import_blocks` to import block data and rebuild indexes. The
function can either reindex all existing block files if the indexes were
previously wiped through the chainstate manager options, or import
blocks from specified file paths.
This allows a user to run the kernel without creating on-disk files for
the block tree and chainstate indexes. This is potentially useful in
scenarios where the user needs to do some ephemeral validation
operations.
One specific use case is when linearizing the blocks on disk. The block
files store blocks out of order, so a program may utilize the library
and its header to read the blocks with one chainstate manager, and then
write them back in order, and without orphans, with another chainstate
maanger. To save disk resources and if the indexes are not required once
done, it may be beneficial to keep the indexes in memory for the
chainstate manager that writes the blocks back again.
Adds options for wiping the chainstate and block tree indexes to the
chainstate manager options. In combination and once the
`*_import_blocks(...)` function is added in a later commit, this
triggers a reindex. For now, it just wipes the existing data.
The added function allows the user process and validate a given block
with the chainstate manager. The *_process_block(...) function does some
preliminary checks on the block before passing it to
`ProcessNewBlock(...)`. These are similar to the checks in the
`submitblock()` rpc.
Richer processing of the block validation result will be made available
in the following commits through the validation interface.
The commits also adds a utility for deserializing a `CBlock`
(`kernel_block_create()`) that may then be passed to the library for
processing.
The tests exercise the function for both mainnet and regtest. The
commit also adds the data of 206 regtest blocks (some blocks also
contain transactions).
The library will now internally load the chainstate when a new
ChainstateManager is instantiated.
Options for controlling details of loading the chainstate will be added
over the next few commits.
This is the main driver class for anything validation related, so expose
it here.
Creating the chainstate manager options will currently also trigger the
creation of their respectively configured directories.
The chainstate manager and block manager options are consolidated into a
single object. The kernel might eventually introduce a separate block
manager object for the purposes of being a light-weight block store
reader.
The chainstate manager will associate with the context with which it was
created for the duration of its lifetime and it keeps it in memory with
a shared pointer.
The tests now also create dedicated temporary directories. This is
similar to the behaviour in the existing unit test framework.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
The notifications are used for notifying on connected blocks and on
warning and fatal error conditions.
The user of the C header may define callbacks that gets passed to the
internal notification object in the
`kernel_NotificationInterfaceCallbacks` struct.
Each of the callbacks take a `user_data` argument that gets populated
from the `user_data` value in the struct. It can be used to recreate the
structure containing the callbacks on the user's side, or to give the
callbacks additional contextual information.
As a first option, add the chainparams. For now these can only be
instantiated with default values. In future they may be expanded to take
their own options for regtest and signet configurations.
This commit also introduces a unique pattern for setting the option
values when calling the `*_set(...)` function.
The context introduced here holds the objects that will be required for
running validation tasks, such as the chosen chain parameters, callbacks
for validation events, and interrupt handling. These will be used by the
chainstate manager introduced in subsequent commits.
This commit also introduces conventions for defining option objects. A
common pattern throughout the C header will be:
```
options = object_option_create();
object = object_create(options);
```
This allows for more consistent usage of a "builder pattern" for
objects where options can be configured independently from
instantiation.
Exposing logging in the kernel library allows users to follow
operations. Users of the C header can use
`kernel_logging_connection_create(...)` to pass a callback function to
Bitcoin Core's internal logger. Additionally the level and category can
be globally configured.
By default, the logger buffers messages until
`kernel_loggin_connection_create(...)` is called. If the user does not
want any logging messages, it is recommended that
`kernel_disable_logging()` is called, which permanently disables the
logging and any buffering of messages.
Co-authored-by: stringintech <stringintech@gmail.com>
As a first step, implement the equivalent of what was implemented in the
now deprecated libbitcoinconsensus header. Also add a test binary to
exercise the header and library.
Unlike the deprecated libbitcoinconsensus the kernel library can now use
the hardware-accelerated sha256 implementations thanks for its
statically-initialzed context. The functions kept around for
backwards-compatibility in the libbitcoinconsensus header are not ported
over. As a new header, it should not be burdened by previous
implementations. Also add a new error code for handling invalid flag
combinations, which would otherwise cause a crash.
The macros used in the new C header were adapted from the libsecp256k1
header.
To make use of the C header from C++ code, a C++ header is also
introduced for wrapping the C header. This makes it safer and easier to
use from C++ code.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
In `QSortFilterProxyModel`, `invalidateFilter()` is scheduled for
deprecation in Qt 6.13.
`beginFilterChange()` was introduced in Qt 6.9.
`endFilterChange()` was introduced in Qt 6.10.
4e352efa2ce756c668664486c99d003eef530e0c qt: add createwallet, createwalletdescriptor, and migratewallet to history filter (WakeTrainDev)
Pull request description:
Added `createwallet`, `createwalletdescriptor` and `migratewallet` RPC commands to the Qt console history filter since they may include passphrases or other sensitive data that should not be stored in command history.
ACKs for top commit:
pablomartin4btc:
utACK 4e352efa2ce756c668664486c99d003eef530e0c
hebasto:
ACK 4e352efa2ce756c668664486c99d003eef530e0c.
Tree-SHA512: dc6a12b95173b1e476d483381df3d74add88a1e225c90b1b60db59eab6d504a2496b66890ccec28c691745e405a3053d72afda9d80ae96a703f12cd256e4ebd6
07a926474b5a6fa1d3d4656362a0117611f6da2f node: change a tx-relay on/off flag to enum (Vasil Dimov)
Pull request description:
Previously the `bool relay` argument to `BroadcastTransaction()` designated:
```
relay=true: add to the mempool and broadcast to all peers
relay=false: add to the mempool
```
Change this to an `enum`, so it is more readable and easier to extend with a 3rd option. Consider these example call sites:
```cpp
Paint(true);
// Or
Paint(/*is_red=*/true);
```
vs
```cpp
Paint(RED);
```
The idea for putting `TxBroadcastMethod` into `node/types.h` by Ryan.
---
This is part of [#29415 Broadcast own transactions only via short-lived Tor or I2P connections](https://github.com/bitcoin/bitcoin/pull/29415). Putting it in its own PR to reduce the size of #29415 and because it does not logically depend on the other commits from there.
ACKs for top commit:
optout21:
ACK 07a926474b5a6fa1d3d4656362a0117611f6da2f
kevkevinpal:
ACK [07a9264](07a926474b)
laanwj:
Concept and code review ACK 07a926474b5a6fa1d3d4656362a0117611f6da2f. Agree with the general reasoning and the change in #29415 is a valid motivation to change this interface.
glozow:
utACK 07a926474b5a6fa1d3d4656362a0117611f6da2f
Tree-SHA512: ec8f6fa56a6d2422a0fbd5941ff2792685e8d8e7b9dd50bba9f3e21ed9b4a4a26c89b0d7e4895d48f30b7a635f2eddd894af26b5266410952cbdaf5c40b42966
1a1f46c2285994908df9c11991c1f363c9733087 refactor/doc: Add blockman param to `GetTransaction` doc comment and reorder out param (Musa Haruna)
Pull request description:
Follow-up to [#27125](https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1190350876)
This PR addresses a minor documentation and style nit mentioned during review:
- Adds the missing `@param[in] blockman` line to the `GetTransaction()` doc comment.
- Moves the output parameter `hashBlock` to the end of both the function
declaration and definition, as suggested in the comment.
ACKs for top commit:
l0rinc:
ACK 1a1f46c2285994908df9c11991c1f363c9733087
maflcko:
re-lgtm-ut-cr-rfm-ACK 1a1f46c2285994908df9c11991c1f363c9733087
kevkevinpal:
reACK [1a1f46c](1a1f46c228)
Tree-SHA512: 5807a1ae6c383e691e948648dcb1e029620eaff3dcdff73d88c6dc268a7af5559a30c491b72f038b3f7e812e1845f4f063b49bd3671edfac1bb3a170c84be4f5
b6f8c48946cbfceb066de660c485ae1bd2c27cc1 coins: increase default `dbbatchsize` to 32 MiB (Lőrinc)
8bbb7b8bf8e3b2b6465f318ec102cc5275e5bf8c refactor: Extract default batch size into kernel (Lőrinc)
Pull request description:
This change is part of [[IBD] - Tracking PR for speeding up Initial Block Download](https://github.com/bitcoin/bitcoin/pull/32043)
### Summary
When the in-memory UTXO set is flushed to LevelDB (after IBD or AssumeUTXO load), it does so in batches to manage memory usage during the flush.
A hidden `-dbbatchsize` config option exists to modify this value. This PR only changes the default from `16` MiB to `32` MiB.
Using a larger default reduces the overhead of many small writes and improves I/O efficiency (especially on HDDs). It may also help LevelDB optimize writes more effectively (e.g., via internal ordering).
The change is meant to speed up a critical part of IBD: dumping the accumulated work to disk.
### Context
The UTXO set has grown significantly since [2017](https://github.com/bitcoin/bitcoin/pull/10148/files#diff-d102b6032635ce90158c1e6e614f03b50e4449aa46ce23370da5387a658342fdR26-R27), when the original fixed 16 MiB batch size was chosen.
With the current multi-gigabyte UTXO set and the common practice of using larger `-dbcache` values, the fixed 16 MiB batch size leads to several inefficiencies:
* Flushing the entire UTXO set often requires thousands of separate 16 MiB write operations.
* Particularly on HDDs, the cumulative disk seek time and per-operation overhead from numerous small writes significantly slow down the flushing process.
* Each `WriteBatch` call incurs internal LevelDB overhead (e.g., MemTable handling, compaction triggering logic). More frequent, smaller batches amplify this cumulative overhead.
Flush times of 20-30 minutes are not uncommon, even on capable hardware.
### Considerations
As [noted by sipa](https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2587500105), flushing involves a temporary memory usage increase as the batch is prepared. A larger batch size naturally leads to a larger peak during this phase. Crashing due to OOM during a flush is highly undesirable, but now that [#30611](https://github.com/bitcoin/bitcoin/pull/30611) is merged, the most we'd lose is the first hour of IBD.
Increasing the LevelDB write batch size from 16 to 32 MiB raised the measured peaks by ~70 MiB in my tests during UTXO dump. The option remains hidden, and users can always override it.
The increased peak memory usage (detailed below) is primarily attributed to LevelDB's `leveldb::Arena` (backing MemTables) and the temporary storage of serialized batch data (e.g., `std::string` in `CDBBatch::WriteImpl`).
Performance gains are most pronounced on systems with slower I/O (HDDs), but some SSDs also show measurable improvements.
### Measurements:
AssumeUTXO proxy, multiple runs with error bars (flushing time is faster that the measured loading + flushing):
* Raspberry Pi, dbcache=500: ~30% faster with 32 MiB vs 16 MiB, peak +~75 MiB and still < 1 GiB.
* i7 + HDD: results vary by dbcache, but 32 MiB usually beats 16 MiB and tracks close to 64 MiB without the larger peak.
* i9 + fast NVMe: roughly flat across 16/32/64 MiB. The goal here is to avoid regressions, which holds.
### Reproducer:
```bash
# Set up a clean demo environment
rm -rfd demo && mkdir -p demo
# Build Bitcoin Core
cmake -B build -DCMAKE_BUILD_TYPE=Release && cmake --build build -j$(nproc)
# Start bitcoind with minimal settings without mempool and internet connection
build/bin/bitcoind -datadir=demo -stopatheight=1
build/bin/bitcoind -datadir=demo -blocksonly=1 -connect=0 -dbcache=3000 -daemon
# Load the AssumeUTXO snapshot, making sure the path is correct
# Expected output includes `"coins_loaded": 184821030`
build/bin/bitcoin-cli -datadir=demo -rpcclienttimeout=0 loadtxoutset ~/utxo-880000.dat
# Stop the daemon and verify snapshot flushes in the logs
build/bin/bitcoin-cli -datadir=demo stop
grep "FlushSnapshotToDisk: completed" demo/debug.log
```
---
This PR originally proposed 64 MiB, then a dynamic size, but both were dropped: 64 MiB increased peaks more than desired on low-RAM systems, and the dynamic variant underperformed across mixed hardware. 32 MiB is a simpler default that captures most of the gains with a modest peak increase.
For more details see: https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-3234329502
---
While the PR isn't about IBD in general, rather about a critical section of it, I have measured a reindex-chainstate until 900k blocks, showing a 1% overall speedup:
<details>
<summary>Details</summary>
```python
COMMITS="e6bfd95d5012fa1d91f83bf4122cb292afd6277f af653f321b135a59e38794b537737ed2f4a0040b"; \
STOP=900000; DBCACHE=10000; \
CC=gcc; CXX=g++; \
BASE_DIR="/mnt/my_storage"; DATA_DIR="$BASE_DIR/BitcoinData"; LOG_DIR="$BASE_DIR/logs"; \
(echo ""; for c in $COMMITS; do git fetch -q origin $c && git log -1 --pretty='%h %s' $c || exit 1; done; echo "") && \
hyperfine \
--sort command \
--runs 1 \
--export-json "$BASE_DIR/rdx-$(sed -E 's/(\w{8})\w+ ?/\1-/g;s/-$//'<<<"$COMMITS")-$STOP-$DBCACHE-$CC.json" \
--parameter-list COMMIT ${COMMITS// /,} \
--prepare "killall bitcoind 2>/dev/null; rm -f $DATA_DIR/debug.log; git checkout {COMMIT}; git clean -fxd; git reset --hard && \
cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release && ninja -C build bitcoind && \
./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=$STOP -dbcache=1000 -printtoconsole=0; sleep 10" \
--cleanup "cp $DATA_DIR/debug.log $LOG_DIR/debug-{COMMIT}-$(date +%s).log" \
"COMPILER=$CC ./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=$STOP -dbcache=$DBCACHE -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0"
e6bfd95d50 Merge bitcoin-core/gui#881: Move `FreespaceChecker` class into its own module
af653f321b coins: derive `batch_write_bytes` from `-dbcache` when unspecified
Benchmark 1: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=900000 -dbcache=10000 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = e6bfd95d5012fa1d91f83bf4122cb292afd6277f)
Time (abs ≡): 25016.346 s [User: 30333.911 s, System: 826.463 s]
Benchmark 2: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=900000 -dbcache=10000 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = af653f321b135a59e38794b537737ed2f4a0040b)
Time (abs ≡): 24801.283 s [User: 30328.665 s, System: 834.110 s]
Relative speed comparison
1.01 COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=900000 -dbcache=10000 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = e6bfd95d5012fa1d91f83bf4122cb292afd6277f)
1.00 COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=900000 -dbcache=10000 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = af653f321b135a59e38794b537737ed2f4a0040b)
```
</details>
ACKs for top commit:
laanwj:
Concept and code review ACK b6f8c48946cbfceb066de660c485ae1bd2c27cc1
TheCharlatan:
ACK b6f8c48946cbfceb066de660c485ae1bd2c27cc1
andrewtoth:
ACK b6f8c48946cbfceb066de660c485ae1bd2c27cc1
Tree-SHA512: a72008feca866e658f0cb4ebabbeee740f9fb13680e517b9d95eaa136e627a9dd5ee328456a2bf040401f4a1977ffa7446ad13f66b286b3419ff0c35095a3521
fa4b52bd16189d40761c5976b8427e30779aba23 fuzz: refactor memcpy to std::ranges::copy to work around ubsan warn (MarcoFalke)
Pull request description:
Using std::ranges::copy from the C++ standard library has a few benefits here:
* It has the additional benefit of being a bit more type safe and document the byte cast explicitly.
* The compiler will likely optimize it to the same asm, but performance doesn't really matter here anyway.
* It has defined semantics for empty source ranges.
Fixes https://github.com/bitcoin/bitcoin/issues/33643
ACKs for top commit:
marcofleon:
tACK fa4b52bd16189d40761c5976b8427e30779aba23
dergoegge:
utACK fa4b52bd16189d40761c5976b8427e30779aba23
Tree-SHA512: 04fcf096e3cfc526e996c9313ec6e0a4d12c382fa19cb846b51564d33de2f0ef78a588fc6a936da0c76ca8bc9d9db4a824c36d99413db4f538a98239864d48f0
5fa81e239a39d161a6d5aba7bcc7e1f22a5be777 test: add valid tx test with minimum-sized ECDSA signature (8 bytes DER-encoded) (Sebastian Falbesoner)
Pull request description:
Currently in our tests, all ECDSA signatures passing verification have sizes of 69 bytes and above (that's the DER-encoded size, i.e. counted without the sighash flag byte) [1]. This PR adds test coverage for the minimum-sized valid case of 8 bytes, by taking an interesting testnet transaction that I stumbled upon:
https://mempool.space/testnet/tx/c6c232a36395fa338da458b86ff1327395a9afc28c5d2daa4273e410089fd433
Note that this is a very obscure construction that only works because the public key used isn't contained in the locking script, but calculated and provided later at spending time (see https://bitcointalk.org/index.php?topic=1729534.msg17309060#msg17309060 for an explainer), to match the message (sighash) and picked signature. So this doesn't represent a use-case that really makes sense in practice, but it can still appear in a block (not in mempool though, due to `SCRIPT_VERIFY_CONST_SCRIPTCODE`), and having test-coverage seems useful.
Can be tested with same patch below (tests crash with the condition `>= 9`, but pass with `>= 8`).
[1] this can be verified by applying the following patch and running the tests:
```diff
diff --git a/src/pubkey.cpp b/src/pubkey.cpp
index a4ca9a170a..bee0caa603 100644
--- a/src/pubkey.cpp
+++ b/src/pubkey.cpp
@@ -288,7 +288,9 @@ bool CPubKey::Verify(const uint256 &hash, const std::vector<unsigned char>& vchS
/* libsecp256k1's ECDSA verification requires lower-S signatures, which have
* not historically been enforced in Bitcoin, so normalize them first. */
secp256k1_ecdsa_signature_normalize(secp256k1_context_static, &sig, &sig);
- return secp256k1_ecdsa_verify(secp256k1_context_static, &sig, hash.begin(), &pubkey);
+ bool ret = secp256k1_ecdsa_verify(secp256k1_context_static, &sig, hash.begin(), &pubkey);
+ if (ret) assert(vchSig.size() >= 69);
+ return ret;
}
```
ACKs for top commit:
ajtowns:
ACK 5fa81e239a39d161a6d5aba7bcc7e1f22a5be777 lgtm
fjahr:
tACK 5fa81e239a39d161a6d5aba7bcc7e1f22a5be777
real-or-random:
utACK 5fa81e239a interesting case
Tree-SHA512: d1f0612fdb71c9238ca0420f574f6f246e60dbd11970b23f21d082c759a89ff98a13b12a1f6266f14f20539ec437b7ab79322082278da32984ddfee2d8893356
Using std::ranges::copy from the C++ standard library has a few benefits
here:
* It has the additional benefit of being a bit more type safe and
document the byte cast explicitly.
* The compiler will likely optimize it to the same asm, but performance
doesn't really matter here anyway.
* It works around an UB-Sanitizer bug, when the source range is empty.
Fixes https://github.com/bitcoin/bitcoin/issues/33643
fa0fa0f70087d08fe5a54832b96799bd14293279 refactor: Revert "disable self-assign warning for tests" (MarcoFalke)
faed118fb30fbc303e9d4c70569abfee397f1759 build: Bump clang minimum supported version to 17 (MarcoFalke)
Pull request description:
Most supported operating systems ship with clang-17 (or later), so bump the minimum to that and allow new code to drop workarounds for previous clang bugs.
(Apart from dropping the small workaround, this bump allows the `ci_native_nowallet_libbitcoinkernel` CI to run on riscv64 without running into an ICE with clang-16.)
This patch will only be released in version 31.x, next year (2026).
For reference:
* https://packages.debian.org/bookworm/clang-19
* https://packages.ubuntu.com/noble/clang (clang-18)
* CentOS-like 8/9/10 ship clang-17 (and later) via Stream
* FreeBSD 12/13 ship clang-17 (and later) via packages
* OpenSuse Tumbleweed ships with https://software.opensuse.org/package/clang (clang21); No idea about OpenSuse Leap
On operating systems where the clang version is not shipped by default, the user would have to use GCC, or install clang in a different way. For example:
* https://packages.debian.org/bookworm/g++ (g++-12)
* https://packages.ubuntu.com/jammy/g++ (g++-11)
* https://apt.llvm.org/, or nix, or guix, or compile clang from source, ...
*Ubuntu 22.04 LTS does not ship with clang-16 (the previous minimum required), nor with clang-17, so one of the above workarounds is needed there.*
macOS 14 is unaffected, and the previous minimum requirement of Xcode15.0 remains, see also 919e6d01e9/depends/hosts/darwin.mk (L3-L4). (Modulo compiling the fuzz tests, which requires 919e6d01e9/.github/workflows/ci.yml (L149))
ACKs for top commit:
janb84:
Concept ACK fa0fa0f70087d08fe5a54832b96799bd14293279
l0rinc:
Code review ACK fa0fa0f70087d08fe5a54832b96799bd14293279
hebasto:
ACK fa0fa0f70087d08fe5a54832b96799bd14293279.
Tree-SHA512: 5973cec39982f80b8b43e493cde012d9d1ab75a0362300b007d155db9f871c6341e7e209e5e63f0c3ca490136b684683de270136d62cb56f6b00b0ac0331dc36
53e4951a5b5b9d166d278db4240513d09b447f58 Switch to ANSI Windows API in `fsbridge::fopen()` function (Hennadii Stepanov)
dbe770d9210666a366f055d52b9f34fa8a3d7305 Switch to ANSI Windows API in `Win32ErrorString()` function (Hennadii Stepanov)
06d0be4e22cef08fd7517f42ee82a44475c6363b Remove no longer necessary `WinCmdLineArgs` class (Hennadii Stepanov)
f366408492f6205ee20fe23e5104813de45dd4b1 cmake: Set process code page to UTF-8 on Windows (Hennadii Stepanov)
dccbb178065f05810a0fad57a86bca2f10995ecf Set minimum supported Windows version to 1903 (May 2019 Update) (Hennadii Stepanov)
Pull request description:
The main goal is to remove [deprecated](https://github.com/bitcoin/bitcoin/issues/32361) code (removed in C++26).
This PR employs Microsoft's modern [approach](https://learn.microsoft.com/en-us/windows/apps/design/globalizing/use-utf8-code-page) to handling UTF-8:
> Until recently, Windows has emphasized "Unicode" -W variants over -A APIs. However, recent releases have used the ANSI code page and -A APIs as a means to introduce UTF-8 support to apps. If the ANSI code page is configured for UTF-8, then -A APIs typically operate in UTF-8. This model has the benefit of supporting existing code built with -A APIs without any code changes.
TODO:
- [x] Handle application manifests properly when building with MSVC.
- [x] Bump the minimum supported Windows version to 1903 (May 2019 Update).
- [x] Remove all remaining use cases of the deprecated `std:wstring_convert`.
- The instance in `subprocess.h` will be addressed in a follow-up PR, as additional tests are likely needed.
- The usage in `common/system.cpp` is handled in https://github.com/bitcoin/bitcoin/pull/32566.
Resolves partially https://github.com/bitcoin/bitcoin/issues/32361.
ACKs for top commit:
laanwj:
re-ACK 53e4951a5b5b9d166d278db4240513d09b447f58
hodlinator:
re-ACK 53e4951a5b5b9d166d278db4240513d09b447f58
davidgumberg:
untested crACK 53e4951a5b
Tree-SHA512: 0dbe9badca8b979ac2b4814fea6e4a7e53c423a1c96cb76ce894253137d3640a87631a5b22b9645e8f0c2a36a107122eb19ed8e92978c17384ffa8b9ab9993b5
1a7fb5eeeef3575c1e7c27915c9b98695191299d fees: return current block height in estimateSmartFee (ismaelsadeeq)
ab49480d9be4e54aa9db1247b8499f957ba9d166 fees: rename fees_args to block_policy_estimator_args (ismaelsadeeq)
06db08a43568910702207a7963b375e1a7446689 fees: refactor: rename fees to block_policy_estimator (ismaelsadeeq)
6dfdd7e034dd3620f0f8ed54dfe20fa407b5382f fees: refactor: rename policy_fee_tests.cpp to feerounder_tests.cpp (ismaelsadeeq)
Pull request description:
This PR is a simple refactoring that does four things:
1. Renames `test/policy_fee_tests.cpp` to `test/feerounder_tests.cpp`.
2. Renames `policy/fees.{h,cpp}` to `policy/fees/block_policy_estimator.{h,cpp}`.
3. Renames `policy/fees_args.cpp` to `policy/fees/block_policy_estimator_args.cpp`.
4. Modifies `estimateSmartFee` to return the block height at which the estimate was made by adding a `best_height` unsigned int value to the `FeeCalculation` struct.
**Motivation**
In preparation for adding a new fee estimator, the `fees` directory is created so we can organize code into `block_policy_estimator` and `mempool` because
a) It would be clunky to add more code directly under `fees`.
b) Having `policy/fees.{h,cpp}` and `policy/mempool.{h,cpp}` would also be undesirable.
Therefore, it makes sense to structure the it as `policy/fees/block_policy_estimator`, `policy/fees/mempool`, etc.
Hence test file were also updated accordingly.
The current block height is also returned because later in #30157 we log the height at which each estimate is made (at the debug log category of fee estimation :) ). This feature is particularly useful for empirical data analysis.
ACKs for top commit:
maflcko:
re-ACK 1a7fb5eeeef3575c1e7c27915c9b98695191299d 🐤
polespinasa:
re ACK 1a7fb5eeeef3575c1e7c27915c9b98695191299d
willcl-ark:
ACK 1a7fb5eeeef3575c1e7c27915c9b98695191299d
janb84:
re ACK 1a7fb5eeeef3575c1e7c27915c9b98695191299d
Tree-SHA512: fef7ace2a9f262ec0361fb7a46df5108afc46b5c4b059caadf2fd114740aefbb2592389d11646c13d0e28bf0ef2cfcfbab3e659c4d4288b8ebe64725fd1963c0
This change updates to IWYU 0.25, which is compatible with Clang 21.
Fixes new "modernize-use-default-member-init" warnings.
The warning in `interpreter.cpp` is a false positive, so it has been
suppressed.
02d2b5a11c921ef71c971ee80eb3dfbc75c8cb0d ci, iwyu: Treat warnings as errors for specific directories (Hennadii Stepanov)
57a3eac387bd26689aed7682b248b648dba42779 refactor: Fix includes in `index` directory (Hennadii Stepanov)
bdb8eadcdc193f398ebad83911d3297b5257e721 refactor: Fix includes in `crypto` directory (Hennadii Stepanov)
56f2a689a2016ba2ae9cc40833447dff648af809 ci: Do not patch `leveldb` to workaround UB in "tidy" CI job (Hennadii Stepanov)
Pull request description:
This PR is the first step towards treating IWYU warnings as errors. At this stage, it applies only to the `crypto` and `index` directories.
ACKs for top commit:
maflcko:
re-ACK 02d2b5a11c921ef71c971ee80eb3dfbc75c8cb0d 💮
ryanofsky:
Code review ACK 02d2b5a11c921ef71c971ee80eb3dfbc75c8cb0d. Just rebased and update tidy patch comment again since last review
willcl-ark:
ACK 02d2b5a11c921ef71c971ee80eb3dfbc75c8cb0d
Tree-SHA512: 1c966e01c47bf3e7d225faa3b819367f757430e2d71e9582fa82d67307aabe3f0d76f69346ee180192e7f5ab194ecc58d2b8ecf178eab26ba3309a6b55bff4b6
664657ed134365588914c2cf6a3975ce368a4f49 bugfix: disallow label for ranged descriptors & allow external non-ranged descriptors to have label (scgbckbone)
Pull request description:
Motivation:
* ranged descriptors MUST not be able to have label (current impl allows it)
* external non-ranged descriptor MUST be able to have label (current impl disallows it, **if** `internal=false` is provided via importdescriptor user data)
Repro steps:
* create blank wallet and import descriptors
* external has `label=test` (not internal)
```
conn = bitcoind.create_wallet(wallet_name=w_name, disable_private_keys=True, blank=True,
passphrase=None, avoid_reuse=False, descriptors=True)
descriptors = [
{
"timestamp": "now",
"label": "test",
"active": True,
"desc": "wpkh([0f056943/84h/1h/0h]tpubDC7jGaaSE66Pn4dgtbAAstde4bCyhSUs4r3P8WhMVvPByvcRrzrwqSvpF9Ghx83Z1LfVugGRrSBko5UEKELCz9HoMv5qKmGq3fqnnbS5E9r/0/*)#erexmnep",
"internal": False
},
{
"desc": "wpkh([0f056943/84h/1h/0h]tpubDC7jGaaSE66Pn4dgtbAAstde4bCyhSUs4r3P8WhMVvPByvcRrzrwqSvpF9Ghx83Z1LfVugGRrSBko5UEKELCz9HoMv5qKmGq3fqnnbS5E9r/1/*)#ghu8xxfe",
"active": True,
"internal": True,
"timestamp": "now"
},
]
r = conn.importdescriptors(descriptors)
print(r)
```
response:
```
[{'error': {'code': -8,
'message': 'Internal addresses should not have a label'},
'success': False,
'warnings': ['Range not given, using default keypool range']},
{'success': True,
'warnings': ['Range not given, using default keypool range']}]
```
But in above, ONLY external has a label.
If you remove `internal: False` from external descriptor import object - it will import no problem:
```
[{'success': True,
'warnings': ['Range not given, using default keypool range']},
{'success': True,
'warnings': ['Range not given, using default keypool range']}]
```
Even tho it should NOT, as the descriptor is ranged. Current implementation relies on checking user provided data to decide whether desc is ranged.
ACKs for top commit:
achow101:
ACK 664657ed134365588914c2cf6a3975ce368a4f49
rkrux:
lgtm crACK 664657ed134365588914c2cf6a3975ce368a4f49
Tree-SHA512: 9e70aea620019c29950ba417d4ae38d65cd94a4f6fcabbc021d67b031de1c44c27d6f6f5cb7e6950a099eb6e58bed9be764d4c6347195daeccb14a5d95c123b2
