WIP Reviewing

src/ripple/app/misc/NegativeUNLVote.cpp

@@ -54,8 +54,8 @@ NegativeUNLVote::doVoting(
         unlNodeIDs.emplace(nid);
     }
 
-    hash_map<NodeID, unsigned int> scoreTable;
-    if (buildScoreTable(prevLedger, unlNodeIDs, validations, scoreTable))
+    if (std::optional<hash_map<NodeID, std::uint32_t>> 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<SHAMapItem>(txID, s.peekData());
-    if (!initialSet->addGiveItem(tItem, true, false))
+    if (!initialSet->addGiveItem(
+            std::make_shared<SHAMapItem>(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<NodeID>& candidates)
+    uint256 const& randomPadData,
+    std::vector<NodeID> 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<hash_map<NodeID, std::uint32_t>>
 NegativeUNLVote::buildScoreTable(
     LedgerConstPtr& prevLedger,
     hash_set<NodeID> const& unl,
-    RCLValidations& validations,
-    hash_map<NodeID, unsigned int>& scoreTable)
+    RCLValidations& validations)
 {
-    assert(scoreTable.empty());
+    hash_map<NodeID, std::uint32_t> scoreTable;
 
     /*
      * Find agreed validation messages received for the last 256 ledgers,
@@ -190,50 +192,52 @@ 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
     for (auto const& k : unl)
     {
         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<NodeID> const& unl,
-    hash_set<NodeID> const& nUnl,
-    hash_map<NodeID, unsigned int> const& scoreTable,
+    hash_set<NodeID> const& negUnl,
+    hash_map<NodeID, std::uint32_t> const& scoreTable,
     std::vector<NodeID>& toDisableCandidates,
     std::vector<NodeID>& 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::size_t>(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())
             {

src/ripple/app/misc/NegativeUNLVote.h

@@ -21,6 +21,11 @@
 #define RIPPLE_APP_MISC_NEGATIVEUNLVOTE_H_INCLUDED
 
 #include <ripple/beast/utility/Journal.h>
+#include <ripple/protocol/Protocol.h>
+#include <ripple/protocol/PublicKey.h>
+#include <ripple/protocol/UintTypes.h>
+
+#include <optional>
 
 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<NodeID, LedgerIndex> newValidators_;
 
     /**
@@ -135,7 +140,9 @@ class NegativeUNLVote final
      * @return the picked candidate
      */
     NodeID
-    pickOneCandidate(uint256 randomPadData, std::vector<NodeID>& candidates);
+    pickOneCandidate(
+        uint256 const& randomPadData,
+        std::vector<NodeID> const& candidates);
 
     /**
      * Build a reliability measurement score table of validators' validation
@@ -144,30 +151,29 @@ 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<hash_map<NodeID, unsigned int>>
     buildScoreTable(
         LedgerConstPtr& prevLedger,
         hash_set<NodeID> const& unl,
-        RCLValidations& validations,
-        hash_map<NodeID, unsigned int>& 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
      */
     void
     findAllCandidates(
         hash_set<NodeID> const& unl,
-        hash_set<NodeID> const& nUnl,
+        hash_set<NodeID> const& negUnl,
         hash_map<NodeID, unsigned int> const& scoreTable,
         std::vector<NodeID>& toDisableCandidates,
         std::vector<NodeID>& toReEnableCandidates);