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

Sapling note plaintext (encryption and decryption) #3391

Merged
merged 5 commits into from Jul 24, 2018

Conversation

@bitcartel
Copy link
Contributor

bitcartel commented Jul 12, 2018

Add encryption and decryption of SaplingNotePlaintext and SaplingOutgoingPlaintext classes.

This is part of #3061 to add Sapling note functionality.

Add encryption of SaplingNotePlaintext and SaplingOutgoingPlaintext c…
…lasses.

This is part of #3061 to add Sapling note functionality.

@str4d str4d added this to the v2.0.0 milestone Jul 12, 2018

@str4d str4d added the Sapling label Jul 12, 2018

@str4d str4d added this to In Progress in Zcashd Team Jul 12, 2018

@@ -116,6 +116,68 @@ class SproutNotePlaintext : public BaseNotePlaintext {
) const;
};

typedef std::pair<std::reference_wrapper<const SaplingEncCiphertext>, std::reference_wrapper<const SaplingNoteEncryption>> SaplingNotePlaintextEncryptionResult;

This comment has been minimized.

@str4d

str4d Jul 12, 2018

Contributor

Because this is const SaplingNoteEncryption, I can't pass it through to SaplingOutputPlaintext::encrypt(), which requires non-const.

This comment has been minimized.

@str4d

str4d Jul 12, 2018

Contributor

Also, why is std::reference_wrapper used here?

This comment has been minimized.

@bitcartel

bitcartel Jul 13, 2018

Contributor

I'm going to wait for @ebfull to push commits to this PR before making any changes, but thanks for taking an early look. (and the non-const issue)

I'm using std::reference_wrapper to create a pair of references, rather than copying the SaplingEncCiphertext and SaplingNoteEncryption objects when creating the pair.

However, I now think its safer to just copy the objects and avoid the potential for dangling references, when the underlying object falls out of scope.


boost::optional<SaplingNote> SaplingNotePlaintext::note(const SaplingIncomingViewingKey& ivk) const
{
auto addr = ivk.address( d );

This comment has been minimized.

@arielgabizon

arielgabizon Jul 17, 2018

Contributor

are the spaces in the parentheses intentional?

This comment has been minimized.

@bitcartel

bitcartel Jul 17, 2018

Contributor

No, will fix.

{
public:
uint256 pk_d;
uint256 esk;

This comment has been minimized.

@arielgabizon

arielgabizon Jul 17, 2018

Contributor

The outgoing plaintext has an esk?

This comment has been minimized.

@arielgabizon

arielgabizon Jul 17, 2018

Contributor

Note that it isn't used in decrypt and encrypt

This comment has been minimized.

@daira

daira Jul 17, 2018

Contributor

If the decryptor gets esk then they are able to also decrypt the note ciphertext and check that it was encrypted correctly. This gives them the note plaintext which is (d, v, rcm, memo). As well as the issue of being able to check the note ciphertext correctness, it would take 564 bytes instead of 32 to encrypt these fields separately to ock.

See section 4.17.3 of the spec for details.

@str4d str4d changed the title [WIP] Sapling note plaintext Sapling note plaintext Jul 17, 2018

@str4d str4d moved this from In Progress to In Review in Zcashd Team Jul 17, 2018

@str4d

str4d approved these changes Jul 17, 2018

Copy link
Contributor

str4d left a comment

ut(ACK+cov)

TEST(noteencryption, NotePlaintext)
{
using namespace libzcash;
auto sk = SaplingSpendingKey(uint256()).expanded_spending_key();

This comment has been minimized.

@str4d

str4d Jul 17, 2018

Contributor

Nit: xsk

This comment has been minimized.

@bitcartel

bitcartel Jul 17, 2018

Contributor

Will fix (note that other parts of the test and code also just use sk).

{
using namespace libzcash;
auto sk = SaplingSpendingKey(uint256()).expanded_spending_key();
auto vk = sk.full_viewing_key();

This comment has been minimized.

@str4d

str4d Jul 17, 2018

Contributor

Nit: fvk

This comment has been minimized.

@bitcartel

bitcartel Jul 17, 2018

Contributor

Will fix (note that other parts of the test and code also just use vk).

@bitcartel bitcartel self-assigned this Jul 17, 2018

@bitcartel bitcartel changed the title Sapling note plaintext Sapling note plaintext (encryption and decryption) Jul 18, 2018

auto decrypted_out_ct_unwrapped = decrypted_out_ct.get();

ASSERT_TRUE(decrypted_out_ct_unwrapped.pk_d == out_pt.pk_d);
ASSERT_TRUE(decrypted_out_ct_unwrapped.esk == out_pt.esk);

This comment has been minimized.

@arielgabizon

arielgabizon Jul 18, 2018

Contributor

I don't see here a testing of actually decrypting the outgoing note correctly, and not just the "metadata" (do we have better name for it?) (esk,pkd)

This comment has been minimized.

@arielgabizon

arielgabizon Jul 18, 2018

Contributor

Ahh I guess the metadata is what we're calling outgoing plaintext, we might need to explain that somewhere as at first read outgoing plaintext sounds like everything that's going out, including the note plaintext @daira

This comment has been minimized.

@bitcartel

bitcartel Jul 19, 2018

Contributor

In pairing with @arielgabizon we identified we need to test the sender decrypting the note ciphertext, using the values obtained through decrypting the "metadata".

This comment has been minimized.

@bitcartel

bitcartel Jul 20, 2018

Contributor

@arielgabizon I added decryption using full viewing key per spec 4.17.3 and updated the test.

This comment has been minimized.

@arielgabizon

arielgabizon Jul 23, 2018

Contributor

Nice job! Except perhaps the method names having 'UsingFullViewingKey'.. the method name suggests
you're doing the whole process - getting (esk,pkd) and decrypting note in that method;
whereas you're just doing the second part after getting (esk,pkd),
so I'd either refactor to get a method that does the whole thing under this name,
or call it 'UsingOutgoingPlaintext'

This comment has been minimized.

@bitcartel

bitcartel Jul 23, 2018

Contributor

@arielgabizon I've updated to just call it decrypt so we use function overloading for now. 'UsingOutgoingPlaintext' implies we pass in a SaplingOutgoingPlaintext object when in fact we are just passing in some components. I think we can rename things at a later date. If this is good, please ack review.


SaplingNotePlaintext(const SaplingNote& note, std::array<unsigned char, ZC_MEMO_SIZE> memo);

static boost::optional<SaplingNotePlaintext> decrypt(

This comment has been minimized.

@arielgabizon

arielgabizon Jul 18, 2018

Contributor

We should add some comments to help understand the receiver vs sender roles; e.g. add above this method 'Enables a note recipient to decrypt their note'

@mdr0id mdr0id requested a review from gtank Jul 23, 2018

@ebfull

This comment has been minimized.

Copy link
Contributor

ebfull commented Jul 23, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jul 23, 2018

📌 Commit e739ca2 has been approved by ebfull

@ebfull

ebfull approved these changes Jul 23, 2018

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jul 23, 2018

⌛️ Testing commit e739ca2 with merge a4c3c19...

zkbot added a commit that referenced this pull request Jul 23, 2018

Auto merge of #3391 - bitcartel:3061_sapling_note_encryption, r=ebfull
Sapling note plaintext (encryption and decryption)

Add encryption and decryption of SaplingNotePlaintext and SaplingOutgoingPlaintext classes.

This is part of #3061 to add Sapling note functionality.
@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Jul 23, 2018

Thanks @ebfull !

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jul 24, 2018

💥 Test timed out

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Jul 24, 2018

12 hrs for test time out?!

@zkbot retry

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jul 24, 2018

⌛️ Testing commit e739ca2 with merge a7a62b7...

zkbot added a commit that referenced this pull request Jul 24, 2018

Auto merge of #3391 - bitcartel:3061_sapling_note_encryption, r=ebfull
Sapling note plaintext (encryption and decryption)

Add encryption and decryption of SaplingNotePlaintext and SaplingOutgoingPlaintext classes.

This is part of #3061 to add Sapling note functionality.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jul 24, 2018

☀️ Test successful - pr-merge
Approved by: ebfull
Pushing a7a62b7 to master...

@zkbot zkbot merged commit e739ca2 into zcash:master Jul 24, 2018

1 check passed

homu Test successful
Details

Zcashd Team automation moved this from In Review to Released (Merged in Master) Jul 24, 2018

TEST(noteencryption, NotePlaintext)
{
using namespace libzcash;
auto xsk = SaplingSpendingKey(uint256()).expanded_spending_key();

This comment has been minimized.

@daira

daira Jul 25, 2018

Contributor

xsk is used in the draft ZIP 32 for an extended spending key. It would be better to disambiguate, e.g. by using expsk for expanded spending keys.


auto ovk = random_uint256();
auto cv = random_uint256();
auto cm = random_uint256();

This comment has been minimized.

@daira

daira Jul 25, 2018

Contributor

Why not use note.cm()?

This comment has been minimized.

@bitcartel

bitcartel Jul 25, 2018

Contributor

@daira Good spot. Does it matter? Let's ping @ebfull about this.

This comment has been minimized.

@ebfull

ebfull Jul 25, 2018

Contributor

It doesn't matter for the test but yeah it could be changed to that.

auto epk = encryptor.get_epk();

// Try to decrypt
auto foo = SaplingNotePlaintext::decrypt(

This comment has been minimized.

@daira

daira Jul 25, 2018

Contributor

Avoid foo, bar, etc. variable names (those are for cases where what you're referring to could be anything).

This comment has been minimized.

@ebfull

ebfull Jul 25, 2018

Contributor

I know, it was me being flippant and frustrated by the lack of shadowing in C++.

return SaplingNotePlaintextEncryptionResult(encciphertext.get(), enc);
}


This comment has been minimized.

@daira

daira Jul 25, 2018

Contributor

We have both SaplingOutgoingPlaintext and SaplingOutPlaintext, but I don't see a definition of the latter in this PR. Why do we need two types?

This comment has been minimized.

@bitcartel

bitcartel Jul 25, 2018

Contributor

@daira SaplingOutPlaintext is a typedef for the array of raw bytes:

typedef std::array<unsigned char, ZC_SAPLING_OUTPLAINTEXT_SIZE> SaplingOutPlaintext;
whereas SaplingOutgoingPlaintext is the class containing esk, pk_d.

I agree that the naming of our various components can be confusing. We can refactor later if necessary.

This comment has been minimized.

@daira

daira Jul 26, 2018

Contributor

This naming makes me sad; yes let's fix it.

const SaplingEncCiphertext &ciphertext,
const uint256 &epk,
const uint256 &esk,
const uint256 &pk_d

This comment has been minimized.

@daira

daira Jul 25, 2018

Contributor

This is confusing; is it the version of note ciphertext decryption that is only used when decrypting using a full viewing key? Why isn't it named that way?

In general I'm having difficulty following the code in this PR because it is structured very differently to the spec. (That's probably why I didn't finish reviewing it before it was merged.)

This comment has been minimized.

@bitcartel

bitcartel Jul 25, 2018

Contributor

@daira Yes, it was named with full viewing key, but then changed. Reason here: #3391 (comment)

This comment has been minimized.

@daira

daira Jul 26, 2018

Contributor

I still don't like this name, but yes we can refactor later.

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