Add -gbtcheckvalidity arg to CLI;"checkvalidity" arg to GBT(L)

Summary
---

As discussed in Slack, perhaps it would be beneficial to optionally skip
the slow `TestBlockValidity` checks when generating a block template.
These checks dominate block template creation for large blocks, and
in some cases perhaps advanced users (miners, etc) may want to skip these
checks.  Note that it is a bug in this codebase if we *ever* generate
an invalid block template anyway, so the checks are largely redundant.

Additoinally, while working on this code, I noticed:

- RPC help for GBT(L) is incomplete. I added the missing `"longpollid"`
documentation.
- RPC help for GBT(L) had some redundant pieces. I put the duplicated
stuff into a common static function `getGBTArgs()`.  This makes it
easier to maintain the arg code for those two very strongly related functions.
- I noticed it's annoying when testing that block templates are cached. Perhaps we
want to skip caching, especially when testing.  I added the optional
RPC argument to the `template_request` called `"ignorecache"` which, if true,
always skips the cached block template and generates a new one.

Summary of Changes
---

- New arg: `-gbtcheckvalidity` to control the default setting for gbt(l).
  If false, we skip the validity checks when generating a block template
  (default: true)
- Added help doc for `"longpollid"` `template_request` argument in RPC for
  gbt(l)
- Added optional `"ignorecache"` `template_request` argument (default:
  false) to skip the cached block template unconditionally (useful for
  testing).
- Added optional `"checkvalidity"` `template_request` argument (default:
  whatever `-gbtcheckvalidity` was set to on CLI), which controls the
  validity checker on a per-template basis.
- Refactored the help for `getblocktemplate` and `getblocktemplatelight`
  into a common function.
- In `checkvalidity=false` mode, also skip the redundant and wasteful
  calculation of block size for the 6th time just to print it to debug log
  (and instead use the estimated worst-case size we already know).

Test Plan
---

1. Review
2. `ninja all check`
3. `test/functional/test_runner.py bchn-rpc-gbt-checkvalidity-ignorecache`

src/config.h

@@ -47,6 +47,9 @@ class Config : public boost::noncopyable {
 
     virtual void SetRejectSubVersions(const std::set<std::string> &reject) = 0;
     virtual const std::set<std::string> & GetRejectSubVersions() const = 0;
+
+    virtual void SetGBTCheckValidity(bool) = 0;
+    virtual bool GetGBTCheckValidity() const = 0;
 };
 
 class GlobalConfig final : public Config {
@@ -72,8 +75,12 @@ class GlobalConfig final : public Config {
     void SetRejectSubVersions(const std::set<std::string> &reject) override { rejectSubVersions = reject; }
     const std::set<std::string> & GetRejectSubVersions() const override { return rejectSubVersions; }
 
+    void SetGBTCheckValidity(bool b) override { gbtCheckValidity = b; }
+    bool GetGBTCheckValidity() const override { return gbtCheckValidity; }
+
 private:
     bool useCashAddr;
+    bool gbtCheckValidity;
     Amount excessUTXOCharge;
     uint64_t nInvBroadcastRate;
     uint64_t nInvBroadcastInterval;
@@ -122,6 +129,9 @@ class DummyConfig : public Config {
         return dummy;
     }
 
+    void SetGBTCheckValidity(bool) override {}
+    bool GetGBTCheckValidity() const override { return false; }
+
 private:
     std::unique_ptr<CChainParams> chainParams;
 };

src/init.cpp

@@ -1051,6 +1051,14 @@ void SetupServerArgs() {
                            DEFAULT_MAX_INITIAL_GBT_TIME),
                  false, OptionsCategory::BLOCK_CREATION);
 
