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

optout21 commented at 5:25 am on January 27, 2026: contributor

Add unit tests to the CChain class, and add a nullptr-check to CChain::Contains() to avoid potential memory access violation by nullptr dereference.

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.

Also, unit tests were added to the CChain class, filling a gap.

Changes:

Add basic unit tests for CChain class methods
Add a nullptr-check to CChain::Contains()

Alternative. I have considered changing Contains() to take a reference instead of pointer, however, when applying that to various methods, it grew to a much bigger changeset – see #34440. Having in mind the friction of larger PRs, I think the present minimal one-check solution (with the tests) makes sense on its own.

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

DrahtBot commented at 5:25 am on January 27, 2026: contributor

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

Code Coverage & Benchmarks

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	sedited
Stale ACK	HowHsu

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:

#34440 (Refactor CChain methods to use references, tests by optout21)

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:

// Call with non-tip & tip blocks, and with nulltr -> // Call with non-tip & tip blocks, and with nullptr [“nulltr” is a misspelling; it should be “nullptr” to match the C++ token and avoid confusion.]

2026-03-25 05:21:17

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

in src/net_processing.cpp:1926 in 7f27d102ae outdated

1922@@ -1923,6 +1923,7 @@ void PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati
1923 bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex* pindex)
1924 {
1925     AssertLockHeld(cs_main);
1926+    if (!pindex) return false;

maflcko commented at 8:28 am on January 27, 2026:

i think this goes in the wrong direction. All call sites have checked against nullptr, and there should never be a reason to call this with nullptr, so this seems it should be a reference. Just like BlockRequested and FetchBlock take a reference.

optout21 commented at 1:19 pm on January 27, 2026:

Following this feedback, I will reconsider the switch to pass-by-reference. As mentioned in the description, I started in that direction, but was concerned about consistency. For consistency, it makes sense to also change the other methods (e.g. Next()), increasing the scope. Const input pointer parameters are relatively easy to change to pass-by-reference; but return values where nullptr is a valid option should be changed to std::option<&...>. These end up in a larger changeset (arguably not adding much value).

maflcko commented at 1:23 pm on January 27, 2026:

Well, i haven’t looked into the lower level chain util methods. For some methods, it could make sense to accept a pointer. My comment was mostly about BlockRequestAllowed, and other similar high level functions that only work on blocks that exist and whose presence has been previously confirmed

maflcko commented at 7:52 pm on January 29, 2026:

bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex*) should be changed to bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex&)

optout21 commented at 8:06 am on January 30, 2026:

Being done in https://github.com/bitcoin/bitcoin/pull/34440/changes/6539829321aa13b4afb8d898a62eb3be382607c0 ( #34440 )

maflcko commented at 9:06 am on January 30, 2026:

i don’t understand why the some line of code should be changed, only to revert it in the next commit. Please submit 6539829321aa13b4afb8d898a62eb3be382607c0 as a separate pull request

maflcko commented at 9:07 am on January 30, 2026:

also, the name pindex should probably be changed to block_index (or so), like in the function immediately below

optout21 commented at 3:06 pm on January 30, 2026:

i don’t understand why the some line of code should be changed, only to revert it in the next commit. Please submit 6539829 as a separate pull request

I can do it in a separate PR, should be trivial. The add/remove is a side-effect of independent commits: one commit changed the method called from here (Contains), so a null check had to be added before the call. Then when this method has also been change to take a reference input, the check is no longer needed. (The check moved outwards.)

optout21 commented at 11:27 pm on February 5, 2026:

Note: BlockRequestAllowed change has been done in #34464.

optout21 marked this as a draft on Jan 27, 2026

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

I’m exploring the option to use references, but I’ve decided to keep this simple PR as first step.

As suggested, I look into using references instead of pointers. Howerver, that is a bigger refactor. I think the minimal change (one extra check) and unit tests from this PR make sense in isolation, as the bigger change will be heavier. Draft PR #34440 opened. I will strip this one a bit more. CC: @maflcko

optout21 force-pushed on Jan 29, 2026

optout21 marked this as ready for review on Jan 29, 2026

optout21 commented at 11:16 am on January 29, 2026: contributor

Marked ready for review.

in src/test/chain_tests.cpp:60 in 0c2df01acd outdated

56@@ -57,9 +57,9 @@ BOOST_AUTO_TEST_CASE(cchain_basic_tests)
57         BOOST_CHECK_EQUAL(chain_0.Tip(), nullptr);
58         BOOST_CHECK_EQUAL(chain_0[0], nullptr);
59         BOOST_CHECK_EQUAL(chain_0.Contains(&genesis), false);
60-        // BOOST_CHECK_EQUAL(chain_0.Contains(nullptr), false); // fail with memory access violation
61+        BOOST_CHECK_EQUAL(chain_0.Contains(nullptr), false);

HowHsu commented at 1:19 pm on January 29, 2026:

Would it be better to adjust the order of these two commits to make this one first, so that no tests needed to be changed in this commit?

optout21 commented at 9:06 am on February 5, 2026:

