[Refactoring] Clean budget manager code redundancies

src/budget/budgetmanager.cpp

@@ -205,43 +205,6 @@ void CBudgetManager::SetBudgetProposalsStr(CFinalizedBudget& finalizedBudget) co
     finalizedBudget.SetProposalsStr(strProposals);
 }
 
-std::string CBudgetManager::GetFinalizedBudgetStatus(const uint256& nHash) const
-{
-    CFinalizedBudget fb;
-    if (!GetFinalizedBudget(nHash, fb))
-        return strprintf("ERROR: cannot find finalized budget %s\n", nHash.ToString());
-
-    std::string retBadHashes = "";
-    std::string retBadPayeeOrAmount = "";
-    int nBlockStart = fb.GetBlockStart();
-    int nBlockEnd = fb.GetBlockEnd();
-
-    for (int nBlockHeight = nBlockStart; nBlockHeight <= nBlockEnd; nBlockHeight++) {
-        CTxBudgetPayment budgetPayment;
-        if (!fb.GetBudgetPaymentByBlock(nBlockHeight, budgetPayment)) {
-            LogPrint(BCLog::MNBUDGET,"%s: Couldn't find budget payment for block %lld\n", __func__, nBlockHeight);
-            continue;
-        }
-
-        CBudgetProposal bp;
-        if (!GetProposal(budgetPayment.nProposalHash, bp)) {
-            retBadHashes += (retBadHashes == "" ? "" : ", ") + budgetPayment.nProposalHash.ToString();
-            continue;
-        }
-
-        if (bp.GetPayee() != budgetPayment.payee || bp.GetAmount() != budgetPayment.nAmount) {
-            retBadPayeeOrAmount += (retBadPayeeOrAmount == "" ? "" : ", ") + budgetPayment.nProposalHash.ToString();
-        }
-    }
-
-    if (retBadHashes == "" && retBadPayeeOrAmount == "") return "OK";
-
-    if (retBadHashes != "") retBadHashes = "Unknown proposal(s) hash! Check this proposal(s) before voting: " + retBadHashes;
-    if (retBadPayeeOrAmount != "") retBadPayeeOrAmount = "Budget payee/nAmount doesn't match our proposal(s)! "+ retBadPayeeOrAmount;
-
-    return retBadHashes + " -- " + retBadPayeeOrAmount;
-}
-
 bool CBudgetManager::AddFinalizedBudget(CFinalizedBudget& finalizedBudget, CNode* pfrom)
 {
     AssertLockNotHeld(cs_budgets);    // need to lock cs_main here (CheckCollateral)
@@ -675,15 +638,6 @@ bool CBudgetManager::GetProposal(const uint256& nHash, CBudgetProposal& bp) cons
     return true;
 }
 
