Repository: bitcoin/bitcoin
File: src/validation.cpp (C++)

Code before:
        }

        // 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
        // setBlockIndexCandidates may have missed entries. This would
        // technically be an inconsistency in the block index, but if we clean
        // it up here, this should be an essentially unobservable error.
        // Loop back over all block index entries and add any missing entries
        // to setBlockIndexCandidates.
        for (auto& [_, block_index] : m_blockman.m_block_index) {
            if (block_index.IsValid(BLOCK_VALID_TRANSACTIONS) && block_index.HaveNumChainTxs() && !setBlockIndexCandidates.value_comp()(&block_index, m_chain.Tip())) {
                setBlockIndexCandidates.insert(&block_index);
            }
        }

        InvalidChainFound(to_mark_failed);
    }

    // Only notify about a new block tip if the active chain was modified.
    if (pindex_was_in_chain) {
        // Ignoring return value for now, this could be changed to bubble up
        // kernel::Interrupted value to the caller so the caller could
        // distinguish between completed and interrupted operations. It might
        // also make sense for the blockTip notification to have an enum
        // parameter indicating the source of the tip change so hooks can
        // distinguish user-initiated invalidateblock changes from other
        // changes.
        (void)m_chainman.GetNotifications().blockTip(
            /*state=*/GetSynchronizationState(m_chainman.IsInitialBlockDownload(), m_chainman.m_blockman.m_blockfiles_indexed),
            /*index=*/*to_mark_failed->pprev,
            /*verification_progress=*/WITH_LOCK(m_chainman.GetMutex(), return m_chainman.GuessVerificationProgress(to_mark_failed->pprev)));

        // Fire ActiveTipChange now for the current chain tip to make sure clients are notified.
        // ActivateBestChain may call this as well, but not necessarily.
        if (m_chainman.m_options.signals) {
            m_chainman.m_options.signals->ActiveTipChange(*Assert(m_chain.Tip()), m_chainman.IsInitialBlockDownload());
        }
    }
    return true;
}

void Chainstate::SetBlockFailureFlags(CBlockIndex* invalid_block)
{

Review comment (suggestion):
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.

Code after:
        if (best_header_needs_update) {
            // pprev is definitely still valid at this point, but there may be better ones
            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) {
                m_chainman.m_best_header = candidate;
            }
            ++candidate_it;
        }

        // Track the last disconnected block, so we can correct its BLOCK_FAILED_CHILD status in future
        // iterations, or, if it's the last one, call InvalidChainFound on it.
        to_mark_failed = invalid_walk_tip;
    }

    m_chainman.CheckBlockIndex();

    {
        LOCK(cs_main);
        if (m_chain.Contains(to_mark_failed)) {
            // If the to-be-marked invalid block is in the active chain, something is interfering and we can't proceed.
            return false;
        }

        // 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);
        }