Use salted hashing for keys for unordered maps/sets in LLMQ code

We must watch out to not blindly use externally provided keys in unordered
sets/maps, as attackers might find ways to cause unbalanced hash buckets
causing performance degradation.

src/llmq/quorums_blockprocessor.h

@@ -10,6 +10,7 @@
 
 #include "consensus/params.h"
 #include "primitives/transaction.h"
+#include "saltedhasher.h"
 #include "sync.h"
 
 #include <map>
@@ -31,7 +32,7 @@ class CQuorumBlockProcessor
     std::map<std::pair<Consensus::LLMQType, uint256>, uint256> minableCommitmentsByQuorum;
     std::map<uint256, CFinalCommitment> minableCommitments;
 
-    std::unordered_map<std::pair<Consensus::LLMQType, uint256>, bool> hasMinedCommitmentCache;
+    std::unordered_map<std::pair<Consensus::LLMQType, uint256>, bool, StaticSaltedHasher> hasMinedCommitmentCache;
 
 public:
     CQuorumBlockProcessor(CEvoDB& _evoDb) : evoDb(_evoDb) {}

src/llmq/quorums_signing.cpp

@@ -173,10 +173,10 @@ void CRecoveredSigsDb::WriteRecoveredSig(const llmq::CRecoveredSig& recSig)
     }
 }
 
-template<typename K>
-static void TruncateCacheMap(std::unordered_map<K, std::pair<bool, int64_t>>& m, size_t maxSize, size_t truncateThreshold)
+template<typename K, typename H>
+static void TruncateCacheMap(std::unordered_map<K, std::pair<bool, int64_t>, H>& m, size_t maxSize, size_t truncateThreshold)
 {
-    typedef typename std::unordered_map<K, std::pair<bool, int64_t>> Map;
+    typedef typename std::unordered_map<K, std::pair<bool, int64_t>, H> Map;
     typedef typename Map::iterator Iterator;
 
     if (m.size() <= truncateThreshold) {
@@ -377,15 +377,15 @@ bool CSigningManager::PreVerifyRecoveredSig(NodeId nodeId, const CRecoveredSig&
 void CSigningManager::CollectPendingRecoveredSigsToVerify(
         size_t maxUniqueSessions,
         std::unordered_map<NodeId, std::list<CRecoveredSig>>& retSigShares,
-        std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr>& retQuorums)
+        std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher>& retQuorums)
 {
     {
         LOCK(cs);
         if (pendingRecoveredSigs.empty()) {
             return;
         }
 
-        std::unordered_set<std::pair<NodeId, uint256>> uniqueSignHashes;
+        std::unordered_set<std::pair<NodeId, uint256>, StaticSaltedHasher> uniqueSignHashes;
         CLLMQUtils::IterateNodesRandom(pendingRecoveredSigs, [&]() {
             return uniqueSignHashes.size() < maxUniqueSessions;
         }, [&](NodeId nodeId, std::list<CRecoveredSig>& ns) {
@@ -443,7 +443,7 @@ void CSigningManager::CollectPendingRecoveredSigsToVerify(
 bool CSigningManager::ProcessPendingRecoveredSigs(CConnman& connman)
 {
     std::unordered_map<NodeId, std::list<CRecoveredSig>> recSigsByNode;
-    std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr> quorums;
+    std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher> quorums;
 
     CollectPendingRecoveredSigsToVerify(32, recSigsByNode, quorums);
     if (recSigsByNode.empty()) {
@@ -472,7 +472,7 @@ bool CSigningManager::ProcessPendingRecoveredSigs(CConnman& connman)
 
     LogPrint("llmq", "CSigningManager::%s -- verified recovered sig(s). count=%d, vt=%d, nodes=%d\n", __func__, verifyCount, verifyTimer.count(), recSigsByNode.size());
 
-    std::unordered_set<uint256> processed;
+    std::unordered_set<uint256, StaticSaltedHasher> processed;
     for (auto& p : recSigsByNode) {
         NodeId nodeId = p.first;
         auto& v = p.second;

src/llmq/quorums_signing.h

@@ -9,20 +9,10 @@
 
 #include "net.h"
 #include "chainparams.h"
+#include "saltedhasher.h"
 
 #include <unordered_map>
 
-namespace std {
-    template <>
-    struct hash<std::pair<Consensus::LLMQType, uint256>>
-    {
-        std::size_t operator()(const std::pair<Consensus::LLMQType, uint256>& k) const
-        {
-            return (std::size_t)((k.first + 1) * k.second.GetCheapHash());
-        }
-    };
-}
-
 namespace llmq
 {
 
@@ -85,9 +75,9 @@ class CRecoveredSigsDb
     CDBWrapper db;
 
     CCriticalSection cs;
-    std::unordered_map<std::pair<Consensus::LLMQType, uint256>, std::pair<bool, int64_t>> hasSigForIdCache;
-    std::unordered_map<uint256, std::pair<bool, int64_t>> hasSigForSessionCache;
-    std::unordered_map<uint256, std::pair<bool, int64_t>> hasSigForHashCache;
+    std::unordered_map<std::pair<Consensus::LLMQType, uint256>, std::pair<bool, int64_t>, StaticSaltedHasher> hasSigForIdCache;
+    std::unordered_map<uint256, std::pair<bool, int64_t>, StaticSaltedHasher> hasSigForSessionCache;
+    std::unordered_map<uint256, std::pair<bool, int64_t>, StaticSaltedHasher> hasSigForHashCache;
 
 public:
     CRecoveredSigsDb(bool fMemory);
@@ -156,7 +146,9 @@ class CSigningManager
     void ProcessMessageRecoveredSig(CNode* pfrom, const CRecoveredSig& recoveredSig, CConnman& connman);
     bool PreVerifyRecoveredSig(NodeId nodeId, const CRecoveredSig& recoveredSig, bool& retBan);
 
-    void CollectPendingRecoveredSigsToVerify(size_t maxUniqueSessions, std::unordered_map<NodeId, std::list<CRecoveredSig>>& retSigShares, std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr>& retQuorums);
+    void CollectPendingRecoveredSigsToVerify(size_t maxUniqueSessions,
+            std::unordered_map<NodeId, std::list<CRecoveredSig>>& retSigShares,
+            std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher>& retQuorums);
     bool ProcessPendingRecoveredSigs(CConnman& connman); // called from the worker thread of CSigSharesManager
     void ProcessRecoveredSig(NodeId nodeId, const CRecoveredSig& recoveredSig, const CQuorumCPtr& quorum, CConnman& connman);
     void Cleanup(); // called from the worker thread of CSigSharesManager

src/llmq/quorums_signing_shares.cpp

@@ -386,7 +386,7 @@ bool CSigSharesManager::PreVerifyBatchedSigShares(NodeId nodeId, const CBatchedS
 void CSigSharesManager::CollectPendingSigSharesToVerify(
         size_t maxUniqueSessions,
         std::unordered_map<NodeId, std::vector<CSigShare>>& retSigShares,
-        std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr>& retQuorums)
+        std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher>& retQuorums)
 {
     {
         LOCK(cs);
@@ -400,7 +400,7 @@ void CSigSharesManager::CollectPendingSigSharesToVerify(
         // invalid, making batch verification fail and revert to per-share verification, which in turn would slow down
         // the whole verification process
 
-        std::unordered_set<std::pair<NodeId, uint256>> uniqueSignHashes;
+        std::unordered_set<std::pair<NodeId, uint256>, StaticSaltedHasher> uniqueSignHashes;
         CLLMQUtils::IterateNodesRandom(nodeStates, [&]() {
             return uniqueSignHashes.size() < maxUniqueSessions;
         }, [&](NodeId nodeId, CSigSharesNodeState& ns) {
@@ -448,7 +448,7 @@ void CSigSharesManager::CollectPendingSigSharesToVerify(
 bool CSigSharesManager::ProcessPendingSigShares(CConnman& connman)
 {
     std::unordered_map<NodeId, std::vector<CSigShare>> sigSharesByNodes;
-    std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr> quorums;
+    std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher> quorums;
 
     CollectPendingSigSharesToVerify(32, sigSharesByNodes, quorums);
     if (sigSharesByNodes.empty()) {
@@ -517,7 +517,10 @@ bool CSigSharesManager::ProcessPendingSigShares(CConnman& connman)
 }
 
 // It's ensured that no duplicates are passed to this method
-void CSigSharesManager::ProcessPendingSigSharesFromNode(NodeId nodeId, const std::vector<CSigShare>& sigShares, const std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr>& quorums, CConnman& connman)
+void CSigSharesManager::ProcessPendingSigSharesFromNode(NodeId nodeId,
+        const std::vector<CSigShare>& sigShares,
+        const std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher>& quorums,
+        CConnman& connman)
 {
     auto& nodeState = nodeStates[nodeId];
 
@@ -668,11 +671,9 @@ void CSigSharesManager::TryRecoverSig(const CQuorumCPtr& quorum, const uint256&
 }
 
 // cs must be held
-void CSigSharesManager::CollectSigSharesToRequest(std::unordered_map<NodeId, std::unordered_map<uint256, CSigSharesInv>>& sigSharesToRequest)
+void CSigSharesManager::CollectSigSharesToRequest(std::unordered_map<NodeId, std::unordered_map<uint256, CSigSharesInv, StaticSaltedHasher>>& sigSharesToRequest)
 {
     int64_t now = GetTimeMillis();
-    std::unordered_map<SigShareKey, std::vector<NodeId>> nodesBySigShares;
-
     const size_t maxRequestsForNode = 32;
 
     // avoid requesting from same nodes all the time
@@ -703,7 +704,7 @@ void CSigSharesManager::CollectSigSharesToRequest(std::unordered_map<NodeId, std
             return false;
         });
 
-        std::unordered_map<uint256, CSigSharesInv>* invMap = nullptr;
+        decltype(sigSharesToRequest.begin()->second)* invMap = nullptr;
 
         for (auto& p2 : nodeState.sessions) {
             auto& signHash = p2.first;
@@ -764,7 +765,7 @@ void CSigSharesManager::CollectSigSharesToRequest(std::unordered_map<NodeId, std
 }
 
 // cs must be held
-void CSigSharesManager::CollectSigSharesToSend(std::unordered_map<NodeId, std::unordered_map<uint256, CBatchedSigShares>>& sigSharesToSend)
+void CSigSharesManager::CollectSigSharesToSend(std::unordered_map<NodeId, std::unordered_map<uint256, CBatchedSigShares, StaticSaltedHasher>>& sigSharesToSend)
 {
     for (auto& p : nodeStates) {
         auto nodeId = p.first;
@@ -774,7 +775,7 @@ void CSigSharesManager::CollectSigSharesToSend(std::unordered_map<NodeId, std::u
             continue;
         }
 
-        std::unordered_map<uint256, CBatchedSigShares>* sigSharesToSend2 = nullptr;
+        decltype(sigSharesToSend.begin()->second)* sigSharesToSend2 = nullptr;
 
         for (auto& p2 : nodeState.sessions) {
             auto& signHash = p2.first;
@@ -821,9 +822,9 @@ void CSigSharesManager::CollectSigSharesToSend(std::unordered_map<NodeId, std::u
 }
 
 // cs must be held
-void CSigSharesManager::CollectSigSharesToAnnounce(std::unordered_map<NodeId, std::unordered_map<uint256, CSigSharesInv>>& sigSharesToAnnounce)
+void CSigSharesManager::CollectSigSharesToAnnounce(std::unordered_map<NodeId, std::unordered_map<uint256, CSigSharesInv, StaticSaltedHasher>>& sigSharesToAnnounce)
 {
-    std::unordered_set<std::pair<Consensus::LLMQType, uint256>> quorumNodesPrepared;
+    std::unordered_set<std::pair<Consensus::LLMQType, uint256>, StaticSaltedHasher> quorumNodesPrepared;
 
     this->sigSharesToAnnounce.ForEach([&](const SigShareKey& sigShareKey, bool) {
         auto& signHash = sigShareKey.first;
@@ -890,9 +891,9 @@ bool CSigSharesManager::SendMessages()
         nodesByAddress.emplace(pnode->addr, pnode->id);
     });
 
-    std::unordered_map<NodeId, std::unordered_map<uint256, CSigSharesInv>> sigSharesToRequest;
-    std::unordered_map<NodeId, std::unordered_map<uint256, CBatchedSigShares>> sigSharesToSend;
-    std::unordered_map<NodeId, std::unordered_map<uint256, CSigSharesInv>> sigSharesToAnnounce;
+    std::unordered_map<NodeId, std::unordered_map<uint256, CSigSharesInv, StaticSaltedHasher>> sigSharesToRequest;
+    std::unordered_map<NodeId, std::unordered_map<uint256, CBatchedSigShares, StaticSaltedHasher>> sigSharesToSend;
+    std::unordered_map<NodeId, std::unordered_map<uint256, CSigSharesInv, StaticSaltedHasher>> sigSharesToAnnounce;
 
     {
         LOCK(cs);
@@ -956,13 +957,13 @@ void CSigSharesManager::Cleanup()
         return;
     }
 
-    std::unordered_set<std::pair<Consensus::LLMQType, uint256>> quorumsToCheck;
+    std::unordered_set<std::pair<Consensus::LLMQType, uint256>, StaticSaltedHasher> quorumsToCheck;
 
     {
         LOCK(cs);
 
         // Remove sessions which were succesfully recovered
-        std::unordered_set<uint256> doneSessions;
+        std::unordered_set<uint256, StaticSaltedHasher> doneSessions;
         sigShares.ForEach([&](const SigShareKey& k, const CSigShare& sigShare) {
             if (doneSessions.count(sigShare.GetSignHash())) {
                 return;
@@ -976,7 +977,7 @@ void CSigSharesManager::Cleanup()
         }
 
         // Remove sessions which timed out
-        std::unordered_set<uint256> timeoutSessions;
+        std::unordered_set<uint256, StaticSaltedHasher> timeoutSessions;
         for (auto& p : timeSeenForSessions) {
             auto& signHash = p.first;
             int64_t firstSeenTime = p.second.first;
@@ -1020,7 +1021,7 @@ void CSigSharesManager::Cleanup()
     {
         // Now delete sessions which are for inactive quorums
         LOCK(cs);
-        std::unordered_set<uint256> inactiveQuorumSessions;
+        std::unordered_set<uint256, StaticSaltedHasher> inactiveQuorumSessions;
         sigShares.ForEach([&](const SigShareKey& k, const CSigShare& sigShare) {
             if (quorumsToCheck.count(std::make_pair((Consensus::LLMQType)sigShare.llmqType, sigShare.quorumHash))) {
                 inactiveQuorumSessions.emplace(sigShare.GetSignHash());

src/llmq/quorums_signing_shares.h

@@ -9,6 +9,7 @@
 #include "chainparams.h"
 #include "net.h"
 #include "random.h"
+#include "saltedhasher.h"
 #include "serialize.h"
 #include "sync.h"
 #include "tinyformat.h"
@@ -28,29 +29,6 @@ namespace llmq
 {
 // <signHash, quorumMember>
 typedef std::pair<uint256, uint16_t> SigShareKey;
-}
-
-namespace std {
-    template <>
-    struct hash<llmq::SigShareKey>
-    {
-        std::size_t operator()(const llmq::SigShareKey& k) const
-        {
-            return (std::size_t)((k.second + 1) * k.first.GetCheapHash());
-        }
-    };
-    template <>
-    struct hash<std::pair<NodeId, uint256>>
-    {
-        std::size_t operator()(const std::pair<NodeId, uint256>& k) const
-        {
-            return (std::size_t)((k.first + 1) * k.second.GetCheapHash());
-        }
-    };
-}
-
-namespace llmq
-{
 
 // this one does not get transmitted over the wire as it is batched inside CBatchedSigShares
 class CSigShare
@@ -158,7 +136,7 @@ template<typename T>
 class SigShareMap
 {
 private:
-    std::unordered_map<uint256, std::unordered_map<uint16_t, T>> internalMap;
+    std::unordered_map<uint256, std::unordered_map<uint16_t, T>, StaticSaltedHasher> internalMap;
 
 public:
     bool Add(const SigShareKey& k, const T& v)
@@ -308,14 +286,14 @@ class CSigSharesNodeState
         CSigSharesInv knows;
     };
     // TODO limit number of sessions per node
-    std::unordered_map<uint256, Session> sessions;
+    std::unordered_map<uint256, Session, StaticSaltedHasher> sessions;
 
     SigShareMap<CSigShare> pendingIncomingSigShares;
     SigShareMap<int64_t> requestedSigShares;
 
     // elements are added whenever we receive a valid sig share from this node
     // this triggers us to send inventory items to him as he seems to be interested in these
-    std::unordered_set<std::pair<Consensus::LLMQType, uint256>> interestedIn;
+    std::unordered_set<std::pair<Consensus::LLMQType, uint256>, StaticSaltedHasher> interestedIn;
 
     bool banned{false};
 
@@ -347,7 +325,7 @@ class CSigSharesManager : public CRecoveredSigsListener
     SigShareMap<CSigShare> sigShares;
 
     // stores time of first and last receivedSigShare. Used to detect timeouts
-    std::unordered_map<uint256, std::pair<int64_t, int64_t>> timeSeenForSessions;
+    std::unordered_map<uint256, std::pair<int64_t, int64_t>, StaticSaltedHasher> timeSeenForSessions;
 
     std::unordered_map<NodeId, CSigSharesNodeState> nodeStates;
     SigShareMap<std::pair<NodeId, int64_t>> sigSharesRequested;
@@ -386,10 +364,15 @@ class CSigSharesManager : public CRecoveredSigsListener
     bool VerifySigSharesInv(NodeId from, const CSigSharesInv& inv);
     bool PreVerifyBatchedSigShares(NodeId nodeId, const CBatchedSigShares& batchedSigShares, bool& retBan);
 
-    void CollectPendingSigSharesToVerify(size_t maxUniqueSessions, std::unordered_map<NodeId, std::vector<CSigShare>>& retSigShares, std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr>& retQuorums);
+    void CollectPendingSigSharesToVerify(size_t maxUniqueSessions,
+            std::unordered_map<NodeId, std::vector<CSigShare>>& retSigShares,
+            std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher>& retQuorums);
     bool ProcessPendingSigShares(CConnman& connman);
 
-    void ProcessPendingSigSharesFromNode(NodeId nodeId, const std::vector<CSigShare>& sigShares, const std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr>& quorums, CConnman& connman);
+    void ProcessPendingSigSharesFromNode(NodeId nodeId,
+            const std::vector<CSigShare>& sigShares,
+            const std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher>& quorums,
+            CConnman& connman);
 
     void ProcessSigShare(NodeId nodeId, const CSigShare& sigShare, CConnman& connman, const CQuorumCPtr& quorum);
     void TryRecoverSig(const CQuorumCPtr& quorum, const uint256& id, const uint256& msgHash, CConnman& connman);
@@ -402,9 +385,9 @@ class CSigSharesManager : public CRecoveredSigsListener
     void BanNode(NodeId nodeId);
 
     bool SendMessages();
-    void CollectSigSharesToRequest(std::unordered_map<NodeId, std::unordered_map<uint256, CSigSharesInv>>& sigSharesToRequest);
-    void CollectSigSharesToSend(std::unordered_map<NodeId, std::unordered_map<uint256, CBatchedSigShares>>& sigSharesToSend);
-    void CollectSigSharesToAnnounce(std::unordered_map<NodeId, std::unordered_map<uint256, CSigSharesInv>>& sigSharesToAnnounce);
+    void CollectSigSharesToRequest(std::unordered_map<NodeId, std::unordered_map<uint256, CSigSharesInv, StaticSaltedHasher>>& sigSharesToRequest);
+    void CollectSigSharesToSend(std::unordered_map<NodeId, std::unordered_map<uint256, CBatchedSigShares, StaticSaltedHasher>>& sigSharesToSend);
+    void CollectSigSharesToAnnounce(std::unordered_map<NodeId, std::unordered_map<uint256, CSigSharesInv, StaticSaltedHasher>>& sigSharesToAnnounce);
     bool SignPendingSigShares();
     void WorkThreadMain();
 };