Skip to content

Conversation

mb300sd
Copy link
Contributor

@mb300sd mb300sd commented Dec 14, 2015

@maflcko
Copy link
Member

maflcko commented Dec 14, 2015

Concept ACK

Could you please squash those commits? Commits should be as small as possible but still atomic.

@petertodd
Copy link
Contributor

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
Copy link
Member

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.

@luke-jr
Copy link
Member

luke-jr commented Dec 14, 2015

@petertodd If they depend on OP_NOP2 being returned, they are likely to be more subtley broken by not updating this...

@dcousens
Copy link
Contributor

Agreed with @luke-jr, the meaning has changed, a break would be the best-case result for anyone depending on this.

@mb300sd
Copy link
Contributor Author

mb300sd commented Dec 14, 2015

Updated with release note update and the compatibility define removed.

@dcousens
Copy link
Contributor

concept/once-over utACK 0bf46e0

@mb300sd
Copy link
Contributor Author

mb300sd commented Dec 15, 2015

@paveljanik Only if the verbose option is specified, the json output contains an asm script field. Standard hex output is not.

@paveljanik
Copy link
Contributor

@mb300sd Right. Thank you. Can you please add "(in verbose mode)"?

@maflcko
Copy link
Member

maflcko commented Dec 17, 2015

utACK 37d271d

@petertodd
Copy link
Contributor

@luke-jr That's a reasonable idea; let's remove OP_NOP2.

Release notes look good, thanks!

@laanwj
Copy link
Member

laanwj commented Dec 22, 2015

utACK (but the tests are sufficient here)
37d271d

@maaku
Copy link
Contributor

maaku commented Dec 22, 2015

tested ACK

@laanwj laanwj merged commit 37d271d into bitcoin:master Dec 22, 2015
laanwj added a commit that referenced this pull request Dec 22, 2015
37d271d Rename OP_NOP2 to OP_CHECKLOCKTIMEVERIFY. (mb300sd)
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Dec 28, 2015
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Dec 28, 2015
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Jan 13, 2016
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants