Merge bitcoin/bitcoin#31112: Improve parallel script validation error debug logging

492e1f0994 [validation] merge all ConnectBlock debug logging code paths (Pieter Wuille)
b49df703f0 [validation] include all logged information in BlockValidationState (Pieter Wuille)
7b267c034f [validation] Add detailed txin/txout information for script error messages (Pieter Wuille)
146a3d5426 [validation] Make script error messages uniform for parallel/single validation (Pieter Wuille)
1ac1c33f3f [checkqueue] support user-defined return type through std::optional (Pieter Wuille)

Pull request description:

  ~~Builds on top of #31097~~ (now merged). Fixes #30960.

  So far, detailed information about script validation failures is only reported when running with `-par=1`, due to a lack of ability to transfer information from the script validation threads to the validation thread. Fix this by extending the `CCheckQueue` functionality to pass more results through than just success/failure, and use this to report the exact Script error, as well as the transaction input in which it occurred.

ACKs for top commit:
  achow101:
    ACK 492e1f0994
  furszy:
    Code review ACK 492e1f0
  maflcko:
    re-ACK 492e1f0994 🍈
  dergoegge:
    ACK 492e1f0994
  instagibbs:
    ACK 492e1f0994
  mzumsande:
    Code Review ACK 492e1f0994

Tree-SHA512: 234f2e7dfd03bdcd2a56200875fe370962f211ea7ed334038a6a9279a758030bf94bb6246f60d06dd0473dac4b9dbf050d9a32ecaa4176f7727eff63572bf4fd

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<int> operator()()
         {
-            return true;
+            return std::nullopt;
         }
     };
 
@@ -62,7 +62,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();
     });
 }
 BENCHMARK(CCheckQueueSpeedPrevectorJob, benchmark::PriorityLevel::HIGH);

src/checkqueue.h

@@ -11,19 +11,24 @@
 
 #include <algorithm>
 #include <iterator>
+#include <optional>
 #include <vector>
 
 /**
  * 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<R>.
+  *
+  * 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 <typename T>
+template <typename T, typename R = std::remove_cvref_t<decltype(std::declval<T>()().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<R> m_result GUARDED_BY(m_mutex);
 
     /**
      * Number of verifications that haven't completed yet.
@@ -62,24 +67,28 @@ private:
     std::vector<std::thread> m_worker_threads;
     bool m_request_stop GUARDED_BY(m_mutex){false};
 
-    /** Internal function that does bulk of the verification work. */
+    /** Internal function that does bulk of the verification work. If fMaster, return the final result. */
-    bool Loop(bool fMaster) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
+    std::optional<R> Loop(bool fMaster) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
     {
         std::condition_variable& cond = fMaster ? m_master_cv : m_worker_cv;
         std::vector<T> vChecks;
         vChecks.reserve(nBatchSize);
         unsigned int nNow = 0;
-        bool fOk = true;
+        std::optional<R> 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<R> 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,12 +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)
+            if (do_work) {
-                if (fOk)
+                for (T& check : vChecks) {
-                    fOk = check();
+                    local_result = check();
+                    if (local_result.has_value()) break;
+                }
+            }
             vChecks.clear();
         } while (true);
     }
@@ -146,8 +159,9 @@ public:
     CCheckQueue(CCheckQueue&&) = delete;
     CCheckQueue& operator=(CCheckQueue&&) = delete;
 
-    //! Wait until execution finishes, and return whether all evaluations were successful.
+    //! Join the execution until completion. If at least one evaluation wasn't successful, return
-    bool Wait() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
+    //! its error.
+    std::optional<R> Complete() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
     {
         return Loop(true /* master thread */);
     }
@@ -188,11 +202,11 @@ public:
  * RAII-style controller object for a CCheckQueue that guarantees the passed
  * queue is finished before continuing.
  */
-template <typename T>
+template <typename T, typename R = std::remove_cvref_t<decltype(std::declval<T>()().value())>>
 class CCheckQueueControl
 {
 private:
-    CCheckQueue<T> * const pqueue;
+    CCheckQueue<T, R> * const pqueue;
     bool fDone;
 
 public:
@@ -207,13 +221,12 @@ public:
         }
     }
 
