Split `CWallet::Create()` into `CreateNew` and `LoadExisting`

davidgumberg commented at 0:37 am on May 29, 2025: contributor

This PR is mostly a refactor which splits out logic used for creating wallets and for loading wallets, both of which are presently contained in CWallet::Create() into CWallet::CreateNew() and CWallet::LoadExisting()

The real win of this PR is that CWallet::Create() uses a very bad heuristic for trying to guess whether or not it is supposed to be creating a new wallet or loading an existing wallet:

https://github.com/bitcoin/bitcoin/blob/370c59261269fd9043674e0f4fd782a89e724473/src/wallet/wallet.cpp#L2882-L2885

This heuristic assumes that wallets with no ScriptPubKeyMans are being created, which sounds reasonable, but as demonstrated in #32112 and #32111, this can happen when the user tries to load a wallet file that is corrupted, both issues are fixed by this PR and any other misbehavior for wallet files which succeeded the broken heuristic’s sniff test for new wallets.

It was already the case that every caller of CWallet::Create() knows whether it is creating a wallet or loading one, so we can avoid replacing this bad heuristic with another one, and just shift the burden to the caller.

DrahtBot commented at 0:37 am on May 29, 2025: 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/32636.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	pablomartin4btc, rkrux
Approach ACK	w0xlt

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:

#33082 (wallet, refactor: Remove Legacy check and error by pablomartin4btc)
#33034 (wallet: Store transactions in a separate sqlite table by achow101)
#32895 (wallet: Prepare for future upgrades by recording versions of last client to open and decrypt by achow101)
#32685 (wallet: Allow read-only database access for info and dump commands by PeterWrighten)
#32138 (wallet, rpc: remove settxfee and paytxfee by polespinasa)
#29278 (Wallet: Add maxfeerate wallet startup option by ismaelsadeeq)

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.

w0xlt commented at 8:14 pm on May 29, 2025: contributor

Approach ACK

achow101 commented at 9:56 pm on May 29, 2025: member

PopulateWalletFromDB inside of CreateNew should no longer be necessary.

It’d be nice if the load or create intention could be propagated down to the DB level as well. In SQLiteDatabase::Open, we pass the flags SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE which does both load and create. I think ideally it would be just SQLITE_OPEN_READWRITE for loading, and SQLITE_OPEN_CREATE for creating to further enforce that we expect a file to already exist for loading, and for no files to exist when creating. Similarly, we could avoid calling TryCreateDirectories when loading.

achow101 added the label Wallet on Jun 4, 2025

luke-jr commented at 9:05 pm on June 6, 2025: member

Any reason not to just pass a parameter to CWallet::Create, just to fix the bug in the simplest manner? Refactoring should ideally be separate from fixing.

achow101 commented at 9:11 pm on June 6, 2025: member

Any reason not to just pass a parameter to CWallet::Create, just to fix the bug in the simplest manner? Refactoring should ideally be separate from fixing.

Because doing the bare minimum to fix a bug that is unreachable in production is how we end up with technical debt.

IMO, this PR is primarily a refactor to split these 2 actions that should not be combined. The fact that it fixes those bugs, which are a result of the combining, is a bonus.

pablomartin4btc commented at 0:47 am on June 15, 2025: member

Concept ACK

The current state of this code in master adds confusion to flows like the legacy to descriptor wallet migration that’s why I think it’s an important work to be done.

in src/wallet/wallet.cpp:3123 in 744ebdc185 outdated

3085-        LOCK(walletInstance->cs_wallet);
3086-        walletInstance->WalletLogPrintf("setKeyPool.size() = %u\n",      walletInstance->GetKeyPoolSize());
3087-        walletInstance->WalletLogPrintf("mapWallet.size() = %u\n",       walletInstance->mapWallet.size());
3088-        walletInstance->WalletLogPrintf("m_address_book.size() = %u\n",  walletInstance->m_address_book.size());
3089-    }
3090+    WITH_LOCK(walletInstance->cs_wallet, walletInstance->LogStats());

rkrux commented at 1:31 pm on June 16, 2025:

In 744ebdc18556223b8749a7fed420c9ae95be418e:

