Avoid overriding validation states, return more specific states in some cases (#3541)

Also, add comments clarifying why the state isn't assigned in return-s.

src/evo/cbtx.cpp

@@ -72,7 +72,8 @@ bool CheckCbTxMerkleRoots(const CBlock& block, const CBlockIndex* pindex, CValid
     if (pindex) {
         uint256 calculatedMerkleRoot;
         if (!CalcCbTxMerkleRootMNList(block, pindex->pprev, calculatedMerkleRoot, state)) {
-            return state.DoS(100, false, REJECT_INVALID, "bad-cbtx-mnmerkleroot");
+            // pass the state returned by the function above
+            return false;
         }
         if (calculatedMerkleRoot != cbTx.merkleRootMNList) {
             return state.DoS(100, false, REJECT_INVALID, "bad-cbtx-mnmerkleroot");
@@ -83,7 +84,8 @@ bool CheckCbTxMerkleRoots(const CBlock& block, const CBlockIndex* pindex, CValid
 
         if (cbTx.nVersion >= 2) {
             if (!CalcCbTxMerkleRootQuorums(block, pindex->pprev, calculatedMerkleRoot, state)) {
-                return state.DoS(100, false, REJECT_INVALID, "bad-cbtx-quorummerkleroot");
+                // pass the state returned by the function above
+                return false;
             }
             if (calculatedMerkleRoot != cbTx.merkleRootQuorums) {
                 return state.DoS(100, false, REJECT_INVALID, "bad-cbtx-quorummerkleroot");
@@ -111,6 +113,7 @@ bool CalcCbTxMerkleRootMNList(const CBlock& block, const CBlockIndex* pindexPrev
     try {
         CDeterministicMNList tmpMNList;
         if (!deterministicMNManager->BuildNewListFromBlock(block, pindexPrev, state, tmpMNList, false)) {
+            // pass the state returned by the function above
             return false;
         }
 
@@ -128,7 +131,10 @@ bool CalcCbTxMerkleRootMNList(const CBlock& block, const CBlockIndex* pindexPrev
 
         if (sml.mnList == smlCached.mnList) {
             merkleRootRet = merkleRootCached;
-            return !mutatedCached;
+            if (mutatedCached) {
+                return state.DoS(100, false, REJECT_INVALID, "mutated-cached-calc-cb-mnmerkleroot");
+            }
+            return true;
         }
 
         bool mutated = false;
@@ -141,7 +147,11 @@ bool CalcCbTxMerkleRootMNList(const CBlock& block, const CBlockIndex* pindexPrev
         merkleRootCached = merkleRootRet;
         mutatedCached = mutated;
 
-        return !mutated;
+        if (mutated) {
+            return state.DoS(100, false, REJECT_INVALID, "mutated-calc-cb-mnmerkleroot");
+        }
+
+        return true;
     } catch (const std::exception& e) {
         LogPrintf("%s -- failed: %s\n", __func__, e.what());
         return state.DoS(100, false, REJECT_INVALID, "failed-calc-cb-mnmerkleroot");
@@ -198,7 +208,7 @@ bool CalcCbTxMerkleRootQuorums(const CBlock& block, const CBlockIndex* pindexPre
         if (tx->nVersion == 3 && tx->nType == TRANSACTION_QUORUM_COMMITMENT) {
             llmq::CFinalCommitmentTxPayload qc;
             if (!GetTxPayload(*tx, qc)) {
-                return state.DoS(100, false, REJECT_INVALID, "bad-qc-payload");
+                return state.DoS(100, false, REJECT_INVALID, "bad-qc-payload-calc-cbtx-quorummerkleroot");
             }
             if (qc.commitment.IsNull()) {
                 continue;
@@ -215,7 +225,7 @@ bool CalcCbTxMerkleRootQuorums(const CBlock& block, const CBlockIndex* pindexPre
             v.emplace_back(qcHash);
             hashCount++;
             if (v.size() > params.signingActiveQuorumCount) {
-                return state.DoS(100, false, REJECT_INVALID, "excess-quorums");
+                return state.DoS(100, false, REJECT_INVALID, "excess-quorums-calc-cbtx-quorummerkleroot");
             }
         }
     }
@@ -239,7 +249,11 @@ bool CalcCbTxMerkleRootQuorums(const CBlock& block, const CBlockIndex* pindexPre
     int64_t nTime5 = GetTimeMicros(); nTimeMerkle += nTime5 - nTime4;
     LogPrint(BCLog::BENCHMARK, "            - ComputeMerkleRoot: %.2fms [%.2fs]\n", 0.001 * (nTime5 - nTime4), nTimeMerkle * 0.000001);
 
-    return !mutated;
+    if (mutated) {
+        return state.DoS(100, false, REJECT_INVALID, "mutated-calc-cbtx-quorummerkleroot");
+    }
+
+    return true;
 }
 
 std::string CCbTx::ToString() const

src/evo/deterministicmns.cpp

@@ -549,6 +549,7 @@ bool CDeterministicMNManager::ProcessBlock(const CBlock& block, const CBlockInde
         LOCK(cs);
 
         if (!BuildNewListFromBlock(block, pindex->pprev, _state, newList, true)) {
+            // pass the state returned by the function above
             return false;
         }
 

src/evo/providertx.cpp

@@ -124,6 +124,7 @@ bool CheckProRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValid
     // It's allowed to set addr to 0, which will put the MN into PoSe-banned state and require a ProUpServTx to be issues later
     // If any of both is set, it must be valid however
     if (ptx.addr != CService() && !CheckService(tx.GetHash(), ptx, state)) {
+        // pass the state returned by the function above
         return false;
     }
 
@@ -201,6 +202,7 @@ bool CheckProRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValid
     if (keyForPayloadSig) {
         // collateral is not part of this ProRegTx, so we must verify ownership of the collateral
         if (!CheckStringSig(ptx, *keyForPayloadSig, state)) {
+            // pass the state returned by the function above
             return false;
         }
     } else {
@@ -229,6 +231,7 @@ bool CheckProUpServTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CVa
     }
 
     if (!CheckService(ptx.proTxHash, ptx, state)) {
+        // pass the state returned by the function above
         return false;
     }
 
@@ -256,9 +259,11 @@ bool CheckProUpServTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CVa
 
         // we can only check the signature if pindexPrev != nullptr and the MN is known
         if (!CheckInputsHash(tx, ptx, state)) {
+            // pass the state returned by the function above
             return false;
         }
         if (!CheckHashSig(ptx, mn->pdmnState->pubKeyOperator.Get(), state)) {
+            // pass the state returned by the function above
             return false;
         }
     }
@@ -338,9 +343,11 @@ bool CheckProUpRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CVal
         }
 
         if (!CheckInputsHash(tx, ptx, state)) {
+            // pass the state returned by the function above
             return false;
         }
         if (!CheckHashSig(ptx, dmn->pdmnState->keyIDOwner, state)) {
+            // pass the state returned by the function above
             return false;
         }
     }
@@ -375,10 +382,14 @@ bool CheckProUpRevTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CVal
         if (!dmn)
             return state.DoS(100, false, REJECT_INVALID, "bad-protx-hash");
 
-        if (!CheckInputsHash(tx, ptx, state))
+        if (!CheckInputsHash(tx, ptx, state)) {
+            // pass the state returned by the function above
             return false;
-        if (!CheckHashSig(ptx, dmn->pdmnState->pubKeyOperator.Get(), state))
+        }
+        if (!CheckHashSig(ptx, dmn->pdmnState->pubKeyOperator.Get(), state)) {
+            // pass the state returned by the function above
             return false;
+        }
     }
 
     return true;

src/evo/specialtx.cpp

@@ -104,9 +104,11 @@ bool ProcessSpecialTxsInBlock(const CBlock& block, const CBlockIndex* pindex, CV
         for (int i = 0; i < (int)block.vtx.size(); i++) {
             const CTransaction& tx = *block.vtx[i];
             if (!CheckSpecialTx(tx, pindex->pprev, state)) {
+                // pass the state returned by the function above
                 return false;
             }
             if (!ProcessSpecialTx(tx, pindex, state)) {
+                // pass the state returned by the function above
                 return false;
             }
         }
@@ -115,6 +117,7 @@ bool ProcessSpecialTxsInBlock(const CBlock& block, const CBlockIndex* pindex, CV
         LogPrint(BCLog::BENCHMARK, "        - Loop: %.2fms [%.2fs]\n", 0.001 * (nTime2 - nTime1), nTimeLoop * 0.000001);
 
         if (!llmq::quorumBlockProcessor->ProcessBlock(block, pindex, state)) {
+            // pass the state returned by the function above
             return false;
         }
 
@@ -122,6 +125,7 @@ bool ProcessSpecialTxsInBlock(const CBlock& block, const CBlockIndex* pindex, CV
         LogPrint(BCLog::BENCHMARK, "        - quorumBlockProcessor: %.2fms [%.2fs]\n", 0.001 * (nTime3 - nTime2), nTimeQuorum * 0.000001);
 
         if (!deterministicMNManager->ProcessBlock(block, pindex, state, fJustCheck)) {
+            // pass the state returned by the function above
             return false;
         }
 
@@ -129,13 +133,15 @@ bool ProcessSpecialTxsInBlock(const CBlock& block, const CBlockIndex* pindex, CV
         LogPrint(BCLog::BENCHMARK, "        - deterministicMNManager: %.2fms [%.2fs]\n", 0.001 * (nTime4 - nTime3), nTimeDMN * 0.000001);
 
         if (fCheckCbTxMerleRoots && !CheckCbTxMerkleRoots(block, pindex, state)) {
+            // pass the state returned by the function above
             return false;
         }
 
         int64_t nTime5 = GetTimeMicros(); nTimeMerkle += nTime5 - nTime4;
         LogPrint(BCLog::BENCHMARK, "        - CheckCbTxMerkleRoots: %.2fms [%.2fs]\n", 0.001 * (nTime5 - nTime4), nTimeMerkle * 0.000001);
     } catch (const std::exception& e) {
-        return error(strprintf("%s -- failed: %s\n", __func__, e.what()).c_str());
+        LogPrintf(strprintf("%s -- failed: %s\n", __func__, e.what()).c_str());
+        return state.DoS(100, false, REJECT_INVALID, "failed-procspectxsinblock");
     }
 
     return true;