From 4807f73f48f4ff1084fcf7aee94e5b14592bfda8 Mon Sep 17 00:00:00 2001 From: w0xlt <94266259+w0xlt@users.noreply.github.com> Date: Tue, 14 Dec 2021 19:18:56 -0300 Subject: [PATCH 1/3] refactor: Implement restorewallet() logic in the wallet section MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/interfaces/wallet.h | 3 +++ src/wallet/interfaces.cpp | 6 ++++++ src/wallet/wallet.cpp | 25 +++++++++++++++++++++++++ src/wallet/wallet.h | 1 + 4 files changed, 35 insertions(+) diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index a56ed8802df..4213a22749c 100644 --- a/src/interfaces/wallet.h +++ b/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 restoreWallet(const std::string& backup_file, const std::string& wallet_name, bilingual_str& error, std::vector& warnings) = 0; + //! Return available wallets in wallet directory. virtual std::vector listWalletDir() = 0; diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 6c9d0ca1326..bba909b8071 100644 --- a/src/wallet/interfaces.cpp +++ b/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 restoreWallet(const std::string& backup_file, const std::string& wallet_name, bilingual_str& error, std::vector& 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()); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index cfe550cb3f9..59ed8c2571e 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -357,6 +357,31 @@ std::shared_ptr CreateWallet(WalletContext& context, const std::string& return wallet; } +std::shared_ptr RestoreWallet(WalletContext& context, const std::string& backup_file, const std::string& wallet_name, std::optional load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector& warnings) +{ + DatabaseOptions options; + options.require_existing = true; + + if (!fs::exists(fs::u8path(backup_file))) { + error = Untranslated("Backup file does not exist"); + status = DatabaseStatus::FAILED_BAD_PATH; + 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); + + return LoadWallet(context, wallet_name, load_on_start, options, status, error, warnings); +} + /** @defgroup mapWallet * * @{ diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index dbf0f6375d0..1dc40d228e2 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -60,6 +60,7 @@ std::vector> GetWallets(WalletContext& context); std::shared_ptr GetWallet(WalletContext& context, const std::string& name); std::shared_ptr LoadWallet(WalletContext& context, const std::string& name, std::optional load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector& warnings); std::shared_ptr CreateWallet(WalletContext& context, const std::string& name, std::optional load_on_start, DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector& warnings); +std::shared_ptr RestoreWallet(WalletContext& context, const std::string& backup_file, const std::string& wallet_name, std::optional load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector& warnings); std::unique_ptr HandleLoadWallet(WalletContext& context, LoadWalletFn load_wallet); std::unique_ptr MakeWalletDatabase(const std::string& name, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error); From abbb7eccef3fc1c36f34756458d2792f6661e29f Mon Sep 17 00:00:00 2001 From: w0xlt <94266259+w0xlt@users.noreply.github.com> Date: Tue, 14 Dec 2021 20:10:21 -0300 Subject: [PATCH 2/3] refactor: Move restorewallet() RPC logic to the wallet section It also simplifies restorewallet() and loadwallet() RPC error handling. --- src/rpc/protocol.h | 1 + src/wallet/db.h | 1 + src/wallet/rpc/backup.cpp | 22 ++++++---------------- src/wallet/rpc/util.cpp | 20 ++++++++------------ src/wallet/rpc/util.h | 3 ++- src/wallet/rpc/wallet.cpp | 10 +++++++++- src/wallet/wallet.cpp | 2 +- test/functional/wallet_backup.py | 8 ++++++-- 8 files changed, 34 insertions(+), 33 deletions(-) diff --git a/src/rpc/protocol.h b/src/rpc/protocol.h index fc00a1efad8..2fc9f55eba5 100644 --- a/src/rpc/protocol.h +++ b/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, diff --git a/src/wallet/db.h b/src/wallet/db.h index 7a0d3d2e07a..3336099eb9e 100644 --- a/src/wallet/db.h +++ b/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. */ diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index 135c3a15f2c..f9c80043942 100644 --- a/src/wallet/rpc/backup.cpp +++ b/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 load_on_start = request.params[2].isNull() ? std::nullopt : std::optional(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 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 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()); diff --git a/src/wallet/rpc/util.cpp b/src/wallet/rpc/util.cpp index e2126b72368..9a40a67ee5b 100644 --- a/src/wallet/rpc/util.cpp +++ b/src/wallet/rpc/util.cpp @@ -122,16 +122,8 @@ std::string LabelFromValue(const UniValue& value) return label; } -std::tuple, std::vector> LoadWalletHelper(WalletContext& context, UniValue load_on_start_param, const std::string wallet_name) +void HandleWalletError(const std::shared_ptr wallet, DatabaseStatus& status, bilingual_str& error) { - DatabaseOptions options; - DatabaseStatus status; - options.require_existing = true; - bilingual_str error; - std::vector warnings; - std::optional load_on_start = load_on_start_param.isNull() ? std::nullopt : std::optional(load_on_start_param.get_bool()); - std::shared_ptr 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::vector> 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 }; -} +} \ No newline at end of file diff --git a/src/wallet/rpc/util.h b/src/wallet/rpc/util.h index a1fa4d49b1d..5b00d2abcbc 100644 --- a/src/wallet/rpc/util.h +++ b/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::vector> LoadWalletHelper(WalletContext& context, UniValue load_on_start_param, const std::string wallet_name); +void HandleWalletError(const std::shared_ptr wallet, DatabaseStatus& status, bilingual_str& error); #endif // BITCOIN_WALLET_RPC_UTIL_H diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index 99ffe5d9644..31a5a018554 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/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 warnings; + std::optional load_on_start = request.params[1].isNull() ? std::nullopt : std::optional(request.params[1].get_bool()); + std::shared_ptr 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()); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 59ed8c2571e..f2d1acb71fd 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -364,7 +364,7 @@ std::shared_ptr RestoreWallet(WalletContext& context, const std::string if (!fs::exists(fs::u8path(backup_file))) { error = Untranslated("Backup file does not exist"); - status = DatabaseStatus::FAILED_BAD_PATH; + status = DatabaseStatus::FAILED_INVALID_BACKUP_FILE; return nullptr; } diff --git a/test/functional/wallet_backup.py b/test/functional/wallet_backup.py index 932df4fbff6..4bcfc3aa71c 100755 --- a/test/functional/wallet_backup.py +++ b/test/functional/wallet_backup.py @@ -115,12 +115,16 @@ class WalletBackupTest(BitcoinTestFramework): 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) def init_three(self): self.init_wallet(node=0) From 62fa61fa4a33ff4d108a65d656ffe2cac4330824 Mon Sep 17 00:00:00 2001 From: w0xlt <94266259+w0xlt@users.noreply.github.com> Date: Tue, 14 Dec 2021 20:49:11 -0300 Subject: [PATCH 3/3] refactor: remove the wallet folder if the restore fails --- src/wallet/wallet.cpp | 9 ++++++++- test/functional/wallet_backup.py | 16 ++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index f2d1acb71fd..1c61d7e981e 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -379,7 +379,14 @@ std::shared_ptr RestoreWallet(WalletContext& context, const std::string auto wallet_file = wallet_path / "wallet.dat"; fs::copy_file(backup_file, wallet_file, fs::copy_option::fail_if_exists); - return LoadWallet(context, wallet_name, load_on_start, options, status, error, warnings); + 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 diff --git a/test/functional/wallet_backup.py b/test/functional/wallet_backup.py index 4bcfc3aa71c..292fe3a3109 100755 --- a/test/functional/wallet_backup.py +++ b/test/functional/wallet_backup.py @@ -110,6 +110,16 @@ 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') @@ -125,6 +135,7 @@ class WalletBackupTest(BitcoinTestFramework): 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) @@ -181,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') @@ -191,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")