Currently restorewallet() logic is written in the RPC layer and it can´t be reused by GUI. So it moves this to the wallet section and then, GUI can access it.
This is necessary to implement the “Restore Wallet” menu item in the GUI (which is already implemented in https://github.com/bitcoin-core/gui/pull/471 ).
This commit also simplifies error handling and adds a new behavior: if the restore fails, the invalid wallet folder is removed.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
src/node/ and src/wallet/ code to node:: and wallet:: namespaces by ryanofsky)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.
Concept ACK
Wow, this PR was a tough nut to crack! It took me quite a while to understand it thoroughly. That’s why I have attached my notes so that other reviewers might have an easier time reviewing this PR.
I agree with the changes introduced in this PR. Though I have some suggestions that might help improve this PR.
Notes:
restoreWallet in src/interfaces/wallet.h file. This function is then overridden in src/wallet/interfaces.h file.src/wallet/wallet.h file. This function does preliminary checks for:
backup_file exist.wallet_path (i.e., where wallet should be created) is empty, and, second, the directory can be created there.
After these steps, the LoadWallet function is called, creating a wallet variable. Finally, if the wallet variable does not exist (if(!wallet)), the wallet_file and wallet_path are removed from the system. Finally, the wallet variable is returned if everything works out correctly.src/wallet/rpc/util.cpp, which used to return {wallet, warning} has now been reduced to HandleWalletError. Reason:
restoreWallet() in src/wallet/rpc/backup.cpp file, and for loadWallet() in src/wallet/rpc/wallet.cpp file.restoreWallet() function was updated to use a more suitable RestoreWallet function, and loadWallet() is still using the LoadWallet function.0 DatabaseOptions options;
1 DatabaseStatus status;
2 options.require_existing = true;
3 bilingual_str error;
4 std::vector<bilingual_str> warnings;
test/functional/wallet_backup.py file:
356@@ -357,6 +357,35 @@ std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string&
357 return wallet;
358 }
359
360+std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const std::string& backup_file, const std::string& wallet_name, std::optional<bool> load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings)
In 33763d02ef1011ae71143c5e6e313c72d9b790a6 “refactor: Move restorewallet() logic to the wallet section”
options does not need to be an argument to this function. The DatabaseOptions for loading a restored wallet are always going to be the same so you can make that object inside of this function.
377+ fs::copy_file(backup_file, wallet_file, fs::copy_option::fail_if_exists);
378+
379+ auto wallet = LoadWallet(context, wallet_name, load_on_start, options, status, error, warnings);
380+
381+ if (!wallet) {
382+ fs::remove(wallet_file);
In 33763d02ef1011ae71143c5e6e313c72d9b790a6 “refactor: Move restorewallet() logic to the wallet section”
This is a change in behavior. I would prefer to have this added in its own commit.
1905
1906- fs::copy_file(backup_file, wallet_file, fs::copy_option::fail_if_exists);
1907+ const std::shared_ptr<CWallet> wallet = RestoreWallet(context, fs::PathToString(backup_file), wallet_name, load_on_start, options, status, error, warnings);
1908
1909- auto [wallet, warnings] = LoadWalletHelper(context, request.params[2], wallet_name);
1910+ HandleWalletError(wallet, status, error);
In 33763d02ef1011ae71143c5e6e313c72d9b790a6 “refactor: Move restorewallet() logic to the wallet section”
HandleWalletError does not handle FAILED_BAD_PATH or FAILED_ALREADY_EXISTS, which results in a slightly incorrect RPC_WALLET_ERROR code. There should be additional error handling for those cases to return better error codes.
121@@ -122,16 +122,8 @@ std::string LabelFromValue(const UniValue& value)
122 return label;
123 }
124
125-std::tuple<std::shared_ptr<CWallet>, std::vector<bilingual_str>> LoadWalletHelper(WalletContext& context, UniValue load_on_start_param, const std::string wallet_name)
126+void HandleWalletError(const std::shared_ptr<CWallet> wallet, DatabaseStatus& status, bilingual_str& error)
In 33763d02ef1011ae71143c5e6e313c72d9b790a6 “refactor: Move restorewallet() logic to the wallet section”
It would be easier to review this diff if this refactor were in its own commit.
Currently restorewallet() logic is written in the RPC layer
and it can´t be reused by GUI. So it reimplements this in the
wallet and interface sections and then, GUI can access it.
It also simplifies restorewallet() and loadwallet() RPC error handling.
219@@ -220,6 +220,7 @@ enum class DatabaseStatus {
220 FAILED_LOAD,
221 FAILED_VERIFY,
222 FAILED_ENCRYPT,
223+ FAILED_INVALID_BACKCUP_FILE,
In 4053a210a6126f85779e5ccab248d13a74d27897 “refactor: Move restorewallet() RPC logic to the wallet section”
nit: typo
0 FAILED_INVALID_BACKUP_FILE,
