validation: Prevent duplicate logging and looping in invalid block handling

mzumsande commented at 4:59 pm on January 11, 2026: contributor

InvalidChainFound() is called from 3 spots:

from InvalidBlockFound()
from ActivateBestChain(), with the highest connectable block of the chain as an arg (after having called InvalidBlockFound() already for the block that failed in ConnectTip())
from InvalidateBlock() (rpc-only)

Most of the logic in InvalidChainFound() is not required for 2) and 3): In ActivateBestChain(), the previous call to InvalidChainFound() before via InvalidBlockFound did the flag updates, best header calculation and logging, so only m_best_invalid could need additional updating. In InvalidateBlock() (which doesn’t call InvalidBlockFound()), we do most of the accounting manually.

Therefore move of all logic but the m_best_invalid update into InvalidBlockFound(), avoiding duplicate logging and unnecessary work (the loops over the block index in SetBlockFailureFlags() and RecalculateBestHeader() were done twice before).

The second commit renames InvalidChainFound to UpdateBestInvalid to adjust to its remaining role.

This addresses past discussions about repeated work in InvalidateBlock() (https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2135717500) and ActivateBestChain() (https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2607189790)

DrahtBot added the label Validation on Jan 11, 2026

DrahtBot commented at 4:59 pm on January 11, 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/34254.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	frankomosh
Stale ACK	bensig

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

No conflicts as of last run.

bensig commented at 9:48 pm on January 12, 2026: contributor

ACK 73e0bab7adb0b16b2ac4d13c9d601a04b7184749

in src/validation.cpp:2017 in c816490e82 outdated

2029+        if (m_chainman.m_best_header != nullptr && m_chainman.m_best_header->GetAncestor(pindex->nHeight) == pindex) {
2030+            m_chainman.RecalculateBestHeader();
2031+        }
2032+
2033+        LogInfo("%s: invalid block=%s  height=%d  log2_work=%f  date=%s\n", __func__,
2034+        pindex->GetBlockHash().ToString(), pindex->nHeight,

Crypt-iQ commented at 8:49 pm on January 20, 2026:

nit: both LogInfo calls could be formatted so the lines here have spaces?

mzumsande commented at 6:24 am on February 1, 2026:

done - applied clang-format to the commit.

Crypt-iQ commented at 9:01 pm on January 20, 2026: contributor

This looks good, going to think about it more. I think the noticeable changes here are that:

~~InvalidateBlock won’t have warnings anymore since CheckForkWarningConditions is no longer called (this seems fine, the warnings mention falling out of consensus which is irrelevant here imo)~~
ActivateBestChainStep does not duplicate logging / work (also fine)

in src/validation.cpp:3737 in 73e0bab7ad outdated

3733@@ -3733,7 +3734,10 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
3734             }
3735         }
3736 
3737-        InvalidChainFound(to_mark_failed);
3738+        UpdateBestInvalid(to_mark_failed);

Crypt-iQ commented at 9:16 pm on January 20, 2026:

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?

mzumsande commented at 6:11 am on February 1, 2026:

good observation, I think that’s correct - it should be a preexisting issue though that is not changed by this PR. It should be possible to fix this by calling UpdateBestInvalid also earlier in the process (which we could do more easily after this PR, because unlike old InvalidChainFound, we no longer iterate over the block index).

On the other hand, m_best_invalid is only used for logging of out-of-consensus warnings, and I’m not sure if these logged warnings are really helpful if the consensus failure is self-inflicted through RPC. What do you think?

Crypt-iQ commented at 8:31 pm on February 2, 2026:

Brain-dump of what I think happens (haven’t written a test) with InvalidateBlock calling InvalidChainFound in master:

InvalidChainFound will:

update m_best_invalid
call SetBlockFailureFlags which updates children to set BLOCK_FAILED_CHILD
call RecalculateBestHeader
call CheckForkWarningConditions

to_mark_failed can either be in-chain or out-of-chain. If it’s in-chain, then it will be set to the last disconnected block. If it’s out-of-chain, it will be unchanged.

If to_mark_failed was in-chain:

