Page MenuHomePhabricator

Prepare for re-enabled opcodes for the May 2018 protocol upgrade
ClosedPublic

Authored by deadalnix on Feb 28 2018, 01:54.

Details

Summary
  • Rename OP_SUBSTR to OP_SPLIT
  • Add OP_BIN2NUM & OP_NUM2BIN, replacing OP_LEFT and OP_RIGHT
  • tests in script_tests.json were updated to use the renamed opcodes
  • Also added FIRST_UNDEFINED_OP_VALUE, the first value of undefined opcodes - OP_NOP10+1 was being used for this purpose which is confusing

Co-authored-by: Joshua Yabut <yabut.joshua@gmail.com>
Co-authored-by: Marcos Mayorga <mm@mm-studios.com>
Co-authored-by: Daniel Connolly <daniel@dconnolly.com>

Test Plan

make check
test/functional/test_runner.py --extended

Diff Detail

Repository
rABC Bitcoin ABC
Branch
arcpatch-D1141
Lint
Lint Skipped
Unit
No Test Coverage
Build Status
Buildable 2035
Build 2218: Bitcoin ABC Buildbot (legacy)
Build 2217: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
deadalnix requested changes to this revision.Feb 28 2018, 10:28
deadalnix added a subscriber: deadalnix.

There are no tests.

This revision now requires changes to proceed.Feb 28 2018, 10:28

There are no tests.

Test diff is located here: https://reviews.bitcoinabc.org/D1142 I can squash these diffs and submit as a single if that'll help.

This is changing the behavior of consensus critical code. There is a flag that is now affecting the behavior in as it changes error codes returns for some ops. That needs to be tested. Even if the flag did change nothing, then it would still require a test that ensure it does nothing. This part of the code is absolutely to be tested for any change - as should any piece of code, but ESPECIALLY this one.

movrcx retitled this revision from Check for SCRIPT_ENABLE_OPCODES0 to enabled new opcodes to Allow new opcodes to be used after the May 2018 hardfork activates.Mar 5 2018, 21:09
movrcx edited the summary of this revision. (Show Details)

This implementation doesn't reuse previous opcodes

Owners added a reviewer: Restricted Owners Package.Mar 8 2018, 06:20
schancel added inline comments.
src/script/script.h
109 ↗(On Diff #3129)

I know we had said that we should just change the numbers. However, I thought we talked again and this was going to replace OP_SUBSTR?

src/test/data/script_tests.json
243 ↗(On Diff #3129)

This is because the opcodes were NOPs and are now disabled? If so need to stay nops or risk an accidental hardfork pre May 15th.

src/validation.cpp
1898 ↗(On Diff #3129)

Remove

src/test/data/script_tests.json
243 ↗(On Diff #3129)

all opcodes values greater than OP_NOP10 were considered BAD_OPCODE, invalid. OP_NOP10 just happened to be the opcode with highest value. three opcodes were added with values 0xba, 0xbb, 0xbc. So 0xbc was the new highest valid value and 0xbd and above were invalid. the comment (NOP13) is not well worded.

With the code that is coming, only two opcodes are added, with values 0xba and 0xbb (because OP_SPLIT will replace OP_SUBSTR). So this will change to 0xbc and the comment will be more expressive.

The OP_NOPs have not been changed.

danconnolly retitled this revision from Allow new opcodes to be used after the May 2018 hardfork activates to Prepare for re-enabled opcodes for the May 2018 protocol upgrade.Mar 9 2018, 22:43
danconnolly edited the summary of this revision. (Show Details)
danconnolly edited the test plan for this revision. (Show Details)
danconnolly edited the summary of this revision. (Show Details)
danconnolly edited the test plan for this revision. (Show Details)

update revision to minimize changes
summary of revision updated already

reorganization of the stack of changes to support re-enabled opcodes

reorganize the stack of changes to support re-enabled opcodes

src/core_read.cpp
29 ↗(On Diff #3141)

OP_NOP10 was being used here as the maximum value of defined opcodes (not including template matching params). Replaced with a #define with a name that is supposed to represent its purpose. This code is extensively used by script_tests to interpret the tests found in script_tests.json.

src/script/interpreter.cpp
316 ↗(On Diff #3141)

This is not used yet but is logically part of this revision. It will be used in the next revision.

deadalnix requested changes to this revision.Mar 10 2018, 20:25

This is doing 3 things at once.
1/ Change the opcode enums to add the new opcodes. This can be a diff on its own. opcodes values do not match the spec. Test coverage seems apropriate.
2/ Adding a flag to ba able to enable future opcodes when the flag is passed. There are no test involving this flag at this point in time.

src/script/script.h
105 ↗(On Diff #3141)

You can have two enum entries with the same value.

138 ↗(On Diff #3141)

These values do not meet the spec.

190 ↗(On Diff #3141)

Add a fake op after op_nop10 and it'll get the right value automatically. This will become obsolete when the code is modified and won't be updated.

test/functional/test_framework/script.py
51 ↗(On Diff #3141)

???

This revision now requires changes to proceed.Mar 10 2018, 20:25
test/functional/test_framework/script.py
51 ↗(On Diff #3141)

these were inserted automatically by the autopep8 python lint checker

Will move the flag up in the stack.

src/script/script.h
105 ↗(On Diff #3141)

I thought it was clearer to replace it.

The mapping of opcode -> string (in GetOpName() in script.cpp) can only be done for one of them, which also affects the reverse (string -> opcode) because of the way this is implemented in ParseScript() in core_read.cpp. Neither of these functions see 'flags' (to enable them to decide which string mapping to use), enabling that could be a massive change.

138 ↗(On Diff #3141)

yes, will update.

190 ↗(On Diff #3141)

yes ok

This comment was removed by danconnolly.

updates based on comments
remove SCRIPT_ENABLE_OPCODES_MONOLITH flag, move to separate diff
correct value of opcodes NUM2BIN and BIN2NUM, replacing LEFT & RIGHT
change MAX_DEFINED_OP_VALUE into FIRST_UNDEFINED_OP_VALUE

In diff https://reviews.bitcoinabc.org/D1141?id=3141#inline-4397 @deadalnix commented on the definition of OP_SPLIT that "You can have two enum entries with the same value.". The definition of OP_SPLIT in that diff replaced the definition of OP_SUBSTR, removing OP_SUBSTR in the process.

I have not taken action on this comment.

It is possible to leave the enum values OP_SUBSTR (replaced by OP_SPLIT), OP_LEFT (replaced by OP_NUM2BIN), and OP_RIGHT (replaced by OP_BIN2NUM) defined but I think it is cleaner to remove them and I dont quite understand why we should leave the old values defined.

This revision is now accepted and ready to land.Mar 11 2018, 22:52
schancel added inline comments.
test/functional/test_framework/script.py
261 ↗(On Diff #3149)

This thing seems to be completely unused. Is there any good reason to keep it around?

deadalnix edited reviewers, added: movrcx; removed: deadalnix.
This revision was automatically updated to reflect the committed changes.