Merge bitcoin/bitcoin#21935: Enable external signer support by defaul…

…t, reduce #ifdef

2f5bdcb gui: misc external signer fixes and translation hints (Sjors Provoost)
d672404 refactor: make ExternalSigner NetworkArg() and m_chain private (Sjors Provoost)
4455145 refactor: reduce #ifdef ENABLE_EXTERNAL_SIGNER usage (Sjors Provoost)
5be90c9 build: enable external signer by default (Sjors Provoost)
7d94530 refactor: clean up external_signer.h includes (Sjors Provoost)
fc0eca3 fuzz: fix fuzz binary linking order (Sjors Provoost)

Pull request description:

  This follows the introduction of GUI support in #4

  I don't think we should expect GUI users to self compile. This also enables external signer support by default for RPC users.

  In addition this PR reduces the number of `#ifdef ENABLE_EXTERNAL_SIGNER`, which also fixes #21919. When compiled with `--disable-external-signer` such wallets can't be created in RPC or GUI, but they can be loaded. Attempting any action that calls HWI will trigger an error.

  Side-note: this PR may or may not (currently) break CI for the GUI repository, as explained here: #4 (comment)

ACKs for top commit:
  achow101:
    ACK 2f5bdcb
  hebasto:
    re-ACK 2f5bdcb

Tree-SHA512: 1b71c5a8bea2be077ee9fa33a01130c957a0cf90951d4b7b04d3d0ef826bb77e474c3963abddfef2e2c1ea99d9c72cd2302d1eb9b5fcb7ba0bd2a625f006aa05

ci/test/00_setup_env_arm.sh

@@ -25,4 +25,4 @@ export RUN_FUNCTIONAL_TESTS=false
 export GOAL="install"
 # -Wno-psabi is to disable ABI warnings: "note: parameter passing for argument of type ... changed in GCC 7.1"
 # This could be removed once the ABI change warning does not show up by default
-export BITCOIN_CONFIG="--enable-glibc-back-compat --enable-reduce-exports CXXFLAGS=-Wno-psabi --enable-external-signer"
+export BITCOIN_CONFIG="--enable-glibc-back-compat --enable-reduce-exports CXXFLAGS=-Wno-psabi"

ci/test/00_setup_env_i686_centos.sh

@@ -11,6 +11,6 @@ export CONTAINER_NAME=ci_i686_centos_8
 export DOCKER_NAME_TAG=centos:8
 export DOCKER_PACKAGES="gcc-c++ glibc-devel.x86_64 libstdc++-devel.x86_64 glibc-devel.i686 libstdc++-devel.i686 ccache libtool make git python3 python3-zmq which patch lbzip2 dash rsync coreutils bison"
 export GOAL="install"
-export BITCOIN_CONFIG="--enable-zmq --with-gui=qt5 --enable-reduce-exports --enable-external-signer"
+export BITCOIN_CONFIG="--enable-zmq --with-gui=qt5 --enable-reduce-exports"
 export CONFIG_SHELL="/bin/dash"
 export TEST_RUNNER_ENV="LC_ALL=en_US.UTF-8"

ci/test/00_setup_env_mac.sh

@@ -15,4 +15,4 @@ export XCODE_BUILD_ID=12A7403
 export RUN_UNIT_TESTS=false
 export RUN_FUNCTIONAL_TESTS=false
 export GOAL="deploy"
-export BITCOIN_CONFIG="--with-gui --enable-reduce-exports --enable-external-signer"
+export BITCOIN_CONFIG="--with-gui --enable-reduce-exports"

ci/test/00_setup_env_mac_host.sh

@@ -9,7 +9,7 @@ export LC_ALL=C.UTF-8
 export HOST=x86_64-apple-darwin18
 export PIP_PACKAGES="zmq lief"
 export GOAL="install"
-export BITCOIN_CONFIG="--with-gui --enable-reduce-exports --enable-external-signer"
+export BITCOIN_CONFIG="--with-gui --enable-reduce-exports"
 export CI_OS_NAME="macos"
 export NO_DEPENDS=1
 export OSX_SDK=""

