From 14470f9aa6baf02ca7162564f397153a2da0c592 Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Mon, 2 Nov 2015 21:27:15 -0500 Subject: [PATCH 1/3] ModifyNewCoins saves database lookups When processing a new transaction, in addition to spending the Coins of its txin's it creates a new Coins for its outputs. The existing ModifyCoins function will first make sure this Coins does not already exist. It can not exist due to BIP 30, but because of that the lookup can't be cached and always has to go to the database. Since we are creating the coins to match the new tx anyway, there is no point in checking if they exist first anyway. However this should not be used for coinbase tx's in order to preserve the historical behavior of overwriting the two existing duplicate tx pairs. --- src/coins.cpp | 9 +++++++++ src/coins.h | 11 +++++++++++ src/main.cpp | 13 ++++++++++--- 3 files changed, 30 insertions(+), 3 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index f02949de5..96b336ce7 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -117,6 +117,15 @@ CCoinsModifier CCoinsViewCache::ModifyCoins(const uint256 &txid) { return CCoinsModifier(*this, ret.first, cachedCoinUsage); } +CCoinsModifier CCoinsViewCache::ModifyNewCoins(const uint256 &txid) { + assert(!hasModifier); + std::pair ret = cacheCoins.insert(std::make_pair(txid, CCoinsCacheEntry())); + ret.first->second.coins.Clear(); + ret.first->second.flags = CCoinsCacheEntry::FRESH; + ret.first->second.flags |= CCoinsCacheEntry::DIRTY; + return CCoinsModifier(*this, ret.first, 0); +} + const CCoins* CCoinsViewCache::AccessCoins(const uint256 &txid) const { CCoinsMap::const_iterator it = FetchCoins(txid); if (it == cacheCoins.end()) { diff --git a/src/coins.h b/src/coins.h index bf4a777b8..3b45cb0a3 100644 --- a/src/coins.h +++ b/src/coins.h @@ -419,6 +419,17 @@ public: */ CCoinsModifier ModifyCoins(const uint256 &txid); + /** + * Return a modifiable reference to a CCoins. Assumes that no entry with the given + * txid exists and creates a new one. This saves a database access in the case where + * the coins were to be wiped out by FromTx anyway. This should not be called with + * the 2 historical coinbase duplicate pairs because the new coins are marked fresh, and + * in the event the duplicate coinbase was spent before a flush, the now pruned coins + * would not properly overwrite the first coinbase of the pair. Simultaneous modifications + * are not allowed. + */ + CCoinsModifier ModifyNewCoins(const uint256 &txid); + /** * Push the modifications applied to this cache to its base. * Failure to call this method before destruction will cause the changes to be forgotten. diff --git a/src/main.cpp b/src/main.cpp index 26a22ae6f..270b4c063 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -1285,10 +1285,17 @@ 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.ModifyCoins(tx.GetHash())->FromTx(tx, nHeight); } void UpdateCoins(const CTransaction& tx, CValidationState &state, CCoinsViewCache &inputs, int nHeight) From 03c82826f97af98c9c90b3500945654606129bc1 Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Wed, 11 Nov 2015 13:02:02 -0500 Subject: [PATCH 2/3] Make CCoinsViewTest behave like CCoinsViewDB --- src/test/coins_tests.cpp | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index 13d848311..65f739df2 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -45,15 +45,18 @@ public: bool BatchWrite(CCoinsMap& mapCoins, const uint256& hashBlock) { for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end(); ) { - map_[it->first] = it->second.coins; - if (it->second.coins.IsPruned() && insecure_rand() % 3 == 0) { - // Randomly delete empty entries on write. - map_.erase(it->first); + if (it->second.flags & CCoinsCacheEntry::DIRTY) { + // Same optimization used in CCoinsViewDB is to only write dirty entries. + map_[it->first] = it->second.coins; + if (it->second.coins.IsPruned() && insecure_rand() % 3 == 0) { + // Randomly delete empty entries on write. + map_.erase(it->first); + } } mapCoins.erase(it++); } - mapCoins.clear(); - hashBestBlock_ = hashBlock; + if (!hashBlock.IsNull()) + hashBestBlock_ = hashBlock; return true; } From 1cf3dd80a614fddbafac387e446fd83d118e0c25 Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Wed, 11 Nov 2015 20:36:16 -0500 Subject: [PATCH 3/3] Add unit test for UpdateCoins --- src/test/coins_tests.cpp | 131 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 131 insertions(+) diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index 65f739df2..946f904df 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -6,6 +6,8 @@ #include "random.h" #include "uint256.h" #include "test/test_bitcoin.h" +#include "main.h" +#include "consensus/validation.h" #include #include @@ -200,4 +202,133 @@ BOOST_AUTO_TEST_CASE(coins_cache_simulation_test) BOOST_CHECK(missed_an_entry); } +// This test is similar to the previous test +// except the emphasis is on testing the functionality of UpdateCoins +// random txs are created and UpdateCoins is used to update the cache stack +// In particular it is tested that spending a duplicate coinbase tx +// has the expected effect (the other duplicate is overwitten at all cache levels) +BOOST_AUTO_TEST_CASE(updatecoins_simulation_test) +{ + bool spent_a_duplicate_coinbase = false; + // A simple map to track what we expect the cache stack to represent. + std::map result; + + // The cache stack. + CCoinsViewTest base; // A CCoinsViewTest at the bottom. + std::vector stack; // A stack of CCoinsViewCaches on top. + stack.push_back(new CCoinsViewCacheTest(&base)); // Start with one cache. + + // Track the txids we've used and whether they have been spent or not + std::map coinbaseids; + std::set alltxids; + std::set duplicateids; + + for (unsigned int i = 0; i < NUM_SIMULATION_ITERATIONS; i++) { + { + CMutableTransaction tx; + tx.vin.resize(1); + tx.vout.resize(1); + tx.vout[0].nValue = i; //Keep txs unique unless intended to duplicate + unsigned int height = insecure_rand(); + + // 1/10 times create a coinbase + if (insecure_rand() % 10 == 0 || coinbaseids.size() < 10) { + // 1/100 times create a duplicate coinbase + if (insecure_rand() % 10 == 0 && coinbaseids.size()) { + std::map::iterator coinbaseIt = coinbaseids.lower_bound(GetRandHash()); + if (coinbaseIt == coinbaseids.end()) { + coinbaseIt = coinbaseids.begin(); + } + //Use same random value to have same hash and be a true duplicate + tx.vout[0].nValue = coinbaseIt->second; + assert(tx.GetHash() == coinbaseIt->first); + duplicateids.insert(coinbaseIt->first); + } + else { + coinbaseids[tx.GetHash()] = tx.vout[0].nValue; + } + assert(CTransaction(tx).IsCoinBase()); + } + // 9/10 times create a regular tx + else { + uint256 prevouthash; + // equally likely to spend coinbase or non coinbase + std::set::iterator txIt = alltxids.lower_bound(GetRandHash()); + if (txIt == alltxids.end()) { + txIt = alltxids.begin(); + } + prevouthash = *txIt; + + // Construct the tx to spend the coins of prevouthash + tx.vin[0].prevout.hash = prevouthash; + tx.vin[0].prevout.n = 0; + + // Update the expected result of prevouthash to know these coins are spent + CCoins& oldcoins = result[prevouthash]; + oldcoins.Clear(); + + // It is of particular importance here that once we spend a coinbase tx hash + // it is no longer available to be duplicated (or spent again) + // BIP 34 in conjunction with enforcing BIP 30 (at least until BIP 34 was active) + // results in the fact that no coinbases were duplicated after they were already spent + alltxids.erase(prevouthash); + coinbaseids.erase(prevouthash); + + // The test is designed to ensure spending a duplicate coinbase will work properly + // if that ever happens and not resurrect the previously overwritten coinbase + if (duplicateids.count(prevouthash)) + spent_a_duplicate_coinbase = true; + + assert(!CTransaction(tx).IsCoinBase()); + } + // Track this tx to possibly spend later + alltxids.insert(tx.GetHash()); + + // Update the expected result to know about the new output coins + CCoins &coins = result[tx.GetHash()]; + coins.FromTx(tx, height); + + CValidationState dummy; + UpdateCoins(tx, dummy, *(stack.back()), height); + } + + // Once every 1000 iterations and at the end, verify the full cache. + if (insecure_rand() % 1000 == 1 || i == NUM_SIMULATION_ITERATIONS - 1) { + for (std::map::iterator it = result.begin(); it != result.end(); it++) { + const CCoins* coins = stack.back()->AccessCoins(it->first); + if (coins) { + BOOST_CHECK(*coins == it->second); + } else { + BOOST_CHECK(it->second.IsPruned()); + } + } + } + + if (insecure_rand() % 100 == 0) { + // Every 100 iterations, change the cache stack. + if (stack.size() > 0 && insecure_rand() % 2 == 0) { + stack.back()->Flush(); + delete stack.back(); + stack.pop_back(); + } + if (stack.size() == 0 || (stack.size() < 4 && insecure_rand() % 2)) { + CCoinsView* tip = &base; + if (stack.size() > 0) { + tip = stack.back(); + } + stack.push_back(new CCoinsViewCacheTest(tip)); + } + } + } + + // Clean up the stack. + while (stack.size() > 0) { + delete stack.back(); + stack.pop_back(); + } + + // Verify coverage. + BOOST_CHECK(spent_a_duplicate_coinbase); +} + BOOST_AUTO_TEST_SUITE_END()