DRAFT: Optimize when to acquire ledgers from the network

src/test/app/LedgerReplay_test.cpp

@@ -75,8 +75,11 @@ class MagicInboundLedgers : public InboundLedgers
     virtual ~MagicInboundLedgers() = default;
 
     virtual std::shared_ptr<Ledger const>
-    acquire(uint256 const& hash, std::uint32_t seq, InboundLedger::Reason)
-        override
+    acquire(
+        uint256 const& hash,
+        std::uint32_t seq,
+        InboundLedger::Reason,
+        char const*) override
     {
         if (bhvr == InboundLedgersBehavior::DropAll)
             return {};

src/xrpld/app/consensus/RCLValidations.cpp

@@ -127,7 +127,7 @@ RCLValidationsAdaptor::acquire(LedgerHash const& hash)
                 JLOG(j_.debug())
                     << "JOB advanceLedger getConsensusLedger2 started";
                 pApp->getInboundLedgers().acquireAsync(
-                    hash, 0, InboundLedger::Reason::CONSENSUS);
+                    hash, 0, InboundLedger::Reason::PREFERRED);
             });
         return std::nullopt;
     }

src/xrpld/app/ledger/InboundLedger.h

@@ -24,9 +24,10 @@ class InboundLedger final : public TimeoutCounter,
 
     // These are the reasons we might acquire a ledger
     enum class Reason {
-        HISTORY,   // Acquiring past ledger
-        GENERIC,   // Generic other reasons
-        CONSENSUS  // We believe the consensus round requires this ledger
+        HISTORY,    // Acquiring past ledger
+        GENERIC,    // Generic other reasons
+        CONSENSUS,  // We believe the consensus round requires this ledger
+        PREFERRED   // We need this ledger for preferred ledger analysis
     };
 
     InboundLedger(
@@ -40,8 +41,10 @@ class InboundLedger final : public TimeoutCounter,
     ~InboundLedger();
 
     // Called when another attempt is made to fetch this same ledger
-    void
-    update(std::uint32_t seq);
+    //
+    // Returns true if this triggers requests to be sent
+    bool
+    update(std::uint32_t seq, bool broadcast);
 
     /** Returns true if we got all the data. */
     bool
@@ -72,7 +75,7 @@ class InboundLedger final : public TimeoutCounter,
     bool
     checkLocal();
     void
-    init(ScopedLockType& collectionLock);
+    init(ScopedLockType& collectionLock, bool broadcast);
 
     bool
     gotData(
@@ -179,24 +182,8 @@ class InboundLedger final : public TimeoutCounter,
     std::unique_ptr<PeerSet> mPeerSet;
 };
 
-inline std::string
-to_string(InboundLedger::Reason reason)
-{
-    using enum InboundLedger::Reason;
-    switch (reason)
-    {
-        case HISTORY:
-            return "HISTORY";
-        case GENERIC:
-            return "GENERIC";
-        case CONSENSUS:
-            return "CONSENSUS";
-        default:
-            UNREACHABLE(
-                "ripple::to_string(InboundLedger::Reason) : unknown value");
-            return "unknown";
-    }
-}
+std::string
+to_string(InboundLedger::Reason reason);
 
 }  // namespace ripple
 

src/xrpld/app/ledger/InboundLedgers.h

@@ -21,7 +21,11 @@ class InboundLedgers
     // Callers should use this if they possibly need an authoritative
     // response immediately.
     virtual std::shared_ptr<Ledger const>
-    acquire(uint256 const& hash, std::uint32_t seq, InboundLedger::Reason) = 0;
+    acquire(
+        uint256 const& hash,
+        std::uint32_t seq,
+        InboundLedger::Reason,
+        char const* context) = 0;
 
     // Callers should use this if they are known to be executing on the Job
     // Queue. TODO review whether all callers of acquire() can use this

src/xrpld/app/ledger/detail/InboundLedger.cpp

@@ -54,6 +54,27 @@ enum {
 // millisecond for each ledger timeout
 auto constexpr ledgerAcquireTimeout = 3000ms;
 
+std::string
+to_string(InboundLedger::Reason reason)
+{
+    using enum InboundLedger::Reason;
+    switch (reason)
+    {
+        case HISTORY:
+            return "HISTORY";
+        case GENERIC:
+            return "GENERIC";
+        case CONSENSUS:
+            return "CONSENSUS";
+        case PREFERRED:
+            return "PREFERRED";
+        default:
+            UNREACHABLE(
+                "ripple::to_string(InboundLedger::Reason) : unknown value");
+            return "unknown";
+    }
+}
+
 InboundLedger::InboundLedger(
     Application& app,
     uint256 const& hash,
@@ -83,7 +104,7 @@ InboundLedger::InboundLedger(
 }
 
 void
-InboundLedger::init(ScopedLockType& collectionLock)
+InboundLedger::init(ScopedLockType& collectionLock, bool broadcast)
 {
     ScopedLockType sl(mtx_);
     collectionLock.unlock();
@@ -94,8 +115,18 @@ InboundLedger::init(ScopedLockType& collectionLock)
 
     if (!complete_)
     {
-        addPeers();
-        queueJob(sl);
+        if (broadcast)
+        {
+            addPeers();
+            queueJob(sl);
+        }
+        else
+        {
+            // Delay to give time to build the ledger before sending
+            JLOG(journal_.debug()) << "init: Deferring peer requests";
+            deferred_ = true;
+            setTimer(sl);
+        }
         return;
     }
 
@@ -113,7 +144,7 @@ InboundLedger::init(ScopedLockType& collectionLock)
     app_.getLedgerMaster().storeLedger(mLedger);
 
     // Check if this could be a newer fully-validated ledger
-    if (mReason == Reason::CONSENSUS)
+    if (mReason >= Reason::CONSENSUS)
         app_.getLedgerMaster().checkAccept(mLedger);
 }
 
@@ -126,8 +157,8 @@ InboundLedger::getPeerCount() const
     });
 }
 
-void
-InboundLedger::update(std::uint32_t seq)
+bool
+InboundLedger::update(std::uint32_t seq, bool broadcast)
 {
     ScopedLockType sl(mtx_);
 
@@ -137,6 +168,27 @@ InboundLedger::update(std::uint32_t seq)
 
     // Prevent this from being swept
     touch();
+
+    // If the signal is to broadcast, and this request has never tried to
+    // broadcast before, cancel any waiting timer, then fire off the job to
+    // broadcast. Note that this is calling mPeerSet->getPeerIds(), not
+    // getPeerCount(), because the latter will filter out peers that have been
+    // tried, but are since lost. This wants to check if peers have _ever_ been
+    // tried. If they have, stick with the normal timer flow.
+    if (broadcast && mPeerSet->getPeerIds().empty())
+    {
+        if (cancelTimer(sl))
+        {
+            JLOG(journal_.debug())
+                << "update: cancelling timer to send peer requests";
+            deferred_ = false;
+            skipNext_ = true;
+            addPeers();
+            queueJob(sl);
+            return true;
+        }
+    }
+    return false;
 }
 
 bool

src/xrpld/app/ledger/detail/InboundLedgers.cpp

@@ -51,7 +51,8 @@ class InboundLedgersImp : public InboundLedgers
     acquire(
         uint256 const& hash,
         std::uint32_t seq,
-        InboundLedger::Reason reason) override
+        InboundLedger::Reason reason,
+        char const* context) override
     {
         auto doAcquire = [&, seq, reason]() -> std::shared_ptr<Ledger const> {
             XRPL_ASSERT(
@@ -64,7 +65,7 @@ class InboundLedgersImp : public InboundLedgers
                     return true;
                 if (reason == InboundLedger::Reason::GENERIC)
                     return true;
-                if (reason == InboundLedger::Reason::CONSENSUS)
+                if (reason >= InboundLedger::Reason::CONSENSUS)
                     return true;
                 return false;
             }();
@@ -73,7 +74,7 @@ class InboundLedgersImp : public InboundLedgers
             ss << "InboundLedger::acquire: "
                << "Request: " << to_string(hash) << ", " << seq
                << " NeedNetworkLedger: " << (needNetworkLedger ? "yes" : "no")
-               << " Reason: " << to_string(reason)
+               << " Reason: " << to_string(reason) << " Context: " << context
                << " Should acquire: " << (shouldAcquire ? "true." : "false.");
 
             /*  Acquiring ledgers is somewhat expensive. It requires lots of
@@ -87,20 +88,34 @@ class InboundLedgersImp : public InboundLedgers
                 // the network, and doesn't have the necessary tx's and
                 // ledger entries to build the ledger.
                 bool const isFull = app_.getOPs().isFull();
+                // fallingBehind means the last closed ledger is at least 2
+                // behind the validated ledger. If the node is falling
+                // behind the network, it probably needs information from
+                // the network to catch up.
+                //
+                // The reason this should not simply be only at least 1
+                // behind the validated ledger is that a slight lag is
+                // normal case because some nodes get there slightly later
+                // than others. A difference of 2 means that at least a full
+                // ledger interval has passed, so the node is beginning to
+                // fall behind.
+                bool const fallingBehind = app_.getOPs().isFallingBehind();
+                // If the ledger is needed for preferred ledger analysis and we
+                // don't have it, chances are we're not going to build it,
+                // because someone else has built it, so download it.
+                bool const preferred =
+                    reason == InboundLedger::Reason::PREFERRED;
                 // If everything else is ok, don't try to acquire the ledger
                 // if the requested seq is in the near future relative to
-                // the validated ledger. If the requested ledger is between
-                // 1 and 19 inclusive ledgers ahead of the valid ledger this
-                // node has not built it yet, but it's possible/likely it
-                // has the tx's necessary to build it and get caught up.
-                // Plus it might not become validated. On the other hand, if
-                // it's more than 20 in the future, this node should request
-                // it so that it can jump ahead and get caught up.
+                // the validated ledger. Because validations lag behind
+                // consensus, if we get any further behind than this, we
+                // risk losing sync, because we don't have the preferred
+                // ledger available.
                 LedgerIndex const validSeq =
                     app_.getLedgerMaster().getValidLedgerIndex();
-                constexpr std::size_t lagLeeway = 20;
-                bool const nearFuture =
-                    (seq > validSeq) && (seq < validSeq + lagLeeway);
+                constexpr std::size_t lagLeeway = 2;
+                bool const nearFuture = (validSeq > 0) && (seq > validSeq) &&
+                    (seq < validSeq + lagLeeway);
                 // If everything else is ok, don't try to acquire the ledger
                 // if the request is related to consensus. (Note that
                 // consensus calls usually pass a seq of 0, so nearFuture
@@ -109,8 +124,10 @@ class InboundLedgersImp : public InboundLedgers
                     reason == InboundLedger::Reason::CONSENSUS;
                 ss << " Evaluating whether to broadcast requests to peers"
                    << ". full: " << (isFull ? "true" : "false")
-                   << ". ledger sequence " << seq
-                   << ". Valid sequence: " << validSeq
+                   << ". falling behind: " << (fallingBehind ? "true" : "false")
+                   << ". needed for preferred ledger analysis: "
+                   << (preferred ? "true" : "false") << ". ledger sequence "
+                   << seq << ". Valid sequence: " << validSeq
                    << ". Lag leeway: " << lagLeeway
                    << ". request for near future ledger: "
                    << (nearFuture ? "true" : "false")
@@ -119,6 +136,12 @@ class InboundLedgersImp : public InboundLedgers
                 // If the node is not synced, send requests.
                 if (!isFull)
                     return true;
+                // If the node is falling behind, send requests.
+                if (fallingBehind)
+                    return true;
+                // If needed for preferred analysis, send requests.
+                if (preferred)
+                    return true;
                 // If the ledger is in the near future, do NOT send requests.
                 // This node is probably about to build it.
                 if (nearFuture)
@@ -129,7 +152,7 @@ class InboundLedgersImp : public InboundLedgers
                     return false;
                 return true;
             }();
-            ss << ". Would broadcast to peers? "
+            ss << ". Broadcast to peers? "
                << (shouldBroadcast ? "true." : "false.");
 
             if (!shouldAcquire)
@@ -164,7 +187,7 @@ class InboundLedgersImp : public InboundLedgers
                         std::ref(m_clock),
                         mPeerSetBuilder->build());
                     mLedgers.emplace(hash, inbound);
-                    inbound->init(sl);
+                    inbound->init(sl, shouldBroadcast);
                     ++mCounter;
                 }
             }
@@ -176,8 +199,12 @@ class InboundLedgersImp : public InboundLedgers
                 return {};
             }
 
-            if (!isNew)
-                inbound->update(seq);
+            bool const didBroadcast = [&]() {
+                if (!isNew)
+                    return inbound->update(seq, shouldBroadcast);
+                return shouldBroadcast;
+            }();
+            ss << " First broadcast: " << (didBroadcast ? "true" : "false");
 
             if (!inbound->isComplete())
             {
@@ -203,7 +230,7 @@ class InboundLedgersImp : public InboundLedgers
         {
             try
             {
-                acquire(hash, seq, reason);
+                acquire(hash, seq, reason, "acquireAsync");
             }
             catch (std::exception const& e)
             {