ci/test/00_setup_env_native_asan.sh

@@ -11,4 +11,4 @@ export PACKAGES="clang llvm python3-zmq qtbase5-dev qttools5-dev-tools libevent-
 export DOCKER_NAME_TAG=ubuntu:hirsute
 export NO_DEPENDS=1
 export GOAL="install"
-export BITCOIN_CONFIG="--enable-zmq --with-incompatible-bdb --with-gui=qt5 CPPFLAGS='-DARENA_DEBUG -DDEBUG_LOCKORDER' --with-sanitizers=address,integer,undefined CC=clang CXX=clang++ --enable-external-signer"
+export BITCOIN_CONFIG="--enable-zmq --with-incompatible-bdb --with-gui=qt5 CPPFLAGS='-DARENA_DEBUG -DDEBUG_LOCKORDER' --with-sanitizers=address,integer,undefined CC=clang CXX=clang++"

ci/test/00_setup_env_native_fuzz.sh

@@ -14,5 +14,5 @@ export RUN_UNIT_TESTS=false
 export RUN_FUNCTIONAL_TESTS=false
 export RUN_FUZZ_TESTS=true
 export GOAL="install"
-export BITCOIN_CONFIG="--enable-fuzz --with-sanitizers=fuzzer,address,undefined,integer CC=clang CXX=clang++ --enable-external-signer"
+export BITCOIN_CONFIG="--enable-fuzz --with-sanitizers=fuzzer,address,undefined,integer CC=clang CXX=clang++"
 export CCACHE_SIZE=200M

ci/test/00_setup_env_native_multiprocess.sh

@@ -11,7 +11,7 @@ export DOCKER_NAME_TAG=ubuntu:20.04
 export PACKAGES="cmake python3 python3-pip llvm clang"
 export DEP_OPTS="DEBUG=1 MULTIPROCESS=1"
 export GOAL="install"
-export BITCOIN_CONFIG="--enable-external-signer --enable-debug CC=clang CXX=clang++"  # Use clang to avoid OOM
+export BITCOIN_CONFIG="--enable-debug CC=clang CXX=clang++"  # Use clang to avoid OOM
 export TEST_RUNNER_ENV="BITCOIND=bitcoin-node"
 export RUN_SECURITY_TESTS="true"
 export PIP_PACKAGES="lief"

ci/test/00_setup_env_native_nowallet.sh

@@ -11,4 +11,4 @@ export DOCKER_NAME_TAG=ubuntu:18.04  # Use bionic to have one config run the tes
 export PACKAGES="python3-zmq clang-5.0 llvm-5.0"  # Use clang-5 to test C++17 compatibility, see doc/dependencies.md
 export DEP_OPTS="NO_WALLET=1"
 export GOAL="install"
-export BITCOIN_CONFIG="--enable-glibc-back-compat --enable-reduce-exports CC=clang-5.0 CXX=clang++-5.0 --enable-external-signer"
+export BITCOIN_CONFIG="--enable-glibc-back-compat --enable-reduce-exports CC=clang-5.0 CXX=clang++-5.0"

ci/test/00_setup_env_native_qt5.sh

@@ -16,4 +16,4 @@ export RUN_UNIT_TESTS="false"
 export GOAL="install"
 export PREVIOUS_RELEASES_TO_DOWNLOAD="v0.15.2 v0.16.3 v0.17.2 v0.18.1 v0.19.1"
 export BITCOIN_CONFIG="--enable-zmq --with-libs=no --with-gui=qt5 --enable-glibc-back-compat --enable-reduce-exports
---enable-debug --disable-fuzz-binary  CFLAGS=\"-g0 -O2 -funsigned-char\" CXXFLAGS=\"-g0 -O2 -funsigned-char\" --enable-external-signer"
+--enable-debug --disable-fuzz-binary  CFLAGS=\"-g0 -O2 -funsigned-char\" CXXFLAGS=\"-g0 -O2 -funsigned-char\""

