Merge bitcoin#21467: Move external signer out of wallet module

fanquake · fanquake · commit f0b457212f98 · 2021-04-13T14:35:16.000+08:00
88d4d5f rpc: add help for enumeratesigners and walletdisplayaddress (Sjors Provoost) b0db187 ci: use --enable-external-signer instead of --with-boost-process (Sjors Provoost) b54b2e7 Move external signer out of wallet module (Sjors Provoost) Pull request description: In addition, this PR enables external signer testing on CI. This PR moves the ExternalSigner class and RPC methods out of the wallet module. The `enumeratesigners` RPC can be used without a wallet since bitcoin#21417. With additional modifications external signers could be used without a wallet in general, e.g. via `signrawtransaction`. The `signerdisplayaddress` RPC is ranamed to `walletdisplayaddress` because it requires wallet context. A future `displayaddress` RPC call without wallet context could take a descriptor argument. This commit fixes a `rpc_help.py` failure when configured with `--disable-wallet`. ACKs for top commit: ryanofsky: Code review ACK 88d4d5f fanquake: ACK 88d4d5f Tree-SHA512: 3242a24e22313aed97eee32a520bfcb1c17495ba32a2b8e06a5e151e2611320e2da5ef35b572d84623af0a49a210d2f9377a2531250868d1a0ccf3e144352a97
diff --git 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 --with-boost-process"
+export BITCOIN_CONFIG="--enable-glibc-back-compat --enable-reduce-exports CXXFLAGS=-Wno-psabi --enable-external-signer"
diff --git 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 --with-boost-process"
+export BITCOIN_CONFIG="--enable-zmq --with-gui=qt5 --enable-reduce-exports --enable-external-signer"
 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
@@ -15,4 +15,4 @@ export XCODE_BUILD_ID=11C505
 export RUN_UNIT_TESTS=false
 export RUN_FUNCTIONAL_TESTS=false
 export GOAL="deploy"
-export BITCOIN_CONFIG="--with-gui --enable-reduce-exports --with-boost-process"
+export BITCOIN_CONFIG="--with-gui --enable-reduce-exports --enable-external-signer"
diff --git 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"
 export GOAL="install"
-export BITCOIN_CONFIG="--with-gui --enable-reduce-exports --with-boost-process"
+export BITCOIN_CONFIG="--with-gui --enable-reduce-exports --enable-external-signer"
 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
@@ -11,4 +11,4 @@ export PACKAGES="clang llvm python3-zmq qtbase5-dev qttools5-dev-tools libevent-
 export DOCKER_NAME_TAG=ubuntu:20.04
 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++ --with-boost-process"
+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"
diff --git 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++ --with-boost-process"
+export BITCOIN_CONFIG="--enable-fuzz --with-sanitizers=fuzzer,address,undefined,integer CC=clang CXX=clang++ --enable-external-signer"
 export CCACHE_SIZE=200M
diff --git a/ci/test/00_setup_env_native_multiprocess.sh b/ci/test/00_setup_env_native_multiprocess.sh
@@ -11,6 +11,6 @@ export DOCKER_NAME_TAG=ubuntu:20.04
 export PACKAGES="cmake python3"
 export DEP_OPTS="MULTIPROCESS=1"
 export GOAL="install"
-export BITCOIN_CONFIG="--with-boost-process"
+export BITCOIN_CONFIG="--enable-external-signer"
 export TEST_RUNNER_ENV="BITCOIND=bitcoin-node"
 export RUN_SECURITY_TESTS="true"
diff --git 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 --with-boost-process"
+export BITCOIN_CONFIG="--enable-glibc-back-compat --enable-reduce-exports CC=clang-5.0 CXX=clang++-5.0 --enable-external-signer"
diff --git 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\" --with-boost-process"
+--enable-debug --disable-fuzz-binary  CFLAGS=\"-g0 -O2 -funsigned-char\" CXXFLAGS=\"-g0 -O2 -funsigned-char\" --enable-external-signer"
diff --git 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:20.04
 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++' --with-boost-process"
+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"
diff --git 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 --with-boost-process"
+export BITCOIN_CONFIG="--enable-reduce-exports --with-incompatible-bdb --enable-external-signer"
diff --git a/ci/test/00_setup_env_win64.sh b/ci/test/00_setup_env_win64.sh
@@ -13,7 +13,7 @@ export DPKG_ADD_ARCH="i386"
 export PACKAGES="python3 nsis g++-mingw-w64-x86-64 wine-binfmt wine64 wine32 file"
 export RUN_FUNCTIONAL_TESTS=false
 export GOAL="deploy"
-export BITCOIN_CONFIG="--enable-reduce-exports --disable-gui-tests --without-boost-process"
+export BITCOIN_CONFIG="--enable-reduce-exports --disable-gui-tests --disable-external-signer"
 
 # Compiler for MinGW-w64 causes false -Wreturn-type warning.
 # See https://sourceforge.net/p/mingw-w64/bugs/306/
