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
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
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.
037ad77071 fuzz: add IPC round-trip target (Enoch Azariah)
8a739a5510 build: allow ipc fuzz builds (Enoch Azariah)
Pull request description:
As discussed in #23015, this PR adds an IPC fuzz target to exercise the Cap'n Proto/libmultiprocess serialization bridge using an in-process two-way pipe and a reflected test interface.
It covers round-trip serialization for `COutPoint`, `CScript`, `std::vector<uint8_t>`, `UniValue`, and transactions, and exercises libmultiprocess proxy/server interaction. The target guarantees at least one IPC operation per input and is included by default when IPC is enabled in fuzz builds
Coverage [report](https://marcofleon.github.io/coverage/ipc/) provided by marcofleon
ACKs for top commit:
marcofleon:
ACK 037ad77071
sedited:
Re-ACK 037ad77071
Tree-SHA512: aae8835c8a378886b36420c422136c743b235877f6fd6f0c681910903c68e18753598c7f0e0279ce2e64d627241ce3a4e08f7edecc2e38765569c9faa61bf45e
Add an ipc fuzz target behind ENABLE_IPC.
Set up an in-process two-way pipe and use a small reflected interface
to exercise libmultiprocess client/server calls.
Round-trip COutPoint, CScript, std::vector<uint8_t>, UniValue, and
transactions.
Add bitcoin_ipc_fuzz static library and link it to the fuzz target
via a new src/ipc/test/fuzz/CMakeLists.txt.
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.
2ee4fafa3f test: add fuzz test for private broadcast (kevkevinpal)
08b7c61fc7 private broadcast: enforce sending to unique node ids (Vasil Dimov)
Pull request description:
Add a fuzz test that exercises the public methods of the `PrivateBroadcast` class from `src/private_broadcast.h` and checks for correctness.
ACKs for top commit:
instagibbs:
ACK 2ee4fafa3f
nervana21:
re-ACK 2ee4fafa3f
frankomosh:
Code Review ACK 2ee4fafa3f.
Tree-SHA512: 35f9efcf9e7ea8bd071f6b607fd7c901b60ccf14610f501e6cf06f0d05c389424ab8a0c25a2aaffd5755c8f920af4834e0979cae72a474680170de2b94d06e1c
Introduce CoinsViewOverlay::StartFetching, which maps all input prevouts of a
block to a new m_inputs vector of InputToFetch elements. Returns a ResetGuard
which is lifetime bound to the block, while the InputToFetch elements are
lifetime bound to the block as well.
Inputs spending outputs of an earlier transaction in the same block won't
be in the cache or the db. They also won't be requested by FetchCoinFromBase,
so we filter them out while building m_inputs to not waste time trying to
fetch them. Build an unordered set of seen txids while flattening m_inputs and
skip any prevout whose hash is already in the set.
Introduce StopFetching to clear the m_inputs vector.
CCoinsViewCache::Reset is made virtual and is overridden in CoinsViewOverlay.
StopFetching is called on Reset, so the InputToFetch objects will not
exceed the lifetime of the block.
Introduce ProcessInput to fetch the utxo of an individual input in m_inputs.
Each caller fetches the input at m_input_head and increments it, so each call
will fetch the next input in the queue.
Fetch coins from the m_inputs vector in FetchCoinFromBase by comparing the
requested outpoint against the single input at m_input_tail. ConnectBlock
requests prevouts in the same order StartFetching queued them, and same-block
spends are filtered out, so the coin to serve is always the one at m_input_tail
(aside from BIP30 checks, an invalid block, or when the thread pool is not yet.
These cases fall back to base->PeekCoin).
This is designed deliberately so multiple threads can call ProcessInput independently.
Co-authored-by: l0rinc <pap.lorinc@gmail.com>
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
Introduce a ThreadPool shared pointer to CoinsViewOverlay. A pool managed
externally can be passed in the constructor.
A global thread pool is used in fuzz harnesses since iterations can happen
faster than the OS can create and tear down thread pools.
This can cause a memory leak when fuzzing.
Co-authored-by: l0rinc <pap.lorinc@gmail.com>
6fa4132298 fuzz: share a single mocked steady clock across FuzzedSock instances (Hao Xu)
Pull request description:
This is a follow-up of https://github.com/bitcoin/bitcoin/pull/35478#issuecomment-4667842057, inspired by maflcko .
Each FuzzedSock used to own its mocked steady clock and call MockableSteadyClock::SetMockTime() directly. Hold the clock by reference to an externally provided SteadyClockContext instead, so that several FuzzedSock instances sharing a test case (e.g. one per peer, or one created via Accept()) advance a single mocked clock, and the mocking goes through the SteadyClockContext RAII helper that resets mocktime on destruction.
SteadyClockContext is a LimitOne type, so each fuzz target constructs one instance per iteration and passes it to ConsumeSock / ConsumeNode / the FuzzedSock constructor.
ACKs for top commit:
maflcko:
review ACK 6fa4132298🌕
marcofleon:
crACK 6fa4132298
Tree-SHA512: 3c773b5c0c3ba42a8245c9ea6042b0bc767df4fad506305f3c200310616b48a59deb1542086eb4ce3e8a1407c4d6b42cef3b37cd84bfe80d4821972b8d3b4286
1a3cfdf1b7 fuzz: connman: cover AddLocalServices/RemoveLocalServices (Bruno Garcia)
c507fb3063 fuzz: connman: add outbound-bytes invariants (Bruno Garcia)
4a6fce43ea fuzz: connman: add AddNode/RemoveAddedNode invariants (Bruno Garcia)
a5859edef4 fuzz: connman: set m_local_services/m_use_addrman_outgoing/m_max_automatic_connections (Bruno Garcia)
4b84c9125a fuzz: connman: add network activity invariants (Bruno Garcia)
Pull request description:
This PR improves the `connman` fuzz target by replacing some "`(void)`" calls with actual invariant checks, adding coverage for previously uncovered methods, and exercising more initialization states.
- Set `m_local_services`, `m_use_addrman_outgoing`, and
`m_max_automatic_connections` via fuzzed values before `Init()` to
explore more startup configurations.
- Add network activity and outbound-bytes invariants.
- Add `AddNode`/`RemoveAddedNode` invariants: e.g. a successful `AddNode`
increases `GetAddedNodeInfo()` by one; adding the same node again
must fail; a subsequent `RemoveAddedNode` must succeed and restore
the original count.
- Add coverage for `AddLocalServices`/`RemoveLocalServices`.
ACKs for top commit:
nervana21:
re-ACK 1a3cfdf1b7
frankomosh:
reACK 1a3cfdf1b7 . Change from the diff is the restoring `(void)connman.RemoveAddedNode(random_string)` arm
sedited:
ACK 1a3cfdf1b7
Tree-SHA512: c7b6799ca65d2e639d8ab9ab0cc77bae663f24fbda934446a8ee2e8ce9e8e36624d16b4f492b1714e2d67375edd35907cb9392d21f368d3d5298275ff1d05c72
Exercise `CCoinsViewDB::CompactFullAsync()` from the `coins_view_db` fuzz target so the new chainstate compaction wrapper can run concurrently with ordinary coins view operations.
The fuzz operation only schedules compaction, matching production; outstanding work is waited for by the `CCoinsViewDB` destructor at the end of the fuzz input.
Each FuzzedSock used to own its mocked steady clock and call
MockableSteadyClock::SetMockTime() directly. Hold the clock by reference
to an externally provided FakeSteadyClock instead, so that several
FuzzedSock instances sharing a test case (e.g. one per peer, or one
created via Accept()) advance a single mocked clock, and the mocking goes
through the FakeSteadyClock RAII helper that resets mocktime on
destruction.
FakeSteadyClock is a LimitOne type, so each fuzz target constructs one
instance per iteration and passes it to ConsumeSock / ConsumeNode / the
FuzzedSock constructor.
48df0939e7 fuzz: Remove unnecessary thread pool mutexes (marcofleon)
a4c3b003f8 fuzz: Speed up dbwrapper_concurrent_reads harness (marcofleon)
Pull request description:
Limiting how many read queries each worker executes significantly speeds up this test, especially when running with sanitizers. This still builds the full query list, and then takes the first 128/2000 after each worker shuffles it. I think that keeps some more operation diversity vs just lowering the query max directly. It also allowed me to reuse and test against a corpus I already had. Let me know if I'm wrong, but I don't think this test needs every worker to execute an identical query list to be effective.
This PR also reverts the `num_entries` max from 3000 back to 5000, as that didn't have much effect on input speed and restores a bit of lost coverage.
Lastly, as https://github.com/bitcoin/bitcoin/pull/35455#discussion_r3394357385 points out, remove the unnecessary `Mutex` from `StartReadPoolIfNeeded()`. Fuzz targets are entered sequentially within a process and parallel fuzzing uses separate processes/forks, so a mutex to prevent two in-process callers from racing to start the pool isn't needed.
ACKs for top commit:
sedited:
ACK 48df0939e7
brunoerg:
ACK 48df0939e7
Tree-SHA512: 24c35e13fa26790b36e5190283b78dd8511165d576841cb701f541a36c5b7c0a73f9bc265e71598a879fa1df9a9438e7841736f72b967214196fa6fe68569eff
Remove the `Mutex` from the `threadpool` and `dbwrapper_concurrent_reads`
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.
Limit how many read queries each worker executes. This significantly
speeds up the test, as each worker runs >90% fewer (2000 to 128)
expensive LevelDB operations (like `IteratorSeek`) but still
ends up hitting the intended target code.
Revert the `num_entries` max from 3000 back to 5000, as that didn't
have much effect on input speed and restores a bit of lost coverage.
This is a refactor to prepare for matching the API of HTTPRequest
definitions in both namespaces http_bitcoin and http_libevent. In
particular, to provide a consistent return type for GetRequestMethod()
in both classes.
This commit is a no-op to isolate HTTP methods and objects that
depend on libevent. Following commits will add replacement objects
and methods in a new namespace for testing and review before
switching over the server.
SteadyClockContext and FakeNodeClock are both LimitOne RAII helpers that mock a
clock in tests -- the steady clock and the node clock, respectively. Rename the
former so the two follow a consistent FakeXClock naming scheme.
-BEGIN VERIFY SCRIPT-
sed -i 's/SteadyClockContext/FakeSteadyClock/g' $(git grep -l SteadyClockContext)
-END VERIFY SCRIPT-
1ce9e26239 fuzz: improve dbwrapper_concurrent_reads performance (Andrew Toth)
Pull request description:
The recently merged fuzz harness targeting concurrent reads suffers from poor performance and memory leaks (https://github.com/bitcoin/bitcoin/pull/34866#issuecomment-4614323925).
Fix this by
- using a global thread pool instead of a local one per iteration
- reduce thread count to 8 from 16
- use a std::map oracle to check results inline instead of reading from the db to get a baseline and storing results
ACKs for top commit:
marcofleon:
reACK 1ce9e26239
l0rinc:
ACK 1ce9e26239
sedited:
ACK 1ce9e26239
Tree-SHA512: 2e532caf246f389105e4a9b487496386d1fe9add7b27fba9ecbbf51a432ef493765ad7095288dd7e0a896860ff150d89ecb6afb8baf311a4af94d8e01b77dba5
526aae3768 fuzz: test non-max descriptor satisfaction weight (woltx)
Pull request description:
The descriptor fuzz target is intended to exercise descriptor satisfaction-size estimation for solvable descriptors.
It currently calls `MaxSatisfactionWeight(true)` twice, so the `false` branch is never exercised.
This PR changes `max_sat_nonmaxsig` to call `MaxSatisfactionWeight(false)`, so fuzzing covers both branches.
ACKs for top commit:
brunoerg:
reACK 526aae3768
sedited:
ACK 526aae3768
Tree-SHA512: 029750d76c1d50f5c6a008b826a0a2dc187feb420be96401d2e15747b44901341d32ac75e86a5e10585919d419c607800d8e117a1cbce50b1db40121d3610f9c
Also assert that the availability of the satisfaction weight estimate
does not depend on the signature-size assumption, and that assuming
non-max-size signatures never increases the estimate.
21a1380c13 key: cleanse ChainCode on destruction (Thomas)
b3a3f88346 crypto: cleanse HMAC stack buffers after use (Thomas)
Pull request description:
`CHMAC_SHA256` and `CHMAC_SHA512` leave two stack buffers populated on return: `rkey[]` holds `K' ⊕ ipad` after the constructor, and `temp[]` holds the inner-hash output after `Finalize()`.
When the HMAC is keyed with sensitive material (chain code in `BIP32Hash()` in `hash.cpp` for BIP32 child key derivation; PRK in HKDF-Expand in `hkdf_sha256_32.cpp`, used for BIP324 transport keying), `rkey` is one constant XOR from that key, and `temp` is a one-way digest covering it.
This PR cleanses both buffers with `memory_cleanse()`, matching the convention already used in `chacha20.cpp` and `chacha20poly1305.cpp`. No observable change for callers.
Update: Cleansing the HMAC primitive's internal buffers still leaves a caller's `ChainCode` value populated in memory after use. The second commit promotes `ChainCode` from `typedef uint256` to a `base_blob<256>` subclass with a `memory_cleanse()` destructor, so chain codes in `CExtKey`, `CExtPubKey`, and local variables are cleansed on scope exit. `MUSIG_CHAINCODE` is retyped from `constexpr uint256` to `const ChainCode` to match its BIP328 semantic role; this also removes the GCC-14 consteval lambda workaround.
ACKs for top commit:
davidgumberg:
crACK 21a1380c13
optout21:
ACK 21a1380c13
achow101:
ACK 21a1380c13
winterrdog:
ACK 21a1380c13
Tree-SHA512: 022c8372da3e2c9c269ef55b695d8415241acf64be04692f30da0e682dd1d05178f95601a3bd208573fd0630656b3dedcf6de34a2a3cf794515c0268e710af75
19b32a2e18 fuzz: reset the mockable steady clock between iterations (Hao Xu)
Pull request description:
Fix the issue mentioned by https://github.com/bitcoin/bitcoin/issues/29018#issuecomment-4422112607
And this is my investigation on it: https://github.com/bitcoin/bitcoin/issues/29018#issuecomment-4639472489
`CheckGlobalsImpl`'s constructor runs at the start of every fuzz iteration and already resets the global RNG flags and the mockable `NodeClock` (`SetMockTime(0s)`), but it never reset the mockable steady clock. A value written to `g_mock_steady_time` by one input therefore leaks into the next iteration.
The most common source is `FuzzedSock`'s constructor, which calls `SetMockTime(INITIAL_MOCK_TIME)` (through `ElapseTime(0s)`) and never clears it: once any input constructs a `FuzzedSock`, the steady clock stays mocked for every subsequent iteration in the same process. This is one of the global-state leaks tracked
in #29018.
### Fix
Reset `MockableSteadyClock` symmetrically with `NodeClock`:
```diff
g_used_system_time = false;
SetMockTime(0s);
+MockableSteadyClock::ClearMockTime();
```
Besides removing the leak, this puts the steady clock under the same discipline as the system clock: a target that reads `MockableSteadyClock::now()` without first mocking it (via `FuzzedSock`, `SteadyClockContext`, …) is now caught by the existing `g_used_system_time` check at the end of the iteration, instead of
silently reusing a value left over from a previous input.
Clearing in `~FuzzedSock()` would be wrong: several `FuzzedSock`s can be alive simultaneously (e.g. `process_messages` adds 1–3 peers), so clearing in one destructor would corrupt the mock observed by the others. Resetting at the iteration boundary keeps it decoupled from socket lifetimes.
### Testing
Verified with the global-state-detector approach from #29018 (snapshotting/diffing the writable globals around each iteration):
- **Before:** a single empty input to `process_message` reports `g_mock_steady_time` changing `00 → 01` (`0` → `INITIAL_MOCK_TIME`).
- **After:** that report is gone; the only remaining diffs are the benign one-time initialization of `ConsumeTime`'s function-local statics.
`p2p_headers_presync` (uses `SteadyClockContext`) and `pcp_request_port_map` (uses `FuzzedSock`) still run to `succeeded` without aborting, confirming existing steady-clock readers are unaffected.
This leak is invisible to coverage-based checks such as `deterministic-fuzz-coverage`, because `g_mock_steady_time` is only consumed through coarse time comparisons (e.g. the 250 ms presync rate-limiter): a changed value doesn't change the executed branches, so only a memory-diffing detector can see it.
ACKs for top commit:
maflcko:
lgtm ACK 19b32a2e18
marcofleon:
Nice catch, ACK 19b32a2e18
Tree-SHA512: b875795addb2914eae489adc703438483f8e464b9a210bd5d76189f13266dae5843c8749590d59e78bf171f19aa7cee21ca678cd311843d8a88cbe9831f20b6a
The previous name did not indicate the type was intended for
testing. Renaming to FakeNodeClock makes this explicit and
allows call sites to drop the ctx suffix on the variable name.
Suggested in #34858 review feedback.
-BEGIN VERIFY SCRIPT-
s() { git grep -l "$1" -- src | xargs sed -i "s/$1/$2/g"; }
s '\<NodeClockContext\>' 'FakeNodeClock'
s '\<clock_ctx\>' 'clock'
-END VERIFY SCRIPT-
`DeserializeHDKeypaths()` was writing into the original `hd_keypaths`
map instead of `deserialized_hd_keypaths`. As a result the latter was
always empty and the round-trip assertion following was trivially true,
so the serialize/deserialize round-trip wasn't actually being exercised.
That bug was introduced with the commit introducing the fuzz target
(commit f898ef65c9, #18994).
CheckGlobalsImpl's constructor runs at the start of every fuzz iteration
and already resets the global RNG flags and the mockable NodeClock via
SetMockTime(0s), but it never reset the mockable steady clock. A value
written to g_mock_steady_time by one input therefore leaked into the
next one. For example, FuzzedSock's constructor calls
SetMockTime(INITIAL_MOCK_TIME) and never clears it, so the mocked steady
time stays set for all subsequent iterations.
Reset MockableSteadyClock symmetrically with NodeClock so each input
starts from an unmocked steady clock. This also brings the steady clock
under the same discipline as the system clock: a target that reads
MockableSteadyClock::now() without first mocking it is now caught by the
existing g_used_system_time check instead of silently reusing a leaked
value.
bf0d257c11 net: un-default the OpenNetworkConnection()'s proxy_override argument (Eugene Siegel)
5a3756d150 test: add a regression test for private broadcast v1 retries (Vasil Dimov)
ab35a028ed test: make reusable filling of a node's addrman (Vasil Dimov)
2333be9cbc test: make reusable starting a standalone P2P listener (Vasil Dimov)
2ffa81fac4 test: make reusable SOCKS5 server starting (Vasil Dimov)
32d072a49f doc: add release notes for #35319 (Vasil Dimov)
d01b461f71 net: ensure no direct private broadcast connections (Vasil Dimov)
fd230f942d net: use the proxy if overriden when doing v2->v1 reconnections (Vasil Dimov)
Pull request description:
This PR includes https://github.com/bitcoin/bitcoin/pull/35319 and on top of that adds a regression functional test.
The functional test exercises the relevant code paths without modifying non-test code. To do that it does:
* Add a bunch of IPv4 addresses to the node's addrman (they will be added without P2P_V2 flag).
* Get them to report P2P_V2 in their service flags and connect to each one, so that the flags
in addrman are updated to contain P2P_V2.
* Get one successful connection to a Tor peer (.onion) so that bitcoind assumes the configured
Tor proxy works and is indeed a proxy to the Tor network. This will make it open private
broadcast connections also to IPv4 addresses via that proxy.
* Start some private broadcast connections.
* Remember the destination IPv4 address of the first connection and get it to fail the v2
transport.
* Wait for a subsequent connection also through the Tor proxy to the same IPv4 and expect
it to be v1, i.e. the v2->v1 downgrade retry.
The test fails without the fix - the v1 retry never arrives to the Tor proxy. And passes with the fix. The fix is in the first commit here and in https://github.com/bitcoin/bitcoin/pull/35319, can remove it by `git show fd230f942d | git apply -R`.
ACKs for top commit:
Crypt-iQ:
reACK bf0d257c11
andrewtoth:
ACK bf0d257c11
instagibbs:
ACK bf0d257c11
sedited:
utACK bf0d257c11
Tree-SHA512: 11e89be36577199e0312e5e63efeac04e295faaba1cf1c13a30e683d35f473c8dbb419d1897b0333c2e993c10637adecafcf90fe08c812065c793cbc903744c9
107d4178d9 versionbits: update VersionBitsCache doc comment to match current behaviour (Antoine Poinsot)
94e3ac0b21 doc: release notes and bips doc update for #34779 (Antoine Poinsot)
1d5240574a qa: test we don't warn for ignored unknown version bits deployments (Antoine Poinsot)
f802edf57c versionbits: Limit live activation params and activation warnings per BIP323 (Anthony Towns)
Pull request description:
This implements https://github.com/bitcoin/bips/pull/2116, which repurposes 24 version bits as extra nonce space for miners rather than soft fork deployment coordination. 24 bits allows a miner to perform up to 72 PH before needing a fresh job from its controller. The current 16 bits in use by miners only allow up to 280 TH, which [apparently led some ASIC designers to start rolling the timestamp field](https://github.com/bitaxeorg/ESP-Miner/pull/1553#issuecomment-3937736319) on their beefier machines.
Mailing list discussion available [here](https://gnusha.org/pi/bitcoindev/6fa0cb45-37d6-4b41-9ff8-03730fd96d6e@mattcorallo.com/). A previous shot at this is https://github.com/bitcoin/bitcoin/pull/13972 (with a smaller extranonce space).
This change only affects the warning logic.
ACKs for top commit:
ajtowns:
ACK 107d4178d9
achow101:
ACK 107d4178d9
sedited:
Re-ACK 107d4178d9
optout21:
ACK 107d4178d9
Tree-SHA512: cfaf5d7de1e8c020a4d7f4b1096b6c3e0e3b41ea840a4652ebcdabc345c5c557161c8304f1d7d6de541a2bf1df3c855ad7b64e49dd8c8af3937876d134bb5aba
8cb8653a22 fuzz: target concurrent leveldb reads (Andrew Toth)
6609088fe6 fuzz: extract ConsumeDBParams helper (Andrew Toth)
Pull request description:
Inspired by https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-4054461591.
We currently do concurrent leveldb reads when accessing our indexes.
1. `txindex` - we call `FindTx()` from multiple RPC threads.
2. `blockfilterindex` - we call `LookupFilter/Header()` concurrently from `msghand` thread for p2p requests as well as RPC threads.
3. `coinstatsindex` - we call `LookUpStats()` from multiple RPC threads.
4. `txospenderindex` - we call `FindSpender()` from multiple RPC threads.
We also read from our chainstate and blocks index while background compactions are writing.
While OSS-Fuzz does cover leveldb (https://github.com/google/oss-fuzz/blob/master/projects/leveldb/fuzz_db.cc), it doesn't cover multi threaded access. Without a deterministic hypervisor this fuzz harness won't be deterministic, but we can at least run it with TSan to get a higher confidence that the synchronization code in leveldb is correct. Hopefully other reviewers find this useful.
This harness creates a threadpool with 16 threads, and then creates an in-memory levelDB which it seeds with deterministically random values. It chooses a random set of keys to query. It first performs all queries on the db on a single thread to get a baseline, then synchronizes all threads on a latch so they hit the db at the same time. Each thread performs the same queries, and afterwards are all checked against the baseline.
It uses a `DeterministicEnv` to capture background compaction work when seeding the db, which is also run immediately after the latch is released. This causes a race between compaction and reading, ensuring we exercise many thread synchronization code paths in leveldb.
I ran both TSan and ASan/UBSan overnight with no issues.
ACKs for top commit:
fjahr:
Code review ACK 8cb8653a22
l0rinc:
ACK 8cb8653a22
Tree-SHA512: 2ca31a824715b92e258c84ecf0c762f43ee2a528e3a3192f94d8aaeddf6e99f820a0297ce9efcc95bc32c7ec74489f240a25bab856d724d768117a7d95a33974
Pull the inline DBParams construction out of TestDbWrapper into a shared
ConsumeDBParams() helper. This is a pure refactor with no behavior change,
preparing for an additional harness that needs to build the same params.
This partially reverts the changes from commit
020166080c to testnet4_miner_tests.cpp
Also, remove some confusing IWYU pragmas. Those were inconsistently
added in 8c58f63578, but without any
rationale why adding them is the correct approach. The correct approach
should be done in a proper follow-up, with a clear rationale.
1e5d3b4f0d doc: add release note for mining option validation (Sjors Provoost)
0317f52022 ci: enforce iwyu for touched files (Sjors Provoost)
8c58f63578 refactor: have mining files include what they use (Sjors Provoost)
3bb6498fb0 mining: store block create options in NodeContext (Sjors Provoost)
4637cd157d mining: reject invalid block create options (Sjors Provoost)
8daac1d6eb mining: add block create option helpers (Sjors Provoost)
128da7c3ff miner: add block_max_weight to BlockCreateOptions (Sjors Provoost)
fa81e51eae mining: parse block creation args in mining_args (Sjors Provoost)
020166080c mining: use interface for tests, bench and fuzzers (Sjors Provoost)
44082bea47 interfaces: make Mining use const NodeContext (Sjors Provoost)
d4368e059c move-only: add node/mining_types.h (Sjors Provoost)
6aeb1fbea2 test: cover IPC blockmaxweight policy (Sjors Provoost)
63b23ea1e9 test: regression test for waitNext mining policy (Sjors Provoost)
24750f8b31 test: add createNewBlock failure helper (Sjors Provoost)
63ee9cd15b test: misc interface_ipc_mining.py improvements (Sjors Provoost)
Pull request description:
Although this PR is primarily a refactor, _there are behavior changes_ documented in the release note:
- the IPC mining interface now rejects out-of-range block template options instead of silently clamping them;
- startup now rejects `-blockmaxweight` values lower than `-blockreservedweight`, instead of allowing them to be clamped later.
The interaction between node startup options like `-blockreservedweight` and runtime options, especially those passed via IPC, is confusing.
They're combined in `BlockAssembler::Options`, which this PR gets rid of in favour of `BlockCreateOptions`.
`BlockCreateOptions` is used by interface clients. As before, IPC clients have access to a safe / sane subset, whereas RPC and test code can use all fields. The same type is also used to store mining defaults parsed once during node startup in `NodeContext`.
The maximum block weight setting (`block_max_weight`) is optional. When read from startup options it matches `-blockmaxweight`; when provided by callers it is a runtime override. `Merge()` fills unset fields from startup defaults while preserving caller-provided values.
This all happens in commits `mining: add block create option helpers` and `mining: store block create options in NodeContext`, and requires some preparation to keep things easy to review.
We get rid of `BlockAssembler::Options` but this is used in many tests. Since large churn is inevitable, we might as well switch all tests, bench and fuzzers over to the Mining interface. The `mining: use interface for tests, bench and fuzzers` commit does that, dramatically reducing direct use of `BlockAssembler`. Two exceptions are documented in the commit message. Because `test_block_validity` wasn't available via the interface and the block_assemble benchmark needs it, it's moved from `BlockAssembler::Options` to `BlockCreateOptions` (still not exposed via IPC).
We need access to mining related structs from both the miner and node initialization code. To avoid having to pull in all of `BlockAssembler` for the latter, the `move-only: add node/mining_types.h` commit introduces `node/mining_types.h` and moves `BlockCreateOptions`, `BlockWaitOptions` and `BlockCheckOptions` there from `src/node/types.h`.
I considered also moving `DEFAULT_BLOCK_MAX_WEIGHT`, `DEFAULT_BLOCK_RESERVED_WEIGHT`, `MINIMUM_BLOCK_RESERVED_WEIGHT` and `DEFAULT_BLOCK_MIN_TX_FEE` there from `policy.h`, since they are distinct from relay policy and not needed by the kernel. But this seems more appropriate for a follow-up and requires additional discussion.
---
I kept variable renaming and other formatting changes to a minimum to ease review with `--color-moved=dimmed-zebra`.
## Commit summary
Tests and test cleanup:
- `test: misc interface_ipc_mining.py improvements`
- `test: add assert_create_fails helper`
- `test: regression test for waitNext mining policy`
- `test: cover IPC blockmaxweight policy`
Refactoring test/bench/fuzz callers:
- `interfaces: make Mining use const NodeContext`
- `mining: use interface for tests, bench and fuzzers`
Moving mining interface types:
- `move-only: add node/mining_types.h`
Separating startup defaults from runtime options:
- `mining: parse block creation args in mining_args`: adds `node/mining_args.{h,cpp}` and moves mining option parsing out of `init.cpp`, without storing the parsed values yet.
- `miner: add block_max_weight to BlockCreateOptions`: moves the runtime maximum block weight setting into `BlockCreateOptions` as an optional value, so it can later be defaulted from startup args when unset.
- `mining: add block create option helpers`: centralizes block template option defaulting and merging, removes `BlockAssembler::Options`, and preserves behavior except for dropping the `Specified ` prefix from startup option error messages.
- `mining: reject invalid block create options`: checks typed `BlockCreateOptions` before block template creation, so invalid runtime options are rejected instead of silently clamped. Startup validation also rejects `-blockmaxweight` values lower than `-blockreservedweight`.
- `mining: store block create options in NodeContext`: stores the startup mining options in `NodeContext` as `BlockCreateOptions`, so startup defaults and runtime overrides can be merged with the same option type.
Include hygiene, CI and release note:
- `refactor: have mining files include what they use`
- `ci: enforce iwyu for touched files`
- `doc: add release note for mining option validation`
ACKs for top commit:
w0xlt:
reACK 1e5d3b4f0d
sedited:
ACK 1e5d3b4f0d
ryanofsky:
Code review ACK 1e5d3b4f0d. Looks good, thanks for the updates!
Tree-SHA512: 28c715023cb78f02775caa787b243c994bd0f8ce4559afc8db9301e93400ebbc74963626a4afe65ae15bcc16b9192d051a745839f4c804848d50746ea5a224b4
Private broadcast connections use either Tor or I2P, which require a
proxy intrinsically or IPv4 or IPv6 which must use a proxy in the
context of private broadcast to avoid leaking the originator's IP
address.
Add a safety check to guard against future mistakes.
Co-authored-by: Andrew Toth <andrewstoth@gmail.com>
dfe5d6a81d fuzz: apply node context reset pattern to p2p_handshake (frankomosh)
Pull request description:
Follow-up to #34302. Applies the node context reset pattern from fabf8d1 to `p2p_handshake`.
Previous code pattern created local `AddrMan` and `node::Warnings` objects, and passed them to `PeerManager::make`. `connman` was left holding a dangling `reference_wrapper<AddrMan>` across iterations, since the local objects destruct at iteration end while `connman` is global.
Like in fabf8d1 , reset and reinstall `node.addrman` and `node.peerman` on each iteration. This PR also removes `includes` made unused by this or prior refactors.
ACKs for top commit:
nervana21:
tACK dfe5d6a81d
maflcko:
review ACK dfe5d6a81d🦏
sedited:
ACK dfe5d6a81d
Tree-SHA512: 141ddec03c6d37f76a3b2d94701d18c851e85ea74e57716abb69ecc955d30371e342c6e267d2669ad853fe2d95fb77dd2fb506e4233ae3a88501d59ee1bbae30
