kernel: Separate UTXO set access from validation functions #32317

pull TheCharlatan wants to merge 8 commits into bitcoin:master from TheCharlatan:spendblock changing 14 files +416 −202
  1. TheCharlatan commented at 1:15 pm on April 21, 2025: contributor

    The current code makes it hard to call functions for checking the consensus-correctness of a block without having a full UTXO set at hand. This makes implementing features such as Utreexo and non-assumevalid swiftsync, where maintaining a full UTXO set defeats their purpose, through the kernel API difficult. Checks like the coinbase subsidy, or block sigops are only available through ConnectBlock, which requires a complete coins cache.

    Solve this by moving accessing coins and the responsibility of spending them out of ConnectBlock and into a new separate function called SpendBlock. Pass a block’s spent coins through the existing CBlockUndo data structure to ConnectBlock.

    While the change is largely a refactor, some smaller behaviour changes were unavoidable. These should be highlighted in the commit messages. The last commit contains the most important part of the coins logic move, the commits before attempt to prepare it piecemeal.

    This pull request was benchmarked on benchcoin, which showed no performance regression. While discussing this pull request it was noted that pre-fetching and eliminating the need for some additional map lookups might improve performance a bit, but this does not make a measurable difference in the grand scheme of things and was not the motivation for this pull request.

    In future changes, ConnectBlock could be made even more self contained, accessing the coins from the internal map could be further optimized by consolidating existence checking and spending, and the function eventually exposed in an external kernel API.

  2. DrahtBot commented at 1:15 pm on April 21, 2025: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32317.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK w0xlt, stickies-v, stringintech, enirox001

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #32575 (refactor: Split multithreaded case out of CheckInputScripts by fjahr)
    • #32427 ((RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store by TheCharlatan)
    • #32379 (p2p: stop special-casing witness-stripped error for unconfirmed transactions by darosior)
    • #31981 (Add checkBlock() to Mining interface by Sjors)
    • #29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
    • #29060 (Policy: Report reason inputs are non standard by ismaelsadeeq)
    • #19463 (Prune locks by luke-jr)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  3. DrahtBot added the label Validation on Apr 21, 2025
  4. TheCharlatan renamed this:
    kernel: Seperate UTXO set access from validation functions
    kernel: Separate UTXO set access from validation functions
    on Apr 21, 2025
  5. in src/validation.h:737 in 2be3c59151 outdated
    708@@ -709,6 +709,9 @@ class Chainstate
    709     bool ConnectBlock(const CBlock& block, BlockValidationState& state, CBlockIndex* pindex,
    710                       CCoinsViewCache& view, bool fJustCheck = false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
    711 
    712+    bool SpendBlock(const CBlock& block, const CBlockIndex* pindex, const uint256& block_hash,
    


    Sjors commented at 1:44 pm on April 21, 2025:

    In 2be3c59151bcb840eb8cbd045a3dada6775ebc9e “validation: Add SpendBlock function”: this or the final commit would be a good time to document both SpendBlock and ConnectBlock.

    IIUC SpendBlock:

    1. Checks that all spent coins existed i) including coins created and spent within the block, in that order
    2. Optimistically updates the UTXO set with newly created coins i) checks BIP30 (not in this order, but conceptually)
    3. Populates Undo data

    And then ConnectBlock does all the other consensus checks (like verifying the input scripts). It uses the Undo data populated by SpendBlock and does not need the UTXO set.


    TheCharlatan commented at 4:15 pm on April 21, 2025:
    Yes, that is the gist of it. Will add a commit at the end to add some documentation in the header.

    TheCharlatan commented at 7:33 am on April 22, 2025:
    Added a commit.
  6. DrahtBot added the label CI failed on Apr 21, 2025
  7. DrahtBot commented at 2:43 pm on April 21, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/40876609750

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  8. TheCharlatan force-pushed on Apr 21, 2025
  9. in src/validation.h:710 in c1b36c4554 outdated
    705@@ -706,8 +706,11 @@ class Chainstate
    706     // Block (dis)connection on a given view:
    707     DisconnectResult DisconnectBlock(const CBlock& block, const CBlockIndex* pindex, CCoinsViewCache& view)
    708         EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
    709-    bool ConnectBlock(const CBlock& block, BlockValidationState& state, CBlockIndex* pindex,
    710-                      CCoinsViewCache& view, bool fJustCheck = false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
    711+    bool ConnectBlock(const CBlock& block, const uint256& block_hash, BlockValidationState& state, CBlockIndex* pindex,
    712+                      CBlockUndo& blockundo, bool fJustCheck = false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
    


    darosior commented at 5:14 pm on April 21, 2025:
    I think it would be clearer to take a vector of spent Coins directly? Or even a span?

    TheCharlatan commented at 7:02 pm on April 21, 2025:
    I might be missing something, but it would have to be a span of CTxUndo, and you’d have to move WriteBlockUndo to ConnectTip too, no? I wasn’t quite sure about doing that change here though. The implications of separately writing the undo data and raising the BlockIndex validity level are not quite clear to me yet. There is always a question where to draw the scope line with these refactors. While it might not be useful to write undo data if we ever get to the alternative kernel API client use cases, there might be other ways to handle that.

    darosior commented at 7:37 pm on April 21, 2025:
    Oh, i was just reading the diff and overlooked WriteBlockUndo! Yes that makes sense.
  10. TheCharlatan force-pushed on Apr 21, 2025
  11. DrahtBot removed the label CI failed on Apr 21, 2025
  12. w0xlt commented at 6:32 pm on April 22, 2025: contributor

    Concept ACK, as this PR increases the decoupling of components.

    nit: The name SpendBlock may not be clear (unless this concept is already known and I’m missing the point). Maybe BIP30Validation, something like that, would be clearer.

  13. in src/validation.h:733 in 76a8f22c5c outdated
    730+     * populates the CBlockUndo data structure.
    731+     *
    732+     * @param[in] block      The block to be spent
    733+     * @param[in] pindex     Points to the block map entry associated with block
    734+     * @param[in] block_hash Hash of block
    735+     * @param[out] view      Its coins are spent and used to populate CBlockUndo during its execution
    


    stickies-v commented at 9:54 am on April 29, 2025:
    nit: I think this is an @param[in,out]?
  14. in src/validation.h:715 in 76a8f22c5c outdated
    712+    /**
    713+     * Validates the block against the coins in the blockundo data, writes the
    714+     * undo data, and raises the block validity in the block index.
    715+     *
    716+     * @param[in] block       The block to be validated
    717+     * @param[in] block_hash  Hash of block
    


    stickies-v commented at 4:47 pm on April 29, 2025:

    I presume adding block_hash is done to avoid calculating CBlockHeader::GetHash() multiple times? Did you find that to have a measurable impact somewhere?

    I think the most elegant approach would be to implement caching inside CBlockHeader::GetHash() (if meaningful), but that would involve a pretty big LoC change making the CBlockHeader members private and adding getters.

    If we stick with this approach, I think at least it should be documented that block_hash is duplicated for performance reasons.


    TheCharlatan commented at 9:15 am on May 14, 2025:
    Yes, that is it. I don’t think this will actually matter in the grand scheme. I think I’ll drop it again.

    TheCharlatan commented at 1:37 pm on May 15, 2025:
    Dropped it again, but I also changed things slightly to get the block hash from the block index now.
  15. in src/consensus/tx_verify.cpp:28 in 76a8f22c5c outdated
    21+        return item.get();
    22+    } else {
    23+        return item;
    24+    }
    25+}
    26+
    


    stickies-v commented at 4:11 pm on May 6, 2025:

    Since std::reference_wrapper<T> implicitly converts to T&, I don’t think we actually need this helper? At least it compiles fine on my machine. If you’ve found any compiler issues, perhaps documenting them would be good so we can remove it in the future?

     0diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
     1index 97f8241089..f2a0259b23 100644
     2--- a/src/consensus/tx_verify.cpp
     3+++ b/src/consensus/tx_verify.cpp
     4@@ -14,16 +14,6 @@
     5 #include <util/check.h>
     6 #include <util/moneystr.h>
     7 
     8-template <typename T>
     9-static constexpr const Coin& GetCoin(const T& item)
    10-{
    11-    if constexpr (std::is_same_v<T, std::reference_wrapper<const Coin>>) {
    12-        return item.get();
    13-    } else {
    14-        return item;
    15-    }
    16-}
    17-
    18 bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime)
    19 {
    20     if (tx.nLockTime == 0)
    21@@ -143,7 +133,7 @@ unsigned int GetP2SHSigOpCount(const CTransaction& tx, const T coins)
    22     Assert(coins.size() == tx.vin.size());
    23     auto input_it = tx.vin.begin();
    24     for (auto it = coins.begin(); it != coins.end(); ++it, ++input_it) {
    25-        const Coin& coin{GetCoin(*it)};
    26+        const Coin& coin{*it};
    27         assert(!coin.IsSpent());
    28         const CTxOut &prevout = coin.out;
    29         if (prevout.scriptPubKey.IsPayToScriptHash())
    30@@ -173,7 +163,7 @@ int64_t GetTransactionSigOpCost(const CTransaction& tx, const T coins, uint32_t
    31     Assert(coins.size() == tx.vin.size());
    32     auto input_it = tx.vin.begin();
    33     for (auto it = coins.begin(); it != coins.end(); ++it, ++input_it) {
    34-        const Coin& coin{GetCoin(*it)};
    35+        const Coin& coin{*it};
    36         assert(!coin.IsSpent());
    37         const CTxOut &prevout = coin.out;
    38         nSigOps += CountWitnessSigOps(input_it->scriptSig, prevout.scriptPubKey, &input_it->scriptWitness, flags);
    39@@ -193,7 +183,7 @@ bool Consensus::CheckTxInputs(const CTransaction& tx, TxValidationState& state,
    40     Assert(coins.size() == tx.vin.size());
    41     auto input_it = tx.vin.begin();
    42     for (auto it = coins.begin(); it != coins.end(); ++it, ++input_it) {
    43-        const Coin& coin{GetCoin(*it)};
    44+        const Coin& coin{*it};
    45         assert(!coin.IsSpent());
    46 
    47         // If prev is coinbase, check that it's matured
    

    stringintech commented at 4:55 pm on May 13, 2025:
    It compiles for me as well.

    TheCharlatan commented at 9:23 am on May 14, 2025:

    stickies-v commented at 11:20 am on May 14, 2025:

    It seems like list initializing coin is triggering MSVC to copy-initialize a Coin temporary, causing a second user-defined conversion and subsequent error. I don’t really understand why direct initialization rules are different in this regard, but using it does seem to make MSVC happy: https://github.com/stickies-v/bitcoin/actions/runs/15018875180/job/42203094843

    Fix: https://github.com/stickies-v/bitcoin/commit/6f1bc11c336db6dcea2954a03520b7064f0f0e6d


    TheCharlatan commented at 12:41 pm on May 14, 2025:
    Sweet, will apply this!
  16. in src/consensus/tx_verify.cpp:162 in 76a8f22c5c outdated
    163+
    164+template unsigned int GetP2SHSigOpCount<std::span<std::reference_wrapper<const Coin>>>(
    165+    const CTransaction& tx, const std::span<std::reference_wrapper<const Coin>>);
    166+
    167+template <typename T>
    168+int64_t GetTransactionSigOpCost(const CTransaction& tx, const T coins, uint32_t flags)
    


    stickies-v commented at 4:49 pm on May 6, 2025:

    nit: for non-trivial functions, I think it helps to narrow down T instead of having to deduct it from its callsites and usage.

    What do you think about adding an IsCoinRef concept, and adding std::span to the function signature?

     0diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
     1index 97f8241089..ab27387cc4 100644
     2--- a/src/consensus/tx_verify.cpp
     3+++ b/src/consensus/tx_verify.cpp
     4@@ -14,6 +14,8 @@
     5 #include <util/check.h>
     6 #include <util/moneystr.h>
     7 
     8+#include <span>
     9+
    10 template <typename T>
    11 static constexpr const Coin& GetCoin(const T& item)
    12 {
    13@@ -158,8 +160,8 @@ template unsigned int GetP2SHSigOpCount<std::span<const Coin>>(
    14 template unsigned int GetP2SHSigOpCount<std::span<std::reference_wrapper<const Coin>>>(
    15     const CTransaction& tx, const std::span<std::reference_wrapper<const Coin>>);
    16 
    17-template <typename T>
    18-int64_t GetTransactionSigOpCost(const CTransaction& tx, const T coins, uint32_t flags)
    19+template<IsCoinRef CoinRef>
    20+int64_t GetTransactionSigOpCost(const CTransaction& tx, std::span<CoinRef> coins, uint32_t flags)
    21 {
    22     int64_t nSigOps = GetLegacySigOpCount(tx) * WITNESS_SCALE_FACTOR;
    23 
    24@@ -172,18 +174,18 @@ int64_t GetTransactionSigOpCost(const CTransaction& tx, const T coins, uint32_t
    25 
    26     Assert(coins.size() == tx.vin.size());
    27     auto input_it = tx.vin.begin();
    28-    for (auto it = coins.begin(); it != coins.end(); ++it, ++input_it) {
    29-        const Coin& coin{GetCoin(*it)};
    30+    for (const Coin& coin : coins) {
    31         assert(!coin.IsSpent());
    32         const CTxOut &prevout = coin.out;
    33         nSigOps += CountWitnessSigOps(input_it->scriptSig, prevout.scriptPubKey, &input_it->scriptWitness, flags);
    34+        ++input_it;
    35     }
    36     return nSigOps;
    37 }
    38-template int64_t GetTransactionSigOpCost<std::span<const Coin>>(
    39+template int64_t GetTransactionSigOpCost<const Coin>(
    40     const CTransaction& tx, std::span<const Coin> coins, uint32_t flags);
    41 
    42-template int64_t GetTransactionSigOpCost<std::span<std::reference_wrapper<const Coin>>>(
    43+template int64_t GetTransactionSigOpCost<std::reference_wrapper<const Coin>>(
    44     const CTransaction& tx, const std::span<std::reference_wrapper<const Coin>> coins, uint32_t flags);
    45 
    46 template <typename T>
    47diff --git a/src/consensus/tx_verify.h b/src/consensus/tx_verify.h
    48index 88b2d6164b..b8ab114306 100644
    49--- a/src/consensus/tx_verify.h
    50+++ b/src/consensus/tx_verify.h
    51@@ -48,6 +48,8 @@ unsigned int GetLegacySigOpCount(const CTransaction& tx);
    52 template <typename T>
    53 unsigned int GetP2SHSigOpCount(const CTransaction& tx, const T coins);
    54 
    55+template<typename T>
    56+concept IsCoinRef = std::convertible_to<T, const Coin&>;
    57 /**
    58  * Compute total signature operation cost of a transaction.
    59  * [@param](/bitcoin-bitcoin/contributor/param/)[in] tx    Transaction for which we are computing the cost
    60@@ -55,8 +57,8 @@ unsigned int GetP2SHSigOpCount(const CTransaction& tx, const T coins);
    61  * [@param](/bitcoin-bitcoin/contributor/param/)[in] flags Script verification flags
    62  * [@return](/bitcoin-bitcoin/contributor/return/) Total signature operation cost of tx
    63  */
    64-template <typename T>
    65-int64_t GetTransactionSigOpCost(const CTransaction& tx, const T coins, uint32_t flags);
    66+template<IsCoinRef CoinRef>
    67+int64_t GetTransactionSigOpCost(const CTransaction& tx, std::span<CoinRef> coins, uint32_t flags);
    68 
    69 /**
    70  * Check if transaction is final and can be included in a block with the
    

    (the same applies for GetLegacySigOpCount and GetP2SHSigOpCount)

    edit: I see this is already (partially) suggested and addressed in #32317 (review), I think adding the IsCoinRef concept could still be nice but less important than specifying the std::span


    TheCharlatan commented at 9:26 am on May 14, 2025:

    edit: I see this is already (partially) suggested and addressed in #32317 (review), I think adding the IsCoinRef concept could still be nice but less important than specifying the std::span

    I did implement a concept initially, but did not find it really that helpful, since the two usable template variants are concretely defined anyway.


    stickies-v commented at 10:07 am on May 14, 2025:

    Imo, there are 2 main advantages:

    • discoverability: grepping for callsites, template instantiations etc is a lot more cumbersome (and easy to miss) than just inspecting the canonical template definition
    • type safety: explicitly narrowing down the actual types that T can be makes reviewing the function easier, because you have to think about edge cases less, e.g. will the function still be safe when a new callsite is added

    This is more a general style preference, and not particularly important for this PR, so feel free to ignore / can mark as resolved.


    TheCharlatan commented at 1:36 pm on May 15, 2025:
    Thanks, added the the concept.
  17. TheCharlatan commented at 10:30 am on May 7, 2025: contributor

    Re #32317#pullrequestreview-2785006540

    nit: The name SpendBlock may not be clear (unless this concept is already known and I’m missing the point). Maybe BIP30Validation, something like that, would be clearer.

    It is doing more than BIP-30 validation, so I don’t think that would accurate either. I thought the name was pretty clear for what it does, but I’d be open to other suggestions.

  18. in src/coins.cpp:182 in 76a8f22c5c outdated
    177+    for (const auto& txin : tx.vin) {
    178+        const COutPoint& prevout = txin.prevout;
    179+        const Coin& coin = AccessCoin(prevout);
    180+        assert(!coin.IsSpent());
    181+        spent_outputs.emplace_back(coin.out);
    182+    }
    


    stickies-v commented at 3:17 pm on May 7, 2025:

    I think you can use AccessCoins here?

    0    for (const Coin& coin : AccessCoins(tx)) {
    1        assert(!coin.IsSpent());
    2        spent_outputs.emplace_back(coin.out);
    3    }
    

    stickies-v commented at 3:19 pm on May 7, 2025:
    nit: inconsistent naming between GetUnspentOutputs and spent_outputs.

    TheCharlatan commented at 1:36 pm on May 15, 2025:
    We could also use AccessCoin, but I think accessing it directly is a bit more efficient, because we don’t allocate another vector. Do you have a better suggestion for the naming there? It is all a bit confusing.
  19. in src/consensus/tx_verify.h:48 in a7e4132623 outdated
    45  * @return maximum number of sigops required to validate this transaction's inputs
    46  * @see CTransaction::FetchInputs
    47  */
    48-unsigned int GetP2SHSigOpCount(const CTransaction& tx, const CCoinsViewCache& mapInputs);
    49+template <typename T>
    50+unsigned int GetP2SHSigOpCount(const CTransaction& tx, const T coins);
    


    stringintech commented at 8:15 pm on May 7, 2025:

    Small nit: I was thinking maybe we could make the template parameter more specific like:

    0template <typename T>
    1unsigned int GetP2SHSigOpCount(const CTransaction& tx, const std::span<T> coins)
    

    This might better align with the “Sorted span” mentioned in the docs, where T would then be either const Coin or std::reference_wrapper<const Coin>. It might also make the implementation slightly more readable when calling iterator methods on the coins parameter.


    TheCharlatan commented at 12:16 pm on May 10, 2025:
    Yeah, that seems better. I had another version of this that tried to enforce everything through concepts, but thought that didn’t really help with keeping things clear, so just dropped all the fancy type hints altogether.
  20. in src/validation.h:712 in 76a8f22c5c outdated
    705@@ -706,8 +706,36 @@ class Chainstate
    706     // Block (dis)connection on a given view:
    707     DisconnectResult DisconnectBlock(const CBlock& block, const CBlockIndex* pindex, CCoinsViewCache& view)
    708         EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
    709-    bool ConnectBlock(const CBlock& block, BlockValidationState& state, CBlockIndex* pindex,
    710-                      CCoinsViewCache& view, bool fJustCheck = false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
    711+
    712+    /**
    713+     * Validates the block against the coins in the blockundo data, writes the
    714+     * undo data, and raises the block validity in the block index.
    


    stickies-v commented at 5:30 pm on May 8, 2025:
    0     * undo data to disk, and raises the block validity in the block index.
    
  21. in src/validation.cpp:2178 in 76a8f22c5c outdated
    2171@@ -2161,7 +2172,7 @@ ValidationCache::ValidationCache(const size_t script_execution_cache_bytes, cons
    2172  * Non-static (and redeclared) in src/test/txvalidationcache_tests.cpp
    2173  */
    2174 bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
    2175-                       const CCoinsViewCache& inputs, unsigned int flags, bool cacheSigStore,
    2176+                       std::vector<CTxOut>&& spent_outputs, unsigned int flags, bool cacheSigStore,
    2177                        bool cacheFullScriptStore, PrecomputedTransactionData& txdata,
    


    stringintech commented at 10:51 am on May 10, 2025:
    Is there a reason we keep passing the spent_outputs instead of removing it (to simplify the API) and requiring callers to initialize txdata before calling CheckInputScripts?

    TheCharlatan commented at 12:38 pm on May 10, 2025:
    My read is that the call to txdata.Init is delayed to after checking the script execution cache for performance reasons. Calling Init does some hashing, that would not be required if it is already in the cache. Moving the spent_outputs vector construction is already a potential performance downside for potential cache hits, though I think that is more acceptable in the grand scheme of things.

    stringintech commented at 8:34 pm on May 11, 2025:
    Oh, it makes sense. Thanks for the insights.
  22. TheCharlatan force-pushed on May 10, 2025
  23. TheCharlatan commented at 4:21 pm on May 10, 2025: contributor

    Updated 76a8f22c5c5cf48a9c36cc40db9224f0454917d0 -> 16a695fbff4a92eba3eb72ec6aa766e71c52f773 (spendblock_0 -> spendblock_1, compare)

  24. in src/validation.cpp:2534 in 16a695fbff outdated
    2557-
    2558         nInputs += tx.vin.size();
    2559 
    2560         if (!tx.IsCoinBase())
    2561         {
    2562+            auto spent_coins = std::span<const Coin>{blockundo.vtxundo[i - 1].vprevout};
    


    stickies-v commented at 2:29 pm on May 13, 2025:

    This is UB if e.g. blockundo is not populated, would add a size assertion at the beginning:

     0diff --git a/src/validation.cpp b/src/validation.cpp
     1index adec151e14..20e1dc6482 100644
     2--- a/src/validation.cpp
     3+++ b/src/validation.cpp
     4@@ -2446,6 +2446,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, const uint256& block_hash, co
     5     assert(pindex);
     6     Assume(block.GetHash() == block_hash);
     7     assert(*pindex->phashBlock == block_hash);
     8+    assert(block.vtx.size() == blockundo.vtxundo.size() + 1);  // blockundo does not contain a CTxUndo for the coinbase
     9     const bool parallel_script_checks{m_chainman.GetCheckQueue().HasThreads()};
    10 
    11     const auto time_start{SteadyClock::now()};
    
  25. in src/validation.cpp:2440 in 16a695fbff outdated
    2438@@ -2437,13 +2439,12 @@ static unsigned int GetBlockScriptFlags(const CBlockIndex& block_index, const Ch
    2439 /** Apply the effects of this block (with given index) on the UTXO set represented by coins.
    2440  *  Validity checks that depend on the UTXO set are also done; ConnectBlock()
    


    stickies-v commented at 2:31 pm on May 13, 2025:

    Validity checks that depend on the UTXO set are also done

    nit: I don’t think that’s correct anymore?

  26. in src/validation.h:717 in 16a695fbff outdated
    714+     * undo data, and raises the block validity in the block index.
    715+     *
    716+     * @param[in] block       The block to be validated
    717+     * @param[in] block_hash  Hash of block
    718+     * @param[in] blockundo   Has to contain all coins spent by block. Written to disk on successful validation
    719+     * @param[out] state      This may be set to an Error state if any error occurred validating block
    


    stickies-v commented at 2:33 pm on May 13, 2025:
    0     * [@param](/bitcoin-bitcoin/contributor/param/)[out] state      This is set to an Error state if any error occurred while validating block
    
  27. in src/validation.h:718 in 16a695fbff outdated
    715+     *
    716+     * @param[in] block       The block to be validated
    717+     * @param[in] block_hash  Hash of block
    718+     * @param[in] blockundo   Has to contain all coins spent by block. Written to disk on successful validation
    719+     * @param[out] state      This may be set to an Error state if any error occurred validating block
    720+     * @param[out] pindex     Points to the block map entry associated with block. On successful validation, raises the block validity
    


    stickies-v commented at 2:34 pm on May 13, 2025:

    nit: grammar change to highlight that pindex can be mutated here

    0     * [@param](/bitcoin-bitcoin/contributor/param/)[out] pindex     Points to the block map entry associated with block. On successful validation, its nStatus block validity is raised
    
  28. in src/test/txvalidationcache_tests.cpp:128 in 16a695fbff outdated
    123@@ -124,6 +124,9 @@ static void ValidateCheckInputsForAllFlags(const CTransaction &tx, uint32_t fail
    124 {
    125     PrecomputedTransactionData txdata;
    126 
    127+    std::vector<CTxOut> spent_outputs{active_coins_tip.GetUnspentOutputs(tx)};
    128+    txdata.Init(tx, std::move(spent_outputs));
    


    stickies-v commented at 3:39 pm on May 13, 2025:
    In a way, it’s a bit silly to add a parameter to the CheckInputScripts signature to then just move it into another parameter (txdata). An alternative approach could be to let the caller handle this, and assert txdata.m_spent_outputs_ready, but that could raise runtime assertion errors instead of catching them at compile time.

    stringintech commented at 4:53 pm on May 13, 2025:
    As @TheCharlatan noted here, delaying txdata.Init could be on purpose for the potential cache hit here. I wonder if there is a cleaner version for achieving the same goal though.
  29. stickies-v commented at 3:57 pm on May 13, 2025: contributor

    Concept ACK

    Reducing the coupling between validation functions and the UTXO set makes the validation functions more usable, and more testable. The approach taken here seems sensible, but I’m going to think about alternatives a bit more.

  30. in src/validation.cpp:2601 in 16a695fbff outdated
    2593@@ -2687,7 +2594,12 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
    2594         // * legacy (always)
    2595         // * p2sh (when P2SH enabled in flags and excludes coinbase)
    2596         // * witness (when witness enabled in flags and excludes coinbase)
    2597-        nSigOpsCost += GetTransactionSigOpCost(tx, view, flags);
    2598+        if (!tx.IsCoinBase()) {
    2599+            nSigOpsCost += GetTransactionSigOpCost(tx, std::span<const Coin>{blockundo.vtxundo[i - 1].vprevout}, flags);
    2600+        } else {
    2601+            std::vector<Coin> coin;
    2602+            nSigOpsCost += GetTransactionSigOpCost(tx, std::span<const Coin>{coin}, flags);
    


    stringintech commented at 8:01 pm on May 13, 2025:

    Looks like we have an early return in GetTransactionSigOpCost for coinbase tx and no use for coins span; so maybe we could just pass an empty span here?

    0            nSigOpsCost += GetTransactionSigOpCost<const Coin>(tx, {}, flags);
    
  31. in src/consensus/tx_verify.cpp:170 in 16a695fbff outdated
    200-{
    201-    // are the actual inputs available?
    202-    if (!inputs.HaveInputs(tx)) {
    203-        return state.Invalid(TxValidationResult::TX_MISSING_INPUTS, "bad-txns-inputs-missingorspent",
    204-                         strprintf("%s: inputs missing/spent", __func__));
    205-    }
    


    stringintech commented at 8:25 pm on May 13, 2025:
    Do we need to update the documentation for CheckTxInputs now that the input existence check is removed from it?
  32. in src/validation.cpp:3005 in 16a695fbff outdated
    3027+                    return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-txns-BIP30",
    3028+                                  "tried to overwrite transaction");
    3029+                }
    3030+            }
    3031+        }
    3032+    }
    


    stringintech commented at 8:47 pm on May 13, 2025:

    In commit 5908680, the description mentions: “By returning early in case of a BIP30 validation failure, an additional failure check for an invalid block reward in case of its failure is left out.”

    To validate my understanding: there are also other checks in ConnectBlock before the !state.IsValid() check that we may have skipped because of the early return when SpendBlock fails - including transaction input validation, script verification, fee range checks, …; Is that correct?

    It makes sense to me not to proceed with additional validation if SpendBlock already detects a consensus failure, but I’m curious why the previous code continued checking for other consensus violations even after detecting an early BIP30 failure?


    TheCharlatan commented at 10:27 am on May 14, 2025:

    There are also other checks in ConnectBlock before the !state.IsValid() check that we may have skipped because of the early return when SpendBlock fails - including transaction input validation, script verification, fee range checks, …; Is that correct?

    Good catch! Actually I think the commit message description is misleading, if not wrong. The checks you mention are skipped in any case, because we check if (!state.IsValid()) break;, but similarly the coinbase check won’t override the validation result if it is already invalid. The check is still executed though before returning.

  33. stringintech commented at 8:59 pm on May 13, 2025: none
    Concept ACK. I had a few more comments (sorry for the scattered feedback; didn’t have the chance to review everything earlier).
  34. stickies-v commented at 0:04 am on May 14, 2025: contributor
    PSA: Bitcoin Core PR Review Club will cover this PR in its next meeting at 2025-05-14 at 17:00 UTC. See https://bitcoincore.reviews/32317 for notes, questions, and instructions on how to join.
  35. TheCharlatan force-pushed on May 15, 2025
  36. TheCharlatan commented at 1:34 pm on May 15, 2025: contributor

    Thank you for the reviews @stringintech and @stickies-v!

    16a695fbff4a92eba3eb72ec6aa766e71c52f773 -> e8ac6d29273c48328f1f77886c3c3f653e6cc58e (spendblock_1 -> spendblock_2, compare)

    • Addressed @stickies-v’s comments 1, 2, 3, 4, 5 improving docstrings for ConnectBlock and SpendBlock.
    • Addressedd @stickies-v’s comment, removed block_hash argument from ConnectBlock and SpendBlock methods.
    • Addressed @stickies-v’s comment, added CoinRef concept.
    • Addressed @stickies-v’s comment, removed unneeded helper, use direct initialization to solve msvc compilation error.
    • Addressed @stickies-v’s comment, add assert for blockundo.vtxundo.size() to rule out UB. *Addressed @stringintech’s comment, pass in empty span to GetTransactionSigOpCost.
    • Addressed @stringintech’s comment, update documentation for the removal of the inputs availability check in CheckTxInputs.
    • Addressed @stringintech’s comment, removed line in commit message mentioning execution of the coinbase amount check even when another check has failed previously.
  37. DrahtBot added the label Needs rebase on May 22, 2025
  38. consensus: Use Coin span in GetP2SHSigOpCount
    Move the responsibility of retrieving coins from GetP2SHSigOpCount to
    its caller.
    
    This is a part of a series of commits for removing access to the
    CCoinsViewCache in consensus verification functions. The goal is to
    allow calling verification functions with pre-fetched, or a user-defined
    set of coins.
    
    Define two explicit template specializations for both a span of
    references to coins and a span of coins. This allows using it for both
    Coin entries referenced from the CCoinsViewCache, and from contiguous
    memory, like the vector in CBlockUndo.
    5a1e0bf6de
  39. consensus: Use Coin span in GetTransactionSigOpCost
    Move the responsibility of retrieving coins from GetTransactionSigOpCost
    to its caller.
    
    This is a part of a series of commits for removing access to the
    CCoinsViewCache in consensus verification functions. The goal is to
    allow calling verification functions with pre-fetched, or a user-defined
    set of coins.
    
    Define two explicit template specializations for both a span of
    references to coins and a span of coins. This allows using it for both
    Coin entries referenced from the CCoinsViewCache, and from contiguous
    memory, like the vector in CBlockUndo.
    2e756b4da4
  40. consensus: Use Coin span in CheckTxInputs
    Move the responsibility of retrieving coins from CheckTxInputs
    to its caller. The missing inputs check will be moved in an upcoming
    commit.
    
    This is a part of a series of commits for removing access to the
    CCoinsViewCache in consensus verification functions. The goal is to
    allow calling verification functions with pre-fetched, or a user-defined
    set of coins.
    
    Define two explicit template specializations for both a span of
    references to coins and a span of coins. This allows using it for both
    Coin entries referenced from the CCoinsViewCache, and from contiguous
    memory, like the vector in CBlockUndo.
    c454515144
  41. validation: Use vector of outputs instead of CCoinsViewCache in CheckInputScripts
    Move the responsibility of retrieving coins from the CCoinsViewCache
    in CheckInputScripts to its caller.
    
    Add a helper method in CCoinsViewCache to collect all Coin's outputs
    spent by a transaction's inputs.
    
    Callers of CCoinsViewCache are updated to either pre-fetch the spent
    outputs, or pass in an empty vector if the precomputed transaction data
    has already been initialized with the required outputs.
    
    This is a part of a series of commits for removing access to the
    CCoinsViewCache in consensus verification functions. The goal is to
    allow calling verification functions with pre-fetched, or a user-defined
    set of coins.
    44ba7c4bd3
  42. validation: Add SpendBlock function
    Move the BIP30 checks from ConnectBlock to a new SpendBlock method.
    This is a move-only change, more content to SpendBlock is added in the
    next commits. The goal is to move logic requiring access to the
    CCoinsViewCache out of ConnectBlock and to the new SpendBlock method.
    SpendBlock will in future handle all UTXO set interactions that
    previously took place in ConnectBlock.
    
    Callers of ConnectBlock now also need to call SpendBlock before. This is
    enforced in a future commit by adding a CBlockUndo argument that needs
    to be passed to both.
    
    This is a part of a series of commits for removing access to the
    CCoinsViewCache in consensus verification functions. The goal is to
    allow calling verification functions with pre-fetched, or a user-defined
    set of coins.
    55d1fac375
  43. validation: Move SetBestBlock out of ConnectBlock
    This is a part of a series of commits for removing access to the
    CCoinsViewCache in consensus verification functions. The goal is to
    allow calling verification functions with pre-fetched, or a user-defined
    set of coins.
    ecca805175
  44. 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.
    076fbdac87
  45. doc: Add docstrings for ConnectBlock and SpendBlock e604a57dcc
  46. TheCharlatan force-pushed on May 23, 2025
  47. TheCharlatan commented at 10:09 am on May 23, 2025: contributor

    Rebased e8ac6d29273c48328f1f77886c3c3f653e6cc58e -> e604a57dccac1c4e6802a3ecc7f4cb4c3229c8ed (spendblock_2 -> spendblock_3, compare)

  48. DrahtBot removed the label Needs rebase on May 23, 2025
  49. enirox001 commented at 8:45 pm on May 27, 2025: contributor
    Concept ACK. I’ll be conducting a detailed commit-by-commit code review shortly.

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-05-28 18:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me