From 54569cc6d6f54788169061004026e62e1c08440e Mon Sep 17 00:00:00 2001 From: fanquake Date: Tue, 13 Apr 2021 14:55:58 +0800 Subject: [PATCH 1/7] refactor: move all signer code inside ENABLE_EXTERNAL_SIGNER #ifdefs --- src/external_signer.cpp | 6 +++--- src/external_signer.h | 7 ++++--- src/wallet/wallet.cpp | 2 +- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/external_signer.cpp b/src/external_signer.cpp index b82dcc503d..be8b06eaba 100644 --- a/src/external_signer.cpp +++ b/src/external_signer.cpp @@ -9,6 +9,8 @@ #include #include +#ifdef ENABLE_EXTERNAL_SIGNER + 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 @@ -16,8 +18,6 @@ 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 @@ -116,4 +116,4 @@ bool ExternalSigner::SignTransaction(PartiallySignedTransaction& psbtx, std::str return true; } -#endif +#endif // ENABLE_EXTERNAL_SIGNER diff --git a/src/external_signer.h b/src/external_signer.h index 17428ba2f9..15436213b2 100644 --- a/src/external_signer.h +++ b/src/external_signer.h @@ -10,6 +10,8 @@ #include #include +#ifdef ENABLE_EXTERNAL_SIGNER + struct PartiallySignedTransaction; class ExternalSignerException : public std::runtime_error { @@ -43,7 +45,6 @@ public: 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 @@ -66,8 +67,8 @@ public: //! Calls ` 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 diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index b00fa851fd..db05586eb6 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4586,7 +4586,7 @@ void CWallet::SetupDescriptorScriptPubKeyMans() } #else throw std::runtime_error(std::string(__func__) + ": Wallets with external signers require Boost::Process library."); -#endif +#endif // ENABLE_EXTERNAL_SIGNER } } From f4652bf1259d5c52ff0d500c732f40ba41256817 Mon Sep 17 00:00:00 2001 From: fanquake Date: Tue, 13 Apr 2021 15:02:10 +0800 Subject: [PATCH 2/7] refactor: add missing includes to external signer code --- src/external_signer.cpp | 3 +++ src/external_signer.h | 6 ++++-- src/rpc/external_signer.cpp | 3 +++ src/wallet/external_signer_scriptpubkeyman.cpp | 7 +++++++ src/wallet/external_signer_scriptpubkeyman.h | 2 ++ 5 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/external_signer.cpp b/src/external_signer.cpp index be8b06eaba..9278d07f49 100644 --- a/src/external_signer.cpp +++ b/src/external_signer.cpp @@ -9,6 +9,9 @@ #include #include +#include +#include + #ifdef ENABLE_EXTERNAL_SIGNER 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 index 15436213b2..070589dac0 100644 --- a/src/external_signer.h +++ b/src/external_signer.h @@ -5,11 +5,13 @@ #ifndef BITCOIN_EXTERNAL_SIGNER_H #define BITCOIN_EXTERNAL_SIGNER_H -#include -#include #include #include +#include +#include +#include + #ifdef ENABLE_EXTERNAL_SIGNER struct PartiallySignedTransaction; diff --git a/src/rpc/external_signer.cpp b/src/rpc/external_signer.cpp index 0f8f197ad8..05d4ce2c91 100644 --- a/src/rpc/external_signer.cpp +++ b/src/rpc/external_signer.cpp @@ -9,6 +9,9 @@ #include #include +#include +#include + #ifdef ENABLE_EXTERNAL_SIGNER static RPCHelpMan enumeratesigners() diff --git a/src/wallet/external_signer_scriptpubkeyman.cpp b/src/wallet/external_signer_scriptpubkeyman.cpp index a113c128d7..fe2c810afa 100644 --- a/src/wallet/external_signer_scriptpubkeyman.cpp +++ b/src/wallet/external_signer_scriptpubkeyman.cpp @@ -6,6 +6,13 @@ #include #include +#include +#include +#include +#include +#include +#include + #ifdef ENABLE_EXTERNAL_SIGNER bool ExternalSignerScriptPubKeyMan::SetupDescriptor(std::unique_ptr desc) diff --git a/src/wallet/external_signer_scriptpubkeyman.h b/src/wallet/external_signer_scriptpubkeyman.h index e60d7b8004..1786958912 100644 --- a/src/wallet/external_signer_scriptpubkeyman.h +++ b/src/wallet/external_signer_scriptpubkeyman.h @@ -8,6 +8,8 @@ #ifdef ENABLE_EXTERNAL_SIGNER #include +#include + class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan { public: From 8fdbb899b84a2be85e632e45f08b222db02395d9 Mon Sep 17 00:00:00 2001 From: fanquake Date: Tue, 13 Apr 2021 15:08:33 +0800 Subject: [PATCH 3/7] refactor: unify external wallet runtime errors Rather than 3 different messages that are confusing / leak implementation details, use a single message, that is similar to other wallet related messages. i.e: "Compiled without sqlite support (required for descriptor wallets)". --- src/wallet/rpcwallet.cpp | 2 +- src/wallet/wallet.cpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index a3e42a34d9..76460f106b 100644 --- a/src/wallet/rpcwallet.cpp +++ b/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 } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index db05586eb6..c297bc2f91 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4516,7 +4516,7 @@ void CWallet::LoadDescriptorScriptPubKeyMan(uint256 id, WalletDescriptor& desc) 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"); + throw std::runtime_error(std::string(__func__) + ": Compiled without external signing support (required for external signing)"); #endif } else { auto spk_manager = std::unique_ptr(new DescriptorScriptPubKeyMan(*this, desc)); @@ -4585,7 +4585,7 @@ void CWallet::SetupDescriptorScriptPubKeyMans() } } #else - throw std::runtime_error(std::string(__func__) + ": Wallets with external signers require Boost::Process library."); + throw std::runtime_error(std::string(__func__) + ": Compiled without external signing support (required for external signing)"); #endif // ENABLE_EXTERNAL_SIGNER } } From 06a0673351282fff1673f3679a7cad9a7faaf987 Mon Sep 17 00:00:00 2001 From: fanquake Date: Tue, 13 Apr 2021 15:18:20 +0800 Subject: [PATCH 4/7] external_signer: remove ignore_errors from Enumerate() This is undocumented and unused. --- src/external_signer.cpp | 5 +---- src/external_signer.h | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/src/external_signer.cpp b/src/external_signer.cpp index 9278d07f49..4809c5abf2 100644 --- a/src/external_signer.cpp +++ b/src/external_signer.cpp @@ -21,19 +21,17 @@ const std::string ExternalSigner::NetworkArg() const return " --chain " + m_chain; } -bool ExternalSigner::Enumerate(const std::string& command, std::vector& signers, std::string chain, bool ignore_errors) +bool ExternalSigner::Enumerate(const std::string& command, std::vector& signers, std::string chain) { // 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)); } @@ -42,7 +40,6 @@ bool ExternalSigner::Enumerate(const std::string& command, std::vector& signers, std::string chain, bool ignore_errors = false); + static bool Enumerate(const std::string& command, std::vector& signers, std::string chain); //! Display address on the device. Calls ` displayaddress --desc `. //! @param[in] descriptor Descriptor specifying which address to display. From aaa4e5a45bd9ec5563ffa7b9e0d46d2de3cb9242 Mon Sep 17 00:00:00 2001 From: fanquake Date: Tue, 13 Apr 2021 18:12:39 +0800 Subject: [PATCH 5/7] wallet: remove CWallet::GetExternalSigner() --- src/wallet/wallet.cpp | 15 +-------------- src/wallet/wallet.h | 3 --- 2 files changed, 1 insertion(+), 17 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c297bc2f91..332e7b1397 100644 --- a/src/wallet/wallet.cpp +++ b/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="); - 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 - 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; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 5bf3c91bec..c4acef8705 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -845,9 +845,6 @@ 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 /** 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); From 9e0b199b976617edeb1c58d4203df5f83a26c1e3 Mon Sep 17 00:00:00 2001 From: fanquake Date: Tue, 13 Apr 2021 19:41:50 +0800 Subject: [PATCH 6/7] external_signer: use const where appropriate --- src/external_signer.cpp | 16 ++++++++-------- src/external_signer.h | 6 +++--- src/rpc/external_signer.cpp | 4 ++-- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/external_signer.cpp b/src/external_signer.cpp index 4809c5abf2..9325daeab2 100644 --- a/src/external_signer.cpp +++ b/src/external_signer.cpp @@ -14,14 +14,14 @@ #ifdef ENABLE_EXTERNAL_SIGNER -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) {} +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; } -bool ExternalSigner::Enumerate(const std::string& command, std::vector& signers, std::string chain) +bool ExternalSigner::Enumerate(const std::string& command, std::vector& signers, const std::string chain) { // Call enumerate const UniValue result = RunCommandParseJSON(command + " enumerate"); @@ -42,10 +42,10 @@ bool ExternalSigner::Enumerate(const std::string& command, std::vector& signers, std::string chain); + static bool Enumerate(const std::string& command, std::vector& signers, const std::string chain); //! Display address on the device. Calls ` displayaddress --desc `. //! @param[in] descriptor Descriptor specifying which address to display. @@ -63,7 +63,7 @@ public: //! Calls ` getdescriptors --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 ` signtransaction` and passes the PSBT via stdin. diff --git a/src/rpc/external_signer.cpp b/src/rpc/external_signer.cpp index 05d4ce2c91..08aa8d8dcb 100644 --- a/src/rpc/external_signer.cpp +++ b/src/rpc/external_signer.cpp @@ -38,12 +38,12 @@ static RPCHelpMan enumeratesigners() { const std::string command = gArgs.GetArg("-signer", ""); if (command == "") throw JSONRPCError(RPC_MISC_ERROR, "Error: restart bitcoind with -signer="); - std::string chain = gArgs.GetChainName(); + const std::string chain = gArgs.GetChainName(); UniValue signers_res = UniValue::VARR; try { std::vector 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); From c8f469c6d50a8db6d92f0aed47a5d1cc82f30f7f Mon Sep 17 00:00:00 2001 From: fanquake Date: Tue, 13 Apr 2021 19:55:59 +0800 Subject: [PATCH 7/7] external_signer: remove ExternalSignerException It's not clear why this need it's own exception class, as opposed to just throwing std::runtime_error(). --- src/external_signer.cpp | 9 +++++---- src/external_signer.h | 6 ------ src/rpc/external_signer.cpp | 2 +- 3 files changed, 6 insertions(+), 11 deletions(-) diff --git a/src/external_signer.cpp b/src/external_signer.cpp index 9325daeab2..f16d21fa60 100644 --- a/src/external_signer.cpp +++ b/src/external_signer.cpp @@ -9,6 +9,7 @@ #include #include +#include #include #include @@ -26,21 +27,21 @@ bool ExternalSigner::Enumerate(const std::string& command, std::vector enumerate const UniValue result = RunCommandParseJSON(command + " enumerate"); if (!result.isArray()) { - 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 (!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()) { - throw ExternalSignerException(strprintf("'%s' received invalid response, missing signer fingerprint", command)); + throw std::runtime_error(strprintf("'%s' received invalid response, missing signer fingerprint", command)); } const std::string fingerprintStr = fingerprint.get_str(); // Skip duplicate signer diff --git a/src/external_signer.h b/src/external_signer.h index 798662672e..b3b202091a 100644 --- a/src/external_signer.h +++ b/src/external_signer.h @@ -8,7 +8,6 @@ #include #include -#include #include #include @@ -16,11 +15,6 @@ struct PartiallySignedTransaction; -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 diff --git a/src/rpc/external_signer.cpp b/src/rpc/external_signer.cpp index 08aa8d8dcb..6ec2b1a07f 100644 --- a/src/rpc/external_signer.cpp +++ b/src/rpc/external_signer.cpp @@ -49,7 +49,7 @@ static RPCHelpMan enumeratesigners() 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);