From 8db21fa178977bf84a378e55dfc053aee73294d8 Mon Sep 17 00:00:00 2001 From: Brad Chase Date: Thu, 11 May 2017 15:42:29 -0400 Subject: [PATCH 1/3] 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 | 14 ++++----- src/ripple/app/misc/NetworkOPs.cpp | 38 ++++++++++++----------- src/ripple/consensus/Validations.h | 25 +++++---------- src/test/consensus/Validations_test.cpp | 21 +++++-------- 4 files changed, 42 insertions(+), 56 deletions(-) diff --git a/src/ripple/app/consensus/RCLConsensus.cpp b/src/ripple/app/consensus/RCLConsensus.cpp index d269e818bdd..1a83225d878 100644 --- 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 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/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index 8d37ae1c609..89a8b3dc4fe 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -1266,7 +1266,9 @@ class ValidationCount { public: int trustedValidations, nodesUsing; - NodeID highNodeUsing, highValidation; + // Ledger hashes to break ties + uint256 highUsingHash, highTrustedValHash; + ValidationCount () : trustedValidations (0), nodesUsing (0) { @@ -1288,10 +1290,10 @@ class ValidationCount if (nodesUsing < v.nodesUsing) return false; - return highNodeUsing > v.highNodeUsing; + return highUsingHash > v.highUsingHash; } - return highValidation > v.highValidation; + return highTrustedValHash > v.highTrustedValHash; } }; @@ -1317,17 +1319,19 @@ bool NetworkOPsImp::checkLastClosedLedger ( hash_map ledgers; { - auto current = app_.getValidations ().currentTrustedDistribution ( - closedLedger, prevClosedLedger, - m_ledgerMaster.getValidLedgerIndex()); + hash_map current = + app_.getValidations().currentTrustedDistribution( + closedLedger, + prevClosedLedger, + m_ledgerMaster.getValidLedgerIndex()); for (auto& it: current) { auto& vc = ledgers[it.first]; - vc.trustedValidations += it.second.count; + vc.trustedValidations += it.second; - if (it.second.highNode > vc.highValidation) - vc.highValidation = it.second.highNode; + if (it.first > vc.highTrustedValHash) + vc.highTrustedValHash = it.first; } } @@ -1336,10 +1340,9 @@ bool NetworkOPsImp::checkLastClosedLedger ( if (mMode >= omTRACKING) { ++ourVC.nodesUsing; - auto const ourNodeID = calcNodeID( - app_.nodeIdentity().first); - if (ourNodeID > ourVC.highNodeUsing) - ourVC.highNodeUsing = ourNodeID; + + if (closedLedger > ourVC.highUsingHash) + ourVC.highUsingHash = closedLedger; } for (auto& peer: peerList) @@ -1351,10 +1354,9 @@ bool NetworkOPsImp::checkLastClosedLedger ( try { auto& vc = ledgers[peerLedger]; - auto const nodeId = calcNodeID(peer->getNodePublic ()); - if (vc.nodesUsing == 0 || nodeId > vc.highNodeUsing) + if (vc.nodesUsing == 0 || peerLedger > vc.highUsingHash) { - vc.highNodeUsing = nodeId; + vc.highUsingHash = peerLedger; } ++vc.nodesUsing; @@ -1381,13 +1383,13 @@ 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.second.highTrustedValHash; else { if (it.second.nodesUsing > 0) { JLOG(m_journal.trace()) - << " TieBreakNU: " << it.second.highNodeUsing; + << " TieBreakNU: " << it.second.highUsingHash; } } diff --git a/src/ripple/consensus/Validations.h b/src/ripple/consensus/Validations.h index ff5c6a31be3..aa77399559e 100644 --- a/src/ripple/consensus/Validations.h +++ b/src/ripple/consensus/Validations.h @@ -487,14 +487,6 @@ class Validations 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 +495,10 @@ class Validations @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 + hash_map currentTrustedDistribution( LedgerID const& currentLedger, LedgerID const& priorLedger, @@ -513,7 +507,7 @@ class Validations bool const valCurrentLedger = currentLedger != beast::zero; bool const valPriorLedger = priorLedger != beast::zero; - hash_map ret; + hash_map ret; current( stalePolicy_.now(), @@ -549,13 +543,10 @@ class Validations << " not " << v.ledgerID(); } - ValidationCounts& p = - countPreferred ? ret[currentLedger] : ret[v.ledgerID()]; - ++(p.count); - - NodeID const ni = v.nodeID(); - if (ni > p.highNode) - p.highNode = ni; + if (countPreferred) + ret[currentLedger]++; + else + ret[v.ledgerID()]++; } }); diff --git a/src/test/consensus/Validations_test.cpp b/src/test/consensus/Validations_test.cpp index 52878f752be..dfd6f1e6d07 100644 --- a/src/test/consensus/Validations_test.cpp +++ b/src/test/consensus/Validations_test.cpp @@ -761,8 +761,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}].highNode == mama.nodeID()); + BEAST_EXPECT(res[ID{2}] == 3); } { @@ -773,10 +772,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}].highNode == mama.nodeID()); - BEAST_EXPECT(res[ID{1}].count == 1); - BEAST_EXPECT(res[ID{1}].highNode == papa.nodeID()); + BEAST_EXPECT(res[ID{2}] == 2); + BEAST_EXPECT(res[ID{1}] == 1); } { @@ -787,12 +784,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}].highNode == papa.nodeID()); - BEAST_EXPECT(res[ID{2}].count == 1); - BEAST_EXPECT(res[ID{2}].highNode == baby.nodeID()); - BEAST_EXPECT(res[ID{3}].count == 1); - BEAST_EXPECT(res[ID{3}].highNode == mama.nodeID()); + BEAST_EXPECT(res[ID{1}] == 1); + BEAST_EXPECT(res[ID{2}] == 1); + BEAST_EXPECT(res[ID{3}] == 1); } { @@ -802,8 +796,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}].highNode == mama.nodeID()); + BEAST_EXPECT(res[ID{2}] == 2); } } From 0d70a071ff60922e045436271ccdab9608e0be73 Mon Sep 17 00:00:00 2001 From: Brad Chase Date: Fri, 19 May 2017 10:37:36 -0400 Subject: [PATCH 2/3] [FOLD] Make prevLedgerID a private detail of Validations --- src/ripple/app/consensus/RCLValidations.h | 21 ------- src/ripple/consensus/Validations.h | 76 ++++++++++++----------- src/ripple/protocol/STValidation.h | 15 ----- src/test/consensus/Validations_test.cpp | 38 ++++-------- 4 files changed, 50 insertions(+), 100 deletions(-) diff --git a/src/ripple/app/consensus/RCLValidations.h b/src/ripple/app/consensus/RCLValidations.h index ec1939e8c43..cb8dbff0578 100644 --- 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 loadFee() const diff --git a/src/ripple/consensus/Validations.h b/src/ripple/consensus/Validations.h index aa77399559e..cd1688bf44f 100644 --- a/src/ripple/consensus/Validations.h +++ b/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; using NodeKey = decay_result_t; using NodeID = decay_result_t; - using ValidationMap = hash_map; + using ScopedLock = std::lock_guard; // 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 current_; //! Recent validations from nodes, indexed by ledger identifier beast::aged_unordered_map< LedgerID, - ValidationMap, + hash_map, std::chrono::steady_clock, beast::uhash<>> byLedger_; @@ -264,15 +261,15 @@ class Validations // 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 @@ class Validations 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 @@ class Validations 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 @@ class Validations // 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.setPreviousLedgerID(prevID); + ins.first->second.val = val; + ins.first->second.prevLedgerID = prevID; } else { @@ -520,8 +518,9 @@ class Validations &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; @@ -535,7 +534,7 @@ class Validations if (!countPreferred && // allow up to one ledger slip in // either direction ((valCurrentLedger && - v.isPreviousLedgerID(currentLedger)) || + (prevLedgerID == currentLedger)) || (valPriorLedger && (v.ledgerID() == priorLedger)))) { countPreferred = true; @@ -573,8 +572,8 @@ class Validations current( boost::none, [&](std::size_t) {}, // nothing to reserve - [&](NodeKey const&, Validation const& v) { - if (v.trusted() && v.isPreviousLedgerID(ledgerID)) + [&](NodeKey const&, ValidationAndPrevID const& v) { + if (v.val.trusted() && v.prevLedgerID == ledgerID) ++count; }); return count; @@ -592,9 +591,9 @@ class Validations current( stalePolicy_.now(), [&](std::size_t numValidations) { ret.reserve(numValidations); }, - [&](NodeKey const&, Validation const& v) { - if (v.trusted()) - ret.push_back(v.unwrap()); + [&](NodeKey const&, ValidationAndPrevID const& v) { + if (v.val.trusted()) + ret.push_back(v.val.unwrap()); }); return ret; } @@ -611,7 +610,7 @@ class Validations 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; } @@ -708,10 +707,13 @@ class Validations JLOG(j_.info()) << "Flushing validations"; hash_map 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)); diff --git a/src/ripple/protocol/STValidation.h b/src/ripple/protocol/STValidation.h index 84113e70309..53baabe568d 100644 --- a/src/ripple/protocol/STValidation.h +++ b/src/ripple/protocol/STValidation.h @@ -100,26 +100,11 @@ class STValidation final // 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 = {}; diff --git a/src/test/consensus/Validations_test.cpp b/src/test/consensus/Validations_test.cpp index dfd6f1e6d07..29855de2601 100644 --- a/src/test/consensus/Validations_test.cpp +++ b/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 loadFee_; - // Shared prevID to ensure value is shared amongst instances - std::shared_ptr prevID_; public: - Validation() : prevID_(std::make_shared(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 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 the 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); } } @@ -943,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 From f8774f8cc970b1af0cd21af80286b82c7ebd38d7 Mon Sep 17 00:00:00 2001 From: Brad Chase Date: Fri, 14 Jul 2017 13:34:19 -0400 Subject: [PATCH 3/3] [FOLD] Address review comments --- src/ripple/app/consensus/RCLConsensus.cpp | 2 +- src/ripple/app/misc/NetworkOPs.cpp | 99 ++++++++--------------- src/test/consensus/Validations_test.cpp | 4 +- 3 files changed, 38 insertions(+), 67 deletions(-) diff --git a/src/ripple/app/consensus/RCLConsensus.cpp b/src/ripple/app/consensus/RCLConsensus.cpp index 1a83225d878..2f5edf6ced0 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 89a8b3dc4fe..22dc5512ea8 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -1262,41 +1262,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) { @@ -1317,6 +1282,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 = @@ -1326,13 +1318,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]; @@ -1340,9 +1326,6 @@ bool NetworkOPsImp::checkLastClosedLedger ( if (mMode >= omTRACKING) { ++ourVC.nodesUsing; - - if (closedLedger > ourVC.highUsingHash) - ourVC.highUsingHash = closedLedger; } for (auto& peer: peerList) @@ -1350,22 +1333,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]; @@ -1383,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.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