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

Code before:
        if (state.GetResult() == BlockValidationResult::BLOCK_MUTATED) {
            // We don't write down blocks to disk if they may have been
            // corrupted, so this should be impossible unless we're having hardware
            // problems.
            return FatalError(m_chainman.GetNotifications(), state, _("Corrupt block found indicating potential hardware failure."));
        }
        LogError("%s: Consensus::CheckBlock: %s\n", __func__, state.ToString());
        return false;
    }

    // verify that the view's current state corresponds to the previous block
    uint256 hashPrevBlock = pindex->pprev == nullptr ? uint256() : pindex->pprev->GetBlockHash();
    assert(hashPrevBlock == view.GetBestBlock());

    m_chainman.num_blocks_total++;

    // Special case for the genesis block, skipping connection of its transactions
    // (its coinbase is unspendable)
    if (block_hash == params.GetConsensus().hashGenesisBlock) {
        if (!fJustCheck)
            view.SetBestBlock(pindex->GetBlockHash());
        return true;
    }

    bool fScriptChecks = true;
    if (!m_chainman.AssumedValidBlock().IsNull()) {
        // 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()) {
            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);

Review comment (bug):
nit: I'd prefer avoiding having this dual state. Here are 2 variations which do that:

Preserving behavior:

```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 7e82c3bbad..008bfe4d71 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2423,8 +2423,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
         return true;
     }
 
-    auto fScriptChecks{true};
-    std::string script_check_reason;
+    const char* script_check_reason{nullptr};
     if (!m_chainman.AssumedValidBlock().IsNull()) {
         constexpr int64_t TWO_WEEKS_IN_SECONDS{60 * 60 * 24 * 7 * 2};
         // We've been configured with the hash of a block which has been externally verified to have a valid history.
@@ -2458,7 +2457,6 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
             //  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 = false;
         }
     } else {
         script_check_reason = "assumevalid=0 (always verify)";
@@ -2573,16 +2571,15 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
              Ticks<SecondsDouble>(m_chainman.time_forks),
              Ticks<MillisecondsDouble>(m_chainman.time_forks) / m_chainman.num_blocks_total);
 
-    if (fScriptChecks != m_prev_script_checks_logged && GetRole() == ChainstateRole::NORMAL) {
-        if (fScriptChecks) {
-            Assume(!script_check_reason.empty());
+    if ((script_check_reason != nullptr) != m_prev_script_checks_logged && GetRole() == ChainstateRole::NORMAL) {
+        if (script_check_reason) {
             LogInfo("Enabling script verification at block #%d (%s): %s.",
                     pindex->nHeight, block_hash.ToString(), script_check_reason);
         } else {
             LogInfo("Disabling script verification at block #%d (%s).",
                     pindex->nHeight, block_hash.ToString());
         }
-        m_prev_script_checks_logged = fScriptChecks;
+        m_prev_script_checks_logged = script_check_reason != nullptr;
     }
 
     CBlockUndo blockundo;
@@ -2593,7 +2590,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
     // doesn't invalidate pointers into the vector, and keep txsdata in scope
     // for as long as `control`.
     std::optional<CCheckQueueControl<CScriptCheck>> control;
-    if (auto& queue = m_chainman.GetCheckQueue(); queue.HasThreads() && fScriptChecks) control.emplace(queue);
+    if (auto& queue = m_chainman.GetCheckQueue(); queue.HasThreads() && script_check_reason != nullptr) control.emplace(queue);
 
     std::vector<PrecomputedTransactionData> txsdata(block.vtx.size());
 
@@ -2652,7 +2649,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
             break;
         }
 
-        if (!tx.IsCoinBase() && fScriptChecks)
+        if (!tx.IsCoinBase() && script_check_reason != nullptr)
         {
             bool fCacheResults = fJustCheck; /* Don't cache results if we're actually connecting blocks (still consult the cache, though) */
             bool tx_ok;
```

<details><summary>The below version will output more often, when switching reasons, not sure which behavior is more desirable (there is no risk of dangling pointers from using `const char*`, but it requires us to be vigilant going forward):</summary>

