Merge #10699: Make all script validation flags backward compatible

01013f5 Simplify tx validation tests (Pieter Wuille)
2dd6f80 Add a test that all flags are softforks (Pieter Wuille)
2851b77 Make all script verification flags softforks (Pieter Wuille)

Pull request description:

  This change makes `SCRIPT_VERIFY_UPGRADABLE_NOPS` not apply to `OP_CHECKLOCKTIMEVERIFY` and `OP_CHECKSEQUENCEVERIFY`. This is a no-op as `UPGRADABLE_NOPS` is only set for mempool transactions, and those always have `SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY` and `SCRIPT_VERIFY_CHECKSEQUENCEVERIFY` set as well. The advantage is that setting more flags now always results in a reduction in acceptable scripts (=softfork).

  This results in a nice and testable property for validation, for which a new test is added.

  This also means that the introduction of a new definition for a NOP or witness version will likely need the following procedure (example OP_NOP8 here)
  * Remove OP_NOP8 from being affected by `SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS`.
  * Add a `SCRIPT_VERIFY_DISCOURAGE_NOP8`, which only applies to `OP_NOP8`.
  * Add a `SCRIPT_VERIFY_NOP8` which implements the new consensus logic.
  * Before activation, add `SCRIPT_VERIFY_DISCOURAGE_NOP8` to the mempool flags.
  * After activation, add `SCRIPT_VERIFY_NOP8` to both the mempool and consensus flags.

Tree-SHA512: d3b4538986ecf646aac9dba13a8d89318baf9e308e258547ca3b99e7c0509747f323edac6b1fea4e87e7d3c01b71193794b41679ae4f86f6e11ed6be3fd62c72

3 files changed, 19 insertions, 16 deletions

diff --git a/src/test/data/script_tests.json b/src/test/data/script_tests.json
index 698e898231..63f43c0fc6 100644
--- a/src/test/data/script_tests.json
+++ b/src/test/data/script_tests.json
@@ -862,8 +862,6 @@
 
 ["Ensure 100% coverage of discouraged NOPS"],
 ["1", "NOP1",  "P2SH,DISCOURAGE_UPGRADABLE_NOPS", "DISCOURAGE_UPGRADABLE_NOPS"],
-["1", "CHECKLOCKTIMEVERIFY",  "P2SH,DISCOURAGE_UPGRADABLE_NOPS", "DISCOURAGE_UPGRADABLE_NOPS"],
-["1", "CHECKSEQUENCEVERIFY",  "P2SH,DISCOURAGE_UPGRADABLE_NOPS", "DISCOURAGE_UPGRADABLE_NOPS"],
 ["1", "NOP4",  "P2SH,DISCOURAGE_UPGRADABLE_NOPS", "DISCOURAGE_UPGRADABLE_NOPS"],
 ["1", "NOP5",  "P2SH,DISCOURAGE_UPGRADABLE_NOPS", "DISCOURAGE_UPGRADABLE_NOPS"],
 ["1", "NOP6",  "P2SH,DISCOURAGE_UPGRADABLE_NOPS", "DISCOURAGE_UPGRADABLE_NOPS"],
diff --git a/src/test/script_tests.cpp b/src/test/script_tests.cpp
index 57b3e501af..f96d867bc6 100644
--- a/src/test/script_tests.cpp
+++ b/src/test/script_tests.cpp
@@ -166,6 +166,17 @@ void DoTest(const CScript& scriptPubKey, const CScript& scriptSig, const CScript
     CMutableTransaction tx2 = tx;
     BOOST_CHECK_MESSAGE(VerifyScript(scriptSig, scriptPubKey, &scriptWitness, flags, MutableTransactionSignatureChecker(&tx, 0, txCredit.vout[0].nValue), &err) == expect, message);
     BOOST_CHECK_MESSAGE(err == scriptError, std::string(FormatScriptError(err)) + " where " + std::string(FormatScriptError((ScriptError_t)scriptError)) + " expected: " + message);
+
+    // Verify that removing flags from a passing test or adding flags to a failing test does not change the result.
+    for (int i = 0; i < 16; ++i) {
+        int extra_flags = InsecureRandBits(16);
+        int combined_flags = expect ? (flags & ~extra_flags) : (flags | extra_flags);
+        // Weed out some invalid flag combinations.
+        if (combined_flags & SCRIPT_VERIFY_CLEANSTACK && ~combined_flags & (SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS)) continue;
+        if (combined_flags & SCRIPT_VERIFY_WITNESS && ~combined_flags & SCRIPT_VERIFY_P2SH) continue;
+        BOOST_CHECK_MESSAGE(VerifyScript(scriptSig, scriptPubKey, &scriptWitness, combined_flags, MutableTransactionSignatureChecker(&tx, 0, txCredit.vout[0].nValue), &err) == expect, message + strprintf(" (with flags %x)", combined_flags));
+    }
+
 #if defined(HAVE_CONSENSUS_LIB)
     CDataStream stream(SER_NETWORK, PROTOCOL_VERSION);
     stream << tx2;
diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp
index fa49b9c33b..fe8cb6126e 100644
--- a/src/test/txvalidationcache_tests.cpp
+++ b/src/test/txvalidationcache_tests.cpp
@@ -103,7 +103,7 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, TestChain100Setup)
 // should fail.
 // Capture this interaction with the upgraded_nop argument: set it when evaluating
 // any script flag that is implemented as an upgraded NOP code.
