[FOLD] Address review comments

XRPLF · Jul 14, 2017 · 94af6c2 · 94af6c2
1 parent 6ca4760
commit 94af6c2
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 67 deletions.
diff --git a/src/ripple/app/consensus/RCLConsensus.cpp b/src/ripple/app/consensus/RCLConsensus.cpp
@@ -256,7 +256,7 @@ RCLConsensus::getPrevLedger(
         // Switch to ledger supported by more peers
         // Or stick with ours on a tie
         if ((it.second > netLgrCount) ||
-            ((it.second== netLgrCount) && (it.first == ledgerID)))
+            ((it.second == netLgrCount) && (it.first == ledgerID)))
         {
             netLgr = it.first;
             netLgrCount = it.second;

diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp
@@ -58,6 +58,7 @@
 #include <ripple/beast/utility/rngfill.h>
 #include <ripple/basics/make_lock.h>
 #include <beast/core/detail/base64.hpp>
+#include <tuple>
 
 namespace ripple {
 
@@ -1219,41 +1220,6 @@ void NetworkOPsImp::setAmendmentBlocked ()
     setMode (omTRACKING);
 }
 
-class ValidationCount
-{
-public:
-    int trustedValidations, nodesUsing;
-    // Ledger hashes to break ties
-    uint256 highUsingHash, highTrustedValHash;
-
-
-    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 highUsingHash > v.highUsingHash;
-        }
-
-        return highTrustedValHash > v.highTrustedValHash;
-    }
-};
-
 void NetworkOPsImp::tryStartConsensus ()
 {
     uint256 networkClosed;
@@ -1314,6 +1280,33 @@ 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;
     {
         hash_map<uint256, std::uint32_t> current =
@@ -1323,46 +1316,22 @@ bool NetworkOPsImp::checkLastClosedLedger (
                 m_ledgerMaster.getValidLedgerIndex());
 
         for (auto& it: current)
-        {
-            auto& vc = ledgers[it.first];
-            vc.trustedValidations += it.second;
-
-            if (it.first > vc.highTrustedValHash)
-                vc.highTrustedValHash = it.first;
-        }
+            ledgers[it.first].trustedValidations += it.second;
     }
 
     auto& ourVC = ledgers[closedLedger];
 
     if (mMode >= omTRACKING)
     {
         ++ourVC.nodesUsing;
-
-        if (closedLedger > ourVC.highUsingHash)
-            ourVC.highUsingHash = closedLedger;
     }
 
     for (auto& peer: peerList)
     {
         uint256 peerLedger = peer->getClosedLedgerHash ();
 
         if (peerLedger.isNonZero ())
-        {
-            try
-            {
-                auto& vc = ledgers[peerLedger];
-                if (vc.nodesUsing == 0 || peerLedger > vc.highUsingHash)
-                {
-                    vc.highUsingHash = peerLedger;
-                }
-
-                ++vc.nodesUsing;
-            }
-            catch (std::exception const&)
-            {
-                // Peer is likely not connected anymore
-            }
-        }
+            ++ledgers[peerLedger].nodesUsing;
     }
 
     auto bestVC = ledgers[closedLedger];
@@ -1380,17 +1349,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.highTrustedValHash;
+                << "  TieBreakTV: " << it.first;
         else
         {
             if (it.second.nodesUsing > 0)
             {
                 JLOG(m_journal.trace())
-                    << "  TieBreakNU: " << it.second.highUsingHash;
+                    << "  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;

diff --git a/src/test/consensus/Validations_test.cpp b/src/test/consensus/Validations_test.cpp
@@ -477,14 +477,14 @@ class Validations_test : public beast::unit_test::suite
                 BEAST_EXPECT(harness.vals().numTrustedForLedger(ID{2}) == 0);
                 BEAST_EXPECT(harness.vals().numTrustedForLedger(ID{20}) == 1);
                 {
-                    // Should the the only trusted for ID{20}
+                    // 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());
                     // ... 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