#36356 mining: move getblocktemplate cache into BlockTemplateManager

ReviewabilityReviewsAgreementSize
#36356 w0xlt mining: move getblocktemplate cache into BlockTemplateManagerReady0Strong71 + 66 tests
  • Tie mining template cache lifetimes to the node context rather than process-scoped static state
  • Prevent cached block pointers from outliving chainstate managers and enable template invalidation unit tests
  • Ready for review
  • Substantive support with no open objections (ismaelsadeeq, Sjors)
  • Concept approval for facilitating future shared caching across RPC and IPC mining (ismaelsadeeq)
  • Support for keeping the scope simple and focused via a three-commit split (Sjors)
  • Author addressed questions on cache lifetime and parameterization (ismaelsadeeq)
  • 71 lines added or modified outside tests
  • 66 lines added or modified in tests
  • 32 lines removed in total
  • 4 files, 3 commits
Mining#8 of 17
P3 · cleanup
  • P3 because it is an internal cleanup removing static global state from mining RPCs
  • Prevents lifetime issues in tests and enforces compile-time thread-safety annotations
  • Adds direct unit testing for template invalidation without altering mining behavior
  • Ranking pass: Moved up: it is the declared base of #36357, so reviewing it first is what makes the user-visible getmininginfo fix mergeable.
Ranking notes
Reviewability: ReadyStalePaused
Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets
Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs.
Size: lines added or modified outside tests, then in tests; greener = smaller.

Goal

Moves the `getblocktemplate` block template cache from function-local static variables in the RPC handler into `BlockTemplateManager`. The cache's lifetime is bound to the node context rather than the process, raw `CBlockIndex*` pointers are replaced with hash lookups, synchronization under `cs_main` is formally annotated, and unit tests are added for template invalidation.

Problem: Template caching in `getblocktemplate` previously used process-scoped static variables. This allowed cached `CBlockIndex*` pointers to outlive their owning `ChainstateManager` during unit tests, bypassed compile-time thread-safety checking for `cs_main`, and prevented unit testing of the caching and refresh logic.

Evidence of importance:

  • Addresses the 'Remove remaining global states' TODO in tracking issue #33758.
  • PR description points out that 'cached CBlockIndex* can outlive its ChainstateManager (as in unit tests)' and 'can't be unit tested.'

Scope: The change is primarily a code movement from `rpc/mining.cpp` to `node/block_template_manager.cpp`, accompanied by unit tests in `test/miner_tests.cpp`.

Risk of a mistake: medium (rpc_api, other). A flaw in the template cache invalidation logic could serve stale templates after a tip change, leading miners to mine on an outdated tip.

Changed since the previous assessment: No structural changes to the commits or implementation; references to follow-up PR #36357 were removed.

Reviewability

Ready

Nothing known that would invalidate a review now.

Waiting on (from the thread read):

  • reviewer: ismaelsadeeq and Sjors to review the author's replies and 3-commit split 2026-10-28

Agreement

Strong

substantive support, no open objection (ismaelsadeeq, Sjors)

Thread notes: Author replied to both of ismaelsadeeq's inline questions/suggestions and restructured the commits into a 3-commit split in response to Sjors's feedback.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
ismaelsadeeq (member)suggestion, scopeImplement a broader cache shared with IPC createnewblock, waitnext, and the mempool fee estimator as attempted in #33421accepted · accepted deferring to follow-upsansweredyes2026-10-08: 'I think we can implement the broader cache as attempted here #33421, mempool estimator dependency on `ChainStateManager` will be gone, and the ipc `createnewblock`, `waitnext` and `getblocktemplate` R'
Status from: 2026-10-09: ismaelsadeeq: 'However, I agree this is a start; we can extend it later.'
ismaelsadeeq (member)question, approachShould the cache be cleared on a block connection instead of keeping it until shutdown?answered · author explained master already keeps it until shutdown and clearing on connect requires a notification hook better left for a follow-upansweredyes2026-10-09: 'The cache is kept forever till shutdown. Should we clear it on a block connection?'
Status from: 2026-10-28: w0xlt: 'Clearing it on block connection would need a new notification hook in the manager, so I'd leave that for a follow-up.'
ismaelsadeeq (member)suggestion, interfacePass max_age as a parameter so that the caching logic is not tied to getblocktemplateanswered · author explained max_age fits better in a follow-up that makes the cache generic, and named the constant GBT_TEMPLATE_REBUILD_INTERVALansweredyes2026-10-09: 'Should we pass the `max_age` so that it is not tied to GBT?'
Status from: 2026-10-28: w0xlt: 'I'd keep `max_age` out of this PR. The cache is still a single entry built with the default options, so another caller with a different `max_age` would just keep rebuilding the GBT template.'

Support:

  • ismaelsadeeq (member) (Concept ACK, 2026-10-08) [read]: moving template caching into BlockTemplateManager creates an opportunity to share template caching across RPC, IPC mining, and fee estimation
  • Sjors (member) (2026-10-28) [read]: prefers to keep this simple and focussed

Participants:

  • Sjors (member, support, 2 statements 2026-10-07 to 2026-10-28): supports keeping the scope simple and focused; suggested a 3-commit split
  • ismaelsadeeq (member, support, 4 statements 2026-10-08 to 2026-10-09): Concept ACK; accepted deferring broader caching, asked about clearing cache on block connection and passing max_age

Files

66 lines under test/bench/ci.

  • src/test/miner_tests.cpp +66/-0
  • src/rpc/mining.cpp +6/-31
  • src/node/block_template_manager.cpp +33/-0
  • src/node/block_template_manager.h +32/-1

Card

The short text the ranking pass compares across PRs.

Moves the getblocktemplate RPC cache from function-local statics into BlockTemplateManager. This eliminates process-wide global state where dangling CBlockIndex pointers could outlive ChainstateManager in tests, adds Clang thread-safety annotations under cs_main, and adds unit test coverage for template caching and invalidation rules. The refactor advances the BlockTemplateManager tracking issue #33758.

Data

dossier JSON · extract JSON · model ledger, generated 2026-10-30T01:12, confidence high, input hash 4218becfb5698fca
thread read through 2026-10-28 (11 statements), head ed3dd1469f
labels: Mining

Categories

Mining

P3 · cleanup (ranking pass: Moved up: it is the declared base of #36357, so reviewing it first is what makes the user-visible getmininginfo fix mergeable.)

Cleans up node lifetime and global state by moving static template caching into `BlockTemplateManager`. As noted in the description, this prevents 'cached CBlockIndex* can outlive its ChainstateManager (as in unit tests)', enables compile-time thread safety checks, and adds unit testing coverage for template invalidation.

Membership: Carries the Mining label, touches `BlockTemplateManager` and `rpc/mining.cpp`, and refactors the `getblocktemplate` caching logic.

Factors: security/stability 1, bug 1, performance 0, user value 0, leverage 1