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, 5 insertions, 0 deletions

diff --git a/src/coins.h b/src/coins.h
index dd6ef6cc3a..6244606356 100644
--- a/src/coins.h
+++ b/src/coins.h
@@ -269,6 +269,11 @@ struct CCoinsCacheEntry
     enum Flags {
         DIRTY = (1 << 0), // This cache entry is potentially different from the version in the parent view.
         FRESH = (1 << 1), // The parent view does not have this entry (or it is pruned).
+        /* Note that FRESH is a performance optimization with which we can
+         * erase coins that are fully spent if we know we do not need to
+         * flush the changes to the parent cache.  It is always safe to
+         * not mark FRESH if that condition is not guaranteed.
+         */
     };
 
     CCoinsCacheEntry() : coins(), flags(0) {}