script: add description for the functionality of each opcode #27109
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
|
Concept ACK Probably worth just squashing this down to 1 commit. |
src/script/script.h
Outdated
| OP_14 = 0x5e, | ||
| OP_15 = 0x5f, | ||
| OP_16 = 0x60, | ||
| OP_0 = 0x00, // empty array of bytes |
There was a problem hiding this comment.
Maybe for OP_0, OP_1..OP_16, OP_1NEGATE, elaborate a bit by stating they they push an element onto the stack, and perhaps give the exact byte encoding rather than just the number.
E.g. OP_3 = 0x53, // Push "\x03" onto the stack (which is interpreted as 3 by numerical opcodes)
There was a problem hiding this comment.
Also, perhaps use /**< ... */ or //!< .. so that doxygen will pick the descriptions up.
src/script/script.h
Outdated
| OP_NOTIF = 0x64, // if top stack value false, execute the statement | ||
| OP_VERIF = 0x65, // mark tx invalid even when occuring in an unexecuted OP_IF branch | ||
| OP_VERNOTIF = 0x66, // mark tx invalid even when occuring in an unexecuted OP_IF branch | ||
| OP_ELSE = 0x67, // if neither OP_IF, OP_NOTIF, OP_ELSE not executed, execute the statement |
There was a problem hiding this comment.
I can't really parse what you're trying to say here.
src/script/script.h
Outdated
| OP_ELSE = 0x67, // if neither OP_IF, OP_NOTIF, OP_ELSE not executed, execute the statement | ||
| OP_ENDIF = 0x68, // end if/else block(must include, otherwise tx become invalid) | ||
| OP_VERIFY = 0x69, // mark tx invalid if top stack value false | ||
| OP_RETURN = 0x6a, // mark tx invalid |
There was a problem hiding this comment.
Maybe add whether or not it also applies in unexecuted branches?
src/script/script.h
Outdated
| OP_TUCK = 0x7d, | ||
| OP_TOALTSTACK = 0x6b, // pop an item from the main stack onto the alt stack | ||
| OP_FROMALTSTACK = 0x6c, // pop an item from the alt stack onto the main stack | ||
| OP_2DROP = 0x6d, // remove top and second-top stack item |
There was a problem hiding this comment.
"second-top" sounds a bit strange to me; perhaps "second from the top"?
Alternatively: "remove the two top stack items" ?
src/script/script.h
Outdated
| OP_LEFT = 0x80, | ||
| OP_RIGHT = 0x81, | ||
| OP_SIZE = 0x82, | ||
| OP_CAT = 0x7e, // disabled, fail the script unconditionally |
There was a problem hiding this comment.
Perhaps add whether this also applies in unexecuted branches? What does the unconditionally refer to?
src/script/script.h
Outdated
| OP_SUBSTR = 0x7f, // disabled, fail the script unconditionally | ||
| OP_LEFT = 0x80, // disabled, fail the script unconditionally | ||
| OP_RIGHT = 0x81, // disabled, fail the script unconditionally | ||
| OP_SIZE = 0x82, // push the length of top stack item |
There was a problem hiding this comment.
Perhaps clarify that this does not pop the top element whose size is inspected.
|
Grateful for kind feedbacks, @sipa |
b9a7e17 to
4e8f9cb
Compare
|
Concept ACK |
1 similar comment
|
Concept ACK |
src/script/script.h
Outdated
| OP_14 = 0x5e, | ||
| OP_15 = 0x5f, | ||
| OP_16 = 0x60, | ||
| OP_0 = 0x00, // push "\x" onto the stack (which is an empty array of bytes) |
There was a problem hiding this comment.
I'd just say "" or the empty array. "\x" isn't any valid syntax I know.
src/script/script.h
Outdated
| OP_PUSHDATA1 = 0x4c, // read the next byte as N and push the next N bytes as an array onto the stack | ||
| OP_PUSHDATA2 = 0x4d, // read the next 2 bytes as N and push the next N bytes as an array onto the stack | ||
| OP_PUSHDATA4 = 0x4e, // // read the next 4 bytes as N and push the next N bytes as an array onto the stack | ||
| OP_1NEGATE = 0x4f, // push "\x-1" onto the stack (which is interpreted as -1 by numerical opcodes) |
There was a problem hiding this comment.
It pushes "\x81" (the byte array consisting of a single byte with value 129).
08c770f to
49bdfa5
Compare
|
applying clang-formatting: |
src/script/script.h
Outdated
| OP_RETURN = 0x6a, | ||
| OP_NOP = 0x61, // do nothing | ||
| OP_VER = 0x62, // mark tx invalid unless occuring in an unexecuted OP_IF branch | ||
| OP_IF = 0x63, // if top stack value true, execute the statement |
There was a problem hiding this comment.
We should probably be consistent in describing how Script deals with true/false, since it's later described as if top stack value != 0. Maybe add a section along the lines of:
/**
* Opcodes that take a true/false value will evaluate the following as false:
* an empty vector
* a vector (of any length) of all zero bytes
* a single byte of "\x80" ('negative zero')
* a vector (of any length) of all zero bytes except the last byte is "\x80"
*
* Any other value will evaluate to true.
* /
I'd also say if the top stack value is true,, and indicate that it consumes the stack item.
49bdfa5 to
d6ab4d5
Compare
|
clang-format applied and description for evaluating false added! |
ed98834 to
16178d0
Compare
No strong opinion, but this purpose is already served by this detailed wiki article presenting how each opcode works along with a bit of history. Using a "opcode, input, output, description" table it is even probably better at explaining what each opcode does than we can ever get in code comments. |
|
Detailed wiki does help dev! However, it's just a complementary material.
These are my personal opinion, and any opinion is welcome! |
e695717 to
4087c15
Compare
Agree with this. So many single line comments look weird. If wiki has some outdated or incorrect information, maybe that can be updated and use its link in the comment above all opcodes. |
|
I followed the way miniscript.h comments. There are so many single lines as well, and look fine & helpful in my opinion. |
5463718 to
b20eacd
Compare
|
rebase done! |
|
This PR has two stale acks and seems like a safe documentation change. I think it would be good to merge if original reviewers want to reack or anyone else wants to review. |
| * a vector (of any length) of all zero bytes except the last byte is "\x80" | ||
| * | ||
| * Any other value will evaluate to true. | ||
| */ |
There was a problem hiding this comment.
In the text below it seems somewhat arbitrary whether opcodes have an effect when appearing in unexecuted branches. I suggest instead adding something like this up front:
* Unless specified otherwise, all opcodes below only have an effect when they occur in executed branches.
src/script/script.h
Outdated
| OP_PUSHDATA2 = 0x4d, // read the next 2 bytes as N and push the next N bytes as an array onto the stack | ||
| OP_PUSHDATA4 = 0x4e, // read the next 4 bytes as N and push the next N bytes as an array onto the stack | ||
| OP_1NEGATE = 0x4f, // push "\x81" onto the stack (which is interpreted as -1 by numerical opcodes) | ||
| OP_RESERVED = 0x50, // mark tx invalid unless occurring in an unexecuted OP_IF branch |
There was a problem hiding this comment.
Suggestion: "mark transaction invalid."
src/script/script.h
Outdated
| OP_VERIFY = 0x69, | ||
| OP_RETURN = 0x6a, | ||
| OP_NOP = 0x61, // do nothing | ||
| OP_VER = 0x62, // mark tx invalid unless occurring in an unexecuted OP_IF branch |
There was a problem hiding this comment.
Suggestion : "mark transaction invalid"
src/script/script.h
Outdated
| OP_RETURN = 0x6a, | ||
| OP_NOP = 0x61, // do nothing | ||
| OP_VER = 0x62, // mark tx invalid unless occurring in an unexecuted OP_IF branch | ||
| OP_IF = 0x63, // if top stack value is true (1 for tapscript), execute the statement |
There was a problem hiding this comment.
More precise: (exactly "\x01" for tapscript)
src/script/script.h
Outdated
| OP_ELSE = 0x67, // if the preceding OP_IF, OP_NOTIF or OP_ELSE not executed, execute the statement | ||
| OP_ENDIF = 0x68, // end if/else block (must include, otherwise tx becomes invalid) | ||
| OP_VERIFY = 0x69, // mark tx invalid if top stack value is false, unless occurring in an unexecuted OP_IF branch | ||
| OP_RETURN = 0x6a, // mark tx invalid unless occurring in an unexecuted OP_IF branch |
src/script/script.h
Outdated
| OP_2MUL = 0x8d, // disabled, fail the script unconditionally (apply in an unexecuted conditional branch) | ||
| OP_2DIV = 0x8e, // disabled, fail the script unconditionally (apply in an unexecuted conditional branch) | ||
| OP_NEGATE = 0x8f, // multiply the top stack item by -1 | ||
| OP_ABS = 0x90, // absolute the top stack item value |
There was a problem hiding this comment.
"replace top stack item by its absolute value"
src/script/script.h
Outdated
| OP_2DIV = 0x8e, // disabled, fail the script unconditionally (apply in an unexecuted conditional branch) | ||
| OP_NEGATE = 0x8f, // multiply the top stack item by -1 | ||
| OP_ABS = 0x90, // absolute the top stack item value | ||
| OP_NOT = 0x91, // convert 0 to 1 and else to 0 |
There was a problem hiding this comment.
convert 0 to "\x01" and everything else to ""
src/script/script.h
Outdated
| OP_NEGATE = 0x8f, // multiply the top stack item by -1 | ||
| OP_ABS = 0x90, // absolute the top stack item value | ||
| OP_NOT = 0x91, // convert 0 to 1 and else to 0 | ||
| OP_0NOTEQUAL = 0x92, // return 0 if the input is 0, otherwise 1 |
There was a problem hiding this comment.
"return" is imprecise I think. How about Replace top stack item by "" if its value is 0, otherwise by "\x01"?
src/script/script.h
Outdated
| OP_LSHIFT = 0x98, // disabled, fail the script unconditionally (apply in an unexecuted conditional branch) | ||
| OP_RSHIFT = 0x99, // disabled, fail the script unconditionally (apply in an unexecuted conditional branch) | ||
|
|
||
| OP_BOOLAND = 0x9a, // return 1 if both top two stack items are not 0, otherwise 0 |
There was a problem hiding this comment.
Pop two top stack items and push "\x01" if they are both true, "" otherwise (and similar things below).
src/script/script.h
Outdated
| OP_CHECKSIGVERIFY = 0xad, | ||
| OP_CHECKMULTISIG = 0xae, | ||
| OP_CHECKMULTISIGVERIFY = 0xaf, | ||
| OP_RIPEMD160 = 0xa6, // hash input using RIPEMD-160 |
There was a problem hiding this comment.
What "input"? I suggest Replace top stack item by its RIPEMD-160 hash.
0e4ae7e to
4db63a5
Compare
|
Thx! I updated |
There was a problem hiding this comment.
The more I look at this (and especially after the switch to notation), the more I think the descriptions should each be on a separate line above the opcode in a block comment (/** ... */). It would allow for longer descriptions where needed without ever exceeding the soft 100-per-line character limit.
The block comments could now include (where appropriate):
- Both a textual description and notation (frankly, I find the notation-only comments quite difficult to read).
- Historical context for an opcode (when was it enabled/disabled).
- Differences between tapscript and pre-tapscript.
Additionally, block comments above declarations are shown by many IDEs when hovering above a symbol, providing immediate access to documentation. Inline comments don't do this.
I've drafted a few examples for these longer descriptions:
/**
* Mark transaction as invalid if the top stack item is greater than
* the transaction's locktime. The top stack item is not removed.
*
* Previously OP_NOP2. Specified in BIP65 and deployed in December 2015
* at block 388380 as a soft fork.
*/
OP_CHECKLOCKTIMEVERIFY = 0xb1,
/**
* See OP_CHECKLOCKTIMEVERIFY.
*/
OP_NOP2 = OP_CHECKLOCKTIMEVERIFY,
/**
* Mark transaction as invalid even when occurring in an unexecuted branch.
*
* Previously concatenated two byte arrays. Disabled in August 2010.
*
* Turned into OP_SUCCESS126 in tapscript.
*/
OP_CAT = 0x7e,
/**
* Duplicate the top three stack items:
*
* [ ... x0 x1 x2 ] -> [ ... x0 x1 x2 x0 x1 x2 ]
*/
OP_3DUP = 0x6f,I understand this will make the section longer by a good few hundred lines, but if Bitcoin Core is to maintain in perpetuity a documentation for all opcodes, it should be at least of similar quality to the one at Bitcoin Wiki, and that is not achievable with short single-line descriptions.
|
@vostrnad's comment makes sense to me. I'll hold off re-reviewing until that's either done or decided against. |
src/script/script.h
Outdated
| OP_0NOTEQUAL = 0x92, // replace top stack item by "" if its value is 0, otherwise by "\x01" | ||
|
|
||
| OP_ADD = 0x93, // pop two top stack items and push their sum | ||
| OP_SUB = 0x94, // pop two top stack items and push the second minus the top |
There was a problem hiding this comment.
pop two doesn't quite work (IMO).
it makes the order somewhat ambiguous.
for example:
pop pop add push is more correct. no?
Being more explicit in this way will make the OP_SUB make more sense, no?.
pop pop subtract push
for OP_ADD it isn't much of an issue.
1 + 1 = 2
1 + -1 = 0
-1 + 1 = 0
for OP_SUB it may be an issue.
1 - 1 = 0
1 - -1 = 0
-1 - 1 = -2
*in common arithmetic notation
There was a problem hiding this comment.
I understand what you intended but it does not seem to be interpreted in that way for pop two top stack items. pop pop add push is more confusing to me. I appreciate your feedback
src/script/script.h
Outdated
| OP_LSHIFT = 0x98, // mark transaction invalid even when occurring in an unexecuted branch | ||
| OP_RSHIFT = 0x99, // mark transaction invalid even when occurring in an unexecuted branch | ||
|
|
||
| OP_BOOLAND = 0x9a, // pop two top stack items and push "\x01" if they are both true, "" otherwise |
There was a problem hiding this comment.
see above.
better boolean notation here would be great.
|
Concept ACK |
a4af3b5 to
cfb3bf1
Compare
cfb3bf1 to
fd0db18
Compare
|
Seems no one's against @vostrnad suggestion, I convert all single lines to block comments, add textual description with notation and reference for OP_SUCCESS opcodes in case of tapscript. |
Indeed. |
fd0db18 to
0bf5534
Compare
|
Understood. Thanks for everyone who put efforts into here tgh🙏 |
|
For those who might look for this, I just created repo for it here. |
|
For anyone interested in documenting opcodes, there is now https://opcodeexplained.com/ (GitHub repo here). |
While miniscript has a very kind explanation for it, original script does not.
I add a simple description for the functionality of each opcode.
It will help a lot for developers who want to do script programming.