Page MenuHomePhabricator

remove SCRIPT_ENABLE_SCHNORR flag and clean up tests

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



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

rABC Bitcoin ABC
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markblundeberg created this revision.Jun 14 2019, 04:25
Herald added a reviewer: Restricted Project. · View Herald TranscriptJun 14 2019, 04:25
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.
1581 ↗(On Diff #9426)

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

markblundeberg added inline comments.Jun 15 2019, 00:34
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.

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
markblundeberg added inline comments.Jun 25 2019, 23:37
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)

markblundeberg added inline comments.Jun 26 2019, 01:23
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.

markblundeberg requested review of this revision.Jun 26 2019, 01:23
Mengerian accepted this revision as: Mengerian.Jun 26 2019, 19:39

It all looks good to me.

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

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 accepted this revision.Jun 27 2019, 17:50
deadalnix added inline comments.
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