script: clarify implicit signature-cache input requirements #36395

l0rinc commented at 10:39 PM on September 30, 2026: contributor

Problem: Signature-cache hashing relies on input requirements that are implicit at its public entry methods. Inspired by the Elements rangeproof-cache vulnerability behind the recent Liquid incident and the subsequent cache-key fix, this change clarifies an easy-to-misread hashing pattern: Write(data, size) hashes the data bytes without encoding the size. Bitcoin Core's cache keys already include the signature hash, public key, and signature in separate ECDSA and Schnorr domains. Bitcoin Core's signature-cache encoding is unambiguous for inputs accepted by its current script checker: a valid ECDSA public key's prefix determines its length, while a Schnorr public key is always 32 bytes, leaving the signature as the remaining suffix.

Fix: Extract the common hashing into one typed helper so changes to the ECDSA and Schnorr layouts stay synchronized. Return the digest by value from the helper and public ComputeEntry* methods so callers explicitly initialize the cache entry, where the previous chained Finalize(entry.begin()) obscured the output assignment. Document the existing layout and assert the valid ECDSA public-key encoding and 64-byte Schnorr signature requirements. The serialized bytes and script validity rules are unchanged.

test: precompute script transaction data

Precompute transaction data for the existing script vectors so plain and caching verification can share the same immutable transactions.
Use `uint32_t` for BIP341 input positions to match the checker interface.

74c70daf6b

test: compare plain and cached verification

Require caching to preserve the existing script vectors' results and errors and the fuzz inputs' verification results.
Repeat script verification after successful signatures are stored, and alternate plain and caching checkers under random flag combinations.
Allow empty ECDSA signatures, which can remain after removing the sighash byte, and require valid public-key encodings for direct calls that bypass the script checker.

1339ded930

DrahtBot added the label Consensus on Sep 30, 2026

DrahtBot commented at 10:39 PM on September 30, 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/36395.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	instagibbs

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36409 (sign: use fresh randomness as BIP340 auxiliary data by fametrano)
#36091 (test: Add debug output to common tested types by rustaceanrob)
#35744 (coins: prevent DB resize from invalidating cursors by l0rinc)
#35569 (Encapsulation for CTransaction by purpleKarrot)
#30342 (kernel, logging: Deliver each context's log output to its own logging connection by ryanofsky)
#29491 ([EXPERIMENTAL] Schnorr batch verification for blocks by fjahr)

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 added the label CI failed on Sep 30, 2026

DrahtBot commented at 11:41 PM on September 30, 2026: contributor

🚧 At least one of the CI tasks failed. <sub>Task lint: https://github.com/bitcoin/bitcoin/actions/runs/36786811940/job/110130078415</sub> <sub>LLM reason (✨ experimental): CI failed due to lint-includes.py detecting a newly introduced Boost include (boost/test/tools/context.hpp) in src/test/key_tests.cpp.</sub>

<details><summary>Hints</summary>

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.

</details>

test: cover public-key encoding lengths

Extend `pubkey_unserialize` to check every header and lengths through one byte beyond the largest key.
Require construction and deserialization to agree, and round-trip each resulting key without unread bytes.

25156225ce

test: cover malformed ECDSA public keys

Extend an existing lax-encoding vector with a non-empty, wrong-length public key to exercise rejection by the script checker.

498bf7d2c9

test: cover ECDSA signatures padded past 255 bytes

Lax DER verification accepts trailing data, including signatures longer than 255 bytes.
Add a padded P2PK vector to exercise that accepted input through both plain and caching verification.

278154f849

test: cover Schnorr cache inputs

Extend the BIP341 vectors to check insertion, non-storing hits, entries that distinguish public keys and sighashes, and rejection without caching of a correctly sized invalid signature.
Cover both 64- and 65-byte script signatures and the exact errors for malformed sizes and an explicit default sighash byte.

92cb7affd5

refactor: return signature-cache entries by value

Return entries through the public `ComputeEntry*` methods so verification sites explicitly initialize them.
This replaces output parameters whose assignment was hidden in the chained `Finalize(entry.begin())`.
The hashing procedure and serialized bytes are unchanged.

670ce0d7a5

refactor: extract and deduplicate sigcache hashing

Extract the common hashing so ECDSA and Schnorr serialization changes stay synchronized.
Constrain `PubKey` to `CPubKey` or `XOnlyPubKey` so swapping the signature and public-key arguments cannot compile.
The salted hash states, field order, and serialized bytes are unchanged.

4d5c4d36e9

script: assert signature-cache input contracts

Assert and document the script checker's input guarantees before hashing cache entries, including direct calls to the caching verifier.
`ComputeEntrySchnorr` is also called directly, so it enforces the 64-byte contract independently of the assertion in `XOnlyPubKey::VerifySchnorr`.

c8d6a97afb

l0rinc force-pushed on Oct 1, 2026

l0rinc closed this on Oct 1, 2026

l0rinc reopened this on Oct 1, 2026

DrahtBot removed the label CI failed on Oct 1, 2026

instagibbs commented at 8:03 PM on October 1, 2026: member

concept ACK on the tests, concept NACKy on the refactors due to blast radius. Will review the tests closer if split out

fametrano commented at 9:48 PM on October 1, 2026: contributor

I ran the two script_tests.json rows this adds or changes, at c8d6a97afb366f82680601713942afb4955c2885, through btclib's script interpreter, and both give the expected result. I haven't reviewed the sigcache changes.

l0rinc commented at 3:38 AM on October 2, 2026: contributor

concept NACKy on the refactors due to blast radius

The refactor is the meat of this change, the original error would still be possible even if we only add tests, and I want to make sure we learn from those mistakes and refactor to make such misunderstandings harder. If we're afraid of touching a part of the code, it means it's not ours yet, and it's probably where the ugly bugs are buried.