validation: refactor TestBlockValidity

A later commit adds checkBlock() to the Mining interface. In order to
avoid passing BlockValidationState over IPC, this commit first
refactors TestBlockValidity to return a boolean instead, and pass failure
reasons via a string.

Comments are expanded.

The ContextualCheckBlockHeader check is moved to after CheckBlock,
which is more similar to normal validation where context-free checks
are done first.

Validation failure reasons are no longer printed through LogError(),
since it depends on the caller whether this implies an actual bug
in the node, or an externally sourced block that happens to be invalid.
When called from getblocktemplate, via BlockAssembler::CreateNewBlock(),
this method already throws an std::runtime_error if validation fails.

Additionally it moves the inconclusive-not-best-prevblk check from RPC
code to TestBlockValidity.

There is no behavior change when callling getblocktemplate with proposal.
Previously this would return a BIP22ValidationResult which can throw for
state.IsError(). But CheckBlock() and the functions it calls only use
state.IsValid().

The final assert is changed into Assume, with a LogError.

src/node/miner.cpp

@@ -173,10 +173,10 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock()
     pblock->nBits          = GetNextWorkRequired(pindexPrev, pblock, chainparams.GetConsensus());
     pblock->nNonce         = 0;
 
-    BlockValidationState state;
-    if (m_options.test_block_validity && !TestBlockValidity(state, chainparams, m_chainstate, *pblock, pindexPrev,
-                                                            /*fCheckPOW=*/false, /*fCheckMerkleRoot=*/false)) {
-        throw std::runtime_error(strprintf("%s: TestBlockValidity failed: %s", __func__, state.ToString()));
+    std::string reason;
+    std::string debug;
+    if (m_options.test_block_validity && !TestBlockValidity(m_chainstate, *pblock, /*check_pow=*/false, /*check_merkle_root=*/false, reason, debug)) {
+        throw std::runtime_error(strprintf("TestBlockValidity failed: %s - %s", reason, debug));
     }
     const auto time_2{SteadyClock::now()};
 

src/rpc/mining.cpp

@@ -387,9 +387,10 @@ static RPCHelpMan generateblock()
         block.vtx.insert(block.vtx.end(), txs.begin(), txs.end());
         RegenerateCommitments(block, chainman);
 
-        BlockValidationState state;
-        if (!TestBlockValidity(state, chainman.GetParams(), chainman.ActiveChainstate(), block, chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock), /*fCheckPOW=*/false, /*fCheckMerkleRoot=*/false)) {
-            throw JSONRPCError(RPC_VERIFY_ERROR, strprintf("TestBlockValidity failed: %s", state.ToString()));
+        std::string reason;
+        std::string debug;
+        if (!TestBlockValidity(chainman.ActiveChainstate(), block, /*check_pow=*/false, /*check_merkle_root=*/false, reason, debug)) {
+            throw JSONRPCError(RPC_VERIFY_ERROR, strprintf("TestBlockValidity failed: %s - %s", reason, debug));
         }
     }
 
@@ -741,13 +742,12 @@ static RPCHelpMan getblocktemplate()
                 return "duplicate-inconclusive";
             }
 
-            // TestBlockValidity only supports blocks built on the current Tip
-            if (block.hashPrevBlock != tip) {
-                return "inconclusive-not-best-prevblk";
-            }
-            BlockValidationState state;
-            TestBlockValidity(state, chainman.GetParams(), chainman.ActiveChainstate(), block, chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock), /*fCheckPOW=*/false, /*fCheckMerkleRoot=*/true);
-            return BIP22ValidationResult(state);
+            std::string reason;
+            std::string debug;
+            bool res{TestBlockValidity(chainman.ActiveChainstate(), block, /*check_pow=*/false, /*check_merkle_root=*/true, reason, debug)};
+            if (res) return UniValue::VNULL;
+            LogDebug(BCLog::RPC, "Invalid block: %s", debug);
+            return UniValue{reason};
         }
 
         const UniValue& aClientRules = oparam.find_value("rules");

src/validation.cpp

@@ -4648,40 +4648,82 @@ MempoolAcceptResult ChainstateManager::ProcessTransaction(const CTransactionRef&
     return result;
 }
 
