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.
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
3d4e3b9cf3outdated
199@@ -200,6 +200,12 @@ struct PubkeyProvider
200201 /** 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.
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
abac3e3630outdated
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};
252253+ //! 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);
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.
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.
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
9a17347086outdated
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
9a17347086outdated
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),
4950+ // Indicates that the wallet is capable of having a wallet hd key
51+ WALLET_FLAG_GLOBAL_HD_KEY = (1ULL << 3),
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?
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
b756f221ccoutdated
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"]
328329+ if self.options.descriptors:
330+ self.log.info("Test background upgrading of descriptor wallets to have hd key")
331+ for node in descriptor_nodes:
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
b756f221ccoutdated
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"]
328329+ 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)
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.
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
8b0af8a56foutdated
3572@@ -3573,6 +3573,10 @@ void CWallet::SetupDescriptorScriptPubKeyMans()
3573 CExtKey master_key;
3574 master_key.SetSeed(seed_key);
35753576+ // Store the master key as our active hd key
3577+ SetActiveHDKey(master_key.Neuter());
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
8b0af8a56foutdated
3572@@ -3573,6 +3573,10 @@ void CWallet::SetupDescriptorScriptPubKeyMans()
3573 CExtKey master_key;
3574 master_key.SetSeed(seed_key);
35753576+ // Store the master key as our active hd key
3577+ SetActiveHDKey(master_key.Neuter());
3578+ AddHDKey(master_key);
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
95da2e9808outdated
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.
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
436eab602foutdated
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
6072b39645outdated
199@@ -200,6 +200,12 @@ struct PubkeyProvider
200201 /** 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
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
6072b39645outdated
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;
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
9a17347086outdated
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
9a17347086outdated
1086@@ -1087,6 +1087,99 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
1087 }
1088 }
10891090+ // 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
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
9ec69417b3outdated
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
0c4b2bc687outdated
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
6bb3bee37eoutdated
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+ ) {
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
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?
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
0571caf632outdated
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) {
in
src/wallet/wallet.cpp:564
in
0571caf632outdated
521@@ -522,6 +522,121 @@ void CWallet::UpgradeDescriptorCache()
522 SetWalletFlag(WALLET_FLAG_LAST_HARDENED_XPUB_CACHED);
523 }
524525+/**
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
If they’re no longer active, we don’t know internalness.
in
src/wallet/wallet.cpp:538
in
0571caf632outdated
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)
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.
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
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
c79539fc20outdated
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
in
src/wallet/wallet.cpp:556
in
95d11f09c2outdated
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
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.
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
utACKc79539fc20d902b5f9e098cc996c4e27c8e5b8c5
Everything since my last review is a rebase, except 95d11f09c2c0d4ad66ba2e4ce2d15be4c002fb94, so I only re-reviewed that, plus 70a2bb75442daaef5685728b4b03c7b2fe49fd38.
S3RK
commented at 7:16 am on June 7, 2023:
contributor
tACKc79539fc20d902b5f9e098cc996c4e27c8e5b8c5
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
ad6971fbf8outdated
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);
29532954- walletInstance->InitWalletFlags(wallet_creation_flags);
2955+ walletInstance->InitWalletFlags(wallet_creation_flags | WALLET_FLAG_GLOBAL_HD_KEY);
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.
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-ACK481f63e
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
3de9672a60outdated
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;
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)
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:
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
87abceeab4outdated
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.
@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
7bbe3b48c3outdated
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
079c74ed40outdated
3577@@ -3578,6 +3578,12 @@ void CWallet::SetupDescriptorScriptPubKeyMans()
3578 CExtKey master_key;
3579 master_key.SetSeed(seed_key);
35803581+ // 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
079c74ed40outdated
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
37527c9c69outdated
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?
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
474eceb8a5outdated
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:
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':
12test/functional/wallet_encryption.py: with open(dump_file, "r") as f:
in
test/functional/wallet_encryption.py:62
in
474eceb8a5outdated
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 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
utACKe986d5a15174c8abf6979f1c03a04913e7c2e90f
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
75ded425eaoutdated
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
283fe566d6outdated
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+ }
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");
3return false;
4}
achow101
commented at 4:03 pm on October 24, 2023:
Done as suggested
in
src/wallet/wallet.cpp:4055
in
283fe566d6outdated
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());
0if (!SetActiveHDKey(data.master_key.Neuter())) {
1 error += _("Error: Unable to set active HD key");
2return false;
3}
achow101
commented at 4:03 pm on October 24, 2023:
Done as suggested
in
test/functional/wallet_encryption.py:54
in
e986d5a151outdated
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')
5051+ # 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")
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
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
e986d5a151outdated
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')
5051+ # 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)
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 ACKdfb58e4
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 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-utACKb1b12d1b88492fe66b26243760689c7f040c67c2
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
8a24755732outdated
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
4ad97b519eoutdated
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
achow101 force-pushed
on Nov 7, 2023
in
src/wallet/walletdb.cpp:308
in
95153f0152outdated
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.
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
96365a0b18outdated
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);
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
9900207360outdated
808@@ -809,6 +809,61 @@ static RPCHelpMan migratewallet()
809 };
810 }
811812+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 "
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
9900207360outdated
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");
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
18dc28f14eoutdated
642+ }
643+ }
644+ }
645+
646+ // For all candidate xpubs, add corresponding private keys if available
647+ for (const auto& [id_pair, key] : desc_keys) {
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
52697ecc3foutdated
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)
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
e3c292b298outdated
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"
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
achow101 force-pushed
on Nov 16, 2023
in
src/wallet/walletdb.cpp:1009
in
9426d3e7e5outdated
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);
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
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:
contributor
The WalletAvailableCoins benchmark crashes on this pull:
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.
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
14738e9a12outdated
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};
317318+ //! The extended key used for new automatically generated descriptors
319+ CExtPubKey m_xpub GUARDED_BY(cs_wallet);
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
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
a86756aed2outdated
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.
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
e491e5c4e6outdated
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.
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
e491e5c4e6outdated
981@@ -973,6 +982,12 @@ static DBErrors LoadDescriptorWalletRecords(CWallet* pwallet, DatabaseBatch& bat
982 return DBErrors::CORRUPT;
983 }
984985+ // 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");
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:
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
e67898f471outdated
965@@ -940,14 +966,20 @@ static DBErrors LoadDescriptorWalletRecords(CWallet* pwallet, DatabaseBatch& bat
966967 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");
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
a5ba1d8382outdated
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;
377378+ // 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;
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
ea7a61ca94outdated
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
achow101 force-pushed
on Dec 11, 2023
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
ce5495a4b2outdated
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
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
8bb7a8b06foutdated
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);
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
2e840bcfc0outdated
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
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
db6b61e9e7outdated
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
db6b61e9e7outdated
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
db6b61e9e7outdated
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
db6b61e9e7outdated
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);
12ASSERT_ELSE(m_hd_crypted_key.empty(), return std::nullopt);
34ASSERT_ELSE(pwallet->IsWalletFlagSet(WALLET_FLAG_GLOBAL_HD_KEY),
5throw JSONRPCError(RPC_WALLET_ERROR, "Wallet has not been upgraded to support global hd keys"));
in
src/wallet/wallet.h:322
in
db6b61e9e7outdated
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};
318319+ //! 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:
0structHDKey {
1 CExtPubKey xpub;
2//! Variant storing CKey if the wallet is unencrypted, otherwise the encrypted key.
3 std::variant<CKey, std::vector<unsignedchar>> 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
2e840bcfc0outdated
1042@@ -1038,6 +1043,9 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
10431044 //! 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
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 record993e5da9cb
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 descriptors5a60d83edb
wallet: Retrieve active hd pubkey5c9f287ef1
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 GetActiveHDPrivkey3342b99adf
wallet, rpc: Add gethdkey RPC6de30ebbb3
descriptor: Be able to get the pubkeys involved in a descriptor17fce45eb1
wallet: Always set WALLET_FLAG_GLOBAL_HD_KEY for new wallets71d9399f65
wallet, rpc: Check WALLET_FLAG_GLOBAL_HD_KEY in gethdkey2bcccba704
achow101 force-pushed
on Dec 21, 2023
DrahtBot removed the label
CI failed
on Dec 21, 2023
in
src/wallet/walletdb.cpp:829
in
442962d441outdated
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
98b6fad226outdated
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
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 key954bfa6b4f
tests: Test for gethdkey31b43b6a83
test: Test automatic upgrade of descriptor wallets to have hd key09849f974a
wallet: Set global hd key for migrated wallets5a5b351548
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 exclusived2ecd4f666
test: Check that no unencrypted records persist after encrypting8a54bfe043
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.
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.
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
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
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
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2025-04-02 18:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me