Remove the “OP_” prefix from transaction script decodes. #5392

pull mruddy wants to merge 1 commits into bitcoin:master from mruddy:op_prefix changing 4 files +116 −101

mruddy commented at 10:31 pm on November 28, 2014: contributor
This set of changes was born out of the discussion of #5264.

The main impact of these changes is on the scriptPubKey “asm” value in the output for transaction decodes. There are also more minor logging and bitcoin-tx references.

Basically, the goal was to just remove the “OP_” prefixes.
1. Example P2PKH Before: OP_DUP OP_HASH160 b9c670c7aa955c9808af7eeb9643b9fd6285cbf5 OP_EQUALVERIFY OP_CHECKSIG After: DUP HASH160 b9c670c7aa955c9808af7eeb9643b9fd6285cbf5 EQUALVERIFY CHECKSIG
2. Example P2SH Before: OP_HASH160 2a5edea39971049a540474c6a99edf0aa4074c58 OP_EQUAL After: HASH160 2a5edea39971049a540474c6a99edf0aa4074c58 EQUAL
If these changes gain acceptance, I’ll add some release notes too when that time comes.
Remove the "OP_" prefix from transaction script decodes. 103bd9603b
TheBlueMatt commented at 11:02 pm on November 28, 2014: member

Ehh, I find the OP_ prefix more readable, and though I dont feel strongly, I dont like the idea of changing the rpc outptut just for this.
mruddy commented at 1:28 pm on November 29, 2014: contributor

Thanks for the feedback.

#5264 was a change to decode the scriptSig signature hash types.

The reason for making this set of “OP_” changes at the same time was to break any dependencies on the output pretty clearly. Helping readability by reducing the “OP_” redundancy was a secondary goal.

I think the growing consensus from the #5264 discussion is that no guarantees to hold these outputs constant have been made. As these changes are made, more unit tests are being added which will firm up these output formats and increase support for those that do want to script against them in the future.

So, that’s at least why this set of changes was put forward.
laanwj commented at 9:27 am on December 1, 2014: member

@thebluematt The rationale for this is that as #5264 changes the asm syntax to add (…) comments, which will break current parsers anyhow, a cleanup like removing OP_ can be done at the same time.

So I’m fine with this if it is decided to merge that pull.
laanwj added the label RPC on Dec 2, 2014
TheBlueMatt commented at 9:35 pm on December 7, 2014: member

Yes, doing them at the same time is reasonable, though I still think the OP_ prefix reads better to me…but, really…who cares?
mruddy commented at 4:18 pm on December 8, 2014: contributor

Prefixes or not, the added value is in the new unit tests that firm up the format. I’d be just as happy to remove my changes that remove the “OP_” prefixes if doing that will get the new unit tests merged (and I’ll update them to verify that the “OP_” prefixes are there). Please let me know if I should do that.

This is also about consistency in the “asm” outputs. When I worked pull request #5264 to resolve issue #3166, I originally had the “SIGHASH_” prefixes. The feedback was to remove those prefixes. Then some more feedback came in saying to create this pull request removing the “OP_” prefixes too. @TheBlueMatt could you comment on #5264 about if you like that change or not (#5264 adds SIGHASH decodes to the “asm” outputs)? That would help move that ticket closer to a result either way too. Thanks!
jgarzik commented at 4:21 pm on December 8, 2014: contributor

Typical asm-style languages do not contain a redundant, common prefix. It’s useful for C++ global namespace disambiguation, but the “native” names should not contain OP_
mruddy commented at 10:10 pm on December 8, 2014: contributor

@jgarzik interesting perspective, and little nugget of knowledge, thanks!
mruddy commented at 11:40 am on December 11, 2014: contributor

Withdrew #5264 so I’m closing this too as it’s not important enough on its own.
mruddy closed this on Dec 11, 2014
mruddy deleted the branch on Dec 1, 2015
MarcoFalke locked this on Sep 8, 2021

Contributors
mruddy TheBlueMatt laanwj jgarzik

Labels
RPC/REST/ZMQ