script: prevent stale sighash caches across transactions #35662

l0rinc commented at 1:58 AM on July 6, 2026: contributor

Problem: PrecomputedTransactionData could be default-constructed and filled later with Init(). That allowed an object to exist without transaction-specific data and left a public reuse path that was easy to get wrong. The old Init() guard only checked m_spent_outputs_ready, which remained false after forced initialization without spent outputs even though force=true cached the BIP143 prevout, sequence, and output hashes for witness-v0 sighash.

Fix: Remove default construction and public Init() so each PrecomputedTransactionData object is built for one transaction. Callers that already have the transaction and optional spent outputs construct it directly. Validation stores txdata in std::optional and emplaces it only after a script-cache miss, keeping precomputation lazy. ConnectBlock() also leaves txdata storage empty when assumevalid skips script checks, covering #35663.

DrahtBot added the label Consensus on Jul 6, 2026

DrahtBot commented at 1:59 AM on July 6, 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/35662.

Reviews

See the guideline and AI policy for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35713 (Remove boost as a unit test runner by rustaceanrob)
#35569 (Encapsulation for CTransaction by purpleKarrot)
#35511 (RFC: consensus: Make CAmount a class by hodlinator)
#32575 (consensus: Remove special treatment for single threaded script checking by fjahr)
#29843 (policy: Allow non-standard scripts with -acceptnonstdtxn=1 (test nets only) by ajtowns)
#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.

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

CheckInputScripts(CTransaction(spend_tx), ..., true, true, ...) in src/test/txvalidationcache_tests.cpp
CheckInputScripts(CTransaction(invalid_with_cltv_tx), ..., true, true, ...) in src/test/txvalidationcache_tests.cpp
CheckInputScripts(CTransaction(invalid_with_csv_tx), ..., true, true, ...) in src/test/txvalidationcache_tests.cpp
CheckInputScripts(CTransaction(tx), ..., true, true, ...) in src/test/txvalidationcache_tests.cpp

in src/script/interpreter.cpp:1415 in 1279d5d95c

1411 | @@ -1412,7 +1412,10 @@ uint256 GetSpentScriptsSHA256(const std::vector<CTxOut>& outputs_spent)
1412 |  template <class T>
1413 |  void PrecomputedTransactionData::Init(const T& txTo, std::vector<CTxOut>&& spent_outputs, bool force)
1414 |  {
1415 | -    assert(!m_spent_outputs_ready);

sedited commented at 8:41 AM on July 8, 2026:

I'm tending NACK here. Afaict this is just a hardening improvement, but I'm not sure if it really is one. We should never be hitting a code path where a re-initialization is possible, and this seems to relax that requirement. I think rather than messing with Init, we should seriously reconsider whether this data can be initialized at construction.

l0rinc commented at 12:51 AM on July 9, 2026:

I agree that resetting readiness flags inside Init() still leaves a reuse API around and makes the type responsible for defending against misuse after construction (I was hoping someone would object).

I’ll rework this so that PrecomputedTransactionData is always built for one transaction and has no public reinitialization path:

Call sites that already have the transaction and spent outputs can construct it directly.
Call sites that need to wait, such as validation before the script execution cache check, can store std::optional<PrecomputedTransactionData> and emplace() once all required data has been gathered.

This also seems like a natural place to fold in #35663: validation already needs optional txdata storage, so the assumevalid path can leave the per-block txdata vector empty when script checks are skipped, while still resizing it before any queued checks can take pointers into it.

l0rinc commented at 6:05 AM on July 9, 2026:

Pushed: PrecomputedTransactionData is no longer default-constructible, public Init() is gone, and each object is built for one transaction. ConnectBlock() also leaves txdata storage empty when script checks are skipped during assumevalid, so that path avoids the unused vector allocation. This is indeed a lot nicer, thanks for the push. What do you think?

l0rinc marked this as a draft on Jul 8, 2026

l0rinc force-pushed on Jul 9, 2026

l0rinc renamed this:
~~script: reset precomputed txdata on reuse~~
script: construct txdata at initialization sites
on Jul 9, 2026

l0rinc renamed this:
~~script: construct txdata at initialization sites~~
script: remove deferred txdata initialization
on Jul 9, 2026

l0rinc renamed this:
~~script: remove deferred txdata initialization~~
script: make txdata non-default-constructible
on Jul 9, 2026

DrahtBot added the label CI failed on Jul 9, 2026

l0rinc force-pushed on Jul 9, 2026

l0rinc marked this as ready for review on Jul 9, 2026

DrahtBot removed the label CI failed on Jul 9, 2026

DrahtBot added the label Needs rebase on Aug 4, 2026

script: construct txdata at simple call sites

Extend the transaction-only constructor to accept spent outputs and a `force` flag, then use it where those inputs are already available.
This preserves each caller's precomputation behavior while removing default construction from the simple cases.

3dd1970831

script: construct txdata at other eager callers

`SignTransaction()` can provide spent outputs only when every input coin is available, so discard a partial collection when one is missing and construct txdata once with forced precomputation.
The `script_sigcache` fuzz target exercises signature-cache methods that do not read txdata, so construct it from an empty transaction to avoid per-iteration hashing.

df7ed36b04

validation: defer txdata construction

Script-cache hits return before validation gathers spent outputs or precomputes transaction data.
Store txdata in `std::optional` and emplace it only after a cache miss to preserve that behavior.
`ConnectBlock()` can skip script checks for blocks in the assumevalid period, so size its txdata storage only when checks can run.

d31ab5f47f

script: make txdata construction mandatory

Remove default construction and public `Init()` so each `PrecomputedTransactionData` object is built for one transaction.
Keep `Init()` as the private shared implementation and cover the supported constructor states.

a5ac29bfb6

l0rinc force-pushed on Aug 5, 2026

l0rinc renamed this:
~~script: make txdata non-default-constructible~~
script: prevent stale sighash caches across transactions
on Aug 5, 2026

l0rinc commented at 10:29 PM on August 5, 2026: contributor

Rebased after recent merges, ready for review again. I understood @theuni has a similar attempt before, if it's published somewhere, please let me know and I'll add him as a coauthor.

DrahtBot removed the label Needs rebase on Aug 5, 2026