kernel: De-globalize signature cache

sedited · sedited · commit 5b1576bbc664 · 2024-05-21T23:18:42.000+02:00
Move its ownership to the ChainstateManager class.

Next to simplifying usage of the kernel library by no longer requiring
manual setup of the cache prior to using validation code, it also slims
down the amount of memory allocated by BasicTestingSetup.

Use this opportunity to make CSignatureCache RAII styled
diff --git a/src/Makefile.am b/src/Makefile.am
@@ -192,7 +192,6 @@ BITCOIN_CORE_H = \
   kernel/mempool_removal_reason.h \
   kernel/messagestartchars.h \
   kernel/notifications_interface.h \
-  kernel/validation_cache_sizes.h \
   key.h \
   key_io.h \
   logging.h \
@@ -234,7 +233,6 @@ BITCOIN_CORE_H = \
   node/transaction.h \
   node/txreconciliation.h \
   node/utxo_snapshot.h \
-  node/validation_cache_args.h \
   noui.h \
   outputtype.h \
   policy/v3_policy.h \
@@ -438,7 +436,6 @@ libbitcoin_node_a_SOURCES = \
   node/transaction.cpp \
   node/txreconciliation.cpp \
   node/utxo_snapshot.cpp \
-  node/validation_cache_args.cpp \
   noui.cpp \
   policy/v3_policy.cpp \
   policy/fees.cpp \
diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp
@@ -15,7 +15,6 @@
 #include <kernel/chainstatemanager_opts.h>
 #include <kernel/checks.h>
 #include <kernel/context.h>
-#include <kernel/validation_cache_sizes.h>
 
 #include <consensus/validation.h>
 #include <core_io.h>
@@ -61,12 +60,6 @@ int main(int argc, char* argv[])
     // properly
     assert(kernel::SanityChecks(kernel_context));
 
-    // Necessary for CheckInputScripts (eventually called by ProcessNewBlock),
-    // which will try the script cache first and fall back to actually
-    // performing the check with the signature cache.
-    kernel::ValidationCacheSizes validation_cache_sizes{};
-    Assert(InitSignatureCache(validation_cache_sizes.signature_cache_bytes));
-
     ValidationSignals validation_signals{std::make_unique<util::ImmediateTaskRunner>()};
 
     class KernelNotifications : public kernel::Notifications
diff --git a/src/init.cpp b/src/init.cpp
@@ -9,7 +9,6 @@
 
 #include <kernel/checks.h>
 #include <kernel/mempool_persist.h>
-#include <kernel/validation_cache_sizes.h>
 
 #include <addrman.h>
 #include <banman.h>
@@ -53,7 +52,6 @@
 #include <node/mempool_persist_args.h>
 #include <node/miner.h>
 #include <node/peerman_args.h>
-#include <node/validation_cache_args.h>
 #include <policy/feerate.h>
 #include <policy/fees.h>
 #include <policy/fees_args.h>
@@ -117,7 +115,6 @@
 
 using kernel::DumpMempool;
 using kernel::LoadMempool;
-using kernel::ValidationCacheSizes;
 
 using node::ApplyArgsManOptions;
 using node::BlockManager;
@@ -1145,10 +1142,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
                   args.GetArg("-datadir", ""), fs::PathToString(fs::current_path()));
     }
 
-    ValidationCacheSizes validation_cache_sizes{};
-    ApplyArgsManOptions(args, validation_cache_sizes);
-    (void)InitSignatureCache(validation_cache_sizes.signature_cache_bytes);
-
     assert(!node.scheduler);
     node.scheduler = std::make_unique<CScheduler>();
     auto& scheduler = *node.scheduler;
diff --git a/src/kernel/chainstatemanager_opts.h b/src/kernel/chainstatemanager_opts.h
@@ -50,6 +50,7 @@ struct ChainstateManagerOpts {
     //! Number of script check worker threads. Zero means no parallel verification.
     int worker_threads_num{0};
     size_t script_execution_cache_bytes{DEFAULT_MAX_SIG_CACHE_BYTES / 2};
+    size_t signature_cache_bytes{DEFAULT_MAX_SIG_CACHE_BYTES / 2};
 };
 
 } // namespace kernel
diff --git a/src/kernel/validation_cache_sizes.h b/src/kernel/validation_cache_sizes.h
diff --git a/src/node/chainstatemanager_args.cpp b/src/node/chainstatemanager_args.cpp
@@ -60,6 +60,7 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& args, ChainstateManage
         // 2. Multiply first, divide after to avoid integer truncation.
         size_t clamped_size_each = std::max<int64_t>(*max_size, 0) * (1 << 20) / 2;
         opts.script_execution_cache_bytes = clamped_size_each;
