External signer support - Wallet Box edition by Sjors · Pull Request #16546 · bitcoin/bitcoin

Sjors · 2019-08-04T21:42:31Z

Big picture overview in this gist.

This PR lets bitcoind call an arbitrary command -signer=<cmd>, e.g. a hardware wallet driver, where it can fetch public keys, ask to display an address, and sign a transaction (using PSBT under the hood).

It's design to work with https://github.com/bitcoin-core/HWI, which supports multiple hardware wallets. Any command with the same arguments and return values will work. It simplifies the manual procedure described here.

Usage is documented in doc/external-signer.md, which also describes what protocol a different signer binary should conform to.

Use --enable-external-signer to opt in, requires Boost::Process:

Options used to compile and link:
  with wallet     = yes
  with gui / qt   = no
  external signer = yes

It adds the following RPC methods:

enumeratesigners: asks for a list of signers (e.g. devices) and their master key fingerprint
signerdisplayaddress <address>: asks to display an address

It enhances the following RPC methods:

createwallet: takes an additional external_signer argument and fetches keys from device
send: automatically sends transaction to device and waits

Usage TL&DR:

clone HWI repo somewhere and launch bitcoind -signer=../HWI/hwi.py
check if you can see your hardware device: bitcoin-cli enumeratesigners
create wallet and auto import keys bitcoin-cli createwallet "hww" true true "" true true true
display address on device: bitcoin-cli signerdisplayaddress ...
to spend, use send RPC and approve transaction on device

Prerequisites:

wallet: load flags before everything else #21127 load wallet flags before everything else
build: remove mostly pointless BOOST_PROCESS macro #21182 remove mostly pointless BOOST_PROCESS macro

Potentially useful followups:

GUI support: UI external signer support (e.g. hardware wallet) bitcoin-core/gui#4
bumpfee support
(automatically) verify (a subset of) keys on the device after import, through message signing

DrahtBot · 2019-08-04T22:16:27Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

MOVEONLY: CWallet transaction code out of wallet.cpp/.h #21207 (MOVEONLY: CWallet transaction code out of wallet.cpp/.h by ryanofsky)
wallet: check when create wallets for the reserved name "wallets" #21073 (wallet: check when create wallets for the reserved name "wallets" by Saibato)
Use std::filesystem. Remove Boost Filesystem & System #20744 ([POC] Use std::filesystem. Remove Boost Filesystem & System by fanquake)
test: Add feature_taproot.py --previous_release #20354 (test: Add feature_taproot.py --previous_release by MarcoFalke)
wallet, refactor: make DescriptorScriptPubKeyMan agnostic of internal flag #20191 (wallet, refactor: make DescriptorScriptPubKeyMan agnostic of internal flag by S3RK)

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.

luke-jr · 2019-08-04T23:24:13Z

This list-of-bools thing seems like a terrible idea...

Indeed, I was complaining about that when blank was added :-)

luke-jr · 2019-08-04T23:25:27Z

What if there are multiple wallets, with different signers?

IMO -signer needs to be replaced with either a wallet-stored path, or a path provided when the wallet is loaded...

Storing signer in the wallet makes sense, but I also think it can wait for a later PR. In practice afaik the only tool that currently works is HWI and it can handle multiple wallets. In the long run however I do hope that wallet manufactures provide their own software that just uses the same commands / responses.

Relaxo143 · 2019-09-12T11:45:16Z

Can we expect this to be merged and included in 0.19? It's a really useful and requested feature! Thanks to everyone who is working on it.

Sjors · 2019-09-16T08:54:00Z

@Relaxo143 not a chance; this is still work in progress and there's several pull requests that need to be reviewed and merged first. There's also a feature freeze on 0.19.

Sjors · 2019-09-16T16:27:25Z

I dropped the dependency on my new send RPC proposal #16378, in favor of just tweaking sendmany and sendtoaddress. It's less powerful, but should reduce the review burden once native descriptors are merged.

achow101 · 2021-02-18T19:54:24Z

re-ACK 96f991a11a871502d1a39f684994c1e2664c3af7

fanquake

In 903f277f982ed390dc68114c9c1eb70b4a59bb19 is where we should be able to drop HAVE_BOOST_PROCESS entirely. I realise that boost process could potentially be used for other things in future, but currently, it's not, and as this is implemented, there's a bit of an awkward split between HAVE_BOOST_PROCESS & ENABLE_EXTERNAL_SIGNER for code which, as I understand it, is all essentially the same feature. So I think for now we should just replace the few usages of HAVE_BOOST_PROCESS with ENABLE_EXTERNAL_SIGNER.

