#35229 refactor: Use CBlockIndex parameters as reference

ReviewabilityReviewsAgreementSize
#35229 optout21 refactor: Use CBlockIndex parameters as referenceReady0Strong72 + 7 tests
  • Enforce non-null input invariants at compile time across block index and chain handling
  • Reduce risks of unexpected null dereferences and redundant defensive checks for maintainers
  • Ready for review
  • Strong support with all reviewer suggestions addressed
  • Approved citing better clarity and stronger compile-time non-null guarantees (musaHaruna)
  • Verified reference conversions, call sites, and ancestry assumptions (musaHaruna)
  • Concept approval without stated reasons (stickies-v, w0xlt)
  • 72 lines added or modified outside tests
  • 7 lines added or modified in tests
  • 68 lines removed in total
  • 12 files, 7 commits
Validation#51 of 53
P4 · cleanup
  • P4 because it is an internal cleanup with no effect on consensus or chain validation correctness
  • The maintainability and robustness improvements across the codebase are modest
Ranking notes
Reviewability: ReadyStalePaused
Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets
Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs.
Size: lines added or modified outside tests, then in tests; greener = smaller.

Goal

This PR refactors multiple internal functions across chain management, block index handling, block filters, and peer processing to take `CBlockIndex` by reference instead of pointer where null arguments are prohibited. It also switches `CChain`'s internal vector storage from raw `CBlockIndex*` pointers to `std::reference_wrapper<CBlockIndex>`.

Problem: Functions accepting pointer arguments where null is invalid fail to communicate or enforce the non-null invariant through the type system, creating risks of unexpected nullptr dereferences or requiring redundant defensive checks.

Evidence of importance:

  • Author describes the change as a 'janitorial refactor, a continuation of #34440' for 'slight improvement in code quality, maintainability and robustness' with 'more explicit representation of the non-null input invariant'.

Risk of a mistake: medium (validation, p2p). Unchecked dereferencing of a pointer passed into the newly converted reference parameters could trigger an assertion failure or crash the node during chain sync or P2P message handling.

Changed since the previous assessment: No functional changes to the scope; appears to be a rebase or minor code cleanup maintaining the same commit structure and affected methods.

Reviewability

Ready

Nothing known that would invalidate a review now.

Waiting on (from the thread read):

  • reviewer: musaHaruna and others to review the updated commits following suggestions and rebase 2026-10-27

Agreement

Strong

substantive support, no open objection (musaHaruna, musaHaruna)

Thread notes: The author responded to musaHaruna's remaining suggestions, pushed fixes for them alongside self-review tweaks, and rebased.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
musaHaruna (contributor)suggestion, maintenancemake the comment concise by removing mention of reserve and stating the core invariantfixed · fix pushed · author simplified the comment in a pushansweredyes2026-05-19: 'Nit: We can make this comment a bit more concise and technically precise.'
Status from: 2026-05-20: optout21: 'Simplified the comment, as suggested, to only "Pointers, never nullptr"'
musaHaruna (contributor)suggestion, interfaceexplicitly pass the default nullptr with an inline comment to match codebase conventionanswered · author declined, stating it is unrelated to the PR and optional parameters should not be passed explicitlyansweredyes2026-05-19: 'I think we can explicitly pass the default `nullptr` with an inline comment here to match the codebase convention.'
Status from: 2026-05-20: optout21: 'This is not related to this PR, plus I don't see why to explicitly pass an optional parameters, so not taking this.'
maflcko (member)objection, correctnessconfusing return-value invariant assumption in LastCommonAncestorfixed · fix pushed · author explained call-site genesis invariant and changed Assume to Assertchangedyes2026-05-19: 'As explained previously, this seems confusing, see https://github.com/bitcoin/bitcoin/pull/34440#discussion_r3085393412'
Status from: 2026-05-20: optout21: 'I've double-checked the usages (call sites), and this can't happen -- all chain segments must have the same genesis. The'
maflcko (member)suggestion, approachuse std::reference_wrapper instead of raw pointers and a code commentanswered · fix pushed · author adopted std::reference_wrapper for CChain::vChain and repliedansweredyes2026-05-19: 'If the code-typing overhead of `.get()` is not too much, why not just use std::reference_wrapper, which enforces everything at compile-time without any runtime overhead and does not rely on a code com'
Status from: 2026-05-26: optout21: 'Did change `CChain::vChain` array to store `std::reference_wrapper<CBlockIndex>` items instead of pointers'
musaHaruna (contributor)suggestion, maintenanceinclude <functional> directly for std::reference_wrapperfixed · fix pushed · author applied the suggestion and pushedansweredyes2026-10-23: 'Could we include `<functional>` directly for `std::reference_wrapper`, as recommended by the developers note?'
Status from: 2026-10-27: optout21: 'Applied your suggestions.'
musaHaruna (contributor)suggestion, styleclarify commit message for cec59ef to describe adding comments instead of assertionfixed · fix pushed · author applied the suggestion and pushedansweredyes2026-10-23: 'In cec59ef The commit message mentions adding an assertion, but the diff only adds comments. It would be more accurate to describe this as documenting the non-null invariant.'
Status from: 2026-10-27: optout21: 'Applied your suggestions.'
musaHaruna (contributor)suggestion, approachuse std::ref(block) directly as resize fill value rather than constructing a local CBlockIndex placeholderfixed · fix pushed · author confirmed and pushed the changeansweredyes2026-10-23: 'Could we use `std::ref(block)` directly as the `resize()` fill value instead of constructing a local `CBlockIndex placeholder`?'
Status from: 2026-10-27: optout21: 'Done.'