+        opts.signature_cache_bytes = clamped_size_each;
     }
 
     return {};
diff --git a/src/node/validation_cache_args.cpp b/src/node/validation_cache_args.cpp
diff --git a/src/node/validation_cache_args.h b/src/node/validation_cache_args.h
diff --git a/src/script/sigcache.cpp b/src/script/sigcache.cpp
@@ -17,7 +17,7 @@
 #include <shared_mutex>
 #include <vector>
 
-CSignatureCache::CSignatureCache()
+CSignatureCache::CSignatureCache(size_t max_size_bytes)
 {
     uint256 nonce = GetRandHash();
     // We want the nonce to be 64 bytes long to force the hasher to process
@@ -30,6 +30,10 @@ CSignatureCache::CSignatureCache()
     m_salted_hasher_ecdsa.Write(PADDING_ECDSA, 32);
     m_salted_hasher_schnorr.Write(nonce.begin(), 32);
     m_salted_hasher_schnorr.Write(PADDING_SCHNORR, 32);
+
+    const auto [num_elems, approx_size_bytes] = setValid.setup_bytes(max_size_bytes);
+    LogPrintf("Using %zu MiB out of %zu MiB requested for signature cache, able to store %zu elements\n",
+              approx_size_bytes >> 20, max_size_bytes >> 20, num_elems);
 }
 
 void CSignatureCache::ComputeEntryECDSA(uint256& entry, const uint256 &hash, const std::vector<unsigned char>& vchSig, const CPubKey& pubkey) const
@@ -56,48 +60,25 @@ void CSignatureCache::Set(const uint256& entry)
     setValid.insert(entry);
 }
 
-std::pair<uint32_t, size_t> CSignatureCache::setup_bytes(size_t n)
-{
-    return setValid.setup_bytes(n);
-}
-
-/* In previous versions of this code, signatureCache was a local static variable
- * in CachingTransactionSignatureChecker::VerifySignature.  We initialize
- * signatureCache outside of VerifySignature to avoid the atomic operation per
- * call overhead associated with local static variables even though
- * signatureCache could be made local to VerifySignature.
-*/
-static CSignatureCache signatureCache;
-
-// To be called once in AppInitMain/BasicTestingSetup to initialize the
-// signatureCache.
-bool InitSignatureCache(size_t max_size_bytes)
-{
-    const auto [num_elems, approx_size_bytes] = signatureCache.setup_bytes(max_size_bytes);
-    LogPrintf("Using %zu MiB out of %zu MiB requested for signature cache, able to store %zu elements\n",
-              approx_size_bytes >> 20, max_size_bytes >> 20, num_elems);
-    return true;
-}
-
 bool CachingTransactionSignatureChecker::VerifyECDSASignature(const std::vector<unsigned char>& vchSig, const CPubKey& pubkey, const uint256& sighash) const
 {
     uint256 entry;
-    signatureCache.ComputeEntryECDSA(entry, sighash, vchSig, pubkey);
-    if (signatureCache.Get(entry, !store))
+    m_signature_cache.ComputeEntryECDSA(entry, sighash, vchSig, pubkey);
+    if (m_signature_cache.Get(entry, !store))
         return true;
     if (!TransactionSignatureChecker::VerifyECDSASignature(vchSig, pubkey, sighash))
         return false;
     if (store)
-        signatureCache.Set(entry);
+        m_signature_cache.Set(entry);
     return true;
 }
 
 bool CachingTransactionSignatureChecker::VerifySchnorrSignature(Span<const unsigned char> sig, const XOnlyPubKey& pubkey, const uint256& sighash) const
 {
     uint256 entry;
-    signatureCache.ComputeEntrySchnorr(entry, sighash, sig, pubkey);
-    if (signatureCache.Get(entry, !store)) return true;
+    m_signature_cache.ComputeEntrySchnorr(entry, sighash, sig, pubkey);
+    if (m_signature_cache.Get(entry, !store)) return true;
     if (!TransactionSignatureChecker::VerifySchnorrSignature(sig, pubkey, sighash)) return false;
-    if (store) signatureCache.Set(entry);
+    if (store) m_signature_cache.Set(entry);
     return true;
 }
diff --git a/src/script/sigcache.h b/src/script/sigcache.h
@@ -46,7 +46,7 @@ class CSignatureCache
     std::shared_mutex cs_sigcache;
 
 public:
-    CSignatureCache();
+    CSignatureCache(size_t max_size_bytes);
 
     void ComputeEntryECDSA(uint256& entry, const uint256 &hash, const std::vector<unsigned char>& vchSig, const CPubKey& pubkey) const;
 
@@ -55,22 +55,19 @@ class CSignatureCache
     bool Get(const uint256& entry, const bool erase);
 
     void Set(const uint256& entry);
