From 8221e89ac40349bfb322f9ca573c7fd9237e46a2 Mon Sep 17 00:00:00 2001
From: TheCharlatan <seb.kung@gmail.com>
Date: Fri, 18 Apr 2025 13:59:56 +0200
Subject: [PATCH] validation: Move coin existence and spend check to SpendBlock

Move the remaining UTXO-related operations from ConnectBlock to
SpendBlock. This includes moving the existence check, the UpdateCoins
call, and CBlockUndo generation.

ConnectBlock now takes a pre-populated CBlockUndo as an argument and no
longer accesses or manipulates the UTXO set.
---
 src/bench/connectblock.cpp        |   6 +-
 src/consensus/tx_verify.cpp       |  12 +---
 src/consensus/tx_verify.h         |   2 +-
 src/test/coinstatsindex_tests.cpp |   6 +-
 src/test/fuzz/coins_view.cpp      |   2 +-
 src/txmempool.cpp                 |   3 +-
 src/validation.cpp                | 102 ++++++++++++++++++++----------
 src/validation.h                  |   6 +-
 8 files changed, 86 insertions(+), 53 deletions(-)

diff --git a/src/bench/connectblock.cpp b/src/bench/connectblock.cpp
index e51333d0d27..1451b88c9d0 100644
--- a/src/bench/connectblock.cpp
+++ b/src/bench/connectblock.cpp
@@ -9,6 +9,7 @@
 #include <script/interpreter.h>
 #include <sync.h>
 #include <test/util/setup_common.h>
+#include <undo.h>
 #include <validation.h>
 
 #include <cassert>
@@ -100,9 +101,10 @@ void BenchmarkConnectBlock(benchmark::Bench& bench, std::vector<CKey>& keys, std
         auto* pindex{chainman->m_blockman.AddToBlockIndex(test_block, chainman->m_best_header)}; // Doing this here doesn't impact the benchmark
         CCoinsViewCache viewNew{&chainstate.CoinsTip()};
 
+        CBlockUndo blockundo;
         const auto block_hash{test_block.GetHash()};
-        assert(chainstate.SpendBlock(test_block, pindex, block_hash, viewNew, test_block_state));
-        assert(chainstate.ConnectBlock(test_block, test_block_state, pindex, viewNew));
+        assert(chainstate.SpendBlock(test_block, pindex, block_hash, viewNew, test_block_state, blockundo));
+        assert(chainstate.ConnectBlock(test_block, block_hash, blockundo, test_block_state, pindex));
     });
 }
 
diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
index fbc7479683a..97f8241089f 100644
--- a/src/consensus/tx_verify.cpp
+++ b/src/consensus/tx_verify.cpp
@@ -187,14 +187,8 @@ template int64_t GetTransactionSigOpCost<std::span<std::reference_wrapper<const
     const CTransaction& tx, const std::span<std::reference_wrapper<const Coin>> coins, uint32_t flags);
 
 template <typename T>
-bool Consensus::CheckTxInputs(const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& inputs, const T coins, int nSpendHeight, CAmount& txfee)
+bool Consensus::CheckTxInputs(const CTransaction& tx, TxValidationState& state, const T coins, int nSpendHeight, CAmount& txfee)
 {
-    // are the actual inputs available?
-    if (!inputs.HaveInputs(tx)) {
-        return state.Invalid(TxValidationResult::TX_MISSING_INPUTS, "bad-txns-inputs-missingorspent",
-                         strprintf("%s: inputs missing/spent", __func__));
-    }
-
     CAmount nValueIn = 0;
     Assert(coins.size() == tx.vin.size());
     auto input_it = tx.vin.begin();
@@ -232,7 +226,7 @@ bool Consensus::CheckTxInputs(const CTransaction& tx, TxValidationState& state,
 }
 
 template bool Consensus::CheckTxInputs<std::span<const Coin>>(
-    const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& inputs, const std::span<const Coin> coins, int nSpendHeight, CAmount& txfee);
+    const CTransaction& tx, TxValidationState& state, const std::span<const Coin> coins, int nSpendHeight, CAmount& txfee);
 
 template bool Consensus::CheckTxInputs<std::span<std::reference_wrapper<const Coin>>>(
-    const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& inputs, const std::span<std::reference_wrapper<const Coin>> coins, int nSpendHeight, CAmount& txfee);
+    const CTransaction& tx, TxValidationState& state, const std::span<std::reference_wrapper<const Coin>> coins, int nSpendHeight, CAmount& txfee);
diff --git a/src/consensus/tx_verify.h b/src/consensus/tx_verify.h
index 0333045aefa..88b2d6164bc 100644
--- a/src/consensus/tx_verify.h
+++ b/src/consensus/tx_verify.h
@@ -26,7 +26,7 @@ namespace Consensus {
  * Preconditions: tx.IsCoinBase() is false.
  */
 template <typename T>
-[[nodiscard]] bool CheckTxInputs(const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& inputs, const T coins, int nSpendHeight, CAmount& txfee);
+[[nodiscard]] bool CheckTxInputs(const CTransaction& tx, TxValidationState& state, const T coins, int nSpendHeight, CAmount& txfee);
 } // namespace Consensus
 
 /** Auxiliary functions for transaction validation (ideally should not be exposed) */
