From 87a97941f667483bbf2ab00929e03a2199cb8a62 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Wed, 30 Oct 2019 20:08:23 +0100 Subject: [PATCH 01/15] configure: add --enable-external-signer This option replaces --with-boost-process This prepares external signer support to be disabled by default. It adds a configure option to enable this feature and to check if Boost::Process is present. This also exposes ENABLE_EXTERNAL_SIGNER to the test suite via test/config.ini --- build_msvc/bitcoin_config.h | 4 +- configure.ac | 82 ++++++++++++++++++++----------------- doc/Doxyfile.in | 2 +- src/test/system_tests.cpp | 10 ++--- src/util/system.cpp | 8 ++-- src/util/system.h | 4 +- test/config.ini.in | 1 + 7 files changed, 59 insertions(+), 52 deletions(-) diff --git a/build_msvc/bitcoin_config.h b/build_msvc/bitcoin_config.h index 23c554e39..40a30b974 100644 --- a/build_msvc/bitcoin_config.h +++ b/build_msvc/bitcoin_config.h @@ -50,8 +50,8 @@ /* define if the Boost::Filesystem library is available */ #define HAVE_BOOST_FILESYSTEM /**/ -/* define if the Boost::Process library is available */ -#define HAVE_BOOST_PROCESS /**/ +/* define if external signer support is enabled (requires Boost::Process) */ +#define ENABLE_EXTERNAL_SIGNER /**/ /* define if the Boost::System library is available */ #define HAVE_BOOST_SYSTEM /**/ diff --git a/configure.ac b/configure.ac index c0e3b32e3..bee584ca6 100644 --- a/configure.ac +++ b/configure.ac @@ -338,10 +338,10 @@ AC_ARG_ENABLE([werror], [enable_werror=$enableval], [enable_werror=no]) -AC_ARG_WITH([boost-process], - [AS_HELP_STRING([--with-boost-process],[Opt in to using Boost Process (default is no)])], - [boost_process=$withval], - [boost_process=no]) +AC_ARG_ENABLE([external-signer], + [AS_HELP_STRING([--enable-external-signer],[compile external signer support (default is no, requires Boost::Process)])], + [use_external_signer=$enableval], + [use_external_signer=no]) AC_LANG_PUSH([C++]) @@ -1253,6 +1253,7 @@ if test "x$enable_fuzz" = "xyes"; then bitcoin_enable_qt_dbus=no enable_wallet=no use_bench=no + use_external_signer=no use_upnp=no use_natpmp=no use_zmq=no @@ -1390,16 +1391,20 @@ fi AX_BOOST_SYSTEM AX_BOOST_FILESYSTEM -dnl Opt-in to Boost Process -if test "x$boost_process" != xno; then +dnl Opt-in to Boost Process if external signer support is requested +if test "x$use_external_signer" != xno; then AC_MSG_CHECKING(for Boost Process) AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include ]], [[ boost::process::child* child = new boost::process::child; delete child; ]])], - [ AC_MSG_RESULT(yes); AC_DEFINE([HAVE_BOOST_PROCESS],,[define if Boost::Process is available])], - [ AC_MSG_ERROR([Boost::Process is not available!])] + [ AC_MSG_RESULT(yes) + AC_DEFINE([ENABLE_EXTERNAL_SIGNER],,[define if external signer support is enabled]) + ], + [ AC_MSG_ERROR([Boost::Process is required for external signer support, but not available!])] ) fi +AM_CONDITIONAL([ENABLE_EXTERNAL_SIGNER], [test "x$use_external_signer" = "xyes"]) + if test x$suppress_external_warnings != xno; then BOOST_CPPFLAGS=SUPPRESS_WARNINGS($BOOST_CPPFLAGS) fi @@ -1810,6 +1815,7 @@ AC_SUBST(ARM_CRC_CXXFLAGS) AC_SUBST(LIBTOOL_APP_LDFLAGS) AC_SUBST(USE_SQLITE) AC_SUBST(USE_BDB) +AC_SUBST(ENABLE_EXTERNAL_SIGNER) AC_SUBST(USE_UPNP) AC_SUBST(USE_QRCODE) AC_SUBST(BOOST_LIBS) @@ -1885,43 +1891,43 @@ esac echo echo "Options used to compile and link:" -echo " boost process = $with_boost_process" -echo " multiprocess = $build_multiprocess" -echo " with libs = $build_bitcoin_libs" -echo " with wallet = $enable_wallet" +echo " external signer = $use_external_signer" +echo " multiprocess = $build_multiprocess" +echo " with libs = $build_bitcoin_libs" +echo " with wallet = $enable_wallet" if test "x$enable_wallet" != "xno"; then - echo " with sqlite = $use_sqlite" - echo " with bdb = $use_bdb" + echo " with sqlite = $use_sqlite" + echo " with bdb = $use_bdb" fi -echo " with gui / qt = $bitcoin_enable_qt" +echo " with gui / qt = $bitcoin_enable_qt" if test x$bitcoin_enable_qt != xno; then - echo " with qr = $use_qr" + echo " with qr = $use_qr" fi -echo " with zmq = $use_zmq" +echo " with zmq = $use_zmq" if test x$enable_fuzz == xno; then - echo " with test = $use_tests" + echo " with test = $use_tests" else - echo " with test = not building test_bitcoin because fuzzing is enabled" - echo " with fuzz = $enable_fuzz" + echo " with test = not building test_bitcoin because fuzzing is enabled" + echo " with fuzz = $enable_fuzz" fi -echo " with bench = $use_bench" -echo " with upnp = $use_upnp" -echo " with natpmp = $use_natpmp" -echo " use asm = $use_asm" -echo " ebpf tracing = $have_sdt" -echo " sanitizers = $use_sanitizers" -echo " debug enabled = $enable_debug" -echo " gprof enabled = $enable_gprof" -echo " werror = $enable_werror" +echo " with bench = $use_bench" +echo " with upnp = $use_upnp" +echo " with natpmp = $use_natpmp" +echo " use asm = $use_asm" +echo " ebpf tracing = $have_sdt" +echo " sanitizers = $use_sanitizers" +echo " debug enabled = $enable_debug" +echo " gprof enabled = $enable_gprof" +echo " werror = $enable_werror" echo -echo " target os = $TARGET_OS" -echo " build os = $build_os" +echo " target os = $TARGET_OS" +echo " build os = $build_os" echo -echo " CC = $CC" -echo " CFLAGS = $PTHREAD_CFLAGS $CFLAGS" -echo " CPPFLAGS = $DEBUG_CPPFLAGS $HARDENED_CPPFLAGS $CPPFLAGS" -echo " CXX = $CXX" -echo " CXXFLAGS = $DEBUG_CXXFLAGS $HARDENED_CXXFLAGS $WARN_CXXFLAGS $NOWARN_CXXFLAGS $ERROR_CXXFLAGS $GPROF_CXXFLAGS $CXXFLAGS" -echo " LDFLAGS = $PTHREAD_LIBS $HARDENED_LDFLAGS $GPROF_LDFLAGS $LDFLAGS" -echo " ARFLAGS = $ARFLAGS" +echo " CC = $CC" +echo " CFLAGS = $PTHREAD_CFLAGS $CFLAGS" +echo " CPPFLAGS = $DEBUG_CPPFLAGS $HARDENED_CPPFLAGS $CPPFLAGS" +echo " CXX = $CXX" +echo " CXXFLAGS = $DEBUG_CXXFLAGS $HARDENED_CXXFLAGS $WARN_CXXFLAGS $NOWARN_CXXFLAGS $ERROR_CXXFLAGS $GPROF_CXXFLAGS $CXXFLAGS" +echo " LDFLAGS = $PTHREAD_LIBS $HARDENED_LDFLAGS $GPROF_LDFLAGS $LDFLAGS" +echo " ARFLAGS = $ARFLAGS" echo diff --git a/doc/Doxyfile.in b/doc/Doxyfile.in index 2f7916821..21bf587ea 100644 --- a/doc/Doxyfile.in +++ b/doc/Doxyfile.in @@ -2073,7 +2073,7 @@ INCLUDE_FILE_PATTERNS = # recursively expanded use the := operator instead of the = operator. # This tag requires that the tag ENABLE_PREPROCESSING is set to YES. -PREDEFINED = HAVE_BOOST_PROCESS +PREDEFINED = ENABLE_EXTERNAL_SIGNER # If the MACRO_EXPANSION and EXPAND_ONLY_PREDEF tags are set to YES then this # tag can be used to specify a list of macro names that should be expanded. The diff --git a/src/test/system_tests.cpp b/src/test/system_tests.cpp index ce555f729..940145b84 100644 --- a/src/test/system_tests.cpp +++ b/src/test/system_tests.cpp @@ -6,22 +6,22 @@ #include #include -#ifdef HAVE_BOOST_PROCESS +#ifdef ENABLE_EXTERNAL_SIGNER #include -#endif // HAVE_BOOST_PROCESS +#endif // ENABLE_EXTERNAL_SIGNER #include BOOST_FIXTURE_TEST_SUITE(system_tests, BasicTestingSetup) -// At least one test is required (in case HAVE_BOOST_PROCESS is not defined). +// At least one test is required (in case ENABLE_EXTERNAL_SIGNER is not defined). // Workaround for https://github.com/bitcoin/bitcoin/issues/19128 BOOST_AUTO_TEST_CASE(dummy) { BOOST_CHECK(true); } -#ifdef HAVE_BOOST_PROCESS +#ifdef ENABLE_EXTERNAL_SIGNER bool checkMessage(const std::runtime_error& ex) { @@ -90,6 +90,6 @@ BOOST_AUTO_TEST_CASE(run_command) } #endif } -#endif // HAVE_BOOST_PROCESS +#endif // ENABLE_EXTERNAL_SIGNER BOOST_AUTO_TEST_SUITE_END() diff --git a/src/util/system.cpp b/src/util/system.cpp index 9a2e719bb..71453eed8 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -5,9 +5,9 @@ #include -#ifdef HAVE_BOOST_PROCESS +#ifdef ENABLE_EXTERNAL_SIGNER #include -#endif // HAVE_BOOST_PROCESS +#endif // ENABLE_EXTERNAL_SIGNER #include #include @@ -1247,7 +1247,7 @@ void runCommand(const std::string& strCommand) } #endif -#ifdef HAVE_BOOST_PROCESS +#ifdef ENABLE_EXTERNAL_SIGNER UniValue RunCommandParseJSON(const std::string& str_command, const std::string& str_std_in) { namespace bp = boost::process; @@ -1282,7 +1282,7 @@ UniValue RunCommandParseJSON(const std::string& str_command, const std::string& return result_json; } -#endif // HAVE_BOOST_PROCESS +#endif // ENABLE_EXTERNAL_SIGNER void SetupEnvironment() { diff --git a/src/util/system.h b/src/util/system.h index 5959bc419..de47b93b6 100644 --- a/src/util/system.h +++ b/src/util/system.h @@ -108,7 +108,7 @@ std::string ShellEscape(const std::string& arg); #if HAVE_SYSTEM void runCommand(const std::string& strCommand); #endif -#ifdef HAVE_BOOST_PROCESS +#ifdef ENABLE_EXTERNAL_SIGNER /** * Execute a command which returns JSON, and parse the result. * @@ -117,7 +117,7 @@ void runCommand(const std::string& strCommand); * @return parsed JSON */ UniValue RunCommandParseJSON(const std::string& str_command, const std::string& str_std_in=""); -#endif // HAVE_BOOST_PROCESS +#endif // ENABLE_EXTERNAL_SIGNER /** * Most paths passed as configuration arguments are treated as relative to diff --git a/test/config.ini.in b/test/config.ini.in index 77c9a720c..e3872181c 100644 --- a/test/config.ini.in +++ b/test/config.ini.in @@ -23,3 +23,4 @@ RPCAUTH=@abs_top_srcdir@/share/rpcauth/rpcauth.py @BUILD_BITCOIND_TRUE@ENABLE_BITCOIND=true @ENABLE_FUZZ_TRUE@ENABLE_FUZZ=true @ENABLE_ZMQ_TRUE@ENABLE_ZMQ=true +@ENABLE_EXTERNAL_SIGNER_TRUE@ENABLE_EXTERNAL_SIGNER=true From f7eb7ecc6750ab267a979d9268ce5b5d151c26de Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Wed, 13 Feb 2019 18:59:16 +0100 Subject: [PATCH 02/15] test: framework: add skip_if_no_external_signer --- test/functional/test_framework/test_framework.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index 70a979844..f7eaaa548 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -827,10 +827,19 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): self.options.previous_releases_path)) return self.options.prev_releases + def skip_if_no_external_signer(self): + """Skip the running test if external signer support has not been compiled.""" + if not self.is_external_signer_compiled(): + raise SkipTest("external signer support has not been compiled.") + def is_cli_compiled(self): """Checks whether bitcoin-cli was compiled.""" return self.config["components"].getboolean("ENABLE_CLI") + def is_external_signer_compiled(self): + """Checks whether external signer support was compiled.""" + return self.config["components"].getboolean("ENABLE_EXTERNAL_SIGNER") + def is_wallet_compiled(self): """Checks whether the wallet module was compiled.""" return self.config["components"].getboolean("ENABLE_WALLET") From 8cf543f96dcd6fdfac1367b9e2b1d7d51be8bb76 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Sat, 27 Apr 2019 19:10:35 +0200 Subject: [PATCH 03/15] wallet: add -signer argument for external signer command Create basic ExternalSigner class with contructor. A Signer() is added to CWallet on load if -signer= is set. --- src/Makefile.am | 2 ++ src/dummywallet.cpp | 1 + src/wallet/external_signer.cpp | 8 ++++++++ src/wallet/external_signer.h | 34 ++++++++++++++++++++++++++++++++++ src/wallet/init.cpp | 3 +++ src/wallet/wallet.h | 2 +- 6 files changed, 49 insertions(+), 1 deletion(-) create mode 100644 src/wallet/external_signer.cpp create mode 100644 src/wallet/external_signer.h diff --git a/src/Makefile.am b/src/Makefile.am index 67efbbeae..3b81056d2 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -265,6 +265,7 @@ BITCOIN_CORE_H = \ wallet/crypter.h \ wallet/db.h \ wallet/dump.h \ + wallet/external_signer.h \ wallet/feebumper.h \ wallet/fees.h \ wallet/ismine.h \ @@ -379,6 +380,7 @@ libbitcoin_wallet_a_SOURCES = \ wallet/crypter.cpp \ wallet/db.cpp \ wallet/dump.cpp \ + wallet/external_signer.cpp \ wallet/feebumper.cpp \ wallet/fees.cpp \ wallet/interfaces.cpp \ diff --git a/src/dummywallet.cpp b/src/dummywallet.cpp index 4543f098a..bb06c95e7 100644 --- a/src/dummywallet.cpp +++ b/src/dummywallet.cpp @@ -38,6 +38,7 @@ void DummyWalletInit::AddWalletOptions(ArgsManager& argsman) const "-paytxfee=", "-rescan", "-salvagewallet", + "-signer=", "-spendzeroconfchange", "-txconfirmtarget=", "-wallet=", diff --git a/src/wallet/external_signer.cpp b/src/wallet/external_signer.cpp new file mode 100644 index 000000000..6f850e477 --- /dev/null +++ b/src/wallet/external_signer.cpp @@ -0,0 +1,8 @@ +// Copyright (c) 2018-2021 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include + +ExternalSigner::ExternalSigner(const std::string& command, const std::string& fingerprint): m_command(command), m_fingerprint(fingerprint) {} diff --git a/src/wallet/external_signer.h b/src/wallet/external_signer.h new file mode 100644 index 000000000..08fb0c9f3 --- /dev/null +++ b/src/wallet/external_signer.h @@ -0,0 +1,34 @@ +// Copyright (c) 2018-2021 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_WALLET_EXTERNAL_SIGNER_H +#define BITCOIN_WALLET_EXTERNAL_SIGNER_H + +#include +#include +#include + +class ExternalSignerException : public std::runtime_error { +public: + using std::runtime_error::runtime_error; +}; + +//! Enables interaction with an external signing device or service, such as +//! a hardware wallet. See doc/external-signer.md +class ExternalSigner +{ +private: + //! The command which handles interaction with the external signer. + std::string m_command; + +public: + //! @param[in] command the command which handles interaction with the external signer + //! @param[in] fingerprint master key fingerprint of the signer + ExternalSigner(const std::string& command, const std::string& fingerprint); + + //! Master key fingerprint of the signer + std::string m_fingerprint; +}; + +#endif // BITCOIN_WALLET_EXTERNAL_SIGNER_H diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp index 0d2be64df..fc530ee28 100644 --- a/src/wallet/init.cpp +++ b/src/wallet/init.cpp @@ -61,6 +61,9 @@ void WalletInit::AddWalletOptions(ArgsManager& argsman) const argsman.AddArg("-paytxfee=", strprintf("Fee (in %s/kB) to add to transactions you send (default: %s)", CURRENCY_UNIT, FormatMoney(CFeeRate{DEFAULT_PAY_TX_FEE}.GetFeePerK())), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET); argsman.AddArg("-rescan", "Rescan the block chain for missing wallet transactions on startup", ArgsManager::ALLOW_ANY, OptionsCategory::WALLET); +#ifdef ENABLE_EXTERNAL_SIGNER + argsman.AddArg("-signer=", "External signing tool, see docs/external-signer.md", ArgsManager::ALLOW_ANY, OptionsCategory::WALLET); +#endif argsman.AddArg("-spendzeroconfchange", strprintf("Spend unconfirmed change when sending transactions (default: %u)", DEFAULT_SPEND_ZEROCONF_CHANGE), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET); argsman.AddArg("-txconfirmtarget=", strprintf("If paytxfee is not set, include enough fee so transactions begin confirmation on average within n blocks (default: %u)", DEFAULT_TX_CONFIRM_TARGET), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET); argsman.AddArg("-wallet=", "Specify wallet path to load at startup. Can be used multiple times to load multiple wallets. Path is to a directory containing wallet data and log files. If the path is not absolute, it is interpreted relative to . This only loads existing wallets and does not create new ones. For backwards compatibility this also accepts names of existing top-level data files in .", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::WALLET); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 4fc446660..2d25f46b6 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -22,6 +22,7 @@ #include #include #include +#include #include #include @@ -95,7 +96,6 @@ constexpr CAmount DEFAULT_TRANSACTION_MAXFEE{COIN / 10}; constexpr CAmount HIGH_TX_FEE_PER_KB{COIN / 100}; //! -maxtxfee will warn if called with a higher fee than this amount (in satoshis) constexpr CAmount HIGH_MAX_TX_FEE{100 * HIGH_TX_FEE_PER_KB}; - //! Pre-calculated constants for input size estimation in *virtual size* static constexpr size_t DUMMY_NESTED_P2WPKH_INPUT_SIZE = 91; From f3e6ce78fba2b31173fe7b606aa9edb5b615bff3 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Sat, 27 Apr 2019 19:11:43 +0200 Subject: [PATCH 04/15] test: add external signer test Includes a mock to mimick the HWI interace. --- test/functional/mocks/signer.py | 28 +++++++++++++++++ test/functional/test_runner.py | 1 + test/functional/wallet_signer.py | 52 ++++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+) create mode 100755 test/functional/mocks/signer.py create mode 100755 test/functional/wallet_signer.py diff --git a/test/functional/mocks/signer.py b/test/functional/mocks/signer.py new file mode 100755 index 000000000..6b5c90388 --- /dev/null +++ b/test/functional/mocks/signer.py @@ -0,0 +1,28 @@ +#!/usr/bin/env python3 +# Copyright (c) 2018 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. + +import os +import sys +import argparse +import json + +def perform_pre_checks(): + mock_result_path = os.path.join(os.getcwd(), "mock_result") + if(os.path.isfile(mock_result_path)): + with open(mock_result_path, "r", encoding="utf8") as f: + mock_result = f.read() + if mock_result[0]: + sys.stdout.write(mock_result[2:]) + sys.exit(int(mock_result[0])) + +parser = argparse.ArgumentParser(prog='./signer.py', description='External signer mock') +subparsers = parser.add_subparsers(description='Commands', dest='command') +subparsers.required = True + +args = parser.parse_args() + +perform_pre_checks() + +args.func(args) diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index d742ef4ee..79ad2cf16 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -111,6 +111,7 @@ BASE_SCRIPTS = [ 'wallet_listtransactions.py --legacy-wallet', 'wallet_listtransactions.py --descriptors', 'feature_taproot.py', + 'wallet_signer.py --descriptors', # vv Tests less than 60s vv 'p2p_sendheaders.py', 'wallet_importmulti.py --legacy-wallet', diff --git a/test/functional/wallet_signer.py b/test/functional/wallet_signer.py new file mode 100755 index 000000000..62d4db837 --- /dev/null +++ b/test/functional/wallet_signer.py @@ -0,0 +1,52 @@ +#!/usr/bin/env python3 +# Copyright (c) 2017-2018 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. +"""Test external signer. + +Verify that a bitcoind node can use an external signer command +""" +import os +import platform + +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import ( + assert_equal, + assert_raises_rpc_error, +) + + +class SignerTest(BitcoinTestFramework): + def mock_signer_path(self): + path = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'mocks', 'signer.py') + if platform.system() == "Windows": + return "py " + path + else: + return path + + def set_test_params(self): + self.num_nodes = 3 + + self.extra_args = [ + [], + [f"-signer={self.mock_signer_path()}", '-keypool=10'], + [f"-signer={self.mock_signer_path()}", '-keypool=10'], + ["-signer=fake.py"], + ] + + def skip_test_if_missing_module(self): + self.skip_if_no_wallet() + self.skip_if_no_external_signer() + + def set_mock_result(self, node, res): + with open(os.path.join(node.cwd, "mock_result"), "w", encoding="utf8") as f: + f.write(res) + + def clear_mock_result(self, node): + os.remove(os.path.join(node.cwd, "mock_result")) + + def run_test(self): + self.log.debug(f"-signer={self.mock_signer_path()}") + +if __name__ == '__main__': + SignerTest().main() From 157ea7c614950d61bfe405310e2aaabcee31f7a3 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Sun, 4 Aug 2019 17:55:31 +0200 Subject: [PATCH 05/15] wallet: add external_signer flag --- src/wallet/wallet.cpp | 14 ++++++++++++++ src/wallet/wallet.h | 4 +++- src/wallet/walletutil.h | 3 +++ 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index db80745db..8a49383af 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -259,6 +259,20 @@ std::shared_ptr CreateWallet(interfaces::Chain& chain, const std::strin wallet_creation_flags |= WALLET_FLAG_BLANK_WALLET; } + // Private keys must be disabled for an external signer wallet + if ((wallet_creation_flags & WALLET_FLAG_EXTERNAL_SIGNER) && !(wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { + error = Untranslated("Private keys must be disabled when using an external signer"); + status = DatabaseStatus::FAILED_CREATE; + return nullptr; + } + + // Descriptor support must be enabled for an external signer wallet + if ((wallet_creation_flags & WALLET_FLAG_EXTERNAL_SIGNER) && !(wallet_creation_flags & WALLET_FLAG_DESCRIPTORS)) { + error = Untranslated("Descriptor support must be enabled when using an external signer"); + status = DatabaseStatus::FAILED_CREATE; + return nullptr; + } + // Wallet::Verify will check if we're trying to create a wallet with a duplicate name. std::unique_ptr database = MakeWalletDatabase(name, options, status, error); if (!database) { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 2d25f46b6..1cc43c1ca 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -115,7 +115,8 @@ static constexpr uint64_t KNOWN_WALLET_FLAGS = | WALLET_FLAG_BLANK_WALLET | WALLET_FLAG_KEY_ORIGIN_METADATA | WALLET_FLAG_DISABLE_PRIVATE_KEYS - | WALLET_FLAG_DESCRIPTORS; + | WALLET_FLAG_DESCRIPTORS + | WALLET_FLAG_EXTERNAL_SIGNER; static constexpr uint64_t MUTABLE_WALLET_FLAGS = WALLET_FLAG_AVOID_REUSE; @@ -126,6 +127,7 @@ static const std::map WALLET_FLAG_MAP{ {"key_origin_metadata", WALLET_FLAG_KEY_ORIGIN_METADATA}, {"disable_private_keys", WALLET_FLAG_DISABLE_PRIVATE_KEYS}, {"descriptor_wallet", WALLET_FLAG_DESCRIPTORS}, + {"external_signer", WALLET_FLAG_EXTERNAL_SIGNER} }; extern const std::map WALLET_FLAG_CAVEATS; diff --git a/src/wallet/walletutil.h b/src/wallet/walletutil.h index 67b2ee2b9..0713f768c 100644 --- a/src/wallet/walletutil.h +++ b/src/wallet/walletutil.h @@ -60,6 +60,9 @@ enum WalletFlags : uint64_t { //! Indicate that this wallet supports DescriptorScriptPubKeyMan WALLET_FLAG_DESCRIPTORS = (1ULL << 34), + + //! Indicates that the wallet needs an external signer + WALLET_FLAG_EXTERNAL_SIGNER = (1ULL << 35), }; //! Get the path of the wallet directory. From 8ce7767071779a0170364e6426bd393ed71bf281 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Wed, 19 Feb 2020 16:40:00 +0100 Subject: [PATCH 06/15] wallet: add ExternalSignerScriptPubKeyMan --- src/Makefile.am | 2 ++ .../external_signer_scriptpubkeyman.cpp | 36 +++++++++++++++++++ src/wallet/external_signer_scriptpubkeyman.h | 28 +++++++++++++++ src/wallet/scriptpubkeyman.h | 5 +-- src/wallet/wallet.cpp | 14 ++++++-- 5 files changed, 81 insertions(+), 4 deletions(-) create mode 100644 src/wallet/external_signer_scriptpubkeyman.cpp create mode 100644 src/wallet/external_signer_scriptpubkeyman.h diff --git a/src/Makefile.am b/src/Makefile.am index 3b81056d2..9c33f7bdf 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -266,6 +266,7 @@ BITCOIN_CORE_H = \ wallet/db.h \ wallet/dump.h \ wallet/external_signer.h \ + wallet/external_signer_scriptpubkeyman.h \ wallet/feebumper.h \ wallet/fees.h \ wallet/ismine.h \ @@ -380,6 +381,7 @@ libbitcoin_wallet_a_SOURCES = \ wallet/crypter.cpp \ wallet/db.cpp \ wallet/dump.cpp \ + wallet/external_signer_scriptpubkeyman.cpp \ wallet/external_signer.cpp \ wallet/feebumper.cpp \ wallet/fees.cpp \ diff --git a/src/wallet/external_signer_scriptpubkeyman.cpp b/src/wallet/external_signer_scriptpubkeyman.cpp new file mode 100644 index 000000000..3dff67f35 --- /dev/null +++ b/src/wallet/external_signer_scriptpubkeyman.cpp @@ -0,0 +1,36 @@ +// Copyright (c) 2020 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include +#include + +#ifdef ENABLE_EXTERNAL_SIGNER + +bool ExternalSignerScriptPubKeyMan::SetupDescriptor(std::unique_ptr desc) +{ + LOCK(cs_desc_man); + assert(m_storage.IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)); + assert(m_storage.IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)); + + int64_t creation_time = GetTime(); + + // Make the descriptor + WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0); + m_wallet_descriptor = w_desc; + + // Store the descriptor + WalletBatch batch(m_storage.GetDatabase()); + if (!batch.WriteDescriptor(GetID(), m_wallet_descriptor)) { + throw std::runtime_error(std::string(__func__) + ": writing descriptor failed"); + } + + // TopUp + TopUp(); + + m_storage.UnsetBlankWalletFlag(batch); + return true; +} + +#endif diff --git a/src/wallet/external_signer_scriptpubkeyman.h b/src/wallet/external_signer_scriptpubkeyman.h new file mode 100644 index 000000000..40edbcf75 --- /dev/null +++ b/src/wallet/external_signer_scriptpubkeyman.h @@ -0,0 +1,28 @@ +// Copyright (c) 2019-2020 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_WALLET_EXTERNAL_SIGNER_SCRIPTPUBKEYMAN_H +#define BITCOIN_WALLET_EXTERNAL_SIGNER_SCRIPTPUBKEYMAN_H + +#ifdef ENABLE_EXTERNAL_SIGNER +#include + +class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan +{ + public: + ExternalSignerScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor) + : DescriptorScriptPubKeyMan(storage, descriptor) + {} + ExternalSignerScriptPubKeyMan(WalletStorage& storage, bool internal) + : DescriptorScriptPubKeyMan(storage, internal) + {} + + /** Provide a descriptor at setup time + * Returns false if already setup or setup fails, true if setup is successful + */ + bool SetupDescriptor(std::unique_ptrdesc); +}; +#endif + +#endif // BITCOIN_WALLET_EXTERNAL_SIGNER_SCRIPTPUBKEYMAN_H diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 51283e791..1aeb6e090 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -517,8 +517,6 @@ public: class DescriptorScriptPubKeyMan : public ScriptPubKeyMan { private: - WalletDescriptor m_wallet_descriptor GUARDED_BY(cs_desc_man); - using ScriptPubKeyMap = std::map; // Map of scripts to descriptor range index using PubKeyMap = std::map; // Map of pubkeys involved in scripts to descriptor range index using CryptedKeyMap = std::map>>; @@ -547,6 +545,9 @@ private: // Fetch the SigningProvider for a given index and optionally include private keys. Called by the above functions. std::unique_ptr GetSigningProvider(int32_t index, bool include_private = false) const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); +protected: + WalletDescriptor m_wallet_descriptor GUARDED_BY(cs_desc_man); + public: DescriptorScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor) : ScriptPubKeyMan(storage), diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 8a49383af..26c52773b 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -32,6 +32,7 @@ #include #include #include +#include #include @@ -4457,8 +4458,17 @@ void CWallet::ConnectScriptPubKeyManNotifiers() void CWallet::LoadDescriptorScriptPubKeyMan(uint256 id, WalletDescriptor& desc) { - auto spk_manager = std::unique_ptr(new DescriptorScriptPubKeyMan(*this, desc)); - m_spk_managers[id] = std::move(spk_manager); + 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__) + ": Configure with --enable-external-signer to use external signer wallets"); +#endif + } else { + auto spk_manager = std::unique_ptr(new DescriptorScriptPubKeyMan(*this, desc)); + m_spk_managers[id] = std::move(spk_manager); + } } void CWallet::SetupDescriptorScriptPubKeyMans() From 07b7c940a7da138d55a484ef83fee19ebf58a867 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Fri, 15 Feb 2019 12:54:29 +0100 Subject: [PATCH 07/15] rpc: add external signer RPC files --- src/Makefile.am | 2 ++ src/wallet/interfaces.cpp | 10 +++++++++ src/wallet/rpcsigner.cpp | 41 +++++++++++++++++++++++++++++++++++++ src/wallet/rpcsigner.h | 25 ++++++++++++++++++++++ test/functional/rpc_help.py | 5 ++++- 5 files changed, 82 insertions(+), 1 deletion(-) create mode 100644 src/wallet/rpcsigner.cpp create mode 100644 src/wallet/rpcsigner.h diff --git a/src/Makefile.am b/src/Makefile.am index 9c33f7bdf..8a9ef49a3 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -271,6 +271,7 @@ BITCOIN_CORE_H = \ wallet/fees.h \ wallet/ismine.h \ wallet/load.h \ + wallet/rpcsigner.h \ wallet/rpcwallet.h \ wallet/salvage.h \ wallet/scriptpubkeyman.h \ @@ -388,6 +389,7 @@ libbitcoin_wallet_a_SOURCES = \ wallet/interfaces.cpp \ wallet/load.cpp \ wallet/rpcdump.cpp \ + wallet/rpcsigner.cpp \ wallet/rpcwallet.cpp \ wallet/scriptpubkeyman.cpp \ wallet/wallet.cpp \ diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index e4e8c50f4..1fb789b12 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include @@ -518,6 +519,15 @@ public: }, command.argNames, command.unique_id); m_rpc_handlers.emplace_back(m_context.chain->handleRpc(m_rpc_commands.back())); } + +#ifdef ENABLE_EXTERNAL_SIGNER + for (const CRPCCommand& command : GetSignerRPCCommands()) { + m_rpc_commands.emplace_back(command.category, command.name, [this, &command](const JSONRPCRequest& request, UniValue& result, bool last_handler) { + return command.actor({request, m_context}, result, last_handler); + }, command.argNames, command.unique_id); + m_rpc_handlers.emplace_back(m_context.chain->handleRpc(m_rpc_commands.back())); + } +#endif } bool verify() override { return VerifyWallets(*m_context.chain); } bool load() override { return LoadWallets(*m_context.chain); } diff --git a/src/wallet/rpcsigner.cpp b/src/wallet/rpcsigner.cpp new file mode 100644 index 000000000..d2478908d --- /dev/null +++ b/src/wallet/rpcsigner.cpp @@ -0,0 +1,41 @@ +// Copyright (c) 2018-2021 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include +#include +#include + +#ifdef ENABLE_EXTERNAL_SIGNER + +// CRPCCommand table won't compile with an empty array +static RPCHelpMan dummy() +{ + return RPCHelpMan{"dummy", + "\nDoes nothing.\n" + "", + {}, + RPCResult{RPCResult::Type::NONE, "", ""}, + RPCExamples{""}, + [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue +{ + return NullUniValue; +}, + }; +} + +Span GetSignerRPCCommands() +{ +// clang-format off +static const CRPCCommand commands[] = +{ // category actor (function) + // --------------------- ------------------------ + { "signer", &dummy, }, +}; +// clang-format on + return MakeSpan(commands); +} + + +#endif // ENABLE_EXTERNAL_SIGNER diff --git a/src/wallet/rpcsigner.h b/src/wallet/rpcsigner.h new file mode 100644 index 000000000..f3ab83c42 --- /dev/null +++ b/src/wallet/rpcsigner.h @@ -0,0 +1,25 @@ +// Copyright (c) 2018-2021 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_WALLET_RPCSIGNER_H +#define BITCOIN_WALLET_RPCSIGNER_H + +#include +#include +#include + +#ifdef ENABLE_EXTERNAL_SIGNER + +class CRPCCommand; + +namespace interfaces { +class Chain; +class Handler; +} + +Span GetSignerRPCCommands(); + +#endif // ENABLE_EXTERNAL_SIGNER + +#endif //BITCOIN_WALLET_RPCSIGNER_H diff --git a/test/functional/rpc_help.py b/test/functional/rpc_help.py index 1eefd109f..de21f4374 100755 --- a/test/functional/rpc_help.py +++ b/test/functional/rpc_help.py @@ -105,10 +105,13 @@ class HelpRpcTest(BitcoinTestFramework): if self.is_wallet_compiled(): components.append('Wallet') + if self.is_external_signer_compiled(): + components.append('Signer') + if self.is_zmq_compiled(): components.append('Zmq') - assert_equal(titles, components) + assert_equal(titles, sorted(components)) def dump_help(self): dump_dir = os.path.join(self.options.tmpdir, 'rpc_help_dump') From 2700f09c4130af6167ce71f46960e92ca800e205 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Fri, 15 Feb 2019 12:54:29 +0100 Subject: [PATCH 08/15] rpc: signer: add enumeratesigners to list external signers --- src/wallet/external_signer.cpp | 54 +++++++++++++++++++++++++++-- src/wallet/external_signer.h | 23 ++++++++++++- src/wallet/rpcsigner.cpp | 59 +++++++++++++++++++++++++------- test/functional/mocks/signer.py | 6 ++++ test/functional/wallet_signer.py | 31 +++++++++++++++++ 5 files changed, 157 insertions(+), 16 deletions(-) diff --git a/src/wallet/external_signer.cpp b/src/wallet/external_signer.cpp index 6f850e477..c06b178bc 100644 --- a/src/wallet/external_signer.cpp +++ b/src/wallet/external_signer.cpp @@ -2,7 +2,57 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include #include -#include -ExternalSigner::ExternalSigner(const std::string& command, const std::string& fingerprint): m_command(command), m_fingerprint(fingerprint) {} +ExternalSigner::ExternalSigner(const std::string& command, const std::string& fingerprint, std::string chain, std::string name): m_command(command), m_fingerprint(fingerprint), m_chain(chain), m_name(name) {} + +const std::string ExternalSigner::NetworkArg() const +{ + return " --chain " + m_chain; +} + +#ifdef ENABLE_EXTERNAL_SIGNER + +bool ExternalSigner::Enumerate(const std::string& command, std::vector& signers, std::string chain, bool ignore_errors) +{ + // Call enumerate + const UniValue result = RunCommandParseJSON(command + " enumerate"); + if (!result.isArray()) { + if (ignore_errors) return false; + throw ExternalSignerException(strprintf("'%s' received invalid response, expected array of signers", command)); + } + for (UniValue signer : result.getValues()) { + // Check for error + const UniValue& error = find_value(signer, "error"); + if (!error.isNull()) { + if (ignore_errors) return false; + if (!error.isStr()) { + throw ExternalSignerException(strprintf("'%s' error", command)); + } + throw ExternalSignerException(strprintf("'%s' error: %s", command, error.getValStr())); + } + // Check if fingerprint is present + const UniValue& fingerprint = find_value(signer, "fingerprint"); + if (fingerprint.isNull()) { + if (ignore_errors) return false; + throw ExternalSignerException(strprintf("'%s' received invalid response, missing signer fingerprint", command)); + } + std::string fingerprintStr = fingerprint.get_str(); + // Skip duplicate signer + bool duplicate = false; + for (ExternalSigner signer : signers) { + if (signer.m_fingerprint.compare(fingerprintStr) == 0) duplicate = true; + } + if (duplicate) break; + std::string name = ""; + const UniValue& model_field = find_value(signer, "model"); + if (model_field.isStr() && model_field.getValStr() != "") { + name += model_field.getValStr(); + } + signers.push_back(ExternalSigner(command, fingerprintStr, chain, name)); + } + return true; +} + +#endif diff --git a/src/wallet/external_signer.h b/src/wallet/external_signer.h index 08fb0c9f3..9d32fe1e0 100644 --- a/src/wallet/external_signer.h +++ b/src/wallet/external_signer.h @@ -8,6 +8,7 @@ #include #include #include +#include class ExternalSignerException : public std::runtime_error { public: @@ -25,10 +26,30 @@ private: public: //! @param[in] command the command which handles interaction with the external signer //! @param[in] fingerprint master key fingerprint of the signer - ExternalSigner(const std::string& command, const std::string& fingerprint); + //! @param[in] chain "main", "test", "regtest" or "signet" + //! @param[in] name device name + ExternalSigner(const std::string& command, const std::string& fingerprint, std::string chain, 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; + +#ifdef ENABLE_EXTERNAL_SIGNER + //! Obtain a list of signers. Calls ` 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 + //! @param chain "main", "test", "regtest" or "signet" + //! @param[out] success Boolean + static bool Enumerate(const std::string& command, std::vector& signers, std::string chain, bool ignore_errors = false); + +#endif }; #endif // BITCOIN_WALLET_EXTERNAL_SIGNER_H diff --git a/src/wallet/rpcsigner.cpp b/src/wallet/rpcsigner.cpp index d2478908d..76f4f3c6a 100644 --- a/src/wallet/rpcsigner.cpp +++ b/src/wallet/rpcsigner.cpp @@ -2,36 +2,69 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include #include +#include #include #include +#include #include #ifdef ENABLE_EXTERNAL_SIGNER -// CRPCCommand table won't compile with an empty array -static RPCHelpMan dummy() +static RPCHelpMan enumeratesigners() { - return RPCHelpMan{"dummy", - "\nDoes nothing.\n" - "", - {}, - RPCResult{RPCResult::Type::NONE, "", ""}, - RPCExamples{""}, - [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue -{ - return NullUniValue; -}, + return RPCHelpMan{ + "enumeratesigners", + "Returns a list of external signers from -signer.", + {}, + RPCResult{ + RPCResult::Type::OBJ, "", "", + { + {RPCResult::Type::ARR, "signers", /* optional */ false, "", + { + {RPCResult::Type::STR_HEX, "masterkeyfingerprint", "Master key fingerprint"}, + {RPCResult::Type::STR, "name", "Device name"}, + }, + } + } + }, + RPCExamples{""}, + [](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { + std::shared_ptr const wallet = GetWalletForJSONRPCRequest(request); + if (!wallet) return NullUniValue; + + const std::string command = gArgs.GetArg("-signer", ""); + if (command == "") throw JSONRPCError(RPC_WALLET_ERROR, "Error: restart bitcoind with -signer="); + std::string chain = gArgs.GetChainName(); + UniValue signers_res = UniValue::VARR; + try { + std::vector signers; + ExternalSigner::Enumerate(command, signers, chain); + for (ExternalSigner signer : signers) { + UniValue signer_res = UniValue::VOBJ; + signer_res.pushKV("fingerprint", signer.m_fingerprint); + signer_res.pushKV("name", signer.m_name); + signers_res.push_back(signer_res); + } + } catch (const ExternalSignerException& e) { + throw JSONRPCError(RPC_WALLET_ERROR, e.what()); + } + UniValue result(UniValue::VOBJ); + result.pushKV("signers", signers_res); + return result; + } }; } Span GetSignerRPCCommands() { + // clang-format off static const CRPCCommand commands[] = { // category actor (function) // --------------------- ------------------------ - { "signer", &dummy, }, + { "signer", &enumeratesigners, }, }; // clang-format on return MakeSpan(commands); diff --git a/test/functional/mocks/signer.py b/test/functional/mocks/signer.py index 6b5c90388..cde85bcd7 100755 --- a/test/functional/mocks/signer.py +++ b/test/functional/mocks/signer.py @@ -17,10 +17,16 @@ def perform_pre_checks(): sys.stdout.write(mock_result[2:]) sys.exit(int(mock_result[0])) +def enumerate(args): + sys.stdout.write(json.dumps([{"fingerprint": "00000001", "type": "trezor", "model": "trezor_t"}, {"fingerprint": "00000002"}])) + parser = argparse.ArgumentParser(prog='./signer.py', description='External signer mock') subparsers = parser.add_subparsers(description='Commands', dest='command') subparsers.required = True +parser_enumerate = subparsers.add_parser('enumerate', help='list available signers') +parser_enumerate.set_defaults(func=enumerate) + args = parser.parse_args() perform_pre_checks() diff --git a/test/functional/wallet_signer.py b/test/functional/wallet_signer.py index 62d4db837..295cdcf39 100755 --- a/test/functional/wallet_signer.py +++ b/test/functional/wallet_signer.py @@ -48,5 +48,36 @@ class SignerTest(BitcoinTestFramework): def run_test(self): self.log.debug(f"-signer={self.mock_signer_path()}") + assert_raises_rpc_error(-4, 'Error: restart bitcoind with -signer=', + self.nodes[0].enumeratesigners + ) + + # Handle script missing: + assert_raises_rpc_error(-1, 'execve failed: No such file or directory', + self.nodes[2].enumeratesigners + ) + + # Handle error thrown by script + self.set_mock_result(self.nodes[1], "2") + assert_raises_rpc_error(-1, 'RunCommandParseJSON error', + self.nodes[1].enumeratesigners + ) + self.clear_mock_result(self.nodes[1]) + + self.set_mock_result(self.nodes[1], '0 [{"type": "trezor", "model": "trezor_t", "error": "fingerprint not found"}]') + assert_raises_rpc_error(-4, 'fingerprint not found', + self.nodes[1].enumeratesigners + ) + self.clear_mock_result(self.nodes[1]) + + # Create new wallets with private keys disabled: + self.nodes[1].createwallet(wallet_name='hww', disable_private_keys=True, descriptors=True) + hww = self.nodes[1].get_wallet_rpc('hww') + + result = hww.enumeratesigners() + assert_equal(len(result['signers']), 2) + assert_equal(result['signers'][0]["fingerprint"], "00000001") + assert_equal(result['signers'][0]["name"], "trezor_t") + if __name__ == '__main__': SignerTest().main() From 2655197e1c2dea9536c32afe1482ced4a1f481e9 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Sun, 4 Aug 2019 17:56:17 +0200 Subject: [PATCH 09/15] rpc: add external_signer option to createwallet --- src/rpc/client.cpp | 1 + src/wallet/rpcwallet.cpp | 8 ++++++++ test/functional/test_framework/test_node.py | 4 ++-- test/functional/wallet_signer.py | 11 +++++++++-- 4 files changed, 20 insertions(+), 4 deletions(-) diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index d1eb849b7..2b593cd10 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -183,6 +183,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "createwallet", 4, "avoid_reuse"}, { "createwallet", 5, "descriptors"}, { "createwallet", 6, "load_on_startup"}, + { "createwallet", 7, "external_signer"}, { "loadwallet", 1, "load_on_startup"}, { "unloadwallet", 1, "load_on_startup"}, { "getnodeaddresses", 0, "count"}, diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 53232db6b..9d61f6fbe 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2726,6 +2726,7 @@ static RPCHelpMan createwallet() {"avoid_reuse", RPCArg::Type::BOOL, /* default */ "false", "Keep track of coin reuse, and treat dirty and clean coins differently with privacy considerations in mind."}, {"descriptors", RPCArg::Type::BOOL, /* default */ "false", "Create a native descriptor wallet. The wallet will use descriptors internally to handle address creation"}, {"load_on_startup", RPCArg::Type::BOOL, /* default */ "null", "Save wallet name to persistent settings and load on startup. True to add wallet to startup list, false to remove, null to leave unchanged."}, + {"external_signer", RPCArg::Type::BOOL, /* default */ "false", "Use an external signer such as a hardware wallet. Requires -signer to be configured. Wallet creation will fail if keys cannot be fetched. Requires disable_private_keys and descriptors set to true."}, }, RPCResult{ RPCResult::Type::OBJ, "", "", @@ -2770,6 +2771,13 @@ static RPCHelpMan createwallet() flags |= WALLET_FLAG_DESCRIPTORS; warnings.emplace_back(Untranslated("Wallet is an experimental descriptor wallet")); } + if (!request.params[7].isNull() && request.params[7].get_bool()) { +#ifdef ENABLE_EXTERNAL_SIGNER + flags |= WALLET_FLAG_EXTERNAL_SIGNER; +#else + throw JSONRPCError(RPC_WALLET_ERROR, "Configure with --enable-external-signer to use this"); +#endif + } #ifndef USE_BDB if (!(flags & WALLET_FLAG_DESCRIPTORS)) { diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index b820c36e6..ce9c1bc02 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -678,10 +678,10 @@ class RPCOverloadWrapper(): def __getattr__(self, name): return getattr(self.rpc, name) - def createwallet(self, wallet_name, disable_private_keys=None, blank=None, passphrase='', avoid_reuse=None, descriptors=None, load_on_startup=None): + def createwallet(self, wallet_name, disable_private_keys=None, blank=None, passphrase='', avoid_reuse=None, descriptors=None, load_on_startup=None, external_signer=None): if descriptors is None: descriptors = self.descriptors - return self.__getattr__('createwallet')(wallet_name, disable_private_keys, blank, passphrase, avoid_reuse, descriptors, load_on_startup) + return self.__getattr__('createwallet')(wallet_name, disable_private_keys, blank, passphrase, avoid_reuse, descriptors, load_on_startup, external_signer) def importprivkey(self, privkey, label=None, rescan=None): wallet_info = self.getwalletinfo() diff --git a/test/functional/wallet_signer.py b/test/functional/wallet_signer.py index 295cdcf39..10795758e 100755 --- a/test/functional/wallet_signer.py +++ b/test/functional/wallet_signer.py @@ -70,8 +70,15 @@ class SignerTest(BitcoinTestFramework): ) self.clear_mock_result(self.nodes[1]) - # Create new wallets with private keys disabled: - self.nodes[1].createwallet(wallet_name='hww', disable_private_keys=True, descriptors=True) + # Create new wallets for an external signer. + # disable_private_keys and descriptors must be true: + assert_raises_rpc_error(-4, "Private keys must be disabled when using an external signer", self.nodes[1].createwallet, wallet_name='not_hww', disable_private_keys=False, descriptors=True, external_signer=True) + if self.is_bdb_compiled(): + assert_raises_rpc_error(-4, "Descriptor support must be enabled when using an external signer", self.nodes[1].createwallet, wallet_name='not_hww', disable_private_keys=True, descriptors=False, external_signer=True) + else: + assert_raises_rpc_error(-4, "Compiled without bdb support (required for legacy wallets)", self.nodes[1].createwallet, wallet_name='not_hww', disable_private_keys=True, descriptors=False, external_signer=True) + + self.nodes[1].createwallet(wallet_name='hww', disable_private_keys=True, descriptors=True, external_signer=True) hww = self.nodes[1].get_wallet_rpc('hww') result = hww.enumeratesigners() From 259f52cc33817a00b91ec9c7d078c07b88db7ab4 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Sun, 4 Aug 2019 17:56:39 +0200 Subject: [PATCH 10/15] test: external_signer wallet flag is immutable --- test/functional/wallet_signer.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/functional/wallet_signer.py b/test/functional/wallet_signer.py index 10795758e..d46b3899c 100755 --- a/test/functional/wallet_signer.py +++ b/test/functional/wallet_signer.py @@ -86,5 +86,10 @@ class SignerTest(BitcoinTestFramework): assert_equal(result['signers'][0]["fingerprint"], "00000001") assert_equal(result['signers'][0]["name"], "trezor_t") + # Flag can't be set afterwards (could be added later for non-blank descriptor based watch-only wallets) + self.nodes[1].createwallet(wallet_name='not_hww', disable_private_keys=True, descriptors=True, external_signer=False) + not_hww = self.nodes[1].get_wallet_rpc('not_hww') + assert_raises_rpc_error(-8, "Wallet flag is immutable: external_signer", not_hww.setwalletflag, "external_signer", True) + if __name__ == '__main__': SignerTest().main() From fc5da520f5c72287f59823b8a6d748dda49c574a Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Thu, 31 Oct 2019 10:27:27 +0100 Subject: [PATCH 11/15] wallet: add GetExternalSigner() --- src/wallet/external_signer_scriptpubkeyman.cpp | 10 ++++++++++ src/wallet/wallet.cpp | 13 +++++++++++++ src/wallet/wallet.h | 3 +++ 3 files changed, 26 insertions(+) diff --git a/src/wallet/external_signer_scriptpubkeyman.cpp b/src/wallet/external_signer_scriptpubkeyman.cpp index 3dff67f35..b8bd69f94 100644 --- a/src/wallet/external_signer_scriptpubkeyman.cpp +++ b/src/wallet/external_signer_scriptpubkeyman.cpp @@ -33,4 +33,14 @@ bool ExternalSignerScriptPubKeyMan::SetupDescriptor(std::unique_ptr return true; } +ExternalSigner ExternalSignerScriptPubKeyMan::GetExternalSigner() { + const std::string command = gArgs.GetArg("-signer", ""); + if (command == "") throw std::runtime_error(std::string(__func__) + ": restart bitcoind with -signer="); + std::vector signers; + ExternalSigner::Enumerate(command, signers, Params().NetworkIDString()); + if (signers.empty()) throw std::runtime_error(std::string(__func__) + ": No external signers found"); + // TODO: add fingerprint argument in case of multiple signers + return signers[0]; +} + #endif diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 26c52773b..ddb74a710 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3573,6 +3573,19 @@ void ReserveDestination::ReturnDestination() address = CNoDestination(); } +#ifdef ENABLE_EXTERNAL_SIGNER +ExternalSigner CWallet::GetExternalSigner() +{ + const std::string command = gArgs.GetArg("-signer", ""); + if (command == "") throw std::runtime_error(std::string(__func__) + ": restart bitcoind with -signer="); + std::vector signers; + ExternalSigner::Enumerate(command, signers, Params().NetworkIDString()); + if (signers.empty()) throw std::runtime_error(std::string(__func__) + ": No external signers found"); + // TODO: add fingerprint argument in case of multiple signers + return signers[0]; +} +#endif + void CWallet::LockCoin(const COutPoint& output) { AssertLockHeld(cs_wallet); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 1cc43c1ca..3c27aee48 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -839,6 +839,9 @@ public: std::vector GroupOutputs(const std::vector& outputs, bool separate_coins, const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate, const CoinEligibilityFilter& filter, bool positive_only) const; +#ifdef ENABLE_EXTERNAL_SIGNER + ExternalSigner GetExternalSigner() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); +#endif bool IsLockedCoin(uint256 hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void LockCoin(const COutPoint& output) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void UnlockCoin(const COutPoint& output) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); From 7ebc7c0215979c53b92a436acc8b5b607b8d735a Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Thu, 31 Oct 2019 10:27:47 +0100 Subject: [PATCH 12/15] wallet: ExternalSigner: add GetDescriptors method --- src/wallet/external_signer.cpp | 5 ++ src/wallet/external_signer.h | 6 ++ src/wallet/external_signer_scriptpubkeyman.h | 3 + src/wallet/scriptpubkeyman.h | 5 ++ src/wallet/wallet.cpp | 78 ++++++++++++++------ test/functional/mocks/signer.py | 24 ++++++ test/functional/wallet_signer.py | 32 ++++++++ 7 files changed, 131 insertions(+), 22 deletions(-) diff --git a/src/wallet/external_signer.cpp b/src/wallet/external_signer.cpp index c06b178bc..ec915d5c5 100644 --- a/src/wallet/external_signer.cpp +++ b/src/wallet/external_signer.cpp @@ -55,4 +55,9 @@ bool ExternalSigner::Enumerate(const std::string& command, std::vector& signers, std::string chain, bool ignore_errors = false); + //! Get receive and change Descriptor(s) from device for a given account. + //! Calls ` getdescriptors --account ` + //! @param[in] account which BIP32 account to use (e.g. `m/44'/0'/account'`) + //! @param[out] UniValue see doc/external-signer.md + UniValue GetDescriptors(int account); + #endif }; diff --git a/src/wallet/external_signer_scriptpubkeyman.h b/src/wallet/external_signer_scriptpubkeyman.h index 40edbcf75..cef0a1807 100644 --- a/src/wallet/external_signer_scriptpubkeyman.h +++ b/src/wallet/external_signer_scriptpubkeyman.h @@ -22,6 +22,9 @@ class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan * Returns false if already setup or setup fails, true if setup is successful */ bool SetupDescriptor(std::unique_ptrdesc); + + static ExternalSigner GetExternalSigner(); + }; #endif diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 1aeb6e090..b8e34fbac 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -582,6 +582,11 @@ public: //! Setup descriptors based on the given CExtkey bool SetupDescriptorGeneration(const CExtKey& master_key, OutputType addr_type); + /** Provide a descriptor at setup time + * Returns false if already setup or setup fails, true if setup is successful + */ + bool SetupDescriptor(std::unique_ptrdesc); + bool HavePrivateKeys() const override; int64_t GetOldestKeyPoolTime() const override; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index ddb74a710..795299d10 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -3864,7 +3865,7 @@ std::shared_ptr CWallet::Create(interfaces::Chain& chain, const std::st walletInstance->SetupLegacyScriptPubKeyMan(); } - if (!(wallet_creation_flags & (WALLET_FLAG_DISABLE_PRIVATE_KEYS | WALLET_FLAG_BLANK_WALLET))) { + if ((wallet_creation_flags & WALLET_FLAG_EXTERNAL_SIGNER) || !(wallet_creation_flags & (WALLET_FLAG_DISABLE_PRIVATE_KEYS | WALLET_FLAG_BLANK_WALLET))) { LOCK(walletInstance->cs_wallet); if (walletInstance->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { walletInstance->SetupDescriptorScriptPubKeyMans(); @@ -4488,32 +4489,65 @@ void CWallet::SetupDescriptorScriptPubKeyMans() { AssertLockHeld(cs_wallet); - // Make a seed - CKey seed_key; - seed_key.MakeNewKey(true); - CPubKey seed = seed_key.GetPubKey(); - assert(seed_key.VerifyPubKey(seed)); + if (!IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) { + // Make a seed + CKey seed_key; + seed_key.MakeNewKey(true); + CPubKey seed = seed_key.GetPubKey(); + assert(seed_key.VerifyPubKey(seed)); - // Get the extended key - CExtKey master_key; - master_key.SetSeed(seed_key.begin(), seed_key.size()); + // Get the extended key + CExtKey master_key; + master_key.SetSeed(seed_key.begin(), seed_key.size()); - for (bool internal : {false, true}) { - for (OutputType t : OUTPUT_TYPES) { - auto spk_manager = std::unique_ptr(new DescriptorScriptPubKeyMan(*this, internal)); - if (IsCrypted()) { - if (IsLocked()) { - throw std::runtime_error(std::string(__func__) + ": Wallet is locked, cannot setup new descriptors"); - } - if (!spk_manager->CheckDecryptionKey(vMasterKey) && !spk_manager->Encrypt(vMasterKey, nullptr)) { - throw std::runtime_error(std::string(__func__) + ": Could not encrypt new descriptors"); + for (bool internal : {false, true}) { + for (OutputType t : OUTPUT_TYPES) { + auto spk_manager = std::unique_ptr(new DescriptorScriptPubKeyMan(*this, internal)); + if (IsCrypted()) { + if (IsLocked()) { + throw std::runtime_error(std::string(__func__) + ": Wallet is locked, cannot setup new descriptors"); + } + if (!spk_manager->CheckDecryptionKey(vMasterKey) && !spk_manager->Encrypt(vMasterKey, nullptr)) { + throw std::runtime_error(std::string(__func__) + ": Could not encrypt new descriptors"); + } } + spk_manager->SetupDescriptorGeneration(master_key, t); + uint256 id = spk_manager->GetID(); + m_spk_managers[id] = std::move(spk_manager); + AddActiveScriptPubKeyMan(id, t, internal); } - spk_manager->SetupDescriptorGeneration(master_key, t); - uint256 id = spk_manager->GetID(); - m_spk_managers[id] = std::move(spk_manager); - AddActiveScriptPubKeyMan(id, t, internal); } + } else { +#ifdef ENABLE_EXTERNAL_SIGNER + ExternalSigner signer = ExternalSignerScriptPubKeyMan::GetExternalSigner(); + + // TODO: add account parameter + int account = 0; + UniValue signer_res = signer.GetDescriptors(account); + + if (!signer_res.isObject()) throw std::runtime_error(std::string(__func__) + ": Unexpected result"); + for (bool internal : {false, true}) { + const UniValue& descriptor_vals = find_value(signer_res, internal ? "internal" : "receive"); + if (!descriptor_vals.isArray()) throw std::runtime_error(std::string(__func__) + ": Unexpected result"); + for (const UniValue& desc_val : descriptor_vals.get_array().getValues()) { + std::string desc_str = desc_val.getValStr(); + FlatSigningProvider keys; + std::string dummy_error; + std::unique_ptr desc = Parse(desc_str, keys, dummy_error, false); + if (!desc->GetOutputType()) { + continue; + } + OutputType t = *desc->GetOutputType(); + auto spk_manager = std::unique_ptr(new ExternalSignerScriptPubKeyMan(*this, internal)); + spk_manager->SetupDescriptor(std::move(desc)); + uint256 id = spk_manager->GetID(); + m_spk_managers[id] = std::move(spk_manager); + AddActiveScriptPubKeyMan(id, t, internal); + } + } +#else + throw std::runtime_error(std::string(__func__) + ": Wallets with external signers require Boost::Process library."); +#endif } } diff --git a/test/functional/mocks/signer.py b/test/functional/mocks/signer.py index cde85bcd7..aedab14ce 100755 --- a/test/functional/mocks/signer.py +++ b/test/functional/mocks/signer.py @@ -20,13 +20,37 @@ def perform_pre_checks(): def enumerate(args): sys.stdout.write(json.dumps([{"fingerprint": "00000001", "type": "trezor", "model": "trezor_t"}, {"fingerprint": "00000002"}])) +def getdescriptors(args): + xpub = "tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B" + + sys.stdout.write(json.dumps({ + "receive": [ + "pkh([00000001/44'/1'/" + args.account + "']" + xpub + "/0/*)#vt6w3l3j", + "sh(wpkh([00000001/49'/1'/" + args.account + "']" + xpub + "/0/*))#r0grqw5x", + "wpkh([00000001/84'/1'/" + args.account + "']" + xpub + "/0/*)#x30uthjs" + ], + "internal": [ + "pkh([00000001/44'/1'/" + args.account + "']" + xpub + "/1/*)#all0v2p2", + "sh(wpkh([00000001/49'/1'/" + args.account + "']" + xpub + "/1/*))#kwx4c3pe", + "wpkh([00000001/84'/1'/" + args.account + "']" + xpub + "/1/*)#h92akzzg" + ] + })) + + parser = argparse.ArgumentParser(prog='./signer.py', description='External signer mock') +parser.add_argument('--fingerprint') +parser.add_argument('--chain', default='main') + subparsers = parser.add_subparsers(description='Commands', dest='command') subparsers.required = True parser_enumerate = subparsers.add_parser('enumerate', help='list available signers') parser_enumerate.set_defaults(func=enumerate) +parser_getdescriptors = subparsers.add_parser('getdescriptors') +parser_getdescriptors.set_defaults(func=getdescriptors) +parser_getdescriptors.add_argument('--account', metavar='account') + args = parser.parse_args() perform_pre_checks() diff --git a/test/functional/wallet_signer.py b/test/functional/wallet_signer.py index d46b3899c..abbffe24d 100755 --- a/test/functional/wallet_signer.py +++ b/test/functional/wallet_signer.py @@ -91,5 +91,37 @@ class SignerTest(BitcoinTestFramework): not_hww = self.nodes[1].get_wallet_rpc('not_hww') assert_raises_rpc_error(-8, "Wallet flag is immutable: external_signer", not_hww.setwalletflag, "external_signer", True) + # assert_raises_rpc_error(-4, "Multiple signers found, please specify which to use", wallet_name='not_hww', disable_private_keys=True, descriptors=True, external_signer=True) + + # TODO: Handle error thrown by script + # self.set_mock_result(self.nodes[1], "2") + # assert_raises_rpc_error(-1, 'Unable to parse JSON', + # self.nodes[1].createwallet, wallet_name='not_hww2', disable_private_keys=True, descriptors=True, external_signer=False + # ) + # self.clear_mock_result(self.nodes[1]) + + assert_equal(hww.getwalletinfo()["keypoolsize"], 3) + + address1 = hww.getnewaddress(address_type="bech32") + assert_equal(address1, "bcrt1qm90ugl4d48jv8n6e5t9ln6t9zlpm5th68x4f8g") + address_info = hww.getaddressinfo(address1) + assert_equal(address_info['solvable'], True) + assert_equal(address_info['ismine'], True) + assert_equal(address_info['hdkeypath'], "m/84'/1'/0'/0/0") + + address2 = hww.getnewaddress(address_type="p2sh-segwit") + assert_equal(address2, "2N2gQKzjUe47gM8p1JZxaAkTcoHPXV6YyVp") + address_info = hww.getaddressinfo(address2) + assert_equal(address_info['solvable'], True) + assert_equal(address_info['ismine'], True) + assert_equal(address_info['hdkeypath'], "m/49'/1'/0'/0/0") + + address3 = hww.getnewaddress(address_type="legacy") + assert_equal(address3, "n1LKejAadN6hg2FrBXoU1KrwX4uK16mco9") + address_info = hww.getaddressinfo(address3) + assert_equal(address_info['solvable'], True) + assert_equal(address_info['ismine'], True) + assert_equal(address_info['hdkeypath'], "m/44'/1'/0'/0/0") + if __name__ == '__main__': SignerTest().main() From 245b4457cf9265190a05529a0a97e1cb258cca8a Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Wed, 19 Feb 2020 14:33:37 +0100 Subject: [PATCH 13/15] rpc: signerdisplayaddress --- src/wallet/external_signer.cpp | 5 +++ src/wallet/external_signer.h | 5 +++ .../external_signer_scriptpubkeyman.cpp | 35 ++++++++++++++++++ src/wallet/external_signer_scriptpubkeyman.h | 1 + src/wallet/rpcsigner.cpp | 37 +++++++++++++++++++ src/wallet/wallet.cpp | 19 ++++++++++ src/wallet/wallet.h | 3 ++ test/functional/mocks/signer.py | 18 +++++++++ test/functional/wallet_signer.py | 11 ++++++ 9 files changed, 134 insertions(+) diff --git a/src/wallet/external_signer.cpp b/src/wallet/external_signer.cpp index ec915d5c5..baf97700e 100644 --- a/src/wallet/external_signer.cpp +++ b/src/wallet/external_signer.cpp @@ -55,6 +55,11 @@ bool ExternalSigner::Enumerate(const std::string& command, std::vector& signers, std::string chain, bool ignore_errors = false); + //! Display address on the device. Calls ` displayaddress --desc `. + //! @param[in] descriptor Descriptor specifying which address to display. + //! Must include a public key or xpub, as well as key origin. + UniValue DisplayAddress(const std::string& descriptor) const; + //! Get receive and change Descriptor(s) from device for a given account. //! Calls ` getdescriptors --account ` //! @param[in] account which BIP32 account to use (e.g. `m/44'/0'/account'`) diff --git a/src/wallet/external_signer_scriptpubkeyman.cpp b/src/wallet/external_signer_scriptpubkeyman.cpp index b8bd69f94..a2071e521 100644 --- a/src/wallet/external_signer_scriptpubkeyman.cpp +++ b/src/wallet/external_signer_scriptpubkeyman.cpp @@ -43,4 +43,39 @@ ExternalSigner ExternalSignerScriptPubKeyMan::GetExternalSigner() { return signers[0]; } +bool ExternalSignerScriptPubKeyMan::DisplayAddress(const CScript scriptPubKey, const ExternalSigner &signer) const +{ + // TODO: avoid the need to infer a descriptor from inside a descriptor wallet + auto provider = GetSolvingProvider(scriptPubKey); + auto descriptor = InferDescriptor(scriptPubKey, *provider); + + signer.DisplayAddress(descriptor->ToString()); + // TODO inspect result + return true; +} + +// If sign is true, transaction must previously have been filled +TransactionError ExternalSignerScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& psbt, int sighash_type, bool sign, bool bip32derivs, int* n_signed) const +{ + if (!sign) { + return DescriptorScriptPubKeyMan::FillPSBT(psbt, sighash_type, false, bip32derivs, n_signed); + } + + // Already complete if every input is now signed + bool complete = true; + for (const auto& input : psbt.inputs) { + // TODO: for multisig wallets, we should only care if all _our_ inputs are signed + complete &= PSBTInputSigned(input); + } + if (complete) return TransactionError::OK; + + std::string strFailReason; + if(!GetExternalSigner().SignTransaction(psbt, strFailReason)) { + tfm::format(std::cerr, "Failed to sign: %s\n", strFailReason); + return TransactionError::EXTERNAL_SIGNER_FAILED; + } + 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 cef0a1807..1cde143ef 100644 --- a/src/wallet/external_signer_scriptpubkeyman.h +++ b/src/wallet/external_signer_scriptpubkeyman.h @@ -25,6 +25,7 @@ class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan static ExternalSigner GetExternalSigner(); + bool DisplayAddress(const CScript scriptPubKey, const ExternalSigner &signer) const; }; #endif diff --git a/src/wallet/rpcsigner.cpp b/src/wallet/rpcsigner.cpp index 76f4f3c6a..607b778c6 100644 --- a/src/wallet/rpcsigner.cpp +++ b/src/wallet/rpcsigner.cpp @@ -3,6 +3,7 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include +#include #include #include #include @@ -57,6 +58,41 @@ static RPCHelpMan enumeratesigners() }; } +static RPCHelpMan signerdisplayaddress() +{ + return RPCHelpMan{ + "signerdisplayaddress", + "Display address on an external signer for verification.\n", + { + {"address", RPCArg::Type::STR, RPCArg::Optional::NO, /* default_val */ "", "bitcoin address to display"}, + }, + RPCResult{RPCResult::Type::NONE,"",""}, + RPCExamples{""}, + [](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { + std::shared_ptr const wallet = GetWalletForJSONRPCRequest(request); + if (!wallet) return NullUniValue; + CWallet* const pwallet = wallet.get(); + + LOCK(pwallet->cs_wallet); + + CTxDestination dest = DecodeDestination(request.params[0].get_str()); + + // Make sure the destination is valid + if (!IsValidDestination(dest)) { + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid address"); + } + + if (!pwallet->DisplayAddress(dest)) { + throw JSONRPCError(RPC_WALLET_ERROR, "Failed to display address"); + } + + UniValue result(UniValue::VOBJ); + result.pushKV("address", request.params[0].get_str()); + return result; + } + }; +} + Span GetSignerRPCCommands() { @@ -65,6 +101,7 @@ static const CRPCCommand commands[] = { // category actor (function) // --------------------- ------------------------ { "signer", &enumeratesigners, }, + { "signer", &signerdisplayaddress, }, }; // clang-format on return MakeSpan(commands); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 795299d10..7c1543806 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3587,6 +3587,25 @@ ExternalSigner CWallet::GetExternalSigner() } #endif +bool CWallet::DisplayAddress(const CTxDestination& dest) +{ +#ifdef ENABLE_EXTERNAL_SIGNER + CScript scriptPubKey = GetScriptForDestination(dest); + const auto spk_man = GetScriptPubKeyMan(scriptPubKey); + if (spk_man == nullptr) { + return false; + } + auto signer_spk_man = dynamic_cast(spk_man); + if (signer_spk_man == nullptr) { + return false; + } + ExternalSigner signer = GetExternalSigner(); // TODO: move signer in spk_man + return signer_spk_man->DisplayAddress(scriptPubKey, signer); +#else + return false; +#endif +} + void CWallet::LockCoin(const COutPoint& output) { AssertLockHeld(cs_wallet); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 3c27aee48..eb797938c 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -842,6 +842,9 @@ public: #ifdef ENABLE_EXTERNAL_SIGNER ExternalSigner GetExternalSigner() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); #endif + /** Display address on an external signer. Returns false if external signer support is not compiled */ + bool DisplayAddress(const CTxDestination& dest) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + bool IsLockedCoin(uint256 hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void LockCoin(const COutPoint& output) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void UnlockCoin(const COutPoint& output) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); diff --git a/test/functional/mocks/signer.py b/test/functional/mocks/signer.py index aedab14ce..4036c785b 100755 --- a/test/functional/mocks/signer.py +++ b/test/functional/mocks/signer.py @@ -37,6 +37,20 @@ def getdescriptors(args): })) +def displayaddress(args): + # Several descriptor formats are acceptable, so allowing for potential + # changes to InferDescriptor: + if args.fingerprint != "00000001": + return sys.stdout.write(json.dumps({"error": "Unexpected fingerprint", "fingerprint": args.fingerprint})) + + expected_desc = [ + "wpkh([00000001/84'/1'/0'/0/0]02c97dc3f4420402e01a113984311bf4a1b8de376cac0bdcfaf1b3ac81f13433c7)#0yneg42r" + ] + if args.desc not in expected_desc: + return sys.stdout.write(json.dumps({"error": "Unexpected descriptor", "desc": args.desc})) + + return sys.stdout.write(json.dumps({"address": "bcrt1qm90ugl4d48jv8n6e5t9ln6t9zlpm5th68x4f8g"})) + parser = argparse.ArgumentParser(prog='./signer.py', description='External signer mock') parser.add_argument('--fingerprint') parser.add_argument('--chain', default='main') @@ -51,6 +65,10 @@ parser_getdescriptors = subparsers.add_parser('getdescriptors') parser_getdescriptors.set_defaults(func=getdescriptors) parser_getdescriptors.add_argument('--account', metavar='account') +parser_displayaddress = subparsers.add_parser('displayaddress', help='display address on signer') +parser_displayaddress.add_argument('--desc', metavar='desc') +parser_displayaddress.set_defaults(func=displayaddress) + args = parser.parse_args() perform_pre_checks() diff --git a/test/functional/wallet_signer.py b/test/functional/wallet_signer.py index abbffe24d..b39f1b4d9 100755 --- a/test/functional/wallet_signer.py +++ b/test/functional/wallet_signer.py @@ -123,5 +123,16 @@ class SignerTest(BitcoinTestFramework): assert_equal(address_info['ismine'], True) assert_equal(address_info['hdkeypath'], "m/44'/1'/0'/0/0") + self.log.info('Test signerdisplayaddress') + result = hww.signerdisplayaddress(address1) + assert_equal(result, {"address": address1}) + + # Handle error thrown by script + self.set_mock_result(self.nodes[1], "2") + assert_raises_rpc_error(-1, 'RunCommandParseJSON error', + hww.signerdisplayaddress, address1 + ) + self.clear_mock_result(self.nodes[1]) + if __name__ == '__main__': SignerTest().main() From d4b0107d68a91ed4d1a5c78c8ca76251329d3f3c Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Sun, 4 Aug 2019 23:26:01 +0200 Subject: [PATCH 14/15] rpc: send: support external signer --- src/util/error.cpp | 4 + src/util/error.h | 2 + src/wallet/external_signer.cpp | 51 ++++++++++++ src/wallet/external_signer.h | 7 ++ src/wallet/external_signer_scriptpubkeyman.h | 2 + src/wallet/rpcwallet.cpp | 6 +- src/wallet/scriptpubkeyman.cpp | 1 + src/wallet/wallet.cpp | 1 + test/functional/mocks/signer.py | 26 ++++++ test/functional/wallet_signer.py | 85 +++++++++++++++++++- 10 files changed, 180 insertions(+), 5 deletions(-) diff --git a/src/util/error.cpp b/src/util/error.cpp index 76fac4d39..48c81693f 100644 --- a/src/util/error.cpp +++ b/src/util/error.cpp @@ -31,6 +31,10 @@ bilingual_str TransactionErrorString(const TransactionError err) return Untranslated("Specified sighash value does not match value stored in PSBT"); case TransactionError::MAX_FEE_EXCEEDED: return Untranslated("Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)"); + case TransactionError::EXTERNAL_SIGNER_NOT_FOUND: + return Untranslated("External signer not found"); + case TransactionError::EXTERNAL_SIGNER_FAILED: + return Untranslated("External signer failed to sign"); // no default case, so the compiler can warn about missing cases } assert(false); diff --git a/src/util/error.h b/src/util/error.h index 6633498d2..4cc35eb1f 100644 --- a/src/util/error.h +++ b/src/util/error.h @@ -30,6 +30,8 @@ enum class TransactionError { PSBT_MISMATCH, SIGHASH_MISMATCH, MAX_FEE_EXCEEDED, + EXTERNAL_SIGNER_NOT_FOUND, + EXTERNAL_SIGNER_FAILED, }; bilingual_str TransactionErrorString(const TransactionError error); diff --git a/src/wallet/external_signer.cpp b/src/wallet/external_signer.cpp index baf97700e..339611176 100644 --- a/src/wallet/external_signer.cpp +++ b/src/wallet/external_signer.cpp @@ -3,6 +3,10 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include +#include +#include +#include +#include #include ExternalSigner::ExternalSigner(const std::string& command, const std::string& fingerprint, std::string chain, std::string name): m_command(command), m_fingerprint(fingerprint), m_chain(chain), m_name(name) {} @@ -65,4 +69,51 @@ UniValue ExternalSigner::GetDescriptors(int account) return RunCommandParseJSON(m_command + " --fingerprint \"" + m_fingerprint + "\"" + NetworkArg() + " getdescriptors --account " + strprintf("%d", account)); } +bool ExternalSigner::SignTransaction(PartiallySignedTransaction& psbtx, std::string& error) +{ + // Serialize the PSBT + CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION); + ssTx << psbtx; + + // Check if signer fingerprint matches any input master key fingerprint + bool match = false; + for (unsigned int i = 0; i < psbtx.inputs.size(); ++i) { + const PSBTInput& input = psbtx.inputs[i]; + for (auto entry : input.hd_keypaths) { + if (m_fingerprint == strprintf("%08x", ReadBE32(entry.second.fingerprint))) match = true; + } + } + + if (!match) { + error = "Signer fingerprint " + m_fingerprint + " does not match any of the inputs:\n" + EncodeBase64(ssTx.str()); + return false; + } + + std::string command = m_command + " --stdin --fingerprint \"" + m_fingerprint + "\"" + NetworkArg(); + std::string stdinStr = "signtx \"" + EncodeBase64(ssTx.str()) + "\""; + + const UniValue signer_result = RunCommandParseJSON(command, stdinStr); + + if (find_value(signer_result, "error").isStr()) { + error = find_value(signer_result, "error").get_str(); + return false; + } + + if (!find_value(signer_result, "psbt").isStr()) { + error = "Unexpected result from signer"; + return false; + } + + PartiallySignedTransaction signer_psbtx; + std::string signer_psbt_error; + if (!DecodeBase64PSBT(signer_psbtx, find_value(signer_result, "psbt").get_str(), signer_psbt_error)) { + error = strprintf("TX decode failed %s", signer_psbt_error); + return false; + } + + psbtx = signer_psbtx; + + return true; +} + #endif diff --git a/src/wallet/external_signer.h b/src/wallet/external_signer.h index 198a939d3..4b9711107 100644 --- a/src/wallet/external_signer.h +++ b/src/wallet/external_signer.h @@ -10,6 +10,8 @@ #include #include +struct PartiallySignedTransaction; + class ExternalSignerException : public std::runtime_error { public: using std::runtime_error::runtime_error; @@ -60,6 +62,11 @@ public: //! @param[out] UniValue see doc/external-signer.md UniValue GetDescriptors(int account); + //! Sign PartiallySignedTransaction on the device. + //! Calls ` signtransaction` and passes the PSBT via stdin. + //! @param[in,out] psbt PartiallySignedTransaction to be signed + bool SignTransaction(PartiallySignedTransaction& psbt, std::string& error); + #endif }; diff --git a/src/wallet/external_signer_scriptpubkeyman.h b/src/wallet/external_signer_scriptpubkeyman.h index 1cde143ef..e60d7b800 100644 --- a/src/wallet/external_signer_scriptpubkeyman.h +++ b/src/wallet/external_signer_scriptpubkeyman.h @@ -26,6 +26,8 @@ class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan static ExternalSigner GetExternalSigner(); bool DisplayAddress(const CScript scriptPubKey, const ExternalSigner &signer) const; + + TransactionError FillPSBT(PartiallySignedTransaction& psbt, int sighash_type = 1 /* SIGHASH_ALL */, bool sign = true, bool bip32derivs = false, int* n_signed = nullptr) const override; }; #endif diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 9d61f6fbe..bfc42ac1b 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -4195,8 +4195,10 @@ static RPCHelpMan send() // Make a blank psbt PartiallySignedTransaction psbtx(rawTx); - // Fill transaction with our data and sign - bool complete = true; + // First fill transaction with our data without signing, + // so external signers are not asked sign more than once. + bool complete; + pwallet->FillPSBT(psbtx, complete, SIGHASH_ALL, false, true); const TransactionError err = pwallet->FillPSBT(psbtx, complete, SIGHASH_ALL, true, false); if (err != TransactionError::OK) { throw JSONRPCTransactionError(err); diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 4630603f8..efb408c16 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include //! Value for the first BIP 32 hardened derivation. Can be used as a bit mask and as a value. See BIP 32 for more details. diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 7c1543806..08e480225 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include