Use CoinsViewOverlay to speed up ConnectBlock in TestBlockValidity. This also has the benefit of not inserting entries fetched from db into the main cache.
cs_main must be held for each place where the overlay is being used, and the reset guard ensures it will be reset for the next caller.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, 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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
~ACK 985d248383b43205f98a6971eb887ffa239adf85~
I was too hasty here, looks like we need to understand the regression a bit better.
For the record: This is a follow up to #31132 (review) and #34164 (review).
https://corecheck.dev/bitcoin/bitcoin/pulls/35751 claimed AssembleBlock slowed down considerably but I can't reproduce it locally (though I can confirm it's called from the benchmark).
<details><summary>Details</summary>
> Before:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 82,662.62 | 12,097.37 | 0.3% | 1.07 | `AssembleBlock`
> After
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 82,993.67 | 12,049.11 | 0.4% | 1.08 | `AssembleBlock`
</details>
Could you provide a reproducible way to test the performance claim, ideally using a deterministic set of mainnet block proposals or a microbenchmark? And since this change targets block-template validation, maybe we should ping @Sjors to get input from reviewers familiar with mining workloads.
https://corecheck.dev/bitcoin/bitcoin/pulls/35751 claimed AssembleBlock slowed down considerably @l0rinc is that the correct benchmark data you meant to post in the details? It doesn't seem to show the a slowdown...
Could you provide a reproducible way to test the performance claim, ideally using a deterministic set of mainnet block proposals or a microbenchmark?
There have been many benchmarks performed already by you in #31132 for this change. Is this not obvious to you it will increase performance?
If I fill up the block properly in the AssembleBlock benchmark, this does indeed then speed up connection. Seems like we should be doing that anyway to make the benchmark realistic?
I'm guessing there is a cutover where the synchronization overhead outweighs getting the coins from cache (or maybe even from disk?). Maybe it would be worthwhile to introduce a heuristic where we skip background fetching if the block does not spend a lot of coins?
Concept ACK on (indirectly) making the checkBlock() IPC faster. This is intended to be called frequently by the Job Declarator Server role in Stratum v2.
Although #35671 introduces a different approach for that, its makeTemplate() method also goes through TextBlockValidity. I wonder if TxCollection could be made even faster if pre-loads coins it needs, before a client calls makeTemplate()to check the full block. _Update: see https://github.com/Sjors/bitcoin/pull/122_
I haven't studied the earlier work, so for that reason alone a quick benchmark might be nice to demonstrate to others too.
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/30417973130/job/90468502585</sub>
<sub>LLM reason (✨ experimental): CI failed because IWYU reported a missing/incorrect #include in src/bench/block_assemble.cpp (triggering the “Failure generated from IWYU” step).</sub>
<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>
Yes, I see the regression in the in corecheck as well. I first read @l0rinc's comment as containing the regression. My apologies.
@Sjors I've included an additional commit at the start that improves the AssembleBlock benchmark by filling the block to max weight with transactions. The new benchmark shows a 10% speedup on my local machine when comparing the first and second commits.
<details><summary>Before this change</summary>
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark | |
|---|---|---|---|---|---|---|---|---|---|---|
| 16,370,829.20 | 61.08 | 0.5% | 169,414,150.61 | 36,723,542.62 | 4.613 | 21,344,193.75 | 0.2% | 5.71 | AssembleBlock |
|
| 16,157,284.00 | 61.89 | 2.9% | 169,412,308.63 | 36,308,394.28 | 4.666 | 21,345,426.37 | 0.2% | 5.61 | AssembleBlock |
|
| 16,328,205.58 | 61.24 | 1.6% | 169,374,584.69 | 36,629,119.06 | 4.624 | 21,343,993.70 | 0.2% | 5.57 | AssembleBlock |
</details>
<details><summary>After this change</summary>
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark | |
|---|---|---|---|---|---|---|---|---|---|---|
| 14,860,137.50 | 67.29 | 1.8% | 160,474,005.09 | 35,797,692.66 | 4.483 | 21,636,973.66 | 0.2% | 5.42 | AssembleBlock |
|
| 14,322,912.28 | 69.82 | 1.1% | 160,432,472.41 | 34,525,565.16 | 4.647 | 21,656,523.97 | 0.2% | 5.41 | AssembleBlock |
|
| 14,767,059.78 | 67.72 | 0.7% | 160,474,240.27 | 35,583,122.53 | 4.510 | 21,644,652.59 | 0.2% | 5.49 | AssembleBlock |
</details>
I'm guessing there is a cutover where the synchronization overhead outweighs getting the coins from cache (or maybe even from disk?). Maybe it would be worthwhile to introduce a heuristic where we skip background fetching if the block does not spend a lot of coins? @sedited There is likely a cutover, but it would be dependent on the system as well as other factors like how many coins are in the cache vs have to be fetched from db. re: a heuristic - what we really care about is that slow blocks are faster. A block spending few coins is already very fast, and so speeding it up from 2ms -> 1ms will not really benefit anything in the grand scheme IMO.
4543 | uint256 block_hash(block.GetHash()); 4544 | index_dummy.pprev = tip; 4545 | index_dummy.nHeight = tip->nHeight + 1; 4546 | index_dummy.phashBlock = &block_hash; 4547 | - CCoinsViewCache view_dummy(&chainstate.CoinsTip()); 4548 | + CoinsViewOverlay& view_dummy{chainstate.ConnectBlockView()};
ff4dd42 validation: use parallel input prevout fetching in TestBlockValidity:
A note on "dummy" naming: before the change, view_dummy was a local-only instance, so the "dummy" naming makes sense. However, now it's a reference to a longer-living instance, so I find "dummy" misleading. I suggest renaming it to view_shared. (There are not many usages to change).
I renamed it to overlay_view.
Another performance improvement, a must-have follow-up of #35295/#34164/#31132, looks good.
Two comments:
I am confused by the description of the 2nd (last) commit. It states:
"Use CoinsViewOverlay to speed up ConnectBlock in TestBlockValidity." -- CoinsViewOverlay was used before; the change seems not about using it, but using an existing shared instance, instead of always a new one.
"This also has the benefit of not populating entries in the main cache." -- that was true also without the change.
There is also another usage of m_connect_block_view, at line 3055:
3055 CoinsViewOverlay& view{*m_coins_views->m_connect_block_view};
3056 const auto reset_guard{view.StartFetching(*block_to_connect)};
Could this also use the newly introduced ConnectBlockView()?
Use CoinsViewOverlay to speed up ConnectBlock in TestBlockValidity.
This also has the benefit of not inserting entries fetched from db
into the main cache.
cs_main must be held for each place where the overlay is being used,
and the reset guard ensures it will be reset for the next caller.
Thanks for your review @optout21.
"Use CoinsViewOverlay to speed up ConnectBlock in TestBlockValidity." -- CoinsViewOverlay was used before; the change seems not about using it, but using an existing shared instance, instead of always a new one.
"This also has the benefit of not populating entries in the main cache." -- that was true also without the change.
It was using a CCoinsViewCache before as an ephemeral cache on top of the main cache. This patch changes it to use the CoinsViewOverlay. The previous version would insert cache misses into the cache before returning to the view_dummy cache, while CoinsViewOverlay does not do that. I updated the commit message and PR description to try and clarify that.
Could this also use the newly introduced ConnectBlockView()?
Done.
44 | tx.vin.emplace_back(MineBlock(test_setup->m_node, options)); 45 | tx.vin.back().scriptWitness = witness; 46 | - tx.vout.emplace_back(1337, P2WSH_OP_TRUE); 47 | + tx.vout.assign(FAN_OUT, CTxOut{100'000, P2WSH_OP_TRUE}); 48 | if (NUM_BLOCKS - b >= COINBASE_MATURITY) 49 | txs.at(b) = MakeTransactionRef(tx);
nit: we could std::move a few of these (please check the other cases as well):
txs.at(b) = MakeTransactionRef(std::move(tx));
62 | + // Collect loose transactions spending the fan-out outputs to fill the assembled block 63 | + { 64 | + LOCK(::cs_main); 65 | + 66 | + for (const auto& txr : txs) { 67 | + for (const uint32_t o : std::views::iota(uint32_t{0}, FAN_OUT)) {
nit: I don't usually mind this iteration, but above we're using standard loop which makes the two iterations assymetric
The commit message claims we fill assembled block to max weight, but we still have leftover transactions - can we assert the claim (and adjust the FAN_OUT to make it true? Just make sure the number is not 110, we're still sensitive to that...)
assert(GetBlockWeight(*PrepareBlock(test_setup->m_node, options)) > MAX_BLOCK_WEIGHT - DEFAULT_BLOCK_RESERVED_WEIGHT);
}
Edit: We could also assert that mining the fan-out transactions empties the mempool, to verify that the inputs are confirmed.
43 | CMutableTransaction tx; 44 | tx.vin.emplace_back(MineBlock(test_setup->m_node, options)); 45 | tx.vin.back().scriptWitness = witness; 46 | - tx.vout.emplace_back(1337, P2WSH_OP_TRUE); 47 | + tx.vout.assign(FAN_OUT, CTxOut{100'000, P2WSH_OP_TRUE}); 48 | if (NUM_BLOCKS - b >= COINBASE_MATURITY)
67 | + for (const uint32_t o : std::views::iota(uint32_t{0}, FAN_OUT)) { 68 | + CMutableTransaction tx; 69 | + tx.vin.emplace_back(COutPoint{txr->GetHash(), o}); 70 | + tx.vin.back().scriptWitness = witness; 71 | + tx.vout.emplace_back(1337, P2WSH_OP_TRUE); 72 | + const MempoolAcceptResult res = test_setup->m_node.chainman->ProcessTransaction(MakeTransactionRef(tx));
8ebb32e bench: fill assembled block to max weight in AssembleBlock:
Nit: the fixture checks the entire growing mempool after each insertion, we could probably disable those checks.
<details><summary>Avoid repeated mempool checks during setup</summary>
diff --git a/src/bench/block_assemble.cpp b/src/bench/block_assemble.cpp
index 5c15f3cebc..9f9dc5fae1 100644
--- a/src/bench/block_assemble.cpp
+++ b/src/bench/block_assemble.cpp
@@ -29,7 +29,7 @@ using node::BlockCreateOptions;
static void AssembleBlock(benchmark::Bench& bench)
{
- const auto test_setup = MakeNoLogFileContext<const TestingSetup>();
+ const auto test_setup = MakeNoLogFileContext<const TestingSetup>(ChainType::REGTEST, {.extra_args = {"-checkmempool=0"}});
CScriptWitness witness;
witness.stack.push_back(WITNESS_STACK_ELEM_OP_TRUE);
</details>
4543 | uint256 block_hash(block.GetHash()); 4544 | index_dummy.pprev = tip; 4545 | index_dummy.nHeight = tip->nHeight + 1; 4546 | index_dummy.phashBlock = &block_hash; 4547 | - CCoinsViewCache view_dummy(&chainstate.CoinsTip()); 4548 | + CoinsViewOverlay& overlay_view{chainstate.ConnectBlockView()};
2ba5628 validation: use parallel input prevout fetching in TestBlockValidity:
TestBlockValidity() previously used a local cache. This PR changes it to reuse ConnectBlockView(), which a synchronous BlockChecked callback can reach while ConnectTip() still has a fetch active.
Could we first record that both paths now select the same overlay?
BOOST_AUTO_TEST_CASE(testblockvalidity_with_active_connect_view)
{
auto tip_hash{WITH_LOCK(cs_main, return Assert(m_node.chainman)->ActiveTip()->GetBlockHash())};
auto outer_block{BadBlock(tip_hash)};
auto candidate_block{GoodBlock(tip_hash)};
LOCK(cs_main);
auto& chainstate{Assert(m_node.chainman)->ActiveChainstate()};
auto reset_guard{chainstate.ConnectBlockView().StartFetching(*outer_block)};
BOOST_CHECK(&chainstate.TestBlockValidityView() != &chainstate.ConnectBlockView());
BOOST_CHECK(TestBlockValidity(chainstate, *candidate_block, /*check_pow=*/false, /*check_merkle_root=*/false).IsValid());
}
I think the reentrancy issue needs to be addressed before this can be merged.
I don't think there is a reentrancy issue? I see though that BlockChecked callbacks in ConnectTip are synchronous, so a future change could theoretically call TestBlockValidity from the callback on the same thread. Nothing does that today. We can prevent this by calling Assert(overlay_view.GetCacheSize() == 0); inside TestBlockValidity. I'm not sure I see the value of this unit test.
Agree, the test is just a demonstration, an assert is the ideal solution here!
I think the reentrancy issue needs to be addressed before this can be merged. I also left a few comments on making the benchmark match its stated workload, plus some nits (feel free to just resolve those, but since we need to push anyway, we might as well clean this up a bit).
@andrewtoth what is the status here? I think it would be interesting to add some more real block creation timings, i.e. run this side-by-side with master on hopefully a similar mempool and see if there is a difference.
@sedited thanks for the ping. I have been distracted by other issues but I am still interested in pursuing this. I will address @l0rinc's latest comments soon.
One thing I would like to understand better is how checkBlock() is used in practice. How often do we actually call it? One thing to consider is that with this method none of the uncached coins will be pre-emptively inserted into the cache. When many of the block's coins are not cached, this will be much faster on the first call. However, currently these coins will all be cached, while after this change they will not be. So subsequent calls with a similar block might be slower due to not inserting the coins in the cache.
@Sjors' idea https://github.com/Sjors/bitcoin/pull/122 might help here as well, but it's a much larger change.
run this side-by-side with master on hopefully a similar mempool and see if there is a difference.
Yes, good idea. I will try and gather some real world data with this.
Marking as draft until I figure out the above.
4544 | index_dummy.pprev = tip; 4545 | index_dummy.nHeight = tip->nHeight + 1; 4546 | index_dummy.phashBlock = &block_hash; 4547 | - CCoinsViewCache view_dummy(&chainstate.CoinsTip()); 4548 | + CoinsViewOverlay& overlay_view{chainstate.ConnectBlockView()}; 4549 | + const auto reset_guard{overlay_view.StartFetching(block)};
2ba5628 validation: use parallel input prevout fetching in TestBlockValidity:
Before #35295, coins were fetched as validation reached each transaction, so any failure avoided fetching later transactions, but StartFetching() can now retain those coins before validation rejects the template.
In normal block connection this is gated behind PoW, so paying for that work to allocate an extra ~250 MiB on a node (~25k max inputs Ă— 10 kB max script sizes) seems like a poor trade-off for an attacker. Not sure if this can be amplified in any way by the threads or if it's a total (probably the latter).
But I wonder if this changes the threat model here, given that these template checks aren't behind PoW.
In normal block connection this is gated behind PoW, so paying for that work to allocate an extra ~250 MiB on a node (~25k max inputs Ă— 10 kB max script sizes) seems like a poor trade-off for an attacker. Not sure if this can be amplified in any way by the threads or if it's a total (probably the latter).
I'm not sure I follow why an attacker would bother creating an invalid block here? If the goal is to have the node allocate ~250MB, they could create a block that spends ~25k distinct utxos with 10kb scripts. Why would they bother creating an invalid block to do this, instead of creating a valid one and keeping the block reward? I also don't see how #35295 changes anything here; this would allocate the same amount of memory before or after that PR.
But I wonder if this changes the threat model here, given that these template checks aren't behind PoW.
I don't think TestBlockValidity can be called via any untrusted paths, so any threat-model is substantially reduced.
These fetches happen before input validation, so the submitter doesn’t need PoW or valid signatures to trigger them. They can reference other people’s UTXOs, even repeating the same large-script UTXO across distinct transactions, and prefetching can allocate copies before validation rejects the attempted spends.