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

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
ACK	vasild
Concept ACK	ismaelsadeeq, ryanofsky
Stale ACK	enirox001

If your review is incorrectly listed, please copy-paste <code></code> 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 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.

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

in src/node/interfaces.cpp:882 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:883 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:

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

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.

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

DrahtBot added the label CI failed on Dec 5, 2025

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

Sjors force-pushed on Dec 19, 2025

Sjors commented at 10:25 AM on December 19, 2025: member

Rebased after #34003. Dropped c548d6f0e8ecc0da6e29256c7085d48d10e10216 test: destroy templates more carefully. That commit also added coverage for feeThreshold == MAX_MONEY, so I moved that into a new commit - not really related to this PR though.

DrahtBot removed the label Needs rebase on Dec 19, 2025

DrahtBot added the label Needs rebase on Jan 13, 2026

Sjors commented at 9:54 AM on January 14, 2026: member

Rebased after #33819

Sjors force-pushed on Jan 14, 2026

DrahtBot removed the label Needs rebase on Jan 14, 2026

DrahtBot added the label Needs rebase on Feb 2, 2026

Sjors force-pushed on Feb 3, 2026

DrahtBot removed the label Needs rebase on Feb 3, 2026

DrahtBot added the label CI failed on Feb 3, 2026

DrahtBot removed the label CI failed on Feb 4, 2026

DrahtBot added the label Needs rebase on Feb 7, 2026

Sjors force-pushed on Feb 19, 2026

Sjors commented at 11:06 AM on February 19, 2026: member

Rebased after #34452 and #33965.

DrahtBot removed the label Needs rebase on Feb 19, 2026

DrahtBot added the label Needs rebase on Feb 20, 2026

Sjors force-pushed on Feb 23, 2026

Sjors commented at 11:09 AM on February 23, 2026: member

Rebased after #34568.

Ancestor CI failure is probably related to #34646.

DrahtBot added the label CI failed on Feb 23, 2026

DrahtBot removed the label Needs rebase on Feb 23, 2026

Sjors force-pushed on Feb 24, 2026

Sjors commented at 12:17 PM on February 24, 2026: member

Rebased after #34184.

DrahtBot removed the label CI failed on Feb 24, 2026

in src/node/context.h:79 in c37d715c1d

  74 | +    //! Track how many templates (which we hold on to on behalf of connected IPC
  75 | +    //! clients) are referencing each transaction.
  76 | +    TxTemplateMap template_tx_refs GUARDED_BY(template_state_mutex);
  77 | +    //! Cache latest getblocktemplate result for BIP 22 long polling. Must be cleared
  78 | +    //! before template_tx_refs.
  79 | +    std::unique_ptr<interfaces::BlockTemplate> gbt_template;

vasild commented at 2:35 PM on March 3, 2026:

Is "gbt_template" supposed to mean "get block template template"? Maybe "gbt_result" or "get_block_template_result" would be better.

vasild commented at 3:43 PM on March 3, 2026:

I think the comment warrants an elaboration. gbt_template is not explicitly cleared anywhere, so that happens at the destructor of NodeContext. struct members are destroyed in reverse order of their declaration. Is this comment intended to prevent swapping the declaration order of template_tx_refs and gbt_template?

Maybe:

    //! Cache latest getblocktemplate result for BIP 22 long polling. Must be cleared
    //! before template_tx_refs because the destructor of this decrements the count
    //! in `template_tx_refs` of each transaction in the template. If it does not find
    //! some of its transactions in `template_tx_refs` then it will abort.

Sjors commented at 7:43 PM on March 5, 2026:

I went for gbt_result

Sjors commented at 7:56 PM on March 5, 2026:

I took your comment.

It’s not only about declaration order. The intent is to destroy all BlockTemplate instances, including the one in gbt_result, while template_tx_refs is still alive so destructors can decrement reference counts. Once all templates are gone (and the map is empty), template_tx_refs can be destroyed.

in src/node/interfaces.cpp:67 in c37d715c1d outdated

  66 | @@ -67,6 +67,7 @@
  67 |  #include <any>

vasild commented at 2:48 PM on March 3, 2026:

In the commit message of ee259ca6b4dd4986be45e31aeee8d73bad8a115a mining: track non-mempool memory usage:

"transactions that are removed from the mempool will ~have~ not have their memory cleared."

Sjors commented at 7:43 PM on March 5, 2026:

Fixed

