Skip to content

Commit b837952

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 85bae24 commit b837952

File tree

3 files changed

+130
-104
lines changed

3 files changed

+130
-104
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 <boost/optional.hpp>
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>(); }
@@ -39,21 +41,24 @@ size_t CCoinsViewCache::DynamicMemoryUsage() const {
3941
return memusage::DynamicUsage(cacheCoins) + cachedCoinsUsage;
4042
}
4143

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

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

102-
void AddCoins(CCoinsViewCache& cache, const CTransaction &tx, int nHeight, bool check_for_overwrite) {
86+
void AddCoins(CCoinsViewCache& cache, const CTransaction &tx, int nHeight, bool check) {
10387
bool fCoinbase = tx.IsCoinBase();
10488
const uint256& txid = tx.GetHash();
10589
for (size_t i = 0; i < tx.vout.size(); ++i) {
106-
bool overwrite = check_for_overwrite ? cache.HaveCoin(COutPoint(txid, i)) : fCoinbase;
107-
// Coinbase transactions can always be overwritten, in order to correctly
90+
bool overwrite = check ? cache.HaveCoin(COutPoint(txid, i)) : fCoinbase;
91+
// Always set the possible_overwrite flag to AddCoin for coinbase txn, in order to correctly
10892
// deal with the pre-BIP30 occurrences of duplicate coinbase transactions.
10993
cache.AddCoin(COutPoint(txid, i), Coin(tx.vout[i], nHeight, fCoinbase), overwrite);
11094
}
11195
}
11296

11397
bool CCoinsViewCache::SpendCoin(const COutPoint &outpoint, Coin* moveout) {
114-
CCoinsMap::iterator it = FetchCoin(outpoint);
115-
if (it == cacheCoins.end()) return false;
116-
cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage();
98+
Modifier modifier(*this, outpoint);
99+
Coin& coin = modifier.Modify();
100+
bool already_spent = coin.IsSpent();
117101
if (moveout) {
118-
*moveout = std::move(it->second.coin);
119-
}
120-
if (it->second.flags & CCoinsCacheEntry::FRESH) {
121-
cacheCoins.erase(it);
122-
} else {
123-
it->second.flags |= CCoinsCacheEntry::DIRTY;
124-
it->second.coin.Clear();
102+
*moveout = std::move(coin);
125103
}
126-
return true;
104+
coin.Clear();
105+
return !already_spent;
127106
}
128107

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

‎src/coins.h‎

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

249+
class Modifier;
249250
public:
250251
CCoinsViewCache(CCoinsView *baseIn);
251252

‎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 );

0 commit comments

Comments
 (0)