script: prevent partial reinitialization of sighash data #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 initialization without spent outputs even when the BIP143 prevout, sequence, and output hashes were already cached. No current caller reuses an object across transactions, so validation and signing results are unchanged. btck_script_pubkey_verify() in the experimental kernel C API also copied caller-supplied txdata on each verification call.

Fix: Remove default construction and 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. The kernel verification path binds directly to caller-supplied txdata and constructs a local fallback only when needed.

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.

Type	Reviewers
Concept ACK	sedited

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:

#36122 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36122.svg"> (BIP460: CISA for Taproot key path spends by fjahr)
#35713 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35713.svg"> (Remove boost as a unit test runner by rustaceanrob)
#35569 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35569.svg"> (Encapsulation for CTransaction by purpleKarrot)
#32575 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/32575.svg"> (consensus: Remove special treatment for single threaded script checking by fjahr)
#29843 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/29843.svg"> (policy: Allow non-standard scripts with -acceptnonstdtxn=1 (test nets only) by ajtowns)
#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 typos and grammar issues:

and keep txsdata in scope -> and keep txdata in scope [typo in the variable name within the comment]

2026-10-01 16:19:55

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

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

in src/script/interpreter.h:197 in a5ac29bfb6

 197 | +    explicit PrecomputedTransactionData(const T& tx, std::vector<CTxOut>&& spent_outputs = {}, bool force = false);
 198 |  
 199 | +private:
 200 |      template <class T>
 201 | -    explicit PrecomputedTransactionData(const T& tx);
 202 | +    void Init(const T& tx, std::vector<CTxOut>&& spent_outputs, bool force);

sedited commented at 1:05 PM on September 22, 2026:

Why not remove it entirely? The diff from doing so is small.

l0rinc commented at 11:25 PM on September 22, 2026:

Nice, I thought that should be a follow-up, but it was actually trivial. I removed Init() entirely and moved its body into the constructor - beautiful!

sedited approved

sedited commented at 1:27 PM on September 22, 2026: contributor

ACK on the changes

I find the title a bit confusing. Which staleness are we exactly preventing with this change?

l0rinc renamed this:
~~script: prevent stale sighash caches across transactions~~
script: prevent reinitializing sighash data for another transaction
on Sep 22, 2026

l0rinc force-pushed on Sep 22, 2026

l0rinc commented at 11:59 PM on September 22, 2026: contributor

Updated the stack to remove the public Init() helper entirely and added a leading kernel API fix that avoids copying caller-supplied txdata on each verification call (had it in my backlog for a while, I could also push it separately if reviewers want). @theuni, this touches the txdata lifetime area involved in the script-check issue you found , would you mind reviewing the approach and letting me know if it has your blessing?

kernel: avoid copying caller-supplied txdata

In the experimental kernel C API, the conditional expression mixes the caller-supplied lvalue with a fallback prvalue. Its result is a prvalue that copies the supplied PrecomputedTransactionData on every verification call with supplied data.

Reproducer: https://godbolt.org/z/PzcKGW9da (GCC 15.2: one copy before, none after).

Keep optional storage only for the fallback and bind directly to emplace(). Both conditional arms are now lvalues, so a supplied object is reused while the fallback retains the same lifetime.

d07c727ff8

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.
The PSBT caller discards partial spent outputs when any input UTXO is missing, preserving its existing all-or-none precomputation.
Cover forced and witness-detected constructor readiness before removing the initialization API.

2e7c28a4ee

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 kernel creation API likewise gathers spent outputs before constructing and releasing its handle.
The signature-cache fuzz checker does not read txdata, so its empty-transaction construction avoids unused hashing.

26b3f43ff4

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.

1302a2ebc4

script: make txdata construction mandatory

Remove default construction and Init() so transaction-specific data is computed in the constructor.
Make the earlier compile-time constructor check reject default construction.
Existing callers already construct fresh data for each transaction, so validation and signing results are unchanged.

35a8964f88

l0rinc force-pushed on Oct 1, 2026

l0rinc renamed this:
~~script: prevent reinitializing sighash data for another transaction~~
script: prevent partial reinitialization of sighash data
on Oct 4, 2026