in src/node/types.h:179 in c37d715c1d

 172 | @@ -172,6 +173,11 @@ enum class TxBroadcast : uint8_t {
 173 |      NO_MEMPOOL_PRIVATE_BROADCAST,
 174 |  };
 175 |  
 176 | +/*
 177 | + * Map how many templates refer to each transaction reference.
 178 | + */
 179 | +using TxTemplateMap = std::map<CTransactionRef, size_t>;

vasild commented at 3:26 PM on March 3, 2026:

How many entries will be supposedly stored in this map? std::map has lookup O(log(size)) whereas std::unordered_map has O(1). Here we do not need the entries to be ordered.

nit: start the comment with /** to make doxygen recognize it and attach it to the following code in the documentation.

Sjors commented at 7:44 PM on March 5, 2026:

Done both.

Assuming one template per second for two hours (long block interval), less than 10k.

vasild commented at 8:55 AM on March 6, 2026:

Just mentioning - it is the same for map and unordered_map when used with shared_ptr (or CTransactionRef) - they compare pointers. So, two distinct objects that have the same values for all their members will be considered different. I am not sure if this is a problem in our code. Grepping the code for (map|set).*CTransactionRef, it looks like this will be the first case in non-test code where we use CTransactionRef as a key without providing custom comparator/hasher.

To illustrate with an explicit example:

    CMutableTransaction mutable_tx;
    // mutable_tx.vin = ...
    // mutable_tx.vout = ...

    CTransaction tx1{mutable_tx};
    CTransaction tx2 = tx1; // tx2 is a copy of tx1, same transaction _logically_

    CTransactionRef tx1_ref{MakeTransactionRef(tx1)};
    CTransactionRef tx2_ref{MakeTransactionRef(tx2)};

    std::map<CTransactionRef, int> m;
    assert(m.emplace(tx1_ref, 5).second); // inserted
    assert(m.emplace(tx2_ref, 6).second); // inserted
    assert(m.size() == 2); // has 2 elements

vasild commented at 11:16 AM on March 6, 2026:

If this needs to be resolved, then the below should do it:

diff --git i/src/node/types.h w/src/node/types.h
index 164667772a..7bb187ac1c 100644
--- i/src/node/types.h
+++ w/src/node/types.h
@@ -11,21 +11,23 @@
 //! files.
 
 #ifndef BITCOIN_NODE_TYPES_H
 #define BITCOIN_NODE_TYPES_H
 
 #include <consensus/amount.h>
-#include <cstddef>
-#include <cstdint>
-#include <optional>
 #include <policy/policy.h>
 #include <primitives/transaction.h>
 #include <script/script.h>
-#include <unordered_map>
 #include <uint256.h>
+#include <util/hasher.h>
 #include <util/time.h>
+
+#include <cstddef>
+#include <cstdint>
+#include <optional>
+#include <unordered_map>
 #include <vector>
 
 namespace node {
 enum class TransactionError {
     OK, //!< No error
     MISSING_INPUTS,
@@ -173,11 +175,11 @@ enum class TxBroadcast : uint8_t {
     NO_MEMPOOL_PRIVATE_BROADCAST,
 };
 
 /**
  * Map how many templates refer to each transaction reference.
  */
-using TxTemplateMap = std::unordered_map<CTransactionRef, size_t>;
+using TxTemplateMap = std::unordered_map<CTransactionRef, size_t, CTransactionRefSaltedHash, CTransactionRefComp>;
 
 } // namespace node
 
 #endif // BITCOIN_NODE_TYPES_H
diff --git i/src/util/hasher.h w/src/util/hasher.h
index 02c7703391..d3b77ba72d 100644
--- i/src/util/hasher.h
+++ w/src/util/hasher.h
@@ -114,7 +114,23 @@ private:
 public:
     SaltedSipHasher();
 
     size_t operator()(const std::span<const unsigned char>& script) const;
 };
 
+struct CTransactionRefSaltedHash {
+    SaltedWtxidHasher m_wtxid_hasher;
+
+    size_t operator()(const CTransactionRef& tx) const
+    {
+        return m_wtxid_hasher(tx->GetWitnessHash());
+    }
+};
+
+struct CTransactionRefComp {
+    bool operator()(const CTransactionRef& a, const CTransactionRef& b) const
+    {
+        return a->GetWitnessHash() == b->GetWitnessHash();
+    }
+};
+
 #endif // BITCOIN_UTIL_HASHER_H

