[Wallet] Clean wallet auto-backup "feature"

src/wallet/init.cpp

@@ -44,7 +44,6 @@ std::string GetWalletHelpString(bool showDebug)
         strUsage += HelpMessageGroup(_("Wallet debugging/testing options:"));
         strUsage += HelpMessageOpt("-dblogsize=<n>", strprintf(_("Flush database activity from memory pool to disk log every <n> megabytes (default: %u)"), DEFAULT_WALLET_DBLOGSIZE));
         strUsage += HelpMessageOpt("-flushwallet", strprintf(_("Run a thread to flush wallet periodically (default: %u)"), DEFAULT_FLUSHWALLET));
-        strUsage += HelpMessageOpt("-printcoinstake", _("Display verbose coin stake messages in the debug.log file."));
         strUsage += HelpMessageOpt("-privdb", strprintf(_("Sets the DB_PRIVATE flag in the wallet db environment (default: %u)"), DEFAULT_WALLET_PRIVDB));
     }
 
@@ -210,9 +209,15 @@ bool InitLoadWallet()
     }
 
     for (const std::string& walletFile : gArgs.GetArgs("-wallet")) {
-        // automatic backups
+        // create/load wallet
+        CWallet * const pwallet = CWallet::CreateWalletFromFile(walletFile);
+        if (!pwallet) {
+            return false;
+        }
+
+        // automatic backup
         std::string strWarning, strError;
-        if(!AutoBackupWallet(walletFile, strWarning, strError)) {
+        if (!AutoBackupWallet(*pwallet, strWarning, strError)) {
             if (!strWarning.empty()) {
                 UIWarning(strprintf("%s: %s", walletFile, strWarning));
             }
@@ -221,10 +226,7 @@ bool InitLoadWallet()
             }
         }
 
-        CWallet * const pwallet = CWallet::CreateWalletFromFile(walletFile);
-        if (!pwallet) {
-            return false;
-        }
+        // add to wallets in use
         vpwallets.emplace_back(pwallet);
     }
 

src/wallet/wallet.cpp

@@ -4079,7 +4079,7 @@ void CWallet::LockIfMyCollateral(const CTransactionRef& ptx)
     }
 }
 
-CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
+CWallet* CWallet::CreateWalletFromFile(const std::string& walletFile)
 {
     // needed to restore wallet transaction meta data after -zapwallettxes
     std::vector<CWalletTx> vWtx;

src/wallet/wallet.h

@@ -1155,7 +1155,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
     bool AbandonTransaction(const uint256& hashTx);
 
     /* Initializes the wallet, returns a new CWallet instance or a null pointer in case of an error */
-    static CWallet* CreateWalletFromFile(const std::string walletFile);
+    static CWallet* CreateWalletFromFile(const std::string& walletFile);
 
     /**
      * Wallet post-init setup

src/wallet/walletdb.cpp

@@ -855,88 +855,36 @@ std::pair<fs::path, fs::path> GetBackupPath(const CWallet& wallet)
     return {pathCustom, pathWithFile};
 }
 
-bool AutoBackupWallet(const std::string& strWalletFile, std::string& strBackupWarning, std::string& strBackupError)
+typedef std::multimap<std::time_t, fs::path> folder_set_t;
+static folder_set_t buildBackupsMapSortedByLastWrite(const std::string& strWalletFile, const fs::path& backupsDir)
 {
-    strBackupWarning = strBackupError = "";
-
-    int nWalletBackups = gArgs.GetArg("-createwalletbackups", DEFAULT_CREATEWALLETBACKUPS);
-    nWalletBackups = std::max(0, std::min(10, nWalletBackups));
-
-    if (nWalletBackups == 0) {
-        LogPrintf("Automatic wallet backups are disabled!\n");
-        return false;
-    }
-
-    fs::path backupsDir = GetDataDir() / "backups";
-    if (!fs::exists(backupsDir)) {
-        // Always create backup folder to not confuse the operating system's file browser
-        LogPrintf("Creating backup folder %s\n", backupsDir.string());
-        if(!fs::create_directories(backupsDir)) {
-            // smth is wrong, we shouldn't continue until it's resolved
-            strBackupError = strprintf(_("Wasn't able to create wallet backup folder %s!"), backupsDir.string());
-            LogPrintf("%s\n", strBackupError);
-            nWalletBackups = -1;
-            return false;
-        }
-    }
-    // Create backup of the ...
-    std::string dateTimeStr = FormatISO8601DateTimeForBackup(GetTime());
-
-    // ... strWalletFile file
-    fs::path sourceFile = GetDataDir() / strWalletFile;
-    fs::path backupFile = backupsDir / (strWalletFile + dateTimeStr);
-    sourceFile.make_preferred();
-    backupFile.make_preferred();
-    if (fs::exists(backupFile)) {
-        strBackupWarning = _("Failed to create backup, file already exists! This could happen if you restarted wallet in less than 60 seconds. You can continue if you are ok with this.");
-        LogPrintf("%s\n", strBackupWarning);
-        return false;
-    }
-    if(fs::exists(sourceFile)) {
-#if BOOST_VERSION >= 105800
-        try {
-            fs::copy_file(sourceFile, backupFile);
-            LogPrintf("Creating backup of %s -> %s\n", sourceFile.string(), backupFile.string());
-        } catch(fs::filesystem_error &error) {
-            strBackupWarning = strprintf(_("Failed to create backup, error: %s"), error.what());
-            LogPrintf("%s\n", strBackupWarning);
-            nWalletBackups = -1;
-            return false;
-        }
-#else
-        std::ifstream src(sourceFile.string(), std::ios::binary);
-        std::ofstream dst(backupFile.string(), std::ios::binary);
-        dst << src.rdbuf();
-#endif
-    }
-
-    // Keep only the last 10 backups, including the new one of course
-    typedef std::multimap<std::time_t, fs::path> folder_set_t;
     folder_set_t folder_set;
     fs::directory_iterator end_iter;
-    backupsDir.make_preferred();
     // Build map of backup files for current(!) wallet sorted by last write time
-    fs::path currentFile;
     for (fs::directory_iterator dir_iter(backupsDir); dir_iter != end_iter; ++dir_iter) {
         // Only check regular files
-        if ( fs::is_regular_file(dir_iter->status())) {
-            currentFile = dir_iter->path().filename();
+        if (fs::is_regular_file(dir_iter->status())) {
             // Only add the backups for the current wallet, e.g. wallet.dat.*
             if(dir_iter->path().stem().string() == strWalletFile) {
                 folder_set.insert(folder_set_t::value_type(fs::last_write_time(dir_iter->path()), *dir_iter));
             }
         }
     }
+    return folder_set;
+}
+
+static bool cleanWalletBackups(folder_set_t& folder_set, int nWalletBackups, std::string& strBackupWarning)
+{
     // Loop backward through backup files and keep the N newest ones (1 <= N <= 10)
     int counter = 0;
-    for (std::pair<const std::time_t, fs::path> file : reverse_iterate(folder_set)) {
+    for (const std::pair<const std::time_t, fs::path>& file : reverse_iterate(folder_set)) {
         counter++;
         if (counter > nWalletBackups) {
             // More than nWalletBackups backups: delete oldest one(s)
             try {
                 fs::remove(file.second);
                 LogPrintf("Old backup deleted: %s\n", file.second);
-            } catch(fs::filesystem_error &error) {
+            } catch (fs::filesystem_error &error) {
                 strBackupWarning = strprintf(_("Failed to delete backup, error: %s"), error.what());
                 LogPrintf("%s\n", strBackupWarning);
                 return false;
@@ -946,59 +894,66 @@ bool AutoBackupWallet(const std::string& strWalletFile, std::string& strBackupWa
     return true;
 }
 
+bool AutoBackupWallet(const CWallet& wallet, std::string& strBackupWarning, std::string& strBackupError)
+{
+    std::string strWalletFile = wallet.GetDBHandle().GetName();
+
+    strBackupWarning = strBackupError = "";
+    int nWalletBackups = std::max(0, std::min(10, (int)gArgs.GetArg("-createwalletbackups", DEFAULT_CREATEWALLETBACKUPS)));
+    if (nWalletBackups == 0) {
+        LogPrintf("Automatic wallet backups are disabled!\n");
+        return false;
+    }
+
+    fs::path backupsDir = GetDataDir() / "backups";
+    backupsDir.make_preferred();
+    TryCreateDirectories(backupsDir);
+
+    // Copy wallet file
+    fs::path sourceFile = GetDataDir() / strWalletFile;
+    fs::path backupFile = backupsDir / (strWalletFile + FormatISO8601DateTimeForBackup(GetTime()));
+    sourceFile.make_preferred();
+    backupFile.make_preferred();
+    if (fs::exists(backupFile)) {
+        LogPrintf("%s\n", _("Failed to create backup, file already exists! This could happen if you restarted wallet in less than 60 seconds. You can continue if you are ok with this."));
+        return false;
+    }
+
+    if (!fs::exists(sourceFile)) {
+        strBackupWarning = _("Failed to create backup, wallet file not found");
+        LogPrintf("%s\n", strBackupWarning);
+        return false;
+    }
+
+    // Try to backup
+    if (!AttemptBackupWallet(&wallet, sourceFile, backupFile)) {
+        return false; // error is logged internally
+    }
+
+    // Keep only 0 < nWalletBackups <= 10 backups, including the new one of course
+    folder_set_t folder_set = buildBackupsMapSortedByLastWrite(strWalletFile, backupsDir);
+    return cleanWalletBackups(folder_set, nWalletBackups, strBackupWarning);
+}
+
 void MultiBackup(const CWallet& wallet, fs::path pathCustom, fs::path pathWithFile, const fs::path& pathSrc)
 {
     int nThreshold = gArgs.GetArg("-custombackupthreshold", DEFAULT_CUSTOMBACKUPTHRESHOLD);
     if (nThreshold > 0) {
-
-        typedef std::multimap<std::time_t, fs::path> folder_set_t;
-        folder_set_t folderSet;
-        fs::directory_iterator end_iter;
-
+        std::string strBackupWarning;
         pathCustom.make_preferred();
-        // Build map of backup files for current(!) wallet sorted by last write time
-
-        fs::path currentFile;
-        for (fs::directory_iterator dir_iter(pathCustom); dir_iter != end_iter; ++dir_iter) {
-            // Only check regular files
-            if (fs::is_regular_file(dir_iter->status())) {
-                currentFile = dir_iter->path().filename();
-                // Only add the backups for the current wallet, e.g. wallet.dat.*
-                if (dir_iter->path().stem().string() == wallet.GetDBHandle().GetName()) {
-                    folderSet.insert(folder_set_t::value_type(fs::last_write_time(dir_iter->path()), *dir_iter));
-                }
-            }
+        folder_set_t folderSet = buildBackupsMapSortedByLastWrite(wallet.GetDBHandle().GetName(), pathCustom);
+        if (!cleanWalletBackups(folderSet, nThreshold, strBackupWarning)) {
+            NotifyBacked(wallet, false, strBackupWarning);
         }
 
-        int counter = 0; //TODO: add seconds to avoid naming conflicts
-        for (auto entry : folderSet) {
-            counter++;
+        // TODO: add seconds to avoid naming conflicts
+        for (const auto& entry : folderSet) {
             if(entry.second == pathWithFile) {
                 pathWithFile += "(1)";
             }
         }
-
-        if (counter >= nThreshold) {
-            std::time_t oldestBackup = 0;
-            for(auto entry : folderSet) {
-                if(oldestBackup == 0 || entry.first < oldestBackup) {
-                    oldestBackup = entry.first;
-                }
-            }
-
-            try {
-                auto entry = folderSet.find(oldestBackup);
-                if (entry != folderSet.end()) {
-                    fs::remove(entry->second);
-                    LogPrintf("Old backup deleted: %s\n", (*entry).second);
-                }
-            } catch (const fs::filesystem_error& error) {
-                std::string strMessage = strprintf("Failed to delete backup %s\n", error.what());
-                NotifyBacked(wallet, false, strMessage);
-            }
-        }
     }
-    AttemptBackupWallet(wallet, pathSrc.string(), pathWithFile.string());
+    AttemptBackupWallet(&wallet, pathSrc.string(), pathWithFile.string());
 }
 
 bool BackupWallet(const CWallet& wallet, const fs::path& strDest)
@@ -1024,7 +979,7 @@ bool BackupWallet(const CWallet& wallet, const fs::path& strDest)
                     if(!exists(pathDest)) create_directory(pathDest);
                     pathDest /= strFile;
                 }
-                bool defaultPath = AttemptBackupWallet(wallet, pathSrc.string(), pathDest.string());
+                bool defaultPath = AttemptBackupWallet(&wallet, pathSrc.string(), pathDest.string());
 
                 if(defaultPath && !pathCustom.empty()) {
                     MultiBackup(wallet, pathCustom, pathWithFile, pathSrc);
@@ -1038,7 +993,7 @@ bool BackupWallet(const CWallet& wallet, const fs::path& strDest)
     return false;
 }
 
-bool AttemptBackupWallet(const CWallet& wallet, const fs::path& pathSrc, const fs::path& pathDest)
+bool AttemptBackupWallet(const CWallet* wallet, const fs::path& pathSrc, const fs::path& pathDest)
 {
     bool retStatus;
     std::string strMessage;
@@ -1059,15 +1014,15 @@ bool AttemptBackupWallet(const CWallet& wallet, const fs::path& pathSrc, const f
         src.close();
         dst.close();
 #endif
-        strMessage = strprintf("copied %s to %s\n", wallet.GetDBHandle().GetName(), pathDest.string());
+        strMessage = strprintf("copied %s to %s\n", pathSrc.string(), pathDest.string());
         LogPrintf("%s : %s\n", __func__, strMessage);
         retStatus = true;
     } catch (const fs::filesystem_error& e) {
         retStatus = false;
         strMessage = strprintf("%s\n", e.what());
         LogPrintf("%s : %s\n", __func__, strMessage);
     }
-    NotifyBacked(wallet, retStatus, strMessage);
+    if (wallet) NotifyBacked(*wallet, retStatus, strMessage);
     return retStatus;
 }
 

src/wallet/walletdb.h

@@ -239,10 +239,12 @@ class CWalletDB
 
 void NotifyBacked(const CWallet& wallet, bool fSuccess, std::string strMessage);
 bool BackupWallet(const CWallet& wallet, const fs::path& strDest);
-bool AttemptBackupWallet(const CWallet& wallet, const fs::path& pathSrc, const fs::path& pathDest);
+// If wallet is null, the NotifyBacked signal will not be broadcasted.
+// todo: move NotifyBacked() signal to the caller side and/or decouple it from here in another function
+bool AttemptBackupWallet(const CWallet* wallet, const fs::path& pathSrc, const fs::path& pathDest);
 
 //! Called during init: Automatic backups of wallet not running (just copying and renaming dat file)
-bool AutoBackupWallet(const std::string& strWalletFile, std::string& strBackupWarning, std::string& strBackupError);
+bool AutoBackupWallet(const CWallet& wallet, std::string& strBackupWarning, std::string& strBackupError);
 
 //! Compacts BDB state so that wallet.dat is self-contained (if there are changes)
 void MaybeCompactWalletDB();