fix(wallet): never prune the just-written backup on clock skew

Retention ranked backups purely by the timestamp in their filenames and
protected the newest -createwalletbackups of them. After the system clock
moved backward, every backup written before the jump sorted ahead of the one
just created, which could then fall outside the count window and be deleted as
a non-oldest member of its age range, right after AutoBackupWallet() had
written it and reset nKeysLeftSinceAutoBackup.

Pass the time of the backup that triggered pruning into GetBackupsToDelete()
and consider only backups dated at or before it. The just-written backup is
therefore always the newest candidate, anchors the age ranges and sits inside
the count window. Backups dated after it cannot be placed relative to it, so
they are left untouched until the clock catches up rather than being ranked
ahead of the new backup or pruned against it. The time is floored to the
minute to match the precision ParseBackupFileTime() reads back from the name.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Author: PastaPastaPasta

doc/managing-wallets.md

@@ -110,7 +110,7 @@ Non-HD wallets must be backed up every 1000 keys used since the previous backup,
 
 For legacy wallets, Dash Core automatically creates a backup in the `backups` directory inside the data directory on every startup and whenever the keypool is replenished. Each file is named after the wallet with the backup time appended, e.g. `wallet.dat.2026-08-02-14-30`.
 
-Older backups are pruned as new ones are made. The most recent `-createwalletbackups` backups (default: 10, max: 20) are always kept. Beyond those, one backup is kept from each widening age range — 1-2 hours old, 2-4 hours, 4-8 hours, and so on, each range twice as long as the one before (512-1024 hours is about 3-6 weeks) — up to `-maxwalletbackups` files in total (default: 30). This preserves restore points going back months, even for a wallet that writes many backups a day while mixing, while keeping the size of the `backups` directory predictable. Several backups made within the same hour do not each get their own range: beyond the most recent `-createwalletbackups`, only the oldest backup in each range is kept.
+Older backups are pruned as new ones are made. The most recent `-createwalletbackups` backups (default: 10, max: 20) are always kept. Beyond those, one backup is kept from each widening age range — 1-2 hours old, 2-4 hours, 4-8 hours, and so on, each range twice as long as the one before (512-1024 hours is about 3-6 weeks) — up to `-maxwalletbackups` files in total (default: 30). This preserves restore points going back months, even for a wallet that writes many backups a day while mixing, while keeping the size of the `backups` directory predictable. Several backups made within the same hour do not each get their own range: beyond the most recent `-createwalletbackups`, only the oldest backup in each range is kept. The backup just written is never pruned, and if the system clock has been set back, backups dated later than it are left untouched until the clock catches up with them.
 
 Setting either option to `0` disables automatic backups, in which case existing backups are left untouched. Renaming a backup also excludes it from pruning, which is a simple way to keep one indefinitely.
 

src/wallet/test/backup_tests.cpp

@@ -42,15 +42,15 @@ BOOST_AUTO_TEST_CASE(time_based_exponential_retention)
     for (int i = 0; i < 5; ++i) {
         backups.insert({MakeBackupTime(i), MakeBackupPath(i)});
     }
-    auto to_delete = GetBackupsToDelete(backups, 10, 50);
+    auto to_delete = GetBackupsToDelete(backups, BACKUP_ANCHOR, 10, 50);
     BOOST_CHECK(to_delete.empty());
 
     // Case 2: Exactly nWalletBackups (10)
     backups.clear();
     for (int i = 0; i < 10; ++i) {
         backups.insert({MakeBackupTime(i), MakeBackupPath(i)});
     }
-    to_delete = GetBackupsToDelete(backups, 10, 50);
+    to_delete = GetBackupsToDelete(backups, BACKUP_ANCHOR, 10, 50);
     BOOST_CHECK(to_delete.empty());
 
     // Case 3: 11 backups - all taken at the same time
@@ -60,7 +60,7 @@ BOOST_AUTO_TEST_CASE(time_based_exponential_retention)
     for (int i = 0; i < 11; ++i) {
         backups.insert({MakeBackupTime(0), MakeBackupPath(0, i)});
     }
-    to_delete = GetBackupsToDelete(backups, 10, 50);
+    to_delete = GetBackupsToDelete(backups, BACKUP_ANCHOR, 10, 50);
     // All backups are 0 hours old, so none fall into [1,2) or later ranges
     // Keep only latest 10, delete 1
     BOOST_CHECK_EQUAL(to_delete.size(), 1);
@@ -83,7 +83,7 @@ BOOST_AUTO_TEST_CASE(time_based_exponential_retention)
     backups.insert({MakeBackupTime(25), MakeBackupPath(25)}); // [512,1024) hours
     backups.insert({MakeBackupTime(30), MakeBackupPath(30)}); // [512,1024) hours
 
-    to_delete = GetBackupsToDelete(backups, 10, 50);
+    to_delete = GetBackupsToDelete(backups, BACKUP_ANCHOR, 10, 50);
 
     // Should keep:
     // - Latest 10 (by count): all backups from today
@@ -108,7 +108,7 @@ BOOST_AUTO_TEST_CASE(time_based_exponential_retention)
         backups.insert({MakeBackupTime(i), MakeBackupPath(i)});
     }
 
-    to_delete = GetBackupsToDelete(backups, 10, 50);
+    to_delete = GetBackupsToDelete(backups, BACKUP_ANCHOR, 10, 50);
 
     // Should keep:
     // - Latest 10 by count and the oldest backup in each exponential time range
@@ -139,7 +139,7 @@ BOOST_AUTO_TEST_CASE(hard_max_limit)
         backups.insert({MakeBackupTime(i), MakeBackupPath(i)});
     }
 
-    auto to_delete = GetBackupsToDelete(backups, 10, 15);
+    auto to_delete = GetBackupsToDelete(backups, BACKUP_ANCHOR, 10, 15);
 
     // Without maxBackups limit, we'd keep 15 backups (see Case 5 above)
     // With maxBackups=15, we still keep all 15 (at the limit)
@@ -160,7 +160,7 @@ BOOST_AUTO_TEST_CASE(hard_max_limit)
     BOOST_CHECK(expected_kept_15 == actual_kept_15);
 
     // Now test with maxBackups=12 (less than natural retention)
-    to_delete = GetBackupsToDelete(backups, 10, 12);
+    to_delete = GetBackupsToDelete(backups, BACKUP_ANCHOR, 10, 12);
 
     // Should cap at 12 backups: keep latest 10 + the 2 most recent time ranges
     // Total: 12 kept, 88 deleted
@@ -202,7 +202,7 @@ BOOST_AUTO_TEST_CASE(irregular_backup_schedule)
     // Day 20: 1 backup (gap)
     backups.insert({MakeBackupTime(20), MakeBackupPath(20)});
 
-    auto to_delete = GetBackupsToDelete(backups, 10, 50);
+    auto to_delete = GetBackupsToDelete(backups, BACKUP_ANCHOR, 10, 50);
 
     // Should keep:
     // - Latest 10 (5 from day 0, 3 from day 1, 2 from day 2)
@@ -234,7 +234,7 @@ BOOST_AUTO_TEST_CASE(long_inactivity_period)
     // New backup today
     backups.insert({MakeBackupTime(0), MakeBackupPath(0)});
 
-    auto to_delete = GetBackupsToDelete(backups, 10, 50);
+    auto to_delete = GetBackupsToDelete(backups, BACKUP_ANCHOR, 10, 50);
 
     // Should keep:
     // - Latest 10 (1 from today, 9 from 60 days ago)
@@ -259,6 +259,40 @@ BOOST_AUTO_TEST_CASE(long_inactivity_period)
     BOOST_CHECK(expected_kept == actual_kept);
 }
 
+BOOST_AUTO_TEST_CASE(clock_moved_backward)
+{
+    std::multimap<std::chrono::system_clock::time_point, fs::path> backups;
+
+    // 40 daily backups written with a correct clock, then the clock jumps back 20 days
+    // and a new backup is written half an hour after the existing day-20 one.
+    for (int i = 0; i < 40; ++i) {
+        backups.insert({MakeBackupTime(i), MakeBackupPath(i)});
+    }
+    const auto backup_time{MakeBackupTime(20) + std::chrono::minutes{30}};
+    const fs::path just_written{MakeBackupPath(20, 30)};
+    backups.insert({backup_time, just_written});
+
+    const auto to_delete = GetBackupsToDelete(backups, backup_time, 10, 50);
+    const std::set<fs::path> actual_deletions{to_delete.begin(), to_delete.end()};
+
+    // Ranked by filename time alone, the new backup would fall behind the 20 later-dated
+    // files, out of the count window, and be deleted as a non-oldest member of its range.
+    BOOST_CHECK(!actual_deletions.count(just_written));
+
+    // The later-dated backups can't be placed relative to the new one, so none are touched.
+    for (int i = 0; i < 20; ++i) {
+        BOOST_CHECK(!actual_deletions.count(MakeBackupPath(i)));
+    }
+
+    // Everything else is pruned as if the new backup were the newest: it and days 20-28
+    // fill the count window, and days 30 and 39 are the oldest in the [128,256) and
+    // [256,512) hour ranges before it.
+    const std::set<fs::path> expected_deletions{MakeBackupPath(29), MakeBackupPath(31), MakeBackupPath(32),
+                                                MakeBackupPath(33), MakeBackupPath(34), MakeBackupPath(35),
+                                                MakeBackupPath(36), MakeBackupPath(37), MakeBackupPath(38)};
+    BOOST_CHECK(expected_deletions == actual_deletions);
+}
+
 BOOST_AUTO_TEST_CASE(frequent_backups_keep_history)
 {
     // A wallet that mixes all day can write more backups a day than the count window
@@ -275,7 +309,7 @@ BOOST_AUTO_TEST_CASE(frequent_backups_keep_history)
         backups.insert({backup_time, fs::u8path(strprintf("wallet.dat.%04i-%02u-%02u-%02i-%02i", int(ymd.year()),
                                                           unsigned(ymd.month()), unsigned(ymd.day()),
                                                           hms.hours().count(), hms.minutes().count()))});
-        for (const auto& path : GetBackupsToDelete(backups, 10, 30)) {
+        for (const auto& path : GetBackupsToDelete(backups, backup_time, 10, 30)) {
             std::erase_if(backups, [&](const auto& entry) { return entry.second == path; });
         }
         BOOST_REQUIRE_LE(backups.size(), 30U);
@@ -300,9 +334,9 @@ BOOST_AUTO_TEST_CASE(non_positive_max_backups)
     }
 
     // maxBackups <= 0 means "delete nothing", regardless of nWalletBackups
-    BOOST_CHECK(GetBackupsToDelete(backups, 10, 0).empty());
-    BOOST_CHECK(GetBackupsToDelete(backups, 10, -1).empty());
-    BOOST_CHECK(GetBackupsToDelete(backups, 0, 0).empty());
+    BOOST_CHECK(GetBackupsToDelete(backups, BACKUP_ANCHOR, 10, 0).empty());
+    BOOST_CHECK(GetBackupsToDelete(backups, BACKUP_ANCHOR, 10, -1).empty());
+    BOOST_CHECK(GetBackupsToDelete(backups, BACKUP_ANCHOR, 0, 0).empty());
 }
 
 BOOST_AUTO_TEST_CASE(count_window_boundaries)
@@ -314,7 +348,7 @@ BOOST_AUTO_TEST_CASE(count_window_boundaries)
 
     // maxBackups == nWalletBackups leaves no room for time buckets, degrading to
     // the pre-exponential "keep the N most recent" policy.
-    auto to_delete = GetBackupsToDelete(backups, 10, 10);
+    auto to_delete = GetBackupsToDelete(backups, BACKUP_ANCHOR, 10, 10);
     BOOST_CHECK_EQUAL(to_delete.size(), 10);
     std::set<fs::path> expected_deleted;
     for (int i = 10; i < 20; ++i) {
@@ -325,7 +359,7 @@ BOOST_AUTO_TEST_CASE(count_window_boundaries)
     // An empty count window leaves retention to the time buckets: the newest backup is
     // kept as their anchor, then the oldest in each of the [16,32), [32,64), [64,128),
     // [128,256) and [256,512) hour ranges.
-    to_delete = GetBackupsToDelete(backups, 0, 30);
+    to_delete = GetBackupsToDelete(backups, BACKUP_ANCHOR, 0, 30);
     std::set<fs::path> expected_kept{MakeBackupPath(0), MakeBackupPath(1),  MakeBackupPath(2),
                                      MakeBackupPath(5), MakeBackupPath(10), MakeBackupPath(19)};
     std::set<fs::path> actual_kept;
@@ -337,7 +371,7 @@ BOOST_AUTO_TEST_CASE(count_window_boundaries)
     BOOST_CHECK(expected_kept == actual_kept);
 
     // Negative nWalletBackups is treated the same as an empty count window.
-    BOOST_CHECK(GetBackupsToDelete(backups, -1, 30) == to_delete);
+    BOOST_CHECK(GetBackupsToDelete(backups, BACKUP_ANCHOR, -1, 30) == to_delete);
 }
 
 BOOST_AUTO_TEST_CASE(init_auto_backup_clamping)

