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 SaplingNote class #3272

Merged
merged 3 commits into from Jun 13, 2018

Conversation

@bitcartel
Copy link
Contributor

bitcartel commented May 16, 2018

Part of #3061, adding SaplingNote class.

@bitcartel bitcartel force-pushed the bitcartel:3061_sapling_add_notes branch 2 times, most recently from a758333 to dcd1a00 May 18, 2018

@bitcartel bitcartel force-pushed the bitcartel:3061_sapling_add_notes branch 2 times, most recently from a67d372 to fb8923d Jun 4, 2018

@zcash zcash deleted a comment from zkbot Jun 8, 2018

@bitcartel bitcartel changed the title [WIP] Add Sapling Notes Add SaplingNote class Jun 8, 2018

@bitcartel bitcartel added the wallet label Jun 8, 2018

@bitcartel bitcartel self-assigned this Jun 8, 2018

@bitcartel bitcartel added this to the v2.0.0 milestone Jun 8, 2018

@bitcartel bitcartel added this to In Review in Zcashd Team Jun 8, 2018

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Jun 8, 2018

This PR does not implement SaplingNotePlaintext, which will rely on NoteEncryption being updated for Sapling #3324

}

// Call librustzcash to compute the commitment
uint256 SaplingNote::cm() const {

This comment has been minimized.

@ebfull

ebfull Jun 9, 2018

Contributor

I would rather return an optional from this.

This comment has been minimized.

@bitcartel

bitcartel Jun 9, 2018

Contributor

The librustzcash functions return false if there is an error. We then throw a std::runtime error.

Under what circumstances do we want to return an optional with no value?

This comment has been minimized.

@ebfull

ebfull Jun 10, 2018

Contributor

I would return an optional from librustzcash if I could cross FFI boundaries with such types. :) In the same way that we return an optional from APIs involving diversifiers. I don't want anyone forgetting that commitments cannot always be computed; they could in the old code but they can't now.

If using an exception here makes something else easier then you could dismiss my review. Everything else looked fine.

This comment has been minimized.

@str4d

str4d Jun 10, 2018

Contributor

Ideally, we'd return a Result<Optional<cm>> from librustzcash, to enable a clear distinction between "inputs are invalid" and "can't compute nullifier". Returning a boolean inherently means we lose information. We could simulate it by having a separate librustzcash function that checks the inputs (to trigger an error) vs the current function (to trigger returning boost::none), but that might be more overhead than we need.

This comment has been minimized.

@bitcartel

bitcartel Jun 12, 2018

Contributor

For future: the librustzcash function could return an int value, instead of bool, where 0=ok, and anything else is some kind of errcode which is defined in the header file.

This comment has been minimized.

@bitcartel

bitcartel Jun 12, 2018

Contributor

Pushed a commit to return optional.

}

// Call librustzcash to compute the nullifier
uint256 SaplingNote::nullifier(const SaplingSpendingKey& sk, const uint64_t position) const

This comment has been minimized.

@ebfull

ebfull Jun 9, 2018

Contributor

Also would rather return an optional here.

This comment has been minimized.

@bitcartel

bitcartel Jun 9, 2018

Contributor

Ditto above.

We did talk about having an optional, for when the wallet doesn't have a position yet, but in that case the wallet wouldn't even call this function and could create an optional with no value.

@@ -38,6 +40,59 @@ uint256 SproutNote::nullifier(const SproutSpendingKey& a_sk) const {
return PRF_nf(a_sk, rho);
}

// Create a note based on a given spending key, with random r and value. Useful for testing.

This comment has been minimized.

@str4d

str4d Jun 10, 2018

Contributor

I'd prefer that testing-only functions live either in the relevant test file, or src/utiltest.cpp. This could be refactored into a SaplingNote::new() function that still generates r, but takes SaplingPaymentAddress and value as inputs (which can be provided by the testing function).

This comment has been minimized.

@bitcartel

bitcartel Jun 12, 2018

Contributor

Pushed a commit to resolve this.

}

