diff --git a/src/coins.cpp b/src/coins.cpp index a65522839f2..25b1ead0c1d 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -113,8 +113,8 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi fresh = !it->second.IsDirty(); } if (!inserted) { - m_dirty_count -= it->second.IsDirty(); - cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage(); + Assume(TrySub(m_dirty_count, it->second.IsDirty())); + Assume(TrySub(cachedCoinsUsage, it->second.coin.DynamicMemoryUsage())); } it->second.coin = std::move(coin); CCoinsCacheEntry::SetDirty(*it, m_sentinel); @@ -153,8 +153,8 @@ void AddCoins(CCoinsViewCache& cache, const CTransaction &tx, int nHeight, bool bool CCoinsViewCache::SpendCoin(const COutPoint &outpoint, Coin* moveout) { CCoinsMap::iterator it = FetchCoin(outpoint); if (it == cacheCoins.end()) return false; - m_dirty_count -= it->second.IsDirty(); - cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage(); + Assume(TrySub(m_dirty_count, it->second.IsDirty())); + Assume(TrySub(cachedCoinsUsage, it->second.coin.DynamicMemoryUsage())); TRACEPOINT(utxocache, spent, outpoint.hash.data(), (uint32_t)outpoint.n, @@ -248,12 +248,12 @@ void CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256& ha if (itUs->second.IsFresh() && it->second.coin.IsSpent()) { // The grandparent cache does not have an entry, and the coin // has been spent. We can just delete it from the parent cache. - m_dirty_count -= itUs->second.IsDirty(); - cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage(); + Assume(TrySub(m_dirty_count, itUs->second.IsDirty())); + Assume(TrySub(cachedCoinsUsage, itUs->second.coin.DynamicMemoryUsage())); cacheCoins.erase(itUs); } else { // A normal modification. - cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage(); + Assume(TrySub(cachedCoinsUsage, itUs->second.coin.DynamicMemoryUsage())); if (cursor.WillErase(*it)) { // Since this entry will be erased, // we can move the coin into us instead of copying it @@ -311,7 +311,7 @@ void CCoinsViewCache::Uncache(const COutPoint& hash) { CCoinsMap::iterator it = cacheCoins.find(hash); if (it != cacheCoins.end() && !it->second.IsDirty()) { - cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage(); + Assume(TrySub(cachedCoinsUsage, it->second.coin.DynamicMemoryUsage())); TRACEPOINT(utxocache, uncache, hash.hash.data(), (uint32_t)hash.n, diff --git a/src/coins.h b/src/coins.h index ba23e3d3303..08c1886f930 100644 --- a/src/coins.h +++ b/src/coins.h @@ -15,6 +15,7 @@ #include #include #include +#include #include #include @@ -278,7 +279,7 @@ struct CoinsViewCacheCursor inline CoinsCachePair* NextAndMaybeErase(CoinsCachePair& current) noexcept { const auto next_entry{current.second.Next()}; - m_dirty_count -= current.second.IsDirty(); + Assume(TrySub(m_dirty_count, current.second.IsDirty())); // If we are not going to erase the cache, we must still erase spent entries. // Otherwise, clear the state of the entry. if (!m_will_erase) { diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index 47a0144f081..a47dc3706c8 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -77,18 +77,7 @@ public: { // Nothing must modify cacheCoins other than BatchWrite. assert(ComputeCacheCoinsSnapshot() == m_expected_snapshot); - try { - CCoinsViewCache::BatchWrite(cursor, block_hash); - } catch (const std::logic_error& e) { - // This error is thrown if the cursor contains a fresh entry for an outpoint that we already have a fresh - // entry for. This can happen if the fuzzer calls AddCoin -> Flush -> AddCoin -> Flush on the child cache. - // There's not an easy way to prevent the fuzzer from reaching this, so we handle it here. - // Since it is thrown in the middle of the write, we reset our own state and iterate through - // the cursor so the caller's state is also reset. - assert(e.what() == std::string{"FRESH flag misapplied to coin that exists in parent cache"}); - Reset(); - for (auto it{cursor.Begin()}; it != cursor.End(); it = cursor.NextAndMaybeErase(*it)) {} - } + CCoinsViewCache::BatchWrite(cursor, block_hash); m_expected_snapshot = ComputeCacheCoinsSnapshot(); } @@ -120,13 +109,9 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsViewCache& co 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); - } + // We can only skip the check if no unspent coin exists for this outpoint. + const bool possible_overwrite{coins_view_cache.PeekCoin(outpoint) || fuzzed_data_provider.ConsumeBool()}; + coins_view_cache.AddCoin(outpoint, std::move(coin), possible_overwrite); } else { coins_view_cache.EmplaceCoinInternalDANGER(std::move(outpoint), std::move(coin)); } @@ -203,8 +188,6 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsViewCache& co LIMITED_WHILE(good_data && fuzzed_data_provider.ConsumeBool(), 10'000) { CCoinsCacheEntry coins_cache_entry; - const auto dirty{fuzzed_data_provider.ConsumeBool()}; - const auto fresh{fuzzed_data_provider.ConsumeBool()}; if (fuzzed_data_provider.ConsumeBool()) { coins_cache_entry.coin = random_coin; } else { @@ -215,26 +198,20 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsViewCache& co } coins_cache_entry.coin = *opt_coin; } + // Avoid setting FRESH for an outpoint that already exists unspent in the parent view. + bool fresh{!coins_view_cache.PeekCoin(random_out_point) && fuzzed_data_provider.ConsumeBool()}; + bool dirty{fresh || fuzzed_data_provider.ConsumeBool()}; 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); dirty_count += dirty; } - bool expected_code_path = false; - try { - auto cursor{CoinsViewCacheCursor(dirty_count, 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(). - if (is_db && best_block.IsNull()) best_block = uint256::ONE; - coins_view_cache.BatchWrite(cursor, best_block); - expected_code_path = true; - } catch (const std::logic_error& e) { - if (e.what() == std::string{"FRESH flag misapplied to coin that exists in parent cache"}) { - expected_code_path = true; - } - } - assert(expected_code_path); + auto cursor{CoinsViewCacheCursor(dirty_count, 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(). + if (is_db && best_block.IsNull()) best_block = uint256::ONE; + coins_view_cache.BatchWrite(cursor, best_block); }); } @@ -280,19 +257,14 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsViewCache& co // coins.cpp:69: void CCoinsViewCache::AddCoin(const COutPoint &, Coin &&, bool): Assertion `!coin.IsSpent()' failed. return; } - bool expected_code_path = false; const int height{int(fuzzed_data_provider.ConsumeIntegral() >> 1)}; - const bool possible_overwrite = fuzzed_data_provider.ConsumeBool(); - try { - AddCoins(coins_view_cache, transaction, height, 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)"}) { - assert(!possible_overwrite); - expected_code_path = true; + const bool check_for_overwrite{transaction.IsCoinBase() || [&] { + for (uint32_t i{0}; i < transaction.vout.size(); ++i) { + if (coins_view_cache.PeekCoin(COutPoint{transaction.GetHash(), i})) return true; } - } - assert(expected_code_path); + return fuzzed_data_provider.ConsumeBool(); + }()}; // We can only skip the check if the current txid has no unspent outputs + AddCoins(coins_view_cache, transaction, height, check_for_overwrite); }, [&] { (void)AreInputsStandard(CTransaction{random_mutable_transaction}, coins_view_cache); diff --git a/src/util/overflow.h b/src/util/overflow.h index 8b8511f86f6..e7498fd8118 100644 --- a/src/util/overflow.h +++ b/src/util/overflow.h @@ -31,6 +31,14 @@ template return i + j; } +template +[[nodiscard]] constexpr bool TrySub(T& i, const U j) noexcept +{ + if (i < T{j}) return false; + i -= T{j}; + return true; +} + template [[nodiscard]] T SaturatingAdd(const T i, const T j) noexcept {