refactor: add helper function to sign messages with blsKeyOperator

Avoid passing around the operator secret key if we can help it. Ask CActiveMasternodeManager to perform the operation for you instead.
vijaydasmp · Mar 24, 2024 · 3eb931b · 3eb931b
1 parent 3827355
commit 3eb931b
Show file tree

Hide file tree

Showing 5 changed files with 54 additions and 34 deletions.
diff --git a/src/coinjoin/coinjoin.cpp b/src/coinjoin/coinjoin.cpp
@@ -52,7 +52,7 @@ bool CCoinJoinQueue::Sign()
     if (!fMasternodeMode) return false;
 
     uint256 hash = GetSignatureHash();
-    CBLSSignature sig = WITH_LOCK(::activeMasternodeManager->cs, return ::activeMasternodeManager->m_info.blsKeyOperator->Sign(hash, false));
+    CBLSSignature sig = ::activeMasternodeManager->Sign(hash, /*is_legacy=*/ false);
     if (!sig.IsValid()) {
         return false;
     }
@@ -104,7 +104,7 @@ bool CCoinJoinBroadcastTx::Sign()
     if (!fMasternodeMode) return false;
 
     uint256 hash = GetSignatureHash();
-    CBLSSignature sig = WITH_LOCK(::activeMasternodeManager->cs, return ::activeMasternodeManager->m_info.blsKeyOperator->Sign(hash, false));
+    CBLSSignature sig = ::activeMasternodeManager->Sign(hash, /*is_legacy=*/ false);
     if (!sig.IsValid()) {
         return false;
     }

diff --git a/src/evo/mnauth.cpp b/src/evo/mnauth.cpp
@@ -23,38 +23,42 @@ void CMNAuth::PushMNAUTH(CNode& peer, CConnman& connman, const CBlockIndex* tip)
 {
     if (!fMasternodeMode) return;
 
-    LOCK(::activeMasternodeManager->cs);
-    if (::activeMasternodeManager->m_info.proTxHash.IsNull()) return;
-
+    CMNAuth mnauth;
     uint256 signHash;
-    const auto receivedMNAuthChallenge = peer.GetReceivedMNAuthChallenge();
-    if (receivedMNAuthChallenge.IsNull()) {
-        return;
-    }
-    // We include fInbound in signHash to forbid interchanging of challenges by a man in the middle (MITM). This way
-    // we protect ourselves against MITM in this form:
-    //   node1 <- Eve -> node2
-    // It does not protect against:
-    //   node1 -> Eve -> node2
-    // This is ok as we only use MNAUTH as a DoS protection and not for sensitive stuff
-    int nOurNodeVersion{PROTOCOL_VERSION};
-    if (Params().NetworkIDString() != CBaseChainParams::MAIN && gArgs.IsArgSet("-pushversion")) {
-        nOurNodeVersion = gArgs.GetArg("-pushversion", PROTOCOL_VERSION);
-    }
-    const bool is_basic_scheme_active{DeploymentActiveAfter(tip, Params().GetConsensus(), Consensus::DEPLOYMENT_V19)};
-    const CBLSPublicKeyVersionWrapper pubKey(*::activeMasternodeManager->m_info.blsPubKeyOperator, !is_basic_scheme_active);
-    if (peer.nVersion < MNAUTH_NODE_VER_VERSION || nOurNodeVersion < MNAUTH_NODE_VER_VERSION) {
-        signHash = ::SerializeHash(std::make_tuple(pubKey, receivedMNAuthChallenge, peer.IsInboundConn()));
-    } else {
-        signHash = ::SerializeHash(std::make_tuple(pubKey, receivedMNAuthChallenge, peer.IsInboundConn(), nOurNodeVersion));
-    }
+    {
+        LOCK(::activeMasternodeManager->cs);
+        if (::activeMasternodeManager->m_info.proTxHash.IsNull()) {
+            return;
+        }
 
-    CMNAuth mnauth;
-    mnauth.proRegTxHash = ::activeMasternodeManager->m_info.proTxHash;
-    mnauth.sig = ::activeMasternodeManager->m_info.blsKeyOperator->Sign(signHash);
+        const auto receivedMNAuthChallenge = peer.GetReceivedMNAuthChallenge();
+        if (receivedMNAuthChallenge.IsNull()) {
+            return;
+        }
+        // We include fInbound in signHash to forbid interchanging of challenges by a man in the middle (MITM). This way
+        // we protect ourselves against MITM in this form:
+        //   node1 <- Eve -> node2
+        // It does not protect against:
+        //   node1 -> Eve -> node2
+        // This is ok as we only use MNAUTH as a DoS protection and not for sensitive stuff
+        int nOurNodeVersion{PROTOCOL_VERSION};
+        if (Params().NetworkIDString() != CBaseChainParams::MAIN && gArgs.IsArgSet("-pushversion")) {
+            nOurNodeVersion = gArgs.GetArg("-pushversion", PROTOCOL_VERSION);
+        }
+        const bool is_basic_scheme_active{DeploymentActiveAfter(tip, Params().GetConsensus(), Consensus::DEPLOYMENT_V19)};
+        const CBLSPublicKeyVersionWrapper pubKey(*::activeMasternodeManager->m_info.blsPubKeyOperator, !is_basic_scheme_active);
+        if (peer.nVersion < MNAUTH_NODE_VER_VERSION || nOurNodeVersion < MNAUTH_NODE_VER_VERSION) {
+            signHash = ::SerializeHash(std::make_tuple(pubKey, receivedMNAuthChallenge, peer.IsInboundConn()));
+        } else {
+            signHash = ::SerializeHash(std::make_tuple(pubKey, receivedMNAuthChallenge, peer.IsInboundConn(), nOurNodeVersion));
+        }
 
-    LogPrint(BCLog::NET_NETCONN, "CMNAuth::%s -- Sending MNAUTH, peer=%d\n", __func__, peer.GetId());
+        mnauth.proRegTxHash = ::activeMasternodeManager->m_info.proTxHash;
+    } // ::activeMasternodeManager->cs
 
+    mnauth.sig = ::activeMasternodeManager->Sign(signHash);
+
+    LogPrint(BCLog::NET_NETCONN, "CMNAuth::%s -- Sending MNAUTH, peer=%d\n", __func__, peer.GetId());
     connman.PushMessage(&peer, CNetMsgMaker(peer.GetCommonVersion()).Make(NetMsgType::MNAUTH, mnauth));
 }
 

