Refactor CChain methods to use references, tests

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

Refactor CChain methods (Contains(), Next(), FindFork()) 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
Add unit tests for CChain::FindFork()
Refactor CChain::Contains() to take reference
Refactor CChain::Next() to take reference
Refactor CChain::FindFork() to take reference
Change pindexMostWork parameter of ActivateBestChainStep() to reference

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 .

Further ideas, not considered in this PR:

Change CChain internals to store references instead of pointers
Change CChain to always have at least one element (genesis), that way there is always genesis and tip.
Check related methods to return reference (guaranteed non-null) – FindFork, FindEarliestAtLeast, FindForkInGlobalIndex, blockman.AddToBlockIndex, etc.

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.

Type	Reviewers
Concept ACK	hodlinator

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#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)
#32875 (index: handle case where pindex_prev equals chain tip in NextSyncBlock() by HowHsu)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

nulltr -> nullptr [Typo in comment: “nulltr” is clearly intended to mean nullptr; as written it harms comprehension.]

2026-03-27 13:51:58

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

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

DrahtBot added the label Needs rebase on Feb 18, 2026

optout21 force-pushed on Mar 12, 2026

DrahtBot removed the label CI failed on Mar 13, 2026

DrahtBot removed the label Needs rebase on Mar 13, 2026

7zkm7b8gw9-web commented at 1:57 am on March 14, 2026: none

?

in src/index/base.cpp:165 in f6ae31af67

162     }
163 
164     // Since block is not in the chain, return the next block in the chain AFTER the last common ancestor.
165     // Caller will be responsible for rewinding back to the common ancestor.
166-    return chain.Next(chain.FindFork(pindex_prev));
167+    Assume(pindex_prev); // should have been checked above

hodlinator commented at 9:11 pm on March 26, 2026:

We just dereferenced pindex_prev a few lines above, I think this is redundant.

optout21 commented at 11:51 am on March 27, 2026:

Done.

in src/validation.cpp:3193 in f6ae31af67 outdated