-
-    std::pair<uint32_t, size_t> setup_bytes(size_t n);
 };
 
 class CachingTransactionSignatureChecker : public TransactionSignatureChecker
 {
 private:
     bool store;
+    CSignatureCache& m_signature_cache;
 
 public:
-    CachingTransactionSignatureChecker(const CTransaction* txToIn, unsigned int nInIn, const CAmount& amountIn, bool storeIn, PrecomputedTransactionData& txdataIn) : TransactionSignatureChecker(txToIn, nInIn, amountIn, txdataIn, MissingDataBehavior::ASSERT_FAIL), store(storeIn) {}
+    CachingTransactionSignatureChecker(const CTransaction* txToIn, unsigned int nInIn, const CAmount& amountIn, bool storeIn, CSignatureCache& signature_cache, PrecomputedTransactionData& txdataIn) : TransactionSignatureChecker(txToIn, nInIn, amountIn, txdataIn, MissingDataBehavior::ASSERT_FAIL), store(storeIn), m_signature_cache(signature_cache)  {}
 
     bool VerifyECDSASignature(const std::vector<unsigned char>& vchSig, const CPubKey& vchPubKey, const uint256& sighash) const override;
     bool VerifySchnorrSignature(Span<const unsigned char> sig, const XOnlyPubKey& pubkey, const uint256& sighash) const override;
 };
 
-[[nodiscard]] bool InitSignatureCache(size_t max_size_bytes);
-
 #endif // BITCOIN_SCRIPT_SIGCACHE_H
