refactor: Use NotNull for CreateNewBlock and pimpls #36451

maflcko commented at 11:35 AM on October 7, 2026: member

Current code uses nullable smart pointers for some stuff that is never null. This is perfectly fine, but a bit confusing:

When reading the code, it is not clear which pointer is nullable or never null.
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.
While some places have the redundant null checks on pointers that are never null, other places do not, which adds to the confusion.

Clarify this at compile-time by using NotNull (or a plain reference)

test: Add some NotNull test cases

Suggested in https://github.com/bitcoin/bitcoin/pull/34844#pullrequestreview-5427569147

fa5b5027fb

refactor: Mark pimpls as NotNull

Suggested in https://github.com/bitcoin/bitcoin/pull/34844#pullrequestreview-5268474301

Mostly to see at compile time which m_impl is nullable. E.g.:
src/ipc/libmultiprocess/include/mp/proxy-io.h:  m_impl.reset();

fa7a0e7744

refactor: Return NotNull from CreateNewBlock

A nullptr is never returned, so the existing code is confusing, because
it either has redundant and dead checks for a nullptr, or the existing
code is "missing" them.

Fix both issues by marking the type as NotNull at compile-time.

fa0e2f600b

DrahtBot renamed this:
~~refactor: Use NotNull for CreateNewBlock and pimpls~~
refactor: Use NotNull for CreateNewBlock and pimpls
on Oct 7, 2026

DrahtBot added the label Refactoring on Oct 7, 2026

DrahtBot commented at 11:35 AM on October 7, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36451
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36451.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36451#author-ralph

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	sedited, stickies-v, Sjors

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36641 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36641.svg"> (wallet: bound pending MuSig2 sessions by fametrano)
#36598 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36598.svg"> ([Demo] validation: remove AssumeUTXO by l0rinc)
#36525 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36525.svg"> (mining: add getTotalFees(), protect m_block_template with mutex by Sjors)
#36357 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36357.svg"> (mining: move BlockAssembler statics into BlockTemplateManager by w0xlt)
#35671 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35671.svg"> (mining: add TxCollection to bandwidth-efficiently validate external block templates by Sjors)
#35511 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35511.svg"> (RFC: consensus: Make CAmount a class by hodlinator)
#33922 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/33922.svg"> (mining: add getMemoryLoad() and track template non-mempool memory footprint by Sjors)
#33117 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/33117.svg"> (Interfaces: Expose UTXO Snapshot Loading and Add Progress Notifications by D33r-Gee)
#32468 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/32468.svg"> (rpc: generateblock to allow multiple outputs by polespinasa)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

sedited commented at 11:54 AM on October 7, 2026: contributor

Concept ACK

maflcko marked this as a draft on Oct 7, 2026

maflcko commented at 12:06 PM on October 7, 2026: member

The Windows MSVC bugs are a bit tiring. Can someone remove it? Ref: https://github.com/bitcoin/bitcoin/issues/36153

DrahtBot added the label CI failed on Oct 7, 2026

stickies-v commented at 1:22 PM on October 7, 2026: contributor

Concept ACK

hebasto commented at 7:55 AM on October 14, 2026: member

The Windows MSVC bugs are a bit tiring.

Here is an MRE: https://godbolt.org/z/Kadaq5h46.

However, https://en.cppreference.com/cpp/compiler_support/20 lists P1825R0 as implemented in MSVC since v19.24 VS 2019 16.4.

in src/node/block_template_manager.cpp:130 in fa0e2f600b

 126 | @@ -127,7 +127,7 @@ void BlockTemplateManager::InterruptWait(bool& interrupt_wait)
 127 |  }
 128 |  
 129 |  std::unique_ptr<CBlockTemplate> BlockTemplateManager::WaitAndCreateNewBlock(
 130 | -    const std::unique_ptr<CBlockTemplate>& block_template,
 131 | +    const CBlockTemplate& block_template,

Sjors commented at 3:42 AM on October 20, 2026:

In fa0e2f600bf901f7c8d4e1574b4d3ad1e80a5cff refactor: Return NotNull from CreateNewBlock: this might be worth its own commit, e.g. https://github.com/Sjors/bitcoin/commit/d5591a9143cae9829fb1c8a375b88b4297ff7070

maflcko commented at 6:26 AM on October 20, 2026:

I think it is easier to review knowing that it can not be null, so I'll leave as-is for now, but happy to rebase on your commit, if other reviewers prefer that.

in src/node/miner.cpp:243 in fa0e2f600b

 239 | @@ -240,7 +240,7 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock()
 240 |               Ticks<MillisecondsDouble>(time_2 - time_1),
 241 |               Ticks<MillisecondsDouble>(time_2 - time_start));
 242 |  
 243 | -    return std::move(pblocktemplate);
 244 | +    return util::NotNull{std::move(pblocktemplate)};

Sjors commented at 3:51 AM on October 20, 2026:

In fa0e2f600bf901f7c8d4e1574b4d3ad1e80a5cff refactor: Return NotNull from CreateNewBlock: having to add util::NotNull{...} is a bit tedious. But if you forget, the error is clear enough:

note: explicit constructor is not a candidate
   49 |     constexpr explicit NotNull(U&& u) noexcept(...)
      |                        ^

(the current code is fine, I don't think we should make the constructor implicit)

maflcko commented at 6:25 AM on October 20, 2026:

Yes, being explicit is intentional, since the NotNull ctor acts as an Assert on the value.

Sjors commented at 4:01 AM on October 20, 2026: member

Concept ACK, at least in the context of CreateNewBlock in fa0e2f600bf901f7c8d4e1574b4d3ad1e80a5cff it adds much useful clarity.