node: add BlockTemplateCache #33421

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.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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.

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         }

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.

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

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

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

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

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

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

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

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

Maybe s/maximum_/max_/

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

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

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.

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.

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.

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.

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.

You are right, what does solution mean here?

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

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)

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.

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)

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

0// The dummy coinbase output script is not compared.

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

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.

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

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.

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.

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}

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.

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.

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)

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

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.

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)

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.

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();

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.

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.

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

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?

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.

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

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.

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.

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()};

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

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.

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.

this looks a bit wrong, because INITIAL_MOCK_TIME represents a time point, but the argument to operator+= is a duration. Also, the += here doesn’t seem to be doing anything and can be removed? If not, it could make sense to add a short comment?

Yeah, there is no need for the bump after constructing the SteadyClockContext. I attempted to fix it in https://github.com/bitcoin/bitcoin/pull/33421/changes/394796f35cfc653d111090605f68a24a59554460

I guess it would be good if someone changed INITIAL_MOCK_TIME to be a time point type.

Good idea, but that is beyond the scope of this PR.

llm nit: Those are hard to read and difficult to maintain. Ref the LLM:

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

BlockConnected({true, false}, nullptr, nullptr) in src/test/fuzz/blocktemplatecache.cpp
BlockConnected(background_role, nullptr, nullptr) in src/test/fuzz/blocktemplatecache.cpp

2026-02-05 16:25:11

The operation with time_2 is a steady clock whilem_creation_time is a mockable steady clock. not worth the conversion imo so not taken.

Thanks for the suggestion

In 4619759de5111223a87bddb2d39ff020a848c547 miner: introduce BlockTemplateCache for reusing block templates: let’s call this m_block_template_cache_limit, so it’s not confused with m_block_templates.size().

(the global DEFAULT_BLOCK_TEMPLATE_CACHE_SIZE is fine though, it’s clear that that refers to a size limit)

In 4619759de5111223a87bddb2d39ff020a848c547 miner: introduce BlockTemplateCache for reusing block templates: this is somewhat surprising behavior in isolation. When you have two clients, one client might want a recent template and the second client allows an older one.

But it makes sense because an evicted template is guaranteed to be replaced.

I tried some alternative approaches, but it was ugly.

Maybe rename to getOrDeleteCachedTemplate?

In 4619759de5111223a87bddb2d39ff020a848c547 miner: introduce BlockTemplateCache for reusing block templates: let’s add a comment here:

0// cs_main must be locked before m_mutex, so we released it above and
1// take it again here. 

Here’s a frivolous optimization for the second lookup, for the common case that nothing changed. Feel free to not use:

 0diff --git a/src/node/miner.cpp b/src/node/miner.cpp
 1index 7620ad80a7..de39b3dac3 100644
 2--- a/src/node/miner.cpp
 3+++ b/src/node/miner.cpp
 4@@ -346,7 +346,9 @@ BlockTemplateRef BlockTemplateCache::createBlockTemplateInternal(const BlockAsse
 5 BlockTemplateRef BlockTemplateCache::getCachedTemplate(
 6     const BlockAssembler::Options& options,
 7-    MillisecondsDouble template_age_limit)
 8+    MillisecondsDouble template_age_limit,
 9+    uint64_t last_nonce)
