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

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

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

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

DrahtBot added the label Build system on Nov 2, 2021

DrahtBot added the label Descriptors on Nov 2, 2021

DrahtBot added the label RPC/REST/ZMQ on Nov 2, 2021

DrahtBot added the label Wallet on Nov 2, 2021

DrahtBot commented at 0:08 am on November 3, 2021: contributor

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

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.

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

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

0// The active HD master key, identified by its extended public key

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

Done

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

0// An HD master key, identified by its extended public key

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

Done

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.

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

Sjors commented at 4:57 pm on November 5, 2021: member

I rebased #22341 on top of this.

Some initial review remarks:

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

 0wallet/keyman.cpp:241:5: warning: calling function 'AssertLockHeldInternal<AnnotatedMixin<std::recursive_mutex>>' requires holding mutex 'cs_keyman' exclusively [-Wthread-safety-analysis]
 1    AssertLockHeld(cs_keyman);
 2    ^
 3./sync.h:83:28: note: expanded from macro 'AssertLockHeld'
 4#define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, &cs)
 5                           ^
 6wallet/keyman.cpp:244:41: warning: reading variable 'm_map_crypted_keys' requires holding mutex 'cs_keyman' [-Wthread-safety-analysis]
 7        for (const auto& [id, key_pair] : m_map_crypted_keys) {
 8                                        ^
 9wallet/keyman.cpp:244:41: warning: reading variable 'm_map_crypted_keys' requires holding mutex 'cs_keyman' [-Wthread-safety-analysis]
10wallet/keyman.cpp:253:12: warning: reading variable 'm_map_keys' requires holding mutex 'cs_keyman' [-Wthread-safety-analysis]
11    return m_map_keys;
12           ^
13wallet/keyman.cpp:264:9: warning: reading variable 'm_map_crypted_keys' requires holding mutex 'cs_keyman' [-Wthread-safety-analysis]
14    if (m_map_crypted_keys.count(id) == 0) {
15        ^
16wallet/keyman.cpp:267:12: warning: reading variable 'm_map_crypted_keys' requires holding mutex 'cs_keyman' [-Wthread-safety-analysis]
17    return m_map_crypted_keys.at(id);
18           ^
196 warnings generated.
20
21...
22
23wallet/walletdb.cpp:813:38: warning: calling function 'LoadActiveHDKey' requires holding mutex 'pwallet->GetKeyManager().cs_keyman' exclusively [-Wthread-safety-analysis]
24            pwallet->GetKeyManager().LoadActiveHDKey(extpub);
25                                     ^
26wallet/walletdb.cpp:1150:38: warning: calling function 'SetActiveHDKey' requires holding mutex 'pwallet->GetKeyManager().cs_keyman' exclusively [-Wthread-safety-analysis]
27            pwallet->GetKeyManager().SetActiveHDKey(best_xpub);

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

DrahtBot added the label Needs rebase on Nov 8, 2021

achow101 force-pushed on Nov 9, 2021

achow101 commented at 8:01 pm on November 9, 2021: member

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

DrahtBot removed the label Needs rebase on Nov 9, 2021

achow101 force-pushed on Nov 9, 2021

achow101 force-pushed on Nov 10, 2021

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

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:

0 /** Return all (extended) public  keys for this descriptor, including any from any subdescriptors.
1     *
2     * [@param](/bitcoin-bitcoin/contributor/param/)[out] pubkeys Any public keys.
3     * [@param](/bitcoin-bitcoin/contributor/param/)[out] pubkeys Any extended public keys.
4     */

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

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:

0 /** Return all (extended) public  keys for this descriptor
1     *
2     * [@param](/bitcoin-bitcoin/contributor/param/)[out] pubkeys Any public keys.
3     * [@param](/bitcoin-bitcoin/contributor/param/)[out] pubkeys Any extended public keys.
4     */

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

Done

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.

Sjors commented at 6:11 pm on November 11, 2021: member

Some more comments after light-weight code review…

achow101 force-pushed on Nov 12, 2021

achow101 force-pushed on Nov 13, 2021

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.

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

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.

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.

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.

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

Sjors approved

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)

achow101 force-pushed on Nov 17, 2021

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.

DrahtBot added the label Needs rebase on Nov 22, 2021

achow101 force-pushed on Nov 22, 2021

DrahtBot removed the label Needs rebase on Nov 22, 2021

DrahtBot added the label Needs rebase on Dec 8, 2021

achow101 force-pushed on Dec 8, 2021

DrahtBot removed the label Needs rebase on Dec 8, 2021

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

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

achow101 force-pushed on Dec 13, 2021

Sjors commented at 8:35 am on December 30, 2021: member

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

DrahtBot added the label Needs rebase on Jan 11, 2022

achow101 force-pushed on Jan 11, 2022

DrahtBot removed the label Needs rebase on Jan 11, 2022

Sjors commented at 3:56 pm on January 12, 2022: member

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

maflcko removed the label Build system on Jan 28, 2022

maflcko removed the label RPC/REST/ZMQ on Jan 28, 2022

DrahtBot added the label Needs rebase on Apr 26, 2022

achow101 force-pushed on Apr 26, 2022

DrahtBot removed the label Needs rebase on Apr 26, 2022

Sjors commented at 12:35 pm on April 29, 2022: member

re-utACK 3992d06c15af5242d955fd7f74e9e089bbb8a166

DrahtBot added the label Needs rebase on Aug 5, 2022

achow101 force-pushed on Aug 5, 2022

DrahtBot removed the label Needs rebase on Aug 6, 2022

Sjors commented at 9:34 am on August 6, 2022: member

CI is not happy :-(

achow101 force-pushed on Aug 10, 2022

Sjors commented at 8:51 am on August 10, 2022: member

re-utACK 8133ce4da3fbbcbecab4346cbe3cc825f4467ce7

DrahtBot added the label Needs rebase on Aug 17, 2022

achow101 force-pushed on Aug 17, 2022

DrahtBot removed the label Needs rebase on Aug 17, 2022

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.

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

Tested that #22341 still works on top of this.

achow101 commented at 3:11 pm on August 17, 2022: member

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

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

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

achow101 force-pushed on Aug 17, 2022

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

DrahtBot added the label Needs rebase on Aug 19, 2022

achow101 force-pushed on Aug 19, 2022

DrahtBot removed the label Needs rebase on Aug 19, 2022

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 ?

0    key.reserve(privkey.size() + pubkey.size());

achow101 commented at 0:56 am on August 20, 2022:

Done

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 ?

0    mutable Mutex m_keyman_mutex;

achow101 commented at 0:56 am on August 20, 2022:

It ends up being used recursively.

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:

0            if (desc.find("pkh(")) {

achow101 commented at 0: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.

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 0: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.

w0xlt approved

w0xlt commented at 11:20 pm on August 19, 2022: contributor

ACK https://github.com/bitcoin/bitcoin/pull/23417/commits/aa91428a1a017d833bd542c8c99fe53d9fc11d1f

achow101 force-pushed on Aug 20, 2022

w0xlt approved

w0xlt commented at 2:00 am on August 20, 2022: contributor

reACK https://github.com/bitcoin/bitcoin/pull/23417/commits/aa91428a1a017d833bd542c8c99fe53d9fc11d1f

Sjors commented at 2:50 pm on August 22, 2022: member

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

achow101 force-pushed on Aug 22, 2022

Sjors commented at 6:57 pm on August 22, 2022: member

re-utACK 5323e56ffb8762d5d99bb89d1623993ad6db5849

achow101 force-pushed on Aug 22, 2022

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.

achow101 force-pushed on Aug 22, 2022

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.

w0xlt approved

w0xlt commented at 0:36 am on August 23, 2022: contributor

reACK https://github.com/bitcoin/bitcoin/pull/23417/commits/93294e30f1f88246c47bab2bd6a091f89a520a68

DrahtBot added the label Needs rebase on Sep 1, 2022

achow101 force-pushed on Sep 1, 2022

DrahtBot removed the label Needs rebase on Sep 1, 2022

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?

Sjors approved

Sjors commented at 11:37 am on September 2, 2022: member

re-utACK 6bdd8a6759c9fc93cd19612bfbd39c1abcc4de00

achow101 force-pushed on Sep 20, 2022

Sjors commented at 9:57 am on September 22, 2022: member

re-utACK 8c5ddf432dceb084be6612b378dacf9211e8cf08

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

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

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:

0        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

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

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

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:

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

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?

achow101 force-pushed on Sep 22, 2022

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

0    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?

achow101 force-pushed on Sep 22, 2022

aureleoules approved

aureleoules commented at 1:39 pm on October 4, 2022: member

re-ACK 5145d1610202356676e5a031c68d4af0b16a62d9 - minor changes since last review

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

achow101 force-pushed on Oct 10, 2022

aureleoules approved

aureleoules commented at 4:41 pm on October 11, 2022: member

re-ACK bdc59fcf8a5ef473461c43c2a035accd4f89e35d

DrahtBot added the label Needs rebase on Oct 13, 2022

moveonly: move WalletStorage to separate file 18054704cf

walletdb: Add HDKey records 312245433d

walletdb: Add WriteKeyManKey and WriteCryptedKeyManKey

These functions write new key records for keys handled by a KeyManager

3fa66d009a

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

descspkm: Track CKeyIDs of our keys

When DescriptorScriptPubKeyMan no longer manages its keys, it still
needs to know the IDs of its keys.

ebec893ce1

wallet: Add KeyManager class db6efef098

descspkm: Add KeyManager to DescriptorScriptPubKeyMan and use for keys 60a499b5dd

descspkm: Encrypt with KeyManager instead of direct map access 1e99ebda77

descspkm: Use KeyManager::LoadKey and LoadCryptedKey when loading 9b44055c4f

descspkm: Replace GetKeys with KeyManager::GetKeys d04dd5becf

descspkm: Replace HavePrivateKeys with KeyManager::HavePrivateKeys() d4c78ace5a

keyman: Make some members private 38e099bd2b

wallet: Have KeyManager in CWallet rather than DescriptorScriptPubKeyMan fc89a37d81

walletdb: Refactor deserialaization of keys with checksums

This will become shared later.

b21982f390

walletdb: Load keys into KeyManager directly 8436d2ff55

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

wallet: Use KeyManager to generate master key b0adf464c2

descriptor: Be able to get the pubkeys involved in a descriptor 4ca53084e7

walletdb: Implement upgrading a wallet to use KeyManager f7fbd0a5fd

descspkm: Remove unneeded key loading

Key management will be done entirely by KeyManager, so
DescriptorScriptPubKeyMan does not need key loading functions.

aee80e5a0d

rpc: Add getxpub command c9af030cd6

achow101 force-pushed on Oct 17, 2022

aureleoules approved

aureleoules commented at 3:25 pm on October 17, 2022: member

reACK c9af030cd60f1fe4e66ccba585b616c1dcc11a50 - minor rebase, added KeyManager m_keyman to CWallet

DrahtBot removed the label Needs rebase on Oct 17, 2022

Sjors commented at 1:23 pm on October 27, 2022: member

re-utACK c9af030cd60f1fe4e66ccba585b616c1dcc11a50

S3RK commented at 8:10 am on November 9, 2022: contributor

Concept ACK. Started code review

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.

achow101 closed this on Dec 19, 2022

bitcoin locked this on Dec 19, 2023

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

Conflicts