diff --git a/src/bench/checkqueue.cpp b/src/bench/checkqueue.cpp index 04f5d9f6dc..10bd25b699 100644 --- a/src/bench/checkqueue.cpp +++ b/src/bench/checkqueue.cpp @@ -34,9 +34,9 @@ static void CCheckQueueSpeedPrevectorJob(benchmark::Bench& bench) explicit PrevectorJob(FastRandomContext& insecure_rand){ p.resize(insecure_rand.randrange(PREVECTOR_SIZE*2)); } - bool operator()() + std::optional operator()() { - return true; + return std::nullopt; } }; @@ -63,7 +63,7 @@ static void CCheckQueueSpeedPrevectorJob(benchmark::Bench& bench) } // control waits for completion by RAII, but // it is done explicitly here for clarity - control.Wait(); + control.Complete(); }); // ELEMENTS: deallocate vChecks diff --git a/src/checkqueue.h b/src/checkqueue.h index 91bf1d2add..ab1a30be33 100644 --- a/src/checkqueue.h +++ b/src/checkqueue.h @@ -11,19 +11,24 @@ #include #include +#include #include /** * Queue for verifications that have to be performed. * The verifications are represented by a type T, which must provide an - * operator(), returning a bool. + * operator(), returning an std::optional. + * + * The overall result of the computation is std::nullopt if all invocations + * return std::nullopt, or one of the other results otherwise. * * One thread (the master) is assumed to push batches of verifications * onto the queue, where they are processed by N-1 worker threads. When * the master is done adding work, it temporarily joins the worker pool * as an N'th worker, until all jobs are done. + * */ -template +template ()().value())>> class CCheckQueue { private: @@ -47,7 +52,7 @@ private: int nTotal GUARDED_BY(m_mutex){0}; //! The temporary evaluation result. - bool fAllOk GUARDED_BY(m_mutex){true}; + std::optional m_result GUARDED_BY(m_mutex); /** * Number of verifications that haven't completed yet. @@ -62,24 +67,28 @@ private: std::vector m_worker_threads; bool m_request_stop GUARDED_BY(m_mutex){false}; - /** Internal function that does bulk of the verification work. */ - bool Loop(bool fMaster) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) + /** Internal function that does bulk of the verification work. If fMaster, return the final result. */ + std::optional Loop(bool fMaster) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) { std::condition_variable& cond = fMaster ? m_master_cv : m_worker_cv; std::vector vChecks; vChecks.reserve(nBatchSize); unsigned int nNow = 0; - bool fOk = true; + std::optional local_result; + bool do_work; do { { WAIT_LOCK(m_mutex, lock); // first do the clean-up of the previous loop run (allowing us to do it in the same critsect) if (nNow) { - fAllOk &= fOk; + if (local_result.has_value() && !m_result.has_value()) { + std::swap(local_result, m_result); + } nTodo -= nNow; - if (nTodo == 0 && !fMaster) + if (nTodo == 0 && !fMaster) { // We processed the last element; inform the master it can exit and return the result m_master_cv.notify_one(); + } } else { // first iteration nTotal++; @@ -88,18 +97,19 @@ private: while (queue.empty() && !m_request_stop) { if (fMaster && nTodo == 0) { nTotal--; - bool fRet = fAllOk; + std::optional to_return = std::move(m_result); // reset the status for new work later - fAllOk = true; + m_result = std::nullopt; // return the current status - return fRet; + return to_return; } nIdle++; cond.wait(lock); // wait nIdle--; } if (m_request_stop) { - return false; + // return value does not matter, because m_request_stop is only set in the destructor. + return std::nullopt; } // Decide how many work units to process now. @@ -112,15 +122,15 @@ private: vChecks.assign(std::make_move_iterator(start_it), std::make_move_iterator(queue.end())); queue.erase(start_it, queue.end()); // Check whether we need to do work at all - fOk = fAllOk; + do_work = !m_result.has_value(); } // execute work - for (T* check : vChecks) { - assert(check); - if (fOk) { - fOk = (*check)(); + if (do_work) { + for (T* check : vChecks) { + local_result = (*check)(); + if (local_result.has_value()) break; + delete check; } - delete check; } vChecks.clear(); } while (true); @@ -150,8 +160,9 @@ public: CCheckQueue(CCheckQueue&&) = delete; CCheckQueue& operator=(CCheckQueue&&) = delete; - //! Wait until execution finishes, and return whether all evaluations were successful. - bool Wait() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) + //! Join the execution until completion. If at least one evaluation wasn't successful, return + //! its error. + std::optional Complete() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) { return Loop(true /* master thread */); } @@ -193,11 +204,11 @@ public: * RAII-style controller object for a CCheckQueue that guarantees the passed * queue is finished before continuing. */ -template +template ()().value())>> class CCheckQueueControl { private: - CCheckQueue* const pqueue; + CCheckQueue * const pqueue; bool fDone; public: @@ -212,13 +223,12 @@ public: } } - bool Wait() + std::optional Complete() { - if (pqueue == nullptr) - return true; - bool fRet = pqueue->Wait(); + if (pqueue == nullptr) return std::nullopt; + auto ret = pqueue->Complete(); fDone = true; - return fRet; + return ret; } void Add(std::vector vChecks) @@ -231,7 +241,7 @@ public: ~CCheckQueueControl() { if (!fDone) - Wait(); + Complete(); if (pqueue != nullptr) { LEAVE_CRITICAL_SECTION(pqueue->m_control_mutex); } diff --git a/src/confidential_validation.cpp b/src/confidential_validation.cpp index 5663256b18..359b93ea9f 100644 --- a/src/confidential_validation.cpp +++ b/src/confidential_validation.cpp @@ -25,6 +25,11 @@ public: static CSecp256k1Init instance_of_csecp256k1; } +template +inline bool is_ok(const std::optional& result) { + return !result.has_value(); +} + bool HasValidFee(const CTransaction& tx) { CAmountMap totalFee; for (unsigned int i = 0; i < tx.vout.size(); i++) { @@ -53,28 +58,36 @@ CAmountMap GetFeeMap(const CTransaction& tx) { return fee; } -bool CRangeCheck::operator()() { +std::optional> CRangeCheck::operator()() { assert(val->IsCommitment()); if (!CachingRangeProofChecker(store).VerifyRangeProof(rangeproof, val->vchCommitment, assetCommitment, scriptPubKey, secp256k1_ctx_verify_amounts)) { error = SCRIPT_ERR_RANGEPROOF; - return false; + std::string debug_str = "Range proof verification failed"; + return std::make_pair(error, std::move(debug_str)); } - return true; + return std::nullopt; }; -bool CBalanceCheck::operator()() { +std::optional> CBalanceCheck::operator()() { if (!secp256k1_pedersen_verify_tally(secp256k1_ctx_verify_amounts, vpCommitsIn.data(), vpCommitsIn.size(), vpCommitsOut.data(), vpCommitsOut.size())) { error = SCRIPT_ERR_PEDERSEN_TALLY; - return false; + std::string debug_str = "Balance check failed"; + return std::make_pair(error, std::move(debug_str)); } - return true; + return std::nullopt; } -bool CSurjectionCheck::operator()() { - return CachingSurjectionProofChecker(store).VerifySurjectionProof(proof, vTags, gen, secp256k1_ctx_verify_amounts, wtxid); +std::optional> CSurjectionCheck::operator()() { + if (!CachingSurjectionProofChecker(store).VerifySurjectionProof(proof, vTags, gen, secp256k1_ctx_verify_amounts, wtxid)) { + std::string debug_str = "Surjection check failed"; + error = SCRIPT_ERR_SURJECTION; + return std::make_pair(error, std::move(debug_str)); + } + + return std::nullopt; } // Destroys the check in the case of no queue, or passes its ownership to the queue. @@ -83,7 +96,7 @@ ScriptError QueueCheck(std::vector* queue, CCheck* check) { queue->push_back(check); return SCRIPT_ERR_OK; } - bool success = (*check)(); + bool success = is_ok((*check)()); ScriptError err = check->GetScriptError(); delete check; return success ? SCRIPT_ERR_OK : err; diff --git a/src/confidential_validation.h b/src/confidential_validation.h index 495fa9e18e..38c4908bca 100644 --- a/src/confidential_validation.h +++ b/src/confidential_validation.h @@ -34,7 +34,7 @@ class CCheck CCheck() = default; virtual ~CCheck() = default; - virtual bool operator()() = 0; + virtual std::optional> operator()() = 0; ScriptError GetScriptError() const { return error; } }; @@ -53,7 +53,7 @@ private: public: CRangeCheck(const CConfidentialValue* val_, const std::vector& rangeproof_, const std::vector& assetCommitment_, const CScript& scriptPubKey_, const bool storeIn) : val(val_), rangeproof(rangeproof_), assetCommitment(assetCommitment_), scriptPubKey(scriptPubKey_), store(storeIn) {} - bool operator()() override; + std::optional> operator()() override; }; /** Closure representing a transaction amount balance check. */ @@ -70,7 +70,7 @@ public: vpCommitsOut.swap(vpCommitsOut_); } - bool operator()() override; + std::optional> operator()() override; }; class CSurjectionCheck : public CCheck @@ -84,7 +84,7 @@ private: public: CSurjectionCheck(secp256k1_surjectionproof& proof_in, std::vector& tags_in, secp256k1_generator& gen_in, uint256& wtxid_in, const bool store_in) : proof(proof_in), vTags(tags_in), gen(gen_in), wtxid(wtxid_in), store(store_in) {} - bool operator()() override; + std::optional> operator()() override; }; ScriptError QueueCheck(std::vector* queue, CCheck* check); diff --git a/src/script/script_error.h b/src/script/script_error.h index 0285255e1e..cb83adccca 100644 --- a/src/script/script_error.h +++ b/src/script/script_error.h @@ -85,6 +85,7 @@ typedef enum ScriptError_t // ELEMENTS: SCRIPT_ERR_RANGEPROOF, SCRIPT_ERR_PEDERSEN_TALLY, + SCRIPT_ERR_SURJECTION, /* Elements: New tapscript related errors */ diff --git a/src/test/checkqueue_tests.cpp b/src/test/checkqueue_tests.cpp index 7acf0e93fa..f2e90f75bc 100644 --- a/src/test/checkqueue_tests.cpp +++ b/src/test/checkqueue_tests.cpp @@ -42,28 +42,26 @@ static const unsigned int QUEUE_BATCH_SIZE = 128; static const int SCRIPT_CHECK_THREADS = 3; struct FakeCheck { - bool operator()() const + std::optional operator()() const { - return true; + return std::nullopt; } }; struct FakeCheckCheckCompletion { static std::atomic n_calls; - bool operator()() + std::optional operator()() { n_calls.fetch_add(1, std::memory_order_relaxed); - return true; + return std::nullopt; } }; -struct FailingCheck { - bool fails; - FailingCheck(bool _fails) : fails(_fails){}; - bool operator()() const - { - return !fails; - } +struct FixedCheck +{ + std::optional m_result; + FixedCheck(std::optional result) : m_result(result){}; + std::optional operator()() const { return m_result; } }; struct UniqueCheck { @@ -71,11 +69,11 @@ struct UniqueCheck { static std::unordered_multiset results GUARDED_BY(m); size_t check_id; UniqueCheck(size_t check_id_in) : check_id(check_id_in){}; - bool operator()() + std::optional operator()() { LOCK(m); results.insert(check_id); - return true; + return std::nullopt; } }; @@ -83,9 +81,9 @@ struct UniqueCheck { struct MemoryCheck { static std::atomic fake_allocated_memory; bool b {false}; - bool operator()() const + std::optional operator()() const { - return true; + return std::nullopt; } MemoryCheck(const MemoryCheck& x) { @@ -110,9 +108,9 @@ struct FrozenCleanupCheck { static std::condition_variable cv; static std::mutex m; bool should_freeze{true}; - bool operator()() const + std::optional operator()() const { - return true; + return std::nullopt; } FrozenCleanupCheck() = default; ~FrozenCleanupCheck() @@ -149,7 +147,7 @@ std::atomic MemoryCheck::fake_allocated_memory{0}; // Queue Typedefs typedef CCheckQueue Correct_Queue; typedef CCheckQueue Standard_Queue; -typedef CCheckQueue Failing_Queue; +typedef CCheckQueue Fixed_Queue; typedef CCheckQueue Unique_Queue; typedef CCheckQueue Memory_Queue; typedef CCheckQueue FrozenCleanup_Queue; @@ -177,7 +175,7 @@ void CheckQueueTest::Correct_Queue_range(std::vector range) total -= vChecks.size(); control.Add(std::move(vChecks)); } - BOOST_REQUIRE(control.Wait()); + BOOST_REQUIRE(!control.Complete().has_value()); BOOST_REQUIRE_EQUAL(FakeCheckCheckCompletion::n_calls, i); } } @@ -220,28 +218,28 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_Correct_Random) } -/** Test that failing checks are caught */ +/** Test that distinct failing checks are caught */ BOOST_AUTO_TEST_CASE(test_CheckQueue_Catches_Failure) { - auto fail_queue = std::make_unique(QUEUE_BATCH_SIZE, SCRIPT_CHECK_THREADS); + auto fixed_queue = std::make_unique(QUEUE_BATCH_SIZE, SCRIPT_CHECK_THREADS); for (size_t i = 0; i < 1001; ++i) { - CCheckQueueControl control(fail_queue.get()); + CCheckQueueControl control(fixed_queue.get()); size_t remaining = i; while (remaining) { size_t r = m_rng.randrange(10); - std::vector vChecks; + std::vector vChecks; vChecks.reserve(r); for (size_t k = 0; k < r && remaining; k++, remaining--) { - vChecks.push_back(new FailingCheck(remaining == 1)); + vChecks.emplace_back(new FixedCheck(remaining == 1 ? std::make_optional(17 * i) : std::nullopt)); } control.Add(std::move(vChecks)); } - bool success = control.Wait(); + auto result = control.Complete(); if (i > 0) { - BOOST_REQUIRE(!success); - } else if (i == 0) { - BOOST_REQUIRE(success); + BOOST_REQUIRE(result.has_value() && *result == static_cast(17 * i)); + } else { + BOOST_REQUIRE(!result.has_value()); } } } @@ -249,20 +247,21 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_Catches_Failure) // future blocks, ie, the bad state is cleared. BOOST_AUTO_TEST_CASE(test_CheckQueue_Recovers_From_Failure) { - auto fail_queue = std::make_unique(QUEUE_BATCH_SIZE, SCRIPT_CHECK_THREADS); + auto fail_queue = std::make_unique(QUEUE_BATCH_SIZE, SCRIPT_CHECK_THREADS); for (auto times = 0; times < 10; ++times) { for (const bool end_fails : {true, false}) { - CCheckQueueControl control(fail_queue.get()); + CCheckQueueControl control(fail_queue.get()); { - std::vector vChecks; - for (size_t i = 0; i < 100; i++) { - vChecks.push_back(new FailingCheck(false)); + std::vector vChecks; + vChecks.reserve(100); + for (int i = 0; i < 100; ++i) { + vChecks.push_back(new FixedCheck(std::nullopt)); } delete vChecks[99]; - vChecks[99] = new FailingCheck(end_fails); + vChecks[99] = new FixedCheck(end_fails ? std::make_optional(2) : std::nullopt); control.Add(std::move(vChecks)); } - bool r =control.Wait(); + bool r = !control.Complete().has_value(); BOOST_REQUIRE(r != end_fails); } } @@ -342,8 +341,8 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_FrozenCleanup) vChecks.push_back(new FrozenCleanupCheck()); vChecks[0]->should_freeze = true; control.Add(std::move(vChecks)); - bool waitResult = control.Wait(); // Hangs here - assert(waitResult); + auto result = control.Complete(); // Hangs here + assert(!result); }); { std::unique_lock l(FrozenCleanupCheck::m); diff --git a/src/test/fuzz/checkqueue.cpp b/src/test/fuzz/checkqueue.cpp index c71f30be5c..3d3ba03c43 100644 --- a/src/test/fuzz/checkqueue.cpp +++ b/src/test/fuzz/checkqueue.cpp @@ -19,9 +19,10 @@ struct DumbCheck { { } - bool operator()() const + std::optional operator()() const { - return result; + if (result) return std::nullopt; + return 1; } }; } // namespace @@ -47,7 +48,7 @@ FUZZ_TARGET(checkqueue) for (auto check : checks_1) delete check; } if (fuzzed_data_provider.ConsumeBool()) { - (void)check_queue_1.Wait(); + (void)check_queue_1.Complete(); } CCheckQueueControl check_queue_control{&check_queue_2}; @@ -57,6 +58,6 @@ FUZZ_TARGET(checkqueue) for (auto check : checks_2) delete check; } if (fuzzed_data_provider.ConsumeBool()) { - (void)check_queue_control.Wait(); + (void)check_queue_control.Complete(); } } diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index 87c4b21fec..7aaab73852 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -403,8 +403,8 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: tx.vout[0].nValue = tx.vout[0].nValue.GetAmount() - LOWFEE; hash = tx.GetHash(); AddToMempool(tx_mempool, entry.Fee(LOWFEE).Time(Now()).SpendsCoinbase(false).FromTx(tx)); - // Should throw block-validation-failed - BOOST_CHECK_EXCEPTION(AssemblerForTest(tx_mempool).CreateNewBlock(scriptPubKey), std::runtime_error, HasReason("block-validation-failed")); + // Should throw mandatory-script-verify-flag-failed + BOOST_CHECK_EXCEPTION(AssemblerForTest(tx_mempool).CreateNewBlock(scriptPubKey), std::runtime_error, HasReason("mandatory-script-verify-flag-failed")); // Delete the dummy blocks again. while (m_node.chainman->ActiveChain().Tip()->nHeight > nHeight) { diff --git a/src/test/script_p2sh_tests.cpp b/src/test/script_p2sh_tests.cpp index ffc1bd75c5..211d9fa202 100644 --- a/src/test/script_p2sh_tests.cpp +++ b/src/test/script_p2sh_tests.cpp @@ -121,7 +121,7 @@ BOOST_AUTO_TEST_CASE(sign) { CScript sigSave = txTo[i].vin[0].scriptSig; txTo[i].vin[0].scriptSig = txTo[j].vin[0].scriptSig; - bool sigOK = CScriptCheck(txFrom.vout[txTo[i].vin[0].prevout.n], CTransaction(txTo[i]), signature_cache, 0, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_STRICTENC, false, &txdata)(); + bool sigOK = !CScriptCheck(txFrom.vout[txTo[i].vin[0].prevout.n], CTransaction(txTo[i]), signature_cache, 0, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_STRICTENC, false, &txdata)().has_value(); if (i == j) BOOST_CHECK_MESSAGE(sigOK, strprintf("VerifySignature %d %d", i, j)); else diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp index 6875af96f5..a4c9d32eb2 100644 --- a/src/test/transaction_tests.cpp +++ b/src/test/transaction_tests.cpp @@ -604,7 +604,7 @@ BOOST_AUTO_TEST_CASE(test_big_witness_transaction) control.Add(std::move(vChecks)); } - bool controlCheck = control.Wait(); + bool controlCheck = !control.Complete().has_value(); assert(controlCheck); } diff --git a/src/validation.cpp b/src/validation.cpp index 73a2e5a0e4..e6e759f024 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2347,10 +2347,17 @@ void UpdateCoins(const CTransaction& tx, CCoinsViewCache& inputs, CTxUndo &txund AddCoins(inputs, tx, nHeight); } -bool CScriptCheck::operator()() { +std::optional> CScriptCheck::operator()() { const CScript &scriptSig = ptxTo->vin[nIn].scriptSig; - const CScriptWitness *witness = ptxTo->witness.vtxinwit.size() > nIn ? &ptxTo->witness.vtxinwit[nIn].scriptWitness : nullptr; - return VerifyScript(scriptSig, m_tx_out.scriptPubKey, witness, nFlags, CachingTransactionSignatureChecker(ptxTo, nIn, m_tx_out.nValue, cacheStore, *m_signature_cache, *txdata), &error); + + const CScriptWitness *witness = ptxTo->witness.vtxinwit.size() > nIn ? &ptxTo->witness.vtxinwit[nIn].scriptWitness : nullptr; + ScriptError error{SCRIPT_ERR_UNKNOWN_ERROR}; + if (VerifyScript(scriptSig, m_tx_out.scriptPubKey, witness, nFlags, CachingTransactionSignatureChecker(ptxTo, nIn, m_tx_out.nValue, cacheStore, *m_signature_cache, *txdata), &error)) { + return std::nullopt; + } else { + auto debug_str = strprintf("input %i of %s (wtxid %s), spending %s:%i", nIn, ptxTo->GetHash().ToString(), ptxTo->GetWitnessHash().ToString(), ptxTo->vin[nIn].prevout.hash.ToString(), ptxTo->vin[nIn].prevout.n); + return std::make_pair(error, std::move(debug_str)); + } } ValidationCache::ValidationCache(const size_t script_execution_cache_bytes, const size_t signature_cache_bytes) @@ -2443,9 +2450,13 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, // spent being checked as a part of CScriptCheck. // Verify signature + // CCheck* check = new CScriptCheck(txdata.m_spent_outputs[i], tx, validation_cache.m_signature_cache, i, flags, cacheSigStore, &txdata); + // ScriptError serror = QueueCheck(pvChecks, check); + // if (serror != SCRIPT_ERR_OK) { CCheck* check = new CScriptCheck(txdata.m_spent_outputs[i], tx, validation_cache.m_signature_cache, i, flags, cacheSigStore, &txdata); - ScriptError serror = QueueCheck(pvChecks, check); - if (serror != SCRIPT_ERR_OK) { + if (pvChecks) { + pvChecks->emplace_back(std::move(check)); + } else if (auto result = (*check)(); result.has_value()) { if (flags & STANDARD_NOT_MANDATORY_VERIFY_FLAGS) { // Check whether the failure was caused by a // non-mandatory script verification check, such as @@ -2457,22 +2468,23 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, // data providers. CScriptCheck check2(txdata.m_spent_outputs[i], tx, validation_cache.m_signature_cache, i, flags & ~STANDARD_NOT_MANDATORY_VERIFY_FLAGS, cacheSigStore, &txdata); - if (check2()) { - return state.Invalid(TxValidationResult::TX_NOT_STANDARD, strprintf("non-mandatory-script-verify-flag (%s)", ScriptErrorString(serror))); + auto mandatory_result = check2(); + if (!mandatory_result.has_value()) { + return state.Invalid(TxValidationResult::TX_NOT_STANDARD, strprintf("non-mandatory-script-verify-flag (%s)", ScriptErrorString(result->first)), result->second); + } else { + // If the second check failed, it failed due to a mandatory script verification + // flag, but the first check might have failed on a non-mandatory script + // verification flag. + // + // Avoid reporting a mandatory script check failure with a non-mandatory error + // string by reporting the error from the second check. + result = mandatory_result; } - - // If the second check failed, it failed due to a mandatory script verification - // flag, but the first check might have failed on a non-mandatory script - // verification flag. - // - // Avoid reporting a mandatory script check failure with a non-mandatory error - // string by reporting the error from the second check. - serror = check2.GetScriptError(); } // MANDATORY flag failures correspond to - // TxValidationResult::TX_CONSENSUS. - return state.Invalid(TxValidationResult::TX_CONSENSUS, strprintf("mandatory-script-verify-flag-failed (%s)", ScriptErrorString(serror))); + return state.Invalid(TxValidationResult::TX_CONSENSUS, strprintf("mandatory-script-verify-flag-failed (%s)", ScriptErrorString(result->first)), result->second); + } } @@ -2924,8 +2936,8 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, for (const auto& tx : block.vtx) { for (size_t o = 0; o < tx->vout.size(); o++) { if (view.HaveCoin(COutPoint(tx->GetHash(), o))) { - LogPrintf("ERROR: ConnectBlock(): tried to overwrite transaction\n"); - return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-txns-BIP30"); + state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-txns-BIP30", + "tried to overwrite transaction"); } } } @@ -2988,6 +3000,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, for (unsigned int i = 0; i < block.vtx.size(); i++) { + if (!state.IsValid()) break; const CTransaction &tx = *(block.vtx[i]); nInputs += tx.vin.size(); @@ -3002,15 +3015,19 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, parallel_script_checks ? &vChecks : nullptr, fCacheResults, fScriptChecks, fedpegscripts)) { // Any transaction validation failure in ConnectBlock is a block consensus failure state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, - tx_state.GetRejectReason(), tx_state.GetDebugMessage()); - LogError("%s: Consensus::CheckTxInputs: %s, %s\n", __func__, tx.GetHash().ToString(), state.ToString()); - return false; + tx_state.GetRejectReason(), + tx_state.GetDebugMessage() + " in transaction " + tx.GetHash().ToString()); + break; } - control.Add(vChecks); - + // control.Add(vChecks); + // + // if (!MoneyRange(fee_map)) { + // LogPrintf("ERROR: %s: accumulated fee in the block out of range.\n", __func__); + // return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-txns-accumulated-fee-outofrange"); if (!MoneyRange(fee_map)) { - LogPrintf("ERROR: %s: accumulated fee in the block out of range.\n", __func__); - return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-txns-accumulated-fee-outofrange"); + state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-txns-accumulated-fee-outofrange", + "accumulated fee in the block out of range"); + break; } // Check that transaction is BIP68 final @@ -3026,8 +3043,9 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, } if (!SequenceLocks(tx, nLockTimeFlags, prevheights, *pindex)) { - LogPrintf("ERROR: %s: contains a non-BIP68-final transaction\n", __func__); - return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-txns-nonfinal"); + state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-txns-nonfinal", + "contains a non-BIP68-final transaction " + tx.GetHash().ToString()); + break; } } @@ -3037,8 +3055,8 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, // * witness (when witness enabled in flags and excludes coinbase) nSigOpsCost += GetTransactionSigOpCost(tx, view, flags); if (nSigOpsCost > MAX_BLOCK_SIGOPS_COST) { - LogPrintf("ERROR: ConnectBlock(): too many sigops\n"); - return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-blk-sigops"); + state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-blk-sigops", "too many sigops"); + break; } if (!tx.IsCoinBase()) @@ -3050,9 +3068,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, // Any transaction validation failure in ConnectBlock is a block consensus failure state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, tx_state.GetRejectReason(), tx_state.GetDebugMessage()); - LogError("ConnectBlock(): CheckInputScripts on %s failed with %s\n", - tx.GetHash().ToString(), state.ToString()); - return false; + break; } control.Add(std::move(vChecks)); } @@ -3072,20 +3088,36 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, Ticks(m_chainman.time_connect), Ticks(m_chainman.time_connect) / m_chainman.num_blocks_total); + // todo: + // CAmountMap block_reward = fee_map; + // block_reward[consensusParams.subsidy_asset] += GetBlockSubsidy(pindex->nHeight, consensusParams); + // if (!MoneyRange(block_reward)) { + // LogPrintf("ERROR: ConnectBlock(): total block reward overflowed\n"); + // return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-blockreward-outofrange"); + // } + // if (!VerifyCoinbaseAmount(*(block.vtx[0]), block_reward)) { + // LogPrintf("ERROR: ConnectBlock(): coinbase pays too much\n"); + // return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cb-amount"); + CAmountMap block_reward = fee_map; block_reward[consensusParams.subsidy_asset] += GetBlockSubsidy(pindex->nHeight, consensusParams); - if (!MoneyRange(block_reward)) { + if (!MoneyRange(block_reward) && state.IsValid()) { LogPrintf("ERROR: ConnectBlock(): total block reward overflowed\n"); - return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-blockreward-outofrange"); - } - if (!VerifyCoinbaseAmount(*(block.vtx[0]), block_reward)) { - LogPrintf("ERROR: ConnectBlock(): coinbase pays too much\n"); - return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cb-amount"); + state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-blockreward-outofrange"); } - if (!control.Wait()) { - LogPrintf("ERROR: %s: CheckQueue failed\n", __func__); - return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "block-validation-failed"); + if (!VerifyCoinbaseAmount(*(block.vtx[0]), block_reward) && state.IsValid()) { + state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cb-amount", + strprintf("coinbase pays too much")); + } + + auto parallel_result = control.Complete(); + if (parallel_result.has_value() && state.IsValid()) { + state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, strprintf("mandatory-script-verify-flag-failed (%s)", ScriptErrorString(parallel_result->first)), parallel_result->second); + } + if (!state.IsValid()) { + LogInfo("Block validation error: %s", state.ToString()); + return false; } const auto time_4{SteadyClock::now()}; m_chainman.time_verify += time_4 - time_2; @@ -3095,8 +3127,9 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, Ticks(m_chainman.time_verify), Ticks(m_chainman.time_verify) / m_chainman.num_blocks_total); - if (fJustCheck) + if (fJustCheck) { return true; + } if (!m_blockman.WriteUndoDataForBlock(blockundo, state, *pindex)) { return false; diff --git a/src/validation.h b/src/validation.h index fc7b394cc0..eba54dc9a3 100644 --- a/src/validation.h +++ b/src/validation.h @@ -358,9 +358,7 @@ public: CScriptCheck(CScriptCheck&&) = default; CScriptCheck& operator=(CScriptCheck&&) = default; - bool operator()() override; - - ScriptError GetScriptError() const { return error; } + std::optional> operator()() override; }; // CScriptCheck is used a lot in std::vector, make sure that's efficient diff --git a/test/functional/feature_cltv.py b/test/functional/feature_cltv.py index a33059f15e..eed831433b 100755 --- a/test/functional/feature_cltv.py +++ b/test/functional/feature_cltv.py @@ -87,7 +87,6 @@ class BIP65Test(BitcoinTestFramework): self.noban_tx_relay = True self.extra_args = [[ f'-testactivationheight=cltv@{CLTV_HEIGHT}', - '-par=1', # Use only one script thread to get the exact reject reason for testing '-acceptnonstdtxn=1', # cltv_invalidate is nonstandard f'-con_bip65height={CLTV_HEIGHT}', # ELEMENTS ]] @@ -176,7 +175,7 @@ class BIP65Test(BitcoinTestFramework): block.hashMerkleRoot = block.calc_merkle_root() block.solve() - with self.nodes[0].assert_debug_log(expected_msgs=[f'CheckInputScripts on {block.vtx[-1].hash} failed with {expected_cltv_reject_reason}']): + with self.nodes[0].assert_debug_log(expected_msgs=[f'Block validation error: {expected_cltv_reject_reason}']): peer.send_and_ping(msg_block(block)) assert_equal(int(self.nodes[0].getbestblockhash(), 16), tip) peer.sync_with_ping() diff --git a/test/functional/feature_csv_activation.py b/test/functional/feature_csv_activation.py index df02bcc6ad..d5b6d0dba9 100755 --- a/test/functional/feature_csv_activation.py +++ b/test/functional/feature_csv_activation.py @@ -99,7 +99,6 @@ class BIP68_112_113Test(BitcoinTestFramework): self.noban_tx_relay = True self.extra_args = [[ f'-testactivationheight=csv@{CSV_ACTIVATION_HEIGHT}', - '-par=1', # Use only one script thread to get the exact reject reason for testing ]] self.supports_cli = False diff --git a/test/functional/feature_dersig.py b/test/functional/feature_dersig.py index 4ab3cd5683..ae879785b7 100755 --- a/test/functional/feature_dersig.py +++ b/test/functional/feature_dersig.py @@ -51,7 +51,6 @@ class BIP66Test(BitcoinTestFramework): self.noban_tx_relay = True self.extra_args = [[ f'-testactivationheight=dersig@{DERSIG_HEIGHT}', - '-par=1', # Use only one script thread to get the exact log msg for testing f'-con_bip66height={DERSIG_HEIGHT}', # ELEMENTS ]] self.setup_clean_chain = True @@ -132,7 +131,7 @@ class BIP66Test(BitcoinTestFramework): block.hashMerkleRoot = block.calc_merkle_root() block.solve() - with self.nodes[0].assert_debug_log(expected_msgs=[f'CheckInputScripts on {block.vtx[-1].hash} failed with mandatory-script-verify-flag-failed (Non-canonical DER signature)']): + with self.nodes[0].assert_debug_log(expected_msgs=[f'Block validation error: mandatory-script-verify-flag-failed (Non-canonical DER signature)']): peer.send_and_ping(msg_block(block)) assert_equal(int(self.nodes[0].getbestblockhash(), 16), tip) peer.sync_with_ping() diff --git a/test/functional/feature_nulldummy.py b/test/functional/feature_nulldummy.py index 4e1dd8a475..43c6d46dee 100755 --- a/test/functional/feature_nulldummy.py +++ b/test/functional/feature_nulldummy.py @@ -58,7 +58,6 @@ class NULLDUMMYTest(BitcoinTestFramework): self.extra_args = [[ f'-testactivationheight=segwit@{COINBASE_MATURITY + 5}', '-addresstype=legacy', - '-par=1', # Use only one script thread to get the exact reject reason for testing ]] def create_transaction(self, *, txid, input_details=None, addr, amount, privkey, fee): diff --git a/test/functional/feature_sighash_rangeproof.py b/test/functional/feature_sighash_rangeproof.py index 595f68fe92..214a67b9f3 100755 --- a/test/functional/feature_sighash_rangeproof.py +++ b/test/functional/feature_sighash_rangeproof.py @@ -203,7 +203,7 @@ class SighashRangeproofTest(BitcoinTestFramework): if assert_valid: self.nodes[0].testproposedblock(block_hex) else: - assert_raises_rpc_error(-25, "block-validation-failed", self.nodes[0].testproposedblock, block_hex) + assert_raises_rpc_error(-25, "mandatory-script-verify-flag-failed", self.nodes[0].testproposedblock, block_hex) # Then try submit the block and check if it was accepted or not. pre = self.nodes[0].getblockcount() diff --git a/test/functional/feature_taproot.py b/test/functional/feature_taproot.py index 85c10fbae3..57156c86f8 100755 --- a/test/functional/feature_taproot.py +++ b/test/functional/feature_taproot.py @@ -1325,7 +1325,7 @@ class TaprootTest(BitcoinTestFramework): # ELEMENTS: to preserve tests which depend on the exact number of outputs, # we turn one of the original outputs into a fee output, which routinely # results in us burning massive amounts of coin. Hence -maxtxfee. - self.extra_args = [["-par=1", "-maxtxfee=100.0"]] * self.num_nodes + self.extra_args = [["-maxtxfee=100.0"]] * self.num_nodes # ELEMENTS: both nodes have Simplicity active. We activate one with evbparams # and the other with vbparams to check that both work. self.extra_args[0].append("-vbparams=simplicity:-1:1") diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py index af3ab2c29b..751ad54049 100755 --- a/test/functional/p2p_segwit.py +++ b/test/functional/p2p_segwit.py @@ -218,6 +218,9 @@ class SegWitTest(BitcoinTestFramework): self.noban_tx_relay = True # This test tests SegWit both pre and post-activation, so use the normal BIP9 activation. self.extra_args = [ + # -par=1 should not affect validation outcome or logging/reported failures. It is kept + # here to exercise the code path still (as it is distinct for multithread script + # validation). ["-acceptnonstdtxn=1", f"-testactivationheight=segwit@{SEGWIT_HEIGHT}", "-par=1"], ["-acceptnonstdtxn=0", f"-testactivationheight=segwit@{SEGWIT_HEIGHT}"], ] @@ -516,10 +519,6 @@ class SegWitTest(BitcoinTestFramework): # When the block is serialized without witness, validation fails because the transaction is # invalid (transactions are always validated with SCRIPT_VERIFY_WITNESS so a segwit v0 transaction # without a witness is invalid). - # Note: The reject reason for this failure could be - # 'block-validation-failed' (if script check threads > 1) or - # 'mandatory-script-verify-flag-failed (Witness program was passed an - # empty witness)' (otherwise). test_witness_block(self.nodes[0], self.test_node, block, accepted=False, with_witness=False, reason='mandatory-script-verify-flag-failed (Witness program was passed an empty witness)') @@ -1043,7 +1042,7 @@ class SegWitTest(BitcoinTestFramework): tx2.vout.append(CTxOut(tx.vout[1].nValue.getAmount())) # fee tx2.wit.vtxinwit.extend([CTxInWitness(), CTxInWitness()]) tx2.wit.vtxinwit[0].scriptWitness.stack = [CScript([CScriptNum(1)]), CScript([CScriptNum(1)]), witness_script] - tx2.wit.vtxinwit[1].scriptWitness.stack = [CScript([OP_TRUE])] + tx2.wit.vtxinwit[1].scriptWitness.stack = [] block = self.build_next_block() self.update_witness_block_with_transactions(block, [tx2])