validation: avoid preallocating unused txdata during assumevalid #35663

pull l0rinc wants to merge 1 commits into bitcoin:master from l0rinc:l0rinc/validation-skip-txdata-precompute changing 1 files +5 −2
  1. l0rinc commented at 2:43 AM on July 6, 2026: contributor

    Problem: During IBD/reindex, ConnectBlock() skips script verification for blocks in the assumevalid period. However, it still performs one heap allocation for a fresh txsdata vector and default-constructs one PrecomputedTransactionData entry for every transaction in each connected block. Because CheckInputScripts() is not called in that path, that work is unused and only adds allocator churn and avoidable heap fragmentation on large blocks.

    Fix: Leave txsdata empty unless fScriptChecks is true, then resize it before queued checks can take pointers into the vector. Update the lifetime comment and add an indexed-use assert that matches the real precondition.

    Reproducer: On a synthetic ~7000-transaction block, the skipped-check path avoids the unused heap allocation and saves ~2MB memory per block.

    <details><summary>ConnectBlockAllSchnorr mem usage: 7000 txs, num_schnorr=1, num_ecdsa=0, fScriptChecks=false</summary>

    for C in 1835f2fcbf482035e1c11c72ae39cee4e24e4c00 832dc594f72bb6e8415674dce434684202073b12; do
      git reset --hard -q && git checkout -q "$C"
      perl -0pi -e 's/(num_txs = )1000/${1}7000/; s@(/\*num_schnorr=\*/)\d+, (/\*num_ecdsa=\*/)\d+@${1}1, ${2}0@; s/(fScriptChecks)\{[^}]+\};/${1}{false};/;' src/bench/connectblock.cpp src/validation.cpp
      cmake -B build -DBUILD_BENCH=ON >/dev/null 2>&1 && cmake --build build --target bench_bitcoin >/dev/null 2>&1
      /usr/bin/time -v build/bin/bench_bitcoin -filter=ConnectBlockAllSchnorr -sanity-check 2>&1 >/dev/null | grep 'Maximum resident'
    done
    

    1835f2fcbf ... Maximum resident set size: 58704 kB 832dc594f7 ... Maximum resident set size: 56664 kB

    </details>

  2. validation: avoid preallocating txsdata when checks are skipped
    During IBD and reindex, `ConnectBlock()` skips script verification for blocks in the `assumevalid` period, but still performs one heap allocation for a fresh `txsdata` vector and default-constructs one `PrecomputedTransactionData` entry for every transaction in each connected block.
    Because `CheckInputScripts()` is not called in that path, that work is unused and only adds allocator churn and avoidable heap fragmentation on large blocks.
    
    Leave `txsdata` empty unless `fScriptChecks` is true, then resize it before queued checks can take pointers into the vector.
    Keep `txsdata` declared before `control` so queued script checks finish before the storage is destroyed.
    6bcd003525
  3. DrahtBot added the label Validation on Jul 6, 2026
  4. DrahtBot commented at 2:44 AM on July 6, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35663.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #32575 (consensus: Remove special treatment for single threaded script checking 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.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  5. darosior commented at 2:17 PM on July 7, 2026: member

    What is the big picture benefit of this change? On its face it seems unnecessarily risky especially as we just had a high-severity vulnerability related to this very data structure that none of all our eyeballs noticed for a decade.

  6. l0rinc commented at 7:05 PM on July 7, 2026: contributor

    We're in agreement: while this is a tiny change with a modest effect, we have to be absolutely sure the risk is low. I would argue that this may be exactly the right time to touch it, while the behavior is still fresh in our heads.

    txsdata only needs storage when CheckInputScripts() can be called. When it can be called, the vector must already be fully sized before any pointers into it are taken, and it must outlive control. This PR tries to encode that invariant more directly.

    If you think there are specific failure modes or missing coverage around this data structure, pointers would be appreciated. I would rather add coverage now, while we are looking at this area carefully, than leave it alone because it has historically been hard to reason about. It could help us encode our current understanding to make sure we're not afraid of it in the future.

  7. l0rinc commented at 12:31 AM on July 9, 2026: contributor
  8. l0rinc closed this on Jul 9, 2026


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-07-09 06:47 UTC