10 {
11     AssertLockHeld(m_mutex);
12+    if (m_nonce == last_nonce) return nullptr;
13     auto it = std::find_if(m_block_templates.begin(), m_block_templates.end(),
14                            [&](const TemplateEntry& entry) { return entry.first == options; });
15@@ -355,4 +357,5 @@ BlockTemplateRef BlockTemplateCache::getCachedTemplate(
16     }
17     if (TimeIntervalElapsed(it->second->m_creation_time, template_age_limit)) {
18+        ++m_nonce;
19         m_block_templates.erase(it);
20         return nullptr;
21@@ -374,16 +377,19 @@ BlockTemplateRef BlockTemplateCache::GetBlockTemplate(
22     // tests and benchmarks. To keep the logic simple, we treat all fields as part of
23     // the cache key and accept cache misses in those cases.
24+    uint64_t nonce{0};
25     {
26         LOCK(m_mutex);
27         auto cached = getCachedTemplate(options, *template_age_limit);
28         if (cached) return cached;
29+        nonce = m_nonce;
30     }
31
32     LOCK2(::cs_main, m_mutex);
33     // Search again - another thread might have added a template while we released m_mutex
34-    auto cached = getCachedTemplate(options, *template_age_limit);
35+    auto cached = getCachedTemplate(options, *template_age_limit, nonce);
36     if (cached) return cached;
37     auto block_template = createBlockTemplateInternal(options);
38     m_block_templates.emplace_back(options, block_template);
39+    ++m_nonce;
40     if (m_block_templates.size() > m_block_template_cache_size) {
41         m_block_templates.pop_front();
42diff --git a/src/node/miner.h b/src/node/miner.h
43index 57bbe2dcc0..abb6927b2b 100644
44--- a/src/node/miner.h
45+++ b/src/node/miner.h
46@@ -176,4 +176,6 @@ class BlockTemplateCache
47     // FIFO cache; at most one template per BlockAssembler::Options.
48     std::deque<TemplateEntry> m_block_templates GUARDED_BY(m_mutex);
49+    // Incremented on every cache mutation to detect unchanged state between lock phases.
50+    uint64_t m_nonce GUARDED_BY(m_mutex){0};
51
52     /**
53@@ -185,5 +187,6 @@ class BlockTemplateCache
54      */
55     BlockTemplateRef getCachedTemplate(const BlockAssembler::Options& options,
56-                                       MillisecondsDouble template_age_limit)
57+                                       MillisecondsDouble template_age_limit,
58+                                       uint64_t last_nonce = 0)
59         EXCLUSIVE_LOCKS_REQUIRED(m_mutex);

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

Can commit message be updated to say why several fuzz tests need this definition now and didn’t need it previously?

I’m guessing these lines are being added to avoid the fuzz test framework detecting these tests as being nondeterministic now that the m_creation_time variable is being added, but it would be good to mention whatever the reason is in the commit.

Also I guess I don’t understand why fuzzing framework wouldn’t just declare it’s own SteadyClockContext variable instead of requiring tests to do it individually, so maybe a comment clarify that as well.

Also I guess I don’t understand why fuzzing framework wouldn’t just declare it’s own SteadyClockContext variable instead of requiring tests to do it individually, so maybe a comment clarify that as well.

That should be the best way to do it, but I consider it out of scope of this PR for now. Hence, I just improved the commit message.

In commit “miner: introduce BlockTemplateCache for reusing block templates” (240f3c591cecd9550a102171ef0fc9a5d94bf3cc)

This could be simplified to return assembler.CreateNewBlock() (unique_ptr is convertible to shared_ptr).

Also, it seems like dropping this method would simplify the PR:

 0--- a/src/node/miner.cpp
 1+++ b/src/node/miner.cpp
 2@@ -358,14 +358,6 @@ void BlockTemplateCache::BlockDisconnected(
 3     m_block_templates.clear();
 4 }
 5 
 6-BlockTemplateRef BlockTemplateCache::createBlockTemplateInternal(const BlockAssembler::Options& options)
 7-{
 8-    AssertLockHeld(::cs_main);
 9-    Chainstate& current_chainstate = m_chainman.CurrentChainstate();
10-    BlockAssembler assembler{current_chainstate, &m_mempool, options};
11-    return std::make_shared<const CBlockTemplate>(std::move(*assembler.CreateNewBlock()));
12-}
13-
14 BlockTemplateRef BlockTemplateCache::getCachedTemplate(
15     const BlockAssembler::Options& options,
16     MillisecondsDouble template_age_limit)
17@@ -388,7 +380,7 @@ BlockTemplateRef BlockTemplateCache::GetBlockTemplate(
18     std::optional<MillisecondsDouble> template_age_limit)
19 {
20     if (!template_age_limit.has_value()) {
21-        return WITH_LOCK(::cs_main, return createBlockTemplateInternal(options));
22+        return BlockAssembler{m_chainman.ActiveChainstate(), &m_mempool, options}.CreateNewBlock();
23     }
24 
25     // Cache lookup requires an exact match on all block creation options.
26@@ -406,7 +398,7 @@ BlockTemplateRef BlockTemplateCache::GetBlockTemplate(
27     // Search again - another thread might have added a template while we released m_mutex
28     auto cached = getCachedTemplate(options, *template_age_limit);
29     if (cached) return cached;
30-    auto block_template = createBlockTemplateInternal(options);
31+    BlockTemplateRef block_template = BlockAssembler{m_chainman.ActiveChainstate(), &m_mempool, options}.CreateNewBlock();
32     m_block_templates.emplace_back(options, block_template);
33     if (m_block_templates.size() > m_block_template_cache_limit) {
34         m_block_templates.pop_front();
35--- a/src/node/miner.h
36+++ b/src/node/miner.h
37@@ -191,14 +191,6 @@ class BlockTemplateCache : public CValidationInterface
38     BlockTemplateRef getCachedTemplate(const BlockAssembler::Options& options,
39                                        MillisecondsDouble template_age_limit)
40         EXCLUSIVE_LOCKS_REQUIRED(m_mutex);
41-    /**
42-     * Create a new block template.
43-     *
44-     * Does not access or modify the cache.
45-     * Requires cs_main to protect chainstate access.
46-     */
47-    BlockTemplateRef createBlockTemplateInternal(const BlockAssembler::Options& options)
48-        EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
49 
50 public:
51     explicit BlockTemplateCache(CTxMemPool& mempool,