rpc: add path to gethdkey #22341

pull Sjors wants to merge 24 commits into bitcoin:master from Sjors:2021/06/getxpub changing 23 files +1069 −191
  1. Sjors commented at 3:12 pm on June 25, 2021: member

    Depends on #26728.

    This PR expands the wallet RPC gethdkey introduced in #26728. It takes a BIP32 path as argument and returns the xpub, along with the master key fingerprint.

    To test this PR:

    • create a regular descriptor wallet
    • call getxpub m/86h/0h/0h
    • call listdescriptors, compare the xpub in the tr() descriptor with the previous step

    Bigger picture

    This paves the way for using Bitcoin Core as one signer in a multisig setup. It simplifies the proposed flow in #22067.

    The eventual flow would be:

    1. Create a blank wallet with a seed (either with no descriptors, or where its single key descriptors are not active, so they don’t get used when calling getnewaddress).
    2. Call getxpub m/87h/0h/0h (as suggested in BIP 87)
    3. (Manually, with Specter or with a simple Python utility - TBD): craft a multisig descriptor containing this xpub
    4. Call importdescriptors which will allow the import if its own fingerprint is recognized (and after checking the xpub)
    5. The usual flow with getnewaddress, send and walletprocesspsbt (and their GUI equivalents)

    This PR makes step (2) possible.

    Step (1) would require a followup, because blank wallets currently can’t have a seed. Step 4 would also require a followup, so that importdescriptors treats an xpub as if it was an xpriv, after checking that it can derive the xpub from the seed (that matches the fingerprint)

    On the GUI side a wizard could perform the above steps and guide the user, where they either:

    • export their own xpub and then import a descriptor from somewhere else (performing some sanity checks); or
    • import xpub(s) from other wallets, compose a multisig and get the descriptors for export; or
    • get xpub(s) from connected hardware devices and configure a multisig (with or without a key on their machine)
  2. in test/functional/wallet_getxpub.py:61 in 75d09830ca outdated 
    56+        w1.sendtoaddress(w2_address, 1)
57+        self.nodes[0].generate(1)
58+
59+        self.log.info("Spend from xpub")
60+        w1_address = self.nodes[0].getnewaddress()
61+        w2.sendtoaddress(w1_address, 0.1)
    Sjors commented at 3:17 pm on June 25, 2021:
    This fails in the current incarnation, because a freshly imported descriptor will not have access to the wallet seed. See todo in rpcdump.cpp
    lsilva01 commented at 2:09 am on July 10, 2021:
    Just confirming that I got the error: test_framework.authproxy.JSONRPCException: Signing transaction failed (-6)
  3. DrahtBot added the label RPC/REST/ZMQ on Jun 25, 2021
  4. DrahtBot added the label Wallet on Jun 25, 2021
  5. Sjors force-pushed on Jun 25, 2021
  6. DrahtBot commented at 11:39 pm on June 25, 2021: contributor

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

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK fjahr, Zero-1729, JeremyRubin, aureleoules, jonatack
    Approach ACK w0xlt
    Stale ACK lsilva01, mjdietzx, Rspigler

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #29154 (tests: improve wallet multisig descriptor test and docs by mjdietzx)
    • #29136 (wallet: addhdkey RPC to add just keys to wallets via new void(KEY) descriptor by achow101)
    • #29130 (wallet: Add createwalletdescriptor and gethdkeys RPCs for adding new automatically generated descriptors by achow101)
    • #29124 (wallet: Automatically repair corrupted metadata with doubled derivation path by achow101)
    • #29016 (RPC: add new listmempooltransactions by niftynei)
    • #28976 (wallet: Fix migration of blank wallets by achow101)
    • #28868 (wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs by achow101)
    • #28710 (Remove the legacy wallet and BDB dependency by achow101)
    • #28610 (wallet: Migrate entire address book entries to watchonly and solvables too by achow101)
    • #28574 (wallet: optimize migration process, batch db transactions by furszy)
    • #28333 (wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively by achow101)
    • #28192 (ParseHDKeypath: support h as hardened marker by Sjors)
    • #27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
    • #27286 (wallet: Keep track of the wallet’s own transaction outputs in memory by achow101)
    • #26008 (wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets by achow101)
    • #24748 (test/BIP324: functional tests for v2 P2P encryption by stratospher)
    • #22838 (descriptors: Be able to specify change and receiving in a single descriptor string by achow101)

    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.

  7. jonatack commented at 5:19 pm on June 26, 2021: contributor
    Thanks for working on this!
  8. in src/wallet/wallet.cpp:3277 in 1ff7a25d5a outdated 
    3272+    auto spk_man = GetScriptPubKeyMan(OutputType::BECH32, true);
3273+    if (!spk_man) return false;
3274+    auto desc_spk_man = dynamic_cast<DescriptorScriptPubKeyMan*>(spk_man);
3275+    CKey seed = desc_spk_man->GetKeys().begin()->second;
3276+    CExtKey master_key;
3277+    master_key.SetSeed(seed.begin(), seed.size());
    Sjors commented at 4:26 pm on June 28, 2021:
    In addition to being an ugly hack, this is also incorrect and does not actually produce the correct master key.
  9. Rspigler commented at 6:01 am on June 30, 2021: contributor

    Thanks so much for continuing to work on this! Love the use of BIP 87 :)

    Call importdescriptors which will allow the import if its own fingerprint is recognized (and after checking the xpub)

    Yes, important to check the xpub also, not just the fingerprint, which is easy to spoof

    • export their own xpub and then import a descriptor…
    • import xpub(s) from other wallets, compose a multisig…
    • get xpub(s) from connected hardware devices…

    I agree that the above 3 encompasses all use cases/needs for Bitcoin Core being a coordinator and a signer in a multisig setup.

    importdescriptors should treat an xpub as if it was an xpriv, after checking that it can derive the xpub from the seed (that matches the fingerprint); it needs to copy the seed from a sibling descriptor??

    Can you explain this to me more?

    As for the blank wallets, why do we need to be working from blank wallets, and if xprivs are derived from seeds, how can a blank wallet have a seed?

  10. Sjors commented at 1:39 pm on July 1, 2021: member

    It depends on how we want to define blank. It would be empty except for a seed, so without any descriptors in it.

    If a wallet has a seed and you call importdescriptors on it with some multisig descriptor, it should check the fingerprint(s) and path(s) mentioned in that descriptor. It should then derive the xpriv & xpub for that path, from the wallet seed, and check if it matches the descriptor xpub. In that case, the wallet should replace that xpub with the xpriv, as if the user provided it upon import. This avoid the dangerous act of exporting and re-importing an xpriv. That way, if you want to sign a transaction, the wallet knows it can provide one of the signatures.

  11. Rspigler commented at 6:46 pm on July 6, 2021: contributor

    Thank you, makes a lot of sense!

    And what about sibling descriptors?

  12. in src/wallet/wallet.cpp:3354 in 1ff7a25d5a outdated 
    3269+    }
3270+
3271+    // TODO: avoid needing a pre-existing ScriptPubKeyMan for obtaining the HD seed
3272+    auto spk_man = GetScriptPubKeyMan(OutputType::BECH32, true);
3273+    if (!spk_man) return false;
3274+    auto desc_spk_man = dynamic_cast<DescriptorScriptPubKeyMan*>(spk_man);
    lsilva01 commented at 1:58 am on July 10, 2021:

    I received the following error while executing ./src/bitcoin-cli -rpcwallet="descwallet" getxpub "m/87h/0h/0h":

    Assertion failed: lock cs_desc_man not held in wallet/scriptpubkeyman.cpp:1747; locks held: 'pwallet->cs_wallet' in wallet/rpcdump.cpp:1868 (in thread 'httpworker.0')

    I changed the code below (adding LOCK(desc_spk_man->cs_desc_man);) and then it worked.

    0    auto desc_spk_man = dynamic_cast<DescriptorScriptPubKeyMan*>(spk_man);
1    {
2        LOCK(desc_spk_man->cs_desc_man);
3        CKey seed = desc_spk_man->GetKeys().begin()->second;
4        // ...
5        std::copy(master_id.begin(), master_id.begin() + 4, info.fingerprint);
6        // TODO: copy path info origin info
7    }
  13. in test/functional/wallet_getxpub.py:65 in 1ff7a25d5a outdated 
    20+        self.skip_if_no_wallet()
21+        self.skip_if_no_sqlite()
22+
23+    def run_test(self):
24+        self.nodes[0].createwallet(wallet_name="w1")
25+        w1 = self.nodes[0].get_wallet_rpc("w1")
    lsilva01 commented at 2:05 am on July 10, 2021:

    With a wallet different from the default one, I receive the error when sending to w2 address. test_framework.authproxy.JSONRPCException: Insufficient funds (-6)

    With the change below, it worked.

    0        w1 = self.nodes[0].get_wallet_rpc(self.default_wallet_name)
  14. in test/functional/wallet_getxpub.py:57 in 1ff7a25d5a outdated 
    52+        # TODO: if createwallet supports setting a seed without descriptors,
53+        #       use that here instead. This allows replacing deriveaddresses
54+        #       with getnewaddress.
55+        w2_address = self.nodes[1].deriveaddresses(descsum_create(desc_receive), 0)[0]
56+        w1.sendtoaddress(w2_address, 1)
57+        self.nodes[0].generate(1)
    lsilva01 commented at 2:07 am on July 10, 2021:

    I think self.sync_all() is necessary for w2 to know that it received money.

    0        self.nodes[0].generate(1)
1        self.sync_all()
  16. mjdietzx commented at 9:30 pm on July 14, 2021: contributor

    Concept ACK

    I’ll be sure to update the tests/docs in #22067 to use this when this PR gets merged

  17. fjahr commented at 7:11 pm on August 1, 2021: contributor
    Concept ACK
  18. Zero-1729 commented at 6:29 am on August 2, 2021: contributor
    Concept ACK
  19. Sjors force-pushed on Oct 21, 2021
  20. Sjors commented at 6:24 pm on October 21, 2021: member
    I managed to reconstruct the master extended private key, by combining the key part of the descriptor CExtKey with the chaincode part of the CExtPubKey stored inside that descriptor. This code is absolute trash though. I’ll try to reuse some of @achow101’s work in https://github.com/achow101/bitcoin/commits/upgrade-to-tr-1 which tries to solve a similar problem, for adding a taproot descriptor to an existing descriptor wallet.
  21. Sjors force-pushed on Nov 5, 2021
  22. Sjors force-pushed on Nov 14, 2021
  23. Sjors force-pushed on Nov 17, 2021
  24. Sjors force-pushed on Nov 17, 2021
  25. Sjors force-pushed on Nov 17, 2021
  26. Sjors commented at 6:59 pm on November 17, 2021: member

    Ok, this is now ready for review.

    I’ll be sure to update the tests/docs in #22067 to use this when this PR gets merged

    I might also take a stab at this, once this PR (and its ancestor) is rebased to include it.

  27. Sjors marked this as ready for review on Nov 17, 2021
  28. DrahtBot added the label Needs rebase on Nov 22, 2021
  29. Sjors force-pushed on Nov 29, 2021
  30. Sjors commented at 8:19 am on November 29, 2021: member
    Rebased and updated the multisig descriptor test and example documentation to use getxpub. Unfortunately we still have to reuse existing wallet keys, e.g. m/44h/1h/0h, because importdescriptors can’t derive private keys yet.
  31. DrahtBot removed the label Needs rebase on Nov 29, 2021
  32. DrahtBot added the label Needs rebase on Dec 7, 2021
  33. Sjors force-pushed on Dec 10, 2021
  34. DrahtBot removed the label Needs rebase on Dec 10, 2021
  35. Sjors force-pushed on Dec 30, 2021
  36. Sjors force-pushed on Dec 30, 2021
  37. DrahtBot added the label Needs rebase on Jan 11, 2022
  38. Sjors force-pushed on Jan 12, 2022
  39. DrahtBot removed the label Needs rebase on Jan 12, 2022
  40. JeremyRubin commented at 3:44 pm on March 29, 2022: contributor

    concept ACK this + using core as a signer like this.

    One also cool idea would be is if, during PSBT signing code, we temporarily treat all descriptors there as “ours”?

  41. Sjors commented at 4:00 pm on March 29, 2022: member
    @JeremyRubin afaik descriptors are always treated as “ours”, both in private key wallets and watch-only wallets. The distinction between watch-only and regular addresses was dropped with the introduction descriptor wallets.
  42. mjdietzx commented at 8:49 pm on April 2, 2022: contributor
    @Sjors in the description of this PR you say “The current draft implementation is not usable.” what specifically are you referring to by that?
  43. Sjors commented at 8:35 pm on April 5, 2022: member
    @mjdietzx I think that refers to an older version of this PR that used a hideous hack, but the current code should be fine to review. I deleted that remark for the description.
  44. mjdietzx commented at 9:01 pm on April 6, 2022: contributor
    Code review ACK 8042053d0ddd662f654207d6554fb8f1e806f729
  45. in doc/descriptors.md:168 in 8042053d0d outdated 
    154@@ -155,8 +155,8 @@ The basic steps are:
155 
156   1. Every participant generates an xpub. The most straightforward way is to create a new descriptor wallet which we will refer to as
157      the participant's signer wallet. Avoid reusing this wallet for any purpose other than signing transactions from the
158-     corresponding multisig we are about to create. Hint: extract the wallet's xpubs using `listdescriptors` and pick the one from the
159-     `pkh` descriptor since it's least likely to be accidentally reused (legacy addresses)
160+     corresponding multisig we are about to create. Hint: extract the wallet's xpubs using `getxpub m/44h/0h/0h` (`m/44h/1h/0h` for testnet and signet). This reuses keys from the
    mjdietzx commented at 3:57 pm on April 19, 2022:

    nit: ParseHDKeypath won’t parse paths where the hardened derivation at a particular level is indicated by an h. So we probably want to update these docs, ie getxpub m/44'/0'/0'

    Is there any reason that ParseHDKeypath shouldn’t allow paths like this though?

    Sjors commented at 9:50 am on April 20, 2022:
    If using h doesn’t work that’s a bug. I’ll look into that (probably after the next rebase).
    Sjors commented at 9:27 am on August 10, 2022:
    The test cover both the use of ' and h. You’re not allowed to mix them in the same path though.
  46. mjdietzx commented at 4:01 pm on April 19, 2022: contributor

    Tested ACK 8042053d0ddd662f654207d6554fb8f1e806f729

    I went through a variety of edge-cases checking that getxpub works as expected for error cases in various test wallets I have setup

  47. DrahtBot added the label Needs rebase on Apr 24, 2022
  48. Sjors force-pushed on Apr 29, 2022
  49. DrahtBot removed the label Needs rebase on Apr 29, 2022
  50. DrahtBot added the label Needs rebase on May 27, 2022
  51. Rspigler commented at 6:02 am on July 11, 2022: contributor

    Tested commit 0ecd8a338e7f015e711cdc05ba005cdd322545e1

    Got this error when running make:

    CXX wallet/libbitcoin_wallet_a-keyman.o In file included from ./script/signingprovider.h:11, from ./wallet/crypter.h:10, from wallet/keyman.cpp:9: ./script/keyorigin.h: In member function ‘std::optional<std::pair<CExtPubKey, KeyOriginInfo> > wallet::KeyManager::GetExtPubKey(std::vector) const’: ./script/keyorigin.h:11:8: warning: ‘origin’ may be used uninitialized in this function [-Wmaybe-uninitialized] 11 | struct KeyOriginInfo | ^~~~~~~~~~~~~ wallet/keyman.cpp:90:19: note: ‘origin’ was declared here 90 | KeyOriginInfo origin; | ^~~~~~ CXX wallet/libbitcoin_wallet_a-load.o

    Still was able to compile

    Tests all pass

    Create a wallet with private keys disabled

    1. Create a blank wallet with a seed (single key descriptor that is not active) as described in OP.

    importdescriptors "[{ \"desc\": \"pk(0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798)#gn28ywm7\", \"active\": false, \"timestamp\": \"now\" }]"

    [ { “success”: true } ]

    1. Call getxpub m/87h/0h/0h

    getxpub m/87h/0h/0h

    This wallet does not have an active master extended key (code -4)

    Hm, ok - I’ll repeat with a descriptor wallet, but won’t be able to test the getnewaddress flow

    1. Create a regular descriptor wallet
    2. Call getxpub m/87h/0h/0h

    getxpub m/87h/0h/0h

    { “xpub”: “xpub6BxZHpQ2UJZML59GZRPirFsWFJDqcYn4jcM6zm5qHUZnvuCC7xmb86meQywCbKuGEiuh3XvrvG1bhRiQDsYGmGRhFkMRNqYnBVtJYPCVt43”, “fingerprint”: “5d340155”, “origin”: “[5d340155/87’/0’/0’]” }

    Test importdescriptors:

    importdescriptors "[{ \"desc\": \"wpkh([5d340155/87'/0'/0]xpub6BxZHpQ2UJZML59GZRPirFsWFJDqcYn4jcM6zm5qHUZnvuCC7xmb86meQywCbKuGEiuh3XvrvG1bhRiQDsYGmGRhFkMRNqYnBVtJYPCVt43/0/*)#zc6l5c3g\", \"timestamp\": \"now\" }]"

    [ { “success”: false, “error”: { “code”: -4, “message”: “Cannot import descriptor without private keys to a wallet with private keys enabled” }, “warnings”: [ “Range not given, using default keypool range” ] } ]

    Not sure how to proceed from here

  52. Sjors commented at 1:28 pm on July 13, 2022: member

    Got this error when running make: @Rspigler are you sure that warning is not related to #23417?

    I clarified the description a bit: the enumerated steps there would require more followups. I added a simpler procedure to manually test this PR.

  53. Rspigler commented at 4:52 pm on July 13, 2022: contributor

    I am able to compile and run #23417 without issue.

    But still get this error when compiling this PR:

    CXX wallet/libbitcoin_wallet_a-keyman.o In file included from ./script/signingprovider.h:11, from ./wallet/crypter.h:10, from wallet/keyman.cpp:9: ./script/keyorigin.h: In member function ‘std::optional<std::pair<CExtPubKey, KeyOriginInfo> > wallet::KeyManager::GetExtPubKey(std::vector) const’: ./script/keyorigin.h:11:8: warning: ‘origin’ may be used uninitialized in this function [-Wmaybe-uninitialized] 11 | struct KeyOriginInfo | ^~~~~~~~~~~~~ wallet/keyman.cpp:90:19: note: ‘origin’ was declared here 90 | KeyOriginInfo origin; | ^~~~~~ CXX wallet/libbitcoin_wallet_a-load.o

    I understand the PR better now, thank you for clarifying the description.

    create a regular descriptor wallet listdescriptors: …(listing taproot descriptors only):

      {
  "desc": "tr([3e89a5ab/86'/0'/0']xpub6DQdRUJdQPzZmkMkSANpS1tEfhM48yMoSXnacafKyCGdBCb8Ky6TUzygGsGF63bwnncozQsVYBAebo3BEqCLuZiPnh7EpCGEWwL13ap99jk/0/*)#p5us77gk",
  "timestamp": 1657730232,
  "active": true,
  "internal": false,
  "range": [
    0,
    999
  ],
  "next": 0
}

    {
   "desc": "tr([3e89a5ab/86'/0'/0']xpub6DQdRUJdQPzZmkMkSANpS1tEfhM48yMoSXnacafKyCGdBCb8Ky6TUzygGsGF63bwnncozQsVYBAebo3BEqCLuZiPnh7EpCGEWwL13ap99jk/1/*)#sqe3rtcw",
   "timestamp": 1657730234,
   "active": true,
   "internal": true,
   "range": [
     0,
     999
   ],
   "next": 0
}

    getxpub "m/86'/0'/0'"

     {
   "xpub": "xpub6DQdRUJdQPzZmkMkSANpS1tEfhM48yMoSXnacafKyCGdBCb8Ky6TUzygGsGF63bwnncozQsVYBAebo3BEqCLuZiPnh7EpCGEWwL13ap99jk",
  "fingerprint": "3e89a5ab",
  "origin": "[3e89a5ab/86'/0'/0']"
  }

    listdescriptors (listing taproot only)

           {
  "desc": "tr([3e89a5ab/86'/0'/0']xpub6DQdRUJdQPzZmkMkSANpS1tEfhM48yMoSXnacafKyCGdBCb8Ky6TUzygGsGF63bwnncozQsVYBAebo3BEqCLuZiPnh7EpCGEWwL13ap99jk/0/*)#p5us77gk",
     "timestamp": 1657730232,
     "active": true,
     "internal": false,
     "range": [
      0,
      999
      ],
     "next": 0
     },

            {
  "desc": "tr([3e89a5ab/86'/0'/0']xpub6DQdRUJdQPzZmkMkSANpS1tEfhM48yMoSXnacafKyCGdBCb8Ky6TUzygGsGF63bwnncozQsVYBAebo3BEqCLuZiPnh7EpCGEWwL13ap99jk/1/*)#sqe3rtcw",
      "timestamp": 1657730234,
      "active": true,
      "internal": true,
      "range": [
      0,
      999
       ],
      "next": 0
       }

    The taproot descriptors are the same

    :heavy_check_mark:

    tACK commit 0ecd8a338e7f015e711cdc05ba005cdd322545e1

  54. Sjors commented at 6:59 pm on July 13, 2022: member

    But still get this error when compiling this PR:

    Ok, I’ll look into that, probably after the next rebase.

  55. Sjors force-pushed on Aug 10, 2022
  56. DrahtBot removed the label Needs rebase on Aug 10, 2022
  57. Sjors force-pushed on Aug 10, 2022
  58. Sjors commented at 9:30 am on August 10, 2022: member
    Rebased. @Rspigler the uninitialized variable warning should be gone now.
  59. Rspigler commented at 7:12 pm on August 10, 2022: contributor

    tACK commit f6133f093e3e305b305506b68499e1ac6cde726b

    Yes, it is all good now

  60. DrahtBot added the label Needs rebase on Aug 17, 2022
  61. Sjors force-pushed on Aug 17, 2022
  62. Sjors commented at 11:25 am on August 17, 2022: member
    (trivial rebase, but not worth reviewing yet because there will be another one soon)
  63. DrahtBot removed the label Needs rebase on Aug 17, 2022
  64. Sjors force-pushed on Aug 18, 2022
  65. Sjors commented at 1:08 pm on August 18, 2022: member
    Rebased and fixed nodiscard warning introduced in #25642.
  66. DrahtBot added the label Needs rebase on Aug 19, 2022
  67. w0xlt commented at 11:37 pm on August 19, 2022: contributor
    Approach ACK
  68. Sjors force-pushed on Aug 22, 2022
  69. DrahtBot removed the label Needs rebase on Aug 22, 2022
  70. DrahtBot added the label Needs rebase on Sep 1, 2022
  71. Sjors force-pushed on Sep 2, 2022
  72. DrahtBot removed the label Needs rebase on Sep 2, 2022
  73. Sjors force-pushed on Sep 22, 2022
  74. in src/rpc/util.cpp:1096 in c76bb8335d outdated 
    1092@@ -1092,6 +1093,15 @@ std::vector<CScript> EvalDescriptorStringOrObject(const UniValue& scanobject, Fl
1093     return ret;
1094 }
1095 
1096+std::vector<uint32_t> ParsePathBIP32(const std::string path)
    aureleoules commented at 2:14 pm on October 4, 2022: 
    0std::vector<uint32_t> ParsePathBIP32(const std::string& path)
  75. in src/wallet/keyman.cpp:87 in c76bb8335d outdated 
    82+        return std::nullopt;
