validation: add reusable coins view for ConnectBlock #34164
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/34164. ReviewsSee the guideline 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. |
l0rinc
left a comment
l0rinc
left a comment
There was a problem hiding this comment.
Concept ACK, carving this out is a small step in speedup up IBD and simplifying the review for the heavier #31132
I want to make sure we make the code safer than before, so I'm suggesting we cleanup up the spread-around state changes a bit since it's hard to reason about it otherwise - especially trying to imagine a multi-threaded context where lazy inits and resets and direct setters and field accesses are potentially competing.
andrewtoth
force-pushed
the
reuse-connect-block
branch
from
3230be9 to
ef3472e
Compare
andrewtoth
force-pushed
the
reuse-connect-block
branch
from
ef3472e to
b46b61b
Compare
l0rinc
left a comment
l0rinc
left a comment
There was a problem hiding this comment.
Redid the change locally, ended up with slightly different setup, but your solution might handle the lifecycle slightly better.
I ran a reindex-chainstate benchmark, the difference is within noise 👍.
Left some comments, but I'm fine with merging as is:
ACK b46b61b
src/validation.cpp
Outdated
| { | ||
| AssertLockHeld(::cs_main); | ||
| m_cacheview = std::make_unique<CCoinsViewCache>(&m_catcherview); | ||
| m_connect_block_view = std::make_unique<CCoinsViewCache>(&*m_cacheview); |
There was a problem hiding this comment.
I noticed ConnectTip() always uses m_coins_views->m_connect_block_view instead of storing a m_connect_block_view member on Chainstate (initialized in Chainstate::InitCoinsCache()).
Is the intent to keep the connect-block cache’s lifetime tightly coupled to m_coins_views/m_cacheview (since CCoinsViewCache holds a raw backend pointer), so we don’t need to re-point/reset it when coins views are reset?
Maybe we have to rethink the lifetime bound of the backing cache pointer...
There was a problem hiding this comment.
Is the intent to keep the connect-block cache’s lifetime tightly coupled to m_coins_views/m_cacheview (since CCoinsViewCache holds a raw backend pointer), so we don’t need to re-point/reset it when coins views are reset?
Yes. It is also declared after m_cacheview so will be destroyed before the backing pointer.
Maybe we have to rethink the lifetime bound of the backing cache pointer...
We could bound it with an annotation, but anything calling SetBackend would negate it and also update the bounds to the new view passed in.
I think to be correct we would want a shared_ptr as the base. But, I think that type of change is out of scope for this PR.
|
ACK b46b61b Built and tested on macOS. Clean implementation - |
andrewtoth
force-pushed
the
reuse-connect-block
branch
from
b46b61b to
fbb68c7
Compare
|
I updated this to have the |
Add a Reset() method to CCoinsViewCache that clears cacheCoins, cachedCoinsUsage, and hashBlock without flushing to the base view. This allows efficiently reusing a cache instance across multiple blocks. Introduce CoinsViewCacheController to wrap a CCoinsViewCache and provide scoped access through a Handle. The Handle dereferences to the cache and automatically calls Reset() on destruction. This RAII pattern ensures the cache is always properly reset between blocks. Add m_connect_block_controller as a persistent controller for ConnectBlock, avoiding repeated memory allocations. Co-authored-by: l0rinc <[email protected]>
More accurately reflects the purpose of the parameter, since we will keep reusing the cache but don't want to reallocate it.
andrewtoth
force-pushed
the
reuse-connect-block
branch
from
fbb68c7 to
f648dcc
Compare
4 participants
This is the first commit of #31132, which can be merged as an independent change. It has a small benefit on its own, but will help in moving the parent PR forward.
Add a
Reset()method toCCoinsViewCachethat clearscacheCoins,cachedCoinsUsage, andhashBlockwithout flushing to thebaseview. This allows efficiently reusing a cache instance across multiple blocks.Introduce
CoinsViewCacheControllerto wrap aCCoinsViewCacheand provide scoped access through aHandle. TheHandledereferences to the cache and automatically callsReset()on destruction. This RAII pattern ensures the cache is always properly reset between blocks.Add
m_connect_block_controlleras a persistent controller forConnectBlock, avoiding repeated memory allocations.