b96f1a696a add clang/llvm based coverage report generation (Prabhat Verma)
Pull request description:
Followed up from the [comment](https://github.com/bitcoin/bitcoin/issues/31927#issuecomment-2674522975) on the issue [#31927](https://github.com/bitcoin/bitcoin/issues/31927) , issues have been observed building coverage reports with `gcov` in MacOs and NixOs. This PR adds the steps to generate a coverage report based on the default llvm/clang tooling.
ACKs for top commit:
Crypt-iQ:
tACK b96f1a696a
hodlinator:
re-ACK b96f1a696a
janb84:
Re ACK [b96f1a6](b96f1a696a)
Tree-SHA512: bc54f170e84bb76b3eba7285bd49f051c0b99b784d583a550d8e51511497bcc4df8964bbe3991777648d2f829809db8eabb0cbf0d25f9da5e49e1cfc62f6d8d0
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
28dc118001 fuzz: wallet: fix crypter target (brunoerg)
Pull request description:
The crypter target has an issue, it's calling `DecryptKey` with a random secret and a random public key that will unlikely be related to the key used to encrypt, so it won't have any effect. This PR changes fixes it and also removes the `DecryptSecret` call since this function is already (and only) called within `DecryptKey`.
ACKs for top commit:
maflcko:
lgtm ACK 28dc118001🥊
Tree-SHA512: e96b7d33879bf06eeec0726e74e8e0d7020997659bf97dfca5d7c1a7ba65c4d93c78e666b97eebde110564cef2eefc7209d3e3586e4658145827b14d1b01dfc9
fa69c42fdf refactor: Remove spurious virtual from final ~CZMQNotificationInterface (MarcoFalke)
Pull request description:
`virtual` does not make sense here, because:
* The class is `final`, thus the destructor isn't overridden in a derived class
* The destructor also isn't overriding the destructor of the base, clarified in commit 2b3ea39de4
* Clang 21 may warn about this
```
src/zmq/zmqnotificationinterface.h:25:13: error: virtual method '~CZMQNotificationInterface' is inside a 'final' class and can never be overridden [-Werror,-Wunnecessary-virtual-specifier]
25 | virtual ~CZMQNotificationInterface();
| ^
```
Fix all issues by removing it.
ACKs for top commit:
davidgumberg:
crACK fa69c42fdf
janb84:
ACK [fa69c42](fa69c42fdf)
TheCharlatan:
ACK fa69c42fdf
Tree-SHA512: 26ea977f31fe24c116d68dea6c583de7c6fc480877e1baefcde11db4ac191e352027d492ee6ad69a60fe4ff537e0841c638b3a3e81356d9e00c60030845fc96e
4774a0c923 test: fix spelling in Python code comment (John Bampton)
Pull request description:
Fixed a couple of typos
Top commit has no ACKs.
Tree-SHA512: 5334995672b2c7d4a9cb916f71dff6a2ce13dc7ced6bbc30ddb0fe8e0ae0b4094b675b3dfced1ffc1b92e3a33ee22df07af3032b8c2928f27051b6376dca3361
4a679936bb ci, windows: Do not exclude `wallet_migration.py` in command line (Hennadii Stepanov)
Pull request description:
This PR amends the recently merged https://github.com/bitcoin/bitcoin/pull/31176 to resolve a silent merge conflict with the previously merged https://github.com/bitcoin/bitcoin/pull/31248.
Since https://github.com/bitcoin/bitcoin/pull/31248, it is no longer necessary to use `--exclude wallet_migration.py`, as the test is skipped due to not using previous releases.
The `wallet_migration.py` test itself still needs to be fixed for Windows by someone who will work on https://github.com/bitcoin/bitcoin/issues/32192.
ACKs for top commit:
davidgumberg:
crACK 4a679936bb
Tree-SHA512: f42428016958cdaccb509cc49341e726eaf1314d85989a7b49888f3862dc4ea0c2988a4792ae62dd925302d0073906397801c8dd2fb06c23381d7cad38730249
Removed the wallet restrictions for rpc_deprecated.py and added specific test case for the current deprecated rpc.
skip_test_if_missing_module will skip the whole test when the wallet is missing, even if a part of the test is non-wallet related.
Rename the `_randomize_credentials` parameter to Proxy's constructor to
`tor_stream_isolation` to make it more clear, and more specific what its
purpose is.
Also change all call sites to use a named parameter.
7bb83f6718 test: create assert_not_equal util and add to where imports are needed (kevkevin)
Pull request description:
In the functional tests there are lots of cases where we assert != which we now swap with assert_not_equal to be more readable
This is motivated/uses logic from this PR which was closed https://github.com/bitcoin/bitcoin/pull/28528
This partially helps https://github.com/bitcoin/bitcoin/issues/23119
I've broken it up to just `assert_not_equal` to keep the PR smaller as suggested in https://github.com/bitcoin/bitcoin/pull/28528#issuecomment-1959945805
I can create follow up PR's if this is wanted
ACKs for top commit:
hodlinator:
re-ACK 7bb83f6718
ryanofsky:
Code review ACK 7bb83f6718. Only change since last review is fixing error message formatting and passing it as a keyword argument
janb84:
Re-ACK [7bb83f6](7bb83f6718)
Tree-SHA512: de09f41a690033a5b61e6f861d3bd69a32b889d6655a28fbc0d5cfac9f7ec9c642432967d33913970882b4cfdd47bdd377d0ddc44e25976cbaa49f7f9d8f7b10
8e4a0ddd50 torcontrol: Add comment explaining Proxy credential randomization for Tor privacy (Eval EXEC)
ec5c0b26ce torcontrol: Define tor reply code as const to improve maintainability (Eval EXEC)
Pull request description:
This PR want to:
1. replace tor repy code with const to improve out maintainability.
2. cherry-picked https://github.com/bitcoin/bitcoin/pull/31973 , add comment to explain Proxy credential randomization for Tor privacy
ACKs for top commit:
hodlinator:
re-ACK 8e4a0ddd50
laanwj:
re-ACK 8e4a0ddd50
Tree-SHA512: 038daa6508ca88fceed5c8e155430614cb56976f36d1f8baee5114bca1141122cf94f51814a869848b3442691ee765cbf609cf946b2b35d5135015a9b749d917
6afffba34e contrib: (asmap) add docs about encode and decode commands (jurraca)
67d5cc2a06 contrib: (asmap) add documentation on diff and diff-addrs commands (jurraca)
e047b1deca contrib: (asmap) add diff-addrs example to README (jurraca)
Pull request description:
This README was a little sparse in my opinion, and was missing a mention of the `diff-addrs` command.
The README updates add background and examples for each command, split in two sections (encode/decode and diff/diff-addrs). This is intended to help people know how and when to run the commands available in the `asmap-tool.py` script.
However, I could use some confirmation on the behavior of the `--fill` flag. It's true that files generated with this flag set cannot be used to diff files after the fact, but i don't quite follow what the fill flag does to make that true. sipa could you maybe provide some insight?
ACKs for top commit:
fjahr:
re-ACK 6afffba34e
brunoerg:
reACK 6afffba34e
laanwj:
re-ACK 6afffba34e
Tree-SHA512: 073e8d7255f7270aa2f5a070332872f5fa6fbe6532eee1f7e3e4158ac0125a49c155f4933bf00655ff3a89f666f3f3bea521e70c516ab09a448845016d2b880a
0ff66b1c4a fuzz: coinselection: cover `SetBumpFeeDiscount` (brunoerg)
Pull request description:
`SetBumpFeeDiscount` sets the bump fee discount which is used to calculate the waste. We currently have no fuzz coverage for this function, so this PR adds it by calling `SetBumpFeeDiscount` before `RecalculateWaste`.
ACKs for top commit:
marcofleon:
ACK 0ff66b1c4a
Tree-SHA512: d5c1d97daaeb7f9b096bf9bdf6374b8a674a75f464e2b9bb3e1e1774a5805b22840ca1f31bae63f106640d9ce27a99432c3034524340be91c235f6ec3b185cff
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
In the functional tests there are lots of cases where we assert != which
this new util will replace, we also are adding the imports and the new assertion
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.
This makes it humanly possible to track progress as only "[N/M]"-lines are printed as long as we succeed.
Also, use char (a, b) to indicate run_id instead of u8 (0, 1).
Also, use emojis to indicate final success or error.
Co-Authored-By: Hodlinator <172445034+hodlinator@users.noreply.github.com>
25b56fd9b4 ci: Test cross-built Windows executables on Windows natively (Hennadii Stepanov)
3501bca8c7 ci: Move "Windows cross" job from Cirrus CI to GHA CI (Hennadii Stepanov)
f8619196ce ci: Use `bash` by default for all platforms (Hennadii Stepanov)
Pull request description:
This PR enables on the CI tests of cross-compiled Windows binaries on Windows.
It is important to have such tests in CI because the release binaries for Windows are also cross-compiled.
Two functional tests, `wallet_migration.py` and `wallet_multiwallet.py`, are temporarily disabled. They require fixes, such as https://github.com/bitcoin/bitcoin/pull/31410, and adjustments for error message handling. Re-enabling these tests will be addressed in follow-up PRs.
Resolves https://github.com/bitcoin/bitcoin/issues/31071.
ACKs for top commit:
davidgumberg:
tested reACK 25b56fd9b4
hodlinator:
re-ACK 25b56fd9b4
willcl-ark:
utACK 25b56fd9b4
maflcko:
review-only ACK 25b56fd9b4🍎
Tree-SHA512: fb9150807b7ebb248e8f4fe7b16e5179251e7be9336459287787f27e542583d73d937e6969667fd836378b676bb9be7f66756dc1abca8a01364bc9ee3e3720a5
This has no functional affect, as the any CBlockIndex*s which
to_mark_failed is set to will already have been marked failed.
Also prevents a situation where block already marked as
BLOCK_FAILED_CHILD is again unconditionally marked as
BLOCK_FAILED_VALID in the final |= BLOCK_FAILED_VALID.
invalid_block ----------> block_index
- before this commit, only if block_index is not invalid, it will mark
block_index as BLOCK_FAILED_CHILD
- it's possible that block_index encountered is invalid and was marked
as BLOCK_FAILED_VALID previously
- in this case, correctly update BlockStatus of block_index by
clearing BLOCK_FAILED_VALID and then setting it to BLOCK_FAILED_CHILD
when a block is invalidated using InvalidateBlock, check that:
1. it's status is BLOCK_FAILED_VALID
2. it's children's status is BLOCK_FAILED_CHILD
and not BLOCK_FAILED_VALID
3. it's ancestors are valid
this block of code is not reached on master since other than
initialisation, all other iterations have invalid_walk_tip
and to_mark_failed pointers in some form of this layout
where 1, 2, 3 and 4 are block heights.
invalid_walk_tip
↓
1 <- 2 <- 3 <- 4
↑
to_mark_failed
fix it so that blocks are correctly marked as BLOCK_FAILED_CHILD
if it's a descendant of BLOCK_FAILED_VALID block.
This change stresses that all ZMQ messages share the same structure
and that they differ only in the format of the bodies. Previously this
was not clear.
Further it removes the notion of endianness of 32-byte hashes,
as it was misleading, and replaces it with the term 'reversed byte
order' (as opposed to natural or normal byte order produced by hashing
functions).
Additionally, it states that ZMQ 32-byte hashes are in the same format
as in RPC. Previously it incorrectly stated that the two were in
different formats.
* Range-for avoids ++i/i++ debate and decreases linecount.
* seen_multipath is only used if multipath_segment_index hasn't already been set. Rename it to seen_substitutes to better describe what it does, now that the context implies its involved in multipath.
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
