wallet: Have the wallet store the key for automatically generated descriptors

achow101 commented at 10:31 pm on December 19, 2022: member

The wallet currently does not know the master key that was used to generate the automatically generated descriptors. This makes it difficult to add new automatically generated descriptors when new ones are introduced. So instead of losing this information after the descriptors are created, have CWallet store it. The xpub will be stored in a new activehdkey field. The private key must be one of the keys that is used by the descriptors, and will be extracted upon loading.

As this is a new field, wallets will be automatically upgraded upon loading. This loading is backwards compatible and uses a new non-required flag WALLET_FLAG_GLOBAL_HD_KEY to signal that the upgrade completed. The upgrade will search for an xpub that is shared by pkh(), wpkh(), and sh(wpkh() descriptors with the derivation pattern that we use. For new wallets, the xpub will be set during descriptor creation rather than trying to reverse engineer it. The flag will be set for all wallets, regardless of whether such an xpub was found or can even exist, in order to indicate that the upgrade will not need to be run in the future.

This allows us to have a gethdkey command which is useful for those who need a simple way to get an xpub from a wallet. gethdkey can also take a boolean parameter to indicate whether it should also output the corresponding xprv.

Supercedes #23417

DrahtBot commented at 10:31 pm on December 19, 2022: contributor

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

Code Coverage

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	RandyMcMillan, ryanofsky
Approach ACK	w0xlt
Stale ACK	furszy, Sjors, S3RK

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29136 (wallet: addhdkey RPC to add just keys to wallets via new void(KEY) descriptor by achow101)
#29130 (wallet: Add createwalletdescriptor and gethdkeys RPCs for adding new automatically generated descriptors by achow101)
#29124 (wallet: Automatically repair corrupted metadata with doubled derivation path by achow101)
#29054 (wallet: reenable sethdseed for descriptor wallets by achow101)
#29016 (RPC: add new listmempooltransactions by niftynei)
#28724 (wallet: Cleanup accidental encryption keys in watchonly wallets by achow101)
#28574 (wallet: optimize migration process, batch db transactions by furszy)
#28333 (wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively by achow101)
#28142 (wallet: Allow users to create a wallet that encrypts all database records by achow101)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
#27286 (wallet: Keep track of the wallet’s own transaction outputs in memory by achow101)
#26008 (wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets by achow101)
#25907 (wallet: rpc to add automatically generated descriptors by achow101)
#24748 (test/BIP324: functional tests for v2 P2P encryption by stratospher)
#22838 (descriptors: Be able to specify change and receiving in a single descriptor string by achow101)
#22341 (rpc: add path to gethdkey by Sjors)

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.

DrahtBot added the label Wallet on Dec 19, 2022

achow101 marked this as ready for review on Dec 19, 2022

achow101 requested review from S3RK on Dec 19, 2022

achow101 force-pushed on Dec 19, 2022

in src/script/descriptor.cpp:208 in 3d4e3b9cf3 outdated

199@@ -200,6 +200,12 @@ struct PubkeyProvider
200 
201     /** Derive a private key, if private data is available in arg. */
202     virtual bool GetPrivKey(int pos, const SigningProvider& arg, CKey& key) const = 0;
203+
204+    /** Return all (extended public keys for this PubkeyProvider
205+     * param[out] pubkeys Any public keys
206+     * param[out] ext_pubs Any extended public keys
207+     */
208+    virtual void GetRootPubkey(std::set<CPubKey>& pubkeys, std::set<CExtPubKey>& ext_pubs) const = 0;

aureleoules commented at 10:23 am on December 20, 2022:

3d4e3b9cf3801a93d1a4af62994900b4f69f2edd I suggest renaming instances of Pubkey to PubKey to be consistent with *PrivKey functions.

0    virtual void GetRootPubKey(std::set<CPubKey>& pubkeys, std::set<CExtPubKey>& ext_pubs) const = 0;

achow101 commented at 7:35 pm on December 20, 2022:

Done

in src/key.cpp:370 in be4f46edd8 outdated

337+    nDepth = xpub.nDepth;
338+    std::copy(xpub.vchFingerprint, xpub.vchFingerprint + sizeof(xpub.vchFingerprint), vchFingerprint);
339+    nChild = xpub.nChild;
340+    chaincode = xpub.chaincode;
341+    key = key_in;
342+}

aureleoules commented at 10:32 am on December 20, 2022:

be4f46edd8c7199b72d3646a1d13cb66d092e43e I suggest using an initialization list.

achow101 commented at 7:35 pm on December 20, 2022:

Done

in src/wallet/walletdb.cpp:1151 in 4b9ac3e12b outdated

1146+            for (const auto& [dtype, count] : dtypes) {
1147+                if (count != 2) {
1148+                    ok = false;
1149+                    break;
1150+                }
1151+            }

aureleoules commented at 10:41 am on December 20, 2022:

3e53cc298ca1037ad2f52c65934a5e71b76f82bd Could be reduced to:

0            bool ok = std::all_of(dtypes.begin(), dtypes.end(), [](const auto& p) { return p.second == 2; });

achow101 commented at 7:35 pm on December 20, 2022:

Done

achow101 force-pushed on Dec 20, 2022

w0xlt commented at 6:08 pm on January 10, 2023: contributor

Approach ACK

in src/wallet/wallet.h:321 in abac3e3630 outdated

249@@ -250,6 +250,12 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
250     //! the current wallet version: clients below this version are not able to load the wallet
251     int nWalletVersion GUARDED_BY(cs_wallet){FEATURE_BASE};
252 
253+    //! The extended public key to be used for new automatically generated descriptors
254+    CExtPubKey m_xpub GUARDED_BY(cs_wallet);
255+    //! The previous and current extended keys that are used for automatically generated descriptors
256+    std::map<CExtPubKey, CKey> m_hd_keys GUARDED_BY(cs_wallet);

S3RK commented at 8:47 am on January 18, 2023:

This presupposes that we will and should allow changing active HD key for a given wallet. I’d like to better understand for what use-cases this is needed.

Sjors commented at 1:44 pm on January 19, 2023:

abac3e3630aa6bdab825ff628fa9389c23eda417 There’s also the scenario where we generate a new HD key and mark the existing one inactive. We could at that point delete the old HD(C)KEY record, in which case m_hd_key could just hold one key. Not sure if that’s much better.

S3RK commented at 7:10 pm on January 19, 2023:

This is exactly what I’m curious about. Should we allow to generate a new HD key for existing wallet? I’m trying to understand why someone would like to generate new HD key for existing wallet instead of just creating a new wallet?

Or should we make a rule 1 wallet = 1 HD key. Which is much simpler to understand and manage.

achow101 commented at 10:06 pm on January 24, 2023:

I’m not entirely sure of the reasoning, but it’s been requested as required feature.

in src/wallet/walletdb.cpp:1133 in 9a17347086 outdated

1162+            if (it == xpubs.end()) {
1163+                continue;
1164+            }
1165+
1166+            CExtKey extkey(it->second, key);
1167+            pwallet->AddHDKey(extkey);

S3RK commented at 8:50 am on January 18, 2023:

Probably I don’t follow, but it seems we don’t need to add all descriptor keys as HD keys to the wallet

Sjors commented at 5:36 pm on January 19, 2023:

Duplicates are skipped (see 8b0af8a56ffe31edcd2ed98d795a63fd302ae67a).

Sjors commented at 5:50 pm on January 19, 2023:

9a173470864f44d494e80b1c155d8ebe8c87c406: this function can fail, and SetActiveHDKey should probably called last.

S3RK commented at 7:17 am on March 30, 2023:

I meant, that we can write only the key for best_xpub. Why do we need to write all the candidates as well?

S3RK commented at 8:57 am on January 18, 2023: contributor

Thank you for taking your time to rework #23417. It looks like a great improvement I left some high-level comments inline and will continue with more in depth review.

in src/wallet/walletutil.h:51 in 9a17347086 outdated

46@@ -47,6 +47,9 @@ enum WalletFlags : uint64_t {
47     // Indicates that the descriptor cache has been upgraded to cache last hardened xpubs
48     WALLET_FLAG_LAST_HARDENED_XPUB_CACHED = (1ULL << 2),
49 
50+    // Indicates that the wallet is capable of having a wallet hd key
51+    WALLET_FLAG_GLOBAL_HD_KEY = (1ULL << 3),

Sjors commented at 1:52 pm on January 19, 2023:

9a173470864f44d494e80b1c155d8ebe8c87c406: if the user downgrades the node, rotates the master key and then upgrades the node again, ACTIVEHDKEY would be pointing to the wrong place. So that’s an argument to disallow opening in older wallets?

S3RK commented at 7:13 pm on January 19, 2023:

Older node has no concept of master key for descriptors wallets, so it can’t rotate the key. Can it?

Sjors commented at 10:09 am on January 20, 2023:

When it encrypts the wallet I think it generates a new set of descriptors based on a new master key.

achow101 commented at 8:22 pm on January 24, 2023:

Yes, encrypting is the only way to rotate a master key currently.

The issue with changing this to a required flag is that it can no longer be an automatic upgrade. It will need to be an explicit upgrade.

S3RK commented at 1:59 pm on November 12, 2023:

Should we add some documentation about this quirk?

ryanofsky commented at 2:41 pm on December 20, 2023:

re: #26728 (review)

Should we add some documentation about this quirk?

This is now described in the WalletBatch::WriteActiveHDKey comment, so marking this resolved.

in test/functional/wallet_backwards_compatibility.py:331 in b756f221cc outdated

325@@ -325,5 +326,35 @@ def run_test(self):
326             wallet = node_v19.get_wallet_rpc("w1_v19")
327             assert wallet.getaddressinfo(address_18075)["solvable"]
328 
329+        if self.options.descriptors:
330+            self.log.info("Test background upgrading of descriptor wallets to have hd key")
331+            for node in descriptor_nodes:

Sjors commented at 4:59 pm on January 19, 2023:

b756f221ccf9885ee08e6a7d20ed54364c6dda93: this test will no longer work as advertised once this PR is in a release. Maybe drop the loop, define node_23 and only use that one? (with a note to bump it to node_24 if that’s added).

If we drop the ability to downgrade (see my other comment), that reduces the need to test against multiple versions.

achow101 commented at 10:01 pm on January 24, 2023:

Changed to test against node_v23

in test/functional/wallet_backwards_compatibility.py:333 in b756f221cc outdated

325@@ -325,5 +326,35 @@ def run_test(self):
326             wallet = node_v19.get_wallet_rpc("w1_v19")
327             assert wallet.getaddressinfo(address_18075)["solvable"]
328 
329+        if self.options.descriptors:
330+            self.log.info("Test background upgrading of descriptor wallets to have hd key")
331+            for node in descriptor_nodes:
332+                # Make the test wallet
333+                node.createwallet(wallet_name="hdkeyupgrade", descriptors=True)

Sjors commented at 5:03 pm on January 19, 2023:

b756f221ccf9885ee08e6a7d20ed54364c6dda93 can you also test the upgrade of a locked wallet?

achow101 commented at 10:01 pm on January 24, 2023:

Done

in src/wallet/walletdb.cpp:310 in 21166afaa5 outdated

305+    std::vector<unsigned char> xpub(BIP32_EXTKEY_SIZE);
306+    extkey.Neuter().Encode(xpub.data());
307+
308+    CPrivKey privkey = extkey.key.GetPrivKey();
309+
310+    // hash pubkey/privkey to accelerate wallet load

Sjors commented at 5:05 pm on January 19, 2023:

21166afaa5106ff31aac3914ad117cff21a999d5: maybe move this repetitive stuff into a helper?

achow101 commented at 10:01 pm on January 24, 2023:

Done in a separate commit.

in src/wallet/walletdb.cpp:821 in 561a8043c6 outdated

822+            {
823+                strErr = "Error reading wallet database: CPubKey/CPrivKey corrupt";
824+                return false;
825+            }
826+
827+            if (!key.Load(pkey, extpub.pubkey, true))

Sjors commented at 5:19 pm on January 19, 2023:

561a8043c6fa41c8f3cdfa57272ca512b6d8ebb2: CKey’s Load() function already checks that the xpriv match the xpub (mostly). I think that makes the above hash check unnecessary. For good measure we could have a Load() function that takes a CExtPubKey instead of just a CPubKey so it can check the other data stored in the xpub too.

achow101 commented at 9:11 pm on January 24, 2023:

The third parameter in this call skips that check. We use the hash check instead because hashing is faster the ec mult.

Sjors commented at 8:46 am on January 25, 2023:

The speed benefit seems pretty marginal when dealing with at most a dozen extended keys. It made more sense in the legacy wallet where we deal with thousands of keys in a pool.

in src/wallet/wallet.cpp:3723 in 8b0af8a56f outdated

3572@@ -3573,6 +3573,10 @@ void CWallet::SetupDescriptorScriptPubKeyMans()
3573         CExtKey master_key;
3574         master_key.SetSeed(seed_key);
3575 
3576+        // Store the master key as our active hd key
3577+        SetActiveHDKey(master_key.Neuter());

Sjors commented at 5:23 pm on January 19, 2023:

8b0af8a56ffe31edcd2ed98d795a63fd302ae67a: a (future) sanity check may want to prevent calling SetActiveHDKey and AddHDKey in this order, especially since the latter can fail.

achow101 commented at 10:02 pm on January 24, 2023:

Reordered and added an Assume to SetActiveHDKey.

in src/wallet/wallet.cpp:3578 in 8b0af8a56f outdated

3572@@ -3573,6 +3573,10 @@ void CWallet::SetupDescriptorScriptPubKeyMans()
3573         CExtKey master_key;
3574         master_key.SetSeed(seed_key);
3575 
3576+        // Store the master key as our active hd key
3577+        SetActiveHDKey(master_key.Neuter());
3578+        AddHDKey(master_key);

Sjors commented at 5:24 pm on January 19, 2023:

8b0af8a56ffe31edcd2ed98d795a63fd302ae67a: this function can fail, so let’s not ignore the return value.

achow101 commented at 10:02 pm on January 24, 2023:

Done

in src/wallet/wallet.cpp:977 in 95da2e9808 outdated

817+            if (!EncryptSecret(_vMasterKey, secret, xpub.pubkey.GetHash(), crypted_secret)) {
818+                encrypted_batch->TxnAbort();
819+                delete encrypted_batch;
820+                encrypted_batch = nullptr;
821+                // We now probably have half of our keys encrypted in memory, and half not...
822+                // die and let the user reload the unencrypted wallet.

Sjors commented at 5:26 pm on January 19, 2023:

95da2e9808c57a519023c017ca47d76ebd2632fd: Maybe log some final words…

achow101 commented at 10:02 pm on January 24, 2023:

Perhaps something to do for a followup for all of the encrypt wallet failures.

in src/wallet/wallet.h:1049 in 18ac74d29c outdated

952@@ -953,6 +953,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
953     bool LoadHDKey(const CExtPubKey& xpub, const CKey& key) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
954     bool LoadHDCryptedKey(const CExtPubKey& xpub, const std::vector<unsigned char>& crypted_key) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
955     bool AddHDKey(const CExtKey& extkey) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
956+    std::optional<CExtKey> GetHDKey(const CExtPubKey& xpub) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

Sjors commented at 5:29 pm on January 19, 2023:

18ac74d29cf3f1a8c4f46c4d030968b5596e9b09: could use documentation, and clarification that this only works for master keys, not any regular xpub, since this function works by looking through m_hd(_crypted)_keys rather than perform derivations.

Alternatively I might “upgrade” it in #22341 to perform derivation, since that’s useful. But given the convention of “hd key” referring to a master key, a function that performs derivation should probably be called GetExtendedKey()

achow101 commented at 10:02 pm on January 24, 2023:

Added a bit more to the comment for this group of functions.

in src/wallet/rpc/wallet.cpp:812 in 436eab602f outdated

755@@ -756,6 +756,61 @@ static RPCHelpMan migratewallet()
756     };
757 }
758 
759+RPCHelpMan getxpub()

Sjors commented at 5:34 pm on January 19, 2023:

Any reason why you’re putting this in rpc/wallet.cpp rather than rpc/addresses.cpp like in #23417?

achow101 commented at 9:22 pm on January 24, 2023:

Nothing in particular, just seems related more to wallet level information rather than addresses.

in test/functional/wallet_hd.py:150 in 1877da9fb5 outdated

146@@ -147,7 +147,28 @@ def run_test(self):
147         else:
148             assert_equal(keypath[0:7], "m/0'/1'")
149 
150-        if not self.options.descriptors:
151+        if self.options.descriptors:

Sjors commented at 5:38 pm on January 19, 2023:

1877da9fb564fc6fa7b6c6bc3bcf63bdb29ace7b: I’d prefer a new wallet_getxpub.py test case that I can expand in #22341.

achow101 commented at 10:03 pm on January 24, 2023:

Done

in src/script/descriptor.cpp:204 in 6072b39645 outdated

199@@ -200,6 +200,12 @@ struct PubkeyProvider
200 
201     /** Derive a private key, if private data is available in arg. */
202     virtual bool GetPrivKey(int pos, const SigningProvider& arg, CKey& key) const = 0;
203+
204+    /** Return all (extended public keys for this PubkeyProvider

Sjors commented at 5:39 pm on January 19, 2023:

6072b3964582e6883220f3af678675531299b444 nit: missing closing bracket

achow101 commented at 10:03 pm on January 24, 2023:

Done

in src/script/descriptor.cpp:208 in 6072b39645 outdated

199@@ -200,6 +200,12 @@ struct PubkeyProvider
200 
201     /** Derive a private key, if private data is available in arg. */
202     virtual bool GetPrivKey(int pos, const SigningProvider& arg, CKey& key) const = 0;
203+
204+    /** Return all (extended public keys for this PubkeyProvider
205+     * param[out] pubkeys Any public keys
206+     * param[out] ext_pubs Any extended public keys
207+     */
208+    virtual void GetRootPubKey(std::set<CPubKey>& pubkeys, std::set<CExtPubKey>& ext_pubs) const = 0;

Sjors commented at 5:42 pm on January 19, 2023:

6072b3964582e6883220f3af678675531299b444 Should this be plural GetRootPubKeys? Or maybe keep this function focussed and only return a single CExtPubKey?

achow101 commented at 10:03 pm on January 24, 2023:

Changed to plural.

It should return multiple to handle multisigs.

in src/script/descriptor.h:167 in 6072b39645 outdated

150+    /** Return all (extended) public keys for this descriptor, including any from subdescriptors.
151+     *
152+     * @param[out] pubkeys Any public keys
153+     * @param[out] ext_pubs Any extended public keys
154+     */
155+    virtual void GetPubKeys(std::set<CPubKey>& pubkeys, std::set<CExtPubKey>& ext_pubs) const = 0;

Sjors commented at 5:45 pm on January 19, 2023:

6072b3964582e6883220f3af678675531299b444: why do we care about this, vs only getting a root key?

achow101 commented at 9:32 pm on January 24, 2023:

IIRC I had a use for this in a different PR and this behavior was imported from there.

Sjors commented at 8:48 am on January 25, 2023:

If that’s no longer the case I would prefer simplifying things. The Descriptor class is already a bit hard to read.

achow101 commented at 5:38 pm on January 25, 2023:

We do actually use it in this PR to check whether a descriptor has any normal pubkeys in it. If it does, it cannot be a candidate for master hd key extraction.

in src/wallet/walletdb.cpp:1124 in 9a17347086 outdated

1119+            if (descs_keys.count(xpub) == 0) {
1120+                descs_keys.emplace(xpub, std::make_pair(tmpl, 0));
1121+            }
1122+
1123+            const std::string desc = w_desc.descriptor->ToString();
1124+            if (desc.find("pkh(") == 0) {

Sjors commented at 5:51 pm on January 19, 2023:

9a173470864f44d494e80b1c155d8ebe8c87c406: instead of this string parsing stuff, can we add some method to Descriptor to directly get what we need? Even an enum with descriptor types seems more clean.

If all we need is to find the active HD master key, why not limit the search to pkh(? That has the nice benefit of there not potentially being manually inserted descriptors that could be ambiguous).

achow101 commented at 9:43 pm on January 24, 2023:

If all we need is to find the active HD master key, why not limit the search to pkh(? That has the nice benefit of there not potentially being manually inserted descriptors that could be ambiguous).

Users can still import pkh( descriptors. By checking all of the different types, we can be more sure about which key is active.

achow101 commented at 10:04 pm on January 24, 2023:

instead of this string parsing stuff, can we add some method to Descriptor to directly get what we need? Even an enum with descriptor types seems more clean.

I’ve changed it to use OutputType that apparently descriptors can report.

in src/wallet/walletdb.cpp:1064 in 9a17347086 outdated

1086@@ -1087,6 +1087,99 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
1087         }
1088     }
1089 
1090+    // Upgrade to having a wallet hd key
1091+    // Find the keys which are used in single key internal and external descriptors with
1092+    // the pre-taproot output types

Sjors commented at 5:53 pm on January 19, 2023:

9a173470864f44d494e80b1c155d8ebe8c87c406: what do we do with tr() descriptors that have been generated since #22364?

achow101 commented at 10:04 pm on January 24, 2023:

Added checking for tr() descriptors.

