node: add BlockTemplateCache #33421

pull ismaelsadeeq wants to merge 7 commits into bitcoin:master from ismaelsadeeq:09-2025-minerman changing 18 files +531 −52
  1. ismaelsadeeq commented at 6:47 pm on September 17, 2025: member

    This PR implements the first step of #33758 by adding a cache layer for node block templates.

    The cache stores newly created block templates along with their configuration options. When clients request a block template, they specify the maximum age in MillisecondsDouble:

    • If a matching template (block template with identical block creation option) exists in the cache and hasn’t exceeded the age limit, the cached version is returned (if multiple exist, the most recent one is returned).
    • If no matching template exists or the age limit has been exceeded, a new template is generated, cached, and returned.

    The cache maintains a configurable maximum size (default: 10 templates). When this limit is exceeded, the oldest template is evicted. This is usually checked after each insertion.

    The cache subscribes to the validation interface and clears itself when blocks are connected (to non-historical chainstate to avoid clearing during assumeutxo background validation) or disconnected from the active chain (during reorgs), preventing stale or invalid templates from being served.

    This PR also removes an unused node/context.h import from miner to prevent a potential circular dependency during exposing the block template cache to the node context.

  2. DrahtBot renamed this:
    node: add `BlockTemplateCache`
    node: add `BlockTemplateCache`
    on Sep 17, 2025
  3. 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
    Concept ACK Sjors
    Stale ACK 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)
    • #33966 (refactor: disentangle miner startup defaults from runtime options by Sjors)
    • #33922 (mining: add getMemoryLoad() and track template non-mempool memory footprint by Sjors)
    • #32420 (mining, ipc: omit dummy extraNonce from coinbase by Sjors)
    • #31382 (kernel: Flush in ChainstateManager destructor by sedited)

    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.

    LLM Linter (✨ experimental)

    Possible typos and grammar issues:

    +is evicted. -> (remove duplicate “is evicted.”) [Duplicate sentence fragment; the phrase “is evicted.” is repeated, making the sentence redundant/confusing.]

    2026-01-29 20:58:29

  4. ismaelsadeeq force-pushed on Sep 17, 2025
  5. ismaelsadeeq force-pushed on Sep 17, 2025
  6. ismaelsadeeq force-pushed on Sep 23, 2025
  7. ismaelsadeeq commented at 4:45 pm on September 23, 2025: member

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

    Main Changes are

    1. Moved the BlockTemplateCache to src/node/miner.cpp to fix the circular dependency issue
    2. The response now return the block template creation time
    3. We search for match from right to left to return the fresh hit first
    4. Added a fuzz test that test various cache condition and add more coverage to the BlockAssembler Code.
  8. 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.

  9. ismaelsadeeq force-pushed on Sep 24, 2025
  10. glozow added the label Bug on Sep 24, 2025
  11. glozow added the label Needs backport (29.x) on Sep 24, 2025
  12. glozow added the label Needs backport (30.x) on Sep 24, 2025
  13. glozow removed the label Bug on Sep 24, 2025
  14. glozow removed the label Needs backport (29.x) on Sep 24, 2025
  15. glozow removed the label Needs backport (30.x) on Sep 24, 2025
  16. 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

  17. fanquake referenced this in commit 05d984b1a4 on Sep 25, 2025
  18. DrahtBot added the label Needs rebase on Sep 25, 2025
  19. 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.

  20. ismaelsadeeq force-pushed on Sep 25, 2025
  21. DrahtBot removed the label Needs rebase on Sep 25, 2025
  22. DrahtBot added the label Needs rebase on Oct 23, 2025
  23. ismaelsadeeq force-pushed on Oct 23, 2025
  24. ismaelsadeeq force-pushed on Oct 27, 2025
  25. DrahtBot removed the label Needs rebase on Oct 27, 2025
  26. 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.

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

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

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

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

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

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

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

  35. 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}
  36. in src/node/miner.h:184 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.

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

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

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

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

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

  42. 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
  43. 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         }
  44. ryanofsky approved
  45. 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.

  46. ismaelsadeeq commented at 7:53 pm on October 29, 2025: member
    Thanks for your review @ryanofsky I will address the comments soon.
  47. ismaelsadeeq renamed this:
    node: add `BlockTemplateCache`
    node: add `BlockTemplate{Cache, Manager}`
    on Oct 30, 2025
  48. 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.

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

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

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

  52. ismaelsadeeq force-pushed on Oct 31, 2025
  53. 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:

    Other update

    • The PR is now using the block.nTime for time comparison.
  54. ismaelsadeeq renamed this:
    node: add `BlockTemplate{Cache, Manager}`
    node: add `BlockTemplateCache`
    on Oct 31, 2025
  55. 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

  56. in src/node/interfaces.cpp:913 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
  57. 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.

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

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

  60. 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_/

  61. ryanofsky approved
  62. 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.

  63. 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/
  64. 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.
  65. 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.

  66. 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) {
  67. 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.

  68. ismaelsadeeq force-pushed on Nov 5, 2025
  69. ismaelsadeeq force-pushed on Nov 5, 2025
  70. DrahtBot added the label CI failed on Nov 5, 2025
  71. ismaelsadeeq force-pushed on Nov 5, 2025
  72. 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.
  73. ismaelsadeeq force-pushed on Nov 5, 2025
  74. maflcko removed the label CI failed on Nov 5, 2025
  75. DrahtBot added the label CI failed on Nov 5, 2025
  76. 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.

  77. ismaelsadeeq force-pushed on Nov 6, 2025
  78. ismaelsadeeq force-pushed on Nov 6, 2025
  79. ismaelsadeeq force-pushed on Nov 6, 2025
  80. ismaelsadeeq force-pushed on Nov 6, 2025
  81. 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
  82. DrahtBot added the label Needs rebase on Nov 10, 2025
  83. ismaelsadeeq force-pushed on Nov 10, 2025
  84. DrahtBot removed the label Needs rebase on Nov 11, 2025
  85. in src/node/miner.cpp:219 in 2ef4370399 outdated 
    183@@ -184,6 +184,7 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock()
184             throw std::runtime_error(strprintf("TestBlockValidity failed: %s", state.ToString()));
185         }
186     }
187+    pblocktemplate->m_creation_time = NodeClock::now();
    Sjors commented at 12:29 pm on November 11, 2025:

    In 2ef43703998f679fc752a77727dd5c0824f6a90b miner: add block creation time point to CBlockTemplate: the most relevant anchor for creation time is what was in the mempool at that moment. So I would suggest moving this above addPackageTxs.

    Since cs_main is locked throughout this function, this currently doesn’t make a difference, but if we ever make locks more granular I could imagine taking a lock on just the mempool and setting the template timestamp at that exact moment.

    (Something like TestBlockValidity shouldn’t have to block cs_main, although we do need to block tip updates between the moment we start adding transactions to the template and when we finish verification. Although even then, block validation could abort with failure if the tip updates during this.)

    Nit: could drop “block” from commit message, so it’s not confused with nTime.

    ismaelsadeeq commented at 5:08 pm on January 13, 2026:

    I’ve been thinking about this for a bit; and would advocate for more granular and TestBlockValidity no longer requires holding the lock. It’s going to be beneficial not just here #32317 (review)

    This seems to me outside the scope of this PR, since the current locking guarantees equivalence even after the suggestion. leaving as is for now thanks.

  86. in src/node/miner.cpp:337 in a620c0a69e outdated 
    450+    // that the cumulative weight and sigops (even if the coinbase output script differs)
451+    // remain within acceptable limits.
452+    //
453+    // We also don’t compare whether block validity has been checked,
454+    // since validity can always be tested again.
455+    // Note: we should update this helper when the constraint changes.
    Sjors commented at 12:36 pm on November 11, 2025:

    In a620c0a69e6d2b0d29f0714f2c6c0bff33e2b4bf miner: add SimilarOptions function: both these comments should be in the header, as they explain both what this function does and why it’s useful.

    Maybe call it ShareableOptions, described as:

    Limit comparison to assembly options that impact the final block (without dummy coinbase scriptPubKey, with solution). Ignore options that can safely be shared between different callers.

    ismaelsadeeq commented at 4:56 pm on January 13, 2026:
    Taken, thanks
  87. in src/node/miner.cpp:460 in a620c0a69e outdated 
    455+    // Note: we should update this helper when the constraint changes.
456+    return a.use_mempool == b.use_mempool &&
457+            a.block_reserved_weight == b.block_reserved_weight &&
458+            a.blockMinFeeRate == b.blockMinFeeRate &&
459+            a.coinbase_output_max_additional_sigops == b.coinbase_output_max_additional_sigops &&
460+            a.nBlockMaxWeight == b.nBlockMaxWeight;
    Sjors commented at 12:44 pm on November 11, 2025:

    In a620c0a69e6d2b0d29f0714f2c6c0bff33e2b4bf miner: add SimilarOptions function: this seems brittle when we add options later.

    Perhaps you can copy a, then set the shareable properties of a_copy to that of b and then do a regular comparison.

    ismaelsadeeq commented at 4:57 pm on January 13, 2026:
    I did not take this because there is no regular comparison operator defined, and I prefer to be explicit here.
  88. Sjors commented at 1:02 pm on November 11, 2025: member

    The following in the PR description had me confused:

    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:

    I thought it meant that templates have an expiration time, but from 8af9ede0608f5bb4fe3eddf7d3fe4fbb57a4649d it’s clear that the caller specifies how old cached templates can be (via max_template_age).

    If you’re having trouble with mock time in the tests / fuzzer, it might help to add some direct tests to TimeIntervalElapsed in 3ed16ab58f5773113efdd973d94911c93459637e.

    Commit ddbad03dddeba255b016622d4569aecbf74b9366 miner: add block template cache introduces quite a lot stuff, without explanation. E.g. I was not expecting to see validation_signals as relevant for a cache. It would be good to expand the commit message. Maybe it’s better to split this commit, and then introduce the unit tests from e45a195def5f8be0793ba585f49e4d5ff5c8b286 gradually to explain where the complexity comes from.

    I wonder if it makes sense to introduce and use BlockTemplateRef as a pure refactor in an earlier commit. That way 9500dca555f7394cd3e51821312309d5b9633539 interfaces: create block template via block template cache just needs to change the type from std::unique_ptr to std::shared_ptr making that commit smaller.

  89. in src/node/interfaces.cpp:913 in 9500dca555 outdated 
    915@@ -917,14 +916,15 @@ class BlockTemplateImpl : public BlockTemplate
