Merge pull request #220 from instagibbs/revertcheck

Revert ownership of CCheck to QueueCheck, return ScriptError

Author: instagibbs

src/script/script_error.h

@@ -77,7 +77,10 @@ typedef enum ScriptError_t
     SCRIPT_ERR_WITHDRAW_VERIFY_BLOCKCONFIRMED,
     SCRIPT_ERR_WITHDRAW_VERIFY_BLINDED_AMOUNTS,
 
-    SCRIPT_ERR_ERROR_COUNT
+    SCRIPT_ERR_ERROR_COUNT,
+
+    SCRIPT_ERR_RANGEPROOF,
+    SCRIPT_ERR_PEDERSEN_TALLY
 } ScriptError;
 
 #define SCRIPT_ERR_LAST SCRIPT_ERR_ERROR_COUNT

src/validation.cpp

@@ -618,15 +618,17 @@ class CSurjectionCheck : public CCheck
     bool operator()();
 };
 
-// Does *not* destroy the check in the case of no queue, or passes its ownership to the queue.
-static inline bool QueueCheck(std::vector<CCheck*>* queue, CCheck* check)
+// Destroys the check in the case of no queue, or passes its ownership to the queue.
+static inline ScriptError QueueCheck(std::vector<CCheck*>* queue, CCheck* check)
 {
     if (queue != NULL) {
         queue->push_back(check);
-        return true;
+        return SCRIPT_ERR_OK;
     }
-    bool ret = (*check)();
-    return ret;
+    bool success = (*check)();
+    ScriptError err = check->GetScriptError();
+    delete check;
+    return success ? SCRIPT_ERR_OK : err;
 }
 
 
@@ -636,13 +638,19 @@ bool CRangeCheck::operator()()
         return true;
     }
 
