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)
#32521 (policy: make pathological transactions packed with legacy sigops non-standard by darosior)
#31989 (BIP-119 (OP_CHECKTEMPLATEVERIFY) (regtest only) by jamesob)
#31682 ([IBD] specialize CheckBlock’s input & coinbase checks by l0rinc)
#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

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.

90bfb11cba

bench: measure `SigOpsBlock` speed separately

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

b78eddaf2d

refactor: rename `GetSigOpCount` to `GetLegacySigOpCount`

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

8e545ed55e

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.

fd76c3fe3a

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

1418830ad4

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

51f960dafa

refactor: pull `IsPayToAnchor` to header

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

471adaec29

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

3cb8ae4b62

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

11e07b1cba

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

e211a3b182

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

7521c54d38

refactor: add `IsPayToWitnessPubKeyHash` helper to script.h

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

061f1f5053

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

d8d20ff455

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.

0582b630d7

l0rinc force-pushed on Jun 12, 2025