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)

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
ACK	stickies-v, alexanderwiederin, mzumsande
Concept ACK	sedited, Prabhat1308, yuvicc, sipa, stringintech
Stale ACK	naiyoma

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:

#34521 (validation: fix UB in LoadChainTip by marcofleon)
#34440 (Refactor CChain methods to use references, tests by optout21)
#34254 (validation: Prevent duplicate logging and looping in invalid block handling by mzumsande)
#30342 (kernel, logging: Pass Logger instances to kernel objects by ryanofsky)
#29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)

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.

sedited 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 sedited 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?

stratospher commented at 6:22 am on November 22, 2025:

now that I think about it again, makes sense to call the rest of the code with last disconnected block (and not pindex) for a consistent + graceful shutdown. thanks for catching this!

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 it seems the suggested diff here wouldn’t face the same issue.

In general, I think it makes sense to clean up every loaded BLOCK_FAILED_CHILD as BLOCK_FAILED_VALID during startup (the first step in the diff), regardless of whether the persisted status makes sense in context.

stratospher commented at 6:21 am on November 22, 2025:

@stringintech great catch! thank you! I’ve updated the PR to use the diff suggested above.

I forgot the 1 edge case scenario where this is possible - #32173 (comment). I ran bitcoind with an inconsistent block index state from the functional test in the issue (BLOCK_FAILED_CHILD happens with valid parents) and current logic wouldn’t work as you mentioned. but it works on the latest push now.

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.

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 {

stratospher commented at 6:22 am on November 22, 2025:

taken. thanks!

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

Concept ACK. Left two comments: 1 and 2

stratospher force-pushed on Nov 22, 2025

stratospher commented at 6:22 am on November 22, 2025: contributor

thanks for the great catches @stringintech and the nicer tests @stickies-v! I’ve pushed an update.

DrahtBot added the label Needs rebase on Dec 16, 2025

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.

18f11695c7

stratospher force-pushed on Feb 9, 2026

DrahtBot added the label CI failed on Feb 9, 2026

DrahtBot commented at 1:42 pm on February 9, 2026: contributor

🚧 At least one of the CI tasks failed. Task macOS-cross to x86_64: https://github.com/bitcoin/bitcoin/actions/runs/21826708453/job/62974666729 LLM reason (✨ experimental): Compilation failed: BLOCK_FAILED_MASK is undeclared (likely a mismatch with BLOCK_VALID_MASK in chain.h).

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot removed the label Needs rebase on Feb 9, 2026

DrahtBot removed the label CI failed on Feb 9, 2026

fanquake added this to the milestone 31.0 on Feb 12, 2026

fanquake commented at 11:49 am on February 12, 2026: member

Plenty of conceptual buy-in here, just seems that review momentum has stalled. This fixes at least 1, if not two bugs, so would be good to land in 31.x. I’ve added it to the milestone.

sedited requested review from stickies-v on Feb 13, 2026

sedited requested review from stringintech on Feb 13, 2026

sedited requested review from naiyoma on Feb 13, 2026

in src/node/blockstorage.cpp:498 in 16636cc5b9

495+            pindex->nStatus = (pindex->nStatus & ~BLOCK_FAILED_CHILD) | BLOCK_FAILED_VALID;
496+            m_dirty_blockindex.insert(pindex);
497+        }
498+        if (!(pindex->nStatus & BLOCK_FAILED_VALID) && pindex->pprev && (pindex->pprev->nStatus & BLOCK_FAILED_VALID)) {
499+            // All descendants of invalid blocks are invalid too.
500+            pindex->nStatus = BLOCK_FAILED_VALID;

stickies-v commented at 4:40 am on February 16, 2026:

I don’t think we mean to override the entire nStatus here, just bitflip? (sorry for getting this wrong in my suggestion earlier)

0            pindex->nStatus |= BLOCK_FAILED_VALID;

The below test change fails without this fix, and passes with.

 0diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
 1index 45c72f5af8..218ee222ef 100644
 2--- a/src/node/blockstorage.cpp
 3+++ b/src/node/blockstorage.cpp
 4@@ -495,7 +495,7 @@ bool BlockManager::LoadBlockIndex(const std::optional<uint256>& snapshot_blockha
 5         }
 6         if (!(pindex->nStatus & BLOCK_FAILED_VALID) && pindex->pprev && (pindex->pprev->nStatus & BLOCK_FAILED_VALID)) {
 7             // All descendants of invalid blocks are invalid too.
 8-            pindex->nStatus = BLOCK_FAILED_VALID;
 9+            pindex->nStatus |= BLOCK_FAILED_VALID;
10             m_dirty_blockindex.insert(pindex);
11         }
12 
13diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
14index 40f99690ce..64aac67ccb 100644
15--- a/src/test/validation_chainstatemanager_tests.cpp
16+++ b/src/test/validation_chainstatemanager_tests.cpp
17@@ -601,6 +601,7 @@ BOOST_FIXTURE_TEST_CASE(loadblockindex_invalid_descendants, TestChain100Setup)
18     //   - child: not invalid
19     // Test that when the block index is loaded, all blocks are marked as BLOCK_FAILED_VALID
20     auto* child{m_node.chainman->ActiveChain().Tip()};
21+    BOOST_CHECK(child->nStatus & BLOCK_HAVE_DATA);
22     auto* parent{child->pprev};
23     auto* grand_parent{parent->pprev};
24     grand_parent->nStatus = (grand_parent->nStatus | BLOCK_FAILED_VALID);
25@@ -614,6 +615,8 @@ BOOST_FIXTURE_TEST_CASE(loadblockindex_invalid_descendants, TestChain100Setup)
26     BOOST_CHECK(grand_parent->nStatus & BLOCK_FAILED_VALID);
27     BOOST_CHECK(parent->nStatus & BLOCK_FAILED_VALID);
28     BOOST_CHECK(child->nStatus & BLOCK_FAILED_VALID);
29+
30+    BOOST_CHECK(child->nStatus & BLOCK_HAVE_DATA);
31 }
32 
33 //! Ensure that snapshot chainstate can be loaded when found on disk after a