fanquake · 2021-02-21T03:43:51Z

Suggested change

const std::string command = gArgs.GetArg("-signer", ""); // DEFAULT_EXTERNAL_SIGNER);

const std::string command = gArgs.GetArg("-signer", DEFAULT_EXTERNAL_SIGNER);

Or if we are going to use "", we should remove the commented code.

Not sure what was going on there, but I put DEFAULT_EXTERNAL_SIGNER back.

Now I remember, I didn't want a circular inclusion. I just switched to "".

fanquake · 2021-02-21T07:02:00Z

From what I can see, GetExternalSigner() is only called from code that is inside ENABLE_EXTERNAL_SIGNER #idfefs. So why can't we wrap this whole function in an #ifdef, and drop the need for the throw()? I think the same goes for most of this file, and some other functions in this PR. It seems weird that when external signing is not enabled, we'd still be compiling functions that aren't called from anywhere, and who's purpose is to throw a runtime error to tell you about external signing and it's need of Boost Process.

If turns out I can put the entire ExternalSignerScriptPubKeyMan inside #ifdef. The most important throw()s are already in wallet.cpp.

This option replaces --with-boost-process 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. This also exposes ENABLE_EXTERNAL_SIGNER to the test suite via test/config.ini

Sjors · 2021-02-21T16:31:12Z

Rebased onto CI fix. @fanquake I replaced the remaining instances of HAVE_BOOST_PROCESS with ENABLE_EXTERNAL_SIGNER. In the process I squashed the MSVC and doc commit, since they're now just a simple rename.

laanwj · 2021-02-22T11:30:13Z

Please squash the move-only: add underscore to externalsigner.h commit as well. It's introduced in this PR, so let's introduce it with the eventual name directly.

Sjors · 2021-02-22T18:41:42Z

That may be a rather big rebase hell though, because the commits make incremental changes to these files so I'd have to divide this commit in 10 pieces, squash them in the right place and then deal with rebase conflicts. Unless there's an intelligent Git incantation to rename a file across all commits?

ryanofsky · 2021-02-22T21:31:30Z

Unless there's an intelligent Git incantation to rename a file across all commits?

Something like this will rename the one file. You can extend it to rename other files or do more replacements across commits:

end=f1824e7af7a8d53f9be50a6c6354511190dcb774
start=b9a262ea0a86f498959e81113522ce26299f1984

git checkout $end
git filter-branch --tree-filter '
  git mv src/wallet/externalsigner.h src/wallet/external_signer.h
  git grep -l externalsigner | xargs sed -i s:wallet/externalsigner.h:wallet/external_signer.h:
' --force $start^..HEAD

laanwj · 2021-02-23T07:41:59Z

Also git tends to be smart enough to take into account renamed files when rebasing. If you go to the first commit with git rebase -i, amend it for editing, git mv src/wallet/externalsigner.h src/wallet/external_signer.h, then git rebase --continue, that should not cause any merge conflicts.

Sjors · 2021-02-23T11:14:59Z

@ryanofsky's thanks! That worked (with some tweaks), and despite warnings from the git, it still compiles and history looks sane. @laanwj I have indeed seen Git behave sanely when it comes to renames, so maybe my worries were not necessary.

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

Includes a mock to mimick the HWI interace.

laanwj · 2021-02-23T16:50:41Z

re-ACK f75e0c1

maflcko · 2021-02-23T17:10:47Z

