Skip to content

Conversation

Jianru-Lin
Copy link

Description

This pull request addresses an error code inconsistency in the handling of empty stacks for the OP_IF and OP_NOTIF Bitcoin Script opcodes.

Currently, if the stack is empty when evaluating these opcodes, the script returns SCRIPT_ERR_UNBALANCED_CONDITIONAL, which is misleading. The correct error code for this scenario should be SCRIPT_ERR_INVALID_STACK_OPERATION, as the issue lies in insufficient stack elements rather than mismatched conditionals.

This change modifies the script execution engine to return SCRIPT_ERR_INVALID_STACK_OPERATION when an empty stack is encountered during OP_IF and OP_NOTIF processing.

Motivation and Improvement

  • Accuracy: The updated error code aligns with the actual nature of the problem, providing more accurate feedback during script validation.
  • Debugging and Analysis: This correction will make it easier for developers and users to diagnose script failures accurately, leading to improved debugging and troubleshooting.
  • Consistency: It brings the error handling for empty stack conditions in line with other parts of the Bitcoin Script interpreter, enhancing overall code consistency.

Testing

This change is accompanied by unit tests in the src/test/ directory that specifically verify the corrected behavior for OP_IF and OP_NOTIF with empty stacks.

Additional Considerations

While this change might appear minor, its impact on error reporting and debugging is significant. It contributes to a more robust and accurate Bitcoin Script validation process.

Note: Please note that no functional tests have been added as this is a minor fix in the script interpreter's logic, not affecting the overall Bitcoin functionality.

I hope this pull request description is helpful. Please let me know if you have any other questions or need further refinements.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 28, 2024

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.
A summary of reviews will appear here.

@fjahr
Copy link
Contributor

fjahr commented Jun 28, 2024

This change is accompanied by unit tests in the src/test/ directory that specifically verify the corrected behavior for OP_IF and OP_NOTIF with empty stacks.

Those seem to be missing

@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is 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.

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

Debug: https://github.com/bitcoin/bitcoin/runs/26818011584

@maflcko
Copy link
Member

maflcko commented Jul 1, 2024

Closing for now. At a minimum for changes like these, a unit test will have to be added. Also, the existing tests will need to pass. You'll have to run make check && ./test/functional/test_runner.py locally (before opening a pull request).

@maflcko maflcko closed this Jul 1, 2024
@bitcoin bitcoin locked and limited conversation to collaborators Jul 1, 2025
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.

4 participants