kernel: Separate UTXO set access from validation functions #32317

sedited 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 (as done here)

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 NACK	ajtowns
Concept ACK	w0xlt, stickies-v, stringintech, enirox001, theuni
Stale ACK	ismaelsadeeq

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35003 (net_processing: improve block data I/O error handling in P2P paths by furszy)
#34875 (refactor: separate deferred script check collection from CheckInputScripts by l0rinc)
#34435 (refactor: use _MiB/_GiB consistently for byte conversions by l0rinc)
#34320 (coins: remove redundant and confusing CCoinsViewDB::HaveCoin by l0rinc)
#34132 (coins: drop error catcher, centralize fatal read handling by l0rinc)
#34124 (validation: make CCoinsView a pure virtual interface by l0rinc)
#32729 (test,refactor: extract script template helpers & widen sigop count coverage by l0rinc)
#32575 (refactor: Remove special treatment for single threaded script checking by fjahr)
#32468 (rpc: generateblock to allow multiple outputs by polespinasa)
#31974 (Drop testnet3 by Sjors)
#31132 (validation: fetch block inputs on parallel threads by andrewtoth)
#29843 (policy: Allow non-standard scripts with -acceptnonstdtxn=1 (test nets only) by ajtowns)
#29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)

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.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

CheckInputScripts(tx, state, {}, test_flags, true, add_to_cache, txdata, validation_cache, nullptr) in src/test/txvalidationcache_tests.cpp
CheckInputScripts(tx, state, {}, test_flags, true, add_to_cache, txdata, validation_cache, &scriptchecks) in src/test/txvalidationcache_tests.cpp
CheckInputScripts(tx, state, {}, test_flags, true, add_to_cache, txdata, validation_cache, &scriptchecks) in src/test/txvalidationcache_tests.cpp
CheckInputScripts(CTransaction(spend_tx), state, {}, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG, true, true, ptd_spend_tx, m_node.chainman->m_validation_cache, nullptr) in src/test/txvalidationcache_tests.cpp
CheckInputScripts(CTransaction(spend_tx), state, {}, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG, true, true, ptd_spend_tx, m_node.chainman->m_validation_cache, &scriptchecks) in src/test/txvalidationcache_tests.cpp
CheckInputScripts(CTransaction(invalid_with_cltv_tx), state, {}, SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY, true, true, txdata, m_node.chainman->m_validation_cache, nullptr) in src/test/txvalidationcache_tests.cpp
CheckInputScripts(CTransaction(invalid_with_csv_tx), state, {}, SCRIPT_VERIFY_CHECKSEQUENCEVERIFY, true, true, txdata, m_node.chainman->m_validation_cache, nullptr) in src/test/txvalidationcache_tests.cpp
CheckInputScripts(CTransaction(tx), state, {}, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, true, true, txdata, m_node.chainman->m_validation_cache, nullptr) in src/test/txvalidationcache_tests.cpp
CheckInputScripts(CTransaction(tx), state, {}, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, true, true, txdata, m_node.chainman->m_validation_cache, &scriptchecks) in src/test/txvalidationcache_tests.cpp
CheckInputScripts(tx, state, std::move(spent_outputs), scriptVerifyFlags, true, false, ws.m_precomputed_txdata, GetValidationCache()) in src/validation.cpp
chainstate.ConnectBlock(block, blockundo, state, &index_dummy, true) in src/validation.cpp

2026-03-24 17:22:14

DrahtBot added the label Validation on Apr 21, 2025

sedited renamed this:
~~kernel: Seperate UTXO set access from validation functions~~
kernel: Separate UTXO set access from validation functions
on Apr 21, 2025

in src/validation.h:712 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:

Checks that all spent coins existed i) including coins created and spent within the block, in that order
Optimistically updates the UTXO set with newly created coins i) checks BIP30 (not in this order, but conceptually)
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.

sedited 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.

sedited commented at 7:33 AM on April 22, 2025:

Added a commit.

DrahtBot added the label CI failed on Apr 21, 2025

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

<details><summary>Hints</summary>

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.

</details>

sedited force-pushed on Apr 21, 2025

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?

sedited 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.

sedited force-pushed on Apr 21, 2025

DrahtBot removed the label CI failed on Apr 21, 2025

?

project_v2_item_status_changed sedited

?

added_to_project_v2 sedited

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.

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]?

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.

sedited 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.

sedited 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.

in src/consensus/tx_verify.cpp:26 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?

diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
index 97f8241089..f2a0259b23 100644
--- a/src/consensus/tx_verify.cpp
+++ b/src/consensus/tx_verify.cpp
@@ -14,16 +14,6 @@
 #include <util/check.h>
 #include <util/moneystr.h>
 
