mining: add getMemoryLoad() and track template non-mempool memory footprint

[Sjors]

Implements a way to track the memory footprint of all non-mempool transactions that are still being referenced by block templates, see discussion in #33899. It does not impose a limit.

IPC clients can query this footprint (total, across all clients) using the getMemoryLoad() IPC method. Its client-side usage is demonstrated here:

• Log getMemoryLoad() stratum-mining/sv2-tp#63

Additionally, the functional test in interface_ipc.py is expanded to demonstrate how template memory management works: templates are not released until the client drops references to them, or calls the template destroy method, or disconnects. The destroy method is called automatically by clients using libmultiprocess, as sv2-tp does. In the Python tests it also happens when references are destroyed or go out of scope.

The PR starts with preparation refactor commits:

1. Tweaks interface_ipc.py so destroy() calls happen in an order that's useful to later demonstrate memory management
2. Change std::unique_ptr<BlockTemplate> block_template from a static defined in rpc/mining.cpp to NodeContext. This prevents a crash when we switch to a non-trivial destructor later (which uses m_node).

Then the main commits:

3. Add template_tx_refs to NodeContext to track how many templates contain any given transaction. This map is updated by the BlockTemplate constructor and destructor.
4. Add GetTemplateMemoryUsage() which loops over this map and sums up the memory footprint for transactions outside the mempool
5. Expose this information to IPC clients via getMemoryLoad() and add test coverage

[DrahtBot]

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/33922.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	vasild
Concept ACK	ismaelsadeeq, ryanofsky
Stale ACK	enirox001

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #34806 (refactor: logging: Various API improvements by ajtowns)
• #34803 (mempool: asynchronous mempool fee rate diagram updates via validation interface by ismaelsadeeq)
• #34644 (mining: add submitBlock to IPC Mining interface by w0xlt)
• #34075 (fees: Introduce Mempool Based Fee Estimation to reduce overestimation by ismaelsadeeq)
• #34020 (mining: add getTransactions(ByWitnessID) IPC methods by Sjors)
• #33966 (refactor: disentangle miner startup defaults from runtime options by Sjors)
• #33421 (node: add BlockTemplateCache by ismaelsadeeq)

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.

[Sjors]

I haven't benchmarked this yet on mainnet, so I'm not sure if checking every (unique) transaction for mempool presence is unacceptably expensive.

If people prefer, I could also add a way for the getblocktemplate RPC to opt-out of the memory bookkeeping, since it holds on to one template max and no longer than a minute.

[DrahtBot]

🚧 At least one of the CI tasks failed.
_{Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/19575422916/job/56059300316
LLM reason (✨ experimental): clang-tidy flagged fatal errors (loop variable copied for range-based for causing a warnings-as-errors failure) in interfaces.cpp, breaking the CI run.}

Hints

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.

[Sjors]

b9306b79b8f5667a2679236af8792bb1c36db817: in addition, we might be wiping the dummy coinbase from the template later: Sjors#106

[ismaelsadeeq]

Concept ACK

I think it would be better if we have internal memory management for the mining interface IPC, since we hold on to the block templates.

I would suggest the following approach:

• Add memory budget for the mining interface.
• Introduce a tracking list of recently built block templates and total memory usage.
• Add templates to the list and increment the memory usage after every createnewblock or waitnext return.
• Whenever the memory budget is exhausted, we should release templates in FIFO order.

I think since we create a new template after a time interval elapses even if fees increase and that interval is usually enough for the client to receive and distribute the template to miners, this mechanism should be safe as the miners have long switch to most recent template when the budget elapsed because of the time interval being used in between returns of waitnext.

Mining interface clients should also handle their own memory internally.

Currently, I don’t see much use for the exposed getMemoryLoad method. In my opinion, we should not rely on the IPC client to manage our memory.

[Sjors]

In my opinion, we should not rely on the IPC client to manage our memory.

Whenever the memory budget is exhausted, we should release templates in FIFO order

It seems counter intuitive, but from a memory management perspective IPC clients are treated no different than our own code. And if we started FIFO deleting templates that are used by our own code, we'd crash.

So I think FIFO deletion should be a last resort (not implemented here).

There's another reason why we should give clients an opportunity to gracefully release templates in whatever order they prefer. Maybe there's 100 downstream ASIC's, one of which is very slow at loading templates, so it's only given a new template when the tip changes, not when there's a fee change. In that scenario you have a specific template that the client wants to "defend" at all cost.

