Page MenuHomePhabricator

Add SCRIPT_ENABLE_MAGNETIC_OPCODES flag to gate opcodes activation for the Magnetic upgrade.
AbandonedPublic

Authored by danconnolly on Jul 25 2018, 10:17.

Details

Reviewers
deadalnix
jasonbcox
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Summary

This change introduces a new script verification flag, SCRIPT_ENABLE_MAGNETIC_OPCODES, to control the validity of the opcodes that will be introduced or re-enabled in the Magnetic upgrade (scheduled for November 15, 2018).

When this flag is set, the opcodes will be valid, when it is clear the opcodes are not valid. New opcodes, such as OP_CHECKDATASIG, will not exist if the flag is clear. Re-enabled opcodes, such as OP_MUL, will remain disabled if the flag is clear.

The current list of opcodes that will probably be covered by the Magnetic upgrade include: OP_CHECKDATASIG, OP_CHECKDATASIGVERIFY, OP_MUL, OP_INVERT, OP_LSHIFT, OP_RSHIFT.

The SCRIPT_ENABLE_MAGNETIC_OPCODES flag replaces the previous SCRIPT_ENABLE_CHECKDATASIG flag that was specific to the CHECKDATASIG operations. We suggest that the STANDARD_CHECKDATASIG_VERIFY_FLAGS flag, defined in src/policy/policy.h, is renamed to STANDARD_MAGNETIC_VERIFY_FLAGS or similar, but this is out of scope for this change and should be implemented in conjunction with the other changes that are taking place for the CHECKDATASIG opcodes.

This change includes the definition of the "MAGNETIC_OPCODES" flag name for use in script_tests.json and also adds tests to script_tests.json which verify that the targetted opcodes remain disabled when the flag is set.

The setting of the SCRIPT_ENABLE_MAGNETIC_OPCODES flag to the correct value dependent on block height, or whatever other mechanism, is out of scope of this change.

The change is structured to set the scene for future changes that will implement the targeted opcodes.

Test Plan

make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
opcodes_1
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 3043
Build 4178: Bitcoin ABC Teamcity Staging
Build 4177: arc lint + arc unit
danconnolly created this revision.Jul 25 2018, 10:17
Owners added a reviewer: Restricted Owners Package.Jul 25 2018, 10:17
Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 25 2018, 10:17
danconnolly updated this revision to Diff 4397.Jul 25 2018, 12:07

re-organize the json tests into their own groups

danconnolly updated this revision to Diff 4398.Jul 25 2018, 12:10

Add a flag to gate opcodes activation for the magnetic upgrade & organize json tests

danconnolly updated this revision to Diff 4399.Jul 25 2018, 12:11

Add a flag to gate opcodes activation for the magnetic upgrade & re-organize json tests

deadalnix requested changes to this revision.Jul 25 2018, 14:42
deadalnix added a subscriber: deadalnix.

There already is a flag for OP_CHCKDATASIG. It can be renamed instead of adding a newer flag.

This revision now requires changes to proceed.Jul 25 2018, 14:42
danconnolly updated this revision to Diff 4425.Jul 25 2018, 21:21
  • merge CHECKDATASIG and MAGNETIC_OPCODES flags
danconnolly updated this revision to Diff 4426.Jul 25 2018, 21:22
  • merge CHECKDATASIG and MAGNETIC_OPCODES flags
Harbormaster failed remote builds in B2960: Diff 4426!
deadalnix requested changes to this revision.Jul 26 2018, 00:03

Please rebase, there are more uses of this flag now.

This revision now requires changes to proceed.Jul 26 2018, 00:03
danconnolly updated this revision to Diff 4434.EditedJul 26 2018, 07:04

rebased and updated

deadalnix requested changes to this revision.Jul 26 2018, 10:23

Not sure renaming the flag set is in the scope of this diff, but the comment clearly is. Daniel, you are a good developer, so I expect better from you than having to do ssh over code review in order to get this patch done.

src/policy/policy.h
95 ↗(On Diff #4434)

This comment is now invalid.

96 ↗(On Diff #4434)

This name is now non descriptive of what it is.

This revision now requires changes to proceed.Jul 26 2018, 10:23

Yes, that was careless of me, sorry.

I think the renaming of STANDARD_CHECKDATASIG_VERIFY_FLAGS should probably be in a separate diff and coordinated in the various other diffs that are taking place for CHECKDATASIG.

I'll update the comment.

danconnolly updated this revision to Diff 4458.Jul 26 2018, 22:31
  • split up tests into their own sections, they will be expanded later
  • merge CHECKDATASIG and MAGNETIC_OPCODES flags
  • rebase & correct extra CHECKDATASIG flag
  • update comment to reflect change in flag
danconnolly retitled this revision from Add a flag to gate opcodes activation for the magnetic upgrade to Add SCRIPT_ENABLE_MAGNETIC_OPCODES flag to gate opcodes activation for the Magnetic upgrade..Jul 28 2018, 10:27
danconnolly edited the summary of this revision. (Show Details)
jasonbcox requested changes to this revision.Aug 6 2018, 15:59
jasonbcox added a subscriber: jasonbcox.

Needs rebase against SCRIPT_ENABLE_CHECKDATASIG

This revision now requires changes to proceed.Aug 6 2018, 15:59
danconnolly updated this revision to Diff 4527.Aug 8 2018, 17:57
danconnolly edited the summary of this revision. (Show Details)
  • split up tests into their own sections, they will be expanded later
  • merge CHECKDATASIG and MAGNETIC_OPCODES flags
  • update comment to reflect change in flag
danconnolly planned changes to this revision.Aug 8 2018, 18:00

need to remove the extra tests added for OP_2MUL & OP_2DIV

danconnolly updated this revision to Diff 4528.Aug 8 2018, 18:42
  • remove OP_2MUL & OP_2DIV tests that were added for magnetic, they are no longer being re-enabled
danconnolly edited the summary of this revision. (Show Details)Aug 8 2018, 18:44
danconnolly updated this revision to Diff 4536.EditedAug 8 2018, 21:40

another rebase

danconnolly abandoned this revision.Tue, Sep 4, 09:29