coins: only adjust `cachedCoinsUsage` on `EmplaceCoinInternalDANGER` insert
What the developer wrote
Authored by Lőrinc
coins: only adjust `cachedCoinsUsage` on `EmplaceCoinInternalDANGER` insert
`EmplaceCoinInternalDANGER()` incremented `cachedCoinsUsage` even when `try_emplace` did not insert (duplicate key), inflating the counter.
This is mostly reachable in tests today since `AssumeUTXO` does not overwrite.
Increment only on successful insert, and capture `coin.DynamicMemoryUsage()` before the move so accounting uses the correct value.
Fuzz: add an `EmplaceCoinInternalDANGER` path to exercise insert-only accounting.
Unit test: emplace two different coins at the same outpoint (with different `DynamicMemoryUsage()`), verify `SelfTest()` passes and `AccessCoin(outpoint)` returns the first coin.
Co-authored-by: Andrew Toth <andrewstoth@gmail.com>
Co-authored-by: w0xlt <woltx@protonmail.com>
What changed, and why it matters
This commit fixes an accounting bug in Bitcoin Core's in-memory coin cache. A function called EmplaceCoinInternalDANGER was adding the memory size of a coin to a running total even when the coin was not actually inserted because the same key already existed. That could slowly inflate the cache-size counter. The patch makes the counter increase only on a successful insert and also records the size before the coin is moved. The commit adds tests and fuzzing coverage to prevent the bug from returning. It is a correctness and resource-accounting fix rather than a direct theft or remote-code-execution vulnerability.
Treat as a low-severity correctness fix. Reviewers should confirm that all callers of EmplaceCoinInternalDANGER already guarantee non-duplicate keys, or that duplicate-key behavior is now harmless. The regression test and fuzz target should be run in CI. No emergency deployment is warranted, but backporting to maintained branches is reasonable because incorrect cache accounting could theoretically affect memory-limit behavior or trigger assertions in SelfTest().
Security signals we found
Resource-accounting imbalance in cache memory tracking
Function name contains DANGER, indicating internal unsafe API
Fix moves accounting inside successful-insert branch
Adds regression unit test and fuzz coverage
No direct cryptographic, network, or consensus code changed
Evidence from the diff
CCoinsViewCache::EmplaceCoinInternalDANGER uses cacheCoins.try_emplace to insert a Coin under a COutPoint. Before this commit, cachedCoinsUsage was incremented unconditionally with coin.DynamicMemoryUsage(), even when try_emplace returned inserted=false because the outpoint was already present. The new code stores mem_usage before the move, then increments cachedCoinsUsage only when inserted is true. A new unit test verifies that emplacing two different coins at the same outpoint leaves the first coin in place and passes SelfTest(), and the fuzz harness now exercises the EmplaceCoinInternalDANGER path. The bug is mostly reachable in tests today because AssumeUTXO does not overwrite, but the function is labeled DANGER precisely because callers are expected to manage duplicates themselves.
Changed components
src/coins.cpp - CCoinsViewCache::EmplaceCoinInternalDANGERsrc/test/coins_tests.cpp - new ccoins_emplace_duplicate_keeps_usage_balanced testsrc/test/fuzz/coins_view.cpp - fuzz path for EmplaceCoinInternalDANGERInspect captured patch +33 / −8
diff --git a/src/coins.cpp b/src/coins.cpp
index 42e83dab..090d36dd 100644
--- a/src/coins.cpp
+++ b/src/coins.cpp
@@ -111,9 +111,12 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi
}
void CCoinsViewCache::EmplaceCoinInternalDANGER(COutPoint&& outpoint, Coin&& coin) {
- cachedCoinsUsage += coin.DynamicMemoryUsage();
+ const auto mem_usage{coin.DynamicMemoryUsage()};
auto [it, inserted] = cacheCoins.try_emplace(std::move(outpoint), std::move(coin));
- if (inserted) CCoinsCacheEntry::SetDirty(*it, m_sentinel);
+ if (inserted) {
+ CCoinsCacheEntry::SetDirty(*it, m_sentinel);
+ cachedCoinsUsage += mem_usage;
+ }
}
void AddCoins(CCoinsViewCache& cache, const CTransaction &tx, int nHeight, bool check_for_overwrite) {
diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
index 46b1e2eb..a1152d24 100644
--- a/src/test/coins_tests.cpp
+++ b/src/test/coins_tests.cpp
@@ -1103,4 +1103,22 @@ BOOST_AUTO_TEST_CASE(ccoins_addcoin_exception_keeps_usage_balanced)
BOOST_CHECK(cache.AccessCoin(outpoint) == coin1);
}
+BOOST_AUTO_TEST_CASE(ccoins_emplace_duplicate_keeps_usage_balanced)
+{
+ CCoinsView root;
+ CCoinsViewCacheTest cache{&root};
+
+ const COutPoint outpoint{Txid::FromUint256(m_rng.rand256()), m_rng.rand32()};
+
+ const Coin coin1{CTxOut{m_rng.randrange(10), CScript{} << m_rng.randbytes(CScriptBase::STATIC_SIZE + 1)}, 1, false};
+ cache.EmplaceCoinInternalDANGER(COutPoint{outpoint}, Coin{coin1});
+ cache.SelfTest();
+
+ const Coin coin2{CTxOut{m_rng.randrange(20), CScript{} << m_rng.randbytes(CScriptBase::STATIC_SIZE + 2)}, 2, false};
+ cache.EmplaceCoinInternalDANGER(COutPoint{outpoint}, Coin{coin2});
+ cache.SelfTest();
+
+ BOOST_CHECK(cache.AccessCoin(outpoint) == coin1);
+}
+
BOOST_AUTO_TEST_SUITE_END()
diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp
index 2b3557ff..c6870654 100644
--- a/src/test/fuzz/coins_view.cpp
+++ b/src/test/fuzz/coins_view.cpp
@@ -61,12 +61,16 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend
}
COutPoint outpoint{random_out_point};
Coin coin{random_coin};
- const bool possible_overwrite{fuzzed_data_provider.ConsumeBool()};
- try {
- coins_view_cache.AddCoin(outpoint, std::move(coin), possible_overwrite);
- } catch (const std::logic_error& e) {
- assert(e.what() == std::string{"Attempted to overwrite an unspent coin (when possible_overwrite is false)"});
- assert(!possible_overwrite);
+ if (fuzzed_data_provider.ConsumeBool()) {
+ const bool possible_overwrite{fuzzed_data_provider.ConsumeBool()};
+ try {
+ coins_view_cache.AddCoin(outpoint, std::move(coin), possible_overwrite);
+ } catch (const std::logic_error& e) {
+ assert(e.what() == std::string{"Attempted to overwrite an unspent coin (when possible_overwrite is false)"});
+ assert(!possible_overwrite);
+ }
+ } else {
+ coins_view_cache.EmplaceCoinInternalDANGER(std::move(outpoint), std::move(coin));
}
},
[&] {
Why this scored 33/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.