error() in disconnect for disk corruption, not inconsistency

random-zebra · random-zebra · commit 10baa701f621 · 2020-08-18T15:17:55.000+02:00
>>> backports bitcoin/bitcoin@f54580e 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).
diff --git a/src/main.cpp b/src/main.cpp
@@ -2023,46 +2023,43 @@ bool UndoReadFromDisk(CBlockUndo& blockundo, const CDiskBlockPos& pos, const uin
 
 } // anon namespace
 
+enum DisconnectResult
+{
+    DISCONNECT_OK,      // All good.
+    DISCONNECT_UNCLEAN, // Rolled back, but UTXO set was inconsistent with block.
+    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 True on success.
+ * @return A DisconnectResult as an int
  */
-bool ApplyTxInUndo(const CTxInUndo& undo, CCoinsViewCache& view, const COutPoint& out)
+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 = fClean && error("%s: undo data overwriting existing transaction", __func__);
-        coins->Clear();
+        if (!coins->IsPruned()) fClean = false; // overwriting existing transaction
         coins->fCoinBase = undo.fCoinBase;
         coins->fCoinStake = undo.fCoinStake;
         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->IsPruned()) fClean = false; // adding output to missing transaction
     }
-    if (coins->IsAvailable(out.n))
-        fClean = fClean && error("%s: undo data overwriting existing output", __func__);
+    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;
+    return fClean ? DISCONNECT_OK : DISCONNECT_UNCLEAN;
 }
 
-enum DisconnectResult
-{
-    DISCONNECT_OK,      // All good.
-    DISCONNECT_UNCLEAN, // Rolled back, but UTXO set was inconsistent with block.
-    DISCONNECT_FAILED   // Something else went wrong.
-};
 
 /** 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. */
@@ -2123,8 +2120,7 @@ DisconnectResult DisconnectBlock(CBlock& block, CBlockIndex* pindex, CCoinsViewC
             // 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("%s : added transaction mismatch? database corrupted", __func__);
+            if (*outs != outsBlock) fClean = false; // transaction mismatch
 
             // remove outputs
             outs->Clear();
@@ -2144,8 +2140,9 @@ DisconnectResult DisconnectBlock(CBlock& block, CBlockIndex* pindex, CCoinsViewC
         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;
         }
 
         if (view.HaveInputs(tx))
diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
@@ -16,7 +16,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