Using a salted hash instead of barely using the first bytes of the transaction id as a hash because somebody may craft a pile of transactions with such ids as to make unordered_map lookup time deteriorate from O(1) to O(size). Not sure if that is an overkill in the current use case. In PrivateBroadcast::m_transactions I used the simpler:

    struct CTransactionRefHash {
        size_t operator()(const CTransactionRef& tx) const
        {   
            return static_cast<size_t>(tx->GetWitnessHash().ToUint256().GetUint64(0));
        }
    };

because there 1. the transactions are originating locally (do not come from untrusted sources) and 2. the expectation is to store a small number of transactions, so even O(size) will not hog the machine.

Sjors commented at 9:25 AM on April 8, 2026:

Sorry I missed this followup earlier.

I ended up using CTransactionRefSaltedHash. Unless performance is actually a problem, it's better to err on the side of caution.

It seems unlikely that someone is going to spend a fortune on grinding 64 bit mempool collisions to marginally slow this function down. But the transactions are not generated locally and with a combination bad mempool weather and a long block interval, the collection can be large.

Also, since we're exposing this function in src/util/hasher.h, where others might reuse it, it seems better to use a safe version.

in src/node/miner.h:17 in c37d715c1d

  13 | @@ -14,6 +14,7 @@
  14 |  #include <util/feefrac.h>
  15 |  
  16 |  #include <cstdint>
  17 | +#include <map>

vasild commented at 4:38 PM on March 3, 2026:

src/node/miner.h:17 col 1 warning: Included header map is not used directly

in src/node/miner.h:14 in c37d715c1d

  13 | @@ -14,6 +14,7 @@
  14 |  #include <util/feefrac.h>

vasild commented at 4:40 PM on March 3, 2026:

In the commit message of ce781758df81bca7bc3b60b8e02e36064634439e ipc: add getMemoryLoad()

"which can later be expand to" -> "which can later be expanded to"

vasild approved

vasild commented at 4:49 PM on March 3, 2026: contributor

ACK c37d715c1d32020fe16ddc92f7ca95f585cc6b4b

DrahtBot requested review from ismaelsadeeq on Mar 3, 2026

DrahtBot requested review from ryanofsky on Mar 3, 2026

Sjors force-pushed on Mar 5, 2026

Sjors commented at 7:58 PM on March 5, 2026: member

Rebased after #34422 for easier testing with Rust, see https://github.com/2140-dev/bitcoin-capnp-types/pull/13.

Implemented all of @vasild's nits.

Sjors referenced this in commit 33d0af5d71 on Mar 5, 2026

vasild approved

vasild commented at 8:57 AM on March 6, 2026: contributor

ACK 04553cd6612f08d757fb20287fe111007bfb6db1

Would be good to figure out if #33922 (review) needs addressing.

Sjors referenced this in commit fa3eb29207 on Mar 6, 2026

Sjors referenced this in commit 202285bae3 on Mar 11, 2026

enirox001 commented at 10:52 AM on April 6, 2026: contributor

ACK 04553cd

Went through each commit:

6f12988 - forward declaration of BlockTemplate in context.h is necessary, mining.h is not in the include chain
b22e535 - destruction ordering of gbt_result before template_tx_refs looks correct. break in ~BlockTemplateImpl makes sense, and no lock inversion between template_state_mutex and cs_main
1d7497f - mempool.cs is intentionally not held for the full loop to avoid blocking relay, which is well documented
9f4b744 - locking looks consistent with commit 3, template_state_mutex guards the map while mempool.cs is acquired per exists() call
04553cd - MAX_MONEY is a good boundary case to have covered

Ran interface_ipc_mining.py and all tests passed successfully.

Sjors force-pushed on Apr 8, 2026

Sjors commented at 9:32 AM on April 8, 2026: member

@vasild wrote:

Would be good to figure out if #33922 (comment) needs addressing.

Thanks for the reminder. I ended up using the salted hasher in a6d4b883ce48ce04721a6dc6160e166bc3a40875.

I also added 430990c0c5bc72f8b64e76056b304b88821b9af0 to delete the default std::hash constructor for CTransactionRef, for good measure.

Deleting operator== would make sense for the same reason, but involves a fair amount of test churn, so I didn't do it here.

Sjors force-pushed on Apr 8, 2026

DrahtBot added the label CI failed on Apr 8, 2026

DrahtBot removed the label CI failed on Apr 8, 2026

in src/primitives/transaction.h:25 in 10acf818ed

  21 | @@ -22,6 +22,7 @@
  22 |  #include <string>
  23 |  #include <tuple>
  24 |  #include <utility>
  25 | +#include <variant>

vasild commented at 10:14 AM on April 9, 2026:

<variant> was added in the last commit 10acf818ed70d4cd2af864278a35a6b64de0bb16 refactor: disable default std::hash for CTransactionRef, but I think that it is not needed for the newly added code:

/** Disable default std::hash for CTransactionRef to prevent accidentally  
 *  comparing by pointer. Use CTransactionRefSaltedHash or provide a custom
 *  hasher. */
template<>                         
struct std::hash<CTransactionRef> {                          
    size_t operator()(const CTransactionRef&) const = delete;
};

Sjors commented at 6:43 PM on April 14, 2026:

It was an IWYU false positive, see comments below.

vasild approved

vasild commented at 10:15 AM on April 9, 2026: contributor

ACK 10acf818ed70d4cd2af864278a35a6b64de0bb16

DrahtBot requested review from enirox001 on Apr 9, 2026

Sjors force-pushed on Apr 14, 2026

DrahtBot added the label CI failed on Apr 14, 2026

Sjors commented at 6:20 PM on April 14, 2026: member

The IWYU linter insists on #include <variant>: https://github.com/bitcoin/bitcoin/actions/runs/24406181800/job/71290035323?pr=33922

But that seems wrong, so I added a commit to override it.

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.

8e4294a7ab

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

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

56609d9598

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.

f9db4928d9

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

a9171534b9

test: cover feeThreshold = MAX_MONEY d37e0fc16c

Sjors force-pushed on Apr 14, 2026

Sjors commented at 6:26 PM on April 14, 2026: member

Trying a rebase without cd1fce108dca9c02abf76cbeb009ed6c2cfa8cc9 first, to see if #34896 fixed the IWYU false positive.

Sjors force-pushed on Apr 14, 2026

Sjors commented at 6:42 PM on April 14, 2026: member

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

DrahtBot removed the label CI failed on Apr 14, 2026

Sjors referenced this in commit 7a41b0bd30 on Apr 15, 2026

Sjors referenced this in commit 4e50c575d8 on Apr 15, 2026

Sjors referenced this in commit daf32c75eb on Apr 15, 2026

Sjors referenced this in commit 766d611ee5 on Apr 15, 2026

Sjors referenced this in commit f4b245fb1c on Apr 15, 2026

Sjors referenced this in commit e7854856fc on Apr 15, 2026

in src/primitives/transaction.h:413 in e8aaada3f2 outdated

 408 | + *  comparing by pointer. Use CTransactionRefSaltedHash or provide a custom
 409 | + *  hasher. */
 410 | +template<>
 411 | +struct std::hash<CTransactionRef> {
 412 | +    size_t operator()(const CTransactionRef&) const = delete;
 413 | +};

hebasto commented at 1:34 PM on April 16, 2026:

Are we sure about this parameter type? According to the docs.html):

std::hash<Key>::operator() ... takes a single argument key of type Key.

Should this be CTransactionRef instead of const CTransactionRef&?

Sjors commented at 1:30 PM on April 17, 2026:

Thanks, I'll look into this after figuring out the iwyu stuff.

maflcko commented at 4:34 PM on April 17, 2026:

Should this be CTransactionRef instead of const CTransactionRef&?

No, I am sure the docs just refer to the logical type . C.f also e.g. libc++:

https://github.com/llvm/llvm-project/blob/25b0ab2d4f7a7a4b165b26d31dd563ef4dde4f17/libcxx/include/string#L3909-L3915

Sjors commented at 9:58 AM on April 24, 2026:

