Validation timing fixes:

* Log whether consensus built a ledger we already had or were acquiring
* Don't trigger an acquire for a validation for a ledger we might be building
* When we finish building a ledger, try to accept that ledger
* If we cannot accept a built ledger, check held validations
* Correctly set isCurrent for untrusted validations
* Add appropriate logging

This fixes a race condition that could cause spurious and expensive
ledger fetches across the network and delayed recognition of
fully-validated ledger.

src/ripple_app/consensus/LedgerConsensus.cpp

@@ -929,7 +929,16 @@ private:
             }
 
             newLCL->setAccepted (closeTime, mCloseResolution, closeTimeCorrect);
-            getApp().getLedgerMaster().storeLedger(newLCL);
+
+            if (getApp().getLedgerMaster().storeLedger (newLCL))
+                WriteLog (lsDEBUG, LedgerConsensus)
+                    << "Consensus built ledger we already had";
+            else if (getApp().getInboundLedgers().find (newLCL->getHash()))
+                WriteLog (lsDEBUG, LedgerConsensus)
+                    << "Consensus built ledger we were acquiring";
+            else
+                WriteLog (lsDEBUG, LedgerConsensus)
+                    << "Consensus built new ledger";
 
             WriteLog (lsDEBUG, LedgerConsensus) 
                 << "Report: NewL  = " << newLCL->getHash () 
@@ -974,6 +983,9 @@ private:
                 WriteLog (lsINFO, LedgerConsensus) 
                     << "CNF newLCL " << newLCLHash;
 
+            // See if we can accept a ledger as fully-validated
+            getApp().getLedgerMaster().consensusBuilt (newLCL);
+
             Ledger::pointer newOL = boost::make_shared<Ledger> 
                 (true, boost::ref (*newLCL));
             LedgerMaster::ScopedLockType sl 
@@ -1485,6 +1497,9 @@ private:
         else
             initialSet = initialLedger.peekTransactionMap ()->snapShot (false);
 
+        // Tell the ledger master not to acquire the ledger we're probably building
+        getApp().getLedgerMaster().setBuildingLedger (mPreviousLedger->getLedgerSeq () + 1);
+
         uint256 txSet = initialSet->getHash ();
         WriteLog (lsINFO, LedgerConsensus) << "initial position " << txSet;
         mapComplete (txSet, initialSet, false);

src/ripple_app/ledger/LedgerHistory.cpp

@@ -39,16 +39,18 @@ LedgerHistory::LedgerHistory ()
 {
 }
 
-void LedgerHistory::addLedger (Ledger::pointer ledger, bool validated)
+bool LedgerHistory::addLedger (Ledger::pointer ledger, bool validated)
 {
     assert (ledger && ledger->isImmutable ());
     assert (ledger->peekAccountStateMap ()->getHash ().isNonZero ());
 
     LedgersByHash::ScopedLockType sl (m_ledgers_by_hash.peekMutex ());
 
-    m_ledgers_by_hash.canonicalize (ledger->getHash(), ledger, true);
+    const bool alreadyHad = m_ledgers_by_hash.canonicalize (ledger->getHash(), ledger, true);
     if (validated)
         mLedgersByIndex[ledger->getLedgerSeq()] = ledger->getHash();
+
+    return alreadyHad;
 }
 
 uint256 LedgerHistory::getLedgerHash (std::uint32_t index)

src/ripple_app/ledger/LedgerHistory.h

@@ -28,7 +28,7 @@ class LedgerHistory : beast::LeakChecked <LedgerHistory>
 public:
     LedgerHistory ();
 
-    void addLedger (Ledger::pointer ledger, bool validated);
+    bool addLedger (Ledger::pointer ledger, bool validated);
 
     float getCacheHitRate ()
     {

src/ripple_app/ledger/LedgerMaster.cpp

@@ -78,6 +78,7 @@ public:
     std::atomic <std::uint32_t> mPubLedgerSeq;
     std::atomic <std::uint32_t> mValidLedgerClose;
     std::atomic <std::uint32_t> mValidLedgerSeq;
+    std::atomic <std::uint32_t> mBuildingLedgerSeq;
 
     //--------------------------------------------------------------------------
 
@@ -97,6 +98,7 @@ public:
         , mPubLedgerSeq (0)
         , mValidLedgerClose (0)
         , mValidLedgerSeq (0)
+        , mBuildingLedgerSeq (0)
     {
     }
 
@@ -271,9 +273,10 @@ public:
         return mLedgerHistory.fixIndex (ledgerIndex, ledgerHash);
     }
 
