Merge bitcoin/bitcoin#32682: wallet: have external signer use PSBT error code EXTERNAL_SIGNER_NOT_FOUND

9dfc61d95f0082672a9b90528386e6bcd7014a78 test: detect no external signer connected (Sjors Provoost)
0a4ee93529d68a31f3ba6c7c6009954be47bbbd6 wallet: use PSBTError::EXTERNAL_SIGNER_NOT_FOUND (Sjors Provoost)
8ba2f9b7c8a6c6a91cc718d256354f7a73083b68 refactor: use util::Result for GetExternalSigner() (Sjors Provoost)

Pull request description:

  When attempting to sign a transaction involving an external signer, if the device isn't connected we throw an `std::runtime_error`. This prevents the (mainly GUI) code that's actually supposed to handle this case from running.

  This PR returns a `PSBTError::EXTERNAL_SIGNER_NOT_FOUND` instead of throwing.

  The first commit is a refactor to have `GetExternalSigner()` return a `util::Result<ExternalSigner>` so the caller can decide how to handle the error. There are two other places where call `GetExternalSigner()` which this PR doesn't change (which I think is fine there).

  Before:
  ![before](https://github.com/user-attachments/assets/2e08863b-fe76-479f-9cc0-60571b357a27)

  After (the translation already exist):
  ![after](https://github.com/user-attachments/assets/0e91c7ed-7d44-4030-beec-20d1694c270c)

  Fixes #32426

  Additionally use `LogWarning` instead of `std::cerr` for both a missing signer and failure to sign.

ACKs for top commit:
  achow101:
    ACK 9dfc61d95f0082672a9b90528386e6bcd7014a78
  brunoerg:
    code review ACK 9dfc61d95f0082672a9b90528386e6bcd7014a78

Tree-SHA512: 22515f4f0b4f50cb0ef532b729e247f11a68be9c90e384942d4277087b2e76806a1cdaa57fb51d5883dacf0a428e5279674aab37cce8c0d3d7de0f96346b8233

src/wallet/external_signer_scriptpubkeyman.cpp

@@ -45,14 +45,14 @@ bool ExternalSignerScriptPubKeyMan::SetupDescriptor(WalletBatch& batch, std::uni
     return true;
 }
 
-ExternalSigner ExternalSignerScriptPubKeyMan::GetExternalSigner() {
+ util::Result<ExternalSigner> ExternalSignerScriptPubKeyMan::GetExternalSigner() {
     const std::string command = gArgs.GetArg("-signer", "");
-    if (command == "") throw std::runtime_error(std::string(__func__) + ": restart bitcoind with -signer=<cmd>");
+    if (command == "") return util::Error{Untranslated("restart bitcoind with -signer=<cmd>")};
     std::vector<ExternalSigner> 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<PSBTError> 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

src/wallet/external_signer_scriptpubkeyman.h

@@ -8,6 +8,7 @@
 #include <wallet/scriptpubkeyman.h>
 
 #include <memory>
+#include <util/result.h>
 
 struct bilingual_str;
 
@@ -27,7 +28,7 @@ class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan
   */
   bool SetupDescriptor(WalletBatch& batch, std::unique_ptr<Descriptor>desc);
 
-  static ExternalSigner GetExternalSigner();
+  static util::Result<ExternalSigner> GetExternalSigner();
 
   /**
   * Display address on the device and verify that the returned value matches.

src/wallet/wallet.cpp

@@ -2575,8 +2575,9 @@ util::Result<void> 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");
 

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)

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()