Skip to content

Commit a40f7e5

Browse files
ryanofskyryanofsky
committed
CCoinsViewCache code cleanup & deduplication
The change moves code responsible for updating the cache out of various CCoinsViewCache methods and into a Modifier class. This way the cache update code is just written once in a general way instead of being duplicated and split up to handle various special cases. This is a refactoring, with changes to cache behavior only in 2 corner cases (with corresponding tests in coins_test.cpp) which don't affect the meaning of data stored in the cache: * In BatchWrite, overwriting a non-dirty pruned cache entry with a fresh pruned cache entry now deletes the cache entry instead of leaving behind a dirty pruned entry that will trigger an unnecessary database write later. * In BatchWrite, overwriting a dirty pruned fresh cache entry with a nonpruned entry updates the entry without dropping the fresh flag. There's no reason to drop the fresh flag in this case because the flag accurately describes the state of the base view and could prevent unnecessary database writes in the future if the utxo is spent later.
1 parent 7cdadf9 commit a40f7e5

File tree

4 files changed

+131
-105
lines changed

4 files changed

+131
-105
lines changed

‎src/coins.cpp‎

Lines changed: 127 additions & 102 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@
99
#include <random.h>
1010
#include <version.h>
1111

12+
#include <optional>
13+
1214
bool CCoinsView::GetCoin(const COutPoint &outpoint, Coin &coin) const { return false; }
1315
uint256 CCoinsView::GetBestBlock() const { return uint256(); }
1416
std::vector<uint256> CCoinsView::GetHeadBlocks() const { return std::vector<uint256>(); }
@@ -37,21 +39,24 @@ size_t CCoinsViewCache::DynamicMemoryUsage() const {
3739
return memusage::DynamicUsage(cacheCoins) + cachedCoinsUsage;
3840
}
3941

42+
class CCoinsViewCache::Modifier
43+
{
44+
public:
45+
Modifier(const CCoinsViewCache& cache, const COutPoint& outpoint);
46+
Modifier(const CCoinsViewCache& cache, const COutPoint& outpoint, CCoinsCacheEntry&& new_entry);
47+
~Modifier() { Flush(); }
48+
Coin& Modify();
49+
CCoinsMap::iterator Flush();
50+
51+
private:
52+
const CCoinsViewCache& m_cache;
53+
const COutPoint& m_outpoint;
54+
CCoinsMap::iterator m_cur_entry = m_cache.cacheCoins.find(m_outpoint);
55+
std::optional<CCoinsCacheEntry> m_new_entry;
56+
};
57+
4058
CCoinsMap::iterator CCoinsViewCache::FetchCoin(const COutPoint &outpoint) const {
41-
CCoinsMap::iterator it = cacheCoins.find(outpoint);
42-
if (it != cacheCoins.end())
43-
return it;
44-
Coin tmp;
45-
if (!base->GetCoin(outpoint, tmp))
46-
return cacheCoins.end();
47-
CCoinsMap::iterator ret = cacheCoins.emplace(std::piecewise_construct, std::forward_as_tuple(outpoint), std::forward_as_tuple(std::move(tmp))).first;
48-
if (ret->second.coin.IsSpent()) {
49-
// The parent only has an empty entry for this outpoint; we can consider our
50-
// version as fresh.
51-
ret->second.flags = CCoinsCacheEntry::FRESH;
52-
}
53-
cachedCoinsUsage += ret->second.coin.DynamicMemoryUsage();
54-
return ret;
59+
return Modifier(*this, outpoint).Flush();
5560
}
5661

