Refactor CChain methods to use references, tests

optout21 commented at 9:21 am on January 29, 2026: contributor

Refactor CChain methods (Contains(), Next(), etc.) to use references instead of pointers, to minimize the risk of accidental nullptr dereference (memory access violation). Also add missing unit tests to the CChain class.

The CChain::Contains() method (in src/chain.h) dereferences its input without checking. The Next() method also calls into this with a nullptr if invoked with nullptr. While most call sites have indirect guarantee that the input is not nullptr, it’s not easy to establish this to all call sites with high confidence. These methods are publicly available. There is no known high-level use case to trigger this error, but the fix is easy, and makes the code safer.

Changes:

Add basic unit tests for CChain class methods
Refactor CChain::Contains() to take reference
Refactor CChain::Next() to take reference
Add unit tests for CChain::FindFork()
Change PeerManagerImpl::BlockRequestAllowed() to take reference

TODO:

? /Change CChain internals to store references instead of pointers/
? /Explore the change to always have at least one element (genesis), that way there is always genesis and tip./
Check related methods to change to reference – FindFork, FindEarliestAtLeast, FindForkInGlobalIndex, blockman.AddToBlockIndex, etc.

Alternative. A simpler change is to stick with pointers, with extra checks where needed, see #34416 .

This change is remotely related to and indirectly triggered by #32875 .

DrahtBot commented at 9:21 am on January 29, 2026: contributor

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

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34565 (refactor: extract BlockDownloadManager from PeerManagerImpl by w0xlt)
#34489 (index: batch db writes during initial sync by furszy)
#34416 (Add nullptr-check to CChain::Contains(), tests by optout21)
#33752 (rest: Query predecessor headers using negative count param by A-Manning)
#33477 (Rollback for dumptxoutset without invalidating blocks by fjahr)
#32950 (validation: remove BLOCK_FAILED_CHILD by stratospher)
#32875 (index: handle case where pindex_prev equals chain tip in NextSyncBlock() by HowHsu)
#29770 (index: Check all necessary block data is available before starting to sync by fjahr)

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 Jan 29, 2026

DrahtBot added the label CI failed on Jan 29, 2026

DrahtBot commented at 9:49 am on January 29, 2026: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/21472539937/job/61848421163 LLM reason (✨ experimental): Lint check failed: RPC code uses fatal asserts (rpc_assert), which the linter flags as inappropriate, causing the lint step to exit with an error.

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.

optout21 force-pushed on Jan 29, 2026

in src/chain.h:423 in d228e16979

422     }
423 
424-    /** Find the successor of a block in this chain, or nullptr if the given index is not found or is the tip. */
425-    CBlockIndex* Next(const CBlockIndex* pindex) const
426+    /** Find the successor of a block in this chain, or nullopt if the given index is not found or is the tip. */
427+    CBlockIndexOptRef Next(const CBlockIndex& pindex) const

maflcko commented at 10:19 am on January 29, 2026:

I don’t understand this change. An optional reference is a pointer. Using something other than a pointer is just extra steps.

If you want to change the code, my recommendation would be to look at high level functions that take a block as argument, and then see if it makes sense to pass that block index as a reference.

See also #34416 (review)

optout21 commented at 10:32 am on January 29, 2026:

An optional reference is a pointer.

My reasoning was, that if input are references, the return value should be as well, since result of Next often will become input. The key point is that the return value must be always checked. A pointers can be turned into a reference, and an unchecked nullptr can be also. Thinking a bit more, even an nullopt optional can be dereferences, and the behaviour is undefined. So it’s not better in this regard, while it is more clutter for sure. Thanks for the feedback!

optout21 commented at 11:03 am on January 29, 2026:

Changed Next to take reference, but keep the return pointer.

optout21 force-pushed on Jan 29, 2026

optout21 commented at 8:08 am on January 30, 2026: contributor

Added new commit to change PeerManagerImpl::BlockRequestAllowed() to take reference, local to src/net_processing.cpp.

optout21 commented at 11:00 am on February 3, 2026: contributor

Added new commit to change CChain::FindFork() to take reference.

DrahtBot removed the label CI failed on Feb 3, 2026

fanquake referenced this in commit 9d76947294 on Feb 5, 2026

DrahtBot added the label Needs rebase on Feb 5, 2026

Add CChain basic tests

Add basic unit tests to the `CChain` class, filling a gap.

317d7aa811

Add CChain::FindFork() tests

Add (lengthier) unit tests for `CChain::FindFork()`.

37f020c714

optout21 force-pushed on Feb 5, 2026

Change CChain::Contains() to take reference

The `CChain::Contains()` method dereferences its input without checking,
potentially resulting in nullptr-dereference if invoked with `nullptr`.
To avoid this possibility, its input is changed to a reference instead.
Call sites are adapted accoringly. In a few cases an extra check
is added to ensure the input is not null (if guard or only `Assume`,
depending on the context.

9e833e118b

Rework CChain::Next() to take reference

To minimize chance of erroneous nullptr dereference, `CChain::Next()`
is changed to take a reference instead of a pointer.
Call sites have been adapted.

7736f2fdc2

Change CChain::FindFork() to take ref b050d0b51b

Header fix 97fd30f3b0

Lint fix (Rpc assume) 32457f42fd

optout21 force-pushed on Feb 5, 2026

DrahtBot added the label CI failed on Feb 5, 2026

optout21 commented at 11:50 pm on February 5, 2026: contributor

Rebased, following #34464 ; one commit got annihilated.

DrahtBot removed the label Needs rebase on Feb 6, 2026

7zkm7b8gw9-web commented at 7:29 am on February 6, 2026: none

Yes

Refactor CChain methods to use references, tests #34440

Reviews

Conflicts