IMHO it’s better to have the test first. The commented out tests could be omitted from the first test commit, and only added with the behavior change, but I chose this way so that it’s visible in the commit how it affects behavior/test.

The general approach I try to follow here:

find/identify an issue
create a test for it that exposes the issue
fix the issue and adjust the test to the correct behavior

In this case the call with nullptr crashes, so there is no easy way to actually test that, hence it was added just as a comment.

HowHsu commented at 9:39 am on February 5, 2026:

IMHO it’s better to have the test first. The commented out tests could be omitted from the first test commit, and only added with the behavior change, but I chose this way so that it’s visible in the commit how it affects behavior/test.

The general approach I try to follow here:

find/identify an issue

create a test for it that exposes the issue

fix the issue and adjust the test to the correct behavior

In this case the call with nullptr crashes, so there is no easy way to actually test that, hence it was added just as a comment.

Make sense.

maflcko changes_requested

HowHsu commented at 9:42 am on February 5, 2026: none

test ACK 0c2df01

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

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

Rebased, following merging of #34464 . One adaptation became obsolete: the added nullptr-check in PeerManagerImpl::BlockRequestAllowed() is no longer needed, as the input parameter has been changed to a reference (in #34464). The file src/net_processing.cpp is no longer in the changeset. 0c2df01acda5891e5c2cea89753ffb5715b2f287..6c3a91f9cefe28a9c7a59963a51c2d64d8c005a2 Ready for (re)review.

DrahtBot added the label CI failed on Feb 6, 2026

DrahtBot removed the label Needs rebase on Feb 6, 2026

HowHsu commented at 10:03 am on February 7, 2026: none

ACK 6c3a91f9cefe28a9c7a59963a51c2d64d8c005a2

By the way, the CI test failed not because of the PR code.

DrahtBot removed the label CI failed on Feb 9, 2026

in src/test/chain_tests.cpp:100 in ced935ac6c outdated

 95+        BOOST_CHECK_EQUAL(chain_2.Contains(&genesis), true);
 96+        BOOST_CHECK_EQUAL(chain_2.Contains(&bi1), true);
 97+        BOOST_CHECK_EQUAL(chain_2.Next(&genesis), &bi1);
 98+        BOOST_CHECK_EQUAL(chain_2.Next(&bi1), nullptr);
 99+    }
100+}

sedited commented at 5:18 pm on March 23, 2026:

This test seems overly verbose to me, and I don’t think it is meaningfully documenting anything either. We have a test that exercises the block index below, maybe extend that instead?

optout21 commented at 4:56 am on March 25, 2026:

The existing chain_test() is a rather specific use case, and it doesn’t use CChain at all, therefore I would not combine with that. What I prefer to do is to remove some tests, and rearrange the calls by called function, and keep only one for each different case. I see some value even in basic tests, such as “what happens if Next() is called with nullptr?” or “What happens if indexer is called with a non-existing index?”. But if you prefer, I can remove some more, keep only the nullptr-input ones, or even none.

in src/chain.h:414 in 6c3a91f9ce

409@@ -410,7 +410,9 @@ class CChain
410     /** Efficiently check whether a block is present in this chain. */
411     bool Contains(const CBlockIndex* pindex) const
412     {
413-        return (*this)[pindex->nHeight] == pindex;
414+        if (pindex)
415+            return (*this)[pindex->nHeight] == pindex;

sedited commented at 5:18 pm on March 23, 2026:

This should either be on the same line as the condition or wrapped in braces per the style guide.

optout21 commented at 4:44 am on March 25, 2026:

Done. (I was following the style of the surrounding code, but better to follow the style guide.)

test: Add CChain basic tests

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

d8518bce73

Add nullptr-check to CChain::Contains()

The `CChain::Contains()` method dereferences its input without checking,
potentially resulting in nullptr-dereference if invoked with `nullptr`.
The `Next()` method can also trigger this. Avoid the memory access
violation by adding an extra check for the `pindex` input.
The result is `false` for `nullptr` input.

c8949c4f12

optout21 force-pushed on Mar 25, 2026

optout21 commented at 5:22 am on March 25, 2026: contributor

Minor rework following review comments:

Reduced the number of test calls, rearranged the calls by called function, kept only one call for each different case.
Also rebased to recent master.

sedited approved

sedited commented at 9:12 am on March 25, 2026: contributor

ACK c8949c4f12d7d64bbb4c661f7944e5a1606bd38b

DrahtBot requested review from HowHsu on Mar 25, 2026

sedited removed review request from HowHsu on Mar 25, 2026

sedited requested review from hodlinator on Mar 25, 2026

in src/chain.h:412 in c8949c4f12

408@@ -409,6 +409,7 @@ class CChain
409     /** Efficiently check whether a block is present in this chain. */
410     bool Contains(const CBlockIndex* pindex) const
411     {
412+        if (!pindex) return false;

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

I do not agree with adding this presently redundant guard. Existing code of master never calls Contains() with nullptr, otherwise it would have crashed.

Much more in favor of #34440.

DrahtBot requested review from HowHsu on Mar 26, 2026

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

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)