Merge #21666: Miscellaneous external signer changes

c8f469c external_signer: remove ExternalSignerException (fanquake)
9e0b199 external_signer: use const where appropriate (fanquake)
aaa4e5a wallet: remove CWallet::GetExternalSigner() (fanquake)
06a0673 external_signer: remove ignore_errors from Enumerate() (fanquake)
8fdbb89 refactor: unify external wallet runtime errors (fanquake)
f4652bf refactor: add missing includes to external signer code (fanquake)
54569cc refactor: move all signer code inside ENABLE_EXTERNAL_SIGNER #ifdefs (fanquake)

Pull request description:

  These are a few followups after #21467.

ACKs for top commit:
  Sjors:
    tACK c8f469c
  instagibbs:
    utACK bitcoin/bitcoin@c8f469c

Tree-SHA512: 3d5ac5df81680075e71e0e4a7595c520d746c3e37f016cf168c1e10da15541ebb1595aecaf2c08575636e9ff77d499644cae53180232b7049cfae0b923106e4e

Author: fanquake

src/external_signer.cpp

@@ -9,43 +9,44 @@
 #include <util/system.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) {}
+#include <stdexcept>
+#include <string>
+#include <vector>
+
+#ifdef ENABLE_EXTERNAL_SIGNER
+
+ExternalSigner::ExternalSigner(const std::string& command, const std::string& fingerprint, const std::string chain, const std::string name): m_command(command), m_fingerprint(fingerprint), m_chain(chain), m_name(name) {}
 
 const std::string ExternalSigner::NetworkArg() const
 {
     return " --chain " + m_chain;
 }
 
-#ifdef ENABLE_EXTERNAL_SIGNER
-
-bool ExternalSigner::Enumerate(const std::string& command, std::vector<ExternalSigner>& signers, std::string chain, bool ignore_errors)
+bool ExternalSigner::Enumerate(const std::string& command, std::vector<ExternalSigner>& signers, const std::string chain)
 {
     // Call <command> 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));
+        throw std::runtime_error(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 std::runtime_error(strprintf("'%s' error", command));
             }
-            throw ExternalSignerException(strprintf("'%s' error: %s", command, error.getValStr()));
+            throw std::runtime_error(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));
+            throw std::runtime_error(strprintf("'%s' received invalid response, missing signer fingerprint", command));
         }
-        std::string fingerprintStr = fingerprint.get_str();
+        const std::string fingerprintStr = fingerprint.get_str();
         // Skip duplicate signer
         bool duplicate = false;
-        for (ExternalSigner signer : signers) {
+        for (const ExternalSigner& signer : signers) {
             if (signer.m_fingerprint.compare(fingerprintStr) == 0) duplicate = true;
         }
         if (duplicate) break;
@@ -64,7 +65,7 @@ UniValue ExternalSigner::DisplayAddress(const std::string& descriptor) const
     return RunCommandParseJSON(m_command + " --fingerprint \"" + m_fingerprint + "\"" + NetworkArg() + " displayaddress --desc \"" + descriptor + "\"");
 }
 
-UniValue ExternalSigner::GetDescriptors(int account)
+UniValue ExternalSigner::GetDescriptors(const int account)
 {
     return RunCommandParseJSON(m_command + " --fingerprint \"" + m_fingerprint + "\"" + NetworkArg() + " getdescriptors --account " + strprintf("%d", account));
 }
@@ -79,7 +80,7 @@ bool ExternalSigner::SignTransaction(PartiallySignedTransaction& psbtx, std::str
     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) {
+        for (const auto& entry : input.hd_keypaths) {
             if (m_fingerprint == strprintf("%08x", ReadBE32(entry.second.fingerprint))) match = true;
         }
     }
@@ -89,8 +90,8 @@ bool ExternalSigner::SignTransaction(PartiallySignedTransaction& psbtx, std::str
         return false;
     }
 