Sometime back I noted that during the creation of encrypted wallets, the wallet metrics are not correctly logged after the SPKMs have been created. This is unlike the creation of non-encrypted wallets where the correct metrics are logged because they are done so after the setup of SPKMS. PFB a sample from my node.

 0^[[C=2025-05-27T15:19:21Z Using SQLite Version 3.43.2
 12025-05-27T15:19:21Z Using wallet /Users/rkrux/Library/ApplicationSupport/Bitcoin/regtest/wallets/encryptedtest
 22025-05-27T15:19:21Z init message: Loading wallet…
 32025-05-27T15:19:21Z [encryptedtest] Legacy Wallet Keys: 0 plaintext, 0 encrypted, 0 w/ metadata, 0 total.
 42025-05-27T15:19:21Z [encryptedtest] Descriptors: 0, Descriptor Keys: 0 plaintext, 0 encrypted, 0 total.
 52025-05-27T15:19:21Z [encryptedtest] Setting minversion to 169900
 62025-05-27T15:19:21Z [encryptedtest] Wallet completed loading in              12ms
 72025-05-27T15:19:21Z [encryptedtest] setKeyPool.size() = 0
 82025-05-27T15:19:21Z [encryptedtest] mapWallet.size() = 0
 92025-05-27T15:19:21Z [encryptedtest] m_address_book.size() = 0
102025-05-27T15:19:22Z [encryptedtest] Encrypting Wallet with an nDeriveIterations of 251048
112025-05-27T15:19:22Z [encryptedtest] Setting spkMan to active: id = a8af620b202a2542a6b4575578133499397089fa67110b27dffced31b3e6ccbd, type = legacy, internal = false
122025-05-27T15:19:22Z [encryptedtest] Setting spkMan to active: id = b389c6313cd85b907990fd2c57a87db230fa0dad1c94c929dd0e010e036d802e, type = p2sh-segwit, internal = false
132025-05-27T15:19:22Z [encryptedtest] Setting spkMan to active: id = 727d3e57942f107c713932ccf8aec8f6968ee76b98f8787802afc62cbaa13d38, type = bech32, internal = false

I was not sure if a PR just for this would garner review but now that the code touching these metrics is being updated anyway, might as well fix this issue. I have not checked if this PR already fixes it, great if it does!

davidgumberg commented at 6:05 am on August 23, 2025:

Thanks for pointing this out, I’ve fixed this issue in https://github.com/bitcoin/bitcoin/pull/32636/commits/9de86da65e3dffb8e9c3dbd9771258f0924db798.

in src/wallet/wallet.cpp:3027 in d5d55b3554 outdated

3020@@ -3021,11 +3021,6 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
3021         return nullptr;
3022     }
3023 
3024-    // This wallet is in its first run if there are no ScriptPubKeyMans and it isn't blank or no privkeys
3025-    const bool fFirstRun = walletInstance->m_spk_managers.empty() &&
3026-                     !walletInstance->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) &&
3027-                     !walletInstance->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET);

rkrux commented at 1:54 pm on June 16, 2025:

The real win of this PR is that CWallet::Create() uses a very bad heuristic for trying to guess whether or not it is supposed to be creating a new wallet or loading an existing wallet

Agree that this is not a good heuristic but the PR description doesn’t seem to explain or summarise why it’s a bad heuristic. Maybe add few points from the findings of the linked issues?

davidgumberg commented at 5:12 am on August 23, 2025:

Thanks for pointing this out, I’ve updated the PR description to address this.

rkrux commented at 2:38 pm on June 16, 2025: contributor

Concept ACK 0f82224d3937db3f60cfaec249ac5fe3264dc3d5 I want to share my initial thoughts before I invest more time reviewing this.

I am in favour of such a refactor because from a reviewing POV I lose mental cycles every time I end up going through CWallet:Create function that requires me to keep a mental context of whether this function was called from the creation flow or the load flow. Also, I feel this is a good time to get this done now that legacy wallets are no longer there.

However, I am slightly less inclined to review this PR in its current format because there are a few moving pieces here, and I’m afraid reviewing it thoroughly might be more time consuming and would induce less confidence in me finally a-c-king it.

