From c6f23e25df05518746ea62138a6b87e98b7dcf13 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 61d50f0a0b08..38553aced04c 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -206,6 +206,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 7bb836c40083..9837179659ed 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 773d1dd6cc7d..2185ef161f7d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -56,6 +56,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 " @@ -3489,13 +3493,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 @@ -3627,20 +3683,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; } } @@ -3848,7 +3899,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 0544840047ee..4d991554b5ba 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -82,6 +82,19 @@ void NotifyWalletLoading(WalletContext& context, const std::shared_ptr& 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 @@ -507,6 +520,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 8e06dde4eec440670728b000c46111cc002b8de5 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 dc0a7a3e6c15a2fec24fb2d3e13bb5325c717787 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 2185ef161f7d..465ed08bac5a 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -57,6 +57,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; @@ -3497,6 +3500,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; } @@ -3552,6 +3560,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 @@ -3903,6 +3916,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 0efabe018ca7db82b35f995002c4ecfd0fddbfcb 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 a646f91aeb7a1d3ea439c7fffeff4720b7e623a3 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 465ed08bac5a..2338c22ba6b6 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3560,7 +3560,7 @@ void CWallet::InitAutoBackup() nMaxWalletBackups = gArgs.GetIntArg("-maxwalletbackups", DEFAULT_MAX_BACKUPS); nMaxWalletBackups = std::max(0, nMaxWalletBackups); - + // Enforce nWalletBackups <= nMaxWalletBackups if (nWalletBackups > nMaxWalletBackups) { nWalletBackups = nMaxWalletBackups; @@ -3916,7 +3916,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 8df14ca07833a90095daa844c5a17d925dc93c33 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 ce185aa3d9029fb78e1906e96e35dac4c5c072bc 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 93768ede68740028879f696819d18f7e062b686d 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 2338c22ba6b6..5f3857a7b7a2 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3510,34 +3510,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 4d991554b5ba..9ef31cfc1b05 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -90,7 +90,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 e2e0006f876138ac9b96307858da28b0c17836ec 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 5f3857a7b7a2..3e9dd4622d03 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3500,7 +3500,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; } @@ -3509,51 +3509,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 9ef31cfc1b05..00fcb08ecae2 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -89,10 +89,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 ccec0c0eb3b02023f37d5befcaf1ebc5334d3959 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 a628851e7420a1888d67dee4b7208a7fdcca4269 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 3e9dd4622d03..481b19e3bba9 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3579,15 +3579,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 00fcb08ecae2..96e8742b60c9 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -946,7 +946,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 30a0fb26f2b86d469792797f3b341d5e1e4f1c0b 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 6474cbd708bb..0192ce5b7759 100644 --- a/src/coinjoin/client.cpp +++ b/src/coinjoin/client.cpp @@ -610,7 +610,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 161b148ea81d..e159c3f2aec1 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -115,6 +115,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 7f11fde0f9a8..380bc89766ce 100644 --- a/src/qt/overviewpage.cpp +++ b/src/qt/overviewpage.cpp @@ -520,9 +520,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; @@ -617,7 +618,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!") + "

" + @@ -661,7 +662,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(); @@ -671,7 +673,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); @@ -776,7 +778,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 d769dbd055be..3ef5c4e925fe 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 08722d5f57bc..be5cfd24f056 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 f6dd6f7a0a8b..0bbecd53c624 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -213,6 +213,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 {