diff --git a/src/llmq/dkgsession.cpp b/src/llmq/dkgsession.cpp
@@ -199,7 +199,7 @@ void CDKGSession::SendContributions(CDKGPendingMessages& pendingMessages)
 
     logger.Batch("encrypted contributions. time=%d", t1.count());
 
-    qc.sig = WITH_LOCK(::activeMasternodeManager->cs, return ::activeMasternodeManager->m_info.blsKeyOperator->Sign(qc.GetSignHash()));
+    qc.sig = ::activeMasternodeManager->Sign(qc.GetSignHash());
 
     logger.Flush();
 
@@ -517,7 +517,7 @@ void CDKGSession::SendComplaint(CDKGPendingMessages& pendingMessages)
 
     logger.Batch("sending complaint. badCount=%d, complaintCount=%d", badCount, complaintCount);
 
-    qc.sig = WITH_LOCK(::activeMasternodeManager->cs, return ::activeMasternodeManager->m_info.blsKeyOperator->Sign(qc.GetSignHash()));
+    qc.sig = ::activeMasternodeManager->Sign(qc.GetSignHash());
 
     logger.Flush();
 
@@ -711,7 +711,7 @@ void CDKGSession::SendJustification(CDKGPendingMessages& pendingMessages, const
         return;
     }
 
-    qj.sig = WITH_LOCK(::activeMasternodeManager->cs, return ::activeMasternodeManager->m_info.blsKeyOperator->Sign(qj.GetSignHash()));
+    qj.sig = ::activeMasternodeManager->Sign(qj.GetSignHash());
 
     logger.Flush();
 
@@ -1003,7 +1003,7 @@ void CDKGSession::SendCommitment(CDKGPendingMessages& pendingMessages)
         (*commitmentHash.begin())++;
     }
 
-    qc.sig = WITH_LOCK(::activeMasternodeManager->cs, return ::activeMasternodeManager->m_info.blsKeyOperator->Sign(commitmentHash));
+    qc.sig = ::activeMasternodeManager->Sign(commitmentHash);
     qc.quorumSig = skShare.Sign(commitmentHash);
 
     if (lieType == 3) {

diff --git a/src/masternode/node.cpp b/src/masternode/node.cpp
@@ -251,3 +251,15 @@ bool CActiveMasternodeManager::IsValidNetAddr(const CService& addrIn)
     return !Params().RequireRoutableExternalIP() ||
            (addrIn.IsIPv4() && IsReachable(addrIn) && addrIn.IsRoutable());
 }
+
+[[nodiscard]] CBLSSignature CActiveMasternodeManager::Sign(const uint256& hash) const
+{
+    AssertLockNotHeld(cs);
+    return WITH_LOCK(cs, return Assert(m_info.blsKeyOperator)->Sign(hash));
+}
+
+[[nodiscard]] CBLSSignature CActiveMasternodeManager::Sign(const uint256& hash, const bool is_legacy) const
+{
+    AssertLockNotHeld(cs);
+    return WITH_LOCK(cs, return Assert(m_info.blsKeyOperator)->Sign(hash, is_legacy));
+}
diff --git a/src/masternode/node.h b/src/masternode/node.h
@@ -12,6 +12,7 @@
 
 class CBLSPublicKey;
 class CBLSSecretKey;
+class CBLSSignature;
 class CDeterministicMNManager;
 
 struct CActiveMasternodeInfo {
@@ -63,6 +64,9 @@ class CActiveMasternodeManager final : public CValidationInterface
 
     static bool IsValidNetAddr(const CService& addrIn);
 
+    [[nodiscard]] CBLSSignature Sign(const uint256& hash) const LOCKS_EXCLUDED(cs);
+    [[nodiscard]] CBLSSignature Sign(const uint256& hash, const bool is_legacy) const LOCKS_EXCLUDED(cs);
+
 private:
     bool GetLocalAddress(CService& addrRet);
 };