script: throw disabled err for op

ChrisCho-H commented at 10:30 am on July 27, 2023: none

Throw DISABLED_OPCODE error for op_ver and its variants as it’s known but just disabled opcodes. Currently, interpreter throws BAD_OPCODE which contains the message of "Opcode missing or not understood", which isn’t accurate and confusing.

DrahtBot commented at 10:30 am on July 27, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	achow101
Stale ACK	jonatack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Consensus on Jul 27, 2023

DrahtBot added the label CI failed on Jul 27, 2023

ChrisCho-H force-pushed on Jul 28, 2023

DrahtBot removed the label CI failed on Jul 28, 2023

achow101 commented at 11:15 pm on July 28, 2023: member

Concept ACK

I agree that BAD_OPCODE is the wrong error for these opcodes. However, instead of adding a new opcode, I think these should just return the existing DISABLED_OPCODE error that is used for the other disabled opcodes.

Also OP_VER should be included here as well as that’s really what is disabled. OP_VERIF and OP_VERNOTIF are really just variants of OP_VER.

Edit: Oh, OP_VER is not an unconditional failure. Even so, it should still have the DISABLED_OPCODE error.

bitcoin deleted a comment on Jul 29, 2023

ChrisCho-H commented at 2:13 am on July 31, 2023: none

Thx! I was also considering it but not 100% sure whether OP_VERIF and OP_VERNOTIF are also disabled. I will use the existing one as your suggestion.

ChrisCho-H force-pushed on Jul 31, 2023

DrahtBot added the label CI failed on Jul 31, 2023

ChrisCho-H force-pushed on Jul 31, 2023

ChrisCho-H renamed this:
~~script: check op_verif and op_vernotif~~
script: throw disabled err for op_ver and its variants
on Jul 31, 2023

DrahtBot removed the label CI failed on Jul 31, 2023

ChrisCho-H commented at 6:59 am on July 31, 2023: none

updated and passed the tests

jonatack commented at 9:27 pm on July 31, 2023: contributor

Approach ACK ffc32938aa962f7c4b8d5f4af69bd710c174ef44

The unit script tests (./src/test/test_bitcoin -t script_tests) don’t pass at the first commit, so it may be best to combine the second commit updating the tests into the first commit with the code changes.

ChrisCho-H force-pushed on Jul 31, 2023

ChrisCho-H commented at 10:22 pm on July 31, 2023: none

squashed into single commit ceab6bf3ac6b09ae7f210c05614d2c898ef7ee37

jonatack commented at 10:29 pm on July 31, 2023: contributor

ACK ceab6bf3ac6b09ae7f210c05614d2c898ef7ee37

It looks like the first sentence of the PR description needs to be updated.
Would it be good to accompany this change with a release note?

ChrisCho-H commented at 10:44 pm on July 31, 2023: none

updated PR description

in src/script/interpreter.cpp:472 in ceab6bf3ac outdated

466@@ -467,7 +467,9 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
467                 opcode == OP_DIV ||
468                 opcode == OP_MOD ||
469                 opcode == OP_LSHIFT ||
470-                opcode == OP_RSHIFT)
471+                opcode == OP_RSHIFT ||
472+                opcode == OP_VERIF ||
473+                opcode == OP_VERNOTIF)

achow101 commented at 8:34 pm on August 1, 2023:

OP_VER can be included here too:

0                opcode == OP_VERNOTIF ||
1                (fExec && opcode == OP_VER))

ChrisCho-H commented at 2:49 am on August 2, 2023:

cool, I’ve implemented!

ChrisCho-H force-pushed on Aug 2, 2023

jonatack commented at 9:12 pm on August 2, 2023: contributor

ACK a4dcdfde2e3d831a562d996fb2c21aa549764c44

in src/script/interpreter.cpp:473 in a4dcdfde2e outdated

466@@ -467,7 +467,10 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
467                 opcode == OP_DIV ||
468                 opcode == OP_MOD ||
469                 opcode == OP_LSHIFT ||
470-                opcode == OP_RSHIFT)
471+                opcode == OP_RSHIFT ||
472+                opcode == OP_VERIF ||
473+                opcode == OP_VERNOTIF ||
474+                (fExec && opcode == OP_VER))

