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 keys in keystore, wallet #3273

Merged
merged 5 commits into from Jul 7, 2018

Conversation

@arcalinea
Copy link
Contributor

arcalinea commented May 17, 2018

  • Add/Have/Get SaplingSpendingKey

  • Add/Remove/Have/Get SaplingFullViewingKey

  • Have/Get SaplingIncomingViewingKey

  • SaplingSpendingKeyMap, SaplingFullViewingKeyMap, SaplingIncomingViewingKeyMap

  • GenerateNewSaplingZKey()

Not included: note decryptors, crypted keystore

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented May 18, 2018

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

@str4d str4d added the wallet label Jun 12, 2018

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

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

@arcalinea arcalinea force-pushed the arcalinea:sapling_keys branch from 5ed0775 to f6915ae Jun 14, 2018

@arcalinea arcalinea requested a review from str4d Jun 14, 2018

@str4d
Copy link
Contributor

str4d left a comment

Remove the two binaries that were added.

@@ -313,4 +346,3 @@ TEST(wallet_zkeys_tests, write_cryptedzkey_direct_to_db) {

ECC_Stop();
}

This comment has been minimized.

@str4d

str4d Jun 19, 2018

Contributor

Leave this in.

@@ -3881,4 +3906,3 @@ void CWallet::GetUnspentFilteredNotes(
}
}
}

This comment has been minimized.

@str4d

str4d Jun 19, 2018

Contributor

Leave this in.

@arcalinea arcalinea force-pushed the arcalinea:sapling_keys branch from 2b847e2 to 99333ec Jun 21, 2018

@arcalinea arcalinea changed the title WIP: Sapling keys in keystore, wallet Sapling keys in keystore, wallet Jun 22, 2018

@arcalinea arcalinea requested a review from bitcartel Jun 22, 2018

@bitcartel
Copy link
Contributor

bitcartel left a comment

Looks good. New methods and tests are independent of existing Sprout keystore. Verified that WriteSaplingZKey is never invoked, so database is not touched.

@arcalinea arcalinea requested review from bitcartel and str4d Jun 30, 2018

@arcalinea arcalinea force-pushed the arcalinea:sapling_keys branch 3 times, most recently from 7343289 to 891760c Jun 30, 2018

@str4d str4d modified the milestones: v1.1.2, v2.0.0 Jul 2, 2018

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

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Jul 3, 2018

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jul 3, 2018

⌛️ Trying commit 891760c with merge 5c98b97...

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

Auto merge of #3273 - arcalinea:sapling_keys, r=<try>
Sapling keys in keystore, wallet

-  Add/Have/Get SaplingSpendingKey
- Add/Remove/Have/Get SaplingFullViewingKey
- Have/Get SaplingIncomingViewingKey

- SaplingSpendingKeyMap, SaplingFullViewingKeyMap, SaplingIncomingViewingKeyMap

- GenerateNewSaplingZKey()

Not included: note decryptors, crypted keystore
@str4d
Copy link
Contributor

str4d left a comment

Looking pretty good overall!

// Add addr -> SaplingIncomingViewing to SaplingIncomingViewingKeyMap
auto ivk = fvk.in_viewing_key();
auto addr = sk.default_address();
mapSaplingIncomingViewingKeys[addr] = ivk;

This comment has been minimized.

@str4d

str4d Jul 3, 2018

Contributor

Reminder to self: when we implement ZIP 32, we need to set the address -> ivk mapping inside AddSaplingFullViewingKey(), not here (which we can't do in this PR because we can't get the default address from fvk).

bool CBasicKeyStore::RemoveViewingKey(const libzcash::SproutViewingKey &vk)
{
LOCK(cs_SpendingKeyStore);
mapViewingKeys.erase(vk.address());
return true;
}

bool CBasicKeyStore::RemoveSaplingFullViewingKey(const libzcash::SaplingFullViewingKey &fvk)

This comment has been minimized.

@str4d

str4d Jul 3, 2018

Contributor

Not sure we need this, but I'm not going to block the PR on removing it.

@@ -70,6 +91,13 @@ typedef std::map<libzcash::SproutPaymentAddress, libzcash::SproutSpendingKey> Sp
typedef std::map<libzcash::SproutPaymentAddress, libzcash::SproutViewingKey> ViewingKeyMap;
typedef std::map<libzcash::SproutPaymentAddress, ZCNoteDecryption> NoteDecryptorMap;

// Full viewing key has equivalent functionality to a transparent address
// When encrypting wallet, encrypt SaplingSpendingKeyMap, while leaving SaplingFullViewingKeyMap unencrypted
typedef std::map<libzcash::SaplingFullViewingKey, libzcash::SaplingSpendingKey> SaplingSpendingKeyMap;

This comment has been minimized.

@str4d

str4d Jul 3, 2018

Contributor

Reminder to self: when we implement ZIP 32, we'll add another map here from fvk -> ExtendedSpendingKey.

This comment has been minimized.

@arcalinea

arcalinea Jul 3, 2018

Contributor

added comment

