Benchmark batch verification performance impact on Chainstate::Connec…

…tBlock

- Add ConnectBlock with and without batch verification benchmarks
- Disable caching in CachingTransactionSignatureChecker for a fair assement

src/Makefile.bench.include

@@ -22,6 +22,7 @@ bench_bench_bitcoin_SOURCES = \
   bench/block_assemble.cpp \
   bench/ccoins_caching.cpp \
   bench/chacha20.cpp \
+  bench/check_connectblock.cpp \
   bench/checkblock.cpp \
   bench/checkqueue.cpp \
   bench/crypto_hash.cpp \

src/batchverify.cpp

@@ -35,8 +35,7 @@ bool BatchSchnorrVerifier::Add(const Span<const unsigned char> sig, const XOnlyP
     }
 
     secp256k1_xonly_pubkey pubkey_parsed;
-    (void)secp256k1_xonly_pubkey_parse(secp256k1_context_static, &pubkey_parsed, pubkey.data());
-
+    if (!secp256k1_xonly_pubkey_parse(secp256k1_context_static, &pubkey_parsed, pubkey.data())) return false;
     return secp256k1_batch_add_schnorrsig(secp256k1_context_static, m_batch, sig.data(), sighash.begin(), 32, &pubkey_parsed);
 }
 

src/bench/check_connectblock.cpp

@@ -0,0 +1,97 @@
+// Copyright (c) 2016-2022 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <bench/bench.h>
+#include <bench/data.h>
+
+#include <addresstype.h>
+#include <batchverify.h>
+#include <interfaces/chain.h>
+#include <script/interpreter.h>
+#include <test/util/setup_common.h>
+#include <validation.h>
+
+#include <cassert>
+#include <utility>
+#include <vector>
+
+std::pair<CBlock, std::unique_ptr<CBlockIndex>> CreateTestBlock(TestChain100Setup* test_setup)
+{
+  Chainstate& chainstate = test_setup->m_node.chainman->ActiveChainstate();
+
+  std::vector<CKey> keys{test_setup->coinbaseKey};
+  std::vector<CMutableTransaction> txs;
+  auto input_tx = test_setup->m_coinbase_txns[0];
+  const int num_txs = 170;
+  const int num_outputs = 49;
+  for (int i = 0; i < num_txs; i++)
+  {
+    Txid txid = input_tx->GetHash();
+    std::vector<COutPoint> inputs;
+    if (input_tx->IsCoinBase())
+    {
+      inputs.emplace_back(txid, 0);
+    } else
+    {
+      for (int i = 0; i < num_outputs; i++)
+      {
+        inputs.emplace_back(txid, i);
+      }
+    }
+
+    std::vector<CTxOut> outputs;
+    for (int i = 0; i < num_outputs; i++)
+    {
+      const CKey key = GenerateRandomKey();
+      const WitnessV1Taproot taproot(XOnlyPubKey(key.GetPubKey()));
+      const CScript scriptpubkey = GetScriptForDestination(taproot);
+      outputs.emplace_back(COIN, scriptpubkey);
+      keys.push_back(key);
+    }
+    const auto taproot_tx = test_setup->CreateValidTransaction(std::vector{input_tx}, inputs, chainstate.m_chain.Height() + 1, keys, outputs, std::nullopt, std::nullopt);
+    txs.push_back(taproot_tx.first);
+    input_tx = MakeTransactionRef(taproot_tx.first);
+  }
+
+  const WitnessV1Taproot taproot(XOnlyPubKey(test_setup->coinbaseKey.GetPubKey()));
+  const CScript coinbase_spk = GetScriptForDestination(taproot);
+  const auto test_block = test_setup->CreateBlock(txs, coinbase_spk, chainstate);
+
+  auto pindex = std::make_unique<CBlockIndex>(test_block);
+  auto test_blockhash = std::make_unique<uint256>(test_block.GetHash());
+  pindex->nHeight = chainstate.m_chain.Height() + 1;
+  pindex->phashBlock = test_blockhash.get();
+  pindex->pprev = chainstate.m_chain.Tip();
+  test_blockhash.release();
+
+  return std::make_pair(test_block, std::move(pindex));
+}
+
+static void ConnectBlockWithBatchVerify(benchmark::Bench& bench)
+{
+  const auto test_setup = MakeNoLogFileContext<TestChain100Setup>();
+  Chainstate& chainstate = test_setup->m_node.chainman->ActiveChainstate();
+  const auto result = CreateTestBlock(test_setup.get());
+  BlockValidationState test_block_state;
+  bench.unit("block").run([&] {
+    CCoinsViewCache viewNew(&chainstate.CoinsTip());
+    BatchSchnorrVerifier batch{};
+    assert(chainstate.ConnectBlock(result.first, test_block_state, result.second.get(), viewNew, true, &batch));
+  });
+}
+
+static void ConnectBlockWithoutBatchVerify(benchmark::Bench& bench)
+{
+  const auto test_setup = MakeNoLogFileContext<TestChain100Setup>();
+  Chainstate& chainstate = test_setup->m_node.chainman->ActiveChainstate();
+  const auto result = CreateTestBlock(test_setup.get());
+  BlockValidationState test_block_state;
+  bench.unit("block").run([&] {
+    CCoinsViewCache viewNew(&chainstate.CoinsTip());
+    assert(chainstate.ConnectBlock(result.first, test_block_state, result.second.get(), viewNew, true));
+  });
+}
+
+BENCHMARK(ConnectBlockWithBatchVerify, benchmark::PriorityLevel::HIGH);
+BENCHMARK(ConnectBlockWithoutBatchVerify, benchmark::PriorityLevel::HIGH);