-template <typename T>
-static constexpr const Coin& GetCoin(const T& item)
-{
-    if constexpr (std::is_same_v<T, std::reference_wrapper<const Coin>>) {
-        return item.get();
-    } else {
-        return item;
-    }
-}
-
 bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime)
 {
     if (tx.nLockTime == 0)
@@ -143,7 +133,7 @@ unsigned int GetP2SHSigOpCount(const CTransaction& tx, const T coins)
     Assert(coins.size() == tx.vin.size());
     auto input_it = tx.vin.begin();
     for (auto it = coins.begin(); it != coins.end(); ++it, ++input_it) {
-        const Coin& coin{GetCoin(*it)};
+        const Coin& coin{*it};
         assert(!coin.IsSpent());
         const CTxOut &prevout = coin.out;
         if (prevout.scriptPubKey.IsPayToScriptHash())
@@ -173,7 +163,7 @@ int64_t GetTransactionSigOpCost(const CTransaction& tx, const T coins, uint32_t
     Assert(coins.size() == tx.vin.size());
     auto input_it = tx.vin.begin();
     for (auto it = coins.begin(); it != coins.end(); ++it, ++input_it) {
-        const Coin& coin{GetCoin(*it)};
+        const Coin& coin{*it};
         assert(!coin.IsSpent());
         const CTxOut &prevout = coin.out;
         nSigOps += CountWitnessSigOps(input_it->scriptSig, prevout.scriptPubKey, &input_it->scriptWitness, flags);
@@ -193,7 +183,7 @@ bool Consensus::CheckTxInputs(const CTransaction& tx, TxValidationState& state,
     Assert(coins.size() == tx.vin.size());
     auto input_it = tx.vin.begin();
     for (auto it = coins.begin(); it != coins.end(); ++it, ++input_it) {
-        const Coin& coin{GetCoin(*it)};
+        const Coin& coin{*it};
         assert(!coin.IsSpent());
 
         // If prev is coinbase, check that it's matured

</details>

stringintech commented at 4:55 PM on May 13, 2025:

It compiles for me as well.

sedited commented at 9:23 AM on May 14, 2025:

Pushed the compiler issue on a separate branch here: https://github.com/TheCharlatan/bitcoin/actions/runs/15016860100/job/42196724477#step:10:198 .

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

sedited commented at 12:41 PM on May 14, 2025:

Sweet, will apply this!

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?

diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
index 97f8241089..ab27387cc4 100644
--- a/src/consensus/tx_verify.cpp
+++ b/src/consensus/tx_verify.cpp
@@ -14,6 +14,8 @@
 #include <util/check.h>
 #include <util/moneystr.h>
 
+#include <span>
+
 template <typename T>
 static constexpr const Coin& GetCoin(const T& item)
 {
@@ -158,8 +160,8 @@ template unsigned int GetP2SHSigOpCount<std::span<const Coin>>(
 template unsigned int GetP2SHSigOpCount<std::span<std::reference_wrapper<const Coin>>>(
     const CTransaction& tx, const std::span<std::reference_wrapper<const Coin>>);
 
-template <typename T>
-int64_t GetTransactionSigOpCost(const CTransaction& tx, const T coins, uint32_t flags)
+template<IsCoinRef CoinRef>
+int64_t GetTransactionSigOpCost(const CTransaction& tx, std::span<CoinRef> coins, uint32_t flags)
 {
     int64_t nSigOps = GetLegacySigOpCount(tx) * WITNESS_SCALE_FACTOR;
 
@@ -172,18 +174,18 @@ int64_t GetTransactionSigOpCost(const CTransaction& tx, const T coins, uint32_t
 
     Assert(coins.size() == tx.vin.size());
     auto input_it = tx.vin.begin();
-    for (auto it = coins.begin(); it != coins.end(); ++it, ++input_it) {
-        const Coin& coin{GetCoin(*it)};
+    for (const Coin& coin : coins) {
         assert(!coin.IsSpent());
         const CTxOut &prevout = coin.out;
         nSigOps += CountWitnessSigOps(input_it->scriptSig, prevout.scriptPubKey, &input_it->scriptWitness, flags);
+        ++input_it;
     }
     return nSigOps;
 }
-template int64_t GetTransactionSigOpCost<std::span<const Coin>>(
+template int64_t GetTransactionSigOpCost<const Coin>(
     const CTransaction& tx, std::span<const Coin> coins, uint32_t flags);
 
-template int64_t GetTransactionSigOpCost<std::span<std::reference_wrapper<const Coin>>>(
+template int64_t GetTransactionSigOpCost<std::reference_wrapper<const Coin>>(
     const CTransaction& tx, const std::span<std::reference_wrapper<const Coin>> coins, uint32_t flags);
 
 template <typename T>
diff --git a/src/consensus/tx_verify.h b/src/consensus/tx_verify.h
index 88b2d6164b..b8ab114306 100644
--- a/src/consensus/tx_verify.h
+++ b/src/consensus/tx_verify.h
@@ -48,6 +48,8 @@ unsigned int GetLegacySigOpCount(const CTransaction& tx);
 template <typename T>
 unsigned int GetP2SHSigOpCount(const CTransaction& tx, const T coins);
 
+template<typename T>
+concept IsCoinRef = std::convertible_to<T, const Coin&>;
 /**
  * Compute total signature operation cost of a transaction.
  * [@param](/bitcoin-bitcoin/contributor/param/)[in] tx    Transaction for which we are computing the cost
@@ -55,8 +57,8 @@ unsigned int GetP2SHSigOpCount(const CTransaction& tx, const T coins);
  * [@param](/bitcoin-bitcoin/contributor/param/)[in] flags Script verification flags
  * [@return](/bitcoin-bitcoin/contributor/return/) Total signature operation cost of tx
  */
-template <typename T>
-int64_t GetTransactionSigOpCost(const CTransaction& tx, const T coins, uint32_t flags);
+template<IsCoinRef CoinRef>
+int64_t GetTransactionSigOpCost(const CTransaction& tx, std::span<CoinRef> coins, uint32_t flags);
 
 /**
  * Check if transaction is final and can be included in a block with the

</details>

(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

sedited 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.

sedited commented at 1:36 PM on May 15, 2025:

Thanks, added the the concept.

sedited 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.

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?

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

stickies-v commented at 3:19 PM on May 7, 2025:

nit: inconsistent naming between GetUnspentOutputs and spent_outputs.

sedited 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.

stickies-v commented at 12:35 PM on May 29, 2025:

I hadn't considered the extra vector allocation, yeah I'm okay leaving this as-is then.

I did nerd-snipe myself into looking into having AccessCoins return a view would be quite elegant in some ways, allowing lazy evaluation, but seems like it would require quite a bit of overhaul and might have other drawbacks I've not considered yet.

    auto AccessCoins(const CTransaction& tx) const
    {
        return tx.vin | std::views::transform([this](const CTxIn& input) -> const Coin& {
            return this->AccessCoin(input.prevout);
        });
    }

alexanderwiederin commented at 9:19 AM on October 24, 2025:

Do you have a better suggestion for the naming there? It is all a bit confusing.

I agree the naming is confusing. I'd suggest GetUnspentOutputs -> GetPreviousOutputs and spent_outputs -> previous_outputs to avoid the spent/unspent terminology. What do you think?

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:

template <typename T>
unsigned 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.

sedited 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.

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:

     * undo data to disk, and raises the block validity in the block index.

in src/validation.cpp:2176 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?

sedited 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.

sedited force-pushed on May 10, 2025

sedited commented at 4:21 PM on May 10, 2025: contributor

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

Addressed @stringintech's comment, now using std::span<T> as templated argument.

in src/validation.cpp:2560 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:

diff --git a/src/validation.cpp b/src/validation.cpp
index adec151e14..20e1dc6482 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2446,6 +2446,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, const uint256& block_hash, co
     assert(pindex);
     Assume(block.GetHash() == block_hash);
     assert(*pindex->phashBlock == block_hash);
+    assert(block.vtx.size() == blockundo.vtxundo.size() + 1);  // blockundo does not contain a CTxUndo for the coinbase
     const bool parallel_script_checks{m_chainman.GetCheckQueue().HasThreads()};
 
     const auto time_start{SteadyClock::now()};

</details>

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?

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:

     * [@param](/bitcoin-bitcoin/contributor/param/)[out] state      This is set to an Error state if any error occurred while validating block

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

     * [@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

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.

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.

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?

            nSigOpsCost += GetTransactionSigOpCost<const Coin>(tx, {}, flags);

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?

in src/validation.cpp:3032 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?

sedited 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.

stringintech commented at 8:59 PM on May 13, 2025: contributor

Concept ACK. I had a few more comments (sorry for the scattered feedback; didn't have the chance to review everything earlier).

stickies-v commented at 12: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.

sedited force-pushed on May 15, 2025

sedited 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.

DrahtBot added the label Needs rebase on May 22, 2025

sedited force-pushed on May 23, 2025

sedited commented at 10:09 AM on May 23, 2025: contributor

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

Fixed conflict with #32575

DrahtBot removed the label Needs rebase on May 23, 2025

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.

DrahtBot added the label CI failed on May 30, 2025

sedited force-pushed on Jun 2, 2025

sedited commented at 2:19 PM on June 2, 2025: contributor

Rebased e604a57dccac1c4e6802a3ecc7f4cb4c3229c8ed -> 15ff9cb1789b7fe69c0ad24d737712aa6955e9cb (spendblock_3 -> spendblock_4, compare)

Fixed silent merge conflict with #32304

DrahtBot removed the label CI failed on Jun 2, 2025

DrahtBot added the label Needs rebase on Jun 3, 2025

sedited force-pushed on Jun 3, 2025

sedited commented at 11:16 AM on June 3, 2025: contributor

Rebased 15ff9cb1789b7fe69c0ad24d737712aa6955e9cb -> df0eb65bd98f300618cccd667720f1ccc6145865 (spendblock_4 -> spendblock_5, compare)

Fixed conflict with #32644

DrahtBot removed the label Needs rebase on Jun 3, 2025

DrahtBot added the label Needs rebase on Jun 11, 2025

sedited commented at 10:22 AM on June 12, 2025: contributor

Rebased df0eb65bd98f300618cccd667720f1ccc6145865 -> 969799291d34da706b43209ba1209038ec349424 (spendblock_5 -> spendblock_6, compare)

Fixed conflict with #32673
Fixed conflict with #32421

sedited force-pushed on Jun 12, 2025

DrahtBot removed the label Needs rebase on Jun 12, 2025

DrahtBot added the label Needs rebase on Jun 19, 2025

sedited force-pushed on Jun 19, 2025

sedited commented at 2:08 PM on June 19, 2025: contributor

Rebased 969799291d34da706b43209ba1209038ec349424 -> 6e90a0eb605b5477b4e91128e08ed1cc5501a939 (spendblock_6 -> spendblock_7, compare)

Fixed conflict with #31981

DrahtBot removed the label Needs rebase on Jun 19, 2025

DrahtBot added the label CI failed on Jul 6, 2025

DrahtBot commented at 10:46 PM on July 6, 2025: contributor

🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/runs/44423644781 LLM reason (✨ experimental): The failure is caused by a compilation error in script_p2sh_tests.cpp due to an incompatible function call to GetP2SHSigOpCount.

<details><summary>Hints</summary>

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.

</details>

sedited force-pushed on Jul 9, 2025

sedited commented at 1:37 PM on July 9, 2025: contributor

Rebased 6e90a0eb605b5477b4e91128e08ed1cc5501a939 -> 1f96d1cefc721b579b7aa3b05c2e65df62c0c5cf (spendblock_7 -> spendblock_8, compare)

Fixed silent merge conflict with #32850

DrahtBot removed the label CI failed on Jul 9, 2025

DrahtBot added the label CI failed on Jul 22, 2025

DrahtBot commented at 11:29 AM on July 22, 2025: contributor

🚧 At least one of the CI tasks failed. Task previous releases, depends DEBUG: https://github.com/bitcoin/bitcoin/runs/45643912981 LLM reason (✨ experimental): The CI failure is caused by compilation errors in transaction_tests.cpp due to mismatched function calls to GetP2SHSigOpCount, indicating a code or API mismatch.

<details><summary>Hints</summary>

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.

</details>

sedited force-pushed on Jul 24, 2025

sedited commented at 10:32 AM on July 24, 2025: contributor

Updated 1f96d1cefc721b579b7aa3b05c2e65df62c0c5cf -> 10c3a0689a3b73eb2b291ee81dd85066fa32497d (spendblock_8 -> spendblock_9, compare)

Fixed silent merge conflict with #32521

DrahtBot removed the label CI failed on Jul 24, 2025

DrahtBot added the label Needs rebase on Aug 8, 2025

sedited force-pushed on Aug 19, 2025

sedited commented at 11:45 AM on August 19, 2025: contributor

Rebased 10c3a0689a3b73eb2b291ee81dd85066fa32497d -> 3843a15f5b5b50d47c2524f607ba0b01c90d42b1 (spendblock_9 -> spendblock_10, compare)

Fixed merge conflict with #33105

DrahtBot removed the label Needs rebase on Aug 19, 2025

DrahtBot added the label Needs rebase on Aug 20, 2025

sedited force-pushed on Aug 21, 2025

sedited commented at 2:49 PM on August 21, 2025: contributor

Rebased 3843a15f5b5b50d47c2524f607ba0b01c90d42b1 -> 5537ac00983a9abd24689411b5d76058d7a02f1b (spendblock_10 -> spendblock_11, compare)

Fixed conflict with #33078

DrahtBot removed the label Needs rebase on Aug 21, 2025

sedited force-pushed on Sep 4, 2025

sedited commented at 1:31 PM on September 4, 2025: contributor

Rebased 5537ac00983a9abd24689411b5d76058d7a02f1b -> b27be6cb18dde53863d83d2f2f61d4c92916257b (spendblock_11 -> spendblock_12, compare)

ajtowns commented at 9:27 AM on September 9, 2025: contributor

Pass a block's spent coins through the existing CBlockUndo data structure to ConnectBlock

I don't think this is really a good direction. We already have a generic interface for supplying info about unspent coins: CCoinsView; I think we should be using that, rather than requiring users to convert their utxo data store into our undo format?

The thing that gets in the way of that seems to be the AccessCoin function returning a Coin by reference rather than value, but that doesn't seem particularly more efficient (it's an 8 byte copy rather than a 56 byte one unless the coin being spent is very non-standard, at least since #32279), and also seems a bit risky (in that the return value's lifetime is dependent on when the cache gets invalidated which is somewhat unpredictable).

But that seems to be fixable: https://github.com/ajtowns/bitcoin/commits/202509-ccoinsview/

(When AccessCoin was introduced in #1677, CScript always involved an additional heap allocation as it was just a vector<char>, so the reference would have saved an allocation. That changed a few years later when prevector was introduced in #6914 which allowed small scripts to not require an allocation. However, I haven't actually checked to see if the 8-vs-56-byte issue is actually as trivial as I think it should be)

sedited commented at 8:44 AM on September 10, 2025: contributor

My hesitation was mostly about re-using the coins cache as is. In my opinion it is the wrong data structure to use in this case. Potential clients of the kernel are often capable of supplying the coins in order as consumed by a transaction's inputs. Requiring these to be stored in a map seems wrong. I.e. if a utreexo client has all the leaf data to go with a transaction, it should not have to use a less efficient data structure that consumes more memory to pass in the coins. But maybe we have to build out some of the usages here in order to see what the actual drawbacks are? My current thinking is though that just passing in the required coins contiguously, i.e. use the undo data structure, is the more pragmatic solution for most external users that do not wish to implement a utxo set.

However this is also not exactly what I think you are suggesting here, since the CCoinsView could eventually provide a view over contiguous data too. As implemented at the moment, that seems more confusing to use though compared to a nested vector of coins.

Another alternative solution that was suggested by cfields is introducing a new transaction type that holds the coins alongside the prevouts. I think that would be an interesting alternative too.

ajtowns commented at 2:19 AM on September 11, 2025: contributor

My hesitation was mostly about re-using the coins cache as is. In my opinion it is the wrong data structure to use in this case. Potential clients of the kernel are often capable of supplying the coins in order as consumed by a transaction's inputs.

I think that's a false optimisation for two reasons: first, is that the primary client of the bitcoin core kernel is the bitcoin core node software, we should be designing for that case, not for other hypothetical clients; second, looking up a map isn't very expensive in comparison to the other parts of block validation, particular signature validation. So if we already know the exact utxos spent in a block, converting just those utxos into a CCoinsView and using that seems likely to be within the margin of error for any benchmarks on realistic blocks. If we were designing this from scratch for a utreexo-based system, then yeah, the undo-centric approach would make sense (and including the prevouts inside the transaction makes a lot of sense then), but that's not the primary use case here (at least not yet).

I think constructing a CCoinsView from a block and undo would look something like this (on top of the previous changes I suggested above, untested):

class BlockUndoCoinsView : public CCoinsView
{
private:
    struct CoinCmp
    {
        bool operator()(const COutPoint* l, const COutPoint* r) const {
            if (l && r) return *l < *r;
            return r != nullptr;
        }
    };
    std::map<const COutPoint*, const Coin*, CoinCmp> m_coins;

public:
    explicit BlockUndoCoinsView(const CBlock& block, const CBlockUndo& undo)
    {
        size_t i = 0;
        for (const auto& txundo : undo.vtxundo) {
            ++i; // skip the coinbase tx
            if (i >= block.vtx.size()) break;
            size_t j = 0;
            const auto& tx = block.vtx[i];
            for (const auto& prevout : txundo.vprevout) {
                if (j >= tx->vin.size()) break;
                const auto& outpoint = tx->vin[j].prevout;
                // XXX skip coins whose txid was from a previous tx in this block? 
                m_coins.try_emplace(&outpoint, &prevout);
                ++j;
            }
        }
    }

    std::optional<Coin> GetCoin(const COutPoint& outpoint) const override
    {
        auto coin = m_coins.find(&outpoint);
        if (coin == m_coins.end()) return std::nullopt;
        return *coin->second;
    }

    bool HaveCoin(const COutPoint &outpoint) const override
    {
        return m_coins.find(&outpoint) != m_coins.end();
    }
};

I guess what I mean is I think we can achieve the stated goal of this PR (being able to validate blocks/txs without the full utxo set) with a smaller change.

If returning a const Coin& rather than a copy is needed for performance, it might be better to do that through an RAII cursor object that guarantees the reference remains valid until the cursor is destroyed.

sedited commented at 2:12 PM on September 11, 2025: contributor

My goal with this PR was making the bulk of ConnectBlock logic work without using a CCoinsViewCache by moving the call to UpdateCoins out of the function. The side effect of that is that the CBlockUndo is populated outside of ConnectBlock too and has to be passed in. Since it already holds all the required coins, I rather took those coins vectors, than additionally requiring a view providing the same data (next to the reasons I provided in my comment above).

The change was motivated by wanting to provide a single method for validating a block with external coins, such that others can use it verbatim in the near future. The alternative of just changing some of the functions that ConnectBlock calls to be able to accept external coins (through either a view or contiguous data), and then splitting the rest of the ConnectBlock logic, such as amount, locktime, assumevalid, and validity semantics into separate functions seemed inferior to me. If an external user would want to use these functions to achieve the same outcome, a single function not only seems friendlier, but also less likely to result in bugs.

I could change this patch set to pass in a CCoinsView and then either handle the undo data outside of ConnectBlock, or populate it through the view within ConnectBlock.

DrahtBot added the label Needs rebase on Oct 8, 2025

sedited force-pushed on Oct 13, 2025

sedited commented at 9:36 AM on October 13, 2025: contributor

Rebased b27be6cb18dde53863d83d2f2f61d4c92916257b -> 9ce01e051bae5fb1d48d6d297eb011b13d20ec76 (spendblock_12 -> spendblock_13, compare)

DrahtBot removed the label Needs rebase on Oct 13, 2025

sedited force-pushed on Nov 5, 2025

sedited commented at 8:13 PM on November 5, 2025: contributor

Rebased 9ce01e051bae5fb1d48d6d297eb011b13d20ec76 -> 5988c7172406e3caca32fc78c011d878a1abdf1e (spendblock_13 -> spendblock_14, compare)

Fixed a silent merge conflict

purpleKarrot commented at 9:50 AM on November 6, 2025: contributor

A function that takes a const std::span<T> coins argument cannot modify coins, but it has mutable access to coins[x].

DrahtBot added the label Needs rebase on Nov 7, 2025

sedited force-pushed on Nov 12, 2025

sedited commented at 2:28 PM on November 12, 2025: contributor

Rebased 5988c7172406e3caca32fc78c011d878a1abdf1e -> 6493e47fd8d32c030d553f791caa48fcdb848c0b (spendblock_14 -> spendblock_15, compare)

DrahtBot removed the label Needs rebase on Nov 12, 2025

DrahtBot added the label Needs rebase on Dec 2, 2025

sedited force-pushed on Dec 3, 2025

sedited commented at 10:29 AM on December 3, 2025: contributor

Rebased 6493e47fd8d32c030d553f791caa48fcdb848c0b -> a0e9578ec1ff1d8ec57f76a37e389aeeb554a639 (spendblock_15 -> spendblock_16, compare)

Fixed conflict with #33960

DrahtBot removed the label Needs rebase on Dec 3, 2025

?

project_v2_item_status_changed sedited

in src/test/script_p2sh_tests.cpp:384 in 5709d073e0 outdated

 380 | @@ -380,7 +381,8 @@ BOOST_AUTO_TEST_CASE(AreInputsStandard)
 381 |      txToNonStd1.vin[0].scriptSig << std::vector<unsigned char>(sixteenSigops.begin(), sixteenSigops.end());
 382 |  
 383 |      BOOST_CHECK(!::AreInputsStandard(CTransaction(txToNonStd1), coins));
 384 | -    BOOST_CHECK_EQUAL(GetP2SHSigOpCount(CTransaction(txToNonStd1), coins), 16U);
 385 | +    auto coinsTxToNonStd1{coins.AccessCoins(CTransaction(txToNonStd1))};

ismaelsadeeq commented at 1:23 PM on December 17, 2025:

In "consensus: Use Coin span in GetP2SHSigOpCount" 5709d073e06280c915e7d74e8f49e4904598c348

nit: here and other places, these new variables should be snake case?

in src/consensus/tx_verify.h:60 in 3bdb4a89a9

  55 | @@ -56,12 +56,13 @@ unsigned int GetP2SHSigOpCount(const CTransaction& tx, const std::span<T> coins)
  56 |  
  57 |  /**
  58 |   * Compute total signature operation cost of a transaction.
  59 | - * @param[in] tx     Transaction for which we are computing the cost
  60 | - * @param[in] inputs Map of previous transactions that have outputs we're spending
  61 | + * @param[in] tx    Transaction for which we are computing the cost
  62 | + * @param[in] coins Sorted span of Coins containing previous transaction outputs we're spending

ismaelsadeeq commented at 1:59 PM on December 17, 2025:

In "consensus: Use Coin span in GetP2SHSigOpCount" 5709d073e06280c915e7d74e8f49e4904598c348

Here and other commits. Sorted in what order?

in src/validation.h:764 in a0e9578ec1 outdated

 759 | +     * @param[in] pindex     Points to the block map entry associated with block
 760 | +     * @param[in, out] view  Its coins are spent and used to populate CBlockUndo during its execution
 761 | +     * @param[out] state     This may be set to an Error state if any error occurred processing them
 762 | +     * @param[out] blockundo Coins consumed by the block are added to it.
 763 | +     */
 764 |      bool SpendBlock(const CBlock& block, const CBlockIndex* pindex,

ismaelsadeeq commented at 4:52 PM on December 17, 2025:

Since we are doing this, I think this should also be separated into two functions.

EnforceBIP30 — a method that performs BIP30 validation.
SpendBlock — for each transaction in the block, checks that the outputs it is spending exist, spends them, and populates the CBlockUndo data structure.

It would also make testing easier, and removing BIP30 after a consensus cleanup soft fork would be straightforward.

What I will want to see is we have just SpendBlock and non-contextual block validation using the block data and undo data in a series of consensus rules validation functions.

in src/validation.cpp:2832 in c937b12745 outdated

2827 | +        return true;
2828 | +    }
2829 | +
2830 | +    // verify that the view's current state corresponds to the previous block
2831 | +    uint256 hashPrevBlock = pindex->pprev == nullptr ? uint256() : pindex->pprev->GetBlockHash();
2832 | +    assert(hashPrevBlock == view.GetBestBlock());

ismaelsadeeq commented at 4:56 PM on December 17, 2025:

In "validation: Add SpendBlock function" c937b12745971435111eab948e505580754d56bd why duplicate this, I think we should just add a comment warning that we expect they are already executed before?

sedited commented at 10:26 PM on December 17, 2025:

I'm not sure what you mean here. Afaict this is moved over from ConnectBlock.

sedited commented at 2:10 PM on December 19, 2025:

I think I'd rather keep these duplicated for now. These are short-circuits to our logic, similar to how the coinbase checks work in various places. Based on that I'd rather keep them in place. This could also easily be improved in a follow-up.

in src/validation.cpp:2812 in c937b12745 outdated

2808 | @@ -2899,6 +2809,127 @@ static void UpdateTipLog(
2809 |                     !warning_messages.empty() ? strprintf(" warning='%s'", warning_messages) : "");
2810 |  }
2811 |  
2812 | +bool Chainstate::SpendBlock(

ismaelsadeeq commented at 5:02 PM on December 17, 2025:

In "validation: Add SpendBlock function" c937b12745971435111eab948e505580754d56bd

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.

This is counter to the objective of the PR; we are still accessing the UTXO set in this block validation function. My hope was that we would have two UTXO-accessing validation functions that 1 populate the block undo data and 2. BIP30 separately, while the remaining validation functions would simply get passed the block undo data and operate as stateless validation functions.

sedited commented at 10:59 AM on December 18, 2025:

It is important that the BIP30 validation is done before we spend the coins. The current split makes sense to me for this reason. Can you elaborate a bit more what you think might not be optimal here?

sedited commented at 2:09 PM on December 19, 2025:

I'm still not sure if this is a good thing. I don't see why you would ever spend the coins in the utxo set without also doing the bip30 checks beforehand. To this end, I did implement it, but added a tag type that gets passed from BIP30Validate to SpendBlock: https://github.com/sedited/bitcoin/tree/spendblock_17 . Let me know what you think.

ismaelsadeeq commented at 11:54 AM on December 22, 2025:

I’ve reviewed the branch, and it looks good overall. My only concern is the last commit that adds the tag. Initially, I thought it should be the caller’s responsibility to decide whether bip30validate is called, rather than mandating it here.

My motivation for separating SpendBlock from BIP30Validate is discussed in: https://github.com/stratum-mining/sv2-apps/issues/120

The branch without the last commit would make it easier to implement a variant of the idea described in https://github.com/stratum-mining/sv2-apps/issues/120, where block validation for this JDS is as follows call SpendBlock and then ConnectBlock skip BIP30. However, the idea of skipping consensus checks is not acceptable.

Given that, I’m inclined to keep this as-is. For https://github.com/stratum-mining/sv2-apps/issues/120, we can instead expose IPC methods: SpendBlock, which takes a block and returns the coins, and ValidateBlock, which performs the full block validation checks. Since blocks from miners tend to share many transactions, they can still benefit from this approach, because the JDS needs to only call SpendBlock to validate newly seen transactions.

For Utreexo client, they provide their own undo data and cannot perform BIP30 validation anyway, so they benefit from this approach as well.

Overall, this is still an improvement over the current status quo.

ismaelsadeeq commented at 5:05 PM on December 17, 2025: member

Concept ACK

ACK first four commits, I think the remaining commits can even be made better. comments are inline

ismaelsadeeq approved

ismaelsadeeq commented at 11:55 AM on December 22, 2025: member

Code review ACK a0e9578ec1ff1d8ec57f76a37e389aeeb554a639

DrahtBot requested review from stringintech on Dec 22, 2025

DrahtBot requested review from stickies-v on Dec 22, 2025

DrahtBot requested review from w0xlt on Dec 22, 2025

sedited force-pushed on Jan 27, 2026

sedited commented at 2:31 PM on January 27, 2026: contributor

Rebased a0e9578ec1ff1d8ec57f76a37e389aeeb554a639 -> 00da3f04701789a6a238c3bee67e389396c01d6a (spendblock_16 -> spendblock_17, compare)

Rebased to get the recent kernel changes upstream

Also addressed some smaller comments that @l0rinc gave out of band:

Fixed documentation (replaced sorted/ordered, clarified where BIP30 checks apply)
Simplified passing outputs to the script checks by using push_back instead of a transform that was left over from an earlier iteration.
Fixed connect block time tracking for benching logs.

l0rinc commented at 12:50 PM on January 29, 2026: contributor

Ran a reindex-chainstate and pruned IBD benchmark on this:

for DBCACHE in 450 4500; do \
  COMMITS="34a5ecadd7203121b03ac692d22a752d2a364111 86afa0807063e88ee0b5c2c54c07504abccd3c18"; \
  STOP=933339; CC=gcc; CXX=g++; \
  BASE_DIR="/mnt/my_storage"; DATA_DIR="$BASE_DIR/BitcoinData"; LOG_DIR="$BASE_DIR/logs"; \
  (echo ""; for c in $COMMITS; do git fetch -q origin $c && git log -1 --pretty='%h %s' $c || exit 1; done) && \
  (echo "" && echo "$(date -I) | reindex-chainstate | ${STOP} blocks | dbcache ${DBCACHE} | $(hostname) | $(uname -m) | $(lscpu | grep 'Model name' | head -1 | cut -d: -f2 | xargs) | $(nproc) cores | $(free -h | awk '/^Mem:/{print $2}') RAM | $(lsblk -no ROTA $(df --output=source $BASE_DIR | tail -1) | grep -q 0 && echo SSD || echo HDD)"; echo "") && \
  hyperfine \
    --sort command \
    --runs 1 \
    --export-json "$BASE_DIR/rdx-$(sed -E 's/(\w{8})\w+ ?/\1-/g;s/-$//'<<<"$COMMITS")-$STOP-$DBCACHE-$CC.json" \
    --parameter-list COMMIT ${COMMITS// /,} \
    --prepare "killall -9 bitcoind 2>/dev/null; rm -f ./build/bin/bitcoind; git clean -fxd; git reset --hard {COMMIT} && \
      cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release -DENABLE_IPC=OFF && ninja -C build bitcoind -j1 && \
      ./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=$STOP -dbcache=1000 -printtoconsole=0; sleep 20; rm -f $DATA_DIR/debug.log" \
    --conclude "killall bitcoind || true; sleep 5; grep -q 'height=0' $DATA_DIR/debug.log && grep -q 'Disabling script verification at block [#1](/bitcoin-bitcoin/1/)' $DATA_DIR/debug.log && grep -q 'height=$STOP' $DATA_DIR/debug.log && grep 'Bitcoin Core version' $DATA_DIR/debug.log | grep -q "$(printf %.12s {COMMIT})"; \
                cp $DATA_DIR/debug.log $LOG_DIR/debug-{COMMIT}-$(date +%s).log" \
    "COMPILER=$CC ./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=$STOP -dbcache=$DBCACHE -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0";
done

34a5ecadd7 Merge bitcoin/bitcoin#34397: doc: fix arg name hints so bugprone can validate them
86afa08070 doc: Add docstrings for ConnectBlock and SpendBlock

2026-01-26 | reindex-chainstate | 933339 blocks | dbcache 450 | i9-ssd | x86_64 | Intel(R) Core(TM) i9-9900K CPU @ 3.60GHz | 16 cores | 62Gi RAM | SSD

Benchmark 1: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=933339 -dbcache=450 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 34a5ecadd7203121b03ac692d22a752d2a364111)
  Time (abs ≡):        21877.110 s               [User: 51445.256 s, System: 3154.079 s]

Benchmark 2: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=933339 -dbcache=450 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 86afa0807063e88ee0b5c2c54c07504abccd3c18)
  Time (abs ≡):        21381.988 s               [User: 50329.401 s, System: 3096.864 s]

Relative speed comparison
        1.02          COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=933339 -dbcache=450 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 34a5ecadd7203121b03ac692d22a752d2a364111)
        1.00          COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=933339 -dbcache=450 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 86afa0807063e88ee0b5c2c54c07504abccd3c18)


34a5ecadd7 Merge bitcoin/bitcoin#34397: doc: fix arg name hints so bugprone can validate them
86afa08070 doc: Add docstrings for ConnectBlock and SpendBlock

2026-01-27 | reindex-chainstate | 933339 blocks | dbcache 4500 | i9-ssd | x86_64 | Intel(R) Core(TM) i9-9900K CPU @ 3.60GHz | 16 cores | 62Gi RAM | SSD

Benchmark 1: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=933339 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 34a5ecadd7203121b03ac692d22a752d2a364111)
  Time (abs ≡):        17358.496 s               [User: 34851.495 s, System: 944.666 s]

Benchmark 2: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=933339 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 86afa0807063e88ee0b5c2c54c07504abccd3c18)
  Time (abs ≡):        16939.910 s               [User: 34064.373 s, System: 932.663 s]

Relative speed comparison
        1.02          COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=933339 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 34a5ecadd7203121b03ac692d22a752d2a364111)
        1.00          COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=933339 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 86afa0807063e88ee0b5c2c54c07504abccd3c18)

</details>

COMMITS="34a5ecadd7203121b03ac692d22a752d2a364111 86afa0807063e88ee0b5c2c54c07504abccd3c18"; \                                                      
STOP=933339; DBCACHE=450; PRUNE=550; \
CC=gcc; CXX=g++; \
BASE_DIR="/mnt/my_storage"; DATA_DIR="$BASE_DIR/ShallowBitcoinData"; LOG_DIR="$BASE_DIR/logs"; \
(echo ""; for c in $COMMITS; do git fetch -q origin $c && git log -1 --pretty='%h %s' $c || exit 1; done) && \
(echo "" && echo "$(date -I) | pruned IBD | ${STOP} blocks | dbcache ${DBCACHE} | pruning ${PRUNE} | $(hostname) | $(uname -m) | $(lscpu | grep 'Model name' | head -1 | cut -d: -f2 | xargs) | $(nproc) cores | $(free -h | awk '/^Mem:/{print $2}') RAM | $(df -T $BASE_DIR | awk 'NR==2{print $2}') | $(lsblk -no ROTA $(df --output=source $BASE_DIR | tail -1) | grep -q 0 && echo SSD || echo HDD)"; echo "") &&\
hyperfine \
  --sort command \
  --runs 2 \
  --export-json "$BASE_DIR/ibd-$(sed -E 's/(\w{8})\w+ ?/\1-/g;s/-$//'<<<"$COMMITS")-$STOP-$DBCACHE-$CC.json" \
  --parameter-list COMMIT ${COMMITS// /,} \
  --prepare "killall -9 bitcoind 2>/dev/null; rm -rf $DATA_DIR/*; git clean -fxd; git reset --hard {COMMIT} && \
    cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release && ninja -C build bitcoind -j2 && \
    ./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=1 -prune=$PRUNE -printtoconsole=0; sleep 20" \
  --conclude "killall bitcoind || true; sleep 5; grep -q 'height=0' $DATA_DIR/debug.log && grep -q 'Disabling script verification at block [#1](/bitcoin-bitcoin/1/)' $DATA_DIR/debug.log && grep -q 'height=$STOP' $DATA_DIR/debug.log && grep 'Bitcoin Core version' $DATA_DIR/debug.log | grep -q "$(printf %.12s {COMMIT})"; \
              cp $DATA_DIR/debug.log $LOG_DIR/debug-{COMMIT}-$(date +%s).log" \
  "COMPILER=$CC ./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=$STOP -dbcache=$DBCACHE -blocksonly -prune=$PRUNE -printtoconsole=0"

34a5ecadd7 Merge bitcoin/bitcoin#34397: doc: fix arg name hints so bugprone can validate them
86afa08070 doc: Add docstrings for ConnectBlock and SpendBlock

2026-01-26 | pruned IBD | 933339 blocks | dbcache 450 | pruning 550 | i7-hdd | x86_64 | Intel(R) Core(TM) i7-7700 CPU @ 3.60GHz | 8 cores | 62Gi RAM | ext4 | HDD

Benchmark 1: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/ShallowBitcoinData -stopatheight=933339 -dbcache=450 -blocksonly -prune=550 -printtoconsole=0 (COMMIT = 34a5ecadd7203121b03ac692d22a752d2a364111)
  Time (mean ± σ):     46755.472 s ± 362.338 s    [User: 67839.785 s, System: 4468.602 s]
  Range (min … max):   46499.260 s … 47011.684 s    2 runs
 
Benchmark 2: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/ShallowBitcoinData -stopatheight=933339 -dbcache=450 -blocksonly -prune=550 -printtoconsole=0 (COMMIT = 86afa0807063e88ee0b5c2c54c07504abccd3c18)
  Time (mean ± σ):     46612.104 s ± 167.365 s    [User: 67656.746 s, System: 4637.458 s]
  Range (min … max):   46493.759 s … 46730.449 s    2 runs
 
Relative speed comparison
        1.00 ±  0.01  COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/ShallowBitcoinData -stopatheight=933339 -dbcache=450 -blocksonly -prune=550 -printtoconsole=0 (COMMIT = 34a5ecadd7203121b03ac692d22a752d2a364111)
        1.00          COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/ShallowBitcoinData -stopatheight=933339 -dbcache=450 -blocksonly -prune=550 -printtoconsole=0 (COMMIT = 86afa0807063e88ee0b5c2c54c07504abccd3c18)

</details>

The actual IBD didn't show any speed difference - but was also quite slow, so likely bandwidth constrained. The reindex-chainstate showed a 2% speedup for both 450 and 4500 dbcache, I would be interested to see if this is just a coincidence, of if not, why would this result in a speedup. I'm planning on reviewing this change in detail, I only managed to do these so far.

Edit: finished reindex-chainstate on i7 and i9 as well:

for DBCACHE in 450 4500; do \
  COMMITS="34a5ecadd7203121b03ac692d22a752d2a364111 86afa0807063e88ee0b5c2c54c07504abccd3c18"; \
  STOP=933339; CC=gcc; CXX=g++; \
  BASE_DIR="/mnt/my_storage"; DATA_DIR="$BASE_DIR/BitcoinData"; LOG_DIR="$BASE_DIR/logs"; \
  (echo ""; for c in $COMMITS; do git fetch -q origin $c && git log -1 --pretty='%h %s' $c || exit 1; done) && \
  (echo "" && echo "$(date -I) | reindex-chainstate | ${STOP} blocks | dbcache ${DBCACHE} | $(hostname) | $(uname -m) | $(lscpu | grep 'Model name' | head -1 | cut -d: -f2 | xargs) | $(nproc) cores |
 $(free -h | awk '/^Mem:/{print $2}') RAM | $(lsblk -no ROTA $(df --output=source $BASE_DIR | tail -1) | grep -q 0 && echo SSD || echo HDD)"; echo "") && \
  hyperfine \
    --sort command \
    --runs 1 \
    --export-json "$BASE_DIR/rdx-$(sed -E 's/(\w{8})\w+ ?/\1-/g;s/-$//'<<<"$COMMITS")-$STOP-$DBCACHE-$CC.json" \
    --parameter-list COMMIT ${COMMITS// /,} \
    --prepare "killall -9 bitcoind 2>/dev/null; rm -f ./build/bin/bitcoind; git clean -fxd; git reset --hard {COMMIT} && \
      cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release -DENABLE_IPC=OFF && ninja -C build bitcoind -j1 && \
      ./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=$STOP -dbcache=1000 -printtoconsole=0; sleep 20; rm -f $DATA_DIR/debug.log" \
    --conclude "killall bitcoind || true; sleep 5; grep -q 'height=0' $DATA_DIR/debug.log && grep -q 'Disabling script verification at block [#1](/bitcoin-bitcoin/1/)' $DATA_DIR/debug.log && grep -q 'height=$STOP' $DATA_DIR/debug.log && grep 'Bitcoin Core version' $DATA_DIR/debug.log | grep -q "$(printf %.12s {COMMIT})"; \
                cp $DATA_DIR/debug.log $LOG_DIR/debug-{COMMIT}-$(date +%s).log" \
    "COMPILER=$CC ./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=$STOP -dbcache=$DBCACHE -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0";
done

34a5ecadd7 Merge bitcoin/bitcoin#34397: doc: fix arg name hints so bugprone can validate them
86afa08070 doc: Add docstrings for ConnectBlock and SpendBlock

2026-01-31 | reindex-chainstate | 933339 blocks | dbcache 450 | i9-ssd | x86_64 | Intel(R) Core(TM) i9-9900K CPU @ 3.60GHz | 16 cores | 62Gi RAM | SSD

Benchmark 1: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=933339 -dbcache=450 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 34a5ecadd7203121b03ac692d22a752d2a364111)
  Time (abs ≡):        21914.825 s               [User: 50950.484 s, System: 3082.755 s]

Benchmark 2: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=933339 -dbcache=450 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 86afa0807063e88ee0b5c2c54c07504abccd3c18)
  Time (abs ≡):        21549.963 s               [User: 50055.298 s, System: 3004.309 s]

Relative speed comparison
        1.02          COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=933339 -dbcache=450 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 34a5ecadd7203121b03ac692d22a752d2a364111)
        1.00          COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=933339 -dbcache=450 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 86afa0807063e88ee0b5c2c54c07504abccd3c18)

34a5ecadd7 Merge bitcoin/bitcoin#34397: doc: fix arg name hints so bugprone can validate them
86afa08070 doc: Add docstrings for ConnectBlock and SpendBlock

2026-02-01 | reindex-chainstate | 933339 blocks | dbcache 4500 | i9-ssd | x86_64 | Intel(R) Core(TM) i9-9900K CPU @ 3.60GHz | 16 cores | 62Gi RAM | SSD

Benchmark 1: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=933339 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 34a5ecadd7203121b03ac692d22a752d2a364111)
  Time (abs ≡):        17411.825 s               [User: 34797.077 s, System: 972.271 s]

Benchmark 2: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=933339 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 86afa0807063e88ee0b5c2c54c07504abccd3c18)
  Time (abs ≡):        16935.349 s               [User: 34009.239 s, System: 932.712 s]

Relative speed comparison
        1.03          COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=933339 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 34a5ecadd7203121b03ac692d22a752d2a364111)
        1.00          COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=933339 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 86afa0807063e88ee0b5c2c54c07504abccd3c18)

