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

Sjors commented at 3:34 pm on November 21, 2025: member

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:

https://github.com/stratum-mining/sv2-tp/pull/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:

Tweaks interface_ipc.py so destroy() calls happen in an order that’s useful to later demonstrate memory management
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:

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.
Add GetTemplateMemoryUsage() which loops over this map and sums up the memory footprint for transactions outside the mempool
Expose this information to IPC clients via getMemoryLoad() and add test coverage

DrahtBot added the label Mining on Nov 21, 2025

DrahtBot commented at 3:34 pm on November 21, 2025: 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/33922.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	ismaelsadeeq, ryanofsky

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:

#34075 (fees: Introduce Mempool Based Fee Estimation to reduce overestimation by ismaelsadeeq)
#34020 (mining: add getTransactions(ByWitnessID) IPC methods by Sjors)
#34003 (test: interface_ipc.py minor fixes and cleanup by ryanofsky)
#33965 (mining: fix -blockreservedweight shadows IPC option by Sjors)
#33936 (mining: pass missing context to createNewBlock() and checkBlock() by Sjors)
#33819 (mining: getCoinbase() returns struct instead of raw tx by Sjors)
#33795 (test: Ignore error message give from python because of PYTHON_GIL by kevkevinpal)
#32420 (miner: drop dummy extraNonce in coinbase scriptSig for templates requested via IPC by Sjors)

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 commented at 3:36 pm on November 21, 2025: member

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.

Sjors force-pushed on Nov 21, 2025

DrahtBot added the label CI failed on Nov 21, 2025

DrahtBot commented at 4:05 pm on November 21, 2025: contributor

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

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.

in src/node/interfaces.cpp:868 in f22413f31f outdated

