refactor: Change CChain methods to use references, add tests by optout21 · Pull Request #34440 · bitcoin/bitcoin

optout21 · 2026-01-29T09:21:03Z

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 · 2026-01-29T09:21:51Z

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, achow101

If your review is incorrectly listed, please copy-paste  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.

DrahtBot · 2026-01-29T09:49:39Z

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

Hints

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 · 2026-01-30T08:08:43Z

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

optout21 · 2026-02-03T11:00:47Z

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

1f8f7d4 Change BlockRequestAllowed() to take ref (optout) Pull request description: As [suggested here](#34416 (comment)), a minor refactor of `PeerManagerImpl::BlockRequestAllowed()` to take reference parameter (instead of pointer). The motivation is to make the code safer, by minimizing the risk of null-dereference, and to be more consistent. The change is local to the `PeerManagerImpl::BlockRequestAllowed()` class. Related to #34440. ACKs for top commit: maflcko: review ACK 1f8f7d4 🎐 l0rinc: tested ACK 1f8f7d4 stickies-v: ACK 1f8f7d4 Tree-SHA512: 9c2de2d3e7d067e018db7ec54c79f512ccc1da54574d4fb362f6697ee6e235783779d7094cf20856cd34e08a1dbc74609d8351fe7b2287cd8ec0c836ef07be19

optout21 · 2026-02-05T23:50:11Z

Rebased, following #34464 ; one commit got annihilated.

7zkm7b8gw9-web

Yes

Add basic unit tests to the `CChain` class, filling a gap. Co-authored-by: l0rinc <pap.lorinc@gmail.com>

Add (lengthier) unit tests for `CChain::FindFork()`. Co-authored-by: l0rinc <pap.lorinc@gmail.com>

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.

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.

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

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

optout21 · 2026-04-20T13:25:09Z

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

git range-diff bd20048e859b523c38976c2ccc24573206261968...7c75244adef1b3471e81fc95662a93cc1b82412f

hodlinator

re-ACK 7c75244

Reasoning: #34440 (review)

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

l0rinc · 2026-04-20T13:57:47Z

reACK 7c75244

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

optout21 · 2026-04-20T14:10:40Z

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

maflcko · 2026-04-21T06:24:48Z

re-review ACK 7c75244 🌅

Show signature

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==

achow101 · 2026-04-22T22:41:08Z

ACK 7c75244

maflcko · 2026-04-23T05:34:48Z

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.

I wonder if there should be a tracking issue for those, to keep track. No rush, but some of them seem interesting to look into in the long term.

optout21 · 2026-04-23T05:44:03Z

I wonder if there should be a tracking issue for those

I haven't consider this topic large/important/systematic enough to warrant a tracking issue or even a plain issue. The mentioned follow-ups are on my list :)

optout21 mentioned this pull request Jan 29, 2026

Add nullptr-check to CChain::Contains(), tests #34416

Closed

optout21 force-pushed the cchain-ref branch from fcb8215 to d228e16 Compare January 29, 2026 09:48

DrahtBot added the CI failed label Jan 29, 2026

optout21 force-pushed the cchain-ref branch from d228e16 to 9db0568 Compare January 29, 2026 09:55

maflcko reviewed Jan 29, 2026

View reviewed changes

Comment thread src/chain.h Outdated

optout21 force-pushed the cchain-ref branch from 9db0568 to 9a55aaa Compare January 29, 2026 11:01

DrahtBot mentioned this pull request Jan 29, 2026

[RFC] CChain Concurrency Improvement (Base + Tail Architecture) #34424

Closed

optout21 mentioned this pull request Jan 30, 2026

Change BlockRequestAllowed() to take ref (minor refactor) #34464

Merged

DrahtBot removed the CI failed label Feb 3, 2026

DrahtBot mentioned this pull request Feb 4, 2026

index: batch db writes during initial sync #34489

Draft

DrahtBot added the Needs rebase label Feb 5, 2026

optout21 force-pushed the cchain-ref branch 2 times, most recently from d7f296b to 32457f4 Compare February 5, 2026 23:47

DrahtBot added the CI failed label Feb 5, 2026

DrahtBot removed the Needs rebase label Feb 6, 2026

This was referenced Feb 6, 2026

index: Check all necessary block data is available before starting to sync #29770

Merged

index: initial sync speedup, parallelize process #26966

Draft

7zkm7b8gw9-web reviewed Feb 6, 2026

View reviewed changes

DrahtBot mentioned this pull request Feb 12, 2026

refactor: extract BlockDownloadManager from PeerManagerImpl #34565

Open

DrahtBot added CI failed Needs rebase and removed CI failed labels Apr 16, 2026

optout21 and others added 6 commits April 20, 2026 08:55

test: Add CChain basic tests

8333abd

Add basic unit tests to the `CChain` class, filling a gap. Co-authored-by: l0rinc <pap.lorinc@gmail.com>

test: Add CChain::FindFork() tests

db56bcd

Add (lengthier) unit tests for `CChain::FindFork()`. Co-authored-by: l0rinc <pap.lorinc@gmail.com>

Change CChain::FindFork() to take ref

c5eb283

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

optout21 force-pushed the cchain-ref branch from bd20048 to 7c75244 Compare April 20, 2026 13:25

hodlinator approved these changes Apr 20, 2026

View reviewed changes

DrahtBot requested a review from l0rinc April 20, 2026 13:31

optout21 closed this Apr 20, 2026

optout21 reopened this Apr 20, 2026

DrahtBot added CI failed and removed CI failed Needs rebase labels Apr 20, 2026

optout21 mentioned this pull request Apr 28, 2026

Avoid nullptr-reference in CChain::Contains() optout21/bitcoin#2

Closed

This was referenced May 6, 2026

[draft] Use CBlockIndex parameters as reference optout21/bitcoin#10

Closed

refactor: Use CBlockIndex parameters as reference #35229

Open

hodlinator mentioned this pull request May 12, 2026

kernel: Refactor process_block_header to return btck_BlockValidationState #33856

Merged

kevkevinpal mentioned this pull request May 21, 2026

refactor: Change CChain methods to use references, add tests (PR #34440) kevkevinpal/bitcoin#374

Open

maflcko mentioned this pull request Jul 6, 2026

init: fix reindex deadlock by waking cv after interrupt #35652

Merged

Uh oh!

Conversation

optout21 commented Jan 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jan 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

DrahtBot commented Jan 29, 2026

Uh oh!

Uh oh!

optout21 commented Jan 30, 2026

Uh oh!

optout21 commented Feb 3, 2026

Uh oh!

optout21 commented Feb 5, 2026

Uh oh!

7zkm7b8gw9-web left a comment

Choose a reason for hiding this comment

Uh oh!

optout21 commented Apr 20, 2026

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

l0rinc commented Apr 20, 2026

Uh oh!

optout21 commented Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Apr 21, 2026

Uh oh!

achow101 commented Apr 22, 2026

Uh oh!

maflcko commented Apr 23, 2026

Uh oh!

optout21 commented Apr 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

optout21 commented Jan 29, 2026 •

edited

Loading

DrahtBot commented Jan 29, 2026 •

edited

Loading

optout21 commented Apr 20, 2026 •

edited

Loading