in src/wallet/walletdb.cpp:1073 in 9a17347086 outdated

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 root xpubs used in pkh(), sh(wpkh()), and wpkh() descriptors
1101+        for (const auto& spkm : pwallet->GetAllScriptPubKeyMans()) {

Sjors commented at 5:59 pm on January 19, 2023:

9a173470864f44d494e80b1c155d8ebe8c87c406: Why not limit ourselves to GetActiveScriptPubKeyMans? Not sure how useful it is to store older HD keys, and it does seem to add some complexity.

achow101 commented at 9:52 pm on January 24, 2023:

The user may have imported other keys for some active spkms, so we’d be unable to determine the master if we only looked at the active ones.

Sjors commented at 6:00 pm on January 19, 2023: member

Concept ACK. Here’s my initial code review (b756f221ccf9885ee08e6a7d20ed54364c6dda93).

achow101 force-pushed on Jan 24, 2023

achow101 force-pushed on Jan 25, 2023

achow101 force-pushed on Jan 26, 2023

in src/wallet/walletdb.cpp:1072 in 9ec69417b3 outdated

1070+        !pwallet->IsWalletFlagSet(WALLET_FLAG_GLOBAL_HD_KEY)
1071+        ) {
1072+        std::map<CExtPubKey, std::pair<std::map<OutputType, int>, uint64_t>> descs_keys;
1073+        std::map<OutputType, int> tmpl = {{OutputType::LEGACY, 0}, {OutputType::P2SH_SEGWIT, 0}, {OutputType::BECH32, 0}, {OutputType::BECH32M, 0}};
1074+
1075+        // Find root xpubs used in pkh(), sh(wpkh()), and wpkh() descriptors

Sjors commented at 4:18 pm on January 27, 2023:

6bb3bee nit: and tr()

Sjors commented at 5:45 pm on January 27, 2023: member

Changes look good, but I still have to study 8f9c7b90bc0971bc48f3fc655267078650e0cbbd and 9ec69417b3ac88ebf6dc719b7571ea0cdb327e13 more. Thanks for the clarification via IRC.

achow101 commented at 8:57 pm on February 10, 2023: member

At the 2023-01-27 IRC meeting, we discussed the key rotation aspect of this PR. The conclusion was that we could not think of a use case for key rotation that is not sufficiently covered by creating a new wallet.

Since this PR does not introduce the ability for users to explicitly rotate keys, I don’t think there’s anything to change to meet this conclusion. Those updates will be in #25907.

It’s currently possible to rotate keys by encrypting the wallet. Since this is a longstanding behavior that has several tests expecting this behavior, I’ve decided to leave it in. Removing this behavior has more issues than not introducing explicit key rotation in descriptor wallets. So I’ve decided to leave the PR as is.

Sjors commented at 10:29 am on February 11, 2023: member

We should add a warning in the release notes that, although you can safely downgrade the node, you should not encrypt the wallet while using that older version. And add a test that shows what happens.

in src/wallet/walletdb.cpp:75 in 0c4b2bc687 outdated

68@@ -66,6 +69,17 @@ const std::unordered_set<std::string> LEGACY_TYPES{CRYPTED_KEY, CSCRIPT, DEFAULT
69 // WalletBatch
70 //
71 
72+template <typename P>
73+static uint256 PrivKeyChecksum(const CPrivKey& privkey, const P& pubkey)
74+{
75+    // hash pubkey/privkey to accelerate wallet load
76+    std::vector<unsigned char> to_hash;

john-moffett commented at 8:59 pm on February 13, 2023:

While you’re touching, maybe a good idea to change to std::vector<unsigned char, secure_allocator<unsigned char>>?

DrahtBot added the label Needs rebase on Feb 27, 2023

achow101 force-pushed on Mar 1, 2023

Sjors commented at 2:54 pm on March 1, 2023: member

Checked the rebase to 3d50555ec6d47f7ebdd1b74739f00a0385be1fab. I still have to study a14a7554a6 and 6bb3bee37e more.

DrahtBot removed the label Needs rebase on Mar 1, 2023

in src/wallet/walletdb.cpp:1068 in 6bb3bee37e outdated

1063+    // Find the keys which are used in single key internal and external descriptors with
1064+    // the pre-taproot output types
1065+    if (pwallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS) &&
1066+        !pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) &&
1067+        !pwallet->IsWalletFlagSet(WALLET_FLAG_GLOBAL_HD_KEY)
1068+        ) {

Sjors commented at 3:37 pm on March 1, 2023:

Some suggestions after our chat:

let’s move this into (a) helper function(s), so that LoadWallet remains somewhat readable
- FindGlobalHDKeyAmongstDescriptors()
- SetGlobalHDKey()
document this function, e.g. explain that it’s trying to find the pattern of 3 or 4 descriptor pairs that use the same master key. Note how some might no longer be active because the user imported another descriptor and made it active. There might also be more than one set matching this pattern due to toggling encryption, so we pick the most recent one.
drop the tr() bit; it’s not necessary, and it could cause a mismatch if a user did something like manually importing only the receive descriptor
document the meaning of WALLET_FLAG_GLOBAL_HD_KEY. It means we support the feature, but does not guarantee presence of an HD key.
add (a TODO for) method to the wallet tool to re-run the helper functions; this would be for advanced users who somehow ended up with a wallet not matching this pattern, but have repented, added the right descriptors, and now want the global HD key field set

S3RK commented at 7:58 am on March 6, 2023:

add (a TODO for) method to the wallet tool to re-run the helper functions; this would be for advanced users who somehow ended up with a wallet not matching this pattern, but have repented, added the right descriptors, and now want the global HD key field set

Is this really needed if we add support for sethdseed later?

achow101 commented at 8:11 pm on March 15, 2023:

Moved it to a separate function, although I did not split it up into multiple as that does not seem to be actually useful. Also added some additional comments.

achow101 force-pushed on Mar 15, 2023

S3RK commented at 7:27 am on March 30, 2023: contributor

Approach ACK.

Doing a proper code review now. Have a question about the upgrade. What do you think we should do in the following situation?

Create a blank wallet with master branch
import 6 descriptors derived from the same key. WALLET_FLAG_GLOBAL_HD_KEY is not set and no master key
getxpub reports “This wallet does not have an active xpub”
reload the wallet. Upgrade code is triggered, it sets the flag and the master key

This will become even more confusing when/if we later introduce addedseed and/or manual wallets. I think it’s better to reduce the amount of magic and only trigger upgrades for wallets created with older versions. Maybe we can set WALLET_FLAG_GLOBAL_HD_KEY for all new wallets even those that are blank?

in src/wallet/wallet.cpp:580 in 0571caf632 outdated

575+        // Automatically generated descriptors have an output type that is one of legacy, p2sh-segwit, or bech32
576+        std::optional<OutputType> output_type = w_desc.descriptor->GetOutputType();
577+        if (!output_type.has_value()) {
578+            continue;
579+        }
580+        if (LEGACY_OUTPUT_TYPES.count(output_type.value()) == 0) {

S3RK commented at 7:37 am on April 3, 2023:

better

0- if (LEGACY_OUTPUT_TYPES.count(output_type.value()) == 0) {
1+ if (tmpl.count(output_type.value()) == 0) {

or

0- if (LEGACY_OUTPUT_TYPES.count(output_type.value()) == 0) {
1+ if (descs_keys[xpub].first.count(output_type.value()) == 0) {

achow101 commented at 3:16 pm on April 10, 2023:

Done

in src/wallet/wallet.cpp:548 in 0571caf632 outdated

543+        ) {
544+        return;
545+    }
546+
547+
548+    std::map<CExtPubKey, std::pair<std::map<OutputType, int>, uint64_t>> descs_keys;

S3RK commented at 7:54 am on April 3, 2023:

naming nit: desc_keys and descs_keys are very easy to mix

achow101 commented at 3:17 pm on April 10, 2023:

Changed

in src/wallet/wallet.cpp:564 in 0571caf632 outdated

521@@ -522,6 +522,121 @@ void CWallet::UpgradeDescriptorCache()
522     SetWalletFlag(WALLET_FLAG_LAST_HARDENED_XPUB_CACHED);
523 }
524 
525+/**
526+ * UpgradeToGlobalHDKey searches through the descriptors in a descriptor wallet and tries to find a CExtPubKey
527+ * which is likely to be the most recent one used to generate all of the automatically generated descriptors.
528+ * The automatically generated descriptors are guaranteed to be 2 p2pkh, 2 p2sh-segwit, and 2 p2wpkh. There may

S3RK commented at 7:59 am on April 3, 2023:

Do we want to check that we have exactly one internal=true and one internal=false descriptors?

achow101 commented at 3:05 pm on April 10, 2023:

If they’re no longer active, we don’t know internalness.

in src/wallet/wallet.cpp:538 in 0571caf632 outdated

533+ * The best candidate will be added to the wallet as the current active HD key. Any other candidates will be added as
534+ * HD keys that have been rotated out.
535+ *
536+ * WALLET_FLAG_GLOBAL_HD_KEY will be set in order to indicate that this upgrade has occurred.
537+ */
538+void CWallet::UpgradeToGlobalHDKey(WalletBatch& batch, const std::map<std::pair<uint256, CKeyID>, CKey>& desc_keys, const std::map<std::pair<uint256, CKeyID>, std::pair<CPubKey, std::vector<unsigned char>>>& desc_crypt_keys)

S3RK commented at 8:00 am on April 3, 2023:

nit: maybe good time to add typedef. I had a very hard time reading the signature of the fuction

achow101 commented at 3:17 pm on April 10, 2023:

Done

in src/wallet/wallet.cpp:590 in 0571caf632 outdated

585+        if (w_desc.creation_time > descs_keys[xpub].second) {
586+            descs_keys[xpub].second = w_desc.creation_time;
587+        }
588+    }
589+
590+    // Find candidate active xpubs

S3RK commented at 8:04 am on April 3, 2023:

they don’t have to be active

0- // Find candidate active xpubs
1+ // Find candidate xpubs

achow101 commented at 3:17 pm on April 10, 2023:

Done

in src/wallet/wallet.cpp:606 in 0571caf632 outdated

601+                best_xpub.emplace(xpub);
602+            }
603+        }
604+    }
605+
606+    // Add hd keys

S3RK commented at 8:07 am on April 3, 2023:

Could clarify comment: For all the candidate xpubs add corresponding private key if available

achow101 commented at 3:17 pm on April 10, 2023:

Done

in src/wallet/wallet.cpp:655 in 0571caf632 outdated

612+            continue;
613+        }
614+
615+        CExtKey extkey(it->second, key);
616+        AddHDKey(extkey);
617+        xpubs.erase(it);

S3RK commented at 8:12 am on April 3, 2023:

why do we need to erase it?

achow101 commented at 3:10 pm on April 10, 2023:

So they aren’t repeatedly written when the same key shows up in multiple descriptors.

S3RK commented at 6:58 am on April 11, 2023:

I guess that’s ok, but AddHDKey already has an early return for existing keys.

in src/wallet/wallet.cpp:667 in 0571caf632 outdated

624+        if (it == xpubs.end()) {
625+            continue;
626+        }
627+
628+        LoadHDCryptedKey(it->second, ckey);
629+        batch.WriteHDCryptedKey(it->second, ckey);

S3RK commented at 8:15 am on April 3, 2023:

This requires compatibility between WALLETDESCRIPTORCKEY and HDCKEY. Should we document it somewhere?

achow101 commented at 3:11 pm on April 10, 2023:

The encrypted secrets are all in the same format for both legacy wallet keys, descriptor keys, and now hd ckeys. I suppose this could be documented somewhere, although it’s not immediately obvious where.

S3RK commented at 6:55 am on April 11, 2023:

Yes this is nice to have. Since the key format could never change due to backward compatibility requirements, this is not an issue. You can resolve this comment.

S3RK commented at 8:15 am on April 3, 2023: contributor

Did some review of 0571caf6324549e2b840e58d8b11c3d31973a7c4

achow101 force-pushed on Apr 10, 2023

achow101 force-pushed on May 2, 2023

DrahtBot added the label CI failed on May 2, 2023

DrahtBot removed the label CI failed on May 2, 2023

in test/functional/wallet_backwards_compatibility.py:362 in c79539fc20 outdated

357+                shutil.rmtree(node_wallet_dir)
358+                shutil.copytree(node_master_wallet_dir, node_wallet_dir)
359+                node_v23.loadwallet(wallet_name)
360+                node_v23.unloadwallet(wallet_name)
361+
362+                # Remove wallet from master for next node

S3RK commented at 7:25 am on May 17, 2023:

nit: for the next ~~node~~test

achow101 commented at 10:19 am on June 8, 2023:

Done

S3RK commented at 7:26 am on May 17, 2023: contributor

Code review ACK c79539fc20d902b5f9e098cc996c4e27c8e5b8c5

I plan to test this manually a bit later

in src/wallet/wallet.cpp:556 in 95d11f09c2 outdated

551+        // Automatically generated descriptors are ranged
552+        if (!w_desc.descriptor->IsRange()) {
553+            continue;
554+        }
555+
556+        // Audotmatically generated descriptors have exactly 1 xpub and no other keys

Sjors commented at 2:24 pm on June 5, 2023:

95d11f09c2c0d4ad66ba2e4ce2d15be4c002fb94 // Automatically

achow101 commented at 10:20 am on June 8, 2023:

Done

in src/wallet/wallet.cpp:647 in 95d11f09c2 outdated

596+            }
597+        }
598+    }
599+
600+    // For all candidate xpubs, add corresponding private keys if available
601+    for (const auto& [id_pair, key] : desc_keys) {

Sjors commented at 2:31 pm on June 5, 2023:

95d11f09c2c0d4ad66ba2e4ce2d15be4c002fb94: why not loop over xpubs?

achow101 commented at 9:49 am on June 8, 2023:

We won’t be able to get the private key since we don’t know the descriptor id in order to do the lookup.

in src/wallet/wallet.cpp:672 in 95d11f09c2 outdated

623+        batch.WriteHDCryptedKey(it->second, ckey);
624+        xpubs.erase(it);
625+    }
626+
627+    if (best_xpub) {
628+        SetActiveHDKey(*best_xpub);

Sjors commented at 2:35 pm on June 5, 2023:

95d11f09c2c0d4ad66ba2e4ce2d15be4c002fb94: can we emit a warning the in the else case?

achow101 commented at 9:50 am on June 8, 2023:

I don’t think that’s necessary as wallet such as ones without private keys or those with only imports won’t have a global hd key.

in src/wallet/wallet.cpp:581 in 95d11f09c2 outdated

531+ */
532+void CWallet::UpgradeToGlobalHDKey(WalletBatch& batch, const std::map<DescIDKeyIDPair, CKey>& desc_keys, const std::map<DescIDKeyIDPair, CryptedKeyPair>& desc_crypt_keys)
533+{
534+    if (!IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS) ||
535+        IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) ||
536+        IsWalletFlagSet(WALLET_FLAG_GLOBAL_HD_KEY)

Sjors commented at 2:36 pm on June 5, 2023:

95d11f09c2c0d4ad66ba2e4ce2d15be4c002fb94: it’s better if the caller checks this flag. It makes it easier to implement a “try again” feature later for folks with complicated wallets where somehow the upgrade fails to find a best xpub. Can wait for followup though.

achow101 commented at 9:50 am on June 8, 2023:

This follows the same pattern for the other upgrading methods.

Sjors commented at 12:41 pm on June 22, 2023:

But those can’t fail. In any case, it can be changed later.

Sjors approved

Sjors commented at 3:06 pm on June 5, 2023: member

utACK c79539fc20d902b5f9e098cc996c4e27c8e5b8c5

Everything since my last review is a rebase, except 95d11f09c2c0d4ad66ba2e4ce2d15be4c002fb94, so I only re-reviewed that, plus 70a2bb75442daaef5685728b4b03c7b2fe49fd38.

I’d still like to see a test and/or release note to cover the case where a user upgrades, then downgrades and encrypts their wallet and then upgrades. See https://github.com/bitcoin/bitcoin/pull/26728/commits/95d11f09c2c0d4ad66ba2e4ce2d15be4c002fb94#r1081289252 But it can wait for a followup, since it seems rare.

S3RK commented at 7:16 am on June 7, 2023: contributor

tACK c79539fc20d902b5f9e098cc996c4e27c8e5b8c5

I found one odd behaviour mentioned in #26728 (comment) Even if you create a wallet with the master branch you still have one opportunity to trigger upgrade procedure

Reproduction steps

create new blank wallet with master branch
import 6 descriptors derived from same HD key
call to getxpub says there is no HD key
reload wallet (this will trigger upgrade procedure)
call to getxpub shows HD key

Note, if you reload the empty wallet before you imported anything - you lost your chance to upgrade

My proposed fix is to disallow upgrade procedure for wallets created with master by setting WALLET_FLAG_GLOBAL_HD_KEY for all newborn wallets. Later we want to add a separate method to change HD key anyway to cover that use case.

glozow requested review from Sjors on Jun 7, 2023

glozow removed review request from Sjors on Jun 7, 2023

glozow requested review from furszy on Jun 7, 2023

achow101 force-pushed on Jun 8, 2023

achow101 commented at 10:24 am on June 8, 2023: member

My proposed fix is to disallow upgrade procedure for wallets created with master by setting WALLET_FLAG_GLOBAL_HD_KEY for all newborn wallets. Later we want to add a separate method to change HD key anyway to cover that use case.

I thought this was doing this already, but it turns out that we don’t set the descriptor flag until after the upgrade stuff is run for creation, so it wasn’t being set. I’ve added a commit which will set the flag for all new wallets.

DrahtBot added the label CI failed on Jun 8, 2023

in src/wallet/wallet.cpp:3072 in ad6971fbf8 outdated

2950@@ -2951,7 +2951,7 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
2951         // ensure this wallet.dat can only be opened by clients supporting HD with chain split and expects no default key
2952         walletInstance->SetMinVersion(FEATURE_LATEST);
2953 
2954-        walletInstance->InitWalletFlags(wallet_creation_flags);
2955+        walletInstance->InitWalletFlags(wallet_creation_flags | WALLET_FLAG_GLOBAL_HD_KEY);

S3RK commented at 8:05 am on June 12, 2023:

This will set WALLET_FLAG_GLOBAL_HD_KEY for non-descriptor wallets as well, which is a minor bug in my opinion.

I think we should set the flag in CreateWallet function in the same place we force sqlite for descriptor wallets. This line begs the question why wallet_creation_flags doesn’t have the WALLET_FLAG_GLOBAL_HD_KEY set already.

achow101 commented at 3:27 pm on June 12, 2023:

Done as suggested

Sjors commented at 11:55 am on June 22, 2023:

fcbdc4873588ebab1644ffe59b7ab30fb2ecbed6: This line is still here though.

Commit description could say “for new descriptor wallets”

achow101 commented at 2:53 pm on June 22, 2023:

Oops, fixed.

achow101 force-pushed on Jun 12, 2023

achow101 commented at 3:27 pm on June 12, 2023: member

Added a commit to make sure that migrated wallets also have the global hd key.

in src/wallet/wallet.cpp:4047 in 24b3ee7ee9 outdated

4020@@ -4021,11 +4021,17 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
4021     m_internal_spk_managers.clear();
4022 
4023     // Setup new descriptors
4024-    SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
4025+    SetWalletFlag(WALLET_FLAG_DESCRIPTORS | WALLET_FLAG_GLOBAL_HD_KEY);

S3RK commented at 7:21 am on June 14, 2023:

Great catch! Should we also set if for watch only wallets? On line 4190?

achow101 commented at 2:40 pm on June 14, 2023:

It doesn’t make a difference for watchonly wallets, but since we are setting it for all descriptor wallets regardless of watchonly, I suppose so.

Done.

S3RK commented at 7:24 am on June 14, 2023: contributor

reACK 24b3ee7ee9b4fd1fb6f07bb03676379b5e58d3ad

Two new commits added since last review to ensure WALLET_FLAG_GLOBAL_HD_KEY is set for all new and migrated descriptor wallets.

DrahtBot requested review from Sjors on Jun 14, 2023

achow101 force-pushed on Jun 14, 2023

DrahtBot removed the label CI failed on Jun 14, 2023

S3RK commented at 7:01 am on June 19, 2023: contributor

reACK 14e2b74332bfdf455ed80ae2487762b24596235d

Now WALLET_FLAG_GLOBAL_HD_KEY is set for all descriptors wallets, including watch only

fanquake requested review from w0xlt on Jun 19, 2023

Sjors commented at 10:28 am on June 22, 2023: member

I mentioned in #27915 that we could make the flag mandatory, but if we do that, let’s keep that to a followup. Will review the last changes asap.

DrahtBot removed review request from w0xlt on Jun 22, 2023

furszy commented at 12:23 pm on June 22, 2023: member

Concept ACK, will jump here in the coming days. (ideally, post #24914 merge which shouldn’t be far 🤞🏼)

Sjors commented at 12:46 pm on June 22, 2023: member

14e2b74332bfdf455ed80ae2487762b24596235d new commits look good, modulo #26728 (review)

DrahtBot requested review from Sjors on Jun 22, 2023

achow101 force-pushed on Jun 22, 2023

Sjors commented at 6:19 pm on June 22, 2023: member

utACK ecb87f090bc53c2d9167d5468c8c9c9129981420

DrahtBot removed review request from Sjors on Jun 22, 2023

DrahtBot requested review from S3RK on Jun 22, 2023

DrahtBot added the label Needs rebase on Jun 28, 2023

achow101 force-pushed on Jun 28, 2023

DrahtBot removed the label Needs rebase on Jun 28, 2023

achow101 force-pushed on Jun 28, 2023

DrahtBot added the label CI failed on Jun 28, 2023

DrahtBot removed the label CI failed on Jun 28, 2023

Sjors commented at 8:20 am on July 2, 2023: member

Looks like a slightly non trivial rebase. For context, which PR’s triggered it?

Sjors commented at 3:29 pm on July 4, 2023: member

Building 8587bfbbbc33f2b5e3737c9afe42e6d09464064c on Ubuntu 23.04 with gcc 12.2.0 fails with:

0wallet/wallet.cpp: In function ‘std::shared_ptr<wallet::CWallet> wallet::CreateWallet(WalletContext&, const std::string&, std::optional<bool>, DatabaseOptions&, DatabaseStatus&, bilingual_str&, std::vector<bilingual_str>&)’:
1wallet/wallet.cpp:342:34: error: ‘WALLET_FLAG_GLOBAL_HD_KEY’ was not declared in this scope
2  342 |         wallet_creation_flags |= WALLET_FLAG_GLOBAL_HD_KEY;
3      |                                  ^~~~~~~~~~~~~~~~~~~~~~~~~
4make[2]: *** [Makefile:11876: wallet/libbitcoin_wallet_a-wallet.o] Error 1

That commit didn’t change since my last review, so it’s a bit odd I didn’t catch it then (I often run a script to build all commits in a PR).

achow101 force-pushed on Jul 4, 2023

achow101 commented at 3:52 pm on July 4, 2023: member

Looks like a slightly non trivial rebase. For context, which PR’s triggered it?

#24914 I think

Building 8587bfb on Ubuntu 23.04 with gcc 12.2.0 fails with:

0wallet/wallet.cpp: In function ‘std::shared_ptr<wallet::CWallet> wallet::CreateWallet(WalletContext&, const std::string&, std::optional<bool>, DatabaseOptions&, DatabaseStatus&, bilingual_str&, std::vector<bilingual_str>&)’:
1wallet/wallet.cpp:342:34: error: ‘WALLET_FLAG_GLOBAL_HD_KEY’ was not declared in this scope
2  342 |         wallet_creation_flags |= WALLET_FLAG_GLOBAL_HD_KEY;
3      |                                  ^~~~~~~~~~~~~~~~~~~~~~~~~
4make[2]: *** [Makefile:11876: wallet/libbitcoin_wallet_a-wallet.o] Error 1

That commit didn’t change since my last review, so it’s a bit odd I didn’t catch it then (I often run a script to build all commits in a PR).

I think I may have changed the order of some commits, should be fixed now.

DrahtBot added the label CI failed on Jul 4, 2023

achow101 force-pushed on Jul 4, 2023

DrahtBot removed the label CI failed on Jul 6, 2023

in src/wallet/walletdb.cpp:821 in 3de9672a60 outdated

815 
816     // Load descriptor record
817     int num_keys = 0;
818     int num_ckeys= 0;
819+    std::map<std::pair<uint256, CKeyID>, CKey> desc_keys;
820+    std::map<std::pair<uint256, CKeyID>, std::pair<CPubKey, std::vector<unsigned char>>> desc_crypt_keys;

furszy commented at 1:32 pm on July 31, 2023:

In 3de9672a:

These two variables are being set but not accessed here. Might be good to move them to the commit where they are used.

achow101 commented at 5:54 pm on September 11, 2023:

Moved to “wallet: Automatically upgrade a wallet to have global hd key”

Sjors commented at 1:38 pm on July 31, 2023: member

re-ACK 481f63e

3de9672a60a385572c294f796aff60ab38c75861 became more complicated compared to my last review, because of #24914. But it makes sense. Rest of the rebase is straight-forward.

in src/wallet/walletdb.cpp:1059 in 3de9672a60 outdated

1037+
1038+        // TODO: Load crypted xprv into the wallet
1039+        return DBErrors::LOAD_OK;
1040+    });
1041+    result = std::max(result, enc_hdkey_res.m_result);
1042+    num_ckeys += enc_hdkey_res.m_records;

