diff options
author | MarcoFalke <falke.marco@gmail.com> | 2021-12-16 08:42:17 +0100 |
---|---|---|
committer | MarcoFalke <falke.marco@gmail.com> | 2021-12-16 08:42:44 +0100 |
commit | a30642926a5ee119ab4e1c97d83f76574f655d26 (patch) | |
tree | 9b3cedd66e671f2fd0e8dcedb63b66ee012daa91 | |
parent | 98c362a4d3d865b5e5ffe6649ffc33203abf8ac0 (diff) | |
parent | 62fa61fa4a33ff4d108a65d656ffe2cac4330824 (diff) | |
download | bitcoin-a30642926a5ee119ab4e1c97d83f76574f655d26.tar.xz |
Merge bitcoin/bitcoin#23721: wallet, refactor: Move restorewallet() logic to the wallet section
62fa61fa4a33ff4d108a65d656ffe2cac4330824 refactor: remove the wallet folder if the restore fails (w0xlt)
abbb7eccef3fc1c36f34756458d2792f6661e29f refactor: Move restorewallet() RPC logic to the wallet section (w0xlt)
4807f73f48f4ff1084fcf7aee94e5b14592bfda8 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 62fa61fa4a33ff4d108a65d656ffe2cac4330824
shaavan:
crACK 62fa61fa4a33ff4d108a65d656ffe2cac4330824
Tree-SHA512: 7ccfbad5943f38616ba0c2dd443c97a4b5bc1f6612dbf5a9e7a0263100aba36671fae929a2e7688442667be394645f44484af137a4802f204a33c4689eb27c39
-rw-r--r-- | src/interfaces/wallet.h | 3 | ||||
-rw-r--r-- | src/rpc/protocol.h | 1 | ||||
-rw-r--r-- | src/wallet/db.h | 1 | ||||
-rw-r--r-- | src/wallet/interfaces.cpp | 6 | ||||
-rw-r--r-- | src/wallet/rpc/backup.cpp | 22 | ||||
-rw-r--r-- | src/wallet/rpc/util.cpp | 20 | ||||
-rw-r--r-- | src/wallet/rpc/util.h | 3 | ||||
-rw-r--r-- | src/wallet/rpc/wallet.cpp | 10 | ||||
-rw-r--r-- | src/wallet/wallet.cpp | 32 | ||||
-rw-r--r-- | src/wallet/wallet.h | 1 | ||||
-rwxr-xr-x | test/functional/wallet_backup.py | 24 |
11 files changed, 91 insertions, 32 deletions
diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index a56ed8802d..4213a22749 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<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; diff --git a/src/rpc/protocol.h b/src/rpc/protocol.h index fc00a1efad..2fc9f55eba 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 7a0d3d2e07..3336099eb9 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/interfaces.cpp b/src/wallet/interfaces.cpp index 6c9d0ca132..bba909b807 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<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()); diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index e3daae9cea..7119c3bbbd 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)); - - if (fs::exists(wallet_path)) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Wallet name already exists."); - } - - if (!TryCreateDirectories(wallet_path)) { - throw JSONRPCError(RPC_WALLET_ERROR, strprintf("Failed to create database path '%s'. Database already exists.", wallet_path.u8string())); - } + std::optional<bool> load_on_start = request.params[2].isNull() ? std::nullopt : std::optional<bool>(request.params[2].get_bool()); - auto wallet_file = wallet_path / "wallet.dat"; + DatabaseStatus status; + bilingual_str error; + std::vector<bilingual_str> warnings; - fs::copy_file(backup_file, wallet_file, fs::copy_option::fail_if_exists); + const std::shared_ptr<CWallet> wallet = RestoreWallet(context, fs::PathToString(backup_file), wallet_name, load_on_start, status, error, warnings); - 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 e2126b7236..9a40a67ee5 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::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 }; -} +}
\ No newline at end of file diff --git a/src/wallet/rpc/util.h b/src/wallet/rpc/util.h index a1fa4d49b1..5b00d2abcb 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::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 diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index 09f50137c6..738f8258bc 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<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()); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c79e917c69..decdbc7090 100644 --- a/src/wallet/wallet.cpp +++ b/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 * * @{ diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 93150cc583..e2a2aebeb5 100644 --- a/src/wallet/wallet.h +++ b/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); diff --git a/test/functional/wallet_backup.py b/test/functional/wallet_backup.py index 932df4fbff..292fe3a310 100755 --- a/test/functional/wallet_backup.py +++ b/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") |