diff --git a/doc/external-signer.md b/doc/external-signer.md
@@ -46,7 +46,7 @@ Display an address on the device:
 
 ```sh
 $ bitcoin-cli -rpcwallet=<wallet> getnewaddress
-$ bitcoin-cli -rpcwallet=<wallet> signerdisplayaddress <address>
+$ bitcoin-cli -rpcwallet=<wallet> walletdisplayaddress <address>
 ```
 
 Replace `<address>` with the result of `getnewaddress`.
@@ -166,6 +166,6 @@ The `createwallet` RPC calls:
 
 It then imports descriptors for all support address types, in a BIP44/49/84 compatible manner.
 
-The `displayaddress` RPC reuses some code from `getaddressinfo` on the provided address and obtains the inferred descriptor. It then calls `<cmd> --fingerprint=00000000 displayaddress --desc=<descriptor>`.
+The `walletdisplayaddress` RPC reuses some code from `getaddressinfo` on the provided address and obtains the inferred descriptor. It then calls `<cmd> --fingerprint=00000000 displayaddress --desc=<descriptor>`.
 
 `sendtoaddress` and `sendmany` check `inputs->bip32_derivs` to see if any inputs have the same `master_fingerprint` as the signer. If so, it calls `<cmd> --fingerprint=00000000 signtransaction <psbt>`. It waits for the device to return a (partially) signed psbt, tries to finalize it and broadcasts the transaction.
diff --git a/src/Makefile.am b/src/Makefile.am
@@ -144,6 +144,7 @@ BITCOIN_CORE_H = \
   core_memusage.h \
   cuckoocache.h \
   dbwrapper.h \
+  external_signer.h \
   flatfile.h \
   fs.h \
   httprpc.h \
@@ -267,13 +268,11 @@ BITCOIN_CORE_H = \
   wallet/crypter.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 \
   wallet/load.h \
-  wallet/rpcsigner.h \
   wallet/rpcwallet.h \
   wallet/salvage.h \
   wallet/scriptpubkeyman.h \
@@ -387,13 +386,11 @@ libbitcoin_wallet_a_SOURCES = \
   wallet/db.cpp \
   wallet/dump.cpp \
   wallet/external_signer_scriptpubkeyman.cpp \
-  wallet/external_signer.cpp \
   wallet/feebumper.cpp \
   wallet/fees.cpp \
   wallet/interfaces.cpp \
   wallet/load.cpp \
   wallet/rpcdump.cpp \
-  wallet/rpcsigner.cpp \
   wallet/rpcwallet.cpp \
   wallet/scriptpubkeyman.cpp \
   wallet/wallet.cpp \
@@ -520,6 +517,7 @@ libbitcoin_common_a_SOURCES = \
   compressor.cpp \
   core_read.cpp \
   core_write.cpp \
+  external_signer.cpp \
   key.cpp \
   key_io.cpp \
   merkleblock.cpp \
@@ -532,6 +530,7 @@ libbitcoin_common_a_SOURCES = \
   protocol.cpp \
   psbt.cpp \
   rpc/rawtransaction_util.cpp \
+  rpc/external_signer.cpp \
   rpc/util.cpp \
   scheduler.cpp \
   script/descriptor.cpp \
diff --git a/src/external_signer.cpp b/src/external_signer.cpp
@@ -7,7 +7,7 @@
 #include <psbt.h>
 #include <util/strencodings.h>
 #include <util/system.h>
-#include <wallet/external_signer.h>
+#include <external_signer.h>
 
 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) {}
 
diff --git a/src/external_signer.h b/src/external_signer.h
@@ -2,8 +2,8 @@
 // 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
+#ifndef BITCOIN_EXTERNAL_SIGNER_H
+#define BITCOIN_EXTERNAL_SIGNER_H
 
 #include <stdexcept>
 #include <string>
@@ -48,7 +48,7 @@ class ExternalSigner
     //! @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
+    //! @returns success
     static bool Enumerate(const std::string& command, std::vector<ExternalSigner>& signers, std::string chain, bool ignore_errors = false);
 
     //! Display address on the device. Calls `<command> displayaddress --desc <descriptor>`.
@@ -59,7 +59,7 @@ class ExternalSigner
     //! Get receive and change Descriptor(s) from device for a given account.
     //! Calls `<command> getdescriptors --account <account>`
     //! @param[in] account  which BIP32 account to use (e.g. `m/44'/0'/account'`)
-    //! @param[out] UniValue see doc/external-signer.md
+    //! @returns see doc/external-signer.md
     UniValue GetDescriptors(int account);
 
     //! Sign PartiallySignedTransaction on the device.
@@ -70,4 +70,4 @@ class ExternalSigner
 #endif
 };
 
