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

1. 
   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.

2. 
   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”

3. DrahtBot added the label Refactoring on May 3, 2025
4. 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
5. theStack force-pushed on May 3, 2025
6. DrahtBot added the label CI failed on May 3, 2025
7. 
   DrahtBot commented at 6:32 pm on May 3, 2025: contributor

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

   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.

8. DrahtBot removed the label CI failed on May 3, 2025
9. 
   sipa commented at 9:06 pm on May 3, 2025: member

   Concept ACK
10. 
    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.

11. DrahtBot requested review from sipa on May 4, 2025
12. rkrux approved
13. 
    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.

14. 
    stratospher commented at 5:51 am on May 6, 2025: contributor

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

    ACK b5f580c580257d28d295cae3f787b55eb1863f16
16. achow101 merged this on May 6, 2025
17. achow101 closed this on May 6, 2025

    ---

18. theStack deleted the branch on May 6, 2025

Contributors
theStack DrahtBot sipa jamesob rkrux stratospher achow101

Review Requested
sipa

Labels
Refactoring