wallet: relax external_signer flag constraints #33112

Sjors commented at 10:11 AM on August 1, 2025: member

The external_signer indicates that an external signer device may be called via HWI or equivalent application.

When it was initially introduced some additional constraints were placed on wallets with this flag: it had to be a descriptor wallet and watch-only. Also the flag could not be added or removed later.

The constraints aren't a problem for the main supported and documented use case of connecting a single hardware wallet and using it just like a normal single sig Bitcoin Core wallet.

But they get in the way of MuSig2 support, see https://github.com/Sjors/bitcoin/pull/91.

This pull request drops the following constraints:

disable_private_keys is no longer mandatory (but still default)
external_signer flag is now mutable

Changing the external_signer flag reloads the wallet so that its descriptor ScriptPubKeyMans are recreated using the new setting.

Additionally it does the following:

make the blank option for createwallet consistent with regular wallets by not importing keys from the connected signer
avoid going through createTransaction for external signers (otherwise the GUI breaks)
create an ExternalSignerScriptPubKeyMan when importing a descriptor into an external signer wallet, instead of requiring an additional reload

This should have no noticeable effect on the default single sig use case described above.

Finally we add test coverage for spending from an imported hot key descriptor in an external signer wallet. The wallet signs with its own keys before involving the external signer.

DrahtBot added the label Wallet on Aug 1, 2025

DrahtBot commented at 10:11 AM on August 1, 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/33112.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	naiyoma, achow101, jeanpablojp
Stale ACK	rkrux, adyshimony, PraneethGunas

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#bitcoin-core/gui/915 (Defer transaction signing until user clicks Send by 151henry151)
#36114 (wallet: harden external signer psbt processing, revamp mock by Sjors)
#35445 (wallet, descriptor: Revert StringType::COMPAT for Miniscript expressions and drop the concept of a Descriptor ID that can be validated by achow101)
#35429 (wallet: avoid global access in external signer SPKM by w0xlt)
#35358 (external signer: verify PSBT is reliable after signing it by brunoerg)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

Disable the possibility of private keys (only watchonlys are possible in this mode). -> Disable the possibility of private keys (only watch-only wallets are possible in this mode). ["watchonlys" is a misspelling/awkward term that can confuse the intended meaning]

2026-08-28 07:20:37

fanquake commented at 10:31 AM on August 1, 2025: member

https://cirrus-ci.com/task/6572167942373376?logs=ci#L1621:

[06:14:38.327] /ci_container_base/src/wallet/rpc/wallet.cpp: In function ‘wallet::createwallet()::<lambda(const RPCHelpMan&, const JSONRPCRequest&)>’:
[06:14:38.327] /ci_container_base/src/wallet/rpc/wallet.cpp:420:45: error: ‘*(unsigned char*)((char*)&disable_private_keys + offsetof(std::optional<bool>,std::optional<bool>::<unnamed>.std::_Optional_base<bool, true, true>::<unnamed>))’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
[06:14:38.327]   420 |     if (disable_private_keys.has_value() && *disable_private_keys) {
[06:14:38.327]       |                                             ^~~~~~~~~~~~~~~~~~~~~
[06:14:38.327] cc1plus: all warnings being treated as errors
[06:14:38.328] gmake[2]: *** [src/wallet/CMakeFiles/bitcoin_wallet.dir/build.make:370: src/wallet/CMakeFiles/bitcoin_wallet.dir/rpc/wallet.cpp.o] Error 1

in src/wallet/rpc/wallet.cpp:383 in dee72e7419 outdated

 378 | @@ -379,9 +379,8 @@ static RPCHelpMan createwallet()
 379 |  {
 380 |      WalletContext& context = EnsureWalletContext(request.context);
 381 |      uint64_t flags = 0;
 382 | -    if (!request.params[1].isNull() && request.params[1].get_bool()) {
 383 | -        flags |= WALLET_FLAG_DISABLE_PRIVATE_KEYS;
 384 | -    }
 385 | +
 386 | +    std::optional<bool> disable_private_keys = request.params[1].isNull() ? std::nullopt : std::optional<bool>(request.params[1].get_bool());

maflcko commented at 11:01 AM on August 1, 2025:

nit: Would be good to use named args, while touching this: self.MaybeArg<bool>("disable_private_keys")

Sjors commented at 11:07 AM on August 1, 2025:

That looks much nicer indeed.

in src/wallet/rpc/wallet.cpp:420 in dee72e7419 outdated

 415 |  #else
 416 |          throw JSONRPCError(RPC_WALLET_ERROR, "Compiled without external signing support (required for external signing)");
 417 |  #endif
 418 |      }
 419 |  
 420 | +    if (disable_private_keys.has_value() && *disable_private_keys) {

maflcko commented at 11:02 AM on August 1, 2025:

not sure if this fixes the maybe-uninitialized false-positive from gcc, but you can try if (disable_private_keys.has_value() && disable_private_keys.value()) {, or add -Wno-error=maybe-uninitialized to the ci task config.

Sjors commented at 11:48 AM on August 1, 2025:

I'll try value_or(false)

Sjors force-pushed on Aug 1, 2025

DrahtBot added the label CI failed on Aug 1, 2025

DrahtBot commented at 11:49 AM on August 1, 2025: contributor

🚧 At least one of the CI tasks failed. Task previous releases, depends DEBUG: https://github.com/bitcoin/bitcoin/runs/47191506122 LLM reason (✨ experimental): The CI failed due to a compilation error caused by treating warnings as errors, specifically an uninitialized variable warning in wallet.cpp.

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

Sjors force-pushed on Aug 1, 2025

Sjors commented at 12:56 PM on August 1, 2025: member

Added 7beb338a0d4343a622236876c7d63d56bf7039e3 wallet: avoid createTransaction() with signer to prevent breaking GUI signing for private key enabled external signer wallets (even when they don't have other keys).

Sjors force-pushed on Aug 1, 2025

DrahtBot removed the label CI failed on Aug 1, 2025

in src/wallet/wallet.cpp:2881 in 2faf306123 outdated

2880 | @@ -2881,7 +2881,13 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
2881 |          // Only descriptor wallets can be created

rkrux commented at 2:00 PM on August 11, 2025:

In 2faf30612350fd90d9d3c44d1bb7b055addc8331 "wallet: don't import external keys at creation if blank"

Nit in commit message:

- For multisig setups than involve an external signer
+ For multisig setups that involve an external signer

in src/qt/walletmodel.cpp:None in 7beb338a0d outdated

 202 | @@ -203,7 +203,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact
 203 |          int nChangePosRet = -1;

rkrux commented at 2:12 PM on August 11, 2025:

In 7beb338a0d4343a622236876c7d63d56bf7039e3 "wallet: avoid createTransaction() with signer"

Nit in commit message:

- wallet: avoid createTransaction() with signer
+ wallet: avoid signing via createTransaction() with external signer

in test/functional/wallet_signer.py:None in 08f7813f53 outdated

  64 | @@ -65,23 +65,22 @@ def run_test(self):
  65 |      def test_valid_signer(self):
  66 |          self.log.debug(f"-signer={self.mock_signer_path()}")
  67 |  
  68 | -        # Create new wallets for an external signer.

rkrux commented at 2:18 PM on August 11, 2025:

In 08f7813f536c242b7a4b65d01cfbc73601846a25 "wallet: make watch-only optional for external signer"

This comment can stay?

Sjors commented at 7:34 AM on September 1, 2025:

Brought it back.

in test/functional/wallet_signer.py:None in 08f7813f53 outdated

  76 | +        # Private keys are disabled by default
  77 | +        assert_equal(hww.getwalletinfo()["private_keys_enabled"], False)
  78 | +
  79 |          # Flag can't be set afterwards (could be added later for non-blank descriptor based watch-only wallets)
  80 | -        self.nodes[1].createwallet(wallet_name='not_hww', disable_private_keys=True, external_signer=False)
  81 | +        self.nodes[1].createwallet(wallet_name='not_hww', external_signer=False)

rkrux commented at 2:31 PM on August 11, 2025:

In 08f7813f536c242b7a4b65d01cfbc73601846a25 "wallet: make watch-only optional for external signer"

Do you intend to add a test for an external signer wallet with private keys enabled in a later PR?

Sjors commented at 6:23 AM on August 15, 2025:

I think that'll become more relevant, and probably easier to test, after MuSig2 support lands.

rkrux commented at 2:34 PM on August 11, 2025: contributor

ACK a9734039a7a34e38145927f02b891685f96ab9e8

Agree with the intent to relax these constraints for external signer wallets.

Sjors force-pushed on Sep 1, 2025

Sjors commented at 7:34 AM on September 1, 2025: member

Rebased and addressed nits.

rkrux approved

rkrux commented at 11:59 AM on September 2, 2025: contributor

re-ACK 03978530ad8dc9124307d2ffc7d64c24b784be0e

git range-diff a973403...0397853

DrahtBot added the label Needs rebase on Feb 10, 2026

Sjors force-pushed on Feb 17, 2026

DrahtBot removed the label Needs rebase on Feb 17, 2026

in test/functional/wallet_signer.py:75 in 7d3fc16736 outdated

  75 |  
  76 | -        # Flag can't be set afterwards (could be added later for non-blank descriptor based watch-only wallets)
  77 | -        self.nodes[1].createwallet(wallet_name='not_hww', disable_private_keys=True, external_signer=False)
  78 | +        # Private keys are disabled by default
  79 | +        assert_equal(hww.getwalletinfo()["private_keys_enabled"], False)
  80 | +

adyshimony commented at 10:50 PM on February 26, 2026:

Maybe add a check that create the wallet with explicitly values upon creation?

# check that private keys can be explicitly enabled for external signer wallets
self.nodes[1].createwallet(wallet_name='hww_hot', external_signer=True, disable_private_keys=False)
hww_hot = self.nodes[1].get_wallet_rpc('hww_hot')
assert_equal(hww_hot.getwalletinfo()["external_signer"], True)
assert_equal(hww_hot.getwalletinfo()["private_keys_enabled"], True)

naiyoma commented at 1:56 PM on March 8, 2026:

+1 on testing when disable_private_keys=False

in test/functional/wallet_signer.py:82 in 7d3fc16736 outdated

  84 |          assert_equal(not_hww.getwalletinfo()["external_signer"], False)
  85 | -        assert_raises_rpc_error(-8, "Wallet flag is immutable: external_signer", not_hww.setwalletflag, "external_signer", True)
  86 | -
  87 | +        not_hww.setwalletflag("external_signer", True)
  88 | +        assert_equal(not_hww.getwalletinfo()["external_signer"], True)
  89 |

adyshimony commented at 10:53 PM on February 26, 2026:

Check the private_keys_enabled is true by default for this case:

assert_equal(not_hww.getwalletinfo()["private_keys_enabled"], True)

adyshimony commented at 11:03 PM on February 26, 2026:

Make sure those paths for external signer are raising errors:

# when external_signer enabled, sendtoaddress/sendmany path is disabled.
assert_raises_rpc_error(-4, "Error: Private keys are disabled for this wallet",
    not_hww.sendtoaddress, self.nodes[0].getnewaddress(), 0.01)
assert_raises_rpc_error(-4, "Error: Private keys are disabled for this wallet",
    not_hww.sendmany, "", {self.nodes[0].getnewaddress(): 0.01})

adyshimony commented at 11:12 PM on February 26, 2026: none

ACK 7d3fc167362a

Built and ran successfully all tests.

Qt UX tested on regtest with mock signer:

Create Wallet defaults with signer detected are correct.
Checkbox interactions behave as expected when toggling external_signer.
getwalletinfo for default external signer wallet shows external_signer=true, private_keys_enabled=false.
Creating with external_signer=true and disable_private_keys manually unchecked gives external_signer=true, private_keys_enabled=true.
setwalletflag "external_signer" set to true/false succeeds and is reflected in getwalletinfo.

DrahtBot requested review from rkrux on Feb 26, 2026

naiyoma commented at 2:15 PM on March 8, 2026: contributor

Concept ACK

Reviewed 3e57405bf6d1b2d75cf831537f364ec3c6092614 b990dbb504fd1b140332ca7c13f92673f74f5735 0ba76bf0a75dc7e60803dcb1e85a8492bb9397eb

and tested 500fc7a0bddee2ed7b59a8228e13637ccebd5e31 createwallet on bitcoin-qt

Before this pr disable_private_keys is always checked and grayed out, so there's no way to uncheck it external signer always watch-only, no exceptions

and on cli

"private_keys_enabled": false,
"flags": [
    "last_hardened_xpub_cached",
    "disable_private_keys",
    "descriptor_wallet",
    "external_signer"
  ],

After PR, disabled_private_keys is checked by default, but can be unchecked

if left checked:
"disable_private_keys" in flags
"private_keys_enabled": false
same as before

and when Unchecked "disable_private_keys" absent "private_keys_enabled": true new state

and on cli

"descriptors": true,                                                                                                                                                                  
  "external_signer": true,                                                                                                                                                              
  "blank": false,                                                                                                                                                                       
  "birthtime": 1772971109,                                                                                                                                                              
  "flags": [                                                                                                                                                                            
    "last_hardened_xpub_cached",                                                                                                                                                        
    "descriptor_wallet",                                                                                                                                                                
    "external_signer"                                                                                                                                                                   
  ],

achow101 commented at 10:02 PM on April 30, 2026: member

Concept ACK

Sjors force-pushed on May 1, 2026

Sjors renamed this:
~~wallet: relax external_signer flag constraints~~
wallet: relax external_signer flag constraints, add musig2 test (partial)
on May 1, 2026

Sjors commented at 10:35 AM on May 1, 2026: member

Rebased for silent merge conflict with #34049.

I've been testing this again in light of https://github.com/Sjors/bitcoin/pull/91, which also helped refresh my memory.

I dropped the first documentation commit 3e57405bf6d1b2d75cf831537f364ec3c6092614, because #33765 is doing a more thorough job there.

Added test coverage, including a new wallet_signer_musig2.py. This only imports a musig(hot wallet, external signer) descriptor for now. Spending from it requires too many other changes that are better left for a followup.

in src/wallet/wallet.cpp:3132 in c05e9c1326

3128 | +        // Don't generate or import keys for a blank wallet
3129 | +        if (!(wallet_creation_flags & WALLET_FLAG_BLANK_WALLET) && (
3130 | +            // Fetch keys from an external signer; or
3131 | +             (wallet_creation_flags & WALLET_FLAG_EXTERNAL_SIGNER) ||
3132 | +            // Generate them, unless private keys are disabled
3133 | +            !(wallet_creation_flags & (WALLET_FLAG_DISABLE_PRIVATE_KEYS)))

rkrux commented at 1:36 PM on May 4, 2026:

In c05e9c13266f5004b1345cd1a967d2d57a4e9ae6 "wallet: don't import external keys at creation if blank"

Redundant parenthesis.

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 747f8ef1e0..b905b9800f 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -2910,7 +2910,7 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
             // Fetch keys from an external signer; or
              (wallet_creation_flags & WALLET_FLAG_EXTERNAL_SIGNER) ||
             // Generate them, unless private keys are disabled
-            !(wallet_creation_flags & (WALLET_FLAG_DISABLE_PRIVATE_KEYS)))
+            !(wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS))
         ) {
             walletInstance->SetupDescriptorScriptPubKeyMans();
         }

in test/functional/wallet_signer.py:97 in 34c447940f

  94 |          assert_equal(not_hww.getwalletinfo()["external_signer"], False)
  95 |          # Without external_signer, private keys are enabled by default
  96 |          assert_equal(not_hww.getwalletinfo()["private_keys_enabled"], True)
  97 | -        assert_raises_rpc_error(-8, "Wallet flag is immutable: external_signer", not_hww.setwalletflag, "external_signer", True)
  98 | +        not_hww.setwalletflag("external_signer", True)
  99 | +        assert_equal(not_hww.getwalletinfo()["external_signer"], True)

rkrux commented at 1:50 PM on May 4, 2026:

In 34c447940fad77dfb291016390da80d8e4d80bb6 "wallet: make external_signer flag mutable"

Nit: the wallet name can be updated now - s/not_hww/not_hww_initially

diff --git a/test/functional/wallet_signer.py b/test/functional/wallet_signer.py
index 6a730bd66d..bb05bac0be 100755
--- a/test/functional/wallet_signer.py
+++ b/test/functional/wallet_signer.py
@@ -74,11 +74,11 @@ class WalletSignerTest(BitcoinTestFramework):
         assert_equal(hww.getwalletinfo()["private_keys_enabled"], False)
 
         # Flag can be set afterwards
-        self.nodes[1].createwallet(wallet_name='not_hww', external_signer=False)
-        not_hww = self.nodes[1].get_wallet_rpc('not_hww')
-        assert_equal(not_hww.getwalletinfo()["external_signer"], False)
-        not_hww.setwalletflag("external_signer", True)
-        assert_equal(not_hww.getwalletinfo()["external_signer"], True)
+        self.nodes[1].createwallet(wallet_name='not_hww_initially', external_signer=False)
+        not_hww_initially = self.nodes[1].get_wallet_rpc('not_hww_initially')
+        assert_equal(not_hww_initially.getwalletinfo()["external_signer"], False)
+        not_hww_initially.setwalletflag("external_signer", True)
+        assert_equal(not_hww_initially.getwalletinfo()["external_signer"], True)
 
         self.set_mock_result(self.nodes[1], '0 {"invalid json"}')
         assert_raises_rpc_error(-1, 'Unable to parse JSON',

Sjors commented at 9:31 AM on May 6, 2026:

Done

in src/wallet/rpc/spend.cpp:179 in e8a54514dd

 172 | @@ -173,8 +173,9 @@ UniValue SendMoney(CWallet& wallet, const CCoinControl &coin_control, std::vecto
 173 |      EnsureWalletIsUnlocked(wallet);
 174 |  
 175 |      // This function is only used by sendtoaddress and sendmany.
 176 | -    // This should always try to sign, if we don't have private keys, don't try to do anything here.
 177 | -    if (wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
 178 | +    // This should always try to sign, if we don't have (all) private keys, don't
 179 | +    // try to do anything here.
 180 | +    if (wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) || wallet.IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) {
 181 |          throw JSONRPCError(RPC_WALLET_ERROR, "Error: Private keys are disabled for this wallet");

rkrux commented at 1:54 PM on May 4, 2026:

In e8a54514ddf0599ede28d5b8ab3f0e2b3f27372a "wallet: avoid signing via createTransaction() with external signer"

This has no effect until the next commit when WALLET_FLAG_EXTERNAL_SIGNER no longer implies WALLET_FLAG_DISABLE_PRIVATE_KEYS.

After the next commit, the associated error here ("Private keys are disabled for this wallet") doesn't seem correct if it's an external signer.

Sjors commented at 9:31 AM on May 6, 2026:

Indeed. I also split this change into its own commit 996064c6db85023e0f8aa4f91277ac494821292d and added a test.

in test/functional/wallet_signer_musig2.py:32 in 8e7ea99972

  27 | +# an xpub for which it already has the matching private key material.
  28 | +
  29 | +class WalletSignerMuSig2Test(BitcoinTestFramework):
  30 | +    def mock_signer_path(self):
  31 | +        path = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'mocks', 'signer.py')
  32 | +        return sys.executable + " " + path

rkrux commented at 2:19 PM on May 4, 2026:

This is duplicated with wallet_signer.py. Can't these be a part of wallet_signer.py?

Sjors commented at 8:52 AM on May 6, 2026:

The MuSig2 test is going to grow by quite a lot, at least in my current (very rough) draft: https://github.com/Sjors/bitcoin/blob/2025/06/musig2-power/test/functional/wallet_signer_musig2.py

But I can move these helpers to the test framework. Done in 0b3ddcd516301a790afcf387ffdbcb7878ca4eb5.

in test/functional/wallet_signer_musig2.py:35 in 8e7ea99972

  30 | +    def mock_signer_path(self):
  31 | +        path = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'mocks', 'signer.py')
  32 | +        return sys.executable + " " + path
  33 | +
  34 | +    def set_test_params(self):
  35 | +        self.num_nodes = 2

rkrux commented at 2:20 PM on May 4, 2026:

2 nodes don't seem to be required, only one (self.nodes[1]) is used.

Sjors commented at 9:32 AM on May 6, 2026:

Fixed.

rkrux commented at 2:25 PM on May 4, 2026: contributor

Code review at 8e7ea9997293df451e98e39300fc75b83329c08e

Sjors force-pushed on May 6, 2026

Sjors commented at 9:34 AM on May 6, 2026: member

Rebased (just in case) and addressed @w0xlt's feedback. In particular this adds 0b3ddcd516301a790afcf387ffdbcb7878ca4eb5 to move mock_signer_path to the test framework and split e8a54514ddf0599ede28d5b8ab3f0e2b3f27372a into 3359674e80e74c5c3566f3207eed3681333d1940 (GUI) and 996064c6db85023e0f8aa4f91277ac494821292d (RPC).

DrahtBot added the label Needs rebase on May 28, 2026

Sjors force-pushed on May 29, 2026

Sjors commented at 8:46 AM on May 29, 2026: member

Rebased after #28333.

DrahtBot removed the label Needs rebase on May 29, 2026

DrahtBot added the label CI failed on May 29, 2026

DrahtBot removed the label CI failed on May 29, 2026

w0xlt referenced this in commit 4fdd4d8d29 on Jun 11, 2026

fanquake referenced this in commit fa8e4700ba on Jun 23, 2026

Sjors force-pushed on Jun 23, 2026

Sjors commented at 3:07 PM on June 23, 2026: member

Rebased after #35424 absorbed a5eb9e13f258c001b3096a61cd6420830cd63bc2.

sedited requested review from adyshimony on Jul 24, 2026

sedited requested review from naiyoma on Jul 24, 2026

Sjors force-pushed on Aug 6, 2026

DrahtBot added the label Needs rebase on Aug 14, 2026

Sjors force-pushed on Aug 17, 2026

Sjors commented at 12:22 PM on August 17, 2026: member

Rebased after #35852.

DrahtBot removed the label Needs rebase on Aug 17, 2026

Sjors force-pushed on Aug 20, 2026

Sjors force-pushed on Aug 24, 2026

Sjors commented at 5:53 PM on August 24, 2026: member

Added wallet: upgrade to ExternalSignerScriptPubKeyMan in AddWalletDescriptor and removed reload workaround from the test.

jeanpablojp commented at 8:23 PM on August 25, 2026: contributor

Concept ACK

Built and ran the wallet and signer tests.

in src/wallet/wallet.h:161 in 5c4ff9ef2c outdated

 156 | @@ -157,7 +157,8 @@ inline constexpr uint64_t KNOWN_WALLET_FLAGS =
 157 |      |   WALLET_FLAG_EXTERNAL_SIGNER;
 158 |  
 159 |  inline constexpr uint64_t MUTABLE_WALLET_FLAGS =
 160 | -        WALLET_FLAG_AVOID_REUSE;
 161 | +        WALLET_FLAG_AVOID_REUSE
 162 | +    |   WALLET_FLAG_EXTERNAL_SIGNER;

jeanpablojp commented at 8:23 PM on August 25, 2026:

Took a watch-only wallet that already had the device's descriptors and set the flag. Right after, walletdisplayaddress fails with There is no ScriptPubKeyManager for this address and send returns a PSBT without calling the device. The SPKM subclass is chosen when it's constructed, and toggling doesn't rebuild the existing ones, so after unloadwallet/loadwallet, with nothing else changed, both work. The AddWalletDescriptor commit already handles this on the import path. Worth doing the same on toggle, or forcing a reload?

Sjors commented at 1:37 PM on August 26, 2026:

I tried to implement having the wallet reload all descriptors, but it's actually easier to just have the RPC automatically reload the whole wallet for this flag. So I went with that.

in src/wallet/wallet.cpp:3807 in 5c4ff9ef2c outdated

3802 | @@ -3810,7 +3803,12 @@ util::Result<std::reference_wrapper<DescriptorScriptPubKeyMan>> CWallet::AddWall
3803 |              return util::Error{util::ErrorString(spkm_res)};
3804 |          }
3805 |      } else {
3806 | -        auto new_spk_man = DescriptorScriptPubKeyMan::CreateFromImport(*this, desc, m_keypool_size, signing_provider);
3807 | +        std::unique_ptr<DescriptorScriptPubKeyMan> new_spk_man;
3808 | +        if (IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) {

jeanpablojp commented at 8:23 PM on August 25, 2026:

Imported a descriptor with the flag on, turned the flag off, imported another one, and without reloading I can't spend from either. Log excerpt:

  -- flag is OFF, wallet NOT reloaded
    spend the utxo imported while flag was ON    RAISED  External signer failed to sign (-25)
    spend the utxo imported while flag was OFF   RAISED  External signer failed to sign (-25)
  -- same wallet, after unload+load, flag still OFF
    spend the utxo imported while flag was ON    OK
    spend the utxo imported while flag was OFF   OK

The second one is what got me, since it was imported after I turned the flag off. FillPSBT returns on the first error, so the leftover external signer SPKM aborts the whole fill. On the previous head, 2e39d5c032, all four succeeded.

Sjors commented at 1:37 PM on August 26, 2026:

That should be fixed now, thanks to the reload.

in src/wallet/wallet.h:159 in 5c4ff9ef2c outdated

 156 | @@ -157,7 +157,8 @@ inline constexpr uint64_t KNOWN_WALLET_FLAGS =
 157 |      |   WALLET_FLAG_EXTERNAL_SIGNER;
 158 |  
 159 |  inline constexpr uint64_t MUTABLE_WALLET_FLAGS =

jeanpablojp commented at 8:23 PM on August 25, 2026:

On a build with -DENABLE_EXTERNAL_SIGNER=OFF I set the flag on an ordinary wallet and it no longer loads on that binary, not even after a restart, with External signer wallet being loaded without external signer support compiled. Since unsetting the flag requires loading the wallet, the simplest way back is a binary built with support. On the merge base the same call returns Wallet flag is immutable: external_signer. There's no ENABLE_EXTERNAL_SIGNER guard here or in setwalletflag, would one make sense?

Sjors commented at 1:37 PM on August 26, 2026:

Added a guard for completeness.

Sjors force-pushed on Aug 26, 2026

DrahtBot added the label CI failed on Aug 26, 2026

DrahtBot commented at 3:13 PM on August 26, 2026: contributor

🚧 At least one of the CI tasks failed. Task fuzzer,address,undefined,integer: https://github.com/bitcoin/bitcoin/actions/runs/32975318518/job/98198506411 LLM reason (✨ experimental): CI failed due to a fuzz-test crash from UBSan detecting an unsigned integer overflow in MemPoolFeeRateEstimator::Read (mempool_estimator.cpp:210:62) during policy_estimator_io.

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

wallet: don't import external keys at creation if blank

There's no need to treat external signer wallets different in this
regard. When the user sets the 'blank' flag, don't generate or
import keys.

For multisig setups that involve an external signer, it may be useful
to start from a blank wallet and manually import descriptors.

dabe9c036e

wallet: avoid signing via createTransaction() with external signer

External signer enabled wallets should always use the process PSBT flow.
Avoid going through CreateTransaction.

This has no effect until a later commit where WALLET_FLAG_EXTERNAL_SIGNER
no longer implies WALLET_FLAG_DISABLE_PRIVATE_KEYS. Without this change
signing with the GUI would break for external signers with private keys
enabled.

2f91efb26f

wallet: make watch-only optional for external signer

Before this change the external_signer flag required the wallet to be watch-only.
This precludes multisig setups in which we hold a hot key.

Remove this as a requirement, but disable private keys by default. This leaves
the typical (and only documented) use case of a single external signer unaffected.

034375fc9c

wallet: make external_signer flag mutable

With the removal of legacy wallets and the relaxing of restrictions
in the previous commit, it's no longer a problem to toggle this flag.

1413480f2e

wallet: extract load and unload wallet RPC helpers

A later commit reuses these helpers to reload a wallet. This does not change behavior.

af00263548

wallet: report whether flag changes require reload

A later commit uses this signal to reload the wallet after changing
flags that affect in-memory state. Existing callers ignore the return
value, so this does not change behavior.

f470ba91ce

wallet: reload wallet when external signer flag changes

Have setwalletflag unload and reload the wallet when a flag setter
reports this is needed. Reuse the normal wallet loading path to
recreate descriptor ScriptPubKeyMans.

Document which flags trigger a reload and warn users to avoid
concurrent wallet RPC clients while changing them.

cd3bfc9692

test: move mock signer path helper to the test framework

Both rpc_signer.py and wallet_signer.py defined identical
mock_signer_path() helpers; the upcoming wallet_signer_musig2.py test
needs the same helper. Move it to BitcoinTestFramework.

517824b93c

Sjors force-pushed on Aug 26, 2026

DrahtBot closed this on Aug 26, 2026

DrahtBot reopened this on Aug 26, 2026

DrahtBot removed the label CI failed on Aug 26, 2026

PraneethGunas commented at 11:42 PM on August 27, 2026: none

Concept ACK. This greatly helps unblock using external signers and hot keys in the same wallet for a multisig setup

in src/wallet/wallet.cpp:3811 in 443f93ae1a outdated

3806 | @@ -3807,7 +3807,12 @@ util::Result<std::reference_wrapper<DescriptorScriptPubKeyMan>> CWallet::AddWall
3807 |              return util::Error{util::ErrorString(spkm_res)};
3808 |          }
3809 |      } else {
3810 | -        auto new_spk_man = DescriptorScriptPubKeyMan::CreateFromImport(*this, desc, m_keypool_size, signing_provider);
3811 | +        std::unique_ptr<DescriptorScriptPubKeyMan> new_spk_man;
3812 | +        if (IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) {

PraneethGunas commented at 11:53 PM on August 27, 2026:

Now that 034375fc9c allows private keys in a signer wallet, a descriptor holding a hot key also gets an ExternalSignerScriptPubKeyMan here, and at LoadDescriptorScriptPubKeyMan. Its FillPSBT() goes straight to the device when sign=true and never falls back.

Tested on a Ledger Flex, regtest. Spending a hot key UTXO gives:

error code: -25, External signer failed to sign debug.log: Signer fingerprint 42036eee does not match any of the inputs:

Should SPKM selection be per descriptor, based on whether the wallet holds the key, rather than per wallet?

Sjors commented at 7:20 AM on August 28, 2026:

Don't do that :-)

I added 5a7c4309da24d9dd3ff5a8f7edc3d316cd9355c0 to have the wallet first sign with its own keys, and only try the external signer if the result is unsigned.

However we shouldn't encourage users to put hot and cold descriptors into the same wallet, so this isn't documented.

Sjors commented at 11:28 AM on August 28, 2026:

While working on this, I realized our external signer mock can't properly test these more complicated scenarios. That's only going to get worse with MuSig2, so I opened #36114 to rewrite it.

PraneethGunas commented at 7:40 PM on August 28, 2026:

Confirmed fixed on a Ledger Flex, thanks. If mixing hot and cold in one wallet isn't encouraged, what's the intended layout for a multisig where one cosigner is an external signer and another is a hot key, one wallet per key?

Sjors commented at 8:57 AM on August 31, 2026:

Ideally there's a single multisig descriptor for which the wallet has one key, e.g. musig2(core_xprv, ledger_xpub). So you start with a blank wallet, with an usused() descriptor, then call gethdkey to export the xprv for m/87'/0'/0', get the device xpub, construct the descriptor and then import it. Having to copy an xprv around is not ideal, so https://github.com/Sjors/bitcoin/pull/91 contains a commit that lets you use the xpub instead, and the wallet figures it out on import.

wallet: upgrade to ExternalSignerScriptPubKeyMan in AddWalletDescriptor

CWallet::AddWalletDescriptor created a plain DescriptorScriptPubKeyMan
even when WALLET_FLAG_EXTERNAL_SIGNER was set. Create the
external-signer variant immediately so newly imported descriptors can
use address display and signing without requiring an unload/reload
cycle.

f4dd05644f

wallet: sign with own keys before using the external signer

ExternalSignerScriptPubKeyMan::FillPSBT went straight to the external
signer whenever sign is set. Now that a signer wallet can hold private
keys, a descriptor with a hot key gets an ExternalSignerScriptPubKeyMan
as well, and its signature was never made.

Let the base class fill and sign first, and only involve the signer if
an input that belongs to this descriptor is still unsigned.

5a7c4309da

Sjors force-pushed on Aug 28, 2026

Sjors commented at 7:20 AM on August 28, 2026: member

added handling for hot descriptors, see #33112 (review)
dropped test: add MuSig2 external signer wallet test to trim the scope a bit.

Sjors renamed this:
~~wallet: relax external_signer flag constraints, add musig2 test (partial)~~
wallet: relax external_signer flag constraints
on Aug 28, 2026

PraneethGunas commented at 12:31 AM on August 29, 2026: none

ACK f4dd056, 5a7c430