diff --git a/src/coinjoin/server.cpp b/src/coinjoin/server.cpp index c0ed66f4e4..148fa6b309 100644 --- a/src/coinjoin/server.cpp +++ b/src/coinjoin/server.cpp @@ -58,7 +58,7 @@ void CCoinJoinServer::ProcessDSACCEPT(CNode& peer, CDataStream& vRecv) LogPrint(BCLog::COINJOIN, "DSACCEPT -- nDenom %d (%s) txCollateral %s", dsa.nDenom, CoinJoin::DenominationToString(dsa.nDenom), dsa.txCollateral.ToString()); /* Continued */ auto mnList = m_dmnman.GetListAtChainTip(); - auto dmn = WITH_LOCK(m_mn_activeman->cs, return mnList.GetValidMNByCollateral(m_mn_activeman->GetOutPoint())); + auto dmn = mnList.GetValidMNByCollateral(m_mn_activeman->GetOutPoint()); if (!dmn) { PushStatus(peer, STATUS_REJECTED, ERR_MN_LIST); return; @@ -69,7 +69,7 @@ void CCoinJoinServer::ProcessDSACCEPT(CNode& peer, CDataStream& vRecv) TRY_LOCK(cs_vecqueue, lockRecv); if (!lockRecv) return; - auto mnOutpoint = WITH_LOCK(m_mn_activeman->cs, return m_mn_activeman->GetOutPoint()); + auto mnOutpoint = m_mn_activeman->GetOutPoint(); if (ranges::any_of(vecCoinJoinQueue, [&mnOutpoint](const auto& q){return q.masternodeOutpoint == mnOutpoint;})) { @@ -335,8 +335,8 @@ void CCoinJoinServer::CommitFinalTransaction() // create and sign masternode dstx transaction if (!m_dstxman.GetDSTX(hashTx)) { CCoinJoinBroadcastTx dstxNew(finalTransaction, - WITH_LOCK(m_mn_activeman->cs, return m_mn_activeman->GetOutPoint()), - WITH_LOCK(m_mn_activeman->cs, return m_mn_activeman->GetProTxHash()), + m_mn_activeman->GetOutPoint(), + m_mn_activeman->GetProTxHash(), GetAdjustedTime()); dstxNew.Sign(*m_mn_activeman); m_dstxman.AddDSTX(dstxNew); @@ -505,8 +505,8 @@ void CCoinJoinServer::CheckForCompleteQueue() SetState(POOL_STATE_ACCEPTING_ENTRIES); CCoinJoinQueue dsq(nSessionDenom, - WITH_LOCK(m_mn_activeman->cs, return m_mn_activeman->GetOutPoint()), - WITH_LOCK(m_mn_activeman->cs, return m_mn_activeman->GetProTxHash()), + m_mn_activeman->GetOutPoint(), + m_mn_activeman->GetProTxHash(), GetAdjustedTime(), true); LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckForCompleteQueue -- queue is ready, signing and relaying (%s) " /* Continued */ "with %d participants\n", dsq.ToString(), vecSessionCollaterals.size()); @@ -721,8 +721,8 @@ bool CCoinJoinServer::CreateNewSession(const CCoinJoinAccept& dsa, PoolMessage& if (!fUnitTest) { //broadcast that I'm accepting entries, only if it's the first entry through CCoinJoinQueue dsq(nSessionDenom, - WITH_LOCK(m_mn_activeman->cs, return m_mn_activeman->GetOutPoint()), - WITH_LOCK(m_mn_activeman->cs, return m_mn_activeman->GetProTxHash()), + m_mn_activeman->GetOutPoint(), + m_mn_activeman->GetProTxHash(), GetAdjustedTime(), false); LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CreateNewSession -- signing and relaying new queue: %s\n", dsq.ToString()); dsq.Sign(*m_mn_activeman); diff --git a/src/evo/mnauth.cpp b/src/evo/mnauth.cpp index 67227af21c..e87ee56163 100644 --- a/src/evo/mnauth.cpp +++ b/src/evo/mnauth.cpp @@ -24,38 +24,36 @@ void CMNAuth::PushMNAUTH(CNode& peer, CConnman& connman, const CBlockIndex* tip) if (!fMasternodeMode) return; CMNAuth mnauth; - uint256 signHash; - { - LOCK(::activeMasternodeManager->cs); - if (::activeMasternodeManager->GetProTxHash().IsNull()) { - return; - } + if (::activeMasternodeManager->GetProTxHash().IsNull()) { + return; + } - 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)}; - auto pk = ::activeMasternodeManager->GetPubKey(); - const CBLSPublicKeyVersionWrapper pubKey(pk, !is_basic_scheme_active); + 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)}; + auto pk = ::activeMasternodeManager->GetPubKey(); + const CBLSPublicKeyVersionWrapper pubKey(pk, !is_basic_scheme_active); + uint256 signHash = [&]() { if (peer.nVersion < MNAUTH_NODE_VER_VERSION || nOurNodeVersion < MNAUTH_NODE_VER_VERSION) { - signHash = ::SerializeHash(std::make_tuple(pubKey, receivedMNAuthChallenge, peer.IsInboundConn())); + return ::SerializeHash(std::make_tuple(pubKey, receivedMNAuthChallenge, peer.IsInboundConn())); } else { - signHash = ::SerializeHash(std::make_tuple(pubKey, receivedMNAuthChallenge, peer.IsInboundConn(), nOurNodeVersion)); + return ::SerializeHash(std::make_tuple(pubKey, receivedMNAuthChallenge, peer.IsInboundConn(), nOurNodeVersion)); } + }(); - mnauth.proRegTxHash = ::activeMasternodeManager->GetProTxHash(); - } // ::activeMasternodeManager->cs + mnauth.proRegTxHash = ::activeMasternodeManager->GetProTxHash(); mnauth.sig = ::activeMasternodeManager->Sign(signHash); @@ -133,7 +131,7 @@ PeerMsgRet CMNAuth::ProcessMessage(CNode& peer, CConnman& connman, const CDeterm } const uint256 myProTxHash = fMasternodeMode ? - WITH_LOCK(::activeMasternodeManager->cs, return ::activeMasternodeManager->GetProTxHash()) : + ::activeMasternodeManager->GetProTxHash() : uint256(); connman.ForEachNode([&](CNode* pnode2) { diff --git a/src/governance/governance.cpp b/src/governance/governance.cpp index 923ef2bdec..b7569017f7 100644 --- a/src/governance/governance.cpp +++ b/src/governance/governance.cpp @@ -691,14 +691,11 @@ std::optional CGovernanceManager::CreateGovernanceTrigg return std::nullopt; } - { - LOCK(::activeMasternodeManager->cs); - if (mn_payees.front()->proTxHash != ::activeMasternodeManager->GetProTxHash()) { - LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s we are not the payee, skipping\n", __func__); - return std::nullopt; - } - gov_sb.SetMasternodeOutpoint(::activeMasternodeManager->GetOutPoint()); - } // ::activeMasternodeManager->cs + if (mn_payees.front()->proTxHash != ::activeMasternodeManager->GetProTxHash()) { + LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s we are not the payee, skipping\n", __func__); + return std::nullopt; + } + gov_sb.SetMasternodeOutpoint(::activeMasternodeManager->GetOutPoint()); gov_sb.Sign(*::activeMasternodeManager); if (std::string strError; !gov_sb.IsValidLocally(m_dmnman->GetListAtChainTip(), strError, true)) { @@ -720,7 +717,7 @@ void CGovernanceManager::VoteGovernanceTriggers(const std::optionalcs, return ::activeMasternodeManager->GetProTxHash().IsNull())) return; + if (::activeMasternodeManager->GetProTxHash().IsNull()) return; LOCK2(cs_main, cs); @@ -763,7 +760,7 @@ void CGovernanceManager::VoteGovernanceTriggers(const std::optionalcs, return ::activeMasternodeManager->GetOutPoint()), nHash, VOTE_SIGNAL_FUNDING, outcome); + CGovernanceVote vote(::activeMasternodeManager->GetOutPoint(), nHash, VOTE_SIGNAL_FUNDING, outcome); vote.SetTime(GetAdjustedTime()); vote.Sign(*::activeMasternodeManager); diff --git a/src/llmq/dkgsessionhandler.cpp b/src/llmq/dkgsessionhandler.cpp index 3edf5446e8..2474100727 100644 --- a/src/llmq/dkgsessionhandler.cpp +++ b/src/llmq/dkgsessionhandler.cpp @@ -194,7 +194,7 @@ bool CDKGSessionHandler::InitNewQuorum(const CBlockIndex* pQuorumBaseBlockIndex) } auto mns = utils::GetAllQuorumMembers(params.type, m_dmnman, pQuorumBaseBlockIndex); - if (!curSession->Init(pQuorumBaseBlockIndex, mns, WITH_LOCK(m_mn_activeman->cs, return m_mn_activeman->GetProTxHash()), quorumIndex)) { + if (!curSession->Init(pQuorumBaseBlockIndex, mns, m_mn_activeman->GetProTxHash(), quorumIndex)) { LogPrintf("CDKGSessionManager::%s -- height[%d] quorum initialization failed for %s qi[%d] mns[%d]\n", __func__, pQuorumBaseBlockIndex->nHeight, curSession->params.name, quorumIndex, mns.size()); return false; } diff --git a/src/llmq/quorums.cpp b/src/llmq/quorums.cpp index 8b6e7205ab..1aa34122b9 100644 --- a/src/llmq/quorums.cpp +++ b/src/llmq/quorums.cpp @@ -103,7 +103,7 @@ bool CQuorum::SetVerificationVector(const std::vector& quorumVecI bool CQuorum::SetSecretKeyShare(const CBLSSecretKey& secretKeyShare, const CActiveMasternodeManager& mn_activeman) { - if (!secretKeyShare.IsValid() || (secretKeyShare.GetPublicKey() != GetPubKeyShare(WITH_LOCK(mn_activeman.cs, return GetMemberIndex(mn_activeman.GetProTxHash()))))) { + if (!secretKeyShare.IsValid() || (secretKeyShare.GetPublicKey() != GetPubKeyShare(GetMemberIndex(mn_activeman.GetProTxHash())))) { return false; } LOCK(cs); @@ -256,7 +256,7 @@ void CQuorumManager::TriggerQuorumDataRecoveryThreads(const CBlockIndex* pIndex) const uint256 proTxHash = [this]() { if (!fMasternodeMode) return uint256(); assert(m_mn_activeman); - return WITH_LOCK(m_mn_activeman->cs, return m_mn_activeman->GetProTxHash()); + return m_mn_activeman->GetProTxHash(); }(); bool fWeAreQuorumTypeMember = ranges::any_of(vecQuorums, [&proTxHash](const auto& pQuorum) { @@ -352,7 +352,7 @@ void CQuorumManager::CheckQuorumConnections(const Consensus::LLMQParams& llmqPar const uint256 myProTxHash = [this]() { if (!fMasternodeMode) return uint256(); assert(m_mn_activeman); - return WITH_LOCK(m_mn_activeman->cs, return m_mn_activeman->GetProTxHash()); + return m_mn_activeman->GetProTxHash(); }(); bool isISType = llmqParams.type == Params().GetConsensus().llmqTypeDIP0024InstantSend; @@ -664,10 +664,10 @@ size_t CQuorumManager::GetQuorumRecoveryStartOffset(const CQuorumCPtr pQuorum, c std::sort(vecProTxHashes.begin(), vecProTxHashes.end()); size_t nIndex{0}; { - LOCK(m_mn_activeman->cs); + auto my_protx_hash = m_mn_activeman->GetProTxHash(); for (const auto i : irange::range(vecProTxHashes.size())) { // cppcheck-suppress useStlAlgorithm - if (m_mn_activeman->GetProTxHash() == vecProTxHashes[i]) { + if (my_protx_hash == vecProTxHashes[i]) { nIndex = i; break; } @@ -928,7 +928,7 @@ void CQuorumManager::StartQuorumDataRecoveryThread(const CQuorumCPtr pQuorum, co vecMemberHashes.reserve(pQuorum->qc->validMembers.size()); for (auto& member : pQuorum->members) { - if (pQuorum->IsValidMember(member->proTxHash) && member->proTxHash != WITH_LOCK(m_mn_activeman->cs, return m_mn_activeman->GetProTxHash())) { + if (pQuorum->IsValidMember(member->proTxHash) && member->proTxHash != m_mn_activeman->GetProTxHash()) { vecMemberHashes.push_back(member->proTxHash); } } @@ -977,7 +977,7 @@ void CQuorumManager::StartQuorumDataRecoveryThread(const CQuorumCPtr pQuorum, co printLog("Connect"); } - auto proTxHash = WITH_LOCK(m_mn_activeman->cs, return m_mn_activeman->GetProTxHash()); + auto proTxHash = m_mn_activeman->GetProTxHash(); connman.ForEachNode([&](CNode* pNode) { auto verifiedProRegTxHash = pNode->GetVerifiedProRegTxHash(); if (pCurrentMemberHash == nullptr || verifiedProRegTxHash != *pCurrentMemberHash) { diff --git a/src/llmq/signing.cpp b/src/llmq/signing.cpp index f088895773..afd2c52cf3 100644 --- a/src/llmq/signing.cpp +++ b/src/llmq/signing.cpp @@ -898,7 +898,7 @@ bool CSigningManager::AsyncSignIfMember(Consensus::LLMQType llmqType, CSigShares if (!fMasternodeMode) return false; assert(m_mn_activeman); - if (WITH_LOCK(m_mn_activeman->cs, return m_mn_activeman->GetProTxHash().IsNull())) return false; + if (m_mn_activeman->GetProTxHash().IsNull()) return false; const CQuorumCPtr quorum = [&]() { if (quorumHash.IsNull()) { @@ -920,7 +920,7 @@ bool CSigningManager::AsyncSignIfMember(Consensus::LLMQType llmqType, CSigShares return false; } - if (!WITH_LOCK(m_mn_activeman->cs, return quorum->IsValidMember(m_mn_activeman->GetProTxHash()))) { + if (!quorum->IsValidMember(m_mn_activeman->GetProTxHash())) { return false; } diff --git a/src/llmq/signing_shares.cpp b/src/llmq/signing_shares.cpp index 395e7d1723..50cb75e832 100644 --- a/src/llmq/signing_shares.cpp +++ b/src/llmq/signing_shares.cpp @@ -221,7 +221,7 @@ void CSigSharesManager::ProcessMessage(const CNode& pfrom, const CSporkManager& if (!fMasternodeMode) return; assert(m_mn_activeman); - if (WITH_LOCK(m_mn_activeman->cs, return m_mn_activeman->GetProTxHash().IsNull())) return; + if (m_mn_activeman->GetProTxHash().IsNull()) return; if (sporkman.IsSporkActive(SPORK_21_QUORUM_ALL_CONNECTED) && msg_type == NetMsgType::QSIGSHARE) { std::vector receivedSigShares; @@ -468,7 +468,7 @@ void CSigSharesManager::ProcessMessageSigShare(NodeId fromId, const CSigShare& s // quorum is too old return; } - if (!quorum->IsMember(WITH_LOCK(m_mn_activeman->cs, return m_mn_activeman->GetProTxHash()))) { + if (!quorum->IsMember(m_mn_activeman->GetProTxHash())) { // we're not a member so we can't verify it (we actually shouldn't have received it) return; } @@ -518,7 +518,7 @@ bool CSigSharesManager::PreVerifyBatchedSigShares(const CActiveMasternodeManager // quorum is too old return false; } - if (!session.quorum->IsMember(WITH_LOCK(mn_activeman.cs, return mn_activeman.GetProTxHash()))) { + if (!session.quorum->IsMember(mn_activeman.GetProTxHash())) { // we're not a member so we can't verify it (we actually shouldn't have received it) return false; } @@ -703,7 +703,7 @@ void CSigSharesManager::ProcessSigShare(const CSigShare& sigShare, const CConnma // prepare node set for direct-push in case this is our sig share std::set quorumNodes; - if (!IsAllMembersConnectedEnabled(llmqType, m_sporkman) && sigShare.getQuorumMember() == quorum->GetMemberIndex(WITH_LOCK(m_mn_activeman->cs, return m_mn_activeman->GetProTxHash()))) { + if (!IsAllMembersConnectedEnabled(llmqType, m_sporkman) && sigShare.getQuorumMember() == quorum->GetMemberIndex(m_mn_activeman->GetProTxHash())) { quorumNodes = connman.GetMasternodeQuorumNodes(sigShare.getLlmqType(), sigShare.getQuorumHash()); } @@ -1496,7 +1496,7 @@ std::optional CSigSharesManager::CreateSigShare(const CQuorumCPtr& qu assert(m_mn_activeman); cxxtimer::Timer t(true); - auto activeMasterNodeProTxHash = WITH_LOCK(m_mn_activeman->cs, return m_mn_activeman->GetProTxHash()); + auto activeMasterNodeProTxHash = m_mn_activeman->GetProTxHash(); if (!quorum->IsValidMember(activeMasterNodeProTxHash)) { return std::nullopt; diff --git a/src/masternode/node.cpp b/src/masternode/node.cpp index b822728dc1..dd3d128f57 100644 --- a/src/masternode/node.cpp +++ b/src/masternode/node.cpp @@ -31,7 +31,7 @@ CActiveMasternodeManager::CActiveMasternodeManager(const CBLSSecretKey& sk, CCon std::string CActiveMasternodeManager::GetStateString() const { - switch (WITH_LOCK(cs, return m_state)) { + switch (WITH_READ_LOCK(cs, return m_state)) { case MASTERNODE_WAITING_FOR_PROTX: return "WAITING_FOR_PROTX"; case MASTERNODE_POSE_BANNED: @@ -53,7 +53,7 @@ std::string CActiveMasternodeManager::GetStateString() const std::string CActiveMasternodeManager::GetStatus() const { - LOCK(cs); + READ_LOCK(cs); switch (m_state) { case MASTERNODE_WAITING_FOR_PROTX: return "Waiting for ProTx to appear on-chain"; @@ -74,9 +74,9 @@ std::string CActiveMasternodeManager::GetStatus() const } } -void CActiveMasternodeManager::Init(const CBlockIndex* pindex) +void CActiveMasternodeManager::InitInternal(const CBlockIndex* pindex) { - LOCK(cs); + AssertLockHeld(cs); if (!fMasternodeMode) return; @@ -153,7 +153,7 @@ void CActiveMasternodeManager::UpdatedBlockTip(const CBlockIndex* pindexNew, con if (!DeploymentDIP0003Enforced(pindexNew->nHeight, Params().GetConsensus())) return; - const auto [cur_state, cur_protx_hash] = WITH_LOCK(cs, return std::make_pair(m_state, m_info.proTxHash)); + const auto [cur_state, cur_protx_hash] = WITH_READ_LOCK(cs, return std::make_pair(m_state, m_info.proTxHash)); if (cur_state == MASTERNODE_READY) { auto oldMNList = Assert(m_dmnman)->GetListForBlock(pindexNew->pprev); auto newMNList = m_dmnman->GetListForBlock(pindexNew); @@ -163,7 +163,7 @@ void CActiveMasternodeManager::UpdatedBlockTip(const CBlockIndex* pindexNew, con m_info.proTxHash = uint256(); m_info.outpoint.SetNull(); // MN might have reappeared in same block with a new ProTx - Init(pindexNew); + InitInternal(pindexNew); }; if (!newMNList.IsMNValid(cur_protx_hash)) { @@ -190,6 +190,7 @@ void CActiveMasternodeManager::UpdatedBlockTip(const CBlockIndex* pindexNew, con bool CActiveMasternodeManager::GetLocalAddress(CService& addrRet) { + AssertLockHeld(cs); // First try to find whatever our own local address is known internally. // Addresses could be specified via externalip or bind option, discovered via UPnP // or added by TorController. Use some random dummy IPv4 peer to prefer the one @@ -207,7 +208,7 @@ bool CActiveMasternodeManager::GetLocalAddress(CService& addrRet) if (!fFoundLocal) { bool empty = true; // If we have some peers, let's try to find our local address from one of them - auto service = WITH_LOCK(cs, return m_info.service); + auto service = m_info.service; m_connman.ForEachNodeContinueIf(CConnman::AllNodes, [&](CNode* pnode) { empty = false; if (pnode->addr.IsIPv4()) @@ -216,7 +217,6 @@ bool CActiveMasternodeManager::GetLocalAddress(CService& addrRet) }); // nothing and no live connections, can't do anything for now if (empty) { - LOCK(cs); m_error = "Can't detect valid external address. Please consider using the externalip configuration option if problem persists. Make sure to use IPv4 address only."; LogPrintf("CActiveMasternodeManager::GetLocalAddress -- ERROR: %s\n", m_error); return false; @@ -238,7 +238,7 @@ template