faada35f9c fuzz: [refactor] Use 100'000 digit separator in __AFL_LOOP (MarcoFalke)
fae067ec4a fuzz: Avoid dangling prevoutfetch threads after AFL fork (MarcoFalke)
Pull request description:
Presumably fixes https://issues.oss-fuzz.com/issues/536943806
This is a bit confusing, because the issue was already fixed in commit f608a409f7, by removing the AFL forkserver.
However, OSS-Fuzz doesn't go through the AFL_LOOP, but through the AFL libFuzzer driver:
```
#0 0x7e055245baab in __pthread_clockjoin_ex /build/glibc-B3wQXB/glibc-2.31/nptl/pthread_join_common.c:89:6
#1 0x5a27e904dcdd in operator() /src/llvm-project/compiler-rt/lib/asan/asan_interceptors.cpp:293:14
#2 0x5a27e904dcdd in Join<(lambda at /src/llvm-project/compiler-rt/lib/asan/asan_interceptors.cpp:292:44)> /src/llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_thread_arg_retval.h:75:9
#3 0x5a27e904dcdd in ___interceptor_pthread_join /src/llvm-project/compiler-rt/lib/asan/asan_interceptors.cpp:292:25
#4 0x5a27e90f4044 in std::__1::thread::join()
#5 0x5a27e9366277 in ThreadPool::Stop() [bitcoin-core/src/util/threadpool.h:146](7d8137c141/src/util/threadpool.h (L146)):53
#6 0x5a27e9365db9 in ThreadPool::~ThreadPool() [bitcoin-core/src/util/threadpool.h:94](7d8137c141/src/util/threadpool.h (L94)):9
...
#32 0x5a27e95a9506 in (anonymous namespace)::ResetChainman(TestingSetup&) (.12669) [bitcoin-core/src/test/fuzz/process_messages.cpp:44](7d8137c141/src/test/fuzz/process_messages.cpp (L44)):27
#33 0x5a27e95a8c60 in process_messages_fuzz_target(std::__1::span<unsigned char const, 18446744073709551615ul>) [bitcoin-core/src/test/fuzz/process_messages.cpp:141](7d8137c141/src/test/fuzz/process_messages.cpp (L141)):9
...
#36 0x5a27e97b7190 in test_one_input(std::__1::span<unsigned char const, 18446744073709551615ul>) bitcoin-core/src/test/fuzz/fuzz.cpp:86:5
#37 0x5a27e97b7190 in LLVMFuzzerTestOneInput bitcoin-core/src/test/fuzz/fuzz.cpp:214:5
#38 0x5a27e90ada19 in LLVMFuzzerRunDriver /src/aflplusplus/utils/aflpp_driver/aflpp_driver.c:427:13
#39 0x5a27e90ad69b in main /src/aflplusplus/utils/aflpp_driver/aflpp_driver.c:323:10
#40 0x7e055223b082 in __libc_start_main /build/glibc-B3wQXB/glibc-2.31/csu/libc-start.c:308:16
#41 0x5a27e8fc602d in _start
```
So the correct fix would be to set `AFL_DRIVER_DONT_DEFER=1`. Ref: ad5304010a/utils/aflpp_driver/aflpp_driver.c (L161)
However, I don't know how to do this on OSS-Fuzz, so just drop the threads for now, because there are dedicated fuzz targets to test the multi-threaded case anyway.
ACKs for top commit:
l0rinc:
ACK faada35f9c
andrewtoth:
lgtm ACK faada35f9c
sedited:
ACK faada35f9c
Tree-SHA512: c249d7267f789084968f8510531f60fc71c9fbd6b4e574a181fd6da1af19a3cb7c03ba60c3ca70244b56ce44c602293517cdb6b0e969b4a2cc9d1afaa49ab0f8
3bfdcbd7ee coins: reuse cache hasher for txid set (Lőrinc)
2beab94896 coins: use SipHash-1-3-UJ for `CCoinsMap` (Lőrinc)
7ff55cc650 bench: add fixed-width SipHash benchmarks (Lőrinc)
3aea85411f test: add SipHash-1-3-UJ coverage (Pieter Wuille)
a0ccd4ad17 crypto: add fixed-width SipHash-1-3-UJ (Pieter Wuille)
c2d7931b5c crypto: add generic SipHash-1-3-UJ (Pieter Wuille)
25bfca06d6 refactor: simplify adding SipHash-1-3-UJ (Lőrinc)
af50ba8500 test: add shared SipHash vectors (Lőrinc)
Pull request description:
**Problem:** The in-memory UTXO cache hashes `COutPoint` keys containing a 32-byte txid and a 32-bit output index.
SipHash-2-4 processes the txid as four independent 64-bit blocks, so its optimized 32-byte and 36-byte paths both take 14 SipRounds.
This also matters for hash-prefix index work such as [#35531](https://github.com/bitcoin/bitcoin/pull/35531): once a persisted key format chooses a hash function, changing it later requires reindexing.
**Fix:** Add `SipHasher13UJ`, a custom block-oriented variant combining Pieter Wuille's jumbo-block suggestion with SipHash-1-3, the reduced-round variant discussed in the [SipHash analysis](https://eprint.iacr.org/2012/351.pdf).
It provides inline `Hash` overloads for the fixed-width inputs used here.
Use a dedicated `SaltedCoinsCacheHasher` for `CCoinsMap` and `CoinsViewOverlay`'s temporary earlier-txid set, while other outpoint tables remain on SipHash-2-4.
The salted hash values vary between restarts and are never persisted or sent over the network.
**Design:** `SipHasher13UJ` accepts normal 64-bit blocks and 256-bit jumbo blocks.
For hash-table use, cryptographic hash outputs must make up all but a small bounded number of retained jumbo blocks.
The construction mixes all four limbs around one SipRound, omits byte-oriented padding, and uses an `"unpadded"` finalizer distinct from standard SipHash-1-3.
The fixed-width paths take four rounds for one `uint256` jumbo block and five when followed by one normal block.
For outpoints, the 32-bit output index is zero-extended into a normal 64-bit block.
Retained `CCoinsMap` entries identify real transaction outputs, so their keys contain computed txids.
Missing-input validation may probe arbitrary claimed prevouts, but `FetchCoin()` immediately erases their temporary entries when the backend lookup fails, so non-hash keys cannot accumulate.
The assumeutxo loader assumes snapshot txids are valid while loading and verifies the complete snapshot's content hash before activation.
Every entry in the temporary earlier-txid set is a computed transaction hash, and the set is bounded by the block's transaction count.
This construction is limited to local hash tables and is not a general-purpose or protocol SipHash replacement.
Pieter discussed the construction with [SipHash co-author Jean-Philippe Aumasson](https://github.com/bitcoin/bitcoin/pull/35215#issuecomment-4385336928), whose preliminary analysis did not find an easier collision construction and supported SipHash-1-3 for this hash-table use.
<img width="2100" height="860" alt="siphash_compare_updated" src="https://github.com/user-attachments/assets/cefec6f8-5ec0-450a-a0a2-f946de9ef36d" />
**Structure:** Shared vectors first cover the existing generic and fixed SipHash-2-4 paths in C++, the generic path in Python, and their randomized equivalence in the fuzzer.
A behavior-neutral refactor then moves the round, compression, and finalization logic into inline `SipHashState` methods; assembly inspection shows that the fixed-width paths retain their instruction counts, while the generic byte loop retains its prior code generation through a local state copy.
Three Pieter-authored commits add the generic UJ specification, fixed-width implementation, and shared correctness coverage.
Benchmarks follow that coverage, then separate commits change `CCoinsMap`'s hasher and reuse it for the temporary earlier-txid set.
**Tests:** The shared JSON supplies the same byte sequences to the generic C++ and Python SipHash-2-4 implementations, with applicable fixed-width paths checked against the same expected output.
The SipHash-2-4 rows include the 64 official vectors for inputs from 0 to 63 bytes and cases that vary input chunking.
The UJ outputs were generated by an independent implementation and are checked using normal blocks, equivalent zero-extended jumbo blocks, and applicable fixed-width `Hash` overloads.
The integer fuzzer extends these comparisons to arbitrary values and mixed normal/jumbo block encodings.
[Counting the dbcache buckets](https://gist.github.com/l0rinc/d68f56c3ed89f76f56da6632ef6f2d92) indicates the new outpoint hasher retains the uniform bucket distribution expected by `CCoinsMap`:
<img width="1200" height="750" alt="ccoinsmap-collisions" src="https://github.com/user-attachments/assets/eeedec81-acdc-4adf-a9c8-bfce089700da" />
**Benchmarks:** Fixed-width microbenchmarks compare SipHash-2-4 with SipHash-1-3-UJ for 32-byte hashes and inputs containing a 32-byte hash plus a 32-bit index.
Reported aarch64 measurements and an [independent x86_64 run](https://github.com/bitcoin/bitcoin/pull/35215#issuecomment-4400609637) show the outpoint path is about 2x faster.
<details><summary>Benchmark runner</summary>
```bash
for COMPILER in gcc clang; do \
if [ "$COMPILER" = gcc ]; then CC=gcc; CXX=g++; else CC=clang; CXX=clang++; fi; \
cmake -B "build-bench-$COMPILER" -DCMAKE_BUILD_TYPE=Release -DBUILD_BENCH=ON -DBUILD_TESTS=OFF -DBUILD_GUI=OFF -DENABLE_WALLET=OFF -DCMAKE_C_COMPILER="$CC" -DCMAKE_CXX_COMPILER="$CXX" >/dev/null 2>&1 && \
cmake --build "build-bench-$COMPILER" --target bench_bitcoin -j"$(nproc)" >/dev/null 2>&1 && \
echo "" && echo "$(date -I) | SipHash fixed-width microbench | $("$CXX" --version | head -1) | $(hostname) | $(uname -m) | $(lscpu | awk -F: '/Model name/{print $2; exit}' | xargs) | $(nproc) cores | $(free -h | awk '/^Mem:/{print $2}') RAM" && \
"build-bench-$COMPILER/bin/bench_bitcoin" -filter='SipHash.*32b|SipHash.*36b' -min-time=10000; \
done
```
</details>
A two-run GCC `-reindex-chainstate` comparison of the same `CCoinsMap` hot path through height 957,759 with `-dbcache=2000` on a Ryzen 7 3700X/SSD reduced mean wall time from 11,278 s to 10,759 s, a ~5% validation speedup.
ACKs for top commit:
achow101:
light ACK 3bfdcbd7ee
sipa:
ACK 3bfdcbd7ee (to the extent the code/ideas aren't my own)
andrewtoth:
ACK 3bfdcbd7ee
optout21:
ACK 3bfdcbd7ee
Tree-SHA512: c3c66051cb1ebdb0cddbc8b8bed2297c524842f92960531de23d9586c5bb950b302c33d06fc15c2320cbbf97273b22ec3f17fd0e7ddcc7df4a8132a47a606277
6aa5d8d948 blockencodings: fix extra transaction count (Lőrinc)
be4e64d9e4 test: characterize extra transaction miscount (Lőrinc)
Pull request description:
A short ID collision can invalidate a mempool-sourced transaction after an unrelated transaction was found in extra_txn.
Track each slot's source so extra_count is decremented only when the invalidated slot came from extra_txn. Retain the source after a collision to preserve the rule that later candidates do not refill the slot.
ACKs for top commit:
l0rinc:
retested ACK 6aa5d8d948
andrewtoth:
ACK 6aa5d8d948
sedited:
ACK 6aa5d8d948
Tree-SHA512: d4407dca7ca2b46795e52a5d611b66072d75ba966416d4ede27fa776ac0a7cbde52c8abae2daec532ec5f24d88e608622168ca066c0d1dbc133e098c8e6fe85a
5d57f2cefe test: cover unused mempool space in coins cache (woltx)
Pull request description:
This PR extends the existing unit test for `Chainstate::GetCoinsCacheSizeState()`.
`Chainstate::GetCoinsCacheSizeState()` calculates when the UTXO/coins cache is too large and should be flushed. Part of that calculation includes unused `-maxmempool` space. For example, if the mempool limit is 300 MiB but the mempool is mostly empty, some of that unused space can be counted toward the coins cache limit.
The existing `validation_flush_tests.cpp` test checks this calculation by calling:
```c++
chainstate.GetCoinsCacheSizeState(MAX_COINS_BYTES, max_mempool_size_bytes)
```
This change extends the test to also check the no-argument call:
```c++
chainstate.GetCoinsCacheSizeState()
```
That is the call used by validation code during normal operation.
The test grows the coins cache above the coins-only limit, then checks that:
- calling the explicit helper with `max_mempool_size_bytes=0` reports `CRITICAL`
- calling the normal no-argument method reports `OK`, because it includes unused mempool space
This makes sure future refactors do not accidentally drop unused mempool space from the normal cache-size calculation.
ACKs for top commit:
l0rinc:
ACK 5d57f2cefe
sedited:
ACK 5d57f2cefe
Tree-SHA512: c4131cdedd7bd48224d3323fdc1bbce668f171105939c8aa3c452080bbe22819f3910d2a5ce3f46a5a1b7eb625b63240a8c0b7c26fbd5f30927712545b0f765f
d24d3cbad0 fuzz: add p2p_private_broadcast harness (frankomosh)
Pull request description:
Add a fuzz harness for `ConnectionType::PRIVATE_BROADCAST`, a privacy-preserving transaction relay mechanism whose p2p code paths had no meaningful fuzz coverage.
Current `process_message` touches it but is insufficient in exercising it. It creates `PRIVATE_BROADCAST` nodes via `ConsumeNode()`, but some structural problems prevent it from covering the relevant logic:
1. `m_tx_for_private_broadcast` is never seeded, `PushPrivateBroadcastTx` always takes the immediate disconnect path (7 accidental hits, all on lines 3559–3562). Lines 3564–3570 (the actual INV send) had 0 hits.
2. `ALL_NET_MESSAGE_TYPES` is used as the message pool. `CConnman::PushMessage` silently drops anything outside the four-type allowlist for private broadcast connections, wasting most iterations.
3. Connection types are picked randomly, hence private broadcast coverage is accidental.
To solve the issues above;
- this harness explicitly constructs nodes with `ConnectionType::PRIVATE_BROADCAST`
- seeds `m_tx_for_private_broadcast` via `InitiateTxBroadcastPrivate` before the peer connects, so `PushPrivateBroadcastTx` reaches the transaction send path
- constrains the message pool to the four types permitted by handshake or private-broadcast filter in PeerManagerImpl::ProcessMessage
- passes `{NODE_NONE}` to `InitializeNode`, matching what `PushNodeVersion` advertises for private broadcast peers.
ACKs for top commit:
instagibbs:
ACK d24d3cbad0
brunoerg:
code review ACK d24d3cbad0
andrewtoth:
ACK d24d3cbad0
Tree-SHA512: 0d2ff9a79aa87a6eb7d7efdbe03ebba545abad1d1e995dfe617e5bd8fd7a5098fd44d92fa6eac1695aa81f38e762b30ed7a7fd0db6c85aaa32bd1d068c946e38
Use `SaltedCoinsCacheHasher` for the temporary set of earlier txids in `CoinsViewOverlay`, and in existing overlay tests to exercise the new `Txid` overload.
Every entry is a computed transaction hash, and the set is limited to a few thousand elements per block, satisfying the SipHash-1-3-UJ jumbo-input requirements.
Co-authored-by: Andrew Toth <andrewstoth@gmail.com>
Use the fixed-width `SipHasher13UJ::Hash` path for `CCoinsMap`, while keeping other `SaltedOutpointHasher` users on SipHash-2-4.
The salted outputs are process-local and must not be persisted, serialized, or compared across processes.
Retained cache entries identify real transaction outputs and therefore contain computed txids.
Missing-input validation may probe arbitrary claimed prevouts, but `FetchCoin()` erases each temporary entry immediately when the backend lookup fails, so non-hash keys cannot accumulate.
The assumeutxo loader assumes snapshot txids are valid while loading and verifies the complete snapshot content hash before activation.
Co-authored-by: Pieter Wuille <pieter@wuille.net>
Add SipHash-1-3-UJ outputs to the shared vectors for sequences of 8- and 32-byte blocks.
Check generic writes and applicable fixed-width `Hash` overloads against those outputs, and fuzz their equivalence including mixed normal/jumbo encodings.
The outputs were generated by an independent implementation that Claude Opus 4.8 produced using only the `SipHasher13UJ` class comment as its prompt.
Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
Lock SipHash-2-4 behavior into shared vectors before refactoring its round and finalization code.
Store inputs as ordered hex byte blocks so `CSipHasher` and the independent Python implementation hash the same byte sequence, with applicable `PresaltedSipHasher` overloads checked against the same vectors.
Add the 64 official SipHash-2-4 vectors alongside block-partition and empty-block cases for the generic path.
Move randomized generic/fixed comparisons to the integer fuzzer.
SipHash-1-3-UJ coverage can add expected outputs for compatible 8- and 32-byte block sequences.
The Python test reads a build-tree copy so functional-test staging behaves consistently when files are symlinked or copied.
Co-authored-by: Pieter Wuille <pieter@wuille.net>
A short ID collision can invalidate a mempool-sourced transaction after an unrelated transaction was found in extra_txn.
Track each slot's source so extra_count is decremented only when the invalidated slot came from extra_txn. Mark collided slots explicitly so later candidates do not refill them.
3e8e21b2ef txgraph: avoid moving primitive members (Lőrinc)
d9f94aa882 rpc: avoid moving RPC enum types (Lőrinc)
b67baed4e7 coins: avoid moving `COutPoint` values (Lőrinc)
Pull request description:
Inspired by https://github.com/bitcoin/bitcoin/pull/34320#discussion_r2751764873.
**Problem:** A few code paths use rvalue references or `std::move()` for types where moving provides no benefit.
`EmplaceCoinInternalDANGER` took `COutPoint&&`, forcing callers to pass trivially copyable outpoints as rvalues even though the cache stores its own key.
Some call sites also use `std::move()` on enum and primitive values, where it only adds noise.
> [!NOTE]
> `CheckTriviallyCopyableMove` remains `false` since `std::move()` on trivially copyable types can still be useful as intent documentation, for example to signal that a value should not be reused after a call.
**Fix:** Take trivially copyable arguments by const reference where the callee only needs to store its own copy, and pass existing values directly at the call sites.
Also remove `std::move()` from enum and primitive assignments where it has no semantic effect.
ACKs for top commit:
maflcko:
review ACK 3e8e21b2ef 🖇
hodlinator:
re-ACK 3e8e21b2ef
andrewtoth:
ACK 3e8e21b2ef
hebasto:
ACK 3e8e21b2ef, I have reviewed the code and it looks OK.
Tree-SHA512: cbe55b13290ae261bba359dc6e5a3bbdfb7ae9d31bdf8e0da2eef65a0df776e4081ceeac3c82731a631bda656a86b3789651fa1f4d87875cee1dc96351bdfd7c
Serialization parameters should be embedded into the object being
serialized rather than passed as a separate argument. This works here
because only serialization is performed and no new object needs to be
constructed.
dab7f2c984 test: cover -externalip/onlynet interaction in functional test (will)
657a5aa3f3 test: cover -externalip bypassing -onlynet (will)
8c87e32bd3 net: let -externalip bypass -onlynet (will)
f4af02e827 net: add an add_even_if_unreachable argument to AddLocal (will)
Pull request description:
`-onlynet` is documented to restrict automatic outbound connections, but it also currently prevents `-externalip` addresses from being advertised when their network is not in the `-onlynet` set. This happens because `AddLocal()` rejects addresses outside `g_reachable_nets`, regardless of whether the address was explicitly configured by the user.
Previous attempts to fix this (#24835 and #25690) removed the `g_reachable_nets` check from `AddLocal()`.
This PR instead adds an explicit `add_even_if_unreachable` argument to `AddLocal()`. The argument defaults to `false`, and is set to `true` only when adding addresses from `-externalip`.
As a result, explicitly configured `-externalip` addresses can still be advertised even when their network is excluded by `-onlynet`, while discovered, mapped, bound, Tor control, and I2P SAM addresses continue to use the existing reachable-network filter.
This keeps the fix scoped to `-externalip` and addresses the concern raised in #25690:
> I think it might also be weird for a user to activate -onlynet and keep on advertising their clearnet address to the network
The branch adds unit coverage for `AddLocal()` and functional coverage in `p2p_addr_selfannouncement.py` for `-onlynet=ipv4 -externalip=<onion>`.
Fixes: #25336Fixes: #25669
ACKs for top commit:
achow101:
ACK dab7f2c984
mzumsande:
re-ACK dab7f2c984
w0xlt:
ACK dab7f2c984
Tree-SHA512: a4ac9334b85da8b6902d3850e21d3a1c9d7dce70bcb79182448c8d5684e24462cd6e440385af7aa4420d9582e4dff9dc9e827ca7a6da0363fff2d3c531784d9b
72db4accbf coins: drop stale cursor null checks (Lőrinc)
3d2f2d8de0 coins: pass UTXO stats view by reference (Lőrinc)
35aedb2823 coins: drop cursor from base view (Lőrinc)
c6fbe2f66c coins: pass DB view to cursor users (Lőrinc)
Pull request description:
**Problem:** `CCoinsView::Cursor()` makes cursor iteration look like a generic coins view operation, but cursor iteration is only supported by the DB-backed coins view.
The cache override only threw, and the `coins_view` fuzz target only asserted that deterministic unsupported throw path.
**Fix:** Make cursor iteration a `CCoinsViewDB` operation.
Cursor users now take the DB-backed view directly, `CCoinsView` no longer exposes `Cursor()`, and the fuzz target keeps DB-backed cursor coverage while dropping the unsupported cache throw probe.
The UTXO stats path is also tightened to pass the non-null DB view by reference, and stale null handling for DB cursors is removed.
This was extracted from review discussion in https://github.com/bitcoin/bitcoin/pull/35295#discussion_r3420576781 and extended based on https://github.com/bitcoin/bitcoin/pull/35562#issuecomment-4746585893.
ACKs for top commit:
achow101:
ACK 72db4accbf
sedited:
Re-ACK 72db4accbf
w0xlt:
ACK 72db4accbf
andrewtoth:
ACK 72db4accbf
Tree-SHA512: 12a81330a6ec1b91a7e4393f3761ea9ed4702ecb24312f1defa5a9a079a396ce921fc52f74fe296e5ac7ab20d5b5a8a84e858c96847f333c58b7fa9de9e8143e
Remove the `Mutex` from the `coins_view` and `coinscache_sim`
pool startup helpers. Fuzz targets are entered sequentially within a
process and parallel fuzzing uses separate processes/forks, which each
have their own copy of the global thread pool. Therefore, a mutex to
prevent two in-process callers from racing to start the pool isn't needed.
fab8eeed82 fuzz: clang-format LIMITED_WHILE (MarcoFalke)
fa0d777ce2 fuzz: Clang-format LIMITED_WHILE like while (MarcoFalke)
fa1a9bde5a fuzz: Remove unused workaround after fix in libmultiprocess byte-span serializer (MarcoFalke)
fa55385ab3 fuzz: Use LIMITED_WHILE over for-loop with consumed size integral (MarcoFalke)
6d5f753921 Squashed 'src/ipc/libmultiprocess/' changes from 28e056576a..e8de5c7b68 (MarcoFalke)
Pull request description:
Includes several changes, to first update the subtree. Then, modify the fuzz test to address review comments:
* https://github.com/bitcoin/bitcoin/pull/35118#discussion_r3506566815
* https://github.com/bitcoin/bitcoin/pull/35118#discussion_r3523175145
ACKs for top commit:
ryanofsky:
Code review ACK fab8eeed82. Just fuzz test clang-format cleanups added since last review, which seem nice
Tree-SHA512: 0836628f8ee54adf02571025456211a74f63d05058b72280b10111ecfbb93d30945f4a48f30cc790774de4e2b489907313e86b0fa6729f3480c64850c94848b4
6667dc4eec kernel: expose scriptSig for btck_TransactionInput (Peter Zafonte)
e6de3a2d3c kernel: expose witness stack for btck_TransactionInput (Peter Zafonte)
Pull request description:
Silent payments scanning needs the public key from every input. For SegWit inputs it is in the witness stack. For P2PKH inputs it is in scriptSig. Without these new functions, callers must deserialize the raw transaction themselves to reach that data, which is difficult and error-prone.
Introduces a `btck_WitnessStack` type and adds the following functions:
**Witness stack:**
`btck_transaction_input_get_witness_stack`: returns a non-owning `const btck_WitnessStack* `view
`btck_witness_stack_count_items `: item count
`btck_witness_stack_get_item_at `: single item by index via btck_WriteBytes
`btck_witness_stack_copy` / `btck_witness_stack_destroy`: lifecycle for owned copies
**scriptSig:**
`btck_transaction_input_get_script_sig`: full scriptSig via btck_WriteBytes
All functions are exposed in the C++ wrapper via `WitnessStackView`, `WitnessStack`, and `WitnessStackApi` , and `GetScriptSig()`.
ACKs for top commit:
sedited:
ACK 6667dc4eec
musaHaruna:
ACK [6667dc](6667dc4eec)
stickies-v:
ACK 6667dc4eec
Tree-SHA512: b5e9d32ec87a5f5a9fea5652ed69737eae1d1f9cfb777544b96d703bbd057c114e0b7afae4e9e9d09b8b546694b14fb923b747c04a8f0211ab6caa015d06967d
This is a whitespace-only clang-format change.
To verify it, one can run:
```sh
(git show | git apply --reverse ) && ( git diff -U0 | ./contrib/devtools/clang-format-diff.py -p1 -i -v ) && git diff HEAD
```
A few minor, non-macro formatting adjustments were made in touched files:
* `src/wallet/test/fuzz/crypter.cpp`: Removed a redundant double semicolon
* `src/test/fuzz/txorphan.cpp`: Corrected indentation on an `else if` block.
* `src/test/fuzz/mini_miner.cpp`: Removed an unnecessary empty line.
d164a04342 node: smooth oversized `-dbcache` warnings (Lőrinc)
Pull request description:
**Problem:** The oversized `-dbcache` warning threshold has a sharp formula cliff when detected RAM crosses the cutoff used by the warning logic.
This was reported during review of [#34641](https://github.com/bitcoin/bitcoin/pull/34641#discussion_r2900769756), where an earlier version could jump from the auto default at `4095 MiB` RAM to `75%` of RAM at `4096 MiB`.
That made `1 MiB` of extra detected RAM raise the warning threshold from about `511 MiB` to `3072 MiB`.
The surviving warning-only code has the same shape at a different boundary.
Below `2 GiB` RAM the cap is `DEFAULT_DB_CACHE` (`450 MiB`), but at `2 GiB` it switches to `75%` of total RAM, so a tiny increase in detected RAM can suddenly raise the warning threshold from `450 MiB` to about `1536 MiB`.
<img width="1484" height="881" alt="Image" src="https://github.com/user-attachments/assets/b51d5d24-31b8-4a2a-8f70-e7536481f855" />
**Fix:** Base the warning on a reserved non-dbcache memory budget instead:
```math
\text{warn if } \mathit{dbcache} > \max\left(\mathit{DEFAULT\_DB\_CACHE}, 0.75 \cdot \max(\text{total RAM} - \mathit{DBCACHE\_WARNING\_RESERVED\_RAM}, 0)\right)
```
`DBCACHE_WARNING_RESERVED_RAM` is `2 GiB`, so the fixed `DEFAULT_DB_CACHE` cap remains the floor below that reserve and the warning threshold grows monotonically above it.
This keeps the warning conservative around low-memory boundaries and avoids treating a boundary-crossing `1 MiB` RAM difference as a reason to allow a much larger explicit `-dbcache`.
**Quick reference:**
| System RAM | Previous warning cap | New warning cap |
| ---------- | -------------------- | --------------- |
| 1 GiB | 450 MiB | 450 MiB |
| 2 GiB | 1536 MiB | 450 MiB |
| 3 GiB | 2304 MiB | 768 MiB |
| 4 GiB | 3072 MiB | 1536 MiB |
| 8 GiB | 6144 MiB | 4608 MiB |
| 16 GiB | 12288 MiB | 10752 MiB |
| 32 GiB | 24576 MiB | 23040 MiB |
On 32-bit builds, effective `-dbcache` values are still capped to `1024 MiB` before the warning check, so thresholds above that cap are not reachable there.
ACKs for top commit:
optout21:
reACK d164a04342
sedited:
Re-ACK d164a04342
w0xlt:
reACK d164a04342
Tree-SHA512: deb81f0e192261f01dda6f1575a7b2e147f1e07e813d0833c7aeaf6a3fbd7594c0145ec1c98ded74efee9005108d229942949279c9458cbec4046913387db845
63c5f9d22c test: Remove `mock_process.cpp` (rustaceanrob)
Pull request description:
Picked from #35587, but I think has motivation to go in on its own.
The previous binary used a number of `Boost.Test` features:
- `boost::unit_test::disable`
- `BOOST_FAIL`
- `boost::exit_test_failure`
This patch duplicates the previous mock process behavior with no boost features.
With the patch we can:
- simplify the test config
- remove a linted boost include
- remove a file that was not actually a test
ACKs for top commit:
kevkevinpal:
ACK [63c5f9d](63c5f9d22c)
maflcko:
review ACK 63c5f9d22c🧀
hebasto:
ACK 63c5f9d22c.
Tree-SHA512: 15ff5ad49256149bb419beb72f002dd55fc62139e61fd80d2cecceff2699f291a5c14b39b027c412352aa965ec320bac320be32170e6d9d944b43e14f4864c35
6d0ea4cf5b doc: add release notes (Andrew Toth)
a2b1c86903 txospenderindex: disable bloom filters to optimize disk usage (Andrew Toth)
Pull request description:
LevelDB bloom filters are only consulted on `Get` point reads. This can be verified in https://github.com/bitcoin/bitcoin/blob/master/src/leveldb/table/table.cc#L224-L228. `InternalGet` is the only place that consults the filter, and it is only reached via a `Get` or `Exists` point read. The filters are never consulted for iterator seeks with an iterator created via `NewIterator`.
txospenderindex only reads via iterator seeks, so building them is wasted effort and space.
For a db as large as txospenderindex, this results in measurable performance and disk usage.
On master, a full sync took 4h37m, and the resulting db was 85.0 GiB.
On this branch, a full sync took 3h57m, and the resulting db was 80.9 GiB.
So this is a sync speedup of 39 minutes (1.17x), and a disk space reduction of 4.2 GiB.
ACKs for top commit:
l0rinc:
ACK 6d0ea4cf5b
sedited:
Re-ACK 6d0ea4cf5b
fjahr:
Code review ACK 6d0ea4cf5b
Tree-SHA512: fb88b9f9a16ff31562d388e3fd9fd9590c7864dbe6093cd9430ecbce9cdc3f2a8d3fc612aade743d26ad4c6eca1e5dc9b3f1ca28d75caea1209e5c784895405d
The previous binary used a number of `Boost.Test` features:
- `boost::unit_test::disable`
- `BOOST_FAIL`
- `boost::exit_test_failure`
This patch duplicates the previous mock process behavior with no boost features.
With the patch we can:
- simplify the test config
- remove a linted boost include
- remove a file that was not actually a test
e9ed898a0d validation: Don't use m_chain.Tip() in FlushStateToDisk (Martin Zumsande)
3679f1ecf5 index: Don't commit ahead of the flushed chainstate (Martin Zumsande)
65735728a5 index: Remove return value from Commit() (Martin Zumsande)
09c06960c6 validation: track last flushed block (Martin Zumsande)
13c02b5466 test: add test for index commits ahead of the last flushed block (Martin Zumsande)
Pull request description:
If indexes commit their data ahead of the flushed chainstate, and there is an unclean shutdown, the index will be corrupted. This is especially the case for the coinstatsindex, which has state (the muhash) which can't easily be rolled back without access to the blocks. This was only partly fixed in #33212 (for reorg scenarios) but could still happen during initial sync.
Fix this more thoroughly by having the node keep track of the last flushed block, and skipping index commits if the current block of the index is not an ancestor of the node's last flushed block (similar to the suggestion by stickies-v in https://github.com/bitcoin/bitcoin/pull/33212#pullrequestreview-31408570890.
Fixes#33208Fixes#34261
ACKs for top commit:
achow101:
ACK e9ed898a0d
sedited:
Re-ACK e9ed898a0d
fjahr:
re-ACK e9ed898a0d
Tree-SHA512: 7f4dc6fb942d6726587eb75dece24c79c679d8630320502aca9fa2d2b03b1d25999cd11255ec20344ebbb8985552747e2554e2557b9d2ad0c75db71652d615ab
The oversized `-dbcache` warning currently switches from a fixed `450 MiB` threshold below `2 GiB` of RAM to `75%` of total RAM at `2 GiB`.
This creates a cliff where a small increase in RAM can raise the warning threshold to about `1536 MiB`.
Apply the `75%` factor only to RAM above a `2 GiB` reserve while keeping `DEFAULT_DB_CACHE` as the minimum threshold.
This removes the cliff: the threshold stays at the default until the percentage term exceeds it, then grows by `0.75 MiB` per additional MiB of RAM.
This also aligns better with the recently merged parallel input prevout fetcher which performs better with slightly lower dbcache memory.
Co-authored-by: Bortlesboat <Bortlesboat@users.noreply.github.com>
Otherwise, if the node has an unclean restart,
indexes with state (coinstatsindex) couldn't reorg to the
last flushed tip and would be corrupted.
Also updates documentation of Commit() -
the locator functionality isn't used, so the previous text was wrong:
We must have the best block in our block index after a restart.
Co-authored-by: Fabian Jahr <fjahr@protonmail.com>
Assigning `ToIntegral<int64_t>("-1")` to the `optional<uint64_t>` `n`
is a silent underflow. `BOOST_CHECK_EQUAL` then promotes `int` to
`uint64_t`, which also underflows. The correct check is to do this
inline.
dc1c17c085 doc: add release notes (Andrew Toth)
0e10937184 fuzz: add coins_view_stacked fuzz harness to test concurrent leveldb reads (Andrew Toth)
ce610a6ff4 fuzz: update harnesses to cover CoinsViewOverlay::StartFetching (Andrew Toth)
760fb22dc3 test: add unit tests for CoinsViewOverlay::StartFetching (Andrew Toth)
d69a3b20de doc: update CoinsViewOverlay docstring to describe parallel fetching (Andrew Toth)
ab2a379237 coins: fetch inputs in parallel (Andrew Toth)
fdf283036a coins: add ready flag to InputToFetch (Andrew Toth)
ede11b8314 validation: collect block inputs in CoinsViewOverlay before ConnectBlock (Andrew Toth)
f82043af50 coins: introduce thread pool in CoinsViewOverlay (Andrew Toth)
5bf1c32008 validation: add -prevoutfetchthreads configuration option (Andrew Toth)
Pull request description:
This PR is a continuation of https://github.com/bitcoin/bitcoin/pull/31132. All outstanding issues raised there have been resolved, but the volume of stale comments can make that change difficult to review.
Currently, when connecting a block, each input prevout is looked up one at a time. For every input we first check the in-memory coins cache, and on a miss we make a synchronous round-trip to the chainstate LevelDB to read the coin from disk. Because these lookups happen serially as the block is being validated, the disk read latency stacks up and dominates the time spent in `ConnectBlock` whenever many inputs are not already in the cache.
This PR moves those disk reads onto a pool of worker threads that run in parallel with block connection. Before entering `ConnectBlock` the block is handed to a `CoinsViewOverlay`, which kicks off the workers to begin fetching all of the block's prevouts from disk and warming the cache. The main validation thread continues to do exactly the same work it does today, hitting the cache for each input in order. The only difference is that by the time it asks, the coin is much more likely to already be there. There are no validation logic or consensus behavior changes. This is purely a parallelization of an existing read pattern.
The number of fetcher threads is configurable via `-prevoutfetchthreads=<n>`, defaulting to 8 and capped at 16. Setting it to 0 disables input fetching entirely and reverts to the previous serial behavior.
We have measured large performance gains for IBD and `-reindex-chainstate`, as well as worst-case steady-state block connection at the tip. l0rinc ran many thorough benchmarking passes on the original PR across multiple machines, storage types, dbcache sizes[^1], operating systems[^2], and fetcher thread counts[^3]. Many other contributors also posted their benchmark results in the original PR. IBD speedups range from 1.18× to over 3× faster[^4]. Worst-case block connection time for network-attached storage was over 2× faster[^5]. Flamegraph comparisons before and after this change are available[^6].
On safety: `ConnectBlock` runs while holding `cs_main`, so nothing else in the node can mutate the chainstate while the fetchers are reading it.
On LevelDB: [concurrent reads are fully supported](https://github.com/bitcoin/bitcoin/blob/master/src/leveldb/include/leveldb/db.h#L44) and [documented as such](https://github.com/bitcoin/bitcoin/blob/master/src/leveldb/doc/index.md#concurrency). We already rely on this in production today against our other LevelDB-backed databases. The `txindex` DB is read by multiple simultaneous HTTP RPC worker threads via the `getrawtransaction` RPC. The `blockfilterindex` DB is called concurrently from both the P2P `cfilters` / `cfheaders` / `cfcheckpt` message handlers on the `msghand` thread, and from the `getblockfilter` RPC on the HTTP RPC worker threads. We have not yet been issuing concurrent reads against the chainstate DB, but there is no LevelDB-side reason we can't. In fact, the chainstate DB is already being touched by more than one thread on master, because LevelDB schedules its own background compaction work.
For reviewers:
The main change is `CoinsViewOverlay` gets 1 new public and 2 new private methods.
- `StartFetching`: public method called in lieu of `CreateResetGuard` before we enter `ConnectBlock`. It still returns a `ResetGuard` so the view is `Reset` before the block it is working on leaves scope. This kicks off worker threads who each just run `while (ProcessInput()) {}` and then return.
- `StopFetching`: private method called on `Reset` whenever the guard leaves scope or `Flush`. Stops all threads and clears multi threaded state.
- `ProcessInput`: private method that fetches a single input prevout. Returns `true` if an input was fetched and `false` otherwise. This is the only method on `CoinsViewOverlay` that is called concurrently by multiple threads. Every other method on the overlay is still called synchronously on the main thread.
The `CoinsViewOverlay::FetchCoinFromBase` method is also extended to lookup the coins fetched from `ProcessInput` first before falling back to `base->PeekCoin`.
Mutating methods `Reset` and `Flush` are overridden in `CoinsViewOverlay` to call `StopFetching` first.
[^1]: https://github.com/bitcoin/bitcoin/pull/31132#pullrequestreview-3515011880
[^2]: https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3767758819
[^3]: https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3617721711
[^4]: https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3678847806
[^5]: https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-4071032270
[^6]: https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3617315125
ACKs for top commit:
l0rinc:
reACK dc1c17c085
willcl-ark:
ACK dc1c17c085
theStack:
re-ACK dc1c17c085
ryanofsky:
Code review ACK dc1c17c085 with changes to StopFetching and AllInputsConsumed checking behavior since last review.
Tree-SHA512: 89c1c2890f65aac5cd546edc44504956c47b6fada256d3b86ced47e6dd8c72f633a4357753b3b9805b9ba6ed02790822090d70578aba2964baf50d7eb956864c
fa615bd163 refactor: Move LoadGenesisBlock to ChainstateManager (MarcoFalke)
Pull request description:
The function does not need anything from any chainstate, so it should not sit in the Chainstate class.
ACKs for top commit:
l0rinc:
Tested ACK fa615bd163
janb84:
reACK fa615bd163
sedited:
ACK fa615bd163
Tree-SHA512: 482b5c140faa35944a890c941fd185a896a3c04fec46d0cc6dd56830f62ee8fe5200fd13d25f7a80a5da0eb4fb42717f42c8bce837d933668432f5786f8380e3
Add a fuzz harness targeting ConnectionType::PRIVATE_BROADCAST.
Seeds m_tx_for_private_broadcast via InitiateTxBroadcastPrivate
so PushPrivateBroadcastTx reaches the send-INV and other paths.
Guarantees one PRIVATE_BROADCAST peer per iteration, optionally
adds peers of other types, uses CallOneOf() branching between
guided and arbitrary message types, and verifies the outbound
INV is well-formed after the handshake completes.
Co-authored-by: Greg Sanders <gsanders87@gmail.com>
9784818442 mining: add getTransactionsByWitnessID() IPC method (Sjors Provoost)
d282ae6883 mining: add getTransactionsByTxID() IPC method (Sjors Provoost)
0d5e4d4712 test: restart node after IPC option override test (Sjors Provoost)
f16b3613cd ipc: Serialize null CTransactionRef as empty Data (Sjors Provoost)
0f466e1094 mempool: add lookup by witness hash (Sjors Provoost)
Pull request description:
For Stratum v2 custom job declaration to be bandwidth efficient, the pool can request[^0] only the transactions that it doesn't know about.
The spec doesn't specify how this is achieved, but one method is to call the `getrawtransaction` RPC on each transaction id listed in [DeclareMiningJob](https://stratumprotocol.org/specification/06-Job-Declaration-Protocol?query=DeclareMiningJob#644-declareminingjob-client-server) (or a subset if the pool software maintains a cache). Using RPC is inefficient, made worse by the need to make multiple calls. It also doesn't support queuing by witness id (yet, see #34013).
This PR introduces two new IPC methods:
- `getTransactionsById()`: takes a list of `Txid`'s
- `getTransactionsByWitnessID()`: : takes a list of `Wtxid`'s
Both return a list of serialised transactions. An empty element is returned for transactions that were not found.
Unlike the RPC counterpart, the IPC methods do not take advantage of `-txindex`. This could be done in a followup. For `Wtxid` that would involve adding a `-witnesstxindex`.
I thought about having a single (or overloaded) `getTransactions()` that works with both `Txid` and `Wtxid`, but I prefer that clients are intentional about which one they want.
A unit and functional test cover the new functionality.
Sv2 probably only needs `getTransactionsByWitnessID()`, but it's easy enough to just add both.
To rest with Rust use:
- https://github.com/2140-dev/bitcoin-capnp-types/pull/11
[^0]: there's two reasons the pool requests these transactions: to approve the template and to broadcast the block if a solution is found (the miner will also broadcast via their template provider). See also https://github.com/stratum-mining/sv2-spec/issues/170
ACKs for top commit:
achow101:
ACK 9784818442
sedited:
Re-ACK 9784818442
ViniciusCestarii:
Re-ACK 9784818442
ismaelsadeeq:
Code review ACK 9784818442
Tree-SHA512: 3c6ceb572ab7d8bd090a8f31b5e331304a7a19a3d1f1551c9c2e1ee41339d76f96ca6c41bd634c87fca0a969e7d9bfa6a16c26fb06c0dd2315f6ca1c76a16a31
The function does not need anything from any chainstate, so it should
not sit in the Chainstate class.
Also, mark it [[nodiscard]], and the one place that ignores the return
value with (void).
Also, change the error log strings to not include the __func__, which is
redundant with -logsourcelocations. This is not a refactor, but this log
is only for debugging extremely rare errors.
4e29de719e private broadcast: add release note for limited cap (Gregory Sanders)
cbf8c107c1 Release cs_main between individual private tx re-attempts (Greg Sanders)
5aea3d0373 private broadcast: limit outstanding txs to count of 10,000 (Gregory Sanders)
Pull request description:
Add a belt-and-suspenders feature, limit the amount of memory and cpu possible when unlucky or simply misconfigured. The worst case limit is roughly 400kB * 10,000 = 4GB, regardless of usage pattern.
Before this change, sheer volume of broadcasts, mismatches in standardness rules, or simply fee mismatches may result in unbounded growth of memory usage. As the feature may be expanded in the future, explicit bounds helps reasoning going forward.
ACKs for top commit:
frankomosh:
tACK 4e29de719e. Ran private_broadcast_tests and p2p_private_broadcast_cap.py. Great to have an explicit bound as the belt-and-suspenders against unbounded queue growth.
vasild:
ACK 4e29de719e
andrewtoth:
ACK 4e29de719e
stickies-v:
ACK 4e29de719e
Tree-SHA512: 18161755f37d07cca185a09e782dbe2fd0025b8befd4f6660e988865cc3a9b705d41769b816161e8142fe6ce31a56e0288bd78efc25135cedfc47fc855011799
a99148d576 test kernel: Don't log on warnings change (sedited)
5b4fd284f4 kernel: Generate a signet with a challenge (sedited)
Pull request description:
Adds a function for creating a chainparams with a signet challenge to the kernel API. This was requested in issue https://github.com/bitcoin/bitcoin/issues/35362.
Also takes this opportunity to de-noise the test kernel binary log output a bit.
ACKs for top commit:
stickies-v:
ACK a99148d576
yuvicc:
lgtm! ACK a99148d576
musaHaruna:
reACK [a99148d](a99148d576)
Tree-SHA512: 44b414e1d43af59080c03940579307c205aa0533138bc94b67a1bfb72d347f3d3c5d7c349f0a4b3983456ee1face42794c1ed46d08154fc6f5b71d3d6915ae98
Add unit coverage for the -onlynet/-externalip interaction.
Check that an unreachable address still fails with normal AddLocal()
arguments, while the same address succeeds when the unreachable-net
override is set for explicit local-address configuration.
LevelDB bloom filters are only consulted on Get point reads.
They are not consulted for iterator seeks.
txospenderindex only reads via iterator seeks, so building
them is wasted effort and space.
The helper created a UniValue with hard-coded constants, which
isn't ideal for fuzz tests. Replace it in the ipc fuzz test with
parsing a UniValue directly from the fuzzed data provider.
f595daf1dd test: ensure HTTPServer race condition is fixed (Matthew Zipkin)
b98b10c072 test: introduce a worker thread in http socket error test (Matthew Zipkin)
922b08d375 test: socket error handling in HTTPServer using ErrorSock mock socket (Matthew Zipkin)
73da2a8a52 http: prevent race condition between worker thread and I/O thread (Matthew Zipkin)
Pull request description:
This prevents a losing race condition that could prevent the server from reading any more requests from an HTTP client.
Found and reported by the fuzzing department: 7fe5f54497
The Race:
A connected socket can either be written to or read from based on the result of `GenerateWaitSockets()`. That method checks the `HTTPRemoteClient` flag `m_send_ready`. If it's `true` the implication is that there is data in the client's send buffer ready to go. Once that data is sent and the buffer is empty, `MaybeSendBytesFromBuffer()` sets it `false` again.
The sad case was when a worker thread calling `WriteReply()` adds data to the send buffer, but before it sets `m_send_ready` to `true`, the I/O thread sends that data and empties the buffer. With the buffer unexpectedly empty, `WriteReply()` sets `m_send_ready` to `true`.
The effect of this is that the socket will stay in "write" mode with nothing to write. With nothing to write, `MaybeSendBytesFromBuffer()` never sets it back to `false` and the socket is stuck forever.
The Fix:
Simply move `m_send_ready = true` inside the block of `WriteReply()` where `m_send_mutex` is still held. This prevents the I/O thread from emptying the send buffer while the worker thread is setting the flag.
Testing:
To observe the race condition, revert the first commit `"http: prevent race condition between worker thread and I/O thread"` and run the unit test from the remainder of the branch. I like to see the logs:
`test_bitcoin --log_level=all --run_test=httpserver_tests -- --printtoconsole --debug=http --debug=lock'
The test will fail with a small probability. The socket will get stuck and the test will abort after a 60 second timeout. To garuntee the race condition loses and fail the test every time, slow down `WriteReply()` in the worker thread:
```diff
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index 99e30ff663..b0c7b516d8 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -614,6 +614,7 @@ void HTTPRequest::WriteReply(HTTPStatusCode status, std::span<const std::byte> r
} else {
// Inform HTTPServer I/O that data is ready to be sent to this client
// in the next loop iteration.
+ std::this_thread::sleep_for(500ms);
m_client->m_send_ready = true;
}
```
With the first commit (the fix) back in place, slowing down the worker thread like this won't fail the test.
Bonus:
The unit test is spread over three commits. First, a method of the socket testing setup is templated so a mock socket that intentionally raises an error can be inserted. The unit test added in that commit covers a race condition that was fixed in #35182 in response to https://github.com/bitcoin/bitcoin/pull/35182/changes#r3358889539 so we get the added benefit of covering an error path, and guaranteeing coverage of both "optimistic send" (directly from worker thread) and regular send (from a tick in the I/O loop thread).
The next commit adds a worker thread to the unit test, at which point a race condition is possible but very unlikely because all requests are sent at once. Finally, we spread out the requests in the top commit and make the race condition much easier to catch.
ACKs for top commit:
janb84:
crACK f595daf1dd
dergoegge:
utACK f595daf1dd
theStack:
Code-review ACK f595daf1dd
Tree-SHA512: 451982fd72724c4115e371fc6392605693d6c3207f00ffebcf027aae9253f7974b5b1165b9f46c91b5436d7fe60c7d27316fb0b79f729ab0bf8f32db2530075f
