Skip to content

Fix 11-year-old mis-categorized error code in OP_IF evaluation #32143

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.

Sign up for GitHub

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

Jump to bottom
Open
cculianu wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix 11-year-old mis-categorized error code in OP_IF evaluation #32143

cculianu wants to merge 1 commit into from
+14 −14

Conversation

@cculianu
Copy link

@cculianu cculianu commented Mar 26, 2025 •
edited
Loading

This was introduced by commit ab9edbd.

It appears the original author may have gotten tired and pasted the wrong error code into this 1 place. Every other situation where the value stack lacks the required number of arguments for the op-code, SCRIPT_ERR_INVALID_STACK_OPERATION is reported. Not so here.

This commit fixes the situation.

EDIT: Note this turns out to be a dupe of the abandoned #30359 .

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 26, 2025 •
edited
Loading

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32143.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK darosior, billymcbip
Stale ACK laanwj

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

No conflicts as of last run.

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 26, 2025

🚧 At least one of the CI tasks failed.
Debug: https://github.com/bitcoin/bitcoin/runs/39412787907

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

  • Possibly due to a silent merge conflict (the changes in this pull request being
    incompatible with the current code in the target branch). If so, make sure to rebase on the latest
    commit of the target branch.

  • A sanitizer issue, which can only be found by compiling with the sanitizer and running the
    affected test.

  • An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

@laanwj
Copy link
Member

laanwj commented Mar 26, 2025 •
edited
Loading

This is failing the "test each commit" check. Please squash test commits where needed to make sure that the intermediate commits pass the tests, too.

Details 
test/script_tests.cpp(908): Entering test case "script_json_test"
2025-03-26T09:15:59.630554Z [test] [test/util/random.cpp:48] [SeedRandomStateForTest] Setting random seed for current tests to RANDOM_CTX_SEED=be1d635f1a8e48c6048a40b25f2bd5e1d235e5ec1571241fe7157b541468c337
2025-03-26T09:15:59.630977Z [test] [init/common.cpp:151] [LogPackageVersion] Bitcoin Core version v29.99.0-da65703b810d (release build)
test/script_tests.cpp(127): error: in "script_tests/script_json_test": INVALID_STACK_OPERATION where UNBALANCED_CONDITIONAL expected: ["NOP","IF 1 ENDIF","P2SH,STRICTENC","UNBALANCED_CONDITIONAL","The following tests check the if(stack.size() < N) tests in each opcode"]
test/script_tests.cpp(127): error: in "script_tests/script_json_test": INVALID_STACK_OPERATION where UNBALANCED_CONDITIONAL expected: ["NOP","NOTIF 1 ENDIF","P2SH,STRICTENC","UNBALANCED_CONDITIONAL","They are here to catch copy-and-paste errors"]
test/script_tests.cpp(127): error: in "script_tests/script_json_test": INVALID_STACK_OPERATION where UNBALANCED_CONDITIONAL expected: ["0x03 0x635168","HASH160 0x14 0xe7309652a8e3f600f06f5d8d52d6df03d2176cc3 EQUAL","P2SH,WITNESS,MINIMALIF","UNBALANCED_CONDITIONAL"]
test/script_tests.cpp(127): error: in "script_tests/script_json_test": INVALID_STACK_OPERATION where UNBALANCED_CONDITIONAL expected: ["0x03 0x645168","HASH160 0x14 0x0c3f8fe3d6ca266e76311ecda544c67d15fdd5b0 EQUAL","P2SH,WITNESS,MINIMALIF","UNBALANCED_CONDITIONAL"]
test/script_tests.cpp(127): error: in "script_tests/script_json_test": INVALID_STACK_OPERATION where UNBALANCED_CONDITIONAL expected: [["635168",0.00000001],"","0 0x20 0xc7eaf06d5ae01a58e376e126eb1e6fab2036076922b96b2711ffbec1e590665d","P2SH,WITNESS","UNBALANCED_CONDITIONAL"]
test/script_tests.cpp(127): error: in "script_tests/script_json_test": INVALID_STACK_OPERATION where UNBALANCED_CONDITIONAL expected: [["635168",0.00000001],"","0 0x20 0xc7eaf06d5ae01a58e376e126eb1e6fab2036076922b96b2711ffbec1e590665d","P2SH,WITNESS,MINIMALIF","UNBALANCED_CONDITIONAL"]
test/script_tests.cpp(127): error: in "script_tests/script_json_test": INVALID_STACK_OPERATION where UNBALANCED_CONDITIONAL expected: [["645168",0.00000001],"","0 0x20 0xf913eacf2e38a5d6fc3a8311d72ae704cb83866350a984dd3e5eb76d2a8c28e8","P2SH,WITNESS","UNBALANCED_CONDITIONAL"]
test/script_tests.cpp(127): error: in "script_tests/script_json_test": INVALID_STACK_OPERATION where UNBALANCED_CONDITIONAL expected: [["645168",0.00000001],"","0 0x20 0xf913eacf2e38a5d6fc3a8311d72ae704cb83866350a984dd3e5eb76d2a8c28e8","P2SH,WITNESS,MINIMALIF","UNBALANCED_CONDITIONAL"]
test/script_tests.cpp(127): error: in "script_tests/script_json_test": INVALID_STACK_OPERATION where UNBALANCED_CONDITIONAL expected: [["635168",0.00000001],"0x22 0x0020c7eaf06d5ae01a58e376e126eb1e6fab2036076922b96b2711ffbec1e590665d","HASH160 0x14 0x9b27ee6d9010c21bf837b334d043be5d150e7ba7 EQUAL","P2SH,WITNESS","UNBALANCED_CONDITIONAL"]
test/script_tests.cpp(127): error: in "script_tests/script_json_test": INVALID_STACK_OPERATION where UNBALANCED_CONDITIONAL expected: [["635168",0.00000001],"0x22 0x0020c7eaf06d5ae01a58e376e126eb1e6fab2036076922b96b2711ffbec1e590665d","HASH160 0x14 0x9b27ee6d9010c21bf837b334d043be5d150e7ba7 EQUAL","P2SH,WITNESS,MINIMALIF","UNBALANCED_CONDITIONAL"]
test/script_tests.cpp(127): error: in "script_tests/script_json_test": INVALID_STACK_OPERATION where UNBALANCED_CONDITIONAL expected: [["645168",0.00000001],"0x22 0x0020f913eacf2e38a5d6fc3a8311d72ae704cb83866350a984dd3e5eb76d2a8c28e8","HASH160 0x14 0xdbb7d1c0a56b7a9c423300c8cca6e6e065baf1dc EQUAL","P2SH,WITNESS","UNBALANCED_CONDITIONAL"]
test/script_tests.cpp(127): error: in "script_tests/script_json_test": INVALID_STACK_OPERATION where UNBALANCED_CONDITIONAL expected: [["645168",0.00000001],"0x22 0x0020f913eacf2e38a5d6fc3a8311d72ae704cb83866350a984dd3e5eb76d2a8c28e8","HASH160 0x14 0xdbb7d1c0a56b7a9c423300c8cca6e6e065baf1dc EQUAL","P2SH,WITNESS,MINIMALIF","UNBALANCED_CONDITIONAL"]
test/script_tests.cpp(908): Leaving test case "script_json_test"; testing time: 184114us

