#34254 validation: Prevent duplicate logging and looping in invalid block handling

ReviewabilityReviewsAgreementSize
#34254 mzumsande validation: Prevent duplicate logging and looping in invalid block handlingAuthor silent 74d2 (+1)Strong35 + 29 tests
  • Prevent duplicate tree traversals and repeated log messages when a node encounters an invalid block
  • Avoid running redundant block failure and header recalculation passes in immediate succession
  • Ready for review
  • Substantive support for improving chain activation error handling (Crypt-iQ)
  • Objection regarding missing failure flags on out-of-chain blocks was resolved and verified (Crypt-iQ)
  • Concept approval for the refactoring approach and cleanup (frankomosh, stratospher)
  • Approvals without detailed comments (bensig, sedited)
  • 35 lines added or modified outside tests
  • 29 lines added or modified in tests
  • 30 lines removed in total
  • 6 files, 3 commits
Validation#43 of 53
P3 · cleanup
  • P3 because it eliminates redundant work in a rare error path during chain activation
  • Performance and log noise improvements are modest and only triggered when encountering invalid blocks
  • Ranking pass: Moved down: redundant work only in a rare invalid-block path, least consequential of the refactor group
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

Refactors invalid block handling in validation by moving block failure flag propagation, best header recalculation, and fork warning checks out of InvalidChainFound into InvalidBlockFound and InvalidateBlock. In ActivateBestChain, this avoids redundant loops over the block index and duplicated log entries previously triggered by running the full failure sequence twice. Renames the residual InvalidChainFound function to UpdateBestInvalid to reflect its narrowed role of tracking the invalid block with the most work.

Problem: When ActivateBestChain encounters an invalid block, it calls InvalidBlockFound on the failed block and then called InvalidChainFound on the chain candidate. Because InvalidChainFound ran SetBlockFailureFlags and RecalculateBestHeader and logged chain state, these block index tree traversals and log messages were executed twice in immediate succession.

Evidence of importance:

  • PR description cites review discussions in PR #31405 (review comment r2135717500) and PR #31533 (review comment r2607189790) identifying repeated work in InvalidateBlock and ActivateBestChain.

Risk of a mistake: medium (validation). A mistake in propagating block failure flags or recalculating the best header could leave nodes tracking an invalid chain tip or misrepresenting chain validity.

Changed since the previous assessment: Added a functional test in test/functional/rpc_invalidateblock.py verifying that invalidating headers that were never connected to the active chain properly marks descendant headers invalid and resets the best header.

Reviewability

Stale · Author silent 74d

No author activity for 74 days; a review may wait a long time for a response.

Waiting on (from the thread read):

  • nothing

Agreement

Strong

substantive support, no open objection (Crypt-iQ)

Thread notes: Brought the record up to date under the new rules. Added questions and suggestions from Crypt-iQ, frankomosh, and stratospher; adjusted clears_with on the out-of-chain bug; and updated sedited's stance to support following their review ACK. / Updated under schema 2 rules: corrected clears_with to change on Crypt-iQ's out-of-chain defect objection and set status to accepted following the fix push and crACK; added questions and suggestions from Crypt-iQ, frankomosh, and stratospher; and updated se

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
Crypt-iQ (member)question, correctnesswhether m_best_invalid fails to be set properly when pindex is in the main chainaccepted · author confirmed it is pre-existing; reviewer agreed it can be addressed in a follow-upansweredyes2026-01-20: 'I think this can fail to set `m_best_invalid` properly since `to_mark_failed` walks backwards (if `pindex` is in the main chain) and then `UpdateBestInvalid` is called after looping?'
Status from: 2026-08-27: Crypt-iQ: 'I also think `m_best_invalid` could be updated properly in a follow-up PR.'
frankomosh (contributor)question, approachwhether the second call to UpdateBestInvalid is redundant with InvalidBlockFound in ConnectTipaccepted · Crypt-iQ explained the difference and frankomosh acceptedansweredby others2026-01-23: 'I think this update is already handled by `InvalidBlockFound()`, called from within `ConnectTip()`, so this second call is maybe unnecessary?'
Status from: 2026-01-24: frankomosh: 'Ah, I see and understand it now. Thanks for clarifying'
Crypt-iQ (member)objection, correctnessInvalidateBlock misses SetBlockFailureFlags and RecalculateBestHeader when the target block is not in the active chainfixed · fix pushed · author restored missing calls in ff06c05 with test; reviewer crACKedchangedyes2026-02-02: 'for the case where to_mark_failed is out-of-chain, I think it's missing SetBlockFailureFlags & RecalculateBestHeader?'
Status from: 2026-08-27: Crypt-iQ: 'crACK ff06c05043bf2324085dd0a1969239a9fd19eda5'
stratospher (member)suggestion, approachcombine the two iterations over the block index into a single passanswered · author explained that optimizing this rare error path is unnecessaryansweredyes2026-03-24: 'wondering if it’s also possible to avoid looping over the block index twice and instead combine this into a single pass?'
Status from: 2026-08-19: mzumsande: 'I didn't do that here - I feel like it's not really necessary to optimize too much'
frankomosh (contributor)objection, interfaceChanges to InvalidateBlock log prefix and format may break external log monitoringanswered · author explained logs are not a stable interface and RPC logging is distinctansweredyes2026-05-18: 'might silently break any log monitoring on the old string.'
Status from: 2026-08-19: mzumsande: 'Since logs are not a guaranteed stable interface, I don't think log monitoring is an issue'

