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: Use native HashWriter serialization in the typed ComputeEntry* methods so ECDSA public keys and signature vectors include their CompactSize lengths. Schnorr retains its fixed-size public-key encoding and raw signature span. Return the digest by value from the public ComputeEntry* methods so callers explicitly initialize the cache entry, where the previous chained Finalize(entry.begin()) obscured the output assignment. Keep the prepared salt prefixes private and const, and copy them locally for each entry. Retain the ECDSA salt padding and drop the Schnorr salt padding. Add a nonce-taking constructor so tests can reproduce the encoding deterministically. The process-local cache-entry bytes change, while script validity rules are unchanged.

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.

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36395
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36395.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36395#author-ralph

Reviews

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

Type	Reviewers
Concept ACK	instagibbs, fametrano

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:

#36721 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36721.svg"> (script: store interpreter stack elements as reference-counted objects by conduition)
#36188 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36188.svg"> (crypto: plug hardware optimized SHA256 into libsecp256k1's context by furszy)
#29491 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/29491.svg"> ([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.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

src/test/script_tests.cpp: TestBuilder(..., "P2PK with signature padded past 255 bytes, without DERSIG", 0)

2026-11-07 20:22:47

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. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/36786811940/job/110130078415 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.

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

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.

fametrano commented at 11:42 AM on October 25, 2026: contributor

Concept ACK

The new tests catch five signature-cache bugs that the current unit tests miss. I made seven one-line breaking changes to sigcache.cpp and ran the unit tests on c8d6a97afb36 and on its base, d4b0e1e4b0.

On the base, the unit tests still pass with six of the seven changes. Only storing failed Schnorr checks is caught, by txvalidationcache_tests. On this PR, script_tests catches five of those six: dropping the public key or the sighash from the ECDSA or the Schnorr entry, and storing failed ECDSA checks. The cached passes in DoTest catch the ECDSA ones, and the new BIP341 vector checks catch the Schnorr ones.

The sixth change gives Schnorr entries the ECDSA salt, and no unit test catches it on either commit. The separate salts are all that keeps the two domains apart: with one salt, the ECDSA entry for a 33-byte key and a 63-byte signature equals the Schnorr entry for the key's first 32 bytes and a signature made of the key's last byte followed by those 63 bytes. A test that builds both entries with ComputeEntryECDSA and ComputeEntrySchnorr and checks that they differ passes on this PR and fails with that change. It could go in this PR; I can share it, and the seven changes.

DrahtBot added the label Needs rebase on Nov 2, 2026

l0rinc force-pushed on Nov 5, 2026

DrahtBot removed the label Needs rebase on Nov 5, 2026

l0rinc commented at 3:39 PM on November 5, 2026: contributor

Rebased and replaced the hashing helper and cache-entry assertions with native serialization (with explicit size commitment), following the Liquid fix and @instagibbs's suggestion. Made the salt prefixes const, removed the padding, and added tests that check the encoded bytes and make sure failed signature checks aren't cached.

test: precompute script transaction data

Precompute transaction data for the existing script vectors so plain and caching verification can share the same immutable transactions.

ba5ef05fb9

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.
Repeat fuzz verification to detect caching of failed checks.

5b9cc7a490

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.

9378db6f0b

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.

d0204b07d0

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.

dcf5ad544f

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.

fbfca28a41

test: cover Schnorr cache inputs

Extend the BIP341 key-path vector test 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.
Use `uint32_t` for the checker index to avoid narrowing in brace initialization, and require successful signing before taking a 64-byte signature span.

8696231914

script: prepare immutable signature-cache prefixes

Make both prepared hash prefixes const `HashWriter` members and copy them inside `ComputeEntryECDSA` and `ComputeEntrySchnorr` before hashing an entry.
Serialize the nonce followed by a one-byte signature-type tag.
Keep 31 zero bytes after the ECDSA tag so the first SHA256 block is compressed once during construction.
With it, 70-72-byte ECDSA signatures need three compressions per entry for both compressed and uncompressed keys, while unpadded uncompressed keys would need four.
Drop the Schnorr padding, since Schnorr still needs three compressions and was slightly faster in local ARM SHA256 measurements.
The cache is process-local, so the changed entry bytes have no persisted compatibility effect.
Add a nonce-taking constructor so encoding tests do not depend on global RNG sequencing.
Characterize the raw field encoding in these public methods before the following native-serialization change.

Suggested by Yudis-bit: https://github.com/bitcoin/bitcoin/pull/36395#discussion_r4499982215

Co-authored-by: Yudis-bit <Yudis-bit@proton.me>

79d82f745f

script: serialize signature-cache inputs

Use HashWriter's native serialization for the public key and signature, following the Elements proof-cache change (https://github.com/ElementsProject/elements/pull/1600).
ECDSA public keys and signature vectors include their CompactSize lengths, while Schnorr retains its fixed-size public-key encoding and raw signature span.
The cache no longer relies on the ECDSA key's header byte to delimit the signature.

a7c23f4278

in src/script/sigcache.cpp:25 in 7c2e89de53

  34 | -    m_salted_hasher_schnorr.Write(nonce.begin(), 32);
  35 | -    m_salted_hasher_schnorr.Write(PADDING_SCHNORR, 32);
  36 | +SignatureCache::SignatureCache(const size_t max_size_bytes) : SignatureCache{max_size_bytes, GetRandHash()} {}
  37 |  
  38 | +SignatureCache::SignatureCache(const size_t max_size_bytes, const uint256& nonce)
  39 | +    : m_salted_hasher_ecdsa{HashWriter{} << nonce << uint8_t{'E'}},

Yudis-bit commented at 4:00 AM on November 7, 2026:

The padding in master was there to fill a 64-byte chunk so the compression ran once at startup. With 33 bytes, the constructor leaves 33 bytes in the buffer and defers that transform into ComputeEntry.

For 65-byte uncompressed keys this also crosses the 192-byte mark, needing 4 transforms instead of 3.

Padding 31 zero bytes keeps the HashWriter changes and the precomputed chunk:

HashWriter{} << nonce << uint8_t{'E'} << std::array<uint8_t, 31>{}

l0rinc commented at 8:25 PM on November 7, 2026:

Indeed, while Schnorr measured slightly faster without it (must be some alignment magic), there is an extra cost for uncompressed ECDSA keys: I restored the padding for ECDSA and added you as coauthor, thanks.

l0rinc force-pushed on Nov 7, 2026