wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively

achow101 commented at 2:55 am on August 24, 2023: member

Instead of constructing ScriptPubKeyMans with no data, and then loading data as we find it, we should gather everything first and then load it all on construction. If there actually is no data and we want to setup generation, then that should also occur in a constructor rather than afterwards.

This change is only applied to DescriptorScriptPubKeyMan and ExternalSignerScriptPubKeyMan, and should be done for any ScriptPubKeyMans added in the future. I don’t think it’s really worth it to do this for LegacyScriptPubKeyMan since it would make loading performance worse (or cause layer violations) and it’s (supposed to be) going away soon.

DrahtBot commented at 2:55 am on August 24, 2023: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/28333.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	rkrux

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33043 ([POC] wallet: Enable non-electronic (paper-based) wallet backup with codex32 by w0xlt)
#32966 (Silent Payments: Receiving by Eunovo)
#32861 (Have createwalletdescriptor auto-detect an unused(KEY) by Sjors)
#32784 (wallet: derivehdkey RPC to get xpub at arbitrary path by Sjors)
#32652 (wallet: add codex32 argument to addhdkey by roconnor-blockstream)
#28201 (Silent Payments: sending by josibake)

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 Aug 24, 2023

in src/wallet/wallet.cpp:424 in ca729a7d60 outdated

390@@ -391,40 +391,18 @@ std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string&
391         return nullptr;
392     }
393 
394+    // Unset the blank flag if not specified by the user
395+    if (!create_blank) {

S3RK commented at 7:00 am on August 28, 2023:

Is this change of behaviour or not?

achow101 commented at 5:56 pm on August 29, 2023:

It is not.

EncryptWallet does not end up calling the SetupGeneration functions when the blank flag is set. Previously, we would set those up manually after encrypting. While making this PR, I realized that we could just unset the blank flag before calling EncryptWallet so that it can do the setup for us and we don’t have to do it again afterwards.

unknown approved

DrahtBot added the label CI failed on Sep 3, 2023

DrahtBot removed the label CI failed on Sep 5, 2023

DrahtBot added the label CI failed on Nov 25, 2023

maflcko commented at 1:35 pm on November 27, 2023: member

0wallet/test/fuzz/scriptpubkeyman.cpp: In lambda function:
1wallet/test/fuzz/scriptpubkeyman.cpp:114:67: error: ‘void wallet::DescriptorScriptPubKeyMan::AddDescriptorKey(const CKey&, const CPubKey&)’ is private within this context
2  114 |                 spk_manager->AddDescriptorKey(key, key.GetPubKey());
3      |

achow101 force-pushed on Nov 27, 2023

DrahtBot removed the label CI failed on Nov 27, 2023

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

achow101 force-pushed on Dec 7, 2023

DrahtBot added the label Needs rebase on Dec 8, 2023

achow101 force-pushed on Dec 8, 2023

DrahtBot removed the label Needs rebase on Dec 8, 2023

DrahtBot added the label CI failed on Jan 12, 2024

DrahtBot added the label Needs rebase on Feb 20, 2024

achow101 force-pushed on Feb 20, 2024

DrahtBot removed the label Needs rebase on Feb 20, 2024

DrahtBot added the label Needs rebase on Mar 29, 2024

achow101 force-pushed on Mar 29, 2024

DrahtBot removed the label Needs rebase on Mar 29, 2024

achow101 force-pushed on Apr 3, 2024

achow101 commented at 10:41 pm on April 3, 2024: member

Added a commit so that the fuzzer will skip the type of inputs that was causing the previous failure. This should be safe as that failure is not reachable in normal usage. It can only be reached if the user’s wallet is corrupted, and in that case, the runtime error would propagate up to them so there would not be a crash.

DrahtBot removed the label CI failed on Apr 4, 2024

DrahtBot added the label Needs rebase on Jun 3, 2024

achow101 force-pushed on Jun 4, 2024

DrahtBot removed the label Needs rebase on Jun 5, 2024

DrahtBot added the label Needs rebase on Jul 11, 2024

achow101 force-pushed on Jul 22, 2024

DrahtBot removed the label Needs rebase on Jul 22, 2024

DrahtBot added the label Needs rebase on Aug 12, 2024

achow101 force-pushed on Aug 29, 2024

DrahtBot removed the label Needs rebase on Aug 29, 2024

achow101 requested review from murchandamus on Oct 15, 2024

DrahtBot added the label Needs rebase on Oct 24, 2024

achow101 force-pushed on Oct 28, 2024

DrahtBot added the label CI failed on Oct 28, 2024

DrahtBot commented at 7:24 pm on October 28, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/32177937035

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

achow101 force-pushed on Oct 28, 2024

DrahtBot removed the label Needs rebase on Oct 28, 2024

DrahtBot removed the label CI failed on Oct 28, 2024

DrahtBot added the label Needs rebase on Apr 23, 2025

achow101 force-pushed on Apr 24, 2025

DrahtBot removed the label Needs rebase on Apr 24, 2025

DrahtBot added the label Needs rebase on Apr 25, 2025

achow101 force-pushed on Apr 25, 2025

DrahtBot removed the label Needs rebase on Apr 25, 2025

DrahtBot added the label Needs rebase on May 7, 2025

achow101 force-pushed on May 7, 2025

DrahtBot removed the label Needs rebase on May 7, 2025

DrahtBot added the label Needs rebase on May 19, 2025

achow101 force-pushed on May 20, 2025

DrahtBot removed the label Needs rebase on May 20, 2025

in src/wallet/scriptpubkeyman.h:317 in 2cee4def97 outdated

308@@ -317,22 +309,33 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan
309     // Fetch the SigningProvider for a given index and optionally include private keys. Called by the above functions.
310     std::unique_ptr<FlatSigningProvider> GetSigningProvider(int32_t index, bool include_private = false) const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man);
311 
312+    void SetCache(const DescriptorCache& cache);
313+
314+    void AddDescriptorKey(const CKey& key, const CPubKey &pubkey);
315+    void UpdateWithSigningProvider(WalletBatch& batch, const FlatSigningProvider& signing_provider) EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man);
316+
317+    //! Setup descriptors based on the given CExtkey

