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

Sjors commented at 5:38 PM on August 5, 2019: member

Big picture overview in this gist.

This PR adds GUI support for external signers. It consists of 7 commits on top of #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...): <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. It's a bit weird though: the device will prompt for signing first and the QT confirmation dialog appears after.

External signer support is enabled by default when the GUI is configured and Boost::Process is present.

DrahtBot added the label Build system on Aug 5, 2019

DrahtBot added the label Docs on Aug 5, 2019

DrahtBot added the label GUI on Aug 5, 2019

DrahtBot added the label RPC/REST/ZMQ on Aug 5, 2019

DrahtBot added the label Scripts and tools on Aug 5, 2019

DrahtBot added the label Tests on Aug 5, 2019

DrahtBot added the label Utils/log/libs on Aug 5, 2019

DrahtBot added the label Wallet on Aug 5, 2019

DrahtBot commented at 9:04 PM on August 5, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19267 (ci: Upgrade most ci configs to focal by MarcoFalke)
#19162 (ci: tsan gui by MarcoFalke)
#19136 (wallet: add dumpwalletdescriptor RPC by achow101)
#19099 (refactor: Move wallet methods out of chain.h and node.h by ryanofsky)
#19077 (wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets by achow101)
#19064 (refactor: Cleanup thread ctor calls by hebasto)
#19046 (Replace CWallet::Set* functions that use memonly with Add/Load variants by achow101)
#19013 (test: add v0.20.0 to backwards compatibility test by Sjors)
#18870 (build: Allow BDB between 4.8 and 5.3 without --with-incompatible-bdb by achow101)
#18789 (qt: Add Create Unsigned button to SendConfirmationDialog by achow101)
#18788 (tests: Update more tests to work with descriptor wallets by achow101)
#18750 (build: optionally skip external warnings by vasild)
#18654 (rpc: separate bumpfee's psbt creation function into psbtbumpfee by achow101)
#18531 (rpc: Assert that RPCArg names are equal to CRPCCommand ones by MarcoFalke)
#18354 (Use shared pointers only in validation interface by bvbfan)
#18351 (Updated text on send confirmation dialog by dannmat)
#18202 (refactor: consolidate sendmany and sendtoaddress code by Sjors)
#18077 (net: Add NAT-PMP port forwarding support by hebasto)
#15937 (Add loadwallet and createwallet load_on_startup options by ryanofsky)
#15936 (Unify bitcoin-qt and bitcoind persistent settings by ryanofsky)
#15719 (Wallet passive startup by ryanofsky)
#15382 (util: add runCommandParseJSON by Sjors)

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.

DrahtBot added the label Needs rebase on Aug 6, 2019

jonasschnelli commented at 11:41 AM on August 12, 2019: contributor

This PR has 146 commits,... is this intentional?

Sjors commented at 1:18 PM on August 12, 2019: member

@jonasschnelli see the list in #16546 for all the all the experimental stuff this PR is based on. The actual change is only a few commits (Github messes up the ordering), compared to RPC-only functionality.

fanquake removed the label Build system on Aug 14, 2019

fanquake removed the label Docs on Aug 14, 2019

fanquake removed the label Scripts and tools on Aug 14, 2019

fanquake removed the label Tests on Aug 14, 2019

fanquake removed the label Utils/log/libs on Aug 14, 2019

Sjors force-pushed on Sep 16, 2019

Sjors commented at 5:19 PM on September 16, 2019: member

Cleaned it up a bit and tweaked the timestamps so it's more clear that only the most recent 4 commits are relevant.

139 commits may seem a bit intimidating, but it's really just a matter of reviewing:

runCommandParseJSON, so the GUI can call HWI: #15382
Wallet boxes #16341
Native descriptor wallets #16528, which in turn needs a rebase on #16341 and the merged #15450 (create wallet)
External signer RPC support: #16546

Sjors force-pushed on Sep 18, 2019

Sjors force-pushed on Oct 29, 2019

DrahtBot removed the label Needs rebase on Oct 29, 2019

Sjors force-pushed on Oct 29, 2019

DrahtBot added the label Needs rebase on Oct 30, 2019

Sjors force-pushed on Oct 30, 2019

Sjors force-pushed on Oct 31, 2019

DrahtBot removed the label Needs rebase on Oct 31, 2019

DrahtBot added the label Needs rebase on Oct 31, 2019

Sjors force-pushed on Nov 7, 2019

DrahtBot removed the label Needs rebase on Nov 7, 2019

DrahtBot added the label Needs rebase on Nov 8, 2019

Sjors force-pushed on Dec 9, 2019

DrahtBot removed the label Needs rebase on Dec 9, 2019

DrahtBot added the label Needs rebase on Dec 16, 2019

Sjors force-pushed on Jan 30, 2020

DrahtBot removed the label Needs rebase on Jan 30, 2020

Sjors force-pushed on Jan 30, 2020

DrahtBot added the label Needs rebase on Feb 4, 2020

Sjors force-pushed on Feb 21, 2020

Sjors commented at 9:38 PM on February 21, 2020: member

Rebased. Sending is a bit of a hack; I'll clean that up after #18027.

DrahtBot removed the label Needs rebase on Feb 21, 2020

DrahtBot added the label Needs rebase on Feb 25, 2020

Sjors force-pushed on Feb 25, 2020

