Why is CheckBlock called twice when processing a new block?

Question

Once the Bitcoin node receives a message with a new block, it calls the ProcessNewBlock function:

 else if (strCommand == "block" && !fImporting && !fReindex) // Ignore blocks received while importing
{
    CBlock block;
    vRecv >> block;

    CInv inv(MSG_BLOCK, block.GetHash());
    LogPrint("net", "received block %s peer=%d\n", inv.hash.ToString(), pfrom->id);

    pfrom->AddInventoryKnown(inv);

    CValidationState state;
    // Process all blocks from whitelisted peers, even if not requested,
    // unless we're still syncing with the network.
    // Such an unrequested block may still be processed, subject to the
    // conditions in AcceptBlock().
    bool forceProcessing = pfrom->fWhitelisted && !IsInitialBlockDownload();
    ProcessNewBlock(state, pfrom, &block, forceProcessing, NULL);
    int nDoS;
    if (state.IsInvalid(nDoS)) {
        assert (state.GetRejectCode() < REJECT_INTERNAL); // Blocks are never rejected with internal reject codes
        pfrom->PushMessage("reject", strCommand, state.GetRejectCode(),
                           state.GetRejectReason().substr(0, MAX_REJECT_MESSAGE_LENGTH), inv.hash);
        if (nDoS > 0) {
            LOCK(cs_main);
            Misbehaving(pfrom->GetId(), nDoS);
        }
    }

Inside this function, CheckBlock and AcceptBlock are called:

// Preliminary checks
bool checked = CheckBlock(*pblock, state);

{
    LOCK(cs_main);
    bool fRequested = MarkBlockAsReceived(pblock->GetHash());
    fRequested |= fForceProcessing;
    if (!checked) {
        return error("%s: CheckBlock FAILED", __func__);
    }

    // Store to disk
    CBlockIndex *pindex = NULL;
    bool ret = AcceptBlock(*pblock, state, &pindex, fRequested, dbp);
    if (pindex && pfrom) {
        mapBlockSource[pindex->GetBlockHash()] = pfrom->GetId();
    }
    CheckBlockIndex();
    if (!ret)
        return error("%s: AcceptBlock FAILED", __func__);
}

Inside the AcceptBlock function CheckBlock is called again:

if ((!CheckBlock(block, state)) || !ContextualCheckBlock(block, state, pindex->pprev)) {
    if (state.IsInvalid() && !state.CorruptionPossible()) {
        pindex->nStatus |= BLOCK_FAILED_VALID;
        setDirtyBlockIndex.insert(pindex);
    }
    return false;

Why is the CheckBlock function called two times when processing a new block?

Answer 1

I suspect this is an artifact of the way the code used to work. As to why it hasn't been taken out, it's probably just out of caution, or a "Don't fix it if it's not broken" mindset. Bitcoin Core coders are very cautious, and reasonably so since it's a piece of software that is the main interface to the network for many big players in the industry.

In addition, on master right now, CheckBlock contains a check at the beginning to avoid doing the same work twice. (Added in this commit.)

if (block.fChecked)
    return true;