Declare and use NonHardenedChildIndex for transparent

[AArnott]

• Declare NonHardenedChildIndex struct
• Change WalletRead::get_transparent_receivers signature to use the new struct
  It needn't return the account id that was given as an input, and it shouldn't return an 11-byte diversifier index when a 31-bit child index is more appropriate.

[codecov]

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (3cbc481) 65.03% compared to head (f1b6dd0) 65.08%.

Files	Patch %	Lines
zcash_client_sqlite/src/wallet.rs	76.47%	4 Missing ⚠️
zcash_client_backend/src/data_api.rs	60.00%	2 Missing ⚠️
zcash_primitives/src/legacy/keys.rs	71.42%	2 Missing ⚠️
zcash_client_backend/src/data_api/error.rs	0.00%	1 Missing ⚠️
zcash_client_backend/src/data_api/wallet.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1185      +/-   ##
==========================================
+ Coverage   65.03%   65.08%   +0.04%     
==========================================
  Files         114      114              
  Lines       11108    11129      +21     
==========================================
+ Hits         7224     7243      +19     
- Misses       3884     3886       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[daira]

Please address @nuttycom's comment on error handling; otherwise looks good.

[nuttycom]

Overall this looks excellent, thank you very much for the contribution! The only blocking comment is for the error handling in zcash_client_sqlite.

[AArnott]

All comments addressed. Thanks for reviewing.

[nuttycom]

utACK 9f221f8, thank you!

[str4d]

Reviewed as of 9f221f8. The changelog changes are non-blocking (we can fix them in a separate PR before the crate releases, if they aren't fixed here).

[str4d]

If all we wanted to return from this API is the transparent address and the child index it was derived from, then I argue that NonHardenedChildIndex should be the key: it is trivial to derive the address from the full viewing key given the index (and providing the address is thus only an optimization), whereas the reverse requires brute forcing, so logically the child index is the unique key here.

However, given the existing usage of this API, the child index is being treated as metadata, so we should keep AddressMetadata as the return type here. This would allow for e.g. marking whether a receiver is external or internal (currently this API only returns external receivers because zcash_client_backend wallets never create transparent change, but if a third party wallet seed phrase is imported that did, we'd need to support scanning at least the default internal transparent receiver to detect it). If we keep AddressMetadata removed then in future we'd also need to change the map to be HashMap<(zip32::Scope, NonHardenedChildIndex), TransparentAddress>.

Separately, I note that AddressMetadata is currently only used for transparent addresses, and is really a detail of this specific WalletRead method. It should probably not have been moved into zcash_keys, and should be moved back into zcash_client_backend (and maybe renamed to be specific to transparent addresses, since we do not expose internal addresses for shielded keys).

---

After a rather deep rabbit-hole session with @nuttycom, our conclusion is we should:

• Move AddressMetadata back into zcash_client_backend::address.
• Change AddressMetadata to replace diversifier_index with NonHardenedChildIndex, remove account_id, and add zip32::Scope.

These changes don't need to happen in this PR, however this PR should be changed to not remove AddressMetadata from the return type of this method. So if you do want to make the other changes here, you are welcome to.

[nuttycom]

More specifically, we realized as part of the discussion that there is probably an existing bug related to the import of seeds from transparent-only wallets, where we are not at present scanning any of the internal key tree (which is where Bitcoin wallets regularly send change) so if you import a seed that was previously used with a transparent-only wallet, you're likely to be missing funds.

Then again, if you do that, then you're also not doing gap limit scanning at the moment, so both of these should be addressed at once. I'll open an issue for that and it can get stuck in the backlog somewhere.

[AArnott]

AddressMetadata includes DiversifierIndex and the ZIP-32 AccountId. Both are problematic because we may not know the address index at all for a given receiver, and similarly we may not know the AccountId. At least, we won't be guaranteed to know these things after #1175 merges. And it is in preparation for that PR that I created this PR in the first place. Although now that I think about it, I guess we're not even guaranteed to know the NonHardenedChildIndex. So what do we do about a function whose primary purpose (based on its name, anyway) is to produce the transparent addresses for a given account, when it also provides metadata for that address which may or may not be available?
What about changing it to return a vector of tuples, where the first element is the address, and the second element is an Option<T> where T is whatever makes the most sense given what we may know?

[daira]

Is it all of the metadata that may be known or unknown, or is it each field of AddressMetadata somewhat independently? If the former then use Option<AddressMetadata>, otherwise make the relevant accessors on AddressMetadata return Options.

[AArnott]

Addressed. Please take another look.
I left the key=value order as it was because in the future when we support importing transparent private keys, we won't have derivation data.

[str4d]

Although now that I think about it, I guess we're not even guaranteed to know the NonHardenedChildIndex.

If we can't even guarantee that, then we cannot spend any funds received to that address, because:

I left the key=value order as it was because in the future when we support importing transparent private keys, we won't have derivation data.

We do not import raw transparent spending keys; everything needs to be at a minimum tied back to a transparent full viewing key (which we define in ZIP 316 as a particular node in the BIP 44 key derivation path).

Importing arbitrary non-HD transparent private keys will break a bunch of assumptions the current wallet architecture makes, and no one has committed to supporting and maintaining that use case in the long term. From a user perspective, it is only necessary in the context of a full node wallet that explicitly supports importing an arbitrary zcashd wallet, and even in that case we should preferably instead have a migration pathway away from those transparent keys. Transparent-only mobile wallets have for basically all of Zcash's existence used HD-derived keys.

[str4d]

While I am strongly opposed to trait changes that point us in the direction of adding support for long-since-deprecated zcashd behaviours, I am not going to block this PR being merged. I have not fully re-reviewed the PR after the recent changes, but the trait returning Option<Metadata> is the least intrusive thing that could be done, and I will almost certainly just be .unwrap()ing everywhere downstream.

[daira]

9f221f8 looks good but @nuttycom and @str4d's other comments are blocking.

[nuttycom]

utACK f1b6dd0

It turns out that I need this to address problems in #1187 so I have marked this as blocking that PR.

[nuttycom]

@AArnott rustfmt is failing; I attempted to push the fix to your branch but it looks like you don't allow edits on the nerdcash branches from upstream contributors.

Looks at a glance like my changes have generally been addressed, but I have not re-reviewed the PR.

[nuttycom]

I will fix the formatting errors in #1187