Moreover, I feel the benefits of such a refactor would be more highlighted and appreciated if this can be broken down into smaller PRs in addition to ensuring data-correctness. Couple reasons I’m suggesting a breakdown are:

I would like to give more attention to each of the commits by trying to understand their consequences on the wallet as a whole.
I don’t suppose all the commits are required to be present sequentially in a single PR based on my initial look, please feel free to correct me if I’m mistaken about their ability to be present in parallel.

The first 3 can be done in parallel & the last one might be dependent on the previous ones:

The PopulateWalletFromDB related changes can be 1 PR - the first 2 commits.
The fBroadcastTransactions change is independent, can be clubbed with the birth-time-update-removal optimisation - 3rd, 6th commits.
The argument parsing changes also seem independent to me and can be another PR - 4th, 5th commits.
No strong opinion on the wallet stats logging commit, can go anywhere.
The last 5 commits can be 1 PR that introduces and uses the LoadExisting & CreateNew functions.

Also, I believe this is a good suggestion that might give dividends in the future and breaking down into smaller PRs might make it easy to incorporate it.

Having suggested all this, if you think the reward for breaking down the PR is not enough because the smaller refactoring PRs might not attract reviewers or might add rebase churn, I would still review this PR later but some kind of breakdown would aid review if not the one that I suggested.

Sorry if this got too long.

in src/qt/test/addressbooktests.cpp:80 in f26bf5fd24 outdated

76@@ -77,7 +77,7 @@ void TestAddAddressesToSendBook(interfaces::Node& node)
77     test.m_node.wallet_loader = wallet_loader.get();
78     node.setContext(&test.m_node);
79     const std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>(node.context()->chain.get(), "", CreateMockableWalletDatabase());
80-    wallet->LoadWallet();
81+    wallet->PopulateWalletFromDB();

achow101 commented at 11:05 pm on July 23, 2025:

In f26bf5fd24ffed501dfdb3528c56cfaee0203a72 “scripted-diff: refactor: rename CWallet::LoadWallet”

I think we could actually drop the PopulateWalletFromDB here and in other test setups that call it immediately after some kind of Create*WalletDatabase() since there’s nothing in the databases anyways to load. These calls should be no-ops.

davidgumberg commented at 1:08 am on August 23, 2025:

This is done in #33082: https://github.com/bitcoin/bitcoin/pull/33082/commits/30c6f64eed304560464f9601b80c811c186db20a

in src/wallet/wallet.cpp:2360 in 0f82224d39 outdated

2339+        } else if (nLoadWalletRet == DBErrors::LEGACY_WALLET) {
2340+            error = strprintf(_("Error loading %s: Wallet is a legacy wallet. Please migrate to a descriptor wallet using the migration tool (migratewallet RPC)."), wallet_file);
2341+        } else {
2342+            error = strprintf(_("Error loading %s"), wallet_file);
2343+        }
2344+    }

w0xlt commented at 9:25 pm on July 24, 2025:

 0    if (nLoadWalletRet != DBErrors::LOAD_OK) {
 1        const auto wallet_file = m_database->Filename();
 2        
 3        switch (nLoadWalletRet) {
 4            case DBErrors::CORRUPT:
 5                error = strprintf(_("Error loading %s: Wallet corrupted"), wallet_file);
 6                break;
 7            case DBErrors::NONCRITICAL_ERROR:
 8                warnings.push_back(strprintf(_("Error reading %s! All keys read correctly, but transaction data"
 9                                           " or address metadata may be missing or incorrect."), wallet_file));
10                break;
11            case DBErrors::TOO_NEW:
12                error = strprintf(_("Error loading %s: Wallet requires newer version of %s"), wallet_file, CLIENT_NAME);
13                break;
14            case DBErrors::EXTERNAL_SIGNER_SUPPORT_REQUIRED:
15                error = strprintf(_("Error loading %s: External signer wallet being loaded without external signer support compiled"), wallet_file);
16                break;
17            case DBErrors::NEED_REWRITE:
18                error = strprintf(_("Wallet needed to be rewritten: restart %s to complete"), CLIENT_NAME);
19                break;
20            case DBErrors::NEED_RESCAN:
21                warnings.push_back(strprintf(_("Error reading %s! Transaction data may be missing or incorrect."
22                                           " Rescanning wallet."), wallet_file));
23                break;
24            case DBErrors::UNKNOWN_DESCRIPTOR:
25                error = strprintf(_("Unrecognized descriptor found. Loading wallet %s\n\n"
26                                "The wallet might had been created on a newer version.\n"
27                                "Please try running the latest software version.\n"), wallet_file);
28                break;
29            case DBErrors::UNEXPECTED_LEGACY_ENTRY:
30                error = strprintf(_("Unexpected legacy entry in descriptor wallet found. Loading wallet %s\n\n"
31                                "The wallet might have been tampered with or created with malicious intent.\n"), wallet_file);
32                break;
33            case DBErrors::LEGACY_WALLET:
34                error = strprintf(_("Error loading %s: Wallet is a legacy wallet. Please migrate to a descriptor wallet using the migration tool (migratewallet RPC)."), wallet_file);
35                break;
36            default:
37                error = strprintf(_("Error loading %s"), wallet_file);
38                break;
39        }
40    }

