Merge bitcoin/bitcoin#23721: wallet, refactor: Move restorewallet() logic to the wallet section

62fa61fa4a refactor: remove the wallet folder if the restore fails (w0xlt)
abbb7eccef refactor: Move restorewallet() RPC logic to the wallet section (w0xlt)
4807f73f48 refactor: Implement restorewallet() logic in the wallet section (w0xlt)

Pull request description:

  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.

ACKs for top commit:
  achow101:
    ACK 62fa61fa4a
  shaavan:
    crACK 62fa61fa4a

Tree-SHA512: 7ccfbad5943f38616ba0c2dd443c97a4b5bc1f6612dbf5a9e7a0263100aba36671fae929a2e7688442667be394645f44484af137a4802f204a33c4689eb27c39

src/interfaces/wallet.h

@@ -322,6 +322,9 @@ public:
    //! Return default wallet directory.
    virtual std::string getWalletDir() = 0;
 
+   //! Restore backup wallet
+   virtual std::unique_ptr<Wallet> restoreWallet(const std::string& backup_file, const std::string& wallet_name, bilingual_str& error, std::vector<bilingual_str>& warnings) = 0;
+
    //! Return available wallets in wallet directory.
    virtual std::vector<std::string> listWalletDir() = 0;
 

src/rpc/protocol.h

@@ -80,6 +80,7 @@ enum RPCErrorCode
     RPC_WALLET_NOT_FOUND            = -18, //!< Invalid wallet specified
     RPC_WALLET_NOT_SPECIFIED        = -19, //!< No wallet specified (error when there are multiple wallets loaded)
     RPC_WALLET_ALREADY_LOADED       = -35, //!< This same wallet is already loaded
+    RPC_WALLET_ALREADY_EXISTS       = -36, //!< There is already a wallet with the same name
 
     //! Backwards compatible aliases
     RPC_WALLET_INVALID_ACCOUNT_NAME = RPC_WALLET_INVALID_LABEL_NAME,

src/wallet/db.h

@@ -220,6 +220,7 @@ enum class DatabaseStatus {
     FAILED_LOAD,
     FAILED_VERIFY,
     FAILED_ENCRYPT,
+    FAILED_INVALID_BACKUP_FILE,
 };
 
 /** Recursively list database paths in directory. */

src/wallet/interfaces.cpp

@@ -552,6 +552,12 @@ public:
         options.require_existing = true;
         return MakeWallet(m_context, LoadWallet(m_context, name, true /* load_on_start */, options, status, error, warnings));
     }
+    std::unique_ptr<Wallet> restoreWallet(const std::string& backup_file, const std::string& wallet_name, bilingual_str& error, std::vector<bilingual_str>& warnings) override
+    {
+        DatabaseStatus status;
+
+        return MakeWallet(m_context, RestoreWallet(m_context, backup_file, wallet_name, /*load_on_start=*/true, status, error, warnings));
+    }
     std::string getWalletDir() override
     {
         return fs::PathToString(GetWalletDir());

src/wallet/rpc/backup.cpp

@@ -1879,27 +1879,17 @@ RPCHelpMan restorewallet()
 
     auto backup_file = fs::u8path(request.params[1].get_str());
 
-    if (!fs::exists(backup_file)) {
-        throw JSONRPCError(RPC_INVALID_PARAMETER, "Backup file does not exist");
-    }
-
     std::string wallet_name = request.params[0].get_str();
 
-    const fs::path wallet_path = fsbridge::AbsPathJoin(GetWalletDir(), fs::u8path(wallet_name));
+    std::optional<bool> load_on_start = request.params[2].isNull() ? std::nullopt : std::optional<bool>(request.params[2].get_bool());
 
-    if (fs::exists(wallet_path)) {
-        throw JSONRPCError(RPC_INVALID_PARAMETER, "Wallet name already exists.");
-    }
+    DatabaseStatus status;
+    bilingual_str error;
+    std::vector<bilingual_str> warnings;
 
-    if (!TryCreateDirectories(wallet_path)) {
-        throw JSONRPCError(RPC_WALLET_ERROR, strprintf("Failed to create database path '%s'. Database already exists.", wallet_path.u8string()));
-    }
+    const std::shared_ptr<CWallet> wallet = RestoreWallet(context, fs::PathToString(backup_file), wallet_name, load_on_start, status, error, warnings);
 
-    auto wallet_file = wallet_path / "wallet.dat";
-
-    fs::copy_file(backup_file, wallet_file, fs::copy_option::fail_if_exists);
-
-    auto [wallet, warnings] = LoadWalletHelper(context, request.params[2], wallet_name);
+    HandleWalletError(wallet, status, error);
 
     UniValue obj(UniValue::VOBJ);
     obj.pushKV("name", wallet->GetName());

src/wallet/rpc/util.cpp

@@ -122,16 +122,8 @@ std::string LabelFromValue(const UniValue& value)
     return label;
 }
 