Support:

  • stickies-v (member) (Concept ACK, 2026-05-07): (no reason given) [not substantive]
  • musaHaruna (contributor) (Concept ACK, 2026-05-19) [read]: Changing parameters from const `CBlockIndex*` to `const CBlockIndex&` where `nullptr` is not a valid state improves clarity and gives stronger compile-time guarantees around non-nullability by making
  • w0xlt (contributor) (Concept ACK, 2026-06-14): (no reason given) [not substantive]
  • musaHaruna (contributor) (2026-10-23) [read]: Reviewed the reference conversions, their call sites, and the relevant non-null and ancestry assumptions. I think the code looks good.

Participants:

  • stickies-v (member, support, 2 statements 2026-05-07 to 2026-05-07): Concept ACK and a naming nit
  • maflcko (member, objection, 2 statements 2026-05-19 to 2026-05-19): pointed out confusing invariant assumption in LastCommonAncestor and suggested std::reference_wrapper
  • musaHaruna (contributor, support, 5 statements 2026-05-19 to 2026-10-23): Concept ACK and code review with suggestions on includes and resize fill value
  • w0xlt (contributor, support, 1 statements 2026-06-14 to 2026-06-14): Concept ACK
  • sedited (member, neutral, 1 statements 2026-09-22 to 2026-09-22): pinged reviewers for another review

Files

14 lines under test/bench/ci.

  • src/net_processing.cpp +24/-20
  • src/chain.cpp +14/-11
  • src/index/base.cpp +9/-9
  • src/chain.h +10/-6
  • src/index/blockfilterindex.cpp +8/-8
  • src/test/blockfilter_index_tests.cpp +6/-6
  • src/index/blockfilterindex.h +2/-2
  • src/rpc/blockchain.cpp +2/-1
  • src/validation.cpp +1/-2
  • src/index/base.h +1/-1
  • src/node/interfaces.cpp +1/-1
  • src/test/chain_tests.cpp +1/-1

Card

The short text the ranking pass compares across PRs.

This pull request converts `CBlockIndex` pointer arguments to reference parameters across `LastCommonAncestor`, block filter lookups, `BaseIndex::ProcessBlock`, and several `PeerManagerImpl` block download helpers where null is invalid. It also changes `CChain`'s internal vector storage to hold `std::reference_wrapper<CBlockIndex>` instead of raw pointers. The change addresses internal type safety and non-null invariant documentation across consensus-adjacent and P2P code paths. The author presents this purely as a janitorial code-quality refactor continuing from merged PR #34440 with no external payoff.

Data

dossier JSON · extract JSON · model ledger, generated 2026-10-29T01:22, confidence high, input hash e6241c11e40af06c
thread read through 2026-10-27 (21 statements), head 61566b5a3a
labels: Refactoring

Categories

Validation

P4 · cleanup

This janitorial refactor updates CChain storage and function signatures to pass CBlockIndex by reference where non-null invariants hold. As the author states, it is a 'janitorial refactor' providing 'slight improvement in code quality, maintainability and robustness' with no impact on consensus, block connection, or chainstate correctness.

Membership: Directly refactors block-index management classes and functions in src/chain.h and src/chain.cpp, including CChain's internal storage.

Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 0