Merge #18907: walletdb: Don't remove database transaction logs and instead error

d0ea9bab28 walletdb: Don't remove database transaction logs and instead error (Andrew Chow)

Pull request description:

  Instead of removing the database transaction logs and retrying the
  wallet loading, just return an error message to the user. Additionally,
  speciically for DB_RUNRECOVERY, notify the user that this could be due
  to different BDB versions.

  Kind of implements the suggestion from https://github.com/bitcoin/bitcoin/pull/18870#discussion_r421647964

ACKs for top commit:
  Sjors:
    re-utACK d0ea9bab28
  ryanofsky:
    Code review ACK d0ea9bab28. Only changes since last review are rebase and expanding error and commit messages.

Tree-SHA512: f6e67dc70f58188742a5c8af7cdc63a2b58779aa0d26ae7f1e75805a239f1a342433860e5a238d6577fae5ab04b9d15e7f11c55b867065dfd13781a6a62e4958

src/wallet/bdb.cpp

@@ -139,7 +139,7 @@ BerkeleyEnvironment::~BerkeleyEnvironment()
     Close();
 }
 
-bool BerkeleyEnvironment::Open(bool retry)
+bool BerkeleyEnvironment::Open(bilingual_str& err)
 {
     if (fDbEnvInit) {
         return true;
@@ -149,6 +149,7 @@ bool BerkeleyEnvironment::Open(bool retry)
     TryCreateDirectories(pathIn);
     if (!LockDirectory(pathIn, ".walletlock")) {
         LogPrintf("Cannot obtain a lock on wallet directory %s. Another instance of bitcoin may be using it.\n", strPath);
+        err = strprintf(_("Error initializing wallet database environment %s!"), Directory());
         return false;
     }
 
@@ -188,24 +189,12 @@ bool BerkeleyEnvironment::Open(bool retry)
             LogPrintf("BerkeleyEnvironment::Open: Error %d closing failed database environment: %s\n", ret2, DbEnv::strerror(ret2));
         }
         Reset();
-        if (retry) {
+        err = strprintf(_("Error initializing wallet database environment %s!"), Directory());
-            // try moving the database env out of the way
+        if (ret == DB_RUNRECOVERY) {
-            fs::path pathDatabaseBak = pathIn / strprintf("database.%d.bak", GetTime());
+            err += Untranslated(" ") + _("This error could occur if this wallet was not shutdown cleanly and was last loaded using a build with a newer version of Berkeley DB. If so, please use the software that last loaded this wallet");
-            try {
-                fs::rename(pathLogDir, pathDatabaseBak);
-                LogPrintf("Moved old %s to %s. Retrying.\n", pathLogDir.string(), pathDatabaseBak.string());
-            } catch (const fs::filesystem_error&) {
-                // failure is ok (well, not really, but it's not worse than what we started with)
         }
-            // try opening it again one more time
-            if (!Open(false /* retry */)) {
-                // if it still fails, it probably means we can't even create the database env
         return false;
     }
-        } else {
-            return false;
-        }
-    }
 
     fDbEnvInit = true;
     fMockDb = false;
@@ -300,8 +289,7 @@ bool BerkeleyDatabase::Verify(bilingual_str& errorStr)
     LogPrintf("Using BerkeleyDB version %s\n", BerkeleyDatabaseVersion());
     LogPrintf("Using wallet %s\n", file_path.string());
 
-    if (!env->Open(true /* retry */)) {
+    if (!env->Open(errorStr)) {
-        errorStr = strprintf(_("Error initializing wallet database environment %s!"), walletDir);
         return false;
     }
 
@@ -342,7 +330,8 @@ BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bo
 
     {
         LOCK(cs_db);
-        if (!env->Open(false /* retry */))
+        bilingual_str open_err;
+        if (!env->Open(open_err))
             throw std::runtime_error("BerkeleyBatch: Failed to open database environment.");
 
         pdb = database.m_db.get();
@@ -482,7 +471,8 @@ void BerkeleyEnvironment::ReloadDbEnv()
     // Reset the environment
     Flush(true); // This will flush and close the environment
     Reset();
-    Open(true);
+    bilingual_str open_err;
+    Open(open_err);
 }
 
 bool BerkeleyDatabase::Rewrite(const char* pszSkip)

src/wallet/bdb.h

@@ -69,7 +69,7 @@ public:
 
     bool Verify(const std::string& strFile);
 
-    bool Open(bool retry);
+    bool Open(bilingual_str& error);
     void Close();
     void Flush(bool fShutdown);
     void CheckpointLSN(const std::string& strFile);

src/wallet/salvage.cpp

@@ -5,6 +5,7 @@
 
 #include <fs.h>
 #include <streams.h>
+#include <util/translation.h>
 #include <wallet/salvage.h>
 #include <wallet/wallet.h>
 #include <wallet/walletdb.h>
@@ -20,8 +21,9 @@ bool RecoverDatabaseFile(const fs::path& file_path)
     std::string filename;
     std::shared_ptr<BerkeleyEnvironment> env = GetWalletEnv(file_path, filename);
 
-    if (!env->Open(true /* retry */)) {
+    bilingual_str open_err;
-        tfm::format(std::cerr, "Error initializing wallet database environment %s!", env->Directory());
+    if (!env->Open(open_err)) {
+        tfm::format(std::cerr, "%s\n", open_err.original);
         return false;
     }