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

Code before:
        // This setting doesn't force the selection of any particular chain but makes validating some faster by
        //  effectively caching the result of part of the verification.
        BlockMap::const_iterator it{m_blockman.m_block_index.find(m_chainman.AssumedValidBlock())};
        if (it != m_blockman.m_block_index.end()) {
            if (it->second.GetAncestor(pindex->nHeight) == pindex &&
                m_chainman.m_best_header->GetAncestor(pindex->nHeight) == pindex &&
                m_chainman.m_best_header->nChainWork >= m_chainman.MinimumChainWork()) {
                // This block is a member of the assumed verified chain and an ancestor of the best header.
                // Script verification is skipped when connecting blocks under the
                // assumevalid block. Assuming the assumevalid block is valid this
                // is safe because block merkle hashes are still computed and checked,
                // Of course, if an assumed valid block is invalid due to false scriptSigs
                // this optimization would allow an invalid chain to be accepted.
                // The equivalent time check discourages hash power from extorting the network via DOS attack
                //  into accepting an invalid block through telling users they must manually set assumevalid.
                //  Requiring a software change or burying the invalid block, regardless of the setting, makes
                //  it hard to hide the implication of the demand.  This also avoids having release candidates
                //  that are hardly doing any signature verification at all in testing without having to
                //  artificially set the default assumed verified block further back.
                // The test against the minimum chain work prevents the skipping when denied access to any chain at
                //  least as good as the expected chain.
                fScriptChecks = (GetBlockProofEquivalentTime(*m_chainman.m_best_header, *pindex, *m_chainman.m_best_header, params.GetConsensus()) <= 60 * 60 * 24 * 7 * 2);
            }
        }
    }

    const auto time_1{SteadyClock::now()};
    m_chainman.time_check += time_1 - time_start;
    LogDebug(BCLog::BENCH, "    - Sanity checks: %.2fms [%.2fs (%.2fms/blk)]\n",
             Ticks<MillisecondsDouble>(time_1 - time_start),
             Ticks<SecondsDouble>(m_chainman.time_check),
             Ticks<MillisecondsDouble>(m_chainman.time_check) / m_chainman.num_blocks_total);

    // Do not allow blocks that contain transactions which 'overwrite' older transactions,
    // unless those are already completely spent.
    // If such overwrites are allowed, coinbases and transactions depending upon those
    // can be duplicated to remove the ability to spend the first instance -- even after
    // being sent to another address.
    // See BIP30, CVE-2012-1909, and https://r6.ca/blog/20120206T005236Z.html for more information.
    // This rule was originally applied to all blocks with a timestamp after March 15, 2012, 0:00 UTC.
    // Now that the whole chain is irreversibly beyond that time it is applied to all blocks except the
    // two in the chain that violate it. This prevents exploiting the issue against nodes during their
    // initial block download.
    bool fEnforceBIP30 = !IsBIP30Repeat(*pindex);

    // Once BIP34 activated it was not possible to create new duplicate coinbases and thus other than starting
    // with the 2 existing duplicate coinbase pairs, not possible to create overwriting txs.  But by the
    // time BIP34 activated, in each of the existing pairs the duplicate coinbase had overwritten the first
    // before the first had been spent.  Since those coinbases are sufficiently buried it's no longer possible to create further
    // duplicate transactions descending from the known pairs either.
    // If we're on the known chain at height greater than where BIP34 activated, we can save the db accesses needed for the BIP30 check.

Review comment (bug):
If we get here, we know from the previous check (`if (it->second.GetAncestor(pindex->nHeight) != pindex) assume_valid = CHECKED_NOT_UNDER_ASSUMEVALID`) that `pindex` now *does exist* below the assumevalid header (or *is* the assumevalid block).

For the check against best header chain to fail for `pindex`, it could either be:

