Fix dangerous condition in ModifyNewCoins.

We were marking coins FRESH before being sure they were not overwriting dirty undo data. This condition was never reached in existing code because undo data was always flushed before UpdateCoins was called with new transactions, but could have been exposed in an otherwise safe refactor.
Clarify in the comments the assumptions made in ModifyNewCoins.
Add ability to undo transactions to UpdateCoins unit test.
Thanks to Russ Yanofsky for suggestion on how to make logic clearer and fixing up the ccoins_modify_new test cases.

1 files changed, 31 insertions, 6 deletions

diff --git a/src/coins.cpp b/src/coins.cpp
index 8ff652b474..fe474fdda2 100644
--- a/src/coins.cpp
+++ b/src/coins.cpp
@@ -117,17 +117,37 @@ CCoinsModifier CCoinsViewCache::ModifyCoins(const uint256 &txid) {
     return CCoinsModifier(*this, ret.first, cachedCoinUsage);
 }
 
-// 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.
+/* ModifyNewCoins allows for faster coin modification when creating the new
+ * outputs from a transaction.  It assumes that BIP 30 (no duplicate txids)
+ * applies and has already been tested for (or the test is not required due to
+ * BIP 34, height in coinbase).  If we can assume BIP 30 then we know that any
+ * non-coinbase transaction we are adding to the UTXO must not already exist in
+ * the utxo unless it is fully spent.  Thus we can check only if it exists DIRTY
+ * at the current level of the cache, in which case it is not safe to mark it
+ * FRESH (b/c then its spentness still needs to flushed).  If it's not dirty and
+ * doesn't exist or is pruned in the current cache, we know it either doesn't
+ * exist or is pruned in parent caches, which is the definition of FRESH.  The
+ * exception to this is the two historical violations of BIP 30 in the chain,
+ * both of which were coinbases.  We do not mark these fresh so we we can ensure
+ * that they will still be properly overwritten when spent.
+ */
 CCoinsModifier CCoinsViewCache::ModifyNewCoins(const uint256 &txid, bool coinbase) {
     assert(!hasModifier);
     std::pair<CCoinsMap::iterator, bool> ret = cacheCoins.insert(std::make_pair(txid, CCoinsCacheEntry()));
-    ret.first->second.coins.Clear();
     if (!coinbase) {
-        ret.first->second.flags = CCoinsCacheEntry::FRESH;
+        // New coins must not already exist.
+        if (!ret.first->second.coins.IsPruned())
+            throw std::logic_error("ModifyNewCoins should not find pre-existing coins on a non-coinbase unless they are pruned!");
+
+        if (!(ret.first->second.flags & CCoinsCacheEntry::DIRTY)) {
+            // If the coin is known to be pruned (have no unspent outputs) in
+            // the current view and the cache entry is not dirty, we know the
+            // coin also must be pruned in the parent view as well, so it is safe
+            // to mark this fresh.
+            ret.first->second.flags |= CCoinsCacheEntry::FRESH;
+        }
     }
+    ret.first->second.coins.Clear();
     ret.first->second.flags |= CCoinsCacheEntry::DIRTY;
     return CCoinsModifier(*this, ret.first, 0);
 }
@@ -200,6 +220,11 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn
                     itUs->second.coins.swap(it->second.coins);
                     cachedCoinsUsage += itUs->second.coins.DynamicMemoryUsage();
                     itUs->second.flags |= CCoinsCacheEntry::DIRTY;
+                    // NOTE: It is possible the child has a FRESH flag here in
+                    // the event the entry we found in the parent is pruned. But
+                    // we must not copy that FRESH flag to the parent as that
+                    // pruned state likely still needs to be communicated to the
+                    // grandparent.
                 }
             }
         }