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

l0rinc · 2025-06-11T14:44:28Z

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 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 pre-taproot / accurate sigop totals for every standard template.
pre-taproot opcode ceiling - documents, enforces, and tests that OP_CHECKSIGADD > MAX_OPCODE.
Extra fuzzing - enabled all possible opcodes in ConsumeOpcodeType

DrahtBot · 2025-06-11T14:44:32Z

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.

Type	Reviewers
ACK	ryanofsky, hodlinator

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34210 (bench: Remove -priority-level= option by maflcko)
#34208 (bench: add fluent API for untimed setup steps in nanobench by l0rinc)
#33645 (refactor: optimize: avoid allocations in script & policy verification by Raimo33)
#32554 (bench: replace embedded raw block with configurable block generator by l0rinc)
#32317 (kernel: Separate UTXO set access from validation functions by sedited)
#31682 ([IBD] specialize CheckBlock's input & coinbase checks by l0rinc)
#29060 (Policy: Report debug message why 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.

DrahtBot · 2025-07-21T10:23:40Z

🚧 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.}

Hints

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.

l0rinc · 2025-07-21T19:58:48Z

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

l0rinc · 2025-07-28T20:42:00Z

Rebased after #32279 - updating the tests with the new convenience template helpers instead of using Solver for them. Ready for review again.

src/script/script.h

DrahtBot · 2025-08-05T02:39:18Z

🚧 At least one of the CI tasks failed.
_{Task MSan, depends: https://github.com/bitcoin/bitcoin/runs/47383324322}
_{LLM reason (✨ experimental): The CI failure is caused by a compilation error in script_ops.cpp: 'GetSigOpCount' is missing in 'CScript'.}

Hints

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.

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.

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

Previous `GetSigOpCount` method was overloaded to take either a `bool` or `scriptSig` as a parameter, without an explanation of when to call each overload. New `CountSigOps` method avoids the overloading and documents how it should be called. The name was chosen to be clearer and consistent with the newer `CountWitnessSigOps` function. Besides the renames, also added primitive argument name reminders to the call sites to highlight the meaning of the call. Co-authored-by: Ryan Ofsky <[email protected]>

The name `CountP2SHSigOps` was chosen to match `CountWitnessSigOps`, since the two functions are counterparts for handling `P2SH` and `SegWit` scripts. Also, it's called by `GetP2SHSigOpCount` in consensus, so the new name clarifies its significance. A goal of this change is to make the `CheckSigopsBIP54` function match the BIP54 specification which says "For each input in the transaction, count the number of CHECKSIG and CHECKMULTISIG in the input scriptSig and previous output's scriptPubKey, including the P2SH redeemScript." Co-authored-by: Ryan Ofsky <[email protected]>

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

Moved the implementation to the header (implicitly inline + noexcept), removed redundant `this` and parentheses and changed the `0x14`/`20` magic constants to descriptive `HASH160_OUTPUT_SIZE`. See: https://learnmeabitcoin.com/technical/script/p2sh/#scriptpubkey

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

Moved the implementation to the header (implicitly inline + noexcept), removed redundant `this` and parentheses.

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

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

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

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

l0rinc

Thanks, addresses remaining nits + fixed test opcode name iteration boundary failure: https://github.com/bitcoin/bitcoin/compare/cf8f476ef25e4fe43e67970ee2ec6a265d49e763..738b620f314780751c4db3af87c9f22792723125
(rebased in a separate push)

src/bench/checkblock.cpp

l0rinc · 2025-08-12T15:46:48Z

src/script/script.cpp

we don't introduce a header + implementation duplication - what would be the advantage in this case of separating them to header + impl?

2,3) done

src/script/script.h

src/test/compress_tests.cpp

src/test/sigopcount_tests.cpp

src/script/script.h

This template helper isn't used outside of tests yet, but for the sake of completeness - and since we're planning on using this for upcoming optimizations -, it's added as a last step. See: https://learnmeabitcoin.com/technical/script/p2wpkh/#scriptpubkey

* `CountSigOpsKnownTemplates` 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). * `CountSigOpsErrors` feeds malformed PUSHDATA sequences to confirm the parser never crashes and still counts sig-ops that appear before the error. * `GetOpName_no_missing_mnemonics` checks all opcode names by category. Co-authored-by: Hodlinator <[email protected]> Co-authored-by: Ryan Ofsky <[email protected]>

…validation Renames `HasValidOps()` to `HasValidBaseOps()` and `MAX_OPCODE` to `MAX_BASE_OPCODE` to clarify that pre-Taproot script validation should only accept opcodes up to OP_NOP10 (0xb9). This prevents Tapscript-only opcodes like OP_CHECKSIGADD (0xba) from being considered valid in scriptPubKey and scriptSig contexts, where only pre-Taproot opcodes should be allowed.

Add explicit test case checking that OP_CHECKSIGADD > MAX_BASE_OPCODE (i.e. that we have opcodes bigger than the max). The other changes in the test are just inlines to reduce scope. Also note that `OpCodeParser` deliberately skips `OP_CHECKSIGADD`, see: `script_parse_tests/parse_script`.

`OP_CHECKSIGADD` wasn't tested this way at all, but we should fuzz all possible values anyway.

ryanofsky

Code review ACK 3219b59. A lot of test improvements since last review, only comment and whitespace changes in non-test code. I think I'm caught up on the discussion now too

src/script/script.h

ryanofsky · 2025-08-13T12:04:15Z

src/script/script.cpp

re: #32729 (comment)

I prefer having them in the same place, @ryanofsky, where do you think these helpers belong, in header or cpp?

No real opinion, I just assumed these needed to be moved for the optimization in d76d753.

