Merge remote-tracking branch 'sjors/2025/07/external-signer-relax'

Author: Sjors

doc/external-signer.md

@@ -11,10 +11,12 @@ When using a hardware wallet, consult the manufacturer website for (alternative)
 Start Bitcoin Core:
 
 ```sh
-$ bitcoind -signer=../HWI/hwi.py
+$ bitcoin node -signer=../HWI/hwi.py
 ```
 
-`bitcoin node` can also be substituted for `bitcoind`.
+(`bitcoin node` is the equivalent of `bitcoind`)
+
+The external signer script path can also be configured from the GUI, see Options -> Wallet.
 
 ### Device setup
 
@@ -25,7 +27,7 @@ Follow the hardware manufacturers instructions for the initial device setup, as
 Get a list of signing devices / services:
 
 ```
-$ bitcoin-cli enumeratesigners
+$ bitcoin rpc enumeratesigners
 {
   "signers": [
     {
@@ -39,18 +41,18 @@ The master key fingerprint is used to identify a device.
 Create a wallet, this automatically imports the public keys:
 
 ```sh
-$ bitcoin-cli createwallet "hww" true true "" true true true
+$ bitcoin rpc createwallet "hww" external_signer=true
 ```
 
-`bitcoin rpc` can also be substituted for `bitcoin-cli`.
+(`bitcoin rpc` is the equivalent of `bitcoin cli -named`)
 
 ### Verify an address
 
 Display an address on the device:
 
 ```sh
-$ bitcoin-cli -rpcwallet=<wallet> getnewaddress
-$ bitcoin-cli -rpcwallet=<wallet> walletdisplayaddress <address>
+$ bitcoin rpc -rpcwallet=<wallet> getnewaddress
+$ bitcoin rpc -rpcwallet=<wallet> walletdisplayaddress <address>
 ```
 
 Replace `<address>` with the result of `getnewaddress`.
@@ -60,7 +62,7 @@ Replace `<address>` with the result of `getnewaddress`.
 Under the hood this uses a [Partially Signed Bitcoin Transaction](psbt.md).
 
 ```sh
