[WIP] External signer support (e.g. hardware wallet) by Sjors · Pull Request #14912 · bitcoin/bitcoin

Sjors · 2018-12-10T14:46:02Z

This PR lets bitcoind to 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 PSBT.

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.

It adds the following RPC methods:

enumeratesigners: asks for a list of signers (e.g. devices) and their master key fingerprint
signerfetchkeys (needs Add getdescriptors command bitcoin-core/HWI#137): asks for descriptors and then fills the keypool (no private keys)
signerdisplayaddress <address>: asks to display an address
signerprocesspsbt <psbt> to send the <psbt> to <cmd> to sign and wait for the result

Usage TL&DR:

clone HWI repo somewhere and launch bitcoind -signer=../HWI/hwi.py
create wallet without private keys: bitcoin-cli createwallet hww true
list hardware devices: bitcoin-cli enumeratesigners
fetch keys from hardware device into the wallet: bitcoin-cli -rpcwallet=hww signerfetchkeys
display address on device, sign transaction: see doc/external-signer.md

For easier review, this builds on the following PRs:

util: add RunCommandParseJSON #15382: add runCommandParseJSON
Descriptor: add GetAddressType() and IsSegWit() #15590 Descriptor: add GetAddressType() and IsSegwit()

Potentially useful followups:

[rpc] signer send and fee bump convenience methods #15876: signer send and bumpfee conveniance methods
Native Descriptor Wallets using DescriptorScriptPubKeyMan #16528: descriptor based wallets (to preserve BIP44/49/84 compatibility with mixed address types)
(automatically) verify (a subset of) keys on the device after import, through message signing

Sjors · 2018-12-15T11:22:06Z

Now that #14491 has been rebased, the hww branch I'm building off should also soon be rebased. At that point I can rebase and make Travis happy. In addition, I'll be able to leverage #14646 to clean up my descriptor related code (I'm currently using string concatenation to build descriptors). I have a few other spring cleaning items on my todo list too.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16542 (Return more specific errors about invalid descriptors by achow101)
#16539 ([wallet] lower -txmaxfee default from 0.1 to 0.01 BTC by Sjors)
#16528 ([WIP] Native Descriptor Wallets (take 2) by achow101)
#16365 (Log RPC parameters (arguments) if -debug=rpcparams by LarryRuane)
#16341 (Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101)
#16273 (refactor: Remove unused includes by practicalswift)
#15876 ([rpc] signer send and fee bump convenience methods by Sjors)
#15529 (Add Qt programs to msvc build (updated, no code changes) by sipsorcery)

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.

src/wallet/rpcwallet.h

practicalswift · 2018-12-18T16:20:53Z

src/wallet/externalsigner.cpp

ExternalSigner signer shadows UniValue signer :-)

src/wallet/rpcdump.cpp

practicalswift · 2018-12-18T16:24:28Z

src/wallet/rpcdump.cpp

Make signedness changing implicit conversion explicit? Also, remove redundant initialization to zero on the line above? Merge the two lines.

The compiler didn't like it when I initialized using gArgs.GetArg.

practicalswift · 2018-12-18T16:29:39Z

src/util/system.cpp

Initialize to zero?

practicalswift · 2018-12-18T16:31:09Z

src/wallet/rpcwallet.cpp

Should be "fingerprint" :-)

practicalswift · 2018-12-18T16:38:09Z

src/util/system.cpp

I haven't reviewed this use of popen(…) closer, but please note the recommendations/risks with regards to popen(…) used described in the CERT secure coding guidelines (more specifically rule ENV33-C).

This is straight from stack overflow, so certainly needs more review... I'm surprised how complicated it is to just read some JSON from stdout.

popen and pclose are unix-like only. You can't use them on Windows. IMO you could use boost process instead.

I think we don't depend on boost process.

I know. But this is the simplest way if this is really necessary to call another process. I don't think that any one here know how to use Windows CreateProcess API.

It would be reasonable to call _popen function here on windows or #define popen _popen: https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/system-wsystem.

You do need to be very careful about special characters passed in the command string, but it should be ok if you stick to hex/base64.

_popen would only work in console program. See https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/popen-wpopen. So it doesn't work in Qt.

practicalswift · 2018-12-18T16:38:57Z

src/util/strencodings.cpp

keypath should be const reference?

practicalswift · 2018-12-18T16:42:48Z

src/wallet/rpcwallet.cpp

Nit: Could use command.empty()? :-)

practicalswift · 2018-12-18T16:43:39Z

