-
Notifications
You must be signed in to change notification settings - Fork 37.7k
Rename OP_NOP2 to OP_CHECKLOCKTIMEVERIFY #7213
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
Conversation
Concept ACK Could you please squash those commits? Commits should be as small as possible but still atomic. |
784b408
to
d19686d
Compare
It'd be a good idea to note that the name has changed in the release notes, as this has the (small) potential to break some code that depends on what decodescript returns; we probably won't release this until v0.13 |
@@ -226,7 +226,8 @@ def __new__(cls, n): | |||
|
|||
# expansion | |||
OP_NOP1 = CScriptOp(0xb0) | |||
OP_NOP2 = CScriptOp(0xb1) | |||
OP_CHECKLOCKTIMEVERIFY = CScriptOp(0xb1) | |||
OP_NOP2 = OP_CHECKLOCKTIMEVERIFY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure this "compatibility" should be retained. If someone is specifying OP_NOP2, they may actually be expecting OP_NOP2 behaviour. Better to error and let them explicitly change to OP_CHECKLOCKTIMEVERIFY.
@petertodd If they depend on OP_NOP2 being returned, they are likely to be more subtley broken by not updating this... |
Agreed with @luke-jr, the meaning has changed, a break would be the best-case result for anyone depending on this. |
d19686d
to
ccd7ebc
Compare
Updated with release note update and the compatibility define removed. |
ccd7ebc
to
0bf46e0
Compare
concept/once-over utACK 0bf46e0 |
@paveljanik Only if the verbose option is specified, the json output contains an asm script field. Standard hex output is not. |
@mb300sd Right. Thank you. Can you please add "(in verbose mode)"? |
0bf46e0
to
37d271d
Compare
utACK 37d271d |
@luke-jr That's a reasonable idea; let's remove OP_NOP2. Release notes look good, thanks! |
utACK (but the tests are sufficient here) |
tested ACK |
37d271d Rename OP_NOP2 to OP_CHECKLOCKTIMEVERIFY. (mb300sd)
Github-Pull: bitcoin#7213 Rebased-From: 37d271d
Github-Pull: bitcoin#7213 Rebased-From: 37d271d
Github-Pull: bitcoin#7213 Rebased-From: 37d271d
As suggested by petertodd here https://www.reddit.com/r/Bitcoin/comments/3wt1b9/bip65_cltv_just_went_into_full_enforcement/cxywon5
:)