// Generate a new Sapling spending key and return its public payment address
SaplingPaymentAddress CWallet::GenerateNewSaplingZKey()
{
AssertLockHeld(cs_wallet); // mapZKeyMetadata

This comment has been minimized.

@str4d

str4d Jul 3, 2018

Contributor

Nit: mapSaplingZKeyMetadata in comment.


// Check for collision, even though it is unlikely to ever occur
if (CCryptoKeyStore::HaveSaplingSpendingKey(fvk))
throw std::runtime_error("CWallet::GenerateNewSaplingZKey(): Collision detected");

This comment has been minimized.

@str4d

str4d Jul 3, 2018

Contributor

Add brackets and fix indentation.

@@ -141,7 +141,7 @@ class CWalletDB : public CDB

bool WriteViewingKey(const libzcash::SproutViewingKey &vk);
bool EraseViewingKey(const libzcash::SproutViewingKey &vk);

This comment has been minimized.

@str4d

str4d Jul 3, 2018

Contributor

Remove whitespace.

@@ -1,3 +1,4 @@

This comment has been minimized.

@str4d

str4d Jul 3, 2018

Contributor

Remove whitespace.

{
AssertLockHeld(cs_wallet); // mapSaplingZKeyMetadata

if (!CCryptoKeyStore::AddSaplingSpendingKey(sk)) {

This comment has been minimized.

@str4d

str4d Jul 3, 2018

Contributor

I suspect this call will fail, because this PR doesn't update CCryptoKeyStore with the new methods. Please add a test that calls GenerateNewSaplingZKey() and/or AddSaplingZKey() to ensure the pathway is exercised.

This comment has been minimized.

@str4d

str4d Jul 3, 2018

Contributor

If the ongoing try passes, I will downgrade this to non-blocking in the interest of unblocking downstream PRs.

This comment has been minimized.

@arcalinea

arcalinea Jul 3, 2018

Contributor

I updated CCryptoKeyStore to add sapling spending keys, and added a test calling GenerateNewSaplingZKey() and AddSaplingZKey(), going to run a try again

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jul 3, 2018

💔 Test failed - pr-try

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

@arcalinea

This comment has been minimized.

Copy link
Contributor

arcalinea commented Jul 3, 2018

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jul 3, 2018

⌛️ Trying commit 2777fe0 with merge f42d1c5...

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

Auto merge of #3273 - arcalinea:sapling_keys, r=<try>
Sapling keys in keystore, wallet

-  Add/Have/Get SaplingSpendingKey
- Add/Remove/Have/Get SaplingFullViewingKey
- Have/Get SaplingIncomingViewingKey

- SaplingSpendingKeyMap, SaplingFullViewingKeyMap, SaplingIncomingViewingKeyMap

- GenerateNewSaplingZKey()

Not included: note decryptors, crypted keystore
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jul 3, 2018

💔 Test failed - pr-try

@arcalinea arcalinea force-pushed the arcalinea:sapling_keys branch 4 times, most recently from 5fa6689 to a33eff6 Jul 5, 2018

@arcalinea arcalinea force-pushed the arcalinea:sapling_keys branch from a33eff6 to b8ec444 Jul 5, 2018

@arcalinea arcalinea force-pushed the arcalinea:sapling_keys branch from b8ec444 to 829042f Jul 5, 2018

@arcalinea arcalinea force-pushed the arcalinea:sapling_keys branch from 829042f to 2173767 Jul 5, 2018

@arcalinea

This comment has been minimized.

Copy link
Contributor

arcalinea commented Jul 6, 2018

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jul 6, 2018

⌛️ Trying commit 2173767 with merge 2b21793...

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

Auto merge of #3273 - arcalinea:sapling_keys, r=<try>
Sapling keys in keystore, wallet

-  Add/Have/Get SaplingSpendingKey
- Add/Remove/Have/Get SaplingFullViewingKey
- Have/Get SaplingIncomingViewingKey

- SaplingSpendingKeyMap, SaplingFullViewingKeyMap, SaplingIncomingViewingKeyMap

- GenerateNewSaplingZKey()

Not included: note decryptors, crypted keystore
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jul 6, 2018

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

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

@daira

daira approved these changes Jul 6, 2018

Copy link
Contributor

daira left a comment

ut(ACK+cov)

auto fvk = sk.full_viewing_key();
auto addr = sk.default_address();

// Check for collision, even though it is unlikely to ever occur

This comment has been minimized.

@daira

daira Jul 6, 2018

Contributor

Technically it is also possible for a collision to occur on ivk, even though fvk does not collide.

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

@str4d
Copy link
Contributor

str4d left a comment

ut(ACK+cov)

//! Sapling
virtual bool AddCryptedSaplingSpendingKey(const libzcash::SaplingFullViewingKey &fvk,
const std::vector<unsigned char> &vchCryptedSecret);
bool AddSaplingSpendingKey(const libzcash::SaplingSpendingKey &sk);

This comment has been minimized.

@str4d

str4d Jul 6, 2018

Contributor

Todo in subsequent PR: implement crypter overrides of HaveSaplingSpendingKey() etc.

CKeyingMaterial vchSecret(ss.begin(), ss.end());
auto address = sk.default_address();
auto fvk = sk.full_viewing_key();
if (!EncryptSecret(vMasterKey, vchSecret, address.GetHash(), vchCryptedSecret)) {

This comment has been minimized.

@str4d

str4d Jul 6, 2018

Contributor

We should use fvk.GetHash() here (as that is what is stored in the mapping with the ciphertext). Non-blocking, as we can fix this in the subsequent PR that fleshes out the crypter for Sapling.

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Jul 6, 2018

Nice work!

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jul 6, 2018

📌 Commit 2173767 has been approved by str4d

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jul 6, 2018

⌛️ Testing commit 2173767 with merge 579ad3b...

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

Auto merge of #3273 - arcalinea:sapling_keys, r=str4d
Sapling keys in keystore, wallet

-  Add/Have/Get SaplingSpendingKey
- Add/Remove/Have/Get SaplingFullViewingKey
- Have/Get SaplingIncomingViewingKey

- SaplingSpendingKeyMap, SaplingFullViewingKeyMap, SaplingIncomingViewingKeyMap

- GenerateNewSaplingZKey()

Not included: note decryptors, crypted keystore
@str4d

str4d approved these changes Jul 6, 2018

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jul 7, 2018

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

@zkbot zkbot merged commit 2173767 into zcash:master Jul 7, 2018

1 check passed

homu Test successful
Details

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

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