Fixed protocol 23 upgrade bug on snapshot boundary

Author: SirTyson

src/bucket/BucketManager.cpp

@@ -1079,15 +1079,17 @@ BucketManager::startBackgroundEvictionScan(uint32_t ledgerSeq,
         mSnapshotManager->copySearchableLiveBucketListSnapshot();
     auto const& sas = cfg.stateArchivalSettings();
 
-    using task_t = std::packaged_task<EvictionResultCandidates()>;
+    using task_t =
+        std::packaged_task<std::unique_ptr<EvictionResultCandidates>()>;
     // MSVC gotcha: searchableBL has to be shared_ptr because MSVC wants to
     // copy this lambda, otherwise we could use unique_ptr.
     auto task = std::make_shared<task_t>(
         [bl = std::move(searchableBL), iter = cfg.evictionIterator(), ledgerSeq,
          ledgerVers, sas, &counters = mBucketListEvictionCounters,
          stats = mEvictionStatistics] {
-            return bl->scanForEviction(ledgerSeq, counters, iter, stats, sas,
-                                       ledgerVers);
+            return std::make_unique<EvictionResultCandidates>(
+                bl->scanForEviction(ledgerSeq, counters, iter, stats, sas,
+                                    ledgerVers));
         });
 
     mEvictionFuture = task->get_future();
@@ -1114,14 +1116,14 @@ BucketManager::resolveBackgroundEvictionScan(
 
     // If eviction related settings changed during the ledger, we have to
     // restart the scan
-    if (!evictionCandidates.isValid(ledgerSeq,
+    if (!evictionCandidates->isValid(ledgerSeq, ledgerVers,
                                     networkConfig.stateArchivalSettings()))
     {
         startBackgroundEvictionScan(ledgerSeq, ledgerVers, networkConfig);
         evictionCandidates = mEvictionFuture.get();
     }
 
-    auto& eligibleEntries = evictionCandidates.eligibleEntries;
+    auto& eligibleEntries = evictionCandidates->eligibleEntries;
 
     for (auto iter = eligibleEntries.begin(); iter != eligibleEntries.end();)
     {
@@ -1139,7 +1141,7 @@ BucketManager::resolveBackgroundEvictionScan(
     auto remainingEntriesToEvict =
         networkConfig.stateArchivalSettings().maxEntriesToArchive;
     auto entryToEvictIter = eligibleEntries.begin();
-    auto newEvictionIterator = evictionCandidates.endOfRegionIterator;
+    auto newEvictionIterator = evictionCandidates->endOfRegionIterator;
 
     // Return vectors include both evicted entry and associated TTL
     std::vector<LedgerKey> deletedKeys;
@@ -1179,7 +1181,7 @@ BucketManager::resolveBackgroundEvictionScan(
     // region
     if (remainingEntriesToEvict != 0)
     {
-        newEvictionIterator = evictionCandidates.endOfRegionIterator;
+        newEvictionIterator = evictionCandidates->endOfRegionIterator;
     }
 
     networkConfig.updateEvictionIterator(ltx, newEvictionIterator);

src/bucket/BucketManager.h

@@ -111,7 +111,7 @@ class BucketManager : NonMovableOrCopyable
     std::map<LedgerEntryTypeAndDurability, medida::Counter&>
         mBucketListEntrySizeCounters;
 
-    std::future<EvictionResultCandidates> mEvictionFuture{};
+    std::future<std::unique_ptr<EvictionResultCandidates>> mEvictionFuture{};
 
     // Copy app's config for thread-safe access
     Config const mConfig;

src/bucket/BucketUtils.cpp

@@ -3,6 +3,8 @@
 // of this distribution or at http://www.apache.org/licenses/LICENSE-2.0
 
 #include "bucket/BucketUtils.h"
+#include "bucket/BucketBase.h"
+#include "util/ProtocolVersion.h"
 #include <medida/metrics_registry.h>
 
 namespace stellar
@@ -94,10 +96,26 @@ MergeCounters::operator==(MergeCounters const& other) const
 // Check that eviction scan is based off of current ledger snapshot and that
 // archival settings have not changed
 bool
-EvictionResultCandidates::isValid(uint32_t currLedger,
+EvictionResultCandidates::isValid(uint32_t currLedgerSeq,
+                                  uint32_t currLedgerVers,
                                   StateArchivalSettings const& currSas) const
 {
-    return initialLedger == currLedger &&
+    // If the eviction scan started before a protocol upgrade, and the protocol
+    // upgrade changes eviction scan behavior during the scan, we need
+    // to restart with the new protocol version. We only care about
+    // `FIRST_PROTOCOL_SUPPORTING_PERSISTENT_EVICTION`, other upgrades don't
+    // affect evictions scans.
+    if (protocolVersionIsBefore(
+            initialLedgerVers,
+            BucketBase::FIRST_PROTOCOL_SUPPORTING_PERSISTENT_EVICTION) &&
+        protocolVersionStartsFrom(
+            currLedgerVers,
+            BucketBase::FIRST_PROTOCOL_SUPPORTING_PERSISTENT_EVICTION))
+    {
+        return false;
+    }
+
+    return initialLedgerSeq == currLedgerSeq &&
            initialSas.maxEntriesToArchive == currSas.maxEntriesToArchive &&
            initialSas.evictionScanSize == currSas.evictionScanSize &&
            initialSas.startingEvictionScanLevel ==

src/bucket/BucketUtils.h

@@ -95,24 +95,28 @@ struct EvictionResultCandidates
     // Eviction iterator at the end of the scan region
     EvictionIterator endOfRegionIterator;
 
-    // LedgerSeq which this scan is based on
-    uint32_t initialLedger{};
+    // LedgerSeq and ledger version which this scan is based on
+    uint32_t const initialLedgerSeq{};
+    uint32_t const initialLedgerVers{};
 
     // State archival settings that this scan is based on
-    StateArchivalSettings initialSas;
+    StateArchivalSettings const initialSas;
 
-    EvictionResultCandidates(StateArchivalSettings const& sas) : initialSas(sas)
+    EvictionResultCandidates(StateArchivalSettings const& sas,
+                             uint32_t initialLedger, uint32_t initialLedgerVers)
+        : initialLedgerSeq(initialLedger)
+        , initialLedgerVers(initialLedgerVers)
+        , initialSas(sas)
     {
     }
 
     // Returns true if this is a valid archival scan for the current ledger
     // and archival settings. This is necessary because we start the scan
     // for ledger N immediately after N - 1 closes. However, ledger N may
-    // contain a network upgrade changing eviction scan settings. Legacy SQL
-    // scans will run based on the changes that occurred during ledger N,
-    // meaning the scan we started at ledger N - 1 is invalid since it was based
-    // off of older settings.
-    bool isValid(uint32_t currLedger,
+    // contain a network upgrade changing eviction scan settings. Eviction scans
+    // must be based on the network settings after applying any upgrades, so if
+    // this occurs we must restart the scan.
+    bool isValid(uint32_t currLedgerSeq, uint32_t currLedgerVers,
                  StateArchivalSettings const& currSas) const;
 };
 

src/bucket/SearchableBucketList.cpp

@@ -31,7 +31,7 @@ SearchableLiveBucketListSnapshot::scanForEviction(
     LiveBucketList::updateStartingEvictionIterator(
         evictionIter, sas.startingEvictionScanLevel, ledgerSeq);
 
-    EvictionResultCandidates result(sas);
+    EvictionResultCandidates result(sas, ledgerSeq, ledgerVers);
     auto startIter = evictionIter;
     auto scanSize = sas.evictionScanSize;
 
@@ -59,7 +59,6 @@ SearchableLiveBucketListSnapshot::scanForEviction(
     }
 
     result.endOfRegionIterator = evictionIter;
-    result.initialLedger = ledgerSeq;
     return result;
 }
 

src/history/HistoryManager.h

@@ -319,14 +319,17 @@ class HistoryManager
     // Calls queueCurrentHistory() if the current ledger is a multiple of
     // getCheckpointFrequency() -- equivalently, the LCL is one _less_ than
     // a multiple of getCheckpointFrequency(). Returns true if checkpoint
-    // publication of the LCL was queued, otherwise false.
-    virtual bool maybeQueueHistoryCheckpoint(uint32_t lcl) = 0;
+    // publication of the LCL was queued, otherwise false. ledgerVers must align
+    // with lcl.
+    virtual bool maybeQueueHistoryCheckpoint(uint32_t lcl,
+                                             uint32_t ledgerVers) = 0;
 
     // Checkpoint the LCL -- both the log of history from the previous
     // checkpoint to it, as well as the bucketlist of its state -- to a
     // publication-queue in the database. This should be followed shortly
-    // (typically after commit) with a call to publishQueuedHistory.
-    virtual void queueCurrentHistory(uint32_t lcl) = 0;
+    // (typically after commit) with a call to publishQueuedHistory. ledgerVers
+    // must align with lcl.
+    virtual void queueCurrentHistory(uint32_t lcl, uint32_t ledgerVers) = 0;
 
     // Return the youngest ledger still in the outgoing publish queue;
     // returns 0 if the publish queue has nothing in it.

src/history/HistoryManagerImpl.cpp

@@ -375,7 +375,8 @@ HistoryManager::getMaxLedgerQueuedToPublish(Config const& cfg)
 }
 
 bool
-HistoryManagerImpl::maybeQueueHistoryCheckpoint(uint32_t lcl)
+HistoryManagerImpl::maybeQueueHistoryCheckpoint(uint32_t lcl,
+                                                uint32_t ledgerVers)
 {
     if (!publishCheckpointOnLedgerClose(lcl, mApp.getConfig()))
     {
@@ -389,12 +390,12 @@ HistoryManagerImpl::maybeQueueHistoryCheckpoint(uint32_t lcl)
         return false;
     }
 
-    queueCurrentHistory(lcl);
+    queueCurrentHistory(lcl, ledgerVers);
     return true;
 }
 
 void
-HistoryManagerImpl::queueCurrentHistory(uint32_t ledger)
+HistoryManagerImpl::queueCurrentHistory(uint32_t ledger, uint32_t ledgerVers)
 {
     ZoneScoped;
 
@@ -407,9 +408,6 @@ HistoryManagerImpl::queueCurrentHistory(uint32_t ledger)
     }
 
     HistoryArchiveState has;
-    auto ledgerVers = mApp.getLedgerManager()
-                          .getLastClosedLedgerHeader()
-                          .header.ledgerVersion;
     if (protocolVersionStartsFrom(
             ledgerVers,
             BucketBase::FIRST_PROTOCOL_SUPPORTING_PERSISTENT_EVICTION))

src/history/HistoryManagerImpl.h

@@ -46,9 +46,10 @@ class HistoryManagerImpl : public HistoryManager
 
     void logAndUpdatePublishStatus() override;
 
-    bool maybeQueueHistoryCheckpoint(uint32_t lcl) override;
+    bool maybeQueueHistoryCheckpoint(uint32_t lcl,
+                                     uint32_t ledgerVers) override;
 
-    void queueCurrentHistory(uint32_t lcl) override;
+    void queueCurrentHistory(uint32_t lcl, uint32_t ledgerVers) override;
 
     void takeSnapshotAndPublish(HistoryArchiveState const& has);
 

src/history/test/HistoryTests.cpp

@@ -1246,6 +1246,16 @@ TEST_CASE("Catchup with protocol upgrade", "[catchup][history]")
             testUpgrade(SOROBAN_PROTOCOL_VERSION);
         }
     }
+    SECTION("hot archive bucket upgrade")
+    {
+        if (protocolVersionEquals(
+                Config::CURRENT_LEDGER_PROTOCOL_VERSION,
+                BucketBase::FIRST_PROTOCOL_SUPPORTING_PERSISTENT_EVICTION))
+        {
+            testUpgrade(
+                BucketBase::FIRST_PROTOCOL_SUPPORTING_PERSISTENT_EVICTION);
+        }
+    }
 }
 
 TEST_CASE("Catchup fatal failure", "[catchup][history]")

src/ledger/LedgerManagerImpl.cpp

@@ -1054,9 +1054,15 @@ LedgerManagerImpl::closeLedger(LedgerCloseData const& ledgerData,
 
     // Step 1. Maybe queue the current checkpoint file for publishing; this
     // should not race with main, since publish on main begins strictly _after_
-    // this call.
+    // this call. There is a bug in the upgrade path where the initial
+    // ledgerVers is used in some places during ledgerClose, and the upgraded
+    // ledgerVers is used in other places (see comment in ledgerClosed).
+    // On the ledger when an upgrade occurs, the ledger header will contain the
+    // newly incremented ledgerVers. Because the history checkpoint must be
+    // consistent with the ledger header, we must base checkpoints off the new
+    // ledgerVers here and not the initial ledgerVers.
     auto& hm = mApp.getHistoryManager();
-    hm.maybeQueueHistoryCheckpoint(ledgerSeq);
+    hm.maybeQueueHistoryCheckpoint(ledgerSeq, maybeNewVersion);
 
     // step 2
     ltx.commit();
@@ -1856,12 +1862,10 @@ LedgerManagerImpl::ledgerClosed(
     // there are two different assumptions in different parts of the
     // ledger-close path:
     //   - In closeLedger we mostly treat the ledger as being on vN, eg.
-    //   during
-    //     tx apply and LCM construction.
+    //     during tx apply and LCM construction.
     //   - In the final stage, when we call ledgerClosed, we pass vN+1
-    //   because
-    //     the upgrade completed and modified the ltx header, and we fish
-    //     the protocol out of the ltx header
+    //     because the upgrade completed and modified the ltx header, and we
+    //     fish the protocol out of the ltx header
     // Before LedgerCloseMetaV1, this inconsistency was mostly harmless
     // since LedgerCloseMeta was not modified after the LTX header was
     // modified. However, starting with protocol 20, LedgerCloseMeta is