From f54580e7e4f225bb615204daef32f72ab8688418 Mon Sep 17 00:00:00 2001
From: Pieter Wuille <pieter.wuille@gmail.com>
Date: Tue, 25 Apr 2017 11:29:09 -0700
Subject: [PATCH] error() in disconnect for disk corruption, not inconsistency

The error() function unconditionally reports an error. It should only
be used for actually exception situations, and not for the type of
inconsistencies that ApplyTxInUndo/DisconnectBlock can graciously deal
with.

This also makes a subtle semantics change: in ApplyTxInUndo, when a
record with metadata is encountered (indicating it is the last spend
from a tx), don't wipe the CCoins record if it wasn't empty at that
point. This makes sure that UTXO operations never affect any other
UTXOs (including those from the same tx).
---
 src/test/coins_tests.cpp |  2 +-
 src/validation.cpp       | 70 +++++++++++++++++++---------------------
 2 files changed, 34 insertions(+), 38 deletions(-)

diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
index 31ed1a50b9..1b5d8e91b5 100644
--- a/src/test/coins_tests.cpp
+++ b/src/test/coins_tests.cpp
@@ -17,7 +17,7 @@
 
 #include <boost/test/unit_test.hpp>
 
-bool ApplyTxInUndo(const CTxInUndo& undo, CCoinsViewCache& view, const COutPoint& out);
+int ApplyTxInUndo(const CTxInUndo& undo, CCoinsViewCache& view, const COutPoint& out);
 void UpdateCoins(const CTransaction& tx, CCoinsViewCache& inputs, CTxUndo &txundo, int nHeight);
 
 namespace
diff --git a/src/validation.cpp b/src/validation.cpp
index ac16af3ee7..ed94be5c26 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -1248,39 +1248,6 @@ bool AbortNode(CValidationState& state, const std::string& strMessage, const std
 
 } // anon namespace
 
-/**
- * Apply the undo operation of a CTxInUndo to the given chain state.
- * @param undo The undo object.
- * @param view The coins view to which to apply the changes.
- * @param out The out point that corresponds to the tx input.
- * @return True on success.
- */
-bool ApplyTxInUndo(const CTxInUndo& undo, CCoinsViewCache& view, const COutPoint& out)
-{
-    bool fClean = true;
-
-    CCoinsModifier coins = view.ModifyCoins(out.hash);
-    if (undo.nHeight != 0) {
-        // undo data contains height: this is the last output of the prevout tx being spent
-        if (!coins->IsPruned())
-            fClean = fClean && error("%s: undo data overwriting existing transaction", __func__);
-        coins->Clear();
-        coins->fCoinBase = undo.fCoinBase;
-        coins->nHeight = undo.nHeight;
-        coins->nVersion = undo.nVersion;
-    } else {
-        if (coins->IsPruned())
-            fClean = fClean && error("%s: undo data adding output to missing transaction", __func__);
-    }
-    if (coins->IsAvailable(out.n))
-        fClean = fClean && error("%s: undo data overwriting existing output", __func__);
-    if (coins->vout.size() < out.n+1)
-        coins->vout.resize(out.n+1);
-    coins->vout[out.n] = undo.txout;
-
-    return fClean;
-}
-
 enum DisconnectResult
 {
     DISCONNECT_OK,      // All good.
@@ -1288,6 +1255,35 @@ enum DisconnectResult
     DISCONNECT_FAILED   // Something else went wrong.
 };
 
+/**
+ * Apply the undo operation of a CTxInUndo to the given chain state.
+ * @param undo The undo object.
+ * @param view The coins view to which to apply the changes.
+ * @param out The out point that corresponds to the tx input.
+ * @return A DisconnectResult as an int
+ */
+int ApplyTxInUndo(const CTxInUndo& undo, CCoinsViewCache& view, const COutPoint& out)
+{
+    bool fClean = true;
+
+    CCoinsModifier coins = view.ModifyCoins(out.hash);
+    if (undo.nHeight != 0) {
+        // undo data contains height: this is the last output of the prevout tx being spent
+        if (!coins->IsPruned()) fClean = false; // overwriting existing transaction
+        coins->fCoinBase = undo.fCoinBase;
+        coins->nHeight = undo.nHeight;
+        coins->nVersion = undo.nVersion;
+    } else {
+        if (coins->IsPruned()) fClean = false; // adding output to missing transaction
+    }
+    if (coins->IsAvailable(out.n)) fClean = false; // overwriting existing output
+    if (coins->vout.size() < out.n+1)
+        coins->vout.resize(out.n+1);
+    coins->vout[out.n] = undo.txout;
+
+    return fClean ? DISCONNECT_OK : DISCONNECT_UNCLEAN;
+}
+
 /** Undo the effects of this block (with given index) on the UTXO set represented by coins.
  *  When UNCLEAN or FAILED is returned, view is left in an indeterminate state. */
 static DisconnectResult DisconnectBlock(const CBlock& block, const CBlockIndex* pindex, CCoinsViewCache& view)
@@ -1329,8 +1325,7 @@ static DisconnectResult DisconnectBlock(const CBlock& block, const CBlockIndex*
         // but it must be corrected before txout nversion ever influences a network rule.
         if (outsBlock.nVersion < 0)
             outs->nVersion = outsBlock.nVersion;
-        if (*outs != outsBlock)
-            fClean = fClean && error("DisconnectBlock(): added transaction mismatch? database corrupted");
+        if (*outs != outsBlock) fClean = false; // transaction mismatch
 
         // remove outputs
         outs->Clear();
@@ -1346,8 +1341,9 @@ static DisconnectResult DisconnectBlock(const CBlock& block, const CBlockIndex*
             for (unsigned int j = tx.vin.size(); j-- > 0;) {
                 const COutPoint &out = tx.vin[j].prevout;
                 const CTxInUndo &undo = txundo.vprevout[j];
-                if (!ApplyTxInUndo(undo, view, out))
-                    fClean = false;
+                int res = ApplyTxInUndo(undo, view, out);
+                if (res == DISCONNECT_FAILED) return DISCONNECT_FAILED;
+                fClean = fClean && res != DISCONNECT_UNCLEAN;
             }
         }
     }