This avoids hardcoding xprvs and xpubs in the test.
Also use key's parent fingerprint and derivation path while creating descriptors
so that all of it is self-documenting and not dependent on hardcoded values,
making them easy to read and update.
Also use descsum_create function more to avoid hardcoding the descriptor
checksum.
Using these classes allows the developers to dynamically create
xprvs and xpubs so that they don't need to hardcode such long keys
in the tests that most of the times clutter the tests and make them
difficult to update.
Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>
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
b83a999b14 btcsignals: delete broken scoped_connection move assignment (Thomas)
Pull request description:
The defaulted move-assignment operator for `scoped_connection` overwrites `m_conn` without first calling `disconnect()`. Since disconnection is signaled via the liveness flag (which is never cleared) the old callback remains registered in the signal and keeps firing, violating the RAII contract:
```cpp
int val{0};
btcsignals::scoped_connection sc0 = sig.connect(IncrementCallback);
btcsignals::scoped_connection sc1 = sig.connect(SquareCallback);
sc0 = std::move(sc1);
val = 3; sig(val);
// Expected: 9 (only SquareCallback)
// Actual: 16 (both callbacks fire, old connection leaked)
```
Move assignment is unused in the codebase, so following the review discussion this deletes the broken operator instead of fixing it. A correct implementation can be added if a use case arises.
Earlier versions of this PR fixed the operator and added a default constructor to enable the member-variable assignment pattern; that was dropped in favor of removing the unused operation.
ACKs for top commit:
maflcko:
lgtm ACK b83a999b14
sedited:
ACK b83a999b14
Tree-SHA512: 794347e9cb868d50957ea298f7df6eac5b9f55b9d35ab09e41be269923c45e0709194431dea66b7977c74f802150ba53cb2d12d35937f4966ec302bffb9c95f8
The defaulted move assignment overwrites m_conn without disconnecting
it first, so the previous callback stays registered with the signal and
keeps firing, violating the RAII contract:
btcsignals::scoped_connection sc0 = sig.connect(IncrementCallback);
btcsignals::scoped_connection sc1 = sig.connect(SquareCallback);
sc0 = std::move(sc1);
val = 3; sig(val); // both callbacks fire: 16 instead of 9
Move assignment is unused in the codebase, so delete it rather than
fixing it. It can be implemented properly if a use case arises.
