Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Jun 9, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/interfaces/node.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;

Expand Down
6 changes: 6 additions & 0 deletions src/interfaces/wallet.h
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,9 @@ class Wallet
//! Get dest values with prefix.
virtual std::vector<std::string> getDestValues(const std::string& prefix) = 0;

//! Display address on external signer
virtual bool displayAddress(const CTxDestination& dest) = 0;

//! Lock coin.
virtual void lockCoin(const COutPoint& output) = 0;

Expand Down Expand Up @@ -255,6 +258,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;

Expand Down
10 changes: 10 additions & 0 deletions src/node/interfaces.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,16 @@ class NodeImpl : public Node
}
return false;
}
#ifdef ENABLE_EXTERNAL_SIGNER
std::vector<ExternalSigner> externalSigners() override
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3f845ea

Should we let this throw runtime_error? cc @ryanofsky

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of these ifdefs are gone in bitcoin/bitcoin#21935

{
std::vector<ExternalSigner> signers = {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3f845ea

nit, drop = {}?

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; }
Expand Down
66 changes: 65 additions & 1 deletion src/qt/createwalletdialog.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include <config/bitcoin-config.h>
#endif

#include <external_signer.h>
#include <qt/createwalletdialog.h>
#include <qt/forms/ui_createwalletdialog.h>

Expand All @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eef8d64

This should take into account ENABLE_EXTERNAL_SIGNER.

May I suggest adding a private slot update or updateState and move all logic there?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a fix in bitcoin/bitcoin#21935. I agree this whole toggling code needs a refactor.


// 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) {
Expand Down Expand Up @@ -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)"));
Sjors marked this conversation as resolved.
Show resolved Hide resolved
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: to be consistent with gui: create wallet with external signer, this should be true (modulo comment about it being ambiguous anyway).

ui->disable_privkeys_checkbox->setEnabled(false);
ui->disable_privkeys_checkbox->setChecked(true);
const std::string label = signers[0].m_name;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

24815c6

Could add comment "TODO show available signers in combobox, for now use 1st signer name"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@promag it is just the name of the wallet so I don't think it would be worth adding a combobox tbh. That might be confusing because it wouldn't actually change between signers, it would just change the 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();
Expand All @@ -99,3 +158,8 @@ bool CreateWalletDialog::isDescriptorWalletChecked() const
{
return ui->descriptor_checkbox->isChecked();
}

bool CreateWalletDialog::isExternalSignerChecked() const
{
return ui->external_signer_checkbox->isChecked();
}
9 changes: 9 additions & 0 deletions src/qt/createwalletdialog.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@

class WalletModel;

#ifdef ENABLE_EXTERNAL_SIGNER
class ExternalSigner;
#endif

namespace Ui {
class CreateWalletDialog;
}
Expand All @@ -23,11 +27,16 @@ class CreateWalletDialog : public QDialog
explicit CreateWalletDialog(QWidget* parent);
virtual ~CreateWalletDialog();

#ifdef ENABLE_EXTERNAL_SIGNER
void setSigners(std::vector<ExternalSigner>& signers);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

24815c6

nit const &.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm adding that to: bitcoin/bitcoin#21935

#endif

QString walletName() const;
bool isEncryptWalletChecked() const;
bool isDisablePrivateKeysChecked() const;
bool isMakeBlankWalletChecked() const;
bool isDescriptorWalletChecked() const;
bool isExternalSignerChecked() const;

private:
Ui::CreateWalletDialog *ui;
Expand Down
11 changes: 11 additions & 0 deletions src/qt/forms/createwalletdialog.ui
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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>
Expand Down
30 changes: 30 additions & 0 deletions src/qt/forms/optionsdialog.ui
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Sjors marked this conversation as resolved.
Show resolved Hide resolved
</item>
<item>
<spacer name="verticalSpacer_Wallet">
<property name="orientation">
Expand Down
13 changes: 13 additions & 0 deletions src/qt/forms/receiverequestdialog.ui
Original file line number Diff line number Diff line change
Expand Up @@ -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">
Expand Down
2 changes: 2 additions & 0 deletions src/qt/optionsdialog.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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(); });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

28cd645

Note to other reviewers. The QLineEdit::textChanged signal has the QString parameter that actually is a new text. Using a lambda instead of a slot makes discarding of this parameter explicit. I think this is a good practice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

6cdbc83

Should disable externalSignerPath if not defined ENABLE_EXTERNAL_SIGNER?

connect(ui->threadsScriptVerif, qOverload<int>(&QSpinBox::valueChanged), this, &OptionsDialog::showRestartWarning);
/* Wallet */
connect(ui->spendZeroConfChange, &QCheckBox::clicked, this, &OptionsDialog::showRestartWarning);
Expand Down Expand Up @@ -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);
Expand Down
15 changes: 15 additions & 0 deletions src/qt/optionsmodel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
1 change: 1 addition & 0 deletions src/qt/optionsmodel.h
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ class OptionsModel : public QAbstractListModel
Prune, // bool
PruneSize, // int
DatabaseCache, // int
ExternalSignerPath, // QString
SpendZeroConfChange, // bool
Listen, // bool
OptionIDRowCount,
Expand Down
6 changes: 6 additions & 0 deletions src/qt/receiverequestdialog.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

62ac119

nit, drop this->.


connect(ui->btnVerify, &QPushButton::clicked, [this] {
model->displayAddress(info.address.toStdString());
});
}

void ReceiveRequestDialog::updateDisplayUnit()
Expand Down
Loading