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

Add Sapling support to z_importwallet and z_exportwallet #3491

Merged
merged 12 commits into from Sep 19, 2018

Conversation

@Eirik0
Copy link
Contributor

Eirik0 commented Aug 28, 2018

Includes code adapted from upstream PR bitcoin/bitcoin#8323

Closes #3218.

@Eirik0 Eirik0 self-assigned this Aug 28, 2018

@Eirik0 Eirik0 added this to Review Backlog in Zcashd Team Aug 28, 2018

@Eirik0 Eirik0 moved this from Review Backlog to In Review in Zcashd Team Aug 28, 2018

@str4d str4d referenced this pull request Aug 28, 2018

Closed

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

4 of 4 tasks complete

@Eirik0 Eirik0 requested review from str4d and daira Aug 28, 2018

@str4d str4d moved this from In Review to Review Backlog in Zcashd Team Aug 30, 2018

@bitcartel bitcartel moved this from Review Backlog to In Review in Zcashd Team Sep 4, 2018

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Sep 4, 2018

@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 Eirik0 force-pushed the Eirik0:3218-sapling-import-export-wallet branch from 0c7c283 to cf52125 Sep 5, 2018

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Sep 5, 2018

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
Copy link
Contributor

bitcartel left a comment

Works in testing. I do have some review comments.

public:
AddSpendingKeyToWallet(CWallet *wallet) : m_wallet(wallet) {}
AddSpendingKeyToWallet(CWallet *wallet, int64_t _nTime, bool _log) : m_wallet(wallet), nTime(_nTime), log(_log) {}

This comment has been minimized.

@bitcartel

bitcartel Sep 11, 2018

Contributor

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.

This comment has been minimized.

@Eirik0

Eirik0 Sep 11, 2018

Contributor

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.

This comment has been minimized.

@bitcartel

bitcartel Sep 11, 2018

Contributor

Ok, that makes sense. Looks good then.

@@ -30,41 +30,6 @@ bool EnsureWalletIsAvailable(bool avoidException);
UniValue dumpwallet_impl(const UniValue& params, bool fHelp, bool fDumpZKeys);
UniValue importwallet_impl(const UniValue& params, bool fHelp, bool fImportZKeys);

class GetSpendingKeyForPaymentAddress : public boost::static_visitor<libzcash::SpendingKey>

This comment has been minimized.

@bitcartel

bitcartel Sep 11, 2018

Contributor

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

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 11, 2018

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

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Sep 11, 2018

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 Eirik0 force-pushed the Eirik0:3218-sapling-import-export-wallet branch from cf52125 to bbf37c7 Sep 11, 2018

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Sep 11, 2018

Rebased and force pushed to address conflicts with #3492.

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

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Sep 12, 2018

See comments in #3218 regarding bitcoin/bitcoin#8323

@@ -2280,6 +2280,8 @@ UniValue getwalletinfo(const UniValue& params, bool fHelp)
" \"keypoolsize\": xxxx, (numeric) how many new keys are pre-generated\n"
" \"unlocked_until\": ttt, (numeric) the timestamp in seconds since epoch (midnight Jan 1 1970 GMT) that the wallet is unlocked for transfers, or 0 if the wallet is locked\n"
" \"paytxfee\": x.xxxx, (numeric) the transaction fee configuration, set in " + CURRENCY_UNIT + "/kB\n"
" \"seedfp\": \"uint256\", (string) the BLAKE2b-256 hash of the HD seed\n"
" \"masterkeyid\": \"<hash160>\", (string) the Hash160 of the hd master pubkey\n"

This comment has been minimized.

@Eirik0

Eirik0 Sep 12, 2018

Contributor

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

This comment has been minimized.

@str4d

str4d Sep 13, 2018

Contributor

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

@Eirik0 Eirik0 force-pushed the Eirik0:3218-sapling-import-export-wallet branch from 42d53ca to 51c1c3d Sep 12, 2018

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

if (pwalletMain->GetSaplingExtendedSpendingKey(addr, extsk)) {
auto ivk = extsk.expsk.full_viewing_key().in_viewing_key();
std::string strTime = EncodeDumpTime(pwalletMain->mapSaplingZKeyMetadata[ivk].nCreateTime);
file << strprintf("%s %s # zaddr=%s\n", EncodeSpendingKey(extsk), strTime, EncodePaymentAddress(addr));

This comment has been minimized.

@str4d

str4d Sep 13, 2018

Contributor

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.

This comment has been minimized.

@Eirik0

Eirik0 Sep 15, 2018

Contributor

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

This comment has been minimized.

@str4d

str4d Sep 17, 2018

Contributor

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

@@ -31,6 +31,67 @@ UniValue dumpwallet_impl(const UniValue& params, bool fHelp, bool fDumpZKeys);
UniValue importwallet_impl(const UniValue& params, bool fHelp, bool fImportZKeys);


class AddSpendingKeyToWallet : public boost::static_visitor<bool>

This comment has been minimized.

@str4d

str4d Sep 13, 2018

Contributor

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).

Added,
NotAdded,
KeyAdded,
KeyNotAdded,

This comment has been minimized.

@str4d

str4d Sep 13, 2018

Contributor

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

