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.
fab52281f7 refactor: Drop unused includes after iwyu CI bump (MarcoFalke)
fa4774d032 ci: Bump APT_LLVM_V-based task configs to Ubuntu 26.04 (MarcoFalke)
fa1414a36a ci: Debian Trixie -> Ubuntu 26.04 (MarcoFalke)
Pull request description:
This is for the upcoming 32.x, because I presume users and devs are more likely using a later distro. This comes with tool bumps, such as:
* GCC 14 -> 15 (https://packages.debian.org/trixie/g++ -> https://packages.ubuntu.com/resolute/g++)
* Clang 19 -> 21
* Cmake 3.31 -> 4.2
* Valgrind 3.24 -> 3.26
ACKs for top commit:
l0rinc:
code review ACK fab52281f7
hebasto:
re-ACK fab52281f7.
Tree-SHA512: 9d5be2f5b15cf7904c50687ce5e8cceeb2f740c7d5180190d6a10e751998ce2c2156098f89352eac49f24c8cd9ab55b78321e310240ac829dcbe48b576b6240c
1ce9e26239 fuzz: improve dbwrapper_concurrent_reads performance (Andrew Toth)
Pull request description:
The recently merged fuzz harness targeting concurrent reads suffers from poor performance and memory leaks (https://github.com/bitcoin/bitcoin/pull/34866#issuecomment-4614323925).
Fix this by
- using a global thread pool instead of a local one per iteration
- reduce thread count to 8 from 16
- use a std::map oracle to check results inline instead of reading from the db to get a baseline and storing results
ACKs for top commit:
marcofleon:
reACK 1ce9e26239
l0rinc:
ACK 1ce9e26239
sedited:
ACK 1ce9e26239
Tree-SHA512: 2e532caf246f389105e4a9b487496386d1fe9add7b27fba9ecbbf51a432ef493765ad7095288dd7e0a896860ff150d89ecb6afb8baf311a4af94d8e01b77dba5
526aae3768 fuzz: test non-max descriptor satisfaction weight (woltx)
Pull request description:
The descriptor fuzz target is intended to exercise descriptor satisfaction-size estimation for solvable descriptors.
It currently calls `MaxSatisfactionWeight(true)` twice, so the `false` branch is never exercised.
This PR changes `max_sat_nonmaxsig` to call `MaxSatisfactionWeight(false)`, so fuzzing covers both branches.
ACKs for top commit:
brunoerg:
reACK 526aae3768
sedited:
ACK 526aae3768
Tree-SHA512: 029750d76c1d50f5c6a008b826a0a2dc187feb420be96401d2e15747b44901341d32ac75e86a5e10585919d419c607800d8e117a1cbce50b1db40121d3610f9c
fa03852e9c test: Use SteadyClockContext in pcp_tests (MarcoFalke)
fa3716c439 test: Use FakeNodeClock in more places (MarcoFalke)
fae9623c8d test: Add FakeNodeClock m_clock to TestChain100Setup (MarcoFalke)
Pull request description:
This switches the remaining cases in the unit tests from `SetMockTime` to `FakeNodeClock` for clarity, as explained in the commit messages.
ACKs for top commit:
frankomosh:
crACK fa03852e9c. This PR continues the FakeNodeClock migration from #35114.
sedited:
ACK fa03852e9c
w0xlt:
ACK fa03852e9c
Tree-SHA512: 4d4f1ff170ce8cfa606a6dc0dc47ff8b89e2db0d0188daeabbbaa422df00143fd99426905adcff1fd152a00ebf8a9004e312cbc77a1d078e57895d5f1334a3b2
f6bdbcf79d lint: Grep for `AUTO` test suites in file names (rustaceanrob)
Pull request description:
Tests without a fixture did not have their file names linted because the grep matches on `BOOST_FIXTURE`. Updates to match `BOOST_FIXTURE` or `BOOST_TEST`.
ACKs for top commit:
l0rinc:
ACK f6bdbcf79d
achow101:
ACK f6bdbcf79d
hebasto:
ACK f6bdbcf79d.
Tree-SHA512: dd1763b6ac90fa87b7e0d2faa56d1c7beedb1e2d37d16367c60ebcadd155f5955113fff7cf5c0ce5eaa9e63aeeb67ffff2c8e081f7c23978cb072207f072f2ef
5a2e359213 clarify blockfilterindex cache allocation rationale (Sebastian van Staa)
d06dabf26b node: allocate index caches proportional to usage patterns (Sebastian van Staa)
Pull request description:
The current cache allocation for optional indexes (txindex, txospenderindex, blockfilterindex) uses a sequential total_cache / 8 approach where each index gets 1/8 of the remaining budget after the previous index has been allocated. This means the order in which indexes appear in the code silently determines how much cache each one gets.
|Index|Current share of total|
|---|---|
|txindex|~12%|
|txospenderindex|~11%|
|blockfilterindex|~10%|
This is unintuitive, undocumented, and probably doesn't reflect actual usage patterns. This PR replaces the sequential 1/8 allocation with explicit percentages based on how the indexes are typically used. The current values are an educated guess, and subject to further benchmark and research of typical client usage patterns.
|Index|Allocation|Rationale|
|---|---|---|
|txindex |10% |Serves getrawtransaction RPCs with mostly unique lookups across the entire blockchain: low cache reuse|
|txospenderindex|5%|Serves gettxspendingprevout RPCs with very specific outpoint queries: likely the least repetitive access pattern|
|blockfilterindex|5%|Serves BIP 157 light clients that repeatedly query the same recent blocks: highest cache benefit|
UPDATE: blockfilterindex allocation changed from 15% to 5% in the course of the discussion
This is a continuation of the related discussion: https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2809088034 and https://github.com/bitcoin/bitcoin/pull/31483.
Further feedback and input is very much appreciated.
ACKs for top commit:
fjahr:
ACK 5a2e359213
rustaceanrob:
ACK 5a2e359213
achow101:
ACK 5a2e359213
sedited:
ACK 5a2e359213
Tree-SHA512: 69be2b0c274b975da58aef2513c3042be8a4c8acf0a86af86b962d4ebfd8cf90bcb1d9251d53995652b4825d0d1da24aabe92cdada9148c627690f8ad2ad8a29
Also assert that the availability of the satisfaction weight estimate
does not depend on the signature-size assumption, and that assuming
non-max-size signatures never increases the estimate.
3f44f9aef7 test: Add coverage for m_blocks_unlinked invariant in LoadBlockIndex (marcofleon)
0e4b0bacec validation: Don't add pruned blocks to m_blocks_unlinked on startup (marcofleon)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/35050
The `m_blocks_unlinked` map keeps track of blocks that have transactions but whose parent (or any ancestor) does not. This happens when a block is received before its parent, or during a reorg, when `FindMostWorkChain()` encounters a block whose ancestors were pruned.
The bug this PR addresses is a rare interaction of these two cases, which happens on startup when `BlockManager::LoadBlockIndex()` rebuilds `m_blocks_unlinked`. The check there only considers whether a block has transactions, and pruned blocks keep `nTx > 0` but clear `BLOCK_HAVE_DATA`. So if there's a pruned block on a stale fork whose parent has no transactions, that block is added to `m_blocks_unlinked` without having data on disk. This violates an [assertion](ad3f73862b/src/validation.cpp (L5352)) in `CheckBlockIndex()`.
Get rid of this unintended case by gating on `BLOCK_HAVE_DATA` before adding to `m_blocks_unlinked`.
ACKs for top commit:
achow101:
ACK 3f44f9aef7
sedited:
Re-ACK 3f44f9aef7
stratospher:
ACK 3f44f9a. nice!
Tree-SHA512: 275d0f8588524c01c4e701c8635973cd4a086d31c10d252a498c1ef668bdb3895ba1cae265dbe88f8983ca7ddbe32247824753c7c1f49e59c8bce0df377b784c
2189a6f5f2 p2p: Saturate LocalServiceInfo::nScore updates at INT_MAX (codeabysss)
Pull request description:
The overflow for signed arithmetic yields undefined behavior.
This changes prevents undefined behavior in local address scoring by saturating `nScore` updates at `INT_MAX` in both `SeenLocal()` and `AddLocal()` update paths.
Fixes: #24049.
ACKs for top commit:
Crypt-iQ:
ACK 2189a6f5f2 pending CI
achow101:
ACK 2189a6f5f2
sedited:
ACK 2189a6f5f2
Tree-SHA512: b861e58ec9d6e18b17768f5cbee31ee825717e1a7216c332eb6fcbe63a7ac24e213ba638aea6f03cb710d9c2d8fe736cc626f11011ed66c3938acf6c38b0ef2a
21a1380c13 key: cleanse ChainCode on destruction (Thomas)
b3a3f88346 crypto: cleanse HMAC stack buffers after use (Thomas)
Pull request description:
`CHMAC_SHA256` and `CHMAC_SHA512` leave two stack buffers populated on return: `rkey[]` holds `K' ⊕ ipad` after the constructor, and `temp[]` holds the inner-hash output after `Finalize()`.
When the HMAC is keyed with sensitive material (chain code in `BIP32Hash()` in `hash.cpp` for BIP32 child key derivation; PRK in HKDF-Expand in `hkdf_sha256_32.cpp`, used for BIP324 transport keying), `rkey` is one constant XOR from that key, and `temp` is a one-way digest covering it.
This PR cleanses both buffers with `memory_cleanse()`, matching the convention already used in `chacha20.cpp` and `chacha20poly1305.cpp`. No observable change for callers.
Update: Cleansing the HMAC primitive's internal buffers still leaves a caller's `ChainCode` value populated in memory after use. The second commit promotes `ChainCode` from `typedef uint256` to a `base_blob<256>` subclass with a `memory_cleanse()` destructor, so chain codes in `CExtKey`, `CExtPubKey`, and local variables are cleansed on scope exit. `MUSIG_CHAINCODE` is retyped from `constexpr uint256` to `const ChainCode` to match its BIP328 semantic role; this also removes the GCC-14 consteval lambda workaround.
ACKs for top commit:
davidgumberg:
crACK 21a1380c13
optout21:
ACK 21a1380c13
achow101:
ACK 21a1380c13
winterrdog:
ACK 21a1380c13
Tree-SHA512: 022c8372da3e2c9c269ef55b695d8415241acf64be04692f30da0e682dd1d05178f95601a3bd208573fd0630656b3dedcf6de34a2a3cf794515c0268e710af75
19b32a2e18 fuzz: reset the mockable steady clock between iterations (Hao Xu)
Pull request description:
Fix the issue mentioned by https://github.com/bitcoin/bitcoin/issues/29018#issuecomment-4422112607
And this is my investigation on it: https://github.com/bitcoin/bitcoin/issues/29018#issuecomment-4639472489
`CheckGlobalsImpl`'s constructor runs at the start of every fuzz iteration and already resets the global RNG flags and the mockable `NodeClock` (`SetMockTime(0s)`), but it never reset the mockable steady clock. A value written to `g_mock_steady_time` by one input therefore leaks into the next iteration.
The most common source is `FuzzedSock`'s constructor, which calls `SetMockTime(INITIAL_MOCK_TIME)` (through `ElapseTime(0s)`) and never clears it: once any input constructs a `FuzzedSock`, the steady clock stays mocked for every subsequent iteration in the same process. This is one of the global-state leaks tracked
in #29018.
### Fix
Reset `MockableSteadyClock` symmetrically with `NodeClock`:
```diff
g_used_system_time = false;
SetMockTime(0s);
+MockableSteadyClock::ClearMockTime();
```
Besides removing the leak, this puts the steady clock under the same discipline as the system clock: a target that reads `MockableSteadyClock::now()` without first mocking it (via `FuzzedSock`, `SteadyClockContext`, …) is now caught by the existing `g_used_system_time` check at the end of the iteration, instead of
silently reusing a value left over from a previous input.
Clearing in `~FuzzedSock()` would be wrong: several `FuzzedSock`s can be alive simultaneously (e.g. `process_messages` adds 1–3 peers), so clearing in one destructor would corrupt the mock observed by the others. Resetting at the iteration boundary keeps it decoupled from socket lifetimes.
### Testing
Verified with the global-state-detector approach from #29018 (snapshotting/diffing the writable globals around each iteration):
- **Before:** a single empty input to `process_message` reports `g_mock_steady_time` changing `00 → 01` (`0` → `INITIAL_MOCK_TIME`).
- **After:** that report is gone; the only remaining diffs are the benign one-time initialization of `ConsumeTime`'s function-local statics.
`p2p_headers_presync` (uses `SteadyClockContext`) and `pcp_request_port_map` (uses `FuzzedSock`) still run to `succeeded` without aborting, confirming existing steady-clock readers are unaffected.
This leak is invisible to coverage-based checks such as `deterministic-fuzz-coverage`, because `g_mock_steady_time` is only consumed through coarse time comparisons (e.g. the 250 ms presync rate-limiter): a changed value doesn't change the executed branches, so only a memory-diffing detector can see it.
ACKs for top commit:
maflcko:
lgtm ACK 19b32a2e18
marcofleon:
Nice catch, ACK 19b32a2e18
Tree-SHA512: b875795addb2914eae489adc703438483f8e464b9a210bd5d76189f13266dae5843c8749590d59e78bf171f19aa7cee21ca678cd311843d8a88cbe9831f20b6a
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.
The lock can be replaced with a net-specific lock when the remaining
CNodeState fields are moved to Peer.
