scripted-diff: adapt script error constant names in feature_taproot.py #32415

pull theStack wants to merge 1 commits into bitcoin:master from theStack:202505-test-adapt_consensus_error_constants changing 1 files +98 −98

theStack commented at 6:28 PM on May 3, 2025: contributor

While reviewing #31622 I noticed that the constant name (SCRIPT_)ERR_SIG_HASHTYPE is used for two different script verification error codes, namely one for legacy and one for Schnorr signatures:

https://github.com/bitcoin/bitcoin/blob/eba5f9c4b63fe46261fbb3e71b9a94832d105b23/src/script/script_error.cpp#L56-L57 https://github.com/bitcoin/bitcoin/blob/eba5f9c4b63fe46261fbb3e71b9a94832d105b23/test/functional/feature_taproot.py#L600

In order to resolve this confusion, this PR adapts all script error constant names in the functional tests (currently only in feature_taproot.py) to the ones used in our C++ codebase (see script_error.cpp) with a scripted diff. This also makes checking whether we have test coverage for a certain script error easier.
DrahtBot commented at 6:28 PM on May 3, 2025: contributor


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/32415.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK jamesob, rkrux, stratospher, achow101

Concept ACK sipa

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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #32301 (test: cover invalid codesep positions for signature in taproot by instagibbs)
- #32270 (test: fix pushdata scripts by instagibbs)
- #32247 (BIP-348 (OP_CHECKSIGFROMSTACK) (regtest only) by jamesob)
- #29491 ([EXPERIMENTAL] Schnorr batch verification for blocks by fjahr)
- #29371 (test: Add leaf_version parameter to taproot_tree_helper() by Christewart)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.



LLM Linter (✨ experimental)

Possible typos and grammar issues:

Error: “neither compressed or uncompressed” → “neither compressed nor uncompressed”
DrahtBot added the label Refactoring on May 3, 2025

Type	Reviewers
ACK	jamesob, rkrux, stratospher, achow101
Concept ACK	sipa

scripted-diff: adapt script error constant names in feature_taproot.py

In order to remove potential confusion, this commit adapts all script
error constant names in the functional tests (currently only in
feature_taproot.py) to the ones used in our C++ codebase. This also
makes checking whether we have test coverage for a certain script error
easier.

-BEGIN VERIFY SCRIPT-

ren() { sed -i "s|$1|$2|g" $( git grep -l "$1" -- "./test" ) ; }

ren ERR_SIG_SIZE          ERR_SCHNORR_SIG_SIZE
ren ERR_SIG_HASHTYPE      ERR_SCHNORR_SIG_HASHTYPE
ren ERR_SIG_SCHNORR       ERR_SCHNORR_SIG
ren ERR_CONTROLBLOCK_SIZE ERR_TAPROOT_WRONG_CONTROL_SIZE
ren ERR_PUSH_LIMIT        ERR_PUSH_SIZE
ren ERR_MINIMALIF         ERR_TAPSCRIPT_MINIMALIF
ren ERR_UNKNOWN_PUBKEY    ERR_PUBKEYTYPE
ren ERR_STACK_EMPTY       ERR_INVALID_STACK_OPERATION
ren ERR_SIGOPS_RATIO      ERR_TAPSCRIPT_VALIDATION_WEIGHT
ren ERR_UNDECODABLE       ERR_BAD_OPCODE
ren ERR_NO_SUCCESS        ERR_EVAL_FALSE
ren ERR_EMPTY_WITNESS     ERR_WITNESS_PROGRAM_WITNESS_EMPTY

-END VERIFY SCRIPT-

b5f580c580

theStack force-pushed on May 3, 2025
DrahtBot added the label CI failed on May 3, 2025
DrahtBot commented at 6:32 PM on May 3, 2025: contributor


🚧 At least one of the CI tasks failed. <sub>Task lint: https://github.com/bitcoin/bitcoin/runs/41591166714</sub> <sub>LLM reason (✨ experimental): The CI failure is caused by errors in the lint check related to script evaluation failures, specifically ERR_EVAL_FALSE errors.</sub>

<details><summary>Hints</summary>

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.

</details>
DrahtBot removed the label CI failed on May 3, 2025
sipa commented at 9:06 PM on May 3, 2025: member

Concept ACK
jamesob commented at 1:45 AM on May 4, 2025: contributor

crACK https://github.com/bitcoin/bitcoin/pull/32415/commits/b5f580c580257d28d295cae3f787b55eb1863f16

Scripted-diff/func-test-only makes this an easy review. Good cleanup for this daunting file.
DrahtBot requested review from sipa on May 4, 2025
rkrux approved
rkrux commented at 12:53 PM on May 5, 2025: contributor

tACK b5f580c580257d28d295cae3f787b55eb1863f16

Having gone through feature_taproot.py file previously that can take a while to understand fully, this seems like a reasonable change to make it easier to follow the file by making the error constant names congruent with the ones in the cpp code.

+1 on using the scripted diff so that the reviewers need not manually verify each change in the file.
stratospher commented at 5:51 AM on May 6, 2025: contributor

ACK b5f580c. liked the consistency in script error names.
achow101 commented at 10:10 PM on May 6, 2025: member

ACK b5f580c580257d28d295cae3f787b55eb1863f16
achow101 merged this on May 6, 2025
achow101 closed this on May 6, 2025
theStack deleted the branch on May 6, 2025
stickies-v referenced this in commit 772a33e052 on May 23, 2025
sedited referenced this in commit cb3cd9b8b9 on May 29, 2025
yuvicc referenced this in commit 069643f094 on Jul 6, 2025
bug-castercv502 referenced this in commit 2858098810 on Sep 28, 2025
Kino1994 referenced this in commit de2f5207dc on Jun 28, 2026
BigcoinBGC referenced this in commit f406e53d66 on Jun 30, 2026
bitcoin locked this on Jul 30, 2026