compile entire codebase with nrvo warning and fix all

PastaPastaPasta · Dec 17, 2024 · 52973db · knst · Dec 17, 2024 · knst
1 parent 3d5dc16
commit 52973db
Show file tree

Hide file tree

Showing 51 changed files with 744 additions and 525 deletions.
diff --git a/src/bls/bls.cpp b/src/bls/bls.cpp
@@ -41,8 +41,9 @@ void CBLSSecretKey::AggregateInsecure(const CBLSSecretKey& o)
 
 CBLSSecretKey CBLSSecretKey::AggregateInsecure(Span<CBLSSecretKey> sks)
 {
+    CBLSSecretKey ret{};
     if (sks.empty()) {
-        return {};
+        return ret;
     }
 
     std::vector<bls::PrivateKey> v;
@@ -51,7 +52,6 @@ CBLSSecretKey CBLSSecretKey::AggregateInsecure(Span<CBLSSecretKey> sks)
         v.emplace_back(sk.impl);
     }
 
-    CBLSSecretKey ret;
     ret.impl = bls::PrivateKey::Aggregate(v);
     ret.fValid = true;
     ret.cachedHash.SetNull();
@@ -106,11 +106,11 @@ bool CBLSSecretKey::SecretKeyShare(Span<CBLSSecretKey> msk, const CBLSId& _id)
 
 CBLSPublicKey CBLSSecretKey::GetPublicKey() const
 {
+    CBLSPublicKey pubKey{};
     if (!IsValid()) {
-        return {};
+        return pubKey;
     }
 
-    CBLSPublicKey pubKey;
     pubKey.impl = impl.GetG1Element();
     pubKey.fValid = true;
     pubKey.cachedHash.SetNull();
@@ -124,11 +124,11 @@ CBLSSignature CBLSSecretKey::Sign(const uint256& hash) const
 
 CBLSSignature CBLSSecretKey::Sign(const uint256& hash, const bool specificLegacyScheme) const
 {
+    CBLSSignature sigRet{};
     if (!IsValid()) {
-        return {};
+        return sigRet;
     }
 
-    CBLSSignature sigRet;
     try {
         sigRet.impl = Scheme(specificLegacyScheme)->Sign(impl, bls::Bytes(hash.begin(), hash.size()));
         sigRet.fValid = true;
@@ -154,8 +154,9 @@ void CBLSPublicKey::AggregateInsecure(const CBLSPublicKey& o)
 
 CBLSPublicKey CBLSPublicKey::AggregateInsecure(Span<CBLSPublicKey> pks)
 {
+    CBLSPublicKey ret{};
     if (pks.empty()) {
-        return {};
+        return ret;
     }
 
     std::vector<bls::G1Element> vecPublicKeys;
@@ -164,7 +165,6 @@ CBLSPublicKey CBLSPublicKey::AggregateInsecure(Span<CBLSPublicKey> pks)
         vecPublicKeys.emplace_back(pk.impl);
     }
 
-    CBLSPublicKey ret;
     try {
         ret.impl = Scheme(bls::bls_legacy_scheme.load())->Aggregate(vecPublicKeys);
         ret.fValid = true;
@@ -232,8 +232,9 @@ void CBLSSignature::AggregateInsecure(const CBLSSignature& o)
 
 CBLSSignature CBLSSignature::AggregateInsecure(Span<CBLSSignature> sigs)
 {
+    CBLSSignature ret{};
     if (sigs.empty()) {
-        return {};
+        return ret;
     }
 
     std::vector<bls::G2Element> v;
@@ -242,7 +243,6 @@ CBLSSignature CBLSSignature::AggregateInsecure(Span<CBLSSignature> sigs)
         v.emplace_back(pk.impl);
     }
 
-    CBLSSignature ret;
     try {
         ret.impl = Scheme(bls::bls_legacy_scheme.load())->Aggregate(v);
         ret.fValid = true;
@@ -258,8 +258,9 @@ CBLSSignature CBLSSignature::AggregateSecure(Span<CBLSSignature> sigs,
                                              Span<CBLSPublicKey> pks,
                                              const uint256& hash)
 {
+    CBLSSignature ret{};
     if (sigs.size() != pks.size() || sigs.empty()) {
-        return {};
+        return ret;
     }
 
     std::vector<bls::G1Element> vecPublicKeys;
@@ -274,7 +275,6 @@ CBLSSignature CBLSSignature::AggregateSecure(Span<CBLSSignature> sigs,
         vecSignatures.push_back(sig.impl);
     }
 
-    CBLSSignature ret;
     try {
         ret.impl = Scheme(bls::bls_legacy_scheme.load())->AggregateSecure(vecPublicKeys, vecSignatures, bls::Bytes(hash.begin(), hash.size()));
         ret.fValid = true;

diff --git a/src/bls/bls_worker.h b/src/bls/bls_worker.h
@@ -164,23 +164,35 @@ class CBLSWorkerCache
 
 private:
     template <typename T, typename Builder>
-    T GetOrBuild(const uint256& cacheKey, std::map<uint256, std::shared_future<T> >& cache, Builder&& builder)
+    T GetOrBuild(const uint256& cacheKey, std::map<uint256, std::shared_future<T>>& cache, Builder&& builder)
     {
-        cacheCs.lock();
-        auto it = cache.find(cacheKey);
-        if (it != cache.end()) {
-            auto f = it->second;
-            cacheCs.unlock();
-            return f.get();
+        std::promise<T> promise;
+
+        auto opt_future = [&]() -> std::optional<std::shared_future<T>> {
+            // Use lock_guard for exception-safe locking
+            std::lock_guard<std::mutex> lock(cacheCs);
+            auto it = cache.find(cacheKey);
+            if (it != cache.end()) {
+                // Cache hit
+                return std::make_optional(it->second);
+            } else {
+                // Cache miss: insert a new promise's future into the cache
+                cache.emplace(cacheKey, promise.get_future());
+                return std::nullopt;
+            }
+        }();
+
+        T result;
+        if (!opt_future.has_value()) {
+            // Build the value outside the lock to avoid blocking other threads
+            result = builder();
+            promise.set_value(result);
+        } else {
+            // Retrieve the value from the existing future
+            result = promise.get_future().get();
         }
 
-        std::promise<T> p;
-        cache.emplace(cacheKey, p.get_future());
-        cacheCs.unlock();
-
-        T v = builder();
-        p.set_value(v);
-        return v;
+        return result; // NRVO can optimize this return
     }
 };
 

diff --git a/src/evo/creditpool.cpp b/src/evo/creditpool.cpp
@@ -207,10 +207,11 @@ CCreditPool CCreditPoolManager::ConstructCreditPool(const gsl::not_null<const CB
             strprintf("Negative limit for CreditPool: %d.%08d\n", currentLimit / COIN, currentLimit % COIN));
     }
 
-    CCreditPool pool{locked, currentLimit, latelyUnlocked, indexes};
-    AddToCache(block_index->GetBlockHash(), block_index->nHeight, pool);
-    return pool;
-
+    return [&]() {
+        CCreditPool pool{locked, currentLimit, latelyUnlocked, indexes};
+        AddToCache(block_index->GetBlockHash(), block_index->nHeight, pool);
+        return pool;
+    }();
 }
 
 CCreditPool CCreditPoolManager::GetCreditPool(const CBlockIndex* block_index, const Consensus::Params& consensusParams)

diff --git a/src/evo/deterministicmns.cpp b/src/evo/deterministicmns.cpp
@@ -105,7 +105,7 @@ CDeterministicMNCPtr CDeterministicMNList::GetValidMN(const uint256& proTxHash)
 {
     auto dmn = GetMN(proTxHash);
     if (dmn && !IsMNValid(*dmn)) {
-        return nullptr;
+        dmn = nullptr;
     }
     return dmn;
 }
@@ -129,7 +129,7 @@ CDeterministicMNCPtr CDeterministicMNList::GetValidMNByCollateral(const COutPoin
 {
     auto dmn = GetMNByCollateral(collateralOutpoint);
     if (dmn && !IsMNValid(*dmn)) {
-        return nullptr;
+        dmn = nullptr;
     }
     return dmn;
 }
@@ -176,15 +176,15 @@ static bool CompareByLastPaid(const CDeterministicMN* _a, const CDeterministicMN
 
 CDeterministicMNCPtr CDeterministicMNList::GetMNPayee(gsl::not_null<const CBlockIndex*> pindexPrev) const
 {
+    CDeterministicMNCPtr best = nullptr;
     if (mnMap.size() == 0) {
-        return nullptr;
+        return best;
     }
 
     const bool isv19Active{DeploymentActiveAfter(pindexPrev, Params().GetConsensus(), Consensus::DEPLOYMENT_V19)};
     const bool isMNRewardReallocation{DeploymentActiveAfter(pindexPrev, Params().GetConsensus(), Consensus::DEPLOYMENT_MN_RR)};
     // EvoNodes are rewarded 4 blocks in a row until MNRewardReallocation (Platform release)
     // For optimization purposes we also check if v19 active to avoid loop over all masternodes
-    CDeterministicMNCPtr best = nullptr;
     if (isv19Active && !isMNRewardReallocation) {
         ForEachMNShared(true, [&](const CDeterministicMNCPtr& dmn) {
             if (dmn->pdmnState->nLastPaidHeight == nHeight) {
@@ -213,15 +213,15 @@ CDeterministicMNCPtr CDeterministicMNList::GetMNPayee(gsl::not_null<const CBlock
 
 std::vector<CDeterministicMNCPtr> CDeterministicMNList::GetProjectedMNPayees(gsl::not_null<const CBlockIndex* const> pindexPrev, int nCount) const
 {
+    std::vector<CDeterministicMNCPtr> result;
     if (nCount < 0 ) {
-        return {};
+        return result;
     }
     const bool isMNRewardReallocation = DeploymentActiveAfter(pindexPrev, Params().GetConsensus(),
                                                               Consensus::DEPLOYMENT_MN_RR);
     const auto weighted_count = isMNRewardReallocation ? GetValidMNsCount() : GetValidWeightedMNsCount();
     nCount = std::min(nCount, int(weighted_count));
 
-    std::vector<CDeterministicMNCPtr> result;
     result.reserve(weighted_count);
 
     int remaining_evo_payments{0};
@@ -1521,20 +1521,21 @@ static bool CheckHashSig(const ProTx& proTx, const CBLSPublicKey& pubKey, TxVali
 template<typename ProTx>
 static std::optional<ProTx> GetValidatedPayload(const CTransaction& tx, gsl::not_null<const CBlockIndex*> pindexPrev, TxValidationState& state)
 {
+    std::optional<ProTx> opt_ptx{std::nullopt};
     if (tx.nType != ProTx::SPECIALTX_TYPE) {
         state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-type");
-        return std::nullopt;
+        return opt_ptx;
     }
 
-    auto opt_ptx = GetTxPayload<ProTx>(tx);
-    if (!opt_ptx) {
+    opt_ptx = GetTxPayload<ProTx>(tx);
+    if (opt_ptx == std::nullopt) {
         state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-payload");
-        return std::nullopt;
+        return opt_ptx;
     }
     const bool is_basic_scheme_active{DeploymentActiveAfter(pindexPrev, Params().GetConsensus(), Consensus::DEPLOYMENT_V19)};
     if (!opt_ptx->IsTriviallyValid(is_basic_scheme_active, state)) {
         // pass the state returned by the function above
-        return std::nullopt;
+        opt_ptx = std::nullopt;
     }
     return opt_ptx;
 }

diff --git a/src/evo/mnhftx.cpp b/src/evo/mnhftx.cpp
@@ -56,15 +56,14 @@ CMNHFManager::~CMNHFManager()
 
 CMNHFManager::Signals CMNHFManager::GetSignalsStage(const CBlockIndex* const pindexPrev)
 {
-    if (!DeploymentActiveAfter(pindexPrev, Params().GetConsensus(), Consensus::DEPLOYMENT_V20)) return {};
+    Signals signals_ret;
+    if (!DeploymentActiveAfter(pindexPrev, Params().GetConsensus(), Consensus::DEPLOYMENT_V20)) return signals_ret;
 
     Signals signals_tmp = GetForBlock(pindexPrev);
 
-    if (pindexPrev == nullptr) return {};
+    if (pindexPrev == nullptr) return signals_ret;
     const int height = pindexPrev->nHeight + 1;
 
-    Signals signals_ret;
-
     for (auto signal : signals_tmp) {
         bool expired{false};
         const auto signal_pindex = pindexPrev->GetAncestor(signal.second);

diff --git a/src/governance/object.cpp b/src/governance/object.cpp
@@ -192,17 +192,18 @@ void CGovernanceObject::ClearMasternodeVotes(const CDeterministicMNList& tip_mn_
 
 std::set<uint256> CGovernanceObject::RemoveInvalidVotes(const CDeterministicMNList& tip_mn_list, const COutPoint& mnOutpoint)
 {
+    std::set<uint256> removedVotes{};
     LOCK(cs);
 
     auto it = mapCurrentMNVotes.find(mnOutpoint);
     if (it == mapCurrentMNVotes.end()) {
         // don't even try as we don't have any votes from this MN
-        return {};
+        return removedVotes;
     }
 
-    auto removedVotes = fileVotes.RemoveInvalidVotes(tip_mn_list, mnOutpoint, m_obj.type == GovernanceObject::PROPOSAL);
+    removedVotes = fileVotes.RemoveInvalidVotes(tip_mn_list, mnOutpoint, m_obj.type == GovernanceObject::PROPOSAL);
     if (removedVotes.empty()) {
-        return {};
+        return removedVotes;
     }
 
     auto nParentHash = GetHash();

diff --git a/src/httpserver.cpp b/src/httpserver.cpp
@@ -592,9 +592,11 @@ std::string HTTPRequest::ReadBody()
     const char* data = (const char*)evbuffer_pullup(buf, size);
     if (!data) // returns nullptr in case of empty buffer
         return "";
-    std::string rv(data, size);
-    evbuffer_drain(buf, size);
-    return rv;
+    return [&]() {
+        std::string rv(data, size);
+        evbuffer_drain(buf, size);
+        return rv;
+    }();
 }
 
 void HTTPRequest::WriteHeader(const std::string& hdr, const std::string& value)

diff --git a/src/llmq/blockprocessor.cpp b/src/llmq/blockprocessor.cpp
@@ -54,14 +54,14 @@ CQuorumBlockProcessor::CQuorumBlockProcessor(CChainState& chainstate, CDetermini
 MessageProcessingResult CQuorumBlockProcessor::ProcessMessage(const CNode& peer, std::string_view msg_type,
                                                               CDataStream& vRecv)
 {
+    MessageProcessingResult ret;
     if (msg_type != NetMsgType::QFCOMMITMENT) {
-        return {};
+        return ret;
     }
 
     CFinalCommitment qc;
     vRecv >> qc;
 
-    MessageProcessingResult ret;
     ret.m_to_erase = CInv{MSG_QUORUM_FINAL_COMMITMENT, ::SerializeHash(qc)};
 
     if (qc.IsNull()) {
@@ -443,7 +443,8 @@ uint256 CQuorumBlockProcessor::GetQuorumBlockHash(const Consensus::LLMQParams& l
     uint256 quorumBlockHash;
     if (!GetBlockHash(active_chain, quorumBlockHash, quorumStartHeight)) {
         LogPrint(BCLog::LLMQ, "[GetQuorumBlockHash] llmqType[%d] h[%d] qi[%d] quorumStartHeight[%d] quorumHash[EMPTY]\n", ToUnderlying(llmqParams.type), nHeight, quorumIndex, quorumStartHeight);
-        return {};
+        quorumBlockHash = uint256(); // This should already be the case as a failed GetBlockHash doesn't set anything. Preserve NRVO
+        return quorumBlockHash;
     }
 
     LogPrint(BCLog::LLMQ, "[GetQuorumBlockHash] llmqType[%d] h[%d] qi[%d] quorumStartHeight[%d] quorumHash[%s]\n", ToUnderlying(llmqParams.type), nHeight, quorumIndex, quorumStartHeight, quorumBlockHash.ToString());

diff --git a/src/llmq/chainlocks.cpp b/src/llmq/chainlocks.cpp
@@ -408,7 +408,8 @@ CChainLocksHandler::BlockTxs::mapped_type CChainLocksHandler::GetBlockTxs(const
             const auto* pindex = m_chainstate.m_blockman.LookupBlockIndex(blockHash);
             CBlock block;
             if (!ReadBlockFromDisk(block, pindex, Params().GetConsensus())) {
-                return nullptr;
+                ret = nullptr;
+                return ret;
             }
 
             ret = std::make_shared<std::unordered_set<uint256, StaticSaltedHasher>>();

diff --git a/src/llmq/dkgsession.cpp b/src/llmq/dkgsession.cpp
@@ -1184,8 +1184,9 @@ std::optional<CInv> CDKGSession::ReceiveMessage(const CDKGPrematureCommitment& q
 
 std::vector<CFinalCommitment> CDKGSession::FinalizeCommitments()
 {
+    std::vector<CFinalCommitment> finalCommitments;
     if (!AreWeMember()) {
-        return {};
+        return finalCommitments;
     }
 
     CDKGLogger logger(*this, __func__, __LINE__);
@@ -1214,7 +1215,6 @@ std::vector<CFinalCommitment> CDKGSession::FinalizeCommitments()
         }
     }
 
-    std::vector<CFinalCommitment> finalCommitments;
     for (const auto& p : commitmentsMap) {
         const auto& cvec = p.second;
         if (cvec.size() < size_t(params.minSize)) {

diff --git a/src/llmq/dkgsessionhandler.cpp b/src/llmq/dkgsessionhandler.cpp
@@ -353,11 +353,11 @@ void CDKGSessionHandler::HandlePhase(QuorumPhase curPhase,
 template<typename Message>
 std::set<NodeId> BatchVerifyMessageSigs(CDKGSession& session, const std::vector<std::pair<NodeId, std::shared_ptr<Message>>>& messages)
 {
+    std::set<NodeId> ret;
     if (messages.empty()) {
-        return {};
+        return ret;
     }
 
-    std::set<NodeId> ret;
     bool revertToSingleVerification = false;
 
     CBLSSignature aggSig;

diff --git a/src/llmq/dkgsessionhandler.h b/src/llmq/dkgsessionhandler.h
@@ -92,12 +92,12 @@ class CDKGPendingMessages
     template<typename Message>
     std::vector<std::pair<NodeId, std::shared_ptr<Message>>> PopAndDeserializeMessages(size_t maxCount)
     {
+        std::vector<std::pair<NodeId, std::shared_ptr<Message>>> ret{};
         auto binaryMessages = PopPendingMessages(maxCount);
         if (binaryMessages.empty()) {
-            return {};
+            return ret;
         }
 
-        std::vector<std::pair<NodeId, std::shared_ptr<Message>>> ret;
         ret.reserve(binaryMessages.size());
         for (const auto& bm : binaryMessages) {
             auto msg = std::make_shared<Message>();