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
Approach ACK	sedited

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: fail fast on database read deserialization errors by l0rinc)
#34124 (refactor: make CCoinsView a purely virtual abstract base class by l0rinc)
#33512 (coins: use number of dirty cache entries in flush warnings/logs 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.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

cache.AddCoin(outpoint, Coin{coin}, false) in src/test/coins_tests.cpp

2026-02-04 19:42:57

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:106 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:44 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:111 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:42 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.

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>

c40f6d743a

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 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:138 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!

coins: don't mutate main cache when connecting block

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>

ea96314622

test: assert CoinsTip is not mutated after invalid ConnectBlock

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>

cb1fb6b68d

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>

9df24c2a63

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>

e42b1fd6bc

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.

fuzz: add target for CoinsViewOverlay

Introduce MutationGuardCoinsViewCache, which asserts that nothing
mutates cacheCoins until BatchWrite is called. It keeps a snapshot
of the cacheCoins state, which it uses for the assertion in BatchWrite.
After the call to the superclass CCoinsViewCache::BatchWrite returns,
it recomputes the snaphshot at that moment.

The coins_view_overlay fuzz target creates a CoinsViewOverlay and a
MutationGuardCoinsViewCache as the base. This allows us to excercise
all methods on a CoinsViewOverlay, while also ensuring that nothing
can mutate the underlying cache until Flush or Sync is called.

Since backend_coins_view can now be a CCoinsViewCache subclass, we need
to handle BatchWrite and Cursor() throwing.

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

babb80d03a

fuzz: provide boolean for will_write when testing BatchWrite 70e93a10b7

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

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…

in src/coins.h:307 in 70e93a10b7

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.

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

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)