-
Notifications
You must be signed in to change notification settings - Fork 36.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make all script validation flags backward compatible #10699
Conversation
Can you rebase this on top of the now-merged #10192 to remove the extra checks in its test? Concept ACK. |
Done. |
utACK 01013f5 I did check some of the test changes failed appropriately, but wouldnt really call it a tested ACK per se. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 01013f5
utACK 2851b77, reviewed both test and interpreter changes, they seem clear and contained and could discover no issues with the changed logic. |
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
…ible 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
…ible 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
See bitcoin#10699, i.e. adding a flag should always reduce the number of acceptable scripts.
See bitcoin#10699, i.e. adding a flag should always reduce the number of acceptable scripts.
See bitcoin#10699, i.e. adding a flag should always reduce the number of acceptable scripts.
See bitcoin#10699, i.e. adding a flag should always reduce the number of acceptable scripts.
See bitcoin#10699, i.e. adding a flag should always reduce the number of acceptable scripts.
See bitcoin#10699, i.e. adding a flag should always reduce the number of acceptable scripts.
See bitcoin#10699, i.e. adding a flag should always reduce the number of acceptable scripts.
See bitcoin#10699, i.e. adding a flag should always reduce the number of acceptable scripts.
See bitcoin#10699, i.e. adding a flag should always reduce the number of acceptable scripts.
See bitcoin#10699, i.e. adding a flag should always reduce the number of acceptable scripts. Co-authored-by: Johnson Lau <jl2012@xbt.hk>
See bitcoin#10699, i.e. adding a flag should always reduce the number of acceptable scripts. Co-authored-by: Johnson Lau <jl2012@xbt.hk>
…ests and assert backwards compatibility 5786a81 Verify that all validation flags are backward compatible (gzhao408) b10ce9a [test] check verification flags are minimal/maximal (gzhao408) a260c22 [test] Check for invalid flag combinations (gzhao408) a7098a2 [refactor] use CheckTxScripts, TrimFlags, FillFlags (gzhao408) 7a77727 Apply minimal validation flags to tx_invalid tests (gzhao408) 9532591 [test] add BADTX setting for invalid txns that fail CheckTransaction (gzhao408) 4c06ebf [test] fix two witness tests in invalid tests with empty vout (gzhao408) 158a0b2 Apply maximal validation flags to tx_valid tests (gzhao408) 0a76a39 [test] fix CSV test missing OP_ADD (gzhao408) 19db590 [test] remove unnecessary OP_1s from CSV and CLTV tests (gzhao408) Pull request description: This uses the first 4 commits of #15045, rebased and added some comments. The diff is quite large already and I want to make it easy to review, so I'm splitting it into 2 PRs (transaction and script). Script one is WIP, I'll link it when I open it. Interpretation of scripts is dependent on the script verification flags passed in. In tests, we should always apply **maximal** verification flags when checking that a transaction is **valid**; any additional flags should invalidate the transaction. A transaction should not be valid because we forgot to include a flag, and we should apply all flags by default. We should apply **minimal** verification flags when asserting that a transaction is **invalid**; if verification flags are applied, removing any one of them should mean the transaction is valid. New verify flags must be backwards compatible; tests should check backwards compatibility and apply the new flags by default. All `tx_invalid` tests should continue to be invalid with the exact same verify flags. All `tx_valid` tests that don't pass with new flags should _explicitly_ indicate that the flags need to be excluded, and fail otherwise. 1. Flip the meaning of `verifyFlags` in tx_valid.json to mean _excluded_ verification flags instead of included flags. Edit the test data accordingly. 2. Trim unneeded flags from tx_invalid.json. 3. Add check to verify that tx_valid tests have maximal flags and tx_invalid tests have minimal flags. 4. Add checks to verify that flags are soft forks (#10699) i.e. adding any flag should only decrease the number of acceptable scripts. Test by adding/removing random flags. ACKs for top commit: achow101: ACK 5786a81 laanwj: ACK 5786a81 Tree-SHA512: 19195d8cf3299e62f47dd3443ae4a95430c5c9d497993a18ab80de9e24b1869787af972774993bf05717784879bc4592fdabaae0fddebd437963d8f3c96d9a73
…ction tests and assert backwards compatibility 5786a81 Verify that all validation flags are backward compatible (gzhao408) b10ce9a [test] check verification flags are minimal/maximal (gzhao408) a260c22 [test] Check for invalid flag combinations (gzhao408) a7098a2 [refactor] use CheckTxScripts, TrimFlags, FillFlags (gzhao408) 7a77727 Apply minimal validation flags to tx_invalid tests (gzhao408) 9532591 [test] add BADTX setting for invalid txns that fail CheckTransaction (gzhao408) 4c06ebf [test] fix two witness tests in invalid tests with empty vout (gzhao408) 158a0b2 Apply maximal validation flags to tx_valid tests (gzhao408) 0a76a39 [test] fix CSV test missing OP_ADD (gzhao408) 19db590 [test] remove unnecessary OP_1s from CSV and CLTV tests (gzhao408) Pull request description: This uses the first 4 commits of bitcoin#15045, rebased and added some comments. The diff is quite large already and I want to make it easy to review, so I'm splitting it into 2 PRs (transaction and script). Script one is WIP, I'll link it when I open it. Interpretation of scripts is dependent on the script verification flags passed in. In tests, we should always apply **maximal** verification flags when checking that a transaction is **valid**; any additional flags should invalidate the transaction. A transaction should not be valid because we forgot to include a flag, and we should apply all flags by default. We should apply **minimal** verification flags when asserting that a transaction is **invalid**; if verification flags are applied, removing any one of them should mean the transaction is valid. New verify flags must be backwards compatible; tests should check backwards compatibility and apply the new flags by default. All `tx_invalid` tests should continue to be invalid with the exact same verify flags. All `tx_valid` tests that don't pass with new flags should _explicitly_ indicate that the flags need to be excluded, and fail otherwise. 1. Flip the meaning of `verifyFlags` in tx_valid.json to mean _excluded_ verification flags instead of included flags. Edit the test data accordingly. 2. Trim unneeded flags from tx_invalid.json. 3. Add check to verify that tx_valid tests have maximal flags and tx_invalid tests have minimal flags. 4. Add checks to verify that flags are soft forks (bitcoin#10699) i.e. adding any flag should only decrease the number of acceptable scripts. Test by adding/removing random flags. ACKs for top commit: achow101: ACK 5786a81 laanwj: ACK 5786a81 Tree-SHA512: 19195d8cf3299e62f47dd3443ae4a95430c5c9d497993a18ab80de9e24b1869787af972774993bf05717784879bc4592fdabaae0fddebd437963d8f3c96d9a73
This change makes
SCRIPT_VERIFY_UPGRADABLE_NOPS
not apply toOP_CHECKLOCKTIMEVERIFY
andOP_CHECKSEQUENCEVERIFY
. This is a no-op asUPGRADABLE_NOPS
is only set for mempool transactions, and those always haveSCRIPT_VERIFY_CHECKLOCKTIMEVERIFY
andSCRIPT_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)
SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS
.SCRIPT_VERIFY_DISCOURAGE_NOP8
, which only applies toOP_NOP8
.SCRIPT_VERIFY_NOP8
which implements the new consensus logic.SCRIPT_VERIFY_DISCOURAGE_NOP8
to the mempool flags.SCRIPT_VERIFY_NOP8
to both the mempool and consensus flags.