Fix to e257a22:

* Fix issue where historical ledgers were acquired only since the last
online deletion interval instead of the configured value to allow deletion.

src/ripple/app/ledger/LedgerMaster.h

@@ -255,6 +255,10 @@ class LedgerMaster
         return !mValidLedger.empty();
     }
 
+    // Returns the minimum ledger sequence in SQL database, if any.
+    boost::optional<LedgerIndex> minSqlSeq();
+
+
 private:
     void setValidLedger(
         std::shared_ptr<Ledger const> const& l);

src/ripple/app/ledger/impl/LedgerMaster.cpp

@@ -50,6 +50,7 @@
 #include <cassert>
 #include <limits>
 #include <memory>
+#include <optional>
 #include <vector>
 
 namespace ripple {
@@ -142,25 +143,14 @@ static constexpr std::chrono::minutes MAX_LEDGER_AGE_ACQUIRE {1};
 // Don't acquire history if write load is too high
 static constexpr int MAX_WRITE_LOAD_ACQUIRE {8192};
 
-// Helper function for LedgerMaster::doAdvance()
-// Returns the minimum ledger sequence in SQL database, if any.
-static boost::optional<LedgerIndex>
-minSqlSeq(Application& app)
-{
-    boost::optional<LedgerIndex> seq;
-    auto db = app.getLedgerDB().checkoutDb();
-    *db << "SELECT MIN(LedgerSeq) FROM Ledgers", soci::into(seq);
-    return seq;
-}
-
 // Helper function for LedgerMaster::doAdvance()
 // Return true if candidateLedger should be fetched from the network.
 static bool
 shouldAcquire (
     std::uint32_t const currentLedger,
     std::uint32_t const ledgerHistory,
     boost::optional<LedgerIndex> minSeq,
-    std::uint32_t const lastRotated,
+    std::optional<LedgerIndex> const minimumOnline,
     std::uint32_t const candidateLedger,
     beast::Journal j)
 {
@@ -174,15 +164,13 @@ shouldAcquire (
         if (currentLedger - candidateLedger <= ledgerHistory)
             return true;
 
-        // Or it's greater than or equal to both:
-        // - the minimum persisted ledger or the maximum possible
-        //   sequence value, if no persisted ledger, and
-        // - minimum ledger that will be persisted as of the next online
-        //   deletion interval, or 1 if online deletion is disabled.
-        return
-            candidateLedger >= std::max(
-                minSeq.value_or(std::numeric_limits<LedgerIndex>::max()),
-                lastRotated + 1);
+        // Or if online deletion is enabled, whether greater than or equal to
+        // the minimum ledger to keep online.
+        if (minimumOnline.has_value())
+            return candidateLedger >= *minimumOnline;
+
+        // Or greater than or equal to the minimum ledger in SQLite, if any.
+        return minSeq.has_value() && candidateLedger >= *minSeq;
     }();
 
     JLOG (j.trace())
@@ -1903,8 +1891,8 @@ void LedgerMaster::doAdvance (std::unique_lock<std::recursive_mutex>& sl)
                         "tryAdvance discovered missing " << *missing;
                     if ((mFillInProgress == 0 || *missing > mFillInProgress) &&
                         shouldAcquire(mValidLedgerSeq, ledger_history_,
-                            minSqlSeq(app_),
-                            app_.getSHAMapStore().getLastRotated(), *missing,
+                            minSqlSeq(),
+                            app_.getSHAMapStore().minimumOnline(), *missing,
                             m_journal))
                     {
                         JLOG(m_journal.trace()) <<
@@ -2159,4 +2147,15 @@ LedgerMaster::getFetchPackCacheSize () const
     return fetch_packs_.getCacheSize ();
 }
 
+// Returns the minimum ledger sequence in SQL database, if any.
+boost::optional<LedgerIndex>
+LedgerMaster::minSqlSeq()
+{
+    boost::optional<LedgerIndex> seq;
+    auto db = app_.getLedgerDB().checkoutDb();
+    *db << "SELECT MIN(LedgerSeq) FROM Ledgers", soci::into(seq);
+    return seq;
+}
+
+
 } // ripple

src/ripple/app/misc/SHAMapStore.h

@@ -24,6 +24,8 @@
 #include <ripple/nodestore/Manager.h>
 #include <ripple/protocol/ErrorCodes.h>
 #include <ripple/core/Stoppable.h>
+#include <atomic>
+#include <optional>
 
 namespace ripple {
 
@@ -66,6 +68,10 @@ class SHAMapStore
 
     /** Returns the number of file descriptors that are needed. */
     virtual int fdRequired() const = 0;
+
+    /** If online deletion is enabled, return the minimum ledger
+     * to keep. */
+    virtual std::optional<LedgerIndex> const minimumOnline() const = 0;
 };
 
 //------------------------------------------------------------------------------

src/ripple/app/misc/SHAMapStoreImp.cpp

@@ -322,7 +322,7 @@ void
 SHAMapStoreImp::run()
 {
     beast::setCurrentThreadName ("SHAMapStore");
-    lastRotated_ = state_db_.getState().lastRotated;
+    LedgerIndex lastRotated = state_db_.getState().lastRotated;
     netOPs_ = &app_.getOPs();
     ledgerMaster_ = &app_.getLedgerMaster();
     fullBelowCache_ = &app_.family().fullbelow();
@@ -331,7 +331,20 @@ SHAMapStoreImp::run()
     ledgerDb_ = &app_.getLedgerDB();
 
     if (advisoryDelete_)
-        canDelete_ = state_db_.getCanDelete ();
+    {
+        canDelete_ = state_db_.getCanDelete();
+        // On startup, don't acquire ledgers from the network that
+        // can be deleted.
+        minimumOnline_ = canDelete_.load() + 1;
+    }
+    else
+    {
+        // On startup, don't acquire ledgers from the network older than
+        // the earliest persisted, if any.
+        auto const minSql = app_.getLedgerMaster().minSqlSeq();
+        if (minSql.has_value())
+            minimumOnline_ = *minSql;
+    }
 
     while (true)
     {
@@ -357,18 +370,18 @@ SHAMapStoreImp::run()
         }
 
         LedgerIndex const validatedSeq = validatedLedger->info().seq;
-        if (!lastRotated_)
+        if (!lastRotated)
         {
-            lastRotated_ = validatedSeq;
-            state_db_.setLastRotated (lastRotated_);
+            lastRotated = validatedSeq;
+            state_db_.setLastRotated (lastRotated);
         }
 
-        // will delete up to (not including) lastRotated_
-        if (validatedSeq >= lastRotated_ + deleteInterval_
-                && canDelete_ >= lastRotated_ - 1)
+        // will delete up to (not including) lastRotated
+        if (validatedSeq >= lastRotated + deleteInterval_
+                && canDelete_ >= lastRotated - 1)
         {
             JLOG(journal_.warn()) << "rotating  validatedSeq " << validatedSeq
-                << " lastRotated_ " << lastRotated_ << " deleteInterval "
+                << " lastRotated " << lastRotated << " deleteInterval "
                 << deleteInterval_ << " canDelete_ " << canDelete_;
 
             switch (health())
@@ -383,7 +396,7 @@ SHAMapStoreImp::run()
                     ;
             }
 
-            clearPrior (lastRotated_);
+            clearPrior (lastRotated);
             switch (health())
             {
                 case Health::stopping:
@@ -448,13 +461,13 @@ SHAMapStoreImp::run()
 
             std::string nextArchiveDir =
                 dbRotating_->getWritableBackend()->getName();
-            lastRotated_ = validatedSeq;
+            lastRotated = validatedSeq;
             std::shared_ptr<NodeStore::Backend> oldBackend;
             {
                 std::lock_guard lock (dbRotating_->peekMutex());
 
                 state_db_.setState (SavedState {newBackend->getName(),
-                                                nextArchiveDir, lastRotated_});
+                                                nextArchiveDir, lastRotated});
                 clearCaches (validatedSeq);
                 oldBackend = dbRotating_->rotateBackends(
                     std::move(newBackend),
@@ -598,7 +611,7 @@ SHAMapStoreImp::clearSql (DatabaseCon& database,
         min = *m;
     }
 
-    if(min > lastRotated || health() != Health::ok)
+    if (min > lastRotated || health() != Health::ok)
         return false;
 
     boost::format formattedDeleteQuery (deleteQuery);
@@ -646,6 +659,9 @@ SHAMapStoreImp::clearPrior (LedgerIndex lastRotated)
     if (health())
         return;
 
+    // Do not allow ledgers to be acquired from the network
+    // that are about to be deleted.
+    minimumOnline_ = lastRotated + 1;
     ledgerMaster_->clearPriorLedgers (lastRotated);
     if (health())
         return;

src/ripple/app/misc/SHAMapStoreImp.h

@@ -24,8 +24,6 @@
 #include <ripple/app/ledger/LedgerMaster.h>
 #include <ripple/core/DatabaseCon.h>
 #include <ripple/nodestore/DatabaseRotating.h>
-
-#include <atomic>
 #include <condition_variable>
 #include <thread>
 
@@ -85,13 +83,8 @@ class SHAMapStoreImp : public SHAMapStore
     static std::uint32_t const minimumDeletionInterval_ = 256;
     // minimum # of ledgers required for standalone mode.
     static std::uint32_t const minimumDeletionIntervalSA_ = 8;
-    // Ledger sequence at which the last deletion interval was triggered,
-    // or the current validated sequence as of first use
-    // if there have been no prior deletions. Deletion occurs up to (but
-    // not including) this value. All ledgers past this value are accumulated
-    // until the next online deletion. This value is persisted to SQLite
-    // nearly immediately after modification.
-    std::atomic<LedgerIndex> lastRotated_ {};
+    // minimum ledger to maintain online.
+    std::atomic<LedgerIndex> minimumOnline_ {};
 
     NodeStore::Scheduler& scheduler_;
     beast::Journal const journal_;
@@ -167,7 +160,7 @@ class SHAMapStoreImp : public SHAMapStore
     LedgerIndex
     getLastRotated() override
     {
-        return lastRotated_;
+        return state_db_.getState().lastRotated;
     }
 
     // All ledgers before and including this are unprotected
@@ -183,6 +176,14 @@ class SHAMapStoreImp : public SHAMapStore
     void rendezvous() const override;
     int fdRequired() const override;
 
+    std::optional<LedgerIndex> const
+    minimumOnline() const override
+    {
+        if (deleteInterval_)
+            return minimumOnline_;
+        return {};
+    }
+
 private:
     // callback for visitNodes
     bool copyNode (std::uint64_t& nodeCount, SHAMapAbstractNode const &node);