consensus: Explain why fCheckDuplicateInputs can not be skipped and remove it

1 files changed, 10 insertions, 9 deletions

diff --git a/src/consensus/tx_check.cpp b/src/consensus/tx_check.cpp
index 1206035839..6793f871cf 100644
--- a/src/consensus/tx_check.cpp
+++ b/src/consensus/tx_check.cpp
@@ -7,7 +7,7 @@
 #include <primitives/transaction.h>
 #include <consensus/validation.h>
 
-bool CheckTransaction(const CTransaction& tx, CValidationState &state, bool fCheckDuplicateInputs)
+bool CheckTransaction(const CTransaction& tx, CValidationState& state)
 {
     // Basic checks that don't depend on any context
     if (tx.vin.empty())
@@ -31,14 +31,15 @@ bool CheckTransaction(const CTransaction& tx, CValidationState &state, bool fChe
             return state.Invalid(ValidationInvalidReason::CONSENSUS, false, "bad-txns-txouttotal-toolarge");
     }
 
-    // Check for duplicate inputs - note that this check is slow so we skip it in CheckBlock
-    if (fCheckDuplicateInputs) {
-        std::set<COutPoint> vInOutPoints;
-        for (const auto& txin : tx.vin)
-        {
-            if (!vInOutPoints.insert(txin.prevout).second)
-                return state.Invalid(ValidationInvalidReason::CONSENSUS, false, "bad-txns-inputs-duplicate");
-        }
+    // Check for duplicate inputs (see CVE-2018-17144)
+    // While Consensus::CheckTxInputs does check if all inputs of a tx are available, and UpdateCoins marks all inputs
+    // of a tx as spent, it does not check if the tx has duplicate inputs.
+    // Failure to run this check will result in either a crash or an inflation bug, depending on the implementation of
+    // the underlying coins database.
+    std::set<COutPoint> vInOutPoints;
+    for (const auto& txin : tx.vin) {
+        if (!vInOutPoints.insert(txin.prevout).second)
+            return state.Invalid(ValidationInvalidReason::CONSENSUS, false, "bad-txns-inputs-duplicate");
     }
 
     if (tx.IsCoinBase())