In practice I'm hoping none of this matters and we can pick and recommend defaults that make it unlikely to get close to a memory limit, other than during some weird token launch.

[ismaelsadeeq]

It seems counter intuitive, but from a memory management perspective IPC clients are treated no different than our own code. And if we started FIFO deleting templates that are used by our own code, we'd crash.

IMHO I think we should separate that, and treat clients differently from our own code, because they are different codebases and separate applications with their own memory.

Maybe there are 100 downstream ASICs, one of which is very slow at loading templates, so it’s only given a new template when the tip changes, not when there’s a fee change. In that scenario you have a specific template that the client wants to “defend” at all costs.

I see your point but I don’t think that’s a realistic scenario, and I think we shouldn’t design software to be one-size-fits-all.
If you want to use only single block templates, then use createnewblock and create a new block template and mine that continuously until the chain tip changes or you mine a block.

waitNext returning indicates that we assume your miners are switching from the block they are currently mining to the new one they receive.
Depending on the budget (which I assume is large), many templates would need to be returned before we exhaust it.

Delegating template eviction responsibility to the client can put us in a situation where they handle it poorly and cause us to OOM (but I guess your argument is that we rather take that chance than being in a situation where we make miners potentially lose on rewards).
However I think if there is a clean separation of concerns between the Bitcoin Core node and its clients and clear interface definition and expectations that should not happen, and I believe the mining interface should not differ in that respect.
Otherwise, if we do want a one-size-fits-all solution capable of handling the scenario you described, we should rethink the design entirely and revert to an approach where we do not retain block templates.

[Sjors]

Delegating template eviction responsibility to the client can put us in a situation where they handle it poorly and cause us to OOM

Note that it's already the clients responsibility, that's inherent to how multiprocess works.

In the scenario where they handle it poorly, we can use FIFO deletion. All getMemoryLoad() does is give clients an opportunity to handle it better. If they're fine with FIFO, then they never have to call this method.

treat clients differently from our own code

We currently don't track whether any given CBlockTemplate is owned by an IPC client or by our internal code. Once we introduce FIFO deletion all call sites will have to check if it's been deleted since, or we need to exempt them from the memory accounting.

an approach where we do not retain block templates.

