Page MenuHomePhabricator

remove SCRIPT_ENABLE_SCHNORR flag and clean up tests
ClosedPublic

Authored by markblundeberg on Jun 14 2019, 04:25.

Details

Summary

This is a backing-out of D2430, which added an impotent
SCRIPT_ENABLE_SCHNORR flag along with a bunch of duplicate tests. Now that
the flag is once again impotent, we remove it.

In addition to removing those tests, there were numerous other script_tests
cases that are now redundant as well, also removed in this diff.

Depends on D3331

Test Plan

make check

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

markblundeberg retitled this revision from remove Schnorr flag and clean up tests to remove SCRIPT_ENABLE_SCHNORR flag and clean up tests.Jun 14 2019, 04:51
jasonbcox added inline comments.
src/test/data/script_tests.json
1581 ↗(On Diff #9426)

Just an observation: No one noticed the typo (errand space): , " 0x21

src/test/data/script_tests.json
1581 ↗(On Diff #9426)

Heh nice catch, I wonder how that happened ...

deadalnix requested changes to this revision.Jun 25 2019, 23:04

Don't use 0 for flags.

src/test/script_tests.cpp
1446 ↗(On Diff #9426)

Dn't use 0 here, there is a flag none or something.

1692 ↗(On Diff #9426)

Why is this one removed?

This revision now requires changes to proceed.Jun 25 2019, 23:04
src/test/script_tests.cpp
1692 ↗(On Diff #9426)

This is just an ECDSA signature check, originally inserted to make sure ECDSA still works with ENABLE_SCHNORR flag on. Now that the flag is gone I figured we don't need to keep it around. (this is the case for many other removed tests as well)

src/test/script_tests.cpp
1446 ↗(On Diff #9426)

Yeah there is SCRIPT_VERIFY_NONE, however that doesn't get used at all in this script_tests.cpp file (in fact very few places in the codebase use it). If you like I can substitute that for 0 in these tests, alternatively I can do a follow-up where all the 0 are substituted in all the tests.

It all looks good to me.

I think changing the test flag from 0 to NONE should be done in a separate Diff.

src/test/script_tests.cpp
1446 ↗(On Diff #9426)

I noticed this also when I was doing the Segwit Recovery Diffs. Although the NONE flag exists for these tests, it is currently unused.

I agree that this should be changed, but it should be in a separate Diff that changes all instances together IMO.

deadalnix added inline comments.
src/test/script_tests.cpp
1446 ↗(On Diff #9426)

It's fine to do it in another diff, but this needs to be done.

This revision is now accepted and ready to land.Jun 27 2019, 17:50