Fix historical ledger acquisition

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_;
     }