Giter Club home page Giter Club logo

Comments (6)

AArnott avatar AArnott commented on September 28, 2024

Given the scanning concern applies only to transparent addresses, @daira argument's argument on discord seems to focus on agreeing on a single transparent address to scan, but this seems wrong. ZIP-316 does not in any way insist that a wallet scan T-index 0 or some other number to match sapling. It only insists that if a UA includes both sapling and transparent receivers, that they use the same index. If a wallet never created such a UA, then there is no requirement in ZIP-316 that the wallet scan anything other than T-index 0, which is the same index that all ZIP-32/BIP-32 compliant wallets have scanned since the beginning. ZIP-316 didn't change that -- if it had caused apps to switch from scanning T-index 0 to T-index = default sapling, that would have caused users to lose their T funds.
So what did ZIP-316 accomplish by requiring indexes to match? It very subtly added a requirement for all wallets to have potentially two T addresses to scan instead of one. And all of them must do this regardless of their own individual adherence to the matching index policy in order to ensure users have access to their T funds.
IMO this is the requirement that should be expressed as MUST: Wallets MUST scan both T-index 0 and (if different) the T-index that matches the sapling default index.
The formulation of UAs with matching indexes seems immaterial after that.

from zips.

daira avatar daira commented on September 28, 2024

Oops, I did not intend to edit the description, only to reply to it. I'll fix that.

from zips.

daira avatar daira commented on September 28, 2024

Regarding:

zips/zip-0316.rst

Lines 690 to 692 in fee271c

To derive a Unified Address from a UIVK we need to choose a diversifier
index, which MUST be valid for all of the Viewing Key Types in the
UIVK. That is,

I propose we change the wording to allow UAs to be formed with distinct diversifier indexes per receiver, at the app's discretion. Specifically, here is the proposed wording:

To derive a Unified Address from a UIVK we need to choose one or more diversifier indexes, which MUST be valid for the Viewing Key Types in the UIVK to which they are applied.

And also add:

Because some wallets present their default UA with a transparent receiver whose address index matches the default sapling diversifier index, all wallets MUST scan transparent addresses at address index 0 as well as at the address index matching the default sapling receiver's diversifier index.

I agree with the second change but not the first. Actually, since the second is uncontroversial and the first very controversial, can we split them into separate tickets please?

While it isn't related to ZIP-316, we might also want to provide the derivation path used by zcashd originally so that other wallets can recover transparent funds collected from that.

Yes that's fine as well.
 
As I said on Discord, the purpose of the MUST-use-same-index requirement is to allow the full UA to be reconstructed from any of its constituent Receivers (in the case where there is no metadata and when the set of Receiver types is known). You can argue that you don't think that's important, but it was a design requirement.

  • Transparent address indexes are limited to 4 bytes. Diversifiers have 11 bytes available. By not tightly coupling these, we open up an additional 7 bytes in the 11 byte diversifier.
    This is particularly useful considering that wallets that start their diversified addresses at index default+1 and increment by ~1 at a time are guaranteed to produce non-unique addresses when the seed phrase is used multiple times (either across wallets or to restore a lost wallet).

That's not true. If you scan the chain fully before generating a new address, you can avoid producing non-unique addresses (also this issue only applies to wallets using timestamps if they include a transparent receiver). I agree that it might be easier to do that if the indices were not required to be synchronized.

  • Transparent address indexes are strongly encouraged to observe a 40 address 'gap limit'. This allows account recovery to discover all the transparent funds in an account. Also: at present at least, I've never seen an app or web service that accepts a UA and uses the transparent receiver in it. Take these two facts together, and there's a strong desire to not include transparent receivers in UAs by default in a wallet app, since it would be difficult to honor the 40 address gap limit and still produce unique addresses for each use, given almost every sender will avoid using the T address.

Yes and that's conformant.

  • Wallets that only consider one transparent address may continue to do so after evolving to support UAs. Before this MUST language, wallets would use address index 0 for the transparent address. But with the MUST language, a UA with a non-zero default sapling index would force a wallet to support receiving transparent funds at multiple addresses (0 for historical reasons, and whatever the sapling index is).

Right, and that is necessary for seed phrase portability which is a design goal. (Wallets that only scan at the default address will never be able to find all of the funds from a wallet that uses diversified addresses, but we at least want the former kind of wallet to be seed-portable between each other.)

  • In the future, who knows but that another shielded pool may have invalid indexes again

That is extremely unlikely. There are efficient total hash-to-curve algorithms from all curve types that are likely to be used in future. A non-elliptic-curve-based shielded protocol would also have no reason for such a restriction. Invalid indices are very much a Sapling-specific issue.

  • There is already inconsistency in the ecosystem in generation of the default UA. YWallet (for example) is a very popular wallet, and uses index 0 for orchard and transparent receivers in its default UA, regardless of the index for sapling. Zcashd and the zcash crates (as far as they support orchard) use the same index across all receivers when generating a UA. So users who take a seed phrase to various wallet types are already going to have to deal with seeing different UAs as their default receiving address. If YWallet were to change to use the same index across all receivers, user who didn't switch wallets would see a new default address, which could be alarming.

This is probably the best argument. But why should other wallets be different from each other just because YWallet ignored the requirement? YWallet could easily follow the requirement for new wallets.

Arguments against

  • A consistent diversifier index in all receivers allows an app to reconstruct the UA for user viewing.
    • Counter-argument: When is this useful? When does that get displayed to the user for a historical transaction? The only UA we're talking about is the wallet's own receiving UA, which is likely to fall into one of two buckets: an app with only one receiver address, or an app that leverages diversified addresses. If one address, history would just show the same address for every incoming transaction, so that's just a waste of pixels and clutters the UI. If diversified addresses are used, there is no way that the address used to receive the funds will be recognizable to the user, so reconstructing the UA from the one receiver the funds came in from isn't going to help the user. This also depends on the app being able to correctly determine which receivers were included in the original UA. Even if the app went to the extra steps to base it on the block/era of the transaction, it would be based on an assumption that every receiver possible for that era was included, which isn't always the case, due at least to apps that either offer UAs w/o transparent receivers or allow the user to pick and choose which to include.

I don't see that UAs without transparent receivers affect this if the behaviour is consistent. Yes, apps that allow the user to pick and choose on an individual UA basis won't have this information if they do a same-app recovery from seed.

  • The original ZIP-316 text should remain so that all wallets scan the same transparent address
    • Counter-argument: But ZIP-316 does not accomplish this goal. Before ZIP-316, roughly all wallets used T-index 0 as the default transparent address. We can't stop scanning that address just because ZIP-316 came along. So by mandating in ZIP-316 that the default transparent receiver change from 0 to match the sapling DI, we've added a new T address that must be scanned. This doesn't feel like a win to me.

We already have wallets that followed the spec and require to scan with that default DI. If we want seed phrase portability, then all wallets have to scan it, regardless of whether they also scan index 0.

Further, ZIP-316 doesn't mandate anything about which receivers the default UA include, and at least two wallets don't include T receivers in their UAs, so ZIP-316 doesn't even lead a wallet dev to believe they have anything other than T-index 0 to scan.

That should be fixed (and it's independent of the MUST).

from zips.

AArnott avatar AArnott commented on September 28, 2024

can we split them into separate tickets please?

Sure. I'll split them up.

You can argue that you don't think that's important, but it was a design requirement.

I do, in fact, argue that this is not important. And if it's a design requirement, it sounds like a requirement that an app might make to allow them to reconstruct UAs (again, I don't think that's useful for any app). But the point I'm making is this doesn't belong in a protocol ZIP because it's strictly about some subset of people's UI goal, and caused the problem of forcing apps to support multiple transparent addresses where they had no need to before.

Wallets that only scan at the default address will never be able to find all of the funds from a wallet that uses diversified addresses,

Sure they can, at least, if you consider 'diversified addresses' to be limited to shielded pools. Diversified addresses don't change anything about discoverability of shielded funds. A wallet that only supports displaying one sapling address for example (like Unstoppable) can still discover all funds sent to sapling diversified addresses for that same account.

But when we consider transparent addresses, such a wallet would not find funds sent to non-zero indexed addresses. So a 'conforming' wallet whose default address is a UA with a transparent receiver at a non-zero index is causing funds to be sent to an address that other wallets will not see. But this was an unnecessary incompatibility due to ZIP-316 conformance.

That's not true. If you scan the chain fully before generating a new address, you can avoid producing non-unique addresses

Scanning the chain cannot prevent producing non-unique addresses. Many 'unique' addresses may not have been used (yet) on the chain, in which case the wallet wouldn't observe them. It would be incorrect and unsafe to assume just because they haven't been sent funds that they also were never shown to anyone.

Invalid indices are very much a Sapling-specific issue.

I'm delighted to hear it.

This is probably the best argument. But why should other wallets be different from each other just because YWallet ignored the requirement? YWallet could easily follow the requirement for new wallets.

Maybe a survey to see where the most users are. If YWallet has most users today, IMO that holds weight for pulling the fewer set of users over.
That, and my belief that this was a bad requirement from the beginning (because again, it shifts existing T-indexes from 0 to another one, without calling that out as a breaking change that requires more features like scanning multiple T addresses), suggests to me we should just remove the poorly documented requirement, and replace it with a callout that now we have 2-3 T addresses we need to scan for bumpy history reasons.

from zips.

AArnott avatar AArnott commented on September 28, 2024

We already have wallets that followed the spec and require to scan with that default DI. If we want seed phrase portability, then all wallets have to scan it, regardless of whether they also scan index 0.

All wallets should always scan index 0 for T addresses, do we agree on that?
ZIP-316 created a new requirement that they must also scan another index, if sapling=0 was invalid. Can we also agree on that?

from zips.

AArnott avatar AArnott commented on September 28, 2024

#772 covers the callout that wallet apps should scan multiple T addresses in order to handle incoming funds sent to the various T receiving addresses that various wallets presented to users over time.

from zips.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.