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

Update CCryptoKeyStore with Sapling support #3396

Merged
merged 14 commits into from Aug 3, 2018

Conversation

@arcalinea
Copy link
Contributor

arcalinea commented Jul 12, 2018

Sapling crypter overrides for various CCryptoKeyStore functions such as:

  • HaveSaplingSpendingKey()
  • GetSaplingSpendingKey()

Also includes some changes to prepare for diversified addresses and ZIP 32.

Closes #3389

@arcalinea arcalinea added the Sapling label Jul 12, 2018

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

@arcalinea arcalinea self-assigned this Jul 12, 2018

@arcalinea arcalinea requested review from bitcartel , str4d and Eirik0 Jul 12, 2018

@arcalinea arcalinea added this to Review Backlog in Zcashd Team Jul 12, 2018

@arcalinea arcalinea changed the title Add Sapling have/get sk crypter overrides Add Sapling have/get spendingkey crypter overrides Jul 12, 2018

@str4d str4d moved this from Review Backlog to In Review in Zcashd Team Jul 13, 2018

@str4d str4d added the wallet label Jul 13, 2018

@str4d
Copy link
Contributor

str4d left a comment

Also add Sapling support to CCryptoKeyStore::EncryptKeys

uint256 SaplingFullViewingKey::GetHash() const {
CDataStream ss(SER_NETWORK, PROTOCOL_VERSION);
ss << *this;
return Hash(ss.begin(), ss.end());

This comment has been minimized.

@str4d

str4d Jul 13, 2018

Contributor

We should use the fingerprint defined in ZIP 32, for consistency: BLAKE2b-256("ZcashSaplingFVFP", fvk)

CBLAKE2bWriter ss(SER_GETHASH, 0, ZCASH_SAPLING_FVFP_PERSONALIZATION);
ss << *this;
return ss.GetHash();
if(!DecryptSecret(vMasterKey, vchCryptedSecret, fvk.GetHash(), vchSecret))
return false;

if (vchSecret.size() != libzcash::SerializedSpendingKeySize)

This comment has been minimized.

@str4d

str4d Jul 13, 2018

Contributor

This is technically incorrect, because it's the Sprout spending key size. But in practice both Sprout and Sapling spending keys serialize to the same size. If another reviewer wants it changed, I'll support that.

This comment has been minimized.

@arcalinea

arcalinea Jul 17, 2018

Contributor

Will add const size_t SerializedSaplingSpendingKeySize = 32; and use that, for clarity

@@ -340,7 +357,7 @@ bool CCryptoKeyStore::AddSaplingSpendingKey(const libzcash::SaplingSpendingKey &
CKeyingMaterial vchSecret(ss.begin(), ss.end());
auto address = sk.default_address();

This comment has been minimized.

@str4d

str4d Jul 13, 2018

Contributor

This can be removed now.

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

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jul 13, 2018

☔️ The latest upstream changes (presumably #3390) made this pull request unmergeable. Please resolve the merge conflicts.

@arcalinea arcalinea force-pushed the arcalinea:3389_sapling_crypter branch from 15e1dea to 8b55ddf Jul 17, 2018

@arcalinea

This comment has been minimized.

Copy link
Contributor

arcalinea commented Jul 17, 2018

Addressed comments - review needed on Sapling key encryption in CCryptoKeyStore::EncryptKeys

@arcalinea arcalinea requested a review from str4d Jul 17, 2018

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

@mdr0id mdr0id moved this from Review Backlog to In Review in Zcashd Team Jul 18, 2018

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

@arcalinea arcalinea force-pushed the arcalinea:3389_sapling_crypter branch from e59702b to 07442c6 Jul 24, 2018

{
LOCK(cs_SpendingKeyStore);
if (!IsCrypted())
return CBasicKeyStore::GetSaplingSpendingKey(fvk, skOut);

This comment has been minimized.

@ebfull

ebfull Jul 25, 2018

Contributor

I'm confused about what this calls, is this not recursing? unconfused

This comment has been minimized.

@daira

daira Aug 3, 2018

Contributor

No, it's a superclass call.

READWRITE(ak);
READWRITE(nk);
READWRITE(ovk);
}

//! Get the 256-bit SHA256d hash of this full viewing key.

This comment has been minimized.

@str4d

str4d Aug 1, 2018

Contributor

Update this comment to match the implementation and ZIP 32.

READWRITE(ak);
READWRITE(nk);
READWRITE(ovk);
}

//! Get the 256-bit SHA256d hash of this full viewing key.
uint256 GetHash() const;

This comment has been minimized.

@str4d

str4d Aug 1, 2018

Contributor

Rename this to GetFingerprint() to match ZIP 32.

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Aug 1, 2018

I'm taking over this PR, as @arcalinea is away this week.

@str4d str4d assigned str4d and unassigned arcalinea Aug 1, 2018

@str4d str4d force-pushed the arcalinea:3389_sapling_crypter branch from 07442c6 to 37de067 Aug 1, 2018

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Aug 1, 2018

Rebased on master, addressed my comments, and added a few missing parts.

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 1, 2018

⌛️ Trying commit 37de067 with merge 068314e...

zkbot added a commit that referenced this pull request Aug 1, 2018

Auto merge of #3396 - arcalinea:3389_sapling_crypter, r=<try>
Add Sapling have/get spendingkey crypter overrides

Sapling crypter overrides for:
- `HaveSaplingSpendingKey()`
- `GetSaplingSpendingKey()`

Closes #3389
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 1, 2018

💔 Test failed - pr-try

str4d added some commits Aug 1, 2018

Add CWallet::AddCryptedSaplingSpendingKey() hook
To be filled in when we implement Sapling persistence to disk.

@str4d str4d force-pushed the arcalinea:3389_sapling_crypter branch from e65383b to 5175a7f Aug 3, 2018

@str4d str4d dismissed their stale review Aug 3, 2018

I've taken over the PR, so I can't review it.

@ebfull

ebfull approved these changes Aug 3, 2018

@daira
Copy link
Contributor

daira left a comment

Renaming changes requested. (Some of my comments are on individual commits.)

libzcash::SaplingSpendingKey& sk)
{
CKeyingMaterial vchSecret;
if(!DecryptSecret(vMasterKey, vchCryptedSecret, fvk.GetFingerprint(), vchSecret))

This comment has been minimized.

@daira

daira Aug 3, 2018

Contributor

nit: if (

if (vchSecret.size() != libzcash::SerializedSaplingSpendingKeySize)
return false;

CSecureDataStream ss(vchSecret, SER_NETWORK, PROTOCOL_VERSION);

This comment has been minimized.

@daira

daira Aug 3, 2018

Contributor

Is this actually dependent on the network protocol version? That doesn't sound right to me.

This comment has been minimized.

@str4d

str4d Aug 3, 2018

Contributor

It matches what we do in e.g. src/key_io.h. The distinction from upstream, as I understand it, is that anything that might get communicated spatially between nodes is technically part of the protocol, whereas anything that is only communicated temporally between a single node's past and future selves is SER_DISK, CLIENT_VERSION.

This comment has been minimized.

@daira

daira Aug 3, 2018

Contributor

I think this is questionable because, if we were to substantially change the network protocol, that would have to be versioned and upgraded quite differently from changing the external key formats. In any case, it's ok for this PR.

{
LOCK(cs_SpendingKeyStore);
if (!IsCrypted())
return CBasicKeyStore::GetSaplingSpendingKey(fvk, skOut);

This comment has been minimized.

@daira

daira Aug 3, 2018

Contributor

No, it's a superclass call.

@@ -19,6 +19,8 @@ const size_t SerializedPaymentAddressSize = 64;
const size_t SerializedViewingKeySize = 64;
const size_t SerializedSpendingKeySize = 32;

This comment has been minimized.

@daira

daira Aug 3, 2018

Contributor

These should be renamed to SerializedSprout....

@@ -173,7 +173,7 @@ bool CCryptoKeyStore::SetCrypted()
LOCK2(cs_KeyStore, cs_SpendingKeyStore);
if (fUseCrypto)
return true;
if (!(mapKeys.empty() && mapSpendingKeys.empty()))
if (!(mapKeys.empty() && mapSpendingKeys.empty() && mapSaplingSpendingKeys.empty()))

This comment has been minimized.

@daira

daira Aug 3, 2018

Contributor

mapSpendingKeys should be renamed to mapSproutSpendingKeys.

This comment has been minimized.

@str4d

str4d Aug 3, 2018

Contributor

I was hoping to avoid that large rename for now, but I'll go rip off the plaster 😂

This comment has been minimized.

@str4d

str4d Aug 3, 2018

Contributor

Oh, turns out it's not a large rename after all!

@@ -999,14 +999,16 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
bool RemoveViewingKey(const libzcash::SproutViewingKey &vk);
//! Adds a viewing key to the store, without saving it to disk (used by LoadWallet)
bool LoadViewingKey(const libzcash::SproutViewingKey &dest);

This comment has been minimized.

@daira

daira Aug 3, 2018

Contributor

These methods should be renamed to include Sprout.

str4d added some commits Aug 3, 2018

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Aug 3, 2018

Addressed @daira's comments.

@ebfull

This comment has been minimized.

Copy link
Contributor

ebfull commented Aug 3, 2018

re-ACK

@ebfull

ebfull approved these changes Aug 3, 2018

@daira daira dismissed their stale review Aug 3, 2018

addressed

@daira

This comment has been minimized.

Copy link
Contributor

daira commented Aug 3, 2018

And rename these too (sorry I didn't catch them before):

     //! Add a spending key to the store.
     virtual bool AddSpendingKey(const libzcash::SproutSpendingKey &sk) =0;
 
     //! Check whether a spending key corresponding to a given payment address is present in the store.
     virtual bool HaveSpendingKey(const libzcash::SproutPaymentAddress &address) const =0;
     virtual bool GetSpendingKey(const libzcash::SproutPaymentAddress &address, libzcash::SproutSpendingKey& skOut) const =0;
     virtual void GetPaymentAddresses(std::set<libzcash::SproutPaymentAddress> &setAddress) const =0;
Rename *SpendingKey -> *SproutSpendingKey
Also GetPaymentAddresses -> GetSproutPaymentAddresses
@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Aug 3, 2018

@daira Done (that was the big one).

@daira

daira approved these changes Aug 3, 2018

Copy link
Contributor

daira left a comment

ut(ACK+cov).

@ebfull

This comment has been minimized.

Copy link
Contributor

ebfull commented Aug 3, 2018

re-ACK

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Aug 3, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 3, 2018

📌 Commit 25d5e80 has been approved by str4d

zkbot added a commit that referenced this pull request Aug 3, 2018

Auto merge of #3396 - arcalinea:3389_sapling_crypter, r=str4d
Update CCryptoKeyStore with Sapling support

Sapling crypter overrides for various `CCryptoKeyStore` functions such as:
- `HaveSaplingSpendingKey()`
- `GetSaplingSpendingKey()`

Also includes some changes to prepare for diversified addresses and ZIP 32.

Closes #3389
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 3, 2018

⌛️ Testing commit 25d5e80 with merge aa32786...

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 3, 2018

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

@zkbot zkbot merged commit 25d5e80 into zcash:master Aug 3, 2018

1 check passed

homu Test successful
Details

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

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