From ae731c3a1cf70bd648ba14ff3e0f0bfae919f272 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 30474f96fc6..360f098e067 100644 --- a/src/ripple/app/consensus/RCLConsensus.cpp +++ b/src/ripple/app/consensus/RCLConsensus.cpp @@ -253,7 +253,7 @@ RCLConsensus::Adaptor::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 71b3cb0f193..10f41e86817 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -59,6 +59,7 @@ #include #include #include +#include namespace ripple { @@ -1216,41 +1217,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; - } -}; - bool NetworkOPsImp::checkLastClosedLedger ( const Overlay::PeerSequence& peerList, uint256& networkClosed) { @@ -1271,6 +1237,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 = @@ -1280,13 +1273,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]; @@ -1294,9 +1281,6 @@ bool NetworkOPsImp::checkLastClosedLedger ( if (mMode >= omTRACKING) { ++ourVC.nodesUsing; - - if (closedLedger > ourVC.highUsingHash) - ourVC.highUsingHash = closedLedger; } for (auto& peer: peerList) @@ -1304,22 +1288,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]; @@ -1337,17 +1306,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