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 61463aaf5e..8f8652bb0b 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 82ad136649..9f24a2f10b 100644 --- a/src/wallet/bdb.h +++ b/src/wallet/bdb.h @@ -63,7 +63,6 @@ public: 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 @@ public: /** 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 @@ public: 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 6a918ec925..6e11d7de88 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -147,9 +147,6 @@ public: 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 @@ public: 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 dde29842ec..84dc5adf66 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 4d7c514019..b5928f2b7e 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 2e6434f5ca..c9ebb94dc1 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 @@ public: /** 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 23c4b69777..29ac52cb3e 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 2548c17508..7e83e3902a 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 5c9d7ff629..cc9d0ac4c4 100755 --- a/test/functional/wallet_multiwallet.py +++ b/test/functional/wallet_multiwallet.py @@ -247,10 +247,12 @@ class MultiWalletTest(BitcoinTestFramework): 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')