</details>

for DBCACHE in 450 4500; do \                                                                                                                                                                                                                                                                                                      
  COMMITS="34a5ecadd7203121b03ac692d22a752d2a364111 86afa0807063e88ee0b5c2c54c07504abccd3c18"; \
  STOP=933339; CC=gcc; CXX=g++; \
  BASE_DIR="/mnt/my_storage"; DATA_DIR="$BASE_DIR/BitcoinData"; LOG_DIR="$BASE_DIR/logs"; \
  (echo ""; for c in $COMMITS; do git fetch -q origin $c && git log -1 --pretty='%h %s' $c || exit 1; done) && \
  (echo "" && echo "$(date -I) | reindex-chainstate | ${STOP} blocks | dbcache ${DBCACHE} | $(hostname) | $(uname -m) | $(lscpu | grep 'Model name' | head -1 | cut -d: -f2 | xargs) | $(nproc) cores | $(free -h | awk '/^Mem:/{print $2}') RAM | $(lsblk -no ROTA $(df --output=source $BASE_DIR | tail -1) | grep -q 0 && echo SSD || echo HDD)"; echo "") && \
  hyperfine \
    --sort command \
    --runs 1 \
    --export-json "$BASE_DIR/rdx-$(sed -E 's/(\w{8})\w+ ?/\1-/g;s/-$//'<<<"$COMMITS")-$STOP-$DBCACHE-$CC.json" \
    --parameter-list COMMIT ${COMMITS// /,} \
    --prepare "killall -9 bitcoind 2>/dev/null; rm -f ./build/bin/bitcoind; git clean -fxd; git reset --hard {COMMIT} && \
      cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release -DENABLE_IPC=OFF && ninja -C build bitcoind -j1 && \
      ./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=$STOP -dbcache=1000 -printtoconsole=0; sleep 20; rm -f $DATA_DIR/debug.log" \
    --conclude "killall bitcoind || true; sleep 5; grep -q 'height=0' $DATA_DIR/debug.log && grep -q 'Disabling script verification at block [#1](/bitcoin-bitcoin/1/)' $DATA_DIR/debug.log && grep -q 'height=$STOP' $DATA_DIR/debug.log && grep 'Bitcoin Core version' $DATA_DIR/debug.log | grep -q "$(printf %.12s {COMMIT})"; \
                cp $DATA_DIR/debug.log $LOG_DIR/debug-{COMMIT}-$(date +%s).log" \
    "COMPILER=$CC ./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=$STOP -dbcache=$DBCACHE -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0";
done

34a5ecadd7 Merge bitcoin/bitcoin#34397: doc: fix arg name hints so bugprone can validate them
86afa08070 doc: Add docstrings for ConnectBlock and SpendBlock

2026-01-29 | reindex-chainstate | 933339 blocks | dbcache 450 | i7-hdd | x86_64 | Intel(R) Core(TM) i7-7700 CPU @ 3.60GHz | 8 cores | 62Gi RAM | HDD

Benchmark 1: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=933339 -dbcache=450 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 34a5ecadd7203121b03ac692d22a752d2a364111)
  Time (abs ≡):        45844.795 s               [User: 51263.762 s, System: 3363.934 s]
 
