miner: Disable proof and signature checks in CreateNewBlock

The only source of transactions for `CreateNewBlock` is the mempool, and
every transaction added to the mempool goes through `AcceptToMemoryPool`
which checks proofs and signatures.

We maintain the ability to enable these checks in `TestBlockValidity`
because it is also used in an (undocumented) `getblocktemplate` mode to
check a proposed block (minus PoW), where we cannot assume the
transactions are valid.

Co-authored-by: Kris Nuttycombe <kris@nutty.land>

doc/release-notes.md

@@ -45,6 +45,10 @@ Option handling
 RPC interface
 -------------
 
+- The `getblocktemplate` RPC method now skips proof and signature checks on
+  templates it creates, as these templates only include transactions that have
+  previously been checked when being added to the mempool.
+
 - The `getrawtransaction` RPC method now includes details about Orchard actions
   within transactions.
 

src/main.cpp

@@ -3065,12 +3065,30 @@ static bool ShouldCheckTransactions(const CChainParams& chainparams, const CBloc
 
 bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pindex,
                   CCoinsViewCache& view, const CChainParams& chainparams,
-                  bool fJustCheck, bool fCheckAuthDataRoot)
+                  bool fJustCheck, CheckAs blockChecks)
 {
     AssertLockHeld(cs_main);
 
+    bool fCheckAuthDataRoot = true;
     bool fExpensiveChecks = true;
 
+    switch (blockChecks) {
+    case CheckAs::Block:
+        break;
+    case CheckAs::BlockTemplate:
+        // Disable checking proofs and signatures for block templates, to avoid
+        // checking them twice for transactions that were already checked when
+        // added to the mempool.
+        fExpensiveChecks = false;
+    case CheckAs::SlowBenchmark:
+        // Disable checking the authDataRoot for block templates and slow block
+        // benchmarks.
+        fCheckAuthDataRoot = false;
+        break;
+    default:
+        assert(false);
+    }
+
     // If this block is an ancestor of a checkpoint, disable expensive checks
     if (fCheckpointsEnabled && Checkpoints::IsAncestorOfLastCheckpoint(chainparams.Checkpoints(), pindex)) {
         fExpensiveChecks = false;
@@ -5087,7 +5105,10 @@ bool ProcessNewBlock(CValidationState& state, const CChainParams& chainparams, c
  * This is only invoked by the miner.
  * The block's proof-of-work is assumed invalid and not checked.
  */
-bool TestBlockValidity(CValidationState& state, const CChainParams& chainparams, const CBlock& block, CBlockIndex* pindexPrev, bool fCheckMerkleRoot)
+bool TestBlockValidity(
+    CValidationState& state, const CChainParams& chainparams,
+    const CBlock& block, CBlockIndex* pindexPrev,
+    bool fIsBlockTemplate)
 {
     AssertLockHeld(cs_main);
     assert(pindexPrev == chainActive.Tip());
@@ -5100,6 +5121,9 @@ bool TestBlockValidity(CValidationState& state, const CChainParams& chainparams,
     // JoinSplit proofs are verified in ConnectBlock
     auto verifier = ProofVerifier::Disabled();
 
+    bool fCheckMerkleRoot = !fIsBlockTemplate;
+    auto blockChecks = fIsBlockTemplate ? CheckAs::BlockTemplate : CheckAs::Block;
+
     // NOTE: CheckBlockHeader is called by CheckBlock
     if (!ContextualCheckBlockHeader(block, state, chainparams, pindexPrev))
         return false;
@@ -5108,7 +5132,7 @@ bool TestBlockValidity(CValidationState& state, const CChainParams& chainparams,
         return false;
     if (!ContextualCheckBlock(block, state, chainparams, pindexPrev, true))
         return false;
-    if (!ConnectBlock(block, state, &indexDummy, viewNew, chainparams, true, fCheckMerkleRoot))
+    if (!ConnectBlock(block, state, &indexDummy, viewNew, chainparams, true, blockChecks))
         return false;
     assert(state.IsValid());
 

src/main.h

@@ -561,18 +561,35 @@ bool ContextualCheckBlock(const CBlock& block, CValidationState& state,
                           CBlockIndex *pindexPrev,
                           bool fCheckTransactions);
 
+/**
+ * How a given block should be checked.
+ *
+ * - `CheckAs::Block` applies all relevant block checks.
+ * - `CheckAs::BlockTemplate` is the same as `CheckAs::Block` except that proofs
+ *   and signatures are not validated, and the authDataRoot is not checked (as
+ *   the coinbase transaction is not fully complete).
+ * - `CheckAs::SlowBenchmark` is the same as `CheckAs::Block` except that the
+ *   authDataRoot is not checked (as the required history tree state is not
+ *   currently faked).
+ */
+enum class CheckAs {
+    Block,
+    BlockTemplate,
+    SlowBenchmark,
+};
+
 /** Apply the effects of this block (with given index) on the UTXO set represented by coins.
  *  Validity checks that depend on the UTXO set are also done; ConnectBlock()
  *  can fail if those validity checks fail (among other reasons). */
 bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pindex, CCoinsViewCache& coins,
                   const CChainParams& chainparams,
-                  bool fJustCheck = false, bool fCheckAuthDataRoot = true);
+                  bool fJustCheck = false, CheckAs blockChecks = CheckAs::Block);
 
 /**
  * Check a block is completely valid from start to finish (only works on top
  * of our current best block, with cs_main held)
  */
-bool TestBlockValidity(CValidationState& state, const CChainParams& chainparams, const CBlock& block, CBlockIndex* pindexPrev, bool fCheckMerkleRoot);
+bool TestBlockValidity(CValidationState& state, const CChainParams& chainparams, const CBlock& block, CBlockIndex* pindexPrev, bool fIsBlockTemplate);
 
 
 /**

src/miner.cpp

@@ -769,7 +769,7 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const MinerAddre
         pblocktemplate->vTxSigOps[0] = GetLegacySigOpCount(pblock->vtx[0]);
 
         CValidationState state;
-        if (!TestBlockValidity(state, chainparams, *pblock, pindexPrev, false))
+        if (!TestBlockValidity(state, chainparams, *pblock, pindexPrev, true))
             throw std::runtime_error(std::string("CreateNewBlock(): TestBlockValidity failed: ") + state.GetRejectReason());
     }
 

src/rpc/mining.cpp

@@ -561,7 +561,7 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp)
                 return "inconclusive-not-best-prevblk";
 
             CValidationState state;
-            TestBlockValidity(state, Params(), block, pindexPrev, true);
+            TestBlockValidity(state, Params(), block, pindexPrev, false);
             return BIP22ValidationResult(state);
         }
     }

src/zcbenchmarks.cpp

@@ -645,7 +645,7 @@ double benchmark_connectblock_sapling()
     CValidationState state;
     struct timeval tv_start;
     timer_start(tv_start);
-    assert(ConnectBlock(block, state, &index, view, Params(), true, false));
+    assert(ConnectBlock(block, state, &index, view, Params(), true, CheckAs::SlowBenchmark));
     auto duration = timer_stop(tv_start);
 
     // Undo alterations to global state
@@ -691,7 +691,7 @@ double benchmark_connectblock_orchard()
     CValidationState state;
     struct timeval tv_start;
     timer_start(tv_start);
-    assert(ConnectBlock(block, state, &index, view, Params(), true, false));
+    assert(ConnectBlock(block, state, &index, view, Params(), true, CheckAs::SlowBenchmark));
     auto duration = timer_stop(tv_start);
 
     // Undo alterations to global state