test,refactor: extract script template helpers & widen sigop count coverage

l0rinc commented at 2:44 pm on June 11, 2025: contributor

Summary

This PR extracts a set of cheap, inline helpers for recognizing the most common script templates, clarifies the definition of valid op-codes for legacy (pre-Taproot) scripts, and splits the existing “deserialize + check-block” benchmark into three orthogonal micro-benchmarks.
The change is behavior-neutral for consensus and policy - every modified call-site performs the same checks as before, just through clearer helper functions.

Context

While working on GetSigOpCount optimizations for known script templates I noticed that feature-test coverage was thin for this code path.
This PR therefore adds tests for error cases (malformed push-data encodings) and for the expected sigop totals of the various script templates (including edge cases like a sigop after an OP_RETURN).

Given the difficulty of reviewing the original optimizations themselves, I split all test / benchmark work into this standalone PR.
The recent burst of sigops related refactor activity (#31624, #32521, #32533) underlines the need for extra safety.

The refactors here eliminate magic numbers, deduplicate template checks, and lay groundwork for future performance work.

Structure

Benchmarks - first commits separate deserialization+hashing, validation, and sigop counting so each cost can be measured independently.
Template helpers - tiny, mechanical commits move each script-template check into script.h, replace all ad-hoc byte-checks, and add tests where necessary.
Tests - a full script-test suite covering malformed PUSHDATA sequences and the legacy / accurate sigop totals for every standard template.
Legacy opcode ceiling - final commit documents, enforces, and tests that OP_CHECKSIGADD > MAX_OPCODE.

DrahtBot commented at 2:44 pm on June 11, 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/32729.

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:

#32554 (bench: replace embedded raw block with configurable block generator by l0rinc)
#31989 (BIP-119 (OP_CHECKTEMPLATEVERIFY) (regtest only) by jamesob)
#29060 (Policy: Report reason inputs are non standard by ismaelsadeeq)

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.

l0rinc renamed this:
~~test,refactor: sigops~~
test: extract script template helpers & widen sigop count coverage
on Jun 11, 2025

DrahtBot added the label Tests on Jun 11, 2025

l0rinc renamed this:
~~test: extract script template helpers & widen sigop count coverage~~
test,refactor: extract script template helpers & widen sigop count coverage
on Jun 11, 2025

l0rinc marked this as ready for review on Jun 11, 2025

DrahtBot added the label Needs rebase on Jun 11, 2025

l0rinc force-pushed on Jun 12, 2025

DrahtBot removed the label Needs rebase on Jun 12, 2025

DrahtBot added the label CI failed on Jul 21, 2025

DrahtBot commented at 10:23 am on July 21, 2025: contributor

🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/runs/43965124848 LLM reason (✨ experimental): The failure is caused by a compilation error in policy.cpp where a boolean value is incorrectly passed to a function expecting a CScript reference.

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.

bench: measure `CheckBlock` speed separately from serialization

To measure CheckBlock performance in isolation from deserialization overhead, a ResetChecked() method was introduced to re-enable the block's validation state, allowing repeated validation of the same block instance.

f06c12b0cc

bench: measure `SigOpsBlock` speed separately

Add benchmark to measure the performance of counting legacy signature operations in a block, independent of other validation steps.

7f13048975

refactor: rename `GetSigOpCount` to `GetLegacySigOpCount`

Also added primitive argument name reminder to the call sites to highlight the meaning of the call.

214a925863

refactor: move the script-hash-size constants to `script.h`

This enables using these constants in script.h definitions in upcoming commits. No naming conflicts exist with these constant names.

7e1bfd0366

refactor: pull `IsPayToScriptHash` to header

Moved the implementation to the header (implicitly inline + noexcept), removed redundant `this` and parentheses, used descriptive `front()`/`back()` methods and changed the `0x14` magic constant to descriptive `WITNESS_V0_KEYHASH_SIZE`.
See: https://learnmeabitcoin.com/technical/script/p2sh/#scriptpubkey

87daf710c8

refactor: pull `IsPayToWitnessScriptHash` to header

Moved the implementation to the header (implicitly inline + noexcept), removed redundant `this` and parentheses, used descriptive `front()` method and changed the `0x20` magic constant to descriptive `WITNESS_V0_SCRIPTHASH_SIZE`.
See: https://learnmeabitcoin.com/technical/script/p2wsh/#scriptpubkey

1d6714d876

refactor: pull `IsPayToAnchor` to header

Moved the implementation to the header (implicitly inline + noexcept), removed redundant `this` and parentheses, used descriptive `front()`/`back()` methods.

a363c2eb56

refactor: pull `IsPayToTaproot` to header

Moved the implementation to the header (implicitly inline + noexcept), removed redundant `this` and parentheses, used descriptive `front()` method and changed the `0x20` magic constant to descriptive `WITNESS_V1_TAPROOT_SIZE`.
See: https://learnmeabitcoin.com/technical/script/p2tr/#scriptpubkey

71355b2de2

refactor: add `IsPayToPubKeyHash` helper to script.h

The usages in `compressor.cpp` and `solver.cpp` were also updated to use the new method.
See: https://learnmeabitcoin.com/technical/script/p2pkh/#scriptpubkey

5c1d1c4f7f

refactor: add `IsCompressedPayToPubKey` helper to script.h

The usages in `compressor.cpp` and `solver.cpp` were also updated to use the new method.
Note that compressor has additional prefix checks as well, which are not properly exercised by the `compressed_p2pk` unit test.
See: https://learnmeabitcoin.com/technical/script/p2pk/#scriptpubkey and https://learnmeabitcoin.com/technical/keys/public-key/#compressed

b7e63066c5

refactor: add `IsUncompressedPayToPubKey` helper to script.h

The usages in `compressor.cpp` and `solver.cpp` were also updated to use the new method.
Note that compressor has additional prefix checks as well, which are not properly exercised by the `uncompressed_p2pk` unit test.
See: https://learnmeabitcoin.com/technical/script/p2pk/#scriptpubkey and https://learnmeabitcoin.com/technical/keys/public-key/#uncompressed

cc64fab7a5

refactor: add `IsPayToWitnessPubKeyHash` helper to script.h

See: https://learnmeabitcoin.com/technical/script/p2wpkh/#scriptpubkey

7eeff0af7f

test: add `SigOpCount` edge-cases & known-template coverage

* `GetLegacySigOpCountErrors` feeds malformed PUSHDATA sequences to confirm the parser never crashes and still counts sig-ops that appear before the error.
* `GetLegacySigOpCountKnownTemplates` asserts the expected legacy/accurate sig-op totals for all common known script templates (P2PKH, P2SH, P2WPKH/WSH, P2TR, compressed & uncompressed P2PK, OP_RETURN, multisig).

1acaed3927

test: make sure OP_CHECKSIGADD isn't considered valid (legacy) sigop

OP_CHECKSIGADD (opcode 0xba) is a Tapscript-only opcode introduced with Taproot and should not be counted as a signature operation in legacy (pre-Tapscript) contexts. It being larger than the "max" is confusing, hence the rename.

5d29ce5c44

l0rinc force-pushed on Jul 21, 2025

l0rinc commented at 7:58 pm on July 21, 2025: contributor

Rebased after https://github.com/bitcoin/bitcoin/pull/32521/files#diff-ea6d307faa4ec9dfa5abcf6858bc19603079f2b8e110e1d62da4df98f4bdb9c0R184