wallet: relax external_signer flag constraints

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 more advanced constructions, which are becoming increasingly practical especially once MuSig2 support lands with #29675. See https://github.com/Sjors/bitcoin/pull/91 and https://github.com/bitcoin-core/HWI/pull/794 for a collection of changes on top of that which facilitate MuSig2 with a hardware wallet. And see #24861.

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)
update external-signer.md mainly to use named arguments and point out a GUI setting

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

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
ACK	rkrux

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30354 (doc: external-signer, example ‘createwallet’ RPC call requires eight argument 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.

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

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

0[06:14:38.327] /ci_container_base/src/wallet/rpc/wallet.cpp: In function ‘wallet::createwallet()::<lambda(const RPCHelpMan&, const JSONRPCRequest&)>’:
1[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]
2[06:14:38.327]   420 |     if (disable_private_keys.has_value() && *disable_private_keys) {
3[06:14:38.327]       |                                             ^~~~~~~~~~~~~~~~~~~~~
4[06:14:38.327] cc1plus: all warnings being treated as errors
5[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.

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.

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

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

in src/qt/walletmodel.cpp:203 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:

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

in test/functional/wallet_signer.py:68 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:77 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.

doc: improve external-signer.md

- use bitcoin rpc instead of bitcoin-cli
- use named "external_signer" argument
- mention GUI setting

bab90450b9

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.

474a1c29d0

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.

00bb8951a2

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.

156c0253ef

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.

03978530ad

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

0git range-diff a973403...0397853

wallet: relax external_signer flag constraints #33112

Code Coverage & Benchmarks

Reviews

Conflicts