refactor: Use CBlockIndex parameters as reference #35229

optout21 commented at 7:11 AM on May 7, 2026: contributor

This is a janitorial refactor, a continuation of #34440. Input method parameters of type const CBlockIndex* are changed to reference const CBlockIndex& in some places where it makes no sense to call with a nullptr.

Motivation: slight improvement in code quality, maintainability and robustness.

more explicit representation of the non-null input invariant
less chance of nullptr-access error with future code changes
more explicit checks at call sites for the non-null input invariant.

The following methods are affected:

LastCommonAncestor() in chain.h
PeerManagerImpl::FindNextBlocks() (net_processing.cpp)
PeerManagerImpl::TryDownloadingHistoricalBlocks() (net_processing.cpp)
BlockFilterIndex::LookupFilterRange(), BlockFilterIndex::LookupFilterHashRange() (blockfilterindex.h), and LookupRange() in blockfilterindex.cpp
BaseIndex::ProcessBlock() (base.h)

General guidelines:

When a method has an input of type const CBlockIndex*, and by contract it cannot be nullptr, change it to const CBlockIndex&. The reference type is an implicit documentation of the nun-nullptr invariant.
When calling a method with reference parameter, ensure that it cannot be nullptr (it is often derived from a pointer), by/because:
- explicit prior check and error handling
- an assert/Assert/Assume
- itself being a non-nullptr invariant input
- it is being an output from a method with no-nullptr output invariant
For local variables, use reference types whenever possible, for non-nullptr references
For technical reasons pointers are still used even in some non-nullptr cases:
- containers/variables that can staty unitialized initially
- containers that need reserve()
- using reference type in std::set would result in some copy constructor calls
- std::optional cannot hold a reference type

DrahtBot added the label Refactoring on May 7, 2026

DrahtBot commented at 7:11 AM on May 7, 2026: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35229.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	stickies-v, musaHaruna

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:

#24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)

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.

optout21 force-pushed on May 7, 2026

DrahtBot added the label CI failed on May 7, 2026

in src/net_processing.cpp:928 in dfe6221407

 926 |      * \param vBlocks      Vector of blocks to download which will be appended to.
 927 | +    *                     Must not contain nullptr.
 928 |      * \param peer         Peer which blocks will be downloaded from.
 929 |      * \param state        Pointer to the state of the peer.
 930 | -    * \param pindexWalk   Pointer to the starting block to add to vBlocks.
 931 | +    * \param indexWalk    The starting block to add to vBlocks.

stickies-v commented at 7:52 AM on May 7, 2026:

nit: developer notes prefer camel_case while you're touching this

optout21 commented at 10:16 AM on May 20, 2026:

Done.

stickies-v commented at 7:52 AM on May 7, 2026: contributor

Concept ACK

DrahtBot removed the label CI failed on May 7, 2026

optout21 marked this as ready for review on May 9, 2026

musaHaruna commented at 11:36 AM on May 19, 2026: contributor

Concept ACK.

Refactor makes sense to me. Changing parameters from const CBlockIndex* to const CBlockIndex& where nullptr is not a valid state improves clarity and gives stronger compile-time guarantees around non-nullability by making the invariant explicit.

