UI external signer support (e.g. hardware wallet) by Sjors · Pull Request #4 · bitcoin-core/gui

Sjors · 2020-06-18T18:10:16Z

Big picture overview in this gist.

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

Then we add an external signer checkbox to the wallet creation dialog:

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...):

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.

Sjors · 2020-06-18T18:11:27Z

The original bitcoin/bitcoin#16549 (comment) has a Concept ACK from @fjahr.

I'll split out the interface changes and PR them to bitcoin/bitcoin later.

…ders 0ecff9d Improve "detected inconsistent lock order" error message (Hennadii Stepanov) bbe9cf4 test: Improve "potential deadlock detected" exception message (Hennadii Stepanov) 3559934 Fix mistakenly swapped "previous" and "current" lock orders (Hennadii Stepanov) Pull request description: In master (8ef15e8) the "previous" and "current" lock orders are mistakenly swapped. This PR: - fixes printed lock orders - improves the `sync_tests` unit test - makes the "detected inconsistent lock order" error message pointing to the lock location rather `tfm::format()` location. Debugger output example with this PR (with modified code, of course): ``` 2020-06-22T15:46:56Z [msghand] POTENTIAL DEADLOCK DETECTED 2020-06-22T15:46:56Z [msghand] Previous lock order was: 2020-06-22T15:46:56Z [msghand] (2) 'cs_main' in net_processing.cpp:2545 (in thread 'msghand') 2020-06-22T15:46:56Z [msghand] (1) 'g_cs_orphans' in net_processing.cpp:1400 (in thread 'msghand') 2020-06-22T15:46:56Z [msghand] Current lock order is: 2020-06-22T15:46:56Z [msghand] (1) 'g_cs_orphans' in net_processing.cpp:2816 (in thread 'msghand') 2020-06-22T15:46:56Z [msghand] (2) 'cs_main' in net_processing.cpp:2816 (in thread 'msghand') Assertion failed: detected inconsistent lock order for 'cs_main' in net_processing.cpp:2816 (in thread 'msghand'), details in debug log. Process 131393 stopped * thread bitcoin-core#15, name = 'b-msghand', stop reason = signal SIGABRT frame #0: 0x00007ffff775c18b libc.so.6`__GI_raise(sig=2) at raise.c:51:1 (lldb) bt * thread bitcoin-core#15, name = 'b-msghand', stop reason = signal SIGABRT * frame #0: 0x00007ffff775c18b libc.so.6`__GI_raise(sig=2) at raise.c:51:1 frame bitcoin-core#1: 0x00007ffff773b859 libc.so.6`__GI_abort at abort.c:79:7 frame bitcoin-core#2: 0x0000555555e5b196 bitcoind`(anonymous namespace)::potential_deadlock_detected(mismatch=0x00007fff99ff6f30, s1=size=2, s2=size=2, lock_location=0x00007fff99ff7010) at sync.cpp:134:9 frame bitcoin-core#3: 0x0000555555e5a1b1 bitcoind`(anonymous namespace)::push_lock(c=0x0000555556379220, locklocation=0x00007fff99ff7010) at sync.cpp:158:13 frame bitcoin-core#4: 0x0000555555e59e8a bitcoind`EnterCritical(pszName="cs_main", pszFile="net_processing.cpp", nLine=2816, cs=0x0000555556379220, fTry=false) at sync.cpp:177:5 frame bitcoin-core#5: 0x00005555555b0500 bitcoind`UniqueLock<AnnotatedMixin<std::recursive_mutex>, std::unique_lock<std::recursive_mutex> >::Enter(this=0x00007fff99ff8c20, pszName="cs_main", pszFile="net_processing.cpp", nLine=2816) at sync.h:134:9 frame bitcoin-core#6: 0x00005555555b017f bitcoind`UniqueLock<AnnotatedMixin<std::recursive_mutex>, std::unique_lock<std::recursive_mutex> >::UniqueLock(this=0x00007fff99ff8c20, mutexIn=0x0000555556379220, pszName="cs_main", pszFile="net_processing.cpp", nLine=2816, fTry=false) at sync.h:160:13 frame bitcoin-core#7: 0x00005555556aa57e bitcoind`ProcessMessage(pfrom=0x00007fff90001180, msg_type=error: summary string parsing error, vRecv=0x00007fff9c005ac0, nTimeReceived=1592840815980751, chainparams=0x00005555564b7110, chainman=0x0000555556380880, mempool=0x0000555556380ae0, connman=0x000055555657aa20, banman=0x00005555565167b0, interruptMsgProc=0x00005555565cae90) at net_processing.cpp:2816:9 ``` ACKs for top commit: laanwj: ACK 0ecff9d vasild: ACK 0ecff9d Tree-SHA512: ff285de8dd3198b5b33c4bfbdadf9b1448189c96143b9696bc4f41c07e784c00851ec169cf3ed45cc325f3617ba6783620803234f57fcce28bf6bc3d6a7234fb

