Also remove STANDARD_CHECKDATASIG_VERIFY_FLAGS.
Details
Details
- Reviewers
deadalnix Fabien markblundeberg jasonbcox - Group Reviewers
Restricted Project - Maniphest Tasks
- T653: Clean up past upgrades
- Commits
- rSTAGINGb2d3f35dac10: Add CHECKDATASIG to standard flags.
rABCb2d3f35dac10: Add CHECKDATASIG to standard flags.
make check
Also did make check with test change only, and with code change only,
to check that it fails as expected.
Did IBD on mainnet and testnet with -checkpoints=0 -assumevalid=0
Depends on D3063
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- cds-standard2
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 5914 Build 9888: Bitcoin ABC Buildbot (legacy) Build 9887: arc lint + arc unit
Event Timeline
Comment Actions
That should really be made a standard flag first for a while. If not then a client could get banned during IBD.
| src/test/checkdatasig_tests.cpp | ||
|---|---|---|
| 23 ↗ | (On Diff #8729) | This very clearly doesn't check the same thing anymore. |
| src/test/checkdatasig_tests.cpp | ||
|---|---|---|
| 23 ↗ | (On Diff #8729) | I changed the test to subtract SCRIPT_ENABLE_CHECKDATASIG from flags during the tests, rather than adding it. So it ends up passing the same flags values into the test (assuming SCRIPT_ENABLE_CHECKDATASIG is included in STANDARD_SCRIPT_VERIFY_FLAGS and MANDATORY_SCRIPT_VERIFY_FLAGS). If we want CHECKDATASIG in STANDARD_SCRIPT_VERIFY_FLAGS only, and not in MANDATORY_SCRIPT_VERIFY_FLAGS, then I will have to re-jig the test in some other way. |
Comment Actions
A few changes, but overall, it looks good.
| src/test/checkdatasig_tests.cpp | ||
|---|---|---|
| 22 ↗ | (On Diff #8730) | It's probably better to do & ~SCRIPT_ENABLE_CHECKDATASIG But more appropriately, just leave it standard and do the setting/clearing in the test. |
| 74 ↗ | (On Diff #8730) | This expect the flag to be present, so it sets it. |
| 85 ↗ | (On Diff #8730) | This expects the flag to be absent, which happens to work due to the test cases provided, but this is actually where the problem is. This test should not assume anything particular about the input that it is given - or, if so, it should check for it in a design by contract fashion ( https://en.wikipedia.org/wiki/Design_by_contract ) |
| src/test/sigopcount_tests.cpp | ||
| 35 ↗ | (On Diff #8730) | Keep the datasigflags name. |
Comment Actions
@deadalnix I tried to address your comments by changing the test in D3063 and then rebasing this Diff on top of that one.
Comment Actions
Given how STANDARD_SCRIPT_VERIFY_FLAGS appears in some important places, please add IBD to the test plan. :-)
| src/test/checkdatasig_tests.cpp | ||
|---|---|---|
| 258 ↗ | (On Diff #8794) | No need for double () |