Benchmark 2: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=933339 -dbcache=450 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 86afa0807063e88ee0b5c2c54c07504abccd3c18)
  Time (abs ≡):        47350.732 s               [User: 50522.961 s, System: 3296.509 s]
 
Relative speed comparison
        1.00          COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=933339 -dbcache=450 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 34a5ecadd7203121b03ac692d22a752d2a364111)
        1.03          COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=933339 -dbcache=450 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 86afa0807063e88ee0b5c2c54c07504abccd3c18)

34a5ecadd7 Merge bitcoin/bitcoin#34397: doc: fix arg name hints so bugprone can validate them
86afa08070 doc: Add docstrings for ConnectBlock and SpendBlock

2026-01-31 | reindex-chainstate | 933339 blocks | dbcache 4500 | i7-hdd | x86_64 | Intel(R) Core(TM) i7-7700 CPU @ 3.60GHz | 8 cores | 62Gi RAM | HDD

Benchmark 1: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=933339 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 34a5ecadd7203121b03ac692d22a752d2a364111)
  Time (abs ≡):        31980.960 s               [User: 35748.111 s, System: 1025.736 s]
 
Benchmark 2: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=933339 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 86afa0807063e88ee0b5c2c54c07504abccd3c18)
  Time (abs ≡):        32447.735 s               [User: 35240.043 s, System: 1026.939 s]
 
