diff --git a/src/ripple/app/consensus/RCLConsensus.cpp b/src/ripple/app/consensus/RCLConsensus.cpp index d269e818bdd..2f5edf6ced0 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/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/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index 8d37ae1c609..22dc5512ea8 100644 --- 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,20 +1282,43 @@ bool NetworkOPsImp::checkLastClosedLedger ( JLOG(m_journal.trace()) << "OurClosed: " << closedLedger; JLOG(m_journal.trace()) << "PrevClosed: " << prevClosedLedger; - hash_map 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 ledgers; + { + hash_map current = + app_.getValidations().currentTrustedDistribution( + closedLedger, + prevClosedLedger, + m_ledgerMaster.getValidLedgerIndex()); + + for (auto& it: current) + ledgers[it.first].trustedValidations += it.second; } auto& ourVC = ledgers[closedLedger]; @@ -1336,10 +1326,6 @@ bool NetworkOPsImp::checkLastClosedLedger ( if (mMode >= omTRACKING) { ++ourVC.nodesUsing; - auto const ourNodeID = calcNodeID( - app_.nodeIdentity().first); - if (ourNodeID > ourVC.highNodeUsing) - ourVC.highNodeUsing = ourNodeID; } for (auto& peer: peerList) @@ -1347,23 +1333,7 @@ bool NetworkOPsImp::checkLastClosedLedger ( 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; diff --git a/src/ripple/consensus/Validations.h b/src/ripple/consensus/Validations.h index ff5c6a31be3..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 { @@ -487,14 +485,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 +493,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 +505,7 @@ class Validations bool const valCurrentLedger = currentLedger != beast::zero; bool const valPriorLedger = priorLedger != beast::zero; - hash_map ret; + hash_map ret; current( stalePolicy_.now(), @@ -526,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; @@ -541,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; @@ -549,13 +542,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()]++; } }); @@ -582,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; @@ -601,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; } @@ -620,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; } @@ -717,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 52878f752be..7c807ac457a 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 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()); - 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); } } @@ -761,8 +746,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 +757,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 +769,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 +781,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); } } @@ -950,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