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

Code before:

    CBlockIndex *pindexDummy = nullptr;
    CBlockIndex *&pindex = ppindex ? *ppindex : pindexDummy;

    bool accepted_header{AcceptBlockHeader(block, state, &pindex, min_pow_checked)};
    CheckBlockIndex();

    if (!accepted_header)
        return false;

    // Check all requested blocks that we do not already have for validity and
    // save them to disk. Skip processing of unrequested blocks as an anti-DoS
    // measure, unless the blocks have more work than the active chain tip, and
    // aren't too far ahead of it, so are likely to be attached soon.
    bool fAlreadyHave = pindex->nStatus & BLOCK_HAVE_DATA;
    bool fHasMoreOrSameWork = (ActiveTip() ? pindex->nChainWork >= ActiveTip()->nChainWork : true);
    // Blocks that are too out-of-order needlessly limit the effectiveness of
    // pruning, because pruning will not delete block files that contain any
    // blocks which are too close in height to the tip.  Apply this test
    // regardless of whether pruning is enabled; it should generally be safe to
    // not process unrequested blocks.
    bool fTooFarAhead{pindex->nHeight > ActiveHeight() + int(MIN_BLOCKS_TO_KEEP)};

    // TODO: Decouple this function from the block download logic by removing fRequested
    // This requires some new chain data structure to efficiently look up if a
    // block is in a chain leading to a candidate for best tip, despite not
    // being such a candidate itself.
    // Note that this would break the getblockfrompeer RPC

    // TODO: deal better with return value and error conditions for duplicate
    // and unrequested blocks.
    if (fAlreadyHave) return true;
    if (!fRequested) {  // If we didn't ask for it:
        if (pindex->nTx != 0) return true;    // This is a previously-processed block that was pruned
        if (!fHasMoreOrSameWork) return true; // Don't process less-work chains
        if (fTooFarAhead) return true;        // Block height is too high

        // Protect against DoS attacks from low-work chains.
        // If our tip is behind, a peer could try to send us
        // low-work blocks on a fake chain that we would never
        // request; don't process these.
        if (pindex->nChainWork < MinimumChainWork()) return true;
    }

    const CChainParams& params{GetParams()};

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

Review comment (bug):
My understanding is that `state.GetResult() != BlockValidationResult::BLOCK_MUTATED` is equivalent to `state.GetResult() == BlockValidationResult::BLOCK_CONSENSUS` here, because none of the other enum values are returned outside of block header validation, and at this point we've already passed `AcceptBlockHeader()`.

I tested this by adding an `Assume` and running the unit and functional test suite with it:

```cpp
...
            Assume(state.GetResult() == BlockValidationResult::BLOCK_CONSENSUS);
            ActiveChainstate().InvalidChainFound(pindex);
...
```

Understanding the DoS safety (as documented in the 163b01eae82a9dd94dcda94cf13001e46b4a8a56 commit message) is fairly straightforward from your documentation, but I spent most time figuring out whether the `InvalidChainFound()` call would be safe e.g. in the case of a `BLOCK_TIME_FUTURE` too. If my understanding that it must be a `BLOCK_CONSENSUS` failure at this point, I think adding the `Assume` and/or updating the commit message to reflect that might be helpful?

Code after:

    // TODO: Decouple this function from the block download logic by removing fRequested
    // This requires some new chain data structure to efficiently look up if a
    // block is in a chain leading to a candidate for best tip, despite not
    // being such a candidate itself.
    // Note that this would break the getblockfrompeer RPC

    // TODO: deal better with return value and error conditions for duplicate
    // and unrequested blocks.
    if (fAlreadyHave) return true;
    if (!fRequested) {  // If we didn't ask for it:
        if (pindex->nTx != 0) return true;    // This is a previously-processed block that was pruned
        if (!fHasMoreOrSameWork) return true; // Don't process less-work chains
        if (fTooFarAhead) return true;        // Block height is too high

        // Protect against DoS attacks from low-work chains.
        // If our tip is behind, a peer could try to send us
        // low-work blocks on a fake chain that we would never
        // request; don't process these.
        if (pindex->nChainWork < MinimumChainWork()) return true;
    }

    const CChainParams& params{GetParams()};

    if (!CheckBlock(block, state, params.GetConsensus()) ||
        !ContextualCheckBlock(block, state, *this, pindex->pprev)) {
        if (Assume(state.IsInvalid())) {
            ActiveChainstate().InvalidBlockFound(pindex, state);
        }
        LogError("%s: %s\n", __func__, state.ToString());
        return false;
    }

    // Header is valid/has work, merkle tree and segwit merkle tree are good...RELAY NOW
    // (but if it does not build on our best tip, let the SendMessages loop relay it)
    if (!IsInitialBlockDownload() && ActiveTip() == pindex->pprev && m_options.signals) {
        m_options.signals->NewPoWValidBlock(pindex, pblock);
    }

    // Write block to history file
    if (fNewBlock) *fNewBlock = true;
    try {
        FlatFilePos blockPos{};
        if (dbp) {
            blockPos = *dbp;
            m_blockman.UpdateBlockInfo(block, pindex->nHeight, blockPos);
        } else {
            blockPos = m_blockman.WriteBlock(block, pindex->nHeight);
            if (blockPos.IsNull()) {
                state.Error(strprintf("%s: Failed to find position to write new block to disk", __func__));
                return false;