@cculianu
Fix 11-year-old mis-categorized error code in OP_IF evaluation
a7b5814 
This was introduced by commit ab9edbd.

It appears the original author may have gotten tired and pasted the wrong
error code into this 1 place. Every other situation where the value stack
lacks the required number of arguments for the op-code,
SCRIPT_ERR_INVALID_STACK_OPERATION is reported. Not so here.

This commit fixes the situation.

Also in this commit:

- Fix script_tests to adjust to the corrected error message
- Fix p2p_invalid_tx functional test to produce the desired error message
@cculianu
Copy link
Author

cculianu commented Mar 26, 2025

Ok, squashed.

@DrahtBot DrahtBot mentioned this pull request Jul 19, 2025
Closed
@achow101 achow101 requested review from darosior and theuni October 22, 2025 14:24
darosior
darosior approved these changes Dec 5, 2025
View reviewed changes
Copy link
Member

@darosior darosior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK a7b5814

@DrahtBot DrahtBot requested a review from laanwj December 5, 2025 14:53
@billymcbip
Copy link
Contributor

billymcbip commented Dec 30, 2025

tACK a7b5814

Cherry-picked on latest master (2bcb3f6) and ran all unit tests (including script_asset_tests).

UNBALANCED_CONDITIONAL is intended for:

  • OP_ELSE/ENDIF not being preceded by OP_IF/NOTIF, or
  • OP_IF/NOTIF not being succeeded by OP_ENDIF

UNBALANCED_CONDITIONAL does not make sense when OP_IF/NOTIF does not have a value to pop from the stack. INVALID_STACK_OPERATION makes sense in that case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@theuni theuni Awaiting requested review from theuni

@laanwj laanwj Awaiting requested review from laanwj

1 more reviewer

@darosior darosior darosior approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Consensus Validation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@cculianu @DrahtBot @laanwj @billymcbip @darosior