diff --git a/src/coinjoin/client.cpp b/src/coinjoin/client.cpp index 53fcda8b52641..c629ba15a6b86 100644 --- a/src/coinjoin/client.cpp +++ b/src/coinjoin/client.cpp @@ -35,92 +35,91 @@ void CCoinJoinClientQueueManager::ProcessMessage(CNode* pfrom, const std::string if (fMasternodeMode) return; if (!CCoinJoinClientOptions::IsEnabled()) return; if (!masternodeSync.IsBlockchainSynced()) return; + if (strCommand != NetMsgType::DSQUEUE) return; if (!CheckDiskSpace(GetDataDir())) { LogPrint(BCLog::COINJOIN, "CCoinJoinClientQueueManager::ProcessMessage -- Not enough disk space, disabling CoinJoin.\n"); return; } - if (strCommand == NetMsgType::DSQUEUE) { - if (pfrom->nVersion < MIN_COINJOIN_PEER_PROTO_VERSION) { - LogPrint(BCLog::COINJOIN, "DSQUEUE -- peer=%d using obsolete version %i\n", pfrom->GetId(), pfrom->nVersion); - if (enable_bip61) { - connman.PushMessage(pfrom, CNetMsgMaker(pfrom->GetSendVersion()).Make(NetMsgType::REJECT, strCommand, - REJECT_OBSOLETE, strprintf( - "Version must be %d or greater", MIN_COINJOIN_PEER_PROTO_VERSION))); - } - return; + if (pfrom->nVersion < MIN_COINJOIN_PEER_PROTO_VERSION) { + LogPrint(BCLog::COINJOIN, "DSQUEUE -- peer=%d using obsolete version %i\n", pfrom->GetId(), pfrom->nVersion); + if (enable_bip61) { + connman.PushMessage(pfrom, CNetMsgMaker(pfrom->GetSendVersion()).Make(NetMsgType::REJECT, strCommand, + REJECT_OBSOLETE, strprintf( + "Version must be %d or greater", MIN_COINJOIN_PEER_PROTO_VERSION))); } + return; + } - CCoinJoinQueue dsq; - vRecv >> dsq; + CCoinJoinQueue dsq; + vRecv >> dsq; - { - TRY_LOCK(cs_vecqueue, lockRecv); - if (!lockRecv) return; + { + TRY_LOCK(cs_vecqueue, lockRecv); + if (!lockRecv) return; - // process every dsq only once - for (const auto& q : vecCoinJoinQueue) { - if (q == dsq) { - return; - } - if (q.fReady == dsq.fReady && q.masternodeOutpoint == dsq.masternodeOutpoint) { - // no way the same mn can send another dsq with the same readiness this soon - LogPrint(BCLog::COINJOIN, "DSQUEUE -- Peer %s is sending WAY too many dsq messages for a masternode with collateral %s\n", pfrom->GetLogString(), dsq.masternodeOutpoint.ToStringShort()); - return; - } + // process every dsq only once + for (const auto& q : vecCoinJoinQueue) { + if (q == dsq) { + return; } - } // cs_vecqueue + if (q.fReady == dsq.fReady && q.masternodeOutpoint == dsq.masternodeOutpoint) { + // no way the same mn can send another dsq with the same readiness this soon + LogPrint(BCLog::COINJOIN, "DSQUEUE -- Peer %s is sending WAY too many dsq messages for a masternode with collateral %s\n", pfrom->GetLogString(), dsq.masternodeOutpoint.ToStringShort()); + return; + } + } + } // cs_vecqueue - LogPrint(BCLog::COINJOIN, "DSQUEUE -- %s new\n", dsq.ToString()); + LogPrint(BCLog::COINJOIN, "DSQUEUE -- %s new\n", dsq.ToString()); - if (dsq.IsTimeOutOfBounds()) return; + if (dsq.IsTimeOutOfBounds()) return; - auto mnList = deterministicMNManager->GetListAtChainTip(); - auto dmn = mnList.GetValidMNByCollateral(dsq.masternodeOutpoint); - if (!dmn) return; + auto mnList = deterministicMNManager->GetListAtChainTip(); + auto dmn = mnList.GetValidMNByCollateral(dsq.masternodeOutpoint); + if (!dmn) return; - if (!dsq.CheckSignature(dmn->pdmnState->pubKeyOperator.Get())) { - LOCK(cs_main); - Misbehaving(pfrom->GetId(), 10); - return; - } + if (!dsq.CheckSignature(dmn->pdmnState->pubKeyOperator.Get())) { + LOCK(cs_main); + Misbehaving(pfrom->GetId(), 10); + return; + } - // if the queue is ready, submit if we can - if (dsq.fReady) { - for (const auto& pair : coinJoinClientManagers) { - if (pair.second->TrySubmitDenominate(dmn->pdmnState->addr, connman)) { - LogPrint(BCLog::COINJOIN, "DSQUEUE -- CoinJoin queue (%s) is ready on masternode %s\n", dsq.ToString(), dmn->pdmnState->addr.ToString()); - return; - } - } - } else { - int64_t nLastDsq = mmetaman.GetMetaInfo(dmn->proTxHash)->GetLastDsq(); - int64_t nDsqThreshold = mmetaman.GetDsqThreshold(dmn->proTxHash, mnList.GetValidMNsCount()); - LogPrint(BCLog::COINJOIN, "DSQUEUE -- nLastDsq: %d nDsqThreshold: %d nDsqCount: %d\n", nLastDsq, nDsqThreshold, mmetaman.GetDsqCount()); - // don't allow a few nodes to dominate the queuing process - if (nLastDsq != 0 && nDsqThreshold > mmetaman.GetDsqCount()) { - LogPrint(BCLog::COINJOIN, "DSQUEUE -- Masternode %s is sending too many dsq messages\n", dmn->proTxHash.ToString()); + // if the queue is ready, submit if we can + if (dsq.fReady) { + for (const auto& [_, coinJoinMan] : coinJoinClientManagers) { + if (coinJoinMan->TrySubmitDenominate(dmn->pdmnState->addr, connman)) { + LogPrint(BCLog::COINJOIN, "DSQUEUE -- CoinJoin queue (%s) is ready on masternode %s\n", dsq.ToString(), dmn->pdmnState->addr.ToString()); return; } + } + } else { + int64_t nLastDsq = mmetaman.GetMetaInfo(dmn->proTxHash)->GetLastDsq(); + int64_t nDsqThreshold = mmetaman.GetDsqThreshold(dmn->proTxHash, mnList.GetValidMNsCount()); + LogPrint(BCLog::COINJOIN, "DSQUEUE -- nLastDsq: %d nDsqThreshold: %d nDsqCount: %d\n", nLastDsq, nDsqThreshold, mmetaman.GetDsqCount()); + // don't allow a few nodes to dominate the queuing process + if (nLastDsq != 0 && nDsqThreshold > mmetaman.GetDsqCount()) { + LogPrint(BCLog::COINJOIN, "DSQUEUE -- Masternode %s is sending too many dsq messages\n", dmn->proTxHash.ToString()); + return; + } - mmetaman.AllowMixing(dmn->proTxHash); + mmetaman.AllowMixing(dmn->proTxHash); - LogPrint(BCLog::COINJOIN, "DSQUEUE -- new CoinJoin queue (%s) from masternode %s\n", dsq.ToString(), dmn->pdmnState->addr.ToString()); + LogPrint(BCLog::COINJOIN, "DSQUEUE -- new CoinJoin queue (%s) from masternode %s\n", dsq.ToString(), dmn->pdmnState->addr.ToString()); - for (const auto& pair : coinJoinClientManagers) { - if (pair.second->MarkAlreadyJoinedQueueAsTried(dsq)) { - break; - } + for (const auto& [_, coinJoinMan] : coinJoinClientManagers) { + if (coinJoinMan->MarkAlreadyJoinedQueueAsTried(dsq)) { + break; } - - TRY_LOCK(cs_vecqueue, lockRecv); - if (!lockRecv) return; - vecCoinJoinQueue.push_back(dsq); - dsq.Relay(connman); } + TRY_LOCK(cs_vecqueue, lockRecv); + if (!lockRecv) return; + vecCoinJoinQueue.push_back(dsq); + dsq.Relay(connman); } + } void CCoinJoinClientManager::ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStream& vRecv, CConnman& connman, bool enable_bip61) @@ -368,22 +367,21 @@ std::string CCoinJoinClientManager::GetSessionDenoms() return strSessionDenoms.empty() ? "N/A" : strSessionDenoms; } -bool CCoinJoinClientSession::GetMixingMasternodeInfo(CDeterministicMNCPtr& ret) const +std::optional CCoinJoinClientSession::GetMixingMasternodeInfo() const { - ret = mixingMasternode; - return ret != nullptr; + return mixingMasternode ? std::optional{mixingMasternode} : std::nullopt; } -bool CCoinJoinClientManager::GetMixingMasternodesInfo(std::vector& vecDmnsRet) const +std::vector CCoinJoinClientManager::GetMixingMasternodesInfo() const { LOCK(cs_deqsessions); + std::vector vecDmnsRet; for (const auto& session : deqSessions) { - CDeterministicMNCPtr dmn; - if (session.GetMixingMasternodeInfo(dmn)) { - vecDmnsRet.push_back(dmn); + if (auto dmn = session.GetMixingMasternodeInfo()) { + vecDmnsRet.push_back(*dmn); } } - return !vecDmnsRet.empty(); + return vecDmnsRet; } // @@ -481,11 +479,11 @@ bool CCoinJoinClientSession::SendDenominate(const std::vector vecTxDSInTmp; std::vector vecTxOutTmp; - for (const auto& pair : vecPSInOutPairsIn) { - vecTxDSInTmp.emplace_back(pair.first); - vecTxOutTmp.emplace_back(pair.second); - tx.vin.emplace_back(pair.first); - tx.vout.emplace_back(pair.second); + for (const auto& [txDsIn, txOut] : vecPSInOutPairsIn) { + vecTxDSInTmp.emplace_back(txDsIn); + vecTxOutTmp.emplace_back(txOut); + tx.vin.emplace_back(txDsIn); + tx.vout.emplace_back(txOut); } LogPrint(BCLog::COINJOIN, "CCoinJoinClientSession::SendDenominate -- Submitting partial tx %s", tx.ToString()); /* Continued */ @@ -581,8 +579,8 @@ bool CCoinJoinClientSession::SignFinalTransaction(const CTransaction& finalTrans } // Make sure all inputs/outputs are valid - PoolMessage nMessageID{MSG_NOERR}; - if (!IsValidInOuts(finalMutableTransaction.vin, finalMutableTransaction.vout, nMessageID, nullptr)) { + auto [success, _, nMessageID] = IsValidInOuts(finalMutableTransaction.vin, finalMutableTransaction.vout); + if (!success) { LogPrint(BCLog::COINJOIN, "CCoinJoinClientSession::%s -- ERROR! IsValidInOuts() failed: %s\n", __func__, CCoinJoin::GetMessageByID(nMessageID)); UnlockCoins(); keyHolderStorage.ReturnAll(); @@ -1057,8 +1055,8 @@ bool CCoinJoinClientSession::JoinExistingQueue(CAmount nBalanceNeedsAnonymized, } // Look through the queues and see if anything matches - CCoinJoinQueue dsq; - while (coinJoinClientQueueManager.GetQueueItemAndTry(dsq)) { + while (auto opt_dsq = coinJoinClientQueueManager.GetQueueItemAndTry()) { + const auto& dsq = *opt_dsq; auto dmn = mnList.GetValidMNByCollateral(dsq.masternodeOutpoint); if (!dmn) { @@ -1078,10 +1076,8 @@ bool CCoinJoinClientSession::JoinExistingQueue(CAmount nBalanceNeedsAnonymized, LogPrint(BCLog::COINJOIN, "CCoinJoinClientSession::JoinExistingQueue -- trying queue: %s\n", dsq.ToString()); - std::vector vecTxDSInTmp; - // Try to match their denominations if possible, select exact number of denominations - if (!mixingWallet.SelectTxDSInsByDenomination(dsq.nDenom, nBalanceNeedsAnonymized, vecTxDSInTmp)) { + if (!mixingWallet.SelectTxDSInsByDenomination(dsq.nDenom, nBalanceNeedsAnonymized)) { LogPrint(BCLog::COINJOIN, "CCoinJoinClientSession::JoinExistingQueue -- Couldn't match denomination %d (%s)\n", dsq.nDenom, CCoinJoin::DenominationToString(dsq.nDenom)); continue; } @@ -1227,10 +1223,11 @@ bool CCoinJoinClientManager::TrySubmitDenominate(const CService& mnAddr, CConnma { LOCK(cs_deqsessions); for (auto& session : deqSessions) { - CDeterministicMNCPtr mnMixing; - if (session.GetMixingMasternodeInfo(mnMixing) && mnMixing->pdmnState->addr == mnAddr && session.GetState() == POOL_STATE_QUEUE) { - session.SubmitDenominate(connman); - return true; + if (auto mnMixing = session.GetMixingMasternodeInfo()) { + if (mnMixing->get()->pdmnState->addr == mnAddr && session.GetState() == POOL_STATE_QUEUE) { + session.SubmitDenominate(connman); + return true; + } } } return false; @@ -1240,10 +1237,11 @@ bool CCoinJoinClientManager::MarkAlreadyJoinedQueueAsTried(CCoinJoinQueue& dsq) { LOCK(cs_deqsessions); for (const auto& session : deqSessions) { - CDeterministicMNCPtr mnMixing; - if (session.GetMixingMasternodeInfo(mnMixing) && mnMixing->collateralOutpoint == dsq.masternodeOutpoint) { - dsq.fTried = true; - return true; + if (auto mnMixing = session.GetMixingMasternodeInfo()) { + if (mnMixing->get()->collateralOutpoint == dsq.masternodeOutpoint) { + dsq.fTried = true; + return true; + } } } return false; @@ -1254,11 +1252,9 @@ bool CCoinJoinClientSession::SubmitDenominate(CConnman& connman) LOCK2(cs_main, mempool.cs); LOCK(mixingWallet.cs_wallet); - std::string strError; - std::vector vecTxDSIn; - std::vector > vecPSInOutPairsTmp; + auto [optVecTxDSIn, strError] = SelectDenominate(); - if (!SelectDenominate(strError, vecTxDSIn)) { + if (!optVecTxDSIn) { LogPrint(BCLog::COINJOIN, "CCoinJoinClientSession::SubmitDenominate -- SelectDenominate failed, error: %s\n", strError); return false; } @@ -1266,9 +1262,9 @@ bool CCoinJoinClientSession::SubmitDenominate(CConnman& connman) std::vector > vecInputsByRounds; for (int i = 0; i < CCoinJoinClientOptions::GetRounds() + CCoinJoinClientOptions::GetRandomRounds(); i++) { - if (PrepareDenominate(i, i, strError, vecTxDSIn, vecPSInOutPairsTmp, true)) { + if (auto vecPSInOutPairs = PrepareDenominate(i, i, strError, *optVecTxDSIn, true)) { LogPrint(BCLog::COINJOIN, "CCoinJoinClientSession::SubmitDenominate -- Running CoinJoin denominate for %d rounds, success\n", i); - vecInputsByRounds.emplace_back(i, vecPSInOutPairsTmp.size()); + vecInputsByRounds.emplace_back(i, (*vecPSInOutPairs).size()); } else { LogPrint(BCLog::COINJOIN, "CCoinJoinClientSession::SubmitDenominate -- Running CoinJoin denominate for %d rounds, error: %s\n", i, strError); } @@ -1280,20 +1276,20 @@ bool CCoinJoinClientSession::SubmitDenominate(CConnman& connman) }); LogPrint(BCLog::COINJOIN, "vecInputsByRounds for denom %d\n", nSessionDenom); - for (const auto& pair : vecInputsByRounds) { - LogPrint(BCLog::COINJOIN, "vecInputsByRounds: rounds: %d, inputs: %d\n", pair.first, pair.second); + for (const auto [rounds, inputs] : vecInputsByRounds) { + LogPrint(BCLog::COINJOIN, "vecInputsByRounds: rounds: %d, inputs: %d\n", rounds, inputs); } int nRounds = vecInputsByRounds.begin()->first; - if (PrepareDenominate(nRounds, nRounds, strError, vecTxDSIn, vecPSInOutPairsTmp)) { + if (auto vecPSInOutPairs = PrepareDenominate(nRounds, nRounds, strError, *optVecTxDSIn)) { LogPrint(BCLog::COINJOIN, "CCoinJoinClientSession::SubmitDenominate -- Running CoinJoin denominate for %d rounds, success\n", nRounds); - return SendDenominate(vecPSInOutPairsTmp, connman); + return SendDenominate(*vecPSInOutPairs, connman); } // We failed? That's strange but let's just make final attempt and try to mix everything - if (PrepareDenominate(0, CCoinJoinClientOptions::GetRounds() - 1, strError, vecTxDSIn, vecPSInOutPairsTmp)) { + if (auto vecPSInOutPairs = PrepareDenominate(0, CCoinJoinClientOptions::GetRounds() - 1, strError, *optVecTxDSIn)) { LogPrint(BCLog::COINJOIN, "CCoinJoinClientSession::SubmitDenominate -- Running CoinJoin denominate for all rounds, success\n"); - return SendDenominate(vecPSInOutPairsTmp, connman); + return SendDenominate(*vecPSInOutPairs, connman); } // Should never actually get here but just in case @@ -1302,46 +1298,41 @@ bool CCoinJoinClientSession::SubmitDenominate(CConnman& connman) return false; } -bool CCoinJoinClientSession::SelectDenominate(std::string& strErrorRet, std::vector& vecTxDSInRet) +std::pair>, std::string> CCoinJoinClientSession::SelectDenominate() { - if (!CCoinJoinClientOptions::IsEnabled()) return false; + if (!CCoinJoinClientOptions::IsEnabled()) return {std::nullopt, "Coinjoin is disabled"}; if (mixingWallet.IsLocked(true)) { - strErrorRet = "Wallet locked, unable to create transaction!"; - return false; + return {std::nullopt, "Wallet locked, unable to create transaction!"}; } if (GetEntriesCount() > 0) { - strErrorRet = "Already have pending entries in the CoinJoin pool"; - return false; + return {std::nullopt, "Already have pending entries in the CoinJoin pool"}; } - vecTxDSInRet.clear(); - - bool fSelected = mixingWallet.SelectTxDSInsByDenomination(nSessionDenom, CCoinJoin::GetMaxPoolAmount(), vecTxDSInRet); - if (!fSelected) { - strErrorRet = "Can't select current denominated inputs"; - return false; + auto selected_denoms = mixingWallet.SelectTxDSInsByDenomination(nSessionDenom, CCoinJoin::GetMaxPoolAmount()); + if (!selected_denoms) { + return {std::nullopt, "Can't select current denominated inputs"}; } - return true; + return {selected_denoms, ""}; } -bool CCoinJoinClientSession::PrepareDenominate(int nMinRounds, int nMaxRounds, std::string& strErrorRet, const std::vector& vecTxDSIn, std::vector >& vecPSInOutPairsRet, bool fDryRun) +std::optional>> CCoinJoinClientSession::PrepareDenominate(int nMinRounds, int nMaxRounds, std::string& strErrorRet, const std::vector& vecTxDSIn, bool fDryRun) { AssertLockHeld(cs_main); AssertLockHeld(mixingWallet.cs_wallet); if (!CCoinJoin::IsValidDenomination(nSessionDenom)) { strErrorRet = "Incorrect session denom"; - return false; + return std::nullopt; } CAmount nDenomAmount = CCoinJoin::DenominationToAmount(nSessionDenom); // NOTE: No need to randomize order of inputs because they were // initially shuffled in CWallet::SelectTxDSInsByDenomination already. int nSteps{0}; - vecPSInOutPairsRet.clear(); + std::vector> vecPSInOutPairsRet; // Try to add up to COINJOIN_ENTRY_MAX_SIZE of every needed denomination for (const auto& entry : vecTxDSIn) { @@ -1363,7 +1354,7 @@ bool CCoinJoinClientSession::PrepareDenominate(int nMinRounds, int nMaxRounds, s const auto pwallet = GetWallet(mixingWallet.GetName()); if (!pwallet) { strErrorRet ="Couldn't get wallet pointer"; - return false; + return std::nullopt; } scriptDenom = keyHolderStorage.AddKey(pwallet.get()); } @@ -1375,19 +1366,17 @@ bool CCoinJoinClientSession::PrepareDenominate(int nMinRounds, int nMaxRounds, s if (vecPSInOutPairsRet.empty()) { keyHolderStorage.ReturnAll(); strErrorRet = "Can't prepare current denominated outputs"; - return false; - } - - if (fDryRun) { - return true; + return std::nullopt; } - for (const auto& pair : vecPSInOutPairsRet) { - mixingWallet.LockCoin(pair.first.prevout); - vecOutPointLocked.push_back(pair.first.prevout); + if (!fDryRun) { + for (const auto& [txDsIn, _] : vecPSInOutPairsRet) { + mixingWallet.LockCoin(txDsIn.prevout); + vecOutPointLocked.push_back(txDsIn.prevout); + } } - return true; + return {vecPSInOutPairsRet}; } // Create collaterals by looping through inputs grouped by addresses @@ -1556,7 +1545,7 @@ bool CCoinJoinClientSession::CreateCollateralTransaction(CMutableTransaction& tx CScript scriptChange; CPubKey vchPubKey; CReserveKey reservekey(&mixingWallet); - bool success = reservekey.GetReservedKey(vchPubKey, true); + [[maybe_unused]] bool success = reservekey.GetReservedKey(vchPubKey, true); assert(success); // should never fail, as we just unlocked scriptChange = GetScriptForDestination(vchPubKey.GetID()); reservekey.KeepKey(); @@ -1703,17 +1692,17 @@ bool CCoinJoinClientSession::CreateDenominated(CAmount nBalanceToDenominate, con } bool finished = true; - for (const auto it : mapDenomCount) { + for (const auto [denom, count] : mapDenomCount) { // Check if this specific denom could use another loop, check that there aren't nCoinJoinDenomsGoal of this // denom and that our nValueLeft/nBalanceToDenominate is enough to create one of these denoms, if so, loop again. - if (it.second < CCoinJoinClientOptions::GetDenomsGoal() && txBuilder.CouldAddOutput(it.first) && nBalanceToDenominate > 0) { + if (count < CCoinJoinClientOptions::GetDenomsGoal() && txBuilder.CouldAddOutput(denom) && nBalanceToDenominate > 0) { finished = false; LogPrint(BCLog::COINJOIN, "CCoinJoinClientSession::%s -- 1 - NOT finished - nDenomValue: %f, count: %d, nBalanceToDenominate: %f, %s\n", - __func__, (float) it.first / COIN, it.second, (float) nBalanceToDenominate / COIN, txBuilder.ToString()); + __func__, (float) denom / COIN, count, (float) nBalanceToDenominate / COIN, txBuilder.ToString()); break; } LogPrint(BCLog::COINJOIN, "CCoinJoinClientSession::%s -- 1 - FINISHED - nDenomValue: %f, count: %d, nBalanceToDenominate: %f, %s\n", - __func__, (float) it.first / COIN, it.second, (float) nBalanceToDenominate / COIN, txBuilder.ToString()); + __func__, (float) denom / COIN, count, (float) nBalanceToDenominate / COIN, txBuilder.ToString()); } if (finished) break; @@ -1782,8 +1771,8 @@ bool CCoinJoinClientSession::CreateDenominated(CAmount nBalanceToDenominate, con LogPrint(BCLog::COINJOIN, "CCoinJoinClientSession::%s -- 3 - nBalanceToDenominate: %f, %s\n", __func__, (float) nBalanceToDenominate / COIN, txBuilder.ToString()); - for (const auto it : mapDenomCount) { - LogPrint(BCLog::COINJOIN, "CCoinJoinClientSession::%s -- 3 - DONE - nDenomValue: %f, count: %d\n", __func__, (float) it.first / COIN, it.second); + for (const auto [denom, count] : mapDenomCount) { + LogPrint(BCLog::COINJOIN, "CCoinJoinClientSession::%s -- 3 - DONE - nDenomValue: %f, count: %d\n", __func__, (float) denom / COIN, count); } // No reasons to create mixing collaterals if we can't create denoms to mix @@ -1892,8 +1881,8 @@ void CCoinJoinClientManager::GetJsonInfo(UniValue& obj) const void DoCoinJoinMaintenance(CConnman& connman) { coinJoinClientQueueManager.DoMaintenance(); - for (const auto& pair : coinJoinClientManagers) { - pair.second->DoMaintenance(connman); + for (const auto& [_, coinJoinMan] : coinJoinClientManagers) { + coinJoinMan->DoMaintenance(connman); } } diff --git a/src/coinjoin/client.h b/src/coinjoin/client.h index ce795da09601d..745ee665fa447 100644 --- a/src/coinjoin/client.h +++ b/src/coinjoin/client.h @@ -9,6 +9,7 @@ #include #include +#include class CDeterministicMN; using CDeterministicMNCPtr = std::shared_ptr; @@ -52,9 +53,9 @@ class CPendingDsaRequest { } - CService GetAddr() const { return addr; } - CCoinJoinAccept GetDSA() const { return dsa; } - bool IsExpired() const { return GetTime() - nTimeCreated > TIMEOUT; } + [[nodiscard]] CService GetAddr() const { return addr; } + [[nodiscard]] CCoinJoinAccept GetDSA() const { return dsa; } + [[nodiscard]] bool IsExpired() const { return GetTime() - nTimeCreated > TIMEOUT; } friend bool operator==(const CPendingDsaRequest& a, const CPendingDsaRequest& b) { @@ -99,10 +100,15 @@ class CCoinJoinClientSession : public CCoinJoinBaseSession bool JoinExistingQueue(CAmount nBalanceNeedsAnonymized, CConnman& connman); bool StartNewQueue(CAmount nBalanceNeedsAnonymized, CConnman& connman); - /// step 0: select denominated inputs and txouts - bool SelectDenominate(std::string& strErrorRet, std::vector& vecTxDSInRet); + /** + * step 0: select denominated inputs and txouts + * @return pair containing optional vector of selected inputs (CTxDSIn), and an error string which is only set + * if returned vector is nullopt + */ + std::pair>, std::string> SelectDenominate(); /// step 1: prepare denominated inputs and outputs - bool PrepareDenominate(int nMinRounds, int nMaxRounds, std::string& strErrorRet, const std::vector& vecTxDSIn, std::vector >& vecPSInOutPairsRet, bool fDryRun = false); + // TODO factor out strErrorRet + std::optional>> PrepareDenominate(int nMinRounds, int nMaxRounds, std::string& strErrorRet, const std::vector& vecTxDSIn, bool fDryRun = false); /// step 2: send denominated inputs and outputs prepared in step 1 bool SendDenominate(const std::vector >& vecPSInOutPairsIn, CConnman& connman); @@ -141,7 +147,7 @@ class CCoinJoinClientSession : public CCoinJoinBaseSession std::string GetStatus(bool fWaitForBlock) const; - bool GetMixingMasternodeInfo(CDeterministicMNCPtr& ret) const; + std::optional GetMixingMasternodeInfo() const; /// Passively run mixing in the background according to the configuration in settings bool DoAutomaticDenominating(CConnman& connman, bool fDryRun = false); @@ -215,7 +221,7 @@ class CCoinJoinClientManager std::string GetStatuses(); std::string GetSessionDenoms(); - bool GetMixingMasternodesInfo(std::vector& vecDmnsRet) const; + std::vector GetMixingMasternodesInfo() const; /// Passively run mixing in the background according to the configuration in settings bool DoAutomaticDenominating(CConnman& connman, bool fDryRun = false); diff --git a/src/coinjoin/coinjoin.cpp b/src/coinjoin/coinjoin.cpp index de7bac827b227..d2b0eb4b159e3 100644 --- a/src/coinjoin/coinjoin.cpp +++ b/src/coinjoin/coinjoin.cpp @@ -175,20 +175,19 @@ void CCoinJoinBaseManager::CheckQueue() } } -bool CCoinJoinBaseManager::GetQueueItemAndTry(CCoinJoinQueue& dsqRet) +std::optional CCoinJoinBaseManager::GetQueueItemAndTry() { TRY_LOCK(cs_vecqueue, lockDS); - if (!lockDS) return false; // it's ok to fail here, we run this quite frequently + if (!lockDS) return std::nullopt; // it's ok to fail here, we run this quite frequently for (auto& dsq : vecCoinJoinQueue) { // only try each queue once if (dsq.fTried || dsq.IsTimeOutOfBounds()) continue; dsq.fTried = true; - dsqRet = dsq; - return true; + return {dsq}; } - return false; + return std::nullopt; } std::string CCoinJoinBaseSession::GetStateString() const @@ -209,52 +208,43 @@ std::string CCoinJoinBaseSession::GetStateString() const } } -bool CCoinJoinBaseSession::IsValidInOuts(const std::vector& vin, const std::vector& vout, PoolMessage& nMessageIDRet, bool* fConsumeCollateralRet) const +std::tuple CCoinJoinBaseSession::IsValidInOuts(const std::vector& vin, const std::vector& vout) const { AssertLockHeld(cs_main); std::set setScripPubKeys; - nMessageIDRet = MSG_NOERR; - if (fConsumeCollateralRet) *fConsumeCollateralRet = false; if (vin.size() != vout.size()) { LogPrint(BCLog::COINJOIN, "CCoinJoinBaseSession::%s -- ERROR: inputs vs outputs size mismatch! %d vs %d\n", __func__, vin.size(), vout.size()); - nMessageIDRet = ERR_SIZE_MISMATCH; - if (fConsumeCollateralRet) *fConsumeCollateralRet = true; - return false; + return {false, true, ERR_SIZE_MISMATCH}; } - auto checkTxOut = [&](const CTxOut& txout) { - int nDenom = CCoinJoin::AmountToDenomination(txout.nValue); - if (nDenom != nSessionDenom) { + // Any checkTxOut failure will result in collateral consumption + auto checkTxOut = [&](const CTxOut& txout) -> std::pair { + if (int nDenom = CCoinJoin::AmountToDenomination(txout.nValue); nDenom != nSessionDenom) { LogPrint(BCLog::COINJOIN, "CCoinJoinBaseSession::IsValidInOuts -- ERROR: incompatible denom %d (%s) != nSessionDenom %d (%s)\n", nDenom, CCoinJoin::DenominationToString(nDenom), nSessionDenom, CCoinJoin::DenominationToString(nSessionDenom)); - nMessageIDRet = ERR_DENOM; - if (fConsumeCollateralRet) *fConsumeCollateralRet = true; - return false; + return {false, ERR_DENOM}; } if (!txout.scriptPubKey.IsPayToPublicKeyHash()) { LogPrint(BCLog::COINJOIN, "CCoinJoinBaseSession::IsValidInOuts -- ERROR: invalid script! scriptPubKey=%s\n", ScriptToAsmStr(txout.scriptPubKey)); - nMessageIDRet = ERR_INVALID_SCRIPT; - if (fConsumeCollateralRet) *fConsumeCollateralRet = true; - return false; + return {false, ERR_INVALID_SCRIPT}; } if (!setScripPubKeys.insert(txout.scriptPubKey).second) { LogPrint(BCLog::COINJOIN, "CCoinJoinBaseSession::IsValidInOuts -- ERROR: already have this script! scriptPubKey=%s\n", ScriptToAsmStr(txout.scriptPubKey)); - nMessageIDRet = ERR_ALREADY_HAVE; - if (fConsumeCollateralRet) *fConsumeCollateralRet = true; - return false; + return {false, ERR_ALREADY_HAVE}; } // IsPayToPublicKeyHash() above already checks for scriptPubKey size, // no need to double-check, hence no usage of ERR_NON_STANDARD_PUBKEY - return true; + return {true, MSG_NOERR}; }; CAmount nFees{0}; for (const auto& txout : vout) { - if (!checkTxOut(txout)) { - return false; + auto [success, poolMessage] = checkTxOut(txout); + if (!success) { + return {false, true, poolMessage}; } nFees -= txout.nValue; } @@ -266,21 +256,19 @@ bool CCoinJoinBaseSession::IsValidInOuts(const std::vector& vin, const st if (txin.prevout.IsNull()) { LogPrint(BCLog::COINJOIN, "CCoinJoinBaseSession::%s -- ERROR: invalid input!\n", __func__); - nMessageIDRet = ERR_INVALID_INPUT; - if (fConsumeCollateralRet) *fConsumeCollateralRet = true; - return false; + return {false, true, ERR_INVALID_INPUT}; } Coin coin; if (!viewMemPool.GetCoin(txin.prevout, coin) || coin.IsSpent() || (coin.nHeight == MEMPOOL_HEIGHT && !llmq::quorumInstantSendManager->IsLocked(txin.prevout.hash))) { LogPrint(BCLog::COINJOIN, "CCoinJoinBaseSession::%s -- ERROR: missing, spent or non-locked mempool input! txin=%s\n", __func__, txin.ToString()); - nMessageIDRet = ERR_MISSING_TX; - return false; + return {false, false, ERR_MISSING_TX}; } - if (!checkTxOut(coin.out)) { - return false; + auto [success, poolMessage] = checkTxOut(coin.out); + if (!success) { + return {false, true, poolMessage}; } nFees += coin.out.nValue; @@ -290,11 +278,10 @@ bool CCoinJoinBaseSession::IsValidInOuts(const std::vector& vin, const st // no need to double-check. If not, we are doing something wrong, bail out. if (nFees != 0) { LogPrint(BCLog::COINJOIN, "CCoinJoinBaseSession::%s -- ERROR: non-zero fees! fees: %lld\n", __func__, nFees); - nMessageIDRet = ERR_FEES; - return false; + return {false, false, ERR_FEES}; } - return true; + return {false, false, MSG_NOERR}; } // Definitions for static data members @@ -457,9 +444,7 @@ CAmount CCoinJoin::DenominationToAmount(int nDenom) */ std::string CCoinJoin::DenominationToString(int nDenom) { - CAmount nDenomAmount = DenominationToAmount(nDenom); - - switch (nDenomAmount) { + switch (CAmount nDenomAmount = DenominationToAmount(nDenom)) { case 0: return "N/A"; case -1: return "out-of-bounds"; case -2: return "non-denom"; diff --git a/src/coinjoin/coinjoin.h b/src/coinjoin/coinjoin.h index 1f9bb07a27b0b..bdc2521c8642f 100644 --- a/src/coinjoin/coinjoin.h +++ b/src/coinjoin/coinjoin.h @@ -240,7 +240,7 @@ class CCoinJoinQueue } } - uint256 GetSignatureHash() const; + [[nodiscard]] uint256 GetSignatureHash() const; /** Sign this mixing transaction * return true if all conditions are met: * 1) we have an active Masternode, @@ -250,14 +250,14 @@ class CCoinJoinQueue */ bool Sign(); /// Check if we have a valid Masternode address - bool CheckSignature(const CBLSPublicKey& blsPubKey) const; + [[nodiscard]] bool CheckSignature(const CBLSPublicKey& blsPubKey) const; bool Relay(CConnman& connman); /// Check if a queue is too old or too far into the future - bool IsTimeOutOfBounds() const; + [[nodiscard]] bool IsTimeOutOfBounds() const; - std::string ToString() const + [[nodiscard]] std::string ToString() const { return strprintf("nDenom=%d, nTime=%lld, fReady=%s, fTried=%s, masternode=%s", nDenom, nTime, fReady ? "true" : "false", fTried ? "true" : "false", masternodeOutpoint.ToStringShort()); @@ -324,14 +324,14 @@ class CCoinJoinBroadcastTx return *this != CCoinJoinBroadcastTx(); } - uint256 GetSignatureHash() const; + [[nodiscard]] uint256 GetSignatureHash() const; bool Sign(); - bool CheckSignature(const CBLSPublicKey& blsPubKey) const; + [[nodiscard]] bool CheckSignature(const CBLSPublicKey& blsPubKey) const; void SetConfirmedHeight(int nConfirmedHeightIn) { nConfirmedHeight = nConfirmedHeightIn; } bool IsExpired(const CBlockIndex* pindex) const; - bool IsValidStructure() const; + [[nodiscard]] bool IsValidStructure() const; }; // base class @@ -351,7 +351,7 @@ class CCoinJoinBaseSession void SetNull(); - bool IsValidInOuts(const std::vector& vin, const std::vector& vout, PoolMessage& nMessageIDRet, bool* fConsumeCollateralRet) const EXCLUSIVE_LOCKS_REQUIRED(cs_main); + std::tuple IsValidInOuts(const std::vector& vin, const std::vector& vout) const EXCLUSIVE_LOCKS_REQUIRED(cs_main); public: int nSessionDenom; // Users must submit a denom matching this @@ -388,8 +388,8 @@ class CCoinJoinBaseManager CCoinJoinBaseManager() : vecCoinJoinQueue() {} - int GetQueueSize() const { LOCK(cs_vecqueue); return vecCoinJoinQueue.size(); } - bool GetQueueItemAndTry(CCoinJoinQueue& dsqRet); + uint64_t GetQueueSize() const { LOCK(cs_vecqueue); return vecCoinJoinQueue.size(); } + std::optional GetQueueItemAndTry(); }; // helper class diff --git a/src/coinjoin/server.cpp b/src/coinjoin/server.cpp index 419a0487649da..af589820530a8 100644 --- a/src/coinjoin/server.cpp +++ b/src/coinjoin/server.cpp @@ -103,10 +103,8 @@ void CCoinJoinServer::ProcessDSACCEPT(CNode* pfrom, const std::string& strComman } } - PoolMessage nMessageID = MSG_NOERR; - - bool fResult = nSessionID == 0 ? CreateNewSession(dsa, nMessageID, connman) - : AddUserToExistingSession(dsa, nMessageID); + auto [fResult, nMessageID] = nSessionID == 0 ? CreateNewSession(dsa, connman) + : AddUserToExistingSession(dsa); if (fResult) { LogPrint(BCLog::COINJOIN, "DSACCEPT -- is compatible, please submit!\n"); PushStatus(pfrom, STATUS_ACCEPTED, nMessageID, connman); @@ -209,10 +207,10 @@ void CCoinJoinServer::ProcessDSVIN(CNode* pfrom, const std::string& strCommand, LogPrint(BCLog::COINJOIN, "DSVIN -- txCollateral %s", entry.txCollateral->ToString()); /* Continued */ - PoolMessage nMessageID = MSG_NOERR; entry.addr = pfrom->addr; - if (AddEntry(connman, entry, nMessageID)) { + auto [success, nMessageID] = AddEntry(connman, entry); + if (success) { PushStatus(pfrom, STATUS_ACCEPTED, nMessageID, connman); CheckPool(connman); RelayStatus(STATUS_ACCEPTED, connman); @@ -579,27 +577,24 @@ bool CCoinJoinServer::IsInputScriptSigValid(const CTxIn& txin) const // // Add a client's transaction inputs/outputs to the pool // -bool CCoinJoinServer::AddEntry(CConnman& connman, const CCoinJoinEntry& entry, PoolMessage& nMessageIDRet) +std::pair CCoinJoinServer::AddEntry(CConnman& connman, const CCoinJoinEntry& entry) { - if (!fMasternodeMode) return false; + if (!fMasternodeMode) return {false, MSG_NOERR}; if (GetEntriesCount() >= vecSessionCollaterals.size()) { LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- ERROR: entries is full!\n", __func__); - nMessageIDRet = ERR_ENTRIES_FULL; - return false; + return {false, ERR_ENTRIES_FULL}; } if (!CCoinJoin::IsCollateralValid(*entry.txCollateral)) { LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- ERROR: collateral not valid!\n", __func__); - nMessageIDRet = ERR_INVALID_COLLATERAL; - return false; + return {false, ERR_INVALID_COLLATERAL}; } if (entry.vecTxDSIn.size() > COINJOIN_ENTRY_MAX_SIZE) { LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- ERROR: too many inputs! %d/%d\n", __func__, entry.vecTxDSIn.size(), COINJOIN_ENTRY_MAX_SIZE); - nMessageIDRet = ERR_MAXIMUM; ConsumeCollateral(connman, entry.txCollateral); - return false; + return {false, ERR_MAXIMUM}; } std::vector vin; @@ -610,11 +605,10 @@ bool CCoinJoinServer::AddEntry(CConnman& connman, const CCoinJoinEntry& entry, P for (const auto& txdsin : inner_entry.vecTxDSIn) { if (txdsin.prevout == txin.prevout) { LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- ERROR: already have this txin in entries\n", __func__); - nMessageIDRet = ERR_ALREADY_HAVE; // Two peers sent the same input? Can't really say who is the malicious one here, // could be that someone is picking someone else's inputs randomly trying to force // collateral consumption. Do not punish. - return false; + return {false, ERR_ALREADY_HAVE}; } } } @@ -623,21 +617,20 @@ bool CCoinJoinServer::AddEntry(CConnman& connman, const CCoinJoinEntry& entry, P LOCK(cs_main); - bool fConsumeCollateral{false}; - if (!IsValidInOuts(vin, entry.vecTxOut, nMessageIDRet, &fConsumeCollateral)) { - LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- ERROR! IsValidInOuts() failed: %s\n", __func__, CCoinJoin::GetMessageByID(nMessageIDRet)); - if (fConsumeCollateral) { + auto [success, consumeCollateral, nMessageID] = IsValidInOuts(vin, entry.vecTxOut); + if (!success) { + LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- ERROR! IsValidInOuts() failed: %s\n", __func__, CCoinJoin::GetMessageByID(nMessageID)); + if (consumeCollateral) { ConsumeCollateral(connman, entry.txCollateral); } - return false; + return {false, nMessageID}; } WITH_LOCK(cs_coinjoin, vecEntries.push_back(entry)); LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- adding entry %d of %d required\n", __func__, GetEntriesCount(), CCoinJoin::GetMaxPoolParticipants()); - nMessageIDRet = MSG_ENTRIES_ADDED; - return true; + return {true, MSG_ENTRIES_ADDED}; } bool CCoinJoinServer::AddScriptSig(const CTxIn& txinNew) @@ -691,44 +684,38 @@ bool CCoinJoinServer::IsSignaturesComplete() const return true; } -bool CCoinJoinServer::IsAcceptableDSA(const CCoinJoinAccept& dsa, PoolMessage& nMessageIDRet) const +std::pair CCoinJoinServer::IsAcceptableDSA(const CCoinJoinAccept& dsa) const { - if (!fMasternodeMode) return false; - // is denom even something legit? if (!CCoinJoin::IsValidDenomination(dsa.nDenom)) { LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- denom not valid!\n", __func__); - nMessageIDRet = ERR_DENOM; - return false; + return {false, ERR_DENOM}; } // check collateral if (!fUnitTest && !CCoinJoin::IsCollateralValid(CTransaction(dsa.txCollateral))) { LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- collateral not valid!\n", __func__); - nMessageIDRet = ERR_INVALID_COLLATERAL; - return false; + return {false, ERR_INVALID_COLLATERAL}; } - return true; + return {true, MSG_NOERR}; } -bool CCoinJoinServer::CreateNewSession(const CCoinJoinAccept& dsa, PoolMessage& nMessageIDRet, CConnman& connman) +std::pair CCoinJoinServer::CreateNewSession(const CCoinJoinAccept& dsa, CConnman& connman) { - if (!fMasternodeMode || nSessionID != 0) return false; + if (!fMasternodeMode || nSessionID != 0) return {false, MSG_NOERR}; // THIS WAS UNDEF // new session can only be started in idle mode if (nState != POOL_STATE_IDLE) { - nMessageIDRet = ERR_MODE; LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CreateNewSession -- incompatible mode: nState=%d\n", nState); - return false; + return {false, ERR_MODE}; } - if (!IsAcceptableDSA(dsa, nMessageIDRet)) { - return false; + if (auto [success, messageIdError] = IsAcceptableDSA(dsa); !success) { + return {false, messageIdError}; } // start new session - nMessageIDRet = MSG_NOERR; nSessionID = GetRandInt(999999) + 1; nSessionDenom = dsa.nDenom; @@ -748,40 +735,37 @@ bool CCoinJoinServer::CreateNewSession(const CCoinJoinAccept& dsa, PoolMessage& LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CreateNewSession -- new session created, nSessionID: %d nSessionDenom: %d (%s) vecSessionCollaterals.size(): %d CCoinJoin::GetMaxPoolParticipants(): %d\n", nSessionID, nSessionDenom, CCoinJoin::DenominationToString(nSessionDenom), vecSessionCollaterals.size(), CCoinJoin::GetMaxPoolParticipants()); - return true; + return {true, MSG_NOERR}; } -bool CCoinJoinServer::AddUserToExistingSession(const CCoinJoinAccept& dsa, PoolMessage& nMessageIDRet) +std::pair CCoinJoinServer::AddUserToExistingSession(const CCoinJoinAccept& dsa) { - if (!fMasternodeMode || nSessionID == 0 || IsSessionReady()) return false; + if (!fMasternodeMode || nSessionID == 0 || IsSessionReady()) return {false, MSG_NOERR}; // WAS UNDEFINED - if (!IsAcceptableDSA(dsa, nMessageIDRet)) { - return false; + if (auto [success, messageIdError] = IsAcceptableDSA(dsa); !success) { + return {false, messageIdError}; } // we only add new users to an existing session when we are in queue mode if (nState != POOL_STATE_QUEUE) { - nMessageIDRet = ERR_MODE; LogPrint(BCLog::COINJOIN, "CCoinJoinServer::AddUserToExistingSession -- incompatible mode: nState=%d\n", nState); - return false; + return {false, ERR_MODE}; } if (dsa.nDenom != nSessionDenom) { LogPrint(BCLog::COINJOIN, "CCoinJoinServer::AddUserToExistingSession -- incompatible denom %d (%s) != nSessionDenom %d (%s)\n", dsa.nDenom, CCoinJoin::DenominationToString(dsa.nDenom), nSessionDenom, CCoinJoin::DenominationToString(nSessionDenom)); - nMessageIDRet = ERR_DENOM; - return false; + return {false, ERR_DENOM}; } // count new user as accepted to an existing session - nMessageIDRet = MSG_NOERR; vecSessionCollaterals.push_back(MakeTransactionRef(dsa.txCollateral)); LogPrint(BCLog::COINJOIN, "CCoinJoinServer::AddUserToExistingSession -- new user accepted, nSessionID: %d nSessionDenom: %d (%s) vecSessionCollaterals.size(): %d CCoinJoin::GetMaxPoolParticipants(): %d\n", nSessionID, nSessionDenom, CCoinJoin::DenominationToString(nSessionDenom), vecSessionCollaterals.size(), CCoinJoin::GetMaxPoolParticipants()); - return true; + return {true, MSG_NOERR}; } // Returns true if either max size has been reached or if the mix timed out and min size was reached diff --git a/src/coinjoin/server.h b/src/coinjoin/server.h index f9fb346d4b1a8..1abd92b059374 100644 --- a/src/coinjoin/server.h +++ b/src/coinjoin/server.h @@ -26,7 +26,7 @@ class CCoinJoinServer : public CCoinJoinBaseSession, public CCoinJoinBaseManager bool fUnitTest; /// Add a clients entry to the pool - bool AddEntry(CConnman& connman, const CCoinJoinEntry& entry, PoolMessage& nMessageIDRet); + std::pair AddEntry(CConnman& connman, const CCoinJoinEntry& entry); /// Add signature to a txin bool AddScriptSig(const CTxIn& txin); @@ -43,10 +43,14 @@ class CCoinJoinServer : public CCoinJoinBaseSession, public CCoinJoinBaseManager void CreateFinalTransaction(CConnman& connman); void CommitFinalTransaction(CConnman& connman); - /// Is this nDenom and txCollateral acceptable? - bool IsAcceptableDSA(const CCoinJoinAccept& dsa, PoolMessage& nMessageIDRet) const; - bool CreateNewSession(const CCoinJoinAccept& dsa, PoolMessage& nMessageIDRet, CConnman& connman); - bool AddUserToExistingSession(const CCoinJoinAccept& dsa, PoolMessage& nMessageIDRet); + /** + * Is this nDenom and txCollateral acceptable? + * @param dsa input to check validity on + * @return PoolMessage if there was an error or not + */ + std::pair IsAcceptableDSA(const CCoinJoinAccept& dsa) const; + std::pair CreateNewSession(const CCoinJoinAccept& dsa, CConnman& connman); + std::pair AddUserToExistingSession(const CCoinJoinAccept& dsa); /// Do we have enough users to take entries? bool IsSessionReady() const; diff --git a/src/evo/cbtx.cpp b/src/evo/cbtx.cpp index e001f4d2b53e0..f5c3d6a9590e2 100644 --- a/src/evo/cbtx.cpp +++ b/src/evo/cbtx.cpp @@ -70,12 +70,12 @@ bool CheckCbTxMerkleRoots(const CBlock& block, const CBlockIndex* pindex, CValid static int64_t nTimeMerkleMNL = 0; static int64_t nTimeMerkleQuorum = 0; - uint256 calculatedMerkleRoot; - if (!CalcCbTxMerkleRootMNList(block, pindex->pprev, calculatedMerkleRoot, state, view)) { + auto optCalculatedMerkleRootList = CalcCbTxMerkleRootMNList(block, pindex->pprev, state, view); + if (!optCalculatedMerkleRootList) { // pass the state returned by the function above return false; } - if (calculatedMerkleRoot != cbTx.merkleRootMNList) { + if (*optCalculatedMerkleRootList != cbTx.merkleRootMNList) { return state.DoS(100, false, REJECT_INVALID, "bad-cbtx-mnmerkleroot"); } @@ -83,11 +83,12 @@ bool CheckCbTxMerkleRoots(const CBlock& block, const CBlockIndex* pindex, CValid LogPrint(BCLog::BENCHMARK, " - CalcCbTxMerkleRootMNList: %.2fms [%.2fs]\n", 0.001 * (nTime3 - nTime2), nTimeMerkleMNL * 0.000001); if (cbTx.nVersion >= 2) { - if (!CalcCbTxMerkleRootQuorums(block, pindex->pprev, calculatedMerkleRoot, state)) { + auto optCalculatedMerkleRoot = CalcCbTxMerkleRootQuorums(block, pindex->pprev, state); + if (!optCalculatedMerkleRoot) { // pass the state returned by the function above return false; } - if (calculatedMerkleRoot != cbTx.merkleRootQuorums) { + if (*optCalculatedMerkleRoot != cbTx.merkleRootQuorums) { return state.DoS(100, false, REJECT_INVALID, "bad-cbtx-quorummerkleroot"); } } @@ -100,7 +101,7 @@ bool CheckCbTxMerkleRoots(const CBlock& block, const CBlockIndex* pindex, CValid return true; } -bool CalcCbTxMerkleRootMNList(const CBlock& block, const CBlockIndex* pindexPrev, uint256& merkleRootRet, CValidationState& state, const CCoinsViewCache& view) +std::optional CalcCbTxMerkleRootMNList(const CBlock& block, const CBlockIndex* pindexPrev, CValidationState& state, const CCoinsViewCache& view) { LOCK(deterministicMNManager->cs); @@ -111,16 +112,16 @@ bool CalcCbTxMerkleRootMNList(const CBlock& block, const CBlockIndex* pindexPrev int64_t nTime1 = GetTimeMicros(); - CDeterministicMNList tmpMNList; - if (!deterministicMNManager->BuildNewListFromBlock(block, pindexPrev, state, view, tmpMNList, false)) { + auto optTmpMNList = deterministicMNManager->BuildNewListFromBlock(block, pindexPrev, state, view, false); + if (!optTmpMNList) { // pass the state returned by the function above - return false; + return std::nullopt; } int64_t nTime2 = GetTimeMicros(); nTimeDMN += nTime2 - nTime1; LogPrint(BCLog::BENCHMARK, " - BuildNewListFromBlock: %.2fms [%.2fs]\n", 0.001 * (nTime2 - nTime1), nTimeDMN * 0.000001); - CSimplifiedMNList sml(tmpMNList); + CSimplifiedMNList sml(*optTmpMNList); int64_t nTime3 = GetTimeMicros(); nTimeSMNL += nTime3 - nTime2; LogPrint(BCLog::BENCHMARK, " - CSimplifiedMNList: %.2fms [%.2fs]\n", 0.001 * (nTime3 - nTime2), nTimeSMNL * 0.000001); @@ -130,15 +131,15 @@ bool CalcCbTxMerkleRootMNList(const CBlock& block, const CBlockIndex* pindexPrev static bool mutatedCached{false}; if (sml.mnList == smlCached.mnList) { - merkleRootRet = merkleRootCached; if (mutatedCached) { - return state.DoS(100, false, REJECT_INVALID, "mutated-cached-calc-cb-mnmerkleroot"); + state.DoS(100, false, REJECT_INVALID, "mutated-cached-calc-cb-mnmerkleroot"); + return std::nullopt; } - return true; + return {merkleRootCached}; } bool mutated = false; - merkleRootRet = sml.CalcMerkleRoot(&mutated); + auto merkleRootRet = sml.CalcMerkleRoot(&mutated); int64_t nTime4 = GetTimeMicros(); nTimeMerkle += nTime4 - nTime3; LogPrint(BCLog::BENCHMARK, " - CalcMerkleRoot: %.2fms [%.2fs]\n", 0.001 * (nTime4 - nTime3), nTimeMerkle * 0.000001); @@ -148,17 +149,19 @@ bool CalcCbTxMerkleRootMNList(const CBlock& block, const CBlockIndex* pindexPrev mutatedCached = mutated; if (mutated) { - return state.DoS(100, false, REJECT_INVALID, "mutated-calc-cb-mnmerkleroot"); + state.DoS(100, false, REJECT_INVALID, "mutated-calc-cb-mnmerkleroot"); + return std::nullopt; } - return true; + return {merkleRootRet}; } catch (const std::exception& e) { LogPrintf("%s -- failed: %s\n", __func__, e.what()); - return state.DoS(100, false, REJECT_INVALID, "failed-calc-cb-mnmerkleroot"); + state.DoS(100, false, REJECT_INVALID, "failed-calc-cb-mnmerkleroot"); + return std::nullopt; } } -bool CalcCbTxMerkleRootQuorums(const CBlock& block, const CBlockIndex* pindexPrev, uint256& merkleRootRet, CValidationState& state) +std::optional CalcCbTxMerkleRootQuorums(const CBlock& block, const CBlockIndex* pindexPrev, CValidationState& state) { static int64_t nTimeMinedAndActive = 0; static int64_t nTimeMined = 0; @@ -186,9 +189,12 @@ bool CalcCbTxMerkleRootQuorums(const CBlock& block, const CBlockIndex* pindexPre v.reserve(p.second.size()); for (const auto& p2 : p.second) { uint256 minedBlockHash; - llmq::CFinalCommitmentPtr qc = llmq::quorumBlockProcessor->GetMinedCommitment(p.first, p2->GetBlockHash(), minedBlockHash); - if (qc == nullptr) return state.DoS(100, false, REJECT_INVALID, "commitment-not-found"); - v.emplace_back(::SerializeHash(*qc)); + auto optQuorumCommitBlockhash = llmq::quorumBlockProcessor->GetMinedCommitment(p.first, p2->GetBlockHash()); + if (!optQuorumCommitBlockhash) { + state.DoS(100, false, REJECT_INVALID, "commitment-not-found"); + return std::nullopt; + } + v.emplace_back(::SerializeHash(optQuorumCommitBlockhash->first.get())); hashCount++; } } @@ -207,7 +213,8 @@ bool CalcCbTxMerkleRootQuorums(const CBlock& block, const CBlockIndex* pindexPre if (tx->nVersion == 3 && tx->nType == TRANSACTION_QUORUM_COMMITMENT) { llmq::CFinalCommitmentTxPayload qc; if (!GetTxPayload(*tx, qc)) { - return state.DoS(100, false, REJECT_INVALID, "bad-qc-payload-calc-cbtx-quorummerkleroot"); + state.DoS(100, false, REJECT_INVALID, "bad-qc-payload-calc-cbtx-quorummerkleroot"); + return std::nullopt; } if (qc.commitment.IsNull()) { continue; @@ -224,7 +231,8 @@ bool CalcCbTxMerkleRootQuorums(const CBlock& block, const CBlockIndex* pindexPre v.emplace_back(qcHash); hashCount++; if (v.size() > llmq_params.signingActiveQuorumCount) { - return state.DoS(100, false, REJECT_INVALID, "excess-quorums-calc-cbtx-quorummerkleroot"); + state.DoS(100, false, REJECT_INVALID, "excess-quorums-calc-cbtx-quorummerkleroot"); + return std::nullopt; } } } @@ -242,16 +250,17 @@ bool CalcCbTxMerkleRootQuorums(const CBlock& block, const CBlockIndex* pindexPre LogPrint(BCLog::BENCHMARK, " - Loop: %.2fms [%.2fs]\n", 0.001 * (nTime4 - nTime3), nTimeLoop * 0.000001); bool mutated = false; - merkleRootRet = ComputeMerkleRoot(qcHashesVec, &mutated); + auto merkleRootRet = ComputeMerkleRoot(qcHashesVec, &mutated); int64_t nTime5 = GetTimeMicros(); nTimeMerkle += nTime5 - nTime4; LogPrint(BCLog::BENCHMARK, " - ComputeMerkleRoot: %.2fms [%.2fs]\n", 0.001 * (nTime5 - nTime4), nTimeMerkle * 0.000001); if (mutated) { - return state.DoS(100, false, REJECT_INVALID, "mutated-calc-cbtx-quorummerkleroot"); + state.DoS(100, false, REJECT_INVALID, "mutated-calc-cbtx-quorummerkleroot"); + return std::nullopt; } - return true; + return {merkleRootRet}; } std::string CCbTx::ToString() const diff --git a/src/evo/cbtx.h b/src/evo/cbtx.h index d7efec4d2dbab..e542175c26c04 100644 --- a/src/evo/cbtx.h +++ b/src/evo/cbtx.h @@ -51,7 +51,7 @@ class CCbTx bool CheckCbTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValidationState& state); bool CheckCbTxMerkleRoots(const CBlock& block, const CBlockIndex* pindex, CValidationState& state, const CCoinsViewCache& view); -bool CalcCbTxMerkleRootMNList(const CBlock& block, const CBlockIndex* pindexPrev, uint256& merkleRootRet, CValidationState& state, const CCoinsViewCache& view); -bool CalcCbTxMerkleRootQuorums(const CBlock& block, const CBlockIndex* pindexPrev, uint256& merkleRootRet, CValidationState& state); +std::optional CalcCbTxMerkleRootMNList(const CBlock& block, const CBlockIndex* pindexPrev, CValidationState& state, const CCoinsViewCache& view); +std::optional CalcCbTxMerkleRootQuorums(const CBlock& block, const CBlockIndex* pindexPrev, CValidationState& state); #endif // BITCOIN_EVO_CBTX_H diff --git a/src/evo/deterministicmns.cpp b/src/evo/deterministicmns.cpp index f1f649507e6b2..eabb0ed20dc1f 100644 --- a/src/evo/deterministicmns.cpp +++ b/src/evo/deterministicmns.cpp @@ -590,10 +590,12 @@ bool CDeterministicMNManager::ProcessBlock(const CBlock& block, const CBlockInde try { LOCK(cs); - if (!BuildNewListFromBlock(block, pindex->pprev, _state, view, newList, true)) { + auto optNewList = BuildNewListFromBlock(block, pindex->pprev, _state, view, true); + if (!optNewList) { // pass the state returned by the function above return false; } + newList = *optNewList; if (fJustCheck) { return true; @@ -687,7 +689,7 @@ void CDeterministicMNManager::UpdatedBlockTip(const CBlockIndex* pindex) tipIndex = pindex; } -bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, const CBlockIndex* pindexPrev, CValidationState& _state, const CCoinsViewCache& view, CDeterministicMNList& mnListRet, bool debugLogs) +std::optional CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, const CBlockIndex* pindexPrev, CValidationState& _state, const CCoinsViewCache& view, bool debugLogs) { AssertLockHeld(cs); @@ -732,7 +734,8 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, const C if (tx.nType == TRANSACTION_PROVIDER_REGISTER) { CProRegTx proTx; if (!GetTxPayload(tx, proTx)) { - return _state.DoS(100, false, REJECT_INVALID, "bad-protx-payload"); + _state.DoS(100, false, REJECT_INVALID, "bad-protx-payload"); + return std::nullopt; } auto dmn = std::make_shared(newList.GetTotalRegisteredCount()); @@ -749,7 +752,8 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, const C if (!proTx.collateralOutpoint.hash.IsNull() && (!view.GetCoin(dmn->collateralOutpoint, coin) || coin.IsSpent() || coin.out.nValue != 1000 * COIN)) { // should actually never get to this point as CheckProRegTx should have handled this case. // We do this additional check nevertheless to be 100% sure - return _state.DoS(100, false, REJECT_INVALID, "bad-protx-collateral"); + _state.DoS(100, false, REJECT_INVALID, "bad-protx-collateral"); + return std::nullopt; } auto replacedDmn = newList.GetMNByCollateral(dmn->collateralOutpoint); @@ -765,10 +769,12 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, const C } if (newList.HasUniqueProperty(proTx.addr)) { - return _state.DoS(100, false, REJECT_DUPLICATE, "bad-protx-dup-addr"); + _state.DoS(100, false, REJECT_DUPLICATE, "bad-protx-dup-addr"); + return std::nullopt; } if (newList.HasUniqueProperty(proTx.keyIDOwner) || newList.HasUniqueProperty(proTx.pubKeyOperator)) { - return _state.DoS(100, false, REJECT_DUPLICATE, "bad-protx-dup-key"); + _state.DoS(100, false, REJECT_DUPLICATE, "bad-protx-dup-key"); + return std::nullopt; } dmn->nOperatorReward = proTx.nOperatorReward; @@ -790,16 +796,19 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, const C } else if (tx.nType == TRANSACTION_PROVIDER_UPDATE_SERVICE) { CProUpServTx proTx; if (!GetTxPayload(tx, proTx)) { - return _state.DoS(100, false, REJECT_INVALID, "bad-protx-payload"); + _state.DoS(100, false, REJECT_INVALID, "bad-protx-payload"); + return std::nullopt; } if (newList.HasUniqueProperty(proTx.addr) && newList.GetUniquePropertyMN(proTx.addr)->proTxHash != proTx.proTxHash) { - return _state.DoS(100, false, REJECT_DUPLICATE, "bad-protx-dup-addr"); + _state.DoS(100, false, REJECT_DUPLICATE, "bad-protx-dup-addr"); + return std::nullopt; } CDeterministicMNCPtr dmn = newList.GetMN(proTx.proTxHash); if (!dmn) { - return _state.DoS(100, false, REJECT_INVALID, "bad-protx-hash"); + _state.DoS(100, false, REJECT_INVALID, "bad-protx-hash"); + return std::nullopt; } auto newState = std::make_shared(*dmn->pdmnState); newState->addr = proTx.addr; @@ -824,12 +833,14 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, const C } else if (tx.nType == TRANSACTION_PROVIDER_UPDATE_REGISTRAR) { CProUpRegTx proTx; if (!GetTxPayload(tx, proTx)) { - return _state.DoS(100, false, REJECT_INVALID, "bad-protx-payload"); + _state.DoS(100, false, REJECT_INVALID, "bad-protx-payload"); + return std::nullopt; } CDeterministicMNCPtr dmn = newList.GetMN(proTx.proTxHash); if (!dmn) { - return _state.DoS(100, false, REJECT_INVALID, "bad-protx-hash"); + _state.DoS(100, false, REJECT_INVALID, "bad-protx-hash"); + return std::nullopt; } auto newState = std::make_shared(*dmn->pdmnState); if (newState->pubKeyOperator.Get() != proTx.pubKeyOperator) { @@ -850,12 +861,14 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, const C } else if (tx.nType == TRANSACTION_PROVIDER_UPDATE_REVOKE) { CProUpRevTx proTx; if (!GetTxPayload(tx, proTx)) { - return _state.DoS(100, false, REJECT_INVALID, "bad-protx-payload"); + _state.DoS(100, false, REJECT_INVALID, "bad-protx-payload"); + return std::nullopt; } CDeterministicMNCPtr dmn = newList.GetMN(proTx.proTxHash); if (!dmn) { - return _state.DoS(100, false, REJECT_INVALID, "bad-protx-hash"); + _state.DoS(100, false, REJECT_INVALID, "bad-protx-hash"); + return std::nullopt; } auto newState = std::make_shared(*dmn->pdmnState); newState->ResetOperatorFields(); @@ -871,7 +884,8 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, const C } else if (tx.nType == TRANSACTION_QUORUM_COMMITMENT) { llmq::CFinalCommitmentTxPayload qc; if (!GetTxPayload(tx, qc)) { - return _state.DoS(100, false, REJECT_INVALID, "bad-qc-payload"); + _state.DoS(100, false, REJECT_INVALID, "bad-qc-payload"); + return std::nullopt; } if (!qc.commitment.IsNull()) { const auto& llmq_params = llmq::GetLLMQParams(qc.commitment.llmqType); @@ -879,7 +893,8 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, const C auto pQuorumBaseBlockIndex = pindexPrev->GetAncestor(quorumHeight); if (!pQuorumBaseBlockIndex || pQuorumBaseBlockIndex->GetBlockHash() != qc.commitment.quorumHash) { // we should actually never get into this case as validation should have caught it...but let's be sure - return _state.DoS(100, false, REJECT_INVALID, "bad-qc-quorum-hash"); + _state.DoS(100, false, REJECT_INVALID, "bad-qc-quorum-hash"); + return std::nullopt; } HandleQuorumCommitment(qc.commitment, pQuorumBaseBlockIndex, newList, debugLogs); @@ -913,9 +928,7 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, const C newList.UpdateMN(payee->proTxHash, newState); } - mnListRet = std::move(newList); - - return true; + return {newList}; } void CDeterministicMNManager::HandleQuorumCommitment(const llmq::CFinalCommitment& qc, const CBlockIndex* pQuorumBaseBlockIndex, CDeterministicMNList& mnList, bool debugLogs) diff --git a/src/evo/deterministicmns.h b/src/evo/deterministicmns.h index 120f82b4d1349..8bd72cf29bbf7 100644 --- a/src/evo/deterministicmns.h +++ b/src/evo/deterministicmns.h @@ -686,8 +686,8 @@ class CDeterministicMNManager void UpdatedBlockTip(const CBlockIndex* pindex); // the returned list will not contain the correct block hash (we can't know it yet as the coinbase TX is not updated yet) - bool BuildNewListFromBlock(const CBlock& block, const CBlockIndex* pindexPrev, CValidationState& state, const CCoinsViewCache& view, - CDeterministicMNList& mnListRet, bool debugLogs) EXCLUSIVE_LOCKS_REQUIRED(cs); + std::optional BuildNewListFromBlock(const CBlock& block, const CBlockIndex* pindexPrev, CValidationState& state, const CCoinsViewCache& view, + bool debugLogs) EXCLUSIVE_LOCKS_REQUIRED(cs); static void HandleQuorumCommitment(const llmq::CFinalCommitment& qc, const CBlockIndex* pQuorumBaseBlockIndex, CDeterministicMNList& mnList, bool debugLogs); static void DecreasePoSePenalties(CDeterministicMNList& mnList); diff --git a/src/evo/simplifiedmns.cpp b/src/evo/simplifiedmns.cpp index 91828bb21ab43..ffb121b75c8e5 100644 --- a/src/evo/simplifiedmns.cpp +++ b/src/evo/simplifiedmns.cpp @@ -101,16 +101,16 @@ bool CSimplifiedMNListDiff::BuildQuorumsDiff(const CBlockIndex* baseBlockIndex, auto baseQuorums = llmq::quorumBlockProcessor->GetMinedAndActiveCommitmentsUntilBlock(baseBlockIndex); auto quorums = llmq::quorumBlockProcessor->GetMinedAndActiveCommitmentsUntilBlock(blockIndex); - std::set> baseQuorumHashes; + std::unordered_map baseQuorumHashes; std::set> quorumHashes; - for (const auto& p : baseQuorums) { - for (const auto& p2 : p.second) { - baseQuorumHashes.emplace(p.first, p2->GetBlockHash()); + for (const auto& [llmqType, vecBlockIndex] : baseQuorums) { + for (const auto& blockindex : vecBlockIndex) { + baseQuorumHashes.emplace(llmqType, blockindex->GetBlockHash()); } } - for (const auto& p : quorums) { - for (const auto& p2 : p.second) { - quorumHashes.emplace(p.first, p2->GetBlockHash()); + for (const auto& [llmqType, vecBlockIndex] : quorums) { + for (const auto& blockindex : vecBlockIndex) { + quorumHashes.emplace(llmqType, blockindex->GetBlockHash()); } } @@ -119,14 +119,13 @@ bool CSimplifiedMNListDiff::BuildQuorumsDiff(const CBlockIndex* baseBlockIndex, deletedQuorums.emplace_back((uint8_t)p.first, p.second); } } - for (auto& p : quorumHashes) { - if (!baseQuorumHashes.count(p)) { - uint256 minedBlockHash; - llmq::CFinalCommitmentPtr qc = llmq::quorumBlockProcessor->GetMinedCommitment(p.first, p.second, minedBlockHash); - if (qc == nullptr) { + for (auto& [llmqType, hash] : quorumHashes) { + if (!baseQuorumHashes.count(llmqType)) { + auto optPairCommitmentBlockHash = llmq::quorumBlockProcessor->GetMinedCommitment(llmqType, hash); + if (!optPairCommitmentBlockHash) { return false; } - newQuorums.emplace_back(*qc); + newQuorums.emplace_back(*optPairCommitmentBlockHash->first); } } return true; @@ -185,40 +184,35 @@ void CSimplifiedMNListDiff::ToJson(UniValue& obj) const } } -bool BuildSimplifiedMNListDiff(const uint256& baseBlockHash, const uint256& blockHash, CSimplifiedMNListDiff& mnListDiffRet, std::string& errorRet) +std::pair, std::string> BuildSimplifiedMNListDiff(const uint256& baseBlockHash, const uint256& blockHash) { AssertLockHeld(cs_main); - mnListDiffRet = CSimplifiedMNListDiff(); const CBlockIndex* baseBlockIndex = ::ChainActive().Genesis(); if (!baseBlockHash.IsNull()) { baseBlockIndex = LookupBlockIndex(baseBlockHash); if (!baseBlockIndex) { - errorRet = strprintf("block %s not found", baseBlockHash.ToString()); - return false; + return {std::nullopt, strprintf("block %s not found", baseBlockHash.ToString())}; } } const CBlockIndex* blockIndex = LookupBlockIndex(blockHash); if (!blockIndex) { - errorRet = strprintf("block %s not found", blockHash.ToString()); - return false; + return {std::nullopt, strprintf("block %s not found", blockHash.ToString())}; } if (!::ChainActive().Contains(baseBlockIndex) || !::ChainActive().Contains(blockIndex)) { - errorRet = strprintf("block %s and %s are not in the same chain", baseBlockHash.ToString(), blockHash.ToString()); - return false; + return {std::nullopt, strprintf("block %s and %s are not in the same chain", baseBlockHash.ToString(), blockHash.ToString())}; } if (baseBlockIndex->nHeight > blockIndex->nHeight) { - errorRet = strprintf("base block %s is higher then block %s", baseBlockHash.ToString(), blockHash.ToString()); - return false; + return {std::nullopt, strprintf("base block %s is higher then block %s", baseBlockHash.ToString(), blockHash.ToString())}; } LOCK(deterministicMNManager->cs); auto baseDmnList = deterministicMNManager->GetListForBlock(baseBlockIndex); auto dmnList = deterministicMNManager->GetListForBlock(blockIndex); - mnListDiffRet = baseDmnList.BuildSimplifiedDiff(dmnList); + auto mnListDiffRet = baseDmnList.BuildSimplifiedDiff(dmnList); // We need to return the value that was provided by the other peer as it otherwise won't be able to recognize the // response. This will usually be identical to the block found in baseBlockIndex. The only difference is when a @@ -226,15 +220,13 @@ bool BuildSimplifiedMNListDiff(const uint256& baseBlockHash, const uint256& bloc mnListDiffRet.baseBlockHash = baseBlockHash; if (!mnListDiffRet.BuildQuorumsDiff(baseBlockIndex, blockIndex)) { - errorRet = strprintf("failed to build quorums diff"); - return false; + return {std::nullopt, "failed to build quorums diff"}; } // TODO store coinbase TX in CBlockIndex CBlock block; if (!ReadBlockFromDisk(block, blockIndex, Params().GetConsensus())) { - errorRet = strprintf("failed to read block %s from disk", blockHash.ToString()); - return false; + return {std::nullopt, strprintf("failed to read block %s from disk", blockHash.ToString())}; } mnListDiffRet.cbTx = block.vtx[0]; @@ -247,5 +239,5 @@ bool BuildSimplifiedMNListDiff(const uint256& baseBlockHash, const uint256& bloc vMatch[0] = true; // only coinbase matches mnListDiffRet.cbTxMerkleTree = CPartialMerkleTree(vHashes, vMatch); - return true; + return {mnListDiffRet, ""}; } diff --git a/src/evo/simplifiedmns.h b/src/evo/simplifiedmns.h index b604e39b28211..7c8637339d38a 100644 --- a/src/evo/simplifiedmns.h +++ b/src/evo/simplifiedmns.h @@ -123,6 +123,6 @@ class CSimplifiedMNListDiff void ToJson(UniValue& obj) const; }; -bool BuildSimplifiedMNListDiff(const uint256& baseBlockHash, const uint256& blockHash, CSimplifiedMNListDiff& mnListDiffRet, std::string& errorRet); +std::pair, std::string> BuildSimplifiedMNListDiff(const uint256& baseBlockHash, const uint256& blockHash); #endif // BITCOIN_EVO_SIMPLIFIEDMNS_H diff --git a/src/llmq/blockprocessor.cpp b/src/llmq/blockprocessor.cpp index c1831cec55685..b3e9486572f83 100644 --- a/src/llmq/blockprocessor.cpp +++ b/src/llmq/blockprocessor.cpp @@ -132,8 +132,8 @@ bool CQuorumBlockProcessor::ProcessBlock(const CBlock& block, const CBlockIndex* return true; } - std::map qcs; - if (!GetCommitmentsFromBlock(block, pindex, qcs, state)) { + auto qcs = GetCommitmentsFromBlock(block, pindex, state); + if (!qcs) { return false; } @@ -148,7 +148,7 @@ bool CQuorumBlockProcessor::ProcessBlock(const CBlock& block, const CBlockIndex* } // does the currently processed block contain a (possibly null) commitment for the current session? - bool hasCommitmentInNewBlock = qcs.count(params.type) != 0; + bool hasCommitmentInNewBlock = qcs.value().count(params.type) != 0; bool isCommitmentRequired = IsCommitmentRequired(params, pindex->nHeight); if (hasCommitmentInNewBlock && !isCommitmentRequired) { @@ -165,8 +165,7 @@ bool CQuorumBlockProcessor::ProcessBlock(const CBlock& block, const CBlockIndex* auto blockHash = block.GetHash(); - for (const auto& p : qcs) { - const auto& qc = p.second; + for (const auto& [_, qc] : *qcs) { if (!ProcessCommitment(pindex->nHeight, blockHash, qc, state, fJustCheck)) { return false; } @@ -254,14 +253,13 @@ bool CQuorumBlockProcessor::UndoBlock(const CBlock& block, const CBlockIndex* pi { AssertLockHeld(cs_main); - std::map qcs; CValidationState dummy; - if (!GetCommitmentsFromBlock(block, pindex, qcs, dummy)) { + auto qcs = GetCommitmentsFromBlock(block, pindex, dummy); + if (!qcs) { return false; } - for (auto& p : qcs) { - auto& qc = p.second; + for (auto& [_, qc] : *qcs) { if (qc.IsNull()) { continue; } @@ -310,18 +308,18 @@ bool CQuorumBlockProcessor::UpgradeDB() bool r = ReadBlockFromDisk(block, pindex, Params().GetConsensus()); assert(r); - std::map qcs; CValidationState dummyState; - GetCommitmentsFromBlock(block, pindex, qcs, dummyState); - - for (const auto& p : qcs) { - const auto& qc = p.second; - if (qc.IsNull()) { - continue; + if (auto qcs = GetCommitmentsFromBlock(block, pindex, dummyState)) { + for (const auto& [_, qc]: *qcs) { + if (qc.IsNull()) { + continue; + } + auto pQuorumBaseBlockIndex = LookupBlockIndex(qc.quorumHash); + evoDb.GetRawDB().Write( + std::make_pair(DB_MINED_COMMITMENT, std::make_pair(qc.llmqType, qc.quorumHash)), + std::make_pair(qc, pindex->GetBlockHash())); + evoDb.GetRawDB().Write(BuildInversedHeightKey(qc.llmqType, pindex->nHeight), pQuorumBaseBlockIndex->nHeight); } - auto pQuorumBaseBlockIndex = LookupBlockIndex(qc.quorumHash); - evoDb.GetRawDB().Write(std::make_pair(DB_MINED_COMMITMENT, std::make_pair(qc.llmqType, qc.quorumHash)), std::make_pair(qc, pindex->GetBlockHash())); - evoDb.GetRawDB().Write(BuildInversedHeightKey(qc.llmqType, pindex->nHeight), pQuorumBaseBlockIndex->nHeight); } evoDb.GetRawDB().Write(DB_BEST_BLOCK_UPGRADE, pindex->GetBlockHash()); @@ -334,26 +332,28 @@ bool CQuorumBlockProcessor::UpgradeDB() return true; } -bool CQuorumBlockProcessor::GetCommitmentsFromBlock(const CBlock& block, const CBlockIndex* pindex, std::map& ret, CValidationState& state) +std::optional> CQuorumBlockProcessor::GetCommitmentsFromBlock(const CBlock& block, const CBlockIndex* pindex, CValidationState& state) { AssertLockHeld(cs_main); const auto& consensus = Params().GetConsensus(); bool fDIP0003Active = pindex->nHeight >= consensus.DIP0003Height; - ret.clear(); + std::map ret; for (const auto& tx : block.vtx) { if (tx->nType == TRANSACTION_QUORUM_COMMITMENT) { CFinalCommitmentTxPayload qc; if (!GetTxPayload(*tx, qc)) { // should not happen as it was verified before processing the block - return state.DoS(100, false, REJECT_INVALID, "bad-qc-payload"); + state.DoS(100, false, REJECT_INVALID, "bad-qc-payload"); + return std::nullopt; } // only allow one commitment per type and per block if (ret.count(qc.commitment.llmqType)) { - return state.DoS(100, false, REJECT_INVALID, "bad-qc-dup"); + state.DoS(100, false, REJECT_INVALID, "bad-qc-dup"); + return std::nullopt; } ret.emplace(qc.commitment.llmqType, std::move(qc.commitment)); @@ -361,10 +361,11 @@ bool CQuorumBlockProcessor::GetCommitmentsFromBlock(const CBlock& block, const C } if (!fDIP0003Active && !ret.empty()) { - return state.DoS(100, false, REJECT_INVALID, "bad-qc-premature"); + state.DoS(100, false, REJECT_INVALID, "bad-qc-premature"); + return std::nullopt; } - return true; + return {ret}; } bool CQuorumBlockProcessor::IsMiningPhase(const Consensus::LLMQParams& llmqParams, int nHeight) @@ -423,15 +424,14 @@ 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) const { auto key = std::make_pair(DB_MINED_COMMITMENT, std::make_pair(llmqType, quorumHash)); std::pair p; if (!evoDb.Read(key, p)) { - return nullptr; + return std::nullopt; } - retMinedBlockHash = p.second; - return std::make_unique(p.first); + return {{std::make_unique(p.first), p.second}}; } // The returned quorums are in reversed order, so the most recent one is at index 0 @@ -531,31 +531,30 @@ void CQuorumBlockProcessor::AddMineableCommitment(const CFinalCommitment& fqc) } } -bool CQuorumBlockProcessor::GetMineableCommitmentByHash(const uint256& commitmentHash, llmq::CFinalCommitment& ret) const +std::optional CQuorumBlockProcessor::GetMineableCommitmentByHash(const uint256& commitmentHash) const { LOCK(minableCommitmentsCs); auto it = minableCommitments.find(commitmentHash); if (it == minableCommitments.end()) { - return false; + return std::nullopt; } - ret = it->second; - return true; + return {it->second}; } // Will return false if no commitment should be mined // Will return true and a null commitment if no mineable commitment is known and none was mined yet -bool CQuorumBlockProcessor::GetMineableCommitment(const Consensus::LLMQParams& llmqParams, int nHeight, CFinalCommitment& ret) const +std::optional CQuorumBlockProcessor::GetMineableCommitment(const Consensus::LLMQParams& llmqParams, int nHeight) const { AssertLockHeld(cs_main); if (!IsCommitmentRequired(llmqParams, nHeight)) { // no commitment required - return false; + return std::nullopt; } uint256 quorumHash = GetQuorumBlockHash(llmqParams, nHeight); if (quorumHash.IsNull()) { - return false; + return std::nullopt; } LOCK(minableCommitmentsCs); @@ -564,34 +563,30 @@ bool CQuorumBlockProcessor::GetMineableCommitment(const Consensus::LLMQParams& l auto it = minableCommitmentsByQuorum.find(k); if (it == minableCommitmentsByQuorum.end()) { // null commitment required - ret = CFinalCommitment(llmqParams, quorumHash); - return true; + return {CFinalCommitment(llmqParams, quorumHash)}; } - ret = minableCommitments.at(it->second); - - return true; + return {minableCommitments.at(it->second)}; } -bool CQuorumBlockProcessor::GetMineableCommitmentTx(const Consensus::LLMQParams& llmqParams, int nHeight, CTransactionRef& ret) const +std::optional CQuorumBlockProcessor::GetMineableCommitmentTx(const Consensus::LLMQParams& llmqParams, int nHeight) const { AssertLockHeld(cs_main); CFinalCommitmentTxPayload qc; - if (!GetMineableCommitment(llmqParams, nHeight, qc.commitment)) { - return false; + auto optQcCommit = GetMineableCommitment(llmqParams, nHeight); + if (!optQcCommit) { + return std::nullopt; } + qc.commitment = *optQcCommit; qc.nHeight = nHeight; CMutableTransaction tx; tx.nVersion = 3; tx.nType = TRANSACTION_QUORUM_COMMITMENT; SetTxPayload(tx, qc); - - ret = MakeTransactionRef(tx); - - return true; + return {MakeTransactionRef(tx)}; } } // namespace llmq diff --git a/src/llmq/blockprocessor.h b/src/llmq/blockprocessor.h index 92c9036978703..93d031b873143 100644 --- a/src/llmq/blockprocessor.h +++ b/src/llmq/blockprocessor.h @@ -53,18 +53,19 @@ class CQuorumBlockProcessor void AddMineableCommitment(const CFinalCommitment& fqc); bool HasMineableCommitment(const uint256& hash) const; - bool GetMineableCommitmentByHash(const uint256& commitmentHash, CFinalCommitment& ret) const; - bool GetMineableCommitment(const Consensus::LLMQParams& llmqParams, int nHeight, CFinalCommitment& ret) const EXCLUSIVE_LOCKS_REQUIRED(cs_main); - bool GetMineableCommitmentTx(const Consensus::LLMQParams& llmqParams, int nHeight, CTransactionRef& ret) const EXCLUSIVE_LOCKS_REQUIRED(cs_main); + std::optional GetMineableCommitmentByHash(const uint256& commitmentHash) const; + std::optional GetMineableCommitment(const Consensus::LLMQParams& llmqParams, int nHeight) const EXCLUSIVE_LOCKS_REQUIRED(cs_main); + std::optional GetMineableCommitmentTx(const Consensus::LLMQParams& llmqParams, int nHeight) 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) const; std::vector GetMinedCommitmentsUntilBlock(Consensus::LLMQType llmqType, const CBlockIndex* pindex, size_t maxCount) const; std::map> GetMinedAndActiveCommitmentsUntilBlock(const CBlockIndex* pindex) const; private: - static bool GetCommitmentsFromBlock(const CBlock& block, const CBlockIndex* pindex, std::map& ret, CValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + static std::optional> GetCommitmentsFromBlock( + const CBlock& block, const CBlockIndex* pindex, CValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main); bool ProcessCommitment(int nHeight, const uint256& blockHash, const CFinalCommitment& qc, CValidationState& state, bool fJustCheck) EXCLUSIVE_LOCKS_REQUIRED(cs_main); static bool IsMiningPhase(const Consensus::LLMQParams& llmqParams, int nHeight); bool IsCommitmentRequired(const Consensus::LLMQParams& llmqParams, int nHeight) const EXCLUSIVE_LOCKS_REQUIRED(cs_main); diff --git a/src/llmq/chainlocks.cpp b/src/llmq/chainlocks.cpp index e59d6dd9df46b..1810cfc1b31f0 100644 --- a/src/llmq/chainlocks.cpp +++ b/src/llmq/chainlocks.cpp @@ -71,17 +71,16 @@ bool CChainLocksHandler::AlreadyHave(const CInv& inv) const return seenChainLocks.count(inv.hash) != 0; } -bool CChainLocksHandler::GetChainLockByHash(const uint256& hash, llmq::CChainLockSig& ret) const +std::optional CChainLocksHandler::GetChainLockByHash(const uint256& hash) const { LOCK(cs); if (hash != bestChainLockHash) { // we only propagate the best one and ditch all the old ones - return false; + return std::nullopt; } - ret = bestChainLock; - return true; + return {bestChainLock}; } CChainLockSig CChainLocksHandler::GetBestChainLock() const diff --git a/src/llmq/chainlocks.h b/src/llmq/chainlocks.h index 1891e613fd031..4d7e8f6a3d6ea 100644 --- a/src/llmq/chainlocks.h +++ b/src/llmq/chainlocks.h @@ -91,7 +91,7 @@ class CChainLocksHandler : public CRecoveredSigsListener void Stop(); bool AlreadyHave(const CInv& inv) const; - bool GetChainLockByHash(const uint256& hash, CChainLockSig& ret) const; + std::optional GetChainLockByHash(const uint256& hash) const; CChainLockSig GetBestChainLock() const; void ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStream& vRecv); diff --git a/src/llmq/debug.cpp b/src/llmq/debug.cpp index 7988e60c3e3ab..462f6dce902f6 100644 --- a/src/llmq/debug.cpp +++ b/src/llmq/debug.cpp @@ -128,10 +128,10 @@ UniValue CDKGDebugStatus::ToJson(int detailLevel) const return ret; } -void CDKGDebugManager::GetLocalDebugStatus(llmq::CDKGDebugStatus& ret) const +llmq::CDKGDebugStatus CDKGDebugManager::GetLocalDebugStatus() const { LOCK(cs); - ret = localStatus; + return localStatus; } void CDKGDebugManager::ResetLocalSessionStatus(Consensus::LLMQType llmqType) diff --git a/src/llmq/debug.h b/src/llmq/debug.h index d4cab07390cd0..79b44b45067b9 100644 --- a/src/llmq/debug.h +++ b/src/llmq/debug.h @@ -95,7 +95,7 @@ class CDKGDebugManager public: CDKGDebugManager(); - void GetLocalDebugStatus(CDKGDebugStatus& ret) const; + CDKGDebugStatus GetLocalDebugStatus() const; void ResetLocalSessionStatus(Consensus::LLMQType llmqType); void InitLocalSessionStatus(const Consensus::LLMQParams& llmqParams, const uint256& quorumHash, int quorumHeight); diff --git a/src/llmq/dkgsession.cpp b/src/llmq/dkgsession.cpp index 4b64fee7d7c16..3c7ef97d19d47 100644 --- a/src/llmq/dkgsession.cpp +++ b/src/llmq/dkgsession.cpp @@ -212,39 +212,33 @@ void CDKGSession::SendContributions(CDKGPendingMessages& pendingMessages) } // only performs cheap verifications, but not the signature of the message. this is checked with batched verification -bool CDKGSession::PreVerifyMessage(const CDKGContribution& qc, bool& retBan) const +CDKGSession::success_ban CDKGSession::PreVerifyMessage(const CDKGContribution& qc) const { CDKGLogger logger(*this, __func__); - retBan = false; - if (qc.quorumHash != m_quorum_base_block_index->GetBlockHash()) { logger.Batch("contribution for wrong quorum, rejecting"); - return false; + return {false, false}; } auto member = GetMember(qc.proTxHash); if (!member) { logger.Batch("contributor not a member of this quorum, rejecting contribution"); - retBan = true; - return false; + return {false, true}; } if (qc.contributions->blobs.size() != members.size()) { logger.Batch("invalid contributions count"); - retBan = true; - return false; + return {false, true}; } if (qc.vvec->size() != params.threshold) { logger.Batch("invalid verification vector length"); - retBan = true; - return false; + return {false, true}; } if (!CBLSWorker::VerifyVerificationVector(*qc.vvec)) { logger.Batch("invalid verification vector"); - retBan = true; - return false; + return {false, true}; } if (member->contributions.size() >= 2) { @@ -252,20 +246,18 @@ bool CDKGSession::PreVerifyMessage(const CDKGContribution& qc, bool& retBan) con // this is a DoS protection against members sending multiple contributions with valid signatures to us // we must bail out before any expensive BLS verification happens logger.Batch("dropping contribution from %s as we already got %d contributions", member->dmn->proTxHash.ToString(), member->contributions.size()); - return false; + return {false, false}; } - return true; + return {true, false}; } -void CDKGSession::ReceiveMessage(const CDKGContribution& qc, bool& retBan) +void CDKGSession::ReceiveMessage(const CDKGContribution& qc) { LOCK(cs_pending); CDKGLogger logger(*this, __func__); - retBan = false; - auto member = GetMember(qc.proTxHash); cxxtimer::Timer t1(true); @@ -536,34 +528,29 @@ void CDKGSession::SendComplaint(CDKGPendingMessages& pendingMessages) } // only performs cheap verifications, but not the signature of the message. this is checked with batched verification -bool CDKGSession::PreVerifyMessage(const CDKGComplaint& qc, bool& retBan) const +CDKGSession::success_ban CDKGSession::PreVerifyMessage(const CDKGComplaint& qc) const { CDKGLogger logger(*this, __func__); - retBan = false; - if (qc.quorumHash != m_quorum_base_block_index->GetBlockHash()) { logger.Batch("complaint for wrong quorum, rejecting"); - return false; + return {false, false}; } auto member = GetMember(qc.proTxHash); if (!member) { logger.Batch("complainer not a member of this quorum, rejecting complaint"); - retBan = true; - return false; + return {false, true}; } if (qc.badMembers.size() != (size_t)params.size) { logger.Batch("invalid badMembers bitset size"); - retBan = true; - return false; + return {false, true}; } if (qc.complainForMembers.size() != (size_t)params.size) { logger.Batch("invalid complainForMembers bitset size"); - retBan = true; - return false; + return {false, true}; } if (member->complaints.size() >= 2) { @@ -572,18 +559,16 @@ bool CDKGSession::PreVerifyMessage(const CDKGComplaint& qc, bool& retBan) const // we must bail out before any expensive BLS verification happens logger.Batch("dropping complaint from %s as we already got %d complaints", member->dmn->proTxHash.ToString(), member->complaints.size()); - return false; + return {false, false}; } - return true; + return {true, false}; } -void CDKGSession::ReceiveMessage(const CDKGComplaint& qc, bool& retBan) +void CDKGSession::ReceiveMessage(const CDKGComplaint& qc) { CDKGLogger logger(*this, __func__); - retBan = false; - logger.Batch("received complaint from %s", qc.proTxHash.ToString()); auto member = GetMember(qc.proTxHash); @@ -730,49 +715,41 @@ void CDKGSession::SendJustification(CDKGPendingMessages& pendingMessages, const } // only performs cheap verifications, but not the signature of the message. this is checked with batched verification -bool CDKGSession::PreVerifyMessage(const CDKGJustification& qj, bool& retBan) const +CDKGSession::success_ban CDKGSession::PreVerifyMessage(const CDKGJustification& qj) const { CDKGLogger logger(*this, __func__); - retBan = false; - if (qj.quorumHash != m_quorum_base_block_index->GetBlockHash()) { logger.Batch("justification for wrong quorum, rejecting"); - return false; + return {false, false}; } auto member = GetMember(qj.proTxHash); if (!member) { logger.Batch("justifier not a member of this quorum, rejecting justification"); - retBan = true; - return false; + return {false, true}; } if (qj.contributions.empty()) { logger.Batch("justification with no contributions"); - retBan = true; - return false; + return {false, true}; } std::set contributionsSet; - for (const auto& p : qj.contributions) { - if (p.first > members.size()) { + for (const auto& [index, skShare] : qj.contributions) { + if (index > members.size()) { logger.Batch("invalid contribution index"); - retBan = true; - return false; + return {false, true}; } - if (!contributionsSet.emplace(p.first).second) { + if (!contributionsSet.emplace(index).second) { logger.Batch("duplicate contribution index"); - retBan = true; - return false; + return {false, true}; } - auto& skShare = p.second; if (!skShare.IsValid()) { logger.Batch("invalid contribution"); - retBan = true; - return false; + return {false, true}; } } @@ -782,18 +759,16 @@ bool CDKGSession::PreVerifyMessage(const CDKGJustification& qj, bool& retBan) co // we must bail out before any expensive BLS verification happens logger.Batch("dropping justification from %s as we already got %d justifications", member->dmn->proTxHash.ToString(), member->justifications.size()); - return false; + return {false, false}; } - return true; + return {true, false}; } -void CDKGSession::ReceiveMessage(const CDKGJustification& qj, bool& retBan) +void CDKGSession::ReceiveMessage(const CDKGJustification& qj) { CDKGLogger logger(*this, __func__); - retBan = false; - logger.Batch("received justification from %s", qj.proTxHash.ToString()); auto member = GetMember(qj.proTxHash); @@ -962,13 +937,12 @@ void CDKGSession::SendCommitment(CDKGPendingMessages& pendingMessages) cxxtimer::Timer timerTotal(true); cxxtimer::Timer t1(true); - std::vector memberIndexes; - std::vector vvecs; - BLSSecretKeyVector skContributions; - if (!dkgManager.GetVerifiedContributions(params.type, m_quorum_base_block_index, qc.validMembers, memberIndexes, vvecs, skContributions)) { + auto optContrib = dkgManager.GetVerifiedContributions(params.type, m_quorum_base_block_index, qc.validMembers); + if (!optContrib) { logger.Batch("failed to get valid contributions"); return; } + const auto& [memberIndexes, vvecs, skContributions] = *optContrib; BLSVerificationVectorPtr vvec = cache.BuildQuorumVerificationVector(::SerializeHash(memberIndexes), vvecs); if (vvec == nullptr) { @@ -1042,51 +1016,43 @@ void CDKGSession::SendCommitment(CDKGPendingMessages& pendingMessages) } // only performs cheap verifications, but not the signature of the message. this is checked with batched verification -bool CDKGSession::PreVerifyMessage(const CDKGPrematureCommitment& qc, bool& retBan) const +CDKGSession::success_ban CDKGSession::PreVerifyMessage(const CDKGPrematureCommitment& qc) const { CDKGLogger logger(*this, __func__); - retBan = false; - if (qc.quorumHash != m_quorum_base_block_index->GetBlockHash()) { logger.Batch("commitment for wrong quorum, rejecting"); - return false; + return {false, false}; } auto member = GetMember(qc.proTxHash); if (!member) { logger.Batch("committer not a member of this quorum, rejecting premature commitment"); - retBan = true; - return false; + return {false, true}; } if (qc.validMembers.size() != (size_t)params.size) { logger.Batch("invalid validMembers bitset size"); - retBan = true; - return false; + return {false, true}; } if (qc.CountValidMembers() < params.minSize) { logger.Batch("invalid validMembers count. validMembersCount=%d", qc.CountValidMembers()); - retBan = true; - return false; + return {false, true}; } if (!qc.sig.IsValid()) { logger.Batch("invalid membersSig"); - retBan = true; - return false; + return {false, true}; } if (!qc.quorumSig.IsValid()) { logger.Batch("invalid quorumSig"); - retBan = true; - return false; + return {false, true}; } for (size_t i = members.size(); i < params.size; i++) { if (qc.validMembers[i]) { - retBan = true; logger.Batch("invalid validMembers bitset. bit %d should not be set", i); - return false; + return {false, true}; } } @@ -1096,18 +1062,16 @@ bool CDKGSession::PreVerifyMessage(const CDKGPrematureCommitment& qc, bool& retB // we must bail out before any expensive BLS verification happens logger.Batch("dropping commitment from %s as we already got %d commitments", member->dmn->proTxHash.ToString(), member->prematureCommitments.size()); - return false; + return {false, false}; } - return true; + return {true, false}; } -void CDKGSession::ReceiveMessage(const CDKGPrematureCommitment& qc, bool& retBan) +void CDKGSession::ReceiveMessage(const CDKGPrematureCommitment& qc) { CDKGLogger logger(*this, __func__); - retBan = false; - cxxtimer::Timer t1(true); logger.Batch("received premature commitment from %s. validMembers=%d", qc.proTxHash.ToString(), qc.CountValidMembers()); @@ -1125,11 +1089,10 @@ void CDKGSession::ReceiveMessage(const CDKGPrematureCommitment& qc, bool& retBan } std::vector memberIndexes; - std::vector vvecs; - BLSSecretKeyVector skContributions; BLSVerificationVectorPtr quorumVvec; - if (dkgManager.GetVerifiedContributions(params.type, m_quorum_base_block_index, qc.validMembers, memberIndexes, vvecs, skContributions)) { - quorumVvec = cache.BuildQuorumVerificationVector(::SerializeHash(memberIndexes), vvecs); + if (auto optContributions = dkgManager.GetVerifiedContributions(params.type, m_quorum_base_block_index, qc.validMembers)) { + memberIndexes = optContributions->memberIndexes; + quorumVvec = cache.BuildQuorumVerificationVector(::SerializeHash(memberIndexes), optContributions->vvecs); } if (quorumVvec == nullptr) { @@ -1146,8 +1109,7 @@ void CDKGSession::ReceiveMessage(const CDKGPrematureCommitment& qc, bool& retBan logger.Batch("calculated quorum public key does not match"); return; } - uint256 vvecHash = ::SerializeHash(*quorumVvec); - if (qc.quorumVvecHash != vvecHash) { + if (auto vvecHash = ::SerializeHash(*quorumVvec); qc.quorumVvecHash != vvecHash) { logger.Batch("calculated quorum vvec hash does not match"); return; } diff --git a/src/llmq/dkgsession.h b/src/llmq/dkgsession.h index 633cdc64a5137..38fa40c30cd7d 100644 --- a/src/llmq/dkgsession.h +++ b/src/llmq/dkgsession.h @@ -297,31 +297,33 @@ class CDKGSession * responsible for further verification of validity (e.g. validate vvecs and SK contributions). */ + struct success_ban { bool success; bool ban; }; + // Phase 1: contribution void Contribute(CDKGPendingMessages& pendingMessages); void SendContributions(CDKGPendingMessages& pendingMessages); - bool PreVerifyMessage(const CDKGContribution& qc, bool& retBan) const; - void ReceiveMessage(const CDKGContribution& qc, bool& retBan); + success_ban PreVerifyMessage(const CDKGContribution& qc) const; + void ReceiveMessage(const CDKGContribution& qc); void VerifyPendingContributions() EXCLUSIVE_LOCKS_REQUIRED(cs_pending); // Phase 2: complaint void VerifyAndComplain(CDKGPendingMessages& pendingMessages); void VerifyConnectionAndMinProtoVersions() const; void SendComplaint(CDKGPendingMessages& pendingMessages); - bool PreVerifyMessage(const CDKGComplaint& qc, bool& retBan) const; - void ReceiveMessage(const CDKGComplaint& qc, bool& retBan); + success_ban PreVerifyMessage(const CDKGComplaint& qc) const; + void ReceiveMessage(const CDKGComplaint& qc); // Phase 3: justification void VerifyAndJustify(CDKGPendingMessages& pendingMessages); void SendJustification(CDKGPendingMessages& pendingMessages, const std::set& forMembers); - bool PreVerifyMessage(const CDKGJustification& qj, bool& retBan) const; - void ReceiveMessage(const CDKGJustification& qj, bool& retBan); + success_ban PreVerifyMessage(const CDKGJustification& qj) const; + void ReceiveMessage(const CDKGJustification& qj); // Phase 4: commit void VerifyAndCommit(CDKGPendingMessages& pendingMessages); void SendCommitment(CDKGPendingMessages& pendingMessages); - bool PreVerifyMessage(const CDKGPrematureCommitment& qc, bool& retBan) const; - void ReceiveMessage(const CDKGPrematureCommitment& qc, bool& retBan); + success_ban PreVerifyMessage(const CDKGPrematureCommitment& qc) const; + void ReceiveMessage(const CDKGPrematureCommitment& qc); // Phase 5: aggregate/finalize std::vector FinalizeCommitments(); diff --git a/src/llmq/dkgsessionhandler.cpp b/src/llmq/dkgsessionhandler.cpp index d0af4e6225b13..c84f27322c948 100644 --- a/src/llmq/dkgsessionhandler.cpp +++ b/src/llmq/dkgsessionhandler.cpp @@ -433,8 +433,7 @@ bool ProcessPendingMessageBatch(CDKGSession& session, CDKGPendingMessages& pendi } continue; } - bool ban = false; - if (!session.PreVerifyMessage(*p.second, ban)) { + if (auto [success, ban] = session.PreVerifyMessage(*p.second); !success) { if (ban) { LogPrint(BCLog::LLMQ_DKG, "%s -- banning node due to failed preverification, peer=%d\n", __func__, nodeId); { @@ -460,19 +459,11 @@ bool ProcessPendingMessageBatch(CDKGSession& session, CDKGPendingMessages& pendi } } - for (const auto& p : preverifiedMessages) { - const NodeId &nodeId = p.first; + for (const auto& [nodeId, ptrMessage] : preverifiedMessages) { if (badNodes.count(nodeId)) { continue; } - bool ban = false; - session.ReceiveMessage(*p.second, ban); - if (ban) { - LogPrint(BCLog::LLMQ_DKG, "%s -- banning node after ReceiveMessage failed, peer=%d\n", __func__, nodeId); - LOCK(cs_main); - Misbehaving(nodeId, 100); - badNodes.emplace(nodeId); - } + session.ReceiveMessage(*ptrMessage); } return true; diff --git a/src/llmq/dkgsessionmgr.cpp b/src/llmq/dkgsessionmgr.cpp index 1d38a207dca67..b0f1fdb22d85f 100644 --- a/src/llmq/dkgsessionmgr.cpp +++ b/src/llmq/dkgsessionmgr.cpp @@ -28,10 +28,10 @@ CDKGSessionManager::CDKGSessionManager(CBLSWorker& _blsWorker, bool unitTests, b db = std::make_unique(unitTests ? "" : (GetDataDir() / "llmq/dkgdb"), 1 << 20, unitTests, fWipe); MigrateDKG(); - for (const auto& qt : Params().GetConsensus().llmqs) { + for (const auto& [llmqType, llmqParams] : Params().GetConsensus().llmqs) { dkgSessionHandlers.emplace(std::piecewise_construct, - std::forward_as_tuple(qt.first), - std::forward_as_tuple(qt.second, blsWorker, *this)); + std::forward_as_tuple(llmqType), + std::forward_as_tuple(llmqParams, blsWorker, *this)); } } @@ -128,15 +128,15 @@ void CDKGSessionManager::MigrateDKG() void CDKGSessionManager::StartThreads() { - for (auto& it : dkgSessionHandlers) { - it.second.StartThread(); + for (auto& [_, dkgSession] : dkgSessionHandlers) { + dkgSession.StartThread(); } } void CDKGSessionManager::StopThreads() { - for (auto& it : dkgSessionHandlers) { - it.second.StopThread(); + for (auto& [_, dkgSession] : dkgSessionHandlers) { + dkgSession.StopThread(); } } @@ -151,8 +151,8 @@ void CDKGSessionManager::UpdatedBlockTip(const CBlockIndex* pindexNew, bool fIni if (!IsQuorumDKGEnabled()) return; - for (auto& qt : dkgSessionHandlers) { - qt.second.UpdatedBlockTip(pindexNew); + for (auto& [_, sessionHandler] : dkgSessionHandlers) { + sessionHandler.UpdatedBlockTip(pindexNew); } } @@ -181,7 +181,7 @@ void CDKGSessionManager::ProcessMessage(CNode* pfrom, const std::string& strComm } // peek into the message and see which LLMQType it is. First byte of all messages is always the LLMQType - Consensus::LLMQType llmqType = (Consensus::LLMQType)*vRecv.begin(); + auto llmqType = (Consensus::LLMQType)*vRecv.begin(); if (!dkgSessionHandlers.count(llmqType)) { LOCK(cs_main); Misbehaving(pfrom->GetId(), 100); @@ -196,100 +196,87 @@ bool CDKGSessionManager::AlreadyHave(const CInv& inv) const if (!IsQuorumDKGEnabled()) return false; - for (const auto& p : dkgSessionHandlers) { - auto& dkgType = p.second; - if (dkgType.pendingContributions.HasSeen(inv.hash) - || dkgType.pendingComplaints.HasSeen(inv.hash) - || dkgType.pendingJustifications.HasSeen(inv.hash) - || dkgType.pendingPrematureCommitments.HasSeen(inv.hash)) { + for (const auto& [_, dkgSession] : dkgSessionHandlers) { + if (dkgSession.pendingContributions.HasSeen(inv.hash) + || dkgSession.pendingComplaints.HasSeen(inv.hash) + || dkgSession.pendingJustifications.HasSeen(inv.hash) + || dkgSession.pendingPrematureCommitments.HasSeen(inv.hash)) { return true; } } return false; } -bool CDKGSessionManager::GetContribution(const uint256& hash, CDKGContribution& ret) const +std::optional CDKGSessionManager::GetContribution(const uint256& hash) const { - if (!IsQuorumDKGEnabled()) - return false; + if (!IsQuorumDKGEnabled()) return std::nullopt; - for (const auto& p : dkgSessionHandlers) { - auto& dkgType = p.second; - LOCK(dkgType.cs); - if (dkgType.phase < QuorumPhase_Initialized || dkgType.phase > QuorumPhase_Contribute) { + for (const auto& [_, dkgSessionHandler] : dkgSessionHandlers) { + LOCK(dkgSessionHandler.cs); + if (dkgSessionHandler.phase < QuorumPhase_Initialized || dkgSessionHandler.phase > QuorumPhase_Contribute) { continue; } - LOCK(dkgType.curSession->invCs); - auto it = dkgType.curSession->contributions.find(hash); - if (it != dkgType.curSession->contributions.end()) { - ret = it->second; - return true; + LOCK(dkgSessionHandler.curSession->invCs); + auto it = dkgSessionHandler.curSession->contributions.find(hash); + if (it != dkgSessionHandler.curSession->contributions.end()) { + return {it->second}; } } - return false; + return std::nullopt; } -bool CDKGSessionManager::GetComplaint(const uint256& hash, CDKGComplaint& ret) const +std::optional CDKGSessionManager::GetComplaint(const uint256& hash) const { - if (!IsQuorumDKGEnabled()) - return false; + if (!IsQuorumDKGEnabled()) return std::nullopt; - for (const auto& p : dkgSessionHandlers) { - auto& dkgType = p.second; - LOCK(dkgType.cs); - if (dkgType.phase < QuorumPhase_Contribute || dkgType.phase > QuorumPhase_Complain) { + for (const auto& [_, dkgSessionHandler] : dkgSessionHandlers) { + LOCK(dkgSessionHandler.cs); + if (dkgSessionHandler.phase < QuorumPhase_Contribute || dkgSessionHandler.phase > QuorumPhase_Complain) { continue; } - LOCK(dkgType.curSession->invCs); - auto it = dkgType.curSession->complaints.find(hash); - if (it != dkgType.curSession->complaints.end()) { - ret = it->second; - return true; + LOCK(dkgSessionHandler.curSession->invCs); + auto it = dkgSessionHandler.curSession->complaints.find(hash); + if (it != dkgSessionHandler.curSession->complaints.end()) { + return {it->second}; } } - return false; + return std::nullopt; } -bool CDKGSessionManager::GetJustification(const uint256& hash, CDKGJustification& ret) const +std::optional CDKGSessionManager::GetJustification(const uint256& hash) const { - if (!IsQuorumDKGEnabled()) - return false; + if (!IsQuorumDKGEnabled()) return std::nullopt; - for (const auto& p : dkgSessionHandlers) { - auto& dkgType = p.second; - LOCK(dkgType.cs); - if (dkgType.phase < QuorumPhase_Complain || dkgType.phase > QuorumPhase_Justify) { + for (const auto& [_, dkgSessionHandler] : dkgSessionHandlers) { + LOCK(dkgSessionHandler.cs); + if (dkgSessionHandler.phase < QuorumPhase_Complain || dkgSessionHandler.phase > QuorumPhase_Justify) { continue; } - LOCK(dkgType.curSession->invCs); - auto it = dkgType.curSession->justifications.find(hash); - if (it != dkgType.curSession->justifications.end()) { - ret = it->second; - return true; + LOCK(dkgSessionHandler.curSession->invCs); + auto it = dkgSessionHandler.curSession->justifications.find(hash); + if (it != dkgSessionHandler.curSession->justifications.end()) { + return {it->second}; } } - return false; + return std::nullopt; } -bool CDKGSessionManager::GetPrematureCommitment(const uint256& hash, CDKGPrematureCommitment& ret) const +std::optional CDKGSessionManager::GetPrematureCommitment(const uint256& hash) const { - if (!IsQuorumDKGEnabled()) - return false; + if (!IsQuorumDKGEnabled()) return std::nullopt; - for (const auto& p : dkgSessionHandlers) { - auto& dkgType = p.second; - LOCK(dkgType.cs); - if (dkgType.phase < QuorumPhase_Justify || dkgType.phase > QuorumPhase_Commit) { + for (const auto& [_, dkgSessionHandler] : dkgSessionHandlers) { + LOCK(dkgSessionHandler.cs); + if (dkgSessionHandler.phase < QuorumPhase_Justify || dkgSessionHandler.phase > QuorumPhase_Commit) { continue; } - LOCK(dkgType.curSession->invCs); - auto it = dkgType.curSession->prematureCommitments.find(hash); - if (it != dkgType.curSession->prematureCommitments.end() && dkgType.curSession->validCommitments.count(hash)) { - ret = it->second; - return true; + LOCK(dkgSessionHandler.curSession->invCs); + auto it = dkgSessionHandler.curSession->prematureCommitments.find(hash); + if (it != dkgSessionHandler.curSession->prematureCommitments.end() && dkgSessionHandler.curSession->validCommitments.count(hash)) { + return {it->second}; } } - return false; + return std::nullopt; } void CDKGSessionManager::WriteVerifiedVvecContribution(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const uint256& proTxHash, const BLSVerificationVectorPtr& vvec) @@ -307,17 +294,13 @@ void CDKGSessionManager::WriteEncryptedContributions(Consensus::LLMQType llmqTyp db->Write(std::make_tuple(DB_ENC_CONTRIB, llmqType, pQuorumBaseBlockIndex->GetBlockHash(), proTxHash), contributions); } -bool CDKGSessionManager::GetVerifiedContributions(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const std::vector& validMembers, std::vector& memberIndexesRet, std::vector& vvecsRet, BLSSecretKeyVector& skContributionsRet) const +std::optional CDKGSessionManager::GetVerifiedContributions(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const std::vector& validMembers) const { LOCK(contributionsCacheCs); auto members = CLLMQUtils::GetAllQuorumMembers(GetLLMQParams(llmqType), pQuorumBaseBlockIndex); - memberIndexesRet.clear(); - vvecsRet.clear(); - skContributionsRet.clear(); - memberIndexesRet.reserve(members.size()); - vvecsRet.reserve(members.size()); - skContributionsRet.reserve(members.size()); + CDKGSessionManager::contributions result(members.size()); + auto& [memberIndexes, vvecs, vecSkContributions] = result; for (size_t i = 0; i < members.size(); i++) { if (validMembers[i]) { const uint256& proTxHash = members[i]->proTxHash; @@ -327,19 +310,19 @@ bool CDKGSessionManager::GetVerifiedContributions(Consensus::LLMQType llmqType, auto vvecPtr = std::make_shared(); CBLSSecretKey skContribution; if (!db->Read(std::make_tuple(DB_VVEC, llmqType, pQuorumBaseBlockIndex->GetBlockHash(), proTxHash), *vvecPtr)) { - return false; + return std::nullopt; } db->Read(std::make_tuple(DB_SKCONTRIB, llmqType, pQuorumBaseBlockIndex->GetBlockHash(), proTxHash), skContribution); it = contributionsCache.emplace(cacheKey, ContributionsCacheEntry{GetTimeMillis(), vvecPtr, skContribution}).first; } - memberIndexesRet.emplace_back(i); - vvecsRet.emplace_back(it->second.vvec); - skContributionsRet.emplace_back(it->second.skContribution); + memberIndexes.emplace_back(i); + vvecs.emplace_back(it->second.vvec); + vecSkContributions.emplace_back(it->second.skContribution); } } - return true; + return {result}; } bool CDKGSessionManager::GetEncryptedContributions(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const std::vector& validMembers, const uint256& nProTxHash, std::vector>& vecRet) const diff --git a/src/llmq/dkgsessionmgr.h b/src/llmq/dkgsessionmgr.h index 83086edd9828c..dad6ddf62774f 100644 --- a/src/llmq/dkgsessionmgr.h +++ b/src/llmq/dkgsessionmgr.h @@ -56,15 +56,18 @@ class CDKGSessionManager void ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStream& vRecv); bool AlreadyHave(const CInv& inv) const; - bool GetContribution(const uint256& hash, CDKGContribution& ret) const; - bool GetComplaint(const uint256& hash, CDKGComplaint& ret) const; - bool GetJustification(const uint256& hash, CDKGJustification& ret) const; - bool GetPrematureCommitment(const uint256& hash, CDKGPrematureCommitment& ret) const; + std::optional GetContribution(const uint256& hash) const; + std::optional GetComplaint(const uint256& hash) const; + std::optional GetJustification(const uint256& hash) const; + std::optional GetPrematureCommitment(const uint256& hash) const; // Contributions are written while in the DKG void WriteVerifiedVvecContribution(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const uint256& proTxHash, const BLSVerificationVectorPtr& vvec); void WriteVerifiedSkContribution(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const uint256& proTxHash, const CBLSSecretKey& skContribution); - bool GetVerifiedContributions(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const std::vector& validMembers, std::vector& memberIndexesRet, std::vector& vvecsRet, BLSSecretKeyVector& skContributionsRet) const; + struct contributions {std::vector memberIndexes; std::vector vvecs; BLSSecretKeyVector vecSkContributions; + explicit contributions(size_t reserve_size): memberIndexes(reserve_size), vvecs(reserve_size), vecSkContributions(reserve_size) {};}; + + std::optional GetVerifiedContributions(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const std::vector& validMembers) const; /// Write encrypted (unverified) DKG contributions for the member with the given proTxHash to the llmqDb void WriteEncryptedContributions(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const uint256& proTxHash, const CBLSIESMultiRecipientObjects& contributions); /// Read encrypted (unverified) DKG contributions for the member with the given proTxHash from the llmqDb diff --git a/src/llmq/quorums.cpp b/src/llmq/quorums.cpp index 93251829b069a..184e8d5286297 100644 --- a/src/llmq/quorums.cpp +++ b/src/llmq/quorums.cpp @@ -51,7 +51,7 @@ CQuorum::CQuorum(const Consensus::LLMQParams& _params, CBLSWorker& _blsWorker) : CQuorum::~CQuorum() = default; -void CQuorum::Init(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 = std::move(_qc); m_quorum_base_block_index = _pQuorumBaseBlockIndex; @@ -304,11 +304,12 @@ CQuorumPtr CQuorumManager::BuildQuorumFromCommitment(const Consensus::LLMQType l assert(pQuorumBaseBlockIndex); const uint256& quorumHash{pQuorumBaseBlockIndex->GetBlockHash()}; - uint256 minedBlockHash; - CFinalCommitmentPtr qc = quorumBlockProcessor->GetMinedCommitment(llmqType, quorumHash, minedBlockHash); - if (qc == nullptr) { + auto optQcBlockHash = quorumBlockProcessor->GetMinedCommitment(llmqType, quorumHash); + if (!optQcBlockHash) { return nullptr; } + auto& [qc, minedBlockHash] = *optQcBlockHash; + assert(qc->quorumHash == pQuorumBaseBlockIndex->GetBlockHash()); const auto& llmqParams = llmq::GetLLMQParams(llmqType); @@ -343,12 +344,11 @@ CQuorumPtr CQuorumManager::BuildQuorumFromCommitment(const Consensus::LLMQType l bool CQuorumManager::BuildQuorumContributions(const CFinalCommitmentPtr& fqc, const std::shared_ptr& quorum) const { - std::vector memberIndexes; - std::vector vvecs; - BLSSecretKeyVector skContributions; - if (!dkgManager.GetVerifiedContributions((Consensus::LLMQType)fqc->llmqType, quorum->m_quorum_base_block_index, fqc->validMembers, memberIndexes, vvecs, skContributions)) { + auto optContributions = dkgManager.GetVerifiedContributions(fqc->llmqType, quorum->m_quorum_base_block_index, fqc->validMembers); + if (!optContributions) { return false; } + const auto& [memberIndexes, vvecs, skContributions] = *optContributions; cxxtimer::Timer t2(true); LOCK(quorum->cs); diff --git a/src/llmq/quorums.h b/src/llmq/quorums.h index 78ba9617c2842..8949ed6f560ad 100644 --- a/src/llmq/quorums.h +++ b/src/llmq/quorums.h @@ -171,7 +171,7 @@ class CQuorum public: CQuorum(const Consensus::LLMQParams& _params, CBLSWorker& _blsWorker); ~CQuorum(); - void Init(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); diff --git a/src/llmq/signing_shares.h b/src/llmq/signing_shares.h index 725397025eb39..9eb143f71d0e6 100644 --- a/src/llmq/signing_shares.h +++ b/src/llmq/signing_shares.h @@ -275,7 +275,7 @@ class CSigSharesNodeState // Used to avoid holding locks too long struct SessionInfo { - Consensus::LLMQType llmqType; + Consensus::LLMQType llmqType{Consensus::LLMQType::LLMQ_NONE}; uint256 quorumHash; uint256 id; uint256 msgHash; diff --git a/src/llmq/utils.cpp b/src/llmq/utils.cpp index b78857cd99df5..7cafe4bba1ad5 100644 --- a/src/llmq/utils.cpp +++ b/src/llmq/utils.cpp @@ -374,9 +374,9 @@ std::map CLLMQUtils::GetEnabledQuorumVvecSyn if (!fLLMQTypePresent || !fModePresent || fTooManyEntries) { throw std::invalid_argument(strprintf("Invalid format in -llmq-qvvec-sync: %s", strEntry)); } - for (const auto& p : Params().GetConsensus().llmqs) { - if (p.second.name == strLLMQType) { - llmqType = p.first; + for (const auto& [type, llmqParams] : Params().GetConsensus().llmqs) { + if (llmqParams.name == strLLMQType) { + llmqType = type; break; } } diff --git a/src/masternode/meta.cpp b/src/masternode/meta.cpp index 2a4df3349ab2f..71cd9199ebd8f 100644 --- a/src/masternode/meta.cpp +++ b/src/masternode/meta.cpp @@ -31,8 +31,8 @@ void CMasternodeMetaInfo::AddGovernanceVote(const uint256& nGovernanceObjectHash LOCK(cs); // Insert a zero value, or not. Then increment the value regardless. This // ensures the value is in the map. - const auto& pair = mapGovernanceObjectsVotedOn.emplace(nGovernanceObjectHash, 0); - pair.first->second++; + const auto& [govObjHashCountPair, _] = mapGovernanceObjectsVotedOn.emplace(nGovernanceObjectHash, 0); + govObjHashCountPair->second++; } void CMasternodeMetaInfo::RemoveGovernanceObject(const uint256& nGovernanceObjectHash) @@ -94,8 +94,8 @@ bool CMasternodeMetaMan::AddGovernanceVote(const uint256& proTxHash, const uint2 void CMasternodeMetaMan::RemoveGovernanceObject(const uint256& nGovernanceObjectHash) { LOCK(cs); - for(const auto& p : metaInfos) { - p.second->RemoveGovernanceObject(nGovernanceObjectHash); + for(const auto& [_, mn_ptr] : metaInfos) { + mn_ptr->RemoveGovernanceObject(nGovernanceObjectHash); } } diff --git a/src/masternode/payments.cpp b/src/masternode/payments.cpp index 4a6609c816bca..f68de21f42686 100644 --- a/src/masternode/payments.cpp +++ b/src/masternode/payments.cpp @@ -23,18 +23,16 @@ CMasternodePayments mnpayments; -bool IsOldBudgetBlockValueValid(const CBlock& block, int nBlockHeight, CAmount blockReward, std::string& strErrorRet) { +std::optional IsOldBudgetBlockValueValid(const CBlock& block, int nBlockHeight, CAmount blockReward) { const Consensus::Params& consensusParams = Params().GetConsensus(); bool isBlockRewardValueMet = (block.vtx[0]->GetValueOut() <= blockReward); if (nBlockHeight < consensusParams.nBudgetPaymentsStartBlock) { - strErrorRet = strprintf("Incorrect block %d, old budgets are not activated yet", nBlockHeight); - return false; + return {strprintf("Incorrect block %d, old budgets are not activated yet", nBlockHeight)}; } if (nBlockHeight >= consensusParams.nSuperblockStartBlock) { - strErrorRet = strprintf("Incorrect block %d, old budgets are no longer active", nBlockHeight); - return false; + return {strprintf("Incorrect block %d, old budgets are no longer active", nBlockHeight)}; } // we are still using budgets, but we have no data about them anymore, @@ -44,25 +42,25 @@ bool IsOldBudgetBlockValueValid(const CBlock& block, int nBlockHeight, CAmount b if(nBlockHeight >= consensusParams.nBudgetPaymentsStartBlock && nOffset < consensusParams.nBudgetPaymentsWindowBlocks) { // NOTE: old budget system is disabled since 12.1 - if(masternodeSync.IsSynced()) { + if (masternodeSync.IsSynced()) { // no old budget blocks should be accepted here on mainnet, // testnet/devnet/regtest should produce regular blocks only LogPrint(BCLog::GOBJECT, "%s -- WARNING: Client synced but old budget system is disabled, checking block value against block reward\n", __func__); - if(!isBlockRewardValueMet) { - strErrorRet = strprintf("coinbase pays too much at height %d (actual=%d vs limit=%d), exceeded block reward, old budgets are disabled", - nBlockHeight, block.vtx[0]->GetValueOut(), blockReward); + if (!isBlockRewardValueMet) { + return {strprintf("coinbase pays too much at height %d (actual=%d vs limit=%d), exceeded block reward, old budgets are disabled", + nBlockHeight, block.vtx[0]->GetValueOut(), blockReward)}; } - return isBlockRewardValueMet; + return std::nullopt; } // when not synced, rely on online nodes (all networks) LogPrint(BCLog::GOBJECT, "%s -- WARNING: Skipping old budget block value checks, accepting block\n", __func__); - return true; + return std::nullopt; } if(!isBlockRewardValueMet) { - strErrorRet = strprintf("coinbase pays too much at height %d (actual=%d vs limit=%d), exceeded block reward, block is not in old budget cycle window", - nBlockHeight, block.vtx[0]->GetValueOut(), blockReward); + return {strprintf("coinbase pays too much at height %d (actual=%d vs limit=%d), exceeded block reward, block is not in old budget cycle window", + nBlockHeight, block.vtx[0]->GetValueOut(), blockReward)}; } - return isBlockRewardValueMet; + return std::nullopt; } /** @@ -76,23 +74,21 @@ bool IsOldBudgetBlockValueValid(const CBlock& block, int nBlockHeight, CAmount b * - When non-superblocks are detected, the normal schedule should be maintained */ -bool IsBlockValueValid(const CBlock& block, int nBlockHeight, CAmount blockReward, std::string& strErrorRet) +std::optional IsBlockValueValid(const CBlock& block, int nBlockHeight, CAmount blockReward) { const Consensus::Params& consensusParams = Params().GetConsensus(); bool isBlockRewardValueMet = (block.vtx[0]->GetValueOut() <= blockReward); - strErrorRet = ""; - if (nBlockHeight < consensusParams.nBudgetPaymentsStartBlock) { // old budget system is not activated yet, just make sure we do not exceed the regular block reward - if(!isBlockRewardValueMet) { - strErrorRet = strprintf("coinbase pays too much at height %d (actual=%d vs limit=%d), exceeded block reward, old budgets are not activated yet", - nBlockHeight, block.vtx[0]->GetValueOut(), blockReward); + if (!isBlockRewardValueMet) { + return {strprintf("coinbase pays too much at height %d (actual=%d vs limit=%d), exceeded block reward, old budgets are not activated yet", + nBlockHeight, block.vtx[0]->GetValueOut(), blockReward)}; } - return isBlockRewardValueMet; + return std::nullopt; } else if (nBlockHeight < consensusParams.nSuperblockStartBlock) { // superblocks are not enabled yet, check if we can pass old budget rules - return IsOldBudgetBlockValueValid(block, nBlockHeight, blockReward, strErrorRet); + return IsOldBudgetBlockValueValid(block, nBlockHeight, blockReward); } LogPrint(BCLog::MNPAYMENTS, "block.vtx[0]->GetValueOut() %lld <= blockReward %lld\n", block.vtx[0]->GetValueOut(), blockReward); @@ -105,24 +101,23 @@ bool IsBlockValueValid(const CBlock& block, int nBlockHeight, CAmount blockRewar if (!CSuperblock::IsValidBlockHeight(nBlockHeight)) { // can't possibly be a superblock, so lets just check for block reward limits if (!isBlockRewardValueMet) { - strErrorRet = strprintf("coinbase pays too much at height %d (actual=%d vs limit=%d), exceeded block reward, only regular blocks are allowed at this height", - nBlockHeight, block.vtx[0]->GetValueOut(), blockReward); + return {strprintf("coinbase pays too much at height %d (actual=%d vs limit=%d), exceeded block reward, only regular blocks are allowed at this height", + nBlockHeight, block.vtx[0]->GetValueOut(), blockReward)}; } - return isBlockRewardValueMet; + return std::nullopt; } // bail out in case superblock limits were exceeded if (!isSuperblockMaxValueMet) { - strErrorRet = strprintf("coinbase pays too much at height %d (actual=%d vs limit=%d), exceeded superblock max value", - nBlockHeight, block.vtx[0]->GetValueOut(), nSuperblockMaxValue); - return false; + return {strprintf("coinbase pays too much at height %d (actual=%d vs limit=%d), exceeded superblock max value", + nBlockHeight, block.vtx[0]->GetValueOut(), nSuperblockMaxValue)}; } if(!masternodeSync.IsSynced() || fDisableGovernance) { LogPrint(BCLog::MNPAYMENTS, "%s -- WARNING: Not enough data, checked superblock max bounds only\n", __func__); // not enough data for full checks but at least we know that the superblock limits were honored. // We rely on the network to have followed the correct chain in this case - return true; + return std::nullopt; } // we are synced and possibly on a superblock now @@ -131,21 +126,21 @@ bool IsBlockValueValid(const CBlock& block, int nBlockHeight, CAmount blockRewar // should NOT allow superblocks at all, when superblocks are disabled // revert to block reward limits in this case LogPrint(BCLog::GOBJECT, "%s -- Superblocks are disabled, no superblocks allowed\n", __func__); - if(!isBlockRewardValueMet) { - strErrorRet = strprintf("coinbase pays too much at height %d (actual=%d vs limit=%d), exceeded block reward, superblocks are disabled", - nBlockHeight, block.vtx[0]->GetValueOut(), blockReward); + if (!isBlockRewardValueMet) { + return {strprintf("coinbase pays too much at height %d (actual=%d vs limit=%d), exceeded block reward, superblocks are disabled", + nBlockHeight, block.vtx[0]->GetValueOut(), blockReward)}; } - return isBlockRewardValueMet; + return std::nullopt; } if (!CSuperblockManager::IsSuperblockTriggered(nBlockHeight)) { // we are on a valid superblock height but a superblock was not triggered // revert to block reward limits in this case - if(!isBlockRewardValueMet) { - strErrorRet = strprintf("coinbase pays too much at height %d (actual=%d vs limit=%d), exceeded block reward, no triggered superblock detected", - nBlockHeight, block.vtx[0]->GetValueOut(), blockReward); + if (!isBlockRewardValueMet) { + return {strprintf("coinbase pays too much at height %d (actual=%d vs limit=%d), exceeded block reward, no triggered superblock detected", + nBlockHeight, block.vtx[0]->GetValueOut(), blockReward)}; } - return isBlockRewardValueMet; + return std::nullopt; } // this actually also checks for correct payees and not only amount @@ -153,12 +148,11 @@ bool IsBlockValueValid(const CBlock& block, int nBlockHeight, CAmount blockRewar // triggered but invalid? that's weird LogPrintf("%s -- ERROR: Invalid superblock detected at height %d: %s", __func__, nBlockHeight, block.vtx[0]->ToString()); /* Continued */ // should NOT allow invalid superblocks, when superblocks are enabled - strErrorRet = strprintf("invalid superblock detected at height %d", nBlockHeight); - return false; + return {strprintf("invalid superblock detected at height %d", nBlockHeight)}; } // we got a valid superblock - return true; + return std::nullopt; } bool IsBlockPayeeValid(const CTransaction& txNew, int nBlockHeight, CAmount blockReward) @@ -213,33 +207,38 @@ bool IsBlockPayeeValid(const CTransaction& txNew, int nBlockHeight, CAmount bloc return false; } -void FillBlockPayments(CMutableTransaction& txNew, int nBlockHeight, CAmount blockReward, std::vector& voutMasternodePaymentsRet, std::vector& voutSuperblockPaymentsRet) +masternode_superblock_payments FillBlockPayments(CMutableTransaction& txNew, int nBlockHeight, CAmount blockReward) { + auto [masternodePayments, superblockPayments] = masternode_superblock_payments{}; // only create superblocks if spork is enabled AND if superblock is actually triggered // (height should be validated inside) - if(AreSuperblocksEnabled() && CSuperblockManager::IsSuperblockTriggered(nBlockHeight)) { + if (AreSuperblocksEnabled() && CSuperblockManager::IsSuperblockTriggered(nBlockHeight)) { LogPrint(BCLog::GOBJECT, "%s -- triggered superblock creation at height %d\n", __func__, nBlockHeight); - CSuperblockManager::GetSuperblockPayments(nBlockHeight, voutSuperblockPaymentsRet); + CSuperblockManager::GetSuperblockPayments(nBlockHeight, superblockPayments); } - if (!CMasternodePayments::GetMasternodeTxOuts(nBlockHeight, blockReward, voutMasternodePaymentsRet)) { + if (auto optMasternodePayments = CMasternodePayments::GetMasternodeTxOuts(nBlockHeight, blockReward); !optMasternodePayments) { LogPrint(BCLog::MNPAYMENTS, "%s -- no masternode to pay (MN list probably empty)\n", __func__); + } else { + masternodePayments = *optMasternodePayments; } - txNew.vout.insert(txNew.vout.end(), voutMasternodePaymentsRet.begin(), voutMasternodePaymentsRet.end()); - txNew.vout.insert(txNew.vout.end(), voutSuperblockPaymentsRet.begin(), voutSuperblockPaymentsRet.end()); + txNew.vout.insert(txNew.vout.end(), masternodePayments.begin(), masternodePayments.end()); + txNew.vout.insert(txNew.vout.end(), superblockPayments.begin(), superblockPayments.end()); std::string voutMasternodeStr; - for (const auto& txout : voutMasternodePaymentsRet) { + for (const auto& txout : masternodePayments) { // subtract MN payment from miner reward txNew.vout[0].nValue -= txout.nValue; - if (!voutMasternodeStr.empty()) + if (!voutMasternodeStr.empty()) { voutMasternodeStr += ","; + } voutMasternodeStr += txout.ToString(); } LogPrint(BCLog::MNPAYMENTS, "%s -- nBlockHeight %d blockReward %lld voutMasternodePaymentsRet \"%s\" txNew %s", __func__, /* Continued */ nBlockHeight, blockReward, voutMasternodeStr, txNew.ToString()); + return {masternodePayments, superblockPayments}; } /** @@ -248,30 +247,27 @@ void FillBlockPayments(CMutableTransaction& txNew, int nBlockHeight, CAmount blo * Get masternode payment tx outputs */ -bool CMasternodePayments::GetMasternodeTxOuts(int nBlockHeight, CAmount blockReward, std::vector& voutMasternodePaymentsRet) +std::optional> CMasternodePayments::GetMasternodeTxOuts(int nBlockHeight, CAmount blockReward) { - // make sure it's not filled yet - voutMasternodePaymentsRet.clear(); + auto optVecMasternodePaymentsRet = GetBlockTxOuts(nBlockHeight, blockReward); - if(!GetBlockTxOuts(nBlockHeight, blockReward, voutMasternodePaymentsRet)) { + if (!optVecMasternodePaymentsRet) { LogPrintf("CMasternodePayments::%s -- no payee (deterministic masternode list empty)\n", __func__); - return false; + return std::nullopt; } - for (const auto& txout : voutMasternodePaymentsRet) { + for (const auto& txout : *optVecMasternodePaymentsRet) { CTxDestination dest; ExtractDestination(txout.scriptPubKey, dest); LogPrintf("CMasternodePayments::%s -- Masternode payment %lld to %s\n", __func__, txout.nValue, EncodeDestination(dest)); } - return true; + return optVecMasternodePaymentsRet; } -bool CMasternodePayments::GetBlockTxOuts(int nBlockHeight, CAmount blockReward, std::vector& voutMasternodePaymentsRet) +std::optional> CMasternodePayments::GetBlockTxOuts(int nBlockHeight, CAmount blockReward) { - voutMasternodePaymentsRet.clear(); - const CBlockIndex* pindex; int nReallocActivationHeight{std::numeric_limits::max()}; @@ -289,7 +285,7 @@ bool CMasternodePayments::GetBlockTxOuts(int nBlockHeight, CAmount blockReward, auto dmnPayee = deterministicMNManager->GetListForBlock(pindex).GetMNPayee(); if (!dmnPayee) { - return false; + return std::nullopt; } CAmount operatorReward = 0; @@ -300,6 +296,7 @@ bool CMasternodePayments::GetBlockTxOuts(int nBlockHeight, CAmount blockReward, masternodeReward -= operatorReward; } + std::vector voutMasternodePaymentsRet; if (masternodeReward > 0) { voutMasternodePaymentsRet.emplace_back(masternodeReward, dmnPayee->pdmnState->scriptPayout); } @@ -307,7 +304,7 @@ bool CMasternodePayments::GetBlockTxOuts(int nBlockHeight, CAmount blockReward, voutMasternodePaymentsRet.emplace_back(operatorReward, dmnPayee->pdmnState->scriptOperatorPayout); } - return true; + return {voutMasternodePaymentsRet}; } bool CMasternodePayments::IsTransactionValid(const CTransaction& txNew, int nBlockHeight, CAmount blockReward) @@ -317,13 +314,13 @@ bool CMasternodePayments::IsTransactionValid(const CTransaction& txNew, int nBlo return true; } - std::vector voutMasternodePayments; - if (!GetBlockTxOuts(nBlockHeight, blockReward, voutMasternodePayments)) { + auto optVMasternodePayments = GetBlockTxOuts(nBlockHeight, blockReward); + if (!optVMasternodePayments) { LogPrintf("CMasternodePayments::%s -- ERROR failed to get payees for block at height %s\n", __func__, nBlockHeight); return true; } - for (const auto& txout : voutMasternodePayments) { + for (const auto& txout : *optVMasternodePayments) { bool found = false; for (const auto& txout2 : txNew.vout) { if (txout == txout2) { diff --git a/src/masternode/payments.h b/src/masternode/payments.h index 25672f76e0e88..df8635a9e987d 100644 --- a/src/masternode/payments.h +++ b/src/masternode/payments.h @@ -7,6 +7,7 @@ #include +#include #include #include @@ -17,9 +18,12 @@ class CMutableTransaction; class CTxOut; /// TODO: all 4 functions do not belong here really, they should be refactored/moved somewhere (main.cpp ?) -bool IsBlockValueValid(const CBlock& block, int nBlockHeight, CAmount blockReward, std::string& strErrorRet); +std::optional IsOldBudgetBlockValueValid(const CBlock& block, int nBlockHeight, CAmount blockReward); +std::optional IsBlockValueValid(const CBlock& block, int nBlockHeight, CAmount blockReward); bool IsBlockPayeeValid(const CTransaction& txNew, int nBlockHeight, CAmount blockReward); -void FillBlockPayments(CMutableTransaction& txNew, int nBlockHeight, CAmount blockReward, std::vector& voutMasternodePaymentsRet, std::vector& voutSuperblockPaymentsRet); + +struct masternode_superblock_payments {std::vector vMasternodePayments; std::vector vSuperblockPayments;}; +masternode_superblock_payments FillBlockPayments(CMutableTransaction& txNew, int nBlockHeight, CAmount blockReward); extern CMasternodePayments mnpayments; @@ -31,10 +35,10 @@ extern CMasternodePayments mnpayments; class CMasternodePayments { public: - static bool GetBlockTxOuts(int nBlockHeight, CAmount blockReward, std::vector& voutMasternodePaymentsRet); + static std::optional> GetBlockTxOuts(int nBlockHeight, CAmount blockReward); static bool IsTransactionValid(const CTransaction& txNew, int nBlockHeight, CAmount blockReward); - static bool GetMasternodeTxOuts(int nBlockHeight, CAmount blockReward, std::vector& voutMasternodePaymentsRet); + static std::optional> GetMasternodeTxOuts(int nBlockHeight, CAmount blockReward); }; #endif // BITCOIN_MASTERNODE_PAYMENTS_H diff --git a/src/masternode/utils.cpp b/src/masternode/utils.cpp index cf0abaa9fae9c..29bf41e241c13 100644 --- a/src/masternode/utils.cpp +++ b/src/masternode/utils.cpp @@ -19,7 +19,7 @@ void CMasternodeUtils::ProcessMasternodeConnections(CConnman& connman) std::vector vecDmns; // will be empty when no wallet #ifdef ENABLE_WALLET for (const auto& pair : coinJoinClientManagers) { - pair.second->GetMixingMasternodesInfo(vecDmns); + vecDmns = pair.second->GetMixingMasternodesInfo(); } #endif // ENABLE_WALLET diff --git a/src/miner.cpp b/src/miner.cpp index c05f4ae853fb4..d3ede2d4414d8 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -148,14 +148,11 @@ std::unique_ptr BlockAssembler::CreateNewBlock(const CScript& sc if (fDIP0003Active_context) { for (const Consensus::LLMQParams& params : llmq::CLLMQUtils::GetEnabledQuorumParams(pindexPrev)) { - CTransactionRef qcTx; - if (llmq::quorumBlockProcessor->GetMineableCommitmentTx(params, - nHeight, - qcTx)) { - pblock->vtx.emplace_back(qcTx); + if (auto optQcTx = llmq::quorumBlockProcessor->GetMineableCommitmentTx(params, nHeight)) { + pblock->vtx.emplace_back(*optQcTx); pblocktemplate->vTxFees.emplace_back(0); pblocktemplate->vTxSigOps.emplace_back(0); - nBlockSize += qcTx->GetTotalSize(); + nBlockSize += (*optQcTx)->GetTotalSize(); ++nBlockTx; } } @@ -203,11 +200,15 @@ std::unique_ptr BlockAssembler::CreateNewBlock(const CScript& sc cbTx.nHeight = nHeight; CValidationState state; - if (!CalcCbTxMerkleRootMNList(*pblock, pindexPrev, cbTx.merkleRootMNList, state, ::ChainstateActive().CoinsTip())) { + if (auto optMerkleRootMNList = CalcCbTxMerkleRootMNList(*pblock, pindexPrev, state, ::ChainstateActive().CoinsTip())) { + cbTx.merkleRootMNList = *optMerkleRootMNList; + } else { throw std::runtime_error(strprintf("%s: CalcCbTxMerkleRootMNList failed: %s", __func__, FormatStateMessage(state))); } if (fDIP0008Active_context) { - if (!CalcCbTxMerkleRootQuorums(*pblock, pindexPrev, cbTx.merkleRootQuorums, state)) { + if (auto optMerkleRootQuorums = CalcCbTxMerkleRootQuorums(*pblock, pindexPrev, state)) { + cbTx.merkleRootQuorums = *optMerkleRootQuorums; + } else { throw std::runtime_error(strprintf("%s: CalcCbTxMerkleRootQuorums failed: %s", __func__, FormatStateMessage(state))); } } @@ -217,7 +218,9 @@ std::unique_ptr BlockAssembler::CreateNewBlock(const CScript& sc // Update coinbase transaction with additional info about masternode and governance payments, // get some info back to pass to getblocktemplate - FillBlockPayments(coinbaseTx, nHeight, blockReward, pblocktemplate->voutMasternodePayments, pblocktemplate->voutSuperblockPayments); + auto masternode_superblock_payments = FillBlockPayments(coinbaseTx, nHeight, blockReward); + pblocktemplate->voutMasternodePayments = masternode_superblock_payments.vMasternodePayments; + pblocktemplate->voutSuperblockPayments = masternode_superblock_payments.vSuperblockPayments; pblock->vtx[0] = MakeTransactionRef(std::move(coinbaseTx)); pblocktemplate->vTxFees[0] = -nFees; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index e0d03c21c0884..451656ff95e17 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1728,39 +1728,33 @@ void static ProcessGetData(CNode* pfrom, const CChainParams& chainparams, CConnm } if (!push && (inv.type == MSG_QUORUM_FINAL_COMMITMENT)) { - llmq::CFinalCommitment o; - if (llmq::quorumBlockProcessor->GetMineableCommitmentByHash( - inv.hash, o)) { - connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::QFCOMMITMENT, o)); + if (auto finalCommit = llmq::quorumBlockProcessor->GetMineableCommitmentByHash(inv.hash)) { + connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::QFCOMMITMENT, *finalCommit)); push = true; } } if (!push && (inv.type == MSG_QUORUM_CONTRIB)) { - llmq::CDKGContribution o; - if (llmq::quorumDKGSessionManager->GetContribution(inv.hash, o)) { - connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::QCONTRIB, o)); + if (auto optDKGContribution = llmq::quorumDKGSessionManager->GetContribution(inv.hash)) { + connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::QCONTRIB, *optDKGContribution)); push = true; } } if (!push && (inv.type == MSG_QUORUM_COMPLAINT)) { - llmq::CDKGComplaint o; - if (llmq::quorumDKGSessionManager->GetComplaint(inv.hash, o)) { - connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::QCOMPLAINT, o)); + if (auto optDKGComplaint = llmq::quorumDKGSessionManager->GetComplaint(inv.hash)) { + connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::QCOMPLAINT, *optDKGComplaint)); push = true; } } if (!push && (inv.type == MSG_QUORUM_JUSTIFICATION)) { - llmq::CDKGJustification o; - if (llmq::quorumDKGSessionManager->GetJustification(inv.hash, o)) { - connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::QJUSTIFICATION, o)); + if (auto optDKGJustif = llmq::quorumDKGSessionManager->GetJustification(inv.hash)) { + connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::QJUSTIFICATION, *optDKGJustif)); push = true; } } if (!push && (inv.type == MSG_QUORUM_PREMATURE_COMMITMENT)) { - llmq::CDKGPrematureCommitment o; - if (llmq::quorumDKGSessionManager->GetPrematureCommitment(inv.hash, o)) { - connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::QPCOMMITMENT, o)); + if (auto optDKGPremCom = llmq::quorumDKGSessionManager->GetPrematureCommitment(inv.hash)) { + connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::QPCOMMITMENT, *optDKGPremCom)); push = true; } } @@ -1773,9 +1767,8 @@ void static ProcessGetData(CNode* pfrom, const CChainParams& chainparams, CConnm } if (!push && (inv.type == MSG_CLSIG)) { - llmq::CChainLockSig o; - if (llmq::chainLocksHandler->GetChainLockByHash(inv.hash, o)) { - connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::CLSIG, o)); + if (auto optClSig = llmq::chainLocksHandler->GetChainLockByHash(inv.hash)) { + connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::CLSIG, *optClSig)); push = true; } } @@ -3858,10 +3851,8 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr LOCK(cs_main); - CSimplifiedMNListDiff mnListDiff; - std::string strError; - if (BuildSimplifiedMNListDiff(cmd.baseBlockHash, cmd.blockHash, mnListDiff, strError)) { - connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::MNLISTDIFF, mnListDiff)); + if (auto [mnListDiff, strError] = BuildSimplifiedMNListDiff(cmd.baseBlockHash, cmd.blockHash); mnListDiff) { + connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::MNLISTDIFF, *mnListDiff)); } else { strError = strprintf("getmnlistdiff failed for baseBlockHash=%s, blockHash=%s. error=%s", cmd.baseBlockHash.ToString(), cmd.blockHash.ToString(), strError); Misbehaving(pfrom->GetId(), 1, strError); diff --git a/src/rpc/masternode.cpp b/src/rpc/masternode.cpp index 602f0b754aeea..0472dd736b737 100644 --- a/src/rpc/masternode.cpp +++ b/src/rpc/masternode.cpp @@ -463,10 +463,9 @@ static UniValue masternode_payments(const JSONRPCRequest& request) nBlockFees += nValueIn - tx->GetValueOut(); } - std::vector voutMasternodePayments, voutDummy; CMutableTransaction dummyTx; CAmount blockReward = nBlockFees + GetBlockSubsidy(pindex->pprev->nBits, pindex->pprev->nHeight, Params().GetConsensus()); - FillBlockPayments(dummyTx, pindex->nHeight, blockReward, voutMasternodePayments, voutDummy); + auto [voutMasternodePayments, _] = FillBlockPayments(dummyTx, pindex->nHeight, blockReward); UniValue blockObj(UniValue::VOBJ); CAmount payedPerBlock{0}; diff --git a/src/rpc/rpcevo.cpp b/src/rpc/rpcevo.cpp index fb733bc1db16f..fc01ee7b6fab3 100644 --- a/src/rpc/rpcevo.cpp +++ b/src/rpc/rpcevo.cpp @@ -1169,14 +1169,13 @@ static UniValue protx_diff(const JSONRPCRequest& request) uint256 baseBlockHash = ParseBlock(request.params[1], "baseBlock"); uint256 blockHash = ParseBlock(request.params[2], "block"); - CSimplifiedMNListDiff mnListDiff; - std::string strError; - if (!BuildSimplifiedMNListDiff(baseBlockHash, blockHash, mnListDiff, strError)) { + auto [mnListDiff, strError] = BuildSimplifiedMNListDiff(baseBlockHash, blockHash); + if (!mnListDiff) { throw std::runtime_error(strError); } UniValue ret; - mnListDiff.ToJson(ret); + mnListDiff.value().ToJson(ret); return ret; } diff --git a/src/rpc/rpcquorums.cpp b/src/rpc/rpcquorums.cpp index 2785693bbc0c4..2c2097753e55d 100644 --- a/src/rpc/rpcquorums.cpp +++ b/src/rpc/rpcquorums.cpp @@ -185,8 +185,7 @@ static UniValue quorum_dkgstatus(const JSONRPCRequest& request) } } - llmq::CDKGDebugStatus status; - llmq::quorumDKGDebugManager->GetLocalDebugStatus(status); + llmq::CDKGDebugStatus status = llmq::quorumDKGDebugManager->GetLocalDebugStatus(); auto ret = status.ToJson(detailLevel); @@ -227,10 +226,9 @@ static UniValue quorum_dkgstatus(const JSONRPCRequest& request) } LOCK(cs_main); - llmq::CFinalCommitment fqc; - if (llmq::quorumBlockProcessor->GetMineableCommitment(llmq_params, tipHeight, fqc)) { + if (auto opt_fqc = llmq::quorumBlockProcessor->GetMineableCommitment(llmq_params, tipHeight)) { UniValue obj(UniValue::VOBJ); - fqc.ToJson(obj); + opt_fqc.value().ToJson(obj); minableCommitments.pushKV(std::string(llmq_params.name), obj); } } diff --git a/src/spork.cpp b/src/spork.cpp index e12f637e7794b..5fac1f7d6a543 100644 --- a/src/spork.cpp +++ b/src/spork.cpp @@ -24,32 +24,29 @@ const std::string CSporkManager::SERIALIZATION_VERSION_STRING = "CSporkManager-V CSporkManager sporkManager; -bool CSporkManager::SporkValueIsActive(SporkId nSporkID, int64_t &nActiveValueRet) const +std::optional CSporkManager::SporkValueIsActive(SporkId nSporkID) const { AssertLockHeld(cs); if (!mapSporksActive.count(nSporkID)) return false; - auto it = mapSporksCachedValues.find(nSporkID); - if (it != mapSporksCachedValues.end()) { - nActiveValueRet = it->second; - return true; + if (auto it = mapSporksCachedValues.find(nSporkID); it != mapSporksCachedValues.end()) { + return {it->second}; } // calc how many values we have and how many signers vote for every value - std::unordered_map mapValueCounts; - for (const auto& pair: mapSporksActive.at(nSporkID)) { - mapValueCounts[pair.second.nValue]++; - if (mapValueCounts.at(pair.second.nValue) >= nMinSporkKeys) { + std::unordered_map mapValueCounts; + for (const auto& [key, sporkMessage]: mapSporksActive.at(nSporkID)) { + mapValueCounts[sporkMessage.nValue]++; + if (mapValueCounts.at(sporkMessage.nValue) >= nMinSporkKeys) { // nMinSporkKeys is always more than the half of the max spork keys number, // so there is only one such value and we can stop here - nActiveValueRet = pair.second.nValue; - mapSporksCachedValues[nSporkID] = nActiveValueRet; - return true; + mapSporksCachedValues[nSporkID] = sporkMessage.nValue; + return {sporkMessage.nValue}; } } - return false; + return std::nullopt; } void CSporkManager::Clear() @@ -170,15 +167,15 @@ void CSporkManager::ProcessSpork(CNode* pfrom, const std::string& strCommand, CD } else if (strCommand == NetMsgType::GETSPORKS) { LOCK(cs); // make sure to not lock this together with cs_main for (const auto& pair : mapSporksActive) { - for (const auto& signerSporkPair: pair.second) { - connman.PushMessage(pfrom, CNetMsgMaker(pfrom->GetSendVersion()).Make(NetMsgType::SPORK, signerSporkPair.second)); + for (const auto& [key, sporkMessage]: pair.second) { + connman.PushMessage(pfrom, CNetMsgMaker(pfrom->GetSendVersion()).Make(NetMsgType::SPORK, sporkMessage)); } } } } -bool CSporkManager::UpdateSpork(SporkId nSporkID, int64_t nValue, CConnman& connman) +bool CSporkManager::UpdateSpork(SporkId nSporkID, SporkValue nValue, CConnman& connman) { CSporkMessage spork = CSporkMessage(nSporkID, nValue, GetAdjustedTime()); @@ -213,12 +210,11 @@ bool CSporkManager::IsSporkActive(SporkId nSporkID) const { LOCK(cs); // If nSporkID is cached, and the cached value is true, then return early true - auto it = mapSporksCachedActive.find(nSporkID); - if (it != mapSporksCachedActive.end() && it->second) { + if (auto it = mapSporksCachedActive.find(nSporkID); it != mapSporksCachedActive.end() && it->second) { return true; } - int64_t nSporkValue = GetSporkValue(nSporkID); + SporkValue nSporkValue = GetSporkValue(nSporkID); // Get time is somewhat costly it looks like bool ret = nSporkValue < GetAdjustedTime(); // Only cache true values @@ -228,13 +224,12 @@ bool CSporkManager::IsSporkActive(SporkId nSporkID) const return ret; } -int64_t CSporkManager::GetSporkValue(SporkId nSporkID) const +SporkValue CSporkManager::GetSporkValue(SporkId nSporkID) const { LOCK(cs); - int64_t nSporkValue = -1; - if (SporkValueIsActive(nSporkID, nSporkValue)) { - return nSporkValue; + if (auto optSporkVal = SporkValueIsActive(nSporkID)) { + return *optSporkVal; } for (const auto& sporkDef : sporkDefs) { @@ -244,7 +239,7 @@ int64_t CSporkManager::GetSporkValue(SporkId nSporkID) const } LogPrint(BCLog::SPORK, "CSporkManager::GetSporkValue -- Unknown Spork ID %d\n", nSporkID); - return -1; + return -1; // TODO use std::optional } SporkId CSporkManager::GetSporkIDByName(const std::string& strName) diff --git a/src/spork.h b/src/spork.h index b113981b83bfc..db6bc0655d73c 100644 --- a/src/spork.h +++ b/src/spork.h @@ -54,10 +54,12 @@ namespace std }; } +using SporkValue = int64_t; + struct CSporkDef { SporkId sporkId{SPORK_INVALID}; - int64_t defaultValue{0}; + SporkValue defaultValue{0}; std::string_view name; }; @@ -99,10 +101,10 @@ class CSporkMessage public: SporkId nSporkID; - int64_t nValue; + SporkValue nValue; int64_t nTimeSigned; - CSporkMessage(SporkId nSporkID, int64_t nValue, int64_t nTimeSigned) : + CSporkMessage(SporkId nSporkID, SporkValue nValue, int64_t nTimeSigned) : nSporkID(nSporkID), nValue(nValue), nTimeSigned(nTimeSigned) @@ -123,14 +125,14 @@ class CSporkMessage /** * GetHash returns the double-sha256 hash of the serialized spork message. */ - uint256 GetHash() const; + [[nodiscard]] uint256 GetHash() const; /** * GetSignatureHash returns the hash of the serialized spork message * without the signature included. The intent of this method is to get the * hash to be signed. */ - uint256 GetSignatureHash() const; + [[nodiscard]] uint256 GetSignatureHash() const; /** * Sign will sign the spork message with the given key. @@ -141,7 +143,7 @@ class CSporkMessage * CheckSignature will ensure the spork signature matches the provided public * key hash. */ - bool CheckSignature(const CKeyID& pubKeyId) const; + [[nodiscard]] bool CheckSignature(const CKeyID& pubKeyId) const; /** * GetSignerKeyID is used to recover the spork address of the key used to @@ -172,7 +174,7 @@ class CSporkManager mutable std::unordered_map mapSporksCachedActive GUARDED_BY(cs); - mutable std::unordered_map mapSporksCachedValues GUARDED_BY(cs); + mutable std::unordered_map mapSporksCachedValues GUARDED_BY(cs); std::unordered_map mapSporksByHash GUARDED_BY(cs); std::unordered_map > mapSporksActive GUARDED_BY(cs); @@ -184,7 +186,7 @@ class CSporkManager * SporkValueIsActive is used to get the value agreed upon by the majority * of signed spork messages for a given Spork ID. */ - bool SporkValueIsActive(SporkId nSporkID, int64_t& nActiveValueRet) const EXCLUSIVE_LOCKS_REQUIRED(cs); + std::optional SporkValueIsActive(SporkId nSporkID) const EXCLUSIVE_LOCKS_REQUIRED(cs); public: @@ -244,7 +246,7 @@ class CSporkManager * UpdateSpork is used by the spork RPC command to set a new spork value, sign * and broadcast the spork message. */ - bool UpdateSpork(SporkId nSporkID, int64_t nValue, CConnman& connman); + bool UpdateSpork(SporkId nSporkID, SporkValue nValue, CConnman& connman); /** * IsSporkActive returns a bool for time-based sporks, and should be used @@ -261,7 +263,7 @@ class CSporkManager * GetSporkValue returns the spork value given a Spork ID. If no active spork * message has yet been received by the node, it returns the default value. */ - int64_t GetSporkValue(SporkId nSporkID) const; + SporkValue GetSporkValue(SporkId nSporkID) const; /** * GetSporkIDByName returns the internal Spork ID given the spork name. diff --git a/src/test/setup_common.cpp b/src/test/setup_common.cpp index f4b8726e19a63..d22e64df0cde5 100644 --- a/src/test/setup_common.cpp +++ b/src/test/setup_common.cpp @@ -219,10 +219,14 @@ CBlock TestChainSetup::CreateBlock(const std::vector& txns, BOOST_ASSERT(false); } CValidationState state; - if (!CalcCbTxMerkleRootMNList(block, ::ChainActive().Tip(), cbTx.merkleRootMNList, state, ::ChainstateActive().CoinsTip())) { + if (auto optMerkleRoot = CalcCbTxMerkleRootMNList(block, ::ChainActive().Tip(), state, ::ChainstateActive().CoinsTip())) { + cbTx.merkleRootMNList = *optMerkleRoot; + } else { BOOST_ASSERT(false); } - if (!CalcCbTxMerkleRootQuorums(block, ::ChainActive().Tip(), cbTx.merkleRootQuorums, state)) { + if (auto optMerkleRoot = CalcCbTxMerkleRootQuorums(block, ::ChainActive().Tip(), state)) { + cbTx.merkleRootQuorums = *optMerkleRoot; + } else { BOOST_ASSERT(false); } CMutableTransaction tmpTx{*block.vtx[0]}; diff --git a/src/validation.cpp b/src/validation.cpp index 1d5f35dcae6f0..edf2763ffa915 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2316,13 +2316,12 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl // TODO: resync data (both ways?) and try to reprocess this block later. CAmount blockReward = nFees + GetBlockSubsidy(pindex->pprev->nBits, pindex->pprev->nHeight, chainparams.GetConsensus()); - std::string strError = ""; int64_t nTime5_2 = GetTimeMicros(); nTimeSubsidy += nTime5_2 - nTime5_1; LogPrint(BCLog::BENCHMARK, " - GetBlockSubsidy: %.2fms [%.2fs (%.2fms/blk)]\n", MILLI * (nTime5_2 - nTime5_1), nTimeSubsidy * MICRO, nTimeSubsidy * MILLI / nBlocksTotal); - if (!IsBlockValueValid(block, pindex->nHeight, blockReward, strError)) { - return state.DoS(0, error("ConnectBlock(DASH): %s", strError), REJECT_INVALID, "bad-cb-amount"); + if (auto optStrError = IsBlockValueValid(block, pindex->nHeight, blockReward)) { + return state.DoS(0, error("ConnectBlock(DASH): %s", *optStrError), REJECT_INVALID, "bad-cb-amount"); } int64_t nTime5_3 = GetTimeMicros(); nTimeValueValid += nTime5_3 - nTime5_2; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 30f1f38031fb2..1ab376d509473 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3218,7 +3218,7 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC return true; } -bool CWallet::SelectTxDSInsByDenomination(int nDenom, CAmount nValueMax, std::vector& vecTxDSInRet) +std::optional> CWallet::SelectTxDSInsByDenomination(int nDenom, CAmount nValueMax) { auto locked_chain = chain().lock(); LOCK(cs_wallet); @@ -3227,11 +3227,10 @@ bool CWallet::SelectTxDSInsByDenomination(int nDenom, CAmount nValueMax, std::ve std::set setRecentTxIds; std::vector vCoins; - - vecTxDSInRet.clear(); + std::vector vecTxDSInRet; if (!CCoinJoin::IsValidDenomination(nDenom)) { - return false; + return std::nullopt; } CAmount nDenomAmount = CCoinJoin::DenominationToAmount(nDenom); @@ -3262,7 +3261,7 @@ bool CWallet::SelectTxDSInsByDenomination(int nDenom, CAmount nValueMax, std::ve LogPrint(BCLog::COINJOIN, "CWallet::%s -- setRecentTxIds.size(): %d\n", __func__, setRecentTxIds.size()); - return nValueTotal > 0; + return {vecTxDSInRet}; } static bool IsCurrentForAntiFeeSniping(interfaces::Chain& chain, interfaces::Chain::Lock& locked_chain) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index e68d4d4dffd40..6d79518ddd9f7 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -872,7 +872,7 @@ class CWallet final : public CCryptoKeyStore, private interfaces::Chain::Notific bool SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector groups, std::set& setCoinsRet, CAmount& nValueRet, const CoinSelectionParams& coin_selection_params, bool& bnb_used, CoinType nCoinType = CoinType::ALL_COINS) const; // Coin selection - bool SelectTxDSInsByDenomination(int nDenom, CAmount nValueMax, std::vector& vecTxDSInRet); + std::optional> SelectTxDSInsByDenomination(int nDenom, CAmount nValueMax); bool SelectDenominatedAmounts(CAmount nValueMax, std::set& setAmountsRet) const; bool SelectCoinsGroupedByAddresses(std::vector& vecTallyRet, bool fSkipDenominated = true, bool fAnonymizable = true, bool fSkipUnconfirmed = true, int nMaxOupointsPerAddress = -1) const;