Page MenuHomePhabricator
Differential D1141

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

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

Details

Reviewers
schancel
movrcx
Group Reviewers
Restricted Project
Restricted Owners Package(Owns No Changed Paths)
Maniphest Tasks
T303: Setup negative-activation unit tests for hardfork
Commits
rSTAGINGce813ee28ee8: Prepare for re-enabled opcodes for the May 2018 protocol upgrade
rABCce813ee28ee8: Prepare for re-enabled opcodes for the May 2018 protocol upgrade
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
clean-opcodes
Lint
Lint Skipped
Unit
No Test Coverage
Build Status
Buildable 2029
Build 2206: Bitcoin ABC Buildbot (legacy)
Build 2205: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
movrcx added a parent revision: D1139: Enable May 15 2018 HF Activation.Feb 28 2018, 01:55
movrcx added a child revision: D1142: Added tests for disabled opcodes before/after the fork..Feb 28 2018, 01:57
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
movrcx added a comment.Feb 28 2018, 16:52
In D1141#21176, @deadalnix wrote:

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.

deadalnix added a comment.Mar 1 2018, 08:37

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 updated this revision to Diff 3083.Mar 5 2018, 20:29

Add unit tests

Harbormaster completed remote builds in B2003: Diff 3083.Mar 5 2018, 20:29
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)
danconnolly added a subscriber: danconnolly.Mar 6 2018, 10:01
movrcx updated this revision to Diff 3129.Mar 8 2018, 06:20

This implementation doesn't reuse previous opcodes

Owners added a reviewer: Restricted Owners Package.Mar 8 2018, 06:20
teamcity added a subscriber: teamcity.Mar 8 2018, 06:22

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

teamcity added a comment.Mar 8 2018, 07:05

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging failed.

Harbormaster failed remote builds in B2029: Diff 3129!Mar 8 2018, 07:05
teamcity added a comment.Mar 8 2018, 22:03

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

Harbormaster completed remote builds in B2029: Diff 3129.Mar 8 2018, 22:48
teamcity added a comment.Mar 8 2018, 22:48

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging passed.

schancel added a subscriber: schancel.Mar 9 2018, 02:20
schancel added inline comments.
src/script/script.h
109

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

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.

schancel edited parent revisions, added: D1149: Activate 32MB blocks after May 15, 2018; removed: D1139: Enable May 15 2018 HF Activation.Mar 9 2018, 10:47
deadalnix added inline comments.Mar 9 2018, 12:15
src/validation.cpp
1898

Remove

danconnolly mentioned this in D1142: Added tests for disabled opcodes before/after the fork..Mar 9 2018, 15:06
danconnolly added a child revision: D1090: Adds Bitwise functionality into interpreter.cpp.
danconnolly added inline comments.Mar 9 2018, 15:35
src/test/data/script_tests.json
243

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 updated this revision to Diff 3138.Mar 9 2018, 22:49
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

teamcity added a comment.Mar 9 2018, 22:50

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

teamcity added a comment.Mar 9 2018, 23:13

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging passed.

Harbormaster completed remote builds in B2035: Diff 3138.Mar 9 2018, 23:14
danconnolly updated this revision to Diff 3140.Mar 10 2018, 09:29

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

teamcity added a comment.Mar 10 2018, 09:31

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

danconnolly updated this revision to Diff 3141.Mar 10 2018, 09:32

reorganize the stack of changes to support re-enabled opcodes

teamcity added a comment.Mar 10 2018, 09:34

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

danconnolly added inline comments.Mar 10 2018, 09:40
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.

teamcity added a comment.Mar 10 2018, 09:54

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging passed.

Harbormaster completed remote builds in B2037: Diff 3140.Mar 10 2018, 09:55
Harbormaster completed remote builds in B2038: Diff 3141.Mar 10 2018, 09:58
teamcity added a comment.Mar 10 2018, 09:58

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging passed.

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
danconnolly added inline comments.Mar 10 2018, 23:24
test/functional/test_framework/script.py
51 ↗(On Diff #3141)

these were inserted automatically by the autopep8 python lint checker

danconnolly added a comment.Mar 11 2018, 00:44

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

danconnolly updated this revision to Diff 3148.Mar 11 2018, 18:30
This comment was removed by danconnolly.
teamcity added a comment.Mar 11 2018, 18:30

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

danconnolly updated this revision to Diff 3149.Mar 11 2018, 18:33

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

teamcity added a comment.Mar 11 2018, 18:36

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

danconnolly edited the summary of this revision. (Show Details)Mar 11 2018, 18:36
danconnolly added a comment.EditedMar 11 2018, 18:42

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.

teamcity added a comment.Mar 11 2018, 18:52

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging passed.

Harbormaster completed remote builds in B2044: Diff 3148.Mar 11 2018, 18:52
teamcity added a comment.Mar 11 2018, 18:59

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging passed.

Harbormaster completed remote builds in B2045: Diff 3149.Mar 11 2018, 18:59
deadalnix accepted this revision.Mar 11 2018, 22:52
This revision is now accepted and ready to land.Mar 11 2018, 22:52
schancel accepted this revision.Mar 12 2018, 02:03
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?

danconnolly edited the summary of this revision. (Show Details)Mar 12 2018, 09:45
schancel removed a child revision: D1142: Added tests for disabled opcodes before/after the fork..Mar 13 2018, 14:46
schancel mentioned this in T303: Setup negative-activation unit tests for hardfork.Mar 15 2018, 14:16
schancel added a task: T303: Setup negative-activation unit tests for hardfork.Mar 15 2018, 14:44
deadalnix commandeered this revision.Mar 15 2018, 21:53
deadalnix edited reviewers, added: movrcx; removed: deadalnix.
deadalnix updated this revision to Diff 3202.Mar 15 2018, 21:53

Remove empty lines.

Closed by commit rABCce813ee28ee8: Prepare for re-enabled opcodes for the May 2018 protocol upgrade (authored by danconnolly, committed by deadalnix). · Explain WhyMar 15 2018, 21:57
This revision was automatically updated to reflect the committed changes.
teamcity added a comment.Mar 15 2018, 22:55

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

teamcity added a comment.Mar 15 2018, 23:15

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging passed.

Harbormaster completed remote builds in B2088: Diff 3202.Mar 15 2018, 23:15

Revision Contents
Changeset List

PathSize
src/
script/
5 lines
31 lines
3 lines
6 lines
test/
data/
11 lines
5 lines

Diff 3129

View Options

src/script/interpreter.h

Loading...
View Options

src/script/interpreter.cpp

Loading...
View Options

src/script/script.h

Loading...
View Options

src/script/script.cpp

Loading...
View Options

src/test/data/script_tests.json

Loading...
View Options

src/validation.cpp

Loading...