xsk = m_32h_cth.Derive(hdChain.saplingAccountCounter | ZIP32_HARDENED_KEY_LIMIT);
metadata.hdKeypath = "m/0'/0'/"+std::to_string(hdChain.saplingAccountCounter)+"'";

This comment has been minimized.

@str4d

str4d Sep 13, 2018

Contributor

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.

@@ -127,7 +127,10 @@ SaplingPaymentAddress CWallet::GenerateNewSaplingZKey()
libzcash::SaplingExtendedSpendingKey xsk;
do
{
// always derive hardened keys

This comment has been minimized.

@str4d

str4d Sep 13, 2018

Contributor

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

@@ -114,6 +113,7 @@ class CKeyMetadata
nVersion = CKeyMetadata::CURRENT_VERSION;
nCreateTime = 0;
hdKeypath.clear();
seedFp.SetNull();

This comment has been minimized.

@str4d

str4d Sep 13, 2018

Contributor

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

This comment has been minimized.

@Eirik0

Eirik0 Sep 14, 2018

Contributor

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...

{
HDSeed hdSeed;
pwalletMain->GetHDSeed(hdSeed);
file << strprintf("# HDSeed=%s fingerprint=%s", pwalletMain->GetHDChain().seedFp.GetHex(), hdSeed.Fingerprint().GetHex());

This comment has been minimized.

@str4d

str4d Sep 13, 2018

Contributor

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...

@@ -700,7 +718,7 @@ UniValue z_importkey(const UniValue& params, bool fHelp)

// Sapling support
auto addResult = boost::apply_visitor(
AddSpendingKeyToWallet(pwalletMain, Params().GetConsensus(), 1, false), spendingkey);
AddSpendingKeyToWallet(pwalletMain, Params().GetConsensus(), 1, boost::none, boost::none, false), spendingkey);

This comment has been minimized.

@str4d

str4d Sep 13, 2018

Contributor

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...

file << strprintf("%s %s # zaddr=%s\n", EncodeSpendingKey(extsk), strTime, EncodePaymentAddress(addr));
CKeyMetadata keyMeta = pwalletMain->mapSaplingZKeyMetadata[ivk];
std::string strTime = EncodeDumpTime(keyMeta.nCreateTime);
file << strprintf("%s %s %s %s # zaddr=%s\n", EncodeSpendingKey(extsk), strTime, keyMeta.hdKeypath, keyMeta.seedFp.GetHex(), EncodePaymentAddress(addr));

This comment has been minimized.

@str4d

str4d Sep 13, 2018

Contributor

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.

This comment has been minimized.

@str4d

str4d Sep 13, 2018

Contributor

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.

@@ -18,6 +18,9 @@ def run_test(self):
# node 0 should have the keys
dump_path0 = self.nodes[0].z_exportwallet('walletdump')
(t_keys0, sprout_keys0, sapling_keys0) = parse_wallet_file(dump_path0)

for sapling_key0 in sapling_keys0.splitlines():
assert_equal(4, len(sapling_key0.split(' #')[0].split()), "Should have 4 parameters before ' #'")

This comment has been minimized.

@str4d

str4d Sep 13, 2018

Contributor

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

@Eirik0 Eirik0 force-pushed the Eirik0:3218-sapling-import-export-wallet branch from 51c1c3d to b37dc4e Sep 15, 2018

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Sep 15, 2018

Addressed @str4d's comments

@ebfull

ebfull approved these changes Sep 18, 2018

@@ -83,7 +84,7 @@ const CWalletTx* CWallet::GetWalletTx(const uint256& hash) const
// Generate a new spending key and return its public payment address
libzcash::PaymentAddress CWallet::GenerateNewZKey()
{
AssertLockHeld(cs_wallet); // mapZKeyMetadata
AssertLockHeld(cs_wallet); // mapSproutZKeyMetadata
// TODO: Add Sapling support

This comment has been minimized.

@ebfull

ebfull Sep 18, 2018

Contributor

Is this comment now stale?

This comment has been minimized.

@str4d

str4d Sep 19, 2018

Contributor

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

@str4d

str4d approved these changes Sep 19, 2018

Copy link
Contributor

str4d left a comment

ut(ACK+cov)

// 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));

This comment has been minimized.

@str4d

str4d Sep 19, 2018

Contributor

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

@@ -4587,6 +4589,9 @@ SpendingKeyAddResult AddSpendingKeyToWallet::operator()(const libzcash::SaplingE
auto ivk = fvk.in_viewing_key();
auto addr = sk.DefaultAddress();
{
if (log){
LogPrint("zrpc", "Importing zaddr %s...\n", EncodePaymentAddress(addr));

This comment has been minimized.

@str4d

str4d Sep 19, 2018

Contributor

Ditto.

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

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Sep 19, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 19, 2018

📌 Commit b37dc4e has been approved by str4d

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

Auto merge of #3491 - Eirik0:3218-sapling-import-export-wallet, r=str4d
Add Sapling support to z_importwallet and z_exportwallet

Includes code adapted from upstream PR bitcoin/bitcoin#8323

Closes #3218.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 19, 2018

⌛️ Testing commit b37dc4e with merge 25c3f90...

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 19, 2018

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

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

1 check passed

homu Test successful
Details

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

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