Merge pull request stellar#2605 from graydon/bug-2604-mutex-guard-gha…

…sher

Remove gHasher and use stack-local incremental SHA256 class.

Reviewed-by: MonsieurNicolas

src/bucket/BucketList.cpp

@@ -30,10 +30,10 @@ BucketLevel::BucketLevel(uint32_t i)
 uint256
 BucketLevel::getHash() const
 {
-    auto hsh = SHA256::create();
-    hsh->add(mCurr->getHash());
-    hsh->add(mSnap->getHash());
-    return hsh->finish();
+    SHA256 hsh;
+    hsh.add(mCurr->getHash());
+    hsh.add(mSnap->getHash());
+    return hsh.finish();
 }
 
 FutureBucket const&
@@ -367,12 +367,12 @@ uint256
 BucketList::getHash() const
 {
     ZoneScoped;
-    auto hsh = SHA256::create();
+    SHA256 hsh;
     for (auto const& lev : mLevels)
     {
-        hsh->add(lev.getHash());
+        hsh.add(lev.getHash());
     }
-    return hsh->finish();
+    return hsh.finish();
 }
 
 // levelShouldSpill is the set of boundaries at which each level should spill,

src/bucket/BucketOutputIterator.cpp

@@ -42,7 +42,6 @@ BucketOutputIterator::BucketOutputIterator(std::string const& tmpDir,
     : mFilename(randomBucketName(tmpDir))
     , mOut(ctx, doFsync)
     , mBuf(nullptr)
-    , mHasher(SHA256::create())
     , mKeepDeadEntries(keepDeadEntries)
     , mMeta(meta)
     , mMergeCounters(mc)
@@ -96,7 +95,7 @@ BucketOutputIterator::put(BucketEntry const& e)
         if (mCmp(*mBuf, e))
         {
             ++mMergeCounters.mOutputIteratorActualWrites;
-            mOut.writeOne(*mBuf, mHasher.get(), &mBytesPut);
+            mOut.writeOne(*mBuf, &mHasher, &mBytesPut);
             mObjectsPut++;
         }
     }
@@ -117,7 +116,7 @@ BucketOutputIterator::getBucket(BucketManager& bucketManager,
     ZoneScoped;
     if (mBuf)
     {
-        mOut.writeOne(*mBuf, mHasher.get(), &mBytesPut);
+        mOut.writeOne(*mBuf, &mHasher, &mBytesPut);
         mObjectsPut++;
         mBuf.reset();
     }
@@ -135,7 +134,7 @@ BucketOutputIterator::getBucket(BucketManager& bucketManager,
         }
         return std::make_shared<Bucket>();
     }
-    return bucketManager.adoptFileAsBucket(mFilename, mHasher->finish(),
+    return bucketManager.adoptFileAsBucket(mFilename, mHasher.finish(),
                                            mObjectsPut, mBytesPut, mergeKey);
 }
 }

src/bucket/BucketOutputIterator.h

@@ -27,7 +27,7 @@ class BucketOutputIterator
     XDROutputFileStream mOut;
     BucketEntryIdCmp mCmp;
     std::unique_ptr<BucketEntry> mBuf;
-    std::unique_ptr<SHA256> mHasher;
+    SHA256 mHasher;
     size_t mBytesPut{0};
     size_t mObjectsPut{0};
     bool mKeepDeadEntries{true};

src/crypto/SHA.cpp

@@ -25,31 +25,13 @@ sha256(ByteSlice const& bin)
     return out;
 }
 
-class SHA256Impl : public SHA256, NonCopyable
-{
-    crypto_hash_sha256_state mState;
-    bool mFinished;
-
-  public:
-    SHA256Impl();
-    void reset() override;
-    void add(ByteSlice const& bin) override;
-    uint256 finish() override;
-};
-
-std::unique_ptr<SHA256>
-SHA256::create()
-{
-    return std::make_unique<SHA256Impl>();
-}
-
-SHA256Impl::SHA256Impl() : mFinished(false)
+SHA256::SHA256()
 {
     reset();
 }
 
 void
