Repository navigation
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32317. ReviewsSee the guideline and AI policy for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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.
2026-06-18 16:13:38 |
sedited
changed the title
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
w0xlt
left a comment
w0xlt
left a comment
There was a problem hiding this comment.
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.
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. |
| 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, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh, it makes sense. Thanks for the insights.
|
Updated 76a8f22 -> 16a695f (spendblock_0 -> spendblock_1, compare)
|
stickies-v
left a comment
stickies-v
left a comment
There was a problem hiding this comment.
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.
| 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); | ||
| } |
There was a problem hiding this comment.
I think you can use AccessCoins here?
| 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); | |
| } |
There was a problem hiding this comment.
nit: inconsistent naming between GetUnspentOutputs and spent_outputs.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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);
});
}
There was a problem hiding this comment.
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?
| std::vector<CTxOut> spent_outputs{active_coins_tip.GetUnspentOutputs(tx)}; | ||
| txdata.Init(tx, std::move(spent_outputs)); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
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. |
|
Thank you for the reviews @stringintech and @stickies-v! 16a695f -> e8ac6d2 (spendblock_1 -> spendblock_2, compare)
|
|
Rebased edc952b -> f047668 (spendblock_24 -> spendblock_25, compare) |
|
Rebased f047668 -> 8217d10 (spendblock_25 -> spendblock_26, compare) |
| LOCK(cs_main); | ||
| CBlockIndex* tip = Assert(chainstate.m_chain.Tip()); | ||
|
|
||
| CBlock block; |
There was a problem hiding this comment.
The high-hash error means the block failed proof-of-work in CheckBlockHeader() before CheckBlock() reached the empty-block size check.
So this test is not actually proving that SpendBlock() handles empty blocks safely. It only proves that SpendBlock() calls CheckBlock() and rejects a header with invalid PoW.
That matters because the original risk was an empty block reaching later SpendBlock() logic and causing block.vtx.size() - 1 underflow. If the test fails earlier with high-hash, it would still pass even if the empty-block size check path were broken or unreachable.
Suggested change:
diff
diff --git a/src/test/validation_chainstate_tests.cpp b/src/test/validation_chainstate_tests.cpp
index 8e8726ce98..9f69b057c6 100644
--- a/src/test/validation_chainstate_tests.cpp
+++ b/src/test/validation_chainstate_tests.cpp
@@ -219,9 +219,13 @@ BOOST_FIXTURE_TEST_CASE(spendblock_rejects_empty_block, TestChain100Setup)
LOCK(cs_main);
CBlockIndex* tip = Assert(chainstate.m_chain.Tip());
- CBlock block;
- // CheckBlock() rejects empty blocks, ensure it gets run in SpendBlock
- block.hashPrevBlock = tip->GetBlockHash();
+ CBlock block{CreateBlock({}, CScript{} << OP_TRUE)};
+ // CheckBlock() rejects empty blocks; keep the header otherwise valid so
+ // SpendBlock reaches the size check instead of failing earlier on PoW.
+ block.vtx.clear();
+ 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()};
@@ -235,7 +239,7 @@ BOOST_FIXTURE_TEST_CASE(spendblock_rejects_empty_block, TestChain100Setup)
BOOST_CHECK(!chainstate.SpendBlock(block, &index_dummy, view_dummy, state, blockundo));
BOOST_CHECK(state.IsInvalid());
- BOOST_CHECK_EQUAL(state.GetRejectReason(), "high-hash");
+ BOOST_CHECK_EQUAL(state.GetRejectReason(), "bad-blk-length");
}
BOOST_AUTO_TEST_SUITE_END()
|
Updated 8217d10 -> b07f732 (spendblock_26 -> spendblock_27, compare) |
w0xlt
left a comment
w0xlt
left a comment
There was a problem hiding this comment.
reACK b07f732
I would suggest splitting two responsibilities inside Chainstate::ConnectBlock:
ValidateBlockUndo(...): a private helper that validates the block using the provided undo data.ConnectBlock(...): callsValidateBlockUndo(...), then writes undo data and updates block-index state if validation succeeds.
diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 6f675a9740..6e7a760051 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2295,15 +2295,28 @@ script_verify_flags GetBlockScriptFlags(const CBlockIndex& block_index, const Ch
return flags;
}
-bool Chainstate::ConnectBlock(const CBlock& block, const CBlockUndo& blockundo, BlockValidationState& state,
- CBlockIndex* pindex, bool fJustCheck)
+struct Chainstate::ConnectBlockValidationResult {
+ uint256 block_hash;
+ int n_inputs{0};
+ int64_t n_sigops_cost{0};
+ SteadyClock::time_point time_start{};
+ SteadyClock::time_point time_validation_done{};
+ bool is_genesis{false};
+};
+
+bool Chainstate::ValidateBlockUndo(
+ const CBlock& block,
+ const CBlockUndo& blockundo,
+ BlockValidationState& state,
+ const CBlockIndex& block_index,
+ bool fJustCheck,
+ ConnectBlockValidationResult& result)
{
AssertLockHeld(cs_main);
- assert(pindex);
- assert(pindex->GetBlockHash() == block.GetHash());
- auto block_hash{pindex->GetBlockHash()};
+ assert(block_index.GetBlockHash() == block.GetHash());
+ result.block_hash = block_index.GetBlockHash();
- const auto time_start{SteadyClock::now()};
+ result.time_start = SteadyClock::now();
const CChainParams& params{m_chainman.GetParams()};
assert(block.vtx.size() == blockundo.vtxundo.size() + 1); // blockundo does not contain a CTxUndo for the coinbase
@@ -2312,7 +2325,8 @@ bool Chainstate::ConnectBlock(const CBlock& block, const CBlockUndo& blockundo,
// Special case for the genesis block, skipping connection of its transactions
// (its coinbase is unspendable)
- if (block_hash == params.GetConsensus().hashGenesisBlock) {
+ if (result.block_hash == params.GetConsensus().hashGenesisBlock) {
+ result.is_genesis = true;
return true;
}
@@ -2329,13 +2343,13 @@ bool Chainstate::ConnectBlock(const CBlock& block, const CBlockUndo& blockundo,
BlockMap::const_iterator it{m_blockman.m_block_index.find(m_chainman.AssumedValidBlock())};
if (it == m_blockman.m_block_index.end()) {
script_check_reason = "assumevalid hash not in headers";
- } else if (it->second.GetAncestor(pindex->nHeight) != pindex) {
- script_check_reason = (pindex->nHeight > it->second.nHeight) ? "block height above assumevalid height" : "block not in assumevalid chain";
- } else if (m_chainman.m_best_header->GetAncestor(pindex->nHeight) != pindex) {
+ } else if (it->second.GetAncestor(block_index.nHeight) != &block_index) {
+ script_check_reason = (block_index.nHeight > it->second.nHeight) ? "block height above assumevalid height" : "block not in assumevalid chain";
+ } else if (m_chainman.m_best_header->GetAncestor(block_index.nHeight) != &block_index) {
script_check_reason = "block not in best header chain";
} else if (m_chainman.m_best_header->nChainWork < m_chainman.MinimumChainWork()) {
script_check_reason = "best header chainwork below minimumchainwork";
- } else if (GetBlockProofEquivalentTime(*m_chainman.m_best_header, *pindex, *m_chainman.m_best_header, params.GetConsensus()) <= TWO_WEEKS_IN_SECONDS) {
+ } else if (GetBlockProofEquivalentTime(*m_chainman.m_best_header, block_index, *m_chainman.m_best_header, params.GetConsensus()) <= TWO_WEEKS_IN_SECONDS) {
script_check_reason = "block too recent relative to best header";
} else {
// This block is a member of the assumed verified chain and an ancestor of the best header.
@@ -2357,20 +2371,20 @@ bool Chainstate::ConnectBlock(const CBlock& block, const CBlockUndo& blockundo,
}
const auto time_1{SteadyClock::now()};
- m_chainman.time_check += time_1 - time_start;
+ m_chainman.time_check += time_1 - result.time_start;
LogDebug(BCLog::BENCH, " - Sanity checks: %.2fms [%.2fs (%.2fms/blk)]\n",
- Ticks<MillisecondsDouble>(time_1 - time_start),
+ Ticks<MillisecondsDouble>(time_1 - result.time_start),
Ticks<SecondsDouble>(m_chainman.time_check),
Ticks<MillisecondsDouble>(m_chainman.time_check) / m_chainman.num_blocks_total);
// Enforce BIP68 (sequence locks)
int nLockTimeFlags = 0;
- if (DeploymentActiveAt(*pindex, m_chainman, Consensus::DEPLOYMENT_CSV)) {
+ if (DeploymentActiveAt(block_index, m_chainman, Consensus::DEPLOYMENT_CSV)) {
nLockTimeFlags |= LOCKTIME_VERIFY_SEQUENCE;
}
// Get the script flags for this block
- script_verify_flags flags{GetBlockScriptFlags(*pindex, m_chainman)};
+ script_verify_flags flags{GetBlockScriptFlags(block_index, m_chainman)};
const auto time_2{SteadyClock::now()};
m_chainman.time_forks += time_2 - time_1;
@@ -2384,10 +2398,10 @@ bool Chainstate::ConnectBlock(const CBlock& block, const CBlockUndo& blockundo,
if (script_check_reason != m_last_script_check_reason_logged && role.validated && !role.historical) {
if (fScriptChecks) {
LogInfo("Enabling script verification at block #%d (%s): %s.",
- pindex->nHeight, block_hash.ToString(), script_check_reason);
+ block_index.nHeight, result.block_hash.ToString(), script_check_reason);
} else {
LogInfo("Disabling script verification at block #%d (%s).",
- pindex->nHeight, block_hash.ToString());
+ block_index.nHeight, result.block_hash.ToString());
}
m_last_script_check_reason_logged = script_check_reason;
}
@@ -2403,13 +2417,11 @@ bool Chainstate::ConnectBlock(const CBlock& block, const CBlockUndo& blockundo,
std::vector<int> prevheights;
CAmount nFees = 0;
- int nInputs = 0;
- int64_t nSigOpsCost = 0;
for (unsigned int i = 0; i < block.vtx.size(); i++)
{
if (!state.IsValid()) break;
const CTransaction &tx = *(block.vtx[i]);
- nInputs += tx.vin.size();
+ result.n_inputs += tx.vin.size();
if (!tx.IsCoinBase())
{
@@ -2417,7 +2429,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, const CBlockUndo& blockundo,
CAmount txfee = 0;
TxValidationState tx_state;
- if (!Consensus::CheckTxInputs(tx, tx_state, spent_coins, pindex->nHeight, txfee)) {
+ if (!Consensus::CheckTxInputs(tx, tx_state, spent_coins, block_index.nHeight, txfee)) {
// Any transaction validation failure in ConnectBlock is a block consensus failure
state.Invalid(BlockValidationResult::BLOCK_CONSENSUS,
tx_state.GetRejectReason(),
@@ -2439,7 +2451,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, const CBlockUndo& blockundo,
prevheights[j] = spent_coins[j].nHeight;
}
- if (!SequenceLocks(tx, nLockTimeFlags, prevheights, *pindex)) {
+ if (!SequenceLocks(tx, nLockTimeFlags, prevheights, block_index)) {
state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-txns-nonfinal",
"contains a non-BIP68-final transaction " + tx.GetHash().ToString());
break;
@@ -2451,11 +2463,11 @@ bool Chainstate::ConnectBlock(const CBlock& block, const CBlockUndo& blockundo,
// * p2sh (when P2SH enabled in flags and excludes coinbase)
// * witness (when witness enabled in flags and excludes coinbase)
if (!tx.IsCoinBase()) {
- nSigOpsCost += GetTransactionSigOpCost(tx, std::span<const Coin>{blockundo.vtxundo[i - 1].vprevout}, flags);
+ result.n_sigops_cost += GetTransactionSigOpCost(tx, std::span<const Coin>{blockundo.vtxundo[i - 1].vprevout}, flags);
} else {
- nSigOpsCost += GetTransactionSigOpCost(tx, std::span<const Coin>{}, flags);
+ result.n_sigops_cost += GetTransactionSigOpCost(tx, std::span<const Coin>{}, flags);
}
- if (nSigOpsCost > MAX_BLOCK_SIGOPS_COST) {
+ if (result.n_sigops_cost > MAX_BLOCK_SIGOPS_COST) {
state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-blk-sigops", "too many sigops");
break;
}
@@ -2493,11 +2505,11 @@ bool Chainstate::ConnectBlock(const CBlock& block, const CBlockUndo& blockundo,
m_chainman.time_connect += time_3 - time_2;
LogDebug(BCLog::BENCH, " - Connect %u transactions: %.2fms (%.3fms/tx, %.3fms/txin) [%.2fs (%.2fms/blk)]\n", (unsigned)block.vtx.size(),
Ticks<MillisecondsDouble>(time_3 - time_2), Ticks<MillisecondsDouble>(time_3 - time_2) / block.vtx.size(),
- nInputs <= 1 ? 0 : Ticks<MillisecondsDouble>(time_3 - time_2) / (nInputs - 1),
+ result.n_inputs <= 1 ? 0 : Ticks<MillisecondsDouble>(time_3 - time_2) / (result.n_inputs - 1),
Ticks<SecondsDouble>(m_chainman.time_connect),
Ticks<MillisecondsDouble>(m_chainman.time_connect) / m_chainman.num_blocks_total);
- CAmount blockReward = nFees + GetBlockSubsidy(pindex->nHeight, params.GetConsensus());
+ CAmount blockReward = nFees + GetBlockSubsidy(block_index.nHeight, params.GetConsensus());
if (block.vtx[0]->GetValueOut() > blockReward && state.IsValid()) {
state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cb-amount",
strprintf("coinbase pays too much (actual=%d vs limit=%d)", block.vtx[0]->GetValueOut(), blockReward));
@@ -2514,12 +2526,32 @@ bool Chainstate::ConnectBlock(const CBlock& block, const CBlockUndo& blockundo,
}
const auto time_4{SteadyClock::now()};
m_chainman.time_verify += time_4 - time_2;
- LogDebug(BCLog::BENCH, " - Verify %u txins: %.2fms (%.3fms/txin) [%.2fs (%.2fms/blk)]\n", nInputs - 1,
+ LogDebug(BCLog::BENCH, " - Verify %u txins: %.2fms (%.3fms/txin) [%.2fs (%.2fms/blk)]\n", result.n_inputs - 1,
Ticks<MillisecondsDouble>(time_4 - time_2),
- nInputs <= 1 ? 0 : Ticks<MillisecondsDouble>(time_4 - time_2) / (nInputs - 1),
+ result.n_inputs <= 1 ? 0 : Ticks<MillisecondsDouble>(time_4 - time_2) / (result.n_inputs - 1),
Ticks<SecondsDouble>(m_chainman.time_verify),
Ticks<MillisecondsDouble>(m_chainman.time_verify) / m_chainman.num_blocks_total);
+ result.time_validation_done = time_4;
+
+ return true;
+}
+
+bool Chainstate::ConnectBlock(const CBlock& block, const CBlockUndo& blockundo, BlockValidationState& state,
+ CBlockIndex* pindex, bool fJustCheck)
+{
+ AssertLockHeld(cs_main);
+ assert(pindex);
+
+ ConnectBlockValidationResult validation_result;
+ if (!ValidateBlockUndo(block, blockundo, state, *pindex, fJustCheck, validation_result)) {
+ return false;
+ }
+
+ if (validation_result.is_genesis) {
+ return true;
+ }
+
if (fJustCheck) {
return true;
}
@@ -2529,9 +2561,9 @@ bool Chainstate::ConnectBlock(const CBlock& block, const CBlockUndo& blockundo,
}
const auto time_5{SteadyClock::now()};
- m_chainman.time_undo += time_5 - time_4;
+ m_chainman.time_undo += time_5 - validation_result.time_validation_done;
LogDebug(BCLog::BENCH, " - Write undo data: %.2fms [%.2fs (%.2fms/blk)]\n",
- Ticks<MillisecondsDouble>(time_5 - time_4),
+ Ticks<MillisecondsDouble>(time_5 - validation_result.time_validation_done),
Ticks<SecondsDouble>(m_chainman.time_undo),
Ticks<MillisecondsDouble>(m_chainman.time_undo) / m_chainman.num_blocks_total);
@@ -2548,12 +2580,12 @@ bool Chainstate::ConnectBlock(const CBlock& block, const CBlockUndo& blockundo,
Ticks<MillisecondsDouble>(m_chainman.time_index) / m_chainman.num_blocks_total);
TRACEPOINT(validation, block_connected,
- block_hash.data(),
+ validation_result.block_hash.data(),
pindex->nHeight,
block.vtx.size(),
- nInputs,
- nSigOpsCost,
- Ticks<std::chrono::nanoseconds>(time_5 - time_start)
+ validation_result.n_inputs,
+ validation_result.n_sigops_cost,
+ Ticks<std::chrono::nanoseconds>(time_5 - validation_result.time_start)
);
return true;
diff --git a/src/validation.h b/src/validation.h
index 3e69c8fe73..7785a84c3b 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -572,6 +572,15 @@ protected:
std::optional<const char*> m_last_script_check_reason_logged GUARDED_BY(::cs_main){};
+ struct ConnectBlockValidationResult;
+ bool ValidateBlockUndo(
+ const CBlock& block,
+ const CBlockUndo& blockundo,
+ BlockValidationState& state,
+ const CBlockIndex& block_index,
+ bool fJustCheck,
+ ConnectBlockValidationResult& result) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+
public:
//! Reference to a BlockManager instance which itself is shared across all
//! Chainstate instances.
Yes, I think this is a good change, but I'm not sure if it should be part of this pull request. Seems like this could be done as a purely mechanical follow-up? |
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.
This should introduce no behaviour change, but avoids a few extra vector allocations and coins lookups
…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. Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>
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. Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>
|
Rebased b07f732 -> ea94091 (spendblock_27 -> spendblock_28, compare)
|
|
🐙 This pull request conflicts with the target branch and needs rebase. |
| for (const CTxIn& input : tx.vin) { | ||
| coins.emplace_back(AccessCoin(input.prevout)); | ||
| } | ||
| return callback(std::span{coins}); |
There was a problem hiding this comment.
minor nit: acc to core guidelines, it would be good if we use std::forward here?
| return callback(std::span{coins}); | |
| return std::forward<Callable>(callback)(std::span{coins}); |
|
Thank you for the reviews and discussion (especially @w0xlt and @ajtowns), but I'm closing this again. Post parallel input fetcher I don't think there is a nice way to achieve the goals of this PR without materially changing its implementation. At that point I would rather open a new pull request. The overarching goal I want to achieve here can also be solved with #35187, which I would like people to focus their review on. @w0xlt do you want to open a separate PR with the unit tests introduced here? |
17 participants
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
ConnectBlockand into a new separate function calledSpendBlock. Pass a block's spent coins through the existingCBlockUndodata structure toConnectBlock.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)