+Create a wallet, this automatically imports the public keys:
+
+```sh
+$ bitcoin-cli createwallet "hww" true true "" true true true


nit: Would be nice to use named args

maflcko · 2021-02-23T17:25:01Z

+        },
+        RPCExamples{""},
+        [](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue {
+            std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);


why does this rpc require a wallet?

I might get rid of that requirement in a followup.

hebasto · 2021-03-02T17:10:09Z

There is a buggy execution path in the configure script. Fixed in #21339.

hebasto · 2021-06-11T04:17:19Z

 @BUILD_BITCOIND_TRUE@ENABLE_BITCOIND=true
 @ENABLE_FUZZ_TRUE@ENABLE_FUZZ=true
 @ENABLE_ZMQ_TRUE@ENABLE_ZMQ=true
+@ENABLE_EXTERNAL_SIGNER_TRUE@ENABLE_EXTERNAL_SIGNER=true


~~It seems the @ENABLE_EXTERNAL_SIGNER_TRUE@ substitution does not work for some reasons.~~

nm, forgot to run ./autogen.sh

Sjors mentioned this pull request Aug 4, 2019

[WIP] External signer support (e.g. hardware wallet) #14912

Closed

DrahtBot added Build system Docs GUI RPC/REST/ZMQ Tests Utils/log/libs Wallet labels Aug 4, 2019

luke-jr reviewed Aug 4, 2019

View reviewed changes

fanquake removed Build system Docs GUI Utils/log/libs labels Aug 4, 2019

Sjors force-pushed the 2019/08/hww-box2 branch from 10b713d to 7421e89 Compare August 5, 2019 11:02

Sjors mentioned this pull request Aug 5, 2019

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

Closed

Sjors force-pushed the 2019/08/hww-box2 branch from 7421e89 to 0e28746 Compare August 5, 2019 17:48

DrahtBot added the Needs rebase label Aug 6, 2019

Sjors force-pushed the 2019/08/hww-box2 branch from 0e28746 to 7511650 Compare September 4, 2019 19:28

DrahtBot removed the Needs rebase label Sep 4, 2019

Sjors force-pushed the 2019/08/hww-box2 branch from 7511650 to 8d88bf9 Compare September 4, 2019 19:35

Sjors mentioned this pull request Sep 4, 2019

Native descriptor wallets justinmoon/junction#21

Open

DrahtBot added the Needs rebase label Sep 7, 2019

Sjors force-pushed the 2019/08/hww-box2 branch from 8d88bf9 to 3e991aa Compare September 16, 2019 16:24

Sjors mentioned this pull request Sep 17, 2019

External signer multisig support #16895

Closed

2 tasks

Sjors force-pushed the 2019/08/hww-box2 branch from 3e991aa to ca42459 Compare September 18, 2019 18:49

fanquake reviewed Feb 21, 2021

View reviewed changes

Sjors added 2 commits February 21, 2021 16:27

test: framework: add skip_if_no_external_signer

f7eb7ec

Sjors added 13 commits February 23, 2021 14:34

wallet: add -signer argument for external signer command

8cf543f

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

test: add external signer test

f3e6ce7

Includes a mock to mimick the HWI interace.

wallet: add external_signer flag

157ea7c

wallet: add ExternalSignerScriptPubKeyMan

8ce7767

rpc: add external signer RPC files

07b7c94

rpc: signer: add enumeratesigners to list external signers

2700f09

rpc: add external_signer option to createwallet

2655197

test: external_signer wallet flag is immutable

259f52c

wallet: add GetExternalSigner()

fc5da52

wallet: ExternalSigner: add GetDescriptors method

7ebc7c0

rpc: signerdisplayaddress

245b445

rpc: send: support external signer

d4b0107

doc: add external-signer.md

f75e0c1

maflcko reviewed Feb 23, 2021

View reviewed changes

This was referenced Feb 24, 2021

build, doc: Fix configure script output indentation and typos #21292

Merged

build: Make AM_CONDITIONAL([ENABLE_EXTERNAL_SIGNER]) unconditional #21339

Merged

Sjors mentioned this pull request Mar 11, 2021

Misc external signer improvement and HWI 2 support #21417

Merged

hebasto reviewed Jun 11, 2021

View reviewed changes

	const std::string command = gArgs.GetArg("-signer", ""); // DEFAULT_EXTERNAL_SIGNER);
	const std::string command = gArgs.GetArg("-signer", DEFAULT_EXTERNAL_SIGNER);

Uh oh!

Conversation

Sjors commented Aug 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

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!

Sjors Aug 5, 2019 • 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!

Relaxo143 commented Sep 12, 2019

Uh oh!

Sjors commented Sep 16, 2019

Uh oh!

Sjors commented Sep 16, 2019

Uh oh!

achow101 commented Feb 18, 2021

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Sjors commented Feb 21, 2021

Uh oh!

laanwj commented Feb 22, 2021

Uh oh!

Sjors commented Feb 22, 2021

Uh oh!

ryanofsky commented Feb 22, 2021

Uh oh!

laanwj commented Feb 23, 2021

Uh oh!

Sjors commented Feb 23, 2021

Uh oh!

laanwj commented Feb 23, 2021

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!

hebasto commented Mar 2, 2021

Uh oh!

hebasto Jun 11, 2021 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

17 participants

Sjors commented Aug 4, 2019 •

edited

Loading

DrahtBot commented Aug 4, 2019 •

edited

Loading

Sjors Aug 5, 2019 •

edited

Loading

hebasto Jun 11, 2021 •

edited

Loading