luke-jr commented at 2:55 pm on August 5, 2023:

How does OP_VER differ from a non-existent opcode?

achow101 commented at 3:49 pm on August 5, 2023:

It’s actually defined and exists in documentation.

ajtowns commented at 8:36 am on August 9, 2023:

I think a more precise distinction is that OP_VER, OP_VERIF and OP_VERNOTIF all were defined and implemented in published versions of bitcoin (like CAT/SUBSTR/etc), while OP_RESERVED, OP_RESERVED1, OP_RESERVERD2, OP_INVALIDOPCODE, OP_PUBKEY, OP_PUBKEYHASH (etc) all have had assigned names, but never had a published implementation in EvalScript (not to mention opcodes 0xbb through 0xf8 or so which didn’t even get given names).

It doesn’t seem that useful to me to have different error messages based on how things were historically if that has no impact on how we interpret the blockchain today though. IMO, better to either just say “invalid opcode” in all cases, or, if anything, document the difference the between “invalid if present anywhere” and “invalid if executed”.

ChrisCho-H commented at 5:03 pm on August 10, 2023:

If there’s not any meaningful difference btw disabled and reserved ones, it would make sense to throw disabled error for both of them as it’s how it works for disabled opcodes. like below

 0    ...
 1    default:
 2        if (opcode == OP_VERIF || 
 3            opcode == OP_VERNOTIF ||
 4            opcode == OP_VER ||
 5            opcode == OP_RESERVED ||
 6            opcode == OP_RESERVED1 ||
 7            opcode == OP_RESERVED2) {
 8            return set_error(serror, SCRIPT_ERR_DISABLED_OPCODE);
 9        } else {
10            return set_error(serror, SCRIPT_ERR_BAD_OPCODE);
11        }

in this way we don’t need to add reserved in bad opcode error message.

ajtowns commented at 2:56 am on August 9, 2023: contributor

I don’t understand why we’d want to touch consensus code for something that’s at most a slightly confusing error message? Just changing the error description as in #28234 seems a strictly better approach.

~As it stands, SCRIPT_ERR_DISABLED_OPCODE covers opcodes that are unusable even in unexecuted IF/ELSE branches, while SCRIPT_ERR_BAD_OPCODE covers opcodes that are only unusable when evaluated.~ (VERIF and VERNOTIF are exceptions to this)

ChrisCho-H commented at 3:29 am on August 9, 2023: none

@ajtowns #28234 is for reserved opcode, but this change is for disabled opcode. Moreover, it’s not just about confusing err msg. I mentioned 2 other reasons(first and second reason above), especially for OP_VERIF and OP_VERNOTIF, which are unusable even in unexecuted IF/ELSE branches. I agree that we need to be conservative for consensus code, though I don’t see the side effect of this change(let me know if there is!).

ajtowns commented at 6:04 am on August 9, 2023: contributor

There’s no difference between “reserving” an opcode and “disabling” it other than the error message that’s passed through when it’s used. Having different error messages for “error even if not executed” and “error only when it’s executed” might make sense, but that isn’t what we currently do (OP_VERIF/OP_VERNOTIF are exceptions) and isn’t what this PR currently does.

Having the error codes be:

0        case SCRIPT_ERR_BAD_OPCODE:
1            return "Attempted to execute invalid/reserved opcode";
2        case SCRIPT_ERR_DISABLED_OPCODE:
3            return "Disabled opcode present in script";

or similar, and just changing the results for VERIF/VERNOTIF by:

0    ...
1    default:
2        if (opcode == OP_VERIF || opcode == OP_VERNOTIF) {
3            return set_error(serror, SCRIPT_ERR_DISABLED_OPCODE);
4        } else {
5            return set_error(serror, SCRIPT_ERR_BAD_OPCODE);
6        }

seems simpler to ensure there’s no meaningful consensus change.

2. but it’s such an inefficient to go through all cases even if it does not need to

