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-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35751.
<!--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:
util::Expected by yuvicc)CAmount a class by hodlinator)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);
}
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));
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).