Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove implementation of disabled opcodes #2610

Merged
merged 1 commit into from May 3, 2013

Conversation

gavinandresen
Copy link
Contributor

So we stop getting pull requests (like #2604) fixing problems with disabled Script opcodes.
A hard fork would be required to re-enable these, and if we ever did that we'd require extensive review and testing.

I double-checked to make sure the script_invalid.json unit tests make sure that the disabled opcodes are handled properly (script invalid even if the disabled opcodes are in the unexecuted branch of an OP_IF); they do.

So we stop getting pull requests (like bitcoin#2604) fixing problems with disabled Script opcodes.
A hard fork would be required to re-enable these, and if we ever did that we'd require extensive review and testing.
@luke-jr
Copy link
Member

luke-jr commented May 2, 2013

Any reason not to remove the definitions of OP_disabledops entirely?

@sipa
Copy link
Member

sipa commented May 2, 2013

The only reason to keep them would be backward compatibility for the decoded version, I guess, and then even only for very weird scripts...

@BitcoinPullTester
Copy link

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/bce697d7faef0d8119ba80ae583cfcce460c7f81 for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

@laanwj
Copy link
Member

laanwj commented May 3, 2013

Good. The code is unused and there is no reason to keep it like this. If it is ever decided to bring the opcodes back, it can be done with better designed semantics w.r.t signed/unsigned handling.

@petertodd
Copy link
Contributor

@luke-jr The code that actually disabled those OP's is executed, so I'd be inclined to leave that in place out of paranoia, at least for now.

FWIW, I manually reviewed the change.

@gavinandresen
Copy link
Contributor Author

RE: remove opcodes entirely: I thought about removing the OP_ enum constants, but then we'd just have to use hard-coded 0x constants in a couple of places (the early if statement, and the opcode-to-string map so unit tests continued working) which would be uglier and more error-prone.

@sipa
Copy link
Member

sipa commented May 3, 2013

ACK

gavinandresen added a commit that referenced this pull request May 3, 2013
Remove implementation of disabled opcodes
@gavinandresen gavinandresen merged commit 4a4e9a3 into bitcoin:master May 3, 2013
@gavinandresen gavinandresen deleted the scriptcleanup branch November 4, 2013 06:17
laudney pushed a commit to reddcoin-project/reddcoin-3.10 that referenced this pull request Mar 19, 2014
Remove implementation of disabled opcodes
pyritepirate referenced this pull request in pyritepirate/pyrite Nov 21, 2018
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants