validation: add reusable coins view for ConnectBlock

andrewtoth commented at 8:05 pm on December 28, 2025: contributor

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 to CCoinsViewCache that clears cacheCoins, cachedCoinsUsage, and hashBlock without flushing to the base view. This allows efficiently reusing a cache instance across multiple blocks.

Add CCoinsViewCache::CreateResetGuard method to return a CCoinsViewCache::ResetGuard. The ResetGuard automatically calls Reset() on destruction. This RAII pattern ensures the cache is always properly reset between blocks.

Add m_connect_block_view as a persistent CCoinsViewCache for ConnectBlock, avoiding repeated memory allocations.

DrahtBot added the label Validation on Dec 28, 2025

DrahtBot commented at 8:05 pm on December 28, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34164.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	sedited, l0rinc, achow101
Stale ACK	bensig, sipa

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34374 (kernel: use structured logging and simplify logging interface by stickies-v)
#34165 (coins: don’t mutate main cache when connecting block by andrewtoth)
#34132 (refactor: inline CCoinsViewErrorCatcher into CCoinsViewDB by l0rinc)
#34124 (refactor: make CCoinsView a purely virtual abstract base class by l0rinc)
#32317 (kernel: Separate UTXO set access from validation functions by sedited)
#31132 (validation: fetch block inputs on parallel threads 3x faster IBD by andrewtoth)

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.

in src/coins.cpp:281 in 3230be92e8

274@@ -275,6 +275,13 @@ bool CCoinsViewCache::Sync()
275     return fOk;
276 }
277 
278+void CCoinsViewCache::Reset() noexcept
279+{
280+    cacheCoins.clear();
281+    cachedCoinsUsage = 0;
282+    hashBlock.SetNull();

l0rinc commented at 10:46 am on January 2, 2026:

This resets hashBlock, but Flush()/Sync() pass the raw hashBlock into base->BatchWrite():

This means that the behavior of these methods depends on whether the GetBestBlock was called before these methods (since the “getter” would set the field after which accessing it directly would yield a different result): https://github.com/bitcoin/bitcoin/blob/adbb4b320834516dc97fa33aaa853a45cbabca67/src/coins.cpp#L176-L180 While it happens to be safe in this particular case as far as I can tell, adding a public Reset() makes the footgun more relevant.

This isn’t just theoretical, we have calls to that getter only for its side-effect: https://github.com/bitcoin/bitcoin/blob/adbb4b320834516dc97fa33aaa853a45cbabca67/src/validation.cpp#L874-L876

I understand that some of this wasn’t introduced in this commit, but this change makes this potential misuse more obvious.

Nit: In BatchWrite() we’re setting hashBlock explicitly: https://github.com/bitcoin/bitcoin/blob/adbb4b320834516dc97fa33aaa853a45cbabca67/src/coins.cpp#L248 Consider using the setter consistently so best-block updates go through one path (helps with debugging and consistency). It’s not strictly related, but seems like a good opportunity to make this area more reliable (otherwise it’s hard to reason about the state if mutations are spread around).

Please see l0rinc/bitcoin@2838a34 (#79) for such an attempt.

andrewtoth commented at 8:47 pm on January 3, 2026:

Done, I use the setter instead of setting directly.

l0rinc commented at 4:18 pm on January 5, 2026:

We will need to deal with the lazy GetBestBlock eventually - I agree it’s not a problem here, can be done in a follow-up.

l0rinc commented at 8:16 pm on January 25, 2026:

I use the setter instead of setting directly.

Seems it was reverted in latest push - if deliberate, please explain

andrewtoth commented at 11:38 pm on January 25, 2026:

Added this back in its own commit.

in src/validation.cpp:3102 in 3230be92e8 outdated

3101@@ -3101,7 +3102,7 @@ bool Chainstate::ConnectTip(
3102     LogDebug(BCLog::BENCH, "  - Load block from disk: %.2fms\n",
3103              Ticks<MillisecondsDouble>(time_2 - time_1));
3104     {
3105-        CCoinsViewCache view(&CoinsTip());
3106+        auto& view{*m_coins_views->m_connect_block_view};

l0rinc commented at 10:55 am on January 2, 2026:

This PR reuses the cache in ConnectTip() only, but we still construct short-lived CCoinsViewCache instances in DisconnectTip() and TestBlockValidity():

We also have a few other ones that may not need any reusability (we might want to comment for those cases for why we chose single-use caches, since they’re rare events and would inflate the cache with more than 1 block of data):

If the goal is avoiding repeated allocations, I think we should also consider reusing the persistent view in the related cases in this PR.

Even if we decide not to do it in this PR, it raises the question: should we assert that nobody is using the same cache at the same time we are, which would require us adding some sanity check after acquiring it to make sure we’re using a properly reset cache, e.g. assert(view.GetCacheSize() == 0).

andrewtoth commented at 8:47 pm on January 3, 2026:

The goal is avoiding repeated allocations in the hot path. The other places are used much less frequently. This will also be refactored to be even more specialized for connecting blocks, so I don’t think we should reuse it for these other places.

l0rinc commented at 4:17 pm on January 5, 2026:

Makes sense, we can deal with the other cases separately

in src/validation.cpp:3124 in 3230be92e8

3120@@ -3119,8 +3121,9 @@ bool Chainstate::ConnectTip(
3121                  Ticks<MillisecondsDouble>(time_3 - time_2),
3122                  Ticks<SecondsDouble>(m_chainman.time_connect_total),
3123                  Ticks<MillisecondsDouble>(m_chainman.time_connect_total) / m_chainman.num_blocks_total);
3124-        bool flushed = view.Flush(/*will_reuse_cache=*/false); // local CCoinsViewCache goes out of scope
3125+        bool flushed = view.Flush(/*will_reuse_cache=*/false); // No need to reallocate since it only has capacity for 1 block

l0rinc commented at 11:09 am on January 2, 2026:

the new comment is useful, but the Flush parameter changed its meaning slightly: we are reusing the cache and yet we pass false because what we really mean is that it’s fine to keep the allocated memory around, it’s not a big deal (since it’s small, unlike when we keep the whole UTXO set in memory) - please consider renaming the parameter to reflect that.

andrewtoth commented at 8:46 pm on January 3, 2026:

Done in a separate commit.

in src/validation.h:492 in 3230be92e8 outdated

487@@ -488,6 +488,10 @@ class CoinsViews {
488     //! can fit per the dbcache setting.
489     std::unique_ptr<CCoinsViewCache> m_cacheview GUARDED_BY(cs_main);
490 
491+    //! Used as an empty view that is only passed into ConnectBlock to help speed up block validation,
492+    //! as well as not pollute the underlying cache with newly created coins in case the block is invalid.

l0rinc commented at 11:40 am on January 2, 2026:

m_connect_block_view isn’t an empty view of the UTXO set - it’s just a sandbox, a scratch space for ConnectBlock(). Consider rewording to avoid implying it has no backing state.

0    //! Temporary `CCoinsViewCache` layered on top of `m_cacheview` and passed to `ConnectBlock()`.
1    //! Reset between calls and flushed only on success, so invalid blocks don't pollute the underlying cache.

l0rinc changes_requested

l0rinc commented at 11:43 am on January 2, 2026: contributor

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.

DrahtBot added the label Needs rebase on Jan 3, 2026

andrewtoth force-pushed on Jan 3, 2026

DrahtBot removed the label Needs rebase on Jan 3, 2026

andrewtoth force-pushed on Jan 3, 2026

in src/validation.cpp:1864 in ee969b523f

1860@@ -1861,6 +1861,7 @@ void CoinsViews::InitCache()
1861 {
1862     AssertLockHeld(::cs_main);
1863     m_cacheview = std::make_unique<CCoinsViewCache>(&m_catcherview);
1864+    m_connect_block_view = std::make_unique<CCoinsViewCache>(&*m_cacheview);

l0rinc commented at 3:48 pm on January 5, 2026:

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…

andrewtoth commented at 3:20 pm on January 15, 2026:

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.

in src/test/coins_tests.cpp:1132 in ee969b523f outdated

1127+
1128+    const COutPoint outpoint{Txid::FromUint256(m_rng.rand256()), m_rng.rand32()};
1129+
1130+    const Coin coin{CTxOut{m_rng.randrange(10), CScript{} << m_rng.randbytes(CScriptBase::STATIC_SIZE + 1)}, 1, false};
1131+    cache.EmplaceCoinInternalDANGER(COutPoint{outpoint}, Coin{coin});
1132+    cache.SetBestBlock(uint256::ONE);

l0rinc commented at 4:01 pm on January 5, 2026:

We currently set the (cache) best block to uint256::ONE and then expect Reset() to bring it back to uint256::ZERO.

That relies on CCoinsViewCache::GetBestBlock() treating 0 as uninitialized and lazily pulling from the base view, and in practice the best block should never be null. Could we instead set the base best block to some random value and set the cache best block to a different value, then assert that after Reset() the cache’s GetBestBlock() matches the base best block? This would better exercise propagation across layers and avoids special-casing 0/1:

 0BOOST_AUTO_TEST_CASE(ccoins_reset)
 1{
 2    CCoinsViewTest root{m_rng};
 3    CCoinsViewCache root_cache{&root};
 4    const uint256 base_best_block{m_rng.rand256()};
 5    root_cache.SetBestBlock(base_best_block);
 6    root_cache.Flush();
 7
 8    CCoinsViewCacheTest cache{&root};
 9
10    const COutPoint outpoint{Txid::FromUint256(m_rng.rand256()), m_rng.rand32()};
11
12    const Coin coin{CTxOut{m_rng.randrange(10), CScript{} << m_rng.randbytes(CScriptBase::STATIC_SIZE + 1)}, 1, false};
13    cache.EmplaceCoinInternalDANGER(COutPoint{outpoint}, Coin{coin});
14
15    const uint256 cache_best_block{uint256::ONE};
16    cache.SetBestBlock(cache_best_block);
17    cache.SelfTest();
18
19    BOOST_CHECK(cache.AccessCoin(outpoint) == coin);
20    BOOST_CHECK(!cache.AccessCoin(outpoint).IsSpent());
21    BOOST_CHECK_EQUAL(cache.GetCacheSize(), 1);
22    BOOST_CHECK_EQUAL(cache.GetBestBlock(), cache_best_block);
23
24    for (int i = 0; i < 2; ++i) { // Reset is idempotent
25        cache.Reset();
26        BOOST_CHECK(cache.AccessCoin(outpoint).IsSpent());
27        BOOST_CHECK_EQUAL(cache.GetCacheSize(), 0);
28        BOOST_CHECK_EQUAL(cache.GetBestBlock(), base_best_block);
29    }
30}

andrewtoth commented at 11:10 pm on January 15, 2026:

Done.

in src/coins.h:456 in ee969b523f outdated

451@@ -452,6 +452,9 @@ class CCoinsViewCache : public CCoinsViewBacked
452      */
453     void Sync();
454 
455+    //! Wipe local state.
456+    void Reset() noexcept;

l0rinc commented at 4:14 pm on January 5, 2026:

Exposing this comes with some new dangers, we might want to document it:

0    /**
1     * Discard all modifications made to this cache without flushing to the base view.
2     * This can be used to efficiently reuse a cache instance across multiple operations.
3     */
4    void Reset() noexcept;

andrewtoth commented at 11:11 pm on January 15, 2026:

Done.

in src/test/fuzz/coins_view.cpp:91 in ee969b523f outdated

84@@ -85,6 +85,11 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend
85                 if (is_db && best_block.IsNull()) best_block = uint256::ONE;
86                 coins_view_cache.SetBestBlock(best_block);
87             },
88+            [&] {
89+                coins_view_cache.Reset();
90+                // Set best block hash to non-null to satisfy the assertion in CCoinsViewDB::BatchWrite().
91+                if (is_db) coins_view_cache.SetBestBlock(uint256::ONE);

l0rinc commented at 4:16 pm on January 5, 2026:

what if we randomized this, too?

andrewtoth commented at 11:11 pm on January 15, 2026:

Done.

l0rinc approved

l0rinc commented at 4:22 pm on January 5, 2026: contributor

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 b46b61bdcefbaa649fc73b8f8936bc80468de763

bensig commented at 10:06 pm on January 5, 2026: contributor

ACK b46b61bdcefbaa649fc73b8f8936bc80468de763

Built and tested on macOS. Clean implementation - Reset() method is straightforward, tests cover it well, and the m_connect_block_view integration in ConnectTip() correctly calls Reset() on both failure and success paths.

andrewtoth force-pushed on Jan 15, 2026

andrewtoth commented at 3:22 pm on January 15, 2026: contributor

I updated this to have the ChainState own a controller instead of the actual CCoinsViewCache. The controller owns a private CCoinsViewCache, and returns a handle to it via a Start method. The handle dereferences to the CCoinsViewCache, and automatically calls Reset when the handle goes out of scope.

andrewtoth force-pushed on Jan 15, 2026

andrewtoth force-pushed on Jan 24, 2026

andrewtoth commented at 7:11 pm on January 24, 2026: contributor

Split the first commit into 3 for easier review. Also added noexcept to several methods in CoinsViewCacheController::Handle as well as const dereference methods.

git diff f648dcc1b6407511dd2bdbb3d2ad8d61c3fabdd1..c5ebbf56ce9085d925fea9c97571df942bf0d61e

in src/validation.cpp:1864 in c5ebbf56ce outdated

1860@@ -1856,6 +1861,7 @@ void CoinsViews::InitCache()
1861 {
1862     AssertLockHeld(::cs_main);
1863     m_cacheview = std::make_unique<CCoinsViewCache>(&m_catcherview);
1864+    m_connect_block_controller = std::make_unique<CoinsViewCacheController>(&*m_cacheview);

l0rinc commented at 8:08 pm on January 25, 2026:

without the lock this could cause a race when we reinit m_cacheview in the previous line which could have been backing a previous m_connect_block_controller - but seems fine as it is.

in src/test/coinsviewcachecontroller_tests.cpp:27 in c5ebbf56ce

22+    CBlock block;
23+    CMutableTransaction coinbase;
24+    coinbase.vin.emplace_back();
25+    block.vtx.push_back(MakeTransactionRef(coinbase));
26+
27+    Txid prevhash{Txid::FromUint256(uint256{1})};

l0rinc commented at 8:09 pm on January 25, 2026:

didn’t we remove this at one point?

andrewtoth force-pushed on Jan 25, 2026

andrewtoth commented at 11:38 pm on January 25, 2026: contributor

Thanks for your review @l0rinc. Indeed, some changes were erroneously not included in the latest push. I added them back.

git diff c5ebbf56ce9085d925fea9c97571df942bf0d61e..3417bdbeab81a6aef9f827220ac58219f68d2732

in src/test/coins_tests.cpp:1128 in 3417bdbeab outdated

1119@@ -1120,4 +1120,38 @@ BOOST_AUTO_TEST_CASE(ccoins_emplace_duplicate_keeps_usage_balanced)
1120     BOOST_CHECK(cache.AccessCoin(outpoint) == coin1);
1121 }
1122 
1123+BOOST_AUTO_TEST_CASE(ccoins_reset)
1124+{
1125+    CCoinsViewTest root{m_rng};
1126+    CCoinsViewCache root_cache{&root};
1127+    uint256 base_best_block{m_rng.rand256()};
1128+    if (base_best_block.IsNull()) base_best_block = uint256::ONE;

l0rinc commented at 4:56 pm on January 26, 2026:

I think this is just noise, if this happens, we’re in big trouble…

in src/test/fuzz/coins_view.cpp:96 in 3417bdbeab outdated

89+                coins_view_cache.Reset();
90+                // Set best block hash to non-null to satisfy the assertion in CCoinsViewDB::BatchWrite().
91+                if (is_db) {
92+                    const uint256 best_block{ConsumeUInt256(fuzzed_data_provider)};
93+                    if (best_block.IsNull()) {
94+                        good_data = false;

l0rinc commented at 5:00 pm on January 26, 2026:

I guess the fuzzer isn’t fully random, so this can happen 👍

in src/test/coinsviewcachecontroller_tests.cpp:19 in 3417bdbeab

14+
15+#include <ranges>
16+
17+BOOST_AUTO_TEST_SUITE(coinsviewcachecontroller_tests)
18+
19+static CBlock CreateBlock() noexcept

l0rinc commented at 5:03 pm on January 26, 2026:

can we also do the anonymous namespace adjustment here, similarly to #34165 (review)?

nit: not sure we care about noexcept in tests, seems like noise

andrewtoth commented at 11:47 pm on January 27, 2026:

This file is removed.

in src/coinsviewcachecontroller.h:14 in 3417bdbeab

 9+
10+/**
11+ * Controller that manages a CCoinsViewCache and provides scoped access through a Handle.
12+ * The Handle dereferences to the internal cache and automatically resets it on destruction.
13+ */
14+class CoinsViewCacheController

l0rinc commented at 5:19 pm on January 26, 2026:

This is a pretty cool way to solve deletion - but it’s extremely verbose for such a simple request, seems like a workaround for pre-smart-pointer-solution.

I wonder if a non-owning std::unique_ptr that has a custom std::unique_ptr<CCoinsViewCache, ScopedCacheDeleter> which calls Reset() but does not delete the object. Let me experiment with it locally and add a patch here if it works.

andrewtoth commented at 6:03 pm on January 26, 2026:

a non-owning std::unique_ptr

I’m not sure that’s the intended use of a unique_ptr, or if that’s even possible :shrug:

l0rinc commented at 7:41 pm on January 26, 2026:

You’re right, I couldn’t make it better than what we have now 👍

What we have here is basically a C++26 scope_exit-style guard (LLVM seems to have this as https://raw.githubusercontent.com/llvm/llvm-project/refs/heads/main/llvm/include/llvm/ADT/ScopeExit.h). I wondered if we need to keep CoinsViewCacheController tied to the current code, or if we can add a general util::ScopeExit that we can reuse in the future, but tried it locally and ended up back at a previous version with an unused guard variable whose only purpose was to be destructed, which I didn’t like.

Also tried reducing the nested controller/handler to a simple ScopedCoinsViewCache, but this would expose too much of the underlying CCoinsViewCache: your current solution is better.

sipa commented at 9:07 pm on January 27, 2026:

What about dropping CoinsViewCacheController, and moving Start to be a CCoinsViewCache member function (e.g. called CCoinsViewCache::CreateHandle()).

I don’t mind the verbosity of the Handle itself, but it seems overkill to create a whole new controller class for it + a separate file.

andrewtoth commented at 10:12 pm on January 27, 2026:

I had something like that initially in the parent PR, but it just called StopFetching instead of Reset #31132 (review). @l0rinc wdyt?

To me it doesn’t seem like we need the Handle to be a proxy and dereference to the cache if we are creating it from the cache itself in the same scope. We can just have it reset the cache when it goes out of scope. It’s just an unused RAII variable similar to holding a lock at the top of a scope.

l0rinc commented at 10:34 pm on January 27, 2026:

I’m not a fan of fake unused variables. If we accidentally convert this to a temporary guard (since the name isn’t referenced anywhere), destruction would happen immediately. For example, instead of:

0ScopeGuard guard([&]{ cache.Reset(); });

we could end up with:

0ScopeGuard([&]{ cache.Reset(); });

which destructs at the end of the statement. [[maybe_unused]] would probably address the warning, but the unused name still feels hacky to me.

That said, I won’t block this if you think it’s better than what we have now. I share the concern about adding new files and classes for every cleanup.

andrewtoth commented at 11:48 pm on January 27, 2026:

Updated #34164 (comment).

andrewtoth commented at 1:40 am on January 28, 2026:

@l0rinc calling CreateResetGuard incorrectly seems difficult. Doing any of the following results in a compiler warning that will fail CI:

0view.CreateResetGuard();
1CCoinsViewCache::ResetGuard(view.CreateResetGuard());
2CCoinsViewCache::ResetGuard{view.CreateResetGuard())};

warning: ignoring return value of ‘CCoinsViewCache::ResetGuard CCoinsViewCache::CreateResetGuard()’, declared with attribute ‘nodiscard’ [-Wunused-result]

l0rinc commented at 5:38 pm on January 26, 2026: contributor

I want to see if there’s a simpler way to do automatic cleanup via smart pointers, pushing my comments in the meantime.

l0rinc commented at 7:44 pm on January 26, 2026: contributor

ACK 3417bdbeab81a6aef9f827220ac58219f68d2732

Experimented with a few alternatives locally, the current implementation seems to be the best solution as far as I can tell. Left a few nits, will gladly reack if applied but I’m also fine with merging as-is.

andrewtoth force-pushed on Jan 27, 2026

DrahtBot added the label CI failed on Jan 27, 2026

andrewtoth commented at 11:47 pm on January 27, 2026: contributor

Thank you for your reviews @sipa, @l0rinc.

Changed the Handle to ResetGuard, and added a CCoinsViewCache::CreateResetGuard method. This is now used in validation. The CoinsViewCacheController is removed. Removed separate coinsviewcachecontroller_tests.cpp file and made a unit test for CreateResetGuard in coins_tests.cpp.

git diff 3417bdbeab81a6aef9f827220ac58219f68d2732..020a624a6995330828fb6a0f762621c6ff498d69

DrahtBot removed the label CI failed on Jan 28, 2026

in src/coins.h:491 in 45a388b153

482@@ -483,6 +483,25 @@ class CCoinsViewCache : public CCoinsViewBacked
483     //! Run an internal sanity check on the cache data structure. */
484     void SanityCheck() const;
485 
486+    class ResetGuard
487+    {
488+    private:
489+        friend CCoinsViewCache;
490+        CCoinsViewCache& m_cache;
491+        explicit ResetGuard(CCoinsViewCache& cache) noexcept : m_cache{cache} {}

sipa commented at 10:34 pm on January 28, 2026:

Want to make the cache argument here as LIFETIMEBOUND?

in src/coins.h:499 in 45a388b153

494+        ResetGuard(const ResetGuard&) = delete;
495+        ResetGuard& operator=(const ResetGuard&) = delete;
496+        ResetGuard(ResetGuard&&) = delete;
497+        ResetGuard& operator=(ResetGuard&&) = delete;
498+
499+        ~ResetGuard() noexcept { m_cache.Reset(); }

sipa commented at 10:35 pm on January 28, 2026:

Destructors are noexcept by default. Doesn’t hurt to add it, though.

sipa commented at 10:38 pm on January 28, 2026: member

Code review ACK 020a624a6995330828fb6a0f762621c6ff498d69. Only nits.

DrahtBot requested review from l0rinc on Jan 28, 2026

andrewtoth force-pushed on Jan 29, 2026

DrahtBot added the label CI failed on Jan 29, 2026

andrewtoth commented at 3:21 am on January 29, 2026: contributor

Thank you @sipa. I’ve addressed your nits.

git diff 020a624a6995330828fb6a0f762621c6ff498d69..b52435c25b08850c71d299ab63e4f2fd305fa0ef

DrahtBot removed the label CI failed on Jan 29, 2026

in src/test/fuzz/coins_view.cpp:89 in 905effc21b outdated

84@@ -85,6 +85,18 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend
85                 if (is_db && best_block.IsNull()) best_block = uint256::ONE;
86                 coins_view_cache.SetBestBlock(best_block);
87             },
88+            [&] {
89+                coins_view_cache.Reset();

sedited commented at 9:48 am on January 29, 2026:

The tests are good, but we are now only testing a member function that does not get called directly in production code. How about making Reset protected and using the RAII guard here directly:

 0diff --git a/src/coins.h b/src/coins.h
 1index e8af03e2c8..c4f8dbed92 100644
 2--- a/src/coins.h
 3+++ b/src/coins.h
 4@@ -379,0 +380,6 @@ protected:
 5+    /**
 6+     * Discard all modifications made to this cache without flushing to the base view.
 7+     * This can be used to efficiently reuse a cache instance across multiple operations.
 8+     */
 9+    void Reset() noexcept;
10+
11@@ -456,6 +461,0 @@ public:
12-    /**
13-     * Discard all modifications made to this cache without flushing to the base view.
14-     * This can be used to efficiently reuse a cache instance across multiple operations.
15-     */
16-    void Reset() noexcept;
17-
18diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
19index 2d061bae54..bf474498fd 100644
20--- a/src/test/coins_tests.cpp
21+++ b/src/test/coins_tests.cpp
22@@ -1148 +1148,3 @@ BOOST_AUTO_TEST_CASE(ccoins_reset)
23-        cache.Reset();
24+        {
25+            auto reset = cache.CreateResetGuard();
26+        }
27diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp
28index bddc34eb10..e23928083f 100644
29--- a/src/test/fuzz/coins_view.cpp
30+++ b/src/test/fuzz/coins_view.cpp
31@@ -89 +89,3 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend
32-                coins_view_cache.Reset();
33+                {
34+                    auto reset{coins_view_cache.CreateResetGuard()};
35+                }
36diff --git a/src/test/fuzz/coinscache_sim.cpp b/src/test/fuzz/coinscache_sim.cpp
37index db68dbc57f..abe7bd9af0 100644
38--- a/src/test/fuzz/coinscache_sim.cpp
39+++ b/src/test/fuzz/coinscache_sim.cpp
40@@ -407 +407,3 @@ FUZZ_TARGET(coinscache_sim)
41-                caches.back()->Reset();
42+                {
43+                    auto res{caches.back()->CreateResetGuard()};
44+                }

andrewtoth commented at 3:08 pm on January 29, 2026:

Done, added you as co-author.

in src/validation.cpp:3103 in 523dd8ed2e outdated

3098@@ -3098,7 +3099,8 @@ bool Chainstate::ConnectTip(
3099     LogDebug(BCLog::BENCH, "  - Load block from disk: %.2fms\n",
3100              Ticks<MillisecondsDouble>(time_2 - time_1));
3101     {
3102-        CCoinsViewCache view(&CoinsTip());
3103+        CCoinsViewCache& view{*m_coins_views->m_connect_block_view};
3104+        const auto reset_guard{view.CreateResetGuard()};

sedited commented at 9:51 am on January 29, 2026:

Just a comment: It would be nice if we could bind access to the view with the guard itself without having to initialize it manually. I tried coming up with something, but it wasn’t really nice either. Not super pressing this, since this is easily verified manually.

andrewtoth commented at 2:53 pm on January 29, 2026:

Indeed, a previous iteration had a CoinsViewCacheController which owned the cache privately. Calling a start method would return a handle which dereferenced to the owned cache, and would reset the cache when the handle was destroyed.

This was deemed too verbose by other reviewers as well.

in src/coins.h:459 in 905effc21b outdated

451@@ -452,6 +452,12 @@ class CCoinsViewCache : public CCoinsViewBacked
452      */
453     void Sync();
454 
455+    /**
456+     * Discard all modifications made to this cache without flushing to the base view.
457+     * This can be used to efficiently reuse a cache instance across multiple operations.
458+     */
459+    void Reset() noexcept;

sedited commented at 9:59 am on January 29, 2026:

Is there a good reason for marking this noexcept?

andrewtoth commented at 2:52 pm on January 29, 2026:

Since this is being called in a destructor, throwing would call terminate which we don’t want to do intentionally. Marking this as noexcept will emit a compiler warning telling us that which will fail CI.

0/home/user/workspace/bitcoin/src/coins.cpp: In member function ‘void CCoinsViewCache::Reset()’:
1/home/user/workspace/bitcoin/src/coins.cpp:283:5: warning: ‘throw’ will always call ‘terminate’ [-Wterminate]
2  283 |     throw std::logic_error("test");
3      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Removing noexcept will not emit this warning.

sedited commented at 10:03 am on January 29, 2026: contributor

Concept ACK

coins: add Reset on CCoinsViewCache

Add a Reset() method to CCoinsViewCache that clears cacheCoins,
cachedCoinsUsage, and hashBlock without flushing to the base view.

Co-authored-by: l0rinc <pap.lorinc@gmail.com>
Co-authored-by: sedited <seb.kung@gmail.com>

8dd9200fc9

coins: use hashBlock setter internally for CCoinsViewCache methods

Co-authored-by: l0rinc <pap.lorinc@gmail.com>

041758f5ed

coins: introduce CCoinsViewCache::ResetGuard

CCoinsViewCache::CreateResetGuard returns a guard that calls
Reset on the cache when the guard goes out of scope.
This RAII pattern ensures the cache is always properly reset
when it leaves current scope.

Co-authored-by: l0rinc <pap.lorinc@gmail.com>
Co-authored-by: sedited <seb.kung@gmail.com>

8fb6043231

validation: reuse same CCoinsViewCache for every ConnectBlock call

Add m_connect_block_view to ChainState's CoinsViews.
Call CreateResetGuard inside ConnectTip to ensure the view
is Reset after each block, avoiding repeated memory allocations.

Co-authored-by: l0rinc <pap.lorinc@gmail.com>

44b4ee194d

refactor: rename will_reuse_cache to reallocate_cache

More accurately reflects the purpose of the parameter, since
we will keep reusing the cache but don't want to reallocate it.

3e0fd0e4dd

andrewtoth force-pushed on Jan 29, 2026

andrewtoth commented at 2:49 pm on January 29, 2026: contributor

Thanks for the review @sedited.

Moved CCoinsViewCache::Reset() to protected, updated tests to use CreateResetGuard() instead of Reset(). This allowed removing a redundant Reset() unit test entirely.

git diff b52435c25b08850c71d299ab63e4f2fd305fa0ef..3e0fd0e4ddd894f0e7db1772f10ceaa1dddfb951

sedited approved

sedited commented at 3:47 pm on January 29, 2026: contributor

ACK 3e0fd0e4ddd894f0e7db1772f10ceaa1dddfb951

Would be good to get another pair of eyes.

DrahtBot requested review from sipa on Jan 29, 2026

in src/test/coins_tests.cpp:1135 in 3e0fd0e4dd

1130+
1131+    CCoinsViewCache cache{&root};
1132+
1133+    const COutPoint outpoint{Txid::FromUint256(m_rng.rand256()), m_rng.rand32()};
1134+
1135+    const Coin coin{CTxOut{m_rng.randrange(10), CScript{} << m_rng.randbytes(CScriptBase::STATIC_SIZE + 1)}, 1, false};

l0rinc commented at 5:18 pm on January 29, 2026:

what’s the reason for m_rng.randbytes(CScriptBase::STATIC_SIZE + 1? It’s meant to make the script bigger than the prevector to make sure it’s stored in the heap - does that make sense in this case? Is it meant to make sure Coin::DynamicMemoryUsage() is non-empty? Yeah, now I understand, a comment would be welcome if you touch again.

l0rinc commented at 5:51 pm on January 29, 2026: contributor

ACK 3e0fd0e4ddd894f0e7db1772f10ceaa1dddfb951

Changes since my last review:

CoinsViewCacheController was reverted back to ResetGuard. We can investigate in a future PR whether we can make this reusable. I dislike that it’s basically a fake instantiation, feels hacky, but we already have similar ones like LOCK and ElapseSteady, I can get used to it.
Instead of the previous controller-test checking the state of the cache, we’re testing the reset_guard indirectly through the cache, checking that without flush nothing gets propagated. It validates all 3 values affected by Reset. I think it could use a few simple comments to delimit the different parts, similarly to the previous test, but it does make sense as it is, can be done in a potential follow-up. With ~ResetGuard() { /*m_cache.Reset();*/ } it correctly fails at the last 3 checks.

I left some comments and expressed my preference about the parts I don’t like, but overall this is the least invasive way to introduce this feature and we can extend and generalize it in the future if scoped exit is needed elsewhere. LGTM - ready to merge, thanks for considering the alternatives.

The reset assures that we’re leaving the cache at high-water mark across blocks, instead of recreating it every time and don’t force reallocation on it. It’s an improvement that gets us closer to the parallel input fetcher \:D/

l0rinc approved

achow101 commented at 10:39 pm on January 29, 2026: member

ACK 3e0fd0e4ddd894f0e7db1772f10ceaa1dddfb951

achow101 merged this on Jan 29, 2026

achow101 closed this on Jan 29, 2026

validation: add reusable coins view for ConnectBlock #34164

Code Coverage & Benchmarks

Reviews

Conflicts