From d2319434f59d55c989f59b43efd0f21cd06a2360 Mon Sep 17 00:00:00 2001 From: seelabs Date: Wed, 29 Apr 2020 13:10:13 -0700 Subject: [PATCH] WIP Reviewing --- src/ripple/app/misc/NegativeUNLVote.cpp | 116 ++++++++++---------- src/ripple/app/misc/NegativeUNLVote.h | 24 ++-- src/test/consensus/NegativeUNLVote_test.cpp | 78 +++++++------ 3 files changed, 121 insertions(+), 97 deletions(-) diff --git a/src/ripple/app/misc/NegativeUNLVote.cpp b/src/ripple/app/misc/NegativeUNLVote.cpp index e2863030946..b0f27d84285 100644 --- a/src/ripple/app/misc/NegativeUNLVote.cpp +++ b/src/ripple/app/misc/NegativeUNLVote.cpp @@ -54,8 +54,8 @@ NegativeUNLVote::doVoting( unlNodeIDs.emplace(nid); } - hash_map scoreTable; - if (buildScoreTable(prevLedger, unlNodeIDs, validations, scoreTable)) + if (std::optional> scoreTable = + buildScoreTable(prevLedger, unlNodeIDs, validations)) { // build next nUNL auto nUnlKeys = prevLedger->nUnl(); @@ -85,7 +85,7 @@ NegativeUNLVote::doVoting( findAllCandidates( unlNodeIDs, nUnlNodeIDs, - scoreTable, + *scoreTable, toDisableCandidates, toReEnableCandidates); @@ -123,8 +123,8 @@ NegativeUNLVote::addTx( uint256 txID = nUnlTx.getTransactionID(); Serializer s; nUnlTx.add(s); - auto tItem = std::make_shared(txID, s.peekData()); - if (!initialSet->addGiveItem(tItem, true, false)) + if (!initialSet->addGiveItem( + std::make_shared(txID, s.peekData()), true, false)) { JLOG(j_.warn()) << "N-UNL: ledger seq=" << seq << ", add ttUNL_MODIFY tx failed"; @@ -140,31 +140,33 @@ NegativeUNLVote::addTx( NodeID NegativeUNLVote::pickOneCandidate( - uint256 randomPadData, - std::vector& candidates) + uint256 const& randomPadData, + std::vector const& candidates) { assert(!candidates.empty()); static_assert(NodeID::bytes <= uint256::bytes); NodeID randomPad = NodeID::fromVoid(randomPadData.data()); NodeID txNodeID = candidates[0]; + NodeID cachedTxNodeIDXor = (txNodeID ^ randomPad); for (int j = 1; j < candidates.size(); ++j) { - if ((candidates[j] ^ randomPad) < (txNodeID ^ randomPad)) + auto const thisNodeIDXor = (candidates[j] ^ randomPad); + if (thisNodeIDXor < cachedTxNodeIDXor) { txNodeID = candidates[j]; + cachedTxNodeIDXor = thisNodeIDXor; } } return txNodeID; } -bool +std::optional> NegativeUNLVote::buildScoreTable( LedgerConstPtr& prevLedger, hash_set const& unl, - RCLValidations& validations, - hash_map& scoreTable) + RCLValidations& validations) { - assert(scoreTable.empty()); + hash_map scoreTable; /* * Find agreed validation messages received for the last 256 ledgers, @@ -190,17 +192,17 @@ NegativeUNLVote::buildScoreTable( if (!hashIndex || !hashIndex->isFieldPresent(sfHashes)) { JLOG(j_.debug()) << "N-UNL: ledger " << seq << " no history."; - return false; + return {}; } - auto ledgerAncestors = hashIndex->getFieldV256(sfHashes).value(); - auto numAncestors = ledgerAncestors.size(); + auto const ledgerAncestors = hashIndex->getFieldV256(sfHashes).value(); + auto const numAncestors = ledgerAncestors.size(); if (numAncestors < 256) { JLOG(j_.debug()) << "N-UNL: ledger " << seq << " not enough history. Can trace back only " << numAncestors << " ledgers."; - return false; + return {}; } // have enough ledger ancestors, build the score table @@ -208,32 +210,34 @@ NegativeUNLVote::buildScoreTable( { scoreTable[k] = 0; } - auto idx = numAncestors - 1; for (int i = 0; i < 256; ++i) { - for (auto const& v : - validations.getTrustedForLedger(ledgerAncestors[idx--])) + for (auto const& v : validations.getTrustedForLedger( + ledgerAncestors[numAncestors - i - 1])) { - if (scoreTable.find(v->getNodeID()) != scoreTable.end()) - ++scoreTable[v->getNodeID()]; + auto const it = scoreTable.find(v->getNodeID()); + if (it != scoreTable.end()) + ++it->second; } } - unsigned int myValidationCount = 0; - if (scoreTable.find(myId_) != scoreTable.end()) - myValidationCount = scoreTable[myId_]; + auto const myValidationCount = [&]() -> std::uint32_t { + if (auto const it = scoreTable.find(myId_); it != scoreTable.end()) + return it->second; + return 0; + }(); if (myValidationCount < nUnlMinLocalValsToVote) { JLOG(j_.debug()) << "N-UNL: ledger " << seq << ". I only issued " << myValidationCount << " validations in last 256 ledgers." << " My reliability measurement could be wrong."; - return false; + return {}; } else if ( myValidationCount > nUnlMinLocalValsToVote && myValidationCount <= 256) { - return true; + return std::move(scoreTable); } else { @@ -242,15 +246,15 @@ NegativeUNLVote::buildScoreTable( JLOG(j_.error()) << "N-UNL: ledger " << seq << ". I issued " << myValidationCount << " validations in last 256 ledgers. Too many!"; - return false; + return {}; } } void NegativeUNLVote::findAllCandidates( hash_set const& unl, - hash_set const& nUnl, - hash_map const& scoreTable, + hash_set const& negUnl, + hash_map const& scoreTable, std::vector& toDisableCandidates, std::vector& toReEnableCandidates) { @@ -269,44 +273,46 @@ NegativeUNLVote::findAllCandidates( * (2) if did not find any by (1), try to find candidates: * (a) is in nUnl and (b) is not in unl. */ - auto maxNegativeListed = (std::size_t)std::ceil(unl.size() * nUnlMaxListed); - std::size_t negativeListed = 0; - for (auto const& n : unl) - { - if (nUnl.find(n) != nUnl.end()) - ++negativeListed; - } - bool canAdd = negativeListed < maxNegativeListed; - JLOG(j_.trace()) << "N-UNL: my nodeId " << myId_ << " lowWaterMark " - << nUnlLowWaterMark << " highWaterMark " - << nUnlHighWaterMark << " canAdd " << canAdd - << " negativeListed " << negativeListed - << " maxNegativeListed " << maxNegativeListed; + auto const maxNegativeListed = + static_cast(std::ceil(unl.size() * nUnlMaxListed)); + auto const canAdd = [&]() -> bool { + std::size_t negativeListed = 0; + for (auto const& n : unl) + { + if (negUnl.find(n) != negUnl.end()) + ++negativeListed; + } + bool const result = negativeListed < maxNegativeListed; + JLOG(j_.trace()) << "N-UNL: my nodeId " << myId_ << " lowWaterMark " + << nUnlLowWaterMark << " highWaterMark " + << nUnlHighWaterMark << " canAdd " << result + << " negativeListed " << negativeListed + << " maxNegativeListed " << maxNegativeListed; + return result; + }(); - for (auto it = scoreTable.cbegin(); it != scoreTable.cend(); ++it) + for (auto const& [nodeId, score] : scoreTable) { - JLOG(j_.trace()) << "N-UNL: node " << it->first << " score " - << it->second; + JLOG(j_.trace()) << "N-UNL: node " << nodeId << " score " << score; - if (canAdd && it->second < nUnlLowWaterMark && - nUnl.find(it->first) == nUnl.end() && - newValidators_.find(it->first) == newValidators_.end()) + if (canAdd && score < nUnlLowWaterMark && + negUnl.find(nodeId) == negUnl.end() && + newValidators_.find(nodeId) == newValidators_.end()) { - JLOG(j_.trace()) << "N-UNL: toDisable candidate " << it->first; - toDisableCandidates.push_back(it->first); + JLOG(j_.trace()) << "N-UNL: toDisable candidate " << nodeId; + toDisableCandidates.push_back(nodeId); } - if (it->second > nUnlHighWaterMark && - nUnl.find(it->first) != nUnl.end()) + if (score > nUnlHighWaterMark && negUnl.find(nodeId) != negUnl.end()) { - JLOG(j_.trace()) << "N-UNL: toReEnable candidate " << it->first; - toReEnableCandidates.push_back(it->first); + JLOG(j_.trace()) << "N-UNL: toReEnable candidate " << nodeId; + toReEnableCandidates.push_back(nodeId); } } if (toReEnableCandidates.empty()) { - for (auto const& n : nUnl) + for (auto const& n : negUnl) { if (unl.find(n) == unl.end()) { diff --git a/src/ripple/app/misc/NegativeUNLVote.h b/src/ripple/app/misc/NegativeUNLVote.h index 27e040bd910..ea7016a8b24 100644 --- a/src/ripple/app/misc/NegativeUNLVote.h +++ b/src/ripple/app/misc/NegativeUNLVote.h @@ -21,6 +21,11 @@ #define RIPPLE_APP_MISC_NEGATIVEUNLVOTE_H_INCLUDED #include +#include +#include +#include + +#include namespace ripple { @@ -107,7 +112,7 @@ class NegativeUNLVote final private: NodeID const myId_; beast::Journal j_; - std::mutex mutex_; + mutable std::mutex mutex_; hash_map newValidators_; /** @@ -135,7 +140,9 @@ class NegativeUNLVote final * @return the picked candidate */ NodeID - pickOneCandidate(uint256 randomPadData, std::vector& candidates); + pickOneCandidate( + uint256 const& randomPadData, + std::vector const& candidates); /** * Build a reliability measurement score table of validators' validation @@ -144,22 +151,21 @@ class NegativeUNLVote final * @param prevLedger the parent ledger * @param unl the trusted master keys * @param validations the validation container - * @param scoreTable the score table to be built - * @return if the score table was successfully filled + * @return the built scoreTable or empty optional if table could not be + * built */ - bool + std::optional> buildScoreTable( LedgerConstPtr& prevLedger, hash_set const& unl, - RCLValidations& validations, - hash_map& scoreTable); + RCLValidations& validations); /** * Process the score table and find all disabling and re-enabling * candidates. * * @param unl the trusted master keys - * @param nUnl the NegativeUNL + * @param negUnl the NegativeUNL * @param scoreTable the score table * @param toDisableCandidates the candidates to disable * @param toReEnableCandidates the candidates to re-enable @@ -167,7 +173,7 @@ class NegativeUNLVote final void findAllCandidates( hash_set const& unl, - hash_set const& nUnl, + hash_set const& negUnl, hash_map const& scoreTable, std::vector& toDisableCandidates, std::vector& toReEnableCandidates); diff --git a/src/test/consensus/NegativeUNLVote_test.cpp b/src/test/consensus/NegativeUNLVote_test.cpp index a991af6c014..b8743bc5f7a 100644 --- a/src/test/consensus/NegativeUNLVote_test.cpp +++ b/src/test/consensus/NegativeUNLVote_test.cpp @@ -245,9 +245,8 @@ class NegativeUNLVoteInternal_test : public beast::unit_test::suite { NodeID myId = nodeIDs[3]; NegativeUNLVote vote(myId, env.journal); - hash_map scoreTable; - BEAST_EXPECT(!vote.buildScoreTable( - history[0], UNLNodeIDs, validations, scoreTable)); + BEAST_EXPECT( + !vote.buildScoreTable(history[0], UNLNodeIDs, validations)); } } @@ -268,9 +267,8 @@ class NegativeUNLVoteInternal_test : public beast::unit_test::suite { NodeID myId = nodeIDs[3]; NegativeUNLVote vote(myId, env.journal); - hash_map scoreTable; BEAST_EXPECT(!vote.buildScoreTable( - history.back(), UNLNodeIDs, validations, scoreTable)); + history.back(), UNLNodeIDs, validations)); } } @@ -302,9 +300,8 @@ class NegativeUNLVoteInternal_test : public beast::unit_test::suite } } NegativeUNLVote vote(myId, env.journal); - hash_map scoreTable; BEAST_EXPECT(!vote.buildScoreTable( - history.back(), UNLNodeIDs, validations, scoreTable)); + history.back(), UNLNodeIDs, validations)); } } @@ -336,12 +333,16 @@ class NegativeUNLVoteInternal_test : public beast::unit_test::suite } } NegativeUNLVote vote(myId, env.journal); - hash_map scoreTable; - BEAST_EXPECT(vote.buildScoreTable( - history.back(), UNLNodeIDs, validations, scoreTable)); - for (auto& s : scoreTable) + std::optional> scoreTable = + vote.buildScoreTable( + history.back(), UNLNodeIDs, validations); + BEAST_EXPECT(scoreTable); + if (scoreTable) { - BEAST_EXPECT(s.second == 256); + for (auto& s : *scoreTable) + { + BEAST_EXPECT(s.second == 256); + } } firstRound = history.back(); } @@ -361,15 +362,18 @@ class NegativeUNLVoteInternal_test : public beast::unit_test::suite validations.add(myId, v); } NegativeUNLVote vote(myId, env.journal); - hash_map scoreTable; BEAST_EXPECT(!vote.buildScoreTable( - history.back(), UNLNodeIDs, validations, scoreTable)); - scoreTable.clear(); - BEAST_EXPECT(vote.buildScoreTable( - firstRound, UNLNodeIDs, validations, scoreTable)); - for (auto& s : scoreTable) + history.back(), UNLNodeIDs, validations)); + std::optional> scoreTable = + vote.buildScoreTable( + firstRound, UNLNodeIDs, validations); + BEAST_EXPECT(scoreTable); + if (scoreTable) { - BEAST_EXPECT(s.second == 256); + for (auto& s : *scoreTable) + { + BEAST_EXPECT(s.second == 256); + } } } } @@ -404,7 +408,7 @@ class NegativeUNLVoteInternal_test : public beast::unit_test::suite NegativeUNLVote vote(myId, env.journal); hash_map scoreTable; BEAST_EXPECT(!vote.buildScoreTable( - history.back(), UNLNodeIDs, validations, scoreTable)); + history.back(), UNLNodeIDs, validations)); } } } @@ -935,9 +939,11 @@ class NegativeUNLVoteScoreTable_test : public beast::unit_test::suite } } NegativeUNLVote vote(myId, env.journal); - hash_map scoreTable; - BEAST_EXPECT(vote.buildScoreTable( - history.back(), UNLNodeIDs, validations, scoreTable)); + std::optional> scoreTable = + vote.buildScoreTable( + history.back(), UNLNodeIDs, validations); + BEAST_EXPECT(scoreTable); + std::uint32_t i = 0; // looping unl auto checkScores = [&](std::uint32_t score, std::uint32_t k) -> bool { @@ -953,17 +959,23 @@ class NegativeUNLVoteScoreTable_test : public beast::unit_test::suite assert(0); return false; }; - for (; i < 2; ++i) - { - BEAST_EXPECT(checkScores(scoreTable[nodeIDs[i]], 0)); - } - for (; i < 4; ++i) - { - BEAST_EXPECT(checkScores(scoreTable[nodeIDs[i]], 1)); - } - for (; i < unlSize; ++i) + if (scoreTable) { - BEAST_EXPECT(checkScores(scoreTable[nodeIDs[i]], 2)); + for (; i < 2; ++i) + { + BEAST_EXPECT( + checkScores((*scoreTable)[nodeIDs[i]], 0)); + } + for (; i < 4; ++i) + { + BEAST_EXPECT( + checkScores((*scoreTable)[nodeIDs[i]], 1)); + } + for (; i < unlSize; ++i) + { + BEAST_EXPECT( + checkScores((*scoreTable)[nodeIDs[i]], 2)); + } } } }