coins: don't mutate main cache when connecting block

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

This is a slightly modified version of the first few commits 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.

When accessing coins via the CCoinsViewCache, methods like GetCoin can call FetchCoin which actually mutate cacheCoins internally to cache entries when they are pulled from the backing db. This is generally a performance improvement for single threaded access patterns, but it precludes us from accessing entries in a CCoinsViewCache from multiple threads without a lock.

Another aspect is that when we use the resettable CCoinsViewCache view backed by the main cache for use in ConnectBlock(), we will insert entries into the main cache even if the block is determined to be invalid. This is not the biggest concern, since an invalid block requires proof-of-work. But, an attacker could craft multiple invalid blocks to fill the main cache. This would make us Flush the cache more often than necessary. Obviously this would be very expensive to do on mainnet.

Introduce CoinsViewOverlay, a CCoinsViewCache subclass that reads coins without mutating the underlying cache via FetchCoin().

Add PeekCoin() to look up a Coin through a stack of CCoinsViewCache layers without populating parent caches. This prevents the main cache from caching inputs pulled from disk for a block that has not yet been fully validated. Once Flush() is called on the view, these inputs will be added as spent to coinsCache in the main cache via BatchWrite().

This is the foundation for async input fetching, where worker threads must not mutate shared state.

DrahtBot added the label UTXO Db and Indexes on Dec 28, 2025

DrahtBot commented at 8:32 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/34165.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	l0rinc, sipa, sedited, willcl-ark, vasild, ryanofsky

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:

#34132 (coins: drop error catcher, centralize fatal read handling by l0rinc)
#34124 (refactor: make CCoinsView a pure virtual interface by l0rinc)
#31132 (validation: fetch block inputs on parallel threads 3x faster IBD by andrewtoth)
#30342 (kernel, logging: Pass Logger instances to kernel objects by ryanofsky)

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/validation.cpp:2982 in 9739ce6b10

2978@@ -2978,7 +2979,7 @@ bool Chainstate::DisconnectTip(BlockValidationState& state, DisconnectedBlockTra
2979     // Apply the block atomically to the chain state.
2980     const auto time_start{SteadyClock::now()};
2981     {
2982-        CCoinsViewCache view(&CoinsTip());
2983+        CoinsViewCacheNonMutating view(&CoinsTip());

l0rinc commented at 1:01 pm on January 2, 2026:

I tried understanding why this was added to DisconnectTip, since every indication was that this is about block connection:

Only used in ConnectBlock to pass as an ephemeral view that can be reset if the block is invalid.

So how are the new tests passing if the implementation is completely broken? It seems we’re only testing CoinsViewCacheNonMutating in isolation but there are no tests added to check it’s actual integration.

andrewtoth commented at 3:00 pm on January 2, 2026:

This was an oopsie. Added CoinsViewCacheNonMutating to ConnectBlock and removed from DisconnectBlock.

l0rinc changes_requested

l0rinc commented at 1:28 pm on January 2, 2026: contributor

The implementation talks about block connection but the implementation was added to block disconnect. I don’t yet know if we should apply it there or not, but I find it very likely that we should do it for block connection. A lot of tests were added for testing the new cache in isolation and they all passed while it was applied to the wrong place as far as I can tell.

We should add a new integration test that actually checks that invalid blocks don’t pollute the underlying cache:

 0diff --git a/src/test/validation_chainstate_tests.cpp b/src/test/validation_chainstate_tests.cpp
 1--- a/src/test/validation_chainstate_tests.cpp	(revision 20f0b8304d089d641060f693d3284e69524fbcbe)
 2+++ b/src/test/validation_chainstate_tests.cpp	(date 1767360120004)
 3@@ -3,10 +3,12 @@
 4 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 5 //
 6 #include <chainparams.h>
 7+#include <consensus/amount.h>
 8 #include <consensus/validation.h>
 9 #include <node/kernel_notifications.h>
10 #include <random.h>
11 #include <rpc/blockchain.h>
12+#include <script/script.h>
13 #include <sync.h>
14 #include <test/util/chainstate.h>
15 #include <test/util/coins.h>
16@@ -63,6 +65,30 @@
17     }
18 }
19 
20+BOOST_FIXTURE_TEST_CASE(connect_tip_does_not_cache_inputs_on_failed_connect, TestChain100Setup)
21+{
22+    Chainstate& chainstate = Assert(m_node.chainman)->ActiveChainstate();
23+
24+    COutPoint outpoint;
25+    {
26+        LOCK(cs_main);
27+        outpoint = AddTestCoin(m_rng, chainstate.CoinsTip());
28+        chainstate.CoinsTip().Flush(/*will_reuse_cache=*/false);
29+    }
30+
31+    CMutableTransaction tx;
32+    tx.vin.emplace_back(outpoint);
33+    tx.vout.emplace_back(MAX_MONEY, CScript{} << OP_TRUE);
34+
35+    const auto tip{WITH_LOCK(cs_main, return chainstate.m_chain.Tip()->GetBlockHash())};
36+    CBlock block = CreateBlock({tx}, CScript{} << OP_TRUE, chainstate);
37+    Assert(m_node.chainman)->ProcessNewBlock(std::make_shared<CBlock>(block), true, true, nullptr);
38+
39+    LOCK(cs_main);
40+    BOOST_CHECK_EQUAL(tip, chainstate.m_chain.Tip()->GetBlockHash()); // block rejected
41+    BOOST_CHECK(!chainstate.CoinsTip().HaveCoinInCache(outpoint));    // input not cached
42+}
43+
44 //! Test UpdateTip behavior for both active and background chainstates.
45 //!
46 //! When run on the background chainstate, UpdateTip should do a subset
47diff --git a/src/validation.cpp b/src/validation.cpp
48--- a/src/validation.cpp	(revision 20f0b8304d089d641060f693d3284e69524fbcbe)
49+++ b/src/validation.cpp	(date 1767360154724)
50@@ -3102,7 +3102,7 @@
51     LogDebug(BCLog::BENCH, "  - Load block from disk: %.2fms\n",
52              Ticks<MillisecondsDouble>(time_2 - time_1));
53     {
54-        CCoinsViewCache view(&CoinsTip());
55+        CoinsViewCacheNonMutating view(&CoinsTip());
56         bool rv = ConnectBlock(*block_to_connect, state, pindexNew, view);
57         if (m_chainman.m_options.signals) {
58             m_chainman.m_options.signals->BlockChecked(block_to_connect, state);

Concept ACK (edited to remove the nack).

andrewtoth force-pushed on Jan 2, 2026

andrewtoth commented at 3:00 pm on January 2, 2026: contributor

Thank you @l0rinc, I have taken your integration test. I had unit tests for this behavior but the integration test is a great idea.

l0rinc referenced this in commit d3a7723aa8 on Jan 2, 2026

andrewtoth force-pushed on Jan 3, 2026

DrahtBot added the label CI failed on Jan 3, 2026

DrahtBot commented at 7:22 pm on January 3, 2026: contributor

🚧 At least one of the CI tasks failed. Task macOS native: https://github.com/bitcoin/bitcoin/actions/runs/20681569759/job/59376499348 LLM reason (✨ experimental): Blockchain tests aborted due to a DisconnectTip failure during tip/disconnect handling (BLOCK_FAILED_VALID), causing the CI run to fail.

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.

DrahtBot removed the label CI failed on Jan 3, 2026

in src/test/coinsviewcachenonmutating_tests.cpp:37 in 7a6e6160cb outdated

32+
33+    for (const auto i : std::views::iota(1, NUM_TXS)) {
34+        CMutableTransaction tx;
35+        Txid txid{Txid::FromUint256(uint256(i))};
36+        tx.vin.emplace_back(txid, 0);
37+        prevhash = tx.GetHash();

l0rinc commented at 8:49 pm on January 3, 2026:

This seems like a leftover.

andrewtoth force-pushed on Jan 3, 2026

in src/test/coinsviewcachenonmutating_tests.cpp:23 in 7a6e6160cb outdated

18+#include <cstring>
19+#include <ranges>
20+
21+BOOST_AUTO_TEST_SUITE(coinsviewcachenonmutating_tests)
22+
23+static CBlock CreateBlock() noexcept

l0rinc commented at 8:54 pm on January 3, 2026:

I understand that we want this to be realistic, but we could simplify considerably if we just returned the outpoints here:

 0static std::vector<COutPoint> CreateOutpoints() noexcept
 1{
 2    static constexpr uint32_t NUM_OUTPOINTS{100};
 3    std::vector<COutPoint> outpoints;
 4    outpoints.reserve(NUM_OUTPOINTS);
 5
 6    for (const auto i : std::views::iota(uint32_t{1}, NUM_OUTPOINTS + 1)) {
 7        outpoints.emplace_back(Txid::FromUint256(uint256(i)), /*nIn=*/0);
 8    }
 9
10    return outpoints;
11}

If you disagree, just resolve without a comment.

l0rinc approved

l0rinc commented at 8:59 pm on January 3, 2026: contributor

I’ll run a reindex-chainstate on these to see how they behave - as long as they don’t slow down anything, I would be fine with them.

andrewtoth force-pushed on Jan 4, 2026

l0rinc referenced this in commit 15c56be893 on Jan 4, 2026

in src/coinsviewcachenonmutating.h:22 in 7fab35c24d

17+{
18+private:
19+    CCoinsMap::iterator FetchCoin(const COutPoint& outpoint) const override
20+    {
21+        auto [ret, inserted]{cacheCoins.try_emplace(outpoint)};
22+        if (!inserted) return ret;

l0rinc commented at 7:21 pm on January 5, 2026:

Hmm, we’re we’re duplicating the FetchCoin implementation here - and also making it virtual which introduces a slight regression on the hot path. Could we extract only the part that changes? I think we can simplify it by adding a virtual GetCoinFromBase which either calls base->GetCoin(outpoint) (default impl) or in CoinsViewCacheNonMutating it could be overwritten with the new base->PeekCoin(outpoint). That way only the if (inserted) branch would pay for virtual dispatch and there would be no duplicated FetchCoin logic, something like:

 0diff --git a/src/coins.cpp b/src/coins.cpp
 1--- a/src/coins.cpp	(revision 85b20448640f8209e453019c3e1494c1ae229d74)
 2+++ b/src/coins.cpp	(revision c3b34ec796a3fd6e34e0dfb862c61bc298ba8d6f)
 3@@ -59,10 +59,15 @@
 4     return memusage::DynamicUsage(cacheCoins) + cachedCoinsUsage;
 5 }
 6 
 7+std::optional<Coin> CCoinsViewCache::GetCoinFromBase(const COutPoint& outpoint) const
 8+{
 9+    return base->GetCoin(outpoint);
10+}
11+
12 CCoinsMap::iterator CCoinsViewCache::FetchCoin(const COutPoint &outpoint) const {
13     const auto [ret, inserted] = cacheCoins.try_emplace(outpoint);
14     if (inserted) {
15-        if (auto coin{base->GetCoin(outpoint)}) {
16+        if (auto coin{GetCoinFromBase(outpoint)}) {
17             ret->second.coin = std::move(*coin);
18             cachedCoinsUsage += ret->second.coin.DynamicMemoryUsage();
19             if (ret->second.coin.IsSpent()) { // TODO GetCoin cannot return spent coins
20diff --git a/src/coins.h b/src/coins.h
21--- a/src/coins.h	(revision 85b20448640f8209e453019c3e1494c1ae229d74)
22+++ b/src/coins.h	(revision c3b34ec796a3fd6e34e0dfb862c61bc298ba8d6f)
23@@ -382,6 +382,8 @@
24     /* Cached dynamic memory usage for the inner Coin objects. */
25     mutable size_t cachedCoinsUsage{0};
26 
27+    virtual std::optional<Coin> GetCoinFromBase(const COutPoint& outpoint) const;
28+
29 public:
30     CCoinsViewCache(CCoinsView *baseIn, bool deterministic = false);
31 
32@@ -489,7 +491,7 @@
33      * [@note](/bitcoin-bitcoin/contributor/note/) this is marked const, but may actually append to `cacheCoins`, increasing
34      * memory usage.
35      */
36-    virtual CCoinsMap::iterator FetchCoin(const COutPoint &outpoint) const;
37+    CCoinsMap::iterator FetchCoin(const COutPoint &outpoint) const;
38 };
39 
40 //! Utility function to add all of a transaction's outputs to a cache.
41diff --git a/src/coinsviewcachenonmutating.h b/src/coinsviewcachenonmutating.h
42--- a/src/coinsviewcachenonmutating.h	(revision 85b20448640f8209e453019c3e1494c1ae229d74)
43+++ b/src/coinsviewcachenonmutating.h	(revision c3b34ec796a3fd6e34e0dfb862c61bc298ba8d6f)
44@@ -9,28 +9,16 @@
45 #include <primitives/transaction.h>
46 
47 /**
48- * CCoinsViewCache subclass that does not call GetCoin on base via FetchCoin, so base will not be mutated before Flush.
49+ * CCoinsViewCache subclass that does not call GetCoin on base, so base will not be mutated before Flush.
50  * Only used in ConnectBlock to pass as an ephemeral view that can be reset if the block is invalid.
51- * It provides the same interface as CCoinsViewCache, overriding the FetchCoin private method.
52+ * It provides the same interface as CCoinsViewCache, overriding GetCoinFromBase().
53  */
54 class CoinsViewCacheNonMutating : public CCoinsViewCache
55 {
56 private:
57-    CCoinsMap::iterator FetchCoin(const COutPoint& outpoint) const override
58+    std::optional<Coin> GetCoinFromBase(const COutPoint& outpoint) const override
59     {
60-        auto [ret, inserted]{cacheCoins.try_emplace(outpoint)};
61-        if (!inserted) return ret;
62-
63-        // TODO: Remove spent checks once we no longer return spent coins in coinscache_sim CoinsViewBottom.
64-        if (auto coin{base->PeekCoin(outpoint)}; coin && !coin->IsSpent()) {
65-            ret->second.coin = std::move(*coin);
66-        } else {
67-            cacheCoins.erase(ret);
68-            return cacheCoins.end();
69-        }
70-
71-        cachedCoinsUsage += ret->second.coin.DynamicMemoryUsage();
72-        return ret;
73+        return base->PeekCoin(outpoint);
74     }
75 
76 public:

andrewtoth commented at 3:40 pm on January 10, 2026:

Nice, done.

in src/validation.cpp:3102 in 7fab35c24d outdated

3098@@ -3098,7 +3099,7 @@ 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+        CoinsViewCacheNonMutating view(&CoinsTip());

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

here or in a follow-up we could add this to TestBlockValidity as well, where it would be even more relevant since it’s invoked with without PoW checks (so your PR arguments apply here even more)

in src/test/coinsviewoverlay_tests.cpp:92 in 7fab35c24d outdated

87+    for (const auto& tx : block.vtx) {
88+        for (const auto& in : tx->vin) {
89+            BOOST_CHECK(!main_cache.HaveCoinInCache(in.prevout));
90+        }
91+    }
92+}

l0rinc commented at 8:35 pm on January 5, 2026:

Checked the coverage of this and compared it with the suggested CCoinsViewCache::GetCoinFromBase - existing FetchCoin is covered that way. We could extend the coverage with HaveCoin()/GetCoin() as well, to check that they also do not populate the parent cache when reading inputs from disk, and that flushing an ephemeral CoinsViewCacheNonMutating layer propagates spentness to its parent:

 0diff --git a/src/test/coinsviewcachenonmutating_tests.cpp b/src/test/coinsviewcachenonmutating_tests.cpp
 1--- a/src/test/coinsviewcachenonmutating_tests.cpp	(revision c3b34ec796a3fd6e34e0dfb862c61bc298ba8d6f)
 2+++ b/src/test/coinsviewcachenonmutating_tests.cpp	(revision 3e8676d49ecf9e3c12cd641ce655e74f85f3a6e6)
 3@@ -82,6 +82,13 @@
 4     PopulateView(block, db);
 5     CCoinsViewCache main_cache{&db};
 6     CoinsViewCacheNonMutating view{&main_cache};
 7+    const auto& outpoint{block.vtx[1]->vin[0].prevout};
 8+
 9+    BOOST_CHECK(view.HaveCoin(outpoint));
10+    BOOST_CHECK(view.GetCoin(outpoint).has_value());
11+    BOOST_CHECK(!main_cache.HaveCoinInCache(outpoint));
12+
13     CheckCache(block, view);
14     // Check that no coins have been moved up to main cache from db
15     for (const auto& tx : block.vtx) {
16@@ -89,6 +96,12 @@
17             BOOST_CHECK(!main_cache.HaveCoinInCache(in.prevout));
18         }
19     }
20+
21+    view.SetBestBlock(uint256::ONE);
22+    BOOST_CHECK(view.SpendCoin(outpoint));
23+    view.Flush();
24+    BOOST_CHECK(!main_cache.PeekCoin(outpoint).has_value());
25 }
26 
27 BOOST_AUTO_TEST_CASE(fetch_inputs_from_cache)

l0rinc changes_requested

andrewtoth force-pushed on Jan 10, 2026

l0rinc commented at 0:53 am on January 11, 2026: contributor

Ran a reindex-chainstate, no measurable change compared to master: 👍

ACK 2e67c5b604d2cf2e56db6b5d2906391d22113941

in src/coins.cpp:43 in 725c750345 outdated

39@@ -38,6 +40,14 @@ void CCoinsViewBacked::BatchWrite(CoinsViewCacheCursor& cursor, const uint256& h
40 std::unique_ptr<CCoinsViewCursor> CCoinsViewBacked::Cursor() const { return base->Cursor(); }
41 size_t CCoinsViewBacked::EstimateSize() const { return base->EstimateSize(); }
42 
43+std::optional<Coin> CCoinsViewCache::PeekCoin(const COutPoint& outpoint) const

willcl-ark commented at 2:35 pm on January 23, 2026:

Do we want to add a CCoinsViewErrorCatcher overrid for PeekCoin as we do for GetCoin and HaveCoin or is this omitted intentionally?

andrewtoth commented at 4:44 pm on January 24, 2026:

Good catch :smile:! Added, thanks.

andrewtoth commented at 4:53 pm on January 24, 2026:

Note that CCoinsViewErrorCatcher will be removed in #34132. So whichever is merged first will have to rebase.

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

Not sure I understand why this was needed, the only way this has database access is through GetCoin:

0	frame [#0](/bitcoin-bitcoin/0/): 0x000000010381616c test_bitcoin`CCoinsViewDB::GetCoin(this=0x0000000109549fe0, outpoint=0x000000016dc24a6c) const at txdb.cpp:70:14
1    frame [#1](/bitcoin-bitcoin/1/): 0x000000010419da40 test_bitcoin`CCoinsView::PeekCoin(this=0x0000000109549fe0, outpoint=0x000000016dc24a6c) const at coins.cpp:17:93
2    frame [#2](/bitcoin-bitcoin/2/): 0x000000010419dea0 test_bitcoin`CCoinsViewBacked::PeekCoin(this=0x000000010954a028, outpoint=0x000000016dc24a6c) const at coins.cpp:36:18
3    frame [#3](/bitcoin-bitcoin/3/): 0x00000001041a3ee4 test_bitcoin`CCoinsViewErrorCatcher::PeekCoin(COutPoint const&) const::$_0::operator()(this=0x000000016dc23918) const at coins.cpp:413:87
4    frame [#4](/bitcoin-bitcoin/4/): 0x00000001041a12e0 test_bitcoin`std::__1::optional<Coin> ExecuteBackedWrapper<std::__1::optional<Coin>, CCoinsViewErrorCatcher::PeekCoin(COutPoint const&) const::$_0>(func=(unnamed class) @ 0x000000016dc23918, err_callbacks=size=0) at coins.cpp:387:16
5    frame [#5](/bitcoin-bitcoin/5/): 0x00000001041a1250 test_bitcoin`CCoinsViewErrorCatcher::PeekCoin(this=0x000000010954a028, outpoint=0x000000016dc24a6c) const at coins.cpp:413:12
6    frame [#6](/bitcoin-bitcoin/6/): 0x000000010419e2d0 test_bitcoin`CCoinsViewCache::PeekCoin(this=0x000000010954aae0, outpoint=0x000000016dc24a6c) const at coins.cpp:51:18
7    frame [#7](/bitcoin-bitcoin/7/): 0x0000000102640b38 test_bitcoin`CoinsViewCacheNonMutating::GetCoinFromBase(this=0x000000016dc251e8, outpoint=0x000000016dc24a6c) const at coinsviewcachenonmutating.h:22:22
8    frame [#8](/bitcoin-bitcoin/8/): 0x000000010419e8b8 test_bitcoin`CCoinsViewCache::FetchCoin(this=0x000000016dc251e8, outpoint=0x000000016dc24a6c) const at coins.cpp:73:23
9    frame [#9](/bitcoin-bitcoin/9/): 0x000000010419f994 test_bitcoin`CCoinsViewCache::HaveCoin(this=0x000000016dc251e8, outpoint=0x000000016dc24a6c) const at coins.cpp:189:36

and without this error catcher we’re basically getting the same:

0	frame [#0](/bitcoin-bitcoin/0/): 0x000000010447616c test_bitcoin`CCoinsViewDB::GetCoin(this=0x000000010a14aaf0, outpoint=0x000000016cfc4a6c) const at txdb.cpp:70:14
1    frame [#1](/bitcoin-bitcoin/1/): 0x0000000104dfda40 test_bitcoin`CCoinsView::PeekCoin(this=0x000000010a14aaf0, outpoint=0x000000016cfc4a6c) const at coins.cpp:19:12
2    frame [#2](/bitcoin-bitcoin/2/): 0x0000000104dfdea0 test_bitcoin`CCoinsViewBacked::PeekCoin(this=0x000000010a14ab38, outpoint=0x000000016cfc4a6c) const at coins.cpp:39:18
3    frame [#3](/bitcoin-bitcoin/3/): 0x0000000104dfe2d0 test_bitcoin`CCoinsViewCache::PeekCoin(this=0x0000000846008c00, outpoint=0x000000016cfc4a6c) const at coins.cpp:54:18
4    frame [#4](/bitcoin-bitcoin/4/): 0x00000001032a0b38 test_bitcoin`CoinsViewCacheNonMutating::GetCoinFromBase(this=0x000000016cfc51e8, outpoint=0x000000016cfc4a6c) const at coinsviewcachenonmutating.h:22:22
5    frame [#5](/bitcoin-bitcoin/5/): 0x0000000104dfe8b8 test_bitcoin`CCoinsViewCache::FetchCoin(this=0x000000016cfc51e8, outpoint=0x000000016cfc4a6c) const at coins.cpp:76:23
6    frame [#6](/bitcoin-bitcoin/6/): 0x0000000104dff994 test_bitcoin`CCoinsViewCache::HaveCoin(this=0x000000016cfc51e8, outpoint=0x000000016cfc4a6c) const at coins.cpp:192:36

(except that the frames are a lot cleaner)

andrewtoth commented at 0:02 am on January 26, 2026:

Ah true, it is not needed. So, reverted adding it. This way we won’t conflict with #34132.

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

Oh hmm it seems I misunderstood. From your frames @l0rinc it looks like we don’t go through the error catcher all the way down to CCoinsViewDB::GetCoin. So, if CCoinsViewDB::GetCoin throws there it will bubble up all the way to CCoinsViewCache::HaveCoin caller and not abort inside the catcher as intended. Does that make sense?

I will re-add the PeekCoin override in the error catcher.

l0rinc commented at 8:30 pm on January 26, 2026:

You’re right, we need it temporarily until https://github.com/bitcoin/bitcoin/pull/34132/changes#diff-cafbe1353eff6084b73fd3b6c3dee603e0827348fdd2fe12dfad1e01003a84edR69 migrates it to CCoinsViewDB - so we can add it here for safety and we can hopefully remove it in #34132 again. But even in that case I wouldn’t add noexcept to PeekCoin and GetCoinFromBase here - only after the database GetCoin is itself a noexcept (will add that to #34132).

andrewtoth commented at 5:07 am on January 27, 2026:

I’ve removed noexcept from PeekCoin but opted to keep it for GetCoinFromBase (see #34165 (review)).

in src/coinsviewcachenonmutating.h:15 in 2e67c5b604

10+
11+/**
12+ * CCoinsViewCache subclass that does not call GetCoin on base via GetCoinFromBase, so base will not be mutated before
13+ * Flush.
14+ * Only used in ConnectBlock to pass as an ephemeral view that can be reset if the block is invalid.
15+ * It provides the same interface as CCoinsViewCache, overriding the FetchCoin private method.

willcl-ark commented at 2:39 pm on January 23, 2026:

“overriding the GetCoinFromBase method.”?

willcl-ark commented at 2:50 pm on January 23, 2026: member

Nice carve-out from #31132.

Left a comment about whether we want to add a CoinsViewErrorCatcher wrapper for PeekCoin, but otherwise looks good to me.

andrewtoth force-pushed on Jan 24, 2026

andrewtoth commented at 4:45 pm on January 24, 2026: contributor

Thank you for your review @willcl-ark. Addressed #34165 (review) and #34165 (review), and added noexcept to PeekCoin.

git diff 2e67c5b604d2cf2e56db6b5d2906391d22113941..3a2cfbea1957f1eed6d97b6f8924f13100e11b1c

l0rinc commented at 8:40 pm on January 25, 2026: contributor

I have taken your integration test.

Was it removed since?

in src/coins.cpp:17 in 3a2cfbea19

13@@ -14,6 +14,7 @@ TRACEPOINT_SEMAPHORE(utxocache, spent);
14 TRACEPOINT_SEMAPHORE(utxocache, uncache);
15 
16 std::optional<Coin> CCoinsView::GetCoin(const COutPoint& outpoint) const { return std::nullopt; }
17+std::optional<Coin> CCoinsView::PeekCoin(const COutPoint& outpoint) const noexcept { return GetCoin(outpoint); }

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

I have mixed feelings about adding noexcept here, since DB reads can still throw dbwrapper_error - which should be fatal, but noexcept muddies it a bit since this can theoretically std::terminate before CCoinsViewErrorCatcher has a chance to react.

Instead of adding it only here (and not to the equivalent GetCoin), I suggest we add it only after #34132 is merged (I can add it there as a last commit if you agree it belongs there)

andrewtoth commented at 0:02 am on January 26, 2026:

Removed noexcept for PeekCoin.

in src/test/coinsviewoverlay_tests.cpp:108 in 3a2cfbea19 outdated

103+}
104+
105+BOOST_AUTO_TEST_CASE(fetch_inputs_from_cache)
106+{
107+    const auto block{CreateBlock()};
108+    CCoinsViewDB db{{.path = "", .cache_bytes = 1_MiB, .memory_only = true}, {}};

l0rinc commented at 9:00 pm on January 25, 2026:

would it make sense to have at least one that’s not in memory, just to have some variety?

andrewtoth commented at 0:08 am on January 26, 2026:

I’m not sure about this. These are unit tests specific to the CoinsViewCacheNonMutating, not for the CCoinsViewDB. We should probably have unit tests specific to CCoinsViewDB and test them for in-memory and not.

in src/test/coinsviewoverlay_tests.cpp:41 in 3a2cfbea19 outdated

36+    }
37+
38+    return block;
39+}
40+
41+void PopulateView(const CBlock& block, CCoinsView& view, bool spent = false)

l0rinc commented at 9:01 pm on January 25, 2026:

nit: these helpers could go to anonymous namespace to avoid global linkage in the test binary.

in src/coinsviewcachenonmutating.h:15 in 3a2cfbea19 outdated

10+
11+/**
12+ * CCoinsViewCache subclass that does not call GetCoin on base via GetCoinFromBase, so base will not be mutated before
13+ * Flush.
14+ * Only used in ConnectBlock to pass as an ephemeral view that can be reset if the block is invalid.
15+ * It provides the same interface as CCoinsViewCache, overriding the GetCoinFromBase private method.

l0rinc commented at 9:03 pm on January 25, 2026:

GetCoinFromBase is protected now: https://github.com/bitcoin/bitcoin/pull/34165/changes#diff-095ce1081a930998a10b37358fae5499ac47f8cb6f25f5df5d88e920a54e0341R386

l0rinc changes_requested

andrewtoth force-pushed on Jan 26, 2026

DrahtBot added the label CI failed on Jan 26, 2026

andrewtoth commented at 0:10 am on January 26, 2026: contributor

@l0rinc thanks for your review. I have taken most of your suggestions. I have added the integration test back as a separate commit.

git diff 3a2cfbea1957f1eed6d97b6f8924f13100e11b1c..8e074187a57220c596eb124091714302903a07cc

DrahtBot removed the label CI failed on Jan 26, 2026

in src/coinsviewcachenonmutating.h:20 in 8e074187a5 outdated

15+ * It provides the same interface as CCoinsViewCache, overriding the GetCoinFromBase protected method.
16+ */
17+class CoinsViewCacheNonMutating : public CCoinsViewCache
18+{
19+private:
20+    std::optional<Coin> GetCoinFromBase(const COutPoint& outpoint) const noexcept override

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

I think we should also remove the noexcept from this for the same reasons: #34165 (review)

andrewtoth commented at 5:08 pm on January 26, 2026:

Can you clarify the reasoning in the parent comment please?

since DB reads can still throw dbwrapper_error - which should be fatal, but noexcept muddies it a bit since this can theoretically std::terminate before CCoinsViewErrorCatcher has a chance to react.

I’m not sure I understand this. The db reads will be caught by the error catcher and we abort before returning here.

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

I might be completely wrong here, but based on previous frames:

0frame [#0](/bitcoin-bitcoin/0/): 0x000000010381616c test_bitcoin`CCoinsViewDB::GetCoin(this=0x0000000109549fe0, outpoint=0x000000016dc24a6c) const at txdb.cpp:70:14
1frame [#1](/bitcoin-bitcoin/1/): 0x000000010419da40 test_bitcoin`CCoinsView::PeekCoin(this=0x0000000109549fe0, outpoint=0x000000016dc24a6c) const at coins.cpp:17:93
2frame [#2](/bitcoin-bitcoin/2/): 0x000000010419dea0 test_bitcoin`CCoinsViewBacked::PeekCoin(this=0x000000010954a028, outpoint=0x000000016dc24a6c) const at coins.cpp:36:18
3frame [#3](/bitcoin-bitcoin/3/): 0x00000001041a3ee4 test_bitcoin`CCoinsViewErrorCatcher::PeekCoin(COutPoint const&) const::$_0::operator()(this=0x000000016dc23918) const at coins.cpp:413:87
4frame [#4](/bitcoin-bitcoin/4/): 0x00000001041a12e0 test_bitcoin`std::__1::optional<Coin> ExecuteBackedWrapper<std::__1::optional<Coin>, CCoinsViewErrorCatcher::PeekCoin(COutPoint const&) const::$_0>(func=(unnamed class) @ 0x000000016dc23918, err_callbacks=size=0) at coins.cpp:387:16
5frame [#5](/bitcoin-bitcoin/5/): 0x00000001041a1250 test_bitcoin`CCoinsViewErrorCatcher::PeekCoin(this=0x000000010954a028, outpoint=0x000000016dc24a6c) const at coins.cpp:413:12
6frame [#6](/bitcoin-bitcoin/6/): 0x000000010419e2d0 test_bitcoin`CCoinsViewCache::PeekCoin(this=0x000000010954aae0, outpoint=0x000000016dc24a6c) const at coins.cpp:51:18

it seemed to me the error catcher can be earlier in the stack (not sure it applies to GetCoinFromBase as well), so it cannot catch calls that convert the exceptions to std::terminate on later child-frames - but if it doesn’t, please ignore this.

andrewtoth commented at 5:06 am on January 27, 2026:

From the frames you are showing, CCoinsViewCache::PeekCoin goes through CCoinsViewErrorCatcher::PeekCoin which will catch whatever is thrown in CCoinsViewDB::GetCoin. GetCoinFromBase is only available in CCoinsViewCache, which calls through CCoinsViewErrorCatcher for both GetCoin and PeekCoin and will therefore never throw an exception.

in src/coinsviewcachenonmutating.h:12 in 8e074187a5

 7+
 8+#include <coins.h>
 9+#include <primitives/transaction.h>
10+
11+/**
12+ * CCoinsViewCache subclass that does not call GetCoin on base via GetCoinFromBase, so base will not be mutated before

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

Is it fair to state that GetCoin is not called when in fact what we’re avoiding is FetchCoin https://github.com/bitcoin/bitcoin/blob/1cc58d3a0c653ac30df04d1010a3cf84c6bc307a/src/coins.cpp#L52-L53

But we are calling GetCoin from PeekCoin, see:

0  frame [#0](/bitcoin-bitcoin/0/): 0x000000010447616c test_bitcoin`CCoinsViewDB::GetCoin(this=0x000000010a14aaf0, outpoint=0x000000016cfc4a6c) const at txdb.cpp:70:14                                                  
1  frame [#1](/bitcoin-bitcoin/1/): 0x0000000104dfda40 test_bitcoin`CCoinsView::PeekCoin(this=0x000000010a14aaf0, outpoint=0x000000016cfc4a6c) const at coins.cpp:19:12                                                  
2  frame [#2](/bitcoin-bitcoin/2/): 0x0000000104dfdea0 test_bitcoin`CCoinsViewBacked::PeekCoin(this=0x000000010a14ab38, outpoint=0x000000016cfc4a6c) const at coins.cpp:39:18                                            
3  frame [#3](/bitcoin-bitcoin/3/): 0x0000000104dfe2d0 test_bitcoin`CCoinsViewCache::PeekCoin(this=0x0000000846008c00, outpoint=0x000000016cfc4a6c) const at coins.cpp:54:18

0 * CCoinsViewCache subclass that uses PeekCoin instead of GetCoin when fetching from base, avoiding FetchCoin mutation on parent cache layers.

(obviously a few other changes are needed besides this line, if you agree with my observation)

andrewtoth commented at 5:09 pm on January 26, 2026:

That comment update is an improvement. What other changes are needed?

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

Can you please check PR description and commit messages? I have just finished the reviews, can only do it myself tomorrow.

andrewtoth commented at 5:56 pm on January 26, 2026:

They all look ok to me, they mention not calling FetchCoin instead of GetCoin.

l0rinc commented at 6:08 pm on January 26, 2026:

the const methods like GetCoin actually mutate cacheCoins internally

I think these should also be updated, since it not all GetCoin implementations that are doing that, e.g. the database GetCoin doesn’t, which is what PeekCoin is calling

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

I think that sentence is pretty clear that it is describing the behavior of CCoinsViewCache and not CCoinsView subclasses in general. What would you prefer it gets changed to?

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

It’s just that GetCoin isn’t necessarily a problem, the eventual FetchCoin call is:

0-When accessing coins via the CCoinsViewCache, the const methods like GetCoin actually mutate cacheCoins internally to cache entries when they are pulled from the backing db.
1+When accessing coins via the CCoinsViewCache, methods like GetCoin can call FetchCoin which actually mutate cacheCoins internally to cache entries when they are pulled from the backing db.

andrewtoth commented at 5:04 am on January 27, 2026:

Done.

l0rinc changes_requested

l0rinc commented at 4:47 pm on January 26, 2026: contributor

Thanks for fixing it so quickly. I think we should remove the noexcept from GetCoinFromBase as well because the underlying PeekCoin -> GetCoin path can still throw. And since PeekCoin does actually call GetCoin, I think we need to update the PR description and comments and commit messages to accurately reflect that we’re using PeekCoin to avoid FetchCoin’s try_emplace on the parent.

andrewtoth force-pushed on Jan 27, 2026

andrewtoth commented at 5:03 am on January 27, 2026: contributor

Thanks again for your review @l0rinc. I took most of your suggestions. I also re-added CCoinsViewErrorCatcher::PeekCoin (thanks @willcl-ark).

git diff 8e074187a57220c596eb124091714302903a07cc..2f100b16b7bb09572ed3b04aa1484523af7509d9

l0rinc commented at 9:22 am on January 27, 2026: contributor

ACK 2f100b16b7bb09572ed3b04aa1484523af7509d9

Integration test was added to validate on a higher level that rejected blocks don’t cache, error catcher (temporarily?) extended with PeekCoin, doc updated to clarify that FetchCoin fills the cache, and test helpers are hidden behind anonymous namespace to avoid pollution.

andrewtoth force-pushed on Jan 29, 2026

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

Rebased due to #34164. Make m_connect_block_view be a CoinsViewCacheNonMutating instead of creating an ephemeral CoinsViewCacheNonMutating.

git range-diff ab233255d444ccf6ffe4a45cb02bfc3e5fb71bdb..2f100b16b7bb09572ed3b04aa1484523af7509d9 6750744eb32da428b0a44c6eaab69e5741a25167..0e45e759e0197f01b82689298a164d442264fc8a

andrewtoth force-pushed on Jan 29, 2026

DrahtBot added the label CI failed on Jan 29, 2026

DrahtBot commented at 11:40 pm on January 29, 2026: contributor

🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/21498245018/job/61938643008 LLM reason (✨ experimental): Clang-tidy failed due to a bugprone-argument-comment error in validation_chainstate_tests.cpp (commented argument name mismatches the function parameter).

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.

DrahtBot removed the label CI failed on Jan 30, 2026

in src/validation.h:493 in a83ff982c7

491@@ -491,7 +492,7 @@ class CoinsViews {
492 
493     //! Temporary CCoinsViewCache layered on top of m_cacheview and passed to ConnectBlock().

l0rinc commented at 9:34 am on January 30, 2026:

Nit: since we’ve changed it from CCoinsViewCache to CoinsViewCacheNonMutating, it doesn’t sound right to call this by its parent class. And in terms of object lifetime it’s not exactly temporary.

0    //! Reused CoinsViewCacheNonMutating layered on top of m_cacheview and passed to ConnectBlock().

in src/test/validation_chainstate_tests.cpp:1 in 77b1f5a3a3 outdated

l0rinc commented at 9:35 am on January 30, 2026:

77b1f5a3a3bba44651aa5da5926a37d6eca9e863 nit: “does indeed not mutate” sounds a bit weird in the commit message

l0rinc commented at 4:15 pm on February 4, 2026:

9df24c2a635087037100b7b93828ca34d3f979d2

This is a slightly modified version of the first two commits of #31132,

This commit necessitates some PR description updates I think

l0rinc commented at 11:06 pm on February 15, 2026:

0            [&]() { // PeekCoin/GetCoin

l0rinc approved

l0rinc commented at 9:42 am on January 30, 2026: contributor

ACK 77b1f5a3a3bba44651aa5da5926a37d6eca9e863

The rebase since my last ack reordered the code a bit to work with the reusable m_connect_block_view. I have re-reviewed every commit, left small nits, LGTM!

in src/test/validation_chainstate_tests.cpp:85 in 77b1f5a3a3

80+    tx.vin.emplace_back(outpoint);
81+    tx.vout.emplace_back(MAX_MONEY, CScript{} << OP_TRUE);
82+
83+    const auto tip{WITH_LOCK(cs_main, return chainstate.m_chain.Tip()->GetBlockHash())};
84+    const CBlock block{CreateBlock({tx}, CScript{} << OP_TRUE, chainstate)};
85+    Assert(m_node.chainman)->ProcessNewBlock(std::make_shared<CBlock>(block), true, true, nullptr);

sedited commented at 12:39 pm on January 31, 2026:

I think it would be good to wrap this in a BOOST_CHECK to ensure that the block was processed correctly (or rather accepted), but is rejected later.

in src/coinsviewcachenonmutating.h:17 in a83ff982c7 outdated

12+ * CCoinsViewCache subclass that uses PeekCoin instead of GetCoin when fetching from base, avoiding FetchCoin mutation
13+ * on parent cache layers.
14+ * Only used in ConnectBlock to pass as an ephemeral view that can be reset if the block is invalid.
15+ * It provides the same interface as CCoinsViewCache, overriding the GetCoinFromBase protected method.
16+ */
17+class CoinsViewCacheNonMutating : public CCoinsViewCache

sedited commented at 12:48 pm on January 31, 2026:

Nit: This name feels a bit misleading, since we can still flush from it among other things, but I don’t have a better option either. Maybe expand the comment a bit to clarify that it still populates its own cache and can flush its state back?

andrewtoth commented at 8:29 pm on January 31, 2026:

I updated the name to CoinsViewOverlay. This will also work with the parent PR #31132 so we don’t have to rename it there.

in src/test/coinsviewoverlay_tests.cpp:139 in a83ff982c7 outdated

134+    PopulateView(block, main_cache, /*spent=*/true);
135+    CoinsViewCacheNonMutating view{&main_cache};
136+    for (const auto& tx : block.vtx) {
137+        for (const auto& in : tx->vin) {
138+            const auto& c{view.AccessCoin(in.prevout)};
139+            BOOST_CHECK(c.IsSpent());

sedited commented at 1:40 pm on January 31, 2026:

Nit: How about checking that HaveCoin and GetCoin also return nothing here (and same with the other test below)?

sedited commented at 1:43 pm on January 31, 2026: contributor

Approach ACK

Not wasting cache space for a potentially invalid block seems like another nice improvement in of itself.

andrewtoth force-pushed on Jan 31, 2026

DrahtBot added the label CI failed on Jan 31, 2026

andrewtoth commented at 8:37 pm on January 31, 2026: contributor

Thank you for the reviews @sedited and @l0rinc. I have taken all of your suggestions.

Renamed CoinsViewCacheNonMutating to CoinsViewOverlay and updated the docstring.
Updated docstring on m_connect_block_view.
Updated text in commit message.
Added more BOOST_CHECKs in tests.
Added CoinsViewOverlay fuzz target.

git diff 77b1f5a3a3bba44651aa5da5926a37d6eca9e863..b798ecb2f2a50a180fe0e79fc8ac84ab6da02c40

l0rinc commented at 8:41 pm on January 31, 2026: contributor

ACK b798ecb2f2a50a180fe0e79fc8ac84ab6da02c40

Since last ack changed all *Mutating suffixed to *Overlay (in filenames, code, PR description and commit messages), added HaveCoin and GetCoin assert to the new tests and asserted the outcome of ProcessNewBlock in test.

DrahtBot requested review from sedited on Jan 31, 2026

in src/coinsviewoverlay.h:23 in b798ecb2f2

18+ * on success, so invalid blocks don't pollute the underlying cache.
19+ */
20+class CoinsViewOverlay : public CCoinsViewCache
21+{
22+private:
23+    std::optional<Coin> GetCoinFromBase(const COutPoint& outpoint) const noexcept override

sedited commented at 8:48 pm on January 31, 2026:

Meant to comment this in the earlier review: Are we sure we don’t want this function to allow to throw? Or is this still left over from when PeekCoin was noexcept before?

l0rinc commented at 8:59 pm on January 31, 2026:

#34165 (review)

sedited commented at 9:41 pm on January 31, 2026:

Mmh, I still feel like it is bit of an anti-pattern. In my mind a CCoinsViewCache instantiation should not necessarily know that its base class’ GetCoin member function is not allowed to throw either (though I realize in practice it doesn’t).

l0rinc commented at 9:51 pm on January 31, 2026:

Isn’t that the point of such a specifier that we explicitly promise something that would be hard to deduce otherwise?

sedited commented at 9:56 pm on January 31, 2026:

I would say so in some circumstances, but in a virtual class that has its member functions commonly overridden by other components that seems like an anti-pattern.

DrahtBot requested review from sedited on Jan 31, 2026

DrahtBot removed the label CI failed on Jan 31, 2026

andrewtoth force-pushed on Jan 31, 2026

andrewtoth commented at 10:08 pm on January 31, 2026: contributor

Thanks again @sedited and @l0rinc. I removed noexcept from GetCoinFromBase.

git diff b798ecb2f2a50a180fe0e79fc8ac84ab6da02c40..2f9b52fa348242db475f4aff1fee951fc74e934e

l0rinc commented at 10:11 pm on January 31, 2026: contributor

ACK 2f9b52fa348242db475f4aff1fee951fc74e934e

sedited requested review from sipa on Feb 1, 2026

in src/coinsviewoverlay.h:20 in e91c8a2b84

15+ * so intermediate CCoinsViewCache layers are not filled.
16+ *
17+ * Used during ConnectBlock() as an ephemeral, resettable top-level view that is flushed only
18+ * on success, so invalid blocks don't pollute the underlying cache.
19+ */
20+class CoinsViewOverlay : public CCoinsViewCache

sipa commented at 1:28 pm on February 2, 2026:

In commit “coins: don’t mutate main cache when connecting block”

Nit: just put this in coins.h.

in src/test/fuzz/coins_view.cpp:337 in 2f9b52fa34

332+FUZZ_TARGET(coins_view_async, .init = initialize_coins_view)
333+{
334+    FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
335+    CCoinsView backend_coins_view;
336+    CoinsViewOverlay coins_view_cache{&backend_coins_view, /*determinsistic=*/true};
337+    TestCoinsView(fuzzed_data_provider, coins_view_cache, backend_coins_view, /*is_db=*/false);

sipa commented at 1:38 pm on February 2, 2026:

In commit “fuzz: add target for CoinsViewOverlay”

Can this test verify that CoinsViewOverlay operations indeed do not modify the parent cache?

andrewtoth commented at 11:50 pm on February 3, 2026:

Added MutationGuardCoinsViewCache subclass in the fuzz target. Now we use that as the middle layer and CoinsViewOverlay at the top. The mutation guard asserts in BatchWrite that cacheCoins has not been mutated since the last call to BatchWrite (or empty initial state). So, if the fuzzer manages to mutate cacheCoins in the middle layer by some other method then calls Flush or Sync on the overlay it will fail the assertion.

andrewtoth commented at 2:30 pm on February 4, 2026:

Thanks for this idea. I’ve fuzzed it overnight (~50 million iterations) with no issues. The parent PR (#31132 which I fuzzed for weeks) verified no mutation via thread sanitizer. Since it was multithreaded, it would break if there was multithreaded mutation of the backing cacheCoins. But, since it was multithreaded it was not a deterministic check. This is a great improvement!

andrewtoth force-pushed on Feb 3, 2026

andrewtoth commented at 11:53 pm on February 3, 2026: contributor

Thank you for your review @sipa. I’ve taken your suggestions.

Moved CoinsViewOverlay to coins.h instead of its own file.
Use a CCoinsViewCache subclass as a middle layer in the CoinsViewOverlay fuzz target. The subclass MutationGuardCoinsViewCache asserts that nothing mutates the internal cacheCoins except calls to Flush and Sync. Had to add some try/catch blocks to handle CCoinsViewCache as the backing layer and move code that directly mutates the backing layer.

git diff 2f9b52fa348242db475f4aff1fee951fc74e934e..2790e6e71f62eb0a7a5f239d1f8ea933be526d88

in src/test/fuzz/coins_view.cpp:66 in 2790e6e71f

61+            s.coin = entry.coin;
62+            snapshot.push_back(std::move(s));
63+        }
64+
65+        std::sort(snapshot.begin(), snapshot.end(),
66+                  [](const CacheCoinSnapshot& a, const CacheCoinSnapshot& b) { return a.outpoint < b.outpoint; });

l0rinc commented at 2:42 pm on February 4, 2026:

We could use std::ranges and maybe even use a projection to extract the key:

0        std::ranges::sort(snapshot, {}, &CacheCoinSnapshot::outpoint);

in src/test/fuzz/coins_view.cpp:62 in 2790e6e71f

57+            CacheCoinSnapshot s;
58+            s.outpoint = outpoint;
59+            s.dirty = entry.IsDirty();
60+            s.fresh = entry.IsFresh();
61+            s.coin = entry.coin;
62+            snapshot.push_back(std::move(s));

l0rinc commented at 2:45 pm on February 4, 2026:

could we emplace it instead?

0            snapshot.emplace_back(outpoint, entry.IsDirty(), entry.IsFresh(), entry.coin);

in src/test/fuzz/coins_view.cpp:130 in 2790e6e71f

123@@ -73,10 +124,24 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsViewCache& co
124                 }
125             },
126             [&] {
127-                coins_view_cache.Flush(/*reallocate_cache=*/fuzzed_data_provider.ConsumeBool());
128+                try {
129+                    coins_view_cache.Flush(/*reallocate_cache=*/fuzzed_data_provider.ConsumeBool());
130+                } catch (const std::logic_error& e) {
131+                    assert(e.what() == std::string{"FRESH flag misapplied to coin that exists in parent cache"});

l0rinc commented at 3:41 pm on February 4, 2026:

This is a bit concerning, the fuzzer shouldn’t be able to trigger this logic error. Would it help if #33018 cleanup landed first, would that simplify this change? And regardless of the merge order, can we instead make the fuzzer avoid constructing invalid states in the first place and then drop the try/catches (e.g. by setting possible_overwrite properly based on HaveCoinInCache in AddCoin)?

andrewtoth-exo commented at 4:50 pm on February 4, 2026:

I’m not sure #34124 or #33018 will fix this error. If we add a coin that doesn’t exist in the cache (with possible_overwrite being false), then it will add that coin as fresh. If we Flush, then add that same coin again to the cache it will be marked fresh again. Then the subsequent Flush will always throw this error. Production code relies on checking that any coin we are about to add does not exist in the utxo set. AddCoin just operates on the cache layer.

andrewtoth commented at 7:44 pm on February 4, 2026:

I keep the error caught in the backend cache’s BatchWrite, and don’t rethrow it.

in src/test/fuzz/coins_view.cpp:266 in 2790e6e71f

263-        assert(is_db == !!coins_view_cursor);
264+        try {
265+            std::unique_ptr<CCoinsViewCursor> coins_view_cursor = backend_coins_view.Cursor();
266+            assert(is_db == !!coins_view_cursor);
267+        } catch (const std::logic_error&) {
268+            assert(dynamic_cast<CCoinsViewCache*>(&backend_coins_view));

l0rinc commented at 3:46 pm on February 4, 2026:

isn’t this a workaround that https://github.com/bitcoin/bitcoin/pull/34124/changes#diff-095ce1081a930998a10b37358fae5499ac47f8cb6f25f5df5d88e920a54e0341R357 would already fix? Would it help if that landed before this? I’m not sure why it’s acceptable to call the unimplemented method if it’s a CCoinsViewCache with default methods - we shouldn’t have that after #34124 as far as I can tell. What if we only checked this for is_db directly:

0  if (is_db) {                                                                                                                                                                                   
1      std::unique_ptr<CCoinsViewCursor> coins_view_cursor{backend_coins_view.Cursor()};
2      assert(!!coins_view_cursor);
3  }

andrewtoth-exo commented at 4:56 pm on February 4, 2026:

Yeah I’m not sure this check adds much to the test. Just gating it on is_db seems acceptable to me.

in src/test/fuzz/coins_view.cpp:79 in 2790e6e71f

74+    {
75+        // Nothing must modify cacheCoins other than BatchWrite.
76+        assert(ComputeCacheCoinsSnapshot() == m_expected_snapshot);
77+        try {
78+            CCoinsViewCache::BatchWrite(cursor, block_hash);
79+            m_expected_snapshot = ComputeCacheCoinsSnapshot();

l0rinc commented at 3:49 pm on February 4, 2026:

could we assert what a new snapshot contains after the flush? Assert the cache is empty after flush for will_erase=true and maybe add a auto m_expected_snapshot_before{ComputeCacheCoinsSnapshot()}; or check that no fresh or dirty remain, that no spent ones were written and removed, etc.

Though I’d add that to the general https://github.com/bitcoin/bitcoin/blob/d4bc620ad8cf6b7975be55d5ae27dc943cceef3f/src/test/fuzz/coins_view.cpp#L165-L170, not here. Though looking at it I’m not sure why /*will_erase=*/false is never tested there, could we do /*will_erase=*/fuzzed_data_provider.ConsumeBool(); instead? But I understand if you think that’s outside the scope of this PR (we could still fuzz it locally without committing to make sure the current change is correct).

andrewtoth-exo commented at 5:01 pm on February 4, 2026:

The MutationGuardCoinsViewCache does not flush. The CoinsViewOverlay calls Flush, which calls MutationGuardCoinsViewCache::BatchWrite.

The next code snippet calls BatchWrite on the CoinsViewOverlay. Making will_erase a fuzzed data boolean makes sense. I wonder why it was hard coded to false?

andrewtoth commented at 7:44 pm on February 4, 2026:

Added a new commit that makes will_erase a fuzz provided boolean.

in src/test/fuzz/coins_view.cpp:180 in e42b1fd6bc outdated

176@@ -177,31 +177,6 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsViewCache& co
177             });
178     }
179 
180-    {

l0rinc commented at 4:18 pm on February 4, 2026:

e42b1fd6bcf092634c2d5e73334277392c93c0d7 Seems like a clean move-only commit, might want to prefix the message with move-only

andrewtoth-exo commented at 5:02 pm on February 4, 2026:

Err, but it updates behavior in that these checks are now done after some other checks. The order of checks change. Some of the checks that were done after and now are done before can mutate the state of the cache, so there is some difference.

l0rinc changes_requested

l0rinc commented at 4:32 pm on February 4, 2026: contributor

Most of the change seems fine to me, but I don’t like the new fuzzers as they are, we’re testing more invalid states now. Instead of catching std::logic_error we should investigate where our setup wasn’t realistic and fix that instead of just yolo-mutating the state in ways that the production code cannot. The tests should help increase our confidence in the code, catching std::logic_error won’t help with that. Landing #34124 and #33018 could probably help a bit in that regard.

andrewtoth force-pushed on Feb 4, 2026

andrewtoth commented at 8:07 pm on February 4, 2026: contributor

Thanks for your review @l0rinc - I’ve taken all your suggestions. I’m not sure #34124 or #33018 would change this PR. They seem like orthogonal improvements.

git diff 2790e6e71f62eb0a7a5f239d1f8ea933be526d88..70e93a10b7fad61483bc9044b483460a7faaa1b8

in src/test/fuzz/coins_view.cpp:82 in 70e93a10b7 outdated

77+            // There's not an easy way to prevent the fuzzer from reaching this, so we handle it here.
78+            // Since it is thrown in the middle of the write, we reset our own state and iterate through
79+            // the cursor so the caller's state is also reset.
80+            assert(e.what() == std::string{"FRESH flag misapplied to coin that exists in parent cache"});
81+            Reset();
82+            for (auto it{cursor.Begin()}; it != cursor.End(); it = cursor.NextAndMaybeErase(*it)) {}

l0rinc commented at 8:24 pm on February 4, 2026:

This is a bit weird, but we’re in a crisis recovery mode in this branch, I’m sure we’ll get back to this in follow-ups…

l0rinc commented at 11:17 pm on February 15, 2026:

Maybe we can replace the manual drain loop with a qualified call to the base implementation:

0            CCoinsView::BatchWrite(cursor, block_hash);

vasild commented at 3:35 pm on February 19, 2026:

Maybe we can replace the manual drain loop with a qualified call to the base implementation

👍

in src/coins.h:307 in 70e93a10b7 outdated

303@@ -304,6 +304,11 @@ class CCoinsView
304     //! Retrieve the Coin (unspent transaction output) for a given outpoint.
305     virtual std::optional<Coin> GetCoin(const COutPoint& outpoint) const;
306 
307+    //! Retrieve the Coin (unspent transaction output) for a given outpoint, without caching results.

l0rinc commented at 8:26 pm on February 4, 2026:

nit: we’re not usually this friendly :)

0    //! Retrieve the Coin for a given outpoint, without caching results.

andrewtoth commented at 9:06 pm on February 4, 2026:

This is copied from 3 lines up :)

l0rinc approved

l0rinc commented at 8:28 pm on February 4, 2026: contributor

lightly tested + code review ACK 70e93a10b7fad61483bc9044b483460a7faaa1b8

I couldn’t test the fuzz locally on a mac, but played around with the rest, LGTM. I hope we can clean up the fuzzer in the future to have even fewer invalid states, but this is a lot better than before.

in src/test/coins_tests.cpp:1162 in c40f6d743a outdated

1158@@ -1159,4 +1159,25 @@ BOOST_AUTO_TEST_CASE(ccoins_reset_guard)
1159     BOOST_CHECK(!root_cache.HaveCoinInCache(outpoint));
1160 }
1161 
1162+BOOST_AUTO_TEST_CASE(ccoins_peekcoin)

sipa commented at 3:33 am on February 9, 2026:

In commit “coins: add PeekCoin()”

There should be a way added to the coinscache_sim fuzz test that exercises PeekCoin:

 0diff --git a/src/test/fuzz/coinscache_sim.cpp b/src/test/fuzz/coinscache_sim.cpp
 1index 8c2d1678386..ec17a903907 100644
 2--- a/src/test/fuzz/coinscache_sim.cpp
 3+++ b/src/test/fuzz/coinscache_sim.cpp
 4@@ -252,12 +252,14 @@ FUZZ_TARGET(coinscache_sim)
 5         CallOneOf(
 6             provider,
 7 
 8-            [&]() { // GetCoin
 9+            [&]() { // GetCoin / PeekCoin
10                 uint32_t outpointidx = provider.ConsumeIntegralInRange<uint32_t>(0, NUM_OUTPOINTS - 1);
11                 // Look up in simulation data.
12                 auto sim = lookup(outpointidx);
13                 // Look up in real caches.
14-                auto realcoin = caches.back()->GetCoin(data.outpoints[outpointidx]);
15+                auto realcoin = provider.ConsumeBool() ?
16+                                caches.back()->PeekCoin(data.outpoints[outpointidx]) :
17+                                caches.back()->GetCoin(data.outpoints[outpointidx]);
18                 // Compare results.
19                 if (!sim.has_value()) {

andrewtoth commented at 3:26 pm on February 10, 2026:

Thanks @sipa. I think to not invalidate the ACKs here I will leave this change to a follow-up, unless you insist it needs to go in this PR.

sipa commented at 3:35 pm on February 10, 2026:

A follow-up is fine.

l0rinc commented at 4:35 pm on February 10, 2026:

@andrewtoth, can you try it locally before merge? I’m personally fine with a response of 👍 / 👎 (just to avoid surprises)

andrewtoth commented at 5:35 pm on February 10, 2026:

@l0rinc I applied the diff locally and fuzzed the target for half an hour (~1.7 million iterations). No issues :+1:

andrewtoth commented at 1:45 am on February 12, 2026:

Added this now and added you as co-author.

sipa commented at 3:38 am on February 9, 2026: member

ACK 70e93a10b7fad61483bc9044b483460a7faaa1b8, with a small suggestion

sedited approved

sedited commented at 4:16 pm on February 9, 2026: contributor

ACK 70e93a10b7fad61483bc9044b483460a7faaa1b8

willcl-ark approved

willcl-ark commented at 1:23 pm on February 10, 2026: member

tACK 70e93a10b7fad61483bc9044b483460a7faaa1b8

Had this branch reindex 50k blocks and then left it to run overnight for a few newly-seen blocks without issue. The code changes generally look good to me, and the rationale (as a building block of #31132) makes sense.

I did not run any of the fuzz tests.

in src/coins.cpp:18 in c40f6d743a

14@@ -15,6 +15,7 @@ TRACEPOINT_SEMAPHORE(utxocache, spent);
15 TRACEPOINT_SEMAPHORE(utxocache, uncache);
16 
17 std::optional<Coin> CCoinsView::GetCoin(const COutPoint& outpoint) const { return std::nullopt; }
18+std::optional<Coin> CCoinsView::PeekCoin(const COutPoint& outpoint) const { return GetCoin(outpoint); }

ryanofsky commented at 5:33 pm on February 10, 2026:

In commit “coins: add PeekCoin()” (c40f6d743a696a66eaf68785688d7cb3d2c4be18)

Two issues I think would be good to address in this commit:

PeekCoin is documented as not modifying the cache, but the default implementation calls GetCoin can modify it.
GetCoin and HaveCoin should probably have documentation saying now that they may modify the cache, to contrast with PeekCoin

The problem with default PeekCoin implementation calling GetCoin could be minimally addressed by just not providing any PeekCoin implementation, which would be in keeping with #34124. IMO, a nicer solution would be to make PeekCoin, GetCoin, and HaveCoin all non-virtual methods, with simpler and more clearly distinguished hooks for subclasses to implement:

0    virtual bool LookupCoin(const COutPoint& outpoint, Coin* coin = nullptr) const = 0;
1    virtual bool MutableLookupCoin(const COutPoint& outpoint, Coin* coin = nullptr) = 0;

642fa06f0af7321d35959f599e2edf74e72c6bd9 is a full implementation to show what this would look like.

More ideally all of this should just be enforced with const. Only methods that don’t mutate the cache should be const and others should be non-const. However, this would be a bigger change since const is currently being used to guard against BatchWrite being called. To fix that it would make sense to drop BatchWrite from the coins “view” interface and add it to a separate “store” interface.

So this is kind of a rabbithole. But I think it’s important to at least document these methods better and to prevent the default PeekCoin implmentation from being able to modify the cache.

l0rinc commented at 8:24 pm on February 10, 2026:

this PR does have a lot of acks so may not be worth changing

I don’t mind re-reviewing, your suggestions are always very useful.

https://github.com/bitcoin/bitcoin/commit/642fa06f0af7321d35959f599e2edf74e72c6bd9 is a full implementation to show what this would look like.

This kinda’ undoes #30849 and has a lot of renames, but I like the new comments.

More ideally all of this should just be enforced with const.

However, this would be a bigger change

👍 - we should do that in a follow-up!

would make sense to drop BatchWrite from the coins “view” interface

Yes, we will have to segregate that interface, it has multiple issues, but first we need to gradually clean up usage to fully understand the narrowest interfaces, see #34280

at least document these methods better and to prevent the default PeekCoin implementation

Agree, I also won’t mind if we do that in follow-up since we will be touching all of these in other cleanup PRs as well

andrewtoth commented at 3:31 am on February 11, 2026:

I think a simpler solution for now is to just make the default PeekCoin return std::nullopt like GetCoin, and override PeekCoin in CCoinsViewDB.

andrewtoth commented at 1:43 am on February 12, 2026:

Made the default PeekCoin return std::nullopt, so subclasses have to explicitly opt in. This should pair well with #34124 as well.

Also took some of your comments from 642fa06f0af7321d35959f599e2edf74e72c6bd9 and added you as a co-author.

I think the further improvements re: LookupCoin and constness are interesting but could be better explored in a more focused follow-up.

andrewtoth commented at 2:38 am on February 13, 2026:

I reverted this change. I think it’s safe to forward to GetCoin here at the base. This would only be an issue if there was a stack like CCoinsViewCache -> CCoinsView -> CCoinsViewCache. In production code, there is only really CoinsViewOverlay -> CCoinsViewCache -> CCoinsViewDB. It doesn’t matter for the db to have PeekCoin because it doesn’t cache anything. I think this is ok to merge as is, and further improvements will follow in #34124.

vasild commented at 2:43 pm on February 19, 2026:

Why revert? I came here because I found it odd to have PeekCoin() calling GetCoin(). Anything looks better compared to calling GetCoin() - return nullopt, or leave it without implementation, or unconditional assert if this is never supposed to be called.

andrewtoth commented at 3:45 pm on February 19, 2026:

@vasild see #34165 (review) for more discussion on why. For now this is an ok solution, but when we get #34124 we can make PeekCoin pure virtual and ensure every implementation adds it. Then it will be safe to delegate to GetCoin.

in src/coins.h:386 in ea96314622

381@@ -382,6 +382,9 @@ class CCoinsViewCache : public CCoinsViewBacked
382      */
383     void Reset() noexcept;
384 
385+    /* Get the coin from base. Used for cache misses in FetchCoin. */
386+    virtual std::optional<Coin> GetCoinFromBase(const COutPoint& outpoint) const;

ryanofsky commented at 6:00 pm on February 10, 2026:

In commit “coins: don’t mutate main cache when connecting block” (ea96314622292dbe0d333d61a9acd4c198532285)

Seems like it would make more sense to call this method FetchCoinFromBase since it is called from FetchCoin, and PR seems to be using Get to mean fetch with caching, and Peek to mean fetch without caching?

in src/validation.cpp:1859 in ea96314622 outdated

1855@@ -1856,7 +1856,7 @@ void CoinsViews::InitCache()
1856 {
1857     AssertLockHeld(::cs_main);
1858     m_cacheview = std::make_unique<CCoinsViewCache>(&m_catcherview);
1859-    m_connect_block_view = std::make_unique<CCoinsViewCache>(&*m_cacheview);
1860+    m_connect_block_view = std::make_unique<CoinsViewOverlay>(&*m_cacheview);

ryanofsky commented at 6:04 pm on February 10, 2026:

In commit “coins: don’t mutate main cache when connecting block” (ea96314622292dbe0d333d61a9acd4c198532285)

It would be helpful if commit description could say what practical effect this change expected to have on ConnectTip performance. Current commit description only says that this should be useful for a parallel fetching in the future, but it’s not clear what impact this change could have by itself. PR description says caching “is generally a performance improvement for single threaded access patterns” but I guess that doesn’t apply here because if the block is successfully connected, m_connect_block_view will be flushed and the cache underneath will contain all of the same entries as before? And if the block fails to connect, probably better if its inputs aren’t cached?

Also it would seem better to introduce this change in the later commit introducing the connect_tip_does_not_cache_inputs_on_failed_connect test which validates it. There is already enough complexity in this commit without this.

l0rinc commented at 8:28 pm on February 10, 2026:

It would be helpful if commit description could say what practical effect this change expected to have on ConnectTip performance

I have measured it via pruned IBD and reindex-chainstate multiple times and couldn’t measure any difference. It’s not necessarily an optimization - but the PR description doesn’t oversell that aspect in my opinion.

andrewtoth commented at 1:41 am on February 12, 2026:

Moved this change to the following commit that introduces the integration test. There is no expected performance change, so I did not feel it was necessary to add a comment here.

in src/test/coinsviewoverlay_tests.cpp:24 in ea96314622 outdated

19+
20+BOOST_AUTO_TEST_SUITE(coinsviewoverlay_tests)
21+
22+namespace {
23+
24+CBlock CreateBlock() noexcept

ryanofsky commented at 6:19 pm on February 10, 2026:

In commit “coins: don’t mutate main cache when connecting block” (ea96314622292dbe0d333d61a9acd4c198532285)

Any background or insight on why ~150 lines of test code added to test a much smaller change which is effectively just swapping base->GetCoin with GetCoinFromBase?

Are the test setup functions newly written or copied from somewhere?

If CoinsViewOverlay and CCoinsViewCache classes are identical except one class calling base->GetCoin and the other calling base->PeekCoin I would not expect them to just use same test setup code and similar test cases instead of having entirely separate tests in different files.

This also makes me wonder if implementation should be simpler and CCoinsViewCache should just take a bool use_peek option and new class CoinsViewOverlay and new GetCoinFromBase method and new test setup code could all go away.

l0rinc commented at 8:30 pm on February 10, 2026:

should just take a bool use_peek option

So the object itself would either always populate underlying cache fetches or never for the GetCoin calls?

ryanofsky commented at 8:43 pm on February 10, 2026:

re: #34165 (review)

So the object itself would either always populate underlying cache fetches or never for the GetCoin calls?

Yes, if CCoinsViewCache and CoinsViewOverlay classes are identical, except one of them uses base->GetCoin and the other one uses base->PeekCoin this could be implemented with a boolean option to avoid needing a new virtual hook GetCoinFromBase hook and maybe let the two classes share more testing code. Current approach also seems ok, just wonder if it makes sense to simplify.

andrewtoth-exo commented at 8:57 pm on February 10, 2026:

So, the extra context here is that this change is pulled out of #31132, where we will extend CoinsViewOverlay::GetCoinFromBase to first lookup coins in a new internal thread safe queue. Only if the coin is not found in the thread safe queue will we fallback to base->PeekCoin. A lot of the design decisions in this PR are due to making this easy to rebase the parent PR onto. The extra test code here will incorporate a StartFetching(CBlock& block) method on CoinsViewOverlay.

in src/test/fuzz/coins_view.cpp:384 in babb80d03a outdated

379@@ -327,3 +380,12 @@ FUZZ_TARGET(coins_view_db, .init = initialize_coins_view)
380     CCoinsViewCache coins_view_cache{&backend_coins_view, /*deterministic=*/true};
381     TestCoinsView(fuzzed_data_provider, coins_view_cache, backend_coins_view, /*is_db=*/true);
382 }
383+
384+FUZZ_TARGET(coins_view_overlay, .init = initialize_coins_view)

ryanofsky commented at 7:01 pm on February 10, 2026:

In commit “fuzz: add target for CoinsViewOverlay” (babb80d03aee7d7ca8ce33c6a96250f080b50a9f)

I think the description of this test in the commit description should be removed and added to the code. Similar the description of MutationGuardCoinsViewCache should be removed and added to that class.

Should be possible to learn this information from the code and not need to look at git history.

in src/test/fuzz/coins_view.cpp:76 in babb80d03a

71+        // Nothing must modify cacheCoins other than BatchWrite.
72+        assert(ComputeCacheCoinsSnapshot() == m_expected_snapshot);
73+        try {
74+            CCoinsViewCache::BatchWrite(cursor, block_hash);
75+        } catch (const std::logic_error& e) {
76+            // This error is thrown if we try and write a fresh entry when we already have the same fresh entry.

ryanofsky commented at 7:37 pm on February 10, 2026:

In commit “fuzz: add target for CoinsViewOverlay” (babb80d03aee7d7ca8ce33c6a96250f080b50a9f)

I don’t understand what causes this exception to happen in the new test when it didn’t seem to happen in previous tests, when the only difference seems to be an extra layer of caching here.

I also don’t understand what the comment means when it says we have “the same fresh entry” when from the error the only thing you would know about the entry is that it is unspent, not that it the same or that it is fresh.

It would be helpful to see a simple example of what operations the fuzzer could do to the overlay to trigger this exception.

l0rinc commented at 8:32 pm on February 10, 2026:

Related to #34165 (review)

andrewtoth commented at 9:11 pm on February 10, 2026:

I don’t understand what causes this exception to happen in the new test when it didn’t seem to happen in previous tests, when the only difference seems to be an extra layer of caching here.

It does happen in the previous tests, https://github.com/bitcoin/bitcoin/blob/master/src/test/fuzz/coins_view.cpp#L170-L176. This is adding a second CCoinsViewCache::BatchWrite in a different spot.

I also don’t understand what the comment means when it says we have “the same fresh entry” when from the error the only thing you would know about the entry is that it is unspent, not that it the same or that it is fresh.

I’m not complete sure what you mean by this. This BatchWrite is called by the child cache when it is writing its contents to it via Flush or Sync. The error is thrown when the child cache has a fresh entry for an outpoint, and this cache has a fresh entry for the same outpoint. Please let me know how this can be made more clear via the comment.

It would be helpful to see a simple example of what operations the fuzzer could do to the overlay to trigger this exception.

This exception is thrown if the fuzzer does “set random_coin” -> AddCoin -> Flush -> AddCoin -> Flush.

andrewtoth commented at 1:40 am on February 12, 2026:

Clarified the comment here to better explain this case. I hope you find it to be an improvement.

ryanofsky commented at 9:55 pm on February 12, 2026:

re: #34165 (review)

Clarified the comment here to better explain this case. I hope you find it to be an improvement.

Thanks. i was confused by “same fresh entry” and thought it was saying the same coin was added to both caches, so my question was how we know they are the same? And why would they be the same? New comment is clear it is only referring to the same outpoints.

I also didn’t realize this error already happened elsewhere in the fuzz test, so now it makes sense that there isn’t something about the new cache setup which is causing this.

I still suspect there might be a better way to handle the case of conflicting AddCoins calls that doesn’t clear the cache and compromise the assert(ComputeCacheCoinsSnapshot() == m_expected_snapshot) check (maybe just writing the entries directly instead of calling CoinsViewCache::BatchWrite?), but maybe this happens rarely enough that improving the check doesn’t matter.

I’d probably need to look at the fuzz test more to fully understand this, but this approach now seems reasonable and thanks for the clarifications!

in src/test/fuzz/coins_view.cpp:82 in babb80d03a outdated

77+            // There's not an easy way to prevent the fuzzer from reaching this, so we handle it here.
78+            // Since it is thrown in the middle of the write, we reset our own state and iterate through
79+            // the cursor so the caller's state is also reset.
80+            assert(e.what() == std::string{"FRESH flag misapplied to coin that exists in parent cache"});
81+            Reset();
82+            for (auto it{cursor.Begin()}; it != cursor.End(); it = cursor.NextAndMaybeErase(*it)) {}

ryanofsky commented at 7:50 pm on February 10, 2026:

In commit “fuzz: add target for CoinsViewOverlay” (babb80d03aee7d7ca8ce33c6a96250f080b50a9f)

What’s the implication of clearing the cache, undoing this write and all previous writes here? Does it make the test coverage less meaningful when this happens?

The fact that reseting this cache is ok me question what the point is of calling TestCoinsView with 3 layers of views (CCoinsView + MutationGuardCoinsViewCache + CoinsViewOverlay) when the existing fuzz tests are fine with just two layers (CCoinsView + CCoinsViewCache) and (CCoinsViewDB + CCoinsViewCache).

Why not follow the previous pattern and test (CCoinsView + CoinsViewOverlay) and (CCoinsViewDB + CoinsViewOverlay), just overriding the CoinsViewOverlay::BatchWrite method to do the assert(ComputeCacheCoinsSnapshot() == m_expected_snapshot) check? This would seem more consistent and avoid the need for extra complexity and reduced reliability of the test this Reset() call seems to add.

andrewtoth commented at 9:17 pm on February 10, 2026:

What’s the implication of clearing the cache, undoing this write and all previous writes here? Does it make the test coverage less meaningful when this happens?

If this exception is thrown it puts the two caches in an invalid state. In production code we do not catch this error and crash. In other parts of this test we set good_data = false; when we want the fuzzer to not go further, but here we would already be in an invalid state so we need to reset to continue.

what the point is of calling TestCoinsView with 3 layers of views

CoinsViewOverlay is designed to not mutate the parent CCoinsViewCache. So, we need a CoinsViewOverlay backed via a CCoinsViewCache in this case. However, a CCoinsViewCache cannot exist by itself without a backing view, so we need to add a third CCoinsView to it.

in src/test/fuzz/coins_view.cpp:215 in 70e93a10b7

211@@ -212,7 +212,7 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsViewCache& co
212                 }
213                 bool expected_code_path = false;
214                 try {
215-                    auto cursor{CoinsViewCacheCursor(sentinel, coins_map, /*will_erase=*/true)};
216+                    auto cursor{CoinsViewCacheCursor(sentinel, coins_map, /*will_erase=*/fuzzed_data_provider.ConsumeBool())};

ryanofsky commented at 7:51 pm on February 10, 2026:

In commit “fuzz: provide boolean for will_write when testing BatchWrite” (70e93a10b7fad61483bc9044b483460a7faaa1b8)

Can commit description state reason for this change? It seems good to expand coverage but is just unrelated to the rest of the PR or related somehow?

l0rinc commented at 8:34 pm on February 10, 2026:

It’s related to #34165 (review) - but I don’t mind if we add it in a separate PR

andrewtoth commented at 1:39 am on February 12, 2026:

Removed this for now. Agree it’s unrelated and can be done in a follow-up.

ryanofsky approved

ryanofsky commented at 8:09 pm on February 10, 2026: contributor

Code review ACK 70e93a10b7fad61483bc9044b483460a7faaa1b8. Behavior change seems very good and I did not see any problems with the implementation. But it does seem like it could be simplified. Like I don’t understand why this wouldn’t just add a bool use_peek option to CCoinsViewCache instead of introducing a new CoinsViewOverlay class and GetCoinFromBase override. Also I don’t understand why CoinsViewOverlay seems to be tested in some different and more complicated ways than CCoinsViewCache. (Details in comments below.)

I could be missing some context, and this PR does have a lot of acks so may not be worth changing. But I suspect various simplifications and cleanups could be possible here, and maybe done in followups.

sipa commented at 8:29 pm on February 10, 2026: member

Those seem like very useful review comments, @ryanofsky. I’m happy to re-review if any of them were to be addressed in this PR.

andrewtoth force-pushed on Feb 12, 2026

andrewtoth commented at 1:39 am on February 12, 2026: contributor

Thank you for your review @ryanofsky! I have taken some of your suggestions and left clarifying comments for others.

git diff 70e93a10b7fad61483bc9044b483460a7faaa1b8..6ca865fb2bd82b1034c6946d27640999f8135c89

in src/txdb.cpp:79 in 6ca865fb2b

73@@ -74,6 +74,11 @@ std::optional<Coin> CCoinsViewDB::GetCoin(const COutPoint& outpoint) const
74     return std::nullopt;
75 }
76 
77+std::optional<Coin> CCoinsViewDB::PeekCoin(const COutPoint& outpoint) const
78+{
79+    return GetCoin(outpoint);

l0rinc commented at 9:25 am on February 12, 2026:

shouldn’t it be vice-versa for safety, i.e. GetCoin delegating to PeekCoin so that we narrow the contract instead of expanding it? Same for coinscache_sim.cpp

andrewtoth commented at 2:56 pm on February 12, 2026:

You’re absolutely right! We can also solve the issue pointed out in #34165 (review) by making CCoinsView::GetCoin delegate to PeekCoin instead of vice-versa. This makes the base case non-mutating and avoids a footgun of having to define both methods. Otherwise one path would return a Coin the other nullopt.

andrewtoth commented at 3:17 am on February 13, 2026:

I think this is only safe if PeekCoin is a pure virtual method so subclasses can’t miss overriding it.

l0rinc commented at 10:55 pm on February 15, 2026:

in #34124 HaveCoin is already delegated like that in the empty view

You decided not to pursue this in the end? I see there was some back and forth here

in src/test/fuzz/coins_view.cpp:393 in 6ca865fb2b

390+    CCoinsViewCache coins_view_cache{&backend_coins_view, /*deterministic=*/true};
391+    TestCoinsView(fuzzed_data_provider, coins_view_cache, backend_coins_view, /*is_db=*/true);
392+}
393+
394+/**
395+ * Creates a CoinsViewOverlay and a

l0rinc commented at 9:26 am on February 12, 2026:

nit: what happened to the formatting here?

in src/coins.cpp:412 in 6ca865fb2b

407@@ -393,3 +408,8 @@ bool CCoinsViewErrorCatcher::HaveCoin(const COutPoint& outpoint) const
408 {
409     return ExecuteBackedWrapper<bool>([&]() { return CCoinsViewBacked::HaveCoin(outpoint); }, m_err_callbacks);
410 }
411+
412+std::optional<Coin> CCoinsViewErrorCatcher::PeekCoin(const COutPoint& outpoint) const

l0rinc commented at 9:27 am on February 12, 2026:

I don’t like this, we’re just adding dead code. Edit: Can we at least do return CCoinsViewErrorCatcher::GetCoin(outpoint); here to demystify this?

l0rinc commented at 10:54 pm on February 15, 2026:

This still stands - or was reverted since

andrewtoth commented at 10:34 pm on February 19, 2026:

I think it’s ok for now. Don’t want to invalidate 6 ACKs.

andrewtoth force-pushed on Feb 12, 2026

in src/test/fuzz/coins_view.cpp:46 in 1dea8f5d46 outdated

37@@ -35,18 +38,73 @@ bool operator==(const Coin& a, const Coin& b)
38     if (a.IsSpent() && b.IsSpent()) return true;
39     return a.fCoinBase == b.fCoinBase && a.nHeight == b.nHeight && a.out == b.out;
40 }
41+
42+/**
43+ * MutationGuardCoinsViewCache asserts that nothing mutates cacheCoins until
44+ * BatchWrite is called. It keeps a snapshot of the cacheCoins state, which it
45+ * uses for the assertion in BatchWrite. After the call to the superclass
46+ * CCoinsViewCache::BatchWrite returns, it recomputes the snaphshot at that

maflcko commented at 3:03 pm on February 12, 2026:

Possible typos and grammar issues:

snaphshot -> snapshot [typo in comment: "recomputes the snaphshot at that moment" makes "snapshot" misspelled]
snaphshot -> snapshot [typo in comment: "ComputeCacheCoinsSnapshot()" description: "keeps a snapshot ... which it uses for the assertion in BatchWrite. After the call ... it recomputes the snaphshot" — same misspelling repeated]
excercise -> exercise [typo in comment: "This allows us to excercise all methods" — "excercise" is misspelled]

2026-02-12 14:55:06

andrewtoth force-pushed on Feb 12, 2026

DrahtBot added the label CI failed on Feb 12, 2026

andrewtoth force-pushed on Feb 12, 2026

andrewtoth commented at 3:26 pm on February 12, 2026: contributor

Thanks @l0rinc and @maflcko, updated to use PeekCoin by default and delegate to it if GetCoin is not overridden.

git diff 6ca865fb2bd82b1034c6946d27640999f8135c89..5b524f311ed74a751040494735b9a47f5c16f0b2

DrahtBot removed the label CI failed on Feb 12, 2026

in src/coins.cpp:18 in f7136201e6

15 TRACEPOINT_SEMAPHORE(utxocache, spent);
16 TRACEPOINT_SEMAPHORE(utxocache, uncache);
17 
18-std::optional<Coin> CCoinsView::GetCoin(const COutPoint& outpoint) const { return std::nullopt; }
19+std::optional<Coin> CCoinsView::GetCoin(const COutPoint& outpoint) const { return PeekCoin(outpoint); }
20+std::optional<Coin> CCoinsView::PeekCoin(const COutPoint& outpoint) const { return std::nullopt; }

ryanofsky commented at 9:12 pm on February 12, 2026:

In commit “coins: add PeekCoin()” (f7136201e6ef4557bbb1070988c46db26b2386a0)

One concern I’d have about this implementation is that a subclass could override GetCoin and forget to implement PeekCoin and the code would appear correct but non-caching lookups would return nullopt.

This almost seems worse than the previous issue where a subclass might only override GetCoin and PeekCoin would incorrectly update the cache.

Might be worth adding a warning to the class definition that subclasses can override PeekCoin without overriding GetCoin but shouldn’t do the reverse. And this might not be a problem for long if #34124 makes these all virtual methods.

andrewtoth commented at 10:22 pm on February 12, 2026:

Indeed, I had this thought too after I changed it. I don’t think either approach is really ideal on its own, and I appreciate #34124 more now after trying to solve this. In #34124 we can make only PeekCoin pure virtual, which will delegate to GetCoin. Or just make them non-virtual and add better hooks as you recommended. But, I suppose this is better discussed in that PR.

For now, I think I will revert to the previous change (which had 5 or 6 ACKs). There is no path I can see that calls a mutating GetCoin after PeekCoin, and this would have been revealed by fuzzing.

in src/coins.h:545 in f7136201e6

541@@ -534,7 +542,7 @@ class CCoinsViewErrorCatcher final : public CCoinsViewBacked
542         m_err_callbacks.emplace_back(std::move(f));
543     }
544 
545-    std::optional<Coin> GetCoin(const COutPoint& outpoint) const override;
546+    std::optional<Coin> PeekCoin(const COutPoint& outpoint) const override;

ryanofsky commented at 9:18 pm on February 12, 2026:

In commit “coins: add PeekCoin()” (f7136201e6ef4557bbb1070988c46db26b2386a0)

Does this change mean errors will no longer be caught if CCoinsViewErrorCatcher::GetCoin is called since CCoinsViewBacked::GetCoin will call the base view without ExecuteBackedWrapper?

in src/coins.h:349 in f7136201e6 outdated

345@@ -340,6 +346,7 @@ class CCoinsViewBacked : public CCoinsView
346 public:
347     CCoinsViewBacked(CCoinsView *viewIn);
348     std::optional<Coin> GetCoin(const COutPoint& outpoint) const override;
349+    std::optional<Coin> PeekCoin(const COutPoint& outpoint) const override;

ryanofsky commented at 9:24 pm on February 12, 2026:

In commit “coins: add PeekCoin()” (f7136201e6ef4557bbb1070988c46db26b2386a0)

It seems like CCoinsViewMemPool is overriding CCoinsViewBacked::GetCoin but not CCoinsViewBacked::PeekCoin so calling PeekCoin may skip querying the mempool and just return coins from the base view?

This seems like it would be a bug if so, but maybe nothing is calling CCoinsViewMemPool::PeekCoin

ryanofsky commented at 10:14 pm on February 12, 2026: contributor

Code review 5b524f311ed74a751040494735b9a47f5c16f0b2. Thanks for the updates and explanations. Since last review, GetCoin was changed to delegate to PeekCoin instead of the reverse, which resolves my original concern about PeekCoin implementations unintentionally modifying the cache, but raises other concerns (see below). Other than that, some comments were just moved and added, some code moved between commits, and GetCoinFromBase was renamed (thanks!).

If my concerns about GetCoin calling PeekCoin aren’t real or aren’t important, would be happy to re-ack. Otherwise it might be better to go back to the previous approach, or just make these methods pure-virtual and accept a little boilerplate in the subclasses.

At a very high level, I will say the approach of making sure connect block code does not modify the cache by passing a different coinsview at runtime does not seem ideal to me. More ideal would be for the coins view to have a const method for looking up without modifying the cache, and a non-const method for looking up with caching, and the connect block code would be changed directly to use non-caching lookups, and the compiler would enforce this because it is passed a const view. Still, this runtime implementation seems like an improvement over status quo, and I’m probably not seeing the bigger picture here since these changes are used in follow up PRs

coins: add PeekCoin()

Introduce a helper to look up a Coin through a stack of CCoinsViewCache layers without populating parent caches.

This is useful for ephemeral views (e.g. during ConnectBlock) that want to avoid polluting CoinsTip() when validating invalid blocks.

Co-authored-by: l0rinc <pap.lorinc@gmail.com>
Co-authored-by: Pieter Wuille <pieter@wuille.net>
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

69b01af0eb

coins: introduce CoinsViewOverlay

Introduce `CoinsViewOverlay`, a `CCoinsViewCache` subclass that reads
coins without mutating the underlying cache via `FetchCoin()`.

Use `PeekCoin()` to look up a Coin through a stack of `CCoinsViewCache` layers without populating parent caches. This prevents the main cache from caching inputs pulled from disk for a block that has not yet been fully validated. Once `Flush()` is called on the view, these inputs will be added as spent to `coinsCache` in the main cache via `BatchWrite()`.

This is the foundation for async input fetching, where worker threads must not
mutate shared state.

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

67c0d1798e

coins: don't mutate main cache when connecting block

Use `CoinsViewOverlay` when connecting blocks in `ConnectTip`.

Add a new integration test to verify that using
CoinsViewOverlay does not mutate the main cache
during validation for an invalid block.

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

73e99a5966

fuzz: pass coins_view_cache to TestCoinsView in coins_view

Refactor TestCoinsView() to accept the cache as a parameter instead of
creating it internally. This prepares for adding a CoinsViewOverlay
fuzz target that needs to pass in a different cache type.

This is a non-functional change.

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

89824fb27b

fuzz: move backend mutating block to end of coins_view

Refactor TestCoinsView() to move code that directly modifies
backend_coins_view to the end of the function.
This prepares for a CoinsViewOverlay fuzz target that asserts
the backend_coins_view is not mutated by any methods before
BatchWrite is called.

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

86eda88c8e

fuzz: add target for CoinsViewOverlay

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

cae6d895f8

andrewtoth force-pushed on Feb 13, 2026

andrewtoth commented at 2:35 am on February 13, 2026: contributor

Thanks again for your review @ryanofsky. I reverted the change to PeekCoin. From the previously well ACK’d 70e93a10b7fad61483bc9044b483460a7faaa1b8, the changes are:

rename GetCoinFromBase -> FetchCoinFromBase
remove the last commit, which touched an unrelated part of the fuzz coins_view target.
added coverage for PeekCoin in coinscache_sim.
Added and improved more comments throughout.

git diff 70e93a10b7fad61483bc9044b483460a7faaa1b8..cae6d895f8a8cf5f57e05519536fda5d62b10841

DrahtBot added the label CI failed on Feb 13, 2026

andrewtoth closed this on Feb 14, 2026

andrewtoth reopened this on Feb 14, 2026

DrahtBot removed the label CI failed on Feb 14, 2026

in src/test/fuzz/coins_view.cpp:259 in cae6d895f8

252@@ -220,8 +253,10 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend
253     }
254 
255     {
256-        std::unique_ptr<CCoinsViewCursor> coins_view_cursor = backend_coins_view.Cursor();
257-        assert(is_db == !!coins_view_cursor);
258+        if (is_db) {
259+            std::unique_ptr<CCoinsViewCursor> coins_view_cursor = backend_coins_view.Cursor();
260+            assert(!!coins_view_cursor);
261+        }

l0rinc commented at 11:25 pm on February 15, 2026:

this is kinda broken currently (the backend isn’t actually changed) - do we really need to modify this here or can we wait for #34124 to land first and have a more meaningful assertion here?

l0rinc commented at 11:33 pm on February 15, 2026: contributor

ACK cae6d895f8a8cf5f57e05519536fda5d62b10841

There was some back-and-forth here since my last review. Left some nits, I’m fine with it as-is, would prefer for #34124 to land first to make this change a bit cleaner, but I don’t mind rebasing that either. It’s better than before, but it’s not our last coins related refactor :)

DrahtBot requested review from sedited on Feb 15, 2026

DrahtBot requested review from willcl-ark on Feb 15, 2026

DrahtBot requested review from ryanofsky on Feb 15, 2026

DrahtBot requested review from sipa on Feb 15, 2026

l0rinc approved

sipa commented at 8:58 pm on February 18, 2026: member

reACK cae6d895f8a8cf5f57e05519536fda5d62b10841

sedited approved

sedited commented at 8:48 am on February 19, 2026: contributor

Re-ACK cae6d895f8a8cf5f57e05519536fda5d62b10841

willcl-ark approved

willcl-ark commented at 9:49 am on February 19, 2026: member

ACK cae6d895f8a8cf5f57e05519536fda5d62b10841

LGTM still. Changes since last review (git range-diff 70e93a1...cae6d895f8a) in the commit reorg are clearer method names (e.g. GetCoinFromBase renamed to FetchCoinFromBase), some commit comments moved into code comments, which seems more useful, and some fuzz tidy-ups.

vasild approved

vasild commented at 3:40 pm on February 19, 2026: contributor

Cursory ACK cae6d895f8a8cf5f57e05519536fda5d62b10841

I don’t get it why the default PeekCoin() is calling GetCoin().

ryanofsky approved

ryanofsky commented at 8:48 pm on February 19, 2026: contributor

Code review ACK cae6d895f8a8cf5f57e05519536fda5d62b10841. PR is basically back to the form I had acked the first time, implementing PeekCoin() by calling GetCoin(). This is not ideal because PeekCoin() is not supposed to modify caches and GetCoin() does that, but it at least avoids problems of the subsequent approach tried where GetCoin() calls PeekCoin and would result in bugs when subclasses implement GetCoin forgetting to override PeekCoin. Hopefully #34124 can clean all of this by making relevant methods pure virtual.

More broadly, I don’t understand a lot of complexity behind the design in this PR. I don’t understand why a new virtual FetchCoinFromBase virtual method, a new CoinsViewOverlay subclass, and new test suite coinsviewoverlay_tests.cpp need to be be written just to pass the connect block code a view that calls Peek instead of Get. It seems like you could add a simple bool call_base_peek_instead_of_get option to the existing CCoinsViewCache class with a 3 line diff and have all the benefits of these changes. But I understand followups extend CoinsViewOverlay and probably provide more justification.

I also don’t think swapping in a different m_connect_block_view class at runtime seems like the ideal way of preventing connect block code from modifying caches, when it should be possible to have const/nonconst methods or const/mutable views enforcing this at compile time. But I understand this would be a more invasive and potentially larger overall change and probably not a priority.

Overall looks like PR is ready for merge but would be good to respond to latest batch of comments from l0rinc #34165 (comment). Thanks for all the followups on my previous reviews and answering all my questions!

andrewtoth commented at 10:37 pm on February 19, 2026: contributor

Thanks for all your reviews! I’m wary of making any changes at this point since this is the second time we are at 6 ACKs. Would like this to go in. @l0rinc I will update with your suggestions if I have to push again. Otherwise many of them can go into #34124 which will need a rebase. @ryanofsky thanks for your comments, I think this will make more sense once #31132 is rebased onto it.

ryanofsky assigned ryanofsky on Feb 20, 2026

ryanofsky merged this on Feb 20, 2026

ryanofsky closed this on Feb 20, 2026

andrewtoth deleted the branch on Feb 20, 2026

fanquake commented at 4:08 pm on February 21, 2026: member

New issue from oss-fuzz: #34645.

coins: don’t mutate main cache when connecting block #34165

Code Coverage & Benchmarks

Reviews

Conflicts