92287ae753 test/wallet: ensure FastWalletRescanFilter is updated during scanning (Novo)
Pull request description:
Part of https://github.com/bitcoin/bitcoin/pull/34400
On master, the `wallet_fast_rescan.py` test will pass even if the fast rescan filters are not updated during wallet rescan. This PR improves the `wallet_fast_rescan.py` test so that this no longer happens. Testers can verify by applying this diff
```diff
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 63dab29972..f7d6c5f84a 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -1898,7 +1898,7 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
bool fetch_block{true};
if (fast_rescan_filter) {
- fast_rescan_filter->UpdateIfNeeded();
+ // fast_rescan_filter->UpdateIfNeeded();
auto matches_block{fast_rescan_filter->MatchesBlock(block_hash)};
if (matches_block.has_value()) {
if (*matches_block) {
```
and running the `wallet_fast_rescan.py` test. The test will pass on master but fail on this PR.
ACKs for top commit:
achow101:
ACK 92287ae753
Bortlesboat:
re-ACK 92287ae753
rkrux:
tACK 92287ae
ismaelsadeeq:
Tested ACK 92287ae753
Tree-SHA512: bcd66db0be6604b084230fa3d3aa8d99f5a1a679a7a92592a5e8962abbcf35a14fb6883f25c9e8e6c4799893b774b1c6766876587417f35c4190562ef5ad39fb
0026b330c4 wallet: fix amount computed as boolean in coin selection (furszy)
Pull request description:
Stumbled upon this tiny bug. This has been working by accident.
The comparison is evaluated first, so `total_amount` ends up holding a boolean instead of the actual amount.
It can be verified by adding the value to the returned error message and running the `wallet_create_tx.py`
test.
Note:
I assume something like `-Wparentheses` in CI (or similar) should help us catching similar issues elsewhere.
ACKs for top commit:
fjahr:
utACK 0026b330c4
achow101:
ACK 0026b330c4
andrewtoth:
utACK 0026b330c4
luke-jr:
utACK 0026b330c4
Tree-SHA512: 289c1eb34e59caae0a9e6814a14e4a7ba72f26e3b26717bb3f9e60335c9c5efcebe7e5997f799752096cf91df416a82b8677a9900e5ec54b6d13921d4299be96
b149a28f6b depends: Do not consider `CC` environment variable when detecting system (Hennadii Stepanov)
Pull request description:
On the master branch @ 3c88eac28e, consider the following commands in the `depends` subdirectory:
```sh
$ make print-build HOST=i686-pc-linux-gnu CC="clang -m32"
build=x86_64-pc-linux-gnu
$ make print-host HOST=i686-pc-linux-gnu CC="clang -m32"
host=i686-pc-linux-gnu
```
The printed variable values are expected.
However, switching the `CC` variable context from Makefile to the shell environment breaks expectations:
```sh
$ CC="clang -m32" make print-build HOST=i686-pc-linux-gnu
build=i686-pc-linux-gnu
$ CC="clang -m32" make print-host HOST=i686-pc-linux-gnu
host=i686-pc-linux-gnu
```
This PR fixes this issue.
#### UPDATE 2026-01-20
On the master branch @ 7f5ebef56a:
```
$ gmake print-build HOST=i686-pc-linux-gnu CC="clang -m32"
build=i686-pc-linux-gnu
$ gmake print-host HOST=i686-pc-linux-gnu CC="clang -m32"
host=i686-pc-linux-gnu
```
ACKs for top commit:
sedited:
Re-ACK b149a28f6b
ryanofsky:
Code review ACK b149a28f6b. `env --unset` was replaced with `unset &&` since last review. I still think it could be better to write `CC=$(build_CC)` here even if `build_CC` may not be defined at this point (https://github.com/bitcoin/bitcoin/pull/29963#issuecomment-3774229317) because it makes intent of the code more obvious, but current PR is already an improvement
Tree-SHA512: bd498706ad46aab93192e21b7cc30c34a714c5f31601122752fc94416dc51846b8d4eaf5d1c3250ba64d4eadf3fd43c9f7f5d9e178a47ee2165474ac6833fa31
c68e3d2c57 doc: add release notes for Tor PoW defenses (Vasil Dimov)
4bae84c94a doc: add a hint to enable PoW defenses to manual hidden services (Vasil Dimov)
4c6798a3d3 tor: enable PoW defenses for automatically created hidden services (Vasil Dimov)
fb993f7604 tor, fuzz: reuse constants instead of duplicating (Vasil Dimov)
Pull request description:
Enable [PoW defenses](https://tpo.pages.torproject.net/onion-services/ecosystem/technology/security/pow/) for hidden services that we create via Tor Control using the [`ADD_ONION` command](https://spec.torproject.org/control-spec/commands.html#add_onion).
The ability to do that has been added in [tor-0.4.9.2-alpha](02c1804446). Previous versions return a syntax error to the `ADD_ONION` command with `PoWDefensesEnabled=1`, so the approach here is to try with PoW and if we get syntax error, then retry without PoW.
Also update `doc/tor.md` with a hint on enabling PoW on manually configured Tor hidden services.
ACKs for top commit:
willcl-ark:
ACK c68e3d2c57
fjahr:
tACK c68e3d2c57
sedited:
ACK c68e3d2c57
Tree-SHA512: 56f57bc770b89389c35a4c0bc2a28804d17b1479ecd4d9b764695d6c9d2994425aee759e71658d7b57088bbe43ce90b94fb972f66d79ef903e0c1a4d6c4562f3
fa70b9ebaa ci: Temporarily use clang in valgrind tasks (MarcoFalke)
faf3ef4ee7 ci: Clarify why valgrind task has gui disabled (MarcoFalke)
fadb77169b test: Scale feature_dbcrash.py timeout with factor (MarcoFalke)
Pull request description:
valgrind currently does not work on GCC compiled executables, due to an upstream bug. https://bugs.kde.org/show_bug.cgi?id=472329
So temporarily switch to clang, so that a long term solution can be figured out in the meantime.
ACKs for top commit:
l0rinc:
ACK fa70b9ebaa
fanquake:
ACK fa70b9ebaa - also checked that it doesn't currently work under aarch64.
Tree-SHA512: 2e7c7a709311efa7bf29c3f9b1db60886b189b2d2bfebb516062163d65f0d7e8de3b6fc21c94cd62f6bd7e786e9c36fba55c4bae956b849851eb8b08e772c03e
578525d31d depends: Remove no longer necessary `dsymutil` (Hennadii Stepanov)
Pull request description:
I can't see where `dsymutil` is used. For example, a shared library under LTO builds fine:
```
cmake -B build --toolchain depends/arm64-apple-darwin/toolchain.cmake -DCMAKE_INTERPROCEDURAL_OPTIMIZATION=ON -DBUILD_KERNEL_LIB=ON -DBUILD_SHARED_LIBS=ON
cmake --build build -t libbitcoinkernel
```
ACKs for top commit:
fanquake:
ACK 578525d31d
Tree-SHA512: 41e7601e26f639383bfa78ceb97e45d03e6cf098cf636bc4f3274360516bb78753aacfd89c8541b38ee4f39cbbf7c7b7a6eb8dc9a653f4438392cd61678e80ce
0fe6fccec2 doc: Document rationale for using `IWYU pragma: export` (Hennadii Stepanov)
cfa3b10d50 iwyu, doc: Document `IWYU pragma: export` for `<logging/categories.h>` (Hennadii Stepanov)
015bea05e6 iwyu, doc: Document `IWYU pragma: export` for `<chrono>` (Hennadii Stepanov)
48bfcfedec iwyu, doc: Document `IWYU pragma: export` for `<threadsafety.h>` (Hennadii Stepanov)
179abb387f refactor: Move `StdMutex` to its own header (Hennadii Stepanov)
6d2952c3c3 serialize: Add missing `<span>` header (Hennadii Stepanov)
Pull request description:
This PR is a prerequisite for https://github.com/bitcoin/bitcoin/pull/34448. It was split into a separate PR to limit the scope and minimize potential merge conflicts.
The first commit improves the accuracy of IWYU suggestions within our heavily templated code. Note that, for now, the `serialize.h` header itself is excluded from IWYU inspection because it lacks a corresponding source file.
The remaining commits follow the Developer Notes [guidance](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#using-iwyu):
> Use `IWYU pragma: export` very sparingly, as this enforces transitive inclusion of headers and undermines the specific purpose of IWYU.
ACKs for top commit:
maflcko:
review ACK 0fe6fccec2👤
ajtowns:
utACK 0fe6fccec2
Tree-SHA512: dc2d4e3ff78b9707a1a26cb9b1c0a456de0d33c59e773bbf692344c2fceaff8936317479c5e898038f29134bc0e5d9d1ef7350e53512dd8e262f46ede578c4f9
7a9304f887 depends: Fix cross-compiling on macOS for Windows (Hennadii Stepanov)
Pull request description:
1. Use `build_os` instead of `host_os` for native packages.
2. `XCODE_VERSION` is available only for `darwin` hosts. Therefore, simply disable the Xcode version check for `native_qt`.
Fixes https://github.com/bitcoin/bitcoin/issues/34874.
ACKs for top commit:
fanquake:
ACK 7a9304f887
Tree-SHA512: ef3349de19f3cdb6f445d44f8650815ded5dbe359faafc02941b2f2f0296905aaaac67a4916372b17de58bebabcbcdcbc21af6fe255ebb8f4cdfab00ff257602
0dc337f73d gui: Fix TransactionsView on setCurrentWallet (pablomartin4btc)
Pull request description:
<details>
<summary>Currenlty on <code>master</code>, when the "mask values" checkbox is ticked if the user selects a different wallet, the history action is enable and if the user clicks on it can see all the transactions in the transaction view.</summary>
</details>
<details>
<summary>This PR fixes it.</summary>
</details>
Note for maintainers: this needs to be backported to 25.x and 26.x.
ACKs for top commit:
hebasto:
ACK 0dc337f73d, tested on Fedora 43.
Tree-SHA512: 54581546917f87b4c1db0ff1eaa1962ee6eb078285dbb205b4c8d027c3e350f3dc46409b376948c10e668f9487b7a5a70bab0dff5faf510deab1a54452f7f0e5
The comparison is evaluated before the assignment, so total_amount
ends up holding a boolean instead of the actual amount:
total_amount = (a - b < c)
which is not what we want here. This has been working by accident.
74f71c5054 Remove Taproot activation height (Sjors Provoost)
Pull request description:
Drop `DEPLOYMENT_TAPROOT` from `consensus.vDeployments`.
Now that the commit (7c08d81e11) which changes taproot to be enforced for all blocks is included in v24.0, it seems a good time to remove no longer needed non-consensus code.
Clarify what is considered a `BuriedDeployment`.
We drop taproot from `getdeploymentinfo` RPC, rather than mark it as `buried`, because this is not a buried deployment in the sense of [BIP 90](https://github.com/bitcoin/bips/blob/master/bip-0090.mediawiki). This is because the activation height has been completely removed from the code, unlike the hardcoded `DEPLOYMENT_SEGWIT` height which is still relied on.[^1]
See discussion in #24737 and #26162.
[^1]: `DEPLOYMENT_SEGWIT` is used in `IsBlockMutated` to determine if witness data is allowed, it's used for [BIP147](https://github.com/bitcoin/bips/blob/master/bip-0147.mediawiki), to trigger `NeedsRedownload()` for users who upgraded after a decade, and for a few other things.
ACKs for top commit:
ajtowns:
reACK 74f71c5054
achow101:
ACK 74f71c5054
sedited:
Re-ACK 74f71c5054
darosior:
utACK 74f71c5054
Tree-SHA512: 217e0ee2e144ccfb04cf012f45b75d08f8541287a5531bd18aa81e38bad2f38d28b772137f471c46b63875840ec044cb61a2a832e3a9e89a6183e8ab36b1b9c9
a1f22a0a6b test: Suppress another unsolicited `mock_process/*` output (Hennadii Stepanov)
Pull request description:
This is a follow-up to https://github.com/bitcoin/bitcoin/pull/33929.
The[ `mock_process/*`](390e7d61bd/src/test/mock_process.cpp (L10)) test cases, which serve as helpers for [`system_tests`](390e7d61bd/src/test/system_tests.cpp (L23)) rather than actual tests, are invoked in a way that suppresses unsolicited output to stdout or stderr to keep the test results reproducible.
However, in debug builds, the Windows CRT still prints false-positive memory leak dumps to stderr.
This PR handles this specific case and documents the other suppressions.
ACKs for top commit:
maflcko:
lgtm ACK a1f22a0a6b
sedited:
ACK a1f22a0a6b
Tree-SHA512: 480f0f74ce50b6fb315bfeb6f6d3a4a4791557a6177aa995197c002bf871f878573df0f313344206f82e4d54c7a62d7cea2fdcb8cd96475b4b6cc7d4ffaf0538
1. Use `build_os` instead of `host_os` for native packages.
2. `XCODE_VERSION` is available only for `darwin` hosts. Therefore,
simply disable the Xcode version check for `native_qt`.
faaea7895f refactor: Use current_time over redundant call to Now() (MarcoFalke)
3333c5023f refactor: Use NodeClock::time_point for m_addr_token_timestamp (MarcoFalke)
fa55723b8f move-only: Extract ProcessAddrs() helper (MarcoFalke)
Pull request description:
It is a bit confusing to have some code use the deprecated `GetTime`, which returns a duration and not a time point, and other code to use `NodeClock` time points.
Fix one place `m_addr_token_timestamp` to use `NodeClock::time_point`.
Also:
* Extract a `ProcessAddrs` helper, similar to the other `Process*()` helpers, to cut down the `ProcessMessage` with a massive scope.
* Rename the confusing `current_a_time` to `now_seconds`. (The `a` in this context refers to the removed "adjusted" time, see commit fadd8b2676, which removed adjusted time here)
ACKs for top commit:
l0rinc:
ACK faaea7895f
ajtowns:
reACK faaea7895f
sedited:
Re-ACK faaea7895f
Tree-SHA512: 67ad13e9d7b88e08e3d723e6b7cd598b38df2a004f5c2338b24f2992e25ae9d8fb8e5325c9c94171e551fe86d87e3e3ec1fe6baae64edbf6b5c125f408ee64e4
b14f2c76a1 tests: applied PYTHON_GIL to the env for every test (kevkevinpal)
Pull request description:
## Summay
If a user is running python3.14.0t they would see a warning log that would fail the integration test suite.
This change adds `PYTHON_GIL=1` to the env when running our functional test suite to ensure that the tests pass for users running python3.14.0t and are not manually setting `PYTHON_GIL=1`.
This resolves https://github.com/bitcoin/bitcoin/issues/33582
### Tests before and after
#### Before
```
./build/test/functional/test_runner.py interface_ipc.py
Temporary test directory at /tmp/test_runner_₿_🏃_20260319_142327
Remaining jobs: [interface_ipc.py]
1/1 - interface_ipc.py failed, Duration: 2 s
stdout:
2026-03-19T18:23:27.330123Z TestFramework (INFO): PRNG seed is: 4933091336597497631
2026-03-19T18:23:27.380917Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20260319_142327/interface_ipc_0
2026-03-19T18:23:28.625944Z TestFramework (INFO): Running echo test
2026-03-19T18:23:28.635856Z TestFramework (INFO): Running mining test
2026-03-19T18:23:28.648965Z TestFramework (INFO): Running deprecated mining interface test
2026-03-19T18:23:28.653589Z TestFramework (INFO): Running disconnect during BlockTemplate.waitNext
2026-03-19T18:23:28.821124Z TestFramework (INFO): Running thread busy test
2026-03-19T18:23:29.195589Z TestFramework (INFO): Stopping nodes
2026-03-19T18:23:29.299135Z TestFramework (INFO): Cleaning up /tmp/test_runner_₿_🏃_20260319_142327/interface_ipc_0 on exit
2026-03-19T18:23:29.299329Z TestFramework (INFO): Tests successful
stderr:
<frozen importlib._bootstrap>:491: RuntimeWarning: The global interpreter lock (GIL) has been enabled to load module 'capnp.lib.capnp', which has not declared that it can run safely without the GIL. To override this behavior and keep the GIL disabled (at your own risk), run with PYTHON_GIL=0 or -Xgil=0.
TEST | STATUS | DURATION
interface_ipc.py | ✖ Failed | 2 s
ALL | ✖ Failed | 2 s (accumulated)
Runtime: 2 s
```
#### After
```
./build/test/functional/test_runner.py interface_ipc.py
Temporary test directory at /tmp/test_runner_₿_🏃_20260319_142221
Remaining jobs: [interface_ipc.py]
1/1 - interface_ipc.py passed, Duration: 2 s
TEST | STATUS | DURATION
interface_ipc.py | ✓ Passed | 2 s
ALL | ✓ Passed | 2 s (accumulated)
Runtime: 2 s
```
ACKs for top commit:
maflcko:
review ACK b14f2c76a1
fanquake:
ACK b14f2c76a1
Tree-SHA512: e5862d2e9211154d4834c88864e8c4e35de195986511ba151871d39266d177e0718960b28020e815ef6b353a0d82800b7cb68e9a6dee82fc85f12d8705e787a8
658e68f95b scripted-diff: Rename `WAIT_TIMEOUT` to `TEST_WAIT_TIMEOUT` (Hennadii Stepanov)
Pull request description:
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.
Split from https://github.com/bitcoin/bitcoin/pull/34448.
Similar to https://github.com/bitcoin/bitcoin/pull/34454.
ACKs for top commit:
w0xlt:
ACK 658e68f95b
Tree-SHA512: 90cf8927e4e41dee24d51fb2ea3335526ff5da4180c24d776d59b642794715b3c6558628088fbc28236d6ac4fffb9a27167fe309cb94ebf04f04c6e5cf957ad5
fa71c6e84c ci: Avoid intermittent Windows generate download failures (MarcoFalke)
Pull request description:
The CI Windows step to generate the build system is problematic, because with a clean cache, it will download artifacts over the network. This may intermittently fail due to intermittent network issues.
Fix this issue, like all other network issues in CI, by retrying once.
ACKs for top commit:
hodlinator:
ACK fa71c6e84c
hebasto:
ACK fa71c6e84c, I have reviewed the code and it looks OK.
Tree-SHA512: 9915e84b6015116cce86a25b4f77e545baef1f26fabe767c8bb07a009b075f76c27f9172dd23f78aa8017fd611cea850b1e985a357bd0b24543e63e1fd85cb6d
e76e886581 guix: doc: zdiff3 doesn't work (David Gumberg)
ea1be38677 guix: doc: Suggest guix-install.sh --uninstall (David Gumberg)
Pull request description:
Add a note about guix issues with git `merge.conflictstyle` being set to `zdiff3`, and add information about how to uninstall for users of `guix-install.sh`
ACKs for top commit:
achow101:
ACK e76e886581
hebasto:
ACK e76e886581.
sedited:
utACK e76e886581
Tree-SHA512: e9edc49851579c52e7ac762221d0bb51c928da4dff5566ece766af7f7eb75466831a76f885ccc5a26207c84b846e3f42bae26e80ddece3df1611de2c2320c28a
d8f4e7caf0 doc: add release notes (ismaelsadeeq)
248c175e3d test: ensure `ValidateInputsStandardness` optionally returns debug string (ismaelsadeeq)
d2716e9e5b policy: update `AreInputsStandard` to return error string (ismaelsadeeq)
Pull request description:
This PR is another attempt at #13525.
Transactions that fail `PreChecks` Validation due to non-standard inputs now returns invalid validation state`TxValidationResult::TX_INPUTS_NOT_STANDARD` along with a debug error message.
Previously, the debug error message for non-standard inputs do not specify why the inputs were considered non-standard.
Instead, the same error string, `bad-txns-nonstandard-inputs`, used for all types of non-standard input scriptSigs.
This PR updates the `AreInputsStandard` to include the reason why inputs are non-standard in the debug message.
This improves the `Precheck` debug message to be more descriptive.
Furthermore, I have addressed all remaining comments from #13525 in this PR.
ACKs for top commit:
instagibbs:
ACK d8f4e7caf0
achow101:
ACK d8f4e7caf0
sedited:
Re-ACK d8f4e7caf0
Tree-SHA512: 19b1a73c68584522f863b9ee2c8d3a735348667f3628dc51e36be3ba59158509509fcc1ffc5683555112c09c8b14da3ad140bb879eac629b6f60b8313cfd8b91
fa4ec13b44 build: Enable -Wcovered-switch-default (MarcoFalke)
fa2670bd4b refactor: Enable -Wswitch in exhaustive switch (MarcoFalke)
Pull request description:
The compiler flag `-Wswitch` is enabled. However, it can not fire when a `default:` case exists. Fix that by removing the default case where a switch is already handling all cases exhaustively.
Also, enable `-Wcovered-switch-default` to catch those cases at compile time in the future.
Also, apply the comment according to the dev notes.
Can be reviewed via `--ignore-all-space`
ACKs for top commit:
stickies-v:
re-ACK fa4ec13b44, no changes except for addressing silent merge conflict from d339884f1d
l0rinc:
ACK fa4ec13b44
achow101:
ACK fa4ec13b44
sedited:
ACK fa4ec13b44
Tree-SHA512: 8dd9e71a8cd338255f43448a59a1a4d40a9fc16e19a707cc10fb71442d4df9f82a0e5fae77868ef49cd0ea27fdd972687572c1a50b6aba7e08c6ce87576afc6a
9f28120a5b kernel: Add API function for getting a tx input's nSequence (Sebastian Falbesoner)
6b64b181d5 kernel: Add API function for getting a tx's nLockTime (Sebastian Falbesoner)
Pull request description:
This PR introduces two new C API functions to libbitcoinkernel:
* `btck_transaction_get_locktime` to access a transaction's `nLockTime` value
* `btck_transaction_input_get_sequence` to access a transaction input's `nSequence` value
Inspired by https://bnoc.xyz/t/forward-compatible-coinbase-locktimes-for-bip-54. After reading this I thought it would be a nice/useful showcase to check BIP54 compliance of (historical) blocks using bitcoinkernel, without having to manually deserialize the transaction (this is just about one of the four BIP54 rules though, especially the sigops limit is much more involved).
ACKs for top commit:
sedited:
ACK 9f28120a5b
yuvicc:
ACK 9f28120a5b
stickies-v:
ACK 9f28120a5b
Tree-SHA512: 9eae795d6e4b9b367bbfe2665b916121ef64031e8d10667c71741344b5eea4c2562862a937bdf1363cc66b67bb5d48392c9f44e52f0d92d2a5a65e10d061b703
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.