A switch statement isn’t made inefficient by the presence of other cases, it’s just a lookup and a jump. (It is made inefficient if the cases are sparse, but they’re not here)

ChrisCho-H commented at 7:33 am on August 9, 2023: none

just out of curiosity, what’s the difference between this(just removed fExec && opcode == OP_VER check)

 0            if (opcode == OP_CAT ||
 1                opcode == OP_SUBSTR ||
 2                opcode == OP_LEFT ||
 3                opcode == OP_RIGHT ||
 4                opcode == OP_INVERT ||
 5                opcode == OP_AND ||
 6                opcode == OP_OR ||
 7                opcode == OP_XOR ||
 8                opcode == OP_2MUL ||
 9                opcode == OP_2DIV ||
10                opcode == OP_MUL ||
11                opcode == OP_DIV ||
12                opcode == OP_MOD ||
13                opcode == OP_LSHIFT ||
14                opcode == OP_RSHIFT ||
15                opcode == OP_VERIF ||
16                opcode == OP_VERNOTIF) 
17                return set_error(serror, SCRIPT_ERR_DISABLED_OPCODE); // Disabled opcodes (CVE-2010-5137).

and this?

0    ...
1    default:
2        if (opcode == OP_VERIF || opcode == OP_VERNOTIF) {
3            return set_error(serror, SCRIPT_ERR_DISABLED_OPCODE);
4        } else {
5            return set_error(serror, SCRIPT_ERR_BAD_OPCODE);
6        }

