Merge bitcoin#14457: test: add invalid tx templates for use in functi…

…onal tests

59e3877 test: add invalid tx templates for use in functional tests (James O'Beirne)

Pull request description:

  This change adds a list of `CTransaction`-generating templates which each correspond to a specific type of invalid transaction. We then use this list to test for a wider variety of invalid tx types in `p2p_invalid_tx.py` and `feature_block.py`.

  Consolidating all invalid tx types will allow us to more easily cover all tx reject cases from a variety of tests without repeating ourselves. Validation logic doesn't differ much between mempool and block acceptance, but there *is* a difference and we should be sure we're testing both comprehensively.

  Right now, I've only added templates covering the tx reject types listed below but if this approach seems worthwhile I will expand the list to be fully comprehensive.
  ```
  bad-txns-in-belowout
  bad-txns-inputs-duplicate
  bad-txns-too-many-sigops
  bad-txns-vin-empty
  bad-txns-vout-empty
  bad-txns-vout-negative
  ```

Tree-SHA512: 05407f4a953fbd7c44c08bb49bb989cefd39a2b05ea00f5b3c92197a3f05e1b302f789e33832445734220e1c333d133aba385740b77b84139b170c583471ce20

test/functional/data/invalid_txs.py

@@ -0,0 +1,180 @@
+#!/usr/bin/env python3
+# Copyright (c) 2015-2018 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""
+Templates for constructing various sorts of invalid transactions.
+
+These templates (or an iterator over all of them) can be reused in different
+contexts to test using a number of invalid transaction types.
+
+Hopefully this makes it easier to get coverage of a full variety of tx
+validation checks through different interfaces (AcceptBlock, AcceptToMemPool,
+etc.) without repeating ourselves.
+
+Invalid tx cases not covered here can be found by running:
+
+    $ diff \
+      <(grep -IREho "bad-txns[a-zA-Z-]+" src | sort -u) \
+      <(grep -IEho "bad-txns[a-zA-Z-]+" test/functional/data/invalid_txs.py | sort -u)
+
+"""
+import abc
+
+from test_framework.messages import CTransaction, CTxIn, CTxOut, COutPoint
+from test_framework import script as sc
+from test_framework.blocktools import create_tx_with_script, MAX_BLOCK_SIGOPS
+
+basic_p2sh = sc.CScript([sc.OP_HASH160, sc.hash160(sc.CScript([sc.OP_0])), sc.OP_EQUAL])
+
+
+class BadTxTemplate:
+    """Allows simple construction of a certain kind of invalid tx. Base class to be subclassed."""
+    __metaclass__ = abc.ABCMeta
+
+    # The expected error code given by bitcoind upon submission of the tx.
+    reject_reason = ""
+
+    # Only specified if it differs from mempool acceptance error.
+    block_reject_reason = ""
+
+    # Do we expect to be disconnected after submitting this tx?
+    expect_disconnect = False
+
+    # Is this tx considered valid when included in a block, but not for acceptance into
+    # the mempool (i.e. does it violate policy but not consensus)?
+    valid_in_block = False
+
+    def __init__(self, *, spend_tx=None, spend_block=None):
+        self.spend_tx = spend_block.vtx[0] if spend_block else spend_tx
+        self.spend_avail = sum(o.nValue for o in self.spend_tx.vout)
+        self.valid_txin = CTxIn(COutPoint(self.spend_tx.sha256, 0), b"", 0xffffffff)
+
+    @abc.abstractmethod
+    def get_tx(self, *args, **kwargs):
+        """Return a CTransaction that is invalid per the subclass."""
+        pass
+
+
+class OutputMissing(BadTxTemplate):
+    reject_reason = "bad-txns-vout-empty"
+    expect_disconnect = False
+
+    def get_tx(self):
+        tx = CTransaction()
+        tx.vin.append(self.valid_txin)
+        tx.calc_sha256()
+        return tx
+
+
+class InputMissing(BadTxTemplate):
+    reject_reason = "bad-txns-vin-empty"
+    expect_disconnect = False
+
+    def get_tx(self):
+        tx = CTransaction()
+        tx.vout.append(CTxOut(0, sc.CScript([sc.OP_TRUE] * 100)))
+        tx.calc_sha256()
+        return tx
+
+
+class SizeTooSmall(BadTxTemplate):
+    reject_reason = "tx-size-small"
+    expect_disconnect = False
+    valid_in_block = True
+
+    def get_tx(self):
+        tx = CTransaction()
+        tx.vin.append(self.valid_txin)
+        tx.vout.append(CTxOut(0, sc.CScript([sc.OP_TRUE])))
+        tx.calc_sha256()
+        return tx
+
+
+class BadInputOutpointIndex(BadTxTemplate):
+    # Won't be rejected - nonexistent outpoint index is treated as an orphan since the coins
+    # database can't distinguish between spent outpoints and outpoints which never existed.
+    reject_reason = None
+    expect_disconnect = False
+
+    def get_tx(self):
+        num_indices = len(self.spend_tx.vin)
+        bad_idx = num_indices + 100
+
+        tx = CTransaction()
+        tx.vin.append(CTxIn(COutPoint(self.spend_tx.sha256, bad_idx), b"", 0xffffffff))
+        tx.vout.append(CTxOut(0, basic_p2sh))
+        tx.calc_sha256()
+        return tx
+
+
+class DuplicateInput(BadTxTemplate):
+    reject_reason = 'bad-txns-inputs-duplicate'
+    expect_disconnect = True
+
+    def get_tx(self):
+        tx = CTransaction()
+        tx.vin.append(self.valid_txin)
+        tx.vin.append(self.valid_txin)
+        tx.vout.append(CTxOut(1, basic_p2sh))
+        tx.calc_sha256()
+        return tx
+
+
+class NonexistentInput(BadTxTemplate):
+    reject_reason = None  # Added as an orphan tx.
+    expect_disconnect = False
+
+    def get_tx(self):
+        tx = CTransaction()
+        tx.vin.append(CTxIn(COutPoint(self.spend_tx.sha256 + 1, 0), b"", 0xffffffff))
+        tx.vin.append(self.valid_txin)
+        tx.vout.append(CTxOut(1, basic_p2sh))
+        tx.calc_sha256()
+        return tx
+
+
+class SpendTooMuch(BadTxTemplate):
+    reject_reason = 'bad-txns-in-belowout'
+    expect_disconnect = True
+
+    def get_tx(self):
+        return create_tx_with_script(
+            self.spend_tx, 0, script_pub_key=basic_p2sh, amount=(self.spend_avail + 1))
+
+
+class SpendNegative(BadTxTemplate):
+    reject_reason = 'bad-txns-vout-negative'
+    expect_disconnect = True
+
+    def get_tx(self):
+        return create_tx_with_script(self.spend_tx, 0, amount=-1)
+
+
+class InvalidOPIFConstruction(BadTxTemplate):
+    reject_reason = "mandatory-script-verify-flag-failed (Invalid OP_IF construction)"
+    expect_disconnect = True
+    valid_in_block = True
+
+    def get_tx(self):
+        return create_tx_with_script(
+            self.spend_tx, 0, script_sig=b'\x64' * 35,
+            amount=(self.spend_avail // 2))
+
+
+class TooManySigops(BadTxTemplate):
+    reject_reason = "bad-txns-too-many-sigops"
+    block_reject_reason = "bad-blk-sigops, out-of-bounds SigOpCount"
+    expect_disconnect = False
+
+    def get_tx(self):
+        lotsa_checksigs = sc.CScript([sc.OP_CHECKSIG] * (MAX_BLOCK_SIGOPS))
+        return create_tx_with_script(
+            self.spend_tx, 0,
+            script_pub_key=lotsa_checksigs,
+            amount=1)
+
+
+def iter_all_templates():
+    """Iterate through all bad transaction template types."""
+    return BadTxTemplate.__subclasses__()

test/functional/feature_block.py

@@ -6,7 +6,13 @@
 import copy
 import struct
 
-from test_framework.blocktools import create_block, create_coinbase, create_tx_with_script, get_legacy_sigopcount_block
+from test_framework.blocktools import (
+    create_block,
+    create_coinbase,
+    create_tx_with_script,
+    get_legacy_sigopcount_block,
+    MAX_BLOCK_SIGOPS,
+)
 from test_framework.key import CECKey
 from test_framework.messages import (
     CBlock,
@@ -44,8 +50,7 @@
 )
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import assert_equal
-
-MAX_BLOCK_SIGOPS = 20000
+from data import invalid_txs
 
 #  Use this class for tests that require behavior other than normal "mininode" behavior.
 #  For now, it is used to serialize a bloated varint (b64).
@@ -99,16 +104,21 @@ def run_test(self):
         self.save_spendable_output()
         self.send_blocks([b0])
 
+        # These constants chosen specifically to trigger an immature coinbase spend
+        # at a certain time below.
+        NUM_BUFFER_BLOCKS_TO_GENERATE = 99
+        NUM_OUTPUTS_TO_COLLECT = 33
+
         # Allow the block to mature
         blocks = []
-        for i in range(99):
-            blocks.append(self.next_block(5000 + i))
+        for i in range(NUM_BUFFER_BLOCKS_TO_GENERATE):
+            blocks.append(self.next_block("maturitybuffer.{}".format(i)))
             self.save_spendable_output()
         self.send_blocks(blocks)
 
         # collect spendable outputs now to avoid cluttering the code later on
         out = []
-        for i in range(33):
+        for i in range(NUM_OUTPUTS_TO_COLLECT):
             out.append(self.get_spendable_output())
 
         # Start by building a couple of blocks on top (which output is spent is
@@ -120,7 +130,39 @@ def run_test(self):
         b2 = self.next_block(2, spend=out[1])
         self.save_spendable_output()
 
-        self.send_blocks([b1, b2])
+        self.sync_blocks([b1, b2], timeout=4)
+
+        # Select a txn with an output eligible for spending. This won't actually be spent,
+        # since we're testing submission of a series of blocks with invalid txns.
+        attempt_spend_tx = out[2]
+
+        # Submit blocks for rejection, each of which contains a single transaction
+        # (aside from coinbase) which should be considered invalid.
+        for TxTemplate in invalid_txs.iter_all_templates():
+            template = TxTemplate(spend_tx=attempt_spend_tx)
+
+            # Something about the serialization code for missing inputs creates
+            # a different hash in the test client than on bitcoind, resulting
+            # in a mismatching merkle root during block validation.
+            # Skip until we figure out what's going on.
+            if TxTemplate == invalid_txs.InputMissing:
+                continue
+            if template.valid_in_block:
+                continue
+
+            self.log.info("Reject block with invalid tx: %s", TxTemplate.__name__)
+            blockname = "for_invalid.%s" % TxTemplate.__name__
+            badblock = self.next_block(blockname)
+            badtx = template.get_tx()
+            self.sign_tx(badtx, attempt_spend_tx)
+            badtx.rehash()
+            badblock = self.update_block(blockname, [badtx])
+            self.sync_blocks(
+                [badblock], success=False,
+                reject_reason=(template.block_reject_reason or template.reject_reason),
+                reconnect=True, timeout=2)
+
+            self.move_tip(2)
 
         # Fork like this:
         #
@@ -1294,7 +1336,7 @@ def update_block(self, block_number, new_transactions):
         self.blocks[block_number] = block
         return block
 
-    def bootstrap_p2p(self):
+    def bootstrap_p2p(self, timeout=10):
         """Add a P2P connection to the node.
 
         Helper to connect and wait for version handshake."""
@@ -1305,15 +1347,15 @@ def bootstrap_p2p(self):
         # an INV for the next block and receive two getheaders - one for the
         # IBD and one for the INV. We'd respond to both and could get
         # unexpectedly disconnected if the DoS score for that error is 50.
-        self.nodes[0].p2p.wait_for_getheaders(timeout=5)
+        self.nodes[0].p2p.wait_for_getheaders(timeout=timeout)
 
-    def reconnect_p2p(self):
+    def reconnect_p2p(self, timeout=60):
         """Tear down and bootstrap the P2P connection to the node.
 
         The node gets disconnected several times in this test. This helper
         method reconnects the p2p and restarts the network thread."""
         self.nodes[0].disconnect_p2ps()
-        self.bootstrap_p2p()
+        self.bootstrap_p2p(timeout=timeout)
 
     def send_blocks(self, blocks, success=True, reject_reason=None, request_block=True, reconnect=False, timeout=60):
         """Sends blocks to test node. Syncs and verifies that tip has advanced to most recent block.
@@ -1322,7 +1364,7 @@ def send_blocks(self, blocks, success=True, reject_reason=None, request_block=Tr
         self.nodes[0].p2p.send_blocks_and_test(blocks, self.nodes[0], success=success, reject_reason=reject_reason, request_block=request_block, timeout=timeout, expect_disconnect=reconnect)
 
         if reconnect:
-            self.reconnect_p2p()
+            self.reconnect_p2p(timeout=timeout)
 
 if __name__ == '__main__':
     FullBlockTest().main()

test/functional/p2p_invalid_tx.py

@@ -5,8 +5,7 @@
 """Test node responses to invalid transactions.
 
 In this test we connect to one node over p2p, and test tx requests."""
-
-from test_framework.blocktools import create_block, create_coinbase, create_tx_with_script
+from test_framework.blocktools import create_block, create_coinbase
 from test_framework.messages import (
     COIN,
     COutPoint,
@@ -20,6 +19,7 @@
     assert_equal,
     wait_until,
 )
+from data import invalid_txs
 
 
 class InvalidTxRequestTest(BitcoinTestFramework):
@@ -74,12 +74,21 @@ def run_test(self):
         self.log.info("Mature the block.")
         self.nodes[0].generatetoaddress(100, self.nodes[0].get_deterministic_priv_key().address)
 
-        # b'\x64' is OP_NOTIF
-        # Transaction will be rejected with code 16 (REJECT_INVALID)
-        # and we get disconnected immediately
-        self.log.info('Test a transaction that is rejected')
-        tx1 = create_tx_with_script(block1.vtx[0], 0, script_sig=b'\x64' * 35, amount=50 * COIN)
-        node.p2p.send_txs_and_test([tx1], node, success=False, expect_disconnect=True)
+        # Iterate through a list of known invalid transaction types, ensuring each is
+        # rejected. Some are consensus invalid and some just violate policy.
+        for BadTxTemplate in invalid_txs.iter_all_templates():
+            self.log.info("Testing invalid transaction: %s", BadTxTemplate.__name__)
+            template = BadTxTemplate(spend_block=block1)
+            tx = template.get_tx()
+            node.p2p.send_txs_and_test(
+                [tx], node, success=False,
+                expect_disconnect=template.expect_disconnect,
+                reject_reason=template.reject_reason,
+            )
+
+            if template.expect_disconnect:
+                self.log.info("Reconnecting to peer")
+                self.reconnect_p2p()
 
         # Make two p2p connections to provide the node with orphans
         # * p2ps[0] will send valid orphan txs (one with low fee)

test/functional/test_framework/blocktools.py

@@ -18,6 +18,8 @@
 from .util import assert_equal, hex_str_to_bytes
 from io import BytesIO
 
+MAX_BLOCK_SIGOPS = 20000
+
 def create_block(hashprev, coinbase, ntime=None):
     """Create a block (with regtest difficulty)."""
     block = CBlock()

test/lint/lint-python-dead-code.sh

@@ -17,7 +17,7 @@ fi
 # Any value below 100 introduces the risk of false positives, which would create an unacceptable maintenance burden.
 if ! vulture \
     --min-confidence 100 \
-    $(git ls-files -- "*.py"); then
+    $(git ls-files -- "*.py" ":(exclude)test/functional/data/invalid_txs.py"); then
     echo "Python dead code detection found some issues"
     exit 1
 fi