// Call librustzcash to compute the commitment
uint256 SaplingNote::cm() const {

This comment has been minimized.

@str4d

str4d Jun 10, 2018

Contributor

Ideally, we'd return a Result<Optional<cm>> from librustzcash, to enable a clear distinction between "inputs are invalid" and "can't compute nullifier". Returning a boolean inherently means we lose information. We could simulate it by having a separate librustzcash function that checks the inputs (to trigger an error) vs the current function (to trigger returning boost::none), but that might be more overhead than we need.

using namespace libzcash;

// Test data from https://github.com/zcash-hackworks/zcash-test-vectors/blob/master/sapling_key_components.py
TEST(SaplingNote, TestVectors)

This comment has been minimized.

@str4d

str4d Jun 10, 2018

Contributor

Non-blocking: once zcash-hackworks/zcash-test-vectors#4 is merged, we should modify this to load a JSON test data file and check every test vector.

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Jun 12, 2018

@str4d @ebfull Ready for re-review. Commits pushed to address review comments.

@ebfull

ebfull approved these changes Jun 12, 2018

result.begin()
))
{
return boost::optional<uint256>();

This comment has been minimized.

@ebfull

ebfull Jun 12, 2018

Contributor

I thought there was a boost::none or something you can use instead. Not blocking.

This comment has been minimized.

@str4d

str4d Jun 12, 2018

Contributor

Yes, return boost::none; works.

This comment has been minimized.

@bitcartel

bitcartel Jun 12, 2018

Contributor

Equivalent. Updated to use boost::none as its easier on the eyes.

@str4d
Copy link
Contributor

str4d left a comment

The librustzcash commit is blocking.

$(package)_sha256_hash=b96a0646d4c4856bc6171dc26cce10644a6129ac92b73a91f94246fb6b7f3516
$(package)_git_commit=18f4945d942cc53e336c40bf13080934179a9047
$(package)_sha256_hash=2ec6ff7aa31e1482637a363772de7866d8f5f3712784ef60827b3ad725e8dbd7
$(package)_git_commit=11552c1579656713495a34f102ba6cbeae8ea780

This comment has been minimized.

@str4d

str4d Jun 12, 2018

Contributor

This should be the merge commit 0af1ce8bf121e1ad367db907c39d214581e270a6.

This comment has been minimized.

@bitcartel

bitcartel Jun 12, 2018

Contributor

Fixed

result.begin()
))
{
return boost::optional<uint256>();

This comment has been minimized.

@str4d

str4d Jun 12, 2018

Contributor

Yes, return boost::none; works.

@daira

daira approved these changes Jun 12, 2018

Copy link
Contributor

daira left a comment

ut(ACK+cov)

@bitcartel bitcartel force-pushed the bitcartel:3061_sapling_add_notes branch from c0461d2 to 268e5df Jun 12, 2018

@ebfull

This comment has been minimized.

Copy link
Contributor

ebfull commented Jun 12, 2018

re-ACK!

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Jun 12, 2018

Two ACKs and the librustzcash commit id has been updated which unblocks another ACK.
@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jun 12, 2018

📌 Commit 268e5df has been approved by bitcartel

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jun 12, 2018

⌛️ Testing commit 268e5df with merge 6bebf28...

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

Auto merge of #3272 - bitcartel:3061_sapling_add_notes, r=bitcartel
Add SaplingNote class

Part of #3061, adding SaplingNote class.
@str4d

str4d approved these changes Jun 12, 2018

Copy link
Contributor

str4d left a comment

ut(ACK+cov)

@bitcartel bitcartel moved this from In Review to Released (Merged in Master) in Zcashd Team Jun 12, 2018

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jun 12, 2018

💔 Test failed - pr-merge

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Jun 12, 2018

Transient timeout (thanks EC2).

@zkbot retry

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jun 12, 2018

⌛️ Testing commit 268e5df with merge 4f18c69...

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

Auto merge of #3272 - bitcartel:3061_sapling_add_notes, r=bitcartel
Add SaplingNote class

Part of #3061, adding SaplingNote class.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jun 13, 2018

☀️ Test successful - pr-merge
Approved by: bitcartel
Pushing 4f18c69 to master...

@zkbot zkbot merged commit 268e5df into zcash:master Jun 13, 2018

1 check passed

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