Skip to content
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

Merged
merged 3 commits into from
Dec 12, 2017

Conversation

sipa
Copy link
Member

@sipa sipa commented Jun 28, 2017

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.

@TheBlueMatt
Copy link
Contributor

Can you rebase this on top of the now-merged #10192 to remove the extra checks in its test? Concept ACK.

@sipa sipa force-pushed the 20170628_softflags branch from 9027cde to 01013f5 Compare June 30, 2017 23:19
@sipa
Copy link
Member Author

sipa commented Jun 30, 2017

Done.

@TheBlueMatt
Copy link
Contributor

utACK 01013f5 I did check some of the test changes failed appropriately, but wouldnt really call it a tested ACK per se.

Copy link
Contributor

@jl2012 jl2012 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 01013f5

@laanwj
Copy link
Member

laanwj commented Dec 1, 2017

utACK 2851b77, reviewed both test and interpreter changes, they seem clear and contained and could discover no issues with the changed logic.

@laanwj laanwj merged commit 01013f5 into bitcoin:master Dec 12, 2017
laanwj added a commit that referenced this pull request Dec 12, 2017
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
azuchi pushed a commit to chaintope/bitcoinrb that referenced this pull request Jan 12, 2018
jl2012 added a commit to jl2012/bitcoin that referenced this pull request Jan 16, 2019
braydonf added a commit to braydonf/bcoin that referenced this pull request Feb 2, 2019
braydonf added a commit to braydonf/bcoin that referenced this pull request Feb 2, 2019
tuxcanfly pushed a commit to tuxcanfly/bcoin that referenced this pull request Apr 19, 2019
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 4, 2020
…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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 5, 2020
…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
glozow pushed a commit to glozow/bitcoin that referenced this pull request Aug 10, 2020
See bitcoin#10699, i.e. adding a flag should always reduce the
number of acceptable scripts.
glozow pushed a commit to glozow/bitcoin that referenced this pull request Aug 10, 2020
See bitcoin#10699, i.e. adding a flag should always reduce the
number of acceptable scripts.
glozow pushed a commit to glozow/bitcoin that referenced this pull request Aug 10, 2020
See bitcoin#10699, i.e. adding a flag should always reduce the
number of acceptable scripts.
glozow pushed a commit to glozow/bitcoin that referenced this pull request Aug 10, 2020
See bitcoin#10699, i.e. adding a flag should always reduce the
number of acceptable scripts.
glozow pushed a commit to glozow/bitcoin that referenced this pull request Aug 10, 2020
See bitcoin#10699, i.e. adding a flag should always reduce the
number of acceptable scripts.
glozow pushed a commit to glozow/bitcoin that referenced this pull request Aug 10, 2020
See bitcoin#10699, i.e. adding a flag should always reduce the
number of acceptable scripts.
glozow pushed a commit to glozow/bitcoin that referenced this pull request Aug 11, 2020
See bitcoin#10699, i.e. adding a flag should always reduce the
number of acceptable scripts.
glozow pushed a commit to glozow/bitcoin that referenced this pull request Aug 20, 2020
See bitcoin#10699, i.e. adding a flag should always reduce the
number of acceptable scripts.
glozow pushed a commit to glozow/bitcoin that referenced this pull request Dec 7, 2020
See bitcoin#10699, i.e. adding a flag should always reduce the
number of acceptable scripts.
glozow added a commit to glozow/bitcoin that referenced this pull request Jan 26, 2021
See bitcoin#10699, i.e. adding a flag should always reduce the
number of acceptable scripts.

Co-authored-by: Johnson Lau <jl2012@xbt.hk>
glozow added a commit to glozow/bitcoin that referenced this pull request Feb 2, 2021
See bitcoin#10699, i.e. adding a flag should always reduce the
number of acceptable scripts.

Co-authored-by: Johnson Lau <jl2012@xbt.hk>
laanwj added a commit that referenced this pull request Feb 23, 2021
…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
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 23, 2021
…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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants