feat(wallet): external signer (hardware signer) #6019
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -319,10 +319,10 @@ AC_ARG_ENABLE([werror], | |
| [enable_werror=$enableval], | ||
| [enable_werror=no]) | ||
|
|
||
| AC_ARG_ENABLE([external-signer], | ||
| [AS_HELP_STRING([--enable-external-signer],[compile external signer support (default is auto, requires Boost::Process)])], | ||
| [use_external_signer=$enableval], | ||
| [use_external_signer=auto]) | ||
|
|
||
| AC_LANG_PUSH([C++]) | ||
|
|
||
|
|
@@ -1437,6 +1437,7 @@ if test "$enable_fuzz" = "yes"; then | |
| bitcoin_enable_qt_dbus=no | ||
| use_bench=no | ||
| use_tests=no | ||
| use_external_signer=no | ||
| use_upnp=no | ||
| use_natpmp=no | ||
| use_zmq=no | ||
|
|
@@ -1578,18 +1579,18 @@ if test "$use_natpmp" != "no"; then | |
| fi | ||
|
|
||
| if test "$build_bitcoin_wallet$build_bitcoin_cli$build_bitcoin_tx$build_bitcoind$bitcoin_enable_qt$use_tests$use_bench$enable_fuzz_binary" = "nononononononono"; then | ||
| use_boost=no | ||
| else | ||
| use_boost=yes | ||
| fi | ||
|
|
||
| if test "$use_boost" = "yes"; then | ||
|
|
||
| dnl Check for Boost headers | ||
| AX_BOOST_BASE([1.73.0],[],[AC_MSG_ERROR([Boost is not available!])]) | ||
| if test "$want_boost" = "no"; then | ||
| AC_MSG_ERROR([[only libdashconsensus can be built without boost]]) | ||
| fi | ||
|
|
||
| dnl we don't use multi_index serialization | ||
| BOOST_CPPFLAGS="$BOOST_CPPFLAGS -DBOOST_MULTI_INDEX_DISABLE_SERIALIZATION" | ||
|
|
@@ -1598,27 +1599,36 @@ fi | |
| BOOST_CPPFLAGS="$BOOST_CPPFLAGS -DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE" | ||
| fi | ||
|
|
||
| dnl Prevent use of std::unary_function, which was removed in C++17, | ||
| dnl and will generate warnings with newer compilers for Boost | ||
| dnl older than 1.80. | ||
| dnl See: https://github.com/boostorg/config/pull/430. | ||
| AX_CHECK_PREPROC_FLAG([-DBOOST_NO_CXX98_FUNCTION_BASE], [BOOST_CPPFLAGS="$BOOST_CPPFLAGS -DBOOST_NO_CXX98_FUNCTION_BASE"], [], [$CXXFLAG_WERROR], | ||
| [AC_LANG_PROGRAM([[#include <boost/config.hpp>]])]) | ||
|
|
||
| if test "$suppress_external_warnings" != "no"; then | ||
| BOOST_CPPFLAGS=SUPPRESS_WARNINGS($BOOST_CPPFLAGS) | ||
| fi | ||
| fi | ||
|
|
||
| if test "$use_external_signer" != "no"; then | ||
| case $host in | ||
| *mingw*) | ||
| dnl Boost Process uses Boost Filesystem when targeting Windows. Also, | ||
| dnl since Boost 1.71.0, Process does not work with mingw-w64 without | ||
| dnl workarounds. See 67669ab425b52a2b6be3d2f3b3b7e3939b676a2c. | ||
| if test "$use_external_signer" = "yes"; then | ||
| AC_MSG_ERROR([External signing is not supported on Windows]) | ||
| fi | ||
| use_external_signer="no"; | ||
| ;; | ||
| *) | ||
| use_external_signer="yes" | ||
| AC_DEFINE([ENABLE_EXTERNAL_SIGNER], [1], [Define if external signer support is enabled]) | ||
| ;; | ||
| esac | ||
| fi | ||
|
Comment on lines
+1614
to
1630
There was a problem hiding this comment. 🧩 Analysis chainVerify Boost.Process availability before enabling external signer. The code enables Based on learnings Run the following script to check if Boost.Process detection exists elsewhere in configure.ac: 🏁 Script executed: #!/bin/bash
# Search for Boost.Process availability checks
rg -n "boost.*process|BOOST.*PROCESS" configure.acLength of output: 0 🏁 Script executed: #!/bin/bash
# Search for Boost detection patterns in configure.ac
echo "=== Boost checks in configure.ac ==="
rg -n "AC_CHECK|boost|BOOST" configure.ac | head -30
echo -e "\n=== Search for Process around lines 1609-1625 ==="
sed -n '1590,1650p' configure.ac
echo -e "\n=== Search for Process dependencies in codebase ==="
rg -i "boost.*process|process.*library" --type=cpp --type=h -C2 | head -40Length of output: 8022 🏁 Script executed: #!/bin/bash
# Check if Boost.Process is mentioned in any build files or documentation
echo "=== Search for Process in build/config files ==="
fd -e "ac$|CMakeLists.txt|Makefile" | xargs rg -l -i "process" 2>/dev/null | head -10
echo -e "\n=== Broader search for external signer and boost in codebase ==="
rg -i "external.signer" -A5 -B5 | head -50Length of output: 2714 🏁 Script executed: #!/bin/bash
# Check how ENABLE_EXTERNAL_SIGNER is used in build system
echo "=== ENABLE_EXTERNAL_SIGNER in Makefile.am ==="
rg -n "ENABLE_EXTERNAL_SIGNER" --type=makefile
echo -e "\n=== Source files with ENABLE_EXTERNAL_SIGNER guard ==="
rg -n "ENABLE_EXTERNAL_SIGNER" --type=cpp --type=h -C2 | head -60
echo -e "\n=== Check for ax_boost or AX_BOOST macros ==="
rg -n "AX_BOOST|ax_boost" configure.acLength of output: 3173 🏁 Script executed: #!/bin/bash
# Check how ENABLE_EXTERNAL_SIGNER is used in build system
echo "=== ENABLE_EXTERNAL_SIGNER in Makefile.am ==="
rg -n "ENABLE_EXTERNAL_SIGNER" --type=makefile
echo -e "\n=== Source files with ENABLE_EXTERNAL_SIGNER guard ==="
rg -n "ENABLE_EXTERNAL_SIGNER" --type=cpp --type=h -C2 | head -60
echo -e "\n=== Check for ax_boost or AX_BOOST macros ==="
rg -n "AX_BOOST|ax_boost" configure.acLength of output: 3505 Add Boost.Process availability check before enabling external signer. The code enables 🤖 Prompt for AI Agents
Collaborator
Author
There was a problem hiding this comment. external signer is currently disabled for Windows completely (see 444a8d0, backport of "Merge bitcoin#24065: build: explicitly disable support for externa...") There was a problem hiding this comment. 🧩 Analysis chain🏁 Script executed: Length of output: 2101 🏁 Script executed: Length of output: 1416 🏁 Script executed: Length of output: 3889 @knst, thank you for the clarification about Windows support. You're correct that commit 444a8d0 (Bitcoin PR bitcoin#24065) explicitly disables external signing on Windows due to Boost.Process's dependency on boost::filesystem, which conflicts with removing Boost filesystem usage. However, the original concern was about non-Windows platforms (line 1621 in the diff). On non-Windows systems, the code sets That said, examining the commit history shows this behavior matches upstream Bitcoin Core - the configure.ac help text mentions "requires Boost::Process" but there's no explicit availability check. Since this is a backport PR and the pattern matches upstream, this appears to be intentional: the assumption is that Boost.Process is available when Boost is present. Given this is a faithful backport and matches Bitcoin Core's approach, the current implementation is acceptable. Users who encounter compilation failures due to missing Boost.Process can use 🧠 Learnings used |
||
| AM_CONDITIONAL([ENABLE_EXTERNAL_SIGNER], [test "$use_external_signer" = "yes"]) | ||
|
|
||
UdjinM6 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| dnl Check for reduced exports | ||
| if test "$use_reduce_exports" = "yes"; then | ||
|
|
@@ -2012,6 +2022,7 @@ AC_SUBST(ARM_SHANI_CXXFLAGS) | |
| AC_SUBST(LIBTOOL_APP_LDFLAGS) | ||
| AC_SUBST(USE_SQLITE) | ||
| AC_SUBST(USE_BDB) | ||
| AC_SUBST(ENABLE_EXTERNAL_SIGNER) | ||
| AC_SUBST(USE_UPNP) | ||
| AC_SUBST(USE_QRCODE) | ||
| AC_SUBST(TESTDEFS) | ||
|
|
@@ -2079,7 +2090,7 @@ esac | |
|
|
||
| echo | ||
| echo "Options used to compile and link:" | ||
| echo " external signer = $use_external_signer" | ||
| echo " multiprocess = $build_multiprocess" | ||
| echo " with libs = $build_bitcoin_libs" | ||
| echo " with wallet = $enable_wallet" | ||
|
|
@@ -2089,36 +2100,36 @@ if test "$enable_wallet" != "no"; then | |
| fi | ||
| echo " with gui / qt = $bitcoin_enable_qt" | ||
| if test $bitcoin_enable_qt != "no"; then | ||
| echo " with qr = $use_qr" | ||
| fi | ||
| echo " with zmq = $use_zmq" | ||
| if test $enable_fuzz = "no"; then | ||
| echo " with test = $use_tests" | ||
| else | ||
| echo " with test = not building test_dash because fuzzing is enabled" | ||
| fi | ||
| echo " with fuzz binary= $enable_fuzz_binary" | ||
| echo " with bench = $use_bench" | ||
| echo " with upnp = $use_upnp" | ||
| echo " with natpmp = $use_natpmp" | ||
| echo " USDT tracing = $use_usdt" | ||
| echo " sanitizers = $use_sanitizers" | ||
| echo " debug enabled = $enable_debug" | ||
| echo " stacktraces = $enable_stacktraces" | ||
| echo " crash hooks = $enable_crashhooks" | ||
| echo " miner enabled = $enable_miner" | ||
| echo " gprof enabled = $enable_gprof" | ||
| echo " werror = $enable_werror" | ||
| echo | ||
| echo " target os = $host_os" | ||
| echo " build os = $build_os" | ||
| echo | ||
| echo " CC = $CC" | ||
| echo " CFLAGS = $DEBUG_CFLAGS $PTHREAD_CFLAGS $BACKTRACE_FLAGS $CFLAGS" | ||
| echo " CPPFLAGS = $DEBUG_CPPFLAGS $HARDENED_CPPFLAGS $CORE_CPPFLAGS $CPPFLAGS" | ||
| echo " CXX = $CXX" | ||
| echo " CXXFLAGS = $DEBUG_CXXFLAGS $HARDENED_CXXFLAGS $WARN_CXXFLAGS $NOWARN_CXXFLAGS $ERROR_CXXFLAGS $GPROF_CXXFLAGS $CORE_CXXFLAGS $BACKTRACE_FLAGS $CXXFLAGS" | ||
| echo " LDFLAGS = $PTHREAD_LIBS $HARDENED_LDFLAGS $GPROF_LDFLAGS $CORE_LDFLAGS $BACKTRACE_LDFLAGS $LDFLAGS" | ||
| echo " AR = $AR" | ||
| echo " ARFLAGS = $ARFLAGS" | ||
| echo | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Consider the scope of CI changes in this PR.
This file removes the
--with-boost-processflag from the build configuration. The coding guidelines state: "Unless specifically prompted, avoid making changes to the ci directory."Given the PR's scope issues, consider isolating CI configuration changes into a separate, focused PR.
As per coding guidelines.
🤖 Prompt for AI Agents