validation: remove BLOCK_FAILED

stratospher commented at 3:04 pm on July 11, 2025: contributor

even though we have a distinction between BLOCK_FAILED_VALID and BLOCK_FAILED_CHILD in the codebase, we don’t use it for anything. Whenever we check for BlockStatus, we use BLOCK_FAILED_MASK which encompasses both of them.

Since there is no functional difference between BLOCK_FAILED_VALID and BLOCK_FAILED_CHILD and it’s added code complexity to correctly categorise them (ex: #31405 (review), #16856 (comment)), we could just remove it.

Looking for conceptual feedback on whether it’s better to improve handling of BLOCK_FAILED_CHILD in the codebase or remove BLOCK_FAILED_CHILD.

Of less relevance, but it would also fix a reconsiderblock crash that could happen in the situation mentioned in #32173 (comment)

Similar attempt in the past in #16856 (comment)

validation: don't update BLOCK_FAILED_VALID to BLOCK_FAILED_CHILD in InvalidateBlock

- there is no functional difference between BLOCK_FAILED_VALID and BLOCK_FAILED_CHILD
and it's unnecessary code complexity to correctly categorise them.

d121579abc

DrahtBot added the label Validation on Jul 11, 2025

DrahtBot commented at 3:04 pm on July 11, 2025: 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/32950.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	TheCharlatan, Prabhat1308, mzumsande, yuvicc, sipa, stringintech
Approach ACK	stickies-v
Stale ACK	naiyoma

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32843 (validation: invalid block handling followups by mzumsande)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

“check block_98, block_99, block_100 is marked as BLOCK_FAILED_VALID after reloading from disk” -> “check block_98, block_99, block_100 are marked as BLOCK_FAILED_VALID after reloading from disk” [subject-verb agreement: plural subject requires “are”]

drahtbot_id_5_m

TheCharlatan commented at 3:32 pm on July 11, 2025: contributor

Concept ACK

DrahtBot added the label CI failed on Jul 11, 2025

Prabhat1308 commented at 3:38 pm on July 12, 2025: contributor

Concept ACK

I think removing is a better option. The only weak argument that we could have for not removing is that BLOCK_FAILED_VALID right now marks the start of the invalid block chain and is better for debugging manually. However , the burden of maintaining this distinction seems too high for debugging purposes with no actual benefit functionally.

in src/node/blockstorage.cpp:467 in 06db700a13

462@@ -463,8 +463,10 @@ bool BlockManager::LoadBlockIndex(const std::optional<uint256>& snapshot_blockha
463                 pindex->m_chain_tx_count = pindex->nTx;
464             }
465         }
466-        if (!(pindex->nStatus & BLOCK_FAILED_MASK) && pindex->pprev && (pindex->pprev->nStatus & BLOCK_FAILED_MASK)) {
467-            pindex->nStatus |= BLOCK_FAILED_CHILD;
468+        if (!(pindex->nStatus & BLOCK_FAILED_VALID) && pindex->pprev && (pindex->pprev->nStatus & BLOCK_FAILED_VALID)) {
469+            // if BLOCK_FAILED_CHILD already exists in disk, clear it

maflcko commented at 10:22 am on July 14, 2025:

exists in disk -> exists on disk [“in disk” is nonstandard; use “on disk”]

An alternative wording could be “if … was persisted, clear it”

stickies-v commented at 9:54 pm on July 17, 2025:

Or:

// BLOCK_FAILED_CHILD is deprecated, but may still exist on disk. Replace it with BLOCK_FAILED_VALID.

stratospher commented at 10:47 am on October 1, 2025:

done.

mzumsande commented at 9:59 pm on July 15, 2025: contributor

Concept ACK

naiyoma commented at 6:58 am on July 17, 2025: contributor

Concept ACK,

Wondering if the CI failure is related to this issue. ->https://github.com/bitcoin/bitcoin/issues/32855

yuvicc commented at 1:21 pm on July 17, 2025: contributor

Concept ACK

The CI failure looks like due to #32855

stickies-v commented at 9:55 pm on July 17, 2025: contributor

Concept ACK for removing it.

naiyoma commented at 8:59 pm on August 7, 2025: contributor

TestedACK 06db700a1315bb655ac7fa12578c626990a22ea5

                  Block 2 (valid)
                    /         \
(valid)Block height 3         Block height 3 (BLOCK_FAILED_CHILD)
                    |         |
(valid) Block height 4        Block height 4(BLOCK_FAILED_CHILD)
                    |         |

Before removing BLOCK_FAILED_CHILD,this test(https://github.com/bitcoin/bitcoin/issues/32173#issue-2960274990) would fail at: self.log.info(self.nodes[0].reconsiderblock(res["bestblock"])) (Reconsidering block at height 3 — which has BLOCK_FAILED_CHILD) This failure happened because:

This check fails, block 3 has BLOCK_FAILED_CHILD (not BLOCK_FAILED_VALID).

and so in this assertion -> https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L5363

pindex->nStatus & BLOCK_FAILED_MASK) != 0.

after removing BLOCK_FAILED_CHILD:

block 3 , status is BLOCK_FAILED_VALID, and pindexFirstInvalid is set correctly.

DrahtBot requested review from Prabhat1308 on Aug 7, 2025

DrahtBot requested review from TheCharlatan on Aug 7, 2025

DrahtBot requested review from mzumsande on Aug 7, 2025

DrahtBot requested review from yuvicc on Aug 7, 2025

DrahtBot requested review from stickies-v on Aug 7, 2025

DrahtBot removed the label CI failed on Aug 20, 2025

in src/validation.cpp:3660 in ca2421d527 outdated

3656@@ -3657,7 +3657,7 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
3657     assert(pindex);
3658     if (pindex->nHeight == 0) return false;
3659 
3660-    CBlockIndex* to_mark_failed = pindex;
3661+    CBlockIndex* invalid_walk_tip = pindex;

mzumsande commented at 6:18 pm on September 24, 2025:

commit message ca2421d52792b32754f1801492822b999839ec3a: “since the previous commit removes BLOCK_FAILED_CHILD” - only the following commit removes it. Or do you mean “any special logic for BLOCK_FAILED_CHILD”?

stratospher commented at 10:47 am on October 1, 2025:

ah I see how it could be confusing, clarified it to “since the previous commit removes BLOCK_FAILED_CHILD usage in InvalidateBlock”

stickies-v commented at 9:34 am on October 6, 2025:

In 507b937eb39866c182b8a8939d2a71a80618f398: I think the invalid_walk_tip can (and if so, should) be kept local to the while (true) loop. Once that loop is finished, invalid_walk_tip should be equivalent to pindex? This better isolates the tip-rewinding logic.

Suggested diff (that also uses {disconnected,new}_tip and moved code closer to where it’s used for readability):

  0diff --git a/src/validation.cpp b/src/validation.cpp
  1index b8200eb1b6..013ae632e2 100644
  2--- a/src/validation.cpp
  3+++ b/src/validation.cpp
  4@@ -3657,10 +3657,6 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
  5     assert(pindex);
  6     if (pindex->nHeight == 0) return false;
  7 
  8-    CBlockIndex* invalid_walk_tip = pindex;
  9-    bool pindex_was_in_chain = false;
 10-    int disconnected = 0;
 11-
 12     // We do not allow ActivateBestChain() to run while InvalidateBlock() is
 13     // running, as that could cause the tip to change while we disconnect
 14     // blocks.
 15@@ -3692,6 +3688,9 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
 16         }
 17     }
 18 
 19+    bool pindex_was_in_chain = false;
 20+    int disconnected = 0;
 21+
 22     // Disconnect (descendants of) pindex, and mark them invalid.
 23     while (true) {
 24         if (m_chainman.m_interrupt) break;
 25@@ -3705,8 +3704,8 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
 26         LOCK(MempoolMutex());
 27         if (!m_chain.Contains(pindex)) break;
 28         pindex_was_in_chain = true;
 29-        invalid_walk_tip = m_chain.Tip();
 30 
 31+        CBlockIndex* disconnected_tip{m_chain.Tip()};
 32         // ActivateBestChain considers blocks already in m_chain
 33         // unconditionally valid already, so force disconnect away from it.
 34         DisconnectedBlockTransactions disconnectpool{MAX_DISCONNECTED_TX_POOL_BYTES};
 35@@ -3718,32 +3717,33 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
 36         // keeping the mempool up to date is probably futile anyway).
 37         MaybeUpdateMempoolForReorg(disconnectpool, /* fAddToMempool = */ (++disconnected <= 10) && ret);
 38         if (!ret) return false;
 39-        assert(invalid_walk_tip->pprev == m_chain.Tip());
 40+        CBlockIndex* new_tip{m_chain.Tip()};
 41+        assert(disconnected_tip->pprev == new_tip);
 42 
 43         // We immediately mark the disconnected blocks as invalid.
 44         // This prevents a case where pruned nodes may fail to invalidateblock
 45         // and be left unable to start as they have no tip candidates (as there
 46         // are no blocks that meet the "have data and are not invalid per
 47         // nStatus" criteria for inclusion in setBlockIndexCandidates).
 48-        invalid_walk_tip->nStatus |= BLOCK_FAILED_VALID;
 49-        m_blockman.m_dirty_blockindex.insert(invalid_walk_tip);
 50-        setBlockIndexCandidates.erase(invalid_walk_tip);
 51-        setBlockIndexCandidates.insert(invalid_walk_tip->pprev);
 52+        disconnected_tip->nStatus |= BLOCK_FAILED_VALID;
 53+        m_blockman.m_dirty_blockindex.insert(disconnected_tip);
 54+        setBlockIndexCandidates.erase(disconnected_tip);
 55+        setBlockIndexCandidates.insert(new_tip);
 56 
 57         // Mark out-of-chain descendants of the invalidated block as invalid
 58         // Add any equal or more work headers that are not invalidated to setBlockIndexCandidates
 59         // Recalculate m_best_header if it became invalid.
 60-        auto candidate_it = highpow_outofchain_headers.lower_bound(invalid_walk_tip->pprev->nChainWork);
 61+        auto candidate_it = highpow_outofchain_headers.lower_bound(new_tip->nChainWork);
 62 
 63-        const bool best_header_needs_update{m_chainman.m_best_header->GetAncestor(invalid_walk_tip->nHeight) == invalid_walk_tip};
 64+        const bool best_header_needs_update{m_chainman.m_best_header->GetAncestor(disconnected_tip->nHeight) == disconnected_tip};
 65         if (best_header_needs_update) {
 66             // pprev is definitely still valid at this point, but there may be better ones
 67-            m_chainman.m_best_header = invalid_walk_tip->pprev;
 68+            m_chainman.m_best_header = new_tip;
 69         }
 70 
 71         while (candidate_it != highpow_outofchain_headers.end()) {
 72             CBlockIndex* candidate{candidate_it->second};
 73-            if (candidate->GetAncestor(invalid_walk_tip->nHeight) == invalid_walk_tip) {
 74+            if (candidate->GetAncestor(disconnected_tip->nHeight) == disconnected_tip) {
 75                 // Children of failed blocks are marked as BLOCK_FAILED_VALID.
 76                 candidate->nStatus |= BLOCK_FAILED_VALID;
 77                 m_blockman.m_dirty_blockindex.insert(candidate);
 78@@ -3752,7 +3752,7 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
 79                 candidate_it = highpow_outofchain_headers.erase(candidate_it);
 80                 continue;
 81             }
 82-            if (!CBlockIndexWorkComparator()(candidate, invalid_walk_tip->pprev) &&
 83+            if (!CBlockIndexWorkComparator()(candidate, new_tip) &&
 84                 candidate->IsValid(BLOCK_VALID_TRANSACTIONS) &&
 85                 candidate->HaveNumChainTxs()) {
 86                 setBlockIndexCandidates.insert(candidate);
 87@@ -3771,7 +3771,7 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
 88 
 89     {
 90         LOCK(cs_main);
 91-        if (m_chain.Contains(invalid_walk_tip)) {
 92+        if (m_chain.Contains(pindex)) {
 93             // If the to-be-marked invalid block is in the active chain, something is interfering and we can't proceed.
 94             return false;
 95         }
 96@@ -3796,7 +3796,7 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
 97             }
 98         }
 99 
100-        InvalidChainFound(invalid_walk_tip);
101+        InvalidChainFound(pindex);
102     }
103 
104     // Only notify about a new block tip if the active chain was modified.
105@@ -3810,8 +3810,8 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
106         // changes.
107         (void)m_chainman.GetNotifications().blockTip(
108             /*state=*/GetSynchronizationState(m_chainman.IsInitialBlockDownload(), m_chainman.m_blockman.m_blockfiles_indexed),
109-            /*index=*/*invalid_walk_tip->pprev,
110-            /*verification_progress=*/WITH_LOCK(m_chainman.GetMutex(), return m_chainman.GuessVerificationProgress(invalid_walk_tip->pprev)));
111+            /*index=*/*pindex->pprev,
112+            /*verification_progress=*/WITH_LOCK(m_chainman.GetMutex(), return m_chainman.GuessVerificationProgress(pindex->pprev)));
113 
114         // Fire ActiveTipChange now for the current chain tip to make sure clients are notified.
115         // ActivateBestChain may call this as well, but not necessarily.

stratospher commented at 3:21 pm on October 8, 2025:

nice! I like the clearer variable names. used this diff in f284834 and added you as a coauthor.

1 difference with using pindex instead of to_mark_failed in the last couple of lines - (ex: InvalidChainFound(to_mark_failed);) is that:

to_mark_failed used to track the last disconnected block index
pindex is the block index we call invalidateblock on

both of them are the same in a normal execution flow. if invalidateblcok gets interrupted, they’d be different. but the remaining code won’t be called anyways. so i don’t think tracking the last disconnected block index adds any value now and this is better.

stringintech commented at 10:57 am on November 4, 2025:

but the remaining code won’t be called anyways

But previously we would call InvalidChainFound() on the last disconnected tip (and the rest of the code after the m_chain.Contains(to_mark_failed) check) even if we were interrupted; right?

in src/chain.h:127 in 4200ecf662 outdated

123@@ -124,7 +124,8 @@ enum BlockStatus : uint32_t {
124 
125     BLOCK_FAILED_VALID       =   32, //!< stage after last reached validness failed
126     BLOCK_FAILED_CHILD       =   64, //!< Unused flag that was previously set when descending from failed block
127-    BLOCK_FAILED_MASK        =   BLOCK_FAILED_VALID | BLOCK_FAILED_CHILD,
128+    BLOCK_FAILED_MASK        =   BLOCK_FAILED_VALID | BLOCK_FAILED_CHILD, //!< Unused flag that was previously used as

mzumsande commented at 6:26 pm on September 24, 2025:

do we need to keep BLOCK_FAILED_MASK in chain.h after it’s no longer used? After all, it’s only used as a mask (unlike BLOCK_FAILED_CHILD).

stratospher commented at 10:47 am on October 1, 2025:

good point. I’ve removed it since BLOCK_FAILED_MASK = 96 is never written to disk but only used while reading BlockStatus values. if 96 exists on disk, it is the responsibility of BLOCK_FAILED_VALID and BLOCK_FAILED_CHILD and we have dealt with those cases.

in src/validation.cpp:5368 in 4200ecf662 outdated

5364@@ -5365,9 +5365,9 @@ void ChainstateManager::CheckBlockIndex() const
5365         if ((pindex->nStatus & BLOCK_VALID_MASK) >= BLOCK_VALID_SCRIPTS) assert(pindexFirstNotScriptsValid == nullptr); // SCRIPTS valid implies all parents are SCRIPTS valid
5366         if (pindexFirstInvalid == nullptr) {
5367             // Checks for not-invalid blocks.
5368-            assert((pindex->nStatus & BLOCK_FAILED_MASK) == 0); // The failed mask cannot be set for blocks without invalid parents.
5369+            assert((pindex->nStatus & BLOCK_FAILED_VALID) == 0); // The failed mask cannot be set for blocks without invalid parents.

mzumsande commented at 6:27 pm on September 24, 2025:

comment still mentions a “mask”

stratospher commented at 10:47 am on October 1, 2025:

done.

mzumsande commented at 6:34 pm on September 24, 2025: contributor

Looks good - just some minor comments.

DrahtBot requested review from mzumsande on Sep 24, 2025

stratospher force-pushed on Oct 1, 2025

sipa commented at 11:58 am on October 1, 2025: member

Concept ACK

in src/node/blockstorage.cpp:466 in 8e6c195396 outdated

462@@ -463,7 +463,7 @@ bool BlockManager::LoadBlockIndex(const std::optional<uint256>& snapshot_blockha
463                 pindex->m_chain_tx_count = pindex->nTx;
464             }
465         }
466-        if (!(pindex->nStatus & BLOCK_FAILED_MASK) && pindex->pprev && (pindex->pprev->nStatus & BLOCK_FAILED_MASK)) {
467+        if (!(pindex->nStatus & BLOCK_FAILED_VALID) && pindex->pprev && (pindex->pprev->nStatus & BLOCK_FAILED_VALID)) {

stickies-v commented at 9:29 am on October 6, 2025:

In 8e6c195396304e3c66471e208b1cd59eea02c11c:

This logic seems to deal with some kind of block index corruption (e.g. unclean shutdown) where not all children of an invalid block have been marked as invalid on-disk.

Let’s assume a chain of blocks A -> B -> C. Do we have strong guarantees that it’s impossible that:

A is marked BLOCK_FAILED_VALID
B is marked BLOCK_FAILED_CHILD
C is not marked invalid

In the above scenario, the blockstorage.cpp logic on master would mark C as BLOCK_FAILED_CHILD, whereas on this PR it wouldn’t be marked as BLOCK_FAILED_VALID.

Perhaps useful to write a test around this?

stratospher commented at 3:05 pm on October 8, 2025:

In the above scenario, the blockstorage.cpp logic on master would mark C as BLOCK_FAILED_CHILD, whereas on this PR it wouldn’t be marked as BLOCK_FAILED_VALID

No, this doesn’t change logic and both master and this PR would mark C as BLOCK_FAILED_VALID.

short answer: because we’re iterating through block indices in increasing order of heights:

we’d first mark B as BLOCK_FAILED_VALID (from BLOCK_FAILED_CHILD) A -> B -> C would then look like:

A is marked BLOCK_FAILED_VALID
B is marked BLOCK_FAILED_VALID
C is not marked invalid

we can enter inside the if condition now(C isn’t BLOCK_FAILED_VALID but it’s parent is BLOCK_FAILED_VALID) and mark C as BLOCK_FAILED_VALID.

Let’s assume a chain of blocks A -> B -> C. Do we have strong guarantees that it’s impossible that:

A is marked BLOCK_FAILED_VALID B is marked BLOCK_FAILED_CHILD C is not marked invalid

I don’t think this would happen naturally. If we receive a block header which is based on top of an invalid block header in our block index, AcceptBlockHeader would mark it as invalid.

Maybe some rare scenario where devs were playing with invalidateblock RPC?

and invalidateblock RPC got interrupted before it could finish marking all descendant blocks as invalid
and they happened to be restarting their node with this commit haha

Still not an issue since we update the invalidity flags during restart in LoadBlockIndex.

Perhaps useful to write a test around this?

makes sense to test this migration behaviour. done in cfadc80.

stickies-v commented at 4:25 pm on October 17, 2025:

because we’re iterating through block indices in increasing order of heights

Yes you’re right, I hadn’t factored this in. Thank you for pointing out my mistake, the logic looks correct indeed.

makes sense to test this migration behaviour. done in https://github.com/bitcoin/bitcoin/commit/cfadc8038c08f9804c81af7950164483761f1db5.

Thanks, great to have more test coverage!

stringintech commented at 4:54 pm on November 5, 2025:

Related to your discussion:

The cleanup check on line 466 assumes invalid chains on disk always start with BLOCK_FAILED_VALID, followed by descendants with BLOCK_FAILED_CHILD:

0if (!(pindex->nStatus & BLOCK_FAILED_VALID) && pindex->pprev && (pindex->pprev->nStatus & BLOCK_FAILED_VALID))

However, if we load a chain where the first invalid block is BLOCK_FAILED_CHILD (with a valid parent), none of the blocks in that chain would be cleaned up.

I’m not sure if this scenario is possible in practice (perhaps through incomplete disk writes or edge cases in how master differentiates between the two flags?), but would the following check be safer to ensure no BLOCK_FAILED_CHILD is left behind during LoadBlockIndex():

0if (!(pindex->nStatus & BLOCK_FAILED_VALID) && pindex->pprev && (pindex->pprev->nStatus & (BLOCK_FAILED_CHILD | BLOCK_FAILED_VALID)))

in src/node/blockstorage.cpp:468 in 2e040d8b3c outdated

463@@ -464,7 +464,8 @@ bool BlockManager::LoadBlockIndex(const std::optional<uint256>& snapshot_blockha
464             }
465         }
466         if (!(pindex->nStatus & BLOCK_FAILED_VALID) && pindex->pprev && (pindex->pprev->nStatus & BLOCK_FAILED_VALID)) {
467-            pindex->nStatus |= BLOCK_FAILED_VALID;
468+            // BLOCK_FAILED_CHILD is deprecated, but may still exist on disk. Replace it with BLOCK_FAILED_VALID.
469+            pindex->nStatus = (pindex->nStatus & ~BLOCK_FAILED_CHILD) | BLOCK_FAILED_VALID;

stickies-v commented at 9:37 am on October 6, 2025:

In 2e040d8b3c861c96c536754a82f4352b635c4d01:

It seems like we’re trying to cram two different operations into one:

ensuring that all descendants of an invalid block are marked as invalid, in case there was any corruption/interruption
phasing out now-deprecated BLOCK_FAILED_CHILD flags

The current approach doesn’t look robust to me, i.e. it won’t catch all BLOCK_FAILED_CHILD flags. Suggested alternative that is more robust and (imo) more readable:

 0diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
 1index 47a7d656a8..d0e91e14d3 100644
 2--- a/src/node/blockstorage.cpp
 3+++ b/src/node/blockstorage.cpp
 4@@ -463,11 +463,16 @@ bool BlockManager::LoadBlockIndex(const std::optional<uint256>& snapshot_blockha
 5                 pindex->m_chain_tx_count = pindex->nTx;
 6             }
 7         }
 8-        if (!(pindex->nStatus & BLOCK_FAILED_VALID) && pindex->pprev && (pindex->pprev->nStatus & BLOCK_FAILED_VALID)) {
 9+        if (pindex->nStatus & BLOCK_FAILED_CHILD) {
10             // BLOCK_FAILED_CHILD is deprecated, but may still exist on disk. Replace it with BLOCK_FAILED_VALID.
11             pindex->nStatus = (pindex->nStatus & ~BLOCK_FAILED_CHILD) | BLOCK_FAILED_VALID;
12             m_dirty_blockindex.insert(pindex);
13         }
14+        if (!(pindex->nStatus & BLOCK_FAILED_VALID) && pindex->pprev && (pindex->pprev->nStatus & BLOCK_FAILED_VALID)) {
15+            // All descendants of invalid blocks are invalid too.
16+            pindex->nStatus = BLOCK_FAILED_VALID;
17+            m_dirty_blockindex.insert(pindex);
18+        }
19         if (pindex->pprev) {
20             pindex->BuildSkip();
21         }

stratospher commented at 3:07 pm on October 8, 2025:

The current approach doesn’t look robust to me, i.e. it won’t catch all BLOCK_FAILED_CHILD flags. Suggested alternative that is more robust and (imo) more readable:

current approach catches all BLOCK_FAILED_CHILD. let’s discuss it in #32950 (review).

git diff on https://github.com/bitcoin/bitcoin/commit/2e040d8b3c861c96c536754a82f4352b635c4d01

thanks! would have used the diff if there was a problem with the current approach.

current approach uses bit flips.
the git diff uses an if guard - if (pindex->nStatus & BLOCK_FAILED_CHILD).

I think avoiding bit flips with an if branch actually costs more than just doing the bit flips. we’d also enter into the if branch only once during the transition period and never again. so if it’s just readability that is an advantage now, I have a slight preference for the current approach and have left it as such. let me know if I’m missing something!

stickies-v commented at 4:26 pm on October 17, 2025:

so if it’s just readability that is an advantage now, I have a slight preference for the current approach

Okay, no big deal either way. I’m not sure the performance difference is going to show up in any meaningful way so I generally prefer readability until the need for performance improvement is shown (or obvious), but up to you and either is fine.

stickies-v commented at 10:40 am on October 6, 2025: contributor

Approach ACK 2e040d8b3c861c96c536754a82f4352b635c4d01

Intuitively it seems helpful to track something like BLOCK_FAILED_CHILD, but since it’s been unused for so long it seems the use case isn’t really there. Block validation logic is complex and critical and simplifying it is worth the effort, so I think this is the way to go.

refactor/validation: remove to_mark_failed

to_mark_failed was useful to track last disconnected block so that
it's children can be marked as BLOCK_FAILED_CHILD. since the
previous commit removes BLOCK_FAILED_CHILD usage in InvalidateBlock,
the existing variable invalid_walk_tip is sufficient.

Improve upon the variable name for `invalid_walk_tip` to make the
InvalidateBlock logic easier to read. Block tip before disconnection
is now tracked directly via `disconnected_tip`, and `new_tip`
is the tip after the disconnect.

Co-authored-by: stickies-v <stickies-v@protonmail.com>

f284834170

validation: remove BLOCK_FAILED_CHILD

even though we have a distinction between BLOCK_FAILED_VALID
and BLOCK_FAILED_CHILD in the codebase, we don't use it for
anything. since there's no functional difference between them
and it's unnecessary code complexity to categorise them correctly,
just mark as BLOCK_FAILED_VALID instead.

350b2ad29c

validation: remove BLOCK_FAILED_MASK

since it's the same as BLOCK_FAILED_VALID now

451f834676

validation: reset BLOCK_FAILED_CHILD to BLOCK_FAILED_VALID when loading from disk

- there maybe existing block indexes stored in disk with
  BLOCK_FAILED_CHILD
- since they don't exist anymore, clean up block index entries with
  BLOCK_FAILED_CHILD and reset it to BLOCK_FAILED_VALID.

87833a18ed

test: check LoadBlockIndex correctly recomputes invalidity flags

Add a test for block index transitioning from legacy
BLOCK_FAILED_CHILD to BLOCK_FAILED_VALID behavior.

In the scenario where a valid block has a BLOCK_FAILED_CHILD
parent and a BLOCK_FAILED_VALID grandparent, ensure that all
three blocks are correctly marked as BLOCK_FAILED_VALID
after reloading the block index.

cfadc8038c

stratospher force-pushed on Oct 8, 2025

stratospher commented at 3:27 pm on October 8, 2025: contributor

thanks @stickies-v! I’ve addressed your review comments.

added a test in https://github.com/bitcoin/bitcoin/pull/32950/commits/cfadc8038c08f9804c81af7950164483761f1db5
used the {disconnected,new}_tip variable tracking suggestions in #32950 (review)

in src/test/blockchain_tests.cpp:165 in cfadc8038c

160+        LOCK(::cs_main);
161+        assert(block_98->nStatus & BLOCK_FAILED_VALID);
162+        block_99->nStatus = (block_99->nStatus & ~BLOCK_FAILED_VALID) | BLOCK_FAILED_CHILD;
163+        block_100->nStatus = (block_100->nStatus & ~BLOCK_FAILED_VALID);
164+
165+        // Reload block index to recompute block status validity flags from disk.

stickies-v commented at 4:20 pm on October 17, 2025:

Note that nothing is written to disk here, this is all happening in-memory. It doesn’t look like the actual from-disk behaviour here is easy to test, because that’s relying on BlockManager internals, so this is probably the closest we can test. (nStatus being a non-const public member while m_dirty_blockindex being private is… not great)

I think it makes sense to add this as a separate test case, it’s not really related to invalidateblock and I find targeted tests easier to understand. Suggested diff with a few other improvements:

 0diff --git a/src/test/blockchain_tests.cpp b/src/test/blockchain_tests.cpp
 1index 5422095404..6b9fe75ea8 100644
 2--- a/src/test/blockchain_tests.cpp
 3+++ b/src/test/blockchain_tests.cpp
 4@@ -125,10 +125,6 @@ BOOST_FIXTURE_TEST_CASE(invalidate_block, TestChain100Setup)
 5     BlockValidationState state;
 6     auto* orig_tip = active.Tip();
 7 
 8-    CBlockIndex* block_100 = active[100];
 9-    CBlockIndex* block_99 = active[99];
10-    CBlockIndex* block_98 = active[98];
11-
12     int height_to_invalidate = orig_tip->nHeight - 10;
13     auto* tip_to_invalidate = active[height_to_invalidate];
14     m_node.chainman->ActiveChainstate().InvalidateBlock(state, tip_to_invalidate);
15@@ -150,26 +146,6 @@ BOOST_FIXTURE_TEST_CASE(invalidate_block, TestChain100Setup)
16         WITH_LOCK(::cs_main, assert(pindex->nStatus & BLOCK_FAILED_VALID));
17         pindex = pindex->pprev;
18     }
19-
20-    {
21-        // consider the chain of blocks block_98 <- block_99 <- block_100
22-        // intentionally mark:
23-        //   - block_98: BLOCK_FAILED_VALID (already marked as BLOCK_FAILED_VALID from previous test)
24-        //   - block_99: BLOCK_FAILED_CHILD (clear BLOCK_FAILED_VALID from previous test and mark as BLOCK_FAILED_CHILD)
25-        //   - block_100: not invalid (clear BLOCK_FAILED_VALID from previous test)
26-        LOCK(::cs_main);
27-        assert(block_98->nStatus & BLOCK_FAILED_VALID);
28-        block_99->nStatus = (block_99->nStatus & ~BLOCK_FAILED_VALID) | BLOCK_FAILED_CHILD;
29-        block_100->nStatus = (block_100->nStatus & ~BLOCK_FAILED_VALID);
30-
31-        // Reload block index to recompute block status validity flags from disk.
32-        m_node.chainman->LoadBlockIndex();
33-
34-        // check block_98, block_99, block_100 is marked as BLOCK_FAILED_VALID after reloading from disk
35-        assert(block_98->nStatus & BLOCK_FAILED_VALID);
36-        assert(block_99->nStatus & BLOCK_FAILED_VALID);
37-        assert(block_100->nStatus & BLOCK_FAILED_VALID);
38-    }
39 }
40 
41 BOOST_AUTO_TEST_SUITE_END()
42diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
43index bf440ca639..a5f4de17c6 100644
44--- a/src/test/validation_chainstatemanager_tests.cpp
45+++ b/src/test/validation_chainstatemanager_tests.cpp
46@@ -557,6 +557,28 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup)
47     BOOST_CHECK_EQUAL(cs2.setBlockIndexCandidates.size(), num_indexes - last_assumed_valid_idx + 1);
48 }
49 
50+BOOST_FIXTURE_TEST_CASE(loadblockindex_invalid_descendants, TestChain100Setup)
51+{
52+    LOCK(Assert(m_node.chainman)->GetMutex());
53+    // Consider the chain of blocks: grand_parent   <-      parent      <-       child
54+    //                            BLOCK_FAILED_VALID   BLOCK_FAILED_CHILD     <not invalid>
55+    // Test that when the block index is loaded, all blocks are marked as BLOCK_FAILED_VALID
56+    auto* child{m_node.chainman->ActiveChain().Tip()};
57+    auto* parent{child->pprev};
58+    auto* grand_parent{parent->pprev};
59+    grand_parent->nStatus = (grand_parent->nStatus | BLOCK_FAILED_VALID);
60+    parent->nStatus = (parent->nStatus & ~BLOCK_FAILED_VALID) | BLOCK_FAILED_CHILD;
61+    child->nStatus = (child->nStatus & ~BLOCK_FAILED_VALID);
62+
63+    // Reload block index to recompute block status validity flags.
64+    m_node.chainman->LoadBlockIndex();
65+
66+    // check grand_parent, parent, child is marked as BLOCK_FAILED_VALID after reloading the block index
67+    BOOST_CHECK(grand_parent->nStatus & BLOCK_FAILED_VALID);
68+    BOOST_CHECK(parent->nStatus & BLOCK_FAILED_VALID);
69+    BOOST_CHECK(child->nStatus & BLOCK_FAILED_VALID);
70+}
71+
72 //! Ensure that snapshot chainstates initialize properly when found on disk.
73 BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_init, SnapshotTestSetup)
74 {

stringintech commented at 10:58 am on November 4, 2025: contributor

Concept ACK. Left two comments: 1 and 2

validation: remove BLOCK_FAILED_CHILD #32950

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)