916 
917     bool submitSolution(uint32_t version, uint32_t timestamp, uint32_t nonce, CTransactionRef coinbase) override
918     {
919-        AddMerkleRootAndCoinbase(m_block_template->block, std::move(coinbase), version, timestamp, nonce);
920-        return chainman().ProcessNewBlock(std::make_shared<const CBlock>(m_block_template->block), /*force_processing=*/true, /*min_pow_checked=*/true, /*new_block=*/nullptr);
921+        auto block_copy = m_block_template->block;
    Sjors commented at 1:13 pm on November 11, 2025:

    In 9500dca555f7394cd3e51821312309d5b9633539 interfaces: create block template via block template cache: how does this interact with #33745? The third commit there adds a comment and test that submitSolution() intentionally modifies the block in place, so that the client can call getBlock() afterwards.

    I don’t mind changing that behavior, but then I’ll need to revive #33374 or change submitSolution() to optionally return the full block.

    ismaelsadeeq commented at 2:12 pm on November 11, 2025:
    No need to revive #33374. I saw it yesterday and will push an update here soon. We already made a copy of the block here; we should copy the whole template after WaitAndCreateNewBlock returns and allow modifications as mentioned in #33374. It is a cheap copy.
    Sjors commented at 3:06 pm on November 11, 2025:
    In other words, each newly requested temple is copied from the cache? Sounds good.
    ismaelsadeeq commented at 3:13 pm on November 11, 2025:
    If you want to modify the template, yes. But for things like sendtemplate, fee estimation, that just want to retrieve data from the template they don’t need a copy.
  90. ismaelsadeeq commented at 8:52 pm on November 11, 2025: member

    I really appreciate the feedback and reviews so far from @ryanofsky, @Sjors, and @brunoerg. Given that I’ve opened #33758 with the overall plan and we’ve had some recent discussions #33756 (comment), I’ll work on fixing the C. I issue here hence marking as draft.

    I will also work on the e2e implementation separately.

  91. ismaelsadeeq marked this as a draft on Nov 11, 2025
  92. DrahtBot added the label Needs rebase on Nov 25, 2025
  93. ismaelsadeeq force-pushed on Dec 30, 2025
  94. DrahtBot removed the label Needs rebase on Dec 30, 2025
  95. ismaelsadeeq force-pushed on Dec 30, 2025
  96. ismaelsadeeq force-pushed on Jan 13, 2026
  97. DrahtBot removed the label CI failed on Jan 13, 2026
  98. DrahtBot added the label Needs rebase on Jan 13, 2026
  99. ismaelsadeeq force-pushed on Jan 13, 2026
  100. ismaelsadeeq marked this as ready for review on Jan 13, 2026
  101. ismaelsadeeq commented at 5:12 pm on January 13, 2026: member
    Thanks for your reviews @Sjors @ryanofsky, and @brunoerg. I have split the commits to be more atomic and added a description based on @Sjors suggestion and rebased.
  102. ismaelsadeeq force-pushed on Jan 13, 2026
  103. DrahtBot added the label CI failed on Jan 13, 2026
  104. DrahtBot removed the label Needs rebase on Jan 13, 2026
  105. DrahtBot removed the label CI failed on Jan 13, 2026
  106. in src/node/miner.h:140 in 107283802e 
    133@@ -134,6 +134,13 @@ class BlockAssembler
134     bool TestChunkTransactions(const std::vector<CTxMemPoolEntryRef>& txs) const;
135 };
136 
137+/**
138+ * Helper to check whether two block assembler options are similar.
139+ * Limit comparison to assembly options that impact the final block (without dummy coinbase
140+ * scriptPubKey, with solution). Ignore options that can safely be shared between different callers.
    Sjors commented at 1:28 pm on January 15, 2026:
    In 107283802e77bb75a621943fb05729914628681c miner: add ShareableOptions function: what do you mean by “with solution”?
    ismaelsadeeq commented at 2:46 pm on January 16, 2026:

    You are right, what does solution mean here?

    fwiw this is taken from your suggestion #33421 (review), reverted.

  107. in src/node/miner.h:152 in 107283802e outdated 
    133@@ -134,6 +134,13 @@ class BlockAssembler
134     bool TestChunkTransactions(const std::vector<CTxMemPoolEntryRef>& txs) const;
135 };
136 
137+/**
138+ * Helper to check whether two block assembler options are similar.
139+ * Limit comparison to assembly options that impact the final block (without dummy coinbase
140+ * scriptPubKey, with solution). Ignore options that can safely be shared between different callers.
141+ */
142+bool ShareableOptions(const BlockAssembler::Options& a, const BlockAssembler::Options& b);
    Sjors commented at 1:33 pm on January 15, 2026:

    In 107283802e77bb75a621943fb05729914628681c miner: add ShareableOptions function: the current same suggests that it’s taking both options and returning the shareable ones, but it only returns a bool.

    Some other names that might make the intention more clear:

    • AreOptionsEquivalent()
    • HasDifferentOption() (reversing the check)
    • HasEquivalentTemplateOptions()

    (none of these are great though)

    ismaelsadeeq commented at 2:50 pm on January 16, 2026:

    I think this is getting bikesheddy because this is also your suggestion that i take here #33421 (review)

    I opt for shareableOptions because any block template generated by the two block creation options can be shared between clients who want a block template with either of the options. I will leave it as is and take a better suggestion. I added a comment to be clearer.

    Sjors commented at 2:35 pm on January 23, 2026:

    An AI overlord suggested CanReuseBlockTemplate(), which looks good:

    0for (auto it = m_block_templates.rbegin(); it != m_block_templates.rend(); it++) {
1    if (CanReuseBlockTemplate(it->first, options) && !TimeIntervalElapsed(it->second->m_creation_time, options.max_template_age)) {

    (not super important of course)

    ismaelsadeeq commented at 7:06 pm on January 26, 2026:
    This is gone in the latest push.
  108. in src/node/miner.cpp:333 in 107283802e outdated 
    328+{
329+    // We intentionally do not compare the coinbase output script.
330+    // It’s acceptable for them to differ because, as long as the reserved
331+    // weight and additional sigops for the coinbase match, we can assume
332+    // that the cumulative weight and sigops (even if the coinbase output script differs)
333+    // remain within acceptable limits.
    Sjors commented at 1:36 pm on January 15, 2026:

    In 107283802e77bb75a621943fb05729914628681c miner: add ShareableOptions function: I think you can replace the above with just:

    0// The dummy coinbase output script is not compared.
    ismaelsadeeq commented at 4:11 pm on January 16, 2026:
    Why should we delete it? I’d rather be explicit here and explain why we are skipping some options; I consider this type of comment to be very useful, as it explains the rationale for some logic. A better solution, as we discussed IRL, is to categorise the options into those that are safe to ignore during comparison and those that are consequential, as a struct that BlockAssembler::Options will derive from, see https://github.com/ismaelsadeeq/bitcoin/commit/650e9cf85d6680c37f3805bdfb4291ea9517226d for an attempt . This would be less brittle; however, it would increase the scope of the PR a bit. Let me know what you think
    ryanofsky commented at 1:42 pm on January 21, 2026:

    re: #33421 (review)

    Why should we delete it?

    Apparently I asked for this comment previously #33421 (review), and I do think some comment here is useful because to me it’s not obvious which options make templates shareable or unshareable.

    Like I’m still not sure why it’s ok for GetBlockTemplate to return a template with a different coinbase output script than was requested. The current GetBlockTemplate documentation says a “cached template exists with identical options” is returned, but this is only checking similar options not identical options. It would be good to understand more and hopefully make this clearer

    Sjors commented at 12:34 pm on January 23, 2026:

    why it’s ok for GetBlockTemplate to return a template with a different coinbase output script than was requested

    Only (some) tests actually use the coinbase output script. Actual miners don’t use it:

    1. the getblocktemplate RPC doesn’t return it
    2. the new getCoinbase() since #33819 also doesn’t return it (and previously sv2-tp would ignore it and not pass it downstream)

    The tests that do use it can probably be modified to stop doing that, if the new caching mechanism breaks them. That might involve re-generating test blocks, which is tedious, so hopefully we don’t need to for now.

    Sjors commented at 1:04 pm on January 23, 2026:

    Maybe add:

     0diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp
 1index 6e384efa05..2152ee4ebd 100644
 2--- a/src/test/miner_tests.cpp
 3+++ b/src/test/miner_tests.cpp
 4@@ -120,6 +120,9 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const
 5     auto mining{MakeMining()};
 6     BlockAssembler::Options options;
 7     options.coinbase_output_script = scriptPubKey;
 8+    // BlockTemplateCache does not key on coinbase_output_script, so
 9+    // explicitly opt-out
10+    options.max_template_age = MillisecondsDouble{0};
11
12     LOCK(tx_mempool.cs);
13     BOOST_CHECK(tx_mempool.size() == 0);
14@@ -338,6 +341,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std::
15
16     BlockAssembler::Options options;
17     options.coinbase_output_script = scriptPubKey;
18+    options.max_template_age = MillisecondsDouble{0};
19
20     {
21         CTxMemPool& tx_mempool{MakeMempool()};
22@@ -664,6 +668,7 @@ void MinerTestingSetup::TestPrioritisedMining(const CScript& scriptPubKey, const
23
24     BlockAssembler::Options options;
25     options.coinbase_output_script = scriptPubKey;
26+    options.max_template_age = MillisecondsDouble{0};
27
28     CTxMemPool& tx_mempool{MakeMempool()};
29     LOCK(tx_mempool.cs);
30@@ -753,6 +758,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
31     CScript scriptPubKey = CScript() << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f"_hex << OP_CHECKSIG;
32     BlockAssembler::Options options;
33     options.coinbase_output_script = scriptPubKey;
34+    options.max_template_age = MillisecondsDouble{0};
35
36     // Create and check a simple template
37     std::unique_ptr<BlockTemplate> block_template = mining->createNewBlock(options);
    ismaelsadeeq commented at 7:08 pm on January 26, 2026:
    I simplify this and just reuse when they are completely identical.
  109. Sjors commented at 1:44 pm on January 15, 2026: member

    I still need to review miner: introduce BlockTemplateCache for reusing shareable block templates and later. Some notes so far:

    The trivial commits 2099bdb903dfa042eb61f748b548d3c31121e335 and 522fcc5734062302dff19e9149aa4d011999d39f should probably be squashed into the first commit that needs the variable they introduce.

    I suggest making 7370591a9a116d3e4eb1e7a9b974e41ecf98af93 the first commit. Good chance some other PR takes it before this one.

  110. ismaelsadeeq force-pushed on Jan 16, 2026
  111. ismaelsadeeq commented at 4:13 pm on January 16, 2026: member

    Thanks for your review @Sjors

    The trivial commits https://github.com/bitcoin/bitcoin/commit/2099bdb903dfa042eb61f748b548d3c31121e335 and https://github.com/bitcoin/bitcoin/commit/522fcc5734062302dff19e9149aa4d011999d39f should probably be squashed into the first commit that needs the variable they introduce.

    I suggest making https://github.com/bitcoin/bitcoin/commit/7370591a9a116d3e4eb1e7a9b974e41ecf98af93 the first commit. Good chance some other PR takes it before this one.

    This is fixed now. see diff 8b8f2c2211e6f751994f446ba1fdeea2b0d4f22d..a4dab3f6e092c0f6b7f8d92f30a4e38309c5de19

  112. in src/node/miner.h:58 in e8064a0c24 
    54@@ -55,6 +55,7 @@ struct CBlockTemplate
55      * miner code.
56      */
57     CoinbaseTx m_coinbase_tx;
58+    NodeClock::time_point m_creation_time;
    ryanofsky commented at 1:14 pm on January 21, 2026:

    In commit “miner: add block creation time point to CBlockTemplate” (e8064a0c24a55aa3202ad5f0ea43b3fbcc7bad09)

    Would be helpful to have a code comment saying what this variable is used for. Also wondering if it should use steady_clock instead of system_clock. E.g. it might be bad if a clock change caused code to prefer older templates.

    Sjors commented at 1:16 pm on January 23, 2026:

    As long as the cache is wiped when the tip changes, I don’t think we need to worry about this.

    I assume clock adjustments are rare and typically tiny. If we end up mining the previous template, we’ll lose a few sats, but we’ll quickly generate a fresh one as fees rise.

    Sjors commented at 1:24 pm on January 23, 2026:

    Using NodeClock is also consistent with validation:

    0// Check timestamp
1if (block.Time() > NodeClock::now() + std::chrono::seconds{MAX_FUTURE_BLOCK_TIME}) {
2    return state.Invalid(BlockValidationResult::BLOCK_TIME_FUTURE, "time-too-new", "block timestamp too far in the future");
3}
    ryanofsky commented at 7:02 pm on January 26, 2026:

    re: #33421 (review)

    I agree those things make using system_clock less bad than it would otherwise seem, but they don’t seem like reasons to prefer system_clock over steady_clock.

    Validation code is doing something pretty different and needs to consider network wide effects. But this code just needs to look at templates it created itself and accurately return how long ago it created them, and which ones it created first. It seems like using steady_clock would allow it to do this predictably and reliably and with the least amount of complexity and using system_clock only brings downsides. Possible I might be missing something though.

    ismaelsadeeq commented at 7:09 pm on January 26, 2026:
    This is a good idea, but it would still not be nice to lose a few sats. So I updated to a mockable steady clock and added the requested comment.
  113. ryanofsky approved
  114. ryanofsky commented at 2:11 pm on January 21, 2026: contributor

    Code review a4dab3f6e092c0f6b7f8d92f30a4e38309c5de19. This looks simpler than I remember it and thanks for addressing all previous comments, especially adding the new unit test which is easy to follow. The PR description and next steps listed in the tracking issue are also pretty clear. I might just clarify in the PR description where it says “when clients request a block template, they specify the maximum age” this is only referring to BlockTemplateCache clients, not IPC or RPC clients since they do not currently have a way to specify maximum age and followups will presumably add this.

    I need to review this in more detail and understand ShareableOptions better (I left another question about it below) but overall this seems like it should be useful and is a pretty straightforward change.

  115. Sjors commented at 4:44 pm on January 23, 2026: member
    I have light preference for making max_template_age an argument for GetBlockTemplate() (default 0) instead of adding it to BlockAssembler::Options. Especially because it’s ignored by ShareableOptions and then checked separately in GetBlockTemplate().
  116. in src/node/miner.cpp:369 in bf202ec043 
    364+                if (BlockValidationState state{TestBlockValidity(m_chainman.ActiveChainstate(), it->second->block,
365+                                                                 /*check_pow=*/false, /*check_merkle_root=*/false)};
366+                    !state.IsValid()) {
367+                    throw std::runtime_error(strprintf("TestBlockValidity failed: %s", state.ToString()));
368+                }
369+                it->first.test_block_validity = true;
    Sjors commented at 4:52 pm on January 23, 2026:

    bf202ec0438dfc18969ff9bf6ea320e4a3b6a553: I don’t think we should be tracking block check status in an options variable like this. We should probably put it->first in a const variable to prevent even the temptation.

    This should be tracked in CBlock, which already takes care of resetting its cache when things are mutated, e.g. see AddMerkleRootAndCoinbase.

    Sjors commented at 5:28 pm on January 23, 2026:

    Since test_block_validity is only set to false in tests / bench, I think we should make our lives easier and never add templates to the cache if test_block_validity is false.

    (it is however to take a template from the cache)

    ismaelsadeeq commented at 7:10 pm on January 26, 2026:
    You are right, this is more of my motivation for just reusing when it’s completely identical. This simplifies a lot in the PR.
  117. in src/node/miner.cpp:342 in 72144fdafe outdated 
    355+    LOCK(m_mutex);
356+    // Clear the cached blocks when the newly connected block
357+    // is not connecting to a historical chain that is used to validate
358+    // the current active chain.
359+    if (!role.historical) {
360+        m_block_templates.clear();
    Sjors commented at 4:55 pm on January 23, 2026:
    72144fdafeaa7576cc7aedeb4d59d84c29ff0238: remind me, this only clears the cache right? Previous callers are not impacted if they hold another shared pointer. Might be worth expanding the comment, especially in the context of IPC clients.
    ismaelsadeeq commented at 8:23 pm on January 26, 2026:

    You are right, it will still be there. I think this is obvious with shared_ptr; once you make a copy, your version exists even if the original version of the shared_ptr is deleted.

    For the IPC, it is safe because the client copies the whole object, not just the shared_ptr. see 63cfe6f

  118. in src/node/miner.cpp:348 in 72144fdafe outdated 
    364+void BlockTemplateCache::BlockDisconnected(
365+    const std::shared_ptr<const CBlock>& /*unused*/,
366+    const CBlockIndex* /*unused*/)
367+{
368+    LOCK(m_mutex);
369+    m_block_templates.clear();
    Sjors commented at 5:00 pm on January 23, 2026:

    72144fdafeaa7576cc7aedeb4d59d84c29ff0238: what happens if two blocks are received on an alternate heavier chain, and it turns out they’re invalid (but with valid headers)? Do we disconnect in order to evaluate the alternative chain, and then reconnect the same tip once we discover the other chain is invalid? Or do we never disconnect in such a scenario?

    If we disconnect and then reconnect the same block, we lose the cache even though it was perfectly valid. I don’t think that’s a problem, but it’s worth a note if that can actually happen.

    ismaelsadeeq commented at 8:28 pm on January 26, 2026:
    Hmm, I think this is not affected by this scenario. IIUC, the block connected notification is only emitted after the block has been fully validated and connected. At that point, we just clear the cache and assume the tip has changed. When blocks are disconnected for whatever reason, we also clear the cache again because a reorg has happened.
  119. in src/node/miner.cpp:490 in 04d41ac3ce 
    490-                chainman.ActiveChainstate(),
491-                mempool,
492-                assemble_options}
493-                              .CreateNewBlock()};
494+            auto new_tmpl{block_template_cache->GetBlockTemplate(assemble_options)};
495+            ;
    Sjors commented at 5:05 pm on January 23, 2026:
    04d41ac3ced7d869b64210cf7862ac2e2e90d03e: spurious semicolon?
    ismaelsadeeq commented at 7:12 pm on January 26, 2026:
    Yep, deleted. I was curious to know why this compile. It turns out empty statements are valid statements and can have a potential use case. see https://stackoverflow.com/questions/36408249/why-putting-2-semicolon-is-not-an-error
  120. in src/node/miner.cpp:353 in 04d41ac3ce outdated 
    422@@ -423,13 +423,13 @@ void InterruptWait(KernelNotifications& kernel_notifications, bool& interrupt_wa
423     kernel_notifications.m_tip_block_cv.notify_all();
424 }
425 
426-std::unique_ptr<CBlockTemplate> WaitAndCreateNewBlock(ChainstateManager& chainman,
427-                                                      KernelNotifications& kernel_notifications,
428-                                                      CTxMemPool* mempool,
    Sjors commented at 5:07 pm on January 23, 2026:

    04d41ac3ced7d869b64210cf7862ac2e2e90d03e: nice side-effect that we longer need to pass the mempool in.

    Maybe we can avoid passing ChainstateManager and KernelNotifications as well? (although this function needs both of those for other reasons anyway)

    ismaelsadeeq commented at 7:14 pm on January 26, 2026:
    Yes, they are needed indeed. The idea is to get rid of them as planned in #33758.
  121. Sjors commented at 5:11 pm on January 23, 2026: member
    Concept ACK
  122. ismaelsadeeq force-pushed on Jan 26, 2026
  123. ismaelsadeeq force-pushed on Jan 26, 2026
  124. DrahtBot added the label CI failed on Jan 26, 2026
  125. DrahtBot commented at 8:19 pm on January 26, 2026: contributor

    🚧 At least one of the CI tasks failed. Task ASan + LSan + UBSan + integer: https://github.com/bitcoin/bitcoin/actions/runs/21370322533/job/61512880460 LLM reason (✨ experimental): miner_tests aborted due to a detected inconsistent lock order (potential deadlock) in miner.cpp.

    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.

  126. ismaelsadeeq commented at 8:39 pm on January 26, 2026: member

    Force-pushed after review by @Sjors and @ryanofsky.

    I made a couple of changes in this push. I got rid of ShareableTemplates completely and now only return from cache if there exists a template in the cache with identical block creation options to the request and the maximum age has not elapsed.

    Updated block creation time to use MockableSteadyClock and improve the test accuracy.

    8309c5de19..7e71277af

    I attempted to have BlockTemplateCache::GetBlockTemplate only lock m_mutex and then lock cs_main when no cache hit occurs, but this resulted in potential double insertion and a potential deadlock: https://github.com/bitcoin/bitcoin/actions/runs/21370322533/job/61512880491?pr=33421

    There seems to be an unrelated failure in the package_eval fuzz test?

    I took the easy approach and just lock cs_main and m_mutex at the same time, because if there is a cache hit, the time between acquiring the locks is negligible, whereas if there is no cache hit, you will need to lock cs_main anyway. However, it would still be nice to not have to lock cs_main in scenarios where there is a cache hit.

    070700ff78d..af743fe020

    The ephemeral_package_eval failure still occurs here—seems unrelated to me?

  127. miner: remove unused `node/context.h` import 
    - This prevents circular dependecy when we introduce
  BlockTemplateCache in miner and want to expose it
  via the node context.
    7d89c55820
  128. in src/node/miner.cpp:375 in b948e78d23 outdated 
    340+
341+BlockTemplateRef BlockTemplateCache::GetBlockTemplate(const BlockAssembler::Options& options, MillisecondsDouble max_template_age)
342+{
343+    LOCK2(cs_main, m_mutex);
344+    for (auto it = m_block_templates.rbegin(); it != m_block_templates.rend(); it++) {
345+        if (it->first == options && !TimeIntervalElapsed(it->second->m_creation_time, max_template_age)) {
    Sjors commented at 10:55 am on January 27, 2026:

    b948e78d2332d1cd03e8ccd76f93b0efc8233a45: the new approach of comparing all options seems fine, but let’s add a comment to point out that differences in test_block_validity, print_modified_fee and coinbase_output_script only impact test cases and benchmarks.

    In particular the latter might confuse readers at first glance, so maybe elaborate and say that the getblocktemplate RPC omits the coinbase transaction entirely and getCoinbaseTx() IPC omits the output script.

    ismaelsadeeq commented at 12:04 pm on January 27, 2026:
    Thanks fixed.
  129. ismaelsadeeq force-pushed on Jan 27, 2026
  130. ismaelsadeeq commented at 1:12 pm on January 27, 2026: member

    The C.I failure is related, since we now have an instance of MockableSteadyClock in miner.h, most test uses the miner, so the fuzz was complaining that I need to SetMockTime at the beginning of those fuzz targets. I fix it by generalising and adding SetMockTime to the BasicTestingSetup constructor, and in the destructor, we clear the mocktime.

    I also rebased added a clarifying comment requested by @Sjors here #33421 (review)

    16912d3676..a943e74

  131. in src/node/miner.cpp:339 in dfee492df4 outdated 
    332+void BlockTemplateCache::BlockConnected(
333+    const ChainstateRole& role,
334+    const std::shared_ptr<const CBlock>& /*unused*/,
335+    const CBlockIndex* /*unused*/)
336+{
337+    LOCK(m_mutex);
    ryanofsky commented at 1:34 pm on January 27, 2026:

    In commit “miner: clear block template after block connection/disconnection” (dfee492df46270784193b6cca9f50c5648d9acd9)

    No need to lock mutex when notification is being ignored. Would suggest simplifying to something like.

    0// Ignore block being connected to historical chain, not current chain.
1if (role.historical) return;
2
3LOCK(m_mutex);
4m_block_templates.clear();
  132. DrahtBot removed the label CI failed on Jan 27, 2026
  133. in src/test/miner_tests.cpp:904 in d4a9ad990a 
    899+        BOOST_CHECK(template_cache->GetBlockTemplate(base_options, max_template_age)->m_creation_time == prev_time);
900+    }
901+
902+    {
903+        // Return a new template when time interval has elapsed
904+        max_template_age = max_template_age - MillisecondsDouble{1}; // Subtract 1 millisecond
    ryanofsky commented at 1:44 pm on January 27, 2026:

    In commit “test: add unit test to BlockTemplateCache” (d4a9ad990ad8abc1350701230551c49d86a8a1d9)

    This seems like a complicated way of setting max age to 2ms instead of 1ms. Could you write max_template_age = 1ms; instead to be more explicit? It would also seem nice other places to avoid MillisecondsDouble and use literal time values.

  134. in src/node/miner.h:51 in 8888f13735 outdated 
    40@@ -40,6 +41,14 @@ class KernelNotifications;
41 
42 static const bool DEFAULT_PRINT_MODIFIED_FEE = false;
43 
44+// Return true if current time is greater or equal to `prev_time + time_interval`, or if
45+// `prev_time` is greater than the current time (indicating clock moved backward; only possible in test).
46+static inline bool TimeIntervalElapsed(const MockableSteadyClock::time_point& prev_time, MillisecondsDouble time_interval)
    ryanofsky commented at 2:21 pm on January 27, 2026:

    In commit “miner: add TimeIntervalElapsed function” (8888f1373559fdf9ee2cad4a3a3023261595ef62)

    Would suggest merging this commit into the next commit “miner: introduce BlockTemplateCache for reusing block templates” (1d14726469f021570bff2811570a5e54a08df72a) because this commit is very small and it’s not possible to understand the next commit without seeing this function. (For example the parameter name max_template_age doesn’t seem appropriate given currrent definition of this function, see my other review comment.)

    Also, about commit messages in this PR, I notice some of them are duplicating documentation that appears in the code, which I would suggest not doing. Using code comments in commit messages provides more detail than is helpful for understanding purpose of the changes, and creates an extra burden for reviewers to check correctness two places instead of one. Better IMO if commit messages provide a higher level summary of behavior than code comments, or focus on providing information not appropriate for code comments, like motivations behind the change or comparisons with previous code.

  135. in src/node/miner.h:61 in 2915bb21dd 
    55@@ -55,6 +56,11 @@ struct CBlockTemplate
56      * miner code.
57      */
58     CoinbaseTx m_coinbase_tx;
59+
60+    /* Uses steady clock because it is monotonic and unaffected by rare system clock changes.
61+     * Intended for checking template staleness with respect to fee increases.
    ryanofsky commented at 2:29 pm on January 27, 2026:

    In commit “miner: add block creation time point to CBlockTemplate” (2915bb21dd64ba98f5e1fb93c11f618030a0235f)

    Reference to fee increases is confusing because fees are only indirectly related to how this field is used. Would suggest something more direct like “Used to decide whether a cached template is eligible for reuse based on its age.”

  136. in src/node/miner.h:183 in 1d14726469 
    178+     *
179+     * @param options The block assembly options to use.
180+     * @param max_template_age This indicate the maximum age of the desired block template in milliseconds (Default 0).
181+     * @return a BlockTemplateRef.
182+     */
183+    BlockTemplateRef GetBlockTemplate(const BlockAssembler::Options& options, MillisecondsDouble max_template_age = MillisecondsDouble{0}) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
    ryanofsky commented at 2:53 pm on January 27, 2026:

    re: sjors #33421 (comment)

    I have light preference for making max_template_age an argument for GetBlockTemplate() (default 0) instead of adding it to BlockAssembler::Options. Especially because it’s ignored by ShareableOptions and then checked separately in GetBlockTemplate(). @Sjors I wonder if you could explain this preference a little more? It seems inconsistent to make this one option a separate parameter when there isn’t a clear difference between it and the other options semantically.

    If the description of upcoming changes in #33758 is accurate, it sounds we will want to add this field to BlockCreateOptions to provide caching functionality over IPC?

    I guess my question is whether you think template age should be specified as a separate parameter permanently and never added to BlockCreateOptions, or if you are just saying it should not be added to BlockCreateOptions now but should be added later, and then the separate parameter can go away?

    ryanofsky commented at 3:16 pm on January 27, 2026:

    In commit “miner: introduce BlockTemplateCache for reusing block templates” (1d14726469f021570bff2811570a5e54a08df72a)

    I think the option name max_template_age is misleading because of its boundary behavior. With the current logic, a cached template is only returned if its age is strictly less than this value. For example, if max_template_age = 30 seconds and a template was created exactly 30 seconds ago, it won’t be returned. That isn’t what the name implies because “maximum age” means “highest age that’s not too old”, not “lowest age that is too old”.

    The actual behavior seems ok (especially with 0 meaning “don’t use the cache”), just using the word “maximum” to describe it doesn’t seem right. A name like template_age_limit, cache_age_limit, or reuse_age_limit would seem better.

    Alternately the name max_template_age could be kept and TimeIntervalElapsed could be changed to return true if current time is > instead of >= the max time. But in that case we would need a different value like -1 or nullopt instead of 0 to mean don’t use the cache. So it would seem simplest just to change the name of the option.

    EDIT: On the other hand, although the idea of using 0 to generate a new template instead of using the cache seems appealing, there are problems if you think about it. The 0 behavior does not seem like a good default because if used it means callers will add new templates to the cache and evict other templates, but probably never use the cached templates they are adding.

    For example if caller A sets a nonzero age limit, and caller B uses a the default 0 value, caller B could fill up the cache with its own templates even though they are never used, in the meantime evicting templates from caller A.

    So I think we will want to have a separate value for calls which shouldn’t use the read from the cache or add to it. Using std::optional<MillisecondsDouble> max_template_age and nullopt would seem to make sense for that, and at that point we could normalize the max boundary behavior and not need to treat 0 specially.

  137. in src/node/miner.h:176 in 1d14726469 
    171+    BlockTemplateCache(CTxMemPool& mempool, ChainstateManager& chainman, size_t block_template_cache_size = DEFAULT_BLOCK_TEMPLATE_CACHE_SIZE);
172+    ~BlockTemplateCache() = default;
173+
174+    /**
175+     * If a cached template exists with identical options and its age is less than
176+     * the specified interval, the cached template is returned.
    ryanofsky commented at 3:26 pm on January 27, 2026:

    In commit “miner: introduce BlockTemplateCache for reusing block templates” (1d14726469f021570bff2811570a5e54a08df72a)

    Minor: maybe replace “interval” with “limit” or “amount”. Right now this little off because age is more of a duration than an interval

  138. in src/node/miner.cpp:349 in 1d14726469 
    344+    // Cache lookup compares all block creation option fields for an exact match. Note that differences
345+    // in test_block_validity, print_modified_fee, and coinbase_output_script only affect
346+    // tests and benchmarks, not production usage.
347+    // This implies that these fields can differ in test scenarios without impacting real-world block template
348+    // functionality. Discrepancies in the above fields will still cause cache misses, but since they are fixed
349+    // in non-test and benchmark environments, it is okay to compare for an exact match.
    ryanofsky commented at 3:53 pm on January 27, 2026:

    In commit “miner: introduce BlockTemplateCache for reusing block templates” (1d14726469f021570bff2811570a5e54a08df72a)

    Saying that certain fields “only affect tests and benchmarks” is confusing because it sounds like it is saying these fields don’t affect caching behavior even though they do. Something like the following would seem clearer:

    0// Cache lookup requires an exact match on all block creation options.
1// While some fields (e.g. test_block_validity, print_modified_fee, coinbase_output_script)
2// could be ignored for cache reuse, differences in these fields mainly occur in
3// tests and benchmarks. To keep the logic simple, we treat all fields as part of
4// the cache key and accept cache misses in those cases.
  139. in src/node/miner.cpp:343 in 1d14726469 
    338+    return block_template;
339+}
340+
341+BlockTemplateRef BlockTemplateCache::GetBlockTemplate(const BlockAssembler::Options& options, MillisecondsDouble max_template_age)
342+{
343+    LOCK2(cs_main, m_mutex);
    ryanofsky commented at 4:01 pm on January 27, 2026:

    In commit “miner: introduce BlockTemplateCache for reusing block templates” (1d14726469f021570bff2811570a5e54a08df72a)

    It seems wasteful to lock cs_main here when iterating over m_block_templates, since if there is a matching template, cs_main shouldn’t be required. Code could just lock m_mutex when searching the cache. Then if nothing is found it could release m_mutex and lock cs_main and m_mutex to add to the cache.

    I’m also not sure about purpose of having a separate CreateBlockTemplateInternal method. It seems like it is only called one place and would be simpler to inline.

  140. in src/node/miner.cpp:334 in 1d14726469 
    329+}
330+BlockTemplateRef BlockTemplateCache::CreateBlockTemplateInternal(const BlockAssembler::Options& options)
331+{
332+    Chainstate& current_chainstate = m_chainman.CurrentChainstate();
333+    BlockAssembler assembler{current_chainstate, &m_mempool, options};
334+    auto block_template = std::make_shared<const CBlockTemplate>(*assembler.CreateNewBlock());
    ryanofsky commented at 4:11 pm on January 27, 2026:

    In commit “miner: introduce BlockTemplateCache for reusing block templates” (1d14726469f021570bff2811570a5e54a08df72a)

    Should be able to avoid copying here with:

    0std::shared_ptr<const CBlockTemplate> block_template{assembler.CreateNewBlock()};
  141. in src/node/miner.h:227 in e3b559c098 
    229-                                                      const BlockAssembler::Options& assemble_options,
230-                                                      bool& interrupt_wait);
231+BlockTemplateRef WaitAndCreateNewBlock(BlockTemplateCache* block_template_cache,
232+                                       ChainstateManager& chainman,
233+                                       KernelNotifications& kernel_notifications,
234+                                       std::unique_ptr<CBlockTemplate>& block_template,
    ryanofsky commented at 4:30 pm on January 27, 2026:

    In commit “interfaces: create block template via block template cache” (e3b559c098f84d0b291889342a1bf0c30863fa84)

    This commit seems to have dropped const from this parameter by accident, so const could be added back here.

    However, the previous type const std::unique_ptr<CBlockTemplate>& block_template also did not make much sense because:

    • The function doesn’t change the template so a mutable template shouldn’t be passed.
    • The function doesn’t check for null so a nullable template shouldn’t be passed.
    • The function doesn’t use any unique_ptr functionality so unique_ptr shouldn’t be used.

    Would make more sense to change parameter type const BlockTemplate& block_template

  142. in src/test/miner_tests.cpp:873 in d4a9ad990a outdated 
    867@@ -867,4 +868,133 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
868     TestPrioritisedMining(scriptPubKey, txFirst);
869 }
870 
871+BOOST_AUTO_TEST_CASE(blocktemplate_cache)
    ryanofsky commented at 4:57 pm on January 27, 2026:

    In commit “test: add unit test to BlockTemplateCache” (d4a9ad990ad8abc1350701230551c49d86a8a1d9)

    Might be nice if this test had a helper to reduce boilerplate when checking for cache hits/misses. Chatgpt suggested:

     0auto tmpl = template_cache->GetBlockTemplate({}, MillisecondsDouble{0ms});
 1auto prev_time = tmpl->m_creation_time;
 2auto now = MockableSteadyClock::INITIAL_MOCK_TIME;
 3auto check_cache = [&](bool expect_hit,
 4                       const BlockAssembler::Options& opts,
 5                       std::chrono::milliseconds max_age,
 6                       std::chrono::milliseconds advance = 1ms) {
 7    MockableSteadyClock::SetMockTime(now += advance);
 8    const auto tmpl = template_cache->GetBlockTemplate(opts, MillisecondsDouble{max_age});
 9    if (expect_hit) {
10        BOOST_CHECK_EQUAL(tmpl->m_creation_time, prev_time);
11    } else {
12        BOOST_CHECK(tmpl->m_creation_time > prev_time);
13        prev_time = tmpl->m_creation_time;
14    }
15};

    Which could be used like:

    0check_cache(/*expect_hit=*/false, {}, 0ms); // max_age=0 disables caching => always miss
1check_cache(/*expect_hit=*/true,  {}, 2ms); // Hit if age < limit
2check_cache(/*expect_hit=*/false, {}, 1ms); // Boundary: age == limit => miss (strict)

    This seems like it would be more readable and a similar pattern could be used in the fuzz test.

    Not too important, but wanted to raise the idea.

  143. ryanofsky approved
  144. ryanofsky commented at 5:50 pm on January 27, 2026: contributor

    Code review ACK a943e749c65f2279363e18dc5fec820f7ed43f01. That code looks good and seems like it should be useful to implement the followups in #33758 like exposing the caching feature externally and eliminating global variables in the RPC implementation.

    I left a number of suggestions below but none are critical. I think maybe biggest concern here would be that by default this seems to add templates to the cache but never use them. Also going forward it doesn’t provide IPC callers an escape hatch to let them them request a new template without adding it to the cache. So I think making max template age std::optional and using nullopt to fully bypass the cache could be a good improvement here.

  145. DrahtBot requested review from Sjors on Jan 27, 2026
  146. ismaelsadeeq commented at 2:05 pm on January 28, 2026: member
    All these suggestions and comments are great. Thanks @ryanofsky, I am planning a force push with the comments addressed.
  147. in src/test/util/setup_common.cpp:123 in a943e749c6 outdated 
    115@@ -116,6 +116,7 @@ BasicTestingSetup::BasicTestingSetup(const ChainType chainType, TestOpts opts)
116     if (!EnableFuzzDeterminism()) {
117         SeedRandomForTest(SeedRand::FIXED_SEED);
118     }
119+    MockableSteadyClock::SetMockTime(MockableSteadyClock::INITIAL_MOCK_TIME);
120
    maflcko commented at 3:57 pm on January 28, 2026:

    not sure about this. Yeah, sure, it is going to silence the fuzz error. However, this will globally set the steady clock to a hard-coded constant for all tests.

    Maybe this is fine for the steady clock and all tests. However, if a test is using the clock, it seems better to locally document that the clock is used. Otherwise, code paths that rely on the clock advancing will silently never be covered.

    So I guess i am -0 on this. It would be better to set to a constant in each test. Ideal would be to actually advance the clock in each test, so that all code paths are reachable and testable.

    maflcko commented at 6:11 pm on January 28, 2026:
    See #34432 for a helper context that could help make this easier to implement.
  148. miner: add block creation time point to `CBlockTemplate` ff20b90b72
  149. miner: introduce `BlockTemplateCache` for reusing block templates 869da6ed94
  150. miner: clear block template after block connection/disconnection 53ddc2e6bb
  151. test: add unit test to `BlockTemplateCache` 
    Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
    12f59b84e2
  152. interfaces: create block template via block template cache 5587724555
  153. test: add blocktemplatecache fuzz test e876ca1118
  154. ismaelsadeeq force-pushed on Jan 29, 2026
  155. ismaelsadeeq commented at 9:22 pm on January 29, 2026: member

    Thanks for your review @ryanofsky I forced pushed from a943e749c65f2279363e18dc5fec820f7ed43f01 to e876ca111856a5afcccaba93c3f79f79e24fa490 compare diff

    • #33421#pullrequestreview-3711244801 Yes, since there is no cache hit, I agree with your suggestion and added an option to make the template age limit optional. When no value is provided, we simply return a new block template without storing it in the cache.

    • #33421 (review) Indeed, there is no need to lock cs_main here. I’ve taken the suggestion.

    • #33421 (review) This is now gone. I’ve taken your suggestion, with some modifications.

    • #33421 (review) I took your suggestion to merge the two commits. I’ve also removed the verbose commit messages since the commits already have descriptions.

    • #33421 (review) I removed the reference to the fee increase in the comment.

    • #33421 (review) I updated the parameter name to template_age_limit.

    • #33421 (review) I took the comment suggestion.

    • #33421 (review) For the optimistic case, there is no need to lock cs_main, but there is a scenario where, after searching and finding no cache hit, the mutex is released and another thread acquires the lock and inserts a new template, possibly with matching options. In that case, we need to check again to avoid double insertion. I added this logic, although I think it is negligible given the cache size. createBlockTemplateInternal is now used in multiple places, so I kept it. I also made the cache store only one instance per template creation option.

    • #33421 (review) That version of the code is now gone.

    • #33421 (review) I took your suggestion and changed the parameter to const CBlockTemplate&.

    • #33421 (review) I took your suggestion—thanks. It reduces the diff quite a bit. I also applied the same idea in the fuzz test.

    • #33421 (review) Good point. I updated the test setup to avoid globally setting mocktime for all tests and instead set it only for the required tests.

  156. DrahtBot added the label CI failed on Jan 29, 2026
  157. DrahtBot commented at 10:38 pm on January 29, 2026: contributor

    🚧 At least one of the CI tasks failed. Task macOS native, fuzz: https://github.com/bitcoin/bitcoin/actions/runs/21494475650/job/61925506730 LLM reason (✨ experimental): Fuzz test crash: TestBlockValidity failed due to unexpected witness data (CheckWitnessMalleation) in tx_pool.

    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 DrahtBot glozow ryanofsky Sjors brunoerg maflcko


Sjors

Labels
CI failed

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-02-01 18:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me