-bool TestBlockValidity(BlockValidationState& state,
-                       const CChainParams& chainparams,
-                       Chainstate& chainstate,
+bool TestBlockValidity(Chainstate& chainstate,
                        const CBlock& block,
-                       CBlockIndex* pindexPrev,
-                       bool fCheckPOW,
-                       bool fCheckMerkleRoot)
+                       const bool check_pow,
+                       const bool check_merkle_root,
+                       std::string& reason,
+                       std::string& debug)
 {
-    AssertLockHeld(cs_main);
-    assert(pindexPrev && pindexPrev == chainstate.m_chain.Tip());
-    CCoinsViewCache viewNew(&chainstate.CoinsTip());
-    uint256 block_hash(block.GetHash());
-    CBlockIndex indexDummy(block);
-    indexDummy.pprev = pindexPrev;
-    indexDummy.nHeight = pindexPrev->nHeight + 1;
-    indexDummy.phashBlock = &block_hash;
+    // Lock must be held throughout this function for two reasons:
+    // 1. We don't want the tip to change during several of the validation steps
+    // 2. To prevent a CheckBlock() race condition for fChecked, see ProcessNewBlock()
+    LOCK(chainstate.m_chainman.GetMutex());
 
-    // NOTE: CheckBlockHeader is called by CheckBlock
-    if (!ContextualCheckBlockHeader(block, state, chainstate.m_blockman, chainstate.m_chainman, pindexPrev)) {
-        LogError("%s: Consensus::ContextualCheckBlockHeader: %s\n", __func__, state.ToString());
+    BlockValidationState state;
+    CBlockIndex* tip{Assert(chainstate.m_chain.Tip())};
+
+    if (block.hashPrevBlock != *Assert(tip->phashBlock)) {
+        reason = "inconclusive-not-best-prevblk";
         return false;
     }
-    if (!CheckBlock(block, state, chainparams.GetConsensus(), fCheckPOW, fCheckMerkleRoot)) {
-        LogError("%s: Consensus::CheckBlock: %s\n", __func__, state.ToString());
+
+    // For signets CheckBlock() verifies the challenge iff fCheckPow is set.
+    if (!CheckBlock(block, state, chainstate.m_chainman.GetConsensus(), /*fCheckPow=*/check_pow, /*fCheckMerkleRoot=*/check_merkle_root)) {
+        reason = state.GetRejectReason();
+        debug = state.GetDebugMessage();
         return false;
     }
-    if (!ContextualCheckBlock(block, state, chainstate.m_chainman, pindexPrev)) {
-        LogError("%s: Consensus::ContextualCheckBlock: %s\n", __func__, state.ToString());
+
+    /**
+     * At this point ProcessNewBlock would call AcceptBlock(), but we
+     * don't want to store the block or its header. Run individual checks
+     * instead:
+     * - skip AcceptBlockHeader() because:
+     *   - we don't want to update the block index
+     *   - we do not care about duplicates
+     *   - we already ran CheckBlockHeader() via CheckBlock()
+     *   - we already checked for prev-blk-not-found
+     *   - we know the tip is valid, so no need to check bad-prevblk
+     * - we already ran CheckBlock()
+     * - do run ContextualCheckBlockHeader()
+     * - do run ContextualCheckBlock()
+     */
+
+    if (!ContextualCheckBlockHeader(block, state, chainstate.m_blockman, chainstate.m_chainman, tip)) {
+        reason = state.GetRejectReason();
+        debug = state.GetDebugMessage();
         return false;
     }
-    if (!chainstate.ConnectBlock(block, state, &indexDummy, viewNew, true)) {
+
+    if (!ContextualCheckBlock(block, state, chainstate.m_chainman, tip)) {
+        reason = state.GetRejectReason();
+        debug = state.GetDebugMessage();
+        return false;
+    }
+
+    // We don't want ConnectBlock to update the actual chainstate, so create
+    // a cache on top of it, along with a dummy block index.
+    CBlockIndex index_dummy{block};
+    uint256 block_hash(block.GetHash());
+    index_dummy.pprev = tip;
+    index_dummy.nHeight = tip->nHeight + 1;
+    index_dummy.phashBlock = &block_hash;
+    CCoinsViewCache tip_view(&chainstate.CoinsTip());
+    CCoinsView blockCoins;
+    CCoinsViewCache view(&blockCoins);
+    view.SetBackend(tip_view);
+
+    // Set fJustCheck to true in order to update, and not clear, validation caches.
+    if(!chainstate.ConnectBlock(block, state, &index_dummy, view, /*fJustCheck=*/true)) {
+        reason = state.GetRejectReason();
+        debug = state.GetDebugMessage();
+        return false;
+    }
+    if (!Assume(state.IsValid())) {
+        LogError("Unexpected invalid validation state");
         return false;
     }
-    assert(state.IsValid());
 
     return true;
 }

src/validation.h

@@ -383,14 +383,26 @@ public:
 /** Context-independent validity checks */
 bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW = true, bool fCheckMerkleRoot = true);
 
-/** Check a block is completely valid from start to finish (only works on top of our current best block) */
-bool TestBlockValidity(BlockValidationState& state,
-                       const CChainParams& chainparams,
-                       Chainstate& chainstate,
+/**
+ * Verify a block, including transactions.
+ *
+ * @param[in]   block       The block we want to process. Must connect to the
+ *                          current tip.
+ * @param[in]   chainstate    The chainstate to connect to.
+ * @param[out]  reason      rejection reason (BIP22)
+ * @param[out]  debug       more detailed rejection reason
+ * @param[in]   check_pow   perform proof-of-work check, nBits in the header
+ *                          is always checked
+ * @param[in]   check_merkle_root check the merkle root
+ *
+ * For signets the challenge verification is skipped when check_pow is false.
+ */
+bool TestBlockValidity(Chainstate& chainstate,
                        const CBlock& block,
-                       CBlockIndex* pindexPrev,
-                       bool fCheckPOW = true,
-                       bool fCheckMerkleRoot = true) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+                       const bool check_pow,
+                       const bool check_merkle_root,
+                       std::string& reason,
+                       std::string& debug);
 
 /** Check with the proof of work on each blockheader matches the value in nBits */
 bool HasValidProofOfWork(const std::vector<CBlockHeader>& headers, const Consensus::Params& consensusParams);