Add Sapling support to z_importwallet and z_exportwallet #3491

Eirik0 · 2018-08-28T22:49:10Z

Includes code adapted from upstream PR bitcoin/bitcoin#8323

Closes #3218.

bitcartel · 2018-09-04T22:13:07Z

@Eirik0 Can you rebase on master? I rebased but building results in errors, such as:

wallet/rpcdump.cpp:33:7: error: redefinition of ‘class GetSpendingKeyForPaymentAddress’
 class GetSpendingKeyForPaymentAddress : public boost::static_visitor<libzcash::SpendingKey>
       ^
In file included from wallet/rpcdump.cpp:15:0:
wallet/wallet.h:1334:7: error: previous definition of ‘class GetSpendingKeyForPaymentAddress’
 class GetSpendingKeyForPaymentAddress : public boost::static_visitor<boost::optional<libzcash::SpendingKey>>
       ^

Eirik0 · 2018-09-05T17:05:07Z

Pushed a commit to fix the build errors. The issue was that the class GetSpendingKeyForPaymentAddress had moved in my PR and had moved and been updated in another.

bitcartel

Works in testing. I do have some review comments.

bitcartel · 2018-09-11T04:17:20Z

src/wallet/rpcdump.cpp

Why do we have a log parameter? If just for developer debugging, we should consider removing the parameter and the log messages associated with it.

It is this way because z_importwallet logged each address before it imported it where as z_importkey didn't, and I wanted to be able to reuse the visitor we created for z_importkey.

I could change AddSpendingKeyToWallet to return a pair of the result and an optional address and remove the logging from inside the visitor which might be more correct, but feels clunkier. I'd also be happy to remove the logging, but I can see use cases where that might be helpful. Let me know what you think or if you have a better idea of how to do this.

Ok, that makes sense. Looks good then.

bitcartel · 2018-09-11T04:22:01Z

src/wallet/rpcdump.cpp

You might be able to rebase and merge this with earlier commits "Add sapling spending keys to z_exportwallet" and/or "Move visitor class" to clean up the function GetSpendingKeyForPaymentAddress().

zkbot · 2018-09-11T13:50:30Z

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

bitcartel · 2018-09-11T16:44:14Z

This PR also needs to import and export SaplingExtendedSpendingKey.

Currently, z_importkey and z_exportkey work with a SaplingSpendingKey. If the RPC interface will support both SaplingSpendingKey and SaplingExtendedSpendingKey, then import and export wallet should also support both.

Eirik0 · 2018-09-11T22:33:01Z

Rebased and force pushed to address conflicts with #3492.

Eirik0 · 2018-09-12T21:05:58Z

See comments in #3218 regarding bitcoin/bitcoin#8323

Eirik0 · 2018-09-12T22:54:16Z

src/wallet/rpcwallet.cpp

masterkeyid should not be here and is removed in a later commit

If you end up rebasing the PR, please squash the later commit into this one.

str4d · 2018-09-13T16:53:20Z

src/wallet/rpcdump.cpp

For diversified addresses, this will print the same spending key multiple times, each with a different zaddr. That's probably fine, though it might be a little confusing to users who open the file.

Would it be beneficial to include the diversifier along with the address?

The address already includes the diversifier, so I don't think so.

str4d · 2018-09-13T16:55:33Z

src/wallet/rpcdump.cpp

If you're moving it for accessibility, please move it to the bottom of wallet.h / wallet.cpp with the other generalisations (see there for how to structure it).

str4d · 2018-09-13T17:03:02Z

src/wallet/rpcdump.cpp

If you rebase this PR to move AddSpendingKeyToWallet into wallet.h, please refactor this into the previous commit (that added SpendingKeyAddResult).

str4d · 2018-09-13T17:09:40Z

src/wallet/wallet.cpp

This needs to be modified to:

metadata.hdKeypath = "m/32'/" + std::to_string(Params().BIP44CoinType()) + "'/" + std::to_string(hdChain.saplingAccountCounter) + "'";