luke-jr · 2020-07-31T19:32:35Z

Shouldn't the external signer program be per-wallet?

Sjors · 2020-08-07T11:33:15Z

@luke-jr in practice there's only 1 program out there: HWI. It already knows what to do for different wallets based on their fingerprint. So, unless more such tools show up, I prefer to wait until we can store arbitrary settings in the wallet: bitcoin/bitcoin#13044 and then keep -signer as the default.

Bosch-0 · 2020-09-16T04:20:03Z

Hey Sjors,

I could not see the signer script UI in the wallets settings when I compiled.

Sjors · 2020-09-16T10:29:36Z

@Bosch-0 oops, I accidentally lost a commit in my rebase fury. Try again.

Bosch-0 · 2020-09-23T09:28:07Z

Awesome, that worked! Having some issues with HWI though. After installing and setting the path to the hwi.py file I keep getting an exception error when creating a wallet.

How would we overcome having to install HWI separately? Would it have to be something like shipping HWI alongside the GUI. I'm a not too familiar with this process but having the path set by default behind the scenes would be ideal from a UX perspective.

Sjors · 2020-10-01T12:42:57Z

What's the error you're seeing?

Also, can you try from the command-line: hwi.py enumerate to see if HWI works standalone? This PR will certainly need followups to make it more user friendly.

Bosch-0 · 2020-10-06T07:32:26Z

Here is the error, though I think HWI is not installed properly.

Testing is being done on windows 10.

Installation appears to be successful when running pip3 install .

When running ./hwi.py enumerate I get the below error. Tried the other dependency installation methods as well as installing libusb-1.0.dll and including it in HWI directory but no luck yet.

Sjors · 2021-05-25T16:44:15Z

Rebased an addressed (most of) @meshcollider's comments.

Sjors · 2021-05-27T12:43:15Z

Rebased and slightly clarified the last commit.

hebasto

Tested 1c4b456 with HWW 2.0.2-rc.1 on Linux Mint 20.1 (Qt 5.12.8).

UPDATE: it was my fault -- hww connection issues. Ignore this.

~~The "External signer" checkbox is disabled when trying Menu -> File -> Create Wallet...~~

~~It could be enabled by checking the "Encrypt Wallet" checkbox, then unchecking it.~~

hebasto

ACK 1c4b456, tested on Linux Mint 20.1 (Qt 5.12.8) with HWW 2.0.2-rc.1.

Tested:

build with --without-bdb configure option
build with --without-sqlite configure option
creating a wallet
verifying an address on the hww screen
signing a tx

The following comments could be addressed in follow ups.

If no external signer is connected, it is possible to activate the "External signer" checkbox by checking the "Encrypt Wallet" checkbox, then unchecking it. Of course, the further attempt to create a wallet with an external signer will ends with the crash. This illness pattern works even if the client is compiled without SQLite support:
The "Confirm send coins" could still be opened while verifying transaction details on a signing device.
All new translatable strings could be annotated with translator comments.

promag

Tested ACK 1c4b456 but rebased with e033ca1, with HWI 2.0.2, with Nano S and Nano X.

Testing included creating wallets, mess with settings, generate and verifying addresses, sign and sending.

I'll test more but wanted to leave the partial review feedback.

promag · 2021-06-05T23:28:27Z

src/qt/createwalletdialog.cpp

+        // 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);


eef8d64

This should take into account ENABLE_EXTERNAL_SIGNER.

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

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

promag · 2021-06-05T23:44:53Z

src/qt/optionsdialog.cpp

    connect(ui->prune, &QCheckBox::clicked, this, &OptionsDialog::togglePruneWarning);
    connect(ui->pruneSize, static_cast<void (QSpinBox::*)(int)>(&QSpinBox::valueChanged), this, &OptionsDialog::showRestartWarning);
    connect(ui->databaseCache, static_cast<void (QSpinBox::*)(int)>(&QSpinBox::valueChanged), this, &OptionsDialog::showRestartWarning);
+    connect(ui->externalSignerPath, &QLineEdit::textChanged, [this]{ showRestartWarning(); });


