Get rid of file path handling in wallet application code and move it down to database layer.
There is no change in behavior except for some changed error messages.
Motivation for this change is to make code more understandable, but also to prepare for adding SQLite support in #19077 so SQLite implementation can be contained at the database layer and wallet loading code does not need to become more complicated.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, 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.
Could the second commit be split up a bit? It seems to be doing a lot and several functions are being renamed and removed.
From what I understand, the second commit adds DatabaseOptions and DatabaseStatus. Then CreateWalletDatabase is renamed to MakeWalletDatabase and all of the various checks (file existence checks and database verify) are pushed into that function with DatabaseOptions informing the function what to actually do. And lastly MakeWalletDatabase becomes a separate distinct step that callers must do instead of letting CWallet::Create (renamed from CreateWalletFromFile) handle internally.
From this, I think you could add a few more commits:
DatabaseOptions and DatabaseStatus to consolidate the current options and statusMakeWalletDatabaseMakeWalletDatabase from CWallet::Create
Could the second commit be split up a bit? It seems to be doing a lot and several functions are being renamed and removed.
Sure, I think I can split up like you’re suggesting. The main change is introducing the MakeBerkeleyDatabase function. Then that function is called various places to get rid of path operations. So I think introducing MakeBerkeleyDatabase could be one commit, and then this can be followed by more commits calling it from RPC code, and then gui code, and then wallet tool code
Oh, also I didn’t want this PR to depend on #19099, but this would be a little smaller with #19099 merged frist, because then there would be no need to change interfaces/node and walletinitinterface code here
Rebased 8994145cb336862a51151cbaa2e222e3e2a5be96 -> 0e4cb50e3c86c3093b18fe2dfb47f563db32afef (pr/path.2 -> pr/path.3, compare) due to conflict with #19102
re: #19619 (comment)
Could the second commit be split up a bit? It seems to be doing a lot and several functions are being renamed and removed.
Took some work, but I split this up into smaller commits that should make it more obvious nothing is changing.
re: #19619 (comment)
I can see why we’d want to move anything about “wallet.dat” or the “wallet is a directory” stuff into the database layer, but IMO it doesn’t make sense to hide the path the wallet is located within…
I don’t think anything has been hidden here. Database object now has a filename method and error messages are more specific than before including full paths rather than path fragments.
Updated 0e4cb50e3c86c3093b18fe2dfb47f563db32afef -> 4c2638281446efc7b23559925866238808a87c71 (pr/path.3 -> pr/path.4, compare) splitting up later commits and making a few minor cleanups
823@@ -824,3 +824,50 @@ std::unique_ptr<DatabaseBatch> BerkeleyDatabase::MakeBatch(const char* mode, boo
824 {
825 return MakeUnique<BerkeleyBatch>(*this, mode, flush_on_close);
826 }
827+
828+std::unique_ptr<BerkeleyDatabase> MakeBerkeleyDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error)
829+{
830+ status = DatabaseStatus::SUCCESS;
re: #19619 (review)
I think the non-BDB specific parts of this should be part of
MakeDatabase. It would make this easier to handle in #19077
You’re right, moved this code since it didn’t really support detecting the database format.
139@@ -142,6 +140,9 @@ class WalletDatabase
140
141 virtual void ReloadDbEnv() = 0;
142
143+ /** Return path to main database file for logs and error messages. */
144+ virtual std::string Filename() = 0;
m_file_path variable in WalletDatabase that does this. If we add Filename, we should remove that.
re: #19619 (review)
There is a
m_file_pathvariable inWalletDatabasethat does this. If we addFilename, we should remove that.
Thanks, cleaned this up.
146@@ -147,9 +147,6 @@ class WalletDatabase
147 unsigned int nLastFlushed;
148 int64_t nLastWalletUpdate;
149
150- /** Verifies the environment and database file */
151- virtual bool Verify(bilingual_str& error) = 0;
re: #19619 (review)
It seems like #19077 still needs
Verify. Even if we don’t necessarily use this outside of database specific code paths, I think it’s still useful to keep it part ofWalletDatabase.
Will take a look. The thought was that the API should either support offline verification at load time or online verification after the database is loaded, or and that load-time verification was probably preferable because:
MakeDatabase rather than MakeBerkeleyDatabase.
re: #19619 (review)
I think we can require that verify is an online verification function and call it from
MakeDatabaserather thanMakeBerkeleyDatabase.
Agree we can add an extra method to the interface, but would want to know advantages to doing this. Sticking with a verify option instead of a verify method seems more flexible for implementations, and simpler and less error prone for callers (https://github.com/bitcoin/bitcoin/pull/19619#discussion_r466593575).
WalletDatabase implementations and always done within MakeDatabase so that there isn’t an implementation that doesn’t do this verification. I think we should be requiring that all WalletDatabase implementations have some kind of online verification, and if a database doesn’t provide this, perhaps we shouldn’t be using it.
re: #19619 (review)
The main idea is that the verify is an option for all
WalletDatabaseimplementations and always done withinMakeDatabaseso that there isn’t an implementation that doesn’t do this verification. I think we should be requiring that allWalletDatabaseimplementations have some kind of online verification, and if a database doesn’t provide this, perhaps we shouldn’t be using it.
This can change in the future, but I don’t think we should require something we don’t need to require, or complicate an API with a method that doesn’t need to be exist, is likely to be forgotten or called inconsistently, and that we even saw being used inconsistently in a recent PR #19324 (review)
111+ std::shared_ptr<CWallet> wallet_instance = MakeWallet(name, path, /* create= */ true);
112 if (wallet_instance) {
113 WalletShowInfo(wallet_instance.get());
114 wallet_instance->Close();
115 }
116 } else if (command == "info" || command == "salvage") {
else if as it’s pointless now.
re: #19619 (review)
Might as well remove this
else ifas it’s pointless now.
Don’t want to increase scope of this PR, but agree wallet tool command dispatching can be simplified later.
17@@ -18,6 +18,13 @@ typedef std::pair<std::vector<unsigned char>, std::vector<unsigned char> > KeyVa
18
19 bool RecoverDatabaseFile(const fs::path& file_path, bilingual_str& error, std::vector<bilingual_str>& warnings)
20 {
21+ DatabaseOptions options;
22+ DatabaseStatus status;
23+ options.require_existing = true;
24+ options.verify = false;
25+ std::unique_ptr<WalletDatabase> database = MakeDatabase(file_path, options, status, error);
26+ if (!database) return false;
database. Having it created and potentially open now could interfere with the salvage below. We only want to do this for the path existence check.
re: #19619 (review)
I think we need to close and destroy the
database. Having it created and potentially open now could interfere with the salvage below.
I’m not sure exactly what change is being asked for here. Closing an unopened database won’t do anything. Goal of this commit is to remove path manipulation from wallet tool while changing it minimally (and nudging code in good future directions), so happy to make any change consistent with that.
We only want to do this for the path existence check.
I don’t think this is right. I think other checks done before opening a database should be done before salvaging a database. Not just checking path existence, but also checking load status and lock status.
I expect having the database object here to be more useful in the future. It’s beyond the scope of this PR, but recovery code below should be able to go through the database object instead of bypassing it.
What I was asking for was to have a database = nullptr here so that database isn’t valid further down.
If we do want to do the salvage through WalletDatabase (I’m not sure that we should though), then I would expect that all of this recovery code ends up back in bdb.cpp/h.
re: #19619 (review)
What I was asking for was to have a
database = nullptrhere so thatdatabaseisn’t valid further down.If we do want to do the salvage through
WalletDatabase(I’m not sure that we should though), then I would expect that all of this recovery code ends up back inbdb.cpp/h.
No harm other than verbosity, but it seems nonsensical to reset the database pointer while the database is being recovered. Also, unless this function is supposed to be berkeley-specific the dbenv->dbrename and db.verify berkely code below will need to move to bdb.cpp. The ReadKeyValue code below shouldn’t move to bdb.cpp because that is higher level wallet code.
143@@ -144,6 +144,9 @@ class BerkeleyDatabase : public WalletDatabase
144 /** Verifies the environment and database file */
145 bool Verify(bilingual_str& error);
146
147+ /** Return path to main database filename */
148+ std::string Filename() override { return (env->Directory() / strFile).string(); }
ee5bb1bb09d9834edb6cd15d233be504f00615db
Could be const?
re: #19619 (review)
Could be const?
I don’t like virtual methods to be const. I don’t think a superclass declaration should prevent subclasses from being able to modify their own state, expose this implementation detail to callers, and allow callers to pass around viral const references which constrain present and future subclass implementations. Adding const in cases like this doesn’t have benefits that I’m aware of and seems likely to lead to code churn (removing viral consts) or confusing workarounds (adding mutable/const_casts) going forward.
3728@@ -3727,23 +3729,23 @@ std::unique_ptr<WalletDatabase> MakeWalletDatabase(const std::string& name, cons
3729 return MakeDatabase(wallet_path, options, status, error_string);
3730 }
3731
3732-std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain, const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings, uint64_t wallet_creation_flags)
3733+std::shared_ptr<CWallet> CWallet::Create(interfaces::Chain& chain, const std::string& name, std::unique_ptr<WalletDatabase> database, uint64_t wallet_creation_flags, bilingual_str& error, std::vector<bilingual_str>& warnings)
ee5bb1bb09d9834edb6cd15d233be504f00615db
Why not take name from database? Is it intentional to allow independent names and filenames?
re: #19619 (review)
Why not take
namefromdatabase? Is it intentional to allow independent names and filenames?
Yes it’s intentional. Database objects don’t have names. They aren’t passed names, can’t construct names, and don’t need names because they don’t need to route GUI or RPC requests, unlike wallet objects.
Concept ACK.
Motivation for this change is to make code more understandable, but also to prepare for adding SQLite support in #19077 so SQLite implementation can be contained at the database layer and wallet loading code does not need to become more complicated.
So now we have MakeWalletDatabase and then CWallet::Create(database) which makes perfect sense to me.
41@@ -42,16 +42,16 @@ bool VerifyWallets(interfaces::Chain& chain, const std::vector<std::string>& wal
42 std::set<fs::path> wallet_paths;
43
44 for (const auto& wallet_file : wallet_files) {
45- WalletLocation location(wallet_file);
46+ fs::path path = fs::absolute(wallet_file, GetWalletDir());
aac063f28e4ed48180c2ba7a51706af969ca1e55, nit
0 const fs::path path = fs::absolute(wallet_file, GetWalletDir());
#include <fs.h> ?
2498@@ -2499,21 +2499,22 @@ static UniValue loadwallet(const JSONRPCRequest& request)
2499 }.Check(request);
2500
2501 WalletContext& context = EnsureWalletContext(request.context);
2502- WalletLocation location(request.params[0].get_str());
2503+ std::string name(request.params[0].get_str());
aac063f28e4ed48180c2ba7a51706af969ca1e55, nit
0 const std::string name(request.params[0].get_str());
Commit 134f1807e51a8b64c813bf994c8752d55ff3469f “wallet: Add MakeDatabase function” confuses me:
bool IsBerkeleyBtree() declaration is added to wallet/bdb.h but its definition remains in wallet/walletutil.cpp
std::unique_ptr<WalletDatabase> MakeDatabase() declaration is added to wallet/db.h but its definition is added to wallet/walletdb.cpp
If this made on purpose, mind explaining such code organization?
835+
836+std::unique_ptr<BerkeleyDatabase> MakeBerkeleyDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error)
837+{
838+ std::unique_ptr<BerkeleyDatabase> db;
839+ {
840+ LOCK(cs_db);
GetWalletEnv() do this job?
re: #19619 (review)
134f1807e51a8b64c813bf994c8752d55ff3469f Why locking is required here? Doesn’t
GetWalletEnv()do this job?
Added comment. The lock needs to cover both GetWalletEnv and the Database constructor to prevent a race condition. In the future I think all these little functions should be inlined so code is clearer, but that’s not really possible without more cleanup and anyway would not be directly related to path code consolidation which is the goal of this PR
Thanks for reviews!
Rebased f190f67ca7b3315c9dc46bf882dbc96af778b607 -> 1233419c88c31f21e1b59b4e3f0cdad34d92413b (pr/path.6 -> pr/path.7, compare) due to silent conflict with #15937 and updated with a few code review suggestions.
re: #19619 (comment)
Commit 134f180 “wallet: Add MakeDatabase function” confuses me:
[…]
If this made on purpose, mind explaining such code organization?
This PR is not trying to change code organization, just consolidate path manipulation code in the wallet. But here is the desired organization:
db.h, db.cpp: should contain Database and Batch classes, DatabaseStatus and DatabaseOptions type definitions, and MakeDatabase() function referencing the class and typesbdb.h, bdb.cpp: should contain BerkeleyDatabase and BerkeleyBatch classes and MakeBerkeleyDatabase() and ExistsBerkeleyDatabase() functionssqlite.h, sqlite.cpp: should contain SqliteDatabase and SqliteBatch classes and MakeSqliteDatabase() and ExistsSqliteDatabase() functionswalletutil.h, walletutil.cpp: should be removed. BerkeleyBTree() should be dropped and inlined or moved to bdb.h / bdb.cpp, other functions similarly should be dropped or moved.I’m ok with changing code organization in this PR if it doesn’t increase size of this PR, but otherwise think it should be left to a separate moveonly PR. Otherwise this PR is just trying to change things as little as possible and not make the circular include linter angry, so it is using forward declarations in the interim some places to avoid the need to move code right now.
2518+ fs::path path(fs::absolute(name, GetWalletDir()));
2519
2520- if (!location.Exists()) {
2521- throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Wallet " + location.GetName() + " not found.");
2522- } else if (fs::is_directory(location.GetPath())) {
2523+ if (fs::symlink_status(path).type() == fs::file_not_found) {
in commit 83e4ead9917735b2ba92c8a35fe37504206ffe32
please don’t change behavior in refactoring commits.
Previously:
0test_framework.authproxy.JSONRPCException: Wallet not found. (-18)
Now:
0test_framework.authproxy.JSONRPCException: Directory does not contain a wallet.dat file. (-18)
2532 bilingual_str error;
2533 std::vector<bilingual_str> warnings;
2534 std::shared_ptr<CWallet> const wallet = LoadWallet(*context.chain, name, options, status, error, warnings);
2535- if (!wallet) throw JSONRPCError(RPC_WALLET_ERROR, error.original);
2536+ if (!wallet) {
2537+ RPCErrorCode code = status == DatabaseStatus::FAILED_NOT_FOUND || status == DatabaseStatus::FAILED_BAD_FORMAT ? RPC_WALLET_NOT_FOUND : RPC_WALLET_ERROR;
FAILED_BAD_FORMAT as a not-found error?
re: #19619 (review)
Why treat
FAILED_BAD_FORMATas a not-found error?
I added a code comment. I can change to different error code, but FAILED_BAD_FORMAT is returned when the wallet directory exists but doesn’t have a wallet.dat file. RPC_WALLET_NOT_FOUND was what the previous code returned in this case so this is not a change in behavior.
Initial read through looks good to me, modulo MarcoFalke’s comment on the first commit. Once rebased I will re-review.
Concept ACK. On macOS wallet_multiwallet.py fails for me with:
02020-08-31T12:20:37.628000Z TestFramework (INFO): Concurrent wallet loading
12020-08-31T12:20:38.768000Z TestFramework (ERROR): Assertion failed
2Traceback (most recent call last):
3 File "/Users/sjors/dev/bitcoin/test/functional/test_framework/test_framework.py", line 118, in main
4 self.run_test()
5 File "test/functional/wallet_multiwallet.py", line 242, in run_test
6 assert_equal(got_loading_error, True)
Rebased 1233419c88c31f21e1b59b4e3f0cdad34d92413b -> 83a8f51da5bb86c39f030e027100f0eb5f1f52ad (pr/path.7 -> pr/path.8, compare) due to conflicts with #19099 and #19671
For some reason github won’t let me reply directly to these comments above:
re: #19619 (review)
please don’t change behavior in refactoring commits.
Good catch. It wasn’t my intention to change behavior in this commit. I think I was paying more attention to the error code than error text. But the new text actually makes a little more sense than the old text, so I just noted the change in the commit message. Both these error messages are replaced anyway in later commit “Remove path checking code from loadwallet RPC”.
re: #19619 (review)
I sort of liked that the functionality was wrapped in one function. This avoids issues exactly like this
Context for this comment may have been lost so I’m not sure what it is referring to. If it is referring to the removed WalletLocation code in loadwallet, the PR is inlining operations that were happening across three functions (loadwallet, WalletLocation::WalletLocation, WalletLocation::Exists) into a straightforward path check done one place..
re: #19619 (comment)
Concept ACK. On macOS
wallet_multiwallet.pyfails for me with:
Does the failure happen reliably? This is known to be a flaky check: #19300 (review)
This removes a source of complexity and indirection that makes it harder to
understand path checking code. Path checks will be simplified in upcoming
commits.
There is no change in behavior in this commit other than a slightly more
descriptive error message in `loadwallet` if the default "" wallet can't be
found. (The error message is improved more in upcoming commit "wallet: Remove
path checking code from loadwallet RPC".)
New function is not currently called but will be called in upcoming commits. It
moves database path checking, and existence checking, and already-loaded
checking, and verification into a single function so this logic does not need
to be repeated all over higher level wallet code, and so higher level code does
not need to change when SQLite support is added in
https://github.com/bitcoin/bitcoin/pull/19077. This also lets higher level
wallet code make fewer assumptions about the contents of wallet directories.
This commit just adds the new function and does not change behavior in any way.
No changes in behavior. Just replaces arguments and return types
Checks are now consolidated in MakeBerkeleyDatabase function instead of
happening in higher level code.
This commit does not change behavior except for error messages which now
include more complete information.
No changes in behavior
This commit does not change behavior except for error messages which now
include more complete information.
This commit does not change behavior except for error messages which now
include more complete information.
This commit does not change behavior except for error messages which now
include more complete information.
2676- case WalletCreationStatus::ENCRYPTION_FAILED:
2677- throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, error.original);
2678- case WalletCreationStatus::SUCCESS:
2679- break;
2680- // no default case, so the compiler can warn about missing cases
2681+ std::shared_ptr<CWallet> wallet = CreateWallet(*context.chain, request.params[0].get_str(), options, status, error, warnings);
4dba0c5ac68223b218b33d49e3621ea863042a07, nit:
0 const auto wallet = CreateWallet(*context.chain, request.params[0].get_str(), options, status, error, warnings);
re: #19619 (review)
4dba0c5, nit:
Disagree with the suggestion. I do like auto when writing the type would be very verbose (iterators) or when type would be just be repeated (new, make_shared, MakeUnique expressions). But this isn’t one of those cases, and using auto here would hide information that helps clarify the code
Ok. Using auto is a matter of taste or philosophy.
But what is wrong with const?
Ok. Using
autois a matter of taste or philosophy.But what is wrong with
const?
I don’t have a problem with it, but it isn’t my preference to tag every variable that could possibly be tagged const with const. It’s clarifying and helpful and some cases, and noise in other cases. In these specific cases, I think shared_ptr<CWallet> helps readability, auto does not help readability, const does not help readability. If there needs to be more consts, probably better to write a developer guideline and linter than discuss in review comments.
150@@ -151,12 +151,13 @@ namespace {
151 std::shared_ptr<CWallet> LoadWalletInternal(interfaces::Chain& chain, const std::string& name, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings)
152 {
153 try {
154- if (!MakeWalletDatabase(name, options, status, error)) {
155+ std::unique_ptr<WalletDatabase> database = MakeWalletDatabase(name, options, status, error);
8c14ac68863bb4727f774315d9b0e3820543252a, nit:
0 auto database = MakeWalletDatabase(name, options, status, error);
157 error = Untranslated("Wallet file verification failed.") + Untranslated(" ") + error;
158 return nullptr;
159 }
160
161- std::shared_ptr<CWallet> wallet = CWallet::CreateWalletFromFile(chain, name, error, warnings);
162+ std::shared_ptr<CWallet> wallet = CWallet::Create(chain, name, std::move(database), options.create_flags, error, warnings);
8c14ac68863bb4727f774315d9b0e3820543252a, nit:
0 const auto wallet = CWallet::Create(chain, name, std::move(database), options.create_flags, error, warnings);
204@@ -204,7 +205,8 @@ std::shared_ptr<CWallet> CreateWallet(interfaces::Chain& chain, const std::strin
205 }
206
207 // Wallet::Verify will check if we're trying to create a wallet with a duplicate name.
208- if (!MakeWalletDatabase(name, options, status, error)) {
209+ std::unique_ptr<WalletDatabase> database = MakeWalletDatabase(name, options, status, error);
8c14ac68863bb4727f774315d9b0e3820543252a, nit:
0 auto database = MakeWalletDatabase(name, options, status, error);
219@@ -218,7 +220,7 @@ std::shared_ptr<CWallet> CreateWallet(interfaces::Chain& chain, const std::strin
220 }
221
222 // Make the wallet
223- std::shared_ptr<CWallet> wallet = CWallet::CreateWalletFromFile(chain, name, error, warnings, wallet_creation_flags);
224+ std::shared_ptr<CWallet> wallet = CWallet::Create(chain, name, std::move(database), wallet_creation_flags, error, warnings);
8c14ac68863bb4727f774315d9b0e3820543252a, nit:
0 const auto wallet = CWallet::Create(chain, name, std::move(database), wallet_creation_flags, error, warnings);
74+ DatabaseStatus status;
75+ options.verify = false; // No need to verify, assuming verified earlier in VerifyWallets()
76 bilingual_str error;
77 std::vector<bilingual_str> warnings;
78- std::shared_ptr<CWallet> pwallet = CWallet::CreateWalletFromFile(chain, walletFile, error, warnings);
79+ std::unique_ptr<WalletDatabase> database = MakeWalletDatabase(name, options, status, error);
8c14ac68863bb4727f774315d9b0e3820543252a, nit:
0 auto database = MakeWalletDatabase(name, options, status, error);
17@@ -18,6 +18,13 @@ typedef std::pair<std::vector<unsigned char>, std::vector<unsigned char> > KeyVa
18
19 bool RecoverDatabaseFile(const fs::path& file_path, bilingual_str& error, std::vector<bilingual_str>& warnings)
20 {
21+ DatabaseOptions options;
22+ DatabaseStatus status;
23+ options.require_existing = true;
24+ options.verify = false;
25+ std::unique_ptr<WalletDatabase> database = MakeDatabase(file_path, options, status, error);
83a8f51da5bb86c39f030e027100f0eb5f1f52ad, nit:
0 auto database = MakeDatabase(file_path, options, status, error);
Rebased 83a8f51da5bb86c39f030e027100f0eb5f1f52ad -> 7bf6dfbb484adfda3b8df26ee3e2ebda239dd263 (pr/path.8 -> pr/path.9, compare) due to conflict with #19754
Thanks for reviews! Reminder to anyone interested that sqlite PR #19077 currently depends on this. And so far achow and hebasto have acked, and meshcollider, promag, and sjors have concept acked this.