9f243cd7fa Introduce `g_fuzzing` global for fuzzing checks (dergoegge)
Pull request description:
This PR introduces a global `g_fuzzing` that indicates if we are fuzzing.
If `g_fuzzing` is `true` then:
* Assume checks are enabled
* Special fuzzing paths are taken (e.g. pow check is reduced to one bit)
Closes #30950#31057
ACKs for top commit:
maflcko:
review ACK 9f243cd7fa 🗜
brunoerg:
crACK 9f243cd7fa
marcofleon:
Tested ACK 9f243cd7fa
Tree-SHA512: 56e4cad0555dec0c565ea5ecc529628ee4f37d20dc660c647fdc6948fbeed8291e6fe290de514bd4c2c7089654d9ce1add607dc9855462828b62be9ee45e4999
552cae243a fuzz: cover `ASMapHealthCheck` in connman target (brunoerg)
33b0f3ae96 fuzz: use `ConsumeNetGroupManager` in connman target (brunoerg)
18c8a0945b fuzz: move `ConsumeNetGroupManager` to util (brunoerg)
fe624631ae fuzz: fuzz `connman` with a non-empty addrman (brunoerg)
0a12cff2a8 fuzz: move `AddrManDeterministic` to util (brunoerg)
Pull request description:
### Motivation
Currently, we fuzz connman with an addrman from `NodeContext`. However,
fuzzing connman with only empty addrman might not be effective, especially
for functions like `GetAddresses` and other ones that plays with addrman. Also,
we do not fuzz connman with ASMap, what would be good for functions that need
`GetGroup`, or even for addrman. Without it, I do not see how effective would be
fuzzing `ASMapHealthCheck`, for example.
### Changes
- Move `AddrManDeterministic` and `ConsumeNetGroupManager` to util.
- Use `ConsumeNetGroupManager` in connman target to construct a netgroupmanager
and use it for `ConnmanTestMsg`.
- Use `AddrManDeterministic` in connman target to create an addrman. It does
not slow down as "filling" the addrman (e.g. with `FillAddrman`).
- Add coverage for `ASMapHealthCheck`.
ACKs for top commit:
maflcko:
review ACK 552cae243a🏀
dergoegge:
Code review ACK 552cae243a
marcofleon:
Code review ACK 552cae243a. Changes match the PR description.
Tree-SHA512: ba861c839602054077e4bf3649763eeb48357cda83ca3ddd32b02a1b61f4e44a0c5070182f001f9bf531d0d64717876279a7de3ddb9de028b343533b89233851
c495731a31 fuzz: wallet: add target for `CreateTransaction` (brunoerg)
3db68e29ec wallet: move `ImportDescriptors`/`FuzzedWallet` to util (brunoerg)
Pull request description:
This PR adds a fuzz target for the `CreateTransaction` function. It is a regression target for https://github.com/bitcoin/bitcoin/pull/27271 and can be testing by applying:
```diff
@@ -1110,7 +1110,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
// This can only happen if feerate is 0, and requested destinations are value of 0 (e.g. OP_RETURN)
// and no pre-selected inputs. This will result in 0-input transaction, which is consensus-invalid anyways
if (selection_target == 0 && !coin_control.HasSelected()) {
- return util::Error{_("Transaction requires one destination of non-0 value, a non-0 feerate, or a pre-selected input")};
+ // return util::Error{_("Transaction requires one destination of non-0 value, a non-0 feerate, or a pre-selected input")};
}
```
Also, it moves `ImportDescriptors` function to `src/wallet/test/util.h` to avoid to duplicate same code.
ACKs for top commit:
marcofleon:
ACK c495731a31
maflcko:
ACK c495731a31 🏻
Tree-SHA512: a439f947b91b01e327e18cd18e63d5ce49f2cb9ca16ca9d56fe337b8cff239b3af4db18fe89478fe5faa5549d37ca935bd321913db7646fbf6818f825cb5d878
4feaa28728 refactor: Rely on returned value of GetCoin instead of parameter (Lőrinc)
46dfbf169b refactor: Return optional of Coin in GetCoin (Lőrinc)
e31bfb26c2 refactor: Remove unrealistic simulation state (Lőrinc)
Pull request description:
While reviewing [the removal of the unreachable combinations from the Coin cache logic](https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1721727681), we've noticed that the related tests often [reflect impossible states](https://github.com/bitcoin/bitcoin/pull/30673/files#r1740154464).
Browsing the Coin cache refactoring history revealed that migrating `bool GetCoin` to `optional<Coin> GetCoin` was [already proposed a few times before](https://github.com/bitcoin/bitcoin/pull/18746#issuecomment-842393167).
This refactor makes certain invalid states impossible, reducing the possibility of errors and making the code easier to understand. This will let us remove test code that exercises the impossible states as well.
The PR is done in multiple small steps, first swapping the new `optional` return value, slowly strangling out the usages of the return parameter, followed by the removal of the parameter.
Most of the invalid test states were still kept, except for https://github.com/bitcoin/bitcoin/pull/30673/files#r1748087322, where the new design prohibits invalid usage and https://github.com/bitcoin/bitcoin/pull/30673/files#r1749350258 was just marked with a TODO, will be removed in a follow-up PR.
ACKs for top commit:
andrewtoth:
re-ACK 4feaa28728
achow101:
ACK 4feaa28728
laanwj:
Code review ACK 4feaa28728
theStack:
Code-review ACK 4feaa28728
Tree-SHA512: 818d60b2e97f58c489a61120fe761fb67a08dffbefe7a3fce712d362fc9eb8c2cced23074f1bec55fe71c616a3561b5a8737919ad6ffb2635467ec4711683df7
This combines the clusterlin_add_dependency and clusterlin_cluster_serialization
fuzz tests into a single clusterlin_depgraph_sim fuzz test. This tests starts
from an empty DepGraph and performs a arbitrary number of AddTransaction,
AddDependencies, and RemoveTransactions operations on it, and compares the
resulting state with a naive reimplementation.
This commits introduces support in DepGraph for the transaction positions to be
non-continuous. Specifically, it adds:
* DepGraph::RemoveTransactions which removes 0 or more positions from a DepGraph.
* DepGraph::Positions() to get a set of which positions are in use.
* DepGraph::PositionRange() to get the highest used position in a DepGraph + 1.
In addition, it extends the DepGraphFormatter format to support holes in a
compatible way (it serializes non-holey DepGraphs identically to the old code,
and deserializes them the same way)
This changes DepGraph::AddDependency into DepGraph::AddDependencies, which takes
in a single child, but a set of parent transactions, making them all dependencies
at once.
This is important for performance. N transactions can have O(N^2) parents combined,
so constructing a full DepGraph using just AddDependency (which is O(N) on its own)
could take O(N^3) time, while doing the same with AddDependencies (also O(N) on its
own) only takes O(N^2).
Notably, this matters for DepGraphFormatter::Unser, which goes from O(N^3) to O(N^2).
Co-Authored-By: Greg Sanders <gsanders87@gmail.com>
A fuzz test already relies on these operations, and a future commit will need
the same logic too. Therefore, abstract them out into proper member functions,
with proper testing.
98c1536852 test: add getorphantxs tests (tdb3)
93f48fceb7 test: add tx_in_orphanage() (tdb3)
34a9c10e8c rpc: add getorphantxs (tdb3)
f511ff3654 refactor: move verbosity parsing to rpc/util (tdb3)
532491faf1 net: add GetOrphanTransactions() to PeerManager (tdb3)
91b65adff2 refactor: add OrphanTxBase for external use (tdb3)
Pull request description:
This PR adds a new hidden rpc, `getorphantxs`, that provides the caller with a list of orphan transactions. This rpc may be helpful when checking orphan behavior/scenarios (e.g. in tests like `p2p_orphan_handling`) or providing additional data for statistics/visualization.
```
getorphantxs ( verbosity )
Shows transactions in the tx orphanage.
EXPERIMENTAL warning: this call may be changed in future releases.
Arguments:
1. verbosity (numeric, optional, default=0) 0 for an array of txids (may contain duplicates), 1 for an array of objects with tx details, and 2 for details from (1) and tx hex
Result (for verbose = 0):
[ (json array)
"hex", (string) The transaction hash in hex
...
]
Result (for verbose = 1):
[ (json array)
{ (json object)
"txid" : "hex", (string) The transaction hash in hex
"wtxid" : "hex", (string) The transaction witness hash in hex
"bytes" : n, (numeric) The serialized transaction size in bytes
"vsize" : n, (numeric) The virtual transaction size as defined in BIP 141. This is different from actual serialized size for witness transactions as witness data is discounted.
"weight" : n, (numeric) The transaction weight as defined in BIP 141.
"expiration" : xxx, (numeric) The orphan expiration time expressed in UNIX epoch time
"from" : [ (json array)
n, (numeric) Peer ID
...
]
},
...
]
Result (for verbose = 2):
[ (json array)
{ (json object)
"txid" : "hex", (string) The transaction hash in hex
"wtxid" : "hex", (string) The transaction witness hash in hex
"bytes" : n, (numeric) The serialized transaction size in bytes
"vsize" : n, (numeric) The virtual transaction size as defined in BIP 141. This is different from actual serialized size for witness transactions as witness data is discounted.
"weight" : n, (numeric) The transaction weight as defined in BIP 141.
"expiration" : xxx, (numeric) The orphan expiration time expressed in UNIX epoch time
"from" : [ (json array)
n, (numeric) Peer ID
...
],
"hex" : "hex" (string) The serialized, hex-encoded transaction data
},
...
]
Examples:
> bitcoin-cli getorphantxs 2
> curl --user myusername --data-binary '{"jsonrpc": "2.0", "id": "curltest", "method": "getorphantxs", "params": [2]}' -H 'content-type: application/json' http://127.0.0.1:8332/
```
```
$ build/src/bitcoin-cli getorphantxs 2
[
{
"txid": "50128aac5deab548228d74d846675ad4def91cd92453d81a2daa778df12a63f2",
"wtxid": "bb61659336f59fcf23acb47c05dc4bbea63ab533a98c412f3a12cb813308d52c",
"bytes": 133,
"vsize": 104,
"weight": 415,
"expiration": 1725663854,
"from": [
1
],
"hex": "020000000001010b992959eaa2018bbf31a4a3f9aa30896a8144dbd5cfaf263bf07c0845a3a6620000000000000000000140fe042a010000002251202913b252fe537830f843bfdc5fa7d20ba48639a87c86ff837b92d083c55ad7c102015121c0000000000000000000000000000000000000000000000000000000000000000100000000"
},
{
"txid": "330bb7f701604a40ade20aa129e9a3eb8a7bf024e599084ca1026d3222b9f8a1",
"wtxid": "b7651f7d4c1a40c4d01f6a1e43a121967091fa0f56bb460146c1c5c068e824f6",
"bytes": 133,
"vsize": 104,
"weight": 415,
"expiration": 1725663854,
"from": [
2
],
"hex": "020000000001013600adfe41e0ebd2454838963d270916d2b47239c9eebb93a992b720d3589a080000000000000000000140fe042a010000002251202913b252fe537830f843bfdc5fa7d20ba48639a87c86ff837b92d083c55ad7c102015121c0000000000000000000000000000000000000000000000000000000000000000100000000"
}
]
```
ACKs for top commit:
glozow:
reACK 98c1536852
hodlinator:
re-ACK 98c1536852
danielabrozzoni:
ACK 98c1536852
pablomartin4btc:
tACK 98c1536852
itornaza:
reACK 98c1536852
Tree-SHA512: 66075f9faa83748350b87397302100d08af92cbef5fadb27f2b4903f028c08020bf34a23e17262b41abb3f379ca9f46cf6cd5459b8681f2b83bffbbaf3c03ff9
a7498cc7e2 Fix bug in p2p_headers_presync harness (marcofleon)
Pull request description:
The calculation for the test chain's work (`total_work`) should be outside of the loop. Previously, `total_work` was being miscalculated due to multiple additions of work from the same headers. Now, each header's work is only counted once, providing an accurate total.
https://github.com/bitcoin/bitcoin/pull/30918 followup
ACKs for top commit:
dergoegge:
utACK a7498cc7e2
instagibbs:
ACK a7498cc7e2
glozow:
makes sense, utACK a7498cc7e2
mzumsande:
ACK a7498cc7e2
Tree-SHA512: b95f25dcf7ace220e30f1d72f50d85ee18777467927c0cc1ed8582b390cb7185ffc0e2f127309eb083044fb41f5a13fce5ebb15b7952718a899bafff26921be8
f482d0e366 fuzz: reduce number of iterations in `crypto_aeadchacha20poly1305` target (brunoerg)
Pull request description:
By reducing the number of iterations we improve the performance of this target and may increase coverage.
Running with `-runs=100000` from qa-assets I noticed a significant performance improvement and an increase on cov:
master:
```
#100000 DONE cov: 567 ft: 4078 corp: 124/33Kb lim: 4096 exec/s: 793 rss: 499Mb
```
PR:
```
#100000 DONE cov: 568 ft: 3833 corp: 113/15188b lim: 1746 exec/s: 1250 rss: 544Mb
```
ACKs for top commit:
achow101:
ACK f482d0e366
marcofleon:
Tested ACK f482d0e366. Saw the same slight increase in coverage. Executed 100,000 runs several times and total time went from 30-35 sec to 20-25 sec.
stratospher:
ACK f482d0e. saw similar coverage stats
Tree-SHA512: 1a96dbc22a0aed396b7f8cc9b13534b7f20a461f64f167c69c650529d535e360499f1a501abc1f957f7541ee1860b36a5580aa488a1edbfa0270c9ed83ef741d
51f7668d31 addrman: change nid_type from int to int64_t (Martin Zumsande)
051ba3290e addrman, refactor: introduce user-defined type for internal nId (Martin Zumsande)
Pull request description:
With `nIdCount` being incremented for each addr received, an attacker could cause an overflow in the past, see https://bitcoincore.org/en/2024/07/31/disclose-addrman-int-overflow/
Even though that attack was made infeasible indirectly by addr rate-limiting (PR #22387), to be on the safe side and prevent any regressions change the `nId`s used internally to `int64_t`.
This is being done by first introducing a user-defined type for `nId`s in the first commit, and then updating it to `int64_t` (thanks sipa for help with this!).
Note that `nId` is only used internally, it is not part of the serialization, so `peers.dat` should not be affected by this.
I assume that the only reason this was not done in the past is to not draw attention to this previously undisclosed issue.
ACKs for top commit:
naumenkogs:
ACK 51f7668d31
stratospher:
ACK 51f7668d31. I think it's a good change to make the nId space large(64 bits) so that the nId values are distinct.
achow101:
ACK 51f7668d31
Tree-SHA512: 68d4b8b0269a01a9544bedfa7c1348ffde00a288537e4c8bf2b88372ac7d96c4566a44dd6b06285f2fcf31b4f9336761e3bca7253fbc20db5e0d04e887156224
58499b00d0 refactor: move `SignSignature` helpers to test utils (Sebastian Falbesoner)
Pull request description:
These helpers haven't been used in production code since segwit was merged more than eight years ago (see commit 605e8473, PR #8149), so it seems appropriate to move them to the test utils module. As suggested by instagibbs, see https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1697515508.
ACKs for top commit:
instagibbs:
ACK 58499b00d0
pablomartin4btc:
ACK 58499b00d0
Tree-SHA512: a52d3b92b477246f2ceb57c3690d0229a492b65a15dae331faeae9d96e5907f7fe1176edc1530243e0f088586984fd7ba435a0a2d2f2531c04d076fdf3f4095f
a240e150e8 streams: remove AutoFile::Get() entirely (Pieter Wuille)
e624a9bef1 streams: cache file position within AutoFile (Pieter Wuille)
Pull request description:
Fixes #30833.
Instead of relying on frequent `ftell` calls (which appear to cause a significant slowdown on some systems) in XOR-enabled `AutoFile`s, cache the file position within `AutoFile` itself.
ACKs for top commit:
achow101:
ACK a240e150e8
davidgumberg:
untested reACK a240e150e8
theStack:
Code-review ACK a240e150e8
Tree-SHA512: fd3681edc018afaf955dc7a41a0c953ca80d46c1129e3c5b306c87c95aae93b2fe7b900794eb8b6f10491f9211645e7939918a28838295e6873eb226fca7006f
e4e3b44e9c net: call `Select` with reachable networks in `ThreadOpenConnections` (brunoerg)
829becd990 addrman: change `Select` to support multiple networks (brunoerg)
f698636ec8 net: add `All()` in `ReachableNets` (brunoerg)
Pull request description:
This PR changes addrman's `Select` to support multiple networks and change `ThreadOpenConnections` to call it with reachable networks. It can avoid unnecessary `Select` calls and avoid exceeding the max number of tries (100), especially when turning a clearnet + Tor/I2P/CJDNS node to Tor/I2P/CJDNS. Compared to #29330, this approach is "less aggresive". It does not add a new init flag and does not impact address relay.
I did an experiment of calling `Select` without passing a network until it finds an address from a network that compose 20% ~ 25% of the addrman (limited to 100 tries).
ACKs for top commit:
achow101:
ACK e4e3b44e9c
vasild:
ACK e4e3b44e9c
naumenkogs:
ACK e4e3b44e9c
Tree-SHA512: e8466b72b85bbc2ad8bfb14471eb27d2c50d4e84218f5ede2c15a6fa3653af61b488cde492dbd398f7502bd847e95bfee1abb7e01092daba2236d3ce3d6d2268
a97f43d63a fuzz: Add harness for p2p headers sync (marcofleon)
a0eaa4749f Add FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION in PoW check (marcofleon)
a3f6f5acd8 build: Automatically define FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION for fuzz builds (marcofleon)
0c02d4b2bd net_processing: Make MAX_HEADERS_RESULTS a PeerManager option (marcofleon)
Pull request description:
This PR reopens https://github.com/bitcoin/bitcoin/pull/28043. It's a regression fuzz test for https://github.com/bitcoin/bitcoin/pull/26355 and [a couple bugs](ed6cddd98e) that were addressed in https://github.com/bitcoin/bitcoin/pull/25717. This should help us move forward with the [removal of mainnet checkpoints](https://github.com/bitcoin/bitcoin/pull/25725).
It seems like the main concern in https://github.com/bitcoin/bitcoin/pull/28043 was the global mock function for proof of work. This PR aims to be an improvement by replacing the previous approach with a fuzz build configured using `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION`. This ensures that the simplified test code will never be in a release binary. If we agree this is the way to go, there are some other places (for future targets) where this method could be used.
In this target, PoW isn't being tested, so the goal is to bypass the check and let the fuzzer do its thing. In the other harnesses where PoW is actually being fuzzed, `CheckProofOfWork` is now `CheckProofOfWorkImpl`. So, the only change to that function is in the name.
More about `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION` can be found at https://llvm.org/docs/LibFuzzer.html#fuzzer-friendly-build-mode and https://github.com/AFLplusplus/AFLplusplus/blob/stable/docs/fuzzing_in_depth.md#d-modifying-the-target.
ACKs for top commit:
naumenkogs:
ACK a97f43d63a
dergoegge:
reACK a97f43d63a
instagibbs:
tested ACK a97f43d63a
brunoerg:
ACK a97f43d63a
Tree-SHA512: 60b0bc6aadd8ca4c39db9cbba2da2debaaf68afcb6a8dd75c1ce48ca9e3996948fda8020930b6771a424e0f7c41b0b1068db4aa7dbe517f8fc152f1f712058ad
Automatically add topologically-valid subsets of the potential set pot
to inc. It can be proven that these must be part of the best reachable
topologically-valid set from that work item.
This is a crucial optimization that (apparently) reduces the maximum
number of iterations from ~2^(N-1) to ~sqrt(2^N).
Co-Authored-By: Suhas Daftuar <sdaftuar@gmail.com>
In each work item, keep track of a conservative overestimate of the best
possible feerate that can be reached from it, and then use these to avoid
exploring hopeless work items.
Before this commit, the worst case for linearization involves clusters which
break apart in several smaller components after the first candidate is
included in the output linearization.
Address this by never considering work items that span multiple components
of what remains of the cluster.
1eac96a503 Compare FromUserHex result against other hex validators and parsers (Lőrinc)
19947863e1 Use BOOST_CHECK_EQUAL for optional, arith_uint256, uint256, uint160 (Lőrinc)
743ac30e34 Add std::optional support to Boost's equality check (Lőrinc)
Pull request description:
Enhanced `FromUserHex` coverage by:
* Added `std::optional` support to `BOOST_CHECK_EQUAL`, allowing direct comparisons of `std::optional<T>` with other `T` expected values.
* Increased fuzz testing for hex parsing to validate against other hex validators and parsers.
----
* Use BOOST_CHECK_EQUAL for https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706637780 arith_uint256, uint256, uint160
Example error before:
> unknown location:0: fatal error: in "validation_chainstatemanager_tests/chainstatemanager_args": std::bad_optional_access: bad_optional_access
test/validation_chainstatemanager_tests.cpp:781: last checkpoint
after:
> test/validation_chainstatemanager_tests.cpp:801: error: in "validation_chainstatemanager_tests/chainstatemanager_args": check set_opts({"-assumevalid=0"}).assumed_valid_block == uint256::ZERO has failed [std::nullopt != 0000000000000000000000000000000000000000000000000000000000000000]
ACKs for top commit:
stickies-v:
re-ACK 1eac96a503
ryanofsky:
Code review ACK 1eac96a503. Only changes since last review were auto type and fuzz test tweaks.
hodlinator:
ACK 1eac96a503
Tree-SHA512: f1d2c65f0ee4e97830700be5b330189207b11ed0c89a8cebf0f97d43308402a6b3732e10130c79a0c044f7d2eeabfb5359990825aadf02c4ec19428dcd982b00
43cd83b0c7 test: move uint256_tests/operator_with_self to arith_uint256_tests (stickies-v)
c6c994cb2b test: remove test-only uint160S (stickies-v)
62cc4656e2 test: remove test-only uint256S (stickies-v)
adc00ad728 test: remove test-only arith_uint256S (stickies-v)
f51b237723 refactor: rpc: use uint256::FromHex for ParseHashV (stickies-v)
Pull request description:
_Continuation of #30569._
Since fad2991ba0, `uint256S()` has been [deprecated](fad2991ba0 (diff-800776e2dda39116e889839f69409571a5d397de048a141da7e4003bc099e3e2R138)) because it is less robust than the `base_blob::FromHex()` introduced in https://github.com/bitcoin/bitcoin/pull/30482. Specifically, it tries to recover from length-mismatches, recover from untrimmed whitespace, 0x-prefix and garbage at the end, instead of simply requiring exactly 64 hex-only characters. (see also https://github.com/bitcoin/bitcoin/pull/30532)
This PR removes `uint256S()` (and `uint160S()`) completely, with no non-test behaviour change.
Specifically, the main changes in this PR are:
- the (minimal) last non-test usage of `uint256S()` in `ParseHashV()` is removed without behaviour change, which can partially be verified by cherry-picking and/or modifying [this test commit](1f2b0fa86d)).
- the test usage of `uint{160,256}S()` is removed, largely replacing it with `uint{160,256}::FromHex()` where applicable, potentially modifying the test by removing non-hex characters or dropping the test entirely if removing non-hex characters makes it redundant
- the now unused `uint{160,256}S()` functions are removed completely.
- unit test coverage on converting `uint256` <-> `arith_uint256` through `UintToArith256()` and `ArithToUint256()` is beefed up, and `arith_uint256` tests are moved to `arith_uint256_tests.cpp`, removing the `uint256_tests.cpp` dependency on `uint256h`, mirroring how the code is structured.
_Note: `uint256::FromUserHex()` exists to more leniently construct uint256 from user input, allowing "0x" prefixes and too-short-input, as safer alternative to `uint256S()` where necessary._
ACKs for top commit:
l0rinc:
reACK 43cd83b0c7
hodlinator:
re-ACK 43cd83b0c7
ryanofsky:
Code review ACK 43cd83b0c7. Only code change is a small refactoring which looks good. The rest of the PR is all test changes, which I only lightly reviewed, but seem to be positive and do what's described
Tree-SHA512: 48147a4c6af671597df0f72c1b477ae4631cd2cae4645ec54d0e327611ff302c9899e344518c81242cdde82930f6ad23a3a7e6e0b80671816e9f457b9de90a5c
uint256S was previously deprecated for being unsafe. All non-test
usage has already been removed in earlier commits.
1. Tests now use uint256::FromHex() or other constructors wherever
possible without further modification.
2. Tests that can't use uint256::FromHex() because they use input
with non-hex digit characters are
a) modified by dropping the non-hex digit characters if that
provides useful test coverage.
b) dropped if the test without non-hex digit characters does
not provide useful test coverage, e.g. because it is now
duplicated.
Additionally, use BOOST_CHECK_EQUAL where relevant on touched lines
to make error messages more readable.
Add `-ipcbind` option to `bitcoin-node` to listen on an IPC socket and accept
connections from other processes. In the future, there will be an `-ipcconnect`
option added to `bitcoin-wallet` and `bitcoin-node` to allow wallet and gui
processes to connect to the node and access it.
Example usage:
src/bitcoin-node -regtest -debug -ipcbind=unix
src/bitcoin-wallet -regtest -ipcconnect=unix info
src/bitcoin-gui -regtest -ipcconnect=unix
src/bitcoin-mine -regtest -ipcconnect=unix
These helpers haven't been used in production code since segwit was
merged more than eight years ago (see commit 605e8473, PR #8149),
so it seems appropriate to move them to the test utils module.
Can be reviewed via `--color-moved=dimmed-zebra`.
01960c53c7 fuzz: make FuzzedDataProvider usage deterministic (Martin Leitner-Ankerl)
Pull request description:
There exist many usages of `fuzzed_data_provider` where it is evaluated directly in the function call.
Unfortunately, [the order of evaluation of function arguments is unspecified](https://en.cppreference.com/w/cpp/language/eval_order), and a simple example shows that it can differ e.g. between clang++ and g++: https://godbolt.org/z/jooMezWWY
When the evaluation order is not consistent, the same fuzzing/random input will produce different output, which is bad for coverage/reproducibility. This PR fixes all these cases I have found where unspecified evaluation order could be a problem.
Finding these has been manual work; I grepped the sourcecode for these patterns, and looked at each usage individually. So there is a chance I missed some.
* `fuzzed_data_provider`
* `.Consume`
* `>Consume`
* `.rand`
I first discovered this in https://github.com/bitcoin/bitcoin/pull/29013#discussion_r1420236394. Note that there is a possibility that due to this fix the evaluation order is now different in many cases than when the fuzzing corpus has been created. If that is the case, the fuzzing corpus will have worse coverage than before.
Update: In list-initialization the order of evaluation is well defined, so e.g. usages in `initializer_list` or constructors that use `{...}` is ok.
ACKs for top commit:
achow101:
ACK 01960c53c7
vasild:
ACK 01960c53c7
ismaelsadeeq:
ACK 01960c53c7
Tree-SHA512: e56d087f6f4bf79c90b972a5f0c6908d1784b3cfbb8130b6b450d5ca7d116c5a791df506b869a23bce930b2a6977558e1fb5115bb4e061969cc40f568077a1ad
c8e6771af0 test: restrict multiple CLI arguments (naiyoma)
8838c4f171 common/args.h: automate check for multiple cli commands (naiyoma)
Pull request description:
This PR is a continuation of the validation suggested [here](https://github.com/bitcoin/bitcoin/pull/27815) to ensure that only one Request Handler can be specified at a time.
ACKs for top commit:
stratospher:
reACK c8e6771.
achow101:
ACK c8e6771af0
tdb3:
cr re ACK c8e6771af0
Tree-SHA512: f4fe036fee342a54f1a7ac702ac35c40bf3d420fde6ab16313a75327292d5ee5c8ece1be9f852a13fcf73da1148b143b37b4894e294052abdde6eefb2e8c6f3f
8888beea8d scripted-diff: fuzz: Rename fuzz_seed_corpus to fuzz_corpora (MarcoFalke)
Pull request description:
Now that cmake was a breaking change for all fuzz scripts, it seems fine to bundle it with another breaking change to rename the fuzz corpora directory, as discussed and approved in https://github.com/bitcoin-core/qa-assets/issues/200:
* The word "seed" in the old name doesn't really apply. In reality it is a collection of fuzz input seeds, as well as fuzz inputs.
* The rename will also allow in the future (when there is a need and desire) to provide a minimal set of possibly hand-crafted or otherwise non-fuzz-generated fuzz seed inputs to some fuzz targets (and possibly store them in a separate folder and validate that their format is still accurate and matches the fuzz target code).
* Finally, "corpus" is renamed to corpora, to clarify that the folder holds the fuzz inputs for several fuzz targets.
ACKs for top commit:
brunoerg:
ACK 8888beea8d
marcofleon:
ACK 8888beea8d
Tree-SHA512: abc693ca5d946850f04b6349e2a98f8fbc2ba9991be5a025bc0f357e341cbe7510f2f5f0e47b997d07136736d818df361270f372b8fb70860995a0605ca81e4d
a2955f0979 validation: Use span for ImportBlocks paths (TheCharlatan)
20515ea3f5 validation: Use span for CalculateClaimedHeadersWork (TheCharlatan)
52575e96e7 validation: Use span for ProcessNewBlockHeaders (TheCharlatan)
Pull request description:
Makes it friendlier for potential future users of the kernel library if they do not store the headers in a std::vector, but can guarantee contiguous memory.
Take this opportunity to also change the argument of ImportBlocks previously taking a `std::vector` to a `std::span`.
ACKs for top commit:
stickies-v:
re-ACK a2955f0979 - no changes except further walking the ~file~ path of modernizing variable names.
maflcko:
ACK a2955f0979🕑
achow101:
ACK a2955f0979
danielabrozzoni:
ACK a2955f0979
Tree-SHA512: 8b07f4ad26e270b65600d1968cd78847b85caca5bfbb83fd9860389f26656b1d9a40b85e0990339f50403d18cedcd2456990054f3b8b0bedce943e50222d2709
