coins: don't mutate main cache when connecting block
What the developer wrote
Authored by Andrew Toth
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>
What changed, and why it matters
This Bitcoin Core change fixes how the coin cache behaves when a new block fails validation. Previously, the same cache used for the main chain was temporarily used during block validation, which could leave behind entries from invalid blocks. Now a separate overlay view is used, so failed blocks don't pollute the main cache. The commit adds a test proving that an invalid block no longer leaves its inputs in the cache. This is a correctness and denial-of-service hardening fix rather than a direct theft bug.
Treat as a routine but worthwhile hardening patch. Reviewers should verify that CoinsViewOverlay correctly discards all changes on destruction/reset and that the overlay flush-on-success path is the only path that can modify m_cacheview. No emergency action is indicated by the diff alone.
Security signals we found
Denial-of-service hardening: invalid blocks could previously pollute the in-memory coin cache, potentially affecting subsequent validation behavior or cache eviction
Correctness fix: separation of validation scratch state from committed chain state
New regression test specifically checks that a rejected block does not cache its inputs
Evidence from the diff
The patch replaces the CCoinsViewCache used as m_connect_block_view with a CoinsViewOverlay layered on top of m_cacheview. The overlay is flushed to the main cache only when ConnectBlock succeeds, so invalid blocks cannot insert or mutate entries in the main coin cache. A new integration test constructs an invalid block (spending a non-existent or already-spent coin with MAX_MONEY output), submits it via ProcessNewBlock, and asserts that the block is rejected and the spent input is not present in the main cache.
Changed components
src/validation.cppsrc/validation.hsrc/test/validation_chainstate_tests.cppCoinsViews::InitCacheChainstate::ConnectTip / ConnectBlock coin view plumbingInspect captured patch +30 / −3
diff --git a/src/test/validation_chainstate_tests.cpp b/src/test/validation_chainstate_tests.cpp
index 9e2c7109..a4a81bba 100644
--- a/src/test/validation_chainstate_tests.cpp
+++ b/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
diff --git a/src/validation.cpp b/src/validation.cpp
index 0dcae858..5d2af01e 100644
--- a/src/validation.cpp
+++ b/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(
diff --git a/src/validation.h b/src/validation.h
index 8aa8fbb4..008922d3 100644
--- a/src/validation.h
+++ b/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
Why this scored 61/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.