wallet: have external signer use PSBT error code EXTERNAL_SIGNER_NOT

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: before

After (the translation already exist): after

Fixes #32426

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

refactor: use util::Result for GetExternalSigner()

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

8ba2f9b7c8

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

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.

DrahtBot added the label Wallet on Jun 5, 2025

brunoerg commented at 2:26 PM on June 9, 2025: contributor

Concept ACK

achow101 commented at 9:45 PM on June 9, 2025: member

ACK 0a4ee93529d68a31f3ba6c7c6009954be47bbbd6

DrahtBot requested review from brunoerg on Jun 9, 2025

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:

diff --git a/test/functional/wallet_signer.py b/test/functional/wallet_signer.py
index 58705a23a3..07f18124c9 100755
--- a/test/functional/wallet_signer.py
+++ b/test/functional/wallet_signer.py
@@ -259,7 +259,7 @@ class WalletSignerTest(BitcoinTestFramework):

         # Try to spend
         dest = hww.getrawchangeaddress()
-        assert_raises_rpc_error(-25, "External signer not found", hww.send, outputs=[{dest:0.5}])
+        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.

DrahtBot requested review from brunoerg on Jun 9, 2025

brunoerg approved

brunoerg commented at 9:48 PM on June 9, 2025: contributor

code review ACK 0a4ee93529d68a31f3ba6c7c6009954be47bbbd6

test: detect no external signer connected 9dfc61d95f

achow101 commented at 12:12 AM on June 14, 2025: member

ACK 9dfc61d95f0082672a9b90528386e6bcd7014a78

DrahtBot requested review from brunoerg on Jun 14, 2025

Sjors commented at 11:25 AM on June 16, 2025: member

cc @theStack, @naiyoma

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.

DrahtBot requested review from brunoerg on Jun 16, 2025

brunoerg approved

brunoerg commented at 6:16 PM on June 16, 2025: contributor

code review ACK 9dfc61d95f0082672a9b90528386e6bcd7014a78

achow101 merged this on Jun 17, 2025

achow101 closed this on Jun 17, 2025

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, ?

luke-jr referenced this in commit 4ed94a8093 on Jul 17, 2025

luke-jr referenced this in commit 2bfde66f64 on Jul 17, 2025

luke-jr referenced this in commit ed1131ebb2 on Jul 17, 2025

stringintech referenced this in commit f19e7819e9 on Jul 19, 2025

alexanderwiederin referenced this in commit 9997c3d993 on Jul 25, 2025

alexanderwiederin referenced this in commit 8d6ebb9c4e on Jul 28, 2025

alexanderwiederin referenced this in commit 533a4585b5 on Jul 28, 2025

yuvicc referenced this in commit 22f55cf11d on Aug 26, 2025

bug-castercv502 referenced this in commit d4bd8a1259 on Sep 28, 2025

stickies-v referenced this in commit a19c56cd7c on Nov 4, 2025

Kino1994 referenced this in commit 2627952e66 on Jun 28, 2026

BigcoinBGC referenced this in commit b6ddb8cb5a on Jun 30, 2026

bitcoin locked this on Jul 30, 2026

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

Code Coverage & Benchmarks

Reviews