2a36d6a561 lint: Require scripted-diff script to succeed (Hodlinator)
Pull request description:
Previous version of commit-script-check.sh would succeed as long as git diff succeeded.
Can be verified through adding a failing scripted diff commit such as:
```
git commit --allow-empty -m $'scripted-diff: foo\n\n-BEGIN VERIFY SCRIPT-\nadsasd\n-END VERIFY SCRIPT-\n'
```
...and running...
```
cargo run --manifest-path ./test/lint/test_runner/Cargo.toml -- --lint=scripted_diff
```
ACKs for top commit:
maflcko:
lgtm ACK 2a36d6a561
furszy:
ACK 2a36d6a561
hebasto:
ACK 2a36d6a561.
Tree-SHA512: 160397cc009b18ccb3bc66c85c1b89404a81c9a85acee534638f4752577165aead56d6cfb21d9fcb424484bc6710658cd0f1d2f483d8298e9e3a2f5243ef8e8b
Previous version of commit-script-check.sh would succeed as long as git diff succeeded.
Can be verified through adding a failing scripted diff commit such as:
git commit --allow-empty -m $'scripted-diff: foo\n\n-BEGIN VERIFY SCRIPT-\nadsasd\n-END VERIFY SCRIPT-\n'
...and running...
cargo run --manifest-path ./test/lint/test_runner/Cargo.toml -- --lint=scripted_diff
32df86f1d8 argsman: Prevent duplicate option registration across categories (Pablo Martin)
Pull request description:
Follow-up to #28802
This PR enforces the invariant that option names are unique across categories.
-<ins>_**Rationale**_</ins>:
- While adapting the `argsman_tests.cpp` cases introduced in #28802 for the GNU-style parsing changes proposed in #33540, I noticed that the same option name can currently be registered in multiple categories.
- At present in `master`, this ambiguity is largely masked by the existing command-line parsing behaviour. However, it relies on assumptions about how options are interpreted based on their position. Future changes to option parsing, such as the GNU-style parsing proposed in #33540, may expose this ambiguity and lead to unexpected option resolution.
- To avoid ambiguous option resolution and make the distinction between global and command-specific options explicit, this PR adds validation in `AddArg()` preventing the same option name from being registered across different categories.
ACKs for top commit:
sedited:
ACK 32df86f1d8
ryanofsky:
Code review ACK 32df86f1d8. Thanks for the simplifications!
Tree-SHA512: 5d3cb951bd90c46cbd8205a229d49336c3a29e5100b7c50c5cba66979dd3cbd480ed42593caa272c6205eb7ac503af631a7e3531af0b726334e3448e17d5c0d5
55a4c946f6 test: descriptor: bare multisig at TOP level with 3 pubkeys is allowed (brunoerg)
Pull request description:
When running mutation testing for `src/script/descriptor.cpp`, I noticed that the following mutant was not killed:
```diff
diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index 3b73a40cce..29775ba75f 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -2393,7 +2393,7 @@ std::vector<std::unique_ptr<DescriptorImpl>> ParseScript(uint32_t& key_exp_index
return {};
}
if (ctx == ParseScriptContext::TOP) {
- if (providers.size() > 3) {
+ if (providers.size() >= 3) {
error = strprintf("Cannot have %u pubkeys in bare multisig; only at most 3 pubkeys", providers.size());
return {};
}
```
We current have a test case that verifies we cannot have 4 pubkeys in bare multisig, but we are not testing that exactly 3 pubkeys are allowed. This PR adds a test case for it.
ACKs for top commit:
darosior:
utACK 55a4c946f6
sedited:
ACK 55a4c946f6
Tree-SHA512: 2e0919b1cf0d991477a545d31de79c60f1378b027d88fb35e03cd3150d6bc9e3db8f231ccaee8f2800b464f50495546848e0f04e0152239296f1b5f6f38bb49a
2cf2b22ff1 depends: Drop trailing slash from `CMAKE_INSTALL_LIBDIR` (Hennadii Stepanov)
Pull request description:
1. The trailing slash is [redundant](https://cmake.org/cmake/help/latest/module/GNUInstallDirs.html).
2. It causes [CMP0177](https://cmake.org/cmake/help/latest/policy/CMP0177.html)-specifc warnings when using CMake >=3.31.
On the master branch:
```
$ gmake -C depends freetype_configured
gmake: Entering directory '/home/hebasto/dev/bitcoin/depends'
Extracting freetype...
/home/hebasto/dev/bitcoin/depends/sources/freetype-2.11.1.tar.gz: OK
Preprocessing freetype...
patching file CMakeLists.txt
Configuring freetype...
-- The C compiler identification is GNU 15.2.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/gcc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Looking for unistd.h
-- Looking for unistd.h - found
-- Looking for fcntl.h
-- Looking for fcntl.h - found
CMake Warning (dev) at CMakeLists.txt:577 (install):
Policy CMP0177 is not set: install() DESTINATION paths are normalized. Run
"cmake --help-policy CMP0177" for policy details. Use the cmake_policy
command to set the policy and suppress this warning.
This warning is for project developers. Use -Wno-dev to suppress it.
CMake Warning (dev) at CMakeLists.txt:596 (install):
Policy CMP0177 is not set: install() DESTINATION paths are normalized. Run
"cmake --help-policy CMP0177" for policy details. Use the cmake_policy
command to set the policy and suppress this warning.
This warning is for project developers. Use -Wno-dev to suppress it.
CMake Warning (dev) at CMakeLists.txt:601 (install):
Policy CMP0177 is not set: install() DESTINATION paths are normalized. Run
"cmake --help-policy CMP0177" for policy details. Use the cmake_policy
command to set the policy and suppress this warning.
This warning is for project developers. Use -Wno-dev to suppress it.
-- Configuring done (0.2s)
-- Generating done (0.0s)
CMake Warning:
Manually-specified variables were not used by the project:
CMAKE_EXPORT_NO_PACKAGE_REGISTRY
-- Build files have been written to: /home/hebasto/dev/bitcoin/depends/work/build/x86_64-pc-linux-gnu/freetype/2.11.1-0539b8d2229/build
gmake: Leaving directory '/home/hebasto/dev/bitcoin/depends'
```
With this PR:
```
$ gmake -C depends freetype_configured
gmake: Entering directory '/home/hebasto/dev/bitcoin/depends'
Extracting freetype...
/home/hebasto/dev/bitcoin/depends/sources/freetype-2.11.1.tar.gz: OK
Preprocessing freetype...
patching file CMakeLists.txt
Configuring freetype...
-- The C compiler identification is GNU 15.2.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/gcc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Looking for unistd.h
-- Looking for unistd.h - found
-- Looking for fcntl.h
-- Looking for fcntl.h - found
-- Configuring done (0.2s)
-- Generating done (0.0s)
CMake Warning:
Manually-specified variables were not used by the project:
CMAKE_EXPORT_NO_PACKAGE_REGISTRY
-- Build files have been written to: /home/hebasto/dev/bitcoin/depends/work/build/x86_64-pc-linux-gnu/freetype/2.11.1-c25288632f9/build
gmake: Leaving directory '/home/hebasto/dev/bitcoin/depends'
```
ACKs for top commit:
ryanofsky:
Code review ACK 2cf2b22ff1. Seems good to clean up this warning, and change is a simplification
Tree-SHA512: 95f1d2fa01ebb62a0841948d2242d6baca6b72b2293b43c5d809d113328046cf0f44017db276408d5cf906ed75746333570dcef679fa72c5e38e5ade6af281e6
fae482b4e6 ci: Use GCC consistently in i686 task (MarcoFalke)
Pull request description:
According to the comment removed in commit fae0295a79, clang was only used to avoid OOM. Using GCC today should be fine.
(Meta note: The task can maybe even be removed in a few years, see https://github.com/bitcoin/bitcoin/pull/35230#issue-4397029749)
If OOM is an issue, or even if there is no issue, maybe `DEBUG=1` should be removed, because the `ci/test/00_setup_env_native_alpine_musl.sh` config is already checking `DEBUG=1` with GCC.
ACKs for top commit:
fanquake:
ACK fae482b4e6
sedited:
ACK fae482b4e6
Tree-SHA512: 6c8567fcb47a2c65d41ca277bb201d5bed462c5cd1c57048be995279e62f900c10f649304b3c8bebab352acf2bceef3122b8b699e9fc9845fe4c90a4bfaf0afe
Added a validation in AddArg() preventing the same option name from
being registered across different categories, avoiding ambiguous option
resolution and make the distinction between global and command-specific
options explicit.
fa98d44951 ci: Rewrite broken wrap-valgrind.sh to .py (MarcoFalke)
faf7e38973 ci: refactor: Avoid warning: INSTALL_BCC_TRACING_TOOLS: unbound variable (MarcoFalke)
Pull request description:
The first commit fixes an error about `INSTALL_BCC_TRACING_TOOLS` being unbound.
The second commit rewrites the wrap-valgrind Bash script to Python to fix the shellcheck SC2044 violation.
Without this, the script would fail in CI when a path with spaces was used:
```
...
+ /ci_container_base/ci/test/wrap-valgrind.sh
Wrap /ci_container_base/ci/scratch_ ...
mv: cannot stat '/ci_container_base/ci/scratch_': No such file or directory
Wrap ₿🧪_/out/bin/bitcoin ...
mv: cannot stat '₿🧪_/out/bin/bitcoin': No such file or directory
/ci_container_base/ci/test/wrap-valgrind.sh: line 14: ₿🧪_/out/bin/bitcoin: No such file or directory
/ci_container_base/ci/test/wrap-valgrind.sh: line 15: ₿🧪_/out/bin/bitcoin: No such file or directory
chmod: cannot access '₿🧪_/out/bin/bitcoin': No such file or directory
ACKs for top commit:
fanquake:
ACK fa98d44951 - tested both on x86_64
hebasto:
ACK fa98d44951, tested on Ubuntu 26.04. I also verified the actual content of the created wrappers.
Tree-SHA512: fd9ccdd08a3af3aa9431eef29e17da0d785c7fe57fd0bfc9a6afdd979dc8860f4f9f0153ba3bb4b516cdec7ef0e071b846fddc1f2d28395cbe9356d2e9c55fb7
d3e40af259 index: shorten indexer thread names (Lőrinc)
d69c46292d util: zero-pad thread number suffixes (Lőrinc)
41e531c4ab util: shorten `ThreadPool` worker names (Lőrinc)
Pull request description:
**Problem:** Linux limits thread names set through [`PR_SET_NAME`](https://man7.org/linux/man-pages/man2/PR_SET_NAME.2const.html) to 15 visible bytes:
> The name can be up to 16 bytes long, including the terminating null byte.
Bitcoin Core prefixes system thread names with `b-`, leaving only 13 bytes for the thread-specific part.
This truncates longer indexer names in system tools, for example `b-coinstatsindex` and `b-txospenderindex`.
It also makes verbose worker suffixes like `b-http_pool_N` spend much of the available space; the current HTTP worker names fit, but the generic suffix leaves less room for longer pool names.
The same limit is documented in the existing thread-name helper:
8b49e2dd4e/src/util/threadnames.cpp (L25)
This was noticed during review of https://github.com/bitcoin/bitcoin/pull/31132#discussion_r3146688138
**Fix:** Shorten the OS-visible thread names while keeping public index identifiers unchanged.
`ThreadPool` workers now use a zero-padded dotted numeric suffix, so HTTP workers are named like `b-http.xx`.
`CCheckQueue` worker suffixes are zero-padded as well, so script-check workers are named like `b-scriptch.xx`.
Indexer sync threads now pass display and thread names separately at each `BaseIndex` call site.
The current indexer thread names use compact `idx`-suffixed names that fit within the Linux limit after the `b-` prefix:
```text
txindex -> txidx
basic block filter index -> blkfltbscidx
coinstatsindex -> coinstatsidx
txospenderindex -> txospenderidx
```
Indexer display names, `getindexinfo` keys, command-line options, and on-disk index paths are unchanged.
**Testing:** See https://godbolt.org/z/oWonrTKcj for a simple reproducer.
Alternatively, start `bitcoind` on Linux with the affected indexes enabled and read the kernel-visible thread names.
Before this change, the relevant names were truncated or used longer forms:
```text
b-txindex
b-basic block f
b-coinstatsinde
b-http_pool_15
b-txospenderind
```
After this change, the same check shows compact, untruncated names:
```text
b-txidx
b-blkfltbscidx
b-coinstatsidx
b-http.15
b-txospenderidx
```
ACKs for top commit:
maflcko:
re-ACK d3e40af259👳
sedited:
ACK d3e40af259
winterrdog:
re-ACK d3e40af259
hodlinator:
re-ACK d3e40af259
Tree-SHA512: 0b4c087661eb81e767fb2c2a1ce2dd54e6593888a7d30402e76c845a84dff5550e3ad72fee39b136f7f5214f051647c0c1f284e3265f8a614d1028f7b49d76da
`BaseIndex` currently uses the same name for logs, `getindexinfo`, prune locks, and the sync thread.
Linux truncates system thread names to 15 visible bytes after the `b-` prefix, so long indexer names are clipped in system tools.
Pass a separate thread name explicitly at each `BaseIndex` call site and shorten the OS-visible indexer names to `txidx`, `blkfltbscidx`, `coinstatsidx`, and `txospenderidx`.
Add an `Assume` to `ThreadRename()` so future OS-visible thread names keep fitting the same Linux limit.
The public index names, `getindexinfo` keys, command-line options, and on-disk paths stay unchanged.
Co-authored-by: winterrdog <winterrdog@users.noreply.github.com>
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
Co-authored-by: sedited <seb.kung@gmail.com>
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Thread names with numeric suffixes are easier to scan when the suffixes use a fixed width.
Format `ThreadPool` and script-check worker suffixes as two digits while keeping the compact dotted convention.
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
ThreadPool workers currently format their names as `name_pool_N`.
Linux truncates the system thread name to 15 visible bytes, so `b-http_pool_N` spends much of that space on the suffix.
Use a dotted numeric suffix so HTTP worker names become `b-http.N`, leaving more room for longer pool names.
58cc2a0453 test: make TestChain100Setup's m_clock timestamp more readable (Hao Xu)
Pull request description:
The timestamp 1598887952 is used as the initial value of the node's mocked wall-clock in TestChain100Setup. Comment its UTC date to make the magic number more readable.
ACKs for top commit:
maflcko:
lgtm ACK 58cc2a0453
Tree-SHA512: bd26977dcbdc6e2b448152dd0c2c71cee4ed73eb64ea32be3cac1e0a3f432f588b9f3261037be804d308e713163c2b47350c020327e21d7e9a9fe9a2827cdf45
726e196ef2 ci: inline runner selection (will)
Pull request description:
We are currently sometimes backlogged on waiting for runner selection.
Selecting Warp or GitHub-hosted runners directly from the repository context avoids serializing all CI jobs behind a metadata job. This keeps forks on public runners while allowing upstream jobs to schedule immediately on the intended runner labels.
This will make the changes needed for "a fork also wanting to use warp runners" slightly larger, but I think that number of people is zero.
ACKs for top commit:
m3dwards:
ACK 726e196ef2
Tree-SHA512: 14f3f22fbc53b3f65fce48bd08aef8db8d60a37409fd8b4b5b128073cf81408f7d3135c1e78fe3f93af8c88b3ee8c1b770e445b1ee7d5650ea91b329bc7ff777
The timestamp 1598887952 is used as the initial value of the node's
mocked wall-clock in TestChain100Setup. Comment its UTC date to make
the magic number more readable.
059edf1908 guix: Fix "Ignoring empty string" CMake warning for non-Linux hosts (Hennadii Stepanov)
2d86083fd4 guix: Drop redundant CMake `--verbose` options (Hennadii Stepanov)
Pull request description:
This PR introduces the following CMake-related improvements to [`contrib/guix/libexec/build.sh`](https://github.com/bitcoin/bitcoin/blob/master/contrib/guix/libexec/build.sh):
1. Drop redundant CMake `--verbose` options, as verbose output is already controlled by the [`VERBOSE`](https://cmake.org/cmake/help/latest/envvar/VERBOSE.html) environment variable, which has been exported since 85f4a4b082.
2. Fixes the "Ignoring empty string" CMake warning for non-Linux hosts.
ACKs for top commit:
fanquake:
ACK 059edf1908
Tree-SHA512: 8f32c29815e1417cba9cd61fe07704f01788c800cd77f571f0693de3d5d63cd8e0b6f6a77dbf68f5de90e5572385571ba82a1c24caac955b7a7617fe12399d13
d92a20b310 iwyu: Fix warning in `bench/pool.cpp` (Hennadii Stepanov)
Pull request description:
This PR resolves a silent merge conflict between recently merged #35414 and #35101 (47d68cd981).
ACKs for top commit:
fanquake:
ACK d92a20b310
Tree-SHA512: e496c56e3fba8c0030f4cfc79f5f678ac07aed87fb24738ed074ba171bfbcf4ea732f542b07dcd12247af5e6799d80f294bdc05ba8216a9465cf5192380bf818
Using shell parameter expansion ensures that on non-Linux hosts the
build script won't pass an empty string argument to CMake. This resolves
the "Ignoring empty string" warning.
6751a323c0 iwyu: Fix warnings in `src/bench` and treat them as error (Hennadii Stepanov)
a6ed29d6c2 bench, refactor: Use `std::string_view` for `BenchRunner` ctor parameter (Hennadii Stepanov)
Pull request description:
This PR addresses [this](https://github.com/bitcoin/bitcoin/pull/35011#discussion_r3323359707) comment:
> I had the impression I already fixed bench in https://github.com/bitcoin/bitcoin/pull/30716 two years ago, but I guess it isn't yet enforced.
>
> Could do that as a next step?
The first two commits act as prerequisites. See the commit messages for details.
The third commit additionally ensures that our drop-in header replacements are used instead of system headers:
- `util/check.h`:10dfdd4b9f/src/util/check.h (L11-L13)
- `util/time.h`:10dfdd4b9f/src/util/time.h (L9-L10)
ACKs for top commit:
maflcko:
re-ACK 6751a323c0📃
BrandonOdiwuor:
ACK 6751a323c0
Tree-SHA512: 159ee734a83dcba3c914682be4b119549e1e4269a43d34c52903e76056d537a2ae02c2f5f4e3adff1b4230082b8ed267c04164abc83b16f717b34fba6e03e359
9fae7e9886 test: doc: remove `--perf` profiling from functional test framework (Sebastian Falbesoner)
Pull request description:
This PR is an alternative to #35509. Rather than fixing, it removes the `--perf` option / `profile_with_perf` context manager features (both introduced in #14519) from the functional test framework. Given that no developer apparently even noticed that it's been broken for more than a year, we can conclude that it's largely unused and getting rid of it to reduce maintenance burden seems a reasonable choice. So far, neither in #35509 nor [on IRC](https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2026-06-12#1227556;) anyone has signaled strong interest to use or wanting to keep this feature.
The corresponding test documentation is removed as well, though the mentioning of `perf` profiling is still kept in the general developer docs -- only the reference to functional test framework integration is removed there.
ACKs for top commit:
l0rinc:
lightweight code review ACK 9fae7e9886
maflcko:
lgtm ACK 9fae7e9886 While this seems useful, no one using it is a good reason to remove it. If a user comes after this is merged, it should be trivial to revert.
Tree-SHA512: cf7bdce72aed877c7dfa52a230840e0729e74b00f0c40c6ceecf04957707f4699f123c2cde5fc1c4ee21f4b1b14319c74acc78cd8066eab2b9a4d6efb4d11539
9bfdde74b5 guix: add package.sh (fanquake)
Pull request description:
Split out packaging code, so that it can be re-used by build scripts. This is the second (mostly move-only) commit split out from #25573, before the changes that begin modifying the build.
ACKs for top commit:
hebasto:
ACK 9bfdde74b5.
willcl-ark:
ACK 9bfdde74b5
Tree-SHA512: da5a8b0f12054e3af100810a7963eb62f8db54c5f003a4e63405fd69b4118387d6392ebfa99468de70457cbaf6575b2733eb9f8d8114ec9dd625d7f16c066ba5
406c2348dd rpc: tighten setmocktime upper bound to UINT32_MAX (stringintech)
Pull request description:
The previous upper bound for `setmocktime` was `std::chrono::nanoseconds::max()` converted to seconds (~year 2262). This was too permissive in two ways:
1. Paths that add an offset to the mocked time can overflow `int64_t` (caught by UBSan). For example, `ContextualCheckBlockHeader` adds a constant to the current time for its future-time check. (see [comment](https://github.com/bitcoin/bitcoin/pull/35496#issuecomment-4678552371))
2. Paths that assign the mocked time to a `uint32_t` field silently truncate it (caught by the integer sanitizer). For example, `miner.cpp` assigns `NodeClock::now()` directly to `pblock->nTime`. (see [comment](https://github.com/bitcoin/bitcoin/pull/35496#issuecomment-4679331674))
`UINT32_MAX` is the natural ceiling since block header `nTime` is `uint32_t`, making mocked values beyond it meaningless for anything consensus-related.
ACKs for top commit:
sedited:
ACK 406c2348dd
winterrdog:
ACK 406c2348dd
Tree-SHA512: 4dc5f5125ed48a11a62661446870dbd2b3b29c30b04094c3f2b4293a2a73ed61ce785e15666b3549da7c4a08055a5d27b0a5598061a432eeda0e69495c37b426
77772e7a30 undo "ui: Compile boost:signals2 only once" (MarcoFalke)
fa45783d55 mv btcsignals.h to src/util (MarcoFalke)
fa4903db8a refactor: Make scoped_connection ctor explicit (MarcoFalke)
fa1bc1fe51 test: Check btcsignals determinism in thread_safety test case (MarcoFalke)
fa86e5dba9 refactor: Properly return from ThreadSafeQuestion signal (MarcoFalke)
fa4badc0fd refactor: Make ThreadSafeMessageBox signal void (MarcoFalke)
faad9d6434 refactor: Mark btcsignals operator [[nodiscard]] (MarcoFalke)
Pull request description:
Previously, the ThreadSafeQuestion signal was using `btcsignals::optional_last_value<bool>`.
However, this only worked by accident:
* Calling `CClientUIInterface::ThreadSafeQuestion` did not return an
`std::optional<bool>`, but `value_or(false)`. This makes it hard for
callers to differentiate between `nullopt` and `false`.
* The return value was further 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.
So fix that by properly returning from the signal.
Also, remove the then-unused `btcsignals::optional_last_value<T>` combiner.
Also, other follow-ups from https://github.com/bitcoin/bitcoin/pull/34495#issuecomment-4212629857
ACKs for top commit:
theuni:
ACK 77772e7a30
hebasto:
ACK 77772e7a30, I have reviewed the code and it looks OK. Tested on Fedora 44.
sedited:
Nice, ACK 77772e7a30
Tree-SHA512: 0d9c83a3f34d98bf7a2d3b53ea122d5f5313566d77936d7c564cb613b8f5bbd0edc3955853d71dd555cf8a20c08b3d6fc30907351361720f4b0c5f8dbe8d6965
359680b74d net: move cs_main up in FetchBlock to fix rpc assert crash (Eugene Siegel)
Pull request description:
A benign, racy assert can fail when calling FetchBlock and the peer is being cleaned up.
1. FetchBlock runs in a http worker thread. It acquires a PeerRef, locks cs_main, then may later call BlockRequested which asserts that CNodeState exists for the peer.
2. FinalizeNode may run in either the bitcoind or b-net threads. It locks cs_main, fetches a PeerRef from RemovePeer, fetches a CNodeState, and later removes it from m_node_states.
Because of the lock placement in FetchBlock, the http worker thread in 1) can acquire a valid PeerRef and block while the b-net thread in 2) is cleaning up the peer in FinalizeNode. When the worker thread later acquires cs_main, it may crash in BlockRequested since no CNodeState exists. Fix this by acquiring the lock earlier in FetchBlock.
I tested the assert can be hit and the fix works by adding sleeps. Was introduced in https://github.com/bitcoin/bitcoin/pull/25514 which moved the lock down.
ACKs for top commit:
maflcko:
lgtm ACK 359680b74d
dergoegge:
utACK 359680b74d
sedited:
ACK 359680b74d
Tree-SHA512: dd29db28bc95c781b32b1cb6e782190fc8abd28bba36a5149fb35c86eaf56448e58a5c68a1c858a50348dd2c940e423942c67340a52e934818711932f538d708
This feature was broken for more than a year and no developer apparently
even noticed, so one can conclude that it is largely unused; it seems
thus reasonable to remove it to reduce maintenance burden.
d186c390f4 Revert "build: exclude mptest target from compile commands" (fanquake)
Pull request description:
This reverts commit 4731049ba4 (#35418), which broke the build with `-DBUILD_TESTS=OFF`:
```bash
-- Performing Test HAVE_PTHREAD_GETTHREADID_NP - Failed
CMake Error at cmake/libmultiprocess.cmake:37 (set_target_properties):
set_target_properties Can not find target to add properties to: mptest
Call Stack (most recent call first):
src/CMakeLists.txt:24 (add_libmultiprocess)
```
Reported by `afiore` on IRC.
ACKs for top commit:
winterrdog:
tested ACK d186c390f4
hebasto:
ACK d186c390f4, tested on Fedora 44.
Tree-SHA512: ec1ede0340da9d1338643980e7f2e4646f0aed2e64f339f8b41a447cbbfb4da8158c1cbcb22f2d0cc383c2dd945075602d02b07a47b33b44cc6215f3292c1d46
The previous bound (~year 2262) was too permissive: paths that add an offset to the mocked time (e.g. the future-time check in ContextualCheckBlockHeader) can overflow int64_t (caught by UBSan), and paths that assign it to a uint32_t field (e.g. pblock->nTime in miner.cpp) silently truncate it (caught by the integer sanitizer). UINT32_MAX is the natural ceiling since block header nTime is uint32_t, and mocking beyond it is meaningless for anything consensus-related.
Add setmocktime bound checks to the existing _test_y2106 case in rpc_blockchain.py, and remove the negative bound check from rpc_uptime.py.
4731049ba4 build: exclude mptest target from compile commands (Sanjana2906)
Pull request description:
Fixes#35361
The `mptest` target includes generated `.capnp.h` files that don't exist
at CMake configure time. When IWYU reads `compile_commands.json`, it sees
these non-existent files and fails.
This PR excludes the `mptest` target from the compilation database by
setting `EXPORT_COMPILE_COMMANDS OFF`, following the same pattern already
applied to the `mpcalculator`, `mpprinter`, and `mpexample` targets in the
same file.
This change belongs in `cmake/libmultiprocess.cmake` (the integration layer).
ACKs for top commit:
ryanofsky:
Code review ACK 4731049ba4. Seems ok to exclude mptest from tidy & iwyu checks as this PR does. It also seems ok to go further and exclude all of libmultiprocess from tidy & iwyu checks as hebasto suggested https://github.com/bitcoin/bitcoin/pull/35418#pullrequestreview-4428862711
Tree-SHA512: 6f198d7f52251e7bc877805da37d958b02b769a0fce6ee4331a0255c34f8f670e508b794fab29b8629f07700bc81b23c638e92019c687b82c4399bc656660cc8
472b950b7f qa: Use custom assert_greater_than() over naked assert (Hodlinator)
f42226d526 qa: Silence socket.timeout exception when substituting it for a JSONRPCException (Hodlinator)
659671ac3d qa: Avoid cleanup when exception is raised (Hodlinator)
Pull request description:
Clean up some cases in which we would trigger multiple tracebacks, which makes it unclear what issue is occuring (https://github.com/bitcoin/bitcoin/issues/31894#issuecomment-4616130031).
CI log of this occurring: https://github.com/bitcoin/bitcoin/actions/runs/26907239657/job/79375388058?pr=35179
<details><summary>Relevant log excerpt</summary>
```
test 2026-06-03T19:28:28.119804Z TestFramework.node0 (DEBUG): TestNode.generate() dispatches `generate` call to `generatetoaddress`
test 2026-06-03T19:28:28.120483Z TestFramework (ERROR): Unexpected exception:
Traceback (most recent call last):
File "/home/runner/work/bitcoin/bitcoin/test/functional/test_framework/authproxy.py", line 165, in _get_response
http_response = self.__conn.getresponse()
^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib/python3.12/http/client.py", line 1448, in getresponse
response.begin()
File "/usr/lib/python3.12/http/client.py", line 336, in begin
version, status, reason = self._read_status()
^^^^^^^^^^^^^^^^^^^
File "/usr/lib/python3.12/http/client.py", line 297, in _read_status
line = str(self.fp.readline(_MAXLINE + 1), "iso-8859-1")
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib/python3.12/socket.py", line 707, in readinto
return self._sock.recv_into(b)
^^^^^^^^^^^^^^^^^^^^^^^
TimeoutError: timed out
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/home/runner/work/bitcoin/bitcoin/ci_build/test/functional/p2p_orphan_handling.py", line 54, in wrapper
func(self)
File "/home/runner/work/bitcoin/bitcoin/ci_build/test/functional/p2p_orphan_handling.py", line 633, in test_maximal_package_protected
testres = node.testmempoolaccept([large_orphan.serialize().hex()])
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/runner/work/bitcoin/bitcoin/test/functional/test_framework/coverage.py", line 50, in __call__
return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/runner/work/bitcoin/bitcoin/test/functional/test_framework/authproxy.py", line 128, in __call__
response, status = self._request('POST', self.__url.path, postdata.encode('utf-8'))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/runner/work/bitcoin/bitcoin/test/functional/test_framework/authproxy.py", line 102, in _request
return self._get_response()
^^^^^^^^^^^^^^^^^^^^
File "/home/runner/work/bitcoin/bitcoin/test/functional/test_framework/authproxy.py", line 167, in _get_response
raise JSONRPCException({
test_framework.util.JSONRPCException: 'testmempoolaccept' RPC took longer than 30.000000 seconds. Consider using larger timeout for calls that take longer to return. (-344) [http_status=None]
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/home/runner/work/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 143, in main
self.run_test()
File "/home/runner/work/bitcoin/bitcoin/ci_build/test/functional/p2p_orphan_handling.py", line 837, in run_test
self.test_maximal_package_protected()
File "/home/runner/work/bitcoin/bitcoin/ci_build/test/functional/p2p_orphan_handling.py", line 57, in wrapper
self.generate(self.nodes[0], 1)
File "/home/runner/work/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 666, in generate
blocks = generator.generate(*args, called_by_framework=True, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/runner/work/bitcoin/bitcoin/test/functional/test_framework/test_node.py", line 445, in generate
return self.generatetoaddress(nblocks=nblocks, address=self.get_deterministic_priv_key().address, maxtries=maxtries, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/runner/work/bitcoin/bitcoin/test/functional/test_framework/test_node.py", line 453, in generatetoaddress
return self.__getattr__('generatetoaddress')(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/runner/work/bitcoin/bitcoin/test/functional/test_framework/coverage.py", line 50, in __call__
return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/runner/work/bitcoin/bitcoin/test/functional/test_framework/authproxy.py", line 128, in __call__
response, status = self._request('POST', self.__url.path, postdata.encode('utf-8'))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/runner/work/bitcoin/bitcoin/test/functional/test_framework/authproxy.py", line 101, in _request
self.__conn.request(method, path, postdata, headers)
File "/usr/lib/python3.12/http/client.py", line 1356, in request
self._send_request(method, url, body, headers, encode_chunked)
File "/usr/lib/python3.12/http/client.py", line 1367, in _send_request
self.putrequest(method, url, **skips)
File "/usr/lib/python3.12/http/client.py", line 1193, in putrequest
raise CannotSendRequest(self.__state)
http.client.CannotSendRequest: Request-sent
test 2026-06-03T19:28:28.124908Z TestFramework (DEBUG): Closing down network thread
```
</details>
Fix in authproxy.py can be verified through applying the below diff with/without PR changes and running `./build/test/functional/p2p_orphan_handling.py`:
```diff
--- a/test/functional/test_framework/authproxy.py
+++ b/test/functional/test_framework/authproxy.py
@@ -162,6 +162,8 @@ class AuthServiceProxy():
def _get_response(self):
req_start_time = time.time()
try:
+ if AuthServiceProxy.__id_count > 55:
+ raise socket.timeout()
http_response = self.__conn.getresponse()
except socket.timeout:
raise JSONRPCException({
```
ACKs for top commit:
maflcko:
review ACK 472b950b7f🔼
polespinasa:
reACK 472b950b7f
Tree-SHA512: ec1ede0340da9d1338643980e7f2e4646f0aed2e64f339f8b41a447cbbfb4da8158c1cbcb22f2d0cc383c2dd945075602d02b07a47b33b44cc6215f3292c1d46
ed11dd6a5f test: add coverage for importdescriptors when manually interrupting a wallet rescan (Pol Espinasa)
d90d7f0a55 test: add coverage for importdescriptors errors when using assumeutxo (Pol Espinasa)
ad388bf254 test: add coverage for importdescriptors while wallet is rescanning (Pol Espinasa)
84d07e471c test: add coverage for importdescriptor with an encrypted wallet (Pol Espinasa)
Pull request description:
The current tests for `importdescriptors` RPC do not check for cases where RPC errors should be thrown.
This PR adds coverage for _importing a descriptor when the wallet is encrypted_ , for _importing a descriptor while the wallet is rescanning_ and _importing a descriptor while using assumeutxo_
For context, this lack of coverage was found while implementing #34861 when a reviewer found that this was being silently broken in the PR.
I am not sure if the "rescanning test" approach is the optimal solution, I am open to suggestions.
ACKs for top commit:
achow101:
ACK ed11dd6a5f
w0xlt:
ACK ed11dd6a5f
Tree-SHA512: 18e7111314ff003d39538d53899a3e2261027f5f965945f259eec4b56ece5c22706faf2891694c47575f3a5089ca02c80ea0bd05c453c4e072335d4a45ab8edd
The test only checked that the single atomic value is greater than 3000.
However, by splitting the atomic into two, one can do one exact check,
and also increase the lower bound on the inexact check.
Also, test disconnect races for every second step, instead of only once
at the end (likely when only one thread is running anyway).
Both changes make the test stricter and may catch non-determinism issues
that are not detected by sanitizers alone.
The test added in this commit should also pass when applied on top of
commit 63c68e2a3f, which is still using
the boost implementation.
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).
3be1115ade ci: Alpine 3.24 (fanquake)
Pull request description:
Switch to [Alpine 3.24](https://www.alpinelinux.org/posts/Alpine-3.24.0-released.html) in the Alpine CI job.
ACKs for top commit:
maflcko:
lgtm ACK 3be1115ade
sedited:
ACK 3be1115ade
Tree-SHA512: 735a193a3511da611ac3399a66917cf69d5f895c39e50667c1293f1e449795ae85bb5a9686ce28928f73547369b036d091e1cf7b523854652a10ec7cf529d1d3
