diff --git a/src/coins.cpp b/src/coins.cpp index 24a102b0bc1..090d36dd041 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -76,9 +76,6 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi bool inserted; std::tie(it, inserted) = cacheCoins.emplace(std::piecewise_construct, std::forward_as_tuple(outpoint), std::tuple<>()); bool fresh = false; - if (!inserted) { - cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage(); - } if (!possible_overwrite) { if (!it->second.coin.IsSpent()) { throw std::logic_error("Attempted to overwrite an unspent coin (when possible_overwrite is false)"); @@ -98,6 +95,9 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi // DIRTY, then it can be marked FRESH. fresh = !it->second.IsDirty(); } + if (!inserted) { + cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage(); + } it->second.coin = std::move(coin); CCoinsCacheEntry::SetDirty(*it, m_sentinel); if (fresh) CCoinsCacheEntry::SetFresh(*it, m_sentinel); @@ -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) { @@ -195,6 +198,7 @@ bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &ha // and mark it as dirty. itUs = cacheCoins.try_emplace(it->first).first; CCoinsCacheEntry& entry{itUs->second}; + assert(entry.coin.DynamicMemoryUsage() == 0); if (cursor.WillErase(*it)) { // Since this entry will be erased, // we can move the coin into us instead of copying it @@ -248,19 +252,19 @@ bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &ha } bool CCoinsViewCache::Flush() { - auto cursor{CoinsViewCacheCursor(cachedCoinsUsage, m_sentinel, cacheCoins, /*will_erase=*/true)}; + auto cursor{CoinsViewCacheCursor(m_sentinel, cacheCoins, /*will_erase=*/true)}; bool fOk = base->BatchWrite(cursor, hashBlock); if (fOk) { cacheCoins.clear(); ReallocateCache(); + cachedCoinsUsage = 0; } - cachedCoinsUsage = 0; return fOk; } bool CCoinsViewCache::Sync() { - auto cursor{CoinsViewCacheCursor(cachedCoinsUsage, m_sentinel, cacheCoins, /*will_erase=*/false)}; + auto cursor{CoinsViewCacheCursor(m_sentinel, cacheCoins, /*will_erase=*/false)}; bool fOk = base->BatchWrite(cursor, hashBlock); if (fOk) { if (m_sentinel.second.Next() != &m_sentinel) { diff --git a/src/coins.h b/src/coins.h index 6725d5a51f6..2fcc764a3fd 100644 --- a/src/coins.h +++ b/src/coins.h @@ -271,11 +271,10 @@ struct CoinsViewCacheCursor //! This is an optimization compared to erasing all entries as the cursor iterates them when will_erase is set. //! Calling CCoinsMap::clear() afterwards is faster because a CoinsCachePair cannot be coerced back into a //! CCoinsMap::iterator to be erased, and must therefore be looked up again by key in the CCoinsMap before being erased. - CoinsViewCacheCursor(size_t& usage LIFETIMEBOUND, - CoinsCachePair& sentinel LIFETIMEBOUND, - CCoinsMap& map LIFETIMEBOUND, - bool will_erase) noexcept - : m_usage(usage), m_sentinel(sentinel), m_map(map), m_will_erase(will_erase) {} + CoinsViewCacheCursor(CoinsCachePair& sentinel LIFETIMEBOUND, + CCoinsMap& map LIFETIMEBOUND, + bool will_erase) noexcept + : m_sentinel(sentinel), m_map(map), m_will_erase(will_erase) {} inline CoinsCachePair* Begin() const noexcept { return m_sentinel.second.Next(); } inline CoinsCachePair* End() const noexcept { return &m_sentinel; } @@ -288,7 +287,7 @@ struct CoinsViewCacheCursor // Otherwise, clear the state of the entry. if (!m_will_erase) { if (current.second.coin.IsSpent()) { - m_usage -= current.second.coin.DynamicMemoryUsage(); + assert(current.second.coin.DynamicMemoryUsage() == 0); // scriptPubKey was already cleared in SpendCoin m_map.erase(current.first); } else { current.second.SetClean(); @@ -299,7 +298,6 @@ struct CoinsViewCacheCursor inline bool WillErase(CoinsCachePair& current) const noexcept { return m_will_erase || current.second.coin.IsSpent(); } private: - size_t& m_usage; CoinsCachePair& m_sentinel; CCoinsMap& m_map; bool m_will_erase; diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index 6ce0c7996f8..a1152d245f7 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -662,8 +662,8 @@ static void WriteCoinsViewEntry(CCoinsView& view, const MaybeCoin& cache_coin) sentinel.second.SelfRef(sentinel); CCoinsMapMemoryResource resource; CCoinsMap map{0, CCoinsMap::hasher{}, CCoinsMap::key_equal{}, &resource}; - auto usage{cache_coin ? InsertCoinsMapEntry(map, sentinel, *cache_coin) : 0}; - auto cursor{CoinsViewCacheCursor(usage, sentinel, map, /*will_erase=*/true)}; + if (cache_coin) InsertCoinsMapEntry(map, sentinel, *cache_coin); + auto cursor{CoinsViewCacheCursor(sentinel, map, /*will_erase=*/true)}; BOOST_CHECK(view.BatchWrite(cursor, {})); } @@ -1085,4 +1085,40 @@ BOOST_AUTO_TEST_CASE(coins_resource_is_used) PoolResourceTester::CheckAllDataAccountedFor(resource); } +BOOST_AUTO_TEST_CASE(ccoins_addcoin_exception_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.AddCoin(outpoint, Coin{coin1}, /*possible_overwrite=*/false); + cache.SelfTest(); + + const Coin coin2{CTxOut{m_rng.randrange(20), CScript{} << m_rng.randbytes(CScriptBase::STATIC_SIZE + 2)}, 2, false}; + BOOST_CHECK_THROW(cache.AddCoin(outpoint, Coin{coin2}, /*possible_overwrite=*/false), std::logic_error); + cache.SelfTest(); + + 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 dacef9a70ad..c687065423a 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -59,25 +59,19 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend if (random_coin.IsSpent()) { return; } - Coin coin = random_coin; - bool expected_code_path = false; - const bool possible_overwrite = fuzzed_data_provider.ConsumeBool(); - try { - coins_view_cache.AddCoin(random_out_point, std::move(coin), possible_overwrite); - expected_code_path = true; - } catch (const std::logic_error& e) { - if (e.what() == std::string{"Attempted to overwrite an unspent coin (when possible_overwrite is false)"}) { + COutPoint outpoint{random_out_point}; + Coin coin{random_coin}; + 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); - expected_code_path = true; - // AddCoin() decreases cachedCoinsUsage by the memory usage of the old coin at the beginning and - // increases it by the value of the new coin at the end. If it throws in the process, the value - // of cachedCoinsUsage would have been incorrectly decreased, leading to an underflow later on. - // To avoid this, use Flush() to reset the value of cachedCoinsUsage in sync with the cacheCoins - // mapping. - (void)coins_view_cache.Flush(); } + } else { + coins_view_cache.EmplaceCoinInternalDANGER(std::move(outpoint), std::move(coin)); } - assert(expected_code_path); }, [&] { (void)coins_view_cache.Flush(); @@ -131,7 +125,6 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend [&] { CoinsCachePair sentinel{}; sentinel.second.SelfRef(sentinel); - size_t usage{0}; CCoinsMapMemoryResource resource; CCoinsMap coins_map{0, SaltedOutpointHasher{/*deterministic=*/true}, CCoinsMap::key_equal{}, &resource}; LIMITED_WHILE(good_data && fuzzed_data_provider.ConsumeBool(), 10'000) @@ -152,11 +145,10 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend auto it{coins_map.emplace(random_out_point, std::move(coins_cache_entry)).first}; if (dirty) CCoinsCacheEntry::SetDirty(*it, sentinel); if (fresh) CCoinsCacheEntry::SetFresh(*it, sentinel); - usage += it->second.coin.DynamicMemoryUsage(); } bool expected_code_path = false; try { - auto cursor{CoinsViewCacheCursor(usage, sentinel, coins_map, /*will_erase=*/true)}; + auto cursor{CoinsViewCacheCursor(sentinel, coins_map, /*will_erase=*/true)}; uint256 best_block{coins_view_cache.GetBestBlock()}; if (fuzzed_data_provider.ConsumeBool()) best_block = ConsumeUInt256(fuzzed_data_provider); // Set best block hash to non-null to satisfy the assertion in CCoinsViewDB::BatchWrite(). diff --git a/test/sanitizer_suppressions/ubsan b/test/sanitizer_suppressions/ubsan index 77aed1aa8a1..0151f9d0253 100644 --- a/test/sanitizer_suppressions/ubsan +++ b/test/sanitizer_suppressions/ubsan @@ -47,11 +47,6 @@ unsigned-integer-overflow:arith_uint256.h unsigned-integer-overflow:CBloomFilter::Hash unsigned-integer-overflow:CRollingBloomFilter::insert unsigned-integer-overflow:RollingBloomHash -unsigned-integer-overflow:CCoinsViewCache::AddCoin -unsigned-integer-overflow:CCoinsViewCache::BatchWrite -unsigned-integer-overflow:CCoinsViewCache::DynamicMemoryUsage -unsigned-integer-overflow:CCoinsViewCache::SpendCoin -unsigned-integer-overflow:CCoinsViewCache::Uncache unsigned-integer-overflow:CompressAmount unsigned-integer-overflow:DecompressAmount unsigned-integer-overflow:crypto/