Deferred initialization of EmptyMerklePath structures till first use … #4131

zebambam · 2019-09-09T23:22:46Z

…to reduce monolith startup penalty for fuzzing. These structures were previously filled statically at load time.

The performance cost of monolith startup even with an alternative main function is significant when you're loading it many times.

Notes on the approach to fuzzing (why do this patch?):

Yes there are other ways to write fuzzers, but their cost is also higher than necessary. This allows us to access any namespace we need within the monolith and know that at least the build requisites are available without having to do a lot of mocking, shimming, etc.. for each fuzzer.

Notes on the structure of the change:

Larry convinced me to use "LOCK" in the end, which meant bringing in another header. I've added optimization around the lock so that it's not needed every time.

I've added calls to initialize into the comparison operator as well as the accessor. I think that's all the places that are relevant.

…to reduce monolith startup penalty for fuzzing.

zebambam · 2019-09-09T23:25:40Z

I tested this fix btw, and Zcash hits the code and seems stable.

LarryRuane

Nice PR!

src/zcash/IncrementalMerkleTree.hpp

daira

NACK due to undefined behaviour (see comments). Concept ACK though.

Eirik0 · 2019-09-11T19:41:35Z

src/zcash/IncrementalMerkleTree.hpp

        return empty_roots.at(depth);
    }
    template <size_t D, typename H>
    friend bool operator==(const EmptyMerkleRoots<D, H>& a,
                           const EmptyMerkleRoots<D, H>& b);
 private:
+    CCriticalSection cs_EmptyMerkleRoots_private_variables_write;
+    bool initialized;


I think this would be better as a std::atomic<bool>. We could then replace the locking with initialized.compare_exchange_strong(...).

template<size_t Depth, typename Hash> class EmptyMerkleRoots { public: EmptyMerkleRoots() : initialized(false) { } void initialize() const { bool unitialized = false; if (initialized.compare_exchange_strong(unitialized, true)) { empty_roots.at(0) = Hash::uncommitted(); for (size_t d = 1; d <= Depth; d++) { empty_roots.at(d) = Hash::combine(empty_roots.at(d-1), empty_roots.at(d-1), d-1); } } } Hash empty_root(size_t depth) const { initialize(); return empty_roots.at(depth); } template <size_t D, typename H> friend bool operator==(const EmptyMerkleRoots<D, H>& a, const EmptyMerkleRoots<D, H>& b); private: mutable std::atomic<bool> initialized; mutable std::array<Hash, Depth+1> empty_roots; };

This is a neat way to do it I guess. I'm not sure that it's so much better than what's proposed that we should hold up the PR though. Maybe someone else disagrees.

Agreed. The two ways should both be fine.

…only (no more double-locking), and simplified CCriticalSection variable name.

Eirik0 · 2019-09-12T16:06:20Z

@zkbot try

zkbot · 2019-09-12T16:06:30Z

⌛ Trying commit cf89467 with merge 7523619...

…_merkle_trees, r=<try> Deferred initialization of EmptyMerklePath structures till first use … …to reduce monolith startup penalty for fuzzing. These structures were previously filled statically at load time. The performance cost of monolith startup even with an alternative main function is significant when you're loading it many times. Notes on the approach to fuzzing (why do this patch?): Yes there are other ways to write fuzzers, but their cost is also higher than necessary. This allows us to access any namespace we need within the monolith and know that at least the build requisites are available without having to do a lot of mocking, shimming, etc.. for each fuzzer. Notes on the structure of the change: Larry convinced me to use "LOCK" in the end, which meant bringing in another header. I've added optimization around the lock so that it's not needed every time. I've added calls to initialize into the comparison operator as well as the accessor. I think that's all the places that are relevant.

zkbot · 2019-09-12T16:19:55Z

💔 Test failed - pr-try

Eirik0

initialize() could be declared as private because there should be no need to call it outside of the EmptyMerkleRoots class. That being said, there is not necessarily any harm in leaving it public.

