node: add `BlockTemplateCache`

ismaelsadeeq commented at 6:47 pm on September 17, 2025: member

This PR implements the first step of #33758.

This PR Implement a cache layer. All newly created block templates should be stored along with their respective configuration options. Client requests for block templates will specify the maximum age of the template in seconds; that is, how fresh they want the template to be:

If a template with matching options exists in the cache and the interval has not elapsed, a cached template is returned. If no template exists or the interval has elapsed, a new template is generated, stored in the cache, and returned. After insertion, the oldest template is evicted if the cache exceeds its maximum size. The cache has a configurable maximum size (default: 10). It also subscribes to the validation interface’s BlockConnected notification and clears the cache when a new block is connected or disconnected, preventing stale templates from being served. If test_block_validity is set to true in the block creation options and the cached template found has not yet been validated, it is checked and then returned. This stage will also allow sharing of template metadata built from the mining interface for a fee estimation mechanism that requires building a block template to obtain package fee rates and use them for fee estimation, and vice versa. See the discussion in #33389.

DrahtBot renamed this:
~~node: add `BlockTemplateCache`~~
node: add `BlockTemplateCache`
on Sep 17, 2025

DrahtBot commented at 6:47 pm on September 17, 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/33421.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	ryanofsky

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33676 (interfaces: enable cancelling running waitNext calls by ismaelsadeeq)
#33629 (Cluster mempool by sdaftuar)
#33591 (Cluster mempool followups by sdaftuar)
#31664 (Fees: add Fee rate Forecaster Manager by ismaelsadeeq)
#31382 (kernel: Flush in ChainstateManager destructor by TheCharlatan)
#30157 (Fee Estimation via Fee rate Forecasters 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.

ismaelsadeeq force-pushed on Sep 17, 2025

ismaelsadeeq force-pushed on Sep 23, 2025

ismaelsadeeq commented at 4:45 pm on September 23, 2025: member

Forced pushed from dca8200c71bb568ffe4821c68209e07a305db80d to 16d55585aa2d26e9177734927b8446faa72570e7 5db80d..16d5558

Main Changes are

Moved the BlockTemplateCache to src/node/miner.cpp to fix the circular dependency issue
The response now return the block template creation time
We search for match from right to left to return the fresh hit first
Added a fuzz test that test various cache condition and add more coverage to the BlockAssembler Code.

ismaelsadeeq commented at 5:01 pm on September 23, 2025: member

It seems the new fuzz test in 16d5558 caught a minor UndefinedBehaviorSanitizer: unsigned-integer-overflow issue on master.

