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

achow101 · 2023-08-24T02:55:39Z

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 · 2023-08-24T02:55:41Z

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:

#33112 (wallet: relax external_signer flag constraints by Sjors)
#32861 (Have createwalletdescriptor auto-detect an unused(KEY) by Sjors)
#32652 (wallet: add codex32 argument to addhdkey by roconnor-blockstream)

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.

S3RK · 2023-08-28T07:00:04Z

src/wallet/wallet.cpp

@@ -391,40 +391,18 @@ std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string&
        return nullptr;
    }

+    // Unset the blank flag if not specified by the user
+    if (!create_blank) {


Is this change of behaviour or not?

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.

maflcko · 2023-11-27T13:35:43Z

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

src/wallet/scriptpubkeyman.h

rkrux

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.

rkrux

Concept ACK 93c3729

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.

- WITH_LOCK(desc_spk_man->cs_desc_man, desc_spk_man->AddDescriptorKeyWithDB(batch, key, key.GetPubKey()));
- 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.

Error: Wallet contains both unencrypted and encrypted keys

Perhaps a Refactoring label is required in the PR?

src/wallet/wallet.cpp

rkrux · 2025-07-15T10:54:01Z

src/wallet/wallet.cpp

@@ -420,30 +420,18 @@ std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string&
        return nullptr;
    }

+    // Unset the blank flag if not specified by the user
+    if (!create_blank) {


In 32dd0e3 "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.

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.

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.

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index bd1b36b4a9..5a3ecc1d19 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -381,9 +381,6 @@ std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string& Assert(wallet_creation_flags & WALLET_FLAG_DESCRIPTORS); options.require_format = DatabaseFormat::SQLITE; - // Indicate that the wallet is actually supposed to be blank and not just blank to make it encrypted - bool create_blank = (wallet_creation_flags & WALLET_FLAG_BLANK_WALLET); - // Born encrypted wallets need to be created blank first. if (!passphrase.empty()) { wallet_creation_flags |= WALLET_FLAG_BLANK_WALLET; @@ -420,13 +417,10 @@ std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string& return nullptr; } - // Unset the blank flag if not specified by the user - if (!create_blank) { - wallet->UnsetWalletFlag(WALLET_FLAG_BLANK_WALLET); - } - // Encrypt the wallet if (!passphrase.empty()) { + // Unset the blank flag that was purposefully set above for encrypted wallets + wallet->UnsetWalletFlag(WALLET_FLAG_BLANK_WALLET); if (!wallet->EncryptWallet(passphrase)) { error = Untranslated("Error: Wallet created but failed to encrypt."); 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.

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

src/wallet/wallet.cpp

src/wallet/walletdb.cpp

rkrux · 2025-07-15T11:26:20Z

src/wallet/scriptpubkeyman.h

+using WatchOnlySet = std::set<CScript>;
+using WatchKeyMap = std::map<CKeyID, CPubKey>;
+using KeyMap = std::map<CKeyID, CKey>;
+using CryptedKeyMap = std::map<CKeyID, std::pair<CPubKey, std::vector<unsigned char>>>;
+using ScriptPubKeyMap = std::map<CScript, int32_t>; // Map of scripts to descriptor range index
+using PubKeyMap = std::map<CPubKey, int32_t>; // Map of pubkeys involved in scripts to descriptor range index


In fdba094 "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?

I think leaving them here is fine.

src/wallet/scriptpubkeyman.h

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

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

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

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

rkrux

ACK 8e9de76

Thanks for addressing comments.

git range-diff 93c3729...8e9de76

rkrux · 2025-07-25T07:28:07Z

src/wallet/wallet.cpp

@@ -420,30 +420,18 @@ std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string&
        return nullptr;
    }