furszy commented at 1:39 pm on July 31, 2023:

In 3de9672a:

Could verify here that when num_ckeys > 0, num_keys == 0 and viceversa. Failing early if it is not true.

achow101 commented at 5:54 pm on September 11, 2023:

Done

in src/wallet/wallet.cpp:647 in 97493ab8e6 outdated

595+        if (std::all_of(dtypes.begin(), dtypes.end(), [](const auto& p) { return p.second == 2;})) {
596+            xpubs.emplace(xpub.pubkey.GetID(), xpub);
597+            if (desc_time > best_time) {
598+                best_time = desc_time;
599+                best_xpub.emplace(xpub);
600+            }

furszy commented at 3:09 pm on July 31, 2023:

In 97493ab8:

What best_time is here? Shouldn’t this be looking for the oldest one?, e.g (best_time == 0 || best_time > desc_time).

Or.. you wrote it in this way because imported descriptors usually have no timestamp, so they are usually 1?.

achow101 commented at 5:55 pm on September 11, 2023:

It’s to find the latest descriptors as the descriptors in the wallet may have been rotated once already.

Added a comment explaining that.

in src/wallet/walletdb.cpp:940 in 3de9672a60 outdated

927@@ -925,6 +928,7 @@ static DBErrors LoadDescriptorWalletRecords(CWallet* pwallet, DatabaseBatch& bat
928                 return DBErrors::CORRUPT;
929             }
930             spk_man->AddKey(pubkey.GetID(), privkey);
931+            desc_keys.insert(std::make_pair(std::make_pair(desc_id, pubkey.GetID()), privkey));

furszy commented at 8:24 pm on July 31, 2023:

Instead of adding all keys, which could be a high number, what about only adding the ones related to xpubs? e.g. (quick and dirty, should also do the same for the encrypted ones)

 0diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
 1--- a/src/wallet/walletdb.cpp	(revision 7f44e83bb544f1e5c398b87c8eb7870eed3ed1ba)
 2+++ b/src/wallet/walletdb.cpp	(date 1690835678842)
 3@@ -903,10 +903,21 @@
 4         assert(spk_man);
 5         spk_man->SetCache(cache);
 6 
 7+        // For upgrade purposes, obtain root xpub (if exist)
 8+        std::optional<CPubKey> op_xpub_pubkey;
 9+        {
10+            std::set<CPubKey> desc_pubkeys;
11+            std::set<CExtPubKey> desc_xpubs;
12+            WITH_LOCK(spk_man->cs_desc_man, spk_man->GetWalletDescriptor().descriptor->GetPubKeys(desc_pubkeys, desc_xpubs));
13+            if (desc_xpubs.size() == 1 && desc_pubkeys.size() == 0) {
14+                op_xpub_pubkey = desc_xpubs.begin()->pubkey;
15+            }
16+        }
17+
18         // Get unencrypted keys
19         prefix = PrefixStream(DBKeys::WALLETDESCRIPTORKEY, id);
20         LoadResult key_res = LoadRecords(pwallet, batch, DBKeys::WALLETDESCRIPTORKEY, prefix,
21-            [&id, &spk_man, &desc_keys] (CWallet* pwallet, DataStream& key, CDataStream& value, std::string& strErr) {
22+            [&id, &spk_man, &desc_keys, &op_xpub_pubkey] (CWallet* pwallet, DataStream& key, CDataStream& value, std::string& strErr) {
23             uint256 desc_id;
24             CPubKey pubkey;
25             key >> desc_id;
26@@ -936,7 +947,10 @@
27                 return DBErrors::CORRUPT;
28             }
29             spk_man->AddKey(pubkey.GetID(), privkey);
30-            desc_keys.insert(std::make_pair(std::make_pair(desc_id, pubkey.GetID()), privkey));
31+
32+            if (op_xpub_pubkey && op_xpub_pubkey == pubkey) {
33+                desc_keys.insert(std::make_pair(std::make_pair(desc_id, pubkey.GetID()), privkey));
34+            }
35             return DBErrors::LOAD_OK;
36         });
37         result = std::max(result, key_res.m_result);

furszy commented at 12:36 pm on August 1, 2023:

Or.. another possible patch to not mix the keys reading code with the upgrade code could be to read the xpub root key alone at the end of the spkm loading process:

 0diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
 1--- a/src/wallet/walletdb.cpp	(revision 7f44e83bb544f1e5c398b87c8eb7870eed3ed1ba)
 2+++ b/src/wallet/walletdb.cpp	(date 1690893250659)
 3@@ -966,6 +966,31 @@
 4         result = std::max(result, ckey_res.m_result);
 5         num_ckeys = ckey_res.m_records;
 6 
 7+        // For upgrade purposes, obtain root xpub privkey (if exist)
 8+        CKey root_key;
 9+        {
10+            std::set<CPubKey> desc_pubkeys;
11+            std::set<CExtPubKey> desc_xpubs;
12+            WITH_LOCK(spk_man->cs_desc_man, spk_man->GetWalletDescriptor().descriptor->GetPubKeys(desc_pubkeys, desc_xpubs));
13+            if (desc_xpubs.size() == 1 && desc_pubkeys.size() == 0) {
14+                const auto& pubkey = desc_xpubs.begin()->pubkey;
15+                std::vector<unsigned char> val;
16+                if (!batch.Read(std::make_pair(DBKeys::WALLETDESCRIPTORKEY, std::make_pair(spk_man->GetID(), pubkey)), val)) {
17+                    strErr = "Error reading wallet database: WALLETDESCRIPTORKEY corrupt";
18+                    return DBErrors::CORRUPT;
19+                }
20+
21+                DataStream stream(val);
22+                CPrivKey privkey;
23+                stream >> privkey;
24+                if (!root_key.Load(privkey, pubkey, false)) {
25+                    strErr = "Error reading wallet database: CPrivKey corrupt";
26+                    return DBErrors::CORRUPT;
27+                }
28+                // todo: do something with the root_key..
29+            }
30+        }
31+
32         return result;
33     });
34     result = std::max(result, desc_res.m_result);

achow101 commented at 4:28 pm on November 3, 2023:

I don’t think this is a substantially useful optimization, and it still results in mixing upgrading code with the loading which I would rather not do.

furszy commented at 8:07 pm on November 7, 2023:

I don’t think this is a substantially useful optimization, and it still results in mixing upgrading code with the loading which I would rather not do.

As far as I remember, the last patch I shared doesn’t mixes the upgrading code with the loading code. It loads only the descriptor root key alone at the end of the spkm loading process so we can avoid loading all descriptors keys into a vector, just to obtain the private keys related to each descriptor xpub later in the upgrade process.

In other words, the difference is on a process who loads all keys in ram vs a process who loads only one key per descriptor.

That being said, my memory could be failing.. so np if you don’t want to tackle it here. I could read what I did and revive it in a follow-up if it worth it.

furszy commented at 8:29 pm on July 31, 2023: member

Just started 🚜 , left few comments.

in src/wallet/rpc/wallet.cpp:835 in 87abceeab4 outdated