DrahtBot commented at 9:35 am on May 21, 2025:

CExtkey -> CExtKey [typographical error in type name]

DrahtBot added the label Needs rebase on May 21, 2025

achow101 force-pushed on May 21, 2025

DrahtBot removed the label Needs rebase on May 21, 2025

DrahtBot added the label Needs rebase on Jun 14, 2025

achow101 force-pushed on Jun 14, 2025

DrahtBot removed the label Needs rebase on Jun 14, 2025

rkrux commented at 10:03 am on July 11, 2025: contributor

I seem to be getting onboard with this idea as I can notice readability improvements on a cursory glance. Do you anticipate any performance improvements as well?

The last sentence related to legacy SPKMs of the last paragraph in the PR description can be removed now.

Will do full review soon.

in src/wallet/wallet.cpp:429 in 32dd0e3525 outdated

424+    if (!create_blank) {
425+        wallet->UnsetWalletFlag(WALLET_FLAG_BLANK_WALLET);
426+    }
427+
428     // Encrypt the wallet
429     if (!passphrase.empty() && !(wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {

rkrux commented at 10:49 am on July 15, 2025:

In 32dd0e35251973f9bdc2114e70c800624894d489 “wallet: Consolidate generation setup callers into one function”

While this flow is being cleaned up. There is a check specially for this flag above on line 400 that forbids this situation and the secondary check seems redundant here now.

0- if (!passphrase.empty() && !(wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
1+ if (!passphrase.empty()) {

achow101 commented at 9:44 pm on July 24, 2025:

Done

in src/wallet/wallet.cpp:424 in 32dd0e3525 outdated

419@@ -420,30 +420,18 @@ std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string&
420         return nullptr;
421     }
422 
423+    // Unset the blank flag if not specified by the user
424+    if (!create_blank) {

rkrux commented at 10:54 am on July 15, 2025:

In https://github.com/bitcoin/bitcoin/commit/32dd0e35251973f9bdc2114e70c800624894d489 “wallet: Consolidate generation setup callers into one function”

Fine as-is but perhaps this block can be put inside the if block below because from what I see if not set by the user, this flag can only be set in case of passphrase not being empty on line 388. This wrapped inside a tighter constraint such as the one below looks better to me.

achow101 commented at 9:24 pm on July 24, 2025:

The blank flag needs to be unset before EncryptWallet so that EncryptWallet can generate the new keys after encrypting. Otherwise, the newly created wallet will be encrypted but blank, which is not what we want in this scenario.

rkrux commented at 7:28 am on July 25, 2025:

I had something like this in mind and I should have left a diff earlier for clarity. It seemed to me that only for encrypted wallets the blank flag was set by the code & hence, believed this flag unsetting could be nested inside the below condition.

 0diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
 1index bd1b36b4a9..5a3ecc1d19 100644
 2--- a/src/wallet/wallet.cpp
 3+++ b/src/wallet/wallet.cpp
 4@@ -381,9 +381,6 @@ std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string&
 5     Assert(wallet_creation_flags & WALLET_FLAG_DESCRIPTORS);
 6     options.require_format = DatabaseFormat::SQLITE;
 7 
 8-    // Indicate that the wallet is actually supposed to be blank and not just blank to make it encrypted
 9-    bool create_blank = (wallet_creation_flags & WALLET_FLAG_BLANK_WALLET);
10-
11     // Born encrypted wallets need to be created blank first.
12     if (!passphrase.empty()) {
13         wallet_creation_flags |= WALLET_FLAG_BLANK_WALLET;
14@@ -420,13 +417,10 @@ std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string&
15         return nullptr;
16     }
17 
18-    // Unset the blank flag if not specified by the user
19-    if (!create_blank) {
20-        wallet->UnsetWalletFlag(WALLET_FLAG_BLANK_WALLET);
21-    }
22-
23     // Encrypt the wallet
24     if (!passphrase.empty()) {
25+        // Unset the blank flag that was purposefully set above for encrypted wallets
26+        wallet->UnsetWalletFlag(WALLET_FLAG_BLANK_WALLET);
27         if (!wallet->EncryptWallet(passphrase)) {
28             error = Untranslated("Error: Wallet created but failed to encrypt.");
29             status = DatabaseStatus::FAILED_ENCRYPT;

But I check now that this fails the test for intentionally blank encrypted wallets. So, this suggestion is incorrect and need not be implemented.

0def test_encrypted(self):
1        self.log.info("Test createwalletdescriptor with encrypted wallets")
2        def_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name)
3        self.nodes[0].createwallet("encrypted", blank=True, passphrase="pass")

in src/wallet/wallet.cpp:3582 in 32dd0e3525 outdated

3585+        (IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET) || IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS))) {
3586+        return true;
3587+    }
3588+    SetupDescriptorScriptPubKeyMans();
3589+    return true;
3590+}

rkrux commented at 11:01 am on July 15, 2025:

In https://github.com/bitcoin/bitcoin/commit/32dd0e35251973f9bdc2114e70c800624894d489 “wallet: Consolidate generation setup callers into one function”

This never seems to return false. Do we need to check for a falsy value while calling it like done above? Maybe not even return a bool? Just a thrown exception that is bubbled up.

achow101 commented at 9:44 pm on July 24, 2025:

Made void

in src/wallet/walletdb.cpp:905 in fdba094a67 outdated

901         });
902         result = std::max(result, key_res.m_result);
903         num_keys = key_res.m_records;
904 
905         // Get encrypted keys
906+        std::map<CKeyID, std::pair<CPubKey, std::vector<unsigned char>>> ckeys;

rkrux commented at 11:23 am on July 15, 2025:

In fdba094a67906cc89918661e24c9c9a5e3f4cb0b “wallet: Load everything into DescSPKM on construction”

0- std::map<CKeyID, std::pair<CPubKey, std::vector<unsigned char>>> ckeys;
1+ CryptedKeyMap ckeys;

achow101 commented at 9:44 pm on July 24, 2025:

Done

in src/wallet/walletdb.cpp:860 in fdba094a67 outdated

859-        spk_man->SetCache(cache);
860+        // Set the cache to the WalletDescriptor
861+        desc.cache = cache;
862 
863         // Get unencrypted keys
864+        std::map<CKeyID, CKey> keys;

rkrux commented at 11:24 am on July 15, 2025:

In fdba094a67906cc89918661e24c9c9a5e3f4cb0b “wallet: Load everything into DescSPKM on construction”

0- std::map<CKeyID, CKey> keys;
1+ KeyMap keys;

achow101 commented at 9:44 pm on July 24, 2025:

Done

in src/wallet/scriptpubkeyman.h:179 in fdba094a67 outdated

172@@ -173,15 +173,18 @@ static const std::unordered_set<OutputType> LEGACY_OUTPUT_TYPES {
173     OutputType::BECH32,
174 };
175 
176+using WatchOnlySet = std::set<CScript>;
177+using WatchKeyMap = std::map<CKeyID, CPubKey>;
178+using KeyMap = std::map<CKeyID, CKey>;
179+using CryptedKeyMap = std::map<CKeyID, std::pair<CPubKey, std::vector<unsigned char>>>;
180+using ScriptPubKeyMap = std::map<CScript, int32_t>; // Map of scripts to descriptor range index
181+using PubKeyMap = std::map<CPubKey, int32_t>; // Map of pubkeys involved in scripts to descriptor range index

rkrux commented at 11:26 am on July 15, 2025:

In fdba094a67906cc89918661e24c9c9a5e3f4cb0b “wallet: Load everything into DescSPKM on construction”

Some of them are now being used in wallet.cpp & external_signer_scriptpubkeyman.h and I suggested using couple in walletdb.cpp too. Maybe they can now go in a utility file like walletutil.h?

achow101 commented at 9:44 pm on July 24, 2025:

I think leaving them here is fine.

in src/wallet/scriptpubkeyman.h:177 in fdba094a67 outdated

172@@ -173,15 +173,18 @@ static const std::unordered_set<OutputType> LEGACY_OUTPUT_TYPES {
173     OutputType::BECH32,
174 };
175 
176+using WatchOnlySet = std::set<CScript>;
177+using WatchKeyMap = std::map<CKeyID, CPubKey>;

rkrux commented at 11:32 am on July 15, 2025:

In https://github.com/bitcoin/bitcoin/commit/fdba094a67906cc89918661e24c9c9a5e3f4cb0b “wallet: Load everything into DescSPKM on construction”

I feel these 2 can be kept inside LegacyDataSPKM itself, they are not used anywhere else, and I don’t suppose they will be as well in the future.

achow101 commented at 9:45 pm on July 24, 2025:

Done

in src/wallet/scriptpubkeyman.h:335 in 93c3729ad5 outdated

331@@ -324,10 +332,8 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan
332     DescriptorScriptPubKeyMan(WalletBatch& batch, WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const FlatSigningProvider& provider);
333     //! Create a DescriptorScriptPubKeyMan from existing data (i.e. during loading)
334     DescriptorScriptPubKeyMan(WalletStorage& storage, const uint256& id, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys);
335-    DescriptorScriptPubKeyMan(WalletStorage& storage, int64_t keypool_size)
336-        :   ScriptPubKeyMan(storage),
337-            m_keypool_size(keypool_size)
338-        {}
339+    //! Create an automatically generated DescriptorScriptPubKeyMan

rkrux commented at 11:53 am on July 15, 2025:

In 93c3729ad5adc02f6d87ec8daa1a1952d7c41009 “wallet: Setup new autogenerated descriptors on construction”

0- //! Create an automatically generated DescriptorScriptPubKeyMan
1+ //! Create an automatically generated DescriptorScriptPubKeyMan (i.e. during wallet or descriptor creation)

achow101 commented at 9:45 pm on July 24, 2025:

Done

in src/wallet/scriptpubkeyman.h:332 in 93c3729ad5 outdated

331@@ -324,10 +332,8 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan
332     DescriptorScriptPubKeyMan(WalletBatch& batch, WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const FlatSigningProvider& provider);

rkrux commented at 11:56 am on July 15, 2025:

In https://github.com/bitcoin/bitcoin/commit/93c3729ad5adc02f6d87ec8daa1a1952d7c41009 “wallet: Setup new autogenerated descriptors on construction”

This will make storage the first argument in all the 4 constructors and this consistency would be nice for readability imo.

0- DescriptorScriptPubKeyMan(WalletBatch& batch, WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const FlatSigningProvider& provider);
1+ DescriptorScriptPubKeyMan(WalletStorage& storage, WalletBatch& batch, WalletDescriptor& descriptor, int64_t keypool_size, const FlatSigningProvider& provider);

achow101 commented at 9:45 pm on July 24, 2025:

Done

rkrux commented at 12:19 pm on July 15, 2025: contributor

Concept ACK 93c3729ad5adc02f6d87ec8daa1a1952d7c41009

This PR makes DescriptorScriptPubKeyMan class RAII compliant by adding 4 constructors that are responsible for allocating all the resources (eg: memory) required by this class during initialisation itself. I feel that it improves the readability quite a lot as the reader can easily gauge what all is needed by this class at one place and there is no need for multiple scattered class-write function calls like the one below that used to appear before.

0- WITH_LOCK(desc_spk_man->cs_desc_man, desc_spk_man->AddDescriptorKeyWithDB(batch, key, key.GetPubKey()));
1- desc_spk_man->TopUpWithDB(batch);

I noticed that this style now allows us to add more error checking that makes the code more robust imo.

0Error: Wallet contains both unencrypted and encrypted keys

Perhaps a Refactoring label is required in the PR?

wallet: Consolidate generation setup callers into one function 23d60002a8

wallet: Load everything into DescSPKM on construction

Instead of creating a DescSPKM that is then progressively loaded, we
should instead create it all at once in a constructor when loading.

c94423fb71

fuzz: Skip adding descriptor to wallet if it cannot be expanded 2c0c41f8c6

wallet: include keys when constructing DescriptorSPKM during import

When importing a descriptor, all of the descriptor data should be
provided at the same time in the constructor.

1e2113ca1a

wallet: Construct ExternalSignerSPKM with the new descriptor

Instead of constructing then setting the descriptor with
SetupDescriptor, just pass in that descriptor to the constructor.

a1a15bb4ac

wallet: Setup new autogenerated descriptors on construction

Instead of having a caller use SetupDescriptorGeneration, just have a
constructor that takes those arguments and sets up the descriptor with
the autogenerated key.

8e9de762f6

achow101 force-pushed on Jul 24, 2025

rkrux approved

rkrux commented at 7:48 am on July 25, 2025: contributor

ACK 8e9de762f66f3e99e02944006999fe5afb1896e4

Thanks for addressing comments.

0git range-diff 93c3729...8e9de76

wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively #28333

Code Coverage & Benchmarks

Reviews

Conflicts