Merge #2346: [QA] Test suite update + P2P invalid blocks and invalid …

…tx functional tests coverage.

2d994c4 qa: adding test case for output value > input value out of range (furszy)
6165c80 qa: update and enable p2p_invalid_tx.py functional test. (furszy)
bb62699 Backport chainparams and validation `fRequireStandard` flags functionality + init 'acceptnonstdtxn' option to skip (most) "non-standard transaction" checks, for testnet/regtest only. (furszy)
0280c39 New regression testing for CVE-2018-17144, CVE-2012-2459, and CVE-2010-5137. (lucash-dev)
ec7d0b0 [qa] Test for duplicate inputs within a transaction (Suhas Daftuar)
a501fe1 qa: update and enable p2p_invalid_block.py. (furszy)
f83d140 qa: Read reject reasons from debug log, not p2p messages (furszy)
8a50165 [tests] Add P2PDataStore class (furszy)
ebed910 qa: Correct coinbase input script height. (furszy)
cae2d43 [tests] Reduce NodeConn connection logging from info to debug (John Newbery)

Pull request description:

  Introduced the `P2PDataStore` class into the functional test suite and updated it up to a point where was able to enable the `p2p_invalid_block.py` and `p2p_invalid_tx.py` functional tests. Plus updated both tests to a much recent version, expanding its testing coverage as well.
  Now the test suite is covering the primary CVE that btc had in the past.

  As both tests were simply unusable before and we have a different synchronization process than upstream, plus we didn't have some params and init/global flags, this work isn't following a PR by PR back port path. Had to made my own way up to the complete tests support.

  Main pilar PRs from upstream from where this work was based:
  * bitcoin#6329.
  * bitcoin#11771.
  * bitcoin#14119 (only `p2p_invalid_tx.py`, `p2p_invalid_block.py` and `mininode.py` changes).
  * bitcoin#14247 (only 9b4a36e).
  * bitcoin#14696.

ACKs for top commit:
  random-zebra:
    ACK 2d994c4

Tree-SHA512: 548830b5fbf8b7f383832a7eea96179d089a4c9c222ef34007846f53736b07c873e37084235f1575eac157b63308c00ab3be78b71d3ed6520f4f73f1c8de81a5

src/chainparams.cpp

@@ -241,6 +241,9 @@ class CMainParams : public CChainParams
 
         vFixedSeeds = std::vector<SeedSpec6>(pnSeed6_main, pnSeed6_main + ARRAYLEN(pnSeed6_main));
 
+        // Reject non-standard transactions by default
+        fRequireStandard = true;
+
         // Sapling
         bech32HRPs[SAPLING_PAYMENT_ADDRESS]      = "ps";
         bech32HRPs[SAPLING_FULL_VIEWING_KEY]     = "pviews";
@@ -364,6 +367,8 @@ class CTestNetParams : public CChainParams
 
         vFixedSeeds = std::vector<SeedSpec6>(pnSeed6_test, pnSeed6_test + ARRAYLEN(pnSeed6_test));
 
+        fRequireStandard = false;
+
         // Sapling
         bech32HRPs[SAPLING_PAYMENT_ADDRESS]      = "ptestsapling";
         bech32HRPs[SAPLING_FULL_VIEWING_KEY]     = "pviewtestsapling";
@@ -486,6 +491,9 @@ class CRegTestParams : public CChainParams
         // Testnet pivx BIP44 coin type is '1' (All coin's testnet default)
         base58Prefixes[EXT_COIN_TYPE] = {0x80, 0x00, 0x00, 0x01};
 
+        // Reject non-standard transactions by default
+        fRequireStandard = true;
+
         // Sapling
         bech32HRPs[SAPLING_PAYMENT_ADDRESS]      = "ptestsapling";
         bech32HRPs[SAPLING_FULL_VIEWING_KEY]     = "pviewtestsapling";

src/chainparams.h

@@ -67,7 +67,10 @@ class CChainParams
     int GetDefaultPort() const { return nDefaultPort; }
 
     const CBlock& GenesisBlock() const { return genesis; }
-
+    /** Policy: Filter transactions that do not match well-defined patterns */
+    bool RequireStandard() const { return fRequireStandard; }
+    /** If this chain is exclusively used for testing */
+    bool IsTestChain() const { return IsTestnet() || IsRegTestNet(); }
     /** Make miner wait to have peers to avoid wasting work */
     bool MiningRequiresPeers() const { return !IsRegTestNet(); }
     /** Headers first syncing is disabled */