-    std::string command = m_command + " --stdin --fingerprint \"" + m_fingerprint + "\"" + NetworkArg();
-    std::string stdinStr = "signtx \"" + EncodeBase64(ssTx.str()) + "\"";
+    const std::string command = m_command + " --stdin --fingerprint \"" + m_fingerprint + "\"" + NetworkArg();
+    const std::string stdinStr = "signtx \"" + EncodeBase64(ssTx.str()) + "\"";
 
     const UniValue signer_result = RunCommandParseJSON(command, stdinStr);
 
@@ -116,4 +117,4 @@ bool ExternalSigner::SignTransaction(PartiallySignedTransaction& psbtx, std::str
     return true;
 }
 
-#endif
+#endif // ENABLE_EXTERNAL_SIGNER

src/external_signer.h

@@ -5,17 +5,15 @@
 #ifndef BITCOIN_EXTERNAL_SIGNER_H
 #define BITCOIN_EXTERNAL_SIGNER_H
 
-#include <stdexcept>
-#include <string>
 #include <univalue.h>
 #include <util/system.h>
 
-struct PartiallySignedTransaction;
+#include <string>
+#include <vector>
 
-class ExternalSignerException : public std::runtime_error {
-public:
-    using std::runtime_error::runtime_error;
-};
+#ifdef ENABLE_EXTERNAL_SIGNER
+
+struct PartiallySignedTransaction;
 
 //! Enables interaction with an external signing device or service, such as
 //! a hardware wallet. See doc/external-signer.md
@@ -30,7 +28,7 @@ class ExternalSigner
     //! @param[in] fingerprint  master key fingerprint of the signer
     //! @param[in] chain        "main", "test", "regtest" or "signet"
     //! @param[in] name         device name
-    ExternalSigner(const std::string& command, const std::string& fingerprint, std::string chain, std::string name);
+    ExternalSigner(const std::string& command, const std::string& fingerprint, const std::string chain, const std::string name);
 
     //! Master key fingerprint of the signer
     std::string m_fingerprint;
@@ -43,13 +41,12 @@ class ExternalSigner
 
     const std::string NetworkArg() const;
 
-#ifdef ENABLE_EXTERNAL_SIGNER
     //! Obtain a list of signers. Calls `<command> enumerate`.
     //! @param[in]              command the command which handles interaction with the external signer
     //! @param[in,out] signers  vector to which new signers (with a unique master key fingerprint) are added
     //! @param chain            "main", "test", "regtest" or "signet"
     //! @returns success
-    static bool Enumerate(const std::string& command, std::vector<ExternalSigner>& signers, std::string chain, bool ignore_errors = false);
+    static bool Enumerate(const std::string& command, std::vector<ExternalSigner>& signers, const std::string chain);
 
     //! Display address on the device. Calls `<command> displayaddress --desc <descriptor>`.
     //! @param[in] descriptor Descriptor specifying which address to display.
@@ -60,14 +57,14 @@ class ExternalSigner
     //! Calls `<command> getdescriptors --account <account>`
     //! @param[in] account  which BIP32 account to use (e.g. `m/44'/0'/account'`)
     //! @returns see doc/external-signer.md
-    UniValue GetDescriptors(int account);
+    UniValue GetDescriptors(const int account);
 
     //! Sign PartiallySignedTransaction on the device.
     //! Calls `<command> signtransaction` and passes the PSBT via stdin.
     //! @param[in,out] psbt  PartiallySignedTransaction to be signed
     bool SignTransaction(PartiallySignedTransaction& psbt, std::string& error);
-
-#endif
 };
 
+#endif // ENABLE_EXTERNAL_SIGNER
+
 #endif // BITCOIN_EXTERNAL_SIGNER_H

src/rpc/external_signer.cpp

@@ -9,6 +9,9 @@
 #include <util/strencodings.h>
 #include <rpc/protocol.h>
 
+#include <string>
+#include <vector>
+
 #ifdef ENABLE_EXTERNAL_SIGNER
 
 static RPCHelpMan enumeratesigners()
