From 3c815cfe54087fd139169161d2fd175e99840e6a Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 4 Aug 2020 19:33:37 -0400 Subject: [PATCH] wallet: Remove Verify and IsLoaded methods Checks are now consolidated in MakeBerkeleyDatabase function instead of happening in higher level code. This commit does not change behavior except for error messages which now include more complete information. --- src/wallet/bdb.cpp | 12 ------------ src/wallet/bdb.h | 6 +----- src/wallet/db.h | 4 ---- src/wallet/load.cpp | 8 ++++---- src/wallet/wallet.cpp | 27 ++++++--------------------- src/wallet/wallet.h | 4 +--- src/wallet/walletdb.cpp | 5 ----- src/wallet/walletdb.h | 3 --- test/functional/wallet_multiwallet.py | 6 ++++-- 9 files changed, 16 insertions(+), 59 deletions(-) diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index 61463aaf5e443..8f8652bb0b133 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -52,18 +52,6 @@ bool WalletDatabaseFileId::operator==(const WalletDatabaseFileId& rhs) const return memcmp(value, &rhs.value, sizeof(value)) == 0; } -bool IsBDBWalletLoaded(const fs::path& wallet_path) -{ - fs::path env_directory; - std::string database_filename; - SplitWalletPath(wallet_path, env_directory, database_filename); - LOCK(cs_db); - auto env = g_dbenvs.find(env_directory.string()); - if (env == g_dbenvs.end()) return false; - auto database = env->second.lock(); - return database && database->IsDatabaseLoaded(database_filename); -} - /** * @param[in] wallet_path Path to wallet directory. Or (for backwards compatibility only) a path to a berkeley btree data file inside a wallet directory. * @param[out] database_filename Filename of berkeley btree data file inside the wallet directory. diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h index 82ad136649e3b..9f24a2f10bc87 100644 --- a/src/wallet/bdb.h +++ b/src/wallet/bdb.h @@ -63,7 +63,6 @@ class BerkeleyEnvironment bool IsMock() const { return fMockDb; } bool IsInitialized() const { return fDbEnvInit; } - bool IsDatabaseLoaded(const std::string& db_filename) const { return m_databases.find(db_filename) != m_databases.end(); } fs::path Directory() const { return strPath; } bool Open(bilingual_str& error); @@ -87,9 +86,6 @@ class BerkeleyEnvironment /** Get BerkeleyEnvironment and database filename given a wallet path. */ std::shared_ptr GetWalletEnv(const fs::path& wallet_path, std::string& database_filename); -/** Return whether a BDB wallet database is currently loaded. */ -bool IsBDBWalletLoaded(const fs::path& wallet_path); - /** Check format of database file */ bool IsBerkeleyBtree(const fs::path& path); @@ -146,7 +142,7 @@ class BerkeleyDatabase : public WalletDatabase void ReloadDbEnv() override; /** Verifies the environment and database file */ - bool Verify(bilingual_str& error) override; + bool Verify(bilingual_str& error); /** * Pointer to shared database environment. diff --git a/src/wallet/db.h b/src/wallet/db.h index 6a918ec925f9a..6e11d7de88eb3 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -147,9 +147,6 @@ class WalletDatabase unsigned int nLastFlushed; int64_t nLastWalletUpdate; - /** Verifies the environment and database file */ - virtual bool Verify(bilingual_str& error) = 0; - std::string m_file_path; /** Make a DatabaseBatch connected to this database */ @@ -192,7 +189,6 @@ class DummyDatabase : public WalletDatabase bool PeriodicFlush() override { return true; } void IncrementUpdateCounter() override { ++nUpdateCounter; } void ReloadDbEnv() override {} - bool Verify(bilingual_str& errorStr) override { return true; } std::unique_ptr MakeBatch(const char* mode = "r+", bool flush_on_close = true) override { return MakeUnique(); } }; diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp index dde29842ecc57..84dc5adf662a2 100644 --- a/src/wallet/load.cpp +++ b/src/wallet/load.cpp @@ -52,11 +52,11 @@ bool VerifyWallets(interfaces::Chain& chain, const std::vector& wal return false; } + DatabaseOptions options; + DatabaseStatus status; + options.verify = true; bilingual_str error_string; - std::vector warnings; - bool verify_success = CWallet::Verify(chain, wallet_file, error_string, warnings); - if (!warnings.empty()) chain.initWarning(Join(warnings, Untranslated("\n"))); - if (!verify_success) { + if (!MakeWalletDatabase(wallet_file, options, status, error_string)) { chain.initError(error_string); return false; } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 4d7c51401966d..b5928f2b7e0d4 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -203,7 +203,7 @@ namespace { std::shared_ptr LoadWalletInternal(interfaces::Chain& chain, const std::string& name, Optional load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector& warnings) { try { - if (!CWallet::Verify(chain, name, error, warnings)) { + if (!MakeWalletDatabase(name, options, status, error)) { error = Untranslated("Wallet file verification failed.") + Untranslated(" ") + error; return nullptr; } @@ -260,7 +260,7 @@ std::shared_ptr CreateWallet(interfaces::Chain& chain, const std::strin } // Wallet::Verify will check if we're trying to create a wallet with a duplicate name. - if (!CWallet::Verify(chain, name, error, warnings)) { + if (!MakeWalletDatabase(name, options, status, error)) { error = Untranslated("Wallet file verification failed.") + Untranslated(" ") + error; status = DatabaseStatus::FAILED_VERIFY; return nullptr; @@ -3779,7 +3779,7 @@ std::vector CWallet::GetDestValues(const std::string& prefix) const return values; } -bool CWallet::Verify(interfaces::Chain& chain, const std::string& name, bilingual_str& error_string, std::vector& warnings) +std::unique_ptr MakeWalletDatabase(const std::string& name, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error_string) { // Do some checking on wallet path. It should be either a: // @@ -3787,7 +3787,6 @@ bool CWallet::Verify(interfaces::Chain& chain, const std::string& name, bilingua // 2. Path to an existing directory. // 3. Path to a symlink to a directory. // 4. For backwards compatibility, the name of a data file in -walletdir. - LOCK(cs_wallets); const fs::path& wallet_path = fs::absolute(name, GetWalletDir()); fs::file_type path_type = fs::symlink_status(wallet_path).type(); if (!(path_type == fs::file_not_found || path_type == fs::directory_file || @@ -3798,24 +3797,10 @@ bool CWallet::Verify(interfaces::Chain& chain, const std::string& name, bilingua "database/log.?????????? files can be stored, a location where such a directory could be created, " "or (for backwards compatibility) the name of an existing data file in -walletdir (%s)", name, GetWalletDir())); - return false; - } - - // Make sure that the wallet path doesn't clash with an existing wallet path - if (IsWalletLoaded(wallet_path)) { - error_string = Untranslated(strprintf("Error loading wallet %s. Duplicate -wallet filename specified.", name)); - return false; - } - - // Keep same database environment instance across Verify/Recover calls below. - std::unique_ptr database = CreateWalletDatabase(wallet_path); - - try { - return database->Verify(error_string); - } catch (const fs::filesystem_error& e) { - error_string = Untranslated(strprintf("Error loading wallet %s. %s", name, fsbridge::get_filesystem_error_message(e))); - return false; + status = DatabaseStatus::FAILED_BAD_PATH; + return nullptr; } + return MakeDatabase(wallet_path, options, status, error_string); } std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, const std::string& name, bilingual_str& error, std::vector& warnings, uint64_t wallet_creation_flags) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 2e6434f5cad5b..c9ebb94dc14f1 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -57,6 +57,7 @@ std::shared_ptr GetWallet(const std::string& name); std::shared_ptr LoadWallet(interfaces::Chain& chain, const std::string& name, Optional load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector& warnings); std::shared_ptr CreateWallet(interfaces::Chain& chain, const std::string& name, Optional load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector& warnings); std::unique_ptr HandleLoadWallet(LoadWalletFn load_wallet); +std::unique_ptr MakeWalletDatabase(const std::string& name, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error); //! -paytxfee default constexpr CAmount DEFAULT_PAY_TX_FEE = 0; @@ -1144,9 +1145,6 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati /** Mark a transaction as replaced by another transaction (e.g., BIP 125). */ bool MarkReplaced(const uint256& originalHash, const uint256& newHash); - //! Verify wallet naming and perform salvage on the wallet if required - static bool Verify(interfaces::Chain& chain, const std::string& name, bilingual_str& error_string, std::vector& warnings); - /* Initializes the wallet, returns a new CWallet instance or a null pointer in case of an error */ static std::shared_ptr CreateWalletFromFile(interfaces::Chain& chain, const std::string& name, bilingual_str& error, std::vector& warnings, uint64_t wallet_creation_flags = 0); diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 23c4b6977794a..29ac52cb3e242 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -1032,11 +1032,6 @@ std::unique_ptr MakeDatabase(const fs::path& path, const Databas return MakeBerkeleyDatabase(path, options, status, error); } -bool IsWalletLoaded(const fs::path& wallet_path) -{ - return IsBDBWalletLoaded(wallet_path); -} - /** Return object for accessing database at specified path. */ std::unique_ptr CreateWalletDatabase(const fs::path& path) { diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index 2548c17508cf5..7e83e3902a280 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -285,9 +285,6 @@ using KeyFilterFn = std::function; //! Unserialize a given Key-Value pair and load it into the wallet bool ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, std::string& strType, std::string& strErr, const KeyFilterFn& filter_fn = nullptr); -/** Return whether a wallet database is currently loaded. */ -bool IsWalletLoaded(const fs::path& wallet_path); - /** Return object for accessing database at specified path. */ std::unique_ptr CreateWalletDatabase(const fs::path& path); diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py index 5c9d7ff62938f..cc9d0ac4c4602 100755 --- a/test/functional/wallet_multiwallet.py +++ b/test/functional/wallet_multiwallet.py @@ -247,10 +247,12 @@ def wallet_file(name): assert_raises_rpc_error(-18, 'Wallet wallets not found.', self.nodes[0].loadwallet, 'wallets') # Fail to load duplicate wallets - assert_raises_rpc_error(-4, 'Wallet file verification failed. Error loading wallet w1. Duplicate -wallet filename specified.', self.nodes[0].loadwallet, wallet_names[0]) + path = os.path.join(self.options.tmpdir, "node0", "regtest", "wallets", "w1", "wallet.dat") + assert_raises_rpc_error(-4, "Wallet file verification failed. Refusing to load database. Data file '{}' is already loaded.".format(path), self.nodes[0].loadwallet, wallet_names[0]) # Fail to load duplicate wallets by different ways (directory and filepath) - assert_raises_rpc_error(-4, "Wallet file verification failed. Error loading wallet wallet.dat. Duplicate -wallet filename specified.", self.nodes[0].loadwallet, 'wallet.dat') + path = os.path.join(self.options.tmpdir, "node0", "regtest", "wallets", "wallet.dat") + assert_raises_rpc_error(-4, "Wallet file verification failed. Refusing to load database. Data file '{}' is already loaded.".format(path), self.nodes[0].loadwallet, 'wallet.dat') # Fail to load if one wallet is a copy of another assert_raises_rpc_error(-4, "BerkeleyDatabase: Can't open database w8_copy (duplicates fileid", self.nodes[0].loadwallet, 'w8_copy')