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

pull andrewtoth wants to merge 6 commits into bitcoin:master from andrewtoth:dont-mutate-cache changing 10 files +386 −38
  1. 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.

  2. DrahtBot added the label UTXO Db and Indexes on Dec 28, 2025
  3. DrahtBot commented at 8:32 PM on December 28, 2025: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

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

    <!--021abf342d371248e50ceaed478a90ca-->

    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 <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--174a7506f384e20aa4161008e828411d-->

    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.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  4. 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.

  5. l0rinc changes_requested
  6. 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:

    diff --git a/src/test/validation_chainstate_tests.cpp b/src/test/validation_chainstate_tests.cpp
--- a/src/test/validation_chainstate_tests.cpp	(revision 20f0b8304d089d641060f693d3284e69524fbcbe)
+++ b/src/test/validation_chainstate_tests.cpp	(date 1767360120004)
@@ -3,10 +3,12 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 //
 #include <chainparams.h>
+#include <consensus/amount.h>
 #include <consensus/validation.h>
 #include <node/kernel_notifications.h>
 #include <random.h>
 #include <rpc/blockchain.h>
+#include <script/script.h>
 #include <sync.h>
 #include <test/util/chainstate.h>
 #include <test/util/coins.h>
@@ -63,6 +65,30 @@
     }
 }
 
+BOOST_FIXTURE_TEST_CASE(connect_tip_does_not_cache_inputs_on_failed_connect, TestChain100Setup)
+{
+    Chainstate& chainstate = Assert(m_node.chainman)->ActiveChainstate();
+
+    COutPoint outpoint;
+    {
+        LOCK(cs_main);
+        outpoint = AddTestCoin(m_rng, chainstate.CoinsTip());
+        chainstate.CoinsTip().Flush(/*will_reuse_cache=*/false);
+    }
+
+    CMutableTransaction tx;
+    tx.vin.emplace_back(outpoint);
+    tx.vout.emplace_back(MAX_MONEY, CScript{} << OP_TRUE);
+
+    const auto tip{WITH_LOCK(cs_main, return chainstate.m_chain.Tip()->GetBlockHash())};
+    CBlock block = CreateBlock({tx}, CScript{} << OP_TRUE, chainstate);
+    Assert(m_node.chainman)->ProcessNewBlock(std::make_shared<CBlock>(block), true, true, nullptr);
+
+    LOCK(cs_main);
+    BOOST_CHECK_EQUAL(tip, chainstate.m_chain.Tip()->GetBlockHash()); // block rejected
+    BOOST_CHECK(!chainstate.CoinsTip().HaveCoinInCache(outpoint));    // input not cached
+}
+
 //! Test UpdateTip behavior for both active and background chainstates.
 //!
 //! When run on the background chainstate, UpdateTip should do a subset