m_best_invalid might be changed to to_mark_failed. Since to_mark_failed was changed, m_best_invalid might not change when it should (what I mentioned above in the previous comment).
SetBlockFailureFlags: Unnecessary because all of the children have been marked BLOCK_FAILED_CHILD in the loop.
RecalculateBestHeader: Unnecessary. If the best header was in-chain, then the loop already updated it. If the best header is out-of-chain, it was not invalidated.
CheckForkWarningConditions: Necessary because the tip definitely changed and m_best_invalid may or may not have changed. Either way, the fork condition may be hit.

Else (to_mark_failed is out-of-chain):

m_best_invalid might be changed to to_mark_failed. Even here, there may be children that could be marked as m_best_invalid instead of to_mark_failed, depending on what block the user calls InvalidateBlock on.
SetBlockFailureFlags: Necessary because if there are children, they need to be marked BLOCK_FAILED_CHILD.
RecalculateBestHeader: Necessary because the loop above did not run.
CheckForkWarningConditions: Necessary because m_best_invalid may have changed.

This PR changes things by only updating m_best_invalid and calling CheckForkWarningConditions. This is fine for the in-chain case, but for the case where to_mark_failed is out-of-chain, I think it’s missing SetBlockFailureFlags & RecalculateBestHeader?

It should be possible to fix this by calling UpdateBestInvalid also earlier in the process (which we could do more easily after this PR, because unlike old InvalidChainFound, we no longer iterate over the block index).

For the in-chain case of setting m_best_invalid improperly, we could call UpdateBestInvalid where it is and change the argument such that it’s either the old tip or to_mark_failed, depending on pindex_was_in_chain = true?

On the other hand, m_best_invalid is only used for logging of out-of-consensus warnings, and I’m not sure if these logged warnings are really helpful if the consensus failure is self-inflicted through RPC. What do you think?

If the logged warnings aren’t helpful, should they be removed? I don’t have a strong opinion here, I did find it a little confusing that m_best_invalid was being updated improperly in master, maybe a comment could suffice that it’s not being set to the actual best invalid? I didn’t mean to imply that this PR introduced that either btw.

in src/validation.cpp:3305 in 73e0bab7ad outdated

3301@@ -3301,7 +3302,7 @@ bool Chainstate::ActivateBestChainStep(BlockValidationState& state, CBlockIndex*
3302                 if (state.IsInvalid()) {
3303                     // The block violates a consensus rule.
3304                     if (state.GetResult() != BlockValidationResult::BLOCK_MUTATED) {
3305-                        InvalidChainFound(vpindexToConnect.front());
3306+                        UpdateBestInvalid(vpindexToConnect.front());

frankomosh commented at 11:34 am on January 23, 2026:

I think this update is already handled by InvalidBlockFound(), called from within ConnectTip(), so this second call is maybe unnecessary?

Crypt-iQ commented at 12:46 pm on January 23, 2026:

It’s slightly different, this second call might update m_best_invalid to the last descendant in the chain. vpindexToConnect is iterated in reverse order, and this call is only a no-op if the last descendant is the one that failed in ConnectTip.

frankomosh commented at 6:54 am on January 24, 2026:

Ah, I see and understand it now. Thanks for clarifying

frankomosh commented at 11:42 am on January 23, 2026: contributor

Concept ACK on the overall refactoring approach.

validation: Move most logic from InvalidChainFound into InvalidBlockFound

InvalidChainFound is called from 3 spots:
1. from InvalidBlockFound
2. from ActivateBestChain, with the highest connectable block of the chain
as an arg (after having called InvalidBlockFound already for the block that
actually failed)
3. from InvalidateBlock

Most of the logic in InvalidChainFound is not required for 2) and 3):
In ActivateBestChain, we already called InvalidChainFound before via
InvalidBlockFound, so only m_best_invalid needs updating.
In InvalidateBlock (which doesn't call InvalidBlockFound), we do most
of the accounting manually.
Therefore move of all logic but the m_best_invalid update into
InvalidBlockFound, avoiding duplicate logging and unnecessary work
(the loops over the block index in SetBlockFailureFlags and RecalculateBestHeader
were done twice before).

c4250249fe

validation: rename InvalidChainFound to UpdateBestInvalid

It's a more descriptive name after most of the logic was moved
into InvalidBlockFound.

f789d4fa16

mzumsande force-pushed on Feb 1, 2026

mzumsande commented at 6:25 am on February 1, 2026: contributor

73e0bab to f789d4f: applied clang-format

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

Code Coverage & Benchmarks

Reviews

Conflicts