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>

src/test/validation_chainstate_tests.cpp

@@ -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_AUTO_TEST_CASE(validation_chainstate_resize_caches)
     }
 }
 
+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(/*reallocate_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())};
+    const CBlock block{CreateBlock({tx}, CScript{} << OP_TRUE, chainstate)};
+    BOOST_CHECK(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

src/validation.cpp

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

src/validation.h

@@ -10,6 +10,7 @@
 #include <attributes.h>
 #include <chain.h>
 #include <checkqueue.h>
+#include <coins.h>
 #include <consensus/amount.h>
 #include <cuckoocache.h>
 #include <deploymentstatus.h>
@@ -489,9 +490,9 @@ public:
     //! can fit per the dbcache setting.
     std::unique_ptr<CCoinsViewCache> m_cacheview GUARDED_BY(cs_main);
 
-    //! Temporary CCoinsViewCache layered on top of m_cacheview and passed to ConnectBlock().
+    //! Reused CoinsViewOverlay layered on top of m_cacheview and passed to ConnectBlock().
     //! Reset between calls and flushed only on success, so invalid blocks don't pollute the underlying cache.
-    std::unique_ptr<CCoinsViewCache> m_connect_block_view GUARDED_BY(cs_main);
+    std::unique_ptr<CoinsViewOverlay> m_connect_block_view GUARDED_BY(cs_main);
 
     //! This constructor initializes CCoinsViewDB and CCoinsViewErrorCatcher instances, but it
     //! *does not* create a CCoinsViewCache instance by default. This is done separately because the