```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 7e82c3bbad..79ad71b64c 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2423,8 +2423,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
         return true;
     }
 
-    auto fScriptChecks{true};
-    std::string script_check_reason;
+    const char* script_check_reason{nullptr};
     if (!m_chainman.AssumedValidBlock().IsNull()) {
         constexpr int64_t TWO_WEEKS_IN_SECONDS{60 * 60 * 24 * 7 * 2};
         // We've been configured with the hash of a block which has been externally verified to have a valid history.
@@ -2458,7 +2457,6 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
             //  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 = false;
         }
     } else {
         script_check_reason = "assumevalid=0 (always verify)";
@@ -2573,16 +2571,15 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
              Ticks<SecondsDouble>(m_chainman.time_forks),
              Ticks<MillisecondsDouble>(m_chainman.time_forks) / m_chainman.num_blocks_total);
 
-    if (fScriptChecks != m_prev_script_checks_logged && GetRole() == ChainstateRole::NORMAL) {
-        if (fScriptChecks) {
-            Assume(!script_check_reason.empty());
+    if (script_check_reason != m_prev_script_check_reason && GetRole() == ChainstateRole::NORMAL) {
+        if (script_check_reason) {
             LogInfo("Enabling script verification at block #%d (%s): %s.",
                     pindex->nHeight, block_hash.ToString(), script_check_reason);
         } else {
             LogInfo("Disabling script verification at block #%d (%s).",
                     pindex->nHeight, block_hash.ToString());
         }
-        m_prev_script_checks_logged = fScriptChecks;
+        m_prev_script_check_reason = script_check_reason;
     }
 
     CBlockUndo blockundo;
@@ -2593,7 +2590,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
     // doesn't invalidate pointers into the vector, and keep txsdata in scope
     // for as long as `control`.
     std::optional<CCheckQueueControl<CScriptCheck>> control;
-    if (auto& queue = m_chainman.GetCheckQueue(); queue.HasThreads() && fScriptChecks) control.emplace(queue);
+    if (auto& queue = m_chainman.GetCheckQueue(); queue.HasThreads() && script_check_reason) control.emplace(queue);
 
     std::vector<PrecomputedTransactionData> txsdata(block.vtx.size());
 
@@ -2652,7 +2649,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
             break;
         }
 
-        if (!tx.IsCoinBase() && fScriptChecks)
+        if (!tx.IsCoinBase() && script_check_reason)
         {
             bool fCacheResults = fJustCheck; /* Don't cache results if we're actually connecting blocks (still consult the cache, though) */
             bool tx_ok;
diff --git a/src/validation.h b/src/validation.h
index fea11c5515..6e426a4660 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -560,7 +560,7 @@ protected:
     //! Cached result of LookupBlockIndex(*m_from_snapshot_blockhash)
     mutable const CBlockIndex* m_cached_snapshot_base GUARDED_BY(::cs_main){nullptr};
 
-    std::optional<bool> m_prev_script_checks_logged GUARDED_BY(::cs_main){};
+    std::optional<const char*> m_prev_script_check_reason GUARDED_BY(::cs_main){};
 
 public:
     //! Reference to a BlockManager instance which itself is shared across all
```
</details>

Code after:
        if (state.GetResult() == BlockValidationResult::BLOCK_MUTATED) {
            // We don't write down blocks to disk if they may have been
            // corrupted, so this should be impossible unless we're having hardware
            // problems.
            return FatalError(m_chainman.GetNotifications(), state, _("Corrupt block found indicating potential hardware failure."));
        }
        LogError("%s: Consensus::CheckBlock: %s\n", __func__, state.ToString());
        return false;
    }

    // verify that the view's current state corresponds to the previous block
    uint256 hashPrevBlock = pindex->pprev == nullptr ? uint256() : pindex->pprev->GetBlockHash();
    assert(hashPrevBlock == view.GetBestBlock());

    m_chainman.num_blocks_total++;

    // Special case for the genesis block, skipping connection of its transactions
    // (its coinbase is unspendable)
    if (block_hash == params.GetConsensus().hashGenesisBlock) {
        if (!fJustCheck)
            view.SetBestBlock(pindex->GetBlockHash());
        return true;
    }

    const char* script_check_reason;
    if (m_chainman.AssumedValidBlock().IsNull()) {
        script_check_reason = "assumevalid=0 (always verify)";
    } else {
        constexpr int64_t TWO_WEEKS_IN_SECONDS{60 * 60 * 24 * 7 * 2};
        // 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