test: Improve STRICTENC/DERSIG unit tests in script_tests.json #33973

1. 
   billymcbip commented at 2:51 pm on November 30, 2025: contributor

   Use the DERSIG flag instead of the STRICTENC flag for signature encoding tests. STRICTENC rules are a superset of DERSIG rules, and only DERSIG is a consensus flag. Also improve test descriptions and add two error cases to the CHECKMULTISIG NOT section to make it easier to understand how these tests work.

   Tests pass on 6224b272ac291afd3af89f91e1deaccf233718bf:

   0cmake -B build -DENABLE_WALLET=OFF
   1cmake --build build --parallel 8
   2ctest --test-dir build --parallel 8
   

   0  6/149 Test  [#93](/bitcoin-bitcoin/93/): script_tests .........................   Passed    1.42 sec
   

2. DrahtBot added the label Tests on Nov 30, 2025
3. 
   DrahtBot commented at 2:51 pm on November 30, 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/33973.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Approach ACK	darosior

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

   LLM Linter (✨ experimental)

   Possible typos and grammar issues:

   +[“pubkeys/signatures so they fail due to the STRICTENC/DERSIG rules on validly encoded”]

   • on validly encoded -> on validly encoded signatures and pubkeys [the phrase ends with “on validly encoded” and is missing the noun(s) it modifies, making the sentence incomplete and unclear.]

   2025-12-12

4. 
   darosior commented at 8:08 pm on December 4, 2025: member

   Makes sense to test DERSIG separately from STRICTENC since it’s the only one enabled by consensus. Concept/Approach ACK.

   Maintainers could we get the CI ran here?

5. DrahtBot added the label CI failed on Dec 4, 2025
6. 
   fanquake commented at 10:00 am on December 5, 2025: member

   Windows failure is unrelated: #34012.
7. maflcko closed this on Dec 5, 2025

   ---

8. maflcko reopened this on Dec 5, 2025

   ---

9. DrahtBot removed the label CI failed on Dec 5, 2025
10. 
    darosior commented at 2:26 pm on December 5, 2025: member

    Thanks!
11. in src/test/data/script_tests.json:712 in a7e91d8e58

    708+],
    709+[
    710+    "0 0 0x47 0x3044022044dc17b0887c161bb67ba9635bf758735bdde503e4b0a0987f587f14a4e1143d022009a215772d49a85dae40d8ca03955af26ad3978a0ff965faa12915e9586249a501",
    711+    "2 0x21 0x02865c40293a680cb9c020e7b1e106d8c1916d3cef99aa431a56d253e69256dac0 0x21 0x02865c40293a680cb9c020e7b1e106d8c1916d3cef99aa431a56d253e69256dac0 2 CHECKMULTISIG NOT",
    712+    "DERSIG", "OK",
    713+    "2-of-2 CHECKMULTISIG NOT with both pubkeys valid, but second signature invalid. Valid pubkey fails, and CHECKMULTISIG exits early, prior to evaluation of second invalid signature (DERSIG enabled)."
    

    ---

    ---

    

    darosior commented at 2:43 pm on December 5, 2025:

    So here the correctly-DER-encoded signature is not a valid signature for the correctly-encoded public key, is that right?

    ---

    

    billymcbip commented at 10:47 pm on December 5, 2025:

    That’s exactly right, but I think there was a mistake in the test. It should use OP_1 for the second signature instead of OP_0, because IsValidSignatureEncoding is not run for empty signatures.
12. 
    darosior commented at 2:45 pm on December 5, 2025: member

    Looks good to me, just want to confirm my understanding of one of the test cases.
13. billymcbip force-pushed on Dec 5, 2025
14. 
    billymcbip commented at 10:55 pm on December 5, 2025: contributor

    Updated CHECKMULTISIG NOT tests:

    • Removed a comment referencing a test file that no longer exists.
    • Improved test descriptions.
    • Added negative cases to make the tests easier to understand.
    • Changed invalid signature push from OP_0 to OP_1 because IsValidSignatureEncoding isn’t called for empty sigs.
15. billymcbip force-pushed on Dec 5, 2025
16. 
    billymcbip commented at 11:06 pm on December 5, 2025: contributor

    On second thought, I think it’s better to simply replace the STRICTENC flag with DERSIG when applicable instead of creating new tests. It makes the test file easier to parse.

    Edit: NVM.

17. billymcbip renamed this:
    test: Add DERSIG unit tests to script_tests.json
    test: Improve STRICTENC/DERSIG unit tests in script_tests.json
    on Dec 5, 2025
18. billymcbip requested review from darosior on Dec 7, 2025
19. billymcbip closed this on Dec 7, 2025

    ---

20. billymcbip deleted the branch on Dec 7, 2025
21. billymcbip restored the branch on Dec 7, 2025
22. billymcbip reopened this on Dec 7, 2025

    ---

23. 
    billymcbip commented at 3:56 pm on December 7, 2025: contributor

    Closed by accident. I still intend to ship this change.
24. 
    darosior commented at 4:35 pm on December 9, 2025: member

    I think it’s better to simply replace the STRICTENC flag with DERSIG when applicable instead of creating new tests.

    I’d recommend sticking to the previous approach of adding those as additional test cases. The marginal cost of an additional test case is negligible, while removing existing cases makes it harder to review because it might reduce coverage.

25. billymcbip force-pushed on Dec 10, 2025
26. 
    billymcbip commented at 4:31 pm on December 10, 2025: contributor

    @darosior I added the STRICTENC tests back. Maintainers can we run CI?
27. test: Improve STRICTENC/DERSIG unit tests in script_tests.json 188f67326d
28. billymcbip force-pushed on Dec 12, 2025
29. billymcbip marked this as a draft on Dec 12, 2025

Contributors
billymcbip DrahtBot darosior fanquake

Review Requested
darosior

Labels
Tests