LBRYCommunity/lbrycrd
1
0
Fork 0
Code Issues 29 Pull requests 1 Projects Releases 32 Packages Wiki Activity

Merge #10699: Make all script validation flags backward compatible

Browse source 
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
This commit is contained in:
Wladimir J. van der Laan 2017-12-12 10:10:14 +01:00
commit c0902624b0
No known key found for this signature in database
GPG key ID: 1E4AED62986CD25D
5 changed files with 33 additions and 31 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

6
src/script/interpreter.cpp
View file

@ -349,9 +349,6 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
{ {
if (!(flags & SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY)) { if (!(flags & SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY)) {
// not enabled; treat as a NOP2 // not enabled; treat as a NOP2
if (flags & SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS) {
return set_error(serror, SCRIPT_ERR_DISCOURAGE_UPGRADABLE_NOPS);
}
break; break;
} }
@ -391,9 +388,6 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
{ {
if (!(flags & SCRIPT_VERIFY_CHECKSEQUENCEVERIFY)) { if (!(flags & SCRIPT_VERIFY_CHECKSEQUENCEVERIFY)) {
// not enabled; treat as a NOP3 // not enabled; treat as a NOP3
if (flags & SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS) {
return set_error(serror, SCRIPT_ERR_DISCOURAGE_UPGRADABLE_NOPS);
}
break; break;
} }

23
src/script/interpreter.h
View file

@ -27,37 +27,40 @@ enum
SIGHASH_ANYONECANPAY = 0x80, SIGHASH_ANYONECANPAY = 0x80,
}; };
/** Script verification flags */ /** Script verification flags.
*
* All flags are intended to be soft forks: the set of acceptable scripts under
* flags (A | B) is a subset of the acceptable scripts under flag (A).
*/
enum enum
{ {
SCRIPT_VERIFY_NONE = 0, SCRIPT_VERIFY_NONE = 0,
// Evaluate P2SH subscripts (softfork safe, BIP16). // Evaluate P2SH subscripts (BIP16).
SCRIPT_VERIFY_P2SH = (1U << 0), SCRIPT_VERIFY_P2SH = (1U << 0),
// Passing a non-strict-DER signature or one with undefined hashtype to a checksig operation causes script failure. // Passing a non-strict-DER signature or one with undefined hashtype to a checksig operation causes script failure.
// Evaluating a pubkey that is not (0x04 + 64 bytes) or (0x02 or 0x03 + 32 bytes) by checksig causes script failure. // Evaluating a pubkey that is not (0x04 + 64 bytes) or (0x02 or 0x03 + 32 bytes) by checksig causes script failure.
// (softfork safe, but not used or intended as a consensus rule). // (not used or intended as a consensus rule).
SCRIPT_VERIFY_STRICTENC = (1U << 1), SCRIPT_VERIFY_STRICTENC = (1U << 1),
// Passing a non-strict-DER signature to a checksig operation causes script failure (softfork safe, BIP62 rule 1) // Passing a non-strict-DER signature to a checksig operation causes script failure (BIP62 rule 1)
SCRIPT_VERIFY_DERSIG = (1U << 2), SCRIPT_VERIFY_DERSIG = (1U << 2),
// Passing a non-strict-DER signature or one with S > order/2 to a checksig operation causes script failure // 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), SCRIPT_VERIFY_LOW_S = (1U << 3),
// verify dummy stack item consumed by CHECKMULTISIG is of zero-length (softfork safe, BIP62 rule 7). // verify dummy stack item consumed by CHECKMULTISIG is of zero-length (BIP62 rule 7).
SCRIPT_VERIFY_NULLDUMMY = (1U << 4), SCRIPT_VERIFY_NULLDUMMY = (1U << 4),
// Using a non-push operator in the scriptSig causes script failure (softfork safe, BIP62 rule 2). // Using a non-push operator in the scriptSig causes script failure (BIP62 rule 2).
SCRIPT_VERIFY_SIGPUSHONLY = (1U << 5), SCRIPT_VERIFY_SIGPUSHONLY = (1U << 5),
// Require minimal encodings for all push operations (OP_0... OP_16, OP_1NEGATE where possible, direct // Require minimal encodings for all push operations (OP_0... OP_16, OP_1NEGATE where possible, direct
// pushes up to 75 bytes, OP_PUSHDATA up to 255 bytes, OP_PUSHDATA2 for anything larger). Evaluating // pushes up to 75 bytes, OP_PUSHDATA up to 255 bytes, OP_PUSHDATA2 for anything larger). Evaluating
// any other push causes the script to fail (BIP62 rule 3). // any other 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). // 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), SCRIPT_VERIFY_MINIMALDATA = (1U << 6),
// Discourage use of NOPs reserved for upgrades (NOP1-10) // Discourage use of NOPs reserved for upgrades (NOP1-10)
@ -68,12 +71,14 @@ enum
// discouraged NOPs fails the script. This verification flag will never be // discouraged NOPs fails the script. This verification flag will never be
// a mandatory flag applied to scripts in a block. NOPs that are not // a mandatory flag applied to scripts in a block. NOPs that are not
// executed, e.g. within an unexecuted IF ENDIF block, are *not* rejected. // executed, e.g. within an unexecuted IF ENDIF block, are *not* rejected.
// NOPs that have associated forks to give them new meaning (CLTV, CSV)
// are not subject to this rule.
SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS = (1U << 7), SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS = (1U << 7),
// Require that only a single stack element remains after evaluation. This changes the success criterion from // Require that only a single stack element remains after evaluation. This changes the success criterion from
// "At least one stack element must remain, and when interpreted as a boolean, it must be true" to // "At least one stack element must 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". // "Exactly one stack element must remain, and when interpreted as a boolean, it must be true".
// (softfork safe, BIP62 rule 6) // (BIP62 rule 6)
// Note: CLEANSTACK should never be used without P2SH or WITNESS. // Note: CLEANSTACK should never be used without P2SH or WITNESS.
SCRIPT_VERIFY_CLEANSTACK = (1U << 8), SCRIPT_VERIFY_CLEANSTACK = (1U << 8),

2
src/test/data/script_tests.json
View file

@ -862,8 +862,6 @@
["Ensure 100% coverage of discouraged NOPS"], ["Ensure 100% coverage of discouraged NOPS"],
["1", "NOP1", "P2SH,DISCOURAGE_UPGRADABLE_NOPS", "DISCOURAGE_UPGRADABLE_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", "NOP4", "P2SH,DISCOURAGE_UPGRADABLE_NOPS", "DISCOURAGE_UPGRADABLE_NOPS"],
["1", "NOP5", "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"], ["1", "NOP6", "P2SH,DISCOURAGE_UPGRADABLE_NOPS", "DISCOURAGE_UPGRADABLE_NOPS"],

11
src/test/script_tests.cpp
View file

@ -166,6 +166,17 @@ void DoTest(const CScript& scriptPubKey, const CScript& scriptSig, const CScript
CMutableTransaction tx2 = tx; CMutableTransaction tx2 = tx;
BOOST_CHECK_MESSAGE(VerifyScript(scriptSig, scriptPubKey, &scriptWitness, flags, MutableTransactionSignatureChecker(&tx, 0, txCredit.vout[0].nValue), &err) == expect, message); 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); 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) #if defined(HAVE_CONSENSUS_LIB)
CDataStream stream(SER_NETWORK, PROTOCOL_VERSION); CDataStream stream(SER_NETWORK, PROTOCOL_VERSION);
stream << tx2; stream << tx2;

22
src/test/txvalidationcache_tests.cpp
View file

@ -103,7 +103,7 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, TestChain100Setup)
// should fail. // should fail.
// Capture this interaction with the upgraded_nop argument: set it when evaluating // Capture this interaction with the upgraded_nop argument: set it when evaluating
// any script flag that is implemented as an upgraded NOP code. // 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); PrecomputedTransactionData txdata(tx);
// If we add many more flags, this loop can get too expensive, but we can // 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 // CheckInputs should succeed iff test_flags doesn't intersect with
// failing_flags // failing_flags
bool expected_return_value = !(test_flags & 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); BOOST_CHECK_EQUAL(ret, expected_return_value);
// Test the caching // 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 // 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 // test later that block validation works fine in the absence of cached
// successes. // 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 // And if we produce a block with this tx, it should be valid (DERSIG not
// enabled yet), even though there's no cache entry. // 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()); std::vector<unsigned char> vchSig2(p2pk_scriptPubKey.begin(), p2pk_scriptPubKey.end());
invalid_under_p2sh_tx.vin[0].scriptSig << vchSig2; 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 // Test CHECKLOCKTIMEVERIFY
@ -266,7 +260,7 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup)
vchSig.push_back((unsigned char)SIGHASH_ALL); vchSig.push_back((unsigned char)SIGHASH_ALL);
invalid_with_cltv_tx.vin[0].scriptSig = CScript() << vchSig << 101; 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 // Make it valid, and check again
invalid_with_cltv_tx.vin[0].scriptSig = CScript() << vchSig << 100; 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); vchSig.push_back((unsigned char)SIGHASH_ALL);
invalid_with_csv_tx.vin[0].scriptSig = CScript() << vchSig << 101; 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 // Make it valid, and check again
invalid_with_csv_tx.vin[0].scriptSig = CScript() << vchSig << 100; 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); UpdateTransaction(valid_with_witness_tx, 0, sigdata);
// This should be valid under all script flags. // 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. // Remove the witness, and check that it is now invalid.
valid_with_witness_tx.vin[0].scriptWitness.SetNull(); 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 // 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 // Check that if the second input is invalid, but the first input is
// valid, the transaction is not cached. // valid, the transaction is not cached.