In commit “Bugfix: Wallet: Wrap RestoreWallet content in a try block to ensure exceptions become returned errors and incomplete wallet directory is removed” (b60a2ce138f4549e7233542e21cd96e0d8e0b5f8)
This might just be difference of opinion about defensive programming, but IMO it makes code harder to reason about and maintain if it is catching exceptions that are never expected to be thrown. I think it would be better to only catch fs::filesystem_exception not std::exception, and only catch exceptions if copy_file throws, not if LoadWallet throws. I also think an error text like “failed to copy wallet” is better than “unexpected exception.” Possible suggestion:
0--- a/src/wallet/wallet.cpp
1+++ b/src/wallet/wallet.cpp
2@@ -394,17 +394,16 @@ std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const fs::path& b
3 }
4
5 auto wallet_file = wallet_path / "wallet.dat";
6- std::shared_ptr<CWallet> wallet;
7-
8+ status = DatabaseStatus::SUCCESS;
9 try {
10 fs::copy_file(backup_file, wallet_file, fs::copy_options::none);
11-
12- wallet = LoadWallet(context, wallet_name, load_on_start, options, status, error, warnings);
13- } catch (const std::exception& e) {
14- wallet.reset(); // just in case
15- if (!error.empty()) error += Untranslated("\n");
16- error += strprintf(Untranslated("Unexpected exception: %s"), e.what());
17+ } catch (const fs::filesystem_error& e) {
18+ error += Untranslated(strprintf(("Failed to copy wallet: %s"), e.what()));
19+ status = DatabaseStatus::FAILED_CREATE;
20 }
21+
22+ auto wallet = status == DatabaseStatus::SUCCESS ? LoadWallet(context, wallet_name, load_on_start, options, status, error, warnings) : nullptr;
23+
24 if (!wallet) {
25 fs::remove(wallet_file);
26 fs::remove(wallet_path);