davidgumberg commented at 6:06 am on August 23, 2025:

Thanks for the suggestion, fixed in https://github.com/bitcoin/bitcoin/pull/32636/commits/da4579d97863924272eab2246c45a0802b226cf8.

DrahtBot added the label Needs rebase on Jul 25, 2025

in src/wallet/wallet.cpp:3047 in 0f82224d39 outdated

3045+                    if (!spk_man->SetupGeneration()) {
3046+                        error = _("Unable to generate initial keys");
3047+                        return nullptr;
3048+                    }
3049+                }
3050+            }

pablomartin4btc commented at 9:06 pm on August 1, 2025:

We have the assert above so we don’t need to check if it’s a descriptor and legacy can’t be created anymore.

0            walletInstance->SetupDescriptorScriptPubKeyMans();

davidgumberg commented at 2:11 am on August 23, 2025:

This was fixed in https://github.com/bitcoin/bitcoin/pull/32481/commits/5431f2dc2159f55e0fbe89d07deb97fe2a73fb43 (nice!), fixed on this branch by rebasing.

in src/wallet/wallet.cpp:3028 in 0f82224d39 outdated

3026+
3027+    {
3028+        LOCK(walletInstance->cs_wallet);
3029+
3030+        // ensure this wallet.dat can only be opened by clients supporting HD with chain split and expects no default key
3031+        walletInstance->SetMinVersion(FEATURE_LATEST);

pablomartin4btc commented at 9:07 pm on August 1, 2025:

Please consider #32977

davidgumberg commented at 1:09 am on August 23, 2025:

Nice find, sorry I missed this.

pablomartin4btc commented at 9:54 pm on August 1, 2025: member

Shouldn’t src/wallet/walletdb.cpp be part of this PR? DBErrors WalletBatch::LoadWallet(CWallet* pwallet) is being called during wallet creation I think. If so, please also consider the note at #33082’s description.

davidgumberg force-pushed on Aug 23, 2025

DrahtBot removed the label Needs rebase on Aug 23, 2025

davidgumberg force-pushed on Aug 23, 2025

DrahtBot added the label CI failed on Aug 23, 2025

DrahtBot commented at 5:46 am on August 23, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/48721895167 LLM reason (✨ experimental): The CI failure is caused by a linting error detected by ruff due to an unused import in the test file, preventing successful completion of the lint check.

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.

davidgumberg referenced this in commit c3e500127c on Aug 23, 2025

davidgumberg force-pushed on Aug 23, 2025

davidgumberg referenced this in commit 2826ccd38a on Aug 23, 2025

davidgumberg force-pushed on Aug 23, 2025

davidgumberg referenced this in commit 84541793da on Aug 23, 2025

davidgumberg force-pushed on Aug 23, 2025

davidgumberg referenced this in commit 9de86da65e on Aug 23, 2025

davidgumberg force-pushed on Aug 23, 2025

davidgumberg commented at 6:42 am on August 23, 2025: contributor

@achow101

PopulateWalletFromDB inside of CreateNew should no longer be necessary. @pablomartin4btc DBErrors WalletBatch::LoadWallet(CWallet* pwallet) is being called during wallet creation I think.

Thanks for catching this, fixed in https://github.com/bitcoin/bitcoin/pull/32636/commits/acc0d3b7d4e8b26e1dc5832497b2b47047b6a453

It’d be nice if the load or create intention could be propagated down to the DB level as well. In SQLiteDatabase::Open, we pass the flags SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE which does both load and create. I think ideally it would be just SQLITE_OPEN_READWRITE for loading, and SQLITE_OPEN_CREATE for creating to further enforce that we expect a file to already exist for loading, and for no files to exist when creating. Similarly, we could avoid calling TryCreateDirectories when loading.

Agreed, I think it might be better to fix this in a follow-up as this PR is already quite large.

@rkrux

I feel the benefits of such a refactor would be more highlighted and appreciated if this can be broken down into smaller PRs

Thanks for the suggestion, despite the large number of commits in this PR, I’ve tried my best to make each one atomic and readable. I would prefer to not split into multiple PR’s since most of the commits here are refactors that are setting-up for splitting wallet creation and loading, the first PR(s) in the series would have very little motivation, but if you insist I am open to splitting it up. Is there a particular commit or group of commits that are difficult to review?

DrahtBot removed the label CI failed on Aug 23, 2025

rkrux commented at 8:19 am on August 25, 2025: contributor

I would prefer to not split into multiple PR’s since most of the commits here are refactors that are setting-up for splitting wallet creation and loading, the first PR(s) in the series would have very little motivation, but if you insist I am open to splitting it up.

No insistence at the moment, I will do a full review of the PR soon.

in src/wallet/wallet.cpp:3102 in 4c6890cdae outdated

3121+    auto nLoadWalletRet = walletInstance->PopulateWalletFromDB(error, warnings);
3122+    if (nLoadWalletRet == DBErrors::NEED_RESCAN) {
3123+        rescan_required = true;
3124+    } else if (nLoadWalletRet != DBErrors::LOAD_OK && nLoadWalletRet != DBErrors::NONCRITICAL_ERROR) {
3125+        return nullptr;
3126+    }

achow101 commented at 11:07 pm on August 27, 2025:

In 4c6890cdae77ca0d230227530b000f4b3c6fd77a “wallet: Create separate function for wallet load”

I think this can be simplified:

0    auto nLoadWalletRet = walletInstance->PopulateWalletFromDB(error, warnings);
1    bool rescan_required = nLoadWalletRet == DBErrors::NEED_RESCAN;
2    if (nLoadWalletRet != DBErrors::LOAD_OK && nLoadWalletRet != DBErrors::NONCRITICAL_ERROR && !rescan_required) {
3        return nullptr;
4    }

davidgumberg commented at 0:18 am on August 28, 2025:

Thanks for catching this, fixed.

in src/wallet/wallet.cpp:3107 in 4c6890cdae outdated

3103@@ -3104,6 +3104,56 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
3104     return walletInstance;
3105 }
3106 
3107+std::shared_ptr<CWallet> CWallet::LoadExisting(WalletContext& context, const std::string& name, std::unique_ptr<WalletDatabase> database, uint64_t wallet_creation_flags, bilingual_str& error, std::vector<bilingual_str>& warnings)

