diff --git a/src/wallet/external_signer_scriptpubkeyman.cpp b/src/wallet/external_signer_scriptpubkeyman.cpp index 7268354f1a9..c2b1b6048a8 100644 --- a/src/wallet/external_signer_scriptpubkeyman.cpp +++ b/src/wallet/external_signer_scriptpubkeyman.cpp @@ -45,14 +45,14 @@ bool ExternalSignerScriptPubKeyMan::SetupDescriptor(WalletBatch& batch, std::uni return true; } -ExternalSigner ExternalSignerScriptPubKeyMan::GetExternalSigner() { + util::Result ExternalSignerScriptPubKeyMan::GetExternalSigner() { const std::string command = gArgs.GetArg("-signer", ""); - if (command == "") throw std::runtime_error(std::string(__func__) + ": restart bitcoind with -signer="); + if (command == "") return util::Error{Untranslated("restart bitcoind with -signer=")}; std::vector signers; ExternalSigner::Enumerate(command, signers, Params().GetChainTypeString()); - if (signers.empty()) throw std::runtime_error(std::string(__func__) + ": No external signers found"); + if (signers.empty()) return util::Error{Untranslated("No external signers found")}; // TODO: add fingerprint argument instead of failing in case of multiple signers. - if (signers.size() > 1) throw std::runtime_error(std::string(__func__) + ": More than one external signer found. Please connect only one at a time."); + if (signers.size() > 1) return util::Error{Untranslated("More than one external signer found. Please connect only one at a time.")}; return signers[0]; } @@ -93,9 +93,15 @@ std::optional ExternalSignerScriptPubKeyMan::FillPSBT(PartiallySigned } if (complete) return {}; - std::string strFailReason; - if(!GetExternalSigner().SignTransaction(psbt, strFailReason)) { - tfm::format(std::cerr, "Failed to sign: %s\n", strFailReason); + auto signer{GetExternalSigner()}; + if (!signer) { + LogWarning("%s", util::ErrorString(signer).original); + return PSBTError::EXTERNAL_SIGNER_NOT_FOUND; + } + + std::string failure_reason; + if(!signer->SignTransaction(psbt, failure_reason)) { + LogWarning("Failed to sign: %s\n", failure_reason); return PSBTError::EXTERNAL_SIGNER_FAILED; } if (finalize) FinalizePSBT(psbt); // This won't work in a multisig setup diff --git a/src/wallet/external_signer_scriptpubkeyman.h b/src/wallet/external_signer_scriptpubkeyman.h index 66c2dd9d170..27c0258ea51 100644 --- a/src/wallet/external_signer_scriptpubkeyman.h +++ b/src/wallet/external_signer_scriptpubkeyman.h @@ -8,6 +8,7 @@ #include #include +#include struct bilingual_str; @@ -27,7 +28,7 @@ class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan */ bool SetupDescriptor(WalletBatch& batch, std::unique_ptrdesc); - static ExternalSigner GetExternalSigner(); + static util::Result GetExternalSigner(); /** * Display address on the device and verify that the returned value matches. diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index bdfe8d66592..20f0aae2e3f 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2575,8 +2575,9 @@ util::Result CWallet::DisplayAddress(const CTxDestination& dest) if (signer_spk_man == nullptr) { continue; } - ExternalSigner signer = ExternalSignerScriptPubKeyMan::GetExternalSigner(); - return signer_spk_man->DisplayAddress(dest, signer); + auto signer{ExternalSignerScriptPubKeyMan::GetExternalSigner()}; + if (!signer) throw std::runtime_error(util::ErrorString(signer).original); + return signer_spk_man->DisplayAddress(dest, *signer); } return util::Error{_("There is no ScriptPubKeyManager for this address")}; } @@ -3548,11 +3549,12 @@ void CWallet::SetupDescriptorScriptPubKeyMans() return true; })) throw std::runtime_error("Error: cannot process db transaction for descriptors setup"); } else { - ExternalSigner signer = ExternalSignerScriptPubKeyMan::GetExternalSigner(); + auto signer = ExternalSignerScriptPubKeyMan::GetExternalSigner(); + if (!signer) throw std::runtime_error(util::ErrorString(signer).original); // TODO: add account parameter int account = 0; - UniValue signer_res = signer.GetDescriptors(account); + UniValue signer_res = signer->GetDescriptors(account); if (!signer_res.isObject()) throw std::runtime_error(std::string(__func__) + ": Unexpected result"); diff --git a/test/functional/mocks/no_signer.py b/test/functional/mocks/no_signer.py new file mode 100755 index 00000000000..04318dac793 --- /dev/null +++ b/test/functional/mocks/no_signer.py @@ -0,0 +1,29 @@ +#!/usr/bin/env python3 +# Copyright (c) 2025-present 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 argparse +import json +import sys + +def enumerate(args): + sys.stdout.write(json.dumps([])) + +parser = argparse.ArgumentParser(prog='./no_signer.py', description='No external signer connected 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) + + +if not sys.stdin.isatty(): + buffer = sys.stdin.read() + if buffer and buffer.rstrip() != "": + sys.argv.extend(buffer.rstrip().split(" ")) + +args = parser.parse_args() + +args.func(args) diff --git a/test/functional/wallet_signer.py b/test/functional/wallet_signer.py index 3159f66641e..208e4c07752 100755 --- a/test/functional/wallet_signer.py +++ b/test/functional/wallet_signer.py @@ -23,6 +23,10 @@ class WalletSignerTest(BitcoinTestFramework): path = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'mocks', 'signer.py') return sys.executable + " " + path + def mock_no_connected_signer_path(self): + path = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'mocks', 'no_signer.py') + return sys.executable + " " + path + def mock_invalid_signer_path(self): path = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'mocks', 'invalid_signer.py') return sys.executable + " " + path @@ -52,6 +56,7 @@ class WalletSignerTest(BitcoinTestFramework): def run_test(self): self.test_valid_signer() + self.test_disconnected_signer() self.restart_node(1, [f"-signer={self.mock_invalid_signer_path()}", "-keypool=10"]) self.test_invalid_signer() self.restart_node(1, [f"-signer={self.mock_multi_signers_path()}", "-keypool=10"]) @@ -234,6 +239,28 @@ class WalletSignerTest(BitcoinTestFramework): # ) # self.clear_mock_result(self.nodes[4]) + def test_disconnected_signer(self): + self.log.info('Test disconnected external signer') + + # First create a wallet with the signer connected + self.nodes[1].createwallet(wallet_name='hww_disconnect', disable_private_keys=True, external_signer=True) + hww = self.nodes[1].get_wallet_rpc('hww_disconnect') + assert_equal(hww.getwalletinfo()["external_signer"], True) + + # Fund wallet + self.nodes[0].sendtoaddress(hww.getnewaddress(address_type="bech32m"), 1) + self.generate(self.nodes[0], 1) + + # Restart node with no signer connected + self.log.debug(f"-signer={self.mock_no_connected_signer_path()}") + self.restart_node(1, [f"-signer={self.mock_no_connected_signer_path()}", "-keypool=10"]) + self.nodes[1].loadwallet('hww_disconnect') + hww = self.nodes[1].get_wallet_rpc('hww_disconnect') + + # Try to spend + dest = hww.getrawchangeaddress() + assert_raises_rpc_error(-25, "External signer not found", hww.send, outputs=[{dest:0.5}]) + def test_invalid_signer(self): self.log.debug(f"-signer={self.mock_invalid_signer_path()}") self.log.info('Test invalid external signer') @@ -243,7 +270,7 @@ class WalletSignerTest(BitcoinTestFramework): self.log.debug(f"-signer={self.mock_multi_signers_path()}") self.log.info('Test multiple external signers') - assert_raises_rpc_error(-1, "GetExternalSigner: More than one external signer found", self.nodes[1].createwallet, wallet_name='multi_hww', disable_private_keys=True, external_signer=True) + assert_raises_rpc_error(-1, "More than one external signer found", self.nodes[1].createwallet, wallet_name='multi_hww', disable_private_keys=True, external_signer=True) if __name__ == '__main__': WalletSignerTest(__file__).main()