Skip to content
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 out_ciphertext to CompactOutput as optional field #351

Open
adityapk00 opened this issue May 5, 2021 · 6 comments
Open

Add out_ciphertext to CompactOutput as optional field #351

adityapk00 opened this issue May 5, 2021 · 6 comments
Assignees

Comments

@adityapk00
Copy link
Contributor

What is your feature request?
The CompactOutput returned by LightwalletD today doesn't have enough information to do a trial decryption of a sapling CompactOutput with an OutgoingViewKey. Specifically, it is missing the out_ciphertext. Please see: zcash/librustzcash#381

Note that out_ciphertext is 80 bytes, so this is some overhead.

It would be nice if Light clients could optionally request this field during GetBlockRange and GetBlock.

How would this feature help you?
This will allow Light wallets that are scanning backwards (i.e., sync that sees spends before receives) to detect outgoing transactions reliably, significantly improving sync speed.

@adityapk00
Copy link
Contributor Author

cc: @LarryRuane @gmale @braddmiller

@gmale
Copy link
Contributor

gmale commented May 5, 2021

Right now, whenever the wallet finds a compact transaction that relates to it, it requests the related full transaction. One of the reasons is to get the memo but another major reason was to find outbound transaction information. Otherwise, on restore a wallet only shows inbound transactions. It would also miss transactions sent with that same seed from another device.

I believe that if we added this out_ciphertext it would reduce some of the need to fetch full transactions at the expense of 80 bytes per output for all transactions in every block. At 15 tx per block and 2 outputs per tx that's rougly 2.8MB per day and 83MB per month.

To me that's reasonable. Especially if it enables significantly faster scanning.

@LarryRuane
Copy link
Collaborator

One slight complication with this is that the compact block cache (used to be in-memory, now on-disk) will have to be rebuilt. That is, lightwalletd will have to resync from Sapling activation height, from zcashd. Last I checked, this took about 40 minutes on mainnet. Not a big deal, but just something to keep in mind. Also, we'll have to make sure that we detect that this needs to be done (on first startup after upgrade), without (obviously) doing this after just the first time.

@defuse
Copy link
Collaborator

defuse commented May 13, 2021

My only remarks from a security point of view is that it adds complexity to lightwalletd, and if it's optional, as @str4d points out in the other ticket, network adversaries can distinguish between requests that ask for the output ciphertexts and those that don't. As far as I can tell that won't actually change anything in the privacy threat model (i.e. it doesn't enable any attacks that I can see), so no objections from me if we're fine with the additional bandwidth usage and the added code complexity in lightwalletd (which is probably nontrivial if it's optional).

@str4d
Copy link
Collaborator

str4d commented May 13, 2021

It's also worth noting that in NU5 we're adding the ability to authenticate CompactBlock data, and this new field won't be included in that. An adversary can't provide an invalid or malicious outCiphertext that they want the client to treat as valid, because the note commitment check of the resulting decrypted note still fail. But they can provide an invalid outCiphertext in place of what would have been a valid one, to undetectably (at the time, detectable in forensics while the block cache exists or if the full transaction is later fetched) prevent the client from decrypting an output.

@adityapk00
Copy link
Contributor Author

Update: This is now lower priority for Zecwallet, since we'll use nullifiers to detect outgoing transactions even for BlazeSync, so that part of the usecase no longer applies.

However, it would be a nice-to-have, in case we wanted to add a feature where users could turn off fetching "full transactions" to improve privacy. In that case, this would allow decoding the outgoing tx amount and address just from the CompactBlock.

@LarryRuane LarryRuane self-assigned this Sep 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants