script: remove deferred txdata initialization #35662

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

Problem: PrecomputedTransactionData::Init() can be called again after a forced initialization with no spent outputs, because the old code guarded only m_spent_outputs_ready and set that flag only when spent outputs were present, while force=true enabled BIP143 precomputation and set m_bip143_segwit_ready. If the transaction changes before the next Init(), the stale BIP143 readiness flag can make witness-v0 SignatureHash() use cached prevout, sequence, and output hashes from the previous transaction.

Current validation, mempool, wallet, and signing paths initialize fresh PrecomputedTransactionData objects for each transaction, so their behavior stays the same.

Fix: Harden the helper for external callers and future code that reuses txdata objects by resetting optional readiness flags at the start of Init() before recomputing the current transaction state.

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

txsdata -> txdata [typo in the comment; the intended variable name appears to be txdata]

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), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG, true, true, ptd_spend_tx, m_node.chainman->m_validation_cache, nullptr) in src/test/txvalidationcache_tests.cpp
CheckInputScripts(CTransaction(invalid_with_cltv_tx), state, m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY, true, true, txdata, m_node.chainman->m_validation_cache, nullptr) in src/test/txvalidationcache_tests.cpp
CheckInputScripts(CTransaction(invalid_with_csv_tx), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_CHECKSEQUENCEVERIFY, true, true, txdata, m_node.chainman->m_validation_cache, nullptr) in src/test/txvalidationcache_tests.cpp
CheckInputScripts(CTransaction(tx), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, true, true, txdata, m_node.chainman->m_validation_cache, nullptr) 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 marked this as a draft on Jul 8, 2026

script: construct txdata at simple call sites

Generalize the transaction-only constructor to also take optional spent outputs and a force flag, forwarding to the existing `Init()` implementation, and use it at call sites where those inputs are already available.
This changes construction spelling without changing the txdata initialization contract yet.

4af5f6ce30

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 the forced-precompute behavior and removes default construction from the signing path.

Kernel API creation already receives the transaction and optional spent outputs before returning a handle, so construct `PrecomputedTransactionData` directly with whatever spent outputs were provided.
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.

ad372bda1d

validation: defer txdata allocation with optional

Validation used default construction so script-cache hits could return before spent outputs were gathered and precomputed.
During IBD and reindex, `ConnectBlock()` can also skip script checks entirely for blocks in the assumevalid period, so its per-block txdata vector was unused in that path.

Keep that lazy behavior at the caller boundary 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 txdata type itself to require construction with transaction data.

72b89cbbff

script: make txdata construction mandatory

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

Remove that surface so constructed txdata is always initialized for a transaction.
Keep the shared implementation in a private `Init()` helper, and pin the constructor contract with focused sighash test coverage.

586620e387

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