ci/test/00_setup_env_native_tsan.sh

@@ -11,4 +11,4 @@ export DOCKER_NAME_TAG=ubuntu:hirsute
 export PACKAGES="clang llvm libc++abi-dev libc++-dev python3-zmq"
 export DEP_OPTS="CC=clang CXX='clang++ -stdlib=libc++'"
 export GOAL="install"
-export BITCOIN_CONFIG="--enable-zmq --with-gui=no CPPFLAGS='-DARENA_DEBUG -DDEBUG_LOCKORDER' CXXFLAGS='-g' --with-sanitizers=thread CC=clang CXX='clang++ -stdlib=libc++' --enable-external-signer"
+export BITCOIN_CONFIG="--enable-zmq --with-gui=no CPPFLAGS='-DARENA_DEBUG -DDEBUG_LOCKORDER' CXXFLAGS='-g' --with-sanitizers=thread CC=clang CXX='clang++ -stdlib=libc++'"

ci/test/00_setup_env_s390x.sh

@@ -23,4 +23,4 @@ export RUN_UNIT_TESTS=true
 export TEST_RUNNER_ENV="LC_ALL=C"
 export RUN_FUNCTIONAL_TESTS=true
 export GOAL="install"
-export BITCOIN_CONFIG="--enable-reduce-exports --with-incompatible-bdb --enable-external-signer"
+export BITCOIN_CONFIG="--enable-reduce-exports --with-incompatible-bdb"

configure.ac

@@ -333,9 +333,9 @@ AC_ARG_ENABLE([werror],
     [enable_werror=no])
 
 AC_ARG_ENABLE([external-signer],
-    [AS_HELP_STRING([--enable-external-signer],[compile external signer support (default is no, requires Boost::Process)])],
+    [AS_HELP_STRING([--enable-external-signer],[compile external signer support (default is yes, requires Boost::Process)])],
     [use_external_signer=$enableval],
-    [use_external_signer=no])
+    [use_external_signer=yes])
 
 AC_LANG_PUSH([C++])
 

src/Makefile.test.include

@@ -35,11 +35,12 @@ BITCOIN_TEST_SUITE = \
   $(TEST_UTIL_H)
 
 FUZZ_SUITE_LD_COMMON = \
+ $(LIBTEST_UTIL) \
+ $(LIBTEST_FUZZ) \
  $(LIBBITCOIN_SERVER) \
+ $(LIBBITCOIN_WALLET) \
  $(LIBBITCOIN_COMMON) \
  $(LIBBITCOIN_UTIL) \
- $(LIBTEST_UTIL) \
- $(LIBTEST_FUZZ) \
  $(LIBBITCOIN_CONSENSUS) \
  $(LIBBITCOIN_CRYPTO) \
  $(LIBBITCOIN_CLI) \
@@ -160,7 +161,6 @@ BITCOIN_TESTS += \
   wallet/test/scriptpubkeyman_tests.cpp
 
 FUZZ_SUITE_LD_COMMON +=\
- $(LIBBITCOIN_WALLET) \
  $(SQLITE_LIBS) \
  $(BDB_LIBS)
 

src/external_signer.cpp

@@ -13,9 +13,7 @@
 #include <string>
 #include <vector>
 
-#ifdef ENABLE_EXTERNAL_SIGNER
-
-ExternalSigner::ExternalSigner(const std::string& command, const std::string& fingerprint, const std::string chain, const std::string name): m_command(command), m_fingerprint(fingerprint), m_chain(chain), m_name(name) {}
+ExternalSigner::ExternalSigner(const std::string& command, const std::string chain, const std::string& fingerprint, const std::string name): m_command(command), m_chain(chain), m_fingerprint(fingerprint), m_name(name) {}
 
 const std::string ExternalSigner::NetworkArg() const
 {
@@ -55,7 +53,7 @@ bool ExternalSigner::Enumerate(const std::string& command, std::vector<ExternalS
         if (model_field.isStr() && model_field.getValStr() != "") {
             name += model_field.getValStr();
         }
-        signers.push_back(ExternalSigner(command, fingerprintStr, chain, name));
+        signers.push_back(ExternalSigner(command, chain, fingerprintStr, name));
     }
     return true;
 }
