Split CWallet::Create() into CreateNew and LoadExisting
#32636
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32636. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
It'd be nice if the load or create intention could be propagated down to the DB level as well. In |
|
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
PR breakdown suggestion
The first 3 can be done in parallel & the last one might be dependent on the previous ones:
- The
PopulateWalletFromDBrelated changes can be 1 PR - the first 2 commits. - The
fBroadcastTransactionschange 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&CreateNewfunctions.
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.
|
Shouldn't |
0f82224 to
349c9bc
Compare
349c9bc to
353cfdd
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
353cfdd to
05cbadf
Compare
Previously creating an encrypted wallet would result in the keypool size incorrectly being reported as 0. See: bitcoin#32636 (comment)
Previously creating an encrypted wallet would result in the keypool size incorrectly being reported as 0. See: bitcoin#32636 (comment)
b29d22a to
a3e24b0
Compare
|
Pushed an update to this branch to address @rkrux's feedback, and feedback from an offline review with @achow101 and @w0xlt.
|
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
w0xlt
left a comment
There was a problem hiding this comment.
ACK a3e24b0
The PR executes a good refactoring and changes the function names to declare their intent (create vs load) .
The identified issues (WriteVersion parameter, public PopulateWalletFromDB) are minor and appropriate for follow-up PRs.
The CI error is unrelated.
1bcff32 to
cd47b5d
Compare
Previously creating an encrypted wallet would result in the keypool size incorrectly being reported as 0. See: bitcoin#32636 (comment)
Co-authored-by: @w0xlt <w0xlt@users.noreply.github.com>
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.
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. Review with `-color-moved=dimmed-zebra`
`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.
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()`.
This will avoid repetition when wallet creation and loading are separated.
Splits out logic relevant only to existing wallets in `CWallet::Create()` into `CWallet::LoadExisting()`
…mDB()` Writing the wallet's `CLIENT_VERSION` (which indicates the last version to have touched a wallet) needs to be done on both wallet creation and wallet loading. The next commit removes the `PopulateWalletFromDatabase()` call from wallet creation, but this behavior needs to be preserved, so this commit factors setting `CLIENT_VERSION` out of `PopulateWalletFromDatabase()` so that wallet creation can use it in the next commit.
Previously creating an encrypted wallet would result in the keypool size incorrectly being reported as 0. See: bitcoin#32636 (comment)
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-
cd47b5d to
f9de11f
Compare
|
Please excuse the mess I've made... I've pushed (a3e24b0..f9de11f) to change one line which is deleting the build-config.h include in $ git diff a3e24b0 f9de11fdiff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp
index b59308bae9..e0de4fc958 100644
--- a/src/wallet/wallettool.cpp
+++ b/src/wallet/wallettool.cpp
@@ -2,8 +2,6 @@
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
-#include <bitcoin-build-config.h> // IWYU pragma: keep
-
#include <wallet/wallettool.h>
#include <common/args.h> |
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()intoCWallet::CreateNew()andCWallet::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:bitcoin/src/wallet/wallet.cpp
Lines 2882 to 2885 in 370c592
This heuristic assumes that wallets with no
ScriptPubKeyMansare 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.