Merge #19935: Move SaltedHashers to separate file and add some new ones

281fd1a4a032cded7f9ea9857e3e99fc793c714b Replace KeyIDHasher with SaltedSipHasher (Andrew Chow)
210b693db66e7c5b618014b5a287aee15af00045 Add generic SaltedSipHasher (Andrew Chow)
95e61c1cf2a91d041c8025306ba36f0ea2806894 Move Hashers to util/hasher.{cpp/h} (Andrew Chow)

Pull request description:

  There are existing `SaltedOutPointHasher` and `SaltedTxidHasher` classes used for `std::unordered_map` and `std::unordered_set` that could be useful in other places in the codebase. So we these to their own `saltedhash.{cpp/h}` file. An existing `KeyIDHasher` is moved there too. Additionally, `ScriptIDHasher`, `SaltedPubkeyHasher`, and `SaltedScriptHasher` are added so that they can be used in future work.

  `KeyIDHasher` and `ScriptIDHasher` are not salted so that equality comparisons of maps and sets keyed by `CKeyID` and `CScriptID` will actually work.

  Split from #19602 (and a few other PRs/branches I have).

ACKs for top commit:
  laanwj:
    Code review ACK 281fd1a4a032cded7f9ea9857e3e99fc793c714b
  jonatack:
    ACK 281fd1a4a032cded7f9ea9857e3e99fc793c714b, code review, debug build and ran bitcoind after rebasing to master @ dff0f6f753ea
  fjahr:
    utACK 281fd1a4a032cded7f9ea9857e3e99fc793c714b

Tree-SHA512: bb03b231ccf3c9ecefc997b8da9c3770af4819f9be5b0a72997a103864e84046a2ac39b8eadf0dc9247bdccd53f86f433642e3a098882e6748341a9e7736271b

11 files changed, 126 insertions, 82 deletions

diff --git a/src/Makefile.am b/src/Makefile.am
index ab355dee3f..2871df124c 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -230,6 +230,7 @@ BITCOIN_CORE_H = \
   util/error.h \
   util/fees.h \
   util/golombrice.h \
+  util/hasher.h \
   util/macros.h \
   util/memory.h \
   util/message.h \
@@ -550,6 +551,7 @@ libbitcoin_util_a_SOURCES = \
   util/bytevectorhash.cpp \
   util/error.cpp \
   util/fees.cpp \
+  util/hasher.cpp \
   util/system.cpp \
   util/message.cpp \
   util/moneystr.cpp \
diff --git a/src/coins.cpp b/src/coins.cpp
index 14f58e956c..dd84e720e7 100644
--- a/src/coins.cpp
+++ b/src/coins.cpp
@@ -31,8 +31,6 @@ bool CCoinsViewBacked::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock)
 CCoinsViewCursor *CCoinsViewBacked::Cursor() const { return base->Cursor(); }
 size_t CCoinsViewBacked::EstimateSize() const { return base->EstimateSize(); }
 
-SaltedOutpointHasher::SaltedOutpointHasher() : k0(GetRand(std::numeric_limits<uint64_t>::max())), k1(GetRand(std::numeric_limits<uint64_t>::max())) {}
-
 CCoinsViewCache::CCoinsViewCache(CCoinsView *baseIn) : CCoinsViewBacked(baseIn), cachedCoinsUsage(0) {}
 
 size_t CCoinsViewCache::DynamicMemoryUsage() const {
diff --git a/src/coins.h b/src/coins.h
index a3e241ac90..d2eb42d8cf 100644
--- a/src/coins.h
+++ b/src/coins.h
@@ -8,11 +8,11 @@
 
 #include <compressor.h>
 #include <core_memusage.h>
-#include <crypto/siphash.h>
 #include <memusage.h>
 #include <primitives/transaction.h>
 #include <serialize.h>
 #include <uint256.h>
+#include <util/hasher.h>
 
 #include <assert.h>
 #include <stdint.h>
@@ -82,33 +82,6 @@ public:
     }
 };
 
-class SaltedOutpointHasher
-{
-private:
-    /** Salt */
-    const uint64_t k0, k1;
-
-public:
-    SaltedOutpointHasher();
-
-    /**
-     * This *must* return size_t. With Boost 1.46 on 32-bit systems the
-     * unordered_map will behave unpredictably if the custom hasher returns a
-     * uint64_t, resulting in failures when syncing the chain (#4634).
-     *
-     * Having the hash noexcept allows libstdc++'s unordered_map to recalculate
-     * the hash during rehash, so it does not have to cache the value. This
-     * reduces node's memory by sizeof(size_t). The required recalculation has
-     * a slight performance penalty (around 1.6%), but this is compensated by
-     * memory savings of about 9% which allow for a larger dbcache setting.
-     *
-     * @see https://gcc.gnu.org/onlinedocs/gcc-9.2.0/libstdc++/manual/manual/unordered_associative.html
-     */
-    size_t operator()(const COutPoint& id) const noexcept {
-        return SipHashUint256Extra(k0, k1, id.hash, id.n);
-    }
-};
-
 /**
  * A Coin in one level of the coins database caching hierarchy.
  *
diff --git a/src/index/blockfilterindex.h b/src/index/blockfilterindex.h
index a9c8188bb6..221ac02c9e 100644
--- a/src/index/blockfilterindex.h
+++ b/src/index/blockfilterindex.h
@@ -9,15 +9,11 @@
 #include <chain.h>
 #include <flatfile.h>
 #include <index/base.h>
+#include <util/hasher.h>
 
 /** Interval between compact filter checkpoints. See BIP 157. */
 static constexpr int CFCHECKPT_INTERVAL = 1000;
 
-struct FilterHeaderHasher
-{
-    size_t operator()(const uint256& hash) const { return ReadLE64(hash.begin()); }
-};
-
 /**
  * BlockFilterIndex is used to store and retrieve block filters, hashes, and headers for a range of
  * blocks by height. An index is constructed for each supported filter type with its own database
diff --git a/src/script/sigcache.h b/src/script/sigcache.h
index 512f61f2bf..bf0ba38c2d 100644
--- a/src/script/sigcache.h
+++ b/src/script/sigcache.h
@@ -8,6 +8,7 @@
 
 #include <script/interpreter.h>
 #include <span.h>
+#include <util/hasher.h>
 
 #include <vector>
 
@@ -20,27 +21,6 @@ static const int64_t MAX_MAX_SIG_CACHE_SIZE = 16384;
 
 class CPubKey;
 
-/**
- * We're hashing a nonce into the entries themselves, so we don't need extra
- * blinding in the set hash computation.
- *
- * This may exhibit platform endian dependent behavior but because these are
- * nonced hashes (random) and this state is only ever used locally it is safe.
- * All that matters is local consistency.
- */
-class SignatureCacheHasher
-{
-public:
-    template <uint8_t hash_select>
-    uint32_t operator()(const uint256& key) const
-    {
-        static_assert(hash_select <8, "SignatureCacheHasher only has 8 hashes available.");
-        uint32_t u;
-        std::memcpy(&u, key.begin()+4*hash_select, 4);
-        return u;
-    }
-};
-
 class CachingTransactionSignatureChecker : public TransactionSignatureChecker
 {
 private:
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index d18182c07d..9ae7b921b3 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -1127,5 +1127,3 @@ CTxMemPool::EpochGuard::~EpochGuard()
     ++pool.m_epoch;
     pool.m_has_epoch_guard = false;
 }
-
-SaltedTxidHasher::SaltedTxidHasher() : k0(GetRand(std::numeric_limits<uint64_t>::max())), k1(GetRand(std::numeric_limits<uint64_t>::max())) {}
diff --git a/src/txmempool.h b/src/txmempool.h
index 15797cbc00..9a1aa9bc2b 100644
--- a/src/txmempool.h
+++ b/src/txmempool.h
@@ -15,13 +15,13 @@
 
 #include <amount.h>
 #include <coins.h>
-#include <crypto/siphash.h>
 #include <indirectmap.h>
 #include <optional.h>
 #include <policy/feerate.h>
 #include <primitives/transaction.h>
 #include <sync.h>
 #include <random.h>
+#include <util/hasher.h>
 
 #include <boost/multi_index_container.hpp>
 #include <boost/multi_index/hashed_index.hpp>
@@ -398,20 +398,6 @@ enum class MemPoolRemovalReason {
     REPLACED,    //!< Removed for replacement
 };
 