882     {
883         assert(m_block_template);
884+
885+        TxTemplateMap& tx_refs{*Assert(m_tx_template_refs)};
886+        // Don't track the dummy coinbase, because it can be modified in-place
887+        // by submitSolution()

Sjors commented at 4:17 pm on November 21, 2025:

b9306b79b8f5667a2679236af8792bb1c36db817: in addition, we might be wiping the dummy coinbase from the template later: https://github.com/Sjors/bitcoin/pull/106

Sjors force-pushed on Nov 21, 2025

ismaelsadeeq commented at 4:22 pm on November 21, 2025: member

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 commented at 4:34 pm on November 21, 2025: member

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.

DrahtBot removed the label CI failed on Nov 21, 2025

ismaelsadeeq commented at 5:38 pm on November 21, 2025: member

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 commented at 10:49 am on November 24, 2025: member

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 commented at 4:56 pm on November 24, 2025: member

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 force-pushed on Nov 25, 2025

Sjors commented at 3:56 pm on November 25, 2025: member

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 force-pushed on Nov 25, 2025

Sjors commented at 5:22 pm on November 25, 2025: member

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.

DrahtBot added the label CI failed on Nov 25, 2025

Sjors force-pushed on Nov 25, 2025

DrahtBot removed the label CI failed on Nov 25, 2025

in src/node/interfaces.cpp:881 in ac1e97a592 outdated

879+    ~BlockTemplateImpl()
880+    {
881+        for (const CTransactionRef& tx : m_block_template->block.vtx | std::views::drop(1)) {
882+            auto ref_count{m_node.template_tx_refs.find(tx)};
883+            if (!Assume(ref_count != m_node.template_tx_refs.end())) break;
884+            if (--ref_count->second == 0) {

brunoerg commented at 1:17 pm on December 2, 2025:

Just ran a mutation testing for this PR and this is the only unkilled mutant - feel free to ignore if it doesn’t make sense to address:

 0diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
 1index f734296b24..13ddbb672f 100644
 2--- a/src/node/interfaces.cpp
 3+++ b/src/node/interfaces.cpp
 4@@ -880,7 +880,7 @@ public:
 5         for (const CTransactionRef& tx : m_block_template->block.vtx | std::views::drop(1)) {
 6             auto ref_count{m_node.template_tx_refs.find(tx)};
 7             if (!Assume(ref_count != m_node.template_tx_refs.end())) break;
 8-            if (--ref_count->second == 0) {
 9+            if (1==1) {
10                 m_node.template_tx_refs.erase(ref_count);
11             }
12         }

Sjors commented at 9:36 am on December 3, 2025:

@brunoerg that’s useful. So far interface_ipc.py never had a transaction appear in multiple templates, so the destructor would always remove the last reference.

I adjusted the test so that it does. Now your mutation causes a crash during this test.

brunoerg commented at 6:06 am on December 4, 2025:

Nice, thank you!

Sjors commented at 8:19 am on December 3, 2025: member

Here’s a slightly more realistic plot from last night on a well connected node running on an Intel i5-8400:

It’s connected to DMND pool, declaring custom templates and getting them approved, but not actually mining. Due to their rate limiting I set -sv2interval=20, so if fees go up, it waits at least 20 seconds before generating a new template. It does not wait when the tip changes.

The machine also runs a lightning node and BTCPay so the moment block comes in the system is quite busy.

test: destroy templates more carefully

Prepare template destruction handling for a later commit that checks
memory management:

- add destroy_template helper which awaits the result and avoids
  calling destroy() if we never received a template
- reverse order and prevent template override. This ensures template
  and template2 (which don't have transactions) are destroyed last.

Additionally, expand the test to demonstrate how setting feeThreshold
to MAX_MONEY ignores new mempool transactions. This extra transaction
is needed in a later commit (to add coverage for reference counting).

c548d6f0e8

rpc: move static block_template to node context

The getblocktemplate RPC uses a static BlockTemplate, which goes out
of scope only after the node completed its shutdown sequence.

This becomes a problem when a later commit implements a destructor
that uses m_node.

a5eee29fd7

Sjors force-pushed on Dec 3, 2025

ryanofsky commented at 9:36 pm on December 3, 2025: contributor

Concept ACK e8f8f7f677bcde0179526be3ed9a657c44998b93. All the changes here seem good and mostly straightforward. The getMemoryLoad() function seems useful by itself and the underlying tracking would seem to provide almost everything needed to limit memory used by block templates.

I am a little concerned about the idea of proactively deleting block templates in FIFO order on behalf of clients, since it seems like this could increase complexity server-side, and client-side if clients have to deal with templates disappearing without being notified. Just not returning new templates after a certain amount of memory has been used would like a simpler approach.

re: #33922#issue-3652141565

Additionally, the functional test in interface_ipc.py is expanded to demonstrate how template memory management works: templates are not released until the client disconnects or calls the destroy() method.

Would be good if this said templates are also released if the python references are destroyed or go out of scope. (This stood out because I tested this yesterday in #33940 (comment).)

Sjors commented at 10:46 am on December 4, 2025: member

Just not returning new templates after a certain amount of memory has been used would like a simpler approach.

It is, but refusing to make new templates doesn’t stop the footprint of existing templates from growing. The worst case extra memory footprint for existing templates is the full size of the mempool.

This is rather unlikely though, it would only happen if between two blocks the entire mempool was gradually RBF’d in such a way that each transaction was at the top of the mempool briefly, and thus made it into a template.

Would be good if this said templates are also released

Added a sentence to the PR description.

in src/rpc/mining.cpp:861 in a5eee29fd7 outdated

857@@ -858,7 +858,7 @@ static RPCHelpMan getblocktemplate()
858     // Update block
859     static CBlockIndex* pindexPrev;
860     static int64_t time_start;
861-    static std::unique_ptr<BlockTemplate> block_template;
862+    std::unique_ptr<BlockTemplate>& block_template{node.gbt_template};

ryanofsky commented at 5:37 pm on December 4, 2025:

In commit “rpc: move static block_template to node context” (a5eee29fd7d177f57c78da9773cb656a129de839)

I think it would actually be nice to move all these static variables to a struct or class like @ismaelsadeeq’s BlockTemplateCache from #33421. But this could be a followup, and doesn’t need to complicate this PR.

in src/node/context.h:74 in 7c4d03d7b2 outdated

68@@ -67,7 +69,11 @@ struct NodeContext {
69     std::unique_ptr<AddrMan> addrman;
70     std::unique_ptr<CConnman> connman;
71     std::unique_ptr<CTxMemPool> mempool;
72-    //! Cache latest getblocktemplate result for BIP 22 long polling
73+    //! Track how many templates (which we hold on to on behalf of connected IPC
74+    //! clients) are referencing each transaction.
75+    TxTemplateMap template_tx_refs;

ryanofsky commented at 5:55 pm on December 4, 2025:

In commit “mining: track non-mempool memory usage” (7c4d03d7b23417612fbca2f22e5bb1a198c9e5a2)

This map can updated from multiple threads, so it needs a mutex to be used safely. I think I’d suggest combining template_tx_refs and gbt_template variables and a mutex into single struct called something like BlockTemplateState and adding a unique_ptr to that struct as a member here. The struct could be replaced with a cache class in #33421.

Sjors commented at 11:09 am on December 5, 2025:

To limit the scope of this PR, I only added the mutex, but called it template_state_mutex in anticipation.

in test/functional/interface_ipc.py:218 in e8f8f7f677 outdated

216             self.log.debug("Wait for another, but time out, since the fee threshold is set now")
217             template7 = await template6.result.waitNext(ctx, waitoptions)
218             assert_equal(template7.to_dict(), {})
219 
220+            self.log.debug("Memory load should be zero because there was no mempool churn")
221+            with self.nodes[0].assert_debug_log(["Calculate template transaction reference memory footprint"]):

ryanofsky commented at 6:11 pm on December 4, 2025:

In commit “ipc: add getMemoryLoad()” (e8f8f7f677bcde0179526be3ed9a657c44998b93)

Seems ok to assert this log message is logged, but I’m wondering if there was a particular reason for doing this. Was the idea to pair the LOG_TIME_MILLIS_WITH_CATEGORY and assert_debug_log calls together?

Sjors commented at 9:26 am on December 5, 2025:

This just checks that bench logging happens.

ryanofsky commented at 6:23 pm on December 4, 2025: contributor

Code review e8f8f7f677bcde0179526be3ed9a657c44998b93. This looks good except for a thread safety issue I think you can address by adding a mutex.

re: #33922 (comment)

Would be good if this said templates are also released

Added a sentence to the PR description.

Sorry, I should have made a more specific suggestion. The problem is is that this sentence is not accurate: “templates are not released until the client disconnects or calls the destroy() method.” Templates will be released if the client drops references to them, even if it never disconnects or calls destroy. I would just change it to “templates are not released until the client drops references to them, or calls the template destroy method, or disconnects”

Sjors force-pushed on Dec 5, 2025

mining: track non-mempool memory usage

IPC clients can hold on to block templates indefinately, which has the
same impact as when the node holds a shared pointer to the
CBlockTemplate. Because each template in turn tracks CTransactionRefs,
transactions that are removed from the mempool will have not have
their memory cleared.

This commit adds bookkeeping to the block template constructor and
destructor that will let us track the resulting memory footprint.

f90ab4ed71

mining: add GetTemplateMemoryUsage()

Calculate the non-mempool memory footprint for template transaction
references.

Add bench logging to collect data on whether caching or simplified
heuristics are needed, such as not checking for mempool presence.

a63d2e1c92

Sjors force-pushed on Dec 5, 2025

DrahtBot added the label CI failed on Dec 5, 2025

ipc: add getMemoryLoad()

Allow IPC clients to inspect the amount of memory consumed by
non-mempool transactions in blocks.

Returns a MemoryLoad struct which can later be expand to e.g.
include a limit.

Expand the interface_ipc.py test to demonstrate the behavior and
to illustrate how clients can call destroy() to reduce memory
pressure.

7f4249b0d0

Sjors force-pushed on Dec 5, 2025

DrahtBot removed the label CI failed on Dec 5, 2025

DrahtBot added the label Needs rebase on Dec 16, 2025

DrahtBot commented at 3:27 pm on December 16, 2025: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

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

Code Coverage & Benchmarks

Reviews

Conflicts