The `gcov`-based `CoverageFuzz` script was introduced in
8b6f1c4353, as a CMake's replacement for
the legacy `cov_fuzz` target. However, neither `cov_fuzz` nor
`CoverageFuzz` has a documented usage.
Instead, #32206 documented compiling for fuzz coverage using the
LLVM/Clang toolchain, which does not involve the `CoverageFuzz` script.
This change removes the never-documented `CoverageFuzz` script, which is
likely unused.
037ad77071 fuzz: add IPC round-trip target (Enoch Azariah)
8a739a5510 build: allow ipc fuzz builds (Enoch Azariah)
Pull request description:
As discussed in #23015, this PR adds an IPC fuzz target to exercise the Cap'n Proto/libmultiprocess serialization bridge using an in-process two-way pipe and a reflected test interface.
It covers round-trip serialization for `COutPoint`, `CScript`, `std::vector<uint8_t>`, `UniValue`, and transactions, and exercises libmultiprocess proxy/server interaction. The target guarantees at least one IPC operation per input and is included by default when IPC is enabled in fuzz builds
Coverage [report](https://marcofleon.github.io/coverage/ipc/) provided by marcofleon
ACKs for top commit:
marcofleon:
ACK 037ad77071
sedited:
Re-ACK 037ad77071
Tree-SHA512: aae8835c8a378886b36420c422136c743b235877f6fd6f0c681910903c68e18753598c7f0e0279ce2e64d627241ce3a4e08f7edecc2e38765569c9faa61bf45e
A bare metal build is now supported by setting CMAKE_SYSTEM_NAME=Generic
Skip the platform-dependent feature checks, such as threads and atomics,
which are typically not available on bare metal. Also only make the
boost headers mandatory if they exist for the target.
Also exclude aes from the crypto library, since that requires the secure
allocator, which uses more advance features, such as mutexes.
a154c05d49 cmake: Check dependencies after build option interaction (Hennadii Stepanov)
Pull request description:
At present, `CMakeLists.txt` interleaves configuration-option handling with dependency discovery. As a result, unnecessary checks may be performed. For example:
```
$ cmake -B build --preset dev-mode -DBUILD_FOR_FUZZING=ON
<snip>
-- Found PkgConfig: /usr/bin/pkg-config (found version "2.3.0")
-- Found ZeroMQ: /usr/lib64 (found suitable version "4.3.5", minimum required is "4.0.0")
-- Performing Test HAVE_USDT_H
-- Performing Test HAVE_USDT_H - Success
-- Found USDT: /usr/include
-- Found QRencode: /usr/lib64/libqrencode.so (found version "4.1.1")
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Success
-- Found Threads: TRUE
-- Performing Test HAVE_STDATOMIC
-- Performing Test HAVE_STDATOMIC - Success
-- Found WrapAtomic: TRUE
-- Found OpenGL: /usr/lib64/libOpenGL.so
-- Found WrapOpenGL: TRUE
-- Found WrapVulkanHeaders: /usr/include
-- Found Qt: /usr/lib64/cmake/Qt6 (found suitable version "6.10.1", minimum required is "6.2")
CMake Warning at CMakeLists.txt:206 (message):
BUILD_FOR_FUZZING=ON will disable all other targets and force
BUILD_FUZZ_BINARY=ON.
<snip>
```
This PR restructures the code to ensure that all dependencies are checked only after the final build option interaction.
ACKs for top commit:
fanquake:
ACK a154c05d49
sedited:
Re-ACK a154c05d49
Tree-SHA512: f863316770a1e27e6c08736932f73f297add326943e9c32c721f7e771f38b0fdc651abfed61add5ee4924cfbf2afff91fde005d41d1fd239d401184b6caf5453
ac1ccc5bd9 build: Add CTAD feature check (Pol Espinasa)
9f273f1c1c build: Add path to doc recommended versions for CLANG, GCC and MSVC (Pol Espinasa)
Pull request description:
Adds a compiler features check. If failed, it returns a fatal error.
This early stop at configure time will avoid contributors losing time trying to fix compilation errors because of not fitting the requirements.
Example of the output for GCC:
Version requirement satisfied:
```
$ cmake -B build
-- The CXX compiler identification is GNU 13.3.0
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
...
-- Checking for required C++ features
-- Checking for required C++ features - done
....
```
Compiler feature requirement not satisfied:
```
$ cmake -B build -DCMAKE_C_COMPILER=/usr/bin/gcc-10 -DCMAKE_CXX_COMPILER=/usr/bin/g++-10
-- The CXX compiler identification is GNU 10.5.0
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/g++-10 - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Setting build type to "RelWithDebInfo" as none was specified
-- Performing Test CXX_SUPPORTS__WERROR
-- Performing Test CXX_SUPPORTS__WERROR - Success
-- Found SQLite3: /usr/include (found suitable version "3.45.1", minimum required is "3.7.17")
-- Checking for required C++ features
CMake Error at cmake/module/CheckCXXFeatures.cmake:32 (message):
Compiler lacks Class Template Argument Deduction (CTAD) for aggregates.
This C++ feature is required for src/util/overloaded.h.
You are probably using an old compiler version
The recommended compiler versions can be checked in:
- GCC -> doc/dependencies.md#compiler
- Clang -> doc/dependencies.md#compiler
- MSVC -> doc/build-windows-msvc.md
Call Stack (most recent call first):
CMakeLists.txt:198 (check_cxx_features)
-- Configuring incomplete, errors occurred!
```
Example of the output for Clang:
Compiler feature requirement satisfied:
```
$ cmake -B build -DCMAKE_C_COMPILER=clang-20 -DCMAKE_CXX_COMPILER=clang++-20
-- The CXX compiler identification is Clang 20.1.2
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/clang++-20 - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
...
-- Checking for required C++ features
-- Checking for required C++ features - done
...
```
Compiler feature requirement not satisfied:
```
$ cmake -B build -DCMAKE_C_COMPILER=clang-16 -DCMAKE_CXX_COMPILER=clang++-16
-- The CXX compiler identification is Clang 16.0.6
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/clang++-16 - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Setting build type to "RelWithDebInfo" as none was specified
-- Performing Test CXX_SUPPORTS__WERROR
-- Performing Test CXX_SUPPORTS__WERROR - Success
-- Performing Test CXX_SUPPORTS__G3
-- Performing Test CXX_SUPPORTS__G3 - Success
-- Performing Test LINKER_SUPPORTS__G3
-- Performing Test LINKER_SUPPORTS__G3 - Success
-- Performing Test CXX_SUPPORTS__FTRAPV
-- Performing Test CXX_SUPPORTS__FTRAPV - Success
-- Performing Test LINKER_SUPPORTS__FTRAPV
-- Performing Test LINKER_SUPPORTS__FTRAPV - Success
-- Found SQLite3: /usr/include (found suitable version "3.45.1", minimum required is "3.7.17")
-- Checking for required C++ features
CMake Error at cmake/module/CheckCXXFeatures.cmake:32 (message):
Compiler lacks Class Template Argument Deduction (CTAD) for aggregates.
This C++ feature is required for src/util/overloaded.h.
You are probably using an old compiler version
The recommended compiler versions can be checked in:
- GCC -> doc/dependencies.md#compiler
- Clang -> doc/dependencies.md#compiler
- MSVC -> doc/build-windows-msvc.md
Call Stack (most recent call first):
CMakeLists.txt:198 (check_cxx_features)
-- Configuring incomplete, errors occurred!
```
Edit: The first version of the PR was to check hardcoded compiler versions. See previous PR description for context here.
<details>
Adds a compiler minimum version check. If failed, it returns a fatal error.
This early stop at configure time will avoid contributors losing time trying to fix compilation errors because of not fitting the requirements.
Example of the output for GCC:
Version requirement satisfied:
```
sliv3r@sliv3r-tuxedo:~/Documentos/Projectes/BitcoinCore/bitcoin$ cmake -B BUILD
-- The CXX compiler identification is GNU 13.3.0
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
....
```
Version requirement not satisfied:
```
$ cmake -B build -S . -DCMAKE_C_COMPILER=/usr/bin/gcc-11 -DCMAKE_CXX_COMPILER=/usr/bin/g++-11
-- The CXX compiler identification is GNU 11.5.0
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/g++-11 - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
CMake Error at CMakeLists.txt:91 (message):
GCC >= 12.1 required.
-- Configuring incomplete, errors occurred!
```
Example of the output for Clang:
Version requirement satisfied:
```
sliv3r@sliv3r-tuxedo:~/Documentos/Projectes/BitcoinCore/bitcoin$ cmake -B build
-- The CXX compiler identification is Clang 18.1.3
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
...
```
Version requirement not satisfied:
```
$ cmake -B build -DCMAKE_C_COMPILER=clang-16 -DCMAKE_CXX_COMPILER=clang++-16
-- The CXX compiler identification is Clang 16.0.6
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/clang++-16 - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
CMake Error at CMakeLists.txt:86 (message):
Clang >= 17.0 required.
-- Configuring incomplete, errors occurred!
```
<\detail>
ACKs for top commit:
davidgumberg:
crACK ac1ccc5bd9
achow101:
ACK ac1ccc5bd9
vasild:
ACK ac1ccc5bd9
w0xlt:
ACK ac1ccc5bd9
ryanofsky:
Code review ACK ac1ccc5bd9. Very much agree with all the previous comments to not check specific compiler versions, and glad this PR is in better shape now.
Tree-SHA512: 15a12f56016846427ed8273406c3e9a52a6435bf7033c8fef82bb0e6c9fd75d40a62c79d33f9dd6da8717500f21a629da07620f160b3d43368dd7579f773b4bc
Do not force ENABLE_IPC=OFF when BUILD_FOR_FUZZING is enabled.
The libfuzzer preset is expected to build the full fuzz surface, and the
ipc target should not require an extra opt-in flag to be compiled.
IPC already depends on the normal top-level ENABLE_IPC option and will fail
naturally when its Capn Proto dependencies are unavailable. Keeping the
fuzz-only override would silently skip IPC coverage in standard fuzz builds.
fa4ec13b44 build: Enable -Wcovered-switch-default (MarcoFalke)
fa2670bd4b refactor: Enable -Wswitch in exhaustive switch (MarcoFalke)
Pull request description:
The compiler flag `-Wswitch` is enabled. However, it can not fire when a `default:` case exists. Fix that by removing the default case where a switch is already handling all cases exhaustively.
Also, enable `-Wcovered-switch-default` to catch those cases at compile time in the future.
Also, apply the comment according to the dev notes.
Can be reviewed via `--ignore-all-space`
ACKs for top commit:
stickies-v:
re-ACK fa4ec13b44, no changes except for addressing silent merge conflict from d339884f1d
l0rinc:
ACK fa4ec13b44
achow101:
ACK fa4ec13b44
sedited:
ACK fa4ec13b44
Tree-SHA512: 8dd9e71a8cd338255f43448a59a1a4d40a9fc16e19a707cc10fb71442d4df9f82a0e5fae77868ef49cd0ea27fdd972687572c1a50b6aba7e08c6ce87576afc6a
CMake version 4.3 deprecated the imported target `Sqlite::Sqlite3`.
Use the preferred name `Sqlite3::Sqlite3` instead and provide an
alias for older versions of CMake. Also define the same alias when
using vcpkg.
The leveldb exclusion is required to avoid this warning in the subtree:
```
src/leveldb/util/status.cc:63:7: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]
63 | default:
| ^
1 warning generated.
```
fa6497ba71 build: Set AUTHOR_WARNING on warnings (MarcoFalke)
Pull request description:
Now that the cmake setting `-Werror=dev` is set since commit 6a13a6106e for the CI, guix and the dev cmake preset, it could make sense to notify developers about any warnings.
So do that with a single `AUTHOR_WARNING`.
This can be tested by introducing a bug, like:
```diff
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 6017775fa7..5610e03c66 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -589,7 +589,7 @@ set(Python3_FIND_FRAMEWORK LAST CACHE STRING "")
# improves compatibility with Python version managers that use shims.
set(Python3_FIND_UNVERSIONED_NAMES FIRST CACHE STRING "")
mark_as_advanced(Python3_FIND_FRAMEWORK Python3_FIND_UNVERSIONED_NAMES)
-find_package(Python3 3.10 COMPONENTS Interpreter)
+find_package(Python3 3.210 COMPONENTS Interpreter)
if(NOT TARGET Python3::Interpreter)
list(APPEND configure_warnings
"Minimum required Python not found."
```
Fixes https://github.com/bitcoin/bitcoin/issues/31476.
ACKs for top commit:
l0rinc:
ACK fa6497ba71
purpleKarrot:
ACK fa6497ba71
stickies-v:
ACK fa6497ba71
hebasto:
ACK fa6497ba71, this effectively allows us to use `-Werror=dev` to automatically catch any warning.
sedited:
ACK fa6497ba71
Tree-SHA512: df3b7fa88451527d6a950bd6ebe46e96d1d2f6447c2b53cbe26c6ece0b63a41663a0accfc0ee20c03bc40328363d46f0e7ad88aab1be9b383ad7ff2621363a30
When running `enable_language()`, CMake checks the platform-specific
toolchain components, which includes `install_name_tool` for macOS.
We disable this check when cross-compiling on Linux because
`install_name_tool` is not used and our toolchain file does not provide
it.
However, the current workaround has been breaking CMake's assumptions
since commit eb8facd39606e9472f9ff6de40d7caf76767d0da (CMake 4.1.0,
backported to 4.0.5), which causes an error during configuration using
the Ninja generator.
This change fixes this behaviour by providing a stub executable instead
of `install_name_tool`, as we currently do in the depends build
subsystem.
231dd04b8d build: define CMAKE_COMPILE_WARNING_AS_ERROR as a cache option (will)
Pull request description:
`CMAKE_COMPILE_WARNING_AS_ERROR` is not a cache variable by default in CMake, so it has no value in the configure summary when not set, and even when set cannot be toggled in `ccmake`. Define it as an `option()` to make it a cache BOOL with a default of OFF.
From the original MR to cmake, this was deliberately not set as a cache variable: https://gitlab.kitware.com/cmake/cmake/-/merge_requests/7187 (see Brad King's reply to the collapsed comments from Marc Chevrier).
Most `CMAKE_*` variables which are expected to be toggled by users are (as far as I can research) cache variables by default. Those that are considered likely to be set by the project (e.g. `CMAKE_CXX_STANDARD` or `CMAKE_POSITION_INDEPENDENT_CODE`) are not, along with read-only variables, script/internal variables, platform sppecific variables, template variables. `CMAKE_COMPILE_WARNING_AS_ERROR` may be a slight outlier here.
I count ~ 600 documented CMAKE_* variables, of which ~ 60 are default cache variables.
I could only see a few of these like:
- CMAKE_COMPILE_WARNING_AS_ERROR
- CMAKE_CXX_STANDARD,
- CMAKE_POSITION_INDEPENDENT_CODE
- CMAKE_INTERPROCEDURAL_OPTIMIZATION
...that we (or any project) _might_ want to expose as user-togglable, and would have to add as an `option()` in CMakeLists.txt.
ACKs for top commit:
ajtowns:
Yeah, seems to work for me. ACK 231dd04b8d
fanquake:
ACK 231dd04b8d
hebasto:
ACK 231dd04b8d, tested on Fedora 43.
Tree-SHA512: e380d79188859c97a9f7e45215a789daff55b8bff945a3d61fe4de3daae54bb7196aca0f6a6f1165979e787238dc26e94d217cff35f9642031ff72f4b39655e1
24699fec84 doc: Add initial asmap data documentation (Fabian Jahr)
bab085d282 ci: Use without embedded asmap build option in one ci job (Fabian Jahr)
e53934422a doc: Expand documentation on asmap feature and tooling (Fabian Jahr)
6244212a55 init, net: Implement usage of binary-embedded asmap data (Fabian Jahr)
6202b50fb9 build: Generate ip_asn.dat.h during build process (Fabian Jahr)
634cd60dc8 build: Add embedded asmap data (Fabian Jahr)
Pull request description:
This is the final in a series of PRs that implement the necessary changes for embedding of asmap data into the binary. This last part add the initial asmap data, implements the build changes and adds further documentation.
Currently an asmap file needs to be acquired by there user from some location or the user needs to generate one themselves. Then they need to move the file to the right place in datadir or pass the path to the file as `-asmap=PATH` in order to use the asmap feature. The change here allows for builds to embed asmap data into the bitcoind binary which makes it possible to use the feature without handling of the asmap file by the user. If the user starts bitcoind with `-asmap` the embedded data will be used for bucketing of nodes.
The data lives in the repository at `src/node/data/ip_asn.dat` and can be replaced with a new version at any time. The idea is that the data should be updated with every release. By default the data at that location is embedded into the binary but there is also a build option to prevent this (`-DWITH_EMBEDDED_ASMAP=OFF`). In this case the original behavior of the `-asmap` option is maintained.
ACKs for top commit:
achow101:
ACK 24699fec84
sipa:
ACK 24699fec84
hodlinator:
ACK 24699fec84
Tree-SHA512: c2e33dbeea387efdfd3d415432bf8fa64de80f272c1207015ea53b85bb77f5c29f1dae5644513a23c844a98fb0a4bb257bf765f38b15bfc4c41984f0315b4c6a
CMAKE_COMPILE_WARNING_AS_ERROR is not a cache variable by default in
CMake, so it has no value in the configure summary when not set, and
even when set cannot be toggled in ccmake. Define it as an option() to
make it a cache BOOL with a default of OFF.
-Werror is added to the previous releases job, given it runs on Ubuntu
22.04, which uses an older CMake.
`--compile-no-warning-as-error` can be used, if needed, in future, to
suppress the `CMAKE_COMPILE_WARNING_AS_ERROR` behaviour from a CI
config.
CMAKE_COMPILE_WARNING_AS_ERROR was added to CMake in 3.24.
See https://cmake.org/cmake/help/latest/prop_tgt/COMPILE_WARNING_AS_ERROR.html.
Co-authored-by: willcl-ark <will8clark@gmail.com>
The -ffile-prefix-map option is no longer used and it seems fine to
remove the warning about it possibly breaking coverage builds.
If this needs documentation, the dev notes seem like a better place,
because it also affects other places, such as depends. C.f. commit
407062f2ac
2594d5a189 build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings (Henry Romp)
Pull request description:
Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings that are no longer needed after reordering the Guix build script to perform binary checks after installation.
This PR also removes the unused CMake maintenance targets (`check-security` and `check-symbols`) and updates the Guix security checks to include binaries in the `libexec/` directory (added in PR #31679).
ACKs for top commit:
purpleKarrot:
ACK 2594d5a189
hebasto:
ACK 2594d5a189.
Tree-SHA512: ed451a298f5aae05c177b0033b092faaa7536caeaa3d84da9b8b611e2aa905e1dd337e57aef0efd69ce6ce6ac0cf77dc57adf175079b95bf53dd96d5d0c8118b
Remove CMake settings that are no longer needed after reordering Guix build script to perform binary checks after installation.
Also removes unused CMake maintenance targets (check-security and check-symbols) and updates security checks to include libexec/ directory binaries (see PR #31679).
40dcbf580d build: add -Wtrailing-whitespace=any (fanquake)
d7659cd7e6 build: add -Wleading-whitespace=spaces (fanquake)
d86650220a cmake: Disable `-Wtrailing-whitespace` warnings for RCC-generated files (Hennadii Stepanov)
aabc5ca6ed cmake: Switch from AUTORCC to `qt6_add_resources` (Hennadii Stepanov)
25ae14c339 subprocess: replace tab with space (fanquake)
0c2b9dadd5 scripted-diff: remove whitespace in sha256_sse4.cpp (fanquake)
4da084fbc9 scripted-diff: change whitespace to spaces in univalue (fanquake)
e6caf150b3 ci: add moreutils to lint job (fanquake)
Pull request description:
GCC 15 now has options to turn leading & trailing whitespace into compile failures: https://gcc.gnu.org/gcc-15/changes.html#c-family. Fix the few cases of leading tabs, and trailing whitespace, and then enable `-Wleading-whitespace` and `-Wtrailing-whitespace`.
We currently get PRs that are opened with various whitespace, i.e #33822, so turning that into compile-time failure where possible, seems useful, to avoid a CI roundtrip.
ACKs for top commit:
ajtowns:
utACK 40dcbf580d
hebasto:
re-ACK 40dcbf580d.
Tree-SHA512: a128001ab2abb41cd6d249dcf46be4167ebd608d6b0f1452212a3ec9a383747bea623ab0382ec7bc0ac7a232a47cca5174e1cd73d4eda6751aa3cb2365ad2ede
The test case no longer detects this specific issue for GCC versions
12.1+, as explained in the
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90348 thread and in this
compiler-explorer playground:
https://godbolt.org/z/Y48osrjM8
So remove the test case and update the -fstack-reuse=none cmake
docstring with the underlying affected GCC versions, and the bug URL.
As a first step, implement the equivalent of what was implemented in the
now deprecated libbitcoinconsensus header. Also add a test binary to
exercise the header and library.
Unlike the deprecated libbitcoinconsensus the kernel library can now use
the hardware-accelerated sha256 implementations thanks for its
statically-initialzed context. The functions kept around for
backwards-compatibility in the libbitcoinconsensus header are not ported
over. As a new header, it should not be burdened by previous
implementations. Also add a new error code for handling invalid flag
combinations, which would otherwise cause a crash.
The macros used in the new C header were adapted from the libsecp256k1
header.
To make use of the C header from C++ code, a C++ header is also
introduced for wrapping the C header. This makes it safer and easier to
use from C++ code.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
Remove CMAKE_SKIP_INSTALL_RPATH from CMakeLists.txt and add CMAKE_SKIP_RPATH to the Guix build script. This keeps build-environment-specific settings in the build scripts rather than hardcoded in the CMake configuration.
This will become available in Clang 21:
> ThreadSafetyAnalysis now supports -Wthread-safety-pointer, which
> enables warning on passing or returning pointers to guarded variables
> as function arguments or return value respectively. Note that
> ThreadSafetyAnalysis still does not perform alias analysis. The
> feature will be default-enabled with -Wthread-safety in a future release.
See
https://github.com/llvm/llvm-project/blob/main/clang/docs/ReleaseNotes.rst.
a5ac43d98d doc: Add release notes describing bitcoin wrapper executable (Ryan Ofsky)
258bda80c0 doc: Mention bitcoin wrapper executable in documentation (Ryan Ofsky)
d2739d75c9 build: add bitcoin.exe to windows installer (Sjors Provoost)
ba649c0006 ci: Run multiprocess tests through wrapper executable (Ryan Ofsky)
29bdd743bb test: Support BITCOIN_CMD environment variable (Ryan Ofsky)
9c8c68891b multiprocess: Add bitcoin wrapper executable (Ryan Ofsky)
5076d20fdb util: Add cross-platform ExecVp and GetExePath functions (Ryan Ofsky)
Pull request description:
Intended to make bitcoin command line features more discoverable and allow installing new multiprocess binaries in libexec/ instead of bin/ so they don't cause confusion.
Idea and implementation of this were discussed in https://github.com/bitcoin/bitcoin/issues/30983.
---
Initial implementation of this feature is deliberately minimal so the UX can evolve in response to feedback and there are not too many details to debate and discuss in a single PR. But many improvements are possible or planned:
- Adding manpage and bash completions.
- Showing nicer error messages that detect if an executable isn't installed and suggest how to fix [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2073194474)
- Showing wrapper command lines in subcommand in help output [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077800405). This could be done conditionally as suggested in the comment or be unconditional.
- Showing wrapper command lines in subcommand error output. There is a bitcoin-cli error pointed out in [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091152243) that is needlessly confusing.
- Integrating help so `bitcoin help subcommand` invokes `bitcoin subcommand -h`. `bitcoin -h subcommand` should also be supported and be equivalent [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2093116725)
- Adding support for `bitcoin-util` subcommands. Ideal interface would probably be more like `bitcoin grind` not `bitcoin util grind` but this has been punted for now. Supporting subcommands directly would require some ArgsManager modifications
- Adding a dedicated python functional test for the wrapper. Right now there is some CI coverage by setting the `BITCOIN_CMD` variable, but this doesn't cover things like the help output and version output, and support for different directory layouts.
- Better `--multiprocess` (`-m`) / `--monolithic` (`-M`) default selection. Right now, default is monolithic but it probably makes sense to chose more intelligently depending on whether -ipc options are enabled and what binaries are available.
- Maybe parsing `bitcoin.conf` and supporting options to control wrapper behavior like custom locations or preferences or aliases.
- Better command command line usability. Allow combining short options like (`-ah`). Allow fuzzy matching of subcommands or suggestions if you misspell. (suggested by stickies in review club)
- Not directly related to this PR but `bitcoin-cli named` implementation used by the wrapper should do a better job disambiguating named arguments from base64 arguments ending in = as pointed out in [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091886628)
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722). A review club meeting for it took place in https://bitcoincore.reviews/31375
ACKs for top commit:
Sjors:
utACK a5ac43d98d
achow101:
ACK a5ac43d98d
vasild:
ACK a5ac43d98d
theStack:
ACK a5ac43d98d
ismaelsadeeq:
fwiw my last review implied an ACK a5ac43d98d
hodlinator:
ACK a5ac43d98d
Tree-SHA512: 570e6a4ff8bd79ef6554da3d01f36c0a7c6d2dd7dace8f8732eca98f4a8bc2284474a9beadeba783114fe2f3dd08b2041b3da7753bae0b7f881ec50668cb821f
Windows application manifests provide several benefits. However, on the
master branch, the linker generates and embeds manifests only when
building with MSVC.
This change unifies manifest embedding for both native and
cross-compilation.
