01dde6b205 fuzz: Fix assertion in txorphan (marcofleon)
Pull request description:
`EraseTx()` calls `LimitOrphans()`, which may evict announcements from a peer that didn't announce the erased transaction, causing that peer's usage to decrease. Relax the assertion in the `EraseTx()` branch that claimed usage of a non-announcer peer should be unchanged. Also, add assertions for the other cases.
ACKs for top commit:
dergoegge:
utACK 01dde6b205
instagibbs:
ACK 01dde6b205
Tree-SHA512: 2e597b85fd41058c2fa79fa55f0d37e12505065b5e27aba7b9680e0c249a5450e6fa97b45394d6ffe1318f42538134ffa9c423b126c455f6f8e6d8ca59eed4b6
9954aa7728 http: don't parse any new requests from a client if m_req_busy = true (Matthew Zipkin)
c7db3ae1f9 test: cover HTTPRequest state machine (Matthew Zipkin)
90676e24ad Add state to HTTPRequest to avoid duplicate work over I/O cycles (Matthew Zipkin)
507e528e84 http: reuse HTTPHeaders to parse chunked trailer (Matthew Zipkin)
902d8908c9 http: only read one HTTPRequest at a time per client (Matthew Zipkin)
Pull request description:
This PR reduces the memory consumption of the HTTP Server when reading data from connected clients, and improves performance especially when requests are large (i.e. requiring multiple TCP packets).
In https://github.com/bitcoin/bitcoin/pull/35182 the server copies as much data as it can from the socket into application memory, and then tries to parse as many complete HTTP requests as possible from that data. If a request is discovered to be incomplete, the in-progress request is abandoned. The server tries again on the next I/O cycle to read the same data from the buffer, duplicating work as many times as it takes before the client finishes sending the request (or times out).
This PR implements two improvements to this:
1. Only parse one request at a time from the receive buffer. The server processes requests from each client in series anyway.
2. Add state to `HTTPRequest` so it can be filled with data from the receive buffer over multiple I/O loop iterations without losing progress.
If a client sends large or multiple requests, that data will sit in the kernel's socket buffer instead of the application memory. Eventually the socket buffer will fill up and TCP backpressure will kick in, dropping the TCP window to 0 and blocking the client from sending any more.
A state machine for `HTTPRemoteClient` was [discussed previously](https://github.com/bitcoin/bitcoin/pull/35182#pullrequestreview-4322490068) to control resource consumption. Another nice benefit of this model (for a follow-up PR) will be to insert the RPC authentication check after reading 8kB-limited headers but before the 32MB-limited request body.
ACKs for top commit:
winterrdog:
re-ACK 9954aa7728
janb84:
re ACK 9954aa7728
frankomosh:
ACK 9954aa7728.
fjahr:
ACK 9954aa7728
Tree-SHA512: b7c913114283fbf1f360b40f6c65a01390a26731bf3b166f460ec260f9206f25d738b3a06887bfa839911c1c6aaf634448181da47a752a9a881aebd907e44868
25bed560be test: add forward-compat functional test for txindex (sedited)
703304ed8c doc: add release notes for txindex disk usage and downgrading (Andrew Toth)
8e5320a2d2 tests: cover txindex hash prefix collisions and legacy fallback (Andrew Toth)
b75efa19ba txindex: skip bloom filters and legacy lookups for new databases (Andrew Toth)
004d7c098c txindex: hash key prefixes and pack block positions (Andrew Toth)
5a255970fd refactor: move txindex db constants and legacy key to txindex_key.h (Andrew Toth)
327660134c txindex: pass the full block to DB::WriteTxs (Andrew Toth)
42771e7998 txindex: use a new block locator for downgrade safety (Andrew Toth)
4b08baed72 txindex: return optional tx and block hash from FindTx (Andrew Toth)
Pull request description:
The current txindex uses the full 32-byte txid as keys, which takes up about 66 GB of disk space today on mainnet. Using a 5-byte key prefix instead drops the disk usage to 26 GB - cutting the size to less than half.
Using the full 32-bytes is unnecessary since a 5-byte salted siphash will produce collisions in about 1 in 1.1 trillion. Some collisions will occur, but the penalty is just an extra disk read, deserialization and hash.
The tx position can be appended to the key instead of used as a value, and a LevelDB iterator can seek to the prefix and then scan for the correct tx. This is an almost identical approach to `txospenderindex`.
Also instead of storing the file position of the block, we can store only the sequence of the connected block and offset of the transaction in the block. This can be packed into a 6-byte key suffix using 3-byte representations of the sequence and offset in the block. The block file can be recovered by the CBlockIndex that is already in memory. The sequence is mapped to the block hash in the db, so we can lookup the block hash to find the CBlockIndex during reads.
If a tx is not found with this method, we fallback to looking up the legacy entry. With this method a user with an existing db can opt to erase the `indexes/txindex` folder and reindex, or keep the current index and new entries will be appended with the smaller footprint.
The time to index was faster on my machine with this method, 1h19m vs current 1h50m.
Lookups are roughly the same, around 0.2ms per lookup with `getrawtransaction`.
When testing on mainnet, I got 894,549 2-way collisions, 395 3-way collision, and 1 4-way collision that worst case could cause an extra 3 false positives when reading.
ACKs for top commit:
l0rinc:
diff reACK 25bed560be
sedited:
ACK 25bed560be
ajtowns:
ACK 25bed560be
Tree-SHA512: a25c79ca7e722e2f372b65f5fc11c8b194ad49f2240b4881c7e606306aabbd3604aede3f1c33606b467486affac3a3f503638f513c896935cebbc02709cb60d8
c3945bfd2b doc: use derivehdkey in multisig tutorial (Sjors Provoost)
3662e33669 test: use derivehdkey in M-of-N multisig demo (Sjors Provoost)
d9570f0838 rpc: add derivehdkey (Sjors Provoost)
62da9f9614 wallet: add GetExtKey helper (Sjors Provoost)
aaf1548475 wallet: generalize GetActiveHDPubKeys helper (Sjors Provoost)
3821452c4a refactor: add hardened derivation helper (Sjors Provoost)
0ab61caafd rpc: ParsePathBIP32 helper (Sjors Provoost)
e36c4b76e1 util: reject out-of-range BIP32 keypath indices (Sjors Provoost)
ba78c31a00 fuzz: check ParseHDKeypath/WriteHDKeypath round-trip (Sjors Provoost)
8cce969085 Have ParseHDKeypath handle h derivation marker (Sjors Provoost)
fc53077762 test: move parse_hd_keypath test to bip32_tests (Sjors Provoost)
dab525eb77 key: add DeriveExtKey() helper (Sjors Provoost)
Pull request description:
Adds a `derivehdkey` RPC that returns an xpub, or optionally the xprv, at an arbitrary BIP32 path (with at least one hardened step), derived from a wallet HD key.
The main use case is coordinating a multisig setup, where each participant shares an xpub derived at a hardened path (e.g. `m/87h/0h/0h`) distinct from their default single-signature descriptors. See the (updated) `doc/multisig-tutorial.md` and (updated) functional test to see how that workflow improves.
The first commits are some helpful helpers:
- _key: add DeriveExtKey() helper_ - performs the actual derivation
- _test: move parse_hd_keypath test to bip32_tests_ - from `psbt_wallet_tests`
- _Have ParseHDKeypath handle h derivation marker_
- _util: reject out-of-range BIP32 keypath indices_ - `ParseHDKeypath` would previously map overflowing values without `h` to hardened.
- _fuzz: check ParseHDKeypath/WriteHDKeypath round-trip_
- _rpc: ParsePathBIP32 helper_
- _refactor: add hardened derivation helper_ - `HasHardenedDerivation()`, to enforce the "at least one hardened step" rule
- _wallet: generalize GetActiveHDPubKeys helper_ - extracts code from `gethdkeys` which `derivehdkey` needs
- _wallet: add GetExtKey helper_ - reconstruct an xprv from a wallet xpub (analog of `GetKey()`); behavior-preserving prep, also simplifies `gethdkeys`.
Meat and potatoes:
- _rpc: add derivehdkey_ - the RPC itself, plus the `UnusedKey` filter on `GetHDPubKeys` that drives key selection.
- _test: use derivehdkey in M-of-N multisig demo_ - rewrites the functional multisig test to use the RPC and `<0;1>` syntax.
- _doc: use derivehdkey in multisig tutorial_ - same for the prose tutorial.
ACKs for top commit:
pseudoramdom:
code review ACK c3945bfd2b
achow101:
ACK c3945bfd2b
w0xlt:
That being the case, ACK c3945bfd2b
Tree-SHA512: 661f17c9bfe26017eb14c27ba7af37093387100d3baa25f5d29bba9c1aedc40d19afe1bdfc126a18d018857bb02f1fc84386f10b8f4f4b8e9d6f4b0691d9e302
EraseTx calls LimitOrphans, which may evict announcements from a peer
that didn't announce the erased transaction, causing that peer's usage
to decrease. Relax the assertion in the EraseTx branch that claimed
usage of a non-announcer peer is unchanged. Also, add assertions for
the other cases.
fab74a0e92 refactor: Use C++14 digit separator for large int literals (MarcoFalke)
fae759be79 scripted-diff: Use inline constexpr over plain constexpr (MarcoFalke)
fa74f58a26 scripted-diff: Use inline const over (static) const (MarcoFalke)
fab1a62c87 refactor: Use inline constexpr for string literals in headers (MarcoFalke)
fa08bbed8d contrib: Adjust generate-seeds.py to write inline constexpr (MarcoFalke)
fad753611b scripted-diff: Use inline constexpr over (static) const (MarcoFalke)
faedb52583 refactor: Make CFeeRate(integral) ctor constexpr (MarcoFalke)
5555d5dcb5 scripted-diff: Use inline constexpr over static constexpr (MarcoFalke)
fa6e1a1e85 refactor: Remove static from constexpr functions in headers (MarcoFalke)
Pull request description:
Both are fine and this refactor doesn't change any behavior.
However, `inline constexpr` from C++17 will ensure each symbol has a single address
across all TU, making the release binary minimally smaller. (For me it is smaller by about 1kB)
ACKs for top commit:
l0rinc:
reACK fab74a0e92
rustaceanrob:
ACK fab74a0e92
hebasto:
ACK fab74a0e92, I have reviewed the code and it looks OK.
Tree-SHA512: 6ec94136c12bcbf696812d0661c9857318a69e367c79fc00b9ca0b4068f269d10e5548d95c9ba2070225308c12d7a54fe8cb8447de7e0979cba99f48892b35f9
8b5da677d7 common: remove ::runtime_error from RunCommandParseJSON (fanquake)
Pull request description:
I don't think there's a code path that can reach `RunCommandParseJSON` if we compile with `ENABLE_EXTERNAL_SIGNER=OFF`. If there is a reason for having the code this way, it could be good to document.
This also requires more workarounds in #35911.
ACKs for top commit:
stickies-v:
re-ACK 8b5da677d7
sedited:
ACK 8b5da677d7
willcl-ark:
ACK 8b5da677d7
Tree-SHA512: b0c50372fed35afe47713310851f0b58cd1803fbe87a3a5a75877772172a3881283394a91663251de22a9972f56b46d84ddc868686dec8b970474cfaf5dc0d32
34c03075a5 test: run generic baseindex tests against every index type (Martin Zumsande)
11b3e251c4 test: make baseindex flush test chain-length agnostic (Martin Zumsande)
8b959f4c6a test: move unclean_shutdown test to baseindex_tests (Martin Zumsande)
2232d6afbe test: move index_reorg_crash to baseindex_tests (Martin Zumsande)
a3597e2683 test: move BuildChain helper into test mining util (Martin Zumsande)
954985e6a3 test: simplify blockfilter test's BuildChain helper (Martin Zumsande)
Pull request description:
In #34897, the `baseindex_tests` unit test was introduced, meant for tests that test basic index functionality (e.g. reorg or unclean shutdown behavior) that should work regardless of the particular index type.
This PR moves two more of these tests (`index_reorg_crash`, `coinstatsindex_unclean_shutdown`) from test files of specific indexes into that folder.
In the second part, tests are executed sequentially for all index types instead of just one particular one, where applicable.
Before moving `index_reorg_crash` I extracted the `BuildChain` helper to `util/mining` so that it can be used by multiple tests. While doing that, I simplified the helper a bit.
ACKs for top commit:
jeanpablojp:
tACK 34c03075a5
sedited:
ACK 34c03075a5
Tree-SHA512: 1d7a43160a9b7ec3c75a8c806967f2031da4855fe449c9c8aac8e44b1940e5ee28fde9473406666e74a682cf135b87f8c0eb9ddba50fce156dce9fc54a7763eb
I don't think there's a code path that can reach RunCommandParseJSON if
we compile with `-DENABLE_EXTERNAL_SIGNER=OFF`. This also requires more
workarounds in #35911.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
d055a3ab10 test: verify disallowed RPC clients are rejected upon `accept()` (winterrdog)
Pull request description:
this is a follow-up PR from a suggestion in this [comment](https://github.com/bitcoin/bitcoin/pull/35592#pullrequestreview-4823271031)
it adds a unit test that confirms that clients not permitted by
`-rpcallowip` are rejected immediately after `accept()`, before any
request bytes are read from the socket
specifically, the test checks that: no request is ever dispatched to the
server's request handler, the connection is closed without any response
to the client, no `HTTPRemoteClient` is ever registered for it, and the
client's request bytes are left completely unread in the socket's
receive buffer
ACKs for top commit:
achow101:
ACK d055a3ab10
pinheadmz:
ACK d055a3ab10
w0xlt:
reACK d055a3ab10
Tree-SHA512: 5b99eb8a795e302333549f3ea5c76118c380402e2ebac8256db0a06ac0b4c0739b0b94471aa2f278214e65574e84f321b416e2543b361bf4fd8eb6854768377a
156f2c6c49 kernel: add `btck_set_mock_time` for testing time-dependent paths (stringintech)
Pull request description:
Some kernel paths read the current time (e.g. header validation's future-time check, and the `btck_SynchronizationState` carried by `btck_NotifyBlockTip` / `btck_NotifyHeaderTip` callbacks), which makes them awkward to exercise deterministically in tests. This PR exposes `btck_set_mock_time` as a wrapper over the existing `SetMockTime`, mirroring what the node has via the `setmocktime` RPC.
Prior IRC discussion: https://gnusha.org/bitcoin-kernel/2026-06-04.log
ACKs for top commit:
josibake:
ACK 156f2c6c49
purpleKarrot:
ACK 156f2c6c49
achow101:
ACK 156f2c6c49
janb84:
ACK 156f2c6c49
sedited:
ACK 156f2c6c49
Tree-SHA512: da61faab0477fdc0de0e16420f228923d57733c42ef91b8947ef576e43abc8a1b78b9c1fa2ae60550ed5fd545f4d1c714dad0191d4442d4063112031ef7d1027
e8691056c0 test: Unroll `&&` conditions in macros (rustaceanrob)
Pull request description:
Picked from #35713. Given that I think this is a strict debugging improvement, I opened as a separate pull:
Using `&&` in `BOOST_CHECK` is problematic as failures will not indicate which condition failed. By unrolling these checks, the user knows exactly which expression is the failing case.
As an example, here is a line that would be particularly hard to debug if it failed:
```
src/test/net_tests.cpp
BOOST_CHECK((*ret)[1] && (*ret)[1]->m_type == "headers" && std::ranges::equal((*ret)[1]->m_recv, MakeByteSpan(msg_data_2)));
```
If any one of these conditions fail, the whole expression fails, with no values printed or indication as to which condition failed.
This is also required when using test macros that support value decomposition, which requires `&&` and `||` are `delete`. Examples include `BOOST_TEST`, doctest, Catch2, etc.
ref: https://catch2-temp.readthedocs.io/en/latest/assertions.html#other-limitations
ref: https://fekir.info/post/decomposing-an-expression/
ACKs for top commit:
maflcko:
re-ACK e8691056c0🌽
ismaelsadeeq:
reACK e8691056c0
sedited:
ACK e8691056c0
Tree-SHA512: 9eb74cecd47ee4fdc3f53beb7d50d5056d543303d023c68b8d47cbe52d37f1156488c8b943faf68dd52c192c43626037bbf08172e7cc24753e0f6070db6e3ab2
0390338692 test: check MuSig import private key warnings (woltx)
5e62fbf09c wallet: check descriptor private key completeness on import (woltx)
cd8d01bf47 descriptors: require complete MuSig private keys (woltx)
Pull request description:
`importdescriptors` currently checks whether all private keys are present by expanding the descriptor and verifying that every expanded origin pubkey has a private key.
This is wrong for MuSig descriptors because expansion includes the synthetic aggregate pubkey. There is no individual private key for that aggregate pubkey, so importing a fully private MuSig descriptor such as `rawtr(musig(A_priv,B_priv))` incorrectly returns:
```
Not all private keys provided. Some wallet functionality may return unexpected errors
```
This PR fixes the issue by making descriptor private-key completeness account for MuSig participant keys, and by having `importdescriptors` use `Descriptor::HavePrivateKeys()` instead of duplicating its own manual completeness check.
The functional test covers both cases:
- `rawtr(musig(A_priv,B_priv))` imports without warnings.
- `rawtr(musig(A_priv,B_pub))` still warns that not all private keys were provided.
ACKs for top commit:
achow101:
ACK 0390338692
theStack:
Code-review ACK 0390338692
Tree-SHA512: a55fb084c63f725a0991556acdfb822f3a5a669f745a00b9f0bf0996b639986cdf5e2be2e8d3d0a2ee3fe5744355f20b40df576601792ef3db698e606629ad52
Chunked transfer trailers are just headers that are included
at the end of the request. We can parse and validate them with code
we already use to read headers. In a future commit we will also
be able to use one MAX_HEADERS_SIZE limit to cover both sections.
Even though we parse and validate, we ignore these data.
it adds a unit test verifying that clients not permitted by
`-rpcallowip` are rejected immediately after `accept()`, before any of
their request bytes are read from the socket.
Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>
Co-authored-by: pinheadmz <pinheadmz@pm.me>
77440814bf fuzz: reset SOCKS5 interrupt between inputs (Hao Xu)
Pull request description:
Reset `g_socks5_interrupt` before each `socks5` fuzz input.
`CThreadInterrupt` remains interrupted until explicitly reset. Previously,
inputs executed after the first input setting the interrupt flag inherited its
state. As corpus inputs are shuffled between all-input coverage runs, the
number of affected inputs and the resulting coverage counts could differ.
Tested with the complete 91-input `socks5` corpus. The all-input deterministic
coverage check passes.
ACKs for top commit:
nervana21:
tACK 77440814bf
maflcko:
lgtm ACK 77440814bf
sedited:
ACK 77440814bf
Tree-SHA512: d1b2b33661f9796628fd7eb1f4ddb212b07110ebbfa7516e305f8aa21bde7898b4bf8fc6f6570df22f8cf6380f1287cb9b6135683ad49b2bdbe83ad9a1af23b9
ParseHDKeypath() parsed each path element with ToIntegral<uint32_t>, so
a bare decimal >= 2^31 (e.g. "m/2147483648" == 0x80000000) was silently
treated as "m/0h".
This commit rejects such overflow instead.
ParseHDKeypath() lives in util/bip32, so its unit test belongs in
bip32_tests rather than psbt_wallet_tests. Pure move, no changes to the
test itself; subsequent commits extend it in its new home.
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
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
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
Both are identical since C++17 and this refactor shouldn't change any
behavior. The benefits are consistency and to be explicit, to avoid
confusion with the C++11/14 constexpr.
Co-Authored-By: l0rinc <pap.lorinc@gmail.com>
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's/^constexpr \S+ \w+(\[\])? ?[={]/inline &/' $( \
git grep -l '^constexpr ' -- \
'*.h' \
':(exclude)src/minisketch' \
)
-END VERIFY SCRIPT-
Both are fine and this refactor shouldn't change any behavior.
However, inline const will ensure each symbol has a single address
across all TU, making the release binary smaller.
-BEGIN VERIFY SCRIPT-
# Replace `static const`
sed -i "s/^static const /inline const /" $( \
git grep -l "^static const " -- \
'*.h' \
':(exclude)src/leveldb' \
':(exclude)src/secp256k1' \
)
# Replace plain `const`
sed -i --regexp-extended 's/^const (\S+ \w+(\[\])? ?[={])/inline &/' $( \
git grep -l '^const ' -- \
'*.h' \
':(exclude)src/leveldb' \
':(exclude)src/secp256k1' \
)
-END VERIFY SCRIPT-
This is required for the next commit.
Also, in a test, use `inline constexpr` for an `auto` type, which is
also needed for the next commit, which hard-codes a list of types for
conversion.
Both are fine and this refactor shouldn't change any behavior.
However, inline constexpr will ensure each symbol has a single address
across all TU, making the release binary smaller.
Review note: In theory the script may also cover functions, but they
were handled in the prior commit, to remove the redundant inline for
them.
-BEGIN VERIFY SCRIPT-
sed --regexp-extended -i 's/^(static constexpr|constexpr static)\>/inline constexpr/g' $( \
git grep --extended-regexp -l '^(static constexpr|constexpr static)' -- \
'*.h' \
':(exclude)src/crc32c' \
':(exclude)src/ipc/libmultiprocess' \
':(exclude)src/minisketch' \
)
-END VERIFY SCRIPT-
6a2de55a0d test: require `TryGetTotalRam()` detection (Lőrinc)
cd086c16dd node, qt: inline `DEFAULT_DB_CACHE` (Lőrinc)
8bd9f46082 kernel: allow setting chainstate `dbcache` (Lőrinc)
8aa21e119b kernel, node: colocate dbcache bounds (Lőrinc)
7cfa21d60a scripted-diff: use `MIN_DBCACHE_BYTES` (Lőrinc)
ab63432576 common: cache total RAM as `uint64_t` (Lőrinc)
031fa402c8 scripted-diff: use `TryGetTotalRam` (Lőrinc)
41c44f5588 node, qt: use `1_MiB` for dbcache conversions (Lőrinc)
Pull request description:
**Problem:** Since #34692, the node chooses a `450 MiB` or `1 GiB` database cache from detected RAM, while Kernel always uses `450 MiB`.
The shared names obscure the difference between the node's automatic policy and Kernel's fixed fallback, and Kernel callers cannot set their own cache budget.
**Fix:** Cache RAM detection as `uint64_t`, keep the node's two-tier default unchanged, and make the fixed Kernel fallback explicit.
Add a chainstate-manager option setter that accepts a total database cache budget and applies the shared bounds and cache split.
ACKs for top commit:
maflcko:
review ACK 6a2de55a0d🚵
stringintech:
re-ACK 6a2de55a
sedited:
ACK 6a2de55a0d
Tree-SHA512: 4c92267647a757efb79e8396015de89290eed56c6ff109d9e81495f33ea68ccf90b77cb4b65412cd7825c29b6bf5384ac4ec77fc6c50fbf3e31ee82cf0b552f0
756afe14b5 test: give each ValidateInputsStandardness case its own scope (JP)
5559fa464b test: fix wrong transaction in GetP2SHSigOpCount assertion (JP)
Pull request description:
While reading through `script_p2sh_tests.cpp` I noticed one of the assertions in `ValidateInputsStandardness` checks the wrong transaction.
The test builds `txToNonStd2_no_scriptSig` (which spends a P2SH prevout with an empty scriptSig) and checks its standardness result ("input 0 P2SH redeemscript missing"), but the `GetP2SHSigOpCount` assertion right after it re-checks the previous transaction: line 433 is byte-identical to line 419. Looks like a copy-paste slip from 248c175e3d, which added a `GetP2SHSigOpCount` check after each constructed transaction.
This PR points the assertion at `txToNonStd2_no_scriptSig` and expects 0 sigops. With an empty scriptSig there's no redeemScript push, so `GetSigOpCount(scriptSig)` ends up counting an empty subscript and returns 0. This case wasn't asserted anywhere before. The line above covers the other side, where the same prevout spent with the actual redeemScript counts 20.
To make sure the fix isn't vacuous I also ran the assertion expecting 20, and it fails with `[0 != 20]`.
Tested with:
```
cmake --build build --target test_bitcoin
build/bin/test_bitcoin --run_test=script_p2sh_tests
```
ACKs for top commit:
l0rinc:
ACK 756afe14b5
sedited:
ACK 756afe14b5
Tree-SHA512: 463eda7bb8790fb55619b36a6bedcd437f5c3d753e8c0abaa57dde3154421cde8c36f04293eb712ddf2745d525b5e254e8a1ab898f4f68538520dd80d873dba3
Add `btck_chainstate_manager_options_set_database_cache_bytes()` so Kernel callers can set the total database cache budget.
Use `uint64_t` for a fixed-width C API, reject values outside the architecture-specific range, and keep `DEFAULT_KERNEL_CACHE` as the fallback.
Apply the selected split to the block tree database and `LoadChainstate()`.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>
Co-authored-by: stringintech <stringintech@gmail.com>
