From c75d53454d89561c61ec057182dcd9429e724a5d Mon Sep 17 00:00:00 2001
From: Alexander Block <ablock84@gmail.com>
Date: Mon, 11 Mar 2019 08:07:52 +0100
Subject: [PATCH] Fix deadlock in CSigSharesManager::SendMessages

Locking "cs" at this location caused a (potential) deadlock due to changed
order of cs and cs_vNodes locking. This changes the method to not require
the session object anymore which removes the need for locking.
---
 src/llmq/quorums_signing_shares.cpp | 19 ++++++++++---------
 src/llmq/quorums_signing_shares.h   |  1 +
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/src/llmq/quorums_signing_shares.cpp b/src/llmq/quorums_signing_shares.cpp
index ca52d2933884e9..a3efcaefb291f0 100644
--- a/src/llmq/quorums_signing_shares.cpp
+++ b/src/llmq/quorums_signing_shares.cpp
@@ -93,6 +93,13 @@ CSigSharesInv CBatchedSigShares::ToInv(Consensus::LLMQType llmqType) const
     return inv;
 }
 
+std::string CBatchedSigShares::ToInvString() const
+{
+    // we use LLMQ_400_60 here no matter what the real type is. The llmqType is only used to properly initialize the
+    // inv object's bitset. We don't really care about that size as we just want to call ToString()
+    return ToInv(Consensus::LLMQ_400_60).ToString();
+}
+
 template<typename T>
 static void InitSession(CSigSharesNodeState::Session& s, const uint256& signHash, T& from)
 {
@@ -443,7 +450,7 @@ bool CSigSharesManager::ProcessMessageBatchedSigShares(CNode* pfrom, const CBatc
     }
 
     LogPrint("llmq", "CSigSharesManager::%s -- signHash=%s, shares=%d, new=%d, inv={%s}, node=%d\n", __func__,
-             sessionInfo.signHash.ToString(), batchedSigShares.sigShares.size(), sigShares.size(), batchedSigShares.ToInv(sessionInfo.llmqType).ToString(), pfrom->id);
+             sessionInfo.signHash.ToString(), batchedSigShares.sigShares.size(), sigShares.size(), batchedSigShares.ToInvString(), pfrom->id);
 
     if (sigShares.empty()) {
         return true;
@@ -1101,14 +1108,8 @@ bool CSigSharesManager::SendMessages()
             std::vector<CBatchedSigShares> msgs;
             for (auto& p : jt->second) {
                 assert(!p.second.sigShares.empty());
-                if (LogAcceptCategory("llmq")) {
-                    LOCK(cs);
-                    auto session = nodeStates[pnode->id].GetSessionBySignHash(p.first);
-                    assert(session);
-                    LogPrint("llmq", "CSigSharesManager::SendMessages -- QBSIGSHARES signHash=%s, inv={%s}, node=%d\n",
-                             p.first.ToString(), p.second.ToInv(session->llmqType).ToString(), pnode->id);
-                }
-
+                LogPrint("llmq", "CSigSharesManager::SendMessages -- QBSIGSHARES signHash=%s, inv={%s}, node=%d\n",
+                         p.first.ToString(), p.second.ToInvString(), pnode->id);
                 if (totalSigsCount + p.second.sigShares.size() > MAX_MSGS_TOTAL_BATCHED_SIGS) {
                     g_connman->PushMessage(pnode, msgMaker.Make(NetMsgType::QBSIGSHARES, msgs), false);
                     msgs.clear();
diff --git a/src/llmq/quorums_signing_shares.h b/src/llmq/quorums_signing_shares.h
index beb781589d72e3..570d729941c13f 100644
--- a/src/llmq/quorums_signing_shares.h
+++ b/src/llmq/quorums_signing_shares.h
@@ -128,6 +128,7 @@ class CBatchedSigShares
     }
 
     CSigSharesInv ToInv(Consensus::LLMQType llmqType) const;
+    std::string ToInvString() const;
 };
 
 template<typename T>