#36451 refactor: Use NotNull for CreateNewBlock and pimpls
| Reviewability | Reviews | Agreement | Size | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| #36451 refactor: Use NotNull for CreateNewBlock and pimpls | CI failing | 0 | Strong | 30 + 13 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
Replaces nullable `std::unique_ptr` with `util::NotNullUniquePtr` (or plain references) for `CreateNewBlock`, `CreateNewTemplate`, and pimpl members across several subsystems (`AddrMan`, `MuSig2SecNonce`, `TxDownloadManager`, `TxReconciliationTracker`, `TxRequestTracker`, and `CDBWrapper`). It drops redundant null checks and assertions at call sites across tests, mining RPCs, and fee estimation, and expands `NotNull` unit tests with conversion and type-trait checks.
Problem: Pointers that are guaranteed never to be null are typed as standard nullable smart pointers. This obscures the API contract, leaves reviewers unsure whether pointers can be null, and produces dead error-handling branches in test coverage and mutation testing.
Evidence of importance:
- Author notes in the description that 'Pointers that are never null still have confusing null checks (with possible error handling return branches), which are always dead in coverage reports or when applying mutation testing.'
- Author notes in commit messages that the pimpl and test changes were suggested during review of PR #34844.
Risk of a mistake: medium (mempool_policy, rpc_api, tests). If block template generation unexpectedly returned a null pointer, removing the runtime error checks and wrapping in NotNull would cause an immediate crash or assertion failure instead of an error response.
Reviewability
Ready · CI failing
CI is red; check whether the failure is the PR's before a deep review.
Waiting on (from the thread read):
- nothing: no pending actions or reviews requested
Agreement
Strong
substantive support, no open objection (Sjors)
Thread notes: Added Concept ACK from Sjors. Sjors's inline remarks regarding commit splitting and the explicit constructor are non-items as commit splitting is not an item and the constructor comment explicitly affirms the current code is fine.
Objections, suggestions, and questions: none recorded.
Support:
- sedited (member) (Concept ACK, 2026-10-07): (no reason given) [not substantive]
- stickies-v (member) (Concept ACK, 2026-10-07): (no reason given) [not substantive]
- Sjors (member) (Concept ACK, 2026-10-20) [read: CreateNewBlock]: at least in the context of `CreateNewBlock` in fa0e2f600bf901f7c8d4e1574b4d3ad1e80a5cff it adds much useful clarity
Participants:
- sedited (member, support, 1 statements 2026-10-07 to 2026-10-07): Concept ACK
- stickies-v (member, support, 1 statements 2026-10-07 to 2026-10-07): Concept ACK
- hebasto (member, neutral, 1 statements 2026-10-14 to 2026-10-14): shared an MRE regarding MSVC compiler support
- Sjors (member, support, 3 statements 2026-10-20 to 2026-10-20): Concept ACK; commented on commit structure and NotNull explicitness
Files
21 lines under test/bench/ci.
- src/node/interfaces.cpp +5/-5
- src/node/block_template_manager.cpp +4/-4
- src/test/util_not_null_tests.cpp +8/-0
- src/rpc/mining.cpp +2/-4
- src/node/block_template_manager.h +3/-2
- src/dbwrapper.h +2/-2
- src/node/miner.cpp +2/-2
- src/test/fuzz/util/check_globals.h +3/-1
- src/test/util/mining.cpp +2/-2
- src/addrman.h +2/-1
- src/musig.h +2/-1
- src/node/miner.h +2/-1
- src/node/txdownloadman.h +2/-1
- src/node/txreconciliation.h +2/-1
- src/txrequest.h +2/-1
- src/test/validation_block_tests.cpp +0/-2
- src/policy/fees/mempool_estimator.cpp +0/-1
- src/test/miner_tests.cpp +0/-1
- src/test/peerman_tests.cpp +0/-1
- src/test/util/setup_common.cpp +0/-1
Card
The short text the ranking pass compares across PRs.
Replaces nullable smart pointers with util::NotNullUniquePtr for CreateNewBlock, CreateNewTemplate, and several internal pimpl members across the codebase. Currently, pointers guaranteed to be non-null are exposed as nullable pointers, prompting redundant null checks and dead error-handling branches. The author notes that compile-time non-null guarantees clarify API contracts and eliminate dead branches from coverage and mutation testing reports. No dependencies are declared.
Data
thread read through 2026-10-20 (9 statements), head fa0e2f600b
labels: Refactoring, CI failed · draft
Categories
Mining
P4 · cleanup
Clarifies non-null semantics for block template construction without changing mining behavior or template validity. Sjors notes that 'in the context of CreateNewBlock... it adds much useful clarity', but internal typing improvements and removal of dead null checks remain low-priority cleanup.
Membership: Modifies CreateNewBlock and BlockTemplateManager interfaces across src/node/miner.h, src/node/miner.cpp, and src/node/block_template_manager.h/cpp.
Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 0
P2P
P4 · cleanup
Replaces standard unique_ptr wrappers with NotNullUniquePtr for pimpl members across p2p classes. This is purely internal type hygiene with no effect on network behavior, protocol handling, or attack resistance.
Membership: Changes pimpl member declarations in p2p subsystem headers src/addrman.h, src/node/txdownloadman.h, src/node/txreconciliation.h, and src/txrequest.h.
Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 0
Utilities (logging, arguments, libraries)
P4 · cleanup
Internal refactoring to express pointer non-null contracts and eliminate dead branches in mutation testing and coverage reports. Addresses code clarity and dead check removal without affecting runtime safety or external behavior.
Membership: Adds unit tests for `NotNull` utility semantics and applies the wrapper across codebases to eliminate redundant null checks.
Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 0