87b080fe2b fuzz: reset the reused mempool in process_message(s) (Hao Xu)
d522fd3196 fuzz: prepare deterministic mempool rebuilds (Hao Xu)
b11456386b fuzz: let the test input toggle IBD in the p2p fuzz targets (Hao Xu)
2a29cee684 test: add helper to reset chainman and mempool (Hao Xu)
2a4ef42d34 fuzz: share a single FakeNodeClock in the chainman-resetting fuzz targets (Hao Xu)
Pull request description:
## Problem
`process_message` and `process_messages` keep the node in IBD (`ResetIbd()`) and
mine their coinbases with the default bare-`OP_TRUE` output script. As a result
`net_processing` returns early at the `IsInitialBlockDownload()` check and never
reaches the transaction-handling path; and even if it did, a tx spending a
bare-`OP_TRUE` coinbase is rejected as `NONSTANDARD` by
`ValidateInputsStandardness`. The reused mempool therefore always stays empty and
that path is never exercised.
## Changes
Both targets now get the same treatment:
1. **Toggle IBD from the test input** — a `bool` decides whether to also
`JumpOutOfIbd()`, exercising both the IBD and non-IBD paths. In
`process_message` it is consumed last, so existing corpus entries read `false`
and are unchanged. In `process_messages` the messages run in a loop, so the
bool must be consumed *first* (see the corpus note below).
2. **Use a spendable `P2WSH_OP_TRUE` coinbase** — both anyone-can-spend (an
`OP_TRUE` witness, no signature) and a standard witness output, so a fuzz-built
tx spending a mature coinbase can actually be accepted into the mempool.
3. **Reset the rng before rebuilding (preparation)** — rebuilding the chainman
(and, in the next commit, the mempool) consumes the global PRNG. Reset it with
`MakeRandDeterministicDANGEROUS()` first so the rebuild is deterministic across
iterations. Mirrors the `cmpctblock` harness.
4. **Reset the reused mempool** — now that the mempool can become non-empty,
rebuild it together with the chainman in `ResetChainmanAndMempool()` when the
block index grew or the mempool changed. A dirty mempool is detected by its
sequence number rather than its size, since a tx can be added and removed
within one iteration (leaving the size unchanged).
## Corpus note
~~In `process_messages` the IBD bool is consumed before the message loop (first
integral read), which shifts the `FuzzedDataProvider` layout. Existing
`process_messages` corpus entries can be migrated by appending a single `0x00`
byte at the end (read as `false`, keeping the IBD path); every other consumed
value stays the same. This is a qa-assets change accompanying this PR.~~
This note no longer applies because the IBD toggle is now consumed inside the
message loop. Appending a single `0x00` byte would not reliably target that bool
or preserve the rest of the input layout.
The accompanying `qa-assets` update should migrate or regenerate the affected
`process_messages` corpus entries for the current layout.
ACKs for top commit:
Crypt-iQ:
crACK 87b080fe2b
maflcko:
review ACK 87b080fe2b🏁
frankomosh:
Review ACK 87b080fe2b
Tree-SHA512: e557b2ca3329767a45fe8315c63df9c3191a3a46a17c5e75ea3e4ad0c25e0e500a687fa650297a386b0a2ebb95503d069089ca5ae3d0a34caab98367aeb28683
fae7ba9aba ci: Fix $BASE_ROOT_DIR installation (MarcoFalke)
Pull request description:
In the ci system, `BASE_ROOT_DIR` has a default value that can be changed. This has problems:
* The docs do not mention that changing the value requires re-building the image, as the value is embedded.
* Many places hard-code the default value, which is confusing and brittle.
Fix all issues by adding docs and replacing the hard-coded default values with `$BASE_ROOT_DIR`.
ACKs for top commit:
willcl-ark:
ACK fae7ba9aba
Tree-SHA512: 6bdfc7acf46962f83664b9a4996b65bddad2f3046c19654a83b89c63a7ac8ac3ab3396f03240d42b091f3eda00b9da50bb49a2d19ded37b0de840a20a7c7e1d9
fa2e76d397 bench: Add base_blob compare bench via uint256 (MarcoFalke)
fa588e9e0f refactor: Mark assertion_fail as [[noreturn]] (MarcoFalke)
faec059dfe refactor: Add uint256::operator<=>() (MarcoFalke)
fa6df14c23 refactor: uint256::operator==() = default (MarcoFalke)
Pull request description:
Some refactors with rationale:
* Default the `uint256` base blob equals operator, because this is standard C++20 practise.
* Add the `uint256` base blob `<=>` operator, because this is standard C++20 practise. Also, `transaction_identifier` already offers such an operator. This allows to remove the non-standard `Compare()` function.
* Add a `[[noreturn]]` to the assertion failure helper that does not return. This is standard C++11 practise.
ACKs for top commit:
optout21:
ACK fa2e76d397
Sjors:
ACK fa2e76d397
purpleKarrot:
ACK fa2e76d397
hebasto:
re-ACK fa2e76d397.
w0xlt:
ACK fa2e76d397 as a simplification/refactor, not as a performance optimization.
Tree-SHA512: 2e64e09fabe71d84c05ed943664417dd1bc1cf6fe6bf31006a58dd9180c458f69915ba7770501537678f1ad51cce16018f79bdf6780e072b73b692ea44b60185
264555af3c rpc: avoid descriptor range counter overflow (Lőrinc)
143a13fb2b test: characterize descriptor range endpoint (Lőrinc)
Pull request description:
**Problem:** The authenticated `scantxoutset`, `scanblocks`, `getdescriptoractivity`, `utxoupdatepsbt`, and `descriptorprocesspsbt` RPCs share a descriptor expansion helper that iterates inclusive `int64_t` ranges with an `int` counter.
A ranged descriptor with an explicit `[begin, end]` range ending at `2^31 - 1` expands that valid position, then overflows when advancing the counter to exit the loop.
Trap-enabled builds terminate, while other builds invoke undefined behavior.
**Fix:** Use `int64_t` for loop control so the one-past-the-end value is representable and every position passed to `Descriptor::Expand()` remains within its existing `int` range.
Related: [#26275](https://github.com/bitcoin/bitcoin/pull/26275) fixed the same endpoint overflow in `deriveaddresses`.
ACKs for top commit:
achow101:
ACK 264555af3c
polespinasa:
ACK 264555af3c
sedited:
ACK 264555af3c
Tree-SHA512: 4326182b5897b6f6672e5f7c7296eafdbb6e3b5ed901d61e8fa2cff9b19d372bb8adb88902368dc520ed400e12dd5264ea68677d9ce0feec76fa2ef55fa0d2f4
fac4b06e99 refactor: Use CLIENT_NAME in buildOpenRPCDoc (MarcoFalke)
fa3aadbc32 refactor: Use self.Arg<bool> in getopenrpcinfo (MarcoFalke)
fa1871a528 refactor: Remove stale NOLINTNEXTLINE above GetAddressInfoBaseFields (MarcoFalke)
fa22647914 rpc: Properly make RPCResult::Type::ANY non-test-only (MarcoFalke)
fa1242dcc0 refactor: Use std::visit in ApplyArgFallback (MarcoFalke)
Pull request description:
Commit 6a1a66c180 attempted to properly render RPC results of the type `ANY`.
However, the commit is incomplete.
Fix it, by properly rendering all `ANY` types.
Moreover, a few trivial refactors after https://github.com/bitcoin/bitcoin/pull/34683 are included here.
ACKs for top commit:
sedited:
ACK fac4b06e99
willcl-ark:
ACK fac4b06e99
Tree-SHA512: b77f1956e4feb9afb93d38245491eec190c4538aec7ac57c327fb2efd86c7da6ee8a1406f81fc18fac06bae77a7d38845a537ca0b8f4ae2d94c38eefd8e88dc1
7502b9ddba fuzz: check http_request body matches framing (ameen-alam)
Pull request description:
The http_request target asserted that ReadBody() returns an empty string. That held for the libevent-based http_libevent::HTTPRequest, where the harness only parsed the request line and headers and never populated a body. Commit 9c20859b5f (PR #35182) replaced libevent with http_bitcoin::HTTPRequest, and the target was switched over in e427c227fa; its LoadBody() now decodes Content-Length and chunked bodies per RFC 9112, so any fully-parsed request carrying a body trips the stale assertion (e.g. "POST / HTTP/1.1\r\nContent-Length: 3\r\n\r\nabc").
Replace the emptiness check with a framing-consistency check that mirrors LoadBody()'s own branch logic: a chunked body is bounded by MAX_BODY_SIZE, a Content-Length body is exactly that many bytes, and a request with neither framing header has no body. This strengthens the target instead of dropping the assertion.
**Steps to reproduce (old assertion):**
Build the fuzz binary and pass this input as a file to the `http_request` target:
`POST / HTTP/1.1\r\nContent-Length: 3\r\n\r\nabc`
→ `test/fuzz/http_request.cpp:49: Assertion 'body.empty()' failed`
**Testing the fix:**
Ran the updated target ~16 min under libFuzzer with ASAN/UBSAN
(14.2M execs, no crashes), plus targeted inputs for each branch:
Content-Length body, chunked, `Transfer-Encoding: identity` + Content-Length,
no framing headers, and `Content-Length: 0`. Happy to contribute the repro
input to qa-assets as a follow-up.
ACKs for top commit:
pinheadmz:
ACK 7502b9ddba
marcofleon:
tACK 7502b9ddba
Tree-SHA512: 4f2eb6bdb3a4556866a84fe0f1d0d8cf506e2efd1b1c7493a99f67ca452b31a140034d418c4064142b1a66c3a6c34b97df0e2b12c21ea86cd4019ffc7cff3b27
1278a5970d net_processing: process unique tx INVs only (Gregory Sanders)
Pull request description:
There is no reason we should process conflicting
advertisements for transactions, as they cannot be both accepted into our mempool.
Avoid processing these and doing spurious work.
Should be no change in observable behavior.
ACKs for top commit:
ajtowns:
ACK 1278a5970d
fjahr:
ACK 1278a5970d
l0rinc:
ACK 1278a5970d
Tree-SHA512: c62ceed2cc634c8c99509a8495e5f9bb6d4d8d050942f709a6539ae4dfe1ec628ce7ab0ec1392656809e0d827d1fecf2e8fb9bb4600d13a5bf9a34c9e9e3ad6e
e98ffd4bd8 doc: fix stale bitcoin_en.xlf reference (cyb3ralbert)
Pull request description:
`doc/release-process.md` still instructs users to create the Transifex resource from `src/qt/locale/bitcoin_en.xlf`, even though that file no longer exists.
It was removed in #34808, which switched the Transifex source to the native Qt `.ts` file. That PR updated the other references to `.xlf` in this document, but this one was missed. The last step in the same list already refers to `bitcoin_en.ts`, as does `.tx/config`.
This patch updates the remaining outdated reference.
Docs-only change. No tests run.
ACKs for top commit:
hebasto:
ACK e98ffd4bd8. This was overlooked in https://github.com/bitcoin/bitcoin/pull/34808.
Tree-SHA512: fc6a295aae8a58e72e3324f52849255fe5c49bc53af71733de44489dd8a5dea86e3dfeaaf01cb47c506e717019a390de97a71505babcb1c98626f5c2fd98274f
fa7bc26d12 test: Check that RPCs do not time out, even under load (MarcoFalke)
fa2bd96cc0 test: Map cli CalledProcessError on server error to JSONRPCException (MarcoFalke)
Pull request description:
It turns out there is no test currently to check that the RPC server does not time out under load. With "load" I mean a flood of trivial payloads. That is, the only work needed is JSON encoding and decoding of (let's say) a block of data of 2 MB or so. This may take a few milliseconds, but should never take more than a few seconds.
So add a test for this.
ACKs for top commit:
enirox001:
ACK fa7bc26d12
sedited:
ACK fa7bc26d12
Tree-SHA512: c60646981b7449c757e9fad499e1cd71030376ffb2ae687c8136c6f70accd0a2d76a4cbfc7dd1bc6626ea3a5310a33616a36cd682cc5c4371ec86aa2c8641aeb
81fcecfe45 Revert "ci: Temporarily remove riscv32 config from GHA matrix" (will)
b283e1751c ci: use mirror for riscv submodules (will)
Pull request description:
The https transport is rate-limited to block AI scrapers.
Switch to a live mirror on fish.foo to re-enable the riscv job.
ACKs for top commit:
maflcko:
lgtm ACK 81fcecfe45
sedited:
ACK 81fcecfe45
Tree-SHA512: 1891046d9847b904a3e4be25b7f313fc2413ad1cc0b2c09b2300f81a2f1f691dad816fa5fcfeeae263f8d5f868d229aca5a7ab3f227ce650d359068488bc1e21
sourceware is blocking/rate-limiting http(s) git requests to combat AI
scrapers.
Switch to a live mirror hosted at fish.foo
Use exported GIT_CONFIG_* to avoid setting `git config --global` which
could clobber a user running on bare host, but still propagate to child
git processes spawned by `make`.
fa9a9a82ac refactor: Remove unused newFeeRate var in ReplacementChecks (MarcoFalke)
Pull request description:
This is unused since cluster-mempool, so it seems confusing to keep around. See the commit that forgot to remove it:
```sh
$ git show 216e693729 -U99999 | grep newFeeRate
CFeeRate newFeeRate(ws.m_modified_fees, ws.m_vsize);
- if (const auto err_string{PaysMoreThanConflicts(ws.m_iters_conflicting, newFeeRate, hash)}) {
```
ACKs for top commit:
sedited:
ACK fa9a9a82ac
Tree-SHA512: f8804109c59dfe37da754cb1924cc16abbc792fbb3619249a1edbc005f66fd88357c4810926625a375415c5be4ff056d68af3029f8075590252289c1d753a2fd
This is unused since cluster-mempool. See the commit that forgot to
remove it:
```sh
$ git show 216e693729 -U99999 | grep newFeeRate
CFeeRate newFeeRate(ws.m_modified_fees, ws.m_vsize);
- if (const auto err_string{PaysMoreThanConflicts(ws.m_iters_conflicting, newFeeRate, hash)}) {
```
There is no reason we should process conflicting
advertisements for transactions, as they cannot be both
accepted into our mempool.
Avoid processing these and doing spurious work.
There is already a non-standard and internally used Compare() function,
and an standard operator<().
Also, there is already transaction_identifier::operator<=>().
It seems more consistent to remove the internal Compare() and have a
single standard C++20 <=> operator.
3ac8b806a6 test: test the result order of a multiple import request is correct (Pol Espinasa)
e4732bf018 test: test invalid or missing timestamp throws importdescriptors (Pol Espinasa)
07fb58b9ef test: Test a locked wallet rejects an empty importdescriptors request (Pol Espinasa)
Pull request description:
In addition to #35179 (already merged) this adds more missing test coverage that was detected while rebasing #34861.
The three tests added checks:
- Locked wallet throws because of being locked if giving an empty importdescriptors request.
- Invalid or missing timestamp throws as a top level RPC error and not a per-item error.
- The order of the requests and the response is the same, even if failing or succeeding.
ACKs for top commit:
nebula-21:
ACK 3ac8b806a6
Bicaru20:
re-ACK 3ac8b806a6
brunoerg:
reACK 3ac8b806a6
Tree-SHA512: b6ba9e16bbdbefcab2529f49f9aab0ae8885bd2d381c6eec36ae442dea1aa2361e6fb339ab5bc2c51c3bef6216d8d939db53e57ec577f05fe54c07fc46f8f255
c9cedebfff coins: group private cache helpers (Lőrinc)
Pull request description:
**Problem:** `CCoinsViewCache::ReallocateCache()` is public even though only `Flush()` uses it.
The private helper declarations are also split between the start and end of the class.
**Fix:** Move `ReallocateCache()` and `FetchCoin()` into the existing private section.
ACKs for top commit:
optout21:
reACK c9cedebfff
sedited:
ACK c9cedebfff
Tree-SHA512: d06062daa9976d767375cd272b3c4ddd838e83fc8b3ac57f6b89a4d8d8837277211786cdd66a794b1a84a39cf6b38f2b9a01699d16e0a648d27b1b93cd0f33b5
aeca061086 rpc: reject null for optional parameters (Ruslan Kasheparov)
Pull request description:
Treat explicitly passed `null` as missing for optional RPC parameters that are required in certain contexts.
ACKs for top commit:
achow101:
ACK aeca061086
maflcko:
review ACK aeca061086🥚
sedited:
ACK aeca061086
Tree-SHA512: 60f146085fd20e532ba3cbefdb76d430938168621706a20b2b62a34318499fd72a8c934b08f690f9b72d19ed26581094517a0986586f95bc4b23fa8743b24d11
21b4b790e4 test: Move cluster_linearize.h contents into cluster_linearize namespace (Hennadii Stepanov)
Pull request description:
Clang recently enabled `-Wunused-template` under `-Wall` (see https://github.com/llvm/llvm-project/pull/206123, https://github.com/llvm/llvm-project/pull/207848, https://github.com/llvm/llvm-project/pull/208001). Our codebase [triggers](https://my.cdash.org/builds/3714664/build) some of these warnings.
This PR:
1. Avoids Clang's `-Wunused-template` warnings in `src/test/util/cluster_linearize.h` when building the `bench_bitcoin` and `fuzz` targets.
2. Follows the C++ Core Guidelines: "[SF.21: Don't use an unnamed (anonymous) namespace in a header](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#sf21-dont-use-an-unnamed-anonymous-namespace-in-a-header)".
3. Drops `[[maybe_unused]]` annotations, which are no longer needed after changing the linkage from internal to external.
Along with https://github.com/bitcoin/bitcoin/pull/35679, this resolves all instances of this warning in the test/bench/fuzz code.
Another related change: https://github.com/bitcoin-core/minisketch/pull/102.
---
Steps to reproduce on the master branch @ 70d9ec7f3d:
```console
$ CXXFLAGS="-Wunused-template" cmake -B build -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DBUILD_BENCH=ON
$ cmake --build build -t test_bitcoin
$ cmake --build build -t bench_bitcoin
[19/62] Building CXX object src/bench/CMakeFiles/bench_bitcoin.dir/cluster_linearize.cpp.o
In file included from /home/hebasto/dev/bitcoin/src/bench/cluster_linearize.cpp:9:
/home/hebasto/dev/bitcoin/src/test/util/cluster_linearize.h:122:17: warning: unused function template 'Ser' [-Wunused-template]
122 | static void Ser(Stream& s, const DepGraph<SetType>& depgraph)
| ^~~
/home/hebasto/dev/bitcoin/src/test/util/cluster_linearize.h:286:6: warning: unused function template 'SanityCheck' [-Wunused-template]
286 | void SanityCheck(const DepGraph<SetType>& depgraph)
| ^~~~~~~~~~~
/home/hebasto/dev/bitcoin/src/test/util/cluster_linearize.h:383:6: warning: unused function template 'SanityCheck' [-Wunused-template]
383 | void SanityCheck(const DepGraph<SetType>& depgraph, std::span<const DepGraphIndex> linearization)
| ^~~~~~~~~~~
3 warnings generated.
[62/62] Linking CXX executable bin/bench_bitcoin
```
ACKs for top commit:
maflcko:
lgtm ACK 21b4b790e4
sedited:
ACK 21b4b790e4
Tree-SHA512: 893082c2dc3295b68738c438094656b3568f85b95e0e2f9b0b9dd178d0613e4cb50cd2300a9595e390b9d0a4c650ca78d2772dd1010c178756cefbeef6e2ed94
This is clearer, because it does not encode the default value in the
source code syntax. Also, using named args is clearer than positional
args.
Also, run clang-format on the new code.
This will render all ANY results.
However, the one in the help RPC should still be hidden. This is done by
applying HelpElisionSkip and also skipping over empty result sections in
ToDescriptionString.
Also, run clang-format. Can be reviewed via --ignore-all-space
ddddffda3a doc: Add doc/release-notes-35836.md (MarcoFalke)
fa7fe798c6 wallet: Remove meaningless bool fallback in FundTransaction (MarcoFalke)
Pull request description:
This mostly removes a no-op and meaningless bool fallback in the `fundrawtransaction` RPC.
This allows to remove a `skip_type_check`. This makes validating the JSON schema from https://github.com/bitcoin/bitcoin/pull/34683 more consistent.
Adding the type check here is useful, because:
* The fallback was added in af4fe7fd12 (more than a decade ago). Retaining backwards compat with more than 10-year old clients seems purely theoretical. There were other breaking RPC changes on shorter notice in the meantime. If someone really forgot to update this over the last 10 years, I don't see a downside of notifying them.
* The compat only works for positional args, which seems another small reason to drop it.
* The compat is now fully irrelevant and a no-op, given that watch-only wallet is not a concept
anymore after commit 1337c72198.
* Keeping the compat means that openrpc spec users do not get any type checks at all here.
ACKs for top commit:
polespinasa:
ACK ddddffda3a
sedited:
ACK ddddffda3a
Tree-SHA512: aa5113bd74159ae5a3bf189edc48d011761db98beaf701cd144ba94eac9f525bec2b8eeb53e588b2f20dc9965ebabef2c5ce55a732b8cef2874a95f398ecae8f
13b53f8bf6 iwyu: Fix warnings in `src/consensus` and treat them as errors (Hennadii Stepanov)
Pull request description:
This PR continues the ongoing effort to enforce IWYU warnings.
See [Developer Notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#using-iwyu).
ACKs for top commit:
maflcko:
review ACK 13b53f8bf6 🖐
sedited:
ACK 13b53f8bf6
Tree-SHA512: c5ec24aec2b618abe9b2046b8ffafa4f3eb61edf8fc9c73a2b6f4e6858797ec214d9cd28c546fbfae1a8f3324d7d6f1c9b6afd4050a1e7fd6075217f7abdfd32
