test: Improve STRICTENC/DERSIG unit tests #34295

billymcbip commented at 10:46 PM on January 14, 2026: contributor

Remove a comment referencing a file that no longer exists in the codebase: script_invalid.json.
Fix a test that isn't implemented as intended. The idea is to test execution order by providing a signature that would cause script failure when parsed. An empty signature does not cause script failure in CHECKMULTISIG. Use OP_1 for the second signature instead of OP_0.
Copy existing STRICTENC tests and change the flag to DERSIG. DERSIG is a consensus flag (unlike STRICTENC), so it'd be good to have dedicated test cases.

script_tests pass on my end.

DrahtBot added the label Tests on Jan 14, 2026

DrahtBot commented at 10:46 PM on January 14, 2026: 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/34295.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	darosior, sipa

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

"to provide a compact way to provide" -> "to provide a compact way to present" [duplicate "provide" makes the phrase ungrammatical/awkward and hinders comprehension]

in src/test/data/script_tests.json:703 in 488550a74f outdated

 698 |      "STRICTENC", "OK",
 699 |      "2-of-2 CHECKMULTISIG NOT with the second pubkey invalid, and both signatures validly encoded. Valid pubkey fails, and CHECKMULTISIG exits early, prior to evaluation of second invalid pubkey."
 700 |  ],
 701 |  [
 702 | -    "0 0 0x47 0x3044022044dc17b0887c161bb67ba9635bf758735bdde503e4b0a0987f587f14a4e1143d022009a215772d49a85dae40d8ca03955af26ad3978a0ff965faa12915e9586249a501",
 703 | +    "0 1 0x47 0x3044022044dc17b0887c161bb67ba9635bf758735bdde503e4b0a0987f587f14a4e1143d022009a215772d49a85dae40d8ca03955af26ad3978a0ff965faa12915e9586249a501",

darosior commented at 7:38 PM on January 29, 2026:

Great catch.

billymcbip commented at 2:35 PM on January 30, 2026:

Thanks!

in src/test/data/script_tests.json:694 in 488550a74f outdated

 690 | @@ -691,15 +691,14 @@
 691 |  ["to test the exact order in which signatures and pubkeys are evaluated by"],
 692 |  ["distinguishing CHECKMULTISIG returning false on the stack and the script as a"],
 693 |  ["whole failing."],
 694 | -["See also the corresponding inverted versions of these tests in script_invalid.json"],

darosior commented at 7:38 PM on January 29, 2026:

This could have been its own commit since it's unrelated to the test fix.

billymcbip commented at 2:34 PM on January 30, 2026:

Agree, done.

darosior approved

darosior commented at 7:40 PM on January 29, 2026: member

ACK 63ff7c4158a46ff5d726a576e3ac1f384e6cc2b7

Could you add the context given in OP to the commit messages? It's useful to be able to have the context for a change (especially one that isn't immediate such as this one) when git blaming or searching through the git history, without having to rely on Github.

test: Remove outdated comment in script_tests

script_invalid.json no longer exists.

527e8ca7b5

test: Fix a STRICTENC test in script_tests

Fix a test that isn't implemented as intended. The idea is to test execution order by providing a signature that would cause script failure when parsed. An empty signature does not cause script failure in CHECKMULTISIG. Use OP_1 for the second signature instead of OP_0.

884978f389

test: Add DERSIG tests to script_tests

Copy existing STRICTENC tests and change the flag to DERSIG.

4dfb6eef70

billymcbip force-pushed on Jan 30, 2026

darosior approved

darosior commented at 2:44 PM on January 30, 2026: member

reACK 4dfb6eef70d719a79904cabc4519d7a725de130a

fanquake requested review from sipa on Jan 30, 2026

sipa commented at 6:08 PM on February 3, 2026: member

ACK 4dfb6eef70d719a79904cabc4519d7a725de130a

fanquake merged this on Feb 4, 2026

fanquake closed this on Feb 4, 2026