diff --git a/src/validation.cpp b/src/validation.cpp
--- a/src/validation.cpp	(revision 20f0b8304d089d641060f693d3284e69524fbcbe)
+++ b/src/validation.cpp	(date 1767360154724)
@@ -3102,7 +3102,7 @@
     LogDebug(BCLog::BENCH, "  - Load block from disk: %.2fms\n",
              Ticks<MillisecondsDouble>(time_2 - time_1));
     {
-        CCoinsViewCache view(&CoinsTip());
+        CoinsViewCacheNonMutating view(&CoinsTip());
         bool rv = ConnectBlock(*block_to_connect, state, pindexNew, view);
         if (m_chainman.m_options.signals) {
             m_chainman.m_options.signals->BlockChecked(block_to_connect, state);

    Concept ACK (edited to remove the nack).

  7. andrewtoth force-pushed on Jan 2, 2026
  8. 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.

  9. l0rinc referenced this in commit d3a7723aa8 on Jan 2, 2026
  10. andrewtoth force-pushed on Jan 3, 2026
  11. andrewtoth force-pushed on Jan 3, 2026
  12. DrahtBot added the label CI failed on Jan 3, 2026
  13. DrahtBot commented at 7:22 PM on January 3, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

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

    <details><summary>Hints</summary>

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

    </details>

  14. DrahtBot removed the label CI failed on Jan 3, 2026
  15. 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.

  16. andrewtoth force-pushed on Jan 3, 2026
  17. 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:

    static std::vector<COutPoint> CreateOutpoints() noexcept
{
    static constexpr uint32_t NUM_OUTPOINTS{100};
    std::vector<COutPoint> outpoints;
    outpoints.reserve(NUM_OUTPOINTS);

    for (const auto i : std::views::iota(uint32_t{1}, NUM_OUTPOINTS + 1)) {
        outpoints.emplace_back(Txid::FromUint256(uint256(i)), /*nIn=*/0);
    }

    return outpoints;
}

    If you disagree, just resolve without a comment.

  18. l0rinc approved
  19. 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.

  20. andrewtoth force-pushed on Jan 4, 2026
  21. l0rinc referenced this in commit 15c56be893 on Jan 4, 2026
  22. 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:

    diff --git a/src/coins.cpp b/src/coins.cpp
--- a/src/coins.cpp	(revision 85b20448640f8209e453019c3e1494c1ae229d74)
+++ b/src/coins.cpp	(revision c3b34ec796a3fd6e34e0dfb862c61bc298ba8d6f)
@@ -59,10 +59,15 @@
     return memusage::DynamicUsage(cacheCoins) + cachedCoinsUsage;
 }
 
+std::optional<Coin> CCoinsViewCache::GetCoinFromBase(const COutPoint& outpoint) const
+{
+    return base->GetCoin(outpoint);
+}
+
 CCoinsMap::iterator CCoinsViewCache::FetchCoin(const COutPoint &outpoint) const {
     const auto [ret, inserted] = cacheCoins.try_emplace(outpoint);
     if (inserted) {
-        if (auto coin{base->GetCoin(outpoint)}) {
+        if (auto coin{GetCoinFromBase(outpoint)}) {
             ret->second.coin = std::move(*coin);
             cachedCoinsUsage += ret->second.coin.DynamicMemoryUsage();
             if (ret->second.coin.IsSpent()) { // TODO GetCoin cannot return spent coins
diff --git a/src/coins.h b/src/coins.h
--- a/src/coins.h	(revision 85b20448640f8209e453019c3e1494c1ae229d74)
+++ b/src/coins.h	(revision c3b34ec796a3fd6e34e0dfb862c61bc298ba8d6f)
@@ -382,6 +382,8 @@
     /* Cached dynamic memory usage for the inner Coin objects. */
     mutable size_t cachedCoinsUsage{0};
 
+    virtual std::optional<Coin> GetCoinFromBase(const COutPoint& outpoint) const;
+
 public:
     CCoinsViewCache(CCoinsView *baseIn, bool deterministic = false);
 
@@ -489,7 +491,7 @@
      * [@note](/github-metadata-backup-bitcoin-bitcoin/contributor/note/) this is marked const, but may actually append to `cacheCoins`, increasing
      * memory usage.
      */
-    virtual CCoinsMap::iterator FetchCoin(const COutPoint &outpoint) const;
+    CCoinsMap::iterator FetchCoin(const COutPoint &outpoint) const;
 };
 
 //! Utility function to add all of a transaction's outputs to a cache.
diff --git a/src/coinsviewcachenonmutating.h b/src/coinsviewcachenonmutating.h
--- a/src/coinsviewcachenonmutating.h	(revision 85b20448640f8209e453019c3e1494c1ae229d74)
+++ b/src/coinsviewcachenonmutating.h	(revision c3b34ec796a3fd6e34e0dfb862c61bc298ba8d6f)
@@ -9,28 +9,16 @@
 #include <primitives/transaction.h>
 
 /**
- * CCoinsViewCache subclass that does not call GetCoin on base via FetchCoin, so base will not be mutated before Flush.
+ * CCoinsViewCache subclass that does not call GetCoin on base, so base will not be mutated before Flush.
  * Only used in ConnectBlock to pass as an ephemeral view that can be reset if the block is invalid.
- * It provides the same interface as CCoinsViewCache, overriding the FetchCoin private method.
+ * It provides the same interface as CCoinsViewCache, overriding GetCoinFromBase().
  */
 class CoinsViewCacheNonMutating : public CCoinsViewCache
 {
 private:
-    CCoinsMap::iterator FetchCoin(const COutPoint& outpoint) const override
+    std::optional<Coin> GetCoinFromBase(const COutPoint& outpoint) const override
     {
-        auto [ret, inserted]{cacheCoins.try_emplace(outpoint)};
-        if (!inserted) return ret;
-
-        // TODO: Remove spent checks once we no longer return spent coins in coinscache_sim CoinsViewBottom.
-        if (auto coin{base->PeekCoin(outpoint)}; coin && !coin->IsSpent()) {
-            ret->second.coin = std::move(*coin);
-        } else {
-            cacheCoins.erase(ret);
-            return cacheCoins.end();
-        }
-
-        cachedCoinsUsage += ret->second.coin.DynamicMemoryUsage();
-        return ret;
+        return base->PeekCoin(outpoint);
     }
 
 public:
    andrewtoth commented at 3:40 PM on January 10, 2026:

    Nice, done.

  23. 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)

  24. 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:

    diff --git a/src/test/coinsviewcachenonmutating_tests.cpp b/src/test/coinsviewcachenonmutating_tests.cpp
--- a/src/test/coinsviewcachenonmutating_tests.cpp	(revision c3b34ec796a3fd6e34e0dfb862c61bc298ba8d6f)
+++ b/src/test/coinsviewcachenonmutating_tests.cpp	(revision 3e8676d49ecf9e3c12cd641ce655e74f85f3a6e6)
@@ -82,6 +82,13 @@
     PopulateView(block, db);
     CCoinsViewCache main_cache{&db};
     CoinsViewCacheNonMutating view{&main_cache};
+    const auto& outpoint{block.vtx[1]->vin[0].prevout};
+
+    BOOST_CHECK(view.HaveCoin(outpoint));
+    BOOST_CHECK(view.GetCoin(outpoint).has_value());
+    BOOST_CHECK(!main_cache.HaveCoinInCache(outpoint));
+
     CheckCache(block, view);
     // Check that no coins have been moved up to main cache from db
     for (const auto& tx : block.vtx) {
@@ -89,6 +96,12 @@
             BOOST_CHECK(!main_cache.HaveCoinInCache(in.prevout));
         }
     }
+
+    view.SetBestBlock(uint256::ONE);
+    BOOST_CHECK(view.SpendCoin(outpoint));
+    view.Flush();
+    BOOST_CHECK(!main_cache.PeekCoin(outpoint).has_value());
 }
 
 BOOST_AUTO_TEST_CASE(fetch_inputs_from_cache)
  25. l0rinc changes_requested
  26. andrewtoth force-pushed on Jan 10, 2026
  27. l0rinc commented at 12:53 AM on January 11, 2026: contributor

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

    ACK 2e67c5b604d2cf2e56db6b5d2906391d22113941

  28. 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:

    	frame [#0](/github-metadata-backup-bitcoin-bitcoin/0/): 0x000000010381616c test_bitcoin`CCoinsViewDB::GetCoin(this=0x0000000109549fe0, outpoint=0x000000016dc24a6c) const at txdb.cpp:70:14
    frame [#1](/github-metadata-backup-bitcoin-bitcoin/1/): 0x000000010419da40 test_bitcoin`CCoinsView::PeekCoin(this=0x0000000109549fe0, outpoint=0x000000016dc24a6c) const at coins.cpp:17:93
    frame [#2](/github-metadata-backup-bitcoin-bitcoin/2/): 0x000000010419dea0 test_bitcoin`CCoinsViewBacked::PeekCoin(this=0x000000010954a028, outpoint=0x000000016dc24a6c) const at coins.cpp:36:18
    frame [#3](/github-metadata-backup-bitcoin-bitcoin/3/): 0x00000001041a3ee4 test_bitcoin`CCoinsViewErrorCatcher::PeekCoin(COutPoint const&) const::$_0::operator()(this=0x000000016dc23918) const at coins.cpp:413:87
    frame [#4](/github-metadata-backup-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
    frame [#5](/github-metadata-backup-bitcoin-bitcoin/5/): 0x00000001041a1250 test_bitcoin`CCoinsViewErrorCatcher::PeekCoin(this=0x000000010954a028, outpoint=0x000000016dc24a6c) const at coins.cpp:413:12
    frame [#6](/github-metadata-backup-bitcoin-bitcoin/6/): 0x000000010419e2d0 test_bitcoin`CCoinsViewCache::PeekCoin(this=0x000000010954aae0, outpoint=0x000000016dc24a6c) const at coins.cpp:51:18
    frame [#7](/github-metadata-backup-bitcoin-bitcoin/7/): 0x0000000102640b38 test_bitcoin`CoinsViewCacheNonMutating::GetCoinFromBase(this=0x000000016dc251e8, outpoint=0x000000016dc24a6c) const at coinsviewcachenonmutating.h:22:22
    frame [#8](/github-metadata-backup-bitcoin-bitcoin/8/): 0x000000010419e8b8 test_bitcoin`CCoinsViewCache::FetchCoin(this=0x000000016dc251e8, outpoint=0x000000016dc24a6c) const at coins.cpp:73:23
    frame [#9](/github-metadata-backup-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:

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

  29. 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."?

  30. 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.

  31. andrewtoth force-pushed on Jan 24, 2026
  32. 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

  33. l0rinc commented at 8:40 PM on January 25, 2026: contributor

    I have taken your integration test.

    Was it removed since?

  34. 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 12:02 AM on January 26, 2026:

    Removed noexcept for PeekCoin.

  35. 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 12: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.

  36. 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.

  37. 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.
  38. l0rinc changes_requested
  39. andrewtoth force-pushed on Jan 26, 2026
  40. andrewtoth force-pushed on Jan 26, 2026
  41. DrahtBot added the label CI failed on Jan 26, 2026
  42. andrewtoth commented at 12: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

  43. DrahtBot removed the label CI failed on Jan 26, 2026
  44. 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:

    frame [#0](/github-metadata-backup-bitcoin-bitcoin/0/): 0x000000010381616c test_bitcoin`CCoinsViewDB::GetCoin(this=0x0000000109549fe0, outpoint=0x000000016dc24a6c) const at txdb.cpp:70:14
frame [#1](/github-metadata-backup-bitcoin-bitcoin/1/): 0x000000010419da40 test_bitcoin`CCoinsView::PeekCoin(this=0x0000000109549fe0, outpoint=0x000000016dc24a6c) const at coins.cpp:17:93
frame [#2](/github-metadata-backup-bitcoin-bitcoin/2/): 0x000000010419dea0 test_bitcoin`CCoinsViewBacked::PeekCoin(this=0x000000010954a028, outpoint=0x000000016dc24a6c) const at coins.cpp:36:18
frame [#3](/github-metadata-backup-bitcoin-bitcoin/3/): 0x00000001041a3ee4 test_bitcoin`CCoinsViewErrorCatcher::PeekCoin(COutPoint const&) const::$_0::operator()(this=0x000000016dc23918) const at coins.cpp:413:87
frame [#4](/github-metadata-backup-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
frame [#5](/github-metadata-backup-bitcoin-bitcoin/5/): 0x00000001041a1250 test_bitcoin`CCoinsViewErrorCatcher::PeekCoin(this=0x000000010954a028, outpoint=0x000000016dc24a6c) const at coins.cpp:413:12
frame [#6](/github-metadata-backup-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.

  45. 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:

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

     * 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:

    -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.
+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.

  46. l0rinc changes_requested
  47. 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.

  48. andrewtoth force-pushed on Jan 27, 2026
  49. 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

  50. 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.

  51. andrewtoth force-pushed on Jan 29, 2026
  52. 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

  53. andrewtoth force-pushed on Jan 29, 2026
  54. DrahtBot added the label CI failed on Jan 29, 2026
  55. DrahtBot commented at 11:40 PM on January 29, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/21498245018/job/61938643008</sub> <sub>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).</sub>

    <details><summary>Hints</summary>

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

    </details>

  56. DrahtBot removed the label CI failed on Jan 30, 2026
  57. 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.

        //! Reused CoinsViewCacheNonMutating layered on top of m_cacheview and passed to ConnectBlock().
  58. 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: 
                [&]() { // PeekCoin/GetCoin
  59. l0rinc approved
  60. 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!

  61. 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.

  62. 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.

  63. 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)?

  64. 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.

  65. andrewtoth force-pushed on Jan 31, 2026
  66. andrewtoth force-pushed on Jan 31, 2026
  67. DrahtBot added the label CI failed on Jan 31, 2026
  68. 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

  69. 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.

  70. DrahtBot requested review from sedited on Jan 31, 2026
  71. 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.

  72. DrahtBot requested review from sedited on Jan 31, 2026
  73. DrahtBot removed the label CI failed on Jan 31, 2026
  74. andrewtoth force-pushed on Jan 31, 2026
  75. 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

  76. l0rinc commented at 10:11 PM on January 31, 2026: contributor

    ACK 2f9b52fa348242db475f4aff1fee951fc74e934e

  77. sedited requested review from sipa on Feb 1, 2026
  78. 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.

  79. 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!

  80. andrewtoth force-pushed on Feb 3, 2026
  81. 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

  82. 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:

            std::ranges::sort(snapshot, {}, &CacheCoinSnapshot::outpoint);
  83. 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?

                snapshot.emplace_back(outpoint, entry.IsDirty(), entry.IsFresh(), entry.coin);
  84. 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.

  85. 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:

      if (is_db) {                                                                                                                                                                                   
      std::unique_ptr<CCoinsViewCursor> coins_view_cursor{backend_coins_view.Cursor()};
      assert(!!coins_view_cursor);
  }
    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.

  86. 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.

  87. 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.

  88. l0rinc changes_requested
  89. 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.

  90. andrewtoth force-pushed on Feb 4, 2026
  91. 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

  92. 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:

                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

    👍

  93. 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 :)

        //! 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 :)

  94. l0rinc approved
  95. 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.

  96. 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:

    diff --git a/src/test/fuzz/coinscache_sim.cpp b/src/test/fuzz/coinscache_sim.cpp
index 8c2d1678386..ec17a903907 100644
--- a/src/test/fuzz/coinscache_sim.cpp
+++ b/src/test/fuzz/coinscache_sim.cpp
@@ -252,12 +252,14 @@ FUZZ_TARGET(coinscache_sim)
         CallOneOf(
             provider,
 
-            [&]() { // GetCoin
+            [&]() { // GetCoin / PeekCoin
                 uint32_t outpointidx = provider.ConsumeIntegralInRange<uint32_t>(0, NUM_OUTPOINTS - 1);
                 // Look up in simulation data.
                 auto sim = lookup(outpointidx);
                 // Look up in real caches.
-                auto realcoin = caches.back()->GetCoin(data.outpoints[outpointidx]);
+                auto realcoin = provider.ConsumeBool() ?
+                                caches.back()->PeekCoin(data.outpoints[outpointidx]) :
+                                caches.back()->GetCoin(data.outpoints[outpointidx]);
                 // Compare results.
                 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.

  97. sipa commented at 3:38 AM on February 9, 2026: member

    ACK 70e93a10b7fad61483bc9044b483460a7faaa1b8, with a small suggestion

  98. sedited approved
  99. sedited commented at 4:16 PM on February 9, 2026: contributor

    ACK 70e93a10b7fad61483bc9044b483460a7faaa1b8

  100. willcl-ark approved
  101. 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.

  102. 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:

        virtual bool LookupCoin(const COutPoint& outpoint, Coin* coin = nullptr) const = 0;
    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.

  103. 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?

  104. 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.

  105. 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.

  106. 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.

  107. 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!

  108. 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.

  109. 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.

  110. ryanofsky approved
  111. 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.

  112. 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.

  113. andrewtoth force-pushed on Feb 12, 2026
  114. 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

  115. 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

  116. 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?

  117. 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.

  118. andrewtoth force-pushed on Feb 12, 2026
  119. 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

  120. andrewtoth force-pushed on Feb 12, 2026
  121. DrahtBot added the label CI failed on Feb 12, 2026
  122. andrewtoth force-pushed on Feb 12, 2026
  123. 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

  124. DrahtBot removed the label CI failed on Feb 12, 2026
  125. 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.

  126. 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?

  127. 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

  128. 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

  129. 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
  130. 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
  131. 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
  132. 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
  133. 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
  134. fuzz: add target for CoinsViewOverlay
    Co-authored-by: l0rinc <pap.lorinc@gmail.com>
    cae6d895f8
  135. andrewtoth force-pushed on Feb 13, 2026
  136. 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

  137. DrahtBot added the label CI failed on Feb 13, 2026
  138. andrewtoth closed this on Feb 14, 2026
  139. andrewtoth reopened this on Feb 14, 2026
  140. DrahtBot removed the label CI failed on Feb 14, 2026
  141. 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?

  142. 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 :)

  143. DrahtBot requested review from sedited on Feb 15, 2026
  144. DrahtBot requested review from willcl-ark on Feb 15, 2026
  145. DrahtBot requested review from ryanofsky on Feb 15, 2026
  146. DrahtBot requested review from sipa on Feb 15, 2026
  147. l0rinc approved
  148. sipa commented at 8:58 PM on February 18, 2026: member

    reACK cae6d895f8a8cf5f57e05519536fda5d62b10841

  149. sedited approved
  150. sedited commented at 8:48 AM on February 19, 2026: contributor

    Re-ACK cae6d895f8a8cf5f57e05519536fda5d62b10841

  151. willcl-ark approved
  152. 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.

  153. vasild approved
  154. 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().

  155. ryanofsky approved
  156. 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!

  157. 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.

  158. ryanofsky assigned ryanofsky on Feb 20, 2026
  159. ryanofsky merged this on Feb 20, 2026
  160. ryanofsky closed this on Feb 20, 2026
  161. andrewtoth deleted the branch on Feb 20, 2026
  162. fanquake commented at 4:08 PM on February 21, 2026: member

    New issue from oss-fuzz: #34645.

  163. sedited referenced this in commit 925759d172 on Feb 23, 2026
  164. sedited referenced this in commit 8640523843 on Feb 23, 2026
  165. l0rinc referenced this in commit 67f80a1d43 on Apr 14, 2026
  166. l0rinc referenced this in commit 37570dc8ca on Apr 14, 2026
  167. l0rinc referenced this in commit b755269b98 on Apr 15, 2026
  168. l0rinc referenced this in commit c45d698d78 on Apr 15, 2026
  169. l0rinc referenced this in commit a5a9958aae on Apr 15, 2026
  170. l0rinc referenced this in commit 8892881963 on Apr 16, 2026
  171. l0rinc referenced this in commit 326bcb3d33 on Apr 16, 2026
  172. l0rinc referenced this in commit ad62800cd8 on Apr 23, 2026
  173. optout21 commented at 10:08 AM on May 5, 2026: contributor

    Post-merge ACK, and documenting some concerns:

Contributors
andrewtothDrahtBotl0rincwillcl-arkseditedsipaandrewtoth-exovasildryanofskymaflckofanquakeoptout21
Labels
UTXO Db and Indexes
Linked (view graph)
#31132 validation: fetch block inputs on parallel threads#34280 Coins Cache Cleanup checklist#35078 validation: merge `PeekCoin` into `GetCoin`
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:52 UTC