From 72e1d79fb84950dd42a6a6598210474f7dfd2545 Mon Sep 17 00:00:00 2001 From: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com> Date: Fri, 17 Dec 2021 11:22:11 -0600 Subject: [PATCH] refactor: misc spork refactoring (#4620) * Use string_view instead of string for SERIALIZATION_VERSION_STRING. Also introduces string_view serialization. Also use string_view where (trivially) possible for params * separate GetSporks and sporks net msg processing into its own method * refactor: spork, use c++17 if-init * refactor: spork, remove unused params, make param pt * refactor: spork, use simplier construction * refactor: spork, make strError part of if-init, also w/o redundant initialization * refactor: spork, use structured binding * refactor: spork, use for instead of while * clang-format: spork.cpp * resolve cppcheck linter warning about unused variable --- src/net_processing.cpp | 2 +- src/serialize.h | 19 +++++ src/spork.cpp | 188 ++++++++++++++++++++--------------------- src/spork.h | 38 +++++---- 4 files changed, 134 insertions(+), 113 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index f12d0e1ae0..7c8e0804df 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3929,7 +3929,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr } #endif // ENABLE_WALLET coinJoinServer.ProcessMessage(pfrom, strCommand, vRecv, *connman, enable_bip61); - sporkManager.ProcessSpork(pfrom, strCommand, vRecv, *connman); + sporkManager.ProcessSporkMessages(pfrom, strCommand, vRecv, *connman); masternodeSync.ProcessMessage(pfrom, strCommand, vRecv); governance.ProcessMessage(pfrom, strCommand, vRecv, *connman, enable_bip61); CMNAuth::ProcessMessage(pfrom, strCommand, vRecv, *connman); diff --git a/src/serialize.h b/src/serialize.h index c99a4c2313..21e2488c76 100644 --- a/src/serialize.h +++ b/src/serialize.h @@ -970,6 +970,25 @@ void Unserialize(Stream& is, std::basic_string& str) is.read((char*)str.data(), nSize * sizeof(C)); } +/** + * string_view + */ +template +void Serialize(Stream& os, const std::basic_string_view& str) +{ + WriteCompactSize(os, str.size()); + if (!str.empty()) + os.write((char*)str.data(), str.size() * sizeof(C)); +} + +template +void Unserialize(Stream& is, std::basic_string_view& str) +{ + unsigned int nSize = ReadCompactSize(is); + str.resize(nSize); + if (nSize != 0) + is.read((char*)str.data(), nSize * sizeof(C)); +} /** diff --git a/src/spork.cpp b/src/spork.cpp index e12f637e77..4f9f7d00d8 100644 --- a/src/spork.cpp +++ b/src/spork.cpp @@ -20,30 +20,27 @@ #include -const std::string CSporkManager::SERIALIZATION_VERSION_STRING = "CSporkManager-Version-2"; - CSporkManager sporkManager; -bool CSporkManager::SporkValueIsActive(SporkId nSporkID, int64_t &nActiveValueRet) const +bool CSporkManager::SporkValueIsActive(SporkId nSporkID, int64_t& nActiveValueRet) const { AssertLockHeld(cs); if (!mapSporksActive.count(nSporkID)) return false; - auto it = mapSporksCachedValues.find(nSporkID); - if (it != mapSporksCachedValues.end()) { + if (auto it = mapSporksCachedValues.find(nSporkID); it != mapSporksCachedValues.end()) { nActiveValueRet = it->second; return true; } // 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) { + for (const auto& [_, spork] : mapSporksActive.at(nSporkID)) { + mapValueCounts[spork.nValue]++; + 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 = pair.second.nValue; + nActiveValueRet = spork.nValue; mapSporksCachedValues[nSporkID] = nActiveValueRet; return true; } @@ -64,11 +61,9 @@ void CSporkManager::Clear() void CSporkManager::CheckAndRemove() { LOCK(cs); - bool fSporkAddressIsSet = !setSporkPubKeyIDs.empty(); - assert(fSporkAddressIsSet); + assert(!setSporkPubKeyIDs.empty()); - auto itActive = mapSporksActive.begin(); - while (itActive != mapSporksActive.end()) { + for (auto itActive = mapSporksActive.begin(); itActive != mapSporksActive.end();) { auto itSignerPair = itActive->second.begin(); while (itSignerPair != itActive->second.end()) { bool fHasValidSig = setSporkPubKeyIDs.find(itSignerPair->first) != setSporkPubKeyIDs.end() && @@ -87,10 +82,9 @@ void CSporkManager::CheckAndRemove() ++itActive; } - auto itByHash = mapSporksByHash.begin(); - while (itByHash != mapSporksByHash.end()) { + for (auto itByHash = mapSporksByHash.begin(); itByHash != mapSporksByHash.end();) { bool found = false; - for (const auto& signer: setSporkPubKeyIDs) { + for (const auto& signer : setSporkPubKeyIDs) { if (itByHash->second.CheckSignature(signer)) { found = true; break; @@ -104,83 +98,91 @@ void CSporkManager::CheckAndRemove() } } -void CSporkManager::ProcessSpork(CNode* pfrom, const std::string& strCommand, CDataStream& vRecv, CConnman& connman) +void CSporkManager::ProcessSporkMessages(CNode* pfrom, std::string_view strCommand, CDataStream& vRecv, CConnman& connman) { + ProcessSpork(pfrom, strCommand, vRecv, connman); + ProcessGetSporks(pfrom, strCommand, connman); +} - if (strCommand == NetMsgType::SPORK) { +void CSporkManager::ProcessSpork(const CNode* pfrom, std::string_view strCommand, CDataStream& vRecv, CConnman& connman) +{ + if (strCommand != NetMsgType::SPORK) return; - CSporkMessage spork; - vRecv >> spork; + CSporkMessage spork; + vRecv >> spork; - uint256 hash = spork.GetHash(); + uint256 hash = spork.GetHash(); - std::string strLogMsg; - { - LOCK(cs_main); - EraseObjectRequest(pfrom->GetId(), CInv(MSG_SPORK, hash)); - if(!::ChainActive().Tip()) return; - strLogMsg = strprintf("SPORK -- hash: %s id: %d value: %10d bestHeight: %d peer=%d", hash.ToString(), spork.nSporkID, spork.nValue, ::ChainActive().Height(), pfrom->GetId()); - } + std::string strLogMsg; + { + LOCK(cs_main); + EraseObjectRequest(pfrom->GetId(), CInv(MSG_SPORK, hash)); + if (!::ChainActive().Tip()) return; + strLogMsg = strprintf("SPORK -- hash: %s id: %d value: %10d bestHeight: %d peer=%d", hash.ToString(), spork.nSporkID, spork.nValue, ::ChainActive().Height(), pfrom->GetId()); + } - if (spork.nTimeSigned > GetAdjustedTime() + 2 * 60 * 60) { - LOCK(cs_main); - LogPrint(BCLog::SPORK, "CSporkManager::ProcessSpork -- ERROR: too far into the future\n"); - Misbehaving(pfrom->GetId(), 100); - return; - } + if (spork.nTimeSigned > GetAdjustedTime() + 2 * 60 * 60) { + LOCK(cs_main); + LogPrint(BCLog::SPORK, "CSporkManager::ProcessSpork -- ERROR: too far into the future\n"); + Misbehaving(pfrom->GetId(), 100); + return; + } - CKeyID keyIDSigner; + CKeyID keyIDSigner; - if (!spork.GetSignerKeyID(keyIDSigner) || WITH_LOCK(cs, return !setSporkPubKeyIDs.count(keyIDSigner))) { - LOCK(cs_main); - LogPrint(BCLog::SPORK, "CSporkManager::ProcessSpork -- ERROR: invalid signature\n"); - Misbehaving(pfrom->GetId(), 100); - return; - } + if (!spork.GetSignerKeyID(keyIDSigner) || WITH_LOCK(cs, return !setSporkPubKeyIDs.count(keyIDSigner))) { + LOCK(cs_main); + LogPrint(BCLog::SPORK, "CSporkManager::ProcessSpork -- ERROR: invalid signature\n"); + Misbehaving(pfrom->GetId(), 100); + return; + } - { - LOCK(cs); // make sure to not lock this together with cs_main - if (mapSporksActive.count(spork.nSporkID)) { - if (mapSporksActive[spork.nSporkID].count(keyIDSigner)) { - if (mapSporksActive[spork.nSporkID][keyIDSigner].nTimeSigned >= spork.nTimeSigned) { - LogPrint(BCLog::SPORK, "%s seen\n", strLogMsg); - return; - } else { - LogPrintf("%s updated\n", strLogMsg); - } + { + LOCK(cs); // make sure to not lock this together with cs_main + if (mapSporksActive.count(spork.nSporkID)) { + if (mapSporksActive[spork.nSporkID].count(keyIDSigner)) { + if (mapSporksActive[spork.nSporkID][keyIDSigner].nTimeSigned >= spork.nTimeSigned) { + LogPrint(BCLog::SPORK, "%s seen\n", strLogMsg); + return; } else { - LogPrintf("%s new signer\n", strLogMsg); + LogPrintf("%s updated\n", strLogMsg); } } else { - LogPrintf("%s new\n", strLogMsg); - } - } - - - { - LOCK(cs); // make sure to not lock this together with cs_main - mapSporksByHash[hash] = spork; - mapSporksActive[spork.nSporkID][keyIDSigner] = spork; - // Clear cached values on new spork being processed - mapSporksCachedActive.erase(spork.nSporkID); - mapSporksCachedValues.erase(spork.nSporkID); - } - spork.Relay(connman); - - } 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)); + LogPrintf("%s new signer\n", strLogMsg); } + } else { + LogPrintf("%s new\n", strLogMsg); } } + + { + LOCK(cs); // make sure to not lock this together with cs_main + mapSporksByHash[hash] = spork; + mapSporksActive[spork.nSporkID][keyIDSigner] = spork; + // Clear cached values on new spork being processed + mapSporksCachedActive.erase(spork.nSporkID); + mapSporksCachedValues.erase(spork.nSporkID); + } + spork.Relay(connman); } +void CSporkManager::ProcessGetSporks(CNode* pfrom, std::string_view strCommand, CConnman& connman) +{ + if (strCommand != NetMsgType::GETSPORKS) return; + + 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)); + } + } +} + + bool CSporkManager::UpdateSpork(SporkId nSporkID, int64_t nValue, CConnman& connman) { - CSporkMessage spork = CSporkMessage(nSporkID, nValue, GetAdjustedTime()); + CSporkMessage spork(nSporkID, nValue, GetAdjustedTime()); { LOCK(cs); @@ -213,8 +215,7 @@ 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; } @@ -232,8 +233,7 @@ int64_t CSporkManager::GetSporkValue(SporkId nSporkID) const { LOCK(cs); - int64_t nSporkValue = -1; - if (SporkValueIsActive(nSporkID, nSporkValue)) { + if (int64_t nSporkValue = -1; SporkValueIsActive(nSporkID, nSporkValue)) { return nSporkValue; } @@ -247,7 +247,7 @@ int64_t CSporkManager::GetSporkValue(SporkId nSporkID) const return -1; } -SporkId CSporkManager::GetSporkIDByName(const std::string& strName) +SporkId CSporkManager::GetSporkIDByName(std::string_view strName) { for (const auto& sporkDef : sporkDefs) { if (sporkDef.name == strName) { @@ -259,7 +259,7 @@ SporkId CSporkManager::GetSporkIDByName(const std::string& strName) return SPORK_INVALID; } -bool CSporkManager::GetSporkByHash(const uint256& hash, CSporkMessage &sporkRet) const +bool CSporkManager::GetSporkByHash(const uint256& hash, CSporkMessage& sporkRet) const { LOCK(cs); @@ -273,10 +273,11 @@ bool CSporkManager::GetSporkByHash(const uint256& hash, CSporkMessage &sporkRet) return true; } -bool CSporkManager::SetSporkAddress(const std::string& strAddress) { +bool CSporkManager::SetSporkAddress(const std::string& strAddress) +{ LOCK(cs); CTxDestination dest = DecodeDestination(strAddress); - const CKeyID *keyID = boost::get(&dest); + const CKeyID* keyID = boost::get(&dest); if (!keyID) { LogPrintf("CSporkManager::SetSporkAddress -- Failed to parse spork address\n"); return false; @@ -288,8 +289,7 @@ bool CSporkManager::SetSporkAddress(const std::string& strAddress) { bool CSporkManager::SetMinSporkKeys(int minSporkKeys) { LOCK(cs); - int maxKeysNumber = setSporkPubKeyIDs.size(); - if ((minSporkKeys <= maxKeysNumber / 2) || (minSporkKeys > maxKeysNumber)) { + if (int maxKeysNumber = setSporkPubKeyIDs.size(); (minSporkKeys <= maxKeysNumber / 2) || (minSporkKeys > maxKeysNumber)) { LogPrintf("CSporkManager::SetMinSporkKeys -- Invalid min spork signers number: %d\n", minSporkKeys); return false; } @@ -301,7 +301,7 @@ bool CSporkManager::SetPrivKey(const std::string& strPrivKey) { CKey key; CPubKey pubKey; - if(!CMessageSigner::GetKeysFromSecret(strPrivKey, key, pubKey)) { + if (!CMessageSigner::GetKeysFromSecret(strPrivKey, key, pubKey)) { LogPrintf("CSporkManager::SetPrivKey -- Failed to parse private key\n"); return false; } @@ -312,8 +312,7 @@ bool CSporkManager::SetPrivKey(const std::string& strPrivKey) return false; } - CSporkMessage spork; - if (!spork.Sign(key)) { + if (!CSporkMessage().Sign(key)) { LogPrintf("CSporkManager::SetPrivKey -- Test signing failed\n"); return false; } @@ -352,13 +351,12 @@ bool CSporkMessage::Sign(const CKey& key) } CKeyID pubKeyId = key.GetPubKey().GetID(); - std::string strError = ""; // Harden Spork6 so that it is active on testnet and no other networks - if (Params().NetworkIDString() == CBaseChainParams::TESTNET) { + if (std::string strError; Params().NetworkIDString() == CBaseChainParams::TESTNET) { uint256 hash = GetSignatureHash(); - if(!CHashSigner::SignHash(hash, key, vchSig)) { + if (!CHashSigner::SignHash(hash, key, vchSig)) { LogPrintf("CSporkMessage::Sign -- SignHash() failed\n"); return false; } @@ -370,12 +368,12 @@ bool CSporkMessage::Sign(const CKey& key) } else { std::string strMessage = std::to_string(nSporkID) + std::to_string(nValue) + std::to_string(nTimeSigned); - if(!CMessageSigner::SignMessage(strMessage, vchSig, key)) { + if (!CMessageSigner::SignMessage(strMessage, vchSig, key)) { LogPrintf("CSporkMessage::Sign -- SignMessage() failed\n"); return false; } - if(!CMessageSigner::VerifyMessage(pubKeyId, vchSig, strMessage, strError)) { + if (!CMessageSigner::VerifyMessage(pubKeyId, vchSig, strMessage, strError)) { LogPrintf("CSporkMessage::Sign -- VerifyMessage() failed, error: %s\n", strError); return false; } @@ -386,10 +384,8 @@ bool CSporkMessage::Sign(const CKey& key) bool CSporkMessage::CheckSignature(const CKeyID& pubKeyId) const { - std::string strError = ""; - // Harden Spork6 so that it is active on testnet and no other networks - if (Params().NetworkIDString() == CBaseChainParams::TESTNET) { + if (std::string strError; Params().NetworkIDString() == CBaseChainParams::TESTNET) { uint256 hash = GetSignatureHash(); if (!CHashSigner::VerifyHash(hash, pubKeyId, vchSig, strError)) { @@ -399,7 +395,7 @@ bool CSporkMessage::CheckSignature(const CKeyID& pubKeyId) const } else { std::string strMessage = std::to_string(nSporkID) + std::to_string(nValue) + std::to_string(nTimeSigned); - if (!CMessageSigner::VerifyMessage(pubKeyId, vchSig, strMessage, strError)){ + if (!CMessageSigner::VerifyMessage(pubKeyId, vchSig, strMessage, strError)) { LogPrint(BCLog::SPORK, "CSporkMessage::CheckSignature -- VerifyMessage() failed, error: %s\n", strError); return false; } @@ -408,7 +404,7 @@ bool CSporkMessage::CheckSignature(const CKeyID& pubKeyId) const return true; } -bool CSporkMessage::GetSignerKeyID(CKeyID &retKeyidSporkSigner) const +bool CSporkMessage::GetSignerKeyID(CKeyID& retKeyidSporkSigner) const { CPubKey pubkeyFromSig; // Harden Spork6 so that it is active on testnet and no other networks diff --git a/src/spork.h b/src/spork.h index b113981b83..b19e1e9ef3 100644 --- a/src/spork.h +++ b/src/spork.h @@ -14,7 +14,6 @@ #include #include -#include #include #include #include @@ -98,9 +97,9 @@ private: std::vector vchSig; public: - SporkId nSporkID; - int64_t nValue; - int64_t nTimeSigned; + SporkId nSporkID{0}; + int64_t nValue{0}; + int64_t nTimeSigned{0}; CSporkMessage(SporkId nSporkID, int64_t nValue, int64_t nTimeSigned) : nSporkID(nSporkID), @@ -108,12 +107,7 @@ public: nTimeSigned(nTimeSigned) {} - CSporkMessage() : - nSporkID((SporkId)0), - nValue(0), - nTimeSigned(0) - {} - + CSporkMessage() = default; SERIALIZE_METHODS(CSporkMessage, obj) { @@ -166,7 +160,7 @@ public: class CSporkManager { private: - static const std::string SERIALIZATION_VERSION_STRING; + static constexpr std::string_view SERIALIZATION_VERSION_STRING = "CSporkManager-Version-2"; mutable CCriticalSection cs; @@ -232,13 +226,25 @@ public: void CheckAndRemove(); /** - * ProcessSpork is used to handle the 'getsporks' and 'spork' p2p messages. + * ProcessSporkMessages is used to call ProcessSpork and ProcessGetSporks. See below + */ + void ProcessSporkMessages(CNode* pfrom, std::string_view strCommand, CDataStream& vRecv, CConnman& connman); + + /** + * ProcessSpork is used to handle the 'spork' p2p message. * - * For 'getsporks', it sends active sporks to the requesting peer. For 'spork', - * it validates the spork and adds it to the internal spork storage and + * For 'spork', it validates the spork and adds it to the internal spork storage and * performs any necessary processing. */ - void ProcessSpork(CNode* pfrom, const std::string& strCommand, CDataStream& vRecv, CConnman& connman); + void ProcessSpork(const CNode* pfrom, std::string_view strCommand, CDataStream& vRecv, CConnman& connman); + + + /** + * ProcessGetSporks is used to handle the 'getsporks' p2p message. + * + * For 'getsporks', it sends active sporks to the requesting peer. + */ + void ProcessGetSporks(CNode* pfrom, std::string_view strCommand, CConnman& connman); /** * UpdateSpork is used by the spork RPC command to set a new spork value, sign @@ -266,7 +272,7 @@ public: /** * GetSporkIDByName returns the internal Spork ID given the spork name. */ - static SporkId GetSporkIDByName(const std::string& strName) ; + static SporkId GetSporkIDByName(std::string_view strName); /** * GetSporkByHash returns a spork message given a hash of the spork message.