New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tests for sapling anchors #3258

Merged
merged 3 commits into from Jun 19, 2018

Conversation

Projects
None yet
5 participants
@Eirik0
Copy link
Contributor

Eirik0 commented May 10, 2018

Closes #3253

@Eirik0 Eirik0 added this to In Review in Protocol Team May 10, 2018

@Eirik0 Eirik0 requested review from ebfull and str4d May 10, 2018

@ebfull

This comment has been minimized.

Copy link
Contributor

ebfull commented May 14, 2018

Good job! Thanks for tackling this.

I would rather not implement template<typename Tree> bool GetAnchorAt(const uint256 &rt, Tree &tree) const; in coins if we don't use it in our code.

Let's add it as an implementation to the coins test wrapper.

@Eirik0 Eirik0 moved this from In Review to In progress in Protocol Team May 14, 2018

@Eirik0 Eirik0 force-pushed the Eirik0:3056-anchor-test-cases branch from 5c32a49 to 3182b4a May 16, 2018

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented May 16, 2018

Made GetAnchorAt(...) specific to those tests.
Force pushed the changes.

@Eirik0 Eirik0 moved this from In progress to In Review in Protocol Team May 16, 2018

@arielgabizon

This comment has been minimized.

Copy link
Contributor

arielgabizon commented May 23, 2018

add "and refactor methods involving Sprout and Sapling trees" to PR name?

@@ -478,13 +478,9 @@ class CCoinsViewCache : public CCoinsViewBacked
CNullifiersMap &mapSaplingNullifiers);


// Adds the tree to mapSproutAnchors and sets the current commitment
// root to this root.
void PushSproutAnchor(const ZCIncrementalMerkleTree &tree);

This comment has been minimized.

@arielgabizon

arielgabizon May 23, 2018

Contributor

Might want to change this to ZCSproutIncrementalMerkleTree at some point

This comment has been minimized.

@Eirik0

Eirik0 May 23, 2018

Contributor

Agreed. That would be consistent with what we have been doing in other places.

#3170 (comment)

@str4d str4d added this to the v2.0.0 milestone May 25, 2018

@@ -142,6 +142,20 @@ class CCoinsViewTest : public CCoinsView
mapNullifiers.clear();
}

template<typename Tree, typename Cache>
void BatchWriteAnchors(Cache& mapAnchors, std::map<uint256, Tree>& cacheAnchors)

This comment has been minimized.

@str4d

str4d May 31, 2018

Contributor

Why is mapAnchors a Cache, and cacheAnchors a std::map?

This comment has been minimized.

@Eirik0

Eirik0 Jun 1, 2018

Contributor

This does seem counterintuitive. This is based on how it is done in coins.cpp

template<typename Map, typename MapIterator, typename MapEntry>
void BatchWriteAnchors(
    Map &mapAnchors,
    Map &cacheAnchors,
    size_t &cachedCoinsUsage
)

and also

void BatchWriteNullifiers(CNullifiersMap& mapNullifiers, std::map<uint256, bool>& cacheNullifiers)

in coins_tests.cpp

I think I called it Cache because the field we pass in, mapSproutAnchors_ in CCoinsViewTest, is equivalent to the field cacheSproutAnchors in CCoinsViewCache

I could changed it from typename Cache to typename Map, but I'm neutral. What do you think?

This comment has been minimized.

@str4d

str4d Jun 13, 2018

Contributor

mapSproutAnchors_ is passed in as the second argument cacheAnchors. The first argument mapAnchors is mapSproutAnchors, which AFAICT is the map. So yes, please change it to typename Map.

BOOST_AUTO_TEST_CASE(anchor_pop_regression_test)
{
anchorPopRegressionTestImpl<ZCIncrementalMerkleTree>(SPROUT);
anchorPopRegressionTestImpl<ZCSaplingIncrementalMerkleTree>(SAPLING);

This comment has been minimized.

@str4d

str4d May 31, 2018

Contributor

Use SCOPED_TRACE to make it clear in error messages which of these two is failing.

This comment has been minimized.

@Eirik0

Eirik0 Jun 1, 2018

Contributor

Is SCOPED_TRACE a gtest thing? Would it be weird to mix and match that with boost?

BOOST_AUTO_TEST_CASE(anchor_regression_test)
{
anchorRegressionTestImpl<ZCIncrementalMerkleTree>(SPROUT);
anchorRegressionTestImpl<ZCSaplingIncrementalMerkleTree>(SAPLING);

This comment has been minimized.

@str4d

str4d May 31, 2018

Contributor

Ditto.

BOOST_AUTO_TEST_CASE(anchors_flush_test)
{
anchorsFlushImpl<ZCIncrementalMerkleTree>(SPROUT);
anchorsFlushImpl<ZCSaplingIncrementalMerkleTree>(SAPLING);

This comment has been minimized.

@str4d

str4d May 31, 2018

Contributor

Ditto.

BOOST_AUTO_TEST_CASE(anchors_test)
{
anchorsTestImpl<ZCIncrementalMerkleTree>(SPROUT);
anchorsTestImpl<ZCSaplingIncrementalMerkleTree>(SAPLING);

This comment has been minimized.

@str4d

str4d May 31, 2018

Contributor

Ditto.

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Jun 4, 2018

@str4d I was not immediately able to get the SCOPED_TRACE to work with the boost tests, but I added BOOST_TEST_CONTEXT which does something similar.

@str4d
Copy link
Contributor

str4d left a comment

Just a couple more tweaks.

BOOST_TEST_CONTEXT("Sprout") {
anchorPopRegressionTestImpl<ZCIncrementalMerkleTree>(SPROUT);
}
BOOST_TEST_CONTEXT("Sprout") {

This comment has been minimized.

@str4d

str4d Jun 13, 2018

Contributor

Sapling

@@ -142,6 +142,20 @@ class CCoinsViewTest : public CCoinsView
mapNullifiers.clear();
}

template<typename Tree, typename Cache>
void BatchWriteAnchors(Cache& mapAnchors, std::map<uint256, Tree>& cacheAnchors)

This comment has been minimized.

@str4d

str4d Jun 13, 2018

Contributor

mapSproutAnchors_ is passed in as the second argument cacheAnchors. The first argument mapAnchors is mapSproutAnchors, which AFAICT is the map. So yes, please change it to typename Map.

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Jun 13, 2018

@str4d renamed the typename from Cache to Map

BOOST_TEST_CONTEXT("Sprout") {
anchorPopRegressionTestImpl<ZCIncrementalMerkleTree>(SPROUT);
}
BOOST_TEST_CONTEXT("Sprout") {

This comment has been minimized.

@str4d

str4d Jun 19, 2018

Contributor

Sapling (not sure why my previous comment about this was marked as obsolete...)

@Eirik0 Eirik0 force-pushed the Eirik0:3056-anchor-test-cases branch from 367ea4a to 762ee0e Jun 19, 2018

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Jun 19, 2018

Fixed the "Sprout" v "Sapling" string mistake and force pushed.

@str4d

str4d approved these changes Jun 19, 2018

Copy link
Contributor

str4d left a comment

utACK

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Jun 19, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jun 19, 2018

📌 Commit 762ee0e has been approved by str4d

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jun 19, 2018

⌛️ Testing commit 762ee0e with merge 9acfa00...

zkbot added a commit that referenced this pull request Jun 19, 2018

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jun 19, 2018

☀️ Test successful - pr-merge
Approved by: str4d
Pushing 9acfa00 to master...

@zkbot zkbot merged commit 762ee0e into zcash:master Jun 19, 2018

1 check passed

homu Test successful
Details

Protocol Team automation moved this from In Review to Complete PRs (Ignore) Jun 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment