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 FindAndDelete and disable OP_CODESEPARATOR #1458

Merged
merged 4 commits into from Oct 5, 2016

Conversation

Projects
None yet
7 participants
@str4d
Copy link
Contributor

str4d commented Sep 30, 2016

Closes #1386

daira and others added some commits Sep 26, 2016

@daira

This comment has been minimized.

Copy link
Contributor

daira commented Oct 2, 2016

Ideally we would have tests to check that OP_CODESEPARATOR is a disabled operation, and that it is included in the input to a sighash. However, IMHO that doesn't need to block this PR; please file another ticket to add those tests. utACK.

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Oct 2, 2016

test_bitcoin fails:

test/transaction_tests.cpp(158): error in "tx_valid": [[["0000000000000000000000000000000000000000000000000000000000000100",0,"DUP HASH160 0x14 0x5b6462475454710f3c22f5fdf0b40704c92f25c3 EQUALVERIFY CHECKSIGVERIFY 1 0x47 0x3044022067288ea50aa799543a536ff9306f8e1cba05b9c6b10951175b924f96732555ed022026d7b5265f38d21541519e4a1e55044d5b9e17e15cdbaf29ae3792e99e883e7a01"]],"01000000010001000000000000000000000000000000000000000000000000000000000000000000006a473044022067288ea50aa799543a536ff9306f8e1cba05b9c6b10951175b924f96732555ed022026d7b5265f38d21541519e4a1e55044d5b9e17e15cdbaf29ae3792e99e883e7a012103ba8c8b86dea131c22ab967e6dd99bdae8eff7a1f75a2c35f1f944109e3fe5e22ffffffff010000000000000000015100000000","P2SH"]
test/transaction_tests.cpp(159): error in "tx_valid": Script failed an OP_CHECKSIGVERIFY operation

@ebfull

This comment has been minimized.

Copy link
Contributor

ebfull commented Oct 3, 2016

OP_CODESEPARATOR should be removed or renamed from the enum inside script.h.

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Oct 3, 2016

Also in script_tests, uncomment the #ifdef to regenerate test data for transaction_tests.

@str4d

This comment has been minimized.

Copy link
Contributor Author

str4d commented Oct 3, 2016

@ebfull that doesn't match the existing semantics for disabled opcodes (e.g. OP_LSHIFT). IIRC we decided to disable instead of removing to avoid issues with anything that assumed that opcode was for OP_CODESEPARATOR. But if we do want to reclaim that opcode, then yes we could rename or remove.

@str4d str4d assigned str4d and bitcartel and unassigned str4d Oct 3, 2016

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Oct 4, 2016

@str4d Action items from earlier were: move test from invalid to valid, document why it is being moved and then ping Coinspect.

@str4d

This comment has been minimized.

Copy link
Contributor Author

str4d commented Oct 4, 2016

I've done the first two locally, pushing now

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Oct 4, 2016

@str4d I have sent a message to @SergioDemianLerner regarding the test that failed. Will wait for feedback before merging.

@str4d

This comment has been minimized.

Copy link
Contributor Author

str4d commented Oct 4, 2016

@ebfull does your thumb-up mean you're happy with leaving OP_CODESEPARATOR as disabled?

@str4d

This comment has been minimized.

Copy link
Contributor Author

str4d commented Oct 4, 2016

@ebfull

@str4d yes w.r.t. the comment you left on github just now

@daira daira removed the review needed label Oct 4, 2016

@daira

This comment has been minimized.

Copy link
Contributor

daira commented Oct 5, 2016

ACK a4f2555

@SergioDemianLerner

This comment has been minimized.

Copy link
Contributor

SergioDemianLerner commented Oct 5, 2016

I'm reviewing now. I'll need 30 minutes.

@SergioDemianLerner

This comment has been minimized.

Copy link
Contributor

SergioDemianLerner commented Oct 5, 2016

No problem found.

@str4d

This comment has been minimized.

Copy link
Contributor Author

str4d commented Oct 5, 2016

Excellent! Thank you @SergioDemianLerner. I'm happy to merge this.
@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 5, 2016

📌 Commit 7f01e43 has been approved by str4d

@str4d

This comment has been minimized.

Copy link
Contributor Author

str4d commented Oct 5, 2016

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 5, 2016

💡 This pull request was already approved, no need to approve it again.

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 5, 2016

🙀 a4f2555 is not a valid commit SHA. Please try again with 7f01e43.

@str4d

This comment has been minimized.

Copy link
Contributor Author

str4d commented Oct 5, 2016

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 5, 2016

💡 This pull request was already approved, no need to approve it again.

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 5, 2016

🙀 a4f2555b3536eb854bda16763142dcf2a0cb067a is not a valid commit SHA. Please try again with 7f01e43.

@str4d

This comment has been minimized.

Copy link
Contributor Author

str4d commented Oct 5, 2016

@zkbot force

@str4d

This comment has been minimized.

Copy link
Contributor Author

str4d commented Oct 5, 2016

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 5, 2016

💡 This pull request was already approved, no need to approve it again.

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 5, 2016

🙀 a4f2555b3536eb854bda16763142dcf2a0cb067a is not a valid commit SHA. Please try again with 7f01e43.

@str4d

This comment has been minimized.

Copy link
Contributor Author

str4d commented Oct 5, 2016

@zkbot r-

@str4d

This comment has been minimized.

Copy link
Contributor Author

str4d commented Oct 5, 2016

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 5, 2016

🙀 a4f2555b3536eb854bda16763142dcf2a0cb067a is not a valid commit SHA. Please try again with 7f01e43.

@ageis

This comment has been minimized.

Copy link
Contributor

ageis commented Oct 5, 2016

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 5, 2016

📌 Commit 7f01e43 has been approved by ageis

zkbot pushed a commit that referenced this pull request Oct 5, 2016

zkbot
Auto merge of #1458 - str4d:1386-remove-findanddelete-and-codeseparat…
…or, r=ageis

Remove FindAndDelete and disable OP_CODESEPARATOR

Closes #1386
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 5, 2016

⌛️ Testing commit a4f2555 with merge b4526f4...

@ageis

This comment has been minimized.

Copy link
Contributor

ageis commented Oct 5, 2016

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 5, 2016

💡 This pull request was already approved, no need to approve it again.

  • This pull request is currently being tested. If there's no response from the continuous integration service, you may use retry to trigger a build again.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 5, 2016

📌 Commit a4f2555 has been approved by ageis

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 5, 2016

☀️ Test successful - zcash

@zkbot zkbot merged commit a4f2555 into zcash:master Oct 5, 2016

1 check passed

homu Test successful
Details

@str4d str4d deleted the str4d:1386-remove-findanddelete-and-codeseparator branch Oct 5, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment