From 94af6c290db40071ca19e29cc1717ca8bb5d1af4 Mon Sep 17 00:00:00 2001 From: Brad Chase Date: Fri, 14 Jul 2017 13:34:19 -0400 Subject: [PATCH] [FOLD] Address review comments --- src/ripple/app/consensus/RCLConsensus.cpp | 2 +- src/ripple/app/misc/NetworkOPs.cpp | 100 ++++++++-------------- src/test/consensus/Validations_test.cpp | 4 +- 3 files changed, 39 insertions(+), 67 deletions(-) diff --git a/src/ripple/app/consensus/RCLConsensus.cpp b/src/ripple/app/consensus/RCLConsensus.cpp index 4264d022119..b4778ed3b0e 100644 --- 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 index da05bccdf39..530b99b221c 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -58,6 +58,7 @@ #include #include #include +#include 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 ledgers; { hash_map current = @@ -1323,13 +1316,7 @@ 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]; @@ -1337,9 +1324,6 @@ bool NetworkOPsImp::checkLastClosedLedger ( if (mMode >= omTRACKING) { ++ourVC.nodesUsing; - - if (closedLedger > ourVC.highUsingHash) - ourVC.highUsingHash = closedLedger; } for (auto& peer: peerList) @@ -1347,22 +1331,7 @@ bool NetworkOPsImp::checkLastClosedLedger ( 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 index 29855de2601..7c807ac457a 100644 --- 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