backport: add CMake build system alongside Autotools - #7481
backport: add CMake build system alongside Autotools#7481PastaPastaPasta wants to merge 14 commits into
Conversation
|
⛔ Blockers found — Sonnet deferred (commit ca2750c) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e732253572
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| set(PACKAGE_TARNAME "bitcoin") | ||
| set(BITCOIN_GUI_NAME "bitcoin-qt") | ||
| set(BITCOIN_DAEMON_NAME "bitcoind") | ||
| set(BITCOIN_CLI_NAME "bitcoin-cli") | ||
| set(BITCOIN_TX_NAME "bitcoin-tx") |
There was a problem hiding this comment.
When the MinGW deploy target is built, Maintenance.cmake stages files using $<TARGET_FILE_NAME:...>, which yields dash-qt.exe, dashd.exe, and the other Dash OUTPUT_NAME values, but share/setup.nsi.in is configured here to load bitcoin-qt.exe, bitcoind.exe, and similar names. Consequently the generated NSIS script references files that were never staged and cannot produce the installer; PACKAGE_TARNAME also registers the wrong URI class. Set these substitutions to the Dash names already defined by the Autotools build.
Useful? React with 👍 / 👎.
|
|
||
| if(WITH_MULTIPROCESS) | ||
| add_library(bitcoin_ipc_test STATIC EXCLUDE_FROM_ALL | ||
| ipc_test.cpp |
There was a problem hiding this comment.
With WITH_MULTIPROCESS=ON and the default BUILD_TESTS=ON, CMake adds ipc_test.cpp, ipc_test.capnp, and ipc_tests.cpp, but a repo-wide search confirms none of those files exists in this Dash tree. Configuration therefore fails before any multiprocess build can start; either backport the test sources or omit this upstream-only test block.
Useful? React with 👍 / 👎.
| list(APPEND installable_targets bitcoind) | ||
| endif() | ||
| if(WITH_MULTIPROCESS) | ||
| add_executable(bitcoin-node |
There was a problem hiding this comment.
When multiprocess support is enabled with tests disabled to bypass the missing test files, CMake installs bitcoin-node here and bitcoin-gui in src/qt/CMakeLists.txt, whereas configure.ac, the multiprocess documentation, and the executables' own EXE_NAME constants identify them as dash-node and dash-gui. Add the corresponding OUTPUT_NAME properties so CMake installations expose the documented binaries and IPC code resolves the expected sibling executable names.
Useful? React with 👍 / 👎.
| unset(dashbls_include_dirs) | ||
|
|
||
| # Stable, backwards-compatible consensus functionality. | ||
| add_library(bitcoin_consensus STATIC EXCLUDE_FROM_ALL |
There was a problem hiding this comment.
For users switching to the new CMake build, this creates only an internal static archive and no install rule includes it, its public header, or its pkg-config metadata. That leaves the documented libdashconsensus API in doc/shared-libraries.md unavailable from cmake --install, unlike the existing Autotools build, which produces libdashconsensus.la; provide an installable shared-library target rather than using only this internal archive.
Useful? React with 👍 / 👎.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR replaces the dummy CMake project with a complete Dash Core build system. It adds configuration and platform detection, dependency and toolchain integration, core libraries and executables, wallet/GUI/ZMQ/IPC support, tests, benchmarks, fuzzing, coverage, deployment scripts, vcpkg metadata, CI workflows, and Windows build documentation. Testing instructions are updated for CMake and CTest. Estimated code review effort: 5 (Critical) | ~150 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (9)
CMakePresets.json (1)
6-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare an out-of-source
binaryDirfor both presets.Without it, the output location depends on the caller’s working directory. Set a deterministic ignored build path, e.g.
${sourceDir}/build/${presetName}, socmake --presetcannot populate the checkout. CMake 3.21 permits omittedbinaryDirand falls back to regular CMake behavior. (cmake.org)Proposed fix
"generator": "Visual Studio 17 2022", "architecture": "x64", + "binaryDir": "${sourceDir}/build/${presetName}", "toolchainFile": "$env{VCPKG_ROOT}\\scripts\\buildsystems\\vcpkg.cmake",Also applies to: 24-39
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CMakePresets.json` around lines 6 - 21, Add a deterministic out-of-source binaryDir to both presets, using the preset name under the source directory (for example, ${sourceDir}/build/${presetName}). Keep the existing preset configuration unchanged so each cmake --preset invocation writes only to its own ignored build directory.CMakeLists.txt (4)
764-782: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDash-specific options are missing from the configure summary.
ENABLE_MINER,ENABLE_STACKTRACES, andENABLE_CRASH_HOOKSare the Dash-only switches restored in this PR (and stacktraces can be silently flipped off by the detection block at lines 625-638), so they're exactly the values a user needs echoed back.♻️ Proposed addition
message(" USDT tracing ........................ ${WITH_USDT}") +message(" in-wallet miner ..................... ${ENABLE_MINER}") +message(" stack traces ........................ ${ENABLE_STACKTRACES}") +message(" crash hooks ......................... ${ENABLE_CRASH_HOOKS}") message(" QR code (GUI) ....................... ${WITH_QRENCODE}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CMakeLists.txt` around lines 764 - 782, Add the Dash-specific configuration switches ENABLE_MINER, ENABLE_STACKTRACES, and ENABLE_CRASH_HOOKS to the configure summary near the existing optional-feature messages. Echo each variable’s final value, including any value changes made by the stacktrace detection logic, so the summary reflects the effective configuration.
674-693: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead
wrap_exceptions_flagsaccumulation.The
VAR wrap_exceptions_flagsresult is never consumed (the literal-Wl,-wrap,...options are added explicitly below) and is then unset. Since onlyRESULT_VARmatters here, droppingVARmakes the probe intent clearer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CMakeLists.txt` around lines 674 - 693, Remove the unused VAR wrap_exceptions_flags argument from the try_append_linker_flag call while preserving RESULT_VAR CRASH_HOOKS_WRAPPED_CXX_ABI and the existing target_link_options behavior. Also remove the now-unnecessary unset(wrap_exceptions_flags) statement.
382-394: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
SANITIZERSis consumed but never declared as a cache variable.It works via
-DSANITIZERS=...(untyped cache entry), but it won't show up in the configure summary tooling /ccmake/cmake-guiand has no help text, unlike the neighboringAPPEND_*cache strings.♻️ Proposed change
+set(SANITIZERS "" CACHE STRING "Comma-separated list of sanitizers to enable, e.g. \"address,undefined\".") + add_library(sanitize_interface INTERFACE)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CMakeLists.txt` around lines 382 - 394, Declare SANITIZERS as a typed cache string with descriptive help text near the neighboring APPEND_* cache variables, preserving its existing use in the sanitizer flag check and allowing it to appear in configure summaries, ccmake, and cmake-gui.
117-126: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
WARN_INCOMPATIBLE_BDB=OFFno longer suppresses the incompatible-BDB warning.The version warning is emitted unconditionally and only the follow-up hint respects
WARN_INCOMPATIBLE_BDB, so the option's help text ("Warn when using a Berkeley DB version other than 4.8.") doesn't match behavior.🛠️ Proposed fix
if(NOT BerkeleyDB_VERSION VERSION_EQUAL 4.8) - message(WARNING "Found Berkeley DB (BDB) other than 4.8.\n" - "BDB (legacy) wallets opened by this build will not be portable!" - ) if(WARN_INCOMPATIBLE_BDB) - message(WARNING "If this is intended, pass \"-DWARN_INCOMPATIBLE_BDB=OFF\".\n" + message(WARNING "Found Berkeley DB (BDB) other than 4.8.\n" + "BDB (legacy) wallets opened by this build will not be portable!\n" + "If this is intended, pass \"-DWARN_INCOMPATIBLE_BDB=OFF\".\n" "Passing \"-DWITH_BDB=OFF\" will suppress this warning." ) endif() endif()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CMakeLists.txt` around lines 117 - 126, Update the incompatible Berkeley DB warning conditional around BerkeleyDB_VERSION so the entire warning, including its follow-up hint, is emitted only when WARN_INCOMPATIBLE_BDB is enabled. Preserve the existing version check and warning messages when the option remains enabled.cmake/ccache.cmake (1)
26-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the missing
include(TryAppendCXXFlags).This module calls
try_append_cxx_flags()but relies on the rootCMakeLists.txthaving included the defining module first. The include guard makes an explicit include free and keeps the module self-contained.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmake/ccache.cmake` around lines 26 - 33, Make cmake/ccache.cmake self-contained by explicitly including the module that defines try_append_cxx_flags before the WITH_CCACHE block. Add include(TryAppendCXXFlags) near the top, relying on its include guard and leaving the existing flag configuration unchanged.cmake/module/ProcessConfigurations.cmake (2)
92-107: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
flagis interpolated as an unanchored regex.
remove_cxx_flag_from_all_configs(-DNDEBUG)would also strip any flag containing that substring (e.g.-DNDEBUG_SOMETHING), and a caller passing a flag with regex metacharacters would misbehave. Anchoring the pattern makes the intent exact.♻️ Proposed change
- list(FILTER flags EXCLUDE REGEX "${flag}") + list(FILTER flags EXCLUDE REGEX "^${flag}$")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmake/module/ProcessConfigurations.cmake` around lines 92 - 107, Update remove_cxx_flag_from_all_configs so list(FILTER ... EXCLUDE REGEX ...) matches the entire flag rather than an unanchored substring. Anchor the interpolated flag pattern and escape any regex metacharacters in flag before filtering, preserving removal of only the exact requested compiler flag.
120-129: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueModule has an implicit dependency on
core_interface_debugexisting.
include(ProcessConfigurations)silently requires the caller to have already createdcore_interface_debug(rootCMakeLists.txtline 250). A short comment or anif(NOT TARGET ...)fatal error would make the contract explicit for future includes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmake/module/ProcessConfigurations.cmake` around lines 120 - 129, Add an explicit prerequisite check before the target_compile_definitions call for core_interface_debug: detect when the target does not exist and fail with a clear fatal error explaining that callers must create it before including ProcessConfigurations. Keep the existing configuration definitions unchanged.cmake/script/GenerateHeaderFromJson.cmake (1)
12-21: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winShared root cause: duplicated, per-byte
file(APPEND)hex-dump loop in both header generators. Both scripts issue onefile(APPEND)call per input byte, which is both a DRY violation and slow for larger fixture files at configure time.
cmake/script/GenerateHeaderFromJson.cmake#L12-L21: accumulate the formatted bytes into a single string and write it with onefile(APPEND)/file(WRITE)call instead of one call per byte; consider extracting the shared loop into a common CMake macro/module reused by both generators.cmake/script/GenerateHeaderFromRaw.cmake#L11-L20: apply the same batching fix and share the extracted macro from the JSON variant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmake/script/GenerateHeaderFromJson.cmake` around lines 12 - 21, The duplicated byte-formatting loops in GenerateHeaderFromJson.cmake and GenerateHeaderFromRaw.cmake perform one file append per byte. Extract the shared loop into a reusable CMake macro/module, have both generators accumulate formatted bytes into one string, and write the complete output with a single file(APPEND) or file(WRITE) call; update both listed sites accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmake/ccache.cmake`:
- Around line 8-14: Replace the readlink-based resolution in the ccache
masquerade check with CMake 3.22’s portable file(REAL_PATH) command. Resolve
both CMAKE_CXX_COMPILER and CCACHE_EXECUTABLE before comparing them in the
existing CCACHE_EXECUTABLE/NOT WITH_CCACHE condition, preserving the warning
behavior when either path reaches the same ccache executable through symlinks.
In `@cmake/module/FindLibevent.cmake`:
- Around line 43-80: Guard the post-discovery setup in both VCPKG and pkg-config
branches with if(Libevent_FOUND): only call check_evhttp_connection_get_peer,
create the libevent aliases, and apply the related mark_as_advanced entries when
discovery succeeded. Keep the existing package discovery and required-variable
handling unchanged.
In `@cmake/module/GenerateSetupNsi.cmake`:
- Around line 5-17: Update generate_setup_nsi to use Dash executable names for
the existing BITCOIN_*_NAME variables, replacing the bitcoin-* values with their
dash-* equivalents, and emit dash-win64-setup.nsi instead of
bitcoin-win64-setup.nsi while preserving the template variable names.
In `@CMakeLists.txt`:
- Around line 222-230: Apply APPEND_CFLAGS to the C compilation rule by
appending it to CMAKE_C_COMPILE_OBJECT, alongside the existing CXX rule updates.
Also append APPEND_CXXFLAGS to CMAKE_OBJCXX_COMPILE_OBJECT so Objective-C++
compilation receives the configured C++ flags, preserving the existing ordering
at the end of each command line.
- Around line 617-623: Update the warn_interface INTERFACE_COMPILE_OPTIONS setup
to transform each entry in warn_interface_flags into its own compile-language
generator expression, preserving each expression as a single list element. Do
not wrap the entire semicolon-separated flag list in one genex; keep the
existing CXX/OBJCXX language scope and cleanup behavior.
In `@doc/build-windows-msvc.md`:
- Line 49: Correct the typo “istructions” to “instructions” in the documentation
sentence describing the Debug and Release configurations.
- Line 27: Update every PowerShell code fence in doc/build-windows-msvc.md to
use the powershell language label, including the fences near the referenced
sections, so each opening fence follows the ```powershell format required by
MD040.
- Around line 11-18: Add a setup and validation step in the Windows build guide
before the first cmake --list-presets or build command to define VCPKG_ROOT for
the installed vcpkg directory and verify that it is set to a valid location.
Ensure the existing vs2022 and vs2022-static preset commands run only after this
variable is available.
In `@src/qt/CMakeLists.txt`:
- Around line 56-93: Move masternodemodel.cpp and masternodemodel.h from the
ENABLE_WALLET-gated source block into the unconditional bitcoinqt source list,
while keeping masternodelist.cpp wallet-gated. Ensure the CMake target includes
MasternodeModel when ENABLE_WALLET=OFF, matching the unconditional
BITCOIN_QT_BASE_CPP and BITCOIN_QT_H lists.
---
Nitpick comments:
In `@cmake/ccache.cmake`:
- Around line 26-33: Make cmake/ccache.cmake self-contained by explicitly
including the module that defines try_append_cxx_flags before the WITH_CCACHE
block. Add include(TryAppendCXXFlags) near the top, relying on its include guard
and leaving the existing flag configuration unchanged.
In `@cmake/module/ProcessConfigurations.cmake`:
- Around line 92-107: Update remove_cxx_flag_from_all_configs so list(FILTER ...
EXCLUDE REGEX ...) matches the entire flag rather than an unanchored substring.
Anchor the interpolated flag pattern and escape any regex metacharacters in flag
before filtering, preserving removal of only the exact requested compiler flag.
- Around line 120-129: Add an explicit prerequisite check before the
target_compile_definitions call for core_interface_debug: detect when the target
does not exist and fail with a clear fatal error explaining that callers must
create it before including ProcessConfigurations. Keep the existing
configuration definitions unchanged.
In `@cmake/script/GenerateHeaderFromJson.cmake`:
- Around line 12-21: The duplicated byte-formatting loops in
GenerateHeaderFromJson.cmake and GenerateHeaderFromRaw.cmake perform one file
append per byte. Extract the shared loop into a reusable CMake macro/module,
have both generators accumulate formatted bytes into one string, and write the
complete output with a single file(APPEND) or file(WRITE) call; update both
listed sites accordingly.
In `@CMakeLists.txt`:
- Around line 764-782: Add the Dash-specific configuration switches
ENABLE_MINER, ENABLE_STACKTRACES, and ENABLE_CRASH_HOOKS to the configure
summary near the existing optional-feature messages. Echo each variable’s final
value, including any value changes made by the stacktrace detection logic, so
the summary reflects the effective configuration.
- Around line 674-693: Remove the unused VAR wrap_exceptions_flags argument from
the try_append_linker_flag call while preserving RESULT_VAR
CRASH_HOOKS_WRAPPED_CXX_ABI and the existing target_link_options behavior. Also
remove the now-unnecessary unset(wrap_exceptions_flags) statement.
- Around line 382-394: Declare SANITIZERS as a typed cache string with
descriptive help text near the neighboring APPEND_* cache variables, preserving
its existing use in the sanitizer flag check and allowing it to appear in
configure summaries, ccmake, and cmake-gui.
- Around line 117-126: Update the incompatible Berkeley DB warning conditional
around BerkeleyDB_VERSION so the entire warning, including its follow-up hint,
is emitted only when WARN_INCOMPATIBLE_BDB is enabled. Preserve the existing
version check and warning messages when the option remains enabled.
In `@CMakePresets.json`:
- Around line 6-21: Add a deterministic out-of-source binaryDir to both presets,
using the preset name under the source directory (for example,
${sourceDir}/build/${presetName}). Keep the existing preset configuration
unchanged so each cmake --preset invocation writes only to its own ignored build
directory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4ac2e910-9426-48d3-be53-31596dc5cb13
📒 Files selected for processing (72)
.gitignoreCMakeLists.txtCMakePresets.jsonREADME.mdcmake/bitcoin-config.h.incmake/ccache.cmakecmake/cov_tool_wrapper.sh.incmake/crc32c.cmakecmake/introspection.cmakecmake/leveldb.cmakecmake/minisketch.cmakecmake/module/AddBoostIfNeeded.cmakecmake/module/AddWindowsResources.cmakecmake/module/CheckSourceCompilesAndLinks.cmakecmake/module/FindBerkeleyDB.cmakecmake/module/FindLibevent.cmakecmake/module/FindMiniUPnPc.cmakecmake/module/FindNATPMP.cmakecmake/module/FindQt5.cmakecmake/module/FindUSDT.cmakecmake/module/FlagsSummary.cmakecmake/module/GenerateHeaders.cmakecmake/module/GenerateSetupNsi.cmakecmake/module/GetTargetInterface.cmakecmake/module/Maintenance.cmakecmake/module/ProcessConfigurations.cmakecmake/module/TestAppendRequiredLibraries.cmakecmake/module/TryAppendCXXFlags.cmakecmake/module/TryAppendLinkerFlag.cmakecmake/module/WarnAboutGlobalProperties.cmakecmake/script/Coverage.cmakecmake/script/CoverageFuzz.cmakecmake/script/CoverageInclude.cmake.incmake/script/GenerateBuildInfo.cmakecmake/script/GenerateHeaderFromJson.cmakecmake/script/GenerateHeaderFromRaw.cmakecmake/script/macos_zip.shcmake/tests.cmakecontrib/guix/README.mddepends/Makefiledepends/funcs.mkdepends/hosts/default.mkdepends/packages/qt.mkdepends/patches/qt/qt.prodepends/toolchain.cmake.indoc/CMakeLists.txtdoc/Doxyfile.indoc/build-windows-msvc.mdsrc/CMakeLists.txtsrc/Makefile.qt.includesrc/bench/CMakeLists.txtsrc/crypto/CMakeLists.txtsrc/dashbls/CMakeLists.txtsrc/dashbls/depends/relic/CMakeLists.txtsrc/dashbls/depends/relic/include/relic_conf.h.cmake.insrc/dashbls/src/CMakeLists.txtsrc/ipc/CMakeLists.txtsrc/qt/CMakeLists.txtsrc/qt/test/CMakeLists.txtsrc/test/CMakeLists.txtsrc/test/fuzz/CMakeLists.txtsrc/test/fuzz/util/CMakeLists.txtsrc/test/util/CMakeLists.txtsrc/univalue/CMakeLists.txtsrc/util/CMakeLists.txtsrc/wallet/CMakeLists.txtsrc/wallet/test/CMakeLists.txtsrc/wallet/test/fuzz/CMakeLists.txtsrc/zmq/CMakeLists.txttest/CMakeLists.txttest/functional/test_runner.pyvcpkg.json
💤 Files with no reviewable changes (1)
- depends/patches/qt/qt.pro
| This guide relies on using CMake and vcpkg package manager provided with the Visual Studio installation. | ||
| Here are requirements for the Visual Studio installation: | ||
| 1. Minimum required version: Visual Studio 2022 version 17.6. | ||
| 2. Installed components: | ||
| - The "Desktop development with C++" workload. | ||
|
|
||
| The commands in this guide should be executed in "Developer PowerShell for VS 2022" or "Developer Command Prompt for VS 2022". | ||
| The former is assumed hereinafter. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
test -n "${VCPKG_ROOT:-}" || {
echo "VCPKG_ROOT is unset"
exit 1
}
test -f "$VCPKG_ROOT/scripts/buildsystems/vcpkg.cmake"Repository: dashpay/dash
Length of output: 170
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== locate build-windows-msvc.md =="
fd -a 'build-windows-msvc\.md$' . || true
echo "== inspect relevant doc section =="
cat -n doc/build-windows-msvc.md | sed -n '1,80p'
echo "== search cmake presets/toolchain references =="
rg -n '"vs2022"|cmake-tools\.json|vcpkg\.cmake|\$env\{VCPKG_ROOT\}|\$env:VCPKG_ROOT|VCPKG_ROOT' . -g '*.json' -g '*.md' -g '*.cmake' -g '*.cmake.in' || true
echo "== locate cmake preset/toolchain files =="
git ls-files | rg '(^|/)(CMakePresets|cmake-tools|vcpkg|presets|\.cmake)$' | sortRepository: dashpay/dash
Length of output: 4303
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== CMakePresets.json =="
cat -n CMakePresets.json | sed -n '1,80p'
echo "== vscode vcpkg settings references =="
rg -n '"CMAKE_TOOLCHAIN_FILE"|"CMAKE_TOOLCHAIN_FILE":|VCPKG_ROOT|vcpkg\.cmake' . -g '.*settings.json$' -g 'CMakePresets.json' -g 'cmake-tools.json' || true
echo "== files mentioning VCPKG_ROOT in repo =="
rg -n 'VCPKG_ROOT|vcpkg root|VcpkgRoot|/include/vcpkg|scripts/buildsystems/vcpkg.cmake' . -g '*.md' -g '*.json' -g '*.cmake' -g '*.ini' -g '*.txt' -g '*.ps1' -g '*.bat' -g '*.cmd' || trueRepository: dashpay/dash
Length of output: 2232
Add a VCPKG_ROOT setup step before using the presets.
CMakePresets.json uses $env{VCPKG_ROOT}\scripts\buildsystems\vcpkg.cmake for both vs2022 and vs2022-static, but this guide never sets or checks that variable. Add setup/validation before the first cmake --list-presets / build command so the presets do not fail to resolve the toolchain.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@doc/build-windows-msvc.md` around lines 11 - 18, Add a setup and validation
step in the Windows build guide before the first cmake --list-presets or build
command to define VCPKG_ROOT for the installed vcpkg directory and verify that
it is set to a valid location. Ensure the existing vs2022 and vs2022-static
preset commands run only after this variable is available.
bitcoin_common compiles common/url.cpp, which includes <event2/http.h>, but did not link libevent. That only builds where the libevent headers happen to sit in a default include path; it fails with Homebrew and with depends. test_bitcoin-qt did not link LevelDB, so dbwrapper.h (reachable from the Dash-specific headers those tests pull in) failed to compile, and qt/test/trafficgraphdatatests.cpp was missing from the target even though test_main.cpp instantiates TrafficGraphDataTests. Name the binary test_dash-qt, like every other Dash target. The dashbls and relic headers are pulled in with -isystem under Autotools but not under CMake, which produced ~13k -Wdocumentation warnings and would break any WERROR build. Mark them as system includes for consumers. Also fix a copy-paste typo in try_append_linker_flag(), where the IF_CHECK_FAILED path referenced a TACXXF_ prefixed variable that is never set. Co-Authored-By: Claude Opus 5 <[email protected]>
The generated bitcoin-config.h was missing a set of macros that Dash sources check. Each of these built and linked cleanly while quietly changing behaviour: ENABLE_MINER: every mining RPC fell through to the "This call is not available because RPC miner isn't compiled" stub, which breaks essentially every functional test. HAVE_SYS_GETRANDOM: CMake probed for the libc getrandom() wrapper and defined HAVE_GETRANDOM, which random.cpp does not read; it checks HAVE_SYS_GETRANDOM and issues the syscall directly. ENABLE_SSSE3, ENABLE_X86_AESNI, ENABLE_ARM_AES and ENABLE_ARM_NEON were missing along with the crypto/x11 SIMD sources they guard, so X11 hashing silently fell back to the generic implementations everywhere. Add the introspection checks and the per-backend libraries, mirroring the sha256 ones. ENABLE_STACKTRACES and ENABLE_CRASH_HOOKS had no option at all. Add them with libbacktrace detection mirroring configure.ac, including -Wl,-export-dynamic/-rdynamic, -gdwarf-4, -fno-standalone-debug and the -Wl,-wrap exception hooks behind CRASH_HOOKS_WRAPPED_CXX_ABI. ENABLE_ZMQ, USE_UPNP and USE_NATPMP are deliberately not added here: unlike Autotools, which defines them in config.h, CMake attaches them as INTERFACE definitions on the bitcoin_zmq, MiniUPnPc::MiniUPnPc and NATPMP::NATPMP targets, and every consumer links those. Also define GSL_NO_IOSTREAMS and the NOWARN_CXXFLAGS suppressions, without which a GCC WERROR build fails on immer, and drop BUILD_KERNEL_LIB: src/kernel/CMakeLists.txt referenced eleven files that do not exist in Dash and was gated behind an EXISTS check so it could never build. Co-Authored-By: Claude Opus 5 <[email protected]>
Upstream's GenerateBuildInfo.cmake emits BUILD_GIT_TAG/BUILD_GIT_COMMIT, but Dash's clientversion.cpp only understands BUILD_GIT_DESCRIPTION, so every non-release CMake build reported itself as vX.Y.Z-unk. Emit the same git describe --abbrev=12 --dirty string that share/genbuild.sh produces. Co-Authored-By: Claude Opus 5 <[email protected]>
test/coinjoin_dstxmanager_tests.cpp, test/coinjoin_inouts_tests.cpp and test/coinjoin_queue_tests.cpp were not in the unit-test target, so three CoinJoin suites were silently not run. PROVIDE_FUZZ_MAIN_FUNCTION was never defined even though the probe for it (FUZZ_BINARY_LINKS_WITHOUT_MAIN_FUNCTION) was performed, so a fuzz binary built without a fuzzing engine that supplies main() fails to link. config.ini.in expects ENABLE_FUZZ_TRUE, but create_test_config() substituted ENABLE_FUZZ_BINARY_TRUE. The unmatched placeholder expanded to nothing, leaving ENABLE_FUZZ=true set unconditionally for test/fuzz/test_runner.py. Co-Authored-By: Claude Opus 5 <[email protected]>
ci/dash/build_src.sh passes BITCOIN_CONFIG straight to ./configure, so the s390x job's -DREDUCE_EXPORTS=ON would fail as an unrecognised option; wrap-wine.sh had dropped the secp256k1 and minisketch test binaries that the Autotools build still produces; and test_runner.py had been switched to read the functional tests out of the build directory, which only the CMake build populates. Revert all three until CMake is the only build system. Co-Authored-By: Claude Opus 5 <[email protected]>
The guide told readers to clone bitcoin/bitcoin and referred to the Bitcoin Core project throughout, and src/qt/README.md had been repointed from our cross-compile guide to the MSVC one. Co-Authored-By: Claude Opus 5 <[email protected]>
configure.ac adds -Wno-error=deprecated-literal-operator for Clang 20 (hana triggers it) whenever --enable-werror is used; the CMake warning block was missing it. Also spell out why config.ini's ENABLE_FUZZ is driven by BUILD_FUZZ_BINARY rather than BUILD_FOR_FUZZING. Co-Authored-By: Claude Opus 5 <[email protected]>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The default native CMake path may build, but five confirmed blockers leave supported optional and deployment paths unusable: the MinGW installer is malformed, multiprocess configuration and installed binaries are broken, and CMake drops Dash's public consensus library. Two commit-history cleanups are also warranted for durable backport provenance and logical WERROR commit grouping.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 5 blocking | 🟡 2 suggestion(s)
2 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `cmake/module/GenerateSetupNsi.cmake`:
- [BLOCKING] cmake/module/GenerateSetupNsi.cmake:9-15: NSIS script references unstaged Bitcoin executable names
The deploy command stages each target using `$<TARGET_FILE_NAME:...>`, and the targets have Dash output names such as `dash-qt.exe`, `dashd.exe`, and `dash-cli.exe`. This generator instead substitutes `bitcoin-qt.exe`, `bitcoind.exe`, `bitcoin-cli.exe`, and the other upstream names into `share/setup.nsi.in`. NSIS therefore cannot find the staged files and cannot build the installer. `PACKAGE_TARNAME` also registers the upstream `bitcoin` URI class instead of preserving Dash's Autotools package name, `dashcore`.
In `cmake/module/Maintenance.cmake`:
- [BLOCKING] cmake/module/Maintenance.cmake:75-84: Windows deploy script never specifies an installer output file
`share/setup.nsi.in` contains no `OutFile` directive, and this command invokes `makensis` without injecting one. The existing Autotools recipe explicitly appends `OutFile "$@"` before invoking NSIS. Without an equivalent directive, NSIS has no output path and cannot produce the custom command's declared `bitcoin-win64-setup.exe`, so the `deploy` target cannot complete.
In `src/test/CMakeLists.txt`:
- [BLOCKING] src/test/CMakeLists.txt:191-210: Missing prerequisite: bitcoin#28921
The imported `WITH_MULTIPROCESS` block references `ipc_test.cpp`, `ipc_test.capnp`, and `ipc_tests.cpp`, but none exists in the PR head. Upstream bitcoin#30454 added this CMake block after bitcoin#28921 had already introduced those files, so copying the block without its prerequisite makes `-DWITH_MULTIPROCESS=ON` fail during CMake generation with the default `BUILD_TESTS=ON`. Backport and adapt the IPC test sources, or omit this test block until that prerequisite is present.
In `src/CMakeLists.txt`:
- [BLOCKING] src/CMakeLists.txt:400-410: Multiprocess node is installed under a name its IPC code cannot spawn
Unlike the other Dash executables, `bitcoin-node` has no `OUTPUT_NAME`, so CMake installs it as `bitcoin-node`. The IPC implementation constructs a sibling path using the literal name passed to `spawnProcess`, and `rpc/node.cpp` passes `dash-node`; an installed CMake build therefore tries to launch a file that does not exist. The corresponding `bitcoin-gui` target in `src/qt/CMakeLists.txt` also lacks the `dash-gui` output name required by `init/bitcoin-gui.cpp` and the documented Autotools behavior.
- [BLOCKING] src/CMakeLists.txt:93-119: Missing prerequisite: bitcoin#29648
bitcoin#30454 was developed after bitcoin#29648 removed Bitcoin Core's public consensus library, but Dash still provides and documents that API. The imported CMake code creates only an internal `STATIC EXCLUDE_FROM_ALL` archive and provides no shared `libdashconsensus`, installed `script/bitcoinconsensus.h`, generated and installed `libdashconsensus.pc`, or `HAVE_CONSENSUS_LIB` definition. This is not equivalent to Dash's default Autotools build and silently skips the public API checks guarded by `HAVE_CONSENSUS_LIB` in `script_tests.cpp`. Preserve Dash's public shared-library target, export definitions, header and pkg-config installation, and API tests in the CMake build.
In `<commit:86f540b>`:
- [SUGGESTION] <commit:86f540b>:1: Record the upstream provenance of the CMake import
The initial commit imports and adapts the CMake system from Bitcoin Core but its permanent commit message contains no upstream PR, source commit, or cherry-pick provenance. Record bitcoin/bitcoin#30454 and the exact merge or source hash used so future maintainers can compare Dash's adaptation with its upstream origin directly from `git log`.
In `<commit:c5915df>`:
- [SUGGESTION] <commit:c5915df>:1: Combine the adjacent WERROR parity commits
Commits `c5915df` and `e732253` are adjacent parts of the same logical change: making CMake's warning, WERROR, and optimization behavior match Autotools. The first commit also contains an unrelated explanatory change in `test/CMakeLists.txt`. Squash the WERROR changes into one commit and move the test-config comment into `fdfafe8`, where that wiring was fixed.
| set(PACKAGE_TARNAME "bitcoin") | ||
| set(BITCOIN_GUI_NAME "bitcoin-qt") | ||
| set(BITCOIN_DAEMON_NAME "bitcoind") | ||
| set(BITCOIN_CLI_NAME "bitcoin-cli") | ||
| set(BITCOIN_TX_NAME "bitcoin-tx") | ||
| set(BITCOIN_WALLET_TOOL_NAME "bitcoin-wallet") | ||
| set(BITCOIN_TEST_NAME "test_bitcoin") |
There was a problem hiding this comment.
🔴 Blocking: NSIS script references unstaged Bitcoin executable names
The deploy command stages each target using $<TARGET_FILE_NAME:...>, and the targets have Dash output names such as dash-qt.exe, dashd.exe, and dash-cli.exe. This generator instead substitutes bitcoin-qt.exe, bitcoind.exe, bitcoin-cli.exe, and the other upstream names into share/setup.nsi.in. NSIS therefore cannot find the staged files and cannot build the installer. PACKAGE_TARNAME also registers the upstream bitcoin URI class instead of preserving Dash's Autotools package name, dashcore.
| set(PACKAGE_TARNAME "bitcoin") | |
| set(BITCOIN_GUI_NAME "bitcoin-qt") | |
| set(BITCOIN_DAEMON_NAME "bitcoind") | |
| set(BITCOIN_CLI_NAME "bitcoin-cli") | |
| set(BITCOIN_TX_NAME "bitcoin-tx") | |
| set(BITCOIN_WALLET_TOOL_NAME "bitcoin-wallet") | |
| set(BITCOIN_TEST_NAME "test_bitcoin") | |
| set(PACKAGE_TARNAME "dashcore") | |
| set(BITCOIN_GUI_NAME "dash-qt") | |
| set(BITCOIN_DAEMON_NAME "dashd") | |
| set(BITCOIN_CLI_NAME "dash-cli") | |
| set(BITCOIN_TX_NAME "dash-tx") | |
| set(BITCOIN_WALLET_TOOL_NAME "dash-wallet") | |
| set(BITCOIN_TEST_NAME "test_dash") |
source: ['codex']
| OUTPUT ${PROJECT_BINARY_DIR}/bitcoin-win64-setup.exe | ||
| COMMAND ${CMAKE_COMMAND} -E make_directory ${PROJECT_BINARY_DIR}/release | ||
| COMMAND ${CMAKE_STRIP} $<TARGET_FILE:bitcoin-qt> -o ${PROJECT_BINARY_DIR}/release/$<TARGET_FILE_NAME:bitcoin-qt> | ||
| COMMAND ${CMAKE_STRIP} $<TARGET_FILE:bitcoind> -o ${PROJECT_BINARY_DIR}/release/$<TARGET_FILE_NAME:bitcoind> | ||
| COMMAND ${CMAKE_STRIP} $<TARGET_FILE:bitcoin-cli> -o ${PROJECT_BINARY_DIR}/release/$<TARGET_FILE_NAME:bitcoin-cli> | ||
| COMMAND ${CMAKE_STRIP} $<TARGET_FILE:bitcoin-tx> -o ${PROJECT_BINARY_DIR}/release/$<TARGET_FILE_NAME:bitcoin-tx> | ||
| COMMAND ${CMAKE_STRIP} $<TARGET_FILE:bitcoin-wallet> -o ${PROJECT_BINARY_DIR}/release/$<TARGET_FILE_NAME:bitcoin-wallet> | ||
| COMMAND ${CMAKE_STRIP} $<TARGET_FILE:bitcoin-util> -o ${PROJECT_BINARY_DIR}/release/$<TARGET_FILE_NAME:bitcoin-util> | ||
| COMMAND ${CMAKE_STRIP} $<TARGET_FILE:test_bitcoin> -o ${PROJECT_BINARY_DIR}/release/$<TARGET_FILE_NAME:test_bitcoin> | ||
| COMMAND makensis -V2 ${PROJECT_BINARY_DIR}/bitcoin-win64-setup.nsi |
There was a problem hiding this comment.
🔴 Blocking: Windows deploy script never specifies an installer output file
share/setup.nsi.in contains no OutFile directive, and this command invokes makensis without injecting one. The existing Autotools recipe explicitly appends OutFile "$@" before invoking NSIS. Without an equivalent directive, NSIS has no output path and cannot produce the custom command's declared bitcoin-win64-setup.exe, so the deploy target cannot complete.
source: ['codex']
| if(WITH_MULTIPROCESS) | ||
| add_library(bitcoin_ipc_test STATIC EXCLUDE_FROM_ALL | ||
| ipc_test.cpp | ||
| ) | ||
|
|
||
| target_capnp_sources(bitcoin_ipc_test ${PROJECT_SOURCE_DIR} | ||
| ipc_test.capnp | ||
| ) | ||
|
|
||
| target_link_libraries(bitcoin_ipc_test | ||
| PRIVATE | ||
| core_interface | ||
| univalue | ||
| ) | ||
|
|
||
| target_sources(test_bitcoin | ||
| PRIVATE | ||
| ipc_tests.cpp | ||
| ) | ||
| target_link_libraries(test_bitcoin bitcoin_ipc_test) |
There was a problem hiding this comment.
🔴 Blocking: Missing prerequisite: bitcoin#28921
The imported WITH_MULTIPROCESS block references ipc_test.cpp, ipc_test.capnp, and ipc_tests.cpp, but none exists in the PR head. Upstream bitcoin#30454 added this CMake block after bitcoin#28921 had already introduced those files, so copying the block without its prerequisite makes -DWITH_MULTIPROCESS=ON fail during CMake generation with the default BUILD_TESTS=ON. Backport and adapt the IPC test sources, or omit this test block until that prerequisite is present.
source: ['codex']
| add_executable(bitcoin-node | ||
| bitcoind.cpp | ||
| init/bitcoin-node.cpp | ||
| ) | ||
| target_link_libraries(bitcoin-node | ||
| core_interface | ||
| bitcoin_node | ||
| bitcoin_ipc | ||
| $<TARGET_NAME_IF_EXISTS:bitcoin_wallet> | ||
| ) | ||
| list(APPEND installable_targets bitcoin-node) |
There was a problem hiding this comment.
🔴 Blocking: Multiprocess node is installed under a name its IPC code cannot spawn
Unlike the other Dash executables, bitcoin-node has no OUTPUT_NAME, so CMake installs it as bitcoin-node. The IPC implementation constructs a sibling path using the literal name passed to spawnProcess, and rpc/node.cpp passes dash-node; an installed CMake build therefore tries to launch a file that does not exist. The corresponding bitcoin-gui target in src/qt/CMakeLists.txt also lacks the dash-gui output name required by init/bitcoin-gui.cpp and the documented Autotools behavior.
source: ['codex']
| # Stable, backwards-compatible consensus functionality. | ||
| add_library(bitcoin_consensus STATIC EXCLUDE_FROM_ALL | ||
| arith_uint256.cpp | ||
| bls/bls.cpp | ||
| consensus/merkle.cpp | ||
| consensus/tx_check.cpp | ||
| hash.cpp | ||
| primitives/block.cpp | ||
| primitives/transaction.cpp | ||
| pubkey.cpp | ||
| script/bitcoinconsensus.cpp | ||
| script/interpreter.cpp | ||
| script/script.cpp | ||
| script/script_error.cpp | ||
| uint256.cpp | ||
| util/strencodings.cpp | ||
| util/string.cpp | ||
| ) | ||
| set_target_properties(bitcoin_consensus PROPERTIES OUTPUT_NAME dashconsensus) | ||
| target_link_libraries(bitcoin_consensus | ||
| PRIVATE | ||
| core_interface | ||
| bitcoin_crypto | ||
| Boost::headers | ||
| dashbls | ||
| secp256k1 | ||
| ) |
There was a problem hiding this comment.
🔴 Blocking: Missing prerequisite: bitcoin#29648
bitcoin#30454 was developed after bitcoin#29648 removed Bitcoin Core's public consensus library, but Dash still provides and documents that API. The imported CMake code creates only an internal STATIC EXCLUDE_FROM_ALL archive and provides no shared libdashconsensus, installed script/bitcoinconsensus.h, generated and installed libdashconsensus.pc, or HAVE_CONSENSUS_LIB definition. This is not equivalent to Dash's default Autotools build and silently skips the public API checks guarded by HAVE_CONSENSUS_LIB in script_tests.cpp. Preserve Dash's public shared-library target, export definitions, header and pkg-config installation, and API tests in the CMake build.
source: ['codex']
e732253 to
1feb706
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1feb706873
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| bitcoind.cpp | ||
| init/bitcoind.cpp | ||
| ) | ||
| add_windows_resources(bitcoind bitcoind-res.rc) |
There was a problem hiding this comment.
When WIN32 is true, add_windows_resources passes this path directly to target_sources, but the repository contains src/dashd-res.rc, not src/bitcoind-res.rc, so CMake cannot generate a default Windows build. The wallet, CLI, and transaction targets have the same mismatch (dash-wallet-res.rc, dash-cli-res.rc, and dash-tx-res.rc are present instead of the referenced Bitcoin-named files), so adapt all four resource arguments to the existing Dash files.
Useful? React with 👍 / 👎.
| set_target_properties(bitcoin-chainstate PROPERTIES OUTPUT_NAME dash-chainstate) | ||
| endif() |
There was a problem hiding this comment.
When configured with BUILD_UTIL_CHAINSTATE=ON, this target is created but never appended to installable_targets, so cmake --install silently omits dash-chainstate. The equivalent Autotools option adds it to bin_PROGRAMS; append this target as the other command-line executables do.
Useful? React with 👍 / 👎.
| Developers are strongly encouraged to write [unit tests](src/test/README.md) for new code, and to | ||
| submit new unit tests for old code. Unit tests can be compiled and run | ||
| (assuming they weren't disabled in configure) with: `make check`. Further details on running | ||
| (assuming they weren't disabled during the generation of the build system) with: `ctest`. Further details on running |
There was a problem hiding this comment.
CMake explicitly forbids in-source builds, so running the documented ctest command from the repository root finds no tests; Autotools builds still require make check as well. Document both build-system commands, or use ctest --test-dir <build-dir>, so following the README actually executes the unit tests.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build-src-cmake.yml:
- Line 42: Update the workflow’s container-image input handling so the image
producer resolves inputs.container-path to an immutable `@sha256` digest before
invoking reusable jobs, including the job using the container image. Reject or
fail when the resolved reference is not digest-pinned, and pass the validated
digest reference rather than forwarding needs.container.outputs.path unchanged.
- Line 68: Stop re-parsing CI inputs as shell source: in
.github/workflows/build-src-cmake.yml at lines 68, 86, 106, and 113, pass
depends-dep-opts through env and invoke make with a validated argument array,
while reading build-target from the environment rather than interpolating it
into shell text. In ci/dash/build_src_cmake.sh lines 36-42, remove bash -c and
invoke CMake and cmake --build using quoted paths and argument arrays.
- Around line 45-49: Update the workflow trigger and checkout configuration
around the checkout step and build execution so untrusted pull-request head code
is not executed in the privileged workflow. Prefer running this build on
pull_request rather than pull_request_target; if checkout remains necessary
under pull_request_target, isolate it as read-only and set persist-credentials
to false before running ci/dash/build_src_cmake.sh and tests.
In `@doc/build-windows-msvc.md`:
- Around line 24-29: Update the “Clone Dash Repository” instructions after the
git clone command to explicitly change into the cloned dash directory before any
subsequent build or CMake commands are run.
- Line 82: Update the build-performance guidance in doc/build-windows-msvc.md to
remove the recommendation for excluding the entire Dash repository from
Microsoft Defender. If exclusion advice is retained, scope it to a temporary
build directory only, make the security trade-off explicit, and require
compliance with local security policy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e5c9da9c-14c7-46fa-83ce-00af43e7ed6c
📒 Files selected for processing (21)
.github/workflows/build-src-cmake.yml.github/workflows/build.ymlCMakeLists.txtci/dash/build_src_cmake.shci/dash/matrix.shci/dash/test_integrationtests_cmake.shci/dash/test_unittests_cmake.shci/test/00_setup_env_native_cmake.shcmake/bitcoin-config.h.incmake/introspection.cmakecmake/module/TryAppendLinkerFlag.cmakecmake/script/GenerateBuildInfo.cmakedoc/build-windows-msvc.mdsrc/CMakeLists.txtsrc/crypto/CMakeLists.txtsrc/qt/test/CMakeLists.txtsrc/test/CMakeLists.txtsrc/test/fuzz/CMakeLists.txtsrc/zmq/CMakeLists.txttest/CMakeLists.txttest/functional/test_runner.py
🚧 Files skipped from review as they are similar to previous changes (12)
- src/zmq/CMakeLists.txt
- test/functional/test_runner.py
- src/qt/test/CMakeLists.txt
- src/test/fuzz/CMakeLists.txt
- test/CMakeLists.txt
- src/crypto/CMakeLists.txt
- cmake/script/GenerateBuildInfo.cmake
- cmake/module/TryAppendLinkerFlag.cmake
- cmake/introspection.cmake
- src/test/CMakeLists.txt
- CMakeLists.txt
- src/CMakeLists.txt
| - name: Checkout code | ||
| uses: actions/checkout@v6 | ||
| with: | ||
| ref: ${{ github.event.pull_request.head.sha }} | ||
| fetch-depth: 50 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow files =="
git ls-files .github/workflows || true
echo "== target workflow excerpt =="
if [ -f .github/workflows/build-src-cmake.yml ]; then
cat -n .github/workflows/build-src-cmake.yml
fi
echo "== all pull_request_target occurrences =="
rg -n "pull_request_target|pull_request\b|permissions:|GITHUB_TOKEN|checkout|permissions =" .github || true
echo "== changed workflow status =="
git diff -- .github/workflows/build-src-cmake.yml || trueRepository: dashpay/dash
Length of output: 11666
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== build.yml relevant excerpt =="
cat -n .github/workflows/build.yml | sed -n '1,120p'
echo "== build-src-cmake workflow_call references =="
rg -n "build-src-cmake|build_src_cmake|matrix\.sh|workflow_call|event_name|head\.sha|permissions:" .github/workflows/build.yml .github/workflows/build-src-cmake.ymlRepository: dashpay/dash
Length of output: 7034
🌐 Web query:
actions/checkout persist-credentials default true .git/config local credential helper
💡 Result:
The persist-credentials input for the actions/checkout GitHub Action determines whether the authentication token or SSH key used to fetch the repository is configured within the local Git configuration of the checked-out repository [1][2][3]. When set to its default value of true, the action enables your workflow scripts to run authenticated Git commands (such as git push or git pull) without needing to manually configure additional credentials [1][2][4]. The token or key is removed during the action's post-job cleanup [1][2][3]. Important changes regarding how this data is persisted have been introduced in recent versions: 1. Evolution of Storage Method: Historically, actions/checkout directly injected credentials into the repository's .git/config file [1][2][5]. Starting with version v6.0.0, the action was updated to store these credentials in a separate file located within the $RUNNER_TEMP directory [5][6]. It then uses Git's includeIf mechanism to conditionally include these credentials only when accessing the relevant Git directory, improving security and isolation [7][5]. 2. Security Context: Because the default is true, subsequent steps in your workflow have access to these credentials [8]. To minimize risk—especially if your workflow includes untrusted code—you can opt out of this behavior by setting persist-credentials: false in your workflow definition [1][2][4][8]. There have been community discussions and proposals to change this default to false to reduce the risk of accidental token exposure, though as of July 2026, the default remains true [9][8]. If you use persist-credentials: true, ensure your workflow adheres to the principle of least privilege by configuring appropriate repository permissions for the GITHUB_TOKEN [10][8].
Citations:
- 1: https://github.com/actions/checkout
- 2: https://github.com/actions/checkout/tree/v6.0.2?tab=readme-ov-file
- 3: https://github.com/actions/checkout/blob/34e114876b0b11c390a56381ad16ebd13914f8d5/action.yml
- 4: https://github.com/actions/checkout/blob/0299a0d2b67d48224ce047d03c69693b37fe77fe/README.md
- 5: actions/checkout@v5.0.1...v6.0.0
- 6: https://github.com/actions/checkout/tree/v6.0.0
- 7: https://github.com/actions/checkout/pull/2286/files
- 8: Remove
persist-credentialsor change the default tofalseactions/checkout#485 - 9: Change the default value of persist-credentials to false actions/checkout#1687
- 10: https://raw.githubusercontent.com/actions/checkout/v6/action.yml
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== build.yml build-src-cmatrix inputs =="
cat -n .github/workflows/build.yml | sed -n '210,245p'
echo "== build-container uses workflow_call and checkout refs =="
cat -n .github/workflows/build-container.yml | sed -n '1,80p'
echo "== build-depends workflow_call and checkout refs =="
cat -n .github/workflows/build-depends.yml | sed -n '1,130p'
echo "== cache-depends-sources workflow_call and checkout refs =="
cat -n .github/workflows/cache-depends-sources.yml | sed -n '1,80p'Repository: dashpay/dash
Length of output: 12296
🌐 Web query:
GitHub Pull_request_target workflows run with contents permission default read pull_request head commit security
💡 Result:
The pull_request_target event is a powerful trigger in GitHub Actions that runs workflows in the context of the base repository's default branch, granting them elevated trust, including access to organization/repository secrets and a write-capable GITHUB_TOKEN [1][2][3]. This trigger was designed to allow automation (like labeling or triage) on pull requests from forks, where standard pull_request events would be restricted [1][4]. Security Risks and Mechanics The primary security risk, often called a "pwn request," occurs when a workflow uses pull_request_target and then checks out the untrusted head commit of a pull request from a fork [5][2]. If that workflow subsequently executes any code from that checkout (e.g., running tests, build scripts, or Makefiles), an attacker can execute arbitrary commands with the base repository's secrets and elevated permissions [2][6]. Mitigation and Recent Changes 1. Automatic Protection: As of June 2026, actions/checkout v7 automatically refuses to fetch fork pull request code in pull_request_target workflows to prevent these vulnerabilities [5]. 2. Opt-Out: If a workflow genuinely requires checking out fork code with elevated trust, authors must explicitly add allow-unsafe-pr-checkout: true to the actions/checkout step [1][5]. This is an intentional hurdle meant to highlight the security risk during code review [5]. 3. Permissions: Setting permissions: contents: read at the job level is a recommended best practice to apply the principle of least privilege [7][8]. However, security researchers warn that this is a partial mitigation [9]. Permissions flags restrict the GITHUB_TOKEN but do not isolate the runner environment [9]. Features such as actions/cache and in-memory OIDC tokens can potentially be exploited by malicious code running on the same runner, regardless of the job's defined permissions [9]. 4. Context Execution: Because pull_request_target workflows run from the default branch, the workflow file itself is trusted [1][6]. The vulnerability only arises if the workflow fetches and executes external, unreviewed code [6]. Recommendation To secure these workflows: - Avoid executing code from a pull request head commit [2]. - If you only need to inspect changes (e.g., labeling), use the GitHub API to read PR data rather than checking out the source code [2]. - If you must use pull_request_target, explicitly define least-privilege permissions [1][7]. - Treat the entire runner environment as a trust boundary, not just the permissions on the GITHUB_TOKEN [9].
Citations:
- 1: https://docs.github.com/en/actions/reference/security/securely-using-pull_request_target
- 2: https://www.systemshardening.com/articles/cicd/github-actions-pull-request-target-injection/
- 3: https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows
- 4: https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/
- 5: https://github.blog/changelog/2026-06-18-safer-pull_request_target-defaults-for-github-actions-checkout/
- 6: https://docs.github.com/en/enterprise-cloud@latest/actions/reference/security/securely-using-pull_request_target
- 7: https://github.blog/changelog/2025-11-07-actions-pull_request_target-and-environment-branch-protections-changes/
- 8: https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax
- 9: https://saschb2b.com/blog/pull-request-target-trap
Do not execute the PR head from this build workflow.
When triggered by pull_request_target, this workflow will checkout github.event.pull_request.head.sha and then run ./ci/dash/build_src_cmake.sh and tests from that ref. Keep PR builds on pull_request, or otherwise isolate execution as read-only and set persist-credentials: false if checkout is needed.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 45-49: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/build-src-cmake.yml around lines 45 - 49, Update the
workflow trigger and checkout configuration around the checkout step and build
execution so untrusted pull-request head code is not executed in the privileged
workflow. Prefer running this build on pull_request rather than
pull_request_target; if checkout remains necessary under pull_request_target,
isolate it as read-only and set persist-credentials to false before running
ci/dash/build_src_cmake.sh and tests.
Source: Linters/SAST tools
There was a problem hiding this comment.
The current checkout-v6.1 default prevents the fork-head checkout before PR code executes, so this is not a presently reachable pwn-request path. It is still a blocker because fork coverage is broken; match Dash commit ba143fe163a by adding both allow-unsafe-pr-checkout: true and persist-credentials: false.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Current CMake workflow checkout step =="
sed -n '40,58p' .github/workflows/build-src-cmake.yml
echo "== Referenced precedent commit, relevant workflow changes =="
git show --format=fuller --no-ext-diff --unified=12 ba143fe163a -- .github/workflows 2>/dev/null || \
echo "Referenced commit is unavailable in this checkout."Length of output: 1109
@thepastaclaw, agreed. The current workflow lacks both settings, and the original finding overstated the currently reachable pwn-request path: checkout blocks fork-head checkout before PR code runs. The real blocker is restoring fork coverage intentionally while ensuring checkout credentials are not exposed to the executed PR code.
- Add the explicit unsafe fork-PR checkout opt-in required by
actions/checkoutv6.1. - Disable persisted checkout credentials as defense in depth for the subsequently executed PR-controlled build and test commands.
- Keep the workflow trigger and existing ref unchanged.
✏️ Learnings added
Learnt from: thepastaclaw
Repo: dashpay/dash PR: 7481
File: .github/workflows/build-src-cmake.yml:45-49
Timestamp: 2026-07-25T22:39:07.618Z
Learning: For `.github/workflows/build-src-cmake.yml`, Dash intentionally executes fork pull-request head code under its `pull_request_target`-based CI flow to retain fork coverage. With `actions/checkout` v6.1 or later, the checkout step must explicitly set `allow-unsafe-pr-checkout: true`; it must also set `persist-credentials: false` so the subsequent PR-controlled build and test commands do not receive persisted checkout credentials.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
- 📌 Commit the changes to this branch
|
|
||
| ### 7. Antivirus Software | ||
|
|
||
| To improve the build process performance, one might add the Dash repository directory to the Microsoft Defender Antivirus exclusions. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Avoid recommending a repository-wide antivirus exclusion.
Excluding the entire repository from Microsoft Defender can allow malicious source files, dependencies, or build artifacts to evade scanning. If retained, make the trade-off explicit and limit the advice to a narrowly scoped, temporary build-directory exclusion subject to local security policy.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@doc/build-windows-msvc.md` at line 82, Update the build-performance guidance
in doc/build-windows-msvc.md to remove the recommendation for excluding the
entire Dash repository from Microsoft Defender. If exclusion advice is retained,
scope it to a temporary build directory only, make the security trade-off
explicit, and require compliance with local security policy.
Dash CI always configures with --enable-werror, but -DWERROR=ON never built. Three reasons, all divergences from what Autotools does: -Wundef is not part of Dash's Autotools warning set, and crypto/x11/sph_types.h tests SPH_* macros that are deliberately left undefined. Keep the diagnostic but downgrade it to -Wno-error=undef; it is exactly the check that catches the '#if ENABLE_MINER' class of bug. AM_CXXFLAGS carries the warning and -Werror flags but AM_CFLAGS does not, so the vendored sphlib C sources under crypto/x11 are never held to them. CMake applies target options to every language, so groestl.c failed on -Wunused-but-set-variable. Restrict warn_interface to C++/Objective-C++. Autotools also builds the sphlib sources as their own library with -O3 (SPHLIB_FLAGS), because they sit on the X11 proof-of-work path. Turn bitcoin_crypto_sph from an INTERFACE target that only carried two defines into a real static library that mirrors that. Co-Authored-By: Claude Opus 5 <[email protected]>
Without a job that exercises it, the CMake build bitrots. This adds one native Linux target that reuses the depends prefix built for the linux64 Autotools job: the same HOST and DEP_OPTS mean the depends cache is shared, and the 'make -C depends' step that restores the prefix is also what writes depends/$HOST/toolchain.cmake, which the build then consumes. The job configures with that toolchain file, so the feature set (GUI, wallet with BDB and SQLite, ZMQ) comes from what depends actually built, and WERROR is on as it is for every other job. It then runs the full unit-test suite through ctest and a three-test functional smoke list. The smoke tests are not redundant with the Autotools functional jobs: mining_basic.py is what catches a missing ENABLE_MINER, which no unit test would notice. Artifact bundling and the full functional suite deliberately stay with the Autotools jobs. Co-Authored-By: Claude Opus 5 <[email protected]>
1feb706 to
47e47e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build-src-cmake.yml:
- Around line 32-36: Update the workflow header documentation to accurately
reflect that the CMake job runs the integration smoke test via
test_integrationtests_cmake.sh, while only the full functional test suite
remains in the Autotools jobs; preserve the existing workflow steps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ae657ff6-af94-47fd-955d-430c509ae5e6
📒 Files selected for processing (9)
.github/workflows/build-src-cmake.yml.github/workflows/build.ymlCMakeLists.txtci/dash/build_src_cmake.shci/dash/matrix.shci/dash/test_integrationtests_cmake.shci/dash/test_unittests_cmake.shci/test/00_setup_env_native_cmake.shsrc/crypto/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/build.yml
- ci/test/00_setup_env_native_cmake.sh
- CMakeLists.txt
The Windows resource targets referenced bitcoind-res.rc, bitcoin-cli-res.rc, bitcoin-tx-res.rc and bitcoin-wallet-res.rc; this tree carries dashd-res.rc, dash-cli-res.rc, dash-tx-res.rc and dash-wallet-res.rc, so no Windows build could be generated at all. bitcoin-node and bitcoin-gui were installed under their upstream names even though rpc/node.cpp spawns 'dash-node' and init/bitcoin-gui.cpp declares EXE_NAME 'dash-gui', and bitcoin-chainstate was built but never added to installable_targets. GenerateSetupNsi.cmake substituted the upstream executable names and the bitcoin URI class into share/setup.nsi.in, so the generated NSIS script referenced files the deploy target never stages. It also never emitted an OutFile directive, which the Autotools recipe pipes in ahead of the script, leaving makensis with no output path. masternodemodel.cpp is part of BITCOIN_QT_BASE_CPP under Autotools but was grouped with masternodelist.cpp inside if(ENABLE_WALLET). rpcconsole.cpp, clientfeeds.cpp and proposalinfo.cpp are all non-wallet sources that use MasternodeModel, so a GUI build without wallet support lost its definition. Finally, Dash still provides the public libdashconsensus API that upstream removed in bitcoin#29648, so build it as a shared library with its own compilation of the consensus and crypto sources (BUILD_BITCOIN_INTERNAL, DISABLE_OPTIMIZED_SHA256), install the header and pkg-config file, and define HAVE_CONSENSUS_LIB so script_tests exercises the public API. The internal static library goes back to its own name to avoid colliding with it. Found by CodeRabbit, Codex and thepastaclaw review on PR dashpay#7481. Co-Authored-By: Claude Opus 5 <[email protected]>
The block referenced ipc_test.cpp, ipc_test.capnp and ipc_tests.cpp, none of which exist in this tree; upstream added them in bitcoin#28921, which Dash has not backported. Configuring with -DWITH_MULTIPROCESS=ON therefore failed during generation, since BUILD_TESTS defaults to ON. Found by Codex and thepastaclaw review on PR dashpay#7481. Co-Authored-By: Claude Opus 5 <[email protected]>
README told readers to run ctest from the repository root, where in-source builds are refused and no tests exist; the MSVC guide never set VCPKG_ROOT that its own presets reference, never entered the cloned directory, and carried an upstream typo. The linux64_cmake workflow header also still claimed functional tests were left entirely to Autotools, which stopped being true when the smoke list was added. Found by CodeRabbit and Codex review on PR dashpay#7481. Co-Authored-By: Claude Opus 5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
doc/build-windows-msvc.md (1)
63-74: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse separate build directories for the two vcpkg triplets.
vs2022andvs2022-staticset differentVCPKG_TARGET_TRIPLETvalues for the samebuilddirectory. Configure the second workflow with a different directory, e.g.build-vs2022andbuild-vs2022-static, or deletebuildbefore switching.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doc/build-windows-msvc.md` around lines 63 - 74, Update the Windows build instructions for the vs2022 and vs2022-static presets to use separate build directories, such as build-vs2022 and build-vs2022-static, consistently across configure, build, test, and install commands. Alternatively, explicitly instruct users to delete the existing build directory before switching presets.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@doc/build-windows-msvc.md`:
- Around line 35-39: Update the VCPKG_ROOT example in the build instructions to
avoid hardcoding the Visual Studio Community edition. Use an edition-independent
placeholder for the Visual Studio install directory or instruct users to
determine their own bundled vcpkg path before configuring.
---
Outside diff comments:
In `@doc/build-windows-msvc.md`:
- Around line 63-74: Update the Windows build instructions for the vs2022 and
vs2022-static presets to use separate build directories, such as build-vs2022
and build-vs2022-static, consistently across configure, build, test, and install
commands. Alternatively, explicitly instruct users to delete the existing build
directory before switching presets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e9cf604b-af1c-4baa-8c3b-ec419ff1384f
📒 Files selected for processing (12)
.github/workflows/build-src-cmake.ymlCMakeLists.txtREADME.mdcmake/bitcoin-config.h.incmake/introspection.cmakecmake/module/GenerateSetupNsi.cmakecmake/module/Maintenance.cmakedoc/build-windows-msvc.mdsrc/CMakeLists.txtsrc/crypto/CMakeLists.txtsrc/qt/CMakeLists.txtsrc/test/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (7)
- README.md
- .github/workflows/build-src-cmake.yml
- src/qt/CMakeLists.txt
- cmake/module/Maintenance.cmake
- src/crypto/CMakeLists.txt
- cmake/bitcoin-config.h.in
- CMakeLists.txt
| The presets below reference `$env{VCPKG_ROOT}`. Set it to the vcpkg instance | ||
| shipped with Visual Studio, or to your own clone, before configuring: | ||
| ``` | ||
| $env:VCPKG_ROOT = "C:\Program Files\Microsoft Visual Studio\2022\Community\VC\vcpkg" | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file excerpt =="
sed -n '1,70p' doc/build-windows-msvc.md
echo
echo "== search for VCPKG_ROOT mentions =="
rg -n "VCPKG_ROOT|vcpkg\.cmake|Visual Studio|VSINSTALLDIR|VCInstallDir" .
echo
echo "== git diff/stat for doc/build-windows-msvc.md (if available) =="
git diff --stat -- doc/build-windows-msvc.md || true
git diff -- doc/build-windows-msvc.md | sed -n '1,220p' || trueRepository: dashpay/dash
Length of output: 7095
🌐 Web query:
Visual Studio 2022 vcpkg install location Community Professional Enterprise programs files VC vcpkg
💡 Result:
In Visual Studio 2022 (version 17.6 and later), the vcpkg package manager is included as an optional component that you can select during installation via the Visual Studio Installer [1][2]. When installed as a component of Visual Studio 2022, vcpkg is located within the Visual Studio installation directory, typically under: C:\Program Files\Microsoft Visual Studio\2022[Edition]\VC\vcpkg In this path, [Edition] corresponds to your specific version (e.g., Community, Professional, or Enterprise) [3][1]. Key details regarding this included version of vcpkg: - Functionality: The version bundled with Visual Studio supports manifest mode but does not support classic mode, as the installation directory is protected and requires Visual Studio installer permissions [1][4]. - Updates: You update this version of vcpkg by updating your Visual Studio installation through the Visual Studio Installer [5]. - Environment Variable Conflicts: When using the bundled vcpkg, Visual Studio may automatically configure the environment (specifically the VCPKG_ROOT variable) to point to this internal location [3]. If you intend to use a separate, standalone instance of vcpkg, you may need to manually override or configure your environment variables (like VCPKG_ROOT) and project settings to point to your desired directory [6][7][2]. - Manual Setup: For developers who require classic mode or specific versions of vcpkg, it remains common practice to download and manage a standalone installation of vcpkg independently of the Visual Studio-bundled component [1][4].
Citations:
- 1: https://devblogs.microsoft.com/cppblog/vcpkg-is-now-included-with-visual-studio/
- 2: https://learn.microsoft.com/en-gb/answers/questions/1691336/does-visual-studio-build-tools-2022-have-a-vcpkg-c
- 3: enforcement of VCPKG_ROOT variable in VS`22, incorrect documentation page for vcpkg + Visual Studio 2022 microsoft/vcpkg#46148
- 4: Why to setup vcpkg if its included in VS 2022? MicrosoftDocs/vcpkg-docs#231
- 5: https://learn.microsoft.com/en-us/vcpkg/about/faq
- 6: https://learn.microsoft.com/en-us/vcpkg/get_started/get-started-msbuild
- 7: https://learn.microsoft.com/en-us/vcpkg/get_started/get-started-vs
🌐 Web query:
"VSInstallRoot" "'Program Files' 'Microsoft Visual Studio' 2022 'VC' 'vcpkg.cmake"
💡 Result:
When you install the vcpkg component in Visual Studio 2022, the vcpkg files are located in a subdirectory of your Visual Studio installation [1][2][3]. The base path, often represented by the MSBuild property
Citations:
- 1: Migrate spdlog from submodule to vcpkg microsoft/PowerToys#48039
- 2: microsoft/terminal@3c5800f
- 3: https://devblogs.microsoft.com/cppblog/vcpkg-is-now-included-with-visual-studio/
- 4: https://github.com/MicrosoftDocs/visualstudio-docs/blob/main/docs/msbuild/msbuild-reserved-and-well-known-properties.md
- 5: https://github.com/microsoft/terminal/blob/47e255f0/src/common.build.pre.props
- 6: https://learn.microsoft.com/en-us/vcpkg/users/buildsystems/cmake-integration
- 7: https://github.com/microsoft/vcpkg-docs/blob/main/vcpkg/consume/classic-mode.md
🌐 Web query:
Microsoft Visual Studio 2022 vcpkg "VC\vcpkg" Professional Enterprise path
💡 Result:
In Visual Studio 2022 (version 17.6 and later), the vcpkg package manager is available as an optional, built-in component [1]. When installed, this component resides within the Visual Studio installation directory at the following path structure [2][3]: C:\Program Files\Microsoft Visual Studio\2022<Edition>\VC\vcpkg In this path, corresponds to your specific installation, such as Professional or Enterprise [3][4]. Key characteristics and common behaviors include: Automatic Environment Setting: When you use the Visual Studio Developer Command Prompt, it automatically sets the VCPKG_ROOT environment variable to this internal path [2]. This ensures that the IDE can immediately locate and utilize the built-in vcpkg instance [1]. Overriding Behavior: Because the built-in vcpkg is automatically integrated, it may override or conflict with custom, standalone vcpkg installations if you attempt to use them simultaneously [4][5]. If you intend to use a separate, external vcpkg repository, you may experience mismatched VCPKG_ROOT warnings or path conflicts [6][7]. To use an external installation exclusively, some users choose to uninstall the built-in vcpkg component via the Visual Studio Installer [4][5]. Integration: Once installed, you can integrate this vcpkg instance with your projects by running vcpkg integrate install (which requires administrative privileges) [1]. This enables MSBuild and CMake integration, allowing Visual Studio to automatically handle dependencies via manifest files (vcpkg.json) [8][1].
Citations:
- 1: https://devblogs.microsoft.com/cppblog/vcpkg-is-now-included-with-visual-studio/
- 2: https://walbourn.github.io/vs-2022-update-7/
- 3: VCPKG_ROOT is not "C:\vcpkg" on Windows Server 2022 actions/runner-images#9269
- 4: MSVS cannot find source files even after VCPKG installation with no errors microsoft/vcpkg#31915
- 5: The find_package directive using cmake in vs2022 cannot find the package microsoft/vcpkg#40441
- 6: ignoring mismatched VCPKG_ROOT environment value microsoft/vcpkg#40544
- 7: https://github.com/microsoft/vcpkg-tool/pull/1931/changes
- 8: https://learn.microsoft.com/en-us/vcpkg/get_started/get-started-vs
Use an edition-independent vcpkg root in the example.
Visual Studio installs the bundled vcpkg under <InstallDir>\VC\vcpkg, where <InstallDir> includes the edition (Community, Professional, Enterprise). Editing line 38 to show only the required validation/test path would avoid pointing Professional/Enterprise or custom installs at a nonexistent Community directory; alternatively, direct users to inspect their own VCPKG_ROOT.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 37-37: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@doc/build-windows-msvc.md` around lines 35 - 39, Update the VCPKG_ROOT
example in the build instructions to avoid hardcoding the Visual Studio
Community edition. Use an edition-independent placeholder for the Visual Studio
install directory or instruct users to determine their own bundled vcpkg path
before configuring.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd328dd146
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| install(FILES script/bitcoinconsensus.h | ||
| DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/script | ||
| ) |
There was a problem hiding this comment.
When consumers switch from an Autotools installation to cmake --install, this places the header under ${includedir}/script, while src/Makefile.am uses include_HEADERS, which installs its basename directly as ${includedir}/bitcoinconsensus.h; the generated pkg-config file also exposes only -I${includedir}. Existing consumers using #include <bitcoinconsensus.h> therefore stop compiling, so install the header directly into ${CMAKE_INSTALL_INCLUDEDIR} to preserve the public API layout.
Useful? React with 👍 / 👎.
| # without the optimised SHA-256 backends; see libdashconsensus_la in | ||
| # src/Makefile.am. | ||
| if(BUILD_CONSENSUS_LIB) | ||
| add_library(dashconsensus SHARED |
There was a problem hiding this comment.
On ELF platforms, creating this shared target without SOVERSION gives it an unversioned libdashconsensus.so soname, whereas the existing libtool target defaults to ABI version 0 and supplies libdashconsensus.so.0. Replacing an Autotools-built package with the CMake installation can consequently leave existing applications unable to load the library; set SOVERSION 0 (and an appropriate VERSION) to preserve runtime compatibility.
Useful? React with 👍 / 👎.
The NSIS generator and the Windows deploy target both need it, and they live in different function scopes, so the name was spelled out by hand in both places. Set it once next to PACKAGE_NAME instead. Also note that the dashconsensus block depends on add_subdirectory(crypto) having already run, since that is what populates the source-list variables it consumes. Co-Authored-By: Claude Opus 5 <[email protected]>
3 participants
Issue being fixed or feature implemented
Dash Core only has an Autotools build. Bitcoin Core replaced Autotools with CMake
in v29, and every backport from that point on carries build-system changes we
cannot apply. This adds the CMake build system alongside Autotools so both
work, which lets us backport upstream build changes incrementally and gives
Windows/MSVC users a supported path.
The first commit is the upstream CMake tree adapted to Dash. The remaining
commits fix the divergences from our Autotools build that the adaptation missed —
several of which silently disabled Dash-specific functionality rather than
failing loudly.
What was done?
build: add cmake build system alongside autotoolsports Bitcoin Core's CMakebuild (top-level
CMakeLists.txt,cmake/modules, per-directoryCMakeLists.txtundersrc/,depends/toolchain.cmake.in,vcpkg.json,CMakePresets.json) and keepsconfigure.ac/Makefile.amfully functional.Follow-up commits:
build: fix CMake build failuresbitcoin_commoncompilescommon/url.cpp, which includes<event2/http.h>, but did not link libevent. This only builds wherelibevent headers happen to sit in a default include path — it fails with
Homebrew and with
depends.test_bitcoin-qtdid not link LevelDB, sodbwrapper.h(reachable from theDash-specific headers those tests pull in) failed to compile.
qt/test/trafficgraphdatatests.cppwas missing from the Qt test targetwhile
test_main.cppstill instantiatesTrafficGraphDataTests, so thebinary could not link even once it compiled.
bitcoin_zmqlinked neither LevelDB nor dashbls, so a-DWITH_ZMQ=ONbuild failed outright:
zmqpublishnotifier.cppreachesdbwrapper.hthrough
node/blockstorage.handbls.hthroughchainlock/chainlock.h.-isystemunder Autotools but notunder CMake, producing ~13k
-Wdocumentationwarnings and breaking anyWERRORbuild. They are now marked as system includes for consumers.build: restore Dash-specific feature detection under CMake— the CMakeconfig header was missing a set of macros that Dash's sources check. Each of
these built and linked fine while quietly changing behaviour:
ENABLE_MINER— every mining RPC (generatetoaddress,generatetodescriptor,generateblock) fell through to the"This call is not available because RPC miner isn't compiled" stub, which
breaks essentially every functional test.
ENABLE_SSSE3,ENABLE_X86_AESNI,ENABLE_ARM_AES,ENABLE_ARM_NEON—detection and the corresponding
crypto/x11/{ssse3,x86_aesni,arm_crypto,arm_neon}sources were absent, so X11 hashing silently fell back to the generic
implementations on every platform.
HAVE_SYS_GETRANDOM— CMake probed for the libcgetrandom()wrapper anddefined
HAVE_GETRANDOM, whichrandom.cppdoes not read; it checksHAVE_SYS_GETRANDOMand issues the syscall directly.ENABLE_STACKTRACES/ENABLE_CRASH_HOOKS/CRASH_HOOKS_WRAPPED_CXX_ABI—no option existed at all. Added with libbacktrace detection mirroring
configure.ac, including-Wl,-export-dynamic/-rdynamic,-gdwarf-4,-fno-standalone-debug, and the-Wl,-wrapexception hooks.GSL_NO_IOSTREAMS, and theNOWARN_CXXFLAGSsuppressions(
-Wno-array-boundsand friends) without which a GCCWERRORbuild failson
immer.build: match the Autotools warning and optimisation settings-Wno-error=deprecated-literal-operator(Clang 20 + hana) was missing fromthe warning block.
-Wundefis not in Dash's Autotools warning set, andcrypto/x11/sph_types.htestsSPH_*macros that are deliberately leftundefined, so any
-DWERROR=ONbuild failed. Downgraded to-Wno-error=undefrather than dropped, since it is exactly the check thatcatches the
#if ENABLE_MINERclass of bug fixed above.-O3underAutotools (
SPHLIB_FLAGS) because they sit on the X11 proof-of-work path.Under CMake they were folded into
bitcoin_cryptoat the default-O2.AM_CFLAGScarries neitherWARN_CXXFLAGSnorERROR_CXXFLAGS, so the vendored sphlib C sources are never built with-Werrorthere. CMake'score_interfaceoptions apply to every language,so
-DWERROR=ONfailed ongroestl.c; the C sources now opt out.build: report the git description in CMake builds— upstream'sGenerateBuildInfo.cmakeemitsBUILD_GIT_TAG/BUILD_GIT_COMMIT, but Dash'sclientversion.cpponly understandsBUILD_GIT_DESCRIPTION, so everynon-release CMake build reported itself as
v23.1.5-unk. It now emits thesame
git describe --abbrev=12 --dirtystring asshare/genbuild.sh.build: drop the unusable libbitcoinkernel CMake target—src/kernel/CMakeLists.txtreferenced eleven files that do not exist in Dash(
kernel/bitcoinkernel.cpp,kernel/chainparams.cpp,policy/truc_policy.cpp,…) and was gated behind an
EXISTScheck so it could never build. Removedalong with
BUILD_KERNEL_LIB;bitcoin-chainstateis unaffected.test: fix CMake test wiring—test/coinjoin_dstxmanager_tests.cpp,test/coinjoin_inouts_tests.cppandtest/coinjoin_queue_tests.cppwere notin the unit-test target;
PROVIDE_FUZZ_MAIN_FUNCTIONwas never defined eventhough the probe for it was; and
config.inisubstitutedENABLE_FUZZ_BINARY_TRUEwhile the template usesENABLE_FUZZ_TRUE, leavingENABLE_FUZZ=trueunconditionally set fortest/fuzz/test_runner.py.ci: keep the Autotools invocations working—00_setup_env_s390x.shhadbeen switched to
BITCOIN_CONFIG="-DREDUCE_EXPORTS=ON", whichci/dash/build_src.shpasses to./configure;wrap-wine.shhad dropped thesecp256k1/minisketch test binaries; and
test_runner.pyhad been switched toread the functional tests out of the build directory, which only CMake
populates. All three are reverted until CMake is the only build system.
doc: adapt the MSVC build guide to Dash— the newdoc/build-windows-msvc.mdtold readers to clone bitcoin/bitcoin, andsrc/qt/README.mdhad been repointed away from our cross-compile guide.ci: add a linux64_cmake job that builds the full stack with CMake— anative Linux target that reuses the depends prefix built for the
linux64Autotools job. Same
HOSTandDEP_OPTS, so the depends cache is sharedrather than duplicated, and the
make -C dependsstep that restores theprefix is also what writes
depends/$HOST/toolchain.cmake, which the buildthen consumes. Feature set therefore comes from what depends actually built
(GUI, wallet with BDB and SQLite, ZMQ),
WERRORis on as it is everywhereelse, and the job runs the full unit-test suite through
ctestplus athree-test functional smoke list.
The smoke list is not redundant with the Autotools functional jobs:
mining_basic.pyis what catches a missingENABLE_MINER, and no unit testwould. Artifact bundling and the full functional suite deliberately stay with
the Autotools jobs. The job is gated on
SKIP_LINUX64_CMAKElike itssiblings.
build: restore the Dash build outputs the CMake port dropped— findingsfrom the automated review on this PR, all verified against
src/Makefile.amand
configure.acbefore acting on them:bitcoind-res.rc,bitcoin-cli-res.rc,bitcoin-tx-res.rcandbitcoin-wallet-res.rc; thistree carries the
dash*-named files, so no Windows build could begenerated at all.
bitcoin-nodeandbitcoin-guiwere installed under their upstream names,while
rpc/node.cppspawnsdash-nodeandinit/bitcoin-gui.cppdeclaresEXE_NAME "dash-gui".bitcoin-chainstatewas built but never installed.GenerateSetupNsi.cmakesubstituted upstream executable names and thebitcoinURI class intoshare/setup.nsi.in, and emitted noOutFiledirective, so the
deploytarget could not produce an installer.masternodemodel.cppwas grouped withmasternodelist.cppunderif(ENABLE_WALLET), but it lives inBITCOIN_QT_BASE_CPPupstream — andrpcconsole.cpp,clientfeeds.cppandproposalinfo.cppare all non-walletsources that use
MasternodeModel.libdashconsensuswas missing entirely. Dash still provides the publicconsensus API that upstream removed in Remove libbitcoinconsensus bitcoin/bitcoin#29648, and Guix passes no
--with-libs, so releases ship it. It is now built as a shared library withits own compilation of the consensus and crypto sources
(
BUILD_BITCOIN_INTERNAL,DISABLE_OPTIMIZED_SHA256), with the header andpkg-config file installed and
HAVE_CONSENSUS_LIBdefined soscript_testsexercises the public API again.
build: drop the multiprocess IPC test block Dash cannot build— itreferenced
ipc_test.cpp,ipc_test.capnpandipc_tests.cpp, which camefrom multiprocess: Add basic type conversion hooks bitcoin/bitcoin#28921 and were never backported, so
-DWITH_MULTIPROCESS=ONfailed during generation.
How Has This Been Tested?
On macOS/arm64 with Homebrew dependencies:
cmake -B build && cmake --build build— clean build ofdashd,dash-cli,dash-tx,dash-util,dash-walletandtest_dash.cmake -B build -DWERROR=ON && cmake --build build— clean, 0 errors../src/test/test_dash— 696 test cases, no errors, on both the default andthe
WERRORbuild (includes the three CoinJoin suites restored here).cmake -B build -DBUILD_GUI=ON -DBUILD_GUI_TESTS=ON && cmake --build build—builds
dash-qtandtest_dash-qt.bitcoin-config.hmacro set,per-target source lists (
Makefile*.am/*.includevs everyCMakeLists.txt),JSON/raw test-data lists, and the generated
obj/build.h.test/lint/lint-python.py,lint-shell.py,lint-whitespace.pypass.libdashconsensus: exports the publicdashconsensus_*symbols,script_testsruns the five API cases thatHAVE_CONSENSUS_LIBguards, andcmake --installlays downlib/,include/script/andlib/pkgconfig/tomatch the Autotools layout.
-DBUILD_GUI=ON -DENABLE_WALLET=OFFlinks, which is the configuration themasternodemodelfix restores.The path the new CI job takes was also run locally end to end, on a native
macOS depends prefix — which is how the
bitcoin_zmqlink failure above wasfound, since it only appears once ZMQ is actually enabled:
make -C depends, thencmake --toolchain depends/<host>/toolchain.cmake -DWERROR=ON -DBUILD_BENCH=ON—configure picks up wallet/BDB/SQLite/ZMQ from depends, builds clean.
mining_basic.py,rpc_blockchain.pyandwallet_basic.pyrun from theCMake build directory and pass (6 variants total across v1/v2 transport and
descriptor/legacy wallets).
mining_basic.pyis a direct regression test forthe
ENABLE_MINERfix.Not exercised here, and worth reviewer attention:
Guix path — the new CI job covers native Linux only.
vcpkg.jsonhas nogmpdependency; dashbls will fallback to relic's
easyarithmetic backend there.WITH_MULTIPROCESS(libmultiprocess/capnp) — it now configures, but Dash hasno IPC test sources, so that block is left out with a comment.
depends/packages/qt.mknow runs a fullmake installfor qtbase/qttools andkeeps
lib/cmake/, which is whatfind_package(Qt5)needs. This invalidatesthe depends cache and grows the Qt staging output for both build systems.
The
linux64_cmakejob added here covers the native Linux path end to end.The cross-compile targets, Guix and MSVC remain Autotools-only for now.
Breaking Changes
None. Autotools remains the build system used by CI,
dependsand Guix; CMakeis additive.
Checklist: