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.
vcgato29 · Aug 10, 2017 · 60dd194 · 60dd194
1 parent 9ae717c
commit 60dd194
Show file tree

Hide file tree

Showing 6 changed files with 111 additions and 204 deletions.
diff --git a/src/ripple/app/consensus/RCLConsensus.cpp b/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) ||
-            ((it.second.count== netLgrCount) && (it.first == ledgerID)))
+        if ((it.second > netLgrCount) ||
+            ((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)
-                stream << "V: " << it.first << ", " << it.second.count;
+            for (auto const & it : ledgerCounts)
+                stream << "V: " << it.first << ", " << it.second;
         }
     }
 

diff --git a/src/ripple/app/consensus/RCLValidations.h b/src/ripple/app/consensus/RCLValidations.h
@@ -99,27 +99,6 @@ class RCLValidation
         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

diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/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,55 +1282,58 @@ bool NetworkOPsImp::checkLastClosedLedger (
     JLOG(m_journal.trace()) << "OurClosed:  " << closedLedger;
     JLOG(m_journal.trace()) << "PrevClosed: " << prevClosedLedger;
 
-    hash_map<uint256, ValidationCount> ledgers;
+    struct ValidationCount
     {
-        auto current = app_.getValidations ().currentTrustedDistribution (
-            closedLedger, prevClosedLedger,
-            m_ledgerMaster.getValidLedgerIndex());
+        int trustedValidations, nodesUsing;
 
-        for (auto& it: current)
+        ValidationCount() : trustedValidations(0), nodesUsing(0)
+        {
+        }
+
+        auto
+        asTie() const
+        {
+            return std::tie(trustedValidations, nodesUsing);
+        }
+
+        bool
+        operator>(const ValidationCount& v) const
         {
-            auto& vc = ledgers[it.first];
-            vc.trustedValidations += it.second.count;
+            return asTie() > v.asTie();
+        }
 
-            if (it.second.highNode > vc.highValidation)
-                vc.highValidation = it.second.highNode;
+        bool
+        operator==(const ValidationCount& v) const
+        {
+            return asTie() == v.asTie();
         }
+    };
+
+    hash_map<uint256, ValidationCount> ledgers;
+    {
+        hash_map<uint256, std::uint32_t> current =
+            app_.getValidations().currentTrustedDistribution(
+                closedLedger,
+                prevClosedLedger,
+                m_ledgerMaster.getValidLedgerIndex());
+
+        for (auto& it: current)
+            ledgers[it.first].trustedValidations += it.second;
     }
 
     auto& ourVC = ledgers[closedLedger];
 
     if (mMode >= omTRACKING)
     {
         ++ourVC.nodesUsing;
-        auto const ourNodeID = calcNodeID(
-            app_.nodeIdentity().first);
-        if (ourNodeID > ourVC.highNodeUsing)
-            ourVC.highNodeUsing = ourNodeID;
     }
 
     for (auto& peer: peerList)
     {
         uint256 peerLedger = peer->getClosedLedgerHash ();
 
         if (peerLedger.isNonZero ())
-        {
-            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
-            }
-        }
+            ++ledgers[peerLedger].nodesUsing;
     }
 
     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;