-$ bitcoin-cli -rpcwallet=<wallet> sendtoaddress <address> <amount>
+$ bitcoin rpc -rpcwallet=<wallet> sendtoaddress <address> <amount>
 ```
 
 This prompts your hardware wallet to sign, and fail if it's not connected. If successful

src/qt/createwalletdialog.cpp

@@ -26,34 +26,25 @@ CreateWalletDialog::CreateWalletDialog(QWidget* parent) :
     });
 
     connect(ui->encrypt_wallet_checkbox, &QCheckBox::toggled, [this](bool checked) {
-        // Disable the disable_privkeys_checkbox and external_signer_checkbox when isEncryptWalletChecked is
+        // Disable the disable_privkeys_checkbox when isEncryptWalletChecked is
         // set to true, enable it when isEncryptWalletChecked is false.
         ui->disable_privkeys_checkbox->setEnabled(!checked);
-#ifdef ENABLE_EXTERNAL_SIGNER
-        ui->external_signer_checkbox->setEnabled(m_has_signers && !checked);
-#endif
+
         // When the disable_privkeys_checkbox is disabled, uncheck it.
         if (!ui->disable_privkeys_checkbox->isEnabled()) {
             ui->disable_privkeys_checkbox->setChecked(false);
         }
-
-        // When the external_signer_checkbox box is disabled, uncheck it.
-        if (!ui->external_signer_checkbox->isEnabled()) {
-            ui->external_signer_checkbox->setChecked(false);
-        }
-
     });
 
     connect(ui->external_signer_checkbox, &QCheckBox::toggled, [this](bool checked) {
-        ui->encrypt_wallet_checkbox->setEnabled(!checked);
-        ui->blank_wallet_checkbox->setEnabled(!checked);
-        ui->disable_privkeys_checkbox->setEnabled(!checked);
+        // In the basic use case all keys will be on the external signer
+        // device and the wallet should be watch-only. Makes this the
+        // default suggestion.
+        ui->disable_privkeys_checkbox->setChecked(checked);
 
-        // The external signer checkbox is only enabled when a device is detected.
-        // In that case it is checked by default. Toggling it restores the other
-        // options to their default.
+        // The external signer box is checked by default when a device is
+        // detected. Toggling it restores the other options to their default.
         ui->encrypt_wallet_checkbox->setChecked(false);
-        ui->disable_privkeys_checkbox->setChecked(checked);
         ui->blank_wallet_checkbox->setChecked(false);
     });
 
@@ -103,12 +94,9 @@ void CreateWalletDialog::setSigners(const std::vector<std::unique_ptr<interfaces
     if (m_has_signers) {
         ui->external_signer_checkbox->setEnabled(true);
         ui->external_signer_checkbox->setChecked(true);
-        ui->encrypt_wallet_checkbox->setEnabled(false);
         ui->encrypt_wallet_checkbox->setChecked(false);
         // The order matters, because connect() is called when toggling a checkbox:
-        ui->blank_wallet_checkbox->setEnabled(false);
         ui->blank_wallet_checkbox->setChecked(false);
-        ui->disable_privkeys_checkbox->setEnabled(false);
         ui->disable_privkeys_checkbox->setChecked(true);
         const std::string label = signers[0]->getName();
         ui->wallet_name_line_edit->setText(QString::fromStdString(label));

src/qt/walletmodel.cpp

@@ -203,7 +203,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact
         int nChangePosRet = -1;
 
         auto& newTx = transaction.getWtx();
-        const auto& res = m_wallet->createTransaction(vecSend, coinControl, /*sign=*/!wallet().privateKeysDisabled(), nChangePosRet, nFeeRequired);
+        const auto& res = m_wallet->createTransaction(vecSend, coinControl, /*sign=*/!wallet().privateKeysDisabled() && !wallet().hasExternalSigner(), nChangePosRet, nFeeRequired);
         newTx = res ? *res : nullptr;
         transaction.setTransactionFee(nFeeRequired);
         if (fSubtractFeeFromAmount && newTx)

src/wallet/rpc/spend.cpp

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

src/wallet/rpc/wallet.cpp

@@ -353,13 +353,13 @@ static RPCHelpMan createwallet()
         "Creates and loads a new wallet.\n",
         {
             {"wallet_name", RPCArg::Type::STR, RPCArg::Optional::NO, "The name for the new wallet. If this is a path, the wallet will be created at the path location."},
-            {"disable_private_keys", RPCArg::Type::BOOL, RPCArg::Default{false}, "Disable the possibility of private keys (only watchonlys are possible in this mode)."},
+            {"disable_private_keys", RPCArg::Type::BOOL, RPCArg::DefaultHint{"false unless external_signer is set"}, "Disable the possibility of private keys (only watchonlys are possible in this mode)."},
             {"blank", RPCArg::Type::BOOL, RPCArg::Default{false}, "Create a blank wallet. A blank wallet has no keys."},
             {"passphrase", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "Encrypt the wallet with this passphrase."},
             {"avoid_reuse", RPCArg::Type::BOOL, RPCArg::Default{false}, "Keep track of coin reuse, and treat dirty and clean coins differently with privacy considerations in mind."},
             {"descriptors", RPCArg::Type::BOOL, RPCArg::Default{true}, "If set, must be \"true\""},
             {"load_on_startup", RPCArg::Type::BOOL, RPCArg::Optional::OMITTED, "Save wallet name to persistent settings and load on startup. True to add wallet to startup list, false to remove, null to leave unchanged."},
-            {"external_signer", RPCArg::Type::BOOL, RPCArg::Default{false}, "Use an external signer such as a hardware wallet. Requires -signer to be configured. Wallet creation will fail if keys cannot be fetched. Requires disable_private_keys and descriptors set to true."},
+            {"external_signer", RPCArg::Type::BOOL, RPCArg::Default{false}, "Use an external signer such as a hardware wallet. Requires -signer to be configured. Wallet creation will fail if keys cannot be fetched."},
         },
         RPCResult{
             RPCResult::Type::OBJ, "", "",
@@ -381,9 +381,8 @@ static RPCHelpMan createwallet()
 {
     WalletContext& context = EnsureWalletContext(request.context);
     uint64_t flags = 0;
-    if (!request.params[1].isNull() && request.params[1].get_bool()) {
-        flags |= WALLET_FLAG_DISABLE_PRIVATE_KEYS;
-    }
+
+    std::optional<bool> disable_private_keys{self.MaybeArg<bool>("disable_private_keys")};
 
     if (!request.params[2].isNull() && request.params[2].get_bool()) {
         flags |= WALLET_FLAG_BLANK_WALLET;
@@ -409,11 +408,21 @@ static RPCHelpMan createwallet()
     if (!request.params[7].isNull() && request.params[7].get_bool()) {
 #ifdef ENABLE_EXTERNAL_SIGNER
         flags |= WALLET_FLAG_EXTERNAL_SIGNER;
+        if (!disable_private_keys.has_value()) {
+            // In the basic use case all keys will be on the external signer
+            // device and the wallet should be watch-only. Makes this the
+            // default.
+            disable_private_keys = true;
+        }
 #else
         throw JSONRPCError(RPC_WALLET_ERROR, "Compiled without external signing support (required for external signing)");
 #endif
     }
 
+    if (disable_private_keys.value_or(false)) {
+        flags |= WALLET_FLAG_DISABLE_PRIVATE_KEYS;
+    }
+
     DatabaseOptions options;
     DatabaseStatus status;
     ReadDatabaseArgs(*context.args, options);

src/wallet/wallet.cpp

@@ -398,13 +398,6 @@ std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string&
         wallet_creation_flags |= WALLET_FLAG_BLANK_WALLET;
     }
 
-    // Private keys must be disabled for an external signer wallet
-    if ((wallet_creation_flags & WALLET_FLAG_EXTERNAL_SIGNER) && !(wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
-        error = Untranslated("Private keys must be disabled when using an external signer");
-        status = DatabaseStatus::FAILED_CREATE;
-        return nullptr;
-    }
-
     // Do not allow a passphrase when private keys are disabled
     if (!passphrase.empty() && (wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
         error = Untranslated("Passphrase provided but private keys are disabled. A passphrase is only used to encrypt private keys, so cannot be used for wallets with private keys disabled.");
@@ -2975,7 +2968,13 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
         // Only descriptor wallets can be created
         assert(walletInstance->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS));
 
-        if ((wallet_creation_flags & WALLET_FLAG_EXTERNAL_SIGNER) || !(wallet_creation_flags & (WALLET_FLAG_DISABLE_PRIVATE_KEYS | WALLET_FLAG_BLANK_WALLET))) {
+        // Don't generate or import keys for a blank wallet
+        if (!(wallet_creation_flags & WALLET_FLAG_BLANK_WALLET) && (
+            // 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)))
+        ) {
             walletInstance->SetupDescriptorScriptPubKeyMans();
         }
 

src/wallet/wallet.h

@@ -159,7 +159,8 @@ static constexpr uint64_t KNOWN_WALLET_FLAGS =
     |   WALLET_FLAG_EXTERNAL_SIGNER;
 
 static constexpr uint64_t MUTABLE_WALLET_FLAGS =
-        WALLET_FLAG_AVOID_REUSE;
+        WALLET_FLAG_AVOID_REUSE
+    |   WALLET_FLAG_EXTERNAL_SIGNER;
 
 static const std::map<WalletFlags, std::string> WALLET_FLAG_TO_STRING{
     {WALLET_FLAG_AVOID_REUSE, "avoid_reuse"},

test/functional/wallet_signer.py

@@ -66,22 +66,23 @@ def test_valid_signer(self):
         self.log.debug(f"-signer={self.mock_signer_path()}")
 
         # Create new wallets for an external signer.
-        # disable_private_keys and descriptors must be true:
-        assert_raises_rpc_error(-4, "Private keys must be disabled when using an external signer", self.nodes[1].createwallet, wallet_name='not_hww', disable_private_keys=False, external_signer=True)
-        self.nodes[1].createwallet(wallet_name='hww', disable_private_keys=True, external_signer=True)
+        self.nodes[1].createwallet(wallet_name='hww', external_signer=True)
         hww = self.nodes[1].get_wallet_rpc('hww')
         assert_equal(hww.getwalletinfo()["external_signer"], True)
 
-        # Flag can't be set afterwards (could be added later for non-blank descriptor based watch-only wallets)
-        self.nodes[1].createwallet(wallet_name='not_hww', disable_private_keys=True, external_signer=False)
+        # Private keys are disabled by default
+        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)
-        assert_raises_rpc_error(-8, "Wallet flag is immutable: external_signer", not_hww.setwalletflag, "external_signer", True)
-
+        not_hww.setwalletflag("external_signer", True)
+        assert_equal(not_hww.getwalletinfo()["external_signer"], True)
 
         self.set_mock_result(self.nodes[1], '0 {"invalid json"}')
         assert_raises_rpc_error(-1, 'Unable to parse JSON',
-            self.nodes[1].createwallet, wallet_name='hww2', disable_private_keys=True, external_signer=True
+            self.nodes[1].createwallet, wallet_name='hww2', external_signer=True
         )
         self.clear_mock_result(self.nodes[1])
 
@@ -210,7 +211,7 @@ def test_disconnected_signer(self):
         self.log.info('Test disconnected external signer')
 
         # First create a wallet with the signer connected
-        self.nodes[1].createwallet(wallet_name='hww_disconnect', disable_private_keys=True, external_signer=True)
+        self.nodes[1].createwallet(wallet_name='hww_disconnect', external_signer=True)
         hww = self.nodes[1].get_wallet_rpc('hww_disconnect')
         assert_equal(hww.getwalletinfo()["external_signer"], True)
 
@@ -231,13 +232,13 @@ def test_disconnected_signer(self):
     def test_invalid_signer(self):
         self.log.debug(f"-signer={self.mock_invalid_signer_path()}")
         self.log.info('Test invalid external signer')
-        assert_raises_rpc_error(-1, "Invalid descriptor", self.nodes[1].createwallet, wallet_name='hww_invalid', disable_private_keys=True, external_signer=True)
+        assert_raises_rpc_error(-1, "Invalid descriptor", self.nodes[1].createwallet, wallet_name='hww_invalid', external_signer=True)
 
     def test_multiple_signers(self):
         self.log.debug(f"-signer={self.mock_multi_signers_path()}")
         self.log.info('Test multiple external signers')
 
-        assert_raises_rpc_error(-1, "More than one external signer found", self.nodes[1].createwallet, wallet_name='multi_hww', disable_private_keys=True, external_signer=True)
+        assert_raises_rpc_error(-1, "More than one external signer found", self.nodes[1].createwallet, wallet_name='multi_hww', external_signer=True)
 
 if __name__ == '__main__':
     WalletSignerTest(__file__).main()