From f4bb06985f9b93bb3ffcd7deef677344304acfcb Mon Sep 17 00:00:00 2001 From: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com> Date: Wed, 19 Oct 2022 13:37:28 -0500 Subject: [PATCH] refactor: use std::optional in some spork code (#4736) * refactor: use std::optional in some spork code * fix: return std::nullopt --- src/net_processing.cpp | 8 +++---- src/spork.cpp | 54 ++++++++++++++++++++---------------------- src/spork.h | 9 +++---- 3 files changed, 34 insertions(+), 37 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 3ee520b954..eada299bbc 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1584,8 +1584,7 @@ bool static AlreadyHave(const CInv& inv, const CTxMemPool& mempool, const llmq:: case MSG_SPORK: { - CSporkMessage spork; - return sporkManager->GetSporkByHash(inv.hash, spork); + return sporkManager->GetSporkByHash(inv.hash).has_value(); } case MSG_GOVERNANCE_OBJECT: @@ -1884,9 +1883,8 @@ void static ProcessGetData(CNode* pfrom, const CChainParams& chainparams, CConnm } if (!push && inv.type == MSG_SPORK) { - CSporkMessage spork; - if (sporkManager->GetSporkByHash(inv.hash, spork)) { - connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::SPORK, spork)); + if (auto opt_spork = sporkManager->GetSporkByHash(inv.hash)) { + connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::SPORK, *opt_spork)); push = true; } } diff --git a/src/spork.cpp b/src/spork.cpp index 3583b6c0bc..bfe9eb99dd 100644 --- a/src/spork.cpp +++ b/src/spork.cpp @@ -24,17 +24,16 @@ std::unique_ptr sporkManager; -bool CSporkManager::SporkValueIsActive(SporkId nSporkID, int64_t& nActiveValueRet) const +std::optional CSporkManager::SporkValueIfActive(SporkId nSporkID) const { AssertLockHeld(cs); - if (!mapSporksActive.count(nSporkID)) return false; + if (!mapSporksActive.count(nSporkID)) return std::nullopt; { LOCK(cs_mapSporksCachedValues); if (auto it = mapSporksCachedValues.find(nSporkID); it != mapSporksCachedValues.end()) { - nActiveValueRet = it->second; - return true; + return {it->second}; } } @@ -45,13 +44,15 @@ bool CSporkManager::SporkValueIsActive(SporkId nSporkID, int64_t& nActiveValueRe if (mapValueCounts.at(spork.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 = spork.nValue; - WITH_LOCK(cs_mapSporksCachedValues, mapSporksCachedValues[nSporkID] = nActiveValueRet); - return true; + { + LOCK(cs_mapSporksCachedValues); + mapSporksCachedValues[nSporkID] = spork.nValue; + } + return {spork.nValue}; } } - return false; + return std::nullopt; } void CSporkManager::Clear() @@ -133,15 +134,17 @@ void CSporkManager::ProcessSpork(const CNode* pfrom, std::string_view msg_type, return; } - CKeyID keyIDSigner; + auto opt_keyIDSigner = spork.GetSignerKeyID(); - if (!spork.GetSignerKeyID(keyIDSigner) || WITH_LOCK(cs, return !setSporkPubKeyIDs.count(keyIDSigner))) { + if (opt_keyIDSigner == std::nullopt || WITH_LOCK(cs, return !setSporkPubKeyIDs.count(*opt_keyIDSigner))) { LOCK(cs_main); LogPrint(BCLog::SPORK, "CSporkManager::ProcessSpork -- ERROR: invalid signature\n"); Misbehaving(pfrom->GetId(), 100); return; } + auto keyIDSigner = *opt_keyIDSigner; + { LOCK(cs); // make sure to not lock this together with cs_main if (mapSporksActive.count(spork.nSporkID)) { @@ -197,8 +200,8 @@ bool CSporkManager::UpdateSpork(SporkId nSporkID, int64_t nValue, CConnman& conn return false; } - CKeyID keyIDSigner; - if (!spork.GetSignerKeyID(keyIDSigner) || !setSporkPubKeyIDs.count(keyIDSigner)) { + auto opt_keyIDSigner = spork.GetSignerKeyID(); + if (opt_keyIDSigner == std::nullopt || !setSporkPubKeyIDs.count(*opt_keyIDSigner)) { LogPrintf("CSporkManager::UpdateSpork: failed to find keyid for private key\n"); return false; } @@ -206,7 +209,7 @@ bool CSporkManager::UpdateSpork(SporkId nSporkID, int64_t nValue, CConnman& conn LogPrintf("CSporkManager::%s -- signed %d %s\n", __func__, nSporkID, spork.GetHash().ToString()); mapSporksByHash[spork.GetHash()] = spork; - mapSporksActive[nSporkID][keyIDSigner] = spork; + mapSporksActive[nSporkID][*opt_keyIDSigner] = spork; // Clear cached values on new spork being processed WITH_LOCK(cs_mapSporksCachedActive, mapSporksCachedActive.erase(spork.nSporkID)); WITH_LOCK(cs_mapSporksCachedValues, mapSporksCachedValues.erase(spork.nSporkID)); @@ -241,8 +244,8 @@ int64_t CSporkManager::GetSporkValue(SporkId nSporkID) const { LOCK(cs); - if (int64_t nSporkValue = -1; SporkValueIsActive(nSporkID, nSporkValue)) { - return nSporkValue; + if (auto opt_sporkValue = SporkValueIfActive(nSporkID)) { + return *opt_sporkValue; } @@ -266,18 +269,14 @@ SporkId CSporkManager::GetSporkIDByName(std::string_view strName) return SPORK_INVALID; } -bool CSporkManager::GetSporkByHash(const uint256& hash, CSporkMessage& sporkRet) const +std::optional CSporkManager::GetSporkByHash(const uint256& hash) const { LOCK(cs); - const auto it = mapSporksByHash.find(hash); + if (const auto it = mapSporksByHash.find(hash); it != mapSporksByHash.end()) + return {it->second}; - if (it == mapSporksByHash.end()) - return false; - - sporkRet = it->second; - - return true; + return std::nullopt; } bool CSporkManager::SetSporkAddress(const std::string& strAddress) @@ -411,13 +410,13 @@ bool CSporkMessage::CheckSignature(const CKeyID& pubKeyId) const return true; } -bool CSporkMessage::GetSignerKeyID(CKeyID& retKeyidSporkSigner) const +std::optional CSporkMessage::GetSignerKeyID() const { CPubKey pubkeyFromSig; // Harden Spork6 so that it is active on testnet and no other networks if (Params().NetworkIDString() == CBaseChainParams::TESTNET) { if (!pubkeyFromSig.RecoverCompact(GetSignatureHash(), vchSig)) { - return false; + return std::nullopt; } } else { std::string strMessage = std::to_string(nSporkID) + std::to_string(nValue) + std::to_string(nTimeSigned); @@ -425,12 +424,11 @@ bool CSporkMessage::GetSignerKeyID(CKeyID& retKeyidSporkSigner) const ss << MESSAGE_MAGIC; ss << strMessage; if (!pubkeyFromSig.RecoverCompact(ss.GetHash(), vchSig)) { - return false; + return std::nullopt; } } - retKeyidSporkSigner = pubkeyFromSig.GetID(); - return true; + return {pubkeyFromSig.GetID()}; } void CSporkMessage::Relay(CConnman& connman) const diff --git a/src/spork.h b/src/spork.h index 1495be8e8a..78eb0cc935 100644 --- a/src/spork.h +++ b/src/spork.h @@ -14,6 +14,7 @@ #include #include +#include #include #include #include @@ -144,7 +145,7 @@ public: * This method was introduced along with the multi-signer sporks feature, * in order to identify which spork key signed this message. */ - bool GetSignerKeyID(CKeyID& retKeyidSporkSigner) const; + std::optional GetSignerKeyID() const; /** * Relay is used to send this spork message to other peers. @@ -178,10 +179,10 @@ private: CKey sporkPrivKey GUARDED_BY(cs); /** - * SporkValueIsActive is used to get the value agreed upon by the majority + * SporkValueIfActive 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 SporkValueIfActive(SporkId nSporkID) const EXCLUSIVE_LOCKS_REQUIRED(cs); public: @@ -284,7 +285,7 @@ public: * hash-based index of sporks for this reason, and this function is the access * point into that index. */ - bool GetSporkByHash(const uint256& hash, CSporkMessage &sporkRet) const LOCKS_EXCLUDED(cs); + std::optional GetSporkByHash(const uint256& hash) const LOCKS_EXCLUDED(cs); /** * SetSporkAddress is used to set a public key ID which will be used to