@@ -99,6 +102,7 @@ class CChainParams
     std::vector<unsigned char> base58Prefixes[MAX_BASE58_TYPES];
     std::string bech32HRPs[MAX_BECH32_TYPES];
     std::vector<SeedSpec6> vFixedSeeds;
+    bool fRequireStandard;
 };
 
 /**

src/init.cpp

@@ -577,6 +577,12 @@ std::string HelpMessage(HelpMessageMode mode)
     strUsage += HelpMessageOpt("-mnoperatorprivatekey=<WIF>", _("Set the masternode operator private key. Only valid with -masternode=1. When set, the masternode acts as a deterministic masternode."));
 
     strUsage += HelpMessageGroup(_("Node relay options:"));
+    if (showDebug) {
+        strUsage += HelpMessageOpt("-acceptnonstdtxn",
+                                   strprintf("Relay and mine \"non-standard\" transactions (%sdefault: %u)",
+                                             "testnet/regtest only; ",
+                                             !CreateChainParams(CBaseChainParams::TESTNET)->RequireStandard()));
+    }
     strUsage += HelpMessageOpt("-datacarrier", strprintf(_("Relay and mine data carrier transactions (default: %u)"), DEFAULT_ACCEPT_DATACARRIER));
     strUsage += HelpMessageOpt("-datacarriersize", strprintf(_("Maximum size of data in data carrier transactions we relay and mine (default: %u)"), MAX_OP_RETURN_RELAY));
     if (showDebug) {
@@ -1163,6 +1169,11 @@ bool AppInitParameterInteraction()
             return UIError(AmountErrMsg("minrelaytxfee", gArgs.GetArg("-minrelaytxfee", "")));
     }
 
+    const CChainParams& chainparams = Params();
+    fRequireStandard = !gArgs.GetBoolArg("-acceptnonstdtxn", !chainparams.RequireStandard());
+    if (!chainparams.IsTestChain() && !fRequireStandard)
+        return UIError(strprintf("acceptnonstdtxn is not currently supported for %s chain", chainparams.NetworkIDString()));
+
 #ifdef ENABLE_WALLET
     strWalletFile = gArgs.GetArg("-wallet", DEFAULT_WALLET_DAT);
     if (!CWallet::ParameterInteraction())

src/validation.cpp

@@ -99,6 +99,7 @@ int nScriptCheckThreads = 0;
 std::atomic<bool> fImporting{false};
 std::atomic<bool> fReindex{false};
 bool fTxIndex = true;
+bool fRequireStandard = true;
 bool fCheckBlockIndex = false;
 bool fVerifyingBlocks = false;
 size_t nCoinCacheUsage = 5000 * 300;
@@ -445,7 +446,7 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState &state, const C
 
     // Rather not work on nonstandard transactions
     std::string reason;
-    if (!IsStandardTx(_tx, nextBlockHeight, reason))
+    if (fRequireStandard && !IsStandardTx(_tx, nextBlockHeight, reason))
         return state.DoS(0, false, REJECT_NONSTANDARD, reason);
     // is it already in the memory pool?
     const uint256& hash = tx.GetHash();
@@ -523,7 +524,7 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState &state, const C
         view.SetBackend(dummy);
 
         // Check for non-standard pay-to-script-hash in inputs
-        if (!Params().IsRegTestNet() && !AreInputsStandard(tx, view))
+        if (fRequireStandard && !AreInputsStandard(tx, view))
             return state.Invalid(false, REJECT_NONSTANDARD, "bad-txns-nonstandard-inputs");
 
         // Check that the transaction doesn't have an excessive number of
@@ -2132,7 +2133,7 @@ bool static ConnectTip(CValidationState& state, CBlockIndex* pindexNew, const st
         if (!rv) {
             if (state.IsInvalid())
                 InvalidBlockFound(pindexNew, state);
-            return error("ConnectTip() : ConnectBlock %s failed", pindexNew->GetBlockHash().ToString());
+            return error("%s: ConnectBlock %s failed, %s", __func__, pindexNew->GetBlockHash().ToString(), FormatStateMessage(state));
         }
         nTime3 = GetTimeMicros();
         nTimeConnectTotal += nTime3 - nTime2;

src/validation.h

@@ -142,6 +142,7 @@ extern std::atomic<bool> fImporting;
 extern std::atomic<bool> fReindex;
 extern int nScriptCheckThreads;
 extern bool fTxIndex;
+extern bool fRequireStandard;
 extern bool fCheckBlockIndex;
 extern size_t nCoinCacheUsage;
 extern CFeeRate minRelayTxFee;

test/functional/p2p_invalid_block.py

@@ -1,86 +1,72 @@
 #!/usr/bin/env python3
-# Copyright (c) 2015-2016 The Bitcoin Core developers
+# Copyright (c) 2015-2017 The Bitcoin Core developers
 # Distributed under the MIT software license, see the accompanying
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Test node responses to invalid blocks.
 
-from test_framework.test_framework import ComparisonTestFramework
-from test_framework.util import *
-from test_framework.comptool import TestManager, TestInstance, RejectResult
-from test_framework.blocktools import *
-import copy
-import time
-
-
-'''
 In this test we connect to one node over p2p, and test block requests:
 1) Valid blocks should be requested and become chain tip.
 2) Invalid block with duplicated transaction should be re-requested.
 3) Invalid block with bad coinbase value should be rejected and not
 re-requested.
-'''
+"""
+import copy
 
-# Use the ComparisonTestFramework with 1 node: only use --testbinary.
-class InvalidBlockRequestTest(ComparisonTestFramework):
+from test_framework.blocktools import create_block, create_coinbase, create_transaction
+from test_framework.messages import COIN
+from test_framework.mininode import network_thread_start, P2PDataStore
+from test_framework.test_framework import PivxTestFramework
+from test_framework.util import assert_equal
 
-    '''
-    Can either run this test as 1 node with expected answers, or two and compare them.
-    Change the "outcome" variable from each TestInstance object to only do the comparison.
-    '''
-    def __init__(self):
-        super().__init__()
+class InvalidBlockRequestTest(PivxTestFramework):
+    def set_test_params(self):
         self.num_nodes = 1
+        self.setup_clean_chain = True
+        self.extra_args = [["-whitelist=127.0.0.1"]]
 
     def run_test(self):
-        test = TestManager(self, self.options.tmpdir)
-        test.add_all_connections(self.nodes)
-        self.tip = None
-        self.block_time = None
-        NetworkThread().start() # Start up network handling in another thread
-        test.run()
-
-    def get_tests(self):
-        if self.tip is None:
-            self.tip = int("0x" + self.nodes[0].getbestblockhash(), 0)
-        self.block_time = int(time.time())+1
-
-        '''
-        Create a new block with an anyone-can-spend coinbase
-        '''
+        # Add p2p connection to node0
+        node = self.nodes[0]  # convenience reference to the node
+        node.add_p2p_connection(P2PDataStore())
+        network_thread_start()
+        node.p2p.wait_for_verack()
+
+        best_block = node.getblock(node.getbestblockhash())
+        tip = int(node.getbestblockhash(), 16)
+        height = best_block["height"] + 1
+        block_time = best_block["time"] + 1
+
+        self.log.info("Create a new block with an anyone-can-spend coinbase")
+
         height = 1
-        block = create_block(self.tip, create_coinbase(height), self.block_time)
-        self.block_time += 1
+        block = create_block(tip, create_coinbase(height), block_time)
         block.solve()
         # Save the coinbase for later
-        self.block1 = block
-        self.tip = block.sha256
-        height += 1
-        yield TestInstance([[block, True]])
-
-        '''
-        Now we need that block to mature so we can spend the coinbase.
-        '''
-        test = TestInstance(sync_every_block=False)
-        for i in range(100):
-            block = create_block(self.tip, create_coinbase(height), self.block_time)
-            block.solve()
-            self.tip = block.sha256
-            self.block_time += 1
-            test.blocks_and_transactions.append([block, True])
-            height += 1
-        yield test
-
-        '''
-        Now we use merkle-root malleability to generate an invalid block with
-        same blockheader.
-        Manufacture a block with 3 transactions (coinbase, spend of prior
-        coinbase, spend of that spend).  Duplicate the 3rd transaction to
-        leave merkle root and blockheader unchanged but invalidate the block.
-        '''
-        block2 = create_block(self.tip, create_coinbase(height), self.block_time)
-        self.block_time += 1
+        block1 = block
+        tip = block.sha256
+        node.p2p.send_blocks_and_test([block1], node, success=True)
+
+        self.log.info("Mature the block.")
+        node.generate(100)
+
+        best_block = node.getblock(node.getbestblockhash())
+        tip = int(node.getbestblockhash(), 16)
+        height = best_block["height"] + 1
+        block_time = best_block["time"] + 1
+
+        # Use merkle-root malleability to generate an invalid block with
+        # same blockheader (CVE-2012-2459).
+        # Manufacture a block with 3 transactions (coinbase, spend of prior
+        # coinbase, spend of that spend).  Duplicate the 3rd transaction to
+        # leave merkle root and blockheader unchanged but invalidate the block.
+        # For more information on merkle-root malleability see src/consensus/merkle.cpp.
+        self.log.info("Test merkle root malleability.")
+
+        block2 = create_block(tip, create_coinbase(height), block_time)
+        block_time += 1
 
         # b'0x51' is OP_TRUE
-        tx1 = create_transaction(self.block1.vtx[0], 0, b'\x51', 50 * COIN)
+        tx1 = create_transaction(block1.vtx[0], 0, b'\x51', 50 * COIN)
         tx2 = create_transaction(tx1, 0, b'\x51', 50 * COIN)
 
         block2.vtx.extend([tx1, tx2])
@@ -94,26 +80,70 @@ def get_tests(self):
         block2.vtx.append(tx2)
         assert_equal(block2.hashMerkleRoot, block2.calc_merkle_root())
         assert_equal(orig_hash, block2.rehash())
-        assert(block2_orig.vtx != block2.vtx)
+        assert block2_orig.vtx != block2.vtx
 
-        self.tip = block2.sha256
-        yield TestInstance([[block2, RejectResult(16, b'bad-txns-duplicate')], [block2_orig, True]])
-        height += 1
+        node.p2p.send_blocks_and_test([block2], node, success=False, reject_reason='bad-txns-duplicate')
+
+        # Check transactions for duplicate inputs (CVE-2018-17144)
+        self.log.info("Test duplicate input block.")
+
+        block2_dup = copy.deepcopy(block2_orig)
+        block2_dup.vtx[2].vin.append(block2_dup.vtx[2].vin[0])
+        block2_dup.vtx[2].rehash()
+        block2_dup.hashMerkleRoot = block2_dup.calc_merkle_root()
+        block2_dup.rehash()
+        block2_dup.solve()
+        node.p2p.send_blocks_and_test([block2_dup], node, success=False, reject_reason='bad-txns-inputs-duplicate')
 
-        '''
-        Make sure that a totally screwed up block is not valid.
-        '''
-        block3 = create_block(self.tip, create_coinbase(height), self.block_time)
-        self.block_time += 1
-        block3.vtx[0].vout[0].nValue = 100 * COIN # Too high!
-        block3.vtx[0].sha256=None
+        self.log.info("Test very broken block.")
+
+        block3 = create_block(tip, create_coinbase(height), block_time)
+        block_time += 1
+        block3.vtx[0].vout[0].nValue = 251 * COIN  # Too high!
+        block3.vtx[0].sha256 = None
         block3.vtx[0].calc_sha256()
         block3.hashMerkleRoot = block3.calc_merkle_root()
         block3.rehash()
         block3.solve()
 
-        yield TestInstance([[block3, RejectResult(16, b'bad-cb-amount')]])
+        node.p2p.send_blocks_and_test([block3], node, success=False, reject_reason='bad-cb-amount')
+
+
+        # Complete testing of CVE-2012-2459 by sending the original block.
+        # It should be accepted even though it has the same hash as the mutated one.
 
+        self.log.info("Test accepting original block after rejecting its mutated version.")
+        node.p2p.send_blocks_and_test([block2_orig], node, success=True, timeout=5)
+
+        # Update tip info
+        height += 1
+        block_time += 1
+        tip = int(block2_orig.hash, 16)
+
+        # Complete testing of CVE-2018-17144, by checking for the inflation bug.
+        # Create a block that spends the output of a tx in a previous block.
+        block4 = create_block(tip, create_coinbase(height), block_time)
+        tx3 = create_transaction(tx2, 0, b'\x51', 50 * COIN)
+
+        # Duplicates input
+        tx3.vin.append(tx3.vin[0])
+        tx3.rehash()
+        block4.vtx.append(tx3)
+        block4.hashMerkleRoot = block4.calc_merkle_root()
+        block4.rehash()
+        block4.solve()
+        self.log.info("Test inflation by duplicating input")
+        node.p2p.send_blocks_and_test([block4], node, success=False,  reject_reason='bad-txns-inputs-duplicate')
+
+        self.log.info("Test output value > input value out of range")
+        # Can be removed when 'feature_block.py' is added to the suite.
+        tx4 = create_transaction(tx2, 0, b'\x51', 260 * COIN)
+        block4 = create_block(tip, create_coinbase(height), block_time)
+        block4.vtx.extend([tx4])
+        block4.hashMerkleRoot = block4.calc_merkle_root()
+        block4.rehash()
+        block4.solve()
+        node.p2p.send_blocks_and_test([block4], node, success=False, reject_reason='bad-txns-in-belowout')
 
 if __name__ == '__main__':
     InvalidBlockRequestTest().main()