3189@@ -3190,7 +3190,7 @@ bool Chainstate::ActivateBestChainStep(BlockValidationState& state, CBlockIndex*
3190     if (m_mempool) AssertLockHeld(m_mempool->cs);
3191 
3192     const CBlockIndex* pindexOldTip = m_chain.Tip();
3193-    const CBlockIndex* pindexFork = m_chain.FindFork(pindexMostWork);
3194+    const CBlockIndex* pindexFork = pindexMostWork ? m_chain.FindFork(*pindexMostWork) : nullptr;

hodlinator commented at 9:18 pm on March 26, 2026:

pindexMostWork can be changed to a reference as well, only caller of ActivateBestChainStep() guards with

0                if (pindexMostWork == nullptr || pindexMostWork == m_chain.Tip()) {
1                    break;
2                }

optout21 commented at 1:28 pm on March 27, 2026:

Done.

in src/index/base.cpp:159 in f6ae31af67

154@@ -155,14 +155,17 @@ static const CBlockIndex* NextSyncBlock(const CBlockIndex* pindex_prev, CChain&
155         return chain.Genesis();
156     }
157 
158-    const CBlockIndex* pindex = chain.Next(pindex_prev);
159+    const CBlockIndex* pindex = chain.Next(*pindex_prev);
160     if (pindex) {

hodlinator commented at 9:23 pm on March 26, 2026:

nit: Could explicitly reduce scope of pindex since it’s not used after the if-block.

0    if (const CBlockIndex* pindex{chain.Next(*pindex_prev)}) {

optout21 commented at 11:52 am on March 27, 2026:

Done.

in src/init.cpp:2350 in f6ae31af67 outdated

2346@@ -2347,8 +2347,9 @@ bool StartIndexBackgroundSync(NodeContext& node)
2347             {
2348                 LOCK(::cs_main);
2349                 pindex = chainman.m_blockman.LookupBlockIndex(summary.best_block_hash);
2350-                if (!index_chain.Contains(pindex)) {
2351-                    pindex = index_chain.FindFork(pindex);
2352+                if (!pindex) break;

hodlinator commented at 9:29 pm on March 26, 2026:

On master, this would just have crashed inside of Contains() - right? I think we might want to return an InitError() or at least do a LogError() as the block manager should be able to find all blocks we have indexed unless a monkey has been going wild on our datadir filesystem.

optout21 commented at 1:42 pm on March 27, 2026:

I’ve realized my change here was incorrect: the break will exit the outer node.indexes loop, which was not intended (the inner block is a lock block, not a break-able loop). The !pindex case is handled just after the lock block, by taking the genesis. The extra check only needs to make sure that calling Contains is skipped if pindex is null, and the flow continues. Fixing.

in src/net_processing.cpp:2861 in f6ae31af67 outdated

2857@@ -2858,7 +2858,7 @@ void PeerManagerImpl::HeadersDirectFetchBlocks(CNode& pfrom, const Peer& peer, c
2858         // very large reorg at a time we think we're close to caught up to
2859         // the main chain -- this shouldn't really happen.  Bail out on the
2860         // direct fetch and rely on parallel download instead.
2861-        if (!m_chainman.ActiveChain().Contains(pindexWalk)) {
2862+        if (!m_chainman.ActiveChain().Contains(*pindexWalk)) {

hodlinator commented at 9:35 pm on March 26, 2026:

I wonder if we can reach the genesis block in the above loop with a null pprev. We might as well crash if we are reorging that ~~for~~ far on the main chain, but I’m thinking it might be worth supporting on test chains.

(Contains() would already have crashed for nullptr on master, so this is not a behavior change).

optout21 commented at 10:46 am on March 27, 2026:

I will check. Here we could change pindexWalk to reference, and vToFetch vector to hold references as well.

optout21 commented at 1:25 pm on March 27, 2026:

Even though apparently this never gets to be nullptr in the wild, in order to be on the safe side, I’ve added a !pindexWalk || clause to this condition. There is no guarantee in the code that his never happens (it depends on many factors, such as the value of MAX_BLOCKS_IN_TRANSIT_PER_PEER).

in src/rpc/blockchain.cpp:1626 in f6ae31af67

1623+        const int branchLen = block->nHeight - active_chain.FindFork(*block)->nHeight;
1624         obj.pushKV("branchlen", branchLen);
1625 
1626         std::string status;
1627-        if (active_chain.Contains(block)) {
1628+        if (block && active_chain.Contains(*block)) {

hodlinator commented at 9:40 pm on March 26, 2026:

Would remove these extra checks, we dereferenced block just above.

0        const int branchLen = block->nHeight - active_chain.FindFork(*block)->nHeight;
1obj.pushKV("branchlen", branchLen);
2
3std::string status;
4        if (active_chain.Contains(*block)) {

optout21 commented at 10:53 am on March 27, 2026:

I think it makes sense to keep the check here, but it’s a good point that it should come 3 lines earlier. Also, the block check in the first if is redundant (and it guards only the first if anyways). Changed accordingly.

in src/rpc/blockchain.cpp:1835 in f6ae31af67

1831@@ -1831,7 +1832,8 @@ static RPCHelpMan getchaintxstats()
1832         if (!pindex) {
1833             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");
1834         }
1835-        if (!chainman.ActiveChain().Contains(pindex)) {
1836+        CHECK_NONFATAL(pindex);

hodlinator commented at 9:42 pm on March 26, 2026:

We just threw right above on null.

Same on L2774.

optout21 commented at 11:52 am on March 27, 2026:

Done.

in src/test/chain_tests.cpp:46 in f6ae31af67

40@@ -41,6 +41,160 @@ const CBlockIndex* NaiveLastCommonAncestor(const CBlockIndex* a, const CBlockInd
41 
42 } // namespace
43 
44+BOOST_AUTO_TEST_CASE(cchain_basic_tests)
45+{
46+    auto genesis = CBlockIndex{};

hodlinator commented at 9:56 pm on March 26, 2026:

nanonit: Less tokens, here and below, also for CChain-instances:

0    CBlockIndex genesis{};

optout21 commented at 11:53 am on March 27, 2026:

Done.

in src/test/chain_tests.cpp:105 in f6ae31af67

100+BOOST_AUTO_TEST_CASE(cchain_findfork_tests)
101+{
102+    // Create a forking chain
103+    // Common section
104+    std::vector<std::unique_ptr<CBlockIndex>> b_common;
105+    for(auto i{0}; i < 10; ++i) {

hodlinator commented at 9:59 pm on March 26, 2026:

nit: Here and below:

0    for (auto i{0}; i < 10; ++i) {

in src/validation.cpp:3572 in f6ae31af67

3568@@ -3569,7 +3569,8 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
3569         // Lock for as long as disconnectpool is in scope to make sure MaybeUpdateMempoolForReorg is
3570         // called after DisconnectTip without unlocking in between
3571         LOCK(MempoolMutex());
3572-        if (!m_chain.Contains(pindex)) break;
3573+        Assume(pindex); // checked above

hodlinator commented at 10:09 pm on March 26, 2026:

Would skip this, we do not re-assign it within the loop.

You could actually make it unable to be re-assigned by making the pointer const:

0-bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pindex)
1+bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* const pindex)

optout21 commented at 11:53 am on March 27, 2026:

Done.

in src/validation.cpp:3645 in f6ae31af67 outdated

3641@@ -3641,7 +3642,7 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
3642 
3643     {
3644         LOCK(cs_main);
3645-        if (m_chain.Contains(to_mark_failed)) {
3646+        if (to_mark_failed && m_chain.Contains(*to_mark_failed)) {

hodlinator commented at 10:13 pm on March 26, 2026:

nit: to_mark_failed is always set, could nail things down a bit through:

0-        CBlockIndex* disconnected_tip{m_chain.Tip()};
1+        CBlockIndex* const disconnected_tip{m_chain.Tip()};

optout21 commented at 1:04 pm on March 27, 2026:

Done.

in src/validation.cpp:5397 in f6ae31af67 outdated

5391@@ -5390,7 +5392,9 @@ void ChainstateManager::CheckBlockIndex() const
5392             pindex = range.first->second;
5393             nHeight++;
5394             continue;
5395-        } else if (best_hdr_chain.Contains(pindex)) {
5396+        }
5397+        Assume(pindex); // checked in loop condition
5398+        if (best_hdr_chain.Contains(*pindex)) {

hodlinator commented at 10:18 pm on March 26, 2026:

nit: We’ve just dereferenced it in the prior if-block around L5362:

0        } else if (best_hdr_chain.Contains(*pindex)) {

optout21 commented at 11:09 am on March 27, 2026:

Hmm, but pindex can change in L5391 as well.

in src/rpc/blockchain.cpp:1 in f6ae31af67 outdated

hodlinator commented at 10:20 pm on March 26, 2026:

The lint fix commit in f6ae31af67af20a016c8b25ba56a28016076b870 should be squashed into the commit(s) which introduce the Assume()s.

optout21 commented at 11:54 am on March 27, 2026:

Done.

hodlinator commented at 10:24 pm on March 26, 2026: contributor

Concept ACK f6ae31af67af20a016c8b25ba56a28016076b870

Making variables explicitly non-null when possible is great.

test: Add CChain basic tests

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

dfbc53ae8d

test: Add CChain::FindFork() tests

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

53dee35c65

optout21 force-pushed on Mar 27, 2026

DrahtBot added the label CI failed on Mar 27, 2026

optout21 commented at 11:51 am on March 27, 2026: contributor

Pushed changes, following review:

Simplify added CChain tests, as done in #34416
Squash fixes (header fix & lint fix commits) to where they were introduced (review comment)
Removed added extra non-null checks in cases where the same check was done a few lines before (review comment) Change pindexMostWork parameter of ActivateBestChainStep() to reference (review comment)
Various nits (review comment)

DrahtBot removed the label CI failed on Mar 27, 2026

optout21 force-pushed on Mar 27, 2026

DrahtBot added the label CI failed on Mar 27, 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.

c981581b78

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.

8b919c2f82

Change CChain::FindFork() to take ref ec3dced826

optout21 force-pushed on Mar 27, 2026

Change pindexMostWork parameter of ActivateBestChainStep() to reference

ActivateBestChainStep() is always called with non-nullptr pindexMostWork parameter,
change the type of the parameter from pointer to reference to enforce this.

a3dc65e030

optout21 force-pushed on Mar 27, 2026

optout21 marked this as ready for review on Mar 27, 2026

optout21 commented at 2:40 pm on March 27, 2026: contributor

Review comments addressed (thanks, @hodlinator !), description updated, moved out of Draft.

Refactor CChain methods to use references, tests #34440

Reviews

Conflicts

LLM Linter (✨ experimental)