stratospher commented at 11:53 am on February 16, 2026:

whoops, thank you for catching that!

stratospher force-pushed on Feb 16, 2026

in src/node/blockstorage.cpp:1 in 3a8226258e outdated

stickies-v commented at 5:02 am on February 17, 2026:

I think it’s unsafe to place commit validation: reset BLOCK_FAILED_CHILD to BLOCK_FAILED_VALID when loading from disk after the validation: remove BLOCK_FAILED_MASK commit. We should only remove BLOCK_FAILED_MASK when there can be no more instances of BLOCK_FAILED_CHILD. A good flow imo is:

stop producing BLOCK_FAILED_CHILD flags
migrate legacy data
remove BLOCK_FAILED_MASK

mzumsande commented at 3:15 pm on February 17, 2026:

I think it’s not too bad because blocks marked as BLOCK_FAILED_CHILD on master should have an ancestor that is BLOCK_FAILED_VALID. So they should be marked as invalid anyway on startup - it would only lead to a wrong state if the original marking was already wrong due to an unrelated bug, as in #32173 (comment). But I agree in general with the suggested flow and that it’s cleaner to do this earlier.

stratospher commented at 4:12 pm on February 17, 2026:

done in this no code change push.

in src/validation.cpp:3187 in b9653739c4 outdated

