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

pull achow101 wants to merge 6 commits into bitcoin:master from achow101:raii-spkm changing 8 files +156 −129
  1. 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.

  2. 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
    Concept ACK murchandamus
    Approach ACK w0xlt, polespinasa
    Stale ACK rkrux, adyshimony

    If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #33112 (wallet: relax external_signer flag constraints by Sjors)
    • #32861 (Have createwalletdescriptor auto-detect an unused(KEY) by Sjors)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

    LLM Linter (✨ experimental)

    Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

    • WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0) in src/wallet/scriptpubkeyman.cpp

    2026-03-23 22:31:16

  3. DrahtBot added the label Wallet on Aug 24, 2023
  4. in src/wallet/wallet.cpp: 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.

  5. unknown approved
  6. DrahtBot added the label CI failed on Sep 3, 2023
  7. DrahtBot removed the label CI failed on Sep 5, 2023
  8. DrahtBot added the label CI failed on Nov 25, 2023
  9. 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      |
  10. achow101 force-pushed on Nov 27, 2023
  11. DrahtBot removed the label CI failed on Nov 27, 2023
  12. DrahtBot added the label Needs rebase on Nov 28, 2023
  13. achow101 force-pushed on Nov 28, 2023
  14. DrahtBot removed the label Needs rebase on Nov 28, 2023
  15. achow101 force-pushed on Dec 7, 2023
  16. DrahtBot added the label Needs rebase on Dec 8, 2023
  17. achow101 force-pushed on Dec 8, 2023
  18. DrahtBot removed the label Needs rebase on Dec 8, 2023
  19. DrahtBot added the label CI failed on Jan 12, 2024
  20. DrahtBot added the label Needs rebase on Feb 20, 2024
  21. achow101 force-pushed on Feb 20, 2024
  22. DrahtBot removed the label Needs rebase on Feb 20, 2024
  23. DrahtBot added the label Needs rebase on Mar 29, 2024
  24. achow101 force-pushed on Mar 29, 2024
  25. DrahtBot removed the label Needs rebase on Mar 29, 2024
  26. achow101 force-pushed on Apr 3, 2024
  27. 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.
  28. DrahtBot removed the label CI failed on Apr 4, 2024
  29. DrahtBot added the label Needs rebase on Jun 3, 2024
  30. achow101 force-pushed on Jun 4, 2024
  31. DrahtBot removed the label Needs rebase on Jun 5, 2024
  32. DrahtBot added the label Needs rebase on Jul 11, 2024
  33. achow101 force-pushed on Jul 22, 2024
  34. DrahtBot removed the label Needs rebase on Jul 22, 2024
  35. DrahtBot added the label Needs rebase on Aug 12, 2024
  36. achow101 force-pushed on Aug 29, 2024
  37. DrahtBot removed the label Needs rebase on Aug 29, 2024
  38. achow101 requested review from murchandamus on Oct 15, 2024
  39. DrahtBot added the label Needs rebase on Oct 24, 2024
  40. achow101 force-pushed on Oct 28, 2024
  41. DrahtBot added the label CI failed on Oct 28, 2024
  42. 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.

  43. achow101 force-pushed on Oct 28, 2024
  44. DrahtBot removed the label Needs rebase on Oct 28, 2024
  45. DrahtBot removed the label CI failed on Oct 28, 2024
  46. DrahtBot added the label Needs rebase on Apr 23, 2025
  47. achow101 force-pushed on Apr 24, 2025
  48. DrahtBot removed the label Needs rebase on Apr 24, 2025
  49. DrahtBot added the label Needs rebase on Apr 25, 2025
  50. achow101 force-pushed on Apr 25, 2025
  51. DrahtBot removed the label Needs rebase on Apr 25, 2025
  52. DrahtBot added the label Needs rebase on May 7, 2025
  53. achow101 force-pushed on May 7, 2025
  54. DrahtBot removed the label Needs rebase on May 7, 2025
  55. DrahtBot added the label Needs rebase on May 19, 2025
  56. achow101 force-pushed on May 20, 2025
  57. DrahtBot removed the label Needs rebase on May 20, 2025
  58. in src/wallet/scriptpubkeyman.h: 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]
  59. DrahtBot added the label Needs rebase on May 21, 2025
  60. achow101 force-pushed on May 21, 2025
  61. DrahtBot removed the label Needs rebase on May 21, 2025
  62. DrahtBot added the label Needs rebase on Jun 14, 2025
  63. achow101 force-pushed on Jun 14, 2025
  64. DrahtBot removed the label Needs rebase on Jun 14, 2025
  65. 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.

  66. in src/wallet/wallet.cpp: 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
  67. in src/wallet/wallet.cpp: 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")
  68. in src/wallet/wallet.cpp: 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
  69. in src/wallet/walletdb.cpp: 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
  70. in src/wallet/walletdb.cpp: 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
  71. in src/wallet/scriptpubkeyman.h: 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.
  72. in src/wallet/scriptpubkeyman.h: 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
  73. in src/wallet/scriptpubkeyman.h: 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
  74. in src/wallet/scriptpubkeyman.h: 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
  75. 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?

  76. achow101 force-pushed on Jul 24, 2025
  77. rkrux approved
  78. rkrux commented at 7:48 am on July 25, 2025: contributor

    ACK 8e9de762f66f3e99e02944006999fe5afb1896e4

    Thanks for addressing comments.

    0git range-diff 93c3729...8e9de76
  79. achow101 requested review from davidgumberg on Oct 22, 2025
  80. DrahtBot added the label Needs rebase on Dec 27, 2025
  81. achow101 force-pushed on Jan 2, 2026
  82. DrahtBot removed the label Needs rebase on Jan 2, 2026
  83. in src/wallet/scriptpubkeyman.cpp:861 in 3c4f53d5e1 outdated 
    856+
857+DescriptorScriptPubKeyMan::DescriptorScriptPubKeyMan(WalletStorage& storage, WalletBatch& batch, int64_t keypool_size, const CExtKey& master_key, OutputType addr_type, bool internal)
858+    :   ScriptPubKeyMan(storage),
859+        m_keypool_size(keypool_size)
860+{
861+    SetupDescriptorGeneration(batch, master_key, addr_type, internal);
    w0xlt commented at 2:54 am on February 12, 2026:

    nit: SetupDescriptorGeneration return value is being silently ignored.

    0    Assume(SetupDescriptorGeneration(batch, master_key, addr_type, internal));
    polespinasa commented at 10:25 am on February 18, 2026:

    Wouldn’t Assume here abort the program if this is hit:

    0
1    // Ignore when there is already a descriptor
2    if (m_wallet_descriptor.descriptor) {
3        return false;
4    }

    It doesn’t look like a reason to abort the program. It was already ignored in the past before this changes https://github.com/bitcoin/bitcoin/blob/59e10a5463dc940ecd88e6961737f634416de6d5/src/wallet/wallet.cpp#L3607

    Maybe could just be a void. I tested it and doesn’t seem to break anything.

     0$ git diff
 1diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
 2index 8a1c0a45a5..60302110ad 100644
 3--- a/src/wallet/scriptpubkeyman.cpp
 4+++ b/src/wallet/scriptpubkeyman.cpp
 5@@ -1132,14 +1132,14 @@ bool DescriptorScriptPubKeyMan::AddDescriptorKeyWithDB(WalletBatch& batch, const
 6     }
 7 }
 8 
 9-bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(WalletBatch& batch, const CExtKey& master_key, OutputType addr_type, bool internal)
10+void DescriptorScriptPubKeyMan::SetupDescriptorGeneration(WalletBatch& batch, const CExtKey& master_key, OutputType addr_type, bool internal)
11 {
12     LOCK(cs_desc_man);
13     assert(m_storage.IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS));
14 
15     // Ignore when there is already a descriptor
16     if (m_wallet_descriptor.descriptor) {
17-        return false;
18+        return;
19     }
20 
21     m_wallet_descriptor = GenerateWalletDescriptor(master_key.Neuter(), addr_type, internal);
22@@ -1156,7 +1156,6 @@ bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(WalletBatch& batch, co
23     TopUpWithDB(batch);
24 
25     m_storage.UnsetBlankWalletFlag(batch);
26-    return true;
27 }
28 
29 bool DescriptorScriptPubKeyMan::IsHDEnabled() const
30diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h
31index 00dd6eed4d..f4e1c8b053 100644
32--- a/src/wallet/scriptpubkeyman.h
33+++ b/src/wallet/scriptpubkeyman.h
34@@ -357,7 +357,7 @@ public:
35     bool IsHDEnabled() const override;
36 
37     //! Setup descriptors based on the given CExtkey
38-    bool SetupDescriptorGeneration(WalletBatch& batch, const CExtKey& master_key, OutputType addr_type, bool internal);
39+    void SetupDescriptorGeneration(WalletBatch& batch, const CExtKey& master_key, OutputType addr_type, bool internal);
40 
41     bool HavePrivateKeys() const override;
42     bool HasPrivKey(const CKeyID& keyid) const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man);

    <\details>

    achow101 commented at 10:32 pm on March 23, 2026:
    Changed to void
  84. in src/wallet/scriptpubkeyman.cpp:848 in 3c4f53d5e1 outdated 
    843+    m_map_keys(keys),
844+    m_map_crypted_keys(ckeys),
845+    m_keypool_size(keypool_size),
846+    m_wallet_descriptor(descriptor)
847+{
848+    if (!m_map_keys.empty() && !m_map_crypted_keys.empty()) {
    w0xlt commented at 3:02 am on February 12, 2026:

    nit: Consistency with the other constructor bodies.

    0    LOCK(cs_desc_man);
1    if (!m_map_keys.empty() && !m_map_crypted_keys.empty()) {
    achow101 commented at 10:09 pm on March 23, 2026:
    I don’t think that “consistency” is a good reason to be acquiring the lock when it is entirely unnecessary. The other constructors need to acquire the lock because they call functions that do AssertLockHeld.
  85. w0xlt commented at 3:03 am on February 12, 2026: contributor
    Approach ACK
  86. adyshimony commented at 0:32 am on February 14, 2026: none

    ACK 3c4f53d5e1ea30d9a2a4bd1d34501ce419d2aebf

    I agree this is a clearer and safer approach. Reviewed and tested.

    Tests: /build/bin/test_bitcoin /build/test/functional/test_runner.py wallet_createwallet.py wallet_encryption.py wallet_descriptor.py wallet_importdescriptors.py wallet_signer.py wallet_blank.py wallet_keypool_topup.py

  87. DrahtBot requested review from w0xlt on Feb 14, 2026
  88. DrahtBot requested review from rkrux on Feb 14, 2026
  89. rkrux commented at 2:33 pm on February 16, 2026: contributor
    Reviewing again.
  90. in src/wallet/wallet.cpp:428 in 6d299761a2 outdated 
    421@@ -422,30 +422,18 @@ std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string&
422         return nullptr;
423     }
424 
425+    // Unset the blank flag if not specified by the user
426+    if (!create_blank) {
427+        wallet->UnsetWalletFlag(WALLET_FLAG_BLANK_WALLET);
428+    }
    polespinasa commented at 9:18 am on February 18, 2026:

    Just a question, in 6d299761a28ec299f21897c4fac57ba3bfc0762f

    Why could the wallet have the flag set and the user not specify it?

    murchandamus commented at 4:51 pm on March 13, 2026:

    This is probably out-of-scope for this PR, but I find the existence of create_blank confusing in general. I’m wondering whether we could avoid surfacing create_blank to the user in general. From what I understand, every wallet starts blank and every user flow needs to add some descriptor to the wallet before it becomes usable for wallet purposes. Wouldn’t it make more sense to optionally allow the creation of a new wallet when you import a descriptor instead of putting the onus on the user to know that they need to create a blank wallet first?

    Then users would either:

    • Create a new wallet (with SPKMs)
    • Import a descriptor to an existing wallet
    • Import a descriptor to create a new wallet that only contains the descriptor

    This way it would always be obvious whether we use custom SPKMs or need to generate fresh ones and blank wallets would be an implementation detail rather than a user interface option. Would there be any other reason why users would want to create blank wallets? (Ignoring for a moment that it would change the user flow existing users have come to expect.)

    achow101 commented at 10:15 pm on March 23, 2026:

    Why could the wallet have the flag set and the user not specify it?

    The flag can be set without the user having done so because that’s how born-encrypted wallets are implemented. We need to tell wallet creation to not generate any descriptors (or keys, back when this was originally implemented) so that the wallet can then be encrypted, and the descriptors generated afterwards. Since creating a wallet with no descriptors in it is also useful to users, this was made into a wallet flag so that users could also explicitly create such a wallet.

    Wouldn’t it make more sense to optionally allow the creation of a new wallet when you import a descriptor instead of putting the onus on the user to know that they need to create a blank wallet first?

    Sure, but that’s way out of scope.

  91. in src/wallet/walletdb.cpp:795 in 82fbca46e7 outdated 
    790-
791-        // Prior to doing anything with this spkm, verify ID compatibility
792-        if (id != spkm.GetID()) {
793-            strErr = "The descriptor ID calculated by the wallet differs from the one in DB";
794-            return DBErrors::CORRUPT;
795-        }
    polespinasa commented at 9:50 am on February 18, 2026:

    In 82fbca46e7bfcc5cec52b8b5902adb55ea538d46

    Why move this check at the end inside pwallet->LoadDescriptorScriptPubKeyMan(id, desc, keys, ckeys); ? Wouldn’t we avoid doing extra useless job if keeping this here?

    achow101 commented at 10:32 pm on March 23, 2026:
    Good point, moved the ID check to after deserializing the WalletDescriptor
  92. in src/wallet/scriptpubkeyman.cpp:606 in 32fb9ffdd1 outdated 
    602@@ -603,9 +603,8 @@ std::optional<MigrationData> LegacyDataSPKM::MigrateToDescriptor()
603         WalletDescriptor w_desc(std::move(descs.at(0)), creation_time, 0, 0, 0);
604 
605         // Make the DescriptorScriptPubKeyMan and get the scriptPubKeys
606-        auto desc_spk_man = std::make_unique<DescriptorScriptPubKeyMan>(m_storage, w_desc, /*keypool_size=*/0);
607-        WITH_LOCK(desc_spk_man->cs_desc_man, desc_spk_man->AddDescriptorKeyWithDB(batch, key, key.GetPubKey()));
608-        desc_spk_man->TopUpWithDB(batch);
609+        keys.keys.emplace(key.GetPubKey().GetID(), key);
    polespinasa commented at 9:57 am on February 18, 2026:

    in 32fb9ffdd1ee0ffbad6a65640ebef2e90b09991e

    nit: I know this is not this PR fault but keys.keys is not clear and makes this difficult to understand. Probably FlatSigningProvider variable should have another name?

    achow101 commented at 10:22 pm on March 23, 2026:
    Will leave for a different PR
  93. polespinasa commented at 10:30 am on February 18, 2026: member

    Approach ACK This is much more clean and clear than before.

    code reviewed lgtm 3c4f53d5e1ea30d9a2a4bd1d34501ce419d2aebf

    I just have some questions and left some non-blocking nits before ACKing it.

  94. in src/wallet/wallet.cpp:3601 in 6d299761a2 outdated 
    3593@@ -3609,6 +3594,17 @@ void CWallet::SetupDescriptorScriptPubKeyMans()
3594     }
3595 }
3596 
3597+void CWallet::SetupWalletGeneration()
3598+{
3599+    LOCK(cs_wallet);
3600+    // Skip if blank or no privkeys
3601+    if (!IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER) &&
    murchandamus commented at 5:26 pm on March 13, 2026:

    Wouldn’t WALLET_FLAG_EXTERNAL_SIGNER indicate that there are no privkeys? So, why don’t we skip on WALLET_FLAG_EXTERNAL_SIGNER true?

    Is the comment here wrong?

    achow101 commented at 10:30 pm on March 23, 2026:

    We don’t skip because Setup for external signers invokes the external signer to get descriptors.

    Updated the comment to be more explicit

  95. murchandamus commented at 6:22 pm on March 13, 2026: member
    Concept ACK
  96. wallet: Consolidate generation setup callers into one function 5767a58f5e
  97. 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.
    0f294cbb6a
  98. fuzz: Skip adding descriptor to wallet if it cannot be expanded f398d80938
  99. 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.
    94e0aa68d0
  100. wallet: Construct ExternalSignerSPKM with the new descriptor 
    Instead of constructing then setting the descriptor with
SetupDescriptor, just pass in that descriptor to the constructor.
    f3f3cc9d38
  101. 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.
    b4dcb9b97e
  102. achow101 force-pushed on Mar 23, 2026


achow101 DrahtBot S3RK maflcko rkrux w0xlt polespinasa adyshimony murchandamus


davidgumberg rkrux w0xlt

Labels
Wallet

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-03-24 03:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me