wallet: relax external_signer flag constraints, add musig2 test (partial) #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 requests drops the following constraints:

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

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)

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

Finally we add the beginnings of a musig(hot key, external signer) test. For now it just imports the descriptor and calls walletdisplayaddress. It takes advantage of the changes in this PR.

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 for information on the review process.

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

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)
#33765 (doc: update interface, --stdin flag, signtx (#31005) by cobratbq)
#28333 (wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively by achow101)
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)

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:

only watchonlys are possible -> only watch-only wallets are possible [“watchonlys” is not standard English and can hinder understanding]

2026-05-01 10:35:47

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

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.

c05e9c1326

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 the next commit when 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.

e8a54514dd

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.

0dce7586fc

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.

34c447940f

test: add MuSig2 external signer wallet test 8e7ea99972

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.