From 17eb276a65985c54ffd3f106b5dea313e5bea20b Mon Sep 17 00:00:00 2001 From: Daniel Kraft Date: Wed, 7 Oct 2020 15:07:42 +0200 Subject: [PATCH 1/4] Small cleanups with masternode payee handling. Minor cleanups in the code handling the masternode payee address, removing unnecessary complications and making the code more readable. --- divi/src/masternode-payments.cpp | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/divi/src/masternode-payments.cpp b/divi/src/masternode-payments.cpp index 779166a8b..13eee588e 100644 --- a/divi/src/masternode-payments.cpp +++ b/divi/src/masternode-payments.cpp @@ -489,15 +489,14 @@ bool CMasternodePayments::IsScheduled(const CMasternode& mn, int nNotBlockHeight if (tip == nullptr) return false; - CScript mnpayee; - mnpayee = GetScriptForDestination(mn.pubKeyCollateralAddress.GetID()); + const CScript mnpayee = GetScriptForDestination(mn.pubKeyCollateralAddress.GetID()); - CScript payee; for (int64_t h = 0; h <= 8; ++h) { if (tip->nHeight + h == nNotBlockHeight) continue; uint256 seedHash; if (!GetBlockHashForScoring(seedHash, tip, h)) continue; auto* payees = GetPayeesForScoreHash(seedHash); + CScript payee; if (payees != nullptr && payees->GetPayee(payee) && payee == mnpayee) return true; } @@ -740,14 +739,11 @@ bool CMasternodePayments::ProcessBlock(const CBlockIndex* pindex, const int offs if (pmn != NULL) { LogPrint("masternode","CMasternodePayments::ProcessBlock() Found by FindOldestNotInVec \n"); - CScript payee = GetScriptForDestination(pmn->pubKeyCollateralAddress.GetID()); - newWinner.AddPayee(payee); - - CTxDestination address1; - ExtractDestination(payee, address1); - CBitcoinAddress address2(address1); + const CTxDestination dest(pmn->pubKeyCollateralAddress.GetID()); + newWinner.AddPayee(GetScriptForDestination(dest)); - LogPrint("masternode","CMasternodePayments::ProcessBlock() Winner payee %s nHeight %d. \n", address2.ToString().c_str(), newWinner.GetHeight()); + const CBitcoinAddress address(dest); + LogPrint("masternode","CMasternodePayments::ProcessBlock() Winner payee %s nHeight %d. \n", address.ToString().c_str(), newWinner.GetHeight()); } else { LogPrint("masternode","CMasternodePayments::ProcessBlock() Failed to find masternode to pay\n"); } From c96db409e5035ab38536b53d98eb9fcff06d1282 Mon Sep 17 00:00:00 2001 From: Daniel Kraft Date: Wed, 7 Oct 2020 15:25:24 +0200 Subject: [PATCH 2/4] Deduplicate getMessageToSign for masternodes. For CMasternodeBroadcast and CMasternodePing, use the method getMessageToSign also when verifying signatures, rather than duplicating the logic inline again. --- divi/src/masternode.cpp | 50 ++++++++++++----------------------------- 1 file changed, 14 insertions(+), 36 deletions(-) diff --git a/divi/src/masternode.cpp b/divi/src/masternode.cpp index 10c117618..44a8aae83 100644 --- a/divi/src/masternode.cpp +++ b/divi/src/masternode.cpp @@ -842,38 +842,18 @@ bool CMasternodeBroadcast::CheckAndUpdate(int& nDos) return false; } - std::string vchPubKey(pubKeyCollateralAddress.begin(), pubKeyCollateralAddress.end()); - std::string vchPubKey2(pubKeyMasternode.begin(), pubKeyMasternode.end()); - std::string strMessage = addr.ToString() + boost::lexical_cast(sigTime) + vchPubKey + vchPubKey2 + boost::lexical_cast(protocolVersion); - if (protocolVersion < masternodePayments.GetMinMasternodePaymentsProto()) { LogPrint("masternode","mnb - ignoring outdated Masternode %s protocol version %d\n", vin.prevout.hash.ToString(), protocolVersion); return false; } - CScript pubkeyScript; - pubkeyScript = GetScriptForDestination(pubKeyCollateralAddress.GetID()); - - if (pubkeyScript.size() != 25) { - LogPrintf("%s : mnb - pubkey the wrong size\n", __func__); - nDos = 100; - return false; - } - - CScript pubkeyScript2; - pubkeyScript2 = GetScriptForDestination(pubKeyMasternode.GetID()); - - if (pubkeyScript2.size() != 25) { - LogPrintf("%s : mnb - pubkey2 the wrong size\n", __func__); - nDos = 100; - return false; - } - if (!vin.scriptSig.empty()) { LogPrint("masternode","mnb - Ignore Not Empty ScriptSig %s\n", vin.prevout.hash.ToString()); return false; } + const std::string strMessage = getMessageToSign(); + std::string errorMessage = ""; if (!CObfuScationSigner::VerifyMessage(pubKeyCollateralAddress, sig, strMessage, errorMessage)) { LogPrintf("%s : - Got bad Masternode address signature\n", __func__); @@ -887,16 +867,15 @@ bool CMasternodeBroadcast::CheckAndUpdate(int& nDos) // no such masternode, nothing to update if (pmn == NULL) return true; - else { - // this broadcast older than we have, it's bad. - if (pmn->sigTime > sigTime) { - LogPrint("masternode","mnb - Bad sigTime %d for Masternode %s (existing broadcast is at %d)\n", - sigTime, vin.prevout.hash.ToString(), pmn->sigTime); - return false; - } - // masternode is not enabled yet/already, nothing to update - if (!pmn->IsEnabled()) return true; + + // this broadcast older than we have, it's bad. + if (pmn->sigTime > sigTime) { + LogPrint("masternode","mnb - Bad sigTime %d for Masternode %s (existing broadcast is at %d)\n", + sigTime, vin.prevout.hash.ToString(), pmn->sigTime); + return false; } + // masternode is not enabled yet/already, nothing to update + if (!pmn->IsEnabled()) return true; // mn.pubkey = pubkey, IsVinAssociatedWithPubkey is validated once below, // after that they just need to match @@ -969,8 +948,7 @@ std::string CMasternodeBroadcast::getMessageToSign() const std::string vchPubKey(pubKeyCollateralAddress.begin(), pubKeyCollateralAddress.end()); std::string vchPubKey2(pubKeyMasternode.begin(), pubKeyMasternode.end()); - std::string strMessage = addr.ToString() + boost::lexical_cast(sigTime) + vchPubKey + vchPubKey2 + boost::lexical_cast(protocolVersion); - return strMessage; + return addr.ToString() + boost::lexical_cast(sigTime) + vchPubKey + vchPubKey2 + boost::lexical_cast(protocolVersion); } bool CMasternodeBroadcast::Sign(CKey& keyCollateralAddress, bool updateTimeBeforeSigning) @@ -978,7 +956,7 @@ bool CMasternodeBroadcast::Sign(CKey& keyCollateralAddress, bool updateTimeBefor std::string errorMessage; if(updateTimeBeforeSigning) sigTime = GetAdjustedTime(); - std::string strMessage = getMessageToSign(); + const std::string strMessage = getMessageToSign(); if (!CObfuScationSigner::SignMessage(strMessage, errorMessage, sig, keyCollateralAddress)) { LogPrint("masternode","CMasternodeBroadcast::Sign() - Error: %s\n", errorMessage); @@ -1033,7 +1011,7 @@ bool CMasternodePing::Sign(CKey& keyMasternode, CPubKey& pubKeyMasternode, bool std::string errorMessage; if(updateTimeBeforeSigning) sigTime = GetAdjustedTime(); - std::string strMessage = getMessageToSign(); + const std::string strMessage = getMessageToSign(); if (!CObfuScationSigner::SignMessage(strMessage, errorMessage, vchSig, keyMasternode)) { LogPrint("masternode","CMasternodePing::Sign() - Error: %s\n", errorMessage); @@ -1074,7 +1052,7 @@ bool CMasternodePing::CheckAndUpdate(CMasternode& mn, int& nDos, bool fRequireEn // update only if there is no known ping for this masternode or // last ping was more then MASTERNODE_MIN_MNP_SECONDS-60 ago comparing to this one if (!mn.IsPingedWithin(MASTERNODE_MIN_MNP_SECONDS - 60, sigTime)) { - std::string strMessage = vin.ToString() + blockHash.ToString() + boost::lexical_cast(sigTime); + const std::string strMessage = getMessageToSign(); std::string errorMessage = ""; if (!CObfuScationSigner::VerifyMessage(mn.pubKeyMasternode, vchSig, strMessage, errorMessage)) { From ef5d8b54baf26ee728abff77403b797a4bdbdbb3 Mon Sep 17 00:00:00 2001 From: Daniel Kraft Date: Wed, 7 Oct 2020 16:14:26 +0200 Subject: [PATCH 3/4] Mark functions in masternode as const. General cleanups in the masternode.h / masternode.cpp code, mostly marking functions as const that should be, and making things more private. --- divi/src/masternode.cpp | 70 +++++++---------------------------------- divi/src/masternode.h | 25 ++++++++------- 2 files changed, 24 insertions(+), 71 deletions(-) diff --git a/divi/src/masternode.cpp b/divi/src/masternode.cpp index 44a8aae83..f928827a4 100644 --- a/divi/src/masternode.cpp +++ b/divi/src/masternode.cpp @@ -211,12 +211,12 @@ CMasternode& CMasternode::operator=(CMasternode from) return *this; } -bool CMasternode::IsBroadcastedWithin(int seconds) +bool CMasternode::IsBroadcastedWithin(int seconds) const { return (GetAdjustedTime() - sigTime) < seconds; } -bool CMasternode::IsPingedWithin(int seconds, int64_t now) +bool CMasternode::IsPingedWithin(int seconds, int64_t now) const { if (now == -1) now = GetAdjustedTime(); @@ -238,7 +238,7 @@ bool CMasternode::IsEnabled() const return activeState == MASTERNODE_ENABLED; } -int CMasternode::GetMasternodeInputAge() +int CMasternode::GetMasternodeInputAge() const { if (chainActive.Tip() == NULL) return 0; @@ -250,7 +250,7 @@ int CMasternode::GetMasternodeInputAge() return cacheInputAge + (chainActive.Tip()->nHeight - cacheInputAgeBlock); } -std::string CMasternode::Status() +std::string CMasternode::Status() const { std::string strStatus = "ACTIVE"; @@ -383,11 +383,8 @@ std::string CMasternode::TierToString(MasternodeTier tier) return "INVALID"; } -int64_t CMasternode::SecondsSincePayment() +int64_t CMasternode::SecondsSincePayment() const { - CScript pubkeyScript; - pubkeyScript = GetScriptForDestination(pubKeyCollateralAddress.GetID()); - int64_t sec = (GetAdjustedTime() - GetLastPaid()); int64_t month = 60 * 60 * 24 * 30; if (sec < month) return sec; //if it's less than 30 days, give seconds @@ -401,7 +398,7 @@ int64_t CMasternode::SecondsSincePayment() return month + hash.GetCompact(false); } -int64_t CMasternode::GetLastPaid() +int64_t CMasternode::GetLastPaid() const { CBlockIndex* pindexPrev = chainActive.Tip(); if (pindexPrev == NULL) return false; @@ -454,7 +451,7 @@ int64_t CMasternode::GetLastPaid() return 0; } -std::string CMasternode::GetStatus() +std::string CMasternode::GetStatus() const { switch (nActiveState) { case CMasternode::MASTERNODE_PRE_ENABLED: @@ -476,7 +473,7 @@ std::string CMasternode::GetStatus() } } -bool CMasternode::IsValidNetAddr() +bool CMasternode::IsValidNetAddr() const { // TODO: regtest is fine with any addresses for now, // should probably be a bit smarter if one day we start to implement tests for this @@ -484,64 +481,19 @@ bool CMasternode::IsValidNetAddr() (IsReachable(addr) && addr.IsRoutable()); } -CMasternodeBroadcast::CMasternodeBroadcast() -{ - vin = CTxIn(); - addr = CService(); - pubKeyCollateralAddress = CPubKey(); - sig = std::vector(); - activeState = MASTERNODE_ENABLED; - sigTime = GetAdjustedTime(); - lastPing = CMasternodePing(); - cacheInputAge = 0; - cacheInputAgeBlock = 0; - unitTest = false; - allowFreeTx = true; - protocolVersion = PROTOCOL_VERSION; - nScanningErrorCount = 0; - nLastScanningErrorBlockHeight = 0; - nTier = MasternodeTier::INVALID; -} - CMasternodeBroadcast::CMasternodeBroadcast(CService newAddr, CTxIn newVin, CPubKey pubKeyCollateralAddressNew, CPubKey pubKeyMasternodeNew, const MasternodeTier nMasternodeTier, int protocolVersionIn) { vin = newVin; addr = newAddr; pubKeyCollateralAddress = pubKeyCollateralAddressNew; pubKeyMasternode = pubKeyMasternodeNew; - sig = std::vector(); - activeState = MASTERNODE_ENABLED; - sigTime = GetAdjustedTime(); - lastPing = CMasternodePing(); - cacheInputAge = 0; - cacheInputAgeBlock = 0; - unitTest = false; - allowFreeTx = true; protocolVersion = protocolVersionIn; - nScanningErrorCount = 0; - nLastScanningErrorBlockHeight = 0; nTier = nMasternodeTier; } CMasternodeBroadcast::CMasternodeBroadcast(const CMasternode& mn) -{ - vin = mn.vin; - addr = mn.addr; - pubKeyCollateralAddress = mn.pubKeyCollateralAddress; - pubKeyMasternode = mn.pubKeyMasternode; - sig = mn.sig; - activeState = mn.activeState; - sigTime = mn.sigTime; - lastPing = mn.lastPing; - cacheInputAge = mn.cacheInputAge; - cacheInputAgeBlock = mn.cacheInputAgeBlock; - unitTest = mn.unitTest; - allowFreeTx = mn.allowFreeTx; - protocolVersion = mn.protocolVersion; - nScanningErrorCount = mn.nScanningErrorCount; - nLastScanningErrorBlockHeight = mn.nLastScanningErrorBlockHeight; - nTier = mn.nTier; -} + : CMasternode(mn) +{} bool CMasternodeBroadcastFactory::checkBlockchainSync(std::string& strErrorRet, bool fOffline) @@ -1101,7 +1053,7 @@ bool CMasternodePing::CheckAndUpdate(CMasternode& mn, int& nDos, bool fRequireEn return false; } -void CMasternodePing::Relay() +void CMasternodePing::Relay() const { CInv inv(MSG_MASTERNODE_PING, GetHash()); RelayInv(inv); diff --git a/divi/src/masternode.h b/divi/src/masternode.h index c3ddfc108..c79c228df 100644 --- a/divi/src/masternode.h +++ b/divi/src/masternode.h @@ -80,7 +80,7 @@ class CMasternodePing bool CheckAndUpdate(CMasternode& mn, int& nDos, bool fRequireEnabled = true) const; std::string getMessageToSign() const; bool Sign(CKey& keyMasternode, CPubKey& pubKeyMasternode, bool updateTimeBeforeSigning = true); - void Relay(); + void Relay() const; uint256 GetHash() const { @@ -129,6 +129,9 @@ class CMasternode mutable CCriticalSection cs; int64_t lastTimeChecked; + mutable int cacheInputAge; + mutable int cacheInputAgeBlock; + public: enum state { MASTERNODE_PRE_ENABLED, @@ -149,8 +152,6 @@ class CMasternode std::vector sig; int activeState; int64_t sigTime; //mnb message time - int cacheInputAge; - int cacheInputAgeBlock; bool unitTest; bool allowFreeTx; int protocolVersion; @@ -214,33 +215,33 @@ class CMasternode nTier = static_cast (tier); } - int64_t SecondsSincePayment(); + int64_t SecondsSincePayment() const; bool UpdateFromNewBroadcast(CMasternodeBroadcast &mnb); void Check(bool forceCheck = false); - bool IsBroadcastedWithin(int seconds); + bool IsBroadcastedWithin(int seconds) const; - bool IsPingedWithin(int seconds, int64_t now = -1); + bool IsPingedWithin(int seconds, int64_t now = -1) const; void Disable(); bool IsEnabled() const; - int GetMasternodeInputAge(); + int GetMasternodeInputAge() const; static CAmount GetTierCollateralAmount(MasternodeTier tier); static MasternodeTier GetTierByCollateralAmount(CAmount nCollateral); static bool IsTierValid(MasternodeTier tier); static std::string TierToString(MasternodeTier tier); - std::string GetStatus(); + std::string GetStatus() const; - std::string Status(); + std::string Status() const; - int64_t GetLastPaid(); - bool IsValidNetAddr(); + int64_t GetLastPaid() const; + bool IsValidNetAddr() const; }; @@ -251,7 +252,7 @@ class CMasternode class CMasternodeBroadcast : public CMasternode { public: - CMasternodeBroadcast(); + CMasternodeBroadcast() = default; CMasternodeBroadcast( CService newAddr, CTxIn newVin, From d529ab13baa1d0d5e9f5fe5a9508fb30a461ce3e Mon Sep 17 00:00:00 2001 From: Daniel Kraft Date: Wed, 7 Oct 2020 15:46:14 +0200 Subject: [PATCH 4/4] Avoid looking up masternode by payee. Looking up a masternode by payee script is error-prone, as it is not a unique identifier. Multiple masternodes could be using the same collateral address (and may in the future use the same reward address). The only place where this is being used is in verifying a mnw message. Instead of the previous logic relying on the lookup (which was hard to read anyway), just explicitly check that the payee matches one of the masternodes in our payment queue. --- divi/src/masternode-payments.cpp | 14 +++++++++----- divi/src/masternodeman.cpp | 13 ------------- divi/src/masternodeman.h | 2 +- 3 files changed, 10 insertions(+), 19 deletions(-) diff --git a/divi/src/masternode-payments.cpp b/divi/src/masternode-payments.cpp index 13eee588e..5864ee989 100644 --- a/divi/src/masternode-payments.cpp +++ b/divi/src/masternode-payments.cpp @@ -695,11 +695,15 @@ bool CMasternodePaymentWinner::IsValid(CNode* pnode, std::string& strError) cons if(!masternodeSync.IsSynced()){ return true;} - std::vector mnQueue = mnodeman.GetMasternodePaymentQueue(seedHash, nBlockHeight, true); - auto it = std::find(mnQueue.begin(),mnQueue.end(), mnodeman.Find(payee)); - if(it == mnQueue.end()) - return false; - return (std::distance(mnQueue.begin(), it) < 2 * MNPAYMENTS_SIGNATURES_TOTAL); + /* Make sure that the payee is in our own payment queue near the top. */ + const std::vector mnQueue = mnodeman.GetMasternodePaymentQueue(seedHash, nBlockHeight, true); + for (int i = 0; i < std::min(2 * MNPAYMENTS_SIGNATURES_TOTAL, mnQueue.size()); ++i) { + const auto& mn = *mnQueue[i]; + const CScript mnPayee = GetScriptForDestination(mn.pubKeyCollateralAddress.GetID()); + if (mnPayee == payee) + return true; + } + return false; } bool CMasternodePayments::ProcessBlock(const CBlockIndex* pindex, const int offset) diff --git a/divi/src/masternodeman.cpp b/divi/src/masternodeman.cpp index b71ce8217..5f9956771 100644 --- a/divi/src/masternodeman.cpp +++ b/divi/src/masternodeman.cpp @@ -365,19 +365,6 @@ void CMasternodeMan::DsegUpdate(CNode* pnode) mWeAskedForMasternodeList[pnode->addr] = askAgain; } -CMasternode* CMasternodeMan::Find(const CScript& payee) -{ - LOCK(cs); - CScript payee2; - - BOOST_FOREACH (CMasternode& mn, vMasternodes) { - payee2 = GetScriptForDestination(mn.pubKeyCollateralAddress.GetID()); - if (payee2 == payee) - return &mn; - } - return NULL; -} - CMasternode* CMasternodeMan::Find(const CTxIn& vin) { LOCK(cs); diff --git a/divi/src/masternodeman.h b/divi/src/masternodeman.h index 386890c8b..8599fb28d 100644 --- a/divi/src/masternodeman.h +++ b/divi/src/masternodeman.h @@ -95,7 +95,7 @@ class CMasternodeMan void DsegUpdate(CNode* pnode); /// Find an entry - CMasternode* Find(const CScript& payee); + CMasternode* Find(const CScript& payee) = delete; CMasternode* Find(const CTxIn& vin); CMasternode* Find(const CPubKey& pubKeyMasternode);