validation: set BLOCK_FAILED

stratospher commented at 2:59 pm on February 10, 2025: contributor

This PR addresses 3 issues related to how BLOCK_FAILED_CHILD is set:

In InvalidateBlock()

Previously, BLOCK_FAILED_CHILD was not being set when it should have been.
This was due to an incorrect traversal condition, which is fixed in this PR.

In SetBlockFailure()

BLOCK_FAILED_VALID is now cleared before setting BLOCK_FAILED_CHILD.

In InvalidateBlock()

if block is already marked as BLOCK_FAILED_CHILD, don’t mark it as BLOCK_FAILED_VALID again.

Also adds a unit test to check BLOCK_FAILED_VALID and BLOCK_FAILED_CHILD status in InvalidateBlock().

An alternate approach could be removing BLOCK_FAILED_CHILD since 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. See similar discussion in #16856.

I have a branch with this approach in https://github.com/stratospher/bitcoin/commits/2025_02_remove_block_failed_child/. Compared to the version in #16856, it also resets BLOCK_FAILED_CHILD already on disk to BLOCK_FAILED_VALID when loading from disk so that we won’t be in a dirty state in a no-BLOCK_FAILED_CHILD-world.

I’m not sure if it’s a good idea to remove BLOCK_FAILED_CHILD though. would be curious to hear what others think of this approach.

thanks @ mzumsande for helpful discussion regarding this PR!

DrahtBot commented at 2:59 pm on February 10, 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/31835.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	mzumsande, TheCharlatan

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:

#31778 (refactor: simplify GetAncestor by kcalvinalvin)
#31405 (validation: stricter internal handling of invalid blocks 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.

DrahtBot added the label Validation on Feb 10, 2025

mzumsande commented at 11:09 pm on February 11, 2025: contributor

Concept ACK

This is a straightforward bugfix.

In general, I’m in favor of removing the BLOCK_FAILED_CHILD / BLOCK_FAILED_VALID distinction as a follow-up, as suggested in #16856 (comment). It’s been introduced in 2012 (#1677) and (as far as I know) has never been used for anything. I also can’t think of a legitimate use - block indexes that are marked as invalid are invalid indefinitely (outside of the reconsiderblock rpc, but that removes both flags from all descendants of a reconsidered block so it doesn’t need the distinction either.) It just adds unnecessary complexity in my opinion.

in src/test/interfaces_tests.cpp:127 in c03ff89eba outdated

122@@ -123,6 +123,32 @@ BOOST_AUTO_TEST_CASE(findCommonAncestor)
123     BOOST_CHECK(!chain->findCommonAncestor({}, orig_tip->GetBlockHash(), {}, {}, FoundBlock().hash(orig_hash)));
124     BOOST_CHECK_EQUAL(active_hash, active.Tip()->GetBlockHash());
125     BOOST_CHECK_EQUAL(orig_hash, orig_tip->GetBlockHash());
126+
127+    // Check BlockStatus when doing InvalidateBlock()

mzumsande commented at 9:18 pm on March 6, 2025:

I think this would be better suited in a different test file since invalidateblock is not part of interfaces/chain.h: While the existing test already calls InvalidateBlock(), this is just as a means to the end to test findCommonAncestor. Maybe blockchain_tests.cpp would be a better place?

stratospher commented at 12:18 pm on March 8, 2025:

right makes sense, I’ve moved it to blockchain_tests.cpp.

stratospher force-pushed on Mar 8, 2025

stratospher commented at 12:24 pm on March 8, 2025: contributor

