refactor: Change CChain methods to use references, add tests #34440

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()
Change CChain::Contains() to take reference
Change CChain::Next() to take reference
Change CChain::FindFork() to take reference
Change pindexMostWork parameter of ActivateBestChainStep() to reference
Rename changed parameters (* pindex --> & index)

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 InvalidateBlock() and PreciousBlock() to take references.
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
ACK	hodlinator, l0rinc, maflcko

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:

#34416 (Add nullptr-check to CChain::Contains(), tests by optout21)
#33752 (rest: Query predecessor headers using negative count param by A-Manning)
#29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)
#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 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. <sub>Task lint: https://github.com/bitcoin/bitcoin/actions/runs/21472539937/job/61848421163</sub> <sub>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.</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 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

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

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

Done.

l0rinc commented at 12:38 PM on April 12, 2026:

f389edc Change CChain::FindFork() to take ref:

As mentioned in https://github.com/bitcoin/bitcoin/pull/34440/changes/f389edcbc527d5431569f5b903017401f2ac3c2d#r3053795887 (at the wrong line), I would prefer avoiding adding and removing the checks here. But it's not a blocker, the end result looks fine.

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.

    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.

hodlinator commented at 7:25 PM on March 30, 2026:

Good, that seems more correct than breaking, but I think it would still be prudent to leave a warning at the minimum:

                if (!pindex) {
                    LogWarning("Failed to find block manager entry for best block %s from %s", summary.best_block_hash.ToString(), summary.name);
                } else if (!index_chain.Contains(*pindex)) {
                    pindex = index_chain.FindFork(*pindex);
                }

hodlinator commented at 11:12 AM on March 31, 2026:

nanonit: Would drop the new empty line:

                 pindex = chainman.m_blockman.LookupBlockIndex(summary.best_block_hash);
-
                 if (!pindex) {

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.

        const int branchLen = block->nHeight - active_chain.FindFork(*block)->nHeight;
obj.pushKV("branchlen", branchLen);

std::string status;
        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:

    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:

    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:

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

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

Done.

hodlinator commented at 7:31 PM on March 30, 2026:

Thanks! It seems fairly trivial to make both InvalidateBlock() and PreciousBlock() take references, but might be better to keep for a follow-up PR.

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:

-        CBlockIndex* disconnected_tip{m_chain.Tip()};
+        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:

        } 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.

hodlinator commented at 7:35 PM on March 30, 2026:

Yes, but after reassigning pindex there we either break out or continue with the while condition checking against nullptr.

optout21 commented at 2:54 PM on April 8, 2026:

Changed.

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 7:59 PM on March 30, 2026:

nanonit: Could be extra paranoid and const the pointer:

-static const CBlockIndex* NextSyncBlock(const CBlockIndex* pindex_prev, CChain& chain) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
+static const CBlockIndex* NextSyncBlock(const CBlockIndex* const pindex_prev, CChain& chain) EXCLUSIVE_LOCKS_REQUIRED(cs_main)

hodlinator commented at 11:17 AM on March 31, 2026:

nit in 8a1a573d6ca67839b952916e755739913072f8c6:

Is there any reason the other commit messages call it "Change" whereas this one calls it "Rework"?

hodlinator commented at 11:33 AM on March 31, 2026:

nanonit in cfe9e1dc8c5a4e2e1e928abada5e2178ca55a37e:

Could expand on commit message to state:

The internal null-guard in FindFork() was removed in favor of adding any missing guards at call sites.

It's somewhat redundant, but feels more complete to spell it out.

optout21 commented at 12:36 PM on April 8, 2026:

No particular reason, changed to be consistent.

optout21 commented at 12:38 PM on April 8, 2026:

Done.

l0rinc commented at 7:37 PM on April 8, 2026:

9f65172 Change CChain::Contains() to take reference:

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.

I couldn't find that in the commit anymore, looks like that part of the message is stale (and formatting is incomplete).

optout21 commented at 9:55 AM on April 10, 2026:

True, there are not many extra checks left, there is no Assume. There is one extra check. Updating the commit message. (https://github.com/bitcoin/bitcoin/pull/34440/changes/9f65172405bb0e3438b5742a92db9a2ebab540af#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3R2864)

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

Concept ACK f6ae31af67af20a016c8b25ba56a28016076b870

Making variables explicitly non-null when possible is great.

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

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.

DrahtBot removed the label CI failed on Mar 30, 2026

DrahtBot added the label Needs rebase on Mar 30, 2026

optout21 force-pushed on Mar 30, 2026

optout21 commented at 1:27 PM on March 30, 2026: contributor

Rebased, post-#32875.

DrahtBot removed the label Needs rebase on Mar 30, 2026

in src/test/chain_tests.cpp:72 in c6c1e070db

  67 | +    BOOST_CHECK_EQUAL(chain_2[2], nullptr);
  68 | +
  69 | +    // Contains: call with contained & non-contained blocks, and with nullptr
  70 | +    BOOST_CHECK_EQUAL(chain_2.Contains(genesis), true);
  71 | +    BOOST_CHECK_EQUAL(chain_2.Contains(bi1), true);
  72 | +    BOOST_CHECK_EQUAL(chain_0.Contains(bi1), false);

hodlinator commented at 6:53 PM on March 30, 2026:

nit: BOOST_CHECK() appears clearer for booleans. The first failure here is BOOST_CHECK():

error: in "chain_tests/cchain_basic_tests": check chain_0.Contains(bi1) has failed
error: in "chain_tests/cchain_basic_tests": check chain_0.Contains(bi1) == true has failed [false != true]

optout21 commented at 9:48 AM on March 31, 2026:

Done

in src/test/chain_tests.cpp:74 in c6c1e070db

  69 | +    // Contains: call with contained & non-contained blocks, and with nullptr
  70 | +    BOOST_CHECK_EQUAL(chain_2.Contains(genesis), true);
  71 | +    BOOST_CHECK_EQUAL(chain_2.Contains(bi1), true);
  72 | +    BOOST_CHECK_EQUAL(chain_0.Contains(bi1), false);
  73 | +
  74 | +    // Call with non-tip & tip blocks, and with nulltr

hodlinator commented at 6:58 PM on March 30, 2026:

nit: Should end up with just this ("nulltr" typo).

    // Call with non-tip & tip blocks

in src/test/chain_tests.cpp:76 in c6c1e070db outdated

  71 | +    BOOST_CHECK_EQUAL(chain_2.Contains(bi1), true);
  72 | +    BOOST_CHECK_EQUAL(chain_0.Contains(bi1), false);
  73 | +
  74 | +    // Call with non-tip & tip blocks, and with nulltr
  75 | +    BOOST_CHECK_EQUAL(chain_2.Next(genesis), &bi1);
  76 | +    BOOST_CHECK_EQUAL(chain_2.Next(bi1), nullptr);

hodlinator commented at 6:59 PM on March 30, 2026:

nit: Could also add:

    BOOST_CHECK_EQUAL(chain_0.Next(genesis), nullptr);

optout21 commented at 9:05 AM on March 31, 2026:

It was there, I've removed following comments about the test being overly verbose. Non-inclusion is tested, and chain without a genesis is not a common case anyways.

hodlinator commented at 11:09 AM on March 31, 2026:

Hm.. okay, maybe testing Next() an extra time is too much. But I think it might be good to show that not even genesis is included in chain_0:

-BOOST_CHECK(!chain_0.Contains(&bi1));
+BOOST_CHECK(!chain_0.Contains(&genesis));

optout21 commented at 12:36 PM on April 8, 2026:

Applied.

in src/test/chain_tests.cpp:149 in c6c1e070db

 144 | +            BOOST_CHECK_EQUAL(fork, b_common.back().get());
 145 | +        }
 146 | +    }
 147 | +    {
 148 | +        // Create a chain with the shorter fork
 149 | +        auto cs = CChain{};

hodlinator commented at 7:03 PM on March 30, 2026:

nits: cs is very associated with Critical Section in the codebase (infamous cs_main). Abbreviating "short chain" might be better, and then adjust to lc above for symmetry:

        CChain sc{};

optout21 commented at 9:52 AM on March 31, 2026:

Done

in src/index/base.cpp:171 in c6c1e070db outdated

 165 | @@ -166,7 +166,9 @@ static const CBlockIndex* NextSyncBlock(const CBlockIndex* pindex_prev, CChain&
 166 |  
 167 |      // Since block is not in the chain, return the next block in the chain AFTER the last common ancestor.
 168 |      // Caller will be responsible for rewinding back to the common ancestor.
 169 | -    return chain.Next(chain.FindFork(pindex_prev));
 170 | +    auto fork = chain.FindFork(*pindex_prev);
 171 | +    if (!fork) return nullptr;
 172 | +    return chain.Next(*fork);

hodlinator commented at 7:53 PM on March 30, 2026:

We have some strict opinions on what qualifies as a refactoring, but I think adding a guard against never observed UB or crash still arguably falls within it: #34918 (review)

I suggest adjusting the PR title to something like "refactor: Make CChain methods use references" as per prefixes in https://github.com/maflcko/DrahtBot/blob/14ccfd94bc7cc584e547e09582373cb87a0e4553/webhook_features/config.yml

optout21 commented at 9:52 AM on March 31, 2026:

Done

l0rinc commented at 11:09 AM on April 6, 2026:

    const auto* fork{chain.FindFork(*pindex_prev)};
    return fork ? chain.Next(*fork) : nullptr;

optout21 commented at 2:36 PM on April 8, 2026:

Applied.

in src/validation.cpp:4726 in c6c1e070db

4721 | @@ -4722,7 +4722,8 @@ VerifyDBResult CVerifyDB::VerifyDB(
4722 |                  reportDone = percentageDone / 10;
4723 |              }
4724 |              m_notifications.progress(_("Verifying blocks…"), percentageDone, false);
4725 | -            pindex = chainstate.m_chain.Next(pindex);
4726 | +            pindex = chainstate.m_chain.Next(*pindex);
4727 | +            Assume(pindex);

hodlinator commented at 8:13 PM on March 30, 2026:

nit: In the beginning of the function, we assert cs_main is taken so the chain is not being messed with externally. We then for-loop from the tip backwards until reaching the depth to check or some other condition, assigning pindex to pindex->pprev. Later, in the while-loop pertaining to these lines, we instead advance but stop looping as soon as we reach the tip. That is why this Assume() has not been necessary, so I think it's fine to skip adding it.

optout21 commented at 9:52 AM on March 31, 2026:

Accepted, done

in src/rpc/blockchain.cpp:3186 in c6c1e070db

3181 | @@ -3182,7 +3182,8 @@ static RPCHelpMan dumptxoutset()
3182 |              disable_network.emplace(connman);
3183 |          }
3184 |  
3185 | -        invalidate_index = WITH_LOCK(::cs_main, return node.chainman->ActiveChain().Next(target_index));
3186 | +        const CBlockIndex* invalidate_index{WITH_LOCK(::cs_main, return node.chainman->ActiveChain().Next(*target_index))};
3187 | +        CHECK_NONFATAL(invalidate_index);

hodlinator commented at 8:27 PM on March 30, 2026:

nit: This is added as an Assume() in c078a9e636f914d845f215d9c716612cf597bbc0 and then changed to CHECK_NONFATAL() in 3a471c21535a3f8c16103f4bcb503f5809121609. Should be CHECK_NONFATAL() from the start.

Although we already checked that we are not at the tip in this if-block, so we can be fairly sure that we have a non-null next entry. Could drop.

optout21 commented at 9:53 AM on March 31, 2026:

Done

hodlinator commented at 8:34 PM on March 30, 2026: contributor

Reviewed c6c1e070dbaf54b70fba9a096cf0b4c9a96a928c

I am confident the changes maintain the current safety properties of the code. FindFork() and ActivateBestChainStep() are the only functions to remove internal null guards, and all cases where external null guards were missing, they have been added.

Any potentially silent merge conflicts I foresee will cause compilation failures through mismatching pointer/reference parameters.

The hit against future git blame sleuthing efforts pays for itself in less surprising code IMO.

optout21 renamed this:
~~Refactor CChain methods to use references, tests~~
refactor: Change CChain methods to use references, add tests
on Mar 31, 2026

DrahtBot added the label Refactoring on Mar 31, 2026

optout21 force-pushed on Mar 31, 2026

optout21 commented at 9:48 AM on March 31, 2026: contributor

Applied changes, mostly removed a few extra checks, and nits, following review comments -- thanks @hodlinator !

optout21 force-pushed on Mar 31, 2026

DrahtBot added the label CI failed on Mar 31, 2026

DrahtBot removed the label CI failed on Mar 31, 2026

in src/chain.h:418 in e289cac727

 416 |  
 417 |      /** Find the successor of a block in this chain, or nullptr if the given index is not found or is the tip. */
 418 |      CBlockIndex* Next(const CBlockIndex* pindex) const
 419 |      {
 420 | -        if (Contains(pindex))
 421 | +        if (pindex && Contains(*pindex))

hodlinator commented at 11:21 AM on March 31, 2026:

e289cac Change CChain::Contains() to take reference:

Would avoid introducing the guard against null pindex here, as on master there is no guard. It makes the following commit removing the guard look more scary than it is.

optout21 commented at 12:37 PM on April 8, 2026:

Applied. Included only a temporary comment.

in src/chain.cpp:50 in 6189b69474

  46 | @@ -47,13 +47,11 @@ CBlockLocator GetLocator(const CBlockIndex* index)
  47 |      return CBlockLocator{LocatorEntries(index)};
  48 |  }
  49 |  
  50 | -const CBlockIndex *CChain::FindFork(const CBlockIndex *pindex) const {
  51 | -    if (pindex == nullptr) {
  52 | -        return nullptr;
  53 | -    }
  54 | +const CBlockIndex *CChain::FindFork(const CBlockIndex &pindex_ref) const {

hodlinator commented at 11:25 AM on March 31, 2026:

nit: Might as well conform to common symbol-placement (matching header) and newlines:

const CBlockIndex* CChain::FindFork(const CBlockIndex& pindex_ref) const
{

optout21 commented at 12:38 PM on April 8, 2026:

Applied.

hodlinator commented at 11:36 AM on March 31, 2026: contributor

Reviewed 6189b69474d598ecd4cea23ab24dbe8fc5e9e64d

Thanks for incorporating my feedback so far, hopefully these are the last nits I can find before A-C-K.

in src/init.cpp:2352 in 6189b69474 outdated

2346 | @@ -2347,8 +2347,11 @@ 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 | +
2353 | +                if (!pindex) {
2354 | +                    LogWarning("Failed to find block manager entry for best block %s from %s", summary.best_block_hash.ToString(), summary.name);

l0rinc commented at 11:12 AM on April 6, 2026:

Should we maybe mention that we're falling back to genesis?

                    LogWarning("Failed to find block manager entry for best block %s from %s, falling back to genesis for index sync",
                               summary.best_block_hash.ToString(), summary.name);

optout21 commented at 2:37 PM on April 8, 2026:

Applied.

in src/kernel/bitcoinkernel.cpp:1347 in 6189b69474 outdated

1343 | @@ -1344,7 +1344,7 @@ const btck_BlockTreeEntry* btck_chain_get_by_height(const btck_Chain* chain, int
1344 |  int btck_chain_contains(const btck_Chain* chain, const btck_BlockTreeEntry* entry)
1345 |  {
1346 |      LOCK(::cs_main);
1347 | -    return btck_Chain::get(chain).Contains(&btck_BlockTreeEntry::get(entry)) ? 1 : 0;
1348 | +    return btck_Chain::get(chain).Contains(btck_BlockTreeEntry::get(entry)) ? 1 : 0;

l0rinc commented at 11:13 AM on April 6, 2026:

nit: It seems to me const btck_BlockTreeEntry* entry cannot be nullptr here, but we can't migrate to reference because of the bitcoinkernel C API, so maybe we can document the invariant via something like Assert(chain && entry);. Just resolve if you think it's unrelated.

in src/validation.cpp:5398 in 6189b69474 outdated

5393 | @@ -5394,7 +5394,8 @@ void ChainstateManager::CheckBlockIndex() const
5394 |              pindex = range.first->second;
5395 |              nHeight++;
5396 |              continue;
5397 | -        } else if (best_hdr_chain.Contains(pindex)) {
5398 | +        }
5399 | +        if (best_hdr_chain.Contains(*pindex)) {

l0rinc commented at 11:19 AM on April 6, 2026:

seems to me this can stay an else if to minimize diff and to signal that they're mutually exclusive without having to check the implementation details

optout21 commented at 2:39 PM on April 8, 2026:

My subjective preference is to not use else when there is a return, continue or break control statement in the if branch, but I've taken your suggestion and reverted the change in this case, to minimize the diff.

in src/test/chain_tests.cpp:72 in 6189b69474

  67 | +    BOOST_CHECK_EQUAL(chain_2[2], nullptr);
  68 | +
  69 | +    // Contains: call with contained & non-contained blocks
  70 | +    BOOST_CHECK(chain_2.Contains(genesis));
  71 | +    BOOST_CHECK(chain_2.Contains(bi1));
  72 | +    BOOST_CHECK(!chain_0.Contains(bi1));

l0rinc commented at 11:40 AM on April 6, 2026:

what about genesis in chain_0? Seems that's a stronger claim

    BOOST_CHECK(!chain_0.Contains(genesis));

optout21 commented at 1:31 PM on April 8, 2026:

This exact remark was done by @hodlinator as well, already done :)

in src/test/chain_tests.cpp:67 in 6189b69474 outdated

  62 | +
  63 | +    // Indexer accessor: call with valid and invalid (low & high) values
  64 | +    BOOST_CHECK_EQUAL(chain_2[0], &genesis);
  65 | +    BOOST_CHECK_EQUAL(chain_2[1], &bi1);
  66 | +    BOOST_CHECK_EQUAL(chain_2[-1], nullptr);
  67 | +    BOOST_CHECK_EQUAL(chain_2[2], nullptr);

l0rinc commented at 11:45 AM on April 6, 2026:

This is weird, but seems nHeight < 0 is checked, we might as well exercise it 👍

Can we make the boundaries ridiculously high to make it obvious why they're nullptr?

    BOOST_CHECK_EQUAL(chain_2[-100], nullptr);
    BOOST_CHECK_EQUAL(chain_2[0], &genesis);
    BOOST_CHECK_EQUAL(chain_2[1], &bi1);
    BOOST_CHECK_EQUAL(chain_2[100], nullptr);

optout21 commented at 1:32 PM on April 8, 2026:

The strongest claim is using the values closest to the limit, I think. For making it clear, there is the comment (above, "invalid (low & high) values"). Leaving as is.

in src/test/chain_tests.cpp:55 in 6189b69474 outdated

  50 | +    CBlockIndex genesis{};
  51 | +    genesis.nHeight = 0;
  52 | +    chain_2.SetTip(genesis);
  53 | +    CBlockIndex bi1{};
  54 | +    bi1.nHeight = 1;
  55 | +    chain_2.SetTip(bi1);

l0rinc commented at 11:54 AM on April 6, 2026:

We should probably set pprev to genesis for this to be a proper 2 block chain (not sure why SetTip was used instead, please corret me if I'm wrong):

    // An empty chain
    const CChain chain_0;
    // A chain with 2 blocks, built from the tip
    CChain chain_2;

    CBlockIndex genesis;
    genesis.nHeight = 0;

    CBlockIndex bi1;
    bi1.pprev = &genesis;
    bi1.nHeight = 1;
    chain_2.SetTip(bi1);

[!NOTE] Plain default construction is the more common and less noisy style in this codebase, so CChain chain_2; may be simpler

optout21 commented at 2:41 PM on April 8, 2026:

I've added setting the pprev as suggested, for correctness (though it is not strictly required for these tests). Also, removed the {} empty constructor initializers (they are unnecessary and ugly).

in src/chain.cpp:51 in 6189b69474 outdated

  46 | @@ -47,13 +47,11 @@ CBlockLocator GetLocator(const CBlockIndex* index)
  47 |      return CBlockLocator{LocatorEntries(index)};
  48 |  }
  49 |  
  50 | -const CBlockIndex *CChain::FindFork(const CBlockIndex *pindex) const {
  51 | -    if (pindex == nullptr) {
  52 | -        return nullptr;
  53 | -    }
  54 | +const CBlockIndex *CChain::FindFork(const CBlockIndex &pindex_ref) const {
  55 | +    auto pindex{&pindex_ref};

l0rinc commented at 12:22 PM on April 6, 2026:

auto hides that this is intentionally a nullable local traversal pointer derived from the non-null reference parameter.

    const CBlockIndex* pindex{&pindex_ref};

optout21 commented at 2:41 PM on April 8, 2026:

Changed to const auto*.

in src/test/chain_tests.cpp:91 in 6189b69474 outdated

  86 | +        if (i > 0) {
  87 | +            b->pprev = blocks_common[i - 1].get();
  88 | +        }
  89 | +        b->nHeight = i;
  90 | +        blocks_common.push_back(std::move(b));
  91 | +    }

l0rinc commented at 12:45 PM on April 6, 2026:

We're doing the same construction multiple times, could we extract them to local lambdas to make the logic clearer? Also, the unique pointers can probably be simplified via array init instead.

BOOST_AUTO_TEST_CASE(cchain_findfork_tests)
{
    const auto init_branch{[](auto& blocks, CBlockIndex* parent, int start_height) {
        for (size_t i{0}; i < blocks.size(); ++i) {
            blocks[i].pprev = i == 0 ? parent : &blocks[i - 1];
            blocks[i].nHeight = start_height + i;
        }
    }};

    const auto check_same{[](const CChain& chain, const auto& blocks) {
        for (const auto& block : blocks) {
            BOOST_CHECK_EQUAL(chain.FindFork(block), &block);
        }
    }};

    const auto check_fork_point{[](const CChain& chain, const auto& blocks, const CBlockIndex* fork_point) {
        for (const auto& block : blocks) {
            BOOST_CHECK_EQUAL(chain.FindFork(block), fork_point);
        }
    }};

    std::array<CBlockIndex, 10> blocks_common{};
    init_branch(blocks_common, nullptr, 0);

    std::array<CBlockIndex, 10> blocks_long{};
    init_branch(blocks_long, &blocks_common.back(), blocks_common.size());

    std::array<CBlockIndex, 5> blocks_short{};
    init_branch(blocks_short, &blocks_common.back(), blocks_common.size());

    CChain chain_long;
    chain_long.SetTip(blocks_long.back());
    BOOST_CHECK_EQUAL(chain_long.Height(), 10 + 10 - 1);
    check_same(chain_long, blocks_common);
    check_same(chain_long, blocks_long);
    check_fork_point(chain_long, blocks_short, &blocks_common.back());

    CChain chain_short;
    chain_short.SetTip(blocks_short.back());
    BOOST_CHECK_EQUAL(chain_short.Height(), 10 + 5 - 1);
    check_same(chain_short, blocks_common);
    check_same(chain_short, blocks_short);
    check_fork_point(chain_short, blocks_long, &blocks_common.back());
}

optout21 commented at 2:42 PM on April 8, 2026:

Thanks for the more succinct and aesthetic version, applied.

hodlinator commented at 12:26 PM on April 10, 2026:

I find the code somewhat harder to follow, but looks neat.

nit: Feels on topic, unless you want to leave open the unused ability to send in nullptr:

-    const auto check_fork_point{[](const CChain& chain, const auto& blocks, const CBlockIndex* fork_point) {
+    const auto check_fork_point{[](const CChain& chain, const auto& blocks, const CBlockIndex& fork_point) {
        for (const auto& block : blocks) {
-            BOOST_CHECK_EQUAL(chain.FindFork(block), fork_point);
+            BOOST_CHECK_EQUAL(chain.FindFork(block), &fork_point);

in src/test/chain_tests.cpp:73 in e289cac727

  65 | @@ -66,11 +66,10 @@ BOOST_AUTO_TEST_CASE(cchain_basic_tests)
  66 |      BOOST_CHECK_EQUAL(chain_2[-1], nullptr);
  67 |      BOOST_CHECK_EQUAL(chain_2[2], nullptr);
  68 |  
  69 | -    // Contains: call with contained & non-contained blocks, and with nullptr
  70 | -    BOOST_CHECK(chain_2.Contains(&genesis));
  71 | -    BOOST_CHECK(chain_2.Contains(&bi1));
  72 | -    BOOST_CHECK(!chain_0.Contains(&bi1));
  73 | -    // BOOST_CHECK(!chain_0.Contains(nullptr)); // fail with memory access violation

l0rinc commented at 12:49 PM on April 6, 2026:

e289cac Change CChain::Contains() to take reference:

+1 for adding these demonstrations early in the commits!

in src/rpc/blockchain.cpp:1647 in e289cac727

1643 | @@ -1644,7 +1644,7 @@ static RPCHelpMan getchaintips()
1644 |          obj.pushKV("branchlen", branchLen);
1645 |  
1646 |          std::string status;
1647 | -        if (active_chain.Contains(block)) {
1648 | +        if (block && active_chain.Contains(*block)) {

l0rinc commented at 12:50 PM on April 6, 2026:

e289cac Change CChain::Contains() to take reference:

This is a theoretical behavior change from UB on nullptr, right?

If block can be null here, then it seems we would also need to guard the earlier dereferences (block->nHeight, block->phashBlock, FindFork(*block)) instead of only this condition.

Otherwise, if block is not expected to be null, it seems clearer to assert that once and keep the rest of the loop non-null.

optout21 commented at 1:35 PM on April 8, 2026:

Not relevant any more. This guard was added, then removed, but not from both commits. Not present any more.

in src/validation.cpp:3578 in e289cac727 outdated

3572 | @@ -3573,9 +3573,9 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
3573 |          // Lock for as long as disconnectpool is in scope to make sure MaybeUpdateMempoolForReorg is
3574 |          // called after DisconnectTip without unlocking in between
3575 |          LOCK(MempoolMutex());
3576 | -        if (!m_chain.Contains(pindex)) break;
3577 | +        if (!m_chain.Contains(*pindex)) break;
3578 |          pindex_was_in_chain = true;
3579 | -        CBlockIndex* disconnected_tip{m_chain.Tip()};
3580 | +        CBlockIndex* const disconnected_tip{m_chain.Tip()};

l0rinc commented at 12:54 PM on April 6, 2026:

e289cac Change CChain::Contains() to take reference:

nit: these new * const additions seem a bit excessive to me. Just resolve if you disagree.

optout21 commented at 1:41 PM on April 8, 2026:

@hodlinator had some remarks regarding adding const's, I've added it as a result. Leaving as-is, even though remarks was not specifically about this line (https://github.com/bitcoin/bitcoin/pull/34440#discussion_r2997918557)

hodlinator commented at 12:31 PM on April 10, 2026:

Yeah, I'm thinking making the pointers const adds a grain of protection against silent merge conflict issues. Here's the comment for this specific one: #34440 (review)

in src/test/chain_tests.cpp:77 in 8a1a573d6c

  70 | @@ -71,10 +71,9 @@ BOOST_AUTO_TEST_CASE(cchain_basic_tests)
  71 |      BOOST_CHECK(chain_2.Contains(bi1));
  72 |      BOOST_CHECK(!chain_0.Contains(bi1));
  73 |  
  74 | -    // Call with non-tip & tip blocks, and with nullptr
  75 | -    BOOST_CHECK_EQUAL(chain_2.Next(&genesis), &bi1);
  76 | -    BOOST_CHECK_EQUAL(chain_2.Next(&bi1), nullptr);
  77 | -    // BOOST_CHECK_EQUAL(chain_0.Next(nullptr), nullptr); // fail with memory access violation

l0rinc commented at 12:57 PM on April 6, 2026:

8a1a573 Rework CChain::Next() to take reference:

Could we enable this test now?

BOOST_CHECK_EQUAL(chain_0.Next(genesis), nullptr);

optout21 commented at 2:43 PM on April 8, 2026:

It's not the same test, but it makes sense, added.

in src/chain.h:443 in cfe9e1dc8c outdated

 439 | @@ -440,7 +440,7 @@ class CChain
 440 |      void SetTip(CBlockIndex& block);
 441 |  
 442 |      /** Find the last common block between this chain and a block index entry. */
 443 | -    const CBlockIndex* FindFork(const CBlockIndex* pindex) const;
 444 | +    const CBlockIndex* FindFork(const CBlockIndex& pindex) const;

l0rinc commented at 1:04 PM on April 6, 2026:

cfe9e1d Change CChain::FindFork() to take ref:

To match the cpp name:

    const CBlockIndex* FindFork(const CBlockIndex& pindex_ref) const;

optout21 commented at 2:44 PM on April 8, 2026:

Suggestion applied. Nicer would be to keep the original pindex parameter name, and internally use a pindex_ptr helper variable, but that would result in larger diff (rename), so I stayed with the pindex_ref/pindex version.

l0rinc changes_requested

l0rinc commented at 1:10 PM on April 6, 2026: contributor

Concept ACK

Left a few comments, I think the new basic CChain test can better reflect the intended linked-chain behavior and make the empty-chain expectations slightly stronger. The startup warning in init.cpp could be a bit more informative about the fallback behavior. I also think a couple of the refactoring changes can stay closer to the original structure for readability and to keep the diff smaller.

optout21 force-pushed on Apr 8, 2026

DrahtBot added the label CI failed on Apr 8, 2026

optout21 force-pushed on Apr 8, 2026

optout21 commented at 12:40 PM on April 8, 2026: contributor

@hodlinator , thanks for the review. Suggested changes applied (minor changes).

DrahtBot removed the label CI failed on Apr 8, 2026

optout21 force-pushed on Apr 8, 2026

optout21 commented at 2:45 PM on April 8, 2026: contributor

@l0rinc , thanks for the review! Suggested changes applied (mostly minor changes).

in src/test/chain_tests.cpp:70 in 5884af4eb5 outdated

  65 | +
  66 | +    // Indexer accessor: call with valid and invalid (low & high) values
  67 | +    BOOST_CHECK_EQUAL(chain_2[0], &genesis);
  68 | +    BOOST_CHECK_EQUAL(chain_2[1], &bi1);
  69 | +    BOOST_CHECK_EQUAL(chain_2[-1], nullptr);
  70 | +    BOOST_CHECK_EQUAL(chain_2[2], nullptr);

l0rinc commented at 7:31 PM on April 8, 2026:

5884af4 test: Add CChain basic tests:

nit: could we sort these so that -1goes before 0?

    BOOST_CHECK_EQUAL(chain_2[-1], nullptr);
    BOOST_CHECK_EQUAL(chain_2[0], &genesis);
    BOOST_CHECK_EQUAL(chain_2[1], &bi1);
    BOOST_CHECK_EQUAL(chain_2[2], nullptr);

in src/test/chain_tests.cpp:63 in 5884af4eb5 outdated

  58 | +    chain_2.SetTip(bi1);
  59 | +
  60 | +    BOOST_CHECK_EQUAL(chain_0.Height(), -1);
  61 | +    BOOST_CHECK_EQUAL(chain_2.Height(), 1);
  62 | +
  63 | +    BOOST_CHECK_EQUAL(chain_0.Tip(), nullptr);

l0rinc commented at 7:32 PM on April 8, 2026:

5884af4 test: Add CChain basic tests:

Can we also add a .Genesis() check for both chains?

optout21 commented at 10:21 AM on April 10, 2026:

Done.

in src/test/chain_tests.cpp:107 in 6828e7e016 outdated

 102 | +        for (const auto& block : blocks) {
 103 | +            BOOST_CHECK_EQUAL(chain.FindFork(&block), fork_point);
 104 | +        }
 105 | +    }};
 106 | +
 107 | +    std::array<CBlockIndex, 10> blocks_common;

l0rinc commented at 7:33 PM on April 8, 2026:

6828e7e test: Add CChain::FindFork() tests:

nit: could you please add a #include <array> as well?

optout21 commented at 10:21 AM on April 10, 2026:

Done.

in src/index/base.cpp:170 in 1d163a09f0 outdated

 165 | @@ -166,7 +166,8 @@ static const CBlockIndex* NextSyncBlock(const CBlockIndex* pindex_prev, CChain&
 166 |  
 167 |      // Since block is not in the chain, return the next block in the chain AFTER the last common ancestor.
 168 |      // Caller will be responsible for rewinding back to the common ancestor.
 169 | -    return chain.Next(chain.FindFork(pindex_prev));
 170 | +    const auto fork = chain.FindFork(pindex_prev);
 171 | +    return fork ? chain.Next(*fork) : nullptr;

l0rinc commented at 7:41 PM on April 8, 2026:

1d163a0 Change CChain::Next() to take reference:

Can we mention on the commit message that NextSyncBlock() now checks the FindFork() result before asking for its successor because the fork lookup may still return null.

optout21 commented at 10:21 AM on April 10, 2026:

Done.

hodlinator commented at 12:44 PM on April 10, 2026:

It's very unlikely that the fork lookup would fail. We would have a crash on master, right?

nit: Could add an assert:

    const auto* fork{chain.FindFork(*pindex_prev)};
    // We should always find a common fork point if we start from the same genesis.
    return Assert(fork) ? chain.Next(*fork) : nullptr;

optout21 commented at 8:29 AM on April 15, 2026:

Update: changed to an Assume and no conditional.

in src/test/chain_tests.cpp:72 in 5884af4eb5

  67 | +    BOOST_CHECK_EQUAL(chain_2[0], &genesis);
  68 | +    BOOST_CHECK_EQUAL(chain_2[1], &bi1);
  69 | +    BOOST_CHECK_EQUAL(chain_2[-1], nullptr);
  70 | +    BOOST_CHECK_EQUAL(chain_2[2], nullptr);
  71 | +
  72 | +    // Contains: call with contained & non-contained blocks, and with nullptr

l0rinc commented at 7:43 PM on April 8, 2026:

5884af4 test: Add CChain basic tests:

    // Contains: call with contained & non-contained blocks

in src/test/chain_tests.cpp:78 in 5884af4eb5

  73 | +    BOOST_CHECK(chain_2.Contains(&genesis));
  74 | +    BOOST_CHECK(chain_2.Contains(&bi1));
  75 | +    BOOST_CHECK(!chain_0.Contains(&genesis));
  76 | +    // BOOST_CHECK(!chain_0.Contains(nullptr)); // fail with memory access violation
  77 | +
  78 | +    // Call with non-tip & tip blocks, and with nullptr

l0rinc commented at 7:44 PM on April 8, 2026:

5884af4 test: Add CChain basic tests:

    // Call with non-tip & tip blocks

in src/validation.cpp:3411 in b7d1919a0d outdated

3407 | @@ -3408,7 +3408,7 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr<
3408 |              } while (!m_chain.Tip() || (starting_tip && CBlockIndexWorkComparator()(m_chain.Tip(), starting_tip)));
3409 |              if (!blocks_connected) return true;
3410 |  
3411 | -            const CBlockIndex* pindexFork = m_chain.FindFork(starting_tip);
3412 | +            const CBlockIndex* pindexFork = starting_tip ? m_chain.FindFork(*starting_tip) : nullptr;

l0rinc commented at 7:48 PM on April 8, 2026:

b7d1919 Change CChain::FindFork() to take ref:

Since we're changing this in the next commit anyway, we could avoid changing it back and forth by doing something like:

            CHECK_NONFATAL(pindexMostWork);
            const CBlockIndex* pindexFork = m_chain.FindFork(*pindexMostWork);

hodlinator commented at 12:58 PM on April 10, 2026:

Suggestion seems to be made at the wrong instance of pindexFork. Think it's meant for L~3190.

I'm slightly on the fence about ostensibly adding a case of exceptions being thrown, but would be okay with the change.

My thinking here is there's a non-zero risk that another reviewer wants to exclude the last commit, "Change pindexMostWork parameter of ActivateBestChainStep() to reference", from this PR as it is slightly tangential.

l0rinc commented at 11:51 AM on April 12, 2026:

Suggestion seems to be made at the wrong instance of pindexFork

Indeed

another reviewer wants to exclude the last commit

I think we could cross that bridge when we see the last commit being reverted - but it also seems fine to me as it is now, just that there's extra churn by adding and removing the condition.

in src/index/base.cpp:169 in b7d1919a0d

 165 | @@ -166,7 +166,7 @@ static const CBlockIndex* NextSyncBlock(const CBlockIndex* const pindex_prev, CC
 166 |  
 167 |      // Since block is not in the chain, return the next block in the chain AFTER the last common ancestor.
 168 |      // Caller will be responsible for rewinding back to the common ancestor.
 169 | -    const auto fork = chain.FindFork(pindex_prev);
 170 | +    const auto fork = chain.FindFork(*pindex_prev);

l0rinc commented at 7:55 PM on April 8, 2026:

b7d1919 Change CChain::FindFork() to take ref:

To highlight that it's a pointer, we could also do:

    const auto* fork{chain.FindFork(*pindex_prev)};

optout21 commented at 10:22 AM on April 10, 2026:

Done.

in src/chain.cpp:56 in 9f65172405 outdated

  52 | @@ -53,7 +53,7 @@ const CBlockIndex *CChain::FindFork(const CBlockIndex *pindex) const {
  53 |      }
  54 |      if (pindex->nHeight > Height())
  55 |          pindex = pindex->GetAncestor(Height());
  56 | -    while (pindex && !Contains(pindex))
  57 | +    while (pindex && !Contains(*pindex))

l0rinc commented at 7:57 PM on April 8, 2026:

9f65172 Change CChain::Contains() to take reference:

nit: if we're already modifying this method we could add braces to the if/while - which would allow you to do the renames you mentioned, too

optout21 commented at 10:23 AM on April 10, 2026:

Leaving as-is, keeping diff minimal.

l0rinc commented at 10:43 AM on April 10, 2026:

That's also fine, but usually if we're already touching a line, we standardize it to leave the campground cleaner than we found it.

l0rinc commented at 8:10 PM on April 8, 2026: contributor

Code review ACK 2f3c974f951a5c5868a3fe203f43b58d60a3db5f

The only remaining nits I had were to add the missing #include <array>, tighten a couple of commit messages so they match the final diffs, prefer const auto* fork{...} in the FindFork() caller to make the pointer-ness obvious, and consider keeping the stronger non-null handling for pindexMostWork visible in the FindFork() commit instead of temporarily making that path nullable.

DrahtBot requested review from hodlinator on Apr 8, 2026

DrahtBot added the label Needs rebase on Apr 9, 2026

optout21 force-pushed on Apr 10, 2026

optout21 commented at 10:21 AM on April 10, 2026: contributor

New minor changes:

Rebased, post #34884 (one conflict, straighforward to resolve)
Some minor chages following new review -- thanks @l0rinc!

DrahtBot removed the label Needs rebase on Apr 10, 2026

hodlinator approved

hodlinator commented at 1:27 PM on April 10, 2026: contributor

ACK f570a194a6aa600740a5742c362aed2d5d118a95

Reasoning: #34440#pullrequestreview-4032653526

DrahtBot requested review from l0rinc on Apr 10, 2026

optout21 force-pushed on Apr 11, 2026

optout21 commented at 5:40 AM on April 11, 2026: contributor

Minor changes applied from new review comments -- @hodlinator & @l0rinc thanks for the detailed remarks.

DrahtBot added the label CI failed on Apr 11, 2026

optout21 force-pushed on Apr 11, 2026

optout21 commented at 11:47 AM on April 11, 2026: contributor

CI/iwyu check failed, due to missing check.h include due to added Assert. Updated.

optout21 force-pushed on Apr 11, 2026

DrahtBot removed the label CI failed on Apr 11, 2026

in src/net_processing.cpp:2865 in 3ebae58171

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

l0rinc commented at 12:29 PM on April 12, 2026:

3ebae58 Change CChain::Contains() to take reference:

Wouldn't !pindexWalk mean that the two chains don't even have the genesis as a common ancestor? Wouldn't if (!m_chainman.ActiveChain().Contains(*Assert(pindexWalk))) { make more sense in HeadersDirectFetchBlocks? Not sure it matters...

optout21 commented at 8:28 AM on April 15, 2026:

Removed the extra if clause and added an Assume. This can happen only if the genesis doesn't match, which is extremely unlikely (and would have resulted in a crash before as well). In release mode there is zero overhead (but it will crash in the unlikely case, unchanged behavior). In debug mode, there is an Assume with minimal overhead (acts as an Assert).

l0rinc approved

l0rinc commented at 1:18 PM on April 12, 2026: contributor

ACK fb229dc748e771a27b739ede43cefb7915acf60d

The PR makes the invariants clearer, removes invalid states that could make us hesitant to touch these places: it's a good change, thanks for taking care of it - and for considering our latest comments.

Rebased locally, all tests are passing. Played around with it a bit, recreated a few parts, LGTM, please resolve my remaining comments. I left a few more nits but I'm fine with merging as is. After the manual review I went over each line with Codex/Claude/Gemini to make sure we didn't miss anything - we have the blessing of our robot overlords ❤️🤖❤️

DrahtBot requested review from hodlinator on Apr 12, 2026

hodlinator approved

hodlinator commented at 12:30 PM on April 13, 2026: contributor

re-ACK fb229dc748e771a27b739ede43cefb7915acf60d

Changes since initial ACK:

Sorted BOOST_CHECK_EQUAL(chain_2[x], ...) checks
Removed , and with nullptr from comments
Made fork_point in tests a reference
Added Assert() around fork in index/base.cpp due to improbable condition
Added braces in FindFork()

in src/test/chain_tests.cpp:44 in ae4db06d08

  40 | @@ -41,6 +41,50 @@ const CBlockIndex* NaiveLastCommonAncestor(const CBlockIndex* a, const CBlockInd
  41 |  
  42 |  } // namespace
  43 |  
  44 | +BOOST_AUTO_TEST_CASE(cchain_basic_tests)

maflcko commented at 3:26 PM on April 13, 2026:

nit in ae4db06d083af10ad53d4b85efb3bedba94394f1: Can just call this basic_tests, as the outer suite is already named chain_tests. Also, to remove the c from cchain.

optout21 commented at 8:25 AM on April 15, 2026:

Done.

in src/test/chain_tests.cpp:89 in 17e7f64b82

  85 | @@ -85,6 +86,60 @@ BOOST_AUTO_TEST_CASE(cchain_basic_tests)
  86 |      BOOST_CHECK_EQUAL(chain_0.Genesis(), nullptr);
  87 |  }
  88 |  
  89 | +BOOST_AUTO_TEST_CASE(cchain_findfork_tests)

maflcko commented at 3:27 PM on April 13, 2026:

17e7f64b822b28ec71f0a0cf1f5700b065b409a3: Same, remove c, or better remove cchain_.

optout21 commented at 8:25 AM on April 15, 2026:

Done as well.

in src/index/base.cpp:170 in c41dc1a8be

 165 | @@ -166,7 +166,8 @@ static const CBlockIndex* NextSyncBlock(const CBlockIndex* pindex_prev, CChain&
 166 |  
 167 |      // Since block is not in the chain, return the next block in the chain AFTER the last common ancestor.
 168 |      // Caller will be responsible for rewinding back to the common ancestor.
 169 | -    return chain.Next(chain.FindFork(pindex_prev));
 170 | +    const auto* fork{chain.FindFork(pindex_prev)};
 171 | +    return Assert(fork) ? chain.Next(*fork) : nullptr;

maflcko commented at 3:29 PM on April 13, 2026:

c41dc1a8be16cd47a53c30cde37ff31521fb78de: This is confusing. An Assert can never return false, because it would have failed before. So writing dead code will just be confusing and imply this is doing something else.

optout21 commented at 8:24 AM on April 15, 2026:

Confusing and unnecessary, indeed. After an Assert an if to handle the same is unnecessary. Reconsidering, I changed to a single Assume and no if. This can happen only if the genesis doesn't match, which is extremely unlikely (and would have resulted in a crash before as well). In release mode there is zero overhead (but it will crash in the unlikely case, unchanged behavior). In debug mode, there is an Assume with minimal overhead, and it acts as an Assert. The chances for occurrence is slightly higher in tests (e.g. various test chains), and those are typically executed in debug mode.

maflcko commented at 7:51 AM on April 16, 2026:

In release mode there is zero overhead

Is there really any overhead measurable? Without benchmarks, I'd highly doubt that.

maflcko commented at 3:31 PM on April 13, 2026: member

review fb229dc748e771a27b739ede43cefb7915acf60d~3 🦆

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review fb229dc748e771a27b739ede43cefb7915acf60d~3 🦆
8YLVEuDUMV+qUlnOJeQhmY6gAd0TcM+HN0326dl48KCL5BzoO9luXZHGj8zy9hqWvg2w6wf21/Ez7VaXkRTMCQ==

</details>

optout21 force-pushed on Apr 15, 2026

optout21 commented at 8:32 AM on April 15, 2026: contributor

Minor changes following review (@l0rinc, @maflcko -- thanks!).

Nullptr checks reconsidered, to be less intrusive, no impact on non-debug build. Both cases are very unlikely to occur -- no common genesis, so a debug-only Assume() is sufficient.
in NextSyncBlock() (base.cpp) Assert + if changed to Assume only.
in HeadersDirectFetchBlocks (net_processing.cpp) extra if clause changed to Assume.
Nit: rename the new test cases.

git diff fb229dc748e771a27b739ede43cefb7915acf60d..7bfb61f5bddb75b77523890158d57b7b0892d97a

in src/index/base.cpp:170 in 7bfb61f5bd

 165 | @@ -166,7 +166,8 @@ static const CBlockIndex* NextSyncBlock(const CBlockIndex* pindex_prev, CChain&
 166 |  
 167 |      // Since block is not in the chain, return the next block in the chain AFTER the last common ancestor.
 168 |      // Caller will be responsible for rewinding back to the common ancestor.
 169 | -    return chain.Next(chain.FindFork(pindex_prev));
 170 | +    const auto* fork{chain.FindFork(*pindex_prev)};
 171 | +    return chain.Next(*Assume(fork));

maflcko commented at 9:23 AM on April 15, 2026:

this is equally confusing. It will crash either way, but the stderr is different for debug and release builds for no reason.

If the failure can't happen, then this should just use Assert.

If the failure can happen, then this shouldn't use either assert, and ideally come with a test to cover both paths.

If it is harmless to fall back to something, it may use Assume with a fallback.

optout21 commented at 12:35 PM on April 15, 2026:

With Assume, error log will different in debug&release, which can be confusing. Switching to Assert, thanks for the clarification. Also added explanation comments on the nullptr case. These cases are the "can't happen" category, there is no point in adding fallback handling and tests. (My concern was to minimize potential extra runtime overhead, but that's only possible for non-debug anyways.)

optout21 commented at 12:37 PM on April 15, 2026: contributor

Minor adjustment: changed previously introduced Assume to Assert, following review clarification.

git diff 7bfb61f5bddb75b77523890158d57b7b0892d97a..eb250c189548b11983b5f9c79b339166375dc84f

optout21 force-pushed on Apr 15, 2026

in src/net_processing.cpp:2866 in eb250c1895

2861 | @@ -2862,7 +2862,9 @@ void PeerManagerImpl::HeadersDirectFetchBlocks(CNode& pfrom, const Peer& peer, c
2862 |          // very large reorg at a time we think we're close to caught up to
2863 |          // the main chain -- this shouldn't really happen.  Bail out on the
2864 |          // direct fetch and rely on parallel download instead.
2865 | -        if (!m_chainman.ActiveChain().Contains(pindexWalk)) {
2866 | +        // Note: pindexWalk cannot be nullptr here (that would mean different
2867 | +        // genesis block, which should not happen).

l0rinc commented at 4:43 PM on April 15, 2026:

I'd leave the details in the commit message only - the Assert already states clearly that it cannot be nullptr.

hodlinator commented at 7:23 PM on April 15, 2026:

I think at minimum a one-line comment is nice for this kind of "business logic".

Side-note: I got a bit paranoid thinking that maybe a peer could be sending us headers that are not connected to the same genesis and have us crash... but we should only be getting here if we've got contiguous headers from genesis living in our block tree DB which gives us CBlockIndex-instances which can be sent into this function AFAIK.

in src/index/base.cpp:170 in eb250c1895

 165 | @@ -166,7 +166,9 @@ static const CBlockIndex* NextSyncBlock(const CBlockIndex* pindex_prev, CChain&
 166 |  
 167 |      // Since block is not in the chain, return the next block in the chain AFTER the last common ancestor.
 168 |      // Caller will be responsible for rewinding back to the common ancestor.
 169 | -    return chain.Next(chain.FindFork(pindex_prev));
 170 | +    const auto* fork{chain.FindFork(*pindex_prev)};
 171 | +    // Note: fork cannot be nullptr here (that would mean different genesis block, which should not happen).

l0rinc commented at 4:45 PM on April 15, 2026:

I'm not a fan of comments, I prefer code explain it instead - and the nullptr is already clear from the Assert. If you insist, something like:

    // Common ancestor must exist.

would likely suffice

hodlinator commented at 7:08 PM on April 15, 2026:

If you decide to touch, would at least add genesis to save the mental look-up:

    // Common ancestor must exist (genesis).

optout21 commented at 11:09 AM on April 16, 2026:

Applied.

l0rinc approved

l0rinc commented at 5:51 PM on April 15, 2026: contributor

code review diff ACK eb250c189548b11983b5f9c79b339166375dc84f

DrahtBot requested review from hodlinator on Apr 15, 2026

hodlinator approved

hodlinator commented at 7:27 PM on April 15, 2026: contributor

re-ACK eb250c189548b11983b5f9c79b339166375dc84f

Reasoning: #34440#pullrequestreview-4032653526

Very minor adjustments since previous ACK.

in src/chain.h:443 in 77ee180af8

 439 | @@ -440,7 +440,7 @@ class CChain
 440 |      void SetTip(CBlockIndex& block);
 441 |  
 442 |      /** Find the last common block between this chain and a block index entry. */
 443 | -    const CBlockIndex* FindFork(const CBlockIndex* pindex) const;
 444 | +    const CBlockIndex* FindFork(const CBlockIndex& pindex_ref) const;

maflcko commented at 7:44 AM on April 16, 2026:

nit in 77ee180af8c660d0cd0b708faef6bfa800034ba9: The name is wrong. pindex is a deprecated meaning for "pointer to an index". So pindex_ref doesn't make any sense.

It should just be const& index or const& block or similar. (Same in other commits/functions in this header file)

optout21 commented at 11:09 AM on April 16, 2026:

Renamed.

in src/chain.cpp:53 in 77ee180af8

  53 | -    }
  54 | -    if (pindex->nHeight > Height())
  55 | +const CBlockIndex* CChain::FindFork(const CBlockIndex& pindex_ref) const
  56 | +{
  57 | +    const auto* pindex{&pindex_ref};
  58 | +    if (pindex->nHeight > Height()) {

maflcko commented at 7:48 AM on April 16, 2026:

nit in 77ee180: You are not touching this line, but fixing up the style here. This makes it harder to review. Also, you are not doing style fixups in the other commits in this pull, even though you are touching the lines.

I think it would be better to drop this one as well.

I don't mind style cleanups, but here, it may be best as a separate commit in the end. Maybe append a commit in the end to adjust the {} in this file?

l0rinc commented at 8:50 AM on April 16, 2026:

I suggested some of these minor cleanups - agree that a separate commit at the end is harmless enough

optout21 commented at 11:09 AM on April 16, 2026:

For simplicity, I omitted adding missing braces, only one line is changed in the method.

in src/validation.h:854 in eb250c1895

 850 | @@ -851,7 +851,7 @@ class Chainstate
 851 |      std::pair<int, int> GetPruneRange(int last_height_can_prune) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 852 |  
 853 |  protected:
 854 | -    bool ActivateBestChainStep(BlockValidationState& state, CBlockIndex* pindexMostWork, const std::shared_ptr<const CBlock>& pblock, bool& fInvalidFound, std::vector<ConnectedBlock>& connected_blocks) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs);
 855 | +    bool ActivateBestChainStep(BlockValidationState& state, CBlockIndex& pindexMostWork, const std::shared_ptr<const CBlock>& pblock, bool& fInvalidFound, std::vector<ConnectedBlock>& connected_blocks) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs);

maflcko commented at 8:03 AM on April 16, 2026:

nit in eb250c189548b11983b5f9c79b339166375dc84f: Same (about naming). index_most_work?

optout21 commented at 11:07 AM on April 16, 2026:

Renamed.

in src/test/chain_tests.cpp:140 in 10071089c2 outdated

 135 | +    // Test the blocks in the common part -> result should be the same
 136 | +    check_same(chain_short, blocks_common);
 137 | +    // Test the blocks on the shorter fork -> result should be the same
 138 | +    check_same(chain_short, blocks_short);
 139 | +    // Test the blocks on the other longer fork -> result should be the fork point
 140 | +    check_fork_point(chain_short, blocks_long, blocks_common.back());

maflcko commented at 8:07 AM on April 16, 2026:

nit in 10071089c2860d812c9462de4d1d72a975317e40: Could it make sense to add a test for the invalid case of mixing two unrelated chains?

Something like (pseudo code):

CBlockIndex unrelated_chain{};  // invalid test case. Mixing chains is not supported
BOOST_CHECK_EQUAL(nullptr, chain_short.FindFork(&unrelated_chain));

optout21 commented at 11:06 AM on April 16, 2026:

Test added.

maflcko approved

maflcko commented at 8:09 AM on April 16, 2026: member

lgtm, I just left some style nits.

review ACK eb250c1895 🍋

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK eb250c1895 🍋
LYKJOQQIAAMSh+NvQnX9y+IFjbAo1fYxyjglxdhC61sbycIfntJfN3SCbG5POJp/IezvCDVTyZYuk01lhFIEBw==

</details>

optout21 commented at 8:38 AM on April 16, 2026: contributor

lgtm, I just left some style nits.

Thanks for the comments, they make sense ("pindex_ref" 😜). I will apply them next time there is need to touch (I expect there will be such), but for now I keep the current version.

hodlinator commented at 8:45 AM on April 16, 2026: contributor

Really agree with the naming nits, happy to re-ACK for those. Shouldn't result in touching more lines than currently as we are changing pointer to ref as well, so dereferencing changes.

l0rinc commented at 8:51 AM on April 16, 2026: contributor

I'm also happy to reack, the comments make sense

optout21 commented at 9:59 AM on April 16, 2026: contributor

As there is a group of active reviewers (👍), I share what I plan to do:

Adjust parameter/variable names as suggested
Comments next to the null-check Assert's (comment vs. short comment vs. no comment vs. self-documenting code): I plan to remove the comments and expand the Assert like this: Assert(fork || (chain.Genesis() == pindex_prev->GetAncestor(0))); telling that fork must not be null and also documenting the precondition that at least the genesis block should be the same (|| is used to make sure the rest is not evaluated usually).
Not-strictly-relevant code formatting: drop or move to a separate trailing commit
To clarify: I don't plan to enhance the code to handle disjoint chains in any way in this PR.

Feel free to comment.

maflcko commented at 10:10 AM on April 16, 2026: member

I plan to remove the comments and expand the Assert like this: Assert(fork || (chain.Genesis() == pindex_prev->GetAncestor(0)));

I think a comment reads smoother than this assert. otherwise lgtm on your plan

optout21 commented at 11:11 AM on April 16, 2026: contributor

Strictly minor changes, as discussed with reviewers:

Adjust parameter names (* pVar -> & var)
Comments for null-asserts shortened but kept
Added a unit test for FindFork for invalid unrelated chain case (2 lines).
Code formatting for unaffected lines dropped

git diff eb250c189548b11983b5f9c79b339166375dc84f..bd20048e859b523c38976c2ccc24573206261968

optout21 force-pushed on Apr 16, 2026

l0rinc commented at 11:34 AM on April 16, 2026: contributor

reACK bd20048e859b523c38976c2ccc24573206261968

Rebased, retested locally, reviewed the diff manually, and ran it through an LLM to see if all pending changes have been applied correctly.

<details><summary>LLM audit of code review requests</summary>

No blocking review requests appear to be left unaddressed.

The author has addressed the substantive requests correctly:

- Test cleanups were applied: renamed to basic_tests/findfork_tests, added #include <array>, set bi1.pprev, added Genesis() checks, switched boolean checks to BOOST_CHECK, removed stale “with
  nullptr” comments, and added the unrelated-chain FindFork() test in src/test/chain_tests.cpp:44.
- The naming nits were addressed consistently: Contains(const CBlockIndex& index), Next(const CBlockIndex& index), FindFork(const CBlockIndex& index), and ActivateBestChainStep(...,
  CBlockIndex& index_most_work, ...) are all in place in src/chain.h:410, src/chain.cpp:50, and src/validation.cpp:3180.
- The two controversial null-invariant spots were settled in the reviewer-requested direction: short explanatory comment plus Assert(...), with no dead fallback branch, in src/index/
  base.cpp:169 and src/net_processing.cpp:2865.
- The ActivateBestChainStep() refactor is internally consistent: caller still guards null, callee now takes a reference, and identity checks use &index_most_work correctly in src/
  validation.cpp:3225.

A few minor suggestions were not taken, but they were optional nits, not unresolved correctness requests:

- No extra Assert(chain && entry) was added in btck_chain_contains().
- The test still uses boundary values -1 and 2 instead of exaggerated values like -100/100.
- The author intentionally did not do wider brace/style cleanup in FindFork().

</details>

DrahtBot requested review from hodlinator on Apr 16, 2026

DrahtBot requested review from maflcko on Apr 16, 2026

hodlinator approved

hodlinator commented at 11:36 AM on April 16, 2026: contributor

re-ACK bd20048e859b523c38976c2ccc24573206261968

Reasoning: #34440#pullrequestreview-4032653526

Nicer naming since previous ACK. Good catch on changing the comment. (Also added unit test check for not find fork point for disjoint chains, and reduced verbosity of added comments).

maflcko commented at 12:29 PM on April 16, 2026: member

review ACK bd20048e859b523c38976c2ccc24573206261968 🚉

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK bd20048e859b523c38976c2ccc24573206261968 🚉
OIldHt4+rM3wSgbV03xbUV6ohECv7s5noYVqE6o03wnNuf4FQfCSj/9t8oh/TVKIZWJBMz1FQnNSLqFD9+MOCg==

</details>

DrahtBot added the label CI failed on Apr 16, 2026

DrahtBot removed the label CI failed on Apr 16, 2026

DrahtBot added the label Needs rebase on Apr 19, 2026

test: Add CChain basic tests

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

Co-authored-by: l0rinc <pap.lorinc@gmail.com>

8333abdd91

test: Add CChain::FindFork() tests

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

Co-authored-by: l0rinc <pap.lorinc@gmail.com>

db56bcd692

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, extra nullptr-check is added as
needed.

fe2d6e25e0

Change 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. Notably, NextSyncBlock() now checks
the FindFork() result before calling into Next(), because
the fork lookup may return null.

20b58e281a

Change CChain::FindFork() to take ref

The internal null-guard in FindFork() was removed in favor of adding any missing guards at call sites.

c5eb283bca

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.
Also rename the parameter (prefix p doesn't make sense any more).

7c75244ade

optout21 commented at 1:25 PM on April 20, 2026: contributor

Rebased to master, conflict due to #33477, one less change in src/rpc/blockchain.cpp.

git range-diff bd20048e859b523c38976c2ccc24573206261968...7c75244adef1b3471e81fc95662a93cc1b82412f

optout21 force-pushed on Apr 20, 2026

hodlinator approved

hodlinator commented at 1:31 PM on April 20, 2026: contributor

re-ACK 7c75244adef1b3471e81fc95662a93cc1b82412f

Reasoning: #34440#pullrequestreview-4032653526

Just smaller diff for "Change CChain::Next() to take reference" since previous ACK.

DrahtBot requested review from l0rinc on Apr 20, 2026

l0rinc commented at 1:57 PM on April 20, 2026: contributor

reACK 7c75244adef1b3471e81fc95662a93cc1b82412f

Rebased locally, got the same result, ran tests locally with rebase, they all passed (checked the failing p2p_orphan_handling.py test separately for db56bcd69278804d17ae58c80187c02f8bc15050).

optout21 closed this on Apr 20, 2026

optout21 commented at 2:10 PM on April 20, 2026: contributor

CI failure could not be reproduced locally, the root cause is likely unrelated, likely same as #34731. Retrying. (commit db56bcd69278804d17ae58c80187c02f8bc15050, https://github.com/bitcoin/bitcoin/actions/runs/24669078228/job/72135341107?pr=34440#step:9:16836)

optout21 reopened this on Apr 20, 2026

DrahtBot added the label CI failed on Apr 20, 2026

DrahtBot removed the label CI failed on Apr 20, 2026

DrahtBot removed the label Needs rebase on Apr 20, 2026

maflcko commented at 6:24 AM on April 21, 2026: member

re-review ACK 7c75244adef1b3471e81fc95662a93cc1b82412f 🌅

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-review ACK 7c75244adef1b3471e81fc95662a93cc1b82412f 🌅
tf4F/Yel4HG+bR3Gxs8oEukOHPFXLhoOXU2wnBhvUdmhlaVKXT7RHySVsHALzJrKT2cqBWJ25oMd9Wj2awASBA==

</details>