Fix historical ledger acquisition when using online deletion:

Commit e257a22 introduced changes in the logic used to acquire historical
ledgers. The logic could cause historical ledgers to be acquired only since
the last online deletion interval instead of the configured value to allow
deletion.

src/ripple/app/ledger/LedgerMaster.h

@@ -286,6 +286,10 @@ class LedgerMaster : public Stoppable, public AbstractFetchPackContainer
         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

@@ -143,29 +143,17 @@ 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,
+    boost::optional<LedgerIndex> const minimumOnline,
     std::uint32_t const candidateLedger,
     beast::Journal j)
 {
-    bool ret = [&]() {
+    bool const ret = [&]() {
         // Fetch ledger if it may be the current ledger
         if (candidateLedger >= currentLedger)
             return true;
@@ -174,15 +162,9 @@ 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 greater than or equal to a specific minimum ledger.
+        // Do nothing if the minimum ledger to keep online is unknown.
+        return minimumOnline.has_value() && candidateLedger >= *minimumOnline;
     }();
 
     JLOG(j.trace()) << "Missing ledger " << candidateLedger
@@ -1899,8 +1881,7 @@ LedgerMaster::doAdvance(std::unique_lock<std::recursive_mutex>& sl)
                         shouldAcquire(
                             mValidLedgerSeq,
                             ledger_history_,
-                            minSqlSeq(app_),
-                            app_.getSHAMapStore().getLastRotated(),
+                            app_.getSHAMapStore().minimumOnline(),
                             *missing,
                             m_journal))
                     {
@@ -2149,4 +2130,14 @@ 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;
+}
+
 }  // namespace ripple

src/ripple/app/misc/SHAMapStore.h

@@ -24,6 +24,7 @@
 #include <ripple/core/Stoppable.h>
 #include <ripple/nodestore/Manager.h>
 #include <ripple/protocol/ErrorCodes.h>
+#include <boost/optional.hpp>
 
 namespace ripple {
 
@@ -74,6 +75,25 @@ class SHAMapStore : public Stoppable
     /** Returns the number of file descriptors that are needed. */
     virtual int
     fdRequired() const = 0;
+
+    /** The minimum ledger to try and maintain in our database.
+
+        This defines the lower bound for attempting to acquire historical
+        ledgers over the peer to peer network.
+
+        If online_delete is enabled, then each time online_delete executes
+        and just prior to clearing SQL databases of historical ledgers,
+        move the value forward to one past the greatest ledger being deleted.
+        This minimizes fetching of ledgers that are in the process of being
+        deleted. Without online_delete or before online_delete is
+        executed, this value is always the minimum value persisted in the
+        ledger database, if any.
+
+        @return The minimum ledger sequence to keep online based on the
+            description above. If not set, then an unseated optional.
+    */
+    virtual boost::optional<LedgerIndex>
+    minimumOnline() const = 0;
 };
 
 //------------------------------------------------------------------------------

src/ripple/app/misc/SHAMapStoreImp.cpp

@@ -305,7 +305,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();
@@ -340,19 +340,19 @@ 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 " << deleteInterval_
+                << "rotating  validatedSeq " << validatedSeq << " lastRotated "
+                << lastRotated << " deleteInterval " << deleteInterval_
                 << " canDelete_ " << canDelete_;
 
             switch (health())
@@ -366,7 +366,7 @@ SHAMapStoreImp::run()
                 default:;
             }
 
-            clearPrior(lastRotated_);
+            clearPrior(lastRotated);
             switch (health())
             {
                 case Health::stopping:
@@ -426,14 +426,14 @@ SHAMapStoreImp::run()
                 default:;
             }
 
-            lastRotated_ = validatedSeq;
+            lastRotated = validatedSeq;
 
             dbRotating_->rotateWithLock(
                 [&](std::string const& writableBackendName) {
                     SavedState savedState;
                     savedState.writableDb = newBackend->getName();
                     savedState.archiveDb = writableBackendName;
-                    savedState.lastRotated = lastRotated_;
+                    savedState.lastRotated = lastRotated;
                     state_db_.setState(savedState);
 
                     clearCaches(validatedSeq);
@@ -624,6 +624,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;
@@ -715,6 +718,16 @@ SHAMapStoreImp::onChildrenStopped()
     }
 }
 
+boost::optional<LedgerIndex>
+SHAMapStoreImp::minimumOnline() const
+{
+    // minimumOnline_ with 0 value is equivalent to unknown/not set.
+    // Don't attempt to acquire ledgers if that value is unknown.
+    if (deleteInterval_ && minimumOnline_)
+        return minimumOnline_.load();
+    return app_.getLedgerMaster().minSqlSeq();
+}
+
 //------------------------------------------------------------------------------
 
 std::unique_ptr<SHAMapStore>

src/ripple/app/misc/SHAMapStoreImp.h

@@ -24,7 +24,6 @@
 #include <ripple/app/misc/SHAMapStore.h>
 #include <ripple/core/DatabaseCon.h>
 #include <ripple/nodestore/DatabaseRotating.h>
-
 #include <atomic>
 #include <condition_variable>
 #include <thread>
@@ -86,13 +85,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_;
@@ -168,7 +162,7 @@ class SHAMapStoreImp : public SHAMapStore
     LedgerIndex
     getLastRotated() override
     {
-        return lastRotated_;
+        return state_db_.getState().lastRotated;
     }
 
     // All ledgers before and including this are unprotected
@@ -187,6 +181,9 @@ class SHAMapStoreImp : public SHAMapStore
     int
     fdRequired() const override;
 
+    boost::optional<LedgerIndex>
+    minimumOnline() const override;
+
 private:
     // callback for visitNodes
     bool

src/test/unit_test/SuiteJournal.h

@@ -98,7 +98,11 @@ class SuiteJournal
         : sink_(partition, threshold, suite), journal_(sink_)
     {
     }
+    // Clang 10.0.0 and 10.0.1 disagree about formatting operator&
+    // TBD Re-enable formatting when we upgrade to clang 11
+    // clang-format off
     operator beast::Journal &()
+    // clang-format on
     {
         return journal_;
     }