src/wallet/externalsigner.cpp

Should be const reference? :-)

practicalswift · 2018-12-18T16:43:54Z

src/wallet/externalsigner.cpp

Should be const reference? :-)

practicalswift · 2018-12-18T16:44:29Z

src/wallet/externalsigner.cpp

Use string equality operator instead of compare :-)

practicalswift · 2018-12-18T16:47:04Z

src/wallet/rpcwallet.cpp

signer can be nullptr here if the check on L4016 is correct?

practicalswift · 2018-12-18T16:47:38Z

src/wallet/rpcwallet.cpp

Same here: signer can be nullptr here if the check on L4016 is correct?

practicalswift · 2018-12-18T16:48:58Z

src/util/strencodings.h

keypath should be const reference?

practicalswift · 2018-12-18T16:50:23Z

src/wallet/rpcdump.cpp

Nit: Could use std::copy instead?

practicalswift · 2018-12-18T16:50:51Z

src/wallet/rpcdump.cpp

Same here: Could you std::copy?

practicalswift · 2018-12-18T16:52:53Z

src/wallet/rpcdump.cpp

Missing space before (. Consider running new code through clang-format :-)

practicalswift · 2018-12-18T16:53:15Z

src/wallet/rpcdump.cpp

Same here: Missing whitespace before (.

src/wallet/rpcwallet.cpp

test/functional/wallet_importmulti.py

Sjors · 2019-06-07T13:24:43Z

Rebased and dropped signersend from this PR, moved it to #15876 (which also contains fee bump support).

Sjors · 2019-07-11T14:55:14Z

I'm considering rebasing* this on top of #16341 (aka "box"), by introducing a ExternalSignerScriptPubKeyMan. It can be make a bit safer in the process.

move all of externalsigner.{h,cpp} into a ExternalSignerScriptPubKeyMan
Get rid of signerfetchkeys RPC; createwallet and keypoolrefill cover this
SetupGeneration: this would call getdescriptors on the device at wallet creation time. The BIP32 account number could be passed as on option to createwallet. Wallet creation will fail if it can't fetch keys.
TopUp will also call getdescriptors on the device, with a higher range. This will become unecessary with native wallet descriptors. In practice with a keypool size of 1000 this is not a big inconvenience. The current behavior of GetReservedDestination calling TopUp needs to go away.
add a feature flag that this wallet should use ExternalSignerScriptPubKeyMan
store the device fingerprint in the wallet metadata at creation time, so enumeratesigners is unnecessary after wallet creation
allow only a single OutputType, store it and refuse getnewaddress for different types. This is necessary to remain compatible with BIP44/49/84, which requires a different derivation path per output type. This can be relaxed with native descriptor wallets, see also Descriptor: add GetAddressType() and IsSegWit() #15590.
The current flow of calling enumeratesigners on an existing wallet won't do anymore, because we already need to know the signer at createwallet time. Instead enumeratesigners could work without a wallet and not store the result anywhere. When creating a wallet it will just use the first available result from enumeratesigners unless a fingerprint option is provided.
signerdisplayaddress remains unchanged
signerprocesspsbt can be melted into processpsbt thanks to the feature flag

* = later though, I'd rather not have a PR with 110 commits

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

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

Sjors · 2019-08-02T18:46:55Z

Rebased now that #15911 is merged. Dropped a few commits that should have been in #15876 and aren't needed anyway after #15713. Removed the need for the unit test to add private keys to the keypool (closing #15414).

I'll keep this and the convenience methods in #15876 up to date and work on a "boxed" version in a separate PR.

Sjors · 2019-08-03T11:05:21Z

Here's a simple rebase on top of a native descriptor wallet #16528 (benefit: gives access to full BIP44/49/84 address tree): https://github.com/Sjors/bitcoin/tree/2019/08/hww-box

Includes a mock to mimick the HWI interace.

Sjors · 2019-08-04T21:43:57Z

Closing in favor of the native descriptor edition in #16546.

Sjors mentioned this pull request Dec 10, 2018

Hardware wallet support #14145

Closed

Sjors force-pushed the 2018/11/rpc-signer branch from e021812 to a2b018d Compare December 10, 2018 14:56

DrahtBot added the Needs rebase label Dec 10, 2018

laanwj added the Wallet label Dec 11, 2018

Sjors force-pushed the 2018/11/rpc-signer branch from a2b018d to 9ec4aac Compare December 17, 2018 18:06

DrahtBot removed the Needs rebase label Dec 17, 2018