From 072e2f864445bc6ef3b390255f08c9e8bec2ea94 Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Thu, 26 Mar 2015 13:52:10 -0400 Subject: [PATCH 1/2] Alter assumptions in CCoinsViewCache::BatchWrite Previously it would break if you flushed a parent cache while there was a child cache referring to it. This change will allow the flushing of parent caches. --- src/coins.cpp | 19 ++++++++++++------- src/test/coins_tests.cpp | 16 ++++++++++++++++ 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index 96b336ce77d..f0ea5c04590 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -160,18 +160,23 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn if (it->second.flags & CCoinsCacheEntry::DIRTY) { // Ignore non-dirty entries (optimization). CCoinsMap::iterator itUs = cacheCoins.find(it->first); if (itUs == cacheCoins.end()) { - if (!it->second.coins.IsPruned()) { - // The parent cache does not have an entry, while the child - // cache does have (a non-pruned) one. Move the data up, and - // mark it as fresh (if the grandparent did have it, we - // would have pulled it in at first GetCoins). - assert(it->second.flags & CCoinsCacheEntry::FRESH); + // The parent cache does not have an entry, while the child does + // We can ignore it if it's both FRESH and pruned in the child + if (!(it->second.flags & CCoinsCacheEntry::FRESH && it->second.coins.IsPruned())) { + // Otherwise we will need to create it in the parent + // and move the data up and mark it as dirty CCoinsCacheEntry& entry = cacheCoins[it->first]; entry.coins.swap(it->second.coins); cachedCoinsUsage += entry.coins.DynamicMemoryUsage(); - entry.flags = CCoinsCacheEntry::DIRTY | CCoinsCacheEntry::FRESH; + entry.flags = CCoinsCacheEntry::DIRTY; + // 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; } } else { + // Found the entry in the parent cache if ((itUs->second.flags & CCoinsCacheEntry::FRESH) && it->second.coins.IsPruned()) { // The grandparent does not have an entry, and the child is // modified and being pruned. This means we can just delete diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index 946f904dfa6..9489a19f630 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -165,13 +165,22 @@ BOOST_AUTO_TEST_CASE(coins_cache_simulation_test) } if (insecure_rand() % 100 == 0) { + // Every 100 iterations, flush an intermediate cache + if (stack.size() > 1 && insecure_rand() % 2 == 0) { + unsigned int flushIndex = insecure_rand() % (stack.size() - 1); + stack[flushIndex]->Flush(); + } + } + if (insecure_rand() % 100 == 0) { // Every 100 iterations, change the cache stack. if (stack.size() > 0 && insecure_rand() % 2 == 0) { + //Remove the top cache stack.back()->Flush(); delete stack.back(); stack.pop_back(); } if (stack.size() == 0 || (stack.size() < 4 && insecure_rand() % 2)) { + //Add a new cache CCoinsView* tip = &base; if (stack.size() > 0) { tip = stack.back(); @@ -305,6 +314,13 @@ BOOST_AUTO_TEST_CASE(updatecoins_simulation_test) } if (insecure_rand() % 100 == 0) { + // Every 100 iterations, flush an intermediate cache + if (stack.size() > 1 && insecure_rand() % 2 == 0) { + unsigned int flushIndex = insecure_rand() % (stack.size() - 1); + stack[flushIndex]->Flush(); + } + } + if (insecure_rand() % 100 == 0) { // Every 100 iterations, change the cache stack. if (stack.size() > 0 && insecure_rand() % 2 == 0) { stack.back()->Flush(); From 8504867b146014c99c6acb180020a1369069c761 Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Thu, 12 Nov 2015 16:57:03 -0500 Subject: [PATCH 2/2] Save the last unnecessary database read It's possible coins with the same hash exist when you create a duplicate coinbase, so previously we were reading from the database to make sure we had the old coins cached so if we were to spend the new ones, the old ones would also be spent. This pull instead just marks the new coins as not fresh if they are from a coinbase, so if they are spent they will be written all the way down to the database anyway overwriting any duplicates. --- src/coins.cpp | 10 ++++++++-- src/coins.h | 2 +- src/main.cpp | 12 ++---------- 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index f0ea5c04590..660181b0c44 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -117,11 +117,17 @@ CCoinsModifier CCoinsViewCache::ModifyCoins(const uint256 &txid) { return CCoinsModifier(*this, ret.first, cachedCoinUsage); } -CCoinsModifier CCoinsViewCache::ModifyNewCoins(const uint256 &txid) { +// ModifyNewCoins has to know whether the new outputs its creating are for a +// coinbase or not. If they are for a coinbase, it can not mark them as fresh. +// This is to ensure that the historical duplicate coinbases before BIP30 was +// in effect will still be properly overwritten when spent. +CCoinsModifier CCoinsViewCache::ModifyNewCoins(const uint256 &txid, bool coinbase) { assert(!hasModifier); std::pair ret = cacheCoins.insert(std::make_pair(txid, CCoinsCacheEntry())); ret.first->second.coins.Clear(); - ret.first->second.flags = CCoinsCacheEntry::FRESH; + if (!coinbase) { + ret.first->second.flags = CCoinsCacheEntry::FRESH; + } ret.first->second.flags |= CCoinsCacheEntry::DIRTY; return CCoinsModifier(*this, ret.first, 0); } diff --git a/src/coins.h b/src/coins.h index 3b45cb0a34d..77b4d564826 100644 --- a/src/coins.h +++ b/src/coins.h @@ -428,7 +428,7 @@ class CCoinsViewCache : public CCoinsViewBacked * would not properly overwrite the first coinbase of the pair. Simultaneous modifications * are not allowed. */ - CCoinsModifier ModifyNewCoins(const uint256 &txid); + CCoinsModifier ModifyNewCoins(const uint256 &txid, bool coinbase); /** * Push the modifications applied to this cache to its base. diff --git a/src/main.cpp b/src/main.cpp index 8fb121c00db..3c9c77ef677 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -1310,17 +1310,9 @@ void UpdateCoins(const CTransaction& tx, CValidationState &state, CCoinsViewCach undo.nVersion = coins->nVersion; } } - // add outputs - inputs.ModifyNewCoins(tx.GetHash())->FromTx(tx, nHeight); - } - else { - // add outputs for coinbase tx - // In this case call the full ModifyCoins which will do a database - // lookup to be sure the coins do not already exist otherwise we do not - // know whether to mark them fresh or not. We want the duplicate coinbases - // before BIP30 to still be properly overwritten. - inputs.ModifyCoins(tx.GetHash())->FromTx(tx, nHeight); } + // add outputs + inputs.ModifyNewCoins(tx.GetHash(), tx.IsCoinBase())->FromTx(tx, nHeight); } void UpdateCoins(const CTransaction& tx, CValidationState &state, CCoinsViewCache &inputs, int nHeight)