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
This commit is contained in:
PastaPastaPasta 2021-12-17 11:22:11 -06:00 committed by GitHub
parent 5bdd245105
commit 72e1d79fb8
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 134 additions and 113 deletions

View File

@ -3929,7 +3929,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
} }
#endif // ENABLE_WALLET #endif // ENABLE_WALLET
coinJoinServer.ProcessMessage(pfrom, strCommand, vRecv, *connman, enable_bip61); 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); masternodeSync.ProcessMessage(pfrom, strCommand, vRecv);
governance.ProcessMessage(pfrom, strCommand, vRecv, *connman, enable_bip61); governance.ProcessMessage(pfrom, strCommand, vRecv, *connman, enable_bip61);
CMNAuth::ProcessMessage(pfrom, strCommand, vRecv, *connman); CMNAuth::ProcessMessage(pfrom, strCommand, vRecv, *connman);

View File

@ -970,6 +970,25 @@ void Unserialize(Stream& is, std::basic_string<C>& str)
is.read((char*)str.data(), nSize * sizeof(C)); is.read((char*)str.data(), nSize * sizeof(C));
} }
/**
* string_view
*/
template<typename Stream, typename C>
void Serialize(Stream& os, const std::basic_string_view<C>& str)
{
WriteCompactSize(os, str.size());
if (!str.empty())
os.write((char*)str.data(), str.size() * sizeof(C));
}
template<typename Stream, typename C>
void Unserialize(Stream& is, std::basic_string_view<C>& str)
{
unsigned int nSize = ReadCompactSize(is);
str.resize(nSize);
if (nSize != 0)
is.read((char*)str.data(), nSize * sizeof(C));
}
/** /**

View File

@ -20,30 +20,27 @@
#include <string> #include <string>
const std::string CSporkManager::SERIALIZATION_VERSION_STRING = "CSporkManager-Version-2";
CSporkManager sporkManager; CSporkManager sporkManager;
bool CSporkManager::SporkValueIsActive(SporkId nSporkID, int64_t &nActiveValueRet) const bool CSporkManager::SporkValueIsActive(SporkId nSporkID, int64_t& nActiveValueRet) const
{ {
AssertLockHeld(cs); AssertLockHeld(cs);
if (!mapSporksActive.count(nSporkID)) return false; if (!mapSporksActive.count(nSporkID)) return false;
auto it = mapSporksCachedValues.find(nSporkID); if (auto it = mapSporksCachedValues.find(nSporkID); it != mapSporksCachedValues.end()) {
if (it != mapSporksCachedValues.end()) {
nActiveValueRet = it->second; nActiveValueRet = it->second;
return true; return true;
} }
// calc how many values we have and how many signers vote for every value // calc how many values we have and how many signers vote for every value
std::unordered_map<int64_t, int> mapValueCounts; std::unordered_map<int64_t, int> mapValueCounts;
for (const auto& pair: mapSporksActive.at(nSporkID)) { for (const auto& [_, spork] : mapSporksActive.at(nSporkID)) {
mapValueCounts[pair.second.nValue]++; mapValueCounts[spork.nValue]++;
if (mapValueCounts.at(pair.second.nValue) >= nMinSporkKeys) { if (mapValueCounts.at(spork.nValue) >= nMinSporkKeys) {
// nMinSporkKeys is always more than the half of the max spork keys number, // 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 // so there is only one such value and we can stop here
nActiveValueRet = pair.second.nValue; nActiveValueRet = spork.nValue;
mapSporksCachedValues[nSporkID] = nActiveValueRet; mapSporksCachedValues[nSporkID] = nActiveValueRet;
return true; return true;
} }
@ -64,11 +61,9 @@ void CSporkManager::Clear()
void CSporkManager::CheckAndRemove() void CSporkManager::CheckAndRemove()
{ {
LOCK(cs); LOCK(cs);
bool fSporkAddressIsSet = !setSporkPubKeyIDs.empty(); assert(!setSporkPubKeyIDs.empty());
assert(fSporkAddressIsSet);
auto itActive = mapSporksActive.begin(); for (auto itActive = mapSporksActive.begin(); itActive != mapSporksActive.end();) {
while (itActive != mapSporksActive.end()) {
auto itSignerPair = itActive->second.begin(); auto itSignerPair = itActive->second.begin();
while (itSignerPair != itActive->second.end()) { while (itSignerPair != itActive->second.end()) {
bool fHasValidSig = setSporkPubKeyIDs.find(itSignerPair->first) != setSporkPubKeyIDs.end() && bool fHasValidSig = setSporkPubKeyIDs.find(itSignerPair->first) != setSporkPubKeyIDs.end() &&
@ -87,10 +82,9 @@ void CSporkManager::CheckAndRemove()
++itActive; ++itActive;
} }
auto itByHash = mapSporksByHash.begin(); for (auto itByHash = mapSporksByHash.begin(); itByHash != mapSporksByHash.end();) {
while (itByHash != mapSporksByHash.end()) {
bool found = false; bool found = false;
for (const auto& signer: setSporkPubKeyIDs) { for (const auto& signer : setSporkPubKeyIDs) {
if (itByHash->second.CheckSignature(signer)) { if (itByHash->second.CheckSignature(signer)) {
found = true; found = true;
break; 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; CSporkMessage spork;
vRecv >> spork; vRecv >> spork;
uint256 hash = spork.GetHash(); uint256 hash = spork.GetHash();
std::string strLogMsg; std::string strLogMsg;
{ {
LOCK(cs_main); LOCK(cs_main);
EraseObjectRequest(pfrom->GetId(), CInv(MSG_SPORK, hash)); EraseObjectRequest(pfrom->GetId(), CInv(MSG_SPORK, hash));
if(!::ChainActive().Tip()) return; 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()); 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) { if (spork.nTimeSigned > GetAdjustedTime() + 2 * 60 * 60) {
LOCK(cs_main); LOCK(cs_main);
LogPrint(BCLog::SPORK, "CSporkManager::ProcessSpork -- ERROR: too far into the future\n"); LogPrint(BCLog::SPORK, "CSporkManager::ProcessSpork -- ERROR: too far into the future\n");
Misbehaving(pfrom->GetId(), 100); Misbehaving(pfrom->GetId(), 100);
return; return;
} }
CKeyID keyIDSigner; CKeyID keyIDSigner;
if (!spork.GetSignerKeyID(keyIDSigner) || WITH_LOCK(cs, return !setSporkPubKeyIDs.count(keyIDSigner))) { if (!spork.GetSignerKeyID(keyIDSigner) || WITH_LOCK(cs, return !setSporkPubKeyIDs.count(keyIDSigner))) {
LOCK(cs_main); LOCK(cs_main);
LogPrint(BCLog::SPORK, "CSporkManager::ProcessSpork -- ERROR: invalid signature\n"); LogPrint(BCLog::SPORK, "CSporkManager::ProcessSpork -- ERROR: invalid signature\n");
Misbehaving(pfrom->GetId(), 100); Misbehaving(pfrom->GetId(), 100);
return; return;
} }
{ {
LOCK(cs); // make sure to not lock this together with cs_main LOCK(cs); // make sure to not lock this together with cs_main
if (mapSporksActive.count(spork.nSporkID)) { if (mapSporksActive.count(spork.nSporkID)) {
if (mapSporksActive[spork.nSporkID].count(keyIDSigner)) { if (mapSporksActive[spork.nSporkID].count(keyIDSigner)) {
if (mapSporksActive[spork.nSporkID][keyIDSigner].nTimeSigned >= spork.nTimeSigned) { if (mapSporksActive[spork.nSporkID][keyIDSigner].nTimeSigned >= spork.nTimeSigned) {
LogPrint(BCLog::SPORK, "%s seen\n", strLogMsg); LogPrint(BCLog::SPORK, "%s seen\n", strLogMsg);
return; return;
} else {
LogPrintf("%s updated\n", strLogMsg);
}
} else { } else {
LogPrintf("%s new signer\n", strLogMsg); LogPrintf("%s updated\n", strLogMsg);
} }
} else { } else {
LogPrintf("%s new\n", strLogMsg); LogPrintf("%s new signer\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));
} }
} 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) bool CSporkManager::UpdateSpork(SporkId nSporkID, int64_t nValue, CConnman& connman)
{ {
CSporkMessage spork = CSporkMessage(nSporkID, nValue, GetAdjustedTime()); CSporkMessage spork(nSporkID, nValue, GetAdjustedTime());
{ {
LOCK(cs); LOCK(cs);
@ -213,8 +215,7 @@ bool CSporkManager::IsSporkActive(SporkId nSporkID) const
{ {
LOCK(cs); LOCK(cs);
// If nSporkID is cached, and the cached value is true, then return early true // If nSporkID is cached, and the cached value is true, then return early true
auto it = mapSporksCachedActive.find(nSporkID); if (auto it = mapSporksCachedActive.find(nSporkID); it != mapSporksCachedActive.end() && it->second) {
if (it != mapSporksCachedActive.end() && it->second) {
return true; return true;
} }
@ -232,8 +233,7 @@ int64_t CSporkManager::GetSporkValue(SporkId nSporkID) const
{ {
LOCK(cs); LOCK(cs);
int64_t nSporkValue = -1; if (int64_t nSporkValue = -1; SporkValueIsActive(nSporkID, nSporkValue)) {
if (SporkValueIsActive(nSporkID, nSporkValue)) {
return nSporkValue; return nSporkValue;
} }
@ -247,7 +247,7 @@ int64_t CSporkManager::GetSporkValue(SporkId nSporkID) const
return -1; return -1;
} }
SporkId CSporkManager::GetSporkIDByName(const std::string& strName) SporkId CSporkManager::GetSporkIDByName(std::string_view strName)
{ {
for (const auto& sporkDef : sporkDefs) { for (const auto& sporkDef : sporkDefs) {
if (sporkDef.name == strName) { if (sporkDef.name == strName) {
@ -259,7 +259,7 @@ SporkId CSporkManager::GetSporkIDByName(const std::string& strName)
return SPORK_INVALID; return SPORK_INVALID;
} }
bool CSporkManager::GetSporkByHash(const uint256& hash, CSporkMessage &sporkRet) const bool CSporkManager::GetSporkByHash(const uint256& hash, CSporkMessage& sporkRet) const
{ {
LOCK(cs); LOCK(cs);
@ -273,10 +273,11 @@ bool CSporkManager::GetSporkByHash(const uint256& hash, CSporkMessage &sporkRet)
return true; return true;
} }
bool CSporkManager::SetSporkAddress(const std::string& strAddress) { bool CSporkManager::SetSporkAddress(const std::string& strAddress)
{
LOCK(cs); LOCK(cs);
CTxDestination dest = DecodeDestination(strAddress); CTxDestination dest = DecodeDestination(strAddress);
const CKeyID *keyID = boost::get<CKeyID>(&dest); const CKeyID* keyID = boost::get<CKeyID>(&dest);
if (!keyID) { if (!keyID) {
LogPrintf("CSporkManager::SetSporkAddress -- Failed to parse spork address\n"); LogPrintf("CSporkManager::SetSporkAddress -- Failed to parse spork address\n");
return false; return false;
@ -288,8 +289,7 @@ bool CSporkManager::SetSporkAddress(const std::string& strAddress) {
bool CSporkManager::SetMinSporkKeys(int minSporkKeys) bool CSporkManager::SetMinSporkKeys(int minSporkKeys)
{ {
LOCK(cs); LOCK(cs);
int maxKeysNumber = setSporkPubKeyIDs.size(); if (int maxKeysNumber = setSporkPubKeyIDs.size(); (minSporkKeys <= maxKeysNumber / 2) || (minSporkKeys > maxKeysNumber)) {
if ((minSporkKeys <= maxKeysNumber / 2) || (minSporkKeys > maxKeysNumber)) {
LogPrintf("CSporkManager::SetMinSporkKeys -- Invalid min spork signers number: %d\n", minSporkKeys); LogPrintf("CSporkManager::SetMinSporkKeys -- Invalid min spork signers number: %d\n", minSporkKeys);
return false; return false;
} }
@ -301,7 +301,7 @@ bool CSporkManager::SetPrivKey(const std::string& strPrivKey)
{ {
CKey key; CKey key;
CPubKey pubKey; CPubKey pubKey;
if(!CMessageSigner::GetKeysFromSecret(strPrivKey, key, pubKey)) { if (!CMessageSigner::GetKeysFromSecret(strPrivKey, key, pubKey)) {
LogPrintf("CSporkManager::SetPrivKey -- Failed to parse private key\n"); LogPrintf("CSporkManager::SetPrivKey -- Failed to parse private key\n");
return false; return false;
} }
@ -312,8 +312,7 @@ bool CSporkManager::SetPrivKey(const std::string& strPrivKey)
return false; return false;
} }
CSporkMessage spork; if (!CSporkMessage().Sign(key)) {
if (!spork.Sign(key)) {
LogPrintf("CSporkManager::SetPrivKey -- Test signing failed\n"); LogPrintf("CSporkManager::SetPrivKey -- Test signing failed\n");
return false; return false;
} }
@ -352,13 +351,12 @@ bool CSporkMessage::Sign(const CKey& key)
} }
CKeyID pubKeyId = key.GetPubKey().GetID(); CKeyID pubKeyId = key.GetPubKey().GetID();
std::string strError = "";
// Harden Spork6 so that it is active on testnet and no other networks // 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(); uint256 hash = GetSignatureHash();
if(!CHashSigner::SignHash(hash, key, vchSig)) { if (!CHashSigner::SignHash(hash, key, vchSig)) {
LogPrintf("CSporkMessage::Sign -- SignHash() failed\n"); LogPrintf("CSporkMessage::Sign -- SignHash() failed\n");
return false; return false;
} }
@ -370,12 +368,12 @@ bool CSporkMessage::Sign(const CKey& key)
} else { } else {
std::string strMessage = std::to_string(nSporkID) + std::to_string(nValue) + std::to_string(nTimeSigned); 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"); LogPrintf("CSporkMessage::Sign -- SignMessage() failed\n");
return false; 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); LogPrintf("CSporkMessage::Sign -- VerifyMessage() failed, error: %s\n", strError);
return false; return false;
} }
@ -386,10 +384,8 @@ bool CSporkMessage::Sign(const CKey& key)
bool CSporkMessage::CheckSignature(const CKeyID& pubKeyId) const bool CSporkMessage::CheckSignature(const CKeyID& pubKeyId) const
{ {
std::string strError = "";
// Harden Spork6 so that it is active on testnet and no other networks // 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(); uint256 hash = GetSignatureHash();
if (!CHashSigner::VerifyHash(hash, pubKeyId, vchSig, strError)) { if (!CHashSigner::VerifyHash(hash, pubKeyId, vchSig, strError)) {
@ -399,7 +395,7 @@ bool CSporkMessage::CheckSignature(const CKeyID& pubKeyId) const
} else { } else {
std::string strMessage = std::to_string(nSporkID) + std::to_string(nValue) + std::to_string(nTimeSigned); 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); LogPrint(BCLog::SPORK, "CSporkMessage::CheckSignature -- VerifyMessage() failed, error: %s\n", strError);
return false; return false;
} }
@ -408,7 +404,7 @@ bool CSporkMessage::CheckSignature(const CKeyID& pubKeyId) const
return true; return true;
} }
bool CSporkMessage::GetSignerKeyID(CKeyID &retKeyidSporkSigner) const bool CSporkMessage::GetSignerKeyID(CKeyID& retKeyidSporkSigner) const
{ {
CPubKey pubkeyFromSig; CPubKey pubkeyFromSig;
// Harden Spork6 so that it is active on testnet and no other networks // Harden Spork6 so that it is active on testnet and no other networks

View File

@ -14,7 +14,6 @@
#include <uint256.h> #include <uint256.h>
#include <array> #include <array>
#include <string>
#include <string_view> #include <string_view>
#include <unordered_map> #include <unordered_map>
#include <vector> #include <vector>
@ -98,9 +97,9 @@ private:
std::vector<unsigned char> vchSig; std::vector<unsigned char> vchSig;
public: public:
SporkId nSporkID; SporkId nSporkID{0};
int64_t nValue; int64_t nValue{0};
int64_t nTimeSigned; int64_t nTimeSigned{0};
CSporkMessage(SporkId nSporkID, int64_t nValue, int64_t nTimeSigned) : CSporkMessage(SporkId nSporkID, int64_t nValue, int64_t nTimeSigned) :
nSporkID(nSporkID), nSporkID(nSporkID),
@ -108,12 +107,7 @@ public:
nTimeSigned(nTimeSigned) nTimeSigned(nTimeSigned)
{} {}
CSporkMessage() : CSporkMessage() = default;
nSporkID((SporkId)0),
nValue(0),
nTimeSigned(0)
{}
SERIALIZE_METHODS(CSporkMessage, obj) SERIALIZE_METHODS(CSporkMessage, obj)
{ {
@ -166,7 +160,7 @@ public:
class CSporkManager class CSporkManager
{ {
private: private:
static const std::string SERIALIZATION_VERSION_STRING; static constexpr std::string_view SERIALIZATION_VERSION_STRING = "CSporkManager-Version-2";
mutable CCriticalSection cs; mutable CCriticalSection cs;
@ -232,13 +226,25 @@ public:
void CheckAndRemove(); 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', * For 'spork', it validates the spork and adds it to the internal spork storage and
* it validates the spork and adds it to the internal spork storage and
* performs any necessary processing. * 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 * 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. * 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. * GetSporkByHash returns a spork message given a hash of the spork message.