From f94083a70153b239a09bb2ab1dae434cca4ad47a Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 19 Jul 2024 11:03:19 +0000 Subject: [PATCH 1/4] refactor: make `GetQuorum` and friends return `std::optional`s `GetQuorum` is a fail-able activity and some portions of the codebase handle the fail state but others implicitly assume that they won't fail (dereferencing without nullptr checks) or make the assumption explicit (through placing `assert`s). As some of these assumptions were within loops, care must be taken that partial changes are not committed (unless the entity is discarded by the caller after reporting the fail state). By making it a `std::optional`, we separate fetching and using the value as two separate steps with error handling in between, assumptions have been documented through code comments but shouldn't cause the client to crash altogether (crashes are harder to extract debug information from if they're sporadic and occur on nodes running release binaries). --- src/evo/assetlocktx.cpp | 6 ++-- src/evo/mnhftx.cpp | 5 +-- src/evo/simplifiedmns.cpp | 5 +-- src/llmq/ehf_signals.cpp | 5 +-- src/llmq/instantsend.cpp | 6 ++-- src/llmq/quorums.cpp | 40 ++++++++++++----------- src/llmq/quorums.h | 10 +++--- src/llmq/signing.cpp | 19 ++++++----- src/llmq/signing_shares.cpp | 64 +++++++++++++++++++++---------------- src/llmq/signing_shares.h | 10 ++++-- src/rpc/quorums.cpp | 25 ++++++++------- 11 files changed, 110 insertions(+), 85 deletions(-) diff --git a/src/evo/assetlocktx.cpp b/src/evo/assetlocktx.cpp index d8f2c502b6042..dce9a9d52d4bf 100644 --- a/src/evo/assetlocktx.cpp +++ b/src/evo/assetlocktx.cpp @@ -137,15 +137,15 @@ bool CAssetUnlockPayload::VerifySig(const llmq::CQuorumManager& qman, const uint return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-assetunlock-too-late"); } - const auto quorum = qman.GetQuorum(llmqType, quorumHash); + const auto quorum_opt = qman.GetQuorum(llmqType, quorumHash); // quorum must be valid at this point. Let's check and throw error just in case - if (!quorum) { + if (!quorum_opt.value()) { LogPrintf("%s: ERROR! No quorum for credit pool found for hash=%s\n", __func__, quorumHash.ToString()); return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-assetunlock-quorum-internal-error"); } + const auto quorum = quorum_opt.value(); const uint256 requestId = ::SerializeHash(std::make_pair(ASSETUNLOCK_REQUESTID_PREFIX, index)); - if (const uint256 signHash = llmq::BuildSignHash(llmqType, quorum->qc->quorumHash, requestId, msgHash); quorumSig.VerifyInsecure(quorum->qc->quorumPublicKey, signHash)) { return true; diff --git a/src/evo/mnhftx.cpp b/src/evo/mnhftx.cpp index 05e24db112d7b..b060a93e0e909 100644 --- a/src/evo/mnhftx.cpp +++ b/src/evo/mnhftx.cpp @@ -94,12 +94,13 @@ bool MNHFTx::Verify(const llmq::CQuorumManager& qman, const uint256& quorumHash, } const Consensus::LLMQType& llmqType = Params().GetConsensus().llmqTypeMnhf; - const auto quorum = qman.GetQuorum(llmqType, quorumHash); + const auto quorum_opt = qman.GetQuorum(llmqType, quorumHash); - if (!quorum) { + if (!quorum_opt.has_value()) { return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-mnhf-missing-quorum"); } + const auto quorum = quorum_opt.value(); const uint256 signHash = llmq::BuildSignHash(llmqType, quorum->qc->quorumHash, requestId, msgHash); if (!sig.VerifyInsecure(quorum->qc->quorumPublicKey, signHash)) { return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-mnhf-invalid"); diff --git a/src/evo/simplifiedmns.cpp b/src/evo/simplifiedmns.cpp index 4695d55c41b82..de32fd48171ca 100644 --- a/src/evo/simplifiedmns.cpp +++ b/src/evo/simplifiedmns.cpp @@ -197,13 +197,14 @@ bool CSimplifiedMNListDiff::BuildQuorumChainlockInfo(const llmq::CQuorumManager& // We assume that we have on hand, quorums that correspond to the hashes queried. // If we cannot find them, something must have gone wrong and we should cease trying // to build any further. - auto quorum = qman.GetQuorum(e.llmqType, e.quorumHash); - if (!quorum) { + auto quorum_opt = qman.GetQuorum(e.llmqType, e.quorumHash); + if (!quorum_opt.has_value()) { LogPrintf("%s: ERROR! Unexpected missing quorum with llmqType=%d, quorumHash=%s\n", __func__, ToUnderlying(e.llmqType), e.quorumHash.ToString()); return false; } + auto quorum = quorum_opt.value(); // In case of rotation, all rotated quorums rely on the CL sig expected in the cycleBlock (the block of the first DKG) - 8 // In case of non-rotation, quorums rely on the CL sig expected in the block of the DKG - 8 const CBlockIndex* pWorkBaseBlockIndex = diff --git a/src/llmq/ehf_signals.cpp b/src/llmq/ehf_signals.cpp index c5388e46dd5e0..29c8ee8f1afbb 100644 --- a/src/llmq/ehf_signals.cpp +++ b/src/llmq/ehf_signals.cpp @@ -75,12 +75,13 @@ void CEHFSignalsHandler::trySignEHFSignal(int bit, const CBlockIndex* const pind return; } - const auto quorum = llmq::SelectQuorumForSigning(llmq_params_opt.value(), m_chainman.ActiveChain(), qman, requestId); - if (!quorum) { + const auto quorum_opt = llmq::SelectQuorumForSigning(llmq_params_opt.value(), m_chainman.ActiveChain(), qman, requestId); + if (!quorum_opt) { LogPrintf("CEHFSignalsHandler::trySignEHFSignal no quorum for id=%s\n", requestId.ToString()); return; } + const auto quorum = quorum_opt.value(); LogPrint(BCLog::EHF, "CEHFSignalsHandler::trySignEHFSignal: bit=%d at height=%d id=%s\n", bit, pindex->nHeight, requestId.ToString()); mnhfPayload.signal.quorumHash = quorum->qc->quorumHash; const uint256 msgHash = mnhfPayload.PrepareTx().GetHash(); diff --git a/src/llmq/instantsend.cpp b/src/llmq/instantsend.cpp index e175009bb8560..6bba07986e216 100644 --- a/src/llmq/instantsend.cpp +++ b/src/llmq/instantsend.cpp @@ -905,11 +905,13 @@ std::unordered_set CInstantSendManager::ProcessPend nSignHeight = blockIndex->nHeight + dkgInterval - 1; } - auto quorum = llmq::SelectQuorumForSigning(llmq_params, m_chainstate.m_chain, qman, id, nSignHeight, signOffset); - if (!quorum) { + auto quorum_opt = llmq::SelectQuorumForSigning(llmq_params, m_chainstate.m_chain, qman, id, nSignHeight, signOffset); + if (!quorum_opt.has_value()) { // should not happen, but if one fails to select, all others will also fail to select return {}; } + + auto quorum = quorum_opt.value(); uint256 signHash = BuildSignHash(llmq_params.type, quorum->qc->quorumHash, id, islock->txid); batchVerifier.PushMessage(nodeId, hash, signHash, islock->sig.Get(), quorum->qc->quorumPublicKey); verifyCount++; diff --git a/src/llmq/quorums.cpp b/src/llmq/quorums.cpp index 1b9902a5a8e2d..057109aec95c1 100644 --- a/src/llmq/quorums.cpp +++ b/src/llmq/quorums.cpp @@ -393,14 +393,14 @@ void CQuorumManager::CheckQuorumConnections(const Consensus::LLMQParams& llmqPar } } -CQuorumPtr CQuorumManager::BuildQuorumFromCommitment(const Consensus::LLMQType llmqType, gsl::not_null pQuorumBaseBlockIndex, bool populate_cache) const +std::optional CQuorumManager::BuildQuorumFromCommitment(const Consensus::LLMQType llmqType, gsl::not_null pQuorumBaseBlockIndex, bool populate_cache) const { const uint256& quorumHash{pQuorumBaseBlockIndex->GetBlockHash()}; uint256 minedBlockHash; CFinalCommitmentPtr qc = quorumBlockProcessor.GetMinedCommitment(llmqType, quorumHash, minedBlockHash); if (qc == nullptr) { LogPrint(BCLog::LLMQ, "CQuorumManager::%s -- No mined commitment for llmqType[%d] nHeight[%d] quorumHash[%s]\n", __func__, ToUnderlying(llmqType), pQuorumBaseBlockIndex->nHeight, pQuorumBaseBlockIndex->GetBlockHash().ToString()); - return nullptr; + return std::nullopt; } assert(qc->quorumHash == pQuorumBaseBlockIndex->GetBlockHash()); @@ -606,14 +606,14 @@ std::vector CQuorumManager::ScanQuorums(Consensus::LLMQType llmqTyp // We assume that every quorum asked for is available to us on hand, if this // fails then we can assume that something has gone wrong and we should stop // trying to process any further and return a blank. - auto quorum = GetQuorum(llmqType, pQuorumBaseBlockIndex, populate_cache); - if (!quorum) { + auto quorum_opt = GetQuorum(llmqType, pQuorumBaseBlockIndex, populate_cache); + if (!quorum_opt.has_value()) { LogPrintf("%s: ERROR! Unexpected missing quorum with llmqType=%d, blockHash=%s, populate_cache=%s\n", __func__, ToUnderlying(llmqType), pQuorumBaseBlockIndex->GetBlockHash().ToString(), populate_cache ? "true" : "false"); return {}; } - vecResultQuorums.emplace_back(quorum); + vecResultQuorums.emplace_back(quorum_opt.value()); } const size_t nCountResult{vecResultQuorums.size()}; @@ -631,7 +631,7 @@ std::vector CQuorumManager::ScanQuorums(Consensus::LLMQType llmqTyp return {vecResultQuorums.begin(), vecResultQuorums.begin() + nResultEndIndex}; } -CQuorumCPtr CQuorumManager::GetQuorum(Consensus::LLMQType llmqType, const uint256& quorumHash) const +std::optional CQuorumManager::GetQuorum(Consensus::LLMQType llmqType, const uint256& quorumHash) const { const CBlockIndex* pQuorumBaseBlockIndex = [&]() { // Lock contention may still be high here; consider using a shared lock @@ -649,19 +649,19 @@ CQuorumCPtr CQuorumManager::GetQuorum(Consensus::LLMQType llmqType, const uint25 }(); if (!pQuorumBaseBlockIndex) { LogPrint(BCLog::LLMQ, "CQuorumManager::%s -- block %s not found\n", __func__, quorumHash.ToString()); - return nullptr; + return std::nullopt; } return GetQuorum(llmqType, pQuorumBaseBlockIndex); } -CQuorumCPtr CQuorumManager::GetQuorum(Consensus::LLMQType llmqType, gsl::not_null pQuorumBaseBlockIndex, bool populate_cache) const +std::optional CQuorumManager::GetQuorum(Consensus::LLMQType llmqType, gsl::not_null pQuorumBaseBlockIndex, bool populate_cache) const { auto quorumHash = pQuorumBaseBlockIndex->GetBlockHash(); // we must check this before we look into the cache. Reorgs might have happened which would mean we might have // cached quorums which are not in the active chain anymore if (!HasQuorum(llmqType, quorumBlockProcessor, quorumHash)) { - return nullptr; + return std::nullopt; } CQuorumPtr pQuorum; @@ -764,11 +764,12 @@ PeerMsgRet CQuorumManager::ProcessMessage(CNode& pfrom, const std::string& msg_t return sendQDATA(CQuorumDataRequest::Errors::QUORUM_BLOCK_NOT_FOUND, request_limit_exceeded); } - const auto pQuorum = GetQuorum(request.GetLLMQType(), pQuorumBaseBlockIndex); - if (pQuorum == nullptr) { + const auto pQuorumOpt = GetQuorum(request.GetLLMQType(), pQuorumBaseBlockIndex); + if (!pQuorumOpt.has_value()) { return sendQDATA(CQuorumDataRequest::Errors::QUORUM_NOT_FOUND, request_limit_exceeded); } + const auto pQuorum = pQuorumOpt.value(); CDataStream ssResponseData(SER_NETWORK, pfrom.GetCommonVersion()); // Check if request wants QUORUM_VERIFICATION_VECTOR data @@ -1200,8 +1201,8 @@ void CQuorumManager::MigrateOldQuorumDB(CEvoDB& evoDb) const LogPrint(BCLog::LLMQ, "CQuorumManager::%s -- done\n", __func__); } -CQuorumCPtr SelectQuorumForSigning(const Consensus::LLMQParams& llmq_params, const CChain& active_chain, const CQuorumManager& qman, - const uint256& selectionHash, int signHeight, int signOffset) +std::optional SelectQuorumForSigning(const Consensus::LLMQParams& llmq_params, const CChain& active_chain, const CQuorumManager& qman, + const uint256& selectionHash, int signHeight, int signOffset) { size_t poolSize = llmq_params.signingActiveQuorumCount; @@ -1221,7 +1222,7 @@ CQuorumCPtr SelectQuorumForSigning(const Consensus::LLMQParams& llmq_params, con if (IsQuorumRotationEnabled(llmq_params, pindexStart)) { auto quorums = qman.ScanQuorums(llmq_params.type, pindexStart, poolSize); if (quorums.empty()) { - return nullptr; + return std::nullopt; } //log2 int int n = std::log2(llmq_params.signingActiveQuorumCount); @@ -1231,7 +1232,7 @@ CQuorumCPtr SelectQuorumForSigning(const Consensus::LLMQParams& llmq_params, con uint64_t signer = (((1ull << n) - 1) & (b >> (64 - n - 1))); if (signer > quorums.size()) { - return nullptr; + return std::nullopt; } auto itQuorum = std::find_if(quorums.begin(), quorums.end(), @@ -1239,13 +1240,13 @@ CQuorumCPtr SelectQuorumForSigning(const Consensus::LLMQParams& llmq_params, con return uint64_t(obj->qc->quorumIndex) == signer; }); if (itQuorum == quorums.end()) { - return nullptr; + return std::nullopt; } return *itQuorum; } else { auto quorums = qman.ScanQuorums(llmq_params.type, pindexStart, poolSize); if (quorums.empty()) { - return nullptr; + return std::nullopt; } std::vector> scores; @@ -1268,11 +1269,12 @@ VerifyRecSigStatus VerifyRecoveredSig(Consensus::LLMQType llmqType, const CChain { const auto& llmq_params_opt = Params().GetLLMQ(llmqType); assert(llmq_params_opt.has_value()); - auto quorum = SelectQuorumForSigning(llmq_params_opt.value(), active_chain, qman, id, signedAtHeight, signOffset); - if (!quorum) { + auto quorum_opt = SelectQuorumForSigning(llmq_params_opt.value(), active_chain, qman, id, signedAtHeight, signOffset); + if (!quorum_opt.has_value()) { return VerifyRecSigStatus::NoQuorum; } + auto quorum = quorum_opt.value(); uint256 signHash = BuildSignHash(llmqType, quorum->qc->quorumHash, id, msgHash); const bool ret = sig.VerifyInsecure(quorum->qc->quorumPublicKey, signHash); return ret ? VerifyRecSigStatus::Valid : VerifyRecSigStatus::Invalid; diff --git a/src/llmq/quorums.h b/src/llmq/quorums.h index 892c0ecd27e31..32e36927ecc0a 100644 --- a/src/llmq/quorums.h +++ b/src/llmq/quorums.h @@ -281,7 +281,7 @@ class CQuorumManager bool RequestQuorumData(CNode* pfrom, Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, uint16_t nDataMask, const uint256& proTxHash = uint256()) const; // all these methods will lock cs_main for a short period of time - CQuorumCPtr GetQuorum(Consensus::LLMQType llmqType, const uint256& quorumHash) const; + std::optional GetQuorum(Consensus::LLMQType llmqType, const uint256& quorumHash) const; std::vector ScanQuorums(Consensus::LLMQType llmqType, size_t nCountRequested) const; // this one is cs_main-free @@ -291,10 +291,10 @@ class CQuorumManager // all private methods here are cs_main-free void CheckQuorumConnections(const Consensus::LLMQParams& llmqParams, const CBlockIndex *pindexNew) const; - CQuorumPtr BuildQuorumFromCommitment(Consensus::LLMQType llmqType, gsl::not_null pQuorumBaseBlockIndex, bool populate_cache) const; + std::optional BuildQuorumFromCommitment(Consensus::LLMQType llmqType, gsl::not_null pQuorumBaseBlockIndex, bool populate_cache) const; bool BuildQuorumContributions(const CFinalCommitmentPtr& fqc, const std::shared_ptr& quorum) const; - CQuorumCPtr GetQuorum(Consensus::LLMQType llmqType, gsl::not_null pindex, bool populate_cache = true) const; + std::optional GetQuorum(Consensus::LLMQType llmqType, gsl::not_null pindex, bool populate_cache = true) const; /// Returns the start offset for the masternode with the given proTxHash. This offset is applied when picking data recovery members of a quorum's /// memberlist and is calculated based on a list of all member of all active quorums for the given llmqType in a way that each member /// should receive the same number of request if all active llmqType members requests data from one llmqType quorum. @@ -312,8 +312,8 @@ class CQuorumManager // which are not 100% at the chain tip. static constexpr int SIGN_HEIGHT_OFFSET{8}; -CQuorumCPtr SelectQuorumForSigning(const Consensus::LLMQParams& llmq_params, const CChain& active_chain, const CQuorumManager& qman, - const uint256& selectionHash, int signHeight = -1 /*chain tip*/, int signOffset = SIGN_HEIGHT_OFFSET); +std::optional SelectQuorumForSigning(const Consensus::LLMQParams& llmq_params, const CChain& active_chain, const CQuorumManager& qman, + const uint256& selectionHash, int signHeight = -1 /*chain tip*/, int signOffset = SIGN_HEIGHT_OFFSET); // Verifies a recovered sig that was signed while the chain tip was at signedAtTip VerifyRecSigStatus VerifyRecoveredSig(Consensus::LLMQType llmqType, const CChain& active_chain, const CQuorumManager& qman, diff --git a/src/llmq/signing.cpp b/src/llmq/signing.cpp index edc8e2f74a373..cb3545f8474d7 100644 --- a/src/llmq/signing.cpp +++ b/src/llmq/signing.cpp @@ -402,14 +402,14 @@ static bool PreVerifyRecoveredSig(const CQuorumManager& quorum_manager, const CR return false; } - auto quorum = quorum_manager.GetQuorum(llmqType, recoveredSig.getQuorumHash()); - - if (!quorum) { + auto quorum_opt = quorum_manager.GetQuorum(llmqType, recoveredSig.getQuorumHash()); + if (!quorum_opt.has_value()) { LogPrint(BCLog::LLMQ, "CSigningManager::%s -- quorum %s not found\n", __func__, recoveredSig.getQuorumHash().ToString()); return false; } - if (!IsQuorumActive(llmqType, quorum_manager, quorum->qc->quorumHash)) { + + if (!IsQuorumActive(llmqType, quorum_manager, quorum_opt.value()->qc->quorumHash)) { return false; } @@ -495,13 +495,15 @@ void CSigningManager::CollectPendingRecoveredSigsToVerify( auto llmqType = recSig->getLlmqType(); auto quorumKey = std::make_pair(recSig->getLlmqType(), recSig->getQuorumHash()); if (!retQuorums.count(quorumKey)) { - auto quorum = qman.GetQuorum(llmqType, recSig->getQuorumHash()); - if (!quorum) { + auto quorum_opt = qman.GetQuorum(llmqType, recSig->getQuorumHash()); + if (!quorum_opt.has_value()) { LogPrint(BCLog::LLMQ, "CSigningManager::%s -- quorum %s not found, node=%d\n", __func__, recSig->getQuorumHash().ToString(), nodeId); it = v.erase(it); continue; } + + auto quorum = quorum_opt.value(); if (!IsQuorumActive(llmqType, qman, quorum->qc->quorumHash)) { LogPrint(BCLog::LLMQ, "CSigningManager::%s -- quorum %s not active anymore, node=%d\n", __func__, recSig->getQuorumHash().ToString(), nodeId); @@ -688,7 +690,7 @@ bool CSigningManager::AsyncSignIfMember(Consensus::LLMQType llmqType, CSigShares if (m_mn_activeman == nullptr) return false; if (m_mn_activeman->GetProTxHash().IsNull()) return false; - const auto quorum = [&]() { + const auto quorum_opt = [&]() { if (quorumHash.IsNull()) { // This might end up giving different results on different members // This might happen when we are on the brink of confirming a new quorum @@ -703,11 +705,12 @@ bool CSigningManager::AsyncSignIfMember(Consensus::LLMQType llmqType, CSigShares } }(); - if (!quorum) { + if (!quorum_opt.has_value()) { LogPrint(BCLog::LLMQ, "CSigningManager::%s -- failed to select quorum. id=%s, msgHash=%s\n", __func__, id.ToString(), msgHash.ToString()); return false; } + auto quorum = quorum_opt.value(); if (!quorum->IsValidMember(m_mn_activeman->GetProTxHash())) { return false; } diff --git a/src/llmq/signing_shares.cpp b/src/llmq/signing_shares.cpp index 6cfb8d16554d0..1806623c3a4ec 100644 --- a/src/llmq/signing_shares.cpp +++ b/src/llmq/signing_shares.cpp @@ -307,8 +307,8 @@ bool CSigSharesManager::ProcessMessageSigSesAnn(const CNode& pfrom, const CSigSe LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- ann={%s}, node=%d\n", __func__, ann.ToString(), pfrom.GetId()); - auto quorum = qman.GetQuorum(llmqType, ann.getQuorumHash()); - if (!quorum) { + auto quorum_opt = qman.GetQuorum(llmqType, ann.getQuorumHash()); + if (!quorum_opt.has_value()) { // TODO should we ban here? LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- quorum %s not found, node=%d\n", __func__, ann.getQuorumHash().ToString(), pfrom.GetId()); @@ -321,7 +321,7 @@ bool CSigSharesManager::ProcessMessageSigSesAnn(const CNode& pfrom, const CSigSe nodeState.sessionByRecvId.erase(session.recvSessionId); nodeState.sessionByRecvId.erase(ann.getSessionId()); session.recvSessionId = ann.getSessionId(); - session.quorum = quorum; + session.quorum = quorum_opt.value(); nodeState.sessionByRecvId.try_emplace(ann.getSessionId(), &session); return true; @@ -458,10 +458,12 @@ void CSigSharesManager::ProcessMessageSigShare(NodeId fromId, const CSigShare& s { assert(m_mn_activeman); - auto quorum = qman.GetQuorum(sigShare.getLlmqType(), sigShare.getQuorumHash()); - if (!quorum) { + auto quorum_opt = qman.GetQuorum(sigShare.getLlmqType(), sigShare.getQuorumHash()); + if (!quorum_opt.has_value()) { return; } + + auto quorum = quorum_opt.value(); if (!IsQuorumActive(sigShare.getLlmqType(), qman, quorum->qc->quorumHash)) { // quorum is too old return; @@ -549,14 +551,20 @@ bool CSigSharesManager::PreVerifyBatchedSigShares(const CActiveMasternodeManager return true; } -bool CSigSharesManager::CollectPendingSigSharesToVerify( - size_t maxUniqueSessions, std::unordered_map>& retSigShares, - std::unordered_map, CQuorumCPtr, StaticSaltedHasher>& retQuorums) +std::optional< + std::pair< + std::unordered_map>, + std::unordered_map, CQuorumCPtr, StaticSaltedHasher> + > +> CSigSharesManager::CollectPendingSigSharesToVerify(size_t maxUniqueSessions) { + std::unordered_map> sig_shares; + std::unordered_map, CQuorumCPtr, StaticSaltedHasher> quorums; + { LOCK(cs); if (nodeStates.empty()) { - return false; + return std::nullopt; } // This will iterate node states in random order and pick one sig share at a time. This avoids processing @@ -581,53 +589,55 @@ bool CSigSharesManager::CollectPendingSigSharesToVerify( AssertLockHeld(cs); if (const bool alreadyHave = this->sigShares.Has(sigShare.GetKey()); !alreadyHave) { uniqueSignHashes.emplace(nodeId, sigShare.GetSignHash()); - retSigShares[nodeId].emplace_back(sigShare); + sig_shares[nodeId].emplace_back(sigShare); } ns.pendingIncomingSigShares.Erase(sigShare.GetKey()); return !ns.pendingIncomingSigShares.Empty(); }, rnd); - if (retSigShares.empty()) { - return false; + if (sig_shares.empty()) { + return std::nullopt; } } // For the convenience of the caller, also build a map of quorumHash -> quorum - for (const auto& [_, vecSigShares] : retSigShares) { + for (const auto& [_, vecSigShares] : sig_shares) { for (const auto& sigShare : vecSigShares) { auto llmqType = sigShare.getLlmqType(); auto k = std::make_pair(llmqType, sigShare.getQuorumHash()); - if (retQuorums.count(k) != 0) { + if (quorums.count(k) != 0) { continue; } - auto quorum = qman.GetQuorum(llmqType, sigShare.getQuorumHash()); + auto quorum_opt = qman.GetQuorum(llmqType, sigShare.getQuorumHash()); // Despite constructing a convenience map, we assume that the quorum *must* be present. // The absence of it might indicate an inconsistent internal state, so we should report // nothing instead of reporting flawed data. - if (!quorum) { + if (!quorum_opt.has_value()) { LogPrintf("%s: ERROR! Unexpected missing quorum with llmqType=%d, quorumHash=%s\n", __func__, ToUnderlying(llmqType), sigShare.getQuorumHash().ToString()); - return false; + return std::nullopt; } - retQuorums.try_emplace(k, quorum); + quorums.try_emplace(k, quorum_opt.value()); } } - return true; + return std::make_pair(sig_shares, quorums); } bool CSigSharesManager::ProcessPendingSigShares(const CConnman& connman) { - std::unordered_map> sigSharesByNodes; - std::unordered_map, CQuorumCPtr, StaticSaltedHasher> quorums; - const size_t nMaxBatchSize{32}; - bool collect_status = CollectPendingSigSharesToVerify(nMaxBatchSize, sigSharesByNodes, quorums); - if (!collect_status || sigSharesByNodes.empty()) { + auto pending_sig_shares = CollectPendingSigSharesToVerify(nMaxBatchSize); + if (!pending_sig_shares.has_value()) { + return false; + } + + auto [sigSharesByNodes, quorums] = pending_sig_shares.value(); + if (sigSharesByNodes.empty()) { return false; } @@ -1277,9 +1287,9 @@ void CSigSharesManager::Cleanup() // Find quorums which became inactive for (auto it = quorums.begin(); it != quorums.end(); ) { if (IsQuorumActive(it->first.first, qman, it->first.second)) { - auto quorum = qman.GetQuorum(it->first.first, it->first.second); - if (quorum) { - it->second = quorum; + auto quorum_opt = qman.GetQuorum(it->first.first, it->first.second); + if (quorum_opt.has_value()) { + it->second = quorum_opt.value(); ++it; continue; } diff --git a/src/llmq/signing_shares.h b/src/llmq/signing_shares.h index cd772b85a0b65..d7ea8ec317180 100644 --- a/src/llmq/signing_shares.h +++ b/src/llmq/signing_shares.h @@ -453,9 +453,13 @@ class CSigSharesManager : public CRecoveredSigsListener static bool PreVerifyBatchedSigShares(const CActiveMasternodeManager& mn_activeman, const CQuorumManager& quorum_manager, const CSigSharesNodeState::SessionInfo& session, const CBatchedSigShares& batchedSigShares, bool& retBan); - bool CollectPendingSigSharesToVerify( - size_t maxUniqueSessions, std::unordered_map>& retSigShares, - std::unordered_map, CQuorumCPtr, StaticSaltedHasher>& retQuorums); + std::optional< + std::pair< + std::unordered_map>, + std::unordered_map, CQuorumCPtr, StaticSaltedHasher> + > + > CollectPendingSigSharesToVerify(size_t maxUniqueSessions); + bool ProcessPendingSigShares(const CConnman& connman); void ProcessPendingSigShares(const std::vector& sigSharesToProcess, diff --git a/src/rpc/quorums.cpp b/src/rpc/quorums.cpp index e4c12e8b218da..c685118c5e6fc 100644 --- a/src/rpc/quorums.cpp +++ b/src/rpc/quorums.cpp @@ -255,12 +255,12 @@ static RPCHelpMan quorum_info() includeSkShare = ParseBoolV(request.params[2], "includeSkShare"); } - const auto quorum = llmq_ctx.qman->GetQuorum(llmqType, quorumHash); - if (!quorum) { + const auto quorum_opt = llmq_ctx.qman->GetQuorum(llmqType, quorumHash); + if (!quorum_opt.has_value()) { throw JSONRPCError(RPC_INVALID_PARAMETER, "quorum not found"); } - return BuildQuorumInfo(*llmq_ctx.quorum_block_processor, quorum, true, includeSkShare); + return BuildQuorumInfo(*llmq_ctx.quorum_block_processor, quorum_opt.value(), true, includeSkShare); }, }; } @@ -451,7 +451,7 @@ static UniValue quorum_sign_helper(const JSONRPCRequest& request, Consensus::LLM if (fSubmit) { return llmq_ctx.sigman->AsyncSignIfMember(llmqType, *llmq_ctx.shareman, id, msgHash, quorumHash); } else { - const auto pQuorum = [&]() { + const auto pQuorumOpt = [&]() { if (quorumHash.IsNull()) { return llmq::SelectQuorumForSigning(llmq_params_opt.value(), chainman.ActiveChain(), *llmq_ctx.qman, id); } else { @@ -459,11 +459,11 @@ static UniValue quorum_sign_helper(const JSONRPCRequest& request, Consensus::LLM } }(); - if (pQuorum == nullptr) { + if (!pQuorumOpt.has_value()) { throw JSONRPCError(RPC_INVALID_PARAMETER, "quorum not found"); } - auto sigShare = llmq_ctx.shareman->CreateSigShare(pQuorum, id, msgHash); + auto sigShare = llmq_ctx.shareman->CreateSigShare(pQuorumOpt.value(), id, msgHash); if (!sigShare.has_value() || !sigShare->sigShare.Get().IsValid()) { throw JSONRPCError(RPC_INVALID_PARAMETER, "failed to create sigShare"); @@ -592,12 +592,12 @@ static RPCHelpMan quorum_verify() } uint256 quorumHash(ParseHashV(request.params[4], "quorumHash")); - const auto quorum = llmq_ctx.qman->GetQuorum(llmqType, quorumHash); - - if (!quorum) { + const auto quorum_opt = llmq_ctx.qman->GetQuorum(llmqType, quorumHash); + if (!quorum_opt.has_value()) { throw JSONRPCError(RPC_INVALID_PARAMETER, "quorum not found"); } + const auto quorum = quorum_opt.value(); uint256 signHash = llmq::BuildSignHash(llmqType, quorum->qc->quorumHash, id, msgHash); return sig.VerifyInsecure(quorum->qc->quorumPublicKey, signHash); }, @@ -724,12 +724,13 @@ static RPCHelpMan quorum_selectquorum() UniValue ret(UniValue::VOBJ); - const auto quorum = llmq::SelectQuorumForSigning(llmq_params_opt.value(), chainman.ActiveChain(), *llmq_ctx.qman, id); - if (!quorum) { + const auto quorum_opt = llmq::SelectQuorumForSigning(llmq_params_opt.value(), chainman.ActiveChain(), *llmq_ctx.qman, id); + if (!quorum_opt.has_value()) { throw JSONRPCError(RPC_MISC_ERROR, "no quorums active"); } - ret.pushKV("quorumHash", quorum->qc->quorumHash.ToString()); + const auto quorum = quorum_opt.value(); + ret.pushKV("quorumHash", quorum->qc->quorumHash.ToString()); UniValue recoveryMembers(UniValue::VARR); for (int i = 0; i < quorum->params.recoveryMembers; ++i) { auto dmn = llmq_ctx.shareman->SelectMemberForRecovery(quorum, id, i); From 33e44a27acbe68f784f801e234a1cbcec682a7cd Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Tue, 3 Dec 2024 10:10:35 +0000 Subject: [PATCH 2/4] refactor: make `GetMinedCommitment` return `std::optional` `std::move` is required as we're dealing with `std::unique_ptr` objects --- src/evo/cbtx.cpp | 6 ++++-- src/evo/simplifiedmns.cpp | 6 +++--- src/llmq/blockprocessor.cpp | 4 ++-- src/llmq/blockprocessor.h | 2 +- src/llmq/quorums.cpp | 6 ++++-- src/llmq/snapshot.cpp | 6 +++--- 6 files changed, 17 insertions(+), 13 deletions(-) diff --git a/src/evo/cbtx.cpp b/src/evo/cbtx.cpp index fc08bd5db57fd..1bb96dd65da6f 100644 --- a/src/evo/cbtx.cpp +++ b/src/evo/cbtx.cpp @@ -212,11 +212,13 @@ auto CachedGetQcHashesQcIndexedHashes(const CBlockIndex* pindexPrev, const llmq: auto& map_indexed_hashes = qcIndexedHashes_cached[llmqType]; for (const auto& blockIndex : vecBlockIndexes) { uint256 dummyHash; - llmq::CFinalCommitmentPtr pqc = quorum_block_processor.GetMinedCommitment(llmqType, blockIndex->GetBlockHash(), dummyHash); - if (pqc == nullptr) { + auto pqc_opt = quorum_block_processor.GetMinedCommitment(llmqType, blockIndex->GetBlockHash(), dummyHash); + if (!pqc_opt.has_value()) { // this should never happen return std::nullopt; } + + auto pqc = std::move(pqc_opt).value(); auto qcHash = ::SerializeHash(*pqc); if (rotation_enabled) { map_indexed_hashes[pqc->quorumIndex] = qcHash; diff --git a/src/evo/simplifiedmns.cpp b/src/evo/simplifiedmns.cpp index de32fd48171ca..573edbc1f10ff 100644 --- a/src/evo/simplifiedmns.cpp +++ b/src/evo/simplifiedmns.cpp @@ -176,11 +176,11 @@ bool CSimplifiedMNListDiff::BuildQuorumsDiff(const CBlockIndex* baseBlockIndex, const auto& [llmqType, hash] = p; if (!baseQuorumHashes.count(p)) { uint256 minedBlockHash; - llmq::CFinalCommitmentPtr qc = quorum_block_processor.GetMinedCommitment(llmqType, hash, minedBlockHash); - if (qc == nullptr) { + auto qc = quorum_block_processor.GetMinedCommitment(llmqType, hash, minedBlockHash); + if (!qc.has_value()) { return false; } - newQuorums.emplace_back(*qc); + newQuorums.emplace_back(*qc.value()); } } diff --git a/src/llmq/blockprocessor.cpp b/src/llmq/blockprocessor.cpp index b11ab32f813ff..5689f54df3624 100644 --- a/src/llmq/blockprocessor.cpp +++ b/src/llmq/blockprocessor.cpp @@ -465,12 +465,12 @@ bool CQuorumBlockProcessor::HasMinedCommitment(Consensus::LLMQType llmqType, con return fExists; } -CFinalCommitmentPtr CQuorumBlockProcessor::GetMinedCommitment(Consensus::LLMQType llmqType, const uint256& quorumHash, uint256& retMinedBlockHash) const +std::optional CQuorumBlockProcessor::GetMinedCommitment(Consensus::LLMQType llmqType, const uint256& quorumHash, uint256& retMinedBlockHash) const { auto key = std::make_pair(DB_MINED_COMMITMENT, std::make_pair(llmqType, quorumHash)); std::pair p; if (!m_evoDb.Read(key, p)) { - return nullptr; + return std::nullopt; } retMinedBlockHash = p.second; return std::make_unique(p.first); diff --git a/src/llmq/blockprocessor.h b/src/llmq/blockprocessor.h index da5dea17fb2d9..3d492db932764 100644 --- a/src/llmq/blockprocessor.h +++ b/src/llmq/blockprocessor.h @@ -62,7 +62,7 @@ class CQuorumBlockProcessor std::optional> GetMineableCommitments(const Consensus::LLMQParams& llmqParams, int nHeight) const EXCLUSIVE_LOCKS_REQUIRED(cs_main); bool GetMineableCommitmentsTx(const Consensus::LLMQParams& llmqParams, int nHeight, std::vector& ret) const EXCLUSIVE_LOCKS_REQUIRED(cs_main); bool HasMinedCommitment(Consensus::LLMQType llmqType, const uint256& quorumHash) const; - CFinalCommitmentPtr GetMinedCommitment(Consensus::LLMQType llmqType, const uint256& quorumHash, uint256& retMinedBlockHash) const; + std::optional GetMinedCommitment(Consensus::LLMQType llmqType, const uint256& quorumHash, uint256& retMinedBlockHash) const; std::vector GetMinedCommitmentsUntilBlock(Consensus::LLMQType llmqType, gsl::not_null pindex, size_t maxCount) const; std::map> GetMinedAndActiveCommitmentsUntilBlock(gsl::not_null pindex) const; diff --git a/src/llmq/quorums.cpp b/src/llmq/quorums.cpp index 057109aec95c1..79a71735154c8 100644 --- a/src/llmq/quorums.cpp +++ b/src/llmq/quorums.cpp @@ -397,11 +397,13 @@ std::optional CQuorumManager::BuildQuorumFromCommitment(const Consen { const uint256& quorumHash{pQuorumBaseBlockIndex->GetBlockHash()}; uint256 minedBlockHash; - CFinalCommitmentPtr qc = quorumBlockProcessor.GetMinedCommitment(llmqType, quorumHash, minedBlockHash); - if (qc == nullptr) { + auto qc_opt = quorumBlockProcessor.GetMinedCommitment(llmqType, quorumHash, minedBlockHash); + if (!qc_opt.has_value()) { LogPrint(BCLog::LLMQ, "CQuorumManager::%s -- No mined commitment for llmqType[%d] nHeight[%d] quorumHash[%s]\n", __func__, ToUnderlying(llmqType), pQuorumBaseBlockIndex->nHeight, pQuorumBaseBlockIndex->GetBlockHash().ToString()); return std::nullopt; } + + auto qc = std::move(qc_opt).value(); assert(qc->quorumHash == pQuorumBaseBlockIndex->GetBlockHash()); const auto& llmq_params_opt = Params().GetLLMQ(llmqType); diff --git a/src/llmq/snapshot.cpp b/src/llmq/snapshot.cpp index 7b349fb31db57..549cd627aac78 100644 --- a/src/llmq/snapshot.cpp +++ b/src/llmq/snapshot.cpp @@ -273,11 +273,11 @@ bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, const ChainstateMa for (const auto& obj : qdata) { uint256 minedBlockHash; - llmq::CFinalCommitmentPtr qc = qblockman.GetMinedCommitment(llmqType, obj.second->GetBlockHash(), minedBlockHash); - if (qc == nullptr) { + auto qc_opt = qblockman.GetMinedCommitment(llmqType, obj.second->GetBlockHash(), minedBlockHash); + if (!qc_opt.has_value()) { return false; } - response.lastCommitmentPerIndex.push_back(*qc); + response.lastCommitmentPerIndex.push_back(*qc_opt.value()); int quorumCycleStartHeight = obj.second->nHeight - (obj.second->nHeight % llmq_params_opt->dkgInterval); snapshotHeightsNeeded.insert(quorumCycleStartHeight - cycleLength); From 46ad133e67664cdb1cb7388fd7779a5f5d676ee4 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Tue, 3 Dec 2024 11:56:04 +0000 Subject: [PATCH 3/4] refactor: make `GetInstantSendLock*` and friends return `std::optional` --- src/llmq/instantsend.cpp | 77 ++++++++++++++++++++++------------------ src/llmq/instantsend.h | 12 +++---- src/net_processing.cpp | 12 +++---- src/validation.cpp | 8 +++-- 4 files changed, 59 insertions(+), 50 deletions(-) diff --git a/src/llmq/instantsend.cpp b/src/llmq/instantsend.cpp index 6bba07986e216..67ed14b5c03e9 100644 --- a/src/llmq/instantsend.cpp +++ b/src/llmq/instantsend.cpp @@ -101,10 +101,11 @@ void CInstantSendDb::RemoveInstantSendLock(CDBBatch& batch, const uint256& hash, { AssertLockHeld(cs_db); if (!islock) { - islock = GetInstantSendLockByHashInternal(hash, false); - if (!islock) { + auto islock_opt = GetInstantSendLockByHashInternal(hash, false); + if (!islock_opt.has_value()) { return; } + islock = islock_opt.value(); } batch.Erase(std::make_tuple(DB_ISLOCK_BY_HASH, hash)); @@ -184,7 +185,8 @@ std::unordered_map CInstantSen auto& islockHash = std::get<2>(curKey); - if (auto islock = GetInstantSendLockByHashInternal(islockHash, false)) { + if (auto islock_opt = GetInstantSendLockByHashInternal(islockHash, false); islock_opt.has_value()) { + auto islock = islock_opt.value(); RemoveInstantSendLock(batch, islockHash, islock); ret.try_emplace(islockHash, std::move(islock)); } @@ -275,7 +277,7 @@ void CInstantSendDb::RemoveBlockInstantSendLocks(const gsl::not_nullExists(std::make_tuple(DB_ARCHIVED_BY_HASH, islockHash)); + return GetInstantSendLockByHashInternal(islockHash).has_value() || db->Exists(std::make_tuple(DB_ARCHIVED_BY_HASH, islockHash)); } size_t CInstantSendDb::GetInstantSendLockCount() const @@ -301,11 +303,11 @@ size_t CInstantSendDb::GetInstantSendLockCount() const return cnt; } -CInstantSendLockPtr CInstantSendDb::GetInstantSendLockByHashInternal(const uint256& hash, bool use_cache) const +std::optional CInstantSendDb::GetInstantSendLockByHashInternal(const uint256& hash, bool use_cache) const { AssertLockHeld(cs_db); if (hash.IsNull()) { - return nullptr; + return std::nullopt; } CInstantSendLockPtr ret; @@ -319,9 +321,10 @@ CInstantSendLockPtr CInstantSendDb::GetInstantSendLockByHashInternal(const uint2 ret = std::make_shared(); exists = db->Read(std::make_tuple(DB_ISLOCK_BY_HASH, hash), *ret); if (!exists || (::SerializeHash(*ret) != hash)) { - ret = nullptr; + return std::nullopt; } } + islockCache.insert(hash, ret); return ret; } @@ -339,19 +342,19 @@ uint256 CInstantSendDb::GetInstantSendLockHashByTxidInternal(const uint256& txid return islockHash; } -CInstantSendLockPtr CInstantSendDb::GetInstantSendLockByTxid(const uint256& txid) const +std::optional CInstantSendDb::GetInstantSendLockByTxid(const uint256& txid) const { LOCK(cs_db); return GetInstantSendLockByHashInternal(GetInstantSendLockHashByTxidInternal(txid)); } -CInstantSendLockPtr CInstantSendDb::GetInstantSendLockByInput(const COutPoint& outpoint) const +std::optional CInstantSendDb::GetInstantSendLockByInput(const COutPoint& outpoint) const { LOCK(cs_db); uint256 islockHash; if (!outpointCache.get(outpoint, islockHash)) { if (!db->Read(std::make_tuple(DB_HASH_BY_OUTPOINT, outpoint), islockHash)) { - return nullptr; + return std::nullopt; } outpointCache.insert(outpoint, islockHash); } @@ -403,11 +406,12 @@ std::vector CInstantSendDb::RemoveChainedInstantSendLocks(const uint256 stack.pop_back(); for (auto& childIslockHash : children) { - auto childIsLock = GetInstantSendLockByHashInternal(childIslockHash, false); - if (!childIsLock) { + auto childIsLockOpt = GetInstantSendLockByHashInternal(childIslockHash, false); + if (!childIsLockOpt.has_value()) { continue; } + auto childIsLock = childIsLockOpt.value(); RemoveInstantSendLock(batch, childIslockHash, childIsLock, false); WriteInstantSendLockArchived(batch, childIslockHash, nHeight); result.emplace_back(childIslockHash); @@ -472,8 +476,9 @@ void CInstantSendManager::ProcessTx(const CTransaction& tx, bool fRetroactive, c return; } - auto conflictingLock = GetConflictingLock(tx); - if (conflictingLock != nullptr) { + auto conflictingLockOpt = GetConflictingLock(tx); + if (conflictingLockOpt.has_value()) { + auto conflictingLock = conflictingLockOpt.value(); auto conflictingLockHash = ::SerializeHash(*conflictingLock); LogPrintf("CInstantSendManager::%s -- txid=%s: conflicts with islock %s, txid=%s\n", __func__, tx.GetHash().ToString(), conflictingLockHash.ToString(), conflictingLock->txid.ToString()); @@ -1000,16 +1005,17 @@ void CInstantSendManager::ProcessInstantSendLock(NodeId from, const uint256& has } } - const auto sameTxIsLock = db.GetInstantSendLockByTxid(islock->txid); - if (sameTxIsLock != nullptr) { + const auto sameTxIsLockOpt = db.GetInstantSendLockByTxid(islock->txid); + if (sameTxIsLockOpt.has_value()) { // can happen, nothing to do return; } + for (const auto& in : islock->inputs) { - const auto sameOutpointIsLock = db.GetInstantSendLockByInput(in); - if (sameOutpointIsLock != nullptr) { + const auto sameOutpointIsLockOpt = db.GetInstantSendLockByInput(in); + if (sameOutpointIsLockOpt.has_value()) { LogPrintf("CInstantSendManager::%s -- txid=%s, islock=%s: conflicting outpoint in islock. input=%s, other islock=%s, peer=%d\n", __func__, - islock->txid.ToString(), hash.ToString(), in.ToStringShort(), ::SerializeHash(*sameOutpointIsLock).ToString(), from); + islock->txid.ToString(), hash.ToString(), in.ToStringShort(), ::SerializeHash(*sameOutpointIsLockOpt.value()).ToString(), from); } } @@ -1092,13 +1098,14 @@ void CInstantSendManager::TransactionRemovedFromMempool(const CTransactionRef& t return; } - CInstantSendLockPtr islock = db.GetInstantSendLockByTxid(tx->GetHash()); - - if (islock == nullptr) { + auto islock_opt = db.GetInstantSendLockByTxid(tx->GetHash()); + if (!islock_opt.has_value()) { return; } LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- transaction %s was removed from mempool\n", __func__, tx->GetHash().ToString()); + + auto islock = islock_opt.value(); RemoveConflictingLock(::SerializeHash(*islock), *islock); } @@ -1503,7 +1510,7 @@ void CInstantSendManager::ProcessPendingRetryLockTxs() if (IsLocked(tx->GetHash())) { continue; } - if (GetConflictingLock(*tx) != nullptr) { + if (auto conflictingLockOpt = GetConflictingLock(*tx); conflictingLockOpt.has_value()) { // should not really happen as we have already filtered these out continue; } @@ -1545,29 +1552,29 @@ bool CInstantSendManager::GetInstantSendLockByHash(const uint256& hash, llmq::CI return false; } - auto islock = db.GetInstantSendLockByHash(hash); - if (!islock) { + auto islock_opt = db.GetInstantSendLockByHash(hash); + if (!islock_opt.has_value()) { LOCK(cs_pendingLocks); auto it = pendingInstantSendLocks.find(hash); if (it != pendingInstantSendLocks.end()) { - islock = it->second.second; + islock_opt = it->second.second; } else { auto itNoTx = pendingNoTxInstantSendLocks.find(hash); if (itNoTx != pendingNoTxInstantSendLocks.end()) { - islock = itNoTx->second.second; + islock_opt = itNoTx->second.second; } else { return false; } } } - ret = *islock; + ret = *islock_opt.value(); return true; } -CInstantSendLockPtr CInstantSendManager::GetInstantSendLockByTxid(const uint256& txid) const +std::optional CInstantSendManager::GetInstantSendLockByTxid(const uint256& txid) const { if (!IsInstantSendEnabled()) { - return nullptr; + return std::nullopt; } return db.GetInstantSendLockByTxid(txid); @@ -1601,23 +1608,23 @@ bool CInstantSendManager::IsWaitingForTx(const uint256& txHash) const return false; } -CInstantSendLockPtr CInstantSendManager::GetConflictingLock(const CTransaction& tx) const +std::optional CInstantSendManager::GetConflictingLock(const CTransaction& tx) const { if (!IsInstantSendEnabled()) { - return nullptr; + return std::nullopt; } for (const auto& in : tx.vin) { auto otherIsLock = db.GetInstantSendLockByInput(in.prevout); - if (!otherIsLock) { + if (!otherIsLock.has_value()) { continue; } - if (otherIsLock->txid != tx.GetHash()) { + if (otherIsLock.value()->txid != tx.GetHash()) { return otherIsLock; } } - return nullptr; + return std::nullopt; } size_t CInstantSendManager::GetInstantSendLockCount() const diff --git a/src/llmq/instantsend.h b/src/llmq/instantsend.h index c0a27d74944b8..20d969513858d 100644 --- a/src/llmq/instantsend.h +++ b/src/llmq/instantsend.h @@ -105,7 +105,7 @@ class CInstantSendDb /** * See GetInstantSendLockByHash */ - CInstantSendLockPtr GetInstantSendLockByHashInternal(const uint256& hash, bool use_cache = true) const EXCLUSIVE_LOCKS_REQUIRED(cs_db); + std::optional GetInstantSendLockByHashInternal(const uint256& hash, bool use_cache = true) const EXCLUSIVE_LOCKS_REQUIRED(cs_db); /** * See GetInstantSendLockHashByTxid @@ -156,7 +156,7 @@ class CInstantSendDb * @param use_cache Should we try using the cache first or not * @return A Pointer object to the IS Lock, returns nullptr if it doesn't exist */ - CInstantSendLockPtr GetInstantSendLockByHash(const uint256& hash, bool use_cache = true) const EXCLUSIVE_LOCKS_REQUIRED(!cs_db) + std::optional GetInstantSendLockByHash(const uint256& hash, bool use_cache = true) const EXCLUSIVE_LOCKS_REQUIRED(!cs_db) { LOCK(cs_db); return GetInstantSendLockByHashInternal(hash, use_cache); @@ -176,13 +176,13 @@ class CInstantSendDb * @param txid The txid for which the IS Lock Pointer is being returned * @return Returns the IS Lock Pointer associated with the txid, returns nullptr if it doesn't exist */ - CInstantSendLockPtr GetInstantSendLockByTxid(const uint256& txid) const EXCLUSIVE_LOCKS_REQUIRED(!cs_db); + std::optional GetInstantSendLockByTxid(const uint256& txid) const EXCLUSIVE_LOCKS_REQUIRED(!cs_db); /** * Gets an IS Lock pointer from an input given * @param outpoint Since all inputs are really just outpoints that are being spent * @return IS Lock Pointer associated with that input. */ - CInstantSendLockPtr GetInstantSendLockByInput(const COutPoint& outpoint) const EXCLUSIVE_LOCKS_REQUIRED(!cs_db); + std::optional GetInstantSendLockByInput(const COutPoint& outpoint) const EXCLUSIVE_LOCKS_REQUIRED(!cs_db); /** * Called when a ChainLock invalidated a IS Lock, removes any chained/children IS Locks and the invalidated IS Lock * @param islockHash IS Lock hash which has been invalidated @@ -328,7 +328,7 @@ class CInstantSendManager : public CRecoveredSigsListener public: bool IsLocked(const uint256& txHash) const; bool IsWaitingForTx(const uint256& txHash) const EXCLUSIVE_LOCKS_REQUIRED(!cs_pendingLocks); - CInstantSendLockPtr GetConflictingLock(const CTransaction& tx) const; + std::optional GetConflictingLock(const CTransaction& tx) const; [[nodiscard]] MessageProcessingResult HandleNewRecoveredSig(const CRecoveredSig& recoveredSig) override EXCLUSIVE_LOCKS_REQUIRED(!cs_creating, !cs_inputReqests, !cs_pendingLocks); @@ -345,7 +345,7 @@ class CInstantSendManager : public CRecoveredSigsListener bool AlreadyHave(const CInv& inv) const EXCLUSIVE_LOCKS_REQUIRED(!cs_pendingLocks); bool GetInstantSendLockByHash(const uint256& hash, CInstantSendLock& ret) const EXCLUSIVE_LOCKS_REQUIRED(!cs_pendingLocks); - CInstantSendLockPtr GetInstantSendLockByTxid(const uint256& txid) const; + std::optional GetInstantSendLockByTxid(const uint256& txid) const; void NotifyChainLock(const CBlockIndex* pindexChainLock) EXCLUSIVE_LOCKS_REQUIRED(!cs_inputReqests, !cs_nonLocked, !cs_pendingRetry); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 8cd7c089e0de5..0fc191983e28c 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2538,9 +2538,9 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::TX, *pblock->vtx[pair.first])); } for (PairType &pair : merkleBlock.vMatchedTxn) { - auto islock = isman.GetInstantSendLockByTxid(pair.second); - if (islock != nullptr) { - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::ISDLOCK, *islock)); + auto islock_opt = isman.GetInstantSendLockByTxid(pair.second); + if (islock_opt.has_value()) { + m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::ISDLOCK, *islock_opt.value())); } } } @@ -6034,10 +6034,10 @@ bool PeerManagerImpl::SendMessages(CNode* pto) tx_relay->m_tx_inventory_known_filter.insert(hash); queueAndMaybePushInv(CInv(nInvType, hash)); - const auto islock = m_llmq_ctx->isman->GetInstantSendLockByTxid(hash); - if (islock == nullptr) continue; + const auto islock_opt = m_llmq_ctx->isman->GetInstantSendLockByTxid(hash); + if (!islock_opt.has_value()) continue; if (pto->nVersion < ISDLOCK_PROTO_VERSION) continue; - uint256 isLockHash{::SerializeHash(*islock)}; + uint256 isLockHash{::SerializeHash(*islock_opt.value())}; tx_relay->m_tx_inventory_known_filter.insert(isLockHash); queueAndMaybePushInv(CInv(MSG_ISDLOCK, isLockHash)); } diff --git a/src/validation.cpp b/src/validation.cpp index 1074bb065ff30..ccd8e4026cf65 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -641,8 +641,9 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) return state.Invalid(TxValidationResult::TX_CONFLICT, "txn-already-in-mempool"); } - llmq::CInstantSendLockPtr conflictLock = llmq::quorumInstantSendManager->GetConflictingLock(tx); - if (conflictLock) { + auto conflictLockOpt = llmq::quorumInstantSendManager->GetConflictingLock(tx); + if (conflictLockOpt.has_value()) { + auto conflictLock = conflictLockOpt.value(); uint256 hashBlock; CTransactionRef txConflict = GetTransaction(/* block_index */ nullptr, &m_pool, conflictLock->txid, chainparams.GetConsensus(), hashBlock); if (txConflict) { @@ -2190,7 +2191,8 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state, for (const auto& tx : block.vtx) { // skip txes that have no inputs if (tx->vin.empty()) continue; - while (llmq::CInstantSendLockPtr conflictLock = m_isman->GetConflictingLock(*tx)) { + for (auto conflictLockOpt = m_isman->GetConflictingLock(*tx); conflictLockOpt.has_value(); conflictLockOpt = m_isman->GetConflictingLock(*tx)) { + auto conflictLock = conflictLockOpt.value(); if (has_chainlock) { LogPrint(BCLog::ALL, "ConnectBlock(DASH): chain-locked transaction %s overrides islock %s\n", tx->GetHash().ToString(), ::SerializeHash(*conflictLock).ToString()); From a197cb7bcf84b049650350727bc59389ec403293 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Mon, 8 Jul 2024 14:54:55 +0000 Subject: [PATCH 4/4] refactor: make `Get*MN*` and friends return `std::optional` --- src/coinjoin/client.cpp | 14 ++- src/coinjoin/server.cpp | 14 ++- src/evo/deterministicmns.cpp | 146 +++++++++++++----------- src/evo/deterministicmns.h | 26 ++--- src/evo/mnauth.cpp | 11 +- src/evo/simplifiedmns.cpp | 10 +- src/governance/governance.cpp | 17 +-- src/governance/object.cpp | 14 ++- src/governance/vote.cpp | 9 +- src/llmq/utils.cpp | 12 +- src/masternode/node.cpp | 14 ++- src/masternode/payments.cpp | 8 +- src/net.cpp | 34 +++--- src/net_processing.cpp | 17 +-- src/qt/masternodelist.cpp | 2 +- src/qt/rpcconsole.cpp | 6 +- src/rpc/evo.cpp | 34 +++--- src/rpc/governance.cpp | 11 +- src/rpc/masternode.cpp | 15 +-- src/rpc/quorums.cpp | 5 +- src/test/evo_deterministicmns_tests.cpp | 38 +++--- src/txmempool.cpp | 26 +++-- 22 files changed, 267 insertions(+), 216 deletions(-) diff --git a/src/coinjoin/client.cpp b/src/coinjoin/client.cpp index 9804b1be5bee9..0c7ac0a0e964d 100644 --- a/src/coinjoin/client.cpp +++ b/src/coinjoin/client.cpp @@ -59,8 +59,8 @@ PeerMsgRet CCoinJoinClientQueueManager::ProcessDSQueue(const CNode& peer, CDataS const auto tip_mn_list = m_dmnman.GetListAtChainTip(); if (dsq.masternodeOutpoint.IsNull()) { - if (auto dmn = tip_mn_list.GetValidMN(dsq.m_protxHash)) { - dsq.masternodeOutpoint = dmn->collateralOutpoint; + if (auto dmn_opt = tip_mn_list.GetValidMN(dsq.m_protxHash); dmn_opt.has_value()) { + dsq.masternodeOutpoint = dmn_opt.value()->collateralOutpoint; } else { return tl::unexpected{10}; } @@ -90,9 +90,10 @@ PeerMsgRet CCoinJoinClientQueueManager::ProcessDSQueue(const CNode& peer, CDataS if (dsq.IsTimeOutOfBounds()) return {}; - auto dmn = tip_mn_list.GetValidMNByCollateral(dsq.masternodeOutpoint); - if (!dmn) return {}; + auto dmn_opt = tip_mn_list.GetValidMNByCollateral(dsq.masternodeOutpoint); + if (!dmn_opt.has_value()) return {}; + auto dmn = dmn_opt.value(); if (dsq.m_protxHash.IsNull()) { dsq.m_protxHash = dmn->proTxHash; } @@ -1085,14 +1086,15 @@ bool CCoinJoinClientSession::JoinExistingQueue(CAmount nBalanceNeedsAnonymized, // Look through the queues and see if anything matches CCoinJoinQueue dsq; while (m_queueman->GetQueueItemAndTry(dsq)) { - auto dmn = mnList.GetValidMNByCollateral(dsq.masternodeOutpoint); + auto dmn_opt = mnList.GetValidMNByCollateral(dsq.masternodeOutpoint); - if (!dmn) { + if (!dmn_opt) { WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::JoinExistingQueue -- dsq masternode is not in masternode list, masternode=%s\n", dsq.masternodeOutpoint.ToStringShort()); continue; } // skip next mn payments winners + auto dmn = dmn_opt.value(); if (dmn->pdmnState->nLastPaidHeight + nWeightedMnCount < mnList.GetHeight() + WinnersToSkip()) { WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::JoinExistingQueue -- skipping winner, masternode=%s\n", dmn->proTxHash.ToString()); continue; diff --git a/src/coinjoin/server.cpp b/src/coinjoin/server.cpp index bb65990dd9006..96187fa40927e 100644 --- a/src/coinjoin/server.cpp +++ b/src/coinjoin/server.cpp @@ -60,12 +60,13 @@ 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 = mnList.GetValidMNByCollateral(m_mn_activeman->GetOutPoint()); - if (!dmn) { + auto dmn_opt = mnList.GetValidMNByCollateral(m_mn_activeman->GetOutPoint()); + if (!dmn_opt.has_value()) { PushStatus(peer, STATUS_REJECTED, ERR_MN_LIST); return; } + auto dmn = dmn_opt.value(); if (vecSessionCollaterals.empty()) { { TRY_LOCK(cs_vecqueue, lockRecv); @@ -129,8 +130,8 @@ PeerMsgRet CCoinJoinServer::ProcessDSQUEUE(const CNode& peer, CDataStream& vRecv const auto tip_mn_list = m_dmnman.GetListAtChainTip(); if (dsq.masternodeOutpoint.IsNull()) { - if (auto dmn = tip_mn_list.GetValidMN(dsq.m_protxHash)) { - dsq.masternodeOutpoint = dmn->collateralOutpoint; + if (auto dmn_opt = tip_mn_list.GetValidMN(dsq.m_protxHash); dmn_opt.has_value()) { + dsq.masternodeOutpoint = dmn_opt.value()->collateralOutpoint; } else { return tl::unexpected{10}; } @@ -157,9 +158,10 @@ PeerMsgRet CCoinJoinServer::ProcessDSQUEUE(const CNode& peer, CDataStream& vRecv if (dsq.IsTimeOutOfBounds()) return {}; - auto dmn = tip_mn_list.GetValidMNByCollateral(dsq.masternodeOutpoint); - if (!dmn) return {}; + auto dmn_opt = tip_mn_list.GetValidMNByCollateral(dsq.masternodeOutpoint); + if (!dmn_opt.has_value()) return {}; + auto dmn = dmn_opt.value(); if (dsq.m_protxHash.IsNull()) { dsq.m_protxHash = dmn->proTxHash; } diff --git a/src/evo/deterministicmns.cpp b/src/evo/deterministicmns.cpp index 96fbe4c02cf02..a2e4d9beeb958 100644 --- a/src/evo/deterministicmns.cpp +++ b/src/evo/deterministicmns.cpp @@ -92,58 +92,58 @@ bool CDeterministicMNList::IsMNPoSeBanned(const CDeterministicMN& dmn) return dmn.pdmnState->IsBanned(); } -CDeterministicMNCPtr CDeterministicMNList::GetMN(const uint256& proTxHash) const +std::optional CDeterministicMNList::GetMN(const uint256& proTxHash) const { auto p = mnMap.find(proTxHash); if (p == nullptr) { - return nullptr; + return std::nullopt; } return *p; } -CDeterministicMNCPtr CDeterministicMNList::GetValidMN(const uint256& proTxHash) const +std::optional CDeterministicMNList::GetValidMN(const uint256& proTxHash) const { auto dmn = GetMN(proTxHash); - if (dmn && !IsMNValid(*dmn)) { - return nullptr; + if (dmn.has_value() && !IsMNValid(*dmn.value())) { + return std::nullopt; } return dmn; } -CDeterministicMNCPtr CDeterministicMNList::GetMNByOperatorKey(const CBLSPublicKey& pubKey) const +std::optional CDeterministicMNList::GetMNByOperatorKey(const CBLSPublicKey& pubKey) const { const auto it = ranges::find_if(mnMap, [&pubKey](const auto& p){return p.second->pdmnState->pubKeyOperator.Get() == pubKey;}); if (it == mnMap.end()) { - return nullptr; + return std::nullopt; } return it->second; } -CDeterministicMNCPtr CDeterministicMNList::GetMNByCollateral(const COutPoint& collateralOutpoint) const +std::optional CDeterministicMNList::GetMNByCollateral(const COutPoint& collateralOutpoint) const { return GetUniquePropertyMN(collateralOutpoint); } -CDeterministicMNCPtr CDeterministicMNList::GetValidMNByCollateral(const COutPoint& collateralOutpoint) const +std::optional CDeterministicMNList::GetValidMNByCollateral(const COutPoint& collateralOutpoint) const { - auto dmn = GetMNByCollateral(collateralOutpoint); - if (dmn && !IsMNValid(*dmn)) { - return nullptr; + auto dmn_opt = GetMNByCollateral(collateralOutpoint); + if (dmn_opt.has_value() && !IsMNValid(*dmn_opt.value())) { + return std::nullopt; } - return dmn; + return dmn_opt; } -CDeterministicMNCPtr CDeterministicMNList::GetMNByService(const CService& service) const +std::optional CDeterministicMNList::GetMNByService(const CService& service) const { return GetUniquePropertyMN(service); } -CDeterministicMNCPtr CDeterministicMNList::GetMNByInternalId(uint64_t internalId) const +std::optional CDeterministicMNList::GetMNByInternalId(uint64_t internalId) const { auto proTxHash = mnInternalIdMap.find(internalId); if (!proTxHash) { - return nullptr; + return std::nullopt; } return GetMN(*proTxHash); } @@ -174,10 +174,10 @@ static bool CompareByLastPaid(const CDeterministicMN* _a, const CDeterministicMN return CompareByLastPaid(*_a, *_b); } -CDeterministicMNCPtr CDeterministicMNList::GetMNPayee(gsl::not_null pindexPrev) const +std::optional CDeterministicMNList::GetMNPayee(gsl::not_null pindexPrev) const { if (mnMap.size() == 0) { - return nullptr; + return std::nullopt; } const bool isv19Active{DeploymentActiveAfter(pindexPrev, Params().GetConsensus(), Consensus::DEPLOYMENT_V19)}; @@ -208,6 +208,7 @@ CDeterministicMNCPtr CDeterministicMNList::GetMNPayee(gsl::not_null 0); - auto dmn = GetMN(proTxHash); - if (!dmn) { + auto dmn_opt = GetMN(proTxHash); + if (!dmn_opt.has_value()) { throw(std::runtime_error(strprintf("%s: Can't find a masternode with proTxHash=%s", __func__, proTxHash.ToString()))); } + auto dmn = dmn_opt.value(); int maxPenalty = CalcMaxPoSePenalty(); auto newState = std::make_shared(*dmn->pdmnState); @@ -394,12 +396,15 @@ CDeterministicMNListDiff CDeterministicMNList::BuildDiff(const CDeterministicMNL to.ForEachMNShared(false, [this, &diffRet](const CDeterministicMNCPtr& toPtr) { auto fromPtr = GetMN(toPtr->proTxHash); - if (fromPtr == nullptr) { + if (!fromPtr.has_value()) { diffRet.addedMNs.emplace_back(toPtr); - } else if (fromPtr != toPtr || fromPtr->pdmnState != toPtr->pdmnState) { - CDeterministicMNStateDiff stateDiff(*fromPtr->pdmnState, *toPtr->pdmnState); - if (stateDiff.fields) { - diffRet.updatedMNs.emplace(toPtr->GetInternalId(), std::move(stateDiff)); + } else { + auto fromPdmnState = fromPtr.value()->pdmnState; + if (fromPtr != toPtr || fromPdmnState != toPtr->pdmnState) { + CDeterministicMNStateDiff stateDiff(*fromPdmnState, *toPtr->pdmnState); + if (stateDiff.fields) { + diffRet.updatedMNs.emplace(toPtr->GetInternalId(), std::move(stateDiff)); + } } } }); @@ -426,21 +431,21 @@ CDeterministicMNList CDeterministicMNList::ApplyDiff(gsl::not_nullnHeight; for (const auto& id : diff.removedMns) { - auto dmn = result.GetMNByInternalId(id); - if (!dmn) { + auto dmn_opt = result.GetMNByInternalId(id); + if (!dmn_opt.has_value()) { throw std::runtime_error(strprintf("%s: can't find a removed masternode, id=%d", __func__, id)); } - result.RemoveMN(dmn->proTxHash); + result.RemoveMN(dmn_opt.value()->proTxHash); } for (const auto& dmn : diff.addedMNs) { result.AddMN(dmn); } for (const auto& p : diff.updatedMNs) { - auto dmn = result.GetMNByInternalId(p.first); - if (!dmn) { + auto dmn_opt = result.GetMNByInternalId(p.first); + if (!dmn_opt.has_value()) { throw std::runtime_error(strprintf("%s: can't find an updated masternode, id=%d", __func__, p.first)); } - result.UpdateMN(*dmn, p.second); + result.UpdateMN(*dmn_opt.value(), p.second); } return result; @@ -553,8 +558,8 @@ void CDeterministicMNList::UpdateMN(const CDeterministicMN& oldDmn, const CDeter void CDeterministicMNList::RemoveMN(const uint256& proTxHash) { - auto dmn = GetMN(proTxHash); - if (!dmn) { + auto dmn_opt = GetMN(proTxHash); + if (!dmn_opt.has_value()) { throw(std::runtime_error(strprintf("%s: Can't find a masternode with proTxHash=%s", __func__, proTxHash.ToString()))); } @@ -562,6 +567,7 @@ void CDeterministicMNList::RemoveMN(const uint256& proTxHash) // Using this temporary map as a checkpoint to roll back to in case of any issues. decltype(mnUniquePropertyMap) mnUniquePropertyMapSaved = mnUniquePropertyMap; + auto dmn = dmn_opt.value(); if (!DeleteUniqueProperty(*dmn, dmn->collateralOutpoint)) { mnUniquePropertyMap = mnUniquePropertyMapSaved; throw(std::runtime_error(strprintf("%s: Can't delete a masternode %s with a collateralOutpoint=%s", __func__, @@ -709,7 +715,7 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, gsl::no newList.SetBlockHash(uint256()); // we can't know the final block hash, so better not return a (invalid) block hash newList.SetHeight(nHeight); - auto payee = oldList.GetMNPayee(pindexPrev); + auto payee_opt = oldList.GetMNPayee(pindexPrev); // we iterate the oldList here and update the newList // this is only valid as long these have not diverged at this point, which is the case as long as we don't add @@ -772,8 +778,9 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, gsl::no return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-collateral"); } - auto replacedDmn = newList.GetMNByCollateral(dmn->collateralOutpoint); - if (replacedDmn != nullptr) { + auto replacedDmnOpt = newList.GetMNByCollateral(dmn->collateralOutpoint); + if (replacedDmnOpt.has_value()) { + auto replacedDmn = replacedDmnOpt.value(); // This might only happen with a ProRegTx that refers an external collateral // In that case the new ProRegTx will replace the old one. This means the old one is removed // and the new one is added like a completely fresh one, which is also at the bottom of the payment list @@ -817,14 +824,16 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, gsl::no return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-payload"); } - if (newList.HasUniqueProperty(opt_proTx->addr) && newList.GetUniquePropertyMN(opt_proTx->addr)->proTxHash != opt_proTx->proTxHash) { + if (auto uniq_opt = newList.GetUniquePropertyMN(opt_proTx->addr); uniq_opt.has_value() && uniq_opt.value()->proTxHash != opt_proTx->proTxHash) { return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-dup-addr"); } - auto dmn = newList.GetMN(opt_proTx->proTxHash); - if (!dmn) { + auto dmn_opt = newList.GetMN(opt_proTx->proTxHash); + if (!dmn_opt.has_value()) { return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-hash"); } + + auto dmn = dmn_opt.value(); if (opt_proTx->nType != dmn->nType) { return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-type-mismatch"); } @@ -862,11 +871,11 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, gsl::no return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-payload"); } - auto dmn = newList.GetMN(opt_proTx->proTxHash); - if (!dmn) { + auto dmn_opt = newList.GetMN(opt_proTx->proTxHash); + if (!dmn_opt.has_value()) { return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-hash"); } - auto newState = std::make_shared(*dmn->pdmnState); + auto newState = std::make_shared(*dmn_opt.value()->pdmnState); if (newState->pubKeyOperator != opt_proTx->pubKeyOperator) { // reset all operator related fields and put MN into PoSe-banned state in case the operator key changes newState->ResetOperatorFields(); @@ -890,11 +899,11 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, gsl::no return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-payload"); } - auto dmn = newList.GetMN(opt_proTx->proTxHash); - if (!dmn) { + auto dmn_opt = newList.GetMN(opt_proTx->proTxHash); + if (!dmn_opt.has_value()) { return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-hash"); } - auto newState = std::make_shared(*dmn->pdmnState); + auto newState = std::make_shared(*dmn_opt.value()->pdmnState); newState->ResetOperatorFields(); newState->BanIfNotBanned(nHeight); newState->nRevocationReason = opt_proTx->nReason; @@ -934,8 +943,9 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, gsl::no // check if any existing MN collateral is spent by this transaction for (const auto& in : tx.vin) { - auto dmn = newList.GetMNByCollateral(in.prevout); - if (dmn && dmn->collateralOutpoint == in.prevout) { + auto dmn_opt = newList.GetMNByCollateral(in.prevout); + if (dmn_opt.has_value() && dmn_opt.value()->collateralOutpoint == in.prevout) { + auto dmn = dmn_opt.value(); newList.RemoveMN(dmn->proTxHash); if (debugLogs) { @@ -948,10 +958,12 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, gsl::no // The payee for the current block was determined by the previous block's list, but it might have disappeared in the // current block. We still pay that MN one last time, however. - if (payee && newList.HasMN(payee->proTxHash)) { - auto dmn = newList.GetMN(payee->proTxHash); + if (payee_opt.has_value() && newList.HasMN(payee_opt.value()->proTxHash)) { + auto payee = payee_opt.value(); + auto dmn_opt = newList.GetMN(payee->proTxHash); // HasMN has reported that GetMN should succeed, enforce that. - assert(dmn); + assert(dmn_opt.has_value()); + auto dmn = dmn_opt.value(); auto newState = std::make_shared(*dmn->pdmnState); newState->nLastPaidHeight = nHeight; // Starting from v19 and until MNRewardReallocation, EvoNodes will be paid 4 blocks in a row @@ -966,10 +978,11 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, gsl::no } newList.UpdateMN(payee->proTxHash, newState); if (debugLogs) { - dmn = newList.GetMN(payee->proTxHash); + dmn_opt = newList.GetMN(payee->proTxHash); // Since the previous GetMN query returned a value, after an update, querying the same // hash *must* give us a result. If it doesn't, that would be a potential logic bug. - assert(dmn); + assert(dmn_opt.has_value()); + dmn = dmn_opt.value(); LogPrint(BCLog::MNPAYMENTS, "CDeterministicMNManager::%s -- MN %s, nConsecutivePayments=%d\n", __func__, dmn->proTxHash.ToString(), dmn->pdmnState->nConsecutivePayments); } @@ -979,7 +992,7 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, gsl::no auto newList2 = newList; newList2.ForEachMN(false, [&](auto& dmn) { if (dmn.nType != MnType::Evo) return; - if (payee != nullptr && dmn.proTxHash == payee->proTxHash && !isMNRewardReallocation) return; + if (payee_opt.has_value() && dmn.proTxHash == payee_opt.value()->proTxHash && !isMNRewardReallocation) return; if (dmn.pdmnState->nConsecutivePayments == 0) return; if (debugLogs) { LogPrint(BCLog::MNPAYMENTS, "CDeterministicMNManager::%s -- MN %s, reset nConsecutivePayments %d->0\n", @@ -1609,7 +1622,7 @@ bool CheckProRegTx(CDeterministicMNManager& dmnman, const CTransaction& tx, gsl: auto mnList = dmnman.GetListForBlock(pindexPrev); // only allow reusing of addresses when it's for the same collateral (which replaces the old MN) - if (mnList.HasUniqueProperty(opt_ptx->addr) && mnList.GetUniquePropertyMN(opt_ptx->addr)->collateralOutpoint != collateralOutpoint) { + if (auto unique_opt = mnList.GetUniquePropertyMN(opt_ptx->addr); unique_opt.has_value() && unique_opt.value()->collateralOutpoint != collateralOutpoint) { return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-dup-addr"); } @@ -1673,19 +1686,20 @@ bool CheckProUpServTx(CDeterministicMNManager& dmnman, const CTransaction& tx, g } auto mnList = dmnman.GetListForBlock(pindexPrev); - auto mn = mnList.GetMN(opt_ptx->proTxHash); - if (!mn) { + auto mn_opt = mnList.GetMN(opt_ptx->proTxHash); + if (!mn_opt.has_value()) { return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-hash"); } + auto mn = mn_opt.value(); // don't allow updating to addresses already used by other MNs - if (mnList.HasUniqueProperty(opt_ptx->addr) && mnList.GetUniquePropertyMN(opt_ptx->addr)->proTxHash != opt_ptx->proTxHash) { + if (auto unique_opt = mnList.GetUniquePropertyMN(opt_ptx->addr); unique_opt.has_value() && unique_opt.value()->proTxHash != opt_ptx->proTxHash) { return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-dup-addr"); } // don't allow updating to platformNodeIds already used by other EvoNodes if (opt_ptx->nType == MnType::Evo) { - if (mnList.HasUniqueProperty(opt_ptx->platformNodeID) && mnList.GetUniquePropertyMN(opt_ptx->platformNodeID)->proTxHash != opt_ptx->proTxHash) { + if (auto unique_opt = mnList.GetUniquePropertyMN(opt_ptx->platformNodeID); unique_opt.has_value() && unique_opt.value()->proTxHash != opt_ptx->proTxHash) { return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-dup-platformnodeid"); } } @@ -1728,11 +1742,12 @@ bool CheckProUpRegTx(CDeterministicMNManager& dmnman, const CTransaction& tx, gs } auto mnList = dmnman.GetListForBlock(pindexPrev); - auto dmn = mnList.GetMN(opt_ptx->proTxHash); - if (!dmn) { + auto dmn_opt = mnList.GetMN(opt_ptx->proTxHash); + if (!dmn_opt.has_value()) { return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-hash"); } + auto dmn = dmn_opt.value(); // don't allow reuse of payee key for other keys (don't allow people to put the payee key onto an online server) if (payoutDest == CTxDestination(PKHash(dmn->pdmnState->keyIDOwner)) || payoutDest == CTxDestination(PKHash(opt_ptx->keyIDVoting))) { return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-payee-reuse"); @@ -1753,9 +1768,8 @@ bool CheckProUpRegTx(CDeterministicMNManager& dmnman, const CTransaction& tx, gs return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-collateral-reuse"); } - if (mnList.HasUniqueProperty(opt_ptx->pubKeyOperator)) { - auto otherDmn = mnList.GetUniquePropertyMN(opt_ptx->pubKeyOperator); - if (opt_ptx->proTxHash != otherDmn->proTxHash) { + if (auto otherDmnOpt = mnList.GetUniquePropertyMN(opt_ptx->pubKeyOperator); otherDmnOpt.has_value()) { + if (opt_ptx->proTxHash != otherDmnOpt.value()->proTxHash) { return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-dup-key"); } } @@ -1787,15 +1801,15 @@ bool CheckProUpRevTx(CDeterministicMNManager& dmnman, const CTransaction& tx, gs } auto mnList = dmnman.GetListForBlock(pindexPrev); - auto dmn = mnList.GetMN(opt_ptx->proTxHash); - if (!dmn) + auto dmn_opt = mnList.GetMN(opt_ptx->proTxHash); + if (!dmn_opt) return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-hash"); if (!CheckInputsHash(tx, *opt_ptx, state)) { // pass the state returned by the function above return false; } - if (check_sigs && !CheckHashSig(*opt_ptx, dmn->pdmnState->pubKeyOperator.Get(), state)) { + if (check_sigs && !CheckHashSig(*opt_ptx, dmn_opt.value()->pdmnState->pubKeyOperator.Get(), state)) { // pass the state returned by the function above return false; } diff --git a/src/evo/deterministicmns.h b/src/evo/deterministicmns.h index 52119350f16eb..e1fd25a92b24e 100644 --- a/src/evo/deterministicmns.h +++ b/src/evo/deterministicmns.h @@ -326,24 +326,24 @@ class CDeterministicMNList [[nodiscard]] bool HasMN(const uint256& proTxHash) const { - return GetMN(proTxHash) != nullptr; + return GetMN(proTxHash).has_value(); } [[nodiscard]] bool HasMNByCollateral(const COutPoint& collateralOutpoint) const { - return GetMNByCollateral(collateralOutpoint) != nullptr; + return GetMNByCollateral(collateralOutpoint).has_value(); } [[nodiscard]] bool HasValidMNByCollateral(const COutPoint& collateralOutpoint) const { - return GetValidMNByCollateral(collateralOutpoint) != nullptr; + return GetValidMNByCollateral(collateralOutpoint).has_value(); } - [[nodiscard]] CDeterministicMNCPtr GetMN(const uint256& proTxHash) const; - [[nodiscard]] CDeterministicMNCPtr GetValidMN(const uint256& proTxHash) const; - [[nodiscard]] CDeterministicMNCPtr GetMNByOperatorKey(const CBLSPublicKey& pubKey) const; - [[nodiscard]] CDeterministicMNCPtr GetMNByCollateral(const COutPoint& collateralOutpoint) const; - [[nodiscard]] CDeterministicMNCPtr GetValidMNByCollateral(const COutPoint& collateralOutpoint) const; - [[nodiscard]] CDeterministicMNCPtr GetMNByService(const CService& service) const; - [[nodiscard]] CDeterministicMNCPtr GetMNByInternalId(uint64_t internalId) const; - [[nodiscard]] CDeterministicMNCPtr GetMNPayee(gsl::not_null pindexPrev) const; + [[nodiscard]] std::optional GetMN(const uint256& proTxHash) const; + [[nodiscard]] std::optional GetValidMN(const uint256& proTxHash) const; + [[nodiscard]] std::optional GetMNByOperatorKey(const CBLSPublicKey& pubKey) const; + [[nodiscard]] std::optional GetMNByCollateral(const COutPoint& collateralOutpoint) const; + [[nodiscard]] std::optional GetValidMNByCollateral(const COutPoint& collateralOutpoint) const; + [[nodiscard]] std::optional GetMNByService(const CService& service) const; + [[nodiscard]] std::optional GetMNByInternalId(uint64_t internalId) const; + [[nodiscard]] std::optional GetMNPayee(gsl::not_null pindexPrev) const; /** * Calculates the projected MN payees for the next *count* blocks. The result is not guaranteed to be correct @@ -401,11 +401,11 @@ class CDeterministicMNList return mnUniquePropertyMap.count(GetUniquePropertyHash(v)) != 0; } template - [[nodiscard]] CDeterministicMNCPtr GetUniquePropertyMN(const T& v) const + [[nodiscard]] std::optional GetUniquePropertyMN(const T& v) const { auto p = mnUniquePropertyMap.find(GetUniquePropertyHash(v)); if (!p) { - return nullptr; + return std::nullopt; } return GetMN(p->first); } diff --git a/src/evo/mnauth.cpp b/src/evo/mnauth.cpp index 63a9cd32043d3..cb7ef1c76aebe 100644 --- a/src/evo/mnauth.cpp +++ b/src/evo/mnauth.cpp @@ -93,14 +93,15 @@ PeerMsgRet CMNAuth::ProcessMessage(CNode& peer, ServiceFlags node_services, CCon return tl::unexpected{MisbehavingError{100, "invalid mnauth signature"}}; } - const auto dmn = tip_mn_list.GetMN(mnauth.proRegTxHash); - if (!dmn) { + const auto dmn_opt = tip_mn_list.GetMN(mnauth.proRegTxHash); + if (!dmn_opt.has_value()) { // in case node was unlucky and not up to date, just let it be connected as a regular node, which gives it // a chance to get up-to-date and thus realize that it's not a MN anymore. We still give it a // low DoS score. return tl::unexpected{MisbehavingError{10, "missing mnauth masternode"}}; } + auto dmn = dmn_opt.value(); uint256 signHash; int nOurNodeVersion{PROTOCOL_VERSION}; if (Params().NetworkIDString() != CBaseChainParams::MAIN && gArgs.IsArgSet("-pushversion")) { @@ -206,10 +207,12 @@ void CMNAuth::NotifyMasternodeListChanged(bool undo, const CDeterministicMNList& if (verifiedProRegTxHash.IsNull()) { return; } - const auto verifiedDmn = oldMNList.GetMN(verifiedProRegTxHash); - if (!verifiedDmn) { + const auto verifiedDmnOpt = oldMNList.GetMN(verifiedProRegTxHash); + if (!verifiedDmnOpt.has_value()) { return; } + + const auto verifiedDmn = verifiedDmnOpt.value(); bool doRemove = false; if (diff.removedMns.count(verifiedDmn->GetInternalId())) { doRemove = true; diff --git a/src/evo/simplifiedmns.cpp b/src/evo/simplifiedmns.cpp index 573edbc1f10ff..3d629090f7cf4 100644 --- a/src/evo/simplifiedmns.cpp +++ b/src/evo/simplifiedmns.cpp @@ -308,13 +308,13 @@ CSimplifiedMNListDiff BuildSimplifiedDiff(const CDeterministicMNList& from, cons diffRet.blockHash = to.GetBlockHash(); to.ForEachMN(false, [&](const auto& toPtr) { - auto fromPtr = from.GetMN(toPtr.proTxHash); - if (fromPtr == nullptr) { + auto fromPtrOpt = from.GetMN(toPtr.proTxHash); + if (!fromPtrOpt.has_value()) { CSimplifiedMNListEntry sme(toPtr); diffRet.mnList.push_back(std::move(sme)); } else { CSimplifiedMNListEntry sme1(toPtr); - CSimplifiedMNListEntry sme2(*fromPtr); + CSimplifiedMNListEntry sme2(*fromPtrOpt.value()); if ((sme1 != sme2) || (extended && (sme1.scriptPayout != sme2.scriptPayout || sme1.scriptOperatorPayout != sme2.scriptOperatorPayout))) { diffRet.mnList.push_back(std::move(sme1)); @@ -323,8 +323,8 @@ CSimplifiedMNListDiff BuildSimplifiedDiff(const CDeterministicMNList& from, cons }); from.ForEachMN(false, [&](auto& fromPtr) { - auto toPtr = to.GetMN(fromPtr.proTxHash); - if (toPtr == nullptr) { + auto toPtrOpt = to.GetMN(fromPtr.proTxHash); + if (!toPtrOpt.has_value()) { diffRet.deletedMNs.emplace_back(fromPtr.proTxHash); } }); diff --git a/src/governance/governance.cpp b/src/governance/governance.cpp index deea85ce26844..8181136fa3560 100644 --- a/src/governance/governance.cpp +++ b/src/governance/governance.cpp @@ -536,9 +536,9 @@ std::vector CGovernanceManager::GetCurrentVotes(const uint256& mapMasternodes.emplace(dmn->collateralOutpoint, dmn); }); } else { - auto dmn = tip_mn_list.GetMNByCollateral(mnCollateralOutpointFilter); - if (dmn) { - mapMasternodes.emplace(dmn->collateralOutpoint, dmn); + auto dmn_opt = tip_mn_list.GetMNByCollateral(mnCollateralOutpointFilter); + if (dmn_opt.has_value()) { + mapMasternodes.emplace(dmn_opt.value()->collateralOutpoint, dmn_opt.value()); } } @@ -1600,10 +1600,11 @@ void CGovernanceManager::RemoveInvalidVotes() std::vector changedKeyMNs; for (const auto& p : diff.updatedMNs) { - auto oldDmn = lastMNListForVotingKeys->GetMNByInternalId(p.first); + auto oldDmnOpt = lastMNListForVotingKeys->GetMNByInternalId(p.first); // BuildDiff will construct itself with MNs that we already have knowledge // of, meaning that fetch operations should never fail. - assert(oldDmn); + assert(oldDmnOpt.has_value()); + auto oldDmn = oldDmnOpt.value(); if ((p.second.fields & CDeterministicMNStateDiff::Field_keyIDVoting) && p.second.state.keyIDVoting != oldDmn->pdmnState->keyIDVoting) { changedKeyMNs.emplace_back(oldDmn->collateralOutpoint); } else if ((p.second.fields & CDeterministicMNStateDiff::Field_pubKeyOperator) && p.second.state.pubKeyOperator != oldDmn->pdmnState->pubKeyOperator) { @@ -1611,11 +1612,11 @@ void CGovernanceManager::RemoveInvalidVotes() } } for (const auto& id : diff.removedMns) { - auto oldDmn = lastMNListForVotingKeys->GetMNByInternalId(id); + auto oldDmnOpt = lastMNListForVotingKeys->GetMNByInternalId(id); // BuildDiff will construct itself with MNs that we already have knowledge // of, meaning that fetch operations should never fail. - assert(oldDmn); - changedKeyMNs.emplace_back(oldDmn->collateralOutpoint); + assert(oldDmnOpt.has_value()); + changedKeyMNs.emplace_back(oldDmnOpt.value()->collateralOutpoint); } for (const auto& outpoint : changedKeyMNs) { diff --git a/src/governance/object.cpp b/src/governance/object.cpp index 6ae0ed619b464..52caa13070d72 100644 --- a/src/governance/object.cpp +++ b/src/governance/object.cpp @@ -72,14 +72,15 @@ bool CGovernanceObject::ProcessVote(CMasternodeMetaMan& mn_metaman, CGovernanceM return false; } - auto dmn = tip_mn_list.GetMNByCollateral(vote.GetMasternodeOutpoint()); - if (!dmn) { + auto dmn_opt = tip_mn_list.GetMNByCollateral(vote.GetMasternodeOutpoint()); + if (!dmn_opt.has_value()) { std::ostringstream ostr; ostr << "CGovernanceObject::ProcessVote -- Masternode " << vote.GetMasternodeOutpoint().ToStringShort() << " not found"; exception = CGovernanceException(ostr.str(), GOVERNANCE_EXCEPTION_PERMANENT_ERROR, 20); return false; } + auto dmn = dmn_opt.value(); auto it = mapCurrentMNVotes.emplace(vote_m_t::value_type(vote.GetMasternodeOutpoint(), vote_rec_t())).first; vote_rec_t& voteRecordRef = it->second; vote_signal_enum_t eSignal = vote.GetSignal(); @@ -421,12 +422,13 @@ bool CGovernanceObject::IsValidLocally(const CDeterministicMNList& tip_mn_list, } std::string strOutpoint = m_obj.masternodeOutpoint.ToStringShort(); - auto dmn = tip_mn_list.GetMNByCollateral(m_obj.masternodeOutpoint); - if (!dmn) { + auto dmn_opt = tip_mn_list.GetMNByCollateral(m_obj.masternodeOutpoint); + if (!dmn_opt.has_value()) { strError = "Failed to find Masternode by UTXO, missing masternode=" + strOutpoint; return false; } + auto dmn = dmn_opt.value(); // Check that we have a valid MN signature if (!CheckSignature(dmn->pdmnState->pubKeyOperator.Get())) { strError = "Invalid masternode signature for: " + strOutpoint + ", pubkey = " + dmn->pdmnState->pubKeyOperator.ToString(); @@ -556,8 +558,8 @@ int CGovernanceObject::CountMatchingVotes(const CDeterministicMNList& tip_mn_lis if (it2 != recVote.mapInstances.end() && it2->second.eOutcome == eVoteOutcomeIn) { // 4x times weight vote for EvoNode owners. // No need to check if v19 is active since no EvoNode are allowed to register before v19s - auto dmn = tip_mn_list.GetMNByCollateral(votepair.first); - if (dmn != nullptr) nCount += GetMnType(dmn->nType).voting_weight; + auto dmn_opt = tip_mn_list.GetMNByCollateral(votepair.first); + if (dmn_opt.has_value()) nCount += GetMnType(dmn_opt.value()->nType).voting_weight; } } return nCount; diff --git a/src/governance/vote.cpp b/src/governance/vote.cpp index 5cfa2b1ac1869..d4fe7b10a76a4 100644 --- a/src/governance/vote.cpp +++ b/src/governance/vote.cpp @@ -98,8 +98,8 @@ CGovernanceVote::CGovernanceVote(const COutPoint& outpointMasternodeIn, const ui std::string CGovernanceVote::ToString(const CDeterministicMNList& tip_mn_list) const { - auto dmn = tip_mn_list.GetMNByCollateral(masternodeOutpoint); - int voteWeight = dmn != nullptr ? GetMnType(dmn->nType).voting_weight : 0; + auto dmn_opt = tip_mn_list.GetMNByCollateral(masternodeOutpoint); + int voteWeight = dmn_opt.has_value() ? GetMnType(dmn_opt.value()->nType).voting_weight : 0; std::ostringstream ostr; ostr << masternodeOutpoint.ToStringShort() << ":" << nTime << ":" @@ -209,12 +209,13 @@ bool CGovernanceVote::IsValid(const CDeterministicMNList& tip_mn_list, bool useV return false; } - auto dmn = tip_mn_list.GetMNByCollateral(masternodeOutpoint); - if (!dmn) { + auto dmn_opt = tip_mn_list.GetMNByCollateral(masternodeOutpoint); + if (!dmn_opt.has_value()) { LogPrint(BCLog::GOBJECT, "CGovernanceVote::IsValid -- Unknown Masternode - %s\n", masternodeOutpoint.ToStringShort()); return false; } + auto dmn = dmn_opt.value(); if (useVotingKey) { return CheckSignature(dmn->pdmnState->keyIDVoting); } else { diff --git a/src/llmq/utils.cpp b/src/llmq/utils.cpp index 59147750ee657..5cf80285d406c 100644 --- a/src/llmq/utils.cpp +++ b/src/llmq/utils.cpp @@ -792,11 +792,11 @@ bool EnsureQuorumConnections(const Consensus::LLMQParams& llmqParams, CConnman& LogAcceptCategory(BCLog::LLMQ, BCLog::Level::Debug)) { std::string debugMsg = strprintf("%s -- adding masternodes quorum connections for quorum %s:\n", __func__, pQuorumBaseBlockIndex->GetBlockHash().ToString()); for (const auto& c : connections) { - auto dmn = tip_mn_list.GetValidMN(c); - if (!dmn) { + auto dmn_opt = tip_mn_list.GetValidMN(c); + if (!dmn_opt.has_value()) { debugMsg += strprintf(" %s (not in valid MN set anymore)\n", c.ToString()); } else { - debugMsg += strprintf(" %s (%s)\n", c.ToString(), dmn->pdmnState->addr.ToStringAddrPort()); + debugMsg += strprintf(" %s (%s)\n", c.ToString(), dmn_opt.value()->pdmnState->addr.ToStringAddrPort()); } } LogPrint(BCLog::NET_NETCONN, debugMsg.c_str()); /* Continued */ @@ -839,11 +839,11 @@ void AddQuorumProbeConnections(const Consensus::LLMQParams& llmqParams, CConnman if (LogAcceptCategory(BCLog::LLMQ, BCLog::Level::Debug)) { std::string debugMsg = strprintf("%s -- adding masternodes probes for quorum %s:\n", __func__, pQuorumBaseBlockIndex->GetBlockHash().ToString()); for (const auto& c : probeConnections) { - auto dmn = tip_mn_list.GetValidMN(c); - if (!dmn) { + auto dmn_opt = tip_mn_list.GetValidMN(c); + if (!dmn_opt.has_value()) { debugMsg += strprintf(" %s (not in valid MN set anymore)\n", c.ToString()); } else { - debugMsg += strprintf(" %s (%s)\n", c.ToString(), dmn->pdmnState->addr.ToStringAddrPort()); + debugMsg += strprintf(" %s (%s)\n", c.ToString(), dmn_opt.value()->pdmnState->addr.ToStringAddrPort()); } } LogPrint(BCLog::NET_NETCONN, debugMsg.c_str()); /* Continued */ diff --git a/src/masternode/node.cpp b/src/masternode/node.cpp index 0a0b51deef7d8..d5ab28526a82b 100644 --- a/src/masternode/node.cpp +++ b/src/masternode/node.cpp @@ -129,12 +129,13 @@ void CActiveMasternodeManager::InitInternal(const CBlockIndex* pindex) CDeterministicMNList mnList = Assert(m_dmnman)->GetListForBlock(pindex); - auto dmn = mnList.GetMNByOperatorKey(m_info.blsPubKeyOperator); - if (!dmn) { + auto dmn_opt = mnList.GetMNByOperatorKey(m_info.blsPubKeyOperator); + if (!dmn_opt.has_value()) { // MN not appeared on the chain yet return; } + auto dmn = dmn_opt.value(); if (!mnList.IsMNValid(dmn->proTxHash)) { if (mnList.IsMNPoSeBanned(dmn->proTxHash)) { m_state = MasternodeState::POSE_BANNED; @@ -200,11 +201,14 @@ void CActiveMasternodeManager::UpdatedBlockTip(const CBlockIndex* pindexNew, con return reset(MasternodeState::REMOVED); } - auto oldDmn = oldMNList.GetMN(cur_protx_hash); - auto newDmn = newMNList.GetMN(cur_protx_hash); - if (!oldDmn || !newDmn) { + auto oldDmnOpt = oldMNList.GetMN(cur_protx_hash); + auto newDmnOpt = newMNList.GetMN(cur_protx_hash); + if (!oldDmnOpt.has_value() || !newDmnOpt.has_value()) { return reset(MasternodeState::SOME_ERROR); } + + auto oldDmn = oldDmnOpt.value(); + auto newDmn = newDmnOpt.value(); if (newDmn->pdmnState->pubKeyOperator != oldDmn->pdmnState->pubKeyOperator) { // MN operator key changed or revoked return reset(MasternodeState::OPERATOR_KEY_CHANGED); diff --git a/src/masternode/payments.cpp b/src/masternode/payments.cpp index b90dc41936168..99767d96e6d47 100644 --- a/src/masternode/payments.cpp +++ b/src/masternode/payments.cpp @@ -51,18 +51,20 @@ CAmount PlatformShare(const CAmount reward) LogPrint(BCLog::MNPAYMENTS, "CMNPaymentsProcessor::%s -- MN reward %lld reallocated to credit pool\n", __func__, platformReward); voutMasternodePaymentsRet.emplace_back(platformReward, CScript() << OP_RETURN); } + const auto mnList = m_dmnman.GetListForBlock(pindexPrev); if (mnList.GetAllMNsCount() == 0) { LogPrint(BCLog::MNPAYMENTS, "CMNPaymentsProcessor::%s -- no masternode registered to receive a payment\n", __func__); return true; } - const auto dmnPayee = mnList.GetMNPayee(pindexPrev); - if (!dmnPayee) { + + auto dmnPayeeOpt = m_dmnman.GetListForBlock(pindexPrev).GetMNPayee(pindexPrev); + if (!dmnPayeeOpt.has_value()) { return false; } CAmount operatorReward = 0; - + auto dmnPayee = dmnPayeeOpt.value(); if (dmnPayee->nOperatorReward != 0 && dmnPayee->pdmnState->scriptOperatorPayout != CScript()) { // This calculation might eventually turn out to result in 0 even if an operator reward percentage is given. // This will however only happen in a few years when the block rewards drops very low. diff --git a/src/net.cpp b/src/net.cpp index 45ebdafbf98f7..0e466dd4ae8f5 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3503,8 +3503,8 @@ void CConnman::ThreadOpenConnections(const std::vector connect, CDe std::tie(addr, addr_last_try) = addrman.Select(false, preferred_net); } - auto dmn = mnList.GetMNByService(addr); - bool isMasternode = dmn != nullptr; + auto dmn_opt = mnList.GetMNByService(addr); + bool isMasternode = dmn_opt.has_value(); // Require outbound IPv4/IPv6 connections, other than feelers, to be to distinct network groups if (!fFeeler && outbound_ipv46_peer_netgroups.count(m_netgroupman.GetGroup(addr))) { @@ -3516,7 +3516,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect, CDe break; // don't try to connect to masternodes that we already have a connection to (most likely inbound) - if (isMasternode && setConnectedMasternodes.count(dmn->proTxHash)) + if (isMasternode && setConnectedMasternodes.count(dmn_opt.value()->proTxHash)) break; // if we selected a local address, restart (local addresses are allowed in regtest and devnet) @@ -3745,10 +3745,12 @@ void CConnman::ThreadOpenMasternodeConnections(CDeterministicMNManager& dmnman, std::vector ret; for (const auto& group : masternodeQuorumNodes) { for (const auto& proRegTxHash : group.second) { - auto dmn = mnList.GetMN(proRegTxHash); - if (!dmn) { + auto dmn_opt = mnList.GetMN(proRegTxHash); + if (!dmn_opt.has_value()) { continue; } + + auto dmn = dmn_opt.value(); const auto& addr2 = dmn->pdmnState->addr; if (connectedNodes.count(addr2) && !connectedProRegTxHashes.count(proRegTxHash)) { // we probably connected to it before it became a masternode @@ -3783,11 +3785,13 @@ void CConnman::ThreadOpenMasternodeConnections(CDeterministicMNManager& dmnman, AssertLockHeld(cs_vPendingMasternodes); std::vector ret; for (auto it = masternodePendingProbes.begin(); it != masternodePendingProbes.end(); ) { - auto dmn = mnList.GetMN(*it); - if (!dmn) { + auto dmn_opt = mnList.GetMN(*it); + if (!dmn_opt.has_value()) { it = masternodePendingProbes.erase(it); continue; } + + auto dmn = dmn_opt.value(); bool connectedAndOutbound = connectedProRegTxHashes.count(dmn->proTxHash) && !connectedProRegTxHashes[dmn->proTxHash]; if (connectedAndOutbound) { // we already have an outbound connection to this MN so there is no theed to probe it again @@ -3813,11 +3817,13 @@ void CConnman::ThreadOpenMasternodeConnections(CDeterministicMNManager& dmnman, LOCK2(m_nodes_mutex, cs_vPendingMasternodes); if (!vPendingMasternodes.empty()) { - auto dmn = mnList.GetValidMN(vPendingMasternodes.front()); + auto dmn_opt = mnList.GetValidMN(vPendingMasternodes.front()); vPendingMasternodes.erase(vPendingMasternodes.begin()); - if (dmn && !connectedNodes.count(dmn->pdmnState->addr) && !IsMasternodeOrDisconnectRequested(dmn->pdmnState->addr)) { - LogPrint(BCLog::NET_NETCONN, "CConnman::%s -- opening pending masternode connection to %s, service=%s\n", _func_, dmn->proTxHash.ToString(), dmn->pdmnState->addr.ToStringAddrPort()); - return dmn; + if (dmn_opt.has_value()) { + if (auto dmn = dmn_opt.value(); !connectedNodes.count(dmn->pdmnState->addr) && !IsMasternodeOrDisconnectRequested(dmn->pdmnState->addr)) { + LogPrint(BCLog::NET_NETCONN, "CConnman::%s -- opening pending masternode connection to %s, service=%s\n", _func_, dmn->proTxHash.ToString(), dmn->pdmnState->addr.ToStringAddrPort()); + return dmn; + } } } @@ -4706,12 +4712,12 @@ bool CConnman::IsMasternodeQuorumNode(const CNode* pnode, const CDeterministicMN // We however only need to know this if the node did not authenticate itself as a MN yet uint256 assumedProTxHash; if (pnode->GetVerifiedProRegTxHash().IsNull() && !pnode->IsInboundConn()) { - auto dmn = tip_mn_list.GetMNByService(pnode->addr); - if (dmn == nullptr) { + auto dmn_opt = tip_mn_list.GetMNByService(pnode->addr); + if (!dmn_opt.has_value()) { // This is definitely not a masternode return false; } - assumedProTxHash = dmn->proTxHash; + assumedProTxHash = dmn_opt.value()->proTxHash; } LOCK(cs_vPendingMasternodes); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 0fc191983e28c..559ca41cda911 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3349,7 +3349,7 @@ std::pair static ValidateDSTX(CDeterministicMN } const CBlockIndex* pindex{nullptr}; - CDeterministicMNCPtr dmn{nullptr}; + std::optional dmn_opt{std::nullopt}; { LOCK(cs_main); pindex = chainman.ActiveChain().Tip(); @@ -3358,29 +3358,30 @@ std::pair static ValidateDSTX(CDeterministicMN // Try to find a MN up to 24 blocks deep to make sure such dstx-es are relayed and processed correctly. if (dstx.masternodeOutpoint.IsNull()) { for (int i = 0; i < 24 && pindex; ++i) { - dmn = dmnman.GetListForBlock(pindex).GetMN(dstx.m_protxHash); - if (dmn) { - dstx.masternodeOutpoint = dmn->collateralOutpoint; + dmn_opt = dmnman.GetListForBlock(pindex).GetMN(dstx.m_protxHash); + if (dmn_opt.has_value()) { + dstx.masternodeOutpoint = dmn_opt.value()->collateralOutpoint; break; } pindex = pindex->pprev; } } else { for (int i = 0; i < 24 && pindex; ++i) { - dmn = dmnman.GetListForBlock(pindex).GetMNByCollateral(dstx.masternodeOutpoint); - if (dmn) { - dstx.m_protxHash = dmn->proTxHash; + dmn_opt = dmnman.GetListForBlock(pindex).GetMNByCollateral(dstx.masternodeOutpoint); + if (dmn_opt.has_value()) { + dstx.m_protxHash = dmn_opt.value()->proTxHash; break; } pindex = pindex->pprev; } } - if (!dmn) { + if (!dmn_opt.has_value()) { LogPrint(BCLog::COINJOIN, "DSTX -- Can't find masternode %s to verify %s\n", dstx.masternodeOutpoint.ToStringShort(), hashTx.ToString()); return {false, true}; } + auto dmn = dmn_opt.value(); if (!mn_metaman.GetMetaInfo(dmn->proTxHash)->IsValidForMixingTxes()) { LogPrint(BCLog::COINJOIN, "DSTX -- Masternode %s is sending too many transactions %s\n", dstx.masternodeOutpoint.ToStringShort(), hashTx.ToString()); return {true, true}; diff --git a/src/qt/masternodelist.cpp b/src/qt/masternodelist.cpp index 7e3eabe4107d6..ecd5e5bdcde5c 100644 --- a/src/qt/masternodelist.cpp +++ b/src/qt/masternodelist.cpp @@ -355,7 +355,7 @@ CDeterministicMNCPtr MasternodeList::GetSelectedDIP3MN() proTxHash.SetHex(strProTxHash); // Caller is responsible for nullptr checking return value - return clientModel->getMasternodeList().first.GetMN(proTxHash); + return clientModel->getMasternodeList().first.GetMN(proTxHash).value_or(nullptr); } void MasternodeList::extraInfoDIP3_clicked() diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp index a69aee89099ce..2d0ccf550d0ce 100644 --- a/src/qt/rpcconsole.cpp +++ b/src/qt/rpcconsole.cpp @@ -1321,8 +1321,8 @@ void RPCConsole::updateDetailWidget() ui->peerPermissions->setText(permissions.join(" & ")); } ui->peerMappedAS->setText(stats->nodeStats.m_mapped_as != 0 ? QString::number(stats->nodeStats.m_mapped_as) : ts.na); - auto dmn = clientModel->getMasternodeList().first.GetMNByService(stats->nodeStats.addr); - if (dmn == nullptr) { + auto dmn_opt = clientModel->getMasternodeList().first.GetMNByService(stats->nodeStats.addr); + if (!dmn_opt.has_value()) { ui->peerNodeType->setText(tr("Regular")); ui->peerPoSeScore->setText(ts.na); } else { @@ -1331,7 +1331,7 @@ void RPCConsole::updateDetailWidget() } else { ui->peerNodeType->setText(tr("Verified Masternode")); } - ui->peerPoSeScore->setText(QString::number(dmn->pdmnState->nPoSePenalty)); + ui->peerPoSeScore->setText(QString::number(dmn_opt.value()->pdmnState->nPoSePenalty)); } // This check fails for example if the lock was busy and diff --git a/src/rpc/evo.cpp b/src/rpc/evo.cpp index a7d040a059baf..ed74352cd2a9b 100644 --- a/src/rpc/evo.cpp +++ b/src/rpc/evo.cpp @@ -1003,10 +1003,12 @@ static UniValue protx_update_service_common_wrapper(const JSONRPCRequest& reques paramIdx += 3; } - auto dmn = dmnman.GetListAtChainTip().GetMN(ptx.proTxHash); - if (!dmn) { + auto dmn_opt = dmnman.GetListAtChainTip().GetMN(ptx.proTxHash); + if (!dmn_opt.has_value()) { throw std::runtime_error(strprintf("masternode with proTxHash %s not found", ptx.proTxHash.ToString())); } + + auto dmn = dmn_opt.value(); if (dmn->nType != mnType) { throw std::runtime_error(strprintf("masternode with proTxHash %s is not a %s", ptx.proTxHash.ToString(), GetMnType(mnType).description)); } @@ -1100,10 +1102,12 @@ static RPCHelpMan protx_update_registrar_wrapper(bool specific_legacy_bls_scheme CProUpRegTx ptx; ptx.proTxHash = ParseHashV(request.params[0], "proTxHash"); - auto dmn = dmnman.GetListAtChainTip().GetMN(ptx.proTxHash); - if (!dmn) { + auto dmn_opt = dmnman.GetListAtChainTip().GetMN(ptx.proTxHash); + if (!dmn_opt.has_value()) { throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("masternode %s not found", ptx.proTxHash.ToString())); } + + auto dmn = dmn_opt.value(); ptx.keyIDVoting = dmn->pdmnState->keyIDVoting; ptx.scriptPayout = dmn->pdmnState->scriptPayout; @@ -1225,11 +1229,12 @@ static RPCHelpMan protx_revoke() ptx.nReason = (uint16_t)nReason; } - auto dmn = dmnman.GetListAtChainTip().GetMN(ptx.proTxHash); - if (!dmn) { + auto dmn_opt = dmnman.GetListAtChainTip().GetMN(ptx.proTxHash); + if (!dmn_opt.has_value()) { throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("masternode %s not found", ptx.proTxHash.ToString())); } + auto dmn = dmn_opt.value(); if (keyOperator.GetPublicKey() != dmn->pdmnState->pubKeyOperator.Get()) { throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("the operator key does not belong to the registered public key")); } @@ -1505,11 +1510,11 @@ static RPCHelpMan protx_info() } auto mnList = dmnman.GetListForBlock(pindex); - auto dmn = mnList.GetMN(proTxHash); - if (!dmn) { + auto dmn_opt = mnList.GetMN(proTxHash); + if (!dmn_opt.has_value()) { throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s not found", proTxHash.ToString())); } - return BuildDMNListEntry(wallet.get(), *dmn, mn_metaman, true, chainman, pindex); + return BuildDMNListEntry(wallet.get(), *dmn_opt.value(), mn_metaman, true, chainman, pindex); }, }; } @@ -1634,20 +1639,21 @@ static RPCHelpMan protx_listdiff() UniValue jremovedMNs(UniValue::VARR); for(const auto& internal_id : mnDiff.removedMns) { - auto dmn = baseBlockMNList.GetMNByInternalId(internal_id); + auto dmn_opt = baseBlockMNList.GetMNByInternalId(internal_id); // BuildDiff will construct itself with MNs that we already have knowledge // of, meaning that fetch operations should never fail. - CHECK_NONFATAL(dmn); - jremovedMNs.push_back(dmn->proTxHash.ToString()); + CHECK_NONFATAL(dmn_opt.has_value()); + jremovedMNs.push_back(dmn_opt.value()->proTxHash.ToString()); } ret.pushKV("removedMNs", jremovedMNs); UniValue jupdatedMNs(UniValue::VARR); for(const auto& [internal_id, stateDiff] : mnDiff.updatedMNs) { - auto dmn = baseBlockMNList.GetMNByInternalId(internal_id); + auto dmn_opt = baseBlockMNList.GetMNByInternalId(internal_id); // BuildDiff will construct itself with MNs that we already have knowledge // of, meaning that fetch operations should never fail. - CHECK_NONFATAL(dmn); + CHECK_NONFATAL(dmn_opt.has_value()); + auto dmn = dmn_opt.value(); UniValue obj(UniValue::VOBJ); obj.pushKV(dmn->proTxHash.ToString(), stateDiff.ToJson(dmn->nType)); jupdatedMNs.push_back(obj); diff --git a/src/rpc/governance.cpp b/src/rpc/governance.cpp index 5123945c3f323..48da2260dd420 100644 --- a/src/rpc/governance.cpp +++ b/src/rpc/governance.cpp @@ -458,8 +458,8 @@ static UniValue VoteWithMasternodes(const JSONRPCRequest& request, const CWallet UniValue statusObj(UniValue::VOBJ); - auto dmn = mnList.GetValidMN(proTxHash); - if (!dmn) { + auto dmn_opt = mnList.GetValidMN(proTxHash); + if (!dmn_opt.has_value()) { nFailed++; statusObj.pushKV("result", "failed"); statusObj.pushKV("errorMessage", "Can't find masternode by proTxHash"); @@ -467,6 +467,7 @@ static UniValue VoteWithMasternodes(const JSONRPCRequest& request, const CWallet continue; } + auto dmn = dmn_opt.value(); CGovernanceVote vote(dmn->collateralOutpoint, hash, eVoteSignal, eVoteOutcome); if (!SignVote(wallet, keyID, vote) || !vote.CheckSignature(keyID)) { @@ -598,12 +599,12 @@ static RPCHelpMan gobject_vote_alias() EnsureWalletIsUnlocked(*wallet); uint256 proTxHash(ParseHashV(request.params[3], "protx-hash")); - auto dmn = CHECK_NONFATAL(node.dmnman)->GetListAtChainTip().GetValidMN(proTxHash); - if (!dmn) { + auto dmn_opt = CHECK_NONFATAL(node.dmnman)->GetListAtChainTip().GetValidMN(proTxHash); + if (!dmn_opt.has_value()) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid or unknown proTxHash"); } - + auto dmn = dmn_opt.value(); const bool is_mine = CheckWalletOwnsKey(*wallet, dmn->pdmnState->keyIDVoting); if (!is_mine) { throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Private key for voting address %s not known by wallet", EncodeDestination(PKHash(dmn->pdmnState->keyIDVoting)))); diff --git a/src/rpc/masternode.cpp b/src/rpc/masternode.cpp index b18f7ef06d2fa..14644e8391109 100644 --- a/src/rpc/masternode.cpp +++ b/src/rpc/masternode.cpp @@ -229,8 +229,9 @@ static RPCHelpMan masternode_status() // keep compatibility with legacy status for now (might get deprecated/removed later) mnObj.pushKV("outpoint", node.mn_activeman->GetOutPoint().ToStringShort()); mnObj.pushKV("service", node.mn_activeman->GetService().ToStringAddrPort()); - auto dmn = CHECK_NONFATAL(node.dmnman)->GetListAtChainTip().GetMN(node.mn_activeman->GetProTxHash()); - if (dmn) { + auto dmn_opt = CHECK_NONFATAL(node.dmnman)->GetListAtChainTip().GetMN(node.mn_activeman->GetProTxHash()); + if (dmn_opt.has_value()) { + auto dmn = dmn_opt.value(); mnObj.pushKV("proTxHash", dmn->proTxHash.ToString()); mnObj.pushKV("type", std::string(GetMnType(dmn->nType).description)); mnObj.pushKV("collateralHash", dmn->collateralOutpoint.hash.ToString()); @@ -321,9 +322,9 @@ static RPCHelpMan masternode_winners() const auto tip_mn_list = CHECK_NONFATAL(node.dmnman)->GetListAtChainTip(); for (int h = nStartHeight; h <= nChainTipHeight; h++) { const CBlockIndex* pIndex = pindexTip->GetAncestor(h - 1); - auto payee = node.dmnman->GetListForBlock(pIndex).GetMNPayee(pIndex); - if (payee) { - std::string strPayments = GetRequiredPaymentsString(*CHECK_NONFATAL(node.govman), tip_mn_list, h, payee); + auto payee_opt = node.dmnman->GetListForBlock(pIndex).GetMNPayee(pIndex); + if (payee_opt.has_value()) { + std::string strPayments = GetRequiredPaymentsString(*CHECK_NONFATAL(node.govman), tip_mn_list, h, payee_opt.value()); if (strFilter != "" && strPayments.find(strFilter) == std::string::npos) continue; obj.pushKV(strprintf("%d", h), strPayments); } @@ -458,8 +459,8 @@ static RPCHelpMan masternode_payments() } // NOTE: we use _previous_ block to find a payee for the current one - const auto dmnPayee = node.dmnman->GetListForBlock(pindex->pprev).GetMNPayee(pindex->pprev); - protxObj.pushKV("proTxHash", dmnPayee == nullptr ? "" : dmnPayee->proTxHash.ToString()); + auto dmnPayeeOpt = node.dmnman->GetListForBlock(pindex->pprev).GetMNPayee(pindex->pprev); + protxObj.pushKV("proTxHash", !dmnPayeeOpt.has_value() ? "" : dmnPayeeOpt.value()->proTxHash.ToString()); protxObj.pushKV("amount", payedPerMasternode); protxObj.pushKV("payees", payeesArr); payedPerBlock += payedPerMasternode; diff --git a/src/rpc/quorums.cpp b/src/rpc/quorums.cpp index c685118c5e6fc..977be7bc5c3a4 100644 --- a/src/rpc/quorums.cpp +++ b/src/rpc/quorums.cpp @@ -397,11 +397,12 @@ static RPCHelpMan quorum_memberof() const CBlockIndex* pindexTip = WITH_LOCK(cs_main, return chainman.ActiveChain().Tip()); auto mnList = CHECK_NONFATAL(node.dmnman)->GetListForBlock(pindexTip); - auto dmn = mnList.GetMN(protxHash); - if (!dmn) { + auto dmn_opt = mnList.GetMN(protxHash); + if (!dmn_opt.has_value()) { throw JSONRPCError(RPC_INVALID_PARAMETER, "masternode not found"); } + auto dmn = dmn_opt.value(); UniValue result(UniValue::VARR); for (const auto& type : llmq::GetEnabledQuorumTypes(pindexTip)) { const auto llmq_params_opt = Params().GetLLMQ(type); diff --git a/src/test/evo_deterministicmns_tests.cpp b/src/test/evo_deterministicmns_tests.cpp index ae82700ad2da6..68f184204e53c 100644 --- a/src/test/evo_deterministicmns_tests.cpp +++ b/src/test/evo_deterministicmns_tests.cpp @@ -466,8 +466,8 @@ void FuncDIP3Protx(TestChainSetup& setup) // check MN reward payments for (size_t i = 0; i < 20; i++) { - auto dmnExpectedPayee = dmnman.GetListAtChainTip().GetMNPayee(chainman.ActiveChain().Tip()); - BOOST_ASSERT(dmnExpectedPayee); + auto dmnExpectedPayeeOpt = dmnman.GetListAtChainTip().GetMNPayee(chainman.ActiveChain().Tip()); + BOOST_ASSERT(dmnExpectedPayeeOpt.has_value()); CBlock block = setup.CreateAndProcessBlock({}, setup.coinbaseKey); dmnman.UpdatedBlockTip(chainman.ActiveChain().Tip()); @@ -475,7 +475,7 @@ void FuncDIP3Protx(TestChainSetup& setup) auto dmnPayout = FindPayoutDmn(dmnman, block); BOOST_REQUIRE(dmnPayout != nullptr); - BOOST_CHECK_EQUAL(dmnPayout->proTxHash.ToString(), dmnExpectedPayee->proTxHash.ToString()); + BOOST_CHECK_EQUAL(dmnPayout->proTxHash.ToString(), dmnExpectedPayeeOpt.value()->proTxHash.ToString()); nHeight++; } @@ -510,8 +510,8 @@ void FuncDIP3Protx(TestChainSetup& setup) BOOST_CHECK_EQUAL(chainman.ActiveChain().Height(), nHeight + 1); nHeight++; - auto dmn = dmnman.GetListAtChainTip().GetMN(dmnHashes[0]); - BOOST_REQUIRE(dmn != nullptr && dmn->pdmnState->addr.GetPort() == 1000); + auto dmn_opt = dmnman.GetListAtChainTip().GetMN(dmnHashes[0]); + BOOST_REQUIRE(dmn_opt.has_value() && dmn_opt.value()->pdmnState->addr.GetPort() == 1000); // test ProUpRevTx tx = CreateProUpRevTx(chainman.ActiveChain(), *(setup.m_node.mempool), utxos, dmnHashes[0], operatorKeys[dmnHashes[0]], setup.coinbaseKey); @@ -520,13 +520,13 @@ void FuncDIP3Protx(TestChainSetup& setup) BOOST_CHECK_EQUAL(chainman.ActiveChain().Height(), nHeight + 1); nHeight++; - dmn = dmnman.GetListAtChainTip().GetMN(dmnHashes[0]); - BOOST_REQUIRE(dmn != nullptr && dmn->pdmnState->GetBannedHeight() == nHeight); + dmn_opt = dmnman.GetListAtChainTip().GetMN(dmnHashes[0]); + BOOST_REQUIRE(dmn_opt.has_value() && dmn_opt.value()->pdmnState->GetBannedHeight() == nHeight); // test that the revoked MN does not get paid anymore for (size_t i = 0; i < 20; i++) { - auto dmnExpectedPayee = dmnman.GetListAtChainTip().GetMNPayee(chainman.ActiveChain().Tip()); - BOOST_REQUIRE(dmnExpectedPayee && dmnExpectedPayee->proTxHash != dmnHashes[0]); + auto dmnExpectedPayeeOpt = dmnman.GetListAtChainTip().GetMNPayee(chainman.ActiveChain().Tip()); + BOOST_REQUIRE(dmnExpectedPayeeOpt.has_value() && dmnExpectedPayeeOpt.value()->proTxHash != dmnHashes[0]); CBlock block = setup.CreateAndProcessBlock({}, setup.coinbaseKey); dmnman.UpdatedBlockTip(chainman.ActiveChain().Tip()); @@ -534,7 +534,7 @@ void FuncDIP3Protx(TestChainSetup& setup) auto dmnPayout = FindPayoutDmn(dmnman, block); BOOST_REQUIRE(dmnPayout != nullptr); - BOOST_CHECK_EQUAL(dmnPayout->proTxHash.ToString(), dmnExpectedPayee->proTxHash.ToString()); + BOOST_CHECK_EQUAL(dmnPayout->proTxHash.ToString(), dmnExpectedPayeeOpt.value()->proTxHash.ToString()); nHeight++; } @@ -542,8 +542,8 @@ void FuncDIP3Protx(TestChainSetup& setup) // test reviving the MN CBLSSecretKey newOperatorKey; newOperatorKey.MakeNewKey(); - dmn = dmnman.GetListAtChainTip().GetMN(dmnHashes[0]); - tx = CreateProUpRegTx(chainman.ActiveChain(), *(setup.m_node.mempool), utxos, dmnHashes[0], ownerKeys[dmnHashes[0]], newOperatorKey.GetPublicKey(), ownerKeys[dmnHashes[0]].GetPubKey().GetID(), dmn->pdmnState->scriptPayout, setup.coinbaseKey); + dmn_opt = dmnman.GetListAtChainTip().GetMN(dmnHashes[0]); + tx = CreateProUpRegTx(chainman.ActiveChain(), *(setup.m_node.mempool), utxos, dmnHashes[0], ownerKeys[dmnHashes[0]], newOperatorKey.GetPublicKey(), ownerKeys[dmnHashes[0]].GetPubKey().GetID(), dmn_opt.value()->pdmnState->scriptPayout, setup.coinbaseKey); // check malleability protection again, but this time by also relying on the signature inside the ProUpRegTx auto tx2 = MalleateProTxPayout(tx); TxValidationState dummy_state; @@ -568,16 +568,16 @@ void FuncDIP3Protx(TestChainSetup& setup) BOOST_CHECK_EQUAL(chainman.ActiveChain().Height(), nHeight + 1); nHeight++; - dmn = dmnman.GetListAtChainTip().GetMN(dmnHashes[0]); - BOOST_REQUIRE(dmn != nullptr && dmn->pdmnState->addr.GetPort() == 100); - BOOST_REQUIRE(dmn != nullptr && !dmn->pdmnState->IsBanned()); + dmn_opt = dmnman.GetListAtChainTip().GetMN(dmnHashes[0]); + BOOST_REQUIRE(dmn_opt.has_value() && dmn_opt.value()->pdmnState->addr.GetPort() == 100); + BOOST_REQUIRE(dmn_opt.has_value() && !dmn_opt.value()->pdmnState->IsBanned()); // test that the revived MN gets payments again bool foundRevived = false; for (size_t i = 0; i < 20; i++) { - auto dmnExpectedPayee = dmnman.GetListAtChainTip().GetMNPayee(chainman.ActiveChain().Tip()); - BOOST_ASSERT(dmnExpectedPayee); - if (dmnExpectedPayee->proTxHash == dmnHashes[0]) { + auto dmnExpectedPayeeOpt = dmnman.GetListAtChainTip().GetMNPayee(chainman.ActiveChain().Tip()); + BOOST_ASSERT(dmnExpectedPayeeOpt.has_value()); + if (dmnExpectedPayeeOpt.value()->proTxHash == dmnHashes[0]) { foundRevived = true; } @@ -587,7 +587,7 @@ void FuncDIP3Protx(TestChainSetup& setup) auto dmnPayout = FindPayoutDmn(dmnman, block); BOOST_REQUIRE(dmnPayout != nullptr); - BOOST_CHECK_EQUAL(dmnPayout->proTxHash.ToString(), dmnExpectedPayee->proTxHash.ToString()); + BOOST_CHECK_EQUAL(dmnPayout->proTxHash.ToString(), dmnExpectedPayeeOpt.value()->proTxHash.ToString()); nHeight++; } diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 291599aea201e..d5a46a504fdb5 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -702,7 +702,9 @@ void CTxMemPool::addUncheckedProTx(indexed_transaction_set::iterator& newit, con auto proTx = *Assert(GetTxPayload(tx)); mapProTxRefs.emplace(proTx.proTxHash, tx.GetHash()); mapProTxBlsPubKeyHashes.emplace(proTx.pubKeyOperator.GetHash(), tx.GetHash()); - auto dmn = Assert(m_dmnman->GetListAtChainTip().GetMN(proTx.proTxHash)); + // addUncheckedProTx is called after AcceptToMemoryPool() has run appropriate checks. So + // we should assume that the contents of this transaction are valid. See comment in addUnchecked. + auto dmn = Assert(*(m_dmnman->GetListAtChainTip().GetMN(proTx.proTxHash))); newit->validForProTxKey = ::SerializeHash(dmn->pdmnState->pubKeyOperator); if (dmn->pdmnState->pubKeyOperator != proTx.pubKeyOperator) { newit->isKeyChangeProTx = true; @@ -710,7 +712,9 @@ void CTxMemPool::addUncheckedProTx(indexed_transaction_set::iterator& newit, con } else if (tx.nType == TRANSACTION_PROVIDER_UPDATE_REVOKE) { auto proTx = *Assert(GetTxPayload(tx)); mapProTxRefs.emplace(proTx.proTxHash, tx.GetHash()); - auto dmn = Assert(m_dmnman->GetListAtChainTip().GetMN(proTx.proTxHash)); + // addUncheckedProTx is called after AcceptToMemoryPool() has run appropriate checks. So + // we should assume that the contents of this transaction are valid. See comment in addUnchecked. + auto dmn = Assert(*(m_dmnman->GetListAtChainTip().GetMN(proTx.proTxHash))); newit->validForProTxKey = ::SerializeHash(dmn->pdmnState->pubKeyOperator); if (dmn->pdmnState->pubKeyOperator.Get() != CBLSPublicKey()) { newit->isKeyChangeProTx = true; @@ -981,10 +985,10 @@ void CTxMemPool::removeProTxSpentCollateralConflicts(const CTransaction &tx) // These are not yet mined ProRegTxs removeSpentCollateralConflict(collateralIt->second); } - auto dmn = mnList.GetMNByCollateral(in.prevout); - if (dmn) { + auto dmn_opt = mnList.GetMNByCollateral(in.prevout); + if (dmn_opt.has_value()) { // These are updates referring to a mined ProRegTx - removeSpentCollateralConflict(dmn->proTxHash); + removeSpentCollateralConflict(dmn_opt.value()->proTxHash); } } } @@ -1404,13 +1408,13 @@ bool CTxMemPool::existsProviderTxConflict(const CTransaction &tx) const { auto& proTx = *opt_proTx; // this method should only be called with validated ProTxs - auto dmn = m_dmnman->GetListAtChainTip().GetMN(proTx.proTxHash); - if (!dmn) { + auto dmn_opt = m_dmnman->GetListAtChainTip().GetMN(proTx.proTxHash); + if (!dmn_opt) { LogPrint(BCLog::MEMPOOL, "%s: ERROR: Masternode is not in the list, proTxHash: %s\n", __func__, proTx.proTxHash.ToString()); return true; // i.e. failed to find validated ProTx == conflict } // only allow one operator key change in the mempool - if (dmn->pdmnState->pubKeyOperator != proTx.pubKeyOperator) { + if (dmn_opt.value()->pdmnState->pubKeyOperator != proTx.pubKeyOperator) { if (hasKeyChangeInMempool(proTx.proTxHash)) { return true; } @@ -1426,13 +1430,13 @@ bool CTxMemPool::existsProviderTxConflict(const CTransaction &tx) const { } auto& proTx = *opt_proTx; // this method should only be called with validated ProTxs - auto dmn = m_dmnman->GetListAtChainTip().GetMN(proTx.proTxHash); - if (!dmn) { + auto dmn_opt = m_dmnman->GetListAtChainTip().GetMN(proTx.proTxHash); + if (!dmn_opt.has_value()) { LogPrint(BCLog::MEMPOOL, "%s: ERROR: Masternode is not in the list, proTxHash: %s\n", __func__, proTx.proTxHash.ToString()); return true; // i.e. failed to find validated ProTx == conflict } // only allow one operator key change in the mempool - if (dmn->pdmnState->pubKeyOperator.Get() != CBLSPublicKey()) { + if (dmn_opt.value()->pdmnState->pubKeyOperator.Get() != CBLSPublicKey()) { if (hasKeyChangeInMempool(proTx.proTxHash)) { return true; }