src/zcash/IncrementalMerkleTree.hpp

LarryRuane · 2019-09-13T07:01:47Z

What do you think about just precomputing these values? LarryRuane@df20788 No worries about locking or const. I did not measure performance.

But if this change was adopted, the test here

zcash/src/gtest/test_merkletree.cpp

Line 225 in a722aa6

TEST(merkletree, emptyroots) {

should be modified. Currently, it compares the production empty roots with hard-coded blobs, and both could be wrong. It would be better to have the test compute the roots, and compare them with the production roots. In other words, currently, the production code computes and the test has hard-coded values. If instead the production code uses hard-coded values, then the test should compute.

LarryRuane · 2019-09-13T15:14:48Z

I updated the tests as explained in the previous comment: 1c54bd47d00c222613eee3ee63806b7408a3a076. To test the test, I modified one bit of the precomputed empty root data structure, and the new tests failed as expected.

daira · 2019-09-14T21:19:03Z

I like Larry's approach. I reviewed master...LarryRuane:empty-roots and it looks correct to me (i.e. utACK for that branch).

defuse

utACK if the answer to my question is "yes"

Edit: GitHub's UI doesn't display my question because the conversation was marked as resolved. Here it is: https://github.com/zcash/zcash/pull/4131/files#r326359814

src/zcash/IncrementalMerkleTree.hpp

daira · 2019-09-21T12:08:08Z

I really prefer Larry's PR #4136 (it's much easier to see that that one is correct), and I think we should close this in favour of that one. It achieves exactly the same goal.

Eirik0 · 2019-09-26T16:16:36Z

@zebambam Are you ok if we close this ticket in favor of @LarryRuane's implementation?

Co-Authored-By: Eirik Ogilvie-Wigley <eirik@z.cash>

zebambam · 2019-10-11T21:03:51Z

initialize() could be declared as private because there should be no need to call it outside of the EmptyMerkleRoots class. That being said, there is not necessarily any harm in leaving it public.

There's a few reasons why some other code might want to force the initialization of this class, given that it was "supposed" to have been initialized already, and will only get initialized once, it seems only natural to give any code living anywhere in the monolith the opportunity to specify when this happens.

zebambam · 2019-10-14T18:18:16Z

I believe that I started this work without a ticket, or at least I couldn't find one just now. Sorry about that. I've made one here: #4155

precompute EmptyMerklePath roots This is an alternative implementation of #4131.

Deferred initialization of EmptyMerklePath structures till first use …

502507e

…to reduce monolith startup penalty for fuzzing.

zebambam requested review from ebfull, Eirik0 and LarryRuane September 9, 2019 23:22

LarryRuane reviewed Sep 10, 2019

View reviewed changes

src/zcash/IncrementalMerkleTree.hpp Outdated Show resolved Hide resolved

src/zcash/IncrementalMerkleTree.hpp Show resolved Hide resolved

src/zcash/IncrementalMerkleTree.hpp Outdated Show resolved Hide resolved

LarryRuane reviewed Sep 10, 2019

View reviewed changes

src/zcash/IncrementalMerkleTree.hpp Show resolved Hide resolved

daira requested changes Sep 10, 2019

View reviewed changes

zebambam requested review from daira and LarryRuane September 11, 2019 18:43

Eirik0 reviewed Sep 11, 2019

View reviewed changes

Marked the empty_roots array mutable, simplified locking to use LOCK …

cf89467

…only (no more double-locking), and simplified CCriticalSection variable name.

zebambam force-pushed the defer_expensive_intialization_of_empty_merkle_trees branch from 33498e3 to cf89467 Compare September 11, 2019 19:51

Eirik0 added this to Needs Prioritization in Arborist Team via automation Sep 12, 2019

Eirik0 moved this from Needs Prioritization to Security Issue Backlog in Arborist Team Sep 12, 2019