@@ -116,5 +114,3 @@ bool ExternalSigner::SignTransaction(PartiallySignedTransaction& psbtx, std::str
 
     return true;
 }
-
-#endif // ENABLE_EXTERNAL_SIGNER

src/external_signer.h

@@ -11,8 +11,6 @@
 #include <string>
 #include <vector>
 
-#ifdef ENABLE_EXTERNAL_SIGNER
-
 struct PartiallySignedTransaction;
 
 //! Enables interaction with an external signing device or service, such as
@@ -23,24 +21,24 @@ class ExternalSigner
     //! The command which handles interaction with the external signer.
     std::string m_command;
 
+    //! Bitcoin mainnet, testnet, etc
+    std::string m_chain;
+
+    const std::string NetworkArg() const;
+
 public:
     //! @param[in] command      the command which handles interaction with the external signer
     //! @param[in] fingerprint  master key fingerprint of the signer
     //! @param[in] chain        "main", "test", "regtest" or "signet"
     //! @param[in] name         device name
-    ExternalSigner(const std::string& command, const std::string& fingerprint, const std::string chain, const std::string name);
+    ExternalSigner(const std::string& command, const std::string chain, const std::string& fingerprint, const std::string name);
 
     //! Master key fingerprint of the signer
     std::string m_fingerprint;
 
-    //! Bitcoin mainnet, testnet, etc
-    std::string m_chain;
-
     //! Name of signer
     std::string m_name;
 
-    const std::string NetworkArg() const;
-
     //! Obtain a list of signers. Calls `<command> enumerate`.
     //! @param[in]              command the command which handles interaction with the external signer
     //! @param[in,out] signers  vector to which new signers (with a unique master key fingerprint) are added
@@ -65,6 +63,4 @@ class ExternalSigner
     bool SignTransaction(PartiallySignedTransaction& psbt, std::string& error);
 };
 
-#endif // ENABLE_EXTERNAL_SIGNER
-
 #endif // BITCOIN_EXTERNAL_SIGNER_H

src/interfaces/node.h

@@ -111,10 +111,8 @@ class Node
     //! Disconnect node by id.
     virtual bool disconnectById(NodeId id) = 0;
 
-#ifdef ENABLE_EXTERNAL_SIGNER
     //! List external signers
     virtual std::vector<ExternalSigner> externalSigners() = 0;
-#endif
 
     //! Get total bytes recv.
     virtual int64_t getTotalBytesRecv() = 0;

src/node/interfaces.cpp

@@ -6,6 +6,7 @@
 #include <banman.h>
 #include <chain.h>
 #include <chainparams.h>
+#include <external_signer.h>
 #include <init.h>
 #include <interfaces/chain.h>
 #include <interfaces/handler.h>
@@ -170,16 +171,24 @@ class NodeImpl : public Node
         }
         return false;
     }
-#ifdef ENABLE_EXTERNAL_SIGNER
     std::vector<ExternalSigner> externalSigners() override
     {
+#ifdef ENABLE_EXTERNAL_SIGNER
         std::vector<ExternalSigner> signers = {};
         const std::string command = gArgs.GetArg("-signer", "");
         if (command == "") return signers;
         ExternalSigner::Enumerate(command, signers, Params().NetworkIDString());
         return signers;
+#else
+        // This result is undistinguisable from a succesful call that returns
+        // no signers. For the current GUI this doesn't matter, because the wallet
+        // creation dialog disables the external signer checkbox in both
+        // cases. The return type could be changed to std::optional<std::vector>
+        // (or something that also includes error messages) if this distinction
+        // becomes important.
+        return {};
+#endif // ENABLE_EXTERNAL_SIGNER
     }