5762
bool CCoinsViewCache::GetCoin(const COutPoint &outpoint, Coin &coin) const {
@@ -66,35 +71,14 @@ bool CCoinsViewCache::GetCoin(const COutPoint &outpoint, Coin &coin) const {
6671
void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possible_overwrite) {
6772
assert(!coin.IsSpent());
6873
if (coin.out.scriptPubKey.IsUnspendable()) return;
69-
CCoinsMap::iterator it;
70-
bool inserted;
71-
std::tie(it, inserted) = cacheCoins.emplace(std::piecewise_construct, std::forward_as_tuple(outpoint), std::tuple<>());
72-
bool fresh = false;
73-
if (!inserted) {
74-
cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage();
75-
}
76-
if (!possible_overwrite) {
77-
if (!it->second.coin.IsSpent()) {
78-
throw std::logic_error("Attempted to overwrite an unspent coin (when possible_overwrite is false)");
79-
}
80-
// If the coin exists in this cache as a spent coin and is DIRTY, then
81-
// its spentness hasn't been flushed to the parent cache. We're
82-
// re-adding the coin to this cache now but we can't mark it as FRESH.
83-
// If we mark it FRESH and then spend it before the cache is flushed
84-
// we would remove it from this cache and would never flush spentness
85-
// to the parent cache.
86-
//
87-
// Re-adding a spent coin can happen in the case of a re-org (the coin
88-
// is 'spent' when the block adding it is disconnected and then
89-
// re-added when it is also added in a newly connected block).
90-
//
91-
// If the coin doesn't exist in the current cache, or is spent but not
92-
// DIRTY, then it can be marked FRESH.
93-
fresh = !(it->second.flags & CCoinsCacheEntry::DIRTY);
94-
}
95-
it->second.coin = std::move(coin);
96-
it->second.flags |= CCoinsCacheEntry::DIRTY | (fresh ? CCoinsCacheEntry::FRESH : 0);
97-
cachedCoinsUsage += it->second.coin.DynamicMemoryUsage();
74+
CCoinsCacheEntry entry;
75+
// If we are not possibly overwriting, any coin in the base view below the
76+
// cache will be spent, so the cache entry can be marked fresh.
77+
// If we are possibly overwriting, we can't make any assumption about the
78+
// coin in the the base view below the cache, so the new cache entry which
79+
// will replace it must be marked dirty.
80+
entry.flags |= possible_overwrite ? CCoinsCacheEntry::DIRTY : CCoinsCacheEntry::FRESH;
81+
Modifier(*this, outpoint, std::move(entry)).Modify() = std::move(coin);
9882
}
9983

10084
void CCoinsViewCache::EmplaceCoinInternalDANGER(COutPoint&& outpoint, Coin&& coin) {
@@ -105,31 +89,26 @@ void CCoinsViewCache::EmplaceCoinInternalDANGER(COutPoint&& outpoint, Coin&& coi
10589
std::forward_as_tuple(std::move(coin), CCoinsCacheEntry::DIRTY));
10690
}
10791

108-
void AddCoins(CCoinsViewCache& cache, const CTransaction &tx, int nHeight, bool check_for_overwrite) {
92+
void AddCoins(CCoinsViewCache& cache, const CTransaction &tx, int nHeight, bool check) {
10993
bool fCoinbase = tx.IsCoinBase();
11094
const uint256& txid = tx.GetHash();
11195
for (size_t i = 0; i < tx.vout.size(); ++i) {
112-
bool overwrite = check_for_overwrite ? cache.HaveCoin(COutPoint(txid, i)) : fCoinbase;
113-
// Coinbase transactions can always be overwritten, in order to correctly
96+
bool overwrite = check ? cache.HaveCoin(COutPoint(txid, i)) : fCoinbase;
97+
// Always set the possible_overwrite flag to AddCoin for coinbase txn, in order to correctly
11498
// deal with the pre-BIP30 occurrences of duplicate coinbase transactions.
11599
cache.AddCoin(COutPoint(txid, i), Coin(tx.vout[i], nHeight, fCoinbase), overwrite);
116100
}
117101
}
118102

119103
bool CCoinsViewCache::SpendCoin(const COutPoint &outpoint, Coin* moveout) {
120-
CCoinsMap::iterator it = FetchCoin(outpoint);
121-
if (it == cacheCoins.end()) return false;
122-
cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage();
104+
Modifier modifier(*this, outpoint);
105+
Coin& coin = modifier.Modify();
106+
bool already_spent = coin.IsSpent();
123107
if (moveout) {
124-
*moveout = std::move(it->second.coin);
125-
}
126-
if (it->second.flags & CCoinsCacheEntry::FRESH) {
127-
cacheCoins.erase(it);
128-
} else {
129-
it->second.flags |= CCoinsCacheEntry::DIRTY;
130-
it->second.coin.Clear();
108+
*moveout = std::move(coin);
131109
}
132-
return true;
110+
coin.Clear();
111+
return !already_spent;
133112
}
134113

135114
static const Coin coinEmpty;
@@ -169,51 +148,7 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn
169148
if (!(it->second.flags & CCoinsCacheEntry::DIRTY)) {
170149
continue;
171150
}
172-
CCoinsMap::iterator itUs = cacheCoins.find(it->first);
173-
if (itUs == cacheCoins.end()) {
174-
// The parent cache does not have an entry, while the child cache does.
175-
// We can ignore it if it's both spent and FRESH in the child
176-
if (!(it->second.flags & CCoinsCacheEntry::FRESH && it->second.coin.IsSpent())) {
177-
// Create the coin in the parent cache, move the data up
178-
// and mark it as dirty.
179-
CCoinsCacheEntry& entry = cacheCoins[it->first];
180-
entry.coin = std::move(it->second.coin);
181-
cachedCoinsUsage += entry.coin.DynamicMemoryUsage();
182-
entry.flags = CCoinsCacheEntry::DIRTY;
183-
// We can mark it FRESH in the parent if it was FRESH in the child
184-
// Otherwise it might have just been flushed from the parent's cache
185-
// and already exist in the grandparent
186-
if (it->second.flags & CCoinsCacheEntry::FRESH) {
187-
entry.flags |= CCoinsCacheEntry::FRESH;
188-
}
189-
}
190-
} else {
191-
// Found the entry in the parent cache
192-
if ((it->second.flags & CCoinsCacheEntry::FRESH) && !itUs->second.coin.IsSpent()) {
193-
// The coin was marked FRESH in the child cache, but the coin
194-
// exists in the parent cache. If this ever happens, it means
195-
// the FRESH flag was misapplied and there is a logic error in
196-
// the calling code.
197-
throw std::logic_error("FRESH flag misapplied to coin that exists in parent cache");
198-
}
199-
200-
if ((itUs->second.flags & CCoinsCacheEntry::FRESH) && it->second.coin.IsSpent()) {
201-
// The grandparent cache does not have an entry, and the coin
202-
// has been spent. We can just delete it from the parent cache.
203-
cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage();
204-
cacheCoins.erase(itUs);
205-
} else {
206-
// A normal modification.
207-
cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage();
208-
itUs->second.coin = std::move(it->second.coin);
209-
cachedCoinsUsage += itUs->second.coin.DynamicMemoryUsage();
210-
itUs->second.flags |= CCoinsCacheEntry::DIRTY;
211-
// NOTE: It isn't safe to mark the coin as FRESH in the parent
212-
// cache. If it already existed and was spent in the parent
213-
// cache then marking it FRESH would prevent that spentness
214-
// from being flushed to the grandparent.
215-
}
216-
}
151+
Modifier(*this, it->first, std::move(it->second));
217152
}
218153
hashBlock = hashBlockIn;
219154
return true;
@@ -288,3 +223,93 @@ bool CCoinsViewErrorCatcher::GetCoin(const COutPoint &outpoint, Coin &coin) cons
288223
std::abort();
289224
}
290225
}
226+
227+
CCoinsViewCache::Modifier::Modifier(const CCoinsViewCache& cache, const COutPoint& outpoint)
228+
: m_cache(cache), m_outpoint(outpoint)
229+
{
230+
if (m_cur_entry == m_cache.cacheCoins.end()) {
231+
m_new_entry.emplace();
232+
m_cache.base->GetCoin(m_outpoint, m_new_entry->coin);
233+
if (m_new_entry->coin.IsSpent()) {
234+
m_new_entry->flags |= CCoinsCacheEntry::FRESH;
235+
}
236+
}
237+
}
238+
239+
CCoinsViewCache::Modifier::Modifier(const CCoinsViewCache& cache,
240+
const COutPoint& outpoint,
241+
CCoinsCacheEntry&& new_entry)
242+
: m_cache(cache), m_outpoint(outpoint)
243+
{
244+
const bool cur_entry = m_cur_entry != m_cache.cacheCoins.end();
245+
const bool cur_spent = cur_entry && m_cur_entry->second.coin.IsSpent();
246+
const bool cur_dirty = cur_entry && m_cur_entry->second.flags & CCoinsCacheEntry::DIRTY;
247+
const bool cur_fresh = cur_entry && m_cur_entry->second.flags & CCoinsCacheEntry::FRESH;
248+
const bool new_spent = new_entry.coin.IsSpent();
249+
const bool new_dirty = new_entry.flags & CCoinsCacheEntry::DIRTY;
250+
const bool new_fresh = new_entry.flags & CCoinsCacheEntry::FRESH;
251+
252+
// If the new value is marked FRESH, assert any existing cache entry is
253+
// spent, otherwise it means the FRESH flag was misapplied.
254+
if (new_fresh && cur_entry && !cur_spent) {
255+
throw std::logic_error("FRESH flag misapplied to cache of unspent coin");
256+
}
257+
258+
// If a cache entry is spent but not dirty, it should be marked fresh.
259+
if (new_spent && !new_fresh && !new_dirty) {
260+
throw std::logic_error("Missing FRESH or DIRTY flags for spent cache entry.");
261+
}
262+
263+
// Create new cache entry that can be merged into the cache in Flush().
264+
m_new_entry.emplace();
265+
m_new_entry->coin = std::move(new_entry.coin);
266+
267+
// If `cur_fresh` is true it means the `m_cache.base` coin is spent, so
268+
// keep the FRESH flag. If `new_fresh` is true, it means that the `m_cache`
269+
// coin is spent, which implies that the `m_cache.base` coin is also spent
270+
// as long as the cache is not dirty, so keep the FRESH flag in this case as
271+
// well.
272+
if (cur_fresh || (new_fresh && !cur_dirty)) {
273+
m_new_entry->flags |= CCoinsCacheEntry::FRESH;
274+
}
275+
276+
if (cur_dirty || new_dirty) {
277+
m_new_entry->flags |= CCoinsCacheEntry::DIRTY;
278+
}
279+
}
280+
281+
// Add DIRTY flag to m_new_entry and return mutable coin reference. Populate
282+
// m_new_entry from existing cache entry if necessary.
283+
Coin& CCoinsViewCache::Modifier::Modify()
284+
{
285+
if (!m_new_entry) {
286+
assert(m_cur_entry != m_cache.cacheCoins.end());
287+
m_new_entry.emplace(m_cur_entry->second);
288+
}
289+
m_new_entry->flags |= CCoinsCacheEntry::DIRTY;
290+
return m_new_entry->coin;
291+
}
292+
293+
// Update m_cache.cacheCoins with the contents of m_new_entry, if present.
294+
CCoinsMap::iterator CCoinsViewCache::Modifier::Flush()
295+
{
296+
if (m_new_entry) {
297+
bool erase = (m_new_entry->flags & CCoinsCacheEntry::FRESH) && m_new_entry->coin.IsSpent();
298+
if (m_cur_entry != m_cache.cacheCoins.end()) {
299+
m_cache.cachedCoinsUsage -= m_cur_entry->second.coin.DynamicMemoryUsage();
300+
if (erase) {
301+
m_cache.cacheCoins.erase(m_cur_entry);
302+
m_cur_entry = m_cache.cacheCoins.end();
303+
} else {
304+
m_cur_entry->second = std::move(*m_new_entry);
305+
}
306+
} else if (!erase) {
307+
m_cur_entry = m_cache.cacheCoins.emplace(m_outpoint, std::move(*m_new_entry)).first;
308+
}
309+
if (!erase) {
310+
m_cache.cachedCoinsUsage += m_cur_entry->second.coin.DynamicMemoryUsage();
311+
}
312+
}
313+
m_new_entry.reset();
314+
return m_cur_entry;
315+
}

