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.
#31132 (validation: fetch block inputs on parallel threads 3x faster IBD by andrewtoth)
#29843 (policy: Allow non-standard scripts with -acceptnonstdtxn=1 (test nets only) by ajtowns)
#29060 (Policy: Report debug message why 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.
LLM Linter (✨ experimental)
Possible typos and grammar issues:
“…(amounts and maturity) This does not modify the UTXO set.” -> “…(amounts and maturity). This does not modify the UTXO set.” [Missing sentence separator (period) after the parenthesis makes the sentence run-on and harms readability.]
“Return a vector of unspent outputs of coins in the cache that are spent by the provided transaction.” -> “Return a vector of unspent outputs (CTxOut) from the cache that are being spent by the provided transaction.” [The original phrasing is internally contradictory/unclear (“unspent outputs … that are spent”); rewording clarifies that these are the outputs in the cache which the transaction spends.]
“@param[in] blockundo Has to contain all coins spent by block. Written to disk on successful validation” -> “@param[in] blockundo Must contain all coins spent by the block and is written to disk on successful validation.” [Sentence fragment and missing auxiliary verb; the original is grammatically fragmented and slightly ambiguous.]
2025-12-03
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 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.
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.
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.
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
76a8f22c5coutdated
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
76a8f22c5coutdated
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.
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?
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
nit: for non-trivial functions, I think it helps to narrow down T instead of having to deduct it from its callsites and usage.
What do you think about adding an IsCoinRef concept, and adding std::span to the function signature?
0diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
1index 97f8241089..ab27387cc4 100644
2--- a/src/consensus/tx_verify.cpp
3+++ b/src/consensus/tx_verify.cpp
4@@ -14,6 +14,8 @@
5 #include <util/check.h>
6 #include <util/moneystr.h>
7 8+#include <span>
9+
10 template <typename T>
11 static constexpr const Coin& GetCoin(const T& item)
12 {
13@@ -158,8 +160,8 @@ template unsigned int GetP2SHSigOpCount<std::span<const Coin>>(
14 template unsigned int GetP2SHSigOpCount<std::span<std::reference_wrapper<const Coin>>>(
15 const CTransaction& tx, const std::span<std::reference_wrapper<const Coin>>);
1617-template <typename T>
18-int64_t GetTransactionSigOpCost(const CTransaction& tx, const T coins, uint32_t flags)
19+template<IsCoinRef CoinRef>
20+int64_t GetTransactionSigOpCost(const CTransaction& tx, std::span<CoinRef> coins, uint32_t flags)
21 {
22 int64_t nSigOps = GetLegacySigOpCount(tx) * WITNESS_SCALE_FACTOR;
2324@@ -172,18 +174,18 @@ int64_t GetTransactionSigOpCost(const CTransaction& tx, const T coins, uint32_t
2526 Assert(coins.size() == tx.vin.size());
27 auto input_it = tx.vin.begin();
28- for (auto it = coins.begin(); it != coins.end(); ++it, ++input_it) {
29- const Coin& coin{GetCoin(*it)};
30+ for (const Coin& coin : coins) {
31 assert(!coin.IsSpent());
32 const CTxOut &prevout = coin.out;
33 nSigOps += CountWitnessSigOps(input_it->scriptSig, prevout.scriptPubKey, &input_it->scriptWitness, flags);
34+ ++input_it;
35 }
36 return nSigOps;
37 }
38-template int64_t GetTransactionSigOpCost<std::span<const Coin>>(
39+template int64_t GetTransactionSigOpCost<const Coin>(
40 const CTransaction& tx, std::span<const Coin> coins, uint32_t flags);
4142-template int64_t GetTransactionSigOpCost<std::span<std::reference_wrapper<const Coin>>>(
43+template int64_t GetTransactionSigOpCost<std::reference_wrapper<const Coin>>(
44 const CTransaction& tx, const std::span<std::reference_wrapper<const Coin>> coins, uint32_t flags);
4546 template <typename T>
47diff --git a/src/consensus/tx_verify.h b/src/consensus/tx_verify.h
48index 88b2d6164b..b8ab114306 100644
49--- a/src/consensus/tx_verify.h
50+++ b/src/consensus/tx_verify.h
51@@ -48,6 +48,8 @@ unsigned int GetLegacySigOpCount(const CTransaction& tx);
52 template <typename T>
53 unsigned int GetP2SHSigOpCount(const CTransaction& tx, const T coins);
5455+template<typename T>
56+concept IsCoinRef = std::convertible_to<T, const Coin&>;
57 /**
58 * Compute total signature operation cost of a transaction.
59 * [@param](/bitcoin-bitcoin/contributor/param/)[in] tx Transaction for which we are computing the cost
60@@ -55,8 +57,8 @@ unsigned int GetP2SHSigOpCount(const CTransaction& tx, const T coins);
61 * [@param](/bitcoin-bitcoin/contributor/param/)[in] flags Script verification flags
62 * [@return](/bitcoin-bitcoin/contributor/return/) Total signature operation cost of tx
63 */
64-template <typename T>
65-int64_t GetTransactionSigOpCost(const CTransaction& tx, const T coins, uint32_t flags);
66+template<IsCoinRef CoinRef>
67+int64_t GetTransactionSigOpCost(const CTransaction& tx, std::span<CoinRef> coins, uint32_t flags);
6869 /**
70 * Check if transaction is final and can be included in a block with the
(the same applies for GetLegacySigOpCount and GetP2SHSigOpCount)
edit: I see this is already (partially) suggested and addressed in #32317 (review), I think adding the IsCoinRef concept could still be nice but less important than specifying the std::span
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.
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.
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.
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.
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
a7e4132623outdated
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);
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.
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
76a8f22c5coutdated
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.
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?
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
This is UB if e.g. blockundo is not populated, would add a size assertion at the beginning:
0diff --git a/src/validation.cpp b/src/validation.cpp
1index adec151e14..20e1dc6482 100644
2--- a/src/validation.cpp
3+++ b/src/validation.cpp
4@@ -2446,6 +2446,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, const uint256& block_hash, co
5 assert(pindex);
6 Assume(block.GetHash() == block_hash);
7 assert(*pindex->phashBlock == block_hash);
8+ assert(block.vtx.size() == blockundo.vtxundo.size() + 1); // blockundo does not contain a CTxUndo for the coinbase
9 const bool parallel_script_checks{m_chainman.GetCheckQueue().HasThreads()};
1011 const auto time_start{SteadyClock::now()};
in
src/validation.cpp:2440
in
16a695fbffoutdated
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()
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
16a695fbffoutdated
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
0 * [@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
16a695fbffoutdated
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
nit: grammar change to highlight that pindex can be mutated here
0 * [@param](/bitcoin-bitcoin/contributor/param/)[out] pindex Points to the block map entry associated with block. On successful validation, its nStatus block validity is raised
in
src/test/txvalidationcache_tests.cpp:128
in
16a695fbffoutdated
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.
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?
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?
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 0:04 am on May 14, 2025:
contributor
PSA: Bitcoin Core PR Review Club will cover this PR in its next meeting at 2025-05-14 at 17:00 UTC. See https://bitcoincore.reviews/32317 for notes, questions, and instructions on how to join.
sedited force-pushed
on May 15, 2025
sedited
commented at 1:34 pm on May 15, 2025:
contributor
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
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.
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.
sedited force-pushed
on Jul 9, 2025
sedited
commented at 1:37 pm on July 9, 2025:
contributor
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.
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.
sedited force-pushed
on Jul 24, 2025
sedited
commented at 10:32 am on July 24, 2025:
contributor
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).
(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):
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
DrahtBot removed the label
Needs rebase
on Nov 12, 2025
DrahtBot added the label
Needs rebase
on Dec 2, 2025
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.
5709d073e0
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.
3bdb4a89a9
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.
162f2668be
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.
f8186cfaf6
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.
c937b12745
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.
d9f1fc7ab3
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.
c2a5497e99
doc: Add docstrings for ConnectBlock and SpendBlocka0e9578ec1
sedited force-pushed
on Dec 3, 2025
sedited
commented at 10:29 am on December 3, 2025:
contributor
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)
5657 /**
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
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
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.
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.
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
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2026-01-07 00:13 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
