fa21f83d2983d97006ec1e3c47634dc0fe0349dc ci: Use G++ in valgrind tasks (MarcoFalke)
fabd05bf651138679f76728f974f141ac8ce99a9 refactor: Fix net_processing iwyu includes (MarcoFalke)
fa1622db208025e1744e78c4f5b135db11b293d4 refactor: Make node_id a const& in RemoveBlockRequest (MarcoFalke)
Pull request description:
Currently, `valgrind` is not usable on a default build with GCC. Specifically, `p2p_compactblocks.py --valgrind` gives a false-positive in `RemoveBlockRequest` when comparing `node_id` with `from_peer`. According to the upstream bug report, this happens because both symbols are on the stack and the compiler can more aggressively optimize the compare (order). See https://bugs.kde.org/show_bug.cgi?id=472329#c7
It is possible to work around this bug by pulling at least one value from the stack. For example, by making `from_peer` a `const` reference. Alternatively, by replacing `auto [node_id, list_it]` with `const auto& [node_id, list_it]`, which is done here.
I think this workaround is acceptable, because it does not look like valgrind can trivially fix this. The alternative would be to add a (temporary?) suppression.
Fixes https://github.com/bitcoin/bitcoin/issues/27741
Also, fix iwyu includes, while touching this module.
Also, switch the CI valgrind scripts to use G++.
ACKs for top commit:
achow101:
ACK fa21f83d2983d97006ec1e3c47634dc0fe0349dc
TheCharlatan:
ACK fa21f83d2983d97006ec1e3c47634dc0fe0349dc
darosior:
utACK fa21f83d2983d97006ec1e3c47634dc0fe0349dc
ryanofsky:
Code review ACK fa21f83d2983d97006ec1e3c47634dc0fe0349dc. Code changes all look good but I'm a little confused about purpose of the third commit, so left a question about that
Tree-SHA512: 7b92cdafd525a5ac53ae2c1a7a92e599bc9b5fd5d315a694b493cd5079ac323d884393b57aa18581b7789247a588c9a27d47698de25b340bc76fc9f1dd1850b4
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.
f00345727b8d2bf73409db5cd342e476671e6425 doc: Update `dependencies.md` for Qt 6 (Hennadii Stepanov)
80b917991ed7ff931f0a9211cebf859f674776c4 build, msvc: Update `vcpkg.json` for Qt 6 (Hennadii Stepanov)
30dd1f1644e0441b5310f1eceecfd6a5abc45f68 ci: Update for Qt 6 (Hennadii Stepanov)
629d292f4d846978c682c5f497240c62d62f4bd1 test: Update sanitizer suppressions for Qt 6 (Hennadii Stepanov)
551e13abf82522bad7fdde4ff4bd15d2c8f88b23 guix: Adjust for Qt 6 (Hennadii Stepanov)
c3e9bd086c498e9f1cbdc505949cb17ac1b39f7e qt: Fix compiling for Windows (Hennadii Stepanov)
ab399c4db2e98aee8e01323c4c6cca48b520dc7f depends: Add `native_qt` package (Hennadii Stepanov)
248613eb3ee034bf143821a51635e697dc114e6c depends: Factor out Qt modules' details (Hennadii Stepanov)
0268f52a4cd2b7aa63934526437bcf6912e47d3c depends: Introduce customizable `$(package)_patches_path` variables (Hennadii Stepanov)
5e794e62024eef612e1fbb71c76ea54d17435c14 depends: Bump `qt` package up to 6.7.3 (Hennadii Stepanov)
6d4214925fadc36d26aa58903db5788c742e68c6 cmake: Require Qt 6 to build GUI (Hennadii Stepanov)
Pull request description:
The currently used Qt 5.15 is approaching [EOL](https://www.qt.io/blog/qt-5.15-extended-support-for-subscription-license-holders) and will reach it before the Bitcoin Core v30 release. The recent migration of the build system to CMake makes it possible to switch to Qt 6.
This PR updates the OS runtime compatibility requirements for the Bitcoin Core GUI as follows:
### 1. Linux
Starting with Qt 6.5.0, the `libxcb-cursor0` package is required to be installed at runtime.
### 2. Windows
Cross-compiling does not support LTO. We have to re-add it in a follow-up.
A new style plugin causes minor visual glitches, such as
which will be fixed in follow-ups.
### 3. macOS
`bitcoin-qt` now uses the [Metal](https://developer.apple.com/metal/) backend.
---
**IMPORTANT.** Don't forget to install [Ninja](https://ninja-build.org/).
---
For historical context, please refer to:
- https://github.com/bitcoin/bitcoin/issues/20627
- https://github.com/bitcoin/bitcoin/pull/24798
---
UPD 2024-10-09. Qt 6.8 has been [released](https://www.qt.io/blog/qt-6.8-released), but it has some [drawbacks](https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2402990346) for us. As a result, this PR will stick to Qt 6.7.
UPD 2025-03-18: [Standard support for Qt 5.15 will end after 26th of May 2025](https://www.qt.io/blog/extended-security-maintenance-for-qt-5.15-begins-may-2025)
ACKs for top commit:
laanwj:
re-ACK f00345727b8d2bf73409db5cd342e476671e6425
hodlinator:
re-ACK f00345727b8d2bf73409db5cd342e476671e6425
Tree-SHA512: 367f722e6c3ea4700b5395871c40b6df8c8062fdc822107090449ea4ae4ad2db75cc53a982a678f4c48ce8f9b2d43ed10e6d23b06165ab78713f161db712d895
1. Do not set `C{PLUS}_INCLUDE_PATH` variables
The build system for Qt 6 differs entirely from that of Qt 5. Building a
set of native Qt 6 tools now forms a separate step when cross-compiling.
Under these new circumstances, the `C{PLUS}_INCLUDE_PATH` environment
variables may alter the default include directories for both native and
cross compilers.
Previously, we explicitly unset these variables when invoking clang for
cross-compiling; however, that approach proved suboptimal (see #30451).
This change sets the native toolchain for dependencies explicitly,
rather than relying on the `C{PLUS}_INCLUDE_PATH` environment variables.
Additionally, it facilitates the transition towards using clang for
building native tools when cross-compiling for macOS.
2. Add `ninja` package.
3. Adjust allowed symbol lists.
fa513101212327f45965092652f6497aa28362ec contrib: Warn about using libFuzzer for coverage check (MarcoFalke)
fa17cdb191de71cdb4151408450aa9b3eaea6cb8 test: Avoid script check worker threads while fuzzing (MarcoFalke)
fa900bb2dce8ef3ee11d5980f008995d66877155 contrib: Only print fuzz output on failure (MarcoFalke)
fa82fe2c7364226a758c4a59e1a4a62e3ac4846b contrib: Use -Xdemangler=llvm-cxxfilt in deterministic-*-coverage (MarcoFalke)
fa7e93113052d09cc5ce4332d1c15904341bd132 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 fa513101212327f45965092652f6497aa28362ec
janb84:
Re-ACK [fa51310](fa51310121)
Tree-SHA512: 1a935eb19da98c7c3810b8bcc5287e5649ffb55bf50ab78c414a424fef8e703839291bb24040a552c49274a4a0292910a00359bdff72fa29a4f53ad36d7a8720
6afffba34e086de4cf0bb86729e12d116c1dcc9b contrib: (asmap) add docs about encode and decode commands (jurraca)
67d5cc2a06e6c74e82221d35e2fc03ed6580b240 contrib: (asmap) add documentation on diff and diff-addrs commands (jurraca)
e047b1deca06fd8aafd4aa31d69fb70ef09a7995 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 6afffba34e086de4cf0bb86729e12d116c1dcc9b
brunoerg:
reACK 6afffba34e086de4cf0bb86729e12d116c1dcc9b
laanwj:
re-ACK 6afffba34e086de4cf0bb86729e12d116c1dcc9b
Tree-SHA512: 073e8d7255f7270aa2f5a070332872f5fa6fbe6532eee1f7e3e4158ac0125a49c155f4933bf00655ff3a89f666f3f3bea521e70c516ab09a448845016d2b880a
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>
fa7a40d952ad7588f45f6229aeae754b02fdfb55 contrib: Print deterministic-coverage runs (MarcoFalke)
fa751639fb667dc31a2220e33cbd134aa6b30741 contrib: Make deterministic-coverage error messages more readable (MarcoFalke)
Pull request description:
This is almost a "refactor" to tidy up the error messages. Apart from the messages, the behavior of the tools is identical.
This was requested in https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1969502508.
Previously, the tool would abort the program early on some errors. Now, the tool propagates an `std::result::Result::Err` up to `main` via an early return. Getting rid of the aborts also allows to drop the `RUST_BACKTRACE` env setting.
ACKs for top commit:
hodlinator:
re-ACK fa7a40d952ad7588f45f6229aeae754b02fdfb55
janb84:
ACK [fa7a40d](fa7a40d952)
Tree-SHA512: 6c97861306e2fececa14b2d12deafb78995fc2bcf75e4e22773cb0ab4231de78834db9f1f89b30c49d77499433b1c16c1d90b97eb4069c81855bd2a7944b554f
Pass bitcoin binary command lines from test framework to signet/miner utility
using shell escaping so they are unambigous and don't get mangled if they
contain spaces.
This change is not needed for tests to pass currently, but is a useful change
to avoid CI failures in followup PR
https://github.com/bitcoin/bitcoin/pull/31375 and to avoid other bugs.
a24419f8bed5e1145ce171dbbdad957750585471 contrib: Fix `gen-bitcoin-conf.sh`. (David Gumberg)
Pull request description:
In #31118, the format of bitcoind's `--help` output changed slightly in a way that breaks `gen-bitcoin-conf.sh`, modify the script to accommodate the new format, by starting after the line that says "Options:" and stripping the `-help` options and descriptions from the script output.
Before this PR, all options above `-help` were excluded from the example bitcoin.conf.
ACKs for top commit:
mabu44:
Tested ACK a24419f8bed5e1145ce171dbbdad957750585471
glozow:
ACK a24419f8bed5e1145ce171dbbdad957750585471
rkrux:
tACK a24419f8bed5e1145ce171dbbdad957750585471
BrandonOdiwuor:
crACK a24419f8bed5e1145ce171dbbdad957750585471
Tree-SHA512: 2ef697166d0b37b61ec1a20e357b91d611c932a0e453c4669f74ab69e6310ea1776dce09c1b77e82746072265763cb0c750e6df4c8b4a7d39068fc03f97b221b
fa99c3b544b631cfe34d52fb5e71636aedb1b423 test: Exclude SeedStartup from coverage counts (MarcoFalke)
fa579d663d716c967ccd45d67b46e779e2fa0b48 contrib: Add deterministic-unittest-coverage (MarcoFalke)
fa3940b1cbc94c8ccfde36be1db1adca04fbcaa6 contrib: deterministic-fuzz-coverage fixups (MarcoFalke)
faf905b9b694313bed4531d1299568a101f33fb8 doc: Remove unused -fPIC (MarcoFalke)
fa1e0a72281fde13d704c7766d4d704e009274da gitignore: target/ (MarcoFalke)
Pull request description:
The `contrib/devtools/test_deterministic_coverage.sh` script is problematic:
* It is written in bash. This can lead to issues when running with the ancient bash version shipped by macOS by default, or can lead to other compatibility issues, such as https://github.com/bitcoin/bitcoin/pull/31588#discussion_r1946784827. Also, pipefail isn't set, so IO errors may be silently ignored.
* It is based on gcov. This can lead to issues, such as https://github.com/bitcoin/bitcoin/pull/31588#pullrequestreview-2602169248 (possibly due to prefix-map), or https://github.com/bitcoin/bitcoin/pull/31588#issuecomment-2646395385 (gcovr processing error), or https://github.com/bitcoin/bitcoin/pull/31588#pullrequestreview-2605954001 (gcovr assertion error).
* The script is severely outdated, with the last update to `NON_DETERMINISTIC_TESTS` being in the prior decade.
Instead of patching around all issues one-by-one, just provide a fresh rewrite, based on the recently added `deterministic-fuzz-coverage` tool based on clang, llvm-cov, and llvm-profdata. (Initial feedback indicates that this is a more promising attempt: https://github.com/bitcoin/bitcoin/pull/31588#issuecomment-2649356408 and https://github.com/bitcoin/bitcoin/pull/31588#issuecomment-2649354598).
The new tool also sets `RANDOM_CTX_SEED=21` as suggested by hodlinator in https://github.com/bitcoin/bitcoin/pull/31588#issuecomment-2650784726.
ACKs for top commit:
Prabhat1308:
Concept ACK [`fa99c3b`](fa99c3b544)
hodlinator:
re-ACK fa99c3b544b631cfe34d52fb5e71636aedb1b423
brunoerg:
light ACK fa99c3b544b631cfe34d52fb5e71636aedb1b423
dergoegge:
tACK fa99c3b544b631cfe34d52fb5e71636aedb1b423
janb84:
Concept ACK [fa99c3b](fa99c3b544)
Tree-SHA512: 491d5e6413d929395a5c7caea54817bdc1a0e00562c9728a374d4e92f2e2017dba4a770ecdb2e7317e049df9fdeb390d83c90dff9aa5709f97aa3f6a0e70cdb4
In #31118, the format of bitcoind's `--help` output changed slightly in
a way that breaks `gen-bitcoin-conf.sh`, modify the script to accomodate
the new format, by starting after the line that says "Options:" and
strip the `-help` option and its description from the output.
568fcdddaec2cc8decba5a098257f31729cc1caa scripted-diff: Adjust documentation per top-level target output location (Hennadii Stepanov)
026bb226e96919603af829d0b677779a234a0f6e cmake: Set top-level target output locations (Hennadii Stepanov)
Pull request description:
This PR sets the target output locations to the `bin` and `lib` subdirectories within the build tree, creating a directory structure that mirrors that of the installed targets.
This approach is widely adopted by the large projects, such as [LLVM](e146c1867e/lldb/cmake/modules/LLDBStandalone.cmake (L128-L130)):
```cmake
set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)
set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX})
set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX})
```
The `libsecp256k1` project has also recently [adopted](https://github.com/bitcoin-core/secp256k1/pull/1553) this approach.
With this PR, all binaries are conveniently located. For example, run:
```
$ ./build/bin/fuzz
```
instead of:
```
$ ./build/src/test/fuzz/fuzz
```
On Windows, all required DLLs are now located in the same directory as the executables, allowing to run `bitcoin-chainstate.exe` (which loads `bitcoinkernel.dll`) without the need to copy DLLs or modify the `PATH` variable.
The idea was briefly discussed among the build team during the recent CoreDev meeting.
---
**Warning**: This PR changes build locations of newly built executables like `bitcoind` and `test_bitcoin` from `src/` to `bin/` without deleting previously built executables. A clean build is recommended to avoid accidentally running old binaries.
ACKs for top commit:
theStack:
Light re-ACK 568fcdddaec2cc8decba5a098257f31729cc1caa
ryanofsky:
Code review ACK 568fcdddaec2cc8decba5a098257f31729cc1caa. Only change since last review was rebasing. I'm ok with this PR in its current form if other developers are happy with it. I just personally think it is inappropriate to \*silently\* break an everyday developer workflow like `git pull; make bitcoind`. I wouldn't have a problem with this PR if it triggered an explicit error, or if the problem was limited to less common workflows like changing cmake options in an existing build.
TheCharlatan:
Re-ACK 568fcdddaec2cc8decba5a098257f31729cc1caa
theuni:
ACK 568fcdddaec2cc8decba5a098257f31729cc1caa
Tree-SHA512: 1aa5ecd3cd49bd82f1dcc96c8e171d2d19c58aec8dade4bc329df89311f9e50cbf6cf021d004c58a0e1016c375b0fa348ccd52761bcdd179c2d1e61c105e3b9f
We shouldn't have | at the end of the last clause, as this will make it match
the empty string too (so effectively everything starting with Satoshi: matches).
While doing this, put the | at the beginning of every line of regex rather than
the end, to make it easier to update in the future without accidentally running
into this problem again.
e181bda061ca63021511be6e286fdf6a5818df49 guix: Apply all codesignatures to Windows binaries (Ava Chow)
aafbd23fd97ac242f7f83e5f0fff20044176e126 guix: Apply codesignatures to all MacOS binaries (Ava Chow)
3656b828dc2204418974e94928cc8d915b10ed95 contrib: Sign all Windows binaries too (Ava Chow)
31d325464d0cf2d06888e0c543ae26a944f2ec6b contrib: Sign and notarize all MacOS binaries (Ava Chow)
710d5b5149d0bc36d2643281d81f8f9b0c51b480 guix: Update signapple (Ava Chow)
e8b3c44da6e060464970717bbd0a5bf84867b82c build: Include all Windows binaries for codesigning (Ava Chow)
dd4ec840eeb468e94cfc9e3c72cfbfd6704dc0da build: Include all MacOS binaries for codesigning (Ava Chow)
4e5c9ceb9dd5a6ad8eea689d916a632e4d482812 guix: Rename Windows unsigned binaries to unsigned.zip (Ava Chow)
d9d49cd533bd430776c0cbe2fd666ffec3e6637b guix: Rename MacOS binaries to unsigned.tar.gz (Ava Chow)
c214e5268fa9322a83cbba6d47d33f830efdd89e guix: Rename unsigned.tar.gz to codesigning.tar.gz (Ava Chow)
Pull request description:
I have updated signapple to notarize MacOS app bundles without adding any additional dependencies. Further, it can also sign and apply detached signatures to standalone binaries.
As such, we can use signapple to perform the notarization and stapling steps so that MacOS will run the app bundle after it is installed. `detached-sig-create.sh` is updated to have a notarization step and to download the ticket which will be included in the detached signatures. The workflow is largely unchanged for the MacOS codesigners except for the additional requirement of having an App Store Connect API key and Team UUID, instructions for which can be found at https://github.com/achow101/signapple/blob/master/docs/notarization.md. For guix builders, the workflow is unchanged.
Additionally, the standalone binaries packaged in the MacOS `.tar.gz` and Windows `.zip` will now be codesigned. `detached-sig-create.sh` was updated to handle these, so the workflow for both MacOS and Windows codesigners remains unchanged. For guix builders, the workflow is also unchanged.
Because those binaries will how have codesigned and unsigned versions, the build command is modified to output `-unsigned.{tar.gz,zip}` archives containing the binaries. Since this happens to conflict with the tarball used for codesigning, the codesigning tarball was renamed to `-codesigning.tar.gz`. Both MacOS and Windows codesigners will need to adjust their workflows to account for the new name.
Fixes#15774 and #29749
ACKs for top commit:
Sjors:
Tested ACK e181bda061ca63021511be6e286fdf6a5818df49
davidgumberg:
Tested ACK e181bda061.
pinheadmz:
tested ACK e181bda061ca63021511be6e286fdf6a5818df49
Tree-SHA512: ce0e2bf38e1748cdaa0d13be6f61c3289cd09cfb7d071a68b0b13d2802b3936c9112eda6e4c7b29c535c0995d56b14871442589cdcea2e7707e35c1b278b9263
* Name the fuzz_corpora dir after its real name.
* Add missing cargo lock file.
* Use git instead of diff command to increase compatibility
* Use --help instead of --version to increase compatibility
* Use assert consistently for unexpected errors.
* Remove redundant Stdio::from.
* Fix typos.
The utxo snapshot metadata doesn't seem to contain any block height as per the
CPP code and no such value is read few lines down by the tool code as well.
Related CPP code: bitcoin/bitcoin/blob/28.x/src/node/utxo_snapshot.h#L60-L66
Running the `dumptxoutset` command without a `type` parameter leads
to the following error. Update the tool documentation to make it
easier to follow.
`Invalid snapshot type "" specified. Please specify "rollback" or "latest"`
This change:
1. Collects build artifacts in dedicated locations.
2. Allows running bitcoin-chainstate.exe with bitcoinkernel.dll directly
from the build tree on Windows.
63a8791e15c3ffb44b84ab3e85db62d7997d25fd contrib: fix BUILDDIR in gen-bitcoin-conf script and gen-manpages.py (jurraca)
Pull request description:
The `gen-bitcoin-conf.sh` and `gen-manpages.py` scripts assume a top level `src/` build dir, but in-tree builds are no longer allowed, nor recommended in the build steps. If a user builds `bitcoind` as recommended, these scripts fail. To fix it, we update the `BUILDDIR` env var and update the README accordingly.
Follows up on initial work and discussion in #31332 .
ACKs for top commit:
fjahr:
Code review ACK 63a8791e15c3ffb44b84ab3e85db62d7997d25fd
achow101:
ACK 63a8791e15c3ffb44b84ab3e85db62d7997d25fd
Tree-SHA512: cf4d5b0d2e8b1f5db759bec01e131d8a0c511a2fd183389d2a0488d5fe4a906db2579d944f408b5c966f619edc6b2534023c3521f1fa5f8edd0216d29f3e48db