Eirik0 moved this from Security Issue Backlog to Current Sprint in Arborist Team Sep 12, 2019

Eirik0 moved this from Current Sprint to In Review in Arborist Team Sep 12, 2019

Eirik0 suggested changes Sep 12, 2019

View reviewed changes

src/zcash/IncrementalMerkleTree.hpp Outdated Show resolved Hide resolved

src/zcash/IncrementalMerkleTree.hpp Outdated Show resolved Hide resolved

LarryRuane mentioned this pull request Sep 17, 2019

precompute EmptyMerklePath roots #4136

Merged

defuse reviewed Sep 19, 2019

View reviewed changes

src/zcash/IncrementalMerkleTree.hpp Show resolved Hide resolved

Update src/zcash/IncrementalMerkleTree.hpp

99e4f2f

Co-Authored-By: Eirik Ogilvie-Wigley <eirik@z.cash>

Update src/zcash/IncrementalMerkleTree.hpp

1f82216

Co-Authored-By: Eirik Ogilvie-Wigley <eirik@z.cash>

daira assigned zebambam Oct 9, 2019

zebambam closed this Oct 11, 2019

Arborist Team automation moved this from In Review to Released (Merged in Master) Oct 11, 2019

zkbot added a commit that referenced this pull request Oct 17, 2019

Auto merge of #4136 - LarryRuane:empty-roots, r=Eirik0

cfc5212

precompute EmptyMerklePath roots This is an alternative implementation of #4131.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deferred initialization of EmptyMerklePath structures till first use … #4131

Deferred initialization of EmptyMerklePath structures till first use … #4131

zebambam commented Sep 9, 2019

zebambam commented Sep 9, 2019

LarryRuane left a comment

daira left a comment

Eirik0 Sep 11, 2019

Eirik0 Sep 11, 2019

zebambam Sep 11, 2019

Eirik0 Sep 12, 2019

Eirik0 commented Sep 12, 2019

zkbot commented Sep 12, 2019

zkbot commented Sep 12, 2019

Eirik0 left a comment

LarryRuane commented Sep 13, 2019 •

edited

LarryRuane commented Sep 13, 2019 •

edited

daira commented Sep 14, 2019

defuse left a comment •

edited

daira commented Sep 21, 2019 •

edited

Eirik0 commented Sep 26, 2019 •

edited by daira

zebambam commented Oct 11, 2019

zebambam commented Oct 14, 2019

Deferred initialization of EmptyMerklePath structures till first use … #4131

Deferred initialization of EmptyMerklePath structures till first use … #4131

Conversation

zebambam commented Sep 9, 2019

zebambam commented Sep 9, 2019

LarryRuane left a comment

Choose a reason for hiding this comment

daira left a comment

Choose a reason for hiding this comment

Eirik0 Sep 11, 2019

Choose a reason for hiding this comment

Eirik0 Sep 11, 2019

Choose a reason for hiding this comment

zebambam Sep 11, 2019

Choose a reason for hiding this comment

Eirik0 Sep 12, 2019

Choose a reason for hiding this comment

Eirik0 commented Sep 12, 2019

zkbot commented Sep 12, 2019

zkbot commented Sep 12, 2019

Eirik0 left a comment

Choose a reason for hiding this comment

LarryRuane commented Sep 13, 2019 • edited

LarryRuane commented Sep 13, 2019 • edited

daira commented Sep 14, 2019

defuse left a comment • edited

Choose a reason for hiding this comment

daira commented Sep 21, 2019 • edited

Eirik0 commented Sep 26, 2019 • edited by daira

zebambam commented Oct 11, 2019

zebambam commented Oct 14, 2019

LarryRuane commented Sep 13, 2019 •

edited

LarryRuane commented Sep 13, 2019 •

edited

defuse left a comment •

edited

daira commented Sep 21, 2019 •

edited

Eirik0 commented Sep 26, 2019 •

edited by daira