In commit "miner: introduce BlockTemplateCache for reusing block templates" (240f3c591cecd9550a102171ef0fc9a5d94bf3cc)
Now that this accepts nullopt, I don't think there is a reason to have a vague "age limit" option instead of clearer "max age" option. Would suggest dropping TimeIntervalElapsed function and age_limit variable everywhere replacing them with max_age returning templates <= than that age, so the API is easier to understand and code is simpler to reason about:
<details><summary>diff</summary>
<p>
--- a/src/node/miner.cpp
+++ b/src/node/miner.cpp
@@ -368,7 +368,7 @@ BlockTemplateRef BlockTemplateCache::createBlockTemplateInternal(const BlockAsse
BlockTemplateRef BlockTemplateCache::getCachedTemplate(
const BlockAssembler::Options& options,
- MillisecondsDouble template_age_limit)
+ MillisecondsDouble template_max_age)
{
AssertLockHeld(m_mutex);
auto it = std::find_if(m_block_templates.begin(), m_block_templates.end(),
@@ -376,7 +376,9 @@ BlockTemplateRef BlockTemplateCache::getCachedTemplate(
if (it == m_block_templates.end()) {
return nullptr;
}
- if (TimeIntervalElapsed(it->second->m_creation_time, template_age_limit)) {
+ const auto now = MockableSteadyClock::now();
+ // Erase template if it is too old, or if the clock moved backwards (should never happen except in tests).
+ if (now - it->second->m_creation_time > template_max_age || now < it->second->m_creation_time) {
m_block_templates.erase(it);
return nullptr;
}
@@ -385,9 +387,9 @@ BlockTemplateRef BlockTemplateCache::getCachedTemplate(
BlockTemplateRef BlockTemplateCache::GetBlockTemplate(
const BlockAssembler::Options& options,
- std::optional<MillisecondsDouble> template_age_limit)
+ std::optional<MillisecondsDouble> template_max_age)
{
- if (!template_age_limit.has_value()) {
+ if (!template_max_age.has_value()) {
return WITH_LOCK(::cs_main, return createBlockTemplateInternal(options));
}
@@ -398,13 +400,13 @@ BlockTemplateRef BlockTemplateCache::GetBlockTemplate(
// the cache key and accept cache misses in those cases.
{
LOCK(m_mutex);
- auto cached = getCachedTemplate(options, *template_age_limit);
+ auto cached = getCachedTemplate(options, *template_max_age);
if (cached) return cached;
}
LOCK2(::cs_main, m_mutex);
// Search again - another thread might have added a template while we released m_mutex
- auto cached = getCachedTemplate(options, *template_age_limit);
+ auto cached = getCachedTemplate(options, *template_max_age);
if (cached) return cached;
auto block_template = createBlockTemplateInternal(options);
m_block_templates.emplace_back(options, block_template);
--- a/src/node/miner.h
+++ b/src/node/miner.h
@@ -46,14 +46,6 @@ class KernelNotifications;
static const bool DEFAULT_PRINT_MODIFIED_FEE = false;
static constexpr size_t DEFAULT_BLOCK_TEMPLATE_CACHE_LIMIT{10};
-// Return true if current time is greater or equal to `prev_time + time_interval`, or if
-// `prev_time` is greater than the current time (indicating clock moved backward; only possible in test).
-static inline bool TimeIntervalElapsed(const MockableSteadyClock::time_point& prev_time, MillisecondsDouble time_interval)
-{
- const auto now = MockableSteadyClock::now();
- return now < prev_time || MillisecondsDouble{now - prev_time} >= time_interval;
-}
-
struct CBlockTemplate
{
CBlock block;
@@ -152,15 +144,15 @@ private:
* template is stored per set of options, and the cache has a fixed maximum size.
*
* When requesting a block template:
- * - If template_age_limit is std::nullopt, a new block template is always
+ * - If template_max_age is std::nullopt, a new block template is always
* created and returned, and nothing is cached.
*
* - Otherwise, the cache is checked for a template with matching options.
- * If a cached template exists and its age (elapsed time since creation) is strictly
- * less than template_age_limit, it is reused.
+ * If a cached template exists and its age (elapsed time since creation)
+ * doesn't exceed template_max_age, it is reused.
*
- * - If a matching template exists but its age has reached or exceeded the
- * limit, it is removed and replaced with a newly created template.
+ * - If a cached template exists but is too old, it is removed and replaced
+ with a newly created template.
*
* - If no matching template exists, a new template is created and cached.
*
@@ -184,12 +176,12 @@ class BlockTemplateCache : public CValidationInterface
/**
* Search for a cached template matching the provided options.
*
- * - If found and within the age limit, return it.
+ * - If found and not too old, return it.
* - If found but too old, evict it and return nullptr.
* - If not found, return nullptr.
*/
BlockTemplateRef getCachedTemplate(const BlockAssembler::Options& options,
- MillisecondsDouble template_age_limit)
+ MillisecondsDouble template_max_age)
EXCLUSIVE_LOCKS_REQUIRED(m_mutex);
/**
* Create a new block template.
@@ -211,12 +203,12 @@ public:
* Cache lookup requires an exact match on all block creation options.
*
* [@param](/bitcoin-bitcoin/contributor/param/) options Block assembly options to match
- * [@param](/bitcoin-bitcoin/contributor/param/) template_age_limit Maximum age for cached templates in milliseconds.
+ * [@param](/bitcoin-bitcoin/contributor/param/) template_max_age Maximum age for cached templates in milliseconds.
* If nullopt, bypasses cache entirely (doesn't insert into, read or delete from cache).
* [@return](/bitcoin-bitcoin/contributor/return/) A shared pointer to the block template
*/
BlockTemplateRef GetBlockTemplate(const BlockAssembler::Options& options,
- std::optional<MillisecondsDouble> template_age_limit = std::nullopt)
+ std::optional<MillisecondsDouble> template_max_age = std::nullopt)
EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
/** Test-only: verify cache invariants */
--- a/src/test/fuzz/blocktemplatecache.cpp
+++ b/src/test/fuzz/blocktemplatecache.cpp
@@ -73,17 +73,17 @@ FUZZ_TARGET(block_template_cache, .init = initialize_block_template_cache)
auto prev_time = tmpl->m_creation_time;
auto CheckCache = [&](bool expect_hit,
const node::BlockAssembler::Options& opts,
- std::optional<MillisecondsDouble> age_limit = std::nullopt,
+ std::optional<MillisecondsDouble> max_age = std::nullopt,
std::chrono::milliseconds advance = 1ms) {
steady_clock += advance;
- auto t = template_cache->GetBlockTemplate(opts, age_limit);
+ auto t = template_cache->GetBlockTemplate(opts, max_age);
assert(t);
if (expect_hit) {
assert(t->m_creation_time == prev_time);
} else {
assert(t->m_creation_time > prev_time);
- if (age_limit) prev_time = t->m_creation_time;
+ if (max_age) prev_time = t->m_creation_time;
}
};
LIMITED_WHILE(fuzzed_data_provider.remaining_bytes(), 10)
@@ -91,13 +91,13 @@ FUZZ_TARGET(block_template_cache, .init = initialize_block_template_cache)
// template age-based behavior
const auto advance_ms =
std::chrono::milliseconds{fuzzed_data_provider.ConsumeIntegralInRange<int>(1, 100000)};
- const auto template_age_limit_ms =
+ const auto template_max_age_ms =
std::chrono::milliseconds{fuzzed_data_provider.ConsumeIntegralInRange<int>(0, 100000)};
- const auto age_limit = MillisecondsDouble{template_age_limit_ms};
+ const auto max_age = MillisecondsDouble{template_max_age_ms};
- // Expected hit iff block template age limit exceeds the elapsed time.
- const bool expect_hit = advance_ms < template_age_limit_ms;
- CheckCache(expect_hit, base_options, age_limit, advance_ms);
+ // Expected hit iff block template isn't older than max_age.
+ const bool expect_hit = advance_ms <= template_max_age_ms;
+ CheckCache(expect_hit, base_options, max_age, advance_ms);
// historical chainstate role should not clear cache
kernel::ChainstateRole background_role{true, true};
--- a/src/test/miner_tests.cpp
+++ b/src/test/miner_tests.cpp
@@ -893,10 +893,10 @@ BOOST_AUTO_TEST_CASE(blocktemplate_cache)
auto prev_time = block_template->m_creation_time;
auto check_cache = [&](bool expect_hit,
const BlockAssembler::Options& opts,
- std::optional<MillisecondsDouble> template_age_limit = std::nullopt,
+ std::optional<MillisecondsDouble> template_max_age = std::nullopt,
std::chrono::milliseconds advance = 1ms) {
steady_clock += advance;
- const auto tmpl = template_cache->GetBlockTemplate(opts, template_age_limit);
+ const auto tmpl = template_cache->GetBlockTemplate(opts, template_max_age);
BOOST_CHECK(tmpl != nullptr);
if (expect_hit) {
BOOST_CHECK(tmpl->m_creation_time == prev_time);
@@ -912,12 +912,15 @@ BOOST_AUTO_TEST_CASE(blocktemplate_cache)
// 0ms limit creates new template (nothing can be <= 0ms old iff time advances)
check_cache(/*expect_hit=*/false, default_options, MillisecondsDouble{0});
- // Hit if age < limit
+ // Hit if age < max_age
check_cache(/*expect_hit=*/true, default_options, 2ms);
- // Boundary: age == limit => miss (TimeIntervalElapsed uses >=)
+ // Miss if age > max_age
check_cache(/*expect_hit=*/false, default_options, MillisecondsDouble{1});
+ // Hit if age == max_age
+ check_cache(/*expect_hit=*/true, default_options, 1ms);
+
// BlockConnected signal with background chainstate role should not invalidate cache
kernel::ChainstateRole bg_role{true, true};
auto chain_tip = WITH_LOCK(cs_main, return m_node.chainman->ActiveChain().Tip());
</p>
</details>
