Details

Reviewers

deadalnix
markblundeberg
jasonbcox
Fabien

Group Reviewers

Restricted Project

Maniphest Tasks

T653: Clean up past upgrades

Commits

rSTAGINGf5fac93cec01: Interpret scripts with CHECKDATASIG opcode always valid.
rABCf5fac93cec01: Interpret scripts with CHECKDATASIG opcode always valid.

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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

Mengerian created this revision.May 23 2019, 15:52

Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 23 2019, 15:52

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

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

Harbormaster completed remote builds in B5956: Diff 8832.May 23 2019, 16:07

Mengerian added a task: T653: Clean up past upgrades.May 23 2019, 16:07

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

markblundeberg added inline comments.May 23 2019, 16:16

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.

In D3100#73242, @markblundeberg wrote:

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

In D3100#73237, @markblundeberg wrote:

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.