-SHA256Impl::reset()
+SHA256::reset()
 {
     if (crypto_hash_sha256_init(&mState) != 0)
     {
@@ -59,7 +41,7 @@ SHA256Impl::reset()
 }
 
 void
-SHA256Impl::add(ByteSlice const& bin)
+SHA256::add(ByteSlice const& bin)
 {
     ZoneScoped;
     if (mFinished)
@@ -73,7 +55,7 @@ SHA256Impl::add(ByteSlice const& bin)
 }
 
 uint256
-SHA256Impl::finish()
+SHA256::finish()
 {
     uint256 out;
     assert(out.size() == crypto_hash_sha256_BYTES);

src/crypto/SHA.h

@@ -6,6 +6,7 @@
 
 #include "crypto/ByteSlice.h"
 #include "crypto/XDRHasher.h"
+#include "sodium/crypto_hash_sha256.h"
 #include "xdr/Stellar-types.h"
 #include <memory>
 
@@ -18,25 +19,24 @@ uint256 sha256(ByteSlice const& bin);
 // SHA256 in incremental mode, for large inputs.
 class SHA256
 {
+    crypto_hash_sha256_state mState;
+    bool mFinished{false};
+
   public:
-    static std::unique_ptr<SHA256> create();
-    virtual ~SHA256(){};
-    virtual void reset() = 0;
-    virtual void add(ByteSlice const& bin) = 0;
-    virtual uint256 finish() = 0;
+    SHA256();
+    void reset();
+    void add(ByteSlice const& bin);
+    uint256 finish();
 };
 
 // Helper for xdrSha256 below.
 struct XDRSHA256 : XDRHasher<XDRSHA256>
 {
-    std::unique_ptr<SHA256> state;
-    XDRSHA256() : state(SHA256::create())
-    {
-    }
+    SHA256 state;
     void
     hashBytes(unsigned char const* bytes, size_t size)
     {
-        state->add(ByteSlice(bytes, size));
+        state.add(ByteSlice(bytes, size));
     }
 };
 
@@ -53,7 +53,7 @@ xdrSha256(T const& t)
     XDRSHA256 xs;
     xdr::archive(xs, t);
     xs.flush();
-    return xs.state->finish();
+    return xs.state.finish();
 }
 
 // HMAC-SHA256 (keyed)

src/crypto/SecretKey.cpp

@@ -35,7 +35,6 @@ namespace stellar
 
 static std::mutex gVerifySigCacheMutex;
 static RandomEvictionCache<Hash, bool> gVerifySigCache(0xffff);
-static std::unique_ptr<SHA256> gHasher = SHA256::create();
 static uint64_t gVerifyCacheHit = 0;
 static uint64_t gVerifyCacheMiss = 0;
 
@@ -45,11 +44,11 @@ verifySigCacheKey(PublicKey const& key, Signature const& signature,
 {
     assert(key.type() == PUBLIC_KEY_TYPE_ED25519);
 
-    gHasher->reset();
-    gHasher->add(key.ed25519());
-    gHasher->add(signature);
-    gHasher->add(bin);
-    return gHasher->finish();
+    SHA256 hasher;
+    hasher.add(key.ed25519());
+    hasher.add(signature);
+    hasher.add(bin);
+    return hasher.finish();
 }
 
 SecretKey::SecretKey() : mKeyType(PUBLIC_KEY_TYPE_ED25519)

src/crypto/test/CryptoTests.cpp

@@ -96,9 +96,9 @@ TEST_CASE("Stateful SHA256 tests", "[crypto]")
     for (auto const& pair : sha256TestVectors)
     {
         LOG(DEBUG) << "fixed test vector SHA256: \"" << pair.second << "\"";
-        auto h = SHA256::create();
-        h->add(pair.first);
-        auto hash = binToHex(h->finish());
+        SHA256 h;
+        h.add(pair.first);
+        auto hash = binToHex(h.finish());
         CHECK(hash.size() == pair.second.size());
         CHECK(hash == pair.second);
     }
@@ -257,6 +257,32 @@ TEST_CASE("sign and verify benchmarking", "[crypto-bench][bench][!hide]")
     }
 }
 