‎src/coins.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,7 @@ class CCoinsViewCache : public CCoinsViewBacked
222222
/* Cached dynamic memory usage for the inner Coin objects. */
223223
mutable size_t cachedCoinsUsage;
224224

225+
class Modifier;
225226
public:
226227
CCoinsViewCache(CCoinsView *baseIn);
227228

‎src/test/coins_tests.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -831,15 +831,15 @@ BOOST_AUTO_TEST_CASE(ccoins_write)
831831
CheckWriteCoins(SPENT , ABSENT, SPENT , DIRTY , NO_ENTRY , DIRTY );
832832
CheckWriteCoins(SPENT , ABSENT, SPENT , DIRTY|FRESH, NO_ENTRY , DIRTY|FRESH);
833833
CheckWriteCoins(SPENT , SPENT , SPENT , 0 , DIRTY , DIRTY );
834-
CheckWriteCoins(SPENT , SPENT , SPENT , 0 , DIRTY|FRESH, DIRTY );
834+
CheckWriteCoins(SPENT , SPENT , ABSENT, 0 , DIRTY|FRESH, NO_ENTRY );
835835
CheckWriteCoins(SPENT , SPENT , ABSENT, FRESH , DIRTY , NO_ENTRY );
836836
CheckWriteCoins(SPENT , SPENT , ABSENT, FRESH , DIRTY|FRESH, NO_ENTRY );
837837
CheckWriteCoins(SPENT , SPENT , SPENT , DIRTY , DIRTY , DIRTY );
838838
CheckWriteCoins(SPENT , SPENT , SPENT , DIRTY , DIRTY|FRESH, DIRTY );
839839
CheckWriteCoins(SPENT , SPENT , ABSENT, DIRTY|FRESH, DIRTY , NO_ENTRY );
840840
CheckWriteCoins(SPENT , SPENT , ABSENT, DIRTY|FRESH, DIRTY|FRESH, NO_ENTRY );
841841
CheckWriteCoins(SPENT , VALUE2, VALUE2, 0 , DIRTY , DIRTY );
842-
CheckWriteCoins(SPENT , VALUE2, VALUE2, 0 , DIRTY|FRESH, DIRTY );
842+
CheckWriteCoins(SPENT , VALUE2, VALUE2, 0 , DIRTY|FRESH, DIRTY|FRESH);
843843
CheckWriteCoins(SPENT , VALUE2, VALUE2, FRESH , DIRTY , DIRTY|FRESH);
844844
CheckWriteCoins(SPENT , VALUE2, VALUE2, FRESH , DIRTY|FRESH, DIRTY|FRESH);
845845
CheckWriteCoins(SPENT , VALUE2, VALUE2, DIRTY , DIRTY , DIRTY );

‎src/test/fuzz/coins_view.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ FUZZ_TARGET_INIT(coins_view, initialize_coins_view)
6262
coins_view_cache.AddCoin(random_out_point, std::move(coin), possible_overwrite);
6363
expected_code_path = true;
6464
} catch (const std::logic_error& e) {
65-
if (e.what() == std::string{"Attempted to overwrite an unspent coin (when possible_overwrite is false)"}) {
65+
if (e.what() == std::string{"FRESH flag misapplied to cache of unspent coin"}) {
6666
assert(!possible_overwrite);
6767
expected_code_path = true;
6868
}

0 commit comments

Comments
 (0)