-    void storeLedger (Ledger::pointer ledger)
+    bool storeLedger (Ledger::pointer ledger)
     {
-        mLedgerHistory.addLedger (ledger, false);
+        // Returns true if we already had the ledger
+        return mLedgerHistory.addLedger (ledger, false);
     }
 
     void forceValid (Ledger::pointer ledger)
@@ -328,6 +331,17 @@ public:
         mEngine.setLedger (mCurrentLedger.getMutable ());
     }
 
+    LedgerIndex getBuildingLedger ()
+    {
+        // The ledger we are currently building, 0 of none
+        return mBuildingLedgerSeq.load ();
+    }
+
+    void setBuildingLedger (LedgerIndex i)
+    {
+        mBuildingLedgerSeq.store (i);
+    }
+
     TER doTransaction (SerializedTransaction::ref txn, TransactionEngineParams params, bool& didApply)
     {
         Ledger::pointer ledger;
@@ -641,10 +655,20 @@ public:
         getApp().getInboundLedgers().findCreate(hash, seq, InboundLedger::fcGENERIC);
     }
 
+    // Check if the specified ledger can become the new last fully-validated ledger
     void checkAccept (uint256 const& hash, std::uint32_t seq)
     {
-        if ((seq == 0) && (seq <= mValidLedgerSeq))
-            return;
+
+        if (seq != 0)
+        {
+            // Ledger is too old
+            if (seq <= mValidLedgerSeq)
+                return;
+
+            // Ledger could match the ledger we're already building
+	    if (seq == mBuildingLedgerSeq)
+	        return;
+	}
 
         Ledger::pointer ledger = mLedgerHistory.getLedgerByHash (hash);
 
@@ -667,16 +691,15 @@ public:
             checkAccept (ledger);
     }
 
-    void checkAccept (Ledger::ref ledger)
+    /**
+     * Determines how many validations are needed to fully-validated a ledger
+     *
+     * @return Number of validations needed
+     */
+    int getNeededValidations ()
     {
-        if (ledger->getLedgerSeq() <= mValidLedgerSeq)
-            return;
-
-        // Can we advance the last fully-validated ledger? If so, can we publish?
-        ScopedLockType ml (m_mutex);
-
-        if (ledger->getLedgerSeq() <= mValidLedgerSeq)
-            return;
+        if (getConfig ().RUN_STANDALONE)
+            return 0;
 
         int minVal = mMinValidations;
 
@@ -690,9 +713,21 @@ public:
                 minVal = val;
         }
 
-        if (getConfig ().RUN_STANDALONE)
-            minVal = 0;
+        return minVal;
+    }
 