and at first I didn’t include OP_VER but @achow101 suggested to include it in disabled err(which makes sense to me). however, if simply changing err msg(https://github.com/bitcoin/bitcoin/pull/28234) could works for OP_VER, we can omit check for OP_VER.

ajtowns commented at 8:16 am on August 9, 2023: contributor

Changing nothing other than the default clause makes it easy to see that the only functional change is in the error code being returned. Changing the logic outside the switch requires more care to see that there aren’t any other weird behaviours being introduced. That is, it’s optimising for simplicity of review.

If you’re wanting to give the same error message for OP_VER (which is only invalid if executed) the same as OP_VERIF (which is invalid even if not executed) then an even simpler change is to just make both SCRIPT_ERR_BAD_OPCODE and SCRIPT_ERR_DISABLED_OPCODE give the same error message and not touch consensus code at all, eg:

0    case SCRIPT_ERR_BAD_OPCODE:
1    case SCRIPT_ERR_DISABLED_OPCODE:
2        return "Invalid opcode (disabled/reserved/unknown)";

ChrisCho-H force-pushed on Aug 10, 2023

ChrisCho-H requested review from ajtowns on Aug 10, 2023

ChrisCho-H renamed this:
~~script: throw disabled err for op_ver and its variants~~
script: throw disabled err for op_verif and op_vernotif
on Aug 10, 2023

ChrisCho-H renamed this:
~~script: throw disabled err for op_verif and op_vernotif~~
script: throw disabled err for op_ver and its variants
on Aug 10, 2023

feat: OP_VERIF and OP_VERNOTIF throw disabled error 2d6530b4c5

ChrisCho-H force-pushed on Aug 10, 2023

ChrisCho-H commented at 5:23 pm on August 10, 2023: none

I move the logic to default clause following @ajtowns opinion, but still include OP_VER to throw disabled error(because it’s disabled opcode). ~~I also added reserved for bad opcode error in 17a0270da5536547850dca06885a278d3e0d3269 which is for reserved opcode.~~

I mentioned above, but If there’s not any meaningful difference btw disabled and reserved ones, it would make sense to throw disabled error for both of them as it’s how it works for disabled opcodes. like below

 0    ...
 1    default:
 2        if (opcode == OP_VERIF || 
 3            opcode == OP_VERNOTIF ||
 4            opcode == OP_VER ||
 5            opcode == OP_RESERVED ||
 6            opcode == OP_RESERVED1 ||
 7            opcode == OP_RESERVED2) {
 8            return set_error(serror, SCRIPT_ERR_DISABLED_OPCODE);
 9        } else {
10            return set_error(serror, SCRIPT_ERR_BAD_OPCODE);
11        }

~~in this way we don’t need to add reserved in bad opcode error message.~~

ajtowns commented at 5:43 am on August 11, 2023: contributor

If we’re not differentiating between OP_VER and OP_VERIF, I think we should just merge the DISABLED_OPCODE and BAD_OPCODE errors.

ChrisCho-H commented at 6:02 am on August 11, 2023: none

If DISABLED_OPCODE and BAD_OPCODE error throw the same message, do we need to have both error case? not sure about the grounds to differentiate opcodes throwing DISABLED_OPCODE and BAD_OPCODE if have same message.

DrahtBot added the label CI failed on Oct 25, 2023

DrahtBot commented at 2:34 pm on February 5, 2024: contributor

🤔 There hasn’t been much activity lately and the CI seems to be failing.

If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn’t been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

ChrisCho-H commented at 3:27 am on March 28, 2024: none

I think it’s better not to use bad_opcode_err for disabled opcodes. The reason is bad_opcode_err is really for unknown err which ends up to default branch, so should maintain its message as it is(which good software should be). Merging bad_opcode_err and disabled_opcode_err is even worse as each of responsibility is literally different. I still think it’s just wrong to throw bad_opcode_err for OP_VER and its variants, as those are not “unknown opcode”, but really disabled ones. It’s ready for any review.

ChrisCho-H force-pushed on Mar 28, 2024

DrahtBot removed the label CI failed on Mar 28, 2024

achow101 commented at 7:39 pm on April 23, 2024: member

ACK 2d6530b4c5300a29d87fb7042c6cec0b08cc9a0f

DrahtBot requested review from jonatack on Apr 23, 2024

ajtowns commented at 7:55 pm on April 23, 2024: contributor

I’m still ~0 on whether this actually improves anything, but I don’t believe it causes any potential problems.

maflcko commented at 6:46 am on April 24, 2024: member

I think it is hard to follow this pull request. The motivation (pull request description) starts with “Edit:”, and then has a list of reasons, where the second one is struck through. I don’t think it makes it easy for reviewers, if they have to re-construct themselves why this pull request was created, and which parts of it were based on misunderstandings. The pull request description ends up in the merge commit, so it should be accurate and on point, not be used as a scratch-pad.

I personally think returning BAD_OPCODE (and it’s message) is fine and refactoring consensus code for better documentation is probably the wrong way to go.

An alternative would be to re-word the BAD_OPCODE message for clarity. Yet another alternative might be to rename the opcode to RESERVED3 (etc)? However, absent a greater picture of this change, I don’t think it is easy to decide which alternative of those is worth it, or if the code is better left as-is for now.

fanquake commented at 11:24 am on April 24, 2024: member

I agree. At a minimum, given the PR description is near impossible to follow, it should be re-written. Especially given the commit only contains feat: OP_VERIF and OP_VERNOTIF throw disabled error.

ChrisCho-H commented at 2:11 am on April 26, 2024: none

Throw DISABLED_OPCODE error for op_ver and its variants as it’s known but just disabled opcodes. Currently, interpreter throws BAD_OPCODE which contains the message of "Opcode missing or not understood", which isn’t accurate and confusing.

Apologize for poor description, I just re-write to make it simple and clear. I agree that this one is practically doesn’t affect any, as only a few will push OP_VER opcodes on stack. It’s such a minor fix, and when I just found out that OP_VER throws BAD_OPCODE error, I thought it’s just wrong to throw unknown opcode error just because it’s not unknown opcode, considering only the limited context of interpreter code itself.

However, as @maflcko mentioned, changing consensus code to fix trivial thing(changing error case to throw) might be wrong(although I think this won’t make problem). It’s ok to close this PR if that is better. As a noob I’m not 100% sure about which direction is better, and think experts like you will make much better decision.

maflcko commented at 5:33 am on April 26, 2024: member

It’s ok to close this PR if that is better.

Ok, I’ll close it for now. When there is need, it can be re-opened later or picked up again. Thanks for working on this for such a long time.

maflcko closed this on Apr 26, 2024

script: throw disabled err for op_ver and its variants #28169

Code Coverage

Reviews