aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorAva Chow <github@achow101.com>2024-08-07 20:06:39 -0400
committerAva Chow <github@achow101.com>2024-08-07 20:06:39 -0400
commit27a770b34b8f1dbb84760f442edb3e23a0c2420b (patch)
tree2c05f979aa1137d0eb56de869c345331ee63cd30
parent0f68a05c084bef3e53e3f549c403bc90b1db319c (diff)
parent589db872e116779ab9cae693171ac8a8c02d9923 (diff)
Merge bitcoin/bitcoin#28280: Don't empty dbcache on prune flushes: >30% faster IBD
589db872e116779ab9cae693171ac8a8c02d9923 validation: don't erase coins cache on prune flushes (Andrew Toth) 0e8918755f725b6269ed2be5a0b46f1611233515 Add linked-list test to CCoinsViewCache::SanityCheck (Pieter Wuille) 05cf4e18758618bb493d26014d3a9c89bf28d898 coins: move Sync logic to CoinsViewCacheCursor (Andrew Toth) 7825b8b9aeceb4ff607650cdc9c49e5de9c7719f coins: pass linked list of flagged entries to BatchWrite (Andrew Toth) a14edada8a051e280af6fedd5130be40247e2d7a test: add cache entry linked list tests (Andrew Toth) 24ce37cb867b95e86d9fd4e50858d64ee8a59abf coins: track flagged cache entries in linked list (Andrew Toth) 58b7ed156d5993b69375bb455b03bd8b17f64fa4 coins: call ClearFlags in CCoinsCacheEntry destructor (Andrew Toth) 8bd3959feaa0e71585bc5e0976f584fb06ee6d14 refactor: require self and sentinel parameters for AddFlags (Andrew Toth) 75f36d241d2a344c5c4ce2c80250bdde91b3295e refactor: add CoinsCachePair alias (Andrew Toth) f08faeade2f99ae1de6f3c481697541cc16186c7 refactor: move flags to private uint8_t and rename to m_flags (Andrew Toth) 4e4fb4cbabcc10e723534f5f80f3e3cf09f6ca50 refactor: disallow setting flags in CCoinsCacheEntry constructors (Andrew Toth) 8737c0cefa6ec49a4d17d9bef9e5e1a7990af1ac refactor: encapsulate flags setting with AddFlags and ClearFlags (Andrew Toth) 9715d3bf1e4013476539f1523a6b54d2055c32c6 refactor: encapsulate flags get access for all other checks (Andrew Toth) df34a94e57659c31873c26c86a8de5a032400061 refactor: encapsulate flags access for dirty and fresh checks (Andrew Toth) Pull request description: Since https://github.com/bitcoin/bitcoin/pull/17487 we no longer need to clear the coins cache when syncing to disk. A warm coins cache significantly speeds up block connection, and only needs to be fully flushed when nearing the `dbcache` limit. For frequent pruning flushes there's no need to empty the cache and kill connect block speed. However, simply using `Sync` in place of `Flush` actually slows down a pruned full IBD with a high `dbcache` value. This is because as the cache grows, sync takes longer since every coin in the cache is scanned to check if it's dirty. For frequent prune flushes and a large cache this constant scanning starts to really slow IBD down, and just emptying the cache on every prune becomes faster. To fix this, we can add two pointers to each cache entry and construct a doubly linked list of dirty entries. We can then only iterate through all dirty entries on each `Sync`, and simply clear the pointers after. With this approach a full IBD with `dbcache=16384` and `prune=550` was 32% faster than master. For default `dbcache=450` speedup was ~9%. All benchmarks were run with `stopatheight=800000`. | | prune | dbcache | time | max RSS | speedup | |-----------:|----------:|------------:|--------:|-------------:|--------------:| | master | 550 | 16384 | 8:52:57 | 2,417,464k | - | | branch | 550 | 16384 | 6:01:00 | 16,216,736k | 32% | | branch | 550 | 450 | 8:05:08 | 2,818,072k | 8.8% | | master | 10000 | 5000 | 8:19:59 | 2,962,752k | - | | branch | 10000 | 5000| 5:56:39 | 6,179,764k | 28.8% | | master | 0 | 16384 | 4:51:53 | 14,726,408k | - | | branch | 0 | 16384 | 4:43:11 | 16,526,348k | 2.7% | | master | 0 | 450 | 7:08:07 | 3,005,892k | - | | branch | 0 | 450 | 6:57:24 | 3,013,556k |2.6%| While the 2 pointers add memory to each cache entry, it did not slow down IBD. For non-pruned IBD results were similar for this branch and master. When I performed the initial IBD, the full UTXO set could be held in memory when using the max `dbcache` value. For non-pruned IBD with max `dbcache` to tip ended up using 12% more memory, but it was also 2.7% faster somehow. For smaller `dbcache` values the `dbcache` limit is respected so does not consume more memory, and the potentially more frequent flushes were not significant enough to cause any slowdown. For reviewers, the commits in order do the following: First 4 commits encapsulate all accesses to `flags` on cache entries, and then the 5th makes `flags` private. Commits `refactor: add CoinsCachePair alias` to `coins: call ClearFlags in CCoinsCacheEntry destructor` create the linked list head nodes and cache entry self references and pass them into `AddFlags`. Commit `coins: track flagged cache entries in linked list` actually adds the entries into a linked list when they are flagged DIRTY or FRESH and removes them from the linked list when they are destroyed or the flags are cleared manually. However, the linked list is not yet used anywhere. Commit `test: add cache entry linked list tests` adds unit tests for the linked list. Commit `coins: pass linked list of flagged entries to BatchWrite` uses the linked list to iterate through DIRTY entries instead of using the entire coins cache. Commit `validation: don't erase coins cache on prune flushes` uses `Sync` instead of `Flush` for pruning flushes, so the cache is no longer cleared. Inspired by [this comment](https://github.com/bitcoin/bitcoin/pull/15265#issuecomment-457720636). Fixes https://github.com/bitcoin/bitcoin/issues/11315. ACKs for top commit: paplorinc: ACK 589db872e116779ab9cae693171ac8a8c02d9923 sipa: reACK 589db872e116779ab9cae693171ac8a8c02d9923 achow101: ACK 589db872e116779ab9cae693171ac8a8c02d9923 mzumsande: re-ACK 589db872e116779ab9cae693171ac8a8c02d9923 Tree-SHA512: 23b2bc01c83edacb5b39aa60bb0b766de9a74ce17f0c59bf13b97b4328a7b758ad9aff6581c3ca88e2973f7658380651530d497444f48d6e22ea0bfc51cc921d
-rw-r--r--src/Makefile.test.include1
-rw-r--r--src/coins.cpp110
-rw-r--r--src/coins.h153
-rw-r--r--src/test/coins_tests.cpp32
-rw-r--r--src/test/coinscachepair_tests.cpp219
-rw-r--r--src/test/fuzz/coins_view.cpp12
-rw-r--r--src/test/fuzz/coinscache_sim.cpp8
-rw-r--r--src/txdb.cpp8
-rw-r--r--src/txdb.h2
-rw-r--r--src/validation.cpp2
10 files changed, 463 insertions, 84 deletions
diff --git a/src/Makefile.test.include b/src/Makefile.test.include
index e2d6f94b24..3a6d56c9e1 100644
--- a/src/Makefile.test.include
+++ b/src/Makefile.test.include
@@ -85,6 +85,7 @@ BITCOIN_TESTS =\
test/checkqueue_tests.cpp \
test/cluster_linearize_tests.cpp \
test/coins_tests.cpp \
+ test/coinscachepair_tests.cpp \
test/coinstatsindex_tests.cpp \
test/common_url_tests.cpp \
test/compilerbug_tests.cpp \
diff --git a/src/coins.cpp b/src/coins.cpp
index a4e4d4ad32..91e02331f5 100644
--- a/src/coins.cpp
+++ b/src/coins.cpp
@@ -12,7 +12,7 @@
bool CCoinsView::GetCoin(const COutPoint &outpoint, Coin &coin) const { return false; }
uint256 CCoinsView::GetBestBlock() const { return uint256(); }
std::vector<uint256> CCoinsView::GetHeadBlocks() const { return std::vector<uint256>(); }
-bool CCoinsView::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase) { return false; }
+bool CCoinsView::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) { return false; }
std::unique_ptr<CCoinsViewCursor> CCoinsView::Cursor() const { return nullptr; }
bool CCoinsView::HaveCoin(const COutPoint &outpoint) const
@@ -27,14 +27,16 @@ bool CCoinsViewBacked::HaveCoin(const COutPoint &outpoint) const { return base->
uint256 CCoinsViewBacked::GetBestBlock() const { return base->GetBestBlock(); }
std::vector<uint256> CCoinsViewBacked::GetHeadBlocks() const { return base->GetHeadBlocks(); }
void CCoinsViewBacked::SetBackend(CCoinsView &viewIn) { base = &viewIn; }
-bool CCoinsViewBacked::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase) { return base->BatchWrite(mapCoins, hashBlock, erase); }
+bool CCoinsViewBacked::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) { return base->BatchWrite(cursor, hashBlock); }
std::unique_ptr<CCoinsViewCursor> CCoinsViewBacked::Cursor() const { return base->Cursor(); }
size_t CCoinsViewBacked::EstimateSize() const { return base->EstimateSize(); }
CCoinsViewCache::CCoinsViewCache(CCoinsView* baseIn, bool deterministic) :
CCoinsViewBacked(baseIn), m_deterministic(deterministic),
cacheCoins(0, SaltedOutpointHasher(/*deterministic=*/deterministic), CCoinsMap::key_equal{}, &m_cache_coins_memory_resource)
-{}
+{
+ m_sentinel.second.SelfRef(m_sentinel);
+}
size_t CCoinsViewCache::DynamicMemoryUsage() const {
return memusage::DynamicUsage(cacheCoins) + cachedCoinsUsage;
@@ -51,7 +53,7 @@ CCoinsMap::iterator CCoinsViewCache::FetchCoin(const COutPoint &outpoint) const
if (ret->second.coin.IsSpent()) {
// The parent only has an empty entry for this outpoint; we can consider our
// version as fresh.
- ret->second.flags = CCoinsCacheEntry::FRESH;
+ ret->second.AddFlags(CCoinsCacheEntry::FRESH, *ret, m_sentinel);
}
cachedCoinsUsage += ret->second.coin.DynamicMemoryUsage();
return ret;
@@ -93,10 +95,10 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi
//
// If the coin doesn't exist in the current cache, or is spent but not
// DIRTY, then it can be marked FRESH.
- fresh = !(it->second.flags & CCoinsCacheEntry::DIRTY);
+ fresh = !it->second.IsDirty();
}
it->second.coin = std::move(coin);
- it->second.flags |= CCoinsCacheEntry::DIRTY | (fresh ? CCoinsCacheEntry::FRESH : 0);
+ it->second.AddFlags(CCoinsCacheEntry::DIRTY | (fresh ? CCoinsCacheEntry::FRESH : 0), *it, m_sentinel);
cachedCoinsUsage += it->second.coin.DynamicMemoryUsage();
TRACE5(utxocache, add,
outpoint.hash.data(),
@@ -108,10 +110,13 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi
void CCoinsViewCache::EmplaceCoinInternalDANGER(COutPoint&& outpoint, Coin&& coin) {
cachedCoinsUsage += coin.DynamicMemoryUsage();
- cacheCoins.emplace(
+ auto [it, inserted] = cacheCoins.emplace(
std::piecewise_construct,
std::forward_as_tuple(std::move(outpoint)),
- std::forward_as_tuple(std::move(coin), CCoinsCacheEntry::DIRTY));
+ std::forward_as_tuple(std::move(coin)));
+ if (inserted) {
+ it->second.AddFlags(CCoinsCacheEntry::DIRTY, *it, m_sentinel);
+ }
}
void AddCoins(CCoinsViewCache& cache, const CTransaction &tx, int nHeight, bool check_for_overwrite) {
@@ -138,10 +143,10 @@ bool CCoinsViewCache::SpendCoin(const COutPoint &outpoint, Coin* moveout) {
if (moveout) {
*moveout = std::move(it->second.coin);
}
- if (it->second.flags & CCoinsCacheEntry::FRESH) {
+ if (it->second.IsFresh()) {
cacheCoins.erase(it);
} else {
- it->second.flags |= CCoinsCacheEntry::DIRTY;
+ it->second.AddFlags(CCoinsCacheEntry::DIRTY, *it, m_sentinel);
it->second.coin.Clear();
}
return true;
@@ -178,42 +183,40 @@ void CCoinsViewCache::SetBestBlock(const uint256 &hashBlockIn) {
hashBlock = hashBlockIn;
}
-bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn, bool erase) {
- for (CCoinsMap::iterator it = mapCoins.begin();
- it != mapCoins.end();
- it = erase ? mapCoins.erase(it) : std::next(it)) {
+bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlockIn) {
+ for (auto it{cursor.Begin()}; it != cursor.End(); it = cursor.NextAndMaybeErase(*it)) {
// Ignore non-dirty entries (optimization).
- if (!(it->second.flags & CCoinsCacheEntry::DIRTY)) {
+ if (!it->second.IsDirty()) {
continue;
}
CCoinsMap::iterator itUs = cacheCoins.find(it->first);
if (itUs == cacheCoins.end()) {
// The parent cache does not have an entry, while the child cache does.
// We can ignore it if it's both spent and FRESH in the child
- if (!(it->second.flags & CCoinsCacheEntry::FRESH && it->second.coin.IsSpent())) {
+ if (!(it->second.IsFresh() && it->second.coin.IsSpent())) {
// Create the coin in the parent cache, move the data up
// and mark it as dirty.
- CCoinsCacheEntry& entry = cacheCoins[it->first];
- if (erase) {
- // The `move` call here is purely an optimization; we rely on the
- // `mapCoins.erase` call in the `for` expression to actually remove
- // the entry from the child map.
+ itUs = cacheCoins.try_emplace(it->first).first;
+ CCoinsCacheEntry& entry{itUs->second};
+ if (cursor.WillErase(*it)) {
+ // Since this entry will be erased,
+ // we can move the coin into us instead of copying it
entry.coin = std::move(it->second.coin);
} else {
entry.coin = it->second.coin;
}
cachedCoinsUsage += entry.coin.DynamicMemoryUsage();
- entry.flags = CCoinsCacheEntry::DIRTY;
+ entry.AddFlags(CCoinsCacheEntry::DIRTY, *itUs, m_sentinel);
// We can mark it FRESH in the parent if it was FRESH in the child
// Otherwise it might have just been flushed from the parent's cache
// and already exist in the grandparent
- if (it->second.flags & CCoinsCacheEntry::FRESH) {
- entry.flags |= CCoinsCacheEntry::FRESH;
+ if (it->second.IsFresh()) {
+ entry.AddFlags(CCoinsCacheEntry::FRESH, *itUs, m_sentinel);
}
}
} else {
// Found the entry in the parent cache
- if ((it->second.flags & CCoinsCacheEntry::FRESH) && !itUs->second.coin.IsSpent()) {
+ if (it->second.IsFresh() && !itUs->second.coin.IsSpent()) {
// The coin was marked FRESH in the child cache, but the coin
// exists in the parent cache. If this ever happens, it means
// the FRESH flag was misapplied and there is a logic error in
@@ -221,7 +224,7 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn
throw std::logic_error("FRESH flag misapplied to coin that exists in parent cache");
}
- if ((itUs->second.flags & CCoinsCacheEntry::FRESH) && it->second.coin.IsSpent()) {
+ 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.
cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage();
@@ -229,16 +232,15 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn
} else {
// A normal modification.
cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage();
- if (erase) {
- // The `move` call here is purely an optimization; we rely on the
- // `mapCoins.erase` call in the `for` expression to actually remove
- // the entry from the child map.
+ if (cursor.WillErase(*it)) {
+ // Since this entry will be erased,
+ // we can move the coin into us instead of copying it
itUs->second.coin = std::move(it->second.coin);
} else {
itUs->second.coin = it->second.coin;
}
cachedCoinsUsage += itUs->second.coin.DynamicMemoryUsage();
- itUs->second.flags |= CCoinsCacheEntry::DIRTY;
+ itUs->second.AddFlags(CCoinsCacheEntry::DIRTY, *itUs, m_sentinel);
// NOTE: It isn't safe to mark the coin as FRESH in the parent
// cache. If it already existed and was spent in the parent
// cache then marking it FRESH would prevent that spentness
@@ -251,12 +253,10 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn
}
bool CCoinsViewCache::Flush() {
- bool fOk = base->BatchWrite(cacheCoins, hashBlock, /*erase=*/true);
+ auto cursor{CoinsViewCacheCursor(cachedCoinsUsage, m_sentinel, cacheCoins, /*will_erase=*/true)};
+ bool fOk = base->BatchWrite(cursor, hashBlock);
if (fOk) {
- if (!cacheCoins.empty()) {
- /* BatchWrite must erase all cacheCoins elements when erase=true. */
- throw std::logic_error("Not all cached coins were erased");
- }
+ cacheCoins.clear();
ReallocateCache();
}
cachedCoinsUsage = 0;
@@ -265,16 +265,12 @@ bool CCoinsViewCache::Flush() {
bool CCoinsViewCache::Sync()
{
- bool fOk = base->BatchWrite(cacheCoins, hashBlock, /*erase=*/false);
- // Instead of clearing `cacheCoins` as we would in Flush(), just clear the
- // FRESH/DIRTY flags of any coin that isn't spent.
- for (auto it = cacheCoins.begin(); it != cacheCoins.end(); ) {
- if (it->second.coin.IsSpent()) {
- cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage();
- it = cacheCoins.erase(it);
- } else {
- it->second.flags = 0;
- ++it;
+ auto cursor{CoinsViewCacheCursor(cachedCoinsUsage, m_sentinel, cacheCoins, /*will_erase=*/false)};
+ bool fOk = base->BatchWrite(cursor, hashBlock);
+ if (fOk) {
+ if (m_sentinel.second.Next() != &m_sentinel) {
+ /* BatchWrite must clear flags of all entries */
+ throw std::logic_error("Not all unspent flagged entries were cleared");
}
}
return fOk;
@@ -283,7 +279,7 @@ bool CCoinsViewCache::Sync()
void CCoinsViewCache::Uncache(const COutPoint& hash)
{
CCoinsMap::iterator it = cacheCoins.find(hash);
- if (it != cacheCoins.end() && it->second.flags == 0) {
+ if (it != cacheCoins.end() && !it->second.IsDirty() && !it->second.IsFresh()) {
cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage();
TRACE5(utxocache, uncache,
hash.hash.data(),
@@ -324,17 +320,33 @@ void CCoinsViewCache::ReallocateCache()
void CCoinsViewCache::SanityCheck() const
{
size_t recomputed_usage = 0;
+ size_t count_flagged = 0;
for (const auto& [_, entry] : cacheCoins) {
unsigned attr = 0;
- if (entry.flags & CCoinsCacheEntry::DIRTY) attr |= 1;
- if (entry.flags & CCoinsCacheEntry::FRESH) attr |= 2;
+ if (entry.IsDirty()) attr |= 1;
+ if (entry.IsFresh()) attr |= 2;
if (entry.coin.IsSpent()) attr |= 4;
// Only 5 combinations are possible.
assert(attr != 2 && attr != 4 && attr != 7);
// Recompute cachedCoinsUsage.
recomputed_usage += entry.coin.DynamicMemoryUsage();
+
+ // Count the number of entries we expect in the linked list.
+ if (entry.IsDirty() || entry.IsFresh()) ++count_flagged;
+ }
+ // Iterate over the linked list of flagged entries.
+ size_t count_linked = 0;
+ for (auto it = m_sentinel.second.Next(); it != &m_sentinel; it = it->second.Next()) {
+ // Verify linked list integrity.
+ assert(it->second.Next()->second.Prev() == it);
+ assert(it->second.Prev()->second.Next() == it);
+ // Verify they are actually flagged.
+ assert(it->second.IsDirty() || it->second.IsFresh());
+ // Count the number of entries actually in the list.
+ ++count_linked;
}
+ assert(count_linked == count_flagged);
assert(recomputed_usage == cachedCoinsUsage);
}
diff --git a/src/coins.h b/src/coins.h
index 76e64b641d..78b8eddacd 100644
--- a/src/coins.h
+++ b/src/coins.h
@@ -13,6 +13,7 @@
#include <serialize.h>
#include <support/allocators/pool.h>
#include <uint256.h>
+#include <util/check.h>
#include <util/hasher.h>
#include <assert.h>
@@ -86,6 +87,9 @@ public:
}
};
+struct CCoinsCacheEntry;
+using CoinsCachePair = std::pair<const COutPoint, CCoinsCacheEntry>;
+
/**
* A Coin in one level of the coins database caching hierarchy.
*
@@ -103,8 +107,29 @@ public:
*/
struct CCoinsCacheEntry
{
+private:
+ /**
+ * These are used to create a doubly linked list of flagged entries.
+ * They are set in AddFlags and unset in ClearFlags.
+ * A flagged entry is any entry that is either DIRTY, FRESH, or both.
+ *
+ * DIRTY entries are tracked so that only modified entries can be passed to
+ * the parent cache for batch writing. This is a performance optimization
+ * compared to giving all entries in the cache to the parent and having the
+ * parent scan for only modified entries.
+ *
+ * FRESH-but-not-DIRTY coins can not occur in practice, since that would
+ * mean a spent coin exists in the parent CCoinsView and not in the child
+ * CCoinsViewCache. Nevertheless, if a spent coin is retrieved from the
+ * parent cache, the FRESH-but-not-DIRTY coin will be tracked by the linked
+ * list and deleted when Sync or Flush is called on the CCoinsViewCache.
+ */
+ CoinsCachePair* m_prev{nullptr};
+ CoinsCachePair* m_next{nullptr};
+ uint8_t m_flags{0};
+
+public:
Coin coin; // The actual cached data.
- unsigned char flags;
enum Flags {
/**
@@ -127,9 +152,59 @@ struct CCoinsCacheEntry
FRESH = (1 << 1),
};
- CCoinsCacheEntry() : flags(0) {}
- explicit CCoinsCacheEntry(Coin&& coin_) : coin(std::move(coin_)), flags(0) {}
- CCoinsCacheEntry(Coin&& coin_, unsigned char flag) : coin(std::move(coin_)), flags(flag) {}
+ CCoinsCacheEntry() noexcept = default;
+ explicit CCoinsCacheEntry(Coin&& coin_) noexcept : coin(std::move(coin_)) {}
+ ~CCoinsCacheEntry()
+ {
+ ClearFlags();
+ }
+
+ //! Adding a flag also requires a self reference to the pair that contains
+ //! this entry in the CCoinsCache map and a reference to the sentinel of the
+ //! flagged pair linked list.
+ inline void AddFlags(uint8_t flags, CoinsCachePair& self, CoinsCachePair& sentinel) noexcept
+ {
+ Assume(&self.second == this);
+ if (!m_flags && flags) {
+ m_prev = sentinel.second.m_prev;
+ m_next = &sentinel;
+ sentinel.second.m_prev = &self;
+ m_prev->second.m_next = &self;
+ }
+ m_flags |= flags;
+ }
+ inline void ClearFlags() noexcept
+ {
+ if (!m_flags) return;
+ m_next->second.m_prev = m_prev;
+ m_prev->second.m_next = m_next;
+ m_flags = 0;
+ }
+ inline uint8_t GetFlags() const noexcept { return m_flags; }
+ inline bool IsDirty() const noexcept { return m_flags & DIRTY; }
+ inline bool IsFresh() const noexcept { return m_flags & FRESH; }
+
+ //! Only call Next when this entry is DIRTY, FRESH, or both
+ inline CoinsCachePair* Next() const noexcept {
+ Assume(m_flags);
+ return m_next;
+ }
+
+ //! Only call Prev when this entry is DIRTY, FRESH, or both
+ inline CoinsCachePair* Prev() const noexcept {
+ Assume(m_flags);
+ return m_prev;
+ }
+
+ //! Only use this for initializing the linked list sentinel
+ inline void SelfRef(CoinsCachePair& self) noexcept
+ {
+ Assume(&self.second == this);
+ m_prev = &self;
+ m_next = &self;
+ // Set sentinel to DIRTY so we can call Next on it
+ m_flags = DIRTY;
+ }
};
/**
@@ -144,8 +219,8 @@ using CCoinsMap = std::unordered_map<COutPoint,
CCoinsCacheEntry,
SaltedOutpointHasher,
std::equal_to<COutPoint>,
- PoolAllocator<std::pair<const COutPoint, CCoinsCacheEntry>,
- sizeof(std::pair<const COutPoint, CCoinsCacheEntry>) + sizeof(void*) * 4>>;
+ PoolAllocator<CoinsCachePair,
+ sizeof(CoinsCachePair) + sizeof(void*) * 4>>;
using CCoinsMapMemoryResource = CCoinsMap::allocator_type::ResourceType;
@@ -168,6 +243,62 @@ private:
uint256 hashBlock;
};
+/**
+ * Cursor for iterating over the linked list of flagged entries in CCoinsViewCache.
+ *
+ * This is a helper struct to encapsulate the diverging logic between a non-erasing
+ * CCoinsViewCache::Sync and an erasing CCoinsViewCache::Flush. This allows the receiver
+ * of CCoinsView::BatchWrite to iterate through the flagged entries without knowing
+ * the caller's intent.
+ *
+ * However, the receiver can still call CoinsViewCacheCursor::WillErase to see if the
+ * caller will erase the entry after BatchWrite returns. If so, the receiver can
+ * perform optimizations such as moving the coin out of the CCoinsCachEntry instead
+ * of copying it.
+ */
+struct CoinsViewCacheCursor
+{
+ //! If will_erase is not set, iterating through the cursor will erase spent coins from the map,
+ //! and other coins will be unflagged (removing them from the linked list).
+ //! If will_erase is set, the underlying map and linked list will not be modified,
+ //! as the caller is expected to wipe the entire map anyway.
+ //! 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) {}
+
+ inline CoinsCachePair* Begin() const noexcept { return m_sentinel.second.Next(); }
+ inline CoinsCachePair* End() const noexcept { return &m_sentinel; }
+
+ //! Return the next entry after current, possibly erasing current
+ inline CoinsCachePair* NextAndMaybeErase(CoinsCachePair& current) noexcept
+ {
+ const auto next_entry{current.second.Next()};
+ // If we are not going to erase the cache, we must still erase spent entries.
+ // Otherwise clear the flags on the entry.
+ if (!m_will_erase) {
+ if (current.second.coin.IsSpent()) {
+ m_usage -= current.second.coin.DynamicMemoryUsage();
+ m_map.erase(current.first);
+ } else {
+ current.second.ClearFlags();
+ }
+ }
+ return next_entry;
+ }
+
+ 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;
+};
+
/** Abstract view on the open txout dataset. */
class CCoinsView
{
@@ -191,8 +322,8 @@ public:
virtual std::vector<uint256> GetHeadBlocks() const;
//! Do a bulk modification (multiple Coin changes + BestBlock change).
- //! The passed mapCoins can be modified.
- virtual bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase = true);
+ //! The passed cursor is used to iterate through the coins.
+ virtual bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256& hashBlock);
//! Get a cursor to iterate over the whole state
virtual std::unique_ptr<CCoinsViewCursor> Cursor() const;
@@ -218,7 +349,7 @@ public:
uint256 GetBestBlock() const override;
std::vector<uint256> GetHeadBlocks() const override;
void SetBackend(CCoinsView &viewIn);
- bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase = true) override;
+ bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) override;
std::unique_ptr<CCoinsViewCursor> Cursor() const override;
size_t EstimateSize() const override;
};
@@ -237,6 +368,8 @@ protected:
*/
mutable uint256 hashBlock;
mutable CCoinsMapMemoryResource m_cache_coins_memory_resource{};
+ /* The starting sentinel of the flagged entry circular doubly linked list. */
+ mutable CoinsCachePair m_sentinel;
mutable CCoinsMap cacheCoins;
/* Cached dynamic memory usage for the inner Coin objects. */
@@ -255,7 +388,7 @@ public:
bool HaveCoin(const COutPoint &outpoint) const override;
uint256 GetBestBlock() const override;
void SetBestBlock(const uint256 &hashBlock);
- bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase = true) override;
+ bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) override;
std::unique_ptr<CCoinsViewCursor> Cursor() const override {
throw std::logic_error("CCoinsViewCache cursor iteration not supported.");
}
diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
index a992e2fa03..fb929a2b0e 100644
--- a/src/test/coins_tests.cpp
+++ b/src/test/coins_tests.cpp
@@ -55,10 +55,10 @@ public:
uint256 GetBestBlock() const override { return hashBestBlock_; }
- bool BatchWrite(CCoinsMap& mapCoins, const uint256& hashBlock, bool erase = true) override
+ bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256& hashBlock) override
{
- for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end(); it = erase ? mapCoins.erase(it) : std::next(it)) {
- if (it->second.flags & CCoinsCacheEntry::DIRTY) {
+ for (auto it{cursor.Begin()}; it != cursor.End(); it = cursor.NextAndMaybeErase(*it)){
+ if (it->second.IsDirty()) {
// Same optimization used in CCoinsViewDB is to only write dirty entries.
map_[it->first] = it->second.coin;
if (it->second.coin.IsSpent() && InsecureRandRange(3) == 0) {
@@ -78,7 +78,7 @@ class CCoinsViewCacheTest : public CCoinsViewCache
public:
explicit CCoinsViewCacheTest(CCoinsView* _base) : CCoinsViewCache(_base) {}
- void SelfTest() const
+ void SelfTest(bool sanity_check = true) const
{
// Manually recompute the dynamic usage of the whole data, and compare it.
size_t ret = memusage::DynamicUsage(cacheCoins);
@@ -89,9 +89,13 @@ public:
}
BOOST_CHECK_EQUAL(GetCacheSize(), count);
BOOST_CHECK_EQUAL(DynamicMemoryUsage(), ret);
+ if (sanity_check) {
+ SanityCheck();
+ }
}
CCoinsMap& map() const { return cacheCoins; }
+ CoinsCachePair& sentinel() const { return m_sentinel; }
size_t& usage() const { return cachedCoinsUsage; }
};
@@ -576,7 +580,7 @@ static void SetCoinsValue(CAmount value, Coin& coin)
}
}
-static size_t InsertCoinsMapEntry(CCoinsMap& map, CAmount value, char flags)
+static size_t InsertCoinsMapEntry(CCoinsMap& map, CoinsCachePair& sentinel, CAmount value, char flags)
{
if (value == ABSENT) {
assert(flags == NO_ENTRY);
@@ -584,10 +588,10 @@ static size_t InsertCoinsMapEntry(CCoinsMap& map, CAmount value, char flags)
}
assert(flags != NO_ENTRY);
CCoinsCacheEntry entry;
- entry.flags = flags;
SetCoinsValue(value, entry.coin);
auto inserted = map.emplace(OUTPOINT, std::move(entry));
assert(inserted.second);
+ inserted.first->second.AddFlags(flags, *inserted.first, sentinel);
return inserted.first->second.coin.DynamicMemoryUsage();
}
@@ -603,17 +607,20 @@ void GetCoinsMapEntry(const CCoinsMap& map, CAmount& value, char& flags, const C
} else {
value = it->second.coin.out.nValue;
}
- flags = it->second.flags;
+ flags = it->second.GetFlags();
assert(flags != NO_ENTRY);
}
}
void WriteCoinsViewEntry(CCoinsView& view, CAmount value, char flags)
{
+ CoinsCachePair sentinel{};
+ sentinel.second.SelfRef(sentinel);
CCoinsMapMemoryResource resource;
CCoinsMap map{0, CCoinsMap::hasher{}, CCoinsMap::key_equal{}, &resource};
- InsertCoinsMapEntry(map, value, flags);
- BOOST_CHECK(view.BatchWrite(map, {}));
+ auto usage{InsertCoinsMapEntry(map, sentinel, value, flags)};
+ auto cursor{CoinsViewCacheCursor(usage, sentinel, map, /*will_erase=*/true)};
+ BOOST_CHECK(view.BatchWrite(cursor, {}));
}
class SingleEntryCacheTest
@@ -622,7 +629,7 @@ public:
SingleEntryCacheTest(CAmount base_value, CAmount cache_value, char cache_flags)
{
WriteCoinsViewEntry(base, base_value, base_value == ABSENT ? NO_ENTRY : DIRTY);
- cache.usage() += InsertCoinsMapEntry(cache.map(), cache_value, cache_flags);
+ cache.usage() += InsertCoinsMapEntry(cache.map(), cache.sentinel(), cache_value, cache_flags);
}
CCoinsView root;
@@ -634,7 +641,7 @@ static void CheckAccessCoin(CAmount base_value, CAmount cache_value, CAmount exp
{
SingleEntryCacheTest test(base_value, cache_value, cache_flags);
test.cache.AccessCoin(OUTPOINT);
- test.cache.SelfTest();
+ test.cache.SelfTest(/*sanity_check=*/false);
CAmount result_value;
char result_flags;
@@ -803,7 +810,7 @@ void CheckWriteCoins(CAmount parent_value, CAmount child_value, CAmount expected
char result_flags;
try {
WriteCoinsViewEntry(test.cache, child_value, child_flags);
- test.cache.SelfTest();
+ test.cache.SelfTest(/*sanity_check=*/false);
GetCoinsMapEntry(test.cache.map(), result_value, result_flags);
} catch (std::logic_error&) {
result_value = FAIL;
@@ -916,6 +923,7 @@ void TestFlushBehavior(
// Flush in reverse order to ensure that flushes happen from children up.
for (auto i = all_caches.rbegin(); i != all_caches.rend(); ++i) {
auto& cache = *i;
+ cache->SanityCheck();
// hashBlock must be filled before flushing to disk; value is
// unimportant here. This is normally done during connect/disconnect block.
cache->SetBestBlock(InsecureRand256());
diff --git a/src/test/coinscachepair_tests.cpp b/src/test/coinscachepair_tests.cpp
new file mode 100644
index 0000000000..61840f1f09
--- /dev/null
+++ b/src/test/coinscachepair_tests.cpp
@@ -0,0 +1,219 @@
+// Copyright (c) 2024-present The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <coins.h>
+
+#include <boost/test/unit_test.hpp>
+
+#include <list>
+
+BOOST_AUTO_TEST_SUITE(coinscachepair_tests)
+
+static constexpr auto NUM_NODES{4};
+
+std::list<CoinsCachePair> CreatePairs(CoinsCachePair& sentinel)
+{
+ std::list<CoinsCachePair> nodes;
+ for (auto i{0}; i < NUM_NODES; ++i) {
+ nodes.emplace_back();
+
+ auto node{std::prev(nodes.end())};
+ node->second.AddFlags(CCoinsCacheEntry::DIRTY, *node, sentinel);
+
+ BOOST_CHECK_EQUAL(node->second.GetFlags(), CCoinsCacheEntry::DIRTY);
+ BOOST_CHECK_EQUAL(node->second.Next(), &sentinel);
+ BOOST_CHECK_EQUAL(sentinel.second.Prev(), &(*node));
+
+ if (i > 0) {
+ BOOST_CHECK_EQUAL(std::prev(node)->second.Next(), &(*node));
+ BOOST_CHECK_EQUAL(node->second.Prev(), &(*std::prev(node)));
+ }
+ }
+ return nodes;
+}
+
+BOOST_AUTO_TEST_CASE(linked_list_iteration)
+{
+ CoinsCachePair sentinel;
+ sentinel.second.SelfRef(sentinel);
+ auto nodes{CreatePairs(sentinel)};
+
+ // Check iterating through pairs is identical to iterating through a list
+ auto node{sentinel.second.Next()};
+ for (const auto& expected : nodes) {
+ BOOST_CHECK_EQUAL(&expected, node);
+ node = node->second.Next();
+ }
+ BOOST_CHECK_EQUAL(node, &sentinel);
+
+ // Check iterating through pairs is identical to iterating through a list
+ // Clear the flags during iteration
+ node = sentinel.second.Next();
+ for (const auto& expected : nodes) {
+ BOOST_CHECK_EQUAL(&expected, node);
+ auto next = node->second.Next();
+ node->second.ClearFlags();
+ node = next;
+ }
+ BOOST_CHECK_EQUAL(node, &sentinel);
+ // Check that sentinel's next and prev are itself
+ BOOST_CHECK_EQUAL(sentinel.second.Next(), &sentinel);
+ BOOST_CHECK_EQUAL(sentinel.second.Prev(), &sentinel);
+
+ // Delete the nodes from the list to make sure there are no dangling pointers
+ for (auto it{nodes.begin()}; it != nodes.end(); it = nodes.erase(it)) {
+ BOOST_CHECK_EQUAL(it->second.GetFlags(), 0);
+ }
+}
+
+BOOST_AUTO_TEST_CASE(linked_list_iterate_erase)
+{
+ CoinsCachePair sentinel;
+ sentinel.second.SelfRef(sentinel);
+ auto nodes{CreatePairs(sentinel)};
+
+ // Check iterating through pairs is identical to iterating through a list
+ // Erase the nodes as we iterate through, but don't clear flags
+ // The flags will be cleared by the CCoinsCacheEntry's destructor
+ auto node{sentinel.second.Next()};
+ for (auto expected{nodes.begin()}; expected != nodes.end(); expected = nodes.erase(expected)) {
+ BOOST_CHECK_EQUAL(&(*expected), node);
+ node = node->second.Next();
+ }
+ BOOST_CHECK_EQUAL(node, &sentinel);
+
+ // Check that sentinel's next and prev are itself
+ BOOST_CHECK_EQUAL(sentinel.second.Next(), &sentinel);
+ BOOST_CHECK_EQUAL(sentinel.second.Prev(), &sentinel);
+}
+
+BOOST_AUTO_TEST_CASE(linked_list_random_deletion)
+{
+ CoinsCachePair sentinel;
+ sentinel.second.SelfRef(sentinel);
+ auto nodes{CreatePairs(sentinel)};
+
+ // Create linked list sentinel->n1->n2->n3->n4->sentinel
+ auto n1{nodes.begin()};
+ auto n2{std::next(n1)};
+ auto n3{std::next(n2)};
+ auto n4{std::next(n3)};
+
+ // Delete n2
+ // sentinel->n1->n3->n4->sentinel
+ nodes.erase(n2);
+ // Check that n1 now points to n3, and n3 still points to n4
+ // Also check that flags were not altered
+ BOOST_CHECK_EQUAL(n1->second.GetFlags(), CCoinsCacheEntry::DIRTY);
+ BOOST_CHECK_EQUAL(n1->second.Next(), &(*n3));
+ BOOST_CHECK_EQUAL(n3->second.GetFlags(), CCoinsCacheEntry::DIRTY);
+ BOOST_CHECK_EQUAL(n3->second.Next(), &(*n4));
+ BOOST_CHECK_EQUAL(n3->second.Prev(), &(*n1));
+
+ // Delete n1
+ // sentinel->n3->n4->sentinel
+ nodes.erase(n1);
+ // Check that sentinel now points to n3, and n3 still points to n4
+ // Also check that flags were not altered
+ BOOST_CHECK_EQUAL(n3->second.GetFlags(), CCoinsCacheEntry::DIRTY);
+ BOOST_CHECK_EQUAL(sentinel.second.Next(), &(*n3));
+ BOOST_CHECK_EQUAL(n3->second.Next(), &(*n4));
+ BOOST_CHECK_EQUAL(n3->second.Prev(), &sentinel);
+
+ // Delete n4
+ // sentinel->n3->sentinel
+ nodes.erase(n4);
+ // Check that sentinel still points to n3, and n3 points to sentinel
+ // Also check that flags were not altered
+ BOOST_CHECK_EQUAL(n3->second.GetFlags(), CCoinsCacheEntry::DIRTY);
+ BOOST_CHECK_EQUAL(sentinel.second.Next(), &(*n3));
+ BOOST_CHECK_EQUAL(n3->second.Next(), &sentinel);
+ BOOST_CHECK_EQUAL(sentinel.second.Prev(), &(*n3));
+
+ // Delete n3
+ // sentinel->sentinel
+ nodes.erase(n3);
+ // Check that sentinel's next and prev are itself
+ BOOST_CHECK_EQUAL(sentinel.second.Next(), &sentinel);
+ BOOST_CHECK_EQUAL(sentinel.second.Prev(), &sentinel);
+}
+
+BOOST_AUTO_TEST_CASE(linked_list_add_flags)
+{
+ CoinsCachePair sentinel;
+ sentinel.second.SelfRef(sentinel);
+ CoinsCachePair n1;
+ CoinsCachePair n2;
+
+ // Check that adding 0 flag has no effect
+ n1.second.AddFlags(0, n1, sentinel);
+ BOOST_CHECK_EQUAL(n1.second.GetFlags(), 0);
+ BOOST_CHECK_EQUAL(sentinel.second.Next(), &sentinel);
+ BOOST_CHECK_EQUAL(sentinel.second.Prev(), &sentinel);
+
+ // Check that adding DIRTY flag inserts it into linked list and sets flags
+ n1.second.AddFlags(CCoinsCacheEntry::DIRTY, n1, sentinel);
+ BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY);
+ BOOST_CHECK_EQUAL(n1.second.Next(), &sentinel);
+ BOOST_CHECK_EQUAL(n1.second.Prev(), &sentinel);
+ BOOST_CHECK_EQUAL(sentinel.second.Next(), &n1);
+ BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n1);
+
+ // Check that adding FRESH flag on new node inserts it after n1
+ n2.second.AddFlags(CCoinsCacheEntry::FRESH, n2, sentinel);
+ BOOST_CHECK_EQUAL(n2.second.GetFlags(), CCoinsCacheEntry::FRESH);
+ BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel);
+ BOOST_CHECK_EQUAL(n2.second.Prev(), &n1);
+ BOOST_CHECK_EQUAL(n1.second.Next(), &n2);
+ BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2);
+
+ // Check that adding 0 flag has no effect, and doesn't change position
+ n1.second.AddFlags(0, n1, sentinel);
+ BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY);
+ BOOST_CHECK_EQUAL(n1.second.Next(), &n2);
+ BOOST_CHECK_EQUAL(n1.second.Prev(), &sentinel);
+ BOOST_CHECK_EQUAL(sentinel.second.Next(), &n1);
+ BOOST_CHECK_EQUAL(n2.second.Prev(), &n1);
+
+ // Check that we can add extra flags, but they don't change our position
+ n1.second.AddFlags(CCoinsCacheEntry::FRESH, n1, sentinel);
+ BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY | CCoinsCacheEntry::FRESH);
+ BOOST_CHECK_EQUAL(n1.second.Next(), &n2);
+ BOOST_CHECK_EQUAL(n1.second.Prev(), &sentinel);
+ BOOST_CHECK_EQUAL(sentinel.second.Next(), &n1);
+ BOOST_CHECK_EQUAL(n2.second.Prev(), &n1);
+
+ // Check that we can clear flags then re-add them
+ n1.second.ClearFlags();
+ BOOST_CHECK_EQUAL(n1.second.GetFlags(), 0);
+ BOOST_CHECK_EQUAL(sentinel.second.Next(), &n2);
+ BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2);
+ BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel);
+ BOOST_CHECK_EQUAL(n2.second.Prev(), &sentinel);
+
+ // Check that calling `ClearFlags` with 0 flags has no effect
+ n1.second.ClearFlags();
+ BOOST_CHECK_EQUAL(n1.second.GetFlags(), 0);
+ BOOST_CHECK_EQUAL(sentinel.second.Next(), &n2);
+ BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2);
+ BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel);
+ BOOST_CHECK_EQUAL(n2.second.Prev(), &sentinel);
+
+ // Adding 0 still has no effect
+ n1.second.AddFlags(0, n1, sentinel);
+ BOOST_CHECK_EQUAL(sentinel.second.Next(), &n2);
+ BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2);
+ BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel);
+ BOOST_CHECK_EQUAL(n2.second.Prev(), &sentinel);
+
+ // But adding DIRTY re-inserts it after n2
+ n1.second.AddFlags(CCoinsCacheEntry::DIRTY, n1, sentinel);
+ BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY);
+ BOOST_CHECK_EQUAL(n2.second.Next(), &n1);
+ BOOST_CHECK_EQUAL(n1.second.Prev(), &n2);
+ BOOST_CHECK_EQUAL(n1.second.Next(), &sentinel);
+ BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n1);
+}
+
+BOOST_AUTO_TEST_SUITE_END()
diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp
index 41c971c237..368c69819a 100644
--- a/src/test/fuzz/coins_view.cpp
+++ b/src/test/fuzz/coins_view.cpp
@@ -120,12 +120,15 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view)
random_mutable_transaction = *opt_mutable_transaction;
},
[&] {
+ 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)
{
CCoinsCacheEntry coins_cache_entry;
- coins_cache_entry.flags = fuzzed_data_provider.ConsumeIntegral<unsigned char>();
+ const auto flags{fuzzed_data_provider.ConsumeIntegral<uint8_t>()};
if (fuzzed_data_provider.ConsumeBool()) {
coins_cache_entry.coin = random_coin;
} else {
@@ -136,11 +139,14 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view)
}
coins_cache_entry.coin = *opt_coin;
}
- coins_map.emplace(random_out_point, std::move(coins_cache_entry));
+ auto it{coins_map.emplace(random_out_point, std::move(coins_cache_entry)).first};
+ it->second.AddFlags(flags, *it, sentinel);
+ usage += it->second.coin.DynamicMemoryUsage();
}
bool expected_code_path = false;
try {
- coins_view_cache.BatchWrite(coins_map, fuzzed_data_provider.ConsumeBool() ? ConsumeUInt256(fuzzed_data_provider) : coins_view_cache.GetBestBlock());
+ auto cursor{CoinsViewCacheCursor(usage, sentinel, coins_map, /*will_erase=*/true)};
+ coins_view_cache.BatchWrite(cursor, fuzzed_data_provider.ConsumeBool() ? ConsumeUInt256(fuzzed_data_provider) : coins_view_cache.GetBestBlock());
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"}) {
diff --git a/src/test/fuzz/coinscache_sim.cpp b/src/test/fuzz/coinscache_sim.cpp
index 648e96b4a0..8e717e96b4 100644
--- a/src/test/fuzz/coinscache_sim.cpp
+++ b/src/test/fuzz/coinscache_sim.cpp
@@ -172,13 +172,13 @@ public:
std::unique_ptr<CCoinsViewCursor> Cursor() const final { return {}; }
size_t EstimateSize() const final { return m_data.size(); }
- bool BatchWrite(CCoinsMap& data, const uint256&, bool erase) final
+ bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256&) final
{
- for (auto it = data.begin(); it != data.end(); it = erase ? data.erase(it) : std::next(it)) {
- if (it->second.flags & CCoinsCacheEntry::DIRTY) {
+ for (auto it{cursor.Begin()}; it != cursor.End(); it = cursor.NextAndMaybeErase(*it)) {
+ if (it->second.IsDirty()) {
if (it->second.coin.IsSpent() && (it->first.n % 5) != 4) {
m_data.erase(it->first);
- } else if (erase) {
+ } else if (cursor.WillErase(*it)) {
m_data[it->first] = std::move(it->second.coin);
} else {
m_data[it->first] = it->second.coin;
diff --git a/src/txdb.cpp b/src/txdb.cpp
index e4a4b3bf72..3865692b6a 100644
--- a/src/txdb.cpp
+++ b/src/txdb.cpp
@@ -88,7 +88,7 @@ std::vector<uint256> CCoinsViewDB::GetHeadBlocks() const {
return vhashHeadBlocks;
}
-bool CCoinsViewDB::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase) {
+bool CCoinsViewDB::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) {
CDBBatch batch(*m_db);
size_t count = 0;
size_t changed = 0;
@@ -114,8 +114,8 @@ bool CCoinsViewDB::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, boo
batch.Erase(DB_BEST_BLOCK);
batch.Write(DB_HEAD_BLOCKS, Vector(hashBlock, old_tip));
- for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end();) {
- if (it->second.flags & CCoinsCacheEntry::DIRTY) {
+ for (auto it{cursor.Begin()}; it != cursor.End();) {
+ if (it->second.IsDirty()) {
CoinEntry entry(&it->first);
if (it->second.coin.IsSpent())
batch.Erase(entry);
@@ -124,7 +124,7 @@ bool CCoinsViewDB::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, boo
changed++;
}
count++;
- it = erase ? mapCoins.erase(it) : std::next(it);
+ it = cursor.NextAndMaybeErase(*it);
if (batch.SizeEstimate() > m_options.batch_write_bytes) {
LogPrint(BCLog::COINDB, "Writing partial batch of %.2f MiB\n", batch.SizeEstimate() * (1.0 / 1048576.0));
m_db->WriteBatch(batch);
diff --git a/src/txdb.h b/src/txdb.h
index c9af0a091e..e0acb09e98 100644
--- a/src/txdb.h
+++ b/src/txdb.h
@@ -63,7 +63,7 @@ public:
bool HaveCoin(const COutPoint &outpoint) const override;
uint256 GetBestBlock() const override;
std::vector<uint256> GetHeadBlocks() const override;
- bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase = true) override;
+ bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) override;
std::unique_ptr<CCoinsViewCursor> Cursor() const override;
//! Whether an unsupported database format is used.
diff --git a/src/validation.cpp b/src/validation.cpp
index f7250778fc..219e72f1b1 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2902,7 +2902,7 @@ bool Chainstate::FlushStateToDisk(
return FatalError(m_chainman.GetNotifications(), state, _("Disk space is too low!"));
}
// Flush the chainstate (which may refer to block index entries).
- const auto empty_cache{(mode == FlushStateMode::ALWAYS) || fCacheLarge || fCacheCritical || fFlushForPrune};
+ const auto empty_cache{(mode == FlushStateMode::ALWAYS) || fCacheLarge || fCacheCritical};
if (empty_cache ? !CoinsTip().Flush() : !CoinsTip().Sync()) {
return FatalError(m_chainman.GetNotifications(), state, _("Failed to write to coin database."));
}