+    void checkAccept (Ledger::ref ledger)
+    {
+        if (ledger->getLedgerSeq() <= mValidLedgerSeq)
+            return;
+
+        // Can we advance the last fully-validated ledger? If so, can we publish?
+        ScopedLockType ml (m_mutex);
+
+        if (ledger->getLedgerSeq() <= mValidLedgerSeq)
+            return;
+
+        int minVal = getNeededValidations();
         int tvc = getApp().getValidations().getTrustedValidationCount(ledger->getHash());
         if (tvc < minVal) // nothing we can do
         {
@@ -728,6 +763,96 @@ public:
         tryAdvance ();
     }
 
+    /** Report that the consensus process built a particular ledger */
+    void consensusBuilt (Ledger::ref ledger) override
+    {
+ 
+        // Because we just built a ledger, we are no longer building one
+        setBuildingLedger (0);
+
+        if (ledger->getLedgerSeq() <= mValidLedgerSeq)
+        {
+            WriteLog (lsINFO,  LedgerConsensus)
+               << "Consensus built old ledger: "
+               << ledger->getLedgerSeq() << " <= " << mValidLedgerSeq;
+            return;
+        }
+
+        // See if this ledger can be the new fully-validated ledger
+        checkAccept (ledger);
+
+        if (ledger->getLedgerSeq() <= mValidLedgerSeq)
+        {
+            WriteLog (lsDEBUG, LedgerConsensus)
+                << "Consensus ledger fully validated";
+            return;
+        }
+
+        // This ledger cannot be the new fully-validated ledger, but
+        // maybe we saved up validations for some other ledger that can be
+
+        auto const val = getApp().getValidations().getCurrentTrustedValidations();
+
+        // Track validation counts with sequence numbers
+        class valSeq
+        {
+            public:
+
+            valSeq () : valCount_ (0), ledgerSeq_ (0) { ; }
+
+            void mergeValidation (LedgerSeq seq)
+            {
+                valCount_++;
+
+                // If we didn't already know the sequence, now we do
+                if (ledgerSeq_ == 0)
+                    ledgerSeq_ = seq;
+            }
+
+            int valCount_;
+            LedgerSeq ledgerSeq_;
+        };
+
+        // Count the number of current, trusted validations
+        ripple::unordered_map <uint256, valSeq> count;
+        for (auto const& v : val)
+        {
+            valSeq& vs = count[v->getLedgerHash()];
+            vs.mergeValidation (v->getFieldU32 (sfLedgerSequence));
+        }
+
+        int neededValidations = getNeededValidations ();
+        LedgerSeq maxSeq = mValidLedgerSeq;
+        uint256 maxLedger = ledger->getHash();
+
+        // Of the ledgers with sufficient validations,
+        // find the one with the highest sequence
+        for (auto& v : count)
+            if (v.second.valCount_ > neededValidations)
+            {
+                // If we still don't know the sequence, get it
+                if (v.second.ledgerSeq_ == 0)
+                {
+                    Ledger::pointer ledger = getLedgerByHash (v.first);
+                    if (ledger)
+                        v.second.ledgerSeq_ = ledger->getLedgerSeq();
+                }
+
+                if (v.second.ledgerSeq_ > maxSeq)
+                {
+                    maxSeq = v.second.ledgerSeq_;
+                    maxLedger = v.first;
+                }
+            }
+
+        if (maxSeq > mValidLedgerSeq)
+        {
+            WriteLog (lsDEBUG, LedgerConsensus)
+                << "Consensus triggered check of ledger";
+            checkAccept (maxLedger, maxSeq);
+        }
+    }
+
     void advanceThread()
     {
         ScopedLockType sl (m_mutex);

src/ripple_app/ledger/LedgerMaster.h

@@ -80,7 +80,7 @@ public:
 
     virtual void pushLedger (Ledger::pointer newLedger) = 0;
     virtual void pushLedger (Ledger::pointer newLCL, Ledger::pointer newOL) = 0;
-    virtual void storeLedger (Ledger::pointer) = 0;
+    virtual bool storeLedger (Ledger::pointer) = 0;
     virtual void forceValid (Ledger::pointer) = 0;
 
     virtual void setFullLedger (Ledger::pointer ledger, bool isSynchronous, bool isCurrent) = 0;
@@ -128,6 +128,10 @@ public:
 
     virtual void checkAccept (Ledger::ref ledger) = 0;
     virtual void checkAccept (uint256 const& hash, std::uint32_t seq) = 0;
+    virtual void consensusBuilt (Ledger::ref ledger) = 0;
+
+    virtual LedgerIndex getBuildingLedger () = 0;
+    virtual void setBuildingLedger (LedgerIndex index) = 0;
 
     virtual void tryAdvance () = 0;
     virtual void newPathRequest () = 0;

src/ripple_app/misc/NetworkOPs.cpp

@@ -502,7 +502,7 @@ private:
     SubMapType                                          mSubTransactions;       // all accepted transactions
     SubMapType                                          mSubRTTransactions;     // all proposed and accepted transactions
 
-    TaggedCache< uint256, Blob>                     mFetchPack;
+    TaggedCache< uint256, Blob>                         mFetchPack;
     std::uint32_t                                       mFetchSeq;
 
     std::uint32_t                                       mLastLoadBase;

src/ripple_app/misc/Validations.cpp

@@ -74,20 +74,18 @@ private:
         RippleAddress signer = val->getSignerPublic ();
         bool isCurrent = false;
 
-        if (val->isTrusted () || getApp().getUNL ().nodeInUNL (signer))
-        {
-            val->setTrusted ();
-            std::uint32_t now = getApp().getOPs ().getCloseTimeNC ();
-            std::uint32_t valClose = val->getSignTime ();
+        if (!val->isTrusted() && getApp().getUNL().nodeInUNL (signer))
+            val->setTrusted();
 
-            if ((now > (valClose - LEDGER_EARLY_INTERVAL)) && (now < (valClose + LEDGER_VAL_INTERVAL)))
-                isCurrent = true;
-            else
-            {
-                WriteLog (lsWARNING, Validations) << "Received stale validation now=" << now << ", close=" << valClose;
-            }
-        }
+        std::uint32_t now = getApp().getOPs().getCloseTimeNC();
+        std::uint32_t valClose = val->getSignTime();
+
+        if ((now > (valClose - LEDGER_EARLY_INTERVAL)) && (now < (valClose + LEDGER_VAL_INTERVAL)))
+            isCurrent = true;
         else
+            WriteLog (lsWARNING, Validations) << "Received stale validation now=" << now << ", close=" << valClose;
+
+        if (!val->isTrusted ())
         {
             WriteLog (lsDEBUG, Validations) << "Node " << signer.humanNodePublic () << " not in UNL st=" << val->getSignTime () <<
                                             ", hash=" << val->getLedgerHash () << ", shash=" << val->getSigningHash () << " src=" << source;
@@ -96,29 +94,37 @@ private:
         uint256 hash = val->getLedgerHash ();
         uint160 node = signer.getNodeID ();
 
+        if (val->isTrusted () && isCurrent)
         {
             ScopedLockType sl (mLock);
 
             if (!findCreateSet (hash)->insert (std::make_pair (node, val)).second)
                 return false;
 
-            if (isCurrent)
-            {
-                ripple::unordered_map<uint160, SerializedValidation::pointer>::iterator it = mCurrentValidations.find (node);
+            auto it = mCurrentValidations.find (node);
 
-                if (it == mCurrentValidations.end ())
-                    mCurrentValidations.emplace (node, val);
-                else if (!it->second)
-                    it->second = val;
-                else if (val->getSignTime () > it->second->getSignTime ())
-                {
-                    val->setPreviousHash (it->second->getLedgerHash ());
-                    mStaleValidations.push_back (it->second);
-                    it->second = val;
-                    condWrite ();
-                }
-                else
-                    isCurrent = false;
+            if (it == mCurrentValidations.end ())
+            {
+                // No previous validation from this validator
+                mCurrentValidations.emplace (node, val);
+            }
+            else if (!it->second)
+            {
+                // Previous validation has expired
+                it->second = val;
+            }
+            else if (val->getSignTime () > it->second->getSignTime ())
+            {
+                // This is a newer validation
+                val->setPreviousHash (it->second->getLedgerHash ());
+                mStaleValidations.push_back (it->second);
+                it->second = val;
+                condWrite ();
+            }
+            else
+            {
+                // We already have a newer validation from this source
+                isCurrent = false;
             }
         }
 
@@ -126,10 +132,13 @@ private:
                                         << " added " << (val->isTrusted () ? "trusted/" : "UNtrusted/") << (isCurrent ? "current" : "stale");
 
         if (val->isTrusted () && isCurrent)
+        {
             getApp().getLedgerMaster ().checkAccept (hash, val->getFieldU32 (sfLedgerSequence));
+            return true;
+        }
 
         // FIXME: This never forwards untrusted validations
-        return isCurrent;
+        return false;
     }
 
     void tune (int size, int age)
@@ -246,7 +255,7 @@ private:
                         fee += it.second->getFieldU32(sfLoadFee);
                     else
                         fee += ref;
-		}
+                 }
             }
         }