-bool CBudgetManager::GetFinalizedBudget(const uint256& nHash, CFinalizedBudget& fb) const
-{
-    LOCK(cs_budgets);
-    auto it = mapFinalizedBudgets.find(nHash);
-    if (it == mapFinalizedBudgets.end()) return false;
-    fb = it->second;
-    return true;
-}
-
 bool CBudgetManager::IsBudgetPaymentBlock(int nBlockHeight, int& nCountThreshold) const
 {
     int nHighestCount = GetHighestVoteCount(nBlockHeight);
@@ -947,16 +901,6 @@ CDataStream CBudgetManager::GetFinalizedBudgetSerialized(const uint256& budgetHa
     return mapFinalizedBudgets.at(budgetHash).GetBroadcast();
 }
 
-bool CBudgetManager::AddAndRelayProposalVote(const CBudgetVote& vote, std::string& strError)
-{
-    if (UpdateProposal(vote, nullptr, strError)) {
-        AddSeenProposalVote(vote);
-        vote.Relay();
-        return true;
-    }
-    return false;
-}
-
 void CBudgetManager::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload)
 {
     NewBlock();
@@ -1625,40 +1569,25 @@ bool CheckCollateral(const uint256& nTxCollateralHash, const uint256& nExpectedH
         return false;
     }
 
-    if (txCollateral->vout.size() < 1) return false;
+    if (txCollateral->vout.empty()) return false;
     if (txCollateral->nLockTime != 0) return false;
+    if (nBlockHash.IsNull()) {
+        strError = strprintf("Collateral transaction %s is unconfirmed", nTxCollateralHash.ToString());
+        return false;
+    }
 
-    CScript findScript;
-    findScript << OP_RETURN << ToByteVector(nExpectedHash);
+    CScript findScript = CScript() << OP_RETURN << ToByteVector(nExpectedHash);
+    CAmount expectedAmount = fBudgetFinalization ?  BUDGET_FEE_TX : PROPOSAL_FEE_TX;
 
     bool foundOpReturn = false;
     for (const CTxOut &o : txCollateral->vout) {
         if (!o.scriptPubKey.IsPayToPublicKeyHash() && !o.scriptPubKey.IsUnspendable()) {
             strError = strprintf("Invalid Script %s", txCollateral->ToString());
             return false;
         }
-        if (fBudgetFinalization) {
-            // Collateral for budget finalization
-            // Note: there are still old valid budgets out there, but the check for the new 5 PIV finalization collateral
-            //       will also cover the old 50 PIV finalization collateral.
-            LogPrint(BCLog::MNBUDGET, "Final Budget: o.scriptPubKey(%s) == findScript(%s) ?\n", HexStr(o.scriptPubKey), HexStr(findScript));
-            if (o.scriptPubKey == findScript) {
-                LogPrint(BCLog::MNBUDGET, "Final Budget: o.nValue(%ld) >= BUDGET_FEE_TX(%ld) ?\n", o.nValue, BUDGET_FEE_TX);
-                if(o.nValue >= BUDGET_FEE_TX) {
-                    foundOpReturn = true;
-                    break;
-                }
-            }
-        } else {
-            // Collateral for normal budget proposal
-            LogPrint(BCLog::MNBUDGET, "Normal Budget: o.scriptPubKey(%s) == findScript(%s) ?\n", HexStr(o.scriptPubKey), HexStr(findScript));
-            if (o.scriptPubKey == findScript) {
-                LogPrint(BCLog::MNBUDGET, "Normal Budget: o.nValue(%ld) >= PROPOSAL_FEE_TX(%ld) ?\n", o.nValue, PROPOSAL_FEE_TX);
-                if(o.nValue >= PROPOSAL_FEE_TX) {
-                    foundOpReturn = true;
-                    break;
-                }
-            }
+        if (o.scriptPubKey == findScript && o.nValue == expectedAmount) {
+            foundOpReturn = true;
+            break;
         }
     }
 
@@ -1669,10 +1598,6 @@ bool CheckCollateral(const uint256& nTxCollateralHash, const uint256& nExpectedH
 
     // Retrieve block height (checking that it's in the active chain) and time
     // both get set in CBudgetProposal/CFinalizedBudget by the caller (AddProposal/AddFinalizedBudget)
-    if (nBlockHash.IsNull()) {
-        strError = strprintf("Collateral transaction %s is unconfirmed", nTxCollateralHash.ToString());
-        return false;
-    }
     nTime = 0;
     int nProposalHeight = 0;
     {

src/budget/budgetmanager.h

@@ -93,15 +93,9 @@ class CBudgetManager : public CValidationInterface
     CDataStream GetFinalizedBudgetVoteSerialized(const uint256& voteHash) const;
     CDataStream GetFinalizedBudgetSerialized(const uint256& budgetHash) const;
 
-    bool AddAndRelayProposalVote(const CBudgetVote& vote, std::string& strError);
-
     // sets strProposal of a CFinalizedBudget reference
     void SetBudgetProposalsStr(CFinalizedBudget& finalizedBudget) const;
 
-    // checks finalized budget proposals (existence, payee, amount) for the finalized budget
-    // in the map, with given nHash. Returns error string if any, or "OK" otherwise
-    std::string GetFinalizedBudgetStatus(const uint256& nHash) const;
-
     void ResetSync() { SetSynced(false); }
     void MarkSynced() { SetSynced(true); }
     // Respond to full budget sync requests and internally triggered partial budget items relay
@@ -128,7 +122,6 @@ class CBudgetManager : public CValidationInterface
     CFinalizedBudget* FindFinalizedBudget(const uint256& nHash);
     // const functions, copying the budget object to a reference and returning true if found
     bool GetProposal(const uint256& nHash, CBudgetProposal& bp) const;
-    bool GetFinalizedBudget(const uint256& nHash, CFinalizedBudget& fb) const;
     // finds the proposal with the given name, with highest net yes count.
     const CBudgetProposal* FindProposalByName(const std::string& strProposalName) const;
 

src/budget/budgetproposal.cpp

@@ -235,15 +235,6 @@ bool CBudgetProposal::AddOrUpdateVote(const CBudgetVote& vote, std::string& strE
     return true;
 }
 
-UniValue CBudgetProposal::GetVotesArray() const
-{
-    UniValue ret(UniValue::VARR);
-    for (const auto& it: mapVotes) {
-        ret.push_back(it.second.ToJSON());
-    }
-    return ret;
-}
-
 void CBudgetProposal::SetSynced(bool synced)
 {
     for (auto& it: mapVotes) {

src/budget/budgetproposal.h

@@ -61,7 +61,6 @@ class CBudgetProposal
     CBudgetProposal(const std::string& name, const std::string& url, int paycount, const CScript& payee, const CAmount& amount, int blockstart, const uint256& nfeetxhash);
 
     bool AddOrUpdateVote(const CBudgetVote& vote, std::string& strError);
-    UniValue GetVotesArray() const;
     void SetSynced(bool synced);    // sets fSynced on votes (true only if valid)
 
     // sync proposal votes with a node

src/budget/finalizedbudget.cpp

@@ -78,16 +78,6 @@ bool CFinalizedBudget::AddOrUpdateVote(const CFinalizedBudgetVote& vote, std::st
     return true;
 }
 
-UniValue CFinalizedBudget::GetVotesObject() const
-{
-    UniValue ret(UniValue::VOBJ);
-    for (const auto& it: mapVotes) {
-        const CFinalizedBudgetVote& vote = it.second;
-        ret.pushKV(vote.GetVin().prevout.ToStringShort(), vote.ToJSON());
-    }
-    return ret;
-}
-
 void CFinalizedBudget::SetSynced(bool synced)
 {
     for (auto& it: mapVotes) {

src/budget/finalizedbudget.h

@@ -60,7 +60,7 @@ class CFinalizedBudget
     CFinalizedBudget(const std::string& name, int blockstart, const std::vector<CTxBudgetPayment>& vecBudgetPaymentsIn, const uint256& nfeetxhash);
 
     bool AddOrUpdateVote(const CFinalizedBudgetVote& vote, std::string& strError);
-    UniValue GetVotesObject() const;
+    std::map<COutPoint, CFinalizedBudgetVote> GetVotes() const { return mapVotes; }
     void SetSynced(bool synced);    // sets fSynced on votes (true only if valid)
 
     // sync budget votes with a node

src/rpc/budget.cpp

@@ -331,8 +331,12 @@ UniValue getbudgetvotes(const JSONRPCRequest& request)
 
     std::string strProposalName = SanitizeString(request.params[0].get_str());
     const CBudgetProposal* pbudgetProposal = g_budgetman.FindProposalByName(strProposalName);
-    if (pbudgetProposal == NULL) throw std::runtime_error("Unknown proposal name");
-    return pbudgetProposal->GetVotesArray();
+    if (pbudgetProposal == nullptr) throw std::runtime_error("Unknown proposal name");
+    UniValue ret(UniValue::VARR);
+    for (const auto& it : pbudgetProposal->GetVotes()) {
+        ret.push_back(it.second.ToJSON());
+    }
+    return ret;
 }
 
 UniValue getnextsuperblock(const JSONRPCRequest& request)
@@ -523,11 +527,11 @@ UniValue mnbudgetrawvote(const JSONRPCRequest& request)
         return "Failure to verify signature.";
     }
 
-    std::string strError;
-    if (g_budgetman.AddAndRelayProposalVote(vote, strError)) {
+    CValidationState state;
+    if (g_budgetman.ProcessProposalVote(vote, nullptr, state)) {
         return "Voted successfully";
     } else {
-        return "Error voting : " + strError;
+        return "Error voting : " + state.GetRejectReason() + ". " + state.GetDebugMessage();
     }
 }
 
@@ -634,6 +638,39 @@ UniValue createrawmnfinalbudget(const JSONRPCRequest& request)
     return ret;
 }
 
+static std::string GetFinalizedBudgetStatus(CFinalizedBudget* fb)
+{
+    std::string retBadHashes;
+    std::string retBadPayeeOrAmount;
+    int nBlockStart = fb->GetBlockStart();
+    int nBlockEnd = fb->GetBlockEnd();
+
+    for (int nBlockHeight = nBlockStart; nBlockHeight <= nBlockEnd; nBlockHeight++) {
+        CTxBudgetPayment budgetPayment;
+        if (!fb->GetBudgetPaymentByBlock(nBlockHeight, budgetPayment)) {
+            LogPrint(BCLog::MNBUDGET,"%s: Couldn't find budget payment for block %lld\n", __func__, nBlockHeight);
+            continue;
+        }
+
+        CBudgetProposal bp;
+        if (!g_budgetman.GetProposal(budgetPayment.nProposalHash, bp)) {
+            retBadHashes += (retBadHashes.empty() ? "" : ", ") + budgetPayment.nProposalHash.ToString();
+            continue;
+        }
+
+        if (bp.GetPayee() != budgetPayment.payee || bp.GetAmount() != budgetPayment.nAmount) {
+            retBadPayeeOrAmount += (retBadPayeeOrAmount.empty() ? "" : ", ") + budgetPayment.nProposalHash.ToString();
+        }
+    }
+
+    if (retBadHashes.empty() && retBadPayeeOrAmount == "") return "OK";
+
+    if (!retBadHashes.empty()) retBadHashes = "Unknown proposal(s) hash! Check this proposal(s) before voting: " + retBadHashes;
+    if (!retBadPayeeOrAmount.empty()) retBadPayeeOrAmount = "Budget payee/nAmount doesn't match our proposal(s)! "+ retBadPayeeOrAmount;
+
+    return retBadHashes + " -- " + retBadPayeeOrAmount;
+}
+
 UniValue mnfinalbudget(const JSONRPCRequest& request)
 {
     std::string strCommand;
@@ -682,7 +719,7 @@ UniValue mnfinalbudget(const JSONRPCRequest& request)
             bObj.pushKV("BlockEnd", (int64_t)finalizedBudget->GetBlockEnd());
             bObj.pushKV("Proposals", finalizedBudget->GetProposalsStr());
             bObj.pushKV("VoteCount", (int64_t)finalizedBudget->GetVoteCount());
-            bObj.pushKV("Status", g_budgetman.GetFinalizedBudgetStatus(nHash));
+            bObj.pushKV("Status", GetFinalizedBudgetStatus(finalizedBudget));
 
             bool fValid = finalizedBudget->IsValid();
             bObj.pushKV("IsValid", fValid);
@@ -704,8 +741,13 @@ UniValue mnfinalbudget(const JSONRPCRequest& request)
 
         LOCK(g_budgetman.cs_budgets);
         CFinalizedBudget* pfinalBudget = g_budgetman.FindFinalizedBudget(hash);
-        if (pfinalBudget == NULL) return "Unknown budget hash";
-        return pfinalBudget->GetVotesObject();
+        if (pfinalBudget == nullptr) return "Unknown budget hash";
+        UniValue ret(UniValue::VOBJ);
+        for (const auto& it: pfinalBudget->GetVotes()) {
+            const CFinalizedBudgetVote& vote = it.second;
+            ret.pushKV(vote.GetVin().prevout.ToStringShort(), vote.ToJSON());
+        }
+        return ret;
     }
 
     return NullUniValue;