diff --git a/src/test/fuzz/script_sigcache.cpp b/src/test/fuzz/script_sigcache.cpp
@@ -29,14 +29,15 @@ void initialize_script_sigcache()
 FUZZ_TARGET(script_sigcache, .init = initialize_script_sigcache)
 {
     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
+    CSignatureCache signature_cache{DEFAULT_MAX_SIG_CACHE_BYTES / 2};
 
     const std::optional<CMutableTransaction> mutable_transaction = ConsumeDeserializable<CMutableTransaction>(fuzzed_data_provider, TX_WITH_WITNESS);
     const CTransaction tx{mutable_transaction ? *mutable_transaction : CMutableTransaction{}};
     const unsigned int n_in = fuzzed_data_provider.ConsumeIntegral<unsigned int>();
     const CAmount amount = ConsumeMoney(fuzzed_data_provider);
     const bool store = fuzzed_data_provider.ConsumeBool();
     PrecomputedTransactionData tx_data;
-    CachingTransactionSignatureChecker caching_transaction_signature_checker{mutable_transaction ? &tx : nullptr, n_in, amount, store, tx_data};
+    CachingTransactionSignatureChecker caching_transaction_signature_checker{mutable_transaction ? &tx : nullptr, n_in, amount, store, signature_cache, tx_data};
     if (fuzzed_data_provider.ConsumeBool()) {
         const auto random_bytes = fuzzed_data_provider.ConsumeBytes<unsigned char>(64);
         const XOnlyPubKey pub_key(ConsumeUInt256(fuzzed_data_provider));
diff --git a/src/test/script_p2sh_tests.cpp b/src/test/script_p2sh_tests.cpp
@@ -58,6 +58,7 @@ BOOST_FIXTURE_TEST_SUITE(script_p2sh_tests, BasicTestingSetup)
 
 BOOST_AUTO_TEST_CASE(sign)
 {
+    CSignatureCache signature_cache{DEFAULT_MAX_SIG_CACHE_BYTES / 2};
     // Pay-to-script-hash looks like this:
     // scriptSig:    <sig> <sig...> <serialized_script>
     // scriptPubKey: HASH160 <hash> EQUAL
@@ -119,7 +120,7 @@ BOOST_AUTO_TEST_CASE(sign)
         {
             CScript sigSave = txTo[i].vin[0].scriptSig;
             txTo[i].vin[0].scriptSig = txTo[j].vin[0].scriptSig;
-            bool sigOK = CScriptCheck(txFrom.vout[txTo[i].vin[0].prevout.n], CTransaction(txTo[i]), 0, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_STRICTENC, false, &txdata)();
+            bool sigOK = CScriptCheck(txFrom.vout[txTo[i].vin[0].prevout.n], CTransaction(txTo[i]), signature_cache, 0, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_STRICTENC, false, &txdata)();
             if (i == j)
                 BOOST_CHECK_MESSAGE(sigOK, strprintf("VerifySignature %d %d", i, j));
             else
diff --git a/src/test/script_tests.cpp b/src/test/script_tests.cpp
@@ -1526,7 +1526,7 @@ static std::vector<unsigned int> AllConsensusFlags()
 /** Precomputed list of all valid combinations of consensus-relevant script validation flags. */
 static const std::vector<unsigned int> ALL_CONSENSUS_FLAGS = AllConsensusFlags();
 
-static void AssetTest(const UniValue& test)
+static void AssetTest(const UniValue& test, CSignatureCache& signature_cache)
 {
     BOOST_CHECK(test.isObject());
 
@@ -1543,7 +1543,7 @@ static void AssetTest(const UniValue& test)
         CTransaction tx(mtx);
         PrecomputedTransactionData txdata;
         txdata.Init(tx, std::vector<CTxOut>(prevouts));
-        CachingTransactionSignatureChecker txcheck(&tx, idx, prevouts[idx].nValue, true, txdata);
+        CachingTransactionSignatureChecker txcheck(&tx, idx, prevouts[idx].nValue, true, signature_cache, txdata);
 
         for (const auto flags : ALL_CONSENSUS_FLAGS) {
             // "final": true tests are valid for all flags. Others are only valid with flags that are
@@ -1561,7 +1561,7 @@ static void AssetTest(const UniValue& test)
         CTransaction tx(mtx);
         PrecomputedTransactionData txdata;
         txdata.Init(tx, std::vector<CTxOut>(prevouts));
-        CachingTransactionSignatureChecker txcheck(&tx, idx, prevouts[idx].nValue, true, txdata);
+        CachingTransactionSignatureChecker txcheck(&tx, idx, prevouts[idx].nValue, true, signature_cache, txdata);
 
         for (const auto flags : ALL_CONSENSUS_FLAGS) {
             // If a test is supposed to fail with test_flags, it should also fail with any superset thereof.
@@ -1577,6 +1577,7 @@ BOOST_AUTO_TEST_CASE(script_assets_test)
 {
     // See src/test/fuzz/script_assets_test_minimizer.cpp for information on how to generate
     // the script_assets_test.json file used by this test.
+    CSignatureCache signature_cache{DEFAULT_MAX_SIG_CACHE_BYTES / 2};
 
     const char* dir = std::getenv("DIR_UNIT_TEST_DATA");
     BOOST_WARN_MESSAGE(dir != nullptr, "Variable DIR_UNIT_TEST_DATA unset, skipping script_assets_test");
@@ -1597,7 +1598,7 @@ BOOST_AUTO_TEST_CASE(script_assets_test)
     BOOST_CHECK(tests.size() > 0);
 
     for (size_t i = 0; i < tests.size(); i++) {
-        AssetTest(tests[i]);
+        AssetTest(tests[i], signature_cache);
     }
     file.close();
 }
diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp
@@ -17,6 +17,7 @@
 #include <policy/settings.h>
 #include <script/script.h>
 #include <script/script_error.h>
+#include <script/sigcache.h>
 #include <script/sign.h>
 #include <script/signingprovider.h>
 #include <script/solver.h>
@@ -541,9 +542,11 @@ BOOST_AUTO_TEST_CASE(test_big_witness_transaction)
         coins.emplace_back(std::move(coin));
     }
 
+    CSignatureCache signature_cache{DEFAULT_MAX_SIG_CACHE_BYTES / 2};
+
     for(uint32_t i = 0; i < mtx.vin.size(); i++) {
         std::vector<CScriptCheck> vChecks;
-        vChecks.emplace_back(coins[tx.vin[i].prevout.n].out, tx, i, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, false, &txdata);
+        vChecks.emplace_back(coins[tx.vin[i].prevout.n].out, tx, signature_cache, i, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, false, &txdata);
         control.Add(std::move(vChecks));
     }
 
diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp
@@ -21,7 +21,7 @@ struct Dersig100Setup : public TestChain100Setup {
 
     Dersig100Setup()
         : TestChain100Setup{ChainType::REGTEST, {"-testactivationheight=dersig@102"}},
-          m_validation_cache{DEFAULT_MAX_SIG_CACHE_BYTES / 2}
+          m_validation_cache{DEFAULT_MAX_SIG_CACHE_BYTES / 2, DEFAULT_MAX_SIG_CACHE_BYTES / 2}
     {
     }
 };
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
diff --git a/src/validation.cpp b/src/validation.cpp
diff --git a/src/validation.h b/src/validation.h

Original file line number	Diff line number	Diff line change
`@@ -60,6 +60,7 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& args, ChainstateManage`
`60`	`60`	`// 2. Multiply first, divide after to avoid integer truncation.`
`61`	`61`	`size_t clamped_size_each = std::max<int64_t>(max_size, 0) (1 << 20) / 2;`
`62`	`62`	`opts.script_execution_cache_bytes = clamped_size_each;`
	`63`	`+ opts.signature_cache_bytes = clamped_size_each;`
`63`	`64`	`}`
`64`	`65`
`65`	`66`	`return {};`