On Windows, the `winerror.h` header defines `WAIT_TIMEOUT` as a macro.
This introduces a fragile dependency on header inclusion order: if
Windows headers happen to be included before using `WAIT_TIMEOUT`, the
preprocessor expands it into a numeric literal, causing syntax errors.
Rename the variable to `TEST_WAIT_TIMEOUT` to remove this fragility and
avoid the collision entirely.
-BEGIN VERIFY SCRIPT-
sed -i 's/\<WAIT_TIMEOUT\>/TEST_WAIT_TIMEOUT/g' $(git grep -l 'WAIT_TIMEOUT' ./src/)
-END VERIFY SCRIPT-
d6f680b427 validation: Move block into BlockDisconnected signal (sedited)
4d02d2b316 validation: Move block into BlockConnected signal (sedited)
8b0fb64c02 validation: Move validation signal events to task runner (sedited)
Pull request description:
This enforces behaviour that is currently already implicit: The destructor for blocks runs mostly in the [scheduler thread](https://bitcoin-dev-tools.github.io/benchcoin/results/pr-176/20472174834/mainnet-default-instrumented-head-flamegraph.svg?x=2762391536960&y=684). The change should make it a bit clearer what the ownership semantics for these validation signals are.
`BlockConnected` already takes a reference to a block that is emplaced in `connected_blocks`. Once `connected_blocks` is iterated through, it is not reused. Similarly `BlockDisconnected` currently takes a reference to a block that is discarded after the call to it. Note that this does not give the guarantee that blocks' lifetimes are extended by other means once they are connected. For example after IBD, the block's lifetime is extended in net_processing's `m_most_recent_block` and `ActivateBestChain` itself takes a copy of the block's shared pointer, meaning its caller may delay de-allocation.
ACKs for top commit:
maflcko:
re-review ACK d6f680b427🔌
stickies-v:
re-ACK d6f680b427
frankomosh:
Re-ACK d6f680b427
Tree-SHA512: 9209a7d23e7af0d76fa70dff958b1329f38ef29ccc49b5a32bcf9f349d59cc2bf70464ebdb130d26077c0ff9362ce9211472231d375ff1c9c89c0ec3020eac80
779e7825db fuzz: wallet: add target for `MigrateToDescriptor` (brunoerg)
Pull request description:
This PR adds fuzz coverage for the scriptpubkeyman migration (`MigrateToDescriptor`). Note that it's a test for the migration of the scriptpubkey manager, not for the whole migration process as tried in #29694, because:
1) The wallet migration deals with DBs which is expensive for fuzzing (was getting around 3 exec/s);
2) Mocking would require lots of refactors.
This target loads keys, HDChain (even inactive ones), watch only and might add tons of different scripts, then calls `MigrateToDescriptor`. It does not play with encrypted stuff because it would make the target super slow. Also, after the migration there are some assertions that would work as a regression test for https://github.com/bitcoin/bitcoin/pull/31452, for example.
ACKs for top commit:
frankomosh:
Code Review ACK 779e7825db
marcofleon:
reACK 779e7825db
Tree-SHA512: 08ef5166602c21658765bc063c5421e81055d094d346c4e2a28215209c6b7768b99a424f3ba47cf718dc8d827a588da22394ba23402a40a71a976d80d65e6c2e
79467e3ec7 threading: never require logging from sync.h (Cory Fields)
Pull request description:
This is an updated version of #34793 after stickies-v [pointed out that the approach there was broken](https://github.com/bitcoin/bitcoin/pull/34793#discussion_r2916369196).
sync.h is low-level and should not require any other subsystems.
Move the lone remaining logging call to the .cpp. Any cost incurred by an additional function call should be trivial compared to the logging itself.
ACKs for top commit:
stickies-v:
re-ACK 79467e3ec7 , thanks for doing the modernization right away 👍
sedited:
ACK 79467e3ec7
Tree-SHA512: 53e4b19cbf6ec81ce6aee06092a07e56877e2247a2614a148ba25c276dc5cf00d68445f9cd43dbd71e4481da74b788e3e20d3634484760d037f94d44bf13ea9c
f3bf63ec4f kernel: acquire coinstats cursor and block info atomically (w0xlt)
5e77072fa6 rpc: fix race condition in gettxoutsetinfo (w0xlt)
Pull request description:
Fixes#34263
A `CHECK_NONFATAL` assertion failure could occur in `gettxoutsetinfo` when a new block was connected between capturing `pindex` and validating it in `GetUTXOStats()`.
The fix removes the early `pindex` capture since `ComputeUTXOStats()` independently fetches the current best block under lock. The response now uses `stats.hashBlock` and `stats.nHeight` (the actual computed values) instead of the potentially stale `pindex`.
ACKs for top commit:
sedited:
ACK f3bf63ec4f
fjahr:
utACK f3bf63ec4f
rkrux:
Concept ACK f3bf63ec4f for removal of the race condition.
Tree-SHA512: c2d5cd5a1b4b4f1c22023c03970fea400a0b78005fa3d09d6567255615ab461c01b584d8a158651ee08769ec86fc4a1200f640ad58fdaa4879c335d90c891f6a
498b6eb6b5 cmake: Migrate away from deprecated SQLite3 target (Daniel Pfeifer)
Pull request description:
CMake version 4.3 deprecated the imported target `Sqlite::Sqlite3`.
Use the preferred name `Sqlite3::Sqlite3` instead and provide an alias for older versions of CMake.
Also define the same alias when using vcpkg.
ACKs for top commit:
hebasto:
ACK 498b6eb6b5, tested on Fedora 43 using CMake 3.22.6 and 4.3.0.
Tree-SHA512: 8ec0e9673ea39c4383d2995d804498675f9bcd1196ec586f338cb3fa63aa632f160dd179bda3c2d70d932d8b1ac821c15d8247e7302465420937c9227f1bf20a
faf71d6cb4 test: [refactor] Use verbosity=0 named arg (MarcoFalke)
99996f6c06 test: Fix intermittent issue in feature_assumeutxo.py (MarcoFalke)
Pull request description:
The test has many issues:
* It fails intermittently, due to the use of `-stopatheight` (https://github.com/bitcoin/bitcoin/issues/33635)
* Using `-stopatheight` is expensive, because it requires two restarts, making the test slow
Fix all issues by using `dumb_sync_blocks` and avoid the two restarts.
Fixes https://github.com/bitcoin/bitcoin/issues/33635
Diff to test:
```diff
diff --git a/src/util/tokenpipe.cpp b/src/util/tokenpipe.cpp
index c982fa6fc4..1dc12ea1d5 100644
--- a/src/util/tokenpipe.cpp
+++ b/src/util/tokenpipe.cpp
@@ -4,2 +4,3 @@
#include <util/tokenpipe.h>
+#include <util/time.h>
@@ -60,2 +61,3 @@ int TokenPipeEnd::TokenRead()
ssize_t result = read(m_fd, &token, 1);
+ UninterruptibleSleep(999ms);
if (result < 0) {
```
On master: Test fails
On this pull: Test passes
ACKs for top commit:
fjahr:
Code review ACK faf71d6cb4
sedited:
ACK faf71d6cb4
Tree-SHA512: 8f7705d2b3f17881134d6e696207fe6710d7c4766f1b74edf5a40b4a6eb5e0f4b12be1adfabe56934d27abc46e789437526bcdd26e4f8172f41a11bc6bed8605
fadf901fd4 rpc: Run type check on decodepsbt result (MarcoFalke)
fa4d5891b9 refactor: Introduce TxDocOptions (MarcoFalke)
fa8250e961 refactor: Add and use RPCResultOptions (MarcoFalke)
Pull request description:
For RPCResults, the type may be ELISION, which is confusing and brittle:
* The elision should only affect the help output, not the type.
* The type should be the real type, so that type checks can be run on it.
Fix this issue by introducing a new print_elision option and using it in `decodepsbt`.
This change will ensure that `RPCResult::MatchesType` is properly run.
Can be tested by introducing a bug:
```diff
diff --git a/src/core_io.cpp b/src/core_io.cpp
index 7492e9ca50..4927b70c8e 100644
--- a/src/core_io.cpp
+++ b/src/core_io.cpp
@@ -436,2 +436,3 @@ void TxToUniv(const CTransaction& tx, const uint256& block_hash, UniValue& entry
entry.pushKV("version", tx.version);
+ entry.pushKV("bug", "error!");
entry.pushKV("size", tx.ComputeTotalSize());
```
And then running (in a debug build) `decodepsbt cHNidP8BAAoCAAAAAAAAAAAAAA==`
Before, on master: passes
Now, on this pull: Properly detects the bug
ACKs for top commit:
nervana21:
tACK fadf901fd4
achow101:
ACK fadf901fd4
willcl-ark:
ACK fadf901fd4
satsfy:
re-ACK fadf901fd4
seduless:
re-ACK fadf901fd4
Tree-SHA512: 4fb000dba9fe39bcd2bac72e2d88553f54134a250c985b4ca7150b483d7185009047d8fe4ba75c522bfc26706de20c913b8905e7552ab0c41802ae744cb92038
fa050da980 test: Move event loop creation to network thread (MarcoFalke)
fa9168ffcd test: Use asyncio.SelectorEventLoop() over deprecated asyncio.WindowsSelectorEventLoopPolicy() (MarcoFalke)
Pull request description:
It is deprecated according to https://docs.python.org/3.14/library/asyncio-policy.html#asyncio.WindowsSelectorEventLoopPolicy The replacement exists since python 3.7
Also, move the event loop creation to happen in the thread that runs the loop.
ACKs for top commit:
l0rinc:
ACK fa050da980
sedited:
ACK fa050da980
Tree-SHA512: dce25596a04e8f133630d84c03a770185a81b1bcd0aae975f0dbdd579d22b7b79a9b1172abf46c61d0845d3f5ab4a6414fa0f17c59f0ea0f6fa9bdcac085a2a7
faea12ecd9 test: Fixup docs for NodeClockContext and SteadyClockContext (MarcoFalke)
eeeeb2a0b9 fuzz: Use NodeClockContext (MarcoFalke)
fa4fae6227 test: Add NodeClockContext (MarcoFalke)
Pull request description:
Iterating over fuzz inputs will usually be done in the same process. As the mocktime is global, it can theoretically leak from one fuzz input run into the next run, making it less deterministic.
Fix this issue, by adding and using a context manager to handle the mocktime and reset it before the end.
This refactor should not change any behavior.
ACKs for top commit:
seduless:
re-ACK faea12ecd9
dergoegge:
utACK faea12ecd9
brunoerg:
code review ACK faea12ecd9
Tree-SHA512: e222c4e4217a504d058b30f1e975dfdfff019363c82385bd62f368b16fb029c46a5d1b43cd773dbdd9efcd7f968d46dbe2c75812971696b1b879b8f081fc6b1b
32debfa1ed fuzz: set whitelist permissions on connman target (Bruno Garcia)
Pull request description:
I noticed that `AddWhitelistPermissionFlags` was always being called with an empty `ranges` (whitelist permissions). This function is called when connecting to a node (`ConnectNode`), and if the connection is a manual one, it uses the `vWhitelistedRangeOutgoing` to populate the permissions. However, since we were not setting any whitelist permission on the target, this vector is always empty. This PR improves this target by populating both `vWhitelistedRangeIncoming` and `vWhitelistedRangeOutgoing`.
ACKs for top commit:
Crypt-iQ:
utACK 32debfa1ed
maflcko:
lgtm ACK 32debfa1ed
frankomosh:
crACK 32debfa1ed
Tree-SHA512: 400ce780b9ae41d849ccad04258da4ad92d9bf780bfdeb9bb9c1684722bcc02ae45f13081b809f9e16b28078bd4efb928c6c7e1c3da638307b6c11b193d6dc04
This makes existing behaviour of the block's destructor triggering on
the scheduler thread more explicit by moving it to the thread. The
scheduler thread doing so is useful, since it does not block the thread
doing validation while releasing a block's memory.
DisconnectTip already creates and destroys the block itself, so moving
it into the validation signals is well scoped.
This makes existing behaviour of the block's destructor triggering on
the scheduler thread more explicit by moving it to the thread. The
scheduler thread doing so is useful, since it does not block the thread
doing validation while releasing a block's memory.
Previously, both the caller and the queued event lambda held copies of
the shared_ptr. The block would typically be freed on the scheduler
thread - but only because it went out of scope before the queued event
on the scheduler thread ran. If the scheduler ran first, the block would
instead be freed on the validation thread.
Now, ownership is transferred at each step when invoking the
BlockConnected signal: connected_blocks yields via std::move,
BlockConnected takes by value, and the event lambda move-captures the
shared_ptr. Though it is possible that this only decrements the block's
reference count, blocks are also read from disk in `ConnectTip`, which
now explicitly results in their memory being released on the scheduler
thread.
Currently arguments passed through the validation interface are copied
three times. Once on capture in the event, again when the event itself
is copied into the task runner lambda, and when the various arguments
used by the logging statement are copied into the lambda.
This change avoids the variables captured by the event being copied
again. Next to avoiding needless copies, this is done in preparation of
the following two commits, which seek to clarify the ownership semantics
of the blocks passed through the validation interface.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
CMake version 4.3 deprecated the imported target `Sqlite::Sqlite3`.
Use the preferred name `Sqlite3::Sqlite3` instead and provide an
alias for older versions of CMake. Also define the same alias when
using vcpkg.
685a44c601 fuzz: set fSuccessfullyConnected in connman harness (frankomosh)
Pull request description:
The connman fuzz harness never sets `fSuccessfullyConnected=true` on nodes added through `AddTestNode()`. `NodeFullyConnected()` gates `ForEachNode()` on that flag, making its callback unreachable in the current harness.
Set `fSuccessfullyConnected=true` before `AddTestNode()` to simulate a node that has completed the version handshake.
ACKs for top commit:
maflcko:
lgtm ACK 685a44c601
sedited:
ACK 685a44c601
marcofleon:
ACK 685a44c601
Tree-SHA512: 2c696b8674cb465f468642b5fd37a467bc34dcbf61dc901d784fd2fe0dd13ced5cd6bd9873bcce0f8e60e25d450052e9a562ece08abeb2ab6472e584dba65c40
551875360c ci: Use arch-appropriate binaries in lint install (will)
Pull request description:
In testing #34547 it has been observed that the lint container does not run on aarch64-linux without `qemu binfmt` (or similar).
This is because some tools are hardcoded to download x64 linux binaries. This has meant the linter works fine on:
- x64 linux
- aarch64 MacOS (via Rosetta)
- platforms using qemu
But does not work on e.g. aarch64-linux _without qemu_.
`shellcheck`` offer many platforms: https://github.com/koalaman/shellcheck/releases/tag/v0.11.0 and `mlc` offers are least x64 and aarch64 linux https://github.com/becheran/mlc/releases/tag/v1.2.0.
Try to download the correct binary for the platform using `uname` detection. This should see the linter work on native aarch64 + amd64, whilst maintaining current (emulated) compatibility.
ACKs for top commit:
maflcko:
lgtm ACK 551875360c
Tree-SHA512: 636cccbed3ffff995549c666b0cad1aa9790291a73a0f2212f0374c8878bd916c04e4ecb17fac1611fc2d72d363cececeeaa997af918ad4225355231376ff7b0
Replace the hardcoded x86_64 binary name with $(uname --machine) so the
correct binary is downloaded when building the lint container, where at
all possible.
bde35d61f9 depends: capnp 1.4.0 (fanquake)
Pull request description:
Update capnp in depends to [`1.4.0`](https://github.com/capnproto/capnproto/releases/tag/v1.4.0).
It contains a number of bugfixes, and fixes for 2 CVEs, of which I think only `Fix benign(?) buffer overrun in async readMessage()` is relevant to us, and it seems to be considered benign:
> This is technically undefined behavior (a buffer overrun), but we suspect that it is benign with all known memory allocators. In C++, a zero-sized allocation (made with `operator new(0)`, as is the case here) is required to return a unique pointer, different from any other such allocation. Because of this, all common memory allocators round up a zero-byte allocation to a word-sized allocation (32-bit or 64-bit, depending on the architecture). The overrun written to this allocation was exactly one pointer in size, so always fits into the actual allocation space.
> Nevertheless, the code is in fact relying on undefined behavior, and it is theoretically possible that some memory allocator implements zero-sized allocations in a way that would make this overrun dangerous.
See https://github.com/capnproto/capnproto/compare/release-1.3.0...release-1.4.0 for all changes since 1.3.0.
ACKs for top commit:
sedited:
ACK bde35d61f9
janb84:
ACK bde35d61f9
hebasto:
ACK bde35d61f9.
Tree-SHA512: 33a6c12684b9a6046a38c3b9dd1a5730db352eae07b5dbfe7244228fde3d1627d039c0e0ba7d35fe0968f91a0f476c239fa8f2e356a37b8ac975ac268d271bc2
6202acd284 test: addrman: successive failures in the last week for IsTerrible (brunoerg)
f611d3bdaf refactor: addrman: move consts to .h (brunoerg)
Pull request description:
This PR adds test coverage for the case that an address is considered terrible if we had N successive failures in the last week.
It kills the following mutant (https://corecheck.dev/mutation/src/addrman.cpp#L88):
```diff
diff --git a/src/addrman.cpp b/src/addrman.cpp
index e3981e6a40..f8045491c1 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -65,7 +65,7 @@ bool AddrInfo::IsTerrible(NodeSeconds now) const
}
if (now - m_last_success > ADDRMAN_MIN_FAIL && nAttempts >= ADDRMAN_MAX_FAILURES) { // N successive failures in the last week
- return true;
+ return false;
}
return false;
```
ACKs for top commit:
frankomosh:
re-ACK 6202acd284
naiyoma:
tACK 6202acd284
danielabrozzoni:
tACK 6202acd284
sedited:
ACK 6202acd284
Tree-SHA512: b4736ef91b75ba4c060dc18a2b796aee94d0d8be5ca58b9b873156248cd0dc6910595595e0f56d75bffff4f94c035319ac8428f7d79f07fe685bdba27a188829
sync.h is low-level and should not require any other subsystems.
Move the lone remaining logging call to the .cpp. Any cost incurred by an
additional function call should be trivial compared to the logging itself.
Without this, NodeFullyConnected() filters out every fuzz-constructed node, making ForEachNode's callback unreachable (0/1.13M branch hits from my end).
For RPCResults, the type may be ELISION, which is confusing and brittle:
* The elision should only affect the help output, not the type.
* The type should be the real type, so that type checks can be run on
it.
Fix this issue by introducing a new print_elision option and using it
in decodepsbt.
This change will ensure that RPCResult::MatchesType is properly run.
Also, this clarifies the RPC output minimally:
```diff
--- a/decodepsbt
+++ b/decodepsbt
@@ -35,7 +35,7 @@ Result:
"inputs" : [ (json array)
{ (json object)
"non_witness_utxo" : { (json object, optional) Decoded network transaction for non-witness UTXOs
- ...
+ ... The layout is the same as the output of decoderawtransaction.
},
"witness_utxo" : { (json object, optional) Transaction output for witness UTXOs
"amount" : n, (numeric) The value in BTC
```
fa90b21430 test: Remove unused feature_segwit.py functions (MarcoFalke)
fa6b05c96f test: Remove unused CUSTOM_._COUNT (MarcoFalke)
fa7bac94d8 test: Remove unused wait_for_addr, firstAddrnServices, on_addr (MarcoFalke)
fa388a3585 test: Remove unused self.p2p_conn_index = 1 (MarcoFalke)
fa803710e2 test: Remove unused AddressType (MarcoFalke)
fab5072ce1 ci: Remove vulture (MarcoFalke)
Pull request description:
Currently, `vulture` is run with `--min-confidence=100`, which reduces its checks to dead code after control statements, which is nice, but not really a common nor severe issue. See the discussion in https://github.com/bitcoin/bitcoin/issues/34810#issuecomment-4045927137 and commit 5c005363a8, which had to remove dead code manually.
Reducing the confidence has shown to be too brittle/tedious in the past, so remove the tool for now from CI.
Of course, removing this from CI does not prevent anyone from running it locally and removing dead code.
Fixes https://github.com/bitcoin/bitcoin/issues/34810
ACKs for top commit:
fanquake:
ACK fa90b21430
willcl-ark:
ACK fa90b21430
Tree-SHA512: 6a5998470dae3a17baec29b70b02333f4cd9b81bc4c6a05b56085ff1ba527ed7bdeccd17b09d9ad785ae03c97982ee1f3147e4df3bd537c66b02e9a44d0e5f15
111864ac30 qa: Avoid duplicating output in case the diff is the same (Hodlinator)
c2e28d455a ci: Enable `wallet_multiwallet.py` in "Windows, test cross-built" job (Hodlinator)
850a80c199 qa: Disable parts of the test when running under Windows or root (Hodlinator)
fb803e3c79 qa: Test scanning errors individually (Hodlinator)
ed43ce57cc qa: Check for platform-independent part of error message (Hodlinator)
64a098a9b6 refactor(qa): Break apart ginormous run_test() (Hodlinator)
bb1aff7ed7 move-only(qa): Move wallet creation check down to others (Hodlinator)
d1a4ddb58e refactor(qa): Lift out functions to outer scopes (Hodlinator)
c811e47367 scripted-diff: self.nodes[0] => node (Hodlinator)
73cf858911 refactor(qa): Remove unused option (Hodlinator)
Pull request description:
Makes the functional test compatible with *Linux->Windows cross-built executables*.
Main parts:
* Commit "qa: Check for platform-independent part of error message" switches to match on platform-independent part of error message.
* Commit "qa: Test scanning errors individually" disentangles code causing the same error message substring, based on #31410.
* Commit "qa: Disable parts of the test when running under Windows or root" enables the test to be run on Windows, based in part on https://github.com/bitcoin/bitcoin/pull/31410#issuecomment-3554721014.
Also:
* Removes unused option in wallet_multiwallet.py.
* Breaks apart wallet_multiwallet.py's `run_test()` into smaller test functions.
* Improves `assert_equal()` output for dicts.
Fixes#31409.
ACKs for top commit:
achow101:
ACK 111864ac30
janb84:
re ACK 111864ac30
w0xlt:
reACK 111864ac30
Tree-SHA512: 4e3ff92588ac9f2611fc963ce62097b6c0dd4d4eb8da7952c72619c7b554ff3cae5163fe1886d4d9bbd7af1acca5b846411e7f5b46f9bddb08719b61108efbba
af0da2fce2 crypto: Use `secure_allocator` for `AES256CBC*::iv` (David Gumberg)
d53852be31 crypto: Use `secure_allocator` for `AES256_ctx` (David Gumberg)
8c6fedaa81 build: `lockedpool.cpp` kernel -> crypto (David Gumberg)
51ac1abf6f bench: Add wallet encryption benchmark (David Gumberg)
9a15872516 wallet: Make encryption derivation clock mockable (David Gumberg)
ae5485fa0d refactor: Generalize derivation target calculation (David Gumberg)
Pull request description:
Fixes#31744
Reuse `secure_allocator` for `AES256_ctx` in the aes 256 encrypters and decrypters and the `iv` of `AES256CBC` encrypters and decrypters. These classes are relevant to `CCrypter`, used for encrypting wallets, and my understanding is that if an attacker knows some or all of the contents of these data structures (`AES256_ctx` & `iv`) they might be able to decrypt a user's wallet.
Presently the `secure_allocator` tries to protect sensitive data with `mlock()` on POSIX systems and `VirtualLock()` on Windows to prevent memory being paged to disk, and by zero'ing out memory contents on deallocation with `memory_cleanse()` which is similar to `OPENSSL_cleanse()` by scaring compilers away from optimizing `memset` calls on non-Windows systems, and using `SecureZeroMemory()` on Windows.
ACKs for top commit:
achow101:
ACK af0da2fce2
furszy:
utACK af0da2fce2
theStack:
re-ACK af0da2fce2
Tree-SHA512: 49067934fd2f2b285fc7b1a7c853fd2d4475431b3a811ae511f61074dc71a99a0826c3ab40ab4a5dfc84b2b9914a90c920d2484b38ac19502e3bd6170ad27622
d62f46eed4 doc: Add `INSTALL.md` to Linux release tarballs (Hennadii Stepanov)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/32097:
> Better I think would be to add instructions for the most popular desktop distros.
ACKs for top commit:
willcl-ark:
utACK d62f46eed4
achow101:
ACK d62f46eed4
janb84:
re ACK d62f46eed4
w0xlt:
ACK d62f46eed4
sedited:
ACK d62f46eed4
Tree-SHA512: eab6904b6acc63eaf0e13112f52d856ee630ce4110fb368f4f5f170655def91706bb7c7f70c123186a51461ea3d48ef026a4e026d665675f1fd76f2d0bbca68c
This should fix https://github.com/bitcoin/bitcoin/issues/34367
I am not familiar with Windows sockets thread-safety, but creating the
event loop on the main thread, and running it in the network thread
could lead to a fast abort in Python on Windows (without any stderr):
```
77/276 - wallet_txn_clone.py failed, Duration: 1 s
stdout:
2025-12-10T08:04:27.500134Z TestFramework (INFO): PRNG seed is: 4018092284830106117
stderr:
Combine the logs and print the last 99999999 lines ...
============
Combined log for D:\a\_temp/test_runner_₿_🏃_20251210_075632/wallet_txn_clone_196:
============
test 2025-12-10T08:04:27.500134Z TestFramework (INFO): PRNG seed is: 4018092284830106117
test 2025-12-10T08:04:27.500433Z TestFramework (DEBUG): Setting up network thread
```
Also, I couldn't find any docs that require the loop must be created on
the thread that runs them:
* https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.new_event_loop
* https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.loop.run_forever
However, the patch seems trivial to review, harmless, and easy to
revert, so it may be a good try to fix the intermittent Windows Python
crash.
d03e3be246 ci: check macos bundle structure and codesigning (fanquake)
66d80d57b4 macdeploy: use plugins dir to find plugins (fanquake)
ab137cbfe2 macdeploy: subprocess out to zip rather than shutil.make_archive (fanquake)
Pull request description:
Fix bundle format.
Add a CI check that codesigning works.
Fixes#34744.
ACKs for top commit:
Sjors:
tACK d03e3be246
hebasto:
ACK d03e3be246, tested on macOS Tahoe 26.3.1.
sedited:
ACK d03e3be246
Tree-SHA512: 5a7db896952edf338ff4fe8c934f1e1c992642850a99d5fafbb1212c6979601b3b72b6f3af880fb6f6ac8759cd4102e9f01792abb05410ceaf36cbffaec48e47
20fb7618b0 args: make most ArgsManager members private (w0xlt)
22b40f34f3 args: replace cs_args RecursiveMutex with Mutex (w0xlt)
3a16ec8582 test: scope cs_args locks to avoid recursive locking (w0xlt)
70b51fef7a args: eliminate all recursive locking of cs_args (w0xlt)
7d61e03c70 args: extract lock-requiring internal helpers (w0xlt)
Pull request description:
Part of #19303.
Replace `ArgsManager::cs_args` from `RecursiveMutex` to `Mutex`.
The conversion follows the pattern established in prior `RecursiveMutex` removals (e.g. `CAddrMan` in #19238, `CBlockPolicyEstimator` in #22014): extract private lock-held helpers with trailing underscore naming (`GetSetting_()`, `GetArgFlags_()`, `GetPathArg_()`), then replace recursive calls in methods that already hold `cs_args` with those helpers.
ACKs for top commit:
l0rinc:
ACK 20fb7618b0
rkrux:
Concept ACK 20fb7618b0
sedited:
Re-ACK 20fb7618b0
hebasto:
ACK 20fb7618b0, only rebased and suggested changes since my recent [review](https://github.com/bitcoin/bitcoin/pull/34745#pullrequestreview-3916715262).
Tree-SHA512: 7ab4278737f00deaa3f3da75e08469f91e95aa31e916820d02af737c754751ae4f73c1c8650f120eeff142a134f9209cf581499696a7b88ffc83d296515e40f2
9085dee476 qa: Add lock order annotation for TxMempool::cs (David Gumberg)
Pull request description:
I added this to see if static analysis would catch any lock order inversions while investigating #34731, it did not catch any but it is a worthwhile defense against statically-analyzable inversions of `cs_main->TxMempool::cs`.
ACKs for top commit:
sedited:
tested ACK 9085dee476
hebasto:
ACK 9085dee476.
w0xlt:
ACK 9085dee476
Tree-SHA512: 498605362e7b197b7643a3f9ffd04589bc9b245aca1d245d289efa145906248c1a72f9fbf4835a9378aabe170b747dc0cc033105915a2bf7a9989de368710c02
c2732146d1 doc: fix process name typo in multiprocess.md (ViniciusCestarii)
Pull request description:
In this [flow](https://github.com/bitcoin/bitcoin/blob/master/doc/design/multiprocess.md#example-use-cases-and-flows), the bitcoin-wallet process acts as the client initiating the RPC request, while the bitcoin-node process acts as the server receiving it. So at "The `getBlockHash` method of the generated `Chain` server subclass in `bitcoin-wallet` receives a Cap’n Proto request object with the `height` parameter, and calls the `getBlockHash` method on its local `Chain` object with the provided `height`." it should say bitcoin-node not bitcoin-wallet.
This PR fix this typo.
ACKs for top commit:
ryanofsky:
Code review ACK c2732146d1. Nice catch!
Tree-SHA512: 7227dc96e556ad36f1f39cc1033d80c92e8c6a47cf61ab68401122b1ef8075068224494e4ffb7094eca8dc325294cb96e561d3e997af7d434158031485f92677
57bfa864fe test: use static methods and clarify comment in addr_relay (stratospher)
7ee8c0abc6 test: protect outbound connection from eviction in getaddr_test (stratospher)
ecb5ce6e76 test: fix addr relay test silent pass and wrong peerinfo index (stratospher)
Pull request description:
couple of improvements in the addr relay test:
- fixes the silent test pass discovered in https://github.com/bitcoin/bitcoin/pull/34717#issuecomment-3990585763 (will remove this if that PR gets merged - the test doesn't fail even though #34717 changes the behaviour)
- correct wrong peerinfo index
- prevent intermittent disconnection warnings like the one shown below by protecting outbound peer from `ConsiderEviction`
```
TestFramework (INFO): Check that we answer getaddr messages only once per connection
TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:58829 due to [Errno 54] Connection reset by peer
```
- remove a no longer applicable test comment since we don't need to send initial GETADDR for intial self announcement anymore
ACKs for top commit:
Bortlesboat:
ACK 57bfa864fe. Ran both `p2p_addr_relay.py` and `p2p_addr_selfannouncement.py` locally, both pass. Good catch on the stale `peerinfo` reference in `inbound_blackhole_tests` — that would silently check the wrong peer.
naiyoma:
ACK 57bfa864fe
mzumsande:
Code Review ACK 57bfa864fe
Tree-SHA512: 22e4f87f66569bfa629a68a8b440cd21b5285c6dad6eb7926514f2d74e16fe3711525b264f82765c83020be976a0438b8d2ab1e48e7c0b7d85437ee672d52324
