Refactor IsBlockValueValid (#1177)

* refactor IsBlockValueValid to return actual error string, use it in error message for bad-cb-amount

* make error messages in IsBlockValueValid even more verbose

src/main.cpp

@@ -2773,12 +2773,9 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin
 
     // DASH : MODIFYED TO CHECK MASTERNODE PAYMENTS AND SUPERBLOCKS
     CAmount blockReward = nFees + GetBlockSubsidy(pindex->pprev->nBits, pindex->pprev->nHeight, chainparams.GetConsensus());
-    if (!IsBlockValueValid(block, pindex->nHeight, blockReward)) {
+    std::string strError = "";
-        LogPrintf("ConnectBlock() -- IsBlockValueValid returned false, nHeight = %d\n", pindex->nHeight);
+    if (!IsBlockValueValid(block, pindex->nHeight, blockReward, strError)) {
-        // TODO: handle error here more accurate - this could actually fail for different reasons
+        return state.DoS(100, error("ConnectBlock(): %s", strError), REJECT_INVALID, "bad-cb-amount");
-        return state.DoS(100, error("ConnectBlock(): coinbase pays too much (actual=%d vs limit=%d)",
-                                block.vtx[0].GetValueOut(), blockReward),
-                                REJECT_INVALID, "bad-cb-amount");
     }
 
     if (!IsBlockPayeeValid(block.vtx[0], pindex->nHeight, blockReward)) {

src/masternode-payments.cpp

@@ -33,8 +33,10 @@ CCriticalSection cs_mapMasternodePaymentVotes;
 *   - When non-superblocks are detected, the normal schedule should be maintained
 */
 
-bool IsBlockValueValid(const CBlock& block, int nBlockHeight, CAmount blockReward)
+bool IsBlockValueValid(const CBlock& block, int nBlockHeight, CAmount blockReward, std::string &strErrorRet)
 {
+    strErrorRet = "";
+
     bool isNormalBlockValueMet = (block.vtx[0].GetValueOut() <= blockReward);
     if(fDebug) LogPrintf("block.vtx[0].GetValueOut() %lld <= blockReward %lld\n", block.vtx[0].GetValueOut(), blockReward);
 
@@ -51,6 +53,10 @@ bool IsBlockValueValid(const CBlock& block, int nBlockHeight, CAmount blockRewar
             if(masternodeSync.IsSynced() && !sporkManager.IsSporkActive(SPORK_13_OLD_SUPERBLOCK_FLAG)) {
                 // no budget blocks should be accepted here, if SPORK_13_OLD_SUPERBLOCK_FLAG is disabled
                 LogPrint("gobject", "IsBlockValueValid -- Client synced but budget spork is disabled, checking block value against normal block reward\n");
+                if(!isNormalBlockValueMet) {
+                    strErrorRet = strprintf("coinbase pays too much at height %d (actual=%d vs limit=%d), exceeded normal block payment limit, budgets are disabled",
+                                            nBlockHeight, block.vtx[0].GetValueOut(), blockReward);
+                }
                 return isNormalBlockValueMet;
             }
             LogPrint("gobject", "IsBlockValueValid -- WARNING: Skipping budget block value checks, accepting block\n");
@@ -58,6 +64,10 @@ bool IsBlockValueValid(const CBlock& block, int nBlockHeight, CAmount blockRewar
             return true;
         }
         // LogPrint("gobject", "IsBlockValueValid -- Block is not in budget cycle window, checking block value against normal block reward\n");
+        if(!isNormalBlockValueMet) {
+            strErrorRet = strprintf("coinbase pays too much at height %d (actual=%d vs limit=%d), exceeded normal block payment limit, block is not in budget cycle window",
+                                    nBlockHeight, block.vtx[0].GetValueOut(), blockReward);
+        }
         return isNormalBlockValueMet;
     }
 
@@ -72,8 +82,16 @@ bool IsBlockValueValid(const CBlock& block, int nBlockHeight, CAmount blockRewar
         // not enough data but at least it must NOT exceed superblock max value
         if(CSuperblock::IsValidBlockHeight(nBlockHeight)) {
             if(fDebug) LogPrintf("IsBlockPayeeValid -- WARNING: Client not synced, checking superblock max bounds only\n");
+            if(!isSuperblockMaxValueMet) {
+                strErrorRet = strprintf("coinbase pays too much at height %d (actual=%d vs limit=%d), exceeded superblock payment limit",
+                                        nBlockHeight, block.vtx[0].GetValueOut(), nSuperblockPaymentsLimit);
+            }
             return isSuperblockMaxValueMet;
         }
+        if(!isNormalBlockValueMet) {
+            strErrorRet = strprintf("coinbase pays too much at height %d (actual=%d vs limit=%d), exceeded normal block payment limit, only normal blocks are allowed at this height",
+                                    nBlockHeight, block.vtx[0].GetValueOut(), blockReward);
+        }
         // it MUST be a regular block otherwise
         return isNormalBlockValueMet;
     }
@@ -91,12 +109,21 @@ bool IsBlockValueValid(const CBlock& block, int nBlockHeight, CAmount blockRewar
             // triggered but invalid? that's weird
             LogPrintf("IsBlockValueValid -- ERROR: Invalid superblock detected at height %d: %s", nBlockHeight, block.vtx[0].ToString());
             // should NOT allow invalid superblocks, when superblocks are enabled
+            strErrorRet = strprintf("invalid superblock detected at height %d", nBlockHeight);
             return false;
         }
         LogPrint("gobject", "IsBlockValueValid -- No triggered superblock detected at height %d\n", nBlockHeight);
+        if(!isNormalBlockValueMet) {
+            strErrorRet = strprintf("coinbase pays too much at height %d (actual=%d vs limit=%d), exceeded normal block payment limit, no triggered superblock detected",
+                                    nBlockHeight, block.vtx[0].GetValueOut(), blockReward);
+        }
     } else {
         // should NOT allow superblocks at all, when superblocks are disabled
         LogPrint("gobject", "IsBlockValueValid -- Superblocks are disabled, no superblocks allowed\n");
+        if(!isNormalBlockValueMet) {
+            strErrorRet = strprintf("coinbase pays too much at height %d (actual=%d vs limit=%d), exceeded normal block payment limit, superblocks are disabled",
+                                    nBlockHeight, block.vtx[0].GetValueOut(), blockReward);
+        }
     }
 
     // it MUST be a regular block

src/masternode-payments.h

@@ -33,7 +33,7 @@ extern CCriticalSection cs_mapMasternodePayeeVotes;
 extern CMasternodePayments mnpayments;
 
 /// TODO: all 4 functions do not belong here really, they should be refactored/moved somewhere (main.cpp ?)
-bool IsBlockValueValid(const CBlock& block, int nBlockHeight, CAmount blockReward);
+bool IsBlockValueValid(const CBlock& block, int nBlockHeight, CAmount blockReward, std::string &strErrorRet);
 bool IsBlockPayeeValid(const CTransaction& txNew, int nBlockHeight, CAmount blockReward);
 void FillBlockPayments(CMutableTransaction& txNew, int nBlockHeight, CAmount blockReward, CTxOut& txoutMasternodeRet, std::vector<CTxOut>& voutSuperblockRet);
 std::string GetRequiredPaymentsString(int nBlockHeight);