Page MenuHomePhabricator

Interpret scripts with CHECKDATASIG opcode always valid.
ClosedPublic

Authored by Mengerian on May 23 2019, 15:52.

Details

Summary

Because CHECKDATASIG uses a previouly invalid opcode number, it does
not appear in any executed branches of scripts prior its introduction in the
Nov 15, 2018 Upgrade. This means that the interpreter can treat it as if
it has always been valid.

Before this change, the SCRIPT_ENABLE_CHECKDATASIG flag had both a permissive
(enabling the opcode) and a restrictive (enforcing siops counting) meaning.
This change simplifies the meaning of the flag to be restrictive only.

Test Plan

make check

Did IBD on mainet and testnet with -checkpoints=0 -assumevalid=0

src/test/checkdatasig_tests.cpp is changed to reflect the new behavior of always allowing op_cds regardless of flag value.

src/test/sigopcount_tests.cpp remains the same to check that the flag still changes sigops counting behavior.

src/test/data/script_tests.json "BAD_OPCODE" test cases removed

Diff Detail

Repository
rABC Bitcoin ABC
Branch
cds-allow-always
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5964
Build 9988: Bitcoin ABC Buildbot (legacy)
Build 9987: arc lint + arc unit

Event Timeline

I am inclined to rename the flag SCRIPT_ENABLE_CHECKDATASIG to something more accurate (since it doesn't enable the opcode, it just enables counting its sigops) but that's debatable...

src/test/data/script_tests.json
2315 ↗(On Diff #8832)

Don't just add this here since it's in the "Automatically generated test cases" section of this file, and will get wiped out next time someone wants to include more auto tests.

Need to add the respective test in script_tests.cpp into the diff.

Then uncomment #define UPDATE_JSON_TESTS at the top of the file temporarily, and get the new script_tests.json and include that in the Diff as well.

Alternatively, we can move CHECKDATASIG tests out of the "Automatically generated test cases" section since they never involve transaction signatures (they are pure self-contained scripts).

This revision now requires changes to proceed.May 23 2019, 16:15
src/test/data/script_tests.json
2315 ↗(On Diff #8832)

(move out of that section and out of script_tests.cpp too, I mean)

I'm also thinking that maybe all the script tests that include the CHECKDATASIG script flag should stop referring to it, except for tests of sigops counting. However if other implementations are relying on those flags being in script_tests.json then maybe they should be kept.

Mengerian planned changes to this revision.EditedMay 23 2019, 17:10

I'm also thinking that maybe all the script tests that include the CHECKDATASIG script flag should stop referring to it, except for tests of sigops counting. However if other implementations are relying on those flags being in script_tests.json then maybe they should be kept.

Yeah, I wasn't sure the best approach here. Since the flag has no effect here, it's odd to include it. On the other hand, you definitely want the opcode to work properly when the flag is enabled, so seems strange to unset the flag for the tests. Maybe all the test should be duplicated, similar to what you did with the Schnorr tests

I am inclined to rename the flag SCRIPT_ENABLE_CHECKDATASIG to something more accurate (since it doesn't enable the opcode, it just enables counting its sigops) but that's debatable...

Yeah, I couldn't think of a better name... Maybe "SCRIPT_VERIFY_CHECKDATASIG_SIGOPS"?
I would also be inclined to do that separately in a subsequent Diff, since that can be a self-contained change.

src/test/data/script_tests.json
2315 ↗(On Diff #8832)

OK, thanks, I will definitely have to change that.

I didn't realize how the test cases were generated.

Mengerian edited the test plan for this revision. (Show Details)

Added duplicate CHECKDATASIG tests with flag off to script_tests.cpp to generate auto test cases.

deadalnix requested changes to this revision.May 23 2019, 23:04
deadalnix added inline comments.
src/test/script_tests.cpp
1542 ↗(On Diff #8835)

This is just bloat. If the flag is irrelevant, which you already ensure via unit tests, then this provides no value.

This revision now requires changes to proceed.May 23 2019, 23:04
Mengerian edited the test plan for this revision. (Show Details)

Remove test bloat.
Make comments in checkdatasig_tests.cpp more precise.
Rebase on master.

Improve comments in checkdatasig_tests.cpp

description nit:

it does not appear in any executed sripts...

to

it does not appear in any executed branches of scripts...

This revision is now accepted and ready to land.May 24 2019, 15:26
Mengerian edited the summary of this revision. (Show Details)

Fix description nit and rebase.