It should be sufficient to set it once. Especially, if the dynamic value
is only used by ResetAndInitialize.
This also avoids non-determistic code paths, when ResetAndInitialize may
re-initialize m_next_inv_to_inbounds.
Without this patch, the tool would report a diff:
cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/bld-cmake/ $PWD/../qa-assets/fuzz_corpora/ p2p_headers_presync 32
...
- 1126| 3| m_next_inv_to_inbounds = now + m_rng.rand_exp_duration(average_interval);
- 1127| 3| }
+ 1126| 10| m_next_inv_to_inbounds = now + m_rng.rand_exp_duration(average_interval);
+ 1127| 10| }
1128| 491| return m_next_inv_to_inbounds;
...
This allows the clock to be mockable in tests. Also, replace cs_main
with GetMutex() while touching this function.
Also, use the ElapseSteady test helper in the p2p_headers_presync fuzz
target to make it more deterministic.
The m_last_presync_update variable is a global that is not reset in
ResetAndInitialize. However, it is only used for logging, so completely
disable it for now.
Without this patch, the tool would report a diff:
cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/bld-cmake/ $PWD/../qa-assets/fuzz_corpora/ p2p_headers_presync 32
...
4468| 81| auto now = std::chrono::steady_clock::now();
4469| 81| if (now < m_last_presync_update + std::chrono::milliseconds{250}) return;
- ^80
+ ^79
...
This refactor clarifies that the MockableSteadyClock::mock_time_point
has millisecond precision by defining a type an using it.
Moreover, a ElapseSteady helper is added which can be re-used easily.
The global m_headers_presync_stats is not reset in ResetAndInitialize.
This may lead to non-determinism.
Fix it by incrementing the global node id counter instead.
Without this patch, the tool would report a diff:
cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/bld-cmake/ $PWD/../qa-assets/fuzz_corpora/ p2p_headers_presync 32
...
2587| 3.73k| if (best_it == m_headers_presync_stats.end()) {
------------------
- | Branch (2587:17): [True: 80, False: 3.65k]
+ | Branch (2587:17): [True: 73, False: 3.66k]
------------------
...
When iterating over all fuzz input files in a folder, the order should
not matter.
However, shuffling may be useful to detect non-determinism.
Thus, shuffle in fuzz.cpp, when using neither libFuzzer, nor AFL.
Also, shuffle in the deterministic-fuzz-coverage tool, when using
libFuzzer.
fa51310121 contrib: Warn about using libFuzzer for coverage check (MarcoFalke)
fa17cdb191 test: Avoid script check worker threads while fuzzing (MarcoFalke)
fa900bb2dc contrib: Only print fuzz output on failure (MarcoFalke)
fa82fe2c73 contrib: Use -Xdemangler=llvm-cxxfilt in deterministic-*-coverage (MarcoFalke)
fa7e931130 contrib: Add optional parallelism to deterministic-fuzz-coverage (MarcoFalke)
Pull request description:
This should make the `partially_downloaded_block` fuzz target even more deterministic.
Follow-up to https://github.com/bitcoin/bitcoin/pull/31841. Tracking issue: https://github.com/bitcoin/bitcoin/issues/29018.
This bundles several changes:
* First, speed up the `deterministic-fuzz-coverage` helper by introducing parallelism.
* Then, a fix to remove spawned test threads or spawn them deterministically. (While testing this, high parallelism and thread contention may be needed)
### Testing
It can be tested via (setting 32 parallel threads):
```
cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/bld-cmake/ $PWD/../b-c-qa-assets/fuzz_corpora/ partially_downloaded_block 32
```
Locally, on a failure, the output would look like:
```diff
....
- 150| 0| m_worker_threads.emplace_back([this, n]() {
- 151| 0| util::ThreadRename(strprintf("scriptch.%i", n));
+ 150| 1| m_worker_threads.emplace_back([this, n]() {
+ 151| 1| util::ThreadRename(strprintf("scriptch.%i", n));
...
```
This excerpt likely indicates that the script threads were started after the fuzz init function returned.
Similarly, for the scheduler thread, it would look like:
```diff
...
227| 0| m_node.scheduler = std::make_unique<CScheduler>();
- 228| 1| m_node.scheduler->m_service_thread = std::thread(util::TraceThread, "scheduler", [&] { m_node.scheduler->serviceQueue(); });
+ 228| 0| m_node.scheduler->m_service_thread = std::thread(util::TraceThread, "scheduler", [&] { m_node.scheduler->serviceQueue(); });
229| 0| m_node.validation_signals =
...
```
ACKs for top commit:
Prabhat1308:
re-ACK [`fa51310`](fa51310121)
hodlinator:
re-ACK fa51310121
janb84:
Re-ACK [fa51310](fa51310121)
Tree-SHA512: 1a935eb19da98c7c3810b8bcc5287e5649ffb55bf50ab78c414a424fef8e703839291bb24040a552c49274a4a0292910a00359bdff72fa29a4f53ad36d7a8720
8284229a28 refactor: deduplicate anchor witness program bytes (`0x4e,0x73`) (Sebastian Falbesoner)
41f2f058d0 test: add missing segwitv1 test cases to `script_standard_tests` (Sebastian Falbesoner)
Pull request description:
Currently we have two segwitv1 output script types that are considered standard:
- `TxoutType::WITNESS_V1_TAPROOT` (P2TR): witness program has size 32 (introduced with taproot soft-fork)
- `TxoutType::ANCHOR` (P2A): witness program is {0x4e, 0x7e} (introduced with #30352)
This PR adds them to the script standardness unit tests where missing, i.e. for using them with the `ExtractDestination` and `GetScriptForDestination` functions.
ACKs for top commit:
rkrux:
ACK 8284229a28
instagibbs:
reACK 8284229a28
hodlinator:
Code Review ACK 8284229a28
Tree-SHA512: d4a3b47fd31ba33f62d4367811e72a7f442c01b046b0a7217a66be0b9dea5c9041eebfe812c31839ec0f0b14c56948c7c016d3d2de79283583ad8e32c192c6ff
aa7a898c23 doc: use testnet4 in developer docs (Sjors Provoost)
6c217d22fd test: use testnet4 in argsman test (Sjors Provoost)
7c200ece80 test: use testnet4 in key_io_valid.json (Sjors Provoost)
d424bd5941 test: drop unused testnet3 magic bytes (Sjors Provoost)
8cfc09fafe test: cover testnet4 magic in assumeutxo.py (Sjors Provoost)
4281e3603a zmq: use testnet4 in zmq_sub.py example (Sjors Provoost)
Pull request description:
In preparation for dropping testnet3 entirely in #31974 this PR migrates a few things to testnet4:
* the ZMQ examples
* developer docs
* various unit tests
* the snapshot magic byte check in `feature_assumeutxo.py`
It drops `testnet3` from `MAGIC_BYTES` in the test framework, since no test uses it.
ACKs for top commit:
fjahr:
re-ACK aa7a898c23
maflcko:
lgtm ACK aa7a898c23🔊
hodlinator:
re-ACK aa7a898c23
Tree-SHA512: 235f74273234e8fb2aedf0017dea5c16bb9813ec7a1f89a51abe85691f09830a5ead834115d7db0936e12e55a40bc81888856a8002fe507c1474407e77f8b9fb
Threads may execute their function any time after they are spawned, so
coverage could be non-deterministic.
Fix this,
* for the script check worker threads by disabling them while fuzzing.
* for the scheduler thread by waiting for it to fully start and run the
service queue.
57d8b1f1b3 cmake: Avoid fuzzer "multiple definition of `main'" errors (Ryan Ofsky)
Pull request description:
This change builds libraries with `-fsanitize=fuzzer-no-link` instead of `-fsanitize=fuzzer` when the cmake `-DSANITIZERS=fuzzer` option is specified. This is necessary to make fuzzing and IPC cmake options compatible with each other and avoid CI failures in #30975 which enables IPC in the fuzzer CI build:
https://cirrus-ci.com/task/5366255504326656?logs=ci#L2817https://cirrus-ci.com/task/5233064575500288?logs=ci#L2384
The failures can also be reproduced by checking out #31741 and building with `cmake -B build -DBUILD_FOR_FUZZING=ON -DSANITIZERS=fuzzer -DENABLE_IPC=ON` with this fix reverted.
The fix updates the cmake build so when `-DSANITIZERS=fuzzer` is specified, the fuzz test binary is built with `-fsanitize=fuzzer` (so it can use libFuzzer's main function), and libraries are built with `-fsanitize=fuzzer-no-link` (so they can be linked into other executables with their own main functions).
Previously when `-DSANITIZERS=fuzzer` was specified, `-fsanitize=fuzzer` was applied to ALL libraries and executables. This was inappropriate because it made it impossible to build any executables other than the fuzz test executable without triggering link errors:
- `` multiple definition of `main' ``
- `` "undefined reference to `LLVMFuzzerTestOneInput' ``
if they depended on any libraries instrumented for fuzzing.
This was especially a problem when the `ENABLE_IPC` option was set because it made building the `mpgen` code generator impossible so nothing else that depended on generated sources, including the fuzz test binary, could be built either.
This commit was previously part of https://github.com/bitcoin/bitcoin/pull/31741 and had some discussion there starting in https://github.com/bitcoin/bitcoin/pull/31741#pullrequestreview-2619682385
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722).
ACKs for top commit:
hebasto:
ACK 57d8b1f1b3, tested on Ubuntu 24.04.
Tree-SHA512: 4011adbc0b08742e83cf7c0560d3d5b5694a863358e6ac9a21239626b4a8fedceca66db34b5a46136a7b26849bb1d8710c894689322ae97e1c407687c3f57d50
This abstracts out the finding of the connected component that includes
a given element from FindConnectedComponent (which just finds any connected
component).
Use this in the txgraph fuzz test, which was effectively reimplementing this
logic. At the same time, improve its performance by replacing a vector with a
set.
b1de59e896 fuzz: extract unsequenced operations with side-effects (Lőrinc)
Pull request description:
https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1817851827 introduced unsequenced operations with side-effects - which is undefined behavior, i.e. the right hand side can be evaluated before the left hand side, which happens to mutate it.
<details>
<summary>Tried to find other occurrences</summary>
```bash
clang++ --analyze -std=c++20 -I./src -I./src/test -I./src/test/fuzz src/test/fuzz/base_encode_decode.cpp src/psbt.cpp
```
but it didn't warn about UB.
Grepped for similar ones, but could find any other one in the codebase:
```bash
> grep -rnE --include='*.cpp' --include='*.h' '\b(\w+)\(([^)]*\b(\w+)\b[^)]*)\)\s*==\s*\3\.' .
./src/test/arith_uint256_tests.cpp:373: BOOST_CHECK(R1L.GetHex() == R1L.ToString());
./src/test/arith_uint256_tests.cpp:374: BOOST_CHECK(R2L.GetHex() == R2L.ToString());
./src/test/arith_uint256_tests.cpp:375: BOOST_CHECK(OneL.GetHex() == OneL.ToString());
./src/test/arith_uint256_tests.cpp:376: BOOST_CHECK(MaxL.GetHex() == MaxL.ToString());
./src/test/fuzz/cluster_linearize.cpp:565: assert(depgraph.FeeRate(best_anc.transactions) == best_anc.feerate);
./src/test/fuzz/cluster_linearize.cpp:646: assert(depgraph.FeeRate(found.transactions) == found.feerate);
./src/test/fuzz/cluster_linearize.cpp:765: assert(depgraph.FeeRate(chunk_info.transactions) == chunk_info.feerate);
./src/test/fuzz/base_encode_decode.cpp:95: assert(DecodeBase64PSBT(psbt, random_string, error) == error.empty());
./src/test/fuzz/key.cpp:102: assert(pubkey.data() == pubkey.begin());
./src/test/skiplist_tests.cpp:42: BOOST_CHECK(vIndex[from].GetAncestor(0) == vIndex.data());
./src/script/signingprovider.cpp:535: ComputeTapbranchHash(node.sub[1]->hash, node.sub[1]->hash) == node.hash) {
./src/pubkey.h:78: return vch.size() > 0 && GetLen(vch[0]) == vch.size();
./src/cluster_linearize.h:881: Assume(elem.inc.feerate.IsEmpty() == elem.pot_feerate.IsEmpty());
```
</details>
Hodlinator deduced the UB on Windows in https://github.com/bitcoin/bitcoin/issues/32135#issuecomment-2751723855Fixes#32135
ACKs for top commit:
maflcko:
lgtm ACK b1de59e896
hodlinator:
ACK b1de59e896
marcofleon:
Nice, ACK b1de59e896
brunoerg:
code review ACK b1de59e896
Tree-SHA512: d66524424c7f749eba870f5bd6038da79666ac638047b31dd8ff15a77d927facb54b4735e8afb7984648fdc9e2dd59ea213996c352301fa05978f041511361d4
b2ea365648 txgraph: Add Get{Ancestors,Descendants}Union functions (feature) (Pieter Wuille)
54bceddd3a txgraph: Multiple inputs to Get{Ancestors,Descendant}Refs (preparation) (Pieter Wuille)
aded047019 txgraph: Add CountDistinctClusters function (feature) (Pieter Wuille)
b685d322c9 txgraph: Add DoWork function (feature) (Pieter Wuille)
295a1ca8bb txgraph: Expose ability to compare transactions (feature) (Pieter Wuille)
22c68cd153 txgraph: Allow Refs to outlive the TxGraph (feature) (Pieter Wuille)
82fa3573e1 txgraph: Destroying Ref means removing transaction (feature) (Pieter Wuille)
6b037ceddf txgraph: Cache oversizedness of graphs (optimization) (Pieter Wuille)
8c70688965 txgraph: Add staging support (feature) (Pieter Wuille)
c99c7300b4 txgraph: Abstract out ClearLocator (refactor) (Pieter Wuille)
34aa3da5ad txgraph: Group per-graph data in ClusterSet (refactor) (Pieter Wuille)
36dd5edca5 txgraph: Special-case removal of tail of cluster (Optimization) (Pieter Wuille)
5801e0fb2b txgraph: Delay chunking while sub-acceptable (optimization) (Pieter Wuille)
57f5499882 txgraph: Avoid looking up the same child cluster repeatedly (optimization) (Pieter Wuille)
1171953ac6 txgraph: Avoid representative lookup for each dependency (optimization) (Pieter Wuille)
64f69ec8c3 txgraph: Make max cluster count configurable and "oversize" state (feature) (Pieter Wuille)
1d27b74c8e txgraph: Add GetChunkFeerate function (feature) (Pieter Wuille)
c80aecc24d txgraph: Avoid per-group vectors for clusters & dependencies (optimization) (Pieter Wuille)
ee57e93099 txgraph: Add internal sanity check function (tests) (Pieter Wuille)
05abf336f9 txgraph: Add simulation fuzz test (tests) (Pieter Wuille)
8ad3ed2681 txgraph: Add initial version (feature) (Pieter Wuille)
6eab3b2d73 feefrac: Introduce tagged wrappers to distinguish vsize/WU rates (Pieter Wuille)
d449773899 scripted-diff: (refactor) ClusterIndex -> DepGraphIndex (Pieter Wuille)
bfeb69f6e0 clusterlin: Make IsAcyclic() a DepGraph member function (Pieter Wuille)
0aa874a357 clusterlin: Add FixLinearization function + fuzz test (Pieter Wuille)
Pull request description:
Part of cluster mempool: #30289.
### 1. Overview
This introduces the `TxGraph` class, which encapsulates knowledge about the (effective) fees, sizes, and dependencies between all mempool transactions, but nothing else. In particular, it lacks knowledge about `CTransaction`, inputs, outputs, txids, wtxids, prioritization, validatity, policy rules, and a lot more. Being restricted to just those aspects of the mempool makes the behavior very easy to fully specify (ignoring the actual linearizations produced), and write simulation-based tests for (which are included in this PR).
### 2. Interface
The interface can be largely categorized into:
* Mutation functions:
* `AddTransaction` (add a new transaction with specified feerate, and get a `Ref` object back to identify it).
* `RemoveTransaction` (given a `Ref` object, remove the transaction).
* `AddDependency` (given two `Ref` objects, add a dependency between them).
* `SetTransactionFee` (modify the fee associated with a Ref object).
* Inspector functions:
* `GetAncestors` (get the ancestor set in the form of `Ref*` pointers)
* `GetAncestorsUnion` (like above, but for the union of ancestors of multiple `Ref*` pointers)
* `GetDescendants` (get the descendant set in the form of `Ref*` pointers)
* `GetDescendantsUnion` (like above, but for the union of ancestors of multiple `Ref*` pointers)
* `GetCluster` (get the connected component set in the form of `Ref*` pointers, in the order they would be mined).
* `GetIndividualFeerate` (get the feerate of a transaction)
* `GetChunkFeerate` (get the mining score of a transaction)
* `CountDistinctClusters` (count the number of distinct clusters a list of `Ref`s belong to)
* Staging functions:
* `StartStaging` (make all future mutations operate on a proposed transaction graph)
* `CommitStaging` (apply all the changes that are staged)
* `AbortStaging` (discard all the changes that are staged)
* Miscellaneous functions:
* `DoWork` (do queued-up computations now, so that future operations are fast)
This `TxGraph::Ref` type used as a "handle" on transactions in the graph can be inherited from, and the idea is that in the full cluster mempool implementation (#28676, after it is rebased on this), `CTxMempoolEntry` will inherit from it, and all actually used Ref objects will be `CTxMempoolEntry`s. With that, the mempool code can just cast any `Ref*` returned by txgraph to `CTxMempoolEntry*`.
### 3. Implementation
Internally the graph data is kept in clustered form (partitioned into connected components), for which linearizations are maintained and updated as needed using the `cluster_linearize.h` algorithms under the hood, but this is hidden from the users of this class. Implementation-wise, mutations are generally applied lazily, appending to queues of to-be-removed transactions and to-be-added dependencies, so they can be batched for higher performance. Inspectors will generally only evaluate as much as is needed to answer queries, with roughly 5 levels of processing to go to fully instantiated and acceptable cluster linearizations, in order:
1. `ApplyRemovals` (take batches of to-be-removed transactions and translate them to "holes" in the corresponding Clusters/DepGraphs).
2. `SplitAll` (creating holes in Clusters may cause them to break apart into smaller connected components, so make turn them into separate Clusters/linearizations).
3. `GroupClusters` (figure out which Clusters will need to be combined in order to add requested to-be-added dependencies, as these may span clusters).
4. `ApplyDependencies` (actually merge Clusters as precomputed by `GroupClusters`, and add the dependencies between them).
5. `MakeAcceptable` (perform the LIMO linearization algorithm on Clusters to make sure their linearizations are acceptable).
### 4. Future work
This is only an initial version of TxGraph, and some functionality is missing before #28676 can be rebased on top of it:
* The ability to get comparative feerate diagrams before/after for the set of staged changes (to evaluate RBF incentive-compatibility).
* Mining interface (ability to iterate transactions quickly in mining score order) (see #31444).
* Eviction interface (reverse of mining order, plus memory usage accounting) (see #31444).
* Ability to fix oversizedness of clusters (before or after committing) - this is needed for reorgs where aborting/rejecting the change just is not an option (see #31553).
* Interface for controlling how much effort is spent on LIMO. In this PR it is hardcoded.
Then there are further improvements possible which would not block other work:
* Making Cluster a virtual class with different implementations based on transaction count (which could dramatically reduce memory usage, as most Clusters are just a single transaction, for which the current implementation is overkill).
* The ability to have background thread(s) for improving cluster linearizations.
ACKs for top commit:
instagibbs:
reACK b2ea365648
ajtowns:
reACK b2ea365648
ismaelsadeeq:
reACK b2ea365648🚀
glozow:
ACK b2ea365648
Tree-SHA512: 0f86f73d37651fe47d469db1384503bbd1237b4556e5d50b1d0a3dd27754792d6fc3481f77a201cf2ed36c6ca76e0e44c30e175d112aacb53dfdb9e11d8abc6b
https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1817851827 introduced an unsequenced operations with side-effects - which is undefined behavior, i.e. the right hand side can be evaluated before the left hand side, which happens to mutate it.
Tried:
```
clang++ --analyze -std=c++20 -I./src -I./src/test -I./src/test/fuzz src/test/fuzz/base_encode_decode.cpp src/psbt.cpp
```
but it didn't warn about UB.
Grepped for similar ones, but could find any other one in the codebase:
> grep -rnE --include='*.cpp' --include='*.h' '\b(\w+)\(([^)]*\b(\w+)\b[^)]*)\)\s*==\s*\3\.' .
```
./src/test/arith_uint256_tests.cpp:373: BOOST_CHECK(R1L.GetHex() == R1L.ToString());
./src/test/arith_uint256_tests.cpp:374: BOOST_CHECK(R2L.GetHex() == R2L.ToString());
./src/test/arith_uint256_tests.cpp:375: BOOST_CHECK(OneL.GetHex() == OneL.ToString());
./src/test/arith_uint256_tests.cpp:376: BOOST_CHECK(MaxL.GetHex() == MaxL.ToString());
./src/test/fuzz/cluster_linearize.cpp:565: assert(depgraph.FeeRate(best_anc.transactions) == best_anc.feerate);
./src/test/fuzz/cluster_linearize.cpp:646: assert(depgraph.FeeRate(found.transactions) == found.feerate);
./src/test/fuzz/cluster_linearize.cpp:765: assert(depgraph.FeeRate(chunk_info.transactions) == chunk_info.feerate);
./src/test/fuzz/base_encode_decode.cpp:95: assert(DecodeBase64PSBT(psbt, random_string, error) == error.empty());
./src/test/fuzz/key.cpp:102: assert(pubkey.data() == pubkey.begin());
./src/test/skiplist_tests.cpp:42: BOOST_CHECK(vIndex[from].GetAncestor(0) == vIndex.data());
./src/script/signingprovider.cpp:535: ComputeTapbranchHash(node.sub[1]->hash, node.sub[1]->hash) == node.hash) {
./src/pubkey.h:78: return vch.size() > 0 && GetLen(vch[0]) == vch.size();
./src/cluster_linearize.h:881: Assume(elem.inc.feerate.IsEmpty() == elem.pot_feerate.IsEmpty());
```
Hodlinator deduced the UB on Windows in https://github.com/bitcoin/bitcoin/issues/32135#issuecomment-2751723855
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
In order to make it possible for higher layers to compare transaction quality
(ordering within the implicit total ordering on the mempool), expose a comparison
function and test it.
Before this commit, if a TxGraph::Ref object is destroyed, it becomes impossible
to refer to, but the actual corresponding transaction node in the TxGraph remains,
and remains indefinitely as there is no way to remove it.
Fix this by making the destruction of TxGraph::Ref trigger immediate removal of
the corresponding transaction in TxGraph, both in main and staging if it exists.
In order to make it easy to evaluate proposed changes to a TxGraph, introduce a
"staging" mode, where mutators (AddTransaction, AddDependency, RemoveTransaction)
do not modify the actual graph, but just a staging version of it. That staging
graph can then be commited (replacing the main one with it), or aborted (discarding
the staging).
Instead of leaving the responsibility on higher layers to guarantee that
no connected component within TxGraph (a barely exposed concept, except through
GetCluster()) exceeds the cluster count limit, move this responsibility to
TxGraph itself:
* TxGraph retains a cluster count limit, but it becomes configurable at construction
time (this primarily helps with testing that it is properly enforced).
* It is always allowed to perform mutators on TxGraph, even if they would cause the
cluster count limit to be exceeded. Instead, TxGraph exposes an IsOversized()
function, which queries whether it is in a special "oversize" state.
* During oversize state, many inspectors are unavailable, but mutators remain valid,
so the higher layer can "fix" the oversize state before continuing.
To make testing more powerful, expose a function to perform an internal sanity
check on the state of a TxGraph. This is especially important as TxGraphImpl
contains many redundantly represented pieces of information:
* graph contains clusters, which refer to entries, but the entries refer back
* graph maintains pointers to Ref objects, which point back to the graph.
This lets us make sure they are always in sync.
This adds a simulation fuzz test for txgraph, by comparing with a naive
reimplementation that models the entire graph as a single DepGraph, and
clusters in TxGraph as connected components within that DepGraph.
Since cluster_linearize.h does not actually have a Cluster type anymore, it is more
appropriate to rename the index type to DepGraphIndex.
-BEGIN VERIFY SCRIPT-
sed -i 's/Data type to represent transaction indices in clusters./Data type to represent transaction indices in DepGraphs and the clusters they represent./' $(git grep -l 'using ClusterIndex')
sed -i 's|\<ClusterIndex\>|DepGraphIndex|g' $(git grep -l 'ClusterIndex')
-END VERIFY SCRIPT-
This function takes an existing ordering for transactions in a DepGraph, and
makes it a valid linearization for it (i.e., topological). Any topological
prefix of the input remains untouched.
63b534f97e fuzz: sanity check hardcoded snapshot in utxo_snapshot target (Antoine Poinsot)
3b85eba83a test util: split up ConnectBlock from MineBlock (Antoine Poinsot)
d1527f6b88 qa: correct off-by-one in utxo snapshot fuzz target (Antoine Poinsot)
Pull request description:
The assumeutxo data for the fuzz target could change and invalidate the hash silently, preventing the fuzz target from reaching some code paths. Fix this by introducing a unit test which would break if the snapshot data the fuzz target relies on were to change.
In implementing this i noticed the height used for coins in the fuzz target is actually off-by-one (as if the first block in the created chain was the genesis but it's block `1`), so fix that too.
ACKs for top commit:
mzumsande:
Code Review ACK 63b534f97e
fjahr:
tACK 63b534f97e
Tree-SHA512: 2399b6e74db9b78aab8efba67c57a405d2d7d880ae3b7d8518a1c96cc6266f61f5e77722cd999adeac5d3e03e73d84cf9ae7bdbcc0afae198cc87049dde4012f
fa4fb6a8f1 fuzz: Use serial task runner to increase fuzz stability (MarcoFalke)
Pull request description:
Leaking a scheduler with a non-empty queue from the fuzz initialization phase into the fuzz target execution phase is problematic, because it messes with coverage data. This in turn is problematic, because it leads to:
* Decrease in fuzz target execution stability (non-determinism when running the fuzz target).
* Decrease in fuzz input merge stability (non-determinism when selecting a minimum set of fuzz input to reach maximum coverage), which leads to qa-assets bloat.
Fix one such issue. Tracking issue: https://github.com/bitcoin/bitcoin/issues/29018
Can be tested via: `RUST_BACKTRACE=1 cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/bld-cmake $PWD/../b-c-qa-assets/fuzz_corpora/ partially_downloaded_block`.
The failure is non-deterministic (obviously) and will show coverage in validation signals such as `UpdatedBlockTip` before this change and will have this one fixed after this change.
ACKs for top commit:
marcofleon:
ACK fa4fb6a8f1
dergoegge:
Code review ACK fa4fb6a8f1
Tree-SHA512: fd1f66562c1d3c21553c7dd324399cdc16faa2fedfdb8e7544ea6a68b8b356e7c81d81815ecf70e0d334307dab6b275c1889b3b889b6f15eec514beee22c95f4
ffff4a293a bench: Update span-serialize comment (MarcoFalke)
fa4d6ec97b refactor: Avoid false-positive gcc warning (MarcoFalke)
fa942332b4 scripted-diff: Bump copyright headers after std::span changes (MarcoFalke)
fa0c6b7179 refactor: Remove unused Span alias (MarcoFalke)
fade0b5e5e scripted-diff: Use std::span over Span (MarcoFalke)
fadccc26c0 refactor: Make Span an alias of std::span (MarcoFalke)
fa27e36717 test: Fix broken span_tests (MarcoFalke)
fadf02ef8b refactor: Return std::span from MakeUCharSpan (MarcoFalke)
fa720b94be refactor: Return std::span from MakeByteSpan (MarcoFalke)
Pull request description:
`Span` has some issues:
* It does not support fixed-size spans, which are available through `std::span`.
* It is confusing to have it available and in use at the same time with `std::span`.
* It does not obey the standard library iterator build hardening flags. See https://github.com/bitcoin/bitcoin/issues/31272 for a discussion. For example, this allows to catch issues like the one fixed in commit fabeca3458.
Both types are type-safe and can even implicitly convert into each other in most contexts.
However, exclusively using `std::span` seems less confusing, so do it here with a scripted-diff.
ACKs for top commit:
l0rinc:
reACK ffff4a293a
theuni:
ACK ffff4a293a.
Tree-SHA512: 9cc2f1f43551e2c07cc09f38b1f27d11e57e9e9bc0c6138c8fddd0cef54b91acd8b14711205ff949be874294a121910d0aceffe0e8914c4cff07f1e0e87ad5b8
fac3d93c2b fuzz: Speed up *_package_eval fuzz targets a bit (MarcoFalke)
fa40fd043a fuzz: [refactor] Avoid confusing c-style cast (MarcoFalke)
Pull request description:
Each target is at least 10% faster for me when running over the current set of qa-assets, which seems nice.
The changes `outpoints_value` from a map to an unordered map, which is safe, because the element order is not used in the fuzz test and the map is only used for lookup.
(`mempool_outpoints` can't be changed, because the order matters here. Using unordered_set here may result in a non-deterministic fuzz target, given the same fuzz input.)
ACKs for top commit:
l0rinc:
ACK fac3d93c2b
dergoegge:
Code review ACK fac3d93c2b
Tree-SHA512: 8ae5d4e281505aff76a4003d6e9ea388dbb73860e167385bd6a0a201b3acc939db29ee212594952a9e80e85b3cc4cd726ce6dd49551f74013cb4da8a15cbdfb3
21e9d39a37 docs: add release notes for 31603 (brunoerg)
a8b548d75d test: `getdescriptorinfo`/`importdescriptors` with whitespace in pubkeys (brunoerg)
c7afca3d62 test: descriptor: check whitespace into keys (brunoerg)
cb722a3cea descriptor: check whitespace in ParsePubkeyInner (brunoerg)
50856695ef test: fix descriptors in `ismine_tests` (brunoerg)
Pull request description:
Currently, we successfully parse descriptors which contains spaces in the beginning or end of the public/private key within a fragment (e.g. `pk( KEY)`, `pk(KEY )` or `pk( KEY )`). I have noticed that one of the reasons is that the `DecodeBase58` function simply ignore these whitespaces.
This PR changes the `ParsePubkeyInner ` to reject pubkeys that contain a whitespace at the beginning and/or at the end. We will only check the whitespace in some RPCs (e.g. `importdescriptors`), but an already imported descriptor won't be affected by this check, especially because we store descriptors from `ToString`.
For context: https://github.com/brunoerg/bitcoinfuzz/issues/72
ACKs for top commit:
rkrux:
tACK 21e9d39a37
darosior:
re-ACK 21e9d39a37
sipa:
utACK 21e9d39a37
Tree-SHA512: 54f48a89a235517e5cdc29a46dceeb7dabbee93c7616a166288ff3f90131808eb0ece43b0797a11fe827a5f7bd51d65e3e75c16789b0a42020934cabb684cc8f
54e6eacc1f test: Enable ResetCoverageCounters beyond Linux (janb84)
Pull request description:
In PR [#31901](https://github.com/bitcoin/bitcoin/pull/31901), Coverage.cpp was introduced as a separate utility file, based on existing code. However, the macro defined in Coverage.cpp was limited to Clang and Linux, which caused issues for users on macOS when using the newly introduced deterministic test tooling.
This change adds fallback functions which are used when building without code coverage on non linux env.
This adds support for macOS to ResetCoverageCounters. ResetCoverageCounters is used by the unit tests in `g_rng_temp_path_init` to support the deterministic unit test tooling. It is also used in fuzz tests to completely suppress coverage from anything init-related.
See [Readme](https://github.com/bitcoin/bitcoin/blob/master/contrib/devtools/README.md) on how to test this for deterministic unit & fuzz test.
Suggestion for test files:
- for unit test: `util_string_tests`
- for fuzz test: `addition_overflow `
These files should give deterministic results
ACKs for top commit:
maflcko:
review-only ACK 54e6eacc1f
hodlinator:
re-ACK 54e6eacc1f
Tree-SHA512: dd71da6f76d4fc9e64bf521bbfe5e7483d77c2ca0380f9e692502e64b529068ea33f21b19399481feb7c6780a23d893d8b7f733cef641a2db18a13397c98deea
4cd95a2921 refactor: modernize remaining outdated trait patterns (Lőrinc)
ab2b67fce2 scripted-diff: modernize outdated trait patterns - values (Lőrinc)
8327889f35 scripted-diff: modernize outdated trait patterns - types (Lőrinc)
Pull request description:
The use of [`std::underlying_type_t<T>`](https://en.cppreference.com/w/cpp/types/underlying_type) or [`std::is_enum_v<T>`](https://en.cppreference.com/w/cpp/types/is_enum) (and similar ones, introduced in C++14) replace the `typename std::underlying_type<T>::type` and `std::is_enum<T>::value` constructs (available in C++11).
The `_t` and `_v` helper alias templates offer a more concise way to extract the type and value directly.
I've modified the instances I found in the codebase one-by-one (noticed them while investigating https://github.com/bitcoin/bitcoin/pull/31868), and afterwards extracted scripted diff commits to do the trivial ones automatically.
The last commit contains the values that were easier done manually.
I've excluded changes from `src/bench/nanobench.h`, `src/leveldb`, `src/minisketch`, `src/span.h` and `src/sync.h` - let me know if you think they should be included instead.
A few of the code changes can also be reproduced by clang-tidy (but not all of them):
```bash
cmake -B build -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DBUILD_FOR_FUZZING=ON && cmake --build build -j$(nproc)
run-clang-tidy -quiet -p build -j $(nproc) -checks='-*,modernize-type-traits' -fix $(git grep -lE '::(value|type)' ./src ':(exclude)src/bench/nanobench.h' ':(exclude)src/leveldb' ':(exclude)src/minisketch' ':(exclude)src/span.h' ':(exclude)src/sync.h')
```
ACKs for top commit:
laanwj:
Concept and code review ACK 4cd95a2921
Tree-SHA512: a4bcf0f267c0f4e02983b4d548ed6f58d464ec379ac5cd1f998b9ec0cf698b53a9f2557a05a342b661f1d94adefc9a0ce2dc8f764d49453aaea95451e2c4c581
Non-Linux linkers require a fallback implementation for when coverage is not enabled.
The fallbacks are marked weak to have lower precedence than built-in implementations when available, removing ambiguity from the linker.
d5537c18a9 fuzz: make sure DecodeBase58(Check) is called with valid values more often (Lőrinc)
bad1433ef2 fuzz: Always restrict base conversion input lengths (Lőrinc)
Pull request description:
This is a follow-up to https://github.com/bitcoin/bitcoin/pull/30746, expanding coverage by:
* restricting every input for the base58 conversions, capping max sizes to `100` instead of `1000` or all available input (suggested by marcofleon in https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1963718683) since most actual usage has lengths of e.g. `21`, `34`, `78`.
* providing more valid values to the decoder (suggested by maflcko in https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1957847712) by randomly providing a random input or a valid encoded one; this also enables unifying the roundtrip tests to a single roundtrip per fuzz.
ACKs for top commit:
mzumsande:
Code Review / lightly tested ACK d5537c18a9
maflcko:
review ACK d5537c18a9🚛
Tree-SHA512: 50365654cdac8a38708a7475eaa43396642b7337e2ee8999374c3faafff4f05457abc1a54c701211e0ed24d36c12af77bcad17b49695699be42664f2be660659