-    bool Wait()
+    std::optional<R> Complete()
     {
-        if (pqueue == nullptr)
+        if (pqueue == nullptr) return std::nullopt;
-            return true;
+        auto ret = pqueue->Complete();
-        bool fRet = pqueue->Wait();
         fDone = true;
-        return fRet;
+        return ret;
     }
 
     void Add(std::vector<T>&& vChecks)
@@ -226,7 +239,7 @@ public:
     ~CCheckQueueControl()
     {
         if (!fDone)
-            Wait();
+            Complete();
         if (pqueue != nullptr) {
             LEAVE_CRITICAL_SECTION(pqueue->m_control_mutex);
         }

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<int> operator()() const
     {
-        return true;
+        return std::nullopt;
     }
 };
 
 struct FakeCheckCheckCompletion {
     static std::atomic<size_t> n_calls;
-    bool operator()()
+    std::optional<int> operator()()
     {
         n_calls.fetch_add(1, std::memory_order_relaxed);
-        return true;
+        return std::nullopt;
     }
 };
 
-struct FailingCheck {
+struct FixedCheck
-    bool fails;
+{
-    FailingCheck(bool _fails) : fails(_fails){};
+    std::optional<int> m_result;
-    bool operator()() const
+    FixedCheck(std::optional<int> result) : m_result(result){};
-    {
+    std::optional<int> operator()() const { return m_result; }
-        return !fails;
-    }
 };
 
 struct UniqueCheck {
@@ -71,11 +69,11 @@ struct UniqueCheck {
     static std::unordered_multiset<size_t> results GUARDED_BY(m);
     size_t check_id;
     UniqueCheck(size_t check_id_in) : check_id(check_id_in){};
-    bool operator()()
+    std::optional<int> operator()()
     {
         LOCK(m);
         results.insert(check_id);
-        return true;
+        return std::nullopt;
     }
 };
 
@@ -83,9 +81,9 @@ struct UniqueCheck {
 struct MemoryCheck {
     static std::atomic<size_t> fake_allocated_memory;
     bool b {false};
-    bool operator()() const
+    std::optional<int> 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<int> operator()() const
     {
-        return true;
+        return std::nullopt;
     }
     FrozenCleanupCheck() = default;
     ~FrozenCleanupCheck()
@@ -149,7 +147,7 @@ std::atomic<size_t> MemoryCheck::fake_allocated_memory{0};
 // Queue Typedefs
 typedef CCheckQueue<FakeCheckCheckCompletion> Correct_Queue;
 typedef CCheckQueue<FakeCheck> Standard_Queue;
-typedef CCheckQueue<FailingCheck> Failing_Queue;
+typedef CCheckQueue<FixedCheck> Fixed_Queue;
 typedef CCheckQueue<UniqueCheck> Unique_Queue;
 typedef CCheckQueue<MemoryCheck> Memory_Queue;
 typedef CCheckQueue<FrozenCleanupCheck> FrozenCleanup_Queue;
@@ -174,7 +172,7 @@ void CheckQueueTest::Correct_Queue_range(std::vector<size_t> 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);
     }
 }
@@ -217,27 +215,27 @@ 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<Failing_Queue>(QUEUE_BATCH_SIZE, SCRIPT_CHECK_THREADS);
+    auto fixed_queue = std::make_unique<Fixed_Queue>(QUEUE_BATCH_SIZE, SCRIPT_CHECK_THREADS);
     for (size_t i = 0; i < 1001; ++i) {
-        CCheckQueueControl<FailingCheck> control(fail_queue.get());
+        CCheckQueueControl<FixedCheck> control(fixed_queue.get());
         size_t remaining = i;
         while (remaining) {
             size_t r = m_rng.randrange(10);
 
-            std::vector<FailingCheck> vChecks;
+            std::vector<FixedCheck> vChecks;
             vChecks.reserve(r);
             for (size_t k = 0; k < r && remaining; k++, remaining--)
-                vChecks.emplace_back(remaining == 1);
+                vChecks.emplace_back(remaining == 1 ? std::make_optional<int>(17 * i) : std::nullopt);
             control.Add(std::move(vChecks));
         }
-        bool success = control.Wait();
+        auto result = control.Complete();
         if (i > 0) {
-            BOOST_REQUIRE(!success);
+            BOOST_REQUIRE(result.has_value() && *result == static_cast<int>(17 * i));
-        } else if (i == 0) {
+        } else {
-            BOOST_REQUIRE(success);
+            BOOST_REQUIRE(!result.has_value());
         }
     }
 }
@@ -245,17 +243,17 @@ 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<Failing_Queue>(QUEUE_BATCH_SIZE, SCRIPT_CHECK_THREADS);
+    auto fail_queue = std::make_unique<Fixed_Queue>(QUEUE_BATCH_SIZE, SCRIPT_CHECK_THREADS);
     for (auto times = 0; times < 10; ++times) {
         for (const bool end_fails : {true, false}) {
-            CCheckQueueControl<FailingCheck> control(fail_queue.get());
+            CCheckQueueControl<FixedCheck> control(fail_queue.get());
             {
-                std::vector<FailingCheck> vChecks;
+                std::vector<FixedCheck> vChecks;
-                vChecks.resize(100, false);
+                vChecks.resize(100, FixedCheck(std::nullopt));
-                vChecks[99] = end_fails;
+                vChecks[99] = FixedCheck(end_fails ? std::make_optional<int>(2) : std::nullopt);
                 control.Add(std::move(vChecks));
             }
-            bool r =control.Wait();
+            bool r = !control.Complete().has_value();
             BOOST_REQUIRE(r != end_fails);
         }
     }
@@ -329,8 +327,8 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_FrozenCleanup)
         CCheckQueueControl<FrozenCleanupCheck> control(queue.get());
         std::vector<FrozenCleanupCheck> vChecks(1);
         control.Add(std::move(vChecks));
