coins: don't mutate main cache when connecting block #34165
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34165. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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. |
There was a problem hiding this comment.
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).
andrewtoth
force-pushed
the
dont-mutate-cache
branch
from
9739ce6 to
7cd938b
Compare
|
Thank you @l0rinc, I have taken your integration test. I had unit tests for this behavior but the integration test is a great idea. |
The summary comment parser treated "Concept ACK, but approach NACK." as a `Concept NACK` because it only matched "Approach NACK" with the exact capitalization. Make the "Approach NACK" pattern case-insensitive and add a regression test. Ref: bitcoin/bitcoin#34165 (review)
andrewtoth
force-pushed
the
dont-mutate-cache
branch
2 times, most recently
from
410a014 to
7a6e616
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
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 <[email protected]>
andrewtoth
force-pushed
the
dont-mutate-cache
branch
from
7a6e616 to
9a37b19
Compare
l0rinc
left a comment
l0rinc
left a comment
There was a problem hiding this comment.
I'll run a reindex-chainstate on these to see how they behave - as long as they don't slow down anything, I would be fine with them.
andrewtoth
force-pushed
the
dont-mutate-cache
branch
from
9a37b19 to
7fab35c
Compare
The summary comment parser treated "Concept ACK, but approach NACK." as a `Concept NACK` because it only matched "Approach NACK" with the exact capitalization. Make the "Approach/Concept N?ACK" pattern capitalization-insensitive and add regression tests. Ref: bitcoin/bitcoin#34165 (review)
| Ticks<MillisecondsDouble>(time_2 - time_1)); | ||
| { | ||
| CCoinsViewCache view(&CoinsTip()); | ||
| CoinsViewCacheNonMutating view(&CoinsTip()); |
There was a problem hiding this comment.
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)
Introduce `CoinsViewCacheNonMutating`, 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 <[email protected]>
andrewtoth
force-pushed
the
dont-mutate-cache
branch
from
7fab35c to
2e67c5b
Compare
|
Ran a ACK 2e67c5b |
3 participants
This is a slightly modified version of the second commit 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, the const methods likeGetCoinactually mutatecacheCoinsinternally 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 aCCoinsViewCachefrom multiple threads without a lock.Another aspect is that when we add an ephemeral
CCoinsViewCacheview backed by the main cache for use inConnectBlock(), 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 usFlushthe cache more often than necessary. Obviously this would be very expensive to do on mainnet.Introduce
CoinsViewCacheNonMutating, aCCoinsViewCachesubclass that reads coins without mutating the underlying cache viaFetchCoin().Add
PeekCoin()to look up a Coin through a stack ofCCoinsViewCachelayers 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. OnceFlush()is called on the view, these inputs will be added as spent tocoinsCachein the main cache viaBatchWrite().This is the foundation for async input fetching, where worker threads must not mutate shared state.