-#endif
     int64_t getTotalBytesRecv() override { return m_context->connman ? m_context->connman->GetTotalBytesRecv() : 0; }
     int64_t getTotalBytesSent() override { return m_context->connman ? m_context->connman->GetTotalBytesSent() : 0; }
     size_t getMempoolSize() override { return m_context->mempool ? m_context->mempool->size() : 0; }

src/qt/createwalletdialog.cpp

@@ -31,8 +31,9 @@ CreateWalletDialog::CreateWalletDialog(QWidget* parent) :
         // Disable the disable_privkeys_checkbox and external_signer_checkbox when isEncryptWalletChecked is
         // set to true, enable it when isEncryptWalletChecked is false.
         ui->disable_privkeys_checkbox->setEnabled(!checked);
+#ifdef ENABLE_EXTERNAL_SIGNER
         ui->external_signer_checkbox->setEnabled(!checked);
-
+#endif
         // When the disable_privkeys_checkbox is disabled, uncheck it.
         if (!ui->disable_privkeys_checkbox->isEnabled()) {
             ui->disable_privkeys_checkbox->setChecked(false);
@@ -112,8 +113,7 @@ CreateWalletDialog::~CreateWalletDialog()
     delete ui;
 }
 
-#ifdef ENABLE_EXTERNAL_SIGNER
-void CreateWalletDialog::setSigners(std::vector<ExternalSigner>& signers)
+void CreateWalletDialog::setSigners(const std::vector<ExternalSigner>& signers)
 {
     if (!signers.empty()) {
         ui->external_signer_checkbox->setEnabled(true);
@@ -132,7 +132,6 @@ void CreateWalletDialog::setSigners(std::vector<ExternalSigner>& signers)
         ui->external_signer_checkbox->setEnabled(false);
     }
 }
-#endif
 
 QString CreateWalletDialog::walletName() const
 {

src/qt/createwalletdialog.h

@@ -7,11 +7,8 @@
 
 #include <QDialog>
 
-class WalletModel;
-
-#ifdef ENABLE_EXTERNAL_SIGNER
 class ExternalSigner;
-#endif
+class WalletModel;
 
 namespace Ui {
     class CreateWalletDialog;
@@ -27,9 +24,7 @@ class CreateWalletDialog : public QDialog
     explicit CreateWalletDialog(QWidget* parent);
     virtual ~CreateWalletDialog();
 
-#ifdef ENABLE_EXTERNAL_SIGNER
-    void setSigners(std::vector<ExternalSigner>& signers);
-#endif
+    void setSigners(const std::vector<ExternalSigner>& signers);
 
     QString walletName() const;
     bool isEncryptWalletChecked() const;

src/qt/optionsdialog.cpp

@@ -92,6 +92,11 @@ OptionsDialog::OptionsDialog(QWidget *parent, bool enableWallet) :
         ui->thirdPartyTxUrls->setVisible(false);
     }
 
+#ifndef ENABLE_EXTERNAL_SIGNER
+    //: "External signing" means using devices such as hardware wallets.
+    ui->externalSignerPath->setToolTip(tr("Compiled without external signing support (required for external signing)"));
+    ui->externalSignerPath->setEnabled(false);
+#endif
     /* Display elements init */
     QDir translations(":translations");
 

src/qt/receiverequestdialog.cpp

@@ -90,7 +90,7 @@ void ReceiveRequestDialog::setInfo(const SendCoinsRecipient &_info)
         ui->wallet_content->hide();
     }
 
