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

Sjors commented at 2:46 pm on December 10, 2018: member

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 https://github.com/bitcoin-core/HWI/pull/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:

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

Potentially useful followups:

#15876: signer send and bumpfee conveniance methods
#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 force-pushed on Dec 10, 2018

DrahtBot added the label Needs rebase on Dec 10, 2018

laanwj added the label Wallet on Dec 11, 2018

Sjors commented at 11:22 am on December 15, 2018: member

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.

in src/wallet/rpcwallet.h:33 in 9ec4aaca28 outdated

24@@ -24,6 +25,17 @@ void RegisterWalletRPCCommands(CRPCTable &t);
25  */
26 std::shared_ptr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& request);
27 
28+/**
29+ * Figures out what external signer to use for a JSONRPCRequest.
30+ *
31+ * @param[in] request JSONRPCRequest that wishes to access a signer
32+ * @param[pos] which argument contains the signer fingerprint. Optional, returns the first signer otherwise
33+ * @param[pwallet] the wallet

practicalswift commented at 4:19 pm on December 18, 2018:

The documentation format is incorrect here :-)

Sjors commented at 11:32 am on December 20, 2018:

Thanks, I’ll look into how that’s supposed to work…

in src/wallet/externalsigner.cpp:28 in 9ec4aaca28 outdated

18+        if (result.isNull())
19+            throw ExternalSignerException(strprintf("'%s' received invalid response, missing signer fingerprint", command));
20+        std::string fingerprintStr = fingerprint.get_str();
21+        // Skip duplicate signer
22+        bool duplicate = false;
23+        for (ExternalSigner signer : signers) {

practicalswift commented at 4:20 pm on December 18, 2018:

ExternalSigner signer shadows UniValue signer :-)

in src/wallet/rpcdump.cpp:1123 in 9ec4aaca28 outdated

1249-        pwallet->MarkDirty();
1250+    const UniValue& priv_keys = data.exists("keys") ? data["keys"].get_array() : UniValue();
1251+
1252+    // Expand all descriptors to get public keys and scripts.
1253+    // TODO: get private keys from descriptors too
1254+    for (int i = range_start; i <= range_end; ++i) {

practicalswift commented at 4:22 pm on December 18, 2018:

The type of i should match the type of range_start to avoid implicit precision losing conversion?

Sjors commented at 5:13 pm on January 18, 2019:

Upstream

in src/wallet/rpcdump.cpp:1150 in 9ec4aaca28 outdated

1315-                throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid private key encoding");
1316-            }
1317+static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)
1318+{
1319+    try {
1320+        bool watch_only = data.exists("watchonly") ? data["watchonly"].get_bool() : false;

practicalswift commented at 4:23 pm on December 18, 2018:

Should be dropped since unused?

Sjors commented at 5:13 pm on January 18, 2019:

(fixed upstream)

in src/wallet/rpcdump.cpp:1548 in 9ec4aaca28 outdated

1540+
1541+    // Use importmulti to process the descriptors:
1542+    UniValue importdata(UniValue::VARR);
1543+
1544+    uint64_t keypool_target_size = 0;
1545+    keypool_target_size = gArgs.GetArg("-keypool", DEFAULT_KEYPOOL_SIZE);

practicalswift commented at 4:24 pm on December 18, 2018:

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

Sjors commented at 11:34 am on December 20, 2018:

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

in src/util/system.cpp:1153 in 9ec4aaca28 outdated

1199@@ -1198,6 +1200,24 @@ void runCommand(const std::string& strCommand)
1200         LogPrintf("runCommand error: system(%s) returned %d\n", strCommand, nErr);
1201 }
1202 
1203+UniValue runCommandParseJSON(const std::string& strCommand)
1204+{
1205+    if (strCommand.empty()) return UniValue::VNULL;
1206+
1207+    std::array<char, 128> buffer;

practicalswift commented at 4:29 pm on December 18, 2018:

Initialize to zero?

in src/wallet/rpcwallet.cpp:4018 in 9ec4aaca28 outdated

4009@@ -3988,6 +4010,37 @@ UniValue walletprocesspsbt(const JSONRPCRequest& request)
4010     bool bip32derivs = request.params[3].isNull() ? false : request.params[3].get_bool();
4011     bool complete = FillPSBT(pwallet, psbtx, nHashType, sign, bip32derivs);
4012 
4013+    // Process again with external signer if needed
4014+    if (!complete && sign && !wallet->m_external_signers.empty()) {
4015+        ExternalSigner *signer = GetSignerForJSONRPCRequest(request, 4, pwallet);
4016+        if (signer && !bip32derivs) JSONRPCError(RPC_WALLET_ERROR, "Using signer requires bip32derivs argument to be true");
4017+
4018+        // Check if signer fingerpint matches any input master key fingerprint

practicalswift commented at 4:31 pm on December 18, 2018:

Should be “fingerprint” :-)

in src/util/system.cpp:1155 in 9ec4aaca28 outdated

1204+{
1205+    if (strCommand.empty()) return UniValue::VNULL;
1206+
1207+    std::array<char, 128> buffer;
1208+    std::string result;
1209+    std::unique_ptr<FILE, decltype(&pclose)> pipe(popen(strCommand.c_str(), "r"), pclose);

practicalswift commented at 4:38 pm on December 18, 2018:

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

Sjors commented at 11:35 am on December 20, 2018:

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.

ken2812221 commented at 6:30 pm on January 20, 2019:

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

promag commented at 6:32 pm on January 20, 2019:

I think we don’t depend on boost process.

ken2812221 commented at 6:43 pm on January 20, 2019:

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.

ryanofsky commented at 6:42 pm on January 22, 2019:

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.

ken2812221 commented at 7:20 pm on January 22, 2019:

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

in src/util/strencodings.cpp:600 in 9ec4aaca28 outdated

595+        if (i >> 31) ret += '\'';
596+    }
597+    return ret;
598+}
599+
600+std::string WriteHdKeypath(const std::vector<uint32_t> keypath)

practicalswift commented at 4:38 pm on December 18, 2018:

keypath should be const reference?

in src/wallet/rpcwallet.cpp:4168 in 9ec4aaca28 outdated

4190+            "}\n"
4191+        );
4192+    }
4193+
4194+    const std::string command = gArgs.GetArg("-signer", DEFAULT_EXTERNAL_SIGNER);
4195+    if (command == "") throw JSONRPCError(RPC_WALLET_ERROR, "Error: restart bitcoind with -signer=<cmd>");

practicalswift commented at 4:42 pm on December 18, 2018:

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

in src/wallet/externalsigner.cpp:21 in 9ec4aaca28 outdated

11+{
12+    // Call <command> enumerate
13+    const UniValue result = runCommandParseJSON(command + " enumerate");
14+    if (!result.isArray())
15+        throw ExternalSignerException(strprintf("'%s' received invalid response, expected array of signers", command));
16+    for (UniValue signer : result.getValues()) {

practicalswift commented at 4:43 pm on December 18, 2018:

Should be const reference? :-)

in src/wallet/externalsigner.cpp:25 in 9ec4aaca28 outdated

15+        throw ExternalSignerException(strprintf("'%s' received invalid response, expected array of signers", command));
16+    for (UniValue signer : result.getValues()) {
17+        const UniValue& fingerprint = find_value(signer, "fingerprint");
18+        if (result.isNull())
19+            throw ExternalSignerException(strprintf("'%s' received invalid response, missing signer fingerprint", command));
20+        std::string fingerprintStr = fingerprint.get_str();

practicalswift commented at 4:43 pm on December 18, 2018:

Should be const reference? :-)

in src/wallet/externalsigner.cpp:29 in 9ec4aaca28 outdated

19+            throw ExternalSignerException(strprintf("'%s' received invalid response, missing signer fingerprint", command));
20+        std::string fingerprintStr = fingerprint.get_str();
21+        // Skip duplicate signer
22+        bool duplicate = false;
23+        for (ExternalSigner signer : signers) {
24+            if (signer.m_fingerprint.compare(fingerprintStr) == 0) duplicate = true;

practicalswift commented at 4:44 pm on December 18, 2018:

Use string equality operator instead of compare :-)

in src/wallet/rpcwallet.cpp:4023 in 9ec4aaca28 outdated

4018+        // Check if signer fingerpint matches any input master key fingerprint
4019+        bool match = false;
4020+        for (unsigned int i = 0; i < psbtx.inputs.size(); ++i) {
4021+            const PSBTInput& input = psbtx.inputs[i];
4022+            for (auto entry : input.hd_keypaths) {
4023+                if (signer->m_fingerprint == strprintf("%08x", ReadBE32(entry.second.fingerprint))) match = true;

practicalswift commented at 4:47 pm on December 18, 2018:

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

in src/wallet/rpcwallet.cpp:4030 in 9ec4aaca28 outdated

4025+        }
4026+
4027+        if (!match) JSONRPCError(RPC_WALLET_ERROR, "Signer fingerprint does not match any of the inputs");
4028+
4029+        // Call signer, get result
4030+        const UniValue signer_result = signer->signTransaction(request.params[0].get_str());

practicalswift commented at 4:47 pm on December 18, 2018:

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

in src/util/strencodings.h:204 in 9ec4aaca28 outdated

203@@ -204,6 +204,10 @@ bool ConvertBits(const O& outfn, I it, I end) {
204 /** Parse an HD keypaths like "m/7/0'/2000". */
205 NODISCARD bool ParseHDKeypath(const std::string& keypath_str, std::vector<uint32_t>& keypath);
206 
207+/** Write HD keypaths as strings */
208+std::string WriteHdKeypath(const std::vector<uint32_t> keypath);