diff --git a/src/test/coinstatsindex_tests.cpp b/src/test/coinstatsindex_tests.cpp
index 9ed1939c1c0..422182894bd 100644
--- a/src/test/coinstatsindex_tests.cpp
+++ b/src/test/coinstatsindex_tests.cpp
@@ -9,6 +9,7 @@
 #include <test/util/index.h>
 #include <test/util/setup_common.h>
 #include <test/util/validation.h>
+#include <undo.h>
 #include <validation.h>
 
 #include <boost/test/unit_test.hpp>
@@ -100,9 +101,10 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_unclean_shutdown, TestChain100Setup)
             BOOST_CHECK(CheckBlock(block, state, params.GetConsensus()));
             BOOST_CHECK(m_node.chainman->AcceptBlock(new_block, state, &new_block_index, true, nullptr, nullptr, true));
             CCoinsViewCache view(&chainstate.CoinsTip());
+            CBlockUndo blockundo;
             const auto block_hash{block.GetHash()};
-            BOOST_CHECK(chainstate.SpendBlock(block, new_block_index, block_hash, view, state));
-            BOOST_CHECK(chainstate.ConnectBlock(block, state, new_block_index, view));
+            BOOST_CHECK(chainstate.SpendBlock(block, new_block_index, block_hash, view, state, blockundo));
+            BOOST_CHECK(chainstate.ConnectBlock(block, block_hash, blockundo, state, new_block_index));
         }
         // Send block connected notification, then stop the index without
         // sending a chainstate flushed notification. Prior to #24138, this
diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp
index e5e5fb45369..5c6ab63f088 100644
--- a/src/test/fuzz/coins_view.cpp
+++ b/src/test/fuzz/coins_view.cpp
@@ -258,7 +258,7 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view)
                 // are the actual inputs available?
                 if (!coins_view_cache.HaveInputs(transaction)) return;
                 auto coins{coins_view_cache.AccessCoins(transaction)};
-                if (Consensus::CheckTxInputs(transaction, state, coins_view_cache, std::span{coins}, fuzzed_data_provider.ConsumeIntegralInRange<int>(0, std::numeric_limits<int>::max()), tx_fee_out)) {
+                if (Consensus::CheckTxInputs(transaction, state, std::span{coins}, fuzzed_data_provider.ConsumeIntegralInRange<int>(0, std::numeric_limits<int>::max()), tx_fee_out)) {
                     assert(MoneyRange(tx_fee_out));
                 }
             },
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index f100621fd19..51b9ac08668 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -777,8 +777,9 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei
         TxValidationState dummy_state; // Not used. CheckTxInputs() should always pass
         CAmount txfee = 0;
         assert(!tx.IsCoinBase());
+        assert(mempoolDuplicate.HaveInputs(tx));
         auto coins{mempoolDuplicate.AccessCoins(tx)};
-        assert(Consensus::CheckTxInputs(tx, dummy_state, mempoolDuplicate, std::span{coins}, spendheight, txfee));
+        assert(Consensus::CheckTxInputs(tx, dummy_state, std::span{coins}, spendheight, txfee));
         for (const auto& input: tx.vin) mempoolDuplicate.SpendCoin(input.prevout);
         AddCoins(mempoolDuplicate, tx, std::numeric_limits<int>::max());
     }
diff --git a/src/validation.cpp b/src/validation.cpp
index 608d1b14103..adec151e14b 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -875,9 +875,15 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
         return state.Invalid(TxValidationResult::TX_PREMATURE_SPEND, "non-BIP68-final");
     }
 
