Merge #4: UI external signer support (e.g. hardware wallet)

1c4b456 gui: send using external signer (Sjors Provoost) 24815c6 gui: wallet creation detects external signer (Sjors Provoost) 3f845ea node: add externalSigners to interface (Sjors Provoost) 62ac119 gui: display address on external signer (Sjors Provoost) 450cb40 wallet: add displayAddress to interface (Sjors Provoost) eef8d64 gui: create wallet with external signer (Sjors Provoost) 6cdbc83 gui: add external signer path to options dialog (Sjors Provoost) Pull request description: Big picture overview in [this gist](https://gist.github.com/Sjors/29d06728c685e6182828c1ce9b74483d). This PR adds GUI support for external signers, based on the since merged bitcoin/bitcoin#16546 (RPC). The UX isn't amazing - especially the blocking calls - but it works. First we adds a GUI setting for the signer script (e.g. path to HWI): <img width="625" alt="Schermafbeelding 2019-08-05 om 19 32 59" src="https://user-images.githubusercontent.com/10217/62483415-e1ff1680-b7b7-11e9-97ca-8d2ce54ca1cb.png"> Then we add an external signer checkbox to the wallet creation dialog: <img width="374" alt="Schermafbeelding 2019-11-07 om 19 17 23" src="https://user-images.githubusercontent.com/10217/68416387-b57ee000-0194-11ea-9730-127d60273008.png"> It's checked by default if HWI detects a device. It also grabs the name. It then creates a fresh wallet and imports the keys. You can verify an address on the device (blocking...): <img width="673" alt="Schermafbeelding 2019-08-05 om 19 29 22" src="https://user-images.githubusercontent.com/10217/62483560-43bf8080-b7b8-11e9-9902-8a036116dc4b.png"> Sending, including coin selection, Just Works(tm) as long the device is present. ~External signer support is enabled by default when the GUI is configured and Boost::Process is present.~ External signer support remains disabled by default, see bitcoin/bitcoin#21935. ACKs for top commit: achow101: Code Review ACK 1c4b456 hebasto: ACK 1c4b456, tested on Linux Mint 20.1 (Qt 5.12.8) with HWW `2.0.2-rc.1`. promag: Tested ACK 1c4b456 but rebased with e033ca1, with HWI 2.0.2, with Nano S and Nano X. meshcollider: re-code-review ACK 1c4b456 Tree-SHA512: 3503113c5c69d40adb6ce364d8e7cae23ce82d032a00474ba9aeb6202eb70f496ef4a6bf2e623e5171e524ad31ade7941a4e0e89539c64518aaec74f4562d86b
bitcoin-core · Jun 9, 2021 · 68a89d7 · 68a89d7
2 parents 7cac262 + 1c4b456
commit 68a89d7
Show file tree

Hide file tree

Showing 19 changed files with 284 additions and 11 deletions.
diff --git a/src/interfaces/node.h b/src/interfaces/node.h
@@ -6,6 +6,7 @@
 #define BITCOIN_INTERFACES_NODE_H
 
 #include <amount.h>     // For CAmount
+#include <external_signer.h>
 #include <net.h>        // For NodeId
 #include <net_types.h>  // For banmap_t
 #include <netaddress.h> // For Network
@@ -110,6 +111,11 @@ class Node
     //! Disconnect node by id.
     virtual bool disconnectById(NodeId id) = 0;
 
+#ifdef ENABLE_EXTERNAL_SIGNER
+    //! List external signers
+    virtual std::vector<ExternalSigner> externalSigners() = 0;
+#endif
+
     //! Get total bytes recv.
     virtual int64_t getTotalBytesRecv() = 0;
 

diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h
@@ -118,6 +118,9 @@ class Wallet
     //! Save or remove receive request.
     virtual bool setAddressReceiveRequest(const CTxDestination& dest, const std::string& id, const std::string& value) = 0;
 
+    //! Display address on external signer
+    virtual bool displayAddress(const CTxDestination& dest) = 0;
+
     //! Lock coin.
     virtual void lockCoin(const COutPoint& output) = 0;
 
@@ -252,6 +255,9 @@ class Wallet
     // Return whether private keys enabled.
     virtual bool privateKeysDisabled() = 0;
 
+    // Return whether wallet uses an external signer.
+    virtual bool hasExternalSigner() = 0;
+
     // Get default address type.
     virtual OutputType getDefaultAddressType() = 0;
 

diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
@@ -170,6 +170,16 @@ class NodeImpl : public Node
         }
         return false;
     }
