remove SCRIPT_ENABLE_SCHNORR flag and clean up tests
ClosedPublic
Actions

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

Details

Reviewers

deadalnix
Mengerian

Group Reviewers

Restricted Project

Maniphest Tasks

T653: Clean up past upgrades

Commits

rSTAGING7cfc38d37a8f: remove SCRIPT_ENABLE_SCHNORR flag and clean up tests
rABC7cfc38d37a8f: remove SCRIPT_ENABLE_SCHNORR flag and clean up tests

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 created this revision.Jun 14 2019, 04:25

Herald added a reviewer: Restricted Project. · View Herald TranscriptJun 14 2019, 04:25

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

markblundeberg added a task: T653: Clean up past upgrades.Jun 14 2019, 04:26

markblundeberg mentioned this in D3253: Remove Schnorr activation.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B6334: Diff 9426.Jun 14 2019, 04:34

markblundeberg added a child revision: D3254: Revert "add flags to VerifySignature and sigcache".Jun 14 2019, 04:47

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 a subscriber: jasonbcox.Jun 15 2019, 00:20

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`

markblundeberg added inline comments.Jun 15 2019, 00:34

src/test/data/script_tests.json
1581 ↗	(On Diff #9426)	Heh nice catch, I wonder how that happened ...

markblundeberg mentioned this in D3331: remove effect of SCRIPT_ENABLE_SCHNORR flag.Jun 23 2019, 16:17

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

markblundeberg added inline comments.Jun 25 2019, 23:37

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)

markblundeberg added inline comments.Jun 26 2019, 01:23

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.

markblundeberg requested review of this revision.Jun 26 2019, 01:23

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.