also added 1 more commit 1771866 (taken from #16856) since I ran into this situation when using the fuzzer from https://github.com/bitcoin/bitcoin/pull/31533

mzumsande commented at 3:43 pm on March 25, 2025: contributor

Code Review ACK 17718660b8c95d1c12124ba2f38baf286a3bddf2

in src/validation.cpp:3750 in 27b5abf946 outdated

3746@@ -3747,7 +3747,7 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
3747         m_blockman.m_dirty_blockindex.insert(invalid_walk_tip);
3748         setBlockIndexCandidates.erase(invalid_walk_tip);
3749         setBlockIndexCandidates.insert(invalid_walk_tip->pprev);
3750-        if (invalid_walk_tip->pprev == to_mark_failed && (to_mark_failed->nStatus & BLOCK_FAILED_VALID)) {
3751+        if (invalid_walk_tip == to_mark_failed->pprev && (to_mark_failed->nStatus & BLOCK_FAILED_VALID)) {

TheCharlatan commented at 1:33 pm on March 28, 2025:

In commit 27b5abf946e737940bba5ea4b82385a100a35156:

The diagram in the commit message is a bit confusing. Are the numbers supposed to be block heights? Should the arrows be inverted then?

stratospher commented at 3:18 am on March 31, 2025:

updated it to look like prev pointers. I thought of it as block heights growing but prev pointers are more accurate/less confusing.

in src/test/blockchain_tests.cpp:135 in 31c3f90b50 outdated

130+
131+    // tip_to_invalidate just got invalidated, so it's BLOCK_FAILED_VALID
132+    WITH_LOCK(::cs_main, assert(tip_to_invalidate->nStatus & BLOCK_FAILED_VALID));
133+    WITH_LOCK(::cs_main, assert((tip_to_invalidate->nStatus & BLOCK_FAILED_CHILD) == 0));
134+
135+    // check all ancestors of block are validated up to BLOCK_VALID_TRANSACTIONS and are not invalid

TheCharlatan commented at 2:09 pm on March 28, 2025:

Nit: Should this say “Check all ancestors of the invalidated block are validated up to…”?

stratospher commented at 3:18 am on March 31, 2025:

done.

TheCharlatan approved

TheCharlatan commented at 2:46 pm on March 28, 2025: contributor

ACK 17718660b8c95d1c12124ba2f38baf286a3bddf2

This looks fine, but I think the commit messages could all be a bit clearer.

validation: fix traversal condition to mark BLOCK_FAILED_CHILD

this block of code is not reached on master since other than
initialisation, all other iterations have invalid_walk_tip
and to_mark_failed pointers in some form of this layout
where 1, 2, 3 and 4 are block heights.

	invalid_walk_tip
	  ↓
1 <- 2 <- 3 <- 4
	       ↑
	      to_mark_failed

fix it so that blocks are correctly marked as BLOCK_FAILED_CHILD
if it's a descendant of BLOCK_FAILED_VALID block.

c99667583d

test: check BlockStatus when InvalidateBlock is used

when a block is invalidated using InvalidateBlock, check that:
1. it's status is BLOCK_FAILED_VALID
2. it's children's status is BLOCK_FAILED_CHILD
   and not BLOCK_FAILED_VALID
3. it's ancestors are valid

9e29653b42

validation: correctly update BlockStatus for invalid block descendants

invalid_block ----------> block_index

- before this commit, only if block_index is not invalid, it will mark
  block_index as BLOCK_FAILED_CHILD
- it's possible that block_index encountered is invalid and was marked
  as BLOCK_FAILED_VALID previously
- in this case, correctly update BlockStatus of block_index by
  clearing BLOCK_FAILED_VALID and then setting it to BLOCK_FAILED_CHILD

aac5488909

validation: clarify final |= BLOCK_FAILED_VALID in InvalidateBlock

This has no functional affect, as the any CBlockIndex*s which
to_mark_failed is set to will already have been marked failed.

Also prevents a situation where block already marked as
BLOCK_FAILED_CHILD is again unconditionally marked as
BLOCK_FAILED_VALID in the final |= BLOCK_FAILED_VALID.

3c3548a70e

stratospher force-pushed on Mar 31, 2025

validation: set BLOCK_FAILED_CHILD correctly #31835

Code Coverage & Benchmarks

Reviews

Conflicts