+    gArgs.AddArg("-gbtcheckvalidity",
+                 strprintf("Set whether to test generated block templates for validity in getblocktemplate and/or "
+                           "getblocktemplatelight. Mining nodes may wish to skip validity checks as a performance "
+                           "optimization, particularly when mining large blocks. Validity checking can also be set "
+                           "on individual gbt calls by specifying the \"checkvalidity\": boolean key in the "
+                           "template_request object given to gbt. (default: %d)", DEFAULT_GBT_CHECK_VALIDITY),
+                 false, OptionsCategory::BLOCK_CREATION);
+
     gArgs.AddArg("-blockmintxfee=<amt>",
                  strprintf("Set lowest fee rate (in %s/kB) for transactions to "
                            "be included in block creation. (default: %s)",
@@ -1910,6 +1918,9 @@ bool AppInitParameterInteraction(Config &config) {
     }
     config.SetInvBroadcastRate(nTxBroadcastRate);
 
+    // process and save -gbtcheckvalidity arg (if specified)
+    config.SetGBTCheckValidity(gArgs.GetBoolArg("-gbtcheckvalidity", DEFAULT_GBT_CHECK_VALIDITY));
+
     // Sanity check argument for min fee for including tx in block
     // TODO: Harmonize which arguments need sanity checking and where that
     // happens.

src/miner.cpp

@@ -120,7 +120,7 @@ void BlockAssembler::resetBlock() {
 }
 
 std::unique_ptr<CBlockTemplate>
-BlockAssembler::CreateNewBlock(const CScript &scriptPubKeyIn, double timeLimitSecs) {
+BlockAssembler::CreateNewBlock(const CScript &scriptPubKeyIn, double timeLimitSecs, bool checkValidity) {
     const int64_t nTimeStart = GetTimeMicros();
 
     resetBlock();
@@ -215,10 +215,12 @@ BlockAssembler::CreateNewBlock(const CScript &scriptPubKeyIn, double timeLimitSe
     pblocktemplate->entries[0].fees = -1 * nFees;
     pblock->vtx[0] = pblocktemplate->entries[0].tx;
 
-    uint64_t nSerializeSize = GetSerializeSize(*pblock, PROTOCOL_VERSION);
+    const uint64_t nByteSize =
+            checkValidity ? GetSerializeSize(*pblock, PROTOCOL_VERSION)
+                          : nBlockSize; // if not checking validity, skip re-serializing the block and estimate the size
 
-    LogPrintf("CreateNewBlock(): total size: %u txs: %u fees: %ld sigops %d\n",
-              nSerializeSize, nBlockTx, nFees, nBlockSigOps);
+    LogPrintf("CreateNewBlock(): %s: %u txs: %u fees: %ld sigops %d\n",
+              checkValidity ? "total size" : "estimated size", nByteSize, nBlockTx, nFees, nBlockSigOps);
 
     // Fill in header.
     pblock->hashPrevBlock = pindexPrev->GetBlockHash();
@@ -227,14 +229,16 @@ BlockAssembler::CreateNewBlock(const CScript &scriptPubKeyIn, double timeLimitSe
     pblock->nNonce = 0;
     pblocktemplate->entries[0].sigOpCount = 0;
 
-    CValidationState state;
-    if (!TestBlockValidity(state, chainparams, *pblock, pindexPrev,
-                           BlockValidationOptions(GetConfig())
-                               .withCheckPoW(false)
-                               .withCheckMerkleRoot(false))) {
-        throw std::runtime_error(strprintf("%s: TestBlockValidity failed: %s",
-                                           __func__,
-                                           FormatStateMessage(state)));
+    if (checkValidity) {
+        CValidationState state;
+        if (!TestBlockValidity(state, chainparams, *pblock, pindexPrev,
+                               BlockValidationOptions(GetConfig())
+                                   .withCheckPoW(false)
+                                   .withCheckMerkleRoot(false))) {
+            throw std::runtime_error(strprintf("%s: TestBlockValidity failed: %s",
+                                               __func__,
+                                               FormatStateMessage(state)));
+        }
     }
     const int64_t nTime2 = GetTimeMicros();
 

src/miner.h

@@ -90,9 +90,13 @@ class BlockAssembler {
      *  @param timeLimitSecs   If >0, limit the amount of time spent
      *                         assembling the block to this time limit,
      *                         in seconds. If <= 0, no time limit.
+     *  @param checkValidity   If false, we do not call TestBlockValidity
+     *                         and instead assume the block we create
+     *                         is valid. This option is offered for
+     *                         performance.
      */
     std::unique_ptr<CBlockTemplate>
-    CreateNewBlock(const CScript &scriptPubKeyIn, double timeLimitSecs = 0.);
+    CreateNewBlock(const CScript &scriptPubKeyIn, double timeLimitSecs = 0., bool checkValidity = true);
 
     uint64_t GetMaxGeneratedBlockSize() const { return nMaxGeneratedBlockSize; }
 

src/policy/policy.h

@@ -32,6 +32,11 @@ static constexpr int64_t DEFAULT_MAX_INITIAL_GBT_TIME = 0;
  * in blocks created by mining code.
  */
 static constexpr Amount DEFAULT_BLOCK_MIN_TX_FEE_PER_KB(1000 * SATOSHI);
+/**
+ * Default for -gbtcheckvalidity, which determines whether we call
+ * TestBlockValidity() on the generated block template.
+ */
+static constexpr bool DEFAULT_GBT_CHECK_VALIDITY = true;
 /**
  * The maximum size for transactions we're willing to relay/mine.
  */

src/rpc/mining.cpp

@@ -371,6 +371,8 @@ static UniValue getblocktemplatecommon(bool fLight, const Config &config, const
 
     std::string strMode = "template";
     const UniValue *lpval = &NullUniValue;
+    bool checkValidity = config.GetGBTCheckValidity();
+    bool ignoreCacheOverride = false;
     std::set<std::string> setClientRules;
     if (!request.params[0].isNull()) {
         const UniValue::Object &oparam = request.params[0].get_obj();
@@ -382,7 +384,15 @@ static UniValue getblocktemplatecommon(bool fLight, const Config &config, const
         } else {
             throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid mode");
         }
+
         lpval = &oparam["longpollid"];
+        // Check the block template for validity? (affects strMode == "template" only)
+        if (const UniValue &tval = oparam["checkvalidity"]; !tval.isNull()) {
+            checkValidity = tval.get_bool();
+        }
+        if (const UniValue &tval = oparam["ignorecache"]; !tval.isNull()) {
+            ignoreCacheOverride = tval.get_bool();
+        }
 
         if (strMode == "proposal") {
             const UniValue &dataval = oparam["data"];
@@ -505,7 +515,7 @@ static UniValue getblocktemplatecommon(bool fLight, const Config &config, const
     static std::unique_ptr<LightResult> plightresult; // fLight mode only, cached result associated with pblocktemplate
     static bool fIgnoreCache = false;
     bool fNewTip = (pindexPrev && pindexPrev != ::ChainActive().Tip());
-    if (pindexPrev != ::ChainActive().Tip() || fIgnoreCache ||
+    if (pindexPrev != ::ChainActive().Tip() || fIgnoreCache || ignoreCacheOverride ||
         (g_mempool.GetTransactionsUpdated() != nTransactionsUpdatedLast &&
          GetTime() - nStart > 5)) {
         // Clear pindexPrev so future calls make a new block, despite any
@@ -535,7 +545,7 @@ static UniValue getblocktemplatecommon(bool fLight, const Config &config, const
         // Create new block
         CScript scriptDummy = CScript() << OP_TRUE;
         pblocktemplate =
-            BlockAssembler(config, g_mempool).CreateNewBlock(scriptDummy, timeLimitSecs);
+            BlockAssembler(config, g_mempool).CreateNewBlock(scriptDummy, timeLimitSecs, checkValidity);
         plightresult.reset();
         if (!pblocktemplate) {
             throw JSONRPCError(RPC_OUT_OF_MEMORY, "Out of memory");
@@ -725,6 +735,38 @@ static UniValue getblocktemplatecommon(bool fLight, const Config &config, const
     return result;
 }
 
+static std::vector<RPCArg> getGBTArgs(const Config &config, bool fLight) {
+    std::vector<RPCArg> ret{
+        {"template_request", RPCArg::Type::OBJ, /* opt */ true, /* default_val */ "",
+         "A json object in the following spec",
+         {
+             {"mode", RPCArg::Type::STR, /* opt */ true, /* default_val */ "",
+              "This must be set to \"template\", \"proposal\" (see BIP23), or omitted"},
+             {"capabilities", RPCArg::Type::ARR, /* opt */ true, /* default_val */ "",
+              "A list of strings",
+              {
+                  {"support", RPCArg::Type::STR, /* opt */ true, /* default_val */ "",
+                   "client side supported feature, "
+                   "'longpoll', 'coinbasetxn', 'coinbasevalue', 'proposal', 'serverlist', 'workid'"},
+                 },
+             },
+             {"longpollid", RPCArg::Type::STR, /* opt */ true, /* default_val */ "",
+              "Enables long-polling mode: specify the current best block hash (hex)"},
+             {"checkvalidity", RPCArg::Type::BOOL, /* opt */ true,
+              /* default_val*/ config.GetGBTCheckValidity() ? "true" : "false",
+              "Specify whether to test the generated block template for validity (\"template\" mode only)"},
+             {"ignorecache", RPCArg::Type::BOOL, /* opt */ true, /* default_val */ "false",
+              "Specify whether to unconditionally ignore the cached block template"},
+         }, "\"template_request\""},
+    };
+    if (fLight) {
+        ret.push_back({"additional_txs", RPCArg::Type::ARR, /* opt */ true, /* default_val */ "[]",
+                       "Hex encoded transactions to add to the block (each tx must be unique and valid)",
+                       {}, "\"additional_txs\""});
+    }
+    return ret;
+}
+
 static UniValue getblocktemplate(const Config &config, const JSONRPCRequest &request) {
     if (request.fHelp || request.params.size() > 1) {
         // If you change the help text of getblocktemplate below,
@@ -735,22 +777,7 @@ static UniValue getblocktemplate(const Config &config, const JSONRPCRequest &req
             "that is used to explicitly select between the default 'template' request or a 'proposal'.\n"
             "It returns data needed to construct a block to work on.\n"
             "For full specification, see BIP22/BIP23.\n",
-            {
-                {"template_request", RPCArg::Type::OBJ, /* opt */ true, /* default_val */ "",
-                 "A json object in the following spec",
-                 {
-                     {"mode", RPCArg::Type::STR, /* opt */ true, /* default_val */ "",
-                      "This must be set to \"template\", \"proposal\" (see BIP23), or omitted"},
-                     {"capabilities", RPCArg::Type::ARR, /* opt */ true, /* default_val */ "",
-                      "A list of strings",
-                      {
-                          {"support", RPCArg::Type::STR, /* opt */ true, /* default_val */ "",
-                           "client side supported feature, "
-                           "'longpoll', 'coinbasetxn', 'coinbasevalue', 'proposal', 'serverlist', 'workid'"},
-                         },
-                     },
-                 }, "\"template_request\""},
-            },
+            getGBTArgs(config, /* light */ false),
             RPCResult{
                 "{\n"
                 "  \"version\" : n,                    (numeric) The preferred block version\n"
@@ -820,26 +847,7 @@ static UniValue getblocktemplatelight(const Config &config, const JSONRPCRequest
             "that is used to explicitly select between the default 'template' request or a 'proposal'.\n"
             "It returns data needed to construct a block to work on.\n"
             "For full specification, see the getblocktemplatelight spec in the doc folder, and BIP22/BIP23.\n",
-            {
-                {"template_request", RPCArg::Type::OBJ, /* opt */ true, /* default_val */ "",
-                 "A json object in the following spec",
-                 {
-                     {"mode", RPCArg::Type::STR, /* opt */ true, /* default_val */ "",
-                      "This must be set to \"template\", \"proposal\" (see BIP23), or omitted"},
-                     {"capabilities", RPCArg::Type::ARR, /* opt */ true, /* default_val */ "",
-                      "A list of strings",
-                      {
-                          {"support", RPCArg::Type::STR, /* opt */ true, /* default_val */ "",
-                           "client side supported feature, "
-                           "'longpoll', 'coinbasetxn', 'coinbasevalue', 'proposal', 'serverlist', 'workid'"},
-                         },
-                     },
-                 }, "\"template_request\""},
-                 {"additional_txs", RPCArg::Type::ARR, /* opt */ true, /* default_val */ "[]",
-                 "Hex encoded transactions to add to the block (each tx must be unique and valid)",
-                 {
-                 }, "\"additional_txs\""},
-            },
+            getGBTArgs(config, true),
             RPCResult{
                 "{\n"
                 "  \"version\" : n,                    (numeric) The preferred block version\n"