+    // are the actual inputs available?
+    if (!m_view.HaveInputs(tx)) {
+        return state.Invalid(TxValidationResult::TX_MISSING_INPUTS, "bad-txns-inputs-missingorspent",
+                             strprintf("%s: inputs missing/spent", __func__));
+    }
+
     // The mempool holds txs for the next block, so pass height+1 to CheckTxInputs
     auto coins{m_view.AccessCoins(tx)};
-    if (!Consensus::CheckTxInputs(tx, state, m_view, std::span{coins}, m_active_chainstate.m_chain.Height() + 1, ws.m_base_fees)) {
+    if (!Consensus::CheckTxInputs(tx, state, std::span{coins}, m_active_chainstate.m_chain.Height() + 1, ws.m_base_fees)) {
         return false; // state filled in by CheckTxInputs
     }
 
@@ -2433,13 +2439,12 @@ static unsigned int GetBlockScriptFlags(const CBlockIndex& block_index, const Ch
 /** Apply the effects of this block (with given index) on the UTXO set represented by coins.
  *  Validity checks that depend on the UTXO set are also done; ConnectBlock()
  *  can fail if those validity checks fail (among other reasons). */
-bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, CBlockIndex* pindex,
-                               CCoinsViewCache& view, bool fJustCheck)
+bool Chainstate::ConnectBlock(const CBlock& block, const uint256& block_hash, const CBlockUndo& blockundo, BlockValidationState& state,
+                              CBlockIndex* pindex, bool fJustCheck)
 {
     AssertLockHeld(cs_main);
     assert(pindex);
-
-    uint256 block_hash{block.GetHash()};
+    Assume(block.GetHash() == block_hash);
     assert(*pindex->phashBlock == block_hash);
     const bool parallel_script_checks{m_chainman.GetCheckQueue().HasThreads()};
 
@@ -2532,8 +2537,6 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
              Ticks<SecondsDouble>(m_chainman.time_forks),
              Ticks<MillisecondsDouble>(m_chainman.time_forks) / m_chainman.num_blocks_total);
 
-    CBlockUndo blockundo;
-
     // Precomputed transaction data pointers must not be invalidated
     // until after `control` has run the script checks (potentially
     // in multiple threads). Preallocate the vector size so a new allocation
@@ -2546,7 +2549,6 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
     CAmount nFees = 0;
     int nInputs = 0;
     int64_t nSigOpsCost = 0;
-    blockundo.vtxundo.reserve(block.vtx.size() - 1);
     for (unsigned int i = 0; i < block.vtx.size(); i++)
     {
         if (!state.IsValid()) break;
@@ -2555,11 +2557,11 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
 
         if (!tx.IsCoinBase())
         {
+            auto spent_coins = std::span<const Coin>{blockundo.vtxundo[i - 1].vprevout};
             CAmount txfee = 0;
             TxValidationState tx_state;
 
-            auto coins{view.AccessCoins(tx)};
-            if (!Consensus::CheckTxInputs(tx, tx_state, view, std::span{coins}, pindex->nHeight, txfee)) {
+            if (!Consensus::CheckTxInputs(tx, tx_state, spent_coins, pindex->nHeight, txfee)) {
                 // Any transaction validation failure in ConnectBlock is a block consensus failure
                 state.Invalid(BlockValidationResult::BLOCK_CONSENSUS,
                               tx_state.GetRejectReason(),
@@ -2578,7 +2580,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
             // be in ConnectBlock because they require the UTXO set
             prevheights.resize(tx.vin.size());
             for (size_t j = 0; j < tx.vin.size(); j++) {
-                prevheights[j] = view.AccessCoin(tx.vin[j].prevout).nHeight;
+                prevheights[j] = spent_coins[j].nHeight;
             }
 
             if (!SequenceLocks(tx, nLockTimeFlags, prevheights, *pindex)) {
@@ -2592,13 +2594,15 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
         // * legacy (always)
         // * p2sh (when P2SH enabled in flags and excludes coinbase)
         // * witness (when witness enabled in flags and excludes coinbase)
-        {
-            auto coins{view.AccessCoins(tx)};
-            nSigOpsCost += GetTransactionSigOpCost(tx, std::span{coins}, flags);
-            if (nSigOpsCost > MAX_BLOCK_SIGOPS_COST) {
-                state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-blk-sigops", "too many sigops");
-                break;
-            }
+        if (!tx.IsCoinBase()) {
+            nSigOpsCost += GetTransactionSigOpCost(tx, std::span<const Coin>{blockundo.vtxundo[i - 1].vprevout}, flags);
+        } else {
+            std::vector<Coin> coin;
+            nSigOpsCost += GetTransactionSigOpCost(tx, std::span<const Coin>{coin}, flags);
+        }
+        if (nSigOpsCost > MAX_BLOCK_SIGOPS_COST) {
+            state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-blk-sigops", "too many sigops");
+            break;
         }
 
         if (!tx.IsCoinBase())
@@ -2606,8 +2610,15 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
             std::vector<CScriptCheck> vChecks;
             bool fCacheResults = fJustCheck; /* Don't cache results if we're actually connecting blocks (still consult the cache, though) */
             TxValidationState tx_state;
+            const std::vector<Coin>& spent_coins = blockundo.vtxundo[i - 1].vprevout;
 
-            std::vector<CTxOut> spent_outputs{view.GetUnspentOutputs(tx)};
+            std::vector<CTxOut> spent_outputs;
+            spent_outputs.reserve(tx.vin.size());
+            std::transform(spent_coins.begin(), spent_coins.end(),
+                           std::back_inserter(spent_outputs),
+                           [](const Coin& coin) -> CTxOut {
+                               return coin.out; // Assuming Coin has a CTxOut member named 'out'
+                           });
 
             if (fScriptChecks && !CheckInputScripts(tx, tx_state, std::move(spent_outputs), flags, fCacheResults, fCacheResults, txsdata[i], m_chainman.m_validation_cache, parallel_script_checks ? &vChecks : nullptr)) {
                 // Any transaction validation failure in ConnectBlock is a block consensus failure
@@ -2617,12 +2628,6 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
             }
             control.Add(std::move(vChecks));
         }
-
-        CTxUndo undoDummy;
-        if (i > 0) {
-            blockundo.vtxundo.emplace_back();
-        }
-        UpdateCoins(tx, view, i == 0 ? undoDummy : blockundo.vtxundo.back(), pindex->nHeight);
     }
     const auto time_3{SteadyClock::now()};
     m_chainman.time_connect += time_3 - time_2;
@@ -2919,7 +2924,8 @@ bool Chainstate::SpendBlock(
     const CBlockIndex* pindex,
     const uint256& block_hash,
     CCoinsViewCache& view,
-    BlockValidationState& state)
+    BlockValidationState& state,
+    CBlockUndo& blockundo)
 {
     AssertLockHeld(cs_main);
     assert(pindex);
@@ -3025,14 +3031,39 @@ bool Chainstate::SpendBlock(
         }
     }
 
+    int nInputs = 0;
+
+    blockundo.vtxundo.reserve(block.vtx.size() - 1);
+    for (const CTransactionRef& tx : block.vtx) {
+        nInputs += tx->vin.size();
+        // are the actual inputs available?
+        if (!view.HaveInputs(*tx)) {
+            state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-txns-inputs-missingorspent",
+                          strprintf("%s: inputs missing/spent in transaction %s", __func__, tx->GetHash().ToString()));
+            break;
+        }
+        if (!tx->IsCoinBase()) {
+            blockundo.vtxundo.emplace_back();
+        }
+        CTxUndo dummy;
+        UpdateCoins(*tx, view, tx->IsCoinBase() ? dummy : blockundo.vtxundo.back(), pindex->nHeight);
+    }
+
     const auto time_1{SteadyClock::now()};
     m_chainman.time_check += time_1 - time_start;
-    LogDebug(BCLog::BENCH, "      - Spend Block processed %u transactions: %.2fms (%.3fms/tx) [%.2fs (%.2fms/blk)]\n", (unsigned)block.vtx.size(),
+    LogDebug(BCLog::BENCH, "      - Spend Block processed %u transactions: %.2fms (%.3fms/tx, %.3fms/txin) [%.2fs (%.2fms/blk)]\n", (unsigned)block.vtx.size(),
              Ticks<MillisecondsDouble>(time_1 - time_start),
              block.vtx.size() == 0 ? 0 : Ticks<MillisecondsDouble>(time_1 - time_start) / block.vtx.size(),
+             nInputs <= 1 ? 0 : Ticks<MillisecondsDouble>(time_1 - time_start) / (nInputs - 1),
              Ticks<SecondsDouble>(m_chainman.time_connect),
              m_chainman.num_blocks_total == 0 ? 0 : Ticks<MillisecondsDouble>(m_chainman.time_connect) / m_chainman.num_blocks_total);
 
+    if (!state.IsValid()) {
+        LogInfo("Block validation error: %s", state.ToString());
+        return false;
+    }
+
+    assert(blockundo.vtxundo.size() == block.vtx.size() - 1);
     return true;
 }
 
@@ -3227,9 +3258,9 @@ bool Chainstate::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew,
              Ticks<MillisecondsDouble>(time_2 - time_1));
     {
         CCoinsViewCache view(&CoinsTip());
-
+        CBlockUndo blockundo;
         const auto block_hash{blockConnecting.GetHash()};
-        if (!SpendBlock(blockConnecting, pindexNew, block_hash, view, state)) {
+        if (!SpendBlock(blockConnecting, pindexNew, block_hash, view, state, blockundo)) {
             assert(state.IsInvalid());
             if (m_chainman.m_options.signals) {
                 m_chainman.m_options.signals->BlockChecked(blockConnecting, state);
@@ -3238,7 +3269,7 @@ bool Chainstate::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew,
             LogError("%s: SpendBlock %s failed, %s\n", __func__, pindexNew->GetBlockHash().ToString(), state.ToString());
             return false;
         }
-        bool rv = ConnectBlock(blockConnecting, state, pindexNew, view);
+        bool rv = ConnectBlock(blockConnecting, block_hash, blockundo, state, pindexNew);
         if (m_chainman.m_options.signals) {
             m_chainman.m_options.signals->BlockChecked(blockConnecting, state);
         }
@@ -4718,10 +4749,11 @@ bool TestBlockValidity(BlockValidationState& state,
         LogError("%s: Consensus::ContextualCheckBlock: %s\n", __func__, state.ToString());
         return false;
     }
-    if (!chainstate.SpendBlock(block, &indexDummy, block_hash, viewNew, state)) {
+    CBlockUndo blockundo;
+    if (!chainstate.SpendBlock(block, &indexDummy, block_hash, viewNew, state, blockundo)) {
         return false;
     }
-    if (!chainstate.ConnectBlock(block, state, &indexDummy, viewNew, true)) {
+    if (!chainstate.ConnectBlock(block, block_hash, blockundo, state, &indexDummy, true)) {
         return false;
     }
     assert(state.IsValid());
@@ -4905,12 +4937,14 @@ VerifyDBResult CVerifyDB::VerifyDB(
                 LogPrintf("Verification error: ReadBlock failed at %d, hash=%s\n", pindex->nHeight, pindex->GetBlockHash().ToString());
                 return VerifyDBResult::CORRUPTED_BLOCK_DB;
             }
+            CBlockUndo blockundo;
             const auto block_hash{block.GetHash()};
-            if (!chainstate.SpendBlock(block, pindex, block_hash, coins, state)) {
+            if (!chainstate.SpendBlock(block, pindex, block_hash, coins, state, blockundo)) {
                 LogError("SpendBlock failed %s\n", state.ToString());
                 return VerifyDBResult::CORRUPTED_BLOCK_DB;
             }
-            if (!chainstate.ConnectBlock(block, state, pindex, coins)) {
+
+            if (!chainstate.ConnectBlock(block, block_hash, blockundo, state, pindex)) {
                 LogPrintf("Verification error: found unconnectable block at %d, hash=%s (%s)\n", pindex->nHeight, pindex->GetBlockHash().ToString(), state.ToString());
                 return VerifyDBResult::CORRUPTED_BLOCK_DB;
             }
diff --git a/src/validation.h b/src/validation.h
index 93be54c6122..fb0267c929a 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -706,11 +706,11 @@ public:
     // Block (dis)connection on a given view:
     DisconnectResult DisconnectBlock(const CBlock& block, const CBlockIndex* pindex, CCoinsViewCache& view)
         EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
-    bool ConnectBlock(const CBlock& block, BlockValidationState& state, CBlockIndex* pindex,
-                      CCoinsViewCache& view, bool fJustCheck = false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+    bool ConnectBlock(const CBlock& block, const uint256& block_hash, const CBlockUndo& blockundo, BlockValidationState& state,
+                      CBlockIndex* pindex, bool fJustCheck = false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
     bool SpendBlock(const CBlock& block, const CBlockIndex* pindex, const uint256& block_hash,
-                    CCoinsViewCache& view, BlockValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+                    CCoinsViewCache& view, BlockValidationState& state, CBlockUndo& blockundo) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
     // Apply the effects of a block disconnection on the UTXO set.
     bool DisconnectTip(BlockValidationState& state, DisconnectedBlockTransactions* disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs);