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
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
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
54de023a7c guix: add setup.sh (fanquake)
Pull request description:
This is the first change in #25573, which splits out the setup & tarball generation code from `build.sh`, so that it can be re-used, from multiple (future) build scripts.
ACKs for top commit:
willcl-ark:
ACK 54de023a7c
hebasto:
ACK 54de023a7c.
Tree-SHA512: 9a7f2fe322d281b9867414511af5243f4dd659ea42637f4eb8cc0c8629c94dab842669bb7c503f9fa67cab3fac65561364f07b5c0fda8e6d8c24e7bf161025ef
This is easier to reason about, because it will automatically take care
of properly setting INITIAL_MOCK_TIME in the ctor. Also, it allows to
drop the ElapseTime and replace it with a call to operator+=()
The context is easier to reason about: E.g.,
* in TestBasicMining it allows to drop manual SetMockTime(0) calls,
* in connections_desirable_service_flags it allows to drop manual calls
to SetMockTime(GetTime<std::chrono::seconds>() + _n_) and replace them
by operator+=(_n_)
* in wallet_tests it clarifies that the mocktime does not persist
outside the AddTx function
Currently, all test cases using TestChain100Setup or a derived class
like BuildChainTestingSetup are using mocktime by default due to the
SetMockTime call in the TestChain100Setup ctor.
This is confusing, because test cases using mocktime explicitly seem to
imply that before they set the mocktime, real time was used.
E.g. index_reorg_crash claimed in a comment to "Enable mock time".
Fix this issue by adding a FakeNodeClock m_clock field to
TestChain100Setup. Then, use the m_clock instead of explicit calls to
SetMockTime or to a (now) shadowing local FakeNodeClock variable.
35a814a045 test: Limit clocks to one active instance (MarcoFalke)
55e402ffef scripted-diff: Rename NodeClockContext to FakeNodeClock (seduless)
1e9546fcf4 test: Use NodeClockContext in more call sites (seduless)
758fea59a8 test: Drop ++ from NodeClockContext default constructor (seduless)
7c2ec3949a test: Enter mocktime before peer creation in block_relay_only_eviction (seduless)
Pull request description:
Follow-up to #34858
Updates remaining `SetMockTime` call sites that are clean, mechanical swaps fitting the spirit of the original PR (see: https://github.com/bitcoin/bitcoin/pull/34858#pullrequestreview-4031647119 and https://github.com/bitcoin/bitcoin/pull/34858#issuecomment-4221757881). Further updates to `SetMockTime` are more complex and deserve separate, isolated PRs.
The default constructor for `NodeClockContext` increments to the next tick, which is a defensive measure to prevent time going backwards on construction. This has caused some confusion (see thread: https://github.com/bitcoin/bitcoin/pull/34858#discussion_r3057648646) and can be safely removed after updating the only test where this is load-bearing (b3c9bd7f2df230525c8e339394a315a2c500055d) (see: https://github.com/bitcoin/bitcoin/pull/34858#discussion_r3091085328). The removal also tightens the `addrman_tests/addrman_evictionworks` test to sit exactly on the `ADDRMAN_REPLACEMENT` boundary (4h), catching mutations such as:
```diff
diff --git a/src/addrman.cpp b/src/addrman.cpp
index d3dae59ae7..d0929c62cb 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -920,3 +920,3 @@ void AddrManImpl::ResolveCollisions_()
// Has successfully connected in last X hours
- if (current_time - info_old.m_last_success < ADDRMAN_REPLACEMENT) {
+ if (current_time - info_old.m_last_success <= ADDRMAN_REPLACEMENT) {
erase_collision = true;
```
The last follow-up item is updating `NodeClockContext` to `FakeNodeClock` to make it clear it is intended for testing (motivated by https://github.com/bitcoin/bitcoin/pull/34858#pullrequestreview-4082110904 and supported in https://github.com/bitcoin/bitcoin/pull/34858#issuecomment-4214352770).
ACKs for top commit:
maflcko:
re-ACK 35a814a045🛒
sedited:
ACK 35a814a045
Tree-SHA512: ade776e288a4b7bbc4c8855c14d61381b5b20329fe1e72fee87f773e47a9519975d58c277fbacda37dd73c0c1d4ce358c92dcdc4ca049d58cb3453ddf751b45b
fa2afba28b p2p: Release m_peer_mutex early in InitiateTxBroadcastToAll (MarcoFalke)
Pull request description:
The `InitiateTxBroadcastToAll` method holds the `m_peer_mutex` while updating the bloom filters for all peers. This is perfectly fine, because updating the bloom filters is fast. Though, from a style-perspective, the lock does not need to be held for the whole function. Also, holding the lock longer, may confuse Tsan into a lock-order inversion false-positive (ref: https://github.com/bitcoin/bitcoin/issues/19303#issuecomment-1514926359).
So "fix" both issues in this style-refactor.
ACKs for top commit:
xyzconstant:
Code review ACK fa2afba28b
shuv-amp:
ACK fa2afba28b
danielabrozzoni:
Code Review ACK fa2afba28b
sedited:
ACK fa2afba28b
Tree-SHA512: c47849a4c3cc11c74b61fec3425db8ec7f78db4ca43d7bf3145ce640f7b0872701c09495f0dfe77109d09d5716d920ad3d7308483fe41564c30867b3e80432e7
fba713a28c scripted-diff: Rename UNIQUE_NAME to BITCOIN_UNIQUE_NAME (Hennadii Stepanov)
Pull request description:
https://github.com/bitcoin/bitcoin/pull/34454#issuecomment-3822800049:
> ... it is annoying that we keep running into the same bug over and over again (IIRC it happened in the past at least once for Bitcoin Core). Surely this is going to happen again in the future.
And here we go again.
---
The `nb30.h` Windows header [defines](b536c4fdb0/mingw-w64-headers/include/nb30.h (L78)) `UNIQUE_NAME` as a macro.
This introduces a fragile dependency on header inclusion order: if Windows headers happen to be included before `UNIQUE_NAME` is used, the preprocessor expands it into a numeric literal, causing syntax errors.
Rename the macro to `BITCOIN_UNIQUE_NAME` to remove this fragility and avoid the collision entirely.
---
Noticed while doing a Guix build of the [QML repo](https://github.com/bitcoin-core/gui-qml) for Windows.
Recent similar PRs: https://github.com/bitcoin/bitcoin/pull/34454 and https://github.com/bitcoin/bitcoin/pull/34868.
ACKs for top commit:
maflcko:
lgtm ACK fba713a28c
sedited:
ACK fba713a28c
w0xlt:
ACK fba713a28c
Tree-SHA512: 7a63b99a754e797eb8fa5d6a598606150f47ae1130d1d26067c509830e6575f0378ce63fe0ca35c69dce9a394451a34ddadd8b3d5f6f9a7e4c529108af546fb6
5b65e31270 test: remove two unnecessary nodes from the test (rkrux)
Pull request description:
A discussion in the review of #35443 PR brought this test to my attention.
The test needs multiple wallets that can be created on a single node, multiple nodes are not required.
As there is a cost associated with setting-up and tearing-down nodes, this patch helps in reducing the test time as well.
ACKs for top commit:
ekzyis:
ACK 5b65e31270
polespinasa:
lgtm ACK 5b65e31270
sedited:
ACK 5b65e31270
Tree-SHA512: f6b4a96b9beee968ef5438fd9db582a48834ff36ba27c19dd7012902528fa713424212530e34cc16b58c19c023f1accd2b89fe846ef2cc36677c24e160c5b817
d0b76c7f3e rpc+bitcoin-tx: Specify correct type for ParseFixedPoint() (Hodlinator)
43ca54ca00 refactor(test): Make CAmount arg explicit for BuildCreditingTransaction() (Hodlinator)
b5e91e946c wallet: Remove CoinsResult::Clear() (Hodlinator)
Pull request description:
The *knapsack_solver_test* in *coinselector_tests.cpp* was accumulating satoshi amounts beyond 21M BTC. This was uncovered while experimenting with adding checks to `CAmount`. Fix that by fully resetting the `CoinsResult` object accumulating those amounts, inspired by https://github.com/bitcoin/bitcoin/issues/35449#issuecomment-4613968627.
Also, while we're at it, add 2 commits which correct some `int64_t`/`CAmount` confusion.
Fixes https://github.com/bitcoin/bitcoin/issues/35449
ACKs for top commit:
sedited:
ACK d0b76c7f3e
furszy:
utACK d0b76c7f3e
brunoerg:
code review ACK d0b76c7f3e
Tree-SHA512: 6d989ded6f6327dc657f437dc256d4adf42a34a1252621421ee38d7851c6cdc97a462f033a4728e3aa7d5514deee4db6e83646105633f9cf7ed6e7e90406b67d
0bfc5e4fff add release notes (Pol Espinasa)
fdc9fc1df2 test: check getprivatebroadcast and abortprivatebroadcast throw if the node is running without -privatebroadcast set (Pol Espinasa)
7b821ef9b7 rpc: getprivatebroadcastinfo and abortprivatebroadcast throw if -privatebroadcast is disabled (Pol Espinasa)
Pull request description:
Makes `getprivatebroadcast` and `abortprivatebroadcast` throw if `-privatebroadcast=0`.
This is motivated by: https://github.com/sparrowwallet/sparrow/issues/1989
Knowing if `privatebroadcast` is set can be useful for some external software like Sparrow to avoid call `getprivatebroadcastinfo` each time to see if broadcast was done through that.
ACKs for top commit:
stickies-v:
ACK 0bfc5e4fff
sedited:
ACK 0bfc5e4fff
rkrux:
code review ACK 0bfc5e4fff
andrewtoth:
ACK 0bfc5e4fff
Tree-SHA512: 3bdb3909e93fc3835d801e1efc2bbec673a75a1ff089debd59e8970a0ff2b44d4e00b7ac26f10c972dcb50bf042521921370e1ec57885d67cd8459b3831da898
The `nb30.h` Windows header defines `UNIQUE_NAME` as a macro.
This introduces a fragile dependency on header inclusion order: if
Windows headers happen to be included before `UNIQUE_NAME` is used, the
preprocessor expands it into a numeric literal, causing syntax errors.
Rename the macro to `BITCOIN_UNIQUE_NAME` to remove this fragility and
avoid the collision entirely.
-BEGIN VERIFY SCRIPT-
sed -i 's/\<UNIQUE_NAME\>/BITCOIN_UNIQUE_NAME/g' $(git grep -l 'UNIQUE_NAME' ./src/)
-END VERIFY SCRIPT-
ec6cf49b91 blockstorage: Remove cs_LastBlockFile recursive mutex (sedited)
Pull request description:
The `cs_LastBlockFile` mutex is redundant: all critical sections are already covered by cs_main. This is demonstrated in this patch by replacing all instances of locking `cs_LastBlockFile` with pairs of `AssertLockHeld(::cs_main)` and `EXCLUSIVE_LOCKS_REQUIRED(::cs_main)` annotations. No additional `::cs_main` LOCK(...)s are introduced (besides for test-only code).
It is also not clear for which sections `cs_LastBlockFile` is responsible for. It is annotated for `m_blockfile_cursors`, but sporadically and inconsistently also covers `m_blockfile_info` (e.g. in `LoadBlockIndexDB`).
Since it has no semantic meaning, and seems confusing to developers, remove it.
An alternative to this patch would be expanding the scope of what `cs_LastBlockFile` covers and turning it into a non-recursive mutex. I prepared such a patch some time ago, but found it unsatisfactory. It was not clear to me if the lock was now covering too much or too little, and its purpose remained unclear. If this patch is accepted, I would expect the project to eventually implement a separate, narrowly-scoped block storage lock to allow for a more parallelizable block processing routine.
ACKs for top commit:
stickies-v:
re-ACK ec6cf49b91
janb84:
re- ACK ec6cf49b91
pablomartin4btc:
ACK ec6cf49b91
Tree-SHA512: e5942bc87300b0db9a0b91d5fe26dab455049e6cef7c96bb12b28141fa04711d46c6af105c0e1a83a9f261edde2c8b8b43ecf577a27d54b4610d784676a85627
The cs_LastBlockFile mutex is redundant: all critical sections are
already covered by cs_main. This is demonstrated in this patch by
replacing all instances of locking cs_LastBlockFile with pairs of
`AssertLockHeld(::cs_main)` and `EXCLUSIVE_LOCKS_REQUIRED(::cs_main)`
annotations. No additional `::cs_main` LOCK(...)s are introduced.
It is also not clear for which sections `cs_LastBlockFile` is
responsible for. It is annotated for `m_blockfile_cursors`, but
sporadically and inconsistently also covers `m_blockfile_info`.
Since it has no semantic meaning, and seems confusing to developers,
remove it.
SteadyClockContext and FakeNodeClock assume they are the only active
instance. Overlapping them in the same scope would silently clobber
each other.
Add a CRTP base class, LimitOne, that asserts at construction if
another instance already exists.
The previous name did not indicate the type was intended for
testing. Renaming to FakeNodeClock makes this explicit and
allows call sites to drop the ctx suffix on the variable name.
Suggested in #34858 review feedback.
-BEGIN VERIFY SCRIPT-
s() { git grep -l "$1" -- src | xargs sed -i "s/$1/$2/g"; }
s '\<NodeClockContext\>' 'FakeNodeClock'
s '\<clock_ctx\>' 'clock'
-END VERIFY SCRIPT-