-        bool waitResult = control.Wait(); // Hangs here
+        auto result = control.Complete(); // Hangs here
-        assert(waitResult);
+        assert(!result);
     });
     {
         std::unique_lock<std::mutex> l(FrozenCleanupCheck::m);

src/test/fuzz/checkqueue.cpp

@@ -19,9 +19,10 @@ struct DumbCheck {
     {
     }
 
-    bool operator()() const
+    std::optional<int> operator()() const
     {
-        return result;
+        if (result) return std::nullopt;
+        return 1;
     }
 };
 } // namespace
@@ -45,7 +46,7 @@ FUZZ_TARGET(checkqueue)
         check_queue_1.Add(std::move(checks_1));
     }
     if (fuzzed_data_provider.ConsumeBool()) {
-        (void)check_queue_1.Wait();
+        (void)check_queue_1.Complete();
     }
 
     CCheckQueueControl<DumbCheck> check_queue_control{&check_queue_2};
@@ -53,6 +54,6 @@ FUZZ_TARGET(checkqueue)
         check_queue_control.Add(std::move(checks_2));
     }
     if (fuzzed_data_provider.ConsumeBool()) {
-        (void)check_queue_control.Wait();
+        (void)check_queue_control.Complete();
     }
 }

src/test/miner_tests.cpp

@@ -403,8 +403,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std::
         tx.vout[0].nValue -= LOWFEE;
         hash = tx.GetHash();
         AddToMempool(tx_mempool, entry.Fee(LOWFEE).Time(Now<NodeSeconds>()).SpendsCoinbase(false).FromTx(tx));
-        // Should throw block-validation-failed
+        BOOST_CHECK_EXCEPTION(AssemblerForTest(tx_mempool).CreateNewBlock(scriptPubKey), std::runtime_error, HasReason("mandatory-script-verify-flag-failed"));
-        BOOST_CHECK_EXCEPTION(AssemblerForTest(tx_mempool).CreateNewBlock(scriptPubKey), std::runtime_error, HasReason("block-validation-failed"));
 
         // Delete the dummy blocks again.
         while (m_node.chainman->ActiveChain().Tip()->nHeight > nHeight) {

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

src/test/transaction_tests.cpp

@@ -588,7 +588,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);
 }
 