src/script/sigcache.cpp

@@ -125,7 +125,7 @@ bool CachingTransactionSignatureChecker::VerifySchnorrSignature(Span<const unsig
     signatureCache.ComputeEntrySchnorr(entry, sighash, sig, pubkey);
     if (signatureCache.Get(entry, !store)) return true;
     if (!TransactionSignatureChecker::VerifySchnorrSignature(sig, pubkey, sighash)) return false;
-    if (store) signatureCache.Set(entry);
+    // if (store) signatureCache.Set(entry);
     return true;
 }
 

src/validation.cpp

@@ -2193,7 +2193,7 @@ static int64_t num_blocks_total = 0;
  *  Validity checks that depend on the UTXO set are also done; ConnectBlock()
  *  can fail if those validity checks fail (among other reasons). */
 bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, CBlockIndex* pindex,
-                               CCoinsViewCache& view, bool fJustCheck)
+                               CCoinsViewCache& view, bool fJustCheck, BatchSchnorrVerifier* batch)
 {
     AssertLockHeld(cs_main);
     assert(pindex);
@@ -2398,7 +2398,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
     int64_t nSigOpsCost = 0;
     blockundo.vtxundo.reserve(block.vtx.size() - 1);
 
-    BatchSchnorrVerifier batch{};
+    // BatchSchnorrVerifier batch{};
 
     for (unsigned int i = 0; i < block.vtx.size(); i++)
     {
@@ -2451,7 +2451,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
             std::vector<CScriptCheck> vChecks;
             bool fCacheResults = fJustCheck; /* Don't cache results if we're actually connecting blocks (still consult the cache, though) */
             TxValidationState tx_state;
-            if (fScriptChecks && !CheckInputScripts(tx, tx_state, view, flags, fCacheResults, fCacheResults, txsdata[i], parallel_script_checks ? &vChecks : nullptr, &batch)) {
+            if (fScriptChecks && !CheckInputScripts(tx, tx_state, view, flags, fCacheResults, fCacheResults, txsdata[i], parallel_script_checks ? &vChecks : nullptr, batch)) {
                 // Any transaction validation failure in ConnectBlock is a block consensus failure
                 state.Invalid(BlockValidationResult::BLOCK_CONSENSUS,
                               tx_state.GetRejectReason(), tx_state.GetDebugMessage());
@@ -2486,7 +2486,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
         return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "block-validation-failed");
     }
 
-    if (!batch.Verify()) {
+    if (batch != nullptr && !batch->Verify()) {
         LogPrintf("ERROR: %s: Batch verification failed\n", __func__);
         return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "block-batch-verify-failed");
     }

src/validation.h

@@ -689,7 +689,7 @@ class Chainstate
     DisconnectResult DisconnectBlock(const CBlock& block, const CBlockIndex* pindex, CCoinsViewCache& view)
         EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
     bool ConnectBlock(const CBlock& block, BlockValidationState& state, CBlockIndex* pindex,
-                      CCoinsViewCache& view, bool fJustCheck = false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+                      CCoinsViewCache& view, bool fJustCheck = false, BatchSchnorrVerifier* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
     // Apply the effects of a block disconnection on the UTXO set.
     bool DisconnectTip(BlockValidationState& state, DisconnectedBlockTransactions* disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs);