validation: remove unused code in FindMostWorkChain

stratospher commented at 3:57 pm on March 20, 2026: contributor

recent PRs like #31405, #30666 mark all m_block_index descendants as invalid immediately whenever an invalid block is encountered in SetBlockFailureFlags. so by the time we reach FindMostWorkChain, the block in setBlockIndexCandidates already has BLOCK_FAILED_VALID set on it - not just on its ancestor. this means pindexTest = pindexFailed whenever fFailedChain fires, and the inner while (pindexTest != pindexFailed) loop body is never reached!

I think we can remove it but I’ve just replaced it with Assume in this PR for safety + good to document this invariant in case the code changes in future. (noticed by @ stickies-v in #32950 (review))

the second commit is unrelated and adds a unit test for the situation in https://github.com/bitcoin/bitcoin/issues/32173

DrahtBot added the label Validation on Mar 20, 2026

DrahtBot commented at 3:58 pm on March 20, 2026: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	optout21, fjahr, w0xlt, ryanofsky
Concept ACK	mzumsande
Stale ACK	stickies-v

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: Change CChain methods to use references, add 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.

stickies-v commented at 4:44 pm on March 20, 2026: contributor

Concept ACK

mzumsande commented at 2:25 pm on March 21, 2026: contributor

Concept ACK

w0xlt commented at 8:02 am on March 22, 2026: contributor

Concept ACK

in src/validation.cpp:3146 in a122fbd4c5

3142@@ -3146,8 +3143,7 @@ CBlockIndex* Chainstate::FindMostWorkChain()
3143                 // Remove the entire chain from the set.
3144                 while (pindexTest != pindexFailed) {
3145                     if (fFailedChain) {
3146-                        pindexFailed->nStatus |= BLOCK_FAILED_VALID;
3147-                        m_blockman.m_dirty_blockindex.insert(pindexFailed);
3148+                        Assume(pindexFailed->nStatus & BLOCK_FAILED_VALID);

stickies-v commented at 3:57 pm on March 23, 2026:

I think we can remove it but I’ve just replaced it with Assume in this PR for safety + good to document this invariant in case the code changes in future.

I think it’s bad practice to assert invariants in places that can’t cause breaking the invariant or are not affected by it. It creates confusion, and unnecessary friction for future work done on this code.

If my understanding is correct, this function is not affected by the invariant, so I suggest we remove the branch entirely. The invariant is checked in CheckBlockIndex() already. If there are other places where the invariant matters, we could assert it there.

stratospher commented at 6:08 pm on March 24, 2026:

I’ve removed it. It would be great if anyone testing could add this Assume and fuzz this with different block constellations. (I’ve fuzzed it for some hours using block_index_tree.cpp + adding 2 more cases for InvalidateBlock/ReconsiderBlock)

in src/test/validation_chainstatemanager_tests.cpp:650 in a122fbd4c5

645+    // genesis(0) <- ... <- block98 <- block99  <- block100
646+    //                              <- block99' <- block100'
647+    // by temporarily invalidating block99. the chain tip now falls to block98,
648+    // mine 2 new blocks on top of block 98 (block99' and block100') and then restore block99 and block 100.
649+    BlockValidationState state;
650+    chainstate.InvalidateBlock(state, block99);

stickies-v commented at 10:06 am on March 24, 2026:

nit: could BOOST_REQUIRE the return value:

0    BOOST_REQUIRE(chainstate.InvalidateBlock(state, block99));

in src/test/validation_chainstatemanager_tests.cpp:1 in a122fbd4c5 outdated

stickies-v commented at 10:51 am on March 24, 2026:

a122fbd4c5795630dbbf9c016c872a1a2588fef6 commit message nit: “throguh” typo, and we’re not really unit testing the functions, but rather their asymmetry. Suggested alternative:

test: add InvalidateBlock/ReconsiderBlock asymmetry test

InvalidateBlock propagates to all descendants in m_block_index, but ReconsiderBlock only clears blocks on the same chain as the reconsidered block (its ancestors and descendants). Blocks on sibling forks are not cleared. This asymmetry can lead to edge cases like in issue #32173 (fixed in 37bc207).

Add a unit test for this scenario to safeguard against future regressions.

stickies-v commented at 11:01 am on March 24, 2026:

commit message nit: “the newer validation commits” is a bit weird. My best effort rewrite:

validation: remove redundant marking in FindMostWorkChain

Since ed764ea, all descendants in m_block_index are required to be marked BLOCK_FAILED_VALID when an invalid block is encountered, as enforced by a CheckBlockIndex assert.

Remove the now-redundant marking in FindMostWorkChain’s inner loop, so it is only responsible for cleaning up setBlockIndexCandidates, not for modifying block validity state

stratospher commented at 6:08 pm on March 24, 2026:

thanks! taken both commit message suggestions.

fjahr commented at 11:30 am on March 24, 2026: contributor

Concept ACK

stratospher force-pushed on Mar 24, 2026

in src/validation.cpp:3145 in e54e78207a

3141@@ -3145,10 +3142,7 @@ CBlockIndex* Chainstate::FindMostWorkChain()
3142                 CBlockIndex *pindexFailed = pindexNew;
3143                 // Remove the entire chain from the set.
3144                 while (pindexTest != pindexFailed) {
3145-                    if (fFailedChain) {
3146-                        pindexFailed->nStatus |= BLOCK_FAILED_VALID;
3147-                        m_blockman.m_dirty_blockindex.insert(pindexFailed);
3148-                    } else if (fMissingData) {
3149+                    if (fMissingData) {

stickies-v commented at 1:00 pm on March 26, 2026:

I think for a pruned node it’s possible that both fFailedChain and fMissingData are true? In which case this would be behaviour change that a failed block with missing data is now added to m_blocks_unlinked when previously it wouldn’t

0                    if (fMissingData && !fFailedChain) {

mzumsande commented at 10:00 am on March 27, 2026:

i wonder if the existing behavior is better though? m_blocks_unlinked and validity are independent from each other in other spots - for example invalidating a block doesn’t remove its descendants from m_blocks_unlinked. Maybe in some weird scenario with invalidateblock/resonsiderblock and re-receiving previously pruned blocks with getblockfrompeer we’d need the block in the set?

Although I think that these scenarios are very unrealistic and not relevant to practice at all.

stickies-v commented at 11:34 am on March 27, 2026:

I’ll need to dive into m_blocks_unlinked a bit more, but I think since this code is already hard enough to reason about, it probably makes sense to keep this PR free of behaviour change, and then m_blocks_unlinked behaviour can be improved in another, if it makes sense?

fjahr commented at 3:51 pm on April 2, 2026:

I would prefer if we would apply @stickies-v ’s change and merge this without the behavior change. I also feel like I have to spend a little more time on this to really get the full picture but at least this line in CheckBlockIndex() seems like a pretty clear indication that we shouldn’t do this:

0        if (!(pindex->nStatus & BLOCK_HAVE_DATA)) assert(!foundInUnlinked); // Can't be in m_blocks_unlinked if we don't HAVE_DATA

Even if in practice this may never actually have real world implications as @mzumsande says, it feels like something that we could run into in a test case scenario or something like that.

stickies-v approved

stickies-v commented at 6:59 pm on March 26, 2026: contributor

ACK e54e78207a351ca53eb52a0507890fce584c7fb3 modulo the fMissingData regression.

For this change to be safe, every BLOCK_FAILED_VALID block must always have all its descendants also marked BLOCK_FAILED_VALID (when not holding cs_main).

I can see 4 places (other than FindMostWorkChain) where a CBlockIndex::nStatus can be marked as BLOCK_FAILED_VALID, and 1 place where it is cleared. I think if we can establish that those operations cannot break the invariant, then this change is safe.

Setting BLOCK_FAILED_VALID:

LoadBlockIndex
SetBlockFailureFlags
InvalidBlockFound
InvalidateBlock

Clearing BLOCK_FAILED_VALID: 5) ResetBlockFailureFlags

For (1)-(3):

is init-only, and propagates BLOCK_FAILED_VALID from parent to child during a height-ordered iteration, ensuring the flag cascades across all generations of descendants. Safe.
marks all the descendants of CBlockIndex* invalid_block (but not invalid_block itself) as BLOCK_FAILED_VALID. Safe.
marks pindex as well as all its descendants through InvalidChainFound->SetBlockFailureFlags. Safe.

InvalidateBlock (4) has more complex logic, but the invariant is maintained by: marking each disconnected tip, marking out-of-chain descendants via the highpow_outofchain_headers loop (which covers all descendants since they necessarily have more chain work than pindex->pprev), and calling SetBlockFailureFlags at the end via InvalidChainFound. Safe.

ResetBlockFailureFlags (5) clears the flag, so could potentially violate the invariant by clearing a descendant without clearing the ancestor. However, it only clears blocks on the direct ancestor-descendant line of pindex. if a block remains BLOCK_FAILED_VALID after the operation, none of its descendants were cleared either. Safe.

DrahtBot requested review from fjahr on Mar 26, 2026

DrahtBot requested review from mzumsande on Mar 26, 2026

in src/test/validation_chainstatemanager_tests.cpp:683 in e54e78207a

678+        BOOST_CHECK(!(fork_block100->nStatus & BLOCK_FAILED_VALID));
679+        BOOST_CHECK(!(fork_block99->nStatus & BLOCK_FAILED_VALID));
680+    }
681+
682+    // Invalidate block98
683+    chainstate.InvalidateBlock(state, block98);

fjahr commented at 3:25 pm on April 2, 2026:

nit: Could have added the same BOOST_REQUIRE as below the first InvalidateBlock() call

fjahr commented at 3:52 pm on April 2, 2026: contributor

Code review ACK modulo @stickies-v ’s suggested change

DrahtBot requested review from fjahr on Apr 2, 2026

in src/test/validation_chainstatemanager_tests.cpp:628 in e54e78207a outdated

624@@ -625,6 +625,92 @@ BOOST_FIXTURE_TEST_CASE(loadblockindex_invalid_descendants, TestChain100Setup)
625     BOOST_CHECK(child->nStatus & BLOCK_FAILED_VALID);
626 }
627 
628+BOOST_FIXTURE_TEST_CASE(invalidate_block_and_reconsider_fork, TestChain100Setup)

ryanofsky commented at 3:55 pm on April 7, 2026:

In commit “test: add InvalidateBlock/ReconsiderBlock asymmetry test” (e54e78207a351ca53eb52a0507890fce584c7fb3)

Since this test has a lot of setup, i feel like it could benefit from a description of what is trying to test. Maybe: // “Verify that ReconsiderBlock clears failure flags for the target block, its ancestors, and descendants, but not for sibling forks that diverge from a shared ancestor.”

stratospher commented at 6:24 pm on April 7, 2026:

right, taken!

ryanofsky approved

ryanofsky commented at 4:13 pm on April 7, 2026: contributor

Code review ACK e54e78207a351ca53eb52a0507890fce584c7fb3 with the assumption that blocks are marked BLOCK_FAILED_VALID if their ancestors are, which has to be true for CheckBlockIndex to pass. I didn’t repeat the great analysis from stickies #34884#pullrequestreview-4016676603 about why this is the case, but that’s reassuring, too.

On the question of whether the condition for adding blocks to m_blocks_unlinked should be fMissingData or fMissingData && !fFailedChain #34884 (review), I agree with martin that ignoring fFailedChain makes the most sense intuitively, but also agree with stickies and fjahr that keeping the !fFailedChain check would be most conservative approach.

I do think if !fFailedChain is going to be dropped, this should be explained in the commit message. And if it is going to be kept, this should be explained in a code comment.

Separately I think switching from while loops to for loops here could be minor readability improvement for a followup commit, but not important at all.

 0--- a/src/validation.cpp
 1+++ b/src/validation.cpp
 2@@ -3123,9 +3123,8 @@ CBlockIndex* Chainstate::FindMostWorkChain()
 3 
 4         // Check whether all blocks on the path between the currently active chain and the candidate are valid.
 5         // Just going until the active chain is an optimization, as we know all blocks in it are valid already.
 6-        CBlockIndex *pindexTest = pindexNew;
 7         bool fInvalidAncestor = false;
 8-        while (pindexTest && !m_chain.Contains(pindexTest)) {
 9+        for (CBlockIndex *pindexTest = pindexNew; pindexTest && !m_chain.Contains(pindexTest); pindexTest = pindexTest->pprev) {
10             assert(pindexTest->HaveNumChainTxs() || pindexTest->nHeight == 0);
11 
12             // Pruned nodes may have entries in setBlockIndexCandidates for
13@@ -3139,9 +3138,8 @@ CBlockIndex* Chainstate::FindMostWorkChain()
14                 if (fFailedChain && (m_chainman.m_best_invalid == nullptr || pindexNew->nChainWork > m_chainman.m_best_invalid->nChainWork)) {
15                     m_chainman.m_best_invalid = pindexNew;
16                 }
17-                CBlockIndex *pindexFailed = pindexNew;
18                 // Remove the entire chain from the set.
19-                while (pindexTest != pindexFailed) {
20+                for (CBlockIndex *pindexFailed = pindexNew; pindexTest != pindexFailed; pindexFailed = pindexFailed->pprev) {
21                     if (fMissingData) {
22                         // If we're missing data, then add back to m_blocks_unlinked,
23                         // so that if the block arrives in the future we can try adding
24@@ -3150,13 +3148,11 @@ CBlockIndex* Chainstate::FindMostWorkChain()
25                             std::make_pair(pindexFailed->pprev, pindexFailed));
26                     }
27                     setBlockIndexCandidates.erase(pindexFailed);
28-                    pindexFailed = pindexFailed->pprev;
29                 }
30                 setBlockIndexCandidates.erase(pindexTest);
31                 fInvalidAncestor = true;
32                 break;
33             }
34-            pindexTest = pindexTest->pprev;
35         }
36         if (!fInvalidAncestor)
37             return pindexNew;

stratospher force-pushed on Apr 7, 2026

stratospher commented at 6:23 pm on April 7, 2026: contributor

Thank you for the reviews! I’ve taken all of your suggestions and went with the if (fMissingData && !fFailedChain) conservative approach in FindMostWorkChain. diff link

fjahr commented at 7:20 pm on April 7, 2026: contributor

Code review ACK 90bd676c208b364985763724dbbfa77835b85861

DrahtBot requested review from ryanofsky on Apr 7, 2026

DrahtBot requested review from stickies-v on Apr 7, 2026

in src/validation.cpp:3142 in 90bd676c20

3137@@ -3139,9 +3138,8 @@ CBlockIndex* Chainstate::FindMostWorkChain()
3138                 if (fFailedChain && (m_chainman.m_best_invalid == nullptr || pindexNew->nChainWork > m_chainman.m_best_invalid->nChainWork)) {
3139                     m_chainman.m_best_invalid = pindexNew;
3140                 }
3141-                CBlockIndex *pindexFailed = pindexNew;
3142                 // Remove the entire chain from the set.
3143-                while (pindexTest != pindexFailed) {
3144+                for (CBlockIndex *pindexFailed = pindexNew; pindexTest != pindexFailed; pindexFailed = pindexFailed->pprev) {

optout21 commented at 9:00 am on April 8, 2026:

90bd676 refactor: use for loops in FindMostWorkChain:

Nit: Since in this for loop pindexFailed is the loop variable, for readability I suggest reverting the end condition such that the loop variable is first, to: pindexFailed != pindexTest. (Probably this condition taken unchanged from the original while loop, but since the whole line is changed to for, I think it’s better to change it.)

stratospher commented at 5:50 pm on April 8, 2026:

thanks! taken.

in src/validation.cpp:3145 in 56689317b8 outdated

3141@@ -3146,10 +3142,7 @@ CBlockIndex* Chainstate::FindMostWorkChain()
3142                 CBlockIndex *pindexFailed = pindexNew;
3143                 // Remove the entire chain from the set.
3144                 while (pindexTest != pindexFailed) {
3145-                    if (fFailedChain) {
3146-                        pindexFailed->nStatus |= BLOCK_FAILED_VALID;
3147-                        m_blockman.m_dirty_blockindex.insert(pindexFailed);
3148-                    } else if (fMissingData) {
3149+                    if (fMissingData && !fFailedChain) {

optout21 commented at 9:10 am on April 8, 2026:

5668931 validation: remove redundant marking in FindMostWorkChain:

Technically this condition is redundant: the outer if condition is (fFailedChain || fMissingData), so at least one of the two is true. So (!fFailedChain) implies (fMissingData). Therefore the condition (!fFailedChain) would be sufficient. Maybe the form below is slightly more optimal execution-wise, but not necessarily better from readability/maintainability, so I don’t have a strong opinion:

0                    if (!fFailedChain) { // implies fMissingData is true

ryanofsky commented at 3:54 pm on April 8, 2026:

In commit “validation: remove redundant marking in FindMostWorkChain” (56689317b87d4c457e3807ff0bcc77cf80832360)

re: #34884 (review)

Maybe the form below is slightly more optimal execution-wise, but not necessarily better from readability/maintainability

Yeah that would be confusing because missing data and failed states should be orthogonal and the most obvious implementation of this code would not reference fFailedChain at all.

It’s also confusing how the code comment here does not mention the !fFailedChain condition. It only says “If we’re missing data, then add back to m_blocks_unlinked, so that if the block arrives in the future we can try adding to setBlockIndexCandidates again.”

IMO that comment needs an extra sentence like “Do not add it when fFailedChain is true because it should not be necessary if the chain is invalid.” to explain why !fFailedChain is present here and that this is a “should not be necessary” check not a “should not be added” check

Comment does not need to get into the “very unrealistic and not relevant to practice” scenarios where reconsiderblock is used where perhaps these blocks should be added to m_blocks_unlinked. It can just say why code is not adding them right now.

stratospher commented at 5:55 pm on April 8, 2026:

added a comment along those lines in the latest push.

ryanofsky commented at 9:14 pm on April 8, 2026:

re: #34884 (review)

added a comment along those lines in the latest push.

Thanks! The comment is at least in sync with the code now.

in src/validation.cpp:3127 in 90bd676c20 outdated

3122@@ -3123,9 +3123,8 @@ CBlockIndex* Chainstate::FindMostWorkChain()
3123 
3124         // Check whether all blocks on the path between the currently active chain and the candidate are valid.
3125         // Just going until the active chain is an optimization, as we know all blocks in it are valid already.
3126-        CBlockIndex *pindexTest = pindexNew;
3127         bool fInvalidAncestor = false;
3128-        while (pindexTest && !m_chain.Contains(pindexTest)) {
3129+        for (CBlockIndex *pindexTest = pindexNew; pindexTest && !m_chain.Contains(pindexTest); pindexTest = pindexTest->pprev) {

optout21 commented at 9:11 am on April 8, 2026:

90bd676 refactor: use for loops in FindMostWorkChain:

Nit: could use auto here?

stratospher commented at 5:57 pm on April 8, 2026:

personally like explicit types, so keeping it as it is. but thanks!

optout21 commented at 9:18 am on April 8, 2026: contributor

crACK ba01b00d45a062d64f56aa201b5940327c9ada85

ReACK following minor review changes.

Prev: crACK 90bd676c208b364985763724dbbfa77835b85861 Left some minor comments. The change removes a few lines that became unnecessary, making the code more consistent. The change scope is reasonably reduced, to validate by code review and the test harness. The change also includes an additional test and a minor refactor.

ryanofsky approved

ryanofsky commented at 4:03 pm on April 8, 2026: contributor

Code review ACK 90bd676c208b364985763724dbbfa77835b85861. But I do think the (fMissingData && !fFailedChain) check is confusing with the current comment only explaining fMissingData not !fFailedChain. Left a suggestion below to extend the comment. Also agree with optout’s suggestion #34884 (review) to write the for-loop in a more natural way

validation: remove redundant marking in FindMostWorkChain

since ed764ea, all descendants in m_block_index are required to be
marked BLOCK_FAILED_VALID when an invalid block is encountered,
as enforced by a CheckBlockIndex assert.

remove the now-redundant marking in FindMostWorkChain's inner loop,
so it is only responsible for cleaning up setBlockIndexCandidates,
not for modifying block validity state

1b0b3e2c2c

test: add InvalidateBlock/ReconsiderBlock asymmetry test

InvalidateBlock propagates to all descendants in m_block_index,
but ReconsiderBlock only clears blocks on the same chain as the
reconsidered block (its direct ancestors and descendants).
Blocks on sibling forks are not cleared. This asymmetry can
lead to edge cases like in issue #32173 (fixed in 37bc207).

Add a unit test for this scenario to safeguard against future
regressions.

aa0eef735b

refactor: use for loops in FindMostWorkChain

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

ba01b00d45

stratospher force-pushed on Apr 8, 2026

fjahr commented at 6:08 pm on April 8, 2026: contributor

re-ACK ba01b00d45a062d64f56aa201b5940327c9ada85

DrahtBot requested review from ryanofsky on Apr 8, 2026

DrahtBot requested review from optout21 on Apr 8, 2026

w0xlt commented at 7:32 pm on April 8, 2026: contributor

ACK ba01b00d45a062d64f56aa201b5940327c9ada85

ryanofsky approved

ryanofsky commented at 9:21 pm on April 8, 2026: contributor

Code review ACK ba01b00d45a062d64f56aa201b5940327c9ada85, just tweaking comment and for loop condition since last review.

Not important, but IMO it would still be good if missing data comment

https://github.com/bitcoin/bitcoin/blob/ba01b00d45a062d64f56aa201b5940327c9ada85/src/validation.cpp#L3144-L3146

could say the reason why invalid blocks are not added. That it’s not because it would be wrong to add them, but just because it’s not necessary to add them in the normal case where invalidate/reconsider haven’t been called.

(My understanding is still that it would be more correct to add them and that there are corner cases #34884 (review) where they should be added.)

ryanofsky assigned ryanofsky on Apr 9, 2026

ryanofsky merged this on Apr 9, 2026

ryanofsky closed this on Apr 9, 2026

validation: remove unused code in FindMostWorkChain #34884

Reviews

Conflicts