6cdbc83

Should disable externalSignerPath if not defined ENABLE_EXTERNAL_SIGNER?

promag · 2021-06-05T23:57:43Z

src/qt/receiverequestdialog.cpp

        ui->wallet_content->hide();
    }
+
+    ui->btnVerify->setVisible(this->model->wallet().hasExternalSigner());


62ac119

nit, drop this->.

promag · 2021-06-06T00:20:53Z

src/node/interfaces.cpp

+#ifdef ENABLE_EXTERNAL_SIGNER
+    std::vector<ExternalSigner> externalSigners() override
+    {
+        std::vector<ExternalSigner> signers = {};


3f845ea

nit, drop = {}?

promag · 2021-06-06T00:27:56Z

src/qt/createwalletdialog.cpp

+        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;


24815c6

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

@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.

promag · 2021-06-06T00:28:14Z

src/qt/createwalletdialog.h

    virtual ~CreateWalletDialog();

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


24815c6

nit const &.

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

promag · 2021-06-06T00:30:03Z

src/node/interfaces.cpp

        return false;
    }
+#ifdef ENABLE_EXTERNAL_SIGNER
+    std::vector<ExternalSigner> externalSigners() override


3f845ea

Should we let this throw runtime_error? cc @ryanofsky

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

achow101 · 2021-06-08T17:31:59Z

Code Review ACK 1c4b456

Mostly reviewed the code, did a little bit of testing. I noticed that when HWI is doing something, there is no indication to the user that it is. It would be nice if there was some dialog box that said something was happening, otherwise there can be a minute or two where Core doesn't indicate anything is happening, and nothing appears on the device.

meshcollider

re-code-review ACK 1c4b456

Agree with achow101's comment about a 'working' message.

meshcollider · 2021-06-09T01:46:29Z

src/qt/createwalletdialog.cpp

+        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);


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

meshcollider · 2021-06-09T01:48:33Z

src/qt/createwalletdialog.cpp

+        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;


@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.

meshcollider · 2021-06-09T01:51:46Z

src/qt/sendcoinsdialog.cpp

    }

