From c6599992f0221112907d264b97a3478717b85692 Mon Sep 17 00:00:00 2001 From: pasta Date: Sat, 22 Nov 2025 18:53:50 -0600 Subject: [PATCH 01/12] feat: implement exponential retention for wallet backups Enhances the wallet backup system to retain more historical backups while managing disk space efficiently. Instead of keeping only the N most recent backups, the system now keeps the N most recent plus exponentially-spaced older backups (16th, 32nd, 64th oldest, etc.). - Add `-maxwalletbackups` parameter for hard limit on total backups - Replace magic numbers with named constants (DEFAULT_MAX_BACKUPS=30, DEFAULT_N_WALLET_BACKUPS=10, MAX_N_WALLET_BACKUPS=20) - Extract backup deletion logic into GetBackupsToDelete() function - Add comprehensive unit tests in wallet/test/backup_tests.cpp - Update help text to clarify exponential spacing behavior --- src/Makefile.test.include | 1 + src/wallet/init.cpp | 3 +- src/wallet/wallet.cpp | 88 +++++++++++++++++++++++++++++++-------- src/wallet/wallet.h | 15 +++++++ 4 files changed, 89 insertions(+), 18 deletions(-) diff --git a/src/Makefile.test.include b/src/Makefile.test.include index dd6dda7178c3..906db74e1c06 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -207,6 +207,7 @@ BITCOIN_TESTS =\ if ENABLE_WALLET BITCOIN_TESTS += \ + wallet/test/backup_tests.cpp \ wallet/test/bip39_tests.cpp \ wallet/test/coinjoin_tests.cpp \ wallet/test/psbt_wallet_tests.cpp \ diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp index 444e066b0021..87e946102176 100644 --- a/src/wallet/init.cpp +++ b/src/wallet/init.cpp @@ -59,7 +59,8 @@ void WalletInit::AddWalletOptions(ArgsManager& argsman) const { argsman.AddArg("-avoidpartialspends", strprintf("Group outputs by address, selecting many (possibly all) or none, instead of selecting on a per-output basis. Privacy is improved as addresses are mostly swept with fewer transactions and outputs are aggregated in clean change addresses. It may result in higher fees due to less optimal coin selection caused by this added limitation and possibly a larger-than-necessary number of inputs being used. Always enabled for wallets with \"avoid_reuse\" enabled, otherwise default: %u.", DEFAULT_AVOIDPARTIALSPENDS), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET); argsman.AddArg("-consolidatefeerate=", strprintf("The maximum feerate (in %s/kvB) at which transaction building may use more inputs than strictly necessary so that the wallet's UTXO pool can be reduced (default: %s).", CURRENCY_UNIT, FormatMoney(DEFAULT_CONSOLIDATE_FEERATE)), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET); - argsman.AddArg("-createwalletbackups=", strprintf("Number of automatic wallet backups (default: %u)", nWalletBackups), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET); + argsman.AddArg("-createwalletbackups=", strprintf("Number of automatic wallet backups to keep most recent (default: %u, max: %u). Older backups are kept at exponentially spaced intervals.", DEFAULT_N_WALLET_BACKUPS, MAX_N_WALLET_BACKUPS), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET); + argsman.AddArg("-maxwalletbackups=", strprintf("Maximum total number of automatic wallet backups to keep (default: %u)", DEFAULT_MAX_BACKUPS), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET); argsman.AddArg("-disablewallet", "Do not load the wallet and disable wallet RPC calls", ArgsManager::ALLOW_ANY, OptionsCategory::WALLET); #if HAVE_SYSTEM argsman.AddArg("-instantsendnotify=", "Execute command when a wallet InstantSend transaction is successfully locked. %s in cmd is replaced by TxID and %w is replaced by wallet name. %w is not currently implemented on Windows. On systems where %w is supported, it should NOT be quoted because this would break shell escaping used to invoke the command.", ArgsManager::ALLOW_ANY, OptionsCategory::WALLET); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 85adcf5ad527..ee4fba6b7a22 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -55,6 +55,10 @@ using interfaces::FoundBlock; namespace wallet { +// Static wallet backup configuration +int CWallet::nWalletBackups = DEFAULT_N_WALLET_BACKUPS; +int CWallet::nMaxWalletBackups = DEFAULT_MAX_BACKUPS; + const std::map WALLET_FLAG_CAVEATS{ {WALLET_FLAG_AVOID_REUSE, "You need to rescan the blockchain in order to correctly mark used " @@ -3349,13 +3353,65 @@ void CWallet::postInitProcess() chain().requestMempoolTransactions(*this); } +std::vector GetBackupsToDelete(const std::multimap& backups, int nWalletBackups, int maxBackups) +{ + std::vector paths_to_delete; + + if (backups.size() <= (size_t)nWalletBackups) { + return paths_to_delete; + } + + // Sort backups by time descending (newest first) + std::vector> sorted_backups; + for (const auto& entry : backups) { + sorted_backups.push_back(entry); + } + std::reverse(sorted_backups.begin(), sorted_backups.end()); + + std::vector indices_to_keep; + for (size_t i = 0; i < sorted_backups.size(); ++i) { + bool keep = false; + if (i < (size_t)nWalletBackups) { + keep = true; + } else { + // Check if (i + 1) is power of 2 + size_t rank = i + 1; + if ((rank & (rank - 1)) == 0) { + keep = true; + } + } + + if (keep) { + indices_to_keep.push_back(i); + } + } + + if (indices_to_keep.size() > (size_t)maxBackups) { + indices_to_keep.resize(maxBackups); + } + + size_t keep_idx = 0; + for (size_t i = 0; i < sorted_backups.size(); ++i) { + if (keep_idx < indices_to_keep.size() && indices_to_keep[keep_idx] == i) { + keep_idx++; + } else { + paths_to_delete.push_back(sorted_backups[i].second); + } + } + + return paths_to_delete; +} + void CWallet::InitAutoBackup() { if (gArgs.GetBoolArg("-disablewallet", DEFAULT_DISABLE_WALLET)) return; - nWalletBackups = gArgs.GetIntArg("-createwalletbackups", 10); - nWalletBackups = std::max(0, std::min(10, nWalletBackups)); + nWalletBackups = gArgs.GetIntArg("-createwalletbackups", DEFAULT_N_WALLET_BACKUPS); + nWalletBackups = std::max(0, std::min(MAX_N_WALLET_BACKUPS, nWalletBackups)); + + nMaxWalletBackups = gArgs.GetIntArg("-maxwalletbackups", DEFAULT_MAX_BACKUPS); + nMaxWalletBackups = std::max(0, nMaxWalletBackups); } bool CWallet::BackupWallet(const std::string& strDest) const @@ -3487,20 +3543,15 @@ bool CWallet::AutoBackupWallet(const fs::path& wallet_path, bilingual_str& error } } - // Loop backward through backup files and keep the N newest ones (1 <= N <= 10) - int counter{0}; - for (const auto& [entry_time, entry] : folder_set | std::views::reverse) { - counter++; - if (counter > nWalletBackups) { - // More than nWalletBackups backups: delete oldest one(s) - try { - fs::remove(entry); - WalletLogPrintf("Old backup deleted: %s\n", fs::PathToString(entry)); - } catch(fs::filesystem_error &error) { - warnings.push_back(strprintf(_("Failed to delete backup, error: %s"), fsbridge::get_filesystem_error_message(error))); - WalletLogPrintf("%s\n", Join(warnings, Untranslated("\n")).original); - return false; - } + std::vector backupsToDelete = GetBackupsToDelete(folder_set, nWalletBackups, nMaxWalletBackups); + for (const auto& path : backupsToDelete) { + try { + fs::remove(path); + WalletLogPrintf("Old backup deleted: %s\n", fs::PathToString(path)); + } catch(fs::filesystem_error &error) { + warnings.push_back(strprintf(_("Failed to delete backup, error: %s"), fsbridge::get_filesystem_error_message(error))); + WalletLogPrintf("%s\n", Join(warnings, Untranslated("\n")).original); + return false; } } @@ -3708,7 +3759,10 @@ bool CWallet::Unlock(const SecureString& strWalletPassphrase, bool fForMixingOnl if(nWalletBackups == -2) { TopUpKeyPool(); WalletLogPrintf("Keypool replenished, re-initializing automatic backups.\n"); - nWalletBackups = m_args.GetIntArg("-createwalletbackups", 10); + nWalletBackups = m_args.GetIntArg("-createwalletbackups", DEFAULT_N_WALLET_BACKUPS); + nWalletBackups = std::max(0, std::min(MAX_N_WALLET_BACKUPS, nWalletBackups)); + nMaxWalletBackups = m_args.GetIntArg("-maxwalletbackups", DEFAULT_MAX_BACKUPS); + nMaxWalletBackups = std::max(0, nMaxWalletBackups); } return true; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 8e0966d4bfae..ab007214921d 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -80,6 +80,19 @@ std::unique_ptr HandleLoadWallet(WalletContext& context, Lo void NotifyWalletLoaded(WalletContext& context, const std::shared_ptr& wallet); std::unique_ptr MakeWalletDatabase(const std::string& name, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error); +//! Wallet backup configuration defaults +static constexpr int DEFAULT_MAX_BACKUPS = 30; +static constexpr int DEFAULT_N_WALLET_BACKUPS = DEFAULT_MAX_BACKUPS / 3; +static constexpr int MAX_N_WALLET_BACKUPS = 20; + +/** + * Determine which backup files to delete based on retention policy. + * Keeps the latest nWalletBackups. + * For older backups, keeps those that are exponentially spaced (powers of 2 indices). + * Enforces a hard limit of maxBackups. + */ +std::vector GetBackupsToDelete(const std::multimap& backups, int nWalletBackups, int maxBackups = DEFAULT_MAX_BACKUPS); + //! -paytxfee default constexpr CAmount DEFAULT_PAY_TX_FEE = 0; //! -fallbackfee default @@ -498,6 +511,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati std::set setLockedCoins GUARDED_BY(cs_wallet); int64_t nKeysLeftSinceAutoBackup; + static int nWalletBackups; + static int nMaxWalletBackups; /** Registered interfaces::Chain::Notifications handler. */ std::unique_ptr m_chain_notifications_handler; From 6b7aa15e589e033f0091540d7309b1a34fa579de Mon Sep 17 00:00:00 2001 From: pasta Date: Tue, 25 Nov 2025 12:11:05 -0600 Subject: [PATCH 02/12] fix: missing file --- src/wallet/test/backup_tests.cpp | 139 +++++++++++++++++++++++++++++++ 1 file changed, 139 insertions(+) create mode 100644 src/wallet/test/backup_tests.cpp diff --git a/src/wallet/test/backup_tests.cpp b/src/wallet/test/backup_tests.cpp new file mode 100644 index 000000000000..e4bffad12c0e --- /dev/null +++ b/src/wallet/test/backup_tests.cpp @@ -0,0 +1,139 @@ +#include +#include + +#include + +#include +#include +#include + +namespace wallet { + +// Forward declaration of the function we want to test (will be implemented in wallet.cpp) +std::vector GetBackupsToDelete(const std::multimap& backups, int nWalletBackups, int maxBackups); + +BOOST_FIXTURE_TEST_SUITE(backup_tests, WalletTestingSetup) + +BOOST_AUTO_TEST_CASE(exponential_backup_logic) +{ + // Helper to create a dummy timestamp + auto make_time = [](int64_t seconds_ago) { + return fs::file_time_type(std::chrono::seconds(std::time(nullptr) - seconds_ago)); + }; + + // Helper to create a dummy path + auto make_path = [](int i) { + return fs::u8path("backup_" + std::to_string(i) + ".dat"); + }; + + std::multimap backups; + + // Case 1: Less than nWalletBackups (10) + for (int i = 0; i < 5; ++i) { + backups.insert({make_time(i * 100), make_path(i)}); + } + auto to_delete = GetBackupsToDelete(backups, 10, 50); + BOOST_CHECK(to_delete.empty()); + + // Case 2: Exactly nWalletBackups (10) + backups.clear(); + for (int i = 0; i < 10; ++i) { + backups.insert({make_time(i * 100), make_path(i)}); + } + to_delete = GetBackupsToDelete(backups, 10, 50); + BOOST_CHECK(to_delete.empty()); + + // Case 3: More than 10, all very recent + // Logic: INDEX-BASED retention (not time-based) + // Keep the latest 10 (indices 0-9 when sorted newest first) + // Then keep backups at ranks that are powers of 2: 16th, 32nd, 64th, etc. + // (i.e., indices 15, 31, 63, ... when 0-indexed) + // If we have 20 backups all created seconds apart, the algorithm doesn't care about time. + // It just keeps: indices 0-9 (latest 10), then index 15 (16th backup) + // All others are deleted. + + backups.clear(); + for (int i = 0; i < 20; ++i) { + // 20 backups, 1 second apart. + // 0 is oldest, 19 is newest. + backups.insert({make_time(200 - i * 10), make_path(i)}); + } + // Times: + // i=0: 200s ago (Oldest) + // i=19: 10s ago (Newest) + + to_delete = GetBackupsToDelete(backups, 10, 50); + // Should keep latest 10 (indices 0-9 in descending sorted order = i=10 to 19). + // Then keep index 15 (16th backup = i=4). + // i=0 to 3 and i=5 to 9 are deleted (10 total). + // Total kept: 11, deleted: 9. + BOOST_CHECK_EQUAL(to_delete.size(), 9); + + // Case 4: Index-based exponential distribution + backups.clear(); + // Create 18 backups with varied ages (not that it matters for index-based logic) + // After sorting newest first, we get indices 0-17 + // Keep: indices 0-9 (latest 10), index 15 (16th backup) + // Delete: indices 10-14, 16-17 (7 total) + + std::vector ages; + for(int i=0; i<10; ++i) ages.push_back(i * 3600); // 0 to 9 hours + ages.push_back(24 * 3600); // 1 day + ages.push_back(2 * 24 * 3600); // 2 days + ages.push_back(4 * 24 * 3600); // 4 days + ages.push_back(8 * 24 * 3600); // 8 days + ages.push_back(16 * 24 * 3600); // 16 days + ages.push_back(32 * 24 * 3600); // 32 days + ages.push_back(3 * 24 * 3600); // 3 days + ages.push_back(5 * 24 * 3600); // 5 days + + int id = 0; + for (auto age : ages) { + backups.insert({make_time(age), make_path(id++)}); + } + + to_delete = GetBackupsToDelete(backups, 10, 50); + + // Total files: 18 + // After sorting by time (newest first), we have indices 0-17 + // Keep indices: 0-9 (latest 10), 15 (16th backup, power of 2) + // Delete: 10-14, 16-17 = 7 files + BOOST_CHECK_EQUAL(to_delete.size(), 7); +} + +BOOST_AUTO_TEST_CASE(hard_max_limit) +{ + auto make_time = [](int64_t seconds_ago) { + return fs::file_time_type(std::chrono::seconds(std::time(nullptr) - seconds_ago)); + }; + auto make_path = [](int i) { + return fs::u8path("backup_" + std::to_string(i) + ".dat"); + }; + + std::multimap backups; + + // Test INDEX-BASED exponential retention: + // - Keep latest nWalletBackups (10) backups + // - For older backups, keep those at ranks that are powers of 2 (16th, 32nd, 64th, etc.) + // - Enforce hard max limit (maxBackups) + // + // With 100 backups and nWalletBackups=10: + // Keep indices: 0-9 (latest 10), 15 (16th), 31 (32nd), 63 (64th) + // Total kept: 13, deleted: 87 + + backups.clear(); + for (int i = 0; i < 100; ++i) { + backups.insert({make_time(1000 - i), make_path(i)}); + } + + // GetBackupsToDelete should return everything EXCEPT: + // Indices 0-9 (latest 10), 15 (16th), 31 (32nd), 63 (64th) + // Total kept: 13, deleted: 87 + + auto to_delete = GetBackupsToDelete(backups, 10, 50); + BOOST_CHECK_EQUAL(to_delete.size(), 87); +} + +BOOST_AUTO_TEST_SUITE_END() + +} // namespace wallet From 4e3849d28994acf569550411c08bcc52e758f5c4 Mon Sep 17 00:00:00 2001 From: pasta Date: Tue, 25 Nov 2025 12:16:40 -0600 Subject: [PATCH 03/12] fix: apply changes as suggested by coderabbit --- src/wallet/wallet.cpp | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index ee4fba6b7a22..caa6d6c34cd8 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -56,6 +56,9 @@ using interfaces::FoundBlock; namespace wallet { // Static wallet backup configuration +// Ensure compile-time invariant: nWalletBackups <= nMaxWalletBackups when nMaxWalletBackups > 0 +static_assert(DEFAULT_MAX_BACKUPS == 0 || DEFAULT_N_WALLET_BACKUPS <= DEFAULT_MAX_BACKUPS, + "DEFAULT_N_WALLET_BACKUPS must not exceed DEFAULT_MAX_BACKUPS when DEFAULT_MAX_BACKUPS > 0"); int CWallet::nWalletBackups = DEFAULT_N_WALLET_BACKUPS; int CWallet::nMaxWalletBackups = DEFAULT_MAX_BACKUPS; @@ -3357,6 +3360,11 @@ std::vector GetBackupsToDelete(const std::multimap paths_to_delete; + // Early guard: if maxBackups <= 0, don't delete any backups + if (maxBackups <= 0) { + return paths_to_delete; + } + if (backups.size() <= (size_t)nWalletBackups) { return paths_to_delete; } @@ -3412,6 +3420,11 @@ void CWallet::InitAutoBackup() nMaxWalletBackups = gArgs.GetIntArg("-maxwalletbackups", DEFAULT_MAX_BACKUPS); nMaxWalletBackups = std::max(0, nMaxWalletBackups); + + // Enforce nWalletBackups <= nMaxWalletBackups + if (nWalletBackups > nMaxWalletBackups) { + nWalletBackups = nMaxWalletBackups; + } } bool CWallet::BackupWallet(const std::string& strDest) const @@ -3763,6 +3776,11 @@ bool CWallet::Unlock(const SecureString& strWalletPassphrase, bool fForMixingOnl nWalletBackups = std::max(0, std::min(MAX_N_WALLET_BACKUPS, nWalletBackups)); nMaxWalletBackups = m_args.GetIntArg("-maxwalletbackups", DEFAULT_MAX_BACKUPS); nMaxWalletBackups = std::max(0, nMaxWalletBackups); + + // Enforce nWalletBackups <= nMaxWalletBackups + if (nWalletBackups > nMaxWalletBackups) { + nWalletBackups = nMaxWalletBackups; + } } return true; } From bd1549efd51b68378319039cff1a5fbe7f1e4035 Mon Sep 17 00:00:00 2001 From: pasta Date: Tue, 25 Nov 2025 12:25:28 -0600 Subject: [PATCH 04/12] fix: coderabbit suggestions in test --- src/wallet/test/backup_tests.cpp | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/src/wallet/test/backup_tests.cpp b/src/wallet/test/backup_tests.cpp index e4bffad12c0e..099f1d2b6980 100644 --- a/src/wallet/test/backup_tests.cpp +++ b/src/wallet/test/backup_tests.cpp @@ -18,7 +18,7 @@ BOOST_AUTO_TEST_CASE(exponential_backup_logic) { // Helper to create a dummy timestamp auto make_time = [](int64_t seconds_ago) { - return fs::file_time_type(std::chrono::seconds(std::time(nullptr) - seconds_ago)); + return fs::file_time_type{std::chrono::file_clock::time_point{std::chrono::seconds(seconds_ago)}}; }; // Helper to create a dummy path @@ -65,7 +65,7 @@ BOOST_AUTO_TEST_CASE(exponential_backup_logic) to_delete = GetBackupsToDelete(backups, 10, 50); // Should keep latest 10 (indices 0-9 in descending sorted order = i=10 to 19). // Then keep index 15 (16th backup = i=4). - // i=0 to 3 and i=5 to 9 are deleted (10 total). + // i=0 to 3 and i=5 to 9 are deleted (9 backups deleted). // Total kept: 11, deleted: 9. BOOST_CHECK_EQUAL(to_delete.size(), 9); @@ -104,7 +104,7 @@ BOOST_AUTO_TEST_CASE(exponential_backup_logic) BOOST_AUTO_TEST_CASE(hard_max_limit) { auto make_time = [](int64_t seconds_ago) { - return fs::file_time_type(std::chrono::seconds(std::time(nullptr) - seconds_ago)); + return fs::file_time_type{std::chrono::file_clock::time_point{std::chrono::seconds(seconds_ago)}}; }; auto make_path = [](int i) { return fs::u8path("backup_" + std::to_string(i) + ".dat"); @@ -134,6 +134,31 @@ BOOST_AUTO_TEST_CASE(hard_max_limit) BOOST_CHECK_EQUAL(to_delete.size(), 87); } +BOOST_AUTO_TEST_CASE(hard_max_limit_caps_retention) +{ + auto make_time = [](int64_t seconds_ago) { + return fs::file_time_type::clock::now() - std::chrono::seconds(seconds_ago); + }; + auto make_path = [](int i) { + return fs::u8path("backup_" + std::to_string(i) + ".dat"); + }; + + std::multimap backups; + + // Test that maxBackups hard cap limits retention when exponential/index-based + // retention would keep more than maxBackups. With 50 backups and nWalletBackups=10, + // exponential logic would keep indices 0-9 (latest 10), 15 (16th), 31 (32nd) = 13 backups. + // But with maxBackups=12, the hard cap should limit kept backups to 12, resulting in 38 deletions. + + backups.clear(); + for (int i = 0; i < 50; ++i) { + backups.insert({make_time(1000 - i), make_path(i)}); + } + + auto to_delete = GetBackupsToDelete(backups, 10, 12); + BOOST_CHECK_EQUAL(to_delete.size(), 38); // 50 - 12 = 38 +} + BOOST_AUTO_TEST_SUITE_END() } // namespace wallet From 39e4adb289e6a13152a773f19e45dc576733ac2d Mon Sep 17 00:00:00 2001 From: pasta Date: Tue, 25 Nov 2025 12:27:41 -0600 Subject: [PATCH 05/12] fix: whitespace --- src/wallet/test/backup_tests.cpp | 8 ++++---- src/wallet/wallet.cpp | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/wallet/test/backup_tests.cpp b/src/wallet/test/backup_tests.cpp index 099f1d2b6980..a151a3d9fa1d 100644 --- a/src/wallet/test/backup_tests.cpp +++ b/src/wallet/test/backup_tests.cpp @@ -27,7 +27,7 @@ BOOST_AUTO_TEST_CASE(exponential_backup_logic) }; std::multimap backups; - + // Case 1: Less than nWalletBackups (10) for (int i = 0; i < 5; ++i) { backups.insert({make_time(i * 100), make_path(i)}); @@ -51,12 +51,12 @@ BOOST_AUTO_TEST_CASE(exponential_backup_logic) // If we have 20 backups all created seconds apart, the algorithm doesn't care about time. // It just keeps: indices 0-9 (latest 10), then index 15 (16th backup) // All others are deleted. - + backups.clear(); for (int i = 0; i < 20; ++i) { // 20 backups, 1 second apart. // 0 is oldest, 19 is newest. - backups.insert({make_time(200 - i * 10), make_path(i)}); + backups.insert({make_time(200 - i * 10), make_path(i)}); } // Times: // i=0: 200s ago (Oldest) @@ -68,7 +68,7 @@ BOOST_AUTO_TEST_CASE(exponential_backup_logic) // i=0 to 3 and i=5 to 9 are deleted (9 backups deleted). // Total kept: 11, deleted: 9. BOOST_CHECK_EQUAL(to_delete.size(), 9); - + // Case 4: Index-based exponential distribution backups.clear(); // Create 18 backups with varied ages (not that it matters for index-based logic) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index caa6d6c34cd8..11288fa53a33 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3420,7 +3420,7 @@ void CWallet::InitAutoBackup() nMaxWalletBackups = gArgs.GetIntArg("-maxwalletbackups", DEFAULT_MAX_BACKUPS); nMaxWalletBackups = std::max(0, nMaxWalletBackups); - + // Enforce nWalletBackups <= nMaxWalletBackups if (nWalletBackups > nMaxWalletBackups) { nWalletBackups = nMaxWalletBackups; @@ -3776,7 +3776,7 @@ bool CWallet::Unlock(const SecureString& strWalletPassphrase, bool fForMixingOnl nWalletBackups = std::max(0, std::min(MAX_N_WALLET_BACKUPS, nWalletBackups)); nMaxWalletBackups = m_args.GetIntArg("-maxwalletbackups", DEFAULT_MAX_BACKUPS); nMaxWalletBackups = std::max(0, nMaxWalletBackups); - + // Enforce nWalletBackups <= nMaxWalletBackups if (nWalletBackups > nMaxWalletBackups) { nWalletBackups = nMaxWalletBackups; From 6e615d9fcea45848ff90677c1f40edb9e6081563 Mon Sep 17 00:00:00 2001 From: pasta Date: Fri, 28 Nov 2025 08:40:25 -0600 Subject: [PATCH 06/12] fix: use ToString and fix trailing whitespace --- src/wallet/test/backup_tests.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/wallet/test/backup_tests.cpp b/src/wallet/test/backup_tests.cpp index a151a3d9fa1d..1b352b940ba6 100644 --- a/src/wallet/test/backup_tests.cpp +++ b/src/wallet/test/backup_tests.cpp @@ -6,6 +6,7 @@ #include #include #include +#include namespace wallet { @@ -23,7 +24,7 @@ BOOST_AUTO_TEST_CASE(exponential_backup_logic) // Helper to create a dummy path auto make_path = [](int i) { - return fs::u8path("backup_" + std::to_string(i) + ".dat"); + return fs::u8path("backup_" + ToString(i) + ".dat"); }; std::multimap backups; @@ -61,7 +62,7 @@ BOOST_AUTO_TEST_CASE(exponential_backup_logic) // Times: // i=0: 200s ago (Oldest) // i=19: 10s ago (Newest) - + to_delete = GetBackupsToDelete(backups, 10, 50); // Should keep latest 10 (indices 0-9 in descending sorted order = i=10 to 19). // Then keep index 15 (16th backup = i=4). @@ -107,7 +108,7 @@ BOOST_AUTO_TEST_CASE(hard_max_limit) return fs::file_time_type{std::chrono::file_clock::time_point{std::chrono::seconds(seconds_ago)}}; }; auto make_path = [](int i) { - return fs::u8path("backup_" + std::to_string(i) + ".dat"); + return fs::u8path("backup_" + ToString(i) + ".dat"); }; std::multimap backups; @@ -140,7 +141,7 @@ BOOST_AUTO_TEST_CASE(hard_max_limit_caps_retention) return fs::file_time_type::clock::now() - std::chrono::seconds(seconds_ago); }; auto make_path = [](int i) { - return fs::u8path("backup_" + std::to_string(i) + ".dat"); + return fs::u8path("backup_" + ToString(i) + ".dat"); }; std::multimap backups; From 3793600c0df7d589deb399d383ab603c3881f908 Mon Sep 17 00:00:00 2001 From: pasta Date: Fri, 28 Nov 2025 08:43:39 -0600 Subject: [PATCH 07/12] fix: redundant declaration --- src/wallet/test/backup_tests.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/wallet/test/backup_tests.cpp b/src/wallet/test/backup_tests.cpp index 1b352b940ba6..387a4a356af3 100644 --- a/src/wallet/test/backup_tests.cpp +++ b/src/wallet/test/backup_tests.cpp @@ -10,9 +10,6 @@ namespace wallet { -// Forward declaration of the function we want to test (will be implemented in wallet.cpp) -std::vector GetBackupsToDelete(const std::multimap& backups, int nWalletBackups, int maxBackups); - BOOST_FIXTURE_TEST_SUITE(backup_tests, WalletTestingSetup) BOOST_AUTO_TEST_CASE(exponential_backup_logic) From 6c43f2d5c4fa85b081ad3c1650f1f22fa7d02985 Mon Sep 17 00:00:00 2001 From: pasta Date: Fri, 28 Nov 2025 14:59:21 -0600 Subject: [PATCH 08/12] fix: use rolling exponential ranges for backup retention MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous algorithm kept backups at fixed power-of-2 indices (16th, 32nd, 64th), but these positions were unreachable since backups are deleted incrementally - the 11th backup was always deleted before accumulating to 16. Changed to keep the oldest backup in each exponential range: - [nWalletBackups, 16), [16, 32), [32, 64), etc. This allows exponential retention to work from the first backup beyond nWalletBackups. With 11 backups, all 11 are now kept. With 12, index 10 is deleted while index 11 (oldest in range) is preserved. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/wallet/test/backup_tests.cpp | 71 +++++++++++++++++++++----------- src/wallet/wallet.cpp | 47 +++++++++++++-------- src/wallet/wallet.h | 4 +- 3 files changed, 78 insertions(+), 44 deletions(-) diff --git a/src/wallet/test/backup_tests.cpp b/src/wallet/test/backup_tests.cpp index 387a4a356af3..5aceb0675b38 100644 --- a/src/wallet/test/backup_tests.cpp +++ b/src/wallet/test/backup_tests.cpp @@ -41,14 +41,35 @@ BOOST_AUTO_TEST_CASE(exponential_backup_logic) to_delete = GetBackupsToDelete(backups, 10, 50); BOOST_CHECK(to_delete.empty()); + // Case 2b: 11 backups - the critical edge case that proves exponential retention works + // Before the fix, the 11th backup would always be deleted, never allowing accumulation. + // With the fix, we keep the oldest in range [10,16), so index 10 is kept. + backups.clear(); + for (int i = 0; i < 11; ++i) { + backups.insert({make_time(i * 100), make_path(i)}); + } + to_delete = GetBackupsToDelete(backups, 10, 50); + // Keep indices 0-9 (latest 10) + index 10 (oldest in [10,16)) = 11 total + // Nothing to delete! + BOOST_CHECK(to_delete.empty()); + + // Case 2c: 12 backups - now we start deleting + backups.clear(); + for (int i = 0; i < 12; ++i) { + backups.insert({make_time(i * 100), make_path(i)}); + } + to_delete = GetBackupsToDelete(backups, 10, 50); + // Keep indices 0-9 (latest 10) + index 11 (oldest in [10,16)) = 11 total + // Delete index 10 + BOOST_CHECK_EQUAL(to_delete.size(), 1); + // Case 3: More than 10, all very recent - // Logic: INDEX-BASED retention (not time-based) + // Logic: INDEX-BASED retention with exponential ranges // Keep the latest 10 (indices 0-9 when sorted newest first) - // Then keep backups at ranks that are powers of 2: 16th, 32nd, 64th, etc. - // (i.e., indices 15, 31, 63, ... when 0-indexed) - // If we have 20 backups all created seconds apart, the algorithm doesn't care about time. - // It just keeps: indices 0-9 (latest 10), then index 15 (16th backup) - // All others are deleted. + // For older backups, keep the oldest in each exponential range: + // [10,16): keep index 15 (or highest available) + // [16,32): keep index 19 (highest available, capped by size-1) + // Total kept: 12 (indices 0-9, 15, 19), deleted: 8 backups.clear(); for (int i = 0; i < 20; ++i) { @@ -61,18 +82,17 @@ BOOST_AUTO_TEST_CASE(exponential_backup_logic) // i=19: 10s ago (Newest) to_delete = GetBackupsToDelete(backups, 10, 50); - // Should keep latest 10 (indices 0-9 in descending sorted order = i=10 to 19). - // Then keep index 15 (16th backup = i=4). - // i=0 to 3 and i=5 to 9 are deleted (9 backups deleted). - // Total kept: 11, deleted: 9. - BOOST_CHECK_EQUAL(to_delete.size(), 9); + // Keep: indices 0-9 (latest 10), 15 (oldest in [10,16)), 19 (oldest in [16,32)) + // Delete: indices 10-14, 16-18 (8 backups deleted). + // Total kept: 12, deleted: 8. + BOOST_CHECK_EQUAL(to_delete.size(), 8); // Case 4: Index-based exponential distribution backups.clear(); // Create 18 backups with varied ages (not that it matters for index-based logic) // After sorting newest first, we get indices 0-17 - // Keep: indices 0-9 (latest 10), index 15 (16th backup) - // Delete: indices 10-14, 16-17 (7 total) + // Keep: indices 0-9 (latest 10), 15 (oldest in [10,16)), 17 (oldest in [16,32)) + // Delete: indices 10-14, 16 (6 total) std::vector ages; for(int i=0; i<10; ++i) ages.push_back(i * 3600); // 0 to 9 hours @@ -94,9 +114,9 @@ BOOST_AUTO_TEST_CASE(exponential_backup_logic) // Total files: 18 // After sorting by time (newest first), we have indices 0-17 - // Keep indices: 0-9 (latest 10), 15 (16th backup, power of 2) - // Delete: 10-14, 16-17 = 7 files - BOOST_CHECK_EQUAL(to_delete.size(), 7); + // Keep indices: 0-9 (latest 10), 15 (oldest in [10,16)), 17 (oldest in [16,32)) + // Delete: 10-14, 16 = 6 files + BOOST_CHECK_EQUAL(to_delete.size(), 6); } BOOST_AUTO_TEST_CASE(hard_max_limit) @@ -110,14 +130,15 @@ BOOST_AUTO_TEST_CASE(hard_max_limit) std::multimap backups; - // Test INDEX-BASED exponential retention: + // Test INDEX-BASED exponential retention with ranges: // - Keep latest nWalletBackups (10) backups - // - For older backups, keep those at ranks that are powers of 2 (16th, 32nd, 64th, etc.) + // - For older backups, keep oldest in each exponential range // - Enforce hard max limit (maxBackups) // // With 100 backups and nWalletBackups=10: - // Keep indices: 0-9 (latest 10), 15 (16th), 31 (32nd), 63 (64th) - // Total kept: 13, deleted: 87 + // Keep indices: 0-9 (latest 10), 15 (oldest in [10,16)), 31 (oldest in [16,32)), + // 63 (oldest in [32,64)), 99 (oldest in [64,128)) + // Total kept: 14, deleted: 86 backups.clear(); for (int i = 0; i < 100; ++i) { @@ -125,17 +146,17 @@ BOOST_AUTO_TEST_CASE(hard_max_limit) } // GetBackupsToDelete should return everything EXCEPT: - // Indices 0-9 (latest 10), 15 (16th), 31 (32nd), 63 (64th) - // Total kept: 13, deleted: 87 + // Indices 0-9, 15, 31, 63, 99 + // Total kept: 14, deleted: 86 auto to_delete = GetBackupsToDelete(backups, 10, 50); - BOOST_CHECK_EQUAL(to_delete.size(), 87); + BOOST_CHECK_EQUAL(to_delete.size(), 86); } BOOST_AUTO_TEST_CASE(hard_max_limit_caps_retention) { auto make_time = [](int64_t seconds_ago) { - return fs::file_time_type::clock::now() - std::chrono::seconds(seconds_ago); + return fs::file_time_type{std::chrono::file_clock::time_point{std::chrono::seconds(seconds_ago)}}; }; auto make_path = [](int i) { return fs::u8path("backup_" + ToString(i) + ".dat"); @@ -145,7 +166,7 @@ BOOST_AUTO_TEST_CASE(hard_max_limit_caps_retention) // Test that maxBackups hard cap limits retention when exponential/index-based // retention would keep more than maxBackups. With 50 backups and nWalletBackups=10, - // exponential logic would keep indices 0-9 (latest 10), 15 (16th), 31 (32nd) = 13 backups. + // exponential logic would keep indices 0-9 (latest 10), 15, 31, 49 = 13 backups. // But with maxBackups=12, the hard cap should limit kept backups to 12, resulting in 38 deletions. backups.clear(); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 11288fa53a33..63cb6696d4da 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3370,34 +3370,45 @@ std::vector GetBackupsToDelete(const std::multimap> sorted_backups; - for (const auto& entry : backups) { - sorted_backups.push_back(entry); - } - std::reverse(sorted_backups.begin(), sorted_backups.end()); + std::vector> sorted_backups(backups.rbegin(), backups.rend()); std::vector indices_to_keep; - for (size_t i = 0; i < sorted_backups.size(); ++i) { - bool keep = false; - if (i < (size_t)nWalletBackups) { - keep = true; - } else { - // Check if (i + 1) is power of 2 - size_t rank = i + 1; - if ((rank & (rank - 1)) == 0) { - keep = true; - } - } - if (keep) { - indices_to_keep.push_back(i); + // Keep latest nWalletBackups + for (size_t i = 0; i < sorted_backups.size() && i < (size_t)nWalletBackups; ++i) { + indices_to_keep.push_back(i); + } + + // For exponential ranges, keep the oldest backup in each range [2^k, 2^(k+1)) + // Start from nWalletBackups (e.g., 10) up to the next power of 2 (16) + // Then 16-32, 32-64, etc. + size_t range_start = nWalletBackups; + size_t range_end = 16; + + // Find first power of 2 >= nWalletBackups + while (range_end < (size_t)nWalletBackups) { + range_end *= 2; + } + + while (range_start < sorted_backups.size()) { + // Keep the oldest backup in range [range_start, range_end) + // That's the highest valid index in the range + size_t oldest_in_range = std::min(range_end - 1, sorted_backups.size() - 1); + if (oldest_in_range >= range_start) { + indices_to_keep.push_back(oldest_in_range); } + + range_start = range_end; + range_end *= 2; } + // Enforce hard max limit if (indices_to_keep.size() > (size_t)maxBackups) { indices_to_keep.resize(maxBackups); } + // Build deletion list + std::sort(indices_to_keep.begin(), indices_to_keep.end()); size_t keep_idx = 0; for (size_t i = 0; i < sorted_backups.size(); ++i) { if (keep_idx < indices_to_keep.size() && indices_to_keep[keep_idx] == i) { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index ab007214921d..c3b54767657c 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -88,7 +88,9 @@ static constexpr int MAX_N_WALLET_BACKUPS = 20; /** * Determine which backup files to delete based on retention policy. * Keeps the latest nWalletBackups. - * For older backups, keeps those that are exponentially spaced (powers of 2 indices). + * For older backups, keeps the oldest backup in each exponential range: + * [nWalletBackups, 16), [16, 32), [32, 64), etc. + * This allows exponential retention to work from the first backup beyond nWalletBackups. * Enforces a hard limit of maxBackups. */ std::vector GetBackupsToDelete(const std::multimap& backups, int nWalletBackups, int maxBackups = DEFAULT_MAX_BACKUPS); From 1e40610881b41a80abb94ca587419d6b982dae6a Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Sat, 29 Nov 2025 19:20:55 +0300 Subject: [PATCH 09/12] fix: use time-based exponential retention for wallet backups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous index-based algorithm was fundamentally flawed - it would delete backups down to 11 total and never accumulate beyond that, making exponential retention impossible. The new time-based algorithm: - Keeps latest N backups by count (regardless of age) - For older backups, keeps one per exponential day range: [1,2), [2,4), [4,8), [8,16), [16,32), [32,64), etc. - Processes ranges until maxBackups limit is reached - Backups naturally accumulate over time with exponential spacing - Works correctly with irregular backup schedules and long gaps 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/wallet/wallet.cpp | 84 ++++++++++++++++++++++++++----------------- src/wallet/wallet.h | 8 ++--- 2 files changed, 55 insertions(+), 37 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 63cb6696d4da..7c2b750a1f89 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3360,7 +3360,7 @@ std::vector GetBackupsToDelete(const std::multimap paths_to_delete; - // Early guard: if maxBackups <= 0, don't delete any backups + // Early guards if (maxBackups <= 0) { return paths_to_delete; } @@ -3369,51 +3369,69 @@ std::vector GetBackupsToDelete(const std::multimap> sorted_backups(backups.rbegin(), backups.rend()); - std::vector indices_to_keep; + std::set indices_to_keep; - // Keep latest nWalletBackups - for (size_t i = 0; i < sorted_backups.size() && i < (size_t)nWalletBackups; ++i) { - indices_to_keep.push_back(i); + // Always keep the latest nWalletBackups (by count, not time) + for (size_t i = 0; i < std::min(sorted_backups.size(), (size_t)nWalletBackups); ++i) { + indices_to_keep.insert(i); } - // For exponential ranges, keep the oldest backup in each range [2^k, 2^(k+1)) - // Start from nWalletBackups (e.g., 10) up to the next power of 2 (16) - // Then 16-32, 32-64, etc. - size_t range_start = nWalletBackups; - size_t range_end = 16; + // For older backups (beyond the latest nWalletBackups), apply time-based exponential retention + if (sorted_backups.size() > (size_t)nWalletBackups) { + auto now = sorted_backups[0].first; // newest backup time - // Find first power of 2 >= nWalletBackups - while (range_end < (size_t)nWalletBackups) { - range_end *= 2; - } + using days = std::chrono::duration>; - while (range_start < sorted_backups.size()) { - // Keep the oldest backup in range [range_start, range_end) - // That's the highest valid index in the range - size_t oldest_in_range = std::min(range_end - 1, sorted_backups.size() - 1); - if (oldest_in_range >= range_start) { - indices_to_keep.push_back(oldest_in_range); - } + // Define exponential day ranges: [1,2), [2,4), [4,8), [8,16), [16,32), [32,64), etc. + // We start from 1 day because the latest nWalletBackups are already kept + int range_start_days = 1; + int range_end_days = 2; - range_start = range_end; - range_end *= 2; - } + // Process exponential time ranges until we hit maxBackups limit or run out of old backups + while (indices_to_keep.size() < (size_t)maxBackups) { + std::optional oldest_in_range; + + // Search through backups beyond the latest nWalletBackups + for (size_t i = nWalletBackups; i < sorted_backups.size(); ++i) { + auto age_duration = now - sorted_backups[i].first; + auto age_days = std::chrono::duration_cast(age_duration).count(); + + if (age_days >= range_start_days && age_days < range_end_days) { + // Keep searching - we want the oldest (highest index) in this range + oldest_in_range = i; + } + } + + if (oldest_in_range.has_value()) { + indices_to_keep.insert(*oldest_in_range); + } else { + // No backups in this range, check if there are any older backups left + bool has_older_backups = false; + for (size_t i = nWalletBackups; i < sorted_backups.size(); ++i) { + auto age_duration = now - sorted_backups[i].first; + auto age_days = std::chrono::duration_cast(age_duration).count(); + if (age_days >= range_end_days) { + has_older_backups = true; + break; + } + } + if (!has_older_backups) { + break; // No more old backups to consider + } + } - // Enforce hard max limit - if (indices_to_keep.size() > (size_t)maxBackups) { - indices_to_keep.resize(maxBackups); + // Move to next exponential range + range_start_days = range_end_days; + range_end_days *= 2; + } } // Build deletion list - std::sort(indices_to_keep.begin(), indices_to_keep.end()); - size_t keep_idx = 0; for (size_t i = 0; i < sorted_backups.size(); ++i) { - if (keep_idx < indices_to_keep.size() && indices_to_keep[keep_idx] == i) { - keep_idx++; - } else { + if (indices_to_keep.find(i) == indices_to_keep.end()) { paths_to_delete.push_back(sorted_backups[i].second); } } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index c3b54767657c..728779fd4092 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -87,10 +87,10 @@ static constexpr int MAX_N_WALLET_BACKUPS = 20; /** * Determine which backup files to delete based on retention policy. - * Keeps the latest nWalletBackups. - * For older backups, keeps the oldest backup in each exponential range: - * [nWalletBackups, 16), [16, 32), [32, 64), etc. - * This allows exponential retention to work from the first backup beyond nWalletBackups. + * Keeps the latest nWalletBackups by count. + * For older backups, keeps the oldest backup in each exponential time range (in days): + * [1,2), [2,4), [4,8), [8,16), [16,32), [32,64), etc. + * This allows backups to accumulate naturally over time with exponential spacing. * Enforces a hard limit of maxBackups. */ std::vector GetBackupsToDelete(const std::multimap& backups, int nWalletBackups, int maxBackups = DEFAULT_MAX_BACKUPS); From 2ca3842a7f2bee54b623c38f6b9fdc26a4337070 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Sat, 29 Nov 2025 19:27:07 +0300 Subject: [PATCH 10/12] test: update backup tests for time-based exponential retention MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rewrote all backup retention tests to use time-based (days) instead of index-based logic. New tests cover: - Basic retention with < nWalletBackups - Time-based exponential ranges [1,2), [2,4), [4,8), etc. - Hard maxBackups limit enforcement - Irregular backup schedules (multiple per day, gaps) - Long inactivity periods 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/wallet/test/backup_tests.cpp | 311 +++++++++++++++++++++---------- 1 file changed, 215 insertions(+), 96 deletions(-) diff --git a/src/wallet/test/backup_tests.cpp b/src/wallet/test/backup_tests.cpp index 5aceb0675b38..8ea85d3fd6cc 100644 --- a/src/wallet/test/backup_tests.cpp +++ b/src/wallet/test/backup_tests.cpp @@ -12,11 +12,15 @@ namespace wallet { BOOST_FIXTURE_TEST_SUITE(backup_tests, WalletTestingSetup) -BOOST_AUTO_TEST_CASE(exponential_backup_logic) +BOOST_AUTO_TEST_CASE(time_based_exponential_retention) { - // Helper to create a dummy timestamp - auto make_time = [](int64_t seconds_ago) { - return fs::file_time_type{std::chrono::file_clock::time_point{std::chrono::seconds(seconds_ago)}}; + // Capture "now" once so all timestamps are relative to the same point + auto now = std::chrono::file_clock::now(); + + // Helper to create a timestamp N days ago from the captured "now" + auto make_time = [&now](int64_t days_ago) { + auto days_duration = std::chrono::duration>(days_ago); + return fs::file_time_type{now - days_duration}; }; // Helper to create a dummy path @@ -28,7 +32,7 @@ BOOST_AUTO_TEST_CASE(exponential_backup_logic) // Case 1: Less than nWalletBackups (10) for (int i = 0; i < 5; ++i) { - backups.insert({make_time(i * 100), make_path(i)}); + backups.insert({make_time(i), make_path(i)}); } auto to_delete = GetBackupsToDelete(backups, 10, 50); BOOST_CHECK(to_delete.empty()); @@ -36,93 +40,109 @@ BOOST_AUTO_TEST_CASE(exponential_backup_logic) // Case 2: Exactly nWalletBackups (10) backups.clear(); for (int i = 0; i < 10; ++i) { - backups.insert({make_time(i * 100), make_path(i)}); + backups.insert({make_time(i), make_path(i)}); } to_delete = GetBackupsToDelete(backups, 10, 50); BOOST_CHECK(to_delete.empty()); - // Case 2b: 11 backups - the critical edge case that proves exponential retention works - // Before the fix, the 11th backup would always be deleted, never allowing accumulation. - // With the fix, we keep the oldest in range [10,16), so index 10 is kept. + // Case 3: 11 backups - all recent (< 1 day old) + // Since all are < 1 day old, no time-based retention applies + // Keep latest 10, but the 11th is also < 1 day so it doesn't get kept backups.clear(); for (int i = 0; i < 11; ++i) { - backups.insert({make_time(i * 100), make_path(i)}); + backups.insert({make_time(0), make_path(i)}); } to_delete = GetBackupsToDelete(backups, 10, 50); - // Keep indices 0-9 (latest 10) + index 10 (oldest in [10,16)) = 11 total - // Nothing to delete! - BOOST_CHECK(to_delete.empty()); - - // Case 2c: 12 backups - now we start deleting - backups.clear(); - for (int i = 0; i < 12; ++i) { - backups.insert({make_time(i * 100), make_path(i)}); - } - to_delete = GetBackupsToDelete(backups, 10, 50); - // Keep indices 0-9 (latest 10) + index 11 (oldest in [10,16)) = 11 total - // Delete index 10 + // All backups are 0 days old, so none fall into [1,2) or later ranges + // Keep only latest 10, delete 1 BOOST_CHECK_EQUAL(to_delete.size(), 1); - // Case 3: More than 10, all very recent - // Logic: INDEX-BASED retention with exponential ranges - // Keep the latest 10 (indices 0-9 when sorted newest first) - // For older backups, keep the oldest in each exponential range: - // [10,16): keep index 15 (or highest available) - // [16,32): keep index 19 (highest available, capped by size-1) - // Total kept: 12 (indices 0-9, 15, 19), deleted: 8 - + // Case 4: 20 backups spanning multiple days + // Latest 10: 0 days old + // Older backups: 1, 2, 3, 5, 7, 10, 15, 20, 25, 30 days old backups.clear(); - for (int i = 0; i < 20; ++i) { - // 20 backups, 1 second apart. - // 0 is oldest, 19 is newest. - backups.insert({make_time(200 - i * 10), make_path(i)}); + for (int i = 0; i < 10; ++i) { + backups.insert({make_time(0), make_path(i)}); } - // Times: - // i=0: 200s ago (Oldest) - // i=19: 10s ago (Newest) + backups.insert({make_time(1), make_path(10)}); // [1,2) days + backups.insert({make_time(2), make_path(11)}); // [2,4) days + backups.insert({make_time(3), make_path(12)}); // [2,4) days + backups.insert({make_time(5), make_path(13)}); // [4,8) days + backups.insert({make_time(7), make_path(14)}); // [4,8) days + backups.insert({make_time(10), make_path(15)}); // [8,16) days + backups.insert({make_time(15), make_path(16)}); // [8,16) days + backups.insert({make_time(20), make_path(17)}); // [16,32) days + backups.insert({make_time(25), make_path(18)}); // [16,32) days + backups.insert({make_time(30), make_path(19)}); // [16,32) days to_delete = GetBackupsToDelete(backups, 10, 50); - // Keep: indices 0-9 (latest 10), 15 (oldest in [10,16)), 19 (oldest in [16,32)) - // Delete: indices 10-14, 16-18 (8 backups deleted). - // Total kept: 12, deleted: 8. - BOOST_CHECK_EQUAL(to_delete.size(), 8); - // Case 4: Index-based exponential distribution + // Should keep: + // - Latest 10 (by count, indices 0-9): backup_0 through backup_9 + // - Oldest in [1,2): backup_10 (1 day) + // - Oldest in [2,4): backup_12 (3 days) + // - Oldest in [4,8): backup_14 (7 days) + // - Oldest in [8,16): backup_16 (15 days) + // - Oldest in [16,32): backup_19 (30 days) + // Total: 15 kept, 5 deleted + BOOST_CHECK_EQUAL(to_delete.size(), 5); + + // Verify exactly which backups are deleted + std::set expected_deletions = { + "backup_11.dat", // 2 days, not oldest in [2,4) + "backup_13.dat", // 5 days, not oldest in [4,8) + "backup_15.dat", // 10 days, not oldest in [8,16) + "backup_17.dat", // 20 days, not oldest in [16,32) + "backup_18.dat" // 25 days, not oldest in [16,32) + }; + std::set actual_deletions; + for (const auto& path : to_delete) { + actual_deletions.insert(fs::PathToString(path.filename())); + } + BOOST_CHECK(expected_deletions == actual_deletions); + + // Case 5: Test that we accumulate over time + // Simulate 100 days of daily backups backups.clear(); - // Create 18 backups with varied ages (not that it matters for index-based logic) - // After sorting newest first, we get indices 0-17 - // Keep: indices 0-9 (latest 10), 15 (oldest in [10,16)), 17 (oldest in [16,32)) - // Delete: indices 10-14, 16 (6 total) - - std::vector ages; - for(int i=0; i<10; ++i) ages.push_back(i * 3600); // 0 to 9 hours - ages.push_back(24 * 3600); // 1 day - ages.push_back(2 * 24 * 3600); // 2 days - ages.push_back(4 * 24 * 3600); // 4 days - ages.push_back(8 * 24 * 3600); // 8 days - ages.push_back(16 * 24 * 3600); // 16 days - ages.push_back(32 * 24 * 3600); // 32 days - ages.push_back(3 * 24 * 3600); // 3 days - ages.push_back(5 * 24 * 3600); // 5 days - - int id = 0; - for (auto age : ages) { - backups.insert({make_time(age), make_path(id++)}); + for (int i = 0; i < 100; ++i) { + backups.insert({make_time(i), make_path(i)}); } to_delete = GetBackupsToDelete(backups, 10, 50); - // Total files: 18 - // After sorting by time (newest first), we have indices 0-17 - // Keep indices: 0-9 (latest 10), 15 (oldest in [10,16)), 17 (oldest in [16,32)) - // Delete: 10-14, 16 = 6 files - BOOST_CHECK_EQUAL(to_delete.size(), 6); + // Should keep: + // - Latest 10 (by count): backup_0 through backup_9 + // - Oldest in [1,2): backup_1 is 1 day (already in latest 10) + // - Oldest in [2,4): backup_3 is 3 days (already in latest 10) + // - Oldest in [4,8): backup_7 is 7 days (already in latest 10) + // - Oldest in [8,16): backup_15 (15 days) + // - Oldest in [16,32): backup_31 (31 days) + // - Oldest in [32,64): backup_63 (63 days) + // - Oldest in [64,128): backup_99 (99 days) + // Total: 14 kept, 86 deleted + BOOST_CHECK_EQUAL(to_delete.size(), 86); + + // Verify specific kept backups in exponential ranges + std::set expected_kept = { + "backup_0.dat", "backup_1.dat", "backup_2.dat", "backup_3.dat", "backup_4.dat", + "backup_5.dat", "backup_6.dat", "backup_7.dat", "backup_8.dat", "backup_9.dat", + "backup_15.dat", "backup_31.dat", "backup_63.dat", "backup_99.dat" + }; + std::set actual_kept; + for (const auto& [time, path] : backups) { + if (std::find(to_delete.begin(), to_delete.end(), path) == to_delete.end()) { + actual_kept.insert(fs::PathToString(path.filename())); + } + } + BOOST_CHECK(expected_kept == actual_kept); } BOOST_AUTO_TEST_CASE(hard_max_limit) { - auto make_time = [](int64_t seconds_ago) { - return fs::file_time_type{std::chrono::file_clock::time_point{std::chrono::seconds(seconds_ago)}}; + auto now = std::chrono::file_clock::now(); + auto make_time = [&now](int64_t days_ago) { + auto days_duration = std::chrono::duration>(days_ago); + return fs::file_time_type{now - days_duration}; }; auto make_path = [](int i) { return fs::u8path("backup_" + ToString(i) + ".dat"); @@ -130,33 +150,59 @@ BOOST_AUTO_TEST_CASE(hard_max_limit) std::multimap backups; - // Test INDEX-BASED exponential retention with ranges: - // - Keep latest nWalletBackups (10) backups - // - For older backups, keep oldest in each exponential range - // - Enforce hard max limit (maxBackups) - // - // With 100 backups and nWalletBackups=10: - // Keep indices: 0-9 (latest 10), 15 (oldest in [10,16)), 31 (oldest in [16,32)), - // 63 (oldest in [32,64)), 99 (oldest in [64,128)) - // Total kept: 14, deleted: 86 - - backups.clear(); + // Create 100 daily backups and set maxBackups=15 for (int i = 0; i < 100; ++i) { - backups.insert({make_time(1000 - i), make_path(i)}); + backups.insert({make_time(i), make_path(i)}); } - // GetBackupsToDelete should return everything EXCEPT: - // Indices 0-9, 15, 31, 63, 99 - // Total kept: 14, deleted: 86 + auto to_delete = GetBackupsToDelete(backups, 10, 15); - auto to_delete = GetBackupsToDelete(backups, 10, 50); + // Without maxBackups limit, we'd keep 14 backups (see Case 5 above) + // With maxBackups=15, we still keep 14 (under the limit) BOOST_CHECK_EQUAL(to_delete.size(), 86); + + // Verify same backups kept as in Case 5 + std::set expected_kept_15 = { + "backup_0.dat", "backup_1.dat", "backup_2.dat", "backup_3.dat", "backup_4.dat", + "backup_5.dat", "backup_6.dat", "backup_7.dat", "backup_8.dat", "backup_9.dat", + "backup_15.dat", "backup_31.dat", "backup_63.dat", "backup_99.dat" + }; + std::set actual_kept_15; + for (const auto& [time, path] : backups) { + if (std::find(to_delete.begin(), to_delete.end(), path) == to_delete.end()) { + actual_kept_15.insert(fs::PathToString(path.filename())); + } + } + BOOST_CHECK(expected_kept_15 == actual_kept_15); + + // Now test with maxBackups=12 (less than natural retention) + to_delete = GetBackupsToDelete(backups, 10, 12); + + // Should cap at 12 backups: keep latest 10 + 2 oldest time ranges + // Total: 12 kept, 88 deleted + BOOST_CHECK_EQUAL(to_delete.size(), 88); + + // Verify exact backups kept when capped + std::set expected_kept_12 = { + "backup_0.dat", "backup_1.dat", "backup_2.dat", "backup_3.dat", "backup_4.dat", + "backup_5.dat", "backup_6.dat", "backup_7.dat", "backup_8.dat", "backup_9.dat", + "backup_15.dat", "backup_31.dat" + }; + std::set actual_kept_12; + for (const auto& [time, path] : backups) { + if (std::find(to_delete.begin(), to_delete.end(), path) == to_delete.end()) { + actual_kept_12.insert(fs::PathToString(path.filename())); + } + } + BOOST_CHECK(expected_kept_12 == actual_kept_12); } -BOOST_AUTO_TEST_CASE(hard_max_limit_caps_retention) +BOOST_AUTO_TEST_CASE(irregular_backup_schedule) { - auto make_time = [](int64_t seconds_ago) { - return fs::file_time_type{std::chrono::file_clock::time_point{std::chrono::seconds(seconds_ago)}}; + auto now = std::chrono::file_clock::now(); + auto make_time = [&now](int64_t days_ago) { + auto days_duration = std::chrono::duration>(days_ago); + return fs::file_time_type{now - days_duration}; }; auto make_path = [](int i) { return fs::u8path("backup_" + ToString(i) + ".dat"); @@ -164,18 +210,91 @@ BOOST_AUTO_TEST_CASE(hard_max_limit_caps_retention) std::multimap backups; - // Test that maxBackups hard cap limits retention when exponential/index-based - // retention would keep more than maxBackups. With 50 backups and nWalletBackups=10, - // exponential logic would keep indices 0-9 (latest 10), 15, 31, 49 = 13 backups. - // But with maxBackups=12, the hard cap should limit kept backups to 12, resulting in 38 deletions. + // Test irregular schedule: multiple backups some days, gaps on others + // Day 0: 5 backups + for (int i = 0; i < 5; ++i) { + backups.insert({make_time(0), make_path(i)}); + } + // Day 1: 3 backups + for (int i = 5; i < 8; ++i) { + backups.insert({make_time(1), make_path(i)}); + } + // Day 2: 2 backups + for (int i = 8; i < 10; ++i) { + backups.insert({make_time(2), make_path(i)}); + } + // Day 10: 1 backup (gap) + backups.insert({make_time(10), make_path(10)}); + // Day 20: 1 backup (gap) + backups.insert({make_time(20), make_path(11)}); - backups.clear(); - for (int i = 0; i < 50; ++i) { - backups.insert({make_time(1000 - i), make_path(i)}); + auto to_delete = GetBackupsToDelete(backups, 10, 50); + + // Should keep: + // - Latest 10 (5 from day 0, 3 from day 1, 2 from day 2) + // - Oldest in [8,16): day 10 + // - Oldest in [16,32): day 20 + // Total: 12 kept, 0 deleted + BOOST_CHECK_EQUAL(to_delete.size(), 0); + + // Verify all backups are kept + std::set expected_kept = { + "backup_0.dat", "backup_1.dat", "backup_2.dat", "backup_3.dat", "backup_4.dat", + "backup_5.dat", "backup_6.dat", "backup_7.dat", "backup_8.dat", "backup_9.dat", + "backup_10.dat", "backup_11.dat" + }; + std::set actual_kept; + for (const auto& [time, path] : backups) { + actual_kept.insert(fs::PathToString(path.filename())); } + BOOST_CHECK(expected_kept == actual_kept); +} - auto to_delete = GetBackupsToDelete(backups, 10, 12); - BOOST_CHECK_EQUAL(to_delete.size(), 38); // 50 - 12 = 38 +BOOST_AUTO_TEST_CASE(long_inactivity_period) +{ + auto now = std::chrono::file_clock::now(); + auto make_time = [&now](int64_t days_ago) { + auto days_duration = std::chrono::duration>(days_ago); + return fs::file_time_type{now - days_duration}; + }; + auto make_path = [](int i) { + return fs::u8path("backup_" + ToString(i) + ".dat"); + }; + + std::multimap backups; + + // 15 backups created 60 days ago, then nothing until today + for (int i = 0; i < 15; ++i) { + backups.insert({make_time(60), make_path(i)}); + } + // New backup today + backups.insert({make_time(0), make_path(15)}); + + auto to_delete = GetBackupsToDelete(backups, 10, 50); + + // Should keep: + // - Latest 10 (1 from today, 9 from 60 days ago) + // - Oldest in [32,64): 6 backups from 60 days ago qualify, keep the oldest + // Total: 11 kept, 5 deleted + BOOST_CHECK_EQUAL(to_delete.size(), 5); + + // Verify exact backups kept + // Sorted order: backup_15 (0 days), then backup_14-0 in reverse insertion order (all 60 days) + // Latest 10 by count: backup_15, backup_14, backup_13, ..., backup_6 + // Oldest in [32,64): All backups 0-14 are 60 days old, backup_0 is oldest by insertion order + std::set expected_kept = { + "backup_0.dat", // oldest in [32,64) range + "backup_6.dat", "backup_7.dat", "backup_8.dat", "backup_9.dat", + "backup_10.dat", "backup_11.dat", "backup_12.dat", "backup_13.dat", "backup_14.dat", + "backup_15.dat" // newest + }; + std::set actual_kept; + for (const auto& [time, path] : backups) { + if (std::find(to_delete.begin(), to_delete.end(), path) == to_delete.end()) { + actual_kept.insert(fs::PathToString(path.filename())); + } + } + BOOST_CHECK(expected_kept == actual_kept); } BOOST_AUTO_TEST_SUITE_END() From d7a51760c46b267e7c2cecbbcbb2b249fcbeeaa6 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Sat, 29 Nov 2025 19:45:12 +0300 Subject: [PATCH 11/12] refactor: deduplicate backup initialization logic Make InitAutoBackup() accept an ArgsManager parameter and call it from Unlock() instead of duplicating the configuration reading and validation logic. This ensures consistency and reduces code duplication. --- src/wallet/wallet.cpp | 18 +++++------------- src/wallet/wallet.h | 2 +- 2 files changed, 6 insertions(+), 14 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 7c2b750a1f89..b3993a418060 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3439,15 +3439,15 @@ std::vector GetBackupsToDelete(const std::multimap nMaxWalletBackups) { - nWalletBackups = nMaxWalletBackups; - } + InitAutoBackup(m_args); } return true; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 728779fd4092..c833946ce02d 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -937,7 +937,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati void postInitProcess(); /* AutoBackup functionality */ - static void InitAutoBackup(); + static void InitAutoBackup(const ArgsManager& args = gArgs); bool AutoBackupWallet(const fs::path& wallet_path, bilingual_str& error_string, std::vector& warnings); bool BackupWallet(const std::string& strDest) const; From 94ffc357d63d46d88568d79dcd720565d93121b6 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Sat, 29 Nov 2025 21:36:39 +0300 Subject: [PATCH 12/12] fix: remove duplicate global nWalletBackups variable Remove the unused global nWalletBackups from util/system and use CWallet::nWalletBackups everywhere. Qt and CoinJoin code now access the backup status through the interfaces::Wallet::getWalletBackupStatus() method instead of directly accessing wallet internals. This fixes a bug where the global was never updated from its initial value of 10, causing Qt/CoinJoin to see incorrect backup status. --- src/coinjoin/client.cpp | 2 +- src/interfaces/wallet.h | 4 ++++ src/qt/overviewpage.cpp | 14 ++++++++------ src/util/system.cpp | 9 --------- src/util/system.h | 1 - src/wallet/interfaces.cpp | 1 + 6 files changed, 14 insertions(+), 17 deletions(-) diff --git a/src/coinjoin/client.cpp b/src/coinjoin/client.cpp index 79651d64e428..597e0042ae8a 100644 --- a/src/coinjoin/client.cpp +++ b/src/coinjoin/client.cpp @@ -724,7 +724,7 @@ bool CCoinJoinClientManager::CheckAutomaticBackup() // We don't need auto-backups for descriptor wallets if (!m_wallet->IsLegacy()) return true; - switch (nWalletBackups) { + switch (CWallet::nWalletBackups) { case 0: strAutoDenomResult = _("Automatic backups disabled") + Untranslated(", ") + _("no mixing available."); WalletCJLogPrint(m_wallet, "CCoinJoinClientManager::CheckAutomaticBackup -- %s\n", strAutoDenomResult.original); diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index e9f209df28f4..a793b5ff9c4a 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -108,6 +108,10 @@ class Wallet //! Get the number of keys since the last auto backup virtual int64_t getKeysLeftSinceAutoBackup() = 0; + //! Get wallet backup status + //! Returns: 1..20 = number of backups to keep, 0 = disabled, -1 = error, -2 = locked + virtual int getWalletBackupStatus() = 0; + //! Get wallet name. virtual std::string getWalletName() = 0; diff --git a/src/qt/overviewpage.cpp b/src/qt/overviewpage.cpp index 80f399bd0328..a9e0b08e8384 100644 --- a/src/qt/overviewpage.cpp +++ b/src/qt/overviewpage.cpp @@ -498,9 +498,10 @@ void OverviewPage::coinJoinStatus(bool fForce) if (!fForce && (clientModel->node().shutdownRequested() || !clientModel->masternodeSync().isBlockchainSynced())) return; // Disable any PS UI for masternode or when autobackup is disabled or failed for whatever reason - if (clientModel->node().isMasternode() || nWalletBackups <= 0) { + int backupStatus = walletModel->wallet().getWalletBackupStatus(); + if (clientModel->node().isMasternode() || backupStatus <= 0) { DisableCoinJoinCompletely(); - if (nWalletBackups <= 0) { + if (backupStatus <= 0) { ui->labelCoinJoinEnabled->setToolTip(tr("Automatic backups are disabled, no mixing available!")); } return; @@ -595,7 +596,7 @@ void OverviewPage::coinJoinStatus(bool fForce) // Warn user that wallet is running out of keys // NOTE: we do NOT warn user and do NOT create autobackups if mixing is not running - if (walletModel->wallet().isLegacy() && nWalletBackups > 0 && walletModel->getKeysLeftSinceAutoBackup() < COINJOIN_KEYS_THRESHOLD_WARNING) { + if (walletModel->wallet().isLegacy() && walletModel->wallet().getWalletBackupStatus() > 0 && walletModel->getKeysLeftSinceAutoBackup() < COINJOIN_KEYS_THRESHOLD_WARNING) { QSettings settings; if(settings.value("fLowKeysWarning").toBool()) { QString strWarn = tr("Very low number of keys left since last automatic backup!") + "

" + @@ -639,7 +640,8 @@ void OverviewPage::coinJoinStatus(bool fForce) ui->labelCoinJoinEnabled->setText(strEnabled); if (walletModel->wallet().isLegacy()) { - if(nWalletBackups == -1) { + int backupStatus = walletModel->wallet().getWalletBackupStatus(); + if (backupStatus == -1) { // Automatic backup failed, nothing else we can do until user fixes the issue manually DisableCoinJoinCompletely(); @@ -649,7 +651,7 @@ void OverviewPage::coinJoinStatus(bool fForce) ui->labelCoinJoinEnabled->setToolTip(strError); return; - } else if(nWalletBackups == -2) { + } else if (backupStatus == -2) { // We were able to create automatic backup but keypool was not replenished because wallet is locked. QString strWarning = tr("WARNING! Failed to replenish keypool, please unlock your wallet to do so."); ui->labelCoinJoinEnabled->setToolTip(strWarning); @@ -754,7 +756,7 @@ void OverviewPage::DisableCoinJoinCompletely() ui->toggleCoinJoin->setText("(" + tr("Disabled") + ")"); ui->frameCoinJoin->setEnabled(false); - if (nWalletBackups <= 0) { + if (walletModel && walletModel->wallet().getWalletBackupStatus() <= 0) { ui->labelCoinJoinEnabled->setText("(" + tr("Disabled") + ")"); } walletModel->coinJoin()->stopMixing(); diff --git a/src/util/system.cpp b/src/util/system.cpp index c9e849b69229..ac9b1b5b82b9 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -78,15 +78,6 @@ const int64_t nStartupTime = GetTime(); //Dash only features const std::string gCoinJoinName = "CoinJoin"; -/** - nWalletBackups: - 1..10 - number of automatic backups to keep - 0 - disabled by command-line - -1 - disabled because of some error during run-time - -2 - disabled because wallet was locked and we were not able to replenish keypool -*/ -int nWalletBackups = 10; - const char * const BITCOIN_CONF_FILENAME = "dash.conf"; const char * const BITCOIN_SETTINGS_FILENAME = "settings.json"; diff --git a/src/util/system.h b/src/util/system.h index 0b228858047f..52a4e3640cd7 100644 --- a/src/util/system.h +++ b/src/util/system.h @@ -35,7 +35,6 @@ //Dash only features -extern int nWalletBackups; extern const std::string gCoinJoinName; class UniValue; diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 6d087a276594..526cdabe1797 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -181,6 +181,7 @@ class WalletImpl : public Wallet return m_wallet->AutoBackupWallet(wallet_path, error_string, warnings); } int64_t getKeysLeftSinceAutoBackup() override { return m_wallet->nKeysLeftSinceAutoBackup; } + int getWalletBackupStatus() override { return CWallet::nWalletBackups; } std::string getWalletName() override { return m_wallet->GetName(); } util::Result getNewDestination(const std::string& label) override {