-void ValidateCheckInputsForAllFlags(CMutableTransaction &tx, uint32_t failing_flags, bool add_to_cache, bool upgraded_nop)
+void ValidateCheckInputsForAllFlags(CMutableTransaction &tx, uint32_t failing_flags, bool add_to_cache)
 {
     PrecomputedTransactionData txdata(tx);
     // If we add many more flags, this loop can get too expensive, but we can
@@ -124,12 +124,6 @@ void ValidateCheckInputsForAllFlags(CMutableTransaction &tx, uint32_t failing_fl
         // CheckInputs should succeed iff test_flags doesn't intersect with
         // failing_flags
         bool expected_return_value = !(test_flags & failing_flags);
-        if (expected_return_value && upgraded_nop) {
-            // If the script flag being tested corresponds to an upgraded NOP,
-            // then script execution should fail if DISCOURAGE_UPGRADABLE_NOPS
-            // is set.
-            expected_return_value = !(test_flags & SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS);
-        }
         BOOST_CHECK_EQUAL(ret, expected_return_value);
 
         // Test the caching
@@ -218,7 +212,7 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup)
         // not present.  Don't add these checks to the cache, so that we can
         // test later that block validation works fine in the absence of cached
         // successes.
-        ValidateCheckInputsForAllFlags(spend_tx, SCRIPT_VERIFY_DERSIG | SCRIPT_VERIFY_LOW_S | SCRIPT_VERIFY_STRICTENC, false, false);
+        ValidateCheckInputsForAllFlags(spend_tx, SCRIPT_VERIFY_DERSIG | SCRIPT_VERIFY_LOW_S | SCRIPT_VERIFY_STRICTENC, false);
 
         // And if we produce a block with this tx, it should be valid (DERSIG not
         // enabled yet), even though there's no cache entry.
@@ -243,7 +237,7 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup)
         std::vector<unsigned char> vchSig2(p2pk_scriptPubKey.begin(), p2pk_scriptPubKey.end());
         invalid_under_p2sh_tx.vin[0].scriptSig << vchSig2;
 
-        ValidateCheckInputsForAllFlags(invalid_under_p2sh_tx, SCRIPT_VERIFY_P2SH, true, false);
+        ValidateCheckInputsForAllFlags(invalid_under_p2sh_tx, SCRIPT_VERIFY_P2SH, true);
     }
 
     // Test CHECKLOCKTIMEVERIFY
@@ -266,7 +260,7 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup)
         vchSig.push_back((unsigned char)SIGHASH_ALL);
         invalid_with_cltv_tx.vin[0].scriptSig = CScript() << vchSig << 101;
 
-        ValidateCheckInputsForAllFlags(invalid_with_cltv_tx, SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY, true, true);
+        ValidateCheckInputsForAllFlags(invalid_with_cltv_tx, SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY, true);
 
         // Make it valid, and check again
         invalid_with_cltv_tx.vin[0].scriptSig = CScript() << vchSig << 100;
@@ -294,7 +288,7 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup)
         vchSig.push_back((unsigned char)SIGHASH_ALL);
         invalid_with_csv_tx.vin[0].scriptSig = CScript() << vchSig << 101;
 
-        ValidateCheckInputsForAllFlags(invalid_with_csv_tx, SCRIPT_VERIFY_CHECKSEQUENCEVERIFY, true, true);
+        ValidateCheckInputsForAllFlags(invalid_with_csv_tx, SCRIPT_VERIFY_CHECKSEQUENCEVERIFY, true);
 
         // Make it valid, and check again
         invalid_with_csv_tx.vin[0].scriptSig = CScript() << vchSig << 100;
@@ -323,11 +317,11 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup)
         UpdateTransaction(valid_with_witness_tx, 0, sigdata);
 
         // This should be valid under all script flags.
-        ValidateCheckInputsForAllFlags(valid_with_witness_tx, 0, true, false);
+        ValidateCheckInputsForAllFlags(valid_with_witness_tx, 0, true);
 
         // Remove the witness, and check that it is now invalid.
         valid_with_witness_tx.vin[0].scriptWitness.SetNull();
-        ValidateCheckInputsForAllFlags(valid_with_witness_tx, SCRIPT_VERIFY_WITNESS, true, false);
+        ValidateCheckInputsForAllFlags(valid_with_witness_tx, SCRIPT_VERIFY_WITNESS, true);
     }
 
     {
@@ -352,7 +346,7 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup)
         }
 
         // This should be valid under all script flags
-        ValidateCheckInputsForAllFlags(tx, 0, true, false);
+        ValidateCheckInputsForAllFlags(tx, 0, true);
 
         // Check that if the second input is invalid, but the first input is
         // valid, the transaction is not cached.