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
fa03852e9c test: Use SteadyClockContext in pcp_tests (MarcoFalke)
fa3716c439 test: Use FakeNodeClock in more places (MarcoFalke)
fae9623c8d test: Add FakeNodeClock m_clock to TestChain100Setup (MarcoFalke)
Pull request description:
This switches the remaining cases in the unit tests from `SetMockTime` to `FakeNodeClock` for clarity, as explained in the commit messages.
ACKs for top commit:
frankomosh:
crACK fa03852e9c. This PR continues the FakeNodeClock migration from #35114.
sedited:
ACK fa03852e9c
w0xlt:
ACK fa03852e9c
Tree-SHA512: 4d4f1ff170ce8cfa606a6dc0dc47ff8b89e2db0d0188daeabbbaa422df00143fd99426905adcff1fd152a00ebf8a9004e312cbc77a1d078e57895d5f1334a3b2
f6bdbcf79d lint: Grep for `AUTO` test suites in file names (rustaceanrob)
Pull request description:
Tests without a fixture did not have their file names linted because the grep matches on `BOOST_FIXTURE`. Updates to match `BOOST_FIXTURE` or `BOOST_TEST`.
ACKs for top commit:
l0rinc:
ACK f6bdbcf79d
achow101:
ACK f6bdbcf79d
hebasto:
ACK f6bdbcf79d.
Tree-SHA512: dd1763b6ac90fa87b7e0d2faa56d1c7beedb1e2d37d16367c60ebcadd155f5955113fff7cf5c0ce5eaa9e63aeeb67ffff2c8e081f7c23978cb072207f072f2ef
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.
2189a6f5f2 p2p: Saturate LocalServiceInfo::nScore updates at INT_MAX (codeabysss)
Pull request description:
The overflow for signed arithmetic yields undefined behavior.
This changes prevents undefined behavior in local address scoring by saturating `nScore` updates at `INT_MAX` in both `SeenLocal()` and `AddLocal()` update paths.
Fixes: #24049.
ACKs for top commit:
Crypt-iQ:
ACK 2189a6f5f2 pending CI
achow101:
ACK 2189a6f5f2
sedited:
ACK 2189a6f5f2
Tree-SHA512: b861e58ec9d6e18b17768f5cbee31ee825717e1a7216c332eb6fcbe63a7ac24e213ba638aea6f03cb710d9c2d8fe736cc626f11011ed66c3938acf6c38b0ef2a
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
This is easier to reason about, because it will automatically take care
of properly setting INITIAL_MOCK_TIME in the ctor. Also, it allows to
drop the ElapseTime and replace it with a call to operator+=()
The context is easier to reason about: E.g.,
* in TestBasicMining it allows to drop manual SetMockTime(0) calls,
* in connections_desirable_service_flags it allows to drop manual calls
to SetMockTime(GetTime<std::chrono::seconds>() + _n_) and replace them
by operator+=(_n_)
* in wallet_tests it clarifies that the mocktime does not persist
outside the AddTx function
Currently, all test cases using TestChain100Setup or a derived class
like BuildChainTestingSetup are using mocktime by default due to the
SetMockTime call in the TestChain100Setup ctor.
This is confusing, because test cases using mocktime explicitly seem to
imply that before they set the mocktime, real time was used.
E.g. index_reorg_crash claimed in a comment to "Enable mock time".
Fix this issue by adding a FakeNodeClock m_clock field to
TestChain100Setup. Then, use the m_clock instead of explicit calls to
SetMockTime or to a (now) shadowing local FakeNodeClock variable.
35a814a045 test: Limit clocks to one active instance (MarcoFalke)
55e402ffef scripted-diff: Rename NodeClockContext to FakeNodeClock (seduless)
1e9546fcf4 test: Use NodeClockContext in more call sites (seduless)
758fea59a8 test: Drop ++ from NodeClockContext default constructor (seduless)
7c2ec3949a test: Enter mocktime before peer creation in block_relay_only_eviction (seduless)
Pull request description:
Follow-up to #34858
Updates remaining `SetMockTime` call sites that are clean, mechanical swaps fitting the spirit of the original PR (see: https://github.com/bitcoin/bitcoin/pull/34858#pullrequestreview-4031647119 and https://github.com/bitcoin/bitcoin/pull/34858#issuecomment-4221757881). Further updates to `SetMockTime` are more complex and deserve separate, isolated PRs.
The default constructor for `NodeClockContext` increments to the next tick, which is a defensive measure to prevent time going backwards on construction. This has caused some confusion (see thread: https://github.com/bitcoin/bitcoin/pull/34858#discussion_r3057648646) and can be safely removed after updating the only test where this is load-bearing (b3c9bd7f2df230525c8e339394a315a2c500055d) (see: https://github.com/bitcoin/bitcoin/pull/34858#discussion_r3091085328). The removal also tightens the `addrman_tests/addrman_evictionworks` test to sit exactly on the `ADDRMAN_REPLACEMENT` boundary (4h), catching mutations such as:
```diff
diff --git a/src/addrman.cpp b/src/addrman.cpp
index d3dae59ae7..d0929c62cb 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -920,3 +920,3 @@ void AddrManImpl::ResolveCollisions_()
// Has successfully connected in last X hours
- if (current_time - info_old.m_last_success < ADDRMAN_REPLACEMENT) {
+ if (current_time - info_old.m_last_success <= ADDRMAN_REPLACEMENT) {
erase_collision = true;
```
The last follow-up item is updating `NodeClockContext` to `FakeNodeClock` to make it clear it is intended for testing (motivated by https://github.com/bitcoin/bitcoin/pull/34858#pullrequestreview-4082110904 and supported in https://github.com/bitcoin/bitcoin/pull/34858#issuecomment-4214352770).
ACKs for top commit:
maflcko:
re-ACK 35a814a045🛒
sedited:
ACK 35a814a045
Tree-SHA512: ade776e288a4b7bbc4c8855c14d61381b5b20329fe1e72fee87f773e47a9519975d58c277fbacda37dd73c0c1d4ce358c92dcdc4ca049d58cb3453ddf751b45b
fba713a28c scripted-diff: Rename UNIQUE_NAME to BITCOIN_UNIQUE_NAME (Hennadii Stepanov)
Pull request description:
https://github.com/bitcoin/bitcoin/pull/34454#issuecomment-3822800049:
> ... it is annoying that we keep running into the same bug over and over again (IIRC it happened in the past at least once for Bitcoin Core). Surely this is going to happen again in the future.
And here we go again.
---
The `nb30.h` Windows header [defines](b536c4fdb0/mingw-w64-headers/include/nb30.h (L78)) `UNIQUE_NAME` as a macro.
This introduces a fragile dependency on header inclusion order: if Windows headers happen to be included before `UNIQUE_NAME` is used, the preprocessor expands it into a numeric literal, causing syntax errors.
Rename the macro to `BITCOIN_UNIQUE_NAME` to remove this fragility and avoid the collision entirely.
---
Noticed while doing a Guix build of the [QML repo](https://github.com/bitcoin-core/gui-qml) for Windows.
Recent similar PRs: https://github.com/bitcoin/bitcoin/pull/34454 and https://github.com/bitcoin/bitcoin/pull/34868.
ACKs for top commit:
maflcko:
lgtm ACK fba713a28c
sedited:
ACK fba713a28c
w0xlt:
ACK fba713a28c
Tree-SHA512: 7a63b99a754e797eb8fa5d6a598606150f47ae1130d1d26067c509830e6575f0378ce63fe0ca35c69dce9a394451a34ddadd8b3d5f6f9a7e4c529108af546fb6
d0b76c7f3e rpc+bitcoin-tx: Specify correct type for ParseFixedPoint() (Hodlinator)
43ca54ca00 refactor(test): Make CAmount arg explicit for BuildCreditingTransaction() (Hodlinator)
b5e91e946c wallet: Remove CoinsResult::Clear() (Hodlinator)
Pull request description:
The *knapsack_solver_test* in *coinselector_tests.cpp* was accumulating satoshi amounts beyond 21M BTC. This was uncovered while experimenting with adding checks to `CAmount`. Fix that by fully resetting the `CoinsResult` object accumulating those amounts, inspired by https://github.com/bitcoin/bitcoin/issues/35449#issuecomment-4613968627.
Also, while we're at it, add 2 commits which correct some `int64_t`/`CAmount` confusion.
Fixes https://github.com/bitcoin/bitcoin/issues/35449
ACKs for top commit:
sedited:
ACK d0b76c7f3e
furszy:
utACK d0b76c7f3e
brunoerg:
code review ACK d0b76c7f3e
Tree-SHA512: 6d989ded6f6327dc657f437dc256d4adf42a34a1252621421ee38d7851c6cdc97a462f033a4728e3aa7d5514deee4db6e83646105633f9cf7ed6e7e90406b67d
The `nb30.h` Windows header defines `UNIQUE_NAME` as a macro.
This introduces a fragile dependency on header inclusion order: if
Windows headers happen to be included before `UNIQUE_NAME` is used, the
preprocessor expands it into a numeric literal, causing syntax errors.
Rename the macro to `BITCOIN_UNIQUE_NAME` to remove this fragility and
avoid the collision entirely.
-BEGIN VERIFY SCRIPT-
sed -i 's/\<UNIQUE_NAME\>/BITCOIN_UNIQUE_NAME/g' $(git grep -l 'UNIQUE_NAME' ./src/)
-END VERIFY SCRIPT-
ec6cf49b91 blockstorage: Remove cs_LastBlockFile recursive mutex (sedited)
Pull request description:
The `cs_LastBlockFile` mutex is redundant: all critical sections are already covered by cs_main. This is demonstrated in this patch by replacing all instances of locking `cs_LastBlockFile` with pairs of `AssertLockHeld(::cs_main)` and `EXCLUSIVE_LOCKS_REQUIRED(::cs_main)` annotations. No additional `::cs_main` LOCK(...)s are introduced (besides for test-only code).
It is also not clear for which sections `cs_LastBlockFile` is responsible for. It is annotated for `m_blockfile_cursors`, but sporadically and inconsistently also covers `m_blockfile_info` (e.g. in `LoadBlockIndexDB`).
Since it has no semantic meaning, and seems confusing to developers, remove it.
An alternative to this patch would be expanding the scope of what `cs_LastBlockFile` covers and turning it into a non-recursive mutex. I prepared such a patch some time ago, but found it unsatisfactory. It was not clear to me if the lock was now covering too much or too little, and its purpose remained unclear. If this patch is accepted, I would expect the project to eventually implement a separate, narrowly-scoped block storage lock to allow for a more parallelizable block processing routine.
ACKs for top commit:
stickies-v:
re-ACK ec6cf49b91
janb84:
re- ACK ec6cf49b91
pablomartin4btc:
ACK ec6cf49b91
Tree-SHA512: e5942bc87300b0db9a0b91d5fe26dab455049e6cef7c96bb12b28141fa04711d46c6af105c0e1a83a9f261edde2c8b8b43ecf577a27d54b4610d784676a85627
The cs_LastBlockFile mutex is redundant: all critical sections are
already covered by cs_main. This is demonstrated in this patch by
replacing all instances of locking cs_LastBlockFile with pairs of
`AssertLockHeld(::cs_main)` and `EXCLUSIVE_LOCKS_REQUIRED(::cs_main)`
annotations. No additional `::cs_main` LOCK(...)s are introduced.
It is also not clear for which sections `cs_LastBlockFile` is
responsible for. It is annotated for `m_blockfile_cursors`, but
sporadically and inconsistently also covers `m_blockfile_info`.
Since it has no semantic meaning, and seems confusing to developers,
remove it.
SteadyClockContext and FakeNodeClock assume they are the only active
instance. Overlapping them in the same scope would silently clobber
each other.
Add a CRTP base class, LimitOne, that asserts at construction if
another instance already exists.
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-
This refactor is a follow-up to commit
faad08e59c and does not
change any behavior.
These call sites are clean mechanical swaps. The remaining ones
require non-trivial test refactoring and are left for future
follow-ups.
The increment was originally added so that mocked time would not appear
to go backward relative to real-clock timestamps captured before
construction, since Now<NodeSeconds>() rounds the current time down to
a whole second. In practice the tests do not mix real and mocked
timestamps in a way that exposes this, so the increment is unnecessary.
This is a follow-up to commit faad08e59c.
Hoisting the NodeClockContext above peer creation ensures m_connected is
captured under mocktime, making the MINIMUM_CONNECT_TIME check
deterministic regardless of which peer is selected for eviction.
This is a prerequisite for the next commit, which removes the
one-second advance from the NodeClockContext default constructor.
`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.
Tests without a fixture did not have their file names linted because the
grep matches on `BOOST_FIXTURE`. Updates to match `BOOST_FIXTURE` or
`BOOST_TEST`.
Co-authored-by: l0rinc <pap.lorinc@gmail.com>
BIP434 defines FEATURE messages which are sent between VERSION and VERACK
to indicate support for new P2P protocol features. This commit provides
the infrastructure for easily using BIP434 negotiation when implementing
such new P2P protocol features. Note that advertised protocol version
is bumped to 70017, as per BIP434's specification.
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
Signed overflow on nScore updates is undefined behavior. Use
SaturatingAdd in AddLocal() and SeenLocal() so increments saturate at
INT_MAX instead of overflowing.
Add unit test coverage for saturation in both code paths.
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.
131fa570b9 test: Add test for BuildSkip() and skip heights (optout)
Pull request description:
The skip height values computed by the (internal) function `GetSkipHeight()`, and the `CBlockIndex::BuildSkip()` method are not tested directly, and the skip logic is not well documented. To improve test coverage, a new test is added, to verify `CBlockIndex::BuildSkip()` and the skip height bit-manipulation logic.
Note: the original version contained a test for the complexity of the `GetAncestor()` algorithm, whose performance is greatly determined by the skip height logic. This test was out-scoped. (see: https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-4486353485)
The motivation is to document the skip value computation through a test. (The issued was noticed while reviewing #33515.)
ACKs for top commit:
l0rinc:
ACK 131fa570b9
sipa:
ACK 131fa570b9
Tree-SHA512: 468070946419d9d2891e43ed014b040348fc9d3b35dc21522487ac28e06b6d5d556ac02ebc4fa7761e202fc80f9e9ab7a7ec47c6e05ae55e33950ff5f8f3596f
570a627640 kernel: assert invalid buffer preconditions in `btck_*_create` functions (stringintech)
Pull request description:
The kernel API appears to use `nullptr` returns to report failures that callers may reasonably want to recover from: malformed serialized input, object construction failures, chainstate/load failures, and similar runtime conditions.
The raw create-function buffer checks seem to be a different case. A failure of `ptr == nullptr && len > 0` does not indicate malformed input data or a failure encountered while deserializing or constructing the requested object. Returning `nullptr` for these checks widens the recoverable error surface with cases that are better treated as programmer errors, similar to other asserted preconditions in this API such as invalid indices and impossible enum/flag states.
This change switches those buffer argument checks from `nullptr` returns to assertions in `btck_transaction_create`, `btck_script_pubkey_create`, `btck_block_create`, `btck_block_header_create`, and `btck_chainstate_manager_options_create`. `btck_block_header_create` additionally asserts the pre-existing documented length contract (must be 80 bytes). These functions still return `nullptr` when the provided bytes cannot be parsed or when object creation fails during processing.
I ended up looking at this while working on the `kernel-bindings-tests` spec/schema for `btck_script_pubkey_create`, where treating this path as a regular error did not seem like the right contract: https://github.com/stringintech/kernel-bindings-tests/pull/14#discussion_r3240859568.
ACKs for top commit:
stickies-v:
ACK 570a627640
janb84:
ACK 570a627640
w0xlt:
ACK 570a627640
sedited:
ACK 570a627640
Tree-SHA512: 064d834abe0c27245a144e5290bbeeb510daf9e4d50bb3a8e50bd8a0bf897b3dcf6ad5acfcabf1d8110da120e5e014ee3aea0241c0f181a21c6f3c14dc452ade
Switch buffer `ptr == nullptr && len > 0` checks from `nullptr` returns to assertions. These checks represent invalid caller preconditions, not failures encountered while deserializing or constructing the requested object. `btck_block_header_create` additionally asserts the pre-existing documented length contract (must be 80 bytes).
3962138cc0 test: add IPC submitBlock functional test (woltx)
5b60f69e40 mining: add submitBlock IPC method to Mining interface (woltx)
813b4a80d7 refactor: introduce SubmitBlock helper (w0xlt)
Pull request description:
This PR adds a `submitBlock` method to the IPC Mining interface, equivalent to the `submitblock` RPC. It accepts a serialized block over IPC, validates/processes it via the normal block-processing path.
The method uses the same result shape as `checkBlock`: `bool` + `reason/debug out-params`. It reports duplicate, inconclusive, and invalid-block rejection details, and initializes reason/debug on every call.
Closes#34626
ACKs for top commit:
Sjors:
ACK 3962138cc0
optout21:
reACK 3962138cc0
ryanofsky:
Code review ACK 3962138cc0. Just rebased since and made suggested changes since last review.
Tree-SHA512: 705cbb89972a80b6ff0ab75a78f686983d6077c97f1758795efe5b8968f01065ebef664ac850eae2bc86af8964efa2a68e8dfc677209c312856650f9387ed006
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.
Add a submitBlock method to the Mining IPC interface, similar to the
submitblock RPC. This accepts a fully assembled block, validates it, and
if accepted as new, processes it into chainstate.
This is needed for Stratum v2 Job Declarator Server (JDS), where accepted
solutions may correspond to jobs not tied to a Bitcoin Core BlockTemplate.
JDS receives PushSolution fields and reconstructs full blocks; without an
IPC submitBlock method, final submission requires the submitblock RPC.
The method returns detailed status (reason/debug strings) matching the
checkBlock pattern, giving callers enough information to handle
validation failures.
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
d61053d97b build: Drop libevent from bitcoin-cli link libraries (Fabian Jahr)
798d051c80 cli: Remove libevent usage (Fabian Jahr)
376e7ef07c util: Expose IOErrorIsPermanent in sock header (Fabian Jahr)
5d562430de netbase: Add timeout parameter to ConnectDirectly (Fabian Jahr)
a988ac592f cli: Add HTTPResponseHeaders class for parsing response headers (Fabian Jahr)
c471c5085b common: Add unused UrlEncode function (Fabian Jahr)
9687ef1bd9 ci: Tolerate unused free functions in intermediate commits (Fabian Jahr)
Pull request description:
Part of the effort to remove the libevent dependency altogether, see #31194
This takes the parsing logic from the [`HTTPHeaders` class](d549f01caa) from #32061 and puts it into `bitcoin-cli` as a small `HTTPResponseHeaders` class with a comment to revisit potentially sharing this code somehow. This decoupled the two pulls which seems like the most sensible way to deal with this since the actual overlap is very small compared to the impact of each of the pulls which should ideally not block each other.
Otherwise the change itself replaces the libevent-based HTTP client with a simple synchronous implementation which uses the `Sock` class directly.
ACKs for top commit:
hodlinator:
re-ACK d61053d97b
theStack:
re-ACK d61053d97b
w0xlt:
ACK d61053d97b
Tree-SHA512: a3580a45faf540ee844aac8cb1dc056a89e8e11b45781d2807baa4736d5c0934284c6066206101b6984111a48a186d67845545d07639b623cb35ccc2d85d3ab2