achow101 commented at 11:09 pm on August 27, 2025:

In 4c6890cdae77ca0d230227530b000f4b3c6fd77a “wallet: Create separate function for wallet load”

When loading, wallet_creation_flags is unnecessary. Flags cannot be updated when loading a wallet, and having a specific warning for WALLET_FLAG_DISABLE_PRIVATE_KEYS seems entirely unnecessary.

davidgumberg commented at 0:11 am on August 28, 2025:

Is the the check for the presence of private keys in a wallet where WALLET_FLAG_DISABLE_PRIVATE_KEYS is set also unnecessary?

davidgumberg commented at 0:18 am on August 28, 2025:

Fixed the suggestion as I understood it, thanks for catching this.

davidgumberg referenced this in commit 1eb926d35a on Aug 28, 2025

davidgumberg force-pushed on Aug 28, 2025

DrahtBot added the label CI failed on Aug 28, 2025

DrahtBot commented at 0:41 am on August 28, 2025: contributor

🚧 At least one of the CI tasks failed. Task TSan, depends, no gui: https://github.com/bitcoin/bitcoin/runs/49052123167 LLM reason (✨ experimental): Compilation failed in wallet tests: LoadExisting is called with six arguments, but its declaration only takes five.

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.

scripted-diff: refactor: rename CWallet::LoadWallet -> PopulateWalletFromDB

