kernel: Separate UTXO set access from validation functions #32317

TheCharlatan · 2025-04-21T13:15:46Z

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.

DrahtBot · 2025-04-21T13:15:49Z

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

Code Coverage & Benchmarks

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

Reviews

See the guideline for information on the review process.

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

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32998 (Bump SCRIPT_VERIFY flags to 64 bit by ajtowns)
#32840 (test: Enhance GetTxSigOpCost tests for coinbase transactions by average-gary)
#32729 (test,refactor: extract script template helpers & widen sigop count coverage by l0rinc)
#32575 (refactor: Split multithreaded case out of CheckInputScripts by fjahr)
#32468 (rpc: generateblock to allow multiple outputs by polespinasa)
#31974 (Drop testnet3 by Sjors)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
#29060 (Policy: Report reason inputs are non standard by ismaelsadeeq)

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.

src/validation.h

DrahtBot · 2025-04-21T14:43:48Z

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

Hints

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.

src/validation.h

w0xlt

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.

TheCharlatan · 2025-05-07T10:30:53Z

Re #32317 (review)

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.

src/consensus/tx_verify.h

stringintech · 2025-05-10T10:51:13Z

src/validation.cpp

 bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
-                       const CCoinsViewCache& inputs, unsigned int flags, bool cacheSigStore,
+                       std::vector<CTxOut>&& spent_outputs, unsigned int flags, bool cacheSigStore,
                       bool cacheFullScriptStore, PrecomputedTransactionData& txdata,


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?

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.

Oh, it makes sense. Thanks for the insights.

TheCharlatan · 2025-05-10T16:21:25Z

Updated 76a8f22 -> 16a695f (spendblock_0 -> spendblock_1, compare)

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

stickies-v

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.

src/validation.h

src/consensus/tx_verify.cpp

stickies-v · 2025-05-07T15:17:42Z

src/coins.cpp

+    for (const auto& txin : tx.vin) {
+        const COutPoint& prevout = txin.prevout;
+        const Coin& coin = AccessCoin(prevout);
+        assert(!coin.IsSpent());
+        spent_outputs.emplace_back(coin.out);
+    }


I think you can use AccessCoins here?

Suggested change

for (const auto& txin : tx.vin) {

const COutPoint& prevout = txin.prevout;

const Coin& coin = AccessCoin(prevout);

assert(!coin.IsSpent());

spent_outputs.emplace_back(coin.out);

}

for (const Coin& coin : AccessCoins(tx)) {

assert(!coin.IsSpent());

spent_outputs.emplace_back(coin.out);

}

nit: inconsistent naming between GetUnspentOutputs and spent_outputs.

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.

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); }); }

src/validation.cpp

src/validation.h

stickies-v · 2025-05-13T15:39:14Z

src/test/txvalidationcache_tests.cpp

+    std::vector<CTxOut> spent_outputs{active_coins_tip.GetUnspentOutputs(tx)};
+    txdata.Init(tx, std::move(spent_outputs));


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.

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.

stringintech

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

src/validation.cpp

src/consensus/tx_verify.cpp

src/validation.cpp

stickies-v · 2025-05-14T00:04:16Z

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.

TheCharlatan · 2025-05-15T13:34:38Z

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

16a695f -> e8ac6d2 (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.

TheCharlatan · 2025-07-09T13:37:39Z

Rebased 6e90a0e -> 1f96d1c (spendblock_7 -> spendblock_8, compare)

Fixed silent merge conflict with test: check P2SH sigop count for coinbase tx #32850

DrahtBot · 2025-07-22T11:29:55Z

🚧 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.}

Hints

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.

TheCharlatan · 2025-07-24T10:32:24Z

Updated 1f96d1c -> 10c3a06 (spendblock_8 -> spendblock_9, compare)

Fixed silent merge conflict with policy: make pathological transactions packed with legacy sigops non-standard #32521

TheCharlatan · 2025-08-19T11:45:49Z

Rebased 10c3a06 -> 3843a15 (spendblock_9 -> spendblock_10, compare)

Fixed merge conflict with validation: detect witness stripping without re-running Script checks #33105

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.

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.

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.

…InputScripts 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.

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.

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.

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.

TheCharlatan · 2025-08-21T14:49:29Z

Rebased 3843a15 -> 5537ac0 (spendblock_10 -> spendblock_11, compare)

Fixed conflict with kernel: improve BlockChecked ownership semantics #33078

DrahtBot added the Validation label Apr 21, 2025