+#ifdef ENABLE_EXTERNAL_SIGNER
+    std::vector<ExternalSigner> externalSigners() override
+    {
+        std::vector<ExternalSigner> signers = {};
+        const std::string command = gArgs.GetArg("-signer", "");
+        if (command == "") return signers;
+        ExternalSigner::Enumerate(command, signers, Params().NetworkIDString());
+        return signers;
+    }
+#endif
     int64_t getTotalBytesRecv() override { return m_context->connman ? m_context->connman->GetTotalBytesRecv() : 0; }
     int64_t getTotalBytesSent() override { return m_context->connman ? m_context->connman->GetTotalBytesSent() : 0; }
     size_t getMempoolSize() override { return m_context->mempool ? m_context->mempool->size() : 0; }

diff --git a/src/qt/createwalletdialog.cpp b/src/qt/createwalletdialog.cpp
@@ -6,6 +6,7 @@
 #include <config/bitcoin-config.h>
 #endif
 
+#include <external_signer.h>
 #include <qt/createwalletdialog.h>
 #include <qt/forms/ui_createwalletdialog.h>
 
@@ -27,14 +28,39 @@ CreateWalletDialog::CreateWalletDialog(QWidget* parent) :
     });
 
     connect(ui->encrypt_wallet_checkbox, &QCheckBox::toggled, [this](bool checked) {
-        // Disable the disable_privkeys_checkbox when isEncryptWalletChecked is
+        // Disable the disable_privkeys_checkbox and external_signer_checkbox when isEncryptWalletChecked is
         // set to true, enable it when isEncryptWalletChecked is false.
         ui->disable_privkeys_checkbox->setEnabled(!checked);
+        ui->external_signer_checkbox->setEnabled(!checked);
 
         // 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);
+        ui->descriptor_checkbox->setEnabled(!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.
+        ui->descriptor_checkbox->setChecked(checked);
+        ui->encrypt_wallet_checkbox->setChecked(false);
+        ui->disable_privkeys_checkbox->setChecked(checked);
+        // The blank check box is ambiguous. This flag is always true for a
+        // watch-only wallet, even though we immedidately fetch keys from the
+        // external signer.
+        ui->blank_wallet_checkbox->setChecked(checked);
     });
 
     connect(ui->disable_privkeys_checkbox, &QCheckBox::toggled, [this](bool checked) {
@@ -63,18 +89,51 @@ CreateWalletDialog::CreateWalletDialog(QWidget* parent) :
         ui->descriptor_checkbox->setToolTip(tr("Compiled without sqlite support (required for descriptor wallets)"));
         ui->descriptor_checkbox->setEnabled(false);
         ui->descriptor_checkbox->setChecked(false);
+        ui->external_signer_checkbox->setEnabled(false);
+        ui->external_signer_checkbox->setChecked(false);
 #endif
+
 #ifndef USE_BDB
         ui->descriptor_checkbox->setEnabled(false);
         ui->descriptor_checkbox->setChecked(true);
 #endif
+
+#ifndef ENABLE_EXTERNAL_SIGNER
+        //: "External signing" means using devices such as hardware wallets.
+        ui->external_signer_checkbox->setToolTip(tr("Compiled without external signing support (required for external signing)"));
+        ui->external_signer_checkbox->setEnabled(false);
+        ui->external_signer_checkbox->setChecked(false);
+#endif
+
 }
 
 CreateWalletDialog::~CreateWalletDialog()
 {
     delete ui;
 }
 
+#ifdef ENABLE_EXTERNAL_SIGNER
+void CreateWalletDialog::setSigners(std::vector<ExternalSigner>& signers)
+{
+    if (!signers.empty()) {
+        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].m_name;
+        ui->wallet_name_line_edit->setText(QString::fromStdString(label));
+        ui->buttonBox->button(QDialogButtonBox::Ok)->setEnabled(true);
+    } else {
+        ui->external_signer_checkbox->setEnabled(false);
+    }
+}
+#endif
+
 QString CreateWalletDialog::walletName() const
 {
     return ui->wallet_name_line_edit->text();
@@ -99,3 +158,8 @@ bool CreateWalletDialog::isDescriptorWalletChecked() const
 {
     return ui->descriptor_checkbox->isChecked();
 }
+
+bool CreateWalletDialog::isExternalSignerChecked() const
+{
+    return ui->external_signer_checkbox->isChecked();
+}
diff --git a/src/qt/createwalletdialog.h b/src/qt/createwalletdialog.h
@@ -9,6 +9,10 @@
 
 class WalletModel;
 
+#ifdef ENABLE_EXTERNAL_SIGNER
+class ExternalSigner;
+#endif
+
 namespace Ui {
     class CreateWalletDialog;
 }
@@ -23,11 +27,16 @@ class CreateWalletDialog : public QDialog
     explicit CreateWalletDialog(QWidget* parent);
     virtual ~CreateWalletDialog();
 
+#ifdef ENABLE_EXTERNAL_SIGNER
+    void setSigners(std::vector<ExternalSigner>& signers);
+#endif
+
     QString walletName() const;
     bool isEncryptWalletChecked() const;
     bool isDisablePrivateKeysChecked() const;
     bool isMakeBlankWalletChecked() const;
     bool isDescriptorWalletChecked() const;
+    bool isExternalSignerChecked() const;
 
 private:
     Ui::CreateWalletDialog *ui;

diff --git a/src/qt/forms/createwalletdialog.ui b/src/qt/forms/createwalletdialog.ui
@@ -109,6 +109,16 @@
         </property>
        </widget>
       </item>
+      <item>
+       <widget class="QCheckBox" name="external_signer_checkbox">
+        <property name="toolTip">
+         <string>Use an external signing device such as a hardware wallet. Configure the external signer script in wallet preferences first.</string>
+        </property>
+        <property name="text">
+         <string>External signer</string>
+        </property>
+       </widget>
+      </item>
      </layout>
     </widget>
    </item>
@@ -143,6 +153,7 @@
   <tabstop>disable_privkeys_checkbox</tabstop>
   <tabstop>blank_wallet_checkbox</tabstop>
   <tabstop>descriptor_checkbox</tabstop>
+  <tabstop>external_signer_checkbox</tabstop>
  </tabstops>
  <resources/>
  <connections>

diff --git a/src/qt/forms/optionsdialog.ui b/src/qt/forms/optionsdialog.ui
@@ -229,6 +229,36 @@
          </layout>
         </widget>
        </item>
+       <item>
+        <widget class="QGroupBox" name="groupBoxHww">
+         <property name="title">
+          <string>External Signer (e.g. hardware wallet)</string>
+         </property>
+         <layout class="QVBoxLayout" name="verticalLayoutHww">
+           <item>
+            <layout class="QHBoxLayout" name="horizontalLayoutHww">
+             <item>
+              <widget class="QLabel" name="externalSignerPathLabel">
+               <property name="text">
+                <string>&amp;External signer script path</string>
+               </property>
+               <property name="buddy">
+                <cstring>externalSignerPath</cstring>
+               </property>
+              </widget>
+             </item>
+             <item>
+              <widget class="QLineEdit" name="externalSignerPath">
+                <property name="toolTip">
+                  <string>Full path to a Bitcoin Core compatible script (e.g. C:\Downloads\hwi.exe or /Users/you/Downloads/hwi.py). Beware: malware can steal your coins!</string>
+                </property>
+              </widget>
+             </item>
+            </layout>
+           </item>
+         </layout>
+        </widget>
+       </item>
        <item>
         <spacer name="verticalSpacer_Wallet">
          <property name="orientation">

diff --git a/src/qt/forms/receiverequestdialog.ui b/src/qt/forms/receiverequestdialog.ui
@@ -254,6 +254,19 @@
        </property>
       </widget>
      </item>
+     <item>
+      <widget class="QPushButton" name="btnVerify">
+       <property name="text">
+        <string>&amp;Verify</string>
+       </property>
+       <property name="toolTip">
+        <string>Verify this address on e.g. a hardware wallet screen</string>
+       </property>
+       <property name="autoDefault">
+        <bool>false</bool>
+       </property>
+      </widget>
+     </item>
      <item>
       <widget class="QPushButton" name="btnSaveAs">
        <property name="text">

diff --git a/src/qt/optionsdialog.cpp b/src/qt/optionsdialog.cpp
@@ -199,6 +199,7 @@ void OptionsDialog::setModel(OptionsModel *_model)
     connect(ui->prune, &QCheckBox::clicked, this, &OptionsDialog::togglePruneWarning);
     connect(ui->pruneSize, qOverload<int>(&QSpinBox::valueChanged), this, &OptionsDialog::showRestartWarning);
     connect(ui->databaseCache, qOverload<int>(&QSpinBox::valueChanged), this, &OptionsDialog::showRestartWarning);
+    connect(ui->externalSignerPath, &QLineEdit::textChanged, [this]{ showRestartWarning(); });
     connect(ui->threadsScriptVerif, qOverload<int>(&QSpinBox::valueChanged), this, &OptionsDialog::showRestartWarning);
     /* Wallet */
     connect(ui->spendZeroConfChange, &QCheckBox::clicked, this, &OptionsDialog::showRestartWarning);
@@ -233,6 +234,7 @@ void OptionsDialog::setMapper()
     /* Wallet */
     mapper->addMapping(ui->spendZeroConfChange, OptionsModel::SpendZeroConfChange);
     mapper->addMapping(ui->coinControlFeatures, OptionsModel::CoinControlFeatures);
+    mapper->addMapping(ui->externalSignerPath, OptionsModel::ExternalSignerPath);
 
     /* Network */
     mapper->addMapping(ui->mapPortUpnp, OptionsModel::MapPortUPnP);

diff --git a/src/qt/optionsmodel.cpp b/src/qt/optionsmodel.cpp
@@ -117,6 +117,13 @@ void OptionsModel::Init(bool resetSettings)
         settings.setValue("bSpendZeroConfChange", true);
     if (!gArgs.SoftSetBoolArg("-spendzeroconfchange", settings.value("bSpendZeroConfChange").toBool()))
         addOverriddenOption("-spendzeroconfchange");
+
+    if (!settings.contains("external_signer_path"))
+        settings.setValue("external_signer_path", "");
+
+    if (!gArgs.SoftSetArg("-signer", settings.value("external_signer_path").toString().toStdString())) {
+        addOverriddenOption("-signer");
+    }
 #endif
 
     // Network
@@ -326,6 +333,8 @@ QVariant OptionsModel::data(const QModelIndex & index, int role) const
 #ifdef ENABLE_WALLET
         case SpendZeroConfChange:
             return settings.value("bSpendZeroConfChange");
+        case ExternalSignerPath:
+            return settings.value("external_signer_path");
 #endif
         case DisplayUnit:
             return nDisplayUnit;
@@ -445,6 +454,12 @@ bool OptionsModel::setData(const QModelIndex & index, const QVariant & value, in
                 setRestartRequired(true);
             }
             break;
+        case ExternalSignerPath:
+            if (settings.value("external_signer_path") != value.toString()) {
+                settings.setValue("external_signer_path", value.toString());
+                setRestartRequired(true);
+            }
+            break;
 #endif
         case DisplayUnit:
             setDisplayUnit(value);

diff --git a/src/qt/optionsmodel.h b/src/qt/optionsmodel.h
@@ -65,6 +65,7 @@ class OptionsModel : public QAbstractListModel
         Prune,                  // bool
         PruneSize,              // int
         DatabaseCache,          // int
+        ExternalSignerPath,     // QString
         SpendZeroConfChange,    // bool
         Listen,                 // bool
         OptionIDRowCount,

diff --git a/src/qt/receiverequestdialog.cpp b/src/qt/receiverequestdialog.cpp
@@ -89,6 +89,12 @@ void ReceiveRequestDialog::setInfo(const SendCoinsRecipient &_info)
         ui->wallet_tag->hide();
         ui->wallet_content->hide();
     }
+
+    ui->btnVerify->setVisible(this->model->wallet().hasExternalSigner());
+
+    connect(ui->btnVerify, &QPushButton::clicked, [this] {
+        model->displayAddress(info.address.toStdString());
+    });
 }
 
 void ReceiveRequestDialog::updateDisplayUnit()