From 5cf97c1b0713ad1598e600c3b57c77eebdbf4048 Mon Sep 17 00:00:00 2001 From: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com> Date: Mon, 29 Nov 2021 00:12:09 -0500 Subject: [PATCH] refactor: Misc LLMQ refactoring (#4590) * use unique_ptr instead of shared Signed-off-by: pasta * unique_ptr over shared_ptr Signed-off-by: pasta * remove unneeded ptr Signed-off-by: pasta * Adjust IsTxSafeForMining checks Signed-off-by: pasta * use const ref Signed-off-by: pasta * add a todo Signed-off-by: pasta * use optional instead of magic max value fixes a hypothetical bug where myIdx is not "initialized" (ie max), and we sleep forever Signed-off-by: pasta * simplify relay check Signed-off-by: pasta * use count_if instead of a loop Signed-off-by: pasta * add a few vector reserves Signed-off-by: pasta --- src/dsnotificationinterface.cpp | 2 +- src/llmq/blockprocessor.cpp | 2 +- src/llmq/blockprocessor.h | 2 +- src/llmq/chainlocks.cpp | 13 ++++++------ src/llmq/chainlocks.h | 2 +- src/llmq/commitment.h | 2 +- src/llmq/dkgsession.cpp | 36 ++++++++++++--------------------- src/llmq/dkgsession.h | 7 ++++--- src/llmq/dkgsessionhandler.cpp | 6 +++--- src/llmq/dkgsessionhandler.h | 2 +- src/llmq/quorums.cpp | 10 ++++----- src/llmq/quorums.h | 4 ++-- 12 files changed, 40 insertions(+), 48 deletions(-) diff --git a/src/dsnotificationinterface.cpp b/src/dsnotificationinterface.cpp index c95cac7642..356cba3104 100644 --- a/src/dsnotificationinterface.cpp +++ b/src/dsnotificationinterface.cpp @@ -67,7 +67,7 @@ void CDSNotificationInterface::UpdatedBlockTip(const CBlockIndex *pindexNew, con #endif // ENABLE_WALLET llmq::quorumInstantSendManager->UpdatedBlockTip(pindexNew); - llmq::chainLocksHandler->UpdatedBlockTip(pindexNew); + llmq::chainLocksHandler->UpdatedBlockTip(); llmq::quorumManager->UpdatedBlockTip(pindexNew, fInitialDownload); llmq::quorumDKGSessionManager->UpdatedBlockTip(pindexNew, fInitialDownload); diff --git a/src/llmq/blockprocessor.cpp b/src/llmq/blockprocessor.cpp index 2dc4aca3d8..c1831cec55 100644 --- a/src/llmq/blockprocessor.cpp +++ b/src/llmq/blockprocessor.cpp @@ -431,7 +431,7 @@ CFinalCommitmentPtr CQuorumBlockProcessor::GetMinedCommitment(Consensus::LLMQTyp return nullptr; } retMinedBlockHash = p.second; - return std::make_shared(p.first); + return std::make_unique(p.first); } // The returned quorums are in reversed order, so the most recent one is at index 0 diff --git a/src/llmq/blockprocessor.h b/src/llmq/blockprocessor.h index f6ba5409aa..92c9036978 100644 --- a/src/llmq/blockprocessor.h +++ b/src/llmq/blockprocessor.h @@ -27,7 +27,7 @@ namespace llmq { class CFinalCommitment; -using CFinalCommitmentPtr = std::shared_ptr; +using CFinalCommitmentPtr = std::unique_ptr; class CQuorumBlockProcessor { diff --git a/src/llmq/chainlocks.cpp b/src/llmq/chainlocks.cpp index 5eef212bb5..e59d6dd9df 100644 --- a/src/llmq/chainlocks.cpp +++ b/src/llmq/chainlocks.cpp @@ -202,7 +202,7 @@ void CChainLocksHandler::AcceptedBlockHeader(const CBlockIndex* pindexNew) } } -void CChainLocksHandler::UpdatedBlockTip(const CBlockIndex* pindexNew) +void CChainLocksHandler::UpdatedBlockTip() { // don't call TrySignChainTip directly but instead let the scheduler call it. This way we ensure that cs_main is // never locked and TrySignChainTip is not called twice in parallel. Also avoids recursive calls due to @@ -460,10 +460,14 @@ bool CChainLocksHandler::IsTxSafeForMining(const uint256& txid) const if (!RejectConflictingBlocks()) { return true; } + if (!isEnabled || !isEnforced) { + return true; + } + if (!IsInstantSendEnabled()) { return true; } - if (!isEnabled || !isEnforced) { + if (quorumInstantSendManager->IsLocked(txid)) { return true; } @@ -476,10 +480,7 @@ bool CChainLocksHandler::IsTxSafeForMining(const uint256& txid) const } } - if (txAge < WAIT_FOR_ISLOCK_TIMEOUT && !quorumInstantSendManager->IsLocked(txid)) { - return false; - } - return true; + return txAge >= WAIT_FOR_ISLOCK_TIMEOUT; } // WARNING: cs_main and cs should not be held! diff --git a/src/llmq/chainlocks.h b/src/llmq/chainlocks.h index d44b9da341..1891e613fd 100644 --- a/src/llmq/chainlocks.h +++ b/src/llmq/chainlocks.h @@ -97,7 +97,7 @@ public: void ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStream& vRecv); void ProcessNewChainLock(NodeId from, const CChainLockSig& clsig, const uint256& hash); void AcceptedBlockHeader(const CBlockIndex* pindexNew); - void UpdatedBlockTip(const CBlockIndex* pindexNew); + void UpdatedBlockTip(); void TransactionAddedToMempool(const CTransactionRef& tx, int64_t nAcceptTime); void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindex, const std::vector& vtxConflicted); void BlockDisconnected(const std::shared_ptr& pblock, const CBlockIndex* pindexDisconnected); diff --git a/src/llmq/commitment.h b/src/llmq/commitment.h index 749e40051b..5d14bdd81d 100644 --- a/src/llmq/commitment.h +++ b/src/llmq/commitment.h @@ -101,7 +101,7 @@ public: obj.pushKV("membersSig", membersSig.ToString()); } }; -using CFinalCommitmentPtr = std::shared_ptr; +using CFinalCommitmentPtr = std::unique_ptr; class CFinalCommitmentTxPayload { diff --git a/src/llmq/dkgsession.cpp b/src/llmq/dkgsession.cpp index 19c733d747..4b64fee7d7 100644 --- a/src/llmq/dkgsession.cpp +++ b/src/llmq/dkgsession.cpp @@ -82,7 +82,7 @@ CDKGPrematureCommitment::CDKGPrematureCommitment(const Consensus::LLMQParams& pa { } -CDKGMember::CDKGMember(CDeterministicMNCPtr _dmn, size_t _idx) : +CDKGMember::CDKGMember(const CDeterministicMNCPtr& _dmn, size_t _idx) : dmn(_dmn), idx(_idx), id(_dmn->proTxHash) @@ -321,7 +321,7 @@ void CDKGSession::ReceiveMessage(const CDKGContribution& qc, bool& retBan) bool complain = false; CBLSSecretKey skContribution; - if (!qc.contributions->Decrypt(myIdx, WITH_LOCK(activeMasternodeInfoCs, return *activeMasternodeInfo.blsKeyOperator), skContribution, PROTOCOL_VERSION)) { + if (!qc.contributions->Decrypt(*myIdx, WITH_LOCK(activeMasternodeInfoCs, return *activeMasternodeInfo.blsKeyOperator), skContribution, PROTOCOL_VERSION)) { logger.Batch("contribution from %s could not be decrypted", member->dmn->proTxHash.ToString()); complain = true; } else if (member->idx != myIdx && ShouldSimulateError("complain-lie")) { @@ -670,7 +670,7 @@ void CDKGSession::VerifyAndJustify(CDKGPendingMessages& pendingMessages) } const auto& qc = WITH_LOCK(invCs, return std::move(complaints.at(*m->complaints.begin()))); - if (qc.complainForMembers[myIdx]) { + if (qc.complainForMembers[*myIdx]) { justifyFor.emplace(qc.proTxHash); } } @@ -874,18 +874,12 @@ void CDKGSession::ReceiveMessage(const CDKGJustification& qj, bool& retBan) } } - int receivedCount = 0; - int expectedCount = 0; - - for (const auto& m : members) { - if (!m->justifications.empty()) { - receivedCount++; - } - - if (m->someoneComplain) { - expectedCount++; - } - } + auto receivedCount = std::count_if(members.cbegin(), members.cend(), [](const auto& m){ + return !m->justifications.empty(); + }); + auto expectedCount = std::count_if(members.cbegin(), members.cend(), [](const auto& m){ + return m->someoneComplain; + }); logger.Batch("verified justification: received=%d/%d time=%d", receivedCount, expectedCount, t1.count()); } @@ -899,7 +893,9 @@ void CDKGSession::VerifyAndCommit(CDKGPendingMessages& pendingMessages) CDKGLogger logger(*this, __func__); std::vector badMembers; + badMembers.reserve(members.size()); std::vector openComplaintMembers; + openComplaintMembers.reserve(members.size()); for (const auto& m : members) { if (m->bad) { @@ -1319,14 +1315,8 @@ void CDKGSession::MarkBadMember(size_t idx) void CDKGSession::RelayInvToParticipants(const CInv& inv) const { g_connman->ForEachNode([&](CNode* pnode) { - bool relay = false; - auto verifiedProRegTxHash = pnode->GetVerifiedProRegTxHash(); - if (pnode->qwatch) { - relay = true; - } else if (!verifiedProRegTxHash.IsNull() && relayMembers.count(verifiedProRegTxHash)) { - relay = true; - } - if (relay) { + if (pnode->qwatch || + (!pnode->GetVerifiedProRegTxHash().IsNull() && relayMembers.count(pnode->GetVerifiedProRegTxHash()))) { pnode->PushInventory(inv); } }); diff --git a/src/llmq/dkgsession.h b/src/llmq/dkgsession.h index 3fdefecd2b..e708cf7762 100644 --- a/src/llmq/dkgsession.h +++ b/src/llmq/dkgsession.h @@ -122,6 +122,7 @@ public: Consensus::LLMQType llmqType; uint256 quorumHash; uint256 proTxHash; + // TODO make this pair a struct with named fields std::vector> contributions; CBLSSignature sig; @@ -190,7 +191,7 @@ public: class CDKGMember { public: - CDKGMember(CDeterministicMNCPtr _dmn, size_t _idx); + CDKGMember(const CDeterministicMNCPtr& _dmn, size_t _idx); CDeterministicMNCPtr dmn; size_t idx; @@ -255,7 +256,7 @@ private: uint256 myProTxHash; CBLSId myId; - size_t myIdx{(size_t)-1}; + std::optional myIdx; // all indexed by msg hash // we expect to only receive a single vvec and contribution per member, but we must also be able to relay @@ -279,7 +280,7 @@ public: bool Init(const CBlockIndex* pQuorumBaseBlockIndex, const std::vector& mns, const uint256& _myProTxHash); - size_t GetMyMemberIndex() const { return myIdx; } + std::optional GetMyMemberIndex() const { return myIdx; } /** * The following sets of methods are for the first 4 phases handled in the session. The flow of message calls diff --git a/src/llmq/dkgsessionhandler.cpp b/src/llmq/dkgsessionhandler.cpp index e1bf0d06ac..254e9ef620 100644 --- a/src/llmq/dkgsessionhandler.cpp +++ b/src/llmq/dkgsessionhandler.cpp @@ -90,7 +90,7 @@ CDKGSessionHandler::CDKGSessionHandler(const Consensus::LLMQParams& _params, CBL params(_params), blsWorker(_blsWorker), dkgManager(_dkgManager), - curSession(std::make_shared(_params, _blsWorker, _dkgManager)), + curSession(std::make_unique(_params, _blsWorker, _dkgManager)), pendingContributions((size_t)_params.size * 2, MSG_QUORUM_CONTRIB), // we allow size*2 messages as we need to make sure we see bad behavior (double messages) pendingComplaints((size_t)_params.size * 2, MSG_QUORUM_COMPLAINT), pendingJustifications((size_t)_params.size * 2, MSG_QUORUM_JUSTIFICATION), @@ -158,7 +158,7 @@ void CDKGSessionHandler::StopThread() bool CDKGSessionHandler::InitNewQuorum(const CBlockIndex* pQuorumBaseBlockIndex) { - curSession = std::make_shared(params, blsWorker, dkgManager); + curSession = std::make_unique(params, blsWorker, dkgManager); if (!deterministicMNManager->IsDIP3Enforced(pQuorumBaseBlockIndex->nHeight)) { return false; @@ -271,7 +271,7 @@ void CDKGSessionHandler::SleepBeforePhase(QuorumPhase curPhase, // Don't expect perfect block times and thus reduce the phase time to be on the secure side (caller chooses factor) double adjustedPhaseSleepTimePerMember = phaseSleepTimePerMember * randomSleepFactor; - int64_t sleepTime = (int64_t)(adjustedPhaseSleepTimePerMember * curSession->GetMyMemberIndex()); + int64_t sleepTime = (int64_t)(adjustedPhaseSleepTimePerMember * curSession->GetMyMemberIndex().value_or(0)); int64_t endTime = GetTimeMillis() + sleepTime; int heightTmp{-1}; int heightStart{-1}; diff --git a/src/llmq/dkgsessionhandler.h b/src/llmq/dkgsessionhandler.h index 7f60aac653..105cf61c26 100644 --- a/src/llmq/dkgsessionhandler.h +++ b/src/llmq/dkgsessionhandler.h @@ -114,7 +114,7 @@ private: int currentHeight GUARDED_BY(cs) {-1}; uint256 quorumHash GUARDED_BY(cs); - std::shared_ptr curSession; + std::unique_ptr curSession; std::thread phaseHandlerThread; // Do not guard these, they protect their internals themselves diff --git a/src/llmq/quorums.cpp b/src/llmq/quorums.cpp index 797155f883..93251829b0 100644 --- a/src/llmq/quorums.cpp +++ b/src/llmq/quorums.cpp @@ -51,9 +51,9 @@ CQuorum::CQuorum(const Consensus::LLMQParams& _params, CBLSWorker& _blsWorker) : CQuorum::~CQuorum() = default; -void CQuorum::Init(const CFinalCommitmentPtr& _qc, const CBlockIndex* _pQuorumBaseBlockIndex, const uint256& _minedBlockHash, const std::vector& _members) +void CQuorum::Init(CFinalCommitmentPtr _qc, const CBlockIndex* _pQuorumBaseBlockIndex, const uint256& _minedBlockHash, const std::vector& _members) { - qc = _qc; + qc = std::move(_qc); m_quorum_base_block_index = _pQuorumBaseBlockIndex; members = _members; minedBlockHash = _minedBlockHash; @@ -315,17 +315,17 @@ CQuorumPtr CQuorumManager::BuildQuorumFromCommitment(const Consensus::LLMQType l auto quorum = std::make_shared(llmqParams, blsWorker); auto members = CLLMQUtils::GetAllQuorumMembers(llmqParams, pQuorumBaseBlockIndex); - quorum->Init(qc, pQuorumBaseBlockIndex, minedBlockHash, members); + quorum->Init(std::move(qc), pQuorumBaseBlockIndex, minedBlockHash, members); bool hasValidVvec = false; if (quorum->ReadContributions(evoDb)) { hasValidVvec = true; } else { - if (BuildQuorumContributions(qc, quorum)) { + if (BuildQuorumContributions(quorum->qc, quorum)) { quorum->WriteContributions(evoDb); hasValidVvec = true; } else { - LogPrint(BCLog::LLMQ, "CQuorumManager::%s -- quorum.ReadContributions and BuildQuorumContributions for block %s failed\n", __func__, qc->quorumHash.ToString()); + LogPrint(BCLog::LLMQ, "CQuorumManager::%s -- quorum.ReadContributions and BuildQuorumContributions for block %s failed\n", __func__, quorum->qc->quorumHash.ToString()); } } diff --git a/src/llmq/quorums.h b/src/llmq/quorums.h index a6cf1113cf..78ba9617c2 100644 --- a/src/llmq/quorums.h +++ b/src/llmq/quorums.h @@ -144,7 +144,7 @@ using CQuorumPtr = std::shared_ptr; using CQuorumCPtr = std::shared_ptr; class CFinalCommitment; -using CFinalCommitmentPtr = std::shared_ptr; +using CFinalCommitmentPtr = std::unique_ptr; class CQuorum @@ -171,7 +171,7 @@ private: public: CQuorum(const Consensus::LLMQParams& _params, CBLSWorker& _blsWorker); ~CQuorum(); - void Init(const CFinalCommitmentPtr& _qc, const CBlockIndex* _pQuorumBaseBlockIndex, const uint256& _minedBlockHash, const std::vector& _members); + void Init(CFinalCommitmentPtr _qc, const CBlockIndex* _pQuorumBaseBlockIndex, const uint256& _minedBlockHash, const std::vector& _members); bool SetVerificationVector(const BLSVerificationVector& quorumVecIn); bool SetSecretKeyShare(const CBLSSecretKey& secretKeyShare);