-    if (model->wallet().privateKeysDisabled()) {
+    if (model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner()) {


What about noPrivateKeyAccess() or something - the condition is that there are keys either in the wallet itself or that the wallet can 'use' via the external signer. Not a big deal though.

jarolrod · 2021-06-18T04:34:31Z

post-merge ACK, wanted to document that I tested this out with a coldcard and the process was smooth.

jb55 · 2021-06-23T18:54:07Z

@Sjors I tried this with my trezor1, I unlocked it on the command line on a wallet with a bip39 password, this is what pops up when I click create wallet:

hwi --stdinpass --device-type=trezor --device-path webusb:001:14 enumerate

[
  {
    "type": "trezor",
    "path": "webusb:001:14",
    "model": "trezor_1",
    "needs_pin_sent": false,
    "needs_passphrase_sent": false,
    "fingerprint": "67e2434a"
  }
]

dunno if I'm doing something wrong

Sjors · 2021-06-24T17:50:15Z

Mmm, I'm not sure how the passphrase flow works. @achow101?

I might be that we need to add support for that, e.g. if needs_pin_sent or needs_passphrase_sent returns true, we prompt for that.

However in your case those are false. Or did you actually enter a password in the command line example? (given --stdinpass)

achow101 · 2021-06-24T17:53:24Z

The passphrase should be cached by the device, so if you try again after running HWI manually, it should work. But we do need to implement a passphrase workflow for needs_passphrase_sent: true.

jb55 · 2021-06-24T18:38:34Z

@achow101 I've never been able to get the trezor_1 passphrase cached with hwi, I've always had to do --stdinpass on every command. hmm maybe I need to update firmware or something...

achow101 · 2021-06-24T18:53:59Z

Oh right, Trezor 1 doesn't have that, only Trezor T.

jb55 · 2021-06-24T19:23:15Z

@achow101 fwiw it caches fine when using trezorctl, dunno what they are doing.

hebasto · 2021-06-28T04:30:00Z

@Sjors

Could you add release notes into https://github.com/bitcoin-core/bitcoin-devwiki/wiki/22.0-Release-Notes-draft#gui-changes?

Sjors · 2021-06-28T09:15:26Z

@hebasto done, also for the RPC.

Sjors changed the title ~~2019/08/hww qt~~ UI external signer support (e.g. hardware wallet) Jun 18, 2020

This was referenced Jun 18, 2020

External signer support - Wallet Box edition bitcoin/bitcoin#16546

Merged

UI external signer support (e.g. hardware wallet) bitcoin/bitcoin#16549

Closed

Sjors force-pushed the 2019/08/hww-qt branch 4 times, most recently from 29e6a9d to 3ad6941 Compare June 25, 2020 18:10

Sjors mentioned this pull request Jul 2, 2020

Import dialog #19

Open

Sjors force-pushed the 2019/08/hww-qt branch from 3ad6941 to becd679 Compare July 2, 2020 18:32

Sjors mentioned this pull request Jul 2, 2020

Establish better delineation between Node & Wallet #26

Open

Sjors force-pushed the 2019/08/hww-qt branch from becd679 to 0b43111 Compare July 6, 2020 09:39

Sjors force-pushed the 2019/08/hww-qt branch from 0b43111 to 46916f4 Compare July 14, 2020 18:46

Sjors force-pushed the 2019/08/hww-qt branch from 46916f4 to e80d1b8 Compare July 18, 2020 08:02

Sjors force-pushed the 2019/08/hww-qt branch from e80d1b8 to b267b4a Compare August 7, 2020 11:29

Sjors force-pushed the 2019/08/hww-qt branch from b267b4a to fd7c7e1 Compare August 28, 2020 19:16

Sjors mentioned this pull request Aug 30, 2020

[Design] New 'Create Wallet' user flow. #77

Open

Bosch-0 mentioned this pull request Sep 11, 2020

Bitcoin Core / Improve Beginner Onboarding BitcoinDesign/Meta#17

Closed

Sjors force-pushed the 2019/08/hww-qt branch from fd7c7e1 to e9b4675 Compare September 15, 2020 15:42

Sjors force-pushed the 2019/08/hww-qt branch from e9b4675 to 7df0674 Compare September 16, 2020 10:29

Sjors mentioned this pull request Sep 17, 2020

Don't abort launch when failing to load a future wallet #95

Open

Sjors force-pushed the 2019/08/hww-qt branch from 7df0674 to f2066ea Compare October 1, 2020 12:43

Sjors force-pushed the 2019/08/hww-qt branch from f45aef4 to 8932aa4 Compare May 26, 2021 18:31

DrahtBot added the Needs rebase label May 27, 2021

Sjors added 7 commits May 27, 2021 14:01

gui: add external signer path to options dialog

6cdbc83

gui: create wallet with external signer

eef8d64

wallet: add displayAddress to interface

450cb40

gui: display address on external signer

62ac119

node: add externalSigners to interface

3f845ea

gui: wallet creation detects external signer

24815c6

gui: send using external signer

1c4b456

Sjors force-pushed the 2019/08/hww-qt branch from 8932aa4 to 1c4b456 Compare May 27, 2021 12:42

hebasto reviewed Jun 1, 2021

View reviewed changes

hebasto approved these changes Jun 1, 2021

View reviewed changes

promag reviewed Jun 6, 2021

View reviewed changes

meshcollider reviewed Jun 9, 2021

View reviewed changes

This was referenced Jun 11, 2021

Make row color alternating in the Peers tab optional #307

Closed

Do not exit and re-enter main event loop during shutdown #336

Merged

This was referenced Jun 15, 2021

Multiprocess bitcoin bitcoin/bitcoin#10102

Draft

interfaces: Expose settings.json methods to GUI bitcoin/bitcoin#15936

Merged

Conversation

Sjors commented Jun 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Jun 18, 2020

Uh oh!

luke-jr commented Jul 31, 2020

Uh oh!

Sjors commented Aug 7, 2020

Uh oh!

Bosch-0 commented Sep 16, 2020

Uh oh!

Sjors commented Sep 16, 2020

Uh oh!

Bosch-0 commented Sep 23, 2020

Uh oh!

Sjors commented Oct 1, 2020

Uh oh!

Bosch-0 commented Oct 6, 2020

Uh oh!

Sjors commented May 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented May 27, 2021

Uh oh!

hebasto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jun 8, 2021

Uh oh!

meshcollider left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jarolrod commented Jun 18, 2021

Uh oh!

jb55 commented Jun 23, 2021

Uh oh!

Sjors commented Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Jun 24, 2021

Uh oh!

jb55 commented Jun 24, 2021

Uh oh!

Sjors commented Jun 18, 2020 •

edited

Loading

Sjors commented May 25, 2021 •

edited

Loading

hebasto left a comment •

edited

Loading

meshcollider left a comment •

edited

Loading

Sjors commented Jun 24, 2021 •

edited

Loading