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_importkey, z_exportkey #3392

Merged
merged 4 commits into from Jul 30, 2018

Conversation

@arcalinea
Copy link
Contributor

arcalinea commented Jul 12, 2018

Add Sapling support to z_importkey and z_exportkey

@arcalinea arcalinea added this to the v2.0.0 milestone Jul 12, 2018

@arcalinea arcalinea self-assigned this Jul 12, 2018

@arcalinea arcalinea requested review from bitcartel and str4d Jul 12, 2018

@arcalinea arcalinea added this to Review Backlog in Zcashd Team Jul 12, 2018

}

return EncodeSpendingKey(sk);
}

This comment has been minimized.

@str4d

str4d Jul 12, 2018

Contributor

Move all this into a boost::static_visitor<SpendingKey>. See #3345 for an example. Then this function would become:

...
    auto sk = boost::apply_visitor(GetSpendingKeyForPaymentAddress(pwalletMain), address);
    return EncodeSpendingKey(sk);
}

It should be fine to throw inside the visitor.

return NullUniValue;
// Sapling support
bool isValidKey = boost::get<libzcash::SproutSpendingKey>(&spendingkey) != nullptr || boost::get<libzcash::SaplingSpendingKey>(&spendingkey) != nullptr;
assert(isValidKey);

This comment has been minimized.

@str4d

str4d Jul 12, 2018

Contributor

There's already an IsValidSpendingKey(spendingkey) check above here, so these two lines can be removed.

if (fRescan) {
pwalletMain->ScanForWalletTransactions(chainActive[nRescanHeight], true);
} else {
auto sk = boost::get<libzcash::SaplingSpendingKey>(spendingkey);

This comment has been minimized.

@str4d

str4d Jul 12, 2018

Contributor

Move these sections into a boost::static_visitor<bool>, with the boolean indicating whether we should return early (from inside the fIgnoreExistingKey check). This function would become:

...
auto keyExists = boost::apply_visitor(AddSpendingKeyToWallet(pwalletMain), spendingkey);
if (keyExists) {
    return NullUniValue;
}
// whenever a key is imported, ...
...
return EncodeSpendingKey(k);
// Sapling support
bool isValidAddr = boost::get<libzcash::SproutPaymentAddress>(&address) != nullptr || boost::get<libzcash::SaplingPaymentAddress>(&address) != nullptr;
assert(isValidAddr);

This comment has been minimized.

@str4d

str4d Jul 12, 2018

Contributor

There's already an IsValidPayamentAddress(address) check above here, so these two lines can be removed.

This comment has been minimized.

@arcalinea

arcalinea Jul 17, 2018

Contributor

But despite that, there was an assert there before that address was not nullptr. Why should we remove it now?

This comment has been minimized.

@str4d

str4d Jul 19, 2018

Contributor

The assert was there specifically so that we would get a crash if someone tried to use a Sapling address before support was added. Now that GetSpendingKeyForPaymentAddress is implemented, we don't need these explicit nullptr checks, because if we ever add a new address type, all the boost::static_visitors will immediately start complaining that they aren't covering all cases 🙂

So please remove these two lines.

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

@arcalinea arcalinea moved this from In Progress to Review Backlog in Zcashd Team Jul 19, 2018

@mdr0id mdr0id moved this from Review Backlog to In Review in Zcashd Team Jul 19, 2018

@str4d
Copy link
Contributor

str4d left a comment

The removal of the nullptr checks is blocking.

throw JSONRPCError(RPC_WALLET_ERROR, "Error adding spending key to wallet");
}

m_wallet->mapSaplingZKeyMetadata[addr].nCreateTime = 1;

This comment has been minimized.

@str4d

str4d Jul 19, 2018

Contributor

Hmm, we should think about how this metadata is used, and whether it should index on addr or fvk.

This comment has been minimized.

@arcalinea

arcalinea Jul 19, 2018

Contributor

Will it still be used on addresses? Not a lot of instances in the code

This comment has been minimized.

@str4d

str4d Jul 25, 2018

Contributor

