Skip to content

validation: stricter internal handling of invalid blocks #31405

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ryanofsky merged 7 commits into from
Jun 11, 2025
Merged

validation: stricter internal handling of invalid blocks #31405

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
15fa5b5
validation: call InvalidBlockFound also from AcceptBlock
mzumsande Dec 1, 2024
4c29326
validation: cache all headers with enough PoW in invalidateblock
mzumsande Mar 18, 2025
8e39f2d
validation: in invalidateblock, mark children as invalid right away
mzumsande Nov 30, 2024
9a70883
validation: in invalidateblock, calculate m_best_header right away
mzumsande Nov 25, 2024
ed764ea
validation: Add more checks to CheckBlockIndex()
mzumsande Dec 2, 2024
ee673b9
validation: remove m_failed_blocks
mzumsande Dec 1, 2024
f6b782f
doc: Improve m_best_header documentation
mzumsande Jun 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 48 additions & 61 deletions src/validation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2091,7 +2091,6 @@ void Chainstate::InvalidBlockFound(CBlockIndex* pindex, const BlockValidationSta
AssertLockHeld(cs_main);
if (state.GetResult() != BlockValidationResult::BLOCK_MUTATED) {
pindex->nStatus |= BLOCK_FAILED_VALID;
m_chainman.m_failed_blocks.insert(pindex);
m_blockman.m_dirty_blockindex.insert(pindex);
setBlockIndexCandidates.erase(pindex);
InvalidChainFound(pindex);
Expand Down Expand Up @@ -3690,7 +3689,7 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
// To avoid walking the block index repeatedly in search of candidates,
// build a map once so that we can look up candidate blocks by chain
// work as we go.
std::multimap<const arith_uint256, CBlockIndex *> candidate_blocks_by_work;
std::multimap<const arith_uint256, CBlockIndex*> highpow_outofchain_headers;

{
LOCK(cs_main);
Expand All @@ -3699,13 +3698,12 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
// We don't need to put anything in our active chain into the
// multimap, because those candidates will be found and considered
// as we disconnect.
// Instead, consider only non-active-chain blocks that have at
// least as much work as where we expect the new tip to end up.
// Instead, consider only non-active-chain blocks that score
// at least as good with CBlockIndexWorkComparator as the new tip.
if (!m_chain.Contains(candidate) &&
!CBlockIndexWorkComparator()(candidate, pindex->pprev) &&
candidate->IsValid(BLOCK_VALID_TRANSACTIONS) &&
candidate->HaveNumChainTxs()) {
candidate_blocks_by_work.insert(std::make_pair(candidate->nChainWork, candidate));
!CBlockIndexWorkComparator()(candidate, pindex->pprev) &&
!(candidate->nStatus & BLOCK_FAILED_MASK)) {
highpow_outofchain_headers.insert({candidate->nChainWork, candidate});
}
}
}
Expand Down Expand Up @@ -3754,15 +3752,42 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
m_blockman.m_dirty_blockindex.insert(to_mark_failed);
}