3183@@ -3184,7 +3184,7 @@ CBlockIndex* Chainstate::FindMostWorkChain()
3184                 // Remove the entire chain from the set.
3185                 while (pindexTest != pindexFailed) {
3186                     if (fFailedChain) {
3187-                        pindexFailed->nStatus |= BLOCK_FAILED_CHILD;
3188+                        pindexFailed->nStatus |= BLOCK_FAILED_VALID;

stickies-v commented at 5:32 am on February 17, 2026:

Review note: probably orthogonal to this PR, but it seems strange that we need to do any nStatus update at all here. FindMostWorkChain() should not be responsible for/have side effects that update block validity. The unit and functional tests pass with the below change, so I think this is a no-op indeed. (it also passes with assert(pindexFailed->nStatus & BLOCK_FAILED_CHILD); on master)

Perhaps useful to address / clean up (if applicable, I’m very much not sure I’m not missing anything) in a separate PR? I think it’s best to keep it as-is in this PR to keep the scope narrow. Alternatively, if my understanding is wrong, it could be useful to extend test coverage to catch this mutation, potentially in this PR?

 0diff --git a/src/validation.cpp b/src/validation.cpp
 1index 4ec917a0d2..96d3fdf5cb 100644
 2--- a/src/validation.cpp
 3+++ b/src/validation.cpp
 4@@ -3184,8 +3184,7 @@ CBlockIndex* Chainstate::FindMostWorkChain()
 5                 // Remove the entire chain from the set.
 6                 while (pindexTest != pindexFailed) {
 7                     if (fFailedChain) {
 8-                        pindexFailed->nStatus |= BLOCK_FAILED_VALID;
 9-                        m_blockman.m_dirty_blockindex.insert(pindexFailed);
10+                        assert(pindexFailed->nStatus & BLOCK_FAILED_VALID);
11                     } else if (fMissingData) {
12                         // If we're missing data, then add back to m_blocks_unlinked,
13                         // so that if the block arrives in the future we can try adding

mzumsande commented at 3:36 pm on February 17, 2026:

Good find - I think this is there for historical reasons mostly: For a long time, BLOCK_FAILED_CHILD was set on a best-effort basis (just mark the easy ones and don’t bother with the rest), and this is a convenient place to correct some spots that were missed.

Some of my past PRs made this more strict (when marking a block as failed, also mark all descendants immediately as failed), with ed764ea2b4edb3cf1925a4bff5f39567a8be54ac finally adding an assert for this to CheckBlockIndex . This means if this update here was still necessary, there should be a hittable assert in regtest / fuzz tests.

So I agree that we should be able to remove this now, but I would suggest to do that in a separate PR.

Adding an assert here would be way too dangerous in my opinion (some bug in a special constellation, and miners could take down large parts of the network by creating that case), but an Assume should be good.

stratospher commented at 4:15 pm on February 17, 2026:

nice find, I tried out a few scenarios like the one below and found that it doesn’t need it since https://github.com/bitcoin/bitcoin/commit/f5149ddb9b7de3559943d7fda0f440e59413dfb5.

we receive headers A, B, C, D built on top of normal-chain -> A -> B -> C -> D
we then receive blocks B, C, D
we later receive A and find out it is invalid in ConnectBlock.

ConnectBlock calls InvalidChainFound and there’s now a SetBlockFailureFlags inside since f5149ddb9b7de3559943d7fda0f440e59413dfb5 which marks B,C,D as invalid too. So we wouldn’t enter a situation where FindMostWorkChain has to mark blocks as invalid since we already dealt with it when the invalid block was found.

(need to think more about what happens during a race condition between ForkMostWorkChain and new header arriving - don’t imagine it’s a problem since we’d need cs_main anyways.)

will open a separate PR for this after some testing.

stickies-v approved

stickies-v commented at 6:04 am on February 17, 2026: contributor

ACK b9653739c403bbc9fc2dc932be053bcad7acbe89 modulo updating the commit ordering.

Prior to this PR, code was setting BLOCK_FAILED_CHILD, but (outside of tests) never directly reading it - always through BLOCK_FAILED_MASK. This, in combination with multiple full re-reviews, gives me good confidence that dropping BLOCK_FAILED_CHILD is safe, even if the validation logic is complex and I don’t always understand all the nuances.

DrahtBot requested review from sipa on Feb 17, 2026

in src/validation.cpp:3649 in e2b06e3877 outdated

3651+        auto candidate_it = highpow_outofchain_headers.lower_bound(new_tip->nChainWork);
3652 
3653-        const bool best_header_needs_update{m_chainman.m_best_header->GetAncestor(invalid_walk_tip->nHeight) == invalid_walk_tip};
3654+        const bool best_header_needs_update{m_chainman.m_best_header->GetAncestor(disconnected_tip->nHeight) == disconnected_tip};
3655         if (best_header_needs_update) {
3656             // pprev is definitely still valid at this point, but there may be better ones

mzumsande commented at 2:03 pm on February 17, 2026:

nit: pprev -> new_tip, it’s not clear what pprev refers to after the rename

in src/chain.h:80 in e86311ca1b outdated

76@@ -77,7 +77,7 @@ enum BlockStatus : uint32_t {
77     BLOCK_HAVE_MASK          =   BLOCK_HAVE_DATA | BLOCK_HAVE_UNDO,
78 
79     BLOCK_FAILED_VALID       =   32, //!< stage after last reached validness failed
80-    BLOCK_FAILED_CHILD       =   64, //!< descends from failed block
81+    BLOCK_FAILED_CHILD       =   64, //!< Unused flag that was previously set when descending from failed block

mzumsande commented at 2:40 pm on February 17, 2026:

nit: “remove BLOCK_FAILED_CHILD” in the commit message may be too strict sind it’s still part of the enum - maybe call it “stop using” instead.

mzumsande commented at 3:43 pm on February 17, 2026: contributor

ACK b9653739c403bbc9fc2dc932be053bcad7acbe89 - I agree that moving commit 3a8226258e9a22ecad43d3e50daad8eb626d4e8b to an earlier spot would be good though.

stratospher force-pushed on Feb 17, 2026

refactor: use clearer variables in InvalidateBlock()

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>

120c631e16

validation: stop using 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.

37bc207852

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.

b5b2956bda

validation: remove BLOCK_FAILED_MASK

since it's the same as BLOCK_FAILED_VALID now

29740c06ac

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.

fb3e1bf9c9

stratospher force-pushed on Feb 17, 2026

DrahtBot added the label CI failed on Feb 17, 2026

DrahtBot removed the label CI failed on Feb 17, 2026

stickies-v commented at 2:56 am on February 18, 2026: contributor

re-ACK fb3e1bf9c9772631571ca46d29c50330ebf54dfd

No changes except addressing review comments to:

improve commit ordering
docstring update
commit message update

DrahtBot requested review from mzumsande on Feb 18, 2026

gniumg-source referenced this in commit a93cf6c8d2 on Feb 18, 2026

gniumg-source referenced this in commit e73f089772 on Feb 18, 2026

in src/node/blockstorage.cpp:494 in b5b2956bda

486@@ -487,10 +487,18 @@ bool BlockManager::LoadBlockIndex(const std::optional<uint256>& snapshot_blockha
487                 pindex->m_chain_tx_count = pindex->nTx;
488             }
489         }
490+
491+        if (pindex->nStatus & BLOCK_FAILED_CHILD) {
492+            // BLOCK_FAILED_CHILD is deprecated, but may still exist on disk. Replace it with BLOCK_FAILED_VALID.
493+            pindex->nStatus = (pindex->nStatus & ~BLOCK_FAILED_CHILD) | BLOCK_FAILED_VALID;
494+            m_dirty_blockindex.insert(pindex);

alexanderwiederin commented at 12:12 pm on February 18, 2026:

When can we remove this, or is the idea to keep it forever?

stratospher commented at 3:10 pm on February 18, 2026:

ideally I’d imagine when there’s <100 nodes (that don’t have a history of upgrading) in the network remaining, meaning all actively-maintained nodes have already migrated. I guess the number 100 is debatable/too conservative. just taking it as an example.

alexanderwiederin commented at 3:34 pm on February 18, 2026:

Not sure how it’s done here in core, but I would suggest we open a follow-up issue for the removal and reference it in the comment. What do you think?

sipa commented at 3:46 pm on February 18, 2026:

This seems so low-cost that we can keep it around for forever, or at least until the next incompatible database change (IIRC, the last one was in 0.15 with per-txout records, and before that, 0.8).

alexanderwiederin commented at 1:56 pm on February 18, 2026: none

Tested locally and reviewed all commits. Question on the migration code is inline.

ACK fb3e1bf9c9772631571ca46d29c50330ebf54dfd

mzumsande commented at 3:41 pm on February 18, 2026: contributor

re-ACK fb3e1bf9c9772631571ca46d29c50330ebf54dfd

fanquake merged this on Feb 18, 2026

fanquake closed this on Feb 18, 2026

sipa commented at 3:49 pm on February 18, 2026: member

Posthumous code review ACK fb3e1bf9c9772631571ca46d29c50330ebf54dfd

Did someone test that downgrading to previous releases works?

alexanderwiederin commented at 4:48 pm on February 18, 2026: none

Did someone test that downgrading to previous releases works?

I just tested on regtest. Verified invalidateblock and reconsiderblock work correctly across old -> new -> old node switches with multiple invalid chains.

stratospher commented at 1:22 pm on February 19, 2026: contributor

Did someone test that downgrading to previous releases works?

yes! only difference which doesn’t have any impact since BLOCK_FAILED_CHILD is only written and not used/read anywhere (not even in CheckBlockIndex) is this:

in previous release suppose this chain existed: genesisblock <- block1 <- block2(BLOCK_FAILED_VALID) <- block3 (BLOCK_FAILED_CHILD) <-block4 (BLOCK_FAILED_CHILD)
in new release it would become genesisblock <- block1 <- block2(BLOCK_FAILED_VALID) <- block3 (BLOCK_FAILED_VALID) <-block4 (BLOCK_FAILED_VALID)
if we change to use previous release again, the nStatus would still remain the same (since we only mark blocks which are valid as BLOCK_FAILED_CHILD in LoadBlockIndex) genesisblock <- block1 <- block2(BLOCK_FAILED_VALID) <- block3 (BLOCK_FAILED_VALID) <-block4 (BLOCK_FAILED_VALID)

validation: remove BLOCK_FAILED_CHILD #32950

Code Coverage & Benchmarks

Reviews

Conflicts