TheCharlatan changed the title ~~kernel: Seperate UTXO set access from validation functions~~ kernel: Separate UTXO set access from validation functions Apr 21, 2025

Sjors reviewed Apr 21, 2025

View reviewed changes

src/validation.h Outdated Show resolved Hide resolved

DrahtBot added the CI failed label Apr 21, 2025

TheCharlatan force-pushed the spendblock branch from 7492904 to c1b36c4 Compare April 21, 2025 16:14

darosior reviewed Apr 21, 2025

View reviewed changes

src/validation.h Outdated Show resolved Hide resolved

TheCharlatan force-pushed the spendblock branch from c1b36c4 to 76a8f22 Compare April 21, 2025 19:50

DrahtBot removed the CI failed label Apr 21, 2025

TheCharlatan moved this to Ready for Review PRs in The libbitcoinkernel Project Apr 22, 2025

TheCharlatan added this to The libbitcoinkernel Project Apr 22, 2025

w0xlt reviewed Apr 22, 2025

View reviewed changes

DrahtBot mentioned this pull request Apr 30, 2025

p2p: stop special-casing witness-stripped error for unconfirmed transactions #32379

Draft

stringintech reviewed May 7, 2025

View reviewed changes

src/consensus/tx_verify.h Outdated Show resolved Hide resolved

stringintech reviewed May 10, 2025

View reviewed changes

TheCharlatan force-pushed the spendblock branch from 76a8f22 to 16a695f Compare May 10, 2025 16:20

stickies-v reviewed May 13, 2025

View reviewed changes

stringintech reviewed May 13, 2025

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/consensus/tx_verify.cpp Show resolved Hide resolved

src/validation.cpp Show resolved Hide resolved

TheCharlatan force-pushed the spendblock branch from 16a695f to e8ac6d2 Compare May 15, 2025 13:25

DrahtBot removed the CI failed label Jul 9, 2025

This was referenced Jul 10, 2025

Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts #32473

Merged

assumevalid: log every script validation state change #32975

Merged

DrahtBot mentioned this pull request Jul 17, 2025

Bump SCRIPT_VERIFY flags to 64 bit #32998

Open

DrahtBot added the CI failed label Jul 22, 2025

TheCharlatan force-pushed the spendblock branch from 1f96d1c to 10c3a06 Compare July 24, 2025 10:32

DrahtBot removed the CI failed label Jul 24, 2025

This was referenced Jul 28, 2025

kernel: improve BlockChecked ownership semantics #33078

Merged

validation: detect witness stripping without re-running Script checks #33105

Merged

kernel, refactor: return error status on all fatal errors #29700

Draft

DrahtBot mentioned this pull request Aug 5, 2025

test,refactor: extract script template helpers & widen sigop count coverage #32729

Open

DrahtBot added the Needs rebase label Aug 8, 2025

TheCharlatan force-pushed the spendblock branch from 10c3a06 to 3843a15 Compare August 19, 2025 11:45

DrahtBot added Needs rebase and removed Needs rebase labels Aug 19, 2025

TheCharlatan added 8 commits August 20, 2025 20:33

validation: Move SetBestBlock out of ConnectBlock

589a1c8

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.

doc: Add docstrings for ConnectBlock and SpendBlock

5537ac0

TheCharlatan force-pushed the spendblock branch from 3843a15 to 5537ac0 Compare August 21, 2025 14:49

DrahtBot removed the Needs rebase label Aug 21, 2025

		std::vector<CTxOut> spent_outputs{active_coins_tip.GetUnspentOutputs(tx)};
		txdata.Init(tx, std::move(spent_outputs));

kernel: Separate UTXO set access from validation functions #32317

Are you sure you want to change the base?

kernel: Separate UTXO set access from validation functions #32317

Conversation

TheCharlatan commented Apr 21, 2025

Uh oh!

DrahtBot commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

Uh oh!

DrahtBot commented Apr 21, 2025

Uh oh!

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

TheCharlatan commented May 7, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheCharlatan commented May 10, 2025

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stringintech left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stickies-v commented May 14, 2025

Uh oh!

TheCharlatan commented May 15, 2025

Uh oh!

TheCharlatan commented Jul 9, 2025

Uh oh!

DrahtBot commented Jul 22, 2025

Uh oh!

TheCharlatan commented Jul 24, 2025

Uh oh!

TheCharlatan commented Aug 19, 2025

Uh oh!

TheCharlatan commented Aug 21, 2025

Uh oh!

Uh oh!

DrahtBot commented Apr 21, 2025 •

edited

Loading

stringintech left a comment •

edited

Loading