There are too many functions in CWallet with names like "Load" and
"Create", disambiguate what CWallet::LoadWallet does by renaming it to
PopulateWalletFromDB.

-BEGIN VERIFY SCRIPT-
sed -i 's|\bLoadWallet()|PopulateWalletFromDB()|g' $(git grep -l 'LoadWallet()' -- ':(exclude)src/wallet/walletdb.cpp')
-END VERIFY SCRIPT-

f92031e3b0

refactor: wallet: move error handling to PopulateWalletFromDB() 8b6a8a04f9

wallet: refactor: PopulateWalletFromDB use switch statement.

Co-authored-by: @w0xlt <w0xlt@users.noreply.github.com>

f6c4454b87

refactor: Move -walletbroadcast setting init

Modifying `fBroadcastTransactions` does not require any locks,
initialization of this wallet parameter can be relocated with all of the
other argument parsing in this function.

3a29186688

refactor: Split out wallet argument loading

This section is necessarily repetitive, makes CWallet::Create() easier
to read, and splits out functionality that will be useful when wallet
creation and loading are separated.

d9a3af962a

wallet: Move argument parsing to before DB load

`m_keypool_size` must be set before `CWallet::PopulateWalletFromDB()`,
in order to move parsing of `-keypool` into `CWallet::LoadWalletArgs`,
`LoadWalletArgs()` invocation in `CWallet::Create()` must be moved
before `PopulateWalletFromDB()` is called.

347834e41b

wallet: Remove redundant birth time update

Checking every SPKM in `CWallet::Create()` is not necessary, since the
only way presently for an SPKM to get added to `m_spk_managers` (the
return value of `GetAllScriptPubKeyMans()`) is through
`AddScriptPubKeyMan()`, which already invokes `MaybeUpdateBirthTime()`.

39503c4667

refactor: Wallet stats logging in its own function

This will avoid repetition when wallet creation and loading are
separated.

cf59f05485

wallet: Create separate function for wallet load

Splits out logic relevant only to existing wallets in
`CWallet::Create()` into `CWallet::LoadExisting()`

29c624ad9e

wallet: Use CWallet::LoadExisting() for loading existing wallets. 7f7dc32765

test: wallet: Split create and load dd720fe5bd

refactor: wallet: Add `WriteVersion()`

Writing the wallet version needs to be done on wallet creation, but
WalletBatch::LoadWallet() does not, so factor this functionality out.

62e0721cae

wallet: remove loading logic from CWallet::Create 8ef5c41445

wallet: Correctly log stats for encrypted messages.

Previously creating an encrypted wallet would result in the keypool size
incorrectly being reported as 0.

See: https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2150021064

8022e04ccf

scripted-diff: refactor: CWallet::Create() -> CreateNew()

Aside from being more legible, changing the name of `CWallet::Create()`
also validates that every instance where a new wallet is `Create()`'ed
is handled in this branch.

-BEGIN VERIFY SCRIPT-
sed -i 's|\bCreate(|CreateNew(|g' src/wallet/wallet.cpp  src/wallet/wallet.h  src/wallet/test/util.cpp src/wallet/test/wallet_tests.cpp
-END VERIFY SCRIPT-

a6b43fa173

davidgumberg force-pushed on Aug 28, 2025

DrahtBot removed the label CI failed on Aug 28, 2025

Split CWallet::Create() into CreateNew and LoadExisting #32636

Code Coverage & Benchmarks

Reviews

Conflicts

Split `CWallet::Create()` into `CreateNew` and `LoadExisting` #32636