Merge pull request #6932

1cf3dd8 Add unit test for UpdateCoins (Alex Morcos)
03c8282 Make CCoinsViewTest behave like CCoinsViewDB (Alex Morcos)
14470f9 ModifyNewCoins saves database lookups (Alex Morcos)

4 files changed, 170 insertions, 9 deletions

diff --git a/src/coins.cpp b/src/coins.cpp
index f02949de53..96b336ce77 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<CCoinsMap::iterator, bool> 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 bf4a777b8a..3b45cb0a34 100644
--- a/src/coins.h
+++ b/src/coins.h
@@ -420,6 +420,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.
      * If false is returned, the state of this cache (and its backing view) will be undefined.
diff --git a/src/main.cpp b/src/main.cpp
index 86bf957436..8fb121c00d 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -1310,10 +1310,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)
diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
index 13d848311a..946f904dfa 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 <vector>
 #include <map>
@@ -45,15 +47,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;
     }
 
@@ -197,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<uint256, CCoins> result;
+
+    // The cache stack.
+    CCoinsViewTest base; // A CCoinsViewTest at the bottom.
+    std::vector<CCoinsViewCacheTest*> 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<uint256, CAmount> coinbaseids;
+    std::set<uint256> alltxids;
+    std::set<uint256> 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<uint256, CAmount>::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<uint256>::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<uint256, CCoins>::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()