test: Remove redundant STRICTENC flag from script_tests #34294

pull billymcbip wants to merge 1 commits into bitcoin:master from billymcbip:strictenc changing 1 files +830 −830
  1. billymcbip commented at 9:21 pm on January 14, 2026: contributor

    Remove the STRICTENC flag from hundreds of script_tests cases that have nothing to do with signature or pubkey encoding. It’s a readability improvement and it makes it easier to find the actual test cases for STRICTENC behavior (those are untouched). Also, STRICTENC is a policy flag, we should default to consensus flags in script_tests.

    The diff is large but mechanical as I’m only removing STRICTENC from flags. That said, if reviewers feel this cleanup is not worthwhile, I am fine dropping it.

    script_tests pass on my end.

  2. test: Remove redundant STRICTENC flag from script_tests faa2dced89
  3. DrahtBot added the label Tests on Jan 14, 2026
  4. DrahtBot commented at 9:21 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/34294.

    Reviews

    See the guideline for information on the review process. A summary of reviews will appear here.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #32729 (test,refactor: extract script template helpers & widen sigop count coverage by l0rinc)
    • #32143 (Fix 11-year-old mis-categorized error code in OP_IF evaluation by cculianu)

    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:

    • “Multiple ELSE’s are valid and executed inverts on each ELSE encountered” -> “Multiple ELSE’s are valid and execution inverts on each ELSE encountered” [the clause “and executed inverts” is ungrammatical; “execution inverts” restores clear meaning]

    • “opcodes above MAX_OPCODE invalid if executed” -> “opcodes above MAX_OPCODE are invalid if executed” [missing verb “are” makes the sentence grammatically incomplete]

    2026-01-14

  5. l0rinc commented at 10:49 pm on January 14, 2026: contributor
  6. billymcbip commented at 10:55 pm on January 14, 2026: contributor
    @l0rinc sounds good, will do if this has a good chance of being merged. Will wait for some Concept (N)ACKs.
  7. DrahtBot commented at 2:22 am on January 20, 2026: contributor
    🐙 This pull request conflicts with the target branch and needs rebase.
  8. DrahtBot added the label Needs rebase on Jan 20, 2026
  9. billymcbip closed this on Jan 21, 2026

  10. darosior commented at 3:17 pm on January 21, 2026: member

    Will wait for some Concept (N)ACKs.

    I was planning on giving feedback, so i’ll share it despite it now being closed, in case it’s useful. TL;DR: I think it’s the right call to focus efforts on something else.

    It’s a readability improvement

    Yes, but it is also technically a test degradation. It might be one we are fine with, but technically it makes tests for valid scripts less strict. Of course it also makes tests for invalid scripts stricter (“this still fails with even less checks”). OP does not weigh the pros and cons of doing this and it’s not immediate to me this is worth it.

    we should default to consensus flags in script_tests

    I don’t know about that. Maybe we should, but this is not what this unit test currently does. It tests various combinations of Script flag regardless of whether they are policy or consensus, which also seems fine.

    scripted-diffs

    It’s not clear to me how, or it would be quite convoluted. It seems to me each test case would have to be reviewed on its own for whether it makes sense to drop the script flag from it.

    Overall it’s not clear to me that this change is desirable, and i don’t find the current rationale given in OP convincing because it omits some considerations. Furthermore, it’s a large change that seems tedious to review and maintain up to date, so this doesn’t seem to be the best bang for our buck. Therefore i would tend toward Concept NACK as it currently stands.


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-01-31 06:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me