src/validation.cpp

@@ -2103,10 +2103,16 @@ void UpdateCoins(const CTransaction& tx, CCoinsViewCache& inputs, CTxUndo &txund
     AddCoins(inputs, tx, nHeight);
 }
 
-bool CScriptCheck::operator()() {
+std::optional<std::pair<ScriptError, std::string>> CScriptCheck::operator()() {
     const CScript &scriptSig = ptxTo->vin[nIn].scriptSig;
     const CScriptWitness *witness = &ptxTo->vin[nIn].scriptWitness;
-    return VerifyScript(scriptSig, m_tx_out.scriptPubKey, witness, nFlags, CachingTransactionSignatureChecker(ptxTo, nIn, m_tx_out.nValue, cacheStore, *m_signature_cache, *txdata), &error);
+    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)
@@ -2195,9 +2201,7 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
         CScriptCheck check(txdata.m_spent_outputs[i], tx, validation_cache.m_signature_cache, i, flags, cacheSigStore, &txdata);
         if (pvChecks) {
             pvChecks->emplace_back(std::move(check));
-        } else if (!check()) {
+        } else if (auto result = check(); result.has_value()) {
-            ScriptError error{check.GetScriptError()};
-
             if (flags & STANDARD_NOT_MANDATORY_VERIFY_FLAGS) {
                 // Check whether the failure was caused by a
                 // non-mandatory script verification check, such as
@@ -2209,21 +2213,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())
+                auto mandatory_result = check2();
-                    return state.Invalid(TxValidationResult::TX_NOT_STANDARD, strprintf("non-mandatory-script-verify-flag (%s)", ScriptErrorString(check.GetScriptError())));
+                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.
-                error = check2.GetScriptError();
+                    result = mandatory_result;
+                }
             }
 
             // MANDATORY flag failures correspond to
             // TxValidationResult::TX_CONSENSUS.
-            return state.Invalid(TxValidationResult::TX_CONSENSUS, strprintf("mandatory-script-verify-flag-failed (%s)", ScriptErrorString(error)));
+            return state.Invalid(TxValidationResult::TX_CONSENSUS, strprintf("mandatory-script-verify-flag-failed (%s)", ScriptErrorString(result->first)), result->second);
         }
     }
 
@@ -2589,8 +2595,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");
+                    state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-txns-BIP30",
-                    return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-txns-BIP30");
+                                  "tried to overwrite transaction");
                 }
             }
         }
@@ -2629,6 +2635,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
     blockundo.vtxundo.reserve(block.vtx.size() - 1);
     for (unsigned int i = 0; i < block.vtx.size(); i++)
     {
+        if (!state.IsValid()) break;
         const CTransaction &tx = *(block.vtx[i]);
 
         nInputs += tx.vin.size();
@@ -2640,14 +2647,15 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
             if (!Consensus::CheckTxInputs(tx, tx_state, view, pindex->nHeight, txfee)) {
                 // Any transaction validation failure in ConnectBlock is a block consensus failure
                 state.Invalid(BlockValidationResult::BLOCK_CONSENSUS,
-                            tx_state.GetRejectReason(), tx_state.GetDebugMessage());
+                              tx_state.GetRejectReason(),
-                LogError("%s: Consensus::CheckTxInputs: %s, %s\n", __func__, tx.GetHash().ToString(), state.ToString());
+                              tx_state.GetDebugMessage() + " in transaction " + tx.GetHash().ToString());
-                return false;
+                break;
             }
             nFees += txfee;
             if (!MoneyRange(nFees)) {
-                LogPrintf("ERROR: %s: accumulated fee in the block out of range.\n", __func__);
+                state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-txns-accumulated-fee-outofrange",
-                return 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
@@ -2659,8 +2667,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__);
+                state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-txns-nonfinal",
-                return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-txns-nonfinal");
+                              "contains a non-BIP68-final transaction " + tx.GetHash().ToString());
+                break;
             }
         }
 
