Skip to content

Commit

Permalink
Merge bitcoin/bitcoin#28967: build: disable external-signer for Windows
Browse files Browse the repository at this point in the history
308aec3 build: disable external-signer for Windows (fanquake)
3553731 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 bitcoin/bitcoin#28486 and discussion in bitcoin/bitcoin#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 bitcoin/bitcoin#24907. Note that the maintaince of Boost Process in general,
  has not really improved. For example, rather than fixing bugs like boostorg/process#111,
  i.e, boostorg/process#317, the maintainer chooses to just wrap exception causing overflows
  in try-catch blocks: boostorg/process@0c42a58. These changes get merged in large,
  unreviewed PRs, i.e boostorg/process#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 308aec3.
  TheCharlatan:
    ACK 308aec3

Tree-SHA512: 7405f7fc9833eeaacd6836c4e5b1c1a7845a40c1fdd55c1060152f8d8189e4777464fde650e11eb1539556a75dddf49667105987078b1457493ee772945da66e
  • Loading branch information
fanquake committed Dec 13, 2023
2 parents 8431a19 + 308aec3 commit f0e8290
Show file tree
Hide file tree
Showing 4 changed files with 14 additions and 8 deletions.
3 changes: 0 additions & 3 deletions build_msvc/bitcoin_config.h.in
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 0 additions & 1 deletion build_msvc/vcpkg.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
"berkeleydb",
"boost-date-time",
"boost-multi-index",
"boost-process",
"boost-signals2",
"boost-test",
"libevent",
Expand Down
2 changes: 1 addition & 1 deletion ci/test/00_setup_env_win64.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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"
16 changes: 13 additions & 3 deletions configure.ac
Original file line number Diff line number Diff line change
Expand Up @@ -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])
Expand Down

0 comments on commit f0e8290

Please sign in to comment.