From fc0eca31b33f87882e2aa329a3746d4e08af1985 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Tue, 15 Jun 2021 16:38:01 +0200 Subject: [PATCH 1/6] fuzz: fix fuzz binary linking order We encountered a linking error when attempting to include external_signer_scriptpubkeyman.cpp when configured with --disable-external-signer. Everywhere else we have LIBBITCOIN_WALLET, it is always before LIBBITCOIN_COMMON. But if you go up to where FUZZ_SUITE_LD_COMMON is first set, you see that we will end up having LIBBITCOIN_COMMON set before LIBBITCOIN_WALLET which means that the linker will have problems linking things common things that the wallet uses. Because the order is correct for the other targets, we only see a linker error for test/fuzz/fuzz. In this diff, LIBTEST_UTIL and LIBTEST_FUZZ are moved to the top because they include LIBBITCOIN_SERVER and LIBBITCOIN_COMMON. LIBBITCOIN_SERVER always needs to be the first item in the linker order since it has the most dependencies. The makefiles for making the fuzz and test binaries should be revisited so that the linking order is made consistent with the rest of the code and to avoid other linker order issues that may crop up in the future. Co-Authored-By: Andrew Chow --- src/Makefile.test.include | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 105d09f730..fc2fd80166 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -35,11 +35,12 @@ BITCOIN_TEST_SUITE = \ $(TEST_UTIL_H) FUZZ_SUITE_LD_COMMON = \ - $(LIBBITCOIN_SERVER) \ - $(LIBBITCOIN_COMMON) \ - $(LIBBITCOIN_UTIL) \ $(LIBTEST_UTIL) \ $(LIBTEST_FUZZ) \ + $(LIBBITCOIN_SERVER) \ + $(LIBBITCOIN_WALLET) \ + $(LIBBITCOIN_COMMON) \ + $(LIBBITCOIN_UTIL) \ $(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) From 7d9453041b827bafbdfc1ac0b01c7b7e1ee2bd4f Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Thu, 13 May 2021 16:19:44 +0200 Subject: [PATCH 2/6] refactor: clean up external_signer.h includes Co-Authored-By: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> --- src/node/interfaces.cpp | 1 + src/qt/walletcontroller.cpp | 1 + src/wallet/scriptpubkeyman.cpp | 1 - src/wallet/wallet.cpp | 1 + src/wallet/wallet.h | 1 - 5 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 2d05f9d5fb..37a5fe4b38 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include diff --git a/src/qt/walletcontroller.cpp b/src/qt/walletcontroller.cpp index 7e5790fd87..24f73a2ea0 100644 --- a/src/qt/walletcontroller.cpp +++ b/src/qt/walletcontroller.cpp @@ -11,6 +11,7 @@ #include #include +#include #include #include #include diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 2eb9ca5c6d..6af936b1ac 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -13,7 +13,6 @@ #include #include #include -#include #include #include diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 4b6630de3c..98fab26030 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index d0e26c416c..66f39edb4d 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -21,7 +21,6 @@ #include #include #include -#include #include #include #include From 5be90c907eba0a38019c7d9826623d5d5f567c66 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Tue, 16 Jun 2020 18:56:11 +0200 Subject: [PATCH 3/6] build: enable external signer by default --- ci/test/00_setup_env_arm.sh | 2 +- ci/test/00_setup_env_i686_centos.sh | 2 +- ci/test/00_setup_env_mac.sh | 2 +- ci/test/00_setup_env_mac_host.sh | 2 +- ci/test/00_setup_env_native_asan.sh | 2 +- ci/test/00_setup_env_native_fuzz.sh | 2 +- ci/test/00_setup_env_native_multiprocess.sh | 2 +- ci/test/00_setup_env_native_nowallet.sh | 2 +- ci/test/00_setup_env_native_qt5.sh | 2 +- ci/test/00_setup_env_native_tsan.sh | 2 +- ci/test/00_setup_env_s390x.sh | 2 +- configure.ac | 4 ++-- 12 files changed, 13 insertions(+), 13 deletions(-) diff --git a/ci/test/00_setup_env_arm.sh b/ci/test/00_setup_env_arm.sh index 07f099b85c..8d2b70e549 100755 --- a/ci/test/00_setup_env_arm.sh +++ b/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" diff --git a/ci/test/00_setup_env_i686_centos.sh b/ci/test/00_setup_env_i686_centos.sh index 05c724fc0b..2ddb932907 100755 --- a/ci/test/00_setup_env_i686_centos.sh +++ b/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" diff --git a/ci/test/00_setup_env_mac.sh b/ci/test/00_setup_env_mac.sh index 196394e908..73ac09c1de 100755 --- a/ci/test/00_setup_env_mac.sh +++ b/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" diff --git a/ci/test/00_setup_env_mac_host.sh b/ci/test/00_setup_env_mac_host.sh index 898c1530a1..c0d951a041 100755 --- a/ci/test/00_setup_env_mac_host.sh +++ b/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="" diff --git a/ci/test/00_setup_env_native_asan.sh b/ci/test/00_setup_env_native_asan.sh index 92af98aa9b..ab185b6e71 100755 --- a/ci/test/00_setup_env_native_asan.sh +++ b/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++" diff --git a/ci/test/00_setup_env_native_fuzz.sh b/ci/test/00_setup_env_native_fuzz.sh index bedd0cf9aa..58388fa928 100755 --- a/ci/test/00_setup_env_native_fuzz.sh +++ b/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 diff --git a/ci/test/00_setup_env_native_multiprocess.sh b/ci/test/00_setup_env_native_multiprocess.sh index 37d714400b..1418dfbc51 100755 --- a/ci/test/00_setup_env_native_multiprocess.sh +++ b/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" diff --git a/ci/test/00_setup_env_native_nowallet.sh b/ci/test/00_setup_env_native_nowallet.sh index a496b5af6e..d167c9198a 100755 --- a/ci/test/00_setup_env_native_nowallet.sh +++ b/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" diff --git a/ci/test/00_setup_env_native_qt5.sh b/ci/test/00_setup_env_native_qt5.sh index 61948ab221..9c57eba62a 100755 --- a/ci/test/00_setup_env_native_qt5.sh +++ b/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\"" diff --git a/ci/test/00_setup_env_native_tsan.sh b/ci/test/00_setup_env_native_tsan.sh index 33f63fa9ba..a5082bdaab 100755 --- a/ci/test/00_setup_env_native_tsan.sh +++ b/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++'" diff --git a/ci/test/00_setup_env_s390x.sh b/ci/test/00_setup_env_s390x.sh index 88b431f3c7..51a0fd9117 100755 --- a/ci/test/00_setup_env_s390x.sh +++ b/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" diff --git a/configure.ac b/configure.ac index aaa932b093..9d306a5445 100644 --- a/configure.ac +++ b/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++]) From 4455145e266450397b45acd7286686966edd072b Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Thu, 13 May 2021 15:23:19 +0200 Subject: [PATCH 4/6] refactor: reduce #ifdef ENABLE_EXTERNAL_SIGNER usage In particular this make the node interface independent on whether external signer support is compiled. --- src/external_signer.cpp | 4 ---- src/external_signer.h | 4 ---- src/interfaces/node.h | 2 -- src/node/interfaces.cpp | 12 ++++++++++-- src/qt/createwalletdialog.cpp | 2 -- src/qt/createwalletdialog.h | 7 +------ src/qt/walletcontroller.cpp | 2 -- src/util/system.cpp | 6 ++++-- src/util/system.h | 2 -- src/wallet/external_signer_scriptpubkeyman.cpp | 4 ---- src/wallet/external_signer_scriptpubkeyman.h | 3 --- src/wallet/wallet.cpp | 12 ------------ 12 files changed, 15 insertions(+), 45 deletions(-) diff --git a/src/external_signer.cpp b/src/external_signer.cpp index f16d21fa60..b2bd7cb261 100644 --- a/src/external_signer.cpp +++ b/src/external_signer.cpp @@ -13,8 +13,6 @@ #include #include -#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) {} const std::string ExternalSigner::NetworkArg() const @@ -116,5 +114,3 @@ bool ExternalSigner::SignTransaction(PartiallySignedTransaction& psbtx, std::str return true; } - -#endif // ENABLE_EXTERNAL_SIGNER diff --git a/src/external_signer.h b/src/external_signer.h index b3b202091a..c4f723a1bb 100644 --- a/src/external_signer.h +++ b/src/external_signer.h @@ -11,8 +11,6 @@ #include #include -#ifdef ENABLE_EXTERNAL_SIGNER - struct PartiallySignedTransaction; //! Enables interaction with an external signing device or service, such as @@ -65,6 +63,4 @@ public: bool SignTransaction(PartiallySignedTransaction& psbt, std::string& error); }; -#endif // ENABLE_EXTERNAL_SIGNER - #endif // BITCOIN_EXTERNAL_SIGNER_H diff --git a/src/interfaces/node.h b/src/interfaces/node.h index 35b6160cea..77129423db 100644 --- a/src/interfaces/node.h +++ b/src/interfaces/node.h @@ -111,10 +111,8 @@ public: //! Disconnect node by id. virtual bool disconnectById(NodeId id) = 0; -#ifdef ENABLE_EXTERNAL_SIGNER //! List external signers virtual std::vector externalSigners() = 0; -#endif //! Get total bytes recv. virtual int64_t getTotalBytesRecv() = 0; diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 37a5fe4b38..fce3c1809c 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -171,16 +171,24 @@ public: } return false; } -#ifdef ENABLE_EXTERNAL_SIGNER std::vector externalSigners() override { +#ifdef ENABLE_EXTERNAL_SIGNER std::vector 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 + // (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; } diff --git a/src/qt/createwalletdialog.cpp b/src/qt/createwalletdialog.cpp index e593697b46..cea41a94ae 100644 --- a/src/qt/createwalletdialog.cpp +++ b/src/qt/createwalletdialog.cpp @@ -112,7 +112,6 @@ CreateWalletDialog::~CreateWalletDialog() delete ui; } -#ifdef ENABLE_EXTERNAL_SIGNER void CreateWalletDialog::setSigners(std::vector& signers) { if (!signers.empty()) { @@ -132,7 +131,6 @@ void CreateWalletDialog::setSigners(std::vector& signers) ui->external_signer_checkbox->setEnabled(false); } } -#endif QString CreateWalletDialog::walletName() const { diff --git a/src/qt/createwalletdialog.h b/src/qt/createwalletdialog.h index 585b1461f7..3f6a32548e 100644 --- a/src/qt/createwalletdialog.h +++ b/src/qt/createwalletdialog.h @@ -7,11 +7,8 @@ #include -class WalletModel; - -#ifdef ENABLE_EXTERNAL_SIGNER class ExternalSigner; -#endif +class WalletModel; namespace Ui { class CreateWalletDialog; @@ -27,9 +24,7 @@ public: explicit CreateWalletDialog(QWidget* parent); virtual ~CreateWalletDialog(); -#ifdef ENABLE_EXTERNAL_SIGNER void setSigners(std::vector& signers); -#endif QString walletName() const; bool isEncryptWalletChecked() const; diff --git a/src/qt/walletcontroller.cpp b/src/qt/walletcontroller.cpp index 24f73a2ea0..3cceb5ca5a 100644 --- a/src/qt/walletcontroller.cpp +++ b/src/qt/walletcontroller.cpp @@ -296,7 +296,6 @@ void CreateWalletActivity::create() { m_create_wallet_dialog = new CreateWalletDialog(m_parent_widget); -#ifdef ENABLE_EXTERNAL_SIGNER std::vector signers; try { signers = node().externalSigners(); @@ -304,7 +303,6 @@ void CreateWalletActivity::create() 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(); diff --git a/src/util/system.cpp b/src/util/system.cpp index 13ccf7463e..5ea0139275 100644 --- a/src/util/system.cpp +++ b/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() { diff --git a/src/util/system.h b/src/util/system.h index c4317c62d0..ea9870a343 100644 --- a/src/util/system.h +++ b/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 diff --git a/src/wallet/external_signer_scriptpubkeyman.cpp b/src/wallet/external_signer_scriptpubkeyman.cpp index fe2c810afa..e5464c5b89 100644 --- a/src/wallet/external_signer_scriptpubkeyman.cpp +++ b/src/wallet/external_signer_scriptpubkeyman.cpp @@ -13,8 +13,6 @@ #include #include -#ifdef ENABLE_EXTERNAL_SIGNER - bool ExternalSignerScriptPubKeyMan::SetupDescriptor(std::unique_ptr desc) { LOCK(cs_desc_man); @@ -84,5 +82,3 @@ TransactionError ExternalSignerScriptPubKeyMan::FillPSBT(PartiallySignedTransact FinalizePSBT(psbt); // This won't work in a multisig setup return TransactionError::OK; } - -#endif diff --git a/src/wallet/external_signer_scriptpubkeyman.h b/src/wallet/external_signer_scriptpubkeyman.h index 1786958912..5df1e6d939 100644 --- a/src/wallet/external_signer_scriptpubkeyman.h +++ b/src/wallet/external_signer_scriptpubkeyman.h @@ -5,7 +5,6 @@ #ifndef BITCOIN_WALLET_EXTERNAL_SIGNER_SCRIPTPUBKEYMAN_H #define BITCOIN_WALLET_EXTERNAL_SIGNER_SCRIPTPUBKEYMAN_H -#ifdef ENABLE_EXTERNAL_SIGNER #include #include @@ -31,6 +30,4 @@ class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan TransactionError FillPSBT(PartiallySignedTransaction& psbt, int sighash_type = 1 /* SIGHASH_ALL */, bool sign = true, bool bip32derivs = false, int* n_signed = nullptr) const override; }; -#endif - #endif // BITCOIN_WALLET_EXTERNAL_SIGNER_SCRIPTPUBKEYMAN_H diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 98fab26030..073d5e269b 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2216,7 +2216,6 @@ void ReserveDestination::ReturnDestination() bool CWallet::DisplayAddress(const CTxDestination& dest) { -#ifdef ENABLE_EXTERNAL_SIGNER CScript scriptPubKey = GetScriptForDestination(dest); const auto spk_man = GetScriptPubKeyMan(scriptPubKey); if (spk_man == nullptr) { @@ -2228,9 +2227,6 @@ bool CWallet::DisplayAddress(const CTxDestination& dest) } ExternalSigner signer = ExternalSignerScriptPubKeyMan::GetExternalSigner(); return signer_spk_man->DisplayAddress(scriptPubKey, signer); -#else - return false; -#endif } void CWallet::LockCoin(const COutPoint& output) @@ -3064,12 +3060,8 @@ void CWallet::ConnectScriptPubKeyManNotifiers() void CWallet::LoadDescriptorScriptPubKeyMan(uint256 id, WalletDescriptor& desc) { if (IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) { -#ifdef ENABLE_EXTERNAL_SIGNER auto spk_manager = std::unique_ptr(new ExternalSignerScriptPubKeyMan(*this, desc)); m_spk_managers[id] = std::move(spk_manager); -#else - throw std::runtime_error(std::string(__func__) + ": Compiled without external signing support (required for external signing)"); -#endif } else { auto spk_manager = std::unique_ptr(new DescriptorScriptPubKeyMan(*this, desc)); m_spk_managers[id] = std::move(spk_manager); @@ -3109,7 +3101,6 @@ void CWallet::SetupDescriptorScriptPubKeyMans() } } } else { -#ifdef ENABLE_EXTERNAL_SIGNER ExternalSigner signer = ExternalSignerScriptPubKeyMan::GetExternalSigner(); // TODO: add account parameter @@ -3136,9 +3127,6 @@ void CWallet::SetupDescriptorScriptPubKeyMans() AddActiveScriptPubKeyMan(id, t, internal); } } -#else - throw std::runtime_error(std::string(__func__) + ": Compiled without external signing support (required for external signing)"); -#endif // ENABLE_EXTERNAL_SIGNER } } From d672404466204444a1d9f2d3498de4448f53d2be Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Tue, 15 Jun 2021 16:58:55 +0200 Subject: [PATCH 5/6] refactor: make ExternalSigner NetworkArg() and m_chain private --- src/external_signer.cpp | 4 ++-- src/external_signer.h | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/external_signer.cpp b/src/external_signer.cpp index b2bd7cb261..d6388b759a 100644 --- a/src/external_signer.cpp +++ b/src/external_signer.cpp @@ -13,7 +13,7 @@ #include #include -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 { @@ -53,7 +53,7 @@ bool ExternalSigner::Enumerate(const std::string& command, std::vector 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 From 2f5bdcbc31a2eeb7c11226a9e51c56f02ac807dd Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Tue, 15 Jun 2021 19:11:09 +0200 Subject: [PATCH 6/6] gui: misc external signer fixes and translation hints --- src/qt/createwalletdialog.cpp | 5 +++-- src/qt/createwalletdialog.h | 2 +- src/qt/optionsdialog.cpp | 5 +++++ src/qt/receiverequestdialog.cpp | 2 +- src/qt/sendcoinsdialog.cpp | 4 ++++ 5 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/qt/createwalletdialog.cpp b/src/qt/createwalletdialog.cpp index cea41a94ae..dc24bbc6a6 100644 --- a/src/qt/createwalletdialog.cpp +++ b/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,7 +113,7 @@ CreateWalletDialog::~CreateWalletDialog() delete ui; } -void CreateWalletDialog::setSigners(std::vector& signers) +void CreateWalletDialog::setSigners(const std::vector& signers) { if (!signers.empty()) { ui->external_signer_checkbox->setEnabled(true); diff --git a/src/qt/createwalletdialog.h b/src/qt/createwalletdialog.h index 3f6a32548e..25ddf97585 100644 --- a/src/qt/createwalletdialog.h +++ b/src/qt/createwalletdialog.h @@ -24,7 +24,7 @@ public: explicit CreateWalletDialog(QWidget* parent); virtual ~CreateWalletDialog(); - void setSigners(std::vector& signers); + void setSigners(const std::vector& signers); QString walletName() const; bool isEncryptWalletChecked() const; diff --git a/src/qt/optionsdialog.cpp b/src/qt/optionsdialog.cpp index 6ad8db4348..b12fe96567 100644 --- a/src/qt/optionsdialog.cpp +++ b/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"); diff --git a/src/qt/receiverequestdialog.cpp b/src/qt/receiverequestdialog.cpp index abe7de8f89..41f22e9c34 100644 --- a/src/qt/receiverequestdialog.cpp +++ b/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()); diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp index 6a5ec435cd..c9bf757dfc 100644 --- a/src/qt/sendcoinsdialog.cpp +++ b/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;