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 support for persisting wallet to disk #3517

Merged
merged 12 commits into from Oct 8, 2018

Conversation

@mdr0id
Copy link
Contributor

mdr0id commented Sep 13, 2018

Closes #3388. Closes #3061.

@mdr0id mdr0id requested review from bitcartel , str4d , Eirik0 and ebfull Sep 13, 2018

@mdr0id mdr0id self-assigned this Sep 13, 2018

Show resolved Hide resolved src/wallet/wallet.cpp Outdated

@mdr0id mdr0id changed the title Adding initial attempt at persisting wallet to disk [WIP] - Adding initial attempt at persisting wallet to disk Sep 14, 2018

@mdr0id mdr0id changed the title [WIP] - Adding initial attempt at persisting wallet to disk [WIP] - Initial attempt at persisting wallet to disk Sep 14, 2018

Show resolved Hide resolved src/wallet/wallet.h Outdated
Show resolved Hide resolved src/wallet/wallet.h
Show resolved Hide resolved src/wallet/walletdb.cpp Outdated
Show resolved Hide resolved src/wallet/walletdb.cpp Outdated
Show resolved Hide resolved src/wallet/walletdb.cpp Outdated
Show resolved Hide resolved src/wallet/wallet.cpp Outdated
@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Sep 19, 2018

Also we need to serialize every entry in mapSaplingIncomingViewingKeys; easiest to do this when we add to that map, which is when we add a new spending key. This ties into #3511 which @Eirik0 is working on, where IIRC he is adding functions to the wallet / keystore stack to enable adding to this map outside of when we add a new spending key.

@mdr0id mdr0id force-pushed the mdr0id:3388_persist_wallet branch from 04fbc19 to 498ec44 Sep 20, 2018

@mdr0id

This comment has been minimized.

Copy link
Contributor

mdr0id commented Sep 20, 2018

@str4d Addressed comments and awaiting another pass to ensure I'm heading in the right direction. I will take a peek at #3511, to see if it fills in the gap of where I'm stuck.

@str4d str4d modified the milestones: v2.0.2, v2.0.1 Sep 21, 2018

@str4d str4d added this to In Progress in Zcashd Team Sep 21, 2018

@mdr0id mdr0id force-pushed the mdr0id:3388_persist_wallet branch from 7dde5bb to d300ac9 Sep 25, 2018

@mdr0id

This comment has been minimized.

Copy link
Contributor

mdr0id commented Sep 25, 2018

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 25, 2018

⌛️ Trying commit d300ac9 with merge c2e3676...

zkbot added a commit that referenced this pull request Sep 25, 2018

Auto merge of #3517 - mdr0id:3388_persist_wallet, r=<try>
[WIP] - Initial attempt at persisting wallet to disk

For #3388

Wanted to put this up to get some guidance. I got some of the initial methods in, but I am fumbling through the implementation details.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 26, 2018

💔 Test failed - pr-try

@mdr0id mdr0id changed the title [WIP] - Initial attempt at persisting wallet to disk [WIP] - Sapling support for persisting wallet to disk Sep 27, 2018

@mdr0id

This comment has been minimized.

Copy link
Contributor

mdr0id commented Sep 27, 2018

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 27, 2018

⌛️ Trying commit 42e583b with merge 7ad8a04...

zkbot added a commit that referenced this pull request Sep 27, 2018

Auto merge of #3517 - mdr0id:3388_persist_wallet, r=<try>
[WIP] - Sapling support for persisting wallet to disk

For #3388

Wanted to put this up to get some guidance. I got some of the initial methods in, but I am fumbling through the implementation details.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 27, 2018

💔 Test failed - pr-try

@Eirik0 Eirik0 referenced this pull request Sep 27, 2018

Open

Look for TODOs and FIXME in code. #3546

20 of 25 tasks complete
@mdr0id

This comment has been minimized.

Copy link
Contributor

mdr0id commented Sep 27, 2018

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 27, 2018

⌛️ Trying commit 4d4cbc3 with merge 0da09a1...

str4d and others added some commits Oct 5, 2018

Store ExtFVK with encrypted Sapling spending key instead of FVK
This ensures that even when the wallet is encrypted, we can derive the default
Sapling payment address for our spending keys.
Serialize Sapling data in CWalletTx
If 2.0.0 nodes upgrade to 2.0.1 after Sapling has activated, the v4 Sapling
transactions in their wallet will be treated as corrupt, and a rescan will be
triggered which will overwrite the old-format transactions with the new
Sapling-aware format.

@str4d str4d force-pushed the mdr0id:3388_persist_wallet branch from b9e55e9 to 9ce6f84 Oct 5, 2018

@str4d str4d dismissed their stale review Oct 5, 2018

I refactored the branch and addressed my comments. I can't review this PR now.

@Eirik0

Eirik0 approved these changes Oct 5, 2018

str4d and others added some commits Oct 5, 2018

Persist Sapling payment address to IVK map
This ensures we remember any diversified addresses manually generated
outside the wallet.
Fix deadlock from calling CWallet::AddSaplingIncomingViewingKey inste…
…ad of CBasicKeyStore::AddSaplingIncomingViewingKey
Ignore decoding errors during -zapwallettxes
The undecoded wallet transaction is logged before proceeding, so later
recovery of metadata might be possible. But the fact that the user is
using -zapwallettxes is a clear indicator that they want
transactions removed from their wallet, so this is the priority.
Show resolved Hide resolved src/keystore.cpp
@ebfull

ebfull approved these changes Oct 7, 2018

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Oct 7, 2018

ACK

@bitcartel
Copy link
Contributor

bitcartel left a comment

PR looks good.

Related qestion:

In CWallet::AddSaplingZKey:

return CWalletDB(strWalletFile).WriteSaplingZKey(ivk, sk, mapSaplingZKeyMetadata[ivk]);

Should there be a check for mapSaplingZKeyMetadata[ivk] actually existing? It doesn't seem as though it gets set in the code path upon entering AddSaplingZKey().

EDIT: In a pairing with @daira this has been resolved as the operator [] on std::map will use the default constructor for CKeyMetadata, but there is a question of whether this is intended behaviour i.e. a default CKeyMetadata is stored with the sapling zkey.

{
AssertLockHeld(cs_wallet); // mapSaplingZKeyMetadata
mapSaplingZKeyMetadata[ivk] = meta;
return true;

This comment has been minimized.

@bitcartel

bitcartel Oct 7, 2018

Contributor

Comment: We never return false, so this could be a void function.

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Oct 8, 2018

reACK

@daira

daira approved these changes Oct 8, 2018

Copy link
Contributor

daira left a comment

ut(ACK+cov). Changes do not block, just nice-to-have.

# Verify Sapling address is persisted in wallet (even when Sapling is not yet active)
sapling_addr = self.nodes[0].z_getnewaddress('sapling')

# Make sure the node has the addresss

This comment has been minimized.

@daira

daira Oct 8, 2018

Contributor

s/addresss/address/ (does not block)

Show resolved Hide resolved src/wallet/crypter.cpp
@@ -288,7 +331,6 @@ TEST(wallet_zkeys_tests, WriteViewingKeyDirectToDB) {
/**
* This test covers methods on CWalletDB to load/save crypted z keys.
*/
/* TODO: Uncomment during PR for #3388
TEST(wallet_zkeys_tests, write_cryptedzkey_direct_to_db) {

This comment has been minimized.

@daira

daira Oct 8, 2018

Contributor

Naming should be CamelCase.

This comment has been minimized.

@str4d

str4d Oct 8, 2018

Contributor

Opened #3575.

// TODO: Persist to disk
if (!IsCrypted()) {
auto ivk = sk.expsk.full_viewing_key().in_viewing_key();
return CWalletDB(strWalletFile).WriteSaplingZKey(ivk, sk, mapSaplingZKeyMetadata[ivk]);

This comment has been minimized.

@daira

daira Oct 8, 2018

Contributor

mapSaplingZKeyMetadata has type std::map<libzcash::SaplingPaymentAddress, CKeyMetadata>, and if the key doesn't exist in the map, std::map's operator[] uses the default constructor of the target type to insert a new instance. In this case it will be a CKeyMetadata instance with the current version and an "unknown" time.

I'm unsure whether that was intended or whether this is a case that shouldn't ever happen.

This comment has been minimized.

@str4d

str4d Oct 8, 2018

Contributor

This is, I believe, expected behaviour:

  • When calling AddSaplingZKey from GenerateNewSaplingZKey, we set mapSaplingZKeyMetadata there first.
  • When calling AddSaplingZKey from AddSpendingKeyToWallet, we set mapSaplingZKeyMetadata appropriately afterwards.
Show resolved Hide resolved src/wallet/walletdb.cpp
Show resolved Hide resolved src/wallet/walletdb.cpp
Show resolved Hide resolved src/wallet/walletdb.cpp
@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Oct 8, 2018

Opened new issues for the remaining non-blocking comments.

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 8, 2018

📌 Commit 01282e4 has been approved by str4d

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 8, 2018

⌛️ Testing commit 01282e4 with merge c2bb0ec...

zkbot added a commit that referenced this pull request Oct 8, 2018

Auto merge of #3517 - mdr0id:3388_persist_wallet, r=str4d
Sapling support for persisting wallet to disk

Closes #3388. Closes #3061.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 8, 2018

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

@zkbot zkbot merged commit 01282e4 into zcash:master Oct 8, 2018

1 check passed

homu Test successful
Details

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

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