-    return CachingRangeProofChecker(store).VerifyRangeProof(rangeproof, val->vchCommitment, assetCommitment, scriptPubKey, secp256k1_ctx_verify_amounts);
+    if (!CachingRangeProofChecker(store).VerifyRangeProof(rangeproof, val->vchCommitment, assetCommitment, scriptPubKey, secp256k1_ctx_verify_amounts)) {
+        error = SCRIPT_ERR_RANGEPROOF;
+        return false;
+    }
+
+    return true;
 };
 
 bool CBalanceCheck::operator()()
 {
     if (!secp256k1_pedersen_verify_tally(secp256k1_ctx_verify_amounts, vpCommitsIn.data(), vpCommitsIn.size(), vpCommitsOut.data(), vpCommitsOut.size())) {
         fAmountError = true;
+        error = SCRIPT_ERR_PEDERSEN_TALLY;
         return false;
     }
 
@@ -822,7 +830,7 @@ bool VerifyAmounts(const CCoinsViewCache& cache, const CTransaction& tx, std::ve
                 p++;
 
                 // Rangecheck must be done for blinded amount
-                if (issuance.nAmount.IsCommitment() && !QueueCheck(pvChecks, new CRangeCheck(&issuance.nAmount, tx.wit.vtxinwit[i].vchIssuanceAmountRangeproof, issuanceAsset.vchCommitment, CScript(), cacheStore))) {
+                if (issuance.nAmount.IsCommitment() && QueueCheck(pvChecks, new CRangeCheck(&issuance.nAmount, tx.wit.vtxinwit[i].vchIssuanceAmountRangeproof, issuanceAsset.vchCommitment, CScript(), cacheStore)) != SCRIPT_ERR_OK) {
                     return false;
                 }
             }
@@ -863,7 +871,7 @@ bool VerifyAmounts(const CCoinsViewCache& cache, const CTransaction& tx, std::ve
                 vpCommitsIn.push_back(p);
                 p++;
 
-                if (issuance.nInflationKeys.IsCommitment() && !QueueCheck(pvChecks, new CRangeCheck(&issuance.nInflationKeys, tx.wit.vtxinwit[i].vchInflationKeysRangeproof, tokenAsset.vchCommitment, CScript(), cacheStore))) {
+                if (issuance.nInflationKeys.IsCommitment() && QueueCheck(pvChecks, new CRangeCheck(&issuance.nInflationKeys, tx.wit.vtxinwit[i].vchInflationKeysRangeproof, tokenAsset.vchCommitment, CScript(), cacheStore)) != SCRIPT_ERR_OK) {
                     return false;
                 }
             } else if (!issuance.nInflationKeys.IsNull()) {
@@ -925,7 +933,7 @@ bool VerifyAmounts(const CCoinsViewCache& cache, const CTransaction& tx, std::ve
     }
 
     // Check balance
-    if (!QueueCheck(pvChecks, new CBalanceCheck(vData, vpCommitsIn, vpCommitsOut))) {
+    if (QueueCheck(pvChecks, new CBalanceCheck(vData, vpCommitsIn, vpCommitsOut)) != SCRIPT_ERR_OK) {
         return false;
     }
 
@@ -949,7 +957,7 @@ bool VerifyAmounts(const CCoinsViewCache& cache, const CTransaction& tx, std::ve
         if (!ptxoutwit || ptxoutwit->vchRangeproof.size() > 5000) {
             return false;
         }
-        if (!QueueCheck(pvChecks, new CRangeCheck(&val, ptxoutwit->vchRangeproof, vchAssetCommitment, tx.vout[i].scriptPubKey, cacheStore))) {
+        if (QueueCheck(pvChecks, new CRangeCheck(&val, ptxoutwit->vchRangeproof, vchAssetCommitment, tx.vout[i].scriptPubKey, cacheStore)) != SCRIPT_ERR_OK) {
             return false;
         }
     }
@@ -976,7 +984,7 @@ bool VerifyAmounts(const CCoinsViewCache& cache, const CTransaction& tx, std::ve
         if (secp256k1_surjectionproof_parse(secp256k1_ctx_verify_amounts, &proof, &ptxoutwit->vchSurjectionproof[0], ptxoutwit->vchSurjectionproof.size()) != 1)
             return false;
 
-        if (!QueueCheck(pvChecks, new CSurjectionCheck(proof, targetGenerators, gen, cacheStore))) {
+        if (QueueCheck(pvChecks, new CSurjectionCheck(proof, targetGenerators, gen, cacheStore)) != SCRIPT_ERR_OK) {
             return false;
         }
         // Each CSurjectionCheck uses swap to keep pointers valid.
@@ -2079,7 +2087,8 @@ bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsVi
 
                 // Verify signature
                 CCheck* check = new CScriptCheck(*coins, tx, i, prevValueIn, flags, cacheStore, &txdata);
-                if (!QueueCheck(pvChecks, check)) {
+                ScriptError serror = QueueCheck(pvChecks, check);
+                if (serror != SCRIPT_ERR_OK) {
                     if (flags & STANDARD_NOT_MANDATORY_VERIFY_FLAGS) {
                         // Check whether the failure was caused by a
                         // non-mandatory script verification check, such as
@@ -2090,7 +2099,7 @@ bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsVi
                         CScriptCheck check2(*coins, tx, i, prevValueIn,
                                 flags & ~STANDARD_NOT_MANDATORY_VERIFY_FLAGS, cacheStore, &txdata);
                         if (check2())
-                            return state.Invalid(false, REJECT_NONSTANDARD, strprintf("non-mandatory-script-verify-flag (%s)", ScriptErrorString(check->GetScriptError())));
+                            return state.Invalid(false, REJECT_NONSTANDARD, strprintf("non-mandatory-script-verify-flag (%s)", ScriptErrorString(serror)));
                     }
                     // Failures of other flags indicate a transaction that is
                     // invalid in new blocks, e.g. a invalid P2SH. We DoS ban
@@ -2099,14 +2108,6 @@ bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsVi
                     // as to the correct behavior - we may want to continue
                     // peering with non-upgraded nodes even after soft-fork
                     // super-majority signaling has occurred.
-
-                    // If the check was deleted already, we have no idea what is
-                    // wrong with it. This should only happen during ConnectBlock
-                    // with a non-NULL queue. Since this code is only reached in
-                    // the one-shot mempool case it will never be deleted.
-                    assert(check);
-                    ScriptError serror = check->GetScriptError();
-                    delete check;
                     if (serror == SCRIPT_ERR_WITHDRAW_VERIFY_BLOCKCONFIRMED)
                         return state.Invalid(false, REJECT_SCRIPT, strprintf("mandatory-script-verify-flag-failed (%s)", ScriptErrorString(serror)));
                     else