+    // Unset the blank flag if not specified by the user
+    if (!create_blank) {


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.

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index bd1b36b4a9..5a3ecc1d19 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -381,9 +381,6 @@ std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string& Assert(wallet_creation_flags & WALLET_FLAG_DESCRIPTORS); options.require_format = DatabaseFormat::SQLITE; - // Indicate that the wallet is actually supposed to be blank and not just blank to make it encrypted - bool create_blank = (wallet_creation_flags & WALLET_FLAG_BLANK_WALLET); - // Born encrypted wallets need to be created blank first. if (!passphrase.empty()) { wallet_creation_flags |= WALLET_FLAG_BLANK_WALLET; @@ -420,13 +417,10 @@ std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string& return nullptr; } - // Unset the blank flag if not specified by the user - if (!create_blank) { - wallet->UnsetWalletFlag(WALLET_FLAG_BLANK_WALLET); - } - // Encrypt the wallet if (!passphrase.empty()) { + // Unset the blank flag that was purposefully set above for encrypted wallets + wallet->UnsetWalletFlag(WALLET_FLAG_BLANK_WALLET); if (!wallet->EncryptWallet(passphrase)) { error = Untranslated("Error: Wallet created but failed to encrypt."); 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.

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

DrahtBot added the Wallet label Aug 24, 2023

S3RK reviewed Aug 28, 2023

View reviewed changes

ghost approved these changes Aug 30, 2023

View reviewed changes

DrahtBot added CI failed and removed CI failed labels Sep 3, 2023

DrahtBot mentioned this pull request Sep 11, 2023

wallet: Receive silent payment transactions #28453

Closed

DrahtBot mentioned this pull request Oct 3, 2023

wallet: optimize migration process, batch db transactions #28574

Merged

DrahtBot mentioned this pull request Oct 24, 2023

Remove the legacy wallet and BDB dependency #28710

Merged

This was referenced Nov 16, 2023

wallet: batch all individual spkms setup db writes in a single db txn #28894

Merged

refactor: VectorWriter and SpanReader without nVersion #28912

Merged

DrahtBot added the CI failed label Nov 25, 2023

achow101 force-pushed the raii-spkm branch from d91026a to 12d12fa Compare November 27, 2023 22:15

DrahtBot removed the CI failed label Nov 27, 2023

DrahtBot mentioned this pull request Nov 28, 2023

refactor: Remove unused SER_DISK, SER_NETWORK, CDataStream #28451

Merged

DrahtBot added the Needs rebase label Nov 28, 2023

achow101 force-pushed the raii-spkm branch from 12d12fa to 632961c Compare November 28, 2023 19:31

DrahtBot removed the Needs rebase label Nov 28, 2023

This was referenced May 13, 2025

wallet, refactor: Remove Legacy wallet unused warnings and errors #32481

Merged

Menu action to export a watchonly wallet bitcoin-core/gui#872

Draft

wallet: Add exportwatchonlywallet RPC to export a watchonly version of a wallet #32489

Open

DrahtBot added the Needs rebase label May 19, 2025

achow101 force-pushed the raii-spkm branch from 8bc7447 to 2cee4de Compare May 20, 2025 00:54

DrahtBot removed the Needs rebase label May 20, 2025

DrahtBot reviewed May 21, 2025

View reviewed changes

src/wallet/scriptpubkeyman.h Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label May 21, 2025

achow101 force-pushed the raii-spkm branch from 2cee4de to 630c7a8 Compare May 21, 2025 18:18

DrahtBot removed the Needs rebase label May 21, 2025

DrahtBot mentioned this pull request May 31, 2025

wallet: add codex32 argument to addhdkey #32652

Draft

DrahtBot added the Needs rebase label Jun 14, 2025

achow101 force-pushed the raii-spkm branch from 630c7a8 to 93c3729 Compare June 14, 2025 02:31

DrahtBot removed the Needs rebase label Jun 14, 2025

DrahtBot mentioned this pull request Jun 20, 2025

wallet: derivehdkey RPC to get xpub at arbitrary path #32784

Draft

DrahtBot mentioned this pull request Jul 3, 2025

Have createwalletdescriptor auto-detect an unused(KEY) #32861

Draft

rkrux reviewed Jul 11, 2025

View reviewed changes

DrahtBot mentioned this pull request Jul 14, 2025

Silent Payments: Receiving #32966

Draft

rkrux reviewed Jul 15, 2025

View reviewed changes

rkrux mentioned this pull request Jul 16, 2025

wallet: remove outdated pszSkip arg of database Rewrite func #32990

Merged

DrahtBot mentioned this pull request Jul 23, 2025

[POC] wallet: Enable non-electronic (paper-based) wallet backup with codex32 #33043

Draft

achow101 added 6 commits July 24, 2025 14:43

wallet: Consolidate generation setup callers into one function

23d6000

wallet: Load everything into DescSPKM on construction

c94423f

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

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

2c0c41f

wallet: include keys when constructing DescriptorSPKM during import

1e2113c

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

wallet: Construct ExternalSignerSPKM with the new descriptor

a1a15bb

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

wallet: Setup new autogenerated descriptors on construction

8e9de76

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

achow101 force-pushed the raii-spkm branch from 93c3729 to 8e9de76 Compare July 24, 2025 21:44

rkrux approved these changes Jul 25, 2025

View reviewed changes

DrahtBot mentioned this pull request Aug 1, 2025

wallet: relax external_signer flag constraints #33112

Open

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

achow101 commented Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

S3RK Aug 28, 2023

Choose a reason for hiding this comment

Uh oh!

achow101 Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Nov 27, 2023

Uh oh!

Uh oh!

rkrux left a comment

Choose a reason for hiding this comment

Uh oh!

rkrux left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rkrux Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

achow101 Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

rkrux Jul 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rkrux Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

achow101 Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rkrux left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkrux Jul 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 commented Aug 24, 2023 •

edited

Loading

DrahtBot commented Aug 24, 2023 •

edited

Loading

achow101 Aug 29, 2023 •

edited

Loading

rkrux left a comment •

edited

Loading