Page MenuHomePhabricator

Remove SCRIPT_VERIFY_CHECKDATASIG_SIGOPS flag from script tests
ClosedPublic

Authored by Mengerian on May 28 2019, 05:22.

Details

Summary

Now that the flag has no effect on the validity of OP_CHECKDATASIG in scripts,
it is unnecessary to pass it into script tests.

The fact that the flag does not affect validity of the opcode is tested in the
checkdatasig_tests.cpp unit tests, and the effect of the flag on sigops counting
is tested in sigopcount_tests.cpp

Test Plan

make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
cds-remove-flag-from-script-tests
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 6040
Build 10139: Bitcoin ABC Buildbot (legacy)
Build 10138: arc lint + arc unit

Event Timeline

Fabien requested changes to this revision.May 28 2019, 11:04
Fabien added inline comments.
src/test/data/script_tests.json
257 ↗(On Diff #8948)

Dropping the flag make these tests getting duplicated, you can remove them.

This revision now requires changes to proceed.May 28 2019, 11:04

Remove duplicate test cases

deadalnix requested changes to this revision.May 28 2019, 15:22
deadalnix added inline comments.
src/test/scriptflags.cpp
34 ↗(On Diff #8963)

Keep

This revision now requires changes to proceed.May 28 2019, 15:22
src/test/scriptflags.cpp
34 ↗(On Diff #8963)

Do you mind if I change the name of the flag in the tests to "CHECKDATASIG_SIGOPS"?

I think that makes the tests more readable. It's mildly confusing (and worse for searching) that the name of the flag is exactly the same as the name of the opcode. (If the flag ever ends up getting used again)

src/test/scriptflags.cpp
34 ↗(On Diff #8963)

I like the name "CHECKDATASIG_SIGOPS" personally.

Keep flag name in scriptflags.cpp list

This revision is now accepted and ready to land.May 28 2019, 16:58