Tweak acquire logging, and "falling behind" flag logic

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

@@ -71,10 +71,10 @@ class InboundLedgersImp : public InboundLedgers
     {
         std::stringstream ss;
         ss << "InboundLedger::acquire: "
-           << "Request: " << to_string(hash) << ", " << std::to_string(seq)
+           << "Request: " << to_string(hash) << ", " << seq
            << " NeedNetworkLedger: "
            << (app_.getOPs().isNeedNetworkLedger() ? "yes" : "no")
-           << " Reason: " << to_string(reason) << " Old acquire: ";
+           << " Reason: " << to_string(reason) << " Old rule: ";
         if (app_.getOPs().isNeedNetworkLedger() &&
             (reason != InboundLedger::Reason::GENERIC) &&
             (reason != InboundLedger::Reason::CONSENSUS))
@@ -87,48 +87,6 @@ class InboundLedgersImp : public InboundLedgers
             reason != InboundLedger::Reason::SHARD ||
             (seq != 0 && app_.getShardStore()));
 
-        /*  Acquiring ledgers is somewhat expensive. It requires lots of
-         *  computation and network communication. Avoid it when it's not
-         *  appropriate. Every validation from a peer for a ledger that
-         *  we do not have locally results in a call to this function: even
-         *  if we are moments away from validating the same ledger.
-         *
-         *  When the following are all true, it is very likely that we will
-         *  soon validate the ledger ourselves. Therefore, avoid acquiring
-         *  ledgers from the network if:
-         *  + Our mode is "full". It is very likely that we will build
-         *    the ledger through the normal consensus process, and
-         *  + Our latest ledger is close to the most recently validated ledger.
-         *    Otherwise, we are likely falling behind the network because
-         *    we have been closing ledgers that have not been validated, and
-         *  + The requested ledger sequence is greater than our validated
-         *    ledger, but not far into the future. Otherwise, it is either a
-         *    request for an historical ledger or, if far into the future,
-         *    likely we're quite behind and will benefit from acquiring it
-         *    from the network.
-         */
-        bool const isFull = app_.getOPs().isFull();
-        bool const fallingBehind = app_.getOPs().isFallingBehind();
-        LedgerIndex const validSeq =
-            app_.getLedgerMaster().getValidLedgerIndex();
-        constexpr std::size_t lagLeeway = 20;
-        bool const nearFuture =
-            (seq > validSeq) && (seq < validSeq + lagLeeway);
-        bool const shouldAcquire = !(isFull && !fallingBehind && nearFuture);
-        ss << " Evaluating whether to acquire ledger " << hash
-           << ". full: " << (isFull ? "true" : "false")
-           << ". falling behind: " << (fallingBehind ? "true" : "false")
-           << ". ledger sequence " << seq << ". Valid sequence: " << validSeq
-           << ". Lag leeway: " << lagLeeway
-           << ". request for near future ledger: "
-           << (nearFuture ? "true" : "false") << ". Acquiring ledger? "
-           << (shouldAcquire ? "true" : "false");
-        if (!shouldAcquire)
-        {
-            JLOG(j_.debug()) << "Abort (rule): " << ss.str();
-            return {};
-        }
-
         bool isNew = true;
         std::shared_ptr<InboundLedger> inbound;
         {
@@ -171,7 +129,7 @@ class InboundLedgersImp : public InboundLedgers
 
         if (!inbound->isComplete())
         {
-            JLOG(j_.debug()) << "Abort (not complete): " << ss.str();
+            JLOG(j_.debug()) << "Abort (incomplete): " << ss.str();
             return {};
         }
 
@@ -195,6 +153,54 @@ class InboundLedgersImp : public InboundLedgers
             else
                 shardStore->storeLedger(inbound->getLedger());
         }