Okay, this data is stored in order to figure out when rescanning how many blocks can be skipped (that won't contain transactions for any of the known addresses). So I think it makes sense to store SaplingFullViewingKey instead of SaplingPaymentAddress, as fvk uniquely corresponds to the key that will be used to scan (both for incoming transactions, and in future for recovering outgoing transaction data). However, this is entirely an in-memory decision, so we can just make an issue, and handle it later (when we update ScanForWalletTransactions() to catch all Sapling (and Sprout) transactions without needing to reindex).

This comment has been minimized.

@bitcartel

bitcartel Jul 29, 2018

Contributor

Filed issue

}
}

bool operator()(const libzcash::InvalidEncoding& no) const { return true; }

This comment has been minimized.

@str4d

str4d Jul 19, 2018

Contributor

This is probably fine; or you could throw an error here (this case should never be triggered, as we call IsValidSpendingKey() first which ensures that spendingkey is not an InvalidEncoding, so getting here would be an implementation error).

return EncodeSpendingKey(k);
// Sapling support
bool isValidAddr = boost::get<libzcash::SproutPaymentAddress>(&address) != nullptr || boost::get<libzcash::SaplingPaymentAddress>(&address) != nullptr;
assert(isValidAddr);

This comment has been minimized.

@str4d

str4d Jul 19, 2018

Contributor

The assert was there specifically so that we would get a crash if someone tried to use a Sapling address before support was added. Now that GetSpendingKeyForPaymentAddress is implemented, we don't need these explicit nullptr checks, because if we ever add a new address type, all the boost::static_visitors will immediately start complaining that they aren't covering all cases 🙂

So please remove these two lines.

@mdr0id mdr0id requested a review from gtank Jul 23, 2018

@str4d

str4d approved these changes Jul 25, 2018

Copy link
Contributor

str4d left a comment

Please squash the final cleanup commit into the second-to-last commit. Otherwise, ACK 🙂

throw JSONRPCError(RPC_WALLET_ERROR, "Error adding spending key to wallet");
}

m_wallet->mapSaplingZKeyMetadata[addr].nCreateTime = 1;

This comment has been minimized.

@str4d

str4d Jul 25, 2018

Contributor

Okay, this data is stored in order to figure out when rescanning how many blocks can be skipped (that won't contain transactions for any of the known addresses). So I think it makes sense to store SaplingFullViewingKey instead of SaplingPaymentAddress, as fvk uniquely corresponds to the key that will be used to scan (both for incoming transactions, and in future for recovering outgoing transaction data). However, this is entirely an in-memory decision, so we can just make an issue, and handle it later (when we update ScanForWalletTransactions() to catch all Sapling (and Sprout) transactions without needing to reindex).

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Jul 26, 2018

@arcalinea I'm reviewing right now; can you push the cleanup per @str4d's review? Thanks.

@bitcartel bitcartel force-pushed the arcalinea:sapling_z_importexport_keys branch from a728d52 to 501de64 Jul 30, 2018

@bitcartel
Copy link
Contributor

bitcartel left a comment

I squashed the last two commits into one per str4d's review comment, rebased on master (clean) and force pushed.

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Jul 30, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jul 30, 2018

📌 Commit 501de64 has been approved by bitcartel

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jul 30, 2018

⌛️ Testing commit 501de64 with merge 28a8d1d...

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

Auto merge of #3392 - arcalinea:sapling_z_importexport_keys, r=bitcartel
Add Sapling support to z_importkey, z_exportkey

Add Sapling support to `z_importkey` and `z_exportkey`
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jul 30, 2018

☀️ Test successful - pr-merge
Approved by: bitcartel
Pushing 28a8d1d to master...

@zkbot zkbot merged commit 501de64 into zcash:master Jul 30, 2018

1 check passed

homu Test successful
Details

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

@str4d str4d referenced this pull request Jul 30, 2018

Closed

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

4 of 4 tasks complete

@str4d str4d referenced this pull request Jul 30, 2018

Closed

Add Sapling support to wallet RPCs #3063

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