script: make txdata non-default-constructible #35662

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

Problem: PrecomputedTransactionData could be default-constructed and filled later with Init(). That made it possible to keep an object around without transaction-specific data, and the public Init() method left a reuse path that was easy to get wrong. In particular, the old Init() guard only checked m_spent_outputs_ready, which stayed false after forced initialization without spent outputs, while force=true still enabled BIP143 precomputation and left cached prevout, sequence, and output hashes ready for witness-v0 sighash reuse.

Fix: Make txdata construction do the initialization. Callers that already have the transaction and optional spent outputs now construct PrecomputedTransactionData directly; validation uses std::optional<PrecomputedTransactionData> so it can still wait until after a script-cache miss. The type is no longer default-constructible, public Init() is gone, and each object is built for one transaction. This hardens the helper against accidental reuse and simplifies the API by removing the default-construct-then-fill pattern. This also covers #35663: ConnectBlock() leaves txdata storage empty when script checks are skipped during assumevalid.

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:

#35587 (Remove boost as a unit test runner by rustaceanrob)
#35501 (wallet: store all witness variants of a transaction by achow101)
#34875 (refactor: separate deferred script check collection from CheckInputScripts by l0rinc)
#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)

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(...) in src/test/txvalidationcache_tests.cpp
CheckInputScripts(...) in src/test/txvalidationcache_tests.cpp
CheckInputScripts(...) 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

script: construct txdata at simple call sites

Generalize the transaction-only constructor to accept spent outputs and a `force` flag, forwarding to the existing `Init()` implementation, and use it at call sites where those inputs are already available.
The txdata initialization contract remains unchanged in this commit.

a65853daeb

script: construct txdata at remaining eager callers

`SignTransaction()` can include spent outputs in txdata only if every input coin is available.
Clear the partially collected outputs as soon as an input coin is missing, and construct `PrecomputedTransactionData` once after the lookup.
This preserves forced precomputation and removes default construction from the signing path.

Kernel API creation already receives the transaction and any supplied spent outputs before returning a handle, so construct `PrecomputedTransactionData` directly.
The `script_sigcache` fuzz target never reads txdata in the exercised checker paths, so construct it from an empty transaction to keep per-iteration work unchanged.

b9bf725181

validation: defer txdata construction with optional

Validation used default construction so script-cache hits could return before gathering spent outputs or precomputing transaction data.
During IBD and reindex, `ConnectBlock()` can also skip script checks entirely for blocks in the assumevalid period, leaving its per-block txdata vector unused.

Keep this lazy behavior at the caller by storing `std::optional<PrecomputedTransactionData>`, emplacing it only after a cache miss, and sizing `ConnectBlock()` txdata storage only when script checks can run.
This prepares the type to require transaction data at construction.

48ffc45096

script: make txdata construction mandatory

`PrecomputedTransactionData` no longer has callers that need default construction or public `Init()`.

Remove both APIs so every constructed txdata object is initialized for a transaction.
Keep the shared implementation in a private `Init()` helper and cover constructor states with focused sighash tests.

92470f76bd

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