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 sapling nullifier set #3191

Merged
merged 5 commits into from Apr 26, 2018

Conversation

6 participants
@Eirik0
Contributor

Eirik0 commented Apr 18, 2018

PR for #3057

@Eirik0

This comment has been minimized.

Contributor

Eirik0 commented Apr 18, 2018

There are a few places where I added comments like "// TODO ... sapling nullifiers" which will need to be addressed once sapling nullifiers are implemented.

@braddmiller braddmiller added this to In Review in Consensus Protocol Team Apr 19, 2018

@bitcartel

This comment has been minimized.

Contributor

bitcartel commented Apr 19, 2018

@Eirik0 In a meeting @str4d @daira @bitcartel agreed that it would be a good design to prepare for future nullifier sets, beyond sapling. So use of bool isSapling could be replaced with an enum identifier.

One area to investigate then would be how to avoid a collision / avoid exhausting the single char written to the database, which is used to distinguish records on reading. In this case, distinguishing between many nullifier sets including DB_NULLIFIER = 's'; DB_SAPLING_NULLIFIER = 'S';

Related: https://github.com/zcash/zcash/blob/master/src/serialize.h#L809

@str4d str4d added this to the v1.1.1 milestone Apr 20, 2018

@ebfull

This comment has been minimized.

Contributor

ebfull commented Apr 20, 2018

Do we want this PR to rebase on top of the v4 transaction format changes, and fix the TODOs? I think so. I'm doing the same thing with my IncrementalMerkleTree PR.

@daira

This comment has been minimized.

Contributor

daira commented Apr 21, 2018

@bitcartel wrote:

One area to investigate then would be how to avoid a collision / avoid exhausting the single char written to the database, which is used to distinguish records on reading. In this case, distinguishing between many nullifier sets including DB_NULLIFIER = 's'; DB_SAPLING_NULLIFIER = 'S';

We can cross the single-char-exhaustion bridge when we come to it, or when we change the database format (which we plan to do anyway). In the meantime we just need a treestate enum, that is mapped to a char manually chosen for each treestate when writing to the db.

@Eirik0

This comment has been minimized.

Contributor

Eirik0 commented Apr 23, 2018

@bitcartel I changed bool flag "isSapling" to an enum.

@str4d

Looks good overall!

return false;
}

This comment has been minimized.

@str4d

str4d Apr 25, 2018

Contributor

Leave this bracket in.

Style note: Upstream tends to leave out brackets for one-line conditional scopes; we tend to put them in.

std::map<uint256, std::pair<double, CAmount> > mapDeltas;

This comment has been minimized.

@str4d

str4d Apr 25, 2018

Contributor

Remove whitespace

@@ -131,6 +131,11 @@ class CTxMemPool
uint64_t totalTxSize = 0; //! sum of all mempool tx' byte sizes
uint64_t cachedInnerUsage; //! sum of dynamic memory usage of all the map elements (NOT the maps themselves)
std::map<uint256, const CTransaction*> mapNullifiers;

This comment has been minimized.

@str4d

str4d Apr 25, 2018

Contributor

If you're moving this and making it private, then at the same time rename it to mapSproutNullifiers.

src/coins.h Outdated
@@ -449,8 +459,8 @@ class CCoinsViewCache : public CCoinsViewBacked
// the new current root.
void PopAnchor(const uint256 &rt);
// Marks a nullifier as spent or not.
void SetNullifier(const uint256 &nullifier, bool spent);
// Marks a nullifiers for a given transaction as spent or not.

This comment has been minimized.

@str4d

str4d Apr 25, 2018

Contributor

Marks nullifiers for...

src/coins.h Outdated
@@ -418,6 +426,7 @@ class CCoinsViewCache : public CCoinsViewBacked
mutable uint256 hashAnchor;
mutable CAnchorsMap cacheAnchors;
mutable CNullifiersMap cacheNullifiers;

This comment has been minimized.

@str4d

str4d Apr 25, 2018

Contributor

Almost every single usage of cacheNullifiers is changed by this PR, so it should only add a couple more lines to the diff to rename it to cacheSproutNullifiers. Please add a commit to do this.

@@ -69,7 +70,8 @@ bool CCoinsViewBacked::BatchWrite(CCoinsMap &mapCoins,
const uint256 &hashBlock,
const uint256 &hashAnchor,
CAnchorsMap &mapAnchors,
CNullifiersMap &mapNullifiers) { return base->BatchWrite(mapCoins, hashBlock, hashAnchor, mapAnchors, mapNullifiers); }
CNullifiersMap &mapNullifiers,

This comment has been minimized.

@str4d

str4d Apr 25, 2018

Contributor

Rename these to mapSproutNullifiers while you're at it, as that similarly should be a minimal increase in the overall PR size.

@@ -19,6 +19,7 @@ using namespace std;
static const char DB_ANCHOR = 'A';
static const char DB_NULLIFIER = 's';

This comment has been minimized.

@str4d

str4d Apr 25, 2018

Contributor

Rename to DB_SPROUT_NULLIFIER

remove(txConflict, removed, true);
}

This comment has been minimized.

@str4d

str4d Apr 25, 2018

Contributor

Leave these brackets as-is.

@@ -118,7 +121,6 @@ bool CTxMemPool::addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry,
return true;
}

This comment has been minimized.

@str4d

str4d Apr 25, 2018

Contributor

Leave this whitespace in (unnecessary part of the diff).

@str4d

This comment has been minimized.

Contributor

str4d commented Apr 25, 2018

@zkbot try

@zkbot

This comment has been minimized.

Contributor

zkbot commented Apr 25, 2018

⌛️ Trying commit cab341e with merge e73b99f...

zkbot added a commit that referenced this pull request Apr 25, 2018

@str4d str4d referenced this pull request Apr 25, 2018

Closed

Implement Sapling consensus rules #3065

25 of 25 tasks complete
@zkbot

This comment has been minimized.

Contributor

zkbot commented Apr 25, 2018

☀️ Test successful - pr-try
State: approved= try=True

@Eirik0

This comment has been minimized.

Contributor

Eirik0 commented Apr 25, 2018

@str4d The changes from the review have been made and pushed.

@ebfull

This PR needs to update CheckTransactionWithoutProofVerification to ensure that it enforces the transaction cannot have repeated Sapling nullifiers as well.

@ebfull

ebfull approved these changes Apr 26, 2018

@str4d

str4d approved these changes Apr 26, 2018

utACK

@str4d

This comment has been minimized.

Contributor

str4d commented Apr 26, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Contributor

zkbot commented Apr 26, 2018

📌 Commit 6679855 has been approved by str4d

@zkbot

This comment has been minimized.

Contributor

zkbot commented Apr 26, 2018

⌛️ Testing commit 6679855 with merge e9067e8...

zkbot added a commit that referenced this pull request Apr 26, 2018

@zkbot

This comment has been minimized.

Contributor

zkbot commented Apr 26, 2018

💔 Test failed - pr-merge

@str4d

This comment has been minimized.

Contributor

str4d commented Apr 26, 2018

All transient failures.

@zkbot retry

@zkbot

This comment has been minimized.

Contributor

zkbot commented Apr 26, 2018

⌛️ Testing commit 6679855 with merge a2ff786...

zkbot added a commit that referenced this pull request Apr 26, 2018

@zkbot

This comment has been minimized.

Contributor

zkbot commented Apr 26, 2018

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

@zkbot zkbot merged commit 6679855 into zcash:master Apr 26, 2018

1 check passed

homu Test successful
Details

Consensus Protocol Team automation moved this from In Review to Complete PRs (Ignore) Apr 26, 2018

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