-#endif // BITCOIN_WALLET_EXTERNAL_SIGNER_H
+#endif // BITCOIN_EXTERNAL_SIGNER_H
diff --git a/src/rpc/external_signer.cpp b/src/rpc/external_signer.cpp
@@ -3,20 +3,17 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <chainparamsbase.h>
-#include <key_io.h>
+#include <external_signer.h>
 #include <rpc/server.h>
 #include <rpc/util.h>
 #include <util/strencodings.h>
-#include <wallet/rpcsigner.h>
-#include <wallet/rpcwallet.h>
-#include <wallet/wallet.h>
+#include <rpc/protocol.h>
 
 #ifdef ENABLE_EXTERNAL_SIGNER
 
 static RPCHelpMan enumeratesigners()
 {
-    return RPCHelpMan{
-        "enumeratesigners",
+    return RPCHelpMan{"enumeratesigners",
         "Returns a list of external signers from -signer.",
         {},
         RPCResult{
@@ -30,10 +27,14 @@ static RPCHelpMan enumeratesigners()
                 }
             }
         },
-        RPCExamples{""},
-        [](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue {
+        RPCExamples{
+            HelpExampleCli("enumeratesigners", "")
+            + HelpExampleRpc("enumeratesigners", "")
+        },
+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
+        {
             const std::string command = gArgs.GetArg("-signer", "");
-            if (command == "") throw JSONRPCError(RPC_WALLET_ERROR, "Error: restart bitcoind with -signer=<cmd>");
+            if (command == "") throw JSONRPCError(RPC_MISC_ERROR, "Error: restart bitcoind with -signer=<cmd>");
             std::string chain = gArgs.GetChainName();
             UniValue signers_res = UniValue::VARR;
             try {
@@ -46,7 +47,7 @@ static RPCHelpMan enumeratesigners()
                     signers_res.push_back(signer_res);
                 }
             } catch (const ExternalSignerException& e) {
-                throw JSONRPCError(RPC_WALLET_ERROR, e.what());
+                throw JSONRPCError(RPC_MISC_ERROR, e.what());
             }
             UniValue result(UniValue::VOBJ);
             result.pushKV("signers", signers_res);
@@ -55,54 +56,18 @@ 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<CWallet> 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<const CRPCCommand> GetSignerRPCCommands()
+void RegisterSignerRPCCommands(CRPCTable &t)
 {
-
 // clang-format off
 static const CRPCCommand commands[] =
 { // category              actor (function)
   // --------------------- ------------------------
   { "signer",              &enumeratesigners,      },
-  { "signer",              &signerdisplayaddress,  },
 };
 // clang-format on
-    return MakeSpan(commands);
+    for (const auto& c : commands) {
+        t.appendCommand(c.name, &c);
+    }
 }
 
-
 #endif // ENABLE_EXTERNAL_SIGNER
diff --git a/src/rpc/register.h b/src/rpc/register.h
@@ -19,6 +19,8 @@ void RegisterMiscRPCCommands(CRPCTable &tableRPC);
 void RegisterMiningRPCCommands(CRPCTable &tableRPC);
 /** Register raw transaction RPC commands */
 void RegisterRawTransactionRPCCommands(CRPCTable &tableRPC);
+/** Register raw transaction RPC commands */
+void RegisterSignerRPCCommands(CRPCTable &tableRPC);
 
 static inline void RegisterAllCoreRPCCommands(CRPCTable &t)
 {
@@ -27,6 +29,9 @@ static inline void RegisterAllCoreRPCCommands(CRPCTable &t)
     RegisterMiscRPCCommands(t);
     RegisterMiningRPCCommands(t);
     RegisterRawTransactionRPCCommands(t);
+#ifdef ENABLE_EXTERNAL_SIGNER
+    RegisterSignerRPCCommands(t);
+#endif // ENABLE_EXTERNAL_SIGNER
 }
 
 #endif // BITCOIN_RPC_REGISTER_H
diff --git a/src/wallet/external_signer_scriptpubkeyman.cpp b/src/wallet/external_signer_scriptpubkeyman.cpp
@@ -3,7 +3,7 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <chainparams.h>
-#include <wallet/external_signer.h>
+#include <external_signer.h>
 #include <wallet/external_signer_scriptpubkeyman.h>
 
 #ifdef ENABLE_EXTERNAL_SIGNER
diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp
@@ -22,7 +22,6 @@
 #include <wallet/fees.h>
 #include <wallet/ismine.h>
 #include <wallet/load.h>
-#include <wallet/rpcsigner.h>
 #include <wallet/rpcwallet.h>
 #include <wallet/wallet.h>
 
@@ -520,17 +519,6 @@ class WalletClientImpl : public WalletClient
             }, 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) {
-                JSONRPCRequest wallet_request = request;
-                wallet_request.context = &m_context;
-                return command.actor(wallet_request, 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.h b/src/wallet/rpcsigner.h
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
diff --git a/test/functional/rpc_signer.py b/test/functional/rpc_signer.py
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
diff --git a/test/functional/wallet_signer.py b/test/functional/wallet_signer.py