Relative speed comparison
        1.00          COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=933339 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 34a5ecadd7203121b03ac692d22a752d2a364111)
        1.01          COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=933339 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 86afa0807063e88ee0b5c2c54c07504abccd3c18)

</details>

DrahtBot added the label Needs rebase on Jan 30, 2026

sedited force-pushed on Jan 30, 2026

sedited commented at 8:44 AM on January 30, 2026: contributor

Rebased 00da3f04701789a6a238c3bee67e389396c01d6a -> 7802fd1f0d57bc20de0d71020c4c000baa65b217 (spendblock_17 -> spendblock_18, compare)

Fixed conflict with #34164

DrahtBot removed the label Needs rebase on Jan 30, 2026

in src/validation.cpp:2818 in 7802fd1f0d outdated

2813 | +
2814 | +    // verify that the view's current state corresponds to the previous block
2815 | +    uint256 hashPrevBlock = pindex->pprev == nullptr ? uint256() : pindex->pprev->GetBlockHash();
2816 | +    assert(hashPrevBlock == view.GetBestBlock());
2817 | +
2818 | +    const auto time_start{SteadyClock::now()};

w0xlt commented at 10:42 PM on February 17, 2026:

SpendBlock can run before CheckBlock (e.g. in ConnectTip, VerifyDB), so malformed blocks may hit blockundo.vtxundo.reserve(block.vtx.size() - 1) without prior validation.

    const auto time_start{SteadyClock::now()};

    // SpendBlock may run before CheckBlock() in some paths (e.g. VerifyDB).
    // Defensively reject structurally invalid blocks here to avoid size_t
    // underflow in blockundo reservation.
    if (block.vtx.empty())
        return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-blk-length", "size limits failed");

