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.

Author: ryanofsky

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.

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<BerkeleyEnvironment> 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.

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<DatabaseBatch> MakeBatch(const char* mode = "r+", bool flush_on_close = true) override { return MakeUnique<DummyBatch>(); }
 };
 

src/wallet/load.cpp

@@ -52,11 +52,11 @@ bool VerifyWallets(interfaces::Chain& chain, const std::vector<std::string>& wal
             return false;
         }
 
+        DatabaseOptions options;
+        DatabaseStatus status;
+        options.verify = true;
         bilingual_str error_string;
-        std::vector<bilingual_str> 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;
         }

src/wallet/wallet.cpp

@@ -203,7 +203,7 @@ namespace {
 std::shared_ptr<CWallet> LoadWalletInternal(interfaces::Chain& chain, const std::string& name, Optional<bool> load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& 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<CWallet> 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,15 +3779,14 @@ std::vector<std::string> 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<bilingual_str>& warnings)
+std::unique_ptr<WalletDatabase> 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:
     //
     // 1. Path where a directory can be created.
     // 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<WalletDatabase> 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> CWallet::CreateWalletFromFile(interfaces::Chain& chain, const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings, uint64_t wallet_creation_flags)

src/wallet/wallet.h

@@ -57,6 +57,7 @@ std::shared_ptr<CWallet> GetWallet(const std::string& name);
 std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, const std::string& name, Optional<bool> load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings);
 std::shared_ptr<CWallet> CreateWallet(interfaces::Chain& chain, const std::string& name, Optional<bool> load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings);
 std::unique_ptr<interfaces::Handler> HandleLoadWallet(LoadWalletFn load_wallet);
+std::unique_ptr<WalletDatabase> 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<bilingual_str>& warnings);
-
     /* Initializes the wallet, returns a new CWallet instance or a null pointer in case of an error */
     static std::shared_ptr<CWallet> CreateWalletFromFile(interfaces::Chain& chain, const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings, uint64_t wallet_creation_flags = 0);
 

src/wallet/walletdb.cpp

@@ -1032,11 +1032,6 @@ std::unique_ptr<WalletDatabase> 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<WalletDatabase> CreateWalletDatabase(const fs::path& path)
 {

src/wallet/walletdb.h

@@ -285,9 +285,6 @@ using KeyFilterFn = std::function<bool(const std::string&)>;
 //! 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<WalletDatabase> CreateWalletDatabase(const fs::path& path);
 

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')