diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -190,13 +190,6 @@ case OP_CHECKLOCKTIMEVERIFY: { if (!(flags & SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY)) { - // not enabled; treat as a NOP2 - if (flags & - SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS) { - return set_error( - serror, - ScriptError::DISCOURAGE_UPGRADABLE_NOPS); - } break; } @@ -243,13 +236,6 @@ case OP_CHECKSEQUENCEVERIFY: { if (!(flags & SCRIPT_VERIFY_CHECKSEQUENCEVERIFY)) { - // not enabled; treat as a NOP3 - if (flags & - SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS) { - return set_error( - serror, - ScriptError::DISCOURAGE_UPGRADABLE_NOPS); - } break; } diff --git a/src/script/script_flags.h b/src/script/script_flags.h --- a/src/script/script_flags.h +++ b/src/script/script_flags.h @@ -22,16 +22,16 @@ SCRIPT_VERIFY_STRICTENC = (1U << 1), // Passing a non-strict-DER signature to a checksig operation causes script - // failure (softfork safe, BIP62 rule 1) + // failure (BIP62 rule 1) SCRIPT_VERIFY_DERSIG = (1U << 2), // Passing a non-strict-DER signature or one with S > order/2 to a checksig // operation causes script failure - // (softfork safe, BIP62 rule 5). + // (BIP62 rule 5). SCRIPT_VERIFY_LOW_S = (1U << 3), // Using a non-push operator in the scriptSig causes script failure - // (softfork safe, BIP62 rule 2). + // (BIP62 rule 2). SCRIPT_VERIFY_SIGPUSHONLY = (1U << 5), // Require minimal encodings for all push operations (OP_0... OP_16, @@ -40,7 +40,6 @@ // push causes the script to fail (BIP62 rule 3). In addition, whenever a // stack element is interpreted as a number, it must be of minimal length // (BIP62 rule 4). - // (softfork safe) SCRIPT_VERIFY_MINIMALDATA = (1U << 6), // Discourage use of NOPs reserved for upgrades (NOP1-10) @@ -58,8 +57,8 @@ // remain, and when interpreted as a boolean, it must be true" to "Exactly // one stack element must remain, and when interpreted as a boolean, it must // be true". - // (softfork safe, BIP62 rule 6) - // Note: CLEANSTACK should never be used without P2SH or WITNESS. + // (BIP62 rule 6) + // Note: CLEANSTACK should never be used without P2SH. // Note: The Segwit Recovery feature is an exception to CLEANSTACK SCRIPT_VERIFY_CLEANSTACK = (1U << 8), diff --git a/src/test/data/script_tests.json b/src/test/data/script_tests.json --- a/src/test/data/script_tests.json +++ b/src/test/data/script_tests.json @@ -1227,8 +1227,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 --- a/src/test/script_tests.cpp +++ b/src/test/script_tests.cpp @@ -175,6 +175,34 @@ std::string(FormatScriptError(err)) + " where " + std::string(FormatScriptError(scriptError)) + " expected: " + message); + + // Verify that removing flags from a passing test or adding flags to a + // failing test does not change the result, except for some special flags. + for (int i = 0; i < 16; ++i) { + uint32_t extra_flags = InsecureRandBits(32); + uint32_t combined_flags = + expect ? (flags & ~extra_flags) : (flags | extra_flags); + // Weed out invalid flag combinations. + if (combined_flags & SCRIPT_VERIFY_CLEANSTACK) { + combined_flags |= SCRIPT_VERIFY_P2SH; + } + // Some flags are not purely-restrictive and thus we can't assume + // anything about what happens when they are flipped. Keep as-is. + constexpr uint32_t keep_mask = SCRIPT_ENABLE_SIGHASH_FORKID | + SCRIPT_ENABLE_REPLAY_PROTECTION | + SCRIPT_ENABLE_SCHNORR_MULTISIG; + combined_flags = (combined_flags & ~keep_mask) | (flags & keep_mask); + + BOOST_CHECK_MESSAGE(VerifyScript(scriptSig, scriptPubKey, + combined_flags, + MutableTransactionSignatureChecker( + &tx, 0, txCredit.vout[0].nValue), + &err) == expect, + message + strprintf(" (with %s flags %08x)", + expect ? "removed" : "added", + combined_flags ^ 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 --- a/src/test/txvalidationcache_tests.cpp +++ b/src/test/txvalidationcache_tests.cpp @@ -95,18 +95,9 @@ // 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. -// CHECKLOCKTIMEVERIFY and CHECKSEQUENCEVERIFY (and future NOP codes that may -// get reassigned) have an interaction with DISCOURAGE_UPGRADABLE_NOPS: if the -// script flags used contain DISCOURAGE_UPGRADABLE_NOPS but don't contain -// CHECKLOCKTIMEVERIFY (or CHECKSEQUENCEVERIFY), but the script does contain -// OP_CHECKLOCKTIMEVERIFY (or OP_CHECKSEQUENCEVERIFY), then script execution -// 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. -static void ValidateCheckInputsForAllFlags(const CTransaction &tx, - uint32_t failing_flags, - uint32_t required_flags, - bool add_to_cache, bool upgraded_nop) +static void +ValidateCheckInputsForAllFlags(const CTransaction &tx, uint32_t failing_flags, + uint32_t required_flags, bool add_to_cache) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { PrecomputedTransactionData txdata(tx); @@ -128,14 +119,6 @@ // 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 @@ -269,7 +252,7 @@ // later that block validation works fine in the absence of cached // successes. ValidateCheckInputsForAllFlags( - tx, SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS, 0, false, false); + tx, SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS, 0, false); } // And if we produce a block with this tx, it should be valid, even though @@ -296,7 +279,7 @@ invalid_under_p2sh_tx.vin[0].scriptSig << vchSig2; ValidateCheckInputsForAllFlags(CTransaction(invalid_under_p2sh_tx), - SCRIPT_VERIFY_P2SH, 0, true, false); + SCRIPT_VERIFY_P2SH, 0, true); } // Test CHECKLOCKTIMEVERIFY @@ -320,10 +303,10 @@ vchSig.push_back(uint8_t(SIGHASH_ALL | SIGHASH_FORKID)); invalid_with_cltv_tx.vin[0].scriptSig = CScript() << vchSig << 101; - ValidateCheckInputsForAllFlags( - CTransaction(invalid_with_cltv_tx), - SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY | SCRIPT_ENABLE_REPLAY_PROTECTION, - SCRIPT_ENABLE_SIGHASH_FORKID, true, true); + ValidateCheckInputsForAllFlags(CTransaction(invalid_with_cltv_tx), + SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY | + SCRIPT_ENABLE_REPLAY_PROTECTION, + SCRIPT_ENABLE_SIGHASH_FORKID, true); // Make it valid, and check again invalid_with_cltv_tx.vin[0].scriptSig = CScript() << vchSig << 100; @@ -358,10 +341,10 @@ vchSig.push_back(uint8_t(SIGHASH_ALL | SIGHASH_FORKID)); invalid_with_csv_tx.vin[0].scriptSig = CScript() << vchSig << 101; - ValidateCheckInputsForAllFlags( - CTransaction(invalid_with_csv_tx), - SCRIPT_VERIFY_CHECKSEQUENCEVERIFY | SCRIPT_ENABLE_REPLAY_PROTECTION, - SCRIPT_ENABLE_SIGHASH_FORKID, true, true); + ValidateCheckInputsForAllFlags(CTransaction(invalid_with_csv_tx), + SCRIPT_VERIFY_CHECKSEQUENCEVERIFY | + SCRIPT_ENABLE_REPLAY_PROTECTION, + SCRIPT_ENABLE_SIGHASH_FORKID, true); // Make it valid, and check again invalid_with_csv_tx.vin[0].scriptSig = CScript() << vchSig << 100; @@ -414,7 +397,7 @@ // convention. ValidateCheckInputsForAllFlags( CTransaction(tx), SCRIPT_ENABLE_REPLAY_PROTECTION, - SCRIPT_ENABLE_SIGHASH_FORKID | SCRIPT_VERIFY_P2SH, true, false); + SCRIPT_ENABLE_SIGHASH_FORKID | SCRIPT_VERIFY_P2SH, true); // Check that if the second input is invalid, but the first input is // valid, the transaction is not cached.