0/home/admin/actions-runner/_work/_temp/src/node/miner.cpp:414:47: runtime error: unsigned integer overflow: 2000 - 4000 cannot be represented in type 'size_t' (aka 'unsigned long')
1    [#0](/bitcoin-bitcoin/0/) 0x5b2b2753aee0 in node::BlockAssembler::addPackageTxs(int&, int&) /home/admin/actions-runner/_work/_temp/build/src/./node/miner.cpp:414:47
2
3
4SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow /home/admin/actions-runner/_work/_temp/src/node/miner.cpp:414:47 
5MS: 5 PersAutoDict-PersAutoDict-EraseBytes-CopyPart-InsertRepeatedBytes- DE: "\000\000\000\000"-"\001\000\000\000\000\000\000\000"-; base unit: 05fe405753166f125559e7c9ac558654f107c7e9
60x1,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x27,0x27,0x27,0x27,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,
7\001\000\000\000\000\000\000\000''''\000\000\000\000\000\000\000\000\000\000\000\000\000
8artifact_prefix='./'; Test unit written to ./crash-52e81f820b02d40ce3132a4d8e27726deb8eb132
9Base64: AQAAAAAAAAAnJycnAAAAAAAAAAAAAAAAAA==

https://github.com/bitcoin/bitcoin/actions/runs/17949962995/job/51047356983?pr=33421

This is possible on master when node is started with -blockmaxweight=2000 , -blockreservedweight=2000 or just blockmaxweight=2000 and mining interface createNewBlock pass blockReservedWeight as 2000.

This undefined behaviour is possible after #32355 because it introduced a fixed BLOCK_FULL_ENOUGH_WEIGHT_DELTA with value 4000. Before #32355 the delta was tied to the options.blockreservedweight so will just be 2000 - 2000 which will result in a valid size_t value.

I’ll push a commit with a fix soon.

ismaelsadeeq force-pushed on Sep 24, 2025

glozow added the label Bug on Sep 24, 2025

glozow added the label Needs backport (29.x) on Sep 24, 2025

glozow added the label Needs backport (30.x) on Sep 24, 2025

glozow removed the label Bug on Sep 24, 2025

glozow removed the label Needs backport (29.x) on Sep 24, 2025

glozow removed the label Needs backport (30.x) on Sep 24, 2025

glozow commented at 5:55 pm on September 24, 2025: member

It seems the new fuzz test in 16d5558 caught a minor UndefinedBehaviorSanitizer: unsigned-integer-overflow issue on master.

Nice! Sorry for the labeling noise, I meant to label #33475

fanquake referenced this in commit 05d984b1a4 on Sep 25, 2025

DrahtBot added the label Needs rebase on Sep 25, 2025

in src/node/miner.cpp:102 in 9ad0c9cc15 outdated

92@@ -93,6 +93,14 @@ BlockAssembler::BlockAssembler(Chainstate& chainstate, const CTxMemPool* mempool
93 {
94 }
95 
96+BlockAssembler::BlockAssembler(Chainstate& chainstate, const CTxMemPool* mempool, const Options& options, AllowOversizedBlocks_tag)

ryanofsky commented at 3:09 pm on September 25, 2025:

In commit “node/miner: allow generation of oversized templates” (bd9bed462e251a5d3bd6e376f25060aeba9539d4)

This ALLOW_OVERSIZED_BLOCKS tag approach seems reasonable, but not ideal because:

It doesn’t only allow larger blocks. It also stops enforcing coinbase sigops limits and stops enforcing minimum clamp values.
It duplicates code between BlockAssembler::BlockAssembler constructors.

I think adding a new BlockAssembler::Options bool allow_oversized_blocks{false} option instead would be a more direct and extensible approach.

Separately (doesn’t need to be part of this PR) I also think it would be good to reverse 7392b8b084be14b75536887b7ff216152d0a3307 (#33222), and throw exceptions when values are out of range instead of clamping them. Clamping values seems likely to hide configuration errors that should be fixed and lead to unintended behavior.

ismaelsadeeq force-pushed on Sep 25, 2025

DrahtBot removed the label Needs rebase on Sep 25, 2025

DrahtBot added the label Needs rebase on Oct 23, 2025

ismaelsadeeq force-pushed on Oct 23, 2025

ismaelsadeeq force-pushed on Oct 27, 2025

DrahtBot removed the label Needs rebase on Oct 27, 2025

in src/init.cpp:1830 in 4de7887ca4 outdated

1825@@ -1825,6 +1826,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1826     ChainstateManager& chainman = *Assert(node.chainman);
1827     auto& kernel_notifications{*Assert(node.notifications)};
1828 
1829+    node.block_template_cache = node::MakeBlockTemplateCache(node.mempool.get(), chainman);
1830+    validation_signals.RegisterSharedValidationInterface(node.block_template_cache);

ryanofsky commented at 12:59 pm on October 29, 2025:

In commit “node: add block template cache to miner” (4de7887ca47197654f480471db04750d44e405de)

Not very important, but would be good to call UnregisterValidationInterface in Shutdown to unregister this. Otherwise, in theory if Init/Shutdown were called multiple times (e.g. in unit tests) there would be a memory leak. node.block_template_cache can be handled the same way as node.fee_estimator in Shutdown.

in src/node/context.h:74 in 4de7887ca4 outdated

70@@ -70,6 +71,7 @@ struct NodeContext {
71     std::unique_ptr<CBlockPolicyEstimator> fee_estimator;
72     std::unique_ptr<PeerManager> peerman;
73     std::unique_ptr<ChainstateManager> chainman;
74+    std::shared_ptr<BlockTemplateCache> block_template_cache;

ryanofsky commented at 1:01 pm on October 29, 2025:

In commit “node: add block template cache to miner” (4de7887ca47197654f480471db04750d44e405de)

I think it would probably be better for this to use unique_ptr instead shared_ptr (like fee_estimator and other members), so lifetime of the BlockTemplateCache is more explicit and predictable.

in src/node/miner.h:264 in 4de7887ca4 outdated

259+     *
260+     * @param options The block assembly options to use.
261+     * @param interval Maximum age of a cached template to be considered fresh.
262+     * @return std::shared_ptr<CBlockTemplate> Shared pointer to the block template.
263+     */
264+    virtual std::pair<std::shared_ptr<CBlockTemplate>, NodeClock::time_point> GetBlockTemplate(

ryanofsky commented at 1:25 pm on October 29, 2025:

In commit “node: add block template cache to miner” (4de7887ca47197654f480471db04750d44e405de)

It seems like there are a number of things here making this code more rigid and complicated than it needs to be which aren’t explained. It seems like it would be more straightforward to:

Make BlockTemplateCache::GetBlockTemplate method non-virtual
Drop MakeBlockTemplateCache function, and just give GetBlockTemplate class a constructor.
Drop BlockTemplateCacheImpl class and just implement functionality in BlockTemplateCache class.
Drop requirement that users of BlockTemplateCache need to deal with shared_ptr. Let them use cache objects with more predictable lifetimes.

I think these changes would make code simpler, PR more straightforward to review and the BlockTemplateCache class easier to use and test.

If there really is a need for the shared_ptr / virtual requirements, that would be also be ok, but they should be explained somewhere.

EDIT: I see PR description does provide a rationale: “MakeBlockTemplateCache returns a shared pointer so it can be registered with the validation interface using RegisterSharedValidationInterface hence not requiring explicit unregistration.”

I think this rationale doesn’t really hold though, because unregistration is still needed to prevent memory leaks. Also RegisterSharedValidationInterface was really intended for use by the wallet, because wallets can be unloaded at any time and need a way to unregister from events while events are still being sent and avoid race conditions. For the block template cache, it makes more sense to use RegisterValidationInterface instead and avoid shared_ptr virality and unpredictability.

in src/node/miner.cpp:517 in 4de7887ca4 outdated

512+    return elapsed >= interval;
513+}
514+
515+bool BlockTemplateCacheImpl::Template::OptionsEqual(const BlockAssembler::Options& other) const
516+{
517+    return m_options.use_mempool == other.use_mempool &&

ryanofsky commented at 1:51 pm on October 29, 2025:

In commit “node: add block template cache to miner” (4de7887ca47197654f480471db04750d44e405de)

Would be good to have a comment saying this is intentionally not comparing the coinbase_output_script field and why that is safe (assuming it is safe).

Also would be good to point out this is intentionally skipping test_block_validity.

in src/node/miner.cpp:515 in 4de7887ca4 outdated

510+        return true;
511+    }
512+    return elapsed >= interval;
513+}
514+
515+bool BlockTemplateCacheImpl::Template::OptionsEqual(const BlockAssembler::Options& other) const

ryanofsky commented at 1:54 pm on October 29, 2025:

In commit “node: add block template cache to miner” (4de7887ca47197654f480471db04750d44e405de)

Would seem nice to implement these comparisons in operator== helpers or other functions directly attached to BlockCreateOptions and BlockAssembler::Options structs. This way if new members are added to the structs, the comparison functions should not get out of date.

Otherwise it would be good to have comments in the two structs pointing out that this function may need to be updated if new fields are added.

I think it could also be nice to introduce options-comparing functionality in a separate commit to make this commit simpler.

in src/node/miner.cpp:470 in 4de7887ca4 outdated

465+    void BlockConnected(ChainstateRole /*unused*/, const std::shared_ptr<const CBlock>& /*unused*/, const CBlockIndex * /*unused*/)
466+        override EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
467+private:
468+     std::pair<std::shared_ptr<CBlockTemplate>, NodeClock::time_point> CreateBlockTemplateInternal(const BlockAssembler::Options& options) EXCLUSIVE_LOCKS_REQUIRED(m_mutex);
469+
470+    struct Template {

ryanofsky commented at 1:57 pm on October 29, 2025:

In commit “node: add block template cache to miner” (4de7887ca47197654f480471db04750d44e405de)

Template name doesn’t seem very descriptive. I’d consider calling it CachedBlockTemplate to be unambiguous, and also making it a top level struct instead of an inner struct so it is less entwined with the BlockTemplateCacheImpl class, and could be forward declared, accessed from tests, etc.

ismaelsadeeq commented at 5:25 pm on October 31, 2025:

This is gone now.

in src/node/miner.h:30 in 4de7887ca4 outdated

26@@ -24,6 +27,7 @@
27 #include <boost/multi_index_container.hpp>
28 
29 class ArgsManager;
30+class BlockTemplateCache;

ryanofsky commented at 2:01 pm on October 29, 2025:

In commit “node: add block template cache to miner” (4de7887ca47197654f480471db04750d44e405de)

This doesn’t seem to be right because it is forward declaring a global ::BlockTemplateCache class not in the ::node namespace. Probably this should just be dropped.

in src/node/miner.h:45 in 4de7887ca4 outdated

41@@ -38,6 +42,8 @@ namespace node {
42 class KernelNotifications;
43 
44 static const bool DEFAULT_PRINT_MODIFIED_FEE = false;
45+static constexpr size_t DEFAULT_CACHE_SIZE{10};

ryanofsky commented at 2:02 pm on October 29, 2025:

In commit “node: add block template cache to miner” (4de7887ca47197654f480471db04750d44e405de)

DEFAULT_CACHE_SIZE is a very generic name for a global, maybe DEFAULT_BLOCK_TEMPLATE_CACHE_SIZE would be better.

in src/node/miner.cpp:461 in 4de7887ca4 outdated

495+    ChainstateRole /*unused*/,
496+    const std::shared_ptr<const CBlock>& /*unused*/,
497+    const CBlockIndex* /*unused*/)
498+{
499+    LOCK(m_mutex);
500+    m_block_templates.clear();

ryanofsky commented at 2:05 pm on October 29, 2025:

In commit “node: add block template cache to miner” (4de7887ca47197654f480471db04750d44e405de)

I’d think cache should be cleared when blocks are disconnected as well as when they are connected. Should there be a BlockDisconnected method that also clears the cache?

Also this should probably have if (role == ChainstateRole::BACKGROUND) return; to work properly when an assumeutxo snapshot is loaded.

in src/node/miner.cpp:503 in 4de7887ca4 outdated

498+{
499+    LOCK(m_mutex);
500+    m_block_templates.clear();
501+}
502+
503+bool BlockTemplateCacheImpl::Template::IntervalElapsed(const std::chrono::seconds& interval) const

ryanofsky commented at 2:22 pm on October 29, 2025:

In commit “node: add block template cache to miner” (4de7887ca47197654f480471db04750d44e405de)

Nothing about this function is really specific to caching templates. I think the both the caching code and the time code could be easier to understand if this was made into a helper function like:

0// Return true if current time is greater or equal to `time + interval`, or if
1// `time` is greater than the current time (indicating clock moved backward).
2static inline bool IntervalElapsed(NodeClock::time_point time, std::chrono::seconds interval)
3{
4    const NodeClock::time_point now = NodeClock::now();
5    return now < time || (now - time) >= interval;
6}

in src/node/miner.h:277 in 4de7887ca4 outdated

253+    virtual ~BlockTemplateCache() = default;
254+
255+    /**
256+     * If a cached template exists with identical options and its age does not
257+     * exceed the specified interval, the cached template is returned.
258+     * Otherwise, a new template is created and stored in the cache.

ryanofsky commented at 2:35 pm on October 29, 2025:

In commit “node: add block template cache to miner” (4de7887ca47197654f480471db04750d44e405de)

Might be good to say if that if interval is 0 a new template is always created.

in src/node/miner.h:257 in 4de7887ca4 outdated

252+public:
253+    virtual ~BlockTemplateCache() = default;
254+
255+    /**
256+     * If a cached template exists with identical options and its age does not
257+     * exceed the specified interval, the cached template is returned.

ryanofsky commented at 2:36 pm on October 29, 2025:

In commit “node: add block template cache to miner” (4de7887ca47197654f480471db04750d44e405de)

I think s/does not exceed/is less than/

in src/node/miner.cpp:481 in 4de7887ca4 outdated

476+        bool OptionsEqual(const BlockAssembler::Options& options) const;
477+    };
478+
479+    std::deque<Template> m_block_templates;
480+    CTxMemPool* m_mempool{nullptr};
481+    ChainstateManager& m_chainman;

ryanofsky commented at 2:59 pm on October 29, 2025:

In commit “node: add block template cache to miner” (4de7887ca47197654f480471db04750d44e405de)

It probably makes more sense for this to contain a Chainstate reference than a ChainstateManager reference, since code here is only calling m_chainman.ActiveChainstate on it anyway. Also logically, it makes more sense to have a per-chainstate cache, instead of a cache over multiple chainstates.

in src/node/miner.cpp:626 in 4de7887ca4 outdated

627-                chainman.ActiveChainstate(),
628-                mempool,
629-                assemble_options}
630-                              .CreateNewBlock()};
631-
632+            auto new_tmpl{block_template_cache->GetBlockTemplate(assemble_options, std::chrono::seconds{0}).first};

ryanofsky commented at 3:04 pm on October 29, 2025:

In commit “node: add block template cache to miner” (4de7887ca47197654f480471db04750d44e405de)

Would be good to note that 0 interval means this will always generate a new template, not use a cached one.

It could make sense to add a max_age or similar option to BlockCreateOptions so this is configurable by the caller.

in src/node/miner.cpp:628 in 4de7887ca4 outdated

630-                              .CreateNewBlock()};
631-
632+            auto new_tmpl{block_template_cache->GetBlockTemplate(assemble_options, std::chrono::seconds{0}).first};
633             // If the tip changed, return the new template regardless of its fees.
634-            if (tip_changed) return new_tmpl;
635+            if (tip_changed) return std::make_unique<CBlockTemplate>(*new_tmpl);

ryanofsky commented at 3:07 pm on October 29, 2025:

In commit “node: add block template cache to miner” (4de7887ca47197654f480471db04750d44e405de)

Is this temporary? Since GetBlockTemplate returns a shared_ptr, I think it would make sense for WaitAndCreateNewBlock to return a shared_ptr as well to avoid these copies.

in src/node/miner.cpp:473 in 4de7887ca4 outdated

468+     std::pair<std::shared_ptr<CBlockTemplate>, NodeClock::time_point> CreateBlockTemplateInternal(const BlockAssembler::Options& options) EXCLUSIVE_LOCKS_REQUIRED(m_mutex);
469+
470+    struct Template {
471+        BlockAssembler::Options m_options;
472+        NodeClock::time_point m_time_generated;
473+        std::shared_ptr<CBlockTemplate> m_block_template;

ryanofsky commented at 3:12 pm on October 29, 2025:

In commit “node: add block template cache to miner” (4de7887ca47197654f480471db04750d44e405de)

I think this type should be std::shared_ptr<const CBlockTemplate> (adding const) here and other places to avoid outside callers mutating the template and potentially messing up the cache. Or if adding const isn’t possible for some reason, it would be good to have a comment here explaining that.

It might also be handy to define a type alias like using BlockTemplateRef = std::shared_ptr<const CBlockTemplate> similar to CTransactionRef.

in src/node/miner.h:266 in 4de7887ca4 outdated

261+     * @param interval Maximum age of a cached template to be considered fresh.
262+     * @return std::shared_ptr<CBlockTemplate> Shared pointer to the block template.
263+     */
264+    virtual std::pair<std::shared_ptr<CBlockTemplate>, NodeClock::time_point> GetBlockTemplate(
265+        const BlockAssembler::Options& options,
266+        std::chrono::seconds interval

ryanofsky commented at 3:27 pm on October 29, 2025:

In commit “node: add block template cache to miner” (4de7887ca47197654f480471db04750d44e405de)

Would it make sense for this to use milliseconds or another unit instead of seconds so callers could have more granularity?

Also as suggested elsewhere I think it could make sense for interval to be a BlockCreateOptions field (called max_age or similar) so existing interfaces would not need to be modified to start taking advantage of template caching.

ismaelsadeeq commented at 5:35 pm on October 31, 2025:

Yes this will be good for precision but In the current iteration of the PR I switched to using the exact block assembly time as save in the block nTime which is in seconds.

However I took the other suggestion of changing the name to maximum age.

ismaelsadeeq commented at 5:18 pm on November 5, 2025:

This is fixed now

in src/node/miner.cpp:548 in 4de7887ca4 outdated

543+                if (BlockValidationState state{TestBlockValidity(m_chainman.ActiveChainstate(), it->m_block_template->block,
544+                                    /*check_pow=*/false, /*check_merkle_root=*/false)}; !state.IsValid()) {
545+                    throw std::runtime_error(strprintf("TestBlockValidity failed: %s", state.ToString()));
546+                }
547+                it->m_options.test_block_validity = true;
548+            }

ryanofsky commented at 4:20 pm on October 29, 2025:

In commit “node: add block template cache to miner” (4de7887ca47197654f480471db04750d44e405de)

Could be nice to dedup this part of the code with CreateNewBlock

 0--- a/src/node/miner.cpp
 1+++ b/src/node/miner.cpp
 2@@ -38,6 +38,16 @@
 3 
 4 namespace node {
 5 
 6+static void EnforceValidity(Chainstate& chainstate, const BlockAssembler::Options& options, const CBlock& block) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
 7+{
 8+    AssertLockHeld(::cs_main);
 9+    if (options.test_block_validity) {
10+       if (BlockValidationState state{TestBlockValidity(chainstate, block, /*check_pow=*/false, /*check_merkle_root=*/false)}; !state.IsValid()) {
11+           throw std::runtime_error(strprintf("TestBlockValidity failed: %s", state.ToString()));
12+       }
13+    }
14+}
15+
16 int64_t GetMinimumTime(const CBlockIndex* pindexPrev, const int64_t difficulty_adjustment_interval)
17 {
18     int64_t min_time{pindexPrev->GetMedianTimePast() + 1};
19@@ -191,12 +201,8 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock()
20     UpdateTime(pblock, chainparams.GetConsensus(), pindexPrev);
21     pblock->nBits          = GetNextWorkRequired(pindexPrev, pblock, chainparams.GetConsensus());
22     pblock->nNonce         = 0;
23+    EnforceValidity(m_chainstate, m_options, *pblock);
24 
25-    if (m_options.test_block_validity) {
26-        if (BlockValidationState state{TestBlockValidity(m_chainstate, *pblock, /*check_pow=*/false, /*check_merkle_root=*/false)}; !state.IsValid()) {
27-            throw std::runtime_error(strprintf("TestBlockValidity failed: %s", state.ToString()));
28-        }
29-    }
30     const auto time_2{SteadyClock::now()};
31 
32     LogDebug(BCLog::BENCH, "CreateNewBlock() packages: %.2fms (%d packages, %d updated descendants), validity: %.2fms (total %.2fms)\n",
33@@ -540,11 +546,8 @@ std::pair<std::shared_ptr<CBlockTemplate>, NodeClock::time_point> BlockTemplateC
34     for (auto it = m_block_templates.rbegin(); it != m_block_templates.rend(); it++){
35         if (it->OptionsEqual(options) && !it->IntervalElapsed(interval)) {
36             if (options.test_block_validity && !it->m_options.test_block_validity) {
37-                if (BlockValidationState state{TestBlockValidity(m_chainman.ActiveChainstate(), it->m_block_template->block,
38-                                    /*check_pow=*/false, /*check_merkle_root=*/false)}; !state.IsValid()) {
39-                    throw std::runtime_error(strprintf("TestBlockValidity failed: %s", state.ToString()));
40-                }
41                 it->m_options.test_block_validity = true;
42+                EnforceValidity(m_chainman.ActiveChainstate(), it->m_options, it->m_block_template->block);
43             }
44             return std::make_pair(it->m_block_template, it->m_time_generated);
45         }

ryanofsky approved

ryanofsky commented at 4:21 pm on October 29, 2025: contributor

Code review ACK 5f798b4a3b1a978b8ac954db54e9e8dfc7da6319, though I didn’t look closely at the fuzz test. Sorry for the delay in reviewing this, I’ve been meaning to get to it for some time. I think all the changes seem good and look correct, but I did suggest a number of possible updates below, mostly simplifications that should make the PR smaller.

I do think while the code changes seem good, it would be good to have more clarity about the cases where cache hits are expected and this cache is most likely to be useful. This seems like something that could be mentioned in the BlockTemplateCache documentation. I know the issue #33389 mentioned a number of cases but it’s unclear if these would be helped by the caching implemented here or if more changes like sharing templates with different options would be required.

If you have more changes building on this, or planned changes, it would be helpful to have a pointer or description. I don’t know if #31664 would benefit from the caching implemented here, for example.

I tend to think even if this PR is only used to improve getblocktemplate and remove global state [1], [2], [3], the changes might be worth it but I’m not sure if you implemented or are planning that.

ismaelsadeeq commented at 7:53 pm on October 29, 2025: member

Thanks for your review @ryanofsky I will address the comments soon.

ismaelsadeeq renamed this:
~~node: add `BlockTemplateCache`~~
node: add `BlockTemplate{Cache, Manager}`
on Oct 30, 2025

ismaelsadeeq commented at 9:45 am on October 30, 2025: member

I do think while the code changes seem good, it would be good to have more clarity about the cases where cache hits are expected and this cache is most likely to be useful. This seems like something that could be mentioned in the BlockTemplateCache documentation. I know the issue #33389 mentioned a number of cases but it’s unclear if these would be helped by the caching implemented here or if more changes like sharing templates with different options would be required.

If you have more changes building on this, or planned changes, it would be helpful to have a pointer or description. I don’t know if #31664 would benefit from the caching implemented here, for example.

I’ve expanded the PR description to be detailed as much as possible as such IMHO this component will do more than caching, it should be called the Block Template Manager instead.

ismaelsadeeq commented at 10:49 am on October 30, 2025: member

I tend to think even if this PR is only used to improve getblocktemplate and remove global state [1], [2], [3], the changes might be worth it but I’m not sure if you implemented or are planning that.

With the block template manager, we should get rid of these globals especially nTransactionsUpdated Currently, we don’t actually know whether the updated transactions affect the top block template or not, so it’s not a strictly the correct approach.

We should update getblocktemplate to also provide a fee threshold like the mining interface instead of these state and because we already have previously built block templates cached we can easily compute the fee difference and reuse the block template manager’s GetNext method (see the PR description).

Clients won’t need to track the previous state; the block template manager will handle that. This allows us to remove GetTransactionsUpdated from the mempool as well, and with that getblocktemplate logic will be simpler I think.

Removing m_last_block_num_txs and m_last_block_weight from the block assembler will also be possible, since that information can easily be retrieved from the block template manager. I’ll add this to the OP thanks for highlighting these global state.

Sjors commented at 12:16 pm on October 31, 2025: member

Plan

Is this the plan for the current PR? Otherwise maybe it makes more sense to move that to the tracking issue.

ryanofsky commented at 2:42 pm on October 31, 2025: contributor

Thanks for the updated description. This all seems reasonable and is helpful. I do agree with Sjors though it could be good to move the information about next steps into #33389 which could become a tracking issue. Or you could close #33389 and make a new tracking issue (if things have changed since #33389 was opened). Also the current description ends abruptly with “It would be nice to also” so looks like some text is missing.

Other thoughts and suggestions:

As long as caching is implemented here, it would seem make sense to expose it to IPC and RPC clients so if there are multiple clients they could take advantage of the cached templates. Implementing this could be as simple as adding max_age options to BlockCreateOption and getblocktemplate.
It would be nice to add a basic c++ unit test using BlockTemplateCache so if there are changes or bugfixes to this code it is easy to write tests covering them them. The fuzz test is good too, but fuzz tests are more awkward to run. Also, if there any are parts of the fuzz test that don’t really benefit from random inputs, it could make sense to make them unit tests instead.
No strong opinion, but I’m not really sure this code needs to have both BlockTemplateCache and BlockTemplateManager classes. I’m hoping the PR will stick to only introducing a BlockTemplateCache now, and not add extra layers to the implementation until they are justified. I think it’s fine for example if BlockTemplateCache holds a cache and a few stats and counters without needing a separate BlockTemplateManager to hold the counters, unless BlockTemplateManager is going to implement more significant functionality.

ismaelsadeeq force-pushed on Oct 31, 2025

ismaelsadeeq commented at 5:27 pm on October 31, 2025: member

Forced pushed from 5f798b4a3b1a978b8ac954db54e9e8dfc7da6319 to 084bfbc1ec7f8f64f54d231bb641285622311b59 Compare diff dfc7da6319..084bfbc1

Thanks for your thorough review. I’ve made significant changes following your feedback:

Removed the first commit, as it is out of scope for now. https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2379497293
Changed the block_template_cache instance in node interfaces to unique_ptr and properly unregistered it from the validation interface. https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2472926339 https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2472932269
Simplified the implementation of the BlockTemplateCache class as suggested. https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473041954
Clarified why I don’t check that the coinbase script must match, and why we don’t check that block validity must also match. https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473203366
Implemented a comparison operator for block creation options. https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473223014
Removed the template. https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473250518
Removed the incorrect BlockTemplateCache forward declaration in miner. https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473275740
Renamed the generic cache size variable to be more specific. https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473282308
Invalidated the cache after block disconnection and handled background validation block connection. https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473298969
Made IntervalElapsed an inline and standalone function. https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473412280
Added a clarifying comment that we always create a new template when the maximum age is 0. https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473491581
Fixed grammar nits. https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473499003
Made the block template cache own the chainstate instead of the chainman. https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473636918
Moved the max_age variable to block creation options. https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473664992
Made GetBlockTemplate return a shared pointer. https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473682869
Defined a BlocktemplateRef type alias. https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473714111
Deduplicated code as suggested. https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2474149528

Other update

The PR is now using the block.nTime for time comparison.

ismaelsadeeq renamed this:
~~node: add `BlockTemplate{Cache, Manager}`~~
node: add `BlockTemplateCache`
on Oct 31, 2025

ismaelsadeeq commented at 6:38 pm on October 31, 2025: member

Thanks for you recent comment @ryanofsky @Sjors

Is this the plan for the current PR? Otherwise maybe it makes more sense to move that to the tracking issue.

This all seems reasonable and is helpful. I do agree with Sjors though it could be good to move the information about next steps into #33389 which could become a tracking issue. Or you could close #33389 and make a new tracking issue (if things have changed since #33389 was opened). Also the current description ends abruptly with “It would be nice to also” so looks like some text is missing.

Yes done, and also fixed the PR description.

Implementing this could be as simple as adding max_age options to BlockCreateOption and getblocktemplate.

Yes good idea, with the current iteration it can easily be done, however I did not see a use case yet (I did not think hard).

It would be nice to add a basic c++ unit test using BlockTemplateCache so if there are changes or bugfixes to this code it is easy to write tests covering them them. The fuzz test is good too, but fuzz tests are more awkward to run. Also, if there any are parts of the fuzz test that don’t really benefit from random inputs, it could make sense to make them unit tests instead.

Unit test is nice to demonstrate usage and test some cases, but I really like the coverage fuzz test provide like catching some subtle bugs that could be missed in review.

No strong opinion, but I’m not really sure this code needs to have both BlockTemplateCache and BlockTemplateManager classes. I’m hoping the PR will stick to only introducing a BlockTemplateCache now, and not add extra layers to the implementation until they are justified. I think it’s fine for example if BlockTemplateCache holds a cache and a few stats and counters without needing a separate BlockTemplateManager to hold the counters, unless BlockTemplateManager is going to implement more significant functionality.

You are right, i’ve limit this to just the cache, we can talk about the block template manager when their is code to review and debate on the best approach.

I see some fuzz test are failing, I will work on fixing it and also will make commit atomic per some of @ryanofsky suggestions

in src/node/interfaces.cpp:914 in cd2e843907 outdated

910@@ -912,20 +911,21 @@ class BlockTemplateImpl : public BlockTemplate
911 
912     bool submitSolution(uint32_t version, uint32_t timestamp, uint32_t nonce, CTransactionRef coinbase) override
913     {
914-        AddMerkleRootAndCoinbase(m_block_template->block, std::move(coinbase), version, timestamp, nonce);
915-        return chainman().ProcessNewBlock(std::make_shared<const CBlock>(m_block_template->block), /*force_processing=*/true, /*min_pow_checked=*/true, /*new_block=*/nullptr);
916+        auto block_copy = m_block_template->block;

ryanofsky commented at 1:56 pm on November 3, 2025:

In commit “node: add block template cache to miner” (cd2e843907a6810c8f1acf2b7d80bbf8273a5a00)

Maybe use auto block_copy = std::make_shared<const CBlock>(m_block_template->block); here to avoid needing to make a shared_ptr and a second copy of the block below.

ismaelsadeeq commented at 5:19 pm on November 5, 2025:

This is not possible I think because we need to pass a non const block further down. I instead avoid the second copy by moving. Let me know what you think

in src/node/miner.h:201 in cd2e843907 outdated

196+        // that the cumulative weight and sigops (even if the coinbase output script differs)
197+        // remain within acceptable limits.
198+        //
199+        // We also don’t compare whether block validity has been checked,
200+        // since validity can always be tested again.
201+        bool operator==(const Options& other) const

ryanofsky commented at 2:01 pm on November 3, 2025:

In commit “node: add block template cache to miner” (cd2e843907a6810c8f1acf2b7d80bbf8273a5a00)

I know I suggested operator== previously but since this is not checking direct equality maybe it is better to call this method something like CanReuse or ReusableTemplate. Just an idea. Keeping operator== or using any method name seems ok.

Would also suggest moving detailed comments about specific fields inisde the method implementation. Could just have a general comment like “intentionally omits some fields from comparison” in function documentation.

in src/node/miner.h:290 in cd2e843907 outdated

285+    size_t m_block_template_cache_size;
286+    mutable Mutex m_mutex;
287+
288+    BlockTemplateRef CreateBlockTemplateInternal(const BlockAssembler::Options& options) EXCLUSIVE_LOCKS_REQUIRED(m_mutex);
289+public:
290+    BlockTemplateCache(CTxMemPool* mempool, Chainstate& chainstate, size_t block_template_block_template_cache_size = DEFAULT_BLOCK_TEMPLATE_CACHE_SIZE);

ryanofsky commented at 2:06 pm on November 3, 2025:

In commit “node: add block template cache to miner” (cd2e843907a6810c8f1acf2b7d80bbf8273a5a00)

Should mempool be a reference rather if this class can’t really function when it is null?

in src/node/miner.h:256 in cd2e843907 outdated

250@@ -220,6 +251,62 @@ class BlockAssembler
251     void SortForBlock(const CTxMemPool::setEntries& package, std::vector<CTxMemPool::txiter>& sortedEntries);
252 };
253 
254+// Return true if current time is greater or equal to `prev_time + interval`, or if
255+// `prev_time` is greater than the current time (indicating clock moved backward).
256+static inline bool IntervalElapsed(const uint32_t& prev_time, const uint32_t& interval)

ryanofsky commented at 2:10 pm on November 3, 2025:

In commit “node: add block template cache to miner” (cd2e843907a6810c8f1acf2b7d80bbf8273a5a00)

Probably should use uint32_t instead of const uint32_t&. Const reference parameter are usually only preferred when parameter types are much bigger.

in src/node/miner.h:190 in cd2e843907 outdated

185@@ -176,6 +186,27 @@ class BlockAssembler
186         // Whether to call TestBlockValidity() at the end of CreateNewBlock().
187         bool test_block_validity{true};
188         bool print_modified_fee{DEFAULT_PRINT_MODIFIED_FEE};
189+        // By default always return a fresh template.
190+        std::chrono::seconds maximum_template_age{0};

ryanofsky commented at 2:12 pm on November 3, 2025:

In commit “node: add block template cache to miner” (cd2e843907a6810c8f1acf2b7d80bbf8273a5a00)

Maybe s/maximum_/max_/

ryanofsky approved

ryanofsky commented at 2:30 pm on November 3, 2025: contributor

Code review ACK 084bfbc1ec7f8f64f54d231bb641285622311b59 reviewing first commit in detail, but reviewed fuzz test in second commit pretty lightly. Changes here all seem very straightforward and implmenetation seems simpler than it was initially.

I do still suspect specifying template age in seconds instead of using a more granular measure may be a mistake if this caching is supposed to be useful for mining, but others would know more about this than me. It would seem straightfoward to add another field and nice to move away from uint32_t nTime anyway to take advantage of better c++ types.

Also still think it could be good to have a least 1 or 2 unit tests and try simplify the fuzz test so all tests are easier to understand and run.

brunoerg commented at 2:53 pm on November 4, 2025: contributor

I left the block_template_cache fuzz target running overnight and generated a coverage report for it: https://brunoerg.xyz/bitcoin-core-coverage/33421/coverage_report/

in src/test/fuzz/blocktemplatecache.cpp:110 in 084bfbc1ec

105+        modified_options.maximum_template_age = std::chrono::seconds::max();
106+        auto third_template = template_cache->GetBlockTemplate(modified_options);
107+        assert(third_template->block.nTime > second_template->block.nTime);
108+
109+        // Same original options must hit the second template in the cache
110+        base_options.maximum_template_age = std::chrono::seconds::max();

brunoerg commented at 3:02 pm on November 4, 2025:

084bfbc1ec7f8f64f54d231bb641285622311b59: I think the CI failure is related to this std::chrono::seconds::max() (long long) because the maximum_template_age is used in IntervalElapsedas the interval which is a uint32_t.

ismaelsadeeq commented at 3:58 pm on November 4, 2025: member

Thanks for the review @ryanofsky and the coverage report @brunoerg

I will update the PR shortly with the suggestions and study the report.

in src/test/fuzz/blocktemplatecache.cpp:63 in 084bfbc1ec

58+                          return g_setup->m_node.chainman->ActiveTip()->Time()));
59+
60+    int32_t max_mempool_weight = fuzzed_data_provider.ConsumeIntegralInRange<int32_t>(0, DEFAULT_BLOCK_MAX_WEIGHT * 10);
61+    PopulateRandTransactionsToMempool(fuzzed_data_provider, node.mempool.get(), max_mempool_weight);
62+    auto num_iterations = fuzzed_data_provider.ConsumeIntegralInRange<unsigned int>(1, 10);
63+    LIMITED_WHILE (fuzzed_data_provider.remaining_bytes(), num_iterations) {

brunoerg commented at 4:27 pm on November 4, 2025:

084bfbc1ec7f8f64f54d231bb641285622311b59: nit: you can get rid of num_iterations.

0    LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10) {

brunoerg commented at 5:37 pm on November 4, 2025: contributor

Unit test is nice to demonstrate usage and test some cases, but I really like the coverage fuzz test provide like catching some subtle bugs that could be missed in review.

I agree with @ryanofsky about having unit test - fuzzing doesn’t exclude the need of having unit tests. Fuzzing depends on the provided inputs, unit tests are usually faster, great for regression, demonstrate usage (especially edge cases), etc. Also, the general goal of fuzzing is catching assertion failures, integer overflows, memory violations, etc - but we usually might leverage it by writing harnesses that can check some behavior things - e.g. we can put some metamorphic relations, round-trip assertions, and others (see An empirical evaluation of fuzz targets using mutation testing) - as you did here. However, we shouldn’t sacrifice performance in favor of it - fuzzing is search. Your fuzz target is good but note that for every single iteration you call GetBlockTemplate several times as well as some BlockConnected/BlockDisconnected to test the cache for every scenario - on my server it barely got more than 50 exec/s.

miner: add `TimeIntervalElapsed` function

- This function returns true if current time is greater or equal to `prev_time + time_interval`
  or if `prev_time` is greater than the current time (indicating clock moved backward).

a8a0218645

miner: add block creation time point to `CBlockTemplate` d9a0094c39

miner: add `SimilarOptions` function

- This function checks whether two block assembler options are similar

92b3ef5b92

miner: add `max_template_age` to `BlockAssember::Options` 98bd551e3e

ismaelsadeeq force-pushed on Nov 5, 2025

DrahtBot added the label CI failed on Nov 5, 2025

ismaelsadeeq force-pushed on Nov 5, 2025

ismaelsadeeq commented at 5:43 pm on November 5, 2025: member

I forced pushed from 084bfbc1ec7f8f64f54d231bb641285622311b59 to a3f0010c2062fc88720e7eae33694f2491c32ebe 2311b59..a3f001

We now avoid the second copy when submitting solution by moving the block by value when constructing the shared pointer #33421 (review)
Removed the operator= in block assembler options and instead introduce a SimilarOptions function, I think it’s better this way because I plan to extend this logic later by indicating whenever options are similar but it’s just the block max weight that differ, this is useful for enabling reusing a larger template in the cache when a smaller one is needed with same matching other options. #33421 (review)
Updated the mempool object in the cache to be stored by reference not pointer #33421 (review)
Make TimeIntervalElapsed parameter time_interval be copied by value. #33421 (review)
Update maximum_template_age to max_template_age #33421 (review)
Fix previous C.I failure by not having to deal with raw seconds value, we use MillisecondsDouble instead #33421 (review)
Get rid of num_iterations variable #33421 (review)
Fixed #33421 (comment) and #33421#pullrequestreview-3411154210 comment that requested for a unit test. @brunoerg thanks the resource it’s helpful.

ismaelsadeeq force-pushed on Nov 5, 2025

maflcko removed the label CI failed on Nov 5, 2025

DrahtBot added the label CI failed on Nov 5, 2025

DrahtBot commented at 8:48 pm on November 5, 2025: contributor

🚧 At least one of the CI tasks failed. Task macOS native, no depends, sqlite only, gui: https://github.com/bitcoin/bitcoin/actions/runs/19111349459/job/54623138521 LLM reason (✨ experimental): miner_tests test suite failed (CTest reported 1 failure).

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.

ismaelsadeeq force-pushed on Nov 6, 2025

miner: add block template cache f05b22c38c

ismaelsadeeq force-pushed on Nov 6, 2025

test: add unit test to `BlockTemplateCache` 048a768203

interfaces: create block template via block template cache 4ed5e39084

test: add blocktemplatecache fuzz test cf05272bb7

ismaelsadeeq force-pushed on Nov 6, 2025

ismaelsadeeq commented at 3:34 pm on November 6, 2025: member

Apologies for the force pushes, I am attempting to fix a C.I failure which happens only remotely due to SetMockTime not able to nudge the node clock in ms, I adjusted the time advances to be in seconds with a large buffer

node: add BlockTemplateCache #33421

Code Coverage & Benchmarks

Reviews

Conflicts

node: add `BlockTemplateCache` #33421