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

1. 
   Sjors commented at 10:06 am on June 5, 2025: member

   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:

   After (the translation already exist):

   Fixes #32426

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

2. refactor: use util::Result for GetExternalSigner()

   This commit does not change behavior, except that the error message no longer contains the function name.

   8ba2f9b7c8
3. wallet: use PSBTError::EXTERNAL_SIGNER_NOT_FOUND

   Instead of throwing a runtime error, let the caller decide how to handle a missing signer.
   
   GUI code was already in place to handle this, but it was unused until this commit.
   
   Fixes #32426
   
   Additionally use LogWarning instead of std::cerr.

   0a4ee93529
4. 
   DrahtBot commented at 10:06 am on June 5, 2025: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32682.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	achow101, brunoerg

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

5. DrahtBot added the label Wallet on Jun 5, 2025
6. 
   brunoerg commented at 2:26 pm on June 9, 2025: contributor

   Concept ACK
7. 
   achow101 commented at 9:45 pm on June 9, 2025: member

   ACK 0a4ee93529d68a31f3ba6c7c6009954be47bbbd6
8. DrahtBot requested review from brunoerg on Jun 9, 2025
9. in src/wallet/external_signer_scriptpubkeyman.cpp:99 in 0a4ee93529 outdated

    93@@ -94,11 +94,14 @@ std::optional<PSBTError> ExternalSignerScriptPubKeyMan::FillPSBT(PartiallySigned
    94     if (complete) return {};
    95 
    96     auto signer{GetExternalSigner()};
    97-    if (!signer) throw std::runtime_error(util::ErrorString(signer).original);
    98+    if (!signer) {
    99+        LogWarning("%s", util::ErrorString(signer).original);
   100+        return PSBTError::EXTERNAL_SIGNER_NOT_FOUND;
   

   ---

   ---

   

   brunoerg commented at 9:47 pm on June 9, 2025:

   Perhaps we could have a test for it since it doesn’t throw a runtime error anymore?

   ---

   

   Sjors commented at 7:44 am on June 13, 2025:

   Added a functional test.

   Note that this test would pass on the original code as well, with just a slightly different error:

   0diff --git a/test/functional/wallet_signer.py b/test/functional/wallet_signer.py
   1index 58705a23a3..07f18124c9 100755
   2--- a/test/functional/wallet_signer.py
   3+++ b/test/functional/wallet_signer.py
   4@@ -259,7 +259,7 @@ class WalletSignerTest(BitcoinTestFramework):
   5
   6         # Try to spend
   7         dest = hww.getrawchangeaddress()
   8-        assert_raises_rpc_error(-25, "External signer not found", hww.send, outputs=[{dest:0.5}])
   9+        assert_raises_rpc_error(-1, "GetExternalSigner: No external signers found", hww.send, outputs=[{dest:0.5}])
   

   A unit test might be able to cover PSBTError::EXTERNAL_SIGNER_NOT_FOUND directly.

10. DrahtBot requested review from brunoerg on Jun 9, 2025
11. brunoerg approved
12. 
    brunoerg commented at 9:48 pm on June 9, 2025: contributor

    code review ACK 0a4ee93529d68a31f3ba6c7c6009954be47bbbd6
13. test: detect no external signer connected 9dfc61d95f
14. 
    achow101 commented at 0:12 am on June 14, 2025: member

    ACK 9dfc61d95f0082672a9b90528386e6bcd7014a78
15. DrahtBot requested review from brunoerg on Jun 14, 2025
16. 
    Sjors commented at 11:25 am on June 16, 2025: member

    cc @theStack, @naiyoma
17. in test/functional/wallet_signer.py:256 in 9dfc61d95f

    251+        self.nodes[0].sendtoaddress(hww.getnewaddress(address_type="bech32m"), 1)
    252+        self.generate(self.nodes[0], 1)
    253+
    254+        # Restart node with no signer connected
    255+        self.log.debug(f"-signer={self.mock_no_connected_signer_path()}")
    256+        self.restart_node(1, [f"-signer={self.mock_no_connected_signer_path()}", "-keypool=10"])
    

    ---

    ---

    

    brunoerg commented at 12:35 pm on June 16, 2025:

    9dfc61d95f0082672a9b90528386e6bcd7014a78: Why do we need to setup -keypool=10 here?

    ---

    

    Sjors commented at 12:47 pm on June 16, 2025:

    To be consistent with extra_args, but I don’t remember why I added this back in the day. Possibly related with legacy wallet performance.
18. DrahtBot requested review from brunoerg on Jun 16, 2025
19. brunoerg approved
20. 
    brunoerg commented at 6:16 pm on June 16, 2025: contributor

    code review ACK 9dfc61d95f0082672a9b90528386e6bcd7014a78
21. achow101 merged this on Jun 17, 2025
22. achow101 closed this on Jun 17, 2025

    ---

23. 
    naiyoma commented at 9:18 am on June 27, 2025: contributor

    Post Merge TAck 9dfc61d I tested this by setting up HWI and using Trezor as my external signer I noticed while reviewing this that some tests have been commented out —> https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_signer.py#L81 and also https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_signer.py#L171 Perhaps a good follow-up would be to work on as well, ?

Contributors
Sjors DrahtBot brunoerg achow101 naiyoma

Labels
Wallet