Page MenuHomePhabricator

Add CHECKDATASIG to standard flags.
ClosedPublic

Authored by Mengerian on May 18 2019, 14:54.

Details

Summary

Also remove STANDARD_CHECKDATASIG_VERIFY_FLAGS.

Test Plan

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

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

Event Timeline

Mengerian created this revision.May 18 2019, 14:54
Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 18 2019, 14:54
deadalnix requested changes to this revision.May 18 2019, 16:23

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.

This revision now requires changes to proceed.May 18 2019, 16:23
Mengerian added inline comments.May 19 2019, 03:01
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.

Mengerian updated this revision to Diff 8730.May 19 2019, 04:25

Add CHECKDATASIG to standard flags, not to mandatory flags.

Mengerian retitled this revision from Add CHECKDATASIG to mandatory flags. to Add CHECKDATASIG to standard flags..May 19 2019, 04:27
Mengerian edited the summary of this revision. (Show Details)
deadalnix requested changes to this revision.May 20 2019, 00:11

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.

This revision now requires changes to proceed.May 20 2019, 00:11
Mengerian updated this revision to Diff 8753.May 20 2019, 21:06

Rebased on top of test changes.

Depends on D3063

Mengerian updated this revision to Diff 8755.May 20 2019, 21:15

Fix rebase

@deadalnix I tried to address your comments by changing the test in D3063 and then rebasing this Diff on top of that one.

Mengerian edited the test plan for this revision. (Show Details)May 20 2019, 21:18
Mengerian edited the test plan for this revision. (Show Details)May 20 2019, 22:40
Mengerian updated this revision to Diff 8764.May 20 2019, 23:31

Rebase on D3063 changes.

Mengerian planned changes to this revision.May 21 2019, 00:01
Mengerian updated this revision to Diff 8770.May 21 2019, 01:58

Rebase on non-broken D3063

markblundeberg requested changes to this revision.May 21 2019, 04:31
markblundeberg added a subscriber: markblundeberg.

Given how STANDARD_SCRIPT_VERIFY_FLAGS appears in some important places, please add IBD to the test plan. :-)

This revision now requires changes to proceed.May 21 2019, 04:31
Mengerian updated this revision to Diff 8794.May 22 2019, 03:44
Mengerian edited the test plan for this revision. (Show Details)

Added IBD to test plan

Mengerian edited the summary of this revision. (Show Details)May 22 2019, 04:07
Mengerian edited the summary of this revision. (Show Details)
jasonbcox accepted this revision.May 22 2019, 16:21
markblundeberg accepted this revision.May 22 2019, 19:11

Great to see the end of STANDARD_CHECKDATASIG_VERIFY_FLAGS :-)

deadalnix accepted this revision.May 23 2019, 12:00
deadalnix added inline comments.
src/test/checkdatasig_tests.cpp
258 ↗(On Diff #8794)

No need for double ()

This revision is now accepted and ready to land.May 23 2019, 12:00
Mengerian updated this revision to Diff 8825.May 23 2019, 15:00

Remove redundant brackets and rebase.

This revision was automatically updated to reflect the committed changes.