Merge #19324: wallet: Move BerkeleyBatch static functions to BerkeleyDatabase

d8e9ca66d1 walletdb: Move Rewrite into BerkeleyDatabase (Andrew Chow)
91d109156d walletdb: Move PeriodicFlush into WalletDatabase (Andrew Chow)
8f1bcf8b7b walletdb: Combine VerifyDatabaseFile and VerifyEnvironment (Andrew Chow)

Pull request description:

  The `BerkeleyBatch` class has 4 static functions that operate on `BerkeleyDatabase` or `BerkeleyEnvironment`. It doesn't make sense for these to be standalone nor for them to be static functions. So instead, move them from `BerkeleyBatch` into `BerkeleyDatabase` and make them member functions instead of static.

  `BerkeleyBatch::VerifyEnvironment` and `BerkeleyBatch::VerifyDatabaseFile` are combined into a single `BerkeleyDatabase::Verify` function that operates on that `BerkeleyDatabase` object.

  `BerkeleyBatch::Rewrite` and `BerkeleyBatch::PeriodicFlush` both took a `BerkeleyDatabase` as an argument and did stuff on it. So we just make it a member function so it doesn't need to take a database as an argument.

  Part of #18971

ACKs for top commit:
  MarcoFalke:
    re-ACK d8e9ca66d1 only change is test fixup 🤞
  promag:
    Code review ACK d8e9ca66d1, good stuff.

Tree-SHA512: 9847e55b13d98bf4e5636cc14bc3f5351d56737f7e320fafffaed128606240765599e5400382c5aecac06690f7e36265ca3e1031f3f6d8a9688f6d5cb1bacd2a

src/wallet/bdb.cpp

@@ -292,11 +292,10 @@ BerkeleyBatch::SafeDbt::operator Dbt*()
     return &m_dbt;
 }
 