-    ui->btnVerify->setVisible(this->model->wallet().hasExternalSigner());
+    ui->btnVerify->setVisible(model->wallet().hasExternalSigner());
 
     connect(ui->btnVerify, &QPushButton::clicked, [this] {
         model->displayAddress(info.address.toStdString());

src/qt/sendcoinsdialog.cpp

@@ -200,12 +200,14 @@ void SendCoinsDialog::setModel(WalletModel *_model)
         ui->optInRBF->setCheckState(Qt::Checked);
 
         if (model->wallet().hasExternalSigner()) {
+            //: "device" usually means a hardware wallet
             ui->sendButton->setText(tr("Sign on device"));
             if (gArgs.GetArg("-signer", "") != "") {
                 ui->sendButton->setEnabled(true);
                 ui->sendButton->setToolTip(tr("Connect your hardware wallet first."));
             } else {
                 ui->sendButton->setEnabled(false);
+                //: "External signer" means using devices such as hardware wallets.
                 ui->sendButton->setToolTip(tr("Set external signer script path in Options -> Wallet"));
             }
         } else if (model->wallet().privateKeysDisabled()) {
@@ -426,11 +428,13 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked)
                 return;
             }
             if (err == TransactionError::EXTERNAL_SIGNER_NOT_FOUND) {
+                //: "External signer" means using devices such as hardware wallets.
                 QMessageBox::critical(nullptr, tr("External signer not found"), "External signer not found");
                 send_failure = true;
                 return;
             }
             if (err == TransactionError::EXTERNAL_SIGNER_FAILED) {
+                //: "External signer" means using devices such as hardware wallets.
                 QMessageBox::critical(nullptr, tr("External signer failure"), "External signer failure");
                 send_failure = true;
                 return;

src/qt/walletcontroller.cpp

@@ -11,6 +11,7 @@
 #include <qt/guiutil.h>
 #include <qt/walletmodel.h>
 
+#include <external_signer.h>
 #include <interfaces/handler.h>
 #include <interfaces/node.h>
 #include <util/string.h>
@@ -295,15 +296,13 @@ void CreateWalletActivity::create()
 {
     m_create_wallet_dialog = new CreateWalletDialog(m_parent_widget);
 
-#ifdef ENABLE_EXTERNAL_SIGNER
     std::vector<ExternalSigner> signers;
     try {
         signers = node().externalSigners();
     } catch (const std::runtime_error& e) {
         QMessageBox::critical(nullptr, tr("Can't list signers"), e.what());
     }
     m_create_wallet_dialog->setSigners(signers);
-#endif
 
     m_create_wallet_dialog->setWindowModality(Qt::ApplicationModal);
     m_create_wallet_dialog->show();

src/util/system.cpp

@@ -1243,9 +1243,9 @@ void runCommand(const std::string& strCommand)
 }
 #endif
 
-#ifdef ENABLE_EXTERNAL_SIGNER
 UniValue RunCommandParseJSON(const std::string& str_command, const std::string& str_std_in)
 {
+#ifdef ENABLE_EXTERNAL_SIGNER
     namespace bp = boost::process;
 
     UniValue result_json;
@@ -1277,8 +1277,10 @@ UniValue RunCommandParseJSON(const std::string& str_command, const std::string&
     if (!result_json.read(result)) throw std::runtime_error("Unable to parse JSON: " + result);
 
     return result_json;
-}
+#else
+    throw std::runtime_error("Compiled without external signing support (required for external signing).");
 #endif // ENABLE_EXTERNAL_SIGNER
+}
 
 void SetupEnvironment()
 {

src/util/system.h

@@ -102,7 +102,6 @@ std::string ShellEscape(const std::string& arg);
 #if HAVE_SYSTEM
 void runCommand(const std::string& strCommand);
 #endif
-#ifdef ENABLE_EXTERNAL_SIGNER
 /**
  * Execute a command which returns JSON, and parse the result.
  *
@@ -111,7 +110,6 @@ void runCommand(const std::string& strCommand);
  * @return parsed JSON
  */
 UniValue RunCommandParseJSON(const std::string& str_command, const std::string& str_std_in="");
-#endif // ENABLE_EXTERNAL_SIGNER
 
 /**
  * Most paths passed as configuration arguments are treated as relative to