With this change, callers can use util/log.h to emit log messages and do not need to
include the full logging implementation in logging.h.
There's a potential performance impact with this change from an extra
`strprintf` call in log statements where `Logger::WillLogCategoryLevel` returns
true but `Logger::Enabled` returns false. This happens when bitcoind is run
with `-noprinttoconsole -nodebuglogfile` options.
For background, log macro arguments are supposed to be evaluated when
`Logger::WillLogCategoryLevel` returns true, even if log output is not enabled.
Changing this behavior would be reasonable but needs consideration in a
separate PR since not evaluating arguments in log statements has the potential
to change non-logging behavior.
The extra `strprintf` call could have been avoided by expanding this change and
making the `ShouldLog()` function return a tri-state DO_LOG / DO_NOT_LOG /
DO_NOT_LOG_ONLY_EVALUATE_ARGS value instead of a bool, but this complexity did
not seem warranted.
Review with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Move logging macros to util/log.h so the entire codebase can use the same
macros.
Review with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
This is not strictly move-only because BCLog::Level is now defined as a type
alias for util::log::Level;
Review with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Logging categories are currently shared between node and kernel. This
separation allows future commits to completely remove kernel's
dependency on logging.h.
Also applies clang-format suggestions to the moved code.
Review with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Introduce util/log.h as a lightweight header for code that only needs to emit
log messages. Move SourceLocation into this header so log-emitting code no
longer needs to include logging.h and depend on the full log management API.
This is a move-only change and the first change of several changes that
separate log generation from log handling. It also applies clang-format
suggestions to the moved code.
Review with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
The DataStream comment was a bit stale, because it was using
CDataStream.
Fix it by using assert_debug_log for a self-documenting and
self-checking test code.
3c8f5e48f7 ci: Treat SHA1 LLVM signing key as warning (will)
Pull request description:
The current SHA1 LLVM signing key is considered not secure since
2026-02-01T00:00:00Z which makes this run fail when downloading
packages.
See: https://github.com/llvm/llvm-project/issues/153385
Apply the fix from the issue to temporarily to treat this error as a
warning, until the upstream key can be updated.
This PR should be reverted once the upstream key is updated.
ACKs for top commit:
hebasto:
ACK 3c8f5e48f7, tested by running the "iwyu" CI job locally on Ubuntu 25.10 after burning all podman's caches.
Tree-SHA512: fbccf98bfd73cb338670f1ceea994d277d746acbc88b9b90a403d9a59d82abda0f3ba34c4d484b70926340c2d0c873259f930c36ccd4f9d18bb1d22d49ee70c4
The current SHA1 LLVM signing key is considered not secure since
2026-02-01T00:00:00Z which makes this run fail when downloading
packages.
See: https://github.com/llvm/llvm-project/issues/153385
Apply the fix from the issue to temporarily to treat this error as a
warning, until the upstream key can be updated.
This PR should be reverted once the upstream key is updated.
With 15 runners we get 150GB of cache space to use, and we currently
have 18 jobs using ccache.
Although each run only generates ~ 200-300 MB of cache data on each run,
the small the small size may be contributing to poor hitrate.
Bump ccache to 2GB per job ~ --> 36GB of the total 150GB cache space to
try and reduce any thrashaing and generally increase hitrate.
facb2aab26 test: Turn ElapseSteady into SteadyClockContext (MarcoFalke)
Pull request description:
`ElapseSteady` was introduced a while back, but is only used in one place. It makes more sense if this were a context manager, so that mocktime does not leak from one test into the next.
So turn it into a context manager, rename it and allow easy time advancement via e.g. `steady_ctx += 1h`.
ACKs for top commit:
l0rinc:
ACK facb2aab26
ismaelsadeeq:
utACK facb2aab26
sedited:
ACK facb2aab26
Tree-SHA512: 1df9cc9685d9be4d3ab8deafd99ac1a5ff752064ae54b83bacd6f44ba2c198b091558a306d49d8b1e2200ac669e95915cc792d589fb3a63b2bef7891d325a1e0
3bd98b4508 refactor: use transparent comparator for setBlockIndexCandidates lookups (joaonevess)
Pull request description:
### Rationale
This PR improves code safety by removing a `const_cast` in `src/validation.cpp`.
Currently, `setBlockIndexCandidates` stores mutable `CBlockIndex*`. However, validation logic (like `CVerifyDB`) often holds `const CBlockIndex*`. Previously, checking for existence in the set required casting away constness. While currently benign, this bypasses compiler safety checks and could mask accidental modifications in future refactors.
### Description
1. **Enable Heterogeneous Lookup:** Added `using is_transparent = void;` to `CBlockIndexWorkComparator` in `src/node/blockstorage.h`. This allows the `std::set` to natively accept `const CBlockIndex*` for lookup (utilizing C++14 heterogeneous lookup).
2. **Remove Cast:** Removed the now unnecessary `const_cast<CBlockIndex*>` in `src/validation.cpp`, allowing the compiler to strictly enforce const-correctness.
### Notes
- **Refactoring only:** No behavioral change.
- **Verification:** `validation_tests` and `blockmanager_tests` pass.
ACKs for top commit:
maflcko:
review ACK 3bd98b4508🚪
frankomosh:
ACK 3bd98b4508. Good use of transparent comparator to eliminate `const_cast` in this specific code path.
sedited:
ACK 3bd98b4508
Tree-SHA512: 0f76bdce2a54b759dfec99633afce1e95586e62f4057ecf1e82eed1a073eb8ecb2d659ccbf28a7a139f0aa09a30f058ac6966cafdfbf1f2ee878fa2d86b2c487
51abf7d15b script: remove unused SCRIPT_ERR_LAST (Antoine Poinsot)
Pull request description:
It was introduced in ab9edbd6b6 and never used since. It seems it might have been intended to be exposed as part of a public library interface, which has since been superseded.
The only call site uses SCRIPT_ERR_ERROR_COUNT directly.
ACKs for top commit:
billymcbip:
tACK 51abf7d15b
sedited:
ACK 51abf7d15b
theStack:
ACK 51abf7d15b
Tree-SHA512: 983b0523b2b5eba57732223af22746c9f29e4759d23366147825d1101f94a9b10c385f305d1425c439a4e29ab28f5a9245691ba6dc31a13f260d3d03b0bf1885
eeb4d28148 validation: follow-up nits for lock-free `IsInitialBlockDownload()` (Lőrinc)
Pull request description:
Innocent follow-up to #34253:
* Add `AssertLockHeld(cs_main)` to `ChainstateManager::UpdateIBDStatus()` given it's already annotated with `EXCLUSIVE_LOCKS_REQUIRED(cs_main)`.
* Fix outdated comment about constness of `ChainstateManager::IsInitialBlockDownload()` (compilation and build passes without it).
* And since we're touching it, we might as well mark `ChainstateManager::IsInitialBlockDownload()` as `noexcept` now.
ACKs for top commit:
davidgumberg:
crACK https://github.com/bitcoin/bitcoin/commit/eeb4d2814803c09af602ca5c9810438dd5e987fb
sedited:
ACK eeb4d28148
mzumsande:
utACK eeb4d28148
Tree-SHA512: 110cf5b03dc4f4cf6e61563ef69da6368e43009cf0fe1b10870cb4f55203c347444c8623aae7357d0ee5ba3f4b10da535b440a5871c9c5a4f7f8f88c2accd1f1
1bf3842223 ci, iwyu: Fix warnings in `src/univalue` and treat them as errors (Hennadii Stepanov)
Pull request description:
This PR continues the ongoing effort to enforce IWYU warnings.
See [Developer Notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#using-iwyu).
ACKs for top commit:
maflcko:
review ACK 1bf3842223🦇
sedited:
ACK 1bf3842223
Tree-SHA512: e2b7ad7b318c6c78de9a6b416a2bcb6da3ec9f4f060590db2b4f5f3a9a167891ac60c68efccea889df806f53b803c98303eed1fc24682257a9c37844ee2ee55d
07af50f789 util: Drop *BSD headers in `batchpriority.cpp` (Hennadii Stepanov)
Pull request description:
Currently, there are issues with headers in `batchpriority.cpp`:
1. `SCHED_BATCH` is not defined on all supported *BSD platforms.
2. `pthread.h` is necessary on other platforms.
This PR addresses both issues and fixes other includes.
ACKs for top commit:
maflcko:
review ACK 07af50f789🤺
w0xlt:
crACK https://github.com/bitcoin/bitcoin/pull/34462/changes/07af50f7896a36a82efc19b5030779ab36302fa4
Tree-SHA512: 388f627e9d9c8b80834ba562034cd6aa44ba37d9156e13cdd6cbcfa93b19fcbf3222228d671784ca363e06fda3c978778f62f4530c3eb4ae3ff96adf91c2d789
9c839aa9e3 iwyu: Document mappings for libc symbols (Hennadii Stepanov)
91824646c5 iwyu: Add temporary mapping to work around upstream bug (Hennadii Stepanov)
37de7d1910 iwyu: Drop backported mapping (Hennadii Stepanov)
Pull request description:
This PR:
1. Removes mappings that have been [backported](https://github.com/include-what-you-use/include-what-you-use/pull/1706) upstream.
2. Adds a new temporary mapping to work around upstream [issue](https://github.com/include-what-you-use/include-what-you-use/issues/1616).
3. Document the existing mappings for libc symbols.
ACKs for top commit:
maflcko:
lgtm ACK 9c839aa9e3
sedited:
ACK 9c839aa9e3
Tree-SHA512: 691fd9fc6798951ca1a621ee607a617ebef2ed2f43196465c78c6cd7a5ea7f0f0e876e68cd54653edf9b6762ae5fdb704469a1524a5f6f1ab8b0eedbff65cc28
b189a34557 test: add case where `TOTAL_TRIES` is exceeded yet solution remains (yancy)
Pull request description:
Show that `CoinGrider` halts searching when the number of attempts exceeds `TOTAL_TRIES`. To do so, show that a solution is found, then add one more entry to the same set of inputs. Since the search orders by `effective_value`, the solution is constructed such that only values with the lowest `effective_value` have the least weight. Only the lowest weight values will not exceed the `max_selection_weight`. Therefore, `CoinGrinder` will not evaluate all lowest weight solutions together before exceeding `TOTAL_TRIES` since they are last found.
This test case was inspired by a similar test for `BnB` currently named `bnb_test`.
ACKs for top commit:
frankomosh:
Code review ACK b189a34
achow101:
ACK b189a34557
murchandamus:
ACK b189a34557
Tree-SHA512: 1df0b6e29ae219edbeed14cfa97f0ad4688d6bf97ed946719ba3c3b69e004f3dee82991578eb5aceb554914b70c5b68feff9e321283c1fc8bc0fedf08df2cb4c
552bc82b17 doc: Use multipath descriptors in descriptors.md and linked test (Anurag chavan)
Pull request description:
Updates documentation and `wallet_miniscript_decaying_multisig_descriptor_psbt.py` to use single multipath descriptors with `<0;1>` syntax instead of separate external/internal descriptors.
## Changes
- **doc/descriptors.md**: Update examples (lines 70-71) to use `/<0;1>/*` multipath syntax
- **doc/descriptors.md**: Update Basic Multisig Example instructions (line 179) to use single multipath descriptor
- **test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py**: Refactor to use single multipath descriptor pattern matching `wallet_multisig_descriptor_psbt.py`
## Implementation
- `_get_xpub()` now extracts external descriptor and converts to multipath format
- `create_multisig()` imports single descriptor that expands to receive and change addresses
- Removed fake checksums from documentation examples
- Added clear comments documenting multipath convention
Fixes#34086
ACKs for top commit:
yashbhutwala:
ACK 552bc82b17
achow101:
ACK 552bc82b17
rkrux:
lgtm tACK 552bc82
Tree-SHA512: cc99271a3955daa475242d9f4ef8f09f4c94c64e48ec4647ecfd95dceb38bb0cdd91b78ec2d5f033b449d175eaecbdda49d6c766c8a1e1a01fed93be4eb0cfc0
6f7b4323cb test: remove UNKNOWN_ERROR from script_tests (Bruno Garcia)
bd31a92d67 script: use SCRIPT_ERR_SCRIPTNUM for CScriptNum errors (Bruno Garcia)
0ca4dcd786 script: add SCRIPT_ERR_SCRIPTNUM error (Bruno Garcia)
Pull request description:
When evaluating a script, the current code is bad for analyzing some errors because it returns `SCRIPT_ERR_UNKNOWN_ERROR` for errors that are clearly known.
`CScriptNum` has two well defined errors: number overflow and non-minimally encoded number. However, for both errors we return as unknown. This PR changes it by adding a new ScriptError that is used for any `CScriptNum` error.
ACKs for top commit:
achow101:
ACK 6f7b4323cb
w0xlt:
ACK https://github.com/bitcoin/bitcoin/pull/34381/commits/6f7b4323cb46687ba9df7073a9cde427985d2dfc
darosior:
ACK 6f7b4323cb
Tree-SHA512: e656d9992251fbc95d33966fa18ce64bf714179d51ba6a7f429e5a55bc58e7fc08827e4ab71ace0dd385dac7e1feaea621b49503387793a30eae7a7e44aa6b0f
964c44cdcd test(miniscript): Prove avoidance of stack overflow (Hodlinator)
198bbaee49 refactor(miniscript): Destroy nodes one full subs-vector at a time (Hodlinator)
50cab8570e refactor(miniscript): Remove NodeRef & MakeNodeRef() (Hodlinator)
15fb34de41 refactor(miniscript): Remove superfluous unique_ptr-indirection (Hodlinator)
e55b23c170 refactor(miniscript): Remove Node::subs mutability (Hodlinator)
c6f798b222 refactor(miniscript): Make fields non-const & private (Hodlinator)
22e4115312 doc(miniscript): Remove mention of shared pointers (Hodlinator)
Pull request description:
Removes one level of unnecessary indirection, which was a change that originally [aided in finding one issue](https://github.com/bitcoin/bitcoin/pull/30866#pullrequestreview-2434704657) in #30866. Simplifies the code one step further than 09a1875ad8 belonging to aforementioned PR.
Also adds test which verifies resistance to stack overflow when it comes to `~Node()` and `Node::Clone()`.
No observed difference when running benchmarks: ExpandDescriptor/WalletIsMineDescriptors/WalletIsMineMigratedDescriptors/WalletLoadingDescriptors.
Followup to #30866.
ACKs for top commit:
achow101:
ACK 964c44cdcd
darosior:
Code review ACK 964c44cdcd
l0rinc:
ACK 964c44cdcd
Tree-SHA512: 32927e8f0f916fb70372ffd110f7ec7207d9e7a099c21c0a7482a12e96593b673c339719f4ab166ad7c086dc43767315fc1742c5b236a3facc45c4cfeb5872e9
516be10bb5 wallet: Rename `RecordType::DELETE` to `RecordType::DELETE_FLAG` (Hennadii Stepanov)
Pull request description:
On Windows, the `winnt.h` header defines `DELETE` as a macro for a "Standard Access Right" bitmask (0x00010000L).
This introduces a fragile dependency on header inclusion order: if Windows headers happen to be included before the `RecordType` enum definition, the preprocessor expands `DELETE` into a numeric literal, causing syntax errors.
Rename the enumerator to `DELETE_FLAG` to remove this fragility and avoid the collision entirely.
Split from https://github.com/bitcoin/bitcoin/pull/34448.
ACKs for top commit:
maflcko:
re-lgtm ACK 516be10bb5
achow101:
ACK 516be10bb5
Tree-SHA512: eba054b395e18c07efb2901b28f542b042b62d85e1a798eeff35f8431530cb667fa791c47c4125cecdb689213b458ba396715495415e9b83bb322509a9376222
Replace the HTTP server's WorkQueue implementation and single threads
handling code with ThreadPool for processing HTTP requests. The
ThreadPool class encapsulates all this functionality on a reusable
class, properly unit and fuzz tested (the previous code was not
unit nor fuzz tested at all).
This cleanly separates responsibilities:
The HTTP server now focuses solely on receiving and dispatching requests,
while ThreadPool handles concurrency, queuing, and execution.
It simplifies init, shutdown and requests tracking.
This also allows us to experiment with further performance improvements at
the task queuing and execution level, such as a lock-free structure, task
prioritization or any other performance improvement in the future, without
having to deal with HTTP code that lives on a different layer.
Currently, there are issues with headers in `batchpriority.cpp`:
1. `SCHED_BATCH` is not defined on all supported *BSD platforms.
2. `pthread.h` is necessary on other platforms.
This addresses both issues and fixes other includes.
Fix a test that isn't implemented as intended. The idea is to test execution order by providing a signature that would cause script failure when parsed. An empty signature does not cause script failure in CHECKMULTISIG. Use OP_1 for the second signature instead of OP_0.
On Windows, the `winnt.h` header defines `DELETE` as a macro for a
"Standard Access Right" bitmask (0x00010000L).
This introduces a fragile dependency on header inclusion order: if
Windows headers happen to be included before this enum definition,
the preprocessor expands `DELETE` into a numeric literal, causing
syntax errors.
Rename the enumerator to `DELETE_FLAG` to remove this fragility and
avoid the collision entirely.
Also, use str(e) consistently in all run helpers.
This refactor does not change any behavior.
This can be reviewed by checking that all instances are exactly
identical code now:
$ git grep --function-context 'def run(cmd'
4fab35cf88 miniscript: correct and_v() properties (Antoine Poinsot)
Pull request description:
`and_v()` must never be 'd'. This is not a bug fix since this was unreachable in valid Miniscripts: the first sub of an `and_v()` must be of type V, which conflicts with (i.e. never has) property 'd'.
ACKs for top commit:
sipa:
ACK 4fab35cf88. Fuzzed for 2 months worth of CPU time.
achow101:
ACK 4fab35cf88
Tree-SHA512: 8932ad2c9188747299cb9147ff097dca8d078ce7bdd0caefa71ee2724ff81d9bef836664211c2081519a45afd50c539974d67c2a3a1a42a65a3b10b1daef8cbe
