[master] sigcache: harden range proof cache keys and add -norangeproofcache option

src/init.cpp

@@ -636,6 +636,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
     argsman.AddArg("-capturemessages", "Capture all P2P messages to disk", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-mocktime=<n>", "Replace actual time with " + UNIX_EPOCH_TIME + " (default: 0)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-maxsigcachesize=<n>", strprintf("Limit sum of signature cache and script execution cache sizes to <n> MiB (default: %u)", DEFAULT_VALIDATION_CACHE_BYTES >> 20), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
+    argsman.AddArg("-rangeproofcache", strprintf("Enable the range proof validation cache (default: %u). Use -norangeproofcache to disable.", 1), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-maxtipage=<n>",
                    strprintf("Maximum tip age in seconds to consider node in initial block download (default: %u)",
                              Ticks<std::chrono::seconds>(DEFAULT_MAX_TIP_AGE)),

src/script/sigcache.cpp

@@ -5,7 +5,9 @@
 
 #include <script/sigcache.h>
 
+#include <common/args.h>
 #include <crypto/sha256.h>
+#include <hash.h>
 #include <logging.h>
 #include <pubkey.h>
 #include <random.h>
@@ -20,10 +22,8 @@
 SignatureCache::SignatureCache(const size_t max_size_bytes)
 {
     uint256 nonce = GetRandHash();
-    // We want the nonce to be 64 bytes long to force the hasher to process
-    // this chunk, which makes later hash computations more efficient. We
-    // just write our 32-byte entropy, and then pad with 'E' for ECDSA and
-    // 'S' for Schnorr (followed by 0 bytes).
+    // Use 64-byte, type-specific salted midstates so later hash computations
+    // can start after the first SHA256 chunk.
     static constexpr unsigned char PADDING_ECDSA[32] = {'E'};
     static constexpr unsigned char PADDING_SCHNORR[32] = {'S'};
     static constexpr unsigned char PADDING_RANGE_PROOF[32] = {'r'};
@@ -32,10 +32,8 @@ SignatureCache::SignatureCache(const size_t max_size_bytes)
     m_salted_hasher_ecdsa.Write(PADDING_ECDSA, 32);
     m_salted_hasher_schnorr.Write(nonce.begin(), 32);
     m_salted_hasher_schnorr.Write(PADDING_SCHNORR, 32);
-    m_salted_hasher_range_proof.Write(nonce.begin(), 32);
-    m_salted_hasher_range_proof.Write(PADDING_RANGE_PROOF, 32);
-    m_salted_hasher_surjection_proof.Write(nonce.begin(), 32);
-    m_salted_hasher_surjection_proof.Write(PADDING_SURJECTION_PROOF, 32);
+    m_salted_hasher_range_proof << nonce << PADDING_RANGE_PROOF;
+    m_salted_hasher_surjection_proof << nonce << PADDING_SURJECTION_PROOF;
 
     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",
@@ -55,13 +53,41 @@ void SignatureCache::ComputeEntrySchnorr(uint256& entry, const uint256& hash, Sp
 }
 
 // ELEMENTS:
-void SignatureCache::ComputeEntryRangeProof(uint256& entry, const std::vector<unsigned char>& proof, const std::vector<unsigned char>& commitment, const std::vector<unsigned char>& asset_commitment, const CScript& scriptPubKey) const {
-    CSHA256 hasher = m_salted_hasher_range_proof;
-    hasher.Write(proof.data(), proof.size()).Write(commitment.data(), commitment.size()).Write(asset_commitment.data(), asset_commitment.size()).Write(scriptPubKey.data(), scriptPubKey.size()).Finalize(entry.begin());
+void SignatureCache::ComputeEntryRangeProof(uint256& entry,
+                                            const std::vector<unsigned char>& proof,
+                                            const std::vector<unsigned char>& commitment,
+                                            const std::vector<unsigned char>& asset_commitment,
+                                            const CScript& script_pub_key) const
+{
+    HashWriter hasher = m_salted_hasher_range_proof;
+    // We commit to both commitments and the scriptPubKey because these are
+    // committed to by the rangeproof itself; a change in any of them would
+    // invalidate the proof. Since these are exactly the arguments to
+    // CachingRangeProofChecker::VerifyRangeProof (below), there is no
+    // additional data that could affect the rangeproof's validity.
+    // Serialization length-prefixes every field, including the variable-length
+    // proof and script, so distinct argument tuples cannot share an encoding.
+    hasher << proof << commitment << asset_commitment << script_pub_key;
+    entry = hasher.GetSHA256();
 }
-void SignatureCache::ComputeEntrySurjectionProof(uint256& entry, const uint256 &hash, const std::vector<unsigned char>& proof, const std::vector<unsigned char>& commitment) const {
-    CSHA256 hasher = m_salted_hasher_surjection_proof;
-    hasher.Write(hash.begin(), 32).Write(proof.data(), proof.size()).Write(commitment.data(), commitment.size()).Finalize(entry.begin());
+
+void SignatureCache::ComputeEntrySurjectionProof(uint256& entry, const uint256 &hash, const std::vector<unsigned char>& proof, const std::vector<unsigned char>& commitment, const std::vector<secp256k1_generator>& vTags) const
+{
+    HashWriter hasher = m_salted_hasher_surjection_proof;
+    // We hash all arguments passed to CachingSurjectionProofChecker::VerifySurjectionProof,
+    // to ensure that any change in the way that the verification function is called will
+    // trigger a cache miss and explicit verification. However, we note that the `wtxid`
+    // (hash) commits to all the other data such that we could technically hash only it.
+    // We retain the other data as a defense against future refactorings.
+    //
+    // Serialize vTags as a flat byte vector (each secp256k1_generator is 64 bytes).
+    std::vector<unsigned char> vTagsBytes;
+    vTagsBytes.reserve(vTags.size() * 64);
+    for (const auto& tag : vTags) {
+        vTagsBytes.insert(vTagsBytes.end(), std::begin(tag.data), std::end(tag.data));
+    }
+    hasher << hash << proof << commitment << vTagsBytes;
+    entry = hasher.GetSHA256();
 }
 
 bool SignatureCache::Get(const uint256& entry, const bool erase)
@@ -109,6 +135,10 @@ namespace {
 // To be called once in AppInit2/TestingSetup to initialize the rangeproof cache
 bool InitRangeproofCache(size_t max_size_bytes)
 {
+    if (!gArgs.GetBoolArg("-rangeproofcache", true)) {
+        LogPrintf("Range proof cache disabled via -norangeproofcache\n");
+        return true;
+    }
     auto setup_results = rangeProofCache.setup_bytes(max_size_bytes);
     if (!setup_results) return false;
     const auto [num_elems, approx_size_bytes] = *setup_results;
@@ -130,11 +160,18 @@ bool InitSurjectionproofCache(size_t max_size_bytes)
 
 bool CachingRangeProofChecker::VerifyRangeProof(const std::vector<unsigned char>& vchRangeProof, const std::vector<unsigned char>& vchValueCommitment, const std::vector<unsigned char>& vchAssetCommitment, const CScript& scriptPubKey, const secp256k1_context* secp256k1_ctx_verify_amounts) const
 {
+    // ELEMENTS: NOTE FOR FUTURE EDITORS: every argument to this function that
+    // carries data (i.e. everything except the secp256k1 context, which is
+    // stateless) MUST be included in ComputeEntryRangeProof. Omitting any
+    // argument risks returning a cached positive result for a proof that was
+    // verified with different inputs.
     uint256 entry;
-    rangeProofCache.ComputeEntryRangeProof(entry, vchRangeProof, vchValueCommitment, vchAssetCommitment, scriptPubKey);
-
-    if (rangeProofCache.Get(entry, !store)) {
-        return true;
+    const bool useCache = gArgs.GetBoolArg("-rangeproofcache", true);
+    if (useCache) {
+        rangeProofCache.ComputeEntryRangeProof(entry, vchRangeProof, vchValueCommitment, vchAssetCommitment, scriptPubKey);
+        if (rangeProofCache.Get(entry, !store)) {
+            return true;
+        }
     }
 
     if (vchRangeProof.size() == 0) {
@@ -163,7 +200,7 @@ bool CachingRangeProofChecker::VerifyRangeProof(const std::vector<unsigned char>
         return false;
     }
 
-    if (store) {
+    if (useCache && store) {
         rangeProofCache.Set(entry);
     }
 
@@ -182,7 +219,7 @@ bool CachingSurjectionProofChecker::VerifySurjectionProof(secp256k1_surjectionpr
     // wtxid commits to all data including surj targets
     // we need to specify the proof and output asset point to be unique
     uint256 entry;
-    surjectionProofCache.ComputeEntrySurjectionProof(entry, wtxid, vchproof, std::vector<unsigned char>(std::begin(gen.data), std::end(gen.data)));
+    surjectionProofCache.ComputeEntrySurjectionProof(entry, wtxid, vchproof, std::vector<unsigned char>(std::begin(gen.data), std::end(gen.data)), vTags);
 
     if (surjectionProofCache.Get(entry, !store)) {
         return true;
@@ -199,5 +236,24 @@ bool CachingSurjectionProofChecker::VerifySurjectionProof(secp256k1_surjectionpr
     return true;
 }
 
+// Test-only hooks (see sigcache.h). Forward to the anonymous-namespace caches.
+void TestComputeEntryRangeProof(uint256& entry,
+                                const std::vector<unsigned char>& proof,
+                                const std::vector<unsigned char>& commitment,
+                                const std::vector<unsigned char>& asset_commitment,
+                                const CScript& script_pub_key)
+{
+    rangeProofCache.ComputeEntryRangeProof(entry, proof, commitment, asset_commitment, script_pub_key);
+}
+
+void TestComputeEntrySurjectionProof(uint256& entry,
+                                     const uint256& hash,
+                                     const std::vector<unsigned char>& proof,
+                                     const std::vector<unsigned char>& commitment,
+                                     const std::vector<secp256k1_generator>& vTags)
+{
+    surjectionProofCache.ComputeEntrySurjectionProof(entry, hash, proof, commitment, vTags);
+}
+
 // END ELEMENTS
 //

src/script/sigcache.h

@@ -9,6 +9,7 @@
 #include <consensus/amount.h>
 #include <crypto/sha256.h>
 #include <cuckoocache.h>
+#include <hash.h>
 #include <script/interpreter.h>
 #include <random.h>
 #include <span.h>
@@ -43,11 +44,11 @@ static_assert(DEFAULT_VALIDATION_CACHE_BYTES == DEFAULT_SIGNATURE_CACHE_BYTES +
 class SignatureCache
 {
 private:
-    //! Entries are SHA256(nonce || 'E' or 'S' || 31 zero bytes || signature hash || public key || signature):
+    //! Salted SHA256 midstates, domain-separated by signature or proof type.
     CSHA256 m_salted_hasher_ecdsa;
     CSHA256 m_salted_hasher_schnorr;
-    CSHA256 m_salted_hasher_range_proof;
-    CSHA256 m_salted_hasher_surjection_proof;
+    HashWriter m_salted_hasher_range_proof;
+    HashWriter m_salted_hasher_surjection_proof;
     typedef CuckooCache::cache<uint256, SignatureCacheHasher> map_type;
     map_type setValid;
     std::shared_mutex cs_sigcache;
@@ -56,10 +57,8 @@ class SignatureCache
     SignatureCache()
     {
         uint256 nonce = GetRandHash();
-        // We want the nonce to be 64 bytes long to force the hasher to process
-        // this chunk, which makes later hash computations more efficient. We
-        // just write our 32-byte entropy, and then pad with 'E' for ECDSA and
-        // 'S' for Schnorr (followed by 0 bytes).
+        // Use 64-byte, type-specific salted midstates so later hash computations
+        // can start after the first SHA256 chunk.
         static constexpr unsigned char PADDING_ECDSA[32] = {'E'};
         static constexpr unsigned char PADDING_SCHNORR[32] = {'S'};
         static constexpr unsigned char PADDING_RANGE_PROOF[32] = {'r'};
@@ -68,10 +67,8 @@ class SignatureCache
         m_salted_hasher_ecdsa.Write(PADDING_ECDSA, 32);
         m_salted_hasher_schnorr.Write(nonce.begin(), 32);
         m_salted_hasher_schnorr.Write(PADDING_SCHNORR, 32);
-        m_salted_hasher_range_proof.Write(nonce.begin(), 32);
-        m_salted_hasher_range_proof.Write(PADDING_RANGE_PROOF, 32);
-        m_salted_hasher_surjection_proof.Write(nonce.begin(), 32);
-        m_salted_hasher_surjection_proof.Write(PADDING_SURJECTION_PROOF, 32);
+        m_salted_hasher_range_proof << nonce << PADDING_RANGE_PROOF;
+        m_salted_hasher_surjection_proof << nonce << PADDING_SURJECTION_PROOF;
     }
 
     SignatureCache(size_t max_size_bytes);
@@ -84,9 +81,13 @@ class SignatureCache
     void ComputeEntrySchnorr(uint256& entry, const uint256 &hash, Span<const unsigned char> sig, const XOnlyPubKey& pubkey) const;
 
     // ELEMENTS:
-    void ComputeEntryRangeProof(uint256& entry, const std::vector<unsigned char>& proof, const std::vector<unsigned char>& commitment, const std::vector<unsigned char>& asset_commitment, const CScript& scriptPubKey) const;
+    void ComputeEntryRangeProof(uint256& entry,
+                                const std::vector<unsigned char>& proof,
+                                const std::vector<unsigned char>& commitment,
+                                const std::vector<unsigned char>& asset_commitment,
+                                const CScript& script_pub_key) const;
 
-    void ComputeEntrySurjectionProof(uint256& entry, const uint256 &hash, const std::vector<unsigned char>& proof, const std::vector<unsigned char>& commitment) const;
+    void ComputeEntrySurjectionProof(uint256& entry, const uint256 &hash, const std::vector<unsigned char>& proof, const std::vector<unsigned char>& commitment, const std::vector<secp256k1_generator>& vTags) const;
 
     bool Get(const uint256& entry, const bool erase);
 
@@ -145,6 +146,20 @@ class CachingSurjectionProofChecker
 [[nodiscard]] bool InitRangeproofCache(size_t max_size_bytes);
 [[nodiscard]] bool InitSurjectionproofCache(size_t max_size_bytes);
 
+// Test-only hooks: expose the (anonymous-namespace) cache-entry computation so
+// unit tests can verify collision-resistance and domain separation. These are
+// NOT part of the consensus/validation API and are only used by unit tests.
+void TestComputeEntryRangeProof(uint256& entry,
+                                const std::vector<unsigned char>& proof,
+                                const std::vector<unsigned char>& commitment,
+                                const std::vector<unsigned char>& asset_commitment,
+                                const CScript& script_pub_key);
+void TestComputeEntrySurjectionProof(uint256& entry,
+                                     const uint256& hash,
+                                     const std::vector<unsigned char>& proof,
+                                     const std::vector<unsigned char>& commitment,
+                                     const std::vector<secp256k1_generator>& vTags);
+
 // END ELEMENTS
 //
 

src/test/CMakeLists.txt

@@ -96,6 +96,7 @@ add_executable(test_elements
   serfloat_tests.cpp
   serialize_tests.cpp
   settings_tests.cpp
+  sigcache_tests.cpp
   sighash_tests.cpp
   sigopcount_tests.cpp
   skiplist_tests.cpp