Some thoughts:

Arguments in favor:
Many people use HWWs. This will lead to an increase in Core full node use.
GUI users don't self-compile (and shouldn't be expected to)
Still opt-in for bitcoind
Advanced users can still compile GUI without

Arguments against:
Requires boost::process
Many security concerns were raised in this PR (#15382) - I can't tell if they were addressed fully enough to change the default to enabled (@practicalswift, @jonasschnelli, @laanwj, @MarcoFalke)

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• refactor: Clean up new wallet spend, receive files added #21207 #22100 (refactor: Clean up new wallet spend, receive files added MOVEONLY: CWallet transaction code out of wallet.cpp/.h #21207 by ryanofsky)
• refactor: Make CWalletTx sync state type-safe #21206 (refactor: Make CWalletTx sync state type-safe by ryanofsky)
• Use std::filesystem. Remove Boost Filesystem & System #20744 ([POC] Use std::filesystem. Remove Boost Filesystem & System by fanquake)
• ci: tsan gui #19162 (ci: tsan gui by MarcoFalke)
• Multiprocess bitcoin #10102 ([experimental] Multiprocess bitcoin by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

The Win64 machine is failing to build the fuzz executable: undefined reference to ExternalSigner::GetDescriptors(int)'. It's configured with --disable-external-signer`, but that shouldn't matter...

Rebased after bitcoin-core/gui#4 was merged.

Windows build failure:

/usr/bin/x86_64-w64-mingw32-ld: libbitcoin_wallet.a(libbitcoin_wallet_a-wallet.o):wallet.cpp:(.text+0x1011f): undefined reference to `ExternalSigner::GetDescriptors(int)'
/usr/bin/x86_64-w64-mingw32-ld: libbitcoin_wallet.a(libbitcoin_wallet_a-external_signer_scriptpubkeyman.o):external_signer_scriptpubkeyman.cpp:(.text+0x422): undefined reference to `ExternalSigner::DisplayAddress(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const'
/usr/bin/x86_64-w64-mingw32-ld: libbitcoin_wallet.a(libbitcoin_wallet_a-external_signer_scriptpubkeyman.o):external_signer_scriptpubkeyman.cpp:(.text+0xf68): undefined reference to `ExternalSigner::Enumerate(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<ExternalSigner, std::allocator<ExternalSigner> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >)'
/usr/bin/x86_64-w64-mingw32-ld: libbitcoin_wallet.a(libbitcoin_wallet_a-external_signer_scriptpubkeyman.o):external_signer_scriptpubkeyman.cpp:(.text+0x1496): undefined reference to `ExternalSigner::SignTransaction(PartiallySignedTransaction&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&)'
collect2: error: ld returned 1 exit status

@fanquake indeed. I was hoping a rebase would magically make it go away... #21935 (comment)

I also haven't checked how this interacts with #22173.

I also haven't checked how this interacts with #22173.

#22173 can be reverted following the changes in this PR.

The Windows linker error is because it is building without external signer enabled. I am able to replicate the same error on linux by using --disable-external-signer. I haven't been able to figure out why there's a linker error.

In the "build: enable external signer by default" commit all --enable-external-signer could be removed from CI tasks.

The link error could be fixed with the following patch:

--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -399,6 +399,7 @@ endif
 libbitcoin_wallet_a_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(SQLITE_CFLAGS)
 libbitcoin_wallet_a_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
 libbitcoin_wallet_a_SOURCES = \
+  external_signer.cpp \
   wallet/coincontrol.cpp \
   wallet/context.cpp \
   wallet/crypter.cpp \
@@ -539,7 +540,6 @@ libbitcoin_common_a_SOURCES = \
   compressor.cpp \
   core_read.cpp \
   core_write.cpp \
-  external_signer.cpp \
   init/common.cpp \
   key.cpp \
   key_io.cpp \

Although, not sure if it is a proper fix.

Here is another possible fixup for the link error:

--- a/src/Makefile.test.include
+++ b/src/Makefile.test.include
@@ -160,7 +160,7 @@ BITCOIN_TESTS += \
   wallet/test/scriptpubkeyman_tests.cpp
 
 FUZZ_SUITE_LD_COMMON +=\
- $(LIBBITCOIN_WALLET) \
+ $(LIBBITCOIN_WALLET) $(LIBBITCOIN_COMMON) \
  $(SQLITE_LIBS) \
  $(BDB_LIBS)
 

The choice between two suggested fixups depends on the decision which library the external_signer belongs to -- libbitcoin_wallet or libbitcoin_common.

@promag I'm not sure what change you're proposing in #21935 (review), but note that model->node().externalSigners() can be slow, since it calls hwi.py enumerate and the result is currently not cached.

Right! Anyway, not a problem now that #22173 is not reverted.

I added one commit that addresses a bunch of small issues raised in the GUI PR, mostly adding translation hints, and disabling some fields when support is not compiled.

Was this ever fixed?

@Rspigler I attempted to in #20614, but got stuck. If someone can review that PR and provide some feedback, then I can rebase it and try again.

I applied @achow101's fuzz linker fix and moved that commit up.

ACK 2f5bdcb

Thanks @ryanofsky, @achow101, @hebasto, @promag, @Rspigler and everyone else for your input here.

Is there still a CI issue?

Is there still a CI issue?

Not sure it's related but the https://bitcoinbuilds.org Win64 job has been failing for a few days now with configure: error: Boost::Process is required for external signer support, but not available!

@Rspigler @jonatack very likely related; the GUI build settings are hardcoded rather than using the config in ci: bitcoin-core/gui#4 (comment)

I don't know how I feel about this being merged when there is still an outstanding CI issue, it introduces a new circular dependency, and there are some serious upstream boost::process issues and unresolved followups

@Rspigler the GUI CI is misconfigured, which is unrelated to this PR (and trivial to fix by whoever has access to it). It's caused problems with other commits that touch CI too, see e.g. bitcoin-core/gui#327

The PR no longer introduces a circular dependency (see fc0eca3).

It makes sense to emphasise that this is still experimental (descriptor wallets already are, which this builds on). But the Boost::Process code is unused if you don't configure -signer=, so I don't think that should block enabling the code. If you point to a malicious Python script, bad things can happen anyway, because that script runs as the same user. So the worst case scenario here is probably just a crash when you try to use a hardware wallet.