#33922 mining: add getMemoryLoad() and track template non-mempool memory footprint
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #33922 mining: add getMemoryLoad() and track template non-mempool memory footprint | Ready | 0 (+3) | Strong | 152 + 26 tests | |||
|
|
|
|
| |||
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
Tracks the memory footprint of transactions retained by active block templates after being removed from the mempool, and exposes the total via a new `getMemoryLoad()` IPC mining interface method. `BlockTemplateManager` maintains reference counts for template transactions across template creation and destruction, calculating non-mempool memory on demand without holding locks across the mempool checks.
Problem: External IPC mining clients, such as Stratum v2 template providers, can hold references to block templates indefinitely. When transactions in those templates are evicted or replaced in the node's mempool, they remain pinned in memory without any mechanism for clients or node operators to inspect the resulting memory footprint.
Evidence of importance:
- Addresses issue #33899: 'Block template memory management (for IPC clients)'
- Commit a9eece073e notes: 'IPC clients can hold on to block templates indefinately... Because each template in turn tracks CTransactionRefs, transactions that are removed from the mempool will not have their memory cleared.'
- Client-side implementation demonstrated in Stratum v2 Template Provider PR stratum-mining/sv2-tp#63
Scope: Moves `CTransactionRefComp` from private broadcast code into `util/hasher.h` alongside a new `CTransactionRefSaltedHash` helper so template transaction references can be keyed in an `std::unordered_map`.
Risk of a mistake: low (rpc_api, other). The logic is isolated to the IPC Mining interface and BlockTemplateManager memory accounting; repeated calls could add minor lock contention on mempool.cs, but mempool state and consensus are untouched.
Changed since the previous assessment: Now copies transaction references under `m_mutex` before querying `mempool.exists()` unlocked in `GetTemplateMemoryUsage()`, avoiding lock contention on template creation/destruction, and adds benchmark timer logging.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- reviewer: review after rebase on #36469 2026-10-14
Agreement
Strong
substantive support, no open objection (ryanofsky, enirox001, xyzconstant)
Thread notes: Sjors rebased after #36469; PR is waiting on reviewer follow-up.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| ismaelsadeeq (member) | objection, approach | delegating template eviction responsibility to the client can cause out of memory crashes if handled poorly | answered · author explained client responsibility is inherent to multiprocess and FIFO eviction can be used as a fallback if needed | answered | yes | 2025-11-21: 'Delegating template eviction responsibility to the client can put us in a situation where they handle it poorly and cause us to OOM' Status from: 2025-11-24: Sjors: 'In the scenario where they handle it poorly, we can use FIFO deletion. All `getMemoryLoad()` does is give clients an opp' |
| ismaelsadeeq (member) | objection, approach | use internal memory management with a memory budget and FIFO template release rather than relying on IPC clients | answered · author explained internal memory management treats IPC clients no differently than internal code, and FIFO eviction can be a last resort | answered | yes | 2025-11-21: 'In my opinion, we should not rely on the IPC client to manage our memory.' Status from: 2025-11-21: Sjors: 'It seems counter intuitive, but from a memory management perspective IPC clients are treated no different than our own c' |
| brunoerg (member) | suggestion, correctness | add test coverage for the unkilled mutant in template_tx_refs reference counting | accepted · fix pushed · author adjusted interface_ipc.py test to cover the mutant, which reviewer confirmed | answered | yes | 2025-12-02: 'Just ran a mutation testing for this PR and this is the only unkilled mutant - feel free to ignore if it doesn't make sense to address' Status from: 2025-12-04: brunoerg: 'Nice, thank you!' |
| ryanofsky (member) | objection, safety | template_tx_refs map accessed across multiple threads without synchronization creates data race hazards | fixed · fix pushed · author added template_state_mutex | changed | yes | 2025-12-04: 'This map can updated from multiple threads, so it needs a mutex to be used safely.' Status from: 2025-12-05: Sjors: 'To limit the scope of this PR, I only added the mutex, but called it `template_state_mutex` in anticipation.' |
| ryanofsky (member) | question, approach | ask if there was a particular reason to assert that the bench log message is logged | answered · author explained it checks that bench logging happens | answered | yes | 2025-12-04: 'Seems ok to assert this log message is logged, but I'm wondering if there was a particular reason for doing this.' Status from: 2025-12-05: Sjors: 'This just checks that bench logging happens.' |
| vasild (member) | suggestion, approach | use std::unordered_map instead of std::map since ordered entries are not needed | fixed · fix pushed · author switched TxTemplateMap to std::unordered_map | answered | yes | 2026-03-03: 'std::map has lookup `O(log(size))` whereas `std::unordered_map` has `O(1)`. Here we do not need the entries to be ordered.' Status from: 2026-03-05: Sjors: 'Done both.' |
| vasild (member) | suggestion, correctness | use custom salted hasher and comparator for CTransactionRef in TxTemplateMap rather than comparing pointers | fixed · fix pushed · author adopted CTransactionRefSaltedHash and CTransactionRefComp | answered | yes | 2026-03-06: 'it is the same for `map` and `unordered_map` when used with `shared_ptr` (or `CTransactionRef`) - they compare pointers.' Status from: 2026-04-09: vasild: 'ACK 10acf818ed70d4cd2af864278a35a6b64de0bb16' |
| hebasto (member) | question, correctness | whether std::hash<CTransactionRef> parameter type should be CTransactionRef instead of const CTransactionRef& | answered · maflcko confirmed the docs refer to the logical type; the commit was later moved to #35101 | answered | yes | 2026-04-16: 'Should this be `CTransactionRef` instead of `const CTransactionRef&`?' Status from: 2026-04-17: maflcko: 'No, I am sure the docs just refer to the logical type' |
| maflcko (member) | objection, maintenance | IWYU workaround in primitives/transaction.h is wrong and harmful | fixed · fix pushed · IWYU workaround and CTransactionRef hasher commit moved to #35101 and dropped from this PR | changed | yes | 2026-04-17: 'NACK This is wrong and harmful, as already explained in https://github.com/bitcoin/bitcoin/pull/35073#issuecomment-4258599969' Status from: 2026-04-17: Sjors: 'The safety commit for `CTransactionRef` and IWYU changes are now in #35101 and dropped from this PR.' |
| ismaelsadeeq (member) | suggestion, scope | drop commit 596cb52b814e5e443f4f015cd5aac91915cff057 | fixed · fix pushed · author confirmed the commit was mistakenly kept and dropped it and the fuzzer extension in push | answered | yes | 2026-09-25: 'I think 596cb52 should be dropped.' Status from: 2026-09-28: Sjors: 'good catch, it was dropped from #35675 and I accidentally kept it in the rebase.' |
Support:
- ryanofsky (member) (Concept ACK, 2025-12-03) [read: getMemoryLoad()]: The `getMemoryLoad()` function seems useful by itself and the underlying tracking would seem to provide almost everything needed to limit memory used by block templates.
- vasild (member) (ACK, 2026-03-03): reviewed code c37d715c1d32020fe16ddc92f7ca95f585cc6b4b [not substantive]
- vasild (member) (ACK, 2026-03-06): reviewed code 04553cd6612f08d757fb20287fe111007bfb6db1 [not substantive]
- enirox001 (contributor) (ACK, 2026-04-06) [both: template_tx_refs, mempool.cs, interface_ipc_mining.py]: verified commit ordering, lifetime/locking of template_tx_refs and mempool.cs, and ran interface_ipc_mining.py
- vasild (member) (ACK, 2026-04-09): reviewed code 10acf818ed70d4cd2af864278a35a6b64de0bb16 [not substantive]
- vasild (member) (ACK, 2026-04-27): PGP-signed ACK for d37e0fc16c235ff511bd9485ef505f0c5615d9e4 [not substantive]
- xyzconstant (contributor) (Tested ACK, 2026-09-28) [both: mempool.cs, BlockTemplateManager]: Tested on mainnet from an sv2-tp instance consuming it for ~20 hours; verified mempool.cs locking trade-off and BlockTemplateManager tracking
Participants:
- ismaelsadeeq (member, objection, 3 statements 2025-11-21 to 2026-09-25): Concept ACK, but objected that delegating template eviction to clients rather than node FIFO eviction could lead to OOM; suggested dropping commit 596cb52
- brunoerg (member, neutral, 2 statements 2025-12-02 to 2025-12-04): reported unkilled mutant from mutation testing, which was resolved with a test update
- ryanofsky (member, support, 5 statements 2025-12-03 to 2025-12-04): Concept ACK; raised thread safety mutex requirement on template_tx_refs which was fixed
- vasild (member, support, 13 statements 2026-03-03 to 2026-04-27): approved across multiple rebases; suggested unordered_map and salted hasher for CTransactionRef map keys
- enirox001 (contributor, support, 1 statements 2026-04-06 to 2026-04-06): reviewed commit ordering, locking, and ran tests
- hebasto (member, question, 1 statements 2026-04-16 to 2026-04-16): asked about the std::hash parameter type for CTransactionRef
- maflcko (member, objection, 5 statements 2026-04-17 to 2026-04-17): NACKed IWYU workaround in primitives/transaction.h, leading Sjors to move the CTransactionRef hasher to #35101
- xyzconstant (contributor, support, 1 statements 2026-09-28 to 2026-09-28): Tested ACK; tested with sv2-tp on mainnet for ~20 hours and reviewed locking and tracking design
Files
33 lines under test/bench/ci.
- src/node/block_template_manager.cpp +67/-0
- src/node/block_template_manager.h +33/-0
- test/functional/interface_ipc_mining.py +25/-0
- src/util/hasher.h +16/-0
- src/interfaces/mining.h +15/-0
- src/node/interfaces.cpp +15/-0
- src/private_broadcast.h +1/-7
- src/test/fuzz/private_broadcast.cpp +1/-7
- src/ipc/capnp/mining.capnp +5/-0
Card
The short text the ranking pass compares across PRs.
Tracks the memory footprint of transactions pinned only by active block templates after leaving the node's mempool. BlockTemplateManager maintains reference counts for template transactions on template creation and destruction, and a new IPC method getMemoryLoad() exposes this non-mempool memory consumption to clients. This addresses block template memory management for external Stratum v2 mining providers (issue #33899) so clients can observe memory pressure and release stale templates.
Data
thread read through 2026-10-14 (80 statements), head 8ea66ad1cb
labels: Mining
Categories
Mining
P2 · new feature
Exposes template memory tracking via `getMemoryLoad()` to solve pinned non-mempool memory visibility for IPC mining clients. It directly supports external miner integration, addressing issue #33899 with an active consumer in Stratum v2 Template Provider (PR stratum-mining/sv2-tp#63).
Membership: Carries the Mining label and modifies interfaces::Mining, src/node/block_template_manager, and mining.capnp.
Factors: security/stability 1, bug 0, performance 0, user value 2, leverage 2