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

Generate an ovk to encrypt outCiphertext for t-addr senders #3516

Merged
merged 2 commits into from Sep 19, 2018

Conversation

@str4d
Copy link
Contributor

str4d commented Sep 13, 2018

Closes #3506.

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

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

@str4d str4d added this to In Review in Zcashd Team Sep 13, 2018

@str4d str4d added the Sapling label Sep 13, 2018

RPC_WALLET_ERROR,
"CWallet::GenerateNewSaplingZKey(): HD seed not found");

auto m = libzcash::SaplingExtendedSpendingKey::Master(seed);

This comment has been minimized.

@arielgabizon

arielgabizon Sep 16, 2018

Contributor

Is there some code duplication here? Guessing there's somewhere in the code where we derive the 0 account. Also perhaps add a reference to zip 32/short explanation to what account 0 means; btw also in zip 32 you currently don't use this terminology - that account i means m_Sapling / purpose' / coin_type'/i. Perhaps add it there, i.e. add around here https://github.com/zcash/zips/pull/157/files#diff-395ced896b8bf805b190247312c2fc4eR334 the sentence 'We sometimes informally refer to the path m_Sapling / purpose' / coin_type'/i as the i'th account'

This comment has been minimized.

@arielgabizon

arielgabizon Sep 16, 2018

Contributor

It seems the DefaultAddress function from zip32.cpp should be useful here, but it seems to return the first address at the root, not following this path, which might be inconsistent with the zip?

This comment has been minimized.

@str4d

str4d Sep 17, 2018

Contributor

There is kind-of duplication here from the key generation code, but not quite because here we know exactly which account we want, whereas there we are searching for an unused one.

We define account in ZIP 32 here to have the same interpretation as in BIP 44.

DefaultAddress() (both in the implementation and the ZIP) refers to the default diversified PaymentAddress for a given extended key (which could be anywhere along any path), not the default account for the specific ZIP 32 Sapling path.

@ebfull

This comment has been minimized.

Copy link
Contributor

ebfull commented Sep 17, 2018

I mentioned that I think we should use ephemeral randomness until we've had time to specify this. I think someone should call the shots on that decision.

I'll review this PR as though we've decided to use this approach anyway.

@ebfull

ebfull approved these changes Sep 17, 2018

Copy link
Contributor

ebfull left a comment

LGTM though someone needs to decide if this is the correct action to take, as I mentioned in another comment.

@bitcartel
Copy link
Contributor

bitcartel left a comment

I propose modifying the PR to use a hash of the seed:

  1. preserves privacy unlike all zero key
  2. no loss of data unlike random key
  3. avoids potential ux issues with HD wallets detecting the t->z txs when given fvk of account 0).

Also needs a test to verify the ovk can decrypt.

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Sep 18, 2018

We've decided to switch to @bitcartel's strategy. We will take the HD seed, compute:

I = BLAKE2b-512("ZcTaddrToSapling", *seed*)

and then compute ovk = truncate_32(PRF_expand(I[0..32], 0x02)) (same as in ZIP 32 and the Sapling spec).

@ebfull

This comment has been minimized.

Copy link
Contributor

ebfull commented Sep 18, 2018

Title of PR should change.

str4d added some commits Sep 18, 2018

@str4d str4d force-pushed the str4d:3506-sendmany-sapling-t-ovk branch from a114897 to bb4b698 Sep 18, 2018

@str4d str4d changed the title Use Sapling account 0 FVK to encrypt outCiphertext for t-addr senders Generate an ovk to encrypt outCiphertext for t-addr senders Sep 18, 2018

@str4d str4d requested review from bitcartel and ebfull Sep 18, 2018

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Sep 18, 2018

Force-pushed to implement the above strategy and add a unit test.

@bitcartel
Copy link
Contributor

bitcartel left a comment

utACK. Looks good.

Suggested improvement:

Let the salt be an optional value in the config file / runtime param, where the default is null.

One use case might be that the wallet owner for some reason wants to intentionally lose the data. For example, maybe they are sharing the hd seed with someone or importing it into another wallet (e.g. online wallet) and they don't want the taddr history to be revealed / reconstructed.

@ebfull

ebfull approved these changes Sep 19, 2018

Copy link
Contributor

ebfull left a comment

Great!

@ebfull

This comment has been minimized.

Copy link
Contributor

ebfull commented Sep 19, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 19, 2018

📌 Commit bb4b698 has been approved by ebfull

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 19, 2018

⌛️ Testing commit bb4b698 with merge 4fc1066...

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

Auto merge of #3516 - str4d:3506-sendmany-sapling-t-ovk, r=ebfull
Generate an ovk to encrypt outCiphertext for t-addr senders

Closes #3506.
@daira

This comment has been minimized.

Copy link
Contributor

daira commented Sep 19, 2018

t-addresses are designed to be ephemeral and numerous. So in practice all this does is make viewing keys less useful and less efficient, by forcing a wallet that wants to present a view of outgoing transactions from t-addresses, to scan the relevant range of transactions for every address (except that in practice this doesn't scale so they'll avoid using the ovk encryption for this at all, at the expense of having to scan more of the block chain).

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 19, 2018

☀️ Test successful - pr-merge
Approved by: ebfull
Pushing 4fc1066 to master...

@zkbot zkbot merged commit bb4b698 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

@str4d str4d deleted the str4d:3506-sendmany-sapling-t-ovk branch Sep 19, 2018

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Sep 19, 2018

@daira I don't understand your comment:

  • A Sapling viewing key shows transactions being received by or sent from a Sapling payment address. This ovk is for Sapling transactions being sent from a t-address. The latter does not interact with the former in any way (except where the t-address sends to a wallet's Sapling address, in which case the user has already detected the transaction so no scanning is required), and thus does not affect the former's usefulness or efficiency.
  • A single ovk is used for all Sapling outputs from any t-address in the wallet, so there is no O(n * m) scanning problem (at the cost of t-address operations being linkable if you know this ovk or the seed, but we already assume that t-address operations within a wallet are linkable).
  • A wallet that wants to present a view of outgoing Sapling transactions from t-addresses (as in showing the recipient information) needs to trial-decrypt every outCiphertext from a t-address it holds by definition. If the wallet only wants to present a view of the transactions involving the t-address, then it won't do any decryptions at all because it is choosing to not show the Sapling recipient data.
@daira

This comment has been minimized.

Copy link
Contributor

daira commented Oct 2, 2018

A single ovk is used for all Sapling outputs from any t-address in the wallet [...]

OK, I misunderstood this. This addresses my objection.

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