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. DrahtBot cross-referenced this on Nov 3, 2021 from issue refactor: Take Span in SetSeed by maflcko
  9. DrahtBot cross-referenced this on Nov 3, 2021 from issue Update basic multisig test/docs to use multipath descriptor by mjdietzx
  10. DrahtBot cross-referenced this on Nov 3, 2021 from issue rpc, wallet: Add listaddresses RPC by lsilva01
  11. DrahtBot cross-referenced this on Nov 3, 2021 from issue descriptors: Be able to specify change and receiving in a single descriptor string by achow101
  12. DrahtBot cross-referenced this on Nov 3, 2021 from issue wallet: Make a tr() descriptor by default by achow101
  13. DrahtBot cross-referenced this on Nov 3, 2021 from issue rpc: add path to gethdkey by Sjors
  14. DrahtBot cross-referenced this on Nov 3, 2021 from issue wallet: Properly support a wallet id by achow101
  15. DrahtBot cross-referenced this on Nov 3, 2021 from issue wallet: Remove WalletDatabase refcounting and enforce only one Batch access the database at a time by achow101
  16. DrahtBot cross-referenced this on Nov 3, 2021 from issue wallet: Migrate legacy wallets to descriptor wallets by achow101
  17. DrahtBot cross-referenced this on Nov 3, 2021 from issue Wallet passive startup by ryanofsky
  18. 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

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

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

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

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

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

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

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

  27. DrahtBot removed the label Needs rebase on Nov 9, 2021
  28. achow101 force-pushed on Nov 9, 2021
  29. achow101 force-pushed on Nov 9, 2021
  30. achow101 force-pushed on Nov 10, 2021
  31. 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

  32. 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](/github-metadata-backup-bitcoin-bitcoin/contributor/param/)[out] pubkeys Any public keys.
         * [@param](/github-metadata-backup-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

  33. 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](/github-metadata-backup-bitcoin-bitcoin/contributor/param/)[out] pubkeys Any public keys.
         * [@param](/github-metadata-backup-bitcoin-bitcoin/contributor/param/)[out] pubkeys Any extended public keys.
         */
    

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

    Done

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

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

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

  36. achow101 force-pushed on Nov 12, 2021
  37. DrahtBot cross-referenced this on Nov 13, 2021 from issue Add `src/node/` and `src/wallet/` code to `node::` and `wallet::` namespaces by ryanofsky
  38. achow101 force-pushed on Nov 13, 2021
  39. DrahtBot cross-referenced this on Nov 14, 2021 from issue wallet: Avoid underpaying transaction fees when signing taproot spends by achow101
  40. 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.

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

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

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

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

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

  46. Sjors approved
  47. 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)

  48. achow101 force-pushed on Nov 17, 2021
  49. 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.

  50. DrahtBot cross-referenced this on Nov 18, 2021 from issue rpc, wallet: addhdseed, infer seed when importing descriptor with xpub by Sjors
  51. DrahtBot added the label Needs rebase on Nov 22, 2021
  52. achow101 force-pushed on Nov 22, 2021
  53. DrahtBot removed the label Needs rebase on Nov 22, 2021
  54. DrahtBot cross-referenced this on Dec 2, 2021 from issue MOVEONLY: Move wallet backup and encryption RPCs out of rpcwallet by meshcollider
  55. DrahtBot cross-referenced this on Dec 4, 2021 from issue Split up rpcwallet by meshcollider
  56. DrahtBot added the label Needs rebase on Dec 8, 2021
  57. achow101 force-pushed on Dec 8, 2021
  58. DrahtBot removed the label Needs rebase on Dec 8, 2021
  59. 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

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

  61. achow101 force-pushed on Dec 13, 2021
  62. DrahtBot cross-referenced this on Dec 18, 2021 from issue docs: avoid C-style casts; use modern C++ casts by PastaPastaPasta
  63. Sjors commented at 8:35 AM on December 30, 2021: member

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

  64. DrahtBot added the label Needs rebase on Jan 11, 2022
  65. achow101 force-pushed on Jan 11, 2022
  66. DrahtBot removed the label Needs rebase on Jan 11, 2022
  67. Sjors commented at 3:56 PM on January 12, 2022: member

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

  68. Sjors cross-referenced this on Jan 28, 2022 from issue Add taproot descriptor to existing descriptor wallet by Sjors
  69. maflcko removed the label Build system on Jan 28, 2022
  70. maflcko removed the label RPC/REST/ZMQ on Jan 28, 2022
  71. DrahtBot cross-referenced this on Mar 29, 2022 from issue wallet: Improve AvailableCoins performance by reducing duplicated operations by achow101
  72. achow101 cross-referenced this on Apr 11, 2022 from issue Add an option to create taproot descriptor for old descriptor wallets by Tracachang
  73. Sjors cross-referenced this on Apr 15, 2022 from issue Awesome multisig PR labyrinth guide by Sjors
  74. DrahtBot cross-referenced this on Apr 17, 2022 from issue [Draft / POC] Silent Payments by w0xlt
  75. DrahtBot cross-referenced this on Apr 19, 2022 from issue wallet: Load database records in a particular order by achow101
  76. DrahtBot cross-referenced this on Apr 25, 2022 from issue Remove not needed clang-format off comments by maflcko
  77. DrahtBot added the label Needs rebase on Apr 26, 2022
  78. achow101 force-pushed on Apr 26, 2022
  79. DrahtBot removed the label Needs rebase on Apr 26, 2022
  80. Sjors commented at 12:35 PM on April 29, 2022: member

    re-utACK 3992d06c15af5242d955fd7f74e9e089bbb8a166

  81. Sjors cross-referenced this on May 2, 2022 from issue psbt: Taproot fields for PSBT by achow101
  82. DrahtBot cross-referenced this on Jun 29, 2022 from issue wallet: group independent db writes on single batched db transaction by furszy
  83. DrahtBot cross-referenced this on Jul 26, 2022 from issue refactor: Make const references to avoid unnecessarily copying objects and enable two clang-tidy checks by aureleoules
  84. DrahtBot cross-referenced this on Jul 29, 2022 from issue wallet, refactor: #24584 follow-ups by josibake
  85. DrahtBot cross-referenced this on Jul 30, 2022 from issue rpc: Filter inputs by type during CoinSelection by aureleoules
  86. DrahtBot cross-referenced this on Aug 2, 2022 from issue wallet: Include a signature with encrypted keys to mitigate a wallet scam by achow101
  87. DrahtBot added the label Needs rebase on Aug 5, 2022
  88. achow101 force-pushed on Aug 5, 2022
  89. DrahtBot removed the label Needs rebase on Aug 6, 2022
  90. Sjors commented at 9:34 AM on August 6, 2022: member

    CI is not happy :-(

  91. DrahtBot cross-referenced this on Aug 9, 2022 from issue wallet: group outputs only once, decouple it from Coin Selection by furszy
  92. DrahtBot cross-referenced this on Aug 9, 2022 from issue wallet: Check max transaction weight in CoinSelection by aureleoules
  93. achow101 force-pushed on Aug 10, 2022
  94. Sjors commented at 8:51 AM on August 10, 2022: member

    re-utACK 8133ce4da3fbbcbecab4346cbe3cc825f4467ce7

  95. DrahtBot added the label Needs rebase on Aug 17, 2022
  96. achow101 force-pushed on Aug 17, 2022
  97. DrahtBot removed the label Needs rebase on Aug 17, 2022
  98. 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.

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

  100. achow101 force-pushed on Aug 17, 2022
  101. 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

  102. DrahtBot cross-referenced this on Aug 18, 2022 from issue wallet: remove UNKNOWN type from OUTPUT_TYPES array by furszy
  103. DrahtBot added the label Needs rebase on Aug 19, 2022
  104. achow101 force-pushed on Aug 19, 2022
  105. DrahtBot removed the label Needs rebase on Aug 19, 2022
  106. 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

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

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

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

  110. w0xlt approved
  111. achow101 force-pushed on Aug 20, 2022
  112. achow101 force-pushed on Aug 20, 2022
  113. w0xlt approved
  114. Sjors commented at 2:50 PM on August 22, 2022: member

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

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

    re-utACK 5323e56ffb8762d5d99bb89d1623993ad6db5849

  117. achow101 force-pushed on Aug 22, 2022
  118. 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.

  119. achow101 force-pushed on Aug 22, 2022
  120. achow101 force-pushed on Aug 22, 2022
  121. achow101 force-pushed on Aug 22, 2022
  122. 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.

  123. achow101 cross-referenced this on Aug 22, 2022 from issue wallet: rpc to add automatically generated descriptors by achow101
  124. w0xlt approved
  125. DrahtBot added the label Needs rebase on Sep 1, 2022
  126. achow101 force-pushed on Sep 1, 2022
  127. DrahtBot removed the label Needs rebase on Sep 1, 2022
  128. 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?

  129. Sjors approved
  130. Sjors commented at 11:37 AM on September 2, 2022: member

    re-utACK 6bdd8a6759c9fc93cd19612bfbd39c1abcc4de00

  131. DrahtBot cross-referenced this on Sep 3, 2022 from issue refactor: Use util::Result class for wallet loading by ryanofsky
  132. achow101 cross-referenced this on Sep 5, 2022 from issue wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets by achow101
  133. DrahtBot cross-referenced this on Sep 20, 2022 from issue Wallet: Add foreign_outputs metadata to support CoinJoin transactions by luke-jr
  134. achow101 force-pushed on Sep 20, 2022
  135. Sjors commented at 9:57 AM on September 22, 2022: member

    re-utACK 8c5ddf432dceb084be6612b378dacf9211e8cf08

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

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

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

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

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

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

  142. achow101 force-pushed on Sep 22, 2022
  143. 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?

  144. achow101 force-pushed on Sep 22, 2022
  145. DrahtBot cross-referenced this on Sep 30, 2022 from issue wallet: #25768 follow ups by stickies-v
  146. aureleoules approved
  147. aureleoules commented at 1:39 PM on October 4, 2022: member

    re-ACK 5145d1610202356676e5a031c68d4af0b16a62d9 - minor changes since last review

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

  149. achow101 force-pushed on Oct 10, 2022
  150. achow101 force-pushed on Oct 10, 2022
  151. aureleoules approved
  152. aureleoules commented at 4:41 PM on October 11, 2022: member

    re-ACK bdc59fcf8a5ef473461c43c2a035accd4f89e35d

  153. DrahtBot added the label Needs rebase on Oct 13, 2022
  154. moveonly: move WalletStorage to separate file 18054704cf
  155. walletdb: Add HDKey records 312245433d
  156. walletdb: Add WriteKeyManKey and WriteCryptedKeyManKey
    These functions write new key records for keys handled by a KeyManager
    3fa66d009a
  157. 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
  158. descspkm: Track CKeyIDs of our keys
    When DescriptorScriptPubKeyMan no longer manages its keys, it still
    needs to know the IDs of its keys.
    ebec893ce1
  159. wallet: Add KeyManager class db6efef098
  160. descspkm: Add KeyManager to DescriptorScriptPubKeyMan and use for keys 60a499b5dd
  161. descspkm: Encrypt with KeyManager instead of direct map access 1e99ebda77
  162. descspkm: Use KeyManager::LoadKey and LoadCryptedKey when loading 9b44055c4f
  163. descspkm: Replace GetKeys with KeyManager::GetKeys d04dd5becf
  164. descspkm: Replace HavePrivateKeys with KeyManager::HavePrivateKeys() d4c78ace5a
  165. keyman: Make some members private 38e099bd2b
  166. wallet: Have KeyManager in CWallet rather than DescriptorScriptPubKeyMan fc89a37d81
  167. walletdb: Refactor deserialaization of keys with checksums
    This will become shared later.
    b21982f390
  168. walletdb: Load keys into KeyManager directly 8436d2ff55
  169. 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
  170. wallet: Use KeyManager to generate master key b0adf464c2
  171. descriptor: Be able to get the pubkeys involved in a descriptor 4ca53084e7
  172. walletdb: Implement upgrading a wallet to use KeyManager f7fbd0a5fd
  173. descspkm: Remove unneeded key loading
    Key management will be done entirely by KeyManager, so
    DescriptorScriptPubKeyMan does not need key loading functions.
    aee80e5a0d
  174. rpc: Add getxpub command c9af030cd6
  175. achow101 force-pushed on Oct 17, 2022
  176. aureleoules approved
  177. aureleoules commented at 3:25 PM on October 17, 2022: member

    reACK c9af030cd60f1fe4e66ccba585b616c1dcc11a50 - minor rebase, added KeyManager m_keyman to CWallet

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

    re-utACK c9af030cd60f1fe4e66ccba585b616c1dcc11a50

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

    Concept ACK. Started code review

  181. DrahtBot cross-referenced this on Nov 25, 2022 from issue Wallet: estimate the size of signed inputs using descriptors by darosior
  182. DrahtBot cross-referenced this on Nov 25, 2022 from issue Wallet: don't underestimate the fees when spending a Taproot output by darosior
  183. DrahtBot cross-referenced this on Nov 29, 2022 from issue wallet: Migrate legacy wallets to descriptor wallets without requiring BDB by achow101
  184. DrahtBot cross-referenced this on Dec 3, 2022 from issue wallet: Migrate non-HD keys with single combo containing a list of keys by achow101
  185. achow101 cross-referenced this on Dec 19, 2022 from issue wallet: Have the wallet store the key for automatically generated descriptors by achow101
  186. 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.

  187. achow101 closed this on Dec 19, 2022

  188. bitcoin locked this on Dec 19, 2023

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:53 UTC