practicalswift commented at 4:48 pm on December 18, 2018:

keypath should be const reference?

in src/wallet/rpcdump.cpp:1506 in 9ec4aaca28 outdated

1498+
1499+    const std::string receive_desc = desc_prefix + signer->m_fingerprint + "/" + purpose + "/" + (signer->m_mainnet ? "0h" : "1h") + "/0h/0/*" + desc_suffix;
1500+    UniValue receive_descriptors = signer->getKeys(receive_desc);
1501+    if (!receive_descriptors.isArray()) JSONRPCError(RPC_WALLET_ERROR, "Expected an array of receive descriptors");
1502+    for (const UniValue& descriptor : receive_descriptors.getValues()) {
1503+        descriptors.push_back(descriptor);

practicalswift commented at 4:50 pm on December 18, 2018:

Nit: Could use std::copy instead?

in src/wallet/rpcdump.cpp:1539 in 9ec4aaca28 outdated

1531+
1532+    const std::string change_desc = desc_prefix + signer->m_fingerprint + "/" + purpose + "/" + (signer->m_mainnet ? "0h" : "1h") + "/0h/1/*" + desc_suffix;
1533+    UniValue change_descriptors = signer->getKeys(change_desc);
1534+    if (!change_descriptors.isArray()) JSONRPCError(RPC_WALLET_ERROR, "Expected an array of change descriptors");
1535+    for (const UniValue& descriptor : change_descriptors.getValues()) {
1536+        descriptors.push_back(descriptor);

practicalswift commented at 4:50 pm on December 18, 2018:

Same here: Could you std::copy?

in src/wallet/rpcdump.cpp:1478 in 9ec4aaca28 outdated

1470+
1471+    UniValue descriptors = UniValue(UniValue::VARR);
1472+    std::string desc_prefix = "";
1473+    std::string desc_suffix = "";
1474+    std::string purpose = "";
1475+    switch(pwallet->m_default_address_type) {

practicalswift commented at 4:52 pm on December 18, 2018:

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

in src/wallet/rpcdump.cpp:1510 in 9ec4aaca28 outdated

1502+    for (const UniValue& descriptor : receive_descriptors.getValues()) {
1503+        descriptors.push_back(descriptor);
1504+    }
1505+
1506+
1507+    switch(pwallet->m_default_change_type) {

practicalswift commented at 4:53 pm on December 18, 2018:

Same here: Missing whitespace before (.

in src/wallet/rpcwallet.cpp:4035 in 9ec4aaca28 outdated

4030+        const UniValue signer_result = signer->signTransaction(request.params[0].get_str());
4031+        if (!find_value(signer_result, "psbt").isStr()) JSONRPCError(RPC_WALLET_ERROR, "Unexpected result from signer");
4032+
4033+        // Process result from signer:
4034+        std::string signer_psbt_error;
4035+        PartiallySignedTransaction signer_psbtx ;

practicalswift commented at 4:53 pm on December 18, 2018:

Remove space before ; :-)

in test/functional/wallet_importmulti.py:52 in 9ec4aaca28 outdated

130+    def test_importmulti(self, req, success, error_code=None, error_message=None, warnings=[]):
131         """Run importmulti and assert success"""
132         result = self.nodes[1].importmulti([req])
133+        observed_warnings = []
134+        if 'warnings' in result[0]:
135+           observed_warnings = result[0]['warnings']

practicalswift commented at 4:55 pm on December 18, 2018:

Indentation is not a multiple of four :-)

Sjors commented at 5:10 pm on January 18, 2019:

Upstream

in src/wallet/rpcdump.cpp:1557 in 9ec4aaca28 outdated

1549+    for (unsigned int i = 0; i < descriptors.size(); ++i) {
1550+        const UniValue& descriptor = descriptors.getValues()[i];
1551+        // TODO: sanity check the descriptors:
1552+        // * check if they're valid descriptors
1553+        // * check that it's the fingerprint we asked for
1554+        // * check it's the deriviation path we asked for

practicalswift commented at 4:57 pm on December 18, 2018:

Should be “derivation” :-)

in src/wallet/rpcwallet.cpp:4203 in 9ec4aaca28 outdated

4227+
4228+    ExternalSigner *signer = GetSignerForJSONRPCRequest(request, 1, pwallet);
4229+
4230+    LOCK(pwallet->cs_wallet);
4231+
4232+    UniValue ret(UniValue::VOBJ);

practicalswift commented at 4:58 pm on December 18, 2018:

Unused?

jonasschnelli commented at 7:12 pm on December 18, 2018: contributor

Great work! I think the signer API (calling an external script/application) seems fine. We should make sure all calls are non-blocking sync it could be, that the signer application has a GUI and requires user interaction on all non-obvious commands like “displayaddress”. Also, the device could require initialisation which could be triggered by a first display/sign command.

Sjors commented at 11:44 am on December 20, 2018: member

@jonasschnelli I have a idea on how to allow asynchronous interaction. That also makes sense if you’re using an online service with a 48 hour cool down period.

The signtransaction command (called by processpsbt in this PR) could take an optional ephemeral public key argument. The commands then immediately returns with a UUID and a timestamp for when the client should come back. We could then add a polltransaction command which takes the UUID and returns the encrypted transaction (or nothing), encrypted using the ephemeral public key. This would involve making the wallet aware of pending transactions, and perhaps an additional RPC call like walletprocesspendingtransactions.

Device initialization could be a new RPC call, and other calls would just throw an error if not initialized. The enumeratesigners call can return more information about the device, such as the initialization status.

DrahtBot added the label Needs rebase on Dec 24, 2018

Sjors force-pushed on Jan 14, 2019

Sjors force-pushed on Jan 18, 2019

Sjors commented at 5:28 pm on January 18, 2019: member

I added a signersend RPC method which Just Works(tm) by combining the functionality of walletcreatefundedpsbt, signerprocesspsbt, finalizepsbt and sendrawtransaction. Updated the documentation.

Addressed some of the nits. Still much to improve in terms of code quality, and I’m waiting on multiple upstream pull requests.

The most useful review at this point is to test the workflow. In addition, feedback on runCommandParseJSON() is welcome.

Some things I plan to work on next:

Create wallet/rpcsigner.cpp and move the various functions there; wallet/rpcwallet.cpp and src/rpc/rpcdump.cpp are already big enough
Clean up the way I made sendrawtransaction reusable (I just noticed #14978 already does that)
Incorporate #14978 (reusable PSBT code, I might wait for that to be merged)
Similar to #14978, find a way to make this code reusable, so I can build GUI support in a followup PR

Sjors force-pushed on Jan 19, 2019

fanquake removed the label Needs rebase on Jan 19, 2019

Sjors force-pushed on Jan 19, 2019

Sjors commented at 6:08 pm on January 19, 2019: member

Rebased on the latest hww branch. Moved a bunch of things to wallet/rpcsigner.cpp. @ken2812221 any idea how I can make AppVeyor happy? My guess is that it doesn’t like runCommandParseJSON() in system.cpp

in src/rpc/rawtransaction.cpp:1008 in 5297b78e79 outdated

1028-            + HelpExampleCli("sendrawtransaction", "\"signedhex\"") +
1029-            "\nAs a JSON-RPC call\n"
1030-            + HelpExampleRpc("sendrawtransaction", "\"signedhex\"")
1031-        );
1032-
1033+std::string sendrawtransaction(std::string txhex, bool allowhighfees) {

practicalswift commented at 8:37 am on January 22, 2019:

txhex should be const ref?

in src/wallet/rpcsigner.cpp:403 in 5297b78e79 outdated

411+
412+    RPCTypeCheck(request.params, {UniValue::VSTR, UniValue::VSTR});
413+
414+    // Unserialize the transaction
415+    PartiallySignedTransaction psbtx;
416+    std::string error;

practicalswift commented at 8:39 am on January 22, 2019:

error shadows function error from L62 in src/util/system.h.

in src/wallet/rpcsigner.cpp:184 in 5297b78e79 outdated

172+    //       not useful (or is it??).
173+
174+    std::string prefix = "";
175+    std::string postfix = "";
176+
177+    if (inferredDescriptor.find("wpkh") == 0) {

practicalswift commented at 9:13 am on January 22, 2019:

compare is more efficient than find when checking prefixes, right?

What about StartsWith(inferredDescriptor, "wpkh") instead?

0static inline bool StartsWith(const std::string& input, const std::string& prefix) {
1    return input.size() >= prefix.size() && input.compare(0, prefix.size(), prefix) == 0;
2}

Sjors commented at 5:11 pm on January 22, 2019:

I plan to ditch this part of the code, so no need to improve it :-)

Sjors force-pushed on Jan 23, 2019

Sjors force-pushed on Jan 24, 2019

Sjors commented at 11:16 am on January 24, 2019: member

updated to incorporate the latest changes in https://github.com/achow101/HWI/pull/73 and https://github.com/achow101/bitcoin/tree/hww
added (BIP32) account argument to signerfetchkeys (will add custom descriptor arguments later)
using RPCHelpMan (surviving the Travis linter again)
using inferred descriptor for signerdisplayaddress
included commits from #14978 to clean up PSBT & sendrawtransaction stuff (and fix Travis)
added the most important remaining todo’s to the PR description

Sjors force-pushed on Jan 24, 2019

DrahtBot added the label Needs rebase on Jan 30, 2019

Sjors force-pushed on Feb 1, 2019

DrahtBot removed the label Needs rebase on Feb 1, 2019

Sjors force-pushed on Feb 1, 2019

DrahtBot added the label Needs rebase on Feb 1, 2019

Sjors force-pushed on Feb 2, 2019

Sjors commented at 1:20 pm on February 2, 2019: member

I changed signerfetchkeys to use getxpub instead of getkeys. You can now use the master branch of HWI, except for displayaddress.

I’ll deal with the rebase once a bit more upstream stuff is merged.

in src/wallet/rpcsigner.cpp:106 in 6323969eef outdated

101+            RPCHelpMan{"signerdissociate",
102+                "Disossociates external signer from the wallet.\n",
103+                {
104+                    {"fingerprint", RPCArg::Type::STR, /* opt */ true, /* default_val */ "", "Master key fingerprint of signer"},
105+                },
106+                RPCResult{""},

MarcoFalke commented at 9:50 pm on February 2, 2019:

The result is “null”, not “”. This causes the crash.

Sjors commented at 10:00 pm on February 2, 2019:

Fixed!

Sjors force-pushed on Feb 2, 2019

Sjors force-pushed on Feb 9, 2019

DrahtBot removed the label Needs rebase on Feb 9, 2019

Sjors force-pushed on Feb 10, 2019

DrahtBot added the label Needs rebase on Feb 10, 2019

jonasschnelli commented at 2:17 am on February 14, 2019: contributor

Conceptually, I think we should initially create the option to allow a wallet to contain a main descriptor (main xpub). The scripts may be derived in-mem only during wallet load. If a form of “getnewaddress” (receive address) (child-pub-key-derviation) is supported, the wallet may want to remain the used child key indexes for the metadata storage rather than the pubkeyhash.

meshcollider commented at 11:56 pm on February 14, 2019: contributor

Time for a big rebase 🎉

Sjors force-pushed on Feb 15, 2019

DrahtBot removed the label Needs rebase on Feb 15, 2019

Sjors force-pushed on Feb 15, 2019

Sjors commented at 12:12 pm on February 15, 2019: member

Giant rebase done! I created separate pull request for a number of commits in order to keep discussion a bit focussed here. Please check the list at the bottom of the PR description before commenting.

There’s still two significant todo’s (plus cleanup) before this is really read for review, but more high level feedback is always welcome:

A way to construct descriptors from code, to get rid of the string concatenation mess in signerfetchkeys (separate PR)
Have the device sign one or more messages using the keys after importing with signerfetchkeys

Sjors force-pushed on Feb 15, 2019

DrahtBot added the label Needs rebase on Feb 16, 2019

Sjors force-pushed on Mar 8, 2019

Sjors commented at 2:30 pm on March 8, 2019: member

Rebased! I changed signerfetchkeys to call hwi.py getdescriptors (https://github.com/bitcoin-core/HWI/pull/137).

DrahtBot removed the label Needs rebase on Mar 8, 2019

Sjors force-pushed on Mar 9, 2019

Sjors force-pushed on Mar 15, 2019

DrahtBot added the label Needs rebase on Mar 21, 2019

Sjors force-pushed on Apr 15, 2019

Sjors force-pushed on Apr 16, 2019

Sjors force-pushed on Apr 24, 2019

DrahtBot removed the label Needs rebase on Apr 24, 2019

Sjors force-pushed on Apr 25, 2019

DrahtBot added the label Needs rebase on Apr 26, 2019

Sjors force-pushed on Apr 27, 2019

jnewbery commented at 11:03 pm on April 29, 2019: member

#15713 adds a broadcastTransaction() chain interface method which is required here, so would remove one of the commits from this PR.

luke-jr commented at 6:10 am on May 1, 2019: member

I don’t understand why users would apparently need to use new RPCs to achieve the same things they can already do?

Sjors commented at 12:15 pm on May 3, 2019: member

@luke-jr that’s true for enumerate which does exactly the same as hwi.py enumerate. But fetching keys, sending transactions and (as a followup) doing RBF is very tedious without these RPC methods. The same goes for generating a new receive address in the wallet and displaying it on the device.

The longer term goal is to get this functionality in the GUI, so I also see the RPC as a foundation for that (combined with making RPC code more reusable in general). It’s also much easier to write RPC tests than GUI tests.

Sjors force-pushed on May 12, 2019

DrahtBot removed the label Needs rebase on May 13, 2019

Sjors force-pushed on May 14, 2019

Sjors commented at 6:57 pm on May 14, 2019: member

I cleaned up signerfetchkeys a bit. It now takes advantage of (still to be discussed) #15590 Descriptor->IsSegWit() and Descriptor->GetAddressType() to pick the right descriptor for the given -addresstype and -changetype.

The RPC documentation warns the user not switch address types for the wallet (due to BIP44/49/84 interoperability), though in the long run native descriptor wallets provide better protection, by making keypool topup unnecessary.

It can now also topup the keypool, though the user needs to specify the range.

Sjors force-pushed on May 14, 2019

jb55 commented at 2:30 pm on May 24, 2019: member

Instead of adding more complexity into the core wallet, shouldn’t all this logic be handled externally? For example: I was envisioning importing all of your hw wallet keys into core. When you want to spend you would just ask core to give you a partially signed transaction from some of your unspent outputs.

I guess I’m confused as to why you would need core to call some external signer when PSBTs already support that use case?

DrahtBot added the label Needs rebase on May 29, 2019

Sjors force-pushed on Jun 6, 2019

DrahtBot removed the label Needs rebase on Jun 6, 2019

DrahtBot added the label Needs rebase on Jun 6, 2019

Sjors force-pushed on Jun 7, 2019

Sjors commented at 1:24 pm on June 7, 2019: member

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

DrahtBot removed the label Needs rebase on Jun 7, 2019

DrahtBot added the label Needs rebase on Jun 19, 2019

Sjors force-pushed on Jul 5, 2019

Sjors commented at 2:55 pm on July 11, 2019: member

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 #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

configure: Clone ax_boost_chrono to ax_boost_process f8bf5b14e2

Add boost::process

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

9dfb01b4a7

[doc] include Doxygen comments for HAVE_BOOST_PROCESS 9edd3a23b5

[test] framework: add skip_if_no_runcommand c7e3d7094c

[util] add runCommandParseJSON fb4a42429d

[build] add IO support for Boost::Optional 2ccb3b5c57

Add AddressType (base58, bech32) f721b78272

Descriptor: add GetAddressType() 614cdee32d

Add IsSegWit() to Descriptor 3a9d515b21

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

f0db4ea442

Sjors force-pushed on Aug 2, 2019

Sjors commented at 6:46 pm on August 2, 2019: member

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.

DrahtBot removed the label Needs rebase on Aug 2, 2019

Sjors force-pushed on Aug 3, 2019

Sjors commented at 11:05 am on August 3, 2019: member

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

[test] add external signer test

Includes a mock to mimick the HWI interace.

b52e11d731

[rpc] add external signer RPC files 1b80eb81a2

[rpc] signer: add enumeratesigners to list external signers 9f5cf3d672

[rpc] make ProcessImport public 072bf754b4

[rpc] signer: GetSignerForJSONRPCRequest 527f1ec69d

[rpc] signer: add ParseDescriptor afc45ffda2

[rpc] signer: add signerfetchkeys to import keys from signer 4bc560843d

[rpc] signer: add signerdissociate ed0c60f527

[rpc] signer: add signerdisplayaddress a80fa9a40e

[rpc] signer: add signerprocesspsbt 84589e5181

[doc] add external-signer.md 264425be1f

Sjors force-pushed on Aug 4, 2019

Sjors commented at 9:43 pm on August 4, 2019: member

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

Sjors closed this on Aug 4, 2019

fanquake deleted a comment on Dec 8, 2020

fanquake locked this on Dec 8, 2020

Sjors deleted the branch on Dec 8, 2020

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

Conflicts