wallet, spkm: Move key management from DescriptorScriptPubKeyMan to wallet level KeyManager #23417

pull achow101 wants to merge 21 commits into bitcoin:master from achow101:wallet-keyman changing 17 files +870 −295
  1. achow101 commented at 6:11 PM on November 2, 2021: member

    This PR changes DescriptorScriptPubKeyMan to no longer handle relevant keys directly. Instead all keys for all DescriptorSPKMs will be handled by a new KeyManager class which exists within CWallet (a reference to it is passed to each DescriptorSPKM). This allows us to have a concept of a wallet HD key for descriptor wallets. This makes it easier to add new single key descriptors that use the same HD master key as the rest of the autogenerated descriptors (e.g. for taproot). Multisigs will also be easier as an xpub belonging to the wallet can be exported without needing to do weird things like descriptor introspection and guessing about which descriptor's key to use.

    KeyManager is a class which handles all of the keys for DescriptorSPKMs. It contains the maps that hold the keys, deals with writing those keys to disk, and handles their encryption. Encryption keys are still managed by CWallet but provided to KeyManager through the WalletStorage interface. Signing is still done through DescriptorScriptPubKeyMan::SignTransaction however this will fetch the keys from KeyManager rather than storing keys in the DescriptorSPKM to be used.

    This change is backwards compatible. Although KeyManager writes and uses keys in new keyman_key and keyman_ckey records, it will still write keys for each descriptor in walletdescriptorkey and walletdescriptorckey records. This allows a descriptor wallet created using this change to be opened by 22.0 and 0.21. Additionally, wallets created with older software will automatically be upgraded to using the KeyManager at first loading. This is done in the background and does not require any user interaction (i.e. no passphrase required).

  2. Sjors commented at 7:47 PM on November 2, 2021: member

    Concept ACK. This will make #22341 a lot easier.

  3. DrahtBot added the label Build system on Nov 2, 2021
  4. DrahtBot added the label Descriptors on Nov 2, 2021
  5. DrahtBot added the label RPC/REST/ZMQ on Nov 2, 2021
  6. DrahtBot added the label Wallet on Nov 2, 2021
  7. DrahtBot commented at 12:08 AM on November 3, 2021: 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:

    • #26627 (wallet: Migrate non-HD keys with single combo containing a list of keys by achow101)
    • #26596 (wallet: Migrate legacy wallets to descriptor wallets without requiring BDB by achow101)
    • #26008 (wallet: cache IsMine scriptPubKeys to improve performance of wallets with a lot of non-ranged descriptors by achow101)
    • #25991 (Wallet: Add foreign_outputs metadata to support CoinJoin transactions by luke-jr)
    • #25907 (wallet: rpc to add automatically generated descriptors by achow101)
    • #25766 (wallet: Include a signature with encrypted keys to mitigate a wallet scam by achow101)
    • #24914 (wallet: Load database records in a particular order by achow101)
    • #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.

  8. in src/pubkey.h:307 in 1399c27ec5 outdated 
     303 | @@ -304,6 +304,7 @@ struct CExtPubKey {
 304 |      {
 305 |          return !(a == b);
 306 |      }
 307 | +    bool operator<(const CExtPubKey& other) const { return pubkey < other.pubkey; }
    Sjors commented at 2:51 PM on November 5, 2021:

    1399c27ec5081af7a1f31a9bb750b216cc68c6cf: nit, maybe order by vchFingerprint < other.vchFingerprint || pubkey < other.pubkey, in case we ever want to use this to sort by master key.

    achow101 commented at 7:59 PM on November 9, 2021:

    Done

  9. in src/wallet/walletdb.h:58 in 2ed25fe1a5 outdated 
      54 | @@ -55,6 +55,7 @@ namespace DBKeys {
  55 |  extern const std::string ACENTRY;
  56 |  extern const std::string ACTIVEEXTERNALSPK;
  57 |  extern const std::string ACTIVEINTERNALSPK;
  58 | +extern const std::string ACTIVEHDKEY;
    Sjors commented at 3:00 PM on November 5, 2021:

    2ed25fe1a512d50065c6e27b23016500f3e6d647

    // The active HD master key, identified by its extended public key
    achow101 commented at 7:59 PM on November 9, 2021:

    Done

  10. in src/wallet/walletdb.h:67 in 2ed25fe1a5 outdated 
      63 | @@ -63,6 +64,7 @@ extern const std::string DEFAULTKEY;
  64 |  extern const std::string DESTDATA;
  65 |  extern const std::string FLAGS;
  66 |  extern const std::string HDCHAIN;
  67 | +extern const std::string HDPUBKEY;
    Sjors commented at 3:01 PM on November 5, 2021:

    2ed25fe1a512d50065c6e27b23016500f3e6d647

    // An HD master key, identified by its extended public key
    achow101 commented at 7:59 PM on November 9, 2021:

    Done

  11. in src/wallet/walletdb.cpp:1094 in 1c76661cc7 outdated 
    1090 | +    // Find the keys which are used in single key internal and external descriptors with
1091 | +    // the pre-taproot output types
1092 | +    if (pwallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS) &&
1093 | +        !pwallet->IsWalletFlagSet(WALLET_FLAG_USES_KEYMAN) &&
1094 | +        !pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)
1095 | +        && last_client <= 220000
    Sjors commented at 4:01 PM on November 5, 2021:

    1c76661cc73a32cfc5a6185aec1aa954dbad3dd6: assuming we don't back port this, shouldn't it be last_client <= 229999. Otherwise we wouldn't upgrade wallets touched by (hypothetical future release) v22.1

    achow101 commented at 8:00 PM on November 9, 2021:

    Done, also changed this to be either the flag is not set, or the last client is <= 229999. This is to handle the upgrade then downgrade and then upgrade again case.

  12. in src/wallet/walletdb.cpp:1099 in 1c76661cc7 outdated 
    1095 | +        && last_client <= 220000
1096 | +        ) {
1097 | +        std::map<CExtPubKey, std::pair<std::map<std::string, int>, uint64_t>> descs_keys;
1098 | +        std::map<std::string, int> tmpl = {{"pkh(", 0}, {"sh(wpkh(", 0}, {"wpkh(", 0}};
1099 | +
1100 | +        // Find xpubs used in pkh(), sh(wpkh()), and wpkh() descriptors
    Sjors commented at 4:06 PM on November 5, 2021:

    1c76661: maybe clarify that we're dealing with the xpub at the root level, not the BIP44/49/84 "account" level.

    achow101 commented at 8:00 PM on November 9, 2021:

    Done

  13. Sjors commented at 4:57 PM on November 5, 2021: member

    I rebased #22341 on top of this.

    Some initial review remarks:

    <details><summary>wallet_transactiontime_rescan.py fails consistently for me</summary>

    2021-11-05T14:52:37.129000Z TestFramework (INFO): Restore user wallet on another node without rescan
2021-11-05T14:52:37.154000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/Users/sjors/dev/bitcoin/test/functional/test_framework/test_framework.py", line 132, in main
    self.run_test()
  File "test/functional/wallet_transactiontime_rescan.py", line 134, in run_test
    assert_equal(restorewo_wallet.getbalance(), 0)
  File "/Users/sjors/dev/bitcoin/test/functional/test_framework/util.py", line 50, in assert_equal
    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(1.00000000 == 0)

    </details>

    <details> <summary>Some thread safety warnings.</summary>

    wallet/keyman.cpp:241:5: warning: calling function 'AssertLockHeldInternal<AnnotatedMixin<std::recursive_mutex>>' requires holding mutex 'cs_keyman' exclusively [-Wthread-safety-analysis]
    AssertLockHeld(cs_keyman);
    ^
./sync.h:83:28: note: expanded from macro 'AssertLockHeld'
#define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, &cs)
                           ^
wallet/keyman.cpp:244:41: warning: reading variable 'm_map_crypted_keys' requires holding mutex 'cs_keyman' [-Wthread-safety-analysis]
        for (const auto& [id, key_pair] : m_map_crypted_keys) {
                                        ^
wallet/keyman.cpp:244:41: warning: reading variable 'm_map_crypted_keys' requires holding mutex 'cs_keyman' [-Wthread-safety-analysis]
wallet/keyman.cpp:253:12: warning: reading variable 'm_map_keys' requires holding mutex 'cs_keyman' [-Wthread-safety-analysis]
    return m_map_keys;
           ^
wallet/keyman.cpp:264:9: warning: reading variable 'm_map_crypted_keys' requires holding mutex 'cs_keyman' [-Wthread-safety-analysis]
    if (m_map_crypted_keys.count(id) == 0) {
        ^
wallet/keyman.cpp:267:12: warning: reading variable 'm_map_crypted_keys' requires holding mutex 'cs_keyman' [-Wthread-safety-analysis]
    return m_map_crypted_keys.at(id);
           ^
6 warnings generated.

...

wallet/walletdb.cpp:813:38: warning: calling function 'LoadActiveHDKey' requires holding mutex 'pwallet->GetKeyManager().cs_keyman' exclusively [-Wthread-safety-analysis]
            pwallet->GetKeyManager().LoadActiveHDKey(extpub);
                                     ^
wallet/walletdb.cpp:1150:38: warning: calling function 'SetActiveHDKey' requires holding mutex 'pwallet->GetKeyManager().cs_keyman' exclusively [-Wthread-safety-analysis]
            pwallet->GetKeyManager().SetActiveHDKey(best_xpub);

    </details>

    Shameless plug: it would be useful is #19013 was merged first, so this PR can include more wallet backward and forward compatibility tests.

  14. DrahtBot added the label Needs rebase on Nov 8, 2021
  15. achow101 force-pushed on Nov 9, 2021
  16. achow101 commented at 8:01 PM on November 9, 2021: member

    Thread safety warnings are fixed, not seeing the wallet_transactiontime_rescan.py failure.

  17. DrahtBot removed the label Needs rebase on Nov 9, 2021
  18. achow101 force-pushed on Nov 9, 2021
  19. achow101 force-pushed on Nov 9, 2021
  20. achow101 force-pushed on Nov 10, 2021
  21. in src/pubkey.h:307 in 7fc1afc000 outdated 
     303 | @@ -304,6 +304,7 @@ struct CExtPubKey {
 304 |      {
 305 |          return !(a == b);
 306 |      }
 307 | +    bool operator<(const CExtPubKey& other) const { return vchFingerprint < other.vchFingerprint || pubkey < other.pubkey; }
    Sjors commented at 3:45 PM on November 11, 2021:

    Oops, my example wasn't very smart, because this is comparing pointers... unsigned char vchFingerprint[4]. The memcmp approach in == above is probably more useful. And we should probably be consistent and compare all the things.

    achow101 commented at 8:25 PM on November 12, 2021:

    Done

  22. in src/script/descriptor.h:155 in 7fc1afc000 outdated 
     145 | @@ -146,6 +146,8 @@ struct Descriptor {
 146 |  
 147 |      /** @return The OutputType of the scriptPubKey(s) produced by this descriptor. Or nullopt if indeterminate (multiple or none) */
 148 |      virtual std::optional<OutputType> GetOutputType() const = 0;
 149 | +
 150 | +    virtual void GetPubkeys(std::set<CPubKey>& pubkeys, std::set<CExtPubKey>& ext_pubs) const = 0;
    Sjors commented at 3:51 PM on November 11, 2021: 
     /** Return all (extended) public  keys for this descriptor, including any from any subdescriptors.
     *
     * [@param](/bitcoin-bitcoin/contributor/param/)[out] pubkeys Any public keys.
     * [@param](/bitcoin-bitcoin/contributor/param/)[out] pubkeys Any extended public keys.
     */

    This recursive function always ends up at GetRootPubkeys; any reason why Root is not present in this function name?

    achow101 commented at 10:05 PM on November 11, 2021:

    Not particularly. I believe originally I wanted the underlying function in PubkeyProvider to be GetPubkey but that was already taken. The Root part came from the fact that GetPubkey in BIP32PubkeyProvider returns a derived key whereas I needed the root extended key.

    achow101 commented at 8:25 PM on November 12, 2021:

    Added the comment

  23. in src/script/descriptor.cpp:208 in 7fc1afc000 outdated 
     184 | @@ -185,6 +185,8 @@ struct PubkeyProvider
 185 |  
 186 |      /** Derive a private key, if private data is available in arg. */
 187 |      virtual bool GetPrivKey(int pos, const SigningProvider& arg, CKey& key) const = 0;
 188 | +
 189 | +    virtual void GetRootPubkey(std::set<CPubKey>& pubkeys, std::set<CExtPubKey>& ext_pubs) const = 0;
    Sjors commented at 4:00 PM on November 11, 2021: 
     /** Return all (extended) public  keys for this descriptor
     *
     * [@param](/bitcoin-bitcoin/contributor/param/)[out] pubkeys Any public keys.
     * [@param](/bitcoin-bitcoin/contributor/param/)[out] pubkeys Any extended public keys.
     */
    achow101 commented at 8:25 PM on November 12, 2021:

    Done

  24. in src/wallet/walletdb.h:71 in 7fc1afc000 outdated 
      63 | @@ -63,8 +64,11 @@ extern const std::string DEFAULTKEY;
  64 |  extern const std::string DESTDATA;
  65 |  extern const std::string FLAGS;
  66 |  extern const std::string HDCHAIN;
  67 | +extern const std::string HDPUBKEY; // A HD key, identified by extended pubkey
    Sjors commented at 4:19 PM on November 11, 2021:

    Let's clarify here, or at the documentation for GetActiveHDKey, that the corresponding extended private key is reconstructed using this extended public key, which includes the chain code, and the right KEYMAN_KEY or KEYMAN_CKEY private key.

    Still this process seems rather complicated, why not just store the (encrypted) extended private key?

    achow101 commented at 10:37 PM on November 11, 2021:

    A lot of places in the codebase do not expect CExtKeys when fetching private keys, even in places where BIP 32 derivation ends up being done. They instead take CKeys and combine them with CExtPubKeys to get the necessary CExtKeys. We maintain this same paradigm for ease of implementation.

    Additionally, having all private keys be universally CKeys makes it easier to support non-ranged descriptors. Instead of having to store and fetch different data types depending on whether the descriptor is ranged, and then converting them into the same data type for expansion and signing, we can use the same datatype throughout. When we do need the extended key, it can be reconstructed, but that (currently) happens rarely.

    Sjors commented at 11:14 AM on November 12, 2021:

    Storing both CExtKey and Ckey (for non-ranged descriptors) could be an approach, but I'm also not sure if that makes the implementation any easier to understand. This is probably the only opportunity to break with the past convention, if we want to.

  25. Sjors commented at 6:11 PM on November 11, 2021: member

    Some more comments after light-weight code review...

  26. achow101 force-pushed on Nov 12, 2021
  27. achow101 force-pushed on Nov 13, 2021
  28. Sjors commented at 12:34 PM on November 16, 2021: member

    Not seeing any thread warning anymore, so that's good.

    wallet_transactiontime_rescan.py only fails for me with configuring --without-bdb.

  29. in src/wallet/keyman.h:48 in 709a917c26 outdated 
      43 | +
  44 | +    void GenerateAndSetHDKey();
  45 | +    std::optional<CExtKey> GetActiveHDKey() const EXCLUSIVE_LOCKS_REQUIRED(cs_keyman);
  46 | +    std::optional<CExtPubKey> GetActiveHDPubKey() const EXCLUSIVE_LOCKS_REQUIRED(cs_keyman);
  47 | +    void SetActiveHDKey(const CExtPubKey& extpub);
  48 | +    void LoadActiveHDKey(const CExtPubKey& extpub);
    Sjors commented at 1:47 PM on November 16, 2021:

    It would be nice to group the wallet loading and unloading functions.

    Similarly let's separate functions that modify wallet storage from those that don't.

    achow101 commented at 7:41 PM on November 17, 2021:

    Done

  30. achow101 commented at 5:42 PM on November 16, 2021: member

    wallet_transactiontime_rescan.py only fails for me with configuring --without-bdb.

    This appears to fail on master as well.

  31. in src/wallet/keyman.cpp:249 in 709a917c26 outdated 
     240 | +}
 241 | +
 242 | +std::map<CKeyID, CKey> KeyManager::GetKeys() const
 243 | +{
 244 | +    AssertLockHeld(cs_keyman);
 245 | +    if (m_storage.HasEncryptionKeys() && !m_storage.IsLocked()) {
    Sjors commented at 1:51 PM on November 17, 2021:

    (partly existing code) If an encrypted wallet is locked, rather than fail, this function just returns the m_map_keys which happens to be empty. Not sure how I feel about that, but let's at least document (that calling this on a locked wallet returns an empty result).

    achow101 commented at 7:41 PM on November 17, 2021:

    Added a comment.

  32. in src/wallet/keyman.cpp:237 in 709a917c26 outdated 
     228 | +    {
 229 | +        CPubKey pubkey = key.GetPubKey();
 230 | +        CKeyingMaterial secret(key.begin(), key.end());
 231 | +        std::vector<unsigned char> crypted_secret;
 232 | +        if (!EncryptSecret(master_key, secret, pubkey.GetHash(), crypted_secret)) {
 233 | +            return false;
    Sjors commented at 2:04 PM on November 17, 2021:

    Clear m_map_crypted_keys first?

    achow101 commented at 7:40 PM on November 17, 2021:

    I think it's fine to not do this. The caller will assert(false) in that case anyways.

  33. in src/wallet/keyman.cpp:200 in 709a917c26 outdated 
     195 | +        if (!DecryptKey(master_key, crypted_secret, pubkey, key)) {
 196 | +            keyFail = true;
 197 | +            break;
 198 | +        }
 199 | +        keyPass = true;
 200 | +        if (m_decryption_thoroughly_checked)
    Sjors commented at 2:09 PM on November 17, 2021:

    Nit: move break inline or add brackets.

    achow101 commented at 7:41 PM on November 17, 2021:

    Done

  34. Sjors approved
  35. Sjors commented at 2:58 PM on November 17, 2021: member

    utACK 709a917c26bf048de454760c7189278b965ca7f3 modulo AddDescriptorKeyWithDB definition

    I found it easier to review the PR as a whole rather the migration through individual commits: git diff HEAD~21 --color-moved=dimmed_zebra. This is because some of the function bodies aren't moved in single commits.

    Looks like you got rid of AddDescriptorKeyWithDB, but it's still defined in header. Also the original function contained an assert for not having WALLET_FLAG_DISABLE_PRIVATE_KEYS and it would check if the key already existed (AddKeyInner does that now).

    (most comments below are about existing code)

  36. achow101 force-pushed on Nov 17, 2021
  37. achow101 commented at 7:42 PM on November 17, 2021: member

    Looks like you got rid of AddDescriptorKeyWithDB, but it's still defined in header

    Fixed

    Also the original function contained an assert for not having WALLET_FLAG_DISABLE_PRIVATE_KEYS and it would check if the key already existed (AddKeyInner does that now).

    Added that back in.

  38. DrahtBot added the label Needs rebase on Nov 22, 2021
  39. achow101 force-pushed on Nov 22, 2021
  40. DrahtBot removed the label Needs rebase on Nov 22, 2021
  41. DrahtBot added the label Needs rebase on Dec 8, 2021
  42. achow101 force-pushed on Dec 8, 2021
  43. DrahtBot removed the label Needs rebase on Dec 8, 2021
  44. Sjors commented at 6:22 AM on December 10, 2021: member

    ~re-utACK b1d76a8d91a070668332f06608a20a9d937ba1a4~

    (the two spurious CI failures should go away after the next rebase)

    Update: apparently one of them was hiding a non-spurious failure

  45. maflcko commented at 8:35 AM on December 13, 2021: member

    /usr/include/c++/8/bits/stl_function.h:386:20: error: ambiguous overload for ‘operator<’ (operand types are ‘const CExtPubKey’ and ‘const CExtPubKey’)

  46. achow101 force-pushed on Dec 13, 2021
  47. Sjors commented at 8:35 AM on December 30, 2021: member

    re-utACK 49234d0dbc11f91e9ae657602c97b4a0dc0ec75e (just a rebase, but with 2edf8b89b4 dropped entirely)

  48. DrahtBot added the label Needs rebase on Jan 11, 2022
  49. achow101 force-pushed on Jan 11, 2022
  50. DrahtBot removed the label Needs rebase on Jan 11, 2022
  51. Sjors commented at 3:56 PM on January 12, 2022: member

    re-utACK 01a4860 (after #23497 namespace stuff)

  52. maflcko removed the label Build system on Jan 28, 2022
  53. maflcko removed the label RPC/REST/ZMQ on Jan 28, 2022
  54. DrahtBot added the label Needs rebase on Apr 26, 2022
  55. achow101 force-pushed on Apr 26, 2022
  56. DrahtBot removed the label Needs rebase on Apr 26, 2022
  57. Sjors commented at 12:35 PM on April 29, 2022: member

    re-utACK 3992d06c15af5242d955fd7f74e9e089bbb8a166

  58. DrahtBot added the label Needs rebase on Aug 5, 2022
  59. achow101 force-pushed on Aug 5, 2022
  60. DrahtBot removed the label Needs rebase on Aug 6, 2022
  61. Sjors commented at 9:34 AM on August 6, 2022: member

    CI is not happy :-(

  62. achow101 force-pushed on Aug 10, 2022
  63. Sjors commented at 8:51 AM on August 10, 2022: member

    re-utACK 8133ce4da3fbbcbecab4346cbe3cc825f4467ce7

  64. DrahtBot added the label Needs rebase on Aug 17, 2022
  65. achow101 force-pushed on Aug 17, 2022
  66. DrahtBot removed the label Needs rebase on Aug 17, 2022
  67. Sjors commented at 11:24 AM on August 17, 2022: member

    Mmm, CI is not running bf104948b42f63d60afee52ae95a8515432eeb1c.

    Looks like #25734 triggered the (trivial) rebase (see ffcdac846b)?

    Building throws a warning, because #25642 marked it as such.

    wallet/keyman.cpp:96:9: warning: ignoring return value of function declared with 'nodiscard' attribute [-Wunused-result]
        ext_key->Derive(*ext_key, i);

    Tested that #22341 still works on top of this.

  68. achow101 commented at 3:11 PM on August 17, 2022: member

    Building throws a warning, because #25642 marked it as such.

    wallet/keyman.cpp:96:9: warning: ignoring return value of function declared with 'nodiscard' attribute [-Wunused-result]
        ext_key->Derive(*ext_key, i);

    Not seeing this error. Is this for a particular commit?

  69. achow101 force-pushed on Aug 17, 2022
  70. Sjors commented at 11:44 AM on August 18, 2022: member

    Is this for a particular commit?

    Oops, in my own code :-)

    re-utACK e310760da891c217d2f7805978b5eca337f5cdc8

  71. DrahtBot added the label Needs rebase on Aug 19, 2022
  72. achow101 force-pushed on Aug 19, 2022
  73. DrahtBot removed the label Needs rebase on Aug 19, 2022
  74. in src/wallet/walletdb.cpp:346 in aa91428a1a outdated 
     341 | +
 342 | +bool WalletBatch::WriteKeyManKey(const CPubKey& pubkey, const CPrivKey& privkey)
 343 | +{
 344 | +    // hash pubkey/privkey to accelerate wallet load
 345 | +    std::vector<unsigned char> key;
 346 | +    key.reserve(pubkey.size() + pubkey.size());
    w0xlt commented at 10:05 PM on August 19, 2022:

    Wouldn't it be privkey.size ?

        key.reserve(privkey.size() + pubkey.size());
    achow101 commented at 12:56 AM on August 20, 2022:

    Done

  75. in src/wallet/keyman.h:41 in aa91428a1a outdated 
      36 | +
  37 | +    bool AddKeyInner(WalletBatch& batch, const CKey& key, const CPubKey& pubkey) EXCLUSIVE_LOCKS_REQUIRED(cs_keyman);
  38 | +    std::vector<unsigned char> AddCryptedKeyInner(WalletBatch& batch, const CKey& key, const CPubKey& pubkey) EXCLUSIVE_LOCKS_REQUIRED(cs_keyman);
  39 | +
  40 | +public:
  41 | +    mutable RecursiveMutex cs_keyman;
    w0xlt commented at 10:40 PM on August 19, 2022:

    Maybe change to Mutex ?

        mutable Mutex m_keyman_mutex;
    achow101 commented at 12:56 AM on August 20, 2022:

    It ends up being used recursively.

  76. in src/wallet/walletdb.cpp:1112 in aa91428a1a outdated 
    1113 | +            if (descs_keys.count(xpub) == 0) {
1114 | +                descs_keys.emplace(xpub, std::make_pair(tmpl, 0));
1115 | +            }
1116 | +
1117 | +            const std::string desc = w_desc.descriptor->ToString();
1118 | +            if (desc.find("pkh(") == 0) {
    w0xlt commented at 11:04 PM on August 19, 2022:

    nit:

                if (desc.find("pkh(")) {
    achow101 commented at 12:57 AM on August 20, 2022:

    Done

    Sjors commented at 2:47 PM on August 22, 2022:

    Are you sure? find returns the position, so this should match wpkh(…) but not pkh(). The else if's seem wrong for the same reason.

    (we should probably never cast find() to a bool, because it's very easy to get confused)

    w0xlt commented at 3:00 PM on August 22, 2022:

    Maybe desc.rfind("pkh(", 0) == 0 ?

    Sjors commented at 3:09 PM on August 22, 2022:

    I think the original desc.find("pkh(") == 0 was fine.

    achow101 commented at 4:46 PM on August 22, 2022:

    Ah, yes. Looked at the docs this time. I've changed these all to be == 0.

  77. in src/wallet/rpc/addresses.cpp:790 in aa91428a1a outdated 
     785 | @@ -786,4 +786,48 @@ RPCHelpMan walletdisplayaddress()
 786 |      };
 787 |  }
 788 |  #endif // ENABLE_EXTERNAL_SIGNER
 789 | +
 790 | +RPCHelpMan getxpub()
    w0xlt commented at 11:19 PM on August 19, 2022:

    Maybe getxupub and listdescriptors can be combined into a single command?

    achow101 commented at 12:58 AM on August 20, 2022:

    I think it's useful to have them be separate. Fundamentally, they deal with different data too. Descriptors are about scripts, whereas this is about the keys. So it doesn't really make sense that listdescriptors would provide a key not in a descriptor.

  78. w0xlt approved
  80. achow101 force-pushed on Aug 20, 2022
  81. achow101 force-pushed on Aug 20, 2022
  82. w0xlt approved
  84. Sjors commented at 2:50 PM on August 22, 2022: member

    Rebase to e8926b1 looks correct, but see find() issue above.

  85. achow101 force-pushed on Aug 22, 2022
  86. Sjors commented at 6:57 PM on August 22, 2022: member

    re-utACK 5323e56ffb8762d5d99bb89d1623993ad6db5849

  87. achow101 force-pushed on Aug 22, 2022
  88. achow101 commented at 7:58 PM on August 22, 2022: member

    Pushed two small changes. The first is to set WALLET_USES_KEYMAN for all newly created wallets to avoid any possibility of attempting to upgrade a wallet when it shouldn't. The second is to bump the last client version check because this isn't going to make it to 24.0.

  89. achow101 force-pushed on Aug 22, 2022
  90. achow101 force-pushed on Aug 22, 2022
  91. achow101 force-pushed on Aug 22, 2022
  92. achow101 commented at 10:56 PM on August 22, 2022: member

    On second thought, I've removed the last client check. It doesn't quite make sense to have that.

  93. w0xlt approved
  95. DrahtBot added the label Needs rebase on Sep 1, 2022
  96. achow101 force-pushed on Sep 1, 2022
  97. DrahtBot removed the label Needs rebase on Sep 1, 2022
  98. in src/wallet/wallet.cpp:2858 in eb75f8d58e outdated 
    2838 | @@ -2839,6 +2839,9 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
2839 |  
2840 |          walletInstance->InitWalletFlags(wallet_creation_flags);
2841 |  
2842 | +        // Set USES_KEYMAN to prevent auto upgrade from trying to auto upgrade (previously) blank wallets that had imported things
2843 | +        walletInstance->SetWalletFlag(WALLET_FLAG_USES_KEYMAN);
    Sjors commented at 11:35 AM on September 2, 2022:

    eb75f8d58ec335e013178d5b94d824cc7739139a: maybe put this under the if (!walletInstance->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) check below?

  99. Sjors approved
  100. Sjors commented at 11:37 AM on September 2, 2022: member

    re-utACK 6bdd8a6759c9fc93cd19612bfbd39c1abcc4de00

  101. achow101 force-pushed on Sep 20, 2022
  102. Sjors commented at 9:57 AM on September 22, 2022: member

    re-utACK 8c5ddf432dceb084be6612b378dacf9211e8cf08

  103. in src/wallet/keyman.cpp:267 in 8c5ddf432d outdated 
     262 | +}
 263 | +
 264 | +bool KeyManager::HavePrivateKeys() const
 265 | +{
 266 | +    LOCK(cs_keyman);
 267 | +    return m_map_keys.size() > 0 || m_map_crypted_keys.size() > 0;
    aureleoules commented at 1:52 PM on September 22, 2022:

    prefer empty()

    achow101 commented at 5:11 PM on September 22, 2022:

    Done

  104. in src/wallet/keyman.cpp:270 in 8c5ddf432d outdated 
     265 | +{
 266 | +    LOCK(cs_keyman);
 267 | +    return m_map_keys.size() > 0 || m_map_crypted_keys.size() > 0;
 268 | +}
 269 | +
 270 | +const std::optional<std::pair<CPubKey, std::vector<unsigned char>>> KeyManager::GetCryptedKey(const CKeyID& id) const
    aureleoules commented at 1:53 PM on September 22, 2022:

    const is redundant because std::optional is a value

    achow101 commented at 5:11 PM on September 22, 2022:

    Done

  105. in src/wallet/wallet.cpp:3440 in 8c5ddf432d outdated 
    3436 | @@ -3434,53 +3437,41 @@ void CWallet::ConnectScriptPubKeyManNotifiers()
3437 |  void CWallet::LoadDescriptorScriptPubKeyMan(uint256 id, WalletDescriptor& desc)
3438 |  {
3439 |      if (IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) {
3440 | -        auto spk_manager = std::unique_ptr<ScriptPubKeyMan>(new ExternalSignerScriptPubKeyMan(*this, desc));
3441 | +        auto spk_manager = std::unique_ptr<ScriptPubKeyMan>(new ExternalSignerScriptPubKeyMan(*this, desc, m_keyman));
    aureleoules commented at 1:59 PM on September 22, 2022: 
            auto spk_manager = std::make_unique<ExternalSignerScriptPubKeyMan>(*this, desc, m_keyman);

    make_unique is less verbose, same below

    achow101 commented at 5:11 PM on September 22, 2022:

    Done

  106. in src/wallet/keyman.cpp:255 in 8c5ddf432d outdated 
     250 | +        std::map<CKeyID, CKey> keys;
 251 | +        for (const auto& [id, key_pair] : m_map_crypted_keys) {
 252 | +            const auto& [pubkey, crypted_secret] = key_pair;
 253 | +            CKey key;
 254 | +            bool ok = DecryptKey(m_storage.GetEncryptionKey(), crypted_secret, pubkey, key);
 255 | +            assert(ok);
    aureleoules commented at 2:16 PM on September 22, 2022:

    can inline this assert

                bool ok = Assert(DecryptKey(m_storage.GetEncryptionKey(), crypted_secret, pubkey, key));
    achow101 commented at 4:46 PM on September 22, 2022:

    Assertions should not have side effects, hence the separation here.

    aureleoules commented at 5:02 PM on September 22, 2022:

    this is true for assert but Assert is a macro defined in util/check.h that always executes and returns the statement, but doesn't always verify the assertion https://github.com/bitcoin/bitcoin/blob/100949af0e2551f22c02a73355f2c64710b68ef1/src/util/check.h#L57-L71

    achow101 commented at 5:49 PM on September 22, 2022:

    Even so, it is more confusing to read be inlining as assertions are expected to be checking invariants rather than doing something.

    aureleoules commented at 5:56 PM on September 22, 2022:

    Well the Assert macro has been used extensively already in the codebase so I don't think it's confusing anymore. But this is a small nit so if you prefer it this way it's fine.

    Sjors commented at 9:19 AM on September 23, 2022:

    I also prefer to keep them on separate lines, for easier review since I've gotten used to that convention.

  107. in src/wallet/scriptpubkeyman.cpp:2043 in 8c5ddf432d outdated 
    2076 | -        }
2077 | -        m_map_crypted_keys[pubkey.GetID()] = make_pair(pubkey, crypted_secret);
2078 | -        batch->WriteCryptedDescriptorKey(GetID(), pubkey, crypted_secret);
2079 | +    for (const CKeyID& id : m_set_stored_keys) {
2080 | +        const auto& ckey_pair = m_keyman.GetCryptedKey(id);
2081 | +        assert(ckey_pair != std::nullopt);
    aureleoules commented at 2:16 PM on September 22, 2022: 
            const auto& ckey_pair = Assert(m_keyman.GetCryptedKey(id));
    achow101 commented at 5:12 PM on September 22, 2022:

    Assertions shouldn't have side effects, hence the separation.

  108. aureleoules commented at 2:54 PM on September 22, 2022: member

    Left some code review nits, otherwise looks good to me. ACK 8c5ddf432dceb084be6612b378dacf9211e8cf08.

    Though, shouldn't 8c5ddf432dceb084be6612b378dacf9211e8cf08 go in #22341?

  109. achow101 force-pushed on Sep 22, 2022
  110. in src/wallet/keyman.cpp:267 in d750754d5b outdated 
     262 | +}
 263 | +
 264 | +bool KeyManager::HavePrivateKeys() const
 265 | +{
 266 | +    LOCK(cs_keyman);
 267 | +    return m_map_keys.empty() || m_map_crypted_keys.empty();
    aureleoules commented at 5:39 PM on September 22, 2022:

    i think you meant

        return !m_map_keys.empty() || !m_map_crypted_keys.empty();
    aureleoules commented at 5:43 PM on September 22, 2022:

    also, maybe HasPrivateKeys is better than HavePrivateKeys ?

    achow101 commented at 5:50 PM on September 22, 2022:

    Oops, yes.

    also, maybe HasPrivateKeys is better than HavePrivateKeys ?

    I prefer to keep it the same for consistent naming.

    Sjors commented at 9:21 AM on September 23, 2022:

    Yikes. Can you add a test for this?

  111. achow101 force-pushed on Sep 22, 2022
  112. aureleoules approved
  113. aureleoules commented at 1:39 PM on October 4, 2022: member

    re-ACK 5145d1610202356676e5a031c68d4af0b16a62d9 - minor changes since last review

  114. in src/wallet/walletdb.cpp:868 in 5145d16102 outdated 
     864 | +            if (!key.Load(pkey, pubkey, true))
 865 | +            {
 866 | +                strErr = "Error reading wallet database: CPrivKey corrupt";
 867 | +                return false;
 868 | +            }
 869 | +            pwallet->GetKeyManager().LoadKey(pubkey.GetID(), key);
    aureleoules commented at 2:22 PM on October 4, 2022:

    this seems to be duplicated code from the else if (strType == DBKeys::WALLETDESCRIPTORKEY) case above, is there a way to re-use the code?

    achow101 commented at 7:02 PM on October 10, 2022:

    Done

  115. achow101 force-pushed on Oct 10, 2022
  116. achow101 force-pushed on Oct 10, 2022
  117. aureleoules approved
  118. aureleoules commented at 4:41 PM on October 11, 2022: member

    re-ACK bdc59fcf8a5ef473461c43c2a035accd4f89e35d

  119. DrahtBot added the label Needs rebase on Oct 13, 2022
  120. moveonly: move WalletStorage to separate file 18054704cf
  121. walletdb: Add HDKey records 312245433d
  122. walletdb: Add WriteKeyManKey and WriteCryptedKeyManKey
    These functions write new key records for keys handled by a KeyManager
    3fa66d009a
  123. walletdb: Allow duplicate descriptor keys
    If a descriptor (crypted) key is being written and one already exists,
make sure that the one being written and the one already on disk
match each other.
    632b29608f
  124. descspkm: Track CKeyIDs of our keys
    When DescriptorScriptPubKeyMan no longer manages its keys, it still
needs to know the IDs of its keys.
    ebec893ce1
  125. wallet: Add KeyManager class db6efef098
  126. descspkm: Add KeyManager to DescriptorScriptPubKeyMan and use for keys 60a499b5dd
  127. descspkm: Encrypt with KeyManager instead of direct map access 1e99ebda77
  128. descspkm: Use KeyManager::LoadKey and LoadCryptedKey when loading 9b44055c4f
  129. descspkm: Replace GetKeys with KeyManager::GetKeys d04dd5becf
  130. descspkm: Replace HavePrivateKeys with KeyManager::HavePrivateKeys() d4c78ace5a
  131. keyman: Make some members private 38e099bd2b
  132. wallet: Have KeyManager in CWallet rather than DescriptorScriptPubKeyMan fc89a37d81
  133. walletdb: Refactor deserialaization of keys with checksums
    This will become shared later.
    b21982f390
  134. walletdb: Load keys into KeyManager directly 8436d2ff55
  135. wallet: Add flag for using KeyManager
    KeyManager will be a backwards compatible background upgrade to
descriptor wallets. A flag indicating that the upgrade has occurred is
added so that the upgrade (not yet implemented) will only happen once.
    6e5fbd56a4
  136. wallet: Use KeyManager to generate master key b0adf464c2
  137. descriptor: Be able to get the pubkeys involved in a descriptor 4ca53084e7
  138. walletdb: Implement upgrading a wallet to use KeyManager f7fbd0a5fd
  139. descspkm: Remove unneeded key loading
    Key management will be done entirely by KeyManager, so
DescriptorScriptPubKeyMan does not need key loading functions.
    aee80e5a0d
  140. rpc: Add getxpub command c9af030cd6
  141. achow101 force-pushed on Oct 17, 2022
  142. aureleoules approved
  143. aureleoules commented at 3:25 PM on October 17, 2022: member

    reACK c9af030cd60f1fe4e66ccba585b616c1dcc11a50 - minor rebase, added KeyManager m_keyman to CWallet

  144. DrahtBot removed the label Needs rebase on Oct 17, 2022
  145. Sjors commented at 1:23 PM on October 27, 2022: member

    re-utACK c9af030cd60f1fe4e66ccba585b616c1dcc11a50

  146. S3RK commented at 8:10 AM on November 9, 2022: contributor

    Concept ACK. Started code review

  147. achow101 commented at 10:35 PM on December 19, 2022: member

    I had a discussion with @S3RK about this PR last week, and we concluded that this might not be the best approach to tackle the issue that it targets. While it is a complete solution that would probably work well if descriptor wallets had been implemented this way in the first place, it seems like it doesn't work well with having to deal with backwards compatibility and various combinations of upgrading and downgrading that may occur.

    Since the goal of this PR is to enable key rotation (via re-enabling sethdseed) and the addition of new automatically generated descriptors, this implementation is probably overkill in addition to the backwards compatibility issues that it introduces.

    I've opened #26728 which implements a much simpler solution of just having CWallet store the master key and any ones that get rotated out. It still enables the things that we want.

  148. achow101 closed this on Dec 19, 2022
  149. bitcoin locked this on Dec 19, 2023
Contributors
achow101SjorsDrahtBotmaflckow0xltaureleoulesS3RK
Labels
WalletDescriptors
Linked (view graph)
#24193 Add taproot descriptor to existing descriptor wallet#26728 wallet: Have the wallet store the key for automatically generated descriptors
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-01 09:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me