DrahtBot removed the label Needs rebase on Feb 25, 2020

DrahtBot added the label Needs rebase on Mar 9, 2020

Sjors force-pushed on Mar 23, 2020

Sjors force-pushed on Mar 27, 2020

DrahtBot removed the label Needs rebase on Mar 27, 2020

Sjors force-pushed on Apr 1, 2020

DrahtBot added the label Needs rebase on Apr 6, 2020

Sjors force-pushed on Apr 22, 2020

DrahtBot removed the label Needs rebase on Apr 22, 2020

DrahtBot added the label Needs rebase on Apr 23, 2020

Sjors force-pushed on Apr 23, 2020

DrahtBot removed the label Needs rebase on Apr 23, 2020

Sjors force-pushed on Apr 27, 2020

DrahtBot commented at 11:09 AM on May 1, 2020: member

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the label Needs rebase on May 1, 2020

Sjors marked this as a draft on May 4, 2020

Sjors renamed this:
~~[WIP] UI external signer support (e.g. hardware wallet)~~
UI external signer support (e.g. hardware wallet)
on May 4, 2020

Sjors commented at 7:35 PM on May 4, 2020: member

Github lets you mark existing pull requests as draft now!

Sjors force-pushed on May 7, 2020

fjahr commented at 11:44 AM on May 11, 2020: member

Concept ACK

I am not a huge GUI user but I compared the way this works here with #16546 and it looks good to me.

Sjors force-pushed on May 18, 2020

Sjors force-pushed on May 22, 2020

jonasschnelli commented at 7:31 AM on May 29, 2020: contributor

I haven't followed the recent HWW work,... but is this still à jour with the concept?

Sjors commented at 6:37 PM on May 29, 2020: member

I'm keeping this PR up to date and it actually works, so it is the concept :-)

Sjors force-pushed on Jun 10, 2020

[depends] boost: patch unused variable in boost_process

Co-Authored-By: fanquake <fanquake@gmail.com>

4037f85033

configure: add ax_boost_process

Co-authored-by: Luke Dashjr <luke-jr+git@utopios.org>

2436908651

[build] make boost-process opt-in 459e567992

[build] msvc: add boost::process

* AppVeyor boost-process vcpkg package.
* Tell Boost linter to ignore it
* Add HAVE_BOOST_PROCESS for MSVC build (bitcoin_config.h)

97f65def30

[doc] include Doxygen comments for HAVE_BOOST_PROCESS 4a093579e1

[ci] use boost::process

Explictly opt-out on win64, in case the default changes.

f97f9e2bd4

[util] add runCommandParseJSON 48c57c7548

[rpc] [DO NOT MERGE] sendmany: return PSBT for wallets without private keys

TODO: replace with #16378 once its prerequisites are merged.

In addition we check to see if the PSBT is complete, in preperation for a ScriptPubKeyMan that can use an external signer.

fc46eb2202

configure: add --enable-external-signer

This prepares external signer support to be disabled by default.
It adds a configure option to enable this feature and to check
if Boost::Process is present.

It can also be enabled using --with-boost-process

This also exposes ENABLE_EXTERNAL_SIGNER to the test suite via test/config.ini

fixup! e192a9dedf1bc5dcddb928466eca9a93c83e5c91

edcf22fc0b

[build] msvc: define ENABLE_EXTERNAL_SIGNER 1532fb3ac5

[doc] include Doxygen comments for ENABLE_EXTERNAL_SIGNER 599c285abd

[test] framework: add skip_if_no_external_signer b66180c034

[wallet] add -signer argument for external signer command

Create basic ExternalSigner class with contructor. A Signer(<cmd>)
is added to CWallet on load if -signer=<cmd> is set.

267ee5adf9

[test] add external signer test

Includes a mock to mimick the HWI interace.

1357381b78

[wallet] add external_signer flag 749bd05feb

[wallet] add ExternalSignerScriptPubKeyMan 1be8ac458d

Sjors force-pushed on Jun 16, 2020

[rpc] add external signer RPC files a468f7a382

[rpc] signer: add enumeratesigners to list external signers dd34019b8e

[rpc] add external_signer option to createwallet d9c27f0a79

[test] external_signer wallet flag is immutable 5ec6357b62

[wallet] ExternalSigner: add getDescriptors method c59a7483fd

[wallet] add GetExternalSigner() d8591fb41c

[wallet] fetch keys from external signer upon creation 489f37b9c3

Add Fingerprint() to Descriptor f481c5fdb5

[rpc] signerdisplayaddress e31b152670

[rpc] sendtoaddress: support external signer 23c992d390

[doc] add external-signer.md 36f8792ede

[gui] options: add external signer path 7218c328b1

[gui] create wallet with external signer 3c21dd566c

Add displayAddress to wallet interface 26a9baffe9

[gui] display address on external signer b5c46cb99e

[gui] node: get external signer list ee8b40eb2e

[gui] wallet creation detects external signer c168ed0028

[gui] send: use external signer 6f9259c790

Sjors force-pushed on Jun 16, 2020

[build] enable external signer by default for GUI e306aaa3c0

Sjors commented at 6:13 PM on June 18, 2020: member

Moving to the new GUI repo: bitcoin-core/gui#4

Sjors closed this on Jun 18, 2020

Sjors deleted the branch on Apr 11, 2021

DrahtBot locked this on Aug 16, 2022