Use ledger hash to break ties (RIPD-1468):

When two ledgers have the same number of validations, the code
will now use the ledger hash itself to break the tie rather than
the highest node ID supporting each validation.

src/ripple/app/consensus/RCLConsensus.cpp

@@ -242,21 +242,21 @@ RCLConsensus::Adaptor::getPrevLedger(
 
     // Get validators that are on our ledger, or "close" to being on
     // our ledger.
-    auto vals =
+    hash_map<uint256, std::uint32_t> ledgerCounts =
         app_.getValidations().currentTrustedDistribution(
             ledgerID, parentID, ledgerMaster_.getValidLedgerIndex());
 
     uint256 netLgr = ledgerID;
     int netLgrCount = 0;
-    for (auto& it : vals)
+    for (auto const & it : ledgerCounts)
     {
         // Switch to ledger supported by more peers
         // Or stick with ours on a tie
-        if ((it.second.count > netLgrCount) ||
+        if ((it.second > netLgrCount) ||
-            ((it.second.count== netLgrCount) && (it.first == ledgerID)))
+            ((it.second == netLgrCount) && (it.first == ledgerID)))
         {
             netLgr = it.first;
-            netLgrCount = it.second.count;
+            netLgrCount = it.second;
         }
     }
 
@@ -267,8 +267,8 @@ RCLConsensus::Adaptor::getPrevLedger(
 
         if (auto stream = j_.debug())
         {
-            for (auto& it : vals)
+            for (auto const & it : ledgerCounts)
-                stream << "V: " << it.first << ", " << it.second.count;
+                stream << "V: " << it.first << ", " << it.second;
         }
     }
 

src/ripple/app/consensus/RCLValidations.h

@@ -99,27 +99,6 @@ public:
         return val_->isTrusted();
     }
 
-    /// Set the prior ledger hash this validation is following
-    void
-    setPreviousLedgerID(uint256 const& hash)
-    {
-        val_->setPreviousHash(hash);
-    }
-
-    /// Get the prior ledger hash this validation is following
-    uint256
-    getPreviousLedgerID() const
-    {
-        return val_->getPreviousHash();
-    }
-
-    /// Check whether the given hash matches this validation's prior hash
-    bool
-    isPreviousLedgerID(uint256 const& hash) const
-    {
-        return val_->isPreviousHash(hash);
-    }
-
     /// Get the load fee of the validation if it exists
     boost::optional<std::uint32_t>
     loadFee() const

src/ripple/app/misc/NetworkOPs.cpp

@@ -1262,39 +1262,6 @@ void NetworkOPsImp::setAmendmentBlocked ()
     setMode (omTRACKING);
 }
 
-class ValidationCount
-{
-public:
-    int trustedValidations, nodesUsing;
-    NodeID highNodeUsing, highValidation;
-
-    ValidationCount () : trustedValidations (0), nodesUsing (0)
-    {
-    }
-
-    bool operator> (const ValidationCount& v) const
-    {
-        if (trustedValidations > v.trustedValidations)
-            return true;
-
-        if (trustedValidations < v.trustedValidations)
-            return false;
-
-        if (trustedValidations == 0)
-        {
-            if (nodesUsing > v.nodesUsing)
-                return true;
-
-            if (nodesUsing < v.nodesUsing)
-                return false;
-
-            return highNodeUsing > v.highNodeUsing;
-        }
-
-        return highValidation > v.highValidation;
-    }
-};
-
 bool NetworkOPsImp::checkLastClosedLedger (
     const Overlay::PeerSequence& peerList, uint256& networkClosed)
 {
@@ -1315,20 +1282,43 @@ bool NetworkOPsImp::checkLastClosedLedger (
     JLOG(m_journal.trace()) << "OurClosed:  " << closedLedger;
     JLOG(m_journal.trace()) << "PrevClosed: " << prevClosedLedger;
 
+    struct ValidationCount
+    {
+        int trustedValidations, nodesUsing;
+
+        ValidationCount() : trustedValidations(0), nodesUsing(0)
+        {
+        }
+
+        auto
+        asTie() const
+        {
+            return std::tie(trustedValidations, nodesUsing);
+        }
+
+        bool
+        operator>(const ValidationCount& v) const
+        {
+            return asTie() > v.asTie();
+        }
+
+        bool
+        operator==(const ValidationCount& v) const
+        {
+            return asTie() == v.asTie();
+        }
+    };
+
     hash_map<uint256, ValidationCount> ledgers;
     {
-        auto current = app_.getValidations ().currentTrustedDistribution (
+        hash_map<uint256, std::uint32_t> current =
-            closedLedger, prevClosedLedger,
+            app_.getValidations().currentTrustedDistribution(
+                closedLedger,
+                prevClosedLedger,
                 m_ledgerMaster.getValidLedgerIndex());
 
         for (auto& it: current)
-        {
+            ledgers[it.first].trustedValidations += it.second;
-            auto& vc = ledgers[it.first];
-            vc.trustedValidations += it.second.count;
-
-            if (it.second.highNode > vc.highValidation)
-                vc.highValidation = it.second.highNode;
-        }
     }
 
     auto& ourVC = ledgers[closedLedger];
@@ -1336,10 +1326,6 @@ bool NetworkOPsImp::checkLastClosedLedger (
     if (mMode >= omTRACKING)
     {
         ++ourVC.nodesUsing;
-        auto const ourNodeID = calcNodeID(
-            app_.nodeIdentity().first);
-        if (ourNodeID > ourVC.highNodeUsing)
-            ourVC.highNodeUsing = ourNodeID;
     }
 
     for (auto& peer: peerList)
@@ -1347,23 +1333,7 @@ bool NetworkOPsImp::checkLastClosedLedger (
         uint256 peerLedger = peer->getClosedLedgerHash ();
 
         if (peerLedger.isNonZero ())
-        {
+            ++ledgers[peerLedger].nodesUsing;
-            try
-            {
-                auto& vc = ledgers[peerLedger];
-                auto const nodeId = calcNodeID(peer->getNodePublic ());
-                if (vc.nodesUsing == 0 || nodeId > vc.highNodeUsing)
-                {
-                    vc.highNodeUsing = nodeId;
-                }
-
-                ++vc.nodesUsing;
-            }
-            catch (std::exception const&)
-            {
-                // Peer is likely not connected anymore
-            }
-        }
     }
 
     auto bestVC = ledgers[closedLedger];
@@ -1381,17 +1351,20 @@ bool NetworkOPsImp::checkLastClosedLedger (
         // Temporary logging to make sure tiebreaking isn't broken
         if (it.second.trustedValidations > 0)
             JLOG(m_journal.trace())
-                << "  TieBreakTV: " << it.second.highValidation;
+                << "  TieBreakTV: " << it.first;
         else
         {
             if (it.second.nodesUsing > 0)
             {
                 JLOG(m_journal.trace())
-                    << "  TieBreakNU: " << it.second.highNodeUsing;
+                    << "  TieBreakNU: " << it.first;
             }
         }
 
-        if (it.second > bestVC)
+        // Switch to a ledger with more support
+        // or the one with higher hash if they have the same support
+        if (it.second > bestVC ||
+            (it.second == bestVC && it.first > closedLedger))
         {
             bestVC = it.second;
             closedLedger = it.first;

src/ripple/consensus/Validations.h

@@ -152,21 +152,6 @@ isCurrent(
         // arrived
         bool trusted() const;
 
-        // The PreviousLedger functions below assume instances corresponding
-        // to the same validation (ID/hash/key etc.) share the same previous
-        // ledger ID storage.
-
-        // Set the previous validation ledger from this publishing node that
-        // this validation replaced
-        void setPreviousLedgerID(LedgerID &);
-
-        // Get the previous validation ledger from this publishing node that
-        // this validation replaced
-        LedgerID getPreviousLedgerID() const;
-
-        // Check if this validation had the given ledger ID as its prior ledger
-        bool isPreviousLedgerID(LedgerID const& ) const;
-
         implementation_specific_t
         unwrap() -> return the implementation-specific type being wrapped
 
@@ -207,20 +192,32 @@ class Validations
         decay_result_t<decltype (&Validation::ledgerID)(Validation)>;
     using NodeKey = decay_result_t<decltype (&Validation::key)(Validation)>;
     using NodeID = decay_result_t<decltype (&Validation::nodeID)(Validation)>;
-    using ValidationMap = hash_map<NodeKey, Validation>;
+
 
     using ScopedLock = std::lock_guard<MutexType>;
 
     // Manages concurrent access to current_ and byLedger_
     MutexType mutex_;
 
+    //! For the most recent validation, we also want to store the ID
+    //! of the ledger it replaces
+    struct ValidationAndPrevID
+    {
+        ValidationAndPrevID(Validation const& v) : val{v}, prevLedgerID{0}
+        {
+        }
+
+        Validation val;
+        LedgerID prevLedgerID;
+    };
+
     //! The latest validation from each node
-    ValidationMap current_;
+    hash_map<NodeKey, ValidationAndPrevID> current_;
 
     //! Recent validations from nodes, indexed by ledger identifier
     beast::aged_unordered_map<
         LedgerID,
-        ValidationMap,
+        hash_map<NodeKey, Validation>,
         std::chrono::steady_clock,
         beast::uhash<>>
         byLedger_;
@@ -264,15 +261,15 @@ private:
             // Check for staleness, if time specified
             if (t &&
                 !isCurrent(
-                    parms_, *t, it->second.signTime(), it->second.seenTime()))
+                    parms_, *t, it->second.val.signTime(), it->second.val.seenTime()))
             {
                 // contains a stale record
-                stalePolicy_.onStale(std::move(it->second));
+                stalePolicy_.onStale(std::move(it->second.val));
                 it = current_.erase(it);
             }
             else
             {
-                auto cit = typename ValidationMap::const_iterator{it};
+                auto cit = typename decltype(current_)::const_iterator{it};
                 // contains a live record
                 f(cit->first, cit->second);
                 ++it;
@@ -397,7 +394,8 @@ public:
             if (!ins.second)
             {
                 // Had a previous validation from the node, consider updating
-                Validation& oldVal = ins.first->second;
+                Validation& oldVal = ins.first->second.val;
+                LedgerID const previousLedgerID = ins.first->second.prevLedgerID;
 
                 std::uint32_t const oldSeq{oldVal.seq()};
                 std::uint32_t const newSeq{val.seq()};
@@ -414,7 +412,7 @@ public:
                         auto const mapIt = byLedger_.find(oldVal.ledgerID());
                         if (mapIt != byLedger_.end())
                         {
-                            ValidationMap& validationMap = mapIt->second;
+                            auto& validationMap = mapIt->second;
                             // If a new validation with the same ID was
                             // reissued we simply replace.
                             if(oldVal.ledgerID() == val.ledgerID())
@@ -449,14 +447,14 @@ public:
                         // In the case the key was revoked and a new validation
                         // for the same ledger ID was sent, the previous ledger
                         // is still the one the now revoked validation had
-                        return oldVal.getPreviousLedgerID();
+                        return previousLedgerID;
                     }();
 
                     // Allow impl to take over oldVal
                     maybeStaleValidation.emplace(std::move(oldVal));
                     // Replace old val in the map and set the previous ledger ID
-                    ins.first->second = val;
+                    ins.first->second.val = val;
-                    ins.first->second.setPreviousLedgerID(prevID);
+                    ins.first->second.prevLedgerID = prevID;
                 }
                 else
                 {
@@ -487,14 +485,6 @@ public:
         beast::expire(byLedger_, parms_.validationSET_EXPIRES);
     }
 
-    struct ValidationCounts
-    {
-        //! The number of trusted validations
-        std::size_t count;
-        //! The highest trusted node ID
-        NodeID highNode;
-    };
-
     /** Distribution of current trusted validations
 
         Calculates the distribution of current validations but allows
@@ -503,8 +493,10 @@ public:
         @param currentLedger The identifier of the ledger we believe is current
         @param priorLedger The identifier of our previous current ledger
         @param cutoffBefore Ignore ledgers with sequence number before this
+
+        @return Map representing the distribution of ledgerID by count
     */
-    hash_map<LedgerID, ValidationCounts>
+    hash_map<LedgerID, std::uint32_t>
     currentTrustedDistribution(
         LedgerID const& currentLedger,
         LedgerID const& priorLedger,
@@ -513,7 +505,7 @@ public:
         bool const valCurrentLedger = currentLedger != beast::zero;
         bool const valPriorLedger = priorLedger != beast::zero;
 
-        hash_map<LedgerID, ValidationCounts> ret;
+        hash_map<LedgerID, std::uint32_t> ret;
 
         current(
             stalePolicy_.now(),
@@ -526,8 +518,9 @@ public:
              &valCurrentLedger,
              &valPriorLedger,
              &priorLedger,
-             &ret](NodeKey const&, Validation const& v) {
+             &ret](NodeKey const&, ValidationAndPrevID const& vp) {
-
+                Validation const& v = vp.val;
+                LedgerID const& prevLedgerID = vp.prevLedgerID;
                 if (!v.trusted())
                     return;
 
@@ -541,7 +534,7 @@ public:
                     if (!countPreferred &&  // allow up to one ledger slip in
                                             // either direction
                         ((valCurrentLedger &&
-                          v.isPreviousLedgerID(currentLedger)) ||
+                          (prevLedgerID == currentLedger)) ||
                          (valPriorLedger && (v.ledgerID() == priorLedger))))
                     {
                         countPreferred = true;
@@ -549,13 +542,10 @@ public:
                                          << " not " << v.ledgerID();
                     }
 
-                    ValidationCounts& p =
+                    if (countPreferred)
-                        countPreferred ? ret[currentLedger] : ret[v.ledgerID()];
+                        ret[currentLedger]++;
-                    ++(p.count);
+                    else
-
+                        ret[v.ledgerID()]++;
-                    NodeID const ni = v.nodeID();
-                    if (ni > p.highNode)
-                        p.highNode = ni;
                 }
             });
 
@@ -582,8 +572,8 @@ public:
         current(
             boost::none,
             [&](std::size_t) {}, // nothing to reserve
-            [&](NodeKey const&, Validation const& v) {
+            [&](NodeKey const&, ValidationAndPrevID const& v) {
-                if (v.trusted() && v.isPreviousLedgerID(ledgerID))
+                if (v.val.trusted() && v.prevLedgerID == ledgerID)
                     ++count;
             });
         return count;
@@ -601,9 +591,9 @@ public:
         current(
             stalePolicy_.now(),
             [&](std::size_t numValidations) { ret.reserve(numValidations); },
-            [&](NodeKey const&, Validation const& v) {
+            [&](NodeKey const&, ValidationAndPrevID const& v) {
-                if (v.trusted())
+                if (v.val.trusted())
-                    ret.push_back(v.unwrap());
+                    ret.push_back(v.val.unwrap());
             });
         return ret;
     }
@@ -620,7 +610,7 @@ public:
         current(
             stalePolicy_.now(),
             [&](std::size_t numValidations) { ret.reserve(numValidations); },
-            [&](NodeKey const& k, Validation const&) { ret.insert(k); });
+            [&](NodeKey const& k, ValidationAndPrevID const&) { ret.insert(k); });
 
         return ret;
     }
@@ -717,10 +707,13 @@ public:
         JLOG(j_.info()) << "Flushing validations";
 
         hash_map<NodeKey, Validation> flushed;
-        using std::swap;
         {
             ScopedLock lock{mutex_};
-            swap(flushed, current_);
+            for (auto it : current_)
+            {
+                flushed.emplace(it.first, std::move(it.second.val));
+            }
+            current_.clear();
         }
 
         stalePolicy_.flush(std::move(flushed));

src/ripple/protocol/STValidation.h

@@ -100,26 +100,11 @@ public:
     // Signs the validation and returns the signing hash
     uint256 sign (SecretKey const& secretKey);
 
-    // The validation this replaced
-    uint256 const& getPreviousHash ()
-    {
-        return mPreviousHash;
-    }
-    bool isPreviousHash (uint256 const& h) const
-    {
-        return mPreviousHash == h;
-    }
-    void setPreviousHash (uint256 const& h)
-    {
-        mPreviousHash = h;
-    }
-
 private:
     static SOTemplate const& getFormat ();
 
     void setNode ();
 
-    uint256 mPreviousHash;
     NodeID mNodeID;
     bool mTrusted = false;
     NetClock::time_point mSeen = {};

src/test/consensus/Validations_test.cpp

@@ -103,11 +103,9 @@ class Validations_test : public beast::unit_test::suite
         std::size_t nodeID_ = 0;
         bool trusted_ = true;
         boost::optional<std::uint32_t> loadFee_;
-        // Shared prevID to ensure value is shared amongst instances
-        std::shared_ptr<ID> prevID_;
 
     public:
-        Validation() : prevID_(std::make_shared<ID>(0))
+        Validation()
         {
         }
 
@@ -153,24 +151,6 @@ class Validations_test : public beast::unit_test::suite
             return trusted_;
         }
 
-        void
-        setPreviousLedgerID(ID const& prevID)
-        {
-            *prevID_ = prevID;
-        }
-
-        bool
-        isPreviousLedgerID(ID const& prevID) const
-        {
-            return *prevID_ == prevID;
-        }
-
-        ID
-        getPreviousLedgerID() const
-        {
-            return *prevID_;
-        }
-
         boost::optional<std::uint32_t>
         loadFee() const
         {
@@ -194,7 +174,6 @@ class Validations_test : public beast::unit_test::suite
                 key_,
                 nodeID_,
                 trusted_,
-                *prevID_,
                 loadFee_);
         }
         bool
@@ -488,19 +467,24 @@ class Validations_test : public beast::unit_test::suite
                 // new ID but the same sequence number
                 a.advanceKey();
 
+                // No validations following ID{2}
+                BEAST_EXPECT(harness.vals().getNodesAfter(ID{2}) == 0);
+
                 BEAST_EXPECT(
                     AddOutcome::sameSeq == harness.add(a, Seq{2}, ID{20}));
 
                 // Old ID should be gone ...
                 BEAST_EXPECT(harness.vals().numTrustedForLedger(ID{2}) == 0);
                 BEAST_EXPECT(harness.vals().numTrustedForLedger(ID{20}) == 1);
-                // ... and the previous ID set to the old ID
                 {
+                    // Should be the only trusted for ID{20}
                     auto trustedVals =
                         harness.vals().getTrustedForLedger(ID{20});
                     BEAST_EXPECT(trustedVals.size() == 1);
                     BEAST_EXPECT(trustedVals[0].key() == a.currKey());
-                    BEAST_EXPECT(trustedVals[0].getPreviousLedgerID() == ID{2});
+                    // ... and should be the only node after ID{2}
+                    BEAST_EXPECT(harness.vals().getNodesAfter(ID{2}) == 1);
+
                 }
 
                 // A new key, but re-issue a validation with the same ID and
@@ -509,13 +493,14 @@ class Validations_test : public beast::unit_test::suite
 
                 BEAST_EXPECT(
                     AddOutcome::sameSeq == harness.add(a, Seq{2}, ID{20}));
-                // Ensure the old prevLedgerID was retained
                 {
+                    // Still the only trusted validation for ID{20}
                     auto trustedVals =
                         harness.vals().getTrustedForLedger(ID{20});
                     BEAST_EXPECT(trustedVals.size() == 1);
                     BEAST_EXPECT(trustedVals[0].key() == a.currKey());
-                    BEAST_EXPECT(trustedVals[0].getPreviousLedgerID() == ID{2});
+                    // and still follows ID{2} since it was a re-issue
+                    BEAST_EXPECT(harness.vals().getNodesAfter(ID{2}) == 1);
                 }
             }
 
@@ -761,8 +746,7 @@ class Validations_test : public beast::unit_test::suite
                 Seq{0});  // No cutoff
 
             BEAST_EXPECT(res.size() == 1);
-            BEAST_EXPECT(res[ID{2}].count == 3);
+            BEAST_EXPECT(res[ID{2}] == 3);
-            BEAST_EXPECT(res[ID{2}].highNode == mama.nodeID());
         }
 
         {
@@ -773,10 +757,8 @@ class Validations_test : public beast::unit_test::suite
                 Seq{0});  // No cutoff
 
             BEAST_EXPECT(res.size() == 2);
-            BEAST_EXPECT(res[ID{2}].count == 2);
+            BEAST_EXPECT(res[ID{2}] == 2);
-            BEAST_EXPECT(res[ID{2}].highNode == mama.nodeID());
+            BEAST_EXPECT(res[ID{1}] == 1);
-            BEAST_EXPECT(res[ID{1}].count == 1);
-            BEAST_EXPECT(res[ID{1}].highNode == papa.nodeID());
         }
 
         {
@@ -787,12 +769,9 @@ class Validations_test : public beast::unit_test::suite
                 Seq{0});  // No cutoff
 
             BEAST_EXPECT(res.size() == 3);
-            BEAST_EXPECT(res[ID{1}].count == 1);
+            BEAST_EXPECT(res[ID{1}] == 1);
-            BEAST_EXPECT(res[ID{1}].highNode == papa.nodeID());
+            BEAST_EXPECT(res[ID{2}] == 1);
-            BEAST_EXPECT(res[ID{2}].count == 1);
+            BEAST_EXPECT(res[ID{3}] == 1);
-            BEAST_EXPECT(res[ID{2}].highNode == baby.nodeID());
-            BEAST_EXPECT(res[ID{3}].count == 1);
-            BEAST_EXPECT(res[ID{3}].highNode == mama.nodeID());
         }
 
         {
@@ -802,8 +781,7 @@ class Validations_test : public beast::unit_test::suite
                 ID{1},    // prior ledger
                 Seq{2});  // Only sequence 2 or later
             BEAST_EXPECT(res.size() == 1);
-            BEAST_EXPECT(res[ID{2}].count == 2);
+            BEAST_EXPECT(res[ID{2}] == 2);
-            BEAST_EXPECT(res[ID{2}].highNode == mama.nodeID());
         }
     }
 
@@ -950,7 +928,6 @@ class Validations_test : public beast::unit_test::suite
         harness.clock().advance(1s);
         auto newVal = a.validation(Seq{2}, ID{2});
         BEAST_EXPECT(AddOutcome::current == harness.add(a, newVal));
-        newVal.setPreviousLedgerID(staleA.ledgerID());
         expected[a.masterKey()] = newVal;
 
         // Now flush