+
+        /*  Acquiring ledgers is somewhat expensive. It requires lots of
+         *  computation and network communication. Avoid it when it's not
+         *  appropriate. Every validation from a peer for a ledger that
+         *  we do not have locally results in a call to this function: even
+         *  if we are moments away from validating the same ledger.
+         *
+         *  When the following are all true, it is very likely that we will
+         *  soon validate the ledger ourselves. Therefore, avoid acquiring
+         *  ledgers from the network if:
+         *  + Our mode is "full". It is very likely that we will build
+         *    the ledger through the normal consensus process, and
+         *  + Our latest ledger is close to the most recently validated ledger.
+         *    Otherwise, we are likely falling behind the network because
+         *    we have been closing ledgers that have not been validated, and
+         *  + The requested ledger sequence is greater than our validated
+         *    ledger, but not far into the future. Otherwise, it is either a
+         *    request for an historical ledger or, if far into the future,
+         *    likely we're quite behind and will benefit from acquiring it
+         *    from the network.
+         */
+        bool const isFull = app_.getOPs().isFull();
+        bool const fallingBehind = app_.getOPs().isFallingBehind();
+        LedgerIndex const validSeq =
+            app_.getLedgerMaster().getValidLedgerIndex();
+        constexpr std::size_t lagLeeway = 20;
+        bool const nearFuture =
+            (seq > validSeq) && (seq < validSeq + lagLeeway);
+        bool const consensus = reason == InboundLedger::Reason::CONSENSUS;
+        bool const shouldAcquire =
+            !(isFull && !fallingBehind && (nearFuture || consensus));
+        ss << " Evaluating whether to acquire ledger " << hash
+           << ". full: " << (isFull ? "true" : "false")
+           << ". falling behind: " << (fallingBehind ? "true" : "false")
+           << ". ledger sequence " << seq << ". Valid sequence: " << validSeq
+           << ". Lag leeway: " << lagLeeway
+           << ". request for near future ledger: "
+           << (nearFuture ? "true" : "false")
+           << ". Consensus: " << (consensus ? "true" : "false")
+           << ". Acquiring ledger? " << (shouldAcquire ? "true" : "false");
+        if (!shouldAcquire)
+        {
+            // This check should be before the others because it's cheaper, but
+            // it's at the end for now to test the effectiveness of the change
+            JLOG(j_.debug()) << "Abort (rule): " << ss.str();
+            return {};
+        }
+
         JLOG(j_.debug()) << "Requesting: " << ss.str();
         return inbound->getLedger();
     }

src/ripple/app/misc/NetworkOPs.cpp

@@ -1816,22 +1816,20 @@ NetworkOPsImp::beginConsensus(uint256 const& networkClosed)
     JLOG(m_journal.info()) << "Consensus time for #" << closingInfo.seq
                            << " with LCL " << closingInfo.parentHash;
 
-    if (closingInfo.seq > m_ledgerMaster.getValidLedgerIndex() + 1)
+    fallingBehind_ = false;
+    if (closingInfo.seq < m_ledgerMaster.getValidLedgerIndex() - 1)
     {
         fallingBehind_ = true;
         JLOG(m_journal.warn()) << "Current ledger " << closingInfo.seq
-                               << "has advanced at least 2 past validated "
+                               << "is at least 2 behind validated "
                                << m_ledgerMaster.getValidLedgerIndex();
     }
-    else
-    {
-        fallingBehind_ = false;
-    }
 
     auto prevLedger = m_ledgerMaster.getLedgerByHash(closingInfo.parentHash);
 
     if (!prevLedger)
     {
+        fallingBehind_ = true;
         // this shouldn't happen unless we jump ledgers
         if (mMode == OperatingMode::FULL)
         {
@@ -1841,6 +1839,11 @@ NetworkOPsImp::beginConsensus(uint256 const& networkClosed)
 
         return false;
     }
+    else if (!prevLedger->info().validated)
+    {
+        JLOG(m_journal.warn()) << "Previous ledger is not validated";
+        fallingBehind_ = true;
+    }
 
     assert(prevLedger->info().hash == closingInfo.parentHash);
     assert(