@@ -35,18 +38,18 @@ static RPCHelpMan enumeratesigners()
         {
             const std::string command = gArgs.GetArg("-signer", "");
             if (command == "") throw JSONRPCError(RPC_MISC_ERROR, "Error: restart bitcoind with -signer=<cmd>");
-            std::string chain = gArgs.GetChainName();
+            const std::string chain = gArgs.GetChainName();
             UniValue signers_res = UniValue::VARR;
             try {
                 std::vector<ExternalSigner> signers;
                 ExternalSigner::Enumerate(command, signers, chain);
-                for (ExternalSigner signer : signers) {
+                for (const 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) {
+            } catch (const std::exception& e) {
                 throw JSONRPCError(RPC_MISC_ERROR, e.what());
             }
             UniValue result(UniValue::VOBJ);

src/wallet/external_signer_scriptpubkeyman.cpp

@@ -6,6 +6,13 @@
 #include <external_signer.h>
 #include <wallet/external_signer_scriptpubkeyman.h>
 
+#include <iostream>
+#include <memory>
+#include <stdexcept>
+#include <string>
+#include <utility>
+#include <vector>
+
 #ifdef ENABLE_EXTERNAL_SIGNER
 
 bool ExternalSignerScriptPubKeyMan::SetupDescriptor(std::unique_ptr<Descriptor> desc)

src/wallet/external_signer_scriptpubkeyman.h

@@ -8,6 +8,8 @@
 #ifdef ENABLE_EXTERNAL_SIGNER
 #include <wallet/scriptpubkeyman.h>
 
+#include <memory>
+
 class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan
 {
   public:

src/wallet/rpcwallet.cpp

@@ -2750,7 +2750,7 @@ static RPCHelpMan createwallet()
 #ifdef ENABLE_EXTERNAL_SIGNER
         flags |= WALLET_FLAG_EXTERNAL_SIGNER;
 #else
-        throw JSONRPCError(RPC_WALLET_ERROR, "Configure with --enable-external-signer to use this");
+        throw JSONRPCError(RPC_WALLET_ERROR, "Compiled without external signing support (required for external signing)");
 #endif
     }
 

src/wallet/wallet.cpp

@@ -3594,19 +3594,6 @@ 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=<cmd>");
-    std::vector<ExternalSigner> 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
-
 bool CWallet::DisplayAddress(const CTxDestination& dest)
 {
 #ifdef ENABLE_EXTERNAL_SIGNER
@@ -3619,7 +3606,7 @@ bool CWallet::DisplayAddress(const CTxDestination& dest)
     if (signer_spk_man == nullptr) {
         return false;
     }
-    ExternalSigner signer = GetExternalSigner(); // TODO: move signer in spk_man
+    ExternalSigner signer = ExternalSignerScriptPubKeyMan::GetExternalSigner();
     return signer_spk_man->DisplayAddress(scriptPubKey, signer);
 #else
     return false;
@@ -4516,7 +4503,7 @@ void CWallet::LoadDescriptorScriptPubKeyMan(uint256 id, WalletDescriptor& desc)
         auto spk_manager = std::unique_ptr<ScriptPubKeyMan>(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");
+        throw std::runtime_error(std::string(__func__) + ": Compiled without external signing support (required for external signing)");
 #endif
     } else {
         auto spk_manager = std::unique_ptr<ScriptPubKeyMan>(new DescriptorScriptPubKeyMan(*this, desc));
@@ -4585,8 +4572,8 @@ void CWallet::SetupDescriptorScriptPubKeyMans()
             }
         }
 #else
-        throw std::runtime_error(std::string(__func__) + ": Wallets with external signers require Boost::Process library.");
-#endif
+        throw std::runtime_error(std::string(__func__) + ": Compiled without external signing support (required for external signing)");
+#endif // ENABLE_EXTERNAL_SIGNER
     }
 }
 

src/wallet/wallet.h

@@ -845,9 +845,6 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
 
     std::vector<OutputGroup> GroupOutputs(const std::vector<COutput>& 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
     /** 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);