Afaik that means revalidating the block from scratch, removing one advantage the submitBlock() approach has over the submitblock RPC (I haven't benchmarked this though).

[Sjors]

I tracked the non-mempool transaction memory footprint for half a day on mainnet, using fairly aggressive template update criteria (minimum fee delta 1 sat and no more than once per second). So far the footprint is minuscule, but of course this depends on the mempool weather:

The memory spike after each new block is because sv2-tp holds on to templates from previous blocks for 10 seconds. Those ~3 MB spikes may look impressive, but keep in mind that the default mempool is 300 MB.

[Sjors]

I restructured the implementation and commits a bit.

The TxTemplateMap now lives on the NodeContext rather than MinerImpl (interface). This reflects the fact that we want to track the global memory footprint instead of per client. It's a lightweight member template_tx_refs which should be easy to fold into a block template manager later.

It's also less code churn because I don't have to touch the BlockTemplateImpl constructor.

It also made it easier to move GetTemplateMemoryUsage from interface.cpp to miner.cpp, where it's more reusable.

This in turn let me split out a separate commit that introduces the actual getMemoryLoad() interface method. So even if we decide against including that method, the rest of the PR should be useful. However I do think it's worth keeping, it's already been a helpful debugging and monitoring tool.

I added some comments to point out that we don't hold a mempool.cs lock during the calculation because we don't need an accurate result (mempool drift) and we don't want to bog down transaction relay with a potentially long lock (1-3ms in my testing so far).

[Sjors]

mining_getblocktemplate_longpoll.py triggered a stack-use-after-return, due to block_template being static (to allow template reuse between RPC calls). I added a commit d752dccaa56b663001d1bb29ab8b9a50628602a9 to move this longpoll template to the node context. This seems more appropriate anyway since BlockTemplate has a m_node member, so it shouldn't be able to outlive the node.

One caveat is that gbt_template has to be cleared before template_tx_refs, so I swapped them and added a comment (cde248a6613b6e37f7f7e35c1aabeb75347ffe95 -> 9c667c362a1639b48113a3657882b751f475082c.

---

Expanded the PR description.

[Sjors]

The workaround is still needed. We can drop it here if #35073 lands.

[DrahtBot]

🚧 At least one of the CI tasks failed.
_{Task macOS-cross to arm64: https://github.com/bitcoin/bitcoin/actions/runs/24570973945/job/71843744324
LLM reason (✨ experimental): CI failed due to a C++ build error: reference to 'hash' is ambiguous in primitives/transaction.h (conflict with std::hash).}

Hints

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.

[Sjors]

Waiting for CI to pass. I'm probably going to move 92fd54b and b89942d in a separate PR. Need to address this too: #33922 (comment)

[Sjors]

The safety commit for CTransactionRef and IWYU changes are now in #35101 and dropped from this PR.

[vasild]

ACK d37e0fc

Show Signature

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK d37e0fc16c235ff511bd9485ef505f0c5615d9e4
-----BEGIN PGP SIGNATURE-----

iQQzBAEBCAAdFiEE5k2NRWFNsHVF2czBVN8G9ktVy78FAmnvNGMACgkQVN8G9ktV
y79uSyAAmswvwjxbv/vFTrf0BERXNHfG0lEVrvURRQavQzajUq0Q0dSiUpBlklGY
Ec+zt5x6G9vSGkYeo7ZFyjV7uwhLMMKlOGxqmnrKUT4PWHYjiItpnsJPEE8iK7hc
M2Xpq7OGNiWfc+ZT81KIYYPo6v+dRBKBh+dHwdgJ/5X7nFSMru7OV0+W4uk4wfu1
xayuZ5A1jWqM/vqYt9jEtmEaPhyVma61CxiMCGoxOIuhsuKGx49gZb+AQi3zuyD0
GKViOOA+A9krE0Ntr8MZeoXRw/+loj+yDp49s/l9LJ6rG8C3o4CKDMjzTcGSbWp8
sc1dN0wohyMwOrBToEidkC+G4eN1+uRG4KDbkfy+gpbeNfzLj7lLDppmxJGXimKm
isCC9sq14Y0caSomDvLGFddtaGgmnq4AoZQLT49j6tleOaQArQv7xWwTgjiVMcG3
OhqCEHeuJJEDoLHaoVFG4MHSx2TrgukC4UtqRvASCayHslU0z/77RlkeKZD4B/k8
iaHOocAMl0m61r+Cw4ryZ0YfGfRKrOoFJ3PLmF/pcjj3iDE2Eq1vZ0pNq/ECzpSm
zOiLRt+3r5NsUphE9jm2AROKtQApaklNjCWidK7bUCRoA884y660m/TIZKJhcDj1
p/OPxjz36ZDdTV5YLQI+JOxgdnndfF1HQqjspZIHPsMOH6e+qkIDfyfW0apExHN+
yDoI1XoEPEIXJv4nsjSFWIUZajSOSTHjV96xrx6/ca3bbE5thix9DqLhhPsvtSHn
ykxzMhbwgUQND3Af2q16YUA9qhjUAo87Ixe99M6Ywnti52gqJ5zRZf4EgWy+A5kk
tm/YLkN3xdnLl3frNZVrBGHZwEbDMZdu8WwpHgnuGPH5DASjG4HcdJaXE0WhCNpc
mVq6YZfxJZ9NNGIpcV6pU4RoFALb981XlFfRhgtC+5Nyrjui/uPF0GrXGsiHSG9w
OzWNsR9ghyBtXyxnZ8iCI+uyX8TVxc6M/vU5PoVZGEoKJaBZqh+NclfgsZLnZqx0
5iwCLPAPJZjXsabuQ8Ya54ctHHySyk36ciqMpwWRpPPkGMW4Ya3HNMfZGSQmhiYb
FeaghFsHwtgmjbBgTLznSUCzZH+Ec7qKdk0vw+uPZveYHHIUrvbAnXuNOlbAiN1Y
XDDpPtLld+wZ89c7XUVM+5tM30fm8/M0w9y5ezDAeELVniU1mcPOnBaqgLBkv570
mJa7CEmLYOaSMzkVi3gx02sjqbEsRJlx9WAQvlETuljWT/GNggZ5hu6WCndHZ8zN
hPdARxrVv9D7p4ktAUcFRRcPalbszWwxrqp4/lnSpJbiYnNZDt5V1w2QniHBg6g/
AdTc9REfGHEi14IqXV3ysK2UMlBDmg==
=q3ZT
-----END PGP SIGNATURE-----

vasild's public key is on openpgp.org