83+    }
84+    return m_active_xpub;
85+}
86+
87+std::optional<std::pair<CExtPubKey, KeyOriginInfo>> KeyManager::GetExtPubKey(const std::vector<uint32_t> path) const {
    aureleoules commented at 2:14 pm on October 4, 2022: 
    0std::optional<std::pair<CExtPubKey, KeyOriginInfo>> KeyManager::GetExtPubKey(const std::vector<uint32_t>& path) const {
  76. aureleoules commented at 2:30 pm on October 4, 2022: member
    Concept ACK & LGTM
  77. DrahtBot added the label Needs rebase on Oct 13, 2022
  78. Sjors force-pushed on Oct 27, 2022
  79. DrahtBot removed the label Needs rebase on Oct 27, 2022
  80. Sjors force-pushed on Nov 29, 2022
  81. Sjors force-pushed on Nov 29, 2022
  82. Sjors force-pushed on Jan 10, 2023
  83. Sjors commented at 1:49 pm on January 10, 2023: member
    Rebased on top of #26728 instead of #23417.
  84. jonatack commented at 6:38 pm on January 10, 2023: contributor
    Re-concept ACK (not sure if it’s ready for review).
  85. Sjors commented at 7:17 pm on January 10, 2023: member
    @jonatack it was a somewhat sloppy rebase. I also need to review the new base PR. Will mark as draft.
  86. Sjors marked this as a draft on Jan 10, 2023
  87. Sjors force-pushed on Jan 27, 2023
  88. Sjors force-pushed on Jan 27, 2023
  89. Sjors marked this as ready for review on Jan 27, 2023
  90. Sjors commented at 4:10 pm on January 27, 2023: member
    Alright, cleaned up a few things, this is ready for review again.
  91. Sjors force-pushed on Jan 27, 2023
  92. DrahtBot added the label Needs rebase on Feb 27, 2023
  93. Sjors force-pushed on Mar 1, 2023
  94. DrahtBot removed the label Needs rebase on Mar 1, 2023
  95. DrahtBot added the label CI failed on May 14, 2023
  96. maflcko commented at 9:59 am on June 5, 2023: member
    Maybe mark as draft if this depends on something?
  97. Sjors marked this as a draft on Jun 5, 2023
  98. Sjors force-pushed on Jun 5, 2023
  99. Sjors force-pushed on Jun 22, 2023
  100. DrahtBot added the label Needs rebase on Jun 28, 2023
  101. Sjors force-pushed on Jul 2, 2023
  102. DrahtBot removed the label Needs rebase on Jul 2, 2023
  103. Sjors force-pushed on Jul 31, 2023
  104. DrahtBot removed the label CI failed on Jul 31, 2023
  105. DrahtBot added the label Needs rebase on Sep 6, 2023
  106. Sjors force-pushed on Sep 7, 2023
  107. DrahtBot removed the label Needs rebase on Sep 7, 2023
  108. S3RK commented at 7:36 am on September 11, 2023: contributor
    The description now says the PR depends on itself :-)
  109. DrahtBot added the label Needs rebase on Oct 3, 2023
  110. Sjors force-pushed on Oct 6, 2023
  111. DrahtBot added the label CI failed on Oct 6, 2023
  112. DrahtBot removed the label Needs rebase on Oct 8, 2023
  113. Sjors force-pushed on Oct 26, 2023
  114. DrahtBot removed the label CI failed on Oct 26, 2023
  115. Sjors force-pushed on Nov 6, 2023
  116. DrahtBot added the label Needs rebase on Nov 7, 2023
  117. Sjors force-pushed on Nov 9, 2023
  118. Sjors renamed this:
    rpc: add getxpub
    rpc: add path to gethdkey
    on Nov 14, 2023
  119. Sjors force-pushed on Nov 14, 2023
  120. Sjors force-pushed on Dec 7, 2023
  121. Sjors force-pushed on Dec 7, 2023
  122. DrahtBot added the label CI failed on Dec 7, 2023
  123. DrahtBot removed the label Needs rebase on Dec 7, 2023
  124. Sjors force-pushed on Dec 7, 2023
  125. DrahtBot removed the label CI failed on Dec 7, 2023
  126. walletdb: Read and write activehdkey record 8bb7a8b06f
  127. wallet: Load activehdkey and its private keys 
    The activehdkey record will now be loaded into the wallet as its own
member variable. Since the private keys will always be in existing
descriptors in the wallet, the key for the active hd key is also pulled
out of those descriptors on loading.
    2e840bcfc0
  128. wallet: Store master key used for automatically generated descriptors 2e2545c3a2
  129. wallet: Retrieve active hd pubkey 4569cb1666
  130. key: Add constructor for CExtKey that takes CExtPubKey and CKey 
    We often need to construct a CExtKey given an CExtPubKey and CKey, so
implement a constructor that does that for us.
    116a4252d6
  131. wallet: Implement GetActiveHDPrivkey 77408c0bdb
  132. wallet, rpc: Add gethdkey RPC 9b760c1049
  133. descriptor: Be able to get the pubkeys involved in a descriptor 5800b75a54
  134. wallet: Always set WALLET_FLAG_GLOBAL_HD_KEY for new wallets 4a82d27872
  135. wallet, rpc: Check WALLET_FLAG_GLOBAL_HD_KEY in gethdkey bb02b9eb94
  136. wallet: Automatically upgrade a wallet to have global hd key ce5495a4b2
  137. tests: Test for gethdkey 9ac7fe7da8
  138. test: Test automatic upgrade of descriptor wallets to have hd key 5679b4a885
  139. wallet: Set global hd key for migrated wallets 18879984d1
  140. test: Also do backwards compat test with encrypted wallets 
    Best reviewed with `git show -w`
    af3667a13c
  141. walletdb: Check that unencrypted and crypted keys are mutually exclusive 1bbbb732b5
  142. test: Check that no unencrypted records persist after encrypting 291cb32237
  143. test: add coverage for re-opening a downgraded encrypted wallet on master 
    The test creates a wallet on master, downgrades and encrypts the wallet.
Then, it tries to open it again on master.
    059d847810
  144. ParseHDKeypath: support h as hardened marker c9f7127013
  145. wallet: GetExtKey() 20a522cf85
  146. rpc: ParsePathBIP32 helper 24d2240d92
  147. rpc: add path argument to gethdkey 259d52f964
  148. test: use getxpub in wallet_multisig_descriptor_psbt.py a4330bac48
  149. doc: use gethdkey in basic multisig descriptor example aebbcd4e77
  150. Sjors force-pushed on Dec 14, 2023
  151. Sjors commented at 10:09 am on December 22, 2023: member

    I plan to re-work this on top of #29130 with a similar interface to createwalletdescriptor. That is, if you have a regular wallet with normal descriptors, it will Just Work(tm). Otherwise you need to specify which master key to use.

    Initially I’ll make a separate PR. If #26728 is closed in favor of the new approach in #29130 then this PR can be closed too.

  152. ryanofsky commented at 3:01 pm on December 23, 2023: contributor

    Initially I’ll make a separate PR. If #26728 is closed in favor of the new approach in #29130 then this PR can be closed too.

    It seems like you could just rebase this on top of #29130, since the description of the PR and new commits aren’t going to change that much, they will just be based on #29130 instead of #26728. Either way seems fine, though.

  153. Sjors commented at 8:29 am on December 27, 2023: member
    @ryanofsky building on #29130 introduces a new argument to specify which master key you intend to use.
  154. DrahtBot added the label Needs rebase on Jan 8, 2024
  155. DrahtBot commented at 2:56 pm on January 8, 2024: contributor

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

  156. Sjors commented at 6:19 pm on January 12, 2024: member

    Given that #26728 was closed, I’ll just modify this PR to build on top of #29130.

    (bit busy with stratum v2 stuff, but it will happen)

  157. Sjors commented at 9:49 am on February 13, 2024: member

    This seems more involved than a simple rebase. #29130 changes gethdkey to gethdkeys making it a less obvious choice to add a path argument to. We probably need a new RPC that’s more similar to createwalletdescriptor in how it’s called.

    I probably won’t have time for this in the near future, so I’m closing this as up for grabs.

  158. Sjors closed this on Feb 13, 2024


Sjors lsilva01 DrahtBot jonatack Rspigler mjdietzx fjahr Zero-1729 JeremyRubin w0xlt aureleoules maflcko S3RK ryanofsky

Labels
Wallet RPC/REST/ZMQ Needs rebase

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: 2024-11-17 03:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me