-bool BerkeleyBatch::VerifyEnvironment(const fs::path& file_path, bilingual_str& errorStr)
+bool BerkeleyDatabase::Verify(bilingual_str& errorStr)
 {
-    std::string walletFile;
-    std::shared_ptr<BerkeleyEnvironment> env = GetWalletEnv(file_path, walletFile);
     fs::path walletDir = env->Directory();
+    fs::path file_path = walletDir / strFile;
 
     LogPrintf("Using BerkeleyDB version %s\n", BerkeleyDatabaseVersion());
     LogPrintf("Using wallet %s\n", file_path.string());
@@ -306,19 +305,10 @@ bool BerkeleyBatch::VerifyEnvironment(const fs::path& file_path, bilingual_str&
         return false;
     }
 
-    return true;
-}
-
-bool BerkeleyBatch::VerifyDatabaseFile(const fs::path& file_path, bilingual_str& errorStr)
-{
-    std::string walletFile;
-    std::shared_ptr<BerkeleyEnvironment> env = GetWalletEnv(file_path, walletFile);
-    fs::path walletDir = env->Directory();
-
-    if (fs::exists(walletDir / walletFile))
+    if (fs::exists(file_path))
     {
-        if (!env->Verify(walletFile)) {
-            errorStr = strprintf(_("%s corrupt. Try using the wallet tool bitcoin-wallet to salvage or restoring a backup."), walletFile);
+        if (!env->Verify(strFile)) {
+            errorStr = strprintf(_("%s corrupt. Try using the wallet tool bitcoin-wallet to salvage or restoring a backup."), file_path);
             return false;
         }
     }
@@ -495,13 +485,11 @@ void BerkeleyEnvironment::ReloadDbEnv()
     Open(true);
 }
 
-bool BerkeleyBatch::Rewrite(BerkeleyDatabase& database, const char* pszSkip)
+bool BerkeleyDatabase::Rewrite(const char* pszSkip)
 {
-    if (database.IsDummy()) {
+    if (IsDummy()) {
         return true;
     }
-    BerkeleyEnvironment *env = database.env.get();
-    const std::string& strFile = database.strFile;
     while (true) {
         {
             LOCK(cs_db);
@@ -515,7 +503,7 @@ bool BerkeleyBatch::Rewrite(BerkeleyDatabase& database, const char* pszSkip)
                 LogPrintf("BerkeleyBatch::Rewrite: Rewriting %s...\n", strFile);
                 std::string strFileRes = strFile + ".rewrite";
                 { // surround usage of db with extra {}
-                    BerkeleyBatch db(database, "r");
+                    BerkeleyBatch db(*this, "r");
                     std::unique_ptr<Db> pdbCopy = MakeUnique<Db>(env->dbenv.get(), 0);
 
                     int ret = pdbCopy->open(nullptr,               // Txn pointer
@@ -625,14 +613,12 @@ void BerkeleyEnvironment::Flush(bool fShutdown)
     }
 }
 
-bool BerkeleyBatch::PeriodicFlush(BerkeleyDatabase& database)
+bool BerkeleyDatabase::PeriodicFlush()
 {
-    if (database.IsDummy()) {
+    if (IsDummy()) {
         return true;
     }
     bool ret = false;
-    BerkeleyEnvironment *env = database.env.get();
-    const std::string& strFile = database.strFile;
     TRY_LOCK(cs_db, lockDb);
     if (lockDb)
     {
@@ -667,11 +653,6 @@ bool BerkeleyBatch::PeriodicFlush(BerkeleyDatabase& database)
     return ret;
 }
 
-bool BerkeleyDatabase::Rewrite(const char* pszSkip)
-{
-    return BerkeleyBatch::Rewrite(*this, pszSkip);
-}
-
 bool BerkeleyDatabase::Backup(const std::string& strDest) const
 {
     if (IsDummy()) {

src/wallet/bdb.h

@@ -131,6 +131,9 @@ public:
     /** Make sure all changes are flushed to disk.
      */
     void Flush(bool shutdown);
+    /* flush the wallet passively (TRY_LOCK)
+       ideal to be called periodically */
+    bool PeriodicFlush();
 
     void IncrementUpdateCounter();
 
@@ -141,6 +144,9 @@ public:
     unsigned int nLastFlushed;
     int64_t nLastWalletUpdate;
 
+    /** Verifies the environment and database file */
+    bool Verify(bilingual_str& error);
+
     /**
      * Pointer to shared database environment.
      *
@@ -213,14 +219,6 @@ public:
     void Flush();
     void Close();
 
-    /* flush the wallet passively (TRY_LOCK)
-       ideal to be called periodically */
-    static bool PeriodicFlush(BerkeleyDatabase& database);
-    /* verifies the database environment */
-    static bool VerifyEnvironment(const fs::path& file_path, bilingual_str& errorStr);
-    /* verifies the database file */
-    static bool VerifyDatabaseFile(const fs::path& file_path, bilingual_str& errorStr);
-
     template <typename K, typename T>
     bool Read(const K& key, T& value)
     {
@@ -291,8 +289,6 @@ public:
     bool TxnBegin();
     bool TxnCommit();
     bool TxnAbort();
-
-    bool static Rewrite(BerkeleyDatabase& database, const char* pszSkip = nullptr);
 };
 
 std::string BerkeleyDatabaseVersion();

src/wallet/salvage.cpp

@@ -20,6 +20,11 @@ bool RecoverDatabaseFile(const fs::path& file_path)
     std::string filename;
     std::shared_ptr<BerkeleyEnvironment> env = GetWalletEnv(file_path, filename);
 
+    if (!env->Open(true /* retry */)) {
+        tfm::format(std::cerr, "Error initializing wallet database environment %s!", env->Directory());
+        return false;
+    }
+
     // Recovery procedure:
     // move wallet file to walletfilename.timestamp.bak
     // Call Salvage with fAggressive=true to

src/wallet/wallet.cpp

@@ -3732,15 +3732,11 @@ bool CWallet::Verify(interfaces::Chain& chain, const WalletLocation& location, b
     std::unique_ptr<WalletDatabase> database = CreateWalletDatabase(wallet_path);
 
     try {
-        if (!WalletBatch::VerifyEnvironment(wallet_path, error_string)) {
-            return false;
-        }
+        return database->Verify(error_string);
     } catch (const fs::filesystem_error& e) {
         error_string = Untranslated(strprintf("Error loading wallet %s. %s", location.GetName(), fsbridge::get_filesystem_error_message(e)));
         return false;
     }
-
-    return WalletBatch::VerifyDatabaseFile(wallet_path, error_string);
 }
 
 std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain, const WalletLocation& location, bilingual_str& error, std::vector<bilingual_str>& warnings, uint64_t wallet_creation_flags)

src/wallet/walletdb.cpp

@@ -967,7 +967,7 @@ void MaybeCompactWalletDB()
         }
 
         if (dbh.nLastFlushed != nUpdateCounter && GetTime() - dbh.nLastWalletUpdate >= 2) {
-            if (BerkeleyBatch::PeriodicFlush(dbh)) {
+            if (dbh.PeriodicFlush()) {
                 dbh.nLastFlushed = nUpdateCounter;
             }
         }
@@ -976,16 +976,6 @@ void MaybeCompactWalletDB()
     fOneThread = false;
 }
 
-bool WalletBatch::VerifyEnvironment(const fs::path& wallet_path, bilingual_str& errorStr)
-{
-    return BerkeleyBatch::VerifyEnvironment(wallet_path, errorStr);
-}
-
-bool WalletBatch::VerifyDatabaseFile(const fs::path& wallet_path, bilingual_str& errorStr)
-{
-    return BerkeleyBatch::VerifyDatabaseFile(wallet_path, errorStr);
-}
-
 bool WalletBatch::WriteDestData(const std::string &address, const std::string &key, const std::string &value)
 {
     return WriteIC(std::make_pair(DBKeys::DESTDATA, std::make_pair(address, key)), value);

src/wallet/wallettool.cpp

@@ -112,7 +112,7 @@ static bool SalvageWallet(const fs::path& path)
     // Initialize the environment before recovery
     bilingual_str error_string;
     try {
-        WalletBatch::VerifyEnvironment(path, error_string);
+        database->Verify(error_string);
     } catch (const fs::filesystem_error& e) {
         error_string = Untranslated(strprintf("Error loading wallet. %s", fsbridge::get_filesystem_error_message(e)));
     }
@@ -140,11 +140,6 @@ bool ExecuteWalletToolFunc(const std::string& command, const std::string& name)
             tfm::format(std::cerr, "Error: no wallet file at %s\n", name);
             return false;
         }
-        bilingual_str error;
-        if (!WalletBatch::VerifyEnvironment(path, error)) {
-            tfm::format(std::cerr, "%s\nError loading %s. Is wallet being used by other process?\n", error.original, name);
-            return false;
-        }
 
         if (command == "info") {
             std::shared_ptr<CWallet> wallet_instance = LoadWallet(name, path);

test/functional/tool_wallet.py

@@ -71,8 +71,7 @@ class ToolWalletTest(BitcoinTestFramework):
         self.assert_raises_tool_error('Error: two methods provided (info and create). Only one method should be provided.', 'info', 'create')
         self.assert_raises_tool_error('Error parsing command line arguments: Invalid parameter -foo', '-foo')
         self.assert_raises_tool_error(
-            'Error initializing wallet database environment "{}"!\nError loading wallet.dat. Is wallet being used by other process?'
-            .format(os.path.join(self.nodes[0].datadir, self.chain, 'wallets')),
+            'Error loading wallet.dat. Is wallet being used by another process?',
             '-wallet=wallet.dat',
             'info',
         )