1. The best header height somehow being below both `pindex` *and assumevalid*. (Edit: I guess this could theoretically happen if a really strong miner ramped up the difficulty and mined a fork of the chain from below the assumevalid height, but still with more total chain work).
2. The best header chain being a totally different chain that doesn't even include the assumevalid hash (unless someone has broken SHA256).

Why would we want to do script checking of the `pindex` block which is in the chain below the assumevalid hash for any of those cases? This is keeping the existing behavior on master, but that behavior is weird to me.

I guess this is also why you've had issues adding a test for `CHECKED_OFF_BESTHEADER_PATH`.

Code after:
        // We've been configured with the hash of a block which has been externally verified to have a valid history.
        // A suitable default value is included with the software and updated from time to time.  Because validity
        //  relative to a piece of software is an objective fact these defaults can be easily reviewed.
        // This setting doesn't force the selection of any particular chain but makes validating some faster by
        //  effectively caching the result of part of the verification.
        BlockMap::const_iterator it{m_blockman.m_block_index.find(m_chainman.AssumedValidBlock())};
        if (it == m_blockman.m_block_index.end()) {
            script_check_reason = "assumevalid hash not in headers";
        } else if (it->second.GetAncestor(pindex->nHeight) != pindex) {
            script_check_reason = (pindex->nHeight > it->second.nHeight) ? "block height above assumevalid height" : "block not in assumevalid chain";
        } else if (m_chainman.m_best_header->GetAncestor(pindex->nHeight) != pindex) {
            script_check_reason = "block not in best header chain";
        } else if (m_chainman.m_best_header->nChainWork < m_chainman.MinimumChainWork()) {
            script_check_reason = "best header chainwork below minimumchainwork";
        } else if (GetBlockProofEquivalentTime(*m_chainman.m_best_header, *pindex, *m_chainman.m_best_header, params.GetConsensus()) <= TWO_WEEKS_IN_SECONDS) {
            script_check_reason = "block too recent relative to best header";
        } else {
            // This block is a member of the assumed verified chain and an ancestor of the best header.
            // Script verification is skipped when connecting blocks under the
            //  assumevalid block. Assuming the assumevalid block is valid this
            //  is safe because block merkle hashes are still computed and checked,
            // Of course, if an assumed valid block is invalid due to false scriptSigs
            //  this optimization would allow an invalid chain to be accepted.
            // The equivalent time check discourages hash power from extorting the network via DOS attack
            //  into accepting an invalid block through telling users they must manually set assumevalid.
            //  Requiring a software change or burying the invalid block, regardless of the setting, makes
            //  it hard to hide the implication of the demand. This also avoids having release candidates
            //  that are hardly doing any signature verification at all in testing without having to
            //  artificially set the default assumed verified block further back.
            // The test against the minimum chain work prevents the skipping when denied access to any chain at
            //  least as good as the expected chain.
            script_check_reason = nullptr;
        }
    }

    const auto time_1{SteadyClock::now()};
    m_chainman.time_check += time_1 - time_start;
    LogDebug(BCLog::BENCH, "    - Sanity checks: %.2fms [%.2fs (%.2fms/blk)]\n",
             Ticks<MillisecondsDouble>(time_1 - time_start),
             Ticks<SecondsDouble>(m_chainman.time_check),
             Ticks<MillisecondsDouble>(m_chainman.time_check) / m_chainman.num_blocks_total);

    // Do not allow blocks that contain transactions which 'overwrite' older transactions,
    // unless those are already completely spent.
    // If such overwrites are allowed, coinbases and transactions depending upon those
    // can be duplicated to remove the ability to spend the first instance -- even after
    // being sent to another address.
    // See BIP30, CVE-2012-1909, and https://r6.ca/blog/20120206T005236Z.html for more information.
    // This rule was originally applied to all blocks with a timestamp after March 15, 2012, 0:00 UTC.
    // Now that the whole chain is irreversibly beyond that time it is applied to all blocks except the
    // two in the chain that violate it. This prevents exploiting the issue against nodes during their