in src/chain.h:385 in dfe6221407

 381 | @@ -379,7 +382,7 @@ class CDiskBlockIndex : public CBlockIndex
 382 |  class CChain
 383 |  {
 384 |  private:
 385 | -    std::vector<CBlockIndex*> vChain;
 386 | +    std::vector<CBlockIndex*> vChain;  // pointer to be able to reserve, but never contains nullptr

musaHaruna commented at 11:37 AM on May 19, 2026:

Nit: We can make this comment a bit more concise and technically precise.

Right now it mentions reserve, but the ultimate underlying constraint is simply that std::vector fundamentally cannot hold references entirely. We can strip out the mention of the specific method and focus purely on the core safety invariant and the language rule.

    std::vector<CBlockIndex*> vChain;  // Pointers, never nullptr (std::vector cannot hold references)

maflcko commented at 11:50 AM on May 19, 2026:

If the code-typing overhead of .get() is not too much, why not just use std::reference_wrapper, which enforces everything at compile-time without any runtime overhead and does not rely on a code comment?

optout21 commented at 11:11 AM on May 20, 2026:

@musaHaruna , @maflcko : I was incorrect here, what I meant is that if reference_wrapper's are used, then resize() has a problem (not reserve()!), because it would call the default constructor, and CBlockIndex does not have one. But I realized now that this can be solved (by passing an instance to resize). So what I did:

Simplified the comment, as suggested, to only "Pointers, never nullptr"
I tried using reference_wrapper locally in PeerManagerImpl::FindNextBlocks(). I think the get() is not a big inconvenience, and the zero memory and runtime overhead is important. In there the resize sets them all to a (*nullptr) initially, which kind of defies the purpose, but at least the intent is clear. I put this in a separate commit.

I may reconsider keeping the reference_wrapper commit or not, pending review. I will also consider using reference_wrapper in the CChain class member vector, which is a riskier change.

in src/index/base.cpp:252 in dfe6221407 outdated

 248 | @@ -249,7 +249,7 @@ void BaseIndex::Sync()
 249 |              pindex = pindex_next;
 250 |  
 251 |  
 252 | -            if (!ProcessBlock(pindex)) return; // error logged internally
 253 | +            if (!ProcessBlock(*pindex)) return; // error logged internally

musaHaruna commented at 11:44 AM on May 19, 2026:

I think we can explicitly pass the default nullptr with an inline comment here to match the codebase convention.

      if (!ProcessBlock(*pindex, /*block_data=*/nullptr)) return; // error logged internally

optout21 commented at 10:18 AM on May 20, 2026:

This is not related to this PR, plus I don't see why to explicitly pass an optional parameters, so not taking this.

in src/chain.cpp:177 in dfe6221407

 173 | @@ -173,5 +174,5 @@ const CBlockIndex* LastCommonAncestor(const CBlockIndex* pa, const CBlockIndex*
 174 |          pa = pa->pprev;
 175 |          pb = pb->pprev;
 176 |      }
 177 | -    return pa;
 178 | +    return *Assume(pa);

maflcko commented at 11:49 AM on May 19, 2026:

As explained previously, this seems confusing, see #34440 (review)

optout21 commented at 10:27 AM on May 20, 2026:

Indeed, thanks for the reminder! I wanted to have here something to "code-show" the return-is-not-nullptr assumption. I've double-checked the usages (call sites), and this can't happen -- all chain segments must have the same genesis. Therefore I've changed it to Assert. Similarly changed one other introduced Assume to Assert in net_processing.cpp.

refactor: In LastCommonAncestor, change CBlockIndex input to reference 1c3d6b2c96

refactor: In FindNextBlocks, change CBlockIndex input to reference 63c9b996f4

optout21 commented at 11:15 AM on May 20, 2026: contributor

Did changes, following recent reviews. Thanks for the reviews! What's changed:

Change of Assume() to more appropriate Assert() (2 places).
Inside PeerManagerImpl::FindNextBlocks(), for the vToFetch vector, use std::reference_wrapper<const CBlockIndex> instead of pointers (new commit!)
Some comment adjustments.
Some minor renaming of touched variable/parameters.

git range-diff dfe62214070408e699067598309b779c1ed1214d...ed44574efd38d4b7815ab96ae6f68f8b02dee88a

optout21 force-pushed on May 20, 2026

DrahtBot added the label CI failed on May 20, 2026

DrahtBot commented at 11:30 AM on May 20, 2026: contributor

🚧 At least one of the CI tasks failed. <sub>Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/26158964766/job/76945600531</sub> <sub>LLM reason (✨ experimental): CI failed due to a compiler error from Clang -Werror: null pointer dereference to a reference in net_processing.cpp (line 1485).</sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

optout21 force-pushed on May 20, 2026

optout21 commented at 11:37 AM on May 20, 2026: contributor

Pushed compile fix: initializing the reference_wrapper with std::ref(*(CBlockIndex*)nullptr) gave a build error on MacOS (but not locally). Now it's filled with the index_walk input parameter as a placeholder.

git range-diff dfe62214070408e699067598309b779c1ed1214d...82fdd038cfe379c0dbdfda37e3504f97f8e21564
git range-diff ed44574efd38d4b7815ab96ae6f68f8b02dee88a...82fdd038cfe379c0dbdfda37e3504f97f8e21564
git range-diff dfe62214070408e699067598309b779c1ed1214d...ed44574efd38d4b7815ab96ae6f68f8b02dee88a

DrahtBot removed the label CI failed on May 20, 2026

refactor: Assert and comments to ensure vBlocks never contains null cec59ef3b6

refactor: In TryDownloadingHistoricalBlocks, change CBlockIndex input to reference 555e7e32e2

refactor: In LookupX methods, change CBlockIndex input to reference

Methods affected: BlockFilterIndex::LookupFilterRange, BlockFilterIndex::LookupFilterHashRange,
and LookupRange, in blockfilterindex.

91e1110aae

refactor: In ProcessBlock change CBlockIndex input to reference b7cc8b99ee

optout21 marked this as a draft on May 26, 2026

optout21 force-pushed on May 26, 2026

optout21 commented at 9:18 AM on May 26, 2026: contributor

Performed changes:

Reverted last addition of storing reference_wrapper's inside PeerManagerImpl::FindNextBlocks(). That's an internal, local usage, so there is not much benefit of this change there, hence I reverted it.
Did change CChain::vChain array to store std::reference_wrapper<CBlockIndex> items instead of pointers (new commit). This is to better show and ensure that entries are never null. The interfaces returning pointers are not changed.

git range-diff 82fdd038cfe379c0dbdfda37e3504f97f8e21564...a1ed86a7e66c4f13dc58fab71982eba3340602e0

optout21 marked this as ready for review on May 26, 2026

refactor: Store reference_wrapper items within CChain class

In the `vChain` internal array of `CChain` class, store items as
`std::reference_wrapper<CBlockIndex>`, instead of `CBlockIndex*` pointers.
This gives a better guarantee that the entries are not null.
The APIs returning pointers are not changed.
Note that the entries are not const references, due to the need to update
during validation.

a1ed86a7e6

optout21 force-pushed on May 26, 2026

DrahtBot added the label CI failed on May 26, 2026

optout21 closed this on May 26, 2026

optout21 reopened this on May 26, 2026

DrahtBot removed the label CI failed on May 26, 2026

optout21 commented at 6:42 AM on May 27, 2026: contributor

There was an unrelated CI failure, but on the second try all CI builds passed. The failure was in "Windows, ucrt, test cross-built":

Error: fatal: unable to access 'https://github.com/bitcoin/bitcoin/': The requested URL returned error: 403

seemingly an intermittent infrastructure issue, most likely unrelated to this PR.

https://github.com/bitcoin/bitcoin/actions/runs/26446276736/job/77856772060?pr=35229