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

Code before:
    // Do anti-DoS checks to determine if we should process or store for later
    // processing.
    if (!already_validated_work && TryLowWorkHeadersSync(peer, pfrom,
                chain_start_header, headers)) {
        // If we successfully started a low-work headers sync, then there
        // should be no headers to process any further.
        Assume(headers.empty());
        return;
    }

    // At this point, we have a set of headers with sufficient work on them
    // which can be processed.

    // If we don't have the last header, then this peer will have given us
    // something new (if these headers are valid).
    bool received_new_header{last_received_header == nullptr};

    // Now process all the headers.
    BlockValidationState state;
    const bool processed{m_chainman.ProcessNewBlockHeaders(headers,
                                                           /*min_pow_checked=*/true,
                                                           state, &pindexLast)};
    if (!processed) {
        if (state.IsInvalid()) {
            MaybePunishNodeForBlock(pfrom.GetId(), state, via_compact_block, "invalid header received");
            return;
        }
    }
    assert(pindexLast);

    if (processed && received_new_header) {
        LogBlockHeader(*pindexLast, pfrom, /*via_compact_block=*/false);
    }

    // Consider fetching more headers if we are not using our headers-sync mechanism.
    if (nCount == m_opts.max_headers_result && !have_headers_sync) {
        // Headers message had its maximum size; the peer may have more headers.
        if (MaybeSendGetHeaders(pfrom, GetLocator(pindexLast), peer)) {
            LogDebug(BCLog::NET, "more getheaders (%d) to end to peer=%d (startheight:%d)\n",
                    pindexLast->nHeight, pfrom.GetId(), peer.m_starting_height);
        }
    }

    UpdatePeerStateForReceivedHeaders(pfrom, peer, *pindexLast, received_new_header, nCount == m_opts.max_headers_result);

    // Consider immediately downloading blocks.
    HeadersDirectFetchBlocks(pfrom, peer, *pindexLast);

    return;
}


Review comment (performance):
I think there is potential for unconditional logging abuse here. If an inbound peer (which doesn't get disconnected for sending `BLOCK_CACHED_INVALID` headers) knows of (or spends the PoW to construct) an invalid block with a valid header, they can resend the same header and spam the log. The impacts should be limited since we now have disk filling mitigation in place, but it's still annoying at the very least, and it's very cheap to execute on the entire network when e.g. a miner accidentally constructs such a block.

Minimal test to showcase:

<details>
<summary>git diff on d55830cd50</summary>

```diff
diff --git a/test/functional/p2p_invalid_block.py b/test/functional/p2p_invalid_block.py
index 88bdbe22a8..c5d22c3382 100755
--- a/test/functional/p2p_invalid_block.py
+++ b/test/functional/p2p_invalid_block.py
@@ -104,6 +104,8 @@ class InvalidBlockRequestTest(BitcoinTestFramework):
         block3.solve()
 
         peer.send_blocks_and_test([block3], node, success=False, reject_reason='bad-cb-amount')
+        for i in range(100):
+            peer.send_blocks_and_test([block3], node, success=False, reject_reason='Received header for a block previously marked')
 
 
         # Complete testing of CVE-2012-2459 by sending the original block.

```
</details>


Code after:
    // Do anti-DoS checks to determine if we should process or store for later
    // processing.
    if (!already_validated_work && TryLowWorkHeadersSync(peer, pfrom,
                chain_start_header, headers)) {
        // If we successfully started a low-work headers sync, then there
        // should be no headers to process any further.
        Assume(headers.empty());
        return;
    }

    // At this point, we have a set of headers with sufficient work on them
    // which can be processed.

    // If we don't have the last header, then this peer will have given us
    // something new (if these headers are valid).
    bool received_new_header{last_received_header == nullptr};

    // Now process all the headers.
    BlockValidationState state;
    const bool processed{m_chainman.ProcessNewBlockHeaders(headers,
                                                           /*min_pow_checked=*/true,
                                                           state, &pindexLast)};
    if (!processed) {
        if (state.IsInvalid()) {
            if (!pfrom.IsInboundConn() && state.GetResult() == BlockValidationResult::BLOCK_CACHED_INVALID) {
                // Warn user if outgoing peers send us headers of blocks that we previously marked as invalid.
                LogWarning("%s (received from peer=%i). "
                           "If this happens with all peers, consider database corruption (that -reindex may fix) "
                           "or a potential consensus incompatibility.",
                           state.GetDebugMessage(), pfrom.GetId());
            }
            MaybePunishNodeForBlock(pfrom.GetId(), state, via_compact_block, "invalid header received");
            return;
        }
    }
    assert(pindexLast);

    if (processed && received_new_header) {
        LogBlockHeader(*pindexLast, pfrom, /*via_compact_block=*/false);
    }

    // Consider fetching more headers if we are not using our headers-sync mechanism.
    if (nCount == m_opts.max_headers_result && !have_headers_sync) {
        // Headers message had its maximum size; the peer may have more headers.
        if (MaybeSendGetHeaders(pfrom, GetLocator(pindexLast), peer)) {
            LogDebug(BCLog::NET, "more getheaders (%d) to end to peer=%d (startheight:%d)\n",
                    pindexLast->nHeight, pfrom.GetId(), peer.m_starting_height);
        }
    }

    UpdatePeerStateForReceivedHeaders(pfrom, peer, *pindexLast, received_new_header, nCount == m_opts.max_headers_result);