Review comment, and handle case of online_delete not having

happened before.

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

@@ -149,8 +149,7 @@ static bool
 shouldAcquire (
     std::uint32_t const currentLedger,
     std::uint32_t const ledgerHistory,
-    boost::optional<LedgerIndex> minSeq,
-    std::optional<LedgerIndex> const minimumOnline,
+    boost::optional<LedgerIndex> const minimumOnline,
     std::uint32_t const candidateLedger,
     beast::Journal j)
 {
@@ -164,13 +163,8 @@ shouldAcquire (
         if (currentLedger - candidateLedger <= ledgerHistory)
             return true;
 
-        // 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;
+        // Or if greater than or equal to a specific minimum ledger.
+        return minimumOnline.has_value() && candidateLedger >= *minimumOnline;
     }();
 
     JLOG (j.trace())
@@ -1891,7 +1885,6 @@ 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_.getSHAMapStore().minimumOnline(), *missing,
                             m_journal))
                     {

src/ripple/app/misc/SHAMapStore.h

@@ -24,8 +24,8 @@
 #include <ripple/nodestore/Manager.h>
 #include <ripple/protocol/ErrorCodes.h>
 #include <ripple/core/Stoppable.h>
+#include <boost/optional.hpp>
 #include <atomic>
-#include <optional>
 
 namespace ripple {
 
@@ -70,8 +70,38 @@ class SHAMapStore
     virtual int fdRequired() const = 0;
 
     /** If online deletion is enabled, return the minimum ledger
-     * to keep. */
-    virtual std::optional<LedgerIndex> const minimumOnline() const = 0;
+     * to keep online, which is the lower bound for attempting to acquire
+     * historical ledgers over the peer to peer network.
+     * If online_delete is not enabled, then the minimum value to
+     * keep online is the minimum ledger that has been persisted already.
+     *
+     * With online_delete,this value is governed by the
+     * following circumstances:
+     *
+     * Upon process startup:
+     *
+     * With advisory_delete enabled and online_delete having been executed
+     * previously:
+     * Upon process startup, this value is set to one past the value
+     * that is allowed to be deleted. In other words, anything greater
+     * than what can be deleted should be acquired from the network and
+     * then retained. If nothing has yet been deleted, then do not
+     * rely upon the value of what is allowed to be deleted because it
+     * could cause fetching of more history than configured by
+     * ledger_history.
+     *
+     * Without advisory_delete or if online_delete has never executed:
+     * Upon process startup, this value is set to the earliest ledger
+     * that has been persisted in SQLite.
+     *
+     * Each time online_delete executes:
+     *
+     * 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.
+     */
+    virtual boost::optional<LedgerIndex> minimumOnline() const = 0;
 };
 
 //------------------------------------------------------------------------------

src/ripple/app/misc/SHAMapStoreImp.cpp

@@ -333,13 +333,15 @@ SHAMapStoreImp::run()
     if (advisoryDelete_)
     {
         canDelete_ = state_db_.getCanDelete();
-        // On startup, don't acquire ledgers from the network that
+
+        // On startup, don't acquire ledgers from the network that we know
         // can be deleted.
-        minimumOnline_ = canDelete_.load() + 1;
+        if (state_db_.getState().lastRotated)
+            minimumOnline_ = canDelete_.load() + 1;
     }
-    else
+    if (!minimumOnline_)
     {
-        // On startup, don't acquire ledgers from the network older than
+        // Or else don't acquire ledgers from the network older than
         // the earliest persisted, if any.
         auto const minSql = app_.getLedgerMaster().minSqlSeq();
         if (minSql.has_value())
@@ -747,6 +749,14 @@ SHAMapStoreImp::onChildrenStopped()
     }
 }
 
+boost::optional<LedgerIndex>
+SHAMapStoreImp::minimumOnline() const
+{
+    if (deleteInterval_)
+        return minimumOnline_.load();
+    return app_.getLedgerMaster().minSqlSeq();
+}
+
 //------------------------------------------------------------------------------
 
 std::unique_ptr<SHAMapStore>

src/ripple/app/misc/SHAMapStoreImp.h

@@ -176,13 +176,7 @@ class SHAMapStoreImp : public SHAMapStore
     void rendezvous() const override;
     int fdRequired() const override;
 
-    std::optional<LedgerIndex> const
-    minimumOnline() const override
-    {
-        if (deleteInterval_)
-            return minimumOnline_;
-        return {};
-    }
+    boost::optional<LedgerIndex> minimumOnline() const override;
 
 private:
     // callback for visitNodes