-std::tuple<std::shared_ptr<CWallet>, std::vector<bilingual_str>> LoadWalletHelper(WalletContext& context, UniValue load_on_start_param, const std::string wallet_name)
+void HandleWalletError(const std::shared_ptr<CWallet> wallet, DatabaseStatus& status, bilingual_str& error)
 {
-    DatabaseOptions options;
-    DatabaseStatus status;
-    options.require_existing = true;
-    bilingual_str error;
-    std::vector<bilingual_str> warnings;
-    std::optional<bool> load_on_start = load_on_start_param.isNull() ? std::nullopt : std::optional<bool>(load_on_start_param.get_bool());
-    std::shared_ptr<CWallet> const wallet = LoadWallet(context, wallet_name, load_on_start, options, status, error, warnings);
-
     if (!wallet) {
         // Map bad format to not found, since bad format is returned when the
         // wallet directory exists, but doesn't contain a data file.
@@ -144,11 +136,15 @@ std::tuple<std::shared_ptr<CWallet>, std::vector<bilingual_str>> LoadWalletHelpe
             case DatabaseStatus::FAILED_ALREADY_LOADED:
                 code = RPC_WALLET_ALREADY_LOADED;
                 break;
+            case DatabaseStatus::FAILED_ALREADY_EXISTS:
+                code = RPC_WALLET_ALREADY_EXISTS;
+                break;
+            case DatabaseStatus::FAILED_INVALID_BACKUP_FILE:
+                code = RPC_INVALID_PARAMETER;
+                break;
             default: // RPC_WALLET_ERROR is returned for all other cases.
                 break;
         }
         throw JSONRPCError(code, error.original);
     }
-
-    return { wallet, warnings };
-}
+}

src/wallet/rpc/util.h

@@ -12,6 +12,7 @@
 
 struct bilingual_str;
 class CWallet;
+enum class DatabaseStatus;
 class JSONRPCRequest;
 class LegacyScriptPubKeyMan;
 class UniValue;
@@ -37,6 +38,6 @@ bool GetAvoidReuseFlag(const CWallet& wallet, const UniValue& param);
 bool ParseIncludeWatchonly(const UniValue& include_watchonly, const CWallet& wallet);
 std::string LabelFromValue(const UniValue& value);
 
-std::tuple<std::shared_ptr<CWallet>, std::vector<bilingual_str>> LoadWalletHelper(WalletContext& context, UniValue load_on_start_param, const std::string wallet_name);
+void HandleWalletError(const std::shared_ptr<CWallet> wallet, DatabaseStatus& status, bilingual_str& error);
 
 #endif // BITCOIN_WALLET_RPC_UTIL_H

src/wallet/rpc/wallet.cpp

@@ -215,7 +215,15 @@ static RPCHelpMan loadwallet()
     WalletContext& context = EnsureWalletContext(request.context);
     const std::string name(request.params[0].get_str());
 
-    auto [wallet, warnings] = LoadWalletHelper(context, request.params[1], name);
+    DatabaseOptions options;
+    DatabaseStatus status;
+    options.require_existing = true;
+    bilingual_str error;
+    std::vector<bilingual_str> warnings;
+    std::optional<bool> load_on_start = request.params[1].isNull() ? std::nullopt : std::optional<bool>(request.params[1].get_bool());
+    std::shared_ptr<CWallet> const wallet = LoadWallet(context, name, load_on_start, options, status, error, warnings);
+
+    HandleWalletError(wallet, status, error);
 
     UniValue obj(UniValue::VOBJ);
     obj.pushKV("name", wallet->GetName());

src/wallet/wallet.cpp

@@ -357,6 +357,38 @@ std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string&
     return wallet;
 }
 
