Skip to content
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

Use ZIP 32 for all Sapling spending keys #3492

Merged
merged 12 commits into from Sep 11, 2018
Merged

Use ZIP 32 for all Sapling spending keys #3492

merged 12 commits into from Sep 11, 2018

Conversation

str4d
Copy link
Contributor

@str4d str4d commented Aug 28, 2018

The wallet now only stores Sapling extended spending keys, and thus can
only be used with keys generated from an HDSeed via ZIP 32. This means
that all Sapling keys and addresses generated by users can be recovered
as long as they have a backup that includes the seed.

Depends on zcash/librustzcash#29

Closes #3380.

@str4d str4d added A-wallet Area: Wallet A-rpc-interface Area: RPC interface M-potential-loss-of-funds This issue risks a potential loss of funds for a user. labels Aug 28, 2018
@str4d str4d added this to the v2.0.1 milestone Aug 28, 2018
@str4d str4d added this to In Review in Arborist Team Aug 28, 2018
@str4d str4d requested review from daira and Eirik0 August 30, 2018 08:54
@str4d
Copy link
Contributor Author

str4d commented Aug 31, 2018

@zkbot try

@zkbot
Copy link
Contributor

zkbot commented Aug 31, 2018

⌛ Trying commit 0baff23c42cfe9994685300441ae3b1c1ef23f90 with merge 2ec91ec28cb0a152261a1f8cffc49ce3dd54b971...

@zkbot
Copy link
Contributor

zkbot commented Aug 31, 2018

💔 Test failed - pr-try

@str4d
Copy link
Contributor Author

str4d commented Aug 31, 2018

Some tests failed because commenting out HDSeed persistence breaks invariants inside CCryptoKeyStore. This only affects wallet encryption which is unsupported, so I've temporarily disabled those tests. They will be re-enabled during the PR for #3388.

@str4d
Copy link
Contributor Author

str4d commented Aug 31, 2018

@zkbot try

@zkbot
Copy link
Contributor

zkbot commented Aug 31, 2018

⌛ Trying commit 6a9a3f665e5522fcf35a99d2d7b5c8a39ec11c6b with merge dd3aad3ab1c866c2d2cbcf9c1ec116335cce8141...

@zkbot
Copy link
Contributor

zkbot commented Aug 31, 2018

💔 Test failed - pr-try

@str4d
Copy link
Contributor Author

str4d commented Aug 31, 2018

Whoops, I forgot that an assertion failure doesn't count as an expected failure in CI, so I have to actually comment out the Google Test rather than disabling it.

@zkbot try

@zkbot
Copy link
Contributor

zkbot commented Aug 31, 2018

⌛ Trying commit 10b8a40 with merge 20e0ba0...

zkbot added a commit that referenced this pull request Aug 31, 2018
Use ZIP 32 for all Sapling spending keys

The wallet now only stores Sapling extended spending keys, and thus can
only be used with keys generated from an HDSeed via ZIP 32. This means
that all Sapling keys and addresses generated by users can be recovered
as long as they have a backup that includes the seed.

Depends on zcash/librustzcash#29

Closes #3380.
@zkbot
Copy link
Contributor

zkbot commented Aug 31, 2018

💔 Test failed - pr-try

@str4d
Copy link
Contributor Author

str4d commented Aug 31, 2018

Transient disk-space failure on one of the latent workers (probably due to running several builds in a row without it shutting down in between). All other supported builders passed 😃

Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ut(ACK+cov) modulo the comment about changing HD seeds and encryption.

uint256S("72a196f93e8abc0935280ea2a96fa57d6024c9913e0f9fb3af96775bb77cc177"));
EXPECT_THAT(
m.ToXFVK().DefaultAddress().d,
testing::ElementsAreArray({ 0xd8, 0x62, 0x1b, 0x98, 0x1c, 0xf3, 0x00, 0xe9, 0xd4, 0xcc, 0x89 }));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice that the default address now uses a diversifier generated with FF1-AES256. I've updated the protocol spec to make a note of this.


#include <zcash/zip32.h>

// From https://github.com/zcash-hackworks/zcash-test-vectors/blob/master/sapling_zip32.py
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment that uint256S takes its input in big-endian byte order whereas Sapling consistently uses little-endian encoding; and that is why those inputs appear byte-reversed.