(this was moved to #35101)

Sjors referenced this in commit 61200d5399 on Apr 17, 2026

Sjors force-pushed on Apr 17, 2026

DrahtBot added the label CI failed on Apr 17, 2026

Sjors force-pushed on Apr 17, 2026

in src/primitives/transaction.h:28 in e23a8f3015

  21 | @@ -22,6 +22,10 @@
  22 |  #include <string>
  23 |  #include <tuple>
  24 |  #include <utility>
  25 | +// IWYU erroneously suggests including <variant> instead of <memory>
  26 | +// for std::hash<CTransactionRef>.
  27 | +// See https://github.com/include-what-you-use/include-what-you-use/issues/2007.
  28 | +// IWYU pragma: no_include <variant>

maflcko commented at 1:53 PM on April 17, 2026:

NACK

This is wrong and harmful, as already explained in #35073 (comment)

Sjors commented at 1:56 PM on April 17, 2026:

Open to suggestions that you've actually tried.

maflcko commented at 2:30 PM on April 17, 2026:

My suggestion would be either commit 10acf818ed70d4cd2af864278a35a6b64de0bb16 (CI passed IIUC, so I think that counts as "tried"?) An alternative would be #35073 (review), which I presume was tried by the author that posted it. I am happy to push that suggestion to CI, if you want me to try it as well.

Sjors commented at 2:43 PM on April 17, 2026:

My suggestion would be either commit https://github.com/bitcoin/bitcoin/commit/10acf818ed70d4cd2af864278a35a6b64de0bb16 (CI passed IIUC, so I think that counts as "tried"?)

That's fair.

An alternative would be #35073 (review),

I'm trying that again now, this time with namespace std that was missing in my earlier attempt.

Sjors commented at 3:30 PM on April 17, 2026:

Unfortunately that leads to a reference to 'hash' is ambiguous (std::__1::hash vs std::hash). Let's see if b89942d29f1e71429070b2d1140d2792eb33bf47 fixes that.

10acf818ed70d4cd2af864278a35a6b64de0bb16 is of course a lot simpler, but also brittle because slightly different builds of IWYU will demand different includes (<variant> or <string_view>).

maflcko commented at 3:42 PM on April 17, 2026:

10acf81 is of course a lot simpler, but also brittle because slightly different builds of IWYU will demand different includes (<variant> or <string_view>).

Correct, but I think this is already true on current master: I think on current master a different build of IWYU will already yield a different result. I know it is not ideal, but I think the current approach is to let the CI spit out the diff when in doubt, then look if the diff has a backdoor or some nasty/wrong stuff, if not, just take the diff.

Unfortunately that leads to a reference to 'hash' is ambiguous (std::__1::hash vs std::hash). Let's see if https://github.com/bitcoin/bitcoin/commit/b89942d29f1e71429070b2d1140d2792eb33bf47 fixes that.

Ah. Thx for testing. This looks a bit nasty to work around. Honestly, I'd just go with https://github.com/bitcoin/bitcoin/commit/10acf818ed70d4cd2af864278a35a6b64de0bb16 for now. If you want, I can try #35073 (review) next week or so, to see if it is overall better, but I wouldn't spend too much time on this.

Sjors commented at 4:07 PM on April 17, 2026:

I found a nicer variant, will open a fresh PR.

maflcko commented at 4:37 PM on April 17, 2026:

That looks a bit verbose. I'd just go with https://github.com/bitcoin/bitcoin/commit/10acf818ed70d4cd2af864278a35a6b64de0bb16. If you really want, I'd say you can open a follow-up if and when this one is merged. But no strong opinion, up to you.

Sjors commented at 7:48 PM on April 17, 2026:

The problem with 10acf818ed70d4cd2af864278a35a6b64de0bb16 is that it only works on our CI. On e.g. my Ubuntu 25.10 with include-what-you-use 0.26, based clang version 22.1.1, that commit fails because it insists on <string_view>.

Ideally we shouldn't end up in the same situation as our linter job that can't be run outside a container.

Sjors commented at 9:58 AM on April 24, 2026:

(this code was changed, and also moved to #35101, so no longer relevant here)

DrahtBot removed the label CI failed on Apr 17, 2026

Sjors force-pushed on Apr 17, 2026

Sjors marked this as a draft on Apr 17, 2026

DrahtBot added the label CI failed on Apr 17, 2026

DrahtBot commented at 3:25 PM on April 17, 2026: contributor

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

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

Sjors force-pushed on Apr 17, 2026

Sjors commented at 4:01 PM on April 17, 2026: member

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

DrahtBot removed the label CI failed on Apr 17, 2026

Sjors force-pushed on Apr 17, 2026

Sjors commented at 7:49 PM on April 17, 2026: member

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

Sjors marked this as ready for review on Apr 17, 2026

vasild approved

vasild commented at 10:04 AM on April 27, 2026: contributor

ACK d37e0fc16c235ff511bd9485ef505f0c5615d9e4

<details> <summary>Show Signature</summary>

vasild's public key is on openpgp.org

</details>