txorphanage: account memory usage instead of weight #36015

pull brunoerg wants to merge 7 commits into bitcoin:master from brunoerg:2026-08-orphanage-mem-bug changing 8 files +637 −105
  1. brunoerg commented at 9:11 PM on August 18, 2026: contributor

    The orphanage limits the "usage" of the orphans it stores, per peer and globally, to bound the amount of memory an attacker can make us hold on to. Weight was used as a proxy for that memory, on the assumption that it is "often higher than the actual memory usage of the transaction".

    That assumption does not hold. Every witness stack element is an individually heap-allocated vector, costing its 24-byte slot in the stack vector plus a 32-byte minimum allocation, while only weighing 2WU. A transaction of 199,000 1-byte witness elements weighs 398,247WU (i.e. it is a standard weight, and witness standardness cannot be checked while the inputs are missing), but uses 11.1MB of memory: 28 times what is accounted for it.

    Account the memory the transaction actually uses instead, and derive the per-peer reservation from the largest orphan we are willing to store, so that a peer relaying one maximally-sized orphan still stays within its allowance (and therefore cannot cause any evictions). Orphans that would not fit in that allowance are now refused by AddTx() rather than stored and immediately evicted again by LimitOrphans(), which would first push out the announcer's other orphans: with weight accounting, a peer that had 10 normal orphans and relayed one maximally-sized one kept 9 of them; accounting memory without the new limit would leave it with none.

  2. DrahtBot commented at 9:11 PM on August 18, 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/36015.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline and AI policy for information on the review process.

    Type Reviewers
    Concept ACK w0xlt, l0rinc

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #35569 (Encapsulation for CTransaction by purpleKarrot)

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

    LLM Linter (✨ experimental)

    Possible typos and grammar issues:

    • "...and is thus remains in the orphanage until the end of LimitOrphans." -> "...and thus remains in the orphanage until the end of LimitOrphans." [“is thus remains” is grammatically broken]

    <sup>2026-08-19 00:09:16</sup>

  3. w0xlt commented at 9:15 PM on August 18, 2026: contributor

    Concept ACK.

  4. brunoerg marked this as a draft on Aug 18, 2026
  5. DrahtBot added the label CI failed on Aug 18, 2026
  6. DrahtBot commented at 10:42 PM on August 18, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task Windows native, fuzz, VS: https://github.com/bitcoin/bitcoin/actions/runs/32186423022/job/95870987922</sub> <sub>LLM reason (✨ experimental): CI failed due to an assertion failure in the fuzz test txorphan (txorphan.cpp:182), causing the fuzzer to exit with code 1.</sub>

    <details><summary>Hints</summary>

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

    </details>

  7. l0rinc commented at 11:08 PM on August 18, 2026: contributor

    Concept ACK The failing orphanage tests seem related (the musl one is the new test, the fuzz ones need a rebase, iwyu is the bench) and the change mixes refactors and hardening - can you check if it's possible to simplify and do refactors that aren't strictly related in follow-ups instead? Especially since #35923 is related and complements this change, and #35919 is also touching the same area.

  8. brunoerg force-pushed on Aug 18, 2026
  9. brunoerg commented at 11:22 PM on August 18, 2026: contributor

    The failing orphanage tests seem related (the musl one is the new test, the fuzz ones need a rebase, iwyu is the bench) and the change mixes refactors and hardening - can you check if it's possible to simplify and do refactors that aren't strictly related in follow-ups instead?

    My bad on that, forgot to push since my latest local change. Just did it.

  10. iwyu: fix includes in node/txorphanage.h
    Drop includes the header does not use, add the ones it does, and forward
    declare FastRandomContext. The IWYU job checks the files a change touches,
    and the following commits modify this header.
    c1977fea67
  11. scripted-diff: rename DEFAULT_RESERVED_ORPHAN_WEIGHT_PER_PEER
    This constant is the default for TxOrphanage::m_reserved_usage_per_peer,
    returned by ReservedPeerUsage() and compared against UsageByPeer(), so
    name it after the quantity it limits rather than after the metric that
    happens to be used to measure it. A following commit changes that metric
    from weight to memory usage.
    
    -BEGIN VERIFY SCRIPT-
    sed -i 's/DEFAULT_RESERVED_ORPHAN_WEIGHT_PER_PEER/DEFAULT_RESERVED_ORPHAN_USAGE_PER_PEER/g' $(git grep -l DEFAULT_RESERVED_ORPHAN_WEIGHT_PER_PEER)
    -END VERIFY SCRIPT-
    c2cfebcf38
  12. txorphanage: add GetOrphanUsage() and cache it per announcement
    The orphanage's notion of an orphan's "usage" is currently duplicated in
    the unit tests, the benchmarks and the fuzz targets, all of which call
    GetTransactionWeight() to predict what the orphanage will account. Move
    that knowledge into a single function so that a change of metric only
    has to happen in one place, and so that all users agree on it.
    
    Also cache the value in the Announcement instead of recomputing it on
    every operation. Announcements hold an immutable CTransactionRef, so the
    value never changes; caching it guarantees that PeerDoSInfo::Add() and
    ::Subtract() always use the same number, and makes Erase() (and therefore
    LimitOrphans()) independent of the transaction's size.
    
    No behavior change.
    80d84c8d09
  13. txorphanage: account memory usage instead of weight
    The orphanage limits the "usage" of the orphans it stores, per peer and
    globally, to bound the amount of memory an attacker can make us hold on
    to. Weight was used as a proxy for that memory, on the assumption that it
    is "often higher than the actual memory usage of the transaction".
    
    That assumption does not hold. Every witness stack element is an
    individually heap-allocated vector, costing its 24-byte slot in the stack
    vector plus a 32-byte minimum allocation, while only weighing 2WU. A
    transaction of 199,000 1-byte witness elements weighs 398,247WU (i.e. it
    is a standard weight, and witness standardness cannot be checked while
    the inputs are missing), but uses 11.1MB of memory: 28 times what is
    accounted for it.
    
    Account the memory the transaction actually uses instead, and derive the
    per-peer reservation from the largest orphan we are willing to store, so
    that a peer relaying one maximally-sized orphan still stays within its
    allowance (and therefore cannot cause any evictions). Orphans that would
    not fit in that allowance are now refused by AddTx() rather than stored
    and immediately evicted again by LimitOrphans(), which would first push
    out the announcer's other orphans: with weight accounting, a peer that
    had 10 normal orphans and relayed one maximally-sized one kept 9 of them;
    accounting memory without the new limit would leave it with none.
    9486c1bbf8
  14. test: cover the orphanage's memory accounting
    Add unit tests for what changed with accounting memory instead of weight:
    
    - orphan_usage_limits: an orphan's contribution to the peer's usage is its
      memory usage; transactions that are small by weight but memory-dense do
      fill up a peer's allowance; the largest storable orphan fits within a
      peer's reserved usage (alongside normal orphans) and one witness element
      more is refused.
    - witness_heavy_orphan_tx: a standard-weight transaction consisting of
      199,000 1-byte witness elements uses over 10 times its weight in memory
      and is not stored, neither on its own (leaving the announcer's other
      orphans in place) nor by announcing one per peer.
    - large_standard_orphan_tx: transactions of maximum standard weight with
      realistically-shaped witnesses are still stored, and do not evict the
      announcer's other orphans.
    85b397a613
  15. test: add p2p functional test for orphanage memory accounting
    Check over p2p that memory usage and not weight is what fills up a peer's
    allowance (three memory-dense but featherweight orphans fit, the fourth
    evicts the oldest), that an orphan using more memory than allowed is not
    stored (neither on its own, without disturbing the announcer's other
    orphans, nor once per connection), and that an orphan of maximum standard
    weight is still stored.
    
    Without accounting memory, the first of these keeps all four orphans and
    the second keeps a ~1MB orphan per connection in the orphanage forever.
    59a602cff8
  16. test: document why the maximal package is protected in the orphanage
    test_maximal_package_protected() builds an ancestor package of the maximum
    allowed 404,000WU and relies on it fitting inside what the orphanage
    reserves for a peer, which is what keeps it from being evicted while other
    peers spam. Now that the reservation is an amount of memory rather than of
    weight, that is no longer apparent from the package's weight.
    d48b8afcae
  17. brunoerg force-pushed on Aug 19, 2026
  18. DrahtBot removed the label CI failed on Aug 19, 2026
  19. brunoerg marked this as ready for review on Aug 19, 2026
  20. brunoerg commented at 11:33 AM on August 19, 2026: contributor

    Ready for review.


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-08-21 04:51 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me