Abstractly, I think it makes sense for these to be in a header file since they are low-level and natural candidates to inline. Ideally I don't think they'd be in this header file for reasons hodlinator mentioned, and I would probably want them to be standalone functions not class methods since I think the purpose of classes is to encapsulate internal state and these aren't accessing anything private.

But I think anything is fine here, and it makes sense to go with your preference if there aren't objections.

hodlinator

ACK 3219b59

Renaming the sigop-counting functions while changing behavior of CScript::GetSigOpCount(const CScript& scriptSig)/CountP2SHSigOps(const CScript& scriptSig, const CScript& scriptPubKey) makes it relatively easy to verify the change, as no lingering GetSigOpCount overloads remain.

Renaming HasValidOps() -> HasValidBaseOps() makes one paranoid that we haven't been supporting Tapscript op codes everywhere, but the function is not used in those contexts.

I disagree with moving IsPayToTaproot() etc from .CPP to .H, but it's not blocking.

Stay humble and stack commits! :)
(Not advocating for merging commits, it is helpful to keep similar transforms applied to different areas in separate commits, other reviewers shouldn't be scared by the high count of them).

l0rinc changed the title ~~test,refactor: sigops~~ test: extract script template helpers & widen sigop count coverage Jun 11, 2025

DrahtBot added the Tests label Jun 11, 2025

l0rinc mentioned this pull request Jun 11, 2025

Short-circuit GetLegacySigOpCount for known scripts #32532

Closed

l0rinc changed the title ~~test: extract script template helpers & widen sigop count coverage~~ test,refactor: extract script template helpers & widen sigop count coverage Jun 11, 2025

l0rinc marked this pull request as ready for review June 11, 2025 18:21

DrahtBot mentioned this pull request Jun 11, 2025

clang-tidy: Apply modernize-deprecated-headers #32673

Merged

DrahtBot added the Needs rebase label Jun 11, 2025

l0rinc force-pushed the l0rinc/sigop-testing branch from 95b67f1 to 0582b63 Compare June 12, 2025 11:12

DrahtBot removed the Needs rebase label Jun 12, 2025

DrahtBot added the CI failed label Jul 21, 2025

l0rinc force-pushed the l0rinc/sigop-testing branch from 0582b63 to 5d29ce5 Compare July 21, 2025 19:57

DrahtBot removed the CI failed label Jul 22, 2025

l0rinc force-pushed the l0rinc/sigop-testing branch from 5d29ce5 to 6bf4c8d Compare July 28, 2025 20:40

DrahtBot mentioned this pull request Jul 30, 2025

[IBD] specialize block serialization #31868

Draft

ryanofsky reviewed Aug 5, 2025

View reviewed changes

src/script/script.h Outdated Show resolved Hide resolved

l0rinc force-pushed the l0rinc/sigop-testing branch 2 times, most recently from 1d49b3b to db4bf84 Compare August 5, 2025 02:25

DrahtBot added the CI failed label Aug 5, 2025

l0rinc force-pushed the l0rinc/sigop-testing branch from db4bf84 to f8208e9 Compare August 5, 2025 03:56

DrahtBot removed the CI failed label Aug 5, 2025

bitcoin deleted a comment from Thisdick69 Aug 12, 2025

l0rinc force-pushed the l0rinc/sigop-testing branch from cf8f476 to 738b620 Compare August 12, 2025 16:25

l0rinc and others added 12 commits August 12, 2025 09:25

bench: measure CheckBlock speed separately from serialization

593354c

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.

bench: measure SigOpsBlock speed separately

3173db1

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

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

f480a57

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

refactor: move IsPayToAnchor to header

a7c29fe

Moved the implementation to the header (implicitly inline + noexcept), removed redundant `this` and parentheses.

refactor: extract IsPayToPubKeyHash helper to script.h

b9e2955

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

l0rinc force-pushed the l0rinc/sigop-testing branch from 738b620 to 8ec20fb Compare August 12, 2025 16:25

l0rinc commented Aug 12, 2025

View reviewed changes

l0rinc and others added 5 commits August 12, 2025 15:19

fuzz: Extend ConsumeOpcodeType to cover all possible ops

3219b59

`OP_CHECKSIGADD` wasn't tested this way at all, but we should fuzz all possible values anyway.

l0rinc force-pushed the l0rinc/sigop-testing branch from 8ec20fb to 3219b59 Compare August 12, 2025 22:21

ryanofsky approved these changes Aug 13, 2025

View reviewed changes

DrahtBot requested a review from hodlinator August 13, 2025 12:12

hodlinator approved these changes Aug 13, 2025

View reviewed changes

DrahtBot mentioned this pull request Oct 24, 2025

refactor: optimize: avoid allocations in script & policy verification #33645

Open

DrahtBot mentioned this pull request Nov 1, 2025

refactor: make script Solver's often-unused solutions parameter optional #33757

Closed

DrahtBot mentioned this pull request Nov 8, 2025

test: Enhance GetTxSigOpCost tests for coinbase transactions #32840

Closed

This was referenced Jan 6, 2026

bench: Remove -priority-level= option #34210

Open

bench: add fluent API for untimed setup steps in nanobench #34208

Open

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

l0rinc commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Context

Structure

Uh oh!

DrahtBot commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

DrahtBot commented Jul 21, 2025

Uh oh!

l0rinc commented Jul 21, 2025

Uh oh!

l0rinc commented Jul 28, 2025

Uh oh!

Uh oh!

DrahtBot commented Aug 5, 2025

Uh oh!

l0rinc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

l0rinc Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

l0rinc commented Jun 11, 2025 •

edited

Loading

DrahtBot commented Jun 11, 2025 •

edited

Loading