You could make Params().BIP44CoinType() a variable after auto m so you can reuse it.

str4d · 2018-09-13T17:13:15Z

src/wallet/wallet.cpp

Remove this comment (wasn't in the commit, and doesn't apply to us in the way it was intended upstream).

str4d · 2018-09-13T17:15:57Z

src/wallet/walletdb.h

This commit lost its authorship details, but it's pretty trivial, so meh.

For this one it merged in just fine, but was not correct for zcash. I did a git cherry-pick ... -n, and a git commit -e. Not sure what happened...

str4d · 2018-09-13T17:18:59Z

src/wallet/rpcdump.cpp

I'd prefer to put this at the top of the file, as part of the header (i.e. after the * Best block... line), since this seed is what we would use for deterministic t-addrs if we bring that in, and as-is it will end up "stranded" in the middle-to-end of the backup file if the user's wallet contains many t-addrs. Sorry that it makes your test harder to write...

str4d · 2018-09-13T17:21:58Z

src/wallet/rpcdump.cpp

You should be able to set the four new parameters as defaults in the operator() declaration, so that you don't have to specify them in these other locations. Then again, we only use this visitor in these two locations, so maybe explicit is better...

str4d · 2018-09-13T17:25:34Z

src/wallet/rpcdump.cpp

What will this do if hdKeypath or seedFp are empty/null (as for a key imported via z_importkey)? If empty I assume we just get additional whitespace in the file, but seedFp.IsNull() is when it is the all-zeroes array, which I think would get printed out here. We might need to be more verbose here in order to only add these two if they are meaningfully-filled.

In fact, I think the import logic will break with this as-is. hdKeypath will be empty, and thus nothing prints for it, but seedFp will print all-zeroes hex. Then on importing, seedFp will be parsed as the third item in the list, and thus be incorrectly interpreted as hdKeypath.

str4d · 2018-09-13T17:29:52Z

qa/rpc-tests/wallet_import_export.py

We should test importing a key with z_importkey before exporting with z_exportwallet; those keys should only have two parameters.

Zcash: modified for zip32

…Null() Zcash: modified for zip32

Eirik0 · 2018-09-15T00:39:17Z

Addressed @str4d's comments

ebfull · 2018-09-18T22:30:18Z

src/wallet/wallet.cpp

 {
-    AssertLockHeld(cs_wallet); // mapZKeyMetadata
+    AssertLockHeld(cs_wallet); // mapSproutZKeyMetadata
    // TODO: Add Sapling support


Is this comment now stale?

Yes it is, as we now use separate functions to generate Sprout and Sapling keys.

str4d

ut(ACK+cov)

str4d · 2018-09-19T12:54:16Z

src/wallet/wallet.cpp

-    // Don't throw error in case a key is already there
+   auto addr = sk.address();
+    if (log){
+        LogPrint("zrpc", "Importing zaddr %s...\n", EncodePaymentAddress(addr));


Nit: this log line now always gets printed, instead of only when importing.

str4d · 2018-09-19T12:55:09Z

src/wallet/wallet.cpp

    auto addr = sk.DefaultAddress();
    {
+        if (log){
+            LogPrint("zrpc", "Importing zaddr %s...\n", EncodePaymentAddress(addr));


str4d · 2018-09-19T19:14:35Z

@zkbot r+

zkbot · 2018-09-19T19:14:35Z

📌 Commit b37dc4e has been approved by str4d

Add Sapling support to z_importwallet and z_exportwallet Includes code adapted from upstream PR bitcoin/bitcoin#8323 Closes #3218.

zkbot · 2018-09-19T19:15:52Z

⌛ Testing commit b37dc4e with merge 25c3f90...

zkbot · 2018-09-19T21:21:48Z

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

Eirik0 self-assigned this Aug 28, 2018

str4d mentioned this pull request Aug 28, 2018

Extend RPC to be able to import and export Sapling keys #3218

Closed

4 tasks

Eirik0 requested review from daira and str4d August 28, 2018 22:51

Eirik0 added A-wallet Area: Wallet A-rpc-interface Area: RPC interface NU1-sapling Network upgrade: Sapling-specific tasks labels Aug 28, 2018

Eirik0 force-pushed the 3218-sapling-import-export-wallet branch from 0c7c283 to cf52125 Compare September 5, 2018 17:03

Eirik0 mentioned this pull request Sep 10, 2018

Use ZIP 32 for all Sapling spending keys #3492

Merged

bitcartel reviewed Sep 11, 2018

View reviewed changes

Eirik0 force-pushed the 3218-sapling-import-export-wallet branch from cf52125 to bbf37c7 Compare September 11, 2018 22:31

Eirik0 force-pushed the 3218-sapling-import-export-wallet branch 2 times, most recently from 9dd7c15 to 3716a1c Compare September 12, 2018 11:04

Eirik0 commented Sep 12, 2018

View reviewed changes

Eirik0 force-pushed the 3218-sapling-import-export-wallet branch from 42d53ca to 51c1c3d Compare September 12, 2018 22:57

str4d changed the title ~~3218 sapling import export wallet~~ Add Sapling support to z_importwallet and z_exportwallet Sep 13, 2018

str4d requested changes Sep 13, 2018

View reviewed changes

Eirik Ogilvie-Wigley and others added 7 commits September 14, 2018 15:07

Rename map to include sprout

d6b31d5

Add sapling spending keys to z_exportwallet

5e360fb

Rename AddZKey to include sprout

a0783bb

Move AddSpendingKeyToWallet

fcab001

Return more information when adding a spending key

0f03de5

Add sapling support to z_importwallet

9bcf90e

[Wallet] extend CKeyMetadata with HD keypath

b6d1af8

Zcash: modified for zip32

jonasschnelli and others added 5 commits September 14, 2018 16:59

[Wallet] print hd masterkeyid in getwalletinfo

82e7123

Zcash: modified for zip32

[Wallet] ensure CKeyMetadata.hdMasterKeyID will be cleared during Set…

002753a

…Null() Zcash: modified for zip32

Export comment on HDSeed and fingerprint with wallet

ae807af

Export zip32 metadata with sapling keys

2fe3956

Don't export empty zip32 metadata

b37dc4e

Eirik0 force-pushed the 3218-sapling-import-export-wallet branch from 51c1c3d to b37dc4e Compare September 15, 2018 00:36

ebfull approved these changes Sep 18, 2018

View reviewed changes

str4d approved these changes Sep 19, 2018

View reviewed changes

str4d added this to the v2.0.1 milestone Sep 19, 2018

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

Auto merge of #3491 - Eirik0:3218-sapling-import-export-wallet, r=str4d

25c3f90

Add Sapling support to z_importwallet and z_exportwallet Includes code adapted from upstream PR bitcoin/bitcoin#8323 Closes #3218.

zkbot merged commit b37dc4e into zcash:master Sep 19, 2018

Add Sapling support to z_importwallet and z_exportwallet #3491

Add Sapling support to z_importwallet and z_exportwallet #3491

Uh oh!

Conversation

Eirik0 commented Aug 28, 2018 • edited by str4d Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bitcartel commented Sep 4, 2018

Uh oh!

Eirik0 commented Sep 5, 2018

Uh oh!

bitcartel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zkbot commented Sep 11, 2018

Uh oh!

bitcartel commented Sep 11, 2018

Uh oh!

Eirik0 commented Sep 11, 2018

Uh oh!

Eirik0 commented Sep 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Eirik0 commented Sep 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

str4d left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

str4d commented Sep 19, 2018

Uh oh!

zkbot commented Sep 19, 2018

Uh oh!

zkbot commented Sep 19, 2018

Eirik0 commented Aug 28, 2018 •

edited by str4d

Loading