445143bfc6 wallet: document structured importdescriptors errors (Renato Britto)
Pull request description:
Related to #29912 and the machine-readable OpenRPC generated from `RPCHelpMan` metadata work discussed in #34683. Split out from #34764 per review feedback that this change is conceptually separate from the broader elision work there.
The `importdescriptors` RPC help currently documents the optional `error` field using an elided `JSONRPC error` placeholder. This PR replaces that with explicit `code` and `message` fields.
`importdescriptors` already returns a structured JSON-RPC error object in practice, so this makes the documented result schema match the existing response shape more closely.
This PR changes `importdescriptors` help text, and the runtime checks on the returned json to be more strict.
ACKs for top commit:
nervana21:
tACK 445143bfc6
maflcko:
lgtm ACK 445143bfc6
Tree-SHA512: 3bda1cc7dd222c1d2e4dfbb2ee4a3f0201914c8b6bed5efb7fe7866227867c43235a9c5f5ec6f56e7ba286db0a43962c782d7ff29eec9aef4e70dc2c3daa6a0e
db3c25cfae index: add explicit early exit in NextSyncBlock() when the input is the chain tip (Hao Xu)
Pull request description:
When `pindex_prev` is the chain tip, `NextSyncBlock()` should return `nullptr` (there's no next block to sync). Currently this works, but relies on the fork/reorg path to produce the result: `chain.Next(tip)` returns `nullptr`, falling through to `FindFork(tip)` which returns `tip` itself, then `chain.Next(tip)` returns `nullptr` again.
Add an explicit early return for this case. This makes `NextSyncBlock()`'s three cases self-documenting:
1. next block exists on the active chain — return it
2. at the chain tip — return `nullptr`
3. on a stale branch — find the fork point and return the block after it
It also makes the existing comment ("Since block is not in the chain") accurate — the tip is in the chain, so it shouldn't reach that path.
ACKs for top commit:
optout21:
crACK db3c25cfae
furszy:
ACK db3c25cfae
stickies-v:
ACK db3c25cfae
Tree-SHA512: c1633bb3d3ffed2643c8e174c2b2283deaeffd0a06eb3fb2cf03eed097bfdc5cf6fbd7ebdcdb44fd56f712be8004ad46c730a33990fb1fc6cfa629c9b14f8cd9
3dcdb2b9ba test: wallet: Warning for excessive fallback fee. (David Gumberg)
6664e41e56 test: wallet: -fallbackfee default is 0 (David Gumberg)
d28c989243 test: wallet: refactor: fallbackfee extract common send failure checks. (David Gumberg)
Pull request description:
In an unmerged branch of #32636 (097e00f907) I unintentionally broke default `-fallbackfee` behavior, but this was not caught by any tests. See https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2706102550.
Something like the following diff does not cause any test failures on master despite causing a behavior change:
```diff
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index bc27018cd2..079610fba0 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -3048,24 +3048,24 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
if (const auto arg{args.GetArg("-fallbackfee")}) {
std::optional<CAmount> fallback_fee = ParseMoney(*arg);
if (!fallback_fee) {
error = strprintf(_("Invalid amount for %s=<amount>: '%s'"), "-fallbackfee", *arg);
return nullptr;
} else if (fallback_fee.value() > HIGH_TX_FEE_PER_KB) {
warnings.push_back(AmountHighWarn("-fallbackfee") + Untranslated(" ") +
_("This is the transaction fee you may pay when fee estimates are not available."));
}
walletInstance->m_fallback_fee = CFeeRate{fallback_fee.value()};
+ // Disable fallback fee in case value was set to 0, enable if non-null value
+ walletInstance->m_allow_fallback_fee = walletInstance->m_fallback_fee.GetFeePerK() != 0;
}
- // Disable fallback fee in case value was set to 0, enable if non-null value
- walletInstance->m_allow_fallback_fee = walletInstance->m_fallback_fee.GetFeePerK() != 0;
```
This PR adds a functional test check that when no `-fallbackfee` argument is set and fee estimation is not possible, that sending fails because `-fallbackfee` is disabled by default.
ACKs for top commit:
maflcko:
review ACK 3dcdb2b9ba🐞
w0xlt:
reACK 3dcdb2b9ba
ismaelsadeeq:
reACK 3dcdb2b9ba👾
Tree-SHA512: 715625673a781ba3ddfed25f0836b01c2197480bd56732fd1ce548e8969573c2a36601de0b8913b3b79a47b8149022aabcf409b62297e7c2c47b68a8843e6570
890a09b1e4 fuzz: Use CAmount for storing best_waste (Ava Chow)
Pull request description:
Waste is a CAmount, which is an int64_t. This will overflow an int, so `best_waste` should also be a `CAmount`.
Fixes#34936
ACKs for top commit:
murchandamus:
ACK 890a09b1e4
furszy:
ACK 890a09b1e4
Tree-SHA512: c6c4f530960f038675d4549c2285c6a4a828099a631486e317ec1215d89688ce109304654a95800978607c360c2ed34803523f5c56ebf7c2324ca095f87825b8
65379bb8d0 ci: add FreeBSD cross CI job (fanquake)
f44191f163 depends: build qrencode for Freebsd (fanquake)
7f7018738e depends: FreeBSD cross with Clang (fanquake)
6464f14081 depends: disable inotify in Freebsd Qt build (fanquake)
Pull request description:
Alternative to #33562, which was adding a native FreeBSD job; however that had issues with permissions/caching, as well as potential determinism issues. This adds a FreeBSD cross job using Linux and Clang.
Would close#33438. The same changes here could also be used to produce FreeBSD binaries out of Guix.
ACKs for top commit:
hebasto:
ACK 65379bb8d0. I've cross-compiled on Ubuntu 25.10 for FreeBSD 14.4 and 15.0. The former binaries (`bitcoind`, `test_bitcoin` and `bitcoin-qt`) were tested on FreeBSD 14.4 locally.
Tree-SHA512: 52a3edaa56fe40ca901416cb9e1af04a84505526edfa7309bfa40024baa7d3b1a05303659553d9fbcf1f49d4e3d42b415a1e2523d448b22724d1415a49331259
f899674639 test: script: boundary at exactly 65535 bytes must use OP_PUSHDATA2 (Bruno Garcia)
Pull request description:
I noticed that the following mutant is not killed by any current test (can be tested with: `cmake --build build -j $(nproc) && ./build/bin/test_bitcoin && ./build/test/functional/test_runner.py -j $(nproc) -F`):
```diff
diff --git a/src/script/script.cpp b/src/script/script.cpp
index 3f764aaf21..5cff51d2cf 100644
--- a/src/script/script.cpp
+++ b/src/script/script.cpp
@@ -387,7 +387,7 @@ bool CheckMinimalPush(const std::vector<unsigned char>& data, opcodetype opcode)
} else if (data.size() <= 255) {
// Must have used OP_PUSHDATA.
return opcode == OP_PUSHDATA1;
- } else if (data.size() <= 65535) {
+ } else if (data.size() < 65535) {
// Must have used OP_PUSHDATA2.
return opcode == OP_PUSHDATA2;
}
```
This PR addresses it by adding a new test case to ensure that the boundary at exactly 65535 bytes must use OP_PUSHDATA2 as well.
ACKs for top commit:
kevkevinpal:
tACK [f899674](f899674639)
danielabrozzoni:
tACK f899674639
darosior:
utACK f899674639
w0xlt:
ACK f899674639
Tree-SHA512: ad35cc992aa351d26151cb79d1b1d5d960b1d80a98b3076a709aa19f7b5135edb87a957d2c84f359e86da8a15f7f0196301bfaff5ae554aecc65d81c97f8af3e
261d229455 test: Replace DEBUG_LOG_OUT with -printtoconsole=1 (Hodlinator)
Pull request description:
`-printtoconsole=1` has the same functionality but works in more cases than `DEBUG_LOG_OUT`, so remove the latter (`-printtoconsole` is already used when fuzzing). This means we can drop `G_TEST_LOG_FUN`.
---
Behavior can be verified through
```shell
ctest --test-dir build --debug
```
and checking for:
```
142: Test command: /home/hodlinator/bc/2/build/bin/test_bitcoin "--run_test=coinselector_tests" "--catch_system_error=no" "--log_level=test_suite" "--" "-printtoconsole=1"
...
142: 2026-03-26T13:40:02.169392Z [test] [../src/kernel/context.cpp:20] [operator()] Using the 'sse4(1way);sse41(4way);avx2(8way)' SHA256 implementation
```
---
Inspired by: https://github.com/bitcoin/bitcoin/pull/34918#discussion_r2994780532
ACKs for top commit:
nkatha23:
ACK 261d229
maflcko:
review ACK 261d229455📬
kevkevinpal:
crACK [261d229](261d229455)
janb84:
cr ACK 261d229455
Tree-SHA512: f4c793b4a00730ade113241baeaaef08ae0333df15ada5929dd46aacd1ac875733e58aa341fac8fb5875eb5ebca735d40c664bf0ef62132094e0c993da5b20e5
70f632bda8f Merge bitcoin-core/libmultiprocess#265: ci: set LC_ALL in shell scripts
8e8e564259a Merge bitcoin-core/libmultiprocess#249: fixes for race conditions on disconnects
05d34cc2ec3 ci: set LC_ALL in shell scripts
e606fd84a8c Merge bitcoin-core/libmultiprocess#264: ci: reduce nproc multipliers
ff0eed1bf18 refactor: Use loop variable in type-context.h
ff1d8ba172a refactor: Move type-context.h getParams() call closer to use
1dbc59a4aa3 race fix: m_on_cancel called after request finishes
1643d05ba07 test: m_on_cancel called after request finishes
f5509a31fcc race fix: getParams() called after request cancel
4a60c39f24a test: getParams() called after request cancel
f11ec29ed20 race fix: worker thread destroyed before it is initialized
a1d643348f4 test: worker thread destroyed before it is initialized
336023382c4 ci: reduce nproc multipliers
b090beb9651 Merge bitcoin-core/libmultiprocess#256: ci: cache gnu32 nix store
be8622816da ci: cache gnu32 nix store
975270b619c Merge bitcoin-core/libmultiprocess#263: ci: bump timeout factor to 40
09f10e5a598 ci: bump timeout factor to 40
db8f76ad290 Merge bitcoin-core/libmultiprocess#253: ci: run some Bitcoin Core CI jobs
55a9b557b19 ci: set Bitcoin Core CI test repetition
fb0fc84d556 ci: add TSan job with instrumented libc++
0f29c38725b ci: add Bitcoin Core IPC tests (ASan + macOS)
3f64320315d Merge bitcoin-core/libmultiprocess#262: ci: enable clang-tidy in macOS job, use nullptr
cd9f8bdc9f0 Merge bitcoin-core/libmultiprocess#258: log: add socket connected info message and demote destroy logs to debug
b5d6258a42f Merge bitcoin-core/libmultiprocess#255: fix: use unsigned char cast and sizeof in LogEscape escape sequence
d94688e2c32 Merge bitcoin-core/libmultiprocess#251: Improved CustomBuildField for std::optional in IPC/libmultiprocess
a9499fad755 mp: use nullptr with pthread_threadid_np
f499e37850f ci: enable clang-tidy in macOS job
98f1352159d log: add socket connected info message and demote destroy logs to debug
554a481ea73 fix: use unsigned char cast and sizeof in LogEscape escape sequence
1977b9f3f65 Use std::forward in CustomBuildField for std::optional to allow move semantics, resolves FIXME
22bec918c97 Merge bitcoin-core/libmultiprocess#247: type-map: Work around LLVM 22 "out of bounds index" error
8a5e3ae6ed2 Merge bitcoin-core/libmultiprocess#242: proxy-types: add CustomHasField hook to map Cap'n Proto values to null C++ values
e8d35246918 Merge bitcoin-core/libmultiprocess#246: doc: Bump version 8 > 9
97d877053b6 proxy-types: add CustomHasField hook for nullable decode paths
8c2f10252c9 refactor: add missing includes to mp/type-data.h
b1638aceb40 doc: Bump version 8 > 9
f61af487217 type-map: Work around LLVM 22 "out of bounds index" error
git-subtree-dir: src/ipc/libmultiprocess
git-subtree-split: 70f632bda8f80449b6240f98da768206a535a04e
fa1ebde1ad fuzz: Use time helpers in node_eviction (MarcoFalke)
Pull request description:
The `node_eviction` fuzz test has many issues:
* It uses the full `int64_t` range (in seconds) as input, which is absurdly large (millions of years) and also violates https://en.cppreference.com/w/cpp/chrono/duration.html:
> Each of the predefined duration types up to hours covers a range of at least ±292 years.
* It does not use the existing `ConsumeDuration` and `ConsumeTime` helpers, which makes specifying a proper range difficult.
So fix all issues by using `ConsumeTime` for time points with default arguments, and `ConsumeDuration` with the same precision, as well as possibly negative values.
ACKs for top commit:
marcofleon:
crACK fa1ebde1ad
brunoerg:
reACK fa1ebde1ad
w0xlt:
ACK fa1ebde1ad
Tree-SHA512: 22045e6c563a9169327737895ea2f3a7b1dcb4fd24fce56d91caa1e132d03a85cbaaa5f78218d23cfa203fe2ee4b147894c02870eb20ae1c232ad55ccdb6f7f7
0d1301b47a test: functional: drop rmtree usage and add lint check (David Gumberg)
8bfb422de8 test: functional: drop unused --keepcache argument (David Gumberg)
a7e4a59d6d qa: Remove all instances of `remove_all` except test cleanup (David Gumberg)
Pull request description:
Both `fs::remove_all` and `shutil::rmtree()` are a bit dangerous, and most of their uses are not necessary, this PR removes most instances of both.
`remove_all()` is still used in in `src/test/util/setup_common.cpp` as part of `BasicTestingSetup::BasicTestingSetup`'s constructor and destructor, and it is used in the kernel test code's [`TestDirectory`](4ae00e9a71/src/test/kernel/test_kernel.cpp (L100-L112)):
734899a4c4/src/test/kernel/test_kernel.cpp (L100-L112)
In both cases, `remove_all` is likely necessary, but the kernel's test code is RAII, ideally `BasicTestingSetup` could be made similar in a follow-up or in this PR if reviewers think it is important.
Similarly in the python code, most usage was unnecessary, but there are a few places where `rmtree()` was necessary, I have added sanity checks to make sure these are inside of the `tmpdir` before doing recursive delete there.
ACKs for top commit:
achow101:
ACK 0d1301b47a
hodlinator:
ACK 0d1301b47a
sedited:
ACK 0d1301b47a
Tree-SHA512: da8ca23846b73eff0eaff61a5f80ba1decf63db783dcd86b25f88f4862ae28816fc9e2e9ee71283ec800d73097b1cfae64e3c5ba0e991be69c200c6098f24d6e
fa73ed467c refactor: Fix redundant conversion to std::string and then to std::string_view [performance-string-view-conversions] (MarcoFalke)
fa270fdacf refactor: Return std::optional from GetProxy (MarcoFalke)
faeac1a931 refactor: Return std::optional from GetNameProxy (MarcoFalke)
Pull request description:
Currently the getters have a mutable reference as inout param and return a bool to indicate success. This is confusing, because the success bool is redundant with the `IsValid()` state on the proxy object.
So in theory, the functions could reset the mutable proxy object to an invalid state and return `void`.
However, this would also be confusing, because devs can forget to check `IsValid()`.
Fix all issues by using `std::optional<Proxy>`, where devs no longer have to check `IsValid()` manually, or a separate bool. Note that new code in the repo is already using `std::optional<Proxy>`, see `git grep 'std::optional<Proxy>' bitcoin-core/master`. Also, `std::optional<Proxy>` will enforce checking at compile time, whereas calling `Proxy::IsValid` is not enforced.
ACKs for top commit:
achow101:
ACK fa73ed467c
sedited:
ACK fa73ed467c
ViniciusCestarii:
ACK fa73ed467c
frankomosh:
Code Review ACK fa73ed467c. Good refactor, correctly replaces the bool + mutable reference output parameter pattern with `std::optional<Proxy>` across `GetProxy` and `GetNameProxy`. Semantics are preserved.
Tree-SHA512: c6a1e1d1691958d2e6507e32e3484f96703fba03ccc710145ae2fb84b1254fb0e6e1d8d78e9b572daf5ea485247b73568704881762379b50bcf939a35494dd13
a49f97ff4a net: Don't log own ips during discover (sedited)
Pull request description:
The -logips option seems to imply that ip addresses are only logged when it is set. This seems like an obvious case for not logging these by default. There might be prior discussion around this, but I was unable to find why these might be exempt. There are also a few issues (both open and closed) where printing these lines was useful.
ACKs for top commit:
l0rinc:
tested ACK a49f97ff4a
achow101:
ACK a49f97ff4a
willcl-ark:
tACK a49f97ff4a
danielabrozzoni:
tACK a49f97ff4a
Tree-SHA512: 177911c5607b794965facf568fec71eb22fc8e9d16f4fee5088745e8aba51cb4ce839876c456470dc52839ebc36976dc6b81a50f546941aebb8db538d8fd4554
fabbfec3b0 fuzz: Remove unused g_setup pointers (MarcoFalke)
Pull request description:
This is unused and avoids a clang warning.
Can be tested via: `git grep --count '\<g_setup' | grep ':2'`
Before: Prints files names.
After: No output.
ACKs for top commit:
hebasto:
ACK fabbfec3b0.
hodlinator:
ACK fabbfec3b0
Tree-SHA512: 21a3459574316617aa98bb04e5b27173c4bb1e89d71ce1fb3fb3066f8d6e2b3bf1d1fd516a74f7ae3f9052b492edeec00d3365cb5781c5f79ebb828da7af520e
1438165b1f wallet: drop stale TODOs (Sjors Provoost)
Pull request description:
I don't remember why I added them. There's also no test that demonstrates the problem, or an open issue requesting a fix.
It's also unclear if this even _should_ be changed: https://github.com/bitcoin/bitcoin/pull/34912#issuecomment-4121780033
ACKs for top commit:
polespinasa:
ACK 1438165b1f
kevkevinpal:
ACK [1438165](1438165b1f)
w0xlt:
ACK 1438165b1f
Tree-SHA512: 5b6024f38222444488ea3892d1b32277a4091d9c17bc4e8d54efda4d8b8ec567fe639b07fe5bfb26fc9cb8a783bac455dd143f1d731ce111449c9a3aec13368d
These are unused and removing them avoids clang warnings like:
src/test/fuzz/deserialize.cpp:42:26: error: variable g_setup set but not used [-Werror,-Wunused-but-set-variable]
3129d4a693 ci: Rename `TIDY_LLVM_V` to `IWYU_LLVM_V` in IWYU-specific code (Hennadii Stepanov)
0b489886f8 ci: Upgrade IWYU to 0.26 compatible with Clang 22 (Hennadii Stepanov)
Pull request description:
This PR upgrades IWYU to [0.26](https://github.com/include-what-you-use/include-what-you-use/releases/tag/0.26) and removes mapping workarounds for issues that have been fixed upstream.
ACKs for top commit:
fanquake:
ACK 3129d4a693
Tree-SHA512: 9a926e489573d040423461c039ecda7636beb70e8214a02c1c594cbd5b89d26331455a9872c38993fa5ee6d27fdfefc2375dedc369721b2933411542a57a3884
`shutil.rmtree` is dangerous because it recursively deletes. There are
not likely to be any issues with it's current uses, but it is possible
that some of the assumptions being made now won't always be true, e.g.
about what some of the variables being passed to `rmtree` represent.
For some remaining uses of rmtree that can't be avoided for now, use
`cleanup_dir` which asserts that the recursively deleted folder is a
child of the the `tmpdir` of the test run. Otherwise,
`tempfile.TemporaryDirectory` should be used which does it's own
deleting on being garbage collected, or old fashioned unlinking and
rmdir in the case of directories with known contents.
At the time this was added in #10197, building the test cache took 21
seconds, as described in that PR, but this is no longer true, as
demonstrated by running the functional test framework with and without
the --keepcache arguments on master prior to this commit:
```
hyperfine --warmup 1 --export-markdown results.md --runs 3 \
-n 'without --keepcache' './build/test/functional/test_runner.py -j $(nproc)' \
-n 'with --keepcache' './build/test/functional/test_runner.py -j $(nproc) --keepcache'
```
| Command | Mean [s] | Min [s] | Max [s] | Relative |
|:----------------------|---------------:|--------:|---:|---:|
| `without --keepcache` | 76.373 ± 3.058 | 74.083 | 79.846 | 1.00 |
| `with --keepcache` | 77.384 ± 1.836 | 75.952 | 79.454 | 1.01 ± 0.05 |
As a consequence, this argument can be removed from the test runner and
this also has the benefit of being able to use an RAII-like
`tempfile.TemporaryDirectory` instead of having to clean up the cache
manually at the end of test runs.
bitcoin/bitcoin#10197: https://github.com/bitcoin/bitcoin/pull/10197
Adds a lint check for `remove_all()`
`fs::remove_all()`/`std::filesystem::remove_all()` is extremely
dangerous, all user-facing instances of it have been removed, and it
also deserves to be removed from the places in our test code where it is
being used unnecessarily.
69baddc910 validation: do not add the snapshot block to candidates of bg chainstate (Martin Zumsande)
Pull request description:
The snapshot block needs to be added to the candidates set of the assumed-valid chain because it will be the tip of that chainstate right after snapshot activation.
However, adding it also to the background chainstate is not necessary for anything. Before, the index would be in the set without being connectable. It will be eventually added to the set as part of the normal block download - no extra logic is necessary here.
This simplifies a unit test which had a comment that having the block in the set is "not intended".
This was suggested [here](https://github.com/bitcoin/bitcoin/pull/34521#discussion_r2849281299) and [here](https://github.com/bitcoin/bitcoin/pull/34521#discussion_r2883024248) in #34521
Note that adding the snapshot block was harmless, since `FindMostWorkChain()` lazily removes blocks without data from the set, so this does not fix a bug but just simplifies some code.
ACKs for top commit:
achow101:
ACK 69baddc910
Bortlesboat:
Concept ACK 69baddc910. Removing `TargetBlock()` correctly limits the snapshot-block special-case to cs2 where it's actually needed — the test's own "not intended" comment was the tell.
sedited:
ACK 69baddc910
fjahr:
ACK 69baddc910
stratospher:
ACK 69baddc.
Tree-SHA512: 8942fc422f1898369dd486e37da11758f2ebd4a488d092aa1637ef5bfb85766c4be9ad0718797fb2080f5e8d61383b2ee932bf2bc2f7abc2fb07fe3d72e070c3
25f69d970a release note (Pol Espinasa)
af629821cf test: add background validation test for getblockchaininfo (Pol Espinasa)
a3d6f32a39 rpc, log: add backgroundvalidation to getblockchaininfo (Pol Espinasa)
5b2e4c4a88 log: update progress calculations for background validation (Pol Espinasa)
Pull request description:
`getblockchaininfo` returns `verificationprogress=1` and `initialblockdownload=false` even if there's background validation.
This PR adds information about background validation to rpc `getblockchaininfo` in a similar way to `validationprogress` does.
If assume utxo was used the output of a "sync" node performing background validation:
```
$ ./build/bin/bitcoin-cli getblockchaininfo
...
"mediantime": 1756933740,
"verificationprogress": 1,
"initialblockdownload": false,
"backgroundvalidation": {
"snapshotheight": 880000,
"blocks": 527589,
"bestblockhash": "0000000000000000002326308420fa5ccd28a9155217f4d1896ab443d84148fa",
"mediantime": 1529076654,
"chainwork": "0000000000000000000000000000000000000000020c92fab9e5e1d8ed2d8dbc",
"verificationprogress": 0.2815790617966284
},
"chainwork": "0000000000000000000000000000000000000000df97866c410b0302954919d2",
"size_on_disk": 61198817285,
...
```
If assume utxo was not used the progress is hidden:
```
$ ./build/bin/bitcoin-cli getblockchaininfo
...
"mediantime": 1756245700,
"verificationprogress": 1,
"initialblockdownload": false,
"chainwork": "00000000000000000000000000000000000000000000000000000656d6bb052b",
"size_on_disk": 3964972194,
...
```
The PR also updates the way we estimate the verification progress returning a 100% on the snapshot block and not on the tip as we will stop doing background validation when reaching it.
ACKs for top commit:
fjahr:
ACK 25f69d970a
danielabrozzoni:
ACK 25f69d970a
achow101:
ACK 25f69d970a
sedited:
ACK 25f69d970a
Tree-SHA512: 5e5e08fd39af5f764962b862bc6d8257b0d2175fe920d4b79dc5105578fd4ebe08aee2fe9bfa5c9cad5d7610197a435ebaac0de23e7a5efa740dfea031a8a9d4
3136559923 doc: Note that generateblock does not collect transaction fees (HouseOfHufflepuff)
Pull request description:
## Summary
- Add a note to the `generateblock` RPC help text clarifying that transaction fees are not collected in the block reward
- This was suggested by maflcko in #31684 as a minimal doc fix
refs #31684
## Test plan
- [x] `./build/bin/test_bitcoin --run_test=rpc_tests` passes
- [x] `cmake --build build --target bitcoind` compiles cleanly
ACKs for top commit:
maflcko:
lgtm ACK 3136559923
sedited:
ACK 3136559923
Tree-SHA512: 173e6ac2a08c5101794a21bf29ec01af834fe2ef177b46be9f46b0c545b8888a9deb816caed868ff917105cbd97e57152682dcd4d4fe47dd92ac14a83bbf5d03
ad75b147b5 test: scale IPC mining wait timeouts by timeout_factor (Enoch Azariah)
e7a918b69a test: verify IPC error handling for invalid coinbase (Enoch Azariah)
63684d6922 test: move make_mining_ctx to ipc_util.py (Enoch Azariah)
4ada575d6c test: verify createNewBlock wakes promptly when tip advances (Enoch Azariah)
Pull request description:
This is a follow-up to implement a couple of test improvements discussed in recent IPC PRs and issues.
- adds a test to `interface_ipc_mining.py` to verify that `createNewBlock` wakes up immediately when the tip advances, rather than waiting for the cooldown timer to expire (https://github.com/bitcoin/bitcoin/pull/34184#discussion_r2842239399).
- moves `make_mining_ctx` into `ipc_util.py` so it can be reused across the IPC tests instead of duplicating the setup code (https://github.com/bitcoin/bitcoin/pull/34422#discussion_r2852445430).
- adds a test case to verify that providing an invalid coinbase to `submitSolution` returns a remote exception instead of crashing the node, closing the loop on the issue reported in #33341.
- scales IPC wait timeouts using the test suite's `timeout_factor` to prevent spurious failures in heavily loaded CI environments, capping extended waits to avoid test runner hangs (bitcoin-core/libmultiprocess#253 (comment)).
Closes#33341.
ACKs for top commit:
Sjors:
ACK ad75b147b5
achow101:
ACK ad75b147b5
sedited:
ACK ad75b147b5
Tree-SHA512: 812aa64c03657761f06707e6a15b8b435ab7c75717a6748a919fcbcae317128e18403b0c1bddd4cdad877d286e69db52389633e4012faaa656acc01939091719
353c660be5 bench: use deterministic `HexStr` payload (Lőrinc)
Pull request description:
### Context
Split out of https://github.com/bitcoin/bitcoin/pull/32554
Inspired by https://github.com/bitcoin/bitcoin/pull/32457#discussion_r2081323234
### Problem
`HexStrBench` uses the bytes from the embedded block fixture as a random source of bytes to measure `HexStr` performance against.
This coupling makes block benchmark migrations in #32554 slightly more work than necessary.
### Fix
We can use deterministic pseudo-random bytes instead so this benchmark keeps stable input without fixture coupling.
Use `MAX_BLOCK_WEIGHT` so the benchmark stays in the same size range and keeps measured work above harness overhead.
This changes the benchmark baseline because input size moves from about 1 MB to 4 MB.
ACKs for top commit:
maflcko:
lgtm ACK 353c660be5
sedited:
ACK 353c660be5
hodlinator:
ACK 353c660be5
Tree-SHA512: 5144b9b71761c581ff13c8f1163efed5e97f247f3c760dd7e513209c84d50f13253aa0d2ab3a431aaa51c204fea51bece41ac128b3d0e3a9ef02d1be11d6a6db
8d2f06853a sync: Use StdMutex for thread safety annotations (Anthony Towns)
cbc231ed8e scripted-diff: logging: Switch from StdLockGuard to STDLOCK (Anthony Towns)
f808786f48 logging: Add missing thread safety annotations (Anthony Towns)
e196cf26e0 util/stdmutex.h: Add STDLOCK() and improve annotation checking for StdMutex (Anthony Towns)
Pull request description:
Using `STDLOCK(mutex)` instead of `StdLockGuard guard(mutex)` allows clang to propagate missing lock annotations backwards (for global locks or locks in the same class, anyway), and also avoids declaring a dummy name. Use this in logging.h, and also use it in sync.cpp, adding annotations around the internal structure.
ACKs for top commit:
theuni:
ACK 8d2f06853a
sedited:
ACK 8d2f06853a
Tree-SHA512: ee23f6a7bcc62cc6d9ea88afa863a9018e53a0932272bb14241441fb69066c6633c4e19aadd3dd7599848d4742099d63756be4eff1969775293b728f3dd187aa
fa30951af5 test: Remove confusing assert_debug_log in wallet_reindex.py (MarcoFalke)
Pull request description:
The `wallet_reindex.py` test has many issues:
* It uses a `assert_debug_log` to ensure the reindex happened via `initload thread exit`. However, no other test uses this pattern, because `restart_node` already achieves the same internally (via `getmempoolinfo.loaded`).
* The `assert_debug_log` may intermittently fail, because the `initload thread exit` log may happen at any time after the inner thread function (lambda) is fully done.
* The test uses an `node.syncwithvalidationinterfacequeue()` without any explanation. No other test uses this pattern, because all wallet RPCs, in particular the next one (`gettransaction`) call it internally (via `BlockUntilSyncedToCurrentChain`).
Fix all issues by removing all confusing, useless, and brittle calls.
Example failure: https://github.com/bitcoin/bitcoin/actions/runs/22671604602/job/65716917459?pr=34419#step:11:22339
```
...
node0 2026-03-04T13:55:23.784715Z (mocktime: 2026-03-04T22:15:17Z) [scheduler] [validationinterface.cpp:185] [operator()] [validation] UpdatedBlockTip: new block hash=24b5cc4c1352bc8841ecbe67a43827acd1adc609bd26b2691e80e89b97eff135 fork block hash=24a4dc8be9c157ac31913397d8bb1653900e12b55539c234039559fdeb6dd2fb (in IBD=true)
node0 2026-03-04T13:55:23.784851Z (mocktime: 2026-03-04T22:15:17Z) [initload] [util/thread.cpp:22] [TraceThread] initload thread exit
test 2026-03-04T13:55:23.813824Z TestFramework (ERROR): Unexpected exception:
Traceback (most recent call last):
File "/home/admin/actions-runner/_work/_temp/test/functional/test_framework/test_framework.py", line 142, in main
self.run_test()
File "/home/admin/actions-runner/_work/_temp/build/test/functional/wallet_reindex.py", line 90, in run_test
self.birthtime_test(node, miner_wallet)
File "/home/admin/actions-runner/_work/_temp/build/test/functional/wallet_reindex.py", line 64, in birthtime_test
with node.assert_debug_log(expected_msgs=["initload thread exit"]):
File "/usr/lib/python3.12/contextlib.py", line 144, in __exit__
next(self.gen)
File "/home/admin/actions-runner/_work/_temp/test/functional/test_framework/test_node.py", line 607, in assert_debug_log
self._raise_assertion_error(f'Expected message(s) {remaining_expected!s} '
File "/home/admin/actions-runner/_work/_temp/test/functional/test_framework/test_node.py", line 241, in _raise_assertion_error
raise AssertionError(self._node_msg(msg))
AssertionError: [node 0] Expected message(s) ['initload thread exit'] not found in log:
```
Diff to reproduce failure:
```diff
diff --git a/src/util/thread.cpp b/src/util/thread.cpp
index 0fde73c4e2..de4c05e752 100644
--- a/src/util/thread.cpp
+++ b/src/util/thread.cpp
@@ -8,2 +8,3 @@
#include <util/log.h>
+#include <util/time.h>
#include <util/threadnames.h>
@@ -21,2 +22,3 @@ void util::TraceThread(std::string_view thread_name, std::function<void()> threa
thread_func();
+ UninterruptibleSleep(999ms);
LogInfo("%s thread exit", thread_name);
diff --git a/test/functional/wallet_reindex.py b/test/functional/wallet_reindex.py
index 71ab69e01b..649df4301a 100755
--- a/test/functional/wallet_reindex.py
+++ b/test/functional/wallet_reindex.py
@@ -62,2 +62,3 @@ class WalletReindexTest(BitcoinTestFramework):
+ import time; time.sleep(1) # Wait for previous initload thread to exit fully
# Reindex and wait for it to finish
```
Before on master: Test fails
After on this pull: Test passes
ACKs for top commit:
rkrux:
ACK fa30951af5 if this PR needs to be backported.
Tree-SHA512: 1e6599511e09b5b9ac4aed96a6b1b8239d5dc63b164fc0c163b6f9f5bc72c1c61f72ad8256a01875208d825af46d057c4d9de61758002f256388ddb324006bb5
