Merge #11739: Enforce SCRIPT_VERIFY_P2SH and SCRIPT_VERIFY_WITNESS from genesis

8b56fc0b91 [qa] Test that v0 segwit outputs can't be spent pre-activation (Suhas Daftuar)
ccb8ca42a4 Always enforce SCRIPT_VERIFY_WITNESS with P2SH (Suhas Daftuar)
5c31b20a35 [qa] Remove some pre-activation segwit tests (Suhas Daftuar)
95749a5836 Separate NULLDUMMY enforcement from SEGWIT enforcement (Suhas Daftuar)
ce650182f4 Use P2SH consensus rules for all blocks (Suhas Daftuar)

Pull request description:

  As discussed at the IRC meeting back in October (https://botbot.me/freenode/bitcoin-core-dev/2017-10-12/?msg=92231929&page=2), I had looked into the feasibility of enforcing P2SH and SCRIPT_VERIFY_WITNESS back to the genesis block.

  The P2SH change is pretty straightforward -- there was only one historical block on mainnet that violated the rule, so I carved out an exception to it, similar to the way we have exceptions for the BIP30 violators.

  The segwit change is not entirely as clear.  The code changes themselves are relatively straightforward: we can just always turn on SCRIPT_VERIFY_WITNESS whenever P2SH is active.  However conceptually, this amounts to splitting up BIP141 into two parts, the part that implements new script rules, and the part that handles witness commitments in blocks.

  Arguably though the script rules are really defined in BIP 143 anyway, and so this really amounts to backdating BIP 143 -- script rules for v0 segwit outputs -- back to genesis.  So maybe conceptually this isn't so bad...

  I don't feel strongly about this change in either direction; I started working on it because I was searching for a way to simplify the way we understand and implement the consensus rules around segwit, but I'm not yet sure whether I think this achieves anything toward that goal.

  ping @TheBlueMatt

Tree-SHA512: 73551d4a983eb9792c7ac67f56005822528ac4d1fd52c27cee6d305ebee953f69687ef4ddee8bdc0fec77f77e6b5a9d669750793efee54c076533a095e233042

6 files changed, 127 insertions, 24 deletions

diff --git a/src/chainparams.cpp b/src/chainparams.cpp
index 6067503b0b..121d95af90 100644
--- a/src/chainparams.cpp
+++ b/src/chainparams.cpp
@@ -75,7 +75,7 @@ public:
     CMainParams() {
         strNetworkID = "main";
         consensus.nSubsidyHalvingInterval = 210000;
-        consensus.BIP16Height = 173805; // 00000000000000ce80a7e057163a4db1d5ad7b20fb6f598c9597b9665c8fb0d4 - April 1, 2012
+        consensus.BIP16Exception = uint256S("0x00000000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22");
         consensus.BIP34Height = 227931;
         consensus.BIP34Hash = uint256S("0x000000000000024b89b42a942fe0d9fea3bb44ab7bd1b19115dd6a759c0808b8");
         consensus.BIP65Height = 388381; // 000000000000000004c2b624ed5d7756c508d90fd0da2c7c679febfa6c4735f0
@@ -190,7 +190,7 @@ public:
     CTestNetParams() {
         strNetworkID = "test";
         consensus.nSubsidyHalvingInterval = 210000;
-        consensus.BIP16Height = 514; // 00000000040b4e986385315e14bee30ad876d8b47f748025b26683116d21aa65
+        consensus.BIP16Exception = uint256S("0x00000000dd30457c001f4095d208cc1296b0eed002427aa599874af7a432b105");
         consensus.BIP34Height = 21111;
         consensus.BIP34Hash = uint256S("0x0000000023b3a96d3484e5abb3755c413e7d41500f8e2a5c3f0dd01299cd8ef8");
         consensus.BIP65Height = 581885; // 00000000007f6655f22f98e72ed80d8b06dc761d5da09df0fa1dc4be4f861eb6
@@ -283,7 +283,7 @@ public:
     CRegTestParams() {
         strNetworkID = "regtest";
         consensus.nSubsidyHalvingInterval = 150;
-        consensus.BIP16Height = 0; // always enforce P2SH BIP16 on regtest
+        consensus.BIP16Exception = uint256();
         consensus.BIP34Height = 100000000; // BIP34 has not activated on regtest (far in the future so block v1 are not rejected in tests)
         consensus.BIP34Hash = uint256();
         consensus.BIP65Height = 1351; // BIP65 activated on regtest (Used in rpc activation tests)
diff --git a/src/consensus/params.h b/src/consensus/params.h
index 4ef808c856..0559304fc2 100644
--- a/src/consensus/params.h
+++ b/src/consensus/params.h
@@ -49,8 +49,8 @@ struct BIP9Deployment {
 struct Params {
     uint256 hashGenesisBlock;
     int nSubsidyHalvingInterval;
-    /** Block height at which BIP16 becomes active */
-    int BIP16Height;
+    /* Block hash that is excepted from BIP16 enforcement */
+    uint256 BIP16Exception;
     /** Block height and hash at which BIP34 becomes active */
     int BIP34Height;
     uint256 BIP34Hash;
diff --git a/src/validation.cpp b/src/validation.cpp
index 3726cb3b14..daa33d3f5a 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -1726,16 +1726,38 @@ public:
 // Protected by cs_main
 static ThresholdConditionCache warningcache[VERSIONBITS_NUM_BITS];
 
+// 0.13.0 was shipped with a segwit deployment defined for testnet, but not for
+// mainnet. We no longer need to support disabling the segwit deployment
+// except for testing purposes, due to limitations of the functional test
+// environment. See test/functional/p2p-segwit.py.
+static bool IsScriptWitnessEnabled(const Consensus::Params& params)
+{
+    return params.vDeployments[Consensus::DEPLOYMENT_SEGWIT].nTimeout != 0;
+}
+
 static unsigned int GetBlockScriptFlags(const CBlockIndex* pindex, const Consensus::Params& consensusparams) {
     AssertLockHeld(cs_main);
 
     unsigned int flags = SCRIPT_VERIFY_NONE;
 
-    // Start enforcing P2SH (BIP16)
-    if (pindex->nHeight >= consensusparams.BIP16Height) {
+    // BIP16 didn't become active until Apr 1 2012 (on mainnet, and
+    // retroactively applied to testnet)
+    // However, only one historical block violated the P2SH rules (on both
+    // mainnet and testnet), so for simplicity, always leave P2SH
+    // on except for the one violating block.
+    if (consensusparams.BIP16Exception.IsNull() || // no bip16 exception on this chain
+        pindex->phashBlock == nullptr || // this is a new candidate block, eg from TestBlockValidity()
+        *pindex->phashBlock != consensusparams.BIP16Exception) // this block isn't the historical exception
+    {
         flags |= SCRIPT_VERIFY_P2SH;
     }
 
+    // Enforce WITNESS rules whenever P2SH is in effect (and the segwit
+    // deployment is defined).
+    if (flags & SCRIPT_VERIFY_P2SH && IsScriptWitnessEnabled(consensusparams)) {
+        flags |= SCRIPT_VERIFY_WITNESS;
+    }
+
     // Start enforcing the DERSIG (BIP66) rule
     if (pindex->nHeight >= consensusparams.BIP66Height) {
         flags |= SCRIPT_VERIFY_DERSIG;
@@ -1751,9 +1773,7 @@ static unsigned int GetBlockScriptFlags(const CBlockIndex* pindex, const Consens
         flags |= SCRIPT_VERIFY_CHECKSEQUENCEVERIFY;
     }
 
-    // Start enforcing WITNESS rules using versionbits logic.
-    if (IsWitnessEnabled(pindex->pprev, consensusparams)) {
-        flags |= SCRIPT_VERIFY_WITNESS;
+    if (IsNullDummyEnabled(pindex->pprev, consensusparams)) {
         flags |= SCRIPT_VERIFY_NULLDUMMY;
     }
 
@@ -3099,6 +3119,12 @@ bool IsWitnessEnabled(const CBlockIndex* pindexPrev, const Consensus::Params& pa
     return (VersionBitsState(pindexPrev, params, Consensus::DEPLOYMENT_SEGWIT, versionbitscache) == ThresholdState::ACTIVE);
 }
 
+bool IsNullDummyEnabled(const CBlockIndex* pindexPrev, const Consensus::Params& params)
+{
+    LOCK(cs_main);
+    return (VersionBitsState(pindexPrev, params, Consensus::DEPLOYMENT_SEGWIT, versionbitscache) == ThresholdState::ACTIVE);
+}
+
 // Compute at which vout of the block's coinbase transaction the witness
 // commitment occurs, or -1 if not found.
 static int GetWitnessCommitmentIndex(const CBlock& block)
@@ -4091,6 +4117,9 @@ bool CChainState::RewindBlockIndex(const CChainParams& params)
 
     int nHeight = 1;
     while (nHeight <= chainActive.Height()) {
+        // Although SCRIPT_VERIFY_WITNESS is now generally enforced on all
+        // blocks in ConnectBlock, we don't need to go back and
+        // re-download/re-verify blocks from before segwit actually activated.
         if (IsWitnessEnabled(chainActive[nHeight - 1], params.GetConsensus()) && !(chainActive[nHeight]->nStatus & BLOCK_OPT_WITNESS)) {
             break;
         }
diff --git a/src/validation.h b/src/validation.h
index 3668484696..b415a85053 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -411,6 +411,9 @@ bool TestBlockValidity(CValidationState& state, const CChainParams& chainparams,
 /** Check whether witness commitments are required for block. */
 bool IsWitnessEnabled(const CBlockIndex* pindexPrev, const Consensus::Params& params);
 
+/** Check whether NULLDUMMY (BIP 147) has activated. */
+bool IsNullDummyEnabled(const CBlockIndex* pindexPrev, const Consensus::Params& params);
+
 /** When there are blocks in the active chain with missing data, rewind the chainstate and remove them from the block index */
 bool RewindBlockIndex(const CChainParams& params);
 
diff --git a/test/functional/feature_segwit.py b/test/functional/feature_segwit.py
index fa1732c4c5..e835b9d777 100755
--- a/test/functional/feature_segwit.py
+++ b/test/functional/feature_segwit.py
@@ -150,19 +150,11 @@ class SegWitTest(BitcoinTestFramework):
         self.skip_mine(self.nodes[2], p2sh_ids[NODE_2][WIT_V0][0], True) #block 426
         self.skip_mine(self.nodes[2], p2sh_ids[NODE_2][WIT_V1][0], True) #block 427
 
-        # TODO: An old node would see these txs without witnesses and be able to mine them
-
-        self.log.info("Verify unsigned bare witness txs in versionbits-setting blocks are valid before the fork")
-        self.success_mine(self.nodes[2], wit_ids[NODE_2][WIT_V0][1], False) #block 428
-        self.success_mine(self.nodes[2], wit_ids[NODE_2][WIT_V1][1], False) #block 429
-
         self.log.info("Verify unsigned p2sh witness txs without a redeem script are invalid")
         self.fail_accept(self.nodes[2], "mandatory-script-verify-flag", p2sh_ids[NODE_2][WIT_V0][1], False)
         self.fail_accept(self.nodes[2], "mandatory-script-verify-flag", p2sh_ids[NODE_2][WIT_V1][1], False)
 
-        self.log.info("Verify unsigned p2sh witness txs with a redeem script in versionbits-settings blocks are valid before the fork")
-        self.success_mine(self.nodes[2], p2sh_ids[NODE_2][WIT_V0][1], False, witness_script(False, self.pubkey[2])) #block 430
-        self.success_mine(self.nodes[2], p2sh_ids[NODE_2][WIT_V1][1], False, witness_script(True, self.pubkey[2])) #block 431
+        self.nodes[2].generate(4) # blocks 428-431
 
         self.log.info("Verify previous witness txs skipped for mining can now be mined")
         assert_equal(len(self.nodes[2].getrawmempool()), 4)
diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py
index f8afe22eaf..4fecd4ffee 100755
--- a/test/functional/p2p_segwit.py
+++ b/test/functional/p2p_segwit.py
@@ -48,7 +48,7 @@ def test_transaction_acceptance(rpc, p2p, tx, with_witness, accepted, reason=Non
         with mininode_lock:
             assert_equal(p2p.last_message["reject"].reason, reason)
 
-def test_witness_block(rpc, p2p, block, accepted, with_witness=True):
+def test_witness_block(rpc, p2p, block, accepted, with_witness=True, reason=None):
     """Send a block to the node and check that it's accepted
 
     - Submit the block over the p2p interface
@@ -59,6 +59,10 @@ def test_witness_block(rpc, p2p, block, accepted, with_witness=True):
         p2p.send_message(msg_block(block))
     p2p.sync_with_ping()
     assert_equal(rpc.getbestblockhash() == block.hash, accepted)
+    if (reason != None and not accepted):
+        # Check the rejection reason as well.
+        with mininode_lock:
+            assert_equal(p2p.last_message["reject"].reason, reason)
 
 class TestP2PConn(P2PInterface):
     def __init__(self):
@@ -272,6 +276,80 @@ class SegWitTest(BitcoinTestFramework):
         self.utxo.pop(0)
         self.utxo.append(UTXO(tx4.sha256, 0, tx4.vout[0].nValue))
 
+    # ~6 months after segwit activation, the SCRIPT_VERIFY_WITNESS flag was
+    # backdated so that it applies to all blocks, going back to the genesis
+    # block.
+    #
+    # Consequently, version 0 witness outputs are never spendable without
+    # witness, and so can't be spent before segwit activation (the point at which
+    # blocks are permitted to contain witnesses).
+    def test_v0_outputs_arent_spendable(self):
+        self.log.info("Testing that v0 witness program outputs aren't spendable before activation")
+
+        assert len(self.utxo), "self.utxo is empty"
+
+        # Create two outputs, a p2wsh and p2sh-p2wsh
+        witness_program = CScript([OP_TRUE])
+        witness_hash = sha256(witness_program)
+        scriptPubKey = CScript([OP_0, witness_hash])
+
+        p2sh_pubkey = hash160(scriptPubKey)
+        p2sh_scriptPubKey = CScript([OP_HASH160, p2sh_pubkey, OP_EQUAL])
+
+        value = self.utxo[0].nValue // 3
+
+        tx = CTransaction()
+        tx.vin = [CTxIn(COutPoint(self.utxo[0].sha256, self.utxo[0].n), b'')]
+        tx.vout = [CTxOut(value, scriptPubKey), CTxOut(value, p2sh_scriptPubKey)]
+        tx.vout.append(CTxOut(value, CScript([OP_TRUE])))
+        tx.rehash()
+        txid = tx.sha256
+
+        # Add it to a block
+        block = self.build_next_block()
+        self.update_witness_block_with_transactions(block, [tx])
+        # Verify that segwit isn't activated. A block serialized with witness
+        # should be rejected prior to activation.
+        test_witness_block(self.nodes[0], self.test_node, block, accepted=False, with_witness=True, reason = b'unexpected-witness')
+        # Now send the block without witness. It should be accepted
+        test_witness_block(self.nodes[0], self.test_node, block, accepted=True, with_witness=False)
+
+        # Now try to spend the outputs. This should fail since SCRIPT_VERIFY_WITNESS is always enabled.
+        p2wsh_tx = CTransaction()
+        p2wsh_tx.vin = [CTxIn(COutPoint(txid, 0), b'')]
+        p2wsh_tx.vout = [CTxOut(value, CScript([OP_TRUE]))]
+        p2wsh_tx.wit.vtxinwit.append(CTxInWitness())
+        p2wsh_tx.wit.vtxinwit[0].scriptWitness.stack = [CScript([OP_TRUE])]
+        p2wsh_tx.rehash()
+
+        p2sh_p2wsh_tx = CTransaction()
+        p2sh_p2wsh_tx.vin = [CTxIn(COutPoint(txid, 1), CScript([scriptPubKey]))]
+        p2sh_p2wsh_tx.vout = [CTxOut(value, CScript([OP_TRUE]))]
+        p2sh_p2wsh_tx.wit.vtxinwit.append(CTxInWitness())
+        p2sh_p2wsh_tx.wit.vtxinwit[0].scriptWitness.stack = [CScript([OP_TRUE])]
+        p2sh_p2wsh_tx.rehash()
+
+        for tx in [p2wsh_tx, p2sh_p2wsh_tx]:
+
+            block = self.build_next_block()
+            self.update_witness_block_with_transactions(block, [tx])
+
+            # When the block is serialized with a witness, the block will be rejected because witness
+            # data isn't allowed in blocks that don't commit to witness data.
+            test_witness_block(self.nodes[0], self.test_node, block, accepted=False, with_witness=True, reason=b'unexpected-witness')
+
+            # When the block is serialized without witness, validation fails because the transaction is
+            # invalid (transactions are always validated with SCRIPT_VERIFY_WITNESS so a segwit v0 transaction
+            # without a witness is invalid).
+            # Note: The reject reason for this failure could be
+            # 'block-validation-failed' (if script check threads > 1) or
+            # 'non-mandatory-script-verify-flag (Witness program was passed an
+            # empty witness)' (otherwise).
+            # TODO: support multiple acceptable reject reasons.
+            test_witness_block(self.nodes[0], self.test_node, block, accepted=False, with_witness=False)
+
+        self.utxo.pop(0)
+        self.utxo.append(UTXO(txid, 2, value))
 
     # Mine enough blocks for segwit's vb state to be 'started'.
     def advance_to_segwit_started(self):
@@ -1479,9 +1557,10 @@ class SegWitTest(BitcoinTestFramework):
         block = self.build_next_block()
         self.update_witness_block_with_transactions(block, [spend_tx])
 
-        # If we're before activation, then sending this without witnesses
-        # should be valid.  If we're after activation, then sending this with
-        # witnesses should be valid.
+        # If we're after activation, then sending this with witnesses should be valid.
+        # This no longer works before activation, because SCRIPT_VERIFY_WITNESS
+        # is always set.
+        # TODO: rewrite this test to make clear that it only works after activation.
         if segwit_activated:
             test_witness_block(self.nodes[0].rpc, self.test_node, block, accepted=True)
         else:
@@ -1900,6 +1979,7 @@ class SegWitTest(BitcoinTestFramework):
         self.test_witness_services() # Verifies NODE_WITNESS
         self.test_non_witness_transaction() # non-witness tx's are accepted
         self.test_unnecessary_witness_before_segwit_activation()
+        self.test_v0_outputs_arent_spendable()
         self.test_block_relay(segwit_activated=False)
 
         # Advance to segwit being 'started'
@@ -1917,7 +1997,6 @@ class SegWitTest(BitcoinTestFramework):
         self.test_unnecessary_witness_before_segwit_activation()
         self.test_witness_tx_relay_before_segwit_activation()
         self.test_block_relay(segwit_activated=False)
-        self.test_p2sh_witness(segwit_activated=False)
         self.test_standardness_v0(segwit_activated=False)
 
         sync_blocks(self.nodes)