827+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
828+        {
829+            const std::shared_ptr<CWallet> pwallet = GetWalletForJSONRPCRequest(request);
830+            if (!pwallet) return NullUniValue;
831+
832+            if (!pwallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {

furszy commented at 12:43 pm on August 1, 2023:

In 87abceea:

Maybe better WALLET_FLAG_GLOBAL_HD_KEY?

achow101 commented at 5:57 pm on September 11, 2023:

Added an additional check for that.

DrahtBot added the label Needs rebase on Sep 6, 2023

achow101 force-pushed on Sep 7, 2023

DrahtBot removed the label Needs rebase on Sep 7, 2023

Sjors commented at 7:18 pm on September 7, 2023: member

Light re-utACK 39ba99e7796bec967ce1ec8f5661723bb5d75fe6

fanquake requested review from darosior on Sep 8, 2023

in src/wallet/walletdb.cpp:311 in b88337daba outdated

306+    std::vector<unsigned char> xpub(BIP32_EXTKEY_SIZE);
307+    extkey.Neuter().Encode(xpub.data());
308+
309+    CPrivKey privkey = extkey.key.GetPrivKey();
310+
311+    return WriteIC(std::make_pair(DBKeys::HDKEY, xpub), std::make_pair(privkey, PrivKeyChecksum(privkey, xpub)), false);

furszy commented at 1:26 pm on September 11, 2023:

I know that this is a limitation of our current db model. But, the amount of db records containing this private key is, at least, unorthodox.

We are writing it 9 times under different db keys; one for the HDKey, then one time per descriptor.

BenWestgate commented at 4:20 pm on September 11, 2023:

I do wonder why the default wallet stores the master extended private key in the private descriptor even though the key itself such as what you see in listdecriptors (not true) is a private child derivation. It would be helpful for privacy if it did not do that. Ie: being able to spend one BIP32 account, allows being able to spend them all in the current default descriptors.

But if the master extended private key is going to be stored anyways now (does it need to be?) then no gain is possible. Seems like just the public masterkey could be stored or just the child extended public key for multisig.

Sjors commented at 8:14 am on October 5, 2023:

@furszy it would be better to address that now than to introduce yet another migration later. @BenWestgate since unlocking a wallet decrypts everything, I don’t think that matters. We could however make export safer by not including the root in the RPC output (and have some other way to obtain the seed), but that’s unrelated to this PR.

in src/wallet/walletdb.cpp:985 in 7bbe3b48c3 outdated

981@@ -982,7 +982,7 @@ static DBErrors LoadDescriptorWalletRecords(CWallet* pwallet, DatabaseBatch& bat
982             err = "Error reading wallet database: CExtPubKey corrupt";
983             return DBErrors::CORRUPT;
984         }
985-        // TODO: Load extpub into the wallet
986+        pwallet->LoadActiveHDKey(extpub);

furszy commented at 1:41 pm on September 11, 2023:

In 7bbe3b48:

Misses to treat the boolean returned from the function (if not needed, the new function shouldn’t return anything).

achow101 commented at 6:00 pm on September 11, 2023:

Done

in src/wallet/walletdb.cpp:1018 in 7bbe3b48c3 outdated

1014@@ -1015,7 +1015,7 @@ static DBErrors LoadDescriptorWalletRecords(CWallet* pwallet, DatabaseBatch& bat
1015             err = "Error reading wallet database: CPrivKey corrupt";
1016             return DBErrors::CORRUPT;
1017         }
1018-        // TODO: Load xprv into the wallet
1019+        pwallet->LoadHDKey(extpub, pkey);

furszy commented at 1:43 pm on September 11, 2023:

In 7bbe3b48:

Same as above; misses to treat the boolean returned from the function (if not needed, the new function shouldn’t return anything).

Same for the LoadHDCryptedKey call.

in src/wallet/wallet.cpp:3585 in 079c74ed40 outdated

3577@@ -3578,6 +3578,12 @@ void CWallet::SetupDescriptorScriptPubKeyMans()
3578         CExtKey master_key;
3579         master_key.SetSeed(seed_key);
3580 
3581+        // Store the master key as our active hd key
3582+        if (!AddHDKey(master_key)) {
3583+            throw std::runtime_error(std::string(__func__) + ": Unable to add new random HD key");
3584+        }
3585+        SetActiveHDKey(master_key.Neuter());

furszy commented at 1:47 pm on September 11, 2023:

In 079c74ed:

This also misses to treat the boolean returned from the function (if not needed, the new function shouldn’t return anything).

in src/wallet/wallet.cpp:4278 in 079c74ed40 outdated

4270@@ -4265,6 +4271,14 @@ bool CWallet::LoadActiveHDKey(const CExtPubKey& xpub)
4271     return true;
4272 }
4273 
4274+bool CWallet::SetActiveHDKey(const CExtPubKey& xpub)
4275+{
4276+    AssertLockHeld(cs_wallet);
4277+    Assume(m_hd_keys.count(xpub) + m_hd_crypted_keys.count(xpub) == 1);
4278+    LoadActiveHDKey(xpub);

furszy commented at 1:49 pm on September 11, 2023:

In 079c74ed:

Should either treat LoadActiveHDKey() bool or remove the return value from the function.

achow101 commented at 6:00 pm on September 11, 2023:

Done

furszy commented at 2:15 pm on September 11, 2023: member

I’m not seeing any reason for keeping the HD keys in memory. Their only use case is the RPC getxpub command. Which shouldn’t be frequent at all, and could easily look them up in db on-demand.

I was thinking on doing it on a follow-up to not make you rework the PR but.. this should simplify the changes proposed here largely.

achow101 force-pushed on Sep 11, 2023

S3RK commented at 7:33 am on September 12, 2023: contributor

I’m not seeing any reason for keeping the HD keys in memory. Their only use case is the RPC getxpub command. Which shouldn’t be frequent at all, and could easily look them up in db on-demand.

Do you mean to only keep in memory and not store them in DB?

This approach won’t allow to create a wallet with just HD key and then generate descriptors from it.

furszy commented at 1:04 pm on September 12, 2023: member

I’m not seeing any reason for keeping the HD keys in memory. Their only use case is the RPC getxpub command. Which shouldn’t be frequent at all, and could easily look them up in db on-demand.

Do you mean to only keep in memory and not store them in DB?

This approach won’t allow to create a wallet with just HD key and then generate descriptors from it.

No, the other way around. Only store the records in DB and not in memory.

The hd keys only usage is the RPC getxpub command. Which shouldn’t be frequent at all. We could easily look up the requested xpub and the matching key in DB on-demand.

DrahtBot added the label Needs rebase on Oct 3, 2023

achow101 force-pushed on Oct 3, 2023

DrahtBot removed the label Needs rebase on Oct 3, 2023

in src/wallet/walletdb.cpp:326 in 37527c9c69 outdated

321+
322+    const auto key = std::make_pair(DBKeys::HDCKEY, xpub);
323+    if (!WriteIC(key, std::make_pair(crypted_key, checksum), false)) {
324+        return false;
325+    }
326+    EraseIC(std::make_pair(DBKeys::HDKEY, xpub));

Sjors commented at 8:06 am on October 5, 2023:

37527c9c6930dd86d5d012e94d2e08d6a207d240 could add a comment here that we’re deleting the unencrypted key. Or make the intention more readable:

0const auto db_key_plain_text = std::make_pair(DBKeys::HDKEY, xpub);
1const auto db_key_encrypted = std::make_pair(DBKeys::HDCKEY, xpub);
2if (!WriteIC(db_key_plain_text, std::make_pair(crypted_key, checksum), false)) {
3    return false;
4}
5EraseIC(db_key_unencrypted);

I find it a worrying that no test caught the fact that we weren’t deleting this in an earlier version of the PR (b88337daba). Can we introduce a test that looks for unencrypted key material e.g. by just checking every record?

achow101 commented at 8:54 pm on October 5, 2023:

Added a test that should catch that in the future.

Sjors commented at 8:27 am on October 5, 2023: member

Rebase and changes for 5d1e7a6cc81d021e49581b2e7f52fd0e54220aa2 mostly look alright, but I’ll need to go through everything again probably. Waiting for some conceptial discussion on two things: @furszy wrote:

I’m not seeing any reason for keeping the HD keys in memory.

What do you think of this?

And the second is whether we really want to store these private keys 9 times, see inline comment.

5d1e7a6cc81d021e49581b2e7f52fd0e54220aa2 introduces a test failure in wallet_multiwallet.py —descriptor, where it finds the wallet is corrupted on load.

DrahtBot added the label Needs rebase on Oct 5, 2023

achow101 force-pushed on Oct 5, 2023

achow101 commented at 8:58 pm on October 5, 2023: member

I’m not seeing any reason for keeping the HD keys in memory.

What do you think of this?

It follows the existing pattern already in use for all other wallet data structures. Loading things on demand from the database is orthogonal to this PR, and when implementing that, it should be done almost all at once, not piecemeal. I’m concerned that a piecemeal approach would result in developers missing that some data is in fact stored on disk and could be read, which could lead to further duplication and confusion when working on the wallet.

And the second is whether we really want to store these private keys 9 times, see inline comment.

It’s done for backwards compatibility reasons. Without storing the master key for each descriptor, a wallet would not be able to be opened in an older version of the software.

achow101 force-pushed on Oct 5, 2023

DrahtBot added the label CI failed on Oct 5, 2023

DrahtBot removed the label Needs rebase on Oct 5, 2023

in test/functional/wallet_encryption.py:64 in 474eceb8a5 outdated

59+        ckey_hex = b"ckey".hex()
60+        # Hex for records to skip
61+        skip_records = [b"mkey".hex(), b"activehdkey".hex(), b"defaultkey".hex(), b"keymeta".hex()]
62+        print(f"key_hex {key_hex}")
63+        print(f"ckey_hex {ckey_hex}")
64+        with open(dump_file, "r") as f:

Sjors commented at 7:22 am on October 6, 2023:

474eceb8a5396596cf283832b6f742dada1a5f8e: lint seems upset out this line

On my Ubuntu machine I get this from the linter (Python 3.8.17):

0Python's open(...) seems to be used to open text files without explicitly specifying encoding='utf8':
1
2test/functional/wallet_encryption.py:        with open(dump_file, "r") as f:

achow101 commented at 3:21 pm on October 6, 2023:

Fixed

in test/functional/wallet_encryption.py:62 in 474eceb8a5 outdated

57+        # Look for records that contain the hex for 'key' but not 'ckey'
58+        key_hex = b"key".hex()
59+        ckey_hex = b"ckey".hex()
60+        # Hex for records to skip
61+        skip_records = [b"mkey".hex(), b"activehdkey".hex(), b"defaultkey".hex(), b"keymeta".hex()]
62+        print(f"key_hex {key_hex}")

Sjors commented at 7:24 am on October 6, 2023:

474eceb8a5396596cf283832b6f742dada1a5f8e: these print statements are a leftover?

achow101 commented at 3:21 pm on October 6, 2023:

Removed

Sjors commented at 9:05 am on October 6, 2023: member

474eceb8a5396596cf283832b6f742dada1a5f8e is almost good to go…

I ran the test suite against every commit, after rebasing on master to include #28027. Probably worth a rebase so CI runs a better backwards compatibility test.

d4c96c9fa4e7efa181e50d46a1a962c6e2095279 is still causing the tests to fail as I mentioned above. The next commit passes again. If it’s hard to avoid that some intermediate commits cause the tests to break, just mention it in the commit message.

achow101 force-pushed on Oct 6, 2023

achow101 commented at 3:21 pm on October 6, 2023: member

d4c96c9 is still causing the tests to fail as I mentioned above. The next commit passes again. If it’s hard to avoid that some intermediate commits cause the tests to break, just mention it in the commit message.

Swapped the two commits so everything works now.

DrahtBot removed the label CI failed on Oct 6, 2023

Sjors commented at 5:33 pm on October 6, 2023: member

utACK e986d5a15174c8abf6979f1c03a04913e7c2e90f

DrahtBot requested review from furszy on Oct 6, 2023

DrahtBot requested review from w0xlt on Oct 6, 2023

S3RK commented at 2:29 pm on October 10, 2023: contributor

Just FYI. I’m still very much interested in this PR as I believe it unblocks many improvements for the wallet. We likely need more reviews anyway, so I’ll wait for @furszy to do a closer review, before re-reviewing again.

DrahtBot removed review request from w0xlt on Oct 10, 2023

DrahtBot requested review from w0xlt on Oct 10, 2023

Sjors commented at 7:37 am on October 11, 2023: member

@josibake may also like this PR since it’s a prerequisite for adding silent payment spend/watch keys to an existing wallet.

DrahtBot removed review request from w0xlt on Oct 11, 2023

DrahtBot requested review from w0xlt on Oct 11, 2023

DrahtBot removed review request from w0xlt on Oct 12, 2023

DrahtBot requested review from w0xlt on Oct 12, 2023

DrahtBot added the label CI failed on Oct 17, 2023

cacrowley approved

DrahtBot removed review request from w0xlt on Oct 17, 2023

DrahtBot requested review from w0xlt on Oct 17, 2023

in src/wallet/wallet.cpp:4528 in 75ded425ea outdated

4377+std::optional<CExtKey> CWallet::GetHDKey(const CExtPubKey& xpub)
4378+{
4379+    AssertLockHeld(cs_wallet);
4380+
4381+    CKey key;
4382+    if (IsCrypted()) {

furszy commented at 9:20 pm on October 18, 2023:

In 75ded425ea:

nit: AddHDKey() uses HasEncryptionKeys(), and GetHDKey() uses IsCrypted(). Let’s pick one of them and stick to it.

achow101 commented at 4:03 pm on October 24, 2023:

Changed AddHDKey() to use IsCrypted()

in src/wallet/wallet.cpp:4031 in 283fe566d6 outdated

4050             SetupDescriptorScriptPubKeyMans(data.master_key);
4051+
4052+            // Store the master key as our active hd key
4053+            if (!AddHDKey(data.master_key)) {
4054+                throw std::runtime_error(std::string(__func__) + ": Unable to add original seed's HD key");
4055+            }

furszy commented at 12:59 pm on October 19, 2023:

In 283fe566d:

As this is part of the migration process, better to error out instead of throwing an exception. The difference is: (1) exception means crashing, (2) error means the migration process will automatically recover from the inconsistent state. Cleaning up the created wallets and restoring the pre-migration legacy wallet.

0// Store the master key as our active hd key
1if (!AddHDKey(data.master_key)) {
2    error += _("Error: Unable to add seed's HD key");
3    return false;
4}

achow101 commented at 4:03 pm on October 24, 2023:

Done as suggested

in src/wallet/wallet.cpp:4055 in 283fe566d6 outdated

4051+
4052+            // Store the master key as our active hd key
4053+            if (!AddHDKey(data.master_key)) {
4054+                throw std::runtime_error(std::string(__func__) + ": Unable to add original seed's HD key");
4055+            }
4056+            SetActiveHDKey(data.master_key.Neuter());

furszy commented at 1:00 pm on October 19, 2023:

In 283fe566d:

Should check the error here.

0if (!SetActiveHDKey(data.master_key.Neuter())) {
1    error += _("Error: Unable to set active HD key");
2    return false;
3}

achow101 commented at 4:03 pm on October 24, 2023:

Done as suggested

in test/functional/wallet_encryption.py:54 in e986d5a151 outdated

47@@ -46,6 +48,25 @@ def run_test(self):
48         assert_raises_rpc_error(-8, "passphrase cannot be empty", self.nodes[0].walletpassphrase, '', 1)
49         assert_raises_rpc_error(-8, "passphrase cannot be empty", self.nodes[0].walletpassphrasechange, '', 'ff')
50 
51+        # Make sure there are no unencrypted key records
52+        # Use the wallet tool's dump mechanism to get all of the records to inspect them.
53+        self.nodes[0].unloadwallet(self.default_wallet_name)
54+        dump_file = os.path.join(self.nodes[0].datadir, "wallet.dump")

furszy commented at 1:24 pm on October 19, 2023:

I’m unable to run this locally. Python 3.11. And seems that the CI is also complaining about it.

0dump_file = os.path.join(self.nodes[0].datadir, "wallet.dump")
1                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2File "<frozen posixpath>", line 76, in join
3TypeError: expected str, bytes or os.PathLike object, not AuthServiceProxyWrapper

furszy commented at 2:30 pm on October 19, 2023:

ok, found the problem. This should use datadir_path not datadir (which is a AuthServiceProxyWrapper):

0self.nodes[0].datadir_path / "wallet.dump"

achow101 commented at 4:03 pm on October 24, 2023:

Fixed

furszy commented at 1:47 pm on October 19, 2023: member

Reviewed everything except the encryption / key rotation aspect. Left few comments, nothing blocking so far. Almost finished it.

Just a future work comment; We can simplify the upgrade code by only caching the extpub root keys, and not all descriptors keys, when we load the records from db.

DrahtBot removed review request from w0xlt on Oct 19, 2023

DrahtBot requested review from w0xlt on Oct 19, 2023

DrahtBot requested review from furszy on Oct 19, 2023

DrahtBot removed review request from w0xlt on Oct 19, 2023

DrahtBot requested review from w0xlt on Oct 19, 2023

DrahtBot removed review request from w0xlt on Oct 19, 2023

DrahtBot requested review from w0xlt on Oct 19, 2023

in test/functional/wallet_encryption.py:55 in e986d5a151 outdated

47@@ -46,6 +48,25 @@ def run_test(self):
48         assert_raises_rpc_error(-8, "passphrase cannot be empty", self.nodes[0].walletpassphrase, '', 1)
49         assert_raises_rpc_error(-8, "passphrase cannot be empty", self.nodes[0].walletpassphrasechange, '', 'ff')
50 
51+        # Make sure there are no unencrypted key records
52+        # Use the wallet tool's dump mechanism to get all of the records to inspect them.
53+        self.nodes[0].unloadwallet(self.default_wallet_name)
54+        dump_file = os.path.join(self.nodes[0].datadir, "wallet.dump")
55+        subprocess.check_call([self.options.bitcoinwallet, f"-datadir={self.nodes[0].datadir}", f"-chain={self.chain}", f"-wallet={self.default_wallet_name}", f"-dumpfile={dump_file}", "dump"], stdout=subprocess.DEVNULL)

furszy commented at 2:31 pm on October 19, 2023:

In e986d5a15:

Same here, need to change f"-datadir={self.nodes[0].datadir}" for f"-datadir={self.nodes[0].datadir_path}".

0        subprocess.check_call([self.options.bitcoinwallet, f"-datadir={self.nodes[0].datadir_path}", f"-chain={self.chain}", f"-wallet={self.default_wallet_name}", f"-dumpfile={dump_file}", "dump"], stdout=subprocess.DEVNULL)

achow101 commented at 4:03 pm on October 24, 2023:

Fixed

DrahtBot removed review request from w0xlt on Oct 19, 2023

DrahtBot requested review from furszy on Oct 19, 2023

DrahtBot requested review from w0xlt on Oct 19, 2023

DrahtBot removed review request from w0xlt on Oct 19, 2023

DrahtBot requested review from w0xlt on Oct 19, 2023

achow101 force-pushed on Oct 24, 2023

DrahtBot removed the label CI failed on Oct 25, 2023

Sjors commented at 6:39 am on October 26, 2023: member

re-utACK dfb58e499e053bf24425bb23cf455d280d2b2c3a

DrahtBot removed review request from w0xlt on Oct 26, 2023

DrahtBot requested review from w0xlt on Oct 26, 2023

in test/functional/wallet_getxpub.py:48 in 7bc7c7cc8a outdated

43+        self.nodes[0].encryptwallet("pass")
44+        assert_raises_rpc_error(-13, "Error: Please enter the wallet passphrase with walletpassphrase first", self.nodes[0].getxpub, True)
45+        self.nodes[0].walletpassphrase("pass", 10)
46+        xpub_info = self.nodes[0].getxpub(True)
47+        assert xpub_info["xpub"] != xpub
48+        assert xpub_info["xprv"] != xprv

furszy commented at 3:31 pm on November 3, 2023:

nit: Could also check that all active descriptors have this new xprv.

achow101 commented at 5:10 pm on November 3, 2023:

Done

in test/functional/wallet_backwards_compatibility.py:333 in 97de2a4af6 outdated

366+                if encrypt:
367+                    wallet_prev.encryptwallet("pass")
368+                    #if self.major_version_less_than(node, 18):
369+                    #    # 0.18 stopped the shutdown on encrypt behavior
370+                    #    node.wait_until_stopped()
371+                    #    self.start_node(node.index)

furszy commented at 3:46 pm on November 3, 2023:

In 97de2a4af: A note explaining why this is not needed would be nice.

achow101 commented at 5:11 pm on November 3, 2023:

This ended up being unnecessary and I just forgot to remove it.

furszy commented at 4:02 pm on November 3, 2023: member

Code review ACK dfb58e4

Besides the code improvement I mentioned in the past (link), it would also be nice to test the scenario where the process does not find the expected number of active descriptors. This should result in the wallet having no active xpub key.

DrahtBot removed review request from w0xlt on Nov 3, 2023

DrahtBot requested review from w0xlt on Nov 3, 2023

DrahtBot removed review request from w0xlt on Nov 3, 2023

DrahtBot requested review from w0xlt on Nov 3, 2023

DrahtBot removed review request from w0xlt on Nov 3, 2023

DrahtBot requested review from w0xlt on Nov 3, 2023

DrahtBot removed review request from w0xlt on Nov 3, 2023

DrahtBot requested review from w0xlt on Nov 3, 2023

cacrowley approved

DrahtBot removed review request from w0xlt on Nov 3, 2023

DrahtBot requested review from w0xlt on Nov 3, 2023

DrahtBot removed review request from w0xlt on Nov 3, 2023

DrahtBot requested review from w0xlt on Nov 3, 2023

achow101 force-pushed on Nov 3, 2023

achow101 commented at 5:43 pm on November 3, 2023: member

it would also be nice to test the scenario where the process does not find the expected number of active descriptors. This should result in the wallet having no active xpub key.

Added to a test that imports a few descriptors whose xpub won’t be chosen for active xpub.

DrahtBot added the label CI failed on Nov 3, 2023

maflcko commented at 7:33 am on November 4, 2023: member

0test/functional/wallet_backwards_compatibility.py:29:1: F401 'test_framework.wallet_util.WalletUnlock' imported but unused

achow101 force-pushed on Nov 4, 2023

DrahtBot removed the label CI failed on Nov 4, 2023

S3RK commented at 2:20 pm on November 5, 2023: contributor

Nice, I’m going to re-review this once I’m done with 25273

Sjors commented at 1:43 am on November 6, 2023: member

re-utACK b1b12d1b88492fe66b26243760689c7f040c67c2

DrahtBot removed review request from w0xlt on Nov 6, 2023

DrahtBot requested review from w0xlt on Nov 6, 2023

DrahtBot requested review from furszy on Nov 6, 2023

in src/wallet/walletdb.cpp:73 in 8a24755732 outdated

67@@ -68,6 +68,17 @@ const std::unordered_set<std::string> LEGACY_TYPES{CRYPTED_KEY, CSCRIPT, DEFAULT
68 // WalletBatch
69 //
70 
71+template <typename P>
72+static uint256 PrivKeyChecksum(const CPrivKey& privkey, const P& pubkey)

S3RK commented at 8:23 am on November 7, 2023:

in 8a2475573270b7bc0a7d99cd94d9908cd48bd6bd

nit: The code would be easier to understand if we drop template and just use const CPubKey&, it’s trivial to get the right type on the caller side.

achow101 commented at 11:31 pm on November 13, 2023:

No, the point is so that when we are writing an HD key, this checksum covers the entire xpub too, not just the pubkey part.

ryanofsky commented at 7:56 pm on December 5, 2023:

In commit “walletdb: Refactor privkey checksum calc” (0a59e878006763817d53017e50d268b32868bd8d)

No, the point is so that when we are writing an HD key, this checksum covers the entire xpub too, not just the pubkey part.

I don’t understand this, since the following change compiles:

 0--- a/src/wallet/walletdb.cpp
 1+++ b/src/wallet/walletdb.cpp
 2@@ -69,8 +69,7 @@ const std::unordered_set<std::string> LEGACY_TYPES{CRYPTED_KEY, CSCRIPT, DEFAULT
 3 // WalletBatch
 4 //
 5 
 6-template <typename P>
 7-static uint256 PrivKeyChecksum(const CPrivKey& privkey, const P& pubkey)
 8+static uint256 PrivKeyChecksum(const CPrivKey& privkey, const CPubKey& pubkey)
 9 {
10     // hash pubkey/privkey to accelerate wallet load
11     std::vector<unsigned char> to_hash;

Maybe you have changes in a later PR depending on this this being a template? IMO, though, it’d be clearer not to make it this template until its needed, and probably add a comment explaining it at that point.

achow101 commented at 11:39 pm on December 5, 2023:

It was previously needed in a later commit when xpubs with private keys were being written, but it is no longer needed. I’ve ended up dropping this commit from this PR.

in test/functional/wallet_backwards_compatibility.py:355 in 4ad97b519e outdated

388+                        },
389+                        {
390+                            "desc": descsum_create("wpkh(tprv8ZgxMBicQKsPeNLUGrbv3b7qhUk1LQJZAGMuk9gVuKh9sd4BWGp1eMsehUni6qGb8bjkdwBxCbgNGdh2bYGACK5C5dRTaif9KBKGVnSezxV/1/*"),
391+                            "timestamp": int(time.time()) + 70,
392+                            "active": True,
393+                            "internal": False,

furszy commented at 7:08 pm on November 7, 2023:

In 4ad97b519e7: As the default value for “internal” is false, I infer you wanted internal=true?.

Same for the other one that is below.

achow101 commented at 8:44 pm on November 7, 2023:

Ah, good catch. Fixed

in src/wallet/wallet.cpp:584 in b1b12d1b88 outdated

576+    if (!IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS) ||
577+        IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) ||
578+        IsWalletFlagSet(WALLET_FLAG_GLOBAL_HD_KEY)
579+        ) {
580+        return;
581+    }

furszy commented at 7:35 pm on November 7, 2023:

In 3e250b10:

There is a discrepancy here. The upgrade mechanism is skipped for private key disabled wallets without set the WALLET_FLAG_GLOBAL_HD_KEY. This differs from what is being done in the wallet creation and the migration processes where we set this flag for all types (only requirement is WALLET_FLAG_DESCRIPTORS).

Is this intended?

achow101 commented at 8:46 pm on November 7, 2023:

I don’t think it particularly matters since a wallet without private keys cannot have a global hd key. However I have changed it so that it is set only if private keys are enabled.

furszy commented at 7:42 pm on November 7, 2023: member

Reviewed, left two comments.

it would also be nice to test the scenario where the process does not find the expected number of active descriptors. This should result in the wallet having no active xpub key.

Added to a test that imports a few descriptors whose xpub won’t be chosen for active xpub.

Cool. Future thing; it would be nice to improve the backwards compatibility test code organization. The more we add there, the harder is to follow. Other test cases coming to my mind are related to blank and watch-only wallets (no need to add them here):

A blank created wallet with no descriptors should end up without an xpub after upgrade, with the global HD flag set.
A blank created wallet with some descriptors should end up with no active xpub after upgrade, with the global HD flag set.
Same two tests for private key disabled wallets.
A watch-only wallet with 8 descriptors in the same form as the default wallet should not have the global HD flag set.

DrahtBot removed review request from w0xlt on Nov 7, 2023

DrahtBot requested review from w0xlt on Nov 7, 2023

DrahtBot requested review from furszy on Nov 7, 2023

DrahtBot removed review request from w0xlt on Nov 7, 2023

DrahtBot requested review from w0xlt on Nov 7, 2023

DrahtBot removed review request from w0xlt on Nov 7, 2023

DrahtBot requested review from w0xlt on Nov 7, 2023

DrahtBot removed review request from w0xlt on Nov 7, 2023

DrahtBot requested review from w0xlt on Nov 7, 2023

achow101 force-pushed on Nov 7, 2023

in src/wallet/walletdb.cpp:308 in 95153f0152 outdated

330+bool WalletBatch::WriteActiveHDKey(const CExtPubKey& extpub)
331+{
332+    std::vector<unsigned char> xpub(BIP32_EXTKEY_SIZE);
333+    extpub.Encode(xpub.data());
334+
335+    return WriteIC(DBKeys::ACTIVEHDKEY, xpub, true);

S3RK commented at 8:31 am on November 8, 2023:

in 95153f0152c057101e894e2ca73aee78be267b33

nit: Should we just add serialization methods to CExtPubKey? Instead of explicitly calling Encode()

achow101 commented at 11:38 pm on November 13, 2023:

I don’t think that’s helpful here since we need to get a copy of the serialized xpub in order to calculate the checksum.

Sjors commented at 1:42 am on November 9, 2023: member

re-utACK b31d9bf1ae764421c8937dd126ad9e29845ebc5e

DrahtBot removed review request from w0xlt on Nov 9, 2023

DrahtBot requested review from w0xlt on Nov 9, 2023

in src/wallet/wallet.cpp:4222 in 5809c90fee outdated

4248@@ -4239,7 +4249,7 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error,
4249         empty_context.args = context.args;
4250 
4251         // Make the wallets
4252-        options.create_flags = WALLET_FLAG_DISABLE_PRIVATE_KEYS | WALLET_FLAG_BLANK_WALLET | WALLET_FLAG_DESCRIPTORS;
4253+        options.create_flags = WALLET_FLAG_DISABLE_PRIVATE_KEYS | WALLET_FLAG_BLANK_WALLET | WALLET_FLAG_DESCRIPTORS | WALLET_FLAG_GLOBAL_HD_KEY;

furszy commented at 2:39 pm on November 9, 2023:

In 5809c90fee6:

Need to remove this change. Otherwise the migrated watch-only and solvables wallets will have the global hd key flag set while newly created private key disabled wallets will not.

This is because the wallets are created calling CWallet::Create() here, which takes this variable and sets the wallet flags calling CWallet::InitWalletFlags().

It would be nice to add a test verifying that we do not create a private key disabled wallet with the global hd key flag set.

S3RK commented at 7:59 am on November 14, 2023:

@achow101 IIUC you addressed this as well

achow101 commented at 2:21 pm on November 16, 2023:

Yes.

DrahtBot removed review request from w0xlt on Nov 9, 2023

DrahtBot requested review from w0xlt on Nov 9, 2023

DrahtBot requested review from furszy on Nov 9, 2023

DrahtBot removed review request from w0xlt on Nov 9, 2023

DrahtBot requested review from w0xlt on Nov 9, 2023

in src/wallet/wallet.cpp:862 in 96365a0b18 outdated

857+                // die and let the user reload the unencrypted wallet.
858+                assert(false);
859+            }
860+
861+            m_hd_crypted_keys[xpub] = crypted_secret;
862+            encrypted_batch->WriteHDCryptedKey(xpub, crypted_secret);

S3RK commented at 12:49 pm on November 12, 2023:

in 96365a0b182858d686e695a153af232c7b3740c9

check return value

achow101 commented at 0:10 am on November 14, 2023:

Done

in src/wallet/wallet.cpp:4507 in 8276bbeb04 outdated

4365@@ -4358,6 +4366,14 @@ bool CWallet::LoadActiveHDKey(const CExtPubKey& xpub)
4366     return true;
4367 }
4368 
4369+bool CWallet::SetActiveHDKey(const CExtPubKey& xpub)
4370+{
4371+    AssertLockHeld(cs_wallet);
4372+    Assume(m_hd_keys.count(xpub) + m_hd_crypted_keys.count(xpub) == 1);

S3RK commented at 12:57 pm on November 12, 2023:

Checking my understanding. This assume verifies that we loaded the key we want to set as active. Correct?

achow101 commented at 11:25 pm on November 13, 2023:

Yes.

in src/wallet/wallet.cpp:4479 in e693bd0261 outdated

4373@@ -4374,6 +4374,15 @@ bool CWallet::SetActiveHDKey(const CExtPubKey& xpub)
4374     return WalletBatch(GetDatabase()).WriteActiveHDKey(xpub);
4375 }
4376 
4377+std::optional<CExtPubKey> CWallet::GetActiveHDPubKey()
4378+{
4379+    AssertLockHeld(cs_wallet);
4380+    if (m_xpub.pubkey.IsValid()) {

S3RK commented at 1:13 pm on November 12, 2023:

in e693bd02610df20f73c008b03c739da5d69fcf76

nit: If the key is not valid - it should be an error and it should be detected in LoadActiveHDKey()

achow101 commented at 0:10 am on November 14, 2023:

Added Assume

in src/wallet/wallet.cpp:4526 in e693bd0261 outdated

4373@@ -4374,6 +4374,15 @@ bool CWallet::SetActiveHDKey(const CExtPubKey& xpub)
4374     return WalletBatch(GetDatabase()).WriteActiveHDKey(xpub);
4375 }
4376 
4377+std::optional<CExtPubKey> CWallet::GetActiveHDPubKey()

S3RK commented at 1:16 pm on November 12, 2023:

in e693bd02610df20f73c008b03c739da5d69fcf76

nit: inconsistent naming.

Propose to rename to GetActiveHDKey() analogous to LoadActiveHDKey() and SetActiveHDKey()

achow101 commented at 0:10 am on November 14, 2023:

Done as suggested

in src/wallet/rpc/wallet.cpp:812 in 9900207360 outdated

808@@ -809,6 +809,61 @@ static RPCHelpMan migratewallet()
809     };
810 }
811 
812+RPCHelpMan getxpub()

S3RK commented at 1:24 pm on November 12, 2023:

in 9900207360a2852041ceed260c3430e92ff860c5

The RPC can return xprv as well as xpub. We should find a better name as it becomes our API and users will depend on it.

How about gethdkey?

achow101 commented at 0:10 am on November 14, 2023:

Renamed as suggested

in src/wallet/rpc/wallet.cpp:816 in 9900207360 outdated

808@@ -809,6 +809,61 @@ static RPCHelpMan migratewallet()
809     };
810 }
811 
812+RPCHelpMan getxpub()
813+{
814+    return RPCHelpMan{"getxpub",
815+                "Returns the xpub most recently used to generate descriptors for this descriptor wallet. "
816+                "Not entirely useful right now as it returns the xpub of the root, and there are "

S3RK commented at 1:25 pm on November 12, 2023:

in 9900207360a2852041ceed260c3430e92ff860c5

The comment is a bit outdated it seems, since the method can also return xprv.

achow101 commented at 0:10 am on November 14, 2023:

Dropped

in src/wallet/rpc/wallet.cpp:846 in 9900207360 outdated

841+            LOCK(pwallet->cs_wallet);
842+
843+            UniValue obj(UniValue::VOBJ);
844+            std::optional<CExtPubKey> extpub = pwallet->GetActiveHDPubKey();
845+            if (!extpub) {
846+                throw JSONRPCError(RPC_WALLET_ERROR, "This wallet does not have an active xpub");

S3RK commented at 1:29 pm on November 12, 2023:

in 9900207360a2852041ceed260c3430e92ff860c5

nit: “The wallet does not have an active HD key”

“active xpub” could mean xpub of active descriptor

achow101 commented at 0:10 am on November 14, 2023:

Done

in src/wallet/rpc/wallet.cpp:856 in 9900207360 outdated

851+            bool priv = !request.params[0].isNull() && request.params[0].get_bool();
852+            if (priv) {
853+                EnsureWalletIsUnlocked(*pwallet);
854+                std::optional<CExtKey> extkey = pwallet->GetHDKey(*extpub);
855+                if (!extkey) {
856+                    throw JSONRPCError(RPC_WALLET_ERROR, "Could not find the xprv for active key");

S3RK commented at 1:29 pm on November 12, 2023:

in 9900207360a2852041ceed260c3430e92ff860c5

nit: “Could not find the xprv for active HD key”

achow101 commented at 0:11 am on November 14, 2023:

Done

in src/script/descriptor.cpp:220 in 8fcd9cfb4c outdated

211@@ -212,6 +212,12 @@ struct PubkeyProvider
212 
213     /** Derive a private key, if private data is available in arg. */
214     virtual bool GetPrivKey(int pos, const SigningProvider& arg, CKey& key) const = 0;
215+
216+    /** Return all (extended) public keys for this PubkeyProvider
217+     * param[out] pubkeys Any public keys
218+     * param[out] ext_pubs Any extended public keys
219+     */
220+    virtual void GetRootPubKeys(std::set<CPubKey>& pubkeys, std::set<CExtPubKey>& ext_pubs) const = 0;

S3RK commented at 1:44 pm on November 12, 2023:

In 8fcd9cfb4c1dac680e9c6702b7af9517a578210d

I think PubkeyProvider can hold only 1 (extended) public key. So should we have a different signature for the method? Do I miss something?

achow101 commented at 0:11 am on November 14, 2023:

Good point, changed to two functions GetRootPubKey and GetRootExtPubKey, each returning an optional CPubKey or CExtPubKey, respectively.

S3RK commented at 1:58 pm on November 12, 2023: contributor

re-review up to the migration commit 18dc28f14ee1de561625913d286d68031ab9b3dd

Left some small nits and suggestion.

Should we spend a bit more time discussing the public API? I.e. what to name getxpub method and how we would like to evolve in the future?

Will continue to re-review with the rest of commits soon.

DrahtBot removed review request from w0xlt on Nov 12, 2023

DrahtBot requested review from w0xlt on Nov 12, 2023

DrahtBot requested review from S3RK on Nov 12, 2023

DrahtBot removed review request from w0xlt on Nov 12, 2023

DrahtBot requested review from w0xlt on Nov 12, 2023

DrahtBot removed review request from w0xlt on Nov 12, 2023

DrahtBot requested review from w0xlt on Nov 12, 2023

Sjors commented at 6:18 am on November 13, 2023: member

Should we spend a bit more time discussing the public API? I.e. what to name getxpub method and how we would like to evolve in the future?

I’m keeping #22341 rebased on top of this. Since that’s a breaking API change anyway, I suggest we move discussion there rather than introduce more changes in this PR.

DrahtBot removed review request from w0xlt on Nov 13, 2023

DrahtBot requested review from w0xlt on Nov 13, 2023

DrahtBot removed review request from w0xlt on Nov 13, 2023

DrahtBot requested review from w0xlt on Nov 13, 2023

in src/wallet/wallet.cpp:647 in 18dc28f14e outdated

642+            }
643+        }
644+    }
645+
646+    // For all candidate xpubs, add corresponding private keys if available
647+    for (const auto& [id_pair, key] : desc_keys) {

S3RK commented at 8:33 am on November 13, 2023:

in 18dc28f14ee1de561625913d286d68031ab9b3dd

nit: it could’ve been just std:map<keyid, key> since desc_id is not used

achow101 commented at 2:48 pm on November 16, 2023:

Done

in src/wallet/wallet.cpp:660 in 18dc28f14e outdated

655+        CExtKey extkey(it->second, key);
656+        AddHDKey(extkey);
657+        xpubs.erase(it);
658+    }
659+    WalletBatch batch(GetDatabase());
660+    for (const auto& [id_pair, key_pair] : desc_crypt_keys) {

S3RK commented at 8:34 am on November 13, 2023:

in 18dc28f14ee1de561625913d286d68031ab9b3dd

nit: it could’ve been just std::map<keyid, ckey> since desc_id and pubkey are not used

achow101 commented at 2:48 pm on November 16, 2023:

Done

in test/functional/wallet_getxpub.py:47 in 52697ecc3f outdated

42+        for desc in descs["descriptors"]:
43+            if "range" in desc:
44+                assert xprv in desc["desc"]
45+
46+        wallet.encryptwallet("pass")
47+        assert_raises_rpc_error(-13, "Error: Please enter the wallet passphrase with walletpassphrase first", wallet.getxpub, True)

S3RK commented at 8:44 am on November 13, 2023:

in 52697ecc3fd46fdf3c02eba0d474949a9d05d885

nit: can test that gethdkey(False) succeeds on encrypted and locked wallet

achow101 commented at 2:48 pm on November 16, 2023:

Added a test

in test/functional/wallet_getxpub.py:53 in 52697ecc3f outdated

48+        with WalletUnlock(wallet, "pass"):
49+            xpub_info = wallet.getxpub(True)
50+            assert xpub_info["xpub"] != xpub
51+            assert xpub_info["xprv"] != xprv
52+            for desc in wallet.listdescriptors(True)["descriptors"]:
53+                if desc["active"]:

S3RK commented at 8:45 am on November 13, 2023:

in 52697ecc3fd46fdf3c02eba0d474949a9d05d885

nit: maybe add a comment, that you’re checking key rotation and non-active descriptors should have pre-rotation key

achow101 commented at 2:48 pm on November 16, 2023:

Added some comments.

DrahtBot removed review request from w0xlt on Nov 13, 2023

DrahtBot requested review from w0xlt on Nov 13, 2023

DrahtBot removed review request from w0xlt on Nov 13, 2023

DrahtBot requested review from w0xlt on Nov 13, 2023

DrahtBot removed review request from w0xlt on Nov 13, 2023

DrahtBot requested review from w0xlt on Nov 13, 2023

achow101 force-pushed on Nov 14, 2023

Sjors commented at 6:12 am on November 14, 2023: member

re-utACK e3c292b298e5e6f2ec319fb93ba67fd64a0f4ac1

Also rebased #22341 after the name change.

DrahtBot removed review request from w0xlt on Nov 14, 2023

DrahtBot requested review from w0xlt on Nov 14, 2023

in test/functional/wallet_backwards_compatibility.py:357 in 2657c1295c outdated

350@@ -351,6 +351,13 @@ def run_test(self):
351             descriptor = f"wpkh([{info['hdmasterfingerprint']}{hdkeypath[1:]}]{pubkey})"
352             assert_equal(info["desc"], descsum_create(descriptor))
353 
354+            # Check that descriptor wallets have hd key
355+            if self.options.descriptors:
356+                descs = wallet.listdescriptors(True)
357+                xpub_info = wallet.gethdkey(True)

S3RK commented at 8:20 am on November 16, 2023:

in 2657c1295cb156d6021844edf18b1a4dc5ce7136

nit: can also check that HD key is the same as return by the old version (e.g. by checking fingerprint with hdmasterfingerprint)

achow101 commented at 2:49 pm on November 16, 2023:

Done

in test/functional/wallet_encryption.py:69 in e3c292b298 outdated

64+            for row in f:
65+                k, v = row.split(",")
66+                # Skip 'mkey' records. These aren't private key records.
67+                if any([skip in k for skip in skip_records]):
68+                    continue
69+                assert (key_hex in k and ckey_hex in k) or (key_hex not in k and ckey_hex not in k), "Unexpected unencrypted key record"

S3RK commented at 8:33 am on November 16, 2023:

in e3c292b298e5e6f2ec319fb93ba67fd64a0f4ac1

if key_hex not in k then ckey_hex not in k is always true by definition Therefore key_hex not in k and ckey_hex not in k seems redundant to me.

achow101 commented at 2:49 pm on November 16, 2023:

Simplified this check

in test/functional/wallet_encryption.py:60 in e3c292b298 outdated

55+        dump_file = os.path.join(self.nodes[0].datadir_path, "wallet.dump")
56+        subprocess.check_call([self.options.bitcoinwallet, f"-datadir={self.nodes[0].datadir_path}", f"-chain={self.chain}", f"-wallet={self.default_wallet_name}", f"-dumpfile={dump_file}", "dump"], stdout=subprocess.DEVNULL)
57+        self.nodes[0].loadwallet(self.default_wallet_name)
58+        # Look for records that contain the hex for 'key' but not 'ckey'
59+        key_hex = b"key".hex()
60+        ckey_hex = b"ckey".hex()

S3RK commented at 8:34 am on November 16, 2023:

in e3c292b298e5e6f2ec319fb93ba67fd64a0f4ac1

Relying that all encrypted keys contain ckey seems fragile. Can we maybe list all the encrypted key records?

achow101 commented at 2:50 pm on November 16, 2023:

I think it’s more futureproof to check for ckey since all new records involving encrypted keys have always had that suffix.

S3RK commented at 8:35 am on November 16, 2023: contributor

I finished going through all commits one-by-one. Need a bit more time to check comprehensively how HD flag is set at creation/migration/upgrade and redo my manual tests.

DrahtBot removed review request from w0xlt on Nov 16, 2023

DrahtBot requested review from w0xlt on Nov 16, 2023

DrahtBot requested review from S3RK on Nov 16, 2023

DrahtBot removed review request from w0xlt on Nov 16, 2023

DrahtBot requested review from w0xlt on Nov 16, 2023

DrahtBot removed review request from w0xlt on Nov 16, 2023

DrahtBot requested review from w0xlt on Nov 16, 2023

DrahtBot removed review request from w0xlt on Nov 16, 2023

DrahtBot requested review from w0xlt on Nov 16, 2023

achow101 force-pushed on Nov 16, 2023

in src/wallet/walletdb.cpp:1009 in 9426d3e7e5 outdated

1066@@ -1063,6 +1067,9 @@ static DBErrors LoadDescriptorWalletRecords(CWallet* pwallet, DatabaseBatch& bat
1067         // Only log if there are no critical errors
1068         pwallet->WalletLogPrintf("Descriptors: %u, Descriptor Keys: %u plaintext, %u encrypted, %u total.\n",
1069                desc_res.m_records, num_keys, num_ckeys, num_keys + num_ckeys);
1070+
1071+        // Upgrade the wallet to have a global HD key
1072+        pwallet->UpgradeToGlobalHDKey(desc_keys, desc_crypt_keys);

S3RK commented at 8:27 am on November 22, 2023:

in 9426d3e7e5c884cd9ed31f2569a38a6e3712cc2c

We do other upgrade procedures in LoadWallet() function and only if result != DBErrors::LOAD_OK Do we want to do the same for UpgradeToGlobalHDKey?

achow101 commented at 7:06 pm on November 27, 2023:

I think it’s fine/better to be doing the upgrade after loading the necessary records. Those other upgrades should probably be moved.

bitcoin deleted a comment on Nov 22, 2023

S3RK commented at 6:44 pm on November 25, 2023: contributor

Almost tested ACK.

Two bugs found:

For blank wallets gehdkey fails due to Assume in GetActiveHDKey
create wallet in the feature branch, downgrade to master, encrypt wallet, upgrade to feature branch again. The wallet will fai to load because it now contains both encrypted and non-encrypted records.

Also quick question. Should we add new flag to KNOWN_WALLET_FLAGS and WALLET_FLAG_MAP maps?

aureleoules commented at 3:19 pm on November 26, 2023: member

The WalletAvailableCoins benchmark crashes on this pull:

0./src/bench/bench_bitcoin -filter=WalletAvailableCoins
1bench_bitcoin: bench/wallet_create_tx.cpp:148: void AvailableCoins(ankerl::nanobench::Bench&, const std::vector<OutputType>&): Assertion `false' failed.
2Aborted

achow101 force-pushed on Nov 27, 2023

achow101 commented at 9:21 pm on November 27, 2023: member

1. For blank wallets gehdkey fails due to Assume in GetActiveHDKey

Fixed.

2. create wallet in the feature branch, downgrade to master, encrypt wallet, upgrade to feature branch again. The wallet will fai to load because it now contains both encrypted and non-encrypted records.

This is slightly problematic since we don’t want to automatically upgrade a wallet in a way that prevents downgrading. I’ve changed the approach in this PR to only store the active xpub. The private keys will instead be retrieved from the descriptors during loading and stored in memory in the wallet, so there is no longer hdkey or hdckey records. This should resolve the downgrading issue while retaining most of the functionality of this PR.

This does mean that key rotation as had been previously implemented is no longer easy to do.

achow101 force-pushed on Nov 27, 2023

achow101 commented at 10:11 pm on November 27, 2023: member

The WalletAvailableCoins benchmark crashes on this pull:

Seems to have been fixed.

Sjors commented at 1:34 pm on November 28, 2023: member

Mmm, I’m not a huge fan of running the relatively complicated migration code in c938680fb97010b16209f1ae05063cd1ff5c8b83 on every wallet load for all future versions of Bitcoin Core, just to deal with an unlikely downgrade -> upgrade path.

achow101 commented at 1:53 pm on November 28, 2023: member

Mmm, I’m not a huge fan of running the relatively complicated migration code in c938680 on every wallet load for all future versions of Bitcoin Core, just to deal with an unlikely downgrade -> upgrade path.

It doesn’t. It still only runs once in order to set activehdkey. After that, on loading, we just check to see if 1) there is an activehdkey and 2) which descriptor key matches the activehdkey. It’s also valid to not have an activehdkey.

DrahtBot added the label Needs rebase on Nov 28, 2023

achow101 force-pushed on Nov 28, 2023

DrahtBot removed the label Needs rebase on Nov 28, 2023

Sjors commented at 9:30 am on November 29, 2023: member

It might be useful to update the PR description to briefly describe the new fields that are added and what the migration does.

furszy commented at 3:14 pm on November 29, 2023: member

create wallet in the feature branch, downgrade to master, encrypt wallet, upgrade to feature branch again. The wallet will fail to load because it now contains both encrypted and non-encrypted records.

Nice catch @S3RK 👌🏼. As we are currently not covering this scenario; added basic coverage for it here https://github.com/furszy/bitcoin-core/commit/c9fcda2de1d6165229f0c45eec9052b02c9516cd. Feel free to take it achow. The test is on an isolated function because I disliked to keep expanding the long code we have there. If you want to merge it with the other stuff, go ahead as well.

It passes at current tip 4195e708 and it fails on a previous version of this PR. Like https://github.com/furszy/bitcoin-core/tree/wallet-knows-master-key which is at tip b31d9bf1 (last version prior to fixing the issue).

RandyMcMillan commented at 3:20 pm on November 29, 2023: contributor

Concept ACK

achow101 commented at 5:19 pm on November 29, 2023: member

added basic coverage for it here furszy@c9fcda2. Feel free to take it achow.

Pulled

It might be useful to update the PR description to briefly describe the new fields that are added and what the migration does.

Updated.

in src/wallet/wallet.cpp:4324 in 14738e9a12 outdated

4316@@ -4317,4 +4317,16 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& walle
4317     }
4318     return res;
4319 }
4320+
4321+void CWallet::LoadActiveHDPubKey(const CExtPubKey& xpub, const CKey& key, const std::vector<unsigned char>& crypted_key)
4322+{
4323+    AssertLockHeld(cs_wallet);
4324+    Assert(xpub.pubkey.IsValid());

S3RK commented at 8:16 am on November 30, 2023:

in 14738e9a12c3d128f52875cf56b2ecacc6e62642

nit. The code is used during wallet loading. Should we instead of Assert return error so we can log proper corruption error?

achow101 commented at 11:39 pm on December 5, 2023:

Changed to if(!Assume...

in src/wallet/wallet.cpp:4321 in 14738e9a12 outdated

4316@@ -4317,4 +4317,16 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& walle
4317     }
4318     return res;
4319 }
4320+
4321+void CWallet::LoadActiveHDPubKey(const CExtPubKey& xpub, const CKey& key, const std::vector<unsigned char>& crypted_key)

S3RK commented at 8:26 am on December 4, 2023:

in 14738e9a12c3d128f52875cf56b2ecacc6e62642

Can we make key optional to avoid passing invalid keys?

achow101 commented at 11:39 pm on December 5, 2023:

Done

in src/wallet/wallet.cpp:4389 in eca964a8c0 outdated

4384+            return std::nullopt;
4385+        }
4386+        if (!Assume(!m_hd_crypted_key.empty())) {
4387+            return std::nullopt;
4388+        }
4389+        if (!DecryptKey(GetEncryptionKey(), m_hd_crypted_key, m_xpub.pubkey, key)) {

S3RK commented at 8:34 am on December 4, 2023:

in eca964a8c07cf43b45e2c0482614ded390d42fec

nit: Could also Assume that decryption is successful

achow101 commented at 11:39 pm on December 5, 2023:

Done

in src/wallet/wallet.h:319 in 14738e9a12 outdated

314@@ -315,6 +315,11 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
315     //! the current wallet version: clients below this version are not able to load the wallet
316     int nWalletVersion GUARDED_BY(cs_wallet){FEATURE_BASE};
317 
318+    //! The extended key used for new automatically generated descriptors
319+    CExtPubKey m_xpub GUARDED_BY(cs_wallet);

S3RK commented at 9:01 am on December 4, 2023:

in 14738e9a12c3d128f52875cf56b2ecacc6e62642

nit: should we make the variables optional to avoid storing invalid keys?

achow101 commented at 11:39 pm on December 5, 2023:

Done

S3RK commented at 8:04 am on December 5, 2023: contributor

Code review ACK ac3b860bff4279ff470560d0ccad9b2fc09672a9

I carefully re-reviewed the new approach. Looks good to me. Going to repeat the tests now.

DrahtBot removed review request from w0xlt on Dec 5, 2023

DrahtBot requested review from w0xlt on Dec 5, 2023

DrahtBot requested review from furszy on Dec 5, 2023

DrahtBot requested review from Sjors on Dec 5, 2023

DrahtBot removed review request from w0xlt on Dec 5, 2023

DrahtBot requested review from w0xlt on Dec 5, 2023

ryanofsky commented at 8:11 pm on December 5, 2023: contributor

Concept ACK, and started reviewing. But I don’t think I understand the reason for adding a new flag to indicate the presence of an ACTIVEHDKEY record, when it seems like code would be simpler and more reliable just handling absence of the record as necessary. It would be helpful if PR description mentioned the reason for adding a new flag and whether it’s actually needed or just desired for some other reason.

DrahtBot removed review request from w0xlt on Dec 5, 2023

DrahtBot requested review from w0xlt on Dec 5, 2023

DrahtBot removed review request from w0xlt on Dec 5, 2023

DrahtBot requested review from w0xlt on Dec 5, 2023

achow101 commented at 8:17 pm on December 5, 2023: member

But I don’t think I understand the reason for adding a new flag to indicate the presence of an ACTIVEHDKEY record, when it seems like code would be simpler and more reliable just handling absence of the record as necessary. It would be helpful if PR description mentioned the reason for adding a new flag and whether it’s actually needed or just desired for some other reason.

The description currently states:

This loading is backwards compatible and uses a new non-required flag WALLET_FLAG_GLOBAL_HD_KEY to signal that the upgrade completed. The upgrade will search for an xpub that is shared by pkh(), wpkh(), and sh(wpkh() descriptors with the derivation pattern that we use. For new wallets, the xpub will be set during descriptor creation rather than trying to reverse engineer it. The flag will be set for all wallets, regardless of whether such an xpub was found or can even exist, in order to indicate that the upgrade will not need to be run in the future.

Basically the point is to signal that the upgrade is done so we don’t do it every time a wallet starts. An important point is that ACTIVEHDKEY does not need to exist after the upgrade was done.

DrahtBot removed review request from w0xlt on Dec 5, 2023

DrahtBot requested review from w0xlt on Dec 5, 2023

ryanofsky commented at 8:42 pm on December 5, 2023: contributor

An important point is that ACTIVEHDKEY does not need to exist after the upgrade was done.

Oh, that explains why you would want a flag. But I’m not sure when this would happen except when the key can’t be determined, which seems like an unusual case. But I need to look into the PR more in any case, so thanks for the correction.

DrahtBot removed review request from w0xlt on Dec 5, 2023

DrahtBot requested review from w0xlt on Dec 5, 2023

achow101 commented at 9:05 pm on December 5, 2023: member

But I’m not sure when this would happen except when the key can’t be determined, which seems like an unusual case.

It would happen for all watch-only wallets, for example, although we do check for that flag separately.

DrahtBot removed review request from w0xlt on Dec 5, 2023

DrahtBot requested review from w0xlt on Dec 5, 2023

achow101 force-pushed on Dec 5, 2023

S3RK commented at 8:38 am on December 7, 2023: contributor

Repeated my manual tests, all works.

The only thing I want to clarify is the downgrade + encrypt situation. So now the wallet loads and not “corrupted” but if the user would generate new descriptors (as proposed in future PRs) the wallet will use the pre-encryption HD key. The scenario seems pretty rare, but still slightly concerning as the pre-encryption HD key could’ve been compromised.

Maybe we should consider only loading key from active descriptors? In this case the wallet will “loose” its HD key, but the behaviour is more safe.

An alternative would be to do the upgrade on every load, but that seems like a worse alternative to me given the scenario should be pretty rare.

Sjors commented at 11:27 am on December 7, 2023: member

I’ve changed the approach in this PR to only store the active xpub. The private keys will instead be retrieved from the descriptors during loading and stored in memory in the wallet, so there is no longer hdkey or hdckey records.

As much as I don’t like having to go through half the PR again, I do think this is a nice simplification. @furszy without key rotation* the only thing that can be done with older descriptor wallets is add the missing taproot descriptor (and future descriptor types). These would then use the old unencrypted master key, which is indeed not good.

One solution could be for the createwalletdescriptor (proposed in #26728) to explicitly check for this scenario. Before creating a descriptor it could check that at least one of the active descriptors derives from activehdkey.

If not, it could tell the user to run the wallet tool, where we could add a method that removes the activehdkey and redo the migration (atomically, so only if that actually works).

* = in #26728 as it’s currently proposed, using key rotation adds a wallet flag that prevents any downgrade.

in test/functional/wallet_backwards_compatibility.py:153 in a86756aed2 outdated

144+        wallet_on_master.backupwallet(backup_path)
145+
146+        # Downgrade and encrypt the wallet.
147+        for node in descriptors_nodes if self.options.descriptors else legacy_nodes:
148+            if self.major_version_less_than(node, 22):
149+                continue  # cannot downgrade further than v22, tr() descriptors aren't supported.

Sjors commented at 11:45 am on December 7, 2023:

You could create a pre-v22 wallet, upgrade to master (which won’t add the tr() descriptor) and then continue the test.

Although I don’t think we care that deeply about downgrading that far, testing a wallet without a tr() descriptor is useful - for followup PR’s that enable adding that desciptor.

in src/wallet/walletdb.cpp:952 in e491e5c4e6 outdated

938@@ -936,6 +939,9 @@ static DBErrors LoadDescriptorWalletRecords(CWallet* pwallet, DatabaseBatch& bat
939                 return DBErrors::CORRUPT;
940             }
941             spk_man->AddKey(pubkey.GetID(), privkey);
942+            if (wallet_xpub && pubkey == wallet_xpub->pubkey) {

Sjors commented at 12:23 pm on December 7, 2023:

e491e5c4e6eb2f9b9ec53f4c2df602a755178726: could make it more clear that this only needs to happen for one descriptor: if (!(wallet_key || wallet_crypted_key) && …

Additionally, either here or elsewhere, it’s useful to clarify the following:

0// Although at this point in the loading process we can't tell if a descriptor is active,
1// checking inactive descriptors is necessary in any case. The private key for the active
2// wallet master extended public key will initially be found in all active descriptors.
3// This changes after a new descriptor is imported, or (exceptionally) if the wallet is
4// downgraded to a pre-ACTIVEHDKEY release and then encrypted.

achow101 commented at 3:54 pm on December 7, 2023:

I don’t think this explainer is relevant in this section, nor really relevant at all. The wallet’s hdkey does not have to be in any active descriptor - the user could have imported new active descriptors with different keys and the hdkey should not be changed.

Sjors commented at 5:35 pm on December 7, 2023:

That’s why it says “This changes after a new descriptor is imported”.

But right now the logic behind populating this field is not documented. Without reading a bunch of pull request comments, one might wonder why we don’t just store the xpriv, why we go through all descriptors instead of one, etc. I get it now, but will probably forget in a few months.

achow101 commented at 5:50 pm on December 7, 2023:

Sure, but that’s not what your suggested text documents. I’ve added a comment earlier during the actual loading that describes it.

in src/wallet/walletdb.cpp:997 in e491e5c4e6 outdated

981@@ -973,6 +982,12 @@ static DBErrors LoadDescriptorWalletRecords(CWallet* pwallet, DatabaseBatch& bat
982         return DBErrors::CORRUPT;
983     }
984 
985+    // Load the active hd key
986+    if (wallet_xpub && !pwallet->LoadActiveHDPubKey(*wallet_xpub, wallet_key, wallet_crypted_key)) {
987+        pwallet->WalletLogPrintf("Error: Unable to load active hd pubkey\n");

Sjors commented at 12:30 pm on December 7, 2023:

e491e5c4e6eb2f9b9ec53f4c2df602a755178726: I thought about adding an additional warning here for the scenario where a user downgraded, encrypted and upgraded. However, for that we need to know if the descriptor is active, which this function (LoadDescriptorWalletRecords) doesn’t know. The caller LoadWallet won’t know it until LoadActiveSPKMs. So it would involve passing a bunch of information around.

Anyway, in principle we could count the number of active descriptors for which pubkey == wallet_xpub->pubkey. Then also count the number of inactive descriptors for which this is the case. Warn if 0 for the active descriptors && >0 for the inactive descriptors.

You could further restrict the warning by seeing if any crypted keys exist (otherwise it may have been the user manually manually importing and overriding all of their active descriptors).

achow101 commented at 3:57 pm on December 7, 2023:

Activeness of descriptors does not necessarily correlate to the hdkey. The user could have imported new active descriptors with different keys and the hdkey would not change.

Also warnings in wallet loading don’t really go anywhere. They show up in the log, but people aren’t going to be looking at their log file unless the wallet isn’t loading.

ryanofsky commented at 3:16 pm on December 20, 2023:

re: #26728 (review)

In commit “wallet: Load activehdkey and its private keys” (2e840bcfc0308a461d210e0c7ed66a9bbc723b62)

Activeness of descriptors does not necessarily correlate to the hdkey. The user could have imported new active descriptors with different keys and the hdkey would not change.

This is counterintuitive and I think it would be really helpful if one of the explanations of the ACTIVEHDKEY field (in the LoadDescriptorWalletRecords comment, or the LoadActiveHDPubKey comment, or the gethdkey RPC documentation) said what the relationship is between the active hd key and active descriptors. Presumeably the active hd key will be used for one set of things, and the active descriptors will be used for another set of things, but I don’t think it’s obvious what those things are.

in src/wallet/walletdb.cpp:991 in e67898f471 outdated

965@@ -940,14 +966,20 @@ static DBErrors LoadDescriptorWalletRecords(CWallet* pwallet, DatabaseBatch& bat
966 
967         return result;
968     });
969+    result = std::max(result, desc_res.m_result);
970+
971+    if (num_ckeys > 0 && num_keys != 0) {
972+        pwallet->WalletLogPrintf("Error: Wallet has both encrypted and unencrypted HD keys\n");

Sjors commented at 12:42 pm on December 7, 2023:

e67898f47152b756acc7ccafb654e2600b0e7a03 nit: could be its own commit?

achow101 commented at 4:05 pm on December 7, 2023:

Done

in src/wallet/wallet.cpp:4515 in 6d7163d8e8 outdated

4368+    }
4369+    return SetActiveHDKey(xprv.Neuter(), key, crypted_key);
4370+}
4371+
4372+bool CWallet::SetActiveHDKey(const CExtPubKey& xpub, const std::optional<CKey>& key, const std::vector<unsigned char>& crypted_key)
4373+{

Sjors commented at 12:56 pm on December 7, 2023:

6d7163d8e8127a18a299eaa1309bf7d613713d51: maybe assert that either key or crypted_key is provided

(though LoadActiveHDPubKey eventually does that)

achow101 commented at 4:00 pm on December 7, 2023:

LoadActiveHDPubKey already does this.

in src/wallet/wallet.cpp:379 in a5ba1d8382 outdated

374@@ -375,6 +375,9 @@ std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string&
375     uint64_t wallet_creation_flags = options.create_flags;
376     const SecureString& passphrase = options.create_passphrase;
377 
378+    // Always set global HD key since the wallet supports it, even if the flag is not useful
379+    wallet_creation_flags |= WALLET_FLAG_GLOBAL_HD_KEY;

Sjors commented at 1:08 pm on December 7, 2023:

a5ba1d83827475ef3f8f3851733007db5b8dc848: not a fan of touching legacy wallets even in the slightest if we don’t have to… in that respect I prefer fca4584ad3.

It seems contradictory to the fact that we always generate the global hd key field when migrating a legacy wallet, whether it was newly created or not. We could set this field when creating a legacy wallet, but we (gladly) choose not to. It wouldn’t do anything useful and would suffer from the same downgrade->encrypt->upgrade incorrectness. So I think we also shouldn’t set the flag.

I do agree with setting the flag for watch-only. That seems future-proof, e.g. imagine a scenario where a WALLET_FLAG_DISABLE_PRIVATE_KEYS can be removed; such a change should take care of setting the master hdkey, and not (implicitly) rely on the migration code in ea7a61ca94cc0d59cfe25307cd60fcda2aedec5f.

That in turn insures that migration code should never have to updated to account for future features, as it’s only ever run on wallets that were last loaded with <v27.0

achow101 commented at 4:02 pm on December 7, 2023:

We always generate new descriptors for migrated wallets, regardless of a preexisting HD seed. I don’t see why setting this flag would be contradictory with that.

Sjors commented at 1:29 pm on December 7, 2023: member

Mostly happy with a86756aed266a1c7569dc3849f66f468f3810821.

Still need to re-review ea7a61ca94cc0d59cfe25307cd60fcda2aedec5f.

in src/wallet/wallet.cpp:561 in ea7a61ca94 outdated

557@@ -558,6 +558,106 @@ void CWallet::UpgradeDescriptorCache()
558     SetWalletFlag(WALLET_FLAG_LAST_HARDENED_XPUB_CACHED);
559 }
560 
561+/**

Sjors commented at 2:32 pm on December 7, 2023:

ea7a61ca94cc0d59cfe25307cd60fcda2aedec5f nit, keeping it closer to 80 characters wide, and drops “Any other candidates will be added as HD keys that have been rotated out.”:

 0/**
 1 * UpgradeToGlobalHDKey searches through the descriptors in a descriptor wallet
 2 * and tries to find a CExtPubKey which is likely to be the most recent one used
 3 * to generate all of the automatically generated descriptors. The automatically
 4 * generated descriptors are guaranteed to be 2 p2pkh, 2 p2sh-segwit, and 2 p2wpkh.
 5 * There may be 2 p2tr descriptors, as well as imports. Those are ignored as they
 6 * are not guaranteed to be present in all wallets.
 7 * Candidate CExtKeys are those that appear in the aforementioned descriptors as
 8 * the only key. The best candidate is selected as the one that was most recently
 9 * used.
10 *
11 * The best candidate will be added to the wallet as the current active HD key.
12 *
13 * WALLET_FLAG_GLOBAL_HD_KEY will be set in order to indicate that this upgrade
14 * has occurred, regardless of whether the active HD key was found.
15 */

I appended “, regardless…”

achow101 commented at 4:05 pm on December 7, 2023:

Taken

achow101 commented at 3:51 pm on December 7, 2023: member

The only thing I want to clarify is the downgrade + encrypt situation. So now the wallet loads and not “corrupted” but if the user would generate new descriptors (as proposed in future PRs) the wallet will use the pre-encryption HD key. The scenario seems pretty rare, but still slightly concerning as the pre-encryption HD key could’ve been compromised.

We have the version which stores the version of the last client which opened the wallet. Maybe we should use that to also determine whether an upgrade needs to be performed? e.g. if the last client’s version is less than 270000, always do the upgrade? This would cover the encryption case, and we could drop the wallet flag.

achow101 force-pushed on Dec 7, 2023

achow101 commented at 4:35 pm on December 7, 2023: member

We have the version which stores the version of the last client which opened the wallet. Maybe we should use that to also determine whether an upgrade needs to be performed?

Hmm, this seems to not necessarily be true for older versions. 23.0 has it has updating only when less than, not just not equal.

Seems like 23.x and earlier didn’t always write the record, so we could use that as a signal here too.

Edit: nvm, doesn’t work for upgrade then downgrade then upgrade again.

achow101 force-pushed on Dec 7, 2023

achow101 force-pushed on Dec 11, 2023

achow101 commented at 7:43 pm on December 11, 2023: member

I’ve pushed a solution for the downgrade and encrypt issue. This will store an extra boolean in ACTIVEHDKEY which indicates whether the wallet was encrypted at the time that record was written. Then, when we detect that the wallet is now encrypted, but the ACTIVEHDKEY was written when the wallet was unencrypted, we will run the upgrade again to detect the new hd key.

For wallets that are encrypted with software supporting the record, the encryption status flag will be updated when the new hd key is written. For wallets encrypted on older software, we will be able to detect the new hd key when the software is upgraded.

Note that key rotation on encryption can happen only once in a wallet’s lifetime - when the wallet is encrypted the first. We do not do key rotation when the passphrase changes.

achow101 force-pushed on Dec 11, 2023

DrahtBot added the label CI failed on Dec 11, 2023

DrahtBot removed the label CI failed on Dec 12, 2023

in src/wallet/walletdb.cpp:1006 in ce5495a4b2 outdated

1000@@ -997,6 +1001,9 @@ static DBErrors LoadDescriptorWalletRecords(CWallet* pwallet, DatabaseBatch& bat
1001         // Only log if there are no critical errors
1002         pwallet->WalletLogPrintf("Descriptors: %u, Descriptor Keys: %u plaintext, %u encrypted, %u total.\n",
1003                desc_res.m_records, num_keys, num_ckeys, num_keys + num_ckeys);
1004+
1005+        // Upgrade the wallet to have a global HD key

Sjors commented at 10:36 am on December 14, 2023:

ce5495a4b27fe9646c9933dc39f0cb4e8510eaff: I still think it’s nicer to check

0// Upgrade the wallet to have a global HD key, or in the special case
1// where the user downgraded and then encrypted / decrypted.
2if(!pwallet->IsWalletFlagSet(WALLET_FLAG_GLOBAL_HD_KEY) || enc_status != IsCrypted()) {

here rather than inside UpgradeToGlobalHDKey, so that it’s clear to people reading this code when they need to open wallet.cpp and look at the function body.

Alternatively, you could expand the description in wallet.h (e.g. using my suggested text), since a good IDE will show that on hover.

achow101 commented at 11:48 pm on December 20, 2023:

I prefer these checks to be in the function itself rather than the caller for these upgrades as we may want to call these from elsewhere in the future.

Sjors commented at 11:05 am on December 14, 2023: member

059d847810051b52d33b5b16711a031c3fd199a9 looks good, except that you need to call SetActiveHDKey() in CWallet::EncryptWallet (to avoid needlessly running the upgrade code).

Note that key rotation on encryption can happen only once in a wallet’s lifetime - when the wallet is encrypted the first. We do not do key rotation when the passphrase changes.

TIL you can’t turn off encryption once it’s turned on (walletpassphrasechange does not allow "" as the password, unlike createwallet), so we don’t have to worry about a downgrade -> encrypt -> decrypt scenario.

—

While running the test suite on each commit, I encounter a failure at 059d847810051b52d33b5b16711a031c3fd199a9. This happens once every five runs or so:

 02023-12-14T11:16:26.443000Z TestFramework (INFO): Test downgrade encryption + re-open on master
 12023-12-14T11:16:27.401000Z TestFramework (ERROR): Assertion failed
 2Traceback (most recent call last):
 3  File "/home/sjors/dev/bitcoin/test/functional/test_framework/test_framework.py", line 131, in main
 4    self.run_test()
 5  File "/home/sjors/dev/bitcoin/test/functional/wallet_backwards_compatibility.py", line 371, in run_test
 6    self.test_downgrade_encryption(node_master, descriptors_nodes, legacy_nodes)
 7  File "/home/sjors/dev/bitcoin/test/functional/wallet_backwards_compatibility.py", line 186, in test_downgrade_encryption
 8    assert xpub != enc_wallet.gethdkey()["xpub"]
 9AssertionError
102023-12-14T11:16:27.453000Z TestFramework (INFO): Stopping nodes

DrahtBot added the label CI failed on Dec 14, 2023

S3RK commented at 9:37 am on December 18, 2023: contributor

The code looks good. That’s quite a small change and fully fixes the problem. Didn’t repeat manual tests though, but there is an automatic test coverage. Great job!

CI failure seems relevant. I was able to reproduce locally, didn’t have time to debug yet though. Will look tomorrow if nobody beats me to it.

0                                   Traceback (most recent call last):
1                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
2                                       self.run_test()
3                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_backwards_compatibility.py", line 371, in run_test
4                                       self.test_downgrade_encryption(node_master, descriptors_nodes, legacy_nodes)
5                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_backwards_compatibility.py", line 186, in test_downgrade_encryption
6                                       assert xpub != enc_wallet.gethdkey()["xpub"]
7                                   AssertionError

059d847 looks good, except that you need to call SetActiveHDKey() in CWallet::EncryptWallet (to avoid needlessly running the upgrade code).

Or maybe even add it to SetupDescriptorScriptPubKeyMans(const CExtKey& master_key)

in src/wallet/walletdb.cpp:823 in 8bb7a8b06f outdated

807+            return DBErrors::CORRUPT;
808+        }
809+        // TODO: Load extpub into the wallet
810+        return DBErrors::LOAD_OK;
811+    });
812+    result = std::max(result, active_hdkey_res.m_result);

furszy commented at 2:24 pm on December 18, 2023:

in 8bb7a8b06:

nit: probably for the future; as ACTIVEHDKEY is unique, we don’t need to call LoadRecords here. Could call a bare Read() or implement a GetActiveHDKey() at the WalletBatch level.

achow101 commented at 11:42 pm on December 20, 2023:

Read requires an unserializable object as a parameter, and since this record has 2 objects, using Read() is a non-trivial change.

in src/wallet/wallet.cpp:4489 in 2e840bcfc0 outdated

4356+        if (!Assume(!key)) {
4357+            return false;
4358+        }
4359+        m_hd_crypted_key = crypted_key;
4360+        return true;
4361+    }

furszy commented at 2:42 pm on December 18, 2023:

In 2e840bcfc03:

Would be good to cleanup m_hd_key when m_hd_crypted_key is set and vice versa. Just as a safety measure in case this method is called twice pre and post encryption.

achow101 commented at 11:43 pm on December 20, 2023:

Both are now a variant in a new struct, so should no longer be a problem.

DrahtBot removed the label CI failed on Dec 19, 2023

achow101 commented at 10:31 pm on December 19, 2023: member

059d847 looks good, except that you need to call SetActiveHDKey() in CWallet::EncryptWallet (to avoid needlessly running the upgrade code).

Or maybe even add it to SetupDescriptorScriptPubKeyMans(const CExtKey& master_key)

It already does that in the SetupDescriptorScriptPubKeyMans() that is called by EncryptWallet.

achow101 force-pushed on Dec 19, 2023

achow101 commented at 11:27 pm on December 19, 2023: member

Intermittent failure should be fixed.

S3RK commented at 7:39 am on December 20, 2023: contributor

Code review ACK db6b61e9e7635c9cb97d73fd44b9e7266b8eef51

DrahtBot removed review request from w0xlt on Dec 20, 2023

DrahtBot requested review from Sjors on Dec 20, 2023

DrahtBot requested review from w0xlt on Dec 20, 2023

DrahtBot requested review from ryanofsky on Dec 20, 2023

in src/wallet/walletdb.cpp:798 in 8bb7a8b06f outdated

790@@ -782,6 +791,25 @@ static DataStream PrefixStream(const Args&... args)
791 static DBErrors LoadDescriptorWalletRecords(CWallet* pwallet, DatabaseBatch& batch, int last_client) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)
792 {
793     AssertLockHeld(pwallet->cs_wallet);
794+    DBErrors result = DBErrors::LOAD_OK;
795+
796+    // Get Active HD Key
797+    LoadResult active_hdkey_res = LoadRecords(pwallet, batch, DBKeys::ACTIVEHDKEY,
798+        [] (CWallet* pwallet, DataStream& key, DataStream& value, std::string& err) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet) {

furszy commented at 1:06 pm on December 20, 2023:

In 8bb7a8b06:

EXCLUSIVE_LOCKS_REQUIRED annotation is not required.

ryanofsky commented at 3:59 pm on December 20, 2023:

re: #26728 (review)

In commit “walletdb: Read and write activehdkey record” (8bb7a8b06f1f0558bdb0bda696f5fb5748dacf7e)

EXCLUSIVE_LOCKS_REQUIRED annotation is not required.

If you drop it would also suggest dropping the unused pwallet variable (just write [] (CWallet*, ...))

achow101 commented at 11:43 pm on December 20, 2023:

Done

in src/wallet/walletdb.cpp:1235 in 8bb7a8b06f outdated

1198@@ -1170,6 +1199,9 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
1199         }
1200 #endif
1201 
1202+        // Load decryption keys
1203+        result = std::max(LoadDecryptionKeys(pwallet, *m_batch), result);
1204+

furszy commented at 1:12 pm on December 20, 2023:

In 8bb7a8b0:

This is moved without an explanation. Could mention the rationale in the commit description.

achow101 commented at 11:43 pm on December 20, 2023:

Moved this to the commit where it actually matters, so should hopefully make more sense.

in src/wallet/wallet.cpp:4394 in 2e2545c3a2 outdated

4389+}
4390+
4391+bool CWallet::SetActiveHDKey(const CExtPubKey& xpub, const std::optional<CKey>& key, const std::vector<unsigned char>& crypted_key)
4392+{
4393+    AssertLockHeld(cs_wallet);
4394+    LoadActiveHDPubKey(xpub, key, crypted_key);

furszy commented at 1:30 pm on December 20, 2023:

In 2e2545c3a:

Missing check for LoadActiveHDPubKey() output.

achow101 commented at 11:43 pm on December 20, 2023:

Done

in src/wallet/wallet.cpp:658 in ce5495a4b2 outdated

653+        std::vector<unsigned char> crypted_key;
654+        auto ckey_it = desc_crypt_keys.find(best_xpub->pubkey);
655+        if (ckey_it != desc_crypt_keys.end()) {
656+            crypted_key = ckey_it->second;
657+        }
658+        SetActiveHDKey(*best_xpub, key, crypted_key);

furszy commented at 1:39 pm on December 20, 2023:

In ce5495a4b27f:

Need to check SetActiveHDKey() output. If it fails, the procedure should fail.

achow101 commented at 11:44 pm on December 20, 2023:

I’ve added an Assume here. There isn’t really a valid failure scenario here other than write failure.

in src/wallet/wallet.cpp:640 in db6b61e9e7 outdated

635+    std::map<CKeyID, CExtPubKey> xpubs;
636+    for (const auto& [xpub, info] : key_counts) {
637+        const auto& [dtypes, desc_time] = info;
638+        if (std::all_of(dtypes.begin(), dtypes.end(), [](const auto& p) { return p.second == 2;})) {
639+            xpubs.emplace(xpub.pubkey.GetID(), xpub);
640+            if (desc_time > best_time) {

ryanofsky commented at 1:42 pm on December 20, 2023:

In commit “wallet: Automatically upgrade a wallet to have global hd key” (ce5495a4b27fe9646c9933dc39f0cb4e8510eaff)

Would seem a little safer to write if (!best_xpub || desc_time > best_time) to work even if desc_time is not set.

achow101 commented at 11:44 pm on December 20, 2023:

Done

furszy commented at 1:47 pm on December 20, 2023: member

Finished another round, only small findings.

DrahtBot removed review request from w0xlt on Dec 20, 2023

DrahtBot requested review from furszy on Dec 20, 2023

DrahtBot requested review from w0xlt on Dec 20, 2023

DrahtBot removed review request from w0xlt on Dec 20, 2023

DrahtBot requested review from w0xlt on Dec 20, 2023

in src/wallet/wallet.cpp:649 in db6b61e9e7 outdated

644+        }
645+    }
646+
647+    if (best_xpub) {
648+        std::optional<CKey> key;
649+        auto key_it = desc_keys.find(best_xpub->pubkey);

ryanofsky commented at 1:52 pm on December 20, 2023:

In commit “wallet: Automatically upgrade a wallet to have global hd key” (ce5495a4b27fe9646c9933dc39f0cb4e8510eaff)

Can you write a comment explaining when this find is expected to fail (also the find below)?

Naively, I would think if this find fails, probably it means “best_xpub” is not really the best xpub. I would have expected these lookups to have already been done in one of the previous two loops and used to determine the best key candidate.

This is probably wrong, but either way, it would be help to have a comment saying why these finds are allowed to fail.

achow101 commented at 11:45 pm on December 20, 2023:

These can fail because the key may or may not be encrypted. I’ve added a few comments.

in src/wallet/wallet.cpp:658 in db6b61e9e7 outdated

653+        std::vector<unsigned char> crypted_key;
654+        auto ckey_it = desc_crypt_keys.find(best_xpub->pubkey);
655+        if (ckey_it != desc_crypt_keys.end()) {
656+            crypted_key = ckey_it->second;
657+        }
658+        SetActiveHDKey(*best_xpub, key, crypted_key);

ryanofsky commented at 1:58 pm on December 20, 2023:

In commit “wallet: Automatically upgrade a wallet to have global hd key” (ce5495a4b27fe9646c9933dc39f0cb4e8510eaff)

Return value here is ignored. It would be good to add [[nodiscard]], (void) and comment saying why failure is ignored.

Instinctively, I’d say it even if makes sense to set the GLOBAL_HD_KEY flag when no HD key candidate is written, I don’t think it makes sense to set it when there is an unexpected error trying to write it, or trying to find it.

This could be wrong, and maybe the current approach to error handling here makes more sense, but it should be explained either way I think.

achow101 commented at 11:45 pm on December 20, 2023:

I’ve changed it to Assume. Also changed the flag setting to only occur if there were no other problems.

in src/wallet/walletdb.h:289 in db6b61e9e7 outdated

284+     * then generate new descriptors by encrypting, and then loading in a version that
285+     * does know about the record. There would then be a mismatch between the stored
286+     * xpub and the actual keys used to generate those descriptors. To account for this,
287+     * we store whether the wallet was encrypted at the time this record was written,
288+     * and can perform the upgrade again if the stored encryption status doesn't
289+     * match the real status.

ryanofsky commented at 2:39 pm on December 20, 2023:

In commit “walletdb: Read and write activehdkey record” (8bb7a8b06f1f0558bdb0bda696f5fb5748dacf7e)

This makes sense, but it sounds so fragile it makes me wonder why we want to store this unreliable value.

Can there be a comment (here or in LoadDescriptorWalletRecords) saying why it is necessary to store this? Or what the drawbacks would be of not storing it and just figuring out the active seed when the wallet is loaded, or when the gethdkey RPC is called?

achow101 commented at 11:47 pm on December 20, 2023:

Added a comment. The purpose is to be consistent so that the user is not confused. Otherwise an import could change the result of gethdkey unexpectedly. With the future sethdseed change, gethdkey’s result being different will at least be explicit, whereas an import may or may not change the detection of the hd key, depending on what else is/has been imported.

in src/wallet/wallet.cpp:4469 in db6b61e9e7 outdated

4452@@ -4336,4 +4453,96 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& walle
4453     }
4454     return res;
4455 }
4456+
4457+bool CWallet::LoadActiveHDPubKey(const CExtPubKey& xpub, const std::optional<CKey>& key, const std::vector<unsigned char>& crypted_key)
4458+{
4459+    AssertLockHeld(cs_wallet);
4460+    if (!Assume(xpub.pubkey.IsValid())) {

ryanofsky commented at 3:25 pm on December 20, 2023:

Not a comment for this PR specifically, but I think this !Assume code is hard to read and unnatural to parse, even though it does seem like a good way to handle bad assumptions at runtime. IMO, the code would be easier to read with a dedicated macro like:

0ASSERT_ELSE(xpub.pubkey.IsValid(), return false);
1
2ASSERT_ELSE(m_hd_crypted_key.empty(), return std::nullopt);
3
4ASSERT_ELSE(pwallet->IsWalletFlagSet(WALLET_FLAG_GLOBAL_HD_KEY), 
5    throw JSONRPCError(RPC_WALLET_ERROR, "Wallet has not been upgraded to support global hd keys"));

in src/wallet/wallet.h:322 in db6b61e9e7 outdated

315@@ -315,6 +316,11 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
316     //! the current wallet version: clients below this version are not able to load the wallet
317     int nWalletVersion GUARDED_BY(cs_wallet){FEATURE_BASE};
318 
319+    //! The extended key used for new automatically generated descriptors
320+    std::optional<CExtPubKey> m_xpub GUARDED_BY(cs_wallet);
321+    std::optional<CKey> m_hd_key GUARDED_BY(cs_wallet);
322+    std::vector<unsigned char> m_hd_crypted_key GUARDED_BY(cs_wallet);

ryanofsky commented at 3:51 pm on December 20, 2023:

In commit “wallet: Load activehdkey and its private keys” (2e840bcfc0308a461d210e0c7ed66a9bbc723b62)

It’s not obvious what the relationship between m_xpub, m_hd_key, and m_hd_crypted_key is here. I think it would clearer to have a struct like:

0struct HDKey {
1    CExtPubKey xpub;
2    //! Variant storing CKey if the wallet is unencrypted, otherwise the encrypted key.
3    std::variant<CKey, std::vector<unsigned char>> xpriv;
4}

Then CWallet could just add:

0//! The extended key used for new automatically generated descriptors
1std::optional<HDKey> m_active_hd_key;

Using a variant and single optional value rather than separate optional values should also make code more type-safe and reduce the number of Assume() checks that are needed.

achow101 commented at 11:47 pm on December 20, 2023:

Done as suggested.

in src/wallet/wallet.h:1048 in 2e840bcfc0 outdated

1042@@ -1038,6 +1043,9 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
1043 
1044     //! Whether the (external) signer performs R-value signature grinding
1045     bool CanGrindR() const;
1046+
1047+    //! Handling wallet HD Master Keys that are used for new automatic descriptors. No derivation will be performed.
1048+    bool LoadActiveHDPubKey(const CExtPubKey& xpub, const std::optional<CKey>& key, const std::vector<unsigned char>& crypted_key) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

ryanofsky commented at 4:08 pm on December 20, 2023:

In commit “wallet: Load activehdkey and its private keys” (2e840bcfc0308a461d210e0c7ed66a9bbc723b62)

What does no deriviation will be performed mean? Would suggest clarifying, or maybe removing.

achow101 commented at 11:47 pm on December 20, 2023:

Removed

ryanofsky commented at 4:41 pm on December 20, 2023: contributor

Mostly reviewed 059d847810051b52d33b5b16711a031c3fd199a9 and it looks good, but a number of things were unclear to me and I left questions.

Another question I had was now that descriptor wallets are aware of the HD seed, does it make sense for getwalletinfo to start returning an hdseedid value? Does it make sense for sethdseed RPC to be supported? ~~Assume the answer to these is no, but I’m not clear on why.~~ I think it probably would make sense to say in the migratewallet documentation how these things map over from legacy to descriptor wallets

EDIT: sethdseed for descriptor wallets is actually implemented in #23544, so answer to that question is yes

DrahtBot removed review request from w0xlt on Dec 20, 2023

DrahtBot requested review from ryanofsky on Dec 20, 2023

DrahtBot requested review from w0xlt on Dec 20, 2023

DrahtBot removed review request from w0xlt on Dec 20, 2023

DrahtBot requested review from w0xlt on Dec 20, 2023

ryanofsky commented at 9:04 pm on December 20, 2023: contributor

re: #26728 (review)

Activeness of descriptors does not necessarily correlate to the hdkey. The user could have imported new active descriptors with different keys and the hdkey would not change.

Looking at related PRs which will build on this one, this approach seems increasingly kludgy to me.

It seems to me wallet usability will be worse and behavior is more confusing if:

the active hd key (seed)
the active set of descriptors

are separate things that are stored independently and are used for different purposes.

I think it would be better not to introduce the concept of an “active hd key” at all, and instead follow an approach more like what apoelstra implemented in #27351 where a hd seed is just extra information associated with a descriptor, and different descriptors can have different seeds.

I think it is useful to have a gethdkey method similar to the one added here that would return the hd key associated with various descriptors. But that could be implemented more simply without changing the database format, or adding a flag, or bringing in other complexity introduced in this PR.

I also think the sethdseed RPC should be extended to work with descriptor wallets like in #29054 to make it easy to bring in seeds and work for applications like #27351. But I think the proposed sethdseed implementation in #29054 which adds an active hd seed that “will only be used for new automatically generated descriptors” going forward is unnecessarily confusing. It would seem more straightforward for the RPC to add the HD key to the wallet and optionally generate new descriptors as a one-time event, without setting an “active” seed which is independent from the active descriptors and could become irrelevant if new descriptors are imported.

I also think the createwalletdescriptor RPC from #25907 should be added to make it easy to upgrade wallets when new address types are added. But this could work the same way as gethdkey and use the HD key already in the wallet, or allow one to be specified if there are multiple.

I also think importdescriptor logic Sjors added in #23544 to support #24861 which builds on GetActiveHDKey from this PR seems very useful. But it seems unnecessarily fragile for it to be based on this. If you are trying to derive private keys associated with an imported descriptor, why should you care whether the seed is “active” or not? It seems like it would be more reliable just to work as whenever the seed is present.

Basically in all these cases it seems like adding the concept of an “active hd key” makes using and understanding the wallet more complicated, and also makes the implementation more complicated than it needs to be.

I have not been following discussion around these topics very long, and I’m probably mistaken about of lot of things, it just seems to me like we should be able to get all of the benefits of this PR without the complexity.

DrahtBot removed review request from w0xlt on Dec 20, 2023

DrahtBot requested review from w0xlt on Dec 20, 2023

DrahtBot requested review from ryanofsky on Dec 20, 2023

DrahtBot removed review request from w0xlt on Dec 20, 2023

DrahtBot requested review from w0xlt on Dec 20, 2023

DrahtBot removed review request from w0xlt on Dec 20, 2023

DrahtBot requested review from w0xlt on Dec 20, 2023

achow101 commented at 9:26 pm on December 20, 2023: member

I think it would be better not to introduce the concept of an “active hd key” at all, and instead follow an approach more like what apoelstra implemented in #27351 where a hd seed is just extra information associated with a descriptor, and different descriptors can have different seeds.

This is essentially what we do today, albeit with hd keys rather than hd seeds.

I think it is useful to have a gethdkey method similar to the one added here that would return the hd key associated with various descriptors. But that could be implemented more simply without changing the database format, or adding a flag, or bringing in other complexity introduced in this PR.

What would the API for that look like?

I also think the createwalletdescriptor RPC from #25907 should be added to make it easy to upgrade wallets when new address types are added. But this could work the same way as gethdkey and use the HD key already in the wallet, or allow one to be specified if there are multiple.

Ultimately, the work in this PR is the product of different iterations of how to implement createwalletdescriptor. The main thing blocking that is the question of which key should be used in the new descriptors. I’ve tried a couple of different approaches, and they all have drawbacks in some form:

Generate a new hd key and use that for the descriptor. Feedback on this was essentially that it would be confusing to users since all other descriptors use the same hd key. It would also make a wallet made in the past inconsistent with the pattern for wallets made today.
Have the user provide the hd key. This really sucks for the user since they need to figure out how to get a hd key. The easiest way is to just pull it out from a descriptor in the same wallet, then we should be smart enough to figure that out for them.
Have the user specify a descriptor. This is basically providing the hd key but worse since descriptors are longer, and it would only work for single key descriptors.
- Specify a descriptor id. While shorter than specifying a full descriptor, it’s still not a good UX. The user has to find a descriptor id, and we don’t currently expose that info. The descriptor id is also completely opaque and we’d then have to settle on what an id even means.

The goal of this PR is to provide an answer for this question. Whenever there is more than one hd key, there is a UX problem in how the user is supposed to specify it. So this PR resolves that by picking only one key. Then there is the problem with upgrading. This PR solves that by being consistent with new wallets - new wallets have a master key that is automatically generated and used in all the automatically generated descriptors, so it tries to reverse engineer the hd key from the existing descriptors. Using the automatically generated key means that all wallets, both old and new, should behave consistently.

does it make sense for getwalletinfo to start returning an hdseedid value?

We can’t do that since we’re dealing with hd keys rather than hd seeds. The seed is hashed in order to become a master key, and descriptor wallets always throw that away once the master key is computed as it is no longer needed. We wouldn’t be able to compute this for old wallets.

DrahtBot removed review request from w0xlt on Dec 20, 2023

DrahtBot requested review from w0xlt on Dec 20, 2023

DrahtBot removed review request from w0xlt on Dec 20, 2023

DrahtBot requested review from w0xlt on Dec 20, 2023

walletdb: Read and write activehdkey record 993e5da9cb

wallet: Load activehdkey and its private keys

The activehdkey record will now be loaded into the wallet as its own
member variable. Since the private keys will always be in existing
descriptors in the wallet, the key for the active hd key is also pulled
out of those descriptors on loading.

39aa4ddbf9

wallet: Store master key used for automatically generated descriptors 5a60d83edb

wallet: Retrieve active hd pubkey 5c9f287ef1

key: Add constructor for CExtKey that takes CExtPubKey and CKey

We often need to construct a CExtKey given an CExtPubKey and CKey, so
implement a constructor that does that for us.

93f4729e17

achow101 force-pushed on Dec 20, 2023

DrahtBot added the label CI failed on Dec 21, 2023

wallet: Implement GetActiveHDPrivkey 3342b99adf

wallet, rpc: Add gethdkey RPC 6de30ebbb3

descriptor: Be able to get the pubkeys involved in a descriptor 17fce45eb1

wallet: Always set WALLET_FLAG_GLOBAL_HD_KEY for new wallets 71d9399f65

wallet, rpc: Check WALLET_FLAG_GLOBAL_HD_KEY in gethdkey 2bcccba704

achow101 force-pushed on Dec 21, 2023

DrahtBot removed the label CI failed on Dec 21, 2023

in src/wallet/walletdb.cpp:829 in 442962d441 outdated

824@@ -825,8 +825,10 @@ static DBErrors LoadDescriptorWalletRecords(CWallet* pwallet, DatabaseBatch& bat
825     // Load descriptor record
826     int num_keys = 0;
827     int num_ckeys= 0;
828+    std::map<CPubKey, CKey> desc_keys;
829+    std::map<CPubKey, std::vector<unsigned char>> desc_crypt_keys;

ryanofsky commented at 2:01 am on December 21, 2023:

In commit “wallet: Automatically upgrade a wallet to have global hd key” (442962d4410edd985fe336f29070c55516ae7de4)

It seems like these variables are unnecessary, and CWallet::UpgradeToGlobalHDKey could just get the keys from the descriptors in the wallet instance, instead of requiring them to be passed as extra argument. This would require adding an extra loop to UpgradeToGlobalHDKey, but also make it less fragile, simpler to call, and easier to understand without having to look at outside code.

achow101 commented at 4:29 pm on December 21, 2023:

Good point, done.

in test/functional/wallet_backwards_compatibility.py:134 in 98b6fad226 outdated

130@@ -131,6 +131,71 @@ def test_v19_addmultisigaddress(self):
131         wallet = node_v19.get_wallet_rpc("w1_v19")
132         assert wallet.getaddressinfo(address_18075)["solvable"]
133 
134+    def test_downgrade_encryption(self, node_master, descriptors_nodes, legacy_nodes):

ryanofsky commented at 2:34 am on December 21, 2023:

In commit “test: add coverage for re-opening a downgraded encrypted wallet on master” (98b6fad226640a0827e39574a91dd3d2fdeac94a)

I think your renaming script went too far 😁, this commit used to be authored by furszy (db6b61e9e7635c9cb97d73fd44b9e7266b8eef51)

achow101 commented at 3:45 am on December 21, 2023:

Oops fixed

ryanofsky commented at 3:01 am on December 21, 2023: contributor

re: #26728 (comment) re: #26728#pullrequestreview-1791595622

This is essentially what we do today, albeit with hd keys rather than hd seeds.

Right. It seems like we can add the features we want without making the wallet data model more complicated, so I think we should do that. If there are features that can’t work under the current model, or would work worse under the current model, that would change things, but in all of the cases I’ve read about it actually looks like the features would work better and be less confusing in the current model compared to the new model.

I think it is useful to have a gethdkey method similar to the one added here that would return the hd key associated with various descriptors. But that could be implemented more simply without changing the database format, or adding a flag, or bringing in other complexity introduced in this PR.

What would the API for that look like?

Instead of returning an object with “xpub” and “xpriv” fields, it would return a list of objects with “xpub”, “xpriv”, and “descriptors” fields. The “descriptors” field would contain a list of descriptors using the hd key.

It would also be good for the method to accept an “active_only” option, and only return hd keys associated with active descriptors by default.

This would make it easy to get the hd key, and easy to understand how the hd key is referenced by descriptors. In the typical case it would only return 1 hd key, and in more complicated cases it would be easier to understand than the gethdmethod in this PR, because it would give you all the information about hd keys, instead of just choosing one hd key and not telling you which descriptors it is associated with.

Ultimately, the work in this PR is the product of different iterations of how to implement createwalletdescriptor. The main thing blocking that is the question of which key should be used in the new descriptors. I’ve tried a couple of different approaches, and they all have drawbacks in some form: […]

Thanks for describing all those approaches. It clarifies a lot, and I agree with all the drawbacks you mentioned for them. The approach I would suggest is making the hd key an optional parameter to createwalletdescriptor, and if it is not specified, using the same hd key as other active descriptors. Unless I am misunderstanding something, in normal cases we would expect there to be exactly one hd key. But in cases where there is more than one key, we could choose the active descriptor for the default output type (like getnewaddress), or be more conservative and raise an exception asking for an hd key to be specified to disambiguate.

For cases where the hd key is specified to createwalletdescriptor, I think we would want it to be flexible and accept multiple formats, but accepting “xpub” value from gethdkey would be an obvious start. It could also accept the xpriv value, or even the seed keys or bytes. If it accepted private keys or seeds, we could treat it as a generalized alternative to sethdseed, which would be great because it would simplify usage, and let us deprecate the sethdseed method instead of keeping both methods.

The goal of this PR is to provide an answer for this question. Whenever there is more than one hd key, there is a UX problem in how the user is supposed to specify it. So this PR resolves that by picking only one key. Then there is the problem with upgrading. This PR solves that by being consistent with new wallets - new wallets have a master key that is automatically generated and used in all the automatically generated descriptors, so it tries to reverse engineer the hd key from the existing descriptors. Using the automatically generated key means that all wallets, both old and new, should behave consistently.

Yeah I appreciate the cleverness of the implementation, and the fact that you actually found a way to add a new field to the database that depends on existing fields, and still allow the wallet to be opened by older software, and magically get the new software to detect whether the existing fields changed to update the new field.

But unless I’m missing something (which is possible and maybe likely), I don’t actually see how this magic improves the UX. It seems like it only returns less information to the user and makes wallet behavior more opaque in the use-cases I’ve seen so far.

does it make sense for getwalletinfo to start returning an hdseedid value?

We can’t do that since we’re dealing with hd keys rather than hd seeds. The seed is hashed in order to become a master key, and descriptor wallets always throw that away once the master key is computed as it is no longer needed. We wouldn’t be able to compute this for old wallets.

Wow yeah, I didn’t realize descriptor wallets actually generate the seed but then throw it away only keeping the master key. I guess there are pros and cons to that, but it’s another topic and mostly unrelated.

achow101 force-pushed on Dec 21, 2023

achow101 commented at 3:56 am on December 21, 2023: member

Instead of returning an object with “xpub” and “xpriv” fields, it would return a list of objects with “xpub”, “xpriv”, and “descriptors” fields. The “descriptors” field would contain a list of descriptors using the hd key.

It would also be good for the method to accept an “active_only” option, and only return hd keys associated with active descriptors by default.

This would make it easy to get the hd key, and easy to understand how the hd key is referenced by descriptors. In the typical case it would only return 1 hd key, and in more complicated cases it would be easier to understand than the gethdmethod in this PR, because it would give you all the information about hd keys, instead of just choosing one hd key and not telling you which descriptors it is associated with.

The approach I would suggest is making the hd key an optional parameter to createwalletdescriptor, and if it is not specified, using the same hd key as other active descriptors. Unless I am misunderstanding something, in normal cases we would expect there to be exactly one hd key. But in cases where there is more than one key, we could choose the active descriptor for the default output type (like getnewaddress), or be more conservative and raise an exception asking for an hd key to be specified to disambiguate.

For cases where the hd key is specified to createwalletdescriptor, I think we would want it to be flexible and accept multiple formats, but accepting “xpub” value from gethdkey would be an obvious start. It could also accept the xpriv value, or even the seed keys or bytes. If it accepted private keys or seeds, we could treat it as a generalized alternative to sethdseed, which would be great because it would simplify usage, and let us deprecate the sethdseed method instead of keeping both methods.

Hmm. I suppose that could be a viable approach. I will experiment with it.

I know that @Sjors has some ideas for gethdkey/getxpub but I’m not sure if those are compatible with this proposed approach. Otherwise, it does seem like it would cover the use cases targeted by this PR and it’s descendant.

S3RK commented at 7:58 am on December 21, 2023: contributor

IIUC the approach proposed by @ryanofsky is to basically figure out the key on demand instead of storing it in the DB.

So this works for generating tr() descriptors for existing wallets, but it doesn’t work that well for #24861. The desired flow is to have a wallet with HD key but without descriptors and then retrieve an xpub to construct multisig descriptor. How would it work if we don’t have an HD key associated with the wallet?

In my opinion, approach proposed in this PR resolves a regression we had with transition to descriptor wallets. In legacy we actually had a proper HD key residing in a wallet. And this is the same thing we want to bring back to descriptors. 1 wallet = 1 HD key. Yes, the matter is complicated by the fact that users can add any other keys they wish, but the model and the default flow are actually pretty straightforward.

Sjors commented at 9:15 am on December 21, 2023: member

@achow101 wrote:

I’ve tried a couple of different approaches, and they all have drawbacks in some form:

Generate a new hd key and use that for the descriptor.

Another drawback of that approach is that it requires a new backup, e.g. for anyone who just wants to add taproot support to a wallet that doesn’t have it yet.

Something I still need to do, but waiting for this PR to land, is to enable adding a tr() to a external signer wallet. Hardware wallets would typically derive those from the same master seed, though from our end that ~~doesn’t~~ shouldn’t matter; external signer wallets are watch-only. Well, except that we identify the physical device by the fingerprint of its master key. @s3rk wrote:

The desired flow is to have a wallet with HD key but without descriptors and then retrieve an xpub to construct multisig descriptor. How would it work if we don’t have an HD key associated with the wallet?

Indeed. There’s different proposals out there of how to derive keys for a multisig wallet, but I’m mainly thinking of two approaches:

Take an existing wallet with the usual descriptors, pick an unused derivation path (e.g. m/48'/..., based on BIP 48) and share that with the other participants. You then import the resulting multisig descriptor.
Create a fresh wallet with only a seed / master key, and no descriptors. Pick any derivation… (same as 1)

I prefer (2) because I think you shouldn’t co-mingle multisig and single sig funds in the same wallet, for privacy reason and because it’s potentially confusing. Multisig setups always require a new backup, so if we need to generate a fresh master key that’s not a big deal to me.

wallet: Automatically upgrade a wallet to have global hd key 954bfa6b4f

tests: Test for gethdkey 31b43b6a83

test: Test automatic upgrade of descriptor wallets to have hd key 09849f974a

wallet: Set global hd key for migrated wallets 5a5b351548

test: Also do backwards compat test with encrypted wallets

Best reviewed with `git show -w`

fccde79863

walletdb: Check that unencrypted and crypted keys are mutually exclusive d2ecd4f666

test: Check that no unencrypted records persist after encrypting 8a54bfe043

test: add coverage for re-opening a downgraded encrypted wallet on master

The test creates a wallet on master, downgrades and encrypts the wallet.
Then, it tries to open it again on master.

d1caefc0e1

achow101 force-pushed on Dec 21, 2023

ryanofsky commented at 4:50 pm on December 21, 2023: contributor

@Sjors and @S3RK, can you help me out by being specific about the flows you have in mind? Like in terms of what RPC calls users will make to do a particular operation? I’m trying to fill in the gaps myself, but in the cases I can think of, it seems like adding the concept of a sticky master key doesn’t actually make things simpler, just more opaque and confusing.

I understand that if you were designing a new wallet model and API, you would both prefer an approach that required all active descriptors to have a single master key, and would take advantage of having that master key to simplify certain things.

But I see real drawbacks to usability and code complexity if we are going to try to pretend that the wallet only has an single active hd key while not actually enforcing this. I think you if compare the API implemented in this PR and its followups to the API proposed in #26728#pullrequestreview-1791999003 I think you would see the same problems and hopefully agree. To be specific:

The gethdkey method implemented in this PR currently picks one master key used by descriptors, and ignores others that may be present. The proposed gethdkey described in #26728 (review) would return any hd key that is actively being used to generate addresses in the wallet by default (and would return inactive hd keys with an option) and also list descriptors associated with hd keys. This should be better and less confusing in every case compared to the implementation in this PR which just returns a single hd key with no descriptor information, with the key chosen in a complicated way that is sticky and can’t be controlled by the user, even if they import new descriptors and set them to be active.
The createwalletdescriptor method implemented in #25907 and sethdseed method added for descriptor wallets in #29054 have overlapping functionality and confusing limitations that would be avoided by dropping sethdseed and making createwalletdescriptor more flexible as described #26728 (review) .
IIUC, the importdescriptor extension from #23544 to allow importing descriptors with known xpubs seems like it would be more robust and less confusing if it just worked with any xpub in the wallet or any active xpub, instead of only working for the sticky master key and failing with an error about not having private keys which are actually present.
IIUC, the importdescriptor extension from #27351 would also seem to work more smoothly with the suggested approach, because there would be no need for a separate sethdseed call, and the descriptor and master key would show up together in gethdkey output.

For external signing and multisig, I could believe in theory that there are cases that may be more complicated by not having a sticky hd key. But I haven’t seen examples of that yet, and don’t think it should be hard to come up with them if they exist. Also I wouldn’t want to make the wallet model more complicated prematurely if it seems like all the improvements we want to make are possible without this.

re: #26728 (comment)

it doesn’t work that well for #24861. The desired flow is to have a wallet with HD key but without descriptors and then retrieve an xpub to construct multisig descriptor. How would it work if we don’t have an HD key associated with the wallet?

This flow actually sounds more complicated than what I would expect. If you have a blank wallet, why would you want creating a multisig desriptor and creating the hd key to be two separate steps, and require a separate sethdseed call before whatever call is used to add the multisig descriptor? Wouldn’t the flow be simpler if you just made one call to createwalletdescriptor or importdescriptor and it did both things? (EDIT: Actually rereading this, I can see why this wouldn’t work because when you have multiple wallets you need to generate all the seeds first before you can generate the descriptor. I’m still not sure that requires all the complexity in this PR, but it does suggest why it is there.) In the case of a non-blank wallet, I also think not having a sticky hd key would be more transparent. Creating a multisig desciptor in that case could use the wallet’s one hd key if there is one or require one to be specified if there are multiple.

In my opinion, approach proposed in this PR resolves a regression we had with transition to descriptor wallets. In legacy we actually had a proper HD key residing in a wallet.

Yes I can see the aspiration to do this, and I could imagine a different wallet model with a less flexible importdescriptor RPC that required all descriptor to have the same hd key, or required any descriptors with different hd keys to be inactive. But this is not the current wallet model we have, and it seems like at this point trying to bolt on the concept of a having one hd key when wallet doesn’t actually enforce it just seems messy and possibly the worst of both worlds.

re: #26728 (comment)

Something I still need to do, but waiting for this PR to land, is to enable adding a tr() to a external signer wallet. Hardware wallets would typically derive those from the same master seed, though from our end that doesn’t shouldn’t matter; external signer wallets are watch-only. Well, except that we identify the physical device by the fingerprint of its master key.

I think I am probably missing the point and don’t have enough context, but I don’t see how the current PR helps with this. I don’t see how the wallet having a sticky hd key would help working with an external signer or creating a tr() descriptor. Like in general I could see how the wallet storing more information could help it work with external signers, but I don’t understand how storing this particular bit of information would help.

Indeed. There’s different proposals out there of how to derive keys for a multisig wallet, but I’m mainly thinking of two approaches […] I prefer (2)

I see, but that suggests this PR is good because the limitations it adds will force users to choose approach (2). So if you have a wallet with default descriptors you can’t import a multisig using the same hd key, and if you have a wallet with a multisig descriptor you can’t create default descriptors with the same hd key. But it seems like the approach in this PR would only be enforcing that selectively, and the resulting behavior would probably just be confusing. Wouldn’t it be simpler and clearer to just have explicit checks when adding descriptors for the combinations of descriptors you don’t want to allow? Again here I think it would be helpful to be more concrete and think about the specific RPC methods that would be used for these flows.

I definitely don’t understand everything and there could be real reasons why the approach in this PR makes sense. Just so far I haven’t seen what the benefits are, and the drawbacks seem unpleasant.

ryanofsky commented at 6:40 pm on December 21, 2023: contributor

re: #26728 (comment)

If you have a blank wallet, why would you want creating a multisig desriptor and creating the hd key to be two separate steps, and require a separate sethdseed call before whatever call is used to add the multisig descriptor? Wouldn’t the flow be simpler if you just made one call to createwalletdescriptor or importdescriptor and it did both things? (EDIT: Actually rereading this, I can see why this wouldn’t work because when you have multiple wallets you need to generate all the seeds first before you can generate the descriptor. I’m still not sure that requires all the complexity in this PR, but it does suggest why it is there.)

Replying to my own comment, since I had a realization after I posted it and made the edit above.

I think neither the approach in this PR nor the suggested approach in #26728#pullrequestreview-1791999003 will support multisig well by themselves. But I think this PR’s approach of deriving a master key from existing (non-multisig) descriptors, storing it separately, and adding new wallet code in followup PRs to rely on it puts us in a worse starting place for providing a good multisig UX than the suggested approach. Mostly because of the complexity it adds, but also because it does not assign a specific purpose to the HD key, so it could encourages uses that “co-mingle multisig and single sig funds in the same wallet” like Sjors was trying to avoid.

I don’t think the flow for how we support multisig has to be decided now, since it’s beyond the scope of this PR. But I do want to spell out how I think things could work more simply without this PR. I think to support multisig and other more complicated cases well we will want to give the wallet the ability to store an HD master key not associated with any descriptor, and which can be used later to create complicated descriptors. But this could be supported with a new wallet field holding a CExtPubKey that is only set temporarily between creating the master key and creating a descriptor that uses it. It would need to be accompanied by a wallet flag preventing older wallets from using the wallet while the field exists, but as soon as the first descriptor is created using the master key, the flag and temporary field could be deleted, and the wallet would be backward compatible again and stay within our current data model.

This would allow a simpler flow for creating multisig wallets without adding a permanent burden and increase in complexity after they are created.

ryanofsky commented at 9:36 pm on December 21, 2023: contributor

achow101, S3RK, Sjors, and me talked about this on a call. I think the plan for now is that achow will make 2 independent PRs targeted at different use-cases:

First PR would add a gethdkey method like the one in this PR and a createwalletdescriptor method like the one in #25907 that should both make reusing existing hd keys easier. Specifically these methods should be useful for adding new types of descriptors to existing wallets and upgrading existing wallets. Unlike the current PRs, this new PR would be fully backwards compatible and not change the database format.
Second PR would implement a sethdseed implementation for descriptor wallets like #29054 or a similar method that generates standalone hd keys not (yet) associated with any descriptor. This would be useful for creating multisig descriptors. The new keys would need to be stored in a new database field, or possibly multiple fields, so this would change the database format, but in a simple way that should avoid upgrade/downgrade pitfalls encountered in previous iterations of this PR with DBKeys::ACTIVEHDKEY. If we are concerned with having more than one HD key per wallet, this could be conservative about when it allows creating standalone HD keys.

~~S3RK~~ achow made a wiki page https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Use-Cases where we can start to look at different use cases, and what RPC calls would look like under different flows and models. On the call S3RK made the point that a model where wallets have exactly one active hd key at a time would be more consistent with other wallet software and more naturally align with the BIPs. I think this is definitely true, and I think we should optimize wallet UX for the case where there is only one active hd key, but also not be broken or confusing in cases where there is more than one hd key. So far, I haven’t seen specific cases where supporting more than one hd key seems to hurt the UX or implementation, so I’m still skeptical of the idea that there is something wrong or bad about our current wallet model, even if it is different. But I think things should become clearer as we add features, and I think the two PR’s above will keep our options open.

Update: replacement for this PR which adds gethdkey and #25907 which adds createwalletdescriptor is #29130 which adds both methods. Replacement for #29054 which adds sethdseed for descriptor wallets is #29136 which introduces an addhdkey method.

ryanofsky commented at 2:01 pm on December 22, 2023: contributor

re: #26728 (comment)

The new keys would need to be stored in a new database field

This is not actually true. Thinking about this more, I wonder if a more natural way of representing standalone keys in the wallet is just to treat them as a new descriptor type, like void(KEY), where the void descriptor would hold the key, but instead of expanding to a scriptPubKey or set of scriptPubKeys like other descriptors it would expand to the empty set.

We could treat this as an internal implementation detail, just using void(xpub....) as a way for the wallet to reference unused hd keys. But there could be benefits to using the type externally. For example, if we included these in listdescriptors output, users could continue using listdescriptors as a form of wallet backup, or way to see all the wallet keys a wallet contains, without having to call a new RPC method like gethdkey.

But I think the main advantage of this approach would just be fitting better into the existing database format, keeping it clean and easy to reason about, and also having more backwards compatibility.

The idea of naming the descriptor void(KEY) just comes the void operator in javascript, and void type in c/c++/java which consume values without returning returning or signify functions that don’t return values.

Sjors commented at 2:32 pm on December 22, 2023: member

@ryanofsky interesting idea, I especially like the backup implication, because descriptors can be used for paper backups.

ryanofsky commented at 4:31 pm on December 22, 2023: contributor

Thanks, I was also trying to think of names for these other than void descriptors. For example maybe they could be called data descriptors and written like data(xpub…) to convey the idea that these descriptors are just used to hold data and don’t produce scriptpubkeys.

achow101 commented at 4:49 pm on December 22, 2023: member

This is not actually true. Thinking about this more, I wonder if a more natural way of representing standalone keys in the wallet is just to treat them as a new descriptor type, like void(KEY), where the void descriptor would hold the key, but instead of expanding to a scriptPubKey or set of scriptPubKeys like other descriptors it would expand to the empty set.

Nice idea, I think that could work. These could be imported with importdescriptors. If the user wants their wallet to generate a key automatically though, we’ll still need a sethdseed equivalent (probably name it something else since it’s not the same as setting a hd seed).

achow101 commented at 0:05 am on January 6, 2024: member

Superseded by #29130

I don’t think there’s a reason to come back to this design, but the branch won’t be deleted so we can reopen if desired.

achow101 closed this on Jan 6, 2024

wallet: Have the wallet store the key for automatically generated descriptors #26728

Code Coverage

Reviews

Conflicts