diff options
author | fanquake <fanquake@gmail.com> | 2023-12-13 11:27:02 +0000 |
---|---|---|
committer | fanquake <fanquake@gmail.com> | 2023-12-13 11:55:21 +0000 |
commit | f0e829022a415c7c9513e715c532079ec7756306 (patch) | |
tree | 1a5f3af7ba70c2555ea4d46e5d097ebae69464d5 | |
parent | 8431a195375bbf4a2dd2d646031b07beace3be80 (diff) | |
parent | 308aec3e5655327d98e0428d8205d246f24d6af5 (diff) | |
download | bitcoin-f0e829022a415c7c9513e715c532079ec7756306.tar.xz |
Merge bitcoin/bitcoin#28967: build: disable external-signer for Windows
308aec3e5655327d98e0428d8205d246f24d6af5 build: disable external-signer for Windows (fanquake)
35537318a19360ddf1ea8f0c1e6d8ad49e635516 ci: remove --enable-external-signer from win64 job (fanquake)
Pull request description:
It's come to light that Boost ASIO (a Boost Process sub dep) has in some
instances, been quietly initialising our network stack on Windows (see
PR https://github.com/bitcoin/bitcoin/pull/28486 and discussion in https://github.com/bitcoin/bitcoin/issues/28940).
This has been shielding a bug in our own code, but the larger issue
is that Boost Process/ASIO is running code before main, and doing things
like setting up networking. This undermines our own assumptions about
how our binary works, happens before we run any sanity checks,
and before we call our own code to setup networking. Note that ASIO also
calls WSAStartup with version `2.0`, whereas we call with `2.2`.
It's also not clear why a feature like external signer would have a
dependency that would be doing anything network/socket related,
given it only exists to spawn a local process.
See also the discussion in https://github.com/bitcoin/bitcoin/issues/24907. Note that the maintaince of Boost Process in general,
has not really improved. For example, rather than fixing bugs like https://github.com/boostorg/process/issues/111,
i.e, https://github.com/boostorg/process/pull/317, the maintainer chooses to just wrap exception causing overflows
in try-catch blocks: https://github.com/boostorg/process/commit/0c42a58eacab6a96b19196e399307bad8a938a27. These changes get merged in large,
unreviewed PRs, i.e https://github.com/boostorg/process/pull/319.
This PR disables external-signer on Windows for now. If, in future, someone
changes how Boost Process works, or replaces it entirely with some
properly reviewed and maintained code, we could reenable this feature on
Windows.
ACKs for top commit:
hebasto:
re-ACK 308aec3e5655327d98e0428d8205d246f24d6af5.
TheCharlatan:
ACK 308aec3e5655327d98e0428d8205d246f24d6af5
Tree-SHA512: 7405f7fc9833eeaacd6836c4e5b1c1a7845a40c1fdd55c1060152f8d8189e4777464fde650e11eb1539556a75dddf49667105987078b1457493ee772945da66e
-rw-r--r-- | build_msvc/bitcoin_config.h.in | 3 | ||||
-rw-r--r-- | build_msvc/vcpkg.json | 1 | ||||
-rwxr-xr-x | ci/test/00_setup_env_win64.sh | 2 | ||||
-rw-r--r-- | configure.ac | 16 |
4 files changed, 14 insertions, 8 deletions
diff --git a/build_msvc/bitcoin_config.h.in b/build_msvc/bitcoin_config.h.in index 1716647486..ec69307f50 100644 --- a/build_msvc/bitcoin_config.h.in +++ b/build_msvc/bitcoin_config.h.in @@ -41,9 +41,6 @@ /* Define this symbol to enable ZMQ functions */ #define ENABLE_ZMQ 1 -/* define if external signer support is enabled (requires Boost::Process) */ -#define ENABLE_EXTERNAL_SIGNER /**/ - /* Define to 1 if you have the declaration of `be16toh', and to 0 if you don't. */ #define HAVE_DECL_BE16TOH 0 diff --git a/build_msvc/vcpkg.json b/build_msvc/vcpkg.json index 18ac8f2f09..b2c406ae12 100644 --- a/build_msvc/vcpkg.json +++ b/build_msvc/vcpkg.json @@ -5,7 +5,6 @@ "berkeleydb", "boost-date-time", "boost-multi-index", - "boost-process", "boost-signals2", "boost-test", "libevent", diff --git a/ci/test/00_setup_env_win64.sh b/ci/test/00_setup_env_win64.sh index ebd4487c52..ba9af480ac 100755 --- a/ci/test/00_setup_env_win64.sh +++ b/ci/test/00_setup_env_win64.sh @@ -16,4 +16,4 @@ export GOAL="deploy" # Prior to 11.0.0, the mingw-w64 headers were missing noreturn attributes, causing warnings when # cross-compiling for Windows. https://sourceforge.net/p/mingw-w64/bugs/306/ # https://github.com/mingw-w64/mingw-w64/commit/1690994f515910a31b9fb7c7bd3a52d4ba987abe -export BITCOIN_CONFIG="--enable-reduce-exports --enable-external-signer --disable-gui-tests CXXFLAGS=-Wno-return-type" +export BITCOIN_CONFIG="--enable-reduce-exports --disable-gui-tests CXXFLAGS=-Wno-return-type" diff --git a/configure.ac b/configure.ac index 58235d67c6..18b00a2b16 100644 --- a/configure.ac +++ b/configure.ac @@ -1505,9 +1505,19 @@ if test "$use_external_signer" != "no"; then CXXFLAGS="$TEMP_CXXFLAGS" AC_MSG_RESULT([$have_boost_process]) if test "$have_boost_process" = "yes"; then - use_external_signer="yes" - AC_DEFINE([ENABLE_EXTERNAL_SIGNER], [1], [Define if external signer support is enabled]) - AC_DEFINE([BOOST_PROCESS_USE_STD_FS], [1], [Defined to avoid Boost::Process trying to use Boost Filesystem]) + case $host in + dnl Boost Process for Windows uses Boost ASIO. Boost ASIO performs + dnl pre-main init of Windows networking libraries, which we do not + dnl want. + *mingw*) + use_external_signer="no" + ;; + *) + use_external_signer="yes" + AC_DEFINE([ENABLE_EXTERNAL_SIGNER], [1], [Define if external signer support is enabled]) + AC_DEFINE([BOOST_PROCESS_USE_STD_FS], [1], [Defined to avoid Boost::Process trying to use Boost Filesystem]) + ;; + esac else if test "$use_external_signer" = "yes"; then AC_MSG_ERROR([External signing is not supported for this Boost version]) |