Support:

  • bensig (contributor) (ACK, 2026-01-12): ACK 73e0bab7adb0b16b2ac4d13c9d601a04b7184749 [not substantive]
  • frankomosh (contributor) (Concept ACK, 2026-01-23): Concept ACK on the overall refactoring approach. [not substantive]
  • stratospher (member) (Concept ACK, 2026-03-24) [read: validation.h]: Concept ACK. nice cleanup! [not substantive]
  • Crypt-iQ (member) (ACK, 2026-08-27) [read: ActivateBestChain, InvalidateBlock]: This PR by itself is a good improvement to ActivateBestChain.
  • sedited (member) (ACK, 2026-10-09): ACK ff06c05043bf2324085dd0a1969239a9fd19eda5 [not substantive]

Participants:

  • bensig (contributor, support, 1 statements 2026-01-12 to 2026-01-12): ACKed an early commit
  • Crypt-iQ (member, objection, 7 statements 2026-01-20 to 2026-08-27): pointed out missing SetBlockFailureFlags and RecalculateBestHeader for out-of-chain InvalidateBlock (fixed); subsequently crACKed
  • frankomosh (contributor, objection, 4 statements 2026-01-23 to 2026-05-18): Concept ACKed overall approach; questioned logging format changes
  • stratospher (member, support, 1 statements 2026-03-24 to 2026-03-24): Concept ACKed as a nice cleanup and suggested combining block index iteration passes
  • sedited (member, support, 3 statements 2026-08-10 to 2026-10-09): pinged for rebase, left a naming nit, and ACKed

Files

36 lines under test/bench/ci.

  • src/validation.cpp +34/-22
  • test/functional/rpc_invalidateblock.py +22/-0
  • src/test/util/validation.cpp +4/-4
  • src/test/fuzz/block_index_tree.cpp +2/-2
  • src/test/util/validation.h +1/-1
  • src/validation.h +1/-1

Card

The short text the ranking pass compares across PRs.

This PR refactors invalid block handling by relocating block failure flag updates, best header recalculations, and logging from InvalidChainFound into InvalidBlockFound and InvalidateBlock. When ActivateBestChain encountered an invalid block, it previously performed duplicate block index traversals and emitted duplicate log messages by invoking the failure routines twice. The change eliminates the redundant iterations and renames the remaining helper to UpdateBestInvalid. The author cites review discussions in PR 31405 and PR 31533 highlighting this repeated work.

Data

dossier JSON · extract JSON · model ledger, generated 2026-10-10T01:23, confidence high, input hash 0a341468018705b3
thread read through 2026-10-09 (24 statements), head ff06c05043
labels: Validation

Categories

Validation

P3 · cleanup (ranking pass: Moved down: redundant work only in a rare invalid-block path, least consequential of the refactor group)

Cleans up invalid block processing in `ActivateBestChain` to eliminate redundant block index traversals and duplicated log messages. As noted in the description citing reviews in #31405 and #31533, moving failure flag propagation and best header recalculation out of `InvalidChainFound` avoids immediately executing the failure sequence twice upon encountering an invalid block.

Membership: Carries the Validation label and refactors invalid block handling and block index tree traversals in `src/validation.cpp`.

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