From e292694f01a54769ae530388c3ace4a3ee419454 Mon Sep 17 00:00:00 2001
From: Jonathan Toomim <j@toom.im>
Date: Sun, 19 Aug 2018 20:26:18 -0700
Subject: [PATCH] Use output-then-input block validation before fork (with
 tests)

---
 src/test/txvalidationcache_tests.cpp | 51 ++++++++++++++++++++++++++++
 src/validation.cpp                   | 24 +++++++++++--
 2 files changed, 73 insertions(+), 2 deletions(-)

diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp
index dc352f5a12..1df4b5ffc9 100644
--- a/src/test/txvalidationcache_tests.cpp
+++ b/src/test/txvalidationcache_tests.cpp
@@ -89,6 +89,57 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, TestChain100Setup) {
     BOOST_CHECK_EQUAL(mempool.size(), 0);
 }
 
+BOOST_FIXTURE_TEST_CASE(tx_block_order, TestChain100Setup) {
+    // Make sure that we correctly validate the order of transctions in a block
+    CScript scriptPubKey = CScript() << ToByteVector(coinbaseKey.GetPubKey())
+                                     << OP_CHECKSIG;
+
+    // Create a 2-length chain of spends of mature coinbase txn:
+    std::vector<CMutableTransaction> spends;
+    spends.resize(2);
+    for (int i = 0; i < 2; i++) {
+        spends[i].nVersion = 1;
+        spends[i].vin.resize(1);
+        if (i==0) {
+            spends[i].vin[0].prevout = COutPoint(coinbaseTxns[0].GetId(), 0);
+        } else {
+            spends[i].vin[0].prevout = COutPoint(spends[0].GetId(), 0);
+        }
+        spends[i].vout.resize(1);
+        spends[i].vout[0].nValue = 11 * CENT;
+        spends[i].vout[0].scriptPubKey = scriptPubKey;
+
+        // Sign:
+        std::vector<uint8_t> vchSig;
+        uint256 hash = SignatureHash(scriptPubKey, CTransaction(spends[i]), 0,
+                                     SigHashType().withForkId(),
+                                     i==0 ? coinbaseTxns[0].vout[0].nValue : spends[i-1].vout[0].nValue);
+        BOOST_CHECK(coinbaseKey.Sign(hash, vchSig));
+        vchSig.push_back(uint8_t(SIGHASH_ALL | SIGHASH_FORKID));
+        spends[i].vin[0].scriptSig << vchSig;
+    }
+
+    CBlock block;
+    std::vector<CMutableTransaction> reorderedSpends;
+    reorderedSpends.resize(2);
+
+    // Test 1: spending an output from a later transaction in the same block is not okay
+    // unless the system clock is greater than fork date
+    reorderedSpends[0] = spends[1];
+    reorderedSpends[1] = spends[0];
+    block = CreateAndProcessBlock(reorderedSpends, scriptPubKey);
+    GlobalConfig config;
+    if (IsMagneticAnomalyEnabled(config, chainActive.Tip())) {
+        BOOST_CHECK(chainActive.Tip()->GetBlockHash() == block.GetHash());
+    } else {
+        BOOST_CHECK(chainActive.Tip()->GetBlockHash() != block.GetHash());
+    }
+
+    // Test 2 : spending an output from an earlier transaction in the same block is okay
+    block = CreateAndProcessBlock(spends, scriptPubKey);
+    BOOST_CHECK(chainActive.Tip()->GetBlockHash() == block.GetHash());
+}
+
 // Run CheckInputs (using pcoinsTip) on the given transaction, for all script
 // flags. Test that CheckInputs passes for all flags that don't overlap with the
 // failing_flags argument, but otherwise fails.
diff --git a/src/validation.cpp b/src/validation.cpp
index ed381cab09..89cad3bbbd 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -42,6 +42,7 @@
 
 #include <atomic>
 #include <sstream>
+#include <unordered_map>
 
 #include <boost/algorithm/string/join.hpp>
 #include <boost/algorithm/string/replace.hpp>
@@ -2126,6 +2127,8 @@ static bool ConnectBlock(const Config &config, const CBlock &block,
     vPos.reserve(block.vtx.size());
     blockundo.vtxundo.reserve(block.vtx.size() - 1);
 
+    std::unordered_map<uint256, uint32_t, SaltedTxidHasher> txpositions;
+    uint32_t curposition = 0;
     for (const auto &ptx : block.vtx) {
         const CTransaction &tx = *ptx;
 
@@ -2139,12 +2142,23 @@ static bool ConnectBlock(const Config &config, const CBlock &block,
             nSigOpsCount += GetSigOpCountWithoutP2SH(tx, flags);
         }
 
-        if (fIsMagneticAnomalyEnabled || tx.IsCoinBase()) {
+        try {
             AddCoins(view, tx, pindex->nHeight);
+        } catch (std::logic_error& e) {
+            // This happens if we try to insert outputs from the same transaction
+            // twice (i.e. pre-BIP30 coinbases).
+            return state.DoS(100, error("ConnectBlock(): inputs missing/spent"),
+                             REJECT_INVALID, "bad-txns-inputs-missingorspent");
+        }
+
+        if (!fIsMagneticAnomalyEnabled) {
+            txpositions[tx.GetId()] = curposition++;
         }
     }
 
+    curposition = -1;
     for (const auto &ptx : block.vtx) {
+        curposition++;
         const CTransaction &tx = *ptx;
         if (tx.IsCoinBase()) {
             continue;
@@ -2205,7 +2219,13 @@ static bool ConnectBlock(const Config &config, const CBlock &block,
         SpendCoins(view, tx, blockundo.vtxundo.back(), pindex->nHeight);
 
         if (!fIsMagneticAnomalyEnabled) {
-            AddCoins(view, tx, pindex->nHeight);
+            for (size_t j = 0; j < tx.vin.size(); j++) {
+                if (txpositions.count(tx.vin[j].prevout.GetTxId()) &&
+                    txpositions[tx.vin[j].prevout.GetTxId()] >= curposition) {
+                    return state.DoS(100, error("ConnectBlock(): inputs missing/spent"),
+                             REJECT_INVALID, "bad-txns-inputs-missingorspent");
+                }
+            }
         }
     }