// Add any equal or more work headers to setBlockIndexCandidates
auto candidate_it = candidate_blocks_by_work.lower_bound(invalid_walk_tip->pprev->nChainWork);
while (candidate_it != candidate_blocks_by_work.end()) {
if (!CBlockIndexWorkComparator()(candidate_it->second, invalid_walk_tip->pprev)) {
setBlockIndexCandidates.insert(candidate_it->second);
candidate_it = candidate_blocks_by_work.erase(candidate_it);
Comment thread
mzumsande marked this conversation as resolved.
Outdated
} else {
++candidate_it;
// Mark out-of-chain descendants of the invalidated block as invalid

@ryanofsky ryanofsky Jun 6, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "validation: in invalidateblock, mark children as invalid right away" (328345d571d9b86af54e915b30183b91d28cab2f)

Would seem more accurate to say this code is marking out-of-chain descendants of the "disconnected block" rather than the "invalidated block" since the invalidated block sounds like the block passed to invalidateblock

// (possibly replacing a pre-existing BLOCK_FAILED_VALID with BLOCK_FAILED_CHILD)
// Add any equal or more work headers that are not invalidated to setBlockIndexCandidates
// Recalculate m_best_header if it became invalid.

@ryanofsky ryanofsky Jun 6, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "validation: in invalidateblock, calculate m_best_header right away" (809413fde6893e60078e02b401a9d98a3e341fc1)

Commit message says "Before, m_best_header would be calculated only after disconnecting multiple blocks" but I'm actually not seeing where this was happening, though I think it must have been happening?

Could help if commit message was clarified (or not if I'm just missing something obvious).

@stickies-v stickies-v Jun 9, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but I'm actually not seeing where this was happening

There's a InvalidChainFound(to_mark_failed) call near the end of InvalidateBlock(), which in turn calls RecalculateBestHeader().

@ryanofsky ryanofsky Jun 9, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #31405 (comment)

In commit "validation: in invalidateblock, calculate m_best_header right away" (9a70883)

Thanks! It looks like the later RecalculateBestHeader() call is now unnecessary and could repeat the work this new code is doing, which doesn't seem great from perspective of duplicating code. InvalidChainFound also seems to be updating
m_best_invalid and m_best_header together while the new code is only updating the latter, so that seems a bit fragile.

I wonder if maybe instead of adding the new code in this commit it might make more sense to just move the InvalidateBlock call above the CheckBlockIndex call?

@mzumsande mzumsande Jun 9, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if maybe instead of adding the new code in this commit it might make more sense to just move the InvalidateBlock call above the CheckBlockIndex call?

You probably meant to suggest moving the InvalidChainFound call.

I think that wouldn't be ideal:
We could put it into the loop (before cs_main is released) and remove all other changed to InvalidateBlock of this PR, but then we'd loop over the entire block index with each iteration, which I'd like to to avoid for performance reasons (e.g. artificial rewinds for ten-thousands of block for AssumeUtxo parameter determination).

If we put it directly before the CheckBlockIndex call, the block index would be in an inconsistent state after each release of cs_main in the main loop, so that other threads can call CheckBlockIndex() (this happens, for example, if a new header is received via p2p, see ProcessNewBlockHeaders).

Maybe one possibility would be to add a parameter to InvalidChainFound to skip RecalculateBestHeader and SetBlockFailureFlags (because I think we want to keep setting of m_best_invalid,the logging, and maybe also CheckForkWarningConditions).

@ryanofsky ryanofsky Jun 10, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #31405 (comment)

In commit "validation: in invalidateblock, calculate m_best_header right away" (9a70883)

Thanks. I was just thinking of moving it right before the CheckBlockIndex call, but didn't think about the fact that CheckBlockIndex could be called by other threads during this time.

Maybe one possibility would be to add a parameter to InvalidChainFound to skip RecalculateBestHeader

Yes, this would be helpful especially if it could allow moving the InvalidChainFound call inside the loop instead of after it. Current code only updating m_best_header inside the loop before cs_main is released, and updating both m_best_header and m_best_invalid afterward with no comments about the redundancy or rationale seems fragile.

@mzumsande mzumsande Jun 10, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll look into this for a follow-up. InvalidChainFound logs inconditionally, so just moving it into the loop could be too verbose when disconnecting thousands of blocks.

auto candidate_it = highpow_outofchain_headers.lower_bound(invalid_walk_tip->pprev->nChainWork);
Comment thread
mzumsande marked this conversation as resolved.
Outdated

const bool best_header_needs_update{m_chainman.m_best_header->GetAncestor(invalid_walk_tip->nHeight) == invalid_walk_tip};
if (best_header_needs_update) {
// pprev is definitely still valid at this point, but there may be better ones

@ryanofsky ryanofsky Jun 6, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "validation: in invalidateblock, calculate m_best_header right away" (809413fde6893e60078e02b401a9d98a3e341fc1)

Maybe replace "but there may be better ones" with "but there could be a better header, which will be found in the loop below", because current comment seems to imply that the code will not actually find the best header.

m_chainman.m_best_header = invalid_walk_tip->pprev;
}

while (candidate_it != highpow_outofchain_headers.end()) {
CBlockIndex* candidate{candidate_it->second};
if (candidate->GetAncestor(invalid_walk_tip->nHeight) == invalid_walk_tip) {
// Children of failed blocks should be marked as BLOCK_FAILED_CHILD instead.
candidate->nStatus &= ~BLOCK_FAILED_VALID;
candidate->nStatus |= BLOCK_FAILED_CHILD;
m_blockman.m_dirty_blockindex.insert(candidate);
// If invalidated, the block is irrelevant for setBlockIndexCandidates
// and for m_best_header and can be removed from the cache.
candidate_it = highpow_outofchain_headers.erase(candidate_it);
continue;
}
if (!CBlockIndexWorkComparator()(candidate, invalid_walk_tip->pprev) &&
candidate->IsValid(BLOCK_VALID_TRANSACTIONS) &&
candidate->HaveNumChainTxs()) {
setBlockIndexCandidates.insert(candidate);
// Do not remove candidate from the highpow_outofchain_headers cache, because it might be a descendant of the block being invalidated
// which needs to be marked failed later.
}
if (best_header_needs_update &&
m_chainman.m_best_header->nChainWork < candidate->nChainWork) {

@ryanofsky ryanofsky Jun 6, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "validation: in invalidateblock, calculate m_best_header right away" (809413fde6893e60078e02b401a9d98a3e341fc1)

It seems like it could be better to use CBlockIndexWorkComparator here instead of relying highpow_outofchain_headers multimap ordering when choosing m_best_header and two headers have the same mount of work because CBlockIndexWorkComparator should be more deterministic and consistent with surrounding code, while map order will depend on iteration order of the m_block_index unordered set.

EDIT: Never mind probably. It looks like RecalculateBestHeader is also relying on m_block_index iteration order so this is consistent with that. Could be helpful to just add a comment about intent though.

@sedited sedited Jun 10, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like a comment here too.

@mzumsande mzumsande Jun 10, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add one in a follow-up if I don't need to push.

m_chainman.m_best_header = candidate;
}
++candidate_it;
}

// Track the last disconnected block, so we can correct its BLOCK_FAILED_CHILD status in future
Expand All @@ -3784,7 +3809,6 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
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
Expand Down Expand Up @@ -3852,7 +3876,6 @@ void Chainstate::ResetBlockFailureFlags(CBlockIndex *pindex) {
// Reset invalid block marker if it was pointing to one of those.
m_chainman.m_best_invalid = nullptr;
}
m_chainman.m_failed_blocks.erase(&block_index);
}
}

Expand All @@ -3861,7 +3884,6 @@ void Chainstate::ResetBlockFailureFlags(CBlockIndex *pindex) {
if (pindex->nStatus & BLOCK_FAILED_MASK) {
pindex->nStatus &= ~BLOCK_FAILED_MASK;
m_blockman.m_dirty_blockindex.insert(pindex);
m_chainman.m_failed_blocks.erase(pindex);
}
pindex = pindex->pprev;
}
Expand Down Expand Up @@ -4357,45 +4379,6 @@ bool ChainstateManager::AcceptBlockHeader(const CBlockHeader& block, BlockValida
LogDebug(BCLog::VALIDATION, "%s: Consensus::ContextualCheckBlockHeader: %s, %s\n", __func__, hash.ToString(), state.ToString());
return false;
}

/* Determine if this block descends from any block which has been found
* invalid (m_failed_blocks), then mark pindexPrev and any blocks between
* them as failed. For example:
*
* D3
* /
* B2 - C2
* / \
* A D2 - E2 - F2
* \
* B1 - C1 - D1 - E1
*
* In the case that we attempted to reorg from E1 to F2, only to find
* C2 to be invalid, we would mark D2, E2, and F2 as BLOCK_FAILED_CHILD
* but NOT D3 (it was not in any of our candidate sets at the time).
*
* In any case D3 will also be marked as BLOCK_FAILED_CHILD at restart
* in LoadBlockIndex.
*/
if (!pindexPrev->IsValid(BLOCK_VALID_SCRIPTS)) {
// The above does not mean "invalid": it checks if the previous block
// hasn't been validated up to BLOCK_VALID_SCRIPTS. This is a performance
// optimization, in the common case of adding a new block to the tip,
// we don't need to iterate over the failed blocks list.
for (const CBlockIndex* failedit : m_failed_blocks) {
Comment thread
ryanofsky marked this conversation as resolved.
Outdated
if (pindexPrev->GetAncestor(failedit->nHeight) == failedit) {
assert(failedit->nStatus & BLOCK_FAILED_VALID);
CBlockIndex* invalid_walk = pindexPrev;
while (invalid_walk != failedit) {
invalid_walk->nStatus |= BLOCK_FAILED_CHILD;
m_blockman.m_dirty_blockindex.insert(invalid_walk);
invalid_walk = invalid_walk->pprev;
}
LogDebug(BCLog::VALIDATION, "header %s has prev block invalid: %s\n", hash.ToString(), block.hashPrevBlock.ToString());
return state.Invalid(BlockValidationResult::BLOCK_INVALID_PREV, "bad-prevblk");
}
}
}
}
if (!min_pow_checked) {
LogDebug(BCLog::VALIDATION, "%s: not adding new block header %s, missing anti-dos proof-of-work validation\n", __func__, hash.ToString());
Expand Down Expand Up @@ -4537,9 +4520,8 @@ bool ChainstateManager::AcceptBlock(const std::shared_ptr<const CBlock>& pblock,

if (!CheckBlock(block, state, params.GetConsensus()) ||
!ContextualCheckBlock(block, state, *this, pindex->pprev)) {
if (state.IsInvalid() && state.GetResult() != BlockValidationResult::BLOCK_MUTATED) {
pindex->nStatus |= BLOCK_FAILED_VALID;
m_blockman.m_dirty_blockindex.insert(pindex);
if (Assume(state.IsInvalid())) {
ActiveChainstate().InvalidBlockFound(pindex, state);
}
LogError("%s: %s\n", __func__, state.ToString());
return false;
Expand Down Expand Up @@ -5280,6 +5262,7 @@ void ChainstateManager::CheckBlockIndex()
// are not yet validated.
CChain best_hdr_chain;
assert(m_best_header);
assert(!(m_best_header->nStatus & BLOCK_FAILED_MASK));
best_hdr_chain.SetTip(*m_best_header);

std::multimap<CBlockIndex*,CBlockIndex*> forward;
Expand Down Expand Up @@ -5393,6 +5376,8 @@ void ChainstateManager::CheckBlockIndex()
if (pindexFirstInvalid == nullptr) {
// Checks for not-invalid blocks.
assert((pindex->nStatus & BLOCK_FAILED_MASK) == 0); // The failed mask cannot be set for blocks without invalid parents.
} else {
Comment thread
mzumsande marked this conversation as resolved.
Outdated
assert(pindex->nStatus & BLOCK_FAILED_MASK); // Invalid blocks and their descendants must be marked as invalid
}
// Make sure m_chain_tx_count sum is correctly computed.
if (!pindex->pprev) {
Expand All @@ -5406,6 +5391,8 @@ void ChainstateManager::CheckBlockIndex()
// block, and must be set if it is.
assert((pindex->m_chain_tx_count != 0) == (pindex == snap_base));
}
// There should be no block with more work than m_best_header, unless it's known to be invalid
assert((pindex->nStatus & BLOCK_FAILED_MASK) || pindex->nChainWork <= m_best_header->nChainWork);

// Chainstate-specific checks on setBlockIndexCandidates
for (auto c : GetAll()) {
Expand Down
26 changes: 4 additions & 22 deletions src/validation.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1038,28 +1038,10 @@ class ChainstateManager
}


/**
* In order to efficiently track invalidity of headers, we keep the set of
* blocks which we tried to connect and found to be invalid here (ie which
* were set to BLOCK_FAILED_VALID since the last restart). We can then
* walk this set and check if a new header is a descendant of something in
* this set, preventing us from having to walk m_block_index when we try
* to connect a bad block and fail.
*
* While this is more complicated than marking everything which descends
* from an invalid block as invalid at the time we discover it to be
* invalid, doing so would require walking all of m_block_index to find all
* descendants. Since this case should be very rare, keeping track of all
* BLOCK_FAILED_VALID blocks in a set should be just fine and work just as
* well.
*
* Because we already walk m_block_index in height-order at startup, we go
* ahead and mark descendants of invalid blocks as FAILED_CHILD at that time,
* instead of putting things in this set.
*/
std::set<CBlockIndex*> m_failed_blocks;

/** Best header we've seen so far (used for getheaders queries' starting points). */
/** Best header we've seen so far for which the block is not known to be invalid
(used, among others, for getheaders queries' starting points).
In case of multiple best headers with the same work, it could point to any
because CBlockIndexWorkComparator tiebreaker rules are not applied. */
CBlockIndex* m_best_header GUARDED_BY(::cs_main){nullptr};

//! The total number of bytes available for us to use across all in-memory
Expand Down
Loading