@@ -2670,8 +2679,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");
+            state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-blk-sigops", "too many sigops");
-            return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-blk-sigops");
+            break;
         }
 
         if (!tx.IsCoinBase())
@@ -2683,9 +2692,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",
+                break;
-                    tx.GetHash().ToString(), state.ToString());
-                return false;
             }
             control.Add(std::move(vChecks));
         }
@@ -2705,14 +2712,18 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
              Ticks<MillisecondsDouble>(m_chainman.time_connect) / m_chainman.num_blocks_total);
 
     CAmount blockReward = nFees + GetBlockSubsidy(pindex->nHeight, params.GetConsensus());
-    if (block.vtx[0]->GetValueOut() > blockReward) {
+    if (block.vtx[0]->GetValueOut() > blockReward && state.IsValid()) {
-        LogPrintf("ERROR: ConnectBlock(): coinbase pays too much (actual=%d vs limit=%d)\n", block.vtx[0]->GetValueOut(), blockReward);
+        state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cb-amount",
-        return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cb-amount");
+                      strprintf("coinbase pays too much (actual=%d vs limit=%d)", block.vtx[0]->GetValueOut(), blockReward));
     }
 
-    if (!control.Wait()) {
+    auto parallel_result = control.Complete();
-        LogPrintf("ERROR: %s: CheckQueue failed\n", __func__);
+    if (parallel_result.has_value() && state.IsValid()) {
-        return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "block-validation-failed");
+        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;
@@ -2722,8 +2733,9 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
              Ticks<SecondsDouble>(m_chainman.time_verify),
              Ticks<MillisecondsDouble>(m_chainman.time_verify) / m_chainman.num_blocks_total);
 
-    if (fJustCheck)
+    if (fJustCheck) {
         return true;
+    }
 
     if (!m_blockman.WriteUndoDataForBlock(blockundo, state, *pindex)) {
         return false;

src/validation.h

@@ -335,7 +335,6 @@ private:
     unsigned int nIn;
     unsigned int nFlags;
     bool cacheStore;
-    ScriptError error{SCRIPT_ERR_UNKNOWN_ERROR};
     PrecomputedTransactionData *txdata;
     SignatureCache* m_signature_cache;
 
@@ -348,9 +347,7 @@ public:
     CScriptCheck(CScriptCheck&&) = default;
     CScriptCheck& operator=(CScriptCheck&&) = default;
 
-    bool operator()();
+    std::optional<std::pair<ScriptError, std::string>> operator()();
-
-    ScriptError GetScriptError() const { return error; }
 };
 
 // CScriptCheck is used a lot in std::vector, make sure that's efficient

test/functional/feature_block.py

@@ -88,7 +88,6 @@ class FullBlockTest(BitcoinTestFramework):
         self.extra_args = [[
             '-acceptnonstdtxn=1',  # This is a consensus block test, we don't care about tx policy
             '-testactivationheight=bip34@2',
-            '-par=1', # Until https://github.com/bitcoin/bitcoin/issues/30960 is fixed
         ]]
 
     def run_test(self):

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
         ]]
         self.setup_clean_chain = True
@@ -175,7 +174,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()

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
 

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
         ]]
         self.setup_clean_chain = True
         self.rpc_timeout = 240
@@ -131,7 +130,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()

test/functional/feature_nulldummy.py

@@ -57,7 +57,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):

test/functional/feature_taproot.py

@@ -1285,7 +1285,6 @@ class TaprootTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 1
         self.setup_clean_chain = True
-        self.extra_args = [["-par=1"]]
 
     def block_submit(self, node, txs, msg, err_msg, cb_pubkey=None, fees=0, sigops_weight=0, witness=False, accept=False):
 

test/functional/p2p_segwit.py

@@ -214,6 +214,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}"],
         ]
@@ -507,10 +510,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)')
 
@@ -1015,7 +1014,7 @@ class SegWitTest(BitcoinTestFramework):
         tx2.vout.append(CTxOut(tx.vout[0].nValue, CScript([OP_TRUE])))
         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])