EXPECT_THAT(
m_1_2hv_3.DefaultAddress().d,
testing::ElementsAreArray({ 0x03, 0x0f, 0xfb, 0x26, 0x3a, 0x93, 0x9e, 0x23, 0x0e, 0x96, 0xdd }));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have checked all of these test vectors against the sapling_zip32.py output.

#include <sodium.h>

const unsigned char ZCASH_HD_SEED_FP_PERSONAL[crypto_generichash_blake2b_PERSONALBYTES] =
{'Z', 'c', 'a', 's', 'h', '_', 'H', 'D', '_', 'S', 'e', 'e', 'd', '_', 'F', 'P'};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't in ZIP 32; should it be added?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

)) {
CDataStream ss_i(i_bytes, SER_NETWORK, PROTOCOL_VERSION);
SaplingExtendedFullViewingKey xfvk_i;
ss_i >> xfvk_i;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this do if the encoding isn't valid? (It should be valid if librustzcash_zip32_xfvk_derive returns true; I'm just looking at defence in depth.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will succeed if the returned encoding is at least 169 bytes long, as the deserialization just reads bytes into internal structures.

boost::optional<std::pair<libzcash::diversifier_t, libzcash::SaplingPaymentAddress>>
Address(libzcash::diversifier_t j) const;

libzcash::SaplingPaymentAddress DefaultAddress() const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why doesn't this have an operator== method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it isn't needed yet (we aren't using it as the key in a map).

{
CKeyingMaterial vchSecret;

// Use seed's fingerprint as IV
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that going to interact badly with changing the seed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, because the seed's fingerprint is being stored alongside the encrypted seed.

ASSERT_TRUE(keyStore.SetHDSeed(seed2));
EXPECT_TRUE(keyStore.HaveHDSeed());
ASSERT_TRUE(keyStore.GetHDSeed(seedOut));
EXPECT_EQ(seed2, seedOut);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't test that locking/unlocking works after changing the seed.

@@ -158,7 +158,7 @@ class CMainParams : public CChainParams {
bech32HRPs[SAPLING_PAYMENT_ADDRESS] = "zs";
bech32HRPs[SAPLING_FULL_VIEWING_KEY] = "zviews";
bech32HRPs[SAPLING_INCOMING_VIEWING_KEY] = "zivks";
bech32HRPs[SAPLING_SPENDING_KEY] = "secret-spending-key-main";
bech32HRPs[SAPLING_EXTENDED_SPEND_KEY] = "secret-extended-key-main";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bye bye spending keys, we won't forget you 😢

@@ -4450,8 +4471,8 @@ bool PaymentAddressBelongsToWallet::operator()(const libzcash::SaplingPaymentAdd
{
libzcash::SaplingIncomingViewingKey ivk;

// If we have a SaplingSpendingKey or SaplingExpandedSpendingKey in the
// wallet, then we will also have the corresponding SaplingFullViewingKey.
// If we have a SaplingExtendedSpendingKey in thewallet, then we will
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/thewallet/the wallet/

@zkbot
Copy link
Contributor

zkbot commented Sep 3, 2018

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

Sapling keys generated from the seed are not yet persisted, so we don't
want to persist the seed or chain state either, otherwise the wallet
could end up in an inconsistent state.

Some tests are temporarily disabled because commenting out HDSeed
persistence breaks invariants inside CCryptoKeyStore.

Revert this commit during the PR for zcash#3388.
We can maybe relax this restriction later once we have worked out the UX
implications.
@str4d
Copy link
Contributor Author

str4d commented Sep 3, 2018

Rebased on master to fix merge conflicts, and address @daira's comments.

@str4d
Copy link
Contributor Author

str4d commented Sep 3, 2018

@zkbot try

@zkbot
Copy link
Contributor

zkbot commented Sep 3, 2018

⌛ Trying commit b33a7ec with merge 03fca7c...

zkbot added a commit that referenced this pull request Sep 3, 2018
Use ZIP 32 for all Sapling spending keys

The wallet now only stores Sapling extended spending keys, and thus can
only be used with keys generated from an HDSeed via ZIP 32. This means
that all Sapling keys and addresses generated by users can be recovered
as long as they have a backup that includes the seed.

Depends on zcash/librustzcash#29

Closes #3380.
@zkbot
Copy link
Contributor

zkbot commented Sep 3, 2018

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

return CBasicKeyStore::SetHDSeed(seed);
}

if (IsLocked())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the surface calling IsLocked() after having called Lock(...) seems counterintuitive. Drilling into the IsLocked() method, it is still a bit confusing as much of what it does seems to overlap with what we do above.

why not say:

LOCK(cs_SpendingKeyStore);
if (IsCrypted() && vMasterKey.empty()) {
	return false;
} else if (!IsCrypted()) {
	return CBasicKeyStore::SetHDSeed(seed);
}

std::vector<unsigned char> vchCryptedSecret;
// ...

I am not sure if this is less confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are two semantically-different locking operations. The LOCK call is locking the mutex that protects the shielded keys. IsLocked() is checking whether the encrypted wallet is locked (and thus can't encrypt or decrypt anything) or not. It happens to lock on the mutex that protects the transparent keys; we probably didn't need to use separate mutexes, but that's an unrelated issue.

@@ -261,6 +287,67 @@ bool CCryptoKeyStore::Unlock(const CKeyingMaterial& vMasterKeyIn)
return true;
}

bool CCryptoKeyStore::SetHDSeed(const HDSeed& seed)
{
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: I see this double { { from time to time before calling lock. Why do we do this. In this case does this cause us to unlock before having returned false rather than after?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The style is inherited from upstream. The { scopes the lock to a block of code. So if there were code before or after which didn't require the lock, it could be added easily and it would also be easy to see which section of code in the function required a lock.

if (cryptedHDSeed.second.empty())
return false;

return DecryptHDSeed(vMasterKey, cryptedHDSeed.second, cryptedHDSeed.first, seedOut);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: we pass in second first and first second. It makes me wonder why cryptedHDSeed is a pair rather than two fields, or a class.

// Sapling addresses can't have been used in transactions prior to activation.
m_wallet->mapSaplingZKeyMetadata[ivk].nCreateTime = std::max(
1, // In case a code fork sets Sapling to always be active
params.vUpgrades[Consensus::UPGRADE_SAPLING].nActivationHeight);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we set the create time to be the activation height?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Pushed a fix as discussed in our pairing.

Copy link
Contributor

@Eirik0 Eirik0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@str4d
Copy link
Contributor Author

str4d commented Sep 11, 2018

@zkbot r+

@zkbot
Copy link
Contributor

zkbot commented Sep 11, 2018

📌 Commit 9454932 has been approved by str4d

@zkbot
Copy link
Contributor

zkbot commented Sep 11, 2018

⌛ Testing commit 9454932 with merge 88f52f0...

zkbot added a commit that referenced this pull request Sep 11, 2018
Use ZIP 32 for all Sapling spending keys

The wallet now only stores Sapling extended spending keys, and thus can
only be used with keys generated from an HDSeed via ZIP 32. This means
that all Sapling keys and addresses generated by users can be recovered
as long as they have a backup that includes the seed.

Depends on zcash/librustzcash#29

Closes #3380.
@zkbot
Copy link
Contributor

zkbot commented Sep 11, 2018

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

@zkbot zkbot merged commit 9454932 into zcash:master Sep 11, 2018
Arborist Team automation moved this from In Review to Released (Merged in Master) Sep 11, 2018
@mdr0id
Copy link
Contributor

mdr0id commented Sep 11, 2018

Late utACK from finishing prior review.

Copy link
Contributor

@bitcartel bitcartel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR commented out two QA tests.
wallet_nullifiers.py
fundrawtransaction.py
Is this because the tests will fail with this PR? Are they due to be fixed in a new PR / issue?

@str4d str4d deleted the zip32 branch September 11, 2018 16:16
@str4d
Copy link
Contributor Author

str4d commented Sep 11, 2018

@bitcartel yes; see the commit message for b7f9a7a (which is the commit that will be git reverted as part of #3388).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rpc-interface Area: RPC interface A-wallet Area: Wallet M-potential-loss-of-funds This issue risks a potential loss of funds for a user.
Projects
Arborist Team
  
Released (Merged in Master)
Development

Successfully merging this pull request may close these issues.

None yet

7 participants