The test below can be use to validate this:

diff --git a/src/test/validation_chainstate_tests.cpp b/src/test/validation_chainstate_tests.cpp
index 9e2c710977..5448745bb7 100644
--- a/src/test/validation_chainstate_tests.cpp
+++ b/src/test/validation_chainstate_tests.cpp
@@ -12,6 +12,7 @@
 #include <test/util/coins.h>
 #include <test/util/random.h>
 #include <test/util/setup_common.h>
+#include <undo.h>
 #include <uint256.h>
 #include <util/check.h>
 #include <validation.h>
@@ -138,4 +139,33 @@ BOOST_FIXTURE_TEST_CASE(chainstate_update_tip, TestChain100Setup)
     BOOST_CHECK_EQUAL(curr_tip, get_notify_tip());
 }
 
+BOOST_FIXTURE_TEST_CASE(spendblock_rejects_empty_block, TestChain100Setup)
+{
+    Chainstate& chainstate = Assert(m_node.chainman)->ActiveChainstate();
+
+    LOCK(cs_main);
+    CBlockIndex* tip = Assert(chainstate.m_chain.Tip());
+
+    CBlock block;
+    // CheckBlock() rejects empty blocks, but SpendBlock can run before
+    // CheckBlock() in some paths (e.g. ConnectTip()).
+    block.hashPrevBlock = tip->GetBlockHash();
+
+    CBlockIndex index_dummy{block};
+    const uint256 block_hash{block.GetHash()};
+    index_dummy.pprev = tip;
+    index_dummy.nHeight = tip->nHeight + 1;
+    index_dummy.phashBlock = &block_hash;
+
+    CCoinsViewCache view_dummy(&chainstate.CoinsTip());
+    BlockValidationState state;
+    CBlockUndo blockundo;
+
+    bool spend_ok{true};
+    BOOST_CHECK_NO_THROW(spend_ok = chainstate.SpendBlock(block, &index_dummy, view_dummy, state, blockundo));
+    BOOST_CHECK(!spend_ok);
+    BOOST_CHECK(state.IsInvalid());
+    BOOST_CHECK_EQUAL(state.GetRejectReason(), "bad-blk-length");
+}

sedited commented at 10:36 AM on February 27, 2026:

Thanks for catching this! ~I wonder if the better approach would be to move the defensive CheckBlock call to SpendBlock and assert that fChecked is set in ConnectBlock. What do you think?~ EDIT: I'll stick to your suggestion, mine introduces some unexpected complexity with the unchecked states.

w0xlt commented at 8:31 PM on March 2, 2026:

It seems the empty block validation from https://github.com/sedited/bitcoin/compare/spendblock_19..spendblock_20 was removed.

sedited commented at 8:44 PM on March 2, 2026:

Yes, I kept the test, but chose to move CheckBlock to the new SpendBlock after all. The reason why I think this one-off length check is probably not what we want to do, is that CheckBlock would normally first compute the merkle root of the block. If it fails to do that, for example through a missing transaction, we currently shutdown and don't mark the block as permanently invalid. This additional length check might shadow that condition.

sedited force-pushed on Feb 27, 2026

sedited commented at 9:59 AM on February 27, 2026: contributor

Rebased 7802fd1f0d57bc20de0d71020c4c000baa65b217 -> 4f92e4e9d3d55ecb51795e9992aaf50be4feb4db (spendblock_18 -> spendblock_19, compare)

Fixed a silent merge conflict.

sedited force-pushed on Feb 28, 2026

sedited commented at 9:59 AM on February 28, 2026: contributor

Updated 4f92e4e9d3d55ecb51795e9992aaf50be4feb4db -> 8140cba28afbeadf67b2db813f50267101c14b5e (spendblock_19 -> spendblock_20, compare)

Addressed @w0xlt's comment, added a validation step checking that a block is not empty in the introduced SpendBlock() function.

ajtowns commented at 4:40 PM on February 28, 2026: contributor

Concept NACK for whatever it's worth. This seems like an extremely high risk change for very little benefit, at least for this project where we do have the UTXO set available. See previous comments at #32317 (comment) and I guess #32317 (review) for an example of the riskiness.

sedited force-pushed on Mar 2, 2026

in src/validation.cpp:2295 in 4fe1ccd49a

2299 |      AssertLockHeld(cs_main);
2300 |      assert(pindex);
2301 | -
2302 | -    uint256 block_hash{block.GetHash()};
2303 | -    assert(*pindex->phashBlock == block_hash);
2304 | +    Assume(pindex->GetBlockHash() == block.GetHash());

w0xlt commented at 7:54 PM on March 2, 2026:

Why was it changed from assert to Assume ? With this, block/pindex mismatch will silently proceed through consensus-critical validation.

sedited commented at 8:12 PM on March 2, 2026:

I reverted it.

sedited force-pushed on Mar 2, 2026

DrahtBot added the label CI failed on Mar 2, 2026

in src/validation.cpp:2771 in a27e01a32b

2766 | +    CBlockUndo& blockundo,
2767 | +    bool fJustCheck)
2768 | +{
2769 | +    AssertLockHeld(cs_main);
2770 | +    assert(pindex);
2771 | +    Assume(pindex->GetBlockHash() == block.GetHash());

w0xlt commented at 8:23 PM on March 2, 2026:

Same as #32317 (review)

sedited commented at 8:25 PM on March 2, 2026:

Fixed.

sedited force-pushed on Mar 2, 2026

sedited commented at 8:41 PM on March 2, 2026: contributor

8140cba28afbeadf67b2db813f50267101c14b5e -> 94f85290f3b999e5ba9a2a2aaf5480da8639fd77 (spendblock_20 -> spendblock_21, compare)

Went with my original suggestion of moving ConnectBlock's call to CheckBlock to the new SpendBlock function after all.

Re #32317 (comment)

This seems like an extremely high risk change for very little benefit, at least for this project where we do have the UTXO set available. See previous comments at #32317 (comment) and I guess #32317 (review) for an example of the riskiness.

Ok, I have to admit that I thought other projects would have picked this up by now, or at least experiment with the approach a bit, but that has not materialized so far. The immediate benefits to this project as is are moderate and motivating a significant change to consensus with a small improvement on IBD speed has landed the project in trouble before.

However I'm still convinced that this is the architecturally sounder approach and makes things easier to reason about. That said, I would like to try out the alternative of exposing some form of coins cache wrapping instead to facilitate a fair comparison, but will wait a few days more to see what other reviewers think. Recent changes in other PRs working with the coins view cache and implementing similar facades through it (i.e. the recently introduced CCoinsViewOverlay) show that this might be a more palatable change.

As for the way we previously called CheckBlock in ConnectBlock, it does not bode well that a bunch of reviewers did not catch this over a few months of review. The whole logic is confusing and horrible to follow. The boolean fChecked traps make it hard to ascertain what is actually being checked and for which purpose. Making a failure to compute the Merkle root a shutdown, but treating others as consensus failures seems a bit inconsistent too. After all, failing to compute the Merkle root might not be related to a disk failure, but a similar "bad block" that the rest of CheckBlock's logic should be guarding against.

DrahtBot removed the label CI failed on Mar 2, 2026

ajtowns commented at 8:08 PM on March 7, 2026: contributor

However I'm still convinced that this is the architecturally sounder approach and makes things easier to reason about.

I'd be interested in reviewing an approach at doing this (replacing CoinsView with a span<Coins>) just for tx (package?) validation but not for block validation, fwiw. I think that might help with disentangling the complexities of the mempool from the kernel while minimising changes/risk related to block validation.

in src/coins.h:443 in e99902f4e3

 438 | +     * found.
 439 | +     *
 440 | +     * Generally, this should only be held for a short scope. The coins should
 441 | +     * generally not be held through any other calls to this cache.
 442 | +     */
 443 | +    std::vector<std::reference_wrapper<const Coin>> AccessCoins(const CTransaction& tx) const;

theuni commented at 5:45 PM on March 13, 2026:

This feels quite brittle and I could see it causing bugs in the future.

Would it be possible to implement as a callback instead, so that the short scope is forced?

Something like:

template <typename Callable>
auto AccessCoins(const CTransaction& tx, const Callable& callback) const
{
    std::vector<std::reference_wrapper<const Coin>> coins;
    coins.reserve(tx.vin.size());
    for (const CTxIn& input : tx.vin) {
        coins.emplace_back(AccessCoin(input.prevout));
    }
    return callback(std::span{coins}));
}

And used like:

auto sigop_count = AccessCoins(tx, [&tx](auto&& coins) {
    return GetP2SHSigOpCount(tx, coins);
});

sedited commented at 9:28 PM on March 17, 2026:

I implemented this over here: spendblock_22_lambda - compare. I was thinking about such an approach before, but did not do it, because it wastes an extra vector allocation in the mempool transaction verification path. Looking at the diff, the changes in actual production code are pretty small, so not sure if this is really needed. Do you think the change is worthwhile?

theuni commented at 11:34 PM on March 17, 2026:

I do think it's pretty footgunny to return a span of references with a mystery lifetime, and just hope that no one ever misuses it. Unless I'm misunderstanding, the callback approach prevents that entirely.

Seems AreInputsStandard and IsWitnessStandard (and even fSpendsCoinbase) could be changed to take spans instead? Then you could do all of those checks in one callback without having to go back to the CCoinsViewCache at all. That seems like a win all around?

ajtowns commented at 1:43 AM on March 18, 2026:

FWIW, I've been thinking about an RCU approach to utxo access (so shared-readers with lock-free access, single writer controlled by mutex, and readers don't block the writer). In that approach you take a shared_ptr to the utxo state, and that state persists until you release that shared_ptr. Here's my claude brainstorming fwiw.

theuni commented at 9:40 PM on March 18, 2026:

@ajtowns that's neat :)

I'd be curious to see what IBD performance looked like doing (speculative) block validation in batches with no io per-batch.

But, for the sake of this PR, I think we should separate the concerns: data retrieval/availability, and what we could actually do with better availability.

(incoming thought dump after poking around here today. Not fully fleshed out)

tl;dr: I think it's worth experimenting with having the db pass shared pointers to coins and letting CCoinsViewCache store them that way. That way we can pass them out with no lifetime concerns, and use the data elsewhere.

This PR purports to "separate UTXO set access from validation functions", though in some cases (mempool) it just hides the accesses via references to cached entries.

I think there's another way that fully severs the db (cache) dependency for data availability: have CCoinsViewCache store a std::shared_ptr<const Coin> rather than just a bare Coin. Then AccessCoins can return a std::vector<std::shared_ptr<const Coin>>. That way there are no lifetime concerns.

I believe that would make life easier for @sedited here, and for you too @ajtowns, as it untangles availability vs what's active.

Imo we want new types: ContextualTransaction and ContextualBlock. SpendBlock() would take a CBlock as input, do the lookups, transform each CTransaction into a corresponding ContextualTransaction, with shared_ptr<Coin>s added to the inputs. Then a ContextualBlock is created from those. SpendBlock now creates a ContextualBlock rather than the awkward CBlock and CBlockUndo and passes it to ConnectBlock.

I realize I'm getting ahead of myself there, but it seems inevitable that we'll eventually want to be able to pass blocks with pre-filled input data to/from the kernel. With that assumption in mind, I'd rather just go ahead and commit to that than abuse CBlockUndo.

theuni commented at 6:03 PM on March 13, 2026: member

Concept ACK.

I agree that this is scary and requires a very comprehensive review (which I have not yet done here), but I think it's worth the effort. Being able to bring your own coins to the core validation functionality would make things much easier to reason about, and is (imo) how it should've been implemented all along.

