#36525 mining: add getTotalFees(), protect m_block_template with mutex

ReviewabilityReviewsAgreementSize
#36525 Sjors mining: add getTotalFees(), protect m_block_template with mutexReady3Strong69 + 61 tests
  • Prevent data races when concurrent IPC worker threads submit solutions while reading active block templates
  • Allow external mining clients to fetch total template fees directly instead of summing individual fees
  • Ready for review
  • Substantive support for resolving the race condition and adding the fee query (ismaelsadeeq, enirox001)
  • Verified the race fix with a concurrent template access test under ThreadSanitizer (enirox001)
  • Suggested making templates immutable instead of locking, deferred due to IPC latency benchmarks (sedited)
  • Concept and code approval without detailed comments (w0xlt)
  • 69 lines added or modified outside tests
  • 61 lines added or modified in tests
  • 33 lines removed in total
  • 9 files, 5 commits
Mining#1 of 17
P1 · bug fix
  • P1 after comparing with the other PRs here (assessed alone as P2): Raised to P1: the only PR fixing a concrete reported data race (#36515) in the interface external miners already use; leaving it at P2 with the feature PRs would flatten a correctness bug into the same level as enhancements.
  • P2 because it fixes a data race when concurrent IPC worker threads access templates during block submission
  • Improves stability and completeness of the mining interface relied on by external Stratum v2 clients
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

Protects block template access in the mining interface implementation with a mutex to serialize concurrent IPC worker thread operations. Adds an `m_total_fees` field to `CBlockTemplate` along with a corresponding `getTotalFees()` getter exposed over IPC. Modifies `WaitAndCreateNewBlock` to accept the previous block hash and fee values by value rather than inspecting the template pointer directly, allowing callers to release the template lock before waiting.

Problem: Concurrent IPC worker threads can trigger data races when one thread modifies an active `CBlockTemplate` during `submitSolution()` while another thread reads from it. Furthermore, external IPC mining clients requiring total fee figures have to repeatedly retrieve and sum individual transaction fees across the IPC boundary.

Evidence of importance:

  • Commit 4774cf1de6 and PR description state that IPC calls on different worker threads can race when submitSolution modifies a template while another call reads it, fixing issue #36515.

Risk of a mistake: medium (other). Mistakes in mutex scoping or lock inversion in BlockTemplateImpl could deadlock IPC mining worker threads or cause mining block submissions to stall.

Changed since the previous assessment: Added a Python functional test (run_concurrent_template_access_test in interface_ipc_mining.py) that exercises concurrent reads and submitSolution operations across separate Cap'n Proto worker threads.

Reviewability

Ready

Nothing known that would invalidate a review now.

Waiting on (from the thread read):

  • reviewer: sedited to respond to benchmark results and proposed deferral of making template const 2026-10-29

Agreement

Strong

substantive support, no open objection (ismaelsadeeq, enirox001)

Thread notes: Sjors benchmarked making the block template immutable and returning the full block in submitSolution(), identifying an IPC single-thread latency penalty in sv2-tp and recommending deferring immutability until IPC improvements land.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
ismaelsadeeq (member)question, interfaceIs returning a vector of individual fees redundant now that total fees can be returned directly?answered · Sjors explained the vector is kept for compatibility with getblocktemplate and possible miner dashboard use casesansweredyes2026-10-15: 'It makes sense to return it so they don't have to sum them, so the vector one is now redundant?'
Status from: 2026-10-15: Sjors: 'Providing the total and individual via IPC is trivial, so I think we should keep it.'
ismaelsadeeq (member)suggestion, safetyAdd a concurrent template access functional test to exercise the data race under TSANaccepted · fix pushed · author added test in c:6205291499 and reviewer reACKedansweredyes2026-10-15: 'See minimal test that fails on our tsan functional tests.'
Status from: 2026-10-16: ismaelsadeeq: 'reACK cbadeff334f182086538d7483dc1146c559859e6'
enirox001 (contributor)suggestion, correctnessinclude the concurrent template access test as it consistently caught the regression under TSANaccepted · fix pushed · author added test in c:6205291499 and reviewer reACKedansweredyes2026-10-15: 'I think the test is worth including. even though reproduction depends on scheduling, it consistently caught the regression on my machine'
Status from: 2026-10-16: enirox001: 'reACK https://github.com/bitcoin/bitcoin/pull/36525/changes/cbadeff334f182086538d7483dc1146c559859e6'
sedited (member)suggestion, approachMake m_block_template non-mutating/const instead of protecting it with a mutexanswered · author benchmarked the immutable approach, found sv2-tp latency regressions due to IPC bottlenecks, and suggested deferringansweredyes2026-10-21: 'Might it be preferable to make m_block_template non-mutating? Do users of the interface even need the mutated template after submitting?'
Status from: 2026-10-29: Sjors: 'So my preference would be to defer making CBlockTemplate const until after these improvements land.'
ismaelsadeeq (member)suggestion, interfaceReturn the block copy after applying the solutionanswered · author explained clients intentionally wait 1 second before requesting it so as not to distract the nodeansweredyes2026-10-25: 'Could we just return the block copy after applying the solution?'
Status from: 2026-10-25: Sjors: 'in theory yes, but clients intentionally wait 1 second before requesting it.'

Support:

  • w0xlt (contributor) (Concept ACK, 2026-10-15): Concept ACK [not substantive]
  • ismaelsadeeq (member) (ACK, 2026-10-15) [read]: ACK 4774cf1de6a4670323686bf93beb93035b3f6dad
  • enirox001 (contributor) (ACK, 2026-10-15) [tested]: Fixes a data race between IPC calls modifying and reading a template, adds getTotalFees IPC method, tested with TSAN
  • ismaelsadeeq (member) (ACK, 2026-10-16): reACK cbadeff334f182086538d7483dc1146c559859e6 [not substantive]
  • enirox001 (contributor) (ACK, 2026-10-16): reACK https://github.com/bitcoin/bitcoin/pull/36525/changes/cbadeff334f182086538d7483dc1146c559859e6 [not substantive]
  • w0xlt (contributor) (ACK, 2026-10-20): ACK cbadeff334 [not substantive]

Participants:

  • enirox001 (contributor, support, 2 statements 2026-10-15 to 2026-10-16): ACKed with TSAN testing, supported including the test, and reACKed after the test was added
  • ismaelsadeeq (member, support, 5 statements 2026-10-15 to 2026-10-25): ACKed, proposed a functional test, reACKed once the test was included, and suggested returning the block copy after applying the solution
  • w0xlt (contributor, support, 3 statements 2026-10-15 to 2026-10-21): ACKed with doc nits, and supported the suggestion to make m_block_template non-mutating
  • sedited (member, question, 3 statements 2026-10-21 to 2026-10-28): suggested making m_block_template non-mutating instead of adding mutex protection, and pushed back on whether block copying distraction is a valid concern

Files

61 lines under test/bench/ci.

  • test/functional/interface_ipc_mining.py +57/-0
  • src/node/interfaces.cpp +33/-13
  • src/node/block_template_manager.cpp +4/-15
  • src/node/block_template_manager.h +16/-3
  • src/interfaces/mining.h +12/-2
  • src/test/miner_tests.cpp +4/-0
  • src/node/miner.h +2/-0
  • src/ipc/capnp/mining.capnp +1/-0
  • src/node/miner.cpp +1/-0

Card

The short text the ranking pass compares across PRs.

This PR resolves a multithreading data race in the mining IPC interface where concurrent worker threads access m_block_template simultaneously, such as when submitSolution modifies a template while another call reads it. It guards template accesses inside BlockTemplateImpl with a mutex and updates WaitAndCreateNewBlock so the template lock can be released before entering the wait loop. In addition, it caches total fees on CBlockTemplate and introduces a getTotalFees() IPC method so clients do not have to aggregate per-transaction fees. The primary motivation is fixing issue #36515.

Data

dossier JSON · extract JSON · model ledger, generated 2026-10-30T01:14, confidence high, input hash df1f7122cce5ecca
thread read through 2026-10-29 (22 statements), head cbadeff334
labels: Mining

Categories

Mining

P1 · bug fix (assessed alone as P2; Raised to P1: the only PR fixing a concrete reported data race (#36515) in the interface external miners already use; leaving it at P2 with the feature PRs would flatten a correctness bug into the same level as enhancements.)

Resolves a data race where concurrent IPC worker threads access `CBlockTemplate` during `submitSolution()` while another thread reads from it. Ensuring thread-safe template access and exposing `getTotalFees()` directly improves the stability and completeness of the mining interface external Stratum v2 mining consumers rely on.

Membership: Has the Mining label and modifies `src/interfaces/mining.h`, `src/node/block_template_manager.*`, and `src/ipc/capnp/mining.capnp`.

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