#36066 validation: Separate check-only version of ConnectBlock

ReviewabilityReviewsAgreementSize
#36066 optout21 validation: Separate check-only version of ConnectBlockReady0Strong60 + 5 tests
  • Clarify mutability guarantees in block validation when running in check-only mode
  • Avoid requiring caller-side dummy objects in validation tests and fuzzing
  • Ready to review.
  • Concept support for clarifying index mutability and cleaning the public interface (l0rinc)
  • Concern over check-only mode still mutating internal state answered as a compatible step (purpleKarrot)
  • Objection to renaming fuzz target resolved by reverting the rename (marcofleon)
  • Clarified that check flag was removed from public interface but kept internally (mzumsande)
  • 60 lines added or modified outside tests
  • 5 lines added or modified in tests
  • 20 lines removed in total
  • 4 files, 1 commits
Validation#49 of 53
P4 · cleanup
  • P4 because it is an internal structural refactor with no consensus, safety, or performance impact.
  • Improves code clarity in test paths without affecting node runtime behavior.
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

Separates the check-only execution path in `Chainstate::ConnectBlock` into a dedicated `TestConnectBlock` method taking `const CBlockIndex&`. The shared UTXO validation checks are moved into an internal helper `ConnectBlockChecks`, eliminating the public `fJustCheck` boolean parameter.

Problem: In check-only validation mode (used by `TestBlockValidity` and fuzz testing), `ConnectBlock` required a mutable `CBlockIndex*` even though the block index was not modified, obscuring mutability guarantees and requiring caller-side dummy objects.

Reviewability

Ready

Nothing known that would invalidate a review now.

Waiting on (from the thread read):

  • reviewer: reviewer response after author answered mzumsande's question and updated commit message and description 2026-09-26

Agreement

Strong

substantive support, no open objection (l0rinc)

Thread notes: Author replied to mzumsande clarifying that fJustCheck was only eliminated from the public interface and updated the commit message and PR description accordingly.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
purpleKarrot (member)objection, approachcalling TestConnectBlock still mutates internal Chainstate state such as num_blocks_totalanswered · author explained the PR is a compatible incremental step towards #35904 · v1, not yet re-checkedansweredyes2026-08-25: 'With the proposed change, calling `TestConnectBlock` still has the side effect of mutating `num_blocks_total`, for example.'
Status from: 2026-09-04: optout21: 'I think that the small change from this PR is compatible with the approach laid out in #35904. It is a small step, but i'
marcofleon (member)objection, maintenancefuzzing corpora are keyed by target name, requiring manual intervention if renamedanswered · author agreed to revert the fuzz target rename · v1, not yet re-checkedansweredyes2026-09-18: 'Our fuzzing infra corpora are keyed by target name, so we'd have to manually deal with renaming.'
Status from: 2026-09-18: optout21: ''
mzumsande (member)question, approachWhy is fJustCheck still present in ConnectBlockChecks when the PR description and commit message indicate it was dropped?answered · author explained the flag was removed from public methods but kept internally, and updated description and commit messageansweredyes2026-09-25: 'The PR description / commit message makes it sound like `fJustCheck` is dropped, but it is clearly still there, just moved to `ConnectBlockChecks`?!'
Status from: 2026-09-26: optout21: 'I agree that the "flag dropped" formulation is not accurate, I will update the commit message and the description, thanks.'

Support:

  • l0rinc (member) (Concept ACK, 2026-08-26) [read]: Separating the check-only entry point makes the CBlockIndex mutability clearer and removes fJustCheck from the public ConnectBlock() interface.

Participants:

  • l0rinc (member, support, 4 statements 2026-08-25 to 2026-08-26): Concept ACK; suggested passing block index by const reference and updating comments
  • purpleKarrot (member, objection, 1 statements 2026-08-25 to 2026-08-25): noted Chainstate still mutates state during check-only calls
  • marcofleon (member, objection, 1 statements 2026-09-18 to 2026-09-18): objected to renaming the fuzz target due to fuzzing corpora keyed by name
  • mzumsande (member, question, 1 statements 2026-09-25 to 2026-09-25): asked why fJustCheck is still present in ConnectBlockChecks when PR description indicates it was dropped

Files

11 lines under test/bench/ci.

  • src/validation.cpp +44/-13
  • src/validation.h +16/-1
  • src/test/fuzz/connect_block.cpp +4/-5
  • test/functional/interface_usdt_utxocache.py +1/-1

Card

The short text the ranking pass compares across PRs.

This PR refactors `Chainstate::ConnectBlock` by separating its check-only mode into a distinct `TestConnectBlock` method that accepts a `const CBlockIndex&`. This removes the `fJustCheck` boolean parameter and clarifies const-correctness for callers that only validate blocks without mutating chainstate, such as `TestBlockValidity` and fuzz tests. The work is code hygiene that also supports ongoing modularization toward side-effect-free validation (#35904). Review feedback from l0rinc has been incorporated and the PR is ready for review.

Data

dossier JSON · extract JSON · model ledger, generated 2026-09-27T01:22, confidence high, input hash 6ecfdbb5d8e6f839
thread read through 2026-09-26 (20 statements), head f478dc075e
Discussion read under schema 1 rules: suggestions and questions may be missing, and an objection may combine several harms (schema history).
labels: Validation

Categories

Validation

P4 · cleanup

Refactors Chainstate::ConnectBlock by splitting out TestConnectBlock, removing the fJustCheck boolean and clarifying constness guarantees for CBlockIndex. While this improves code clarity and eliminates caller-side dummy objects in test paths, it is an internal structural refactor with no consensus, safety, or performance impact.

Membership: Validation label and changes to Chainstate block validation interfaces in src/validation.{h,cpp}.

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