+TEST_CASE("verify-hit benchmarking", "[crypto-bench][bench][!hide]")
+{
+    size_t k = 10;
+    size_t n = 100000;
+    std::vector<SignVerifyTestcase> cases;
+    for (size_t i = 0; i < k; ++i)
+    {
+        cases.push_back(SignVerifyTestcase::create());
+    }
+
+    for (auto& c : cases)
+    {
+        c.sign();
+    }
+
+    LOG(INFO) << "Benchmarking " << n << " verify-hits on " << k
+              << " signatures";
+    for (size_t i = 0; i < n; ++i)
+    {
+        for (auto& c : cases)
+        {
+            c.verify();
+        }
+    }
+}
+
 TEST_CASE("StrKey tests", "[crypto]")
 {
     std::regex b32("^([A-Z2-7])+$");

src/herder/HerderImpl.cpp

@@ -1207,21 +1207,21 @@ static Hash
 getQmapHash(QuorumTracker::QuorumMap const& qmap)
 {
     ZoneScoped;
-    std::unique_ptr<SHA256> hasher = SHA256::create();
+    SHA256 hasher;
     std::map<NodeID, SCPQuorumSetPtr> ordered_map(qmap.begin(), qmap.end());
     for (auto const& pair : ordered_map)
     {
-        hasher->add(xdr::xdr_to_opaque(pair.first));
+        hasher.add(xdr::xdr_to_opaque(pair.first));
         if (pair.second)
         {
-            hasher->add(xdr::xdr_to_opaque(*pair.second));
+            hasher.add(xdr::xdr_to_opaque(*pair.second));
         }
         else
         {
-            hasher->add("\0");
+            hasher.add("\0");
         }
     }
-    return hasher->finish();
+    return hasher.finish();
 }
 
 void

src/herder/TxSetFrame.cpp

@@ -442,13 +442,13 @@ TxSetFrame::getContentsHash()
     if (!mHash)
     {
         sortForHash();
-        auto hasher = SHA256::create();
-        hasher->add(mPreviousLedgerHash);
+        SHA256 hasher;
+        hasher.add(mPreviousLedgerHash);
         for (unsigned int n = 0; n < mTransactions.size(); n++)
         {
-            hasher->add(xdr::xdr_to_opaque(mTransactions[n]->getEnvelope()));
+            hasher.add(xdr::xdr_to_opaque(mTransactions[n]->getEnvelope()));
         }
-        mHash = make_optional<Hash>(hasher->finish());
+        mHash = make_optional<Hash>(hasher.finish());
     }
     return *mHash;
 }

src/history/HistoryArchive.cpp

@@ -203,15 +203,15 @@ HistoryArchiveState::getBucketListHash() const
     // relatively-different representations. Everything will explode if there is
     // any difference in these algorithms anyways, so..
 
-    auto totalHash = SHA256::create();
+    SHA256 totalHash;
     for (auto const& level : currentBuckets)
     {
-        auto levelHash = SHA256::create();
-        levelHash->add(hexToBin(level.curr));
-        levelHash->add(hexToBin(level.snap));
-        totalHash->add(levelHash->finish());
+        SHA256 levelHash;
+        levelHash.add(hexToBin(level.curr));
+        levelHash.add(hexToBin(level.snap));
+        totalHash.add(levelHash.finish());
     }
-    return totalHash->finish();
+    return totalHash.finish();
 }
 
 std::vector<std::string>

src/history/test/HistoryTestsUtils.cpp

@@ -148,12 +148,12 @@ BucketOutputIteratorForTesting::writeTmpTestBucket()
 
     // Finish writing and close the bucket file
     REQUIRE(mBuf);
-    mOut.writeOne(*mBuf, mHasher.get(), &mBytesPut);
+    mOut.writeOne(*mBuf, &mHasher, &mBytesPut);
     mObjectsPut++;
     mBuf.reset();
     mOut.close();
 
-    return std::pair<std::string, uint256>(mFilename, mHasher->finish());
+    return std::pair<std::string, uint256>(mFilename, mHasher.finish());
 };
 
 TestBucketGenerator::TestBucketGenerator(

src/historywork/VerifyBucketWork.cpp

@@ -79,7 +79,7 @@ VerifyBucketWork::spawnVerifier()
         std::static_pointer_cast<VerifyBucketWork>(shared_from_this()));
     app.postOnBackgroundThread(
         [&app, filename, weak, hash]() {
-            auto hasher = SHA256::create();
+            SHA256 hasher;
             asio::error_code ec;
             {
                 ZoneNamedN(verifyZone, "bucket verify", true);
@@ -93,9 +93,9 @@ VerifyBucketWork::spawnVerifier()
                 while (in)
                 {
                     in.read(buf, sizeof(buf));
-                    hasher->add(ByteSlice(buf, in.gcount()));
+                    hasher.add(ByteSlice(buf, in.gcount()));
                 }
-                uint256 vHash = hasher->finish();
+                uint256 vHash = hasher.finish();
                 if (vHash == hash)
                 {
                     CLOG(DEBUG, "History")