src/wallet/wallet.cpp

@@ -3663,18 +3663,22 @@ void CWallet::postInitProcess()
 }
 
 std::vector<fs::path> GetBackupsToDelete(const std::multimap<std::chrono::system_clock::time_point, fs::path>& backups,
-                                         int nWalletBackups, int maxBackups)
+                                         std::chrono::system_clock::time_point backup_time, int nWalletBackups,
+                                         int maxBackups)
 {
     if (maxBackups <= 0) return {};
 
     // CWallet::nWalletBackups doubles as an error status and can be negative, so
     // treat anything below zero as an empty count window rather than wrapping the cast.
     const size_t keep_by_count{nWalletBackups > 0 ? static_cast<size_t>(nWalletBackups) : 0};
-    if (backups.size() <= keep_by_count) return {};
 
-    // Newest first, so age ascends with the index.
-    const std::vector<std::pair<std::chrono::system_clock::time_point, fs::path>> sorted_backups(backups.rbegin(),
-                                                                                                backups.rend());
+    // Backups dated after the one just written can only come from a clock that has since
+    // moved backward. They can't be placed relative to it, and ranking them as newer would
+    // push it out of the count window, so leave them alone. Newest first, so age ascends
+    // with the index.
+    const std::vector<std::pair<std::chrono::system_clock::time_point, fs::path>> sorted_backups(
+        std::make_reverse_iterator(backups.upper_bound(backup_time)), backups.rend());
+    if (sorted_backups.size() <= keep_by_count) return {};
     const auto newest = sorted_backups[0].first;
 
     // Always keep the newest backup, which anchors the age ranges, plus the count window.
@@ -3806,8 +3810,8 @@ bool CWallet::AutoBackupWallet(const fs::path& wallet_path, bilingual_str& error
     }
 
     // Create backup of the ...
+    const std::chrono::sys_seconds secs{GetTime<std::chrono::seconds>()};
     std::string dateTimeStr = [&]() {
-        const std::chrono::sys_seconds secs{GetTime<std::chrono::seconds>()};
         const auto days{std::chrono::floor<std::chrono::days>(secs)};
         const std::chrono::year_month_day ymd{days};
         const std::chrono::hh_mm_ss hms{secs - days};
@@ -3877,7 +3881,9 @@ bool CWallet::AutoBackupWallet(const fs::path& wallet_path, bilingual_str& error
         }
     }
 
-    std::vector<fs::path> backupsToDelete = GetBackupsToDelete(folder_set, nWalletBackups, nMaxWalletBackups);
+    // The filename only carries minutes, so compare at that precision.
+    std::vector<fs::path> backupsToDelete = GetBackupsToDelete(folder_set, std::chrono::floor<std::chrono::minutes>(secs),
+                                                               nWalletBackups, nMaxWalletBackups);
     for (const auto& path : backupsToDelete) {
         try {
             fs::remove(path);

src/wallet/wallet.h

@@ -100,9 +100,14 @@ static constexpr int MAX_N_WALLET_BACKUPS = 20;
  * maxBackups <= 0 deletes nothing, and a non-positive nWalletBackups leaves retention
  * entirely to the age ranges. Callers must keep nWalletBackups <= maxBackups for the
  * total to hold; InitAutoBackup() enforces that.
+ *
+ * backup_time is when the backup that triggered pruning was written. Backups dated after
+ * it, which only a clock that moved backward can produce, are never deleted and take no
+ * part in retention, so the backup just written is always the newest one and is kept.
  */
 std::vector<fs::path> GetBackupsToDelete(const std::multimap<std::chrono::system_clock::time_point, fs::path>& backups,
-                                         int nWalletBackups, int maxBackups = DEFAULT_MAX_BACKUPS);
+                                         std::chrono::system_clock::time_point backup_time, int nWalletBackups,
+                                         int maxBackups = DEFAULT_MAX_BACKUPS);
 
 /**
  * Read back the UTC timestamp AutoBackupWallet() embeds in backup filenames (e.g.