Move external signer out of wallet module

This commit moves the ExternalSigner class and RPC methods out of the wallet module.

The enumeratesigners RPC can be used without a wallet since #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.

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.

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 \

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) {}
 

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
+#ifndef BITCOIN_EXTERNAL_SIGNER_H
-#define BITCOIN_WALLET_EXTERNAL_SIGNER_H
+#define BITCOIN_EXTERNAL_SIGNER_H
 
 #include <stdexcept>
 #include <string>
@@ -48,7 +48,7 @@ public:
     //! @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 @@ public:
     //! 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 @@ public:
 #endif
 };
 
-#endif // BITCOIN_WALLET_EXTERNAL_SIGNER_H
+#endif // BITCOIN_EXTERNAL_SIGNER_H

src/rpc/external_signer.cpp

@@ -3,13 +3,11 @@
 // 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 <rpc/protocol.h>
-#include <wallet/rpcwallet.h>
-#include <wallet/wallet.h>
 
 #ifdef ENABLE_EXTERNAL_SIGNER
 
@@ -33,7 +31,7 @@ static RPCHelpMan enumeratesigners()
         RPCExamples{""},
         [](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 +44,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 +53,18 @@ static RPCHelpMan enumeratesigners()
     };
 }
 
-static RPCHelpMan signerdisplayaddress()
+void RegisterSignerRPCCommands(CRPCTable &t)
 {
-    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()
-{
-
 // 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

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

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

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 @@ 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) {
-                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); }

src/wallet/rpcsigner.h

@@ -1,25 +0,0 @@
-// 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 <span.h>
-#include <util/system.h>
-#include <vector>
-
-#ifdef ENABLE_EXTERNAL_SIGNER
-
-class CRPCCommand;
-
-namespace interfaces {
-class Chain;
-class Handler;
-}
-
-Span<const CRPCCommand> GetSignerRPCCommands();
-
-#endif // ENABLE_EXTERNAL_SIGNER
-
-#endif //BITCOIN_WALLET_RPCSIGNER_H

src/wallet/rpcwallet.cpp

@@ -4526,6 +4526,48 @@ static RPCHelpMan upgradewallet()
     };
 }
 
+#ifdef ENABLE_EXTERNAL_SIGNER
+static RPCHelpMan walletdisplayaddress()
+{
+    return RPCHelpMan{
+        "walletdisplayaddress",
+        "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::OBJ,"","",
+            {
+                {RPCResult::Type::STR, "address", "The address as confirmed by the signer"},
+            }
+        },
+        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_MISC_ERROR, "Failed to display address");
+            }
+
+            UniValue result(UniValue::VOBJ);
+            result.pushKV("address", request.params[0].get_str());
+            return result;
+        }
+    };
+}
+#endif // ENABLE_EXTERNAL_SIGNER
+
 RPCHelpMan abortrescan();
 RPCHelpMan dumpprivkey();
 RPCHelpMan importprivkey();
@@ -4602,6 +4644,9 @@ static const CRPCCommand commands[] =
     { "wallet",             &unloadwallet,                   },
     { "wallet",             &upgradewallet,                  },
     { "wallet",             &walletcreatefundedpsbt,         },
+#ifdef ENABLE_EXTERNAL_SIGNER
+    { "wallet",             &walletdisplayaddress,           },
+#endif // ENABLE_EXTERNAL_SIGNER
     { "wallet",             &walletlock,                     },
     { "wallet",             &walletpassphrase,               },
     { "wallet",             &walletpassphrasechange,         },

src/wallet/scriptpubkeyman.cpp

@@ -13,7 +13,7 @@
 #include <util/system.h>
 #include <util/time.h>
 #include <util/translation.h>
-#include <wallet/external_signer.h>
+#include <external_signer.h>
 #include <wallet/scriptpubkeyman.h>
 
 #include <optional>

src/wallet/wallet.h

@@ -22,7 +22,7 @@
 #include <wallet/coinselection.h>
 #include <wallet/crypter.h>
 #include <wallet/scriptpubkeyman.h>
-#include <wallet/external_signer.h>
+#include <external_signer.h>
 #include <wallet/walletdb.h>
 #include <wallet/walletutil.h>
 

test/functional/rpc_signer.py

@@ -0,0 +1,79 @@
+#!/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.
+See also wallet_signer.py for tests that require wallet context.
+"""
+import os
+import platform
+
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import (
+    assert_equal,
+    assert_raises_rpc_error,
+)
+
+
+class RPCSignerTest(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 = 4
+
+        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_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()}")
+
+        assert_raises_rpc_error(-1, 'Error: restart bitcoind with -signer=<cmd>',
+            self.nodes[0].enumeratesigners
+        )
+
+        # Handle script missing:
+        assert_raises_rpc_error(-1, 'execve failed: No such file or directory',
+            self.nodes[3].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(-1, 'fingerprint not found',
+            self.nodes[1].enumeratesigners
+        )
+        self.clear_mock_result(self.nodes[1])
+
+        result = self.nodes[1].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__':
+    RPCSignerTest().main()

test/functional/test_runner.py

@@ -111,6 +111,7 @@ BASE_SCRIPTS = [
     'wallet_listtransactions.py --legacy-wallet',
     'wallet_listtransactions.py --descriptors',
     'feature_taproot.py',
+    'rpc_signer.py',
     'wallet_signer.py --descriptors',
     # vv Tests less than 60s vv
     'p2p_sendheaders.py',

test/functional/wallet_signer.py

@@ -5,6 +5,7 @@
 """Test external signer.
 
 Verify that a bitcoind node can use an external signer command
+See also rpc_signer.py for tests without wallet context.
 """
 import os
 import platform
@@ -16,7 +17,7 @@ from test_framework.util import (
 )
 
 
-class SignerTest(BitcoinTestFramework):
+class WalletSignerTest(BitcoinTestFramework):
     def mock_signer_path(self):
         path = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'mocks', 'signer.py')
         if platform.system() == "Windows":
@@ -25,18 +26,16 @@ class SignerTest(BitcoinTestFramework):
             return path
 
     def set_test_params(self):
-        self.num_nodes = 4
+        self.num_nodes = 2
 
         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()
+        self.skip_if_no_wallet()
 
     def set_mock_result(self, node, res):
         with open(os.path.join(node.cwd, "mock_result"), "w", encoding="utf8") as f:
@@ -48,28 +47,6 @@ 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=<cmd>',
-            self.nodes[0].enumeratesigners
-        )
-
-        # Handle script missing:
-        assert_raises_rpc_error(-1, 'execve failed: No such file or directory',
-            self.nodes[3].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 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)
@@ -81,11 +58,6 @@ class SignerTest(BitcoinTestFramework):
         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()
-        assert_equal(len(result['signers']), 2)
-        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')
@@ -123,14 +95,14 @@ 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')
+        self.log.info('Test walletdisplayaddress')
-        result = hww.signerdisplayaddress(address1)
+        result = hww.walletdisplayaddress(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
+            hww.walletdisplayaddress, address1
         )
         self.clear_mock_result(self.nodes[1])
 
@@ -214,4 +186,4 @@ class SignerTest(BitcoinTestFramework):
         # self.clear_mock_result(self.nodes[4])
 
 if __name__ == '__main__':
-    SignerTest().main()
+    WalletSignerTest().main()