+std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const std::string& backup_file, const std::string& wallet_name, std::optional<bool> load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings)
+{
+    DatabaseOptions options;
+    options.require_existing = true;
+
+    if (!fs::exists(fs::u8path(backup_file))) {
+        error = Untranslated("Backup file does not exist");
+        status = DatabaseStatus::FAILED_INVALID_BACKUP_FILE;
+        return nullptr;
+    }
+
+    const fs::path wallet_path = fsbridge::AbsPathJoin(GetWalletDir(), fs::u8path(wallet_name));
+
+    if (fs::exists(wallet_path) || !TryCreateDirectories(wallet_path)) {
+        error = Untranslated(strprintf("Failed to create database path '%s'. Database already exists.", fs::PathToString(wallet_path)));
+        status = DatabaseStatus::FAILED_ALREADY_EXISTS;
+        return nullptr;
+    }
+
+    auto wallet_file = wallet_path / "wallet.dat";
+    fs::copy_file(backup_file, wallet_file, fs::copy_option::fail_if_exists);
+
+    auto wallet = LoadWallet(context, wallet_name, load_on_start, options, status, error, warnings);
+
+    if (!wallet) {
+        fs::remove(wallet_file);
+        fs::remove(wallet_path);
+    }
+
+    return wallet;
+}
+
 /** @defgroup mapWallet
  *
  * @{

src/wallet/wallet.h

@@ -60,6 +60,7 @@ std::vector<std::shared_ptr<CWallet>> GetWallets(WalletContext& context);
 std::shared_ptr<CWallet> GetWallet(WalletContext& context, const std::string& name);
 std::shared_ptr<CWallet> LoadWallet(WalletContext& context, const std::string& name, std::optional<bool> load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings);
 std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string& name, std::optional<bool> load_on_start, DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings);
+std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const std::string& backup_file, const std::string& wallet_name, std::optional<bool> load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings);
 std::unique_ptr<interfaces::Handler> HandleLoadWallet(WalletContext& context, LoadWalletFn load_wallet);
 std::unique_ptr<WalletDatabase> MakeWalletDatabase(const std::string& name, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error);
 

test/functional/wallet_backup.py

@@ -110,17 +110,32 @@ class WalletBackupTest(BitcoinTestFramework):
         os.remove(os.path.join(self.nodes[1].datadir, self.chain, 'wallets', self.default_wallet_name, self.wallet_data_filename))
         os.remove(os.path.join(self.nodes[2].datadir, self.chain, 'wallets', self.default_wallet_name, self.wallet_data_filename))
 
+    def restore_invalid_wallet(self):
+        node = self.nodes[3]
+        invalid_wallet_file = os.path.join(self.nodes[0].datadir, 'invalid_wallet_file.bak')
+        open(invalid_wallet_file, 'a', encoding="utf8").write('invald wallet')
+        wallet_name = "res0"
+        not_created_wallet_file = os.path.join(node.datadir, self.chain, 'wallets', wallet_name)
+        error_message = "Wallet file verification failed. Failed to load database path '{}'. Data is not in recognized format.".format(not_created_wallet_file)
+        assert_raises_rpc_error(-18, error_message, node.restorewallet, wallet_name, invalid_wallet_file)
+        assert not os.path.exists(not_created_wallet_file)
+
     def restore_nonexistent_wallet(self):
         node = self.nodes[3]
         nonexistent_wallet_file = os.path.join(self.nodes[0].datadir, 'nonexistent_wallet.bak')
         wallet_name = "res0"
         assert_raises_rpc_error(-8, "Backup file does not exist", node.restorewallet, wallet_name, nonexistent_wallet_file)
+        not_created_wallet_file = os.path.join(node.datadir, self.chain, 'wallets', wallet_name)
+        assert not os.path.exists(not_created_wallet_file)
 
     def restore_wallet_existent_name(self):
         node = self.nodes[3]
-        wallet_file = os.path.join(self.nodes[0].datadir, 'wallet.bak')
+        backup_file = os.path.join(self.nodes[0].datadir, 'wallet.bak')
         wallet_name = "res0"
-        assert_raises_rpc_error(-8, "Wallet name already exists.", node.restorewallet, wallet_name, wallet_file)
+        wallet_file = os.path.join(node.datadir, self.chain, 'wallets', wallet_name)
+        error_message = "Failed to create database path '{}'. Database already exists.".format(wallet_file)
+        assert_raises_rpc_error(-36, error_message, node.restorewallet, wallet_name, backup_file)
+        assert os.path.exists(wallet_file)
 
     def init_three(self):
         self.init_wallet(node=0)
@@ -177,6 +192,7 @@ class WalletBackupTest(BitcoinTestFramework):
         ##
         self.log.info("Restoring wallets on node 3 using backup files")
 
+        self.restore_invalid_wallet()
         self.restore_nonexistent_wallet()
 
         backup_file_0 = os.path.join(self.nodes[0].datadir, 'wallet.bak')
@@ -187,6 +203,10 @@ class WalletBackupTest(BitcoinTestFramework):
         self.nodes[3].restorewallet("res1", backup_file_1)
         self.nodes[3].restorewallet("res2", backup_file_2)
 
+        assert os.path.exists(os.path.join(self.nodes[3].datadir, self.chain, 'wallets', "res0"))
+        assert os.path.exists(os.path.join(self.nodes[3].datadir, self.chain, 'wallets', "res1"))
+        assert os.path.exists(os.path.join(self.nodes[3].datadir, self.chain, 'wallets', "res2"))
+
         res0_rpc = self.nodes[3].get_wallet_rpc("res0")
         res1_rpc = self.nodes[3].get_wallet_rpc("res1")
         res2_rpc = self.nodes[3].get_wallet_rpc("res2")