-class SaltedTxidHasher
-{
-private:
-    /** Salt */
-    const uint64_t k0, k1;
-
-public:
-    SaltedTxidHasher();
-
-    size_t operator()(const uint256& txid) const {
-        return SipHashUint256(k0, k1, txid);
-    }
-};
-
 /**
  * CTxMemPool stores valid-according-to-the-current-best-chain transactions
  * that may be included in the next block.
diff --git a/src/util/hasher.cpp b/src/util/hasher.cpp
new file mode 100644
index 0000000000..5900daf050
--- /dev/null
+++ b/src/util/hasher.cpp
@@ -0,0 +1,19 @@
+// Copyright (c) 2019 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 <random.h>
+#include <util/hasher.h>
+
+#include <limits>
+
+SaltedTxidHasher::SaltedTxidHasher() : k0(GetRand(std::numeric_limits<uint64_t>::max())), k1(GetRand(std::numeric_limits<uint64_t>::max())) {}
+
+SaltedOutpointHasher::SaltedOutpointHasher() : k0(GetRand(std::numeric_limits<uint64_t>::max())), k1(GetRand(std::numeric_limits<uint64_t>::max())) {}
+
+SaltedSipHasher::SaltedSipHasher() : m_k0(GetRand(std::numeric_limits<uint64_t>::max())), m_k1(GetRand(std::numeric_limits<uint64_t>::max())) {}
+
+size_t SaltedSipHasher::operator()(const Span<const unsigned char>& script) const
+{
+    return CSipHasher(m_k0, m_k1).Write(script.data(), script.size()).Finalize();
+}
diff --git a/src/util/hasher.h b/src/util/hasher.h
new file mode 100644
index 0000000000..fa2fea30d8
--- /dev/null
+++ b/src/util/hasher.h
@@ -0,0 +1,99 @@
+// Copyright (c) 2019 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_UTIL_HASHER_H
+#define BITCOIN_UTIL_HASHER_H
+
+#include <crypto/siphash.h>
+#include <primitives/transaction.h>
+#include <uint256.h>
+
+class SaltedTxidHasher
+{
+private:
+    /** Salt */
+    const uint64_t k0, k1;
+
+public:
+    SaltedTxidHasher();
+
+    size_t operator()(const uint256& txid) const {
+        return SipHashUint256(k0, k1, txid);
+    }
+};
+
+class SaltedOutpointHasher
+{
+private:
+    /** Salt */
+    const uint64_t k0, k1;
+
+public:
+    SaltedOutpointHasher();
+
+    /**
+     * This *must* return size_t. With Boost 1.46 on 32-bit systems the
+     * unordered_map will behave unpredictably if the custom hasher returns a
+     * uint64_t, resulting in failures when syncing the chain (#4634).
+     *
+     * Having the hash noexcept allows libstdc++'s unordered_map to recalculate
+     * the hash during rehash, so it does not have to cache the value. This
+     * reduces node's memory by sizeof(size_t). The required recalculation has
+     * a slight performance penalty (around 1.6%), but this is compensated by
+     * memory savings of about 9% which allow for a larger dbcache setting.
+     *
+     * @see https://gcc.gnu.org/onlinedocs/gcc-9.2.0/libstdc++/manual/manual/unordered_associative.html
+     */
+    size_t operator()(const COutPoint& id) const noexcept {
+        return SipHashUint256Extra(k0, k1, id.hash, id.n);
+    }
+};
+
+struct FilterHeaderHasher
+{
+    size_t operator()(const uint256& hash) const { return ReadLE64(hash.begin()); }
+};
+
+/**
+ * We're hashing a nonce into the entries themselves, so we don't need extra
+ * blinding in the set hash computation.
+ *
+ * This may exhibit platform endian dependent behavior but because these are
+ * nonced hashes (random) and this state is only ever used locally it is safe.
+ * All that matters is local consistency.
+ */
+class SignatureCacheHasher
+{
+public:
+    template <uint8_t hash_select>
+    uint32_t operator()(const uint256& key) const
+    {
+        static_assert(hash_select <8, "SignatureCacheHasher only has 8 hashes available.");
+        uint32_t u;
+        std::memcpy(&u, key.begin()+4*hash_select, 4);
+        return u;
+    }
+};
+
+struct BlockHasher
+{
+    // this used to call `GetCheapHash()` in uint256, which was later moved; the
+    // cheap hash function simply calls ReadLE64() however, so the end result is
+    // identical
+    size_t operator()(const uint256& hash) const { return ReadLE64(hash.begin()); }
+};
+
+class SaltedSipHasher
+{
+private:
+    /** Salt */
+    const uint64_t m_k0, m_k1;
+
+public:
+    SaltedSipHasher();
+
+    size_t operator()(const Span<const unsigned char>& script) const;
+};
+
+#endif // BITCOIN_UTIL_HASHER_H
diff --git a/src/validation.h b/src/validation.h
index d10b260d8a..1b4e40cd53 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -23,6 +23,7 @@
 #include <txdb.h>
 #include <versionbits.h>
 #include <serialize.h>
+#include <util/hasher.h>
 
 #include <atomic>
 #include <map>
@@ -93,14 +94,6 @@ static const unsigned int DEFAULT_CHECKLEVEL = 3;
 // Setting the target to >= 550 MiB will make it likely we can respect the target.
 static const uint64_t MIN_DISK_SPACE_FOR_BLOCK_FILES = 550 * 1024 * 1024;
 
-struct BlockHasher
-{
-    // this used to call `GetCheapHash()` in uint256, which was later moved; the
-    // cheap hash function simply calls ReadLE64() however, so the end result is
-    // identical
-    size_t operator()(const uint256& hash) const { return ReadLE64(hash.begin()); }
-};
-
 /** Current sync state passed to tip changed callbacks. */
 enum class SynchronizationState {
     INIT_REINDEX,
diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h
index cec46a0fbb..8f6b69bc78 100644
--- a/src/wallet/scriptpubkeyman.h
+++ b/src/wallet/scriptpubkeyman.h
@@ -304,7 +304,7 @@ private:
 
     /* the HD chain data model (external chain counters) */
     CHDChain m_hd_chain;
-    std::unordered_map<CKeyID, CHDChain, KeyIDHasher> m_inactive_hd_chains;
+    std::unordered_map<CKeyID, CHDChain, SaltedSipHasher> m_inactive_hd_chains;
 
     /* HD derive new child key (on internal or external chain) */
     void DeriveNewChildKey(WalletBatch& batch, CKeyMetadata& metadata, CKey& secret, CHDChain& hd_chain, bool internal = false) EXCLUSIVE_LOCKS_REQUIRED(cs_KeyStore);