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

pull Sjors wants to merge 21 commits into bitcoin:master from Sjors:2018/11/rpc-signer changing 40 files +1690 −27
  1. 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 <cmd> for a list of signers (e.g. devices) and their master key fingerprint
    • signerfetchkeys (needs https://github.com/bitcoin-core/HWI/pull/137): asks <cmd> for descriptors and then fills the keypool (no private keys)
    • signerdisplayaddress <address>: asks <cmd> 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
  2. Sjors cross-referenced this on Dec 10, 2018 from issue Hardware wallet support by Sjors
  3. Sjors force-pushed on Dec 10, 2018
  4. DrahtBot added the label Needs rebase on Dec 10, 2018
  5. laanwj added the label Wallet on Dec 11, 2018
  6. 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.

    See also the wallet meeting notes.

  7. Sjors force-pushed on Dec 17, 2018
  8. DrahtBot removed the label Needs rebase on Dec 17, 2018
  9. DrahtBot commented at 8:12 PM on December 17, 2018: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

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

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

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

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

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

  15. 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?

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

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

  18. 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?

  19. 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()? :-)

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

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

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

  23. 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?

  24. 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?

  25. 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?

  26. 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?

  27. 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?

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

  29. 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 (.

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

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

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

  33. 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?

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

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

  36. DrahtBot added the label Needs rebase on Dec 24, 2018
  37. Sjors force-pushed on Jan 14, 2019
  38. Sjors force-pushed on Jan 14, 2019
  39. Sjors cross-referenced this on Jan 17, 2019 from issue gui: Add dynamic wallets support by promag
  40. Sjors force-pushed on Jan 18, 2019
  41. Sjors force-pushed on Jan 18, 2019
  42. Sjors force-pushed on Jan 18, 2019
  43. 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:

    1. Create wallet/rpcsigner.cpp and move the various functions there; wallet/rpcwallet.cpp and src/rpc/rpcdump.cpp are already big enough
    2. Clean up the way I made sendrawtransaction reusable (I just noticed #14978 already does that)
    3. Incorporate #14978 (reusable PSBT code, I might wait for that to be merged)
    4. Similar to #14978, find a way to make this code reusable, so I can build GUI support in a followup PR
  44. Sjors cross-referenced this on Jan 18, 2019 from issue Factor out PSBT utilities from RPCs for use in GUI code; related refactoring. by gwillen
  45. Sjors force-pushed on Jan 19, 2019
  46. Sjors force-pushed on Jan 19, 2019
  47. Sjors force-pushed on Jan 19, 2019
  48. fanquake removed the label Needs rebase on Jan 19, 2019
  49. Sjors force-pushed on Jan 19, 2019
  50. Sjors force-pushed on Jan 19, 2019
  51. 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

    <img width="899" alt="schermafbeelding 2019-01-19 om 19 08 39" src="https://user-images.githubusercontent.com/10217/51430571-a5a20980-1c1d-11e9-87b4-f4dab92e4dc3.png">

  52. Sjors cross-referenced this on Jan 21, 2019 from issue WIP: bitcoind signer support by Sjors
  53. Sjors cross-referenced this on Jan 21, 2019 from issue WIP: descriptor request using "getkeys" by Sjors
  54. 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?

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

  56. 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?

    static inline bool StartsWith(const std::string& input, const std::string& prefix) {
    return input.size() >= prefix.size() && input.compare(0, prefix.size(), prefix) == 0;
}
    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 :-)

  57. Sjors force-pushed on Jan 23, 2019
  58. Sjors force-pushed on Jan 23, 2019
  59. Sjors force-pushed on Jan 24, 2019
  60. Sjors force-pushed on Jan 24, 2019
  61. 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
  62. Sjors force-pushed on Jan 24, 2019
  63. Sjors force-pushed on Jan 24, 2019
  64. DrahtBot added the label Needs rebase on Jan 30, 2019
  65. Sjors force-pushed on Feb 1, 2019
  66. DrahtBot removed the label Needs rebase on Feb 1, 2019
  67. Sjors force-pushed on Feb 1, 2019
  68. Sjors force-pushed on Feb 1, 2019
  69. DrahtBot added the label Needs rebase on Feb 1, 2019
  70. Sjors force-pushed on Feb 2, 2019
  71. 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.

  72. 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!

  73. Sjors force-pushed on Feb 2, 2019
  74. Sjors cross-referenced this on Feb 3, 2019 from issue gui: When private key is disabled, only show watch-only balance by ken2812221
  75. Sjors force-pushed on Feb 9, 2019
  76. DrahtBot removed the label Needs rebase on Feb 9, 2019
  77. Sjors cross-referenced this on Feb 10, 2019 from issue Tools for deterministic builds of standalone binaries and distribution archives by achow101
  78. Sjors force-pushed on Feb 10, 2019
  79. DrahtBot added the label Needs rebase on Feb 10, 2019
  80. Sjors cross-referenced this on Feb 11, 2019 from issue util: add RunCommandParseJSON by Sjors
  81. 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.

  82. Sjors cross-referenced this on Feb 14, 2019 from issue TODO for 1.0 release by achow101
  83. meshcollider commented at 11:56 PM on February 14, 2019: contributor

    Time for a big rebase 🎉

  84. Sjors cross-referenced this on Feb 15, 2019 from issue [wallet] allow adding pubkeys from imported private keys to keypool by Sjors
  85. Sjors force-pushed on Feb 15, 2019
  86. DrahtBot removed the label Needs rebase on Feb 15, 2019
  87. Sjors force-pushed on Feb 15, 2019
  88. Sjors cross-referenced this on Feb 15, 2019 from issue [test] functional: allow custom cwd, use tmpdir as default by Sjors
  89. 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:

    1. A way to construct descriptors from code, to get rid of the string concatenation mess in signerfetchkeys (separate PR)
    2. Have the device sign one or more messages using the keys after importing with signerfetchkeys
  90. Sjors force-pushed on Feb 15, 2019
  91. Sjors force-pushed on Feb 15, 2019
  92. Sjors force-pushed on Feb 15, 2019
  93. DrahtBot added the label Needs rebase on Feb 16, 2019
  94. Sjors cross-referenced this on Feb 26, 2019 from issue [WIP] descriptor based wallet serialization and import by Sjors
  95. Sjors force-pushed on Mar 8, 2019
  96. Sjors force-pushed on Mar 8, 2019
  97. Sjors force-pushed on Mar 8, 2019
  98. 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).

  99. DrahtBot removed the label Needs rebase on Mar 8, 2019
  100. Sjors cross-referenced this on Mar 8, 2019 from issue Enhance `bumpfee` to include inputs when targeting a feerate by instagibbs
  101. Sjors cross-referenced this on Mar 9, 2019 from issue Make OutputType consistent with Descriptor and return it by Sjors
  102. Sjors force-pushed on Mar 9, 2019
  103. Sjors cross-referenced this on Mar 13, 2019 from issue Descriptor: add GetAddressType() and IsSegWit() by Sjors
  104. Sjors force-pushed on Mar 15, 2019
  105. Sjors cross-referenced this on Mar 15, 2019 from issue Add option to enter commands over stdin by achow101
  106. Sjors force-pushed on Mar 15, 2019
  107. Sjors force-pushed on Mar 15, 2019
  108. DrahtBot added the label Needs rebase on Mar 21, 2019
  109. Sjors force-pushed on Apr 15, 2019
  110. Sjors force-pushed on Apr 15, 2019
  111. Sjors force-pushed on Apr 15, 2019
  112. Sjors force-pushed on Apr 16, 2019
  113. Sjors force-pushed on Apr 16, 2019
  114. Sjors force-pushed on Apr 16, 2019
  115. ryanofsky cross-referenced this on Apr 17, 2019 from issue refactor: Replace chain relayTransactions/submitMemoryPool by higher method by ariard
  116. Sjors cross-referenced this on Apr 23, 2019 from issue [rpc] signer send and fee bump convenience methods by Sjors
  117. Sjors force-pushed on Apr 24, 2019
  118. DrahtBot removed the label Needs rebase on Apr 24, 2019
  119. Sjors force-pushed on Apr 25, 2019
  120. DrahtBot added the label Needs rebase on Apr 26, 2019
  121. Sjors force-pushed on Apr 27, 2019
  122. Sjors force-pushed on Apr 27, 2019
  123. 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.

  124. 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?

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

  126. Sjors force-pushed on May 12, 2019
  127. DrahtBot removed the label Needs rebase on May 13, 2019
  128. Sjors force-pushed on May 14, 2019
  129. Sjors force-pushed on May 14, 2019
  130. Sjors force-pushed on May 14, 2019
  131. 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.

  132. Sjors force-pushed on May 14, 2019
  133. jb55 commented at 2:30 PM on May 24, 2019: contributor

    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?

  134. DrahtBot added the label Needs rebase on May 29, 2019
  135. Sjors force-pushed on Jun 6, 2019
  136. DrahtBot removed the label Needs rebase on Jun 6, 2019
  137. DrahtBot added the label Needs rebase on Jun 6, 2019
  138. Sjors force-pushed on Jun 7, 2019
  139. Sjors force-pushed on Jun 7, 2019
  140. 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).

  141. DrahtBot removed the label Needs rebase on Jun 7, 2019
  142. DrahtBot added the label Needs rebase on Jun 19, 2019
  143. Sjors force-pushed on Jul 5, 2019
  144. 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.

    1. move all of externalsigner.{h,cpp} into a ExternalSignerScriptPubKeyMan
    2. Get rid of signerfetchkeys RPC; createwallet and keypoolrefill cover this
    3. 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.
    4. 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.
    5. add a feature flag that this wallet should use ExternalSignerScriptPubKeyMan
    6. store the device fingerprint in the wallet metadata at creation time, so enumeratesigners is unnecessary after wallet creation
    7. 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.
    8. 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.
    9. signerdisplayaddress remains unchanged
    10. signerprocesspsbt can be melted into processpsbt thanks to the feature flag

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

  145. Sjors cross-referenced this on Jul 11, 2019 from issue Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101
  146. Sjors cross-referenced this on Jul 12, 2019 from issue The ultimate send RPC by Sjors
  147. configure: Clone ax_boost_chrono to ax_boost_process f8bf5b14e2
  148. 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
  149. [doc] include Doxygen comments for HAVE_BOOST_PROCESS 9edd3a23b5
  150. [test] framework: add skip_if_no_runcommand c7e3d7094c
  151. [util] add runCommandParseJSON fb4a42429d
  152. [build] add IO support for Boost::Optional 2ccb3b5c57
  153. Add AddressType (base58, bech32) f721b78272
  154. Descriptor: add GetAddressType() 614cdee32d
  155. Add IsSegWit() to Descriptor 3a9d515b21
  156. [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
  157. Sjors force-pushed on Aug 2, 2019
  158. 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.

  159. DrahtBot removed the label Needs rebase on Aug 2, 2019
  160. Sjors force-pushed on Aug 3, 2019
  161. 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

  162. Sjors cross-referenced this on Aug 3, 2019 from issue Add getdescriptors command by Sjors
  163. [test] add external signer test
    Includes a mock to mimick the HWI interace.
    b52e11d731
  164. [rpc] add external signer RPC files 1b80eb81a2
  165. [rpc] signer: add enumeratesigners to list external signers 9f5cf3d672
  166. [rpc] make ProcessImport public 072bf754b4
  167. [rpc] signer: GetSignerForJSONRPCRequest 527f1ec69d
  168. [rpc] signer: add ParseDescriptor afc45ffda2
  169. [rpc] signer: add signerfetchkeys to import keys from signer 4bc560843d
  170. [rpc] signer: add signerdissociate ed0c60f527
  171. [rpc] signer: add signerdisplayaddress a80fa9a40e
  172. [rpc] signer: add signerprocesspsbt 84589e5181
  173. [doc] add external-signer.md 264425be1f
  174. Sjors force-pushed on Aug 4, 2019
  175. Sjors cross-referenced this on Aug 4, 2019 from issue External signer support - Wallet Box edition by Sjors
  176. Sjors commented at 9:43 PM on August 4, 2019: member

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

  177. Sjors closed this on Aug 4, 2019
  178. bitcoin deleted a comment on Dec 8, 2020
  179. bitcoin locked this on Dec 8, 2020
  180. Sjors deleted the branch on Dec 8, 2020
Contributors
SjorsDrahtBotpracticalswiftken2812221promagryanofskyjonasschnelliMarcoFalkemeshcolliderjnewberyluke-jrjb55
Labels
Wallet
Linked (view graph)
#13100 gui: Add dynamic wallets support #13966 gui: When private key is disabled, only show watch-only balance#14021 Import key origin data through descriptors in importmulti#14075 Import watch only pubkeys to the keypool if private keys are disabled#14145 Hardware wallet support#14491 Allow descriptor imports with importmulti#14565 Overhaul importmulti logic#14646 Add expansion cache functions to descriptors (unused for now)#14938 Support creating an empty wallet#14978 Factor out PSBT utilities from RPCs for use in GUI code; related refactoring.#15226 Allow creating blank (empty) wallets (alternative)#15382 util: add RunCommandParseJSON#15414 [wallet] allow adding pubkeys from imported private keys to keypool#15415 [test] functional: allow custom cwd, use tmpdir as default#15457 Check std::system for -[alert|block|wallet]notify#15487 [WIP] descriptor based wallet serialization and import#15557 Enhance `bumpfee` to include inputs when targeting a feerate#15567 Make OutputType consistent with Descriptor and return it#15590 Descriptor: add GetAddressType() and IsSegWit()#15713 refactor: Replace chain relayTransactions/submitMemoryPool by higher method#15741 Batch write imported stuff in importmulti#15764 Native descriptor wallets#15876 [rpc] signer send and fee bump convenience methods#15911 Use wallet RBF default for walletcreatefundedpsbt#16341 Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes)#16378 The ultimate send RPC#16528 Native Descriptor Wallets using DescriptorScriptPubKeyMan#16546 External signer support - Wallet Box edition
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:54 UTC