[Refactor] Decouple CBudgetProposal::UpdateValid checks

random-zebra · random-zebra · commit 12d411f0353f · 2020-10-03T00:25:19.000+02:00
diff --git a/src/masternode-budget.cpp b/src/masternode-budget.cpp
@@ -1387,14 +1387,17 @@ void CBudgetProposal::SyncVotes(CNode* pfrom, bool fPartial, int& nInvCount) con
     }
 }
 
-bool CBudgetProposal::UpdateValid(int nCurrentHeight, bool fCheckCollateral)
+bool CBudgetProposal::IsHeavilyDownvoted()
 {
-    fValid = false;
     if (GetNays() - GetYeas() > mnodeman.CountEnabled(ActiveProtocol()) / 10) {
         strInvalid = "Proposal " + strProposalName + ": Active removal";
-        return false;
+        return true;
     }
+    return false;
+}
 
+bool CBudgetProposal::CheckStartEnd()
+{
     if (nBlockStart < 0) {
         strInvalid = "Invalid Proposal";
         return false;
@@ -1405,23 +1408,38 @@ bool CBudgetProposal::UpdateValid(int nCurrentHeight, bool fCheckCollateral)
         return false;
     }
 
+    int nProposalEnd = nBlockStart + (Params().GetConsensus().nBudgetCycleBlocks + 1) * GetTotalPaymentCount();
+    if (nBlockEnd != nProposalEnd) {
+        strInvalid = "Proposal " + strProposalName + ": Invalid nBlockEnd (mismatch with payments count)";
+        return false;
+    }
+
+    return true;
+}
+
+bool CBudgetProposal::CheckAmount(const CAmount& nTotalBudget)
+{
+    // check minimum amount
     if (nAmount < 10 * COIN) {
-        strInvalid = "Proposal " + strProposalName + ": Invalid nAmount";
+        strInvalid = "Proposal " + strProposalName + ": Invalid nAmount (too low)";
         return false;
     }
 
-    if (address == CScript()) {
-        strInvalid = "Proposal " + strProposalName + ": Invalid Payment Address";
+    // check maximum amount
+    // can only pay out 10% of the possible coins (min value of coins)
+    if (nAmount > nTotalBudget) {
+        strInvalid = "Proposal " + strProposalName + ": Invalid nAmount (too high)";
         return false;
     }
 
-    if (fCheckCollateral) {
-        int nConf = 0;
-        std::string strError;
-        if (!IsBudgetCollateralValid(nFeeTXHash, GetHash(), strError, nTime, nConf)) {
-            strInvalid = "Proposal " + strProposalName + ": Invalid collateral (" + strError + ")";
-            return false;
-        }
+    return true;
+}
+
+bool CBudgetProposal::CheckAddress()
+{
+    if (address == CScript()) {
+        strInvalid = "Proposal " + strProposalName + ": Invalid Payment Address (null)";
+        return false;
     }
 
     /*
@@ -1432,35 +1450,49 @@ bool CBudgetProposal::UpdateValid(int nCurrentHeight, bool fCheckCollateral)
         return false;
     }
 
-    //if proposal doesn't gain traction within 2 weeks, remove it
-    // nTime not being saved correctly
-    // -- TODO: We should keep track of the last time the proposal was valid, if it's invalid for 2 weeks, erase it
-    // if(nTime + (60*60*24*2) < GetAdjustedTime()) {
-    //     if(GetYeas()-GetNays() < (mnodeman.CountEnabled(ActiveProtocol())/10)) {
-    //         strError = "Not enough support";
-    //         return false;
-    //     }
-    // }
+    return true;
+}
 
-    //can only pay out 10% of the possible coins (min value of coins)
-    if (nAmount > budget.GetTotalBudget(nBlockStart)) {
-        strInvalid = "Proposal " + strProposalName + ": Payment more than max";
-        return false;
+bool CBudgetProposal::IsWellFormed(const CAmount& nTotalBudget)
+{
+    return CheckStartEnd() && CheckAmount(nTotalBudget) && CheckAddress();
+}
+
+bool CBudgetProposal::IsExpired(int nCurrentHeight)
+{
+    if (nBlockEnd < nCurrentHeight) {
+        strInvalid = "Proposal " + strProposalName + ": Proposal expired";
+        return true;
     }
+    return false;
+}
 
-    // Calculate maximum block this proposal will be valid, which is start of proposal + (number of payments * cycle)
-    int nProposalEnd = GetBlockStart() + (Params().GetConsensus().nBudgetCycleBlocks * GetTotalPaymentCount());
+bool CBudgetProposal::UpdateValid(int nCurrentHeight, bool fCheckCollateral)
+{
+    fValid = false;
 
-    if (nCurrentHeight <= 0) {
-        strInvalid = "Proposal " + strProposalName + ": Tip is NULL";
-        return true;
+    if (IsHeavilyDownvoted()) {
+        return false;
+    }
+
+    // Checks that don't change. !TODO: remove from here, they should be done only once.
+    if (!IsWellFormed(budget.GetTotalBudget(nBlockStart))) {
+        return false;
     }
 
-    if(nProposalEnd < nCurrentHeight) {
-        strInvalid = "Proposal " + strProposalName + ": Invalid nBlockEnd (" + std::to_string(nProposalEnd) + ") < current height (" + std::to_string(nCurrentHeight) + ")";
+    if (IsExpired(nCurrentHeight)) {
         return false;
     }
 
+    if (fCheckCollateral) {
+        int nConf = 0;
+        std::string strError;
+        if (!IsBudgetCollateralValid(nFeeTXHash, GetHash(), strError, nTime, nConf)) {
+            strInvalid = "Proposal " + strProposalName + ": Invalid collateral (" + strError + ")";
+            return false;
+        }
+    }
+
     fValid = true;
     strInvalid.clear();
     return true;
diff --git a/src/masternode-budget.h b/src/masternode-budget.h
@@ -540,6 +540,15 @@ class CBudgetProposal
     bool fValid;
     std::string strInvalid;
 
+    // Functions used inside UpdateValid() (!TODO: move) - setting strInvalid
+    bool IsHeavilyDownvoted();
+    bool IsExpired(int nCurrentHeight);
+    bool CheckStartEnd();
+    bool CheckAmount(const CAmount& nTotalBudget);
+    bool CheckAddress();
+    // Static checks that should be done only once
+    bool IsWellFormed(const CAmount& nTotalBudget);
+
 protected:
     std::map<uint256, CBudgetVote> mapVotes;
     std::string strProposalName;