yancyribbens commented at 4:32 PM on March 19, 2026: contributor

from #32317 (comment)

first, is that the primary client of the bitcoin core kernel is the bitcoin core node software

Is this actually true though given the bitcoinkernel project? If other software can now import the bitcoinkernel, then there can be a multitude of other node projects interacting with the UTXO set. How might other node projects then interact with the UTXO set? One possibility is to just parse the blk files once they are written to disk, although this seems sub-optimal if it's only used as an intermediate step.

DrahtBot added the label Needs rebase on Mar 19, 2026

ajtowns commented at 5:33 AM on March 23, 2026: contributor

Is this actually true though given the bitcoinkernel project?

If it's not true and the bitcoinkernel project is going to make it harder to maintain the node software, that seems like a big problem. :(

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.

7c9ecca92e

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.

a3d0138286

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.

89e18e2dd1

validation: Consolidate AccessCoins calls in PreChecks

This should introduce no behaviour change, but avoids a few extra vector
allocations and coins lookups

3b0523e173

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.

ae63ca4733

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.

e39f8015d5

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.

00bf059f67

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.

Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>

68c79f0e37

doc: Add docstrings for ConnectBlock and SpendBlock c361e1283c

sedited force-pushed on Mar 24, 2026

sedited commented at 5:21 PM on March 24, 2026: contributor

Thanks for taking a look @theuni,

Rebased and updated 94f85290f3b999e5ba9a2a2aaf5480da8639fd77 -> c361e1283c068b0e78837695d794ed3612499870 (spendblock_21 -> spendblock_22, compare)

Took @theuni's suggestion, AccessCoins now takes a callback that it passes its retrieved coins to, which prevents any unwanted lifetime extensions.

DrahtBot removed the label Needs rebase on Mar 24, 2026

in src/validation.cpp:3100 in c361e1283c

3094 | @@ -3033,7 +3095,17 @@ bool Chainstate::ConnectTip(
3095 |      {
3096 |          CCoinsViewCache& view{*m_coins_views->m_connect_block_view};
3097 |          const auto reset_guard{view.CreateResetGuard()};
3098 | -        bool rv = ConnectBlock(*block_to_connect, state, pindexNew, view);
3099 | +        CBlockUndo blockundo;
3100 | +        if (!SpendBlock(*block_to_connect, pindexNew, view, state, blockundo)) {
3101 | +            assert(state.IsInvalid());

w0xlt commented at 6:50 PM on April 12, 2026:

The current code assumes that SpendBlock can only fail when state.IsInvalid() is true. However, in the BLOCK_MUTATED path, SpendBlock calls FatalError(), which sets the state to IsError() instead of IsInvalid(). As a result, this assertion can cause the node to crash.

diff --git a/src/validation.cpp b/src/validation.cpp
index be2fb8687d..8e1d07b74b 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
+++ b/src/validation.cpp
@@ -3097,11 +3097,12 @@ bool Chainstate::ConnectTip(
         const auto reset_guard{view.CreateResetGuard()};
         CBlockUndo blockundo;
         if (!SpendBlock(*block_to_connect, pindexNew, view, state, blockundo)) {
-            assert(state.IsInvalid());
             if (m_chainman.m_options.signals) {
                 m_chainman.m_options.signals->BlockChecked(block_to_connect, state);
             }
-            InvalidBlockFound(pindexNew, state);
+            if (state.IsInvalid()) {
+                InvalidBlockFound(pindexNew, state);
+            }
             LogError("%s: SpendBlock %s failed, %s\n", __func__, pindexNew->GetBlockHash().ToString(), state.ToString());
             return false;
         }

Suggested test:

--- a/src/test/validation_chainstate_tests.cpp
+++ b/src/test/validation_chainstate_tests.cpp
@@ -4,8 +4,10 @@
 //
 #include <chainparams.h>
 #include <consensus/amount.h>
+#include <consensus/merkle.h>
 #include <consensus/validation.h>
 #include <node/kernel_notifications.h>
+#include <pow.h>
 #include <random.h>
 #include <rpc/blockchain.h>
 #include <script/script.h>
@@ -192,4 +194,39 @@ BOOST_FIXTURE_TEST_CASE(spendblock_rejects_empty_block, TestChain100Setup)
     BOOST_CHECK_EQUAL(state.GetRejectReason(), "high-hash");
 }
 
+BOOST_FIXTURE_TEST_CASE(activate_best_chain_does_not_invalidate_mutated_spendblock_failure, TestChain100Setup)
+{
+    Chainstate& chainstate = Assert(m_node.chainman)->ActiveChainstate();
+
+    auto block = std::make_shared<CBlock>(CreateBlock({}, CScript{} << OP_TRUE, chainstate));
+    block->hashMerkleRoot = uint256{1};
+    BOOST_REQUIRE(block->hashMerkleRoot != BlockMerkleRoot(*block));
+    block->nNonce = 0;
+    while (!CheckProofOfWork(block->GetHash(), block->nBits, Params().GetConsensus())) ++block->nNonce;
+
+    CBlockIndex index_dummy{*block};
+    const uint256 block_hash{block->GetHash()};
+
+    {
+        LOCK(cs_main);
+        CBlockIndex* tip = Assert(chainstate.m_chain.Tip());
+        index_dummy.pprev = tip;
+        index_dummy.nHeight = tip->nHeight + 1;
+        index_dummy.phashBlock = &block_hash;
+        index_dummy.nChainWork = tip->nChainWork + GetBlockProof(index_dummy);
+        index_dummy.nTx = block->vtx.size();
+        index_dummy.m_chain_tx_count = tip->m_chain_tx_count + index_dummy.nTx;
+        index_dummy.nStatus = BlockStatus(BLOCK_VALID_TRANSACTIONS | BLOCK_HAVE_DATA);
+        chainstate.setBlockIndexCandidates.insert(&index_dummy);
+    }
+
+    BlockValidationState state;
+    BOOST_CHECK(!chainstate.ActivateBestChain(state, block));
+    BOOST_CHECK(state.IsError());
+    BOOST_CHECK_EQUAL(state.GetResult(), BlockValidationResult::BLOCK_MUTATED);
+    BOOST_CHECK(!(index_dummy.nStatus & BLOCK_FAILED_VALID));
+
+    WITH_LOCK(cs_main, chainstate.setBlockIndexCandidates.erase(&index_dummy));
+}
+

</details>

DrahtBot added the label Needs rebase on Apr 13, 2026

DrahtBot commented at 2:51 PM on April 13, 2026: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

ismaelsadeeq commented at 4:07 PM on April 20, 2026: member

After the recent conversation here between @sedited, @theuni, and @ajtowns, I've done further work to evaluate the motivation of this PR and wanted to share the work and my conclusions. See the work doc

From the discussion above, I think there are two motivations.

Goal (i) is enabling clients to supply their own coins for block validation, i.e. the immediate need for kernel API users.
Goal (ii) is decoupling the validation functions, improving the coins view cache, and rearchitecting the block validation flow, i.e. the broader architectural improvement.

These goals overlap but are not in conflict, and critically, goal (i) does not require goal (ii) as a prerequisite. A kernel API could simply accept a chain tip, a block, and the coins that block consumed, and validate against those inputs directly; the internal implementation is an orthogonal concern.

I implemented two variants of block validation with caller-supplied coins.

TestBlockValidityWithSpentCoins, using the coins view cache approach suggested by @ajtowns previously. I did not use the proposed BlockUndo wrapper to CCoinsView as I find it similar in effect to simply having the caller pass a list of coins that are appended to the view, which is what this approach does: https://github.com/ismaelsadeeq/bitcoin/tree/04-2026-test-block-validity-spentouts-rebased.
TestBlockValidityWithSpentCoinsUndo, building on this PR's approach: https://github.com/ismaelsadeeq/bitcoin/tree/pr-32317.

Both variants have been exposed through the Bitcoin kernel C header API, making it technically feasible for client projects such as SwiftSync or Utreexo to call either path. Note that headers must be synced first; the precondition is documented in the API. I also opened #35000 with unit tests for block validation that are useful independently of the outcome of this PR.

I now think the two goals motivating this PR should be pursued separately. Goal (i) does not require the architectural refactor in this PR as a prerequisite; the coins view approach achieves this trivially, as is evident in the POC above. Goal (ii) is valuable and worth pursuing, but should be done incrementally with rigorous fuzz and unit tests at each step, as raised by @ajtowns and @theuni and in light of the recent bugs found by @w0xlt.

I am inclined to suggest that the refactoring work in goal (ii) proceed methodically once we have expanded the tests in #35000 to cover not just TestBlockValidity but test/simulate other validation paths, so that we can catch bugs early. Something like #34895 should similarly be expanded to cover block validity and other block validation paths beyond just ConnectBlock see https://github.com/ismaelsadeeq/bitcoin/tree/fixup-connect-block-harness that expanded it to test block validity and testblockvalidity with spent outputs.

As a concrete demonstration that testing can de-risk this work: the bug caught by @w0xlt at #32317 (review) was reproduced by the unit and fuzz tests after I substituted the vulnerable commit into https://github.com/ismaelsadeeq/bitcoin/tree/pr-32317. I expect the same approach would catch #32317 (review) and any other hidden issues (if they exist), as long as we simulate all the block validation paths with their specific quirks and feed them sufficiently adversarial blocks.