Merge #13930: doc: Better explain GetAncestor check for m_failed_blocks in AcceptBlockHeader

66e15e8f97 Explain GetAncestor check for m_failed_blocks in AcceptBlockHeader (Sjors Provoost)

Pull request description:

  Salvaged (but slightly modified) from #12138, the comment there was really helpful to wrap my head around that part of the code.

  In addition, a naive reader like yours truly will first think `IsValid(BLOCK_VALID_SCRIPTS)` means the previous block was invalid. But IIUC that's not what  it means. Instead, it means the block hasn't been checked for validity at the `BLOCK_VALID_SCRIPTS` level yet. So in that case the existing text "previous block index isn't valid" is wrong.

Tree-SHA512: 442a319a83290d94697fdf51376463b70454e0f3909d4a45594ddc2e7c26cd19dc703808385a25e26d6d2dddab0aa35ca41722f2e65ee6fe57bbaf62652d3ec8

src/validation.cpp

@@ -3374,10 +3374,30 @@ bool CChainState::AcceptBlockHeader(const CBlockHeader& block, CValidationState&
         if (!ContextualCheckBlockHeader(block, state, chainparams, pindexPrev, GetAdjustedTime()))
             return error("%s: Consensus::ContextualCheckBlockHeader: %s, %s", __func__, hash.ToString(), FormatStateMessage(state));
 
-        // If the previous block index isn't valid, determine if it descends from any block which
+        /* Determine if this block descends from any block which has been found
-        // has been found invalid (m_failed_blocks), then mark pindexPrev and any blocks
+         * invalid (m_failed_blocks), then mark pindexPrev and any blocks between
-        // between them as failed.
+         * 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) {
                 if (pindexPrev->GetAncestor(failedit->nHeight) == failedit) {
                     assert(failedit->nStatus & BLOCK_FAILED_VALID);