From 27b5abf946e737940bba5ea4b82385a100a35156 Mon Sep 17 00:00:00 2001 From: stratospher <44024636+stratospher@users.noreply.github.com> Date: Fri, 7 Feb 2025 09:29:31 +0530 Subject: [PATCH 1/4] validation: fix traversal condition to mark BLOCK_FAILED_CHILD MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. 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. --- src/validation.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/validation.cpp b/src/validation.cpp index 0384018bc36..673c9272fe6 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3747,7 +3747,7 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde m_blockman.m_dirty_blockindex.insert(invalid_walk_tip); setBlockIndexCandidates.erase(invalid_walk_tip); setBlockIndexCandidates.insert(invalid_walk_tip->pprev); - if (invalid_walk_tip->pprev == to_mark_failed && (to_mark_failed->nStatus & BLOCK_FAILED_VALID)) { + if (invalid_walk_tip == to_mark_failed->pprev && (to_mark_failed->nStatus & BLOCK_FAILED_VALID)) { // We only want to mark the last disconnected block as BLOCK_FAILED_VALID; its children // need to be BLOCK_FAILED_CHILD instead. to_mark_failed->nStatus = (to_mark_failed->nStatus ^ BLOCK_FAILED_VALID) | BLOCK_FAILED_CHILD; From 31c3f90b509754a274e9beb9c46db28d6e5c3114 Mon Sep 17 00:00:00 2001 From: stratospher <44024636+stratospher@users.noreply.github.com> Date: Wed, 15 Jan 2025 14:59:09 +0530 Subject: [PATCH 2/4] 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 --- src/test/blockchain_tests.cpp | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/test/blockchain_tests.cpp b/src/test/blockchain_tests.cpp index 4ecc15041cc..91a2dcd8fb7 100644 --- a/src/test/blockchain_tests.cpp +++ b/src/test/blockchain_tests.cpp @@ -117,4 +117,36 @@ BOOST_AUTO_TEST_CASE(num_chain_tx_max) BOOST_CHECK_EQUAL(block_index.m_chain_tx_count, std::numeric_limits::max()); } +BOOST_FIXTURE_TEST_CASE(invalidate_block, TestChain100Setup) +{ + const CChain& active{*WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return &Assert(m_node.chainman)->ActiveChain())}; + + // Check BlockStatus when doing InvalidateBlock() + BlockValidationState state; + auto* orig_tip = active.Tip(); + int height_to_invalidate = orig_tip->nHeight - 10; + auto* tip_to_invalidate = active[height_to_invalidate]; + m_node.chainman->ActiveChainstate().InvalidateBlock(state, tip_to_invalidate); + + // tip_to_invalidate just got invalidated, so it's BLOCK_FAILED_VALID + WITH_LOCK(::cs_main, assert(tip_to_invalidate->nStatus & BLOCK_FAILED_VALID)); + WITH_LOCK(::cs_main, assert((tip_to_invalidate->nStatus & BLOCK_FAILED_CHILD) == 0)); + + // check all ancestors of block are validated up to BLOCK_VALID_TRANSACTIONS and are not invalid + auto pindex = tip_to_invalidate->pprev; + while (pindex) { + WITH_LOCK(::cs_main, assert(pindex->IsValid(BLOCK_VALID_TRANSACTIONS))); + WITH_LOCK(::cs_main, assert((pindex->nStatus & BLOCK_FAILED_MASK) == 0)); + pindex = pindex->pprev; + } + + // check all descendants of block are BLOCK_FAILED_CHILD + pindex = orig_tip; + while (pindex && pindex != tip_to_invalidate) { + WITH_LOCK(::cs_main, assert((pindex->nStatus & BLOCK_FAILED_VALID) == 0)); + WITH_LOCK(::cs_main, assert(pindex->nStatus & BLOCK_FAILED_CHILD)); + pindex = pindex->pprev; + } +} + BOOST_AUTO_TEST_SUITE_END() From 6c4f03cedbf811d56cc265369302968f96f77d4a Mon Sep 17 00:00:00 2001 From: stratospher <44024636+stratospher@users.noreply.github.com> Date: Fri, 7 Feb 2025 10:22:39 +0530 Subject: [PATCH 3/4] 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 --- src/validation.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 673c9272fe6..0714fb7f437 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3826,8 +3826,9 @@ void Chainstate::SetBlockFailureFlags(CBlockIndex* invalid_block) AssertLockHeld(cs_main); for (auto& [_, block_index] : m_blockman.m_block_index) { - if (block_index.GetAncestor(invalid_block->nHeight) == invalid_block && !(block_index.nStatus & BLOCK_FAILED_MASK)) { - block_index.nStatus |= BLOCK_FAILED_CHILD; + if (invalid_block != &block_index && block_index.GetAncestor(invalid_block->nHeight) == invalid_block) { + block_index.nStatus = (block_index.nStatus & ~BLOCK_FAILED_VALID) | BLOCK_FAILED_CHILD; + m_blockman.m_dirty_blockindex.insert(&block_index); } } } From 17718660b8c95d1c12124ba2f38baf286a3bddf2 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sat, 8 Mar 2025 17:43:44 +0530 Subject: [PATCH 4/4] 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. --- src/test/blockchain_tests.cpp | 5 +++++ src/validation.cpp | 12 +++++++----- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/test/blockchain_tests.cpp b/src/test/blockchain_tests.cpp index 91a2dcd8fb7..c010c8778c1 100644 --- a/src/test/blockchain_tests.cpp +++ b/src/test/blockchain_tests.cpp @@ -147,6 +147,11 @@ BOOST_FIXTURE_TEST_CASE(invalidate_block, TestChain100Setup) WITH_LOCK(::cs_main, assert(pindex->nStatus & BLOCK_FAILED_CHILD)); pindex = pindex->pprev; } + + // don't mark already invalidated block (orig_tip is BLOCK_FAILED_CHILD) with BLOCK_FAILED_VALID again + m_node.chainman->ActiveChainstate().InvalidateBlock(state, orig_tip); + WITH_LOCK(::cs_main, assert(orig_tip->nStatus & BLOCK_FAILED_CHILD)); + WITH_LOCK(::cs_main, assert((orig_tip->nStatus & BLOCK_FAILED_VALID) == 0)); } BOOST_AUTO_TEST_SUITE_END() diff --git a/src/validation.cpp b/src/validation.cpp index 0714fb7f437..6185556c02c 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3779,11 +3779,13 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde return false; } - // Mark pindex (or the last disconnected block) as invalid, even when it never was in the main chain - to_mark_failed->nStatus |= BLOCK_FAILED_VALID; - m_blockman.m_dirty_blockindex.insert(to_mark_failed); - setBlockIndexCandidates.erase(to_mark_failed); - m_chainman.m_failed_blocks.insert(to_mark_failed); + // Mark pindex as invalid if it never was in the main chain + if (!pindex_was_in_chain && !(pindex->nStatus & BLOCK_FAILED_MASK)) { + pindex->nStatus |= BLOCK_FAILED_VALID; + m_blockman.m_dirty_blockindex.insert(pindex); + setBlockIndexCandidates.erase(pindex); + m_chainman.m_failed_blocks.insert(pindex); + } // If any new blocks somehow arrived while we were disconnecting // (above), then the pre-calculation of what should go into