#36451 refactor: Use NotNull for CreateNewBlock and pimpls

ReviewabilityReviewsAgreementSize
#36451 maflcko refactor: Use NotNull for CreateNewBlock and pimplsCI failing0Strong30 + 13 tests
  • Clarify pointer contracts by enforcing non-null semantics for block creation and internal implementations
  • Eliminate confusing null checks and dead error-handling branches in tests and callers
  • CI is failing so check whether the failure belongs to this PR before a deep review
  • Concept approval because it adds useful clarity to block template creation (Sjors)
  • Concept approval without stated reasons (sedited, stickies-v)
  • 30 lines added or modified outside tests
  • 13 lines added or modified in tests
  • 34 lines removed in total
  • 20 files, 3 commits
Mining#17 of 17
P4 · cleanup
  • P4 because internal typing improvements and dead null check removal in block creation are low-priority cleanup
  • Does not change mining behavior, template construction, or template validity
P2P#69 of 71
P4 · cleanup
  • P4 because changing internal pimpl pointers across p2p classes is purely internal type hygiene
  • Does not affect network behavior, protocol handling, or attack resistance
Utilities (logging, arguments, libraries)#56 of 59
P4 · cleanup
  • P4 because expressing pointer non-null contracts and expanding unit tests is internal code cleanup
  • Cleans dead branches in coverage and mutation testing without affecting runtime safety or external behavior
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

dossier JSON · extract JSON · model ledger, generated 2026-10-21T01:19, confidence high, input hash 31bf09ff01c4b4f3
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